refactor(ui): fix React anti-patterns in BaseSettingsDialog

[ayush31010]

Summary

Fixes the two React anti-patterns identified in #21140.

1. Redundant callbacks (Exhibit A)

handleScopeHighlight and handleScopeSelect had identical bodies. Replaced with a single handleScopeChange passed to both onSelect and onHighlight on <RadioButtonSelect />.

// Before
const handleScopeHighlight = useCallback((scope) => onScopeChange?.(scope), [onScopeChange]);
const handleScopeSelect    = useCallback((scope) => onScopeChange?.(scope), [onScopeChange]);

// After
const handleScopeChange = useCallback((scope) => onScopeChange?.(scope), [onScopeChange]);

2. Effect used for derived state (Exhibit B)

// Before — effect calling setState to enforce a derived value
useEffect(() => {
  if (!showScopeSelector && focusSection === 'scope') {
    setFocusSection('settings');
  }
}, [showScopeSelector, focusSection]);

// After — derived inline during render
const effectiveFocusSection =
  !showScopeSelector && focusSection === 'scope' ? 'settings' : focusSection;

Effects are for subscribing to external systems. This value is purely derived from existing state and props, so computing it on render is the correct pattern.

Test plan

• All 28 existing BaseSettingsDialog tests pass unchanged
• ESLint + Prettier clean (pre-commit hook passes)

Closes #21140

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 refactors the BaseSettingsDialog component to improve its adherence to React best practices. It specifically targets and resolves two identified anti-patterns: the use of redundant callback functions and the incorrect application of useEffect for managing derived state. These changes enhance the component's performance and readability by simplifying its internal logic and ensuring state derivations occur efficiently during the render cycle.

Highlights

• Eliminated Redundant Callbacks: The handleScopeHighlight and handleScopeSelect functions, which had identical logic, were consolidated into a single handleScopeChange callback. This callback is now used for both onSelect and onHighlight props of the RadioButtonSelect component, reducing code duplication.
• Replaced Effect for Derived State: A useEffect hook previously used to enforce a derived state (focusSection) when showScopeSelector was hidden has been removed. The logic for effectiveFocusSection is now computed inline during render, adhering to React best practices where effects are reserved for external system subscriptions.
• Improved Code Clarity and Maintainability: By addressing these two React anti-patterns, the component's logic is now clearer, more efficient, and easier to maintain, aligning with recommended React patterns for state management and derived values.

Changelog

• packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx
  • Replaced a useEffect hook with an inline derived state calculation for effectiveFocusSection to handle focus when the scope selector is hidden.
  • Consolidated handleScopeHighlight and handleScopeSelect into a single handleScopeChange callback.
  • Updated all references from focusSection to effectiveFocusSection in conditional rendering, styling, and focus logic.
  • Modified the RadioButtonSelect component to utilize the new handleScopeChange for both its onSelect and onHighlight props.

Activity

• All 28 existing BaseSettingsDialog tests passed unchanged, ensuring no regressions were introduced.
• ESLint and Prettier checks passed, confirming code quality and formatting standards are maintained.
• This pull request addresses and closes issue React patterns in BaseSettingsDialog #21140, resolving the identified React anti-patterns.

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 👍 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

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. ↩

[gemini-code-assist]

Code Review

This pull request refactors the BaseSettingsDialog component to address two React anti-patterns. The redundant handleScopeHighlight and handleScopeSelect callbacks have been consolidated into a single handleScopeChange function, reducing code duplication. Additionally, a useEffect hook that was incorrectly used for deriving state has been replaced with an inline computation during the render phase. These changes improve code quality and align with React best practices without altering functionality.

[psinha40898]

Hey I'm already assigned to this issue. I'm the one who filed it.

I would also say it's a little more complicated than only addressing the exact two examples I highlighted in the filed issue.

My advice to you would be to consider that #21003 was merged in today, so help wanted issues should begin to clear up in the next weeks.

Also you should actually verify that your /assign command succeeded before you assume it did! I was already assigned lol

Really sorry about the miscommunication.

[psinha40898]

@spencer426
#21206

merging this will just cause conflicts with #21206 which addresses the problem more holistically and is a path approved by jacob

[psinha40898]

@spencer426 #21206
merging this will just cause conflicts with #21206 which addresses the problem more holistically and is a path approved by jacob

Idt it will create that of an issue

I apologize if this comes across the wrong way:

1. I'm assigned to the issue, which btw I filed, which I pointed out and you ignored me
2. this PR just copies the exact two small issues identified in the issue that I filed...those were actually the smallest examples. anybody who copy pastes that issue into an LLM will generate an almost identical pull request. which btw is how these anti patterns get here in the first place.

The actual work of pruning out these anti patterns is something that was started in #15840 and has been proceeding with
#14915 and #18963 and soon #21206

Merging this does not close the issue or fix the issue, so I will have to get it reopened. I also then have to worry abt whether the automated workflow will close my PR because it thinks the issue is fixed.

It just fixes two of the easiest unfortunate react patterns that were the easiest examples to provide

I don't mind resolving conflicts and you're right that it isn't a big deal but this is the situation and I'm sorry if my tone is unfortunate

[spencer426]

@Solventerritory apologies for the confusion as @psinha40898 pointed this issue is being addressed and this PR will not be merged.

[spencer426]

Ignore my previous approval as this issue is already addressed.