fix(home): a shelf move acts on the order shown, one write at a time
- 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.
This commit is contained in:
@@ -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 () => {
|
it("re-asks the server when a move fails", 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.
|
|
||||||
reorderComfortPins.mockRejectedValue(new Error("nope"));
|
reorderComfortPins.mockRejectedValue(new Error("nope"));
|
||||||
mount();
|
mount();
|
||||||
await screen.findByText("Comfort A");
|
await screen.findByText("Comfort A");
|
||||||
@@ -223,6 +220,51 @@ describe("Home — the comfort shelf's context menu", () => {
|
|||||||
await waitFor(() => expect(discover).toHaveBeenCalledTimes(2));
|
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 () => {
|
it("unpins with the pin id the shelf card carries", async () => {
|
||||||
mount();
|
mount();
|
||||||
fireEvent.contextMenu(await screen.findByText("Comfort C"));
|
fireEvent.contextMenu(await screen.findByText("Comfort C"));
|
||||||
|
|||||||
@@ -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 { useTranslation } from "react-i18next";
|
||||||
import { keepPreviousData, useQuery, useQueryClient } from "@tanstack/react-query";
|
import { keepPreviousData, useQuery, useQueryClient } from "@tanstack/react-query";
|
||||||
import {
|
import {
|
||||||
@@ -95,6 +95,7 @@ function Rail({
|
|||||||
onToggleSave,
|
onToggleSave,
|
||||||
onToggleWatched,
|
onToggleWatched,
|
||||||
onMove,
|
onMove,
|
||||||
|
moveSaving = false,
|
||||||
}: {
|
}: {
|
||||||
title: string;
|
title: string;
|
||||||
ariaLabel: string;
|
ariaLabel: string;
|
||||||
@@ -106,8 +107,11 @@ function Rail({
|
|||||||
onToggleSave: (id: string, saved: boolean) => void;
|
onToggleSave: (id: string, saved: boolean) => void;
|
||||||
onToggleWatched: (card: PlexCard) => void;
|
onToggleWatched: (card: PlexCard) => void;
|
||||||
/** Only the comfort shelf is orderable. Given, this rail's menu grows the four move rows; omitted,
|
/** Only the comfort shelf is orderable. Given, this rail's menu grows the four move rows; omitted,
|
||||||
* its tiles offer the plain pin instead. */
|
* its tiles offer the plain pin instead. `rendered` is the order this rail is SHOWING — the one a
|
||||||
onMove?: (card: DiscoverCard, to: ShelfMove) => void;
|
* 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();
|
const { t } = useTranslation();
|
||||||
// ONE right-click menu for the rail (see ui/ContextMenu). It is the shelf's whole editor: menu
|
// 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: <Icon className="w-4 h-4" />,
|
icon: <Icon className="w-4 h-4" />,
|
||||||
separatorBefore: i === 0,
|
separatorBefore: i === 0,
|
||||||
// The same rule the move itself runs, so a row is never enabled onto a no-op.
|
// The same rule the move itself runs, so a row is never enabled onto a no-op.
|
||||||
disabled: shelfMoveTarget(shelfIdx, to, cards.length) < 0,
|
disabled: moveSaving || shelfMoveTarget(shelfIdx, to, cards.length) < 0,
|
||||||
onSelect: () => onMove(target, to),
|
note: moveSaving && i === 0 ? t("home.shelf.moveSaving") : undefined,
|
||||||
|
onSelect: () => onMove(target, to, cards),
|
||||||
})),
|
})),
|
||||||
{
|
{
|
||||||
id: "shelf-unpin",
|
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
|
// 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
|
// tile moves under the pointer, then written.
|
||||||
// page showing an order the server never accepted (the same lesson the playlist reorder learned).
|
//
|
||||||
|
// 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(
|
const moveInShelf = useCallback(
|
||||||
(card: DiscoverCard, to: ShelfMove) => {
|
(card: DiscoverCard, to: ShelfMove, rendered: DiscoverCard[]) => {
|
||||||
|
if (shelfSavingRef.current) return;
|
||||||
const key = qk.discover(budget);
|
const key = qk.discover(budget);
|
||||||
const before = qc.getQueryData<DiscoverPayload>(key);
|
const cards = rendered;
|
||||||
const cards = before?.comfort ?? [];
|
|
||||||
const from = cards.findIndex((c) => c.pin_id === card.pin_id);
|
const from = cards.findIndex((c) => c.pin_id === card.pin_id);
|
||||||
const target = shelfMoveTarget(from, to, cards.length);
|
const target = shelfMoveTarget(from, to, cards.length);
|
||||||
if (target < 0) return;
|
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
|
// type, and a short list would be rejected by the server rather than silently reordering half
|
||||||
// the shelf.
|
// the shelf.
|
||||||
const ids = next.map((c) => c.pin_id).filter((id): id is number => id != null);
|
const ids = next.map((c) => c.pin_id).filter((id): id is number => id != null);
|
||||||
|
shelfSavingRef.current = true;
|
||||||
|
setShelfSaving(true);
|
||||||
void api
|
void api
|
||||||
.reorderComfortPins(ids)
|
.reorderComfortPins(ids)
|
||||||
.catch((e: unknown) => notifyActionError(e, t("home.shelf.moveFailed")))
|
.catch((e: unknown) => {
|
||||||
|
qc.setQueryData<DiscoverPayload>(key, (old) => (old ? { ...old, comfort: cards } : old));
|
||||||
|
notifyActionError(e, t("home.shelf.moveFailed"));
|
||||||
|
})
|
||||||
.finally(() => {
|
.finally(() => {
|
||||||
|
shelfSavingRef.current = false;
|
||||||
|
setShelfSaving(false);
|
||||||
// The shelf is also the pin menus' source of truth everywhere else in the app.
|
// The shelf is also the pin menus' source of truth everywhere else in the app.
|
||||||
void qc.invalidateQueries({ queryKey: qk.comfortPins() });
|
void qc.invalidateQueries({ queryKey: qk.comfortPins() });
|
||||||
// And the PREFIX, not this budget's key — which is what actually re-asks the server, and
|
// And the PREFIX, not this budget's key: the shelf is budget-independent server-side but
|
||||||
// is what the comment above this callback promises. Restoring the captured snapshot
|
// cached per budget, so without it the next chip click renders the pre-move order and a
|
||||||
// instead was wrong twice over: two overlapping moves each reverted to their own
|
// move from there writes it back. (The rollback above restores only `comfort`, so a rail
|
||||||
// snapshot, so the shelf settled on an order neither side ever held; and `before` is the
|
// refresh that landed meanwhile is kept.)
|
||||||
// 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.
|
|
||||||
void qc.invalidateQueries({ queryKey: qk.discover() });
|
void qc.invalidateQueries({ queryKey: qk.discover() });
|
||||||
});
|
});
|
||||||
},
|
},
|
||||||
@@ -742,6 +763,7 @@ export default function Home() {
|
|||||||
onToggleSave={onToggleSave}
|
onToggleSave={onToggleSave}
|
||||||
onToggleWatched={onToggleWatched}
|
onToggleWatched={onToggleWatched}
|
||||||
onMove={moveInShelf}
|
onMove={moveInShelf}
|
||||||
|
moveSaving={shelfSaving}
|
||||||
/>
|
/>
|
||||||
{!resumeConsumed && (
|
{!resumeConsumed && (
|
||||||
<Rail
|
<Rail
|
||||||
|
|||||||
@@ -38,7 +38,8 @@
|
|||||||
"moveLeft": "Left",
|
"moveLeft": "Left",
|
||||||
"moveRight": "Right",
|
"moveRight": "Right",
|
||||||
"moveLast": "To the end",
|
"moveLast": "To the end",
|
||||||
"moveFailed": "Could not reorder the shelf."
|
"moveFailed": "Could not reorder the shelf.",
|
||||||
|
"moveSaving": "Saving the new order…"
|
||||||
},
|
},
|
||||||
"pin": {
|
"pin": {
|
||||||
"menu": "Comfort shelf",
|
"menu": "Comfort shelf",
|
||||||
|
|||||||
@@ -38,7 +38,8 @@
|
|||||||
"moveLeft": "Balra",
|
"moveLeft": "Balra",
|
||||||
"moveRight": "Jobbra",
|
"moveRight": "Jobbra",
|
||||||
"moveLast": "A polc végére",
|
"moveLast": "A polc végére",
|
||||||
"moveFailed": "Nem sikerült átrendezni a polcot."
|
"moveFailed": "Nem sikerült átrendezni a polcot.",
|
||||||
|
"moveSaving": "Az új sorrend mentése…"
|
||||||
},
|
},
|
||||||
"pin": {
|
"pin": {
|
||||||
"menu": "Komfort polc",
|
"menu": "Komfort polc",
|
||||||
|
|||||||
Reference in New Issue
Block a user