[IngridMorstrad]

Problem / Motivation

Goal: Escape closes a dialog as soon as the dialog is on screen, even when it opened from a network answer instead of a click.

A dialog that opens from something other than a click can ignore the first Escape. The unscanned-download confirm ("Too large to scan") opens when the download request answers 409. An Escape pressed right as it appears does nothing: the dialog stays open and the download keeps waiting. CI caught this as a flake in unscannedDownload.test.tsx > "a download over the scan limit asks first" > "Escape cancels" (Frontend Tests (4), run 40182366810, on PR #10132, which cannot reach this code): Test timed out in 15000ms.

Why it matters

Escape is the keyboard way out of every Modal and every dialog built on useDialogFocusTrap. A first key press that does nothing makes the dialog look stuck to a keyboard user. The unscanned-download confirm is opened from the app root after the menu that started it has closed, so a dropped Escape leaves the download waiting on a dialog the user thinks they dismissed. The same race also turns a real product test into a cross-PR flake.

Not a goal

• Changing what Escape does, or the order in which nested layers consume it. Modal's handler logic, its defaultPrevented check and its bubble phase stay the same.
• Changing Modal's body scroll lock. It stays in its own useEffect, exactly as on main.
• Converting the hand-rolled Escape listeners in other components (see Sibling sweep).

What changed (motivation → approach → change)

• Symptom: an Escape fired at the first moment the dialog is in the DOM never reaches onClose, and the confirm promise never settles.
• Root cause: Modal attached its window Escape listener in a useEffect, and useDialogFocusTrap attached its keydown trap the same way. When the open comes from a default-priority update (a fetch answering) and the render takes longer than React's 5 ms scheduler slice, React commits the DOM and then yields to the host before it runs passive effects. In that gap the dialog is visible and nothing is listening, so the key is dropped.
• Change: both keydown listeners now attach in a useLayoutEffect, which runs in the same commit that inserts the dialog. The handlers, their deps and their phases are unchanged. In Modal, the scroll lock is split out and stays in a passive useEffect, so its ordering against other scroll lockers is the same as on main.

Listener	Before	After
Modal window Escape (bubble)	useEffect	useLayoutEffect
useDialogFocusTrap window keydown (capture: Escape + Tab trap)	useEffect	useLayoutEffect
Modal body scroll lock	useEffect	useEffect (unchanged)
useDialogFocusTrap focus-in / restore	useEffect	useEffect (unchanged)

Backwards compatibility

Compatible: the same listeners run with the same logic. They are now attached one task earlier, in the commit that shows the dialog, and they are removed at the same unmount.

Removes nothing: no path, record, validator, key or function is removed. Two listeners change only when they attach.

Tests

Proof: not provable -- frontend test (prove.py runs Python only)

Two new deterministic tests. Each opens a dialog with a default-priority update outside act(), and gives it a render that takes 20 ms, longer than the scheduler slice, so React must yield before passive effects. Then it presses Escape from the MutationObserver callback that first sees [role="dialog"], which is the moment findByRole resolves on. Nothing waits on wall-clock time, and elapsed time is not asserted.

• Modal.escape.test.tsx > "an Escape pressed the moment the dialog appears still closes it": locks in that onClose is called once.
• useDialogFocusTrap.arming.test.tsx > "an Escape pressed the moment the dialog appears reaches onEscape": locks in that onEscape is called once.

Red/green evidence (head 476c26c5f8):

Run	Result on origin/main product code	Result with this fix
Modal.escape new test, 10 separate vitest runs	failed 10/10 (expected "vi.fn()" to be called 1 times, but got 0 times)	pass
useDialogFocusTrap.arming new test, 5 runs	failed 5/5	pass
Same Modal test with the 20 ms render removed (control), 3 runs	failed 3/3	—
Original "Escape cancels" flow, a temporary uncommitted copy with { repeats: 40, timeout: 3000 }, 32 parallel vitest processes beside 32 CPU-hog processes on 32 cores	18 of 96 processes hung (Test timed out in 3000ms, the CI signature)	0 of 192 processes failed (7,872 runs; 64 of them on this head, 128 on an earlier local revision with the same listener change)

Determinism: the two new test files plus unscannedDownload.test.tsx, 20 repeats, 20/20 green. A shuffled run of those three plus the four other Modal and focus-trap test files (--sequence.shuffle --sequence.seed=20310): 54/54 green.

Local gates: the full vitest run (3,336 files) was green on the first full run. One file failed on the second: MarkdownPanel.editFocus.test.tsx, Unable to find an element by: [data-testid="pierre-editable"]. That test uses neither Modal nor the trap. It fails on origin/main under the same load too (11 of 32 parallel runs) and passes 10/10 alone with this fix. tsc -p tsconfig.app.json, eslint on the 4 files, test/test_flake_pattern_ratchet.py (60) and semgrep scan --config semgrep/ (1.78.0, diff baseline) all pass. local-gate.py's backend cross-surface set reports 61 failed and 15 errors on this host. A clean origin/main worktree with the same venv reports the same 61 and 15, so CI is the authority there.

Manual verification

N/A — unit coverage sufficient: the defect is a window between a DOM commit and React's passive-effect flush. A person cannot press a key inside it on demand, and the new tests force it exactly.

Screenshots / video

Why no screenshot: No pixel changes; only when two keydown listeners attach changes, and the dialogs render identically.

Related Issues

Closes #24282

Sibling sweep

Swept: git grep -nE "addEventListener\('keydown'" -- website/src (70 files), filtered to listeners that live in a dialog-like component which can open from a non-click update
Sibling: website/src/hooks/useDialogFocusTrap.ts:144 -- changed here
Sibling: website/src/components/UpdateFoundModal.tsx:336 -- tracked in #24283
Sibling: website/src/components/notifications/NotificationBanner.tsx:304 -- tracked in #24283
Sibling: website/src/apps/md-notebook/ConfirmDialog.tsx:64 -- not affected -- it opens only from a row's Delete click, and React flushes a click's passive effects within that same commit

Pattern harvest

Rule candidate: review-prompt
Pattern: "a dismissal or key listener for an overlay that can open from async data is attached in useEffect: React may paint before the passive flush, so attach it in useLayoutEffect"

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — N/A, no documented behaviour changes
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

Contributions are accepted under the project's Apache-2.0 license; see the Licensing section of CONTRIBUTING.md.

[github-actions]

UX Review (Opus 5.5, fork) — ✅ PASS

UX-level review of 476c26c5f8cacf7e768fe84a5db8f745b30ae4ef via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

UX-Verdict: PASS

Nothing to check. The diff only moves two Escape/Tab keydown listeners from useEffect to useLayoutEffect. It adds or changes no visible control, string or state, so no screenshot is needed.

[UX-REVIEWED] 476c26c

Description read

[DESCRIPTION-READ] 3df2ab1dbd5e95015002c854a781e2df39154bc49df4e64bf63d1335c5a23110

That sha256 names everything this verdict read: the PR description, plus the 3 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

[github-actions]

Design Review (Opus 5.5, fork) — ✅ PASS

Design-level review of 476c26c5f8cacf7e768fe84a5db8f745b30ae4ef via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

Design-Verdict: PASS

Nothing blocks. The fix targets the real cause: the listener attached in a passive effect, which can run after the dialog is already painted. Only the attach timing changes.

Notes

• Window listeners in the same phase run in the order they were added. A layout effect now registers Modal's bubble-phase Escape listener before any passive-effect keydown listener mounted in the same commit. Nested layers that rely on defaultPrevented should already handle Escape in the capture phase. A layer that mounts together with its Modal and listens in the bubble phase would no longer pre-empt it.

[DESIGN-REVIEWED] 476c26c

Description read

[DESCRIPTION-READ] dd5c4b5e25a5aded6edfca376f6b0f1d090ab9c3e9724d6fc305b8bb3f389d30

That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

[github-actions]

First Principles Review (Opus 5.5, fork) — ✅ PASS

Premise-level review of 476c26c5f8cacf7e768fe84a5db8f745b30ae4ef via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

First-Principles-Verdict: PASS

Nothing blocks. Two keydown listeners now attach in a layout effect, which fixes the gap between the dialog's commit and React's passive-effect flush. A test that fails on base shows the defect, the PR closes #24282, and the unfixed siblings are tracked in #24283.

What this change ships

Inventory (4 items) — 4 justified

Intent: Escape closes a dialog as soon as it appears, even when a network response opened it. FIX.

1. An Escape pressed the moment a Modal appears now closes it — justified
2. Escape and the Tab trap in useDialogFocusTrap dialogs work from the first frame — justified
3. Modal's scroll lock moves into its own passive effect, so its timing stays as on main — justified
4. Two regression tests force the yield between commit and passive flush — justified

[FIRST-PRINCIPLES-REVIEWED] 476c26c

Description read

[DESCRIPTION-READ] 0f0fc6aa0a1d1730c129659e0b208ecdf9f4a86fa3cf34ba24cdca1a73368988

That sha256 names the PR description this verdict read. Recompute it with .github/scripts/pr-description-capture.sh; if it differs, the description changed after this verdict was formed and any finding drawn from the description is unproven.

[github-actions]

GPT 6.1 Review (fork) — ✅ no blocking findings

Reviewed 476c26c5f8cacf7e768fe84a5db8f745b30ae4ef via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 476c26c

[github-actions]

Opus 5.5 Review (fork) — ✅ no blocking findings

Reviewed 476c26c5f8cacf7e768fe84a5db8f745b30ae4ef via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 476c26c

[bolichen97]

Reviewed at 476c26c.

The change is the smallest fix for the stated cause. Both keydown listeners now attach in a layout effect, which runs in the same commit that inserts the dialog. The handler bodies, deps and phases are unchanged, and the scroll lock stays in its own passive effect.

What I checked:

• The new tests press Escape from a MutationObserver callback. That callback is a microtask, and React runs layout effects synchronously inside the commit, so with the fix the listener is always attached first. The pass side is deterministic.
• Listener order: Modal's bubble listener now registers before any passive effect listener mounted in the same commit. The nested layers that consume Escape (InfoTip, AgentSelector, AgentSkillsEditor) listen in the capture phase and only while open, so they still run first and the defaultPrevented check still holds.
• 4 files, the linked issue is a bug, no security path is touched, and all review lanes pass on this head.

This looks good to ship. The remaining check is PR Readiness, which is still pending on this head because the backend and frontend test shards are running. I will approve after readiness passes on this same head.

[KiroCrewCaptain]

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, security checklist all-NO, no reviewer-lane findings, readiness signal fresh for this head SHA, declared category matches the diff. Category: a dialog's Escape listener now attaches in a layout effect, so it is armed in the commit that shows the dialog (closes #24282), with tests.