Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
64 changes: 43 additions & 21 deletions Sources/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10575,6 +10575,40 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
return true
}

private func handleContextIndependentShortcut(event: NSEvent) -> Bool {
if matchConfiguredShortcut(event: event, action: .quit) {
return handleQuitShortcutWarning()
}

if matchConfiguredShortcut(event: event, action: .openSettings) {
openPreferencesWindow(debugSource: "shortcut.openSettings")
return true
}

if matchConfiguredShortcut(event: event, action: .reloadConfiguration) {
GhosttyApp.shared.reloadConfiguration(source: "shortcut.reloadConfiguration")
return true
}

if matchConfiguredShortcut(event: event, action: .newWindow) {
openNewMainWindow(preferredWindow: mainWindowForShortcutEvent(event))
return true
}

return false
}

private var contextIndependentShortcutActions: [KeyboardShortcutSettings.Action] {
// Keep main-window lifecycle commands, such as Close Window, on the synchronized
// path because their AppKit delegates depend on the active terminal context.
[
.quit,
.openSettings,
.reloadConfiguration,
.newWindow,
]
}
Comment on lines +10578 to +10610

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.

P2 handleContextIndependentShortcut and contextIndependentShortcutActions must be kept in sync manually

The dispatch logic in handleContextIndependentShortcut and the chord-detection list in contextIndependentShortcutActions enumerate the same four actions in two separate, unconnected places. If a future change adds a new action to the dispatch function but misses the array (or vice versa), chord-shortcut detection for that action will silently break with no compiler or test coverage to catch it. Consider having handleContextIndependentShortcut driven by the same set declared in the property, or at minimum add a comment explicitly calling out the coupling requirement.

Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)


private func handleCustomShortcut(event: NSEvent) -> Bool {
guard event.type == .keyDown else {
clearConfiguredShortcutChordState()
Expand Down Expand Up @@ -10946,6 +10980,15 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
return true
}

if handleContextIndependentShortcut(event: event) {
return true
}

if activeConfiguredShortcutChordPrefixForCurrentEvent == nil,
armConfiguredShortcutChordIfNeeded(event: event, actions: contextIndependentShortcutActions) {
return true
}

let hasEventWindowContext = shortcutEventHasAddressableWindow(event)
let didSynchronizeShortcutContext = synchronizeShortcutRoutingContext(event: event)
if hasEventWindowContext && !didSynchronizeShortcutContext {
Expand Down Expand Up @@ -11036,18 +11079,6 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
return true
}

if matchConfiguredShortcut(event: event, action: .quit) {
return handleQuitShortcutWarning()
}
if matchConfiguredShortcut(event: event, action: .openSettings) {
openPreferencesWindow(debugSource: "shortcut.openSettings")
return true
}
if matchConfiguredShortcut(event: event, action: .reloadConfiguration) {
GhosttyApp.shared.reloadConfiguration(source: "shortcut.reloadConfiguration")
return true
}

if matchConfiguredShortcut(event: event, action: .toggleFullScreen) {
guard let targetWindow = mainWindowForShortcutEvent(event) else {
return false
Expand Down Expand Up @@ -11078,15 +11109,6 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
return true
}

// New Window: Cmd+Shift+N
// Handled here instead of relying on SwiftUI's CommandGroup menu item because
// after a browser panel has been shown, SwiftUI's menu dispatch can silently
// consume the key equivalent without firing the action closure.
if matchConfiguredShortcut(event: event, action: .newWindow) {
openNewMainWindow(preferredWindow: mainWindowForShortcutEvent(event))
return true
}

// Open Folder: Cmd+O
// Handled here to prevent AppKit's default NSDocumentController from opening
// the Documents folder when SwiftUI menu dispatch fails due to focus bugs.
Expand Down
55 changes: 55 additions & 0 deletions cmuxTests/AppDelegateShortcutRoutingTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2791,6 +2791,61 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
XCTAssertEqual(workspace.panels.count, panelCountBefore)
}

func testCmdCommaOpensSettingsFromWindowWithoutTerminalShortcutContext() {
guard let appDelegate = AppDelegate.shared else {
XCTFail("Expected AppDelegate.shared")
return
}

let windowId = appDelegate.createMainWindow()
defer { closeWindow(withId: windowId) }

let auxiliaryWindow = NSWindow(
contentRect: NSRect(x: 0, y: 0, width: 360, height: 240),
styleMask: [.titled, .closable],
backing: .buffered,
defer: false
)
auxiliaryWindow.isReleasedWhenClosed = false
auxiliaryWindow.identifier = NSUserInterfaceItemIdentifier("cmux.shortcut-routing-test")
auxiliaryWindow.makeKeyAndOrderFront(nil)
RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05))
defer {
auxiliaryWindow.orderOut(nil)
auxiliaryWindow.close()
RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05))
}

var settingsOpenCount = 0
#if DEBUG
SettingsWindowPresenter.resetForTests()
defer { SettingsWindowPresenter.resetForTests() }
#endif
SettingsWindowPresenter.configure(openWindow: {
settingsOpenCount += 1
})
Comment on lines +2819 to +2826

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.

P1 Global presenter state leaked in non-DEBUG test runs

SettingsWindowPresenter.configure(openWindow:) is an unconditional production API that writes directly to the type's static var openWindow. The matching cleanup (SettingsWindowPresenter.resetForTests()) is guarded by #if DEBUG. In a non-DEBUG test run the cleanup is compiled away, leaving the global openWindow closure set for the remainder of the XCTest session — any later test that calls SettingsWindowPresenter.show() (or openPreferencesWindow) would invoke a dangling test closure against a no-longer-valid settingsOpenCount. The configure(openWindow:) call should be placed inside the same #if DEBUG fence as its cleanup.

Comment on lines +2819 to +2826

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

SettingsWindowPresenter.configure called unconditionally while resetForTests() is #if DEBUG-gated — potential state leak in non-Debug test runs.

configure(openWindow:) sets a global handler on SettingsWindowPresenter in all build configurations, but the matching resetForTests() teardown only runs in DEBUG. In a Release-configuration test binary, the closure (capturing settingsOpenCount) is installed and never cleared, and subsequent tests that open settings may observe the stale handler.

Move the configure call inside the same #if DEBUG guard so it is always paired with its cleanup:

🛡️ Proposed fix
 var settingsOpenCount = 0
 `#if` DEBUG
 SettingsWindowPresenter.resetForTests()
+SettingsWindowPresenter.configure(openWindow: {
+    settingsOpenCount += 1
+})
 defer { SettingsWindowPresenter.resetForTests() }
 `#endif`
-SettingsWindowPresenter.configure(openWindow: {
-    settingsOpenCount += 1
-})
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 2819 - 2826,
The test installs a global handler via
SettingsWindowPresenter.configure(openWindow:) unconditionally while the
teardown SettingsWindowPresenter.resetForTests() and its defer live inside a `#if`
DEBUG block, risking a leaked handler in non-DEBUG test runs; move the
SettingsWindowPresenter.configure(openWindow:) call (and the settingsOpenCount
setup if desired) inside the same `#if` DEBUG / `#endif` region alongside the
resetForTests() and its defer so the configure and reset are always paired
(refer to SettingsWindowPresenter.configure(openWindow:) and
SettingsWindowPresenter.resetForTests()).


guard let event = makeKeyDownEvent(
key: ",",
modifiers: [.command],
keyCode: 43,
windowNumber: auxiliaryWindow.windowNumber
) else {
XCTFail("Failed to construct Cmd+, event")
return
}

#if DEBUG
XCTAssertTrue(
appDelegate.debugHandleCustomShortcut(event: event),
"Cmd+, should remain app-scoped when the key event comes from a non-terminal window"
)
#else
XCTFail("debugHandleCustomShortcut is only available in DEBUG")
#endif
XCTAssertEqual(settingsOpenCount, 1)
}

func testCmdIStillTriggersShowNotificationsShortcut() {
guard let appDelegate = AppDelegate.shared else {
XCTFail("Expected AppDelegate.shared")
Expand Down
Loading