Skip to content

Fix Cmd+I and notifications shortcut conflict #1135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
austinywang wants to merge 3 commits into from
Closed

Fix Cmd+I and notifications shortcut conflict #1135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
19f85f4
Fix Cmd+I and notifications shortcut conflict
austinywang Mar 10, 2026
f7e32f6
Merge branch 'main' of https://github.com/manaflow-ai/cmux into issue…
austinywang Mar 12, 2026
2aa419d
Fix Dvorak command shortcut matching
austinywang Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 28 additions & 9 deletions Sources/AppDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8419,8 +8419,8 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
return true
}

// Safari defaults:
// - Option+Command+I => Show/Toggle Web Inspector
// Browser shortcuts:
// - Command+I => Show/Toggle Developer Tools
// - Option+Command+C => Show JavaScript Console
if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleBrowserDeveloperTools)) {
#if DEBUG
Expand Down Expand Up @@ -8957,10 +8957,12 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent

let chars = (event.charactersIgnoringModifiers ?? "").lowercased()
let flags = event.modifierFlags.intersection(.deviceIndependentFlagsMask)
if flags == [.command, .option] {
if flags == [.command] {
if chars == "i" || event.keyCode == 34 {
return "toggle.literal"
}
}
if flags == [.command, .option] {
if chars == "c" || event.keyCode == 8 {
return "console.literal"
}
Expand Down Expand Up @@ -9242,20 +9244,37 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
return event.keyCode == 36 || event.keyCode == 76
}

let eventCharsIgnoringModifiers = event.charactersIgnoringModifiers
let eventCharacters = event.characters
if shortcutCharacterMatches(
eventCharacter: eventCharsIgnoringModifiers,
eventCharacter: eventCharacters,
shortcutKey: shortcutKey,
applyShiftSymbolNormalization: flags.contains(.shift),
eventKeyCode: event.keyCode
) {
return true
}

// For command-based shortcuts, trust AppKit's layout-aware characters when present.
// Keep this strict for letter shortcuts to avoid physical-key collisions across layouts,
// while still allowing keyCode fallback for digit/punctuation shortcuts on non-US layouts.
let hasEventChars = !(eventCharsIgnoringModifiers?.isEmpty ?? true)
let eventCharsIgnoringModifiers = event.charactersIgnoringModifiers
if eventCharsIgnoringModifiers != eventCharacters,
shortcutCharacterMatches(
eventCharacter: eventCharsIgnoringModifiers,
shortcutKey: shortcutKey,
applyShiftSymbolNormalization: flags.contains(.shift),
eventKeyCode: event.keyCode
) {
return true
}

// Prefer the semantic command character when available. Some input sources
// report the translated command key in `characters` while leaving
// `charactersIgnoringModifiers` tied to the physical ANSI key.
//
// Keep command-letter shortcuts strict once either event field carries a
// concrete character so layout-aware copy/paste-style shortcuts don't fall
// through to physical-key matching.
let hasEventChars =
!(eventCharacters?.isEmpty ?? true)
|| !(eventCharsIgnoringModifiers?.isEmpty ?? true)
if hasEventChars,
flags.contains(.command),
!flags.contains(.control),
Expand Down
6 changes: 2 additions & 4 deletions Sources/KeyboardShortcutSettings.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ enum KeyboardShortcutSettings {
case .sendFeedback:
return StoredShortcut(key: "f", command: true, shift: false, option: true, control: false)
case .showNotifications:
return StoredShortcut(key: "i", command: true, shift: false, option: false, control: false)
return StoredShortcut(key: "i", command: true, shift: true, option: false, control: false)
case .jumpToUnread:
return StoredShortcut(key: "u", command: true, shift: true, option: false, control: false)
case .triggerFlash:
Expand Down Expand Up @@ -173,10 +173,8 @@ enum KeyboardShortcutSettings {
case .openBrowser:
return StoredShortcut(key: "l", command: true, shift: true, option: false, control: false)
case .toggleBrowserDeveloperTools:
// Safari default: Show Web Inspector.
return StoredShortcut(key: "i", command: true, shift: false, option: true, control: false)
return StoredShortcut(key: "i", command: true, shift: false, option: false, control: false)
case .showBrowserJavaScriptConsole:
// Safari default: Show JavaScript Console.
return StoredShortcut(key: "c", command: true, shift: false, option: true, control: false)
}
}
Expand Down
52 changes: 32 additions & 20 deletions cmuxTests/AppDelegateShortcutRoutingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
XCTAssertNil(self.window(withId: windowId), "Confirming Cmd+Ctrl+W should close the window")
}

func testCmdPhysicalIWithDvorakCharactersDoesNotTriggerShowNotifications() {
func testCmdPhysicalIWithDvorakCharactersDoesNotTriggerCmdIAppShortcuts() {
guard let appDelegate = AppDelegate.shared else {
XCTFail("Expected AppDelegate.shared")
return
Expand All @@ -436,9 +436,15 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
return
}

withTemporaryShortcut(action: .showNotifications) {
// Dvorak: physical ANSI "I" key can produce the character "c".
// This should behave like Cmd+C (copy), not match the Cmd+I app shortcut.
withTemporaryShortcut(
action: .showNotifications,
shortcut: StoredShortcut(key: "i", command: true, shift: false, option: false, control: false)
) {
// Dvorak-style command layouts can translate the semantic shortcut
// into `characters` while leaving the physical ANSI key in
// `charactersIgnoringModifiers`.
// This should behave like Cmd+C (copy), not match either Cmd+I app
// shortcut in this scope.
guard let event = NSEvent.keyEvent(
with: .keyDown,
location: .zero,
Expand All @@ -447,7 +453,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
windowNumber: window.windowNumber,
context: nil,
characters: "c",
charactersIgnoringModifiers: "c",
charactersIgnoringModifiers: "i",
isARepeat: false,
keyCode: 34 // kVK_ANSI_I
) else {
Expand Down Expand Up @@ -488,7 +494,8 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
}
defer { NotificationCenter.default.removeObserver(token) }

// Dvorak: physical ANSI "P" key can produce "l".
// Dvorak-style command layouts can keep the physical ANSI key in
// `charactersIgnoringModifiers` while `characters` reflects Cmd+L.
// This should behave as Cmd+L, not as physical Cmd+P.
guard let event = NSEvent.keyEvent(
with: .keyDown,
Expand All @@ -498,7 +505,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
windowNumber: window.windowNumber,
context: nil,
characters: "l",
charactersIgnoringModifiers: "l",
charactersIgnoringModifiers: "p",
isARepeat: false,
keyCode: 35 // kVK_ANSI_P
) else {
Expand Down Expand Up @@ -740,7 +747,8 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
}
defer { NotificationCenter.default.removeObserver(token) }

// Dvorak: physical ANSI "P" key can produce "l".
// Dvorak-style command layouts can keep the physical ANSI key in
// `charactersIgnoringModifiers` while `characters` reflects Cmd+Shift+L.
// This should behave as Cmd+Shift+L, not as physical Cmd+Shift+P.
guard let event = NSEvent.keyEvent(
with: .keyDown,
Expand All @@ -750,7 +758,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
windowNumber: window.windowNumber,
context: nil,
characters: "l",
charactersIgnoringModifiers: "l",
charactersIgnoringModifiers: "p",
isARepeat: false,
keyCode: 35 // kVK_ANSI_P
) else {
Expand Down Expand Up @@ -781,7 +789,8 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
return
}

// Dvorak: physical ANSI "T" key can produce "y".
// Dvorak-style command layouts can keep the physical ANSI key in
// `charactersIgnoringModifiers` while `characters` reflects Cmd+Option+Y.
// This should not match the Cmd+Option+T app shortcut.
guard let event = NSEvent.keyEvent(
with: .keyDown,
Expand All @@ -791,7 +800,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
windowNumber: window.windowNumber,
context: nil,
characters: "y",
charactersIgnoringModifiers: "y",
charactersIgnoringModifiers: "t",
isARepeat: false,
keyCode: 17 // kVK_ANSI_T
) else {
Expand Down Expand Up @@ -824,7 +833,8 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {

let panelCountBefore = workspace.panels.count

// Dvorak: physical ANSI "W" key can produce ",".
// Dvorak-style command layouts can keep the physical ANSI key in
// `charactersIgnoringModifiers` while `characters` reflects Cmd+,.
// This should not match the Cmd+W close-panel shortcut.
guard let event = NSEvent.keyEvent(
with: .keyDown,
Expand All @@ -834,7 +844,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
windowNumber: window.windowNumber,
context: nil,
characters: ",",
charactersIgnoringModifiers: ",",
charactersIgnoringModifiers: "w",
isARepeat: false,
keyCode: 13 // kVK_ANSI_W
) else {
Expand All @@ -850,7 +860,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
XCTAssertEqual(workspace.panels.count, panelCountBefore)
}

func testCmdIStillTriggersShowNotificationsShortcut() {
func testCmdShiftIStillTriggersShowNotificationsShortcut() {
guard let appDelegate = AppDelegate.shared else {
XCTFail("Expected AppDelegate.shared")
return
Expand All @@ -868,7 +878,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
guard let event = NSEvent.keyEvent(
with: .keyDown,
location: .zero,
modifierFlags: [.command],
modifierFlags: [.command, .shift],
timestamp: ProcessInfo.processInfo.systemUptime,
windowNumber: window.windowNumber,
context: nil,
Expand All @@ -877,7 +887,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
isARepeat: false,
keyCode: 34 // kVK_ANSI_I
) else {
XCTFail("Failed to construct Cmd+I event")
XCTFail("Failed to construct Cmd+Shift+I event")
return
}

Expand Down Expand Up @@ -1232,7 +1242,8 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
defer { NotificationCenter.default.removeObserver(switcherToken) }

withTemporaryShortcut(action: .renameTab) {
// Dvorak: physical ANSI "O" key can produce "r".
// Dvorak-style command layouts can keep the physical ANSI key in
// `charactersIgnoringModifiers` while `characters` reflects Cmd+R.
// This should behave as semantic Cmd+R (rename tab), not Cmd+P.
guard let event = NSEvent.keyEvent(
with: .keyDown,
Expand All @@ -1242,7 +1253,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
windowNumber: window.windowNumber,
context: nil,
characters: "r",
charactersIgnoringModifiers: "r",
charactersIgnoringModifiers: "o",
isARepeat: false,
keyCode: 31 // kVK_ANSI_O
) else {
Expand Down Expand Up @@ -1298,7 +1309,8 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
}
defer { NotificationCenter.default.removeObserver(renameTabToken) }

// Dvorak: physical ANSI "R" key can produce "p".
// Dvorak-style command layouts can keep the physical ANSI key in
// `charactersIgnoringModifiers` while `characters` reflects Cmd+P.
// This should behave as semantic Cmd+P (palette switcher), not Cmd+R.
guard let event = NSEvent.keyEvent(
with: .keyDown,
Expand All @@ -1308,7 +1320,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
windowNumber: window.windowNumber,
context: nil,
characters: "p",
charactersIgnoringModifiers: "p",
charactersIgnoringModifiers: "r",
isARepeat: false,
keyCode: 15 // kVK_ANSI_R
) else {
Expand Down
17 changes: 13 additions & 4 deletions cmuxTests/CmuxWebViewKeyEquivalentTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1291,17 +1291,26 @@ final class SidebarSelectedWorkspaceColorTests: XCTestCase {
XCTAssertEqual(color.alphaComponent, 0.65, accuracy: 0.001)
}
}
final class BrowserDeveloperToolsShortcutDefaultsTests: XCTestCase {
func testSafariDefaultShortcutForToggleDeveloperTools() {
final class BrowserShortcutDefaultsTests: XCTestCase {
func testDefaultShortcutForShowNotifications() {
let shortcut = KeyboardShortcutSettings.Action.showNotifications.defaultShortcut
XCTAssertEqual(shortcut.key, "i")
XCTAssertTrue(shortcut.command)
XCTAssertTrue(shortcut.shift)
XCTAssertFalse(shortcut.option)
XCTAssertFalse(shortcut.control)
}
Comment on lines +1295 to +1302

@greptile-apps greptile-apps Bot Mar 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test added to mismatched class

testDefaultShortcutForShowNotifications tests the showNotifications action, but it has been placed inside the BrowserDeveloperToolsShortcutDefaultsTests class. That class name (and its two existing tests) is scoped to the browser developer-tools shortcuts. Adding an unrelated notification shortcut test here will make the suite harder to navigate over time.

Consider either renaming the class (e.g. BrowserAndNotificationShortcutDefaultsTests) or moving the new test to an existing class that covers general shortcut defaults.


func testDefaultShortcutForToggleDeveloperTools() {
let shortcut = KeyboardShortcutSettings.Action.toggleBrowserDeveloperTools.defaultShortcut
XCTAssertEqual(shortcut.key, "i")
XCTAssertTrue(shortcut.command)
XCTAssertTrue(shortcut.option)
XCTAssertFalse(shortcut.option)
XCTAssertFalse(shortcut.shift)
XCTAssertFalse(shortcut.control)
}

func testSafariDefaultShortcutForShowJavaScriptConsole() {
func testDefaultShortcutForShowJavaScriptConsole() {
let shortcut = KeyboardShortcutSettings.Action.showBrowserJavaScriptConsole.defaultShortcut
XCTAssertEqual(shortcut.key, "c")
XCTAssertTrue(shortcut.command)
Expand Down
Loading