From e6688edbcc80a6dd0b7c99ef16b22b3d3451e4f1 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 14 Aug 2026 03:26:30 +0200 Subject: [PATCH] refactor(nav): drop redundant sub-view markDirty; add useFilterState re-hydrate tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the 2 findings from /code-review high on 726c382: - NavigationProvider: openChannel/enterYtSearch no longer markDirty. A sub-view is an overlay, not a page change — the page re-hydrate's replaceState already spreads the _chan/_yt marker, so the overlay survives on top of the RESTORED page. Removing the markDirty lets a fresh tab still restore the stored landing page under a pre-pin overlay (previously it stuck on the default). setPage keeps markDirty (a real navigation). - useFilterState.dom.test.ts: model a mutable pinned account and add 3 integration tests — null->id re-hydrate adopts the stored map, the migrate seed carries a legacy value forward on re-hydrate when the key is empty, and a pre-pin edit is preserved (markDirty guard). gate-green: vite build + vitest 120 + eslint-0 + knip + prettier. Fresh-tab restore re-E2E'd (lands on Plex with full state, zero console errors). --- .../src/components/NavigationProvider.tsx | 46 +++++++-------- frontend/src/lib/useFilterState.dom.test.ts | 58 ++++++++++++++++--- 2 files changed, 72 insertions(+), 32 deletions(-) diff --git a/frontend/src/components/NavigationProvider.tsx b/frontend/src/components/NavigationProvider.tsx index 9e26bb2b..c96ad3be 100644 --- a/frontend/src/components/NavigationProvider.tsx +++ b/frontend/src/components/NavigationProvider.tsx @@ -115,8 +115,8 @@ export function NavigationProvider({ children }: { children: ReactNode }) { setPageState(stored); setAccountRaw(PAGE_KEY, stored); // no-op re-write, keeps storage authoritative // 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.) + // (_chan) or YT search (_yt) opened in the pre-pin window survives on top of the restored page, + // rather than being orphaned from history so Back can't close it. window.history.replaceState({ ...(window.history.state || {}), sfPage: stored }, ""); }); @@ -137,35 +137,31 @@ export function NavigationProvider({ children }: { children: ReactNode }) { [markDirty], ); - 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], - ); + // A sub-view (channel / YT search) is an OVERLAY, not a page change, so it does NOT markDirty: a + // pre-pin account re-hydrate should still restore the stored page underneath — the re-hydrate's + // replaceState spreads the _chan/_yt markers, so the overlay survives on top of the restored page. + 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 closeChannel = useCallback(() => { if (window.history.state?._chan) window.history.back(); else setChannelView(null); }, []); - 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 enterYtSearch = useCallback((q: string) => { + // Overlay, not a page change — see openChannel: no markDirty, the re-hydrate preserves _yt. + 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 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/useFilterState.dom.test.ts b/frontend/src/lib/useFilterState.dom.test.ts index 76d4156f..c6d5655a 100644 --- a/frontend/src/lib/useFilterState.dom.test.ts +++ b/frontend/src/lib/useFilterState.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() })); +// `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) => store.get(k) ?? null, + getAccountRaw: (k: string) => (ctl.account == null ? null : (store.get(k) ?? null)), setAccountRaw: (k: string, v: string) => { - store.set(k, v); + if (ctl.account != null) 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 + useSortState.dom.test.ts. -vi.mock("./activeAccount", () => ({ useActiveAccountId: () => 1 })); +vi.mock("./activeAccount", () => ({ useActiveAccountId: () => ctl.account })); import { useFilterState } from "./useFilterState"; describe("useFilterState", () => { - beforeEach(() => store.clear()); + beforeEach(() => { + store.clear(); + ctl.account = 1; + }); it("hydrates to an empty map when storage is empty", () => { const { result } = renderHook(() => useFilterState("k")); @@ -71,4 +77,42 @@ describe("useFilterState", () => { // A seed that returns null falls back to an empty map. expect(renderHook(() => useFilterState("k3", () => null)).result.current[0]).toEqual({}); }); + + // The account-pin re-hydrate (via useAccountRehydrate) integrated into useFilterState. + it("re-hydrates the stored map once the account is pinned after mount", () => { + ctl.account = null; // fresh tab: no account at mount + store.set("k", JSON.stringify({ sync: "fully", tags: ["7"] })); + const { result, rerender } = renderHook(() => useFilterState("k")); + expect(result.current[0]).toEqual({}); // mount read with no account → empty + act(() => { + ctl.account = 1; // pins the account + rerender(); + }); + expect(result.current[0]).toEqual({ sync: "fully", tags: ["7"] }); + }); + + it("applies the migrate seed on the account-pin re-hydrate when the key is still empty", () => { + ctl.account = null; + // Mirrors migrateSyncChip: reads an account-scoped legacy key, so it yields null until pinned. + const migrate = () => (ctl.account == null ? null : { sync: "needsFull" }); + const { result, rerender } = renderHook(() => useFilterState("k", migrate)); + expect(result.current[0]).toEqual({}); // migrate can't read the legacy key with no account yet + act(() => { + ctl.account = 1; + rerender(); + }); + expect(result.current[0]).toEqual({ sync: "needsFull" }); // migrate carries the legacy value forward + }); + + 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({ sync: "fully" })); // the stored value + const { result, rerender } = renderHook(() => useFilterState("k")); + act(() => result.current[1]({ sync: "needsFull" })); // user edits before the pin + act(() => { + ctl.account = 1; + rerender(); + }); + expect(result.current[0]).toEqual({ sync: "needsFull" }); // the edit survives, not "fully" + }); });