From 726c3825248781334a86da2deb991e5970075028 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 14 Aug 2026 03:12:50 +0200 Subject: [PATCH] =?UTF-8?q?refactor(account):=20address=20account-pin=20re?= =?UTF-8?q?-hydrate=20review=20=E2=80=94=20extract=20module,=20harden=20gu?= =?UTF-8?q?ards?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the 5 findings from /code-review high on the account-pin work: - Extract the reactive-account primitives (ACTIVE_ACCOUNT_KEY, activeAccountId, notifyActiveAccountChange, useActiveAccountId) into a new dependency-free lib/activeAccount.ts, breaking the storage.ts <-> useAccountRehydrate.ts import cycle. storage.ts / api/core.ts / useAccountRehydrate.ts now import from it. - NavigationProvider: the page re-hydrate now PRESERVES existing history.state markers (spread, not overwrite), and openChannel/enterYtSearch markDirty so a sub-view opened before the account pins isn't clobbered by the page override. - useAccountRehydrate: reset the dirty flag after each account transition, so a future non-reloading account switch can't skip the re-hydrate and leak the prior account's state. - Doc-link useAccountRehydrate to the pre-existing useEffect([me?.id]) server-adopt pattern. - Tests: model a mutable pinned account and add useSortState integration tests for the null->id re-hydrate + the markDirty no-clobber guard (vitest 115->117). gate-green: vite build + vitest 117 + eslint-0 + knip + prettier. Self-E2E'd on a fresh tab: still lands on Plex with Movies/In-progress/Year restored, zero console errors. --- .../src/components/NavigationProvider.tsx | 43 ++++++++++------ frontend/src/lib/activeAccount.ts | 39 ++++++++++++++ frontend/src/lib/api/core.ts | 2 +- frontend/src/lib/storage.ts | 39 ++------------ .../src/lib/useAccountRehydrate.dom.test.ts | 2 +- frontend/src/lib/useAccountRehydrate.ts | 11 +++- frontend/src/lib/useFilterState.dom.test.ts | 6 +-- frontend/src/lib/useSortState.dom.test.ts | 51 +++++++++++++++---- 8 files changed, 126 insertions(+), 67 deletions(-) create mode 100644 frontend/src/lib/activeAccount.ts diff --git a/frontend/src/components/NavigationProvider.tsx b/frontend/src/components/NavigationProvider.tsx index dfbfbf71..9e26bb2b 100644 --- a/frontend/src/components/NavigationProvider.tsx +++ b/frontend/src/components/NavigationProvider.tsx @@ -114,7 +114,10 @@ export function NavigationProvider({ children }: { children: ReactNode }) { if (stored === pageRef.current) return; setPageState(stored); setAccountRaw(PAGE_KEY, stored); // no-op re-write, keeps storage authoritative - window.history.replaceState({ sfPage: stored }, ""); + // Correct the CURRENT entry (replace, not push) and PRESERVE any markers already on it — a channel + // (_chan) or YT search (_yt) opened in the pre-pin window must survive, or its overlay is orphaned + // from history and Back can't close it. (Those setters also markDirty, so this usually no-ops then.) + window.history.replaceState({ ...(window.history.state || {}), sfPage: stored }, ""); }); const setPage = useCallback( @@ -134,27 +137,35 @@ export function NavigationProvider({ children }: { children: ReactNode }) { [markDirty], ); - const openChannel = useCallback((id: string, name?: string) => { - setChannelView({ id, name }); - const st = window.history.state || {}; - if (st._chan) window.history.replaceState({ ...st, _chan: id, _chanName: name ?? null }, ""); - else window.history.pushState({ ...st, _chan: id, _chanName: name ?? null }, ""); - }, []); + const openChannel = useCallback( + (id: string, name?: string) => { + markDirty(); // opening a sub-view is a navigation — don't let a late account-pin re-hydrate override the page + setChannelView({ id, name }); + const st = window.history.state || {}; + if (st._chan) window.history.replaceState({ ...st, _chan: id, _chanName: name ?? null }, ""); + else window.history.pushState({ ...st, _chan: id, _chanName: name ?? null }, ""); + }, + [markDirty], + ); const closeChannel = useCallback(() => { if (window.history.state?._chan) window.history.back(); else setChannelView(null); }, []); - const enterYtSearch = useCallback((q: string) => { - setYtSearch(q); - // Drop any prior _ytScrape marker — the new search's source isn't known yet; Feed re-stamps it - // once the results resolve, so a reload only restores a confirmed scrape-sourced search. - const { _ytScrape: _drop, ...st } = window.history.state || {}; - if (st._yt) - window.history.replaceState({ ...st, _yt: q }, ""); // refine current search - else window.history.pushState({ ...st, sfPage: "feed", _yt: q }, ""); // new sub-view entry - }, []); + const enterYtSearch = useCallback( + (q: string) => { + markDirty(); // a sub-view navigation — see openChannel + setYtSearch(q); + // Drop any prior _ytScrape marker — the new search's source isn't known yet; Feed re-stamps it + // once the results resolve, so a reload only restores a confirmed scrape-sourced search. + const { _ytScrape: _drop, ...st } = window.history.state || {}; + if (st._yt) + window.history.replaceState({ ...st, _yt: q }, ""); // refine current search + else window.history.pushState({ ...st, sfPage: "feed", _yt: q }, ""); // new sub-view entry + }, + [markDirty], + ); const exitYtSearch = useCallback(() => { // Pop our search entry (so Forward still works); fall back to a plain clear if we don't own one. diff --git a/frontend/src/lib/activeAccount.ts b/frontend/src/lib/activeAccount.ts new file mode 100644 index 00000000..e9813435 --- /dev/null +++ b/frontend/src/lib/activeAccount.ts @@ -0,0 +1,39 @@ +import { useSyncExternalStore } from "react"; + +// The account a browser TAB is acting as (per-tab, in sessionStorage — see api.ts), plus its reactive +// wrapper. Its OWN module (imports nothing internal) so it can be shared by both `storage.ts` (which +// keys per-account localStorage on it) and `useAccountRehydrate.ts` without a circular import — the +// account id is a lower-level primitive than the storage helpers that consume it. +// +// ACTIVE_ACCOUNT_KEY is duplicated here rather than imported from api.ts so this stays dependency-free. +export const ACTIVE_ACCOUNT_KEY = "siftlode.activeAccount"; + +export function activeAccountId(): number | null { + try { + const raw = sessionStorage.getItem(ACTIVE_ACCOUNT_KEY); + if (!raw) return null; + const n = Number(raw); + return Number.isFinite(n) ? n : null; + } catch { + return null; + } +} + +// Reactive active-account. setActiveAccount/clearActiveAccount write sessionStorage — a silent op that +// triggers no React render — so this tab's per-account state (sort/filter/view/layout, all keyed by the +// account) would never re-read once the account is finally known. That bites exactly once, on first +// load: the provider tree above mounts with NO account, then pins it after /api/me resolves. +// The account setters call notifyActiveAccountChange() so useActiveAccountId subscribers can re-hydrate. +const accountListeners = new Set<() => void>(); +function subscribeActiveAccount(cb: () => void): () => void { + accountListeners.add(cb); + return () => accountListeners.delete(cb); +} +export function notifyActiveAccountChange(): void { + for (const cb of [...accountListeners]) cb(); +} +/** The current tab's active account id, as reactive state — re-renders when it is (re)pinned. The + * snapshot is a primitive (number | null), so useSyncExternalStore's Object.is check is stable. */ +export function useActiveAccountId(): number | null { + return useSyncExternalStore(subscribeActiveAccount, activeAccountId, activeAccountId); +} diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index 4074d8fb..9fa40cb2 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -5,7 +5,7 @@ import { notify, remove } from "../notifications"; import i18n from "../../i18n"; import { reportError } from "../errorDialog"; -import { activeAccountId, ACTIVE_ACCOUNT_KEY, notifyActiveAccountChange } from "../storage"; +import { activeAccountId, ACTIVE_ACCOUNT_KEY, notifyActiveAccountChange } from "../activeAccount"; export class HttpError extends Error { status: number; diff --git a/frontend/src/lib/storage.ts b/frontend/src/lib/storage.ts index 3afddc4b..ee1759c5 100644 --- a/frontend/src/lib/storage.ts +++ b/frontend/src/lib/storage.ts @@ -1,4 +1,5 @@ -import { useCallback, useState, useSyncExternalStore } from "react"; +import { useCallback, useState } from "react"; +import { activeAccountId } from "./activeAccount"; import { useAccountRehydrate } from "./useAccountRehydrate"; // Central registry of every localStorage key the app uses. One place to see them all, rename @@ -68,40 +69,8 @@ export const LS = { } as const; // --- per-account scoping ----------------------------------------------------------------- -// The account a browser TAB is acting as (per-tab, in sessionStorage — see api.ts). Duplicated -// here (rather than imported from api.ts) so storage.ts stays dependency-free and usable by the -// low-level helpers below. -export const ACTIVE_ACCOUNT_KEY = "siftlode.activeAccount"; - -export function activeAccountId(): number | null { - try { - const raw = sessionStorage.getItem(ACTIVE_ACCOUNT_KEY); - if (!raw) return null; - const n = Number(raw); - return Number.isFinite(n) ? n : null; - } catch { - return null; - } -} - -// Reactive active-account. setActiveAccount/clearActiveAccount write sessionStorage — a silent op that -// triggers no React render — so this tab's per-account state (sort/filter/view/layout, all keyed by the -// account) would never re-read once the account is finally known. That bites exactly once, on first -// load: the provider tree above mounts with NO account, then pins it after /api/me resolves. -// The account setters call notifyActiveAccountChange() so useActiveAccountId subscribers can re-hydrate. -const accountListeners = new Set<() => void>(); -function subscribeActiveAccount(cb: () => void): () => void { - accountListeners.add(cb); - return () => accountListeners.delete(cb); -} -export function notifyActiveAccountChange(): void { - for (const cb of [...accountListeners]) cb(); -} -/** The current tab's active account id, as reactive state — re-renders when it is (re)pinned. The - * snapshot is a primitive (number | null), so useSyncExternalStore's Object.is check is stable. */ -export function useActiveAccountId(): number | null { - return useSyncExternalStore(subscribeActiveAccount, activeAccountId, activeAccountId); -} +// The active-account primitive (activeAccountId / useActiveAccountId / notify) lives in ./activeAccount +// so it can be shared without a cycle; the helpers below key localStorage on it. /** Suffix a base localStorage key with an account id so per-account UI state (filters, selected * playlist, notifications, tab positions, cached prefs…) is NOT shared across the accounts signed diff --git a/frontend/src/lib/useAccountRehydrate.dom.test.ts b/frontend/src/lib/useAccountRehydrate.dom.test.ts index a9aac577..8ba5cacd 100644 --- a/frontend/src/lib/useAccountRehydrate.dom.test.ts +++ b/frontend/src/lib/useAccountRehydrate.dom.test.ts @@ -6,7 +6,7 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; import { renderHook, act } from "@testing-library/react"; let account: number | null = null; -vi.mock("./storage", () => ({ +vi.mock("./activeAccount", () => ({ useActiveAccountId: () => account, })); diff --git a/frontend/src/lib/useAccountRehydrate.ts b/frontend/src/lib/useAccountRehydrate.ts index 980ea6f6..51f7cdac 100644 --- a/frontend/src/lib/useAccountRehydrate.ts +++ b/frontend/src/lib/useAccountRehydrate.ts @@ -1,5 +1,5 @@ import { useCallback, useEffect, useRef } from "react"; -import { useActiveAccountId } from "./storage"; +import { useActiveAccountId } from "./activeAccount"; // Re-read per-account persisted state when this tab's active account becomes known (or changes). The // provider tree above mounts before the account is pinned, so its useState initializers seed from @@ -9,6 +9,11 @@ import { useActiveAccountId } from "./storage"; // // Keyed on the account id ONLY — the user's own writes go through setAccountRaw, which does not change // the account id, so this effect can't loop on them. +// +// This is the localStorage-only counterpart of the `useEffect([me?.id])` server-prefs adopt pattern in +// FeedFiltersProvider / FeedViewProvider / PrefsProvider (and App's shell prefs): those re-load from the +// SERVER when the account query resolves; this re-loads from localStorage when the account is pinned. +// A provider needs one or the other — not both — to survive a fresh tab's first paint. export function useAccountRehydrate(rehydrate: () => void): () => void { const accountId = useActiveAccountId(); const dirty = useRef(false); @@ -22,6 +27,10 @@ export function useAccountRehydrate(rehydrate: () => void): () => void { if (accountId === prev.current) return; // mount, and renders with no real transition prev.current = accountId; if (!dirty.current) fn.current(); + // Reset per account: `dirty` means "the user edited under THIS account". Today a real account + // switch full-reloads (fresh instance), but clearing it here keeps the guard correct even if a + // future switch stops reloading — the next account starts clean rather than inheriting dirtiness. + dirty.current = false; }, [accountId]); return useCallback(() => { diff --git a/frontend/src/lib/useFilterState.dom.test.ts b/frontend/src/lib/useFilterState.dom.test.ts index bac35ee8..76d4156f 100644 --- a/frontend/src/lib/useFilterState.dom.test.ts +++ b/frontend/src/lib/useFilterState.dom.test.ts @@ -12,10 +12,10 @@ vi.mock("./storage", () => ({ setAccountRaw: (k: string, v: string) => { store.set(k, v); }, - // A stable account id keeps useAccountRehydrate a no-op here (no null→id transition); the re-hydrate - // path itself is covered in useAccountRehydrate.dom.test.ts. - useActiveAccountId: () => 1, })); +// A stable account id keeps useAccountRehydrate a no-op here (no null→id transition); the re-hydrate +// path itself is covered in useAccountRehydrate.dom.test.ts + useSortState.dom.test.ts. +vi.mock("./activeAccount", () => ({ useActiveAccountId: () => 1 })); import { useFilterState } from "./useFilterState"; diff --git a/frontend/src/lib/useSortState.dom.test.ts b/frontend/src/lib/useSortState.dom.test.ts index 4cf751f7..758e4249 100644 --- a/frontend/src/lib/useSortState.dom.test.ts +++ b/frontend/src/lib/useSortState.dom.test.ts @@ -6,21 +6,27 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; import { renderHook, act } from "@testing-library/react"; -const { store } = vi.hoisted(() => ({ store: new Map() })); -vi.mock("./storage", () => ({ - getAccountRaw: (k: string) => store.get(k) ?? null, - setAccountRaw: (k: string, v: string) => { - store.set(k, v); - }, - // A stable account id keeps useAccountRehydrate a no-op here (no null→id transition); the re-hydrate - // path itself is covered in useAccountRehydrate.dom.test.ts. - useActiveAccountId: () => 1, +// `ctl.account` models the tab's pinned account: getAccountRaw returns null while it's null (mirrors the +// real accountKey → no key → null), so a fresh-tab mount reads defaults until the account is pinned. +const { store, ctl } = vi.hoisted(() => ({ + store: new Map(), + ctl: { account: 1 as number | null }, })); +vi.mock("./storage", () => ({ + getAccountRaw: (k: string) => (ctl.account == null ? null : (store.get(k) ?? null)), + setAccountRaw: (k: string, v: string) => { + if (ctl.account != null) store.set(k, v); + }, +})); +vi.mock("./activeAccount", () => ({ useActiveAccountId: () => ctl.account })); import { useSortState } from "./useSortState"; describe("useSortState", () => { - beforeEach(() => store.clear()); + beforeEach(() => { + store.clear(); + ctl.account = 1; + }); it("hydrates to the fallback when storage is empty", () => { const { result } = renderHook(() => useSortState("k")); @@ -56,4 +62,29 @@ describe("useSortState", () => { const { result } = renderHook(() => useSortState("k")); expect(result.current[0]).toEqual({ key: "anything", dir: "desc" }); }); + + // The account-pin re-hydrate (via useAccountRehydrate) integrated into useSortState. + it("re-hydrates the stored sort once the account is pinned after mount", () => { + ctl.account = null; // fresh tab: no account at mount + store.set("k", JSON.stringify({ key: "views", dir: "desc" })); + const { result, rerender } = renderHook(() => useSortState("k")); + expect(result.current[0]).toBeNull(); // mount read with no account → default + act(() => { + ctl.account = 1; // pins the account + rerender(); + }); + expect(result.current[0]).toEqual({ key: "views", dir: "desc" }); + }); + + it("does NOT clobber a pre-pin user edit when the account is then pinned (markDirty guard)", () => { + ctl.account = null; + store.set("k", JSON.stringify({ key: "views", dir: "desc" })); // the stored value + const { result, rerender } = renderHook(() => useSortState("k")); + act(() => result.current[1]({ key: "name", dir: "asc" })); // user edits before the pin + act(() => { + ctl.account = 1; + rerender(); + }); + expect(result.current[0]).toEqual({ key: "name", dir: "asc" }); // the edit survives, not "views" + }); });