372 Commits

Include renames

Author	SHA1	Message	Date
timothyandClaude Fable 5.1	4a56d54283	fix(911): a should-fix finding sends the round back, a failed-lens round is an error, and a harness executes the loop (#912) ... fixes #911 Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV	2026-09-05 02:43:44 +00:00
timothyandClaude Fable 5.1	692affca97	fix(911): a round with no reviews is an error, the history binds each fix to the round it answered, and a harness executes the loop ... A round in which every lens returned null read as a clean pass and would now have been quoted verbatim into the PR body; it is an error before the push, in the review loop and in the post-rebase round, which also gains the same blocking-or-should-fix filter; a fixer that dies or stops (no done) is an error too, the same test the implementer already gets. The history entry now carries the fix that answered that round and only the commits that fix added (a line-set difference against the previous branch log — a fixer that reformats or rebases mid-loop defeats it, which is why the finisher is told to read git show, not the list). An empty fix-commit set is described as "answered without a new commit" when a fix round ran, and as "round one was clean" only when none did. web/scripts/orchestration-workflow-loop.test.mjs compiles the committed script bodies with stubbed agent/parallel and pins eleven paths per script (22 tests). Measured: reverting the loop condition to blocking-only reddens six cases per script (every case that needs a should-fix round to reach the fixer); deleting any of the three zero-lens guards, the fixer guard or its done half, or the empty-fix sentence branch reddens its own case, in both scripts. web/vite.config.ts is untouched: it is pinned whole by test_image_build_delegates_the_spa_suite.py, comments included. Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV	2026-09-05 02:50:23 +02:00
timothyandClaude Fable 5.1	f119bfcb29	docs(554): the opt-in is a third path back to starting, and the invariant must say so ... The hint's single-guard invariant was stated as "every path back to `starting` clears the flag itself", enumerating Retry and a channel change. `onOptIn` is a third such path and clears nothing, so the sentence was false as written — in the code comment, in the Retry test's comment, and in docs/spa-conventions.md §5b. Adding a clear to `onOptIn` would be dead code no test could distinguish, which is the exact shape this branch removed from `onPlaying`. The omission is correct for a reason none of the three places stated: the opt-in button renders only while `started` is false, `started` only goes false in the render-phase reset that clears the flag two lines later, and no player exists to set the flag while `started` is false. State that exception, and pin the reachability premise it rests on with a test that fails if the opt-in button outlives the mounted player. Refs #554 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV	2026-09-05 02:42:39 +02:00
timothyandClaude Fable 5.1	b6b4fb6619	test(554): pin both clears the hint's single guard depends on ... Dropping the onPlaying clear made the `state === 'starting'` render guard the hint's only guard, which moves the burden onto the two paths back to `starting`: each has to clear `autoplayBlocked` itself. Both were assertable but unasserted — either `setAutoplayBlocked(false)` could be deleted with the whole panel suite green, so the invariant the code comment and docs/spa-conventions.md §5b both state was unpinned in both of its named paths. Add one test per path (Retry; a channel switch), each asserting the hint is gone while the panel is back at `starting` — so the render guard cannot be what hid it. Each also asserts the player really re-mounted (loadSource count / last URL, plus a non-null <video>), so the hint cannot be absent merely because the `resolvedSrc` block is unrendered. Measured on this tree, each mutation caught by exactly one test: deleting the onRetry clear reds only 'clicking Retry clears the autoplay-blocked hint' (Tests 1 failed | 25 passed); deleting the render-phase reset clear reds only 'clears the autoplay-blocked hint when switching to a different channel' (1 failed | 25 passed); replacing `autoplayBlocked && state === 'starting' &&` with `autoplayBlocked &&` reds one test too. Unmutated: 26 passed. Refs #554 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV	2026-09-05 02:08:36 +02:00
timothyandClaude Fable 5.1	4e042fb7de	fix(554): the hint's cause must be measured, and muting is the consumer's call ... Round one measured three defects in the first commit. onAutoplayBlocked fired on ANY rejected video.play(), so the panel could say "autoplay was blocked" when it was not. A play() interrupted by teardown rejects with AbortError — which is exactly what the panel's own Retry produces while the MANIFEST_PARSED play() is still pending — and because the element was muted, a genuine NotAllowedError is the rare case, so the realistic firings were the mislabelled ones. Report only a DOMException named NotAllowedError, on both the MSE and native paths. `muted` was applied to the shared player unconditionally, which silently muted the playback-troubleshooting screen — the tool whose job includes verifying the audio side of an FFmpeg profile, and which the legacy Blazor player never muted. Make it an opt-in `muted` prop defaulting to false; the channel preview passes it, troubleshooting does not, and a test on each side pins its own value. The two clauses hiding the hint once playback starts masked each other: removing either alone left the panel suite green. Every path back to 'starting' (Retry, a channel change) already clears the flag itself, so the clear in onPlaying could never be the load-bearing guard — drop it and let the `state === 'starting'` render guard be the single pinned one. Also cover the native-HLS (Safari) branch, which no test had ever executed: its play() kick, its playing/error wiring, and both autoplay rejection names. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV	2026-09-05 01:46:37 +02:00
timothyandClaude Fable 5.1	02e1c583e9	fix(554): mute the preview player so autoplay is never blocked ... HlsPlayer's manifest GET can block until segments exist (unbounded maxTimeToFirstByteMs), so MANIFEST_PARSED can arrive past the browser's transient user-activation window and video.play() gets rejected as blocked autoplay — the channel preview panel then sat at "starting" over a black frame with no hint the operator just needed to press play. Render the <video> element muted (browsers permit autoplay of muted media without user activation) so the common case starts on its own, and add an optional onAutoplayBlocked callback for the residual case (stricter policy/extension) that the channel preview panel wires to a "press play" hint shown only while still starting. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV	2026-09-05 01:12:39 +02:00
timothyandClaude Fable 5.1	e1d211cd1b	test(830): pin which ARM the call site reaches, not only that the hook has one ... Review measured M6: swapping `reportFailure(...)` for `onAddFailed(...)` in `AddItemsDialog`'s catch left all 490 tests across the 42 `src/screens` files green. Under that mutant a failure that lands while the dialog is STILL OPEN renders into `ManualItemsView`'s screen banner, which sits behind the dialog's `createPortal` panel with `aria-modal="true"` — covered for sighted users, hidden from AT, and the surface the user is actually looking at stays blank. That is the exact shape the decision record calls "its own defect", and the whole gap was the call-site wiring: the hook's inline branch is pinned at unit level in `hooks.test.tsx`, but a unit test of the hook cannot see which arm a consumer reaches. Adds the integration assertion: fail the POST with the dialog still up, assert the message is inside `[role="dialog"]` and appears exactly once in the tree. Re-executed the mutation with it in place — 1 failed / 490 passed, and the red is this test alone. Records the new pin as mechanics (5) on `spa.dismissible-write-failure-reporting` and the general form in `spa-conventions.md` §3c, so the next site wired to the hook pins both arms at its call site rather than inheriting the hook's unit coverage. Refs #830 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV	2026-09-05 00:23:09 +02:00
timothyandClaude Opus 5	e4c346e649	test(830): pin the ORDINARY close too — the negative test did not cover it ... Round 6 verdict was MERGEABLE with two follow-ups; both are one-liners on lines this branch just touched, so they are in rather than deferred. L11. I believed the suite covered "a normal add still closes the dialog". Review MEASURED that it did not: deleting the `onClose()` call entirely -- so a successful add leaves the picker open forever -- kept the whole suite green, 1277/1277. The new #830 test only pins the NEGATIVE direction (do not close when unmounted), so a future edit dropping the call, believing the guard had made it dead, would have shipped silently. The Song add test now asserts the dialog closes; with that line, the same deletion reddens. Both directions of the report/dismiss split are pinned. Worth naming the shape: I asserted coverage from plausibility rather than from a mutation, in the same PR whose whole subject is claims that were written down before they were measured. N12. The guards test's "exactly ONE post-await write to state THIS component owns" is still true, but it now reads as a census of `mountedRef` reads, and `submit` has two -- the success path's guarded `onClose()` is the other, which that failure-path test never reaches. Added the clause so nobody derives the guard population from that number. 1277 tests green, tsc/eslint/build clean, pytest 1228 passed, validator OK, catalog no drift. The two red CI contexts on the previous head are runner flakes, not this branch: both failed inside `Post Checkout` with `Cannot find module '/var/run/act/actions/<hash>/dist/index.js'`, their logs are timestamped 19:18 (before that head existed), this branch touches no CI or docker/ci file, and both contexts were green on its earlier heads. refs #830, #877 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY	2026-09-05 00:23:09 +02:00
timothyandClaude Opus 5	86e9ad41a6	fix(830): gate onClose at the exemplar — the convention contradicted its own reference ... Round 5 found that §3c and the `rule:` field now instruct readers to gate the DISMISS request, while `AddItemsDialog` -- the one site this record names as "the shape fixed here" -- called `onClose()` unguarded, with a comment arguing that was correct. So a reader following the convention wrote the gate and a reader copying the reference implementation did not. The defect is pre-existing; the CONTRADICTION is mine, introduced when round 3 withdrew H1's code but kept the convention it produced. I checked the docs against the withdrawn addTo code and did not re-check them against the exemplar that stayed. Measured at this site: submit, Escape mid-request, reopen the picker to retry, first POST returns 204 -> the stale instance's `onClose()` (`() => setPickerOpen(false)`) closes the dialog the user just reopened, discarding the selection they rebuilt. Identical mechanism to the addTo clobber. Unlike the addTo layer, the one-line gate IS sufficient here, and that difference is the point: `AddItemsDialog`'s parent has no competing closer (`onAdded` is `load`, which never touches `pickerOpen`), whereas `AddToMenu.handleAdded` closes its dialog itself. That is now stated in the record as the concrete reason one half shipped and the other went to #877. - `onAdded()` stays unguarded -- it REPORTS, and the parent's list reload must survive dismissal - `onClose()` is guarded -- it REQUESTS A DISMISSAL, and after dismissal it aims at whatever the user opened next - comment rewritten to say which is which and why, instead of defending both as "belong to the still-mounted PARENT" Pinned, and nothing pinned it before: "a late SUCCESS does not close the dialog the user reopened after dismissing (#830)". It carries an anti-vacuity check that the late response was actually processed -- `onAdded` is `load`, so a second GET of the items endpoint must have happened -- because otherwise "the dialog is still open" holds trivially. Executed: deleting the `if (mountedRef.current)` around `onClose()` reddens it alone. Also rewrapped five record body lines left ragged by earlier splices. 1277 tests green, tsc/eslint/build clean, pytest 1228 passed, validator OK, catalog no drift. refs #830, #877 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY	2026-09-05 00:23:09 +02:00
timothyandClaude Opus 5	4b12125f4d	docs(830): drop a coverage claim this record's own mechanics field contradicts ... Round 4 found one clause left, and it is a good example of the thing this record is about. The one-consumer paragraph said `hooks.test.tsx` "is the ONLY thing pinning the diverted branch". Measured at the previous head, disarming `reportRef.current(message)` reddens THREE tests -- both hooks.test.tsx divert tests AND the CollectionsScreen integration test -- which is exactly what the `mechanics:` field of the same record says 58 lines earlier. So the record asserted a coverage fact and then contradicted itself. The concrete harm is not the inconsistency: a future session pruning tests reads "hooks.test.tsx is the only pin", concludes the CollectionsScreen #830 test is redundant, and deletes the only end-to-end pin of the whole path -- the one that actually drives Escape-dismissal through the real dialog. Clause dropped; the argument the paragraph needed (the hook's shape earns its own unit tests) survives without it. The clause originated in the reviewer's round-3 wording and I transcribed it without checking it against a field I had written myself two rounds earlier. Worth recording: a review finding is not exempt from verification just because it came from the reviewer. Also: - the `AddToMenu` clobber sentence now splits what was MEASURED (a late success closes a reopened dialog) from what was READ (both parents call `clearSelection()` unconditionally, so the wipe follows). On a record whose subject is over-attributing measurements, that distinction has to hold in its own prose. - §3c now carries the same "nothing diverts to those screens today" disclaimer the record's limit (2) has, so the two artifacts say the same thing - rewrapped one 141-char comment line left ragged by the previous round's splice Docs only, plus one comment rewrap. 1276 tests green, tsc/eslint/build/validator/catalog clean. refs #830, #877 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY	2026-09-05 00:23:09 +02:00
timothyandClaude Opus 5	626cbfbf9d	docs(830): three sentences survived the revert of the code they described ... Round 3 verified the withdrawal itself is clean -- the eight reverted files are byte-identical to origin/main, no orphans, and the mutation gives the stated three reds -- but found docs still asserting the withdrawn change shipped. That is the stale-comment failure in its usual form: after a retraction, the retracted WORDING has to be swept, not just the code. - `hooks.ts` said "#830 removed that gate", flatly false at this head, in the hook's own doc comment right above the export -- the first thing a maintainer reads. It also carried round 2's framing ("both halves of the outcome") as the hook's purpose, when what ships carries only the failure half. Rewritten to the present tense of the shipped tree. - The `rule:` field still said the surviving surface "differs per screen", naming MediaBrowseScreen and SearchScreen as wired. They wire nothing. This one matters beyond an ordinary sentence: `rule:` is the canonical summary, it is what the catalog row shows, and it is what gets mirrored per-key into MemPalace -- so it is the version a future session retrieves WITHOUT opening the file. Now: exactly one wired screen, the Toast pair named as a CANDIDATE. - The "two limits" bullet described a failure being diverted to those same screens and announced politely. Nothing can divert there -- they receive no reporting callback. Restated as the limit the second surface will have when it is wired. - `onFailed` in a hooks.ts comment was a dangling identifier; the real prop is `onAddFailed`. Also added the caveat the reviewer asked for rather than leaving it to be discovered: this is a shared hook with exactly ONE consumer. It earns that shape (directly unit-tested, and those tests are the only thing pinning the diverted branch; prescribed by §3c; #877 queued as a second consumer) -- but #877 may land a shared reporting SURFACE instead of a per-site prop, in which case the second consumer never arrives. Accepted risk, now written down. Docs only. No code change, 1276 tests still green, tsc/eslint/validator/catalog clean. refs #830, #877 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY	2026-09-05 00:23:09 +02:00
timothyandClaude Opus 5	de9432ff49	fix(830): withdraw the media/addTo extension — two rounds, two defects, one coupled gate ... Round 2 of adversarial review found that my round-1 fix introduced an adjacent defect, and it is the same mechanism both times: `onAdded?.()` and `onClose()` sat behind ONE unmount gate in the four `media/addTo/` dialogs, and those two callbacks do not mean the same thing. - Gate both (origin/main): a write that SUCCEEDS after dismissal reports nothing. Measured on `AddToCollectionDialog` -- `onAdded` called 0 times after dismissal. That was round 1's finding. - Un-gate both (my round-1 fix): a late success closes a dialog the user REOPENED to retry, and on SearchScreen/MediaBrowseScreen `clearSelection()` wipes a multi-select they rebuilt. Measured against the real `AddToMenu`. That was round 2's finding, and I introduced it. - Gate only `onClose`: still wrong on its own, because `AddToMenu.handleAdded` nulls the dialog itself. Needs three coupled edits across five files -- plus a genuine product question nobody has answered: should `clearSelection()` fire for a write the user walked away from? That is a design change, not a bug fix, and #830 never asked for it -- the issue is about `AddItemsDialog`. Two defects from one mechanism in two rounds is the signal to stop widening, so the `media/addTo/` extension is REVERTED here and moves to #877 with every measurement attached (#877 comment). What ships is the thing the issue asked for, proved: - `useDismissSafeError` + `AddItemsDialog` + `CollectionsScreen` wiring - the witnessed red is unchanged: disarming `reportRef.current(message)` reddens the integration test on `Unable to find an element with the text: Request failed with status 500` Docs now describe what is actually true rather than what I hoped: - the record says ONE A1 site is fixed and explains why the other four were withdrawn, keeping the wrong first claim visible because "one site read, four assumed" is the lesson - §3c splits the rule the round-2 defect came from: report the OUTCOME unguarded, gate the DISMISS request separately -- the earlier text lumped `onClose` in with `onAdded` and would have propagated the clobber to the next screen that adopted it - §5c no longer tells authors to wire an `onFailed` that the addTo layer does not have; it says the layer has no failure channel at all and points at #877 - the reporting prop is REQUIRED where the host has a surface (`AddItemsDialog.onAddFailed`), which is what the docs now say instead of calling it optional - `mechanics:` no longer implies the shared clause reddens one test; it reddens three, so re-running the mutation should expect three refs #830, #877 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY	2026-09-05 00:23:09 +02:00
timothyandClaude Opus 5	f61764d051	fix(830): report SUCCESS past dismissal too — the review measured my claim false ... Adversarial review (cold, worktree-isolated) blocked the first commit on its central prose claim, correctly. I wrote "at every one of these sites SUCCESS already outlives dismissal" into three durable artifacts -- the decision record, spa-conventions §3c and the hooks.ts header -- after reading ONE site. `AddItemsDialog` does report success past dismissal and says so in a comment; I generalised from it. The review probed the other four instead and MEASURED `onAdded` called 0 times after dismissal: all four `media/addTo/` dialogs gated `onAdded?.()`/`onSaved?.()` behind their own `activeRef`, exactly like the failure path. So after the first commit those four were still asymmetric, just inverted: dismiss-then-fail loud, dismiss-then-succeed silent -- and additionally leaving the caller's selection state stale, because SearchScreen's `onAddedToSelectionTarget` never ran to clear it. The record's own advice ("add the failure counterpart") followed literally would have reproduced it. Fixes, each proved by execution: - the `activeRef` gate above `onAdded?.()`/`onSaved?.()` is removed in all four dialogs; those two statements belong to the still-mounted PARENT, which is the reasoning AddItemsDialog already had - `AddToCollectionDialog.test.tsx` covers the media/addTo half in BOTH directions. It had NO coverage before: reverting `reportFailure` to `setInlineError` in all four left the whole suite green. Restoring the success gate reddens the SUCCESS test alone; disarming `reportRef.current(message)` reddens the FAILURE test alone - the three prose sites now say what was measured, and the record keeps the wrong first version visible, because "one site read, four assumed, written down before measuring" is the finding Also from the review: - hooks.ts said "React 18"; package.json pins 19.2.7. Now "React 18+" - the "nothing better to do" comment overclaimed: diversion reaches ONE level, so Back out of a collection mid-add still drops the message. Stated, with where it would be fixed - recorded two limits rather than leaving them to be rediscovered: useIsMountedRef clears in a PASSIVE effect cleanup, leaving a narrow window where the message renders inline into a detached tree (useLayoutEffect would close it, but that hook is shared by every async caller -- #877, not a bug fix); and Toast is role="status" with one last-writer-wins slot, so it is not equivalent to CollectionsScreen's role="alert" - §5c now cross-links §3c, since that is the section a screen author reads before wiring AddToMenu - the sweep count is 67 caller-owned + ConfirmDialog's own internal <Dialog> Three other media/addTo dialogs remain unpinned; they are identical in shape to the covered one, which is a reason to expect the same behaviour, not evidence of it. Said so in the record. refs #830, #877 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY	2026-09-05 00:23:08 +02:00
timothyandClaude Opus 5	9ba397e1dd	fix(830): a write failure reports to a surface that outlives the dialog that started it ... `AddItemsDialog.submit` POSTed to `/api/v1/collections/{id}/items` and reported failure into a banner rendered from its OWN state. The dialog is dismissible mid-request through three paths that never consult `adding` -- Escape and a backdrop click (both `useOverlayBehavior`) and the header close button -- and the caller remounts it on `key={`add-${pickerOpen}`}`, so dismissal genuinely unmounts it. Select 12 items, Add, press Escape, the request fails: nothing surfaces, the list reloads unchanged, and the user believes 12 items were added. drop deliberate rather than accidental. A deliberate drop is still a user who is told nothing. The asymmetry is the finding: SUCCESS already outlived dismissal everywhere here, because it is reported through a parent callback (`onAdded`/`onDone`, which the screens turn into a `Toast`). Only failure died with the surface. So this is not a new notification system -- it routes failure through the channel success already uses. `AddToMenu` had `onDone` and no counterpart at all. `useDismissSafeError` (`web/src/hooks.ts`) renders the message INLINE while the surface is mounted -- the better surface, since it keeps the user's selections and context -- and diverts to a caller-supplied `onFailed` once it is gone. The surviving surface belongs to the parent and differs per screen (a `role="alert"` banner on CollectionsScreen, `notice`+`Toast` on MediaBrowse/Search), so it is a prop contract rather than a rendering decision. Gating dismissal on the busy flag was considered and rejected: it traps the user behind an in-flight request with no cancel path, and would not cancel the write anyway. Applied to the A1 shape -- where the surface owns the error state and is really unmounted: AddItemsDialog plus the four `web/src/media/addTo/` dialogs, whose failures previously could not reach the screen Toast that already showed their successes. Proofs, executed rather than described: - deleting `reportRef.current(message)` alone reddens the new CollectionsScreen test on `Unable to find an element with the text: Request failed with status 500` - `hooks.test.tsx` pins both branches directly, plus that the report goes through the LATEST callback rather than the one captured on first render - `CollectionsScreen.guards.test.tsx`'s is-mounted read count moves 2 -> 1 because the catch's guard migrated into the hook (its `...actual` module mock cannot see the hook's internal `useIsMountedRef()`); removing the surviving `finally` guard takes it to 0 and reddens, so the anti-masking property that count was added for is intact Scope is stated rather than implied. A sweep of all 68 Dialog/ConfirmDialog/SlideOver call sites found three shapes; only A1 is fixed here. A2 -- error state that survives but whose render site is gated by the same condition dismissal clears, mostly delete-confirm flows -- is left open in #877 because its right answer is probably a shared surface, not twenty prop threads. fixes #830 refs #877, #740, #685 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY	2026-09-05 00:23:08 +02:00
timothyandClaude Fable 5.1	a7d91bf15a	fix(876): sweep session narrative out of hooks, workflows, scripts, tests and code comments; grow the detector to the process corpus ... `docs.no-session-narrative` reaches every durable artifact, but its detector scanned only `docs/**/*.md` and root markdown, and nothing had ever swept the rest. The issue named four sites from one grep and called them a floor. Deriving the population instead — a whitespace-joined sweep over every tracked file outside the detector, for the detector's own phrasings plus the attribution and review-round class #812 found — gave 453 sites in 108 files at `fb5592971`, and a second pass for phrasings the first list missed (hyphenated `round-N`, "an earlier version", "the reviewer proved") added residuals in the same files. Every site was classified with #812's three dispositions (CUT / SEVER / KEEP with its sub-kind) under the who-benefits test; the per-site manifests are on the PR. The rejected designs, tested-and-rejected fixtures, measurements and traps stay; the attribution of who found them and the round in which they were found go. The detector's population grows to `.claude/`, `.gitea/`, `.husky/` and `scripts/` regardless of extension, minus the detector and its own test (whose fixtures ARE the phrasings) and minus `scripts/tests/fixtures/` (test data, including decision-record copies — the same reasoning as the records' own exemption, and what keeps the record's depth measurement true), and `--all` lists tracked REGULAR files only — a symlink's content is its target and a gitlink has none. The #812 argument for leaving `docs/superpowers/**` in the population runs the other way here: `--diff` sees only ADDED lines, and 287 of the 453 sites were under 30 days old — this corpus is where narrative is being added, so the advisory nudge has reach. Density agrees: 56 line-mode hits over the 113 regular files the predicate admits, against 9 over 66 docs files before #812. `web/` and C# stay out on the same measurement (3 of 74 PATTERNS-matching sites, ~4,600 files). The predicate did not grow: PATTERNS matched 74 of 453 sites, and widening the word list to the attribution class is the treadmill the withdrawn parity test ran on. The population oracle is restated over segments with the new arms, the synthetic cross product gains the process heads and non-markdown extensions, a fixture witnesses that a tracked symlink is neither scanned nor counted, a `.py.bak` axis separates a by-name exemption from a `startswith` over the same tuple, and eight mutants (drop the process arm, drop the by-name exemption, exempt by `startswith`, drop or add a prefix, drop the fixtures exemption, list only markdown, drop the symlink filter, test the mode per row instead of per path) each redden it. A pre-existing silent drop in `--diff` goes with it: git tab-terminates a `+++` filename that contains a space, and the kept tab made `is_scanned_path` refuse the file with no notice — fixed, with a positive control and its own mutant. Code is unchanged by construction, measured per file type against `origin/main`: Python modules are AST-equal with docstrings stripped, except `#` lines inside the embedded fixture programs (string literals) of three test modules; workflows differ only in `#` lines inside `run:` block scalars; shell, C#, TypeScript and jq are equal with comment lines stripped. The stated exceptions: the detector and its test, 26 vitest titles that carried review-round or severity labels or a reviewer attribution (call sites whose title changed — every changed title line walked back to its `it(` / `it.each(...)(` anchor, so a `' + '` concatenation counts once), two registry note strings and the mutation manifest's prose fields. scripts/tests: 1565 passed. Web: lint, typecheck, 1319 tests green. Closes #876. Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEcBoFw7ctrf3Nb7R7x7wk	2026-09-03 20:51:39 +02:00
timothyandClaude Opus 5	3ec73f3769	fix(887): a pin assumes it is pinning the thing that still DECIDES ... Round 6 found three more false greens and named the class they share, which is worth more than any of the three fixes: * `web/vitest.config.ts` OUTRANKS the pinned `vite.config.ts` — closed in the previous commit, found by probing vitest rather than reading about it. * A DECOY first `test: {` block. The comparison took `text.index("test: {")`, so a copy of the pin placed above `defineConfig` satisfied it while the real block was narrowed. Exactly one is now required — the same assertion this file already made about the gating step's NAME, for the same reason, not carried across. * A `needs:` edge matched by bare job id. `needs:` resolves within its own workflow, so a SECOND workflow publishing this Dockerfile while needing its own unrelated job called `test` satisfied it. Now bound to `GATING_WORKFLOW`. (The reviewer downgraded this to MEDIUM on measuring that `test_remote_state_inventory.py` forces a human to classify any new workflow — so the hole is "the guard is blind", not "silent". The forced review asks about remote state, not about whether the image is gated, so the one-line fix stands.) * A vite PLUGIN can shell out to the suite from `buildStart()`. The plugin ARRAY is pinned; the plugin BODIES are a stated residual, mitigated because `trackedSourceFilesPlugin` is deliberately lazy — a fact its own comment now marks as LOAD-BEARING for the image build rather than leaving as an optimisation note. THE CLASS: **a pin assumes it is pinning the artifact that still decides.** Every route found so far is authority moving where the pin is not looking — to another FILE, another OCCURRENCE in the same file, another WORKFLOW, or a HOOK the pinned command invokes. That question is now written down for the next person adding a pin, because a list of four instances is not what generalises. Prose, all refuted by execution: the residual naming the uncovered COPY shapes was wrong a THIRD time at the same site (`/source/web /elsewhere` IS recognised — only the destination is renamed — and the file's own test 700 lines below said so); "only an `ENV` is unmodelled" was an absolute and is now a list; "Reach: N mutants, 0 missed" is restated as a DEVELOPMENT BATTERY, since it is not in the repo, nothing re-derives it, and an independent battery found misses against an earlier head; and `PUBLISH_ACTION` was claimed covered by anti-vacuity, which proves the selector is non-empty and cannot prove it complete. Battery 61 -> 64, 0 missed. Refs: #887 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019T79beF1Ufid3dXju4yqkF	2026-08-30 14:07:47 +02:00
timothyandClaude Opus 5	a4df8f7958	fix(887): the gate must be REAL, not merely present — two cold reviews' findings ... Both independent reviews (Codex GPT-5.6 cross-family, and a cold Opus agent in an isolated worktree) returned BLOCKED. Both independently confirmed the CI path itself is sound — neither found a route that publishes an image on which the suite never ran — so every finding is about the guard's reach, plus one factual error in the prose. THE STRUCTURAL ONE. The guard asserted a `needs:` edge EXISTS, never that it is load bearing. Since this change deletes the in-image run, that edge is the only remaining layer, so `continue-on-error: true`, `if: false`, a job-level `if:`, `npm test … || true`, a pipe into `tee`, and `set +e` each certified a publish over a red suite with every assertion green. `test_the_gating_suite_run_is_NOT_ADVISORY` closes all six. A filter written into `web/package.json`'s script body was invisible at the call site: `"test": "vitest --exclude x"` with a workflow saying `npm test -- --run` is a filtered gating run reading as clean — the removed defect, one level down. `vitest_scripts()` now derives each script's own narrowing arguments and `suite_args` prepends them. PARSER REACH, every case measured rather than argued. `shlex.split` yields `lint&&npm` as one token, so unspaced `&&` and `;` re-adds were invisible; `shlex` in punctuation_chars mode splits them. Added: `sh -c` payload expansion, `npm --prefix`/`npx -p` flag skipping, `xargs`, heredoc bodies as DATA (a `cat > f <<'EOF' … npm test … EOF` block counted as a real run), `ADD`/JSON-form/no-trailing-slash `COPY` in `carries_spa_source`, and redirections no longer read as spec filters. `--root` and `--config` moved to the narrowing set: both change which specs vitest collects. A FACTUAL ERROR, in five places including the mutation `expect`: "the build context is `web/` + `design-system/`, so there is no `.git`". The context is the repository root (`context: .`) and `.dockerignore` does not exclude `.git`. The true statement is about the STAGE, which copies only those two directories. The conclusion survives — bookworm slim has no git binary either — but a reader who checked would have found `.git` in the context and concluded the note was stale. ONE FINDING WAS MINE, from the mutant battery rather than from either review, and it is the reason the battery exists: `failure_suppressions` tokenised the whole multi-line `run:` body at once. A newline is not a shell separator, so a realistic two-line step — the `ci-step-ran.sh` marker line, then the suite — merged into ONE segment whose head was the marker script, and three suppression mutants passed while my single-line unit test was green. It now works per logical line, and the regression test uses the two-line shape. 17 mutants, 0 missed, each caught by the intended assertion; baseline green. The `docs/guard-inventory.md` residual list is rewritten as MEASURED reach — the previous one was wrong rather than merely short, which cold review rightly called worse than silence. Refs: #887 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019T79beF1Ufid3dXju4yqkF	2026-08-30 13:37:58 +02:00
timothyandClaude Opus 5	febaad77d7	fix(887): the image build builds the SPA and does not test it ... `docker/Dockerfile`'s web-build stage is gitless twice over — the build context is `web/` + `design-system/` so there is no `.git`, and `node:22-bookworm-slim` ships no git binary. Members of the SPA suite need one or the other, so running the suite there required naming the ones that cannot run. That list was a population nothing derived: #883 added a third member without updating the hand-written pair of `--exclude`s, and because `Build & push image (amd64)` is `if: github.event_name != 'pull_request'` the resulting red was unreachable on a PR. It landed on `main` and on the `v*` tag path instead — every image build failed, `:latest` stopped being republished, and a release cut would have failed at the image build. Adding a third `--exclude` re-arms the trap, so the list is removed rather than extended: the stage now lints, typechecks and BUILDS the SPA, and the suite runs once, unfiltered, in `docker-build.yml`'s `test` job on a real checkout. `build` carries `needs: [test, migrations, scan]`, so no image is published past a red suite. `scripts/tests/test_image_build_delegates_the_spa_suite.py` holds both halves — the negative one alone would be satisfied by deleting the `needs:` edge. Three populations, all derived: tracked Dockerfiles and workflows from the git index, and which npm scripts ARE the suite from `web/package.json` (so `test` is in and the Playwright `test:ui-e2e` is out, with no exemption list). Publishing jobs come from the `docker/build-push-action` step and the Dockerfile each builds from that step's own `file:` input, which is why `ci-image.yml` is out of scope by derivation rather than by an entry that would outlive its reason. Four mutants witnessed red, each by the intended test: a filtered suite run put back into the Dockerfile, the `needs:` edge deleted, and the gating run narrowed in both the block and the single-line `run:` step forms. Refs: #887 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019T79beF1Ufid3dXju4yqkF	2026-08-30 13:37:58 +02:00
timothyandtimothy	e8f80c42ce	fix(820): derive where Complete&lt;T&gt; is APPLIED, not just what it means (#883) ... `Complete<T>` (#807) makes SPA full-replace bodies fail typecheck when a builder omits a schema member. Nothing checked it was APPLIED: `completeRequest.guard.test.ts` proves the type's semantics and would stay green with every annotation deleted, and `test_optional_request_members.py`'s COVERED disposition — "the builder is annotated `Complete<T>`" — was a claim about another language's source that nothing verified. Adds `completeAnnotationScan.ts` (compiler-API scanners) + `completeAnnotations.guard.test.ts`, with a synthetic-source fixture suite. Two derived populations: the `Complete<…>` annotations (SPA AST ∩ git index) and the droppable schemas (parsed from the generated `v1.d.ts`, a pass-through of the OpenAPI `required` array). It asserts a production annotation per schema dispositioned as needing one, NO annotation on the server-computed and load-bearing-omission schemas, that every `Complete<X>` resolves to a generated schema rather than a hand-written mirror, and set equality between droppable schemas and the reviewed dispositions. `test_complete_annotation_dispositions.py` cross-checks that table against the authoritative Python one and ships a declared, harness-executed mutation. Found one live defect: `playouts.ts` declared two request types as hand-written mirrors SHADOWING generated schemas of the same name, so their `Complete<>` was checking a local copy rather than the contract — the #754 mechanism wearing the annotation meant to prevent it. Eight review rounds, seven BLOCKED, two independent cold reviewers. A wrapper-signature scanner was built and REMOVED: every blocker traced to that one mechanism (obligation on the wrong population; reachability mistaken for protection, since `Complete<T>` is shallow; body discovery keyed on a parameter name, then parameter-vs-local; and finally `export function` → `export const` blinding the scanner and its cross-check together). Five defects from one mechanism, so the mechanism went rather than a sixth patch. Residuals stated in §4b, the guard-inventory row and the record: per-SCHEMA not per-site or per-wrapper; token presence not liveness; the phantom direction unchecked (#777); a second `setupFiles` entry could discharge; and plugin-level population integrity borrowed from the sibling guard. fixes #820 Co-authored-by: Timothy <timothy@noreply.gitea.tblindustries.be>	2026-08-29 22:06:38 +00:00
timothyandtimothy	8aeacd534a	fix(819): derive the SPA page-size guard population from the git index (#875) ... The guard asserted EXACT completeness over a population enumerated by a directory walk, so an untracked .ts/.tsx under web/src/ entered it and failed as unregistered on that developer's checkout while CI — which only ever checks out tracked files — stayed green. The glob still supplies file CONTENT; the POPULATION is now the git index, read by web/vite-plugins/trackedSourceFiles.ts in Vite's own Node context and handed to the app project as a virtual module. That reaches the index without admitting @types/node to tsconfig.app.json, the obstacle that deferred this in #818. Three mechanisms carry the proof, each added because the previous was measured insufficient: a closed-form restatement of the shared scope predicate (sharing no helper at any depth with what it checks); a second independent `ls-files --others` query cross-checking the population; and real-git tests that execute the derivation against a temp repository. Six residuals are stated with their MEASURED fail-directions, and testing.guard-derives-population-from-source gains a bounded exception plus the closed-form criterion. fixes #819 Co-authored-by: Timothy <timothy@noreply.gitea.tblindustries.be>	2026-08-29 09:14:15 +00:00
timothyandtimothy	ed8b602445	feat(735): bound the numeric FFmpeg profile fields with a 422, and expose readrate pacing (#847) ... Co-authored-by: Timothy <timothy@noreply.gitea.tblindustries.be>	2026-08-26 22:05:38 +00:00
timothyandtimothy	40a3232d9e	feat(734): field-level progressive disclosure — shared FieldHelp trigger + panel (#841) ... Implements the three-level field-help pattern from #734 as a shared component: field name + optional one-sentence summary → a one-short-paragraph panel behind a consistent Info-icon trigger → a future external-docs deep link (`docsHref`, built and typed; no screen passes one yet). Adopted on FFmpegProfilesScreen (9 fields), documented as docs/spa-conventions.md §15 with decision record `spa.field-progressive-disclosure`, and mirrored into the design-system prototype. The panel is portalled to document.body: `.ctv-card` sets `overflow: hidden`, which clips a positioned descendant whatever its z-index, and one field's explainer rendered 12px of a 92px paragraph in every state of the Audio card. A `::before` hover bridge was added and then WITHDRAWN — it held for a vertical descent onto the panel and failed for a diagonal one, leaving a safe sideways exit of 1.25px on an 18px icon. Hover reads the paragraph in place; the panel's interactive content is reached by pinning. Four cold adversarial review rounds; the first three returned BLOCKED. They found five wrong copy claims across nine paragraphs and two vacuous tests in a row for the same mechanism. Deferred with owners: #839 (placement verified by hand, not by a test) and #840 (the portal puts a docsHref link at the end of the tab order). fixes #734 Co-authored-by: Timothy <timothy@noreply.gitea.tblindustries.be>	2026-08-26 18:05:53 +00:00
timothyandtimothy	7453dd3a82	fix(721,740): align Auto-Tune proposal rows on a grid; guard AddItemsDialog's async searches (#831) ... Co-authored-by: Timothy <timothy@noreply.gitea.tblindustries.be>	2026-08-25 21:55:23 +00:00
timothyandtimothy	3e2c07b42f	fix(807): SPA full-replace bodies are built as Complete&lt;T&gt;, with a derived guard over droppable request members (#827) ... Co-authored-by: Timothy <timothy@noreply.gitea.tblindustries.be>	2026-08-23 00:58:19 +00:00
timothy	7be42654fe	fix(685): suppress both empty-states on error; name the addable-kind derivation ... Round-4 verification returned MERGEABLE with no new defects and no BLOCKER/HIGH/MEDIUM. These are its remaining LOW and nits. - "No results — try a search above." asserts a search that COMPLETED and found nothing, so it was false beside a failed request. Both empty-state messages are now suppressed on error and the role="alert" banner is the whole message, as that test's comment already claimed. Pinned positively and negatively so a refactor cannot satisfy the assertion by rendering nothing at all. - The (typeof ADDABLE_TYPE_LIST)[number] derivation is now the named AddableKind, spelled once instead of twice: a third ingress into the searched kinds is most likely to be written by copying one of the existing two, and the "one list, all ingresses" property should be visible at a glance rather than reassembled. - The §3b lesson cited two measured test counts, which go stale against the very suite they describe — a count taken before the helper had unit tests no longer holds now that it does. Scoped the observation to the sha it was measured on and replaced the counts with the invariant they were evidence for: every gate needs at least one test that reddens when that gate ALONE is removed. - Documented why the error banner stays conditionally mounted while the hint's live region does not: role="alert" is the one live-region role screen readers reliably announce on insertion, so the two are correct for opposite reasons. The review flagged the divergence as unexplained, not as wrong. refs #740	2026-08-05 19:40:07 +02:00
timothy	d7725c274c	fix(685): close the second ingress; stop the error path claiming a short query ... Verification round returned MERGEABLE with all three blocking findings resolved by measurement. These are its four remaining items. - DEFAULT_SEARCH_KINDS was the SECOND ingress into the searched kinds and was not derived, so the previous commit's "enforced by the type system" claim held for one of two paths. A non-addable kind there typechecked clean and would have overstated the hint with every one of its rows dropped — the exact defect the derivation exists to prevent. Now derived; verified by mutation that adding 'Collection' to it is a compile error. - The error path fell into the min-query guidance branch, so a valid 2-character query that got a 500 told the user to type at least 2 characters. That branch conflated "nothing searched yet" with "the last search failed". Newly introduced by the previous commit's error-path reset; now gated on !error and pinned by a test. - The aria-live region was mounted conditionally, creating the region and its text in one commit — which most screen readers do not announce. It is now mounted unconditionally with the condition inside. - The §3b lesson mis-stated where the duplicate gate lived: it was inside runSearch, the genuine single sink, NOT at one of the callers — so the rule as written ("put the gate in the single sink, not at each caller") described the revision that was rejected. Reworded to the actual lesson: the gate's home is the shared helper, and "it's the single sink" is not evidence it is the only guard. That misreading is why #685 got this wrong twice. Declined again, with reasons: the NaN pageSize edge (faithful to the sibling helper), the clamp test's unpinned lower bound (same), and the registry's disclosed same-identity substitution gap. refs #740	2026-08-05 19:20:26 +02:00
timothy	b6bf94f129	fix(685): test the helper's bound; delete the masking duplicate gate ... Independent review round 2 returned BLOCKED on two findings, both correct. - The helper's bound was dead code to the suite. searchLibraryBrowseItems had zero tests, so deleting its clamp OR its gate left the whole suite green — while the registry note claimed a caller "cannot skip the bound". That is the previous round's finding relocated, not removed. It now has the three tests its sibling searchLibraryPickerOptions already had (clamp, gate, compile), plus one pinning the full-row return that is its reason to exist. - The screen kept a second copy of the min-query check, and the two masked each other: the 1-character boundary test passed with EITHER gate alone, so it pinned nothing. The screen's copy is deleted; the helper is the sole gate. Measured before/after: with the duplicate present, removing the helper's gate left that test green; with it gone, the same removal reddens it. - §3b contradicted itself two lines apart — the parent still said "there is no truncation, so there is no truncation hint" above a sub-bullet mandating one. Reworded so a hint is permitted, required only where bulk selection makes the count actionable. Same correction to the 'search-bounded' definition. - A failed search left results/totalMatches stale, rendering a confident "Showing 75 of 60000 matches" beside the error banner. The catch clears them. - Results now carry a `Results for "<query>"` heading and the guidance is keyed to the settled query, not the live input, so rows are never shown without saying which search produced them. `selected` persists across queries (correct for a multi-select picker); the Add button's count keeps it discoverable. - The hint sums pre-filter totalCount against post-filter rows, which is only correct because every filterable kind is addable. MediaKindFilter is now derived from ADDABLE_TYPE_LIST, making that a compile error rather than prose. - aria-live on the hint; the #740 doc caveat no longer overstates the typeahead rule as a mandate this screen violates. Declined: the NaN pageSize edge (copied faithfully from the sibling helper) and the registry's same-identity substitution gap (already disclosed in that file). refs #740	2026-08-05 19:08:13 +02:00
timothy	4be3f247d8	fix(685): move the picker bound into the helper; surface the per-kind cap ... Independent review round 2. Verdict was MERGEABLE with no blockers; this takes the two recommended fixes plus the structural one it listed as a follow-up. - The bound was caller discipline, not code: getLibraryBrowseItems does not clamp pageSize, so the bound was only the constant this one call site chose to pass, and §3b is explicit that a bound a caller can exceed is not a bound. New searchLibraryBrowseItems in libraryBrowse.ts owns the min-query gate, the pageSize clamp and the titleContainsQuery compile, returning full LibraryBrowseItem rows plus totalCount (searchLibraryPickerOptions' {id,name} shape loses the mediaType that toAddItemsRequest needs). runSearch keeps one early return, for the spinner only, and no longer re-implements the gate. - The min-query guidance was keyed to the LIVE input, so backspacing below the gate after a search wiped the rendered rows and their checkmarks while `selected` and the Add button still counted them. Keyed to results.length too. - "Nothing left to hint at" was false: each kind is still capped at LIBRARY_PICKER_RESULTS and totalCount was never read. This is a bulk multi-select add, so the cap is surfaced — per-kind totalCounts are summed and rendered as "Showing N of M matches" once it exceeds the rendered rows. The registry note and the §3b bullet are corrected to stop claiming otherwise. - Gate boundary tested at 1 character (§3b: inclusive endpoints, or a > for >= slip passes the whole suite). - The guard test's deviation loop iterates an empty list now, so it gains one bidirectional assertion that is non-vacuous: the set carrying an `issue` field must equal the set classified 'deviation'. - The §3b bullet no longer reads as a conformance certificate: AddItemsDialog still lacks the seqRef and useIsMountedRef guards §3b mandates. That defect is PRE-EXISTING, not introduced here, and is tracked in #740. refs #740	2026-08-05 18:46:43 +02:00
timothy	28ce8c4dfe	fix(685): gate AddItemsDialog on a real query instead of windowing the whole type ... AddItemsDialog.runSearch was reachable with an empty query two ways — a blank form submit, and a kind-chip click, which called it immediately — and getLibraryBrowseItems omits a falsy `query`, so each path degraded into an unfiltered browse of the whole media-library type (first 50 rows, per kind) presented as the answer with nothing surfacing the truncation. All ten ADDABLE_TYPE_LIST entries are spa-conventions §3b Class B media-library types. The dialog is multi-select, so §3b's SearchPicker (single-select) does not fit; it takes §3b's constraints instead: - no request below LIBRARY_PICKER_MIN_QUERY, enforced in runSearch — the single sink both entry paths route through, not duplicated per caller - typed text compiled with titleContainsQuery rather than forwarded raw (a second latent §3b violation here: the search index's default field does not match bare title words) - each kind bounded to LIBRARY_PICKER_RESULTS - merged.slice(0, 50) removed — it silently dropped up to 100 of 150 fetched rows even for a real query Tests assert zero requests below the gate on both paths, exactly one bounded request per kind above it (20k-row fixture), the compiled+escaped query, and that no fetched row is dropped. Each was verified to fail with its mechanism removed. The pageSize registry entry moves from `deviation` to `search-bounded`. That leaves zero deviation entries, so the anti-vacuity assertion guarding that list is deleted deliberately, per its own instruction. Done-when box 4 (collection-family truncation hint) has no subject: this screen offers no collection-family type. fixes #685	2026-08-05 18:46:43 +02:00
timothyandClaude Opus 5	689451161e	fix(684): review round 2 -- drop a false exhaustiveness claim I introduced ... The L1 fix (name every class-b shape) collided with the M1 fix (move SmartCollectionDialog into class-b) in the same commit: the comment claimed "exactly three shapes, which is the whole list" while the registry 80 lines below already held four. That is the same false-exhaustiveness defect this PR exists to remove -- a reader adding a fifth class-b site would conclude theirs cannot be class-b despite rendering a real totalCount hint, and reach for search-bounded or deviation. The operative rule is now stated first and the shape list is explicitly illustrative: a site qualifies by RENDERING a totalCount-backed hint, not by resembling a listed shape. Also, both non-blocking review items: - the deviation prose said the tracking issue goes "in the note", while enforcement is on the structural `issue` field -- it now points at the mechanism that actually binds, and says why the note scrape was rejected; - the UNREGISTERED report prints every position sharing an identity, so it now says "identity seen at:" rather than implying all of them are unregistered. A positionless key cannot know which occurrence is excess; the candidate set is the honest answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 19:17:07 +02:00
timothyandClaude Opus 5	fc8353c75c	fix(684): key the pageSize guard registry on identity, not source position ... The #650 guard pinned every pageSize call site by absolute line:column, making the registry a function of every OTHER file in the repo. The guard was BORN RED. #651 moved AutoTuneScreen.tsx up ten lines and FillerPresetsScreen.tsx down seventy-two, and merged to main BEFORE the guard's own PR (#675) did -- so the registry, authored against a pre-#651 base, was stale the instant it landed. Its own merge run was CANCELLED, so nothing reported it; the red first surfaced on the next push (#676's merge, which touches no web/src file and is not the cause). One ordering accident, not a recurring pattern -- but the exposure is general, because every PR is green against its own base and the breakage exists only in the merge result. Identity is now (file, kind, value). New/removed/changed sites all still fail. The MULTISET comparison is preserved, so a shared identity must be discovered exactly as many times as it is registered. The scanner's positional pageSizeSiteId is untouched: pageSizeScan.test.ts asserts real AST positions against fixed fixtures, the opposite case, with no churn to remove. The one case this costs is stated rather than implied: a same-identity substitution within one file (delete a registered site, add a different unreviewed one with the same kind and value token) now passes. Narrow, and caught only incidentally before. Named in the guard and the record because "costs no coverage" is a claim that outlives whoever made it. Failure reports still print the discovered line:column -- identity and diagnostics need not share a format, and a bare id was useless in a file holding two such sites. Registry reconciled with #651: Playlists/RerunCollections lost their bounded windows to the shared searchLibraryPickerOptions, now registered in its place. Adds a 'search-bounded' class for that shape. Adds a 'deviation' class rather than laundering a live defect into a compliant-looking label. Reconciling the registry surfaced a §3b violation (#685, filed): CollectionsScreen's AddItemsDialog degrades to an unfiltered whole-type window on an empty query and surfaces nothing. Both existing labels would have been false, and either would have made the guard vouch for behaviour that does not exist. Deviation entries must name a tracking issue, enforced by a structural field -- a #\d+ scrape of the note passed with the reference deleted, because notes legitimately cite historical issues. Corrects SmartCollectionDialog to class-b: it does render a totalCount badge, which is class-b's defining evidence. fixes #684 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 19:08:26 +02:00
timothy	d751f5e01d	Merge pull request 'fix(650): two at-cap list truncations, and a completeness guard that keys on the defect' (#675) from fix/650-at-cap-truncation into main	2026-07-27 06:08:13 +00:00
timothyandClaude Opus 5	66c8500e94	fix(651): pre-merge asks — an empty filter loop asserts nothing; fix "1 item need" ... FIX 1: `PlaylistsScreen.test.tsx`'s group-id test filtered POSTs and asserted inside a `for` loop over the result. On the fixed build that array is empty — the unmatched value leaves the select at '', so Create is disabled and jsdom won't dispatch its onClick — so ZERO assertions ran and the title claimed more than the body proved. A future change that re-enabled Create and POSTed `playlistGroupId: null` would still have passed. Added the unconditional `expect(posts).toHaveLength(0)` before the loop. Worth noting this is the ninth instance of the shape on this branch, and the sibling strengthening in the *same commit* got it right (`RerunCollectionsScreen.test.tsx` uses an unconditional `toHaveLength(0)`) — so the lesson didn't generalize even one file over. The rule is: an assertion inside a loop over a filtered collection proves nothing until the collection's length is asserted. FIX 2: "1 item need a selection" — the noun was pluralized, the verb wasn't, and singular is the common case. My test used `/need a selection/i`, which matches both the right and wrong grammar, so nothing could catch it; it now asserts the exact string '1 item needs a selection'. FIX 3: two comments about clicking an already-disabled button read as contradictory policy. They're not — on the parent the Playlists button was ENABLED, so there the click genuinely discriminates, while the rerun button was disabled on both sides, making it a restatement of `toBeDisabled()`. Both comments now say which case they are and why. Added to #677: the row label falls back to "(no X selected)" on empty `selectedName` regardless of `selectedId` (the mirror image of the bug fixed here), and an all-unbindable group list disables Create with no reason shown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 05:19:49 +02:00
timothyandClaude Opus 5	27867e03cf	fix(651): make the stated invariant true on Playlists; pin the predicate's endpoints ... The review's headline finding was in my prose, not my code: spa-conventions and the round-8 commit both claimed an unbindable id "surfaces as 'no selection' with Save disabled" and that regressions "assert zero writes are reachable". True on RerunCollections and FillerPresets. False on PlaylistsScreen in all three respects — `draftFromItem` nulled the id but KEPT `selectedName`, so the row read "Cool Movie" over a null draft; Save had no selection check; and clicking it did issue the PUT with `mediaItemId: null`. Only the server's `ReplacePlaylistItemsHandler` 422 stood there, and the DB would have persisted it (`PlaylistItemConfiguration` marks all four FKs `IsRequired(false)`). Rather than weaken the claim, made it true: a dropped id now clears its label, and Save is gated on every item having a selection, with a visible count as the reason. `playlistGroupId` was the same class on the same screen — seeded from the wire into `AddPlaylistDialog`, re-parsed with a bare `Number()`, and POSTed as an entity reference — so "every path by which an id from the wire becomes editor state" was not literally true. Now filtered from the group options and normalized on submit. Added `selectionId.test.ts`. The predicate had become the single point of failure for eleven call sites across three screens while being exercised only indirectly; nothing pinned the inclusive endpoints, so a `>` for `>=` slip passed the entire suite. Verified by mutating each comparison. Also documented why `0` and negatives are accepted — the contract is bindability, not existence — because every other id check in this repo uses `id > 0` and the next reader would otherwise "fix" the inconsistency. Two of my assertions were vacuous, the eighth of that shape on this branch: one clicked a button it had just asserted disabled (a restatement of `toBeDisabled()`), and one asserted a POST count on a path that never attempted a save. The first is deleted; the second now actually attempts the write, which makes it fail against the unguarded parent. Corrected claim: all five round-8 regressions do fail against their parent, but on their load-bearing assertions (`getByText('A selection is required')`, `queryByText('Bogus Collection')`) — not on the write-count ones, which were passengers. Follow-up filed as #677 (ScheduleItemInspector's unguarded ingresses; list-backed pickers dropping malformed options silently). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	39c4e8df0a	fix(651): review round 8 — put the selection-id predicate at the boundary, not the site ... Round 7 added the int32 check inside `isSearchPickerOption` — the place the defect was found — which left every other door into editor state open. A malformed successful response carrying `1.5` or `2147483648` still entered `draft` through list-backed options and through the selection restored from the detail read, keeping Save enabled and sending a value the API cannot bind, while the identical value arriving via SearchPicker was correctly rejected. The predicate now lives once, in `web/src/api/selectionId.ts`, and sits on every path by which an id from the wire becomes editor state. The class crosses all three screens, not just the one the finding named, so all three are covered: - RerunCollectionsScreen: `toPickerOptions` (3 list branches) + `draftFromRerun` - PlaylistsScreen: `toPickerOptions` (3 list branches) + `draftFromItem` (4 id fields) - FillerPresetsScreen: `draftFromPreset` (5 id fields) + the collection-family browse options - pickers.tsx: `isSearchPickerOption` now delegates rather than carrying its own copy An unbindable id is treated as ABSENT, never coerced — rounding 1.5 to 1 would submit a DIFFERENT record — so it surfaces as "no selection" with Save disabled and a visible reason; an option that cannot be selected safely is dropped rather than rendered. Five regressions assert zero writes are reachable via each previously-unguarded path. Also corrects two of my own test descriptions, per the review: the padded-ETag test is a regression guard rather than a round-7 defect demonstration (Headers strips outer whitespace before the app sees it), and the late-settlement test guards the abort/race COMPOSITION — what it actually fails is an abort-only implementation whose fetch ignores its signal, which is why its stub ignores `init.signal`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	e605e4006a	fix(651): review round 7 — treat "no usable token" as one class, not three values ... HIGH: the fail-closed gate rejected `null` but not the adjacent values. `Headers.get('ETag')` returns `''` for an empty or whitespace-only header, which PASSED the gate and produced an editable draft; `updateRerunCollection`'s `ifMatch ? … : undefined` then dropped the empty string as falsy and sent no `If-Match`, silently overwriting a collaborator — the exact class the gate exists to make unreachable, reached through the value next door. Absent, empty and whitespace are now one case ("no usable concurrency token"), normalized by a single `usableEtag` helper that returns the TRIMMED token or null, so `etagRef` can only ever hold something that will actually be sent. Tested across four blank shapes asserting zero PUTs are reachable, plus a padded ETag that must be trimmed and USED rather than dropped. MEDIUM: the deadline abandoned the wait without cancelling the work, so each Retry stacked another live connection. It now aborts via an AbortSignal (threaded through `getRerunCollectionWithMeta`) AND clears its timer on settlement and unmount. Both halves are kept deliberately: aborting cancels the work, while the rejected race stops the UI waiting even if the abort never propagates — cancellation and giving-up are not the same guarantee, which the late-settlement test proves by using a stub that ignores its signal. MEDIUM: `Number.isFinite` accepted ids the API cannot bind — `1.5` and values outside int32 rendered, committed through `onSelect`, and would fail server-side on `selectedId`. Validated as an int32 integer. MEDIUM: a malformed or failed page was reported as "No matches", telling the user the library is empty when the request actually failed and giving no hint that reopening retries. Failures now surface as a distinct alert. MEDIUM: `spa-conventions.md` still mandated the deleted "never let a refresh clear an id it failed to name" guard and said "the client guard stays" — contradicting the initialize-once bullet 20 lines below it. Rewritten to state that the guard is gone and must not be rebuilt, with the reason (it only ever preserved a list-seeded value that is null in production). Grepping the DELETED TERMS across all docs — the lesson from round 6's stale `rule:` — also caught two stale `signals:` tokens on the record that the rule fix had missed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	a973fc48e2	fix(651): review round 6 — fail closed on a missing ETag; validate elements, not containers ... HIGH: "the draft is only created alongside the ETag" was not the invariant I claimed. The response can OMIT the header, in which case the draft was still created and the subsequent PUT carried no `If-Match` — the round-4 force-write hole in a new form. The editor now fails closed: no concurrency token, no editable draft (error + Retry/Back). Six tests were leaning on ETag-less detail mocks, which is exactly how this stayed invisible; every single-record GET mock now goes through a `detailResponse` helper that always sets one, and the absent case is tested explicitly — asserting zero PUTs are even reachable. MEDIUM: the detail GET had neither deadline nor recovery affordance, so a never-settling request left a bare spinner with no way out. It is now raced against a 15s deadline, the loading view carries a Back control, and the error view offers Retry. MEDIUM: the malformed-body guard checked the container, not the elements. `[null]` passes `Array.isArray`, reaches `setResults`, and throws on `option.id` during render; a wrong-typed `id` would commit an invalid value through `onSelect`. Each element is now validated, and a malformed payload is treated as a failed attempt so it stays retryable rather than cached. MEDIUM: the decision record's `rule:` — the authoritative string, copied verbatim into the catalog that is the documented entry point — still mandated the machinery round 5 deleted: touched-field hydration, `replaceDraft`, conflict reconciliation. Anyone following it would have rebuilt the rejected design. Rewritten to the initialize-once policy and the catalog regenerated; historical prose no longer says `replaceDraft` "is now" separate. MEDIUM: the replacement Reload test resolved its second GET immediately and returned a non-null selection, so it observed neither a pending reload nor the dirty-selection discard — removing `setDraft(null)` could leave it green. It now holds the reload open, asserts the form is ABSENT while pending, and returns `selectedId: null` to pin the case round 3 showed could resurrect a dirty id over a collaborator's change. Checklist item taken from this: when a mechanism is deleted, the decision record's `rule:` is the single most likely thing left stale, and the one string that propagates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	78cd9e0ebf	fix(651): review round 5 — delete the draft-reconciliation layer instead of extending it ... Took the coordinator's proposal. Rounds 2-4 built and rebuilt a layer that merged a late detail response into a draft the user was already editing; it produced a HIGH finding in three consecutive rounds, three of them cross-user lost updates. Round 5's finding was unfixable in kind: `identityConflicts` had no immutable baseline, so it could not tell "the user changed this" from "the server changed this" — giving both a missed conflict (same type, different id: v2 ETag installed over the user's id, third lost update) and a false one (local type switch: dialog opens spuriously, and "Keep editing" leaves `etagRef` null so the next PUT is a silent FORCE-WRITE). The race is removed rather than refereed. `RerunCollectionEditor` now initializes its draft EXACTLY ONCE from the detail GET and withholds the form until it lands; conflicts are detected at save time by the `If-Match` -> 412 -> Reload path that already existed. Deleted: `touchedRef`, `hydrateDraft`, `hydrateSelection`, `hydrateIdentity`, `identityOf`, `identityConflicts`, `replaceDraft`, `replacePending` and the hydrate/replace load mode. Reload simply sets the draft back to null and re-runs the same load, so the form is unmounted while the replacement is in flight — round 4's MEDIUM-4 becomes structural. Two facts make this lossless rather than a regression. The list row could never have helped: `GetPagedRerunCollectionsHandler` applies ZERO `.Include()`s where `GetRerunCollectionByIdHandler` applies fourteen, and both project through the same mapper, so the list response is a strict SUBSET of the detail one — the id round 1 preserved from it is null in production for every row (#671), and existed only in test fixtures. And FillerPresetsScreen/PlaylistsScreen already worked this way; RerunCollections was the outlier, which is why nearly every finding in rounds 3-5 traced to it. The ETag is now written in the same callback that sets the draft, so `draft != null` implies an ETag and a PUT without `If-Match` is unreachable by construction. MEDIUM-2: a failed search retried every debounce forever — a fresh `{ok:false}` re-ran the effect and the success guard declined it. `ok` (is the held answer authoritative) is now separate from `attemptRef` (have we already tried this exact source+query); only an explicit reopen/focus/edit re-arms a retry. MEDIUM-3: a malformed 2xx body resolves as `undefined` (client.ts swallows the SyntaxError), and `setResults(undefined)` threw on the next render. A non-array is now treated as a failed attempt, and `search` is raced against a 10s deadline since a caller-supplied promise carries no abort signal. Two tests were hiding findings and are fixed: the failed-search test pressed Escape before the unintended retry could fire, and the round-4 conflict test claimed to select id 9 while firing the already-selected id 5. Fourteen tests of the deleted machinery are gone with it, replaced by six asserting the new invariants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	f601d957a6	fix(651): review round 4 — an id never travels without its namespace ... Round 4's through-line: rounds 2-3 made HYDRATION treat {collectionType, selectedId, selectedName} as one value, but the fix was applied to the structure that was named rather than to every structure that carries an id. Three more instances of the same class, plus two independent findings. HIGH-1 + HIGH-2 (one change, per the structural directive): every result, option and cached result set now carries its SOURCE, and identity is compared as (type, id). - `SearchPicker` takes a REQUIRED `source` prop and caches results against (source, query), not query text. Keying on text let the round-3 re-query guard SUPPRESS the new source's request and leave the previous namespace's hit clickable under the new label — a Collection id stored as a SmartCollection id. Results from another source are now hidden outright rather than dimmed: they are not stale, they are wrong. Required rather than defaulted, because a default would silently opt every caller out. - `pickerFor` tags list-backed options with the type they were loaded for, on both RerunCollectionsScreen and PlaylistsScreen, so the previous type's rows stop being selectable the instant the active type changes rather than lingering through the replacement load. HIGH-3: a touched identity contradicting the server's type is a CONFLICT. Pinning the user's edit was right; adopting the response's newest ETag alongside it authorized a Save that silently overwrote the collaborator's type change with no 412. The conflict dialog is raised and the stale ETag kept, so even a forced Save 412s. With round 3's Reload defect this is the second cross-user lost update, so it is recorded as a category: never install a save-authorizing ETag over a local edit the server contradicts. MEDIUM-4: the editor is inert while a Reload is pending. The dialog closes immediately, so an edit typed before the replacement landed was silently erased along with the touched set that protected it. MEDIUM-5: cached search provenance records `ok`, so a transient 500 is retried instead of being cached as an authoritative "No matches" that reopening can never clear. Also: `npx tsc --noEmit` typechecks NOTHING in web/ — the root tsconfig is solution-style ("files": [] + references), so it resolves to zero inputs and exits 0. The real gate, and what CI runs, is `npm run typecheck` (tsc -b). Verified by planting a deliberate type error: --noEmit stayed green, -b caught it. Running the real gate surfaced four genuine errors in tests written earlier this branch (a missing required prop and three `never has no call signatures` from closure-assigned mock variables), fixed with the repo's existing holder-object pattern rather than casts. Recorded in spa-conventions so the next session doesn't repeat it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	5b0ba08aab	fix(651): review round 3 — cover the class, not the named instance ... Round 3 found four defects that are all one mistake: each earlier fix enumerated one instance of a class instead of covering the class. Fixed by asking what else is in each class rather than patching the named case. CRITICAL — cross-user lost update. Conflict "Reload" ran through the refresh policy with a touched-set reset. Because a reloaded RemoteStream reports `selectedId: null` under the #671 server gap, the keep-ours-when-the-id-is-missing fallback restored the user's DIRTY selection, the fresh ETag was installed, and the next Save silently overwrote the collaborator's change — with the very edits the user had explicitly asked to discard. "Keep ours when the server omits the id" is a REFRESH policy; a reload is a REPLACE. `replaceDraft` is now a separate function and the mode travels with the load, so the two cannot be confused at the call site. HIGH — `collectionType` and the selection still hydrated apart. `collectionType` says which table an id indexes, so it is part of the same indivisible value as the id and its name; splitting it is the identical bug to splitting id from name. A record retyped server-side mid-load hydrated the new type while retaining the old id, displaying and saving a Collection id as a RemoteStream id. All three fields now resolve as one `Identity` unit: either half touched pins the whole thing, a differing type takes the response's unit whole (null selection included), and only once both sides agree on the type does the id/name rule apply. HIGH — stale results were still committable by pointer. Enter was gated and `onClick` was not: the same defect in another modality. The guard moved into the single `choose()` sink so every commit path is covered, including any added later, and a superseded list is now genuinely inert (`aria-disabled` + dimmed) rather than looking normal and silently no-opping. MEDIUM — reopening after Escape re-queried an already-current result set; the duplicate response reset the cursor the user had since moved, leaving Enter doing nothing. The effect now skips the search when the cached results match the trimmed query, and reopening places the cursor per the ARIA APG instead of swallowing the keypress. Also corrects an overstated justification in FillerPresetsScreen: the render-time id check was dropped because there is no reachable path TODAY, not because "every writer sets both" — the initial load writes the id alone, and a stale resolver can repopulate the label after a clear. Same enumeration error as the findings above; the comment now says what is actually true and what to do if a path appears. Note: an apostrophe I introduced inside the single-quoted `rule:` scalar broke PyYAML while `decisions_validate.py` (hand parser) stayed green — the same trap class as the unquoted `#`, caught only by scripts/tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	ba52219a9a	fix(651): review round 2 — hydrate untouched fields, never merge a selection field-by-field ... 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>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	e7e425fa25	fix(651): review round 1 — never clear an unnamed id, complete the Lucene escaping, keyboard-operable picker ... 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>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	fad6805b91	feat(651): media-library pickers resolve by search instead of a bounded window ... 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>	2026-07-27 04:25:14 +02:00
timothy	17c25e75fa	fix(650): replace node:fs/path/url with import.meta.glob in the pageSize guard ... The gate is npm run typecheck (tsc -b --pretty false, project-mode) — the prior fix for the 3 node:*-unresolvable errors deferred a decision rather than resolving it, per instruction not to change tsconfig unilaterally. Evaluated and rejected: adding "node" to tsconfig.app.json's `types` (makes Node globals type-visible to production browser code, dissolving a deliberate separation the repo documents) and a file-local `/// <reference types="node" />` (empirically leaks Node's ambient `setTimeout` into the whole tsc -b program, breaking 3 unrelated window.setTimeout mocks — confirmed by trying it). Adopted instead: Vite's `import.meta.glob('/src/**/*.{ts,tsx,mts,cts}', { query: '?raw', import: 'default', eager: true })`, resolved natively by Vite/vitest at transform time — no node:fs, no node:path, no node:url, no tsconfig change, no cross-project references, and the guard test stays exactly where it is (`src/api/`). `vite/client` types (hence `import.meta.glob`) were already wired in via `src/vite-env.d.ts`. `isScannableSourceFileName` is unchanged and still the single place the glob's results are filtered through — the extension set moved into the glob literal, but discovery still runs every matched file through this same named, tested predicate rather than a second copy of the logic. Verified the discovery mechanism swap changes nothing observable: a side-by-side comparison test (old fs-based walk vs new glob-based walk, both run against the current repo, not committed — temporary) found byte-identical results — 19 call sites, 136 scannable files, zero set difference in either direction. Re-ran both required plants against the new mechanism: 1. Duplicate at-cap literal appended to builder/ChannelBuilder.tsx (an already-registered file): caught — `UNREGISTERED (1) ... + builder/ChannelBuilder.tsx:2058:54:literal:100`. 2. New file with a `https://` string (no false match), a shorthand `{ pageSize }`, and two literal pageSize values in one ternary's two branches on a single line: all 3 real sites caught at distinct columns — `+ screens/_fakeDefectPlant.ts:4:30:shorthand:pageSize`, `+ screens/_fakeDefectPlant.ts:8:22:literal:111`, `+ screens/_fakeDefectPlant.ts:8:48:literal:222`. Also re-verified the .mts/.cts discovery pin still fails when the extension set is narrowed back to `.ts`/`.tsx` only (2 of 14 predicate cases fail, as before). All three plants reverted after verification; registry/predicate otherwise untouched. Full local gate: `npm run lint` clean, `npm run typecheck` (tsc -b) clean — zero errors, including the 3 node:* ones this commit resolves — `npx vitest run` 110 files / 1078 tests passed (re-run three times; two runs hit transient vitest worker-pool timeouts from overlapping background processes on this machine, unrelated to the code — a clean sequential run passed in full both before and after).	2026-07-27 02:40:31 +02:00
timothy	5b46214774	fix(650): fix 3 tsc -b never-callable errors in gate helpers (project-mode typecheck) ... 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.	2026-07-27 02:08:19 +02:00
timothy	937ee92a3f	fix(650): close two test-adjacency gaps — hook-level single-flight pin, predicate-level glob pin ... 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).	2026-07-27 01:29:15 +02:00
timothy	1c86a1c1fc	fix(650): enforce single-flight in useLibraryBrowse; close scanner false negatives ... 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).	2026-07-27 01:29:15 +02:00
timothy	ca99bedb1a	fix(650): rewrite the pageSize guard on the TS compiler API; fix two append-ownership races ... 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).	2026-07-27 01:29:15 +02:00
timothy	2d049e9a28	fix(650): follow-up — per-occurrence guard identity, shorthand pageSize detection, and three UI defects ... 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.	2026-07-27 01:29:15 +02:00
timothy	ad4ac6c7e0	fix(650): page Trakt lists to completeness and report real totals in loadCollections ... 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).	2026-07-27 01:29:15 +02:00