From 584139568d15478acb0d97254f17164fdb5de679 Mon Sep 17 00:00:00 2001
From: ChristmasSun <vedanand@stanford.edu>
Date: Fri, 7 Aug 2026 17:06:49 -0700
Subject: [PATCH] fix(server): match keybinding rules whose key uses an alias
 spelling

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.
---
 apps/server/src/keybindings.test.ts | 23 +++++++++++++++++++++++
 apps/server/src/keybindings.ts      | 20 +++++++++++++++-----
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/apps/server/src/keybindings.test.ts b/apps/server/src/keybindings.test.ts
index b674688f0410..aec04b467d28 100644
--- a/apps/server/src/keybindings.test.ts
+++ b/apps/server/src/keybindings.test.ts
@@ -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;
diff --git a/apps/server/src/keybindings.ts b/apps/server/src/keybindings.ts
index 10d98bf64290..e48280e4071f 100644
--- a/apps/server/src/keybindings.ts
+++ b/apps/server/src/keybindings.ts
@@ -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 {
+  const parsed = parseKeybindingShortcut(rule.key);
+  if (!parsed) return null;
+  return encodeShortcut(parsed);
 }
 
 function keybindingShortcutContext(rule: KeybindingRule): string | null {