Block assigning one shortcut to multiple keybindings

[FuJacob]

Summary

Bug: Toggle Tabby's hotkey appeared to do nothing. Root cause is a binding collision, not the tap wiring. The default Accept Entire Suggestion key is backtick (`), the same key a user naturally assigns to the toggle. Nothing guarded the global toggle against duplicating another action's shortcut, so both ended up on the same key. The accept tap is head-inserted into the event chain ahead of the toggle tap, so it consumes the shared key first and the toggle callback never fires. The runtime logs confirm it: Accept tap consumed keyCode=50 (backtick) appears while the toggle-tap install/consume lines never do.

Fix: Prevent the collision at the source. A new single source of truth, SuggestionSettingsModel.conflictingShortcutName(keyCode:modifiers:excluding:), reports which action already owns a proposed combo. KeyRecorderView consults it and refuses to commit a duplicate, staying in recording mode with a red "Already used by X" prompt so the user can immediately try another key. Wired into both the Settings shortcuts pane and the onboarding Keybinds step.

Notes:

• The disabled ("None") sentinel never conflicts, so multiple actions may stay unbound.
• Same key with a different modifier set stays a distinct binding (e.g. ` and ⇧`).

Validation

swiftlint lint --strict --quiet <changed files>
# exit 0

xcodegen generate   # new test file added to the project

xcodebuild -project Cotabby.xcodeproj -scheme Cotabby -destination 'platform=macOS' build
# ** BUILD SUCCEEDED **

xcodebuild test -project Cotabby.xcodeproj -scheme Cotabby -destination 'platform=macOS' \
  -only-testing:CotabbyTests/ShortcutConflictTests CODE_SIGNING_ALLOWED=NO
# ** TEST SUCCEEDED **  5 tests, 0 failures

Diagnosis was driven from ~/Library/Logs/Cotabby/cotabby.jsonl*: across the captured sessions the toggle tap install message never appears while the accept tap (33x) and observer tap (8x) do, and backtick (keyCode 50) is logged as consumed by the accept tap.

Linked issues

None.

Risk / rollout notes

• Pure guard added at the recording layer; the existing setters and their silent same-key clear between the two accept bindings are unchanged, so no migration of existing stored bindings. A user who already saved a colliding toggle can now re-record it onto a free key.
• Cotabby.xcodeproj was regenerated with XcodeGen to pick up the new test file (no project.yml change).

Greptile Summary

This PR introduces a guard against binding two shortcuts to the same key combination. A new conflictingShortcutName(keyCode:modifiers:excluding:) method on SuggestionSettingsModel serves as the single source of truth; KeyRecorderView consults it before committing a new binding and shows a red "Already used by X" message instead of accepting the duplicate.

• SuggestionSettingsModel gains a ShortcutAction enum and conflictingShortcutName / shortcutBinding helpers to centralise conflict detection across all three configurable shortcuts.
• KeyRecorderView gains an optional conflictChecker closure that blocks commitment and surfaces a red conflict prompt, while keeping the monitor active so the user can immediately try another key.
• ShortcutsPaneView and WelcomeView are wired up to pass the appropriate conflictChecker, and five unit tests in ShortcutConflictTests cover the key cases.

Confidence Score: 4/5

Safe to merge. The change is a pure UI-layer guard with no migrations or mutations to stored bindings, so existing user settings are unaffected.

The conflict-detection logic is correct and well-tested. The two observations — installMonitor() not clearing conflictMessage, and a test relying on a hardcoded key code assumption — are both benign today but worth addressing before the view or defaults evolve.

KeyRecorderView.swift and ShortcutConflictTests.swift have the minor hardening gaps noted in the comments; all other files are straightforward wiring.

Important Files Changed

Filename	Overview
Cotabby/Models/SuggestionSettingsModel.swift	Adds ShortcutAction enum and conflictingShortcutName/shortcutBinding — logic is correct; disabled-sentinel guard prevents false positives; modifier distinctness is preserved.
Cotabby/UI/KeyRecorderView.swift	Gains conflictChecker closure and conflictMessage state; conflict detection is wired correctly; conflictMessage is never reset inside installMonitor(), though in practice the view is always recreated per session.
Cotabby/UI/Settings/Panes/ShortcutsPaneView.swift	All three KeybindRow usages wired with the correct excluding: action; conflictChecker promoted to a required property on KeybindRow.
Cotabby/UI/WelcomeView.swift	All three keybindRow call sites updated with the new action: parameter and conflictChecker closure; no missed usages.
CotabbyTests/ShortcutConflictTests.swift	Five tests cover the main cases; some hardcode key codes (49, 50) as assumptions about default bindings, which could silently become stale if defaults change.
Cotabby.xcodeproj/project.pbxproj	Adds ShortcutConflictTests.swift to both the file references and test target sources — mechanical XcodeGen regeneration, no concerns.

Sequence Diagram

sequenceDiagram
    participant User
    participant KeyRecorderView
    participant conflictChecker
    participant SuggestionSettingsModel

    User->>KeyRecorderView: Press key combo
    KeyRecorderView->>conflictChecker: (keyCode, modifiers)
    conflictChecker->>SuggestionSettingsModel: conflictingShortcutName(keyCode:modifiers:excluding:)
    alt another action owns this combo
        SuggestionSettingsModel-->>conflictChecker: Accept Entire Suggestion
        conflictChecker-->>KeyRecorderView: conflict name
        KeyRecorderView-->>User: Red Already used by X. Try another key.
    else combo is free
        SuggestionSettingsModel-->>conflictChecker: nil
        conflictChecker-->>KeyRecorderView: nil
        KeyRecorderView->>KeyRecorderView: removeMonitor()
        KeyRecorderView-->>User: Recording committed, onKeyRecorded called
    end

Loading

Comments Outside Diff (1)

1. Cotabby/UI/KeyRecorderView.swift, line 38-41 (link)

   installMonitor() resets liveModifiers but leaves any stale conflictMessage in place. Today this is harmless because KeyRecorderView lives behind an if isRecording branch so it is always torn down and recreated, resetting @State. If that conditional is ever relaxed (e.g. the view is kept alive between sessions), the red "Already used by…" message from the previous recording attempt would be visible the moment the new one starts. A one-line reset here makes the method self-contained.

   

_{Reviews (1): Last reviewed commit: "Block assigning one shortcut to multiple..." | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

[greptile-apps]

The "allows unused combo" test hard-codes keyCode 49 (Space) with a comment that it is "unused by any default binding." If the default bindings ever change and Space is assigned, the test would still pass but for a subtly wrong reason. Adding precondition assertions makes the assumption explicit and the test self-documenting.

Suggested change

	func test_conflict_allowsUnusedCombo() {
	let model = makeModel()
	// keyCode 49 is Space, unused by any default binding.
	let conflict = model.conflictingShortcutName(
	keyCode: 49,
	modifiers: [],
	excluding: .toggleTabby
	)
	XCTAssertNil(conflict)
	}
	func test_conflict_allowsUnusedCombo() {
	let model = makeModel()
	// keyCode 49 is Space. Verify it isn't already taken by any of the other defaults before
	// using it as the "free combo" probe, so a future default-binding change won't silently
	// make this test pass for the wrong reason.
	let spaceKeyCode: CGKeyCode = 49
	XCTAssertNil(model.conflictingShortcutName(keyCode: spaceKeyCode, modifiers: [], excluding: .acceptWord),
	"Precondition: Space must not be a default binding for acceptWord")
	XCTAssertNil(model.conflictingShortcutName(keyCode: spaceKeyCode, modifiers: [], excluding: .acceptEntireSuggestion),
	"Precondition: Space must not be a default binding for acceptEntireSuggestion")
	let conflict = model.conflictingShortcutName(
	keyCode: spaceKeyCode,
	modifiers: [],
	excluding: .toggleTabby
	)
	XCTAssertNil(conflict)
	}

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!