diff --git a/frontend/src/lib/useUndoable.dom.test.ts b/frontend/src/lib/useUndoable.dom.test.ts index 88003698..8b6d6129 100644 --- a/frontend/src/lib/useUndoable.dom.test.ts +++ b/frontend/src/lib/useUndoable.dom.test.ts @@ -18,6 +18,16 @@ describe("useUndoable", () => { expect(result.current.value).toEqual([10, 20]); }); + it("peeks at a value set in this tick, before it renders", () => { + const { result } = renderHook(() => useUndoable([1])); + const { peek, set } = result.current; + act(() => { + set([1, 2]); + expect(peek()).toEqual([1, 2]); + }); + expect(peek()).toEqual([1, 2]); + }); + it("keeps the undo history through a rebase", () => { const { result } = renderHook(() => useUndoable([1])); act(() => result.current.set([1, 2])); diff --git a/frontend/src/lib/useUndoable.ts b/frontend/src/lib/useUndoable.ts index 99505d4f..5e3caf21 100644 --- a/frontend/src/lib/useUndoable.ts +++ b/frontend/src/lib/useUndoable.ts @@ -16,6 +16,8 @@ import { useLatest } from "./useLatest"; // Render never reads the ref: a compiled component would otherwise memoize a stale value. export interface Undoable { value: T; + /** The value as of NOW, with a `set` not rendered yet, for code that runs after an await. */ + peek: () => T; set: (next: T) => void; reset: (value: T) => void; rebase: (map: (value: T) => T) => void; @@ -110,8 +112,11 @@ export function useUndoable( setH(over); }, []); + const peek = useCallback(() => ref.current.present, []); + return { value: h.present, + peek, set, reset, rebase, diff --git a/frontend/src/modules/playlists/Playlists.dom.test.tsx b/frontend/src/modules/playlists/Playlists.dom.test.tsx index deb5e9b1..3e3b2dcf 100644 --- a/frontend/src/modules/playlists/Playlists.dom.test.tsx +++ b/frontend/src/modules/playlists/Playlists.dom.test.tsx @@ -2290,12 +2290,15 @@ describe("Playlists — writes that overlap", () => { }; it("keeps a row out when a read sent before its removal lands after it, and its history too", async () => { - vi.spyOn(api, "reorderPlaylist").mockResolvedValue(undefined as never); + const lists = { 1: [...abc()[1], vid("D", 0)] }; + vi.spyOn(api, "reorderPlaylist").mockImplementation((async () => { + lists[1] = [vid("A", 0), vid("C", 0), vid("B", 0), vid("D", 0)]; + }) as never); const second = later(); vi.spyOn(api, "removeManyFromPlaylist") .mockResolvedValueOnce(wentOut("A") as never) .mockReturnValueOnce(second.promise as never); - mount({ 1: [...abc()[1], vid("D", 0)] }); + mount(lists); await inPage(); await rowMenu("Video C", "common.order.up"); await waitFor(() => expect(undoOrder().disabled).toBe(false)); @@ -2591,16 +2594,17 @@ describe("Playlists — writes that overlap", () => { }); it("keeps a row out, and the history, when a read from before its removal landed and a patch comes after", async () => { - vi.spyOn(api, "reorderPlaylist").mockResolvedValue(undefined as never); const write = later(); vi.spyOn(api, "removeManyFromPlaylist").mockReturnValue(write.promise as never); const lists = abc(); + vi.spyOn(api, "reorderPlaylist").mockImplementation((async () => { + lists[1] = [vid("A", 0), vid("C", 0), vid("B", 0)]; + }) as never); mount(lists); await inPage(); await landed(); await rowMenu("Video C", "common.order.up"); await waitFor(() => expect(undoOrder().disabled).toBe(false)); - lists[1] = [vid("A", 0), vid("C", 0), vid("B", 0)]; await landed(); await removeRow("Video A", "Video C", "Video B"); await act(async () => { @@ -2704,6 +2708,261 @@ describe("Playlists — writes that overlap", () => { await waitFor(() => expect(restore).toHaveBeenCalledWith(1, ["B"], [2])); }); + describe("the order on the server", () => { + it("asks for an Undo's place only when the last of two writes out has landed", async () => { + const writes = [later(), later()]; + const restore = vi + .spyOn(api, "restoreToPlaylist") + .mockResolvedValue({ ok: true, added: 1 } as never); + const lists = abc(); + vi.spyOn(api, "removeManyFromPlaylist") + .mockImplementationOnce((async () => wentOut("D")) as never) + .mockReturnValueOnce(writes[0]!.promise as never) + .mockReturnValueOnce(writes[1]!.promise as never); + mount({ 1: [...lists[1], vid("D", 0)] }); + vi.mocked(api.playlist).mockImplementation((async (id: number) => + list(id, lists[1])) as never); + await inPage(); + await landed(); + await removeRow("Video D", "Video A", "Video B", "Video C"); + await waitFor(() => expect(lastUndo()).toBeTruthy()); + const undo = lastUndo()!; + await landed(); + await removeRow("Video A", "Video B", "Video C"); + await removeRow("Video B", "Video C"); + await act(async () => undo.action!.onClick()); + await act(async () => writes[0]!.ok(wentOut("A"))); + await landed(); + expect(restore).not.toHaveBeenCalled(); + await act(async () => writes[1]!.ok(wentOut("B"))); + await waitFor(() => expect(restore).toHaveBeenCalled()); + }); + + const named = (lists: Record, id: number, ids: readonly string[]) => { + lists[id] = ids.map((v) => vid(v, 0)); + }; + const onList = async (id: number, title: string, rerender: () => void) => { + h.selectedId = id; + rerender(); + await screen.findByRole("button", { name: `card.play — ${title}` }); + await landed(); + }; + + it("shows an order changed elsewhere, and forgets the history of the one it replaces", async () => { + const lists = abc(); + vi.spyOn(api, "reorderPlaylist").mockImplementation((async (id: 1, ids: string[]) => { + named(lists, id, ids); + }) as never); + mount(lists); + await inPage(); + await landed(); + await rowMenu("Video C", "common.order.up"); + await waitFor(() => expect(undoOrder().disabled).toBe(false)); + await landed(); + // A revert to YouTube, another device, a sync: the same rows, in another order. + named(lists, 1, ["B", "A", "C"]); + await act(async () => { + await qc.invalidateQueries({ queryKey: qk.playlist(1) }); + }); + await waitFor(() => expect(rowsOnScreen()).toEqual(["Video B", "Video A", "Video C"])); + expect(undoOrder().disabled).toBe(true); + }); + + it("keeps the history when a read only confirms the reorder", async () => { + const lists = abc(); + vi.spyOn(api, "reorderPlaylist").mockImplementation((async (id: 1, ids: string[]) => { + named(lists, id, ids); + }) as never); + mount(lists); + await inPage(); + await landed(); + await rowMenu("Video C", "common.order.up"); + await waitFor(() => expect(undoOrder().disabled).toBe(false)); + await act(async () => { + await qc.invalidateQueries({ queryKey: qk.playlist(1) }); + }); + await landed(); + expect(rowsOnScreen()).toEqual(["Video A", "Video C", "Video B"]); + expect(undoOrder().disabled).toBe(false); + }); + + it("waits with an order changed elsewhere until no list has a write out", async () => { + const post = later(); + vi.spyOn(api, "removeManyFromPlaylist").mockReturnValue(post.promise as never); + const lists: Record = { 1: [vid("A", 0)], 2: [vid("X", 0), vid("Y", 0)] }; + const { rerender } = mount(lists); + await play("Video A"); + await waitFor(() => expect(h.player).not.toBeNull()); + await onList(2, "Video Y", rerender); + // The open player removes from the list it was launched from: a write out, on another list. + act(() => void h.player!.onRemoveFromList!(vid("A", 0))); + await landed(); + named(lists, 2, ["Y", "X"]); + await act(async () => { + await qc.invalidateQueries({ queryKey: qk.playlist(2) }); + }); + await landed(); + expect(rowsOnScreen()).toEqual(["Video X", "Video Y"]); + await act(async () => post.ok(wentOut("A"))); + await waitFor(() => expect(rowsOnScreen()).toEqual(["Video Y", "Video X"])); + }); + + it("shows a reorder accepted after its list was picked again", async () => { + const put = later(); + vi.spyOn(api, "reorderPlaylist").mockReturnValue(put.promise as never); + const lists: Record = { ...abc(), 2: [vid("X", 0)] }; + const { rerender } = mount(lists); + await inPage(); + await landed(); + qc.setQueryData(qk.playlist(2), list(2, lists[2]!)); + await rowMenu("Video C", "common.order.up"); + await waitFor(() => expect(api.reorderPlaylist).toHaveBeenCalled()); + await onList(2, "Video X", rerender); + await onList(1, "Video A", rerender); + named(lists, 1, ["A", "C", "B"]); + await act(async () => put.ok(undefined)); + await waitFor(() => expect(rowsOnScreen()).toEqual(["Video A", "Video C", "Video B"])); + }); + + it("keeps another list's history when a failure's read lands after a reorder there began", async () => { + const put = later(); + vi.spyOn(api, "reorderPlaylist").mockReturnValue(put.promise as never); + vi.spyOn(api, "removeManyFromPlaylist").mockRejectedValue(new TypeError("Failed to fetch")); + const lists: Record = { ...abc(), 2: [vid("X", 0), vid("Y", 0)] }; + const { rerender } = mount(lists); + await inPage(); + await landed(); + qc.setQueryData(qk.playlist(2), list(2, lists[2]!)); + const read = later(); + vi.mocked(api.playlist).mockReturnValue(read.promise as never); + await rowMenu("Video C", "playlists.removeItem"); + await waitFor(() => expect(h.notify).toHaveBeenCalled()); + await landed(); + await onList(2, "Video Y", rerender); + await rowMenu("Video Y", "common.order.up"); + await waitFor(() => expect(api.reorderPlaylist).toHaveBeenCalled()); + vi.mocked(api.playlist).mockImplementation((async (id: number) => + list(id, lists[id] ?? [])) as never); + await act(async () => read.ok(list(1, lists[1]!))); + await landed(); + named(lists, 2, ["Y", "X"]); + await act(async () => put.ok(undefined)); + await landed(); + expect(rowsOnScreen()).toEqual(["Video Y", "Video X"]); + expect(undoOrder().disabled).toBe(false); + }); + + it("shows the server's place for a row whose removal failed on a list left meanwhile", async () => { + const post = later(); + vi.spyOn(api, "removeManyFromPlaylist").mockReturnValue(post.promise as never); + const lists: Record = { + 1: [vid("A", 0), vid("B", 0), vid("C", 0), vid("D", 0)], + 2: [vid("X", 0)], + }; + // The server keeps a row the PUT does not name after the ones it does. + vi.spyOn(api, "reorderPlaylist").mockImplementation((async (id: 1, ids: string[]) => { + named(lists, id, [...ids, "B"]); + }) as never); + const { rerender } = mount(lists); + await inPage(); + await landed(); + qc.setQueryData(qk.playlist(2), list(2, lists[2]!)); + await removeRow("Video B", "Video A", "Video C", "Video D"); + await rowMenu("Video D", "common.order.up"); + await waitFor(() => expect(api.reorderPlaylist).toHaveBeenCalled()); + await landed(); + await onList(2, "Video X", rerender); + await act(async () => post.no(new TypeError("Failed to fetch"))); + await landed(); + await onList(1, "Video A", rerender); + await waitFor(() => + expect(rowsOnScreen()).toEqual(["Video A", "Video D", "Video C", "Video B"]), + ); + }); + + it("sends a list's reorders one after the other", async () => { + const first = later(); + const reorder = vi + .spyOn(api, "reorderPlaylist") + .mockReturnValueOnce(first.promise as never) + .mockResolvedValueOnce(undefined as never); + mount(abc()); + await inPage(); + await landed(); + await rowMenu("Video C", "common.order.up"); + await waitFor(() => expect(rowsOnScreen()).toEqual(["Video A", "Video C", "Video B"])); + await rowMenu("Video C", "common.order.up"); + await waitFor(() => expect(rowsOnScreen()).toEqual(["Video C", "Video A", "Video B"])); + await landed(); + expect(reorder).toHaveBeenCalledTimes(1); + await act(async () => first.ok(undefined)); + await waitFor(() => expect(reorder).toHaveBeenCalledTimes(2)); + expect(reorder).toHaveBeenLastCalledWith(1, ["C", "A", "B"]); + }); + + it("keeps a stored position that landed during a reorder when the read after it fails", async () => { + const put = later(); + vi.spyOn(api, "reorderPlaylist").mockReturnValue(put.promise as never); + mount(abc()); + await inPage(); + await landed(); + await rowMenu("Video C", "common.order.up"); + await waitFor(() => expect(api.reorderPlaylist).toHaveBeenCalled()); + act(() => patchCachedProgress(qc, new Map([["C", 830]]))); + await landed(); + vi.mocked(api.playlist).mockRejectedValue(new TypeError("Failed to fetch")); + await act(async () => put.ok(undefined)); + await landed(); + await play("Video C"); + await waitFor(() => expect(h.player?.video.position_seconds).toBe(830)); + }); + + it.each([ + ["a removal", "Video A", ["B", "D"], 1], + ["a reorder", "Video D", ["A", "D", "B"], 3], + ] as const)( + "asks for an Undo's place only when %s still out has landed", + async (kind, title, after, at) => { + const write = later(); + const restore = vi + .spyOn(api, "restoreToPlaylist") + .mockResolvedValue({ ok: true, added: 1 } as never); + const lists: Record = { + 1: [vid("A", 0), vid("B", 0), vid("C", 0), vid("D", 0)], + }; + const remove = vi.spyOn(api, "removeManyFromPlaylist"); + remove.mockImplementationOnce((async () => { + lists[1] = lists[1]!.filter((v) => v.id !== "C"); + return wentOut("C"); + }) as never); + vi.spyOn(api, "reorderPlaylist").mockReturnValue(write.promise as never); + mount(lists); + await inPage(); + await landed(); + await removeRow("Video C", "Video A", "Video B", "Video D"); + await waitFor(() => expect(lastUndo()).toBeTruthy()); + const undo = lastUndo()!; + await landed(); + if (kind === "a removal") { + remove.mockReturnValueOnce(write.promise as never); + await removeRow(title, "Video B", "Video D"); + } else { + await rowMenu(title, "common.order.up"); + await waitFor(() => expect(api.reorderPlaylist).toHaveBeenCalled()); + } + await landed(); + const before = reads(); + await act(async () => undo.action!.onClick()); + await landed(); + expect(reads()).toBe(before); + expect(restore).not.toHaveBeenCalled(); + named(lists, 1, after); + await act(async () => write.ok(wentOut("A"))); + await waitFor(() => expect(restore).toHaveBeenCalledWith(1, ["C"], [at])); + }, + ); + }); + it("speaks of one video when one row found nothing to do", async () => { vi.spyOn(api, "removeManyFromPlaylist").mockResolvedValue(wentOut() as never); mount(abc()); @@ -2891,6 +3150,24 @@ describe("Playlists — the focus when what holds it goes", () => { expect(document.activeElement?.closest("table")).toBeNull(); }); + it("does not count on Play all, disabled by the last row going, to take it back", async () => { + mountList([vid("A", 0)], "A"); + const all = await screen.findByRole("button", { + name: "playlists.playAll", + }); + all.focus(); + fireEvent.click(all); + await waitFor(() => expect(h.mounted).toBe(true)); + card = document.createElement("div"); + card.tabIndex = -1; + document.body.appendChild(card); + card.focus(); + await removeFromPlayer(vid("A", 0)); + await waitFor(() => expect(all.disabled).toBe(true)); + close(); + await waitFor(() => expect(document.activeElement).not.toBe(document.body)); + }); + it("moves nothing while the one that opened it is still there", async () => { mount({ 1: [vid("A", 0), vid("B", 0)] }); await openFrom("Video B"); diff --git a/frontend/src/modules/playlists/Playlists.tsx b/frontend/src/modules/playlists/Playlists.tsx index 8bc34054..9a35ec45 100644 --- a/frontend/src/modules/playlists/Playlists.tsx +++ b/frontend/src/modules/playlists/Playlists.tsx @@ -138,6 +138,8 @@ const idsKey = (items: Video[]) => .sort() .join(","); +const orderOf = (items: Video[]) => items.map((v) => v.id).join(","); + const moveTargetIcon = (pl: Playlist) => pl.source === "youtube" || pl.yt_playlist_id ? ( @@ -315,9 +317,11 @@ function PlaylistDetail() { // only a row's own fields, a stored position, are taken. const writesOut = useRef(new Map()); const refreshDue = useRef(new Set()); - // A rollback sets rows back by what the cache held, which an accepted reorder may have moved on from, - // and the membership guard never corrects an order: the read after the last write decides it. - const orderInDoubt = useRef(false); + // The last reorder sent on each list. Each is the whole list and the server applies what it is given, + // so the next one waits for it: two side by side could reach the server's lock either way round. + const reordersOut = useRef(new Map>()); + // Who waits for a list's writes to land (see `writesLanded`). + const landingWaits = useRef(new Map void)[]>()); // The stored order is undoable: drag and "save this order" both go through `order.set`, so each // is reversible (buttons + Ctrl+Z/Y). onApply persists the new order to the server. const order = useUndoable([], { @@ -325,36 +329,39 @@ function PlaylistDetail() { if (selectedId == null) return; const listId = selectedId; writeStarts(listId); - api - .reorderPlaylist( - listId, - next.map((v) => v.id), + const sent = (reordersOut.current.get(listId) ?? Promise.resolve()) + .then(() => + api.reorderPlaylist( + listId, + next.map((v) => v.id), + ), ) .then(() => { // Write the accepted order into the cache BEFORE the refetch. The cache is what // `restoreFromCache` rolls back to, and until this refetch landed it still held the order // from before this reorder — so a SECOND reorder failing in that window rolled the rows - // back to an order the server does not have, permanently (the membership guard cannot - // correct an order). One accepted write, one cache entry that matches it. + // back to an order the server does not have. One accepted write, one cache entry that + // matches it. // A row taken out while this was on its way stays out: `next` is from before that. A row it // does not name stays too, after the ones it does, where the server keeps it: one whose - // removal failed meanwhile, or that came in from elsewhere. + // removal failed meanwhile, or that came in from elsewhere. Each row as the cache has it: a + // stored position may have landed since `next` was made. writeDone(listId); qc.setQueryData(qk.playlist(listId), (old) => { if (!old) return old; - const held = new Set(old.items.map((v) => v.id)); + const held = new Map(old.items.map((v) => [v.id, v])); const named = new Set(next.map((v) => v.id)); return { ...old, items: [ - ...next.filter((v) => held.has(v.id)), + ...next.flatMap((v) => held.get(v.id) ?? []), ...old.items.filter((v) => !named.has(v.id)), ], }; }); // Refresh the sidebar (cover may change) and the detail (so the now-dirty state, - // and the Reset/Unsynced indicators, show without an F5). The membership guard - // keeps the refetch from wiping the undo history on a pure reorder. + // and the Reset/Unsynced indicators, show without an F5). A read that only confirms + // this order keeps the undo history. refreshAfterEdit([]); }) .catch((e) => { @@ -366,6 +373,7 @@ function PlaylistDetail() { restoreFromCache(listId); refreshAfterEdit([]); }); + reordersOut.current.set(listId, sent); }, }); const items = order.value; @@ -376,9 +384,9 @@ function PlaylistDetail() { const [viewSort, setViewSort] = useState(opened.sort); const [filters, setFilters] = useState(opened.filters); const [groupBy, setGroupBy] = useState(opened.groupBy); - // Tracks the membership (id set) currently loaded into `order`, so a refetch that only - // re-confirms our own reorder doesn't wipe the undo history — we reset history only when - // the actual set of items changes (different playlist, or an add/remove). + // Tracks the membership (id set) currently loaded into `order`: a different playlist, or an add or + // a remove, resets the undo history. So does another order (see the effect below); a refetch that + // only re-confirms our own reorder does not. const lastSetRef = useRef(""); // The selection as of NOW, for code that runs after an await or on behalf of the open player. const selectedRef = useLatest(selectedId); @@ -393,7 +401,6 @@ function PlaylistDetail() { if (!cur) return; lastSetRef.current = `${listId}:${idsKey(cur.items)}`; order.reset(cur.items); - orderInDoubt.current = true; } // What the player was LAUNCHED with, snapshotted when it opens: the video, the queue and the index // inside it. `shown` is derived (sort / filter / view), and the player freezes its queue and owns @@ -462,7 +469,7 @@ function PlaylistDetail() { // Destructured so the effect below can name what it calls: both are stable useCallbacks, while // the object around them is rebuilt every render (lib/useUndoable). - const { reset: resetOrder, rebase: rebaseOrder } = order; + const { reset: resetOrder, rebase: rebaseOrder, peek: orderNow } = order; useEffect(() => { if (!detail) return; // A write out on the list these rows are from: this may be a read from before it landed, and the @@ -473,13 +480,19 @@ function PlaylistDetail() { // Scoped to the playlist: two lists holding the SAME videos would otherwise look identical to // the guard, so the second would render in the first's order and inherit its undo stack. const key = `${selectedId}:${idsKey(detail.items)}`; - if (!held && key !== lastSetRef.current) { + // The same rows in another order: a revert to YouTube, a sync, another device. Our own writes are + // on screen before they are in the cache, so this only counts with none out on ANY list: a read + // can answer from before one, and the refresh that calls such a read off waits for them all + // (`refreshAfterEdit`, which then settles what this skipped). The history goes with it: it holds + // orders built on the one just replaced, and the next step back would write them over it. + const reordered = !writesOut.current.size && orderOf(detail.items) !== orderOf(orderNow()); + if (!held && (key !== lastSetRef.current || reordered)) { lastSetRef.current = key; resetOrder(detail.items); } else { - // Same membership, fresher ROWS — a watch position the player just stored, a status. The - // guard above looks at the id set only, so without this the list kept showing, and handing the - // player, each row as it was when the list first loaded: watch a video to 830 s, close it, + // Same rows in the same order, fresher FIELDS — a watch position the player just stored, a + // status. Without this the list kept showing, and handing the player, each row as it was when + // the list first loaded: watch a video to 830 s, close it, // reopen it, and it resumed at the old spot and the next checkpoint wrote that over the 830. // `rebase` takes the fresh rows into every snapshot and keeps the order and its undo history. const fresh = new Map(detail.items.map((v) => [v.id, v])); @@ -487,7 +500,7 @@ function PlaylistDetail() { } // `reset` and `rebase` are stable useCallbacks (lib/useUndoable), so naming them costs nothing; // `selectedId` moves only together with `detail` anyway, since detail IS the selected playlist's. - }, [detail, selectedId, resetOrder, rebaseOrder]); + }, [detail, selectedId, resetOrder, rebaseOrder, orderNow]); // Reset the view when switching playlists (the stored order itself is per-playlist) — during the // render that switches, so the incoming playlist is never drawn once through the previous one's @@ -789,13 +802,15 @@ function PlaylistDetail() { linkedOutOfReach && selectedId === railMovedTo ? linkedOver : selectedId, ); - // The player has handed the focus back: to a row gone by now it could not, and the next Tab would - // start over at the top of the page. The row now in that place takes it, else the header's box, else - // (no rows, no table) the list's "…". A focus the reader has put elsewhere stays. + // The player has handed the focus back: to a row gone by now it could not, nor to a Play all disabled + // by the last row going, and the next Tab would start over at the top of the page. The row now in + // that place takes it, else the header's box, else (no rows, no table) the list's "…". A focus the + // reader has put elsewhere stays. const focusAfterPlayer = useEffectEvent(() => { const from = openedFrom.current; openedFrom.current = null; - if (!from || from.el?.isConnected || document.activeElement !== document.body) return; + const heldItsPlace = from?.el?.isConnected && !from.el.matches(":disabled"); + if (!from || heldItsPlace || document.activeElement !== document.body) return; const rows = shownRef.current.length; if (rows && tableRef.current) focusNavItem(tableRef.current, Math.min(from.index, rows - 1)); else (selectAllRef.current ?? renameTarget.current)?.focus(); @@ -863,8 +878,20 @@ function PlaylistDetail() { } function writeDone(listId: number) { const out = writesOut.current.get(listId); - if (out && out.count > 1) out.count--; - else writesOut.current.delete(listId); + if (out && out.count > 1) { + out.count--; + return; + } + writesOut.current.delete(listId); + for (const go of landingWaits.current.get(listId) ?? []) go(); + landingWaits.current.delete(listId); + } + /** When no write is out on the list any more: until then the server's list is about to change. */ + function writesLanded(listId: number) { + if (!writesOut.current.has(listId)) return Promise.resolve(); + return new Promise((go) => { + landingWaits.current.set(listId, [...(landingWaits.current.get(listId) ?? []), go]); + }); } // The write went through: a read from before it may have brought its rows back into the cache since, // and the next thing to rewrite that entry would hand them to the membership guard. @@ -901,15 +928,13 @@ function PlaylistDetail() { void read.then(settleOrder); } // The refresh is in. A read matching the cache exactly runs no effect, so what the cache got while a - // write was out is taken here, and so is the order after a rollback. A write started since has its - // own refresh to wait for. + // write was out is taken here, and so is the order after a rollback, which went by what the cache held + // and an accepted reorder may have moved on from. A write started since has its own refresh to wait for. function settleOrder() { const listId = selectedRef.current; if (listId == null || writesOut.current.size) return; const cur = qc.getQueryData(qk.playlist(listId)); - const changed = cur && `${listId}:${idsKey(cur.items)}` !== lastSetRef.current; - if (orderInDoubt.current || changed) restoreFromCache(listId); - orderInDoubt.current = false; + if (cur && orderOf(cur.items) !== orderOf(order.peek())) restoreFromCache(listId); } const titleOf = (rows: Video[], id: string | undefined) => raw(rows.find((v) => v.id === id)?.title ?? ""); @@ -917,16 +942,20 @@ function PlaylistDetail() { const tickOf = (v: Video) => (selection.isSelected(v.id) ? [v.id] : []); // Where rows go back when their Undo comes: against the list as it is NOW, fetched fresh, and after // their neighbours in the STORED order they stood in, so neither a view sort on screen nor a change - // in between shifts them (`reinsertAt`). Past the cache: a removal starting meanwhile calls off the - // cache's read, which then answers with what the cache holds, not with what is fresh. + // in between shifts them (`reinsertAt`). After the writes still out: the place is an index, and one + // landing between this read and the Undo's own write would shift it. Past the cache: a removal starting + // meanwhile calls off the cache's read, which then answers with what the cache holds, not with what + // is fresh. const placesNow = (listId: number, storedBefore: string[], ids: string[]) => - api.playlist(listId).then((now) => - reinsertAt( - storedBefore, - ids, - now.items.map((v) => v.id), - ), - ); + writesLanded(listId) + .then(() => api.playlist(listId)) + .then((now) => + reinsertAt( + storedBefore, + ids, + now.items.map((v) => v.id), + ), + ); // An Undo that could not bring everything back says so: the server leaves out a video it cannot put // back (gone on from the target, or from the catalog), and silence would read as all of them. function sayWhatCameBack(asked: number, added: number) {