From 5686759bcad20098d160cdbc3ca276902a9af040 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 01:39:26 +0100 Subject: [PATCH] fix(nav): a page settling keeps a reload's overlay entry; an old entry's sub-view goes to its own (review round 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - useUrlMirror: a change before the user has done anything since the load is the page settling (the rail stepping off a playlist deleted since, Files off a folder gone), not another view: the reload mark stays, and the player that reopens takes its entry over — one Back closes it, as before. - useHistorySubview: an entry an older build wrote is a module's only when its _sub is the view the address names under that module's key (the card's push kept the season's `v` beside its `m`). - Playlists: the rail's default selection and the page's "the rail moved there" read one helper. - moduleUrl contract: writer literals in a choice's branches are scanned too; the sub-view census walks type arguments of any depth and refuses an aliased import. - A stale comment on the nav press over a live search. --- .../navigation/NavigationProvider.tsx | 2 +- frontend/src/lib/history.subview.dom.test.ts | 11 +++- frontend/src/lib/history.ts | 8 ++- .../useUrlMirror.reloadedSettle.dom.test.tsx | 37 +++++++++++ frontend/src/lib/useUrlMirror.ts | 12 +++- frontend/src/moduleUrl.contract.test.ts | 64 +++++++++++++------ .../modules/playlists/Playlists.dom.test.tsx | 20 ++++++ frontend/src/modules/playlists/Playlists.tsx | 3 +- .../src/modules/playlists/PlaylistsRail.tsx | 3 +- .../src/modules/playlists/playlistsInit.ts | 6 ++ 10 files changed, 137 insertions(+), 29 deletions(-) create mode 100644 frontend/src/lib/useUrlMirror.reloadedSettle.dom.test.tsx diff --git a/frontend/src/features/navigation/NavigationProvider.tsx b/frontend/src/features/navigation/NavigationProvider.tsx index 269eb1e1..93989d48 100644 --- a/frontend/src/features/navigation/NavigationProvider.tsx +++ b/frontend/src/features/navigation/NavigationProvider.tsx @@ -144,7 +144,7 @@ export function NavigationProvider({ children }: { children: ReactNode }) { closeOverlaysThenNavigate(() => { // A channel page overlays the content column, so a nav click must close it even when the // underlying page is unchanged (e.g. "Feed" while a channel opened from the feed). - // A live search on it is one too: the press opens the page's root, as over a channel. + // So must a live search on it, though that press ends only the search: the page's view stays. const samePage = next === pageRef.current && !channelViewRef.current; if (samePage && ytSearchRef.current == null) { then?.(); diff --git a/frontend/src/lib/history.subview.dom.test.ts b/frontend/src/lib/history.subview.dom.test.ts index 3ce4e66e..858f56b5 100644 --- a/frontend/src/lib/history.subview.dom.test.ts +++ b/frontend/src/lib/history.subview.dom.test.ts @@ -51,7 +51,7 @@ describe("useHistorySubview seed", () => { }); it("prefers history.state._sub over the URL param (in-app entry)", () => { - setUrl("?v=a", { _sub: "b" }); + setUrl("?v=a", { _sub: "b", _subOf: "v" }); const { result } = renderHook(() => useHistorySubview("root", url)); expect(result.current.view).toBe("b"); }); @@ -161,6 +161,15 @@ describe("useHistorySubview on an entry another module's sub-view stood on", () expect(renderHook(() => useHistorySubview("root", url)).result.current.view).toBe("season"); }); + it("an older build's entry whose address names both: the _sub goes to the one it is the view of", () => { + // The card's push kept the season's `v` beside its own `m`. + setUrl("?v=season&m=register", { _sub: "register" }); + expect(renderHook(() => useHistorySubview("root", url)).result.current.view).toBe("season"); + expect(renderHook(() => useHistorySubview("signin", other)).result.current.view).toBe( + "register", + ); + }); + it("reads its own from an entry it rewrote in place, which no address names", () => { const first = renderHook(() => useHistorySubview("root")); act(() => first.result.current.open("x", { replace: true })); diff --git a/frontend/src/lib/history.ts b/frontend/src/lib/history.ts index 182cb61c..52d660c8 100644 --- a/frontend/src/lib/history.ts +++ b/frontend/src/lib/history.ts @@ -164,12 +164,16 @@ export function useHistorySubview( // The entry's _sub is this module's only when it says so (`_subOf`): one entry can carry another // module's — the sign-in card mounts on the entry a Plex season stood on when signed out there, and // took that season for one of its modes (a card with no button for it). An entry an older build - // wrote says nothing: its _sub is this module's when its address names one of this module's views. + // wrote says nothing: its _sub is this module's when it is the view the address names under this + // module's key. The key alone is not enough: the card's push kept the season's `v` beside its `m`. const readView = (): T => { const st = window.history.state; const raw = url ? new URLSearchParams(window.location.search).get(url.key) : null; const decoded = url && raw != null ? url.decode(raw) : null; - const owned = st?._subOf === undefined ? decoded != null : st._subOf === subOwner(url); + const owned = + st?._subOf === undefined + ? decoded != null && st?._sub != null && url?.encode(st._sub as T) === raw + : st._subOf === subOwner(url); const fromState = owned ? (st?._sub as T | undefined) : undefined; if (fromState != null) return fromState; if (decoded != null) return decoded; diff --git a/frontend/src/lib/useUrlMirror.reloadedSettle.dom.test.tsx b/frontend/src/lib/useUrlMirror.reloadedSettle.dom.test.tsx new file mode 100644 index 00000000..8dc04b96 --- /dev/null +++ b/frontend/src/lib/useUrlMirror.reloadedSettle.dom.test.tsx @@ -0,0 +1,37 @@ +// @vitest-environment jsdom +// +// A reload under an overlay that reopens (the player) loads on the overlay's entry, its mark on it. +// A page that settles before the overlay is back (the rail steps off a playlist deleted since, before +// the player's row has loaded) has not been sent to another view: nothing the user did moved it. The +// mark stays, and the reopened overlay takes its entry over rather than pushing one more — one more +// was a Back that went nowhere. Its own file: the entry is read as the history module loads, so it is +// set up before the import. +import { describe, expect, it } from "vitest"; +import { act, renderHook } from "@testing-library/react"; + +describe("a page settling on the entry a reload under an overlay started from", () => { + it("before the user has done anything, leaves the entry to the overlay that reopens", async () => { + Object.defineProperty(navigator, "userActivation", { + configurable: true, + value: { hasBeenActive: false }, + }); + window.history.replaceState({ sfPage: "playlists" }, "", "/?page=playlists&pl=7"); + window.history.pushState( + { sfPage: "playlists", _ov: 1, _ovd: 1 }, + "", + "/?page=playlists&pl=7&watch=Z", + ); + const { useUrlMirror } = await import("./useUrlMirror"); + const { useBackToClose } = await import("./history"); + const mirror = renderHook(({ pl }) => useUrlMirror({ pl }), { initialProps: { pl: "7" } }); + mirror.rerender({ pl: "1" }); + expect(new URLSearchParams(window.location.search).get("pl")).toBe("1"); + + const depth = window.history.length; + renderHook(() => useBackToClose(() => {})); + await act(async () => { + await Promise.resolve(); + }); + expect(window.history.length).toBe(depth); + }); +}); diff --git a/frontend/src/lib/useUrlMirror.ts b/frontend/src/lib/useUrlMirror.ts index 751e1fc3..68d3c6da 100644 --- a/frontend/src/lib/useUrlMirror.ts +++ b/frontend/src/lib/useUrlMirror.ts @@ -13,6 +13,12 @@ import { urlWithParams } from "./urlState"; // or a drawer's, a sub-view's) is not in it, and a reload would bring the old view back over the one // on screen. (One landed on by an overlay's step back that another step will leave is written too, // and harmlessly: the next landing writes the entry that stays.) +// Whether a click or a key press has reached the page since it loaded. A browser without the API +// counts as yes: every change is then another view, as before it existed. +const userHasActed = (): boolean => + (navigator as { userActivation?: { hasBeenActive: boolean } }).userActivation?.hasBeenActive ?? + true; + export function useUrlMirror(updates: Record): void { // The caller typically passes an inline object literal (new identity every render); key on its // serialized value so the effect only fires when a param actually changes, not every render. @@ -23,11 +29,13 @@ export function useUrlMirror(updates: Record): void { // A change of what this mirror writes, after its first write, is another view on the same entry // (replaceView: it drops the marks a reload under an overlay left). Its first write on a mount and // its write on a landing name the view the entry already shows, so the marks stay: an overlay that - // reopens after a reload still finds its entry. + // reopens after a reload still finds its entry. So does a change before the user has done anything + // since the load: that is the page settling (the rail stepping off a playlist deleted since), and + // the address under the entry names the same screen, settled again on the way back. const written = useRef(null); const mirror = useEffectEvent(() => { const href = urlWithParams(updates); - const changed = written.current != null && written.current !== dep; + const changed = written.current != null && written.current !== dep && userHasActed(); written.current = dep; // Every mirror runs on every landing: one with nothing to change writes nothing (a browser // throttles a page that rewrites its address too often). diff --git a/frontend/src/moduleUrl.contract.test.ts b/frontend/src/moduleUrl.contract.test.ts index b2d5a931..791eb79d 100644 --- a/frontend/src/moduleUrl.contract.test.ts +++ b/frontend/src/moduleUrl.contract.test.ts @@ -85,22 +85,15 @@ function sources(dir: string): string[] { }); } -// The object literals handed to an address writer: `urlWithParams` / `useUrlMirror`, an overlay's -// own keys (ui/Modal's `url`, `useBackToClose`'s). Each pattern ends at the literal's opening brace; -// the literal runs to its matching close (`writerObjects`), nested arguments and all. -const WRITER_OBJECTS = [ - /(?:urlWithParams|useUrlMirror)\(\{/g, - /\burl=\{\{/g, - // Its first argument a name, a call or an arrow (`() => setOpen(false)`). - /useBackToClose\((?:[^,()]|\([^()]*\))+,\s*\{/g, -]; -// One of each form: a regex that matched nothing in the tree would pass every scan. +// One of each form a writer is handed its value in: a scan that matched nothing in the tree would +// pass every check. const WRITER_FIXTURE = ` useUrlMirror({ fxa: x, fxf }); useBackToClose(onClose, { fxc: "1" }); useBackToClose(() => setOpen(false), { fxd: "1" }); urlWithParams({ ...fxCodec({ ...fxState, job }), fxe: "1" }); + useUrlMirror(shown ? { fxg: x } : null); `; // Where a writer outside lib/ is handed its value: the address writers' argument, and an @@ -134,14 +127,32 @@ function topLevel(text: string, sep: string): string[] { out.push(text.slice(from)); return out.map((s) => s.trim()); } -/** The top-level pieces of every writer's object literal in `text` (`a: x`, `...f(y)`, `[K]: v`, `b`). */ +/** The top-level pieces of every writer's object literal in `text` (`a: x`, `...f(y)`, `[K]: v`, `b`): + * the values `writerValueOk` passes, read the same way, a choice's branches included — every literal + * it lets through is one whose keys are scanned. */ function writerObjects(text: string): string[][] { - return WRITER_OBJECTS.flatMap((re) => - [...text.matchAll(re)].map((m) => { - const open = m.index + m[0].length - 1; - return topLevel(text.slice(open + 1, closing(text, open)), ",").filter(Boolean); - }), - ); + return writerValues(text).flatMap(literalObjects); +} +function literalObjects(value: string): string[][] { + const v = value.trim(); + const choice = topLevel(v, "?"); + if (choice.length === 2) return topLevel(choice[1]!, ":").flatMap(literalObjects); + return v.startsWith("{") ? [topLevel(v.slice(1, closing(v, 0)), ",").filter(Boolean)] : []; +} +/** Where the call of the name ending at `at` opens its arguments, past a type argument list of any + * depth (`>`, `<(v: V) => V>`); null when the name is not called there (an import). */ +function callOpen(text: string, at: number): number | null { + let i = at; + while (/\s/.test(text[i] ?? "")) i++; + if (text[i] === "<") { + let depth = 0; + for (; i < text.length; i++) { + if (text[i] === "<") depth++; + else if (text[i] === ">" && text[i - 1] !== "=" && --depth === 0) break; + } + i++; + } + return text[i] === "(" ? i : null; } /** The key a piece names as a literal: `key: value`, or the shorthand `key`. */ const keyOf = (piece: string) => /^(\w+)\s*:/.exec(piece)?.[1] ?? /^(\w+)$/.exec(piece)?.[1]; @@ -372,8 +383,8 @@ describe("module URL keys", () => { const keys = writerObjects(WRITER_FIXTURE).flatMap((pieces) => pieces.flatMap((piece) => (piece.startsWith("...") ? [] : (keyOf(piece) ?? []))), ); - expect(keys.sort()).toEqual(["fxa", "fxb", "fxc", "fxd", "fxe", "fxf"]); - expect(writerValues(WRITER_FIXTURE)).toHaveLength(5); + expect(keys.sort()).toEqual(["fxa", "fxb", "fxc", "fxd", "fxe", "fxf", "fxg"]); + expect(writerValues(WRITER_FIXTURE)).toHaveLength(6); }); it("are written outside lib/ only as a literal or a lib/ function's result", () => { @@ -404,8 +415,9 @@ describe("module URL keys", () => { // `satisfies`) is one the census above does not count. it("hand each sub-view a definition the census names", () => { const handed = (text: string) => - [...text.matchAll(/\buseHistorySubview(?:<[^>()]*>)?\(/g)].flatMap((m) => { - const open = m.index + m[0].length - 1; + [...text.matchAll(/\buseHistorySubview\b/g)].flatMap((m) => { + const open = callOpen(text, m.index + m[0].length); + if (open == null) return []; const args = topLevel(text.slice(open + 1, closing(text, open)), ","); return args.length > 1 ? [args[1]!] : []; }); @@ -422,6 +434,16 @@ describe("module URL keys", () => { '{ key: "zz", encode, decode }', ]); expect(unnamed("useHistorySubview(sub, PLEX_SUB_URL)")).toEqual([]); + expect(unnamed('useHistorySubview>(sub, { key: "zz" })')).toEqual([ + '{ key: "zz" }', + ]); + expect(unnamed('useHistorySubview<(v: V) => V>(sub, { key: "zz" })')).toEqual([ + '{ key: "zz" }', + ]); + // Under another name it is a call no scan sees. + const aliased = (text: string) => /\buseHistorySubview\s+as\b/.test(text); + expect(files.filter((f) => aliased(readFileSync(f, "utf8")))).toEqual([]); + expect(aliased('import { useHistorySubview as useSub } from "./history";')).toBe(true); }); it("the computed-key scan sees what it is meant to (lib's own writers compute theirs)", () => { diff --git a/frontend/src/modules/playlists/Playlists.dom.test.tsx b/frontend/src/modules/playlists/Playlists.dom.test.tsx index e48d022d..d6950002 100644 --- a/frontend/src/modules/playlists/Playlists.dom.test.tsx +++ b/frontend/src/modules/playlists/Playlists.dom.test.tsx @@ -265,6 +265,26 @@ describe("Playlists — a player the address names", () => { expect(new URLSearchParams(window.location.search).get("watch")).toBe("Z"); }); + it("plays alone over a playlist this account does not hold, the rail moving to the first of several", async () => { + window.history.replaceState(null, "", "/?page=playlists&pl=7&watch=Z"); + h.selectedId = 7; + let answer: (v: Video) => void = () => {}; + vi.spyOn(api, "videoRow").mockReturnValue(new Promise((r) => (answer = r)) as never); + const { rerender } = mount( + { 1: [vid("A", 0)], 2: [vid("B", 0)] }, + {}, + { + detail: async (id) => (id === 7 ? new Promise(() => {}) : list(id, [vid("A", 0)])), + }, + ); + await waitFor(() => expect(qc.getQueryData(qk.playlists())).toBeDefined()); + h.selectedId = 1; // the rail's own move: to the first of the account's list + rerender(); + act(() => answer(vid("Z", 0))); + await waitFor(() => expect(h.player?.video.id).toBe("Z")); + expect(h.player!.queue?.map((v) => v.id)).toEqual(["Z"]); + }); + it("plays alone over a playlist this account does not hold, the rail moved on before the page came", async () => { window.history.replaceState(null, "", "/?page=playlists&pl=1&watch=Z"); // The rail's keep-valid step ran before the page mounted: the selection is already 1, and 1 even diff --git a/frontend/src/modules/playlists/Playlists.tsx b/frontend/src/modules/playlists/Playlists.tsx index aa020e50..3ac7cf0b 100644 --- a/frontend/src/modules/playlists/Playlists.tsx +++ b/frontend/src/modules/playlists/Playlists.tsx @@ -65,6 +65,7 @@ import NavGrid from "../../components/ui/NavGrid"; import { PageToolbar } from "../../components/ui/PageShell"; import { usePlaylistActions } from "./usePlaylistActions"; import { usePlaylists } from "./playlistsContext"; +import { railDefaultId } from "./playlistsInit"; import { usePref } from "../../lib/usePref"; import { navItemAttrs } from "../../lib/listNav"; import { ColumnHead } from "../../components/ui/tableHeader"; @@ -593,7 +594,7 @@ function PlaylistDetail() { const linkedOver = linkedId; const linkedOutOfReach = listQuery.data != null && !listQuery.data.some((p) => p.id === linkedOver); - const railMovedTo = listQuery.data?.[0]?.id ?? null; + const railMovedTo = railDefaultId(listQuery.data); const linkedListIn = linkedOutOfReach || (detail != null && (detail.items.length === 0 || items.length > 0)) || diff --git a/frontend/src/modules/playlists/PlaylistsRail.tsx b/frontend/src/modules/playlists/PlaylistsRail.tsx index c6b21521..e4474984 100644 --- a/frontend/src/modules/playlists/PlaylistsRail.tsx +++ b/frontend/src/modules/playlists/PlaylistsRail.tsx @@ -21,6 +21,7 @@ import { type PanelItem } from "../../components/ui/PanelGroups"; import SortControl from "../../components/ui/SortControl"; import { type SortOption } from "../../lib/columnSort"; import { usePlaylists, usePlaylistsActions } from "./playlistsContext"; +import { railDefaultId } from "./playlistsInit"; import { useMe } from "../../lib/useMe"; import { playlistName } from "../../lib/playlistName"; import { usePlaylistActions } from "./usePlaylistActions"; @@ -105,7 +106,7 @@ export default function PlaylistsRail() { return; } if (selectedId == null || !playlists.some((p) => p.id === selectedId)) { - setSelectedId(playlists[0]!.id); // playlists is non-empty (the length guard above returns) + setSelectedId(railDefaultId(playlists)); } }, [playlists, selectedId, listQuery.data, setSelectedId]); diff --git a/frontend/src/modules/playlists/playlistsInit.ts b/frontend/src/modules/playlists/playlistsInit.ts index dd6ec737..84c7ea6e 100644 --- a/frontend/src/modules/playlists/playlistsInit.ts +++ b/frontend/src/modules/playlists/playlistsInit.ts @@ -12,3 +12,9 @@ export function readSelectedId(): number | null { parsePositiveIntId(getAccountRaw(LS.playlist)) ); } + +/** Where the rail puts the selection when it holds none of the account's playlists: the first one. + * The page reads the same, so a move there is told apart from a pick (a linked player's scope). */ +export function railDefaultId(list: readonly { id: number }[] | undefined): number | null { + return list?.[0]?.id ?? null; +}