Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ChristmasSun wants to merge 1 commit into
base: main
Choose a base branch
from
+38 −5
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions apps/server/src/keybindings.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,29 @@ it.layer(NodeServices.layer)("keybindings", (it) => {
}).pipe(Effect.provide(makeKeybindingsLayer())),
);

it.effect("replaces a rule whose stored key uses an alias spelling", () =>
Effect.gen(function* () {
const { keybindingsConfigPath } = yield* ServerConfig.ServerConfig;
yield* writeKeybindingsConfig(keybindingsConfigPath, [
{ key: "escape", command: "script.run-tests.run" },
]);
yield* Effect.gen(function* () {
const keybindings = yield* Keybindings.Keybindings;
// The settings UI renders a stored "escape" rule as "esc", so the
// replace target arrives spelled differently than it was persisted.
return yield* keybindings.upsertKeybindingRule({
key: "mod+m",
command: "script.run-tests.run",
replace: { key: "esc", command: "script.run-tests.run" },
});
});

const persisted = yield* readKeybindingsConfig(keybindingsConfigPath);
const persistedView = persisted.map(({ key, command }) => ({ key, command }));
assert.deepEqual(persistedView, [{ key: "mod+m", command: "script.run-tests.run" }]);
}).pipe(Effect.provide(makeKeybindingsLayer())),
);

it.effect("removes only the targeted custom keybinding", () =>
Effect.gen(function* () {
const { keybindingsConfigPath } = yield* ServerConfig.ServerConfig;
Expand Down
20 changes: 15 additions & 5 deletions apps/server/src/keybindings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,21 @@ export const ResolvedKeybindingsFromConfig = Schema.Array(ResolvedKeybindingFrom
);

function isSameKeybindingRule(left: KeybindingRule, right: KeybindingRule): boolean {
return (
left.command === right.command &&
left.key === right.key &&
(left.when ?? undefined) === (right.when ?? undefined)
);
if (left.command !== right.command) return false;
if ((left.when ?? undefined) !== (right.when ?? undefined)) return false;
if (left.key === right.key) return true;
// A key can be spelled more than one way ("esc"/"escape", "space"/" "), and
// the settings UI renders a stored rule back as the alias. Comparing raw
// strings would then fail to match a rule against itself, so replacing that
// rule would leave the original behind instead of updating it.
const leftKey = canonicalKeybindingKey(left);
return leftKey !== null && leftKey === canonicalKeybindingKey(right);
}

function canonicalKeybindingKey(rule: KeybindingRule): string | null {

@macroscopeapp macroscopeapp Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

@ChristmasSun ChristmasSun Aug 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@macroscopeapp macroscopeapp Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const parsed = parseKeybindingShortcut(rule.key);
if (!parsed) return null;
return encodeShortcut(parsed);
}

function keybindingShortcutContext(rule: KeybindingRule): string | null {
Expand Down
Loading