Repository navigation
Conversation
github-actions
Bot
added
vouch:trusted
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| event.stopPropagation(); | ||
| if (!event.repeat) void onInterrupt(); | ||
| }; | ||
| window.addEventListener("keydown", handler); |
There was a problem hiding this comment.
🟡 Custom stop shortcuts fail in editors
With a custom thread.stop chord, focused controls that stop bubbling prevent the shortcut from firing. The former capture listener handled these chords before target handlers, so users cannot stop active turns from those controls.
Learn more
The bubble listener is needed for the new unmodified Escape default because contextual UI gets first refusal. However, the change routes every binding for thread.stop through that listener. Some focused controls stop keydown propagation without regard to the resolved command, such as AssistantCitationCommentEditor, so the event never reaches window in the bubble phase. The previous capture handler received the same custom chord before these target handlers.
Example: A user binds thread.stop to mod+x and focuses the citation comment editor while a turn runs. Pressing mod+x is stopped by the editor and never reaches this listener, while the stop button remains available.
Recommended fix: Defer only the unmodified Escape binding to the bubble phase. Continue handling other resolved thread.stop bindings in the capture listener, or use a capture listener that schedules Escape interruption after propagation and cancels it when the event becomes claimed.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 38af18a. Only Escape-keyed thread.stop bindings wait for the bubble phase now. Other custom chords are handled in the capture-phase shortcut handler again, so controls that stop propagation can't swallow them.
| { key: "mod+alt+shift+t", command: "themeEditor.toggle" }, | ||
| { key: "mod+s", command: "composer.stash", when: "!terminalFocus" }, | ||
| { key: "mod+shift+enter", command: "thread.steerQueuedMessage", when: "!terminalFocus" }, | ||
| { key: "escape", command: "thread.stop", when: "!terminalFocus" }, |
There was a problem hiding this comment.
🟡 Existing Escape bindings become shadowed
After upgrade, an existing escape binding with a non-identical condition is shadowed by thread.stop outside terminals. Startup appends the new default, and last-match resolution selects it. The user's existing Escape action no longer runs.
Learn more
Startup backfills a newly added default unless another rule has the same serialized shortcut and exactly the same when string. An existing unconditional Escape binding therefore does not conflict with escape plus !terminalFocus. syncDefaultKeybindingsOnStartup appends the new default after existing rules, while resolveShortcutCommand searches from the end. The new rule consequently wins everywhere outside terminals, even when no turn is running.
Example: A user already has { "key": "escape", "command": "script.cancel-preview.run" }. After upgrading, startup appends { "key": "escape", "command": "thread.stop", "when": "!terminalFocus" }; Escape now resolves to thread.stop throughout the chat and never runs the script.
Recommended fix: During default migration, treat same-key rules with overlapping when conditions as conflicts, or avoid backfilling this default when any existing unmodified Escape rule can match outside terminals. Add a server migration test covering an unconditional existing Escape binding.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 38af18a. Startup backfill now treats an unconditional rule on the same key as a conflict, so an existing unconditional escape rule keeps the default from being appended. At runtime, mergeWithDefaultKeybindings puts retained defaults before custom rules, so the user's rule still wins. Added a server test for this.
There was a problem hiding this comment.
Reverted in 1b9ce41. An existing unconditional escape rule can only come from a hand-edited keybindings.json: the Settings recorder treats Escape as cancel, and no earlier default used Escape. Appended defaults shadowing looser user rules is how keybinding backfill works in general, not something this PR introduces. If it's worth changing, it should be fixed in the backfill logic for all defaults, separately.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
jagrat7
changed the title
jagrat7
changed the title
Thread transfer impact
This comment will update automatically after the next completed run. |
jagrat7
changed the title
| // Keybindings run before the composer, so a bare Escape binding can take Escape from Stop shortcut. | ||
| const hasEscapeBinding = | ||
| settings.stopShortcut === "escape" && | ||
| (environment?.serverConfig?.keybindings ?? []).some( |
There was a problem hiding this comment.
If Settings is scoped to an environment other than the primary one, this checks the selected environment’s bindings while chat uses the primary environment’s bindings. The Stop shortcut row can miss a binding that takes Escape from the composer, or warn about one that does not.
jagrat7
changed the title
| (environment?.serverConfig?.keybindings ?? []) | ||
| .filter((binding) => shortcutToKeybindingInput(binding.shortcut) === "esc") | ||
| .map((binding) => commandLabel(binding.command)), |
There was a problem hiding this comment.
A bare Escape binding conditioned on terminalFocus cannot take Escape from a focused composer, but this warning still names its command as a conflict. Users may change a binding that does not interfere with Escape to stop. The Keybindings row warning also checks the key without considering the binding’s condition.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| <EscapeToStopConflictWarning | ||
| labels={escapeToStop && editor.keyDraft === "esc" ? ["Escape to stop"] : []} | ||
| /> |
There was a problem hiding this comment.
With Escape to stop off, an existing bare Escape binding no longer shows the warning that it can take Escape away from dialogs and menus. A binding loaded from a hand-edited keybindings file can still be handled before those controls receive the key, so users lose notice of a conflict unrelated to the new switch. The same change affects the new-binding row.
5fedfc2 to
ad6168f
Compare
| if (phase !== "running" || !settings.escapeToStop) return false; | ||
| if (!event.repeat) onInterrupt(); |
There was a problem hiding this comment.
When Escape to stop is enabled and a turn is running, pressing Ctrl+Escape in the focused composer can stop the turn if no keybinding consumes it first. The handler checks for Escape but not for modifier keys, so a shortcut other than bare Escape can unexpectedly interrupt the agent.
| if (phase !== "running" || !settings.escapeToStop) return false; | |
| if (!event.repeat) onInterrupt(); | |
| if (event.altKey || event.ctrlKey || event.metaKey || event.shiftKey) return false; | |
| if (phase !== "running" || !settings.escapeToStop) return false; | |
| if (!event.repeat) onInterrupt(); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
ad6168f to
112878b
Compare
This branch has not been deployed
1 participant
Problem
There was no quick keyboard way to stop a running turn.
thread.stopexists as a keybinding command but has no default key. Binding it to Escape doesn't work well either: the stop handler listens on the whole window and runs before anything else, so Escape stops the agent even when you meant to close a menu or dialog.Fix
A new Settings → General → Escape to stop switch sits under Send shortcut. It's off by default.
Escape is handled in the composer's key handler, the same place Enter to send is. In the composer:
/or@suggestion menu is open, Escape closes it. This already worked.Because it only works while the composer has focus, dialogs, menus, and other inputs keep Escape. Escape during IME composition is left alone, and holding Escape stops the turn only once.
thread.stopis unchanged and still has no default key.While Escape to stop is on, a keybinding on bare Escape shows "Conflicts with Escape to stop." in Settings → Keybindings, and the Escape to stop row shows "Conflicts with ." Keybindings run before the composer, so they would take Escape first.
Added a line to the composer controls section of
docs/user/keybindings.md.Web and desktop share the composer. Mobile has no Escape key, so it's unchanged. The stop path is the same for every provider, and nothing crosses the wire.
Verification
apps/webtypecheck is clean.vp test runforkeybindings.test.ts,KeybindingsSettings.logic.test.ts,settingsSearch.test.ts, andpackages/contracts/src/settings.test.tspasses.Done by Claude Opus 5.5 (1M context) in Claude Code, running inside T3 Code.
🤖 Generated with Claude Code
The PR appears safe to merge, though the outstanding warning and modified-Escape behavior and the lost regression coverage merit follow-up.
Summary
The PR adds an opt-in Escape-to-stop composer setting, conflict warnings in Settings, search and documentation entries, and a persisted settings field. The latest changes simplify conflict detection and remove several setting-specific tests.
Reviews (15) · Last reviewed commit: "feat(web): add an Escape to stop setting..."