diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index b15fd02b..94103800 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -37,6 +37,7 @@ import Toaster from "./components/ui/Toaster"; import KeyboardHelp from "./components/ui/KeyboardHelp"; import AppCommandPalette from "./components/AppCommandPalette"; import { useGlobalKeymap } from "./lib/keymap"; +import { navigateOverOverlays } from "./lib/history"; import { StateMessage } from "./components/ui/QueryState"; import ErrorDialog from "./components/ErrorDialog"; import VersionBanner from "./components/VersionBanner"; @@ -141,7 +142,11 @@ export default function App() { // Home ("mit nézzek?"). Bound only once signed in, because that is where the overlays are mounted // (bottom of this tree) and where the modules exist at all — see useGlobalKeymap on why binding it // above the signed-out branch is worse than not binding it. - useGlobalKeymap(!!meQuery.data, () => setPage("home")); + // Through `navigateOverOverlays`, because a global key can fire while an overlay is open and does + // not know it: the page change unmounts the player (it lives inside the module), whose reconcile + // then reclaimed the entry this push had just made — Shift+W over a playing video closed the + // player and snapped straight back to the feed. + useGlobalKeymap(!!meQuery.data, () => navigateOverOverlays(() => setPage("home"))); // Pin this tab to whatever account it first loaded as. Without this, a tab that never picked // an account (it was riding the session's default) would silently swap identity when ANOTHER diff --git a/frontend/src/components/Home.dom.test.tsx b/frontend/src/components/Home.dom.test.tsx index 2fa249e3..f444637d 100644 --- a/frontend/src/components/Home.dom.test.tsx +++ b/frontend/src/components/Home.dom.test.tsx @@ -11,8 +11,9 @@ // a Carousel (useHScroll → ResizeObserver), stubbed for jsdom. import { StrictMode } from "react"; import { describe, it, expect, afterEach, beforeAll, beforeEach, vi } from "vitest"; -import { render, screen, fireEvent, cleanup, waitFor } from "@testing-library/react"; +import { act, render, screen, fireEvent, cleanup, waitFor } from "@testing-library/react"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { qk } from "../lib/queryKeys"; beforeAll(() => { class Stub { @@ -143,12 +144,20 @@ const payload = (heroId: string, tier = "resume") => ({ }); function mount() { + return mountWithClient().result; +} + +/** Same mount, handing back the QueryClient — the tests that need to drive an invalidation (i.e. a + * background refetch, the only way to reach the has-data-and-errored state) cannot reach it + * otherwise. */ +function mountWithClient() { const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); - return render( + const result = render( , ); + return { qc, result }; } beforeEach(() => { @@ -178,6 +187,31 @@ describe("Home", () => { expect(await screen.findByText("home.hero.resumeLeft:8:00")).toBeTruthy(); }); + it("renders an error, never the empty state, when the payload cannot be loaded", async () => { + // The one thing this surface must not do: report a backend outage as "nothing to suggest yet" + // over a library full of things to suggest. + discover.mockRejectedValue(new Error("boom")); + mount(); + expect(await screen.findByText("home.error")).toBeTruthy(); + expect(screen.queryByText("home.hero.emptyAny")).toBeNull(); + }); + + it("keeps the page on screen when a BACKGROUND refetch fails", async () => { + // react-query flips `status` to error on a failed refetch while keeping the cached payload, and + // every mutation here invalidates `qk.discover()` — so an unguarded `isError` blanked a fully + // populated landing page over one blip. + const { qc } = mountWithClient(); + await screen.findByText("Hero v1"); + discover.mockRejectedValue(new Error("blip")); + await act(async () => { + await qc.invalidateQueries({ queryKey: qk.discover() }); + }); + // The refetch must actually have been attempted, or this asserts nothing at all. + await waitFor(() => expect(discover).toHaveBeenCalledTimes(2)); + expect(screen.queryByText("home.error")).toBeNull(); + expect(screen.getByText("Hero v1")).toBeTruthy(); + }); + it("does not print the hero again as the first resume tile", async () => { mount(); await screen.findByText("home.hero.resumeLeft:8:00"); @@ -186,6 +220,111 @@ describe("Home", () => { expect(screen.getByText("Second thing")).toBeTruthy(); }); + it("does not print a Plex series twice when the hero is the episode it resolves to", async () => { + // The commonest Plex resume case, and the one a single-key match misses: the rail row is the + // SHOW while the hero is the episode that show resolves to, so their `source:id` keys differ. + // Renamed on `plex` too, not only on the unified card: the rail tile is a `PlexPosterCard`, which + // takes its heading from the NATIVE payload — renaming just the outer title made this vacuous. + const s = showCard(); + const show = { ...s, title: "The Series Tile", plex: { ...s.plex, title: "The Series Tile" } }; + discover.mockResolvedValue({ + budget: "any", + hero: { tier: "resume", card: show.next }, + continue_watching: [show, ytCard("v2", "Second thing")], + comfort: [], + }); + mount(); + await screen.findByText("Second thing"); + expect(screen.queryByText("The Series Tile")).toBeNull(); + }); + + it("hides the resume rail rather than contradicting the hero that emptied it", async () => { + // One in-progress item: it becomes the hero, the dedupe filter takes it out of the rail, and the + // rail then announced "nothing half-finished right now" under a hero captioned "where you left + // off". The shelf's own empty state is untouched — it still has something to teach. + discover.mockResolvedValue({ + budget: "any", + hero: { tier: "resume", card: ytCard("v1", "Hero v1") }, + continue_watching: [ytCard("v1", "Hero v1")], + comfort: [], + }); + mount(); + await screen.findByText("Hero v1"); + expect(screen.queryByText("home.continue.empty")).toBeNull(); + expect(screen.getByText("home.comfort.empty")).toBeTruthy(); + }); + + it("holds the hero it last found when a reroll fails", async () => { + // A failed pick has no data of its own (every press is a new key, and `keepPreviousData` is + // pending-only), so the hero used to rewind to the card the page LOADED with — several presses + // back — while `core.req`'s own dialog explained the failure. + // No resume rail: "Hero v1" then appears exactly where the hero is, so its reappearance is + // unambiguous evidence of a rewind rather than a rail tile that stopped being deduplicated. + discover.mockResolvedValue({ + budget: "any", + hero: { tier: "resume", card: ytCard("v1", "Hero v1") }, + continue_watching: [], + comfort: [], + }); + discoverPick.mockResolvedValueOnce({ + budget: "any", + hero: { tier: "surprise", card: ytCard("v9", "Rerolled") }, + }); + mount(); + await screen.findByText("Hero v1"); + fireEvent.click(screen.getByText("home.hero.reroll")); + await screen.findByText("Rerolled"); + + // Rejected on OUR schedule: asserting straight after the call goes out catches the query still + // pending, where `keepPreviousData` is legitimately holding the old hero — which is not the + // state under test and made this pass with or without the fix. + let failPick!: (e: Error) => void; + discoverPick.mockReturnValueOnce( + new Promise((_resolve, reject) => { + failPick = reject; + }), + ); + fireEvent.click(screen.getByText("home.hero.reroll")); + await waitFor(() => expect(discoverPick).toHaveBeenCalledTimes(2)); + await act(async () => { + failPick(new Error("boom")); + await new Promise((r) => setTimeout(r, 0)); + }); + expect(screen.getByText("Rerolled")).toBeTruthy(); + expect(screen.queryByText("Hero v1")).toBeNull(); + }); + + it("a mutation's invalidation reaches the rerolled hero too", async () => { + // The reroll has its own query key so pressing the button does not refetch the rails — but under + // the SAME root, or the prefix every mutation invalidates misses it and a rerolled hero outlives + // the page: play it, finish it, and it was still the answer, with a zeroed progress bar. + discoverPick.mockResolvedValue({ + budget: "any", + hero: { tier: "surprise", card: ytCard("v9", "Rerolled") }, + }); + const { qc } = mountWithClient(); + await screen.findByText("Hero v1"); + fireEvent.click(screen.getByText("home.hero.reroll")); + await screen.findByText("Rerolled"); + expect(discoverPick).toHaveBeenCalledTimes(1); + + await act(async () => { + await qc.invalidateQueries({ queryKey: qk.discover() }); + }); + await waitFor(() => expect(discoverPick).toHaveBeenCalledTimes(2)); + }); + + it("refreshes the payload the bookmark is read from", async () => { + mount(); + await screen.findByText("Second thing"); + expect(discover).toHaveBeenCalledTimes(1); + fireEvent.click(screen.getAllByTitle("card.saveForLater")[0]!); + // `VideoCard` derives the filled/empty star straight from `video.saved` in this payload and + // keeps no optimistic state of its own, so without re-asking, the icon never changed and the + // next click re-sent the same call — the control looked dead. + await waitFor(() => expect(discover).toHaveBeenCalledTimes(2)); + }); + it("asks for the chosen budget and keeps the previous answer on screen meanwhile", async () => { mount(); await screen.findByText("Hero v1"); diff --git a/frontend/src/components/Home.pin.dom.test.tsx b/frontend/src/components/Home.pin.dom.test.tsx index 91bd08ab..0cd3e386 100644 --- a/frontend/src/components/Home.pin.dom.test.tsx +++ b/frontend/src/components/Home.pin.dom.test.tsx @@ -188,6 +188,37 @@ describe("Home — the comfort shelf's context menu", () => { await waitFor(() => expect(reorderComfortPins).toHaveBeenCalledWith([7, 3, 9])); }); + it("moves the tile under the pointer, not only in the request", async () => { + mount(); + fireEvent.contextMenu(await screen.findByText("Comfort A")); + // Hold the refetch that follows the write: it would answer with this mock's ORIGINAL order, + // which is not what is under test. The claim here is that the tile moves under the pointer, + // before any round trip — `keepPreviousData` keeps the patched cache on screen meanwhile. + discover.mockReturnValue(new Promise(() => {})); + fireEvent.click(item("home.shelf.moveRight")); + await waitFor(() => expect(reorderComfortPins).toHaveBeenCalled()); + // The RENDERED order, not just the payload sent: drop the optimistic cache write and the request + // is still perfectly right while the shelf visibly does not move, so the user clicks again. + expect(screen.getAllByText(/^Comfort [ABC]$/).map((e) => e.textContent)).toEqual([ + "Comfort B", + "Comfort A", + "Comfort C", + ]); + }); + + it("re-asks the server when a move fails instead of restoring a snapshot", async () => { + // Writing a captured `before` back was wrong twice: two overlapping moves each reverted to their + // own snapshot and the shelf settled on an order neither side held, and `before` is the WHOLE + // payload, so the rollback also discarded any rail refresh that landed mid-flight. + reorderComfortPins.mockRejectedValue(new Error("nope")); + mount(); + await screen.findByText("Comfort A"); + expect(discover).toHaveBeenCalledTimes(1); + fireEvent.contextMenu(screen.getByText("Comfort A")); + fireEvent.click(item("home.shelf.moveRight")); + await waitFor(() => expect(discover).toHaveBeenCalledTimes(2)); + }); + it("unpins with the pin id the shelf card carries", async () => { mount(); fireEvent.contextMenu(await screen.findByText("Comfort C")); diff --git a/frontend/src/components/Home.tsx b/frontend/src/components/Home.tsx index bd5e1d8c..93eee986 100644 --- a/frontend/src/components/Home.tsx +++ b/frontend/src/components/Home.tsx @@ -371,6 +371,14 @@ export default function Home() { // forbid. Measured in the browser (2026-09-06); the button's own spinner is the loading signal. placeholderData: keepPreviousData, }); + // The last pick that actually RESOLVED. Every press is a new query key, so a failed one has no + // data of its own and `keepPreviousData` (pending-only) cannot cover it — without this the hero + // silently rewound to the card the page loaded with, several presses back, while `core.req`'s own + // dialog was explaining the failure. A reroll that fails leaves the answer on screen alone. + const [lastPick, setLastPick] = useState(null); + useEffect(() => { + if (pickQ.data !== undefined) setLastPick(pickQ.data.hero); + }, [pickQ.data]); const data = q.data; // Which hero is on screen. The middle branch is the load-bearing one: `keepPreviousData` holds the @@ -384,12 +392,23 @@ export default function Home() { ? (data?.hero ?? null) : pickQ.data !== undefined ? pickQ.data.hero - : (data?.hero ?? null); + : (lastPick ?? data?.hero ?? null); // The hero is the top of the resume rail by construction, so leaving it in the rail below prints // the same card twice, a hand's width apart. Matched on source+id rather than on the tier alone: // a rerolled hero can be a surprise pick that happens to be nothing to do with this rail. + // + // Against BOTH of a row's keys, the way `hero.pick`'s own exclusion does. For a Plex series the + // rail row is the SHOW while the hero is the episode that show RESOLVES to, so a single-key match + // never fires and the most common Plex resume case printed the series twice. const heroKey = hero ? discoverCardKey(hero.card) : null; - const resumeRail = (data?.continue_watching ?? []).filter((c) => discoverCardKey(c) !== heroKey); + const resumeRail = (data?.continue_watching ?? []).filter( + (c) => discoverCardKey(c) !== heroKey && discoverCardKey(c.next ?? c) !== heroKey, + ); + // Empty because the hero ATE the rail, not because nothing is half-finished. An empty rail earns + // its empty state by teaching you how to fill it; here there is nothing to teach, and "Nothing + // half-finished right now" under a hero captioned "Where you left off" is the page contradicting + // itself on one screen. + const resumeConsumed = !resumeRail.length && (data?.continue_watching ?? []).length > 0; const onState = useCallback( (id: string, status: VideoStatus) => { @@ -406,6 +425,11 @@ export default function Home() { void (saved ? api.watchLaterAdd(id) : api.watchLaterRemove(id)).then(() => { qc.invalidateQueries({ queryKey: qk.playlists() }); qc.invalidateQueries({ queryKey: qk.playlist() }); + // And the payload the star is READ from. `VideoCard` derives the filled/empty bookmark + // straight from `video.saved` and keeps no optimistic state of its own (the feed instead + // holds a `savedOverrides` map), so without this the icon never changed and a second click + // re-sent the same call — the control looked dead. + qc.invalidateQueries({ queryKey: qk.discover() }); }); }, [qc], @@ -486,13 +510,19 @@ export default function Home() { const ids = next.map((c) => c.pin_id).filter((id): id is number => id != null); void api .reorderComfortPins(ids) - .catch((e: unknown) => { - qc.setQueryData(key, before); - notifyActionError(e, t("home.shelf.moveFailed")); - }) + .catch((e: unknown) => notifyActionError(e, t("home.shelf.moveFailed"))) .finally(() => { // The shelf is also the pin menus' source of truth everywhere else in the app. void qc.invalidateQueries({ queryKey: qk.comfortPins() }); + // And the PREFIX, not this budget's key — which is what actually re-asks the server, and + // is what the comment above this callback promises. Restoring the captured snapshot + // instead was wrong twice over: two overlapping moves each reverted to their own + // snapshot, so the shelf settled on an order neither side ever held; and `before` is the + // whole payload, so a rollback also threw away any rail refresh that had landed while the + // request was in flight. On the success path the prefix matters for a second reason — the + // shelf is budget-independent server-side but cached per budget, so without it the next + // chip click renders the pre-move order and a move from there writes it back. + void qc.invalidateQueries({ queryKey: qk.discover() }); }); }, [budget, qc, t], @@ -523,7 +553,11 @@ export default function Home() { }, [surpriseRequest, reroll]); if (q.isPending) return ; - if (q.isError) + // `&& !data`, the codebase-wide convention (and the one `ui/QueryState` documents): react-query + // flips `status` to error on a FAILED REFETCH while keeping the cached payload, and every mutation + // on this page invalidates `qk.discover()` — so without the guard, marking one card watched over a + // blip replaced a fully populated landing page with an error line. + if (q.isError && !data) return ( void q.refetch()}> {t("home.error")} @@ -581,17 +615,19 @@ export default function Home() { onToggleWatched={onToggleWatched} onMove={moveInShelf} /> - + {!resumeConsumed && ( + + )} {player && ( diff --git a/frontend/src/lib/history.dom.test.ts b/frontend/src/lib/history.dom.test.ts index 38cb7e8b..cced69c0 100644 --- a/frontend/src/lib/history.dom.test.ts +++ b/frontend/src/lib/history.dom.test.ts @@ -11,7 +11,7 @@ // no reconcile-driven history.back() runs and the shared module state stays clean between tests. import { describe, it, expect, vi, afterEach } from "vitest"; import { renderHook, act } from "@testing-library/react"; -import { closeThenNavigate, useBackToClose, useBackToExit } from "./history"; +import { closeThenNavigate, navigateOverOverlays, useBackToClose, useBackToExit } from "./history"; // Let the mount effect's coalesced reconcile microtask settle. const settle = () => @@ -163,3 +163,27 @@ describe("closeThenNavigate", () => { } }); }); + +describe("navigateOverOverlays", () => { + // The global-shortcut half of the same trap `closeThenNavigate` solves. Shift+W does not know an + // overlay is open and cannot close it — it just navigates, and the page change unmounts the player + // (which lives inside the module column). That unmount's reconcile then reclaimed the entry the + // navigation had only just pushed, so the player closed and the page snapped straight back. + it("a navigation made from outside an open overlay survives that overlay's unmount", async () => { + const overlay = renderHook(() => useBackToClose(() => {})); + await settle(); + const back = vi.spyOn(window.history, "back"); + try { + const navigate = vi.fn(() => window.history.pushState({ sfPage: "home" }, "")); + act(() => navigateOverOverlays(navigate)); + expect(navigate).toHaveBeenCalledTimes(1); + + overlay.unmount(); // what the page change does to the player + await settle(); + // The mark is consumed instead: no back(), so the entry `navigate` pushed is still current. + expect(back).not.toHaveBeenCalled(); + } finally { + back.mockRestore(); + } + }); +}); diff --git a/frontend/src/lib/history.ts b/frontend/src/lib/history.ts index 28d9f4f5..692b6e2c 100644 --- a/frontend/src/lib/history.ts +++ b/frontend/src/lib/history.ts @@ -254,6 +254,26 @@ export function closeThenNavigate(close: () => void, navigate: () => void): void close(); } +/** Navigate from OUTSIDE any overlay — a global keyboard shortcut — when one may be open. + * + * `closeThenNavigate` is for a control that lives INSIDE the overlay and can close it, and it waits + * for that overlay's own entry to come back. A global key has neither: it does not know which + * overlay is open, or whether one is at all. It just navigates, and the page change unmounts + * whatever the module column was holding — whose reconcile then reclaims the entry the navigation + * had just pushed. Shift+W over an open player closed the player and snapped back to the page it + * started on, which reads as a dead shortcut. + * + * Same fix as `closeThenNavigate`, one step shorter: bury the live overlay entries FIRST, so a + * later unmount consumes the marks instead of issuing `history.back()`. There is no popstate to + * wait for, because nothing is being closed on purpose. An overlay the navigation does NOT unmount + * (the palette and the cheat sheet live above the page column) keeps its entry buried too — the + * same trade `closeThenNavigate` already makes, and the reason closing the palette afterwards no + * longer pops the page out from under the user. */ +export function navigateOverOverlays(navigate: () => void): void { + buriedEntries = historyDepth; + navigate(); +} + /** While the calling component is mounted, Back (or the browser/mouse back button) closes it via * `onClose` instead of navigating away. Mount the component only while the overlay is open. */ export function useBackToClose(onClose: () => void): void { diff --git a/frontend/src/lib/queryKeys.ts b/frontend/src/lib/queryKeys.ts index 5dad4dd8..6bb7bbae 100644 --- a/frontend/src/lib/queryKeys.ts +++ b/frontend/src/lib/queryKeys.ts @@ -102,9 +102,12 @@ export const qk = { // anyway. Single-id plex keys stay precisely typed. // The Home surface (E24). `discover()` with no args is the prefix every mutation invalidates — // pinning something changes the shelf AND can change the hero, so the two are never invalidated - // separately. The reroll has its own key so pressing "Mást kérek" does not refetch the rails. + // separately. The reroll has its own key so pressing "Mást kérek" does not refetch the rails — + // but it sits UNDER the same root, because a sibling root ("discover-pick") is not matched by + // that prefix and the rerolled hero silently outlived every invalidation on the page: play it, + // finish it, and it was still the answer on screen with a zeroed progress bar. discover: (...rest: unknown[]) => ["discover", ...rest], - discoverPick: (...rest: unknown[]) => ["discover-pick", ...rest], + discoverPick: (...rest: unknown[]) => ["discover", "pick", ...rest], comfortPins: () => ["comfort-pins"] as const, plex: () => ["plex"] as const, plexWatchLink: () => ["plex-watch-link"] as const,