From 81ddd4e5ca1f241a1133ec545538e475aaf8fc79 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 10 Jul 2026 23:25:42 +0200 Subject: [PATCH] fix(spa): gate mutation controls + add-all on stale result sets during refetch (fixes #221) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Search and Media browse keep the previous successful result set rendered during a refetch (query on Search; kind/query/page on Media browse) with no gating, so per-card Add-to, Select/select-mode, the selection action bar, Add all, and Save-as-smart-collection stayed live over stale, about-to-be-replaced items. Worst path: SearchScreen.addAll only checked activeRef, so a late GET /api/search/all-items could open a bulk-add dialog scoped to the previous query's entire result set. Key the success state to the request params that produced it and derive a `refreshing` flag; while refreshing, keep cards visible but disable every mutation surface, show a "Refreshing…" cue, and dim the grid. Card navigation stays live. Bind addAll's completion to its query via lastQueryRef so a stale all-items result is discarded. Same pattern applied to both screens. Docs: spa-conventions §3a (refreshing/gating pattern) + §8 (temporal-semantics review checklist); blazor-route-parity search/media-browse verdicts. Co-Authored-By: Claude Fable 5 --- docs/blazor-route-parity.md | 12 ++- docs/spa-conventions.md | 42 +++++++++ web/src/screens/MediaBrowseScreen.test.tsx | 47 ++++++++++ web/src/screens/MediaBrowseScreen.tsx | 41 +++++++-- web/src/screens/SearchScreen.test.tsx | 101 +++++++++++++++++++++ web/src/screens/SearchScreen.tsx | 46 ++++++++-- web/src/shell.css | 8 ++ 7 files changed, 280 insertions(+), 17 deletions(-) diff --git a/docs/blazor-route-parity.md b/docs/blazor-route-parity.md index bb617695a..e45d238ef 100644 --- a/docs/blazor-route-parity.md +++ b/docs/blazor-route-parity.md @@ -64,9 +64,9 @@ redirect). > | Collections | PARITY-OK (custom-order endpoint + reorder UI, all-10-kind add picker, 2026-07-09) | — | > | Channel editor | GAPS (external logo URL, bare create, pickers) | #212 | > | Channels-numbers / Logs | PARITY-OK / minors | #213 | -> | Search | PARITY-OK (card nav, per-card/multi-select add-to, add-all, save-as-smart-collection, 2026-07-10) | — | +> | Search | PARITY-OK (card nav, per-card/multi-select add-to, add-all, save-as-smart-collection, 2026-07-10; mutation controls + Add-all now gated during refetch — #221) | — | > | Media browse/detail (read paths + image browser) | PARITY-OK | — | -> | Media browse/detail (mutations, per-show scan, episode info/troubleshoot) | PARITY-OK (shared Add-to layer + scan/info/troubleshoot wired, 2026-07-10) | — | +> | Media browse/detail (mutations, per-show scan, episode info/troubleshoot) | PARITY-OK (shared Add-to layer + scan/info/troubleshoot wired, 2026-07-10; mutation controls gated on kind/query/page refetch — #221) | — | > | Schedules editors | **disproven — moved to Section 3** | **#207** | > | Media sources | **disproven — moved to Section 3** | **#202** | > @@ -192,6 +192,14 @@ query-wide Add All via `GET /api/search/all-items`, Save As Smart Collection) an pages + child grids, per-show Quick/Deep scan gated to Plex/Jellyfin/Emby, per-episode Media Info + Troubleshoot entries; `POST /api/playlists/{id}/items` added). Known accepted deviations (select-mode toggle, per-card target superset) recorded in `docs/decisions.md`. +CLOSED 2026-07-10: **#221** (adversarial-reviewer#18 follow-up to #208/#209) — those PRs added +mutation actions to two screens whose fetch model keeps the previous result set rendered during a +refetch. On Search and Media browse the per-card Add-to menu, Select/select-mode, selection action +bar, Add-all, and Save-as-smart-collection are now **gated while a refetch is in flight** (query on +Search; kind/query/page on Media browse), with a visible "Refreshing…" cue and dimmed grid; card +navigation stays live. `SearchScreen.addAll` also binds its completion to the requesting query so a +late `GET /api/search/all-items` can no longer open a bulk-add dialog scoped to the previous query. +See `docs/spa-conventions.md` §3a for the pattern. ## Section 4 — Blazor home / escape hatch diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index 3e4b29422..65036d80f 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -60,6 +60,35 @@ fetches from the API: `eslint-plugin-react-hooks` in `web/eslint.config.js` — a synchronous `setState` in an effect body will fail `npm run lint`. +## 3a. "Keep results visible during refetch" ⇒ gate mutations + show a refreshing cue + +Some grid screens deliberately keep the **previous** successful result set rendered while a refetch +is in flight (no full-screen loading state on a query/kind/page change), so the grid doesn't flash +empty. `SearchScreen.tsx` and `MediaBrowseScreen.tsx` do this. If such a screen also carries +**mutation surfaces** (per-card Add-to menu, Select/select-mode, a selection action bar, "Add all", +"Save as smart collection"), those surfaces would otherwise stay live over a **stale** result set — +an add/select action then targets the about-to-be-replaced items, or (worse) a query-wide "Add all" +bulk request resolves against the previous query. This was issue #221 (adversarial-reviewer#18). + +Convention — when a screen keeps stale results visible during a refetch: + +- **Key the success state to the request params that produced it.** Store the identifying params on + the `status: 'success'` variant (`SearchScreen`: the `query`; `MediaBrowseScreen`: a + `kind|query|page` `key`), set in the seq-guarded `.then`. Derive + `const refreshing = state.status === 'success' && state. !== ;` in render. + Prefer this over a synchronously-set `refreshing` flag: setting state synchronously from the load + path trips the `react-hooks` "no set-state-in-effect" rule (§3), and a param-keyed derivation is + self-correcting (it can never get stuck true/false). +- **While `refreshing`:** show a visible cue (a `role="status"` "Refreshing…" row with `` + plus the `.ctv-media-grid-dim` opacity class on the grid) and **disable every mutation surface** — + per-card Add-to menu (withhold the `actions` node), select toggle + in-grid selection + (`const canSelect = selectMode && !refreshing;` gates `onToggleSelect`), the selection action bar, + "Add all", "Save as smart collection". Card navigation (`onOpen`) **may** stay live. +- **Bind async bulk completions to their request params, not just mount.** A whole-query/whole-set + request (e.g. `getSearchAllItems`) must, on resolve, check that its snapshotted params are still + current (compare against a ref that always holds the committed value — `SearchScreen` reuses + `lastQueryRef`) and **discard** otherwise. Checking only `activeRef` (mounted) is insufficient. + ## 4. API client modules One file per domain in `web/src/api/`, e.g. `logs.ts`, `blocks.ts`, `playouts.ts`. Pattern (see @@ -145,3 +174,16 @@ npm run build # tsc -b && vite build Also run `npm run check:api` if you touched anything OpenAPI-relevant (see `api-conventions.md` §5) — it regenerates `src/api/generated/v1.d.ts` and fails the build if it's out of sync with what's committed. + +## 8. Review checklist — temporal semantics + +- **For every effect / timer / async completion, ask: *when* does it fire (mount, dependency change, + unmount, StrictMode double-invoke) and *which* render/request does it still own?** A debounce timer + fires on mount too (§3, the `lastQueryRef` no-change guard exists precisely for that); a `.then` + can resolve after the params it was launched for have moved on (§3a, the `refreshing` gate and the + Add-all query binding exist for that). A guard that only checks "still mounted" (`activeRef`) does + not answer "still current". +- **For any "make X consistent with Y" change, re-validate the exemplar Y's temporal behavior before + copying it.** #221 came from copying a fetch model that keeps stale results visible onto screens + that had gained mutation surfaces — the exemplar was safe read-only, the copy was not. Copying a + pattern copies its *assumptions*; confirm they still hold in the new context. diff --git a/web/src/screens/MediaBrowseScreen.test.tsx b/web/src/screens/MediaBrowseScreen.test.tsx index 50812c246..bec84464c 100644 --- a/web/src/screens/MediaBrowseScreen.test.tsx +++ b/web/src/screens/MediaBrowseScreen.test.tsx @@ -130,6 +130,53 @@ describe('MediaBrowseScreen', () => { }); }); + it('gates old-kind cards and shows a refreshing cue during a kind-change refetch (issue #221)', async () => { + function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((r) => { + resolve = r; + }); + return { promise, resolve }; + } + + const browseDeferreds: Array<{ resolve: (body: unknown) => void }> = []; + vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL) => { + const url = input.toString(); + if (url.startsWith('/api/library/browse')) { + const d = deferred(); + browseDeferreds.push({ resolve: (body) => d.resolve(jsonResponse(body)) }); + return d.promise; + } + if (url === '/api/collections') { + return Promise.resolve(jsonResponse([{ collectionType: 'Collection', id: 7, name: 'Favorites', useCustomPlaybackOrder: false }])); + } + if (url === '/api/playlists/groups') { + return Promise.resolve(jsonResponse([])); + } + return Promise.resolve(new Response(null, { status: 204 })); + }); + + render(); + browseDeferreds[0].resolve({ page: items, totalCount: items.length }); + await waitFor(() => expect(screen.getByText('Blade Runner')).toBeInTheDocument()); + expect(screen.getAllByRole('button', { name: 'Add to…' })).toHaveLength(items.length); + + // Change kind: the refetch for the new kind is held pending while old-kind cards stay rendered. + fireEvent.change(screen.getByRole('combobox'), { target: { value: 'shows' } }); + await waitFor(() => expect(browseDeferreds.length).toBe(2)); + + // Refreshing window: cue visible, mutation surfaces gated, old-kind cards still visible but inert. + await waitFor(() => expect(screen.getByText('Refreshing…')).toBeInTheDocument()); + expect(screen.getByText('Blade Runner')).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: 'Add to…' })).toBeNull(); + expect(screen.getByRole('button', { name: 'Select' })).toBeDisabled(); + + // New-kind results resolve: controls re-enable, cue gone. + browseDeferreds[1].resolve({ page: items, totalCount: items.length }); + await waitFor(() => expect(screen.queryByText('Refreshing…')).not.toBeInTheDocument()); + expect(screen.getAllByRole('button', { name: 'Add to…' })).toHaveLength(items.length); + }); + it('selects every loaded item with Select all on page', async () => { mockFetch(); render(); diff --git a/web/src/screens/MediaBrowseScreen.tsx b/web/src/screens/MediaBrowseScreen.tsx index 876b9b7cf..70046957d 100644 --- a/web/src/screens/MediaBrowseScreen.tsx +++ b/web/src/screens/MediaBrowseScreen.tsx @@ -55,10 +55,17 @@ function kindFromSlug(slug: string | null): MediaKind { } type BrowseState = - | { items: LibraryBrowseItem[]; error: null; status: 'success'; totalCount: number } + // `key` records the request params (kind + query + page) that produced this result set. When it + // no longer matches the current params, the visible items are stale (a refetch is in flight) — + // see `refreshing` below. + | { items: LibraryBrowseItem[]; error: null; status: 'success'; totalCount: number; key: string } | { items: []; error: string; status: 'error'; totalCount: 0 } | { items: []; error: null; status: 'loading'; totalCount: 0 }; +function browseKeyOf(mediaType: LibraryBrowseMediaType, query: string, pageNum: number): string { + return `${mediaType}\u0000${query}\u0000${pageNum}`; +} + type Notice = { tone: 'ok' | 'error'; message: string }; export function MediaBrowseScreen() { @@ -108,10 +115,11 @@ export function MediaBrowseScreen() { const load = useCallback(() => { const id = ++seqRef.current; + const key = browseKeyOf(kind.mediaType, query, pageNum); getLibraryBrowseItems({ mediaType: kind.mediaType, query: query || undefined, pageNum, pageSize: PAGE_SIZE }) .then((paged) => { if (activeRef.current && id === seqRef.current) { - setState({ items: paged.page ?? [], error: null, status: 'success', totalCount: paged.totalCount ?? 0 }); + setState({ items: paged.page ?? [], error: null, status: 'success', totalCount: paged.totalCount ?? 0, key }); } }) .catch((error: unknown) => { @@ -185,6 +193,13 @@ export function MediaBrowseScreen() { const selectedItems = Array.from(selected.values()); const totalPages = state.status === 'success' ? Math.max(1, Math.ceil(state.totalCount / PAGE_SIZE)) : 1; + // A refetch is in flight when the currently-rendered items were produced by different params than + // the current kind/query/page (items stay visible during refetch — see `load`). While refreshing we + // keep items visible but gate every mutation surface and show a refreshing cue, so no add/select + // action is scoped to the stale, about-to-be-replaced result set (issue #221). + const refreshing = state.status === 'success' && state.key !== browseKeyOf(kind.mediaType, query, pageNum); + const canSelect = selectMode && !refreshing; + return (
@@ -213,6 +228,7 @@ export function MediaBrowseScreen() { )}
)} + {refreshing && ( +
+ + Refreshing… +
+ )} + {!hasQuery && (
Type a query to search across every media kind.
@@ -344,21 +372,23 @@ export function SearchScreen() { )}
-
+
{data.items.map((item) => { const key = itemKey(item); const detailPath = mediaDetailPath(item); return ( setNotice({ tone: 'ok', message })} /> ) } item={item} key={key} - onOpen={!selectMode && detailPath ? () => navigateToPath(detailPath) : undefined} - onToggleSelect={selectMode ? toggleSelect : undefined} + onOpen={!canSelect && detailPath ? () => navigateToPath(detailPath) : undefined} + onToggleSelect={canSelect ? toggleSelect : undefined} selected={selectMode ? selected.has(key) : undefined} /> ); diff --git a/web/src/shell.css b/web/src/shell.css index f51cc1482..4a1fee71d 100644 --- a/web/src/shell.css +++ b/web/src/shell.css @@ -2509,6 +2509,14 @@ body { gap: var(--space-6, 12px); } +/* Dim the stale result set while a refetch is in flight (search/media browse — issue #221). The + per-card mutation menu is withheld in the same state; navigation stays live so no pointer-events + change here. */ +.ctv-media-grid-dim { + opacity: 0.5; + transition: opacity var(--dur-fast, 120ms) var(--ease-standard, ease); +} + .ctv-media-card { position: relative; border-radius: var(--radius-sm);