Skip to content

🤖 fix: Escape closes the agent picker, sidebar drawer and notifications popover - #5736

Merged
ThomasK33 merged 5 commits into
mainfrom
fix-escape-handling
Oct 6, 2026
Merged

ThomasK33 merged 5 commits into
mainfrom
fix-escape-handling

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Summary

Escape now closes three layers that ignored it: the agent picker opened with Ctrl+Shift+A, the narrow-screen sidebar drawer, and the notifications popover (which used to reopen as a duplicate tooltip). A click on the notifications bell now only opens its settings and no longer changes them.

Fixes #5676
Fixes #5685
Fixes #5691

Refs #5701 (dropped from this PR, see Scope below)

Background

All of them came from the agent bug bash in #5670.

Implementation

  • Agent picker: the open commits with flushSync and focuses the list right away. The frame-deferred focus stays, because the command palette restores focus to the composer after its action runs.
  • New useEscapeToDismiss(enabled, onDismiss) hook for the drawer. It listens in the window's bubble phase, so open layers handle Escape first. Radix popovers, menus and dialogs stop Escape in the document capture phase. React handlers stop it with stopKeyboardPropagation. Document listeners call preventDefault. The hook also skips IME composition, modified Escape, terminals, remote desktops and open modal dialogs. 🤖 fix: agent bug bash (make bug-bash) and the bugs it found #5670 tried a capture-phase listener and reverted it, because that one took Escape from every popover, dialog and edit mode.
  • Stream interrupt is unchanged when no overlay is open. The interrupt listener (useAIViewKeybinds) is in the same phase and mounts first. It now yields Escape while an overlay is open (isEscapeDismissOverlayOpen()), so one Escape never closes an overlay and also stops a stream. When no overlay is open, the hook has no listener, so Escape behaves exactly as on main. Tests cover both cases: a plain composer, body, and a composer that opts in with data-escape-interrupts-stream.

Scope

The tutorial's Escape (#5701) is not in this PR. Three review rounds each found a new ordering case between the tutorial and the layers under it (the drawer, opened before or after the tutorial, and a picker opened by shortcut beneath the tutorial). Each fix needed more ordering mechanism, so I removed the tutorial part instead of growing it. #5701 stays open. A fix there needs a decision about which layer owns Escape while a tutorial covers the app.

Known limitation: when a tutorial shows over the open drawer, Escape closes the drawer behind the tutorial. On main, Escape does nothing in that case.

  • Drawer width: LeftSidebar now subscribes to its (max-width: 768px) media query. It used to read the query once per render, so a window narrowed into the drawer width on the new-workspace screen did not get Escape.
  • Notifications bell (🤖 Notifications bell: Escape on the popover opens a duplicate tooltip, and a click toggles the setting #5691, Option A approved by the coordinator): the click only opens the popover. The checkbox and the shortcut toggle the setting. The tooltip is a plain label ("Notifications", the current state, and the shortcut on desktop). The button's accessible name is now "Notifications", the same as the tooltip. aria-pressed is gone because the button no longer toggles anything. The docs page says to check the box.

Validation

Pre-fix failures (unit tests on main):

AgentModePicker > Escape closes the picker right after the open shortcut
  Expected: 0  Received: 3

New bug-bash repros in tests/bugbash/repros/escapeClosesOverlays.e2e.ts (4 tests, web and phone targets). I proved each one by restoring the old code in a scratch build. Each failed on its own assertion and passed after the restore:

Repro Old code result
picker (#5676) ASSERTION_FAILED: expect.not.toBeExpanded
drawer (#5685) ASSERTION_FAILED: expect.toHaveCount
bell click (#5691) ASSERTION_FAILED: expect.not.toBeChecked
bell Escape (#5691) ASSERTION_FAILED: expect.toHaveCount

The repros that read the bell's aria-pressed (notificationsShortcut, knownFailureDetailsShortcuts, creationFormForgetsName) now read the popover's checkbox through a shared helper. knownFailureDetailsShortcuts keeps its known-failure tag for #5671, which a later PR fixes. It reads the setting only at the end, because opening the bell popover would move focus off the details button. It still fails on its own assertion (expect.toBeChecked), and make test-bugbash-known-failures fails the same tests as on main. tests/ui/compaction enables notifications with the shortcut, because Radix popover content does not render in happy-dom.

Mutation check: with the interrupt's overlay check removed, useEscapeToDismiss.test.tsx fails.

Escape timing in the repros

A Radix popover ignores Escape for a short time after it opens. This window exists on main, too. The CI run on 387b350 sent Escape inside it, so Test / E2E (linux 1/4) failed. The repros now press Escape exactly once, after an in-page probe in tests/bugbash/repros/helpers.ts reports that the layer takes Escape. The probe needs no sleep, no retry and no product change.

Why Radix ignores the early Escape (@radix-ui/react-dismissable-layer 1.1.11): the layer registers in an effect (context.layers.add(node)), then sends dismissableLayer.update. It finds its own index only on the re-render that this event forces (force({})). Its Escape listener calls the handler from the last committed render, and a passive effect installs that handler. Until then index is -1, and Escape does nothing.

The probe proves each step:

  1. The test arms the probe before the open.
  2. On dismissableLayer.update, the probe checks that context.layers holds the popover element, and records the layer's force state.
  3. A stand-in React DevTools hook gets onPostCommitFiberRoot, which React calls after a commit's passive effects have run. The probe reports "ready" the first time the force state has changed there.

If Radix or React changes the internals that the probe reads, the wait fails with a named error and does not race.

notificationsShortcut.e2e.ts failed in CI for the same reason, although this change does not edit it. It calls expectNotifyOnAllResponses three times, and CI failed in that helper (helpers.ts:98, expect(setting).toBeHidden() right after its Escape). The helper now waits for the probe before its Escape. Ctrl+Shift+Comma opens no layer, so the shortcut presses need no wait.

Screenshots

Drawer at 390 px, Escape. Before:

Drawer still open after Escape

After (the drawer was open before Escape as in this shot, then closed):

Drawer open before Escape

Drawer closed after Escape

Bell after Escape. Before, a second copy of the settings:

Duplicate settings tooltip

After, a plain label:

Plain label tooltip

Bell popover at 390 px:

Notifications popover on a phone width

Risks

Low. Escape routing changes only while the narrow drawer is open. During that time Escape closes the overlay instead of interrupting a stream. Users who click the bell to toggle notifications now need the checkbox or the shortcut.


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

@mintlify

mintlify Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
Mux 🟢 Ready View Preview Oct 6, 2026, 1:10 PM

💡 Tip: Enable Automations to automatically generate PRs for you.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-06T15:02:12.279501Z 203b95a New commits
🔒 Security Review ✅ Completed 2026-10-06T15:03:08.462733Z 203b95a New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1af03c8c97

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/hooks/useEscapeToDismiss.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c51399154f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/hooks/useEscapeToDismiss.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 40624951a9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/components/TutorialTooltip/TutorialTooltip.tsx
@ThomasK33 ThomasK33 changed the title 🤖 fix: Escape closes the agent picker, sidebar drawer, tutorial and notifications popover 🤖 fix: Escape closes the agent picker, sidebar drawer and notifications popover Oct 6, 2026
…fication repros

A Radix DismissableLayer ignores Escape until it has re-rendered after registering and the
re-render's passive effects have run. CI sent Escape inside that window. The repros now arm an
in-page probe (a stand-in React DevTools hook plus the layer's fiber) and press Escape once,
when the probe reports ready.
@ThomasK33

Copy link
Copy Markdown
Member Author

Escape timing in the repros: evidence for the test-only push

Head: 203b95a8e6. The diff vs 387b3500a4 touches only tests/bugbash/repros/escapeClosesOverlays.e2e.ts and tests/bugbash/repros/helpers.ts. The PR body section "Escape timing in the repros" explains the readiness signal.

The window exists on main

Same fixture (tests/bugbash/e2e.config.ts, mock AI, --workers 1), same build (make build-main build-renderer build-static), same scratch probe test on both builds. Baseline is 033d612f3c (merge-base, #5722). Branch is 387b3500a4.

In-page probe, 3 rounds per target. "Lost" means the popover stayed open after Escape.

When Escape is sent after the open Baseline web Baseline phone Branch web Branch phone
Same tick lost 3/3 lost 3/3 lost 3/3 lost 3/3
After one microtask lost 3/3 lost 3/3 lost 3/3 lost 3/3
Right after the content mounts lost 3/3 lost 3/3 lost 3/3 lost 3/3
After one more task (0, 5, 16 or 50 ms) closed 3/3 closed 3/3 closed 3/3 closed 3/3

Repro timing (real tap, wait for visible content, one CDP Escape), 10 tries per target:

Build Target 1 lost Target 2 lost
Baseline 033d612f3c 1/10 8/10
Branch 387b3500a4 8/10 0/10

On both builds, every lost press had the probe at "not ready" at keydown. Every press that closed the popover had it at "ready".

Mutation check

I made the notifications popover ignore Escape (onEscapeKeyDown={(e) => e.preventDefault()}) and rebuilt. All 6 notification tests (3 tests on web and phone) failed with ASSERTION_FAILED after the readiness wait had passed: 4 on expect.toBeHidden, 2 on expect.toBeFocused. Then I restored the file (clean diff).

Green runs on the new head

  • escapeClosesOverlays + notificationsShortcut, web and phone: 6 runs, 10/10 tests each.
  • notificationsShortcut alone, web and phone: 10 runs, 2/2 tests each.
  • make test-bugbash-repros (mock AI, the CI path): 18/18 and 6/6.
  • make static-check: passed.

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

@ThomasK33

Copy link
Copy Markdown
Member Author

Readiness: ready with tracked follow-ups

Head: 203b95a8e65433d02a5c36ea946669542910a2fb


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

@ThomasK33
ThomasK33 added this pull request to the merge queue Oct 6, 2026
Merged via the queue into main with commit fa30fa4 Oct 6, 2026
60 of 63 checks passed
@ThomasK33
ThomasK33 deleted the fix-escape-handling branch October 6, 2026 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant