feat(ui): add visual indicators for hook execution#15408
Conversation
Summary of ChangesHello @abhipatel12, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user experience by providing immediate visual feedback in the Terminal User Interface (TUI) when background hooks are actively executing. It integrates new event emissions from the core hook logic into the UI's state management, allowing for a dynamic display that informs users about ongoing processes. This feature is also made configurable, giving users control over its visibility. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Size Change: +5.64 kB (+0.03%) Total Size: 22.2 MB
ℹ️ View Unchanged
|
![gemini-code-assist[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a transient visual indicator for hook execution, which is a great enhancement for user feedback. The implementation is solid, using useReducer for robust state management of active hooks and correctly handling concurrent identical hooks. The new feature is also made configurable with clear documentation and schema updates. The accompanying tests are thorough and cover the new logic well. Overall, this is a high-quality contribution.
|
Consider my approval for the non-UX/UI bits. Should still get sign-off from someone else on the UX fronts |
|
I think we should align this further with the existing infrastructure for showing messages like "Press ctrl-C again to exit" |
| }, | ||
| }; | ||
| const { lastFrame, unmount } = renderWithWidth(120, props); | ||
| expect(lastFrame()).toMatchSnapshot(); |
There was a problem hiding this comment.
the previous test was more robust as it would fail if the snapshot was wrong. with this change someone could commit a bad snapshot.
| const startTimes = hookStartTimes.current; | ||
|
|
||
| const handleHookStart = (payload: HookStartPayload) => { | ||
| const key = `${payload.hookName}:${payload.eventName}`; |
There was a problem hiding this comment.
this behavior seems a little suspcious if there are multiple nested hooks with the same hookName and eventName. Don't we at least need a count of how many hooks are active with the same key so we don't close the outer hook just because the innder one has ended.
There was a problem hiding this comment.
This should work as intended. Since we're splicing the index for the first instance, even if we had a hook with the same name/event, we would only be removing one at a time.
I added a test based on another comment that should also verify this.
Lmk if that looks okay!
There was a problem hiding this comment.
as a followup we need to cleanup uiState.. All these items are just for displaying the status messages so should be part of separate provider.
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
Looks good. Approved once these comments are addressed
There was a problem hiding this comment.
move this all to a separate StatusDisplay component. This list is becoming a tragedy of the commons and isn't related to Composer.
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
A couple other pieces of feedback from Gemini CLI /review-frontend that I generally agree with.
useStatevs.useReducer: While the PR description claims to use useReducer for robust handling of rapid-fire
events, the implementation in useHookDisplayState.ts actually uses useState with functional updates. Given the
asynchronous nature of hook start/end events and the setTimeout logic, a reducer might be more idiomatic and robust
here.- Non-Null Assertion: There is a non-null assertion (!) in useHookDisplayState.ts (line 40:
startTimes.get(key)!.push(now);). Although it follows a has check, we typically prefer to avoid these.
abhipatel12
left a comment
abhipatel12
left a comment
There was a problem hiding this comment.
Thanks for the review! Let me know what you think!
| }, | ||
| }; | ||
| const { lastFrame, unmount } = renderWithWidth(120, props); | ||
| expect(lastFrame()).toMatchSnapshot(); |
| const startTimes = hookStartTimes.current; | ||
|
|
||
| const handleHookStart = (payload: HookStartPayload) => { | ||
| const key = `${payload.hookName}:${payload.eventName}`; |
There was a problem hiding this comment.
This should work as intended. Since we're splicing the index for the first instance, even if we had a hook with the same name/event, we would only be removing one at a time.
I added a test based on another comment that should also verify this.
Lmk if that looks okay!
Align hook execution display with notification infrastructure: - Implement `useHookDisplayState` with 1s minimum display duration - Replace context summary with hook status when hooks are active - Remove hook emoji per feedback - Add snapshots for UI components
|
Thanks for the reviews! |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
Summary
Adds a transient visual indicator (
🪝 Executing Hook(s)) to the TUI context line to provide real-time feedback during hook execution.Details
HookStartandHookEndevents toCoreEventEmitterand instrumentedHookRunner/HookEventHandlerto emit them during the hook lifecycle.AppContainerto trackactiveHooksusinguseReducer, ensuring robust handling of rapid-fire events.🪝 Executing Hook(s)display inContextSummaryDisplaywith support for pluralization and terminal-width-aware truncation (e.g.,... (+N more)).hooks.notificationssetting (default:true) in the 'Advanced' category to toggle visibility.configuration.mdandsettings.schema.jsonto include the new setting.Demo:
Screen.Recording.2025-12-21.at.6.15.03.PM.mov
Related Issues
Resolves #15262
How to Validate
settings.json(e.g., aSessionStarthook with a sleep).🪝 Executing Hook: <name>indicator appearing above the input bar during startup.BeforeAgent) and verify the pluralization and truncation logic in narrow terminal windows./settingsand toggleHook Notificationsto verify the UI correctly respects the setting.npx vitest packages/cli/src/ui/AppContainer.test.tsx packages/cli/src/ui/components/ContextSummaryDisplay.test.tsxPre-Merge Checklist