Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ayush31010 wants to merge 3 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd5802b
refactor(ui): fix React anti-patterns in BaseSettingsDialog
ayush31010 Mar 4, 2026
7b0840d
Merge branch 'main' into fix/base-settings-dialog-react-patterns
ayush31010 Mar 8, 2026
9021615
Merge branch 'main' into fix/base-settings-dialog-react-patterns
ayush31010 Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 22 additions & 30 deletions packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,12 +188,11 @@ export function BaseSettingsDialog({
return () => clearInterval(interval);
}, [editingKey]);

// Ensure focus stays on settings when scope selection is hidden
useEffect(() => {
if (!showScopeSelector && focusSection === 'scope') {
setFocusSection('settings');
}
}, [showScopeSelector, focusSection]);
// When the scope selector is hidden, treat any 'scope' focus as 'settings'.
// Derived on render rather than via an effect — effects are for subscribing
// to external systems, not computing values from existing state/props.
const effectiveFocusSection =
!showScopeSelector && focusSection === 'scope' ? 'settings' : focusSection;

// Scope selector items
const scopeItems = getScopeItems().map((item) => ({
Expand Down Expand Up @@ -229,16 +228,8 @@ export function BaseSettingsDialog({
setEditCursorPos(0);
}, [editingKey, editBuffer, currentItem, onEditCommit]);

// Handle scope highlight (for RadioButtonSelect)
const handleScopeHighlight = useCallback(
(scope: LoadableSettingScope) => {
onScopeChange?.(scope);
},
[onScopeChange],
);

// Handle scope select (for RadioButtonSelect)
const handleScopeSelect = useCallback(
// Single callback used for both onSelect and onHighlight on RadioButtonSelect
const handleScopeChange = useCallback(
(scope: LoadableSettingScope) => {
onScopeChange?.(scope);
},
Expand Down Expand Up @@ -360,7 +351,7 @@ export function BaseSettingsDialog({
}

// Not in edit mode - handle navigation and actions
if (focusSection === 'settings') {
if (effectiveFocusSection === 'settings') {
// Up/Down navigation with wrap-around
if (keyMatchers[Command.DIALOG_NAVIGATION_UP](key)) {
const newIndex = activeIndex > 0 ? activeIndex - 1 : items.length - 1;
Expand Down Expand Up @@ -427,7 +418,7 @@ export function BaseSettingsDialog({
},
{
isActive: true,
priority: focusSection === 'settings' && !editingKey,
priority: effectiveFocusSection === 'settings' && !editingKey,
},
);

Expand All @@ -444,10 +435,10 @@ export function BaseSettingsDialog({
{/* Title */}
<Box marginX={1}>
<Text
bold={focusSection === 'settings' && !editingKey}
bold={effectiveFocusSection === 'settings' && !editingKey}
wrap="truncate"
>
{focusSection === 'settings' ? '> ' : ' '}
{effectiveFocusSection === 'settings' ? '> ' : ' '}
{title}{' '}
</Text>
</Box>
Expand All @@ -459,7 +450,7 @@ export function BaseSettingsDialog({
borderColor={
editingKey
? theme.border.default
: focusSection === 'settings'
: effectiveFocusSection === 'settings'
? theme.ui.focus
: theme.border.default
}
Expand All @@ -468,7 +459,7 @@ export function BaseSettingsDialog({
marginTop={1}
>
<TextInput
focus={focusSection === 'settings' && !editingKey}
focus={effectiveFocusSection === 'settings' && !editingKey}
buffer={searchBuffer}
placeholder={searchPlaceholder}
/>
Expand All @@ -492,7 +483,8 @@ export function BaseSettingsDialog({
{visibleItems.map((item, idx) => {
const globalIndex = idx + scrollOffset;
const isActive =
focusSection === 'settings' && activeIndex === globalIndex;
effectiveFocusSection === 'settings' &&
activeIndex === globalIndex;

// Compute display value with edit mode cursor
let displayValue: string;
Expand Down Expand Up @@ -604,19 +596,19 @@ export function BaseSettingsDialog({
{/* Scope Selection */}
{showScopeSelector && (
<Box marginX={1} flexDirection="column">
<Text bold={focusSection === 'scope'} wrap="truncate">
{focusSection === 'scope' ? '> ' : ' '}Apply To
<Text bold={effectiveFocusSection === 'scope'} wrap="truncate">
{effectiveFocusSection === 'scope' ? '> ' : ' '}Apply To
</Text>
<RadioButtonSelect
items={scopeItems}
initialIndex={scopeItems.findIndex(
(item) => item.value === selectedScope,
)}
onSelect={handleScopeSelect}
onHighlight={handleScopeHighlight}
isFocused={focusSection === 'scope'}
showNumbers={focusSection === 'scope'}
priority={focusSection === 'scope'}
onSelect={handleScopeChange}
onHighlight={handleScopeChange}
isFocused={effectiveFocusSection === 'scope'}
showNumbers={effectiveFocusSection === 'scope'}
priority={effectiveFocusSection === 'scope'}
/>
</Box>
)}
Expand Down