diff --git a/packages/cli/src/ui/components/SettingsDialog.test.tsx b/packages/cli/src/ui/components/SettingsDialog.test.tsx index 9425c9f1e9..ab2b522c19 100644 --- a/packages/cli/src/ui/components/SettingsDialog.test.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.test.tsx @@ -129,6 +129,100 @@ vi.mock('../../utils/settingsUtils.js', async () => { }; }); +// Shared test schemas +enum StringEnum { + FOO = 'foo', + BAR = 'bar', + BAZ = 'baz', +} + +const ENUM_SETTING: SettingDefinition = { + type: 'enum', + label: 'Theme', + options: [ + { + label: 'Foo', + value: StringEnum.FOO, + }, + { + label: 'Bar', + value: StringEnum.BAR, + }, + { + label: 'Baz', + value: StringEnum.BAZ, + }, + ], + category: 'UI', + requiresRestart: false, + default: StringEnum.BAR, + description: 'The color theme for the UI.', + showInDialog: true, +}; + +const ENUM_FAKE_SCHEMA: SettingsSchemaType = { + ui: { + showInDialog: false, + properties: { + theme: { + ...ENUM_SETTING, + }, + }, + }, +} as unknown as SettingsSchemaType; + +const TOOLS_SHELL_FAKE_SCHEMA: SettingsSchemaType = { + tools: { + type: 'object', + label: 'Tools', + category: 'Tools', + requiresRestart: false, + default: {}, + description: 'Tool settings.', + showInDialog: false, + properties: { + shell: { + type: 'object', + label: 'Shell', + category: 'Tools', + requiresRestart: false, + default: {}, + description: 'Shell tool settings.', + showInDialog: false, + properties: { + showColor: { + type: 'boolean', + label: 'Show Color', + category: 'Tools', + requiresRestart: false, + default: false, + description: 'Show color in shell output.', + showInDialog: true, + }, + enableInteractiveShell: { + type: 'boolean', + label: 'Enable Interactive Shell', + category: 'Tools', + requiresRestart: true, + default: true, + description: 'Enable interactive shell mode.', + showInDialog: true, + }, + pager: { + type: 'string', + label: 'Pager', + category: 'Tools', + requiresRestart: false, + default: 'cat', + description: 'The pager command to use for shell output.', + showInDialog: true, + }, + }, + }, + }, + }, +} as unknown as SettingsSchemaType; + // Helper function to simulate key presses (commented out for now) // const simulateKeyPress = async (keyData: Partial & { name: string }) => { // if (currentKeypressHandler) { @@ -395,11 +489,11 @@ describe('SettingsDialog', () => { expect(vi.mocked(saveModifiedSettings)).toHaveBeenCalledWith( new Set(['general.disableAutoUpdate']), - { - general: { + expect.objectContaining({ + general: expect.objectContaining({ disableAutoUpdate: true, - }, - }, + }), + }), expect.any(LoadedSettings), SettingScope.User, ); @@ -408,51 +502,10 @@ describe('SettingsDialog', () => { }); describe('enum values', () => { - enum StringEnum { - FOO = 'foo', - BAR = 'bar', - BAZ = 'baz', - } - - const SETTING: SettingDefinition = { - type: 'enum', - label: 'Theme', - options: [ - { - label: 'Foo', - value: StringEnum.FOO, - }, - { - label: 'Bar', - value: StringEnum.BAR, - }, - { - label: 'Baz', - value: StringEnum.BAZ, - }, - ], - category: 'UI', - requiresRestart: false, - default: StringEnum.BAR, - description: 'The color theme for the UI.', - showInDialog: true, - }; - - const FAKE_SCHEMA: SettingsSchemaType = { - ui: { - showInDialog: false, - properties: { - theme: { - ...SETTING, - }, - }, - }, - } as unknown as SettingsSchemaType; - it('toggles enum values with the enter key', async () => { vi.mocked(saveModifiedSettings).mockClear(); - vi.mocked(getSettingsSchema).mockReturnValue(FAKE_SCHEMA); + vi.mocked(getSettingsSchema).mockReturnValue(ENUM_FAKE_SCHEMA); const settings = createMockSettings(); const onSelect = vi.fn(); const component = ( @@ -474,11 +527,11 @@ describe('SettingsDialog', () => { expect(vi.mocked(saveModifiedSettings)).toHaveBeenCalledWith( new Set(['ui.theme']), - { - ui: { + expect.objectContaining({ + ui: expect.objectContaining({ theme: StringEnum.BAZ, - }, - }, + }), + }), expect.any(LoadedSettings), SettingScope.User, ); @@ -488,7 +541,7 @@ describe('SettingsDialog', () => { it('loops back when reaching the end of an enum', async () => { vi.mocked(saveModifiedSettings).mockClear(); - vi.mocked(getSettingsSchema).mockReturnValue(FAKE_SCHEMA); + vi.mocked(getSettingsSchema).mockReturnValue(ENUM_FAKE_SCHEMA); const settings = createMockSettings(); settings.setValue(SettingScope.User, 'ui.theme', StringEnum.BAZ); const onSelect = vi.fn(); @@ -511,11 +564,11 @@ describe('SettingsDialog', () => { expect(vi.mocked(saveModifiedSettings)).toHaveBeenCalledWith( new Set(['ui.theme']), - { - ui: { + expect.objectContaining({ + ui: expect.objectContaining({ theme: StringEnum.FOO, - }, - }, + }), + }), expect.any(LoadedSettings), SettingScope.User, ); @@ -928,6 +981,129 @@ describe('SettingsDialog', () => { }); }); + describe('Race Condition Regression Tests', () => { + it('should not reset sibling settings when toggling a nested setting multiple times', async () => { + vi.mocked(saveModifiedSettings).mockClear(); + + vi.mocked(getSettingsSchema).mockReturnValue(TOOLS_SHELL_FAKE_SCHEMA); + + const settings = createMockSettings({ + tools: { + shell: { + showColor: false, + enableInteractiveShell: true, + }, + }, + }); + + const onSelect = vi.fn(); + const component = ( + + + + ); + + const { stdin, unmount } = render(component); + + await wait(); + + // Toggle showColor 5 times to trigger race condition + for (let i = 0; i < 5; i++) { + act(() => { + stdin.write(TerminalKeys.ENTER as string); + }); + await wait(50); + } + + await waitFor(() => { + expect( + vi.mocked(saveModifiedSettings).mock.calls.length, + ).toBeGreaterThan(0); + }); + + // Verify sibling settings are preserved + const calls = vi.mocked(saveModifiedSettings).mock.calls; + calls.forEach((call) => { + const [modifiedKeys, pendingSettings] = call; + + if (modifiedKeys.has('tools.shell.showColor')) { + expect(pendingSettings.tools?.shell?.enableInteractiveShell).toBe( + true, + ); + expect(modifiedKeys.has('tools.shell.enableInteractiveShell')).toBe( + false, + ); + } + }); + + expect(calls.length).toBeGreaterThan(0); + + unmount(); + }); + + it('should preserve multiple sibling settings in nested objects during rapid toggles', async () => { + vi.mocked(saveModifiedSettings).mockClear(); + + vi.mocked(getSettingsSchema).mockReturnValue(TOOLS_SHELL_FAKE_SCHEMA); + + const settings = createMockSettings({ + tools: { + shell: { + showColor: false, + enableInteractiveShell: true, + pager: 'less', + }, + }, + }); + + const onSelect = vi.fn(); + const component = ( + + + + ); + + const { stdin, unmount } = render(component); + + await wait(); + + // Rapid toggles + for (let i = 0; i < 3; i++) { + act(() => { + stdin.write(TerminalKeys.ENTER as string); + }); + await wait(30); + } + + await waitFor(() => { + expect( + vi.mocked(saveModifiedSettings).mock.calls.length, + ).toBeGreaterThan(0); + }); + + // Verify all siblings preserved + 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; + expect(shellSettings?.['enableInteractiveShell']).toBe(true); + expect(shellSettings?.['pager']).toBe('less'); + expect(modifiedKeys.size).toBe(1); + expect(modifiedKeys.has('tools.shell.enableInteractiveShell')).toBe( + false, + ); + expect(modifiedKeys.has('tools.shell.pager')).toBe(false); + } + }); + + unmount(); + }); + }); + describe('Keyboard Shortcuts Edge Cases', () => { it('should handle rapid key presses gracefully', async () => { const settings = createMockSettings(); diff --git a/packages/cli/src/ui/components/SettingsDialog.tsx b/packages/cli/src/ui/components/SettingsDialog.tsx index ef3a897090..6e186e2686 100644 --- a/packages/cli/src/ui/components/SettingsDialog.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.tsx @@ -153,18 +153,15 @@ export function SettingsDialog({ ); } - setPendingSettings((prev) => - setPendingSettingValue(key, newValue as boolean, prev), - ); - if (!requiresRestart(key)) { const immediateSettings = new Set([key]); + const currentScopeSettings = + settings.forScope(selectedScope).settings; const immediateSettingsObject = setPendingSettingValueAny( key, newValue, - {} as Settings, + currentScopeSettings, ); - console.log( `[DEBUG SettingsDialog] Saving ${key} immediately with value:`, newValue, @@ -205,11 +202,6 @@ export function SettingsDialog({ next.delete(key); return next; }); - - // Refresh pending settings from the saved state - setPendingSettings( - structuredClone(settings.forScope(selectedScope).settings), - ); } else { // For restart-required settings, track as modified setModifiedSettings((prev) => { @@ -299,10 +291,11 @@ export function SettingsDialog({ if (!requiresRestart(key)) { const immediateSettings = new Set([key]); + const currentScopeSettings = settings.forScope(selectedScope).settings; const immediateSettingsObject = setPendingSettingValueAny( key, parsed, - {} as Settings, + currentScopeSettings, ); saveModifiedSettings( immediateSettings, @@ -658,14 +651,16 @@ export function SettingsDialog({ typeof defaultValue === 'string' ? defaultValue : undefined; + const currentScopeSettings = + settings.forScope(selectedScope).settings; const immediateSettingsObject = toSaveValue !== undefined ? setPendingSettingValueAny( currentSetting.value, toSaveValue, - {} as Settings, + currentScopeSettings, ) - : ({} as Settings); + : currentScopeSettings; saveModifiedSettings( immediateSettings,