fix(player): the resume position reaches the playlist page and live search, not just the feed
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.
This commit is contained in:
@@ -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<typeof import("react-i18next")>();
|
||||
return {
|
||||
...actual,
|
||||
useTranslation: () => ({
|
||||
t: (key: string, o?: Record<string, unknown>) => (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(<QueryClientProvider client={qc}>{ui}</QueryClientProvider>);
|
||||
}
|
||||
|
||||
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(
|
||||
<PlayerModal video={A} queue={[A, B]} startIndex={0} onClose={() => {}} 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);
|
||||
});
|
||||
});
|
||||
@@ -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<boolean>;
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock("react-i18next", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("react-i18next")>();
|
||||
return {
|
||||
...actual,
|
||||
useTranslation: () => ({
|
||||
t: (key: string, o?: Record<string, unknown>) => (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<typeof h.player>) => {
|
||||
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<number, Video[]>) {
|
||||
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(
|
||||
<QueryClientProvider client={qc}>
|
||||
<Playlists />
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
}
|
||||
|
||||
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));
|
||||
});
|
||||
});
|
||||
@@ -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]);
|
||||
|
||||
@@ -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());
|
||||
|
||||
@@ -61,7 +61,11 @@ export function patchCachedProgress(qc: QueryClient, positions: Map<string, numb
|
||||
return old;
|
||||
};
|
||||
|
||||
// Prefix keys: every filter combination of the feed, and every playlist detail.
|
||||
// Prefix keys: every filter combination of the feed, every playlist detail, and every live YouTube
|
||||
// search — its rows come from the same projection, carry the same per-user position, and open the
|
||||
// same player, so leaving it out left the rewind in place on that surface. (Patched, never
|
||||
// invalidated: a refetch of an API-sourced search page spends 100 quota units.)
|
||||
qc.setQueriesData<Cached>({ queryKey: qk.feed() }, patch);
|
||||
qc.setQueriesData<Cached>({ queryKey: qk.playlist() }, patch);
|
||||
qc.setQueriesData<Cached>({ queryKey: qk.ytSearch() }, patch);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user