feat(cli): remove Plan Mode from rotation when actively working#19262
feat(cli): remove Plan Mode from rotation when actively working#19262
Conversation
Summary of ChangesHello @jerop, 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 of the CLI by intelligently managing the availability of Plan Mode. It ensures that users cannot inadvertently switch into Plan Mode when the application is busy processing or requires immediate input, thereby streamlining interaction and preventing potential confusion. The change centralizes the logic for Plan Mode availability and updates the UI and underlying hooks to respect this new conditional behavior. 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
|
![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 effectively removes Plan Mode from the approval mode rotation when the agent is busy, as described. The implementation is clean and well-structured. I appreciate the refactoring that consolidates the logic for Plan Mode availability into a new allowPlanMode state variable in AppContainer and passes it down, decoupling the useApprovalModeIndicator hook from this logic. The accompanying updates to tests and documentation are thorough and accurate. Overall, this is a high-quality change with no issues found.
|
Size Change: +190 B (0%) Total Size: 24.4 MB ℹ️ View Unchanged
|
- Use `streamingState === StreamingState.Idle && !hasPendingActionRequired` for `allowPlanMode`. - Consolidate Plan Mode availability logic into `allowPlanMode` prop. - Update `AppContainer` to pass computed `allowPlanMode` to `useApprovalModeIndicator` hook and `ApprovalModeIndicator` component. - Rename `isPlanEnabled` prop to `allowPlanMode` in `ApprovalModeIndicator` for consistency. - Reorder state logic in `AppContainer` to avoid 'used before declaration' errors. - Add unit tests for `allowPlanMode` in `AppContainer.test.tsx`. Fixes #19021
jerop
force-pushed
the
fix/remove-plan-mode-active-rotation
branch
from
b5242b4 to
6b2d8d5
Compare
gemini-cli
bot
added
area/core
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
Please cleanup the tests as part of a follow up. they look like a good candidate for parameterization to reduce duplicate code.
| ); | ||
| }); | ||
|
|
||
| it('should cycle to PLAN when allowPlanMode is true', () => { |
There was a problem hiding this comment.
nit: parameterize these two tests.
| describe('Plan Mode Availability', () => { | ||
| it('should allow plan mode when enabled and idle', async () => { | ||
| vi.spyOn(mockConfig, 'isPlanEnabled').mockReturnValue(true); | ||
| mockedUseGeminiStream.mockReturnValue({ |
There was a problem hiding this comment.
nit: please parameterize these tests as well
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
Summary
This PR successfully removes Plan Mode from the Shift+Tab approval mode rotation when the CLI is actively working (streaming or awaiting a tool confirmation). This is a solid UX improvement that prevents users from accidentally cycling into Plan Mode while waiting for an ongoing action to complete or while dealing with confirmation dialogs.
Findings
- Critical: None. The changes are logically sound, cleanly implemented, and covered by new tests. All
npm run preflightchecks pass locally. - Improvements: The logic correctly avoids forcing a mode switch if the user is already in Plan Mode when an action starts, while ensuring they cannot cycle back into it until the CLI is idle again.
allowPlanModeis well-integrated withuseApprovalModeIndicatorand correctly replaces the previous usage ofisPlanEnabled. ThehasPendingActionRequiredcheck provides a robust way of ensuring the agent is fully idle before allowing the switch. - Nitpicks: None. The documentation and keyboard shortcut updates correctly reflect the new behavior.
Conclusion
Approved. The code is clean, tests are comprehensive, and validation checks passed without issues. Excellent contribution!
2 participants
Summary
This PR removes Plan Mode from the approval mode rotation when the CLI is actively working (processing or showing confirmation dialogs).
Details
allowPlanModeprop.AppContainerto computeallowPlanModebased onstreamingState === StreamingState.Idle && !hasPendingActionRequired.useApprovalModeIndicatorhook andApprovalModeIndicatorcomponent to use the new prop.isPlanEnabledtoallowPlanModeinApprovalModeIndicatorfor consistency.AppContainerto ensure correct initialization and dependency ordering.AppContainer.test.tsx.docs/cli/plan-mode.mdanddocs/cli/keyboard-shortcuts.mdto reflect the behavioral change.Related Issues
Fixes #19021
How to Validate
Idle State:
/settings-> search "Plan").Shift+Tab.Default->Auto-Edit->Plan->Default.Busy State (Streaming):
Shift+Tab.DefaultandAuto-Edit.Planshould be skipped.Waiting for Confirmation:
Shift+Tab.DefaultandAuto-Edit.Planshould be skipped.