wip refactor(cli): SettingsDialog.tsx implement better React patterns by deriving state and effect and use of reducer and custom hook

[psinha40898]

Summary

WIP this wip is not ready for review Opened as draft to use Gemini CLI PR reviewing

Reduced useState calls: 15 → 1 (+ 1 useReducer with 7 values + 1 custom hook with 4 values) Reduced useEffect calls: 4 → 1

SettingsDialog.test.tsx remains unchanged with all tests passing

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

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

IMPORTANT: It retains one React Anti Pattern: Maintain use of pendingSettings state to drive React renders for non-restart-required setting changes. When settings is mutated (the instance of the Settings class passed in as a prop) it does not force a React render because SettingsContext does not provide and sync its own React state with the mutable Settings instance. This change is beyond the scope of this PR because it would increase complexity to review, and the SettingsDialog seems to be the only place where a React render is desired after updating the Settings instance.

It may make sense to have the Context updater drive React renders in a future PR.

Details

States

---

Retained as useState (1):

• pendingSettings: Retained to force React re-renders when non-restart settings toggle (which save immediately without affecting reducer state)

Moved to reducer (7):

• focusSection: Centralizes navigation logic, prevents focus traps
• selectedScope: Ensures atomic scope switching with other state updates
• activeSettingIndex: Coupled with scrollOffset in NAVIGATE actions to prevent visual jitter
• scrollOffset: Managed alongside index to keep cursor in view
• searchQuery: Input handling now dispatches actions
• filteredKeys: Updates atomically with searchQuery
• globalPendingChanges: Now single source of truth for restart-required changes

Moved to custom hook (4):

• editingKey, editBuffer, editCursorPos, cursorVisible: Encapsulated in useInlineEdit hook

Deleted (2):

• modifiedSettings: Replaced by checking state.globalPendingChanges.keys()
• _restartRequiredSettings: Redundant since globalPendingChanges only stores restart-required items

Now derived (2):

• showRestartPrompt: Derived as state.globalPendingChanges.size > 0
• effectiveFocusSection: Computed inline based on showScopeSelection

Effects

---

Replaced with derivation (2):

• Focus correction when scope hidden: Now computed inline as effectiveFocusSection
• Restart prompt sync: Now derived from globalPendingChanges.size

Moved to custom hook (1):

• Inline-edit cursor blink: Encapsulated in useInlineEdit

Retained (1):

• Fuzzy-search filter: Still performs async fzf lookup, but now dispatches SET_FILTERED_KEYS through reducer instead of mutating local state

Overlay effect: Previously a useEffect iterated over globalPendingChanges and synced multiple states. The iteration now only occurs where needed in a memoized call within handleScopeHighlight. The rest of the states are updated more effectively through the aforementioned refactors.

Summary

• Reduced useState calls: 15 → 1 (+ 1 useReducer with 7 values + 1 custom hook with 4 values)
• Reduced useEffect calls: 4 → 1

## Related Issues

<!-- Use keywords to auto-close issues (Closes #123, Fixes #456). If this PR is
only related to an issue or is a partial fix, simply reference the issue number
without a keyword (Related to #123). -->

## How to Validate

<!-- List exact steps for reviewers to validate the change. Include commands,
expected results, and edge cases. -->

## Pre-Merge Checklist

<!-- Check all that apply before requesting review or merging. -->

- [ ] 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 focuses on enhancing the SettingsDialog component's architecture by adopting more robust React patterns. The primary goal is to streamline state management, reduce component complexity, and improve maintainability by migrating from numerous individual useState and useEffect hooks to a centralized useReducer and a dedicated custom hook for inline editing. This refactoring also emphasizes deriving state where possible, leading to a cleaner and more efficient component structure.

Highlights

• State Management Refactor: The SettingsDialog.tsx component has undergone a significant refactor to improve state management, reducing useState calls from 15 to 1 and useEffect calls from 4 to 1. This was achieved by centralizing state logic using useReducer and encapsulating inline editing logic within a new custom hook.
• Introduction of useReducer: A new useReducer hook now manages core dialog states including focus section, selected scope, active setting index, scroll offset, search query, filtered keys, and global pending changes. This consolidates related state updates and logic into a single settingsDialogReducer.
• New Custom Hook for Inline Editing: A custom hook, useInlineEdit, has been introduced to manage all aspects of inline text editing, including the editing key, buffer content, cursor position, and cursor visibility. This abstracts complex editing logic away from the main component.
• State Derivation: Several states, such as showRestartPrompt and effectiveFocusSection, are now derived directly from other states, eliminating the need for dedicated useState or useEffect calls to manage them. This reduces boilerplate and potential for synchronization issues.
• Improved Test Coverage: New test files (settingsDialogReducer.test.ts and useInlineEdit.test.ts) have been added to ensure the correctness and reliability of the new reducer and custom hook implementations.

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]

/gemini review

[psinha40898]

/gemini review

[psinha40898]

REVIEW GENERATED BY GEMINI 3-PRO-PREVIEW

This is a strong refactoring PR that significantly improves the state management of SettingsDialog.tsx by introducing a reducer and a custom hook.

Here are the findings and specific requests for changes:

1. State Management Regression (Critical)

The setPendingSettings call does not use the functional update form. When updating state based on the previous state, it's crucial to use the functional setState(prevState => ...) form to avoid potential race conditions and bugs from using a stale state value from a closure.

This pattern is repeated in several places in this file when updating pendingSettings (e.g., lines 228-230, 313-315, 328-330, 673-681, and 705-711) and should be corrected in all instances.

2. setNestedValue Helper

Regarding the local definition of setNestedValue vs importing from utils:

The helper is tiny, invoked only when a user highlights a different scope (not on every render frame), and its closure captures nothing from the component.

I agree with this reasoning. The cognitive load and maintenance burden of a local helper here are low enough that strict adherence to DRY isn't worth exposing internal utils or refactoring settingsUtils just for this. Status: Accepted as is.

3. Conventional Commits

The PR title follows the convention, but the recent commits (e.g., fix test state names, the effect should only depend on...) do not. Please squash or reword these commits to follow Conventional Commits before merging (e.g., fix(ui): fix test state names).

4. Logging

In SettingsDialog.tsx, there is a usage of console.error in the catch block for toggleVimEnabled.

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

Please check if debugLogger should be used here instead to avoid polluting the user's console output.

5. Testing

The new tests for settingsDialogReducer and useInlineEdit look comprehensive. Please ensure that the existing SettingsDialog.test.tsx (which is not modified in this PR) still passes, as it serves as a regression test.

[psinha40898]

This PR successfully refactors SettingsDialog.tsx to use useReducer and a custom hook, significantly improving state management and maintainability.

Regarding the specific questions raised:

1. Functionality Retention: Yes, all functionality regarding the Settings Dialog appears to be retained. The logic for handling settings updates—saving non-restart settings immediately (clearing the asterisk) and holding restart-required settings (keeping the asterisk)—is preserved.
2. modifiedSettings Replacement: It is true that modifiedSettings is fully replaced by checking globalPendingChanges.keys(). On main, modifiedSettings was used to track changes that hadn't been fully applied (restart-required). Since non-restart settings are saved immediately in the new logic (and removed from pending), globalPendingChanges correctly tracks only the restart-required settings that need to be persisted, mirroring the behavior of modifiedSettings for asterisks.
3. _restartRequiredSettings Redundancy: Yes, this state was redundant as it mirrored information already available in modifiedSettings (and now globalPendingChanges).
4. showRestartPrompt & Focus useEffect Redundancy: Yes, both were redundant. showRestartPrompt is now correctly derived from globalPendingChanges.size > 0, and the focus logic is correctly handled by deriving effectiveFocusSection during render, eliminating the need for an effect and extra state.

Code Quality & Best Practices Review:

• Reducer Exhaustiveness: In settingsDialogReducer.ts, the default case in the switch statement returns state. It is best practice to use the checkExhaustive helper (available in packages/cli/src/utils/checks.ts) in the default case to ensure all action types are handled.

  import { checkExhaustive } from '../../utils/checks.js';
  // ...
  default:
    checkExhaustive(action);
    return state;
  

• Logging: In SettingsDialog.tsx, console.error is used in the toggleVimEnabled catch block. Please use debugLogger instead to avoid polluting the user's console output.
• Tests:
  • The new tests in useInlineEdit.test.ts and settingsDialogReducer.test.ts correctly use act and renderHook (from ../../test-utils/render.js).
  • Please ensure the existing integration tests in SettingsDialog.test.tsx (which are unmodified) still pass to confirm no regressions in the UI behavior.
• Commit Messages: Please squash and reword commits to follow Conventional Commits (e.g., refactor(cli): ...).
• Wait For: Ensure any async tests use the custom waitFor wrapper if needed, though the current new tests seem to rely on act and timers which is appropriate for hooks/reducers.

The refactor is solid and addresses the complexities of the previous implementation well.