From 79c022b591c1cef74cce9246b893eb8528279b5f Mon Sep 17 00:00:00 2001 From: Ramon Bartl Date: Tue, 15 Sep 2026 09:11:14 +0200 Subject: [PATCH] Stop writing one profile's state into another profileStorage prefixes every localStorage key with the active profile name and gets that name from a fetch on module load. These helpers are synchronous and run during render, so there is a real window on every page load where the name is not known. The old code assumed "default". A write in that window landed under default: while org-mode was active. Worse, a read moved a legacy value into the wrong profile and deleted the original: profileGet migrates an unprefixed key under the current prefix and removes it, and during the window that prefix is default:. The value the active profile owned was filed elsewhere and the original unlinked. That one does not undo itself. Both are reachable. This install has two profiles, and isDeadlineAcked runs while task cards render, well inside the window. The resolved name is now cached in localStorage and read back synchronously on the next load, which closes the window except on the first load after a switch. For that load writes are buffered and flushed once the name arrives, and the migration waits rather than guessing. useSwitchProfile records the new name before its caller reloads, closing that case too. What is left is that a read in the window can return the previous profile's value: wrong on screen for a few milliseconds, and unlike the other two it corrects itself. --- frontend/src/hooks/useSettings.ts | 9 +- .../utils/__tests__/profileStorage.test.ts | 118 +++++++++++++++++ frontend/src/utils/profileStorage.ts | 122 +++++++++++++++--- 3 files changed, 229 insertions(+), 20 deletions(-) create mode 100644 frontend/src/utils/__tests__/profileStorage.test.ts 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"; }