refactor(cli): fully remove React anti patterns, improve type safety and fix UX oversights in SettingsDialog.tsx#18963
Conversation
- Replace imperative state-syncing with reactive useSettingsStore() from context - Delete 5 local states: pendingSettings, modifiedSettings, globalPendingChanges, _restartRequiredSettings, showRestartPrompt - Delete 5 obsolete utility functions from settingsUtils.ts - Simplify all callbacks to call setSetting() directly (no intermediate state) - Restart-required tracking uses snapshot diff with JSON.stringify comparison - Only track dialog-visible restart settings via new getDialogRestartRequiredSettings() - Remove settings prop from SettingsDialog (reads from context instead) - Update all tests mechanically
Summary of ChangesHello @psinha40898, 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 refactors the 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
|
psinha40898
changed the title
![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 is an excellent refactoring of the SettingsDialog component. Moving the complex local state management into a centralized store pattern using useSettingsStore dramatically simplifies the component, removing the anti-patterns and bloat mentioned in the pull request title. The new implementation is much cleaner, more maintainable, and less prone to state-related bugs. The logic for handling setting updates and the 'restart required' prompt is now significantly more straightforward and robust. The corresponding updates to tests and utility functions are thorough and align perfectly with the new architecture. Overall, this is a high-quality improvement to the codebase.
gemini-cli
bot
added
priority/p1
| // has an object value (structuredClone breaks reference equality). | ||
| for (const key of getDialogRestartRequiredSettings()) { | ||
| const value = getEffectiveValue(key, {}, merged); | ||
| snapshot.set(key, JSON.stringify(value)); |
There was a problem hiding this comment.
JSON.stringify is required because some Settings (that are both restart required AND show in dialog) have Arrays as values. It's also defensive against future settings that may be both restart-required AND show in dialog and have Objects or Arrays as values.
There was a problem hiding this comment.
In general this refactor seeks to fully implement UI functionality for settings that have non primitives as values.
It's included in this refactor because the functionality is kind of a side effect of improved type safety ergonomics just like the reactive restart prompt. It would be messy to split them up IMO. (related #19093)
… per codebase conventions
…ettings which also allows for them to be edited by the UI as intended
psinha40898
changed the title
psinha40898
changed the title
There was a problem hiding this comment.
Review from Gemini
Excellent work on this refactoring! Transitioning to a centralized settings store significantly simplifies the SettingsDialog component and eliminates several React anti-patterns. The code is much cleaner, more maintainable, and the improved type safety for complex types like arrays and objects is a great addition. All tests passed locally, and the logic for handling immediate saves while tracking restart-required settings is robust. Once you're ready to take this out of draft, it's safe to merge.
| /** Raw value for edit mode initialization */ | ||
| rawValue?: string | number | boolean; | ||
| rawValue?: SettingsValue; | ||
| /** Optional pre-formatted edit buffer value for complex types */ |
There was a problem hiding this comment.
A simple conversion of rawValue to string works for primitives but some settings in the UI have values that are not primitives, like arrays
editValue allows SettingsDialog to handle the parsing and pass it down to BaseSettingsDialog which allows for non primitive settings like arrays to work through the UI.
psinha40898
changed the title
| return rawValue.join(', '); | ||
| } | ||
|
|
||
| if (type === 'object' && rawValue !== null && typeof rawValue === 'object') { |
There was a problem hiding this comment.
This ships with support for Settings values that are objects, but no such setting exists in the UI yet
psinha40898
changed the title
…pdating nested values and testing the asterisks displays
psinha40898
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request refactors the SettingsDialog to use a more reactive approach with useSettingsStore and derived state, which simplifies the component and fixes several UX issues. It also improves type safety and adds support for editing array and object settings. However, a regression was found in the commit handler for inline editing that prevents setting values to 0 or empty strings.
| ['System', settings.system.settings], | ||
| ]; | ||
|
|
||
| // Iterate through the nested map snapshot in activeRestartRequiredSettings, diff with current settings |
There was a problem hiding this comment.
The diffing of restart required settings across scope here is the heart of the UX. This diffs the settings files on component mount with the current settings (updated as soon as a user toggles)
The purpose of the diff is to handle the user experience for restart required settings
Alternatively, we could read the restart required settings that are currently active to use changes pending against application restart instead of changes pending against component re mount, but this would require some greater code changes.
| isSettingModified, | ||
| // Business logic utilities, | ||
| TEST_ONLY, | ||
| settingExistsInScope, |
There was a problem hiding this comment.
Utils that were designed to interact with bloated react states and mutable objects were all removed
| export function getEffectiveValue( | ||
| key: string, | ||
| settings: Settings, | ||
| mergedSettings: Settings, |
There was a problem hiding this comment.
this guy was never called with this parameter in the component code. he was always called with an empty mergedSettings which meant it never checked for an inherited value.
| /** | ||
| * Gets a value from a nested object using a key path array iteratively. | ||
| */ | ||
| export function getNestedValue(obj: unknown, path: string[]): unknown { |
There was a problem hiding this comment.
This removes type assertions by accepting unknown. Notably iterating down a type of the previous unsafe assertion Record<string, unknown> results in iterating into unknown anyway.
The type predicate in isRecord removes the eslint disable because our type predicate narrows the type instead of an assertion
I think there has got to be a better way with purely Typescript compile but it might require changes to the settings schemas and could be done separately.
For now replacing eslint ignores and type assertions with two type predicates seems to at least document whats going on much better
| if (value !== undefined) { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| return value as SettingsValue; | ||
| const value = getNestedValue(settings, path); |
There was a problem hiding this comment.
Same thing here, the type predicate replaces the eslint disables and type assertions potentially pending larger scale changes
| } | ||
|
|
||
| /** | ||
| * Get a nested value from an object using a path array |
There was a problem hiding this comment.
The changes here can be reverted to reduce the scope of the PR.
Head branch was pushed to by a user without write access
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
Approved once fix for alternate buffer mode lands.
3 participants
Summary
Next step from #14915
Removes excessive state and implements maintainable React and type safety patterns in the SettingsDialog component
The approach of synchronizing loose states and batching saves results in code that has hard to read concerns and becomes hard to maintain.
Examples of this can be seen in the restart required prompt which currently
does not toggle off if you toggle the setting back to what it was
does toggle off if you change a default setting to a written value and then press control L
does not toggle on if you use control L to change a non default restart required setting to default
The new dialog would be really easy to follow:
user changes Setting in the UI -> useSettingsStore triggers a re render -> the component diffs the initial restart required keys with the current -> user sees updated view
Details
The SettingsDialog and its corresponding settingsUtil are extremely bloated. Some of this stems from the reliance of a React Anti Pattern that can be removed after #14915
Instead of managing and syncing many loose React states the SettingsDialog now works in a sensible way.
This removed many inconsistencies like
Notes:
This retains the current scope-aligned UX, so the restart required prompt will remain active if you add something to scope and toggle it back because that does not remove it from scope. But if you press Control L it will remove the prompt.
Similarly, asterisks continue to show next to Settings that exist in scope (same as main)
Aside from the above improvements, the SettingsDialog retains UX parity with the old implementation.
So stars still appear next to Settings that exist in scope.
final side notes:
this PR simplifies and improves the type safety and the three es-lint ignores.
It has some very sparse changes to AgentConfigDialog to support the new setNestedValue util for similar dialogs
i renamed settingValueExistsInScope to isInSettingsScope
Related Issues
RELATED TO #15840
Fixes #18980
Fixes #19093
Fixes #19232
Fixes #20077
How to Validate
Pre-Merge Checklist