fix(ui): stop Cmd+W deleting a thread out from under a dialog (#2474) - #2551
Merged
Conversation
Cmd/Ctrl+W deletes the active thread. Nothing checked whether the user was looking at a dialog when they pressed it, so closing Settings with the keystroke that closes things everywhere else destroyed a conversation instead. There is no undo. The guard reads the DOM — is any `<dialog>` open — rather than asking a list of `isXOpen()` predicates. The renderer has seventeen dialogs and the one hand-maintained list of them, three lines above this in the Cmd/Ctrl+F handler, named four: settings, the command palette, file search, keyboard shortcuts. That list is the failure mode the issue anticipates when it says "or any other dialog for that matter", so the fix is shaped so a new dialog is covered the moment it exists. The find bar now uses the same check, which is a behaviour change worth noticing: it also defers to the thirteen the list missed — an approval, an SSH passphrase, a confirm — each a question it should not open underneath. `show()` counts as well as `showModal()`. The approval prompt appears inline over the chat rather than modally, and it is still something the user is answering. `preventDefault()` deliberately stays outside the guard. It is what keeps the keystroke from also reaching macOS's File ▸ Close accelerator, and moving it inside would hand Cmd+W back to the menu exactly when a dialog is up. So the keystroke is still swallowed; only the delete is skipped. Doing nothing is the right answer rather than closing the dialog for them — Esc already does that, in the handler immediately below, and every dialog honours it. Left alone deliberately: the shortcut is matched inline as `meta && e.key === 'w'` rather than through a `matchXShortcut` helper like its neighbours, so Caps Lock (which makes `e.key` `'W'`) silently disables it. Routing it through a matcher would be tidier, but it would also widen which keystrokes delete a thread, and that is the wrong direction to move a destructive shortcut without being asked. main.ts binds its shortcuts inside a boot function with no seam to call, so the wiring is pinned at the source level — the file's existing test does the same for the layout boot — while the guard itself is unit-tested against real `<dialog>` elements. The wiring assertions check the property rather than the text: every `confirmDeleteThread()` call site is guarded, not just the one that exists today. Verified locally: 8641 tests, 8631 pass, 10 skipped, 0 fail, plus typecheck, lint, format, dead-code, oracle, e2e-syntax and demo:site. All three wiring assertions were confirmed to fail with the guard removed. Refs #2474 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hpk1gEma9LhMUuvr2TwUj6
jonathanKingston
force-pushed
the
claude/dialog-guard-cmd-w-2474
branch
from
September 7, 2026 18:41
c97d6a0 to
79f212a
Compare
jonathanKingston
marked this pull request as ready for review
September 7, 2026 18:41
Contributor
🖥️ PR preview
|
Contributor
Reference screenshots ready for reviewReview GitHub’s image diffs in screenshot PR #2562. Rendered for If this source branch moves, a later successful render closes the stale review PR and replaces this link. |
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.
Closes #2474 (P2,
area:ui).The bug
Cmd/Ctrl+W deletes the active thread. Nothing checked whether a dialog was on screen, so closing Settings with the keystroke that closes things everywhere else destroyed a conversation instead. There is no undo.
The fix
The guard reads the DOM — is any
<dialog>open — rather than asking a list ofisXOpen()predicates.That shape is deliberate. The renderer has seventeen dialogs, and the one hand-maintained list of them sits three lines above this in the Cmd/Ctrl+F handler, naming four: settings, the command palette, file search, keyboard shortcuts. That list is exactly the failure the issue anticipates when it says "or any other dialog for that matter", so the check is built so a new dialog is covered the moment it exists, with nobody having to remember to register it.
A behaviour change worth noticing: the find bar now uses the same check, so it also defers to the thirteen dialogs the list missed — an approval, an SSH passphrase, a confirm. Each is a question it should not open underneath, but it is a change beyond the reported bug, so push back if you'd rather I left the four-item list alone.
show()counts as well asshowModal(): the approval prompt appears inline over the chat rather than modally, and it is still something the user is answering.preventDefault()stays outside the guardIt is what keeps the keystroke from also reaching macOS's File ▸ Close accelerator (
app-menu-file-items.tsregisters{ role: 'close' }there). Moving it inside would hand Cmd+W back to the menu exactly when a dialog is up. So the keystroke is still swallowed; only the delete is skipped.Doing nothing is the right answer rather than closing the dialog for them — Esc already does that, in the handler immediately below, and every dialog honours it.
Left alone deliberately
The shortcut is matched inline as
meta && e.key === 'w'rather than through amatchXShortcuthelper like its neighbours inkeyboard-shortcuts.ts. One consequence: with Caps Lock on,e.keyis'W'and the shortcut silently does nothing.Routing it through a matcher would be tidier and testable in the same place as its siblings — but it would also widen which keystrokes delete a thread, and that is the wrong direction to move a destructive shortcut without being asked. Happy to do it if you want it.
Testing
main.tsbinds its shortcuts inside a boot function with no seam to call, so the wiring is pinned at the source level — the file's existing test does the same for the layout boot — while the guard itself is unit-tested against real<dialog>elements in happy-dom.The wiring assertions test the property, not the text: every
confirmDeleteThread()call site is guarded, so a second unguarded one added later fails the suite rather than slipping past an assertion that only looked at the first.node scripts/run-tests.mts(whole repo)tsc --noEmit(node + web)pnpm run lint/oxfmt --check .check:dead-code(840 modules) /check:oracle(243 specs, 15 invariants)check:e2e-syntax(269 files) /demo:site:check9 new tests: 5 on
isAnyDialogOpen(closed dialog, modal, non-modal, several at once, and one inserted as raw HTML that nothing registered), 3 on the Cmd+W / Cmd+F wiring, and the existing suite unchanged.Two local-environment notes, neither affecting CI: reaching a full run needs an uncommitted shim over node-pty's native loader (it cannot be rebuilt in this sandbox — the agent proxy denies
iojs.organdwww.electronjs.org), reverted before committing; and this container'snode_modulespredates #2383, sopatches/@anthropic-ai__sandbox-runtime@0.0.74.patchwas not applied andgit-commit-signing.test.tsfailed until I applied its one-line hunk by hand. That failure reproduces identically on52c839awith my changes stashed, so it is a stale install here, not a base-branch break.Refs #2474
🤖 Generated with Claude Code
https://claude.ai/code/session_01Hpk1gEma9LhMUuvr2TwUj6
Generated by Claude Code