Cold adversarial review of fe342a6a found the blanket loadAllPages-everywhere fix
dangerous for the three getLibraryBrowseItems pickers (RerunCollectionsScreen,
PlaylistsScreen, FillerPresetsScreen): paging Episode/Song/Image/Movie/MusicVideo
to completeness can mean ~200 serial requests against a 20k-row library, each more
expensive than the last, to populate a <select> with thousands of <option> nodes.
- Class A (bounded-by-construction lists: rerun collections, multi-collections,
playlists) keep loadAllPages. Class B (media-library pickers) now fetch ONE
bounded page and surface truncation via a `Showing the first N of M` hint wired
to the real totalCount, instead of paging to completeness or truncating silently.
- loadAllPages: reports `{ items, complete }` instead of just `T[]` so a caller
can no longer mistake a defensive empty-page break for a full list (F4); accepts
an optional AbortSignal so a superseded loop stops issuing further page requests
(F2); baseParams is now required via a conditional rest-tuple whenever the
loader's params type has a field beyond pageNum/pageSize (F6); pushes into the
accumulator instead of re-spreading it every page (F7).
- MultiCollectionsScreen/RerunCollectionsScreen/SchedulesScreen: add a seqRef +
AbortController guard around the list/bootstrap loads so a stale loadAllPages
loop can't resolve after a newer one and resurrect deleted rows (F3); log and
surface an incomplete load rather than rendering it as whole.
- docs/spa-conventions.md §3b rewritten for the Class A / Class B split; new
decision record docs/decisions/records/spa/list-completeness-vs-bounded-pickers.md
(spa.list-completeness-vs-bounded-pickers), catalog regenerated.
- Tests: paging.test.ts covers null/undefined totalCount, null page, a
short-but-non-empty page, a page-2 rejection, the complete:false flag, and
cancellation (asserting fetch call COUNT stays put after abort), plus a
compile-time @ts-expect-error pinning the F6 typing fix. Screen-level tests
pin a real second HTTP request for a >100-item Class A list
(MultiCollectionsScreen) and exactly one /library/browse request plus the
truncation hint for a Class B picker (RerunCollectionsScreen).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
288 lines
12 KiB
TypeScript
288 lines
12 KiB
TypeScript
import { beforeEach, describe, expect, it, vi } from 'vitest';
|
|
import { loadAllPages, type PagedResult, type PagingParams } from './paging';
|
|
import { getMultiCollections } from './multiCollections';
|
|
import { getLibraryBrowseItems } from './libraryBrowse';
|
|
|
|
function jsonResponse(body: unknown, status = 200): Response {
|
|
return new Response(JSON.stringify(body), {
|
|
headers: { 'Content-Type': 'application/json' },
|
|
status
|
|
});
|
|
}
|
|
|
|
// Builds a "server" of `total` items with predictable ids/names, clamped to `cap` per page —
|
|
// mirrors the real RerunCollectionController/MultiCollectionController/LibraryBrowseController
|
|
// behavior (MaxPageSize=100 for all three, confirmed by reading the controllers for #644).
|
|
function fakeItem(id: number) {
|
|
return { id, name: `item-${id}` };
|
|
}
|
|
|
|
describe('loadAllPages', () => {
|
|
it('stops after a single page when totalCount fits within pageSize', async () => {
|
|
const fetchPage = vi.fn(async (params: { pageNum?: number; pageSize?: number }): Promise<PagedResult<{ id: number }>> => {
|
|
expect(params.pageNum).toBe(0);
|
|
expect(params.pageSize).toBe(100);
|
|
return { page: [fakeItem(1), fakeItem(2)], totalCount: 2 };
|
|
});
|
|
|
|
const result = await loadAllPages(fetchPage);
|
|
|
|
expect(result).toEqual({ complete: true, items: [fakeItem(1), fakeItem(2)] });
|
|
expect(fetchPage).toHaveBeenCalledTimes(1);
|
|
});
|
|
|
|
it('pages to completeness across more than one page boundary (250 items @ cap 100 -> 3 requests)', async () => {
|
|
const total = 250;
|
|
const cap = 100;
|
|
const all = Array.from({ length: total }, (_, i) => fakeItem(i + 1));
|
|
|
|
const fetchPage = vi.fn(async (params: { pageNum?: number; pageSize?: number }): Promise<PagedResult<{ id: number }>> => {
|
|
const pageNum = params.pageNum ?? 0;
|
|
const pageSize = params.pageSize ?? cap;
|
|
const start = pageNum * pageSize;
|
|
return { page: all.slice(start, start + pageSize), totalCount: total };
|
|
});
|
|
|
|
const result = await loadAllPages(fetchPage, undefined, cap);
|
|
|
|
expect(fetchPage).toHaveBeenCalledTimes(3);
|
|
expect(fetchPage.mock.calls.map((call) => call[0])).toEqual([
|
|
{ pageNum: 0, pageSize: 100 },
|
|
{ pageNum: 1, pageSize: 100 },
|
|
{ pageNum: 2, pageSize: 100 }
|
|
]);
|
|
|
|
expect(result.complete).toBe(true);
|
|
|
|
// Pin the exact set: ids 1..250 in order, nothing dropped at either page boundary.
|
|
expect(result.items.map((item) => item.id)).toEqual(Array.from({ length: total }, (_, i) => i + 1));
|
|
expect(result.items[99]).toEqual(fakeItem(100));
|
|
expect(result.items[100]).toEqual(fakeItem(101));
|
|
expect(result.items[249]).toEqual(fakeItem(250));
|
|
});
|
|
|
|
it('threads extra base params (e.g. mediaType) into every page request', async () => {
|
|
const fetchPage = vi.fn(async (): Promise<PagedResult<{ id: number }>> => ({ page: [fakeItem(1)], totalCount: 1 }));
|
|
|
|
await loadAllPages(fetchPage, { mediaType: 'Movie' });
|
|
|
|
expect(fetchPage).toHaveBeenCalledWith({ mediaType: 'Movie', pageNum: 0, pageSize: 100 });
|
|
});
|
|
|
|
it('breaks on an empty page even if totalCount claims more remain (defensive, never loops forever) and reports incomplete', async () => {
|
|
const fetchPage = vi.fn(async (params: { pageNum?: number }): Promise<PagedResult<{ id: number }>> => {
|
|
if ((params.pageNum ?? 0) === 0) {
|
|
return { page: [fakeItem(1)], totalCount: 5 };
|
|
}
|
|
return { page: [], totalCount: 5 };
|
|
});
|
|
|
|
const result = await loadAllPages(fetchPage);
|
|
|
|
expect(result).toEqual({ complete: false, items: [fakeItem(1)] });
|
|
expect(fetchPage).toHaveBeenCalledTimes(2);
|
|
});
|
|
|
|
it('a short-but-non-empty page keeps requesting, then reports incomplete once a later page comes back empty', async () => {
|
|
// First page under-fills (1 item though pageSize is 100) but totalCount claims 5 remain, so the
|
|
// loop must keep going by actual accumulated length, not by whether the page "looked full".
|
|
const calls: Array<number | undefined> = [];
|
|
const fetchPage = vi.fn(async (params: { pageNum?: number }): Promise<PagedResult<{ id: number }>> => {
|
|
calls.push(params.pageNum);
|
|
if ((params.pageNum ?? 0) === 0) {
|
|
return { page: [fakeItem(1)], totalCount: 5 };
|
|
}
|
|
if ((params.pageNum ?? 0) === 1) {
|
|
return { page: [fakeItem(2), fakeItem(3)], totalCount: 5 };
|
|
}
|
|
return { page: [], totalCount: 5 };
|
|
});
|
|
|
|
const result = await loadAllPages(fetchPage);
|
|
|
|
expect(result).toEqual({ complete: false, items: [fakeItem(1), fakeItem(2), fakeItem(3)] });
|
|
expect(fetchPage).toHaveBeenCalledTimes(3);
|
|
});
|
|
|
|
it('treats a null/undefined totalCount as "just this page" and reports complete', async () => {
|
|
const fetchPage = vi.fn(async (): Promise<PagedResult<{ id: number }>> => ({ page: [fakeItem(1), fakeItem(2)], totalCount: undefined }));
|
|
|
|
const result = await loadAllPages(fetchPage);
|
|
|
|
expect(result).toEqual({ complete: true, items: [fakeItem(1), fakeItem(2)] });
|
|
expect(fetchPage).toHaveBeenCalledTimes(1);
|
|
});
|
|
|
|
it('treats a null page as empty and reports complete with zero items', async () => {
|
|
const fetchPage = vi.fn(async (): Promise<PagedResult<{ id: number }>> => ({ page: null, totalCount: null }));
|
|
|
|
const result = await loadAllPages(fetchPage);
|
|
|
|
expect(result).toEqual({ complete: true, items: [] });
|
|
expect(fetchPage).toHaveBeenCalledTimes(1);
|
|
});
|
|
|
|
it('propagates a rejection on page 2 without retrying or issuing further requests', async () => {
|
|
const fetchPage = vi.fn(async (params: { pageNum?: number }): Promise<PagedResult<{ id: number }>> => {
|
|
if ((params.pageNum ?? 0) === 0) {
|
|
return { page: [fakeItem(1)], totalCount: 3 };
|
|
}
|
|
throw new Error('page 2 failed');
|
|
});
|
|
|
|
await expect(loadAllPages(fetchPage)).rejects.toThrow('page 2 failed');
|
|
expect(fetchPage).toHaveBeenCalledTimes(2);
|
|
});
|
|
|
|
it('cancellation: an already-aborted signal issues no requests at all', async () => {
|
|
const fetchPage = vi.fn(async (): Promise<PagedResult<{ id: number }>> => ({ page: [fakeItem(1)], totalCount: 1 }));
|
|
const controller = new AbortController();
|
|
controller.abort();
|
|
|
|
const result = await loadAllPages(fetchPage, undefined, 100, controller.signal);
|
|
|
|
expect(result).toEqual({ complete: false, items: [] });
|
|
expect(fetchPage).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it('cancellation: aborting after page 1 stops the loop from issuing page 2 or later', async () => {
|
|
const total = 250;
|
|
const cap = 100;
|
|
const all = Array.from({ length: total }, (_, i) => fakeItem(i + 1));
|
|
const controller = new AbortController();
|
|
|
|
const fetchPage = vi.fn(async (params: { pageNum?: number; pageSize?: number }): Promise<PagedResult<{ id: number }>> => {
|
|
const pageNum = params.pageNum ?? 0;
|
|
const pageSize = params.pageSize ?? cap;
|
|
if (pageNum === 0) {
|
|
// Abort as soon as the first page resolves, before the loop issues its next request.
|
|
controller.abort();
|
|
}
|
|
const start = pageNum * pageSize;
|
|
return { page: all.slice(start, start + pageSize), totalCount: total };
|
|
});
|
|
|
|
const result = await loadAllPages(fetchPage, undefined, cap, controller.signal);
|
|
|
|
// The whole point: assert the CALL COUNT stayed at 1 — no page 2/3 request was ever issued.
|
|
expect(fetchPage).toHaveBeenCalledTimes(1);
|
|
expect(result).toEqual({ complete: false, items: all.slice(0, cap) });
|
|
});
|
|
|
|
it('type-level: baseParams is required when the loader params type has a required field beyond pageNum/pageSize (F6)', () => {
|
|
interface RequiredExtraParams extends PagingParams {
|
|
requiredThing: string;
|
|
}
|
|
const fetchPage: (params: RequiredExtraParams) => Promise<PagedResult<{ id: number }>> = async () => ({
|
|
page: [],
|
|
totalCount: 0
|
|
});
|
|
|
|
// @ts-expect-error baseParams is required here — omitting it must NOT compile (the old
|
|
// `= {} as Omit<P, ...>` default silently defeated this check for every P, #644 follow-up F6).
|
|
void loadAllPages(fetchPage);
|
|
|
|
// The correctly-called form still type-checks.
|
|
void loadAllPages(fetchPage, { requiredThing: 'ok' });
|
|
});
|
|
});
|
|
|
|
describe('loadAllPages against real domain loaders', () => {
|
|
beforeEach(() => {
|
|
window.localStorage.clear();
|
|
vi.restoreAllMocks();
|
|
});
|
|
|
|
it('getMultiCollections: pins the exact merged set across a >cap (250-item) list and asserts fetch call params', async () => {
|
|
const total = 250;
|
|
const cap = 100;
|
|
const all = Array.from({ length: total }, (_, i) => ({ id: i + 1, items: [], name: `MC ${i + 1}` }));
|
|
|
|
const fetchMock = vi.spyOn(window, 'fetch').mockImplementation(async (input) => {
|
|
const url = new URL(String(input), 'http://localhost');
|
|
const pageNum = Number(url.searchParams.get('pageNum') ?? '0');
|
|
const pageSize = Number(url.searchParams.get('pageSize') ?? String(cap));
|
|
const start = pageNum * pageSize;
|
|
return jsonResponse({ page: all.slice(start, start + pageSize), totalCount: total });
|
|
});
|
|
|
|
const result = await loadAllPages(getMultiCollections);
|
|
|
|
expect(fetchMock).toHaveBeenCalledTimes(3);
|
|
expect(result.complete).toBe(true);
|
|
|
|
const requestedUrls = fetchMock.mock.calls.map((call) => new URL(String(call[0]), 'http://localhost'));
|
|
expect(requestedUrls.map((url) => url.pathname)).toEqual([
|
|
'/api/v1/multi-collections',
|
|
'/api/v1/multi-collections',
|
|
'/api/v1/multi-collections'
|
|
]);
|
|
expect(requestedUrls.map((url) => [url.searchParams.get('pageNum'), url.searchParams.get('pageSize')])).toEqual([
|
|
['0', '100'],
|
|
['1', '100'],
|
|
['2', '100']
|
|
]);
|
|
|
|
// Pin the exact ids/names returned, including the two page boundaries (index 99/100, 199/200).
|
|
expect(result.items.map((entry) => entry.id)).toEqual(Array.from({ length: total }, (_, i) => i + 1));
|
|
expect(result.items[99].name).toBe('MC 100');
|
|
expect(result.items[100].name).toBe('MC 101');
|
|
expect(result.items[199].name).toBe('MC 200');
|
|
expect(result.items[200].name).toBe('MC 201');
|
|
expect(result.items[249].name).toBe('MC 250');
|
|
});
|
|
|
|
it('getMultiCollections: a list at exactly the cap (100) still issues only one request', async () => {
|
|
const all = Array.from({ length: 100 }, (_, i) => ({ id: i + 1, items: [], name: `MC ${i + 1}` }));
|
|
const fetchMock = vi.spyOn(window, 'fetch').mockResolvedValue(jsonResponse({ page: all, totalCount: 100 }));
|
|
|
|
const result = await loadAllPages(getMultiCollections);
|
|
|
|
expect(fetchMock).toHaveBeenCalledTimes(1);
|
|
expect(result.complete).toBe(true);
|
|
expect(result.items.map((entry) => entry.id)).toEqual(Array.from({ length: 100 }, (_, i) => i + 1));
|
|
});
|
|
|
|
it('getLibraryBrowseItems: pins the exact merged set across a >cap (150-item) list and threads mediaType into every page', async () => {
|
|
const total = 150;
|
|
const cap = 100;
|
|
const all = Array.from({ length: total }, (_, i) => ({
|
|
id: i + 1,
|
|
mediaItemId: i + 1,
|
|
mediaType: 'Movie' as const,
|
|
title: `Movie ${i + 1}`
|
|
}));
|
|
|
|
const fetchMock = vi.spyOn(window, 'fetch').mockImplementation(async (input) => {
|
|
const url = new URL(String(input), 'http://localhost');
|
|
const pageNum = Number(url.searchParams.get('pageNum') ?? '0');
|
|
const pageSize = Number(url.searchParams.get('pageSize') ?? String(cap));
|
|
const start = pageNum * pageSize;
|
|
return jsonResponse({ page: all.slice(start, start + pageSize), totalCount: total });
|
|
});
|
|
|
|
const result = await loadAllPages(getLibraryBrowseItems, { mediaType: 'Movie' });
|
|
|
|
expect(fetchMock).toHaveBeenCalledTimes(2);
|
|
expect(result.complete).toBe(true);
|
|
|
|
const requestedUrls = fetchMock.mock.calls.map((call) => new URL(String(call[0]), 'http://localhost'));
|
|
expect(
|
|
requestedUrls.map((url) => [
|
|
url.searchParams.get('mediaType'),
|
|
url.searchParams.get('pageNum'),
|
|
url.searchParams.get('pageSize')
|
|
])
|
|
).toEqual([
|
|
['Movie', '0', '100'],
|
|
['Movie', '1', '100']
|
|
]);
|
|
|
|
// Pin the exact titles across the page boundary at index 99/100.
|
|
expect(result.items.map((item) => item.title)).toEqual(Array.from({ length: total }, (_, i) => `Movie ${i + 1}`));
|
|
expect(result.items[99].title).toBe('Movie 100');
|
|
expect(result.items[100].title).toBe('Movie 101');
|
|
expect(result.items[149].title).toBe('Movie 150');
|
|
});
|
|
});
|