From 997f7e7a873c719c945c63b05dd0c8b45025084e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sun, 16 Aug 2026 23:31:22 +0000 Subject: [PATCH] the account stores a theme, not a colour mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Your theme now follows the account instead of living in one browser's localStorage. Choosing Plain on a laptop reaches the phone. `appearance.colorMode` is superseded by `appearance.theme`. Along the way this resolves a mismatch that had been sitting there: the server's default was `appearance: { theme: 'light' }` while the client read `appearance.colorMode`, so that default had never once been read by anything. Both sides say `theme` now. The Light/Dark item is gone from the user menu. Light and dark are two themes, both already in the list above it, and that toggle was the last writer of the legacy key. ── Two bugs found by testing this, both mine, both silent ── **A default that looked like a choice.** `useSettings` returns `DEFAULT_SETTINGS` until the request lands — `settings` is never null, it is a complete valid object. So "wait for settings" fired on the first render against the defaults, marked itself done, applied the default theme and never saw the real one. Then the persist side wrote that default over the account's actual preference. It overwrote the owner's own `dark` while I was testing it; restored by hand from the `colorMode` it had not yet destroyed. `useSettings` now exposes `settingsLoaded` so the difference is expressible. **Defaults masking a migration.** With that fixed, an account holding only `colorMode: 'dark'` STILL came up light. The client merges `DEFAULT_SETTINGS` over whatever arrives, so a missing `theme` is filled in with the default before any consumer sees it — and a default is indistinguishable from a choice. The migration therefore belongs on the SERVER, which sees the stored object before anything is merged over it. It is read-only: a GET that rewrites the row on every call is a surprise, and the client persists on its next change anyway. Verified by putting the account into the pre-themes shape and asking the new handler directly, rather than by restarting to find out: stored: {"colorMode":"dark"} -> returned: {"colorMode":"dark","theme":"duck-dark"} ── Needs a restart ── The first server-side change in this work. Everything until now was frontend and a rebuild was enough. 950 pass / 7 fail unchanged. Co-Authored-By: Claude Opus 5 --- .../Dashboard/Layout/Header/UserMenu.tsx | 27 +++---- src/apps/officer-web/state/useThemeSync.ts | 71 ++++++++++++------- src/servers/api/settings/settings.ts | 33 ++++++++- src/workspaces/state/src/useSettings.ts | 26 +++++-- 4 files changed, 105 insertions(+), 52 deletions(-) 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'],