From 8133d63ac691d08b065199ed9c957e911383d9dd Mon Sep 17 00:00:00 2001 From: Pyush Sinha Date: Mon, 2 Mar 2026 13:30:58 -0800 Subject: [PATCH] refactor(cli): fully remove React anti patterns, improve type safety and fix UX oversights in SettingsDialog.tsx (#18963) Co-authored-by: Jacob Richman --- packages/cli/src/gemini.tsx | 2 +- packages/cli/src/test-utils/render.tsx | 2 +- .../src/ui/components/AgentConfigDialog.tsx | 67 +-- .../cli/src/ui/components/DialogManager.tsx | 2 - .../src/ui/components/SettingsDialog.test.tsx | 377 ++++++------- .../cli/src/ui/components/SettingsDialog.tsx | 486 +++++------------ .../shared/BaseSettingsDialog.test.tsx | 31 ++ .../components/shared/BaseSettingsDialog.tsx | 16 +- .../cli/src/ui/contexts/SettingsContext.tsx | 3 +- .../cli/src/ui/contexts/VimModeContext.tsx | 31 +- .../cli/src/utils/dialogScopeUtils.test.ts | 10 +- packages/cli/src/utils/dialogScopeUtils.ts | 15 +- packages/cli/src/utils/settingsUtils.test.ts | 505 ++---------------- packages/cli/src/utils/settingsUtils.ts | 432 ++++++--------- 14 files changed, 589 insertions(+), 1390 deletions(-) diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index 2e238765e8..88f9f404cd 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -243,7 +243,7 @@ export async function startInteractiveUI( - + - + void; } -/** - * Get a nested value from an object using a path array - */ -function getNestedValue( - obj: Record | undefined, - path: string[], -): unknown { - if (!obj) return undefined; - let current: unknown = obj; - for (const key of path) { - if (current === null || current === undefined) return undefined; - if (typeof current !== 'object') return undefined; - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - current = (current as Record)[key]; - } - return current; -} - /** * Set a nested value in an object using a path array, creating intermediate objects as needed */ -function setNestedValue( - obj: Record, - path: string[], - value: unknown, -): Record { +function setNestedValue(obj: unknown, path: string[], value: unknown): unknown { + if (!isRecord(obj)) return obj; + const result = { ...obj }; let current = result; @@ -144,12 +125,17 @@ function setNestedValue( const key = path[i]; if (current[key] === undefined || current[key] === null) { current[key] = {}; - } else { - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - current[key] = { ...(current[key] as Record) }; + } else if (isRecord(current[key])) { + current[key] = { ...current[key] }; + } + + const next = current[key]; + if (isRecord(next)) { + current = next; + } else { + // Cannot traverse further through non-objects + return result; } - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - current = current[key] as Record; } const finalKey = path[path.length - 1]; @@ -267,11 +253,7 @@ export function AgentConfigDialog({ const items: SettingsDialogItem[] = useMemo( () => AGENT_CONFIG_FIELDS.map((field) => { - const currentValue = getNestedValue( - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - pendingOverride as Record, - field.path, - ); + const currentValue = getNestedValue(pendingOverride, field.path); const defaultValue = getFieldDefaultFromDefinition(field, definition); const effectiveValue = currentValue !== undefined ? currentValue : defaultValue; @@ -324,23 +306,18 @@ export function AgentConfigDialog({ const field = AGENT_CONFIG_FIELDS.find((f) => f.key === key); if (!field || field.type !== 'boolean') return; - const currentValue = getNestedValue( - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - pendingOverride as Record, - field.path, - ); + const currentValue = getNestedValue(pendingOverride, field.path); const defaultValue = getFieldDefaultFromDefinition(field, definition); const effectiveValue = currentValue !== undefined ? currentValue : defaultValue; const newValue = !effectiveValue; + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const newOverride = setNestedValue( - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - pendingOverride as Record, + pendingOverride, field.path, newValue, ) as AgentOverride; - setPendingOverride(newOverride); setModifiedFields((prev) => new Set(prev).add(key)); @@ -375,9 +352,9 @@ export function AgentConfigDialog({ } // Update pending override locally + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const newOverride = setNestedValue( - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - pendingOverride as Record, + pendingOverride, field.path, parsed, ) as AgentOverride; @@ -398,9 +375,9 @@ export function AgentConfigDialog({ if (!field) return; // Remove the override (set to undefined) + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const newOverride = setNestedValue( - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - pendingOverride as Record, + pendingOverride, field.path, undefined, ) as AgentOverride; diff --git a/packages/cli/src/ui/components/DialogManager.tsx b/packages/cli/src/ui/components/DialogManager.tsx index f7f050a53f..3cca19b0b0 100644 --- a/packages/cli/src/ui/components/DialogManager.tsx +++ b/packages/cli/src/ui/components/DialogManager.tsx @@ -281,14 +281,12 @@ export const DialogManager = ({ return ( uiActions.closeSettingsDialog()} onRestartRequest={async () => { await runExitCleanup(); process.exit(RELAUNCH_EXIT_CODE); }} availableTerminalHeight={terminalHeight - staticExtraHeight} - config={config} /> ); diff --git a/packages/cli/src/ui/components/SettingsDialog.test.tsx b/packages/cli/src/ui/components/SettingsDialog.test.tsx index 3dd5374a18..be99dfcc26 100644 --- a/packages/cli/src/ui/components/SettingsDialog.test.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.test.tsx @@ -14,7 +14,6 @@ * - Focus section switching between settings and scope selector * - Scope selection and settings persistence across scopes * - Restart-required vs immediate settings behavior - * - VimModeContext integration * - Complex user interaction workflows * - Error handling and edge cases * - Display values for inherited and overridden settings @@ -25,12 +24,12 @@ import { render } from '../../test-utils/render.js'; import { waitFor } from '../../test-utils/async.js'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { SettingsDialog } from './SettingsDialog.js'; -import { LoadedSettings, SettingScope } from '../../config/settings.js'; +import { SettingScope } from '../../config/settings.js'; import { createMockSettings } from '../../test-utils/settings.js'; -import { VimModeProvider } from '../contexts/VimModeContext.js'; import { KeypressProvider } from '../contexts/KeypressContext.js'; import { act } from 'react'; -import { saveModifiedSettings, TEST_ONLY } from '../../utils/settingsUtils.js'; +import { TEST_ONLY } from '../../utils/settingsUtils.js'; +import { SettingsContext } from '../contexts/SettingsContext.js'; import { getSettingsSchema, type SettingDefinition, @@ -38,10 +37,6 @@ import { } from '../../config/settingsSchema.js'; import { terminalCapabilityManager } from '../utils/terminalCapabilityManager.js'; -// Mock the VimModeContext -const mockToggleVimEnabled = vi.fn().mockResolvedValue(undefined); -const mockSetVimMode = vi.fn(); - vi.mock('../contexts/UIStateContext.js', () => ({ useUIState: () => ({ terminalWidth: 100, // Fixed width for consistent snapshots @@ -68,27 +63,6 @@ vi.mock('../../config/settingsSchema.js', async (importOriginal) => { }; }); -vi.mock('../contexts/VimModeContext.js', async () => { - const actual = await vi.importActual('../contexts/VimModeContext.js'); - return { - ...actual, - useVimMode: () => ({ - vimEnabled: false, - vimMode: 'INSERT' as const, - toggleVimEnabled: mockToggleVimEnabled, - setVimMode: mockSetVimMode, - }), - }; -}); - -vi.mock('../../utils/settingsUtils.js', async () => { - const actual = await vi.importActual('../../utils/settingsUtils.js'); - return { - ...actual, - saveModifiedSettings: vi.fn(), - }; -}); - // Shared test schemas enum StringEnum { FOO = 'foo', @@ -131,6 +105,62 @@ const ENUM_FAKE_SCHEMA: SettingsSchemaType = { }, } as unknown as SettingsSchemaType; +const ARRAY_FAKE_SCHEMA: SettingsSchemaType = { + context: { + type: 'object', + label: 'Context', + category: 'Context', + requiresRestart: false, + default: {}, + description: 'Context settings.', + showInDialog: false, + properties: { + fileFiltering: { + type: 'object', + label: 'File Filtering', + category: 'Context', + requiresRestart: false, + default: {}, + description: 'File filtering settings.', + showInDialog: false, + properties: { + customIgnoreFilePaths: { + type: 'array', + label: 'Custom Ignore File Paths', + category: 'Context', + requiresRestart: false, + default: [] as string[], + description: 'Additional ignore file paths.', + showInDialog: true, + items: { type: 'string' }, + }, + }, + }, + }, + }, + security: { + type: 'object', + label: 'Security', + category: 'Security', + requiresRestart: false, + default: {}, + description: 'Security settings.', + showInDialog: false, + properties: { + allowedExtensions: { + type: 'array', + label: 'Extension Source Regex Allowlist', + category: 'Security', + requiresRestart: false, + default: [] as string[], + description: 'Allowed extension source regex patterns.', + showInDialog: true, + items: { type: 'string' }, + }, + }, + }, +} as unknown as SettingsSchemaType; + const TOOLS_SHELL_FAKE_SCHEMA: SettingsSchemaType = { tools: { type: 'object', @@ -185,7 +215,7 @@ const TOOLS_SHELL_FAKE_SCHEMA: SettingsSchemaType = { // Helper function to render SettingsDialog with standard wrapper const renderDialog = ( - settings: LoadedSettings, + settings: ReturnType, onSelect: ReturnType, options?: { onRestartRequest?: ReturnType; @@ -193,14 +223,15 @@ const renderDialog = ( }, ) => render( - - - , + + + + + , ); describe('SettingsDialog', () => { @@ -210,7 +241,6 @@ describe('SettingsDialog', () => { terminalCapabilityManager, 'isKittyProtocolEnabled', ).mockReturnValue(true); - mockToggleVimEnabled.mockRejectedValue(undefined); }); afterEach(() => { @@ -394,9 +424,8 @@ describe('SettingsDialog', () => { describe('Settings Toggling', () => { it('should toggle setting with Enter key', async () => { - vi.mocked(saveModifiedSettings).mockClear(); - const settings = createMockSettings(); + const setValueSpy = vi.spyOn(settings, 'setValue'); const onSelect = vi.fn(); const { stdin, unmount, lastFrame, waitUntilReady } = renderDialog( @@ -414,29 +443,16 @@ describe('SettingsDialog', () => { await act(async () => { stdin.write(TerminalKeys.ENTER as string); }); - await waitUntilReady(); - // Wait for the setting change to be processed + // Wait for setValue to be called await waitFor(() => { - expect( - vi.mocked(saveModifiedSettings).mock.calls.length, - ).toBeGreaterThan(0); + expect(setValueSpy).toHaveBeenCalled(); }); - // Wait for the mock to be called - await waitFor(() => { - expect(vi.mocked(saveModifiedSettings)).toHaveBeenCalled(); - }); - - expect(vi.mocked(saveModifiedSettings)).toHaveBeenCalledWith( - new Set(['general.vimMode']), - expect.objectContaining({ - general: expect.objectContaining({ - vimMode: true, - }), - }), - expect.any(LoadedSettings), + expect(setValueSpy).toHaveBeenCalledWith( SettingScope.User, + 'general.vimMode', + true, ); unmount(); @@ -455,13 +471,13 @@ describe('SettingsDialog', () => { expectedValue: StringEnum.FOO, }, ])('$name', async ({ initialValue, expectedValue }) => { - vi.mocked(saveModifiedSettings).mockClear(); vi.mocked(getSettingsSchema).mockReturnValue(ENUM_FAKE_SCHEMA); const settings = createMockSettings(); if (initialValue !== undefined) { settings.setValue(SettingScope.User, 'ui.theme', initialValue); } + const setValueSpy = vi.spyOn(settings, 'setValue'); const onSelect = vi.fn(); @@ -482,20 +498,13 @@ describe('SettingsDialog', () => { await waitUntilReady(); await waitFor(() => { - expect(vi.mocked(saveModifiedSettings)).toHaveBeenCalled(); + expect(setValueSpy).toHaveBeenCalledWith( + SettingScope.User, + 'ui.theme', + expectedValue, + ); }); - expect(vi.mocked(saveModifiedSettings)).toHaveBeenCalledWith( - new Set(['ui.theme']), - expect.objectContaining({ - ui: expect.objectContaining({ - theme: expectedValue, - }), - }), - expect.any(LoadedSettings), - SettingScope.User, - ); - unmount(); }); }); @@ -692,30 +701,6 @@ describe('SettingsDialog', () => { }); }); - describe('Error Handling', () => { - it('should handle vim mode toggle errors gracefully', async () => { - mockToggleVimEnabled.mockRejectedValue(new Error('Toggle failed')); - - const settings = createMockSettings(); - const onSelect = vi.fn(); - - const { stdin, unmount, waitUntilReady } = renderDialog( - settings, - onSelect, - ); - await waitUntilReady(); - - // Try to toggle a setting (this might trigger vim mode toggle) - await act(async () => { - stdin.write(TerminalKeys.ENTER as string); // Enter - }); - await waitUntilReady(); - - // Should not crash - unmount(); - }); - }); - describe('Complex State Management', () => { it('should track modified settings correctly', async () => { const settings = createMockSettings(); @@ -767,31 +752,6 @@ describe('SettingsDialog', () => { }); }); - describe('VimMode Integration', () => { - it('should sync with VimModeContext when vim mode is toggled', async () => { - const settings = createMockSettings(); - const onSelect = vi.fn(); - - const { stdin, unmount, waitUntilReady } = render( - - - - - , - ); - await waitUntilReady(); - - // Navigate to and toggle vim mode setting - // This would require knowing the exact position of vim mode setting - await act(async () => { - stdin.write(TerminalKeys.ENTER as string); // Enter - }); - await waitUntilReady(); - - unmount(); - }); - }); - describe('Specific Settings Behavior', () => { it('should show correct display values for settings with different states', async () => { const settings = createMockSettings({ @@ -861,7 +821,7 @@ describe('SettingsDialog', () => { // Should not show restart prompt initially await waitFor(() => { expect(lastFrame()).not.toContain( - 'To see changes, Gemini CLI must be restarted', + 'Changes that require a restart have been modified', ); }); @@ -957,63 +917,41 @@ describe('SettingsDialog', () => { pager: 'less', }, }, - ])( - 'should $name', - async ({ toggleCount, shellSettings, expectedSiblings }) => { - vi.mocked(saveModifiedSettings).mockClear(); + ])('should $name', async ({ toggleCount, shellSettings }) => { + vi.mocked(getSettingsSchema).mockReturnValue(TOOLS_SHELL_FAKE_SCHEMA); - vi.mocked(getSettingsSchema).mockReturnValue(TOOLS_SHELL_FAKE_SCHEMA); + const settings = createMockSettings({ + tools: { + shell: shellSettings, + }, + }); + const setValueSpy = vi.spyOn(settings, 'setValue'); - const settings = createMockSettings({ - tools: { - shell: shellSettings, - }, + const onSelect = vi.fn(); + + const { stdin, unmount } = renderDialog(settings, onSelect); + + for (let i = 0; i < toggleCount; i++) { + act(() => { + stdin.write(TerminalKeys.ENTER as string); }); + } - const onSelect = vi.fn(); + await waitFor(() => { + expect(setValueSpy).toHaveBeenCalled(); + }); - const { stdin, unmount, waitUntilReady } = renderDialog( - settings, - onSelect, - ); - await waitUntilReady(); + // With the store pattern, setValue is called atomically per key. + // Sibling preservation is handled by LoadedSettings internally. + const calls = setValueSpy.mock.calls; + expect(calls.length).toBeGreaterThan(0); + calls.forEach((call) => { + // Each call should target only 'tools.shell.showColor' + expect(call[1]).toBe('tools.shell.showColor'); + }); - for (let i = 0; i < toggleCount; i++) { - await act(async () => { - stdin.write(TerminalKeys.ENTER as string); - }); - await waitUntilReady(); - } - - await waitFor(() => { - expect( - vi.mocked(saveModifiedSettings).mock.calls.length, - ).toBeGreaterThan(0); - }); - - const calls = vi.mocked(saveModifiedSettings).mock.calls; - calls.forEach((call) => { - const [modifiedKeys, pendingSettings] = call; - - if (modifiedKeys.has('tools.shell.showColor')) { - const shellSettings = pendingSettings.tools?.shell as - | Record - | undefined; - - Object.entries(expectedSiblings).forEach(([key, value]) => { - expect(shellSettings?.[key]).toBe(value); - expect(modifiedKeys.has(`tools.shell.${key}`)).toBe(false); - }); - - expect(modifiedKeys.size).toBe(1); - } - }); - - expect(calls.length).toBeGreaterThan(0); - - unmount(); - }, - ); + unmount(); + }); }); describe('Keyboard Shortcuts Edge Cases', () => { @@ -1319,7 +1257,7 @@ describe('SettingsDialog', () => { await waitFor(() => { expect(lastFrame()).toContain( - 'To see changes, Gemini CLI must be restarted', + 'Changes that require a restart have been modified', ); }); @@ -1366,7 +1304,7 @@ describe('SettingsDialog', () => { await waitFor(() => { expect(lastFrame()).toContain( - 'To see changes, Gemini CLI must be restarted', + 'Changes that require a restart have been modified', ); }); @@ -1385,9 +1323,11 @@ describe('SettingsDialog', () => { const onSelect = vi.fn(); const { stdin, unmount, rerender, waitUntilReady } = render( - - - , + + + + + , ); await waitUntilReady(); @@ -1424,14 +1364,13 @@ describe('SettingsDialog', () => { path: '', }, }); - await act(async () => { - rerender( + rerender( + - - , - ); - }); - await waitUntilReady(); + + + , + ); // Press Escape to exit await act(async () => { @@ -1447,6 +1386,74 @@ describe('SettingsDialog', () => { }); }); + describe('Array Settings Editing', () => { + const typeInput = async ( + stdin: { write: (data: string) => void }, + input: string, + ) => { + for (const ch of input) { + await act(async () => { + stdin.write(ch); + }); + } + }; + + it('should parse comma-separated input as string arrays', async () => { + vi.mocked(getSettingsSchema).mockReturnValue(ARRAY_FAKE_SCHEMA); + const settings = createMockSettings(); + const setValueSpy = vi.spyOn(settings, 'setValue'); + + const { stdin, unmount } = renderDialog(settings, vi.fn()); + + await act(async () => { + stdin.write(TerminalKeys.ENTER as string); // Start editing first array setting + }); + await typeInput(stdin, 'first/path, second/path,third/path'); + await act(async () => { + stdin.write(TerminalKeys.ENTER as string); // Commit + }); + + await waitFor(() => { + expect(setValueSpy).toHaveBeenCalledWith( + SettingScope.User, + 'context.fileFiltering.customIgnoreFilePaths', + ['first/path', 'second/path', 'third/path'], + ); + }); + + unmount(); + }); + + it('should parse JSON array input for allowedExtensions', async () => { + vi.mocked(getSettingsSchema).mockReturnValue(ARRAY_FAKE_SCHEMA); + const settings = createMockSettings(); + const setValueSpy = vi.spyOn(settings, 'setValue'); + + const { stdin, unmount } = renderDialog(settings, vi.fn()); + + await act(async () => { + stdin.write(TerminalKeys.DOWN_ARROW as string); // Move to second array setting + }); + await act(async () => { + stdin.write(TerminalKeys.ENTER as string); // Start editing + }); + await typeInput(stdin, '["^github\\\\.com/.*$", "^gitlab\\\\.com/.*$"]'); + await act(async () => { + stdin.write(TerminalKeys.ENTER as string); // Commit + }); + + await waitFor(() => { + expect(setValueSpy).toHaveBeenCalledWith( + SettingScope.User, + 'security.allowedExtensions', + ['^github\\.com/.*$', '^gitlab\\.com/.*$'], + ); + }); + + unmount(); + }); + }); + describe('Search Functionality', () => { it('should display text entered in search', async () => { const settings = createMockSettings(); diff --git a/packages/cli/src/ui/components/SettingsDialog.tsx b/packages/cli/src/ui/components/SettingsDialog.tsx index e426e9bbe3..23e8a55a7d 100644 --- a/packages/cli/src/ui/components/SettingsDialog.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.tsx @@ -5,40 +5,35 @@ */ import type React from 'react'; -import { useState, useEffect, useMemo, useCallback } from 'react'; +import { useState, useMemo, useCallback, useEffect } from 'react'; import { Text } from 'ink'; import { AsyncFzf } from 'fzf'; import type { Key } from '../hooks/useKeypress.js'; import { theme } from '../semantic-colors.js'; -import type { - LoadableSettingScope, - LoadedSettings, - Settings, -} from '../../config/settings.js'; +import type { LoadableSettingScope, Settings } from '../../config/settings.js'; import { SettingScope } from '../../config/settings.js'; import { getScopeMessageForSetting } from '../../utils/dialogScopeUtils.js'; import { getDialogSettingKeys, - setPendingSettingValue, getDisplayValue, - hasRestartRequiredSettings, - saveModifiedSettings, getSettingDefinition, - isDefaultValue, - requiresRestart, - getRestartRequiredFromModified, - getEffectiveDefaultValue, - setPendingSettingValueAny, + getDialogRestartRequiredSettings, getEffectiveValue, + isInSettingsScope, + getEditValue, + parseEditedValue, } from '../../utils/settingsUtils.js'; -import { useVimMode } from '../contexts/VimModeContext.js'; +import { + useSettingsStore, + type SettingsState, +} from '../contexts/SettingsContext.js'; import { getCachedStringWidth } from '../utils/textUtils.js'; import { + type SettingsType, type SettingsValue, TOGGLE_TYPES, } from '../../config/settingsSchema.js'; -import { coreEvents, debugLogger } from '@google/gemini-cli-core'; -import type { Config } from '@google/gemini-cli-core'; +import { debugLogger } from '@google/gemini-cli-core'; import { useSearchBuffer } from '../hooks/useSearchBuffer.js'; import { @@ -55,31 +50,56 @@ interface FzfResult { } interface SettingsDialogProps { - settings: LoadedSettings; onSelect: (settingName: string | undefined, scope: SettingScope) => void; onRestartRequest?: () => void; availableTerminalHeight?: number; - config?: Config; } const MAX_ITEMS_TO_SHOW = 8; +// Create a snapshot of the initial per-scope state of Restart Required Settings +// This creates a nested map of the form +// restartRequiredSetting -> Map { scopeName -> value } +function getActiveRestartRequiredSettings( + settings: SettingsState, +): Map> { + const snapshot = new Map>(); + const scopes: Array<[string, Settings]> = [ + ['User', settings.user.settings], + ['Workspace', settings.workspace.settings], + ['System', settings.system.settings], + ]; + + for (const key of getDialogRestartRequiredSettings()) { + const scopeMap = new Map(); + for (const [scopeName, scopeSettings] of scopes) { + // Raw per-scope value (undefined if not in file) + const value = isInSettingsScope(key, scopeSettings) + ? getEffectiveValue(key, scopeSettings) + : undefined; + scopeMap.set(scopeName, JSON.stringify(value)); + } + snapshot.set(key, scopeMap); + } + return snapshot; +} + export function SettingsDialog({ - settings, onSelect, onRestartRequest, availableTerminalHeight, - config, }: SettingsDialogProps): React.JSX.Element { - // Get vim mode context to sync vim mode changes - const { vimEnabled, toggleVimEnabled } = useVimMode(); + // Reactive settings from store (re-renders on any settings change) + const { settings, setSetting } = useSettingsStore(); - // Scope selector state (User by default) const [selectedScope, setSelectedScope] = useState( SettingScope.User, ); - const [showRestartPrompt, setShowRestartPrompt] = useState(false); + // Snapshot restart-required values at mount time for diff tracking + const [activeRestartRequiredSettings] = useState(() => + getActiveRestartRequiredSettings(settings), + ); // Search state const [searchQuery, setSearchQuery] = useState(''); @@ -136,52 +156,34 @@ export function SettingsDialog({ }; }, [searchQuery, fzfInstance, searchMap]); - // Local pending settings state for the selected scope - const [pendingSettings, setPendingSettings] = useState(() => - // Deep clone to avoid mutation - structuredClone(settings.forScope(selectedScope).settings), - ); + // Track whether a restart is required to apply the changes in the Settings json file + // This does not care for inheritance + // It checks whether a proposed change from this UI to a settings.json file requires a restart to take effect in the app + const pendingRestartRequiredSettings = useMemo(() => { + const changed = new Set(); + const scopes: Array<[string, Settings]> = [ + ['User', settings.user.settings], + ['Workspace', settings.workspace.settings], + ['System', settings.system.settings], + ]; - // Track which settings have been modified by the user - const [modifiedSettings, setModifiedSettings] = useState>( - new Set(), - ); - - // Preserve pending changes across scope switches - type PendingValue = boolean | number | string; - const [globalPendingChanges, setGlobalPendingChanges] = useState< - Map - >(new Map()); - - // Track restart-required settings across scope changes - const [_restartRequiredSettings, setRestartRequiredSettings] = useState< - Set - >(new Set()); - - useEffect(() => { - // Base settings for selected scope - let updated = structuredClone(settings.forScope(selectedScope).settings); - // Overlay globally pending (unsaved) changes so user sees their modifications in any scope - const newModified = new Set(); - const newRestartRequired = new Set(); - for (const [key, value] of globalPendingChanges.entries()) { - const def = getSettingDefinition(key); - if (def?.type === 'boolean' && typeof value === 'boolean') { - updated = setPendingSettingValue(key, value, updated); - } else if ( - (def?.type === 'number' && typeof value === 'number') || - (def?.type === 'string' && typeof value === 'string') - ) { - updated = setPendingSettingValueAny(key, value, updated); + // Iterate through the nested map snapshot in activeRestartRequiredSettings, diff with current settings + for (const [key, initialScopeMap] of activeRestartRequiredSettings) { + for (const [scopeName, scopeSettings] of scopes) { + const currentValue = isInSettingsScope(key, scopeSettings) + ? getEffectiveValue(key, scopeSettings) + : undefined; + const initialJson = initialScopeMap.get(scopeName); + if (JSON.stringify(currentValue) !== initialJson) { + changed.add(key); + break; // one scope changed is enough + } } - newModified.add(key); - if (requiresRestart(key)) newRestartRequired.add(key); } - setPendingSettings(updated); - setModifiedSettings(newModified); - setRestartRequiredSettings(newRestartRequired); - setShowRestartPrompt(newRestartRequired.size > 0); - }, [selectedScope, settings, globalPendingChanges]); + return changed; + }, [settings, activeRestartRequiredSettings]); + + const showRestartPrompt = pendingRestartRequiredSettings.size > 0; // Calculate max width for the left column (Label/Description) to keep values aligned or close const maxLabelOrDescriptionWidth = useMemo(() => { @@ -222,16 +224,10 @@ export function SettingsDialog({ return settingKeys.map((key) => { const definition = getSettingDefinition(key); - const type = definition?.type ?? 'string'; + const type: SettingsType = definition?.type ?? 'string'; // Get the display value (with * indicator if modified) - const displayValue = getDisplayValue( - key, - scopeSettings, - mergedSettings, - modifiedSettings, - pendingSettings, - ); + const displayValue = getDisplayValue(key, scopeSettings, mergedSettings); // Get the scope message (e.g., "(Modified in Workspace)") const scopeMessage = getScopeMessageForSetting( @@ -240,28 +236,28 @@ export function SettingsDialog({ settings, ); - // Check if the value is at default (grey it out) - const isGreyedOut = isDefaultValue(key, scopeSettings); + // Grey out values that defer to defaults + const isGreyedOut = !isInSettingsScope(key, scopeSettings); - // Get raw value for edit mode initialization - const rawValue = getEffectiveValue(key, pendingSettings, {}); + // Some settings can be edited by an inline editor + const rawValue = getEffectiveValue(key, scopeSettings); + // The inline editor needs a string but non primitive settings like Arrays and Objects exist + const editValue = getEditValue(type, rawValue); return { key, label: definition?.label || key, description: definition?.description, - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - type: type as 'boolean' | 'number' | 'string' | 'enum', + type, displayValue, isGreyedOut, scopeMessage, - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - rawValue: rawValue as string | number | boolean | undefined, + rawValue, + editValue, }; }); - }, [settingKeys, selectedScope, settings, modifiedSettings, pendingSettings]); + }, [settingKeys, selectedScope, settings]); - // Scope selection handler const handleScopeChange = useCallback((scope: LoadableSettingScope) => { setSelectedScope(scope); }, []); @@ -273,17 +269,21 @@ export function SettingsDialog({ if (!TOGGLE_TYPES.has(definition?.type)) { return; } - const currentValue = getEffectiveValue(key, pendingSettings, {}); + + const scopeSettings = settings.forScope(selectedScope).settings; + const currentValue = getEffectiveValue(key, scopeSettings); let newValue: SettingsValue; + if (definition?.type === 'boolean') { - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - newValue = !(currentValue as boolean); - setPendingSettings((prev) => - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - setPendingSettingValue(key, newValue as boolean, prev), - ); + if (typeof currentValue !== 'boolean') { + return; + } + newValue = !currentValue; } else if (definition?.type === 'enum' && definition.options) { const options = definition.options; + if (options.length === 0) { + return; + } const currentIndex = options?.findIndex( (opt) => opt.value === currentValue, ); @@ -292,303 +292,58 @@ export function SettingsDialog({ } else { newValue = options[0].value; // loop back to start. } - setPendingSettings((prev) => - setPendingSettingValueAny(key, newValue, prev), - ); - } - - if (!requiresRestart(key)) { - const immediateSettings = new Set([key]); - const currentScopeSettings = settings.forScope(selectedScope).settings; - const immediateSettingsObject = setPendingSettingValueAny( - key, - newValue, - currentScopeSettings, - ); - debugLogger.log( - `[DEBUG SettingsDialog] Saving ${key} immediately with value:`, - newValue, - ); - saveModifiedSettings( - immediateSettings, - immediateSettingsObject, - settings, - selectedScope, - ); - - // Special handling for vim mode to sync with VimModeContext - if (key === 'general.vimMode' && newValue !== vimEnabled) { - // Call toggleVimEnabled to sync the VimModeContext local state - toggleVimEnabled().catch((error) => { - coreEvents.emitFeedback( - 'error', - 'Failed to toggle vim mode:', - error, - ); - }); - } - - // Remove from modifiedSettings since it's now saved - setModifiedSettings((prev) => { - const updated = new Set(prev); - updated.delete(key); - return updated; - }); - - // Also remove from restart-required settings if it was there - setRestartRequiredSettings((prev) => { - const updated = new Set(prev); - updated.delete(key); - return updated; - }); - - // Remove from global pending changes if present - setGlobalPendingChanges((prev) => { - if (!prev.has(key)) return prev; - const next = new Map(prev); - next.delete(key); - return next; - }); } else { - // For restart-required settings, track as modified - setModifiedSettings((prev) => { - const updated = new Set(prev).add(key); - const needsRestart = hasRestartRequiredSettings(updated); - debugLogger.log( - `[DEBUG SettingsDialog] Modified settings:`, - Array.from(updated), - 'Needs restart:', - needsRestart, - ); - if (needsRestart) { - setShowRestartPrompt(true); - setRestartRequiredSettings((prevRestart) => - new Set(prevRestart).add(key), - ); - } - return updated; - }); - - // Record pending change globally - setGlobalPendingChanges((prev) => { - const next = new Map(prev); - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - next.set(key, newValue as PendingValue); - return next; - }); - } - }, - [pendingSettings, settings, selectedScope, vimEnabled, toggleVimEnabled], - ); - - // Edit commit handler - const handleEditCommit = useCallback( - (key: string, newValue: string, _item: SettingsDialogItem) => { - const definition = getSettingDefinition(key); - const type = definition?.type; - - if (newValue.trim() === '' && type === 'number') { - // Nothing entered for a number; cancel edit return; } - let parsed: string | number; - if (type === 'number') { - const numParsed = Number(newValue.trim()); - if (Number.isNaN(numParsed)) { - // Invalid number; cancel edit - return; - } - parsed = numParsed; - } else { - // For strings, use the buffer as is. - parsed = newValue; - } - - // Update pending - setPendingSettings((prev) => - setPendingSettingValueAny(key, parsed, prev), + debugLogger.log( + `[DEBUG SettingsDialog] Saving ${key} immediately with value:`, + newValue, ); - - if (!requiresRestart(key)) { - const immediateSettings = new Set([key]); - const currentScopeSettings = settings.forScope(selectedScope).settings; - const immediateSettingsObject = setPendingSettingValueAny( - key, - parsed, - currentScopeSettings, - ); - saveModifiedSettings( - immediateSettings, - immediateSettingsObject, - settings, - selectedScope, - ); - - // Remove from modified sets if present - setModifiedSettings((prev) => { - const updated = new Set(prev); - updated.delete(key); - return updated; - }); - setRestartRequiredSettings((prev) => { - const updated = new Set(prev); - updated.delete(key); - return updated; - }); - - // Remove from global pending since it's immediately saved - setGlobalPendingChanges((prev) => { - if (!prev.has(key)) return prev; - const next = new Map(prev); - next.delete(key); - return next; - }); - } else { - // Mark as modified and needing restart - setModifiedSettings((prev) => { - const updated = new Set(prev).add(key); - const needsRestart = hasRestartRequiredSettings(updated); - if (needsRestart) { - setShowRestartPrompt(true); - setRestartRequiredSettings((prevRestart) => - new Set(prevRestart).add(key), - ); - } - return updated; - }); - - // Record pending change globally for persistence across scopes - setGlobalPendingChanges((prev) => { - const next = new Map(prev); - next.set(key, parsed as PendingValue); - return next; - }); - } + setSetting(selectedScope, key, newValue); }, - [settings, selectedScope], + [settings, selectedScope, setSetting], + ); + + // For inline editor + const handleEditCommit = useCallback( + (key: string, newValue: string, _item: SettingsDialogItem) => { + const definition = getSettingDefinition(key); + const type: SettingsType = definition?.type ?? 'string'; + const parsed = parseEditedValue(type, newValue); + + if (parsed === null) { + return; + } + + setSetting(selectedScope, key, parsed); + }, + [selectedScope, setSetting], ); // Clear/reset handler - removes the value from settings.json so it falls back to default const handleItemClear = useCallback( (key: string, _item: SettingsDialogItem) => { - const defaultValue = getEffectiveDefaultValue(key, config); - - // Update local pending state to show the default value - if (typeof defaultValue === 'boolean') { - setPendingSettings((prev) => - setPendingSettingValue(key, defaultValue, prev), - ); - } else if ( - typeof defaultValue === 'number' || - typeof defaultValue === 'string' - ) { - setPendingSettings((prev) => - setPendingSettingValueAny(key, defaultValue, prev), - ); - } - - // Clear the value from settings.json (set to undefined to remove the key) - if (!requiresRestart(key)) { - settings.setValue(selectedScope, key, undefined); - - // Special handling for vim mode - if (key === 'general.vimMode') { - const booleanDefaultValue = - typeof defaultValue === 'boolean' ? defaultValue : false; - if (booleanDefaultValue !== vimEnabled) { - toggleVimEnabled().catch((error) => { - coreEvents.emitFeedback( - 'error', - 'Failed to toggle vim mode:', - error, - ); - }); - } - } - } - - // Remove from modified sets - setModifiedSettings((prev) => { - const updated = new Set(prev); - updated.delete(key); - return updated; - }); - setRestartRequiredSettings((prev) => { - const updated = new Set(prev); - updated.delete(key); - return updated; - }); - setGlobalPendingChanges((prev) => { - if (!prev.has(key)) return prev; - const next = new Map(prev); - next.delete(key); - return next; - }); - - // Update restart prompt - setShowRestartPrompt((_prev) => { - const remaining = getRestartRequiredFromModified(modifiedSettings); - return remaining.filter((k) => k !== key).length > 0; - }); + setSetting(selectedScope, key, undefined); }, - [ - config, - settings, - selectedScope, - vimEnabled, - toggleVimEnabled, - modifiedSettings, - ], + [selectedScope, setSetting], ); - const saveRestartRequiredSettings = useCallback(() => { - const restartRequiredSettings = - getRestartRequiredFromModified(modifiedSettings); - const restartRequiredSet = new Set(restartRequiredSettings); - - if (restartRequiredSet.size > 0) { - saveModifiedSettings( - restartRequiredSet, - pendingSettings, - settings, - selectedScope, - ); - - // Remove saved keys from global pending changes - setGlobalPendingChanges((prev) => { - if (prev.size === 0) return prev; - const next = new Map(prev); - for (const key of restartRequiredSet) { - next.delete(key); - } - return next; - }); - } - }, [modifiedSettings, pendingSettings, settings, selectedScope]); - - // Close handler const handleClose = useCallback(() => { - // Save any restart-required settings before closing - saveRestartRequiredSettings(); onSelect(undefined, selectedScope as SettingScope); - }, [saveRestartRequiredSettings, onSelect, selectedScope]); + }, [onSelect, selectedScope]); // Custom key handler for restart key const handleKeyPress = useCallback( (key: Key, _currentItem: SettingsDialogItem | undefined): boolean => { // 'r' key for restart if (showRestartPrompt && key.sequence === 'r') { - saveRestartRequiredSettings(); - setShowRestartPrompt(false); - setModifiedSettings(new Set()); - setRestartRequiredSettings(new Set()); if (onRestartRequest) onRestartRequest(); return true; } return false; }, - [showRestartPrompt, onRestartRequest, saveRestartRequiredSettings], + [showRestartPrompt, onRestartRequest], ); // Calculate effective max items and scope visibility based on terminal height @@ -673,11 +428,10 @@ export function SettingsDialog({ showRestartPrompt, ]); - // Footer content for restart prompt const footerContent = showRestartPrompt ? ( - To see changes, Gemini CLI must be restarted. Press r to exit and apply - changes now. + Changes that require a restart have been modified. Press r to exit and + apply changes now. ) : null; diff --git a/packages/cli/src/ui/components/shared/BaseSettingsDialog.test.tsx b/packages/cli/src/ui/components/shared/BaseSettingsDialog.test.tsx index fbbc6ff517..4047ec9ef8 100644 --- a/packages/cli/src/ui/components/shared/BaseSettingsDialog.test.tsx +++ b/packages/cli/src/ui/components/shared/BaseSettingsDialog.test.tsx @@ -531,6 +531,37 @@ describe('BaseSettingsDialog', () => { }); describe('edit mode', () => { + it('should prioritize editValue over rawValue stringification', async () => { + const objectItem: SettingsDialogItem = { + key: 'object-setting', + label: 'Object Setting', + description: 'A complex object setting', + displayValue: '{"foo":"bar"}', + type: 'object', + rawValue: { foo: 'bar' }, + editValue: '{"foo":"bar"}', + }; + const { stdin } = await renderDialog({ + items: [objectItem], + }); + + // Enter edit mode and immediately commit + await act(async () => { + stdin.write(TerminalKeys.ENTER); + }); + await act(async () => { + stdin.write(TerminalKeys.ENTER); + }); + + await waitFor(() => { + expect(mockOnEditCommit).toHaveBeenCalledWith( + 'object-setting', + '{"foo":"bar"}', + expect.objectContaining({ type: 'object' }), + ); + }); + }); + it('should commit edit on Enter', async () => { const items = createMockItems(4); const stringItem = items.find((i) => i.type === 'string')!; diff --git a/packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx b/packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx index 29592b479b..58f15aa85a 100644 --- a/packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx +++ b/packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx @@ -9,6 +9,10 @@ import { Box, Text } from 'ink'; import chalk from 'chalk'; import { theme } from '../../semantic-colors.js'; import type { LoadableSettingScope } from '../../../config/settings.js'; +import type { + SettingsType, + SettingsValue, +} from '../../../config/settingsSchema.js'; import { getScopeItems } from '../../../utils/dialogScopeUtils.js'; import { RadioButtonSelect } from './RadioButtonSelect.js'; import { TextInput } from './TextInput.js'; @@ -33,7 +37,7 @@ export interface SettingsDialogItem { /** Optional description below label */ description?: string; /** Item type for determining interaction behavior */ - type: 'boolean' | 'number' | 'string' | 'enum'; + type: SettingsType; /** Pre-formatted display value (with * if modified) */ displayValue: string; /** Grey out value (at default) */ @@ -41,7 +45,9 @@ export interface SettingsDialogItem { /** Scope message e.g., "(Modified in Workspace)" */ scopeMessage?: string; /** Raw value for edit mode initialization */ - rawValue?: string | number | boolean; + rawValue?: SettingsValue; + /** Optional pre-formatted edit buffer value for complex types */ + editValue?: string; } /** @@ -381,9 +387,11 @@ export function BaseSettingsDialog({ if (currentItem.type === 'boolean' || currentItem.type === 'enum') { onItemToggle(currentItem.key, currentItem); } else { - // Start editing for string/number + // Start editing for string/number/array/object const rawVal = currentItem.rawValue; - const initialValue = rawVal !== undefined ? String(rawVal) : ''; + const initialValue = + currentItem.editValue ?? + (rawVal !== undefined ? String(rawVal) : ''); startEditing(currentItem.key, initialValue); } return true; diff --git a/packages/cli/src/ui/contexts/SettingsContext.tsx b/packages/cli/src/ui/contexts/SettingsContext.tsx index 2c5ae37dfd..259f4c21a2 100644 --- a/packages/cli/src/ui/contexts/SettingsContext.tsx +++ b/packages/cli/src/ui/contexts/SettingsContext.tsx @@ -12,6 +12,7 @@ import type { SettingsFile, } from '../../config/settings.js'; import { SettingScope } from '../../config/settings.js'; +import { checkExhaustive } from '@google/gemini-cli-core'; export const SettingsContext = React.createContext( undefined, @@ -66,7 +67,7 @@ export const useSettingsStore = (): SettingsStoreValue => { case SettingScope.SystemDefaults: return snapshot.systemDefaults; default: - throw new Error(`Invalid scope: ${scope}`); + checkExhaustive(scope); } }, }), diff --git a/packages/cli/src/ui/contexts/VimModeContext.tsx b/packages/cli/src/ui/contexts/VimModeContext.tsx index d4495846d2..7f7a7ea2a3 100644 --- a/packages/cli/src/ui/contexts/VimModeContext.tsx +++ b/packages/cli/src/ui/contexts/VimModeContext.tsx @@ -4,15 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { - createContext, - useCallback, - useContext, - useEffect, - useState, -} from 'react'; -import type { LoadedSettings } from '../../config/settings.js'; +import { createContext, useCallback, useContext, useState } from 'react'; import { SettingScope } from '../../config/settings.js'; +import { useSettingsStore } from './SettingsContext.js'; export type VimMode = 'NORMAL' | 'INSERT'; @@ -27,35 +21,22 @@ const VimModeContext = createContext(undefined); export const VimModeProvider = ({ children, - settings, }: { children: React.ReactNode; - settings: LoadedSettings; }) => { - const initialVimEnabled = settings.merged.general.vimMode; - const [vimEnabled, setVimEnabled] = useState(initialVimEnabled); + const { settings, setSetting } = useSettingsStore(); + const vimEnabled = settings.merged.general.vimMode; const [vimMode, setVimMode] = useState('INSERT'); - useEffect(() => { - // Initialize vimEnabled from settings on mount - const enabled = settings.merged.general.vimMode; - setVimEnabled(enabled); - // When vim mode is enabled, start in INSERT mode - if (enabled) { - setVimMode('INSERT'); - } - }, [settings.merged.general.vimMode]); - const toggleVimEnabled = useCallback(async () => { const newValue = !vimEnabled; - setVimEnabled(newValue); // When enabling vim mode, start in INSERT mode if (newValue) { setVimMode('INSERT'); } - settings.setValue(SettingScope.User, 'general.vimMode', newValue); + setSetting(SettingScope.User, 'general.vimMode', newValue); return newValue; - }, [vimEnabled, settings]); + }, [vimEnabled, setSetting]); const value = { vimEnabled, diff --git a/packages/cli/src/utils/dialogScopeUtils.test.ts b/packages/cli/src/utils/dialogScopeUtils.test.ts index a2032bda6d..ab4a69886e 100644 --- a/packages/cli/src/utils/dialogScopeUtils.test.ts +++ b/packages/cli/src/utils/dialogScopeUtils.test.ts @@ -11,7 +11,7 @@ import { getScopeItems, getScopeMessageForSetting, } from './dialogScopeUtils.js'; -import { settingExistsInScope } from './settingsUtils.js'; +import { isInSettingsScope } from './settingsUtils.js'; vi.mock('../config/settings', () => ({ SettingScope: { @@ -24,7 +24,7 @@ vi.mock('../config/settings', () => ({ })); vi.mock('./settingsUtils', () => ({ - settingExistsInScope: vi.fn(), + isInSettingsScope: vi.fn(), })); describe('dialogScopeUtils', () => { @@ -53,7 +53,7 @@ describe('dialogScopeUtils', () => { }); it('should return empty string if not modified in other scopes', () => { - vi.mocked(settingExistsInScope).mockReturnValue(false); + vi.mocked(isInSettingsScope).mockReturnValue(false); const message = getScopeMessageForSetting( 'key', SettingScope.User, @@ -63,7 +63,7 @@ describe('dialogScopeUtils', () => { }); it('should return message indicating modification in other scopes', () => { - vi.mocked(settingExistsInScope).mockReturnValue(true); + vi.mocked(isInSettingsScope).mockReturnValue(true); const message = getScopeMessageForSetting( 'key', @@ -88,7 +88,7 @@ describe('dialogScopeUtils', () => { return { settings: {} }; }); - vi.mocked(settingExistsInScope).mockImplementation( + vi.mocked(isInSettingsScope).mockImplementation( (_key, settings: unknown) => { if (settings === workspaceSettings) return true; if (settings === systemSettings) return false; diff --git a/packages/cli/src/utils/dialogScopeUtils.ts b/packages/cli/src/utils/dialogScopeUtils.ts index ccf93b6a68..35c1d41917 100644 --- a/packages/cli/src/utils/dialogScopeUtils.ts +++ b/packages/cli/src/utils/dialogScopeUtils.ts @@ -4,12 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { - LoadableSettingScope, - LoadedSettings, -} from '../config/settings.js'; +import type { LoadableSettingScope, Settings } from '../config/settings.js'; import { isLoadableSettingScope, SettingScope } from '../config/settings.js'; -import { settingExistsInScope } from './settingsUtils.js'; +import { isInSettingsScope } from './settingsUtils.js'; /** * Shared scope labels for dialog components that need to display setting scopes @@ -43,7 +40,9 @@ export function getScopeItems(): Array<{ export function getScopeMessageForSetting( settingKey: string, selectedScope: LoadableSettingScope, - settings: LoadedSettings, + settings: { + forScope: (scope: LoadableSettingScope) => { settings: Settings }; + }, ): string { const otherScopes = Object.values(SettingScope) .filter(isLoadableSettingScope) @@ -51,7 +50,7 @@ export function getScopeMessageForSetting( const modifiedInOtherScopes = otherScopes.filter((scope) => { const scopeSettings = settings.forScope(scope).settings; - return settingExistsInScope(settingKey, scopeSettings); + return isInSettingsScope(settingKey, scopeSettings); }); if (modifiedInOtherScopes.length === 0) { @@ -60,7 +59,7 @@ export function getScopeMessageForSetting( const modifiedScopesStr = modifiedInOtherScopes.join(', '); const currentScopeSettings = settings.forScope(selectedScope).settings; - const existsInCurrentScope = settingExistsInScope( + const existsInCurrentScope = isInSettingsScope( settingKey, currentScopeSettings, ); diff --git a/packages/cli/src/utils/settingsUtils.test.ts b/packages/cli/src/utils/settingsUtils.test.ts index 75bdeb65e6..d06743a4e9 100644 --- a/packages/cli/src/utils/settingsUtils.test.ts +++ b/packages/cli/src/utils/settingsUtils.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { // Schema utilities getSettingsByCategory, @@ -22,18 +22,10 @@ import { getDialogSettingsByCategory, getDialogSettingsByType, getDialogSettingKeys, - // Business logic utilities - getSettingValue, - isSettingModified, + // Business logic utilities, TEST_ONLY, - settingExistsInScope, - setPendingSettingValue, - hasRestartRequiredSettings, - getRestartRequiredFromModified, + isInSettingsScope, getDisplayValue, - isDefaultValue, - isValueInherited, - getEffectiveDisplayValue, } from './settingsUtils.js'; import { getSettingsSchema, @@ -255,41 +247,15 @@ describe('SettingsUtils', () => { describe('getEffectiveValue', () => { it('should return value from settings when set', () => { const settings = makeMockSettings({ ui: { requiresRestart: true } }); - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: false }, - }); - const value = getEffectiveValue( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(value).toBe(true); - }); - - it('should return value from merged settings when not set in current scope', () => { - const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: true }, - }); - - const value = getEffectiveValue( - 'ui.requiresRestart', - settings, - mergedSettings, - ); + const value = getEffectiveValue('ui.requiresRestart', settings); expect(value).toBe(true); }); it('should return default value when not set anywhere', () => { const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({}); - const value = getEffectiveValue( - 'ui.requiresRestart', - settings, - mergedSettings, - ); + const value = getEffectiveValue('ui.requiresRestart', settings); expect(value).toBe(false); // default value }); @@ -297,27 +263,18 @@ describe('SettingsUtils', () => { const settings = makeMockSettings({ ui: { accessibility: { enableLoadingPhrases: false } }, }); - const mergedSettings = makeMockSettings({ - ui: { accessibility: { enableLoadingPhrases: true } }, - }); const value = getEffectiveValue( 'ui.accessibility.enableLoadingPhrases', settings, - mergedSettings, ); expect(value).toBe(false); }); it('should return undefined for invalid settings', () => { const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({}); - const value = getEffectiveValue( - 'invalidSetting', - settings, - mergedSettings, - ); + const value = getEffectiveValue('invalidSetting', settings); expect(value).toBeUndefined(); }); }); @@ -483,7 +440,9 @@ describe('SettingsUtils', () => { expect(dialogKeys.length).toBeGreaterThan(0); }); - it('should handle nested settings display correctly', () => { + const nestedDialogKey = 'context.fileFiltering.respectGitIgnore'; + + function mockNestedDialogSchema() { vi.mocked(getSettingsSchema).mockReturnValue({ context: { type: 'object', @@ -517,128 +476,27 @@ describe('SettingsUtils', () => { }, }, } as unknown as SettingsSchemaType); + } - // Test the specific issue with fileFiltering.respectGitIgnore - const key = 'context.fileFiltering.respectGitIgnore'; - const initialSettings = makeMockSettings({}); - const pendingSettings = makeMockSettings({}); + it('should include nested file filtering setting in dialog keys', () => { + mockNestedDialogSchema(); - // Set the nested setting to true - const updatedPendingSettings = setPendingSettingValue( - key, - true, - pendingSettings, - ); - - // Check if the setting exists in pending settings - const existsInPending = settingExistsInScope( - key, - updatedPendingSettings, - ); - expect(existsInPending).toBe(true); - - // Get the value from pending settings - const valueFromPending = getSettingValue( - key, - updatedPendingSettings, - {}, - ); - expect(valueFromPending).toBe(true); - - // Test getDisplayValue should show the pending change - const displayValue = getDisplayValue( - key, - initialSettings, - {}, - new Set(), - updatedPendingSettings, - ); - expect(displayValue).toBe('true'); // Should show true (no * since value matches default) - - // Test that modified settings also show the * indicator - const modifiedSettings = new Set([key]); - const displayValueWithModified = getDisplayValue( - key, - initialSettings, - {}, - modifiedSettings, - {}, - ); - expect(displayValueWithModified).toBe('true*'); // Should show true* because it's in modified settings and default is true + const dialogKeys = getDialogSettingKeys(); + expect(dialogKeys).toContain(nestedDialogKey); }); }); }); describe('Business Logic Utilities', () => { - describe('getSettingValue', () => { - it('should return value from settings when set', () => { - const settings = makeMockSettings({ ui: { requiresRestart: true } }); - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: false }, - }); - - const value = getSettingValue( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(value).toBe(true); - }); - - it('should return value from merged settings when not set in current scope', () => { - const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: true }, - }); - - const value = getSettingValue( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(value).toBe(true); - }); - - it('should return default value for invalid setting', () => { - const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({}); - - const value = getSettingValue( - 'invalidSetting', - settings, - mergedSettings, - ); - expect(value).toBe(false); // Default fallback - }); - }); - - describe('isSettingModified', () => { - it('should return true when value differs from default', () => { - expect(isSettingModified('ui.requiresRestart', true)).toBe(true); - expect( - isSettingModified('ui.accessibility.enableLoadingPhrases', false), - ).toBe(true); - }); - - it('should return false when value matches default', () => { - expect(isSettingModified('ui.requiresRestart', false)).toBe(false); - expect( - isSettingModified('ui.accessibility.enableLoadingPhrases', true), - ).toBe(false); - }); - }); - - describe('settingExistsInScope', () => { + describe('isInSettingsScope', () => { it('should return true for top-level settings that exist', () => { const settings = makeMockSettings({ ui: { requiresRestart: true } }); - expect(settingExistsInScope('ui.requiresRestart', settings)).toBe(true); + expect(isInSettingsScope('ui.requiresRestart', settings)).toBe(true); }); it('should return false for top-level settings that do not exist', () => { const settings = makeMockSettings({}); - expect(settingExistsInScope('ui.requiresRestart', settings)).toBe( - false, - ); + expect(isInSettingsScope('ui.requiresRestart', settings)).toBe(false); }); it('should return true for nested settings that exist', () => { @@ -646,121 +504,25 @@ describe('SettingsUtils', () => { ui: { accessibility: { enableLoadingPhrases: true } }, }); expect( - settingExistsInScope( - 'ui.accessibility.enableLoadingPhrases', - settings, - ), + isInSettingsScope('ui.accessibility.enableLoadingPhrases', settings), ).toBe(true); }); it('should return false for nested settings that do not exist', () => { const settings = makeMockSettings({}); expect( - settingExistsInScope( - 'ui.accessibility.enableLoadingPhrases', - settings, - ), + isInSettingsScope('ui.accessibility.enableLoadingPhrases', settings), ).toBe(false); }); it('should return false when parent exists but child does not', () => { const settings = makeMockSettings({ ui: { accessibility: {} } }); expect( - settingExistsInScope( - 'ui.accessibility.enableLoadingPhrases', - settings, - ), + isInSettingsScope('ui.accessibility.enableLoadingPhrases', settings), ).toBe(false); }); }); - describe('setPendingSettingValue', () => { - it('should set top-level setting value', () => { - const pendingSettings = makeMockSettings({}); - const result = setPendingSettingValue( - 'ui.hideWindowTitle', - true, - pendingSettings, - ); - - expect(result.ui?.hideWindowTitle).toBe(true); - }); - - it('should set nested setting value', () => { - const pendingSettings = makeMockSettings({}); - const result = setPendingSettingValue( - 'ui.accessibility.enableLoadingPhrases', - true, - pendingSettings, - ); - - expect(result.ui?.accessibility?.enableLoadingPhrases).toBe(true); - }); - - it('should preserve existing nested settings', () => { - const pendingSettings = makeMockSettings({ - ui: { accessibility: { enableLoadingPhrases: false } }, - }); - const result = setPendingSettingValue( - 'ui.accessibility.enableLoadingPhrases', - true, - pendingSettings, - ); - - expect(result.ui?.accessibility?.enableLoadingPhrases).toBe(true); - }); - - it('should not mutate original settings', () => { - const pendingSettings = makeMockSettings({}); - setPendingSettingValue('ui.requiresRestart', true, pendingSettings); - - expect(pendingSettings).toEqual({}); - }); - }); - - describe('hasRestartRequiredSettings', () => { - it('should return true when modified settings require restart', () => { - const modifiedSettings = new Set([ - 'advanced.autoConfigureMemory', - 'ui.requiresRestart', - ]); - expect(hasRestartRequiredSettings(modifiedSettings)).toBe(true); - }); - - it('should return false when no modified settings require restart', () => { - const modifiedSettings = new Set(['test']); - expect(hasRestartRequiredSettings(modifiedSettings)).toBe(false); - }); - - it('should return false for empty set', () => { - const modifiedSettings = new Set(); - expect(hasRestartRequiredSettings(modifiedSettings)).toBe(false); - }); - }); - - describe('getRestartRequiredFromModified', () => { - it('should return only settings that require restart', () => { - const modifiedSettings = new Set([ - 'ui.requiresRestart', - 'test', - ]); - const result = getRestartRequiredFromModified(modifiedSettings); - - expect(result).toContain('ui.requiresRestart'); - expect(result).not.toContain('test'); - }); - - it('should return empty array when no settings require restart', () => { - const modifiedSettings = new Set([ - 'requiresRestart', - 'hideTips', - ]); - const result = getRestartRequiredFromModified(modifiedSettings); - - expect(result).toEqual([]); - }); - }); - describe('getDisplayValue', () => { describe('enum behavior', () => { enum StringEnum { @@ -830,14 +592,8 @@ describe('SettingsUtils', () => { const mergedSettings = makeMockSettings({ ui: { theme: NumberEnum.THREE }, }); - const modifiedSettings = new Set(); - const result = getDisplayValue( - 'ui.theme', - settings, - mergedSettings, - modifiedSettings, - ); + const result = getDisplayValue('ui.theme', settings, mergedSettings); expect(result).toBe('Three*'); }); @@ -867,13 +623,11 @@ describe('SettingsUtils', () => { }, }, } as unknown as SettingsSchemaType); - const modifiedSettings = new Set(); const result = getDisplayValue( 'ui.theme', makeMockSettings({}), makeMockSettings({}), - modifiedSettings, ); expect(result).toBe('Three'); }); @@ -886,14 +640,8 @@ describe('SettingsUtils', () => { const mergedSettings = makeMockSettings({ ui: { theme: StringEnum.BAR }, }); - const modifiedSettings = new Set(); - const result = getDisplayValue( - 'ui.theme', - settings, - mergedSettings, - modifiedSettings, - ); + const result = getDisplayValue('ui.theme', settings, mergedSettings); expect(result).toBe('Bar*'); }); @@ -907,14 +655,8 @@ describe('SettingsUtils', () => { } as unknown as SettingsSchemaType); const settings = makeMockSettings({ ui: { theme: 'xyz' } }); const mergedSettings = makeMockSettings({ ui: { theme: 'xyz' } }); - const modifiedSettings = new Set(); - const result = getDisplayValue( - 'ui.theme', - settings, - mergedSettings, - modifiedSettings, - ); + const result = getDisplayValue('ui.theme', settings, mergedSettings); expect(result).toBe('xyz*'); }); @@ -926,242 +668,71 @@ describe('SettingsUtils', () => { }, }, } as unknown as SettingsSchemaType); - const modifiedSettings = new Set(); const result = getDisplayValue( 'ui.theme', makeMockSettings({}), makeMockSettings({}), - modifiedSettings, ); expect(result).toBe('Bar'); }); }); - it('should show value without * when setting matches default', () => { - const settings = makeMockSettings({ - ui: { requiresRestart: false }, - }); // false matches default, so no * + it('should show value with * when setting exists in scope', () => { + const settings = makeMockSettings({ ui: { requiresRestart: true } }); const mergedSettings = makeMockSettings({ - ui: { requiresRestart: false }, + ui: { requiresRestart: true }, }); - const modifiedSettings = new Set(); const result = getDisplayValue( 'ui.requiresRestart', settings, mergedSettings, - modifiedSettings, ); - expect(result).toBe('false*'); + expect(result).toBe('true*'); }); - - it('should show default value when setting is not in scope', () => { + it('should not show * when key is not in scope', () => { const settings = makeMockSettings({}); // no setting in scope const mergedSettings = makeMockSettings({ ui: { requiresRestart: false }, }); - const modifiedSettings = new Set(); const result = getDisplayValue( 'ui.requiresRestart', settings, mergedSettings, - modifiedSettings, ); expect(result).toBe('false'); // shows default value }); - it('should show value with * when changed from default', () => { - const settings = makeMockSettings({ ui: { requiresRestart: true } }); // true is different from default (false) - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: true }, - }); - const modifiedSettings = new Set(); - - const result = getDisplayValue( - 'ui.requiresRestart', - settings, - mergedSettings, - modifiedSettings, - ); - expect(result).toBe('true*'); - }); - - it('should show default value without * when setting does not exist in scope', () => { - const settings = makeMockSettings({}); // setting doesn't exist in scope, show default - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: false }, - }); - const modifiedSettings = new Set(); - - const result = getDisplayValue( - 'ui.requiresRestart', - settings, - mergedSettings, - modifiedSettings, - ); - expect(result).toBe('false'); // default value (false) without * - }); - - it('should show value with * when user changes from default', () => { - const settings = makeMockSettings({}); // setting doesn't exist in scope originally - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: false }, - }); - const modifiedSettings = new Set(['ui.requiresRestart']); - const pendingSettings = makeMockSettings({ - ui: { requiresRestart: true }, - }); // user changed to true - - const result = getDisplayValue( - 'ui.requiresRestart', - settings, - mergedSettings, - modifiedSettings, - pendingSettings, - ); - expect(result).toBe('true*'); // changed from default (false) to true - }); - }); - - describe('isDefaultValue', () => { - it('should return true when setting does not exist in scope', () => { - const settings = makeMockSettings({}); // setting doesn't exist - - const result = isDefaultValue('ui.requiresRestart', settings); - expect(result).toBe(true); - }); - - it('should return false when setting exists in scope', () => { - const settings = makeMockSettings({ ui: { requiresRestart: true } }); // setting exists - - const result = isDefaultValue('ui.requiresRestart', settings); - expect(result).toBe(false); - }); - - it('should return true when nested setting does not exist in scope', () => { - const settings = makeMockSettings({}); // nested setting doesn't exist - - const result = isDefaultValue( - 'ui.accessibility.enableLoadingPhrases', - settings, - ); - expect(result).toBe(true); - }); - - it('should return false when nested setting exists in scope', () => { + it('should show value with * when setting exists in scope, even when it matches default', () => { const settings = makeMockSettings({ - ui: { accessibility: { enableLoadingPhrases: true } }, - }); // nested setting exists - - const result = isDefaultValue( - 'ui.accessibility.enableLoadingPhrases', - settings, - ); - expect(result).toBe(false); - }); - }); - - describe('isValueInherited', () => { - it('should return false for top-level settings that exist in scope', () => { - const settings = makeMockSettings({ ui: { requiresRestart: true } }); - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: true }, - }); - - const result = isValueInherited( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(result).toBe(false); - }); - - it('should return true for top-level settings that do not exist in scope', () => { - const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({ - ui: { requiresRestart: true }, - }); - - const result = isValueInherited( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(result).toBe(true); - }); - - it('should return false for nested settings that exist in scope', () => { - const settings = makeMockSettings({ - ui: { accessibility: { enableLoadingPhrases: true } }, - }); - const mergedSettings = makeMockSettings({ - ui: { accessibility: { enableLoadingPhrases: true } }, - }); - - const result = isValueInherited( - 'ui.accessibility.enableLoadingPhrases', - settings, - mergedSettings, - ); - expect(result).toBe(false); - }); - - it('should return true for nested settings that do not exist in scope', () => { - const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({ - ui: { accessibility: { enableLoadingPhrases: true } }, - }); - - const result = isValueInherited( - 'ui.accessibility.enableLoadingPhrases', - settings, - mergedSettings, - ); - expect(result).toBe(true); - }); - }); - - describe('getEffectiveDisplayValue', () => { - it('should return value from settings when available', () => { - const settings = makeMockSettings({ ui: { requiresRestart: true } }); + ui: { requiresRestart: false }, + }); // false matches default, but key is explicitly set in scope const mergedSettings = makeMockSettings({ ui: { requiresRestart: false }, }); - const result = getEffectiveDisplayValue( + const result = getDisplayValue( 'ui.requiresRestart', settings, mergedSettings, ); - expect(result).toBe(true); + expect(result).toBe('false*'); }); - it('should return value from merged settings when not in scope', () => { - const settings = makeMockSettings({}); + it('should show schema default (not inherited merged value) when key is not in scope', () => { + const settings = makeMockSettings({}); // no setting in current scope const mergedSettings = makeMockSettings({ ui: { requiresRestart: true }, - }); + }); // inherited merged value differs from schema default (false) - const result = getEffectiveDisplayValue( + const result = getDisplayValue( 'ui.requiresRestart', settings, mergedSettings, ); - expect(result).toBe(true); - }); - - it('should return default value for undefined values', () => { - const settings = makeMockSettings({}); - const mergedSettings = makeMockSettings({}); - - const result = getEffectiveDisplayValue( - 'ui.requiresRestart', - settings, - mergedSettings, - ); - expect(result).toBe(false); // Default value + expect(result).toBe('false'); }); }); }); diff --git a/packages/cli/src/utils/settingsUtils.ts b/packages/cli/src/utils/settingsUtils.ts index 3fa1d8bd5d..87ca920899 100644 --- a/packages/cli/src/utils/settingsUtils.ts +++ b/packages/cli/src/utils/settingsUtils.ts @@ -4,11 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { - Settings, - LoadedSettings, - LoadableSettingScope, -} from '../config/settings.js'; +import type { Settings } from '../config/settings.js'; import type { SettingDefinition, SettingsSchema, @@ -52,9 +48,6 @@ function clearFlattenedSchema() { _FLATTENED_SCHEMA = undefined; } -/** - * Get all settings grouped by category - */ export function getSettingsByCategory(): Record< string, Array @@ -75,25 +68,16 @@ export function getSettingsByCategory(): Record< return categories; } -/** - * Get a setting definition by key - */ export function getSettingDefinition( key: string, ): (SettingDefinition & { key: string }) | undefined { return getFlattenedSchema()[key]; } -/** - * Check if a setting requires restart - */ export function requiresRestart(key: string): boolean { return getFlattenedSchema()[key]?.requiresRestart ?? false; } -/** - * Get the default value for a setting - */ export function getDefaultValue(key: string): SettingsValue { return getFlattenedSchema()[key]?.default; } @@ -120,9 +104,6 @@ export function getEffectiveDefaultValue( return getDefaultValue(key); } -/** - * Get all setting keys that require restart - */ export function getRestartRequiredSettings(): string[] { return Object.values(getFlattenedSchema()) .filter((definition) => definition.requiresRestart) @@ -130,35 +111,55 @@ export function getRestartRequiredSettings(): string[] { } /** - * Recursively gets a value from a nested object using a key path array. + * Get restart-required setting keys that are also visible in the dialog. + * Non-dialog restart keys (e.g. parent container objects like mcpServers, tools) + * are excluded because users cannot change them through the dialog. */ -export function getNestedValue( - obj: Record, - path: string[], -): unknown { - const [first, ...rest] = path; - if (!first || !(first in obj)) { - return undefined; - } - const value = obj[first]; - if (rest.length === 0) { - return value; - } - if (value && typeof value === 'object' && value !== null) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - return getNestedValue(value as Record, rest); - } - return undefined; +export function getDialogRestartRequiredSettings(): string[] { + return Object.values(getFlattenedSchema()) + .filter( + (definition) => + definition.requiresRestart && definition.showInDialog !== false, + ) + .map((definition) => definition.key); +} + +export function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null; +} + +function isSettingsValue(value: unknown): value is SettingsValue { + if (value === undefined) return true; + if (value === null) return false; + const type = typeof value; + return ( + type === 'string' || + type === 'number' || + type === 'boolean' || + type === 'object' + ); } /** - * Get the effective value for a setting, considering inheritance from higher scopes - * Always returns a value (never undefined) - falls back to default if not set anywhere + * Gets a value from a nested object using a key path array iteratively. + */ +export function getNestedValue(obj: unknown, path: string[]): unknown { + let current = obj; + for (const key of path) { + if (!isRecord(current) || !(key in current)) { + return undefined; + } + current = current[key]; + } + return current; +} + +/** + * Get the effective value for a setting falling back to the default value */ export function getEffectiveValue( key: string, settings: Settings, - mergedSettings: Settings, ): SettingsValue { const definition = getSettingDefinition(key); if (!definition) { @@ -168,33 +169,19 @@ export function getEffectiveValue( const path = key.split('.'); // Check the current scope's settings first - let value = getNestedValue(settings as Record, path); - if (value !== undefined) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - return value as SettingsValue; - } - - // Check the merged settings for an inherited value - value = getNestedValue(mergedSettings as Record, path); - if (value !== undefined) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - return value as SettingsValue; + const value = getNestedValue(settings, path); + if (value !== undefined && isSettingsValue(value)) { + return value; } // Return default value if no value is set anywhere return definition.default; } -/** - * Get all setting keys from the schema - */ export function getAllSettingKeys(): string[] { return Object.keys(getFlattenedSchema()); } -/** - * Get settings by type - */ export function getSettingsByType( type: SettingsType, ): Array { @@ -203,9 +190,6 @@ export function getSettingsByType( ); } -/** - * Get settings that require restart - */ export function getSettingsRequiringRestart(): Array< SettingDefinition & { key: string; @@ -223,22 +207,22 @@ export function isValidSettingKey(key: string): boolean { return key in getFlattenedSchema(); } -/** - * Get the category for a setting - */ export function getSettingCategory(key: string): string | undefined { return getFlattenedSchema()[key]?.category; } -/** - * Check if a setting should be shown in the settings dialog - */ export function shouldShowInDialog(key: string): boolean { return getFlattenedSchema()[key]?.showInDialog ?? true; // Default to true for backward compatibility } +export function getDialogSettingKeys(): string[] { + return Object.values(getFlattenedSchema()) + .filter((definition) => definition.showInDialog !== false) + .map((definition) => definition.key); +} + /** - * Get all settings that should be shown in the dialog, grouped by category + * Get all settings that should be shown in the dialog, grouped by category like "Advanced", "General", etc. */ export function getDialogSettingsByCategory(): Record< string, @@ -262,9 +246,6 @@ export function getDialogSettingsByCategory(): Record< return categories; } -/** - * Get settings by type that should be shown in the dialog - */ export function getDialogSettingsByType( type: SettingsType, ): Array { @@ -274,197 +255,30 @@ export function getDialogSettingsByType( ); } -/** - * Get all setting keys that should be shown in the dialog - */ -export function getDialogSettingKeys(): string[] { - return Object.values(getFlattenedSchema()) - .filter((definition) => definition.showInDialog !== false) - .map((definition) => definition.key); -} - -// ============================================================================ -// BUSINESS LOGIC UTILITIES (Higher-level utilities for setting operations) -// ============================================================================ - -/** - * Get the current value for a setting in a specific scope - * Always returns a value (never undefined) - falls back to default if not set anywhere - */ -export function getSettingValue( - key: string, - settings: Settings, - mergedSettings: Settings, -): boolean { - const definition = getSettingDefinition(key); - if (!definition) { - return false; // Default fallback for invalid settings - } - - const value = getEffectiveValue(key, settings, mergedSettings); - // Ensure we return a boolean value, converting from the more general type - if (typeof value === 'boolean') { - return value; - } - - return false; // Final fallback -} - -/** - * Check if a setting value is modified from its default - */ -export function isSettingModified(key: string, value: boolean): boolean { - const defaultValue = getDefaultValue(key); - // Handle type comparison properly - if (typeof defaultValue === 'boolean') { - return value !== defaultValue; - } - // If default is not a boolean, consider it modified if value is true - return value === true; -} - -/** - * Check if a setting exists in the original settings file for a scope - */ -export function settingExistsInScope( +export function isInSettingsScope( key: string, scopeSettings: Settings, ): boolean { const path = key.split('.'); - const value = getNestedValue(scopeSettings as Record, path); + const value = getNestedValue(scopeSettings, path); return value !== undefined; } /** - * Recursively sets a value in a nested object using a key path array. - */ -function setNestedValue( - obj: Record, - path: string[], - value: unknown, -): Record { - const [first, ...rest] = path; - if (!first) { - return obj; - } - - if (rest.length === 0) { - obj[first] = value; - return obj; - } - - if (!obj[first] || typeof obj[first] !== 'object') { - obj[first] = {}; - } - - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - setNestedValue(obj[first] as Record, rest, value); - return obj; -} - -/** - * Set a setting value in the pending settings - */ -export function setPendingSettingValue( - key: string, - value: boolean, - pendingSettings: Settings, -): Settings { - const path = key.split('.'); - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const newSettings = JSON.parse(JSON.stringify(pendingSettings)); - setNestedValue(newSettings, path, value); - // eslint-disable-next-line @typescript-eslint/no-unsafe-return - return newSettings; -} - -/** - * Generic setter: Set a setting value (boolean, number, string, etc.) in the pending settings - */ -export function setPendingSettingValueAny( - key: string, - value: SettingsValue, - pendingSettings: Settings, -): Settings { - const path = key.split('.'); - const newSettings = structuredClone(pendingSettings); - setNestedValue(newSettings, path, value); - return newSettings; -} - -/** - * Check if any modified settings require a restart - */ -export function hasRestartRequiredSettings( - modifiedSettings: Set, -): boolean { - return Array.from(modifiedSettings).some((key) => requiresRestart(key)); -} - -/** - * Get the restart required settings from a set of modified settings - */ -export function getRestartRequiredFromModified( - modifiedSettings: Set, -): string[] { - return Array.from(modifiedSettings).filter((key) => requiresRestart(key)); -} - -/** - * Save modified settings to the appropriate scope - */ -export function saveModifiedSettings( - modifiedSettings: Set, - pendingSettings: Settings, - loadedSettings: LoadedSettings, - scope: LoadableSettingScope, -): void { - modifiedSettings.forEach((settingKey) => { - const path = settingKey.split('.'); - const value = getNestedValue( - pendingSettings as Record, - path, - ); - - if (value === undefined) { - return; - } - - const existsInOriginalFile = settingExistsInScope( - settingKey, - loadedSettings.forScope(scope).settings, - ); - - const isDefaultValue = value === getDefaultValue(settingKey); - - if (existsInOriginalFile || !isDefaultValue) { - loadedSettings.setValue(scope, settingKey, value); - } - }); -} - -/** - * Get the display value for a setting, showing current scope value with default change indicator + * Appends a star (*) to settings that exist in the scope */ export function getDisplayValue( key: string, - settings: Settings, + scopeSettings: Settings, _mergedSettings: Settings, - modifiedSettings: Set, - pendingSettings?: Settings, ): string { - // Prioritize pending changes if user has modified this setting const definition = getSettingDefinition(key); + const existsInScope = isInSettingsScope(key, scopeSettings); let value: SettingsValue; - if (pendingSettings && settingExistsInScope(key, pendingSettings)) { - // Show the value from the pending (unsaved) edits when it exists - value = getEffectiveValue(key, pendingSettings, {}); - } else if (settingExistsInScope(key, settings)) { - // Show the value defined at the current scope if present - value = getEffectiveValue(key, settings, {}); + if (existsInScope) { + value = getEffectiveValue(key, scopeSettings); } else { - // Fall back to the schema default when the key is unset in this scope value = getDefaultValue(key); } @@ -475,50 +289,108 @@ export function getDisplayValue( valueString = option?.label ?? `${value}`; } - // Check if value is different from default OR if it's in modified settings OR if there are pending changes - const defaultValue = getDefaultValue(key); - const isChangedFromDefault = value !== defaultValue; - const isInModifiedSettings = modifiedSettings.has(key); - - // Mark as modified if setting exists in current scope OR is in modified settings - if (settingExistsInScope(key, settings) || isInModifiedSettings) { - return `${valueString}*`; // * indicates setting is set in current scope - } - if (isChangedFromDefault || isInModifiedSettings) { - return `${valueString}*`; // * indicates changed from default value + if (existsInScope) { + return `${valueString}*`; } return valueString; } -/** - * Check if a setting doesn't exist in current scope (should be greyed out) - */ -export function isDefaultValue(key: string, settings: Settings): boolean { - return !settingExistsInScope(key, settings); +/**Utilities for parsing Settings that can be inline edited by the user typing out values */ +function tryParseJsonStringArray(input: string): string[] | null { + try { + const parsed: unknown = JSON.parse(input); + if ( + Array.isArray(parsed) && + parsed.every((item): item is string => typeof item === 'string') + ) { + return parsed; + } + return null; + } catch { + return null; + } } -/** - * Check if a setting value is inherited (not set at current scope) - */ -export function isValueInherited( - key: string, - settings: Settings, - _mergedSettings: Settings, -): boolean { - return !settingExistsInScope(key, settings); +function tryParseJsonObject(input: string): Record | null { + try { + const parsed: unknown = JSON.parse(input); + if (isRecord(parsed) && !Array.isArray(parsed)) { + return parsed; + } + return null; + } catch { + return null; + } } -/** - * Get the effective value for display, considering inheritance - * Always returns a boolean value (never undefined) - */ -export function getEffectiveDisplayValue( - key: string, - settings: Settings, - mergedSettings: Settings, -): boolean { - return getSettingValue(key, settings, mergedSettings); +function parseStringArrayValue(input: string): string[] { + const trimmed = input.trim(); + if (trimmed === '') return []; + + return ( + tryParseJsonStringArray(trimmed) ?? + input + .split(',') + .map((p) => p.trim()) + .filter((p) => p.length > 0) + ); +} + +function parseObjectValue(input: string): Record | null { + const trimmed = input.trim(); + if (trimmed === '') { + return null; + } + + return tryParseJsonObject(trimmed); +} + +export function parseEditedValue( + type: SettingsType, + newValue: string, +): SettingsValue | null { + if (type === 'number') { + if (newValue.trim() === '') { + return null; + } + + const numParsed = Number(newValue.trim()); + if (Number.isNaN(numParsed)) { + return null; + } + + return numParsed; + } + + if (type === 'array') { + return parseStringArrayValue(newValue); + } + + if (type === 'object') { + return parseObjectValue(newValue); + } + + return newValue; +} + +export function getEditValue( + type: SettingsType, + rawValue: SettingsValue, +): string | undefined { + if (rawValue === undefined) { + return undefined; + } + + if (type === 'array' && Array.isArray(rawValue)) { + return rawValue.join(', '); + } + + if (type === 'object' && rawValue !== null && typeof rawValue === 'object') { + return JSON.stringify(rawValue); + } + + return undefined; } export const TEST_ONLY = { clearFlattenedSchema };