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.
This commit is contained in:
2026-10-04 02:48:10 +02:00
parent 98ec1abf25
commit 3741d1a46d
3 changed files with 60 additions and 14 deletions
+23 -2
View File
@@ -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<number, Video[]>, prefs: Record<string, unknown> = {}) {
function mount(
lists: Record<number, Video[]>,
prefs: Record<string, unknown> = {},
{ 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<number, Video[]>, prefs: Record<string, unknown> =
);
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 = () => (
<QueryClientProvider client={qc}>
<PageToolbarSlot.Provider value={slot}>
<Playlists />
</PageToolbarSlot.Provider>
</QueryClientProvider>
);
const tree = () => (strictOverCache ? <StrictMode>{page()}</StrictMode> : 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
+29
View File
@@ -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<number[]>([]));
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<number[]>([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]);
});
});
+8 -12
View File
@@ -81,7 +81,8 @@ export function useUndoable<T>(
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<T>(
// 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,