From 65defab45a31266c3792718b018935a1a88c7bef Mon Sep 17 00:00:00 2001 From: Jacob Fu <141651335+FuJacob@users.noreply.github.com> Date: Sat, 30 May 2026 19:52:31 -0700 Subject: [PATCH] Block assigning one shortcut to multiple keybindings Toggle Tabby silently did nothing whenever its hotkey collided with an accept binding: the default Accept Entire Suggestion key is backtick, and a user who bound the toggle to the same key hit a collision the toggle never won. The head-inserted accept tap consumes the shared key before the toggle tap sees it (logs show 'Accept tap consumed keyCode=50'), so the toggle callback never fires. Nothing in the recorder or setters guarded the global toggle against duplicating another action's binding. Add a single source of truth, SuggestionSettingsModel.conflictingShortcutName, that reports which action already owns a proposed key combo (the disabled sentinel never conflicts; same key with different modifiers stays distinct). KeyRecorderView now consults it and refuses to commit a duplicate, staying in recording mode with a red 'Already used by X' prompt so the user can pick another key. Wired into both the Settings shortcuts pane and onboarding. Add ShortcutConflictTests covering the toggle-vs-full-accept collision, the self-exclusion, the disabled sentinel, and modifier distinctness. --- Cotabby.xcodeproj/project.pbxproj | 4 + Cotabby/Models/SuggestionSettingsModel.swift | 52 ++++++++++++ Cotabby/UI/KeyRecorderView.swift | 18 +++- .../UI/Settings/Panes/ShortcutsPaneView.swift | 32 +++++++- Cotabby/UI/WelcomeView.swift | 11 +++ CotabbyTests/ShortcutConflictTests.swift | 82 +++++++++++++++++++ 6 files changed, 194 insertions(+), 5 deletions(-) create mode 100644 CotabbyTests/ShortcutConflictTests.swift diff --git a/Cotabby.xcodeproj/project.pbxproj b/Cotabby.xcodeproj/project.pbxproj index 6bd715f6..a7186913 100644 --- a/Cotabby.xcodeproj/project.pbxproj +++ b/Cotabby.xcodeproj/project.pbxproj @@ -124,6 +124,7 @@ 7E99F5676A1D1DF7EA7D7702 /* SuggestionCoordinator+Prediction.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1ED1EA9282E0AC7592E60889 /* SuggestionCoordinator+Prediction.swift */; }; 7FC103944F4EF39DB965F469 /* InMemoryLogging in Frameworks */ = {isa = PBXBuildFile; productRef = 88921938DC814625ED57D605 /* InMemoryLogging */; }; 82D4ADEAF05337ABDE4C586C /* RuntimeBootstrapModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 60629DFE309C1A4BD8A7FB3B /* RuntimeBootstrapModel.swift */; }; + 8441299082E6B68F7F88911B /* ShortcutConflictTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0850B07CCDBA67C756C6EC59 /* ShortcutConflictTests.swift */; }; 84A4CA05AF6885AE4FA4C13A /* SettingsAttentionEvaluator.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2A02336442BB735EE2E8D064 /* SettingsAttentionEvaluator.swift */; }; 87806DE08881D11F2608A13D /* MarkerSelectionSynthesizerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 52BAFA2F989C3C4F7FB892B5 /* MarkerSelectionSynthesizerTests.swift */; }; 8865B95FE84198D70390DF80 /* ClipboardContentDistillerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = EFD89799BB82AF7A92559AEB /* ClipboardContentDistillerTests.swift */; }; @@ -237,6 +238,7 @@ 04E25414C307A20B6F9F20EC /* FocusSnapshotResolver.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FocusSnapshotResolver.swift; sourceTree = ""; }; 050D929E13BE52E6282B64D2 /* VisualContextStartCoalescerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = VisualContextStartCoalescerTests.swift; sourceTree = ""; }; 07480CE96ED0EBD94817C6B1 /* GeneralPaneView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GeneralPaneView.swift; sourceTree = ""; }; + 0850B07CCDBA67C756C6EC59 /* ShortcutConflictTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShortcutConflictTests.swift; sourceTree = ""; }; 09FADF683BE7B3558377FA76 /* FocusPollBackoff.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FocusPollBackoff.swift; sourceTree = ""; }; 0A3D1125B962CBE0269EEDDB /* SuggestionInserter.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SuggestionInserter.swift; sourceTree = ""; }; 0AC3BF78835C8F2C315932F1 /* EmojiCatalog.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EmojiCatalog.swift; sourceTree = ""; }; @@ -690,6 +692,7 @@ 0D80CC2CCAAFE3F23FB8C37A /* PromptContextSanitizerTests.swift */, 4696A84D17890B154533A08F /* PromptPolicyTests.swift */, 2BC293F6125E2B14DCF05AD9 /* SettingsAttentionEvaluatorTests.swift */, + 0850B07CCDBA67C756C6EC59 /* ShortcutConflictTests.swift */, C05B0439348261163B37C508 /* SuggestionAvailabilityEvaluatorTests.swift */, C375227649689775275AA4B3 /* SuggestionCoordinatorAcceptanceTests.swift */, CDB25ABC4FFB0E63477CDCB0 /* SuggestionOverlayStabilityGateTests.swift */, @@ -1157,6 +1160,7 @@ 934885ACC2DEA20B27F10948 /* PromptContextSanitizerTests.swift in Sources */, 3CF1A4E39F24917DF0470A7D /* PromptPolicyTests.swift in Sources */, C618C5595DA9C57C806A3E03 /* SettingsAttentionEvaluatorTests.swift in Sources */, + 8441299082E6B68F7F88911B /* ShortcutConflictTests.swift in Sources */, 88BCD795A14E1C9308F7BB31 /* SuggestionAvailabilityEvaluatorTests.swift in Sources */, 5B404450B412A6102F514250 /* SuggestionCoordinatorAcceptanceTests.swift in Sources */, 4C6D8ED0A7B45D2EADF06DA5 /* SuggestionOverlayStabilityGateTests.swift in Sources */, diff --git a/Cotabby/Models/SuggestionSettingsModel.swift b/Cotabby/Models/SuggestionSettingsModel.swift index 4eea9dde..f08662e5 100644 --- a/Cotabby/Models/SuggestionSettingsModel.swift +++ b/Cotabby/Models/SuggestionSettingsModel.swift @@ -2,6 +2,22 @@ import ApplicationServices import Combine import Foundation +/// Identifies one of the three user-configurable keyboard shortcuts so the recorder can ask which +/// other action (if any) already owns a proposed key combination before committing it. +enum ShortcutAction: CaseIterable { + case acceptWord + case acceptEntireSuggestion + case toggleTabby + + var displayName: String { + switch self { + case .acceptWord: return "Accept Word" + case .acceptEntireSuggestion: return "Accept Entire Suggestion" + case .toggleTabby: return "Toggle Tabby" + } + } +} + /// File overview: /// Owns the durable autocomplete preferences that are shared across the app: /// engine selection, completion length, indicator appearance, and profile @@ -819,6 +835,42 @@ final class SuggestionSettingsModel: ObservableObject { setGloballyEnabled(!isGloballyEnabled) } + /// Returns the user-facing name of the shortcut action already bound to `(keyCode, modifiers)`, + /// excluding `action` itself, or `nil` when the combo is free. + /// + /// This is the single source of truth the recorder consults before committing a new binding. + /// Without it the global-toggle hotkey can silently collide with an accept key: the toggle tap + /// is head-inserted but the accept tap (installed later while a suggestion is visible) sits ahead + /// of it and consumes the shared key first, so the toggle never fires. Blocking the duplicate up + /// front keeps every binding unambiguous. The disabled sentinel never conflicts — several actions + /// may be left unbound at once. + func conflictingShortcutName( + keyCode: CGKeyCode, + modifiers: ShortcutModifierMask, + excluding action: ShortcutAction + ) -> String? { + guard keyCode != Self.disabledKeyCode else { return nil } + + for other in ShortcutAction.allCases where other != action { + let binding = shortcutBinding(for: other) + if binding.keyCode == keyCode, binding.modifiers == modifiers { + return other.displayName + } + } + return nil + } + + private func shortcutBinding(for action: ShortcutAction) -> (keyCode: CGKeyCode, modifiers: ShortcutModifierMask) { + switch action { + case .acceptWord: + return (acceptanceKeyCode, acceptanceKeyModifiers) + case .acceptEntireSuggestion: + return (fullAcceptanceKeyCode, fullAcceptanceKeyModifiers) + case .toggleTabby: + return (globalToggleKeyCode, globalToggleKeyModifiers) + } + } + private func persistSelectedEngine(_ engine: SuggestionEngineKind) { userDefaults.set(engine.rawValue, forKey: Self.selectedEngineDefaultsKey) } diff --git a/Cotabby/UI/KeyRecorderView.swift b/Cotabby/UI/KeyRecorderView.swift index 8f4b529a..0387653c 100644 --- a/Cotabby/UI/KeyRecorderView.swift +++ b/Cotabby/UI/KeyRecorderView.swift @@ -11,18 +11,26 @@ import SwiftUI struct KeyRecorderView: View { let onKeyRecorded: (CGKeyCode, ShortcutModifierMask, String) -> Void var onCancelled: (() -> Void)? + /// Returns the name of the action already bound to a proposed combo, or `nil` if it's free. + /// When it reports a conflict the recorder refuses to commit and keeps listening, so a shortcut + /// can never be assigned to two actions at once. + var conflictChecker: ((CGKeyCode, ShortcutModifierMask) -> String?)? @State private var monitor: Any? @State private var liveModifiers: ShortcutModifierMask = [] + @State private var conflictMessage: String? var body: some View { Text(promptText) - .foregroundStyle(.secondary) + .foregroundStyle(conflictMessage == nil ? .secondary : Color.red) .onAppear { installMonitor() } .onDisappear { removeMonitor() } } private var promptText: String { + if let conflictMessage { + return conflictMessage + } let glyphs = KeyCodeLabels.modifierGlyphs(liveModifiers) return glyphs.isEmpty ? "Press a key…" : "\(glyphs) + key…" } @@ -60,6 +68,14 @@ struct KeyRecorderView: View { // consumes the key while a suggestion is visible (otherwise it passes through and does // its normal job). So even Return/Delete or `⌘V` are safe to bind — they only intercept // in the moment a suggestion is showing. + // Reject a combo that another action already owns. Staying in recording mode (rather than + // committing or cancelling) lets the user immediately try a different key, and the red + // prompt explains why the press was ignored. + if let conflict = conflictChecker?(keyCode, modifiers) { + conflictMessage = "Already used by \(conflict). Try another key." + return nil + } + let label = KeyCodeLabels.label( for: keyCode, modifiers: modifiers, diff --git a/Cotabby/UI/Settings/Panes/ShortcutsPaneView.swift b/Cotabby/UI/Settings/Panes/ShortcutsPaneView.swift index 3664b489..acd5090f 100644 --- a/Cotabby/UI/Settings/Panes/ShortcutsPaneView.swift +++ b/Cotabby/UI/Settings/Panes/ShortcutsPaneView.swift @@ -39,7 +39,14 @@ struct ShortcutsPaneView: View { ) }, onClear: { suggestionSettings.clearAcceptanceKey() }, - clearHelp: "Unbind this shortcut. No key will accept word-by-word." + clearHelp: "Unbind this shortcut. No key will accept word-by-word.", + conflictChecker: { keyCode, modifiers in + suggestionSettings.conflictingShortcutName( + keyCode: keyCode, + modifiers: modifiers, + excluding: .acceptWord + ) + } ) } label: { SettingsRowLabel( @@ -70,7 +77,14 @@ struct ShortcutsPaneView: View { ) }, onClear: { suggestionSettings.clearFullAcceptanceKey() }, - clearHelp: "Unbind this shortcut. No key will accept the whole suggestion at once." + clearHelp: "Unbind this shortcut. No key will accept the whole suggestion at once.", + conflictChecker: { keyCode, modifiers in + suggestionSettings.conflictingShortcutName( + keyCode: keyCode, + modifiers: modifiers, + excluding: .acceptEntireSuggestion + ) + } ) } label: { SettingsRowLabel( @@ -98,7 +112,14 @@ struct ShortcutsPaneView: View { }, onReset: nil, onClear: { suggestionSettings.clearGlobalToggleKey() }, - clearHelp: "Unbind this shortcut. No key will toggle Tabby on or off." + clearHelp: "Unbind this shortcut. No key will toggle Tabby on or off.", + conflictChecker: { keyCode, modifiers in + suggestionSettings.conflictingShortcutName( + keyCode: keyCode, + modifiers: modifiers, + excluding: .toggleTabby + ) + } ) } label: { SettingsRowLabel( @@ -126,6 +147,8 @@ private struct KeybindRow: View { let onReset: (() -> Void)? let onClear: () -> Void let clearHelp: String + /// Names the action that already owns a proposed combo so the recorder can block duplicates. + let conflictChecker: (CGKeyCode, ShortcutModifierMask) -> String? var body: some View { HStack(spacing: 8) { @@ -143,7 +166,8 @@ private struct KeybindRow: View { onRecord(keyCode, modifiers, label) isRecording = false }, - onCancelled: { isRecording = false } + onCancelled: { isRecording = false }, + conflictChecker: conflictChecker ) } else { Button("Change") { diff --git a/Cotabby/UI/WelcomeView.swift b/Cotabby/UI/WelcomeView.swift index 576b7132..b994320f 100644 --- a/Cotabby/UI/WelcomeView.swift +++ b/Cotabby/UI/WelcomeView.swift @@ -401,6 +401,7 @@ extension WelcomeView { keybindRow( title: "Accept Word", keyLabel: suggestionSettings.acceptanceKeyLabel, + action: .acceptWord, isRecording: $isRecordingOnboardingKeybind, onKeyRecorded: { keyCode, modifiers, label in suggestionSettings.setAcceptanceKey( @@ -424,6 +425,7 @@ extension WelcomeView { keybindRow( title: "Accept Entire Suggestion", keyLabel: suggestionSettings.fullAcceptanceKeyLabel, + action: .acceptEntireSuggestion, isRecording: $isRecordingOnboardingFullAcceptKeybind, onKeyRecorded: { keyCode, modifiers, label in suggestionSettings.setFullAcceptanceKey( @@ -450,6 +452,7 @@ extension WelcomeView { keybindRow( title: "Toggle Tabby", keyLabel: suggestionSettings.globalToggleKeyLabel, + action: .toggleTabby, isRecording: $isRecordingOnboardingGlobalToggleKeybind, onKeyRecorded: { keyCode, modifiers, label in suggestionSettings.setGlobalToggleKey( @@ -468,6 +471,7 @@ extension WelcomeView { fileprivate func keybindRow( title: String, keyLabel: String, + action: ShortcutAction, isRecording: Binding, onKeyRecorded: @escaping (CGKeyCode, ShortcutModifierMask, String) -> Void, onReset: (() -> Void)? = nil @@ -495,6 +499,13 @@ extension WelcomeView { }, onCancelled: { isRecording.wrappedValue = false + }, + conflictChecker: { keyCode, modifiers in + suggestionSettings.conflictingShortcutName( + keyCode: keyCode, + modifiers: modifiers, + excluding: action + ) } ) } else { diff --git a/CotabbyTests/ShortcutConflictTests.swift b/CotabbyTests/ShortcutConflictTests.swift new file mode 100644 index 00000000..f8789cf4 --- /dev/null +++ b/CotabbyTests/ShortcutConflictTests.swift @@ -0,0 +1,82 @@ +import XCTest +@testable import Cotabby + +/// Locks down the duplicate-shortcut guard that keeps the three keybindings unambiguous. +/// +/// The original bug: the global-toggle hotkey was excluded from any conflict check, so a user could +/// bind it to a key already used by an accept binding (the default full-accept key is backtick). The +/// head-inserted accept tap then consumed the shared key before the toggle tap saw it, so Toggle +/// Tabby silently did nothing. `conflictingShortcutName` is the single source of truth the recorder +/// consults to refuse such a binding up front. +@MainActor +final class ShortcutConflictTests: XCTestCase { + + /// A combo already owned by another action is reported, keyed by that action's display name. + func test_conflict_reportsActionOwningTheCombo() { + let model = makeModel() + // Default full-accept is backtick (keyCode 50). Binding the toggle to the same key must + // be flagged as a conflict with "Accept Entire Suggestion". + let conflict = model.conflictingShortcutName( + keyCode: 50, + modifiers: [], + excluding: .toggleTabby + ) + XCTAssertEqual(conflict, "Accept Entire Suggestion") + } + + /// An action never conflicts with itself, so re-recording the same key for the same binding is + /// allowed (e.g. confirming the current value). + func test_conflict_excludesTheActionBeingEdited() { + let model = makeModel() + let conflict = model.conflictingShortcutName( + keyCode: model.acceptanceKeyCode, + modifiers: model.acceptanceKeyModifiers, + excluding: .acceptWord + ) + XCTAssertNil(conflict) + } + + /// A free combo (default Accept Word is Tab; some other key is unused) reports no conflict. + 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) + } + + /// The disabled sentinel never conflicts: multiple actions may be left unbound at once. + func test_conflict_ignoresDisabledSentinel() { + let model = makeModel() + let conflict = model.conflictingShortcutName( + keyCode: SuggestionSettingsModel.disabledKeyCode, + modifiers: [], + excluding: .toggleTabby + ) + XCTAssertNil(conflict) + } + + /// Same key but different modifiers is a distinct binding and must not be flagged. + func test_conflict_treatsModifierSetsAsDistinct() { + let model = makeModel() + // Full-accept default is bare backtick; backtick + shift is a different binding. + let conflict = model.conflictingShortcutName( + keyCode: 50, + modifiers: [.shift], + excluding: .toggleTabby + ) + XCTAssertNil(conflict) + } + + // MARK: - Helpers + + private func makeModel() -> SuggestionSettingsModel { + let suiteName = "cotabby.test.shortcutConflict.\(UUID().uuidString)" + let defaults = UserDefaults(suiteName: suiteName)! + defaults.removePersistentDomain(forName: suiteName) + return SuggestionSettingsModel(configuration: .standard, userDefaults: defaults) + } +}