From ca99bedb1a2a042a49232ad6a1d6725bb2bc28cf Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 27 Jul 2026 00:16:13 +0200 Subject: [PATCH] fix(650): rewrite the pageSize guard on the TS compiler API; fix two append-ownership races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second cold cross-family (Codex, BLOCKED) re-review of b90f8a3b found the regex/bracket-tracking guard scanner still defeated in five ways, and two new High-severity races introduced by the F3/F4 fixes. Addressed as a further follow-up (b90f8a3b left untouched). GUARD REWRITE (per the review's explicit direction — stop patching the regex, use the compiler): - New `web/src/api/pageSizeScan.ts`: `scanPageSizeSites` parses each file with `ts.createSourceFile` and walks the real AST for `pageSize` PropertyAssignment/ShorthandPropertyAssignment nodes inside an ObjectLiteralExpression. This eliminates categorically (not case-by-case): - M-3: comments and string/template CONTENTS are never revisited as code, so a `'https://...'` string can't be misread as an unterminated string that swallows the rest of the file. - M-4: an object literal nested in a ternary, `??`, or JSX expression container is still found — the walk visits every descendant node regardless of the syntactic context above the ObjectLiteralExpression. - M-5: template-literal interpolations are real AST children, not opaque text. - L-7: a type literal (`type P = { pageSize: 100 }`), an interface PropertySignature, and a destructuring ObjectBindingPattern (parameter or nested) are structurally different node kinds from ObjectLiteralExpression — excluded by kind, not by a preceding-character heuristic a stray `{`/`(`/`,` could fool. `getLineAndCharacterOfPosition` gives exact line+column (fixes M-6 identity granularity) instead of the prior line-only identity. - `pageSizeCallSites.guard.test.ts` now imports the shared scanner; identity is `file:line:column:kind:value`, compared as a MULTISET (count, not membership) in both directions. - Both directions (unregistered / stale) are computed and folded into ONE thrown Error so a failure always shows the complete picture in one run, addressing the line-churn "second direction never renders" concern. - New `pageSizeScan.test.ts`: a FIXTURE test (inline source strings, no repo scan) pinning the exact discovered set for every case the review named — comment-in-string, string containing the literal text `pageSize: 100`, template interpolation, ternary, `??`, JSX container, same-line duplicates, parameter/nested destructuring, a type literal, an interface property, a forwarded call expression, a React dependency array. This is what actually protects the scanner going forward — the guard test alone only ever proved today's snapshot of real call sites, never the scanner's handling of input classes it hadn't happened to encounter yet. - Re-verified both original plants (a duplicate at-cap call in an already-registered file, and a new file with both a literal and a shorthand site) against the rewritten scanner; both still fail with the new combined-direction message. Also verified a run with BOTH directions simultaneously non-empty renders both in one report. HIGH-1 (ChannelBuilder.tsx useLibraryBrowse, now web/src/builder/libraryBrowse.ts): `reqId` identifies a query GENERATION, not an individual fetch — a page-0 refresh and a "Load more" append can be outstanding simultaneously under the same reqId (query changes while an append is in flight for the new generation). Whichever settled first used to clear `loadingMore`, letting a second click fire an out-of-order/duplicate page fetch. Fixed with a per-fetch `fetchId` plus a `loadingFetchIdRef`/`loadingFetchReqIdRef` pair: only the fetch that OWNS the currently-displayed spinner can clear it; a same-generation page-0 refresh leaves a same-generation append's spinner alone, while a page-0 refresh for a NEW generation still retires an abandoned OLDER-generation append's spinner (preserving the original #650 F3 fix). Reproduced the exact interleaving from the review in a new test (gate B's page-0 and page-1 fetches independently, click "Load more" while B's page-0 is still in flight) and confirmed it fails without the fix (button re-enables while the append is still pending). HIGH-2 (same file): the append-failure rollback mutated whatever `pageRef.current` currently held, rather than the specific page THIS fetch requested — under an overlapping-append race, a later page's success followed by an earlier page's failure could roll the cursor back past already-appended progress, corrupting a retry into refetching a duplicate. Fixed with a compare-and-set guard (`if (pageRef.current === pageNum)`) so the rollback only fires when nothing has advanced the cursor since. Since this overlap is UI-unreachable once HIGH-1's single-flight disabling is wired up (verified empirically: two synchronous fireEvent.click calls in RTL only produce one request, since act() flushes the disabling render between them), the regression test drives `useLibraryBrowse` directly via `renderHook` (now exported) to force the exact interleaving and confirms it fails without the fix (page 2 gets duplicated, page 3 never requested). Extracted `useLibraryBrowse` (plus `loadCollections`/`loadLibraryItems`/ `BrowseState`/the media-type const arrays) into a new non-JSX module `web/src/builder/libraryBrowse.ts` — exporting a hook from a .tsx file tripped `react-refresh/only-export-components`; this also makes the hook importable by `renderHook` without pulling in the whole screen component. No server-side/C# change. Full local gate: lint clean, tsc clean, full vitest run 110 files / 1051 tests passed (one LibrariesScreen.test.tsx flake reproduced under full-suite parallel load, confirmed pre-existing and unrelated — passes in isolation, never touched that file). --- web/src/api/pageSizeCallSites.guard.test.ts | 289 +++++++------------- web/src/api/pageSizeScan.test.ts | 160 +++++++++++ web/src/api/pageSizeScan.ts | 83 ++++++ web/src/builder/ChannelBuilder.test.tsx | 181 +++++++++++- web/src/builder/ChannelBuilder.tsx | 170 +----------- web/src/builder/libraryBrowse.ts | 230 ++++++++++++++++ 6 files changed, 754 insertions(+), 359 deletions(-) create mode 100644 web/src/api/pageSizeScan.test.ts create mode 100644 web/src/api/pageSizeScan.ts create mode 100644 web/src/builder/libraryBrowse.ts diff --git a/web/src/api/pageSizeCallSites.guard.test.ts b/web/src/api/pageSizeCallSites.guard.test.ts index 1edd1d4e9..bdf4caba4 100644 --- a/web/src/api/pageSizeCallSites.guard.test.ts +++ b/web/src/api/pageSizeCallSites.guard.test.ts @@ -2,6 +2,7 @@ import { readFileSync, readdirSync } from 'node:fs'; import { dirname, join, relative } from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; +import { pageSizeSiteId, scanPageSizeSites } from './pageSizeScan'; /** * #650 guard: an ENUMERATING allow-list over every `pageSize` call site in the SPA. @@ -14,36 +15,32 @@ import { describe, expect, it } from 'vitest'; * just don't match a "pageSize above the cap" pattern. * * So this guard does NOT pattern-match on the pageSize VALUE (that repeats the #644 mistake for - * the next magic number). It enumerates every call site that passes a `pageSize` property into an - * object literal — either `pageSize: ` or the ES6 shorthand `{ ..., pageSize }` - * — and cross-checks the discovered set against a hand-reviewed registry below, in BOTH - * directions: + * the next magic number). It enumerates every `pageSize` property inside a real object-literal + * expression via `scanPageSizeSites` (the TypeScript compiler API — see `pageSizeScan.ts`'s doc + * comment for why a hand-rolled text/regex scan was replaced) and cross-checks the discovered set + * against a hand-reviewed registry below, in BOTH directions: * - a NEWLY discovered, unregistered site fails (a new call site was added without a documented * classification — the exact way #650 could recur invisibly); * - a REGISTERED site no longer discovered fails (the registry has gone stale — e.g. a site was * removed or refactored to no longer pass a `pageSize` property, and the registry should * shrink to match, not silently claim coverage of code that no longer exists). + * Both directions are computed and reported in a SINGLE combined failure message (not two + * sequential `expect` calls) — an early throw would otherwise hide the second direction's result + * in the same run, understating what actually needs fixing. * - * **Identity is per-OCCURRENCE, not per-file** (#650 follow-up F5): a second at-cap call added to - * an already-registered file must not hide behind that file's existing entry. Each discovered/ - * registered site is keyed by `file:line:kind:value`, not `file:value` — two matches in the same - * file on different lines are two distinct identities, and a duplicate literal on a NEW line in an - * already-registered file is therefore an unregistered site like any other. + * **Identity is per-OCCURRENCE** (line + column from the real AST position, #650 follow-up F5/M-6): + * two `pageSize` properties on the same line (e.g. both branches of a ternary) get distinct + * identities because each has its own token position. Comparison is by MULTISET count, not just + * set membership, so an accidental duplicate registry entry — or, in principle, two structurally + * identical sites colliding on identity — is still caught rather than one occurrence silently + * covering both. * - * **Known residual gap (#650 follow-up F6):** this scan is text/bracket-based, not a real parser. - * It resolves the ES6 shorthand form (`{ ..., pageSize }`, distinguishing an object-literal `{` - * from a block-statement `{` and an array `[` by looking at the token immediately before the - * opening brace), but it CANNOT resolve: - * - a `pageSize` value carried through object SPREAD, e.g. `getFoo(makeAtCapParams())` where - * `makeAtCapParams` returns `{ pageSize: 100 }` from another module or `getFoo({ ...opts })` - * where `opts` was built elsewhere with an at-cap `pageSize` — the literal/shorthand is not - * textually present at the call site; - * - a `pageSize` passed as a bare POSITIONAL argument (`getSearchAllItems(query, pageNum, - * pageSize)`) rather than an object-literal property — `api/search.ts`'s - * `getAllSearchItemIds` (the api.search-allitems-paging precedent) does this and is - * deliberately out of this guard's reach. - * These are written down here, not silently absent: a call site introduced through either path - * would not be caught by this guard and needs a human re-grep if that shape becomes common. + * `scanPageSizeSites` itself is verified against inline fixture source strings covering every + * input class a text-level scanner previously got wrong (comment-in-string, template + * interpolation, ternary, `??`, JSX container, same-line duplicates, parameter/nested + * destructuring, a type literal, a string containing the text `pageSize: 100`) in + * `pageSizeScan.test.ts` — that test does not depend on the real repo, so it protects the SCANNER + * itself, not just today's snapshot of call sites. * * Each registry entry classifies the site per `docs/spa-conventions.md` §3b / * `docs/decisions/records/spa/list-completeness-vs-bounded-pickers.md`: @@ -55,22 +52,31 @@ import { describe, expect, it } from 'vitest'; * - 'paged-ui' — real paging UI (a page/"load more" control, or a user-adjustable page-size * selector, keyed to a genuine `totalCount`), so a `pageSize` at or below the * cap is correct as-is. + * + * **Known residual gap:** object SPREAD (`getFoo({ ...opts })` where `opts` was built elsewhere + * with an at-cap `pageSize`) and a `pageSize` passed as a bare POSITIONAL argument rather than an + * object-literal property (`api/search.ts`'s `getAllSearchItemIds(query, pageNum, pageSize)`, the + * api.search-allitems-paging precedent) are NOT resolvable by this scan — there is no `pageSize` + * token inside an object-literal expression to find. Written down here, not silently absent: a + * call site introduced through either path needs a human re-grep if that shape becomes common. */ interface RegistryEntry { file: string; line: number; + column: number; kind: 'literal' | 'shorthand'; value: string; classification: 'class-a' | 'class-b' | 'paged-ui'; note: string; } -// Keep in file order, then line order, so a diff against the discovered set is easy to read. +// Keep in file order, then position order, so a diff against the discovered set is easy to read. const REGISTRY: RegistryEntry[] = [ { file: 'api/paging.ts', line: 83, + column: 62, kind: 'shorthand', value: 'pageSize', classification: 'class-a', @@ -81,6 +87,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'api/paging.ts', line: 93, + column: 60, kind: 'shorthand', value: 'pageSize', classification: 'class-a', @@ -88,7 +95,8 @@ const REGISTRY: RegistryEntry[] = [ }, { file: 'builder/ChannelBuilder.tsx', - line: 447, + line: 427, + column: 79, kind: 'literal', value: '100', classification: 'class-b', @@ -99,8 +107,9 @@ const REGISTRY: RegistryEntry[] = [ "'Showing the first N of M seasons' hint if a show somehow exceeds the cap." }, { - file: 'builder/ChannelBuilder.tsx', - line: 539, + file: 'builder/libraryBrowse.ts', + line: 50, + column: 98, kind: 'shorthand', value: 'pageSize', classification: 'paged-ui', @@ -110,8 +119,9 @@ const REGISTRY: RegistryEntry[] = [ 'is meaningful — this is the paged-ui replacement for the original truncating implementation.' }, { - file: 'builder/ChannelBuilder.tsx', - line: 566, + file: 'builder/libraryBrowse.ts', + line: 77, + column: 9, kind: 'shorthand', value: 'pageSize', classification: 'paged-ui', @@ -120,6 +130,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'builder/SmartCollectionDialog.tsx', line: 92, + column: 52, kind: 'literal', value: '24', classification: 'class-b', @@ -130,6 +141,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/AutoTuneScreen.tsx', line: 1527, + column: 105, kind: 'literal', value: 'MEMBER_PREVIEW_SIZE', classification: 'class-b', @@ -138,6 +150,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/AutoTuneScreen.tsx', line: 1556, + column: 67, kind: 'literal', value: 'ADD_SOURCE_RESULTS', classification: 'class-b', @@ -146,6 +159,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/BlockPlayoutTroubleshootingScreen.tsx', line: 161, + column: 76, kind: 'shorthand', value: 'pageSize', classification: 'paged-ui', @@ -156,6 +170,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/CollectionsScreen.tsx', line: 234, + column: 58, kind: 'literal', value: '50', classification: 'class-b', @@ -166,6 +181,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/FillerPresetsScreen.tsx', line: 501, + column: 67, kind: 'literal', value: 'LIBRARY_BROWSE_PAGE_CAP', classification: 'class-b', @@ -174,6 +190,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/LogsScreen.tsx', line: 106, + column: 32, kind: 'shorthand', value: 'pageSize', classification: 'paged-ui', @@ -184,6 +201,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/MediaBrowseScreen.tsx', line: 119, + column: 92, kind: 'literal', value: 'PAGE_SIZE', classification: 'paged-ui', @@ -192,6 +210,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/MediaDetailScreen.tsx', line: 267, + column: 49, kind: 'literal', value: 'CHILD_PAGE_SIZE', classification: 'paged-ui', @@ -200,6 +219,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/PlaylistsScreen.tsx', line: 193, + column: 76, kind: 'literal', value: 'LIBRARY_BROWSE_PAGE_CAP', classification: 'class-b', @@ -208,6 +228,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/RerunCollectionsScreen.tsx', line: 146, + column: 76, kind: 'literal', value: 'LIBRARY_BROWSE_PAGE_CAP', classification: 'class-b', @@ -218,6 +239,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/SearchScreen.tsx', line: 118, + column: 40, kind: 'literal', value: 'PAGE_SIZE', classification: 'paged-ui', @@ -226,6 +248,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/TrashScreen.tsx', line: 82, + column: 44, kind: 'literal', value: 'PAGE_SIZE', classification: 'paged-ui', @@ -234,6 +257,7 @@ const REGISTRY: RegistryEntry[] = [ { file: 'screens/TrashScreen.tsx', line: 113, + column: 79, kind: 'literal', value: 'PAGE_SIZE', classification: 'paged-ui', @@ -244,15 +268,6 @@ const REGISTRY: RegistryEntry[] = [ // This file lives in `src/api/`; the scan root is `src/`. const SRC_DIR = join(dirname(fileURLToPath(import.meta.url)), '..'); -// Matches an object-literal `pageSize:` key followed by either a decimal literal or an -// identifier — NOT a type annotation (`pageSize: number`) and NOT a forwarded call expression -// (`pageSize: String(pageSize)`, a dynamic passthrough of a caller-supplied value, not a literal). -const PAGE_SIZE_LITERAL = /\bpageSize:\s*(\d+|[A-Za-z_][A-Za-z0-9_]*)\b/g; -const TYPE_ANNOTATION_VALUES = new Set(['number', 'string', 'boolean', 'undefined', 'null', 'any', 'unknown']); -// A `{` is treated as opening an object LITERAL (vs a block statement) only when the token -// immediately before it puts us in expression position. -const OBJECT_CONTEXT_PRECEDERS = new Set(['(', ',', '[', ':', '=']); - function listSourceFiles(dir: string): string[] { const out: string[] = []; for (const entry of readdirSync(dir, { withFileTypes: true })) { @@ -271,129 +286,10 @@ function listSourceFiles(dir: string): string[] { return out; } -// Strips block comments (preserving newlines, so line numbers stay accurate) and line comments. -// Block comments must go first: a JSDoc block routinely contains an ODD mid-comment backtick (an -// inline-code marker like `` `pageSize` ``) that would otherwise desync the quote-tracking bracket -// scan below across the rest of the file. -function stripComments(text: string): string { - const noBlockComments = text.replace(/\/\*[\s\S]*?\*\//g, (block) => block.replace(/[^\n]/g, ' ')); - return noBlockComments - .split('\n') - .map((line) => { - const idx = line.indexOf('//'); - return idx >= 0 ? line.slice(0, idx) : line; - }) - .join('\n'); -} - -function lineNumberAt(text: string, index: number): number { - let line = 1; - for (let i = 0; i < index; i++) { - if (text[i] === '\n') { - line++; - } - } - return line; -} - -// Finds every ES6 shorthand `pageSize` property (bare `pageSize`, no `:`) that sits directly -// inside an object-literal `{ ... }` — not a block statement, not an array, not a destructuring -// default array (`const [pageSize = 100] = ...`), not a string/template literal. Bracket- and -// quote-tracked rather than a single regex, because a naive "preceded by `{` or `,`" check cannot -// tell `getFoo({ pageNum, pageSize })` (an object literal — a real call-site risk) apart from -// `useCallback(fn, [pageNum, pageSize])` (a React dependency array — not a call site at all); both -// have `pageSize` immediately preceded by a comma. -function findShorthandIndices(codeText: string): number[] { - const stack: Array<{ ch: '{' | '[' | '('; isObjectContext?: boolean }> = []; - const results: number[] = []; - const len = codeText.length; - let i = 0; - - while (i < len) { - const ch = codeText[i]; - - if (ch === '{') { - let p = i - 1; - while (p >= 0 && /\s/.test(codeText[p])) { - p--; - } - let isObjectContext = OBJECT_CONTEXT_PRECEDERS.has(codeText[p]); - if (!isObjectContext && p >= 0) { - const wordEnd = p + 1; - let wordStart = wordEnd; - while (wordStart > 0 && /[A-Za-z0-9_]/.test(codeText[wordStart - 1])) { - wordStart--; - } - isObjectContext = codeText.slice(wordStart, wordEnd) === 'return'; - } - stack.push({ ch, isObjectContext }); - i++; - continue; - } - - if (ch === '[' || ch === '(') { - stack.push({ ch: ch as '[' | '(' }); - i++; - continue; - } - - if (ch === '}' || ch === ']' || ch === ')') { - stack.pop(); - i++; - continue; - } - - if (ch === '"' || ch === "'" || ch === '`') { - const quote = ch; - i++; - while (i < len && codeText[i] !== quote) { - if (codeText[i] === '\\') { - i++; - } - i++; - } - i++; - continue; - } - - if (/[A-Za-z_]/.test(ch)) { - let j = i; - while (j < len && /[A-Za-z0-9_]/.test(codeText[j])) { - j++; - } - const word = codeText.slice(i, j); - if (word === 'pageSize') { - let k = j; - while (k < len && /\s/.test(codeText[k])) { - k++; - } - // Skip an optional `?` (a TS optional-property declaration, e.g. `pageSize?: number`) so - // the colon check below still correctly excludes it. - if (codeText[k] === '?') { - k++; - while (k < len && /\s/.test(codeText[k])) { - k++; - } - } - const isColonForm = codeText[k] === ':'; - const enclosing = stack[stack.length - 1]; - if (!isColonForm && enclosing && enclosing.ch === '{' && enclosing.isObjectContext) { - results.push(i); - } - } - i = j; - continue; - } - - i++; - } - - return results; -} - interface DiscoveredSite { file: string; line: number; + column: number; kind: 'literal' | 'shorthand'; value: string; } @@ -404,32 +300,43 @@ function discoverPageSizeCallSites(): DiscoveredSite[] { for (const absPath of files) { const relPath = relative(SRC_DIR, absPath); - const codeText = stripComments(readFileSync(absPath, 'utf8')); + const text = readFileSync(absPath, 'utf8'); - for (const match of codeText.matchAll(PAGE_SIZE_LITERAL)) { - const value = match[1]; - if (TYPE_ANNOTATION_VALUES.has(value)) { - continue; - } - // A forwarded call expression, e.g. `pageSize: String(pageSize)` — the character right - // after the matched identifier is `(`, i.e. this is a function call, not a literal/const. - const afterIndex = match.index! + match[0].length; - if (codeText[afterIndex] === '(') { - continue; - } - sites.push({ file: relPath, line: lineNumberAt(codeText, match.index!), kind: 'literal', value }); - } - - for (const idx of findShorthandIndices(codeText)) { - sites.push({ file: relPath, line: lineNumberAt(codeText, idx), kind: 'shorthand', value: 'pageSize' }); + for (const site of scanPageSizeSites(text, absPath)) { + sites.push({ file: relPath, ...site }); } } return sites; } -function siteId(site: { file: string; line: number; kind: string; value: string }): string { - return `${site.file}:${site.line}:${site.kind}:${site.value}`; +function fullId(site: { file: string; line: number; column: number; kind: string; value: string }): string { + return `${site.file}:${pageSizeSiteId(site)}`; +} + +// Multiset (count per identity) comparison, not plain array `.includes` membership (#650 +// follow-up M-6) — so a registry that accidentally lists the same identity twice, or a future +// scanner change that could (in principle) emit a duplicate, is still caught rather than one +// occurrence silently covering both. +function toCounts(ids: string[]): Map { + const counts = new Map(); + for (const id of ids) { + counts.set(id, (counts.get(id) ?? 0) + 1); + } + return counts; +} + +// Returns entries present in `left` more times than in `right`, expanded per the excess count — +// e.g. a `left` id appearing 3 times against 1 in `right` yields that id listed twice. +function multisetExcess(left: Map, right: Map): string[] { + const excess: string[] = []; + for (const [id, count] of left) { + const remaining = count - (right.get(id) ?? 0); + for (let i = 0; i < remaining; i++) { + excess.push(id); + } + } + return excess.sort(); } describe('pageSize call-site guard (#650)', () => { @@ -444,20 +351,24 @@ describe('pageSize call-site guard (#650)', () => { }); it('matches the discovered pageSize call sites EXACTLY against the reviewed registry (not a non-empty check)', () => { - const discovered = discoverPageSizeCallSites().map(siteId).sort(); - const registered = REGISTRY.map(siteId).sort(); + const discoveredCounts = toCounts(discoverPageSizeCallSites().map(fullId)); + const registeredCounts = toCounts(REGISTRY.map(fullId)); - // Two separate assertions rather than one set-equality check, so a failure message names - // exactly which side is wrong: an unregistered NEW site (a defect risk) vs a stale registry - // entry that no longer matches anything in the source (a maintenance smell), never both - // collapsed into an opaque "sets differ" message. Identity is per-occurrence (file:line:kind: - // value, #650 follow-up F5) — a second at-cap call added to an already-registered FILE is a - // distinct, unregistered identity, not silently absorbed by that file's existing entry. - const unregistered = discovered.filter((site) => !registered.includes(site)); - const stale = registered.filter((site) => !discovered.includes(site)); + const unregistered = multisetExcess(discoveredCounts, registeredCounts); + const stale = multisetExcess(registeredCounts, discoveredCounts); - expect(unregistered, 'discovered pageSize call site(s) missing from the REGISTRY above').toEqual([]); - expect(stale, 'REGISTRY entries no longer found as a real pageSize call site').toEqual([]); + // Both directions are folded into ONE assertion so a failure always shows the complete + // picture in a single run (#650 follow-up, line-churn concern) — two sequential `expect` + // calls would throw on the first failing direction and never evaluate/report the second. + if (unregistered.length > 0 || stale.length > 0) { + const report = [ + `UNREGISTERED (${unregistered.length}) — discovered pageSize call site(s) missing from the REGISTRY above:`, + ...unregistered.map((id) => ` + ${id}`), + `STALE (${stale.length}) — REGISTRY entries no longer found as a real pageSize call site:`, + ...stale.map((id) => ` - ${id}`) + ].join('\n'); + throw new Error(report); + } }); it('every registry entry documents its class per docs/spa-conventions.md §3b', () => { diff --git a/web/src/api/pageSizeScan.test.ts b/web/src/api/pageSizeScan.test.ts new file mode 100644 index 000000000..419430d31 --- /dev/null +++ b/web/src/api/pageSizeScan.test.ts @@ -0,0 +1,160 @@ +import { describe, expect, it } from 'vitest'; +import { pageSizeSiteId, scanPageSizeSites, type PageSizeSite } from './pageSizeScan'; + +/** + * Fixture test for `scanPageSizeSites` itself — NOT a scan of the real repo (that's + * `pageSizeCallSites.guard.test.ts`). This is what actually protects the SCANNER going forward: + * a prior hand-rolled regex/bracket-tracking version passed the guard test against unmodified + * source at both #650 commits while still being defeated by every case below, because the guard + * only ever exercised today's snapshot of real call sites — it never proved the scanner handles + * the INPUT CLASSES that expose a text-level scanner's blind spots. Pinning the exact discovered + * set against synthetic source strings closes that gap. + */ + +function ids(sites: PageSizeSite[]): string[] { + return sites.map(pageSizeSiteId); +} + +describe('scanPageSizeSites', () => { + it('finds a literal pageSize: property in a plain object-literal call argument', () => { + const source = `getFoo({ pageSize: 100, query });\n`; + const sites = scanPageSizeSites(source, 'fixture.ts'); + expect(ids(sites)).toEqual(['1:10:literal:100']); + }); + + it('finds the ES6 shorthand pageSize property in a plain object-literal call argument', () => { + const source = `getFoo({ pageSize, query });\n`; + const sites = scanPageSizeSites(source, 'fixture.ts'); + expect(ids(sites)).toEqual(['1:10:shorthand:pageSize']); + }); + + it('is NOT fooled by a "//" inside a string literal (M-3)', () => { + // A naive text scanner that treats every `//` as a comment start turns this into an + // unterminated string, then swallows the REAL call site below it. + const source = [`const endpoint = 'https://example.test';`, `getFoo({ pageSize: 100 });`, ''].join('\n'); + const sites = scanPageSizeSites(source, 'fixture.ts'); + expect(ids(sites)).toEqual(['2:10:literal:100']); + }); + + it('does NOT match a string literal that merely CONTAINS the text "pageSize: 100" (L-7)', () => { + const source = `const label = "pageSize: 100";\n`; + expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]); + }); + + it('finds an object literal passed inside a template-literal interpolation (M-5)', () => { + const source = 'const url = `${await getFoo({ pageSize })}`;\n'; + const sites = scanPageSizeSites(source, 'fixture.ts'); + expect(ids(sites)).toEqual(['1:31:shorthand:pageSize']); + }); + + it('finds an object literal in each branch of a ternary, even on the SAME line (M-4, M-6)', () => { + const source = `return ok ? getA({ pageSize }) : getB({ pageSize });\n`; + const sites = scanPageSizeSites(source, 'fixture.ts'); + // Two distinct occurrences on one line get distinct identities (different columns) — a + // single registry entry cannot silently cover both. + expect(ids(sites)).toEqual(['1:20:shorthand:pageSize', '1:41:shorthand:pageSize']); + expect(sites[0].column).not.toBe(sites[1].column); + }); + + it('finds an object literal on the right-hand side of ?? (M-4)', () => { + const source = `getFoo(options ?? { pageSize: 50 });\n`; + const sites = scanPageSizeSites(source, 'fixture.ts'); + expect(ids(sites)).toEqual(['1:21:literal:50']); + }); + + it('finds an object literal inside a JSX expression container attribute (M-4)', () => { + const source = `const el = ;\n`; + const sites = scanPageSizeSites(source, 'fixture.tsx'); + expect(ids(sites)).toEqual(['1:34:shorthand:pageSize']); + }); + + it('does NOT match a parameter destructuring pattern (L-7)', () => { + const source = `function f({ pageSize }: { pageSize: number }) {}\n`; + // The destructured PARAMETER `{ pageSize }` is an ObjectBindingPattern, not an + // ObjectLiteralExpression — excluded by node kind. Its TYPE annotation `{ pageSize: number }` + // is a TypeLiteral (PropertySignature), also excluded by node kind — never an object literal. + expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]); + }); + + it('does NOT match a nested destructuring pattern (L-7)', () => { + const source = `const { nested: { pageSize } } = input;\n`; + expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]); + }); + + it('does NOT match a type-literal declaration (L-7)', () => { + const source = `type P = { pageSize: 100 };\n`; + expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]); + }); + + it('does NOT match an interface property declaration', () => { + const source = `interface Params {\n pageSize?: number;\n}\n`; + expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]); + }); + + it('does NOT match a forwarded call expression (a dynamic passthrough, not a fixed value)', () => { + const source = `getFoo({ pageSize: String(pageSize) });\n`; + expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]); + }); + + it('is not confused by a pageSize reference inside a comment', () => { + const source = [`// pageSize: 999 — this is just prose, not code`, `getFoo({ query });`, ''].join('\n'); + expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]); + }); + + it('is not confused by a pageSize reference inside a block/JSDoc comment', () => { + const source = ['/**', ' * Uses `pageSize` under the hood — see also `{ pageSize: 100 }`.', ' */', 'getFoo({ query });', ''].join( + '\n' + ); + expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]); + }); + + it('does NOT match a React dependency array containing pageSize', () => { + const source = `useCallback(load, [pageNum, pageSize, sortField]);\n`; + expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]); + }); + + it('finds a const-identifier literal value (not just a numeric literal)', () => { + const source = `getFoo({ pageSize: LIBRARY_BROWSE_PAGE_CAP });\n`; + const sites = scanPageSizeSites(source, 'fixture.ts'); + expect(ids(sites)).toEqual(['1:10:literal:LIBRARY_BROWSE_PAGE_CAP']); + }); + + it('covers every case above together in one multi-line fixture and pins the exact discovered set', () => { + const source = [ + `const endpoint = 'https://example.test';`, // M-3: not a comment + `const label = "pageSize: 100";`, // L-7: string contents, not code + `// pageSize: 999 in a line comment`, // not code + `/** block comment mentioning \`pageSize\` */`, // not code + `type P = { pageSize: 100 };`, // L-7: type literal, not a value + `interface Q { pageSize?: number; }`, // not a value + `function f({ pageSize }: { pageSize: number }) {}`, // L-7: destructuring + its type + `const { nested: { pageSize } } = input;`, // L-7: nested destructuring + `useCallback(load, [pageNum, pageSize]);`, // dependency array, not an object literal + `getFoo({ pageSize: String(pageSize) });`, // forwarded call, not a fixed value + `getFoo({ pageSize: 100 });`, // REAL: literal + `getBar({ pageSize });`, // REAL: shorthand + `getBaz(options ?? { pageSize: 50 });`, // REAL: ?? context (M-4) + `const url = \`\${await getQux({ pageSize })}\`;`, // REAL: template interpolation (M-5) + `return ok ? getA({ pageSize }) : getB({ pageSize });` // REAL x2: ternary, same line (M-4/M-6) + ].join('\n'); + + const sites = scanPageSizeSites(source, 'fixture.ts'); + expect(ids(sites)).toEqual([ + '11:10:literal:100', + '12:10:shorthand:pageSize', + '13:21:literal:50', + '14:31:shorthand:pageSize', + '15:20:shorthand:pageSize', + '15:41:shorthand:pageSize' + ]); + // Anti-vacuity: the fixture packs in 10 non-matching traps ahead of the 6 real sites — a + // scanner that matched everything (or nothing) would fail this count, not just the ids above. + expect(sites.length).toBe(6); + }); + + it('scans .tsx source using the TSX script kind (JSX does not parse under plain .ts rules)', () => { + const source = `export function C() {\n return
;\n}\n`; + const sites = scanPageSizeSites(source, 'fixture.tsx'); + expect(ids(sites)).toEqual(['2:23:literal:10']); + }); +}); diff --git a/web/src/api/pageSizeScan.ts b/web/src/api/pageSizeScan.ts new file mode 100644 index 000000000..60064d18c --- /dev/null +++ b/web/src/api/pageSizeScan.ts @@ -0,0 +1,83 @@ +import * as ts from 'typescript'; + +/** + * #650 follow-up: an AST-based scanner for every `pageSize` property that appears inside a real + * object LITERAL expression. Extracted into its own module so both the enumerating guard + * (`pageSizeCallSites.guard.test.ts`, which scans the real repo) and a fixture test + * (`pageSizeScan.test.ts`, which scans synthetic source strings and does NOT touch the repo) can + * exercise the exact same scanning logic. + * + * A prior hand-rolled regex/bracket-tracking version of this scan was replaced after a review + * found it defeated by comments-in-strings, template-literal interpolations, ternary/`??` + * contexts, JSX containers, and same-line duplicates — each a DIFFERENT input class a text-level + * lexer has to special-case one at a time. The TypeScript compiler API sidesteps the whole + * category: comments and string/template CONTENTS are trivia/literal text the parser never + * revisits as code, and a real object-literal expression (`ObjectLiteralExpression`) is a + * structurally different AST node from a type literal (`type X = { pageSize: number }`, + * `PropertySignature` inside a `TypeLiteralNode`/`InterfaceDeclaration`) or a destructuring + * pattern (`ObjectBindingPattern`, e.g. `function f({ pageSize }) {}` or + * `const { pageSize } = x`) — so those are excluded by NODE KIND, not by a preceding-character + * heuristic that can be fooled by an unrelated `{`/`(`/`,`. + */ + +export interface PageSizeSite { + /** 1-based source line of the `pageSize` property (name), matching editor line numbers. */ + line: number; + /** 1-based source column of the `pageSize` property (name). */ + column: number; + kind: 'literal' | 'shorthand'; + /** + * For `kind: 'literal'`: the numeric-literal text or the referenced const identifier's name. + * For `kind: 'shorthand'`: always the literal string `'pageSize'` (the shorthand form only ever + * forwards whatever `pageSize` binding is in scope — there is no separate "value" to name). + */ + value: string; +} + +function scriptKindFor(fileName: string): ts.ScriptKind { + return fileName.endsWith('.tsx') ? ts.ScriptKind.TSX : ts.ScriptKind.TS; +} + +export function scanPageSizeSites(sourceText: string, fileName: string): PageSizeSite[] { + const sourceFile = ts.createSourceFile(fileName, sourceText, ts.ScriptTarget.Latest, true, scriptKindFor(fileName)); + const sites: PageSizeSite[] = []; + + function positionOf(node: ts.Node): { line: number; column: number } { + const { line, character } = sourceFile.getLineAndCharacterOfPosition(node.getStart(sourceFile)); + return { line: line + 1, column: character + 1 }; + } + + function visit(node: ts.Node): void { + if (ts.isObjectLiteralExpression(node)) { + for (const property of node.properties) { + if (ts.isPropertyAssignment(property) && ts.isIdentifier(property.name) && property.name.text === 'pageSize') { + const initializer = property.initializer; + // Only a numeric literal or a bare identifier (a const/variable reference) counts as a + // fixed value baked into THIS call site. A forwarded expression — `String(pageSize)`, a + // ternary, a template, a function call — is a dynamic passthrough of whatever the + // caller supplied, not a literal this site chose; it is deliberately not recorded here + // (see the module doc comment on `loadAllPages`/positional-argument residual gaps). + if (ts.isNumericLiteral(initializer)) { + const { line, column } = positionOf(property.name); + sites.push({ line, column, kind: 'literal', value: initializer.text }); + } else if (ts.isIdentifier(initializer)) { + const { line, column } = positionOf(property.name); + sites.push({ line, column, kind: 'literal', value: initializer.text }); + } + } else if (ts.isShorthandPropertyAssignment(property) && property.name.text === 'pageSize') { + const { line, column } = positionOf(property.name); + sites.push({ line, column, kind: 'shorthand', value: 'pageSize' }); + } + } + } + + ts.forEachChild(node, visit); + } + + visit(sourceFile); + return sites.sort((a, b) => (a.line === b.line ? a.column - b.column : a.line - b.line)); +} + +export function pageSizeSiteId(site: { line: number; column: number; kind: string; value: string }): string { + return `${site.line}:${site.column}:${site.kind}:${site.value}`; +} diff --git a/web/src/builder/ChannelBuilder.test.tsx b/web/src/builder/ChannelBuilder.test.tsx index d8fb34d3c..b012612a5 100644 --- a/web/src/builder/ChannelBuilder.test.tsx +++ b/web/src/builder/ChannelBuilder.test.tsx @@ -1,7 +1,8 @@ -import { cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import { act, cleanup, fireEvent, render, renderHook, screen, waitFor, within } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { applyDesignSystemTheme } from '../designSystem'; import { ChannelBuilderScreen } from './ChannelBuilder'; +import { useLibraryBrowse } from './libraryBrowse'; function jsonResponse(body: unknown, status = 200): Response { return new Response(JSON.stringify(body), { @@ -832,6 +833,99 @@ describe('Channel Builder (#89)', () => { } ); + it( + 'does not let a SAME-generation page-0 refresh clear a still-in-flight "Load more" spinner ' + + '(#650 follow-up HIGH-1)', + async () => { + // Reproduces the exact interleaving from the review: query A is displayed; the query + // changes to B, starting a page-0 refresh under a NEW reqId; WHILE that page-0 refresh is + // still in flight (before it has resolved), the user clicks "Load more" — reachable because + // the button still reflects query A's stale (but real) canLoadMore state, since "previous + // results stay visible until the new page resolves" is deliberate. That "Load more" click + // starts a B/page-1 append under the SAME reqId as B's page-0. If B/page-0 settling clears + // `loadingMore` (as it did before the fix, since both fetches shared one `reqId`), the + // button re-enables while B/page-1 is still genuinely in flight. + const pageA0 = [browseItem({ id: 1, mediaItemId: 1, title: 'Item A1' })]; + const pageB0 = [browseItem({ id: 2, mediaItemId: 2, title: 'Item B1' })]; + const pageB1 = [browseItem({ id: 3, mediaItemId: 3, title: 'Item B2' })]; + + let releaseB0: (() => void) | null = null; + const gateB0 = new Promise((resolve) => { + releaseB0 = resolve; + }); + let releaseB1: (() => void) | null = null; + const gateB1 = new Promise((resolve) => { + releaseB1 = resolve; + }); + + const fetchMock = mockBuilderApi({ + ...builderDefaults(), + browseHandler: async (search) => { + if (search.get('mediaType') !== 'TelevisionShow') { + return { page: [], totalCount: 0 }; + } + + const query = search.get('query') ?? ''; + const pageNum = Number(search.get('pageNum') ?? '0'); + + if (query === '') { + // Query A: resolves immediately, canLoadMore true (1 of 3). + return { page: pageA0, totalCount: 3 }; + } + + if (pageNum === 0) { + await gateB0; + return { page: pageB0, totalCount: 3 }; + } + + await gateB1; + return { page: pageB1, totalCount: 3 }; + } + }); + + await renderBuilder(); + + expect(await screen.findByText('Item A1')).toBeInTheDocument(); + const loadMoreButton = screen.getByRole('button', { name: 'Load more' }); + expect((loadMoreButton as HTMLButtonElement).disabled).toBe(false); + + // Change the query — the search box debounces (280ms) before `query` state (and the reqId + // bump) actually happens, so wait for B/page-0's request to genuinely be ISSUED before + // clicking — otherwise the click below would still target query A's generation. + fireEvent.change(screen.getByPlaceholderText('Search shows & movies…'), { target: { value: 'B' } }); + await waitFor(() => { + const b0Calls = fetchMock.mock.calls.filter(([input]) => { + const url = new URL(input.toString(), 'http://localhost'); + return ( + url.pathname === '/api/v1/library/browse' && + url.searchParams.get('mediaType') === 'TelevisionShow' && + url.searchParams.get('query') === 'B' && + (url.searchParams.get('pageNum') ?? '0') === '0' + ); + }); + expect(b0Calls.length).toBeGreaterThan(0); + }); + + // Click "Load more" WHILE B/page-0 is still in flight — starts B/page-1 under the same reqId. + fireEvent.click(loadMoreButton); + expect((loadMoreButton as HTMLButtonElement).disabled).toBe(true); + + // Release B/page-0 — it replaces the stale A items with B's page-0 items, but must NOT + // clear the append spinner: B/page-1 (the actual owner of that spinner) is still pending. + releaseB0?.(); + await screen.findByText('Item B1'); + expect((screen.getByRole('button', { name: 'Load more' }) as HTMLButtonElement).disabled).toBe(true); + + // Now let B/page-1 resolve — ONLY this must clear the spinner and append onto B's items. + releaseB1?.(); + await screen.findByText('Item B2'); + expect(screen.getByText('Item B1')).toBeInTheDocument(); + await waitFor(() => { + expect((screen.getByRole('button', { name: 'Load more' }) as HTMLButtonElement).disabled).toBe(false); + }); + } + ); + it( 'fires "Load more" in Collections mode once merged items reach the 100-row server cap, using ' + 'the REAL summed totalCount rather than the truncated page length (#650)', @@ -1106,4 +1200,89 @@ describe('Channel Builder (#89)', () => { expect(document.documentElement).toHaveAttribute('data-theme', 'dual'); expect(screen.getByPlaceholderText('Search shows & movies…')).toBeInTheDocument(); }); + + it( + 'HIGH-2: does not corrupt the page cursor when an EARLIER page fails after a LATER, ' + + 'overlapping append already succeeded', + async () => { + // This overlap (two same-generation appends genuinely in flight at once) is UI-unreachable + // once HIGH-1's fix disables "Load more" for the full lifetime of the owning fetch — a + // literal double-click in a test is absorbed by React's synchronous act() batching before + // the second click ever reaches the (by-then-disabled) button. So this drives the + // `useLibraryBrowse` hook directly via `renderHook`, calling `loadMore()` twice back-to-back + // to force exactly the interleaving the review described: page 1 issued, then (before it + // resolves) page 2 issued; page 2 SUCCEEDS while page 1 is still pending; THEN page 1 fails. + const page0 = [browseItem({ id: 1, mediaItemId: 1, mediaType: 'TelevisionShow', title: 'Item A' })]; + const page2Items = [browseItem({ id: 3, mediaItemId: 3, mediaType: 'TelevisionShow', title: 'Item C' })]; + + let releasePage1: (() => void) | null = null; + const page1Gate = new Promise((resolve) => { + releasePage1 = resolve; + }); + const televisionShowPageNumsRequested: number[] = []; + + vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL) => { + const url = new URL(input.toString(), 'http://localhost'); + if (url.pathname !== '/api/v1/library/browse') { + return Promise.resolve(new Response(null, { status: 204 })); + } + + const mediaType = url.searchParams.get('mediaType'); + if (mediaType !== 'TelevisionShow') { + return Promise.resolve(jsonResponse({ page: [], totalCount: 0 })); + } + + const pageNum = Number(url.searchParams.get('pageNum') ?? '0'); + televisionShowPageNumsRequested.push(pageNum); + + if (pageNum === 0) { + return Promise.resolve(jsonResponse({ page: page0, totalCount: 5 })); + } + if (pageNum === 1) { + // Held open until AFTER page 2 (below) has already resolved successfully — then fails. + return page1Gate.then(() => new Response(null, { status: 500 })); + } + if (pageNum === 2) { + return Promise.resolve(jsonResponse({ page: page2Items, totalCount: 5 })); + } + return Promise.resolve(jsonResponse({ page: [], totalCount: 0 })); + }); + + const { result } = renderHook(() => useLibraryBrowse('library', '', null)); + + await waitFor(() => expect(result.current.state.status).toBe('success')); + expect(result.current.state.items.map((item) => item.title)).toEqual(['Item A']); + + // Force the overlap: two `loadMore()` calls back-to-back, synchronously — the second reads + // `pageRef` AFTER the first has already (synchronously) advanced it, so this issues page 1 + // and page 2 as two genuinely simultaneous in-flight requests. + act(() => { + result.current.loadMore(); + result.current.loadMore(); + }); + + // Page 2 succeeds first (page 1 is still gated). + await waitFor(() => expect(result.current.state.items.map((item) => item.title)).toContain('Item C')); + expect(result.current.state.items.map((item) => item.title)).toEqual(['Item A', 'Item C']); + + // Now let page 1 fail. + await act(async () => { + releasePage1?.(); + }); + await waitFor(() => expect(result.current.loadMoreError).not.toBeNull()); + + // Item A and Item C must both still be present (page 1's failure must not wipe state — + // #650 follow-up F4), AND the page cursor must not have been corrupted back to 1 (which + // would make the retry below re-request page 2 — already successfully appended — as a + // duplicate, per the review's exact failure description). + expect(result.current.state.items.map((item) => item.title)).toEqual(['Item A', 'Item C']); + + act(() => { + result.current.loadMore(); + }); + + await waitFor(() => expect(televisionShowPageNumsRequested).toContain(3)); + expect(televisionShowPageNumsRequested.filter((pageNum) => pageNum === 2)).toHaveLength(1); + } + ); }); diff --git a/web/src/builder/ChannelBuilder.tsx b/web/src/builder/ChannelBuilder.tsx index 7b20f356a..6b41ee102 100644 --- a/web/src/builder/ChannelBuilder.tsx +++ b/web/src/builder/ChannelBuilder.tsx @@ -59,7 +59,6 @@ import { type FFmpegProfile, type FillerPreset, type LibraryBrowseItem, - type LibraryBrowseMediaType, type MediaSource, type Watermark } from '../api'; @@ -107,6 +106,7 @@ import { type SubtitleMode, type TranscodeMode } from './advancedOptions'; +import { useLibraryBrowse } from './libraryBrowse'; import { SmartCollectionDialog } from './SmartCollectionDialog'; // Advanced-options model (enum unions, INHERIT/omit semantics, the override hook) @@ -115,26 +115,6 @@ import { SmartCollectionDialog } from './SmartCollectionDialog'; // TYPE_ICON / TYPE_LABEL / hueOf are shared with the media browse/search/trash screens // (see ../media/mediaKinds) so the per-kind icon+label map has a single source of truth. -const COLLECTION_MEDIA_TYPES: LibraryBrowseMediaType[] = [ - 'Collection', - 'SmartCollection', - 'MultiCollection', - 'RerunCollection', - 'Playlist' -]; - -// Browsing a library shows the "pickable" top-level kinds only, not every -// episode/song/etc. nested underneath them. Kept explicit here since -// `GET /api/v1/library/browse` now spans all 10 media kinds when `mediaType` is -// omitted. TelevisionSeason is intentionally excluded so a multi-season show -// renders as a single tile instead of flooding the grid with per-season tiles -// (issue #180); seasons are reachable via the show tile's Seasons drill-in. -const LIBRARY_MEDIA_TYPES: LibraryBrowseMediaType[] = [ - 'Movie', - 'TelevisionShow', - 'Artist' -]; - // ---- Small pure helpers ---------------------------------------------------- const mono: CSSProperties = { fontFamily: 'var(--font-mono)', fontVariantNumeric: 'tabular-nums' }; @@ -516,154 +496,6 @@ function SeasonsDialog({ ); } -// ---- Library browse data hook --------------------------------------------- -interface BrowseState { - status: 'loading' | 'error' | 'success'; - items: LibraryBrowseItem[]; - totalCount: number; - error: string | null; -} - -// Fan out across the "pickable" collection-ish kinds (Collection, SmartCollection, -// MultiCollection, RerunCollection, Playlist). Reports the REAL summed per-kind `totalCount` -// (not `merged.length`, i.e. the truncated page) — #650: returning the page length as the total -// made `canLoadMore`'s `items.length < totalCount` comparison permanently `merged.length >= -// merged.length`, so "load more" could never fire once any one kind hit the 100-row server cap. -// Mirrors loadLibraryItems's per-kind pageNum/totalCount pattern below. -async function loadCollections( - query: string, - pageNum: number, - pageSize: number -): Promise<{ page: LibraryBrowseItem[]; totalCount: number }> { - const results = await Promise.all( - COLLECTION_MEDIA_TYPES.map((mediaType) => getLibraryBrowseItems({ query, mediaType, pageNum, pageSize })) - ); - const merged = results - .flatMap((result) => result.page) - .sort((left, right) => left.title.localeCompare(right.title, undefined, { sensitivity: 'base' })); - const totalCount = results.reduce((sum, result) => sum + result.totalCount, 0); - return { page: merged, totalCount }; -} - -// Fan out across the pickable top-level library kinds (movies/shows/artists) -// instead of the unscoped browse, which now also returns episodes/songs/etc. -// Mirrors loadCollections's per-kind Promise.all pattern, but preserves real -// paging (each kind keeps its own pageNum/totalCount, summed across kinds) so -// canLoadMore stays meaningful for large libraries. -async function loadLibraryItems( - query: string, - libraryId: number | null, - pageNum: number, - pageSize: number -): Promise<{ page: LibraryBrowseItem[]; totalCount: number }> { - const results = await Promise.all( - LIBRARY_MEDIA_TYPES.map((mediaType) => - getLibraryBrowseItems({ - query: query || undefined, - libraryId: libraryId ?? undefined, - mediaType, - pageNum, - pageSize - }) - ) - ); - const merged = results - .flatMap((result) => result.page) - .sort((left, right) => left.title.localeCompare(right.title, undefined, { sensitivity: 'base' })); - const totalCount = results.reduce((sum, result) => sum + result.totalCount, 0); - return { page: merged, totalCount }; -} - -function useLibraryBrowse(source: 'library' | 'collections', query: string, libraryId: number | null) { - const [state, setState] = useState({ status: 'loading', items: [], totalCount: 0, error: null }); - const [loadingMore, setLoadingMore] = useState(false); - // Append-only error, kept separate from `state.error` (#650 follow-up F4): a rejected "load - // more" must NOT flip `state.status` to 'error' — that branch renders instead of the item grid - // and would destroy every already-loaded row with no way back. This renders inline next to the - // (still-present) Load more button so the fetch can simply be retried. - const [loadMoreError, setLoadMoreError] = useState(null); - const reqRef = useRef(0); - const pageRef = useRef(0); - - const runFetch = useCallback( - (pageNum: number, reqId: number, append: boolean) => { - const promise = - source === 'collections' - ? loadCollections(query, pageNum, 100) - : loadLibraryItems(query, libraryId, pageNum, 100); - - promise - .then((result) => { - if (reqRef.current !== reqId) { - return; - } - // Clear unconditionally (not just when THIS request was itself an append): a fresh - // (non-append) fetch that supersedes an in-flight append must also clear any stale - // append error left over from a previous failed "Load more" (#650 follow-up F3/F4). - setLoadMoreError(null); - setState((prev) => ({ - status: 'success', - error: null, - items: append ? [...prev.items, ...result.page] : result.page, - totalCount: result.totalCount - })); - }) - .catch((error: unknown) => { - if (reqRef.current !== reqId) { - return; - } - if (append) { - // Preserve already-rendered items/totalCount — only surface the failure inline so - // "Load more" can be retried, rather than wiping a page's worth of loaded rows on one - // rejected per-kind request (#650 follow-up F4). Roll the page cursor back so the - // retry re-requests the SAME failed page instead of skipping past it. - pageRef.current = Math.max(0, pageRef.current - 1); - setLoadMoreError(messageFromError(error)); - } else { - setState({ status: 'error', items: [], totalCount: 0, error: messageFromError(error) }); - } - }) - .finally(() => { - // Clear whenever THIS request is still the current one — not gated on `append` (#650 - // follow-up F3). A stale append's `finally` is already skipped by the reqId check above - // (a fresh fetch bumps `reqRef.current` past it), so without this a query/library change - // made while a "Load more" request is in flight strands the button in its loading (and - // disabled) state forever: the stale append's own `finally` never matches the current - // reqId, and the superseding fetch is a non-append request that never touched it either. - if (reqRef.current === reqId) { - setLoadingMore(false); - } - }); - }, - [source, query, libraryId] - ); - - // Refetch when inputs change. Following the guide-hook pattern, setState only - // runs inside runFetch's async callbacks - the previous results stay visible - // until the new page resolves (initial 'loading' shows the first-load spinner). - useEffect(() => { - const reqId = reqRef.current + 1; - reqRef.current = reqId; - pageRef.current = 0; - runFetch(0, reqId, false); - }, [runFetch]); - - const loadMore = useCallback(() => { - const nextPage = pageRef.current + 1; - pageRef.current = nextPage; - setLoadingMore(true); - setLoadMoreError(null); - runFetch(nextPage, reqRef.current, true); - }, [runFetch]); - - // #650: `totalCount` is now the real summed per-kind total for both sources (loadCollections - // no longer reports the truncated page length as the total), so this comparison is meaningful - // for 'collections' too — no need to gate it to 'library' only. - const canLoadMore = state.status === 'success' && state.items.length < state.totalCount; - - return { state, loadingMore, loadMore, canLoadMore, loadMoreError }; -} - // ---- Builder support data (templates, pickers, channels) ------------------- interface BuilderData { channels: ChannelSummary[]; diff --git a/web/src/builder/libraryBrowse.ts b/web/src/builder/libraryBrowse.ts new file mode 100644 index 000000000..4e3450efc --- /dev/null +++ b/web/src/builder/libraryBrowse.ts @@ -0,0 +1,230 @@ +import { useCallback, useEffect, useRef, useState } from 'react'; +import { getLibraryBrowseItems, messageFromError, type LibraryBrowseItem, type LibraryBrowseMediaType } from '../api'; + +// Extracted from ChannelBuilder.tsx into its own non-JSX module (#650 follow-up) so +// `useLibraryBrowse` — a plain hook, no component — can be exported without tripping +// `react-refresh/only-export-components` (that lint rule wants a file that exports React +// Fast-Refresh components to export ONLY components). It also lets `ChannelBuilder.test.tsx` +// unit-test the hook's fetch-ownership/rollback guards directly via `renderHook` — the HIGH-2 +// interleaving (two same-generation appends genuinely overlapping) is not reachable through the +// rendered "Load more" button once HIGH-1's single-flight disabling is wired up, so the +// regression test drives the hook directly rather than asserting something the UI can no longer +// produce. + +export const COLLECTION_MEDIA_TYPES: LibraryBrowseMediaType[] = [ + 'Collection', + 'SmartCollection', + 'MultiCollection', + 'RerunCollection', + 'Playlist' +]; + +// Browsing a library shows the "pickable" top-level kinds only, not every +// episode/song/etc. nested underneath them. Kept explicit here since +// `GET /api/v1/library/browse` now spans all 10 media kinds when `mediaType` is +// omitted. TelevisionSeason is intentionally excluded so a multi-season show +// renders as a single tile instead of flooding the grid with per-season tiles +// (issue #180); seasons are reachable via the show tile's Seasons drill-in. +export const LIBRARY_MEDIA_TYPES: LibraryBrowseMediaType[] = ['Movie', 'TelevisionShow', 'Artist']; + +// ---- Library browse data hook --------------------------------------------- +export interface BrowseState { + status: 'loading' | 'error' | 'success'; + items: LibraryBrowseItem[]; + totalCount: number; + error: string | null; +} + +// Fan out across the "pickable" collection-ish kinds (Collection, SmartCollection, +// MultiCollection, RerunCollection, Playlist). Reports the REAL summed per-kind `totalCount` +// (not `merged.length`, i.e. the truncated page) — #650: returning the page length as the total +// made `canLoadMore`'s `items.length < totalCount` comparison permanently `merged.length >= +// merged.length`, so "load more" could never fire once any one kind hit the 100-row server cap. +// Mirrors loadLibraryItems's per-kind pageNum/totalCount pattern below. +async function loadCollections( + query: string, + pageNum: number, + pageSize: number +): Promise<{ page: LibraryBrowseItem[]; totalCount: number }> { + const results = await Promise.all( + COLLECTION_MEDIA_TYPES.map((mediaType) => getLibraryBrowseItems({ query, mediaType, pageNum, pageSize })) + ); + const merged = results + .flatMap((result) => result.page) + .sort((left, right) => left.title.localeCompare(right.title, undefined, { sensitivity: 'base' })); + const totalCount = results.reduce((sum, result) => sum + result.totalCount, 0); + return { page: merged, totalCount }; +} + +// Fan out across the pickable top-level library kinds (movies/shows/artists) +// instead of the unscoped browse, which now also returns episodes/songs/etc. +// Mirrors loadCollections's per-kind Promise.all pattern, but preserves real +// paging (each kind keeps its own pageNum/totalCount, summed across kinds) so +// canLoadMore stays meaningful for large libraries. +async function loadLibraryItems( + query: string, + libraryId: number | null, + pageNum: number, + pageSize: number +): Promise<{ page: LibraryBrowseItem[]; totalCount: number }> { + const results = await Promise.all( + LIBRARY_MEDIA_TYPES.map((mediaType) => + getLibraryBrowseItems({ + query: query || undefined, + libraryId: libraryId ?? undefined, + mediaType, + pageNum, + pageSize + }) + ) + ); + const merged = results + .flatMap((result) => result.page) + .sort((left, right) => left.title.localeCompare(right.title, undefined, { sensitivity: 'base' })); + const totalCount = results.reduce((sum, result) => sum + result.totalCount, 0); + return { page: merged, totalCount }; +} + +export function useLibraryBrowse(source: 'library' | 'collections', query: string, libraryId: number | null) { + const [state, setState] = useState({ status: 'loading', items: [], totalCount: 0, error: null }); + const [loadingMore, setLoadingMore] = useState(false); + // Append-only error, kept separate from `state.error` (#650 follow-up F4): a rejected "load + // more" must NOT flip `state.status` to 'error' — that branch renders instead of the item grid + // and would destroy every already-loaded row with no way back. This renders inline next to the + // (still-present) Load more button so the fetch can simply be retried. + const [loadMoreError, setLoadMoreError] = useState(null); + // Query GENERATION (bumped on query/library/source change) — invalidates every fetch from an + // earlier generation, whether it was a page-0 refresh or an append. + const reqRef = useRef(0); + const pageRef = useRef(0); + // #650 follow-up HIGH-1: `reqRef` alone identifies a query GENERATION, not an individual + // in-flight request — a page-0 refresh and a "Load more" append can be outstanding + // SIMULTANEOUSLY under the same `reqId` (e.g. the user clicks "Load more" while a query change's + // fresh page-0 fetch is still resolving). Comparing only `reqRef.current === reqId` in + // `finally` cannot tell those two fetches apart, so whichever settles FIRST clears + // `loadingMore` — even if it isn't the append the spinner represents, re-enabling the button + // while the real append is still in flight and letting a second click fire an out-of-order or + // duplicate page fetch. Each individual fetch now gets its own monotonic `fetchId`; only the + // fetch that started the CURRENTLY-DISPLAYED spinner (`loadingFetchIdRef`) may clear it, and + // `loadingFetchReqIdRef` records which GENERATION that owning fetch belongs to, so a fresh + // page-0 fetch can tell "this is a same-generation append I must not touch" (HIGH-1) apart from + // "this is an abandoned append from a query the user already left" (the original #650 F3 — + // still needs cleaning up once its generation is truly gone, or the spinner strands forever). + const fetchSeqRef = useRef(0); + const loadingFetchIdRef = useRef(null); + const loadingFetchReqIdRef = useRef(null); + + const runFetch = useCallback( + (pageNum: number, reqId: number, append: boolean) => { + const fetchId = ++fetchSeqRef.current; + if (append) { + loadingFetchIdRef.current = fetchId; + loadingFetchReqIdRef.current = reqId; + } + + const promise = + source === 'collections' + ? loadCollections(query, pageNum, 100) + : loadLibraryItems(query, libraryId, pageNum, 100); + + promise + .then((result) => { + if (reqRef.current !== reqId) { + return; + } + // Clear unconditionally (not just when THIS request was itself an append): a fresh + // (non-append) fetch that supersedes an in-flight append must also clear any stale + // append error left over from a previous failed "Load more" (#650 follow-up F3/F4). + setLoadMoreError(null); + setState((prev) => ({ + status: 'success', + error: null, + items: append ? [...prev.items, ...result.page] : result.page, + totalCount: result.totalCount + })); + }) + .catch((error: unknown) => { + if (reqRef.current !== reqId) { + return; + } + if (append) { + // Preserve already-rendered items/totalCount — only surface the failure inline so + // "Load more" can be retried, rather than wiping a page's worth of loaded rows on one + // rejected per-kind request (#650 follow-up F4). Roll the page cursor back so the + // retry re-requests the SAME failed page — but ONLY if nothing has advanced the + // cursor since this specific fetch started (#650 follow-up HIGH-2): tying the + // rollback to whatever `pageRef.current` currently holds, rather than to the `pageNum` + // THIS fetch actually requested, can under an overlapping-append race decrement a + // cursor a LATER, already-succeeded page already advanced past — silently losing that + // later page and refetching it as a duplicate. A compare-and-set guard (only roll + // back when the cursor is still exactly where this fetch left it) makes the rollback + // a no-op instead of a corruption when that overlap happens. + if (pageRef.current === pageNum) { + pageRef.current = pageNum - 1; + } + setLoadMoreError(messageFromError(error)); + } else { + setState({ status: 'error', items: [], totalCount: 0, error: messageFromError(error) }); + } + }) + .finally(() => { + if (append) { + // Only the fetch that OWNS the currently-displayed spinner may clear it (#650 + // follow-up HIGH-1) — evaluated purely on fetch identity, independent of whether this + // fetch's generation is still current, so an abandoned append still retires its own + // spinner if nothing else already did (the branch below, from the SUPERSEDING + // generation's own page-0 fetch, normally beats it to this). + if (loadingFetchIdRef.current === fetchId) { + setLoadingMore(false); + loadingFetchIdRef.current = null; + loadingFetchReqIdRef.current = null; + } + return; + } + + // This settling fetch is a page-0 refresh. If it belongs to the CURRENT generation and + // there is a tracked append spinner from an OLDER, now-abandoned generation, retire it + // here — that append can never again be "what the user is waiting on" (#650 follow-up + // F3). A SAME-generation append is left untouched; only its own completion (above) may + // clear it (#650 follow-up HIGH-1) — that's what stops the button re-enabling while a + // same-generation append is still genuinely in flight. + if ( + reqRef.current === reqId && + loadingFetchIdRef.current !== null && + loadingFetchReqIdRef.current !== null && + loadingFetchReqIdRef.current !== reqId + ) { + setLoadingMore(false); + loadingFetchIdRef.current = null; + loadingFetchReqIdRef.current = null; + } + }); + }, + [source, query, libraryId] + ); + + // Refetch when inputs change. Following the guide-hook pattern, setState only + // runs inside runFetch's async callbacks - the previous results stay visible + // until the new page resolves (initial 'loading' shows the first-load spinner). + useEffect(() => { + const reqId = reqRef.current + 1; + reqRef.current = reqId; + pageRef.current = 0; + runFetch(0, reqId, false); + }, [runFetch]); + + const loadMore = useCallback(() => { + const nextPage = pageRef.current + 1; + pageRef.current = nextPage; + setLoadingMore(true); + setLoadMoreError(null); + runFetch(nextPage, reqRef.current, true); + }, [runFetch]); + + // #650: `totalCount` is now the real summed per-kind total for both sources (loadCollections + // no longer reports the truncated page length as the total), so this comparison is meaningful + // for 'collections' too — no need to gate it to 'library' only. + const canLoadMore = state.status === 'success' && state.items.length < state.totalCount; + + return { state, loadingMore, loadMore, canLoadMore, loadMoreError }; +}