Fix ctrl+enter resulting in duplicate triggers on non-Mac platforms. #7592
Conversation
…nt query. On Mac this results in both ctrl+enter and command+enter being bound, but on other platforms mod becomes ctrl and we bind ctrl+enter TWICE. Every shortcut invocation happens twice, and it's a race condition as to whether one server-side job or two are created. Instead, do our own translation and dedupe before binding.
…upports command+ as a key combo
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="client/app/services/KeyboardShortcuts.js">
<violation number="1" location="client/app/services/KeyboardShortcuts.js:44">
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.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| .map(trim); | ||
| each(keys, k => { | ||
| // Translate platform‑specific modifiers and dedupe. | ||
| const transformed = rawKeys.map(k => |
There was a problem hiding this comment.
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 => {
+ // Translate platform‑specific modifiers and dedupe.
+ const transformed = rawKeys.map(k =>
+ k.replace(/mod/g, modKey.toLowerCase()).replace(/alt/g, altKey.toLowerCase())
+ );
</file context>
✅ Addressed in 82880ce
|
@Jakdaw Thank you for your contribution! |
No; I think the first keyboard combo is used to generate the human readable description for tool tips and the like. So if we replace "mod+enter" with "command+enter" the tooltips will be incorrect on non-Mac. Removing "ctrl+enter" would have fixed it - I'm not sure why Mac users would want in addition to - but that got added 5 years ago, so I'd kinda take the view that that ship has sailed. |
|
|
I tested on My Mac, and it looks "Command-Enter" does not work. |
…pple don't shorten "Command" on their keyboards, so makes more sense for the tooltip to use the whole word
Very good point - yes, looking at Mousetrap we should be sending it "command+enter" not "cmd+enter". For that matter Apple write the full "command" on their keyboards - so I'm going to change the string that's used both for mousetrap and for the tooltip, seems that "Command" is more appropriate there than "Cmd". |
|
Hah - it seems old Mac's did have "Cmd" on the keyboard, but since about 2015 they appear to have changed to "command"; so there we go I'm bringing it up to date! Reminded why I normally stay the hell away from UIs ;-) Thanks for your help Tsuneo! |
| each(keys, k => { | ||
| // Translate platform-specific modifiers and dedupe. | ||
| const transformed = rawKeys.map(k => | ||
| k.replace(/mod/g, modKey.toLowerCase()) |
There was a problem hiding this comment.
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") )
2 participants
At present, we bind both mod+enter and ctrl+enter to execute the current query. On Mac this results in both ctrl+enter and command+enter being bound, but on other platforms mod becomes ctrl and we bind ctrl+enter TWICE. Every shortcut invocation happens twice, and it's a race condition as to whether one server-side job or two are created. Instead, do our own translation and dedupe before binding.
Even without the race condition triggering it's easy to reproduce the bug:
What type of PR is this?
How is this tested?
Disclaimer: I don't have a Mac nor any JS experience!