timothy/ersatztv
1
0
Fork 0
Code Issues 57 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

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

New Issue
Closed
opened 2026-08-25 23:24:21 +02:00 by timothy · 2 comments
timothy commented 2026-08-25 23:24:21 +02:00
Owner
Copy Link
Copy Source

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

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

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.

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 bugpriority: mediumfrontend labels 2026-08-25 23:24:21 +02:00
timothy added the in-progress label 2026-08-29 18:56:27 +02:00
timothy commented 2026-08-29 18:56:37 +02:00
Author
Owner
Copy Link
Copy Source

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 commented 2026-09-05 00:41:22 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Shipped in PR #878 (fixes #830, refs #877/#740/#685) — head e1d211cd1. AddItemsDialog's failed add now reports through useDismissSafeError (web/src/hooks.ts): inline, inside the still-open dialog, while it is mounted, and via a caller-supplied onFailed to CollectionsScreen's screen-level banner once it is not.
Root cause: Dialog dismisses through three paths (Escape, backdrop click, header close) that never consult the adding busy flag, and the caller remounts the dialog on a keyed prop, so dismissal genuinely unmounts the component holding the error banner. A write failure after that point had nowhere left to render.
Decisions/conventions changed: New record spa.dismissible-write-failure-reporting (docs/decisions/records/spa/) + docs/spa-conventions.md §3c/§5c — the report/dismiss split (onFailed reports an outcome, onClose requests a dismissal; gate the latter on mount, never the former) and which arm of the hook a call site must reach when the surface is still open.
Reusable knowledge: A callback that reports an outcome and one that requests a dismissal are not interchangeable for async-guard purposes — gating both drops a completed write silently; gating neither reopens the discarded-selection bug. The four web/src/media/addTo/ dialogs share this shape but gate both halves together in a way this fix does not generalize to safely; tracked separately as #877 rather than folded in here.
Verification: npm run check:api && lint && typecheck && build && test (1325 vitest passed, 121 files), pytest scripts/tests (1569 passed, 3 skipped), check-doc-narrative.py clean, decisions catalog regenerates with no drift. Rebased twice onto a moving origin/main (latest merge-base 92db6dc81) with the code diff verified byte-identical each time; both rebases hit zero conflicts. CI armed on e1d211cd1 (PR #878).
Deferred: The web/src/media/addTo/ dialogs (shape A2) — tracked as #877, with the two rejected extension attempts and their measurements recorded there and in the decision record.
Docs updated: docs/decisions/records/spa/dismissible-write-failure-reporting.md (new) + regenerated docs/decisions/README.md; docs/spa-conventions.md §3c/§5c.

## Closing record **Outcome:** Shipped in PR #878 (fixes #830, refs #877/#740/#685) — head `e1d211cd1`. `AddItemsDialog`'s failed add now reports through `useDismissSafeError` (`web/src/hooks.ts`): inline, inside the still-open dialog, while it is mounted, and via a caller-supplied `onFailed` to `CollectionsScreen`'s screen-level banner once it is not. **Root cause:** `Dialog` dismisses through three paths (Escape, backdrop click, header close) that never consult the `adding` busy flag, and the caller remounts the dialog on a keyed prop, so dismissal genuinely unmounts the component holding the error banner. A write failure after that point had nowhere left to render. **Decisions/conventions changed:** New record `spa.dismissible-write-failure-reporting` (`docs/decisions/records/spa/`) + `docs/spa-conventions.md` §3c/§5c — the report/dismiss split (`onFailed` reports an outcome, `onClose` requests a dismissal; gate the latter on mount, never the former) and which arm of the hook a call site must reach when the surface is still open. **Reusable knowledge:** A callback that *reports an outcome* and one that *requests a dismissal* are not interchangeable for async-guard purposes — gating both drops a completed write silently; gating neither reopens the discarded-selection bug. The four `web/src/media/addTo/` dialogs share this shape but gate both halves together in a way this fix does not generalize to safely; tracked separately as #877 rather than folded in here. **Verification:** `npm run check:api && lint && typecheck && build && test` (1325 vitest passed, 121 files), `pytest scripts/tests` (1569 passed, 3 skipped), `check-doc-narrative.py` clean, decisions catalog regenerates with no drift. Rebased twice onto a moving `origin/main` (latest merge-base `92db6dc81`) with the code diff verified byte-identical each time; both rebases hit zero conflicts. CI armed on `e1d211cd1` (PR #878). **Deferred:** The `web/src/media/addTo/` dialogs (shape A2) — tracked as #877, with the two rejected extension attempts and their measurements recorded there and in the decision record. **Docs updated:** `docs/decisions/records/spa/dismissible-write-failure-reporting.md` (new) + regenerated `docs/decisions/README.md`; `docs/spa-conventions.md` §3c/§5c.
timothy removed the in-progress label 2026-09-05 01:02:50 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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 bug frontend priority: medium
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#830