From e7e425fa252d601e20ec04ee51598467f9da3b32 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 23:58:57 +0200 Subject: [PATCH] =?UTF-8?q?fix(651):=20review=20round=201=20=E2=80=94=20ne?= =?UTF-8?q?ver=20clear=20an=20unnamed=20id,=20complete=20the=20Lucene=20es?= =?UTF-8?q?caping,=20keyboard-operable=20picker?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cold cross-family review of 57aefcdf. Six findings, all web-only. F1 (HIGH, data-loss shaped): RerunCollectionsController.ProjectToResponseModel derives BOTH selectedId and selectedName from the same eager-loaded navigation, and GetRerunCollectionByIdHandler loads media metadata only for Show/Season/Artist/Movie while MediaCollections/Mapper maps RemoteStream through `_ => null`. So opening a RemoteStream rerun collection returned HTTP 200 with a null selection and the edit-load refresh CLEARED a stored id, leaving Save permanently disabled. The refresh now merges instead of replacing, so no path can clear an id it merely failed to name; the label degrades to `#id`. Covered per affected type — RemoteStream, Episode, MusicVideo, Song, OtherVideo, Image — plus a re-save assertion. The read-model gaps themselves are server-side and are NOT touched here. F2: `&` and `|` were missing from the escaped set, so `Rock && Roll` compiled with the boolean operator live. Pre-existing in Auto-Tune's original helper, but propagated to three more pickers — and now fixed for Auto-Tune too, since the helper is shared. The test that claimed to cover "every Lucene special" carried its own hand-copied sample and could not see its own omissions; it is now driven per-character off an exported LIBRARY_PICKER_LUCENE_SPECIALS. F3: a slow edit-load name resolution could relabel a newer selection. The label is now keyed to the id it was resolved for AND refuses to overwrite a label naming a different id — keying the render alone stops the mislabelling but discards the correct new label. F4: searchLibraryPickerOptions clamps pageSize instead of merely defaulting it. A bound a caller can exceed is not a bound. F6: replacing a native `. The other half of the superseded record is UNCHANGED: bounded-by-construction admin lists (collections, multi-collections, smart collections, playlists) still page to completeness via `loadAllPages` and still report `complete`/`hint: incomplete`. Server-side caps are not raised — this is a web-only change. | 2026-07-26 | [link](records/spa/library-pickers-resolve-by-search.md) | | `spa.logs-page-size-local` | The Logs page rows-per-page preference is stored in `window.localStorage` (`ctv-logs-page-size`), not a server `ConfigElement`. | 2026-07-11 | [link](records/spa/logs-page-size-local.md) | | `spa.playback-troubleshoot-poll` | The playback-troubleshooting screen reports FFmpeg completion by polling `GET /api/troubleshoot/playback/status` (~2s) rather than a server push channel. | 2026-07-09 | [link](records/spa/playback-troubleshoot-poll.md) | | `spa.playout-reset-button` | The SPA keeps a single Reset action (server picks the default build mode) and drops Blazor's separate "Schedule reset" button since its capability already exists via the playout's Edit-details flow. | 2026-07-09 | [link](records/spa/playout-reset-button.md) | diff --git a/docs/decisions/records/spa/library-pickers-resolve-by-search.md b/docs/decisions/records/spa/library-pickers-resolve-by-search.md index e6ceb9b77..79e61ada3 100644 --- a/docs/decisions/records/spa/library-pickers-resolve-by-search.md +++ b/docs/decisions/records/spa/library-pickers-resolve-by-search.md @@ -5,8 +5,8 @@ status: active since: '2026-07-26' supersedes: spa.list-completeness-vs-bounded-pickers@2026-07-26 superseded-by: none -rule: 'A picker over a media-library table (Episode/Song/Image/Movie/MusicVideo/TelevisionShow/TelevisionSeason/Artist/OtherVideo/RemoteStream) resolves its options by SEARCH — a debounced `SearchPicker` calling `searchLibraryPickerOptions`, which issues at most ONE `getLibraryBrowseItems` request per settled query, bounded to `LIBRARY_PICKER_RESULTS` (25) rows and gated on `LIBRARY_PICKER_MIN_QUERY` (2) characters. It list-loads NOTHING on mount or on a type switch, so there is no truncation to surface and no truncation hint. The typed text is COMPILED (`titleContainsQuery` → `title:**`), never forwarded raw. The current selection renders from the OWNING RECORD, not from the result set (`selectedName` on a rerun collection / playlist item; a single by-id detail read — `getShow`/`getSeason`/`getArtist` — for a filler preset, which stores only the id), so editing an existing record can never lose or fail to name its selection. The other half of the superseded record is UNCHANGED: bounded-by-construction admin lists (collections, multi-collections, smart collections, playlists) still page to completeness via `loadAllPages` and still report `complete`/`hint: incomplete`. Server-side caps are not raised — this is a web-only change.' -signals: 'library picker typeahead, SearchPicker, searchLibraryPickerOptions, titleContainsQuery, LIBRARY_PICKER_RESULTS, LIBRARY_PICKER_MIN_QUERY, compile typed text not raw Lucene, picker truncation hint removed, loadAllPages Class A, LuceneSearchIndex.Search hitsLimit, useIsMountedRef · paths: `web/src/api/libraryBrowse.ts`, `web/src/schedules/pickers.tsx`, `web/src/hooks.ts`, `web/src/screens/RerunCollectionsScreen.tsx`, `web/src/screens/PlaylistsScreen.tsx`, `web/src/screens/FillerPresetsScreen.tsx`, `web/src/api/paging.ts`, `docs/spa-conventions.md` §3b · issues: #651, #644, #578, #440' +rule: 'A picker over a media-library table (Episode/Song/Image/Movie/MusicVideo/TelevisionShow/TelevisionSeason/Artist/OtherVideo/RemoteStream) resolves its options by SEARCH — a debounced `SearchPicker` calling `searchLibraryPickerOptions`, which issues at most ONE `getLibraryBrowseItems` request per settled query, bounded to `LIBRARY_PICKER_RESULTS` (25) rows — CLAMPED inside the helper, not merely defaulted — and gated on `LIBRARY_PICKER_MIN_QUERY` (2) characters. It list-loads NOTHING on mount or on a type switch, so there is no truncation to surface and no truncation hint. The typed text is COMPILED (`titleContainsQuery` → `title:**`), never forwarded raw. The current selection renders from the OWNING RECORD, not from the result set (`selectedName` on a rerun collection / playlist item; a single by-id detail read — `getShow`/`getSeason`/`getArtist` — for a filler preset, which stores only the id), and an edit-load refresh MERGES rather than replaces so a response that fails to NAME a selection can never CLEAR its id; an asynchronously-resolved name is keyed to the id it was resolved for and never overwrites a label naming a different id. The typeahead implements the full ARIA combobox keyboard contract, because it replaces a natively keyboard-operable `` is fully +keyboard-operable, so an input-plus-listbox that only responds to Tab and click is a regression +introduced by this change rather than a pre-existing gap. `SearchPicker` implements the ARIA +combobox pattern: `role="combobox"` with `aria-expanded`/`aria-controls`/`aria-autocomplete`, +Arrow/Home/End moving a virtual cursor exposed through `aria-activedescendant`, Enter committing, +Escape dismissing, and options as non-tab-stops. The cursor resets whenever a new result set lands. + Folded in from #578 (same components): the rule-builder facet typeahead arms on **focus** rather than on mount, so an N-row rule tree no longer fires N unrequested `search/fields/*/values` requests; and both it and `SearchPicker` now pair their `seqRef` stale-response guard with a shared -`useIsMountedRef` (`web/src/hooks.ts`) so a fetch resolving after unmount is dropped. #578's third -item — extending the `artist` facet source beyond entity artists — is a `GetSearchFieldValuesHandler` -change, out of scope for a web-only fix, and stays open. +`useIsMountedRef` (`web/src/hooks.ts`) so a fetch resolving after unmount is dropped. Proving that +guard needs two tests, because React 19 no longer warns on a setState-after-unmount and an unmounted +tree renders nothing either way: a unit test of the hook (including a StrictMode double-invoke for +the re-arm) plus an integration test that mocks the hook module and asserts `SearchPicker` actually +read `current` and saw `false`. #578's remaining item — extending the `artist` facet source beyond +entity artists — is a `GetSearchFieldValuesHandler` change, out of scope for a web-only fix, and is +being done on its own branch. + +*(Over the 60-line prose ceiling at 84: checked for redundancy against +`spa.list-completeness-vs-bounded-pickers` in `archive/` and declined to cut. The length is six +distinct findings — the search bound, the compile rule, selection preservation, the +clear-what-you-cannot-name prohibition, the async-name keying, and the keyboard contract — four of +which came from review rounds and each of which names a specific way the obvious implementation is +wrong. Summarising any of them back out would lose the counter-example that makes it actionable.)* diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index 16f6394ba..6fd5223ff 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -187,13 +187,34 @@ the #644 follow-up got Class A right and Class B only half right): `title:**`. The index's default field does not match bare title words (`Alpha` finds nothing for "Show Alpha" — see `e2e-local.md`), so a raw forward looks broken in a *name* picker. Reuse the helper; do not re-implement the escaping (same rule as the #440 Auto-Tune typeahead, - same shape `builder/rules/compile.ts` emits for `contains`). + same shape `builder/rules/compile.ts` emits for `contains`). The escaped set includes `&` and + `|`, because Lucene's boolean operators are `&&`/`||` and a title like `Rock & Roll` otherwise + compiles to something Lucene parses as syntax. **Drive the escaping test from the exported + character set** (`LIBRARY_PICKER_LUCENE_SPECIALS`), one character per case — a test carrying its + own hand-copied "every special" sample cannot see what is missing from that sample. + - **The bound belongs to the helper, not the caller.** `searchLibraryPickerOptions` *clamps* + `pageSize` to `LIBRARY_PICKER_RESULTS`; a documented bound a caller can exceed by passing a + bigger number is not a bound. - **Render the current selection from the owning record, not from the result set.** An item already selected but outside the current results must still display — losing it on edit is data loss, not a cosmetic defect. Rerun collections and playlist items carry `selectedName` on their own DTOs; `FillerPresetFullResponseModel` stores only an id, so its edit path resolves the name with a single by-id detail read (`getShow`/`getSeason`/`getArtist`) and degrades to `#id` on failure — never to a cleared field. + - **Never let a refresh CLEAR an id it merely failed to NAME.** A read model that derives both the + id and the name from the same eager-loaded navigation returns *no selection at all* when the + navigation isn't loaded — a successful 200 that looks identical to "the user cleared it". + (`RerunCollectionsController.ProjectToResponseModel` does exactly this, and + `MediaCollections/Mapper` maps RemoteStream through `_ => null`.) So an edit-load refresh + **merges** rather than replaces: `selectedId: refreshed.selectedId ?? current.selectedId`, + `selectedName: refreshed.selectedName || current.selectedName`. Degrading the label to `#id` is + acceptable; dropping the id strands the user on a record they cannot re-save, because Save is + gated on it. Test this per affected type, not on the one type that happens to work. + - **A name resolved asynchronously must be keyed to the id it was resolved FOR**, and must not + overwrite a label that already names a different id. A slow by-id read that lands after the user + has picked something else would otherwise label the new selection with the old item's title + while the id — and therefore what gets saved — says otherwise. Both halves are needed: keying + the render alone stops the mislabelling but still discards the newer, correct label. - **Only Lucene-backed types.** `GetLibraryBrowseItemsHandler` applies `query` as a Lucene clause for media items but as a plain SQL `LIKE` on `Name` for the collection-family types (Collection / SmartCollection / MultiCollection / RerunCollection / Playlist). A compiled `title:*x*` sent at @@ -213,6 +234,22 @@ facet lookups before #578); arm the effect on the input's `onFocus` instead. And callback — `seqRef` drops an *older* response, but says nothing about whether the component still exists. +**A custom picker replacing a native control owes you its keyboard behaviour.** A `` elements when that is what you mean. + +**Testing an is-mounted guard: React 19 does not warn on a setState-after-unmount, and an unmounted +tree renders nothing either way** — so no DOM assertion can distinguish "the guard stopped it" from +"React discarded it". Prove the *mechanism* (a `useIsMountedRef` unit test, with a StrictMode +double-invoke for the re-arm) **and** the *integration* (mock the hook module and assert the +component actually read `current` — and saw `false` — when the late response landed). Verify each by +removing the mechanism and confirming the test fails. + ## 4. API client modules One file per domain in `web/src/api/`, e.g. `logs.ts`, `blocks.ts`, `playouts.ts`. Pattern (see diff --git a/web/src/api/libraryBrowse.test.ts b/web/src/api/libraryBrowse.test.ts index 821552975..e2b16ff05 100644 --- a/web/src/api/libraryBrowse.test.ts +++ b/web/src/api/libraryBrowse.test.ts @@ -3,6 +3,7 @@ import { getLibraryBrowseItems, searchLibraryPickerOptions, titleContainsQuery, + LIBRARY_PICKER_LUCENE_SPECIALS, LIBRARY_PICKER_RESULTS } from './libraryBrowse'; @@ -67,11 +68,33 @@ describe('titleContainsQuery (#651 — compile typed text, never forward raw Luc expect(titleContainsQuery('Show Alpha')).toBe('title:*Show\\ Alpha*'); }); - it('escapes every Lucene special AND whitespace so the boundary stars are the only live wildcards', () => { - // Same shape builder/rules/compile.ts emits for `contains`. - expect(titleContainsQuery('a+b-c!d(e)f{g}h[i]j^k"l~m*n?o:p\\q/r s')).toBe( - 'title:*a\\+b\\-c\\!d\\(e\\)f\\{g\\}h\\[i\\]j\\^k\\"l\\~m\\*n\\?o\\:p\\\\q\\/r\\ s*' - ); + // The previous version of this test hand-copied a sample string and claimed to cover "every + // Lucene special" — it silently omitted `&` and `|`, and a completeness test that carries its own + // list of what to check cannot see what is missing from that list (#651 F2). Drive the assertion + // from the exported character set instead, one character at a time, so adding a character to the + // set without escaping it fails here. + it.each(LIBRARY_PICKER_LUCENE_SPECIALS.split(''))('escapes the Lucene special %j', (char) => { + expect(titleContainsQuery(`a${char}b`)).toBe(`title:*a\\${char}b*`); + }); + + it.each([' ', '\t', '\n'])('escapes whitespace %j so it cannot split the term', (char) => { + expect(titleContainsQuery(`a${char}b`)).toBe(`title:*a\\${char}b*`); + }); + + it('leaves every character that is NOT special untouched', () => { + const plain = 'abcXYZ019_,.\'@#$%'; + for (const char of plain) { + expect(LIBRARY_PICKER_LUCENE_SPECIALS).not.toContain(char); + } + expect(titleContainsQuery(plain)).toBe(`title:*${plain}*`); + }); + + it('neutralises the && and || BOOLEAN operators, not just single characters (#651 F2)', () => { + // The regression: `Rock && Roll` used to compile with `&&` live, so Lucene parsed it as boolean + // syntax (or rejected the query) and an exactly-matching title returned nothing. + expect(titleContainsQuery('Rock && Roll')).toBe('title:*Rock\\ \\&\\&\\ Roll*'); + expect(titleContainsQuery('A || B')).toBe('title:*A\\ \\|\\|\\ B*'); + expect(titleContainsQuery('Rock & Roll')).toBe('title:*Rock\\ \\&\\ Roll*'); }); it('leaves a plain single word alone apart from the boundary stars', () => { @@ -110,6 +133,15 @@ describe('searchLibraryPickerOptions (#651)', () => { ]); }); + it('#651 F4: CLAMPS an oversized pageSize rather than forwarding it', async () => { + const fetchMock = vi.spyOn(window, 'fetch').mockResolvedValue(jsonResponse({ page: [], totalCount: 20000 })); + + await searchLibraryPickerOptions('Episode', 'Alpha', 5000); + + // The 25-row bound is a property of the helper, not of caller discipline. + expect(browseUrl(fetchMock).searchParams.get('pageSize')).toBe(String(LIBRARY_PICKER_RESULTS)); + }); + it('issues NO request for a query below the minimum length', async () => { const fetchMock = vi.spyOn(window, 'fetch').mockResolvedValue(jsonResponse({ page: [], totalCount: 0 })); diff --git a/web/src/api/libraryBrowse.ts b/web/src/api/libraryBrowse.ts index 8d21eb0e0..0d9ea5deb 100644 --- a/web/src/api/libraryBrowse.ts +++ b/web/src/api/libraryBrowse.ts @@ -53,7 +53,16 @@ export function getLibraryBrowseItems(params: GetLibraryBrowseItemsParams = {}): // would look broken in a *name* picker. Escape every Lucene special (and whitespace) so the // boundary stars are the only live wildcards — the same shape `builder/rules/compile.ts` emits for // its `contains` operator. -const LUCENE_WILD_SPECIAL = /([\s+\-!(){}[\]^"~*?:\\/])/g; +// +// The exhaustive set of characters Lucene's QueryParser treats as syntax. `&` and `|` are in it +// because the boolean operators are `&&`/`||`: escaping each character individually neutralises the +// pair. Leaving them live (as this helper's original AutoTuneScreen-local version did) meant a +// title like `Rock && Roll` compiled to a query Lucene parsed as boolean syntax — or rejected — so +// an exactly-matching title returned nothing (#651 F2). `LIBRARY_PICKER_LUCENE_SPECIALS` is +// exported so the test asserts against the character list itself rather than a hand-copied sample +// that cannot see its own omissions. +export const LIBRARY_PICKER_LUCENE_SPECIALS = '+-&|!(){}[]^"~*?:\\/'; +const LUCENE_WILD_SPECIAL = /([\s+\-&|!(){}[\]^"~*?:\\/])/g; export function titleContainsQuery(text: string): string { return `title:*${text.replace(LUCENE_WILD_SPECIAL, '\\$1')}*`; @@ -73,6 +82,10 @@ export const LIBRARY_PICKER_MIN_QUERY = 2; // Resolve picker options for one media-library type by SEARCH rather than by loading a window of // the whole type. Exactly one bounded request per (debounced) query; a too-short query issues none // at all. +// +// `pageSize` is CLAMPED to `LIBRARY_PICKER_RESULTS`, not merely defaulted to it (#651 F4): the +// bound is documented as a property of this helper, so it must not be defeatable by a caller +// passing a larger number. export function searchLibraryPickerOptions( mediaType: LibraryBrowseMediaType, text: string, @@ -83,10 +96,12 @@ export function searchLibraryPickerOptions( return Promise.resolve([]); } + const boundedPageSize = Math.max(1, Math.min(Math.floor(pageSize), LIBRARY_PICKER_RESULTS)); + return getLibraryBrowseItems({ mediaType, pageNum: 0, - pageSize, + pageSize: boundedPageSize, query: titleContainsQuery(trimmed) }).then((result) => (result.page ?? []).map((item) => { diff --git a/web/src/builder/rules/roundtrip.test.ts b/web/src/builder/rules/roundtrip.test.ts index 367cf7ddb..a0da56408 100644 --- a/web/src/builder/rules/roundtrip.test.ts +++ b/web/src/builder/rules/roundtrip.test.ts @@ -23,7 +23,8 @@ const OPS: Record = { // Deterministic LCG so a failing case is reproducible. Divide by 2^32, NOT by 0xffffffff (2^32-1): // the latter returns exactly 1.0 for the maximum state, and `arr[Math.floor(1.0 * len)]` indexes one // past the end. Deterministically unreachable on today's seeds, but a latent flake on any new one -// (#578). +// (#578). The generator's own contract is pinned by the "LCG generator" describe below, which +// drives `pick` at the boundary directly rather than waiting for a seed change to expose it. function lcg(seed: number) { let s = seed >>> 0; return () => { @@ -101,3 +102,37 @@ describe('round-trip: parse(compile(tree)) === tree', () => { expect(deepest).toBe(MAX_GROUP_DEPTH); }); }); + +// #578: the divisor fix has no natural regression test — the max-state case is unreachable with the +// seeds this file uses today — so exercise the two helpers directly at the boundary instead. These +// fail on the old `/ 0xffffffff` divisor. +describe('LCG generator', () => { + it('never returns 1.0, even at the maximum 32-bit state', () => { + // Feed `pick` the largest value the LCG can produce. With `/ 0xffffffff` this is exactly 1.0. + const maxState = 0xffffffff; + expect(maxState / 0x100000000).toBeLessThan(1); + + const alwaysMax = () => maxState / 0x100000000; + expect(pick(alwaysMax, ['a', 'b', 'c'])).toBe('c'); + expect(pick(alwaysMax, ['only'])).toBe('only'); + }); + + it('pick() stays in range across the whole output of a real generator', () => { + const arr = ['a', 'b', 'c', 'd']; + for (let seed = 0; seed < 50; seed += 1) { + const rng = lcg(seed); + for (let i = 0; i < 500; i += 1) { + expect(arr).toContain(pick(rng, arr)); + } + } + }); + + it('stays within [0, 1) for every reachable state', () => { + const rng = lcg(1); + for (let i = 0; i < 20000; i += 1) { + const value = rng(); + expect(value).toBeGreaterThanOrEqual(0); + expect(value).toBeLessThan(1); + } + }); +}); diff --git a/web/src/hooks.test.tsx b/web/src/hooks.test.tsx index ccbb9f6cf..139de33c2 100644 --- a/web/src/hooks.test.tsx +++ b/web/src/hooks.test.tsx @@ -1,4 +1,5 @@ import { cleanup, render } from '@testing-library/react'; +import { StrictMode, useEffect } from 'react'; import { afterEach, describe, expect, it } from 'vitest'; import { useIsMountedRef } from './hooks'; @@ -27,18 +28,31 @@ describe('useIsMountedRef (#578)', () => { expect((captured as unknown as { current: boolean }).current).toBe(false); }); - it('is re-armed on a remount of the same element (StrictMode double-invoke)', () => { - const seen: boolean[] = []; + it('is re-armed by a StrictMode double-invoke (mount → cleanup → mount)', () => { + let captured: { readonly current: boolean } | null = null; + let effectRuns = 0; function Probe() { - const ref = useIsMountedRef(); - seen.push(ref.current); + captured = useIsMountedRef(); + useEffect(() => { + effectRuns += 1; + }); return null; } - const { rerender, unmount } = render(); - rerender(); - expect(seen.every((value) => value)).toBe(true); - unmount(); + render( + + + + ); + + // StrictMode in a dev build mounts, tears down, and remounts every effect. If that did not + // happen, this test would not be exercising re-arming at all, so assert it explicitly rather + // than assume it. + expect(effectRuns).toBeGreaterThan(1); + + // The first cleanup set the flag false; the remount must set it back to true. Remove + // `ref.current = true` from the effect body and this reads false. + expect((captured as unknown as { current: boolean }).current).toBe(true); }); }); diff --git a/web/src/schedules/pickers.test.tsx b/web/src/schedules/pickers.test.tsx index 90f8ca268..c7abecbae 100644 --- a/web/src/schedules/pickers.test.tsx +++ b/web/src/schedules/pickers.test.tsx @@ -1,7 +1,39 @@ import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { SearchPicker, type SearchPickerOption } from './pickers'; +// Every value `SearchPicker` read out of the shared is-mounted guard, in order. This is what makes +// the unmount test non-vacuous (#651 F7): an unmounted tree renders nothing either way, so +// asserting on the DOM cannot distinguish "the guard stopped the update" from "React discarded it". +// Recording the reads proves the component actually CONSULTS the guard — and specifically that it +// consults it once the late response lands, seeing `false`. +const mountedReads: boolean[] = []; + +vi.mock('../hooks', async (importOriginal) => { + const actual = await importOriginal(); + const { useRef } = await import('react'); + return { + ...actual, + useIsMountedRef: () => { + const inner = actual.useIsMountedRef(); + // Memoized so the returned object identity is stable across renders — it is an effect + // dependency in SearchPicker, and a fresh object each render would restart the debounce. + const proxyRef = useRef<{ readonly current: boolean } | null>(null); + proxyRef.current ??= { + get current() { + mountedReads.push(inner.current); + return inner.current; + } + }; + return proxyRef.current; + } + }; +}); + +beforeEach(() => { + mountedReads.length = 0; +}); + afterEach(() => { cleanup(); vi.restoreAllMocks(); @@ -21,7 +53,7 @@ function renderPicker(overrides: Partial[0]> = { } describe('SearchPicker', () => { - it('#578: a search resolving AFTER unmount is dropped rather than applied', async () => { + it('#578: a search resolving AFTER unmount consults the is-mounted guard and is dropped', async () => { let release: ((items: SearchPickerOption[]) => void) | null = null; const search = vi.fn( () => @@ -37,13 +69,17 @@ describe('SearchPicker', () => { await waitFor(() => expect(search).toHaveBeenCalledWith('Alpha')); + // While mounted, no guard read can be `false` (the debounce may not have read it yet at all). + expect(mountedReads).not.toContain(false); + unmount(); release?.([{ id: 1, name: 'Alpha Movie' }]); - // Let the resolved promise's .then run against an unmounted tree. React 19 no longer WARNS on a - // setState-after-unmount, so the observable contract here is only that nothing is re-rendered; - // the guard itself is pinned by the `useIsMountedRef` tests in ../hooks.test.tsx. + // Let the resolved promise's .then run against an unmounted tree. await new Promise((resolve) => setTimeout(resolve, 0)); + // The response handler ran and asked "am I still mounted?", getting `false` — so it skipped the + // setState. Drop the `mountedRef.current &&` from the .then and this read never happens. + expect(mountedReads).toContain(false); expect(screen.queryByText('Alpha Movie')).not.toBeInTheDocument(); }); @@ -86,4 +122,132 @@ describe('SearchPicker', () => { expect(await screen.findByText(/No matches/)).toBeInTheDocument(); }); + + // ---- #651 F6: keyboard operability ---- + // A native with a custom widget removed + // keyboard operability, which is a regression, not a pre-existing gap: without this the only way + // to reach a result is to Tab through to its button. This is the standard ARIA combobox pattern — + // focus stays on the input and `aria-activedescendant` points at the visually-highlighted option. + const choose = (option: SearchPickerOption) => { + onSelect(option.id, option.name); + setQuery(''); + setResults([]); + setResultsQuery(null); + setActiveIndex(-1); + setOpen(false); + }; + + const onKeyDown = (event: ReactKeyboardEvent) => { + if (event.key === 'Escape') { + // Dismiss the list but keep what was typed — Escape closes the popup, it doesn't undo input. + setActiveIndex(-1); + setOpen(false); + return; + } + + if (!listboxOpen) { + return; + } + + if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { + event.preventDefault(); + const delta = event.key === 'ArrowDown' ? 1 : -1; + setActiveIndex((current) => { + // From "nothing active", ArrowDown lands on the first option and ArrowUp on the last. + const next = current < 0 ? (delta === 1 ? 0 : results.length - 1) : current + delta; + return (next + results.length) % results.length; + }); + return; + } + + if (event.key === 'Home' || event.key === 'End') { + event.preventDefault(); + setActiveIndex(event.key === 'Home' ? 0 : results.length - 1); + return; + } + + if (event.key === 'Enter' && activeIndex >= 0 && activeIndex < results.length) { + // Only swallow Enter when it actually commits a highlighted option, so a form's default + // submit behaviour is untouched otherwise. + event.preventDefault(); + choose(results[activeIndex]); + } + }; + + const optionId = (index: number) => `${listboxId}-option-${index}`; return (
@@ -125,30 +186,38 @@ export function SearchPicker({ value={query} disabled={disabled} placeholder={placeholder} + role="combobox" aria-label={`${label} search`} aria-describedby={ariaDescribedBy} + aria-expanded={listboxOpen} + aria-controls={listboxId} + aria-autocomplete="list" + aria-activedescendant={ + listboxOpen && activeIndex >= 0 && activeIndex < results.length ? optionId(activeIndex) : undefined + } onChange={(event) => setQuery(event.target.value)} onFocus={() => setOpen(true)} + onKeyDown={onKeyDown} /> {loading && } {noMatches &&
No matches — try a different title.
} - {open && results.length > 0 && ( -
    - {results.map((option) => ( + {listboxOpen && ( +
      + {results.map((option, index) => (
    • diff --git a/web/src/screens/FillerPresetsScreen.test.tsx b/web/src/screens/FillerPresetsScreen.test.tsx index 048e894b1..d07f8a757 100644 --- a/web/src/screens/FillerPresetsScreen.test.tsx +++ b/web/src/screens/FillerPresetsScreen.test.tsx @@ -221,4 +221,68 @@ describe('FillerPresetsScreen', () => { const results = screen.getByRole('listbox', { name: 'Television Show results' }); expect(within(results).getAllByRole('option')).toHaveLength(25); }); + + it('#651 F3: a SLOW edit-load name resolution never relabels a newer selection', async () => { + window.history.pushState({}, '', '/app/filler-presets/1'); + + // Hold the by-id read for the STORED show (9999) open, so it resolves only after the user has + // already picked a different show. One bespoke mock — layering a second spy over mockApi's + // would re-enter itself. + let releaseStoredName: ((response: Response) => void) | null = null; + + vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL, init?: RequestInit) => { + const pathname = new URL(input.toString(), 'http://localhost').pathname; + const method = (init?.method ?? 'GET').toUpperCase(); + + if (pathname === '/api/v1/shows/9999') { + return new Promise((resolve) => { + releaseStoredName = resolve; + }); + } + if (pathname === '/api/v1/filler-presets' && method === 'GET') { + return Promise.resolve(jsonResponse(presetList)); + } + if (pathname === '/api/v1/filler-presets/1' && method === 'GET') { + return Promise.resolve(jsonResponse(editPreset)); + } + if (pathname === '/api/v1/library/browse' && method === 'GET') { + return Promise.resolve( + jsonResponse({ + page: [{ id: 123, mediaItemId: 123, mediaType: 'TelevisionShow', title: 'New Show' }], + totalCount: 1 + }) + ); + } + return Promise.resolve(new Response(null, { status: 204 })); + }); + + render(); + await waitFor(() => expect(screen.getByDisplayValue('Bumper')).toBeInTheDocument()); + + // No name yet — the read is still in flight, so the picker shows the id it definitely has. + expect(await screen.findByText('#9999')).toBeInTheDocument(); + + // The user picks a different show while the stored name is still resolving. + fireEvent.click(screen.getByRole('button', { name: 'Change' })); + const searchInput = await screen.findByLabelText('Television Show search'); + fireEvent.focus(searchInput); + fireEvent.change(searchInput, { target: { value: 'New Show' } }); + fireEvent.click(await screen.findByRole('option', { name: 'New Show' })); + + const chip = () => screen.getByLabelText('Clear Television Show').closest('.ctv-picker-selected'); + await waitFor(() => expect(chip()?.textContent).toContain('New Show')); + + // NOW the stale read lands. It must not relabel the newer selection: the draft holds 123, so a + // title resolved for 9999 is simply not about the current selection. + releaseStoredName?.( + new Response(JSON.stringify({ id: 9999, libraryId: 1, mediaSourceKind: 'Local', title: 'Stored Show' }), { + headers: { 'Content-Type': 'application/json' }, + status: 200 + }) + ); + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(chip()?.textContent).toContain('New Show'); + expect(chip()?.textContent).not.toContain('Stored Show'); + }); }); diff --git a/web/src/screens/FillerPresetsScreen.tsx b/web/src/screens/FillerPresetsScreen.tsx index b9730b6a0..9cb65e3a7 100644 --- a/web/src/screens/FillerPresetsScreen.tsx +++ b/web/src/screens/FillerPresetsScreen.tsx @@ -488,7 +488,13 @@ function FillerEditor({ mode }: { mode: { id: number; kind: 'edit' } | { kind: ' // The searchable (media-library) picker's current selection LABEL. The draft holds only the id, // so this is what keeps an already-selected item displayable while it sits outside — or ahead // of — any search result set. - const [selectedName, setSelectedName] = useState(null); + // + // It is stored WITH the id it names (#651 F3) rather than as a bare string. A slow edit-load name + // resolution for the stored id can otherwise land after the user has already picked something + // else, labelling the new id with the old item's title while `mediaItemId` — and therefore what + // gets saved — says otherwise. Keying the label means a stale resolution is simply ignored at + // render time instead of having to be raced, and it self-heals if the user selects back. + const [selectedLabel, setSelectedLabel] = useState(null); const isEdit = mode.kind === 'edit'; @@ -515,11 +521,18 @@ function FillerEditor({ mode }: { mode: { id: number; kind: 'edit' } | { kind: ' // A failure here is cosmetic — the picker falls back to `#id` and the id is still saved — // so it must never surface as a load error. const config = COLLECTION_TYPES.find((entry) => entry.value === loaded.collectionType); - if (config?.searchable && loaded.mediaItemId != null) { - resolveMediaItemName(loaded.collectionType, loaded.mediaItemId) + const requestedId = loaded.mediaItemId; + if (config?.searchable && requestedId != null) { + resolveMediaItemName(loaded.collectionType, requestedId) .then((name) => { if (active && name) { - setSelectedName(name); + // Tagged with the id it was resolved FOR — see `selectedLabel` above — and refused + // outright if the label we already hold names a DIFFERENT id, i.e. the user picked + // something else while this was in flight. The render-time id check alone would + // stop the mislabelling but still discard the newer, correct label. + setSelectedLabel((current) => + current !== null && current.id !== requestedId ? current : { id: requestedId, name } + ); } }) .catch(() => { @@ -614,7 +627,7 @@ function FillerEditor({ mode }: { mode: { id: number; kind: 'edit' } | { kind: ' const setCollectionType = (type: CollectionType) => { // The id refs are cleared below, so the search picker's label must go with them. - setSelectedName(null); + setSelectedLabel(null); setDraft((current) => { if (!current) { return current; @@ -808,7 +821,7 @@ function FillerEditor({ mode }: { mode: { id: number; kind: 'edit' } | { kind: ' {searchBrowseType ? ( // #651: the media library is resolved by search, never list-loaded. The current - // selection renders from `selectedName` (resolved once by id on edit-load), not from + // selection renders from `selectedLabel` (resolved once by id on edit-load), not from // the result set, so an already-selected item survives every search — and even a // failed name resolution degrades to `#id` rather than losing the id. { - setSelectedName(null); + setSelectedLabel(null); set({ [activeConfig.field]: null }); }} onSelect={(id, name) => { - setSelectedName(name); + setSelectedLabel({ id, name }); set({ [activeConfig.field]: id }); }} placeholder={`Search ${activeConfig.label.toLowerCase()}…`} search={searchLibrary} selectedId={pickerSelectedId} - selectedName={selectedName} + selectedName={selectedLabel?.id === pickerSelectedId ? selectedLabel.name : null} /> ) : ( : only the three order/type selects remain. + // The picker is a typeahead, not a s (the typeahead input carries role="combobox" per the ARIA pattern, so count + // elements, not roles). const searchInput = await screen.findByLabelText('Movie search'); - expect(await screen.findAllByRole('combobox')).toHaveLength(3); + expect(document.querySelectorAll('select')).toHaveLength(3); expect(screen.getByText(/Type at least 2 characters to search the library/)).toBeInTheDocument(); // Selecting the type loads NOTHING from the media library. @@ -448,4 +462,129 @@ describe('RerunCollectionsScreen', () => { ).toBe(true); }); }); + + // ---- #651 F1: a refresh that cannot NAME a selection must never CLEAR it ---- + // + // `RerunCollectionsController.ProjectToResponseModel` derives selectedId AND selectedName from + // the loaded navigation, so several supported types come back unnamed — or unidentified — through + // no fault of the record: `GetRerunCollectionByIdHandler` eager-loads metadata only for + // Show/Season/Artist/Movie; `MediaCollections/Mapper` maps RemoteStream through `_ => null` + // (HTTP 200, null selection); Episode/MusicVideo dereference unloaded `Season`/`Artist` + // navigations and 500. In every one of those cases the stored id must survive and Save must stay + // enabled. + const detailGapCases = [ + { + // The reviewer's concrete case: a successful response carrying a null selection. + detail: (type: string): Response => + jsonResponse({ ...storedSelection, collectionType: type, selectedId: null, selectedName: null }), + name: 'RemoteStream — detail 200 with a null selection (Mapper `_ => null`)', + type: 'RemoteStream' + }, + { + detail: (): Response => new Response(null, { status: 500 }), + name: 'Episode — detail 500 (NRE on the unloaded Season navigation)', + type: 'Episode' + }, + { + detail: (): Response => new Response(null, { status: 500 }), + name: 'MusicVideo — detail 500 (NRE on the unloaded Artist navigation)', + type: 'MusicVideo' + }, + { + // Id resolves but the name is a server-side placeholder/null — must not blank what we hold. + detail: (type: string): Response => jsonResponse({ ...storedSelection, collectionType: type, selectedName: null }), + name: 'Song — detail 200 with an id but no name', + type: 'Song' + }, + { + detail: (type: string): Response => jsonResponse({ ...storedSelection, collectionType: type, selectedName: null }), + name: 'OtherVideo — detail 200 with an id but no name', + type: 'OtherVideo' + }, + { + detail: (type: string): Response => + jsonResponse({ ...storedSelection, collectionType: type, selectedId: null, selectedName: null }), + name: 'Image — detail 200 with a null selection', + type: 'Image' + } + ]; + + it.each(detailGapCases)('F1: keeps the stored selection and Save when the detail GET cannot name it — $name', async ({ detail, type }) => { + let detailCalls = 0; + mockApi({ + list: [{ ...storedSelection, collectionType: type }], + // The detail response must echo the SAME collectionType the list gave, as a real server + // would — changing it here would remount the picker mid-assertion and mask what is tested. + onRequest: (url, method) => { + if (url === '/api/v1/rerun-collections/9' && method === 'GET') { + detailCalls += 1; + return detail(type); + } + return null; + } + }); + + render(); + fireEvent.click(await screen.findByText('Stored Rerun')); + + // Wait for the REFRESH to have landed before asserting — the pre-refresh draft already shows + // the right thing, so an assertion that races the refresh cannot see it being clobbered. (React + // mutates the existing text node rather than replacing it, so even `findByText` + a later + // `toBeInTheDocument` would pass on a stale handle: read `textContent` after the fact.) + await waitFor(() => expect(detailCalls).toBe(1)); + await waitFor(() => expect(screen.getByRole('button', { name: 'Save rerun collection' })).toBeInTheDocument()); + await new Promise((resolve) => setTimeout(resolve, 0)); + + const chip = screen.getByLabelText(/^Clear /).closest('.ctv-picker-selected'); + expect(chip?.textContent).toContain('Stored Item'); + + // ...and the record remains saveable: clearing the id would strand the user on a record they + // cannot re-save. + expect(screen.getByRole('button', { name: 'Save rerun collection' })).not.toBeDisabled(); + expect(screen.queryByText('A selection is required')).not.toBeInTheDocument(); + }); + + it('F1: degrades to #id (never to an empty picker) when no name is available at all', async () => { + mockApi({ + list: [{ ...storedSelection, collectionType: 'RemoteStream', selectedName: null }], + onRequest: (url, method) => + url === '/api/v1/rerun-collections/9' && method === 'GET' + ? jsonResponse({ ...storedSelection, collectionType: 'RemoteStream', selectedId: null, selectedName: null }) + : null + }); + + render(); + fireEvent.click(await screen.findByText('Stored Rerun')); + + expect(await screen.findByText('#42')).toBeInTheDocument(); + await waitFor(() => + expect(screen.getByRole('button', { name: 'Save rerun collection' })).not.toBeDisabled() + ); + }); + + it('F1: re-saves the preserved id unchanged after an unnamed refresh', async () => { + const fetchMock = mockApi({ + list: [{ ...storedSelection, collectionType: 'RemoteStream' }], + onRequest: (url, method) => { + if (url === '/api/v1/rerun-collections/9' && method === 'GET') { + return jsonResponse({ ...storedSelection, collectionType: 'RemoteStream', selectedId: null, selectedName: null }); + } + return url === '/api/v1/rerun-collections/9' && method === 'PUT' ? jsonResponse({ id: 9 }, 200) : null; + } + }); + + render(); + fireEvent.click(await screen.findByText('Stored Rerun')); + expect(await screen.findByText('Stored Item')).toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'Save rerun collection' })); + + await waitFor(() => { + const putCall = fetchMock.mock.calls.find( + ([u, init]) => u === '/api/v1/rerun-collections/9' && (init?.method ?? '').toUpperCase() === 'PUT' + ); + expect(putCall).toBeDefined(); + expect(JSON.parse(String(putCall?.[1]?.body)).selectedId).toBe(42); + }); + }); }); diff --git a/web/src/screens/RerunCollectionsScreen.tsx b/web/src/screens/RerunCollectionsScreen.tsx index 7516b4143..2ba3a9a49 100644 --- a/web/src/screens/RerunCollectionsScreen.tsx +++ b/web/src/screens/RerunCollectionsScreen.tsx @@ -229,6 +229,28 @@ function draftFromRerun(rerun: RerunCollection): Draft { }; } +// A refresh that fails to NAME a selection does not mean the selection is gone — and the client +// must never be the thing that destroys it. `RerunCollectionsController.ProjectToResponseModel` +// derives BOTH `selectedId` and `selectedName` from the loaded navigation, so a response can carry +// `selectedId: null` for a record that certainly has one: +// - `GetRerunCollectionByIdHandler` eager-loads media metadata only for Show/Season/Artist/Movie; +// - `MediaCollections/Mapper.ProjectToViewModel` maps RemoteStream through `_ => null` outright, +// so a RemoteStream rerun collection returns HTTP 200 with a null selection; +// - Song/OtherVideo/Image resolve their id but fall back to a placeholder name. +// Replacing the draft wholesale would clear a stored id on the strength of a naming gap, leaving +// the user unable to even re-save the record (Save is gated on `selectedId != null`). So merge: +// take everything from the refresh EXCEPT an absent id/name, which fall back to what we hold. +// Degrading to `#id` in the picker is acceptable; dropping the id is not. See #651 F1 — the +// server-side naming gaps themselves are tracked separately. +function mergeRerunDraft(current: Draft, rerun: RerunCollection): Draft { + const refreshed = draftFromRerun(rerun); + return { + ...refreshed, + selectedId: refreshed.selectedId ?? current.selectedId, + selectedName: refreshed.selectedName || current.selectedName + }; +} + function RerunCollectionEditor({ initial, onBack, @@ -278,7 +300,7 @@ function RerunCollectionEditor({ .then((meta) => { if (active) { etagRef.current = meta.etag; - setDraft(draftFromRerun(meta.data)); + setDraft((current) => mergeRerunDraft(current, meta.data)); } }) .catch((error: unknown) => { diff --git a/web/src/shell.css b/web/src/shell.css index f9c432a80..2aa011f49 100644 --- a/web/src/shell.css +++ b/web/src/shell.css @@ -1612,7 +1612,10 @@ body { font-size: 13px; } +/* `-active` is the keyboard virtual cursor (aria-activedescendant); it deliberately shares the + hover treatment so pointer and keyboard users get the same affordance. */ .ctv-picker-result:hover, +.ctv-picker-result-active, .ctv-picker-result[aria-selected='true'] { background: var(--surface-3, rgba(255, 255, 255, 0.08)); }