From c6bf84a5780717482b8a165d1fece6a82bdb8d56 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 08:13:07 +0200 Subject: [PATCH] fix(home): a shelf move acts on the order shown, one write at a time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - During a budget switch the rail shows the previous budget's payload (keepPreviousData) while the new key is empty; the move read that empty cache, found no tile and silently did nothing. It now acts on the order the rail rendered. - A refused move was undone only by a re-read; when that read failed too, the shelf kept showing the order the user had just been told was not saved. Moves are now written one at a time (the move rows wait, with a "saving" note), which makes restoring the shown order correct — the overlapping-moves problem that ruled a rollback out cannot occur. --- frontend/src/components/Home.pin.dom.test.tsx | 50 ++++++++++++++-- frontend/src/components/Home.tsx | 60 +++++++++++++------ frontend/src/i18n/locales/en/home.json | 3 +- frontend/src/i18n/locales/hu/home.json | 3 +- 4 files changed, 91 insertions(+), 25 deletions(-) diff --git a/frontend/src/components/Home.pin.dom.test.tsx b/frontend/src/components/Home.pin.dom.test.tsx index 6489ab53..3fe11a54 100644 --- a/frontend/src/components/Home.pin.dom.test.tsx +++ b/frontend/src/components/Home.pin.dom.test.tsx @@ -210,10 +210,7 @@ describe("Home — the comfort shelf's context menu", () => { ]); }); - it("re-asks the server when a move fails instead of restoring a snapshot", async () => { - // Writing a captured `before` back was wrong twice: two overlapping moves each reverted to their - // own snapshot and the shelf settled on an order neither side held, and `before` is the WHOLE - // payload, so the rollback also discarded any rail refresh that landed mid-flight. + it("re-asks the server when a move fails", async () => { reorderComfortPins.mockRejectedValue(new Error("nope")); mount(); await screen.findByText("Comfort A"); @@ -223,6 +220,51 @@ describe("Home — the comfort shelf's context menu", () => { await waitFor(() => expect(discover).toHaveBeenCalledTimes(2)); }); + it("puts the shown order back when a move is refused, even if the re-read fails too", async () => { + // Only a re-read used to undo a refused move — and when that read failed for the same reason the + // write did, react-query kept the last data: the shelf showed the order the user had just been + // told was not saved. With one write at a time, the order shown before it IS the server's. + reorderComfortPins.mockRejectedValue(new Error("nope")); + mount(); + await screen.findByText("Comfort A"); + discover.mockRejectedValue(new Error("still down")); + fireEvent.contextMenu(screen.getByText("Comfort A")); + fireEvent.click(item("home.shelf.moveRight")); + await waitFor(() => expect(discover).toHaveBeenCalledTimes(2)); + await waitFor(() => + expect(screen.getAllByText(/^Comfort [ABC]$/).map((e) => e.textContent)).toEqual([ + "Comfort A", + "Comfort B", + "Comfort C", + ]), + ); + }); + + it("writes one move at a time: the rows wait, and say so, while one is being saved", async () => { + reorderComfortPins.mockReturnValue(new Promise(() => {})); + discover.mockResolvedValueOnce(payload()).mockReturnValue(new Promise(() => {})); + mount(); + fireEvent.contextMenu(await screen.findByText("Comfort A")); + fireEvent.click(item("home.shelf.moveRight")); + await waitFor(() => expect(reorderComfortPins).toHaveBeenCalledTimes(1)); + fireEvent.contextMenu(screen.getByText("Comfort C")); + // The first row carries the note, so its text is its label followed by the note. + expect(item("home.shelf.moveFirst" + "home.shelf.moveSaving").disabled).toBe(true); + expect(item("home.shelf.moveLeft").disabled).toBe(true); + }); + + it("moves what the rail SHOWS while a new budget is still loading", async () => { + // Under keepPreviousData the rail keeps the previous budget's payload on screen while the new + // key holds nothing yet; the move read that empty cache, found no tile and did nothing. + mount(); + await screen.findByText("Comfort A"); + discover.mockReturnValue(new Promise(() => {})); // the new budget never arrives + fireEvent.click(screen.getByText("home.budget.short")); + fireEvent.contextMenu(screen.getByText("Comfort A")); + fireEvent.click(item("home.shelf.moveRight")); + await waitFor(() => expect(reorderComfortPins).toHaveBeenCalledWith([7, 3, 9])); + }); + it("unpins with the pin id the shelf card carries", async () => { mount(); fireEvent.contextMenu(await screen.findByText("Comfort C")); diff --git a/frontend/src/components/Home.tsx b/frontend/src/components/Home.tsx index 4e96f003..370a266e 100644 --- a/frontend/src/components/Home.tsx +++ b/frontend/src/components/Home.tsx @@ -1,4 +1,4 @@ -import { lazy, Suspense, useCallback, useEffect, useState } from "react"; +import { lazy, Suspense, useCallback, useEffect, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { keepPreviousData, useQuery, useQueryClient } from "@tanstack/react-query"; import { @@ -95,6 +95,7 @@ function Rail({ onToggleSave, onToggleWatched, onMove, + moveSaving = false, }: { title: string; ariaLabel: string; @@ -106,8 +107,11 @@ function Rail({ onToggleSave: (id: string, saved: boolean) => void; onToggleWatched: (card: PlexCard) => void; /** Only the comfort shelf is orderable. Given, this rail's menu grows the four move rows; omitted, - * its tiles offer the plain pin instead. */ - onMove?: (card: DiscoverCard, to: ShelfMove) => void; + * its tiles offer the plain pin instead. `rendered` is the order this rail is SHOWING — the one a + * move must act on (see `moveInShelf`). */ + onMove?: (card: DiscoverCard, to: ShelfMove, rendered: DiscoverCard[]) => void; + /** A reorder is being written: the move rows wait for it (one write at a time). */ + moveSaving?: boolean; }) { const { t } = useTranslation(); // ONE right-click menu for the rail (see ui/ContextMenu). It is the shelf's whole editor: menu @@ -138,8 +142,9 @@ function Rail({ icon: , separatorBefore: i === 0, // The same rule the move itself runs, so a row is never enabled onto a no-op. - disabled: shelfMoveTarget(shelfIdx, to, cards.length) < 0, - onSelect: () => onMove(target, to), + disabled: moveSaving || shelfMoveTarget(shelfIdx, to, cards.length) < 0, + note: moveSaving && i === 0 ? t("home.shelf.moveSaving") : undefined, + onSelect: () => onMove(target, to, cards), })), { id: "shelf-unpin", @@ -594,13 +599,26 @@ export default function Home() { ); // Shelf reorder, from the comfort rail's menu. Optimistic on THIS budget's cached payload so the - // tile moves under the pointer, then written; a failure re-asks the server rather than leaving the - // page showing an order the server never accepted (the same lesson the playlist reorder learned). + // tile moves under the pointer, then written. + // + // It acts on the order the rail RENDERED, not on a fresh read of this budget's cache: during a + // budget switch the rail still shows the previous budget's payload (`keepPreviousData`) while the + // new key holds nothing yet, so `from` came back -1 and the move silently did nothing — the + // "looks available, then no-ops" row `shelfMoveTarget` exists to rule out. + // + // ONE write at a time (`shelfSaving`; the move rows wait with a note). That is what makes a + // failure's rollback right: with no overlapping move, "the order the rail showed before this move" + // is the order the server still holds, so a refused move puts it back instead of leaving the + // rejected order on screen whenever the follow-up read failed too. (Two overlapping moves, each + // reverting to its own snapshot, settled on an order neither side ever held — why the rollback + // used to be a re-read only.) + const [shelfSaving, setShelfSaving] = useState(false); + const shelfSavingRef = useRef(false); const moveInShelf = useCallback( - (card: DiscoverCard, to: ShelfMove) => { + (card: DiscoverCard, to: ShelfMove, rendered: DiscoverCard[]) => { + if (shelfSavingRef.current) return; const key = qk.discover(budget); - const before = qc.getQueryData(key); - const cards = before?.comfort ?? []; + const cards = rendered; const from = cards.findIndex((c) => c.pin_id === card.pin_id); const target = shelfMoveTarget(from, to, cards.length); if (target < 0) return; @@ -613,20 +631,23 @@ export default function Home() { // type, and a short list would be rejected by the server rather than silently reordering half // the shelf. const ids = next.map((c) => c.pin_id).filter((id): id is number => id != null); + shelfSavingRef.current = true; + setShelfSaving(true); void api .reorderComfortPins(ids) - .catch((e: unknown) => notifyActionError(e, t("home.shelf.moveFailed"))) + .catch((e: unknown) => { + qc.setQueryData(key, (old) => (old ? { ...old, comfort: cards } : old)); + notifyActionError(e, t("home.shelf.moveFailed")); + }) .finally(() => { + shelfSavingRef.current = false; + setShelfSaving(false); // The shelf is also the pin menus' source of truth everywhere else in the app. void qc.invalidateQueries({ queryKey: qk.comfortPins() }); - // And the PREFIX, not this budget's key — which is what actually re-asks the server, and - // is what the comment above this callback promises. Restoring the captured snapshot - // instead was wrong twice over: two overlapping moves each reverted to their own - // snapshot, so the shelf settled on an order neither side ever held; and `before` is the - // whole payload, so a rollback also threw away any rail refresh that had landed while the - // request was in flight. On the success path the prefix matters for a second reason — the - // shelf is budget-independent server-side but cached per budget, so without it the next - // chip click renders the pre-move order and a move from there writes it back. + // And the PREFIX, not this budget's key: the shelf is budget-independent server-side but + // cached per budget, so without it the next chip click renders the pre-move order and a + // move from there writes it back. (The rollback above restores only `comfort`, so a rail + // refresh that landed meanwhile is kept.) void qc.invalidateQueries({ queryKey: qk.discover() }); }); }, @@ -742,6 +763,7 @@ export default function Home() { onToggleSave={onToggleSave} onToggleWatched={onToggleWatched} onMove={moveInShelf} + moveSaving={shelfSaving} /> {!resumeConsumed && (