fix(830): a write failure reports to a surface that outlives the dialog that started it #878
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#878
Reference in New Issue
Block a user
fixes #830
refs #877, #740, #685
The defect
AddItemsDialog.submitPOSTs to/api/v1/collections/{id}/itemsand reported failure into a banner rendered from its own state. The dialog is dismissible mid-request through three paths that never consultadding— Escape and a backdrop click (both viauseOverlayBehavior) and the header close button — and the caller remounts it onkey={`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
setErrorwith 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-suppliedonFailedonce it is gone.CollectionsScreenreports into its screen-levelrole="alert"banner.The load-bearing distinction: a callback that REPORTS an outcome and a callback that REQUESTS A DISMISSAL are not the same thing.
onAddedreports — it reloads the parent's list, and dropping it after a write that succeeded leaves that list stale. Unguarded.onCloserequests 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
reportRef.current(message)reddens the integration test onUnable 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.if (mountedRef.current)aroundonClose()reddensa 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.tsxpins both hook branches plus that the report goes through the latest callback.reportFailure(...)foronAddFailed(...)in the catch — i.e. diverting a failure to the parent banner while the dialog is still open — reddensrenders a failed add INSIDE the dialog while it is still open (#830)alone (1 failed / 490 passed across the 42src/screensfiles). The hook's unit tests cannot see this: they prove the inline branch exists, not that this call site reaches it.CollectionsScreen.guards.test.tsx's is-mounted read count moves 2 → 1 (the catch's guard migrated into the hook, which that test's...actualmock cannot see). Removing the survivingfinallyguard 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/SlideOversites found three shapes. This fixes A1 atAddItemsDialogonly.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:AddToCollectionDialog:onAddedcalled 0 times after dismissal.AddToMenu, plusclearSelection()wiping a rebuilt multi-select.AddItemsDialogis not sufficient there, becauseAddToMenu.handleAddedcloses 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
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 — added post-rebase — pin the arm the call site reaches, not only the hook's) and §5c (which now says theaddTolayer has no failure channel at all, instead of telling authors to wire anonFailedthat does not exist).Verification
Re-measured 2026-09-05 on a second rebase (merge-base
92db6dc81, up from the2026-09-04 rebase's
efadbec29—origin/mainmoved again in between). Thecode diff is byte-identical across both rebases:
git diff <pre-rebase-head>...HEAD -- web/compared equal to
git diff origin/main...HEAD -- web/on the new head, andthis rebase hit zero conflicts (nothing to hand-resolve). The decisions
catalog was regenerated (
build_decisions_catalog.py) and produced no diff —it was already in sync.
npm run check:apinpm run lintnpm run typecheck/npm run buildnpm test(vitest)pytest scripts/testscheck-doc-narrative.py --diff origin/maindecisions_validate.pyNo
.csfiles are touched by this branch, so the .NET build/test gate and theBOM/
dotnet formatcheck do not apply. No.tsx/.tssource needed conflictresolution (the rebase was clean), so live-E2E was not re-run; CI's Functional
E2E already ran green on the prior head.
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 cleanorigin/mainfirst to confirm it was mine; no harness change.Post-rebase round. Rebased onto current
origin/main(efadbec29) with the same code diff, then re-reviewed cold. It found that nothing pinned which arm ofuseDismissSafeErrorthe call site reaches — the mutation above left the wholesrc/screenssuite green, and under it a failure landing while the dialog is up renders into the screen banner, which sits behind acreatePortalpanel witharia-modal="true": covered for sighted users, hidden from AT, and the dialog the user is looking at stays blank. That is the shape the record itself names as its own defect. Closed by an integration assertion (fail the POST with the dialog still open; assert the message is inside[role="dialog"]and appears exactly once in the tree), recorded asmechanics(5) on the decision record and as general guidance in §3c.🤖 Generated with Claude Code
https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY
https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV
Review-verdict: MERGEABLE @
f376d6cSeven 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.
`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_01XE2tF2aUasK2hWPmBRsrMYAdversarial 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_01XE2tF2aUasK2hWPmBRsrMYf376d6cdf4toe1d211cd1bReview-verdict: MERGEABLE @
e1d211cRebase resume onto post-#908 main by the orchestrated-session workflow: diff-of-diffs against the previously verdicted head
f376d6cbyte-identical for web/ and docs/; the three mutation proofs re-executed on the rebased head (3 reds / 1 red / 1 red, as the PR body claims); two cold review rounds on the rebased tree (correctness Opus high in its own worktree, conformance sonnet high), round two nits only; one fix commit pins that the exemplar reaches the hook's inline arm. Cross-family review: not required (routine risk class; SPA + docs). Orchestrator session.