From 3741d1a46d4333d125908f0a4e5fb5ff0991632d Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 4 Oct 2026 02:48:10 +0200 Subject: [PATCH] fix(playlists): a list reopened from the cache keeps its rows Leaving the Playlists page and coming back painted the selected list empty ("0 videos") while the rail counted its rows. The page re-seeds its undoable order from the cached list in an effect; StrictMode runs that effect twice on the remount, so a reset was followed in the same tick by a rebase. Since E32 S3 a reset is state only and the rebase read the mirror ref, which still held the empty initial history, and wrote it back over the reset. rebase is now a state updater, so it builds on whatever an earlier call in the tick installed. Both tests seen RED (hook: [] for [10, 20]; page: no rows), and the bug reproduced in the browser on the old hook. --- .../src/components/Playlists.dom.test.tsx | 25 ++++++++++++++-- frontend/src/lib/useUndoable.dom.test.ts | 29 +++++++++++++++++++ frontend/src/lib/useUndoable.ts | 20 +++++-------- 3 files changed, 60 insertions(+), 14 deletions(-) create mode 100644 frontend/src/lib/useUndoable.dom.test.ts diff --git a/frontend/src/components/Playlists.dom.test.tsx b/frontend/src/components/Playlists.dom.test.tsx index eb1988aa..0254e75a 100644 --- a/frontend/src/components/Playlists.dom.test.tsx +++ b/frontend/src/components/Playlists.dom.test.tsx @@ -6,6 +6,7 @@ import { describe, it, expect, vi, afterEach, beforeAll, beforeEach } from "vitest"; import { render, screen, fireEvent, waitFor, cleanup, act } from "@testing-library/react"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { StrictMode } from "react"; const h = vi.hoisted(() => ({ notify: vi.fn(), @@ -112,7 +113,11 @@ const list = (id: number, items: Video[]) => ({ let qc: QueryClient; -function mount(lists: Record, prefs: Record = {}) { +function mount( + lists: Record, + prefs: Record = {}, + { strictOverCache = false } = {}, +) { vi.spyOn(api, "playlists").mockResolvedValue( Object.entries(lists).map(([id, items]) => list(Number(id), items)) as never, ); @@ -121,17 +126,22 @@ function mount(lists: Record, prefs: Record = ); qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); qc.setQueryData(qk.me(), { id: 1, can_write: true, prefs: {}, preferences: prefs }); + // Coming back to the page: the selected list is already cached, and StrictMode (dev) runs its + // effects twice on that mount. + if (strictOverCache && h.selectedId != null) + qc.setQueryData(qk.playlist(h.selectedId), list(h.selectedId, lists[h.selectedId] ?? [])); // The header portals into the shell's fixed toolbar band; without a slot it renders nothing, and // it went untested for exactly that reason. const slot = document.createElement("div"); document.body.appendChild(slot); - const tree = () => ( + const page = () => ( ); + const tree = () => (strictOverCache ? {page()} : page()); const view = render(tree()); return { rerender: () => view.rerender(tree()) }; } @@ -177,6 +187,17 @@ describe("Playlists — the row the player is launched with", () => { }); }); +describe("Playlists — coming back to the page", () => { + it("shows the cached list's rows, not an empty list", async () => { + // The seeding effect ran twice over the cached list (StrictMode): reset, then — same membership — + // rebase, which rebased the empty list from before the reset and wrote it back. The header read + // "0 videos" and the page said the list was empty while the rail counted its rows. + mount({ 1: [vid("A", 0), vid("B", 0)] }, {}, { strictOverCache: true }); + expect(await screen.findByRole("button", { name: "card.play — Video A" })).toBeTruthy(); + expect(screen.queryByText("playlists.empty")).toBeNull(); + }); +}); + describe("Playlists — removing a video from inside the player", () => { it("removes it from the list the player was launched from, even after the selection moved", async () => { // The selection can change under an open player (the command palette opens over it); the diff --git a/frontend/src/lib/useUndoable.dom.test.ts b/frontend/src/lib/useUndoable.dom.test.ts new file mode 100644 index 00000000..65fca73b --- /dev/null +++ b/frontend/src/lib/useUndoable.dom.test.ts @@ -0,0 +1,29 @@ +// @vitest-environment jsdom +// +// The undo container chains calls made in one tick: a `reset` not yet committed is what the next +// call builds on. A `rebase` read the mirror ref, which only follows a commit, so it rebased the +// value from BEFORE the reset and wrote that back — the playlist page, remounted under StrictMode +// over a cached list, re-ran its seeding effect (reset, then rebase) and painted an empty list. +import { describe, expect, it } from "vitest"; +import { act, renderHook } from "@testing-library/react"; +import { useUndoable } from "./useUndoable"; + +describe("useUndoable", () => { + it("rebases the value a reset in the same tick installed, not the one before it", () => { + const { result } = renderHook(() => useUndoable([])); + act(() => { + result.current.reset([1, 2]); + result.current.rebase((xs) => xs.map((x) => x * 10)); + }); + expect(result.current.value).toEqual([10, 20]); + }); + + it("keeps the undo history through a rebase", () => { + const { result } = renderHook(() => useUndoable([1])); + act(() => result.current.set([1, 2])); + act(() => result.current.rebase((xs) => xs.map((x) => x + 1))); + expect(result.current.value).toEqual([2, 3]); + act(() => result.current.undo()); + expect(result.current.value).toEqual([2]); + }); +}); diff --git a/frontend/src/lib/useUndoable.ts b/frontend/src/lib/useUndoable.ts index def30da1..d0766abd 100644 --- a/frontend/src/lib/useUndoable.ts +++ b/frontend/src/lib/useUndoable.ts @@ -81,7 +81,8 @@ export function useUndoable( onApplyRef.current?.(next); }, [commit, onApplyRef]); - // State only: safe to call during render. The other actions see it from the next commit on. + // State only: safe to call during render. `rebase` builds on it at once; `set`/`undo`/`redo` read + // the mirror, so they see it from the next commit on (no caller acts in the tick of a reset). const reset = useCallback((value: T) => { setH({ past: [], present: value, future: [] }); }, []); @@ -91,17 +92,12 @@ export function useUndoable( // removing a playlist item is not itself undoable, yet dropping it from each stored order keeps // the user's reorder history valid and usable instead of throwing it away (which is what a // `reset` does). No `onApply`: the caller is already persisting the change by its own route. - const rebase = useCallback( - (map: (value: T) => T) => { - const s = ref.current; - commit({ - past: s.past.map(map), - present: map(s.present), - future: s.future.map(map), - }); - }, - [commit], - ); + // An updater, not the mirror: a `reset` earlier in the same tick is state only, so the mirror + // still held the history before it, and rebasing that wrote the old value back over the reset (a + // cached playlist remounted under StrictMode re-seeds with reset-then-rebase and painted empty). + const rebase = useCallback((map: (value: T) => T) => { + setH((s) => ({ past: s.past.map(map), present: map(s.present), future: s.future.map(map) })); + }, []); return { value: h.present,