Conversation
Escape was hardcoded as the shortcut for entering command mode in the default keymap, so it could be neither remapped nor disabled. The vim keymap already routed through a configurable `command.vimEnterCommandMode` hotkey. Add a `command.enterCommandMode` action (default `Escape`) and use it in place of the hardcoded check. Overrides flow through the existing `keymap.overrides` config, so the shortcut can now be remapped, and an explicit empty override disables it. `OverridingHotkeyProvider` previously treated an empty override as "no override" and fell back to the default, which made disabling any hotkey impossible. Only `undefined` now falls back. The rest of the codebase already treats an empty key as disabled (`parseShortcut`, `KeyboardHotkeys`, duplicate detection). The shortcuts dialog hint now reflects the configured key, and is hidden when the shortcut is disabled. Note: the previous check was `evt.key === "Escape"`, which also fired when modifiers were held. `parseShortcut` rejects unlisted modifiers, so Shift/Ctrl+Escape no longer enters command mode. Fixes marimo-team#7426
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
kirangadhave
requested review from
kirangadhave
and
a lite review from Copilot
August 24, 2026 16:54
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the “enter command mode” shortcut configurable (defaulting to Escape) so users can remap it or disable it entirely via keymap.overrides, addressing issue #7426 about accidental command-mode activation.
Changes:
- Added a new
command.enterCommandModehotkey action (defaultEscape) and used it for non-vim command-mode entry instead of a hardcodedevt.key === "Escape"check. - Updated
OverridingHotkeyProviderto treat an explicit empty-string override ("") as “disabled”, falling back only when the override isundefined. - Updated the shortcuts dialog hint to reflect the configured shortcut and hide the hint when the shortcut is disabled (non-vim path), plus added tests for the new override semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| frontend/src/core/hotkeys/hotkeys.ts | Adds command.enterCommandMode and updates override resolution to allow disabling via empty string. |
| frontend/src/core/hotkeys/tests/hotkeys.test.ts | Adds tests covering disable/fallback/remap behavior for overrides. |
| frontend/src/components/editor/navigation/navigation.ts | Uses the configurable command.enterCommandMode shortcut for non-vim command-mode entry. |
| frontend/src/components/editor/controls/keyboard-shortcuts.tsx | Updates the shortcuts dialog hint to show/hide based on the configured command-mode shortcut. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
274
to
292
| const renderCommandGroup = (group: HotkeyGroup) => { | ||
| const isVim = config.keymap.preset === "vim"; | ||
| const commandModeKey = hotkeys.getHotkey("command.enterCommandMode").key; | ||
| // Nothing to advertise if the user has disabled the shortcut. | ||
| if (!isVim && commandModeKey === "") { | ||
| return renderGroup(group); | ||
| } | ||
| return renderGroup( | ||
| group, | ||
| <p className="text-xs text-muted-foreground flex items-center gap-1"> | ||
| Press{" "} | ||
| {config.keymap.preset === "vim" ? ( | ||
| {isVim ? ( | ||
| <> | ||
| <KeyboardHotkeys shortcut={isPlatformMac() ? "Cmd" : "Ctrl"} /> | ||
| <Kbd>Esc</Kbd> | ||
| </> | ||
| ) : ( | ||
| <Kbd>Esc</Kbd> | ||
| <KeyboardHotkeys shortcut={commandModeKey} /> | ||
| )}{" "} |
Member
There was a problem hiding this comment.
@iam-kira please address the review comment if PR is ready. I'll start my review after
The shortcuts dialog hardcoded the vim hint as Cmd/Ctrl+Escape. That is wrong on Windows, where `command.vimEnterCommandMode` defaults to `Shift-Escape`, and it ignored user overrides now that an empty override can disable a hotkey. Both presets now resolve their own action through the hotkey provider, so the hint follows the platform default and any override, and is omitted entirely when the resolved key is empty. This collapses the two branches into one path. Adds a test pinning the per-platform resolution of the vim binding, since the dialog advertises that key rather than a hardcoded guess.
Author
|
Addressed both presets now resolve their binding through the hotkey |
Author
|
Gentle bump before this goes stale, it's still ready whenever a maintainer has a moment. Review comments from the first pass are addressed and pushed. |
Member
|
Thanks for the bump @iam-kira I'll take a look at this today |
kirangadhave
requested changes
Sep 15, 2026
| if (evt.key === "Escape") { | ||
| handleEscape(); | ||
| } | ||
| } else if (commandModeShortcut(evt)) { |
Member
There was a problem hiding this comment.
This condition also disables Escape’s editor-cleanup behavior.
handleEscape() closes completion and signature hints before it enters command mode. CodeMirror removes its own Escape completion binding. After I remap the command-mode shortcut to q, Escape no longer closes the completion popup.
This breaks the workflow described in #7426. Escape must continue to dismiss temporary editor UI when command-mode entry is remapped or disabled.
Can we separate these behaviors? Keep Escape responsible for editor cleanup, and apply the configured shortcut only to the command-mode transition. Please add navigation tests for remapped and empty overrides with an open completion popup.
Clipboard-20260915-190359-460.mp4
I press Escape after the completion popup opens. The popup stays open because remapping command-mode entry also disables Escape’s cleanup behavior.
| } else if (commandModeShortcut(evt)) { | ||
| // For non-vim mode, the configurable shortcut (Escape by default) | ||
| // exits to command mode. An empty override disables it. | ||
| handleEscape(); |
Member
There was a problem hiding this comment.
Confirmed with command.enterCommandMode mapped to q.
When I select hello and press q, the cell stays in edit mode, the selection collapses, and the code becomes qhello.
When completion is open after mo., pressing q closes the old popup, inserts q, and opens a new popup for mo.q.
handleEscape() returns after simplifying the selection or closing completion. Because this branch does not prevent the event, CodeMirror processes the same key as text input.
Please call evt.preventDefault() as soon as the configured shortcut matches. Add regression tests for both a selected range and an open completion popup.
See video:
Clipboard-20260915-191431-695.mp4
First, q collapses the selection but also changes hello to qhello.
Then q closes the completion popup, inserts itself, and opens a new popup for mo.q.
When typing accordion, the completion popup filters, but when typing q it closes and opens up again.
Author
|
Gentle bump before this goes stale — it's still ready whenever a maintainer has a moment. Review comments from the first pass are addressed and pushed. |
Member
|
@iam-kira already requested changes with two major blocking issues |
Addresses review on marimo-team#10638: - Escape always dismisses temporary editor UI (completion/signature popups, multi-cursor selection), even when the command-mode shortcut is remapped to another key or disabled. Previously, remapping the shortcut away from Escape also disabled Escape's cleanup. - Call evt.preventDefault() as soon as the configured shortcut matches, so a remapped printable key (e.g. 'q') is no longer also typed into the editor. - Add navigation tests for remapped and empty (disabled) overrides with an open completion popup and a selected range. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
|
Thanks for the detailed review and the repros @kirangadhave — both addressed in the latest push:
Added navigation tests covering remapped ( |
Bundle ReportChanges will increase total bundle size by 17.35kB (0.07%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: marimo-esmAssets Changed:
Files in
Files in
Files in
|
kirangadhave
self-requested a review
September 23, 2026 18:52
This branch was successfully deployed
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.
Fixes #7426
Escape was hardcoded as the shortcut for entering command mode in the default keymap, so it could be neither remapped nor disabled. The vim keymap already routed through a configurable command.vimEnterCommandMode hotkey.
This adds a command.enterCommandMode action (default Escape) and uses it in place of the hardcoded check. Overrides flow through the existing keymap.overrides config, so the shortcut can be remapped, and an explicit empty override disables it.
OverridingHotkeyProvider previously treated an empty override as "no override" and fell back to the default, which made disabling any hotkey impossible. Only undefined now falls back. The rest of the codebase already treats an empty key as disabled (parseShortcut, KeyboardHotkeys, duplicate detection).
The shortcuts dialog hint now reflects the configured key and is hidden when the shortcut is disabled.
Behavior change: the previous check was evt.key === "Escape", which also fired when modifiers were held. parseShortcut rejects unlisted modifiers, so Shift/Ctrl+Escape no longer enters command mode. This matches how every other hotkey behaves, but flagging it explicitly.
Testing: 3 new tests covering disable, fallback, and remap. 120 tests pass across navigation.test.ts, core/hotkeys/, and useDuplicateShortcuts.test.ts.