Centralize Key Binding Logic and Refactor#1969
Centralize Key Binding Logic and Refactor#1969Lee-WonJun wants to merge 5 commits intogoogle-gemini:mainfrom
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| // Ctrl+L (Clear Screen) | ||
| if (key.ctrl && input === 'l') { | ||
| if (keyMatchers.clearScreen(key, input)) { | ||
| onClearScreen(); | ||
| return; | ||
| } | ||
| // Ctrl+P (History Up) | ||
| if (key.ctrl && input === 'p' && !completion.showSuggestions) { | ||
| if (keyMatchers.historyUp(key, input) && !completion.showSuggestions) { | ||
| inputHistory.navigateUp(); | ||
| return; | ||
| } | ||
| // Ctrl+N (History Down) | ||
| if (key.ctrl && input === 'n' && !completion.showSuggestions) { | ||
| if (keyMatchers.historyDown(key, input) && !completion.showSuggestions) { | ||
| inputHistory.navigateDown(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The duplicated key binding handling logic for clearScreen, historyUp, and historyDown in the useInput hook introduces a potential inconsistency. If a key binding's behavior needs modification, changes in one location might be missed in the other, leading to unexpected behavior. Refactor useInput to consolidate these handlers into a single location to ensure consistent behavior. This is a high severity issue because inconsistent keybindings would be a confusing user experience.
gemini-cli
bot
added
kind/documentation
priority/p2
|
Hi @Lee-WonJun I'm going back through some stale pR's. I love the refactoring here and think it lceans things up nicely. That said there are a good number of merge conflicts and some new functionaly that needs included. I'm going to close this out but please feel free to reoopen and address the conflicts and tag me if you'd like to move it forward. Either way thanks for the contribution. |
|
I think it would be better to implement this in a separate PR. Also, from what I briefly saw, it looks like there's already an epic issue related to the config component: #5153. So I assume it's already being worked on elsewhere. Thank you for saying you like my PR style — I appreciate it. Anyway, I'll try to create a separate PR myself sometime soon. |
|
Thanks for the update, @Lee-WonJun. It's understandable that addressing merge conflicts and new functionality might be better suited for a new PR, especially with an existing epic issue. Your refactoring work here is indeed valuable and sets a great foundation. Please feel free to tag me if you create a new PR, and I'll be happy to review it. |
3 participants
TLDR
Centralized key binding logic into a single place to improve maintainability and consistency. Hardcoded key combinations like
Ctrl+LandCtrl+Phave been moved to thekeyMatchersobject inkeyBindings.ts. This lays the groundwork for future support for user-defined key bindings.Dive Deeper
Previously, key bindings were scattered across different files like
App.tsxandInputPrompt.tsx, making them hard to manage. This change introduces:keyBindings.tsunderkeyMatchersshowErrorDetails,toggleToolDescriptions)A few exceptions remain:
Ctrl+XandCtrl+Einteractions with external editors involve complex logic, so the original code is left untouchedtext-bufferare also left unchanged, as improvements have already been made by the gemini-cli team (e.g., handlingdelvsbackspace)Reviewer Test Plan
Confirm that all previous key bindings still work as expected.
Testing Matrix
Linked issues / bugs
#1950
This PR presents a sample / illustrative approach to centralizing key bindings.
It’s not meant to dictate the only possible solution—if the original structure (e.g., keeping bindings directly in terminal-style components) was intentionally chosen, please feel free to point that out.