diff --git a/src/apps/officer-web/Screens/Dashboard/Layout/Header/UserMenu.tsx b/src/apps/officer-web/Screens/Dashboard/Layout/Header/UserMenu.tsx index 531da9a2..00b836a0 100644 --- a/src/apps/officer-web/Screens/Dashboard/Layout/Header/UserMenu.tsx +++ b/src/apps/officer-web/Screens/Dashboard/Layout/Header/UserMenu.tsx @@ -1,28 +1,19 @@ import { Link } from 'react-router'; import * as Dropdown from '@/components/ui/dropdown-menu'; import { Avatar, AvatarFallback, AvatarImage } from '@/components/ui/avatar'; -import { User, LogOut, Settings, Puzzle, Sun, Moon, Bot, UserCog, Palette } from 'lucide-react'; +import { User, LogOut, Settings, Puzzle, Bot, UserCog, Palette } from 'lucide-react'; import { useAuth } from 'hooks/useAuth'; import { useTranslation } from '@/lib/i18n'; -import { useColorMode, useTheme } from '@/components/ui/ThemeProvider'; -import { useSettings } from 'state/useSettings'; +import { useTheme } from '@/components/ui/ThemeProvider'; const { DropdownMenu, DropdownMenuContent, DropdownMenuItem, DropdownMenuSeparator, DropdownMenuTrigger } = Dropdown; export function UserMenu() { const { user, isLoading } = useAuth(); const { t } = useTranslation(); - const { colorMode, setColorMode } = useColorMode(); const { themeId, setTheme, themes } = useTheme(); - const { settings, saveSettings } = useSettings(); if (isLoading) return null; - const toggleColorMode = () => { - const next = colorMode === 'dark' ? 'light' : 'dark'; - setColorMode(next); - saveSettings({ ...settings, appearance: { ...settings.appearance, colorMode: next } }); - }; - return ( @@ -37,9 +28,12 @@ export function UserMenu() { {/* - Every theme this build ships, listed. Deliberately the crudest possible control: the point tonight - is proving the app can wear a different skin at all, not designing a picker. A real one belongs in - an appearance settings screen, which does not exist yet. + Every theme this build ships. This replaced a Light/Dark toggle, which had become a second way to + do the same thing — light and dark are two themes now, both already in this list, and the toggle + was the last thing writing the legacy `appearance.colorMode`. + + Deliberately the crudest possible control: proving the app can wear another skin, not designing a + picker. A real one belongs in an appearance settings screen, which does not exist yet. */} {themes.map((theme) => ( setTheme(theme.id)} className="cursor-pointer"> @@ -87,11 +81,6 @@ export function UserMenu() { )} - - {colorMode === 'dark' ? : } - {colorMode === 'dark' ? 'Light Mode' : 'Dark Mode'} - - diff --git a/src/apps/officer-web/state/useThemeSync.ts b/src/apps/officer-web/state/useThemeSync.ts index c02dea75..ceda0d91 100644 --- a/src/apps/officer-web/state/useThemeSync.ts +++ b/src/apps/officer-web/state/useThemeSync.ts @@ -1,46 +1,63 @@ import { useEffect, useRef } from 'react'; -import { useColorMode, useTheme } from '@/components/ui/ThemeProvider'; +import { useTheme } from '@/components/ui/ThemeProvider'; +import { themeFromLegacyColorMode, isKnownTheme } from 'theme'; import { useSettings } from 'state/useSettings'; /** - * Bring the account's saved appearance down from the server, once. + * Keep the account's theme and this browser's theme agreed, in both directions. * - * ── Why "once" is the whole point ── + * ── Why it is two effects and not one ── * - * This used to re-apply `settings.appearance.colorMode` whenever the settings query resolved, which was - * correct while colour mode was the ONLY appearance state — there was nothing else for it to overwrite. - * With themes there is: picking Plain and reloading put Plain in localStorage, and then this hook stamped - * `colorMode: 'dark'` back over it a moment later. The theme changed and then silently changed back, which - * looks like the picker not working. + * They answer different questions. HYDRATE asks "this browser has never chosen — what does the account + * say?" and runs once. PERSIST asks "the person just chose — tell the account", and runs on every change. + * Collapsing them produces the bug this file used to have: the server's value re-applied on every settings + * fetch, so choosing Plain and reloading silently reverted it. * - * So the server's value is treated as what it is — a STARTING POINT for a browser that has never chosen — - * rather than as a continuous instruction. A local choice wins, because it is the more recent statement of - * what this person wants, and it is already persisted. + * A local choice wins on load because it is the more recent statement of intent AND it is already on the + * screen — re-applying the server's value would be a visible flip a second after the page settles. * - * The proper fix is for the server to store a theme id rather than a colour mode, at which point this hook - * becomes a one-line sync of the same field in both places. That needs a settings migration and is - * deliberately not in the extraction commit. + * ── Migration ── + * + * Accounts predating themes stored `appearance.colorMode`. It is read once, mapped to the matching Duck + * theme, and never written again. Somebody who chose dark a year ago keeps dark. + * + * Worth knowing: the server's default was `appearance: { theme: 'light' }` while the client read + * `appearance.colorMode`, so that default had never been read by anything. Both sides say `theme` now. */ -const LOCAL_CHOICE_KEY = 'officer-theme'; +const LOCAL_KEY = 'officer-theme'; export const useColorModeSync = () => { - const { settings } = useSettings(); - const { setColorMode } = useColorMode(); - const { themeId } = useTheme(); - const applied = useRef(false); + const { settings, settingsLoaded, saveSettings } = useSettings(); + const { themeId, setTheme } = useTheme(); + const hydrated = useRef(false); + // HYDRATE — once, and only into a browser that has not chosen for itself. useEffect(() => { - if (!settings || applied.current) return; - applied.current = true; + // `settingsLoaded`, not `settings` — see its note. `settings` is the DEFAULTS until the request + // lands, and acting on those is how this hook overwrote the account's real preference with the + // default one and then saved it back. + if (!settingsLoaded || hydrated.current) return; + hydrated.current = true; - // This browser has already been told what to wear. The server's colour mode is older information. try { - if (window.localStorage.getItem(LOCAL_CHOICE_KEY)) return; + if (window.localStorage.getItem(LOCAL_KEY)) return; } catch { - // Storage unavailable — fall through and honour the server, which is the best guess left. + // Storage unavailable. Honouring the account is the best guess left. } - const saved = settings.appearance?.colorMode; - if (saved === 'light' || saved === 'dark') setColorMode(saved); - }, [settings, setColorMode, themeId]); + const saved = settings.appearance?.theme; + if (isKnownTheme(saved)) { + setTheme(saved); + return; + } + const migrated = themeFromLegacyColorMode(settings.appearance?.colorMode); + if (migrated) setTheme(migrated); + }, [settingsLoaded, settings, setTheme]); + + // PERSIST — so the choice follows the account to another browser rather than living in one localStorage. + useEffect(() => { + if (!settingsLoaded || !hydrated.current) return; + if (settings.appearance?.theme === themeId) return; + saveSettings({ ...settings, appearance: { ...settings.appearance, theme: themeId } }); + }, [themeId, settingsLoaded, settings, saveSettings]); }; diff --git a/src/servers/api/settings/settings.ts b/src/servers/api/settings/settings.ts index 017c92e6..4c7a689a 100644 --- a/src/servers/api/settings/settings.ts +++ b/src/servers/api/settings/settings.ts @@ -11,7 +11,9 @@ const DEFAULT_SETTINGS = { defaultPwd: '~', }, appearance: { - theme: 'light', + // A theme id, not a colour mode. `'light'` sat here for a long time while the client read + // `appearance.colorMode`, so this default was never once read by anything. + theme: 'duck-light', }, }; @@ -22,6 +24,33 @@ export const settingsRouter = createRouter(); settingsRouter.route('/', selfPermissionsRouter); // GET /settings — return user settings from DB, default if empty +/** + * An account from before themes existed, expressed in the vocabulary that replaced it. + * + * Themes superseded `appearance.colorMode`, and an existing account still has one. The migration has to + * happen HERE rather than in the client, and that is the whole point of this function: the client merges + * `DEFAULT_SETTINGS` over whatever arrives, so by the time it sees the data, a missing `theme` has already + * been filled in with the default — and a default is indistinguishable from a choice. Verified the wrong + * way round first: an account holding only `colorMode: 'dark'` came up light, because the client's own + * default `theme` masked it. + * + * The server sees the stored object before anything is merged over it, so it is the only place that can + * tell "never chose a theme" from "chose the default one". + * + * Read-only: nothing is written back here. A GET that rewrites the row on every call is a surprise, and + * the client persists the resolved theme on its next change anyway. + */ +function withThemeMigrated(settings: Record): Record { + const appearance = settings.appearance as { theme?: unknown; colorMode?: unknown } | undefined; + if (!appearance || typeof appearance.theme === 'string') return settings; + + const migrated = + appearance.colorMode === 'dark' ? 'duck-dark' : appearance.colorMode === 'light' ? 'duck-light' : undefined; + if (!migrated) return settings; + + return { ...settings, appearance: { ...appearance, theme: migrated } }; +} + settingsRouter.get('/settings', async (ctx) => { const userId = ctx.get('user').id; const settings = await getUserSettings(userId); @@ -31,7 +60,7 @@ settingsRouter.get('/settings', async (ctx) => { return ctx.json(DEFAULT_SETTINGS); } - return ctx.json(settings); + return ctx.json(withThemeMigrated(settings as Record)); }); // PUT /settings — full replacement diff --git a/src/workspaces/state/src/useSettings.ts b/src/workspaces/state/src/useSettings.ts index 4ded21eb..a167d8dd 100644 --- a/src/workspaces/state/src/useSettings.ts +++ b/src/workspaces/state/src/useSettings.ts @@ -23,7 +23,19 @@ export const useSettings = () => { const { isAuthenticated } = useAuth(); const queryClient = useQueryClient(); - const { data: settings = DEFAULT_SETTINGS } = useQuery({ + const { + data: settings = DEFAULT_SETTINGS, + /** + * Whether the ACCOUNT's settings have arrived, as opposed to the defaults standing in for them. + * + * `settings` is never null — it falls back to `DEFAULT_SETTINGS`, which is a complete and entirely + * valid object. So a consumer that wants to act once on the saved value cannot tell "not loaded yet" + * from "loaded, and it happens to equal the defaults". `useColorModeSync` could not, and applied the + * default theme on first render, marked itself done, and never saw the real one — then persisted that + * default over the account's actual preference. + */ + isSuccess: settingsLoaded, + } = useQuery({ queryKey: QUERY_KEY, enabled: isAuthenticated, queryFn: async () => { @@ -41,7 +53,7 @@ export const useSettings = () => { [client, queryClient], ); - return { settings, saveSettings }; + return { settings, settingsLoaded, saveSettings }; }; export type UseSettingsType = ReturnType; @@ -66,7 +78,13 @@ export type UserSettings = { defaultModel: string | null; }; appearance: { - colorMode: 'light' | 'dark'; + /** The active theme's id — `duck-light`, `duck-dark`, `plain`, … */ + theme: string; + /** + * The pre-themes preference, read once to migrate an existing account and never written again. + * Optional because a new account has only `theme`. + */ + colorMode?: 'light' | 'dark'; }; languages: { spoken: string[]; @@ -99,7 +117,7 @@ export const DEFAULT_SETTINGS: UserSettings = { defaultModel: 'claude-code', }, appearance: { - colorMode: 'light', + theme: 'duck-light', }, languages: { spoken: ['en'],