Merge pull request #3274 from kydorn/fix/theme-save-echo-clobber

fix(ui): stop settings save echoes from clobbering theme preferences

packages/ui/src/components/auth/SessionAuthGate.tsx

@@ -602,7 +602,7 @@ export const SessionAuthGate: React.FC<SessionAuthGateProps> = ({
       hasBootstrapResyncedRef.current = true;
       void (async () => {
         await initializeAppearancePreferences();
-        await syncDesktopSettings({ adoptWorkspace: isBootstrapResync });
+        await syncDesktopSettings({ bootstrap: isBootstrapResync });
         if (isBootstrapResync) {
           await applyPersistedDirectoryPreferences();
         }

packages/ui/src/contexts/ThemeSystemContext.tsx

@@ -34,6 +34,7 @@ import { getRuntimeKey, subscribeRuntimeEndpointChanged } from '@/lib/runtime-sw
 import {
   adoptThemePreferencesForRuntime,
   resolveThemePreferencesForRuntime,
+  resolveThemePreferencesFromSettingsSync,
   resolveThemePreferencesFromStorageEvent,
   writeThemePreferencesForRuntime,
 } from './theme-storage';
@@ -590,46 +591,14 @@ export function ThemeSystemProvider({ children, defaultThemeId }: ThemeSystemPro
       return;
     }
     const handleSettingsSynced = (event: Event) => {
-      const detail = (event as CustomEvent<SettingsSyncedDetail>).detail?.settings;
+      const detail = (event as CustomEvent<SettingsSyncedDetail>).detail?.settings
+        ? (event as CustomEvent<SettingsSyncedDetail>).detail
+        : null;
       if (!detail) {
         return;
       }
 
-      setPreferences((prev) => {
-        let nextMode = prev.themeMode;
-        if (detail.useSystemTheme === true) {
-          nextMode = 'system';
-        } else if (detail.useSystemTheme === false) {
-          if (detail.themeVariant === 'dark' || detail.themeVariant === 'light') {
-            nextMode = detail.themeVariant;
-          }
-        }
-
-        let nextLight = prev.lightThemeId;
-        if (typeof detail.lightThemeId === 'string' && detail.lightThemeId.length > 0) {
-          nextLight = detail.lightThemeId.trim();
-        }
-
-        let nextDark = prev.darkThemeId;
-        if (typeof detail.darkThemeId === 'string' && detail.darkThemeId.length > 0) {
-          nextDark = detail.darkThemeId.trim();
-        }
-
-        const same =
-          nextMode === prev.themeMode &&
-          nextLight === prev.lightThemeId &&
-          nextDark === prev.darkThemeId;
-
-        if (same) {
-          return prev;
-        }
-
-        return {
-          themeMode: nextMode,
-          lightThemeId: nextLight,
-          darkThemeId: nextDark,
-        };
-      });
+      setPreferences((prev) => resolveThemePreferencesFromSettingsSync(detail, prev) ?? prev);
     };
 
     window.addEventListener('openchamber:settings-synced', handleSettingsSynced);

packages/ui/src/contexts/theme-storage.test.ts

@@ -7,6 +7,7 @@ import {
   getThemePreferencesStorageKey,
   readThemePreferencesForRuntime,
   resolveThemePreferencesForRuntime,
+  resolveThemePreferencesFromSettingsSync,
   resolveThemePreferencesFromStorageEvent,
   writeThemePreferencesForRuntime,
 } from './theme-storage';
@@ -288,3 +289,40 @@ describe('theme storage event resolution', () => {
     expect(resolveThemePreferencesFromStorageEvent(getThemePreferencesStorageKey('runtime-a'), 'runtime-a', current)).toBeNull();
   });
 });
+
+describe('settings sync resolution', () => {
+  const current = { themeMode: 'system' as const, lightThemeId: 'light-theme', darkThemeId: 'dark-theme' };
+  const serverTheme = { useSystemTheme: false as const, themeVariant: 'dark' as const, lightThemeId: 'server-light', darkThemeId: 'server-dark' };
+
+  test('a non-bootstrap sync (settings save echo) never changes preferences', () => {
+    expect(resolveThemePreferencesFromSettingsSync({ adoptTheme: false, settings: serverTheme }, current)).toBeNull();
+    expect(resolveThemePreferencesFromSettingsSync(null, current)).toBeNull();
+  });
+
+  test('a bootstrap sync adopts the server theme', () => {
+    expect(resolveThemePreferencesFromSettingsSync({ adoptTheme: true, settings: serverTheme }, current)).toEqual({
+      themeMode: 'dark',
+      lightThemeId: 'server-light',
+      darkThemeId: 'server-dark',
+    });
+  });
+
+  test('theme fields omitted by the server keep the current preferences (not-set is not reset-to-defaults)', () => {
+    expect(resolveThemePreferencesFromSettingsSync({ adoptTheme: true, settings: {} }, current)).toBeNull();
+    expect(
+      resolveThemePreferencesFromSettingsSync({ adoptTheme: true, settings: { useSystemTheme: true } }, current),
+    ).toBeNull();
+    expect(
+      resolveThemePreferencesFromSettingsSync({ adoptTheme: true, settings: { lightThemeId: 'server-light' } }, current),
+    ).toEqual({ themeMode: 'system', lightThemeId: 'server-light', darkThemeId: 'dark-theme' });
+  });
+
+  test('a bootstrap sync carrying the current preferences resolves to no change', () => {
+    expect(
+      resolveThemePreferencesFromSettingsSync(
+        { adoptTheme: true, settings: { useSystemTheme: true, lightThemeId: 'light-theme', darkThemeId: 'dark-theme' } },
+        current,
+      ),
+    ).toBeNull();
+  });
+});

packages/ui/src/contexts/theme-storage.ts

@@ -1,4 +1,5 @@
 import type { ThemeMode } from '@/types/theme';
+import type { DesktopSettings } from '@/lib/desktop';
 import { DEFAULT_DARK_THEME_ID, DEFAULT_LIGHT_THEME_ID, getThemeById } from '@/lib/theme/themes';
 import { isTransientRuntimeKey } from '@/lib/runtime-switch';
 
@@ -179,3 +180,48 @@ export const adoptThemePreferencesForRuntime = (
   runtimeKey: string,
   current: StoredThemePreferences,
 ): StoredThemePreferences => readThemePreferencesForRuntime(runtimeKey) ?? current;
+
+type SettingsSyncThemePayload = Pick<
+  DesktopSettings,
+  'useSystemTheme' | 'themeVariant' | 'lightThemeId' | 'darkThemeId'
+>;
+
+/**
+ * Resolve the preferences a settings sync should apply. Returns null — meaning
+ * "keep current preferences" — when the sync is not bootstrap-grade (a
+ * mid-session save echo replays the server document, which may hold another
+ * window's theme, a default, or a stale value; the scoped entry and bootstrap
+ * syncs own this window's theme) and when the resolved values already match
+ * the current ones (the identity check breaks adoption loops).
+ *
+ * Theme fields absent from the server document mean "not set", not "reset to
+ * defaults": each one independently keeps the current preference. The payload
+ * is the already-sanitized settings document (sanitizeWebSettings), so fields
+ * present in it are domain-valid; an unknown id injected some other way fails
+ * theme lookup downstream and falls back cosmetically.
+ */
+export const resolveThemePreferencesFromSettingsSync = (
+  detail: { adoptTheme: boolean; settings: SettingsSyncThemePayload } | null,
+  current: StoredThemePreferences,
+): StoredThemePreferences | null => {
+  if (!detail?.adoptTheme) {
+    return null;
+  }
+
+  const settings = detail.settings;
+  let themeMode = current.themeMode;
+  if (settings.useSystemTheme === true) {
+    themeMode = 'system';
+  } else if (settings.useSystemTheme === false && (settings.themeVariant === 'dark' || settings.themeVariant === 'light')) {
+    themeMode = settings.themeVariant;
+  }
+
+  const lightThemeId = settings.lightThemeId?.trim() || current.lightThemeId;
+  const darkThemeId = settings.darkThemeId?.trim() || current.darkThemeId;
+
+  if (themeMode === current.themeMode && lightThemeId === current.lightThemeId && darkThemeId === current.darkThemeId) {
+    return null;
+  }
+
+  return { themeMode, lightThemeId, darkThemeId };
+};

packages/ui/src/lib/persistence.test.ts

@@ -15,6 +15,7 @@ import {
   subscribeToSettingsSaveState,
   syncDesktopSettings,
   updateDesktopSettings,
+  type SettingsSyncedDetail,
 } from './persistence';
 import { switchRuntimeEndpoint } from './runtime-switch';
 
@@ -843,6 +844,100 @@ describe('updateDesktopSettings', () => {
     expect(useUIStore.getState().autoSaveEnabled).toBe(true);
     expect(saveCalls.some((changes) => changes.autoSaveEnabled === true)).toBe(true);
   });
+
+  test('does not invent theme defaults when the authoritative snapshot omits theme fields', async () => {
+    getWindow();
+    invalidateSettingsCache();
+    registerSettingsApi(
+      // SAFETY: this mock echoes back exactly the partial changes it received;
+      // the tests below only read fields the changes actually contain.
+      async (changes) => ({ ...changes } as SettingsPayload),
+      async () => ({
+        settings: { draftStartersCraftGoalAdded: true, draftStartersScheduleTaskAdded: true },
+        source: 'web',
+      }),
+    );
+
+    const synced: SettingsSyncedDetail[] = [];
+    const listener = (event: Event): void => {
+      // SAFETY: dispatchSettingsSynced is the only emitter for this key and
+      // always sends a CustomEvent<SettingsSyncedDetail>.
+      const detail = (event as CustomEvent<SettingsSyncedDetail>).detail;
+      if (detail) synced.push(detail);
+    };
+    window.addEventListener('openchamber:settings-synced', listener);
+    try {
+      await syncDesktopSettings();
+    } finally {
+      window.removeEventListener('openchamber:settings-synced', listener);
+    }
+
+    expect(synced.length).toBeGreaterThan(0);
+    const bootstrapSync = synced.find((detail) => detail.bootstrap);
+    expect(bootstrapSync).toBeTruthy();
+    expect(bootstrapSync?.adoptTheme).toBe(true);
+    expect(bootstrapSync?.settings.useSystemTheme).toBe(undefined);
+    expect(bootstrapSync?.settings.lightThemeId).toBe(undefined);
+    expect(bootstrapSync?.settings.darkThemeId).toBe(undefined);
+  });
+
+  test('marks settings save echoes as non-bootstrap syncs', async () => {
+    getWindow();
+    invalidateSettingsCache();
+    registerSettingsApi(
+      // SAFETY: this mock echoes back exactly the partial changes it received;
+      // the assertions below only read fields the changes actually contain.
+      async (changes) => ({ ...changes } as SettingsPayload),
+    );
+
+    const synced: SettingsSyncedDetail[] = [];
+    const listener = (event: Event): void => {
+      // SAFETY: dispatchSettingsSynced is the only emitter for this key and
+      // always sends a CustomEvent<SettingsSyncedDetail>.
+      const detail = (event as CustomEvent<SettingsSyncedDetail>).detail;
+      if (detail) synced.push(detail);
+    };
+    window.addEventListener('openchamber:settings-synced', listener);
+    try {
+      await updateDesktopSettings({ themeVariant: 'dark' });
+    } finally {
+      window.removeEventListener('openchamber:settings-synced', listener);
+    }
+
+    expect(synced.length).toBeGreaterThan(0);
+    expect(synced.every((detail) => detail.bootstrap === false)).toBe(true);
+    expect(synced.every((detail) => detail.adoptTheme === false)).toBe(true);
+    expect(synced.every((detail) => detail.settings.themeVariant === 'dark')).toBe(true);
+  });
+
+  test('allows a bootstrap sync to preserve the current window theme', async () => {
+    getWindow();
+    invalidateSettingsCache();
+    registerSettingsApi(
+      async (changes) => ({ ...changes } as SettingsPayload),
+      async () => ({
+        settings: { activeProjectId: 'project-a', themeVariant: 'dark' },
+        source: 'web',
+      }),
+    );
+
+    const synced: SettingsSyncedDetail[] = [];
+    const listener = (event: Event): void => {
+      const detail = (event as CustomEvent<SettingsSyncedDetail>).detail;
+      if (detail) synced.push(detail);
+    };
+    window.addEventListener('openchamber:settings-synced', listener);
+    try {
+      await syncDesktopSettings({ adoptTheme: false });
+    } finally {
+      window.removeEventListener('openchamber:settings-synced', listener);
+    }
+
+    const broadcastSync = synced.find((detail) => detail.bootstrap && !detail.adoptTheme);
+    expect(broadcastSync).toBeTruthy();
+    expect(broadcastSync?.settings.activeProjectId).toBe('project-a');
+    expect(broadcastSync?.settings.themeVariant).toBe('dark');
+  });
 });
 
 describe('unload lifecycle flush (#2197)', () => {

packages/ui/src/lib/persistence.ts

@@ -20,7 +20,6 @@ import { runtimeFetch } from '@/lib/runtime-fetch';
 import { isCapacitorApp } from '@/lib/platform';
 import { isTerminalShell } from '@/lib/terminalShell';
 import { getRuntimeKey, subscribeRuntimeEndpointChanged, subscribeRuntimeEndpointWillChange } from '@/lib/runtime-switch';
-import { DEFAULT_DARK_THEME_ID, DEFAULT_LIGHT_THEME_ID } from '@/lib/theme/themes';
 import { DEFAULT_OPEN_IN_APP_ID } from '@/lib/openInApps';
 import { useSessionDisplayStore } from '@/stores/useSessionDisplayStore';
 
@@ -197,20 +196,27 @@ const persistToLocalStorage = (settings: DesktopSettings) => {
 
 export interface SettingsSyncedDetail {
   settings: DesktopSettings;
-  /** Whether listeners may adopt cross-window workspace pointers
-      (activeProjectId / lastDirectory). True only for a bootstrap-grade sync:
-      the settings document is shared by every window of this server, so a
-      mid-session reconciliation adopting them would hijack this window's
-      workspace with another window's choice. */
-  adoptWorkspace: boolean;
+  /** Whether listeners may adopt authoritative state that this window owns a
+      live copy of (workspace pointers, theme). True only for a bootstrap-grade
+      sync: the settings document is shared by every window of this server, so
+      a mid-session reconciliation adopting them would hijack this window's
+      choices with another window's. Every settings save echoes the full
+      document back as a sync event with bootstrap=false — the echo itself is
+      not filtered; listeners gate their adoption on this flag and keep their
+      live state for the fields they own. */
+  bootstrap: boolean;
+  /** Whether this sync may replace this window's theme preferences. VS Code
+      settings broadcasts remain bootstrap-grade for shared workspace pointers,
+      but must not copy one webview's theme into another webview. */
+  adoptTheme: boolean;
 }
 
-const dispatchSettingsSynced = (settings: DesktopSettings, adoptWorkspace: boolean): void => {
+const dispatchSettingsSynced = (settings: DesktopSettings, bootstrap: boolean, adoptTheme = bootstrap): void => {
   if (typeof window === 'undefined') {
     return;
   }
   window.dispatchEvent(new CustomEvent<SettingsSyncedDetail>('openchamber:settings-synced', {
-    detail: { settings, adoptWorkspace },
+    detail: { settings, bootstrap, adoptTheme },
   }));
 };
 
@@ -534,9 +540,11 @@ const materializeAuthoritativeUiSettings = (settings: DesktopSettings): DesktopS
   const defaults = useUIStore.getInitialState();
 
   return {
-    useSystemTheme: true,
-    lightThemeId: DEFAULT_LIGHT_THEME_ID,
-    darkThemeId: DEFAULT_DARK_THEME_ID,
+    // Theme fields are deliberately NOT defaulted: the theme authority is the
+    // ThemeSystemContext (scoped per-runtime entry + bootstrap syncs). A
+    // server document without theme fields means "not set" — inventing
+    // defaults here would clobber the window's theme and write it back to the
+    // server. Absent fields keep the current preferences.
     openInAppId: DEFAULT_OPEN_IN_APP_ID,
     showReasoningTraces: defaults.showReasoningTraces,
     streamingAutoFollowEnabled: defaults.streamingAutoFollowEnabled,
@@ -1896,8 +1904,9 @@ export const invalidateSettingsCache = (): void => {
   _settingsCache = null;
 };
 
-export const syncDesktopSettings = async (options?: { adoptWorkspace?: boolean }): Promise<void> => {
-  const adoptWorkspace = options?.adoptWorkspace !== false;
+export const syncDesktopSettings = async (options?: { bootstrap?: boolean; adoptTheme?: boolean }): Promise<void> => {
+  const bootstrap = options?.bootstrap !== false;
+  const adoptTheme = options?.adoptTheme ?? bootstrap;
   if (typeof window === 'undefined') {
     return;
   }
@@ -2026,7 +2035,7 @@ export const syncDesktopSettings = async (options?: { adoptWorkspace?: boolean }
       if (!isSettingsRuntimeContextCurrent(context)) return;
     }
 
-    dispatchSettingsSynced(authoritativeSettings, adoptWorkspace);
+    dispatchSettingsSynced(authoritativeSettings, bootstrap, adoptTheme);
   };
 
   try {

packages/ui/src/stores/DOCUMENTATION.md

@@ -84,7 +84,7 @@ Permission auto-accept policy is authoritative in the active Web server or VS Co
 
 Shared safe storage treats durable failures per key. A quota or access failure creates an ephemeral override or tombstone for that key without disabling reads and writes for unrelated keys; later writes retry the durable backend. Deferred adapters retain failed operations for a later flush, and malformed Zustand JSON is removed and treated as missing so hydration can recover.
 
-Project and UI settings use successful settings synchronization as authority. Omitted fields in a complete snapshot reset to canonical client defaults, including an omitted project list becoming empty; transport or settings-load failure dispatches no synchronization event and preserves current state. Settings save responses are partial patches and must not clear unrelated in-memory preferences or local mirrors. Debounced settings writes flush best-effort on page hide, document hidden, app freeze, and unload — canceling the pending timer so the write happens exactly once — because a write lost inside the debounce window lets the stale server snapshot override the change on next startup; a hard process kill can still lose the in-flight request. The unload flush uses `keepalive: true` on the HTTP write, because a plain fetch started from `pagehide`/`beforeunload` is cancelled with the document; `navigator.sendBeacon` is not used, as it cannot carry the runtime bearer header. On Capacitor neither `pagehide` nor `beforeunload` fires when the OS suspends the app, so the flush also runs on `App.appStateChange` going inactive.
+Project and UI settings use successful settings synchronization as authority. Omitted fields in a complete snapshot reset to canonical client defaults, including an omitted project list becoming empty. Theme fields are the exception: only bootstrap-grade theme adoption applies fields supplied by the server, while omitted fields preserve this window's current runtime-scoped theme and settings save echoes never adopt a theme. VS Code settings broadcasts may still adopt shared workspace pointers without replacing each webview's editor-derived theme. Transport or settings-load failure dispatches no synchronization event and preserves current state. Settings save responses are partial patches and must not clear unrelated in-memory preferences or local mirrors. Debounced settings writes flush best-effort on page hide, document hidden, app freeze, and unload — canceling the pending timer so the write happens exactly once — because a write lost inside the debounce window lets the stale server snapshot override the change on next startup; a hard process kill can still lose the in-flight request. The unload flush uses `keepalive: true` on the HTTP write, because a plain fetch started from `pagehide`/`beforeunload` is cancelled with the document; `navigator.sendBeacon` is not used, as it cannot carry the runtime bearer header. On Capacitor neither `pagehide` nor `beforeunload` fires when the OS suspends the app, so the flush also runs on `App.appStateChange` going inactive.
 
 Project ordering defaults to manual. Session display persistence v3 migrates the previously shipped `recent` project order to `manual` while preserving every other explicit sort mode.
 

packages/ui/src/stores/useProjectsStore.ts

@@ -1112,7 +1112,7 @@ if (typeof window !== 'undefined') {
     const detail = (event as CustomEvent<SettingsSyncedDetail>).detail;
     if (detail && typeof detail === 'object' && detail.settings) {
       useProjectsStore.getState().synchronizeFromSettings(detail.settings, {
-        adoptActiveProject: detail.adoptWorkspace,
+        adoptActiveProject: detail.bootstrap,
       });
     }
   });

packages/vscode/webview/main.tsx

@@ -1859,7 +1859,7 @@ window.addEventListener('openchamber:vscode-notification-event', (event) => {
 // Listen for settings sync command from extension (broadcast to all VS Code webviews)
 onCommand('settingsSynced', () => {
   import('@openchamber/ui/lib/persistence').then(({ syncDesktopSettings }) => {
-    void syncDesktopSettings();
+    void syncDesktopSettings({ adoptTheme: false });
   });
 });