From fe342a6a0b63834cf9c21cdf875c32440d66752a Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 20:32:06 +0200 Subject: [PATCH] fix(644): page SPA list loaders to completeness instead of inflating pageSize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven call sites (rerun-collections, multi-collections, library/browse) requested pageSize far above each endpoint's server-side MaxPageSize=100 clamp and took the single response page as the whole list, so rows past 100 silently vanished with no error or truncation indicator. Extract the loadAllRerunCollections pattern from SchedulesScreen (#634) into a shared, generic web/src/api/paging.ts::loadAllPages helper that pages against totalCount with an empty-page defensive break, and refactor SchedulesScreen plus the seven over-cap call sites in RerunCollectionsScreen, MultiCollectionsScreen, PlaylistsScreen, and FillerPresetsScreen to use it. Server caps are unchanged (api.search-allitems-paging precedent: client pages, server stays bounded). Document the convention in docs/spa-conventions.md §3b. --- docs/spa-conventions.md | 28 ++++ web/src/api/index.ts | 1 + web/src/api/paging.test.ts | 179 +++++++++++++++++++++ web/src/api/paging.ts | 58 +++++++ web/src/screens/FillerPresetsScreen.tsx | 7 +- web/src/screens/MultiCollectionsScreen.tsx | 7 +- web/src/screens/PlaylistsScreen.tsx | 9 +- web/src/screens/RerunCollectionsScreen.tsx | 15 +- web/src/screens/SchedulesScreen.tsx | 34 +--- 9 files changed, 295 insertions(+), 43 deletions(-) create mode 100644 web/src/api/paging.test.ts create mode 100644 web/src/api/paging.ts diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index 4d55f7906..441397a1d 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -121,6 +121,34 @@ Convention — when a screen keeps stale results visible during a refetch: current (compare against a ref that always holds the committed value — `SearchScreen` reuses `lastQueryRef`) and **discard** otherwise. Checking only `activeRef` (mounted) is insufficient. +## 3b. Paged list endpoints clamp server-side — page to completeness, don't inflate `pageSize` + +Every paged `/api/v1` list endpoint (rerun-collections, multi-collections, library/browse, search, +trakt-lists, …) clamps `pageSize` to its own controller's `MaxPageSize` (100, as of #644) regardless +of what the client requests. A screen that asks for `pageSize: 1000` to "get everything in one call" +gets only the first `MaxPageSize` rows back, silently — no error, no truncation indicator, no paging +UI to notice the gap. This was issue #644 (following on from #634, which fixed the first instance — +`SchedulesScreen`'s rerun-collections picker load). + +**If a screen genuinely needs the complete list** (not a paginated view — e.g. a picker/typeahead +data source), use the shared `loadAllPages` helper (`web/src/api/paging.ts`, re-exported via +`web/src/api/index.ts`) instead of an inflated `pageSize`: + +```ts +loadAllPages(getMultiCollections) // pages against totalCount, cap defaults to 100 +loadAllPages(getLibraryBrowseItems, { mediaType: 'Movie' }) // extra fixed params thread into every page +``` + +It pages `pageNum` from 0 (per §"paging-zero-based" in `api-conventions.md`) against the response's +`totalCount`, breaking early on an empty page as a defensive guard against a `totalCount` that never +converges. **Do not raise the server-side cap to work around this** — the `api.search-allitems-paging` +precedent is that the client pages and the server stays bounded; that's a backend decision, out of +scope for a screen fix. + +**If a screen shows a bounded preview or has real paging UI** (a "load more" button, a page-size +selector, a fixed-size typeahead result list), a `pageSize` at or below the cap is correct as-is — +`loadAllPages` is only for "I need literally everything" call sites. + ## 4. API client modules One file per domain in `web/src/api/`, e.g. `logs.ts`, `blocks.ts`, `playouts.ts`. Pattern (see diff --git a/web/src/api/index.ts b/web/src/api/index.ts index 0fc10728f..181107266 100644 --- a/web/src/api/index.ts +++ b/web/src/api/index.ts @@ -23,6 +23,7 @@ export * from './mediaDetail'; export * from './mediaItems'; export * from './mediaSources'; export * from './multiCollections'; +export * from './paging'; export * from './pickers'; export * from './playlists'; export * from './playoutHistory'; diff --git a/web/src/api/paging.test.ts b/web/src/api/paging.test.ts new file mode 100644 index 000000000..0b0919881 --- /dev/null +++ b/web/src/api/paging.test.ts @@ -0,0 +1,179 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { loadAllPages, type PagedResult } 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> => { + 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([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> => { + 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, {}, 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 } + ]); + + // Pin the exact set: ids 1..250 in order, nothing dropped at either page boundary. + expect(result.map((item) => item.id)).toEqual(Array.from({ length: total }, (_, i) => i + 1)); + expect(result[99]).toEqual(fakeItem(100)); + expect(result[100]).toEqual(fakeItem(101)); + expect(result[249]).toEqual(fakeItem(250)); + }); + + it('threads extra base params (e.g. mediaType) into every page request', async () => { + const fetchPage = vi.fn(async (): Promise> => ({ 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)', async () => { + const fetchPage = vi.fn(async (params: { pageNum?: number }): Promise> => { + if ((params.pageNum ?? 0) === 0) { + return { page: [fakeItem(1)], totalCount: 5 }; + } + return { page: [], totalCount: 5 }; + }); + + const result = await loadAllPages(fetchPage); + + expect(result).toEqual([fakeItem(1)]); + expect(fetchPage).toHaveBeenCalledTimes(2); + }); +}); + +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); + + 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.map((entry) => entry.id)).toEqual(Array.from({ length: total }, (_, i) => i + 1)); + expect(result[99].name).toBe('MC 100'); + expect(result[100].name).toBe('MC 101'); + expect(result[199].name).toBe('MC 200'); + expect(result[200].name).toBe('MC 201'); + expect(result[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.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); + + 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.map((item) => item.title)).toEqual(Array.from({ length: total }, (_, i) => `Movie ${i + 1}`)); + expect(result[99].title).toBe('Movie 100'); + expect(result[100].title).toBe('Movie 101'); + expect(result[149].title).toBe('Movie 150'); + }); +}); diff --git a/web/src/api/paging.ts b/web/src/api/paging.ts new file mode 100644 index 000000000..ec8d08cb1 --- /dev/null +++ b/web/src/api/paging.ts @@ -0,0 +1,58 @@ +/** + * Shared client-side paging helper (issue #644, extending the `loadAllRerunCollections` pattern + * introduced for SchedulesScreen in #634). + * + * Paged list endpoints under `/api/v1` clamp `pageSize` server-side (each controller's own + * `MaxPageSize`, currently 100 for rerun-collections, multi-collections, and library/browse — see + * `RerunCollectionController`/`MultiCollectionController`/`LibraryBrowseController`). Requesting a + * `pageSize` above the cap buys nothing: the server silently clamps it, so a single oversized + * request only ever returns the first page's worth of rows and the rest vanish with no error and + * no truncation indicator. + * + * A screen that needs the FULL list (not a paginated view) must page to completeness against + * `totalCount` instead of inflating `pageSize` — the client pages, the server stays bounded + * (the `api.search-allitems-paging` precedent). Use this helper rather than copying the loop. + */ + +export interface PagedResult { + page?: T[] | null; + totalCount?: number | null; +} + +/** Matches every generated paged-list params shape (`GetMultiCollectionsParams`, etc). */ +export interface PagingParams { + pageNum?: number; + pageSize?: number; +} + +/** + * Repeatedly calls `fetchPage` with increasing `pageNum` (0-based, per `api.paging-zero-based`) + * until the accumulated results reach `totalCount`, or a page comes back empty (defensive break + * against a `totalCount` that never converges). `pageSize` defaults to 100, the cap shared by + * every paged `/api/v1` list endpoint today; pass a smaller value only if a specific endpoint's + * cap is lower. + */ +export async function loadAllPages( + fetchPage: (params: P) => Promise>, + baseParams: Omit = {} as Omit, + pageSize = 100 +): Promise { + const first = await fetchPage({ ...baseParams, pageNum: 0, pageSize } as P); + let all = first.page ?? []; + const totalCount = first.totalCount ?? all.length; + let pageNum = 1; + + while (all.length < totalCount) { + const next = await fetchPage({ ...baseParams, pageNum, pageSize } as P); + const nextPage = next.page ?? []; + + if (nextPage.length === 0) { + break; + } + + all = [...all, ...nextPage]; + pageNum += 1; + } + + return all; +} diff --git a/web/src/screens/FillerPresetsScreen.tsx b/web/src/screens/FillerPresetsScreen.tsx index 85858d8ac..fa23156f2 100644 --- a/web/src/screens/FillerPresetsScreen.tsx +++ b/web/src/screens/FillerPresetsScreen.tsx @@ -9,6 +9,7 @@ import { getFillerPreset, getFillerPresets, getLibraryBrowseItems, + loadAllPages, messageFromFillerPresetError, updateFillerPreset, type CreateFillerPresetRequest, @@ -486,10 +487,10 @@ function FillerEditor({ mode }: { mode: { id: number; kind: 'edit' } | { kind: ' let active = true; - getLibraryBrowseItems({ mediaType: config.browse, pageSize: 500 }) - .then((result) => { + loadAllPages(getLibraryBrowseItems, { mediaType: config.browse }) + .then((list) => { if (active) { - setPickerItems(result.page ?? []); + setPickerItems(list); setPickerError(null); } }) diff --git a/web/src/screens/MultiCollectionsScreen.tsx b/web/src/screens/MultiCollectionsScreen.tsx index d0ff0360d..43a999a09 100644 --- a/web/src/screens/MultiCollectionsScreen.tsx +++ b/web/src/screens/MultiCollectionsScreen.tsx @@ -10,6 +10,7 @@ import { getMultiCollectionWithMeta, getMultiCollections, getSmartCollections, + loadAllPages, messageFromMultiCollectionError, updateMultiCollection, type MediaCollection, @@ -37,10 +38,10 @@ function useMultiCollectionsData() { }, []); const load = useCallback(() => { - getMultiCollections({ pageSize: 1000 }) - .then((result) => { + loadAllPages(getMultiCollections) + .then((list) => { if (activeRef.current) { - setState({ data: result.page ?? [], error: null, status: 'success' }); + setState({ data: list, error: null, status: 'success' }); } }) .catch((error: unknown) => { diff --git a/web/src/screens/PlaylistsScreen.tsx b/web/src/screens/PlaylistsScreen.tsx index dd0f7fdc6..c0ffe20c0 100644 --- a/web/src/screens/PlaylistsScreen.tsx +++ b/web/src/screens/PlaylistsScreen.tsx @@ -31,6 +31,7 @@ import { getPlaylistItemsWithMeta, getPlaylists, getSmartCollections, + loadAllPages, messageFromPlaylistError, previewPlaylist, updatePlaylist, @@ -149,14 +150,14 @@ function loadPickerOptions(type: CollectionType): Promise { case 'collection': return getCollections().then((list) => list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }))); case 'multi': - return getMultiCollections({ pageSize: 1000 }).then((result) => - (result.page ?? []).map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` })) + return loadAllPages(getMultiCollections).then((list) => + list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` })) ); case 'smart': return getSmartCollections().then((list) => list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }))); default: - return getLibraryBrowseItems({ mediaType: config.browse, pageSize: 500 }).then((result) => - (result.page ?? []).map((item) => ({ id: item.mediaItemId ?? item.id, name: item.title ?? `#${item.id}` })) + return loadAllPages(getLibraryBrowseItems, { mediaType: config.browse }).then((list) => + list.map((item) => ({ id: item.mediaItemId ?? item.id, name: item.title ?? `#${item.id}` })) ); } } diff --git a/web/src/screens/RerunCollectionsScreen.tsx b/web/src/screens/RerunCollectionsScreen.tsx index 4fbcf193d..bc9caa73c 100644 --- a/web/src/screens/RerunCollectionsScreen.tsx +++ b/web/src/screens/RerunCollectionsScreen.tsx @@ -13,6 +13,7 @@ import { getRerunCollectionWithMeta, getRerunCollections, getSmartCollections, + loadAllPages, messageFromRerunCollectionError, updateRerunCollection, type CreateRerunCollectionRequest, @@ -102,14 +103,14 @@ function loadPickerOptions(type: RerunCollectionType): Promise { case 'collection': return getCollections().then((list) => list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }))); case 'multi': - return getMultiCollections({ pageSize: 1000 }).then((result) => - (result.page ?? []).map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` })) + return loadAllPages(getMultiCollections).then((list) => + list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` })) ); case 'smart': return getSmartCollections().then((list) => list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }))); default: - return getLibraryBrowseItems({ mediaType: config.browse, pageSize: 500 }).then((result) => - (result.page ?? []).map((item) => ({ id: item.mediaItemId ?? item.id, name: item.title ?? `#${item.id}` })) + return loadAllPages(getLibraryBrowseItems, { mediaType: config.browse }).then((list) => + list.map((item) => ({ id: item.mediaItemId ?? item.id, name: item.title ?? `#${item.id}` })) ); } } @@ -133,10 +134,10 @@ function useRerunCollectionsData() { }, []); const load = useCallback(() => { - getRerunCollections({ pageSize: 1000 }) - .then((result) => { + loadAllPages(getRerunCollections) + .then((list) => { if (activeRef.current) { - setState({ data: result.page ?? [], error: null, status: 'success' }); + setState({ data: list, error: null, status: 'success' }); } }) .catch((error: unknown) => { diff --git a/web/src/screens/SchedulesScreen.tsx b/web/src/screens/SchedulesScreen.tsx index ab30f1bb8..00a1f44ad 100644 --- a/web/src/screens/SchedulesScreen.tsx +++ b/web/src/screens/SchedulesScreen.tsx @@ -20,6 +20,7 @@ import { getSchedules, getGraphicsElements, getWatermarks, + loadAllPages, messageFromScheduleError, type ProgramSchedule, replaceScheduleItems, @@ -45,33 +46,14 @@ const DIRTY_PROMPT = 'You have unsaved schedule changes. Discard them?'; // Server clamps pageSize to MaxPageSize=100 (RerunCollectionController) regardless of what's // requested, so a single `pageSize: 1000` request only ever returns the first 100 rows. Page to // completeness against totalCount instead of raising the server cap (api.search-allitems-paging -// precedent — the client pages, the server stays bounded). Mirrors CollectionsScreen.enterReorder. +// precedent — the client pages, the server stays bounded) via the shared `loadAllPages` helper +// (`web/src/api/paging.ts`, #644). Mirrors CollectionsScreen.enterReorder. // -// `pageNum` is 0-BASED here and everywhere on /api/v1 (api.paging-zero-based): the first page is 0, -// so the loop below starts its follow-up requests at 1. Before ersatztv#616 this call site passed -// `pageNum: 1`, which skipped the first page entirely and showed nothing at all for the ordinary -// case of <=100 rerun collections — the same call site, one defect earlier. -const RERUN_PAGE_SIZE = 100; - -async function loadAllRerunCollections(): Promise { - const first = await getRerunCollections({ pageNum: 0, pageSize: RERUN_PAGE_SIZE }); - let all = first.page ?? []; - const totalCount = first.totalCount ?? all.length; - let pageNum = 1; - - while (all.length < totalCount) { - const next = await getRerunCollections({ pageNum, pageSize: RERUN_PAGE_SIZE }); - const nextPage = next.page ?? []; - - if (nextPage.length === 0) { - break; - } - - all = [...all, ...nextPage]; - pageNum += 1; - } - - return all; +// `pageNum` is 0-BASED here and everywhere on /api/v1 (api.paging-zero-based). Before ersatztv#616 +// this call site passed `pageNum: 1`, which skipped the first page entirely and showed nothing at +// all for the ordinary case of <=100 rerun collections — the same call site, one defect earlier. +function loadAllRerunCollections(): Promise { + return loadAllPages(getRerunCollections); } type BootState =