refactor(cli): SettingsDialog and SettingsContext better React patterns

[psinha40898]

Summary

pr to solve these two issues:

1. Non idiomatic Settings object in Context Slight extensions to LoadedSettings so that it behaves as a store Expand SettingsContext to sync to the Settings store with useSyncExternalStore

2. Complexity of SettingsDialog With Idiomatic Context synced to LoadedSettings as an external store, components can directly render the snapshot from context as well as call updaters to trigger re renders from React. Reduced useState calls: 15 → 0 (+ 1 useReducer with 7 values + 1 custom hook with 4 values) Reduced useEffect calls: 4 → 1 (+ 1 moved to the custom hook)

SettingsDialog.test.tsx remains mostly unchanged

Details

The Refactor exists in these major components that seek to adhere to React patterns:

Settings Context and LoadedSettings

1. Modifying LoadedSettings to behave like a store, have the SettingsContext Provider expose a snapshot of the store as well as an updater and sync it with useSyncExternalStore to drive Idiomatic React renders whenever the store is updated.

Parameters subscribe: A function that takes a single callback argument and subscribes it to the store. When the store changes, it should invoke the provided callback, which will cause React to re-call getSnapshot and (if needed) re-render the component. The subscribe function should return a function that cleans up the subscription.

getSnapshot: A function that returns a snapshot of the data in the store that’s needed by the component. While the store has not changed, repeated calls to getSnapshot must return the same value. If the store changes and the returned value is different (as compared by Object.is), React re-renders the component.

https://react.dev/reference/react/useSyncExternalStore#

SettingsDialog Component

1. A reducer to describe state changes for navigation, search, and globalPendingChanges.

As your components grow in complexity, it can get harder to see at a glance all the different ways in which a component’s state gets updated.

https://react.dev/learn/extracting-state-logic-into-a-reducer

2. Derive all states related to restart required settings from globalPendingChanges

If the value you need can be computed entirely from the current props or other state, remove that redundant state altogether.

https://react.dev/reference/react/useState

3. Refactors inline editing state into a custom hook.

whenever you write an Effect, consider whether it would be clearer to also wrap it in a custom Hook.

https://react.dev/learn/reusing-logic-with-custom-hooks#when-to-use-custom-hooks

4. Derives effect when possible (like the effect that forces focus on the Settings menu when the Scope menu is hidden).

You don’t need Effects to transform data for rendering. (...) To avoid the unnecessary render passes, transform all the data at the top level of your components.

https://react.dev/learn/you-might-not-need-an-effect

Related Issues

How to Validate

Pre-Merge Checklist

• [ ] Updated relevant documentation and README (if needed)
• [ ] Added/updated tests (if needed)
• [ ] Noted breaking changes (if any)
• [ ] Validated on required platforms/methods:
  • [ ] MacOS
    • [ ] npm run
    • [ ] npx
    • [ ] Docker
    • [ ] Podman
    • [ ] Seatbelt
  • [ ] Windows
    • [ ] npm run
    • [ ] npx
    • [ ] Docker
  • [ ] Linux
    • [ ] npm run
    • [ ] npx
    • [ ] Docker

[gemini-code-assist[bot]]

Summary of Changes

Hello @psinha40898, I'm Gemini Code Assist[^1]! 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 undertakes a substantial refactor of the SettingsDialog component to align with modern React patterns. The primary goal is to simplify state management by consolidating related state into a useReducer and extracting complex UI logic into a custom hook. This approach aims to make the component more maintainable, predictable, and performant by reducing redundant state and effects.

Highlights

• State Management Refactor: The SettingsDialog.tsx component has been significantly refactored to centralize state management using a useReducer hook. This replaces numerous useState calls for navigation, search, and pending changes, reducing useState instances from 15 to 1.
• Custom Hook for Inline Editing: Inline editing logic (key, buffer, cursor position, and visibility) has been encapsulated into a new useInlineEdit custom hook, improving modularity and reusability. This also contributes to the reduction of useState calls.
• Derived State and Effects: Several states and effects are now derived from existing state, adhering to React best practices. For example, showRestartPrompt is derived from globalPendingChanges, and a focus correction effect is replaced by a derived effectiveFocusSection.
• Reduced useEffect Calls: The refactoring has led to a reduction in useEffect calls from 4 to 1, indicating a more efficient and less side-effect-prone component.
• New Reducer and Hook Implementations: Two new files, settingsDialogReducer.ts and useInlineEdit.ts, have been added to implement the new state management logic and custom hook, respectively. Both include comprehensive unit tests.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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 :thumbsup: and :thumbsdown: 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.

[^1]: Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

[psinha40898]

The PR successfully refactors SettingsDialog.tsx to use useReducer and useInlineEdit, significantly improving state management and readability. I have reviewed the code and tests, and here are my findings.

Code Quality & Patterns

• Refactor: The move to useReducer for complex state (navigation, search, pending changes) is excellent. It makes the state transitions explicit and testable.
• Hooks: Extraction of useInlineEdit cleans up the component logic significantly.
• React Anti-Pattern: The retention of pendingSettings to drive renders (due to the mutable nature of Settings) is acceptable in this context, as noted in the description.
• Logging: The use of debugLogger is good.
  • Note: There is a console.error left in SettingsDialog.tsx (line 235: console.error('Failed to toggle vim mode:', error);). Consider using debugLogger or a UI error state. Given it's a catch block for an async action in the UI, it might get lost or mess up the TUI.

Tests

• New Tests: The new tests for settingsDialogReducer and useInlineEdit are well-structured and use vitest correctly.
• Render Wrapper: Usage of renderHook from ../../test-utils/render.js correctly wraps interactions in act, following the project's testing guidelines.
• Existing Tests: It is great to see that SettingsDialog.test.tsx passed without modification, indicating the refactor preserved behavior.

Logic Issue (Reset to Default)

I confirmed the potential issue with resetting to default values for restart-required settings:

• The PendingValue type in settingsDialogReducer.ts is boolean | number | string.
• In SettingsDialog.tsx, when resetting a restart-required setting:

  if (
    (currentSetting.type === 'boolean' && typeof defaultValue === 'boolean') ||
    // ...
  ) {
     dispatch({ type: 'ADD_PENDING_CHANGE', ... });
  }
  

• If defaultValue is undefined (which getDefaultValue returns for settings with no explicit default in the schema), this block is skipped.
• Consequently, the "reset" (unsetting the value) is not tracked in globalPendingChanges, and the user's intent to reset it will be lost if they don't save immediately.
• Suggestion: Update PendingValue to include undefined or a symbol to represent "unset", and handle this case in the reducer and saveModifiedSettings logic.

// Example fix for the PendingValue type
export type PendingValue = boolean | number | string | undefined; // undefined represents 'unset'

Recommendation

This is a high-quality refactor. Please address the console.error and the default value logic issue.

[psinha40898]

Logic Issue (Reset to Default)

I confirmed the potential issue with resetting to default values for restart-required settings:

• The PendingValue type in settingsDialogReducer.ts is boolean | number | string.
• In SettingsDialog.tsx, when resetting a restart-required setting:

  if (
    (currentSetting.type === 'boolean' && typeof defaultValue === 'boolean') ||
    // ...
  ) {
     dispatch({ type: 'ADD_PENDING_CHANGE', ... });
  }
  

• If defaultValue is undefined (which getDefaultValue returns for settings with no explicit default in the schema), this block is skipped.
• Consequently, the "reset" (unsetting the value) is not tracked in globalPendingChanges, and the user's intent to reset it will be lost if they don't save immediately.
• Suggestion: Update PendingValue to include undefined or a symbol to represent "unset", and handle this case in the reducer and saveModifiedSettings logic.

// Example fix for the PendingValue type
export type PendingValue = boolean | number | string | undefined; // undefined represents 'unset'

I can push a defect test a long with a fix but will note it for now and wait for a reviewer to decide whether this refactor PR should modify any SettingsDialog.test.tsx tests. Because the defect test would also fail on main.

• Logging: The use of debugLogger is good.

  • Note: There is a console.error left in SettingsDialog.tsx (line 235: console.error('Failed to toggle vim mode:', error);). Consider using debugLogger or a UI error state. Given it's a catch block for an async action in the UI, it might get lost or mess up the TUI.

Same here because the console.error is inherited from main.

Because the focus of the PR is on React usage

[psinha40898]

Thanks for the significant refactor! This cleans up the state management in SettingsDialog and aligns well with our React guidelines.

I've reviewed the changes and the discussion. Here are the items to address before merging:

1. Reset to Default Logic (Undefined Values): Re: your comment about the logic issue with undefined default values: Please go ahead and include the fix and the test case in this PR. It is better to resolve this known bug now, especially since you have identified the solution.

   • Update PendingValue in settingsDialogReducer.ts:

     export type PendingValue = boolean | number | string | undefined;
     

   • Update the logic in SettingsDialog.tsx to handle undefined correctly.
   • Add the test case to settingsDialogReducer.test.ts.

2. Console Error: Re: your comment about the inherited console.error:

   Same here because the console.error is inherited from main.

   While I understand it is pre-existing, GEMINI.md explicitly advises against console logs in production code. Since you are already modifying this file significantly, please switch it to debugLogger.log (or handle it in the UI) to be consistent with the rest of the project. It's a small change that improves code quality.

3. Exhaustive Check: In settingsDialogReducer.ts, please use the checkExhaustive helper in the default case of the switch statement. This is a project pattern to ensure all action types are handled.

   • Import it from ../../utils/checks.js.
   • Usage:

     default:
       checkExhaustive(action);
       return state;
     

Verification: The new tests in settingsDialogReducer.test.ts and useInlineEdit.test.ts look correct and use the proper test utils.

Great work on simplifying this component!

[jacob314]

let me know when this is ready fore view. there are a few merge conflicts.

[psinha40898]

from gemini 3 /review-frontend through gemini cli:

This is a significant and well-executed refactor. The move to useSyncExternalStore for settings and useReducer for the dialog state makes the data flow much more predictable and idiomatic.

I have a few minor comments:

1. console.error: In packages/cli/src/ui/components/SettingsDialog.tsx, there is a call to console.error when toggling vim mode:

   toggleVimEnabled().catch((error) => {
     console.error('Failed to toggle vim mode:', error);
   });
   

   Per project guidelines (GEMINI.md), we should avoid console.error. Please verify if this can be replaced with debugLogger.error.

2. Synchronous I/O: setSetting (and thus saveSettings) performs synchronous file I/O. Since this is now triggered directly from the UI event handlers (toggling/committing), it blocks the render thread. Given this is a CLI and these are explicit commit actions, it is likely acceptable, but please ensure this pattern is not used in any high-frequency inputs (e.g., typing in a search field without debouncing, though the current search implementation looks correct).

3. Test Helpers: Verified that SettingsDialog.test.tsx correctly uses the project-specific render and waitFor helpers from test-utils. This is excellent.

4. Exhaustive Checks: Good use of checkExhaustive in the reducer.

[psinha40898]

now two hooks instead of diffing 70 files

[psinha40898]

The changes establish the necessary infrastructure for a reactive settings store, aligning well with the goal of adopting useSyncExternalStore to fix existing React anti-patterns.

Code Quality & Best Practices:

• structuredClone Usage: Verified that package.json specifies node >= 20.0.0, ensuring global availability of structuredClone without polyfills. This is a robust choice for snapshot immutability.
• Performance: computeSnapshot deep-clones the entire settings object on every setValue. Given that settings updates are typically user-driven and infrequent, this overhead is acceptable and preferable for safety.
• Architecture: The observable pattern (subscribe, getSnapshot) is implemented correctly for future React integration.

Tests:

• The tests in packages/cli/src/config/settings.test.ts comprehensively cover the new subscription logic and snapshot independence.
• No React-specific test utilities are required for these unit tests.

Nit:

• The PR title is somewhat broad for this specific change (which only touches LoadedSettings), but I understand it falls under the umbrella of the larger refactor described in the summary.

Conclusion: This is a solid foundational change.