fix(hotkeys): make copy line up/down hotkeys editable#10241
Open
Abdulrehman-PIAIC80387 wants to merge 1 commit into
Open
fix(hotkeys): make copy line up/down hotkeys editable#10241Abdulrehman-PIAIC80387 wants to merge 1 commit into
Abdulrehman-PIAIC80387 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Copy line(s) up/down were listed in the Keyboard Shortcuts panel but marked `editable: false`, because they were served by CodeMirror's built-in `defaultKeymap` and so never consulted marimo's hotkey config. Route both commands through the existing override mechanism already used by `toggleComment`/`toggleBlockComment`: add them to OVERRIDDEN_COMMANDS so the hardcoded bindings are filtered out of the default keymap, then re-bind them in `overrideKeymap` using the configured hotkey. Default keybindings are unchanged. Closes marimo-team#7056
Abdulrehman-PIAIC80387
force-pushed
the
fix/editable-copy-line-hotkeys-7056
branch
from
July 20, 2026 10:07
7c1ea93 to
f8e6a53
Compare
Author
|
I have read the CLA Document and I hereby sign the CLA |
Author
|
recheck |
Contributor
There was a problem hiding this comment.
Pull request overview
Enables user customization of the editor hotkeys for CodeMirror’s built-in “copy line up/down” commands by filtering their hardcoded default bindings and re-binding them through marimo’s hotkey provider, and by marking these actions as editable in the shortcuts UI.
Changes:
- Remove the
editable: falserestriction forcell.copyLineUp/cell.copyLineDownso they can be customized in the Keyboard Shortcuts panel. - Override CodeMirror’s
defaultKeymapbindings forcopyLineUp/copyLineDownand re-bind them usingkeymap.getHotkey(...)so user overrides take effect. - Add a focused unit test to assert that user overrides (e.g.,
Ctrl-d) propagate into the resulting CodeMirror keymap bindings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| frontend/src/core/hotkeys/hotkeys.ts | Makes “Copy line(s) up/down” editable by removing the explicit non-editable flags. |
| frontend/src/core/codemirror/keymaps/keymaps.ts | Filters out CodeMirror’s hardcoded bindings for copyLineUp/down and re-binds them via marimo’s configured hotkeys. |
| frontend/src/core/codemirror/keymaps/tests/keymaps.test.ts | Adds a regression test ensuring hotkey overrides actually reach the CodeMirror keymap for copyLineUp/down. |
Comment on lines
255
to
259
| "cell.copyLineUp": { | ||
| name: "Copy line(s) up", | ||
| group: "Editing", | ||
| key: "Alt-Shift-ArrowUp", | ||
| editable: false, | ||
| }, |
Member
|
@Abdulrehman-PIAIC80387 Can you please attach a demo video. I'll be happy to approve after that |
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.
This pull request was authored by a coding agent.
📝 Summary
Closes #7056
Copy line(s) up/Copy line(s) downappear in the Keyboard Shortcuts panel but are markededitable: false, so the panel advertises two rows it silently refuses to customize.That flag was a deliberate stopgap in #6843, which said as much:
The commands are served by CodeMirror's built-in
defaultKeymap, which hardcodesShift-Alt-Arrow, so marimo's hotkey config never reached them. @mscolnick invited a follow-up FR for this in #6338 ("we can look into making it editable. mind filing a FR?"), which became #7056.🔍 Description of Changes
Uses the override mechanism already established in
keymaps.tsfortoggleComment/toggleBlockComment— no new machinery:frontend/src/core/codemirror/keymaps/keymaps.ts— addcopyLineUp/copyLineDowntoOVERRIDDEN_COMMANDS(filters the hardcoded bindings out of the default keymap), then re-bind them inoverrideKeymap()viakeymap.getHotkey(...)so they follow user config.frontend/src/core/hotkeys/hotkeys.ts— removeeditable: falsefrom both entries.Default keybindings are unchanged — only the ability to reconfigure them is added. Applies to both the
defaultandvimpresets, sincekeymapBundleinstallsoverrideKeymapfor each.Both directions are included because the issue asks for both and they share one mechanism.
🧪 Testing
make fe-check— cleanpnpm vitest run src/core/codemirror src/core/hotkeys— 660 passed, 1 skipped (47 files)setup.test.tsis unchanged at 18 duplicates — no new conflicts.The pre-existing generic assertions in
keymaps.test.ts(overrideKeymap().length === OVERRIDDEN_COMMANDS.size, and no overridden command surviving in the filtered keymap) picked up the two new commands automatically.Added a test asserting an override to
Ctrl-Dactually reaches the keymap — the exact rebinding @mullimanko asked for. It guards the user-facing behavior rather than the plumbing: reverting theoverrideKeymaphalf while leavingeditableon would still pass the generic tests, but fails this one.Opening as a draft. This issue carries neither
readynorneeds discussion, and CONTRIBUTING says to treat unlabeled issues as needing discussion — so please read this as a concrete proposal awaiting your go-ahead, not a review request. Happy to close it if you'd prefer this waited or went a different way.Demo media for the settings-panel change is still to come before this would be marked ready.
📋 Pre-Review Checklist
✅ Merge Checklist