From 946c8f26dbedf3c3fe2470858d1555eb121e172c Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 17:30:31 +0200 Subject: [PATCH] test(plex): drive the playlist's reorder and rollback through a hook The Plex playlist's reorder is a dnd-kit drag, and neither of its sensors activates under jsdom, so `commit()`'s failure path could not be reached from a test. The query, the optimistic copy and the two writes (reorder, remove) move into `usePlexPlaylistEdits`, unchanged, and are driven directly: a failed reorder is said and the rows go back; an accepted order lands in the detail cache; a failed removal comes back. It also settles the `putBack` question that could not be driven before: with the refreshed list taken in BEFORE the reorder fails, a rollback bound to the render that started the reorder puts a removed row back, and the deep-equal refetch never clears it. The live cache read the code has now is what keeps it out; the test fails if it goes back to the closure's `q.data`. --- frontend/src/components/PlexPlaylistView.tsx | 89 +------------ .../usePlexPlaylistEdits.dom.test.tsx | 125 ++++++++++++++++++ .../src/components/usePlexPlaylistEdits.ts | 99 ++++++++++++++ 3 files changed, 230 insertions(+), 83 deletions(-) create mode 100644 frontend/src/components/usePlexPlaylistEdits.dom.test.tsx create mode 100644 frontend/src/components/usePlexPlaylistEdits.ts diff --git a/frontend/src/components/PlexPlaylistView.tsx b/frontend/src/components/PlexPlaylistView.tsx index 5937760e..321a7245 100644 --- a/frontend/src/components/PlexPlaylistView.tsx +++ b/frontend/src/components/PlexPlaylistView.tsx @@ -1,7 +1,7 @@ import { useEffect, useMemo, useState } from "react"; import { qk } from "../lib/queryKeys"; import { useTranslation } from "react-i18next"; -import { useQuery, useQueryClient } from "@tanstack/react-query"; +import { useQueryClient } from "@tanstack/react-query"; import { DndContext, KeyboardSensor, @@ -40,6 +40,7 @@ import { Button } from "./ui/Button"; import { LoadingState, StateMessage } from "./ui/QueryState"; import ViewSwitcher, { type ViewOption } from "./ViewSwitcher"; import { FocusSafeButton } from "./ui/FocusSafeButton"; +import { usePlexPlaylistEdits } from "./usePlexPlaylistEdits"; // A playlist's items, grouped for readability: movies stay standalone, while a run of episodes from the // same show collapses into a season/show group so a whole series doesn't sprawl into an endless flat @@ -381,7 +382,6 @@ export default function PlexPlaylistView({ const confirm = useConfirm(); const [renaming, setRenaming] = useState(false); const [name, setName] = useState(""); - const [busy, setBusy] = useState(false); const [layout, setLayout] = useState(() => getAccountRaw(LS.plexPlaylistLayout) === "tree" ? "tree" : "accordion", ); @@ -393,15 +393,10 @@ export default function PlexPlaylistView({ const [expandedShows, setExpandedShows] = useState>(new Set()); const [collapsedSeasons, setCollapsedSeasons] = useState>(new Set()); - const q = useQuery({ - queryKey: qk.plexPlaylist(playlistId), - queryFn: () => api.plexPlaylist(playlistId), - }); - // Local optimistic copy so drag/remove are instant; re-synced whenever the query returns. - const [items, setItems] = useState([]); - useEffect(() => { - if (q.data) setItems(q.data.items); - }, [q.data]); + // The query, the optimistic copy and the two writes that edit it (reorder, remove) live in + // `usePlexPlaylistEdits`, where their failure paths can be driven directly: the drag that reaches + // `commit` cannot be, since dnd-kit's sensors do not activate under jsdom. + const { q, items, busy, invalidate, commit, onRemove } = usePlexPlaylistEdits(playlistId); const blocks = useMemo(() => buildBlocks(items), [items]); const order = useMemo(() => items.map((i) => i.id), [items]); // Total playtime of the whole playlist, mirroring the YouTube playlist header (E9 12D): items with @@ -411,78 +406,6 @@ export default function PlexPlaylistView({ [items], ); - const invalidate = () => { - qc.invalidateQueries({ queryKey: qk.plexPlaylist(playlistId) }); - qc.invalidateQueries({ queryKey: qk.plexPlaylists() }); - }; - - // Every edit here is optimistic on the local copy, and none of them used to handle a failure: a - // removal that failed stayed removed on screen (the resync never ran), and a 404 — the playlist - // deleted on another device, now a structured `not_found` — was an unhandled rejection that said - // nothing at all (deep-review round 4; the YouTube side was fixed in round 3). A failure is said, - // and the rows are put back from what the server last sent — explicitly, because a resync that - // returns the same list keeps the same `q.data` and the re-sync effect above never runs. - // Read the cache AT ROLLBACK TIME, not the `q.data` of the render that bound the failing handler: - // a removal completing while a reorder was in flight refreshed the list, and the reorder's stale - // closure then put the removed row back on screen — where the deep-equal refetch could not clear - // it, and the next drag persisted an order containing it (deep-review round 5). `Playlists.tsx`'s - // sibling `restoreFromCache` reads it live for the same reason. - const putBack = () => { - const cur = qc.getQueryData<{ items: PlexCard[] }>(qk.plexPlaylist(playlistId)); - if (cur) setItems(cur.items); - invalidate(); - }; - - function commit(next: PlexCard[]) { - if (busy) return; // a reorder racing a removal is what makes a rollback ambiguous - setItems(next); - api - .plexReorderPlaylist( - playlistId, - next.map((i) => i.id), - ) - .then(() => { - // Write the accepted order into the DETAIL cache, not just the list. That cache is what - // `putBack` rolls back to and what a re-entry inside the 30 s staleTime renders without - // refetching — so after an accepted reorder it showed the pre-reorder order again, and the - // next drag from there persisted it, silently undoing the one the server took. The - // YouTube sibling (`Playlists.tsx`) got this in round 5; this half was left behind (round - // 6). One accepted write, one cache entry that matches it. - qc.setQueryData<{ items: PlexCard[] }>(qk.plexPlaylist(playlistId), (old) => - old ? { ...old, items: next } : old, - ); - qc.invalidateQueries({ queryKey: qk.plexPlaylist(playlistId) }); - // …and then REFETCH it, exactly as the sibling does. `next` is this render's membership, and - // the server only reorders the keys it was given — so anything that landed meanwhile (a row - // removed from another surface, an item added elsewhere) is clobbered by that write. With - // only the list key invalidated nothing corrected it: staleTime is 30 s and - // refetchOnWindowFocus is off, so the wrong membership stayed on screen for as long as the - // view was mounted and the next drag persisted an order built from it (round 7). - qc.invalidateQueries({ queryKey: qk.plexPlaylists() }); - }) - .catch((e: unknown) => { - notifyActionError(e, t("plex.playlist.reorderFailed")); - putBack(); - }); - } - - async function onRemove(keys: string[]) { - if (busy || !keys.length) return; - setBusy(true); - const set = new Set(keys); - setItems((prev) => prev.filter((i) => !set.has(i.id))); - try { - if (keys.length === 1) await api.plexPlaylistRemoveItem(playlistId, keys[0]!); - else await api.plexPlaylistRemoveBulk(playlistId, keys); - invalidate(); - } catch (e) { - notifyActionError(e, t("plex.playlist.removeFailed")); - putBack(); - } finally { - setBusy(false); - } - } - async function rename() { const title = name.trim(); if (!title) return; diff --git a/frontend/src/components/usePlexPlaylistEdits.dom.test.tsx b/frontend/src/components/usePlexPlaylistEdits.dom.test.tsx new file mode 100644 index 00000000..e28cdace --- /dev/null +++ b/frontend/src/components/usePlexPlaylistEdits.dom.test.tsx @@ -0,0 +1,125 @@ +// @vitest-environment jsdom +// +// The Plex playlist's reorder is a dnd-kit drag, and neither of dnd-kit's sensors activates under +// jsdom — so `commit()`'s failure path, the one edit written as a promise chain (the easiest to lose +// in an async/await refactor), was unreachable from any test (deep-review round 5). The writes live +// in this hook now, and are driven directly. +import { describe, it, expect, vi, afterEach } from "vitest"; +import { renderHook, waitFor, act, cleanup } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { ReactNode } from "react"; + +vi.mock("react-i18next", async (importOriginal) => ({ + ...(await importOriginal()), + useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }), +})); +const { notify } = vi.hoisted(() => ({ + notify: vi.fn((_n: { level: string; message: string }) => 0), +})); +vi.mock("../lib/notifications", async (importOriginal) => ({ + ...(await importOriginal()), + notify, +})); + +import { usePlexPlaylistEdits } from "./usePlexPlaylistEdits"; +import { api, type PlexCard } from "../lib/api"; +import { qk } from "../lib/queryKeys"; + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); + notify.mockClear(); +}); + +const movie = (id: string) => ({ id, type: "movie", title: `Movie ${id}` }) as unknown as PlexCard; +const ids = (items: PlexCard[]) => items.map((i) => i.id); +// The failure is said under the action's own title (`notifyActionError(err, title)`). +const errors = () => + notify.mock.calls + .filter(([n]) => n.level === "error") + .map(([n]) => (n as { title?: string }).title); + +function setup(items: PlexCard[]) { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + const detail = vi + .spyOn(api, "plexPlaylist") + .mockResolvedValue({ id: 5, title: "F", items } as never); + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ); + const hook = renderHook(() => usePlexPlaylistEdits(5), { wrapper }); + return { qc, hook, detail }; +} + +describe("usePlexPlaylistEdits · the reorder", () => { + it("says a failed reorder, and puts the rows back in the order the server holds", async () => { + const { hook } = setup([movie("1"), movie("2"), movie("3")]); + await waitFor(() => expect(ids(hook.result.current.items)).toEqual(["1", "2", "3"])); + vi.spyOn(api, "plexReorderPlaylist").mockRejectedValue(new Error("offline")); + + act(() => hook.result.current.commit([movie("3"), movie("1"), movie("2")])); + expect(ids(hook.result.current.items)).toEqual(["3", "1", "2"]); // optimistic + await waitFor(() => expect(errors()).toEqual(["plex.playlist.reorderFailed"])); + expect(ids(hook.result.current.items)).toEqual(["1", "2", "3"]); + }); + + it("writes an accepted order into the detail cache, which a rollback and a re-entry read", async () => { + const { hook, qc, detail } = setup([movie("1"), movie("2")]); + await waitFor(() => expect(hook.result.current.items).toHaveLength(2)); + // The follow-up refetch never answers, so only the accepted write can put the order there. + detail.mockReturnValue(new Promise(() => {}) as never); + const reorder = vi.spyOn(api, "plexReorderPlaylist").mockResolvedValue(undefined as never); + + act(() => hook.result.current.commit([movie("2"), movie("1")])); + await waitFor(() => expect(reorder).toHaveBeenCalledWith(5, ["2", "1"])); + await waitFor(() => + expect(ids(qc.getQueryData<{ items: PlexCard[] }>(qk.plexPlaylist(5))!.items)).toEqual([ + "2", + "1", + ]), + ); + }); + + // B2, settled here rather than in a browser: `putBack` must read the cache AT ROLLBACK TIME. A + // removal that landed while the reorder was in flight refreshed the list; a rollback bound to the + // render that STARTED the reorder put the removed row back on screen, where no refetch cleared it + // and the next drag persisted an order containing it (deep-review round 5). + it("rolls back to the list as it is NOW, not as it was when the reorder began", async () => { + const { hook, qc, detail } = setup([movie("1"), movie("2"), movie("3")]); + await waitFor(() => expect(hook.result.current.items).toHaveLength(3)); + let fail: (e: unknown) => void = () => {}; + vi.spyOn(api, "plexReorderPlaylist").mockReturnValue( + new Promise((_, reject) => (fail = reject)) as never, + ); + + act(() => hook.result.current.commit([movie("3"), movie("2"), movie("1")])); + // "2" was removed elsewhere meanwhile, and the refreshed list reached the cache. The server + // answers the same from now on — so the rollback's own refetch comes back deep-equal, keeps + // the same `q.data`, and could never correct a stale rollback. + detail.mockResolvedValue({ id: 5, title: "F", items: [movie("1"), movie("3")] } as never); + act(() => + qc.setQueryData(qk.plexPlaylist(5), { id: 5, title: "F", items: [movie("1"), movie("3")] }), + ); + // The view has taken the refreshed list in BEFORE the reorder fails — the race as it happens. + // (react-query batches its notification, so without this wait the refresh landed only after the + // rollback and quietly corrected a stale one, which proved nothing.) + await waitFor(() => expect(ids(hook.result.current.items)).toEqual(["1", "3"])); + await act(async () => fail(new Error("offline"))); + + await waitFor(() => expect(errors()).toEqual(["plex.playlist.reorderFailed"])); + expect(ids(hook.result.current.items)).toEqual(["1", "3"]); + }); +}); + +describe("usePlexPlaylistEdits · the removal", () => { + it("puts a row back when its removal fails, and says why", async () => { + const { hook } = setup([movie("1"), movie("2")]); + await waitFor(() => expect(hook.result.current.items).toHaveLength(2)); + vi.spyOn(api, "plexPlaylistRemoveItem").mockRejectedValue(new Error("offline")); + + await act(() => hook.result.current.onRemove(["2"])); + expect(errors()).toEqual(["plex.playlist.removeFailed"]); + expect(ids(hook.result.current.items)).toEqual(["1", "2"]); + expect(hook.result.current.busy).toBe(false); + }); +}); diff --git a/frontend/src/components/usePlexPlaylistEdits.ts b/frontend/src/components/usePlexPlaylistEdits.ts new file mode 100644 index 00000000..fe270cd4 --- /dev/null +++ b/frontend/src/components/usePlexPlaylistEdits.ts @@ -0,0 +1,99 @@ +import { useEffect, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { useQuery, useQueryClient } from "@tanstack/react-query"; + +import { api, type PlexCard } from "../lib/api"; +import { notifyActionError } from "../lib/actionErrors"; +import { qk } from "../lib/queryKeys"; + +/** A Plex playlist's items and the two writes that edit them in place — a reorder and a removal. + * Split from `PlexPlaylistView` so the failure paths are reachable from a test: the reorder is + * driven by a dnd-kit drag, and neither of its sensors activates under jsdom. */ +export function usePlexPlaylistEdits(playlistId: number) { + const { t } = useTranslation(); + const qc = useQueryClient(); + const [busy, setBusy] = useState(false); + const q = useQuery({ + queryKey: qk.plexPlaylist(playlistId), + queryFn: () => api.plexPlaylist(playlistId), + }); + // Local optimistic copy so drag/remove are instant; re-synced whenever the query returns. + const [items, setItems] = useState([]); + useEffect(() => { + if (q.data) setItems(q.data.items); + }, [q.data]); + + const invalidate = () => { + qc.invalidateQueries({ queryKey: qk.plexPlaylist(playlistId) }); + qc.invalidateQueries({ queryKey: qk.plexPlaylists() }); + }; + + // Every edit here is optimistic on the local copy, and none of them used to handle a failure: a + // removal that failed stayed removed on screen (the resync never ran), and a 404 — the playlist + // deleted on another device, now a structured `not_found` — was an unhandled rejection that said + // nothing at all (deep-review round 4; the YouTube side was fixed in round 3). A failure is said, + // and the rows are put back from what the server last sent — explicitly, because a resync that + // returns the same list keeps the same `q.data` and the re-sync effect above never runs. + // Read the cache AT ROLLBACK TIME, not the `q.data` of the render that bound the failing handler: + // a removal completing while a reorder was in flight refreshed the list, and the reorder's stale + // closure then put the removed row back on screen — where the deep-equal refetch could not clear + // it, and the next drag persisted an order containing it (deep-review round 5). `Playlists.tsx`'s + // sibling `restoreFromCache` reads it live for the same reason. + const putBack = () => { + const cur = qc.getQueryData<{ items: PlexCard[] }>(qk.plexPlaylist(playlistId)); + if (cur) setItems(cur.items); + invalidate(); + }; + + function commit(next: PlexCard[]) { + if (busy) return; // a reorder racing a removal is what makes a rollback ambiguous + setItems(next); + api + .plexReorderPlaylist( + playlistId, + next.map((i) => i.id), + ) + .then(() => { + // Write the accepted order into the DETAIL cache, not just the list. That cache is what + // `putBack` rolls back to and what a re-entry inside the 30 s staleTime renders without + // refetching — so after an accepted reorder it showed the pre-reorder order again, and the + // next drag from there persisted it, silently undoing the one the server took. The + // YouTube sibling (`Playlists.tsx`) got this in round 5; this half was left behind (round + // 6). One accepted write, one cache entry that matches it. + qc.setQueryData<{ items: PlexCard[] }>(qk.plexPlaylist(playlistId), (old) => + old ? { ...old, items: next } : old, + ); + qc.invalidateQueries({ queryKey: qk.plexPlaylist(playlistId) }); + // …and then REFETCH it, exactly as the sibling does. `next` is this render's membership, and + // the server only reorders the keys it was given — so anything that landed meanwhile (a row + // removed from another surface, an item added elsewhere) is clobbered by that write. With + // only the list key invalidated nothing corrected it: staleTime is 30 s and + // refetchOnWindowFocus is off, so the wrong membership stayed on screen for as long as the + // view was mounted and the next drag persisted an order built from it (round 7). + qc.invalidateQueries({ queryKey: qk.plexPlaylists() }); + }) + .catch((e: unknown) => { + notifyActionError(e, t("plex.playlist.reorderFailed")); + putBack(); + }); + } + + async function onRemove(keys: string[]) { + if (busy || !keys.length) return; + setBusy(true); + const set = new Set(keys); + setItems((prev) => prev.filter((i) => !set.has(i.id))); + try { + if (keys.length === 1) await api.plexPlaylistRemoveItem(playlistId, keys[0]!); + else await api.plexPlaylistRemoveBulk(playlistId, keys); + invalidate(); + } catch (e) { + notifyActionError(e, t("plex.playlist.removeFailed")); + putBack(); + } finally { + setBusy(false); + } + } + + return { q, items, busy, invalidate, commit, onRemove }; +}