From 4869d6c1083725910f7956b5f49396cbc31e3f25 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 11 Sep 2026 01:19:14 +0200 Subject: [PATCH] fix(player): the resume position reaches the playlist page and live search, not just the feed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deep-review R4. 1630a3df wrote the YT player's stored position into the react-query cache so a reopen could not rewind it — but only the feed actually read it back: - Playlists renders from its own undoable copy of the list, re-seeded only when the SET of ids changes, so the patched position never reached it: close a video at 830 s, reopen it from the playlist, and it resumed at the old spot and the next checkpoint wrote that over the 830. A detail with the same membership now rebases its fresh rows into every snapshot, keeping order and undo history. (The ids-only guard is older than this round; the write that depended on it was new.) - Live YouTube search results carry the same per-user position and open the same player, but their cache was never patched. It is now — patched only, never invalidated, since refetching an API-sourced search page spends 100 quota units. Tests for what had none: a Playlists harness (PlayerModal stubbed to record its launch props) and a PlayerModal harness with a fake IFrame API that records where each item is started — the latter pins that stepping back to an item watched earlier in the same session resumes at THIS session's position. 3 mutations, each red on its assertion. --- .../src/components/PlayerModal.dom.test.tsx | 115 ++++++++++++++++ .../src/components/Playlists.dom.test.tsx | 124 ++++++++++++++++++ frontend/src/components/Playlists.tsx | 8 ++ .../playerModalHooks.progress.dom.test.tsx | 13 ++ frontend/src/lib/progressCache.ts | 6 +- 5 files changed, 265 insertions(+), 1 deletion(-) create mode 100644 frontend/src/components/PlayerModal.dom.test.tsx create mode 100644 frontend/src/components/Playlists.dom.test.tsx diff --git a/frontend/src/components/PlayerModal.dom.test.tsx b/frontend/src/components/PlayerModal.dom.test.tsx new file mode 100644 index 00000000..f7aec589 --- /dev/null +++ b/frontend/src/components/PlayerModal.dom.test.tsx @@ -0,0 +1,115 @@ +// @vitest-environment jsdom +// +// The YouTube player modal, rendered for real. Its pure helpers (afterRemoval, the progress cache) +// had tests; what the modal DOES with them had none — a deep-review of the 2026-09 bug hunt found +// that deleting the lines that apply them left every lane green. +// +// `window.YT` is a fake installed before the module's API loader first runs (that loader memoises +// its promise in a module-level singleton, so this file must not share a module instance with a +// test that leaves `window.YT` unset). Each item the modal plays constructs a new YT.Player, and the +// fake records what it was asked for — that is how these tests see where playback starts. +import { describe, it, expect, vi, afterEach, beforeAll } from "vitest"; +import { render, screen, fireEvent, waitFor, cleanup } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { ReactNode } from "react"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ + t: (key: string, o?: Record) => (o?.title ? `${key}:${o.title}` : key), + i18n: { language: "en" }, + }), + }; +}); +vi.mock("./NavigationProvider", () => ({ useNavigationActions: () => ({ openChannel: vi.fn() }) })); +// Their own queries and popovers are not what is under test here. +vi.mock("./AddToPlaylist", () => ({ default: () => null })); +vi.mock("./DownloadButton", () => ({ default: () => null })); + +import PlayerModal from "./PlayerModal"; +import { api, type Video } from "../lib/api"; + +beforeAll(() => { + class Stub { + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + } + globalThis.ResizeObserver ??= Stub as unknown as typeof ResizeObserver; + globalThis.IntersectionObserver ??= Stub as unknown as typeof IntersectionObserver; +}); + +// --- the fake IFrame API ------------------------------------------------------------------------ +const clock = { current: 0, duration: 3000 }; +const started: { videoId: string; start: number | undefined }[] = []; +const base = { + getCurrentTime: () => clock.current, + getDuration: () => clock.duration, + getVideoData: () => null, +}; +// Anything else the modal calls on the player (volume, rate, destroy…) is a no-op. Not `then`: +// a thenable would be swallowed by the first `await` that touched it. +const fakePlayer = new Proxy(base, { + get: (t, k) => (k in t ? t[k as keyof typeof t] : k === "then" ? undefined : () => undefined), +}); +window.YT = { + Player: function (_el: unknown, opts: { videoId: string; playerVars: { start?: number } }) { + started.push({ videoId: opts.videoId, start: opts.playerVars.start }); + return fakePlayer; + } as never, +}; + +const vid = (id: string, position_seconds: number): Video => + ({ + id, + title: `Video ${id}`, + channel_id: "UCx", + channel_title: "Channel", + status: "new", + position_seconds, + duration_seconds: 3000, + watch_url: `https://www.youtube.com/watch?v=${id}`, + }) as unknown as Video; + +function renderModal(ui: ReactNode) { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render({ui}); +} + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); + started.length = 0; +}); + +const lastStart = () => started[started.length - 1]; + +describe("PlayerModal — where each item starts", () => { + it("resumes an item played earlier in the SAME session where this session left it", async () => { + // The launch queue is frozen at mount, so its rows keep the position they were snapshotted + // with (748). Stepping next→prev back to an item already watched to 830 used to start it at + // 748 — and the next checkpoint then wrote 748 over the 830 the server held. + vi.spyOn(api, "videoDetail").mockReturnValue(new Promise(() => {})); + vi.spyOn(api, "saveProgress").mockImplementation(async (id: string) => ({ + video_id: id, + position_seconds: id === "A" ? 830 : 0, + })); + const A = vid("A", 748); + const B = vid("B", 0); + clock.current = 830; + renderModal( + {}} onState={() => {}} />, + ); + await waitFor(() => expect(lastStart()).toEqual({ videoId: "A", start: 748 })); + fireEvent.click(screen.getAllByRole("button", { name: "player.next" })[0]!); + await waitFor(() => expect(lastStart()?.videoId).toBe("B")); + fireEvent.click(screen.getAllByRole("button", { name: "player.previous" })[0]!); + await waitFor(() => expect(lastStart()?.videoId).toBe("A")); + expect(lastStart()?.start).toBe(830); + }); +}); diff --git a/frontend/src/components/Playlists.dom.test.tsx b/frontend/src/components/Playlists.dom.test.tsx new file mode 100644 index 00000000..cf5ab858 --- /dev/null +++ b/frontend/src/components/Playlists.dom.test.tsx @@ -0,0 +1,124 @@ +// @vitest-environment jsdom +// +// The Playlists page as the player sees it: which row it is launched with, and what it is told to +// do when the viewer removes that row from inside it. PlayerModal itself is replaced by a stand-in +// that records its props — its own behaviour is pinned in PlayerModal.dom.test.tsx. +import { describe, it, expect, vi, afterEach, beforeAll, beforeEach } from "vitest"; +import { render, screen, fireEvent, waitFor, cleanup, act } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +const h = vi.hoisted(() => ({ + selectedId: 1 as number | null, + player: null as null | { + video: { id: string; position_seconds: number }; + onClose: () => void; + onRemoveFromList?: (v: unknown) => Promise; + }, +})); + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ + t: (key: string, o?: Record) => (o?.title ? `${key}:${o.title}` : key), + i18n: { language: "en" }, + }), + }; +}); +vi.mock("./PlaylistsProvider", () => ({ usePlaylists: () => ({ selectedId: h.selectedId }) })); +vi.mock("./ConfirmProvider", () => ({ useConfirm: () => async () => true })); +vi.mock("./usePlaylistActions", () => ({ usePlaylistActions: () => ({ remove: vi.fn() }) })); +vi.mock("./PlayerModal", () => ({ + default: (props: NonNullable) => { + h.player = props; + return null; + }, +})); + +import Playlists from "./Playlists"; +import { api, type Video } from "../lib/api"; +import { qk } from "../lib/queryKeys"; +import { patchCachedProgress } from "../lib/progressCache"; + +beforeAll(() => { + class Stub { + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + } + globalThis.ResizeObserver ??= Stub as unknown as typeof ResizeObserver; + globalThis.IntersectionObserver ??= Stub as unknown as typeof IntersectionObserver; +}); + +const vid = (id: string, position_seconds: number): Video => + ({ + id, + title: `Video ${id}`, + channel_id: "UCx", + channel_title: "Channel", + status: "new", + position_seconds, + duration_seconds: 3000, + published_at: "2026-01-01T00:00:00Z", + watch_url: `https://www.youtube.com/watch?v=${id}`, + }) as unknown as Video; + +const list = (id: number, items: Video[]) => ({ + id, + name: `List ${id}`, + kind: "user", + yt_playlist_id: null, + item_count: items.length, + items, +}); + +let qc: QueryClient; + +function mount(lists: Record) { + vi.spyOn(api, "playlists").mockResolvedValue( + Object.entries(lists).map(([id, items]) => list(Number(id), items)) as never, + ); + vi.spyOn(api, "playlist").mockImplementation( + async (id: number) => list(id, lists[id] ?? []) as never, + ); + qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + qc.setQueryData(qk.me(), { id: 1, can_write: true, prefs: {} }); + return render( + + + , + ); +} + +const play = async (title: string) => + fireEvent.click(await screen.findByRole("button", { name: `card.play — ${title}` })); + +beforeEach(() => { + h.selectedId = 1; + h.player = null; +}); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe("Playlists — the row the player is launched with", () => { + it("carries the position the player just stored, not the one the list first loaded with", async () => { + // The page renders from its own undoable copy of the list, re-seeded only when the SET of ids + // changes — so the position the player wrote into the cache never reached it: reopen a video + // closed at 830 s and it resumed at the old spot, and the next checkpoint wrote that over 830. + mount({ 1: [vid("V", 748)] }); + await play("Video V"); + await waitFor(() => expect(h.player?.video.position_seconds).toBe(748)); + act(() => h.player!.onClose()); + h.player = null; + // What the player's cleanup does with the position the server confirmed. + act(() => patchCachedProgress(qc, new Map([["V", 830]]))); + await play("Video V"); + await waitFor(() => expect(h.player?.video.position_seconds).toBe(830)); + }); +}); diff --git a/frontend/src/components/Playlists.tsx b/frontend/src/components/Playlists.tsx index 6bb1d140..197e14ce 100644 --- a/frontend/src/components/Playlists.tsx +++ b/frontend/src/components/Playlists.tsx @@ -266,6 +266,14 @@ export default function Playlists() { if (key !== lastSetRef.current) { lastSetRef.current = key; order.reset(detail.items); + } else { + // Same membership, fresher ROWS — a watch position the player just stored, a status. The + // guard above looks at the id set only, so without this the list kept showing, and handing the + // player, each row as it was when the list first loaded: watch a video to 830 s, close it, + // reopen it, and it resumed at the old spot and the next checkpoint wrote that over the 830. + // `rebase` takes the fresh rows into every snapshot and keeps the order and its undo history. + const fresh = new Map(detail.items.map((v) => [v.id, v])); + order.rebase((list) => list.map((v) => fresh.get(v.id) ?? v)); } // eslint-disable-next-line react-hooks/exhaustive-deps }, [detail]); diff --git a/frontend/src/lib/playerModalHooks.progress.dom.test.tsx b/frontend/src/lib/playerModalHooks.progress.dom.test.tsx index 69745c07..c27fee77 100644 --- a/frontend/src/lib/playerModalHooks.progress.dom.test.tsx +++ b/frontend/src/lib/playerModalHooks.progress.dom.test.tsx @@ -152,6 +152,19 @@ describe("patchCachedProgress", () => { expect(after.items[1]?.position_seconds).toBe(12); }); + it("patches a live YouTube search too — its rows open the same player", () => { + // Left out, reopening a search result resumed from the position the search was fetched with + // (0 for a fresh find) and the next checkpoint wrote that over the stored one. + const qc = new QueryClient(); + const key = qk.ytSearch("melyik szállás", 20); + qc.setQueryData(key, { pages: [{ items: [video()], next_cursor: null }], pageParams: [null] }); + + patchCachedProgress(qc, new Map([[VID, 830]])); + + const after = qc.getQueryData(key) as { pages: { items: Video[] }[] }; + expect(after.pages[0]?.items[0]?.position_seconds).toBe(830); + }); + it("is a no-op when nothing was stored", () => { const qc = new QueryClient(); seedFeed(qc, video()); diff --git a/frontend/src/lib/progressCache.ts b/frontend/src/lib/progressCache.ts index 087c2523..fda24d37 100644 --- a/frontend/src/lib/progressCache.ts +++ b/frontend/src/lib/progressCache.ts @@ -61,7 +61,11 @@ export function patchCachedProgress(qc: QueryClient, positions: Map({ queryKey: qk.feed() }, patch); qc.setQueriesData({ queryKey: qk.playlist() }, patch); + qc.setQueriesData({ queryKey: qk.ytSearch() }, patch); }