From 568bf93510c4963e26c7c2b04ff3af64e2d600a2 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 9 Oct 2026 22:01:27 +0200 Subject: [PATCH] fix(feed): an optimistic write outlives only the server answers older than it A reset wrote a position mask over the cached list, so a replay's saved position stayed hidden: a reopen resumed at 0 and its first checkpoint wrote over the progress. The reset now writes 0 into the cache, where the player's closing patch writes the next one. Overrides carry their write order, and a landing refetch drops only those written before it set out. The player session ends after the modal unmounts, so a clip auto-marked watched as it closes still refetches the list. --- .../components/Feed.overrides.dom.test.tsx | 177 ++++++++++++++++++ .../Feed.playerSession.dom.test.tsx | 71 +++++-- .../src/components/Feed.reset.dom.test.tsx | 63 +++++-- frontend/src/components/Feed.tsx | 87 +++++---- 4 files changed, 327 insertions(+), 71 deletions(-) create mode 100644 frontend/src/components/Feed.overrides.dom.test.tsx diff --git a/frontend/src/components/Feed.overrides.dom.test.tsx b/frontend/src/components/Feed.overrides.dom.test.tsx new file mode 100644 index 00000000..95535216 --- /dev/null +++ b/frontend/src/components/Feed.overrides.dom.test.tsx @@ -0,0 +1,177 @@ +// @vitest-environment jsdom +// +// The feed's optimistic layer: a card shows what was just written to it until the server's own answer +// lands. Which answers count is the rule pinned here: a refetch that LANDS (for what was written +// before it set out), a filter change; never an append of the next page. A mark made while a refetch +// was in flight is newer than its answer, and dropping it put a card marked watched back into an +// Unwatched view. The grid is a stub that shows what Feed hands it. +import { describe, it, expect, vi, beforeAll, afterEach } from "vitest"; +import { render, screen, fireEvent, waitFor, cleanup, act } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +vi.mock("react-i18next", async (importOriginal) => ({ + ...(await importOriginal()), + useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }), +})); +vi.mock("./wizardContext", () => ({ useWizard: () => ({ openWizard: vi.fn() }) })); +vi.mock("./VirtualFeed", () => ({ + default: ({ + items, + onState, + fetchNextPage, + }: { + items: { id: string; status: string }[]; + onState: (id: string, status: string) => void; + fetchNextPage: (o: object) => unknown; + }) => ( +
    + {items.map((v) => ( +
  1. + {`${v.id}:${v.status}`} + +
    2. + ))} + +
+ ), +})); + +import Feed from "./Feed"; +import { api, DEFAULT_TAG_MODES, type FeedFilters, type Video } from "../lib/api"; +import { qk } from "../lib/queryKeys"; + +beforeAll(() => { + class Stub { + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + } + globalThis.ResizeObserver ??= Stub as unknown as typeof ResizeObserver; + globalThis.IntersectionObserver ??= Stub as unknown as typeof IntersectionObserver; +}); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +const FILTERS: FeedFilters = { + tags: [], + tagModes: DEFAULT_TAG_MODES, + channelIds: [], + q: "", + sort: "newest", + scope: "my", + includeNormal: true, + includeShorts: false, + includeLive: false, + show: "all", +}; + +const video = (id: string) => + ({ + id, + title: id, + channel_id: "UCx", + channel_title: "Channel", + status: "new", + position_seconds: 0, + duration_seconds: 600, + published_at: "2026-01-01T00:00:00Z", + watch_url: `https://www.youtube.com/watch?v=${id}`, + }) as unknown as Video; + +// The server never takes the mark in these tests: whatever a list shows as "watched" is the override. +const page = (ids: string[], next: string | null = null) => + ({ items: ids.map(video), next_cursor: next }) as never; + +function mount(feed: ReturnType) { + 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); + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + qc.setQueryData(qk.me(), { id: 1, can_write: true, prefs: {} }); + const ui = (filters: FeedFilters) => ( + + {}} + view="cards" + setView={() => {}} + canRead + ytSearch={null} + onYtSearch={() => {}} + onExitYtSearch={() => {}} + /> + + ); + const r = render(ui(FILTERS)); + return { qc, rerender: (f: FeedFilters) => r.rerender(ui(f)) }; +} + +const settle = () => act(() => new Promise((r) => setTimeout(r, 20))); + +describe("Feed — optimistic overrides", () => { + it("give way to a refetch that lands after them", async () => { + let answer: (v: unknown) => void = () => {}; + const feed = vi + .fn() + .mockResolvedValueOnce(page(["v1"])) + .mockImplementationOnce(() => new Promise((r) => (answer = r))); + const { qc } = mount(feed); + 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 waitFor(() => expect(screen.getByText("v1:new")).toBeTruthy()); + }); + + it("keep a mark made while the refetch was in flight", async () => { + let answer: (v: unknown) => void = () => {}; + const feed = vi + .fn() + .mockResolvedValueOnce(page(["v1"])) + .mockImplementationOnce(() => new Promise((r) => (answer = r))); + const { qc } = mount(feed); + await screen.findByText("v1:new"); + act(() => void qc.invalidateQueries({ queryKey: qk.feed() })); + await waitFor(() => expect(feed).toHaveBeenCalledTimes(2)); + await settle(); + fireEvent.click(screen.getByText("watch v1")); + await screen.findByText("v1:watched"); + // The answer the server gave before it heard of the mark. + await act(async () => answer(page(["v1"]))); + await settle(); + expect(screen.getByText("v1:watched")).toBeTruthy(); + }); + + it("survive an append of the next page", async () => { + let answer: (v: unknown) => void = () => {}; + const feed = vi + .fn() + .mockResolvedValueOnce(page(["v1"], "c1")) + .mockImplementationOnce(() => new Promise((r) => (answer = r))); + mount(feed); + fireEvent.click(await screen.findByText("watch v1")); + await screen.findByText("v1:watched"); + fireEvent.click(screen.getByText("more")); + await waitFor(() => expect(feed).toHaveBeenCalledTimes(2)); + await settle(); + await act(async () => answer(page(["v2"]))); + await screen.findByText("v2:new"); + await settle(); + expect(screen.getByText("v1:watched")).toBeTruthy(); + }); + + it("are dropped by a change of the filters", async () => { + const { rerender } = mount(vi.fn().mockResolvedValue(page(["v1"]))); + fireEvent.click(await screen.findByText("watch v1")); + await screen.findByText("v1:watched"); + rerender({ ...FILTERS }); + await waitFor(() => expect(screen.getByText("v1:new")).toBeTruthy()); + }); +}); diff --git a/frontend/src/components/Feed.playerSession.dom.test.tsx b/frontend/src/components/Feed.playerSession.dom.test.tsx index c4e1bae9..6b735527 100644 --- a/frontend/src/components/Feed.playerSession.dom.test.tsx +++ b/frontend/src/components/Feed.playerSession.dom.test.tsx @@ -39,22 +39,36 @@ vi.mock("./VirtualFeed", () => ({ ) ), })); -vi.mock("./PlayerModal", () => ({ - default: ({ - video, - onState, - onClose, - }: { - video: { id: string }; - onState: (id: string, status: string) => void; - onClose: () => void; - }) => ( -
- - -
- ), -})); +// 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 })); +vi.mock("./PlayerModal", async () => { + const { useEffect } = await import("react"); + return { + default: function Player({ + video, + onState, + onClose, + }: { + video: { id: string }; + onState: (id: string, status: string) => void; + onClose: () => void; + }) { + useEffect( + () => () => { + if (player.watchOnUnmount) onState(video.id, "watched"); + }, + [video.id, onState], + ); + return ( +
+ + +
+ ); + }, + }; +}); import Feed from "./Feed"; import { api, DEFAULT_TAG_MODES, type FeedFilters, type Video } from "../lib/api"; @@ -76,6 +90,7 @@ beforeAll(() => { afterEach(() => { cleanup(); vi.restoreAllMocks(); + player.watchOnUnmount = false; }); const FILTERS: FeedFilters = { @@ -164,6 +179,30 @@ describe("Feed — a video marked watched in the player", () => { expect(screen.queryByText("open v1")).toBeNull(); }); + it("refetches when the player marks the clip watched as it closes", async () => { + const { feed } = mount(); + fireEvent.click(await screen.findByText("open v1")); + player.watchOnUnmount = true; + const before = feed.mock.calls.length; + fireEvent.click(await screen.findByText("close")); + await waitFor(() => expect(feed.mock.calls.length).toBeGreaterThan(before)); + }); + + it("waits for the session's writes to land before it refetches", async () => { + const { feed } = mount(); + let land: (v: unknown) => void = () => {}; + vi.mocked(api.setState).mockReturnValue(new Promise((r) => (land = r)) as never); + 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))); + // A refetch now would ask a server that has not taken the mark yet. + expect(feed.mock.calls.length).toBe(before); + await act(async () => land({ ok: true })); + 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 09cc25b3..fbd4edd9 100644 --- a/frontend/src/components/Feed.reset.dom.test.tsx +++ b/frontend/src/components/Feed.reset.dom.test.tsx @@ -5,7 +5,7 @@ // the next refetch (found in a UAT). The grid is replaced by a list that shows what Feed // hands it: each video's position, and a button that fires the reset. import { describe, it, expect, vi, beforeAll, afterEach } from "vitest"; -import { render, screen, fireEvent, waitFor, cleanup } from "@testing-library/react"; +import { render, screen, fireEvent, waitFor, cleanup, act } from "@testing-library/react"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; vi.mock("react-i18next", async (importOriginal) => ({ @@ -35,6 +35,7 @@ vi.mock("./VirtualFeed", () => ({ import Feed from "./Feed"; import { api, DEFAULT_TAG_MODES, type FeedFilters, type Video } from "../lib/api"; import { qk } from "../lib/queryKeys"; +import { patchCachedProgress } from "../lib/progressCache"; beforeAll(() => { class Stub { @@ -78,30 +79,52 @@ const halfWatched = { watch_url: "https://www.youtube.com/watch?v=v1", } as unknown as Video; +function mount(clear: () => Promise) { + vi.spyOn(api, "feed").mockResolvedValue({ items: [halfWatched], next_cursor: null } as never); + vi.spyOn(api, "feedSummary").mockResolvedValue({ count: 1, facets: {} } as never); + vi.spyOn(api, "clearState").mockImplementation(clear as never); + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + qc.setQueryData(qk.me(), { id: 1, can_write: true, prefs: {} }); + render( + + {}} + view="cards" + setView={() => {}} + canRead + ytSearch="" + onYtSearch={() => {}} + onExitYtSearch={() => {}} + /> + , + ); + return qc; +} + describe("Feed — resetting a half-watched video", () => { it("clears its resume position at once, not only its status", async () => { - vi.spyOn(api, "feed").mockResolvedValue({ items: [halfWatched], next_cursor: null } as never); - vi.spyOn(api, "feedSummary").mockResolvedValue({ count: 1, facets: {} } as never); // Never settles: what the card shows before the server answers is the optimistic layer alone. - vi.spyOn(api, "clearState").mockReturnValue(new Promise(() => {}) as never); - const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); - qc.setQueryData(qk.me(), { id: 1, can_write: true, prefs: {} }); - render( - - {}} - view="cards" - setView={() => {}} - canRead - ytSearch="" - onYtSearch={() => {}} - onExitYtSearch={() => {}} - /> - , - ); + mount(() => new Promise(() => {})); await waitFor(() => expect(screen.getByText("v1@95")).toBeTruthy()); fireEvent.click(screen.getByText("reset v1")); await waitFor(() => expect(screen.getByText("v1@0")).toBeTruthy()); }); + + it("shows the position a replay after it saved, so a reopen resumes there", async () => { + // Reset, played to 40 s and closed: the player writes what the server stored into the cache. + // A reset mask over the cache kept showing 0, a reopen resumed at 0, and its first checkpoint + // wrote over the 40. + const qc = mount(() => Promise.resolve({ ok: true })); + fireEvent.click(await screen.findByText("reset v1")); + await screen.findByText("v1@0"); + act(() => patchCachedProgress(qc, new Map([["v1", 40]]))); + await waitFor(() => expect(screen.getByText("v1@40")).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")); + await waitFor(() => expect(screen.getByText("v1@95")).toBeTruthy()); + }); }); diff --git a/frontend/src/components/Feed.tsx b/frontend/src/components/Feed.tsx index 6333df32..318cd191 100644 --- a/frontend/src/components/Feed.tsx +++ b/frontend/src/components/Feed.tsx @@ -27,6 +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 { buildSort, feedSortOptions, @@ -102,6 +103,21 @@ 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. +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)); +// 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; + const next = { ...o }; + delete next[id]; + return next; +}; + // How many live-search results one fetch asks for; `Load more` pulls another batch of the same size. const YT_COUNTS = [20, 40, 60, 100] as const; @@ -146,11 +162,11 @@ export default function Feed({ const sortKeys = channelScoped ? SORT_KEYS.filter((k) => k !== "subscribers" && k !== "priority") : SORT_KEYS; - const [overrides, setOverrides] = useState>({}); - const [savedOverrides, setSavedOverrides] = useState>({}); - // A reset drops the resume position too, which a status override cannot say: without this the card - // kept its progress bar and its Continue button until the next refetch. - const [resetIds, setResetIds] = useState>({}); + 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. + const writeSeq = useRef(0); + const refetchFrom = useRef(0); // 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, @@ -174,9 +190,14 @@ export default function Feed({ // depends on `onClose`, so a fresh closure every Feed render re-subscribed those listeners, // re-ran focusModal() under the open player, dropped its in-flight timers and flushed the // debounced volume write on each pass. - const closePlayer = useCallback(() => { - setActiveVideo(null); + 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. + const playerOpen = activeVideo !== null; + useEffect(() => { const session = playerSession.current; + if (playerOpen || !session.open) return; session.open = false; if (!session.changed) return; session.changed = false; @@ -184,7 +205,7 @@ export default function Feed({ void qc.invalidateQueries({ queryKey: qk.feed() }); void qc.invalidateQueries({ queryKey: qk.feedSummary() }); }); - }, [qc]); + }, [playerOpen, qc]); const ytActive = !!ytSearch; // How many live-search results to gather (the count selector replaces a manual "load more"; @@ -263,21 +284,23 @@ export default function Feed({ setOverridesFor(filters); setOverrides({}); setSavedOverrides({}); - setResetIds({}); } // ...but only when a refetch LANDS, the server's own answer. Not on an infinite-scroll append (its // 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`). 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) { - setOverrides({}); - setSavedOverrides({}); - setResetIds({}); + const since = refetchFrom.current; + setOverrides((o) => writtenAfter(o, since)); + setSavedOverrides((o) => writtenAfter(o, since)); } }, [refetching, query.status]); @@ -302,7 +325,8 @@ export default function Feed({ const onStateRef = useRef<(id: string, status: VideoStatus) => void>(() => {}); const onState = useCallback( (id: string, status: VideoStatus) => { - setOverrides((o) => ({ ...o, [id]: status })); + const seq = ++writeSeq.current; + setOverrides((o) => ({ ...o, [id]: { value: status, seq } })); const write = api.setState(id, status); const session = playerSession.current; if (session.open) { @@ -367,11 +391,7 @@ export default function Feed({ // reverts to the real (unchanged) state instead of staying phantom-hidden, and say so: // a card that silently snaps back reads as a misclick. notifyActionError(e, i18n.t("errors.writeFailed.videoState")); - setOverrides((o) => { - const next = { ...o }; - delete next[id]; - return next; - }); + setOverrides((o) => withoutWrite(o, id, seq)); }); }, [qc], @@ -381,24 +401,21 @@ export default function Feed({ (id: string) => { // Reset to pristine (drop the whole VideoState row, incl. resume position) — the // un-watch toggle can't clear an in-progress position, this can. - setOverrides((o) => ({ ...o, [id]: "new" })); - setResetIds((r) => ({ ...r, [id]: true })); + const seq = ++writeSeq.current; + setOverrides((o) => ({ ...o, [id]: { value: "new", seq } })); + // The position goes into the cached lists, where the player's own closing patch writes the + // next one: as a mask over them it outlived a replay, so a reopen resumed at 0 and its first + // checkpoint wrote over the progress just made. + const before = loadedRef.current.find((v) => v.id === id)?.position_seconds; + patchCachedProgress(qc, new Map([[id, 0]])); api .clearState(id) // Stale-mark only (see setState above): the optimistic override already reset the card. .then(() => qc.invalidateQueries({ queryKey: qk.feed(), refetchType: "none" })) .catch((e: unknown) => { notifyActionError(e, i18n.t("errors.writeFailed.videoReset")); - setOverrides((o) => { - const next = { ...o }; - delete next[id]; - return next; - }); - setResetIds((r) => { - const next = { ...r }; - delete next[id]; - return next; - }); + setOverrides((o) => withoutWrite(o, id, seq)); + if (before !== undefined) patchCachedProgress(qc, new Map([[id, before]])); }); }, [qc], @@ -406,7 +423,8 @@ export default function Feed({ const onToggleSave = useCallback( (id: string, saved: boolean) => { - setSavedOverrides((o) => ({ ...o, [id]: saved })); + const seq = ++writeSeq.current; + setSavedOverrides((o) => ({ ...o, [id]: { value: saved, seq } })); (saved ? api.watchLaterAdd(id) : api.watchLaterRemove(id)) .then(() => { qc.invalidateQueries({ queryKey: qk.playlists() }); @@ -419,7 +437,7 @@ export default function Feed({ saved ? "errors.writeFailed.watchLaterAdd" : "errors.writeFailed.watchLaterRemove", ), ); - setSavedOverrides((o) => ({ ...o, [id]: !saved })); + setSavedOverrides((o) => withoutWrite(o, id, seq)); }); }, [qc], @@ -436,12 +454,11 @@ export default function Feed({ list .map((v) => { const status = overrides[v.id]; // read once so the narrowing sticks - return status ? { ...v, status } : v; + return status ? { ...v, status: status.value } : v; }) - .map((v) => (resetIds[v.id] ? { ...v, position_seconds: 0 } : v)) .map((v) => { const saved = savedOverrides[v.id]; - return saved !== undefined ? { ...v, saved } : v; + return saved ? { ...v, saved: saved.value } : v; }); const merged: Video[] = withOverrides(loaded); const items: Video[] = merged.filter((v) => matchesView(v.status, filters.show));