fix(644): stop seven SPA list loads truncating silently — one shared pager, bounded media pickers #656

Merged

timothy merged 3 commits from fix/644-spa-paging into main 2026-07-26 22:10:04 +02:00

Conversation 1 Commits 3 Files Changed 16

+1155 -85

timothy commented 2026-07-26 21:38:46 +02:00

Owner

Copy Link

Copy Source

fixes #644

Extracts one shared paging helper and stops seven list loads silently truncating at the server cap — but not in the way the issue originally specified. The scope change is deliberate and is recorded on the issue.

The scope amendment, up front

The issue asked for all seven call sites to "return the complete list regardless of count." A cold review established that doing so for three of them would be worse than the bug.

RerunCollectionsScreen, PlaylistsScreen and FillerPresetsScreen each populate a native <select> from getLibraryBrowseItems, whose mediaType can be Episode, Song, Image, Movie or MusicVideo — the largest tables in any install. Paging those to completeness on a 20k-episode library means ~200 serial requests on effect mount, each more expensive than the last (LuceneSearchIndex.Search computes hitsLimit = skip + limit and collects that many sorted top-docs, then runs a second search for the page map), feeding tens of thousands of <option> nodes into one dropdown.

So the sites are split into two classes:

Class	Sites	Behaviour
A — bounded, admin-created	multi-collections, rerun-collections (+ the #634 SchedulesScreen loader)	Page to completeness
B — media-library pickers	the three getLibraryBrowseItems pickers	One bounded page + a visible truncation hint

The defect this issue names is silent truncation. Class B removes the silence; making those pickers genuinely usable needs a searchable source, tracked as #651. #644's first Done-when box was amended rather than ticked, because I chose not to satisfy it as written.

What shipped

• web/src/api/paging.ts — loadAllPages, one implementation, returning { items, complete }. SchedulesScreen's #634 loop is now a delegate, so no eighth copy survives.
• Class B pickers request one page and render "Showing the first N of M — use search to narrow."
• Server caps unchanged. api.search-allitems-paging is that the client pages and the server stays bounded. All three caps verified as MaxPageSize = 100 by reading the controllers, not trusting the issue.

Review rounds

Three rounds; each found real defects in the previous round's fix.

Round 2 (after the Class A/B split): no cancellation — a superseded N-request loop kept issuing requests, harmless for one request and not for N; the empty-page break returned a partial list indistinguishable from a complete one, reachable in normal operation because HydrateMediaItems drops hits whose DB rows vanished and Lucene totalCount can be stale — i.e. the same silent-truncation class this issue is about, reintroduced inside its own fix; a stale-overwrite window widened from one RTT to N; a generic typing hole that let a loader with a required param compile without it; and an O(n²) accumulator.

Round 3: one boolean was carrying two meanings, so an incomplete Class A load rendered "Showing the first 47 of 47 — use search to narrow" — arithmetically vacuous, pointing at a search box that doesn't exist for that picker. Now a hint: 'truncated' | 'incomplete' | 'none' discriminator. Also: FillerPresetsScreen and ScheduleItemInspector lacked the out-of-list current-selection injection the other two screens had, so editing a record whose id sits past row 100 displayed (none) while the draft still held the real id; a spurious console.warn fired on deliberately-aborted loads; the hint wasn't associated with its <select>; and FillerPresetsScreen had no test file at all.

The round-2 review ran a negative control on the typing fix — reverting the conditional rest-tuple and confirming tsc then reports TS2578: Unused '@ts-expect-error' directive, proving that guard is load-bearing rather than decorative.

Verification

• npm test → 107 files, 1019 tests passed
• npm run lint → clean
• npm run build → tsc -b && vite build succeeded (pre-existing >500 kB chunk advisory only)
• decisions-validate: OK; catalog regenerated

Tests pin exact sets, never non-empty checks: the 250-item case asserts the full 1..250 id sequence and the per-call pageNum/pageSize, including both boundaries (99/100, 199/200). Cancellation tests assert exact fetch call counts — the only thing that proves no further requests. Class B tests pin exactly one request and the hint's presence/absence either side of the cap.

Known limitation, stated rather than glossed: the AbortSignal stops the loop issuing further pages; it is not threaded into the domain loaders, so an already-in-flight request still completes. Doing that would touch every module in web/src/api/. The docs say "stops issuing further page requests", which is accurate.

Docs

docs/spa-conventions.md §3b and a new decision record spa.list-completeness-vs-bounded-pickers covering the Class A/B split and the hint discriminator.

Also filed from these reviews

• #650 — two more truncations that request exactly the cap, so this PR's over-cap grep guard structurally could not see them. TraktListsScreen renders "101 lists" above 100 rows; ChannelBuilder.loadCollections reports the truncated length as the total so canLoadMore can never fire. The guard keys on the symptom (an inflated pageSize), not the defect (treating one page as the whole list).
• #651 — searchable library-browse pickers.

fixes #644 Extracts one shared paging helper and stops seven list loads silently truncating at the server cap — but **not** in the way the issue originally specified. The scope change is deliberate and is recorded on the issue. ## The scope amendment, up front The issue asked for all seven call sites to "return the complete list regardless of count." A cold review established that doing so for three of them would be **worse than the bug**. `RerunCollectionsScreen`, `PlaylistsScreen` and `FillerPresetsScreen` each populate a native `<select>` from `getLibraryBrowseItems`, whose `mediaType` can be `Episode`, `Song`, `Image`, `Movie` or `MusicVideo` — the largest tables in any install. Paging those to completeness on a 20k-episode library means ~200 **serial** requests on effect mount, each more expensive than the last (`LuceneSearchIndex.Search` computes `hitsLimit = skip + limit` and collects that many sorted top-docs, then runs a second search for the page map), feeding tens of thousands of `<option>` nodes into one dropdown. So the sites are split into two classes: | Class | Sites | Behaviour | |---|---|---| | **A** — bounded, admin-created | multi-collections, rerun-collections (+ the #634 `SchedulesScreen` loader) | Page to completeness | | **B** — media-library pickers | the three `getLibraryBrowseItems` pickers | One bounded page + a **visible** truncation hint | The defect this issue names is **silent** truncation. Class B removes the silence; making those pickers genuinely usable needs a searchable source, tracked as **#651**. `#644`'s first Done-when box was amended rather than ticked, because I chose not to satisfy it as written. ## What shipped - `web/src/api/paging.ts` — `loadAllPages`, one implementation, returning `{ items, complete }`. `SchedulesScreen`'s #634 loop is now a delegate, so no eighth copy survives. - Class B pickers request one page and render "Showing the first N of M — use search to narrow." - **Server caps unchanged.** `api.search-allitems-paging` is that the client pages and the server stays bounded. All three caps verified as `MaxPageSize = 100` by reading the controllers, not trusting the issue. ## Review rounds Three rounds; each found real defects in the previous round's fix. **Round 2** (after the Class A/B split): no cancellation — a superseded N-request loop kept issuing requests, harmless for one request and not for N; the empty-page `break` returned a partial list indistinguishable from a complete one, *reachable in normal operation* because `HydrateMediaItems` drops hits whose DB rows vanished and Lucene `totalCount` can be stale — i.e. the same silent-truncation class this issue is about, reintroduced inside its own fix; a stale-overwrite window widened from one RTT to N; a generic typing hole that let a loader with a required param compile without it; and an O(n²) accumulator. **Round 3**: one boolean was carrying two meanings, so an incomplete Class A load rendered "Showing the first 47 of 47 — use search to narrow" — arithmetically vacuous, pointing at a search box that doesn't exist for that picker. Now a `hint: 'truncated' | 'incomplete' | 'none'` discriminator. Also: `FillerPresetsScreen` and `ScheduleItemInspector` lacked the out-of-list current-selection injection the other two screens had, so editing a record whose id sits past row 100 displayed `(none)` while the draft still held the real id; a spurious `console.warn` fired on deliberately-aborted loads; the hint wasn't associated with its `<select>`; and `FillerPresetsScreen` had no test file at all. The round-2 review ran a **negative control** on the typing fix — reverting the conditional rest-tuple and confirming `tsc` then reports `TS2578: Unused '@ts-expect-error' directive`, proving that guard is load-bearing rather than decorative. ## Verification - `npm test` → **107 files, 1019 tests passed** - `npm run lint` → clean - `npm run build` → `tsc -b && vite build` succeeded (pre-existing >500 kB chunk advisory only) - `decisions-validate: OK`; catalog regenerated Tests pin **exact** sets, never non-empty checks: the 250-item case asserts the full 1..250 id sequence *and* the per-call `pageNum`/`pageSize`, including both boundaries (99/100, 199/200). Cancellation tests assert exact fetch **call counts** — the only thing that proves no further requests. Class B tests pin exactly one request and the hint's presence/absence either side of the cap. **Known limitation, stated rather than glossed:** the `AbortSignal` stops the loop *issuing further pages*; it is not threaded into the domain loaders, so an already-in-flight request still completes. Doing that would touch every module in `web/src/api/`. The docs say "stops issuing further page requests", which is accurate. ## Docs `docs/spa-conventions.md` §3b and a new decision record `spa.list-completeness-vs-bounded-pickers` covering the Class A/B split and the `hint` discriminator. ## Also filed from these reviews - **#650** — two more truncations that request *exactly* the cap, so this PR's over-cap grep guard structurally could not see them. `TraktListsScreen` renders "101 lists" above 100 rows; `ChannelBuilder.loadCollections` reports the truncated length as the total so `canLoadMore` can never fire. The guard keys on the symptom (an inflated `pageSize`), not the defect (treating one page as the whole list). - **#651** — searchable library-browse pickers.

timothy added 3 commits 2026-07-26 21:38:46 +02:00

fix(644): page SPA list loaders to completeness instead of inflating pageSize ... fe342a6a0b

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.

fix(644): split loadAllPages by list class; bound media-library pickers to one page ... 94182cdd53

Cold adversarial review of fe342a6a found the blanket loadAllPages-everywhere fix
dangerous for the three getLibraryBrowseItems pickers (RerunCollectionsScreen,
PlaylistsScreen, FillerPresetsScreen): paging Episode/Song/Image/Movie/MusicVideo
to completeness can mean ~200 serial requests against a 20k-row library, each more
expensive than the last, to populate a <select> with thousands of <option> nodes.

- Class A (bounded-by-construction lists: rerun collections, multi-collections,
  playlists) keep loadAllPages. Class B (media-library pickers) now fetch ONE
  bounded page and surface truncation via a `Showing the first N of M` hint wired
  to the real totalCount, instead of paging to completeness or truncating silently.
- loadAllPages: reports `{ items, complete }` instead of just `T[]` so a caller
  can no longer mistake a defensive empty-page break for a full list (F4); accepts
  an optional AbortSignal so a superseded loop stops issuing further page requests
  (F2); baseParams is now required via a conditional rest-tuple whenever the
  loader's params type has a field beyond pageNum/pageSize (F6); pushes into the
  accumulator instead of re-spreading it every page (F7).
- MultiCollectionsScreen/RerunCollectionsScreen/SchedulesScreen: add a seqRef +
  AbortController guard around the list/bootstrap loads so a stale loadAllPages
  loop can't resolve after a newer one and resurrect deleted rows (F3); log and
  surface an incomplete load rather than rendering it as whole.
- docs/spa-conventions.md §3b rewritten for the Class A / Class B split; new
  decision record docs/decisions/records/spa/list-completeness-vs-bounded-pickers.md
  (spa.list-completeness-vs-bounded-pickers), catalog regenerated.
- Tests: paging.test.ts covers null/undefined totalCount, null page, a
  short-but-non-empty page, a page-2 rejection, the complete:false flag, and
  cancellation (asserting fetch call COUNT stays put after abort), plus a
  compile-time @ts-expect-error pinning the F6 typing fix. Screen-level tests
  pin a real second HTTP request for a >100-item Class A list
  (MultiCollectionsScreen) and exactly one /library/browse request plus the
  truncation hint for a Class B picker (RerunCollectionsScreen).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(644): round-3 review — split truncated/incomplete picker hints, F2 out-of-list gaps, F3 abort warns, F4 aria wiring, F5 FillerPresetsScreen tests ... daedf003e5

Addresses the round-3 cold re-review's five low-severity findings on #644's client-side paging fix:

- F1: `loadPickerOptions` (RerunCollectionsScreen, PlaylistsScreen) returned one `truncated:
  boolean` for two different conditions — a real Class B cap hit vs an unconverged Class A
  `loadAllPages` load — so an incomplete multi-collection load rendered the self-contradictory
  "Showing the first 47 of 47 — use search to narrow." Replaced with a `hint: 'incomplete' | 'none'
  | 'truncated'` discriminator and distinct copy per value; 'incomplete' matches the wording already
  used by the Class A list-load warn Badge.
- F2: mirrored the out-of-list current-selection injection (RerunCollectionsScreen/PlaylistsScreen's
  `selectedInList` prepend) into FillerPresetsScreen and ScheduleItemInspector's rerun-collection
  picker, so an id outside the loaded page still renders as selected instead of misrepresenting the
  stored value as "(none)".
- F3: gated the `console.warn` on an incomplete Class A load with `!signal?.aborted` in the `multi`
  branches (RerunCollectionsScreen, PlaylistsScreen) and SchedulesScreen.loadAllRerunCollections, so
  a superseded/aborted load (Retry, or a type switch mid-load) no longer logs a false warning.
- F4: added `Select`'s `ariaDescribedBy` prop and wired the truncation/incomplete hint span to it via
  `useId()` in RerunCollectionsScreen and PlaylistsScreen, so screen readers announce the hint
  (FillerPresetsScreen already routed it through `Row help=`).
- F5: added FillerPresetsScreen.test.tsx (previously untested) covering the Class B single-request
  guarantee, the truncation hint's totalCount>100/<=100 boundary, and the F2 injection; added the
  two assertions the re-review found missing anywhere in the suite — the Class A `incomplete` warn
  Badge actually rendering, and a screen-level seqRef stale-overwrite race — to
  RerunCollectionsScreen.test.tsx.

Updates docs/spa-conventions.md §3b and the
spa.list-completeness-vs-bounded-pickers decision record to describe the hint discriminator.

Decisions-Edit: yes

timothy commented 2026-07-26 22:09:34 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ daedf00

Three cold review rounds; round-2 verdict sound with a negative control on the type guard. Scope amended on the issue: 3 library/browse pickers stay bounded (#651). 1019 vitest, lint, build green; full CI green.

Review-verdict: MERGEABLE @ daedf00 Three cold review rounds; round-2 verdict sound with a negative control on the type guard. Scope amended on the issue: 3 library/browse pickers stay bounded (#651). 1019 vitest, lint, build green; full CI green.

timothy merged commit c0376dcbce into main 2026-07-26 22:10:04 +02:00

timothy deleted branch fix/644-spa-paging 2026-07-26 22:10:05 +02:00

timothy referenced this issue from a commit 2026-07-26 22:10:06 +02:00

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

timothy referenced this pull request 2026-07-26 22:11:56 +02:00

Seven more SPA list loads silently truncate at the server cap (same class as #634, one screen over) #644

timothy referenced this pull request 2026-07-26 22:11:57 +02:00

Seven more SPA list loads silently truncate at the server cap (same class as #634, one screen over) #644

timothy referenced this pull request 2026-07-26 22:12:00 +02:00

Seven more SPA list loads silently truncate at the server cap (same class as #634, one screen over) #644

timothy referenced this pull request 2026-07-26 22:12:00 +02:00

Seven more SPA list loads silently truncate at the server cap (same class as #634, one screen over) #644

timothy referenced this pull request 2026-07-26 22:12:02 +02:00

Seven more SPA list loads silently truncate at the server cap (same class as #634, one screen over) #644

timothy referenced this pull request 2026-07-26 22:12:03 +02:00

Seven more SPA list loads silently truncate at the server cap (same class as #634, one screen over) #644

timothy referenced this pull request 2026-07-26 22:12:04 +02:00

Seven more SPA list loads silently truncate at the server cap (same class as #634, one screen over) #644

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#656