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

New Issue

2026-08-25T23:24:21+02:00

timothy commented

2026-08-25 23:24:21 +02:00

Surfaced by the independent review of #740 (PR for that issue). Pre-existing and NOT fixed by #740 — that issue added the is-mounted guard, which correctly stops a setState on an unmounted tree; it deliberately does not change the UX, and should not have.

The defect

AddItemsDialog.submit (web/src/screens/CollectionsScreen.tsx) POSTs to /api/v1/collections/{id}/items and reports failure by rendering an error banner inside the dialog. The dialog is closable mid-request: the footer Cancel button is disabled={adding}, but Dialog closes on Escape, on a backdrop click and via the header close button, and none of those consult adding (web/src/components/overlay.tsx). Closing unmounts the instance — the caller renders it under key={`add-${pickerOpen}`}.

So: select 12 items → Add → press Escape → the request fails. There is no longer a dialog to render the error into, nothing else surfaces it, and the collection list reloads unchanged. The user believes 12 items were added and nothing was.

The write itself is not lost or partial — it simply never happened, and the UI does not say so.

Why it was left out of #740

#740's scope is the two async guards §3b mandates. Guarding setError there makes the drop deliberate rather than accidental, which is correct — the fix for the missing feedback is a different mechanism (one that outlives the dialog), not a guard. Bundling a notification surface into that PR would have widened it well past its issue.

What to decide

The error has to survive the dialog. Options, in rough order of size:

Don't let it be closed mid-request — gate Escape/backdrop/close on adding, the way the Cancel button already is. Smallest diff, but it traps the user behind a request with no cancel path, which is why Dialog doesn't do this by default.
Surface it on the parent screen — hoist the add call, or its error, to CollectionsScreen so the banner renders where the user actually is. Consistent with the collection screen already owning messageFromCollectionError output.
A toast/notification surface — check whether one exists before adding one; if not, that is its own decision.

Option 2 looks right, but the pattern should be checked against how other screens report a write failure whose originating surface can close, since this is unlikely to be the only instance — sweep for it rather than fixing this one site (fix-the-boundary-not-the-site).

Done-when

A failed add is reported to the user even when the dialog was closed before the request settled
The chosen mechanism is checked against other write paths whose originating surface can close mid-request, and either reused or recorded as deliberately local
A test pins it, and reddens when the reporting path alone is removed
Adversarial review passed

Resolution — option 2, and option 1 was tested and rejected. Gating dismissal on adding traps the user behind an in-flight request with no cancel path, and would not cancel the write anyway. There is no toast host in this SPA (option 3), so none was added for one screen. The mechanism is useDismissSafeError (web/src/hooks.ts): inline while mounted, diverted to a caller-supplied reporter once dismissed. PR #878, record spa.dismissible-write-failure-reporting, docs/spa-conventions.md §3c.

Box 2 — the sweep ran and its answer was "reuse it at ONE site, record the rest". All 68 Dialog/ConfirmDialog/SlideOver call sites were triaged into three shapes: (A1) the surface owns the error state and really unmounts; (A2) the state survives but its render site is gated by the same condition dismissal clears — the majority, mostly delete-confirm flows; (B) an already-correct screen-level banner. Only A1 at AddItemsDialog is fixed here. The four media/addTo/ dialogs are the same shape and were fixed, then withdrawn after review measured the extension unsafe twice — see #877, which carries every measurement. A2 is #877 as well.

Box 3 — witnessed, and the split is pinned in both directions. Disarming reportRef.current(message) reddens the integration test on Unable to find an element with the text: Request failed with status 500. Separately: deleting the if (mountedRef.current) around onClose() reddens only the reopen test; deleting the onClose() call entirely reddens only the ordinary-close assertion. The two are orthogonal — neither masks the other.

Box 4 — seven rounds, and the review changed the shipped result four times. It measured my central prose claim FALSE, caught a regression my own fix introduced, found three sentences that outlived the code they described, one coverage claim this record's own mechanics: field contradicted, and a convention that contradicted its own named exemplar. Recorded because the last two are the same shape: after a retraction, the retracted WORDING has to be swept, not just the code. Caveat stated rather than implied: the cross-family (Codex) pass exhausted its quota mid-read and produced no findings, so this was a same-family cold, worktree-isolated reviewer.

Surfaced by the independent review of #740 (PR for that issue). **Pre-existing and NOT fixed by #740** — that issue added the is-mounted guard, which correctly stops a setState on an unmounted tree; it deliberately does not change the UX, and should not have. ## The defect `AddItemsDialog.submit` (`web/src/screens/CollectionsScreen.tsx`) POSTs to `/api/v1/collections/{id}/items` and reports failure by rendering an error banner **inside the dialog**. The dialog is closable mid-request: the footer Cancel button is `disabled={adding}`, but `Dialog` closes on **Escape**, on a **backdrop click** and via the **header close button**, and none of those consult `adding` (`web/src/components/overlay.tsx`). Closing unmounts the instance — the caller renders it under ``key={`add-${pickerOpen}`}``. So: select 12 items → Add → press Escape → the request fails. There is no longer a dialog to render the error into, nothing else surfaces it, and the collection list reloads unchanged. **The user believes 12 items were added and nothing was.** The write itself is not lost or partial — it simply never happened, and the UI does not say so. ## Why it was left out of #740 #740's scope is the two async guards §3b mandates. Guarding `setError` there makes the drop *deliberate* rather than accidental, which is correct — the fix for the missing feedback is a different mechanism (one that outlives the dialog), not a guard. Bundling a notification surface into that PR would have widened it well past its issue. ## What to decide The error has to survive the dialog. Options, in rough order of size: 1. **Don't let it be closed mid-request** — gate Escape/backdrop/close on `adding`, the way the Cancel button already is. Smallest diff, but it traps the user behind a request with no cancel path, which is why `Dialog` doesn't do this by default. 2. **Surface it on the parent screen** — hoist the add call, or its error, to `CollectionsScreen` so the banner renders where the user actually is. Consistent with the collection screen already owning `messageFromCollectionError` output. 3. **A toast/notification surface** — check whether one exists before adding one; if not, that is its own decision. Option 2 looks right, but the pattern should be checked against how other screens report a write failure whose originating surface can close, since this is unlikely to be the only instance — sweep for it rather than fixing this one site (`fix-the-boundary-not-the-site`). ## Done-when - [x] A failed add is reported to the user even when the dialog was closed before the request settled - [x] The chosen mechanism is checked against other write paths whose originating surface can close mid-request, and either reused or recorded as deliberately local - [x] A test pins it, and reddens when the reporting path alone is removed - [x] Adversarial review passed **Resolution — option 2, and option 1 was tested and rejected.** Gating dismissal on `adding` traps the user behind an in-flight request with no cancel path, and would not cancel the write anyway. There is no toast host in this SPA (option 3), so none was added for one screen. The mechanism is `useDismissSafeError` (`web/src/hooks.ts`): inline while mounted, diverted to a caller-supplied reporter once dismissed. PR #878, record `spa.dismissible-write-failure-reporting`, `docs/spa-conventions.md` §3c. **Box 2 — the sweep ran and its answer was "reuse it at ONE site, record the rest".** All 68 `Dialog`/`ConfirmDialog`/`SlideOver` call sites were triaged into three shapes: (A1) the surface owns the error state and really unmounts; (A2) the state survives but its render site is gated by the same condition dismissal clears — the majority, mostly delete-confirm flows; (B) an already-correct screen-level banner. Only A1 at `AddItemsDialog` is fixed here. The four `media/addTo/` dialogs are the same shape and were fixed, then **withdrawn** after review measured the extension unsafe twice — see #877, which carries every measurement. A2 is #877 as well. **Box 3 — witnessed, and the split is pinned in both directions.** Disarming `reportRef.current(message)` reddens the integration test on `Unable to find an element with the text: Request failed with status 500`. Separately: deleting the `if (mountedRef.current)` around `onClose()` reddens only the reopen test; deleting the `onClose()` call entirely reddens only the ordinary-close assertion. The two are orthogonal — neither masks the other. **Box 4 — seven rounds, and the review changed the shipped result four times.** It measured my central prose claim FALSE, caught a regression my own fix introduced, found three sentences that outlived the code they described, one coverage claim this record's own `mechanics:` field contradicted, and a convention that contradicted its own named exemplar. Recorded because the last two are the same shape: after a retraction, the retracted WORDING has to be swept, not just the code. Caveat stated rather than implied: the cross-family (Codex) pass exhausted its quota mid-read and produced no findings, so this was a **same-family** cold, worktree-isolated reviewer. ## Related #740 (the guards, which made this drop deliberate), #685 (the AddItemsDialog work that preceded it), #877 (the withdrawn A1 half + all of A2), `docs/spa-conventions.md` §3b.

timothy added the bug priority: medium frontend labels 2026-08-25 23:24:21 +02:00

timothy referenced this issue

2026-08-25 23:29:06 +02:00

fix(721,740): align Auto-Tune proposal rows on a grid; guard AddItemsDialog's async searches #831

timothy referenced this issue

2026-08-25 23:56:10 +02:00

AddItemsDialog.runSearch has no stale-response or is-mounted guard (pre-existing, surfaced during #685) #740

timothy added the in-progress label 2026-08-29 18:56:27 +02:00

timothy commented

2026-08-29 18:56:37 +02:00

Claiming. Claude Code / Opus 5, orchestrator tier. Worktree ~/ersatztv-wt/830-add-items-error-surface, branch fix/830-add-items-error-surface off a fresh origin/main (8aeacd534).

Pre-claim checks (process.parallel-session-claim), all four clear as of 2026-08-29 19:00 CEST: no open PR body references #830 (the five open PRs are all Renovate); git ls-remote --heads origin has no branch naming 830 or additems; the issue had zero comments, so no claim predates the label; git fetch origin main is current.

Context for whoever reads this next: three other sessions are live on this repo right now — #823+#824 (worktree 823-824-701-deferrals), #820 (main-3), #812 (main-2). Those four are the entire in-progress set, so #830 does not overlap any of them.

Scope note on done-when box 2 (the sweep): the issue asks that the chosen mechanism be checked against other write paths whose originating surface can close mid-request, per fix-the-boundary-not-the-site. I am treating that sweep as in-scope for this PR rather than a follow-up, and will derive the population of such surfaces from source rather than listing them by hand — an issue's own file list is not the population.

Claiming. Claude Code / Opus 5, orchestrator tier. Worktree `~/ersatztv-wt/830-add-items-error-surface`, branch `fix/830-add-items-error-surface` off a fresh `origin/main` (8aeacd534). Pre-claim checks (`process.parallel-session-claim`), all four clear as of 2026-08-29 19:00 CEST: no open PR body references #830 (the five open PRs are all Renovate); `git ls-remote --heads origin` has no branch naming 830 or `additems`; the issue had zero comments, so no claim predates the label; `git fetch origin main` is current. Context for whoever reads this next: three other sessions are live on this repo right now — #823+#824 (worktree `823-824-701-deferrals`), #820 (`main-3`), #812 (`main-2`). Those four are the entire `in-progress` set, so #830 does not overlap any of them. Scope note on done-when box 2 (the sweep): the issue asks that the chosen mechanism be checked against other write paths whose originating surface can close mid-request, per `fix-the-boundary-not-the-site`. I am treating that sweep as in-scope for this PR rather than a follow-up, and will derive the population of such surfaces from source rather than listing them by hand — an issue's own file list is not the population.

timothy referenced this issue

2026-08-29 19:30:54 +02:00

Dismissible write failures, shape A2: the error state survives but its render site is gated by the same condition dismissal clears #877

timothy referenced this issue from a commit

2026-08-29 19:32:39 +02:00

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

timothy referenced a pull request that will close this issue

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

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

timothy referenced this issue from a commit

2026-08-29 20:08:56 +02:00

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