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,