fix(cli): hide read-only settings scopes#26249
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 refines the CLI's settings management by ensuring that the settings dialog only presents editable scopes to the user. This change resolves an issue where users could select read-only scopes, leading to confusion when changes made within them were not persisted. The update enhances the clarity and reliability of the settings interface by guiding users towards valid modification targets. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request implements logic to handle read-only settings scopes within the CLI's settings dialog. It updates the LoadedSettings class to preserve readOnly metadata during snapshots and modifies the SettingsDialog component to filter out non-writable scopes from the UI. The changes include logic to automatically fall back to a writable scope if the current selection is read-only and prevent editing actions when no writable scopes are available. Comprehensive tests were added to verify these behaviors across different scope configurations. I have no feedback to provide.
ac0158f to
a74fa70
Compare
BerriAI/litellm#27106 (mcp oauth auth-gate fix), charmbracelet/crush#2606 (split-pane+pty infra), google-gemini/gemini-cli#26249 (hide read-only settings scopes).
|
Great fix! The React logic and test coverage are excellent. I have two minor suggestions regarding the core logic and test utils that could make this implementation a bit more elegant and robust against future changes: 1. Use object spread in const cloneSettingsFile = (file: SettingsFile): SettingsFile => ({
...file,
settings: structuredClone(file.settings),
originalSettings: structuredClone(file.originalSettings),
});2. Update Otherwise, this is a very high-quality PR and looks good to go! |
|
@scidomino, thanks for the review, I’ve addressed both of your suggestions.
All tests and type checks pass on my end. Let me know if there’s anything else I should adjust. On a side note, I appreciate the detailed feedback |
|
Remove this code from the useEffect and just replace it with direct code. This will be more efficient since it won't need to render twice: if (writableSelectedScope && selectedScope !== writableSelectedScope) {
setSelectedScope(writableSelectedScope);
} |
|
@scidomino Done! thank you for the feedback |
Summary
Fixes a bug in the /settings dialog that incorrectly allowed users to select read-only scopes. Previously, modifying these scopes would only update the temporary runtime state and fail to save permanently to settings.json
Details
SettingsDialog.readOnlymetadata inLoadedSettingsSnapshot.SettingsDialogcallbacks to usewritableSelectedScopeandeffectiveSelectedScopeinstead ofselectedScope.getSnapshot()and scope filtering behavior.Related Issues
Fixes #25429
How to Validate
npm run test --workspace @google/gemini-cli -- src/config/settings.test.ts src/ui/components/SettingsDialog.test.tsxnpm run buildnpm run typecheckPre-Merge Checklist