From 5d7be687d3c2e3e850cd143d99dc5189a1e176db Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 24 Aug 2026 23:41:49 +0200 Subject: [PATCH] test(views,layer): close the two polish-tail deep-review test gaps UX-8 layerStack: add an integration test wired through the real useLayer hook (popover document-listener + stamp) and a PlayerModal-shaped window listener, driven by a real dispatched keydown. The popover close is wrapped in flushSync so its layer is popped synchronously between the two listeners -- the production mid-dispatch race -- making the escapeHandled stamp the only thing that keeps the player from also closing. Mutation-checked: removing the guard fails exactly this case. SavedViews: cover feedCanSeedDefault's "yield to a bookmarked filter URL" branch (a filter-bearing search string -> no seed) and the per-source seededRef guard against the 60s refetch (a changed-list refetch re-runs the seed effect; the guard blocks re-seed). The refetch test waits for the changed list to reach the component so the effect has its chance to wrongly re-fire. Both mutation-checked. Frontend gate green: 464 vitest (+4). Backend untouched. --- .../SavedViewsProvider.dom.test.tsx | 58 +++++++++++- .../lib/layerStack.integration.dom.test.tsx | 93 +++++++++++++++++++ 2 files changed, 148 insertions(+), 3 deletions(-) create mode 100644 frontend/src/lib/layerStack.integration.dom.test.tsx diff --git a/frontend/src/components/SavedViewsProvider.dom.test.tsx b/frontend/src/components/SavedViewsProvider.dom.test.tsx index ad23dae0..6a6a8a9b 100644 --- a/frontend/src/components/SavedViewsProvider.dom.test.tsx +++ b/frontend/src/components/SavedViewsProvider.dom.test.tsx @@ -46,6 +46,7 @@ import { SavedViewsProvider, useSavedViews } from "./SavedViewsProvider"; import { api, setActiveAccount, type SavedView } from "../lib/api"; import { LS, accountKey } from "../lib/storage"; import { ACTIVE_ACCOUNT_KEY } from "../lib/activeAccount"; +import { qk } from "../lib/queryKeys"; const ID = 7; const view = (over: Partial): SavedView => ({ @@ -58,12 +59,19 @@ const view = (over: Partial): SavedView => ({ }); function Probe() { - const { loading } = useSavedViews(); - return
{String(loading)}
; + const { loading, views } = useSavedViews(); + return ( +
+ {String(loading)} +
+ ); } async function mountResolved(views: SavedView[]) { - vi.spyOn(api, "savedViews").mockResolvedValue(views); + // Fresh array + object copies per call, like a real fetch — so a refetch changes viewsQuery.data's + // identity and re-runs the seed effect (which is what makes the per-source guard actually load-bearing + // in the re-seed test; a shared reference would skip the effect and hide a missing guard). + vi.spyOn(api, "savedViews").mockImplementation(async () => views.map((v) => ({ ...v }))); const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); render( @@ -74,11 +82,15 @@ async function mountResolved(views: SavedView[]) { ); // Wait for the query to resolve so the first-resolve effect has run. await waitFor(() => expect(screen.getByTestId("loading").textContent).toBe("false")); + return qc; } beforeEach(() => { localStorage.clear(); sessionStorage.setItem(ACTIVE_ACCOUNT_KEY, String(ID)); + // Reset the URL: feedCanSeedDefault reads window.location.search, and a stale ?sort= from a prior + // case would suppress seeding in the others. + window.history.replaceState({}, "", "/"); h.page = "feed"; h.setFeedFilters.mockClear(); h.setPlexFilters.mockClear(); @@ -123,6 +135,46 @@ describe("SavedViewsProvider — default-view seeding on first resolve", () => { expect(h.setFeedFilters).not.toHaveBeenCalled(); }); + it("feed: does NOT seed when the URL carries filter params (a shared/bookmarked view wins)", async () => { + // A link like ?sort=oldest is an explicit filter the user arrived on; the starred default must not + // stomp it. feedCanSeedDefault() bails on hasFilterParams — plex ignores the URL, so this is feed-only. + window.history.replaceState({}, "", "/?sort=oldest"); + await mountResolved([ + view({ id: 2, is_default: true, filters: { scope: "all", show: "watched" } }), + ]); + expect(h.setFeedFilters).not.toHaveBeenCalled(); + }); + + it("does NOT re-seed on a background refetch that returns a changed list (per-source one-shot guard)", async () => { + // The 60s refetch re-resolves the query; when it returns a CHANGED list (e.g. the user just added a + // view elsewhere) react-query hands down a new reference and the seed effect re-runs. seededRef must + // stop it re-applying the default over filters the user has since edited. (An identical refetch is + // deduped by structural sharing and never reaches the effect, so the changed list is the real test.) + const def = view({ id: 2, is_default: true, filters: { scope: "all", show: "watched" } }); + let calls = 0; + vi.spyOn(api, "savedViews").mockImplementation(async () => { + calls += 1; + return calls === 1 ? [def] : [def, view({ id: 9, name: "added-later" })]; + }); + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + render( + + + + + , + ); + await waitFor(() => expect(screen.getByTestId("loading").textContent).toBe("false")); + expect(h.setFeedFilters).toHaveBeenCalledTimes(1); + await act(async () => { + await qc.refetchQueries({ queryKey: qk.savedViews("feed") }); + }); + // Wait until the changed list has actually reached the component, so the seed effect has had its + // chance to (wrongly) re-fire — otherwise the assertion could win by racing ahead of the re-render. + await waitFor(() => expect(screen.getByTestId("loading").dataset.count).toBe("2")); + expect(h.setFeedFilters).toHaveBeenCalledTimes(1); + }); + it("plex: seeds the starred default into a fresh module (parity with the feed)", async () => { h.page = "plex"; await mountResolved([view({ id: 3, is_default: true, filters: { genres: ["Action"] } })]); diff --git a/frontend/src/lib/layerStack.integration.dom.test.tsx b/frontend/src/lib/layerStack.integration.dom.test.tsx new file mode 100644 index 00000000..a9e4309a --- /dev/null +++ b/frontend/src/lib/layerStack.integration.dom.test.tsx @@ -0,0 +1,93 @@ +// @vitest-environment jsdom +// +// Integration guard for the UX-8 single-Escape fix — the sibling of layerStack.dom.test.ts, but wired +// through the REAL `useLayer` hook and a REAL dispatched keydown instead of hand-rolled listeners. It +// mounts the two production listener shapes as React components: +// - the POPOVER exactly as AnchoredPopover uses it: `useLayer(open, onEscape)` installs the real +// `document` keydown listener that stamps the event and closes on topmost; +// - the PLAYER exactly as PlayerModal reads it: a `window` keydown listener that bails on +// `escapeHandled(e)` first, then on `anyAbove()`. +// The load-bearing subtlety the primitives test can only fake: the popover's close is wrapped in +// `flushSync`, so React tears the popover down (and `useLayer` cleanup pops its layer) SYNCHRONOUSLY +// between the `document` and `window` listeners — the real mid-dispatch flush. That makes the stack read +// "nothing above the player" by the time the player's listener runs, so ONLY the event stamp can stop +// the player from also closing. If the stamp regressed, test 1 would see the player close on the first +// Escape. floating-ui isn't involved here (we drive `useLayer` directly), so no observer stubs needed. +import { describe, it, expect, vi, afterEach } from "vitest"; +import { useEffect, useState } from "react"; +import { flushSync } from "react-dom"; +import { render, act, cleanup } from "@testing-library/react"; +import { useLayer, escapeHandled } from "./layerStack"; + +afterEach(cleanup); + +// The player: registers a layer (no onEscape — it owns a richer handler) and a `window` listener that +// mirrors PlayerModal's guard order (consumed-stamp first, then anyAbove). +function Player({ onClose }: { onClose: () => void }) { + const { anyAbove } = useLayer(true); + useEffect(() => { + const onKey = (e: KeyboardEvent) => { + if (e.key !== "Escape") return; + if (escapeHandled(e)) return; // the UX-8 fix + if (anyAbove()) return; // the old, race-prone guard + onClose(); + }; + window.addEventListener("keydown", onKey); + return () => window.removeEventListener("keydown", onKey); + }, [anyAbove, onClose]); + return
; +} + +// The popover: the real AnchoredPopover usage. flushSync makes the unmount (and its layer removal) run +// synchronously inside the `document` listener — the production mid-dispatch flush the fix must survive. +function Popover({ onClosed }: { onClosed: () => void }) { + const [open, setOpen] = useState(true); + useLayer(open, () => { + flushSync(() => setOpen(false)); + onClosed(); + }); + return open ?
: null; +} + +function dispatchEscape(): KeyboardEvent { + const e = new KeyboardEvent("keydown", { key: "Escape", bubbles: true }); + act(() => { + document.dispatchEvent(e); + }); + return e; +} + +describe("layerStack Escape coordination — live useLayer + real dispatch (UX-8)", () => { + it("one Escape closes only the topmost popover; a second closes the player", () => { + const playerClose = vi.fn(); + const popoverClosed = vi.fn(); + const { queryByTestId } = render( + <> + + + , + ); + // Player registers first, popover second → popover is topmost. + expect(queryByTestId("popover")).not.toBeNull(); + + // First Escape: popover acts (stamps + flushSync-closes, removing its layer synchronously), player + // defers on the stamp even though the stack now shows nothing above it. + const first = dispatchEscape(); + expect(escapeHandled(first)).toBe(true); + expect(popoverClosed).toHaveBeenCalledOnce(); + expect(queryByTestId("popover")).toBeNull(); + expect(playerClose).not.toHaveBeenCalled(); + + // Second Escape: nothing above the player, unstamped → the player closes. + const second = dispatchEscape(); + expect(escapeHandled(second)).toBe(false); + expect(playerClose).toHaveBeenCalledOnce(); + }); + + it("with no layer above, a lone player closes on the first Escape", () => { + const playerClose = vi.fn(); + render(); + dispatchEscape(); + expect(playerClose).toHaveBeenCalledOnce(); + }); +});