Skip to content

fix(webapp): make Esc close consistently across dialogs, sheets and panels - #4567

Closed
claude[bot] wants to merge 5 commits into
mainfrom
claude/modal-esc-close
Closed

fix(webapp): make Esc close consistently across dialogs, sheets and panels#4567
claude[bot] wants to merge 5 commits into
mainfrom
claude/modal-esc-close

Conversation

@claude

@claude claude Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Requested by Chris Arderne · Slack thread

Escape works nearly everywhere in the dashboard because every overlay is built on a library that dismisses on Escape by default. This PR fixes the handful of places where it doesn't, or where the UI says it does and it doesn't.

Before / After

Vercel setup modal (marketplace flow) — the modal is intentionally locked while you finish setup, but it still showed a close X and an Esc hint that did nothing. Now the close button and hint are hidden in that flow, so nothing is advertised that won't work. Dismissal stays blocked, as before.

Authenticator app setup — pressing Esc on the QR code step did nothing, and there was no close button. Now Esc (or the close button) cancels setup, the same as the Cancel button and the same as the Disable MFA dialog. Outside clicks stay blocked, as they were before. The recovery codes step stays deliberately non-dismissible: MFA is already on at that point and the codes are only shown once, so you still have to confirm you've saved them.

Chat panel — the only way to close it was the X in its header. Now Esc closes it too, and the header shows the Esc hint. Esc while the cursor is in the message box only leaves the box, so a half-typed message isn't lost to a stray Esc; a second Esc closes the panel.

Side sheets — there are 5 of them, and the close affordance was inconsistent: the shortcuts sheet and the alerts sheet each showed a close button with an Esc hint, but built separately in different places, while the two schedule sheets (create and inspect) had no visible close button at all. Now the button and hint come from one place in SheetContent, so those two gain one and the other two keep theirs unchanged. The query editor sheet opts out — its own nav bar already has an Esc-labelled Cancel.

A dialog on top of a panel — opening the Cancel run dialog from the run inspector and pressing Esc closed the dialog and the inspector behind it. Now Esc only closes the dialog; a second Esc closes the panel. The same applies to dropdowns and filter menus opened inside a panel.

How

The Esc hint on sheets moved out of SheetTitle and into a shared close button in SheetContent, mirroring how DialogContent already works, with a showCloseButton opt-out for the one sheet whose own nav bar already has an Esc-labelled Cancel button.

That button is absolutely positioned over the sheet's content, so headers underneath it have to reserve room. SheetContent publishes whether it rendered one through context (useSheetHasCloseButton) and those headers read it, rather than each one guessing. UpsertScheduleForm and ScheduleInspector need this because both render inside a sheet and as standalone pages — reserving the space unconditionally left an empty gap on the standalone routes.

The double-close came from two independent document-level Escape listeners: the panel's Button/LinkButton shortcut and the dialog's own dismissal. Both fired on the same keypress. useShortcutKeys now takes the element the shortcut belongs to and skips the shortcut when that element sits behind an open overlay — either because the keypress came from a layered surface that doesn't contain it, or because the surrounding content has been marked aria-hidden by a modal.

Panel close shortcuts are registered on document, so they deliberately stay disabled while focus is in a text field: a page-level search box or the chat composer is not "inside" the panel as far as the listener is concerned, and firing from there closed the wrong thing. The focused field handles Esc itself instead, and a second Esc closes the panel.

✅ Checklist

  • I have followed every step in the contributing guide
  • The PR title follows the convention.
  • I ran and tested the code works

Testing

Typecheck (pnpm run typecheck --filter webapp), pnpm run format and pnpm run lint:fix all pass.

Exercised in a local dashboard: the shared sheet close button (including the loading and "schedule no longer exists" states of the schedule sheet, and the shortcuts sheet scrolled so its sticky title passes under the button), Esc closing a detail panel, Esc in a dropdown opened over a panel closing only the dropdown, and Esc in the chat composer leaving the draft intact.

The later useSheetHasCloseButton change is verified by typecheck and by reading the call sites, not by a second browser pass — it only switches a pr-14/pr-16 class on or off, and the sheet-side rendering it affects is what the pass above already covered.


Changelog

Esc now closes the authenticator app setup window and the chat panel, and side sheets now show a close button with their Esc shortcut. Esc in the chat panel's message box just leaves the box, so an unsent message survives and a second Esc closes the panel. Closing a dialog with Esc no longer also closes the panel behind it, and the Vercel setup modal no longer shows a close button that does nothing.


Notes for the reviewer

  • Esc from a plain form field inside a panel (say the schedule edit form) still does nothing — the field doesn't handle Esc and the panel's shortcut is off inside inputs. Making it close the panel needs the shortcut scoped to the panel's own subtree rather than document, which is a bigger change than this PR wants to be.
  • With both the chat panel and a detail panel (run, log, deployment…) open, one Esc closes both. Neither is an overlay over the other, so the guard above doesn't apply; ordering them would need a shared notion of which panel is "innermost".
  • app/components/primitives/Sheet.tsx (the v1 sheet) has zero importers — everything uses SheetV3. Left alone here, but it's dead code.
  • AlertDialog still has no close button or Esc hint. Both call sites already render an explicit Cancel button, and forcing an explicit choice is the point of an alert dialog, so adding an X looked like the wrong call rather than a cheap win.

…anels

- Vercel onboarding modal no longer renders a close button and Esc hint in
  the marketplace flow, where dismissal is intentionally blocked
- The authenticator app setup dialog can be dismissed with Esc or the close
  button; the recovery codes step stays deliberately non-dismissible
- The dashboard chat panel closes with Esc, using the same shortcut button
  the other detail panels use
- Every sheet gets the Esc hint from a shared close button in SheetContent
  instead of opting in via SheetTitle
- Panel close shortcuts fire while a form field is focused
- A shortcut is ignored while its button sits behind an open overlay, so one
  Esc no longer closes both a dialog and the panel behind it

Co-Authored-By: Claude <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Aug 11, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: e83beb4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@carderne
carderne marked this pull request as ready for review August 11, 2026 13:29
devin-ai-integration[bot]

This comment was marked as resolved.

- MFA setup: only Escape and the close button cancel the pending TOTP
  secret; outside clicks are prevented and onOpenChange guards on `open`.
- Make Esc-from-input a per-panel decision: drop it on the panels that
  host an editor or a form (waitpoint token, span, bulk-action inspector)
  and blur instead of closing when the agent composer holds a draft.
- Scope the overlay guard to Escape shortcuts so it no longer changes
  behaviour for every Button/LinkButton shortcut app-wide.
- Reserve room for the sheet's floating close button in the alerts and
  schedule inspector headers, and drop a vestigial `justify-between`.

Co-Authored-By: Claude <noreply@anthropic.com>
devin-ai-integration[bot]

This comment was marked as resolved.

The Esc close shortcuts are registered on `document`, so
`enabledOnInputElements: true` made them fire from every input on the page,
not just fields inside the panel. Typing in a page-level search box or the
chat composer and pressing Esc closed the panel behind it.

The composer and SearchInput tried to guard against this with
`stopPropagation` on the React `onKeyDown`, which cannot work: the app
hydrates into `document`, so React's delegated listener and the hotkey
listener are siblings on the same node and only
`stopImmediatePropagation` would stop the other one.

Drop the flag from the nine Esc close shortcuts instead, so the focused
field handles Esc itself: it clears a search box or leaves the composer with
the draft intact, and a second Esc closes the panel.

Verified in a browser against the seeded References org: Esc from a
page-level search box or a composer with a draft no longer closes the chat
panel, a second Esc does, and Esc still closes dialogs, sheets, detail
panels, and closes only an open dropdown when one is layered over a panel.

Co-Authored-By: Claude <noreply@anthropic.com>
devin-ai-integration[bot]

This comment was marked as resolved.

The note claimed Esc closes detail panels and works while typing in one of
their fields. Neither ships: sheets already closed on Esc before this branch,
and the panel close shortcuts stay disabled inside inputs. It also claimed
outside clicks no longer cancel the authenticator setup, which they never did.

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Observability map

As of e2a727d.

18/100 over 414 measured of 430 entry points (base 18, no change)

What this PR changed

route base head now failing
/_app/orgs/new 0 50

FIX FIRST

  • /api/v1/projects/:projectRef/envvars (sensitive) - auth-boundary, request-context
  • /auth/sso (sensitive) - auth-boundary, request-context
  • /_app/orgs/:organizationSlug/settings/team (sensitive) - error-classification, auth-scope, request-context

AUDIT 3 of 50 sensitive mutations record an actor. 47 without one.
CONTEXT 12 of 414 entry points name a tenant on a failure path. 325 appear only here, 39 of them sensitive, in the JSON rather than the fix list.

What the score is made of
CHECKS
  error-classification  169 applicable,  94 pass,   0 sole, global without it 10
  auth-boundary          62 applicable,  57 pass,   0 sole, global without it 15
  auth-scope             19 applicable,  17 pass,   0 sole, global without it 18
  request-context       414 applicable,  12 pass, 223 sole, global without it 63
  audit-trail            50 applicable,   3 pass,   0 sole, not in the score

The score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md.

UpsertScheduleForm and ScheduleInspector each render both inside a sheet
and as a standalone page. Reserving room for the sheet's floating close
button unconditionally left an empty gap on the standalone edit route.

SheetContent now publishes whether it rendered the button via context and
the headers that sit under it read that, which also replaces the "has
header actions" stand-in for "am I in a sheet".

Co-Authored-By: Claude <noreply@anthropic.com>
@carderne carderne closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants