Skip to content

Fix ctrl+enter resulting in duplicate triggers on non-Mac platforms. #7592

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
Jakdaw wants to merge 4 commits into
base: master
Choose a base branch
from
+17 −6
Open

Fix ctrl+enter resulting in duplicate triggers on non-Mac platforms. #7592

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
934c830
At present we bind both mod+enter and ctrl+enter to execute the curre…
Jakdaw Dec 8, 2025
dba0eb9
Second thoughts: let's not mess with alt+ - I don't think Mousetrap s…
Jakdaw Dec 8, 2025
82880ce
Make unbind symmetrical with bind
Jakdaw Dec 9, 2025
537845e
Mousetrap expects command+enter rather than cmd+enter. Additionally A…
Jakdaw Dec 9, 2025
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: 17 additions & 6 deletions client/app/services/KeyboardShortcuts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { each, filter, map, toLower, toString, trim, upperFirst, without } from
import Mousetrap from "mousetrap";
import "mousetrap/plugins/global-bind/mousetrap-global-bind";

const modKey = /Mac|iPod|iPhone|iPad/.test(navigator.platform) ? "Cmd" : "Ctrl";
const modKey = /Mac|iPod|iPhone|iPad/.test(navigator.platform) ? "Command" : "Ctrl";
const altKey = /Mac|iPod|iPhone|iPad/.test(navigator.platform) ? "Option" : "Alt";

export function humanReadableShortcut(shortcut, limit = Infinity) {
Expand Down Expand Up @@ -35,11 +35,17 @@ const KeyboardShortcuts = {

bind: keymap => {
each(keymap, (fn, key) => {
const keys = key
// Resolve platform-specific modifiers and remove duplicates.
const rawKeys = key
.toLowerCase()
.split(",")
.map(trim);
each(keys, k => {
// Translate platform-specific modifiers and dedupe.
const transformed = rawKeys.map(k =>
Copy link

@cubic-dev-ai cubic-dev-ai bot Dec 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

P1: The unbind function was not updated with the same key transformation logic. When binding mod+enter, it gets stored as ctrl+enter in handlers, but unbind will still look for mod+enter, causing silent failures when trying to remove handlers.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At client/app/services/KeyboardShortcuts.js, line 44:

<comment>The `unbind` function was not updated with the same key transformation logic. When binding `mod+enter`, it gets stored as `ctrl+enter` in handlers, but unbind will still look for `mod+enter`, causing silent failures when trying to remove handlers.</comment>

<file context>
@@ -35,11 +35,17 @@ const KeyboardShortcuts = {
         .map(trim);
-      each(keys, k =&gt; {
+      // Translate platform‑specific modifiers and dedupe.
+      const transformed = rawKeys.map(k =&gt;
+        k.replace(/mod/g, modKey.toLowerCase()).replace(/alt/g, altKey.toLowerCase())
+      );
</file context>

✅ Addressed in 82880ce

k.replace(/mod/g, modKey.toLowerCase())
Copy link
Contributor

@yoshiokatsuneo yoshiokatsuneo Dec 10, 2025

Choose a reason for hiding this comment

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

I'm just feeling that it might be better not to depends on "modKey", as "modKey" is not for mapping but for human readable name. (For example, the name might be localized in the future.)

How about just writing directly like below ?

   k.replace(/mod/g, (/Mac|iPod|iPhone|iPad/.test(navigator.platform) ? "command" : "ctrl") )

);
const uniqueKeys = [...new Set(transformed)];
each(uniqueKeys, k => {
handlers[k] = [...without(handlers[k], fn), fn];
Mousetrap.bindGlobal(k, onShortcut);
});
Expand All @@ -48,13 +54,18 @@ const KeyboardShortcuts = {

unbind: keymap => {
each(keymap, (fn, key) => {
const keys = key
const rawKeys = key
.toLowerCase()
.split(",")
.map(trim);
each(keys, k => {
// Apply the same transformation as in bind
const transformed = rawKeys.map(k =>
k.replace(/mod/g, modKey.toLowerCase())
);
const uniqueKeys = [...new Set(transformed)];
each(uniqueKeys, k => {
handlers[k] = without(handlers[k], fn);
if (handlers[k].length === 0) {
if (!handlers[k] || handlers[k].length === 0) {
handlers[k] = undefined;
Mousetrap.unbind(k);
}
Expand Down
Loading