feat(plan): support clearing conversation context on plan approval#21168
feat(plan): support clearing conversation context on plan approval#21168
Conversation
Summary of ChangesHello, 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 plan approval workflow by introducing a feature to selectively clear conversation history. This allows users to maintain a cleaner, more focused conversation by removing irrelevant planning drafts and iterations from both the terminal UI and the LLM's memory, ensuring that only critical context persists after a plan is approved. The changes span core logic for history management, UI updates for user control, and event-driven synchronization between components. 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
|
Adib234
changed the title
|
Size Change: +7.41 kB (+0.03%) Total Size: 26.5 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
is it possible to provide a dialog to clear context after approval instead of baking it into the approval prompts?
- approval options stay the same
- if approved, provide an additional dialog to clear context the standard options, e.g.
allow once,allow in this session,allow in future sessionsetc
![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 introduces a valuable feature to clear the conversation context upon plan approval, which helps in reducing noise from intermediate planning iterations. The implementation is well-structured, spanning across the core logic, UI components, and context management. The use of a new CoreEvent.ApprovalModeChanged event for state synchronization between the core and UI is a good design choice. The history truncation logic in exit-plan-mode.ts is precise and correctly implemented to retain the essential parts of the conversation. The accompanying updates to tests and snapshots ensure the new functionality is well-covered. Overall, this is a solid contribution with no major issues found.
gemini-cli
bot
added
area/core
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to selectively clear conversation history upon plan approval, which helps in removing intermediate planning discussions. The implementation correctly tracks the start of a planning session, modifies the exit_plan_mode tool for surgical history truncation, and updates the UI to offer this new option to the user. The changes are well-tested and the logic appears sound. I have no high or critical severity feedback at this time.
|
I've reviewed the changes and have a few concerns regarding the implementation:
|
|
I've reviewed the changes and have a few concerns regarding the implementation:
|
|
Thanks for the PR! I've reviewed the changes and found a few areas that need to be addressed to align with our development guidelines, particularly around React state management. Feedback
Please address these points, especially the |
|
Adib234
force-pushed
the
adibakm/clear-context-conversation-approval
branch
from
6c6ed34 to
e3b1f31
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for clearing conversation context upon plan approval, which will improve the user experience by reducing noise from planning iterations. The implementation is comprehensive, touching UI, core logic, and configuration. However, I've identified a high-severity issue in the new dialog logic where selecting "No" to clearing context unintentionally makes a permanent configuration change, which violates established UX consistency principles. My review includes a specific suggestion to correct this behavior.
Note: Security Review did not run due to the size of the PR.
| } else if (answer === ContextOption.No) { | ||
| clearConversation = false; | ||
| setSetting( | ||
| SettingScope.User, | ||
| 'general.plan.clearContextOnApproval', | ||
| false, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The current implementation for the "No" option in the context clearing dialog has an unintended side effect. It permanently sets the general.plan.clearContextOnApproval setting to false, which prevents the user from being prompted again in future sessions. The option's label "No" and description "Keep context" imply a one-time choice for the current plan approval, not a permanent configuration change. This can lead to a confusing user experience, as the user would have to manually edit their settings file or use the /settings command to re-enable the feature or be prompted again.
To align with user expectations, this option should only affect the current plan approval. Please remove the call to setSetting.
} else if (answer === ContextOption.No) {
clearConversation = false;
}
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. The 'No' option having a permanent configuration change effect is a non-standard and confusing UX pattern that deviates from typical user expectations for such a prompt.
There was a problem hiding this comment.
This is intended, "No" option clears the context dialog forever to not annoy users in the future
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to clear conversation history upon plan approval, which is a great UX improvement. The implementation involves changes across the UI, core logic, and configuration to track plan mode state and surgically truncate both UI and LLM history. The overall approach is solid. I've found one potential issue in the new React component logic that could lead to unintended side effects, which I've detailed in a comment.
Note: Security Review did not run due to the size of the PR.
|
Thanks for the PR! I've reviewed the changes and found a few areas that need to be addressed to align with our strict development guidelines, particularly around React state management and settings. Code Review Feedback
Please address these points to ensure the implementation is robust and consistent with the repository's patterns. Let me know if you have any questions! |
Adib234
force-pushed
the
adibakm/clear-context-conversation-approval
branch
from
dc99005 to
e78099f
Compare
Adib234
changed the title
|
Note to reviewers: excluding the snapshot file changes the lines of code changed in this PR are |
| if (payload.clearConversation) { | ||
| const geminiClient = this.config.getGeminiClient(); | ||
| if (geminiClient) { | ||
| const history = geminiClient.getHistory(); | ||
| const startIndex = this.config.getPlanModeHistoryStartIndex(); | ||
|
|
||
| // Find the user message that initiated the plan mode. | ||
| let planningUserIndex = -1; | ||
|
|
||
| // If the message exactly at startIndex is a user message, then plan mode | ||
| // was initiated via a UI/CLI command before the message was added to history. | ||
| if ( | ||
| startIndex < history.length && | ||
| history[startIndex].role === 'user' | ||
| ) { | ||
| planningUserIndex = startIndex; | ||
| } else { | ||
| // Otherwise, Plan mode was initiated via the `enter_plan_mode` tool. | ||
| // The initiating user message is the last user message before startIndex. | ||
| for ( | ||
| let i = Math.min(startIndex, history.length - 1); | ||
| i >= 0; | ||
| i-- | ||
| ) { | ||
| if (history[i].role === 'user') { | ||
| planningUserIndex = i; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (planningUserIndex !== -1) { | ||
| const lastModelMessage = history[history.length - 1]; | ||
| // Keep everything before the plan, plus the initial plan request turn. | ||
| const newHistory = [ | ||
| ...history.slice(0, planningUserIndex + 1), | ||
| lastModelMessage, | ||
| ]; | ||
| geminiClient.setHistory(newHistory); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
can we refactor the logic in
into a shared util that's used both for/clear and this?
| enum ContextOption { | ||
| Once = 'Allow once', | ||
| Session = 'Allow for this session', | ||
| Always = 'Allow for all future sessions', | ||
| No = 'No', | ||
| } | ||
|
|
There was a problem hiding this comment.
instead of redefining this, can we reuse the standard tool confirmation?
|
Closing PR, see linked issue |
2 participants
Summary
This PR adds support for surgically clearing the conversation context (both terminal UI and LLM history) upon plan approval. This allows users to eliminate "noisy" intermediate planning iterations while preserving critical context that occurred before the planning session began.
Details
planModeHistoryStartIndexto theConfigservice to precisely track when a session transitions into Plan Mode.exit_plan_modetool to perform surgical history manipulation. It now retains all turns prior to the plan, the initial planning request turn, and the final approval turn, effectively discarding outdated draft discussions.CoreEvent.ApprovalModeChangedevent. TheAppContainerlistens for this to track UI history indices and implementshandleClearPlanContextfor visual truncation usingrefreshStatic.ExitPlanModeDialogto provide users with explicit choices to clear conversation context alongside existing approval modes (Default/Auto-Edit).Related Issues
Fixes #17723
Notice that when we cleared context, the UI did not clear the history before plan mode was active.
How to Validate
/plan.Yes, automatically accept edits & clear conversation.npm test -w @google/gemini-cli-core -- src/tools/exit-plan-mode.test.tsandnpm test -w @google/gemini-cli -- src/ui/components/ExitPlanModeDialog.test.tsx.Pre-Merge Checklist