style(cli) : Dialog pattern for /hooks Command#17930
style(cli) : Dialog pattern for /hooks Command#17930abhipatel12 merged 6 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @AbdulTawabJuly, 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 refactors the Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request is a nice UI improvement, refactoring the /hooks command to use a modal dialog, which aligns its behavior with other commands like /settings and reduces noise in the chat history. The implementation of the new HooksDialog component is well-done, including comprehensive tests for its functionality and scrolling behavior. I've identified a couple of areas for improvement: one is a minor bug in the new dialog related to React keys that could cause rendering issues, and the other is some leftover code from the old implementation that should be removed to keep the codebase clean. Overall, great work on improving the user experience.
|
Hi @abhipatel12, I've created a fresh PR for the issue Hooks: Dialog pattern for /hooks Command and closed the old one due to excessive conflicts. I've addressed all the requirements from the ticket, plus a few extras. Could you please take a look soon to prevent new conflicts from arising? |
|
Hi @abhipatel12, It’s been some time since I opened this PR. Could you please take a look and review it when you get a chance? |
5b10dac to
8925f0e
Compare
8925f0e to
eaa80bd
Compare
|
Hi @abhipatel12, I apologize for reaching out repeatedly. I just wanted to check if this PR is still relevant or not, especially since the issue is now marked as I appreciate your time, and I’m sorry for any inconvenience. It’s been quite a while since I first opened this PR (and later reopened it), so I wanted to clarify before taking any action. |
eaa80bd to
5811a78
Compare
b32c01b to
cdc61a0
Compare
abhipatel12
left a comment
There was a problem hiding this comment.
Hey! This looks great and sorry for the delayed review. If you can take a look at the 3 comments i left, i can stamp and get this merged.
Thanks again!
|
@abhipatel12 thank you for your response! I’ve implemented your suggested changes. Please let me know if you’d like any further improvements. |
Hey thanks for the quick response! I dont see the snapshots file. Did the snapshots actually get updated! |
|
Looks like the tests are failing, so you may need to run the command to update snapshots |
@abhipatel12 I ran the test again using
|
|
I noticed that the CI pipeline is failing due to an issue with the Evals test. However, my changes are unrelated to the Evals functionality. |
|
A little update The failure is due to Gemini API connectivity issues in the runner (HTTP 503 "high demand" errors and HeadersTimeoutError timeouts), not related to my changes. |
abhipatel12
left a comment
There was a problem hiding this comment.
LGTM, thanks for your work on this!
I know the CI is having trouble, but no worries. We're figuring that out, but ill have this merged as soon as possible.
Thank you for your patience with this PR, it’s been a while since I opened it, and I truly appreciate you reviewing it. If there’s anything else you’d like me to work on, especially around the Hooks Post V1 features (like Interactive Hook Configuration GUI), I’d be happy to help. Those were some of my favorite issues to work on! |
Summary
Convert the
/hookscommand output into a modalHooksDialog(using the existingcustom_dialogpattern) and align its UI and behavior with other dialogs such as/settings. This reduces chat-history noise, improves visual consistency, fixes a border alignment bug, and satisfies lint/style requirements.Changes
custom_dialogfor/hooksinstead of writing to chat history (ephemeral, ESC to close).Hooks Panel (1-8 of X)) to matchBaseSettingsDialogvisual style.▲ more above/▼ more belowto▲/▼to match/settings.paddingLeft={2}and prevent indicator boxes from expanding withminWidth={0}— fixes right-border misalignment.eventNameinto a display array (headers + hooks) so scrolling uses a simple slice of the flattened list.ReadonlyArray<HookEntry>withreadonly HookEntry[]andArray<HookEntry>withHookEntry[]to satisfy project linting rules.▲/▼indicators and scrolling behavior).Related Issues
#15273
Testing
/aboutand/settingscommandsPre-Merge Checklist