diff --git a/frontend/src/hooks/useSettings.ts b/frontend/src/hooks/useSettings.ts index 804ba112..edb63191 100644 --- a/frontend/src/hooks/useSettings.ts +++ b/frontend/src/hooks/useSettings.ts @@ -68,6 +68,7 @@ import { updateTag, updateUrlAllowlist, } from "../api/client"; +import { setActiveProfile } from "../utils/profileStorage"; import type { ExternalEditorSettings } from "../api/client"; import type { AiSettings, TaskState } from "../types"; @@ -640,7 +641,13 @@ export function useSwitchProfile() { const qc = useQueryClient(); return useMutation({ mutationFn: (profile: string) => switchProfile(profile), - onSuccess: () => { + onSuccess: (_data, profile) => { + // Record the new name before the caller reloads. + // profileStorage reads this synchronously on the next + // load, which is what keeps the reads immediately + // after a switch on the right profile rather than on + // the previous one until its fetch lands. + setActiveProfile(profile); void qc.invalidateQueries(); }, }); diff --git a/frontend/src/utils/__tests__/profileStorage.test.ts b/frontend/src/utils/__tests__/profileStorage.test.ts new file mode 100644 index 00000000..4abf6a71 --- /dev/null +++ b/frontend/src/utils/__tests__/profileStorage.test.ts @@ -0,0 +1,118 @@ +import { + afterEach, beforeEach, describe, expect, it, vi, +} from "vitest"; + +/** Let every queued microtask and one macrotask run. + * Three Promise.resolve() ticks are not enough for the + * fetch -> json -> setActiveProfile chain. */ +const settle = () => + new Promise((r) => setTimeout(r, 0)); + +/** + * The module fetches the active profile on import and + * caches it, so each test needs a fresh module registry + * with its own fetch stub. + */ +async function loadModule( + profile: string, + opts: { resolve?: boolean } = {}, +) { + const { resolve = true } = opts; + let release!: () => void; + const gate = new Promise((r) => { release = r; }); + + vi.stubGlobal("fetch", vi.fn(async () => { + if (!resolve) await gate; + return { + json: async () => ({ active: profile }), + } as Response; + })); + + vi.resetModules(); + const mod = await import("../profileStorage"); + if (resolve) await settle(); + return { ...mod, release }; +} + +beforeEach(() => { + localStorage.clear(); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe("once the profile is known", () => { + it("prefixes reads and writes with it", async () => { + const m = await loadModule("org-mode"); + expect(m.getActiveProfile()).toBe("org-mode"); + + m.profileSet("filter", "open"); + expect(localStorage.getItem("org-mode:filter")) + .toBe("open"); + expect(m.profileGet("filter")).toBe("open"); + + m.profileRemove("filter"); + expect(m.profileGet("filter")).toBeNull(); + }); + + it("keeps two profiles apart", async () => { + const a = await loadModule("default"); + a.profileSet("filter", "aus default"); + + const b = await loadModule("org-mode"); + expect(b.profileGet("filter")).toBeNull(); + b.profileSet("filter", "aus org-mode"); + + expect(localStorage.getItem("default:filter")) + .toBe("aus default"); + expect(localStorage.getItem("org-mode:filter")) + .toBe("aus org-mode"); + }); + + it("migrates an unprefixed value once", async () => { + localStorage.setItem("filter", "alt"); + const m = await loadModule("org-mode"); + expect(m.profileGet("filter")).toBe("alt"); + expect(localStorage.getItem("org-mode:filter")) + .toBe("alt"); + expect(localStorage.getItem("filter")).toBeNull(); + }); +}); + +describe("before the profile is known", () => { + it("does not write into the wrong profile", async () => { + // The fetch is in flight. A component that renders + // now -- task cards read deadline acks during render + // -- calls straight through. + const m = await loadModule("org-mode", { + resolve: false, + }); + m.profileSet("filter", "gehoert zu org-mode"); + + m.release(); + await settle(); + + expect(localStorage.getItem("default:filter")) + .toBeNull(); + expect(localStorage.getItem("org-mode:filter")) + .toBe("gehoert zu org-mode"); + }); + + it("does not migrate an old value into the wrong profile", + async () => { + localStorage.setItem("filter", "alt"); + const m = await loadModule("org-mode", { + resolve: false, + }); + m.profileGet("filter"); + + m.release(); + await settle(); + + // The unprefixed value belongs to whichever profile + // is actually active, not to "default". + expect(localStorage.getItem("default:filter")) + .toBeNull(); + }); +}); diff --git a/frontend/src/utils/profileStorage.ts b/frontend/src/utils/profileStorage.ts index 327c7878..2c0449f5 100644 --- a/frontend/src/utils/profileStorage.ts +++ b/frontend/src/utils/profileStorage.ts @@ -5,44 +5,119 @@ * switching profiles isolates per-profile state (advisor * chat history, unread badges, view filters, etc.). * - * The active profile is fetched once on module load and - * cached. Since the page reloads on every profile switch, - * this is always current. + * The active profile comes from the server, and that is a + * fetch, which means there is a window on every page load + * where it is not yet known. These helpers are synchronous + * and get called during render, so the window is real: + * task cards read deadline acks while the request is still + * in flight. + * + * What used to happen in that window was to assume + * "default". Two things went wrong with that: + * + * - A write landed under ``default:`` no matter which + * profile was active, so one profile's state leaked + * into another's namespace. + * - A read of a legacy unprefixed key MOVED it under + * ``default:`` and deleted the original. The profile + * it actually belonged to lost it for good. + * + * So: the resolved name is cached in localStorage and read + * back synchronously on the next load, which closes the + * window entirely except on the first load after a switch. + * For that one, writes are buffered until the name + * arrives, and the legacy migration waits too. A read in + * the window can still return the previous profile's + * value, which is wrong on screen for a few milliseconds + * and, unlike the other two, undoes itself. */ -let _profile = "default"; +/** Where the last known profile name is cached. Not + * prefixed: it is what tells us the prefix. */ +const ACTIVE_KEY = "kaisho:activeProfile"; + +let _profile: string | null = null; +/** Writes made before the name was known. */ +const pending = new Map(); + +function readCached(): string | null { + try { + return localStorage.getItem(ACTIVE_KEY); + } catch { + return null; + } +} + +_profile = readCached(); + +/** Apply the buffered writes now that the prefix is + * known, and run any migration that was held back. */ +function flush(): void { + for (const [key, value] of pending) { + if (value === null) { + localStorage.removeItem(prefix(key)); + } else { + localStorage.setItem(prefix(key), value); + } + } + pending.clear(); +} + +export function setActiveProfile(name: string): void { + _profile = name || "default"; + try { + localStorage.setItem(ACTIVE_KEY, _profile); + } catch { + // A full or disabled store is not worth failing over; + // the fetch will resolve it again next load. + } + flush(); +} -// Fetch the active profile name eagerly on module load. -// Falls back to "default" if the fetch fails. +// Confirm (or correct) the cached name. Always runs, even +// when the cache was warm, so a profile switched in another +// window is picked up. fetch("/api/settings/profiles") .then((r) => r.json()) .then((data: { active: string }) => { - _profile = data.active || "default"; + setActiveProfile(data.active || "default"); }) - .catch(() => {}); + .catch(() => { + // Offline or the backend is not up yet. Keep whatever + // the cache said; flush so buffered writes are not + // lost, since "default" is the honest guess when there + // is nothing better. + if (_profile === null) _profile = "default"; + flush(); + }); function prefix(key: string): string { - return `${_profile}:${key}`; + return `${_profile ?? "default"}:${key}`; } /** * Get a profile-scoped value from localStorage. * - * Before the profile is loaded, falls back to the - * unprefixed key for backwards compatibility (first - * page load before the fetch completes). + * A value written in this session but not yet flushed is + * returned from the buffer, so a set/get pair inside the + * window behaves the way a caller expects. */ export function profileGet(key: string): string | null { + if (pending.has(key)) return pending.get(key) ?? null; + const prefixed = localStorage.getItem(prefix(key)); if (prefixed !== null) return prefixed; - // Backwards compat: migrate old unprefixed value + const old = localStorage.getItem(key); - if (old !== null) { + if (old === null) return null; + // Read it, but only move it once the prefix is known. + // Migrating under a guessed prefix deletes the original + // and files it under the wrong profile. + if (_profile !== null) { localStorage.setItem(prefix(key), old); localStorage.removeItem(key); - return old; } - return null; + return old; } /** Set a profile-scoped value in localStorage. */ @@ -50,18 +125,27 @@ export function profileSet( key: string, value: string, ): void { + if (_profile === null) { + pending.set(key, value); + return; + } localStorage.setItem(prefix(key), value); } /** Remove a profile-scoped value from localStorage. */ export function profileRemove(key: string): void { + if (_profile === null) { + pending.set(key, null); + return; + } localStorage.removeItem(prefix(key)); } /** - * Get the current active profile name. - * Returns "default" before the initial fetch completes. + * Get the current active profile name. Returns "default" + * while it is still unknown, which is the right guess on a + * first-ever load and the only one available. */ export function getActiveProfile(): string { - return _profile; + return _profile ?? "default"; }