From f376d6cdf4e1a74b226b8b91202a90cd3ec2fd2f Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 21:38:10 +0200 Subject: [PATCH] =?UTF-8?q?test(830):=20pin=20the=20ORDINARY=20close=20too?= =?UTF-8?q?=20=E2=80=94=20the=20negative=20test=20did=20not=20cover=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 c3940430b..d8a63d1f3 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(