Sign up
View stargazersSign up

fix(server): match keybinding rules whose key uses an alias spelling#5668

closed1
main
GitHub fork: fix/keybinding-alias-replacement
openedAug 8, 2026byCChristmasSun
2 files changed
View on GitHubTwo-way sync enabledPR last imported 12d ago

What changed

Keybinding rule comparison now falls back to comparing the parsed shortcut when two rules spell their key differently.

Why

A key can be written more than one way. parseKeybindingShortcut normalizes "esc" → "escape" and "space" → " ", while the settings UI renders a stored shortcut back using the alias:

ts
1// apps/web/src/components/settings/KeybindingsSettings.logic.ts:52
2parts.push(shortcut.key === " " ? "space" : shortcut.key === "escape" ? "esc" : shortcut.key);

So editing a rule bound to Escape sends a replace target of "esc" for a rule persisted as "escape". Comparison used raw string equality:

ts
1// apps/server/src/keybindings.ts
2left.command === right.command && left.key === right.key && ...

"esc" !== "escape", so the replace target never matches, the original rule survives alongside the new one, and the command ends up bound twice.

No default binding currently uses an aliased key, which is why this has gone unnoticed — every shipped default is a mod+… combination whose stored and rendered spellings are identical.

How to reproduce

Note that the recorder cannot capture bare Escape: it uses Escape to cancel recording (KeybindingsSettings.tsx:807), which is the behavior you want. So reaching this state today means writing the rule to disk directly:

  1. In keybindings.json, bind any command to escape (or space — the other aliased key).
  2. In Settings → Keybindings, edit that row and record a different shortcut.
  3. Reopen Settings → Keybindings.

The command now appears twice: once on the original Escape and once on the new shortcut.

This is reachable without hand-editing anything as soon as a default binding uses one of those keys — see #5669, which is how I ran into it.

Test

Added a case that persists a rule as escape and replaces it targeting esc. It fails on main and passes with this change.

before (keybind duplicates after trying to change off of escape): image

after: (doesnt duplicate as is desired) image

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • Regression test that fails without the fix

Added 1 commit

  • fix(server): match keybinding rules whose key uses an alias spelling5841395
CcoderabbitaiBotAug 8, 2026
Synced from GitHub

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 134ac7ab-d0e0-4de5-928c-25469a57be71

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

apps/server/src/keybindings.tsline 115Open thread
……@@ -101,11 +101,21 @@ export const ResolvedKeybindingsFromConfig = Schema.Array(ResolvedKeybindingFrom
112+ return leftKey !== null && leftKey === canonicalKeybindingKey(right);
113+}
114+
115+function canonicalKeybindingKey(rule: KeybindingRule): string | null {
MmacroscopeappBoton line 115Aug 8, 2026

🟠 High src/keybindings.ts:115

canonicalKeybindingKey does not canonicalize the " " (literal space) alias for the space key. parseKeybindingShortcut trims each token, so rule.key === " " is parsed as the "+" key, while "space" parses and re-encodes as "space". As a result, isSameKeybindingRule treats "space" and " " as different rules, so an upsert or remove that uses one alias when the stored rule uses the other fails to match the existing entry and leaves the old binding persisted. Consider normalizing the space alias before or within parseKeybindingShortcut so both spellings produce the same canonical key.

🤖 Copy this AI Prompt to have your agent fix this:
text
1In file @apps/server/src/keybindings.ts around line 115:
2
3`canonicalKeybindingKey` does not canonicalize the `" "` (literal space) alias for the space key. `parseKeybindingShortcut` trims each token, so `rule.key === " "` is parsed as the `"+"` key, while `"space"` parses and re-encodes as `"space"`. As a result, `isSameKeybindingRule` treats `"space"` and `" "` as different rules, so an upsert or remove that uses one alias when the stored rule uses the other fails to match the existing entry and leaves the old binding persisted. Consider normalizing the space alias before or within `parseKeybindingShortcut` so both spellings produce the same canonical key.
CChristmasSunAug 8, 2026

Good catch on the helper in isolation, but I believe this is unreachable through any validated input, so I've left it out to keep the diff minimal.

Every path into isSameKeybindingRule carries keys that passed KeybindingValue, which is TrimmedString.check(isMinLength(1), …) — decode trims, so a literal " " key becomes "" and fails validation before comparison:

  • RPC upsert / replace targets: ServerUpsertKeybindingInput / ServerRemoveKeybindingInput use KeybindingValue
  • persisted config: entries decode through KeybindingRule, which uses the same schema, so a hand-edited " " is dropped as a config issue

That leaves "space" as the only representable spelling — and unlike escape/esc, it round-trips losslessly: shortcutToKeybindingInput renders the parsed space back as "space", so stored and replace-target spellings always agree. The escape/esc pair was the only alias where the round-trip is lossy, which is what this PR fixes.

Happy to add the defensive normalization if maintainers prefer it.

MmacroscopeappBotAug 8, 2026

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

MmacroscopeappBotAug 8, 2026
Synced from GitHub

Approvability

Verdict: Needs human review

1 blocking correctness issue found. This keybinding alias fix has a High-severity unresolved finding identifying that the space key alias (" ") won't be properly canonicalized, which would leave the same bug pattern for that specific alias. Additionally, the author is a new contributor to this file.

You can customize Macroscope's approvability policy. Learn more.

JJulius MarmingeSep 7, 2026

Closing as part of the open-PR backlog sweep (wave 1).

Reason: Keybinding matching moved to shared; this PR targets old server code Related: #10069

Reopen if this is still wanted and you’re willing to rebase onto current main.