#616 filed three MCP/API paging traps. Two were real; one was not, and one was
already half-fixed on main. Verified each against the code before changing it.
REAL — pageNum documented as 1-based. `ToolCatalog.Page()` described pageNum as
"1-based page number" while every paged controller defaults it to 0, floors it
with `Math.Max(0, pageNum)`, and skips `PageNum * PageSize`. A caller that
trusted the description started at page 1 and silently lost the first page: no
error, just a short set that reads as data loss rather than an off-by-one (it
cost #487 a verification pass). Fixed in the description rather than by making
the MCP layer 1-based: /api/v1 is additive-only post-freeze, 0-based is
load-bearing in a dozen controllers and the SPA, and a 1-based wrapper over a
0-based API would make the same parameter name mean two different things on two
surfaces a reader reads together.
NOT REAL — "pageSize caps the page but the offset honors the requested value".
Not reproducible on any endpoint. Every controller clamps before passing, every
handler skips by the clamped size, and GetCollectionItemsHandler re-clamps
defensively. The reported observation (pageSize=500&pageNum=2 on a 204-item
collection returning 4 items) is exactly correct 0-based behaviour at the
clamped width of 100 — page 2 is items 201-204. The issue's own trap-1 table
states this. Pinned by test rather than "fixed".
ALREADY FIXED — playout LIST rows gained channelId in #297 (2026-07-22), three
days before #616 was filed; the report was measured against prod, which runs an
older :prod image. The DETAIL response (PlayoutResponseModel) genuinely still
lacked it, so channelId is added there (additive) and the reset_channel_playout
argument now names the trap: the id spaces overlap numerically, so passing a
playout id silently resets a different channel and returns a plausible 202.
Tests, both mutation-verified (each fails when its fix is reverted):
- ToolCatalogTests pins "0-based" on EVERY paged tool's pageNum description,
with a non-empty guard so it can't pass vacuously over an empty tool set.
- GetCollectionItemsHandlerTests pins 0-based page boundaries and proves the
offset derives from the clamped pageSize (page 1 at pageSize=500 returns
items 101-150; the mutation that honors 500 returns an empty page).
Docs: new decision record api.paging-zero-based (catalog regenerated), the
api-conventions paging bullet, and a Paging section in docs/mcp.md. OpenAPI
v1.json + web/src/api/generated/v1.d.ts regenerated for the added field.
fixes#616
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Text-only follow-up; no behavior change (14/14 tests, lint clean, tsc clean).
- AutoTuneScreen.tsx: the addSource comment claimed picking a hit that is
already a base member makes "the existing row read as customised". It does
not — patchSource(id, {}) materializes a DEFAULT draft, sourceCustomized is
false for it, and sourcesRequest omits it, so the pick is a payload no-op
whose only visible effect is the query clearing. Comment now states that.
- spa-conventions.md §11: said WEIGHT_MIN/WEIGHT_MAX are "the same const pair
the multi-collection editor uses". Same VALUES, separate screen-local consts
— there is no shared module. The old wording invited a future reader to
assume a shared seam that does not exist.
Both were nits in the independent review of d3c89d87 (verdict
MERGEABLE-WITH-NITS). Fixed rather than deferred because a comment that states
the opposite of the code, and a doc that implies a nonexistent shared const,
are exactly the kind of thing the next session reads and trusts.
Remaining review nits deferred to a follow-up issue: exporting compile.ts's
Lucene escaper instead of duplicating it, an exclude-all warning, and >50-source
axis handling.
Wires the Auto-Tune DetailPanel's Content-sources pane to #425's backend: each
member row gains a 1..1000 weight stepper and an include/exclude toggle, and a
library typeahead adds a source that isn't in the axis's base set. Edits
accumulate in the screen's per-channel draft (the existing §8/§11 guard covers
them) and are flushed as the create request's `sources` array.
- Only genuinely customised rows are sent, mirroring the server's own
`customized` predicate — an all-default array is a backend no-op, so the field
is omitted entirely and the channel keeps the cheaper fair-share shape.
- Weights are clamped to 1..1000 on blur and again at save, so an out-of-range
value never reaches the server as a raw 400 (spa-conventions §4a).
- The add-untagged picker compiles typed text to `title:*…*` rather than
forwarding raw Lucene: the index's default field does not match bare title
words, so a raw forward would silently find nothing.
- Removes the read-only #425 hint.
Docs: spa-conventions.md §11 records the per-source correction-row convention.
fixes#440
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
Closes#415. Server-derived health object on the channel list + detail DTOs (built-timeline detection, kind-agnostic across all 5 PlayoutScheduleKind; assessable gate keyed to the owning channel's mode), single "Problems" SPA filter with per-fault badges. Supersedes #72's api.channel-health-signal decision.
Co-authored-by: Timothy <timothy.look@gmail.com>
Co-committed-by: Timothy <timothy.look@gmail.com>
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>
Adds GET /api/v1/search/fields/{name}/values?q=&limit= — the backend slice of the
visual rule builder's facet-value typeahead (#434). Enumerates distinct Lucene term
values for a text field via MultiFields.GetTerms + TermsEnum, filtered by a
case-insensitive prefix, limit clamped to [1,50]. 404s when the field is absent from
SearchFieldCatalog or is not type "text". ElasticSearchIndex (the optional external
backend) throws NotSupportedException for this method — its text fields are analyzed,
not keyword-mapped, so a terms aggregation isn't safe to guess at without verifying
against a live cluster.
Regenerated OpenAPI trio (v1.json, v1.d.ts, endpoint-index.md).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Added ChannelId to PlayoutNameViewModel and all 6 construction sites
(Mapper, GetPlayoutByIdHandler, and the Update{,Scripted,ExternalJson,Sequential}
PlayoutHandler commands), plus the list DTO PlayoutListItemResponseModel and the
PlayoutController list projection. Regenerated OpenAPI (v1.json) and the TS client
(v1.d.ts); endpoint-index.md unchanged (no endpoint/operation delta). Simplified
PlayoutsScreen resetSelectedChannel to key directly on selectedSummary.channelId
instead of resolving via channelStates. Updated controller + SPA tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
focus trap added to shared useOverlayBehavior (covers Dialog + SlideOver);
Tab/Shift+Tab now cycle within the panel; test added.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-review of 60a0c505 caught a regression the prior fix introduced: onRetry cleared
the token cache and bumped playToken but kept the old resolvedSrc, so HlsPlayer
reloaded the stale-token URL before the remint resolved — a duplicate manifest
session and a stale 401 that could stick the panel as failed even after the fresh
stream succeeded.
Null resolvedSrc in onRetry before bumping playToken so the player unmounts until the
async effect resolves the freshly-minted URL. Added a controlled-async test proving
the stale-token URL is never reloaded and the retry loads the new token (validated by
negative control: the test fails with the fix removed, and only that test).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cold review (no Critical/High). Folded:
- Low: clamp JWT:BrowserTokenLifetimeMinutes to a 24h max so a seconds-vs-minutes
typo can't mint a multi-year bearer token (non-positive/unparseable still falls
back to 60 min).
- Low: reset the SPA iptv-token cache on the preview panel's Retry and on each
troubleshooting Play, so a stale token (key rotated) or a stale "JWT disabled"
latch (backend reconfigured since page load) can't wedge a user-initiated retry.
Deferred to #559 (tracked): redact access_token from Serilog request logs and set
no-store on token-bearing /iptv manifests — pre-existing properties of the shared
?access_token= transport (Jellyfin/M3U already use it), now bounded by the 60-min
lifetime; cross-cutting fixes beyond this feature's scope.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Under a JWT-enabled deployment (JWT:IssuerSigningKey set), /iptv/* is gated by
ConditionalIptvAuthorizeFilter and the "jwt" scheme does not accept the SPA's
ctv-session cookie, and nothing minted a JWT for the browser. So the #60 channel
preview was declared Unavailable and could not run at all.
Add GET /api/v1/auth/iptv-token (session-gated, on the [IgnoreApi] AuthController):
mints a short-lived global token via JwtHelper.GenerateBrowserToken (60 min default,
JWT:BrowserTokenLifetimeMinutes override), 204 when JWT is disabled. The SPA's new
withIptvToken(url) helper appends it as ?access_token= to the manifest URL (a no-op
when JWT is off), used by the channel-preview panel and the troubleshooting screen.
Mapper.GetPreview drops its iptvJwtEnabled -> Unavailable guard; preview is now
JWT-agnostic.
Live-E2E under JWT: /iptv manifest 401s without a token and passes with a valid one
(garbage token -> 401); token endpoint 401s anonymous, mints with a session.
Honest finding: the issue's point 2 (troubleshooting screen broken under JWT) does
not reproduce -- its live.m3u8 is static-served (UseStaticFiles at /iptv/session),
outside the JWT filter, so it was never gated. The withIptvToken call there is a
harmless defensive no-op.
Docs: security.iptv-browser-token (api-auth-security.md), amended
api.channel-preview-capability, spa-conventions §5b. No OpenAPI change (IgnoreApi +
unchanged Preview schema).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "stops scan polling and refreshes sources once when scans complete" test
fired the poll tick via runPollTick but discarded the promise loadScanStatuses
returns, so the progress-clear state update and the fire-and-forget
loadSources() refetch it triggers landed on real microtasks AFTER act() had
resolved. The test out-waited that race with two waitFor({ timeout: 5000 })
calls, which were marginal on a starved CI VM (timed out on PR #509/run 918);
prior timeout bumps (#447) were diminishing whack-a-mole.
Drive the chain to completion deterministically instead of out-waiting it:
runPollTick now awaits the promise the handler returns (settling the
progress-clear) and then yields to a single macrotask (setTimeout 0) to drain
the microtask queue — including the fire-and-forget loadSources() refetch —
all INSIDE act(async () => ...). The mock fetches resolve synchronously on
microtasks and setInterval is the only mocked timer, so one macrotask turn
completes both chains with no wall-clock delay. The two waitFor calls are
replaced by synchronous assertions (progress label gone; exactly one extra
/api/v1/media-sources fetch), and the 20s test-level budget is dropped.
Verified: LibrariesScreen suite green 15/15 consecutive runs; npm run lint and
npm run build clean. No production code changed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes#512
The reset moved from render-phase into an effect; the existing switch test
asserted only that the error banner cleared, which stays green even if the
effect is deleted. Now the new channel must also reach 'playing'.
Negative control: disabling the reset fails exactly this test, and only it.
Refs #60
- ChannelPreviewPanel: a manual play-button click on a video already
showing a fatal error was clearing the error, silently hiding the
fault the panel exists to reveal. onPlaying now ignores the event
while a fatal error is showing (tracked via a ref, reset in an
effect keyed on channel.id); Retry remains the only way to clear it.
- shell.css: .ctv-preview-facts spacing was dead — equal-specificity
.ctv-detail-infogrid{margin:0} later in the file won. Raised
specificity with a compound selector instead of touching
.ctv-detail-infogrid, which MediaDetailScreen also relies on.
- ChannelPreviewTests: added two cases exercising two simultaneously-
true Unavailable causes, so the documented guard precedence in
Mapper.GetPreview is actually pinned by a test.
- design doc: fixed a garbled sentence describing which DTO gained
the Preview field.
- HlsPlayer: drive onPlaying from the <video> element's own `playing` event on
BOTH the hls.js and Safari-native paths instead of MANIFEST_PARSED, which
fires before any media has decoded (an HttpLiveStreamingDirect manifest
always parses, even over a black video). MANIFEST_PARSED now only kicks
play(). Restore `void video.play().catch(...)` at both call sites and stub
HTMLMediaElement.prototype.play in setupTests.ts instead, so the `?.` that
existed only to survive jsdom is gone from production code.
- HlsPlayer.test.tsx: assert the auto-recovery guard against hls.js's own
startLoad()/recoverMediaError(), not just loadSource's call count.
- ChannelPreviewPanel: reuse existing ctv-* classes (ctv-channels-error,
ctv-settings-warn-callout, ctv-detail-actions, ctv-detail-infogrid) instead
of five undefined ctv-preview-* classes; add the two genuinely new rules
(ctv-preview-video max-width, spacing tweaks) to shell.css.
- Add an exported ChannelPreviewAvailability union (web/src/api/channels.ts)
so a typo like 'ForcedHLSOnly' fails to compile instead of silently
disabling a branch forever; use it in ChannelPreviewPanel's prop type and
at the ChannelsScreen comparison sites.
Wires the ChannelPreviewPanel (Task 4) into ChannelsScreen: a single
panel instance is rendered per screen and its `channel` prop is swapped
via previewChannelId state rather than remounting per row. The Play
button now reads the server-derived channel.preview.availability
(Task 2) instead of being permanently disabled -- Unavailable stays
disabled with the server's unavailableReason surfaced in the title;
Available and ForcedHlsOnly both enable it, since the panel itself
handles the forced-HLS opt-in and caveat.
Also fixes App.test.tsx's #244 channel fixture, which lacked the now-
required preview field and crashed once ChannelTableRow started
reading it.
Two review findings on the channel preview panel (#60):
- ChannelPreviewPanel's synchronous render-phase reset (started/state/error/playToken
on channel.id change) was reachable in prod (the channels screen keeps one panel
mounted and swaps the channel prop) but untested. Added tests proving no auto-start
switching into a ForcedHlsOnly channel, error clearing on switch between Available
channels, and no playToken leak across the switch.
- PlaybackState included 'playing' but nothing ever set it. Added HlsPlayer onPlaying,
fired from Hls.Events.MANIFEST_PARSED and the native-HLS <video> 'playing' event,
mirroring onError's optional/stable-callback contract; ChannelPreviewPanel now wires
it to reach 'playing'. Also fixed a latent bug hit while exercising this path:
video.play().catch(...) assumed a Promise, but jsdom's play() returns undefined.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Renders an in-browser HLS preview of a channel (operator diagnostic).
Deviates from the plan brief per updated requirements: an explicit
user-initiated Retry control replaces the hardcoded playToken, the
forced-HLS caveat renders both before and after opting in, and the
caveat string is exported verbatim as FORCED_HLS_CAVEAT.
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>
Second review returned MERGEABLE with one Medium and three Lows. Addressed all four:
- Medium: the save-time normalization had zero test coverage, so a later refactor
dropping Math.Max would leave the suite green (the FFmpegState floor keeps the
pipeline correct, hiding the regression until someone reads a stored 0 back).
Added Create/Update_Should_Floor_QsvExtraHardwareFrames over 0, -8, 63, 64 and 128,
plus Create_Should_Leave_Null_QsvExtraHardwareFrames_Null for the null-passthrough
branch, following the existing QsvPreferNativeDecoder tests' seed/handle/re-read
shape. Negative-controlled: reverting both handlers fails exactly 5.
- Low: the SPA `min` was cosmetic. Input does forward it to the DOM, but there is no
<form> — save is an onClick gated only on validate(), which had no branch for this
field, so a typed 10 submitted fine and was silently changed to 64 with a 200 and no
message. validate() now rejects it client-side.
- Low: the warning fires at the top of SetAccelState, before we know whether the
pipeline uploads at all, so a fully-hardware path could be told "using 64 instead"
when nothing consumed either value. Reworded to "will use ... wherever frames are
uploaded".
- Low: recorded in the decision entry that the save-time normalization is
unconditional on hardwareAcceleration (a non-QSV profile's stored value moves too),
and that a client PUTting 0 reads back 64 — a transform the OpenAPI description does
not advertise.
Verified in production, not just asserted. Set prod's profile to 64 (operator-approved)
and drove the exposed pipeline myself via the troubleshooting playback API on an mpeg4
.avi, which forces software decode + hwupload:
hwupload=extra_hw_frames=64,vpp_qsv=w=1875:h=1080 exit 0, speed 12.0x, 0 ENOMEM
Then the negative control on prod's own hardware, same command, only the pool differing:
extra_hw_frames=64 -> exit 0, 8 segments, 0 ENOMEM
extra_hw_frames=0 -> exit 244, 0 segments, 3 ENOMEM
which reproduces the six overnight production failures and confirms the fix.
Full suite green: 4095 .NET, 891 web.
Refs #350, #516, #519.
Cold adversarial review returned BLOCKED on the documentation half. Addressed:
- The new decision record carried no lifecycle metadata block, taking the repo from
82/82 to 83/82 and making it invisible to the by-key catalog lookup that #521
established the same day. Added key/status/since/supersedes/superseded-by
(ffmpeg.qsv-extra-hw-frames-floor) and regenerated docs/decisions/README.md;
decisions_validate.py now reports OK with no legacy-unmigrated notice.
- The entry claimed to correct the #350 record but left that record untouched, so the
stale "the burst is bounded" claim stayed authoritative for anyone resolving
ffmpeg.hls-cold-start-burst. Added a forward-pointing correction note there (hence
the [decisions-edit] token on this commit).
- The floor was applied silently. QsvPipelineBuilder.SetAccelState now logs a warning
naming both the configured and applied value, because raising a deliberately small
pool costs additional surfaces (64 NV12 1080p surfaces is roughly 190 MiB, 760 MiB
at 4K) on memory-constrained iGPUs.
- Narrowed an overstated claim in the entry: 1..63 are untested, not known-bad. We
raise them because the risk is a channel serving nothing, not because asking for
less is illegitimate. Recorded as a deliberate over-reach with a stated cost.
- Corrected a factual error: SubtitleScaleQsvFilter also formats extra_hw_frames but
is dead code with no construction site, so it is NOT covered by the guard.
- Config-vs-behavior mismatch: Create/UpdateFFmpegProfileHandler now normalize on
save so stored rows converge on what the pipeline runs, and the SPA field carries
min=64 rather than defaulting the display to 0. Render-time flooring is kept as the
net that fixes existing deployments with no migration; the remaining gap for
un-resaved rows is recorded as an accepted residual.
- Tests strengthened: pinned to the literal measured 64 rather than to the constant
(so lowering the floor cannot quietly satisfy them), added a negative-value case,
added a deinterlace-upload case, and replaced the narrow ShouldNotContain with a
regex asserting EVERY extra_hw_frames occurrence in the command is >= the minimum.
Negative control re-run against the strengthened tests: reverting the floor fails 5,
with the build verified succeeded first. Full suite green (4086 .NET, 891 web).
Review finding that needed no change: the "single point" claim was independently
verified — no bypass exists, every FFmpegState construction routes through
MaybeQsvExtraHardwareFrames.
Refs #350, #516, #519.
- Add ChannelEditScreen tests exercising the geometry-fetch/preview path (previously
untested because the fixture's blank logo path kept BugPreview from ever rendering):
asserts the fetched watermark geometry lands on the preview image with asymmetric
values, and that an external logo URL suppresses the preview.
- Fix a toggle-off/toggle-on data loss: re-enabling "use logo as on-screen bug" used to
always rebind to the default 'Channel Bug' preset, discarding a channel's own
per-geometry ChannelLogo preset. Remember the last referenced logo-driven watermark id
in a ref and prefer it, falling back to the shared default.
- Strip the geometry cache's `id` key before it reaches BugPreview's props (was leaking
via a spread).
- Drop the "(see #502)" issue reference from the visible help text; keep it in the code
comment.
- Constrain findLogoBugWatermark's imageSource param to the generated
ChannelWatermarkImageSource union instead of `string`, and add unit tests for its
preference/fallback/none-found behavior.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
timothyandClaude Opus 5