Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
51 changes: 50 additions & 1 deletion apps/server/src/keybindings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ it.layer(NodeServices.layer)("keybindings", (it) => {
}).pipe(Effect.provide(makeKeybindingsLayer())),
);

it.effect("replaces existing custom keybinding for the same command", () =>
it.effect("appends additional custom keybindings for the same command", () =>
Effect.gen(function* () {
const { keybindingsConfigPath } = yield* ServerConfig;
yield* writeKeybindingsConfig(keybindingsConfigPath, [
Expand All @@ -368,6 +368,55 @@ it.layer(NodeServices.layer)("keybindings", (it) => {
});
});

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

it.effect("replaces only the targeted custom keybinding", () =>
Effect.gen(function* () {
const { keybindingsConfigPath } = yield* ServerConfig;
yield* writeKeybindingsConfig(keybindingsConfigPath, [
{ key: "mod+r", command: "script.run-tests.run" },
{ key: "mod+shift+r", command: "script.run-tests.run" },
]);
yield* Effect.gen(function* () {
const keybindings = yield* Keybindings;
return yield* keybindings.upsertKeybindingRule({
key: "mod+alt+r",
command: "script.run-tests.run",
replace: { key: "mod+r", command: "script.run-tests.run" },
});
});

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

it.effect("removes only the targeted custom keybinding", () =>
Effect.gen(function* () {
const { keybindingsConfigPath } = yield* ServerConfig;
yield* writeKeybindingsConfig(keybindingsConfigPath, [
{ key: "mod+r", command: "script.run-tests.run" },
{ key: "mod+shift+r", command: "script.run-tests.run" },
]);
yield* Effect.gen(function* () {
const keybindings = yield* Keybindings;
return yield* keybindings.removeKeybindingRule({
key: "mod+r",
command: "script.run-tests.run",
});
});

const persisted = yield* readKeybindingsConfig(keybindingsConfigPath);
const persistedView = persisted.map(({ key, command }) => ({ key, command }));
assert.deepEqual(persistedView, [{ key: "mod+shift+r", command: "script.run-tests.run" }]);
Expand Down
62 changes: 59 additions & 3 deletions apps/server/src/keybindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
MAX_KEYBINDINGS_COUNT,
ResolvedKeybindingRule,
ResolvedKeybindingsConfig,
type ServerRemoveKeybindingInput,
type ServerUpsertKeybindingInput,
type ServerConfigIssue,
} from "@t3tools/contracts";
import {
Expand Down Expand Up @@ -124,6 +126,25 @@ function hasSameShortcutContext(left: KeybindingRule, right: KeybindingRule): bo
return leftContext === rightContext;
}

function keybindingRuleFromUpsertInput(input: ServerUpsertKeybindingInput): KeybindingRule {
return input.when === undefined
? { key: input.key, command: input.command }
: { key: input.key, command: input.command, when: input.when };
}

function replaceTargetFromUpsertInput(input: ServerUpsertKeybindingInput): KeybindingRule | null {
if (!input.replace) return null;
return input.replace.when === undefined
? { key: input.replace.key, command: input.replace.command }
: { key: input.replace.key, command: input.replace.command, when: input.replace.when };
}

function keybindingRuleFromRemoveInput(input: ServerRemoveKeybindingInput): KeybindingRule {
return input.when === undefined
? { key: input.key, command: input.command }
: { key: input.key, command: input.command, when: input.when };
}

function encodeShortcut(shortcut: KeybindingShortcut): string | null {
const modifiers: string[] = [];
if (shortcut.modKey) modifiers.push("mod");
Expand Down Expand Up @@ -259,7 +280,14 @@ export interface KeybindingsShape {
* oldest entries when needed.
*/
readonly upsertKeybindingRule: (
rule: KeybindingRule,
input: ServerUpsertKeybindingInput,
) => Effect.Effect<ResolvedKeybindingsConfig, KeybindingsConfigError>;

/**
* Remove a single persisted keybinding rule by exact key/command/when match.
*/
readonly removeKeybindingRule: (
input: ServerRemoveKeybindingInput,
) => Effect.Effect<ResolvedKeybindingsConfig, KeybindingsConfigError>;
}

Expand Down Expand Up @@ -616,12 +644,19 @@ const makeKeybindings = Effect.gen(function* () {
get streamChanges() {
return Stream.fromPubSub(changesPubSub);
},
upsertKeybindingRule: (rule) =>
upsertKeybindingRule: (input) =>
upsertSemaphore.withPermits(1)(
Effect.gen(function* () {
const customConfig = yield* loadWritableCustomKeybindingsConfig();
const rule = keybindingRuleFromUpsertInput(input);
const replaceTarget = replaceTargetFromUpsertInput(input);
const nextConfig = [
...customConfig.filter((entry) => entry.command !== rule.command),
...customConfig.filter((entry) => {
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:654

In upsertKeybindingRule, when replaceTarget is provided, the filter only removes entries matching replaceTarget but does not remove entries that also match the new rule. This leaves duplicate entries in the config. For example, replacing {key: "a", command: "cmd1"} with {key: "b", command: "cmd2"} when {key: "b", command: "cmd2"} already exists results in two identical entries. The filter should also exclude entries matching the new rule to prevent duplicates.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/keybindings.ts around line 654:

In `upsertKeybindingRule`, when `replaceTarget` is provided, the filter only removes entries matching `replaceTarget` but does not remove entries that also match the new `rule`. This leaves duplicate entries in the config. For example, replacing `{key: "a", command: "cmd1"}` with `{key: "b", command: "cmd2"}` when `{key: "b", command: "cmd2"}` already exists results in two identical entries. The filter should also exclude entries matching the new `rule` to prevent duplicates.

Evidence trail:
apps/server/src/keybindings.ts lines 654-660 (filter logic in upsertKeybindingRule); apps/server/src/keybindings.ts lines 129-133 (keybindingRuleFromUpsertInput); apps/server/src/keybindings.ts lines 135-140 (replaceTargetFromUpsertInput). The `if (replaceTarget)` branch only filters by replaceTarget and skips filtering by rule.

if (replaceTarget) {
return !isSameKeybindingRule(entry, replaceTarget);
}
return !isSameKeybindingRule(entry, rule);
}),
rule,
];
const cappedConfig =
Expand Down Expand Up @@ -649,6 +684,27 @@ const makeKeybindings = Effect.gen(function* () {
return nextResolved;
}),
),
removeKeybindingRule: (input) =>
upsertSemaphore.withPermits(1)(
Effect.gen(function* () {
const customConfig = yield* loadWritableCustomKeybindingsConfig();
const target = keybindingRuleFromRemoveInput(input);
const nextConfig = customConfig.filter((entry) => !isSameKeybindingRule(entry, target));
yield* writeConfigAtomically(nextConfig);
const nextResolved = mergeWithDefaultKeybindings(
compileResolvedKeybindingsConfig(nextConfig),
);
yield* Cache.set(resolvedConfigCache, resolvedConfigCacheKey, {
keybindings: nextResolved,
issues: [],
});
yield* emitChange({
keybindings: nextResolved,
issues: [],
});
return nextResolved;
}),
),
} satisfies KeybindingsShape;
});

Expand Down
36 changes: 36 additions & 0 deletions apps/server/src/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1918,6 +1918,42 @@ it.layer(NodeServices.layer)("server router seam", (it) => {
}).pipe(Effect.provide(NodeHttpServer.layerTest)),
);

it.effect("routes websocket rpc server.removeKeybinding", () =>
Effect.gen(function* () {
const rule: KeybindingRule = {
command: "terminal.toggle",
key: "ctrl+k",
};
const resolved: ResolvedKeybindingRule = {
command: "terminal.toggle",
shortcut: {
key: "j",
metaKey: false,
ctrlKey: false,
shiftKey: false,
altKey: false,
modKey: true,
},
};

yield* buildAppUnderTest({
layers: {
keybindings: {
removeKeybindingRule: () => Effect.succeed([resolved]),
},
},
});

const wsUrl = yield* getWsServerUrl("/ws");
const response = yield* Effect.scoped(
withWsRpcClient(wsUrl, (client) => client[WS_METHODS.serverRemoveKeybinding](rule)),
);

assert.deepEqual(response.issues, []);
assert.deepEqual(response.keybindings, [resolved]);
}).pipe(Effect.provide(NodeHttpServer.layerTest)),
);

it.effect("rejects websocket rpc handshake when session authentication is missing", () =>
Effect.gen(function* () {
const fs = yield* FileSystem.FileSystem;
Expand Down
9 changes: 9 additions & 0 deletions apps/server/src/ws.ts
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,15 @@ const makeWsRpcLayer = (currentSessionId: AuthSessionId) =>
}),
{ "rpc.aggregate": "server" },
),
[WS_METHODS.serverRemoveKeybinding]: (rule) =>
observeRpcEffect(
WS_METHODS.serverRemoveKeybinding,
Effect.gen(function* () {
const keybindingsConfig = yield* keybindings.removeKeybindingRule(rule);
return { keybindings: keybindingsConfig, issues: [] };
}),
{ "rpc.aggregate": "server" },
),
[WS_METHODS.serverGetSettings]: (_input) =>
observeRpcEffect(
WS_METHODS.serverGetSettings,
Expand Down
10 changes: 7 additions & 3 deletions apps/web/src/components/KeybindingsToast.browser.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -532,16 +532,20 @@ describe("Keybindings update toast", () => {
document.body.innerHTML = "";
});

it("shows a toast for each consecutive keybinding update with no issues", async () => {
it("coalesces rapid consecutive keybinding update toasts with no issues", async () => {
const mounted = await mountApp();

try {
sendServerConfigUpdatedPush([]);
await waitForToast("Keybindings updated", 1);

// Each server push represents a distinct file change, so it should produce its own toast.
// A single edit can produce several reload notifications as the direct update and
// filesystem watcher settle, so avoid stacking identical success toasts.
sendServerConfigUpdatedPush([]);
await waitForToast("Keybindings updated", 2);
await new Promise((resolve) => setTimeout(resolve, 250));

const titles = queryToastTitles();
expect(titles.filter((title) => title === "Keybindings updated")).toHaveLength(1);
} finally {
await mounted.cleanup();
}
Expand Down
55 changes: 2 additions & 53 deletions apps/web/src/components/ProjectScriptsControl.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ import {
keybindingValueForCommand,
decodeProjectScriptKeybindingRule,
} from "~/lib/projectScriptKeybindings";
import { keybindingFromKeyboardEvent } from "~/components/settings/KeybindingsSettings.logic";
import {
commandForProjectScript,
nextProjectScriptId,
primaryProjectScript,
} from "~/projectScripts";
import { shortcutLabelForCommand } from "~/keybindings";
import { isMacPlatform } from "~/lib/utils";
import {
AlertDialog,
AlertDialogClose,
Expand Down Expand Up @@ -96,57 +96,6 @@ interface ProjectScriptsControlProps {
onDeleteScript: (scriptId: string) => Promise<void> | void;
}

function normalizeShortcutKeyToken(key: string): string | null {
const normalized = key.toLowerCase();
if (
normalized === "meta" ||
normalized === "control" ||
normalized === "ctrl" ||
normalized === "shift" ||
normalized === "alt" ||
normalized === "option"
) {
return null;
}
if (normalized === " ") return "space";
if (normalized === "escape") return "esc";
if (normalized === "arrowup") return "arrowup";
if (normalized === "arrowdown") return "arrowdown";
if (normalized === "arrowleft") return "arrowleft";
if (normalized === "arrowright") return "arrowright";
if (normalized.length === 1) return normalized;
if (normalized.startsWith("f") && normalized.length <= 3) return normalized;
if (normalized === "enter" || normalized === "tab" || normalized === "backspace") {
return normalized;
}
if (normalized === "delete" || normalized === "home" || normalized === "end") {
return normalized;
}
if (normalized === "pageup" || normalized === "pagedown") return normalized;
return null;
}

function keybindingFromEvent(event: KeyboardEvent<HTMLInputElement>): string | null {
const keyToken = normalizeShortcutKeyToken(event.key);
if (!keyToken) return null;

const parts: string[] = [];
if (isMacPlatform(navigator.platform)) {
if (event.metaKey) parts.push("mod");
if (event.ctrlKey) parts.push("ctrl");
} else {
if (event.ctrlKey) parts.push("mod");
if (event.metaKey) parts.push("meta");
}
if (event.altKey) parts.push("alt");
if (event.shiftKey) parts.push("shift");
if (parts.length === 0) {
return null;
}
parts.push(keyToken);
return parts.join("+");
}

export default function ProjectScriptsControl({
scripts,
keybindings,
Expand Down Expand Up @@ -186,7 +135,7 @@ export default function ProjectScriptsControl({
setKeybinding("");
return;
}
const next = keybindingFromEvent(event);
const next = keybindingFromKeyboardEvent(event, navigator.platform);
if (!next) return;
setKeybinding(next);
};
Expand Down
Loading
Loading