fix(playlists): a refused edit is taken back on screen, and a gone list or tag is said, not swallowed

Deep-review round 4, the playlist surfaces.

- A failed move or reorder relied on the resync to put the rows back — but the server's list is
  unchanged, so the refetch comes back deep-equal, react-query keeps the same `data` object, and the
  re-seed effect keyed on it never ran: the moved row stayed gone, the rejected order stayed on
  screen. Both restore from the cached detail now.
- A failed removal puts its row back itself: when the server is unreachable the resync fails too,
  and round 3's "keep the list on a failed refetch" guard then kept showing it as removed.
- The Plex playlist page (remove / rename / delete / reorder), the add-to-playlist popover and the
  tag manager had no failure handling at all: a list or tag deleted elsewhere (a 404, now a
  structured `not_found` the request layer leaves to the caller) failed in silence and an
  optimistic removal stayed removed. Each says it and resyncs; deleting a Plex playlist that is
  already gone just leaves the page.
- Playlists' own loading/error branches use the shared `LoadingState` / `StateMessage` (the loading
  line had no live region).
- The "player stays mounted" tests count mounts: a remount ended with the same flag as staying up.
This commit is contained in:
2026-09-11 23:34:33 +02:00
parent 6d49da17fe
commit 267fd7f70f
12 changed files with 309 additions and 37 deletions
+73 -3
View File
@@ -11,6 +11,7 @@ const h = vi.hoisted(() => ({
notify: vi.fn(),
selectedId: 1 as number | null,
mounted: false,
mounts: 0,
player: null as null | {
video: { id: string; position_seconds: number };
onClose: () => void;
@@ -43,6 +44,7 @@ vi.mock("./PlayerModal", async () => {
h.player = props;
useEffect(() => {
h.mounted = true;
h.mounts++;
return () => {
h.mounted = false;
};
@@ -95,7 +97,7 @@ const list = (id: number, items: Video[]) => ({
let qc: QueryClient;
function mount(lists: Record<number, Video[]>) {
function mount(lists: Record<number, Video[]>, prefs: Record<string, unknown> = {}) {
vi.spyOn(api, "playlists").mockResolvedValue(
Object.entries(lists).map(([id, items]) => list(Number(id), items)) as never,
);
@@ -103,7 +105,7 @@ function mount(lists: Record<number, Video[]>) {
async (id: number) => list(id, lists[id] ?? []) as never,
);
qc = new QueryClient({ defaultOptions: { queries: { retry: false } } });
qc.setQueryData(qk.me(), { id: 1, can_write: true, prefs: {} });
qc.setQueryData(qk.me(), { id: 1, can_write: true, prefs: {}, preferences: prefs });
const tree = () => (
<QueryClientProvider client={qc}>
<Playlists />
@@ -120,6 +122,7 @@ beforeEach(() => {
h.selectedId = 1;
h.player = null;
h.mounted = false;
h.mounts = 0;
h.notify.mockClear();
});
afterEach(() => {
@@ -248,8 +251,12 @@ describe("Playlists — the player outlives a change of selection", () => {
vi.mocked(api.playlist).mockReturnValue(new Promise(() => {}));
h.selectedId = 2;
rerender();
await screen.findByText("playlists.loading");
// The shared loading state, which a screen reader hears (the old bare line had no live region).
expect(
(await screen.findByText("playlists.loading")).closest('[role="status"]'),
).not.toBeNull();
expect(h.mounted).toBe(true);
expect(h.mounts).toBe(1); // never went down — a remount would also end `mounted`
});
it("stays mounted when the newly selected list fails to load — a list deleted elsewhere", async () => {
@@ -261,6 +268,7 @@ describe("Playlists — the player outlives a change of selection", () => {
rerender();
await screen.findByText("playlists.loadFailed");
expect(h.mounted).toBe(true);
expect(h.mounts).toBe(1);
});
it("stays mounted when nothing is selected any more", async () => {
@@ -271,6 +279,7 @@ describe("Playlists — the player outlives a change of selection", () => {
rerender();
await screen.findByText("playlists.pickOne");
expect(h.mounted).toBe(true);
expect(h.mounts).toBe(1);
});
it("refreshes the list it was launched from when the viewer marks the video in it", async () => {
@@ -289,3 +298,64 @@ describe("Playlists — the player outlives a change of selection", () => {
expect(qc.getQueryState(qk.playlist(1))?.isInvalidated).toBe(true);
});
});
const rowsOnScreen = () =>
screen
.getAllByRole("button", { name: /^card\.play — / })
.map((b) => b.getAttribute("aria-label")?.replace("card.play — ", ""));
async function rowMenu(title: string, item: string) {
const row = (await screen.findByRole("button", { name: `card.play — ${title}` })).closest("tr")!;
fireEvent.contextMenu(row);
await waitFor(() =>
expect(
screen.getAllByRole("menuitem", { hidden: true }).some((b) => b.textContent === item),
).toBe(true),
);
fireEvent.click(
screen.getAllByRole("menuitem", { hidden: true }).find((b) => b.textContent === item)!,
);
}
describe("Playlists — a change the server refused is taken back on screen", () => {
// The resync refetch returns the SAME list the server had, react-query keeps the same `data`
// object for a deep-equal answer, and the page's re-seed effect is keyed on that object — so the
// optimistic change used to stay on screen for good.
it("puts back a row whose move failed", async () => {
vi.spyOn(api, "moveToPlaylist").mockRejectedValue(
new HttpError(404, "gone", { code: "not_found", reason: "move_target" }),
);
mount({ 1: [vid("V", 0), vid("W", 0)], 2: [] }, { lastMovePlaylistId: 2 });
await rowMenu("Video V", "playlists.moveToRecent");
await waitFor(() =>
expect(h.notify).toHaveBeenCalledWith(expect.objectContaining({ level: "error" })),
);
await act(async () => void (await new Promise((r) => setTimeout(r, 20))));
expect(rowsOnScreen()).toEqual(["Video V", "Video W"]);
});
it("puts back the order when saving a reorder failed", async () => {
vi.spyOn(api, "reorderPlaylist").mockRejectedValue(new HttpError(500, "boom"));
mount({ 1: [vid("V", 0), vid("W", 0)] });
await rowMenu("Video V", "playlists.orderDown");
await waitFor(() => expect(api.reorderPlaylist).toHaveBeenCalled());
await act(async () => void (await new Promise((r) => setTimeout(r, 20))));
expect(rowsOnScreen()).toEqual(["Video V", "Video W"]);
});
it("puts back a row whose removal failed, even when the resync fails too", async () => {
// A removal during an API restart: the DELETE fails, and so does the refetch meant to put the row
// back — the list kept showing it as removed while it was still in the playlist.
vi.spyOn(api, "removeFromPlaylist").mockRejectedValue(new TypeError("Failed to fetch"));
mount({ 1: [vid("V", 0), vid("W", 0)] });
await screen.findByRole("button", { name: "card.play — Video V" });
vi.mocked(api.playlist).mockRejectedValue(new TypeError("Failed to fetch"));
await rowMenu("Video V", "playlists.removeItem");
await waitFor(() => expect(api.removeFromPlaylist).toHaveBeenCalled());
await act(async () => void (await new Promise((r) => setTimeout(r, 20))));
expect(rowsOnScreen()).toEqual(["Video V", "Video W"]);
expect(h.notify).toHaveBeenCalledWith(
expect.objectContaining({ level: "error", message: "playlists.removeFailed" }),
);
});
});