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

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

New Issue
Open
opened 2026-08-29 19:30:54 +02:00 by timothy · 1 comment
timothy commented 2026-08-29 19:30:54 +02:00
Owner
Copy Link
Copy Source

Split out of #830, which fixed one of the three shapes a call-site sweep found and named this one rather than folding twenty screens into that PR.

What #830 established

Dialog, SlideOver and ConfirmDialog all dismiss through Escape, a backdrop/scrim click (useOverlayBehavior) and a header close button, and none of those consult a busy flag — disabling the footer Cancel button, which nearly every dialog here does, looks like it closes the hole and does not.

A sweep of all 68 Dialog/ConfirmDialog/SlideOver call sites (2026-08-29) found three shapes:

  • A1 — hard unmount. The dismissible component owns the error state and is genuinely unmounted ({x && <Foo/>}, props.open early-return, or a key= remount), so the state dies with it. Fixed in #830 via useDismissSafeError at AddItemsDialog and the four web/src/media/addTo/ dialogs.
  • A2 — gated display. The error state lives in a parent that never unmounts, so it survives — but the JSX that renders it is nested inside the same open/target condition the dismiss handler just cleared, so there is no DOM node left to render into. Same user-visible outcome as A1, different mechanism. This issue.
  • B — genuine survivor. A screen-level role="alert" banner outside every dialog. DecoTemplatesScreen and BlocksScreen already have this.

The A2 shape, concretely

SettingsScreen.tsx:1198-1201 is the clearest instance, and its own comment shows the tradeoff was considered:

onCancel={() => {
  setDeleteTarget(null);
  setDeleteError(null);
}}

deleteTarget gates the ConfirmDialog's message, so dismissing mid-DELETE nulls both the display condition and the message. The later .catch then sets deleteError on an error nobody can ever see.

CollectionsScreen.tsx:1090-1092 is the same thing one level out — the caller force-nulls the child's error prop:

error={createOpen ? dialogError : null}

Population (from the #830 sweep — verified vs. inferred is marked)

Verified by reading: SettingsScreen.tsx:1185-1206, CollectionsScreen.tsx:1090-1092 (create/rename/smart), PlayoutsScreen.tsx:119-133,259+, PlaylistsScreen.tsx:299-350,351-413, TraktListsScreen.tsx:91-142, ChannelBuilder.tsx:1587-1594,1625-1645, builder/SmartCollectionDialog.tsx:19-33.

Matched structurally, NOT individually verified (message={target ? … : ''} + onCancel nulling the same target): FFmpegProfilesScreen.tsx:452-465, FillerPresetsScreen.tsx:468-478, WatermarksScreen.tsx:333-346, SchedulesScreen.tsx:636-645, RemoteSourceScreen.tsx:202-215.

Not triaged at all (~16 sites / 8 files, time-boxed out of the sweep): MultiCollectionsScreen.tsx, RerunCollectionsScreen.tsx, PlayoutScheduleEditors.tsx, TrashScreen.tsx, PlexSourceScreen.tsx, schedules/ScheduleForm.tsx, and the editor-level dialogs in DecosScreen.tsx / TemplatesScreen.tsx.

Treat that three-way split as the starting point, not the answer — re-derive the population from source, because an issue's file list is not the population.

Why it was not done in #830

#830's mechanism is a prop contract: the child reports to a caller-supplied onFailed, and the parent decides where it lands. That is the right answer when the child owns the state. For A2 the state is already in the parent — the bug is purely that its render site is conditional — so useDismissSafeError is the wrong tool and applying it would be cargo-culting.

A2 is also dominated by delete-confirm flows, which are near-identical to each other. That suggests a shared answer (an unconditional screen-level banner region, or a small toast host wired at AppShell) rather than 20 per-site prop threads — and that is a design decision worth its own review, not a widening of a bug fix.

What to decide

  1. Whether A2 gets a shared reporting surface (a toast host at AppShell, or a convention that every list screen carries an unconditional role="alert" region like DecoTemplatesScreen already does), or per-site fixes.
  2. Whether Dialog/ConfirmDialog should expose the busy state so dismissal can at least be reported rather than silently gated — noting #830 rejected BLOCKING dismissal on busy, because it traps the user behind an in-flight request with no cancel path.
  3. Whether MediaDetailScreen's AddToMenu usages (which wire neither onDone nor onFailed) get a surface — a known, deliberate A1 gap left open by #830.

Done-when

  • The A2 population is re-derived from source, not taken from the list above
  • A mechanism is chosen and recorded (extend spa.dismissible-write-failure-reporting, or a new record superseding the relevant part of it)
  • At least one A2 site is fixed with a test that is WITNESSED RED when the reporting path alone is removed (quote the red)
  • Sites deliberately left unfixed are enumerated and recorded as such, not left implied
  • Adversarial review passed

Related

#830 (A1 + the mechanism + the sweep), #740 (the async guards that made the drop deliberate), docs/decisions/records/spa/dismissible-write-failure-reporting.md, docs/spa-conventions.md §3c.

Split out of #830, which fixed one of the three shapes a call-site sweep found and named this one rather than folding twenty screens into that PR. ## What #830 established `Dialog`, `SlideOver` and `ConfirmDialog` all dismiss through Escape, a backdrop/scrim click (`useOverlayBehavior`) and a header close button, and **none of those consult a busy flag** — disabling the footer Cancel button, which nearly every dialog here does, looks like it closes the hole and does not. A sweep of all 68 `Dialog`/`ConfirmDialog`/`SlideOver` call sites (2026-08-29) found three shapes: - **A1 — hard unmount.** The dismissible component owns the error state and is genuinely unmounted (`{x && <Foo/>}`, `props.open` early-return, or a `key=` remount), so the state dies with it. **Fixed in #830** via `useDismissSafeError` at `AddItemsDialog` and the four `web/src/media/addTo/` dialogs. - **A2 — gated display.** The error state lives in a parent that never unmounts, so it survives — but the JSX that renders it is nested inside the same `open`/`target` condition the dismiss handler just cleared, so there is no DOM node left to render into. **Same user-visible outcome as A1, different mechanism. This issue.** - **B — genuine survivor.** A screen-level `role="alert"` banner outside every dialog. `DecoTemplatesScreen` and `BlocksScreen` already have this. ## The A2 shape, concretely `SettingsScreen.tsx:1198-1201` is the clearest instance, and its own comment shows the tradeoff was considered: ```tsx onCancel={() => { setDeleteTarget(null); setDeleteError(null); }} ``` `deleteTarget` gates the `ConfirmDialog`'s `message`, so dismissing mid-DELETE nulls both the display condition and the message. The later `.catch` then sets `deleteError` on an error nobody can ever see. `CollectionsScreen.tsx:1090-1092` is the same thing one level out — the caller force-nulls the child's error prop: ```tsx error={createOpen ? dialogError : null} ``` ## Population (from the #830 sweep — verified vs. inferred is marked) **Verified by reading:** `SettingsScreen.tsx:1185-1206`, `CollectionsScreen.tsx:1090-1092` (create/rename/smart), `PlayoutsScreen.tsx:119-133,259+`, `PlaylistsScreen.tsx:299-350,351-413`, `TraktListsScreen.tsx:91-142`, `ChannelBuilder.tsx:1587-1594,1625-1645`, `builder/SmartCollectionDialog.tsx:19-33`. **Matched structurally, NOT individually verified** (`message={target ? … : ''}` + `onCancel` nulling the same target): `FFmpegProfilesScreen.tsx:452-465`, `FillerPresetsScreen.tsx:468-478`, `WatermarksScreen.tsx:333-346`, `SchedulesScreen.tsx:636-645`, `RemoteSourceScreen.tsx:202-215`. **Not triaged at all** (~16 sites / 8 files, time-boxed out of the sweep): `MultiCollectionsScreen.tsx`, `RerunCollectionsScreen.tsx`, `PlayoutScheduleEditors.tsx`, `TrashScreen.tsx`, `PlexSourceScreen.tsx`, `schedules/ScheduleForm.tsx`, and the editor-level dialogs in `DecosScreen.tsx` / `TemplatesScreen.tsx`. Treat that three-way split as the starting point, not the answer — **re-derive the population from source**, because an issue's file list is not the population. ## Why it was not done in #830 #830's mechanism is a **prop contract**: the child reports to a caller-supplied `onFailed`, and the parent decides where it lands. That is the right answer when the child owns the state. For A2 the state is *already* in the parent — the bug is purely that its render site is conditional — so `useDismissSafeError` is the wrong tool and applying it would be cargo-culting. A2 is also dominated by **delete-confirm** flows, which are near-identical to each other. That suggests a shared answer (an unconditional screen-level banner region, or a small toast host wired at `AppShell`) rather than 20 per-site prop threads — and that is a design decision worth its own review, not a widening of a bug fix. ## What to decide 1. Whether A2 gets a shared reporting surface (a toast host at `AppShell`, or a convention that every list screen carries an unconditional `role="alert"` region like `DecoTemplatesScreen` already does), or per-site fixes. 2. Whether `Dialog`/`ConfirmDialog` should expose the busy state so dismissal can at least be *reported* rather than silently gated — noting #830 rejected BLOCKING dismissal on busy, because it traps the user behind an in-flight request with no cancel path. 3. Whether `MediaDetailScreen`'s `AddToMenu` usages (which wire neither `onDone` nor `onFailed`) get a surface — a known, deliberate A1 gap left open by #830. ## Done-when - [ ] The A2 population is re-derived from source, not taken from the list above - [ ] A mechanism is chosen and recorded (extend `spa.dismissible-write-failure-reporting`, or a new record superseding the relevant part of it) - [ ] At least one A2 site is fixed with a test that is WITNESSED RED when the reporting path alone is removed (quote the red) - [ ] Sites deliberately left unfixed are enumerated and recorded as such, not left implied - [ ] Adversarial review passed ## Related #830 (A1 + the mechanism + the sweep), #740 (the async guards that made the drop deliberate), `docs/decisions/records/spa/dismissible-write-failure-reporting.md`, `docs/spa-conventions.md` §3c.
timothy added the bugpriority: lowfrontend labels 2026-08-29 19:30:54 +02:00
timothy commented 2026-08-29 20:27:34 +02:00
Author
Owner
Copy Link
Copy Source

Scope grew: the four media/addTo/ dialogs (A1) are now here too, with measurements

#830 originally fixed A1 at five sites. Two adversarial review rounds found the media/addTo/ four were not safe to fix in that PR, so they were withdrawn from it and belong here. What follows was all measured during #830 — it is evidence, not a plan.

The media/addTo/ four gate BOTH halves of the outcome

AddToCollectionDialog, AddToPlaylistDialog, AddToScheduleDialog, SaveAsSmartCollectionDialog each wrap the success callback in their own unmount guard:

.then((collection) => {
  if (!activeRef.current) { return; }
  onAdded?.(collection.name);
  onClose();
})

Measured on AddToCollectionDialog (deferred POST, submit, unmount via open={false}, then settle): onAdded is called 0 times after dismissal. The other three carry a visibly identical gate — read, not probed. So dismiss-then-succeed is as silent as dismiss-then-fail: no confirmation Toast, and on SearchScreen / MediaBrowseScreen the selection is never cleared because onAddedToSelectionTarget / onBulkAdded never run.

AddToMenu also exposes onDone and no failure counterpart at all, so AddToCollectionDialog's submitError has no route to the screen's Toast even when the dialog is still open.

Why the obvious fix is wrong — this is the part worth keeping

Removing that gate reports success correctly and also un-gates onClose(). The two mean different things:

  • onAdded / onDone — "tell the parent what happened". Must survive dismissal.
  • onClose — "close me". After dismissal "me" is whatever the user opened next.

Measured against the real AddToMenu, with the gate removed: submit → Escape → user reopens the dialog to retry the add they think failed → the first POST returns 204 → the reopened dialog closes under them. On SearchScreen / MediaBrowseScreen the success handler also runs clearSelection(), wiping a multi-select the user had rebuilt.

Also measured: gating onClose alone does not fix it, because AddToMenu.handleAdded itself calls setDialog(null):

const handleAdded = (message: string) => {
  setDialog(null);
  onDone?.(message);
};

The combination that measured clean was: gate only the close in the dialog (onAdded?.(name); if (activeRef.current) { onClose(); }), and drop the now-redundant setDialog(null) from AddToMenu.handleAdded, and the same in SearchScreen.onAddedToSelectionTarget. That is three coupled edits across five files.

And it leaves one genuine open question: should clearSelection() fire for a write the user walked away from? The write succeeded, so the selection that produced it is stale — but the user may have built a different selection since. That is a product decision, which is why it did not ride along in a bug fix.

What to do here

  • Give the media/addTo/ layer a failure channel (onFailed on AddToMenu and the four dialogs), using useDismissSafeError per spa.dismissible-write-failure-reporting
  • Un-gate the success callback, and separately gate onClose, moving the parents off their own setDialog(null)
  • Decide the clearSelection() question explicitly and record it
  • Cover it: reverting reportFailure in all four dialogs left the entire 1276-test suite green, so this layer currently asserts nothing about either direction. A witnessed-red test per direction, on at least AddToCollectionDialog
  • MediaDetailScreen wires neither onDone nor onFailed at seven AddToMenu sites — decide whether it gets a surface or is recorded as deliberately silent

Also carried over from the #830 review

  • useIsMountedRef clears in a PASSIVE effect cleanup, so there is a narrow window where the DOM node is detached but the flag still reads true — the message then renders inline into a dead tree instead of diverting. useLayoutEffect closes it deterministically. Not done in #830 because that hook is shared by every async caller in the SPA (#578) and retiming it does not belong in a bug fix.
  • The two surviving surfaces are not equivalent. CollectionsScreen uses role="alert"; Toast is role="status" (polite) with a single last-writer-wins notice slot, so a later success can overwrite a pending failure. Worth settling if a shared surface is introduced.

Related: #830 (the mechanism + the one fixed A1 site), docs/decisions/records/spa/dismissible-write-failure-reporting.md, docs/spa-conventions.md §3c and §5c.

## Scope grew: the four `media/addTo/` dialogs (A1) are now here too, with measurements #830 originally fixed A1 at **five** sites. Two adversarial review rounds found the `media/addTo/` four were not safe to fix in that PR, so they were **withdrawn from it** and belong here. What follows was all measured during #830 — it is evidence, not a plan. ### The `media/addTo/` four gate BOTH halves of the outcome `AddToCollectionDialog`, `AddToPlaylistDialog`, `AddToScheduleDialog`, `SaveAsSmartCollectionDialog` each wrap the success callback in their own unmount guard: ```tsx .then((collection) => { if (!activeRef.current) { return; } onAdded?.(collection.name); onClose(); }) ``` **Measured** on `AddToCollectionDialog` (deferred POST, submit, unmount via `open={false}`, then settle): `onAdded` is called **0 times** after dismissal. The other three carry a visibly identical gate — read, not probed. So dismiss-then-**succeed** is as silent as dismiss-then-fail: no confirmation Toast, and on `SearchScreen` / `MediaBrowseScreen` the selection is never cleared because `onAddedToSelectionTarget` / `onBulkAdded` never run. `AddToMenu` also exposes `onDone` and **no failure counterpart at all**, so `AddToCollectionDialog`'s `submitError` has no route to the screen's `Toast` even when the dialog is still open. ### Why the obvious fix is wrong — this is the part worth keeping Removing that gate reports success correctly **and also un-gates `onClose()`**. The two mean different things: - `onAdded` / `onDone` — "tell the parent what happened". Must survive dismissal. - `onClose` — "close me". After dismissal "me" is whatever the user opened next. **Measured against the real `AddToMenu`**, with the gate removed: submit → Escape → user reopens the dialog to retry the add they think failed → the first POST returns 204 → **the reopened dialog closes under them**. On `SearchScreen` / `MediaBrowseScreen` the success handler also runs `clearSelection()`, wiping a multi-select the user had rebuilt. Also measured: **gating `onClose` alone does not fix it**, because `AddToMenu.handleAdded` itself calls `setDialog(null)`: ```tsx const handleAdded = (message: string) => { setDialog(null); onDone?.(message); }; ``` The combination that measured clean was: gate only the close in the dialog (`onAdded?.(name); if (activeRef.current) { onClose(); }`), **and** drop the now-redundant `setDialog(null)` from `AddToMenu.handleAdded`, **and** the same in `SearchScreen.onAddedToSelectionTarget`. That is three coupled edits across five files. And it leaves one genuine open question: **should `clearSelection()` fire for a write the user walked away from?** The write succeeded, so the selection that produced it is stale — but the user may have built a *different* selection since. That is a product decision, which is why it did not ride along in a bug fix. ### What to do here - [ ] Give the `media/addTo/` layer a failure channel (`onFailed` on `AddToMenu` and the four dialogs), using `useDismissSafeError` per `spa.dismissible-write-failure-reporting` - [ ] Un-gate the success callback, and **separately** gate `onClose`, moving the parents off their own `setDialog(null)` - [ ] Decide the `clearSelection()` question explicitly and record it - [ ] Cover it: reverting `reportFailure` in all four dialogs left the entire 1276-test suite green, so this layer currently asserts nothing about either direction. A witnessed-red test per direction, on at least `AddToCollectionDialog` - [ ] `MediaDetailScreen` wires neither `onDone` nor `onFailed` at seven `AddToMenu` sites — decide whether it gets a surface or is recorded as deliberately silent ### Also carried over from the #830 review - **`useIsMountedRef` clears in a PASSIVE effect cleanup**, so there is a narrow window where the DOM node is detached but the flag still reads `true` — the message then renders inline into a dead tree instead of diverting. `useLayoutEffect` closes it deterministically. Not done in #830 because that hook is shared by every async caller in the SPA (#578) and retiming it does not belong in a bug fix. - **The two surviving surfaces are not equivalent.** `CollectionsScreen` uses `role="alert"`; `Toast` is `role="status"` (polite) with a single last-writer-wins `notice` slot, so a later success can overwrite a pending failure. Worth settling if a shared surface is introduced. Related: #830 (the mechanism + the one fixed A1 site), `docs/decisions/records/spa/dismissible-write-failure-reporting.md`, `docs/spa-conventions.md` §3c and §5c.
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: low
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#877