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,