Re-review of a04d9f0b came back BLOCKED: the F1 merge fixed a visible data loss and
introduced a silent one. Four blockers, all web-only.
BLOCKER 1: `selectedId` and `selectedName` were coalesced independently, but they are one
value. Against a Song response (id resolves, name does not), a user selecting a different
song while the detail GET was in flight got the NEW name paired with the STORED id — chip
read "New Song", Save wrote 42, no error and no visual cue. Strictly worse than the
original defect, which at least cleared the field visibly. The same merge resurrected a
deliberate clear and reverted a type switch.
Replaced with two rules. The id/name pair resolves atomically (no id in the response ->
keep ours whole; same id -> its name may fill ours in; different id -> take theirs whole).
And hydration loses every race against the user: a `touchedRef`, fed by a single `edit()`
funnel so "touched" cannot drift from "changed", limits the refresh to fields the user has
not edited; an explicit conflict-reload clears it, since discarding local edits is its
entire purpose. Three interleaving tests hold the detail response open, act as the user,
then release it.
BLOCKER 2: Enter could commit a result from the previous query — highlight Alpha for "Al",
retype "Be", press Enter before the debounce. The highlight now drops on input change
rather than when the next response happens to arrive, and every keyboard action is gated
on the results matching what is typed. The stale list stays visible (hiding it flickers on
each keystroke), it just stops being actionable.
BLOCKER 3: Escape closed the popup while focus stayed in the input, where `onFocus` can
never re-arm it — the picker was dead until the user blurred and refocused. Typing and
ArrowDown now both reopen it.
BLOCKER 4: the LCG boundary test recomputed the divisor instead of exercising `lcg`, so
all three tests passed with the old `/ 0xffffffff`. Since the recurrence is a bijection
mod 2^32, the seed whose first step lands on 0xffffffff is solvable exactly (653637408);
the tests now drive the real generator into that state.
Also: the rerun #id-degrade and re-save tests did not await the refresh, so they were
satisfiable from the initial draft; they now await it and re-read live textContent. The
Episode/MusicVideo cases are relabelled as error-path guards — a 500 never reaches the
hydrate. FillerPresets' render-time id comparison is dropped as unreachable (every writer
sets label and id together, and the one async writer refuses a mismatched id); an
unreachable guard is an untested one.
#671 is referenced from spa-conventions §3b as the server-side root cause the client guard
defends against.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cold cross-family review of 57aefcdf. Six findings, all web-only.
F1 (HIGH, data-loss shaped): RerunCollectionsController.ProjectToResponseModel derives
BOTH selectedId and selectedName from the same eager-loaded navigation, and
GetRerunCollectionByIdHandler loads media metadata only for Show/Season/Artist/Movie
while MediaCollections/Mapper maps RemoteStream through `_ => null`. So opening a
RemoteStream rerun collection returned HTTP 200 with a null selection and the edit-load
refresh CLEARED a stored id, leaving Save permanently disabled. The refresh now merges
instead of replacing, so no path can clear an id it merely failed to name; the label
degrades to `#id`. Covered per affected type — RemoteStream, Episode, MusicVideo, Song,
OtherVideo, Image — plus a re-save assertion. The read-model gaps themselves are
server-side and are NOT touched here.
F2: `&` and `|` were missing from the escaped set, so `Rock && Roll` compiled with the
boolean operator live. Pre-existing in Auto-Tune's original helper, but propagated to
three more pickers — and now fixed for Auto-Tune too, since the helper is shared. The
test that claimed to cover "every Lucene special" carried its own hand-copied sample and
could not see its own omissions; it is now driven per-character off an exported
LIBRARY_PICKER_LUCENE_SPECIALS.
F3: a slow edit-load name resolution could relabel a newer selection. The label is now
keyed to the id it was resolved for AND refuses to overwrite a label naming a different
id — keying the render alone stops the mislabelling but discards the correct new label.
F4: searchLibraryPickerOptions clamps pageSize instead of merely defaulting it. A bound a
caller can exceed is not a bound.
F6: replacing a native <select> with an input+listbox dropped keyboard operability. Full
ARIA combobox pattern added — role/aria-expanded/aria-controls/aria-autocomplete,
Arrow/Home/End over aria-activedescendant, Enter to commit, Escape to dismiss, options as
non-tab-stops, cursor reset on each new result set.
F7: both is-mounted tests were unsound. React 19 no longer warns on setState-after-unmount
and an unmounted tree renders nothing either way, so the DOM assertion could not fail; the
hook re-arm test used rerender rather than an effect cleanup. Now: a hook-module mock
proving SearchPicker actually reads the guard and sees false, and a StrictMode
double-invoke for the re-arm. Both verified by removing the mechanism and watching them
fail. Same for the LCG divisor, which now has a direct boundary test.
F5 (FillerPresets collection-family names) is filed as #670, not fixed here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three `getLibraryBrowseItems` pickers (RerunCollectionsScreen, PlaylistsScreen,
FillerPresetsScreen) populated a native <select> from a 100-row window over media-library
tables that can hold tens of thousands of rows. #644 made that truncation visible; it did
not make the picker usable, and paging to completeness would have been worse than the bug
(~200 serial requests, each more expensive than the last).
They now resolve by SEARCH through the shared `SearchPicker` over a new
`searchLibraryPickerOptions` helper: zero requests on mount or on a type switch, at most ONE
bounded request (25 rows) per settled query, nothing below 2 characters. Typed text is
compiled via the now-shared `titleContainsQuery` (`title:*<escaped>*`) rather than forwarded
raw, since the index's default field does not match bare title words. The current selection
renders from the owning record — `selectedName` for rerun collections and playlist items, and
for filler presets (which store only an id) a single by-id detail read — so editing an
existing record can never lose or fail to name its selection.
Class A stays put: bounded-by-construction admin lists still page to completeness via
`loadAllPages`, and the collection-family filler-preset types keep their bounded single page
(their `query` is a SQL LIKE, which a compiled Lucene query would not match). No server-side
cap is raised; this is a web-only change.
Folded in from #578: the rule-builder facet typeahead arms on focus rather than on mount (an
N-rule tree fired N unrequested lookups), both typeaheads pair their `seqRef` guard with a
shared `useIsMountedRef`, and the roundtrip test's LCG divides by 2^32 so `pick()` can no
longer index one past the end.
Decision record `spa.list-completeness-vs-bounded-pickers` is archived as superseded by the
new `spa.library-pickers-resolve-by-search`; spa-conventions §3b rewritten to match.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
npm run typecheck (tsc -b --pretty false, the real gate — npx tsc --noEmit
was silently checking nothing meaningful due to the root tsconfig.json's
empty `files: []`) flagged 3 errors: `let x: (() => void) | null = null`
gate-release helpers, reassigned only inside a Promise executor, narrowed a
later `x?.()` call to `never` under tsc -b's project-mode control-flow
analysis. Not reproducible under a bare `tsc --noEmit` invocation.
Fixed by adopting the no-op-initializer pattern already established
elsewhere in this repo (api/libraries.test.ts's `releasePost`): declare as
`let x = () => {};` instead of `(() => void) | null = null`, dropping the
now-unnecessary optional chain at each call site. Same runtime behavior,
no `as any`/`@ts-expect-error`, no change to what any test asserts.
3 call sites fixed: releaseStrandedAppend (F3 test), and releaseB0 in both
the inverse-settlement-order test and the round-4 hook-level single-flight
test.
Verified: `npm run typecheck` no longer reports these 3; `npm run lint`
clean; `npx vitest run` 110 files / 1078 tests still pass.
3 more typecheck errors remain (node:fs/node:path/node:url unresolvable in
src/api/pageSizeCallSites.guard.test.ts, the only file under src that needs
real Node fs access) — deliberately NOT fixed here. @types/node is already
a devDependency and physically installed, but tsconfig.app.json (covering
all of src) has no "node" in its `types` array, and a file-local
`/// <reference types="node" />` was tried and reverted: under tsc -b's
single-program compilation, it leaked Node's ambient `setTimeout` (returning
NodeJS.Timeout) into the whole app project, breaking 3 unrelated
window.setTimeout mocks expecting the DOM signature (confirmed by trying
it — TS2345 in ChannelsScreen.test.tsx/LibrariesScreen.test.tsx/
PlayoutsScreen.test.tsx). The clean fix is a real project-config change
(either widen tsconfig.app.json's types, accepting Node globals become
type-visible in browser app code too, or move this one file into
tsconfig.node.json's project alongside the e2e specs, which would also need
a "references" wire-up for its cross-project import of pageSizeScan.ts) —
left for the coordinator to choose per their explicit instruction not to
make this call unilaterally.
Fourth cold cross-family review: no runtime correctness finding this round
(single-flight held against synchronous throws, generation overlap,
StrictMode, unmount, and separate hook instances). Two test-gap findings
remained, both the same shape as prior rounds' review feedback: asserting on
something ADJACENT to the mechanism rather than the mechanism itself.
1. ChannelBuilder.test.tsx's page-0 single-flight test asserted on the
disabled BUTTON — during a page-0 refresh, `loadingMore` disables the
native button, so `fireEvent.click` never reaches `loadMore` at all.
Deleting `busyRef.current = true` at libraryBrowse.ts's generation-change
effect would leave that test green while direct hook calls could still
overlap page 0 and page 1. Added a hook-level test that calls
`result.current.loadMore()` directly (via `renderHook`, bypassing the
button/DOM layer) while a gated page-0 fetch is in flight, and asserts no
page-1 request is ever issued. Verified: removing `busyRef.current = true`
at that line makes the new test fail with
`expected [ +0, 1 ] to deeply equal [ +0 ]` (a page-1 request was issued
when the guard should have ignored the direct call); restored, green
again.
2. pageSizeScan.test.ts's `.mts`/`.cts` tests exercised the SCANNER'S
PARSING, not the guard's file-discovery glob — reverting the glob back to
`.ts`/`.tsx` left both those tests green (the scanner still parses a
`.mts`/`.cts` filename as plain TS regardless of extension) AND the
real-source guard green (this repo has no committed `.mts`/`.cts` file
for the reverted glob to miss). A prior verification planted a real
`.mts` file and watched the guard notice it, which proved the behavior
exists today but pinned nothing going forward. Extracted the inline glob
condition into a standalone, exported `isScannableSourceFileName`
predicate and added 14 parametrized cases asserting it BY FILENAME (no
filesystem involved) — `.ts`/`.tsx`/`.mts`/`.cts` accepted, their
`.test.*` and `.guard.test.ts` variants excluded, non-TS extensions
rejected. Verified: reverting the predicate's regex back to `.ts`/`.tsx`
only makes 2 of the 14 cases fail (`isScannableSourceFileName('*.mts')`
and `'*.cts'` both wrongly return `false`); restored, green again.
Nothing else changed — the reviewer confirmed no further findings (loader
async/throw handling, generation sequencing, the unconditional rollback,
StrictMode/unmount behavior, the disabled-during-refresh UX, type-only node
exclusion, computed-name exactness, wrapped-forwarded-call rejection, and
all existing test labelling were all confirmed correct as-is).
Full local gate: lint clean, tsc clean, full vitest run 110 files / 1078
tests passed, re-run twice for stability (no flakes).
Third cold cross-family review (BLOCKED) found the append/page-0-refresh
races were being fixed one interleaving at a time — round 1 fixed
page-0-settles-first, round 2's compare-and-set rollback fixed the
duplicate-append case but introduced a permanently-skipped page, and the
reviewer found the exact mirror of round 1's fix (page-1-settles-first,
erasing page 1 with no cursor reset). Direction from the review: stop
enumerating orderings, make the overlap structurally impossible.
SINGLE-FLIGHT (web/src/builder/libraryBrowse.ts): a new `busyRef` guard is
true from the moment ANY fetch (a page-0 refresh OR an append) for the
current query generation is issued until it settles. `loadMore` checks it
SYNCHRONOUSLY and returns immediately (ignored, not queued) if a fetch is
already in flight — including a page-0 refresh, not just a prior append, so
a "Load more" click that lands while a query change is still resolving is a
no-op rather than starting a second, overlapping request. With overlapping
fetches eliminated by construction, the append-failure rollback no longer
needs the round-2 compare-and-set: single-flight guarantees nothing else
could have moved `pageRef` since a given fetch started, so it now always
rolls back and retries the exact page that failed, unconditionally.
Visual feedback (the button showing loading/disabled during a page-0
refresh, not just an append) is set via `queueMicrotask(() => setLoadingMore
(true))` rather than a bare synchronous call in the generation-change effect
— `react-hooks/set-state-in-effect` flags the latter; a microtask-deferred
call resolves before any human-perceptible input, satisfies the lint rule
(the same reason `.then()` callbacks elsewhwere in this hook aren't flagged),
and keeps the actual correctness guarantee (the ref check) perfectly
synchronous regardless.
TESTS REWRITTEN, not just added — the round-2 "HIGH-2" hook test explicitly
asserted the NEXT request after a failed page 1 (following an overlapping
page 2 success) should be page 3, i.e. it blessed page 1's permanent loss.
Replaced with two hook-level tests: single-flight ignores a synchronous
double `loadMore()` call (only one fetch issued), and a failed page is
retried as the SAME page number. Replaced the round-2 component-level
"HIGH-1" test (which drove the now-impossible overlap through the DOM) with
one asserting the click during a pending page-0 refresh is ignored, and that
once free, the correct page-1-then-page-2 sequence completes with both
pages' rows present. Verified all three new/rewritten tests against the
prior committed hook (7b1ae48b0): the two single-flight-specific tests fail
as expected (`[0, 1]` requested when only `[0]` should have been); the
retry-semantics test happens to pass against 7b1ae48b0 too (compare-and-set
and unconditional rollback coincide in the non-overlapping case) but is kept
because it is the correct "retry as page 1, not page 3" pin the review asked
for, replacing the one that asserted the wrong thing.
SCANNER (pageSizeScan.ts) — closed three documented false-negative classes:
- Transparent TS wrappers around the initializer (`pageSize: 100 as const`,
`100 satisfies number`, parenthesized) are now unwrapped before the
NumericLiteral/Identifier check.
- Non-Identifier property names: a quoted string key (`'pageSize': 100`) or
a statically-resolvable computed key (`['pageSize']: 100`) are now
accepted; a computed key that isn't a literal correctly stays unresolved.
- `.mts`/`.cts` are no longer silently excluded from the guard's file
discovery glob (tsconfig.app.json's `include` covers all of `src`; no such
files exist in the repo today, but the glob shouldn't hide one if it ever
does).
10 new fixture tests in pageSizeScan.test.ts pin each case (plus a rejection
test confirming a forwarded call wrapped in `as` still doesn't match, and
one confirming an unresolvable computed key stays unmatched).
TEST LABELLING: relabeled the URL/M-3 and `??`/M-4 fixtures as CONTRACT
fixtures rather than regression pins — a round-3 review found round 1's
plain literal regex already handled those two exact inputs correctly on its
own; only the combined multi-case fixture (and the string-contains-text,
template-interpolation, same-line-identity, JSX, and destructuring fixtures)
actually fail against round 1. Labeled the guard test's 4 tests as BASELINE
assertions (they all pass on clean b90f8a3b) rather than implying they prove
this round's specific fixes — pageSizeScan.test.ts's fixtures are what
actually regression-pin the scanner.
No server-side/C# change. Full local gate: lint clean, tsc clean, full
vitest run 110 files / 1063 tests passed (re-run twice, stable).
Second cold cross-family (Codex, BLOCKED) re-review of b90f8a3b found the
regex/bracket-tracking guard scanner still defeated in five ways, and two new
High-severity races introduced by the F3/F4 fixes. Addressed as a further
follow-up (b90f8a3b left untouched).
GUARD REWRITE (per the review's explicit direction — stop patching the regex,
use the compiler):
- New `web/src/api/pageSizeScan.ts`: `scanPageSizeSites` parses each file with
`ts.createSourceFile` and walks the real AST for `pageSize`
PropertyAssignment/ShorthandPropertyAssignment nodes inside an
ObjectLiteralExpression. This eliminates categorically (not case-by-case):
- M-3: comments and string/template CONTENTS are never revisited as code,
so a `'https://...'` string can't be misread as an unterminated string
that swallows the rest of the file.
- M-4: an object literal nested in a ternary, `??`, or JSX expression
container is still found — the walk visits every descendant node
regardless of the syntactic context above the ObjectLiteralExpression.
- M-5: template-literal interpolations are real AST children, not opaque
text.
- L-7: a type literal (`type P = { pageSize: 100 }`), an interface
PropertySignature, and a destructuring ObjectBindingPattern (parameter
or nested) are structurally different node kinds from
ObjectLiteralExpression — excluded by kind, not by a
preceding-character heuristic a stray `{`/`(`/`,` could fool.
`getLineAndCharacterOfPosition` gives exact line+column (fixes M-6 identity
granularity) instead of the prior line-only identity.
- `pageSizeCallSites.guard.test.ts` now imports the shared scanner; identity
is `file:line:column:kind:value`, compared as a MULTISET (count, not
membership) in both directions.
- Both directions (unregistered / stale) are computed and folded into ONE
thrown Error so a failure always shows the complete picture in one run,
addressing the line-churn "second direction never renders" concern.
- New `pageSizeScan.test.ts`: a FIXTURE test (inline source strings, no repo
scan) pinning the exact discovered set for every case the review named —
comment-in-string, string containing the literal text `pageSize: 100`,
template interpolation, ternary, `??`, JSX container, same-line duplicates,
parameter/nested destructuring, a type literal, an interface property, a
forwarded call expression, a React dependency array. This is what actually
protects the scanner going forward — the guard test alone only ever proved
today's snapshot of real call sites, never the scanner's handling of input
classes it hadn't happened to encounter yet.
- Re-verified both original plants (a duplicate at-cap call in an
already-registered file, and a new file with both a literal and a
shorthand site) against the rewritten scanner; both still fail with the
new combined-direction message. Also verified a run with BOTH directions
simultaneously non-empty renders both in one report.
HIGH-1 (ChannelBuilder.tsx useLibraryBrowse, now web/src/builder/libraryBrowse.ts):
`reqId` identifies a query GENERATION, not an individual fetch — a page-0
refresh and a "Load more" append can be outstanding simultaneously under the
same reqId (query changes while an append is in flight for the new
generation). Whichever settled first used to clear `loadingMore`, letting a
second click fire an out-of-order/duplicate page fetch. Fixed with a
per-fetch `fetchId` plus a `loadingFetchIdRef`/`loadingFetchReqIdRef` pair:
only the fetch that OWNS the currently-displayed spinner can clear it; a
same-generation page-0 refresh leaves a same-generation append's spinner
alone, while a page-0 refresh for a NEW generation still retires an
abandoned OLDER-generation append's spinner (preserving the original #650 F3
fix). Reproduced the exact interleaving from the review in a new test
(gate B's page-0 and page-1 fetches independently, click "Load more" while
B's page-0 is still in flight) and confirmed it fails without the fix
(button re-enables while the append is still pending).
HIGH-2 (same file): the append-failure rollback mutated whatever
`pageRef.current` currently held, rather than the specific page THIS fetch
requested — under an overlapping-append race, a later page's success
followed by an earlier page's failure could roll the cursor back past
already-appended progress, corrupting a retry into refetching a duplicate.
Fixed with a compare-and-set guard (`if (pageRef.current === pageNum)`) so
the rollback only fires when nothing has advanced the cursor since. Since
this overlap is UI-unreachable once HIGH-1's single-flight disabling is
wired up (verified empirically: two synchronous fireEvent.click calls in RTL
only produce one request, since act() flushes the disabling render between
them), the regression test drives `useLibraryBrowse` directly via
`renderHook` (now exported) to force the exact interleaving and confirms it
fails without the fix (page 2 gets duplicated, page 3 never requested).
Extracted `useLibraryBrowse` (plus `loadCollections`/`loadLibraryItems`/
`BrowseState`/the media-type const arrays) into a new non-JSX module
`web/src/builder/libraryBrowse.ts` — exporting a hook from a .tsx file
tripped `react-refresh/only-export-components`; this also makes the hook
importable by `renderHook` without pulling in the whole screen component.
No server-side/C# change. Full local gate: lint clean, tsc clean, full
vitest run 110 files / 1051 tests passed (one LibrariesScreen.test.tsx
flake reproduced under full-suite parallel load, confirmed pre-existing and
unrelated — passes in isolation, never touched that file).
Cold cross-family (Codex) review of 9763fdca found real defects; addressed as a
follow-up rather than amending that commit.
MUST FIX, addressed:
- F5: pageSizeCallSites.guard.test.ts collapsed call-site identity to
`file:value`, so a SECOND at-cap call in an already-registered file was
invisible (verified: appending a duplicate `getLibraryBrowseItems({
mediaType: 'Movie', pageSize: 100 })` to ChannelBuilder.tsx passed all 4
guard tests before this fix). Identity is now `file:line:kind:value` — a
bracket/quote-tracked scan resolves each occurrence's exact line, so a
duplicate on a new line is a new, unregistered identity.
- F6: the guard now also detects the ES6 shorthand property form (`{ ...,
pageSize }`), not just `pageSize: <value>`. Implemented as a bracket-stack
scan that distinguishes an object-literal `{` (real risk) from a
block-statement `{` or an array `[` (false positives from things like
`useCallback` dependency arrays and `const pageSize = 100;` inside a
function body) by inspecting the token immediately preceding each `{`.
Six real shorthand sites are now registered (the two inside loadAllPages
itself, ChannelBuilder's two per-kind fan-outs, and two genuine
user-adjustable pagers in LogsScreen/BlockPlayoutTroubleshootingScreen).
Object SPREAD and positional-argument pageSize (api/search.ts's
api.search-allitems-paging precedent) remain a documented residual gap,
written down in the test file's own header comment, not silently absent.
- F2 (TraktListsScreen.tsx): an incomplete load with zero accumulated rows
rendered BOTH "List may be incomplete" and the unsupported "No Trakt lists
yet." claim. The zero-row empty state now branches on `incomplete` first.
- F3 (ChannelBuilder.tsx useLibraryBrowse): changing the query/library while
a "Load more" append was in flight stranded the button in its
loading/disabled state forever (the stale append's own `finally` no longer
matched the current request id, and the superseding fresh fetch never
cleared `loadingMore` either). `finally` now clears `loadingMore` whenever
the settling request is still the CURRENT one, regardless of whether that
particular request was an append.
- F4 (ChannelBuilder.tsx useLibraryBrowse): one rejected per-kind request in
an append's `Promise.all` wiped every already-loaded row via
`items: []` with no way back. Append failures now preserve state, surface
the error inline next to a still-present "Load more" button, and roll the
page cursor back so a retry re-requests the same page instead of skipping
it.
Both new UI fixes are pre-existing defects in the 'library' source that
9763fdca's loadCollections fix newly made reachable from 'collections' too.
Verified all four fixes against negative controls: reverted each in turn and
confirmed its dedicated test fails with the expected message, then restored.
DO NOT FIX (filed as timothy/ersatztv#665 instead, bug+frontend+priority:low):
- F1: loadCollections/loadLibraryItems sort each fetched page independently,
so appended pages are only locally sorted, not globally sorted across the
accumulated list.
- F7: an overclaiming totalCount can leave "Load more" clickable after every
kind is actually exhausted (no auto-loop; a user click is still required
each time).
Not touched (reviewer confirmed correct as-is): Trakt sequence/abort/unmount
handling, the Class-A vs Class-B incomplete-copy distinction, and the
`lists.length` footer count.
Two SPA list loads requested EXACTLY the server's pageSize cap (100), truncating
identically to #634/#644's over-cap defect but invisible to that fix's manual
"pageSize above the cap" grep:
- TraktListsScreen requested pageSize:100 and rendered BOTH the truncated page
AND the real totalCount, so 101 lists showed as "101 lists" over a 100-row
table. Trakt lists are bounded-by-construction (Class A), so this now pages
to completeness via the shared loadAllPages helper, surfaces an "incomplete"
badge if a page ever comes back short of totalCount, and passes an
AbortSignal from the effect cleanup.
- ChannelBuilder's loadCollections (fanning out per collection kind) reported
the truncated merged.length as totalCount, so canLoadMore's
`items.length < totalCount` comparison was permanently false and "load more"
could never fire. It now sums the real per-kind totalCount, mirroring the
existing loadLibraryItems pattern in the same file, and canLoadMore is no
longer gated to the 'library' source only.
Also found and fixed a third at-cap site not named in #650: ChannelBuilder's
SeasonsDialog (TelevisionSeason browse scoped to one show) reads pageSize:100
but never read the response's totalCount. No real show has 100+ seasons, so
this stays a single bounded page (Class B) rather than paging to completeness,
but now surfaces a "Showing the first N of M seasons" hint instead of silently
truncating if a show somehow exceeds the cap.
Codifies the missing completeness guard as an enumerating allow-list vitest
test (web/src/api/pageSizeCallSites.guard.test.ts): scans every `pageSize:`
call site in the SPA and diffs it against a hand-reviewed registry in both
directions (unregistered site = new defect risk, stale entry = registry rot),
with anti-vacuity floors on files-scanned and sites-discovered. Verified the
guard actually fails on a planted defect and a planted stale entry before
finalizing it.
No server-side change: the client pages, the server stays bounded
(api.search-allitems-paging precedent).
The rule builder's Group nesting was capped at one level (#176's Kodi
model). Generalize it to recursive nesting bounded by a single shared
constant, MAX_GROUP_DEPTH (types.ts, = 5, root group is depth 0):
- parse.ts: replace the allowNested boolean with a depth counter that
recurses to the cap; deeper input stays out of subset (null -> raw-text
fallback), so parse remains the exact inverse of compile. Sub-group
detection now requires the leading '(' to be the one closed by the
trailing ')' (quote/escape aware), so '(a)x(b)' can't be mistaken for
one wrapped group.
- RuleBuilder.tsx: 'Add group' is offered while depth < MAX_GROUP_DEPTH
instead of only at the root; nested group boxes get box-sizing:
border-box so per-level padding can't overflow (no global reset).
- roundtrip.test.ts: the 500-tree generator nests to the cap and asserts
the corpus actually reached it; explicit depth-3 cases added to
compile/parse/validation tests and a depth-gate test to RuleBuilder.
compile.ts and validation.ts already recursed correctly and are unchanged.
No backend/OpenAPI change.
Docs: spa-conventions.md §12; decisions lifecycle — new active record
spa.rulebuilder-nesting, predecessor spa.smartcollection-rule-builder
relocated to docs/decisions/archive/spa.md as superseded.
fixes#436
Adopt the reusable RuleBuilder (#176) for inline query authoring in the Channel
Builder (/app/new-channel). Extract CollectionsScreen's smart-collection dialog
into a shared, self-contained component (SmartCollectionDialog) and consume it in
both screens; the Channel Builder's Collections source gains a "New smart query"
action that persists the authored query as a real SmartCollection and adds it to
the lineup by smartCollectionId. Pure frontend — no REST/MCP surface change (the
MCP already exposes ersatztv_create_smart_collection).
The Auto-Tune half of #437 is a different primitive (group-by, not single-query
filtering) and a backend epic; it is designed separately in
docs/superpowers/specs/2026-07-23-auto-tune-arbitrary-field-design.md and filed as
its own issue rather than wired here.
Verification: web typecheck + lint clean; full vitest suite green (981, incl. a new
inline-smart-query test); cold-context review clean; live-E2E on a real instance
(query authored in the SPA persisted as SmartCollection "Action Picks" and added to
the lineup, 0 console errors).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CreateChannelFromLineupHandler resolved every advanced override with
advanced.X ?? template.X, so null always meant INHERIT and a channel could
not drop a template-set watermark / filler / preferred language. Add an
optional typed `clear` enum list to CreateChannelFromLineupAdvancedOptions:
omitted/null still inherits (byte-stable for existing clients), a field named
in `clear` is forced to none. Set+clear of the same field is a 422.
The enum (CreateChannelFromLineupClearField) lives in ErsatzTV.Core so the
OpenAPI string-enum scan renders it as a string enum, matching every sibling
advanced-options enum. Handler resolves clearable fields once via
ResolveClearable and validates set/clear conflicts via ValidateClear;
reference validation skips existence checks for cleared (null) refs.
SPA: the shared advancedOptions model re-adds a real "None" option to the five
id selects (watermark + fillers) in both the Channel Builder and the Auto-Tune
DetailPanel, routed through a CLEAR overrides sentinel that applyOverridesToRequest
folds into advanced.clear (never leaking onto the wire as a field value). The
backend enum also covers the preferred audio/subtitle language strings for
machine clients; the SPA text inputs keep "empty = inherit" (tri-state deferred).
Docs: api-conventions.md §2, spa-conventions.md §11, decisions.md record
api.from-lineup-clear-to-none; v1.json + generated TS regenerated.
fixes#135
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Task 6: getSearchFields() client fn + SearchField type in api/search.ts,
and the useSearchFields() hook in builder/rules/fieldCatalog.ts that
reshapes the catalog into fieldTypes (name -> FieldType) and byGroup
views for the rule builder (Tasks 7-8).
Generated SearchFieldResponseModel has all-nullable fields (name/label/
type/group/values), unlike the brief's non-null assumption -- adapted by
dropping fields with a null name and defaulting group to 'Other' / type
to 'text' rather than indexing under undefined.
Adds a right-hand "Configure" slide-over to each Auto-Tune Preview row, making
a proposed channel editable before bulk-create — against the shipped #384/#385
backend only, so no control lacks a wire target.
- New reusable SlideOver primitive (components/overlay.tsx), sharing a
useOverlayBehavior hook (focus/scroll-lock/Escape/scrim) with Dialog.
- Extract the Channel Builder's advanced-options model to builder/advancedOptions.tsx
(enum catalogs, ADVANCED_KEYS, effectiveValue, INHERIT/omit useAdvancedOverrides
hook); ChannelBuilder imports it unchanged (its tests pass byte-for-byte). The
DetailPanel writes its own field JSX over the same hook — shared logic, per-screen
layout.
- Panes: identity (name/number + logo upload), Playback (Shuffle/Always-playing →
advanced.playbackOrder/playoutMode), per-channel template picker, Advanced
disclosure, lean read-only Query&size, read-only Content-sources via GET /members.
- getAutoTuneChannelMembers API client (#384 read endpoint) + tests.
- Screen-scoped §8 unsaved-changes guard + "Edited" row badge.
- Dropped as backend-less decoration: MiniEpg, bug-initials generator, query text.
Deferred to #425 with an in-pane hint: per-source weight steppers + corrections.
- Docs: spa-conventions §11 (SlideOver + shared advanced-options), decisions.md.
Refs #386
Version every /api route to /api/v1 (251 controller routes + ~24 Location
headers + the scanner callback URL + the Startup request-log literal),
uniform across the machine API, auth, scanner and scripted-build surfaces.
Add ApiVersionRewriteMiddleware: a legacy unversioned /api/* request is
rewritten (NOT redirected) to /api/v1/* in-pipeline — method, body, auth
headers and query survive — carrying RFC 8594 Deprecation/Sunset headers,
so curl / the future MCP server / bookmarks keep working. An already-
versioned path passes through; a future /api/v2 is never forced to v1.
Standardize the route convention (leading-slash absolute route per method,
no class-[Route] — except the two Scanner/Scripted controllers whose ~all
actions share a parametrized {id} prefix), enforced by ApiRouteVersioningTests
(^/api/v\d+/ over the whole Controllers.Api surface; browser-nav
/auth/oidc/login is out of scope).
Regenerate v1.json (160 paths, all /api/v1)/endpoint-index/v1.d.ts; sweep 945
SPA request literals + the test mocks (regex + positional URL parsers). /api/v1
is additive-only after freeze; the legacy-rewrite shim sunsets in ~2 releases
(owner decision) with removal tracked as a Phase-3 follow-up.
Docs: decisions.md 2026-07-13, api-conventions §1/§9, rest-api/spa-conventions/
blazor-route-parity/e2e-local/domain-model.
fixes#286
refs #197
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regenerate v1.d.ts from the aligned spec (fFmpegProfileId->ffmpegProfileId,
fFmpegProfile->ffmpegProfile) and update every SPA reader/writer and test mock
to the runtime casing:
- ChannelEditScreen: read channel.ffmpegProfileId; draft/set/select use
ffmpegProfileId.
- ChannelBuilder: template reads/writes + ADVANCED_KEYS use ffmpegProfileId.
- App.tsx channel list: read channel.ffmpegProfile (was fFmpegProfile, a latent
bug that always rendered "Unassigned" since the runtime never sent that key).
- PlaybackTroubleshootingScreen: drop the #198 escape hatch (rawChannel cast +
dual-read) and read data.channel.ffmpegProfileId directly.
- Test mocks now use runtime casing; pinning comments updated.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Part of the #180 library picker fixes (SPA side).
Channel builder: TelevisionSeason is removed from the library grid fan-out, so a
multi-season show renders as one tile instead of one tile per season. Show tiles
gain a "Seasons" drill-in affordance (both grid and compact layouts) that opens a
dialog listing that show's seasons (via the new GET /api/library/browse?parentId=
&mediaType=TelevisionSeason), each with title, artwork and an Add button that
drops the specific season into the lineup.
Collections add-items dialog: the default search fan-out now excludes seasons,
and a media-kind filter row (All / Movies / Shows / Seasons / Artists,
default = All-without-seasons) keeps seasons reachable when explicitly selected.
Client: getLibraryBrowseItems gains an optional parentId param. Tests cover the
param mapping, the builder no longer requesting TelevisionSeason, and the
collections dialog default-excluding vs explicitly-including seasons.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GET /api/library/browse without mediaType now spans all 10 media kinds
(was 4: Movie/TelevisionShow/TelevisionSeason/Artist), which flooded two
pickers with episodes/songs/etc. mixed in with the intended top-level
items. Fan both consumers out per-kind and merge, restoring pre-#168
scoping without touching the API:
- ChannelBuilder's library browse tab now fans out Movie/TelevisionShow/
TelevisionSeason/Artist per page (mirrors the existing loadCollections
pattern) and sums per-kind totalCount so paging/load-more still works.
- CollectionsScreen's add-items search now fans out over the same 4
addable kinds instead of filtering one unscoped page client-side.
Extended App.test.tsx and CollectionsScreen.test.tsx to assert the
per-kind requests and updated a few fixtures that relied on the old
unscoped browse call returning collection-kind items under the library
tab.
- MediaBrowseScreen (/app/media): one parameterized screen for all 9 top-level
kinds with an in-screen kind switcher, per-kind search box, 100/page paging
- SearchScreen (/app/search): grouped per-kind results with counts + 'See all';
TopBar search input now navigates here
- TrashScreen (/app/trash): state:FileNotFound results, multi-select + Empty Trash
via DELETE /api/media-items and /api/maintenance/empty_trash (confirm dialogs)
- Shared media/mediaKinds (icon/label maps, hueOf, duration helpers) + MediaPosterCard;
ChannelBuilder and CollectionsScreen now reuse the single-source maps (their
exhaustive Record<LibraryBrowseMediaType> had to cover the 6 new kinds anyway)
- api: search.ts, mediaItems.ts, maintenance.ts (+ URL-assert tests)
- One consolidated 'Browse' nav entry with an in-screen kind switcher instead of
9 per-kind nav rows (deviation from Blazor's per-kind Media links)
- Nav: Media group gains Browse/Search/Trash; Settings Classic-UI help trimmed
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Complete the SPA-surface rebrand: SVG favicon (chicorytv-icon) served
from web/public, description + theme-color meta, sidebar brand mark
switched to the canonical transparent chicory-mark.svg per the design
system lockup, and the Channel Builder default group ErsatzTV->ChicoryTV
(tests updated). Legacy Blazor UI and backend strings intentionally
untouched (product-wide rebrand = epic #59).
closes#90
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Dialog: focus only on open transition (was stealing focus from its own
inputs every parent re-render); Escape listener via latest-ref
- Advanced overrides: honest 'Inherit from template' semantics - the
handler coalesces null with the template value so 'None' could never
clear anything (clear-to-none follow-up: #135); save-as-template now
composes from the same effective-value helper
- toggle rows: label->div (label activation forwarding double-fired in
real browsers); clear stale 422 row highlight on lineup mutations
- port .ctv-lift/.ctv-press motion utilities + apply per prototype;
Advanced expand scroll-into-view; Info glyph on advanced info line
- fixture fidelity: real 'Validation failed' title, verbatim handler
detail message, omit null keys, collectionKind on rerun fixture
- cache successful artwork upload across create retries
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
timothy
