diff --git a/frontend/src/components/Playlists.dom.test.tsx b/frontend/src/components/Playlists.dom.test.tsx index b92903bc..1c338ef4 100644 --- a/frontend/src/components/Playlists.dom.test.tsx +++ b/frontend/src/components/Playlists.dom.test.tsx @@ -449,9 +449,10 @@ describe("Playlists — removing a row, and undoing it", () => { }); it("puts it back where it stood in the stored order, whatever the view on screen", async () => { - vi.spyOn(api, "removeFromPlaylist").mockResolvedValue(undefined as never); + const lists = { 1: [vid("A", 0), vid("B", 0), vid("C", 0)] }; + serverRemoves(lists); vi.spyOn(api, "addToPlaylist").mockResolvedValue(undefined as never); - mount({ 1: [vid("A", 0), vid("B", 0), vid("C", 0)] }); + mount(lists); await screen.findByRole("button", { name: "card.play — Video C" }); // Sort the VIEW by title, descending: C, B, A on screen — the stored order is still A, B, C. fireEvent.click(screen.getByRole("button", { name: /playlists\.colTitle/ })); @@ -463,10 +464,25 @@ describe("Playlists — removing a row, and undoing it", () => { await waitFor(() => expect(api.addToPlaylist).toHaveBeenCalledWith(1, "A", 0)); }); + // E28 review C4: the stored index drifts when the list changes before the Undo (a row added at + // the top in another tab) — the row goes back after its old neighbour instead. + it("puts it back after its neighbour when the list changed before the Undo", async () => { + const lists = { 1: [vid("A", 0), vid("B", 0), vid("C", 0)] }; + serverRemoves(lists); + vi.spyOn(api, "addToPlaylist").mockResolvedValue(undefined as never); + mount(lists); + await rowMenu("Video B", "playlists.removeItem"); + await waitFor(() => expect(lastToast()?.action).toBeTruthy()); + lists[1] = [vid("X", 0), ...lists[1]!]; + act(() => lastToast().action!.onClick()); + await waitFor(() => expect(api.addToPlaylist).toHaveBeenCalledWith(1, "B", 2)); + }); + it("says so when the Undo fails", async () => { - vi.spyOn(api, "removeFromPlaylist").mockResolvedValue(undefined as never); + const lists = { 1: [vid("A", 0), vid("B", 0)] }; + serverRemoves(lists); vi.spyOn(api, "addToPlaylist").mockRejectedValue(new TypeError("Failed to fetch")); - mount({ 1: [vid("A", 0), vid("B", 0)] }); + mount(lists); await rowMenu("Video A", "playlists.removeItem"); await waitFor(() => expect(lastToast()?.action).toBeTruthy()); act(() => lastToast().action!.onClick()); diff --git a/frontend/src/components/Playlists.tsx b/frontend/src/components/Playlists.tsx index d4433a36..0d6fb62b 100644 --- a/frontend/src/components/Playlists.tsx +++ b/frontend/src/components/Playlists.tsx @@ -24,6 +24,7 @@ import { notify } from "../lib/notifications"; import { notifyActionError } from "../lib/actionErrors"; import { notifyYouTubeActionError } from "../lib/youtubeErrors"; import { playlistName } from "../lib/playlistName"; +import { reinsertAt } from "../lib/reinsert"; import { useUndoable } from "../lib/useUndoable"; import { cycleSort, type SortState } from "../lib/columnSort"; import { formatRuntime } from "../lib/format"; @@ -476,9 +477,10 @@ export default function Playlists() { // so the guard reset the order: the removed row came back and the undo history the rebase had // kept went with it. (A failure resyncs below, which puts the row back where it belongs.) const before = qc.getQueryData(qk.playlist(listId)); - // Where it stood in the STORED order — what the Undo puts it back at (the server's list, so a - // view sort on screen does not decide it). - const at = before?.items.findIndex((v) => v.id === video.id) ?? -1; + // The STORED order it stood in — the Undo puts it back after its neighbour there (the server's + // list, so a view sort on screen does not decide it; `reinsertAt`, so a change in between does + // not shift it off by the rows added or moved meanwhile). + const storedBefore = before?.items.map((v) => v.id) ?? []; qc.setQueryData(qk.playlist(listId), (old) => old ? { ...old, items: old.items.filter((v) => v.id !== video.id) } : old, ); @@ -503,10 +505,23 @@ export default function Playlists() { action: { label: t("common.undo"), // ONE request that puts it back at its index (the server renumbers around it): an append - // plus a whole-list reorder would land it at the end whenever the second call failed. + // plus a whole-list reorder would land it at the end whenever the second call failed. The + // index is worked out against the list as it is NOW, fetched fresh. onClick: () => - void api - .addToPlaylist(listId, video.id, at < 0 ? undefined : at) + void qc + .fetchQuery({ + queryKey: qk.playlist(listId), + queryFn: () => api.playlist(listId), + staleTime: 0, + }) + .then((now) => { + const back = reinsertAt( + storedBefore, + [video.id], + now.items.map((v) => v.id), + ); + if (back.keys.length) return api.addToPlaylist(listId, video.id, back.positions[0]); + }) .catch((e: unknown) => notifyActionError(e, t("playlists.undoRemoveFailed"))) .finally(() => { refreshAll(listId); diff --git a/frontend/src/components/PlexPlaylistView.menu.dom.test.tsx b/frontend/src/components/PlexPlaylistView.menu.dom.test.tsx index 81fe8196..f1d3eda7 100644 --- a/frontend/src/components/PlexPlaylistView.menu.dom.test.tsx +++ b/frontend/src/components/PlexPlaylistView.menu.dom.test.tsx @@ -152,14 +152,22 @@ describe("PlexPlaylistView · a row's menu", () => { it("removes a title with no confirm, and the toast's Undo puts it back at its index", async () => { mount([movie("1"), movie("2"), movie("3")]); await screen.findByText("Movie 2"); - vi.spyOn(api, "plexPlaylistRemoveItem").mockResolvedValue({} as never); + vi.spyOn(api, "plexPlaylistRemoveItem").mockImplementation(async () => { + // The server's side of it: the Undo reads the list fresh, and the row is gone from it. + vi.mocked(api.plexPlaylist).mockResolvedValue({ + id: 5, + title: "Films", + items: [movie("1"), movie("3")], + } as never); + return {} as never; + }); const add = vi.spyOn(api, "plexPlaylistAddBulk").mockResolvedValue({} as never); await pick(() => fireEvent.contextMenu(rowOf("Movie 2")), "plex.playlist.removeFromPlaylist"); await waitFor(() => expect(api.plexPlaylistRemoveItem).toHaveBeenCalledWith(5, "2")); expect(h.confirm).not.toHaveBeenCalled(); await waitFor(() => expect(toast()?.message).toBe("plex.playlist.removedToast:Movie 2")); act(() => toast()!.action!.onClick()); - expect(add).toHaveBeenCalledWith(5, ["2"], [1]); + await waitFor(() => expect(add).toHaveBeenCalledWith(5, ["2"], [1])); }); it("plays and removes a whole show from its group's menu", async () => { @@ -313,3 +321,24 @@ describe("PlexPlaylistView · the header's '…'", () => { ); }); }); + +// E28 review E2: the watched toggle flips the view's LOCAL copy of the rows. On a failure the +// resync refetches an identical list, react-query keeps the same `data`, and the copy was never +// re-seeded — the row stayed "watched" on screen while Plex still had it unwatched. +describe("PlexPlaylistView · a failed watched toggle", () => { + it("puts the row's watched state back", async () => { + mount([movie("1"), movie("2")]); + await screen.findByText("Movie 2"); + vi.spyOn(api, "plexSetState").mockRejectedValue(new Error("offline")); + await pick(() => fireEvent.contextMenu(rowOf("Movie 2")), "plex.markWatched"); + await waitFor(() => + expect(h.notify).toHaveBeenCalledWith( + expect.objectContaining({ level: "error", title: "plex.watchedFailed" }), + ), + ); + await act(async () => {}); + fireEvent.keyDown(document.activeElement ?? document.body, { key: "Escape" }); + await openMenu(() => fireEvent.contextMenu(rowOf("Movie 2"))); + expect(menuRows()).toContain("plex.markWatched"); + }); +}); diff --git a/frontend/src/components/PlexPlaylistView.tsx b/frontend/src/components/PlexPlaylistView.tsx index 36c06df5..d4771e42 100644 --- a/frontend/src/components/PlexPlaylistView.tsx +++ b/frontend/src/components/PlexPlaylistView.tsx @@ -560,6 +560,9 @@ export default function PlexPlaylistView({ onToggleWatched: (c) => void toggleWatchState(c, t, { optimistic: (next) => setStatus(c.id, next), + // The flip is on the view's LOCAL copy: a failed write's resync refetches an identical + // list, react-query keeps the same `data`, and the copy is never re-seeded from it. + revert: () => setStatus(c.id, c.status ?? "new"), resync: invalidate, }), }, diff --git a/frontend/src/components/usePlexPlaylistEdits.dom.test.tsx b/frontend/src/components/usePlexPlaylistEdits.dom.test.tsx index 8be51dad..5aedae94 100644 --- a/frontend/src/components/usePlexPlaylistEdits.dom.test.tsx +++ b/frontend/src/components/usePlexPlaylistEdits.dom.test.tsx @@ -129,9 +129,12 @@ describe("usePlexPlaylistEdits · the removal", () => { // E28 C1: no confirm — the toast's Undo is the way back, and it puts each row where it stood. it("says what went, and its Undo puts the rows back at their own indices in one request", async () => { - const { hook } = setup([movie("1"), movie("2"), movie("3"), movie("4")]); + const { hook, detail } = setup([movie("1"), movie("2"), movie("3"), movie("4")]); await waitFor(() => expect(hook.result.current.items).toHaveLength(4)); - vi.spyOn(api, "plexPlaylistRemoveBulk").mockResolvedValue({} as never); + vi.spyOn(api, "plexPlaylistRemoveBulk").mockImplementation(async () => { + detail.mockResolvedValue({ id: 5, title: "F", items: [movie("1"), movie("3")] } as never); + return {} as never; + }); const add = vi.spyOn(api, "plexPlaylistAddBulk").mockResolvedValue({} as never); // A season whose episodes do not sit together: rows 2 and 4. @@ -140,14 +143,35 @@ describe("usePlexPlaylistEdits · the removal", () => { expect(toast.message).toBe("plex.playlist.removedToast"); expect(toast.action!.label).toBe("common.undo"); act(() => toast.action!.onClick()); - expect(add).toHaveBeenCalledTimes(1); - expect(add).toHaveBeenCalledWith(5, ["4", "2"], [3, 1]); + await waitFor(() => expect(add).toHaveBeenCalledTimes(1)); + expect(add).toHaveBeenCalledWith(5, ["2", "4"], [1, 3]); + }); + + // E28 review C4: a raw index drifts when the list changes before the Undo — another tab added a + // row at the top — and the row came back one place off. It goes back after its old neighbour. + it("puts a row back after its neighbour when the list changed before the Undo", async () => { + const { hook, detail } = setup([movie("1"), movie("2"), movie("3")]); + await waitFor(() => expect(hook.result.current.items).toHaveLength(3)); + vi.spyOn(api, "plexPlaylistRemoveItem").mockResolvedValue({} as never); + const add = vi.spyOn(api, "plexPlaylistAddBulk").mockResolvedValue({} as never); + await act(() => hook.result.current.onRemove(["2"], "Movie 2")); + detail.mockResolvedValue({ + id: 5, + title: "F", + items: [movie("x"), movie("1"), movie("3")], + } as never); + const toast = notify.mock.calls.map(([n]) => n).find((n) => n.action)!; + act(() => toast.action!.onClick()); + await waitFor(() => expect(add).toHaveBeenCalledWith(5, ["2"], [2])); }); it("says so when the Undo fails", async () => { - const { hook } = setup([movie("1"), movie("2")]); + const { hook, detail } = setup([movie("1"), movie("2")]); await waitFor(() => expect(hook.result.current.items).toHaveLength(2)); - vi.spyOn(api, "plexPlaylistRemoveItem").mockResolvedValue({} as never); + vi.spyOn(api, "plexPlaylistRemoveItem").mockImplementation(async () => { + detail.mockResolvedValue({ id: 5, title: "F", items: [movie("2")] } as never); + return {} as never; + }); vi.spyOn(api, "plexPlaylistAddBulk").mockRejectedValue(new Error("offline")); await act(() => hook.result.current.onRemove(["1"], "Movie 1")); const toast = notify.mock.calls.map(([n]) => n).find((n) => n.action)!; diff --git a/frontend/src/components/usePlexPlaylistEdits.ts b/frontend/src/components/usePlexPlaylistEdits.ts index c4bdf94a..7f497f47 100644 --- a/frontend/src/components/usePlexPlaylistEdits.ts +++ b/frontend/src/components/usePlexPlaylistEdits.ts @@ -6,6 +6,7 @@ import { api, type PlexCard } from "../lib/api"; import { notifyActionError } from "../lib/actionErrors"; import { notify } from "../lib/notifications"; import { qk } from "../lib/queryKeys"; +import { reinsertAt } from "../lib/reinsert"; /** 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 @@ -81,14 +82,15 @@ export function usePlexPlaylistEdits(playlistId: number) { } /** Take `keys` out — one row, a season or a whole show — with no confirm: the toast's Undo puts - * each row back at the index it held (E28 C1, the same bargain as a YouTube playlist's rows). - * `title` names what went in that toast. */ + * each row back where it stood (E28 C1, the same bargain as a YouTube playlist's rows) — after + * its old neighbour in the list as it is at Undo time (`reinsertAt`), since a raw index drifts + * once anything changes in between. `title` names what went in that toast. */ async function onRemove(keys: string[], title: string) { if (busy || !keys.length) return; setBusy(true); - // Indices over the list as it stands now, which is the server's order: a season's episodes need - // not sit together (a reorder inside the show interleaves them), so each key keeps its own. - const positions = keys.map((k) => items.findIndex((i) => i.id === k)); + // The list as it stands now, which is the server's order: a season's episodes need not sit + // together (a reorder inside the show interleaves them), so each key keeps its own neighbour. + const before = items.map((i) => i.id); const set = new Set(keys); setItems((prev) => prev.filter((i) => !set.has(i.id))); try { @@ -102,19 +104,27 @@ export function usePlexPlaylistEdits(playlistId: number) { } finally { setBusy(false); } - const back = keys.filter((_, i) => positions[i]! >= 0); notify({ level: "success", message: t("plex.playlist.removedToast", { title }), action: { label: t("common.undo"), onClick: () => - void api - .plexPlaylistAddBulk( - playlistId, - back, - positions.filter((p) => p >= 0), - ) + void qc + .fetchQuery({ + queryKey: qk.plexPlaylist(playlistId), + queryFn: () => api.plexPlaylist(playlistId), + staleTime: 0, + }) + .then((now) => { + const back = reinsertAt( + before, + keys, + now.items.map((i) => i.id), + ); + if (back.keys.length) + return api.plexPlaylistAddBulk(playlistId, back.keys, back.positions); + }) .catch((e: unknown) => notifyActionError(e, t("plex.playlist.undoRemoveFailed"))) .finally(invalidate), }, diff --git a/frontend/src/lib/plexWatch.ts b/frontend/src/lib/plexWatch.ts index 7749af7c..ed3b2139 100644 --- a/frontend/src/lib/plexWatch.ts +++ b/frontend/src/lib/plexWatch.ts @@ -7,8 +7,10 @@ import { notifyActionError } from "./actionErrors"; // `resync` (invalidate) — which corrects the optimistic flip to truth either way. Shared by the // browse grid and the person page, each of which owns a different cache shape. // -// `revert` is for a cache `resync` must NOT refetch — a random draw (Discover's Surprise rail), which -// a refetch would reshuffle under the viewer. It is called on a failure only, to put the card back. +// `revert` is for state `resync` cannot correct, called on a failure only to put the card back: a +// cache it must NOT refetch — a random draw (Discover's Surprise rail), which a refetch would +// reshuffle under the viewer — or a component's local copy of the rows (a Plex playlist), which an +// identical refetch never re-seeds (react-query keeps the same `data`). export async function toggleWatchState( card: PlexCard, t: TFunction, diff --git a/frontend/src/lib/reinsert.test.ts b/frontend/src/lib/reinsert.test.ts new file mode 100644 index 00000000..947bd09e --- /dev/null +++ b/frontend/src/lib/reinsert.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; + +import { reinsertAt } from "./reinsert"; + +describe("reinsertAt", () => { + it("puts a row back where it was when nothing changed", () => { + expect(reinsertAt(["a", "b", "c"], ["b"], ["a", "c"])).toEqual({ + keys: ["b"], + positions: [1], + }); + }); + + it("follows its neighbour when a row was added above it meanwhile", () => { + // "x" landed at the top between the removal and the Undo: index 1 would now sit before "a". + expect(reinsertAt(["a", "b", "c"], ["b"], ["x", "a", "c"])).toEqual({ + keys: ["b"], + positions: [2], + }); + }); + + it("follows its neighbour through a reorder", () => { + expect(reinsertAt(["a", "b", "c"], ["b"], ["c", "a"])).toEqual({ keys: ["b"], positions: [2] }); + }); + + it("walks further back when its neighbour is gone too", () => { + expect(reinsertAt(["a", "b", "c", "d"], ["c"], ["b", "d"])).toEqual({ + keys: ["c"], + positions: [1], + }); + }); + + it("goes to the start when it was first or no earlier row is left", () => { + expect(reinsertAt(["a", "b"], ["a"], ["x", "b"])).toEqual({ keys: ["a"], positions: [0] }); + expect(reinsertAt(["a", "b", "c"], ["b"], ["c"])).toEqual({ keys: ["b"], positions: [0] }); + }); + + it("keeps scattered and adjacent removed rows in their order, ascending", () => { + // A season's episodes interleaved with another's: b and d out, then "x" added at the top. + expect(reinsertAt(["a", "b", "c", "d", "e"], ["d", "b"], ["x", "a", "c", "e"])).toEqual({ + keys: ["b", "d"], + positions: [2, 4], + }); + expect(reinsertAt(["a", "b", "c", "d"], ["b", "c"], ["a", "d"])).toEqual({ + keys: ["b", "c"], + positions: [1, 2], + }); + }); + + it("skips a row that is already back", () => { + expect(reinsertAt(["a", "b", "c"], ["b", "c"], ["a", "c"])).toEqual({ + keys: ["b"], + positions: [1], + }); + }); +}); diff --git a/frontend/src/lib/reinsert.ts b/frontend/src/lib/reinsert.ts new file mode 100644 index 00000000..657255b7 --- /dev/null +++ b/frontend/src/lib/reinsert.ts @@ -0,0 +1,39 @@ +/** Where removed rows go back when their Undo comes LATER than the removal. + * + * A raw index recorded at removal time drifts: an add or a reorder landing in between (another tab, + * the rail, the player) shifts everything after it, and the row came back one or more places off + * (E28 review C4). So each removed row goes back right after the row that preceded it in the list + * it was removed from — its neighbour, looked up in the list as it stands NOW — or at the start when + * it was first or no earlier neighbour is left. Rows already back in the list are skipped (the + * server's add is idempotent and would leave them where they are). + * + * @param before the list's ids when the rows were removed, in stored order + * @param removed the ids taken out + * @param now the list's ids at Undo time, in stored order + * @returns the ids to add back and, paired one to one, the index each lands at — ascending, the + * shape the server's insert takes (each index counts the rows put back before it). */ +export function reinsertAt( + before: readonly string[], + removed: readonly string[], + now: readonly string[], +): { keys: string[]; positions: number[] } { + const out = new Set(removed); + const list = [...now]; + const placed: string[] = []; + for (let i = 0; i < before.length; i++) { + const id = before[i]!; + if (!out.has(id) || now.includes(id)) continue; + let at = 0; + for (let j = i - 1; j >= 0; j--) { + const k = list.indexOf(before[j]!); + if (k >= 0) { + at = k + 1; + break; + } + } + list.splice(at, 0, id); + placed.push(id); + } + const keys = placed.map((id) => ({ id, at: list.indexOf(id) })).sort((a, b) => a.at - b.at); + return { keys: keys.map((k) => k.id), positions: keys.map((k) => k.at) }; +}