Desktop: Fix shortcut editor Save button blocked by pre-existing conflicts (#11670)

packages/lib/services/KeymapService.test.js

@@ -345,4 +345,24 @@ describe('services_KeymapService', () => {
 		keymapService.registerCommandAccelerator('some-command', null);
 		expect(keymapService.getAriaKeyShortcuts('some-command')).toBe(undefined);
 	});
+
+	describe('validateKeymap', () => {
+		beforeEach(() => keymapService.initialize([], 'linux'));
+
+		it('should throw when proposed accelerator conflicts with an existing command', () => {
+			// Ctrl+N is the default for newNote
+			expect(() => keymapService.validateKeymap({ accelerator: 'Ctrl+N', command: 'synchronize' })).toThrow();
+		});
+
+		it('should not throw when proposed accelerator is unique', () => {
+			expect(() => keymapService.validateKeymap({ accelerator: 'Ctrl+Shift+Alt+F12', command: 'synchronize' })).not.toThrow();
+		});
+
+		it('should not throw for pre-existing conflicts when checking an unrelated command', () => {
+			// Force a duplicate in the keymap directly, simulating a pre-existing conflict
+			keymapService.setAccelerator('newTodo', 'Ctrl+N'); // newNote also has Ctrl+N
+			// The pre-existing newNote/newTodo conflict should not block saving an unrelated shortcut
+			expect(() => keymapService.validateKeymap({ accelerator: 'Ctrl+Shift+Alt+F12', command: 'synchronize' })).not.toThrow();
+		});
+	});
 });

packages/lib/services/KeymapService.ts

@@ -350,25 +350,38 @@ export default class KeymapService extends BaseService {
 	}
 
 	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;
+		}
 
-		// Validate as if the proposed change is already present in the current keymap
-		// Helpful for detecting any errors that'll occur, when the proposed change is performed on the keymap
-		if (proposedKeymapItem) usedAccelerators.add(proposedKeymapItem.accelerator);
+		// Validate the entire keymap for duplicates
+		const usedAccelerators = new Set();
 
 		for (const item of Object.values(this.keymap)) {
 			const [itemAccelerator, itemCommand] = [item.accelerator, item.command];
-			if (proposedKeymapItem && itemCommand === proposedKeymapItem.command) continue; // Ignore the original accelerator
 
 			if (usedAccelerators.has(itemAccelerator)) {
-				const originalItem = (proposedKeymapItem && proposedKeymapItem.accelerator === itemAccelerator)
-					? proposedKeymapItem
-					: Object.values(this.keymap).find(_item => _item.accelerator === itemAccelerator);
+				const originalItem = Object.values(this.keymap).find(_item => _item.accelerator === itemAccelerator);
 
 				throw new Error(_(
 					'Accelerator "%s" is used for "%s" and "%s" commands. This may lead to unexpected behaviour.',
 					itemAccelerator,
-					originalItem.command,
+					// originalItem must exist here — its accelerator was added to usedAccelerators in a prior iteration
+					originalItem!.command,
 					itemCommand,
 				));
 			} else if (itemAccelerator) {