diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index 63a26266f..45764c1ec 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -178,10 +178,42 @@ the #644 follow-up got Class A right and Class B only half right): The helper owns both bounds: at most ONE `getLibraryBrowseItems` request per settled query, at most `LIBRARY_PICKER_RESULTS` (25) rows, and no request at all below `LIBRARY_PICKER_MIN_QUERY` (2) - characters. Selecting a media-library type must issue **zero** requests. There is no truncation, so - there is no truncation hint — the old `Showing the first 100 of 5000 — use search to narrow.` copy - is gone from these pickers along with the window it described. Prove the bound with a - **request-count assertion against a large (20k-row) fixture**, not by inspection. + characters. Selecting a media-library type must issue **zero** requests. The per-kind + `LIBRARY_PICKER_RESULTS` cap is the only truncation this class has — there is no whole-type window + left to hint at, so the old `Showing the first 100 of 5000 — use search to narrow.` copy is gone + from these pickers along with the window it described. Surfacing the per-kind cap is *permitted* + wherever it is reachable, and *required* only where bulk selection makes the count actionable — see + the `AddItemsDialog` sub-bullet below, which sums the cap across kinds and renders a `Showing N of + M matches` hint for exactly that reason. Prove the bound with a **request-count assertion against a + large (20k-row) fixture**, not by inspection. + + - **`SearchPicker` is the single-select SHAPE, not the rule itself.** A MULTI-select picker + (`CollectionsScreen`'s `AddItemsDialog` — checkbox rows, many items added at once, fanned out + over several kinds) cannot render `SearchPicker` and must not be forced to. It satisfies this + section by taking the same *constraints* the helper enforces for single-select — the gate on + `LIBRARY_PICKER_MIN_QUERY`, `titleContainsQuery` the typed text, `LIBRARY_PICKER_RESULTS` per + kind — via `searchLibraryBrowseItems` (`web/src/api/libraryBrowse.ts`), a sibling of + `searchLibraryPickerOptions` that returns full `LibraryBrowseItem` rows plus `totalCount` + instead of `{id, name}`, so the bound lives in the helper rather than the caller (#685 review + finding 2). There is no post-fetch `slice`, but the per-kind cap can still truncate the real + match count — this is a bulk multi-select add, where "add the 40 matching episodes" is a + first-class use, so `AddItemsDialog` sums each kind's `totalCount` and renders a `Showing N of + M matches` hint once it exceeds the rendered rows (finding 4 — an earlier revision of this + bullet called the truncation nothing left to hint at). **The gate's home is the shared HELPER, + not the screen — however single-sink the screen's own function looks.** #685 got this wrong + twice in a row, and the second time is the instructive one: the check sat inside `runSearch`, + which genuinely IS the one sink both entry paths route through, so it read as correct. It was + still a duplicate of the helper's gate, and the two masked each other: as of `4be3f247d` — + which had no unit tests on the helper — deleting EITHER copy left the whole suite green, so the + min-query boundary test pinned nothing. Removing the screen's copy is what made the helper's + gate load-bearing. **The invariant, not the count: every gate must have at least one test that + reddens when that gate ALONE is removed.** A guard you cannot redden is not a guard, and "it's + the single sink" is not evidence that it is the only one. **Outstanding on this screen**: `AddItemsDialog` still lacks the monotonic `seqRef` + stale-response guard and `useIsMountedRef()` — the same class of guard "Debounced typeaheads" + below mandates there, applied to a debounced-while-typing fetch; `AddItemsDialog` is an explicit + Search-button submission, not a typeahead, so that mandate doesn't reach it directly, but the + same race (a superseded search settling after a newer one) can still occur here — tracked in + **ersatztv#740**, not yet fixed here. - **Compile typed text; never forward raw Lucene.** Send `titleContainsQuery(text)` → `title:**`. The index's default field does not match bare title words (`Alpha` finds diff --git a/web/src/api/libraryBrowse.test.ts b/web/src/api/libraryBrowse.test.ts index e2b16ff05..2b4db9db8 100644 --- a/web/src/api/libraryBrowse.test.ts +++ b/web/src/api/libraryBrowse.test.ts @@ -1,6 +1,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { getLibraryBrowseItems, + searchLibraryBrowseItems, searchLibraryPickerOptions, titleContainsQuery, LIBRARY_PICKER_LUCENE_SPECIALS, @@ -150,3 +151,53 @@ describe('searchLibraryPickerOptions (#651)', () => { expect(fetchMock).not.toHaveBeenCalled(); }); }); + +describe('searchLibraryBrowseItems (#685 — AddItemsDialog sibling of searchLibraryPickerOptions)', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + // The reviewer proved this helper was dead code to the suite: deleting its clamp, or deleting + // its gate, both left the whole suite green. These three tests mirror the ones above for + // searchLibraryPickerOptions so the same bound is pinned for the sibling helper. + + it('#651 F4: CLAMPS an oversized pageSize rather than forwarding it', async () => { + const fetchMock = vi.spyOn(window, 'fetch').mockResolvedValue(jsonResponse({ page: [], totalCount: 20000 })); + + await searchLibraryBrowseItems('Episode', 'Alpha', 5000); + + expect(browseUrl(fetchMock).searchParams.get('pageSize')).toBe(String(LIBRARY_PICKER_RESULTS)); + }); + + it('issues NO request for a query below the minimum length, and resolves an empty result', async () => { + const fetchMock = vi.spyOn(window, 'fetch').mockResolvedValue(jsonResponse({ page: [], totalCount: 0 })); + + expect(await searchLibraryBrowseItems('Episode', 'a')).toEqual({ items: [], totalCount: 0 }); + expect(await searchLibraryBrowseItems('Episode', ' ')).toEqual({ items: [], totalCount: 0 }); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('compiles/escapes the trimmed query, and returns the FULL row (mediaType present) plus totalCount', async () => { + const fetchMock = vi.spyOn(window, 'fetch').mockResolvedValue( + jsonResponse({ + page: [{ id: 1, mediaItemId: 7, mediaType: 'Movie', title: 'Show Alpha' }], + totalCount: 42 + }) + ); + + const result = await searchLibraryBrowseItems('Movie', ' Show Alpha '); + + expect(fetchMock).toHaveBeenCalledTimes(1); + const url = browseUrl(fetchMock); + expect(url.searchParams.get('query')).toBe(titleContainsQuery('Show Alpha')); + expect(url.searchParams.get('mediaType')).toBe('Movie'); + expect(url.searchParams.get('pageNum')).toBe('0'); + expect(url.searchParams.get('pageSize')).toBe(String(LIBRARY_PICKER_RESULTS)); + // The reason this helper exists rather than reusing searchLibraryPickerOptions: the full row + // (mediaType included), not the {id, name} shape. + expect(result).toEqual({ + items: [{ id: 1, mediaItemId: 7, mediaType: 'Movie', title: 'Show Alpha' }], + totalCount: 42 + }); + }); +}); diff --git a/web/src/api/libraryBrowse.ts b/web/src/api/libraryBrowse.ts index 0d9ea5deb..59f0857ec 100644 --- a/web/src/api/libraryBrowse.ts +++ b/web/src/api/libraryBrowse.ts @@ -111,6 +111,41 @@ export function searchLibraryPickerOptions( ); } +export interface LibraryBrowseSearchResult { + items: LibraryBrowseItem[]; + totalCount: number; +} + +// Like `searchLibraryPickerOptions` above — same min-query gate, same clamp, same compiled query — +// but for a MULTI-select caller (`CollectionsScreen`'s `AddItemsDialog`) that needs the full +// `LibraryBrowseItem` row (mediaType + id, for `toAddItemsRequest`) rather than the `{id, name}` +// shape a single-select `SearchPicker` renders, plus the response's `totalCount` so the caller can +// surface how much of a match was actually returned. The gate/clamp/compile live HERE, not at the +// call site, so no caller can accidentally skip them (§3b — "the bound belongs to the helper, not +// the caller"). +export function searchLibraryBrowseItems( + mediaType: LibraryBrowseMediaType, + text: string, + pageSize: number = LIBRARY_PICKER_RESULTS +): Promise { + const trimmed = text.trim(); + if (trimmed.length < LIBRARY_PICKER_MIN_QUERY) { + return Promise.resolve({ items: [], totalCount: 0 }); + } + + const boundedPageSize = Math.max(1, Math.min(Math.floor(pageSize), LIBRARY_PICKER_RESULTS)); + + return getLibraryBrowseItems({ + mediaType, + pageNum: 0, + pageSize: boundedPageSize, + query: titleContainsQuery(trimmed) + }).then((result) => ({ + items: result.page ?? [], + totalCount: result.totalCount ?? 0 + })); +} + export function messageFromLibraryBrowseError(error: unknown, fallback = 'Unable to load library items'): string { if (error instanceof ApiError) { return error.detail ?? error.message; diff --git a/web/src/api/pageSizeCallSites.guard.test.ts b/web/src/api/pageSizeCallSites.guard.test.ts index a923d9126..d729f0e3a 100644 --- a/web/src/api/pageSizeCallSites.guard.test.ts +++ b/web/src/api/pageSizeCallSites.guard.test.ts @@ -83,12 +83,15 @@ import { scanPageSizeSites } from './pageSizeScan'; * mechanism, and the row bound is a property of the CODE rather than of a * caller's discipline (a clamp inside the shared helper for #651's * library picker; a fixed small constant at the inline preview sites). - * Nothing is list-loaded, so there is no truncation to surface and the - * absence of a truncation hint is correct — which is why such a site - * cannot be filed under 'class-b', whose defining evidence IS a surfaced - * `totalCount`. Applies only where the query is genuinely required: a - * site that degrades to an unfiltered browse when the query is empty is - * NOT search-bounded (see 'deviation'). + * Nothing is list-loaded, so a truncation hint is not REQUIRED here — but, + * unlike 'class-b', it is not the defining evidence either: a search-bounded + * site MAY surface its own per-kind cap (e.g. summed across kinds) once that + * count is actionable, without that hint reclassifying it as 'class-b'. What + * distinguishes the two classes is the SHAPE of the bound (one clamped + * search request per settled query vs. one bounded page of a list), not + * whether a hint is rendered. Applies only where the query is genuinely + * required: a site that degrades to an unfiltered browse when the query is + * empty is NOT search-bounded (see 'deviation'). * - 'class-b' — one bounded page at (or under) the cap, with the real truncation * (`totalCount` vs items shown) surfaced to the user. Post-#651 this no * longer covers media-library pickers (those are 'search-bounded'). @@ -163,6 +166,35 @@ const REGISTRY: RegistryEntry[] = [ 'correctly absent). The bound is a clamp, not a default: Math.min(pageSize, ' + 'LIBRARY_PICKER_RESULTS) inside the helper, so a caller cannot widen it.' }, + { + file: 'api/libraryBrowse.ts', + kind: 'literal', + value: 'boundedPageSize', + classification: 'search-bounded', + note: + "searchLibraryBrowseItems — the #685 review fix that moved AddItemsDialog.runSearch's " + + 'pageSize call site out of screens/CollectionsScreen.tsx and into this shared helper (same ' + + 'gate/clamp/compile as searchLibraryPickerOptions above), so a multi-select caller needing ' + + 'full LibraryBrowseItem rows plus totalCount cannot skip the bound either. A deliberate ' + + 'second occurrence of the same (file, kind, value) identity — the multiset comparison ' + + 'requires it be discovered twice. No request is issued below LIBRARY_PICKER_MIN_QUERY (a ' + + 'blank form submit and a kind-chip click below the gate both resolve every kind to ' + + '{items: [], totalCount: 0} via the HELPER\'s own gate — CollectionsScreen no longer keeps a ' + + 'second copy of this check; the #685 second review proved the two masked each other), the ' + + 'typed text is compiled via titleContainsQuery rather than forwarded raw, and each kind is ' + + 'bounded to one request per settled query at LIBRARY_PICKER_RESULTS rows. Unlike the #685 ' + + 'first fix, this is NOT "nothing left to hint at": the per-kind cap can still truncate the ' + + "real match count below what totalCount reports, and AddItemsDialog now sums each kind's " + + "totalCount and renders a 'Showing N of M' hint when it exceeds the rendered rows. The only " + + 'remaining client-side filter in AddItemsDialog (ADDABLE_TYPES.has(item.mediaType)) is inert, ' + + 'not a silent drop, and this is now enforced by the type system rather than by convention on ' + + 'BOTH ingress paths into the searched kinds — MediaKindFilter (the explicit-chip path) and ' + + 'DEFAULT_SEARCH_KINDS (the `all` fan-out) are each derived from ADDABLE_TYPE_LIST via ' + + '`(typeof ADDABLE_TYPE_LIST)[number]`, so adding a non-addable kind to either is a compile ' + + 'error. Enforcing only the first was the #685 round-3 review finding: the hint sums ' + + 'PRE-filter totalCounts against POST-filter rows, so one unenforced ingress is enough to ' + + 'overstate it with every row of that kind dropped.' + }, { file: 'api/paging.ts', kind: 'shorthand', @@ -246,22 +278,6 @@ const REGISTRY: RegistryEntry[] = [ 'Playout block history: forwards a user-adjustable `pageSize` state (persisted, backed by a ' + 'page-size