fix(650): follow-up — per-occurrence guard identity, shorthand pageSize detection, and three UI defects
Cold cross-family (Codex) review of 9763fdca found real defects; addressed as a
follow-up rather than amending that commit.
MUST FIX, addressed:
- F5: pageSizeCallSites.guard.test.ts collapsed call-site identity to
`file:value`, so a SECOND at-cap call in an already-registered file was
invisible (verified: appending a duplicate `getLibraryBrowseItems({
mediaType: 'Movie', pageSize: 100 })` to ChannelBuilder.tsx passed all 4
guard tests before this fix). Identity is now `file:line:kind:value` — a
bracket/quote-tracked scan resolves each occurrence's exact line, so a
duplicate on a new line is a new, unregistered identity.
- F6: the guard now also detects the ES6 shorthand property form (`{ ...,
pageSize }`), not just `pageSize: <value>`. Implemented as a bracket-stack
scan that distinguishes an object-literal `{` (real risk) from a
block-statement `{` or an array `[` (false positives from things like
`useCallback` dependency arrays and `const pageSize = 100;` inside a
function body) by inspecting the token immediately preceding each `{`.
Six real shorthand sites are now registered (the two inside loadAllPages
itself, ChannelBuilder's two per-kind fan-outs, and two genuine
user-adjustable pagers in LogsScreen/BlockPlayoutTroubleshootingScreen).
Object SPREAD and positional-argument pageSize (api/search.ts's
api.search-allitems-paging precedent) remain a documented residual gap,
written down in the test file's own header comment, not silently absent.
- F2 (TraktListsScreen.tsx): an incomplete load with zero accumulated rows
rendered BOTH "List may be incomplete" and the unsupported "No Trakt lists
yet." claim. The zero-row empty state now branches on `incomplete` first.
- F3 (ChannelBuilder.tsx useLibraryBrowse): changing the query/library while
a "Load more" append was in flight stranded the button in its
loading/disabled state forever (the stale append's own `finally` no longer
matched the current request id, and the superseding fresh fetch never
cleared `loadingMore` either). `finally` now clears `loadingMore` whenever
the settling request is still the CURRENT one, regardless of whether that
particular request was an append.
- F4 (ChannelBuilder.tsx useLibraryBrowse): one rejected per-kind request in
an append's `Promise.all` wiped every already-loaded row via
`items: []` with no way back. Append failures now preserve state, surface
the error inline next to a still-present "Load more" button, and roll the
page cursor back so a retry re-requests the same page instead of skipping
it.
Both new UI fixes are pre-existing defects in the 'library' source that
9763fdca's loadCollections fix newly made reachable from 'collections' too.
Verified all four fixes against negative controls: reverted each in turn and
confirmed its dedicated test fails with the expected message, then restored.
DO NOT FIX (filed as timothy/ersatztv#665 instead, bug+frontend+priority:low):
- F1: loadCollections/loadLibraryItems sort each fetched page independently,
so appended pages are only locally sorted, not globally sorted across the
accumulated list.
- F7: an overclaiming totalCount can leave "Load more" clickable after every
kind is actually exhausted (no auto-loop; a user click is still required
each time).
Not touched (reviewer confirmed correct as-is): Trakt sequence/abort/unmount
handling, the Class-A vs Class-B incomplete-copy distinction, and the
`lists.length` footer count.
This commit is contained in:
@@ -4,7 +4,7 @@ import { fileURLToPath } from 'node:url';
|
||||
import { describe, expect, it } from 'vitest';
|
||||
|
||||
/**
|
||||
* #650 guard: an ENUMERATING allow-list over every `pageSize:` call site in the SPA.
|
||||
* #650 guard: an ENUMERATING allow-list over every `pageSize` call site in the SPA.
|
||||
*
|
||||
* #644 fixed every call site that requested an OVER-cap `pageSize` (e.g. `pageSize: 1000`) to
|
||||
* "get everything in one call" — a pattern that silently truncates to the server's `MaxPageSize`
|
||||
@@ -14,27 +14,53 @@ 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 literal `pageSize:` — a
|
||||
* number literal or a const identifier that resolves to one — and cross-checks the discovered
|
||||
* set against a hand-reviewed registry below, in BOTH directions:
|
||||
* the next magic number). It enumerates every call site that passes a `pageSize` property into an
|
||||
* object literal — either `pageSize: <literal-or-const>` or the ES6 shorthand `{ ..., pageSize }`
|
||||
* — 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 literal pageSize, and the registry should shrink
|
||||
* to match, not silently claim coverage of code that no longer exists).
|
||||
* 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).
|
||||
*
|
||||
* **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.
|
||||
*
|
||||
* **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.
|
||||
*
|
||||
* Each registry entry classifies the site per `docs/spa-conventions.md` §3b /
|
||||
* `docs/decisions/records/spa/list-completeness-vs-bounded-pickers.md`:
|
||||
* - 'class-a' — bounded-by-construction list, paged to completeness via `loadAllPages`,
|
||||
* with a `complete`/`incomplete` flag surfaced (never silently partial).
|
||||
* - 'class-b' — media-library-scoped picker: one bounded page at (or under) the cap, with
|
||||
* the real truncation (`totalCount` vs items shown) surfaced to the user.
|
||||
* - 'paged-ui' — real paging UI (a page/"load more" control keyed to a genuine
|
||||
* `totalCount`), so a `pageSize` at or below the cap is correct as-is.
|
||||
* - 'class-a' — bounded-by-construction list, paged to completeness via `loadAllPages` (or,
|
||||
* for the two sites INSIDE `loadAllPages` itself, its implementation), with a
|
||||
* `complete`/`incomplete` flag surfaced (never silently partial).
|
||||
* - 'class-b' — media-library-scoped picker: one bounded page at (or under) the cap, with the
|
||||
* real truncation (`totalCount` vs items shown) surfaced to the user.
|
||||
* - '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.
|
||||
*/
|
||||
|
||||
interface RegistryEntry {
|
||||
file: string;
|
||||
line: number;
|
||||
kind: 'literal' | 'shorthand';
|
||||
value: string;
|
||||
classification: 'class-a' | 'class-b' | 'paged-ui';
|
||||
note: string;
|
||||
@@ -42,8 +68,28 @@ interface RegistryEntry {
|
||||
|
||||
// Keep in file order, then line order, so a diff against the discovered set is easy to read.
|
||||
const REGISTRY: RegistryEntry[] = [
|
||||
{
|
||||
file: 'api/paging.ts',
|
||||
line: 83,
|
||||
kind: 'shorthand',
|
||||
value: 'pageSize',
|
||||
classification: 'class-a',
|
||||
note:
|
||||
"loadAllPages's own first-page fetch. This IS the Class A completeness helper every other " +
|
||||
'bounded list uses — not a defect, the fix itself.'
|
||||
},
|
||||
{
|
||||
file: 'api/paging.ts',
|
||||
line: 93,
|
||||
kind: 'shorthand',
|
||||
value: 'pageSize',
|
||||
classification: 'class-a',
|
||||
note: "loadAllPages's subsequent-page fetch inside the completeness loop; same helper as line 83."
|
||||
},
|
||||
{
|
||||
file: 'builder/ChannelBuilder.tsx',
|
||||
line: 447,
|
||||
kind: 'literal',
|
||||
value: '100',
|
||||
classification: 'class-b',
|
||||
note:
|
||||
@@ -52,16 +98,65 @@ const REGISTRY: RegistryEntry[] = [
|
||||
"but #650 found the response's totalCount went unread; it is now surfaced as a " +
|
||||
"'Showing the first N of M seasons' hint if a show somehow exceeds the cap."
|
||||
},
|
||||
{
|
||||
file: 'builder/ChannelBuilder.tsx',
|
||||
line: 539,
|
||||
kind: 'shorthand',
|
||||
value: 'pageSize',
|
||||
classification: 'paged-ui',
|
||||
note:
|
||||
"loadCollections's per-kind fan-out (#650 fix): forwards a real pageNum/pageSize from the " +
|
||||
"caller and sums each kind's real totalCount, so the builder's Load more button (canLoadMore) " +
|
||||
'is meaningful — this is the paged-ui replacement for the original truncating implementation.'
|
||||
},
|
||||
{
|
||||
file: 'builder/ChannelBuilder.tsx',
|
||||
line: 566,
|
||||
kind: 'shorthand',
|
||||
value: 'pageSize',
|
||||
classification: 'paged-ui',
|
||||
note: "loadLibraryItems's per-kind fan-out — same real pageNum/pageSize/totalCount pattern as loadCollections above."
|
||||
},
|
||||
{
|
||||
file: 'builder/SmartCollectionDialog.tsx',
|
||||
line: 92,
|
||||
kind: 'literal',
|
||||
value: '24',
|
||||
classification: 'class-b',
|
||||
note:
|
||||
'Inline smart-query preview: a small bounded sample (well under the 100 cap) shown while ' +
|
||||
'authoring a query; the query itself is the narrowing mechanism, not paging.'
|
||||
},
|
||||
{
|
||||
file: 'screens/AutoTuneScreen.tsx',
|
||||
line: 1527,
|
||||
kind: 'literal',
|
||||
value: 'MEMBER_PREVIEW_SIZE',
|
||||
classification: 'class-b',
|
||||
note: "Channel-member preview; renders 'showing first N' once totalCount exceeds the preview size."
|
||||
},
|
||||
{
|
||||
file: 'screens/AutoTuneScreen.tsx',
|
||||
line: 1556,
|
||||
kind: 'literal',
|
||||
value: 'ADD_SOURCE_RESULTS',
|
||||
classification: 'class-b',
|
||||
note: 'Tiny (8-row) debounced add-source search preview, far under the cap; query narrows results.'
|
||||
},
|
||||
{
|
||||
file: 'screens/BlockPlayoutTroubleshootingScreen.tsx',
|
||||
line: 161,
|
||||
kind: 'shorthand',
|
||||
value: 'pageSize',
|
||||
classification: 'paged-ui',
|
||||
note:
|
||||
'Playout block history: forwards a user-adjustable `pageSize` state (persisted, backed by a ' +
|
||||
'page-size <Select>) to a real pager keyed off the response totalCount.'
|
||||
},
|
||||
{
|
||||
file: 'screens/CollectionsScreen.tsx',
|
||||
line: 234,
|
||||
kind: 'literal',
|
||||
value: '50',
|
||||
classification: 'class-b',
|
||||
note:
|
||||
@@ -69,31 +164,51 @@ const REGISTRY: RegistryEntry[] = [
|
||||
'narrows results; capped to a slice(0, 50) preview by design, not a truncated "whole list".'
|
||||
},
|
||||
{
|
||||
file: 'screens/MediaDetailScreen.tsx',
|
||||
value: 'CHILD_PAGE_SIZE',
|
||||
file: 'screens/FillerPresetsScreen.tsx',
|
||||
line: 501,
|
||||
kind: 'literal',
|
||||
value: 'LIBRARY_BROWSE_PAGE_CAP',
|
||||
classification: 'class-b',
|
||||
note: 'Media-library picker pattern shared with RerunCollectionsScreen, for filler preset content.'
|
||||
},
|
||||
{
|
||||
file: 'screens/LogsScreen.tsx',
|
||||
line: 106,
|
||||
kind: 'shorthand',
|
||||
value: 'pageSize',
|
||||
classification: 'paged-ui',
|
||||
note: 'Season/episode child list has a real page-number pager driven off the real totalCount.'
|
||||
note:
|
||||
'Log listing: forwards a user-adjustable `pageSize` state (persisted, backed by a page-size ' +
|
||||
'<Select>) to a real pager keyed off the response totalCount.'
|
||||
},
|
||||
{
|
||||
file: 'screens/MediaBrowseScreen.tsx',
|
||||
line: 119,
|
||||
kind: 'literal',
|
||||
value: 'PAGE_SIZE',
|
||||
classification: 'paged-ui',
|
||||
note: 'Library browse grid has a real page-number pager driven off the real totalCount.'
|
||||
},
|
||||
{
|
||||
file: 'screens/TrashScreen.tsx',
|
||||
value: 'PAGE_SIZE',
|
||||
file: 'screens/MediaDetailScreen.tsx',
|
||||
line: 267,
|
||||
kind: 'literal',
|
||||
value: 'CHILD_PAGE_SIZE',
|
||||
classification: 'paged-ui',
|
||||
note: 'Per-group trash listing; "See all N" load-more gated on totalCount > items.length.'
|
||||
note: 'Season/episode child list has a real page-number pager driven off the real totalCount.'
|
||||
},
|
||||
{
|
||||
file: 'screens/SearchScreen.tsx',
|
||||
value: 'PAGE_SIZE',
|
||||
classification: 'paged-ui',
|
||||
note: 'Per-group search results; hasMore gated on totalCount > items.length with a load-more.'
|
||||
file: 'screens/PlaylistsScreen.tsx',
|
||||
line: 193,
|
||||
kind: 'literal',
|
||||
value: 'LIBRARY_BROWSE_PAGE_CAP',
|
||||
classification: 'class-b',
|
||||
note: 'Media-library picker pattern shared with RerunCollectionsScreen, for playlist entries.'
|
||||
},
|
||||
{
|
||||
file: 'screens/RerunCollectionsScreen.tsx',
|
||||
line: 146,
|
||||
kind: 'literal',
|
||||
value: 'LIBRARY_BROWSE_PAGE_CAP',
|
||||
classification: 'class-b',
|
||||
note:
|
||||
@@ -101,28 +216,28 @@ const REGISTRY: RegistryEntry[] = [
|
||||
"'Showing the first N of M — use search to narrow' hint (list-completeness-vs-bounded-pickers)."
|
||||
},
|
||||
{
|
||||
file: 'screens/PlaylistsScreen.tsx',
|
||||
value: 'LIBRARY_BROWSE_PAGE_CAP',
|
||||
classification: 'class-b',
|
||||
note: 'Same media-library picker pattern as RerunCollectionsScreen, for playlist entries.'
|
||||
file: 'screens/SearchScreen.tsx',
|
||||
line: 118,
|
||||
kind: 'literal',
|
||||
value: 'PAGE_SIZE',
|
||||
classification: 'paged-ui',
|
||||
note: 'Per-group search results; hasMore gated on totalCount > items.length with a load-more.'
|
||||
},
|
||||
{
|
||||
file: 'screens/FillerPresetsScreen.tsx',
|
||||
value: 'LIBRARY_BROWSE_PAGE_CAP',
|
||||
classification: 'class-b',
|
||||
note: 'Same media-library picker pattern as RerunCollectionsScreen, for filler preset content.'
|
||||
file: 'screens/TrashScreen.tsx',
|
||||
line: 82,
|
||||
kind: 'literal',
|
||||
value: 'PAGE_SIZE',
|
||||
classification: 'paged-ui',
|
||||
note: 'Per-group trash listing; "See all N" load-more gated on totalCount > items.length.'
|
||||
},
|
||||
{
|
||||
file: 'screens/AutoTuneScreen.tsx',
|
||||
value: 'MEMBER_PREVIEW_SIZE',
|
||||
classification: 'class-b',
|
||||
note: "Channel-member preview; renders 'showing first N' once totalCount exceeds the preview size."
|
||||
},
|
||||
{
|
||||
file: 'screens/AutoTuneScreen.tsx',
|
||||
value: 'ADD_SOURCE_RESULTS',
|
||||
classification: 'class-b',
|
||||
note: 'Tiny (8-row) debounced add-source search preview, far under the cap; query narrows results.'
|
||||
file: 'screens/TrashScreen.tsx',
|
||||
line: 113,
|
||||
kind: 'literal',
|
||||
value: 'PAGE_SIZE',
|
||||
classification: 'paged-ui',
|
||||
note: 'Same per-group trash listing as line 82, the load-more request handler.'
|
||||
}
|
||||
];
|
||||
|
||||
@@ -132,8 +247,11 @@ 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_CALL_SITE = /\bpageSize:\s*(\d+|[A-Za-z_][A-Za-z0-9_]*)\b/g;
|
||||
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[] = [];
|
||||
@@ -153,8 +271,130 @@ 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;
|
||||
kind: 'literal' | 'shorthand';
|
||||
value: string;
|
||||
}
|
||||
|
||||
@@ -164,54 +404,55 @@ function discoverPageSizeCallSites(): DiscoveredSite[] {
|
||||
|
||||
for (const absPath of files) {
|
||||
const relPath = relative(SRC_DIR, absPath);
|
||||
const text = readFileSync(absPath, 'utf8');
|
||||
const codeText = stripComments(readFileSync(absPath, 'utf8'));
|
||||
|
||||
for (const line of text.split('\n')) {
|
||||
// Skip a `//` line comment referencing `pageSize:` in prose (e.g. SchedulesScreen.tsx's
|
||||
// explanatory comment about the server cap) — a real call site is code, not commentary.
|
||||
const commentStart = line.indexOf('//');
|
||||
const codePart = commentStart >= 0 ? line.slice(0, commentStart) : line;
|
||||
|
||||
for (const match of codePart.matchAll(PAGE_SIZE_CALL_SITE)) {
|
||||
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 (codePart[afterIndex] === '(') {
|
||||
continue;
|
||||
}
|
||||
sites.push({ file: relPath, value });
|
||||
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' });
|
||||
}
|
||||
}
|
||||
|
||||
return sites;
|
||||
}
|
||||
|
||||
function siteId(site: { file: string; line: number; kind: string; value: string }): string {
|
||||
return `${site.file}:${site.line}:${site.kind}:${site.value}`;
|
||||
}
|
||||
|
||||
describe('pageSize call-site guard (#650)', () => {
|
||||
it('scans a healthy number of source files (anti-vacuity: a broken glob must not pass on zero input)', () => {
|
||||
const files = listSourceFiles(SRC_DIR);
|
||||
expect(files.length).toBeGreaterThan(50);
|
||||
});
|
||||
|
||||
it('discovers a healthy number of pageSize call sites (anti-vacuity: a broken regex must not pass on zero matches)', () => {
|
||||
it('discovers a healthy number of pageSize call sites (anti-vacuity: a broken scan must not pass on zero matches)', () => {
|
||||
const sites = discoverPageSizeCallSites();
|
||||
expect(sites.length).toBeGreaterThan(10);
|
||||
});
|
||||
|
||||
it('matches the discovered pageSize call sites EXACTLY against the reviewed registry (not a non-empty check)', () => {
|
||||
const discovered = discoverPageSizeCallSites()
|
||||
.map((site) => `${site.file}:${site.value}`)
|
||||
.sort();
|
||||
const registered = REGISTRY.map((entry) => `${entry.file}:${entry.value}`).sort();
|
||||
const discovered = discoverPageSizeCallSites().map(siteId).sort();
|
||||
const registered = REGISTRY.map(siteId).sort();
|
||||
|
||||
// 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.
|
||||
// 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));
|
||||
|
||||
|
||||
@@ -98,6 +98,7 @@ function requestBodyFor(path: string): Record<string, unknown> {
|
||||
|
||||
function mockBuilderApi({
|
||||
artworkUploadFailure = null,
|
||||
browseErrorOn = null,
|
||||
browseHandler = null,
|
||||
browseItems = [],
|
||||
channels = [],
|
||||
@@ -109,7 +110,15 @@ function mockBuilderApi({
|
||||
fromLineupResponse = { channelId: 1, playlistId: null, playoutId: 3, programScheduleId: 2 }
|
||||
}: {
|
||||
artworkUploadFailure?: { status: number } | null;
|
||||
browseHandler?: ((search: URLSearchParams) => { page: unknown[]; totalCount: number }) | null;
|
||||
// Predicate over a /api/v1/library/browse request's query params: when it returns true, that
|
||||
// ONE request answers with a 500 instead of a page (#650 follow-up F4 — simulates one per-kind
|
||||
// fan-out request rejecting mid-Promise.all).
|
||||
browseErrorOn?: ((search: URLSearchParams) => boolean) | null;
|
||||
// May return the page synchronously OR a Promise of one (#650 follow-up F3 — lets a test hold a
|
||||
// specific request open to simulate a superseded in-flight append).
|
||||
browseHandler?:
|
||||
| ((search: URLSearchParams) => { page: unknown[]; totalCount: number } | Promise<{ page: unknown[]; totalCount: number }>)
|
||||
| null;
|
||||
browseItems?: unknown[];
|
||||
channels?: unknown[];
|
||||
channelTemplates?: unknown[];
|
||||
@@ -138,8 +147,13 @@ function mockBuilderApi({
|
||||
|
||||
if (path.startsWith('/api/v1/library/browse')) {
|
||||
const search = new URL(path, window.location.origin).searchParams;
|
||||
|
||||
if (browseErrorOn?.(search)) {
|
||||
return Promise.resolve(new Response(null, { status: 500 }));
|
||||
}
|
||||
|
||||
if (browseHandler) {
|
||||
return Promise.resolve(jsonResponse(browseHandler(search)));
|
||||
return Promise.resolve(browseHandler(search)).then((body) => jsonResponse(body));
|
||||
}
|
||||
|
||||
const mediaType = search.get('mediaType');
|
||||
@@ -708,6 +722,116 @@ describe('Channel Builder (#89)', () => {
|
||||
expect(screen.queryByRole('button', { name: 'Load more' })).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it(
|
||||
'preserves already-loaded rows when a "Load more" append request fails, and lets it be retried ' +
|
||||
'(#650 follow-up F4)',
|
||||
async () => {
|
||||
const page0 = [browseItem({ id: 1, mediaItemId: 1, title: 'Item A' })];
|
||||
// Loaded once via `Load more`, then again on retry after the first attempt fails.
|
||||
const page1 = [browseItem({ id: 2, mediaItemId: 2, title: 'Item B' })];
|
||||
let page1Attempts = 0;
|
||||
|
||||
mockBuilderApi({
|
||||
...builderDefaults(),
|
||||
browseHandler: (search) => {
|
||||
if (search.get('mediaType') !== 'TelevisionShow') {
|
||||
return { page: [], totalCount: 0 };
|
||||
}
|
||||
const pageNum = Number(search.get('pageNum') ?? '0');
|
||||
if (pageNum === 0) {
|
||||
return { page: page0, totalCount: 3 };
|
||||
}
|
||||
page1Attempts += 1;
|
||||
if (page1Attempts === 1) {
|
||||
return Promise.reject(new Error('network blip'));
|
||||
}
|
||||
return { page: page1, totalCount: 3 };
|
||||
}
|
||||
});
|
||||
|
||||
await renderBuilder();
|
||||
|
||||
expect(await screen.findByText('Item A')).toBeInTheDocument();
|
||||
|
||||
const loadMoreButton = screen.getByRole('button', { name: 'Load more' });
|
||||
fireEvent.click(loadMoreButton);
|
||||
|
||||
// The rejected append must NOT wipe Item A — only report the failure inline.
|
||||
expect(await screen.findByText('network blip')).toBeInTheDocument();
|
||||
expect(screen.getByText('Item A')).toBeInTheDocument();
|
||||
expect(screen.queryByText('No titles match.')).not.toBeInTheDocument();
|
||||
|
||||
// The button survives the failure so the SAME page can be retried (the page cursor rolls
|
||||
// back rather than skipping ahead to a page that was never fetched).
|
||||
const retryButton = screen.getByRole('button', { name: 'Load more' });
|
||||
fireEvent.click(retryButton);
|
||||
|
||||
expect(await screen.findByText('Item B')).toBeInTheDocument();
|
||||
expect(screen.getByText('Item A')).toBeInTheDocument();
|
||||
expect(page1Attempts).toBe(2);
|
||||
}
|
||||
);
|
||||
|
||||
it(
|
||||
'clears the "Load more" spinner instead of stranding it when the query changes while an ' +
|
||||
'append request is still in flight (#650 follow-up F3)',
|
||||
async () => {
|
||||
const page0ForA = [browseItem({ id: 1, mediaItemId: 1, title: 'Item A' })];
|
||||
const page0ForZ = [browseItem({ id: 9, mediaItemId: 9, title: 'Item Z' })];
|
||||
|
||||
let releaseStrandedAppend: (() => void) | null = null;
|
||||
const strandedAppendGate = new Promise<void>((resolve) => {
|
||||
releaseStrandedAppend = resolve;
|
||||
});
|
||||
|
||||
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 (pageNum === 0) {
|
||||
return { page: query === 'zzz' ? page0ForZ : page0ForA, totalCount: 3 };
|
||||
}
|
||||
|
||||
// The one append (page 1) request never resolves until the test releases it — long
|
||||
// after the query change below has superseded it.
|
||||
await strandedAppendGate;
|
||||
return { page: [browseItem({ id: 2, mediaItemId: 2, title: 'Item B' })], totalCount: 3 };
|
||||
}
|
||||
});
|
||||
|
||||
await renderBuilder();
|
||||
|
||||
expect(await screen.findByText('Item A')).toBeInTheDocument();
|
||||
|
||||
const loadMoreButton = screen.getByRole('button', { name: 'Load more' });
|
||||
fireEvent.click(loadMoreButton);
|
||||
expect((loadMoreButton as HTMLButtonElement).disabled).toBe(true);
|
||||
|
||||
// Change the query while the append is still in flight — this supersedes it with a fresh
|
||||
// (non-append) page-0 fetch.
|
||||
fireEvent.change(screen.getByPlaceholderText('Search shows & movies…'), { target: { value: 'zzz' } });
|
||||
|
||||
// Without the F3 fix, the superseded append's `finally` never fires its `setLoadingMore(false)`
|
||||
// (its own request id no longer matches) AND the new fetch's `finally` only clears it when
|
||||
// `append` is true — so the button would stay disabled/loading forever.
|
||||
expect(await screen.findByText('Item Z')).toBeInTheDocument();
|
||||
await waitFor(() => {
|
||||
expect((screen.getByRole('button', { name: 'Load more' }) as HTMLButtonElement).disabled).toBe(false);
|
||||
});
|
||||
|
||||
// Let the stranded append resolve too, after the fact — it must not resurrect stale rows.
|
||||
releaseStrandedAppend?.();
|
||||
await waitFor(() => expect(screen.getByText('Item Z')).toBeInTheDocument());
|
||||
expect(screen.queryByText('Item A')).not.toBeInTheDocument();
|
||||
}
|
||||
);
|
||||
|
||||
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)',
|
||||
|
||||
@@ -577,6 +577,11 @@ async function loadLibraryItems(
|
||||
function useLibraryBrowse(source: 'library' | 'collections', query: string, libraryId: number | null) {
|
||||
const [state, setState] = useState<BrowseState>({ 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<string | null>(null);
|
||||
const reqRef = useRef(0);
|
||||
const pageRef = useRef(0);
|
||||
|
||||
@@ -592,6 +597,10 @@ function useLibraryBrowse(source: 'library' | 'collections', query: string, libr
|
||||
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,
|
||||
@@ -603,10 +612,25 @@ function useLibraryBrowse(source: 'library' | 'collections', query: string, libr
|
||||
if (reqRef.current !== reqId) {
|
||||
return;
|
||||
}
|
||||
setState({ status: 'error', items: [], totalCount: 0, error: messageFromError(error) });
|
||||
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(() => {
|
||||
if (reqRef.current === reqId && append) {
|
||||
// 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);
|
||||
}
|
||||
});
|
||||
@@ -628,6 +652,7 @@ function useLibraryBrowse(source: 'library' | 'collections', query: string, libr
|
||||
const nextPage = pageRef.current + 1;
|
||||
pageRef.current = nextPage;
|
||||
setLoadingMore(true);
|
||||
setLoadMoreError(null);
|
||||
runFetch(nextPage, reqRef.current, true);
|
||||
}, [runFetch]);
|
||||
|
||||
@@ -636,7 +661,7 @@ function useLibraryBrowse(source: 'library' | 'collections', query: string, libr
|
||||
// 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 };
|
||||
return { state, loadingMore, loadMore, canLoadMore, loadMoreError };
|
||||
}
|
||||
|
||||
// ---- Builder support data (templates, pickers, channels) -------------------
|
||||
@@ -1306,6 +1331,11 @@ function ChannelBuilder({
|
||||
<Button variant="secondary" size="sm" loading={browse.loadingMore} onClick={browse.loadMore}>
|
||||
Load more
|
||||
</Button>
|
||||
{browse.loadMoreError && (
|
||||
<span className="ctv-field-error" role="alert">
|
||||
{browse.loadMoreError}
|
||||
</span>
|
||||
)}
|
||||
</div>
|
||||
)}
|
||||
</>
|
||||
|
||||
@@ -107,4 +107,30 @@ describe('TraktListsScreen', () => {
|
||||
expect(screen.getByText('List may be incomplete — retry to reload')).toBeInTheDocument();
|
||||
expect(screen.getByText('1 list')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it(
|
||||
'does NOT claim "No Trakt lists yet" when zero rows accumulated from an incomplete load ' +
|
||||
'(#650 follow-up F2)',
|
||||
async () => {
|
||||
// Every page comes back empty even though totalCount (50) never converges, so
|
||||
// loadAllPages resolves { items: [], complete: false } — zero rows AND incomplete at the
|
||||
// same time. The empty-state text is an unsupported positive claim ("there are zero lists")
|
||||
// when the load never actually finished; only the incomplete badge/message should show.
|
||||
vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL) => {
|
||||
const url = new URL(input.toString(), 'http://localhost');
|
||||
if (url.pathname === '/api/v1/trakt/lists') {
|
||||
return Promise.resolve(jsonResponse({ page: [], totalCount: 50 }));
|
||||
}
|
||||
if (url.pathname === '/api/v1/trakt/status') {
|
||||
return Promise.resolve(jsonResponse({ busy: false }));
|
||||
}
|
||||
return Promise.resolve(new Response(null, { status: 204 }));
|
||||
});
|
||||
|
||||
render(<TraktListsScreen />);
|
||||
|
||||
expect(await screen.findByText('List may be incomplete — retry to reload')).toBeInTheDocument();
|
||||
expect(screen.queryByText('No Trakt lists yet.')).not.toBeInTheDocument();
|
||||
}
|
||||
);
|
||||
});
|
||||
|
||||
@@ -456,6 +456,11 @@ export function TraktListsScreen() {
|
||||
<Spinner size={18} />
|
||||
<span>Loading Trakt lists…</span>
|
||||
</div>
|
||||
) : lists.length === 0 && incomplete ? (
|
||||
// #650 follow-up F2: an incomplete load with zero accumulated rows must not also claim
|
||||
// "No Trakt lists yet." — that's an unsupported positive claim when the load never
|
||||
// finished. The "may be incomplete" badge above already carries the real state.
|
||||
<div className="ctv-collections-empty">Unable to load Trakt lists — retry to reload.</div>
|
||||
) : lists.length === 0 ? (
|
||||
<div className="ctv-collections-empty">No Trakt lists yet.</div>
|
||||
) : (
|
||||
|
||||
Reference in New Issue
Block a user