fix(ui): stop settings save echoes from clobbering theme preferences
Every updateDesktopSettings PUT replays the server's full settings document as an openchamber:settings-synced event, and the theme listener adopted it unconditionally - so switching sessions across directories (any lastDirectory/activeProjectId write) could flip the theme to whatever the server document held at that moment. A bootstrap GET with missing theme fields made it worse: materializeAuthoritativeUiSettings invented useSystemTheme + openchamber defaults and the persist effect wrote them back to the server and the scoped localStorage entry. Theme is now adopted only from bootstrap-grade syncs (the renamed bootstrap flag, formerly adoptWorkspace), missing fields mean 'not set' and keep the current preference, and the materializer no longer invents theme defaults. Cross-window same-instance theme sync still rides the scoped-key storage event; runtime endpoint switches still adopt the new instance's server theme. Closes the 'theme flashes to OpenChamber when switching sessions' report; same family as the pre-#2897 'color mode forced to light' symptom.
This commit is contained in:
@@ -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();
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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({ bootstrap: false, settings: serverTheme }, current)).toBeNull();
|
||||
expect(resolveThemePreferencesFromSettingsSync(null, current)).toBeNull();
|
||||
});
|
||||
|
||||
test('a bootstrap sync adopts the server theme', () => {
|
||||
expect(resolveThemePreferencesFromSettingsSync({ bootstrap: 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({ bootstrap: true, settings: {} }, current)).toBeNull();
|
||||
expect(
|
||||
resolveThemePreferencesFromSettingsSync({ bootstrap: true, settings: { useSystemTheme: true } }, current),
|
||||
).toBeNull();
|
||||
expect(
|
||||
resolveThemePreferencesFromSettingsSync({ bootstrap: 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(
|
||||
{ bootstrap: true, settings: { useSystemTheme: true, lightThemeId: 'light-theme', darkThemeId: 'dark-theme' } },
|
||||
current,
|
||||
),
|
||||
).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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: { bootstrap: boolean; settings: SettingsSyncThemePayload } | null,
|
||||
current: StoredThemePreferences,
|
||||
): StoredThemePreferences | null => {
|
||||
if (!detail?.bootstrap) {
|
||||
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 };
|
||||
};
|
||||
|
||||
@@ -15,6 +15,7 @@ import {
|
||||
subscribeToSettingsSaveState,
|
||||
syncDesktopSettings,
|
||||
updateDesktopSettings,
|
||||
type SettingsSyncedDetail,
|
||||
} from './persistence';
|
||||
import { switchRuntimeEndpoint } from './runtime-switch';
|
||||
|
||||
@@ -843,6 +844,69 @@ 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?.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.settings.themeVariant === 'dark')).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('unload lifecycle flush (#2197)', () => {
|
||||
|
||||
@@ -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,23 @@ 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;
|
||||
}
|
||||
|
||||
const dispatchSettingsSynced = (settings: DesktopSettings, adoptWorkspace: boolean): void => {
|
||||
const dispatchSettingsSynced = (settings: DesktopSettings, bootstrap: boolean): void => {
|
||||
if (typeof window === 'undefined') {
|
||||
return;
|
||||
}
|
||||
window.dispatchEvent(new CustomEvent<SettingsSyncedDetail>('openchamber:settings-synced', {
|
||||
detail: { settings, adoptWorkspace },
|
||||
detail: { settings, bootstrap },
|
||||
}));
|
||||
};
|
||||
|
||||
@@ -534,9 +536,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 +1900,8 @@ 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 }): Promise<void> => {
|
||||
const bootstrap = options?.bootstrap !== false;
|
||||
if (typeof window === 'undefined') {
|
||||
return;
|
||||
}
|
||||
@@ -2026,7 +2030,7 @@ export const syncDesktopSettings = async (options?: { adoptWorkspace?: boolean }
|
||||
if (!isSettingsRuntimeContextCurrent(context)) return;
|
||||
}
|
||||
|
||||
dispatchSettingsSynced(authoritativeSettings, adoptWorkspace);
|
||||
dispatchSettingsSynced(authoritativeSettings, bootstrap);
|
||||
};
|
||||
|
||||
try {
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user