fix(cli): improve focus navigation for interactive and background shells#18343
fix(cli): improve focus navigation for interactive and background shells#18343
Conversation
- Update key bindings to explicitly support Shift+Tab for unfocusing shells. - Implement Tab-to-focus and Shift+Tab-to-unfocus in AppContainer for both interactive and background shell displays. - Allow Tab to bubble to the focused shell (unblocking auto-completion) while still displaying the unfocus hint. - Enable Tab focus directly from ShellToolMessage components via useKeypress. - Ensure Ctrl+B correctly toggles (opens and closes) the BackgroundShellDisplay by allowing it to bubble from the display component. - Add unit tests for the new focus/unfocus behavior in AppContainer.
Summary of ChangesHello @galz10, 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 refines the user experience for interacting with interactive and background shells within the CLI. It introduces more intuitive keyboard navigation for focusing and unfocusing shells and resolves issues where global commands, such as toggling the background shell, were not reliably propagating through the UI. These changes aim to make shell interactions smoother and more predictable for users. Highlights
Changelog
Activity
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.17 kB (-0.02%) Total Size: 23.8 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 does a great job of refactoring the shell focus navigation to be more robust and centralized in AppContainer. The changes to allow key events to bubble up from child components are well-implemented and improve the overall architecture. I've found one issue where a race condition can cause an incorrect warning to be displayed to the user immediately after focusing the shell. The fix is to remove a redundant state update from a child component, reinforcing the principle of consistent UI behavior across components. Otherwise, the changes look solid and the updated tests and documentation are appreciated.
| setEmbeddedShellFocused(true); | ||
| } | ||
| return true; | ||
| return false; |
There was a problem hiding this comment.
Returning false here is correct for allowing the event to bubble up to AppContainer. However, the setEmbeddedShellFocused(true) call on line 937 causes a problem. When the event bubbles, AppContainer sees that embeddedShellFocused is already true and incorrectly displays a warning: "Press Shift+Tab to focus out."
The InputPrompt should not be setting this state. Please remove the if block on lines 933-938 that calls setEmbeddedShellFocused to make AppContainer the single source of truth for this logic.
References
- Maintain consistency with existing UI behavior across components. Defer non-standard UX pattern improvements to be addressed holistically rather than in a single component.
gemini-cli
bot
added
the
status/need-issue
|
This review was generated by Overall, the focus navigation improvements and test coverage look great. However, there are a few React-specific issues to address before merging:
Everything else (keybinding utils, snapshot updates, and Tab/Shift+Tab behavior) looks solid! |
|
This review was generated by the Overall, this is a solid PR that centralizes focus navigation and fixes several bubbling issues. The extraction of keybinding formatting into There are a few React-specific issues that need to be addressed before merging: 1. Stale Timeout Refs in
|
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
I've reviewed the frontend React code. There are a couple of points regarding useEffect dependencies and cleanup functions in ShellToolMessage.tsx and AppContainer.tsx that I've left as separate comments. Otherwise, the logic and focus management improvements look solid.
Review by /review-frontend, audited by Jacob.
- Fix timeout cleanup in AppContainer.tsx by moving it to a dedicated unmount effect, satisfying linter without eslint-disable. - Remove unused and non-functional tabFocusTimeoutRef from AppContainer. - Add missing dependencies to useEffect hooks to comply with exhaustive-deps. - Remove redundant focus-resetting useEffect hooks in AppContainer, useGeminiStream, and ShellToolMessage to eliminate cascading state updates. - Focus is now managed imperatively within terminal-state handlers. - Clean up unused imports and test helpers in ShellToolMessage.test.tsx.
| | Dismiss background shell list. | `Esc` | | ||
| | Move focus from background shell to Gemini. | `Shift + Tab` | | ||
| | Move focus from background shell list to Gemini. | `Tab (no Shift)` | | ||
| | Show warning when trying to unfocus background shell via Tab. | `Tab (no Shift)` | |
There was a problem hiding this comment.
these are super low value keyboard shortcut documentation messages. I think we need an option to hide them adding to keyBindings and respected by the doc generation script.
| // If the shell hasn't produced output in the last 100ms, it's considered idle. | ||
| const isIdle = now - lastOutputTimeRef.current >= 100; | ||
| if (isIdle && !activePtyId) { | ||
|
|
There was a problem hiding this comment.
As a follow up pr can you add a hook loaded by appContainer that has all the background shell logic. we need to start moving stuff out of AppContainer again.
…ackground shells - Re-introduce `tabFocusTimeoutRef` in `AppContainer` with a 150ms responsive threshold. - Implement smart-unfocus logic that detects shell activity; automatically unfocuses on Tab if no PTY output is received, otherwise shows a guidance warning. - Consolidate all shell unfocus handling (`UNFOCUS_SHELL_INPUT`, `UNFOCUS_BACKGROUND_SHELL`, `UNFOCUS_BACKGROUND_SHELL_LIST`) into the `AppContainer` priority keypress handler. - Remove redundant and conflicting unfocus warning logic from `ShellInputPrompt` and `BackgroundShellDisplay` to prevent double-messaging. - Update `shellReducer` to track `lastShellOutputTime` for background shells, ensuring consistent activity detection. - Promote `AppContainer` global keypress handler to high priority to ensure navigation events are intercepted before child component consumption. - Clean up unused imports and destructured variables in `ShellInputPrompt` and `BackgroundShellDisplay`.
|
/patch preview |
|
✅ Patch workflow(s) dispatched successfully! 📋 Details:
🔗 Track Progress: |
|
/patch preview |
|
✅ Patch workflow(s) dispatched successfully! 📋 Details:
🔗 Track Progress: |
|
🚀 Patch PR Created! 📋 Patch Details:
📝 Next Steps:
🔗 Track Progress: |
|
🚀 Patch Release Started! 📋 Release Details:
⏳ Status: The patch release is now running. You'll receive another update when it completes. 🔗 Track Progress: |
|
✅ Patch Release Complete! 📦 Release Details:
🎉 Status: Your patch has been successfully released and published to npm! 📝 What's Available:
🔗 Links: |
3 participants
Summary
Improve shell focus navigation and global toggle bubbling in the CLI. This PR enables
Tabto focus the interactive shell from message history,Shift+Tabto unfocus/escape the shell, and ensuresCtrl+Bcorrectly toggles the background shell display by allowing events to bubble out of specialized UI components.Details
keyBindings.tsto explicitly defineShift+TabforUNFOCUS_SHELL_INPUT, allowing for a dedicated "escape hatch" from focused shells.AppContainer.tsxto handleTab-to-focusandShift+Tab-to-unfocuscorrectly. The logic now allowsTabto bubble through to the shell (unblocking auto-completion) while still showing the relevant UI hints.useKeypressintoShellToolMessage.tsxto allow users to focus a running shell directly from the history usingTab.Ctrl+B(TOGGLE_BACKGROUND_SHELL) bubbling inInputPrompt.tsxandBackgroundShellDisplay.tsx. This ensures the global toggle always reachesAppContainerregardless of which component currently has priority.ToolShared.tsxto explicitly mentionShift+Tabfor unfocusing, improving discoverability.Related Issues
How to Validate
!vim).Tabwhile the shell is focused still performs its shell-native function (e.g., in vim).Shift+Tabwhile the shell is focused escapes focus and returns focus to the Gemini prompt.Tabto return focus to the shell.Ctrl+Bto background the shell.Ctrl+Bagain to open the background shell display.Ctrl+Bto close it.npx vitest run packages/cli/src/ui/AppContainer.test.tsx packages/cli/src/ui/components/InputPrompt.test.tsx packages/cli/src/ui/components/messages/ShellToolMessage.test.tsxPre-Merge Checklist