fix(650): page Trakt lists to completeness and report real totals in loadCollections
Two SPA list loads requested EXACTLY the server's pageSize cap (100), truncating identically to #634/#644's over-cap defect but invisible to that fix's manual "pageSize above the cap" grep: - TraktListsScreen requested pageSize:100 and rendered BOTH the truncated page AND the real totalCount, so 101 lists showed as "101 lists" over a 100-row table. Trakt lists are bounded-by-construction (Class A), so this now pages to completeness via the shared loadAllPages helper, surfaces an "incomplete" badge if a page ever comes back short of totalCount, and passes an AbortSignal from the effect cleanup. - ChannelBuilder's loadCollections (fanning out per collection kind) reported the truncated merged.length as totalCount, so canLoadMore's `items.length < totalCount` comparison was permanently false and "load more" could never fire. It now sums the real per-kind totalCount, mirroring the existing loadLibraryItems pattern in the same file, and canLoadMore is no longer gated to the 'library' source only. Also found and fixed a third at-cap site not named in #650: ChannelBuilder's SeasonsDialog (TelevisionSeason browse scoped to one show) reads pageSize:100 but never read the response's totalCount. No real show has 100+ seasons, so this stays a single bounded page (Class B) rather than paging to completeness, but now surfaces a "Showing the first N of M seasons" hint instead of silently truncating if a show somehow exceeds the cap. Codifies the missing completeness guard as an enumerating allow-list vitest test (web/src/api/pageSizeCallSites.guard.test.ts): scans every `pageSize:` call site in the SPA and diffs it against a hand-reviewed registry in both directions (unregistered site = new defect risk, stale entry = registry rot), with anti-vacuity floors on files-scanned and sites-discovered. Verified the guard actually fails on a planted defect and a planted stale entry before finalizing it. No server-side change: the client pages, the server stays bounded (api.search-allitems-paging precedent).
This commit is contained in:
@@ -0,0 +1,228 @@
|
||||
import { readFileSync, readdirSync } from 'node:fs';
|
||||
import { dirname, join, relative } from 'node:path';
|
||||
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.
|
||||
*
|
||||
* #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`
|
||||
* (100 today) with no error and no truncation indicator. #644's own completeness check (box 4)
|
||||
* was a manual grep for an inflated `pageSize`, which is why it could not see #650: two call
|
||||
* sites requesting EXACTLY the cap (100) truncate exactly as much as an over-cap request, they
|
||||
* 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:
|
||||
* - 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).
|
||||
*
|
||||
* 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.
|
||||
*/
|
||||
|
||||
interface RegistryEntry {
|
||||
file: string;
|
||||
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.
|
||||
const REGISTRY: RegistryEntry[] = [
|
||||
{
|
||||
file: 'builder/ChannelBuilder.tsx',
|
||||
value: '100',
|
||||
classification: 'class-b',
|
||||
note:
|
||||
'SeasonsDialog: TelevisionSeason browse scoped to one show (parentId). No real show has ' +
|
||||
"100+ seasons, so this stays a single bounded page rather than paging to completeness — " +
|
||||
"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/SmartCollectionDialog.tsx',
|
||||
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/CollectionsScreen.tsx',
|
||||
value: '50',
|
||||
classification: 'class-b',
|
||||
note:
|
||||
'AddItemsDialog per-kind library search preview (well under the 100 cap): a typed query ' +
|
||||
'narrows results; capped to a slice(0, 50) preview by design, not a truncated "whole list".'
|
||||
},
|
||||
{
|
||||
file: 'screens/MediaDetailScreen.tsx',
|
||||
value: 'CHILD_PAGE_SIZE',
|
||||
classification: 'paged-ui',
|
||||
note: 'Season/episode child list has a real page-number pager driven off the real totalCount.'
|
||||
},
|
||||
{
|
||||
file: 'screens/MediaBrowseScreen.tsx',
|
||||
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',
|
||||
classification: 'paged-ui',
|
||||
note: 'Per-group trash listing; "See all N" load-more gated on totalCount > items.length.'
|
||||
},
|
||||
{
|
||||
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/RerunCollectionsScreen.tsx',
|
||||
value: 'LIBRARY_BROWSE_PAGE_CAP',
|
||||
classification: 'class-b',
|
||||
note:
|
||||
'Media-library picker backing a rerun-collection <select>; single bounded page with a ' +
|
||||
"'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/FillerPresetsScreen.tsx',
|
||||
value: 'LIBRARY_BROWSE_PAGE_CAP',
|
||||
classification: 'class-b',
|
||||
note: 'Same media-library picker pattern as RerunCollectionsScreen, for filler preset content.'
|
||||
},
|
||||
{
|
||||
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.'
|
||||
}
|
||||
];
|
||||
|
||||
// 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_CALL_SITE = /\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']);
|
||||
|
||||
function listSourceFiles(dir: string): string[] {
|
||||
const out: string[] = [];
|
||||
for (const entry of readdirSync(dir, { withFileTypes: true })) {
|
||||
if (entry.name === 'generated') {
|
||||
continue;
|
||||
}
|
||||
const full = join(dir, entry.name);
|
||||
if (entry.isDirectory()) {
|
||||
out.push(...listSourceFiles(full));
|
||||
continue;
|
||||
}
|
||||
if (/\.(ts|tsx)$/.test(entry.name) && !/\.test\.tsx?$/.test(entry.name) && !entry.name.endsWith('.guard.test.ts')) {
|
||||
out.push(full);
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
interface DiscoveredSite {
|
||||
file: string;
|
||||
value: string;
|
||||
}
|
||||
|
||||
function discoverPageSizeCallSites(): DiscoveredSite[] {
|
||||
const files = listSourceFiles(SRC_DIR);
|
||||
const sites: DiscoveredSite[] = [];
|
||||
|
||||
for (const absPath of files) {
|
||||
const relPath = relative(SRC_DIR, absPath);
|
||||
const text = 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 });
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return sites;
|
||||
}
|
||||
|
||||
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)', () => {
|
||||
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();
|
||||
|
||||
// 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.
|
||||
const unregistered = discovered.filter((site) => !registered.includes(site));
|
||||
const stale = registered.filter((site) => !discovered.includes(site));
|
||||
|
||||
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([]);
|
||||
});
|
||||
|
||||
it('every registry entry documents its class per docs/spa-conventions.md §3b', () => {
|
||||
for (const entry of REGISTRY) {
|
||||
expect(['class-a', 'class-b', 'paged-ui']).toContain(entry.classification);
|
||||
expect(entry.note.length).toBeGreaterThan(20);
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -708,6 +708,97 @@ describe('Channel Builder (#89)', () => {
|
||||
expect(screen.queryByRole('button', { name: 'Load more' })).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)',
|
||||
async () => {
|
||||
// Boundary: 100 vs 101 Collection-kind rows. #650's bug reported `merged.length` as
|
||||
// `totalCount`, so `items.length < totalCount` was always `100 < 100` (false) — this pins
|
||||
// that canLoadMore now reads the real totalCount (101) and DOES fire past the cap.
|
||||
const page0 = Array.from({ length: 100 }, (_, i) =>
|
||||
browseItem({
|
||||
id: i + 1,
|
||||
mediaItemId: undefined,
|
||||
collectionId: i + 1,
|
||||
collectionType: 'Collection',
|
||||
mediaType: 'Collection',
|
||||
title: `Coll ${String(i + 1).padStart(3, '0')}`
|
||||
})
|
||||
);
|
||||
const page1 = [
|
||||
browseItem({
|
||||
id: 101,
|
||||
mediaItemId: undefined,
|
||||
collectionId: 101,
|
||||
collectionType: 'Collection',
|
||||
mediaType: 'Collection',
|
||||
title: 'Coll 101'
|
||||
})
|
||||
];
|
||||
|
||||
mockBuilderApi({
|
||||
...builderDefaults(),
|
||||
browseHandler: (search) => {
|
||||
if (search.get('mediaType') !== 'Collection') {
|
||||
return { page: [], totalCount: 0 };
|
||||
}
|
||||
const pageNum = Number(search.get('pageNum') ?? '0');
|
||||
return { page: pageNum === 0 ? page0 : page1, totalCount: 101 };
|
||||
}
|
||||
});
|
||||
|
||||
await renderBuilder();
|
||||
fireEvent.click(screen.getByRole('button', { name: 'Collections' }));
|
||||
|
||||
expect(await screen.findByText('Coll 001')).toBeInTheDocument();
|
||||
expect(screen.getByText('Coll 100')).toBeInTheDocument();
|
||||
expect(screen.queryByText('Coll 101')).not.toBeInTheDocument();
|
||||
|
||||
const loadMoreButton = screen.getByRole('button', { name: 'Load more' });
|
||||
fireEvent.click(loadMoreButton);
|
||||
|
||||
expect(await screen.findByText('Coll 101')).toBeInTheDocument();
|
||||
expect(screen.getByText('Coll 001')).toBeInTheDocument();
|
||||
expect(screen.queryByRole('button', { name: 'Load more' })).not.toBeInTheDocument();
|
||||
}
|
||||
);
|
||||
|
||||
it('surfaces a truncation hint in the Seasons drill-in when a show has more than 100 seasons (#650)', async () => {
|
||||
// TelevisionSeason browse is scoped to one show (parentId) and requests a single page at the
|
||||
// cap (Class B, spa-conventions §3b) — no real show has 100+ seasons, but the response's
|
||||
// totalCount must still be read and surfaced rather than silently truncating if it somehow did.
|
||||
const seasons = Array.from({ length: 100 }, (_, i) =>
|
||||
browseItem({
|
||||
id: 200 + i,
|
||||
mediaItemId: 200 + i,
|
||||
mediaType: 'TelevisionSeason',
|
||||
title: `Season ${i + 1}`
|
||||
})
|
||||
);
|
||||
|
||||
mockBuilderApi({
|
||||
...builderDefaults(),
|
||||
browseItems: [browseItem()],
|
||||
browseHandler: (search) => {
|
||||
if (search.get('mediaType') === 'TelevisionSeason') {
|
||||
return { page: seasons, totalCount: 140 };
|
||||
}
|
||||
if (search.get('mediaType') === 'TelevisionShow') {
|
||||
return { page: [browseItem()], totalCount: 1 };
|
||||
}
|
||||
return { page: [], totalCount: 0 };
|
||||
}
|
||||
});
|
||||
|
||||
await renderBuilder();
|
||||
expect(await screen.findByText('Looney Tunes')).toBeInTheDocument();
|
||||
|
||||
fireEvent.click(screen.getByRole('button', { name: 'Seasons' }));
|
||||
|
||||
expect(await screen.findByText('Season 1')).toBeInTheDocument();
|
||||
expect(screen.getByText('Showing the first 100 of 140 seasons.')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('saves the current settings as a new template and selects it', async () => {
|
||||
const created = channelTemplate({
|
||||
id: 55,
|
||||
|
||||
@@ -433,15 +433,22 @@ function SeasonsDialog({
|
||||
onClose: () => void;
|
||||
}) {
|
||||
const [seasons, setSeasons] = useState<LibraryBrowseItem[]>([]);
|
||||
const [totalCount, setTotalCount] = useState(0);
|
||||
const [status, setStatus] = useState<'loading' | 'error' | 'success'>('loading');
|
||||
const [error, setError] = useState<string | null>(null);
|
||||
|
||||
useEffect(() => {
|
||||
let active = true;
|
||||
// Class B (media-library-scoped, spa-conventions §3b): a single bounded page at the
|
||||
// server cap is intentional here — no real show has anywhere near 100 seasons, so this
|
||||
// isn't paged to completeness — but #650 found the response's real `totalCount` went
|
||||
// unread, so a show that somehow did exceed the cap would truncate silently. Read it and
|
||||
// surface it below instead, same as the other Class B pickers.
|
||||
getLibraryBrowseItems({ mediaType: 'TelevisionSeason', parentId: show.id, pageSize: 100 })
|
||||
.then((result) => {
|
||||
if (active) {
|
||||
setSeasons(result.page ?? []);
|
||||
setTotalCount(result.totalCount ?? (result.page ?? []).length);
|
||||
setStatus('success');
|
||||
}
|
||||
})
|
||||
@@ -479,24 +486,31 @@ function SeasonsDialog({
|
||||
) : seasons.length === 0 ? (
|
||||
<div className="ctv-builder-empty">This show has no seasons.</div>
|
||||
) : (
|
||||
<div className="ctv-builder-seasons-list">
|
||||
{seasons.map((season) => {
|
||||
const added = addedKeys.has(lineupKey(season));
|
||||
return (
|
||||
<div key={lineupKey(season)} className="ctv-builder-seasons-row">
|
||||
<Poster item={season} width={40} height={54} mini />
|
||||
<div className="ctv-builder-seasons-row-title">{season.title}</div>
|
||||
<IconButton size="sm" title={added ? 'Added' : 'Add to lineup'} onClick={() => onAdd(season)}>
|
||||
{added ? (
|
||||
<Check size={15} color="var(--action-primary)" aria-hidden="true" />
|
||||
) : (
|
||||
<Plus size={15} aria-hidden="true" />
|
||||
)}
|
||||
</IconButton>
|
||||
</div>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
<>
|
||||
{seasons.length < totalCount && (
|
||||
<span className="ctv-field-help">
|
||||
Showing the first {seasons.length} of {totalCount} seasons.
|
||||
</span>
|
||||
)}
|
||||
<div className="ctv-builder-seasons-list">
|
||||
{seasons.map((season) => {
|
||||
const added = addedKeys.has(lineupKey(season));
|
||||
return (
|
||||
<div key={lineupKey(season)} className="ctv-builder-seasons-row">
|
||||
<Poster item={season} width={40} height={54} mini />
|
||||
<div className="ctv-builder-seasons-row-title">{season.title}</div>
|
||||
<IconButton size="sm" title={added ? 'Added' : 'Add to lineup'} onClick={() => onAdd(season)}>
|
||||
{added ? (
|
||||
<Check size={15} color="var(--action-primary)" aria-hidden="true" />
|
||||
) : (
|
||||
<Plus size={15} aria-hidden="true" />
|
||||
)}
|
||||
</IconButton>
|
||||
</div>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
</>
|
||||
)}
|
||||
</Dialog>
|
||||
);
|
||||
@@ -510,14 +524,25 @@ interface BrowseState {
|
||||
error: string | null;
|
||||
}
|
||||
|
||||
async function loadCollections(query: string): Promise<{ page: LibraryBrowseItem[]; totalCount: number }> {
|
||||
// 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, pageSize: 100 }))
|
||||
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' }));
|
||||
return { page: merged, totalCount: merged.length };
|
||||
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)
|
||||
@@ -558,7 +583,9 @@ function useLibraryBrowse(source: 'library' | 'collections', query: string, libr
|
||||
const runFetch = useCallback(
|
||||
(pageNum: number, reqId: number, append: boolean) => {
|
||||
const promise =
|
||||
source === 'collections' ? loadCollections(query) : loadLibraryItems(query, libraryId, pageNum, 100);
|
||||
source === 'collections'
|
||||
? loadCollections(query, pageNum, 100)
|
||||
: loadLibraryItems(query, libraryId, pageNum, 100);
|
||||
|
||||
promise
|
||||
.then((result) => {
|
||||
@@ -604,7 +631,10 @@ function useLibraryBrowse(source: 'library' | 'collections', query: string, libr
|
||||
runFetch(nextPage, reqRef.current, true);
|
||||
}, [runFetch]);
|
||||
|
||||
const canLoadMore = source === 'library' && state.status === 'success' && state.items.length < state.totalCount;
|
||||
// #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 };
|
||||
}
|
||||
|
||||
@@ -0,0 +1,110 @@
|
||||
import { cleanup, render, screen } from '@testing-library/react';
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest';
|
||||
import { TraktListsScreen } from './TraktListsScreen';
|
||||
|
||||
function jsonResponse(body: unknown, status = 200): Response {
|
||||
return new Response(JSON.stringify(body), {
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
status
|
||||
});
|
||||
}
|
||||
|
||||
describe('TraktListsScreen', () => {
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it(
|
||||
'pages to completeness: a 101-item list (one over the 100 server cap) issues a SECOND request and ' +
|
||||
'renders every row, with the footer count agreeing with the row count (#650)',
|
||||
async () => {
|
||||
// Trakt lists are bounded-by-construction (admin-added), so this pins the exact boundary named
|
||||
// in #650: a request for EXACTLY the cap (100) truncates just as much as an over-cap request,
|
||||
// so 101 rows (one over the cap) must still page to completeness rather than showing "101
|
||||
// lists" over a 100-row table (the self-contradictory symptom #650 described).
|
||||
const total = 101;
|
||||
const cap = 100;
|
||||
const all = Array.from({ length: total }, (_, i) => ({
|
||||
autoRefresh: false,
|
||||
generatePlaylist: false,
|
||||
id: i + 1,
|
||||
itemCount: 10,
|
||||
matchCount: 5,
|
||||
name: `List ${i + 1}`,
|
||||
slug: `list-${i + 1}`,
|
||||
traktId: 1000 + i
|
||||
}));
|
||||
|
||||
const fetchMock = vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL) => {
|
||||
const url = new URL(input.toString(), 'http://localhost');
|
||||
if (url.pathname === '/api/v1/trakt/lists') {
|
||||
const pageNum = Number(url.searchParams.get('pageNum') ?? '0');
|
||||
const pageSize = Number(url.searchParams.get('pageSize') ?? String(cap));
|
||||
const start = pageNum * pageSize;
|
||||
return Promise.resolve(jsonResponse({ page: all.slice(start, start + pageSize), totalCount: total }));
|
||||
}
|
||||
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 101')).toBeInTheDocument();
|
||||
expect(screen.getByText('List 1')).toBeInTheDocument();
|
||||
|
||||
// Row count and footer count must AGREE at the boundary — the #650 defect was rendering
|
||||
// "101 lists" above a table truncated to 100 rows.
|
||||
expect(screen.getAllByRole('row')).toHaveLength(total + 1); // +1 header row
|
||||
expect(screen.getByText(`${total} lists`)).toBeInTheDocument();
|
||||
expect(screen.queryByText('List may be incomplete — retry to reload')).not.toBeInTheDocument();
|
||||
|
||||
const listCalls = fetchMock.mock.calls.filter(
|
||||
([u]) => new URL(u.toString(), 'http://localhost').pathname === '/api/v1/trakt/lists'
|
||||
);
|
||||
expect(listCalls).toHaveLength(2);
|
||||
}
|
||||
);
|
||||
|
||||
it('surfaces an incomplete-load warning when a page comes back short of totalCount (#650)', async () => {
|
||||
// Defensive break in loadAllPages: an empty (or short) page before totalCount converges must
|
||||
// render the same "may be incomplete" state used by the other Class A screens, not silently
|
||||
// show a partial list as if it were the whole one.
|
||||
vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL) => {
|
||||
const url = new URL(input.toString(), 'http://localhost');
|
||||
if (url.pathname === '/api/v1/trakt/lists') {
|
||||
const pageNum = Number(url.searchParams.get('pageNum') ?? '0');
|
||||
// Page 0 returns one row; every subsequent page comes back empty even though
|
||||
// totalCount (50) never converges — the defensive break loadAllPages relies on.
|
||||
const page =
|
||||
pageNum === 0
|
||||
? [
|
||||
{
|
||||
autoRefresh: false,
|
||||
generatePlaylist: false,
|
||||
id: 1,
|
||||
itemCount: 1,
|
||||
matchCount: 1,
|
||||
name: 'Only List',
|
||||
slug: 'only-list',
|
||||
traktId: 1
|
||||
}
|
||||
]
|
||||
: [];
|
||||
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('Only List')).toBeInTheDocument();
|
||||
expect(screen.getByText('List may be incomplete — retry to reload')).toBeInTheDocument();
|
||||
expect(screen.getByText('1 list')).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
@@ -9,6 +9,7 @@ import {
|
||||
getTraktListById,
|
||||
getTraktLists,
|
||||
getTraktStatus,
|
||||
loadAllPages,
|
||||
matchTraktList,
|
||||
messageFromTraktError,
|
||||
updateTraktList,
|
||||
@@ -265,7 +266,7 @@ export function TraktListsScreen() {
|
||||
const editingId = traktListIdFromPathname(window.location.pathname);
|
||||
|
||||
const [lists, setLists] = useState<TraktList[]>([]);
|
||||
const [totalCount, setTotalCount] = useState(0);
|
||||
const [incomplete, setIncomplete] = useState(false);
|
||||
const [loading, setLoading] = useState(true);
|
||||
const [error, setError] = useState<string | null>(null);
|
||||
const [addOpen, setAddOpen] = useState(false);
|
||||
@@ -276,6 +277,12 @@ export function TraktListsScreen() {
|
||||
const [deleteError, setDeleteError] = useState<string | null>(null);
|
||||
const [rowError, setRowError] = useState<string | null>(null);
|
||||
const activeRef = useRef(true);
|
||||
// Monotonic request id (spa-conventions §3): `refresh()`/the busy->idle transition can
|
||||
// re-trigger `load()`, and now that a load is a multi-request `loadAllPages` loop, an older
|
||||
// loop can resolve after a newer one — guarding on `activeRef` (still mounted) alone isn't
|
||||
// enough (#644 follow-up F3).
|
||||
const seqRef = useRef(0);
|
||||
const abortRef = useRef<AbortController | null>(null);
|
||||
|
||||
// TopBar "Add Trakt List" primary action. On the list, open the add dialog. On the
|
||||
// detail sub-route (/app/trakt-lists/{id}) the add dialog isn't mounted and this component
|
||||
@@ -294,21 +301,36 @@ export function TraktListsScreen() {
|
||||
// shows the spinner; later quiet reloads (e.g. after a busy -> idle transition) never
|
||||
// touch `loading`, so the table stays visible instead of flashing back to a spinner.
|
||||
const load = useCallback(() => {
|
||||
getTraktLists({ pageSize: 100 })
|
||||
.then((paged) => {
|
||||
if (activeRef.current) {
|
||||
setLists(paged.page ?? []);
|
||||
setTotalCount(paged.totalCount ?? 0);
|
||||
abortRef.current?.abort();
|
||||
const controller = new AbortController();
|
||||
abortRef.current = controller;
|
||||
const seq = (seqRef.current += 1);
|
||||
|
||||
// Trakt lists are admin-created (bounded by construction), so this pages to completeness
|
||||
// rather than requesting a single at-cap page (#650 — the same defect class as #644/#634:
|
||||
// treating one page as the whole list, just triggered by an exact-cap request instead of
|
||||
// an over-cap one).
|
||||
loadAllPages(getTraktLists, undefined, undefined, controller.signal)
|
||||
.then(({ complete, items }) => {
|
||||
if (activeRef.current && seqRef.current === seq) {
|
||||
if (!complete && !controller.signal.aborted) {
|
||||
// #644 follow-up F4: a partial result is otherwise indistinguishable from a complete
|
||||
// one. Surfaced via `incomplete` below; also logged so it shows up outside the UI.
|
||||
console.warn('TraktListsScreen: trakt-lists load did not complete; some lists may be missing');
|
||||
}
|
||||
|
||||
setLists(items);
|
||||
setIncomplete(!complete);
|
||||
setError(null);
|
||||
}
|
||||
})
|
||||
.catch((loadError: unknown) => {
|
||||
if (activeRef.current) {
|
||||
if (activeRef.current && seqRef.current === seq) {
|
||||
setError(messageFromTraktError(loadError));
|
||||
}
|
||||
})
|
||||
.finally(() => {
|
||||
if (activeRef.current) {
|
||||
if (activeRef.current && seqRef.current === seq) {
|
||||
setLoading(false);
|
||||
}
|
||||
});
|
||||
@@ -325,6 +347,7 @@ export function TraktListsScreen() {
|
||||
|
||||
return () => {
|
||||
activeRef.current = false;
|
||||
abortRef.current?.abort();
|
||||
};
|
||||
}, [load]);
|
||||
|
||||
@@ -394,6 +417,7 @@ export function TraktListsScreen() {
|
||||
<Spinner size={12} /> Busy
|
||||
</Badge>
|
||||
)}
|
||||
{incomplete && <Badge tone="warn">List may be incomplete — retry to reload</Badge>}
|
||||
<span className="ctv-channels-spacer" />
|
||||
<Button
|
||||
disabled={busy}
|
||||
@@ -497,7 +521,7 @@ export function TraktListsScreen() {
|
||||
</div>
|
||||
<div className="ctv-channels-footer">
|
||||
<span>
|
||||
{totalCount} list{totalCount === 1 ? '' : 's'}
|
||||
{lists.length} list{lists.length === 1 ? '' : 's'}
|
||||
</span>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
Reference in New Issue
Block a user