Repository navigation
Make Settings open structurally reliable: AppKit-owned window lifecycle #7783
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.
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
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
33313fc
Add failing regression tests: Settings open must always produce a vis…
austinywang 94b7af2
Make Settings open structurally reliable: AppKit-owned window lifecycle
austinywang 8977ae1
Address review: deliver navigation under hidden app, surface menu-pat…
austinywang ee23c79
Address codex review: no-activate opens don't steal focus, determinis…
austinywang 708080a
Address review round 2: supersede-safe queued navigation, Settings si…
austinywang 40edeae
Fix workflow-guard-tests: move Settings-open entrypoints out of AppDe…
austinywang 7788d21
Address review round 3: content-readiness gating for navigation, brow…
austinywang 43e05e5
Fix CI test compile and Cursor findings: decouple test from package s…
austinywang ebbe77b
Fix Swift warning budget: drop deprecated activation option, remove m…
austinywang 9526370
Address review round 4: reentrancy-safe teardown recovery
austinywang File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 changes: 8 additions & 3 deletions
11
...olSocket/Sources/CmuxControlSocket/Coordinator/System/ControlSettingsOpenResolution.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,15 @@ | ||
| /// The outcome of `settings.open` (the legacy `v2SettingsOpen` body): the app | ||
| /// validates the target against its `SettingsNavigationTarget` enum and | ||
| /// schedules the window presentation. | ||
| /// presents the window before replying, so `opened` means a window actually | ||
| /// materialized (https://github.com/manaflow-ai/cmux/issues/7775). | ||
| public enum ControlSettingsOpenResolution: Sendable, Equatable { | ||
| /// The `target` param did not name a known settings pane. | ||
| case invalidTarget | ||
| /// The settings window presentation was scheduled. Carries the resolved | ||
| /// target raw value (`"general"` when no target was given). | ||
| /// The settings window was presented (or ordered front under a hidden | ||
| /// app). Carries the resolved target raw value (`"general"` when no | ||
| /// target was given). | ||
| case opened(target: String) | ||
| /// The app could not make a settings window visible. `message` carries | ||
| /// the diagnostic reason from the window presenter. | ||
| case failed(message: String) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import AppKit | ||
|
|
||
| /// The Settings-open entrypoints shared by the app menu, ⌘,, the command | ||
| /// palette, help commands, and the menu-bar extra. Split out of | ||
| /// `AppDelegate.swift` (file-length budget) alongside the AppKit-owned | ||
| /// Settings window lifecycle (https://github.com/manaflow-ai/cmux/issues/7777). | ||
| extension AppDelegate { | ||
| @MainActor | ||
| static func presentPreferencesWindow( | ||
| navigationTarget: SettingsNavigationTarget? = nil, | ||
| showFallbackSettingsWindow: (@MainActor (SettingsNavigationTarget?) -> Void)? = nil, | ||
| // The legacy body also passed .activateIgnoringOtherApps; the option | ||
| // is deprecated and documented as a no-op on macOS 14+ (this target's | ||
| // minimum), so dropping it is behavior-neutral. | ||
| activateApplication: @MainActor () -> Void = { | ||
| NSRunningApplication.current.activate(options: [.activateAllWindows]) | ||
| } | ||
| ) { | ||
| #if DEBUG | ||
| cmuxDebugLog("settings.open.present path=appkitWindow") | ||
| #endif | ||
| if let showFallbackSettingsWindow { | ||
| showFallbackSettingsWindow(navigationTarget) | ||
| } else if case .failed = SettingsWindowPresenter.show(navigationTarget: navigationTarget) { | ||
| // The presenter already logged the loud failure diagnostics; | ||
| // surface the failed menu/⌘, action instead of silently activating. | ||
| NSSound.beep() | ||
| return | ||
| } | ||
| activateApplication() | ||
| #if DEBUG | ||
| cmuxDebugLog("settings.open.present activate=1") | ||
| #endif | ||
| } | ||
|
|
||
| @MainActor | ||
| func openPreferencesWindow(debugSource: String, navigationTarget: SettingsNavigationTarget? = nil) { | ||
| #if DEBUG | ||
| cmuxDebugLog("settings.open.request source=\(debugSource)") | ||
| #endif | ||
| Self.presentPreferencesWindow(navigationTarget: navigationTarget) | ||
| } | ||
|
|
||
| @objc func openPreferencesWindow() { | ||
| openPreferencesWindow(debugSource: "appDelegate") | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.