fix(server): match keybinding rules whose key uses an alias spelling - #5668
fix(server): match keybinding rules whose key uses an alias spelling#5668ChristmasSun wants to merge 1 commit into
Conversation
A key can be written more than one way: parseKeybindingShortcut normalizes "esc" to "escape" and "space" to " ". The settings UI renders a stored rule back using the alias, so editing a rule bound to Escape sent a replace target of "esc" for a rule persisted as "escape". Rule comparison used raw string equality, so the target never matched, the original rule survived, and the command ended up bound twice. Compare the parsed shortcut when the raw keys differ. No default binding uses an aliased key today, which is why this has gone unnoticed.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| return leftKey !== null && leftKey === canonicalKeybindingKey(right); | ||
| } | ||
|
|
||
| function canonicalKeybindingKey(rule: KeybindingRule): string | null { |
There was a problem hiding this comment.
🟠 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:
In file @apps/server/src/keybindings.ts around line 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.
There was a problem hiding this comment.
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/ServerRemoveKeybindingInputuseKeybindingValue - 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.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
ApprovabilityVerdict: 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. |
1 participant
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.
parseKeybindingShortcutnormalizes"esc"→"escape"and"space"→" ", while the settings UI renders a stored shortcut back using the alias:So editing a rule bound to Escape sends a replace target of
"esc"for a rule persisted as"escape". Comparison used raw string equality:"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:keybindings.json, bind any command toescape(orspace— the other aliased key).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
escapeand replaces it targetingesc. It fails onmainand passes with this change.before (keybind duplicates after trying to change off of escape):
after: (doesnt duplicate as is desired)
Checklist