fix(830): a write failure reports to a surface that outlives the dialog that started it #878

Open

timothy wants to merge 7 commits from fix/830-add-items-error-surface into main

pull from: fix/830-add-items-error-surface

merge into: :main

:main

:renovate/meziantou.analyzer-3.x

:release/v26.15.0-notes

:fix/830-add-items-error-surface

:renovate/lucene.net

:renovate/cliwrap-3.x

:issue-806-guard-populations

:renovate/dotnet-monorepo

:scratch/767b-poisoned

:scratch/767b-control

:release/v26.14.0-notes

:release/v26.14.0

:renovate/sqlitepclraw.bundle_e_sqlite3-3.x

:docs/510-skill-logo-bug-policy

:fix/510-watermark-resolution-policy

:fix/629-verdict-classifier-falseopens

:fix/609-decisions-edit-token-scope

:issue-135-clear-to-none

:release/v26.12.0-notes

:fix/409b-lastscan-api-parity

:fix/401-updatechannel-mirror-422

:fix/327-playlist-rename-validation

:fix/410-scancancel-log-level

:fix/409-447-librariesscreen-neverscanned

:fix/338-zap-exit-code

:fix/367-plex-budget-message

:fix/310-debom-legacy-cs

:ci/604-lane-rebalance

:feat/388-design-mirror

:feat/247-test-ownership

:feat/247-primary-action

:feat/357-player-owned-playback

:feat/357-jellyfin-plugin-poc

:fix/289-mcp-hardening

:issue58-mcp

:feat/244-channels-extract

:ci/auto-bump-prod-compose

:feat/multi-rerun-collections-api

:feat/collections-api

:feat/quick-wins

:feat/185-docs-part2

:feat/140-collections-screen

:feat/146-channel-edit

:feat/147-classic-ui-link

:issue22-renovate-dashboard

:feat/91-cutover

:feat/63-composite-create

:feat/65-library-browse

:feat/85-epg

:feat/86-schedule-editor

:feat/109-dashboard-data

:feat/99-session-tracking

:fix/dockerfile-node-tag

:feat/59-spa-foundation

:docs/59-ui-redesign-brief

:feat/102-json-guide

:feat/111-schedule-durations

:feat/104-artwork-upload

:feat/103-media-sources-api

:feat/playouts-read-api

:feat/108-health-api

:feat/105-picker-list-endpoints

:issue-97-channel-state-api

:issue42-jellyfin-musicvideos

:issue46-rest-api-error-contract

:dependabot/nuget/ErsatzTV.FFmpeg.Tests/multi-d307a2e06f

:qsv-improvements

:hdr-vulkan-cuda-test

Conversation 1 Commits 7 Files Changed 8

+465 -31

timothy commented 2026-08-29 19:33:02 +02:00

Owner

Copy Link

Copy Source

fixes #830
refs #877, #740, #685

The defect

AddItemsDialog.submit POSTs 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 via 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 when none were.

#740 guarded that setError with an is-mounted check. That was right and stays — it made the drop deliberate rather than accidental. A deliberate drop is still a user who is told nothing.

The mechanism, and the distinction that took five review rounds to get right

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. CollectionsScreen reports into its screen-level role="alert" banner.

The load-bearing distinction: a callback that REPORTS an outcome and a callback that REQUESTS A DISMISSAL are not the same thing.

• onAdded reports — it reloads the parent's list, and dropping it after a write that succeeded leaves that list stale. Unguarded.
• onClose requests a dismissal — and after dismissal it aims at whatever the user opened next. Guarded.

Conflating them was measured wrong in both directions: gating both makes a completed write silent; un-gating both makes a late success close a dialog the user reopened to retry, discarding the selection they rebuilt.

Gating dismissal itself 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.

Proofs — executed, not described

• Disarming reportRef.current(message) reddens the integration test on Unable to find an element with the text: Request failed with status 500. That clause is shared, so it reddens three tests — expect three reds, not one.
• Deleting the if (mountedRef.current) around onClose() reddens a late SUCCESS does not close the dialog the user reopened after dismissing (#830) alone. That test carries an anti-vacuity check — it waits for a second GET of the items endpoint, proving the late response was processed — because "the dialog is still open" holds trivially otherwise.
• hooks.test.tsx pins both hook branches plus that the report goes through the latest callback.
• CollectionsScreen.guards.test.tsx's is-mounted read count moves 2 → 1 (the catch's guard migrated into the hook, which that test's ...actual mock cannot see). Removing the surviving finally guard takes it to 0 and reddens, so the anti-masking property is intact rather than weakened.

Scope — one site, and why the rest were withdrawn

A sweep of all 68 Dialog/ConfirmDialog/SlideOver sites found three shapes. This fixes A1 at AddItemsDialog only.

The four web/src/media/addTo/ dialogs are the same shape and were fixed here across two commits, then reverted after review measured the extension unsafe:

• They gate both halves. Measured on AddToCollectionDialog: onAdded called 0 times after dismissal.
• Un-gating both — my first fix — introduced the reopen-clobber, measured against the real AddToMenu, plus clearSelection() wiping a rebuilt multi-select.
• The one-line gate that works at AddItemsDialog is not sufficient there, because AddToMenu.handleAdded closes its own dialog. That asymmetry is the concrete reason one half shipped and the other did not.

Two defects from one mechanism in two rounds, in code the issue never asked me to touch → #877, with every measurement attached. A2 and B are unchanged; A2 is also #877.

Docs

• New decision record spa.dismissible-write-failure-reporting + regenerated catalog. It keeps the wrong first claim visible — I asserted "success already outlives dismissal at every one of these sites" after reading one site — because "one read, four assumed, written into three durable artifacts before measuring" is the lesson.
• docs/spa-conventions.md §3c (the report/dismiss split) and §5c (which now says the addTo layer has no failure channel at all, instead of telling authors to wire an onFailed that does not exist).

Verification

Gate	Result
npx vitest run	1277 passed (119 files)
npm run lint	0 errors, 0 warnings
npm run build / tsc -b	clean
pytest scripts/tests	1228 passed, 2 skipped
decisions_validate.py	OK, catalog regenerates with no drift

Review

Six rounds, cold and worktree-isolated. It found a Blocker (my central prose claim measured false), an adjacent defect my own fix introduced, three sentences that survived the revert of the code they described, a coverage claim this record's own mechanics: field contradicted, and finally that the convention contradicted its own named exemplar. All measured, none taken on assertion.

The cross-family (Codex) pass exhausted its usage quota mid-read and produced no findings, so the review was same-family. Stating that rather than implying cross-family coverage I don't have.

One note on the Python suite: test_mutation_harness.py[scripts/decisions_validate.py] fails until the new record is staged — the harness builds its sandbox from git-tracked content, so an untracked record plus a regenerated catalog reads as "catalog is stale". Verified green on a clean origin/main first to confirm it was mine; no harness change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY

fixes #830 refs #877, #740, #685 ## The defect `AddItemsDialog.submit` POSTs 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 via `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 when none were.** #740 guarded that `setError` with an is-mounted check. That was right and stays — it made the drop *deliberate* rather than accidental. A deliberate drop is still a user who is told nothing. ## The mechanism, and the distinction that took five review rounds to get right `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. `CollectionsScreen` reports into its screen-level `role="alert"` banner. The load-bearing distinction: **a callback that REPORTS an outcome and a callback that REQUESTS A DISMISSAL are not the same thing.** - `onAdded` reports — it reloads the parent's list, and dropping it after a write that succeeded leaves that list stale. **Unguarded.** - `onClose` requests a dismissal — and after dismissal it aims at whatever the user opened next. **Guarded.** Conflating them was measured wrong in both directions: gating both makes a completed write silent; un-gating both makes a late success close a dialog the user reopened to retry, discarding the selection they rebuilt. Gating *dismissal itself* 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. ## Proofs — executed, not described - Disarming `reportRef.current(message)` reddens the integration test on `Unable to find an element with the text: Request failed with status 500`. That clause is shared, so it reddens **three** tests — expect three reds, not one. - Deleting the `if (mountedRef.current)` around `onClose()` reddens `a late SUCCESS does not close the dialog the user reopened after dismissing (#830)` **alone**. That test carries an anti-vacuity check — it waits for a *second* GET of the items endpoint, proving the late response was processed — because "the dialog is still open" holds trivially otherwise. - `hooks.test.tsx` pins both hook branches plus that the report goes through the **latest** callback. - `CollectionsScreen.guards.test.tsx`'s is-mounted read count moves **2 → 1** (the catch's guard migrated into the hook, which that test's `...actual` mock cannot see). Removing the surviving `finally` guard takes it to **0** and reddens, so the anti-masking property is intact rather than weakened. ## Scope — one site, and why the rest were withdrawn A sweep of all 68 `Dialog`/`ConfirmDialog`/`SlideOver` sites found three shapes. This fixes **A1 at `AddItemsDialog` only**. The four `web/src/media/addTo/` dialogs are the same shape and were fixed here across two commits, then **reverted** after review measured the extension unsafe: - They gate **both** halves. Measured on `AddToCollectionDialog`: `onAdded` called **0 times** after dismissal. - Un-gating both — my first fix — introduced the reopen-clobber, measured against the real `AddToMenu`, plus `clearSelection()` wiping a rebuilt multi-select. - The one-line gate that works at `AddItemsDialog` is **not sufficient** there, because `AddToMenu.handleAdded` closes its own dialog. That asymmetry is the concrete reason one half shipped and the other did not. Two defects from one mechanism in two rounds, in code the issue never asked me to touch → **#877**, with every measurement attached. A2 and B are unchanged; A2 is also #877. ## Docs - New decision record `spa.dismissible-write-failure-reporting` + regenerated catalog. It **keeps the wrong first claim visible** — I asserted "success already outlives dismissal at every one of these sites" after reading one site — because "one read, four assumed, written into three durable artifacts before measuring" is the lesson. - `docs/spa-conventions.md` §3c (the report/dismiss split) and §5c (which now says the `addTo` layer has no failure channel at all, instead of telling authors to wire an `onFailed` that does not exist). ## Verification | Gate | Result | |---|---| | `npx vitest run` | 1277 passed (119 files) | | `npm run lint` | 0 errors, 0 warnings | | `npm run build` / `tsc -b` | clean | | `pytest scripts/tests` | 1228 passed, 2 skipped | | `decisions_validate.py` | OK, catalog regenerates with no drift | ## Review Six rounds, cold and worktree-isolated. It found a Blocker (my central prose claim measured false), an adjacent defect my own fix introduced, three sentences that survived the revert of the code they described, a coverage claim this record's own `mechanics:` field contradicted, and finally that the convention contradicted its own named exemplar. All measured, none taken on assertion. The cross-family (Codex) pass exhausted its usage quota mid-read and produced **no** findings, so the review was **same-family**. Stating that rather than implying cross-family coverage I don't have. One note on the Python suite: `test_mutation_harness.py[scripts/decisions_validate.py]` fails until the new record is **staged** — the harness builds its sandbox from git-tracked content, so an untracked record plus a regenerated catalog reads as "catalog is stale". Verified green on a clean `origin/main` first to confirm it was mine; no harness change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY

timothy added 1 commit 2026-08-29 19:33:03 +02:00

fix(830): a write failure reports to a surface that outlives the dialog that started it ... 044b64ee87

`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.

#740 guarded that `setError` with an is-mounted check, which was right and stays -- it made the
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

timothy added 1 commit 2026-08-29 20:08:56 +02:00

fix(830): report SUCCESS past dismissal too — the review measured my claim false ... 06a582de50

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

timothy added 1 commit 2026-08-29 20:35:06 +02:00

fix(830): withdraw the media/addTo extension — two rounds, two defects, one coupled gate ... 5d035e3d3e

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

timothy added 1 commit 2026-08-29 20:46:54 +02:00

docs(830): three sentences survived the revert of the code they described ... 96a800dd61

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

timothy added 1 commit 2026-08-29 20:57:59 +02:00

docs(830): drop a coverage claim this record's own mechanics field contradicts ... 3717f623db

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

timothy added 1 commit 2026-08-29 21:18:31 +02:00

fix(830): gate onClose at the exemplar — the convention contradicted its own reference ... d90326f762

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

timothy added 1 commit 2026-08-29 21:38:36 +02:00

test(830): pin the ORDINARY close too — the negative test did not cover it ... f376d6cdf4

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

timothy commented 2026-08-29 21:49:19 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ f376d6c

Seven rounds, cold worktree-isolated reviewer. Found and closed: a false central premise (measured), a coverage gap, a regression the fix introduced, three sentences that outlived the code they described, a self-contradicting coverage claim, and a convention contradicted by its own exemplar. Each red witnessed in isolation; the two onClose mutations are orthogonal. Cross-family (Codex) pass produced NO findings - quota exhausted mid-read - so this is same-family review.

Review-verdict: MERGEABLE @ f376d6c Seven rounds, cold worktree-isolated reviewer. Found and closed: a false central premise (measured), a coverage gap, a regression the fix introduced, three sentences that outlived the code they described, a self-contradicting coverage claim, and a convention contradicted by its own exemplar. Each red witnessed in isolation; the two onClose mutations are orthogonal. Cross-family (Codex) pass produced NO findings - quota exhausted mid-read - so this is same-family review.

timothy referenced this pull request 2026-08-29 21:49:59 +02:00

AddItemsDialog: a failed add is silently swallowed when the dialog is closed mid-request #830

All checks were successful

Hide all checks

Build ErsatzTV Image / CI toolchain image resolves (pull_request) Successful in 7s

Details

Build ErsatzTV Image / Delimiter ban (release path) (pull_request) Successful in 19s

Details

PR Gates / Docs update reminder (pull_request) Successful in 11s

Details

PR Gates / CI image pin matches docker/ci (pull_request) Successful in 12s

Details

PR Gates / decisions lifecycle (pull_request) Successful in 26s

Details

PR Gates / Fix proofs (Proves trailers) (pull_request) Successful in 18s

Details

Review verdict / Set review-verdict status (pull_request_target) Successful in 19s

Details

review-verdict/h10 Review-verdict: MERGEABLE @ f376d6c (base: main)

Required

Details

PR Gates / Script lint and tests (ruff + pytest) (pull_request) Successful in 11m35s

Details

Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 13m37s

Required

Details

Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 9m18s

Required

Details

Build ErsatzTV Image / Build & push image (amd64) (pull_request) Skipped

Details

Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 9m9s

Details

Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 11s

Details

Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 10s

Details

This pull request has changes conflicting with the target branch.

• web/src/screens/CollectionsScreen.tsx

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/830-add-items-error-surface:fix/830-add-items-error-surface

git checkout fix/830-add-items-error-surface

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

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

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#878