Merge pull request 'fix(644): stop seven SPA list loads truncating silently — one shared pager, bounded media pickers' (#656) from fix/644-spa-paging into main
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Has been skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Has been skipped
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Has been cancelled
Build ErsatzTV Image / Build & push image (amd64) (push) Has been cancelled
Build ErsatzTV Image / Build & test (.NET) (push) Has been cancelled
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Has been cancelled
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Has been skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Has been skipped
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Has been cancelled
Build ErsatzTV Image / Build & push image (amd64) (push) Has been cancelled
Build ErsatzTV Image / Build & test (.NET) (push) Has been cancelled
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Has been cancelled
This commit was merged in pull request #656.
This commit is contained in:
@@ -161,6 +161,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `spa.deco-templates-table` | The deco-templates editor also renders its day/deco assignment as a table, extending (not replacing) the templates-editor-table convention. | 2026-07-09 | [link](records/spa/deco-templates-table.md) |
|
||||
| `spa.download-sample-gate` | The SPA disables both Download Media Sample and Download Results while a troubleshooting session is starting/running (Blazor only gated Download Results). | 2026-07-09 | [link](records/spa/download-sample-gate.md) |
|
||||
| `spa.legacy-redirect-matcher` | `LegacyUiRedirects.TryGetRedirect` is a two-tier matcher — an exact `OrdinalIgnoreCase` `Map` (Tier 1) then an ordered segment-template pattern list (Tier 2, first-match-wins) — collision-free by construction, with a guard invariant that no rule may prefix-match `/api`, `/artwork`, `/docs`, `/openapi`, `/iptv`, `/app`, or `/media/sources`. | 2026-07-11 | [link](records/spa/legacy-redirect-matcher.md) |
|
||||
| `spa.list-completeness-vs-bounded-pickers` | The shared `loadAllPages` helper (`web/src/api/paging.ts`) pages a `/api/v1` list to completeness against `totalCount` and is used ONLY for lists that are bounded by construction (rerun collections, multi-collections, playlists — admin-created, hundreds of rows at most). A `getLibraryBrowseItems` picker over a media-library table (Episode/Song/Image/Movie/MusicVideo, tens of thousands of rows possible) must NOT page to completeness — it fetches ONE bounded page (the server cap) and surfaces the truncation (a `ctv-field-help` hint wired to the real `totalCount`) instead of silently dropping the rest. | 2026-07-26 | [link](records/spa/list-completeness-vs-bounded-pickers.md) |
|
||||
| `spa.logs-page-size-local` | The Logs page rows-per-page preference is stored in `window.localStorage` (`ctv-logs-page-size`), not a server `ConfigElement`. | 2026-07-11 | [link](records/spa/logs-page-size-local.md) |
|
||||
| `spa.playback-troubleshoot-poll` | The playback-troubleshooting screen reports FFmpeg completion by polling `GET /api/troubleshoot/playback/status` (~2s) rather than a server push channel. | 2026-07-09 | [link](records/spa/playback-troubleshoot-poll.md) |
|
||||
| `spa.playout-reset-button` | The SPA keeps a single Reset action (server picks the default build mode) and drops Blazor's separate "Schedule reset" button since its capability already exists via the playout's Edit-details flow. | 2026-07-09 | [link](records/spa/playout-reset-button.md) |
|
||||
|
||||
@@ -0,0 +1,53 @@
|
||||
---
|
||||
key: spa.list-completeness-vs-bounded-pickers
|
||||
title: '2026-07-26 — `loadAllPages` is for bounded-by-construction lists only; media-library pickers stay bounded and show truncation (#644 follow-up)'
|
||||
status: active
|
||||
since: '2026-07-26'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'The shared `loadAllPages` helper (`web/src/api/paging.ts`) pages a `/api/v1` list to completeness against `totalCount` and is used ONLY for lists that are bounded by construction (rerun collections, multi-collections, playlists — admin-created, hundreds of rows at most). A `getLibraryBrowseItems` picker over a media-library table (Episode/Song/Image/Movie/MusicVideo, tens of thousands of rows possible) must NOT page to completeness — it fetches ONE bounded page (the server cap) and surfaces the truncation (a `ctv-field-help` hint wired to the real `totalCount`) instead of silently dropping the rest.'
|
||||
signals: '`loadAllPages`, Class A vs Class B picker, LuceneSearchIndex.Search hitsLimit, picker truncation hint, ctv-field-help, PagedResult, `complete` flag · paths: `web/src/api/paging.ts`, `web/src/screens/RerunCollectionsScreen.tsx`, `web/src/screens/PlaylistsScreen.tsx`, `web/src/screens/FillerPresetsScreen.tsx`, `web/src/screens/MultiCollectionsScreen.tsx`, `docs/spa-conventions.md` §3b · issues: #644'
|
||||
mechanics: '`docs/spa-conventions.md` §3b'
|
||||
---
|
||||
|
||||
`fe342a6a` (#644) extracted the `loadAllPages` client-side paging helper and applied it at every
|
||||
call site that had been requesting an over-cap `pageSize` to "get everything in one call" — a
|
||||
pattern that silently truncated to the server's `MaxPageSize` (100) with no error and no
|
||||
truncation indicator. A cold adversarial review of that fix found it was correct for the
|
||||
admin-created lists (rerun collections, multi-collections, playlists — bounded by construction,
|
||||
hundreds of rows at most) but dangerous for three call sites: the `getLibraryBrowseItems` pickers
|
||||
in `RerunCollectionsScreen`, `PlaylistsScreen`, and `FillerPresetsScreen`, which populate a native
|
||||
`<select>` whose `mediaType` can be `Episode`, `Song`, `Image`, `Movie`, or `MusicVideo` — the
|
||||
largest tables in an install. Paging one of those to completeness means on the order of 200 serial
|
||||
requests against a 20,000-row library, each **more** expensive than the last (`LuceneSearchIndex
|
||||
.Search` computes `hitsLimit = skip + limit`, so later pages re-scan a growing prefix), ending in a
|
||||
`<select>` with 20,000 `<option>` nodes rendered into the DOM. That is worse than the defect #644
|
||||
set out to fix.
|
||||
|
||||
The fix keeps `loadAllPages` unchanged in behavior for the bounded lists (it now also reports a
|
||||
`complete: boolean` flag and accepts an `AbortSignal`, per the same follow-up review's F4/F2
|
||||
findings) and removes it entirely from the three media-library picker call sites. Those instead
|
||||
call `getLibraryBrowseItems` directly for a single page at the server cap (`pageSize: 100`) and
|
||||
read the response's `totalCount` to detect truncation. The defect named in #644's title is
|
||||
"*silently* truncate" — the silence is the bug, not the bound. So a truncated picker load renders a
|
||||
`ctv-field-help` hint next to the `<select>` (`Showing the first 100 of 5000 — use search to
|
||||
narrow.`) instead of either paging forever or truncating without saying so. A full
|
||||
typeahead/search-driven picker over the media library is a materially larger feature (a `query`
|
||||
param already exists on `getLibraryBrowseItems` for it) and is deliberately out of scope here — a
|
||||
follow-up issue, not this fix.
|
||||
|
||||
**2026-07-26 addendum (round-3 review F1):** `loadPickerOptions`'s `multi` branch (a Class A
|
||||
source — `MultiCollection`) reused the same `truncated: boolean` field as the Class B media-library
|
||||
pickers, but the two conditions are not the same thing: Class B's `truncated` means "there are more
|
||||
rows than fit in one page — narrow via search," while a Class A picker's flag meant "the
|
||||
`loadAllPages` loop didn't converge" (`complete: false`) — a defensive/incomplete load, not a cap.
|
||||
Rendering both through the shared "Showing the first N of M — use search to narrow" copy produced a
|
||||
self-contradictory "Showing the first 47 of 47" on an incomplete Class A load, pointing at a search
|
||||
box that picker doesn't have. `RerunCollectionsScreen.tsx`/`PlaylistsScreen.tsx` now return a
|
||||
`hint: 'incomplete' | 'none' | 'truncated'` discriminator instead of a boolean, and render distinct
|
||||
copy per value — `'truncated'` keeps the existing search-narrowing text, `'incomplete'` renders
|
||||
"List may be incomplete — retry to reload" (matching the wording already used for the Class A
|
||||
list-load warn `Badge`). A picker's `console.warn` on an incomplete load — and the analogous one in
|
||||
`SchedulesScreen.loadAllRerunCollections` — is also gated on `!signal?.aborted`, so a superseded or
|
||||
user-aborted load (Retry, or a type switch mid-load) no longer logs a false "did not complete"
|
||||
warning.
|
||||
@@ -121,6 +121,70 @@ 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 ONLY for bounded lists, never a media-library picker
|
||||
|
||||
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).
|
||||
|
||||
**Two classes of call site, treated differently** (decision record:
|
||||
`docs/decisions/records/spa/list-completeness-vs-bounded-pickers.md`, `spa.list-completeness-vs-bounded-pickers`
|
||||
— a #644 follow-up review found the original blanket "use `loadAllPages` for any picker" guidance
|
||||
below was itself the defect for one class of caller):
|
||||
|
||||
- **Bounded-by-construction lists** (rerun collections, multi-collections, playlists — admin-created,
|
||||
hundreds of rows at most): genuinely need the complete list, and completeness is cheap. Use the
|
||||
shared `loadAllPages` helper (`web/src/api/paging.ts`, re-exported via `web/src/api/index.ts`)
|
||||
instead of an inflated `pageSize`:
|
||||
|
||||
```ts
|
||||
const { items, complete } = await loadAllPages(getMultiCollections); // pages against totalCount, cap defaults to 100
|
||||
```
|
||||
|
||||
It pages `pageNum` from 0 (per §"paging-zero-based" in `api-conventions.md`) against the response's
|
||||
`totalCount`, stopping — and reporting `complete: false` — on an empty page (defensive guard
|
||||
against a `totalCount` that never converges) or on an aborted `signal`. **Always check `complete`**:
|
||||
a caller that needs the full list must not treat a resolved promise as proof the list is whole (a
|
||||
partial result is otherwise silently indistinguishable from a complete one — the same defect class
|
||||
as #644 itself, since `GetLibraryBrowseItemsHandler.HydrateMediaItems` can legitimately drop stale
|
||||
Lucene hits and produce a short/empty page in normal operation). Pass an `AbortSignal` (4th arg)
|
||||
from the caller's effect cleanup so a superseded load stops issuing further page requests instead
|
||||
of hammering the server for a result nobody will see. **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.
|
||||
|
||||
- **Media-library pickers** (`getLibraryBrowseItems` backing a `<select>` for Episode / Song / Image /
|
||||
Movie / MusicVideo / etc. — the largest tables in an install, tens of thousands of rows possible):
|
||||
must **NOT** use `loadAllPages`. Paging to completeness here means on the order of 200 serial
|
||||
requests for a 20k-row library — each more expensive than the last, since
|
||||
`LuceneSearchIndex.Search` computes `hitsLimit = skip + limit` — to populate a native `<select>`
|
||||
with thousands of `<option>` nodes. That is worse than the truncation bug it would "fix". Instead,
|
||||
fetch **one bounded page** directly (`pageSize` at the cap) and make the truncation **visible**
|
||||
rather than silent — e.g. a `ctv-field-help` hint next to the picker: `Showing the first 100 of
|
||||
5000 — use search to narrow.` (wire the response's real `totalCount`). See
|
||||
`RerunCollectionsScreen.tsx`/`PlaylistsScreen.tsx`/`FillerPresetsScreen.tsx`'s `loadPickerOptions`
|
||||
for the pattern. A full typeahead/search-driven picker is a separate, larger feature — out of scope
|
||||
for this fix.
|
||||
|
||||
**A Class B truncation and a Class A `complete: false` are different conditions — don't collapse
|
||||
them into one boolean** (#644 follow-up round-3 review F1): a `loadPickerOptions` result that can
|
||||
come from either a Class A (`loadAllPages`) or Class B (single bounded page) source should carry a
|
||||
`hint: 'incomplete' | 'none' | 'truncated'` discriminator, not a `truncated: boolean` reused for
|
||||
both. `'truncated'` (Class B, an expected cap) keeps the "Showing the first N of M — use search to
|
||||
narrow" copy; `'incomplete'` (Class A, `loadAllPages`'s `complete: false`) renders different copy
|
||||
("List may be incomplete — retry to reload") — rendering both through the search-narrowing text
|
||||
produces a self-contradictory "Showing the first 47 of 47" when a Class A load doesn't converge.
|
||||
Also gate any `console.warn` on a Class A `complete: false` with `!signal?.aborted` — a superseded
|
||||
or user-aborted load returns `complete: false` too, and that's expected, not a defect.
|
||||
|
||||
**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, and the list is small by construction"
|
||||
call sites.
|
||||
|
||||
## 4. API client modules
|
||||
|
||||
One file per domain in `web/src/api/`, e.g. `logs.ts`, `blocks.ts`, `playouts.ts`. Pattern (see
|
||||
|
||||
@@ -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';
|
||||
|
||||
@@ -0,0 +1,287 @@
|
||||
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');
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,107 @@
|
||||
/**
|
||||
* 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.
|
||||
*
|
||||
* **This is only for lists that are bounded by construction** (admin-created collections/rerun
|
||||
* entries/playlists — hundreds of rows at most). It must NOT be used as a picker/typeahead data
|
||||
* source over a media library table (Episode/Song/Image/Movie/MusicVideo can run into the tens of
|
||||
* thousands) — see `docs/decisions/records/spa/list-completeness-vs-bounded-pickers.md` (#644
|
||||
* follow-up review). Those call sites fetch a single bounded page directly and surface the
|
||||
* truncation instead.
|
||||
*/
|
||||
|
||||
export interface PagedResult<T> {
|
||||
page?: T[] | null;
|
||||
totalCount?: number | null;
|
||||
}
|
||||
|
||||
/** Matches every generated paged-list params shape (`GetMultiCollectionsParams`, etc). */
|
||||
export interface PagingParams {
|
||||
pageNum?: number;
|
||||
pageSize?: number;
|
||||
}
|
||||
|
||||
export interface LoadAllPagesResult<T> {
|
||||
/**
|
||||
* `false` when the loop stopped before reaching `totalCount` — either because a page came back
|
||||
* empty (defensive break; the totalCount never converged) or because `signal` was aborted
|
||||
* mid-loop. A caller that needs the FULL list must check this rather than trusting `items` to be
|
||||
* complete just because the promise resolved without throwing (#644 follow-up finding F4 — the
|
||||
* old `break` returned a partial list indistinguishable from a complete one, and this is
|
||||
* reachable in normal operation: `GetLibraryBrowseItemsHandler.HydrateMediaItems` drops Lucene
|
||||
* hits whose DB rows have since vanished, and the handler's own comment notes Lucene's
|
||||
* `TotalCount` can be stale).
|
||||
*/
|
||||
complete: boolean;
|
||||
items: T[];
|
||||
}
|
||||
|
||||
type BaseParams<P extends PagingParams> = Omit<P, 'pageNum' | 'pageSize'>;
|
||||
|
||||
// `baseParams` is required whenever `P` (minus `pageNum`/`pageSize`) has any required field of its
|
||||
// own; it's only optional when every remaining field is optional (an empty-object-assignable
|
||||
// type). This has to be encoded as a conditional REST TUPLE, not a `baseParams?: X | never`
|
||||
// parameter — marking the parameter itself optional with `?` makes an omitted argument type-check
|
||||
// regardless of `X`, which is exactly the hole this is meant to close (#644 follow-up finding F6;
|
||||
// the old single-signature `= {} as Omit<P, ...>` default silently defeated the check for every
|
||||
// `P`, required fields included).
|
||||
// "is an empty object type assignable here" is exactly the "does BaseParams<P> have any required
|
||||
// field" check, so the `{}` is intentional.
|
||||
// eslint-disable-next-line @typescript-eslint/no-empty-object-type
|
||||
type LoadAllPagesArgs<P extends PagingParams> = {} extends BaseParams<P>
|
||||
? [baseParams?: BaseParams<P>, pageSize?: number, signal?: AbortSignal]
|
||||
: [baseParams: BaseParams<P>, pageSize?: number, signal?: AbortSignal];
|
||||
|
||||
/**
|
||||
* 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), or `signal` is aborted. `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<T, P extends PagingParams>(
|
||||
fetchPage: (params: P) => Promise<PagedResult<T>>,
|
||||
...rest: LoadAllPagesArgs<P>
|
||||
): Promise<LoadAllPagesResult<T>> {
|
||||
const [baseParams = {} as BaseParams<P>, pageSize = 100, signal] = rest as [BaseParams<P>?, number?, AbortSignal?];
|
||||
|
||||
if (signal?.aborted) {
|
||||
return { complete: false, items: [] };
|
||||
}
|
||||
|
||||
const first = await fetchPage({ ...baseParams, pageNum: 0, pageSize } as P);
|
||||
const items: T[] = first.page ? [...first.page] : [];
|
||||
const totalCount = first.totalCount ?? items.length;
|
||||
let pageNum = 1;
|
||||
|
||||
while (items.length < totalCount) {
|
||||
if (signal?.aborted) {
|
||||
return { complete: false, items };
|
||||
}
|
||||
|
||||
const next = await fetchPage({ ...baseParams, pageNum, pageSize } as P);
|
||||
const nextPage = next.page ?? [];
|
||||
|
||||
if (nextPage.length === 0) {
|
||||
return { complete: false, items };
|
||||
}
|
||||
|
||||
// Push in place rather than `[...items, ...nextPage]` (#644 follow-up finding F7) — the spread
|
||||
// form reallocates and copies the whole accumulator on every page, making a long list O(n²).
|
||||
items.push(...nextPage);
|
||||
pageNum += 1;
|
||||
}
|
||||
|
||||
return { complete: true, items };
|
||||
}
|
||||
@@ -187,6 +187,10 @@ export interface SelectProps {
|
||||
size?: ControlSize;
|
||||
fullWidth?: boolean;
|
||||
style?: CSSProperties;
|
||||
// Associates an out-of-band hint (e.g. a truncation message rendered as a sibling) with this
|
||||
// control for screen readers, matching how `error`/`help` text is otherwise co-located with a
|
||||
// field. Pass the id of the element carrying the hint text.
|
||||
ariaDescribedBy?: string;
|
||||
}
|
||||
|
||||
export function Select({
|
||||
@@ -197,7 +201,8 @@ export function Select({
|
||||
disabled = false,
|
||||
size = 'md',
|
||||
fullWidth = true,
|
||||
style
|
||||
style,
|
||||
ariaDescribedBy
|
||||
}: SelectProps) {
|
||||
const normalizedOptions = options.map((option) =>
|
||||
typeof option === 'string' ? { value: option, label: option } : option
|
||||
@@ -215,7 +220,13 @@ export function Select({
|
||||
)}
|
||||
style={style}
|
||||
>
|
||||
<select className="ctv-select" value={value} onChange={onChange} disabled={disabled}>
|
||||
<select
|
||||
aria-describedby={ariaDescribedBy}
|
||||
className="ctv-select"
|
||||
value={value}
|
||||
onChange={onChange}
|
||||
disabled={disabled}
|
||||
>
|
||||
{normalizedOptions.map((option) => (
|
||||
<option key={option.value} value={option.value}>
|
||||
{option.label}
|
||||
|
||||
@@ -368,6 +368,13 @@ function SourcePicker({
|
||||
}
|
||||
|
||||
if (type === 'RerunFirstRun' || type === 'RerunRerun') {
|
||||
// Out-of-list injection (round-3 review F2): the picker source is a Class A `loadAllPages`
|
||||
// loop (SchedulesScreen.loadAllRerunCollections) that can legitimately return an incomplete
|
||||
// page set, so the item's stored rerunCollectionId may not be among `pickers.rerunCollections`
|
||||
// — mirrors RerunCollectionsScreen's and PlaylistsScreen's `selectedInList` prepend.
|
||||
const rerunSelectedInList =
|
||||
item.rerunCollectionId != null && pickers.rerunCollections.some((c) => c.id === item.rerunCollectionId);
|
||||
|
||||
return (
|
||||
<Select
|
||||
label="Rerun collection"
|
||||
@@ -377,7 +384,13 @@ function SourcePicker({
|
||||
const name = pickers.rerunCollections.find((c) => c.id === id)?.name ?? null;
|
||||
onChange({ ...item, rerunCollectionId: id, rerunCollectionName: name });
|
||||
}}
|
||||
options={[{ value: '', label: '(none)' }, ...pickers.rerunCollections.map((c) => ({ value: `${c.id}`, label: c.name ?? `Rerun ${c.id}` }))]}
|
||||
options={[
|
||||
{ value: '', label: '(none)' },
|
||||
...(item.rerunCollectionId != null && !rerunSelectedInList
|
||||
? [{ value: `${item.rerunCollectionId}`, label: item.rerunCollectionName || `Rerun ${item.rerunCollectionId}` }]
|
||||
: []),
|
||||
...pickers.rerunCollections.map((c) => ({ value: `${c.id}`, label: c.name ?? `Rerun ${c.id}` }))
|
||||
]}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,159 @@
|
||||
import { cleanup, render, screen, waitFor, within } from '@testing-library/react';
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import { FillerPresetsScreen } from './FillerPresetsScreen';
|
||||
|
||||
function jsonResponse(body: unknown, status = 200): Response {
|
||||
return new Response(JSON.stringify(body), {
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
status
|
||||
});
|
||||
}
|
||||
|
||||
const presetList = [{ fillerKind: 'PreRoll', id: 1, name: 'Bumper' }];
|
||||
|
||||
// Editing this preset: collectionType 'TelevisionShow', mediaItemId 9999 — deliberately NOT among
|
||||
// the library-browse page below, to exercise the F2 out-of-list injection. (FillerPresetsScreen's
|
||||
// COLLECTION_TYPES doesn't offer a raw 'Movie' entry — TelevisionShow/TelevisionSeason/Artist are
|
||||
// its media-item-backed types.)
|
||||
const editPreset = {
|
||||
allowWatermarks: false,
|
||||
collectionId: null,
|
||||
collectionType: 'TelevisionShow',
|
||||
count: 3,
|
||||
duration: null,
|
||||
expression: null,
|
||||
fillerKind: 'PreRoll',
|
||||
fillerMode: 'Count',
|
||||
id: 1,
|
||||
mediaItemId: 9999,
|
||||
multiCollectionId: null,
|
||||
name: 'Bumper',
|
||||
padToNearestMinute: null,
|
||||
playlistId: null,
|
||||
smartCollectionId: null,
|
||||
useChaptersAsMediaItems: false
|
||||
};
|
||||
|
||||
// One bounded page (cap 100) of browse items, none of which is id 9999. Every id-ish field is
|
||||
// populated (not just mediaItemId) so the fixture works regardless of which COLLECTION_TYPES
|
||||
// entry's `itemId` extractor is in play (Collection reads collectionId, Movie reads mediaItemId).
|
||||
function browsePage(count: number, totalCount: number) {
|
||||
return {
|
||||
page: Array.from({ length: count }, (_, i) => ({
|
||||
collectionId: i + 1,
|
||||
id: i + 1,
|
||||
mediaItemId: i + 1,
|
||||
mediaType: 'Movie' as const,
|
||||
multiCollectionId: i + 1,
|
||||
smartCollectionId: i + 1,
|
||||
title: `Movie ${i + 1}`
|
||||
})),
|
||||
totalCount
|
||||
};
|
||||
}
|
||||
|
||||
interface MockOptions {
|
||||
browseCount?: number;
|
||||
browseTotal?: number;
|
||||
onRequest?: (url: string, method: string, body: unknown) => Response | null;
|
||||
}
|
||||
|
||||
function mockApi(options: MockOptions = {}) {
|
||||
const browseCount = options.browseCount ?? 3;
|
||||
const browseTotal = options.browseTotal ?? 3;
|
||||
|
||||
return vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL, init?: RequestInit) => {
|
||||
const url = new URL(input.toString(), 'http://localhost');
|
||||
const pathname = url.pathname;
|
||||
const method = (init?.method ?? 'GET').toUpperCase();
|
||||
const body = init?.body ? JSON.parse(String(init.body)) : undefined;
|
||||
|
||||
if (options.onRequest) {
|
||||
const override = options.onRequest(input.toString(), method, body);
|
||||
if (override) {
|
||||
return Promise.resolve(override);
|
||||
}
|
||||
}
|
||||
|
||||
if (pathname === '/api/v1/filler-presets' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse(presetList));
|
||||
}
|
||||
|
||||
if (pathname === '/api/v1/filler-presets/1' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse(editPreset));
|
||||
}
|
||||
|
||||
if (pathname === '/api/v1/library/browse' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse(browsePage(browseCount, browseTotal)));
|
||||
}
|
||||
|
||||
return Promise.resolve(new Response(null, { status: 204 }));
|
||||
});
|
||||
}
|
||||
|
||||
describe('FillerPresetsScreen', () => {
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
vi.restoreAllMocks();
|
||||
window.history.pushState({}, '', '/app/filler-presets');
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
window.localStorage.clear();
|
||||
window.history.pushState({}, '', '/app/filler-presets');
|
||||
});
|
||||
|
||||
it('renders the filler preset list', async () => {
|
||||
mockApi();
|
||||
render(<FillerPresetsScreen />);
|
||||
|
||||
expect(await screen.findByText('Bumper')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('Class B picker: the default Collection-type picker issues exactly ONE /library/browse request (#644 follow-up)', async () => {
|
||||
window.history.pushState({}, '', '/app/filler-presets/add');
|
||||
const fetchMock = mockApi();
|
||||
render(<FillerPresetsScreen />);
|
||||
|
||||
// Default draft collectionType is 'Collection'; wait for its picker to finish loading.
|
||||
await screen.findByText('Movie 1');
|
||||
|
||||
const browseCalls = fetchMock.mock.calls.filter(
|
||||
([u]) => new URL(u.toString(), 'http://localhost').pathname === '/api/v1/library/browse'
|
||||
);
|
||||
expect(browseCalls).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('shows the truncation hint when totalCount exceeds the loaded page, and hides it when it does not', async () => {
|
||||
window.history.pushState({}, '', '/app/filler-presets/add');
|
||||
mockApi({ browseCount: 100, browseTotal: 250 });
|
||||
render(<FillerPresetsScreen />);
|
||||
|
||||
expect(await screen.findByText('Showing the first 100 of 250 — use search to narrow.')).toBeInTheDocument();
|
||||
|
||||
cleanup();
|
||||
|
||||
window.history.pushState({}, '', '/app/filler-presets/add');
|
||||
mockApi({ browseCount: 50, browseTotal: 50 });
|
||||
render(<FillerPresetsScreen />);
|
||||
|
||||
await screen.findByText('Movie 1');
|
||||
expect(screen.queryByText(/Showing the first/)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('F2: injects the out-of-list current selection so an id outside the loaded page still renders as selected', async () => {
|
||||
window.history.pushState({}, '', '/app/filler-presets/1');
|
||||
mockApi();
|
||||
render(<FillerPresetsScreen />);
|
||||
|
||||
await waitFor(() => expect(screen.getByDisplayValue('Bumper')).toBeInTheDocument());
|
||||
|
||||
// Select order: Kind, Mode, Pad to nearest minute, Collection type, then the activeConfig
|
||||
// ("Movie") picker last — it shows the out-of-list #9999 option, selected, never falling back
|
||||
// to "(none)" even though 9999 isn't in the 3-item loaded page.
|
||||
const comboboxes = await screen.findAllByRole('combobox');
|
||||
const pickerSelect = comboboxes[comboboxes.length - 1];
|
||||
expect((pickerSelect as HTMLSelectElement).value).toBe('9999');
|
||||
expect(within(pickerSelect).getByText('#9999')).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
@@ -19,6 +19,14 @@ import {
|
||||
|
||||
const BASE_PATH = '/app/filler-presets';
|
||||
|
||||
// Class B picker (#644 follow-up): the filler-preset collection-type picker browses the largest
|
||||
// media-library tables (Episode, Song, Image, Movie, MusicVideo, ...), which can run into the tens
|
||||
// of thousands of rows. Paging to completeness would mean ~200 serial requests — each more
|
||||
// expensive than the last (LuceneSearchIndex.Search computes hitsLimit = skip + limit) — to
|
||||
// populate a native <select> with thousands of <option> nodes. Load ONE bounded page instead and
|
||||
// surface the truncation (see docs/decisions/records/spa/list-completeness-vs-bounded-pickers.md).
|
||||
const LIBRARY_BROWSE_PAGE_CAP = 100;
|
||||
|
||||
type Draft = CreateFillerPresetRequest;
|
||||
type FillerKind = Draft['fillerKind'];
|
||||
type FillerMode = Draft['fillerMode'];
|
||||
@@ -444,6 +452,8 @@ function FillerEditor({ mode }: { mode: { id: number; kind: 'edit' } | { kind: '
|
||||
const [saving, setSaving] = useState(false);
|
||||
const [pickerItems, setPickerItems] = useState<LibraryBrowseItem[]>([]);
|
||||
const [pickerError, setPickerError] = useState<null | string>(null);
|
||||
const [pickerTruncated, setPickerTruncated] = useState(false);
|
||||
const [pickerTotalCount, setPickerTotalCount] = useState<null | number>(null);
|
||||
|
||||
const isEdit = mode.kind === 'edit';
|
||||
|
||||
@@ -486,17 +496,25 @@ function FillerEditor({ mode }: { mode: { id: number; kind: 'edit' } | { kind: '
|
||||
|
||||
let active = true;
|
||||
|
||||
getLibraryBrowseItems({ mediaType: config.browse, pageSize: 500 })
|
||||
// Class B (#644 follow-up) — load ONE bounded page rather than paging to completeness; see the
|
||||
// module-level comment on LIBRARY_BROWSE_PAGE_CAP.
|
||||
getLibraryBrowseItems({ mediaType: config.browse, pageNum: 0, pageSize: LIBRARY_BROWSE_PAGE_CAP })
|
||||
.then((result) => {
|
||||
if (active) {
|
||||
setPickerItems(result.page ?? []);
|
||||
const page = result.page ?? [];
|
||||
const totalCount = result.totalCount ?? page.length;
|
||||
setPickerItems(page);
|
||||
setPickerError(null);
|
||||
setPickerTruncated(totalCount > page.length);
|
||||
setPickerTotalCount(totalCount);
|
||||
}
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
if (active) {
|
||||
setPickerItems([]);
|
||||
setPickerError(messageFromFillerPresetError(error, 'Unable to load picker items'));
|
||||
setPickerTruncated(false);
|
||||
setPickerTotalCount(null);
|
||||
}
|
||||
});
|
||||
|
||||
@@ -559,14 +577,31 @@ function FillerEditor({ mode }: { mode: { id: number; kind: 'edit' } | { kind: '
|
||||
};
|
||||
|
||||
const pickerValue = activeConfig ? String((draft[activeConfig.field] as null | number) ?? '') : '';
|
||||
const pickerSelectedId = activeConfig ? (draft[activeConfig.field] as null | number) : null;
|
||||
const pickerSelectedInList =
|
||||
pickerSelectedId != null &&
|
||||
pickerItems.some((item) => activeConfig?.itemId(item) === pickerSelectedId);
|
||||
// Out-of-list injection (round-3 review F2): the Class B picker loads only ONE bounded page
|
||||
// (LIBRARY_BROWSE_PAGE_CAP), so a preset whose stored id sits outside that page would otherwise
|
||||
// render as "(none)" while the draft still holds the id — mirrors RerunCollectionsScreen's and
|
||||
// PlaylistsScreen's `selectedInList` prepend.
|
||||
const pickerOptions = [
|
||||
{ label: '(none)', value: '' },
|
||||
...(pickerSelectedId != null && !pickerSelectedInList
|
||||
? [{ label: `#${pickerSelectedId}`, value: String(pickerSelectedId) }]
|
||||
: []),
|
||||
...pickerItems.flatMap((item) => {
|
||||
const id = activeConfig?.itemId(item);
|
||||
return id == null ? [] : [{ label: item.title ?? `#${id}`, value: String(id) }];
|
||||
})
|
||||
];
|
||||
|
||||
const pickerHelp = pickerError
|
||||
? pickerError
|
||||
: pickerTruncated
|
||||
? `Showing the first ${pickerItems.length} of ${pickerTotalCount} — use search to narrow.`
|
||||
: undefined;
|
||||
|
||||
const collectionTypeOptions = COLLECTION_TYPES.filter(
|
||||
(entry) => !entry.playlistOnly || (draft.fillerKind !== 'Fallback' && draft.fillerKind !== 'Tail')
|
||||
).map((entry) => ({ label: entry.label, value: entry.value }));
|
||||
@@ -703,7 +738,7 @@ function FillerEditor({ mode }: { mode: { id: number; kind: 'edit' } | { kind: '
|
||||
/>
|
||||
</Row>
|
||||
{activeConfig && (
|
||||
<Row control={360} help={pickerError ?? undefined} label={activeConfig.label}>
|
||||
<Row control={360} help={pickerHelp} label={activeConfig.label}>
|
||||
<Select
|
||||
onChange={(event) =>
|
||||
set({ [activeConfig.field]: event.target.value === '' ? null : Number(event.target.value) })
|
||||
|
||||
@@ -281,6 +281,35 @@ describe('MultiCollectionsScreen', () => {
|
||||
expect((screen.getByLabelText('Weight for Action') as HTMLInputElement).value).toBe('1');
|
||||
});
|
||||
|
||||
it('pages to completeness: a >100-item list issues a SECOND request and renders every row (#644 follow-up)', async () => {
|
||||
// Unlike `mockApi`, this responds according to the requested pageNum/pageSize — pinning that the
|
||||
// SCREEN itself (not just the `loadAllPages` helper in isolation) actually issues a second
|
||||
// request rather than silently truncating to the server's first-page cap.
|
||||
const total = 120;
|
||||
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((input: RequestInfo | URL) => {
|
||||
const url = new URL(input.toString(), 'http://localhost');
|
||||
if (url.pathname === '/api/v1/multi-collections') {
|
||||
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 }));
|
||||
}
|
||||
return Promise.resolve(new Response(null, { status: 204 }));
|
||||
});
|
||||
|
||||
render(<MultiCollectionsScreen />);
|
||||
|
||||
expect(await screen.findByText('MC 120')).toBeInTheDocument();
|
||||
expect(screen.getByText('MC 1')).toBeInTheDocument();
|
||||
expect(screen.getByText(`${total} multi-collections`)).toBeInTheDocument();
|
||||
|
||||
const listCalls = fetchMock.mock.calls.filter(([u]) => new URL(u.toString(), 'http://localhost').pathname === '/api/v1/multi-collections');
|
||||
expect(listCalls).toHaveLength(2);
|
||||
});
|
||||
|
||||
it('confirms and DELETEs a multi-collection', async () => {
|
||||
const fetchMock = mockApi();
|
||||
|
||||
|
||||
@@ -10,6 +10,7 @@ import {
|
||||
getMultiCollectionWithMeta,
|
||||
getMultiCollections,
|
||||
getSmartCollections,
|
||||
loadAllPages,
|
||||
messageFromMultiCollectionError,
|
||||
updateMultiCollection,
|
||||
type MediaCollection,
|
||||
@@ -21,30 +22,46 @@ import {
|
||||
/* ---------- data hook ---------- */
|
||||
|
||||
type ListState =
|
||||
| { data: MultiCollection[]; error: null; status: 'success' }
|
||||
| { data: MultiCollection[]; error: null; incomplete: boolean; status: 'success' }
|
||||
| { data: null; error: string; status: 'error' }
|
||||
| { data: null; error: null; status: 'loading' };
|
||||
|
||||
function useMultiCollectionsData() {
|
||||
const [state, setState] = useState<ListState>({ data: null, error: null, status: 'loading' });
|
||||
const activeRef = useRef(true);
|
||||
// Monotonic request id (spa-conventions §3): `refresh()` is reachable repeatedly (delete/save),
|
||||
// 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);
|
||||
|
||||
useEffect(() => {
|
||||
activeRef.current = true;
|
||||
return () => {
|
||||
activeRef.current = false;
|
||||
abortRef.current?.abort();
|
||||
};
|
||||
}, []);
|
||||
|
||||
const load = useCallback(() => {
|
||||
getMultiCollections({ pageSize: 1000 })
|
||||
.then((result) => {
|
||||
if (activeRef.current) {
|
||||
setState({ data: result.page ?? [], error: null, status: 'success' });
|
||||
abortRef.current?.abort();
|
||||
const controller = new AbortController();
|
||||
abortRef.current = controller;
|
||||
const seq = (seqRef.current += 1);
|
||||
|
||||
loadAllPages(getMultiCollections, undefined, undefined, controller.signal)
|
||||
.then(({ complete, items }) => {
|
||||
if (activeRef.current && seqRef.current === seq) {
|
||||
if (!complete) {
|
||||
// #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('MultiCollectionsScreen: multi-collections list load did not complete; some items may be missing');
|
||||
}
|
||||
setState({ data: items, error: null, incomplete: !complete, status: 'success' });
|
||||
}
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
if (activeRef.current) {
|
||||
if (activeRef.current && seqRef.current === seq) {
|
||||
setState({ data: null, error: messageFromMultiCollectionError(error), status: 'error' });
|
||||
}
|
||||
});
|
||||
@@ -546,6 +563,9 @@ export function MultiCollectionsScreen() {
|
||||
<div className="ctv-collections">
|
||||
<div className="ctv-channels-actionbar">
|
||||
<Badge tone="neutral">{multiCollections.length} multi-collections</Badge>
|
||||
{state.incomplete && (
|
||||
<Badge tone="warn">List may be incomplete — retry to reload</Badge>
|
||||
)}
|
||||
<span className="ctv-channels-spacer" />
|
||||
<Button onClick={() => setEditing({ kind: 'new' })} size="sm" startIcon={<Plus aria-hidden="true" size={14} />}>
|
||||
New multi-collection
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useCallback, useEffect, useRef, useState } from 'react';
|
||||
import { useCallback, useEffect, useId, useRef, useState } from 'react';
|
||||
import {
|
||||
ArrowDown,
|
||||
ArrowLeft,
|
||||
@@ -31,6 +31,7 @@ import {
|
||||
getPlaylistItemsWithMeta,
|
||||
getPlaylists,
|
||||
getSmartCollections,
|
||||
loadAllPages,
|
||||
messageFromPlaylistError,
|
||||
previewPlaylist,
|
||||
updatePlaylist,
|
||||
@@ -51,6 +52,22 @@ interface PickerOption {
|
||||
name: string;
|
||||
}
|
||||
|
||||
// #644 follow-up: `browse` (media-library) picker sources are Class B — bounded to one page, with
|
||||
// `hint: 'truncated'`/`totalCount` telling the caller there's more than fits (a real, expected
|
||||
// cap — narrow via search). `collection`/`multi`/`smart` sources stay Class A (page to
|
||||
// completeness) since they're inherently small, admin-created lists; `hint: 'incomplete'` there
|
||||
// instead reflects `loadAllPages`'s `complete` flag, i.e. a defensive load that did not converge,
|
||||
// not a cap. These two are NOT the same condition and must render different copy (round-3 review
|
||||
// F1): "showing the first N of M" is arithmetically vacuous — and points at a search box that
|
||||
// doesn't exist for this picker — when N === M on an incomplete Class A load.
|
||||
interface PickerLoadResult {
|
||||
items: PickerOption[];
|
||||
totalCount: number | null;
|
||||
hint: 'incomplete' | 'none' | 'truncated';
|
||||
}
|
||||
|
||||
const LIBRARY_BROWSE_PAGE_CAP = 100;
|
||||
|
||||
// The 12 playlist item types (mirrors PlaylistEditor.razor's Collection Type select).
|
||||
// Playlist / RemoteStream and the rerun-only types are intentionally excluded here.
|
||||
// Each entry knows how to load its picker options; every media-item type maps to a
|
||||
@@ -139,24 +156,47 @@ function orderOptionsWithCurrent(type: CollectionType, current: PlaybackOrder):
|
||||
return options.some((option) => option.value === current) ? options : [...options, orderOption(current)];
|
||||
}
|
||||
|
||||
function loadPickerOptions(type: CollectionType): Promise<PickerOption[]> {
|
||||
function loadPickerOptions(type: CollectionType, signal?: AbortSignal): Promise<PickerLoadResult> {
|
||||
const config = configFor(type);
|
||||
if (!config) {
|
||||
return Promise.resolve([]);
|
||||
return Promise.resolve({ hint: 'none', items: [], totalCount: 0 });
|
||||
}
|
||||
|
||||
switch (config.source) {
|
||||
case 'collection':
|
||||
return getCollections().then((list) => list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` })));
|
||||
return getCollections().then((list) => {
|
||||
const items = list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }));
|
||||
return { hint: 'none' as const, items, totalCount: items.length };
|
||||
});
|
||||
case 'multi':
|
||||
return getMultiCollections({ pageSize: 1000 }).then((result) =>
|
||||
(result.page ?? []).map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }))
|
||||
);
|
||||
return loadAllPages(getMultiCollections, undefined, undefined, signal).then(({ complete, items: list }) => {
|
||||
const items = list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }));
|
||||
if (!complete && !signal?.aborted) {
|
||||
// #644 follow-up F3: a superseded/aborted load (a type switch mid-load) also returns
|
||||
// `complete: false` — that's expected, not a defect, so don't warn on it.
|
||||
console.warn('PlaylistsScreen: multi-collection picker load did not complete; some items may be missing');
|
||||
}
|
||||
return { hint: complete ? ('none' as const) : ('incomplete' as const), items, totalCount: items.length };
|
||||
});
|
||||
case 'smart':
|
||||
return getSmartCollections().then((list) => list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` })));
|
||||
return getSmartCollections().then((list) => {
|
||||
const items = list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }));
|
||||
return { hint: 'none' as const, items, totalCount: items.length };
|
||||
});
|
||||
default:
|
||||
return getLibraryBrowseItems({ mediaType: config.browse, pageSize: 500 }).then((result) =>
|
||||
(result.page ?? []).map((item) => ({ id: item.mediaItemId ?? item.id, name: item.title ?? `#${item.id}` }))
|
||||
// Class B (#644 follow-up): a media-library picker over the largest tables (Episode, Song,
|
||||
// Image, Movie, MusicVideo, ...), which can run into the tens of thousands of rows. Paging to
|
||||
// completeness here would mean ~200 serial requests — each more expensive than the last
|
||||
// (LuceneSearchIndex.Search computes hitsLimit = skip + limit) — to populate a native <select>
|
||||
// with thousands of <option> nodes. Load ONE bounded page instead and surface the truncation
|
||||
// (see docs/decisions/records/spa/list-completeness-vs-bounded-pickers.md).
|
||||
return getLibraryBrowseItems({ mediaType: config.browse, pageNum: 0, pageSize: LIBRARY_BROWSE_PAGE_CAP }).then(
|
||||
(result) => {
|
||||
const page = result.page ?? [];
|
||||
const items = page.map((item) => ({ id: item.mediaItemId ?? item.id, name: item.title ?? `#${item.id}` }));
|
||||
const totalCount = result.totalCount ?? items.length;
|
||||
return { hint: totalCount > items.length ? ('truncated' as const) : ('none' as const), items, totalCount };
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -375,6 +415,9 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
|
||||
const [selectedKey, setSelectedKey] = useState<string | null>(null);
|
||||
const [pickerItems, setPickerItems] = useState<PickerOption[]>([]);
|
||||
const [pickerError, setPickerError] = useState<string | null>(null);
|
||||
const [pickerHint, setPickerHint] = useState<PickerLoadResult['hint']>('none');
|
||||
const [pickerTotalCount, setPickerTotalCount] = useState<number | null>(null);
|
||||
const pickerHelpId = useId();
|
||||
const [saving, setSaving] = useState(false);
|
||||
const [saveError, setSaveError] = useState<string | null>(null);
|
||||
const [previewItems, setPreviewItems] = useState<PlaylistPreviewItem[] | null>(null);
|
||||
@@ -426,29 +469,36 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
|
||||
const selectedType = selectedItem?.collectionType;
|
||||
const isSystem = state.status === 'ready' && state.playlist.isSystem;
|
||||
|
||||
// Load the picker list for the selected item's type. Reset only in the async callbacks.
|
||||
// Load the picker list for the selected item's type. Reset only in the async callbacks. Uses an
|
||||
// AbortController so switching type mid-load (a Class A `loadAllPages` loop, e.g.
|
||||
// multi-collections) stops issuing further requests rather than just discarding the eventual
|
||||
// result (#644 follow-up F2).
|
||||
useEffect(() => {
|
||||
if (selectedType === undefined) {
|
||||
return;
|
||||
}
|
||||
|
||||
let active = true;
|
||||
loadPickerOptions(selectedType)
|
||||
.then((options) => {
|
||||
if (active) {
|
||||
setPickerItems(options);
|
||||
const controller = new AbortController();
|
||||
loadPickerOptions(selectedType, controller.signal)
|
||||
.then(({ hint, items, totalCount }) => {
|
||||
if (!controller.signal.aborted) {
|
||||
setPickerItems(items);
|
||||
setPickerError(null);
|
||||
setPickerHint(hint);
|
||||
setPickerTotalCount(totalCount);
|
||||
}
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
if (active) {
|
||||
if (!controller.signal.aborted) {
|
||||
setPickerItems([]);
|
||||
setPickerError(messageFromPlaylistError(error, 'Unable to load picker items'));
|
||||
setPickerHint('none');
|
||||
setPickerTotalCount(null);
|
||||
}
|
||||
});
|
||||
|
||||
return () => {
|
||||
active = false;
|
||||
controller.abort();
|
||||
};
|
||||
}, [selectedType]);
|
||||
|
||||
@@ -730,16 +780,27 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
|
||||
|
||||
<div style={{ marginTop: 12 }}>
|
||||
<Select
|
||||
ariaDescribedBy={pickerError || pickerHint !== 'none' ? pickerHelpId : undefined}
|
||||
label={activeConfig?.label ?? 'Selection'}
|
||||
onChange={(event) => setItemSelection(selectedItem.key, event.target.value)}
|
||||
options={pickerOptions}
|
||||
value={selectedItem.selectedId == null ? '' : String(selectedItem.selectedId)}
|
||||
/>
|
||||
{pickerError && (
|
||||
<span className="ctv-field-error" role="alert">
|
||||
<span className="ctv-field-error" id={pickerHelpId} role="alert">
|
||||
{pickerError}
|
||||
</span>
|
||||
)}
|
||||
{!pickerError && pickerHint === 'truncated' && (
|
||||
<span className="ctv-field-help" id={pickerHelpId}>
|
||||
Showing the first {pickerItems.length} of {pickerTotalCount} — use search to narrow.
|
||||
</span>
|
||||
)}
|
||||
{!pickerError && pickerHint === 'incomplete' && (
|
||||
<span className="ctv-field-help" id={pickerHelpId}>
|
||||
List may be incomplete — retry to reload.
|
||||
</span>
|
||||
)}
|
||||
</div>
|
||||
|
||||
<div style={{ marginTop: 12 }}>
|
||||
|
||||
@@ -162,6 +162,57 @@ describe('RerunCollectionsScreen', () => {
|
||||
expect(await within(refreshed[1]).findByText('Bundle')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('Class B picker: a media-library type issues exactly ONE /library/browse request and surfaces the truncation hint (#644 follow-up)', async () => {
|
||||
const total = 5000;
|
||||
const cap = 100;
|
||||
const page = Array.from({ length: cap }, (_, i) => ({
|
||||
id: i + 1,
|
||||
mediaItemId: i + 1,
|
||||
mediaType: 'Movie' as const,
|
||||
title: `Movie ${i + 1}`
|
||||
}));
|
||||
|
||||
const fetchMock = mockApi({ list: [] });
|
||||
fetchMock.mockImplementation((input: RequestInfo | URL, init?: RequestInit) => {
|
||||
const url = new URL(input.toString(), 'http://localhost');
|
||||
const method = (init?.method ?? 'GET').toUpperCase();
|
||||
|
||||
if (url.pathname === '/api/v1/library/browse' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse({ page, totalCount: total }));
|
||||
}
|
||||
if (url.pathname === '/api/v1/rerun-collections' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse({ page: [], totalCount: 0 }));
|
||||
}
|
||||
if (url.pathname === '/api/v1/collections' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse(manualCollections));
|
||||
}
|
||||
if (url.pathname === '/api/v1/multi-collections' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse({ page: multiCollections, totalCount: multiCollections.length }));
|
||||
}
|
||||
if (url.pathname === '/api/v1/smart-collections' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse([]));
|
||||
}
|
||||
return Promise.resolve(new Response(null, { status: 204 }));
|
||||
});
|
||||
|
||||
render(<RerunCollectionsScreen />);
|
||||
fireEvent.click(await screen.findByRole('button', { name: 'New rerun collection' }));
|
||||
|
||||
const typeSelect = (await screen.findAllByRole('combobox'))[0];
|
||||
fireEvent.change(typeSelect, { target: { value: 'Movie' } });
|
||||
|
||||
// The bounded page renders (100 options) and the truncation hint appears.
|
||||
expect(await screen.findByText(/Showing the first 100 of 5000/)).toBeInTheDocument();
|
||||
const picker = (await screen.findAllByRole('combobox'))[1];
|
||||
expect(await within(picker).findByText('Movie 100')).toBeInTheDocument();
|
||||
|
||||
// Exactly ONE /library/browse request — no paging-to-completeness loop over the media library.
|
||||
const browseCalls = fetchMock.mock.calls.filter(
|
||||
([u]) => new URL(u.toString(), 'http://localhost').pathname === '/api/v1/library/browse'
|
||||
);
|
||||
expect(browseCalls).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('preserves an out-of-set stored playback order on edit-load and re-saves it unchanged', async () => {
|
||||
const outOfSet = [
|
||||
{
|
||||
@@ -214,6 +265,100 @@ describe('RerunCollectionsScreen', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('renders the "List may be incomplete" badge when the rerun-collections load does not complete (round-3 review)', async () => {
|
||||
let callCount = 0;
|
||||
|
||||
vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL, init?: RequestInit) => {
|
||||
const url = new URL(input.toString(), 'http://localhost');
|
||||
const method = (init?.method ?? 'GET').toUpperCase();
|
||||
|
||||
if (url.pathname === '/api/v1/rerun-collections' && method === 'GET') {
|
||||
callCount += 1;
|
||||
// First page reports totalCount 2 but only returns 1 item; the second page (pageNum=1)
|
||||
// then comes back EMPTY, which loadAllPages treats as a defensive break: `complete: false`.
|
||||
if (callCount === 1) {
|
||||
return Promise.resolve(jsonResponse({ page: [rerunCollections[0]], totalCount: 2 }));
|
||||
}
|
||||
return Promise.resolve(jsonResponse({ page: [], totalCount: 2 }));
|
||||
}
|
||||
|
||||
return Promise.resolve(new Response(null, { status: 204 }));
|
||||
});
|
||||
|
||||
render(<RerunCollectionsScreen />);
|
||||
|
||||
expect(await screen.findByText('List may be incomplete — retry to reload')).toBeInTheDocument();
|
||||
expect(callCount).toBe(2);
|
||||
});
|
||||
|
||||
it('screen-level: an older overlapping refresh must NOT overwrite a newer one (seqRef guard, round-3 review)', async () => {
|
||||
const itemA = { ...rerunCollections[0], id: 101, name: 'Item A' };
|
||||
const itemB = { ...rerunCollections[0], id: 102, name: 'Item B' };
|
||||
const staleAfterA = { ...rerunCollections[0], id: 201, name: 'Stale After A' };
|
||||
const freshAfterB = { ...rerunCollections[0], id: 202, name: 'Fresh After B' };
|
||||
|
||||
let listCallCount = 0;
|
||||
const staleRelease: { resolve: (() => void) | null } = { resolve: null };
|
||||
|
||||
const fetchMock = vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL, init?: RequestInit) => {
|
||||
const url = new URL(input.toString(), 'http://localhost');
|
||||
const method = (init?.method ?? 'GET').toUpperCase();
|
||||
|
||||
if (url.pathname === '/api/v1/rerun-collections' && method === 'GET') {
|
||||
listCallCount += 1;
|
||||
// Call 1: initial mount load. Call 2: refresh after deleting A — held pending (the OLDER
|
||||
// overlapping load). Call 3: refresh after deleting B — resolves immediately (the NEWER
|
||||
// load), before call 2 is ever released.
|
||||
if (listCallCount === 1) {
|
||||
return Promise.resolve(jsonResponse({ page: [itemA, itemB], totalCount: 2 }));
|
||||
}
|
||||
if (listCallCount === 2) {
|
||||
return new Promise<Response>((resolve) => {
|
||||
staleRelease.resolve = () => resolve(jsonResponse({ page: [staleAfterA], totalCount: 1 }));
|
||||
});
|
||||
}
|
||||
return Promise.resolve(jsonResponse({ page: [freshAfterB], totalCount: 1 }));
|
||||
}
|
||||
|
||||
if (/^\/api\/v1\/rerun-collections\/\d+$/.test(url.pathname) && method === 'DELETE') {
|
||||
return Promise.resolve(new Response(null, { status: 204 }));
|
||||
}
|
||||
|
||||
return Promise.resolve(new Response(null, { status: 204 }));
|
||||
});
|
||||
|
||||
render(<RerunCollectionsScreen />);
|
||||
|
||||
expect(await screen.findByText('Item A')).toBeInTheDocument();
|
||||
expect(screen.getByText('Item B')).toBeInTheDocument();
|
||||
|
||||
// Delete A -> refresh() issues the OLDER overlapping load (call 2), held pending.
|
||||
const rowA = screen.getByText('Item A').closest('.ctv-settings-flush-row') as HTMLElement;
|
||||
fireEvent.click(within(rowA).getByRole('button', { name: 'Delete' }));
|
||||
fireEvent.click(within(await screen.findByRole('dialog')).getByRole('button', { name: 'Delete' }));
|
||||
|
||||
await waitFor(() => expect(listCallCount).toBe(2));
|
||||
// The pending call 2 hasn't resolved, so the list (and Item B) is still showing.
|
||||
expect(screen.getByText('Item B')).toBeInTheDocument();
|
||||
|
||||
// Delete B -> refresh() issues the NEWER load (call 3), which resolves right away.
|
||||
const rowB = screen.getByText('Item B').closest('.ctv-settings-flush-row') as HTMLElement;
|
||||
fireEvent.click(within(rowB).getByRole('button', { name: 'Delete' }));
|
||||
fireEvent.click(within(await screen.findByRole('dialog')).getByRole('button', { name: 'Delete' }));
|
||||
|
||||
expect(await screen.findByText('Fresh After B')).toBeInTheDocument();
|
||||
|
||||
// Now release the OLDER (call 2) load. Its seq no longer matches seqRef.current, so it must
|
||||
// NOT overwrite the already-rendered newer result.
|
||||
staleRelease.resolve?.();
|
||||
await waitFor(() => expect(fetchMock.mock.calls.length).toBeGreaterThan(0));
|
||||
// Let the resolved (but stale) promise's `.then` run.
|
||||
await new Promise((resolve) => setTimeout(resolve, 0));
|
||||
|
||||
expect(screen.queryByText('Stale After A')).not.toBeInTheDocument();
|
||||
expect(screen.getByText('Fresh After B')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('confirms and DELETEs a rerun collection', async () => {
|
||||
const fetchMock = mockApi();
|
||||
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useCallback, useEffect, useRef, useState } from 'react';
|
||||
import { useCallback, useEffect, useId, useRef, useState } from 'react';
|
||||
import { ArrowLeft, Check, Plus, Repeat, Trash2, TriangleAlert } from 'lucide-react';
|
||||
import { Badge, Button, Card, ConfirmDialog, IconButton, Input, Select, Spinner } from '../components';
|
||||
import type { SelectOption } from '../components';
|
||||
@@ -13,6 +13,7 @@ import {
|
||||
getRerunCollectionWithMeta,
|
||||
getRerunCollections,
|
||||
getSmartCollections,
|
||||
loadAllPages,
|
||||
messageFromRerunCollectionError,
|
||||
updateRerunCollection,
|
||||
type CreateRerunCollectionRequest,
|
||||
@@ -28,6 +29,22 @@ interface PickerOption {
|
||||
name: string;
|
||||
}
|
||||
|
||||
// #644 follow-up: `browse` (media-library) picker sources are Class B — bounded to one page, with
|
||||
// `hint: 'truncated'`/`totalCount` telling the caller there's more than fits (a real, expected
|
||||
// cap — narrow via search). `collection`/`multi`/`smart` sources stay Class A (page to
|
||||
// completeness) since they're inherently small, admin-created lists; `hint: 'incomplete'` there
|
||||
// instead reflects `loadAllPages`'s `complete` flag, i.e. a defensive load that did not converge,
|
||||
// not a cap. These two are NOT the same condition and must render different copy (round-3 review
|
||||
// F1): "showing the first N of M" is arithmetically vacuous — and points at a search box that
|
||||
// doesn't exist for this picker — when N === M on an incomplete Class A load.
|
||||
interface PickerLoadResult {
|
||||
items: PickerOption[];
|
||||
totalCount: number | null;
|
||||
hint: 'incomplete' | 'none' | 'truncated';
|
||||
}
|
||||
|
||||
const LIBRARY_BROWSE_PAGE_CAP = 100;
|
||||
|
||||
// The REST-supported selection types (RerunCollectionRequestMapping.IsSupportedSelectionType).
|
||||
// Playlist / RerunFirstRun / RerunRerun / SearchQuery / Fake* are intentionally excluded.
|
||||
// Each entry knows how to load its picker options; every media-item type maps to a
|
||||
@@ -92,24 +109,47 @@ function orderOptionsWithCurrent(type: RerunCollectionType, current: PlaybackOrd
|
||||
return options.some((option) => option.value === current) ? options : [...options, orderOption(current)];
|
||||
}
|
||||
|
||||
function loadPickerOptions(type: RerunCollectionType): Promise<PickerOption[]> {
|
||||
function loadPickerOptions(type: RerunCollectionType, signal?: AbortSignal): Promise<PickerLoadResult> {
|
||||
const config = COLLECTION_TYPES.find((entry) => entry.value === type);
|
||||
if (!config) {
|
||||
return Promise.resolve([]);
|
||||
return Promise.resolve({ hint: 'none', items: [], totalCount: 0 });
|
||||
}
|
||||
|
||||
switch (config.source) {
|
||||
case 'collection':
|
||||
return getCollections().then((list) => list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` })));
|
||||
return getCollections().then((list) => {
|
||||
const items = list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }));
|
||||
return { hint: 'none' as const, items, totalCount: items.length };
|
||||
});
|
||||
case 'multi':
|
||||
return getMultiCollections({ pageSize: 1000 }).then((result) =>
|
||||
(result.page ?? []).map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }))
|
||||
);
|
||||
return loadAllPages(getMultiCollections, undefined, undefined, signal).then(({ complete, items: list }) => {
|
||||
const items = list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }));
|
||||
if (!complete && !signal?.aborted) {
|
||||
// #644 follow-up F3: a superseded/aborted load (Retry, or a type switch mid-load) also
|
||||
// returns `complete: false` — that's expected, not a defect, so don't warn on it.
|
||||
console.warn('RerunCollectionsScreen: multi-collection picker load did not complete; some items may be missing');
|
||||
}
|
||||
return { hint: complete ? ('none' as const) : ('incomplete' as const), items, totalCount: items.length };
|
||||
});
|
||||
case 'smart':
|
||||
return getSmartCollections().then((list) => list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` })));
|
||||
return getSmartCollections().then((list) => {
|
||||
const items = list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }));
|
||||
return { hint: 'none' as const, items, totalCount: items.length };
|
||||
});
|
||||
default:
|
||||
return getLibraryBrowseItems({ mediaType: config.browse, pageSize: 500 }).then((result) =>
|
||||
(result.page ?? []).map((item) => ({ id: item.mediaItemId ?? item.id, name: item.title ?? `#${item.id}` }))
|
||||
// Class B (#644 follow-up): a media-library picker over the largest tables (Episode, Song,
|
||||
// Image, Movie, MusicVideo, ...), which can run into the tens of thousands of rows. Paging to
|
||||
// completeness here would mean ~200 serial requests — each more expensive than the last
|
||||
// (LuceneSearchIndex.Search computes hitsLimit = skip + limit) — to populate a native <select>
|
||||
// with thousands of <option> nodes. Load ONE bounded page instead and surface the truncation
|
||||
// (see docs/decisions/records/spa/list-completeness-vs-bounded-pickers.md).
|
||||
return getLibraryBrowseItems({ mediaType: config.browse, pageNum: 0, pageSize: LIBRARY_BROWSE_PAGE_CAP }).then(
|
||||
(result) => {
|
||||
const page = result.page ?? [];
|
||||
const items = page.map((item) => ({ id: item.mediaItemId ?? item.id, name: item.title ?? `#${item.id}` }));
|
||||
const totalCount = result.totalCount ?? items.length;
|
||||
return { hint: totalCount > items.length ? ('truncated' as const) : ('none' as const), items, totalCount };
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -117,30 +157,46 @@ function loadPickerOptions(type: RerunCollectionType): Promise<PickerOption[]> {
|
||||
/* ---------- data hook ---------- */
|
||||
|
||||
type ListState =
|
||||
| { data: RerunCollection[]; error: null; status: 'success' }
|
||||
| { data: RerunCollection[]; error: null; incomplete: boolean; status: 'success' }
|
||||
| { data: null; error: string; status: 'error' }
|
||||
| { data: null; error: null; status: 'loading' };
|
||||
|
||||
function useRerunCollectionsData() {
|
||||
const [state, setState] = useState<ListState>({ data: null, error: null, status: 'loading' });
|
||||
const activeRef = useRef(true);
|
||||
// Monotonic request id (spa-conventions §3): `refresh()` is reachable repeatedly (delete/save),
|
||||
// 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);
|
||||
|
||||
useEffect(() => {
|
||||
activeRef.current = true;
|
||||
return () => {
|
||||
activeRef.current = false;
|
||||
abortRef.current?.abort();
|
||||
};
|
||||
}, []);
|
||||
|
||||
const load = useCallback(() => {
|
||||
getRerunCollections({ pageSize: 1000 })
|
||||
.then((result) => {
|
||||
if (activeRef.current) {
|
||||
setState({ data: result.page ?? [], error: null, status: 'success' });
|
||||
abortRef.current?.abort();
|
||||
const controller = new AbortController();
|
||||
abortRef.current = controller;
|
||||
const seq = (seqRef.current += 1);
|
||||
|
||||
loadAllPages(getRerunCollections, undefined, undefined, controller.signal)
|
||||
.then(({ complete, items }) => {
|
||||
if (activeRef.current && seqRef.current === seq) {
|
||||
if (!complete) {
|
||||
// #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('RerunCollectionsScreen: rerun-collections list load did not complete; some items may be missing');
|
||||
}
|
||||
setState({ data: items, error: null, incomplete: !complete, status: 'success' });
|
||||
}
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
if (activeRef.current) {
|
||||
if (activeRef.current && seqRef.current === seq) {
|
||||
setState({ data: null, error: messageFromRerunCollectionError(error), status: 'error' });
|
||||
}
|
||||
});
|
||||
@@ -209,6 +265,9 @@ function RerunCollectionEditor({
|
||||
);
|
||||
const [pickerItems, setPickerItems] = useState<PickerOption[]>([]);
|
||||
const [pickerError, setPickerError] = useState<string | null>(null);
|
||||
const [pickerHint, setPickerHint] = useState<PickerLoadResult['hint']>('none');
|
||||
const [pickerTotalCount, setPickerTotalCount] = useState<number | null>(null);
|
||||
const pickerHelpId = useId();
|
||||
const [saveError, setSaveError] = useState<string | null>(null);
|
||||
const [saving, setSaving] = useState(false);
|
||||
const [conflictOpen, setConflictOpen] = useState(false);
|
||||
@@ -248,26 +307,32 @@ function RerunCollectionEditor({
|
||||
}, [initial, reloadKey]);
|
||||
|
||||
// Load the picker list for the active type. Resets only in the async callbacks (never
|
||||
// synchronously in the effect body) per spa-conventions §3.
|
||||
// synchronously in the effect body) per spa-conventions §3. Uses an AbortController so a type
|
||||
// switch mid-load (a Class A `loadAllPages` loop, e.g. multi-collections) stops issuing further
|
||||
// requests rather than just discarding the eventual result (#644 follow-up F2).
|
||||
useEffect(() => {
|
||||
let active = true;
|
||||
const controller = new AbortController();
|
||||
|
||||
loadPickerOptions(collectionType)
|
||||
.then((items) => {
|
||||
if (active) {
|
||||
loadPickerOptions(collectionType, controller.signal)
|
||||
.then(({ hint, items, totalCount }) => {
|
||||
if (!controller.signal.aborted) {
|
||||
setPickerItems(items);
|
||||
setPickerError(null);
|
||||
setPickerHint(hint);
|
||||
setPickerTotalCount(totalCount);
|
||||
}
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
if (active) {
|
||||
if (!controller.signal.aborted) {
|
||||
setPickerItems([]);
|
||||
setPickerError(messageFromRerunCollectionError(error, 'Unable to load picker items'));
|
||||
setPickerHint('none');
|
||||
setPickerTotalCount(null);
|
||||
}
|
||||
});
|
||||
|
||||
return () => {
|
||||
active = false;
|
||||
controller.abort();
|
||||
};
|
||||
}, [collectionType]);
|
||||
|
||||
@@ -391,16 +456,27 @@ function RerunCollectionEditor({
|
||||
|
||||
<div style={{ marginTop: 12 }}>
|
||||
<Select
|
||||
ariaDescribedBy={pickerError || pickerHint !== 'none' ? pickerHelpId : undefined}
|
||||
label={activeConfig?.label ?? 'Selection'}
|
||||
onChange={(event) => setSelected(event.target.value)}
|
||||
options={pickerOptions}
|
||||
value={draft.selectedId == null ? '' : String(draft.selectedId)}
|
||||
/>
|
||||
{pickerError && (
|
||||
<span className="ctv-field-error" role="alert">
|
||||
<span className="ctv-field-error" id={pickerHelpId} role="alert">
|
||||
{pickerError}
|
||||
</span>
|
||||
)}
|
||||
{!pickerError && pickerHint === 'truncated' && (
|
||||
<span className="ctv-field-help" id={pickerHelpId}>
|
||||
Showing the first {pickerItems.length} of {pickerTotalCount} — use search to narrow.
|
||||
</span>
|
||||
)}
|
||||
{!pickerError && pickerHint === 'incomplete' && (
|
||||
<span className="ctv-field-help" id={pickerHelpId}>
|
||||
List may be incomplete — retry to reload.
|
||||
</span>
|
||||
)}
|
||||
</div>
|
||||
|
||||
<div style={{ marginTop: 12 }}>
|
||||
@@ -518,6 +594,9 @@ export function RerunCollectionsScreen() {
|
||||
<div className="ctv-collections">
|
||||
<div className="ctv-channels-actionbar">
|
||||
<Badge tone="neutral">{rerunCollections.length} rerun collections</Badge>
|
||||
{state.incomplete && (
|
||||
<Badge tone="warn">List may be incomplete — retry to reload</Badge>
|
||||
)}
|
||||
<span className="ctv-channels-spacer" />
|
||||
<Button onClick={() => setEditing({ kind: 'new' })} size="sm" startIcon={<Plus aria-hidden="true" size={14} />}>
|
||||
New rerun collection
|
||||
|
||||
@@ -20,6 +20,7 @@ import {
|
||||
getSchedules,
|
||||
getGraphicsElements,
|
||||
getWatermarks,
|
||||
loadAllPages,
|
||||
messageFromScheduleError,
|
||||
type ProgramSchedule,
|
||||
replaceScheduleItems,
|
||||
@@ -45,33 +46,24 @@ 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<RerunCollection[]> {
|
||||
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;
|
||||
// `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(signal?: AbortSignal): Promise<RerunCollection[]> {
|
||||
return loadAllPages(getRerunCollections, undefined, undefined, signal).then(({ complete, items }) => {
|
||||
if (!complete && !signal?.aborted) {
|
||||
// #644 follow-up F4: a partial result is otherwise indistinguishable from a complete one —
|
||||
// log it so a stalled/incomplete rerun-collections load doesn't silently render as whole.
|
||||
// A superseded/aborted load (retry, or a newer boot supersedes this one — see the abort
|
||||
// below) also returns `complete: false`; that's expected, not a defect, so don't warn on it
|
||||
// (round-3 review F3).
|
||||
console.warn('SchedulesScreen: rerun-collections picker load did not complete; some items may be missing');
|
||||
}
|
||||
|
||||
all = [...all, ...nextPage];
|
||||
pageNum += 1;
|
||||
}
|
||||
|
||||
return all;
|
||||
return items;
|
||||
});
|
||||
}
|
||||
|
||||
type BootState =
|
||||
@@ -83,10 +75,10 @@ type BootState =
|
||||
// getFillerPresetsByKind) hit endpoints that return a plain, unpaged array server-side — no
|
||||
// pageNum/pageSize params, no clamp — so they aren't subject to the same silent-truncation defect
|
||||
// as rerun collections and don't need the same treatment (ersatztv#634).
|
||||
async function loadPickerData(): Promise<SchedulePickerData> {
|
||||
async function loadPickerData(signal?: AbortSignal): Promise<SchedulePickerData> {
|
||||
const [rerunCollections, playlistGroups, watermarks, graphicsElements, languages, pre, mid, post, tail, fallback] =
|
||||
await Promise.all([
|
||||
loadAllRerunCollections(),
|
||||
loadAllRerunCollections(signal),
|
||||
getPlaylistGroups(),
|
||||
getWatermarks(),
|
||||
getGraphicsElements(),
|
||||
@@ -136,6 +128,13 @@ export function SchedulesScreen() {
|
||||
// Last-seen concurrency ETag: set from the items GET, replaced by every successful save's response
|
||||
// ETag (a same-tab second save must use the new tag or it would 412 against its own write) (#253).
|
||||
const etagRef = useRef<string | null>(null);
|
||||
// Monotonic request id + AbortController for the bootstrap load (spa-conventions §3 / #644
|
||||
// follow-up F2, F3): the Retry button can re-trigger `loadBootstrap` while a previous call
|
||||
// (whose `loadAllRerunCollections` may still be mid-loop) is in flight — abort the stale
|
||||
// controller so it stops issuing further paging requests, and guard on the seq so a stale
|
||||
// resolve can't overwrite a newer one.
|
||||
const bootSeqRef = useRef(0);
|
||||
const bootAbortRef = useRef<AbortController | null>(null);
|
||||
|
||||
const setDirtyState = useCallback((value: boolean) => {
|
||||
dirtyRef.current = value;
|
||||
@@ -146,6 +145,7 @@ export function SchedulesScreen() {
|
||||
activeRef.current = true;
|
||||
return () => {
|
||||
activeRef.current = false;
|
||||
bootAbortRef.current?.abort();
|
||||
};
|
||||
}, []);
|
||||
|
||||
@@ -153,16 +153,21 @@ export function SchedulesScreen() {
|
||||
// Only sets state in promise callbacks (never synchronously in the effect body — see
|
||||
// spa-conventions.md §3). The retry button flips to 'loading' itself before calling this.
|
||||
const loadBootstrap = useCallback(() => {
|
||||
Promise.all([getSchedules(), loadPickerData()])
|
||||
bootAbortRef.current?.abort();
|
||||
const controller = new AbortController();
|
||||
bootAbortRef.current = controller;
|
||||
const seq = (bootSeqRef.current += 1);
|
||||
|
||||
Promise.all([getSchedules(), loadPickerData(controller.signal)])
|
||||
.then(([schedules, pickers]) => {
|
||||
if (!activeRef.current) {
|
||||
if (!activeRef.current || bootSeqRef.current !== seq) {
|
||||
return;
|
||||
}
|
||||
setBoot({ status: 'ready', schedules, pickers });
|
||||
setActiveId((current) => current ?? schedules[0]?.id ?? null);
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
if (activeRef.current) {
|
||||
if (activeRef.current && bootSeqRef.current === seq) {
|
||||
setBoot({ status: 'error', error: messageFromScheduleError(error, 'Unable to load schedules') });
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user