Repository navigation
Fix Cmd+, settings shortcut routing - #3722
lawrencecchen wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAppDelegate's shortcut handling is refactored to extract context-independent shortcuts (quit, settings, reload configuration, new window) into an early dispatch helper. The main ChangesShortcut Routing Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 13 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes the Cmd+, settings shortcut being silently dropped when the key event originates from a window that has no addressable terminal context (e.g. the settings window itself). The fix extracts the four app-global shortcuts (quit, openSettings, reloadConfiguration, newWindow) into a new
Confidence Score: 4/5The production routing change is correct and targeted; the main risk is in the new test where uncleaned global presenter state can pollute the XCTest session in non-DEBUG builds. The production AppDelegate change moves four well-understood shortcuts before the sync gate and is covered by the new regression test. The test itself has a real defect: SettingsWindowPresenter.configure(openWindow:) mutates global state unconditionally while the matching resetForTests() cleanup is compiled away in non-DEBUG, leaving a stale closure in the presenter for the rest of the test run. cmuxTests/AppDelegateShortcutRoutingTests.swift — the #if DEBUG guard placement around configure/cleanup needs attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[NSEvent keyDown arrives at handleCustomShortcut] --> B{Recorder active?}
B -- yes --> Z[return false]
B -- no --> C[Notifications popover / sidebar checks]
C --> D[handleContextIndependentShortcut\nquit · openSettings · reloadConfig · newWindow]
D -- matched --> Y[return true]
D -- no match --> E{activeChordPrefix == nil?\narmChordIfNeeded for contextIndependentActions?}
E -- chord armed --> Y2[return true chord prefix consumed]
E -- no chord --> F[shortcutEventHasAddressableWindow\nsynchronizeShortcutRoutingContext]
F -- unresolved window ctx --> Z2[return false]
F -- synchronized --> G[find · toggleFullScreen · etc. dispatchers]
G --> H[return true / false]
Reviews (1): Last reviewed commit: "Fix context-independent shortcut routing" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| var settingsOpenCount = 0 | ||
| #if DEBUG | ||
| SettingsWindowPresenter.resetForTests() | ||
| defer { SettingsWindowPresenter.resetForTests() } | ||
| #endif | ||
| SettingsWindowPresenter.configure(openWindow: { | ||
| settingsOpenCount += 1 | ||
| }) |
There was a problem hiding this comment.
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.
| 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, | ||
| ] | ||
| } |
There was a problem hiding this comment.
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)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 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()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e33ce9cb-17d2-4327-a738-706a6de3d823
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
| var settingsOpenCount = 0 | ||
| #if DEBUG | ||
| SettingsWindowPresenter.resetForTests() | ||
| defer { SettingsWindowPresenter.resetForTests() } | ||
| #endif | ||
| SettingsWindowPresenter.configure(openWindow: { | ||
| settingsOpenCount += 1 | ||
| }) |
There was a problem hiding this comment.
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()).
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary:
Tests:
Notes:
Note
Medium Risk
Adjusts keyboard shortcut routing order, which can change how global/app-level shortcuts are handled across different window contexts. Risk is limited to input/shortcut behavior but could cause regressions in chord handling or window-specific routing.
Overview
Fixes shortcut routing so app-scoped shortcuts (notably Cmd+, settings) are handled even when the key event originates from a window that lacks terminal shortcut context, by routing a small set of context-independent actions (
quit,openSettings,reloadConfiguration,newWindow) before the terminal-context synchronization gate.Adds a regression test ensuring Cmd+, opens Settings from an auxiliary (non-terminal) window, and updates chord arming so these context-independent actions can still participate in configured shortcut chords without requiring terminal window context.
Reviewed by Cursor Bugbot for commit 2eea49a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes the Settings shortcut (Cmd+,) so it works from non-terminal windows by handling app-level shortcuts before terminal routing. App-scoped actions like Quit, Settings, Reload Config, and New Window now work from any window.
Written for commit 2eea49a. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests