refactor(cli): keyboard handling and AskUserDialog #17414
Conversation
Summary of ChangesHello @jacob314, 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 significantly refactors and enhances the user experience for CLI dialogs, particularly focusing on keyboard navigation and input handling. By introducing new dialog navigation commands, centralizing tab-based navigation with a dedicated hook, and implementing a priority system for keypress events, it addresses issues related to event bubbling and ensures a more intuitive and consistent interaction model. The changes also include UI improvements for keyboard hints and accessibility. 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
|
|
Size Change: +8.92 kB (+0.04%) Total Size: 23.5 MB
ℹ️ View Unchanged
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring of the keypress handling system within the CLI UI. It introduces a priority-based subscription model that allows keypress handlers to consume events, preventing them from bubbling up. This effectively resolves issues where global navigation shortcuts could be unintentionally triggered from within text input fields. The changes are extensive, touching numerous components and hooks, but are implemented consistently. The addition of a new regression test to verify the fix is a great touch. I've identified one high-severity issue where a component prop was being ignored, which could lead to UI layout inconsistencies.
gemini-cli
bot
added
the
priority/p1
jacob314
force-pushed
the
ask_question_cleanup
branch
from
8f93a9c to
df37466
Compare
Detailed Code Review for PR 17414Overall, this is a high-quality PR that significantly improves the CLI's internal event plumbing. |
|
|
||
| const handleInput = useCallback( | ||
| (key: Key): void => { | ||
| (key: Key): boolean => { |
There was a problem hiding this comment.
these changes are all trivial. We return true after each key we handled and false for ones where the key didn't cause an action.
| }); | ||
|
|
||
| writeKey(stdin, '\x1b[D'); // Left arrow should work when NOT focusing a text input | ||
| // Wait, Async question is a CHOICE question, so Left arrow SHOULD work. |
There was a problem hiding this comment.
cleanup of no-op comment
gemini-cli
bot
added
the
status/need-issue
jackwotherspoon
changed the title
|
Hi there! Thank you for your contribution to Gemini CLI. To improve our contribution process and better track changes, we now require all pull requests to be associated with an existing issue, as announced in our recent discussion and as detailed in our CONTRIBUTING.md. This pull request is being closed because it is not currently linked to an issue. Once you have updated the description of this PR to link an issue (e.g., by adding How to link an issue: Thank you for your understanding and for being a part of our community! |
4 participants
Summary
Change is smaller than it looks.
Most of the changes are trivial boilerplate returning true for key events that were handled and false for ones that weren't.
This let us simplify the way we handle when multiple components handle the same events.
Details
Summary of Changes
This PR performs a systemic overhaul of keyboard handling in Gemini CLI, transitioning to a priority-based bubbling system. It also refactors
AskUserDialogfor better consistency and maintainability.Intent and PR Hygiene
refactor(cli): overhaul keyboard handling and AskUserDialog).Architecture and Keyboard Handling
boolean | voidforKeypressHandleris an excellent architectural improvement. It allows components to explicitly signal whether they've handled an event, enabling clean bubbling.prioritySubscribersandnormalSubscribersinKeypressContextcorrectly uses a stack-like behavior (reverse iteration), ensuring that the most recently mounted components (like dialogs or overlays) get first priority.AppContainer(persistent root) registers first and thus runs last in the normal priority queue, allowing dynamic overlays to correctly intercept global shortcuts.Refactoring and UI/UX
useTabbedNavigationand shared components is a great reduction in complexity.aria-role="tablist"andaria-labelin the newAskUserDialogstructure.Technical Issues and Suggestions
TextBuffer: Inpackages/cli/src/ui/components/shared/text-buffer.ts, theTextBufferinterface still defineshandleInput: (key: Key) => void;. This should be updated tobooleanto match the implementation.ChoiceQuestionView,TextQuestionView, andReviewVieware now shrinking to fit their content width instead of taking the full available width.width="100%"to the outerBoxof these components to ensure they occupy theavailableWidthprovided byAskUserDialogand maintain visual stability.←and│inTabHeader.tsxis fine for this codebase, but ensure your editor is configured for UTF-8 to avoid accidental corruption.Tests Audit
waitForfromtest-utils/async.jsinstead ofvi.waitFor.BubblingRegression.test.tsxis a high-value addition that prevents future regressions in the new bubbling logic.SettingsDialogwere verified as consequences of the shared component updates.The aria tweaks aren't really needed but are harmless and could help in the future.