From 2cff4ad954a7381e8839509efb835ee4ff89e319 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 9 Oct 2026 23:46:39 +0200 Subject: [PATCH] fix(feed): an override yields only to a refetch that set out after its write was answered Send order let a refetch read before an unanswered mark committed and drop it. A reset's position 0 is written again after a refetch that set out before the server took the reset. The player-session refetch also waits for the player's closing checkpoint, which the player now tracks (progressFlushes); refetched first, the list put the older position back. --- .../components/Feed.overrides.dom.test.tsx | 44 +++++++++++++- .../Feed.playerSession.dom.test.tsx | 26 +++++++- .../src/components/Feed.reset.dom.test.tsx | 17 ++++++ frontend/src/components/Feed.tsx | 60 +++++++++++++------ .../playerModalHooks.progress.dom.test.tsx | 23 ++++++- frontend/src/lib/playerModalHooks.ts | 23 +++---- frontend/src/lib/progressCache.ts | 13 ++++ 7 files changed, 173 insertions(+), 33 deletions(-) diff --git a/frontend/src/components/Feed.overrides.dom.test.tsx b/frontend/src/components/Feed.overrides.dom.test.tsx index 95535216..c858fe7b 100644 --- a/frontend/src/components/Feed.overrides.dom.test.tsx +++ b/frontend/src/components/Feed.overrides.dom.test.tsx @@ -29,6 +29,7 @@ vi.mock("./VirtualFeed", () => ({
  • {`${v.id}:${v.status}`} +
    • ))} @@ -87,10 +88,11 @@ const video = (id: string) => const page = (ids: string[], next: string | null = null) => ({ items: ids.map(video), next_cursor: next }) as never; -function mount(feed: ReturnType) { +function mount(feed: ReturnType, setState?: () => Promise) { vi.spyOn(api, "feed").mockImplementation(feed as never); vi.spyOn(api, "feedSummary").mockResolvedValue({ count: 1, facets: {} } as never); - vi.spyOn(api, "setState").mockResolvedValue({ ok: true } as never); + if (setState) vi.spyOn(api, "setState").mockImplementation(setState as never); + else vi.spyOn(api, "setState").mockResolvedValue({ ok: true } as never); const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); qc.setQueryData(qk.me(), { id: 1, can_write: true, prefs: {} }); const ui = (filters: FeedFilters) => ( @@ -149,6 +151,44 @@ describe("Feed — optimistic overrides", () => { expect(screen.getByText("v1:watched")).toBeTruthy(); }); + it("keep a mark sent before the refetch set out but not answered by then", async () => { + // The server may read the list before it commits the mark: that answer is older than the mark. + let answer: (v: unknown) => void = () => {}; + let commit: (v: unknown) => void = () => {}; + const feed = vi + .fn() + .mockResolvedValueOnce(page(["v1"])) + .mockImplementationOnce(() => new Promise((r) => (answer = r))); + const { qc } = mount(feed, () => new Promise((r) => (commit = r))); + fireEvent.click(await screen.findByText("watch v1")); + await screen.findByText("v1:watched"); + act(() => void qc.invalidateQueries({ queryKey: qk.feed() })); + await waitFor(() => expect(feed).toHaveBeenCalledTimes(2)); + await settle(); + await act(async () => answer(page(["v1"]))); + await settle(); + expect(screen.getByText("v1:watched")).toBeTruthy(); + await act(async () => commit({ ok: true })); + }); + + it("are taken back by a failed write only while no later write stands", async () => { + // Marked watched, then hidden before the first answer came back: the refused mark must not take + // the hide with it and put the card back in the view. + const writes: { resolve: (v: unknown) => void; reject: (e: unknown) => void }[] = []; + mount( + vi.fn().mockResolvedValue(page(["v1", "v2"])), + () => new Promise((resolve, reject) => writes.push({ resolve, reject })), + ); + fireEvent.click(await screen.findByText("watch v1")); + fireEvent.click(await screen.findByText("hide v1")); + await waitFor(() => expect(screen.queryByText(/^v1:/)).toBeNull()); + await act(async () => writes[0]?.reject(new Error("refused"))); + await settle(); + expect(screen.queryByText(/^v1:/)).toBeNull(); + await act(async () => writes[1]?.reject(new Error("refused"))); + await waitFor(() => expect(screen.getByText("v1:new")).toBeTruthy()); + }); + it("survive an append of the next page", async () => { let answer: (v: unknown) => void = () => {}; const feed = vi diff --git a/frontend/src/components/Feed.playerSession.dom.test.tsx b/frontend/src/components/Feed.playerSession.dom.test.tsx index 6b735527..6dc404f7 100644 --- a/frontend/src/components/Feed.playerSession.dom.test.tsx +++ b/frontend/src/components/Feed.playerSession.dom.test.tsx @@ -40,10 +40,14 @@ vi.mock("./VirtualFeed", () => ({ ), })); // The player's progress hook marks a clip closed near its end watched from its unmount cleanup: -// `watchOnUnmount` stands in for that. -const player = vi.hoisted(() => ({ watchOnUnmount: false })); +// `watchOnUnmount` stands in for that, and `flush` for its closing checkpoint, which it tracks. +const player = vi.hoisted(() => ({ + watchOnUnmount: false, + flush: null as Promise | null, +})); vi.mock("./PlayerModal", async () => { const { useEffect } = await import("react"); + const { trackProgressFlush } = await import("../lib/progressCache"); return { default: function Player({ video, @@ -57,6 +61,7 @@ vi.mock("./PlayerModal", async () => { useEffect( () => () => { if (player.watchOnUnmount) onState(video.id, "watched"); + if (player.flush) trackProgressFlush(player.flush); }, [video.id, onState], ); @@ -91,6 +96,7 @@ afterEach(() => { cleanup(); vi.restoreAllMocks(); player.watchOnUnmount = false; + player.flush = null; }); const FILTERS: FeedFilters = { @@ -203,6 +209,22 @@ describe("Feed — a video marked watched in the player", () => { await waitFor(() => expect(feed.mock.calls.length).toBeGreaterThan(before)); }); + it("waits for the player's closing checkpoint before it refetches", async () => { + // Refetched before that write landed, the list read the older position and, landing after the + // player's patch, put it back: a reopen resumed early. + const { feed } = mount(); + let land: (v: unknown) => void = () => {}; + player.flush = new Promise((r) => (land = r)); + fireEvent.click(await screen.findByText("open v1")); + fireEvent.click(await screen.findByText("mark")); + const before = feed.mock.calls.length; + fireEvent.click(screen.getByText("close")); + await act(() => new Promise((r) => setTimeout(r, 30))); + expect(feed.mock.calls.length).toBe(before); + await act(async () => land(undefined)); + await waitFor(() => expect(feed.mock.calls.length).toBeGreaterThan(before)); + }); + it("does not refetch on a close with nothing changed", async () => { const { feed } = mount(); fireEvent.click(await screen.findByText("open v1")); diff --git a/frontend/src/components/Feed.reset.dom.test.tsx b/frontend/src/components/Feed.reset.dom.test.tsx index fbd4edd9..cf3815f5 100644 --- a/frontend/src/components/Feed.reset.dom.test.tsx +++ b/frontend/src/components/Feed.reset.dom.test.tsx @@ -122,6 +122,23 @@ describe("Feed — resetting a half-watched video", () => { await waitFor(() => expect(screen.getByText("v1@40")).toBeTruthy()); }); + it("keeps the position at 0 through a refetch that set out before the reset", async () => { + // That refetch read the row the reset deletes: its answer put the old position back under a + // card the reset had made new, and a reopen resumed there. + let answer: (v: unknown) => void = () => {}; + const qc = mount(() => new Promise(() => {})); + await screen.findByText("v1@95"); + vi.mocked(api.feed).mockImplementationOnce(() => new Promise((r) => (answer = r)) as never); + act(() => void qc.invalidateQueries({ queryKey: qk.feed() })); + await waitFor(() => expect(api.feed).toHaveBeenCalledTimes(2)); + await act(() => new Promise((r) => setTimeout(r, 20))); + fireEvent.click(screen.getByText("reset v1")); + await screen.findByText("v1@0"); + await act(async () => answer({ items: [halfWatched], next_cursor: null })); + await act(() => new Promise((r) => setTimeout(r, 20))); + expect(screen.getByText("v1@0")).toBeTruthy(); + }); + it("puts the position back when the server refuses the reset", async () => { mount(() => Promise.reject(new Error("down"))); fireEvent.click(await screen.findByText("reset v1")); diff --git a/frontend/src/components/Feed.tsx b/frontend/src/components/Feed.tsx index 318cd191..709768fe 100644 --- a/frontend/src/components/Feed.tsx +++ b/frontend/src/components/Feed.tsx @@ -27,7 +27,7 @@ import i18n from "../i18n"; import { notifyActionError } from "../lib/actionErrors"; import { notify, resolveVideo } from "../lib/notifications"; import { useDebounced } from "../lib/useDebounced"; -import { patchCachedProgress } from "../lib/progressCache"; +import { patchCachedProgress, progressFlushes } from "../lib/progressCache"; import { buildSort, feedSortOptions, @@ -103,13 +103,14 @@ function matchesView(status: VideoStatus, show: string): boolean { } } -// An optimistic value and its place in this feed's write order. A refetch that lands is the server's -// word only on what was written BEFORE it set out: a mark made while it was in flight is newer than -// its answer, and dropping it put a card marked watched back into an Unwatched view. +// An optimistic value and the write that made it. A refetch that lands is the server's word only on +// the writes it ANSWERED before that refetch set out: a mark made while it was in flight is newer +// than its answer, and so is one sent just before it whose answer had not come back (the server may +// read the list first). Dropping either put a card marked watched back into an Unwatched view. type Stamped = { value: T; seq: number }; type Overrides = Record>; -const writtenAfter = (o: Overrides, since: number): Overrides => - Object.fromEntries(Object.entries(o).filter(([, s]) => s.seq > since)); +const unconfirmed = (o: Overrides, answered: ReadonlySet): Overrides => + Object.fromEntries(Object.entries(o).filter(([, s]) => !answered.has(s.seq))); // A failed write takes back only its OWN value: a later write for the same video stands. const withoutWrite = (o: Overrides, id: string, seq: number): Overrides => { if (o[id]?.seq !== seq) return o; @@ -164,9 +165,14 @@ export default function Feed({ : SORT_KEYS; const [overrides, setOverrides] = useState>({}); const [savedOverrides, setSavedOverrides] = useState>({}); - // The write order the stamps above count in, and where it stood when the running refetch set out. + // The write order the stamps above count in, the writes the server has answered, and which of + // them it had answered when the running refetch set out. const writeSeq = useRef(0); - const refetchFrom = useRef(0); + const answered = useRef(new Set()); + const answeredAtRefetch = useRef>(new Set()); + // A reset's position 0 lives in the cached lists (see onResetState), where a refetch that set out + // before the server took the reset puts the old position back: written again on its landing. + const resets = useRef(new Map()); // The open player: which video and where to start (null = resume from saved position). const [activeVideo, setActiveVideo] = useState<{ video: Video; startAt: number | null } | null>( null, @@ -184,6 +190,8 @@ export default function Feed({ }); const openVideo = useCallback((video: Video, startAt: number | null = null) => { playerSession.current.open = true; + // Its player writes the next position: a reset no longer speaks for it. + resets.current.delete(video.id); setActiveVideo({ video, startAt }); }, []); // Stable identity, like Playlists' `closePlayer`: PlayerModal's keydown effect @@ -193,7 +201,9 @@ export default function Feed({ const closePlayer = useCallback(() => setActiveVideo(null), []); // The session ends once the player has UNMOUNTED, not on the close click: its progress hook's // cleanup marks a clip closed near its end watched, a write that lands after the click. React runs - // every unmount cleanup before any effect's setup, so here that write is already in the session. + // every unmount cleanup before any effect's setup, so here that write is already in the session, + // and so is the player's closing checkpoint (`progressFlushes`): refetched before it lands, the + // list read the older position and, landing after the player's patch, put it back. const playerOpen = activeVideo !== null; useEffect(() => { const session = playerSession.current; @@ -201,7 +211,7 @@ export default function Feed({ session.open = false; if (!session.changed) return; session.changed = false; - void Promise.allSettled([...session.pending]).then(() => { + void Promise.allSettled([...session.pending, ...progressFlushes()]).then(() => { void qc.invalidateQueries({ queryKey: qk.feed() }); void qc.invalidateQueries({ queryKey: qk.feedSummary() }); }); @@ -289,20 +299,26 @@ export default function Feed({ // page 1 rows are unchanged, so a just-hidden card would flash back), and not on a local write into // the cache: the player's closing position patch bumped `dataUpdatedAt` too, cleared the overrides, // and put a card marked watched back into an Unwatched view as "new", with nothing to refetch it. - // And only the overrides written before it set out (`Stamped`). + // And only the overrides whose write it had answered when it set out (`Stamped`). const refetching = query.isFetching && !query.isFetchingNextPage; const wasRefetching = useRef(false); useEffect(() => { const started = !wasRefetching.current && refetching; const landed = wasRefetching.current && !refetching && query.status === "success"; wasRefetching.current = refetching; - if (started) refetchFrom.current = writeSeq.current; - if (landed) { - const since = refetchFrom.current; - setOverrides((o) => writtenAfter(o, since)); - setSavedOverrides((o) => writtenAfter(o, since)); + if (started) answeredAtRefetch.current = new Set(answered.current); + if (!landed) return; + const known = answeredAtRefetch.current; + setOverrides((o) => unconfirmed(o, known)); + setSavedOverrides((o) => unconfirmed(o, known)); + for (const seq of known) answered.current.delete(seq); + const zero = new Map(); + for (const [id, seq] of resets.current) { + if (known.has(seq)) resets.current.delete(id); + else zero.set(id, 0); } - }, [refetching, query.status]); + patchCachedProgress(qc, zero); + }, [refetching, query.status, qc]); // One shared query with the sidebar (same key + filters -> React Query dedupes to a single request): // the feed reads `.count`, the sidebar reads `.facets` (PERF-13, replaces feedCount + facets). @@ -346,6 +362,7 @@ export default function Feed({ // already show the correct state (a hidden/watched card drops out immediately), so refetching // every loaded infinite-query page here was pure waste — 10 pages x rapid marks = a refetch // storm (PERF-2). The next filter change / remount refetches fresh from the server. + answered.current.add(seq); qc.invalidateQueries({ queryKey: qk.feed(), refetchType: "none" }); if (status === "hidden") { const v = loadedRef.current.find((x) => x.id === id); @@ -408,13 +425,19 @@ export default function Feed({ // checkpoint wrote over the progress just made. const before = loadedRef.current.find((v) => v.id === id)?.position_seconds; patchCachedProgress(qc, new Map([[id, 0]])); + resets.current.set(id, seq); api .clearState(id) // Stale-mark only (see setState above): the optimistic override already reset the card. - .then(() => qc.invalidateQueries({ queryKey: qk.feed(), refetchType: "none" })) + .then(() => { + answered.current.add(seq); + qc.invalidateQueries({ queryKey: qk.feed(), refetchType: "none" }); + }) .catch((e: unknown) => { notifyActionError(e, i18n.t("errors.writeFailed.videoReset")); setOverrides((o) => withoutWrite(o, id, seq)); + if (resets.current.get(id) !== seq) return; + resets.current.delete(id); if (before !== undefined) patchCachedProgress(qc, new Map([[id, before]])); }); }, @@ -427,6 +450,7 @@ export default function Feed({ setSavedOverrides((o) => ({ ...o, [id]: { value: saved, seq } })); (saved ? api.watchLaterAdd(id) : api.watchLaterRemove(id)) .then(() => { + answered.current.add(seq); qc.invalidateQueries({ queryKey: qk.playlists() }); qc.invalidateQueries({ queryKey: qk.playlist() }); }) diff --git a/frontend/src/lib/playerModalHooks.progress.dom.test.tsx b/frontend/src/lib/playerModalHooks.progress.dom.test.tsx index 30bb2c78..6d45f7ec 100644 --- a/frontend/src/lib/playerModalHooks.progress.dom.test.tsx +++ b/frontend/src/lib/playerModalHooks.progress.dom.test.tsx @@ -14,7 +14,7 @@ import { renderHook } from "@testing-library/react"; import { QueryClient } from "@tanstack/react-query"; import { createRef, type RefObject } from "react"; import { useYtProgress } from "./playerModalHooks"; -import { patchCachedProgress } from "./progressCache"; +import { patchCachedProgress, progressFlushes } from "./progressCache"; import { qk } from "./queryKeys"; import { api, type Video } from "./api"; import type { YTPlayer } from "./youtube"; @@ -113,6 +113,27 @@ describe("useYtProgress — the checkpoint reaches the cache", () => { expect(cachedPos(qc)).toBe(830); }); + it("hands its closing checkpoint out until it has landed and been patched in", async () => { + // A list refetched on the player's close waits for it: read before it landed, the list held the + // older position and, landing after the patch, put it back. + const qc = new QueryClient(); + seedFeed(qc, video()); + atPosition(830, 3118); + let land: (v: unknown) => void = () => {}; + vi.spyOn(api, "saveProgress").mockReturnValue(new Promise((r) => (land = r)) as never); + + const { unmount } = mountPlayer(qc, new Map()); + await settle(); + unmount(); + const flushes = progressFlushes(); + expect(flushes).toHaveLength(1); + land({ video_id: VID, position_seconds: 830 }); + await flushes[0]; + expect(cachedPos(qc)).toBe(830); + await settle(); + expect(progressFlushes()).toHaveLength(0); + }); + it("mirrors what the SERVER stored, not what the player read", async () => { const qc = new QueryClient(); seedFeed(qc, video({ position_seconds: 748 })); diff --git a/frontend/src/lib/playerModalHooks.ts b/frontend/src/lib/playerModalHooks.ts index 3b283d65..e99411a7 100644 --- a/frontend/src/lib/playerModalHooks.ts +++ b/frontend/src/lib/playerModalHooks.ts @@ -19,7 +19,7 @@ import { import { useBackToExit } from "./history"; import { api, type Video } from "./api"; import { qk } from "./queryKeys"; -import { patchCachedProgress } from "./progressCache"; +import { patchCachedProgress, trackProgressFlush } from "./progressCache"; import type { YTNamespace, YTPlayer, YTPlayerEvent } from "./youtube"; import { logYouTubeApiFailed, logYouTubeError, logYouTubeState } from "./mediaLog"; import type { AutoMode, LoopMode } from "./playerModes"; @@ -527,15 +527,18 @@ export function useYtProgress({ window.removeEventListener("pagehide", onHide); // 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). - void 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, sessionPos); - qc.invalidateQueries({ queryKey: qk.feed(), refetchType: "none" }); - qc.invalidateQueries({ queryKey: qk.playlist(), refetchType: "none" }); - }); + // Tracked, so a list refetch on the player's close waits for it (`progressFlushes`). + trackProgressFlush( + 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, sessionPos); + qc.invalidateQueries({ queryKey: qk.feed(), refetchType: "none" }); + qc.invalidateQueries({ queryKey: qk.playlist(), refetchType: "none" }); + }), + ); const p = playerRef.current; if (p && typeof p.destroy === "function") { try { diff --git a/frontend/src/lib/progressCache.ts b/frontend/src/lib/progressCache.ts index fda24d37..abbfe323 100644 --- a/frontend/src/lib/progressCache.ts +++ b/frontend/src/lib/progressCache.ts @@ -25,6 +25,19 @@ const isItemList = (v: unknown): v is ItemList => * 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. */ +// The player's closing flushes still on their way: its last checkpoint and the patch above. A list +// refetched before one lands read the older checkpoint and, landing after the patch, put it back. +const flushes = new Set>(); + +/** Track a closing flush until it settles (`progressFlushes` hands out the ones still running). */ +export function trackProgressFlush(flush: Promise): void { + flushes.add(flush); + const settled = () => void flushes.delete(flush); + flush.then(settled, settled); +} + +export const progressFlushes = (): Promise[] => [...flushes]; + export function patchCachedProgress(qc: QueryClient, positions: Map): void { if (positions.size === 0) return;