Desktop: Fix shortcut editor Save button blocked by pre-existing conflicts (#11670)#15185
Desktop: Fix shortcut editor Save button blocked by pre-existing conflicts (#11670)#15185samkaminski wants to merge 1 commit intolaurent22:devfrom
Conversation
…licts When validateKeymap was called with a proposedKeymapItem, it accumulated all seen accelerators in a Set and threw on any duplicate — including pre-existing conflicts between other commands unrelated to the proposed change. This caused the Save button in ShortcutRecorder to be disabled whenever any pre-existing shortcut conflict existed in the keymap, even if the user's edit introduced no new conflict. Fix: when proposedKeymapItem is provided, only check if that accelerator directly conflicts with another command's accelerator. The full duplicate scan (no proposedKeymapItem) used by overrideKeymap is unchanged. Fixes laurent22#11670 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/lib/services/KeymapService.test.js (1)
349-367: LGTM — good targeted coverage for the fix.The three cases map cleanly to the new two-path behaviour: proposed conflict throws, proposed unique passes, and pre-existing unrelated conflict no longer blocks a save. Using
setAcceleratorto seed the pre-existing duplicate is a nice way to simulate the bug scenario from#11670.One optional addition worth considering: an assertion that proposing an accelerator which matches the pre-existing duplicate (e.g. proposing
Ctrl+NforsynchronizeafternewTodowas set toCtrl+N) still throws — this locks in that the fix only relaxes unrelated conflicts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/services/KeymapService.test.js` around lines 349 - 367, Add a new assertion in the validateKeymap test to ensure that if you seed a pre-existing duplicate via keymapService.setAccelerator('newTodo','Ctrl+N') and then propose the same accelerator for a different command, keymapService.validateKeymap({ accelerator: 'Ctrl+N', command: 'synchronize' }) still throws; this confirms validateKeymap enforces conflicts for the proposed shortcut even when the duplicate was pre-existing (use the existing describe('validateKeymap') setup and the setAccelerator helper to seed the state).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lib/services/KeymapService.ts`:
- Around line 352-369: In validateKeymap, guard against a null/empty
proposedKeymapItem.accelerator by short-circuiting when
proposedKeymapItem.accelerator is falsy so you don't treat disabled shortcuts as
conflicts; inside the proposedKeymapItem branch of validateKeymap (the loop over
this.keymap), skip any comparison if !proposedKeymapItem.accelerator (or
explicitly check truthiness) and only compare item.accelerator ===
proposedKeymapItem.accelerator when proposedKeymapItem.accelerator is truthy
(mirroring the existing else if (itemAccelerator) behavior for the full-keymap
path).
---
Nitpick comments:
In `@packages/lib/services/KeymapService.test.js`:
- Around line 349-367: Add a new assertion in the validateKeymap test to ensure
that if you seed a pre-existing duplicate via
keymapService.setAccelerator('newTodo','Ctrl+N') and then propose the same
accelerator for a different command, keymapService.validateKeymap({ accelerator:
'Ctrl+N', command: 'synchronize' }) still throws; this confirms validateKeymap
enforces conflicts for the proposed shortcut even when the duplicate was
pre-existing (use the existing describe('validateKeymap') setup and the
setAccelerator helper to seed the state).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60fd0d0b-19c5-4d1e-b3c2-bc48f8bef399
📒 Files selected for processing (2)
packages/lib/services/KeymapService.test.jspackages/lib/services/KeymapService.ts
| public validateKeymap(proposedKeymapItem: KeymapItem = null) { | ||
| const usedAccelerators = new Set(); | ||
| if (proposedKeymapItem) { | ||
| // When checking a proposed change, only throw if the proposed accelerator | ||
| // conflicts with another command. Pre-existing conflicts between other commands | ||
| // should not block the user from saving an unrelated shortcut change. | ||
| for (const item of Object.values(this.keymap)) { | ||
| if (item.command === proposedKeymapItem.command) continue; | ||
| if (item.accelerator === proposedKeymapItem.accelerator) { | ||
| throw new Error(_( | ||
| 'Accelerator "%s" is used for "%s" and "%s" commands. This may lead to unexpected behaviour.', | ||
| proposedKeymapItem.accelerator, | ||
| item.command, | ||
| proposedKeymapItem.command, | ||
| )); | ||
| } | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Guard against a null/empty proposed accelerator.
If a caller (other than ShortcutRecorder, which already guards on accelerator truthiness) passes a proposedKeymapItem with accelerator === null (a disabled shortcut), this loop will falsely report a conflict against any other command whose accelerator is also null (several defaults, e.g. commands added via additionalDefaultCommandNames, are null). Consider short-circuiting when the proposed accelerator is falsy, matching the spirit of the full-keymap path which skips empty accelerators via the else if (itemAccelerator) branch.
🛡️ Proposed guard
if (proposedKeymapItem) {
+ // A null/empty accelerator means the shortcut is disabled — nothing to conflict with.
+ if (!proposedKeymapItem.accelerator) return;
// When checking a proposed change, only throw if the proposed accelerator
// conflicts with another command. Pre-existing conflicts between other commands
// should not block the user from saving an unrelated shortcut change.
for (const item of Object.values(this.keymap)) {
if (item.command === proposedKeymapItem.command) continue;
if (item.accelerator === proposedKeymapItem.accelerator) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public validateKeymap(proposedKeymapItem: KeymapItem = null) { | |
| const usedAccelerators = new Set(); | |
| if (proposedKeymapItem) { | |
| // When checking a proposed change, only throw if the proposed accelerator | |
| // conflicts with another command. Pre-existing conflicts between other commands | |
| // should not block the user from saving an unrelated shortcut change. | |
| for (const item of Object.values(this.keymap)) { | |
| if (item.command === proposedKeymapItem.command) continue; | |
| if (item.accelerator === proposedKeymapItem.accelerator) { | |
| throw new Error(_( | |
| 'Accelerator "%s" is used for "%s" and "%s" commands. This may lead to unexpected behaviour.', | |
| proposedKeymapItem.accelerator, | |
| item.command, | |
| proposedKeymapItem.command, | |
| )); | |
| } | |
| } | |
| return; | |
| } | |
| public validateKeymap(proposedKeymapItem: KeymapItem = null) { | |
| if (proposedKeymapItem) { | |
| // A null/empty accelerator means the shortcut is disabled — nothing to conflict with. | |
| if (!proposedKeymapItem.accelerator) return; | |
| // When checking a proposed change, only throw if the proposed accelerator | |
| // conflicts with another command. Pre-existing conflicts between other commands | |
| // should not block the user from saving an unrelated shortcut change. | |
| for (const item of Object.values(this.keymap)) { | |
| if (item.command === proposedKeymapItem.command) continue; | |
| if (item.accelerator === proposedKeymapItem.accelerator) { | |
| throw new Error(_( | |
| 'Accelerator "%s" is used for "%s" and "%s" commands. This may lead to unexpected behaviour.', | |
| proposedKeymapItem.accelerator, | |
| item.command, | |
| proposedKeymapItem.command, | |
| )); | |
| } | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lib/services/KeymapService.ts` around lines 352 - 369, In
validateKeymap, guard against a null/empty proposedKeymapItem.accelerator by
short-circuiting when proposedKeymapItem.accelerator is falsy so you don't treat
disabled shortcuts as conflicts; inside the proposedKeymapItem branch of
validateKeymap (the loop over this.keymap), skip any comparison if
!proposedKeymapItem.accelerator (or explicitly check truthiness) and only
compare item.accelerator === proposedKeymapItem.accelerator when
proposedKeymapItem.accelerator is truthy (mirroring the existing else if
(itemAccelerator) behavior for the full-keymap path).
1 participant
Summary
Fixes #11670.
In Joplin's desktop keyboard shortcut settings dialog, the Save button in the
ShortcutRecordercomponent was disabled whenever any shortcut conflict existed in the keymap — even pre-existing conflicts that the user had nothing to do with. This made it impossible to save a change to command C if commands A and B happened to share an accelerator, regardless of what C was being set to.Root cause:
KeymapService.validateKeymap(proposedKeymapItem)accumulated all seen accelerators in aSetas it iterated the full keymap. When two pre-existing commands shared an accelerator, the second occurrence triggered a throw — even whenproposedKeymapItemwas for a completely unrelated command with a completely different accelerator.Fix: When
proposedKeymapItemis provided (the "check a proposed change" path), only verify that the proposed accelerator doesn't directly appear in another command. Pre-existing conflicts between other commands are not the user's responsibility and should not block saving. The no-argument path (called fromoverrideKeymapfor full keymap validation) is unchanged.Changes
packages/lib/services/KeymapService.ts— splitvalidateKeymapinto two code paths: a targeted check when aproposedKeymapItemis given, and the existing full duplicate scan when called with no arguments.packages/lib/services/KeymapService.test.js— added three unit tests covering: conflict detection still works, no conflict passes, and pre-existing conflicts between other commands no longer block an unrelated save.Test plan
yarn test packages/lib/services/KeymapService.test.jspasses (15/15 tests)🤖 Generated with Claude Code