fix(SettingsDialog): race condition in SettingsDialog causing settings to be unexpectedly cleared (#10875)

Co-authored-by: Gal Zahavi <38544478+galz10@users.noreply.github.com>
Co-authored-by: Jacob Richman <jacob314@gmail.com>

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<Key> & { name: string }) => {
 //   if (currentKeypressHandler) {
@@ -395,11 +489,11 @@ describe('SettingsDialog', () => {
 
       expect(vi.mocked(saveModifiedSettings)).toHaveBeenCalledWith(
         new Set<string>(['general.disableAutoUpdate']),
-        {
+        expect.objectContaining({
-          general: {
+          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<string>(['ui.theme']),
-          {
+          expect.objectContaining({
-            ui: {
+            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<string>(['ui.theme']),
-          {
+          expect.objectContaining({
-            ui: {
+            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 = (
+        <KeypressProvider kittyProtocolEnabled={false}>
+          <SettingsDialog settings={settings} onSelect={onSelect} />
+        </KeypressProvider>
+      );
+
+      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 = (
+        <KeypressProvider kittyProtocolEnabled={false}>
+          <SettingsDialog settings={settings} onSelect={onSelect} />
+        </KeypressProvider>
+      );
+
+      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<string, unknown>
+            | 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();

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,