fix(player): the YT progress checkpoint reaches the cache, so a reopen cannot rewind it
The player checkpointed every 5s and the server stored it, but nothing wrote the new position back into react-query: the close path only marked qk.feed() stale with refetchType "none", and nothing refetches the feed until it remounts (staleTime 30s, refetchOnWindowFocus off). So the resume read the position the cache had been FETCHED with, and the next checkpoint then persisted that older value over the newer one. Measured on localdev: played to 830s, closed (server had 830), reopened the same card without a reload -> the row was back to 751s. 79 seconds of progress destroyed by a reopen, and the card's progress bar never moved either. The Plex player already solved this for its detail query, with a comment describing the identical failure; this is the feed-side twin that was missed in that sweep. patchCachedProgress writes the position the SERVER stored (the endpoint clears trivially-early and near-finished checkpoints to 0, so mirroring our own reading would disagree with the row) into every cached feed page and playlist detail, keeping array identity where nothing changed. A session map covers the second half: the launch queue is frozen at mount, so stepping next->prev back to an item played earlier in the same session resumed from that snapshot. Verified in the browser: 811 -> close 816 -> reopen 825, and the card now shows Continue/Restart.
This commit is contained in:
@@ -129,9 +129,17 @@ export default function PlayerModal({
|
||||
if (index < 0) index = 0;
|
||||
const active = queue?.[index] ?? video; // the queue item at index, else the opened video (never undefined)
|
||||
const hasQueue = !!queue && queue.length > 1;
|
||||
// startAt only applies to the item we opened with; advanced items resume their own pos.
|
||||
// Positions this session has checkpointed (server-confirmed), per video id — see useYtProgress.
|
||||
// The launch queue is frozen at mount, so its items keep the position they were snapshotted with;
|
||||
// without this map, stepping next→prev back to an item played earlier in the SAME session would
|
||||
// resume at that snapshot and then persist it over the newer position.
|
||||
const sessionPosRef = useRef<Map<string, number>>(new Map());
|
||||
const sessionPos = sessionPosRef.current.get(active.id);
|
||||
// startAt only applies to the item we opened with; advanced items resume their own pos. A
|
||||
// position this session already stored wins over both — it is the freshest truth we have.
|
||||
const resumeAt =
|
||||
startAt != null && active.id === video.id ? startAt : active.position_seconds || 0;
|
||||
sessionPos ??
|
||||
(startAt != null && active.id === video.id ? startAt : active.position_seconds || 0);
|
||||
const mountRef = useRef<HTMLDivElement | null>(null);
|
||||
const playerRef = useRef<YTPlayer | null>(null);
|
||||
// Keyboard/wheel controls. We keep focus on the modal card (not the cross-origin player
|
||||
@@ -429,6 +437,7 @@ export default function PlayerModal({
|
||||
reapplyStored,
|
||||
focusModal,
|
||||
advanceOnEnd,
|
||||
sessionPosRef,
|
||||
qc,
|
||||
});
|
||||
|
||||
|
||||
@@ -40,6 +40,12 @@ export interface VideoDetail {
|
||||
duration_seconds: number | null;
|
||||
}
|
||||
|
||||
/** The stored answer of `POST /api/videos/{id}/progress` — the row as it now stands. */
|
||||
export interface StoredProgress {
|
||||
video_id: string;
|
||||
position_seconds: number;
|
||||
}
|
||||
|
||||
export interface FeedResponse {
|
||||
items: Video[];
|
||||
has_more: boolean;
|
||||
@@ -179,8 +185,16 @@ export const feedApi = {
|
||||
),
|
||||
clearState: (id: string) =>
|
||||
req(`/api/videos/${id}/state`, { method: "DELETE" }, { idempotent: true }),
|
||||
saveProgress: (id: string, positionSeconds: number, durationSeconds: number) =>
|
||||
req(
|
||||
// Returns the position the server actually STORED — which is not always what we sent: a
|
||||
// trivially-early or near-finished checkpoint is cleared to 0 (see set_video_progress). Callers
|
||||
// that mirror the position into a cache must write this answer, never their own reading, so the
|
||||
// client can never disagree with the row.
|
||||
saveProgress: (
|
||||
id: string,
|
||||
positionSeconds: number,
|
||||
durationSeconds: number,
|
||||
): Promise<StoredProgress> =>
|
||||
req<StoredProgress>(
|
||||
`/api/videos/${id}/progress`,
|
||||
{
|
||||
method: "POST",
|
||||
|
||||
@@ -0,0 +1,164 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// The YT player's progress checkpoint must reach the CACHE, not just the server. Before this, the
|
||||
// modal's cleanup only marked `qk.feed()` stale with `refetchType: "none"` — and nothing refetches
|
||||
// the feed until it remounts (staleTime 30s, refetchOnWindowFocus off), so reopening the same card
|
||||
// in the same session resumed from the position the cache had been fetched with, and the next
|
||||
// checkpoint then persisted THAT older value over the newer one. Measured on localdev before the
|
||||
// fix: played to 830s → closed (server had 830) → reopened → the row was back to 751s.
|
||||
//
|
||||
// Its own file on purpose: `loadYouTubeApi` memoises the API promise in a module-level singleton,
|
||||
// so a test that installs `window.YT` cannot share a module instance with tests that do not.
|
||||
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||
import { renderHook } from "@testing-library/react";
|
||||
import { QueryClient } from "@tanstack/react-query";
|
||||
import { createRef, type MutableRefObject } from "react";
|
||||
import { useYtProgress } from "./playerModalHooks";
|
||||
import { patchCachedProgress } from "./progressCache";
|
||||
import { qk } from "./queryKeys";
|
||||
import { api, type Video } from "./api";
|
||||
import type { YTPlayer } from "./youtube";
|
||||
|
||||
const VID = "EC5oFYy9z9g";
|
||||
|
||||
const video = (over: Partial<Video> = {}): Video =>
|
||||
({ id: VID, title: "Melyik szállás a Drágább?", position_seconds: 748, ...over }) as Video;
|
||||
|
||||
/** A feed cache shaped like the real infinite query: pages of `{ items }`. */
|
||||
const seedFeed = (qc: QueryClient, v: Video) =>
|
||||
qc.setQueryData(qk.feed({ q: "" } as never), { pages: [{ items: [v] }], pageParams: [null] });
|
||||
|
||||
const cachedPos = (qc: QueryClient): number | undefined => {
|
||||
const data = qc.getQueryData(qk.feed({ q: "" } as never)) as
|
||||
| { pages: { items: Video[] }[] }
|
||||
| undefined;
|
||||
return data?.pages[0]?.items[0]?.position_seconds;
|
||||
};
|
||||
|
||||
// ONE fake namespace with a MUTABLE clock, installed once. `loadYouTubeApi` memoises the API
|
||||
// promise in a module-level singleton, so the namespace the first mount resolves is the one every
|
||||
// later mount gets — re-installing a fresh `window.YT` per test would silently have no effect (an
|
||||
// earlier draft of this file did exactly that, and its "near the end" case was really still
|
||||
// reading the first test's clock).
|
||||
const clock = { current: 0, duration: 0 };
|
||||
const fakePlayer = {
|
||||
getCurrentTime: () => clock.current,
|
||||
getDuration: () => clock.duration,
|
||||
destroy: () => {},
|
||||
} as unknown as YTPlayer;
|
||||
window.YT = {
|
||||
Player: function () {
|
||||
return fakePlayer;
|
||||
} as never,
|
||||
};
|
||||
|
||||
/** Point the shared fake player at a position. */
|
||||
function atPosition(currentTime: number, duration: number) {
|
||||
clock.current = currentTime;
|
||||
clock.duration = duration;
|
||||
}
|
||||
|
||||
/** Let the API promise, the player construction and the checkpoint POST all settle. */
|
||||
const settle = async () => {
|
||||
for (let i = 0; i < 6; i++) await Promise.resolve();
|
||||
await new Promise((r) => setTimeout(r, 0));
|
||||
for (let i = 0; i < 6; i++) await Promise.resolve();
|
||||
};
|
||||
|
||||
function mountPlayer(qc: QueryClient, sessionPos: Map<string, number>) {
|
||||
const mount = document.createElement("div");
|
||||
document.body.appendChild(mount);
|
||||
return renderHook(() =>
|
||||
useYtProgress({
|
||||
activeId: VID,
|
||||
resumeAt: 748,
|
||||
mountRef: { current: mount },
|
||||
playerRef: createRef<YTPlayer>() as MutableRefObject<YTPlayer | null>,
|
||||
currentIdRef: { current: VID },
|
||||
navRef: { current: { queue: undefined, index: 0 } },
|
||||
modeRef: { current: { autoMode: "off", loopMode: "off" } },
|
||||
setWatched: () => {},
|
||||
setPlayerError: () => {},
|
||||
setLiveData: () => {},
|
||||
reapplyStored: () => {},
|
||||
focusModal: () => {},
|
||||
advanceOnEnd: () => {},
|
||||
sessionPosRef: { current: sessionPos },
|
||||
qc,
|
||||
}),
|
||||
);
|
||||
}
|
||||
|
||||
describe("useYtProgress — the checkpoint reaches the cache", () => {
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("writes the position the player reached into the cached feed on close", async () => {
|
||||
const qc = new QueryClient();
|
||||
seedFeed(qc, video());
|
||||
atPosition(830, 3118);
|
||||
const save = vi
|
||||
.spyOn(api, "saveProgress")
|
||||
.mockResolvedValue({ video_id: VID, position_seconds: 830 });
|
||||
|
||||
const { unmount } = mountPlayer(qc, new Map());
|
||||
await settle();
|
||||
unmount();
|
||||
await settle();
|
||||
|
||||
expect(save).toHaveBeenCalled();
|
||||
// Without the patch this stays 748 — the value a reopen would resume from, and then persist
|
||||
// back over the 830 the server already holds.
|
||||
expect(cachedPos(qc)).toBe(830);
|
||||
});
|
||||
|
||||
it("mirrors what the SERVER stored, not what the player read", async () => {
|
||||
const qc = new QueryClient();
|
||||
seedFeed(qc, video({ position_seconds: 748 }));
|
||||
// 3s in — under the endpoint's 5s floor, so it stores 0 and answers 0. (The OTHER clearing
|
||||
// rule, near-the-end, is unreachable from here on purpose: the player auto-marks watched and
|
||||
// deliberately skips that checkpoint, so it never asks the server at all.)
|
||||
atPosition(3, 3118);
|
||||
vi.spyOn(api, "saveProgress").mockResolvedValue({ video_id: VID, position_seconds: 0 });
|
||||
const sessionPos = new Map<string, number>();
|
||||
|
||||
const { unmount } = mountPlayer(qc, sessionPos);
|
||||
await settle();
|
||||
unmount();
|
||||
await settle();
|
||||
|
||||
// Mirroring `getCurrentTime()` would put 3 here — a resume bar and a resume point the server
|
||||
// does not have, which the next checkpoint would then keep re-asserting.
|
||||
expect(sessionPos.get(VID)).toBe(0);
|
||||
expect(cachedPos(qc)).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe("patchCachedProgress", () => {
|
||||
it("patches a playlist detail and leaves untouched lists identical", () => {
|
||||
const qc = new QueryClient();
|
||||
const other = video({ id: "other", position_seconds: 12 });
|
||||
qc.setQueryData(qk.playlist(3), { id: 3, items: [video(), other] });
|
||||
const before = qc.getQueryData(qk.playlist(3)) as { items: Video[] };
|
||||
|
||||
patchCachedProgress(qc, new Map([[VID, 830]]));
|
||||
|
||||
const after = qc.getQueryData(qk.playlist(3)) as { items: Video[] };
|
||||
expect(after.items[0]?.position_seconds).toBe(830);
|
||||
// Same object identity for the row that did not change — so an open player's checkpoint does
|
||||
// not re-render every card in the list behind it.
|
||||
expect(after.items[1]).toBe(before.items[1]);
|
||||
expect(after.items[1]?.position_seconds).toBe(12);
|
||||
});
|
||||
|
||||
it("is a no-op when nothing was stored", () => {
|
||||
const qc = new QueryClient();
|
||||
seedFeed(qc, video());
|
||||
const before = qc.getQueryData(qk.feed({ q: "" } as never));
|
||||
|
||||
patchCachedProgress(qc, new Map());
|
||||
|
||||
expect(qc.getQueryData(qk.feed({ q: "" } as never))).toBe(before);
|
||||
});
|
||||
});
|
||||
@@ -19,6 +19,7 @@ import {
|
||||
import { useBackToExit } from "./history";
|
||||
import { api, type Video } from "./api";
|
||||
import { qk } from "./queryKeys";
|
||||
import { patchCachedProgress } from "./progressCache";
|
||||
import type { YTNamespace, YTPlayer, YTPlayerEvent } from "./youtube";
|
||||
import type { AutoMode, LoopMode } from "../components/playerModalControls";
|
||||
|
||||
@@ -308,6 +309,7 @@ export function useYtProgress({
|
||||
reapplyStored,
|
||||
focusModal,
|
||||
advanceOnEnd,
|
||||
sessionPosRef,
|
||||
qc,
|
||||
}: {
|
||||
activeId: string;
|
||||
@@ -323,6 +325,10 @@ export function useYtProgress({
|
||||
reapplyStored: () => void;
|
||||
focusModal: () => void;
|
||||
advanceOnEnd: () => void;
|
||||
// Positions this player session has actually STORED, per video id. Written from the server's
|
||||
// answer, read back by the modal's resume so stepping next→prev inside one session does not
|
||||
// rewind to the value the frozen launch queue was snapshotted with.
|
||||
sessionPosRef: MutableRefObject<Map<string, number>>;
|
||||
qc: QueryClient;
|
||||
}): void {
|
||||
const autoMarkedRef = useRef(false);
|
||||
@@ -359,7 +365,14 @@ export function useYtProgress({
|
||||
// Only checkpoint the library item we opened with — a navigated-to video may not be in this
|
||||
// user's library, so the server would 404 on it.
|
||||
if (currentIdRef.current === id) {
|
||||
return api.saveProgress(id, cur, dur).catch(() => {});
|
||||
return (
|
||||
api
|
||||
.saveProgress(id, cur, dur)
|
||||
// Record what the server STORED, not `cur`: a near-end or under-5s checkpoint is
|
||||
// cleared to 0 there, and the resume must agree with the row, not with our reading.
|
||||
.then((res) => void sessionPosRef.current.set(id, res.position_seconds))
|
||||
.catch(() => {})
|
||||
);
|
||||
}
|
||||
} catch {
|
||||
/* player may be tearing down */
|
||||
@@ -445,6 +458,11 @@ export function useYtProgress({
|
||||
// Final flush, then mark the feed + playlists stale so resume bars reflect this session on the next
|
||||
// natural refetch — WITHOUT refetching every loaded feed page on each player close (PERF-2).
|
||||
Promise.resolve(persist()).finally(() => {
|
||||
// Write this session's stored positions INTO the cached lists first. Marking them stale is
|
||||
// not enough on its own: nothing refetches the feed until it remounts (staleTime 30s,
|
||||
// refetchOnWindowFocus off), so a reopen used to resume from the position the cache was
|
||||
// fetched with — and the next checkpoint then persisted that older value over this one.
|
||||
patchCachedProgress(qc, sessionPosRef.current);
|
||||
qc.invalidateQueries({ queryKey: qk.feed(), refetchType: "none" });
|
||||
qc.invalidateQueries({ queryKey: qk.playlist(), refetchType: "none" });
|
||||
});
|
||||
|
||||
@@ -0,0 +1,67 @@
|
||||
import type { QueryClient } from "@tanstack/react-query";
|
||||
import { qk } from "./queryKeys";
|
||||
import type { Video } from "./api";
|
||||
|
||||
// Every cached list that carries feed videos is one of two shapes: a single object with `items`
|
||||
// (a playlist detail) or an infinite query's `{ pages: [...] }` over those objects. Narrow
|
||||
// structurally rather than by type name so a new list surface is covered the day it is added.
|
||||
type ItemList = { items: Video[] };
|
||||
type Cached = Partial<ItemList> & { pages?: unknown[] };
|
||||
|
||||
const isItemList = (v: unknown): v is ItemList =>
|
||||
!!v && typeof v === "object" && Array.isArray((v as ItemList).items);
|
||||
|
||||
/**
|
||||
* Write server-stored resume positions into every cached list holding those videos.
|
||||
*
|
||||
* The player checkpoints progress every few seconds, but until this ran, nothing wrote the new
|
||||
* position back into react-query — so the feed cache kept the value it was fetched with, a reopen
|
||||
* of the same card resumed from THAT stale value, and the next checkpoint then persisted the older
|
||||
* position over the newer one (measured: 830s → reopen → 751s). The Plex player already solved the
|
||||
* same problem for its detail query (`plexPlayerHooks`, "reopening the same item in the SAME
|
||||
* session read react-query's CACHED detail"); this is the feed-side twin.
|
||||
*
|
||||
* `positions` must hold what the SERVER stored (`api.saveProgress`'s answer), not what the player
|
||||
* read: the endpoint clears trivially-early and near-finished checkpoints to 0, and mirroring our
|
||||
* own reading would re-introduce that disagreement on the client.
|
||||
*/
|
||||
export function patchCachedProgress(qc: QueryClient, positions: Map<string, number>): void {
|
||||
if (positions.size === 0) return;
|
||||
|
||||
const patchList = (items: Video[]): Video[] => {
|
||||
let changed = false;
|
||||
const next = items.map((v) => {
|
||||
const pos = positions.get(v.id);
|
||||
if (pos === undefined || v.position_seconds === pos) return v;
|
||||
changed = true;
|
||||
return { ...v, position_seconds: pos };
|
||||
});
|
||||
// Keep the previous array identity when nothing moved, so an untouched list does not
|
||||
// re-render every card behind the open player.
|
||||
return changed ? next : items;
|
||||
};
|
||||
|
||||
const patch = (old: Cached | undefined): Cached | undefined => {
|
||||
if (!old) return old;
|
||||
if (Array.isArray(old.pages)) {
|
||||
let changed = false;
|
||||
const pages = old.pages.map((page) => {
|
||||
if (!isItemList(page)) return page;
|
||||
const items = patchList(page.items);
|
||||
if (items === page.items) return page;
|
||||
changed = true;
|
||||
return { ...page, items };
|
||||
});
|
||||
return changed ? { ...old, pages } : old;
|
||||
}
|
||||
if (isItemList(old)) {
|
||||
const items = patchList(old.items);
|
||||
return items === old.items ? old : { ...old, items };
|
||||
}
|
||||
return old;
|
||||
};
|
||||
|
||||
// Prefix keys: every filter combination of the feed, and every playlist detail.
|
||||
qc.setQueriesData<Cached>({ queryKey: qk.feed() }, patch);
|
||||
qc.setQueriesData<Cached>({ queryKey: qk.playlist() }, patch);
|
||||
}
|
||||
Reference in New Issue
Block a user