fix(app): destructive-action confirm — focus trap + SharesPage Unpublish modal#371
Merged
Merged
Conversation
…ish modal Two renderer-side a11y/UX fixes from the post-launch audit. useFocusTrap (new hook): aria-modal="true" on the confirm dialogs is a hint to assistive tech but doesn't actually contain keyboard focus — Tab/Shift+Tab could escape into the underlying page. On a destructive confirmation surface (Unpublish / Schedule deletion) this is a real footgun: the next Tab might land on a destructive control the user never saw. Wires into both UnpublishConfirmModal and DeleteAccountConfirmModal. Initial focus lands on Cancel (safe default — pressing Enter immediately on open doesn't commit the action), Tab/Shift+Tab cycles inside the modal, focus restores to the previously-active element on close. Focusable detection uses offsetWidth/offsetHeight (not offsetParent, which returns null for position:fixed modals) AND guards against aria-hidden tab-stops. SharesPage Unpublish — confirm modal instead of single-click revoke: The Share popover's unpublish already routes through UnpublishConfirmModal. The Published-list Unpublish action did not — clicking the EyeOff icon on a row immediately fired spoolShare.revoke(). Inconsistent with the destructive-action discipline elsewhere in the app. PublishedList now opens UnpublishConfirmModal on click; the existing optimistic-revoke flow (markRevoked / noteLocalMutation / refresh) moves into the confirm callback. While a revoke is in flight, every row's Unpublish button is `locked` so clicking another row can't silently no-op (the row-level `busy` spinner shows on the active row; other rows show as disabled). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two renderer-side a11y/UX fixes from the post-launch audit.
useFocusTrap— Tab/Shift+Tab containment on destructive modalsaria-modal="true"on the confirm dialogs is a hint to assistive tech but doesn't actually contain keyboard focus — Tab/Shift+Tab could escape into the underlying page. On a destructive confirmation surface (Unpublish / Schedule deletion) this is a real footgun: the next Tab might land on a destructive control the user never saw.New hook wires into both
UnpublishConfirmModalandDeleteAccountConfirmModal:offsetWidth > 0 && offsetHeight > 0(NOToffsetParent, which returns null forposition: fixedoverlays) AND guards againstaria-hiddentab-stopsSharesPage Unpublish — confirm modal instead of single-click revoke
The Share popover's unpublish already routes through
UnpublishConfirmModal. The Published-list Unpublish action did not — clicking the EyeOff icon on a row immediately firedspoolShare.revoke(). Inconsistent with the destructive-action discipline elsewhere in the app.stateDiagram-v2 [*] --> Idle Idle --> Confirming: click EyeOff on row A Confirming --> Idle: cancel / Esc / outside-click Confirming --> Revoking: confirm Revoking --> Idle: 200 + cache.markRevoked + refresh Revoking --> Confirming: error (inline + toast) state Confirming { [*] --> RowsLocked RowsLocked --> RowsLocked: other rows<br/>disabled (locked prop) } state Revoking { [*] --> ActiveRowBusy ActiveRowBusy --> ActiveRowBusy: spinner on row A<br/>others stay locked }While a revoke is in flight, every row's Unpublish button is
lockedso clicking another row can't silently no-op (the row-levelbusyspinner shows on the active row; other rows show as disabled — the user gets unambiguous feedback that the system is busy).Verification
pnpm --filter @spool/app exec tsc --noEmitcleanpnpm --filter @spool/app test→ 433 unit pass (the 1 fail is the pre-existingsessionResume.test.ts:38macOS path-slug flake, unrelated)Risk
VITE_FEATURE_SHAREPUBLISHwhich CI doesn't set, so e2e doesn't exercise this path — covered by manual smoke.lockedprop onPublishedRowis optional with defaultfalse; existing callers (none external) unaffected.Submitted by @graydawnc.