From 9ba397e1ddf5768e9caedb2dde6ad3f4d01fc994 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 19:31:56 +0200 Subject: [PATCH 1/8] fix(830): a write failure reports to a surface that outlives the dialog that started it `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) Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY --- docs/decisions/README.md | 1 + .../dismissible-write-failure-reporting.md | 46 ++++++++++++ docs/spa-conventions.md | 38 ++++++++++ web/src/hooks.test.tsx | 71 ++++++++++++++++++- web/src/hooks.ts | 52 +++++++++++++- web/src/media/addTo/AddToCollectionDialog.tsx | 22 ++++-- web/src/media/addTo/AddToMenu.tsx | 11 ++- web/src/media/addTo/AddToPlaylistDialog.tsx | 29 +++++--- web/src/media/addTo/AddToScheduleDialog.tsx | 21 ++++-- .../addTo/SaveAsSmartCollectionDialog.tsx | 21 ++++-- .../screens/CollectionsScreen.guards.test.tsx | 17 +++-- web/src/screens/CollectionsScreen.test.tsx | 69 +++++++++++++++++- web/src/screens/CollectionsScreen.tsx | 45 +++++++----- web/src/screens/MediaBrowseScreen.tsx | 9 ++- web/src/screens/SearchScreen.tsx | 10 ++- 15 files changed, 402 insertions(+), 60 deletions(-) create mode 100644 docs/decisions/records/spa/dismissible-write-failure-reporting.md diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 89994e5b7..837a71fef 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -195,6 +195,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `spa.collection-custom-order-ui` | Collection custom ordering uses per-row Move up/Move down buttons (not drag) and is offered for any manual collection with custom order enabled, not just movies-only. | 2026-07-09 | [link](records/spa/collection-custom-order-ui.md) | | `spa.datetime-local-input` | The channel-mode date/time input uses a native `` instead of free-text Chronic natural-language parsing. | 2026-07-09 | [link](records/spa/datetime-local-input.md) | | `spa.deco-templates-table` | The deco-templates editor also renders its day/deco assignment as a table, extending (not replacing) the templates-editor-table convention. | 2026-07-09 | [link](records/spa/deco-templates-table.md) | +| `spa.dismissible-write-failure-reporting` | A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. The surviving surface belongs to the PARENT and differs per screen (a screen-level `role="alert"` banner on `CollectionsScreen`, the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen`), so the mechanism is a PROP CONTRACT rather than a rendering decision; there is no global toast host in this SPA and this decision does not add one. `onFailed` is optional because not every host screen has a surface to report onto — where it is omitted the failure is dropped exactly as before, which is a KNOWN remaining gap, not a claim of coverage. | 2026-08-29 | [link](records/spa/dismissible-write-failure-reporting.md) | | `spa.download-sample-gate` | The SPA disables both Download Media Sample and Download Results while a troubleshooting session is starting/running (Blazor only gated Download Results). | 2026-07-09 | [link](records/spa/download-sample-gate.md) | | `spa.field-progressive-disclosure` | A consequential settings field explains itself through one shared `FieldHelp` icon trigger beside the field name — never the label itself, never a widened `Tooltip` — with the paragraph declared `as const` in the screen's own `FIELD_HELP` record and the panel portalled to `document.body`. | 2026-08-26 | [link](records/spa/field-progressive-disclosure.md) | | `spa.legacy-redirect-matcher` | `LegacyUiRedirects.TryGetRedirect` is a two-tier matcher — an exact `OrdinalIgnoreCase` `Map` (Tier 1) then an ordered segment-template pattern list (Tier 2, first-match-wins) — collision-free by construction, with a guard invariant that no rule may prefix-match `/api`, `/artwork`, `/docs`, `/openapi`, `/iptv`, `/app`, or `/media/sources`. | 2026-07-11 | [link](records/spa/legacy-redirect-matcher.md) | diff --git a/docs/decisions/records/spa/dismissible-write-failure-reporting.md b/docs/decisions/records/spa/dismissible-write-failure-reporting.md new file mode 100644 index 000000000..180a763e4 --- /dev/null +++ b/docs/decisions/records/spa/dismissible-write-failure-reporting.md @@ -0,0 +1,46 @@ +--- +key: spa.dismissible-write-failure-reporting +title: '2026-08-29 — A write failure reports to a surface that OUTLIVES the dismissible surface that started it (#830)' +status: active +since: '2026-08-29' +supersedes: none +superseded-by: none +rule: 'A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. The surviving surface belongs to the PARENT and differs per screen (a screen-level `role="alert"` banner on `CollectionsScreen`, the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen`), so the mechanism is a PROP CONTRACT rather than a rendering decision; there is no global toast host in this SPA and this decision does not add one. `onFailed` is optional because not every host screen has a surface to report onto — where it is omitted the failure is dropped exactly as before, which is a KNOWN remaining gap, not a claim of coverage.' +signals: 'failed add silently swallowed · dialog closed mid-request · Escape backdrop close button ignore adding flag · error banner unmounts with the dialog · useDismissSafeError inline vs onFailed · success outlives dismissal but failure does not · onDone has no failure counterpart · paths: `web/src/hooks.ts`, `web/src/screens/CollectionsScreen.tsx`, `web/src/media/addTo/`, `web/src/components/overlay.tsx` · issues: #830, #877, #740, #685' +mechanics: 'Pinned three ways. (1) `web/src/hooks.test.tsx` → "useDismissSafeError (#830)" pins both branches directly: mounted reports INLINE and does not call `onFailed`, unmounted calls `onFailed`, and the report goes through the LATEST callback rather than the one captured on first render. (2) `CollectionsScreen.test.tsx` → "reports a failed add on the screen when the dialog was dismissed before the request settled (#830)" drives the whole path — it parks the POST in flight, dismisses via Escape, then settles the request and asserts the message is on the screen and NOT inside a dialog. Executed: deleting `reportRef.current(message)` alone reddens it on `Unable to find an element with the text: Request failed with status 500`. (3) `CollectionsScreen.guards.test.tsx` counts is-mounted reads and moved from 2 to 1 when the catch''s guard migrated into the hook; its `...actual` module mock cannot see the hook''s internal `useIsMountedRef()`, which is why (1) exists. Executed: removing the surviving `finally` guard takes that count to 0 and reddens.' +--- + +**The asymmetry is the whole finding.** At every one of these sites SUCCESS already outlives +dismissal, because it is reported through a parent callback (`onAdded` / `onDone`, which +`MediaBrowseScreen` and `SearchScreen` turn into a `Toast`). Only FAILURE was rendered from the +dismissible component's own state. So the fix is not a new notification system — it is routing +failure through the channel success already uses. `AddToMenu` had `onDone` and no counterpart at +all, which is why a failed "Add to collection" from a media card reported nowhere. + +**Why the inline branch is kept rather than always reporting to the parent.** While the dialog is +up, inline is the better surface: it keeps the user's selections and the context they are looking +at. Diverting to a parent banner in that case would be its own defect — the message would surface +somewhere else while the dialog the user is staring at stays blank. + +**Why not simply gate dismissal on the busy flag.** That was considered and rejected: gating +Escape/backdrop/close on `adding` traps the user behind an in-flight request with no cancel path, +which is exactly why `Dialog` does not do it by default. The request is also genuinely still +running — cancelling the UI would not cancel the write. + +**#740 guarded this and did not fix it, correctly.** That issue added the is-mounted guard so the +`setError` could not fire on an unmounted tree. That is right and stays. Its scope was the async +guards, and it deliberately did not change the UX — which left the guard making the drop +*deliberate* rather than accidental. A deliberate drop is still a user who believes twelve items +were added when none were, and that is the half this record closes. + +**Scope, stated rather than implied.** A sweep of all 68 `Dialog`/`ConfirmDialog`/`SlideOver` call +sites (2026-08-29) found three shapes: (A1) the surface hard-unmounts and takes its own error state +with it — the shape fixed here, at `AddItemsDialog` and the four `web/src/media/addTo/` dialogs; +(A2) the error state lives in a parent that never unmounts, but the JSX rendering it is nested +inside the same `open`/`target` condition dismissal just cleared, so it has no DOM node to render +into — the majority pattern, mostly delete-confirm flows; (B) a genuinely surviving screen-level +banner, which `DecoTemplatesScreen` and `BlocksScreen` already have. Only A1 is fixed here. A2 is +LEFT OPEN deliberately: it is a larger, mostly-delete-confirm population whose right answer may be +a shared reporting surface rather than a per-site prop, and folding it in would have made this PR a +rewrite of twenty screens. It is tracked in #877, which carries the sweep's verified-vs-inferred +split forward, rather than being recorded as done here. diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index 25a7b4dd4..f30ded67b 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -404,6 +404,44 @@ double-invoke for the re-arm) **and** the *integration* (mock the hook module an component actually read `current` — and saw `false` — when the late response landed). Verify each by removing the mechanism and confirming the test fails. +## 3c. A write failure must outlive the dismissible surface that started it (#830) + +`Dialog`, `SlideOver` and `ConfirmDialog` are all dismissible **mid-request** through three paths +that never consult a busy flag: Escape and a backdrop/scrim click (both via `useOverlayBehavior`), +and the header close button. Disabling the footer Cancel button — which nearly every dialog here +does — looks like it closes that hole and does not. + +So an error rendered from the dismissible component's own state has nowhere to go once the user +dismisses it. The request still runs, still fails, and the screen reloads unchanged: the user reads +that as success. Guarding the `setError` with an is-mounted check (§3b) is necessary but **not +sufficient** — it makes the drop deliberate rather than accidental, which is still a drop. + +**Use `useDismissSafeError` (`web/src/hooks.ts`).** It renders the message inline while the surface +is mounted — the better surface, since it keeps the user's context — and hands it to a +caller-supplied `onFailed` once the surface is gone: + +```tsx +const { inlineError, reportFailure, setInlineError } = useDismissSafeError(onFailed); +// ... +} catch (err) { + reportFailure(messageFromXError(err, 'Unable to …')); +} +``` + +The surviving surface belongs to the **parent** and differs per screen — a screen-level +`role="alert"` banner on `CollectionsScreen`, the `notice` + `Toast` pair on `MediaBrowseScreen` and +`SearchScreen` — so `onFailed` is a prop contract, not a rendering decision. There is no global +toast host in this SPA; do not add one for a single screen. + +Note the asymmetry this exists to correct: **success already outlives dismissal** at these sites, +because it is reported through `onAdded` / `onDone`. Failure was the half that died with the +surface. When you add a success callback to a dialog, add its failure counterpart. + +`onFailed` is optional, because not every host screen has a surface to report onto +(`MediaDetailScreen` wires neither callback). Where it is omitted the failure is dropped as before — +a known gap, not coverage (#877). Rationale and the full call-site sweep: +`docs/decisions/records/spa/dismissible-write-failure-reporting.md`. + ## 4. API client modules One file per domain in `web/src/api/`, e.g. `logs.ts`, `blocks.ts`, `playouts.ts`. Pattern (see diff --git a/web/src/hooks.test.tsx b/web/src/hooks.test.tsx index 139de33c2..45e81f7aa 100644 --- a/web/src/hooks.test.tsx +++ b/web/src/hooks.test.tsx @@ -1,7 +1,7 @@ -import { cleanup, render } from '@testing-library/react'; +import { act, cleanup, render } from '@testing-library/react'; import { StrictMode, useEffect } from 'react'; import { afterEach, describe, expect, it } from 'vitest'; -import { useIsMountedRef } from './hooks'; +import { useDismissSafeError, useIsMountedRef } from './hooks'; afterEach(cleanup); @@ -56,3 +56,70 @@ describe('useIsMountedRef (#578)', () => { expect((captured as unknown as { current: boolean }).current).toBe(true); }); }); + +describe('useDismissSafeError (#830)', () => { + interface Handle { + inlineError: string | null; + reportFailure: (message: string) => void; + } + + function renderProbe(report: (message: string) => void) { + const handle: Handle = { inlineError: null, reportFailure: () => {} }; + + function Probe() { + const hook = useDismissSafeError(report); + handle.inlineError = hook.inlineError; + handle.reportFailure = hook.reportFailure; + return {hook.inlineError}; + } + + return { handle, ...render() }; + } + + it('renders the failure INLINE while the surface is still mounted', async () => { + const reported: string[] = []; + const { handle, findByText } = renderProbe((message) => reported.push(message)); + + act(() => handle.reportFailure('nope')); + + // Inline is the better surface while the dialog is up: it keeps the user's context. Diverting + // to the parent here would ALSO be a defect — the message would appear somewhere else while the + // dialog the user is looking at stays blank. + expect(await findByText('nope')).toBeInTheDocument(); + expect(reported).toEqual([]); + }); + + it('diverts the failure to the surviving surface once the surface is gone', () => { + const reported: string[] = []; + const { handle, unmount } = renderProbe((message) => reported.push(message)); + + unmount(); + handle.reportFailure('gone'); + + // This is the #830 defect itself: before the fix the message was dropped here and the user was + // left believing the write succeeded. Delete `reportRef.current(message)` and this goes red. + expect(reported).toEqual(['gone']); + }); + + it('reports through the LATEST callback, not the one captured on first render', () => { + const first: string[] = []; + const second: string[] = []; + + function Probe({ sink }: { sink: (message: string) => void }) { + handle = useDismissSafeError(sink); + return null; + } + + let handle: ReturnType = null as never; + const { rerender, unmount } = render( first.push(m)} />); + rerender( second.push(m)} />); + unmount(); + + handle.reportFailure('latest'); + + // Parents hand down a fresh closure every render (the same reason `useOverlayBehavior` keeps a + // latest-ref). Capturing the first one would report into a stale closure. + expect(first).toEqual([]); + expect(second).toEqual(['latest']); + }); +}); diff --git a/web/src/hooks.ts b/web/src/hooks.ts index 8ed9100d5..f13be5fcd 100644 --- a/web/src/hooks.ts +++ b/web/src/hooks.ts @@ -1,4 +1,4 @@ -import { useEffect, useRef } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; // Shared is-mounted guard for async callbacks (#578). A monotonic `seqRef` drops a STALE response // (an older request resolving after a newer one) but says nothing about whether the component is @@ -19,3 +19,53 @@ export function useIsMountedRef(): { readonly current: boolean } { return ref; } + +// A write failure must land on a surface that OUTLIVES the component that started it (#830). +// +// `Dialog` and `SlideOver` are dismissible mid-request through three paths that do not consult a +// busy flag -- Escape and the backdrop/scrim click via `useOverlayBehavior`, plus the header close +// button (`components/overlay.tsx`). Callers commonly disable only the footer Cancel button, which +// looks like it closes the hole and does not. Dismissal unmounts the surface, so an error rendered +// from ITS OWN state has nowhere to go: the request still runs, still fails, and the user is told +// nothing while the list reloads unchanged. +// +// The asymmetry this fixes: SUCCESS already outlives dismissal at these sites, because it is +// reported through a parent callback (`onAdded`/`onDone`, which the screen turns into a Toast). +// Failure was the half that died with the surface. So the fix is not a new notification system -- +// it is routing failure through the channel success already uses. +// +// While mounted the error stays INLINE, which is the better surface: it keeps the user's selections +// and their context. `reportFailure` only diverts once the surface is gone. +// +// `reportAfterDismiss` is read through a latest-ref for the same reason `useOverlayBehavior` does +// it: parents hand down a fresh closure every render, and the ref must still be callable AFTER this +// component unmounts, which is precisely when it is used. If the parent screen has itself unmounted +// (the user navigated away entirely) the setState inside it is a no-op on React 18 -- there is no +// surface left to report to in that case, so there is nothing better to do. +export function useDismissSafeError(reportAfterDismiss: (message: string) => void): { + readonly inlineError: string | null; + readonly setInlineError: (message: string | null) => void; + readonly reportFailure: (message: string) => void; +} { + const mountedRef = useIsMountedRef(); + const [inlineError, setInlineError] = useState(null); + const reportRef = useRef(reportAfterDismiss); + + useEffect(() => { + reportRef.current = reportAfterDismiss; + }); + + const reportFailure = useCallback( + (message: string) => { + if (mountedRef.current) { + setInlineError(message); + return; + } + + reportRef.current(message); + }, + [mountedRef] + ); + + return { inlineError, reportFailure, setInlineError }; +} diff --git a/web/src/media/addTo/AddToCollectionDialog.tsx b/web/src/media/addTo/AddToCollectionDialog.tsx index 21ffcc22b..03fa0513d 100644 --- a/web/src/media/addTo/AddToCollectionDialog.tsx +++ b/web/src/media/addTo/AddToCollectionDialog.tsx @@ -10,6 +10,7 @@ import { type AddItemsToCollectionRequest, type MediaCollection } from '../../api'; +import { useDismissSafeError } from '../../hooks'; import type { AddToItems } from './scheduleItem'; export interface AddToCollectionDialogProps { @@ -17,6 +18,11 @@ export interface AddToCollectionDialogProps { onClose: () => void; items: AddToItems; onAdded?: (collectionName: string) => void; + // #830: `onFailed` is the SURVIVING surface. A write failure that lands after the dialog was + // dismissed (Escape / backdrop / header X — none of which consult `submitting`) has nowhere to + // render, so it is handed to the caller instead. Optional because not every host screen has a + // surface to show it on; where it is omitted the failure is dropped exactly as it was before. + onFailed?: (message: string) => void; } const NEW_COLLECTION = '__new__'; @@ -33,14 +39,14 @@ export function AddToCollectionDialog(props: AddToCollectionDialogProps) { return ; } -function AddToCollectionDialogBody({ onClose, items, onAdded }: AddToCollectionDialogProps) { +function AddToCollectionDialogBody({ onClose, items, onAdded, onFailed }: AddToCollectionDialogProps) { const [loading, setLoading] = useState(true); const [loadError, setLoadError] = useState(null); const [collections, setCollections] = useState([]); const [selected, setSelected] = useState(NEW_COLLECTION); const [newName, setNewName] = useState(''); const [submitting, setSubmitting] = useState(false); - const [submitError, setSubmitError] = useState(null); + const { inlineError, reportFailure, setInlineError } = useDismissSafeError(onFailed ?? (() => {})); const activeRef = useRef(true); useEffect(() => { @@ -86,7 +92,7 @@ function AddToCollectionDialogBody({ onClose, items, onAdded }: AddToCollectionD } setSubmitting(true); - setSubmitError(null); + setInlineError(null); const request = resolveRequest(items); @@ -111,11 +117,15 @@ function AddToCollectionDialogBody({ onClose, items, onAdded }: AddToCollectionD onClose(); }) .catch((error: unknown) => { + // Reported BEFORE the activeRef gate: swallowing this on dismissal is the #830 defect. + // `reportFailure` renders inline while the dialog is up, and diverts to `onFailed` once + // it is gone. `setSubmitting` stays gated — that state died with the component. + reportFailure(messageFromCollectionError(error, 'Unable to add items')); + if (!activeRef.current) { return; } - setSubmitError(messageFromCollectionError(error, 'Unable to add items')); setSubmitting(false); }); }; @@ -172,9 +182,9 @@ function AddToCollectionDialogBody({ onClose, items, onAdded }: AddToCollectionD value={newName} /> )} - {submitError && ( + {inlineError && ( - {submitError} + {inlineError} )} diff --git a/web/src/media/addTo/AddToMenu.tsx b/web/src/media/addTo/AddToMenu.tsx index fc220c3d4..586e7978c 100644 --- a/web/src/media/addTo/AddToMenu.tsx +++ b/web/src/media/addTo/AddToMenu.tsx @@ -15,13 +15,17 @@ export interface AddToMenuProps { items: LibraryBrowseItem[]; targets?: AddToTarget[]; onDone?: (message: string) => void; + // #830: the failure counterpart of `onDone`. Success already outlives dismissal because it is + // reported through the caller; failure did not, so it died with the dialog. Optional for the + // same reason `onDone` is — MediaDetailScreen wires neither and is unchanged by this. + onFailed?: (message: string) => void; compact?: boolean; } // A small dropdown-button that opens the right Add-to dialog for the given items. Meant to be // dropped onto media cards / detail pages by the wiring screens; it owns the popover + which // dialog is open, and surfaces a success message to the caller via onDone. -export function AddToMenu({ items, targets = DEFAULT_TARGETS, onDone, compact = false }: AddToMenuProps) { +export function AddToMenu({ items, targets = DEFAULT_TARGETS, onDone, onFailed, compact = false }: AddToMenuProps) { const [menuOpen, setMenuOpen] = useState(false); const [dialog, setDialog] = useState(null); const rootRef = useRef(null); @@ -106,13 +110,14 @@ export function AddToMenu({ items, targets = DEFAULT_TARGETS, onDone, compact = )} - handleAdded(`Added to “${name}”`)} onClose={closeDialog} open={dialog === 'collection'} /> - handleAdded(`Added to “${name}”`)} onClose={closeDialog} open={dialog === 'playlist'} /> + handleAdded(`Added to “${name}”`)} onClose={closeDialog} onFailed={onFailed} open={dialog === 'collection'} /> + handleAdded(`Added to “${name}”`)} onClose={closeDialog} onFailed={onFailed} open={dialog === 'playlist'} /> {scheduleItem && ( handleAdded(`Added to “${name}”`)} onClose={closeDialog} + onFailed={onFailed} open={dialog === 'schedule'} /> )} diff --git a/web/src/media/addTo/AddToPlaylistDialog.tsx b/web/src/media/addTo/AddToPlaylistDialog.tsx index fcb0fbc81..3982221fc 100644 --- a/web/src/media/addTo/AddToPlaylistDialog.tsx +++ b/web/src/media/addTo/AddToPlaylistDialog.tsx @@ -11,6 +11,7 @@ import { type Playlist, type PlaylistGroup } from '../../api'; +import { useDismissSafeError } from '../../hooks'; import type { AddToItems } from './scheduleItem'; export interface AddToPlaylistDialogProps { @@ -18,6 +19,10 @@ export interface AddToPlaylistDialogProps { onClose: () => void; items: AddToItems; onAdded?: (playlistName: string) => void; + // #830: the SURVIVING surface. A write failure landing after dismissal (Escape / backdrop / + // header X — none of which consult `submitting`) has nowhere to render, so it is handed to the + // caller. Optional: where a host screen has no surface for it, the failure is dropped as before. + onFailed?: (message: string) => void; } function resolveRequest(items: AddToItems): AddItemsToCollectionRequest { @@ -32,7 +37,7 @@ export function AddToPlaylistDialog(props: AddToPlaylistDialogProps) { return ; } -function AddToPlaylistDialogBody({ onClose, items, onAdded }: AddToPlaylistDialogProps) { +function AddToPlaylistDialogBody({ onClose, items, onAdded, onFailed }: AddToPlaylistDialogProps) { const [loading, setLoading] = useState(true); const [loadError, setLoadError] = useState(null); const [groups, setGroups] = useState([]); @@ -41,7 +46,7 @@ function AddToPlaylistDialogBody({ onClose, items, onAdded }: AddToPlaylistDialo const [playlistsLoading, setPlaylistsLoading] = useState(false); const [selectedPlaylist, setSelectedPlaylist] = useState(''); const [submitting, setSubmitting] = useState(false); - const [submitError, setSubmitError] = useState(null); + const { inlineError, reportFailure, setInlineError } = useDismissSafeError(onFailed ?? (() => {})); const activeRef = useRef(true); const groupSeqRef = useRef(0); @@ -78,10 +83,12 @@ function AddToPlaylistDialogBody({ onClose, items, onAdded }: AddToPlaylistDialo return; } - setSubmitError(messageFromPlaylistError(error, 'Unable to load playlists')); + setInlineError(messageFromPlaylistError(error, 'Unable to load playlists')); setPlaylistsLoading(false); }); - }, []); + // `setInlineError` is a useState setter behind `useDismissSafeError`, so it is referentially + // stable; it is listed because exhaustive-deps cannot see that through a custom hook. + }, [setInlineError]); const load = useCallback(() => { getPlaylistGroups() @@ -116,7 +123,7 @@ function AddToPlaylistDialogBody({ onClose, items, onAdded }: AddToPlaylistDialo const onGroupChange = (value: string) => { setSelectedGroup(value); - setSubmitError(null); + setInlineError(null); loadPlaylists(Number(value)); }; @@ -128,7 +135,7 @@ function AddToPlaylistDialogBody({ onClose, items, onAdded }: AddToPlaylistDialo } setSubmitting(true); - setSubmitError(null); + setInlineError(null); const playlistId = Number(selectedPlaylist); const name = playlists.find((playlist) => String(playlist.id) === selectedPlaylist)?.name ?? 'playlist'; @@ -143,11 +150,15 @@ function AddToPlaylistDialogBody({ onClose, items, onAdded }: AddToPlaylistDialo onClose(); }) .catch((error: unknown) => { + // Reported BEFORE the activeRef gate: swallowing this on dismissal is the #830 defect. + // `reportFailure` renders inline while the dialog is up, and diverts to `onFailed` once it + // is gone. `setSubmitting` stays gated — that state died with the component. + reportFailure(messageFromPlaylistError(error, 'Unable to add items')); + if (!activeRef.current) { return; } - setSubmitError(messageFromPlaylistError(error, 'Unable to add items')); setSubmitting(false); }); }; @@ -204,9 +215,9 @@ function AddToPlaylistDialogBody({ onClose, items, onAdded }: AddToPlaylistDialo } value={selectedPlaylist} /> - {submitError && ( + {inlineError && ( - {submitError} + {inlineError} )} diff --git a/web/src/media/addTo/AddToScheduleDialog.tsx b/web/src/media/addTo/AddToScheduleDialog.tsx index c0cf444c5..f23752e66 100644 --- a/web/src/media/addTo/AddToScheduleDialog.tsx +++ b/web/src/media/addTo/AddToScheduleDialog.tsx @@ -7,6 +7,7 @@ import { type LibraryBrowseMediaType, type ProgramSchedule } from '../../api'; +import { useDismissSafeError } from '../../hooks'; import { ApiError } from '../../api/client'; import { collectionTypeForMediaType, scheduleItemRequestForMediaItem } from './scheduleItem'; @@ -21,6 +22,10 @@ export interface AddToScheduleDialogProps { onClose: () => void; item: AddToScheduleItem; onAdded?: (scheduleName: string) => void; + // #830: the SURVIVING surface. A write failure landing after dismissal (Escape / backdrop / + // header X — none of which consult `submitting`) has nowhere to render, so it is handed to the + // caller. Optional: where a host screen has no surface for it, the failure is dropped as before. + onFailed?: (message: string) => void; } function messageFromError(error: unknown, fallback: string): string { @@ -43,13 +48,13 @@ export function AddToScheduleDialog(props: AddToScheduleDialogProps) { return ; } -function AddToScheduleDialogBody({ onClose, item, onAdded }: AddToScheduleDialogProps) { +function AddToScheduleDialogBody({ onClose, item, onAdded, onFailed }: AddToScheduleDialogProps) { const [loading, setLoading] = useState(true); const [loadError, setLoadError] = useState(null); const [schedules, setSchedules] = useState([]); const [selected, setSelected] = useState(''); const [submitting, setSubmitting] = useState(false); - const [submitError, setSubmitError] = useState(null); + const { inlineError, reportFailure, setInlineError } = useDismissSafeError(onFailed ?? (() => {})); const activeRef = useRef(true); useEffect(() => { @@ -95,7 +100,7 @@ function AddToScheduleDialogBody({ onClose, item, onAdded }: AddToScheduleDialog } setSubmitting(true); - setSubmitError(null); + setInlineError(null); const scheduleId = Number(selected); const name = schedules.find((schedule) => String(schedule.id) === selected)?.name ?? 'schedule'; @@ -111,11 +116,15 @@ function AddToScheduleDialogBody({ onClose, item, onAdded }: AddToScheduleDialog onClose(); }) .catch((error: unknown) => { + // Reported BEFORE the activeRef gate: swallowing this on dismissal is the #830 defect. + // `reportFailure` renders inline while the dialog is up, and diverts to `onFailed` once it + // is gone. `setSubmitting` stays gated — that state died with the component. + reportFailure(messageFromError(error, 'Unable to add to schedule')); + if (!activeRef.current) { return; } - setSubmitError(messageFromError(error, 'Unable to add to schedule')); setSubmitting(false); }); }; @@ -163,9 +172,9 @@ function AddToScheduleDialogBody({ onClose, item, onAdded }: AddToScheduleDialog options={schedules.map((schedule) => ({ value: String(schedule.id), label: schedule.name ?? `Schedule ${schedule.id}` }))} value={selected} /> - {submitError && ( + {inlineError && ( - {submitError} + {inlineError} )} diff --git a/web/src/media/addTo/SaveAsSmartCollectionDialog.tsx b/web/src/media/addTo/SaveAsSmartCollectionDialog.tsx index f6e1c2126..e79917e4d 100644 --- a/web/src/media/addTo/SaveAsSmartCollectionDialog.tsx +++ b/web/src/media/addTo/SaveAsSmartCollectionDialog.tsx @@ -8,6 +8,7 @@ import { updateSmartCollection, type SmartCollection } from '../../api'; +import { useDismissSafeError } from '../../hooks'; export interface SaveAsSmartCollectionDialogProps { open: boolean; @@ -15,6 +16,10 @@ export interface SaveAsSmartCollectionDialogProps { // The search query the saved smart collection will store. query: string; onSaved?: (collectionName: string) => void; + // #830: the SURVIVING surface. A write failure landing after dismissal (Escape / backdrop / + // header X — none of which consult `submitting`) has nowhere to render, so it is handed to the + // caller. Optional: where a host screen has no surface for it, the failure is dropped as before. + onFailed?: (message: string) => void; } const NEW_COLLECTION = '__new__'; @@ -27,14 +32,14 @@ export function SaveAsSmartCollectionDialog(props: SaveAsSmartCollectionDialogPr return ; } -function SaveAsSmartCollectionDialogBody({ onClose, query, onSaved }: SaveAsSmartCollectionDialogProps) { +function SaveAsSmartCollectionDialogBody({ onClose, query, onSaved, onFailed }: SaveAsSmartCollectionDialogProps) { const [loading, setLoading] = useState(true); const [loadError, setLoadError] = useState(null); const [collections, setCollections] = useState([]); const [selected, setSelected] = useState(NEW_COLLECTION); const [newName, setNewName] = useState(''); const [submitting, setSubmitting] = useState(false); - const [submitError, setSubmitError] = useState(null); + const { inlineError, reportFailure, setInlineError } = useDismissSafeError(onFailed ?? (() => {})); const activeRef = useRef(true); useEffect(() => { @@ -80,7 +85,7 @@ function SaveAsSmartCollectionDialogBody({ onClose, query, onSaved }: SaveAsSmar } setSubmitting(true); - setSubmitError(null); + setInlineError(null); // New: create with the query baked in. Existing: overwrite its query (keeping its name). const save: Promise = isNew @@ -103,11 +108,15 @@ function SaveAsSmartCollectionDialogBody({ onClose, query, onSaved }: SaveAsSmar onClose(); }) .catch((error: unknown) => { + // Reported BEFORE the activeRef gate: swallowing this on dismissal is the #830 defect. + // `reportFailure` renders inline while the dialog is up, and diverts to `onFailed` once it + // is gone. `setSubmitting` stays gated — that state died with the component. + reportFailure(messageFromCollectionError(error, 'Unable to save smart collection')); + if (!activeRef.current) { return; } - setSubmitError(messageFromCollectionError(error, 'Unable to save smart collection')); setSubmitting(false); }); }; @@ -167,9 +176,9 @@ function SaveAsSmartCollectionDialogBody({ onClose, query, onSaved }: SaveAsSmar value={newName} /> )} - {submitError && ( + {inlineError && ( - {submitError} + {inlineError} )} diff --git a/web/src/screens/CollectionsScreen.guards.test.tsx b/web/src/screens/CollectionsScreen.guards.test.tsx index 67a9e649d..5d1827d12 100644 --- a/web/src/screens/CollectionsScreen.guards.test.tsx +++ b/web/src/screens/CollectionsScreen.guards.test.tsx @@ -300,11 +300,18 @@ describe('AddItemsDialog async guards (#740)', () => { mountedReads.length = 0; parked[0].fail(); - // `submit` has exactly TWO post-await writes to this component's own state — `setError` in the - // catch and `setAdding` in the finally — and the invariant is that BOTH consult the guard. - // Counting them is what keeps the two from masking each other: asserting only that SOME read - // saw `false` stays green with either one removed, because the survivor still reads the ref. - await waitFor(() => expect(mountedReads).toHaveLength(2)); + // `submit` has exactly ONE post-await write left to state THIS component owns directly — + // `setAdding` in the finally. It was two until #830 moved the catch's report into + // `useDismissSafeError`, which keeps its own is-mounted guard and decides between the inline + // banner and the parent's surviving surface; that guard is invisible here because the mock + // above spreads `...actual`, so the hook's INTERNAL `useIsMountedRef()` call resolves to the + // real one rather than this proxy. Both halves of it are pinned directly by + // `hooks.test.tsx` → "useDismissSafeError (#830)". + // + // The exact count still does the anti-masking work it was added for: asserting only that SOME + // read saw `false` would stay green with the remaining guard deleted, because there would be no + // read left to contradict it. Pinning the number means removing this one gives 0 and reddens. + await waitFor(() => expect(mountedReads).toHaveLength(1)); expect(mountedReads.every((read) => read === false)).toBe(true); }); }); diff --git a/web/src/screens/CollectionsScreen.test.tsx b/web/src/screens/CollectionsScreen.test.tsx index 1d5c35fb1..4a22c2c73 100644 --- a/web/src/screens/CollectionsScreen.test.tsx +++ b/web/src/screens/CollectionsScreen.test.tsx @@ -49,6 +49,9 @@ interface MockOptions { manual?: unknown[]; smart?: unknown[]; onRequest?: (url: string, method: string, body: unknown) => Response | null; + // Returns a promise the TEST settles, so a request can be left in flight while the surface that + // started it is dismissed (#830). `onRequest` cannot express that: it answers synchronously. + onRequestDeferred?: (url: string, method: string, body: unknown) => Promise | null; } function mockApi(options: MockOptions = {}) { @@ -60,6 +63,14 @@ function mockApi(options: MockOptions = {}) { const method = (init?.method ?? 'GET').toUpperCase(); const body = init?.body ? JSON.parse(String(init.body)) : undefined; + if (options.onRequestDeferred) { + const deferred = options.onRequestDeferred(url, method, body); + + if (deferred) { + return deferred; + } + } + if (options.onRequest) { const override = options.onRequest(url, method, body); @@ -360,8 +371,9 @@ describe('CollectionsScreen', () => { RemoteStream: { id: 10, mediaType: 'RemoteStream', title: 'Live Feed' } }; - function mockAddItemsApi() { + function mockAddItemsApi(onRequestDeferred?: MockOptions['onRequestDeferred']) { return mockApi({ + onRequestDeferred, onRequest: (url) => { if (url.startsWith('/api/v1/library/browse')) { const params = new URL(url, 'http://localhost').searchParams; @@ -452,6 +464,61 @@ describe('CollectionsScreen', () => { ).toBeInTheDocument(); }); + // #830. The dialog is dismissible mid-request through three paths that never consult `adding` + // (Escape, backdrop click, header close button — `components/overlay.tsx`); only the footer Cancel + // is disabled, which looks like it closes the hole. Dismissal genuinely unmounts the instance, + // because the caller keys it on `add-${pickerOpen}`. #740 guarded the in-dialog `setError` so the + // drop became deliberate; the user-visible half is that a deliberate drop still reads as success. + // + // The request is left IN FLIGHT across the dismissal on purpose — settling it before the Escape + // would take the still-mounted path and prove nothing about the bug. + it('reports a failed add on the screen when the dialog was dismissed before the request settled (#830)', async () => { + let failAdd: (() => void) | undefined; + const addInFlight = new Promise((resolve) => { + failAdd = () => resolve(new Response(null, { status: 500 })); + }); + + const fetchMock = mockAddItemsApi((url, method) => + url === '/api/v1/collections/1/items' && method === 'POST' ? addInFlight : null + ); + + const dialog = await openAddItemsDialog(); + fireEvent.change(within(dialog).getByPlaceholderText('Search movies, shows, seasons, artists…'), { + target: { value: 'za' } + }); + fireEvent.click(within(dialog).getByRole('button', { name: 'Search' })); + expect(await within(dialog).findByText('Zathura')).toBeInTheDocument(); + + fireEvent.click(within(dialog).getByText('Zathura')); + fireEvent.click(within(dialog).getByRole('button', { name: /Add 1 item/ })); + + // The POST is out, and nothing has answered it yet. + await waitFor(() => { + expect( + fetchMock.mock.calls.some( + ([u, init]) => u === '/api/v1/collections/1/items' && (init?.method ?? '').toUpperCase() === 'POST' + ) + ).toBe(true); + }); + + // Dismiss via Escape — the path the disabled Cancel button does not cover. + fireEvent.keyDown(document, { key: 'Escape' }); + await waitFor(() => { + expect(screen.queryByRole('dialog')).toBeNull(); + }); + + failAdd?.(); + + // The failure lands on the SCREEN's own role="alert" banner, which outlives the dialog. Asserting + // the message rather than merely "an alert exists" keeps this from passing on an unrelated banner. + const alert = await screen.findByText('Request failed with status 500'); + expect(alert).toBeInTheDocument(); + + // ...and it is genuinely on the screen, not inside a dialog that somehow survived. + expect(screen.queryByRole('dialog')).toBeNull(); + expect(alert.closest('[role="dialog"]')).toBeNull(); + }); + it('selecting a newly-addable kind filter (Song) surfaces it and buckets it correctly on add', async () => { const fetchMock = mockAddItemsApi(); diff --git a/web/src/screens/CollectionsScreen.tsx b/web/src/screens/CollectionsScreen.tsx index 3c55c1497..9fbfab410 100644 --- a/web/src/screens/CollectionsScreen.tsx +++ b/web/src/screens/CollectionsScreen.tsx @@ -48,7 +48,7 @@ import { type MediaCollection, type SmartCollection } from '../api'; -import { useIsMountedRef } from '../hooks'; +import { useDismissSafeError, useIsMountedRef } from '../hooks'; import { TYPE_LABEL } from '../media/mediaKinds'; import { SmartCollectionDialog } from '../builder/SmartCollectionDialog'; @@ -224,11 +224,13 @@ function NameDialog({ function AddItemsDialog({ collection, onAdded, + onAddFailed, onClose, open }: { collection: MediaCollection; onAdded: () => void; + onAddFailed: (message: string) => void; onClose: () => void; open: boolean; }) { @@ -242,7 +244,9 @@ function AddItemsDialog({ const [submittedQuery, setSubmittedQuery] = useState(''); const [searching, setSearching] = useState(false); const [selected, setSelected] = useState>(() => new Map()); - const [error, setError] = useState(null); + // #830: `inlineError` renders here while the dialog is up; `reportFailure` diverts to + // `onAddFailed` once it has been dismissed, so a failed add cannot vanish with the surface. + const { inlineError, reportFailure, setInlineError } = useDismissSafeError(onAddFailed); const [adding, setAdding] = useState(false); const [kindFilter, setKindFilter] = useState('all'); const seqRef = useRef(0); @@ -271,7 +275,7 @@ function AddItemsDialog({ const isCurrent = () => mountedRef.current && seqRef.current === seq; setSearching(true); - setError(null); + setInlineError(null); try { const kinds = filter === 'all' ? DEFAULT_SEARCH_KINDS : [filter]; @@ -298,7 +302,7 @@ function AddItemsDialog({ if (!isCurrent()) { return; } - setError(messageFromCollectionError(searchError, 'Unable to search library')); + setInlineError(messageFromCollectionError(searchError, 'Unable to search library')); // A failed search must not leave a stale results/totalMatches pair rendering a confident // "Showing N of M matches" hint beside the error banner (#685 review). setResults([]); @@ -342,7 +346,7 @@ function AddItemsDialog({ } setAdding(true); - setError(null); + setInlineError(null); try { await addItemsToCollection(collection.id, toAddItemsRequest([...selected.values()])); @@ -352,15 +356,15 @@ function AddItemsDialog({ onAdded(); onClose(); } catch (addError) { - // Guarded: these two are this component's own state. The dialog is unmount-reachable while a - // request is in flight even though the Add button is disabled — Escape, a backdrop click and - // the header close button all reach `onClose` without consulting `adding` - // (`components/overlay.tsx`), and the caller remounts on `key={`add-${pickerOpen}`}`. This is - // the is-mounted guard's own trigger, unmount-while-in-flight, and it needs no `seq`: the - // disabled Add button means there is never a second submission to be superseded by. - if (mountedRef.current) { - setError(messageFromCollectionError(addError, 'Unable to add items')); - } + // The dialog is unmount-reachable while a request is in flight even though the Add button is + // disabled — Escape, a backdrop click and the header close button all reach `onClose` without + // consulting `adding` (`components/overlay.tsx`), and the caller remounts on + // `key={`add-${pickerOpen}`}`. #740 guarded this `setError` so the drop was deliberate rather + // than accidental; #830 is the missing half — a deliberate drop still leaves the user believing + // the add succeeded. `reportFailure` renders inline while the dialog is up and hands the + // message to the parent screen once it is gone. No `seq` is needed: the disabled Add button + // means there is never a second submission to supersede this one. + reportFailure(messageFromCollectionError(addError, 'Unable to add items')); } finally { if (mountedRef.current) { setAdding(false); @@ -428,9 +432,9 @@ function AddItemsDialog({ {/* Conditionally mounted, unlike the aria-live hint below — deliberately, not by oversight: `role="alert"` is the one live-region role screen readers reliably announce on INSERTION, so mounting it together with its content is right here and would be wrong there (#685). */} - {error && ( + {inlineError && ( - {error} + {inlineError} )} {submittedQuery !== '' && ( @@ -448,7 +452,7 @@ function AddItemsDialog({ {totalMatches > results.length ? `Showing ${results.length} of ${totalMatches} matches — narrow your search.` : ''}

- {results.length === 0 && submittedQuery === '' && !error ? ( + {results.length === 0 && submittedQuery === '' && !inlineError ? ( // Keyed to submittedQuery, not the live input: without it, backspacing the query back // below the min-query length after a successful search wiped the rendered rows AND their // checkmarks while `selected` (and the Add button's count) still held them (#685) — and, @@ -457,8 +461,8 @@ function AddItemsDialog({
Type at least {LIBRARY_PICKER_MIN_QUERY} characters to search.
- ) : results.length === 0 && !searching && !error ? ( - // Also suppressed on `error`: "No results" asserts a search that COMPLETED and found + ) : results.length === 0 && !searching && !inlineError ? ( + // Also suppressed on `inlineError`: "No results" asserts a search that COMPLETED and found // nothing, which is false when the request failed. The role="alert" banner above is the // whole message in that state (#685).
No results — try a search above.
@@ -821,6 +825,9 @@ function ManualItemsView({ collection={collection} key={`add-${pickerOpen}`} onAdded={load} + // #830: the surviving surface. This screen's own `role="alert"` banner outlives the dialog, + // so a failure that lands after dismissal is still reported where the user actually is. + onAddFailed={setError} onClose={() => setPickerOpen(false)} open={pickerOpen} /> diff --git a/web/src/screens/MediaBrowseScreen.tsx b/web/src/screens/MediaBrowseScreen.tsx index 8770ce5d5..d7a2eca43 100644 --- a/web/src/screens/MediaBrowseScreen.tsx +++ b/web/src/screens/MediaBrowseScreen.tsx @@ -285,12 +285,14 @@ export function MediaBrowseScreen() { items={selectedItems} onAdded={onBulkAdded} onClose={() => setBulkDialog(null)} + onFailed={(message) => setNotice({ tone: 'error', message })} open={bulkDialog === 'collection'} /> setBulkDialog(null)} + onFailed={(message) => setNotice({ tone: 'error', message })} open={bulkDialog === 'playlist'} /> @@ -334,7 +336,12 @@ export function MediaBrowseScreen() { // card navigation stays live. actions={ selectMode || refreshing ? undefined : ( - setNotice({ tone: 'ok', message })} /> + setNotice({ tone: 'ok', message })} + onFailed={(message) => setNotice({ tone: 'error', message })} + /> ) } item={item} diff --git a/web/src/screens/SearchScreen.tsx b/web/src/screens/SearchScreen.tsx index b74f21a7b..88f402d2d 100644 --- a/web/src/screens/SearchScreen.tsx +++ b/web/src/screens/SearchScreen.tsx @@ -387,7 +387,12 @@ export function SearchScreen() { // card navigation stays live. actions={ selectMode || refreshing ? undefined : ( - setNotice({ tone: 'ok', message })} /> + setNotice({ tone: 'ok', message })} + onFailed={(message) => setNotice({ tone: 'error', message })} + /> ) } item={item} @@ -412,12 +417,14 @@ export function SearchScreen() { items={dialog?.kind === 'collection' ? dialog.items : []} onAdded={onAddedToSelectionTarget('collection')} onClose={() => setDialog(null)} + onFailed={(message) => setNotice({ tone: 'error', message })} open={dialog?.kind === 'collection'} /> setDialog(null)} + onFailed={(message) => setNotice({ tone: 'error', message })} open={dialog?.kind === 'playlist'} /> setNotice({ tone: 'error', message })} open={dialog?.kind === 'save-smart'} query={query.trim()} /> From f61764d051b7f6cd8c0f7bc7d0ae643e6981171b Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 20:08:24 +0200 Subject: [PATCH 2/8] =?UTF-8?q?fix(830):=20report=20SUCCESS=20past=20dismi?= =?UTF-8?q?ssal=20too=20=E2=80=94=20the=20review=20measured=20my=20claim?= =?UTF-8?q?=20false?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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) Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY --- docs/decisions/README.md | 2 +- .../dismissible-write-failure-reporting.md | 37 +++++++--- docs/spa-conventions.md | 17 ++++- web/src/hooks.ts | 17 +++-- .../addTo/AddToCollectionDialog.test.tsx | 71 +++++++++++++++++++ web/src/media/addTo/AddToCollectionDialog.tsx | 10 +-- web/src/media/addTo/AddToPlaylistDialog.tsx | 10 +-- web/src/media/addTo/AddToScheduleDialog.tsx | 10 +-- .../addTo/SaveAsSmartCollectionDialog.tsx | 10 +-- 9 files changed, 148 insertions(+), 36 deletions(-) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 837a71fef..aa4e86525 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -195,7 +195,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `spa.collection-custom-order-ui` | Collection custom ordering uses per-row Move up/Move down buttons (not drag) and is offered for any manual collection with custom order enabled, not just movies-only. | 2026-07-09 | [link](records/spa/collection-custom-order-ui.md) | | `spa.datetime-local-input` | The channel-mode date/time input uses a native `` instead of free-text Chronic natural-language parsing. | 2026-07-09 | [link](records/spa/datetime-local-input.md) | | `spa.deco-templates-table` | The deco-templates editor also renders its day/deco assignment as a table, extending (not replacing) the templates-editor-table convention. | 2026-07-09 | [link](records/spa/deco-templates-table.md) | -| `spa.dismissible-write-failure-reporting` | A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. The surviving surface belongs to the PARENT and differs per screen (a screen-level `role="alert"` banner on `CollectionsScreen`, the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen`), so the mechanism is a PROP CONTRACT rather than a rendering decision; there is no global toast host in this SPA and this decision does not add one. `onFailed` is optional because not every host screen has a surface to report onto — where it is omitted the failure is dropped exactly as before, which is a KNOWN remaining gap, not a claim of coverage. | 2026-08-29 | [link](records/spa/dismissible-write-failure-reporting.md) | +| `spa.dismissible-write-failure-reporting` | A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report SUCCESS past dismissal too — `onAdded`/`onClose` belong to the parent, and gating them makes a completed write silent (measured, #830). The surviving surface belongs to the PARENT and differs per screen (a screen-level `role="alert"` banner on `CollectionsScreen`, the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen`), so the mechanism is a PROP CONTRACT rather than a rendering decision; there is no global toast host in this SPA and this decision does not add one. `onFailed` is optional because not every host screen has a surface to report onto — where it is omitted the failure is dropped exactly as before, which is a KNOWN remaining gap, not a claim of coverage. | 2026-08-29 | [link](records/spa/dismissible-write-failure-reporting.md) | | `spa.download-sample-gate` | The SPA disables both Download Media Sample and Download Results while a troubleshooting session is starting/running (Blazor only gated Download Results). | 2026-07-09 | [link](records/spa/download-sample-gate.md) | | `spa.field-progressive-disclosure` | A consequential settings field explains itself through one shared `FieldHelp` icon trigger beside the field name — never the label itself, never a widened `Tooltip` — with the paragraph declared `as const` in the screen's own `FIELD_HELP` record and the panel portalled to `document.body`. | 2026-08-26 | [link](records/spa/field-progressive-disclosure.md) | | `spa.legacy-redirect-matcher` | `LegacyUiRedirects.TryGetRedirect` is a two-tier matcher — an exact `OrdinalIgnoreCase` `Map` (Tier 1) then an ordered segment-template pattern list (Tier 2, first-match-wins) — collision-free by construction, with a guard invariant that no rule may prefix-match `/api`, `/artwork`, `/docs`, `/openapi`, `/iptv`, `/app`, or `/media/sources`. | 2026-07-11 | [link](records/spa/legacy-redirect-matcher.md) | diff --git a/docs/decisions/records/spa/dismissible-write-failure-reporting.md b/docs/decisions/records/spa/dismissible-write-failure-reporting.md index 180a763e4..67193e174 100644 --- a/docs/decisions/records/spa/dismissible-write-failure-reporting.md +++ b/docs/decisions/records/spa/dismissible-write-failure-reporting.md @@ -5,17 +5,25 @@ status: active since: '2026-08-29' supersedes: none superseded-by: none -rule: 'A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. The surviving surface belongs to the PARENT and differs per screen (a screen-level `role="alert"` banner on `CollectionsScreen`, the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen`), so the mechanism is a PROP CONTRACT rather than a rendering decision; there is no global toast host in this SPA and this decision does not add one. `onFailed` is optional because not every host screen has a surface to report onto — where it is omitted the failure is dropped exactly as before, which is a KNOWN remaining gap, not a claim of coverage.' +rule: 'A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report SUCCESS past dismissal too — `onAdded`/`onClose` belong to the parent, and gating them makes a completed write silent (measured, #830). The surviving surface belongs to the PARENT and differs per screen (a screen-level `role="alert"` banner on `CollectionsScreen`, the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen`), so the mechanism is a PROP CONTRACT rather than a rendering decision; there is no global toast host in this SPA and this decision does not add one. `onFailed` is optional because not every host screen has a surface to report onto — where it is omitted the failure is dropped exactly as before, which is a KNOWN remaining gap, not a claim of coverage.' signals: 'failed add silently swallowed · dialog closed mid-request · Escape backdrop close button ignore adding flag · error banner unmounts with the dialog · useDismissSafeError inline vs onFailed · success outlives dismissal but failure does not · onDone has no failure counterpart · paths: `web/src/hooks.ts`, `web/src/screens/CollectionsScreen.tsx`, `web/src/media/addTo/`, `web/src/components/overlay.tsx` · issues: #830, #877, #740, #685' -mechanics: 'Pinned three ways. (1) `web/src/hooks.test.tsx` → "useDismissSafeError (#830)" pins both branches directly: mounted reports INLINE and does not call `onFailed`, unmounted calls `onFailed`, and the report goes through the LATEST callback rather than the one captured on first render. (2) `CollectionsScreen.test.tsx` → "reports a failed add on the screen when the dialog was dismissed before the request settled (#830)" drives the whole path — it parks the POST in flight, dismisses via Escape, then settles the request and asserts the message is on the screen and NOT inside a dialog. Executed: deleting `reportRef.current(message)` alone reddens it on `Unable to find an element with the text: Request failed with status 500`. (3) `CollectionsScreen.guards.test.tsx` counts is-mounted reads and moved from 2 to 1 when the catch''s guard migrated into the hook; its `...actual` module mock cannot see the hook''s internal `useIsMountedRef()`, which is why (1) exists. Executed: removing the surviving `finally` guard takes that count to 0 and reddens.' +mechanics: 'Pinned three ways. (1) `web/src/hooks.test.tsx` → "useDismissSafeError (#830)" pins both branches directly: mounted reports INLINE and does not call `onFailed`, unmounted calls `onFailed`, and the report goes through the LATEST callback rather than the one captured on first render. (2) `CollectionsScreen.test.tsx` → "reports a failed add on the screen when the dialog was dismissed before the request settled (#830)" drives the whole path — it parks the POST in flight, dismisses via Escape, then settles the request and asserts the message is on the screen and NOT inside a dialog. Executed: deleting `reportRef.current(message)` alone reddens it on `Unable to find an element with the text: Request failed with status 500`. (3) `AddToCollectionDialog.test.tsx` → "an outcome that lands after dismissal (#830)" covers the `media/addTo/` half in BOTH directions, which nothing did in the first draft — reverting `reportFailure` to `setInlineError` in all four dialogs left the whole 1276-test suite green. Executed: restoring the `activeRef` gate above `onAdded?.()` reddens the SUCCESS test alone, and disarming `reportRef.current(message)` reddens the FAILURE test alone. The other three `media/addTo/` dialogs are still UNPINNED — they are byte-identical in shape to the covered one, which is an argument for expecting them to behave the same, not evidence that they do. (4) `CollectionsScreen.guards.test.tsx` counts is-mounted reads and moved from 2 to 1 when the catch''s guard migrated into the hook; its `...actual` module mock cannot see the hook''s internal `useIsMountedRef()`, which is why (1) exists. Executed: removing the surviving `finally` guard takes that count to 0 and reddens.' --- -**The asymmetry is the whole finding.** At every one of these sites SUCCESS already outlives -dismissal, because it is reported through a parent callback (`onAdded` / `onDone`, which -`MediaBrowseScreen` and `SearchScreen` turn into a `Toast`). Only FAILURE was rendered from the -dismissible component's own state. So the fix is not a new notification system — it is routing -failure through the channel success already uses. `AddToMenu` had `onDone` and no counterpart at -all, which is why a failed "Add to collection" from a media card reported nowhere. +**BOTH halves of the outcome were dying with the surface, and the first draft of this record got +that wrong.** It claimed SUCCESS already outlived dismissal "at every one of these sites", reasoning +from `AddItemsDialog` — which does report success past dismissal, and says so in a comment. An +adversarial review probed the other four (`web/src/media/addTo/`) instead of reading one and +generalising, and MEASURED `onAdded` called 0 times after dismissal: all four gated +`onAdded?.()` / `onSaved?.()` behind their own `activeRef` alongside the failure path. So +dismiss-then-SUCCEED was equally silent there, and additionally left the caller's selection state +stale, because `SearchScreen`'s `onAddedToSelectionTarget` never ran to clear it. + +That gate is removed here: `onAdded`/`onSaved` and `onClose` belong to the still-mounted PARENT, the +same reasoning `AddItemsDialog` already carried. The record keeps the wrong first version visible +because the failure mode is the point — one site was read, four were assumed, and the claim was +written into three durable artifacts before it was measured. `AddToMenu` had `onDone` and no failure +counterpart at all, which is why a failed "Add to collection" from a media card reported nowhere. **Why the inline branch is kept rather than always reporting to the parent.** While the dialog is up, inline is the better surface: it keeps the user's selections and the context they are looking @@ -33,8 +41,19 @@ guards, and it deliberately did not change the UX — which left the guard makin *deliberate* rather than accidental. A deliberate drop is still a user who believes twelve items were added when none were, and that is the half this record closes. +**Two limits of the mechanism, recorded rather than left to be rediscovered.** (1) `useIsMountedRef` +clears its flag in a PASSIVE effect cleanup, so there is a narrow window in which the DOM node is +detached but the flag still reads true — the message then renders inline into a dead tree instead of +diverting. `useLayoutEffect` would close it deterministically, but that hook is shared by every +async caller in the SPA (#578) and changing its timing is not something to do inside a bug fix; +tracked with the rest in #877. (2) The surviving surfaces are not equivalent: `CollectionsScreen` +reports into `role="alert"`, while `MediaBrowseScreen`/`SearchScreen` use `Toast`, which is +`role="status"` (polite) and a single last-writer-wins slot — so a diverted failure is announced +less assertively and can be overwritten by a later success. Both are still strictly better than the +drop they replace. + **Scope, stated rather than implied.** A sweep of all 68 `Dialog`/`ConfirmDialog`/`SlideOver` call -sites (2026-08-29) found three shapes: (A1) the surface hard-unmounts and takes its own error state +sites (2026-08-29; 67 caller-owned plus `ConfirmDialog`'s own internal ` void): { readonly inlineError: string | null; readonly setInlineError: (message: string | null) => void; diff --git a/web/src/media/addTo/AddToCollectionDialog.test.tsx b/web/src/media/addTo/AddToCollectionDialog.test.tsx index 888be9c9f..e355af3a4 100644 --- a/web/src/media/addTo/AddToCollectionDialog.test.tsx +++ b/web/src/media/addTo/AddToCollectionDialog.test.tsx @@ -126,4 +126,75 @@ describe('AddToCollectionDialog', () => { const addCall = calls.find((call) => call.url === '/api/v1/collections/1/items'); expect(addCall?.body).toEqual(override); }); + // #830. This dialog is unmounted the moment it is dismissed — `AddToCollectionDialog` early-returns + // on `!props.open`, so `...Body` and every piece of state it owns go away — and the write keeps + // running. Both directions of the outcome have to reach the surviving caller, or the user is told + // nothing about a request they started. + // + // These two are the ONLY coverage of the `media/addTo/` half of #830: reverting `reportFailure` + // back to `setInlineError` in all four dialogs left the whole 1276-test suite green before they + // existed. + describe('an outcome that lands after dismissal (#830)', () => { + function mockDeferredAdd() { + let settle: ((response: Response) => void) | undefined; + const inFlight = new Promise((resolve) => { + settle = resolve; + }); + + const fetchMock = vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? 'GET').toUpperCase(); + + if (url === '/api/v1/collections' && method === 'GET') { + return Promise.resolve(jsonResponse(collections)); + } + + if (url === '/api/v1/collections/1/items' && method === 'POST') { + return inFlight; + } + + return Promise.resolve(new Response(null, { status: 204 })); + }); + + return { fetchMock, settle: settle as (response: Response) => void }; + } + + async function submitThenDismiss(props: Record) { + const deferred = mockDeferredAdd(); + const view = render( {}} open {...props} />); + + await waitFor(() => expect(screen.getByRole('option', { name: 'Favorites' })).toBeTruthy()); + fireEvent.click(screen.getByRole('button', { name: /Add to collection/ })); + await waitFor(() => expect(deferred.fetchMock.mock.calls.some(([u]) => u === '/api/v1/collections/1/items')).toBe(true)); + + // Dismissal, for real: the wrapper early-returns on `!open`, so the body unmounts. + view.rerender( {}} open={false} {...props} />); + expect(screen.queryByRole('dialog')).toBeNull(); + + return deferred; + } + + it('a FAILURE reaches onFailed', async () => { + const onFailed = vi.fn(); + const deferred = await submitThenDismiss({ onFailed }); + + deferred.settle(new Response(null, { status: 500 })); + + // Delete `reportRef.current(message)` from useDismissSafeError and this stays at 0 calls. + await waitFor(() => expect(onFailed).toHaveBeenCalledTimes(1)); + expect(String(onFailed.mock.calls[0][0])).toContain('500'); + }); + + it('a SUCCESS reaches onAdded', async () => { + const onAdded = vi.fn(); + const deferred = await submitThenDismiss({ onAdded }); + + deferred.settle(new Response(null, { status: 204 })); + + // Restore the `if (!activeRef.current) return;` gate above `onAdded?.()` and this stays at 0 — + // the write succeeded, the caller never heard, and its selection state stayed stale. + await waitFor(() => expect(onAdded).toHaveBeenCalledTimes(1)); + expect(onAdded).toHaveBeenCalledWith('Favorites'); + }); + }); }); diff --git a/web/src/media/addTo/AddToCollectionDialog.tsx b/web/src/media/addTo/AddToCollectionDialog.tsx index 03fa0513d..0ba56e300 100644 --- a/web/src/media/addTo/AddToCollectionDialog.tsx +++ b/web/src/media/addTo/AddToCollectionDialog.tsx @@ -109,10 +109,12 @@ function AddToCollectionDialogBody({ onClose, items, onAdded, onFailed }: AddToC ensureCollection .then((collection) => addItemsToCollection(collection.id, request).then(() => collection)) .then((collection) => { - if (!activeRef.current) { - return; - } - + // NOT guarded: both belong to the still-mounted PARENT (#830). `activeRef` gated these + // until 2026-08-29, so dismissing mid-request while the write SUCCEEDED reported nothing and + // left the caller's selection state stale — the same silent-outcome defect as the failure + // half, in the other direction. Measured on `AddToCollectionDialog`: with the gate in place + // `onAdded` is called 0 times after dismissal. `AddItemsDialog` never had the gate here and + // carries the same reasoning. onAdded?.(collection.name); onClose(); }) diff --git a/web/src/media/addTo/AddToPlaylistDialog.tsx b/web/src/media/addTo/AddToPlaylistDialog.tsx index 3982221fc..0a5be0698 100644 --- a/web/src/media/addTo/AddToPlaylistDialog.tsx +++ b/web/src/media/addTo/AddToPlaylistDialog.tsx @@ -142,10 +142,12 @@ function AddToPlaylistDialogBody({ onClose, items, onAdded, onFailed }: AddToPla addItemsToPlaylist(playlistId, resolveRequest(items)) .then(() => { - if (!activeRef.current) { - return; - } - + // NOT guarded: both belong to the still-mounted PARENT (#830). `activeRef` gated these + // until 2026-08-29, so dismissing mid-request while the write SUCCEEDED reported nothing and + // left the caller's selection state stale — the same silent-outcome defect as the failure + // half, in the other direction. Measured on `AddToCollectionDialog`: with the gate in place + // `onAdded` is called 0 times after dismissal. `AddItemsDialog` never had the gate here and + // carries the same reasoning. onAdded?.(name); onClose(); }) diff --git a/web/src/media/addTo/AddToScheduleDialog.tsx b/web/src/media/addTo/AddToScheduleDialog.tsx index f23752e66..333ade729 100644 --- a/web/src/media/addTo/AddToScheduleDialog.tsx +++ b/web/src/media/addTo/AddToScheduleDialog.tsx @@ -108,10 +108,12 @@ function AddToScheduleDialogBody({ onClose, item, onAdded, onFailed }: AddToSche addScheduleItem(scheduleId, payload) .then(() => { - if (!activeRef.current) { - return; - } - + // NOT guarded: both belong to the still-mounted PARENT (#830). `activeRef` gated these + // until 2026-08-29, so dismissing mid-request while the write SUCCEEDED reported nothing and + // left the caller's selection state stale — the same silent-outcome defect as the failure + // half, in the other direction. Measured on `AddToCollectionDialog`: with the gate in place + // `onAdded` is called 0 times after dismissal. `AddItemsDialog` never had the gate here and + // carries the same reasoning. onAdded?.(name); onClose(); }) diff --git a/web/src/media/addTo/SaveAsSmartCollectionDialog.tsx b/web/src/media/addTo/SaveAsSmartCollectionDialog.tsx index e79917e4d..b1e5ab21a 100644 --- a/web/src/media/addTo/SaveAsSmartCollectionDialog.tsx +++ b/web/src/media/addTo/SaveAsSmartCollectionDialog.tsx @@ -100,10 +100,12 @@ function SaveAsSmartCollectionDialogBody({ onClose, query, onSaved, onFailed }: save .then((name) => { - if (!activeRef.current) { - return; - } - + // NOT guarded: both belong to the still-mounted PARENT (#830). `activeRef` gated these + // until 2026-08-29, so dismissing mid-request while the write SUCCEEDED reported nothing and + // left the caller's selection state stale — the same silent-outcome defect as the failure + // half, in the other direction. Measured on `AddToCollectionDialog`: with the gate in place + // `onAdded` is called 0 times after dismissal. `AddItemsDialog` never had the gate here and + // carries the same reasoning. onSaved?.(name); onClose(); }) From de9432ff49d203ca79f4351655f66232b1201b34 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 20:34:40 +0200 Subject: [PATCH 3/8] =?UTF-8?q?fix(830):=20withdraw=20the=20media/addTo=20?= =?UTF-8?q?extension=20=E2=80=94=20two=20rounds,=20two=20defects,=20one=20?= =?UTF-8?q?coupled=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY --- docs/decisions/README.md | 2 +- .../dismissible-write-failure-reporting.md | 43 ++++++----- docs/spa-conventions.md | 49 +++++++------ .../addTo/AddToCollectionDialog.test.tsx | 71 ------------------- web/src/media/addTo/AddToCollectionDialog.tsx | 40 ++++------- web/src/media/addTo/AddToMenu.tsx | 11 +-- web/src/media/addTo/AddToPlaylistDialog.tsx | 47 +++++------- web/src/media/addTo/AddToScheduleDialog.tsx | 39 ++++------ .../addTo/SaveAsSmartCollectionDialog.tsx | 39 ++++------ web/src/screens/MediaBrowseScreen.tsx | 9 +-- web/src/screens/SearchScreen.tsx | 10 +-- 11 files changed, 120 insertions(+), 240 deletions(-) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index aa4e86525..f6cc6b256 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -195,7 +195,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `spa.collection-custom-order-ui` | Collection custom ordering uses per-row Move up/Move down buttons (not drag) and is offered for any manual collection with custom order enabled, not just movies-only. | 2026-07-09 | [link](records/spa/collection-custom-order-ui.md) | | `spa.datetime-local-input` | The channel-mode date/time input uses a native `` instead of free-text Chronic natural-language parsing. | 2026-07-09 | [link](records/spa/datetime-local-input.md) | | `spa.deco-templates-table` | The deco-templates editor also renders its day/deco assignment as a table, extending (not replacing) the templates-editor-table convention. | 2026-07-09 | [link](records/spa/deco-templates-table.md) | -| `spa.dismissible-write-failure-reporting` | A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report SUCCESS past dismissal too — `onAdded`/`onClose` belong to the parent, and gating them makes a completed write silent (measured, #830). The surviving surface belongs to the PARENT and differs per screen (a screen-level `role="alert"` banner on `CollectionsScreen`, the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen`), so the mechanism is a PROP CONTRACT rather than a rendering decision; there is no global toast host in this SPA and this decision does not add one. `onFailed` is optional because not every host screen has a surface to report onto — where it is omitted the failure is dropped exactly as before, which is a KNOWN remaining gap, not a claim of coverage. | 2026-08-29 | [link](records/spa/dismissible-write-failure-reporting.md) | +| `spa.dismissible-write-failure-reporting` | A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report the OUTCOME past dismissal in both directions — a success callback gated on an is-mounted check makes a completed write silent too (measured on `AddToCollectionDialog`, #877) — but gate the DISMISS request (`onClose`) separately, because closing a surface that is no longer yours closes whatever replaced it. The surviving surface belongs to the PARENT and differs per screen (a screen-level `role="alert"` banner on `CollectionsScreen`, the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen`), so the mechanism is a PROP CONTRACT rather than a rendering decision; there is no global toast host in this SPA and this decision does not add one. The reporting prop is REQUIRED where the host has a surface (`AddItemsDialog.onAddFailed`), so a failure cannot be dropped by forgetting to wire it; it is optional only where some host genuinely has nowhere to report, and there an omitted callback drops the failure exactly as before — a KNOWN remaining gap, not a claim of coverage. | 2026-08-29 | [link](records/spa/dismissible-write-failure-reporting.md) | | `spa.download-sample-gate` | The SPA disables both Download Media Sample and Download Results while a troubleshooting session is starting/running (Blazor only gated Download Results). | 2026-07-09 | [link](records/spa/download-sample-gate.md) | | `spa.field-progressive-disclosure` | A consequential settings field explains itself through one shared `FieldHelp` icon trigger beside the field name — never the label itself, never a widened `Tooltip` — with the paragraph declared `as const` in the screen's own `FIELD_HELP` record and the panel portalled to `document.body`. | 2026-08-26 | [link](records/spa/field-progressive-disclosure.md) | | `spa.legacy-redirect-matcher` | `LegacyUiRedirects.TryGetRedirect` is a two-tier matcher — an exact `OrdinalIgnoreCase` `Map` (Tier 1) then an ordered segment-template pattern list (Tier 2, first-match-wins) — collision-free by construction, with a guard invariant that no rule may prefix-match `/api`, `/artwork`, `/docs`, `/openapi`, `/iptv`, `/app`, or `/media/sources`. | 2026-07-11 | [link](records/spa/legacy-redirect-matcher.md) | diff --git a/docs/decisions/records/spa/dismissible-write-failure-reporting.md b/docs/decisions/records/spa/dismissible-write-failure-reporting.md index 67193e174..631c7795e 100644 --- a/docs/decisions/records/spa/dismissible-write-failure-reporting.md +++ b/docs/decisions/records/spa/dismissible-write-failure-reporting.md @@ -5,25 +5,34 @@ status: active since: '2026-08-29' supersedes: none superseded-by: none -rule: 'A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report SUCCESS past dismissal too — `onAdded`/`onClose` belong to the parent, and gating them makes a completed write silent (measured, #830). The surviving surface belongs to the PARENT and differs per screen (a screen-level `role="alert"` banner on `CollectionsScreen`, the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen`), so the mechanism is a PROP CONTRACT rather than a rendering decision; there is no global toast host in this SPA and this decision does not add one. `onFailed` is optional because not every host screen has a surface to report onto — where it is omitted the failure is dropped exactly as before, which is a KNOWN remaining gap, not a claim of coverage.' +rule: 'A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report the OUTCOME past dismissal in both directions — a success callback gated on an is-mounted check makes a completed write silent too (measured on `AddToCollectionDialog`, #877) — but gate the DISMISS request (`onClose`) separately, because closing a surface that is no longer yours closes whatever replaced it. The surviving surface belongs to the PARENT and differs per screen (a screen-level `role="alert"` banner on `CollectionsScreen`, the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen`), so the mechanism is a PROP CONTRACT rather than a rendering decision; there is no global toast host in this SPA and this decision does not add one. The reporting prop is REQUIRED where the host has a surface (`AddItemsDialog.onAddFailed`), so a failure cannot be dropped by forgetting to wire it; it is optional only where some host genuinely has nowhere to report, and there an omitted callback drops the failure exactly as before — a KNOWN remaining gap, not a claim of coverage.' signals: 'failed add silently swallowed · dialog closed mid-request · Escape backdrop close button ignore adding flag · error banner unmounts with the dialog · useDismissSafeError inline vs onFailed · success outlives dismissal but failure does not · onDone has no failure counterpart · paths: `web/src/hooks.ts`, `web/src/screens/CollectionsScreen.tsx`, `web/src/media/addTo/`, `web/src/components/overlay.tsx` · issues: #830, #877, #740, #685' -mechanics: 'Pinned three ways. (1) `web/src/hooks.test.tsx` → "useDismissSafeError (#830)" pins both branches directly: mounted reports INLINE and does not call `onFailed`, unmounted calls `onFailed`, and the report goes through the LATEST callback rather than the one captured on first render. (2) `CollectionsScreen.test.tsx` → "reports a failed add on the screen when the dialog was dismissed before the request settled (#830)" drives the whole path — it parks the POST in flight, dismisses via Escape, then settles the request and asserts the message is on the screen and NOT inside a dialog. Executed: deleting `reportRef.current(message)` alone reddens it on `Unable to find an element with the text: Request failed with status 500`. (3) `AddToCollectionDialog.test.tsx` → "an outcome that lands after dismissal (#830)" covers the `media/addTo/` half in BOTH directions, which nothing did in the first draft — reverting `reportFailure` to `setInlineError` in all four dialogs left the whole 1276-test suite green. Executed: restoring the `activeRef` gate above `onAdded?.()` reddens the SUCCESS test alone, and disarming `reportRef.current(message)` reddens the FAILURE test alone. The other three `media/addTo/` dialogs are still UNPINNED — they are byte-identical in shape to the covered one, which is an argument for expecting them to behave the same, not evidence that they do. (4) `CollectionsScreen.guards.test.tsx` counts is-mounted reads and moved from 2 to 1 when the catch''s guard migrated into the hook; its `...actual` module mock cannot see the hook''s internal `useIsMountedRef()`, which is why (1) exists. Executed: removing the surviving `finally` guard takes that count to 0 and reddens.' +mechanics: 'Pinned three ways. (1) `web/src/hooks.test.tsx` → "useDismissSafeError (#830)" pins both branches directly: mounted reports INLINE and does not call `onFailed`, unmounted calls `onFailed`, and the report goes through the LATEST callback rather than the one captured on first render. (2) `CollectionsScreen.test.tsx` → "reports a failed add on the screen when the dialog was dismissed before the request settled (#830)" drives the whole path — it parks the POST in flight, dismisses via Escape, then settles the request and asserts the message is on the screen and NOT inside a dialog. Executed: deleting `reportRef.current(message)` alone reddens it on `Unable to find an element with the text: Request failed with status 500`. That one clause is shared, so the same mutation reddens THREE tests (this one plus the two divert tests in (1)) — expect three reds, not one, when re-running it. (3) `CollectionsScreen.guards.test.tsx` counts is-mounted reads and moved from 2 to 1 when the catch''s guard migrated into the hook; its `...actual` module mock cannot see the hook''s internal `useIsMountedRef()`, which is why (1) exists. Executed: removing the surviving `finally` guard takes that count to 0 and reddens.' --- -**BOTH halves of the outcome were dying with the surface, and the first draft of this record got -that wrong.** It claimed SUCCESS already outlived dismissal "at every one of these sites", reasoning -from `AddItemsDialog` — which does report success past dismissal, and says so in a comment. An -adversarial review probed the other four (`web/src/media/addTo/`) instead of reading one and -generalising, and MEASURED `onAdded` called 0 times after dismissal: all four gated -`onAdded?.()` / `onSaved?.()` behind their own `activeRef` alongside the failure path. So -dismiss-then-SUCCEED was equally silent there, and additionally left the caller's selection state -stale, because `SearchScreen`'s `onAddedToSelectionTarget` never ran to clear it. +**This record applies to ONE site, and the reason the rest were dropped is the useful part.** +An earlier draft claimed SUCCESS already outlives dismissal "at every one of these sites", reasoning +from `AddItemsDialog` — which does report success past dismissal, and says so in a comment — and +generalising to the four `web/src/media/addTo/` dialogs without reading them. Adversarial review +probed `AddToCollectionDialog` and MEASURED `onAdded` called 0 times after dismissal; the other +three carry a visibly identical gate (read, not probed). So those four gate BOTH halves of the +outcome behind their own `activeRef`, and dismiss-then-succeed is as silent there as +dismiss-then-fail. -That gate is removed here: `onAdded`/`onSaved` and `onClose` belong to the still-mounted PARENT, the -same reasoning `AddItemsDialog` already carried. The record keeps the wrong first version visible -because the failure mode is the point — one site was read, four were assumed, and the claim was -written into three durable artifacts before it was measured. `AddToMenu` had `onDone` and no failure -counterpart at all, which is why a failed "Add to collection" from a media card reported nowhere. +Extending the mechanism to them was attempted and WITHDRAWN, which is why they are #877 and not this +record. Removing that gate reports success correctly but also un-gates `onClose()`, and the two mean +different things: `onAdded` is "tell the parent what happened", `onClose` is "close me" — addressed +to a surface that no longer exists. Measured against the real `AddToMenu`: a late success from a +DISMISSED dialog closed a dialog the user had since reopened, and on `SearchScreen` / +`MediaBrowseScreen` the parents' success handlers also run `clearSelection()`, wiping a multi-select +the user had rebuilt. Un-gating both is wrong, gating both is wrong, and gating only `onClose` still +needs the parents to stop nulling the dialog themselves — three coupled edits across five files, +plus an unresolved question about whether `clearSelection()` should fire for a write the user walked +away from. That is a design decision, not a bug fix, so it goes to #877 with the measurements +attached rather than riding along here. + +`AddToMenu` also has `onDone` and no failure counterpart at all, which is why a failed "Add to +collection" from a media card reports nowhere. Same issue. **Why the inline branch is kept rather than always reporting to the parent.** While the dialog is up, inline is the better surface: it keeps the user's selections and the context they are looking @@ -54,11 +63,11 @@ drop they replace. **Scope, stated rather than implied.** A sweep of all 68 `Dialog`/`ConfirmDialog`/`SlideOver` call sites (2026-08-29; 67 caller-owned plus `ConfirmDialog`'s own internal ` { const addCall = calls.find((call) => call.url === '/api/v1/collections/1/items'); expect(addCall?.body).toEqual(override); }); - // #830. This dialog is unmounted the moment it is dismissed — `AddToCollectionDialog` early-returns - // on `!props.open`, so `...Body` and every piece of state it owns go away — and the write keeps - // running. Both directions of the outcome have to reach the surviving caller, or the user is told - // nothing about a request they started. - // - // These two are the ONLY coverage of the `media/addTo/` half of #830: reverting `reportFailure` - // back to `setInlineError` in all four dialogs left the whole 1276-test suite green before they - // existed. - describe('an outcome that lands after dismissal (#830)', () => { - function mockDeferredAdd() { - let settle: ((response: Response) => void) | undefined; - const inFlight = new Promise((resolve) => { - settle = resolve; - }); - - const fetchMock = vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL, init?: RequestInit) => { - const url = input.toString(); - const method = (init?.method ?? 'GET').toUpperCase(); - - if (url === '/api/v1/collections' && method === 'GET') { - return Promise.resolve(jsonResponse(collections)); - } - - if (url === '/api/v1/collections/1/items' && method === 'POST') { - return inFlight; - } - - return Promise.resolve(new Response(null, { status: 204 })); - }); - - return { fetchMock, settle: settle as (response: Response) => void }; - } - - async function submitThenDismiss(props: Record) { - const deferred = mockDeferredAdd(); - const view = render( {}} open {...props} />); - - await waitFor(() => expect(screen.getByRole('option', { name: 'Favorites' })).toBeTruthy()); - fireEvent.click(screen.getByRole('button', { name: /Add to collection/ })); - await waitFor(() => expect(deferred.fetchMock.mock.calls.some(([u]) => u === '/api/v1/collections/1/items')).toBe(true)); - - // Dismissal, for real: the wrapper early-returns on `!open`, so the body unmounts. - view.rerender( {}} open={false} {...props} />); - expect(screen.queryByRole('dialog')).toBeNull(); - - return deferred; - } - - it('a FAILURE reaches onFailed', async () => { - const onFailed = vi.fn(); - const deferred = await submitThenDismiss({ onFailed }); - - deferred.settle(new Response(null, { status: 500 })); - - // Delete `reportRef.current(message)` from useDismissSafeError and this stays at 0 calls. - await waitFor(() => expect(onFailed).toHaveBeenCalledTimes(1)); - expect(String(onFailed.mock.calls[0][0])).toContain('500'); - }); - - it('a SUCCESS reaches onAdded', async () => { - const onAdded = vi.fn(); - const deferred = await submitThenDismiss({ onAdded }); - - deferred.settle(new Response(null, { status: 204 })); - - // Restore the `if (!activeRef.current) return;` gate above `onAdded?.()` and this stays at 0 — - // the write succeeded, the caller never heard, and its selection state stayed stale. - await waitFor(() => expect(onAdded).toHaveBeenCalledTimes(1)); - expect(onAdded).toHaveBeenCalledWith('Favorites'); - }); - }); }); diff --git a/web/src/media/addTo/AddToCollectionDialog.tsx b/web/src/media/addTo/AddToCollectionDialog.tsx index 0ba56e300..21ffcc22b 100644 --- a/web/src/media/addTo/AddToCollectionDialog.tsx +++ b/web/src/media/addTo/AddToCollectionDialog.tsx @@ -10,7 +10,6 @@ import { type AddItemsToCollectionRequest, type MediaCollection } from '../../api'; -import { useDismissSafeError } from '../../hooks'; import type { AddToItems } from './scheduleItem'; export interface AddToCollectionDialogProps { @@ -18,11 +17,6 @@ export interface AddToCollectionDialogProps { onClose: () => void; items: AddToItems; onAdded?: (collectionName: string) => void; - // #830: `onFailed` is the SURVIVING surface. A write failure that lands after the dialog was - // dismissed (Escape / backdrop / header X — none of which consult `submitting`) has nowhere to - // render, so it is handed to the caller instead. Optional because not every host screen has a - // surface to show it on; where it is omitted the failure is dropped exactly as it was before. - onFailed?: (message: string) => void; } const NEW_COLLECTION = '__new__'; @@ -39,14 +33,14 @@ export function AddToCollectionDialog(props: AddToCollectionDialogProps) { return ; } -function AddToCollectionDialogBody({ onClose, items, onAdded, onFailed }: AddToCollectionDialogProps) { +function AddToCollectionDialogBody({ onClose, items, onAdded }: AddToCollectionDialogProps) { const [loading, setLoading] = useState(true); const [loadError, setLoadError] = useState(null); const [collections, setCollections] = useState([]); const [selected, setSelected] = useState(NEW_COLLECTION); const [newName, setNewName] = useState(''); const [submitting, setSubmitting] = useState(false); - const { inlineError, reportFailure, setInlineError } = useDismissSafeError(onFailed ?? (() => {})); + const [submitError, setSubmitError] = useState(null); const activeRef = useRef(true); useEffect(() => { @@ -92,7 +86,7 @@ function AddToCollectionDialogBody({ onClose, items, onAdded, onFailed }: AddToC } setSubmitting(true); - setInlineError(null); + setSubmitError(null); const request = resolveRequest(items); @@ -109,25 +103,19 @@ function AddToCollectionDialogBody({ onClose, items, onAdded, onFailed }: AddToC ensureCollection .then((collection) => addItemsToCollection(collection.id, request).then(() => collection)) .then((collection) => { - // NOT guarded: both belong to the still-mounted PARENT (#830). `activeRef` gated these - // until 2026-08-29, so dismissing mid-request while the write SUCCEEDED reported nothing and - // left the caller's selection state stale — the same silent-outcome defect as the failure - // half, in the other direction. Measured on `AddToCollectionDialog`: with the gate in place - // `onAdded` is called 0 times after dismissal. `AddItemsDialog` never had the gate here and - // carries the same reasoning. - onAdded?.(collection.name); - onClose(); - }) - .catch((error: unknown) => { - // Reported BEFORE the activeRef gate: swallowing this on dismissal is the #830 defect. - // `reportFailure` renders inline while the dialog is up, and diverts to `onFailed` once - // it is gone. `setSubmitting` stays gated — that state died with the component. - reportFailure(messageFromCollectionError(error, 'Unable to add items')); - if (!activeRef.current) { return; } + onAdded?.(collection.name); + onClose(); + }) + .catch((error: unknown) => { + if (!activeRef.current) { + return; + } + + setSubmitError(messageFromCollectionError(error, 'Unable to add items')); setSubmitting(false); }); }; @@ -184,9 +172,9 @@ function AddToCollectionDialogBody({ onClose, items, onAdded, onFailed }: AddToC value={newName} /> )} - {inlineError && ( + {submitError && ( - {inlineError} + {submitError} )}
diff --git a/web/src/media/addTo/AddToMenu.tsx b/web/src/media/addTo/AddToMenu.tsx index 586e7978c..fc220c3d4 100644 --- a/web/src/media/addTo/AddToMenu.tsx +++ b/web/src/media/addTo/AddToMenu.tsx @@ -15,17 +15,13 @@ export interface AddToMenuProps { items: LibraryBrowseItem[]; targets?: AddToTarget[]; onDone?: (message: string) => void; - // #830: the failure counterpart of `onDone`. Success already outlives dismissal because it is - // reported through the caller; failure did not, so it died with the dialog. Optional for the - // same reason `onDone` is — MediaDetailScreen wires neither and is unchanged by this. - onFailed?: (message: string) => void; compact?: boolean; } // A small dropdown-button that opens the right Add-to dialog for the given items. Meant to be // dropped onto media cards / detail pages by the wiring screens; it owns the popover + which // dialog is open, and surfaces a success message to the caller via onDone. -export function AddToMenu({ items, targets = DEFAULT_TARGETS, onDone, onFailed, compact = false }: AddToMenuProps) { +export function AddToMenu({ items, targets = DEFAULT_TARGETS, onDone, compact = false }: AddToMenuProps) { const [menuOpen, setMenuOpen] = useState(false); const [dialog, setDialog] = useState(null); const rootRef = useRef(null); @@ -110,14 +106,13 @@ export function AddToMenu({ items, targets = DEFAULT_TARGETS, onDone, onFailed, )} - handleAdded(`Added to “${name}”`)} onClose={closeDialog} onFailed={onFailed} open={dialog === 'collection'} /> - handleAdded(`Added to “${name}”`)} onClose={closeDialog} onFailed={onFailed} open={dialog === 'playlist'} /> + handleAdded(`Added to “${name}”`)} onClose={closeDialog} open={dialog === 'collection'} /> + handleAdded(`Added to “${name}”`)} onClose={closeDialog} open={dialog === 'playlist'} /> {scheduleItem && ( handleAdded(`Added to “${name}”`)} onClose={closeDialog} - onFailed={onFailed} open={dialog === 'schedule'} /> )} diff --git a/web/src/media/addTo/AddToPlaylistDialog.tsx b/web/src/media/addTo/AddToPlaylistDialog.tsx index 0a5be0698..fcb0fbc81 100644 --- a/web/src/media/addTo/AddToPlaylistDialog.tsx +++ b/web/src/media/addTo/AddToPlaylistDialog.tsx @@ -11,7 +11,6 @@ import { type Playlist, type PlaylistGroup } from '../../api'; -import { useDismissSafeError } from '../../hooks'; import type { AddToItems } from './scheduleItem'; export interface AddToPlaylistDialogProps { @@ -19,10 +18,6 @@ export interface AddToPlaylistDialogProps { onClose: () => void; items: AddToItems; onAdded?: (playlistName: string) => void; - // #830: the SURVIVING surface. A write failure landing after dismissal (Escape / backdrop / - // header X — none of which consult `submitting`) has nowhere to render, so it is handed to the - // caller. Optional: where a host screen has no surface for it, the failure is dropped as before. - onFailed?: (message: string) => void; } function resolveRequest(items: AddToItems): AddItemsToCollectionRequest { @@ -37,7 +32,7 @@ export function AddToPlaylistDialog(props: AddToPlaylistDialogProps) { return ; } -function AddToPlaylistDialogBody({ onClose, items, onAdded, onFailed }: AddToPlaylistDialogProps) { +function AddToPlaylistDialogBody({ onClose, items, onAdded }: AddToPlaylistDialogProps) { const [loading, setLoading] = useState(true); const [loadError, setLoadError] = useState(null); const [groups, setGroups] = useState([]); @@ -46,7 +41,7 @@ function AddToPlaylistDialogBody({ onClose, items, onAdded, onFailed }: AddToPla const [playlistsLoading, setPlaylistsLoading] = useState(false); const [selectedPlaylist, setSelectedPlaylist] = useState(''); const [submitting, setSubmitting] = useState(false); - const { inlineError, reportFailure, setInlineError } = useDismissSafeError(onFailed ?? (() => {})); + const [submitError, setSubmitError] = useState(null); const activeRef = useRef(true); const groupSeqRef = useRef(0); @@ -83,12 +78,10 @@ function AddToPlaylistDialogBody({ onClose, items, onAdded, onFailed }: AddToPla return; } - setInlineError(messageFromPlaylistError(error, 'Unable to load playlists')); + setSubmitError(messageFromPlaylistError(error, 'Unable to load playlists')); setPlaylistsLoading(false); }); - // `setInlineError` is a useState setter behind `useDismissSafeError`, so it is referentially - // stable; it is listed because exhaustive-deps cannot see that through a custom hook. - }, [setInlineError]); + }, []); const load = useCallback(() => { getPlaylistGroups() @@ -123,7 +116,7 @@ function AddToPlaylistDialogBody({ onClose, items, onAdded, onFailed }: AddToPla const onGroupChange = (value: string) => { setSelectedGroup(value); - setInlineError(null); + setSubmitError(null); loadPlaylists(Number(value)); }; @@ -135,32 +128,26 @@ function AddToPlaylistDialogBody({ onClose, items, onAdded, onFailed }: AddToPla } setSubmitting(true); - setInlineError(null); + setSubmitError(null); const playlistId = Number(selectedPlaylist); const name = playlists.find((playlist) => String(playlist.id) === selectedPlaylist)?.name ?? 'playlist'; addItemsToPlaylist(playlistId, resolveRequest(items)) .then(() => { - // NOT guarded: both belong to the still-mounted PARENT (#830). `activeRef` gated these - // until 2026-08-29, so dismissing mid-request while the write SUCCEEDED reported nothing and - // left the caller's selection state stale — the same silent-outcome defect as the failure - // half, in the other direction. Measured on `AddToCollectionDialog`: with the gate in place - // `onAdded` is called 0 times after dismissal. `AddItemsDialog` never had the gate here and - // carries the same reasoning. - onAdded?.(name); - onClose(); - }) - .catch((error: unknown) => { - // Reported BEFORE the activeRef gate: swallowing this on dismissal is the #830 defect. - // `reportFailure` renders inline while the dialog is up, and diverts to `onFailed` once it - // is gone. `setSubmitting` stays gated — that state died with the component. - reportFailure(messageFromPlaylistError(error, 'Unable to add items')); - if (!activeRef.current) { return; } + onAdded?.(name); + onClose(); + }) + .catch((error: unknown) => { + if (!activeRef.current) { + return; + } + + setSubmitError(messageFromPlaylistError(error, 'Unable to add items')); setSubmitting(false); }); }; @@ -217,9 +204,9 @@ function AddToPlaylistDialogBody({ onClose, items, onAdded, onFailed }: AddToPla } value={selectedPlaylist} /> - {inlineError && ( + {submitError && ( - {inlineError} + {submitError} )} diff --git a/web/src/media/addTo/AddToScheduleDialog.tsx b/web/src/media/addTo/AddToScheduleDialog.tsx index 333ade729..c0cf444c5 100644 --- a/web/src/media/addTo/AddToScheduleDialog.tsx +++ b/web/src/media/addTo/AddToScheduleDialog.tsx @@ -7,7 +7,6 @@ import { type LibraryBrowseMediaType, type ProgramSchedule } from '../../api'; -import { useDismissSafeError } from '../../hooks'; import { ApiError } from '../../api/client'; import { collectionTypeForMediaType, scheduleItemRequestForMediaItem } from './scheduleItem'; @@ -22,10 +21,6 @@ export interface AddToScheduleDialogProps { onClose: () => void; item: AddToScheduleItem; onAdded?: (scheduleName: string) => void; - // #830: the SURVIVING surface. A write failure landing after dismissal (Escape / backdrop / - // header X — none of which consult `submitting`) has nowhere to render, so it is handed to the - // caller. Optional: where a host screen has no surface for it, the failure is dropped as before. - onFailed?: (message: string) => void; } function messageFromError(error: unknown, fallback: string): string { @@ -48,13 +43,13 @@ export function AddToScheduleDialog(props: AddToScheduleDialogProps) { return ; } -function AddToScheduleDialogBody({ onClose, item, onAdded, onFailed }: AddToScheduleDialogProps) { +function AddToScheduleDialogBody({ onClose, item, onAdded }: AddToScheduleDialogProps) { const [loading, setLoading] = useState(true); const [loadError, setLoadError] = useState(null); const [schedules, setSchedules] = useState([]); const [selected, setSelected] = useState(''); const [submitting, setSubmitting] = useState(false); - const { inlineError, reportFailure, setInlineError } = useDismissSafeError(onFailed ?? (() => {})); + const [submitError, setSubmitError] = useState(null); const activeRef = useRef(true); useEffect(() => { @@ -100,7 +95,7 @@ function AddToScheduleDialogBody({ onClose, item, onAdded, onFailed }: AddToSche } setSubmitting(true); - setInlineError(null); + setSubmitError(null); const scheduleId = Number(selected); const name = schedules.find((schedule) => String(schedule.id) === selected)?.name ?? 'schedule'; @@ -108,25 +103,19 @@ function AddToScheduleDialogBody({ onClose, item, onAdded, onFailed }: AddToSche addScheduleItem(scheduleId, payload) .then(() => { - // NOT guarded: both belong to the still-mounted PARENT (#830). `activeRef` gated these - // until 2026-08-29, so dismissing mid-request while the write SUCCEEDED reported nothing and - // left the caller's selection state stale — the same silent-outcome defect as the failure - // half, in the other direction. Measured on `AddToCollectionDialog`: with the gate in place - // `onAdded` is called 0 times after dismissal. `AddItemsDialog` never had the gate here and - // carries the same reasoning. - onAdded?.(name); - onClose(); - }) - .catch((error: unknown) => { - // Reported BEFORE the activeRef gate: swallowing this on dismissal is the #830 defect. - // `reportFailure` renders inline while the dialog is up, and diverts to `onFailed` once it - // is gone. `setSubmitting` stays gated — that state died with the component. - reportFailure(messageFromError(error, 'Unable to add to schedule')); - if (!activeRef.current) { return; } + onAdded?.(name); + onClose(); + }) + .catch((error: unknown) => { + if (!activeRef.current) { + return; + } + + setSubmitError(messageFromError(error, 'Unable to add to schedule')); setSubmitting(false); }); }; @@ -174,9 +163,9 @@ function AddToScheduleDialogBody({ onClose, item, onAdded, onFailed }: AddToSche options={schedules.map((schedule) => ({ value: String(schedule.id), label: schedule.name ?? `Schedule ${schedule.id}` }))} value={selected} /> - {inlineError && ( + {submitError && ( - {inlineError} + {submitError} )} diff --git a/web/src/media/addTo/SaveAsSmartCollectionDialog.tsx b/web/src/media/addTo/SaveAsSmartCollectionDialog.tsx index b1e5ab21a..f6e1c2126 100644 --- a/web/src/media/addTo/SaveAsSmartCollectionDialog.tsx +++ b/web/src/media/addTo/SaveAsSmartCollectionDialog.tsx @@ -8,7 +8,6 @@ import { updateSmartCollection, type SmartCollection } from '../../api'; -import { useDismissSafeError } from '../../hooks'; export interface SaveAsSmartCollectionDialogProps { open: boolean; @@ -16,10 +15,6 @@ export interface SaveAsSmartCollectionDialogProps { // The search query the saved smart collection will store. query: string; onSaved?: (collectionName: string) => void; - // #830: the SURVIVING surface. A write failure landing after dismissal (Escape / backdrop / - // header X — none of which consult `submitting`) has nowhere to render, so it is handed to the - // caller. Optional: where a host screen has no surface for it, the failure is dropped as before. - onFailed?: (message: string) => void; } const NEW_COLLECTION = '__new__'; @@ -32,14 +27,14 @@ export function SaveAsSmartCollectionDialog(props: SaveAsSmartCollectionDialogPr return ; } -function SaveAsSmartCollectionDialogBody({ onClose, query, onSaved, onFailed }: SaveAsSmartCollectionDialogProps) { +function SaveAsSmartCollectionDialogBody({ onClose, query, onSaved }: SaveAsSmartCollectionDialogProps) { const [loading, setLoading] = useState(true); const [loadError, setLoadError] = useState(null); const [collections, setCollections] = useState([]); const [selected, setSelected] = useState(NEW_COLLECTION); const [newName, setNewName] = useState(''); const [submitting, setSubmitting] = useState(false); - const { inlineError, reportFailure, setInlineError } = useDismissSafeError(onFailed ?? (() => {})); + const [submitError, setSubmitError] = useState(null); const activeRef = useRef(true); useEffect(() => { @@ -85,7 +80,7 @@ function SaveAsSmartCollectionDialogBody({ onClose, query, onSaved, onFailed }: } setSubmitting(true); - setInlineError(null); + setSubmitError(null); // New: create with the query baked in. Existing: overwrite its query (keeping its name). const save: Promise = isNew @@ -100,25 +95,19 @@ function SaveAsSmartCollectionDialogBody({ onClose, query, onSaved, onFailed }: save .then((name) => { - // NOT guarded: both belong to the still-mounted PARENT (#830). `activeRef` gated these - // until 2026-08-29, so dismissing mid-request while the write SUCCEEDED reported nothing and - // left the caller's selection state stale — the same silent-outcome defect as the failure - // half, in the other direction. Measured on `AddToCollectionDialog`: with the gate in place - // `onAdded` is called 0 times after dismissal. `AddItemsDialog` never had the gate here and - // carries the same reasoning. - onSaved?.(name); - onClose(); - }) - .catch((error: unknown) => { - // Reported BEFORE the activeRef gate: swallowing this on dismissal is the #830 defect. - // `reportFailure` renders inline while the dialog is up, and diverts to `onFailed` once it - // is gone. `setSubmitting` stays gated — that state died with the component. - reportFailure(messageFromCollectionError(error, 'Unable to save smart collection')); - if (!activeRef.current) { return; } + onSaved?.(name); + onClose(); + }) + .catch((error: unknown) => { + if (!activeRef.current) { + return; + } + + setSubmitError(messageFromCollectionError(error, 'Unable to save smart collection')); setSubmitting(false); }); }; @@ -178,9 +167,9 @@ function SaveAsSmartCollectionDialogBody({ onClose, query, onSaved, onFailed }: value={newName} /> )} - {inlineError && ( + {submitError && ( - {inlineError} + {submitError} )} diff --git a/web/src/screens/MediaBrowseScreen.tsx b/web/src/screens/MediaBrowseScreen.tsx index d7a2eca43..8770ce5d5 100644 --- a/web/src/screens/MediaBrowseScreen.tsx +++ b/web/src/screens/MediaBrowseScreen.tsx @@ -285,14 +285,12 @@ export function MediaBrowseScreen() { items={selectedItems} onAdded={onBulkAdded} onClose={() => setBulkDialog(null)} - onFailed={(message) => setNotice({ tone: 'error', message })} open={bulkDialog === 'collection'} /> setBulkDialog(null)} - onFailed={(message) => setNotice({ tone: 'error', message })} open={bulkDialog === 'playlist'} /> @@ -336,12 +334,7 @@ export function MediaBrowseScreen() { // card navigation stays live. actions={ selectMode || refreshing ? undefined : ( - setNotice({ tone: 'ok', message })} - onFailed={(message) => setNotice({ tone: 'error', message })} - /> + setNotice({ tone: 'ok', message })} /> ) } item={item} diff --git a/web/src/screens/SearchScreen.tsx b/web/src/screens/SearchScreen.tsx index 88f402d2d..b74f21a7b 100644 --- a/web/src/screens/SearchScreen.tsx +++ b/web/src/screens/SearchScreen.tsx @@ -387,12 +387,7 @@ export function SearchScreen() { // card navigation stays live. actions={ selectMode || refreshing ? undefined : ( - setNotice({ tone: 'ok', message })} - onFailed={(message) => setNotice({ tone: 'error', message })} - /> + setNotice({ tone: 'ok', message })} /> ) } item={item} @@ -417,14 +412,12 @@ export function SearchScreen() { items={dialog?.kind === 'collection' ? dialog.items : []} onAdded={onAddedToSelectionTarget('collection')} onClose={() => setDialog(null)} - onFailed={(message) => setNotice({ tone: 'error', message })} open={dialog?.kind === 'collection'} /> setDialog(null)} - onFailed={(message) => setNotice({ tone: 'error', message })} open={dialog?.kind === 'playlist'} /> setNotice({ tone: 'error', message })} open={dialog?.kind === 'save-smart'} query={query.trim()} /> From 626cbfbf9de0c93301cb6c7dc9c7ff468e2abf9e Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 20:46:30 +0200 Subject: [PATCH 4/8] docs(830): three sentences survived the revert of the code they described MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY --- docs/decisions/README.md | 2 +- .../dismissible-write-failure-reporting.md | 21 +++++++++++++------ web/src/hooks.ts | 15 ++++++------- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index f6cc6b256..b024b729a 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -195,7 +195,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `spa.collection-custom-order-ui` | Collection custom ordering uses per-row Move up/Move down buttons (not drag) and is offered for any manual collection with custom order enabled, not just movies-only. | 2026-07-09 | [link](records/spa/collection-custom-order-ui.md) | | `spa.datetime-local-input` | The channel-mode date/time input uses a native `` instead of free-text Chronic natural-language parsing. | 2026-07-09 | [link](records/spa/datetime-local-input.md) | | `spa.deco-templates-table` | The deco-templates editor also renders its day/deco assignment as a table, extending (not replacing) the templates-editor-table convention. | 2026-07-09 | [link](records/spa/deco-templates-table.md) | -| `spa.dismissible-write-failure-reporting` | A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report the OUTCOME past dismissal in both directions — a success callback gated on an is-mounted check makes a completed write silent too (measured on `AddToCollectionDialog`, #877) — but gate the DISMISS request (`onClose`) separately, because closing a surface that is no longer yours closes whatever replaced it. The surviving surface belongs to the PARENT and differs per screen (a screen-level `role="alert"` banner on `CollectionsScreen`, the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen`), so the mechanism is a PROP CONTRACT rather than a rendering decision; there is no global toast host in this SPA and this decision does not add one. The reporting prop is REQUIRED where the host has a surface (`AddItemsDialog.onAddFailed`), so a failure cannot be dropped by forgetting to wire it; it is optional only where some host genuinely has nowhere to report, and there an omitted callback drops the failure exactly as before — a KNOWN remaining gap, not a claim of coverage. | 2026-08-29 | [link](records/spa/dismissible-write-failure-reporting.md) | +| `spa.dismissible-write-failure-reporting` | A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report the OUTCOME past dismissal in both directions — a success callback gated on an is-mounted check makes a completed write silent too (measured on `AddToCollectionDialog`, #877) — but gate the DISMISS request (`onClose`) separately, because closing a surface that is no longer yours closes whatever replaced it. The surviving surface belongs to the PARENT, so the mechanism is a PROP CONTRACT rather than a rendering decision; as of 2026-08-29 exactly ONE screen is wired (`CollectionsScreen`, into its screen-level `role="alert"` banner), and the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen` is a CANDIDATE second surface, not a wired one; there is no global toast host in this SPA and this decision does not add one. The reporting prop is REQUIRED where the host has a surface (`AddItemsDialog.onAddFailed`), so a failure cannot be dropped by forgetting to wire it; it is optional only where some host genuinely has nowhere to report, and there an omitted callback drops the failure exactly as before — a KNOWN remaining gap, not a claim of coverage. | 2026-08-29 | [link](records/spa/dismissible-write-failure-reporting.md) | | `spa.download-sample-gate` | The SPA disables both Download Media Sample and Download Results while a troubleshooting session is starting/running (Blazor only gated Download Results). | 2026-07-09 | [link](records/spa/download-sample-gate.md) | | `spa.field-progressive-disclosure` | A consequential settings field explains itself through one shared `FieldHelp` icon trigger beside the field name — never the label itself, never a widened `Tooltip` — with the paragraph declared `as const` in the screen's own `FIELD_HELP` record and the panel portalled to `document.body`. | 2026-08-26 | [link](records/spa/field-progressive-disclosure.md) | | `spa.legacy-redirect-matcher` | `LegacyUiRedirects.TryGetRedirect` is a two-tier matcher — an exact `OrdinalIgnoreCase` `Map` (Tier 1) then an ordered segment-template pattern list (Tier 2, first-match-wins) — collision-free by construction, with a guard invariant that no rule may prefix-match `/api`, `/artwork`, `/docs`, `/openapi`, `/iptv`, `/app`, or `/media/sources`. | 2026-07-11 | [link](records/spa/legacy-redirect-matcher.md) | diff --git a/docs/decisions/records/spa/dismissible-write-failure-reporting.md b/docs/decisions/records/spa/dismissible-write-failure-reporting.md index 631c7795e..68ae56901 100644 --- a/docs/decisions/records/spa/dismissible-write-failure-reporting.md +++ b/docs/decisions/records/spa/dismissible-write-failure-reporting.md @@ -5,7 +5,7 @@ status: active since: '2026-08-29' supersedes: none superseded-by: none -rule: 'A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report the OUTCOME past dismissal in both directions — a success callback gated on an is-mounted check makes a completed write silent too (measured on `AddToCollectionDialog`, #877) — but gate the DISMISS request (`onClose`) separately, because closing a surface that is no longer yours closes whatever replaced it. The surviving surface belongs to the PARENT and differs per screen (a screen-level `role="alert"` banner on `CollectionsScreen`, the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen`), so the mechanism is a PROP CONTRACT rather than a rendering decision; there is no global toast host in this SPA and this decision does not add one. The reporting prop is REQUIRED where the host has a surface (`AddItemsDialog.onAddFailed`), so a failure cannot be dropped by forgetting to wire it; it is optional only where some host genuinely has nowhere to report, and there an omitted callback drops the failure exactly as before — a KNOWN remaining gap, not a claim of coverage.' +rule: 'A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report the OUTCOME past dismissal in both directions — a success callback gated on an is-mounted check makes a completed write silent too (measured on `AddToCollectionDialog`, #877) — but gate the DISMISS request (`onClose`) separately, because closing a surface that is no longer yours closes whatever replaced it. The surviving surface belongs to the PARENT, so the mechanism is a PROP CONTRACT rather than a rendering decision; as of 2026-08-29 exactly ONE screen is wired (`CollectionsScreen`, into its screen-level `role="alert"` banner), and the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen` is a CANDIDATE second surface, not a wired one; there is no global toast host in this SPA and this decision does not add one. The reporting prop is REQUIRED where the host has a surface (`AddItemsDialog.onAddFailed`), so a failure cannot be dropped by forgetting to wire it; it is optional only where some host genuinely has nowhere to report, and there an omitted callback drops the failure exactly as before — a KNOWN remaining gap, not a claim of coverage.' signals: 'failed add silently swallowed · dialog closed mid-request · Escape backdrop close button ignore adding flag · error banner unmounts with the dialog · useDismissSafeError inline vs onFailed · success outlives dismissal but failure does not · onDone has no failure counterpart · paths: `web/src/hooks.ts`, `web/src/screens/CollectionsScreen.tsx`, `web/src/media/addTo/`, `web/src/components/overlay.tsx` · issues: #830, #877, #740, #685' mechanics: 'Pinned three ways. (1) `web/src/hooks.test.tsx` → "useDismissSafeError (#830)" pins both branches directly: mounted reports INLINE and does not call `onFailed`, unmounted calls `onFailed`, and the report goes through the LATEST callback rather than the one captured on first render. (2) `CollectionsScreen.test.tsx` → "reports a failed add on the screen when the dialog was dismissed before the request settled (#830)" drives the whole path — it parks the POST in flight, dismisses via Escape, then settles the request and asserts the message is on the screen and NOT inside a dialog. Executed: deleting `reportRef.current(message)` alone reddens it on `Unable to find an element with the text: Request failed with status 500`. That one clause is shared, so the same mutation reddens THREE tests (this one plus the two divert tests in (1)) — expect three reds, not one, when re-running it. (3) `CollectionsScreen.guards.test.tsx` counts is-mounted reads and moved from 2 to 1 when the catch''s guard migrated into the hook; its `...actual` module mock cannot see the hook''s internal `useIsMountedRef()`, which is why (1) exists. Executed: removing the surviving `finally` guard takes that count to 0 and reddens.' --- @@ -55,11 +55,20 @@ clears its flag in a PASSIVE effect cleanup, so there is a narrow window in whic detached but the flag still reads true — the message then renders inline into a dead tree instead of diverting. `useLayoutEffect` would close it deterministically, but that hook is shared by every async caller in the SPA (#578) and changing its timing is not something to do inside a bug fix; -tracked with the rest in #877. (2) The surviving surfaces are not equivalent: `CollectionsScreen` -reports into `role="alert"`, while `MediaBrowseScreen`/`SearchScreen` use `Toast`, which is -`role="status"` (polite) and a single last-writer-wins slot — so a diverted failure is announced -less assertively and can be overwritten by a later success. Both are still strictly better than the -drop they replace. +tracked with the rest in #877. (2) A limit the SECOND surface will have, stated now so it is +not discovered when it is wired: `CollectionsScreen` reports into `role="alert"`, but the candidate +surface on `MediaBrowseScreen`/`SearchScreen` is `Toast`, which is `role="status"` (polite) and a +single last-writer-wins slot — so a failure diverted there would be announced less assertively and +could be overwritten by a later success. Nothing diverts to those screens today; they receive no +reporting callback. + +**The hook has exactly one consumer, and that is worth saying.** `useDismissSafeError` is a shared +module with a single call site today. It earns the shape three ways: it is unit-tested directly in +`hooks.test.tsx`, which is the ONLY thing pinning the diverted branch (the guards test cannot see +inside it); §3c prescribes it for the next site; and #877 is a queued second consumer. The caveat: +#877 may conclude that A2's right answer is a shared reporting SURFACE rather than a per-site prop, +in which case this hook may never gain that second consumer and stays a one-off with a convention +section attached. That is an accepted risk, not an oversight. **Scope, stated rather than implied.** A sweep of all 68 `Dialog`/`ConfirmDialog`/`SlideOver` call sites (2026-08-29; 67 caller-owned plus `ConfirmDialog`'s own internal ` Date: Sat, 29 Aug 2026 20:57:13 +0200 Subject: [PATCH 5/8] docs(830): drop a coverage claim this record's own mechanics field contradicts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY --- .../records/spa/dismissible-write-failure-reporting.md | 10 +++++----- docs/spa-conventions.md | 7 ++++--- web/src/hooks.ts | 5 +++-- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/docs/decisions/records/spa/dismissible-write-failure-reporting.md b/docs/decisions/records/spa/dismissible-write-failure-reporting.md index 68ae56901..a072e9357 100644 --- a/docs/decisions/records/spa/dismissible-write-failure-reporting.md +++ b/docs/decisions/records/spa/dismissible-write-failure-reporting.md @@ -23,9 +23,9 @@ Extending the mechanism to them was attempted and WITHDRAWN, which is why they a record. Removing that gate reports success correctly but also un-gates `onClose()`, and the two mean different things: `onAdded` is "tell the parent what happened", `onClose` is "close me" — addressed to a surface that no longer exists. Measured against the real `AddToMenu`: a late success from a -DISMISSED dialog closed a dialog the user had since reopened, and on `SearchScreen` / -`MediaBrowseScreen` the parents' success handlers also run `clearSelection()`, wiping a multi-select -the user had rebuilt. Un-gating both is wrong, gating both is wrong, and gating only `onClose` still +DISMISSED dialog closed a dialog the user had since reopened. Read, not measured: the success +handlers on `SearchScreen` / `MediaBrowseScreen` also call `clearSelection()` unconditionally, so +the same late success would wipe a multi-select the user had rebuilt. Un-gating both is wrong, gating both is wrong, and gating only `onClose` still needs the parents to stop nulling the dialog themselves — three coupled edits across five files, plus an unresolved question about whether `clearSelection()` should fire for a write the user walked away from. That is a design decision, not a bug fix, so it goes to #877 with the measurements @@ -64,8 +64,8 @@ reporting callback. **The hook has exactly one consumer, and that is worth saying.** `useDismissSafeError` is a shared module with a single call site today. It earns the shape three ways: it is unit-tested directly in -`hooks.test.tsx`, which is the ONLY thing pinning the diverted branch (the guards test cannot see -inside it); §3c prescribes it for the next site; and #877 is a queued second consumer. The caveat: +`hooks.test.tsx`, in a way that would be awkward to keep if the hook were inlined into +`CollectionsScreen`; §3c prescribes it for the next site; and #877 is a queued second consumer. The caveat: #877 may conclude that A2's right answer is a shared reporting SURFACE rather than a per-site prop, in which case this hook may never gain that second consumer and stays a one-off with a convention section attached. That is an accepted risk, not an oversight. diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index 8f5c5ac95..e6f365410 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -434,9 +434,10 @@ site today. The other candidate surface is the `notice` + `Toast` pair that `Med `SearchScreen` already use for success. There is no global toast host in this SPA; do not add one for a single screen. -Those two surfaces are **not equivalent**, so pick deliberately: `Toast` is `role="status"` (polite, -announced less assertively than `role="alert"`) and each screen has a single `notice` slot, so a -later success Toast overwrites a pending failure one. Both still beat dropping the message. +The two are **not equivalent**, so pick deliberately if you wire the second: `Toast` is +`role="status"` (polite, announced less assertively than `role="alert"`) and each screen has a +single `notice` slot, so a later success Toast would overwrite a pending failure one. Nothing +diverts to those screens today — they receive no reporting callback. **Both halves of the outcome must reach the surviving caller**, and they are not the same kind of callback. Report the OUTCOME (`onAdded` / `onDone` / `onFailed`) unguarded — gating a success diff --git a/web/src/hooks.ts b/web/src/hooks.ts index ba4476372..b651eef60 100644 --- a/web/src/hooks.ts +++ b/web/src/hooks.ts @@ -41,8 +41,9 @@ export function useIsMountedRef(): { readonly current: boolean } { // // `reportAfterDismiss` is read through a latest-ref for the same reason `useOverlayBehavior` does // it: parents hand down a fresh closure every render, and the ref must still be callable AFTER this -// component unmounts, which is precisely when it is used. Note the reach is ONE level: the reporting -// callback (`AddItemsDialog.onAddFailed`) targets whatever component owns the surviving surface, so if THAT unmounts too -- navigating away, +// component unmounts, which is precisely when it is used. Note the reach is ONE level: the +// reporting callback (`AddItemsDialog.onAddFailed`) targets whatever component owns the surviving +// surface, so if THAT unmounts too -- navigating away, // or `CollectionsScreen`'s Back out of a collection while an add is in flight -- the setState is a // no-op on React 18+ and the message is still lost. Hoisting the banner further up is what would // shrink that, not a change here. From 86e9ad41a67311c3b28756bdbff02c63801c2331 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 21:18:03 +0200 Subject: [PATCH 6/8] =?UTF-8?q?fix(830):=20gate=20onClose=20at=20the=20exe?= =?UTF-8?q?mplar=20=E2=80=94=20the=20convention=20contradicted=20its=20own?= =?UTF-8?q?=20reference?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY --- .../dismissible-write-failure-reporting.md | 27 +++++++-- web/src/screens/CollectionsScreen.test.tsx | 55 +++++++++++++++++++ web/src/screens/CollectionsScreen.tsx | 18 ++++-- 3 files changed, 90 insertions(+), 10 deletions(-) diff --git a/docs/decisions/records/spa/dismissible-write-failure-reporting.md b/docs/decisions/records/spa/dismissible-write-failure-reporting.md index a072e9357..dea595236 100644 --- a/docs/decisions/records/spa/dismissible-write-failure-reporting.md +++ b/docs/decisions/records/spa/dismissible-write-failure-reporting.md @@ -7,7 +7,7 @@ supersedes: none superseded-by: none rule: 'A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report the OUTCOME past dismissal in both directions — a success callback gated on an is-mounted check makes a completed write silent too (measured on `AddToCollectionDialog`, #877) — but gate the DISMISS request (`onClose`) separately, because closing a surface that is no longer yours closes whatever replaced it. The surviving surface belongs to the PARENT, so the mechanism is a PROP CONTRACT rather than a rendering decision; as of 2026-08-29 exactly ONE screen is wired (`CollectionsScreen`, into its screen-level `role="alert"` banner), and the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen` is a CANDIDATE second surface, not a wired one; there is no global toast host in this SPA and this decision does not add one. The reporting prop is REQUIRED where the host has a surface (`AddItemsDialog.onAddFailed`), so a failure cannot be dropped by forgetting to wire it; it is optional only where some host genuinely has nowhere to report, and there an omitted callback drops the failure exactly as before — a KNOWN remaining gap, not a claim of coverage.' signals: 'failed add silently swallowed · dialog closed mid-request · Escape backdrop close button ignore adding flag · error banner unmounts with the dialog · useDismissSafeError inline vs onFailed · success outlives dismissal but failure does not · onDone has no failure counterpart · paths: `web/src/hooks.ts`, `web/src/screens/CollectionsScreen.tsx`, `web/src/media/addTo/`, `web/src/components/overlay.tsx` · issues: #830, #877, #740, #685' -mechanics: 'Pinned three ways. (1) `web/src/hooks.test.tsx` → "useDismissSafeError (#830)" pins both branches directly: mounted reports INLINE and does not call `onFailed`, unmounted calls `onFailed`, and the report goes through the LATEST callback rather than the one captured on first render. (2) `CollectionsScreen.test.tsx` → "reports a failed add on the screen when the dialog was dismissed before the request settled (#830)" drives the whole path — it parks the POST in flight, dismisses via Escape, then settles the request and asserts the message is on the screen and NOT inside a dialog. Executed: deleting `reportRef.current(message)` alone reddens it on `Unable to find an element with the text: Request failed with status 500`. That one clause is shared, so the same mutation reddens THREE tests (this one plus the two divert tests in (1)) — expect three reds, not one, when re-running it. (3) `CollectionsScreen.guards.test.tsx` counts is-mounted reads and moved from 2 to 1 when the catch''s guard migrated into the hook; its `...actual` module mock cannot see the hook''s internal `useIsMountedRef()`, which is why (1) exists. Executed: removing the surviving `finally` guard takes that count to 0 and reddens.' +mechanics: 'Pinned four ways. (1) `web/src/hooks.test.tsx` → "useDismissSafeError (#830)" pins both branches directly: mounted reports INLINE and does not call `onFailed`, unmounted calls `onFailed`, and the report goes through the LATEST callback rather than the one captured on first render. (2) `CollectionsScreen.test.tsx` → "reports a failed add on the screen when the dialog was dismissed before the request settled (#830)" drives the whole path — it parks the POST in flight, dismisses via Escape, then settles the request and asserts the message is on the screen and NOT inside a dialog. Executed: deleting `reportRef.current(message)` alone reddens it on `Unable to find an element with the text: Request failed with status 500`. That one clause is shared, so the same mutation reddens THREE tests (this one plus the two divert tests in (1)) — expect three reds, not one, when re-running it. (3) `CollectionsScreen.test.tsx` → "a late SUCCESS does not close the dialog the user reopened after dismissing (#830)" pins the OTHER half of the split: it dismisses mid-request, reopens the picker, then settles the request 204, and asserts the reopened dialog is still there — with 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. (4) `CollectionsScreen.guards.test.tsx` counts is-mounted reads and moved from 2 to 1 when the catch''s guard migrated into the hook; its `...actual` module mock cannot see the hook''s internal `useIsMountedRef()`, which is why (1) exists. Executed: removing the surviving `finally` guard takes that count to 0 and reddens.' --- **This record applies to ONE site, and the reason the rest were dropped is the useful part.** @@ -25,7 +25,8 @@ different things: `onAdded` is "tell the parent what happened", `onClose` is "cl to a surface that no longer exists. Measured against the real `AddToMenu`: a late success from a DISMISSED dialog closed a dialog the user had since reopened. Read, not measured: the success handlers on `SearchScreen` / `MediaBrowseScreen` also call `clearSelection()` unconditionally, so -the same late success would wipe a multi-select the user had rebuilt. Un-gating both is wrong, gating both is wrong, and gating only `onClose` still +the same late success would wipe a multi-select the user had rebuilt. Un-gating both is wrong, +gating both is wrong, and gating only `onClose` still needs the parents to stop nulling the dialog themselves — three coupled edits across five files, plus an unresolved question about whether `clearSelection()` should fire for a write the user walked away from. That is a design decision, not a bug fix, so it goes to #877 with the measurements @@ -34,6 +35,16 @@ attached rather than riding along here. `AddToMenu` also has `onDone` and no failure counterpart at all, which is why a failed "Add to collection" from a media card reports nowhere. Same issue. +**The outcome and the dismissal are different callbacks, and the exemplar proves it.** Review +measured the cost of conflating them twice: gating both makes a completed write silent, un-gating +both makes a late success close the dialog the user reopened to retry. `AddItemsDialog` now does +each correctly — `onAdded()` unguarded, because it reloads the parent's list and that must survive; +`onClose()` guarded, because after dismissal it is `setPickerOpen(false)` aimed at whatever the user +opened next. That split is only this simple where the parent has no competing closer: here +`onAdded` is `load`, which never touches `pickerOpen`. In the `media/addTo/` layer +`AddToMenu.handleAdded` closes the dialog itself, so the same one-line gate is NOT sufficient there +— measured — which is the concrete reason that half is #877 and not this record. + **Why the inline branch is kept rather than always reporting to the parent.** While the dialog is up, inline is the better surface: it keeps the user's selections and the context they are looking at. Diverting to a parent banner in that case would be its own defect — the message would surface @@ -65,18 +76,22 @@ reporting callback. **The hook has exactly one consumer, and that is worth saying.** `useDismissSafeError` is a shared module with a single call site today. It earns the shape three ways: it is unit-tested directly in `hooks.test.tsx`, in a way that would be awkward to keep if the hook were inlined into -`CollectionsScreen`; §3c prescribes it for the next site; and #877 is a queued second consumer. The caveat: +`CollectionsScreen`; §3c prescribes it for the next site; and #877 is a queued second consumer. The +caveat: #877 may conclude that A2's right answer is a shared reporting SURFACE rather than a per-site prop, in which case this hook may never gain that second consumer and stays a one-off with a convention section attached. That is an accepted risk, not an oversight. **Scope, stated rather than implied.** A sweep of all 68 `Dialog`/`ConfirmDialog`/`SlideOver` call -sites (2026-08-29; 67 caller-owned plus `ConfirmDialog`'s own internal ` { expect(alert.closest('[role="dialog"]')).toBeNull(); }); + // #830, the other half of the same mechanism. `onAdded` and `onClose` are not the same kind of + // callback: the first REPORTS an outcome and must survive dismissal, the second REQUESTS A + // DISMISSAL and must not — after dismissal it closes whatever the user opened next. Un-gating both + // was measured wrong in review; this pins the split at the one site that ships it. + it('a late SUCCESS does not close the dialog the user reopened after dismissing (#830)', async () => { + let succeedAdd: (() => void) | undefined; + const addInFlight = new Promise((resolve) => { + succeedAdd = () => resolve(new Response(null, { status: 204 })); + }); + + const fetchMock = mockAddItemsApi((url, method) => + url === '/api/v1/collections/1/items' && method === 'POST' ? addInFlight : null + ); + + const dialog = await openAddItemsDialog(); + fireEvent.change(within(dialog).getByPlaceholderText('Search movies, shows, seasons, artists…'), { + target: { value: 'za' } + }); + fireEvent.click(within(dialog).getByRole('button', { name: 'Search' })); + expect(await within(dialog).findByText('Zathura')).toBeInTheDocument(); + + fireEvent.click(within(dialog).getByText('Zathura')); + fireEvent.click(within(dialog).getByRole('button', { name: /Add 1 item/ })); + await waitFor(() => { + expect( + fetchMock.mock.calls.some( + ([u, init]) => u === '/api/v1/collections/1/items' && (init?.method ?? '').toUpperCase() === 'POST' + ) + ).toBe(true); + }); + + fireEvent.keyDown(document, { key: 'Escape' }); + await waitFor(() => expect(screen.queryByRole('dialog')).toBeNull()); + + // The user reopens the picker to retry the add they believe failed. + fireEvent.click(screen.getByRole('button', { name: 'Add items' })); + const reopened = await screen.findByRole('dialog'); + + // ...and only now does the first request come back, successfully. + succeedAdd?.(); + + // ANTI-VACUITY: prove the late response was actually processed, so the assertion below is not + // just "nothing has happened yet". `onAdded` is `load`, which refetches the collection's items. + await waitFor(() => { + const itemReads = fetchMock.mock.calls.filter( + ([u, init]) => /\/api\/v1\/collections\/1\/items/.test(u.toString()) && (init?.method ?? 'GET').toUpperCase() === 'GET' + ); + expect(itemReads.length).toBeGreaterThan(1); + }); + + // Remove the `if (mountedRef.current)` around `onClose()` and this dialog vanishes under the user. + expect(reopened).toBeInTheDocument(); + expect(screen.queryByRole('dialog')).not.toBeNull(); + }); + it('selecting a newly-addable kind filter (Song) surfaces it and buckets it correctly on add', async () => { const fetchMock = mockAddItemsApi(); diff --git a/web/src/screens/CollectionsScreen.tsx b/web/src/screens/CollectionsScreen.tsx index 9fbfab410..87b0a2a3f 100644 --- a/web/src/screens/CollectionsScreen.tsx +++ b/web/src/screens/CollectionsScreen.tsx @@ -350,11 +350,21 @@ function AddItemsDialog({ try { await addItemsToCollection(collection.id, toAddItemsRequest([...selected.values()])); - // NOT guarded: both belong to the still-mounted PARENT. `onAdded` reloads the collection's - // items, and dropping it because this dialog went away would leave the list stale after a - // write that succeeded. + // These two are NOT the same kind of callback, and only one of them survives dismissal. + // + // `onAdded` REPORTS what happened: it reloads the parent's item list, and dropping it because + // this dialog went away would leave that list stale after a write that succeeded. Unguarded. + // + // `onClose` REQUESTS A DISMISSAL, and after dismissal the thing it closes is no longer this + // instance -- `onClose` is `() => setPickerOpen(false)` on a parent that has since reopened + // the picker. Firing it from a dead instance closes the dialog the user reopened to retry, + // discarding the selection they just rebuilt (measured). Guarded. See `spa-conventions.md` + // §3c: report the outcome unguarded, gate the dismiss request. onAdded(); - onClose(); + + if (mountedRef.current) { + onClose(); + } } catch (addError) { // The dialog is unmount-reachable while a request is in flight even though the Add button is // disabled — Escape, a backdrop click and the header close button all reach `onClose` without From e4c346e649eae25843010976d19e99da83653e83 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 21:38:10 +0200 Subject: [PATCH 7/8] =?UTF-8?q?test(830):=20pin=20the=20ORDINARY=20close?= =?UTF-8?q?=20too=20=E2=80=94=20the=20negative=20test=20did=20not=20cover?= =?UTF-8?q?=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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//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) Claude-Session: https://claude.ai/code/session_01XE2tF2aUasK2hWPmBRsrMY --- .../records/spa/dismissible-write-failure-reporting.md | 2 +- web/src/screens/CollectionsScreen.guards.test.tsx | 4 +++- web/src/screens/CollectionsScreen.test.tsx | 6 ++++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/decisions/records/spa/dismissible-write-failure-reporting.md b/docs/decisions/records/spa/dismissible-write-failure-reporting.md index dea595236..fe8f01bf5 100644 --- a/docs/decisions/records/spa/dismissible-write-failure-reporting.md +++ b/docs/decisions/records/spa/dismissible-write-failure-reporting.md @@ -7,7 +7,7 @@ supersedes: none superseded-by: none rule: 'A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report the OUTCOME past dismissal in both directions — a success callback gated on an is-mounted check makes a completed write silent too (measured on `AddToCollectionDialog`, #877) — but gate the DISMISS request (`onClose`) separately, because closing a surface that is no longer yours closes whatever replaced it. The surviving surface belongs to the PARENT, so the mechanism is a PROP CONTRACT rather than a rendering decision; as of 2026-08-29 exactly ONE screen is wired (`CollectionsScreen`, into its screen-level `role="alert"` banner), and the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen` is a CANDIDATE second surface, not a wired one; there is no global toast host in this SPA and this decision does not add one. The reporting prop is REQUIRED where the host has a surface (`AddItemsDialog.onAddFailed`), so a failure cannot be dropped by forgetting to wire it; it is optional only where some host genuinely has nowhere to report, and there an omitted callback drops the failure exactly as before — a KNOWN remaining gap, not a claim of coverage.' signals: 'failed add silently swallowed · dialog closed mid-request · Escape backdrop close button ignore adding flag · error banner unmounts with the dialog · useDismissSafeError inline vs onFailed · success outlives dismissal but failure does not · onDone has no failure counterpart · paths: `web/src/hooks.ts`, `web/src/screens/CollectionsScreen.tsx`, `web/src/media/addTo/`, `web/src/components/overlay.tsx` · issues: #830, #877, #740, #685' -mechanics: 'Pinned four ways. (1) `web/src/hooks.test.tsx` → "useDismissSafeError (#830)" pins both branches directly: mounted reports INLINE and does not call `onFailed`, unmounted calls `onFailed`, and the report goes through the LATEST callback rather than the one captured on first render. (2) `CollectionsScreen.test.tsx` → "reports a failed add on the screen when the dialog was dismissed before the request settled (#830)" drives the whole path — it parks the POST in flight, dismisses via Escape, then settles the request and asserts the message is on the screen and NOT inside a dialog. Executed: deleting `reportRef.current(message)` alone reddens it on `Unable to find an element with the text: Request failed with status 500`. That one clause is shared, so the same mutation reddens THREE tests (this one plus the two divert tests in (1)) — expect three reds, not one, when re-running it. (3) `CollectionsScreen.test.tsx` → "a late SUCCESS does not close the dialog the user reopened after dismissing (#830)" pins the OTHER half of the split: it dismisses mid-request, reopens the picker, then settles the request 204, and asserts the reopened dialog is still there — with 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. (4) `CollectionsScreen.guards.test.tsx` counts is-mounted reads and moved from 2 to 1 when the catch''s guard migrated into the hook; its `...actual` module mock cannot see the hook''s internal `useIsMountedRef()`, which is why (1) exists. Executed: removing the surviving `finally` guard takes that count to 0 and reddens.' +mechanics: 'Pinned four ways. (1) `web/src/hooks.test.tsx` → "useDismissSafeError (#830)" pins both branches directly: mounted reports INLINE and does not call `onFailed`, unmounted calls `onFailed`, and the report goes through the LATEST callback rather than the one captured on first render. (2) `CollectionsScreen.test.tsx` → "reports a failed add on the screen when the dialog was dismissed before the request settled (#830)" drives the whole path — it parks the POST in flight, dismisses via Escape, then settles the request and asserts the message is on the screen and NOT inside a dialog. Executed: deleting `reportRef.current(message)` alone reddens it on `Unable to find an element with the text: Request failed with status 500`. That one clause is shared, so the same mutation reddens THREE tests (this one plus the two divert tests in (1)) — expect three reds, not one, when re-running it. (3) `CollectionsScreen.test.tsx` → "a late SUCCESS does not close the dialog the user reopened after dismissing (#830)" pins the OTHER half of the split: it dismisses mid-request, reopens the picker, then settles the request 204, and asserts the reopened dialog is still there — with 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. The POSITIVE direction is pinned separately, in the Song add test, because the negative one does not cover it: review measured that deleting the `onClose()` call ENTIRELY — so an ordinary successful add never closes the picker — left the whole suite green, so that test now asserts the dialog closes. (4) `CollectionsScreen.guards.test.tsx` counts is-mounted reads and moved from 2 to 1 when the catch''s guard migrated into the hook; its `...actual` module mock cannot see the hook''s internal `useIsMountedRef()`, which is why (1) exists. Executed: removing the surviving `finally` guard takes that count to 0 and reddens.' --- **This record applies to ONE site, and the reason the rest were dropped is the useful part.** diff --git a/web/src/screens/CollectionsScreen.guards.test.tsx b/web/src/screens/CollectionsScreen.guards.test.tsx index 5d1827d12..b419e606c 100644 --- a/web/src/screens/CollectionsScreen.guards.test.tsx +++ b/web/src/screens/CollectionsScreen.guards.test.tsx @@ -301,7 +301,9 @@ describe('AddItemsDialog async guards (#740)', () => { parked[0].fail(); // `submit` has exactly ONE post-await write left to state THIS component owns directly — - // `setAdding` in the finally. It was two until #830 moved the catch's report into + // `setAdding` in the finally. That is a census of OWN-STATE writes, not of `mountedRef` reads: + // the SUCCESS path has its own guarded `onClose()` (#830), which this failure-path test never + // reaches, so do not derive the guard population from this number. It was two until #830 moved the catch's report into // `useDismissSafeError`, which keeps its own is-mounted guard and decides between the inline // banner and the parent's surviving surface; that guard is invisible here because the mock // above spreads `...actual`, so the hook's INTERNAL `useIsMountedRef()` call resolves to the diff --git a/web/src/screens/CollectionsScreen.test.tsx b/web/src/screens/CollectionsScreen.test.tsx index e9a115cab..123089b02 100644 --- a/web/src/screens/CollectionsScreen.test.tsx +++ b/web/src/screens/CollectionsScreen.test.tsx @@ -601,6 +601,12 @@ describe('CollectionsScreen', () => { expect(addCall).toBeDefined(); expect(JSON.parse(String(addCall?.[1]?.body)).songIds).toEqual([7]); }); + + // #830: the POSITIVE direction of the report/dismiss split. `onClose()` is guarded on + // `mountedRef` so a LATE success cannot close a dialog the user reopened; this pins that the + // guard did not make the ordinary close dead. Measured: without this line, deleting the + // `onClose()` call entirely left the whole suite green. + await waitFor(() => expect(screen.queryByRole('dialog')).toBeNull()); }); it( From e1d211cd1bcd186b040cacf23f11fa61490f16f5 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 4 Sep 2026 23:35:03 +0200 Subject: [PATCH 8/8] test(830): pin which ARM the call site reaches, not only that the hook has one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review measured M6: swapping `reportFailure(...)` for `onAddFailed(...)` in `AddItemsDialog`'s catch left all 490 tests across the 42 `src/screens` files green. Under that mutant a failure that lands while the dialog is STILL OPEN renders into `ManualItemsView`'s screen banner, which sits behind the dialog's `createPortal` panel with `aria-modal="true"` — covered for sighted users, hidden from AT, and the surface the user is actually looking at stays blank. That is the exact shape the decision record calls "its own defect", and the whole gap was the call-site wiring: the hook's inline branch is pinned at unit level in `hooks.test.tsx`, but a unit test of the hook cannot see which arm a consumer reaches. Adds the integration assertion: fail the POST with the dialog still up, assert the message is inside `[role="dialog"]` and appears exactly once in the tree. Re-executed the mutation with it in place — 1 failed / 490 passed, and the red is this test alone. Records the new pin as mechanics (5) on `spa.dismissible-write-failure-reporting` and the general form in `spa-conventions.md` §3c, so the next site wired to the hook pins both arms at its call site rather than inheriting the hook's unit coverage. Refs #830 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../dismissible-write-failure-reporting.md | 6 ++- docs/spa-conventions.md | 8 ++++ web/src/screens/CollectionsScreen.test.tsx | 37 +++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/docs/decisions/records/spa/dismissible-write-failure-reporting.md b/docs/decisions/records/spa/dismissible-write-failure-reporting.md index fe8f01bf5..be11af505 100644 --- a/docs/decisions/records/spa/dismissible-write-failure-reporting.md +++ b/docs/decisions/records/spa/dismissible-write-failure-reporting.md @@ -7,7 +7,7 @@ supersedes: none superseded-by: none rule: 'A component that starts an async WRITE from inside a dismissible surface (`Dialog`, `SlideOver`, `ConfirmDialog`) must report failure through `useDismissSafeError` (`web/src/hooks.ts`), which renders the message INLINE while the surface is mounted and hands it to a caller-supplied `onFailed` once the surface is gone. Guarding the `setError` with an is-mounted check and stopping there is NOT sufficient: it converts a silent data-loss risk into a silent NO-OP the user reads as success. All three primitives dismiss through Escape and a backdrop/scrim click (`useOverlayBehavior`) plus a header close button, and NONE of those consult a busy flag — disabling the footer Cancel button, which every one of these dialogs does, looks like it closes the hole and does not. Report the OUTCOME past dismissal in both directions — a success callback gated on an is-mounted check makes a completed write silent too (measured on `AddToCollectionDialog`, #877) — but gate the DISMISS request (`onClose`) separately, because closing a surface that is no longer yours closes whatever replaced it. The surviving surface belongs to the PARENT, so the mechanism is a PROP CONTRACT rather than a rendering decision; as of 2026-08-29 exactly ONE screen is wired (`CollectionsScreen`, into its screen-level `role="alert"` banner), and the `notice`+`Toast` pair on `MediaBrowseScreen`/`SearchScreen` is a CANDIDATE second surface, not a wired one; there is no global toast host in this SPA and this decision does not add one. The reporting prop is REQUIRED where the host has a surface (`AddItemsDialog.onAddFailed`), so a failure cannot be dropped by forgetting to wire it; it is optional only where some host genuinely has nowhere to report, and there an omitted callback drops the failure exactly as before — a KNOWN remaining gap, not a claim of coverage.' signals: 'failed add silently swallowed · dialog closed mid-request · Escape backdrop close button ignore adding flag · error banner unmounts with the dialog · useDismissSafeError inline vs onFailed · success outlives dismissal but failure does not · onDone has no failure counterpart · paths: `web/src/hooks.ts`, `web/src/screens/CollectionsScreen.tsx`, `web/src/media/addTo/`, `web/src/components/overlay.tsx` · issues: #830, #877, #740, #685' -mechanics: 'Pinned four ways. (1) `web/src/hooks.test.tsx` → "useDismissSafeError (#830)" pins both branches directly: mounted reports INLINE and does not call `onFailed`, unmounted calls `onFailed`, and the report goes through the LATEST callback rather than the one captured on first render. (2) `CollectionsScreen.test.tsx` → "reports a failed add on the screen when the dialog was dismissed before the request settled (#830)" drives the whole path — it parks the POST in flight, dismisses via Escape, then settles the request and asserts the message is on the screen and NOT inside a dialog. Executed: deleting `reportRef.current(message)` alone reddens it on `Unable to find an element with the text: Request failed with status 500`. That one clause is shared, so the same mutation reddens THREE tests (this one plus the two divert tests in (1)) — expect three reds, not one, when re-running it. (3) `CollectionsScreen.test.tsx` → "a late SUCCESS does not close the dialog the user reopened after dismissing (#830)" pins the OTHER half of the split: it dismisses mid-request, reopens the picker, then settles the request 204, and asserts the reopened dialog is still there — with 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. The POSITIVE direction is pinned separately, in the Song add test, because the negative one does not cover it: review measured that deleting the `onClose()` call ENTIRELY — so an ordinary successful add never closes the picker — left the whole suite green, so that test now asserts the dialog closes. (4) `CollectionsScreen.guards.test.tsx` counts is-mounted reads and moved from 2 to 1 when the catch''s guard migrated into the hook; its `...actual` module mock cannot see the hook''s internal `useIsMountedRef()`, which is why (1) exists. Executed: removing the surviving `finally` guard takes that count to 0 and reddens.' +mechanics: 'Pinned five ways. (1) `web/src/hooks.test.tsx` → "useDismissSafeError (#830)" pins both branches directly: mounted reports INLINE and does not call `onFailed`, unmounted calls `onFailed`, and the report goes through the LATEST callback rather than the one captured on first render. (2) `CollectionsScreen.test.tsx` → "reports a failed add on the screen when the dialog was dismissed before the request settled (#830)" drives the whole path — it parks the POST in flight, dismisses via Escape, then settles the request and asserts the message is on the screen and NOT inside a dialog. Executed: deleting `reportRef.current(message)` alone reddens it on `Unable to find an element with the text: Request failed with status 500`. That one clause is shared, so the same mutation reddens THREE tests (this one plus the two divert tests in (1)) — expect three reds, not one, when re-running it. (3) `CollectionsScreen.test.tsx` → "a late SUCCESS does not close the dialog the user reopened after dismissing (#830)" pins the OTHER half of the split: it dismisses mid-request, reopens the picker, then settles the request 204, and asserts the reopened dialog is still there — with 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. The POSITIVE direction is pinned separately, in the Song add test, because the negative one does not cover it: review measured that deleting the `onClose()` call ENTIRELY — so an ordinary successful add never closes the picker — left the whole suite green, so that test now asserts the dialog closes. (4) `CollectionsScreen.guards.test.tsx` counts is-mounted reads and moved from 2 to 1 when the catch''s guard migrated into the hook; its `...actual` module mock cannot see the hook''s internal `useIsMountedRef()`, which is why (1) exists. Executed: removing the surviving `finally` guard takes that count to 0 and reddens. (5) `CollectionsScreen.test.tsx` → "renders a failed add INSIDE the dialog while it is still open (#830)" pins which ARM the call site reaches, which (1) cannot: (1) proves the hook HAS an inline branch, not that this screen calls into it. It fails the POST with the dialog still up and asserts the message sits inside `[role="dialog"]` and appears exactly once in the tree. Executed 2026-09-04: swapping `reportFailure(...)` for `onAddFailed(...)` in the catch — the divert-while-open shape this record names as its own defect — reddens that test and NOTHING else (1 failed / 490 passed across the 42 `src/screens` files).' --- **This record applies to ONE site, and the reason the rest were dropped is the useful part.** @@ -48,7 +48,9 @@ opened next. That split is only this simple where the parent has no competing cl **Why the inline branch is kept rather than always reporting to the parent.** While the dialog is up, inline is the better surface: it keeps the user's selections and the context they are looking at. Diverting to a parent banner in that case would be its own defect — the message would surface -somewhere else while the dialog the user is staring at stays blank. +somewhere else while the dialog the user is staring at stays blank. Behind the dialog, in fact: +the panel is a `createPortal` with `aria-modal="true"`, so the screen banner is hidden from AT and +covered for everyone else. Pinned rather than argued — mechanics (5). **Why not simply gate dismissal on the busy flag.** That was considered and rejected: gating Escape/backdrop/close on `adding` traps the user behind an in-flight request with no cancel path, diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index e6f365410..10d7b9935 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -456,6 +456,14 @@ one will be, because `MediaDetailScreen` wires no outcome callbacks at all); the `onFailed` drops the failure exactly as today — a known gap, not coverage (#877). Rationale and the full call-site sweep: `docs/decisions/records/spa/dismissible-write-failure-reporting.md`. +**Pin the arm the CALL SITE reaches, not only the hook's.** A unit test of `useDismissSafeError` +proves the inline branch exists; it cannot see a call site that reports through `onFailed` directly +and so pushes the message onto the parent banner while the surface is still up — behind a +`createPortal` panel with `aria-modal="true"`, i.e. covered for sighted users and hidden from AT. +Add an integration assertion for each arm: fail the write with the surface still open and assert the +message is inside `[role="dialog"]`, and fail it again across a dismissal and assert the message is +on the screen and not in a dialog. + ## 4. API client modules One file per domain in `web/src/api/`, e.g. `logs.ts`, `blocks.ts`, `playouts.ts`. Pattern (see diff --git a/web/src/screens/CollectionsScreen.test.tsx b/web/src/screens/CollectionsScreen.test.tsx index 123089b02..e6bf2ae30 100644 --- a/web/src/screens/CollectionsScreen.test.tsx +++ b/web/src/screens/CollectionsScreen.test.tsx @@ -464,6 +464,43 @@ describe('CollectionsScreen', () => { ).toBeInTheDocument(); }); + // #830, the arm the dismissal test below cannot see. `useDismissSafeError` has two of them and the + // call site picks one by calling `reportFailure` rather than `onAddFailed` directly. While the + // dialog is STILL UP the message belongs inline, beside the selections the user built: the + // screen's banner sits behind a `createPortal` panel carrying `aria-modal="true"` + // (`components/overlay.tsx`), so diverting there would leave the surface the user is staring at + // blank and hide the message from assistive tech. Measured: wiring `onAddFailed` at this call + // site instead of `reportFailure` leaves every other test in `src/screens` green. + it('renders a failed add INSIDE the dialog while it is still open (#830)', async () => { + mockAddItemsApi((url, method) => + url === '/api/v1/collections/1/items' && method === 'POST' + ? Promise.resolve(new Response(null, { status: 500 })) + : null + ); + + const dialog = await openAddItemsDialog(); + fireEvent.change(within(dialog).getByPlaceholderText('Search movies, shows, seasons, artists…'), { + target: { value: 'za' } + }); + fireEvent.click(within(dialog).getByRole('button', { name: 'Search' })); + expect(await within(dialog).findByText('Zathura')).toBeInTheDocument(); + + fireEvent.click(within(dialog).getByText('Zathura')); + fireEvent.click(within(dialog).getByRole('button', { name: /Add 1 item/ })); + + // The message lands in the dialog's own role="alert", not the screen's. + const inline = await within(dialog).findByText('Request failed with status 500'); + expect(inline.closest('[role="dialog"]')).toBe(dialog); + + // ...and NOWHERE else. One copy in the whole tree proves the parent banner did not also receive + // it, which is what a `reportFailure` -> `onAddFailed` swap at the call site would produce. + expect(screen.getAllByText('Request failed with status 500')).toHaveLength(1); + + // Still up, with the selection intact — the reason inline is the right surface here at all. + expect(screen.getByRole('dialog')).toBe(dialog); + expect(within(dialog).getByRole('button', { name: /Add 1 item/ })).toBeInTheDocument(); + }); + // #830. The dialog is dismissible mid-request through three paths that never consult `adding` // (Escape, backdrop click, header close button — `components/overlay.tsx`); only the footer Cancel // is disabled, which looks like it closes the hole. Dismissal genuinely unmounts the instance,