Repository navigation
Make Settings open structurally reliable: AppKit-owned window lifecycle - #7783
Merged
Merged
Conversation
…ible window End-to-end tests for the recurring "Settings won't open" family (#7777, #7775, #5770, #4053): every SettingsWindowPresenter.show() must end with a visible Settings window — from a fresh state, after close, after open/close churn, while a previous window is mid-close, and after the window was stranded off every active screen. These are red on the current SwiftUI-scene-based open path, which hands the request to openWindow(id:) and can silently no-op. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes #7777. The Settings window is no longer created through a SwiftUI Window scene's openWindow(id:), which has no failure callback and could wedge permanently (relaunch-while-open #7775, scene mid-teardown #5770), leaving menu, Cmd-comma, and CLI opens silently dead until app restart. SettingsWindowPresenter is now the single source of truth for the window lifecycle: it synchronously builds the NSWindow itself (SettingsWindowFactory: NSHostingController(SettingsWindowRoot) with toolbar/title scene bridging — the same AppKit-owned model as the main window and TaskManagerWindowController), tears down any unusable existing window instead of re-fronting it, clamps stranded frames onto a visible screen, verifies isVisible before returning, recreates once on failure, and fails loudly otherwise. Closing strips the identifier and releases the content tree so a closed window can never absorb a future open request. The deferral flags, 500ms verification timer, and retry machinery from PR #5806 are deleted; the CLI settings.open reply now reflects the real outcome (opened iff a window materialized; new .failed resolution maps to a socket error), and navigation targets are delivered on first open. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSettings presentation now uses an AppKit-owned window with synchronous result reporting, explicit failure diagnostics, persisted selection state, navigation delivery, lifecycle recovery, and expanded regression coverage. ChangesSettings window presentation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ControlCommandCoordinator
participant TerminalController
participant SettingsWindowPresenter
participant SettingsWindowFactory
participant SettingsWindowHostRoot
ControlCommandCoordinator->>TerminalController: settings.open(target)
TerminalController->>SettingsWindowPresenter: show(navigationTarget:activateApp:)
SettingsWindowPresenter->>SettingsWindowFactory: makeSettingsWindow()
SettingsWindowFactory-->>SettingsWindowPresenter: NSWindow
SettingsWindowPresenter->>SettingsWindowHostRoot: order window front
SettingsWindowHostRoot-->>SettingsWindowPresenter: deliver pending navigation
SettingsWindowPresenter-->>TerminalController: presented or failed result
TerminalController-->>ControlCommandCoordinator: opened or failed resolution
Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (20 passed)
✨ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR moves Settings window presentation to an AppKit-owned lifecycle. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (9): Last reviewed commit: "Address review round 4: reentrancy-safe ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+126
to
+133
| if NSApp.isHidden && !activateApp { | ||
| // Ordering front succeeded as far as AppKit allows without | ||
| // unhiding the app; the window appears on unhide. | ||
| Self.log.notice( | ||
| "settings.window.show ordered front while app is hidden; deferring visibility to unhide" | ||
| ) | ||
| return .orderedWhileAppHidden | ||
| } |
Contributor
There was a problem hiding this comment.
When settings.open runs with activate=false while the app is hidden and a settings window already exists, this branch returns orderedWhileAppHidden before posting the requested target. The reused window will not run the host root's first-appear consumer again, so the CLI can report opened=true and later unhide to the old pane instead of the requested one.
Suggested change
| if NSApp.isHidden && !activateApp { | |
| // Ordering front succeeded as far as AppKit allows without | |
| // unhiding the app; the window appears on unhide. | |
| Self.log.notice( | |
| "settings.window.show ordered front while app is hidden; deferring visibility to unhide" | |
| ) | |
| return .orderedWhileAppHidden | |
| } | |
| if NSApp.isHidden && !activateApp { | |
| // Ordering front succeeded as far as AppKit allows without | |
| // unhiding the app; the window appears on unhide. | |
| deliverNavigation(reusedExistingWindow: reusedExisting) | |
| Self.log.notice( | |
| "settings.window.show ordered front while app is hidden; deferring visibility to unhide" | |
| ) | |
| return .orderedWhileAppHidden | |
| } |
Contributor
Author
There was a problem hiding this comment.
Fixed in 8977ae1 exactly as suggested — deliverNavigation(reusedExistingWindow:) is called before returning orderedWhileAppHidden.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Addressed across rounds 1/3 of this PR and completed in follow-up PR #7800: the hidden-app branch delivers navigation to ready reused content before returning orderedWhileAppHidden, an unready/fresh window keeps the target pending until the content's onAppear drains it, and #7800 makes that readiness signal instance-scoped (routed to the presenter that owns the window rather than the singleton), so the CLI can no longer report opened=true while the requested pane is dropped.
— Claude Code
…h failure, truthful UI-test capture - deliverNavigation now runs before the orderedWhileAppHidden return, so a reused live Settings window still receives the requested pane when the app is hidden and activate=false (Cursor/Greptile finding). - presentPreferencesWindow consumes the show result: on .failed it beeps instead of silently activating, matching the CLI's error surfacing. - The DEBUG UI-test capture records opened from the verified outcome, not the request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/SettingsWindowPresenterTests.swift`:
- Around line 490-541: Extract the duplicated private ReopenSettingsOnWillClose
helper into a shared test helper file, preserving its
NSWindow.willCloseNotification observation, reopen callback, and stopObserving
behavior. Remove the local class definitions from both
SettingsWindowOpenRegressionTests.swift and SettingsWindowPresenterTests.swift,
and update both suites to use the shared helper.
In `@Sources/App/SettingsWindowPresenter.swift`:
- Around line 104-133: In the hidden-app branch of the presentation loop in
SettingsWindowPresenter, call deliverNavigation(reusedExistingWindow:
reusedExisting) before returning .orderedWhileAppHidden, ensuring reused windows
receive the pending navigation target before unhide. Preserve the existing
logging and return behavior.
🪄 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: b992813c-4b80-46de-afc3-df6dd910d9b9
📒 Files selected for processing (15)
Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/System/ControlCommandCoordinator+SystemMisc.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/System/ControlSettingsOpenResolution.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsWindowScene.swiftResources/Localizable.xcstringsSources/App/SettingsWindowFactory.swiftSources/App/SettingsWindowOpenOutcome.swiftSources/App/SettingsWindowPresenter.swiftSources/AppDelegate.swiftSources/Panels/BrowserPanelView.swiftSources/TerminalController+ControlSystemContext.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SettingsWindowOpenRegressionTests.swiftcmuxTests/SettingsWindowPresenterTests.swiftcmuxUITests/BrowserImportProfilesUITests.swift
💤 Files with no reviewable changes (2)
- Sources/App/SettingsWindowOpenOutcome.swift
- cmuxUITests/BrowserImportProfilesUITests.swift
…tic mid-close rejection, fail-closed nil context - show(activateApp: false) now only orders the window in (no makeKey, no parent-window raise, no unhide/activate), honoring the socket no-focus-steal contract for settings.open --activate=false. - SettingsHostWindow records close-begin so show() deterministically refuses to reuse a dying window regardless of notification-observer order; demolish/strip split keeps mid-close teardown re-entrant-safe. - settings.open with a missing control-system context now returns an unavailable error instead of synthesizing opened=true. - Pure geometry statics moved to SettingsWindowGeometry.swift so the presenter stays under the 500-line budget threshold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/App/SettingsWindowPresenter.swift (1)
157-169: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDo not return AppKit diagnostics to socket callers.
failureReasonexposes window state, screen count, and frame geometry;controlSettingsOpenforwards it verbatim in.failed(message:). Keep these diagnostics inLoggerand return a stable product-safe error.Proposed fix
- return .failed(reason: failureReason) + return .failed(reason: "Settings window could not be opened")As per coding guidelines, “User-facing errors, alerts, command output, API error bodies, and recovery copy must not expose implementation details.”
Also applies to: 298-309
🤖 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 `@Sources/App/SettingsWindowPresenter.swift` around lines 157 - 169, Do not expose the AppKit diagnostic failureReason through the socket response. In the presenter’s failed result path and the controlSettingsOpen caller, continue logging failureReason for diagnostics but return a stable, product-safe error message in .failed(message:) instead of forwarding the window state, screen count, or frame geometry.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@Sources/App/SettingsWindowPresenter.swift`:
- Around line 157-169: Do not expose the AppKit diagnostic failureReason through
the socket response. In the presenter’s failed result path and the
controlSettingsOpen caller, continue logging failureReason for diagnostics but
return a stable, product-safe error message in .failed(message:) instead of
forwarding the window state, screen count, or frame geometry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7cf06dbf-534e-408d-965c-18c5b1e196ca
📒 Files selected for processing (2)
Sources/App/SettingsWindowPresenter.swiftSources/AppDelegate.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/App/SettingsWindowPresenter.swift (1)
16-19: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSanitize the settings-open failure message
Sources/App/SettingsWindowPresenter.swift:16-18buildsfailureReasonfrom AppKit window/app state, andSources/TerminalController+ControlSystemContext.swift:238-242passes it straight through as.failed(message: reason). Return a product-level error to the caller and keep the detailed state in logs only.🤖 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 `@Sources/App/SettingsWindowPresenter.swift` around lines 16 - 19, Update the settings-open failure flow around SettingsWindowPresenter’s failed(reason:) and TerminalController’s .failed(message: reason) handling so callers receive a stable product-level error message instead of diagnostic window/app state; retain the detailed failureReason only in the appropriate log entry.
🤖 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.
Outside diff comments:
In `@Sources/App/SettingsWindowPresenter.swift`:
- Around line 16-19: Update the settings-open failure flow around
SettingsWindowPresenter’s failed(reason:) and TerminalController’s
.failed(message: reason) handling so callers receive a stable product-level
error message instead of diagnostic window/app state; retain the detailed
failureReason only in the appropriate log entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f635f29a-d7cf-44c1-bc0a-d6e71d0a6aaf
📒 Files selected for processing (6)
Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/System/ControlCommandCoordinator+SystemMisc.swiftSources/App/SettingsWindowFactory.swiftSources/App/SettingsWindowGeometry.swiftSources/App/SettingsWindowPresenter.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SettingsWindowPresenterTests.swift
…debar toggle routing - A fresh window's deferred navigation post is generation-guarded: a newer targeted show that delivered in the meantime supersedes the queued post instead of being overridden by it. - The shared Toggle Left Sidebar command now routes to the Settings split view when the Settings window is key (the AppKit-hosted window lost SwiftUI SidebarCommands with the scene removal); the package root listens for the toggle request and flips columnVisibility. - New SettingsWindowNavigationRoutingTests cover queued delivery, supersession, and sidebar-command routing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…legate.swift AppDelegate.swift is over the 900-line hard cap, so it may not grow at all in a PR; the presentPreferencesWindow failure-surfacing change added 5 net lines. Move presentPreferencesWindow/openPreferencesWindow into Sources/App/AppDelegateSettingsPresentation.swift (AppDelegate now shrinks by 29 lines net). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ser entrypoint failure surfacing, Khmer localization - Navigation posts only after the Settings content signals readiness (host root onAppear); an NSWindow existing is not enough, so a rapid second targeted open can no longer post into the void and lose the pane. Latest target stays pending until the content is live. - openBrowserImportSettings routes through the shared AppDelegate.presentPreferencesWindow so a failed presentation beeps instead of silently doing nothing. - settings.window.runtimeUnavailable gains the Khmer (km) translation the catalog ships. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ymbol, preserve pending target on untargeted shows - SettingsSidebarToggleRecorder observes the raw notification name so the test target does not depend on package-symbol visibility through @testable import, which differs across toolchains (CI failed with 'cannot find SettingsWindowRoot in scope' while local Xcode compiled). - An untargeted show() no longer erases a still-undelivered pending navigation target (Cursor findings); regression test added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ain-actor default argument - activateIgnoringOtherApps is a documented no-op on macOS 14+ (this target's minimum); dropping it from the moved presentPreferencesWindow default closure is behavior-neutral and keeps the new file warning-free. - handleSidebarToggleIfSettingsWindowIsKey takes keyWindow explicitly; a NSApp.keyWindow default argument is evaluated outside the main actor and warns under strict concurrency. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 `@Sources/App/AppDelegateSettingsPresentation.swift`:
- Around line 8-34: Remove the showFallbackSettingsWindow parameter and its
conditional branch from presentPreferencesWindow, making
SettingsWindowPresenter.show(navigationTarget:) the sole presentation path.
Preserve the existing failed-result handling, beep, early return, and
activateApplication behavior only after successful presentation.
🪄 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: cebbe4dc-7156-44fe-adbd-4ba2c40d8511
📒 Files selected for processing (10)
Resources/Localizable.xcstringsSources/App/AppDelegateSettingsPresentation.swiftSources/App/SettingsWindowFactory.swiftSources/App/SettingsWindowPresenter.swiftSources/AppDelegate.swiftSources/Panels/BrowserPanelView.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SettingsWindowNavigationRoutingTests.swiftcmuxTests/SettingsWindowPresenterTests.swift
demolish() closes windows synchronously, and a foreign willClose observer may re-enter show() from that close. The retry loop now re-checks for a usable existing window on every attempt, so a healthy replacement created by re-entrant show() is adopted instead of duplicated, and a small re-entrant depth bound converts a pathological reopen-on-close observer combined with persistent presentation failure into a loud .failed instead of unbounded recursion. Covered by reentrantReopenDuringTeardownAdoptsReplacementWindow and pathologicalReopenOnCloseFailsLoudlyInsteadOfRecursing. unusableWindowReason moved to the pure-helpers file for the length budget. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
enabled auto-merge (squash)
July 10, 2026 03:08
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9526370. Configure here.
| reusedExisting: reusedExisting | ||
| ) | ||
| Self.log.error("settings.window.show \(failureReason, privacy: .public)") | ||
| demolish(window) |
There was a problem hiding this comment.
Miniaturized window visibility demolish
Medium Severity
After orderFront, success is gated on window.isVisible (or hidden-app orderedWhileAppHidden). A reused Settings window that is still in the Dock or not yet visible on the same run-loop turn after deminiaturize can fail that check while the app is visible, so performShow treats a valid reuse as failure and demolish tears down the live window instead of presenting it.
Reviewed by Cursor Bugbot for commit 9526370. Configure here.
Contributor
Author
There was a problem hiding this comment.
Fixed in follow-up PR #7800 (this PR merged before the fix could land here). performShow now captures wasMiniaturized before ordering front; a window that was miniaturized and is deminiaturizing is treated as a successful presentation (visibility follows the unminiaturize animation) instead of being demolished as a failure. Covered by reusedMiniaturizedWindowIsNotDemolishedWhileDeminiaturizing, using a test window that models AppKit's deferred post-deminiaturize visibility.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Design update in #7800 after its own review round: preserving the window through the async animation created a pending state every consumer mistook for final success, so the final fix replaces a Dock-miniaturized window upfront with a fresh, immediately visible one (saved frame restored). Your reported bug shape — a valid miniaturized reuse consuming a failure attempt / being demolished as an error — is gone either way: the replacement is intentional, logged, and always ends in a visible window with a synchronous .presented result.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Final design in #7800 (after an empirical AppKit probe): the miniaturized window is reused, not replaced — probing shows deminiaturize alone leaves isVisible false on the same run-loop turn, but deminiaturize immediately followed by orderFrontRegardless (the presenter's exact sequence) commits visibility on the same turn. So the reuse keeps unsaved Settings state AND satisfies the verified visible-on-return contract synchronously. A stalled-commit simulation test pins the fallback (replacement with a fresh visible window) for any OS where that ever changes.
— Claude Code
austinywang
added a commit
that referenced
this pull request
Jul 10, 2026
- presentPreferencesWindow: replace the result-less showFallbackSettingsWindow seam with a presentSettingsWindow seam that must report a SettingsWindowShowResult; the failed -> beep gate now applies to every path (CodeRabbit) - performShow: a reused window mid-deminiaturize from the Dock is a successful presentation, not a failure to demolish (Cursor Bugbot) - performShow: a failed request clears its own pending navigation target so it cannot leak into a later untargeted open (codex P2) - Content readiness is instance-scoped: the factory takes onContentAppear and the presenter passes itself into the factory; singleton shims removed (codex P2) - The three settings-window suites nest under one serialized parent suite so they cannot interleave on NSApp windows / frame autosave state (codex P2) - Budget mechanics: presentPreferencesWindow tests extracted to AppDelegatePresentPreferencesWindowTests.swift; presentationFailureReason moved to SettingsWindowGeometry.swift; no TSV changes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
added a commit
that referenced
this pull request
Jul 10, 2026
) * Address post-merge review findings from #7783 - presentPreferencesWindow: replace the result-less showFallbackSettingsWindow seam with a presentSettingsWindow seam that must report a SettingsWindowShowResult; the failed -> beep gate now applies to every path (CodeRabbit) - performShow: a reused window mid-deminiaturize from the Dock is a successful presentation, not a failure to demolish (Cursor Bugbot) - performShow: a failed request clears its own pending navigation target so it cannot leak into a later untargeted open (codex P2) - Content readiness is instance-scoped: the factory takes onContentAppear and the presenter passes itself into the factory; singleton shims removed (codex P2) - The three settings-window suites nest under one serialized parent suite so they cannot interleave on NSApp windows / frame autosave state (codex P2) - Budget mechanics: presentPreferencesWindow tests extracted to AppDelegatePresentPreferencesWindowTests.swift; presentationFailureReason moved to SettingsWindowGeometry.swift; no TSV changes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Deminiaturize outcome is honest: .deminiaturizing result with bounded self-heal verification Review P2 on the previous commit: returning .presented while the window is verifiably not visible violated the presenter's verified-result contract — a stalled Dock transition would be a silent success. The branch now: - returns a distinct .deminiaturizing result (window live, AppKit animating it in) instead of claiming .presented - runs a bounded follow-up that demolishes the window loudly if visibility never arrives (skipped if re-miniaturized or the app hid meanwhile), so the next open self-heals with a fresh window - checks the hidden-app branch before the deminiaturize branch, since a hidden app defers visibility regardless of the animation - maps .deminiaturizing to opened in the settings.open socket reply Also fixes the workflow-guard-tests failure: the CI budget enforces per-PR growth (+63 > 25 allowance) on tracked SettingsWindowPresenterTests.swift, so the miniaturized-reuse test and Dock-simulation window moved to the untracked SettingsWindowNavigationRoutingTests.swift, and clampToVisibleAreaIfNeeded moved to SettingsWindowGeometry.swift to keep the presenter under the 500-line tracking threshold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Replace async .deminiaturizing state with synchronous upfront replacement; activate only after visibility Review round 2 found four P2s all rooted in .deminiaturizing being an asynchronous state that consumers (socket reply, UI-test capture, repeated shows) treated as final success. Eliminate the state instead of patching each consumer: - A Dock-miniaturized window can never satisfy the verified visible-on-return contract (deminiaturization animates across run-loop turns), so usableExistingWindow now demolishes it upfront and the show presents a fresh, immediately visible window at the saved frame. Every result is synchronous truth again: .presented / .orderedWhileAppHidden / .failed. The socket mapping, UI-test capture, and repeated-show paths need no special cases. - Activation is a post-verification step: orderFrontWithoutActivation orders the window in first, and unhide/activate/makeKey run only after the window is verifiably visible (or when unhiding a hidden app is itself what visibility waits on). A failed presentation can no longer activate the app or steal focus as a side effect. Tests: miniaturizedSettingsWindowIsReplacedWithAFreshVisibleWindow and failedShowNeverActivatesOrKeysAnInvisibleWindow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Preserve a miniaturized Settings window: deminiaturize+orderFront commits visibility synchronously Review round 3: replacing a Dock-miniaturized window destroyed unsaved Settings drafts (P1), and the hidden-app verification unhide left the app activated on failure (P2). Empirical probe (macOS 26): NSWindow.deminiaturize alone leaves isVisible false on the same run-loop turn, but deminiaturize followed immediately by orderFrontRegardless commits visibility on the SAME turn. That is exactly the presenter's sequence, so the miniaturized window can be REUSED — its SwiftUI tree and unsaved edits intact — while the verified visible-on-return contract still holds with no async state: - usableExistingWindow treats a miniaturized window as usable again; orderFrontWithoutActivation deminiaturizes before ordering front - if an OS ever breaks the same-turn commit, the attempt loop falls back to replacing the window with a fresh visible one (covered by a stalled- commit simulation test) — never a pending state reported as success - the hidden-app unhide gamble is recorded and the failure exit re-hides the app, so a failed presentation leaves focus exactly as found Tests: miniaturizedSettingsWindowIsReusedAndVisibleOnReturn (simulation faithful to the probed AppKit behavior) and stalledDeminiaturizeCommitFallsBackToAFreshVisibleWindow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Preserve minimized windows across async deminiaturize; never activate to verify Review round 4: - P1: the same-turn deminiaturize commit was probed only on macOS 26 while cmux supports macOS 14+. performShow now waits out an initiated deminiaturization with a bounded main-run-loop pump (the same nested pumping AppKit's modal/menu tracking uses) before concluding anything, so a live window full of unsaved edits survives on every supported OS; replacement remains only for a transition that never lands (genuinely wedged). New test asyncDeminiaturizeCommitIsAwaitedWithoutReplacingTheWindow proves the later-turn commit is awaited without replacing the window; the stalled test now overrides the settle timeout to stay fast. - P2: hidden-app verification uses NSApp.unhideWithoutActivation() (the API that exists for exactly this) instead of full activation, so a request that ultimately fails has never activated the app or redirected focus; activation runs strictly after verified visibility, and the failure exit re-hides. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Split presentation-contract tests into their own file to stay under the length threshold SettingsWindowNavigationRoutingTests.swift reached 540 lines (untracked files must stay under 500). The Dock-deminiaturize, failed-activation, and re-entrant teardown tests move to SettingsWindowPresentationContractTests (wired into the pbxproj, nested under the same serialized parent suite); the window helpers become target-internal and are shared. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Skip the deminiaturize wait for hidden non-activating opens; abort on mid-wait ownership change Review round 5: - The hidden-app non-activating branch (.orderedWhileAppHidden) now runs before the deminiaturize wait: under a hidden app no amount of waiting produces visibility, and this is the synchronous socket path that must not stall for the full settle timeout. - The nested run-loop pump can process a close (or a re-entrant show) while the outer attempt still holds the window: awaitVisibility now also exits when window ownership changes, and the attempt aborts — adopting an already-visible replacement, or failing loudly — instead of building a fresh window that resurrects one the user just closed. New test: closingTheWindowDuringTheDeminiaturizeWaitIsNotResurrected. - logExistingWindowState moved to SettingsWindowGeometry.swift to keep the presenter under the 500-line threshold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Coalesce re-entrant opens onto an in-flight deminiaturization; adopted replacements honor activation Review round 6 (Cursor + codex): - Cursor: adopting a visible replacement window after a mid-wait ownership change returned .presented without honoring activateApp — an activating open (menu, CLI with activation) could report success while the window was never keyed and the app stayed inactive. activateAndSurface now runs for the adopted window. Test: reentrantReplacementDuringDeminiaturizeWaitIsActivated. - codex P1: deminiaturize clears isMiniaturized before visibility lands, so a re-entrant show during the bounded wait saw a plain invisible window and demolished the live transition (destroying unsaved edits). The presenter now tracks the window whose deminiaturization is being awaited and re-entrant shows coalesce onto the transition. Test: reentrantShowDuringDeminiaturizeWaitCoalescesOntoTheTransition. - Navigation delivery moved to SettingsWindowNavigationDelivery.swift (wired into the app target) to keep the presenter under the 500-line threshold; its state properties become target-internal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Convert AppDelegatePresentPreferencesWindowTests to Swift Testing Review round 7: the extraction from AppDelegateShortcutRoutingTests carried its XCTest style along, but repo policy reserves XCTest for UI suites — non-UI unit tests use Swift Testing. Same five tests, now @Suite/@Test/#expect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix deminiaturize-wait tests on CI: schedule test events on the run loop, not the main queue The three mid-wait tests failed on CI: they scheduled their mid-wait events (window close, re-entrant show, delayed commit) via DispatchQueue.main.asyncAfter, but the presenter's bounded wait pumps the run loop from inside the current main-queue job, and a nested pump cannot drain the serial main dispatch queue — the events never fired and every wait timed out into the replacement fallback. Timer is a run-loop source, fires during the pump, and faithfully models how AppKit delivers the real transition (CA/WindowServer events, not main-queue blocks). This also documents a property of the production design: main-queue work (including queued socket commands) cannot re-enter the presenter during the bounded wait; only run-loop-source events (user input, notifications) can, and those paths are covered by the coalescing/ownership tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 13, 2026
austinywang
added a commit
that referenced
this pull request
Jul 14, 2026
Pin the structure the SwiftUI-owned WindowGroup scene produced for the Settings window, on top of the AppKit-owned lifecycle from #7783: - .fullSizeContentView in the styleMask (the sidebar extends under the titlebar), default titlebar styling otherwise — probe-app verified as exactly what SwiftUI sets on its own NavigationSplitView window - an AppKit-owned toolbar of [flexible space, sidebar toggle, sidebar tracking separator], the exact item layout SwiftUI builds; the NSHostingController scene bridge never materializes the implicit toggle in an AppKit-hosted window, so .toolbars bridging is pinned OFF and the toolbar is deterministic (CI-testable, no bridge wait) - the toolbar toggle posts the same notification the Toggle Left Sidebar menu command uses (one shared mutation path) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
added a commit
that referenced
this pull request
Jul 14, 2026
The AppKit-hosted Settings window (#7783) lost two structural pieces of the SwiftUI WindowGroup chrome: 1. .fullSizeContentView, without which the NavigationSplitView sidebar stops at an opaque titlebar band instead of running full height. 2. The sidebar toggle. NSHostingController's .toolbars scene bridging never materializes NavigationSplitView's implicit toggle in an AppKit-hosted window, so the factory now owns the toolbar in AppKit with SwiftUI's exact item layout: [flexible space, toggle, sidebar tracking separator]. The toggle posts the same notification the Toggle Left Sidebar menu command routes, keeping one mutation path for the sidebar state, and the tracking separator follows the split divider so the title renders at the detail column's leading edge. The window renders in the system's current design (Liquid Glass on macOS 26 SDK builds); no design opt-outs. Localization: the toolbar toggle reuses the existing fully-localized shortcut.toggleLeftSidebar.label and titlebar.sidebar.tooltip keys; no new user-facing strings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
added a commit
that referenced
this pull request
Jul 14, 2026
Pin the structure the SwiftUI-owned WindowGroup scene produced for the Settings window, on top of the AppKit-owned lifecycle from #7783: - .fullSizeContentView in the styleMask (the sidebar extends under the titlebar), default titlebar styling otherwise — probe-app verified as exactly what SwiftUI sets on its own NavigationSplitView window - an AppKit-owned toolbar of [flexible space, sidebar toggle, sidebar tracking separator], the exact item layout SwiftUI builds; the NSHostingController scene bridge never materializes the implicit toggle in an AppKit-hosted window, so .toolbars bridging is pinned OFF and the toolbar is deterministic (CI-testable, no bridge wait) - the toolbar toggle posts the same notification the Toggle Left Sidebar menu command uses (one shared mutation path) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
added a commit
that referenced
this pull request
Jul 14, 2026
The AppKit-hosted Settings window (#7783) lost two structural pieces of the SwiftUI WindowGroup chrome: 1. .fullSizeContentView, without which the NavigationSplitView sidebar stops at an opaque titlebar band instead of running full height. 2. The sidebar toggle. NSHostingController's .toolbars scene bridging never materializes NavigationSplitView's implicit toggle in an AppKit-hosted window, so the factory now owns the toolbar in AppKit with SwiftUI's exact item layout: [flexible space, toggle, sidebar tracking separator]. The toggle posts the same notification the Toggle Left Sidebar menu command routes, keeping one mutation path for the sidebar state, and the tracking separator follows the split divider so the title renders at the detail column's leading edge. The window renders in the system's current design (Liquid Glass on macOS 26 SDK builds); no design opt-outs. Localization: the toolbar toggle reuses the existing fully-localized shortcut.toggleLeftSidebar.label and titlebar.sidebar.tooltip keys; no new user-facing strings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
added a commit
that referenced
this pull request
Jul 14, 2026
…-hosted window (#8047) * test: cover Settings sidebar chrome regression * test: require SwiftUI Settings toolbar ownership * fix: restore Settings sidebar-owned chrome * fix: preserve native Settings sidebar toolbar * test: exercise native Settings sidebar toolbar * test: verify bridged Settings sidebar toggle * test: require 0.64.17 Settings chrome * fix: restore 0.64.17 Settings window chrome * test: verify Settings sidebar visibility toggle * test: keep Settings chrome coverage deterministic * test: pin the native Settings split-view chrome contract Pin the structure the SwiftUI-owned WindowGroup scene produced for the Settings window, on top of the AppKit-owned lifecycle from #7783: - .fullSizeContentView in the styleMask (the sidebar extends under the titlebar), default titlebar styling otherwise — probe-app verified as exactly what SwiftUI sets on its own NavigationSplitView window - an AppKit-owned toolbar of [flexible space, sidebar toggle, sidebar tracking separator], the exact item layout SwiftUI builds; the NSHostingController scene bridge never materializes the implicit toggle in an AppKit-hosted window, so .toolbars bridging is pinned OFF and the toolbar is deterministic (CI-testable, no bridge wait) - the toolbar toggle posts the same notification the Toggle Left Sidebar menu command uses (one shared mutation path) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: restore the Settings sidebar toggle and full-height sidebar The AppKit-hosted Settings window (#7783) lost two structural pieces of the SwiftUI WindowGroup chrome: 1. .fullSizeContentView, without which the NavigationSplitView sidebar stops at an opaque titlebar band instead of running full height. 2. The sidebar toggle. NSHostingController's .toolbars scene bridging never materializes NavigationSplitView's implicit toggle in an AppKit-hosted window, so the factory now owns the toolbar in AppKit with SwiftUI's exact item layout: [flexible space, toggle, sidebar tracking separator]. The toggle posts the same notification the Toggle Left Sidebar menu command routes, keeping one mutation path for the sidebar state, and the tracking separator follows the split divider so the title renders at the detail column's leading edge. The window renders in the system's current design (Liquid Glass on macOS 26 SDK builds); no design opt-outs. Localization: the toolbar toggle reuses the existing fully-localized shortcut.toggleLeftSidebar.label and titlebar.sidebar.tooltip keys; no new user-facing strings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
6 tasks done
hhsw2015
pushed a commit
to hhsw2015/cmux
that referenced
this pull request
Jul 16, 2026
…-hosted window (manaflow-ai#8047) * test: cover Settings sidebar chrome regression * test: require SwiftUI Settings toolbar ownership * fix: restore Settings sidebar-owned chrome * fix: preserve native Settings sidebar toolbar * test: exercise native Settings sidebar toolbar * test: verify bridged Settings sidebar toggle * test: require 0.64.17 Settings chrome * fix: restore 0.64.17 Settings window chrome * test: verify Settings sidebar visibility toggle * test: keep Settings chrome coverage deterministic * test: pin the native Settings split-view chrome contract Pin the structure the SwiftUI-owned WindowGroup scene produced for the Settings window, on top of the AppKit-owned lifecycle from manaflow-ai#7783: - .fullSizeContentView in the styleMask (the sidebar extends under the titlebar), default titlebar styling otherwise — probe-app verified as exactly what SwiftUI sets on its own NavigationSplitView window - an AppKit-owned toolbar of [flexible space, sidebar toggle, sidebar tracking separator], the exact item layout SwiftUI builds; the NSHostingController scene bridge never materializes the implicit toggle in an AppKit-hosted window, so .toolbars bridging is pinned OFF and the toolbar is deterministic (CI-testable, no bridge wait) - the toolbar toggle posts the same notification the Toggle Left Sidebar menu command uses (one shared mutation path) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: restore the Settings sidebar toggle and full-height sidebar The AppKit-hosted Settings window (manaflow-ai#7783) lost two structural pieces of the SwiftUI WindowGroup chrome: 1. .fullSizeContentView, without which the NavigationSplitView sidebar stops at an opaque titlebar band instead of running full height. 2. The sidebar toggle. NSHostingController's .toolbars scene bridging never materializes NavigationSplitView's implicit toggle in an AppKit-hosted window, so the factory now owns the toolbar in AppKit with SwiftUI's exact item layout: [flexible space, toggle, sidebar tracking separator]. The toggle posts the same notification the Toggle Left Sidebar menu command routes, keeping one mutation path for the sidebar state, and the tracking separator follows the split divider so the title renders at the detail column's leading edge. The window renders in the system's current design (Liquid Glass on macOS 26 SDK builds); no design opt-outs. Localization: the toolbar toggle reuses the existing fully-localized shortcut.toggleLeftSidebar.label and titlebar.sidebar.tooltip keys; no new user-facing strings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 1574b7b)
This branch was successfully deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #7777
Fixes #7775
Fixes #4053
Problem
Settings intermittently cannot be opened at all: cmux → Settings…, ⌘,, and CLI
settings openall silently do nothing, and only an app restart recovers. This has recurred across builds (#5770 → closed, still recurring; #4053; #7775), and the point fix in PR #5806 (deferred verification + one retry) did not hold.The #7775 capture shows the worst shape: the tag-bound socket returns
OK target=accountwhile accessibility inspection proves no Settings window exists at all — not offscreen, not hidden, never created.Root cause
Settings window creation was delegated to a SwiftUI single-
Windowscene throughopenWindow(id: "settings"). That call returns nothing, reports nothing, and its internal scene state can wedge permanently (relaunch-while-open per #7775, scene mid-teardown per the live root-cause confirmation on #5770). The presenter could only request-and-hope, so each observed failure mode grew its own patch: anisOpeningSettingsWindowflag, ashouldOpenWhenConfigureddeferral, a 500 ms verification timer, and a bounded retry. When the scene wedged, the retry retried into the same wedged scene and gave up — the exact "accepted but nothing happened" symptom.Fix: AppKit-owned window lifecycle, SwiftUI only for content
SettingsWindowPresenteris rewritten as the single source of truth for the Settings window lifecycle. It constructs theNSWindowitself (SettingsWindowFactory:NSHostingController(SettingsWindowRoot)withsceneBridgingOptions = [.toolbars, .title]), the same ownership model the main window (AppDelegate.createMainWindow) andTaskManagerWindowControlleralready use. The SwiftUI settingsWindowscene is deleted.show()is now synchronous and self-healing:isVisiblebefore returning; on failure, tear down and recreate once, then fail loudly (Loggerfault with full window/app/screen state).Deleted with the scene: the
openWindowclosures,configure(openWindow:)/configure(window:)+WindowAccessorwiring,shouldOpenWhenConfigured,isOpeningSettingsWindow, the verification timer,SettingsWindowOpenOutcome, and the retry machinery.Invariant established: every open request ends in exactly one of — a visible Settings window, a window ordered front under a hidden app (non-activating CLI opens only), or a loud diagnostic failure carried in the return value. "Returned OK but nothing happened" is unrepresentable.
CLI truth-telling (#7775)
ControlSystemContextis@MainActor, socontrolSettingsOpennow presents synchronously and replies from the actual outcome:ControlSettingsOpenResolutiongains.failed(message:), mapped to anunavailablesocket error.settings.openreturnsopened=trueif-and-only-if a window actually materialized. Theactivateflag now also gates app activation on the shared path (socket focus policy), instead ofshow()unconditionally activating.Navigation targets are also now delivered on first open: previously a fresh window's pending target was never consumed (dead
consumePendingNavigationTarget); the newSettingsWindowHostRootconsumes it once the content is live.Why this also closes #4053
#4053 is the same "menu click produces nothing" symptom on a stable build with no deterministic repro. The live root-cause confirmation on #5770 (same symptom family) proved the mechanism:
openPreferencesWindow → show() → openWindow(id:) silently no-ops with no window created. That entire creation path no longer exists; any wedge inside it is gone by construction, and if AppKit itself ever refused to present, the CLI/log now say so explicitly instead of no-oping.Regression tests (two-commit red/green)
Commit 1 adds
SettingsWindowOpenRegressionTests— end-to-end, real-factory tests that are red on the old code and unchanged by the fix commit:show()always produces a visible Settings window (fresh state)show()while the previous window is mid-close still produces a visible windowshow()recovers a window parked off every active screenCommit 2 (the fix) rewrites
SettingsWindowPresenterTestsfor the new lifecycle using an injected window factory: creation/reuse/fresh-after-close, content released on close, contentless-husk teardown + recreation, mid-close reopen, bounded recreate → loud.failedwhen no window becomes visible, min-size + clamp repair on show, immediate vs pending navigation delivery, and the pure usability policy. The multi-monitor recovery and clamp-geometry unit tests are carried over unchanged.Interactive-only modes (stated plainly, not faked): the #7775 relaunch-while-open recipe, real display disconnect/reconnect, and hidden-app CLI opens are not reproducible in unit tests. The relaunch wedge is eliminated structurally (no scene state survives to wedge); display changes are covered by the pure
targetVisibleFrame/clampedFrametests plus the offscreen-frame end-to-end test; the hidden-app path is covered by theorderedWhileAppHiddenresult mapping.Localization audit
settings.window.runtimeUnavailable(fallback shown only if the settings runtime is missing at window creation — loud instead of silent). Added toResources/Localizable.xcstringswith translations for all 19 catalog locales (ar, bs, da, de, en, es, fr, it, ja, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant).settings.titlekey.settings.titlekey; no keys were orphaned. All other new strings areos.Logger/debug diagnostics (not localized by policy).Budgets / guards
swift_file_length_budget.pypasses with no TSV changes: new filesSettingsWindowFactory.swift(81) and rewrittenSettingsWindowPresenter.swift(462) are under 500;SettingsWindowPresenterTests.swiftshrank 668 → 542;BrowserPanelView.swift,cmuxApp.swift,SettingsWindowScene.swift(dead scene struct deleted) all shrank.lint-pbxproj-test-wiring.sh,check-workspace-package-groups.py --check,check-package-resolved-policy.pyall pass.swift-warning-budget.tsvchanges.Red/green CI proof
Both commits were pushed together, so in addition to the PR checks:
33313fc): e2e run 29063423332 — all 5SettingsWindowOpenRegressionTestsFAILED with "no visible settings window", proving the tests catch the bug.SettingsWindowOpenRegressionTests) and run 29063489144 (SettingsWindowPresenterTests) passed; re-runs on the final HEAD: 29064369806, 29064371100, 29064372539 (SettingsWindowNavigationRoutingTests).Review follow-ups (pushed in
8977ae1)activate=false(Cursor medium / Greptile P1).presentPreferencesWindowconsumes the show result:.failedbeeps instead of silently activating, matching the CLI's error surfacing (Cursor medium).openedfrom the verified outcome, not the request (Cursor low).Review follow-ups round 2 (codex structured review, pushed in
ee23c79and708080a)settings.open --activate=falseno longer keys the window, raises the main window, or unhides/activates the app (socket no-focus-steal contract).SettingsHostWindowrecords close-begin, soshow()deterministically refuses to reuse a dying window regardless of notification-observer order.settings.openwith a missing control-system context fails closed (unavailable) instead of synthesizingopened=true.SidebarCommands); covered by newSettingsWindowNavigationRoutingTests.SettingsWindowGeometry.swiftto keep the presenter under the 500-line budget threshold.Review follow-ups round 3–4 (pushed in
40edeae,7788d21,43e05e5)presentPreferencesWindow/openPreferencesWindowmoved toSources/App/AppDelegateSettingsPresentation.swift—AppDelegate.swiftis over the 900-line hard cap and may not grow, which failedworkflow-guard-tests; it now shrinks 29 lines net.onAppear— anNSWindowexisting is not enough, so rapid targeted opens can no longer post into a not-yet-subscribed window and lose the pane.show()(menu click) no longer erases a still-undelivered pending navigation target (Cursor findings).openBrowserImportSettingsroutes through the sharedAppDelegate.presentPreferencesWindow, so a failed presentation beeps there too instead of silently no-oping.km) translation forsettings.window.runtimeUnavailable(the catalog shipskm; the key now covers all 20 catalog locales).@testable importpackage-symbol visibility differs across toolchains and broke the CI test compile while local Xcode accepted it.activateIgnoringOtherAppsfrom the moved default closure; the sidebar-toggle helper takeskeyWindowexplicitly (aNSApp.keyWindowdefault argument warns under strict concurrency) (ebbe77b).9526370): the retry loop re-checks for a usable window on every attempt so a re-entrantshow()from a willClose observer is adopted instead of duplicated, and a small depth bound turns a pathological reopen-on-close observer plus persistent presentation failure into a loud.failedinstead of unbounded recursion — both covered by new tests.🤖 Generated with Claude Code