Repository navigation
Fix blank Settings window on every open after the first - #4965
philip-zhan wants to merge 2 commits into
Conversation
The Settings window renders a blank placeholder on every open after the first within a session (open -> close -> reopen), in any appearance mode. SwiftUI reuses the Settings Window scene across close/reopen, so its content-visibility state persists and the WindowAccessor (which dedupes by window) never fires again on the second open. The content-visibility optimization from manaflow-ai#4661 forgets the observed window on close and then rejects every subsequent "became visible" notification, so content never renders again. Extract the lifecycle logic into a pure SettingsWindowContentVisibility model and rewire SettingsWindowRootView to delegate to it, so the close -> reopen path can be exercised in isolation. This commit intentionally reproduces the bug (no re-adoption on reopen): SettingsWindowContentVisibilityTests' restoresContentWhenReusedWindowReopensAfterClose and survivesRepeatedCloseReopenCycles fail here, proving the test catches the bug. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When the Settings window becomes visible again, re-adopt it by its stable identifier (cmux.settings) if a prior close cleared the observed-window reference. SwiftUI reuses the Window scene across close/reopen and the WindowAccessor does not fire a second time, so this is the only signal that the reused window is back. Restores content rendering on the second and subsequent opens within a session. All SettingsWindowContentVisibilityTests now pass (6/6). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@philip-zhan is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create an environment for this repo. |
📝 WalkthroughWalkthroughIntroduces ChangesSettings Window Content Visibility Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 17 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (17 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@codex review |
@philip-zhan I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedReview triggered.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
To use Codex here, create a Codex account and connect to github. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Greptile SummaryFixes a bug where the Settings window showed only its title bar (blank content area) on every open after the first within a session. SwiftUI reuses the
Confidence Score: 4/5The fix is correct and well-tested; the only observation is a style point on an explicit The close→reopen state machine is logically sound, idempotent across repeated notifications, and correctly guards re-adoption behind the window's stable
Important Files Changed
Sequence DiagramsequenceDiagram
participant SC as Settings Scene (SwiftUI)
participant WA as WindowAccessor
participant SWCV as SettingsWindowContentVisibility
participant SRV as SettingsWindowRootView
Note over SC,SRV: First open
SC->>WA: background callback fires
WA->>SWCV: windowConfigured(id, isMiniaturized: false)
SWCV-->>SRV: "shouldRenderContent = true"
Note over SC,SRV: Close (⌘W)
SC->>SRV: willCloseNotification
SRV->>SWCV: windowWillClose(id)
SWCV-->>SRV: "shouldRenderContent = false, observedWindow = nil"
Note over SC,SRV: Reopen — WindowAccessor does NOT re-fire (reused scene)
SC->>SRV: didBecomeKeyNotification
SRV->>SWCV: windowDidBecomeVisible(id, isSettingsWindow: true)
Note over SWCV: observedWindow == nil → re-adopt
SWCV-->>SRV: "observedWindow = id, shouldRenderContent = true"
Note over SC,SRV: Miniaturize
SC->>SRV: didMiniaturizeNotification
SRV->>SWCV: windowDidMiniaturize(id)
SWCV-->>SRV: "shouldRenderContent = false"
Note over SC,SRV: Deminiaturize
SC->>SRV: didDeminiaturizeNotification
SRV->>SWCV: windowDidBecomeVisible(id, isSettingsWindow: true)
SWCV-->>SRV: "shouldRenderContent = true"
Reviews (1): Last reviewed commit: "Fix blank Settings window on reopen by r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @MainActor | ||
| struct SettingsWindowContentVisibility: Equatable { |
There was a problem hiding this comment.
SettingsWindowContentVisibility is a pure value type whose fields are Bool and ObjectIdentifier? — it holds no UI state and has no dependency on the main actor. Marking it @MainActor couples every declaration and mutating method to the main actor unnecessarily, which is why the test suite also has to carry @MainActor. Per the actor-isolation guideline, pure value model types should be nonisolated (no annotation); callers on the main actor can still create and mutate them freely, and future non-UI callsites won't be forced onto the main thread.
| @MainActor | |
| struct SettingsWindowContentVisibility: Equatable { | |
| struct SettingsWindowContentVisibility: Equatable { |
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes the blank Settings window on re-open by extracting a pure, testable
Confidence Score: 4/5Safe to merge; the fix correctly addresses the blank-window repro with no behavioral regressions on the normal open/close/reopen path. The re-adoption logic in windowDidBecomeVisible is sound — re-adoption only fires when observedWindow is nil and the window carries the stable cmux.settings identifier, and the five-cycle stress test plus unrelated-window guard tests confirm the boundaries. The only open items are a missing dedup guard in windowConfigured and a missing test for the isMiniaturized: true configure path. Sources/SettingsWindowContentVisibility.swift — the windowConfigured dedup and the test gap for the miniaturized-on-configure path. Important Files Changed
Sequence DiagramsequenceDiagram
participant SWR as SettingsWindowRootView
participant WA as WindowAccessor
participant SWCV as SettingsWindowContentVisibility
participant NC as NotificationCenter
Note over SWR,SWCV: First open
WA->>SWR: callback(window)
SWR->>SWCV: windowConfigured(id, isMiniaturized: false)
SWCV-->>SWCV: "observedWindow = id, shouldRenderContent = true"
NC->>SWR: didBecomeKeyNotification
SWR->>SWCV: windowDidBecomeVisible(id, isSettingsWindow: true)
SWCV-->>SWCV: "observedWindow already set, shouldRenderContent = true"
Note over SWR,SWCV: User closes with ⌘W
NC->>SWR: willCloseNotification
SWR->>SWCV: windowWillClose(id)
SWCV-->>SWCV: "shouldRenderContent = false, observedWindow = nil"
Note over SWR,SWCV: Reopen (scene reused — WindowAccessor does NOT fire again)
NC->>SWR: didBecomeKeyNotification
SWR->>SWCV: windowDidBecomeVisible(id, isSettingsWindow: true)
SWCV-->>SWCV: "observedWindow == nil → re-adopt, observedWindow = id"
SWCV-->>SWCV: "shouldRenderContent = true ✅"
Reviews (2): Last reviewed commit: "Fix blank Settings window on reopen by r..." | Re-trigger Greptile |
| mutating func windowConfigured(_ window: ObjectIdentifier, isMiniaturized: Bool) { | ||
| observedWindow = window | ||
| shouldRenderContent = !isMiniaturized | ||
| } |
There was a problem hiding this comment.
setContentVisibility had a guard shouldRenderSettingsContent != isVisible else { return } dedup to prevent no-op @State mutations (which still trigger a SwiftUI re-render even when the value doesn't change). windowConfigured unconditionally writes both observedWindow and shouldRenderContent on every call. WindowAccessor typically fires once, but on a window-moved-between-screens event or any future path that re-triggers the accessor, this will cause an avoidable render. Adding an early exit when the values are already correct restores the prior optimization.
| mutating func windowConfigured(_ window: ObjectIdentifier, isMiniaturized: Bool) { | |
| observedWindow = window | |
| shouldRenderContent = !isMiniaturized | |
| } | |
| mutating func windowConfigured(_ window: ObjectIdentifier, isMiniaturized: Bool) { | |
| let newShouldRender = !isMiniaturized | |
| guard observedWindow != window || shouldRenderContent != newShouldRender else { return } | |
| observedWindow = window | |
| shouldRenderContent = newShouldRender | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| @Test func rendersContentOnFirstConfigure() { | ||
| var visibility = SettingsWindowContentVisibility() | ||
| let window = WindowToken() | ||
|
|
||
| visibility.windowConfigured(ObjectIdentifier(window), isMiniaturized: false) | ||
|
|
||
| #expect(visibility.shouldRenderContent) | ||
| } |
There was a problem hiding this comment.
windowConfigured(isMiniaturized: true)
The suite has no case that calls windowConfigured(id, isMiniaturized: true) and verifies shouldRenderContent == false. If a future refactor accidentally flips the negation (e.g. shouldRenderContent = isMiniaturized), none of the six existing tests would catch it — they all configure with isMiniaturized: false.
Greptile SummaryFixes the Settings window going blank on every open after the first by extracting window-lifecycle state into a new
Confidence Score: 4/5Safe to merge; the fix correctly restores Settings content on every reopen and is backed by targeted regression tests. The re-adoption logic in Sources/SettingsWindowContentVisibility.swift — re-adoption coupling and actor annotation; everything else is straightforward. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([SettingsWindowContentVisibility.init
shouldRenderContent=true
observedWindow=nil]) --> B
B[WindowAccessor fires
first open only] -->|windowConfigured| C
C[observedWindow = id
shouldRenderContent = !isMiniaturized]
C -->|NSWindow.willCloseNotification| D
D[observedWindow = nil
shouldRenderContent = false]
D -->|NSWindow.didBecomeKey or
didBecomeMain notification| E{observedWindow == nil
AND isSettingsWindow?}
E -->|YES – re-adopt| F[observedWindow = window
shouldRenderContent = true]
E -->|NO – unrelated window| G([no-op, early return])
C -->|NSWindow.didMiniaturizeNotification| H[shouldRenderContent = false]
H -->|NSWindow.didDeminiaturizeNotification
or didBecomeKey/Main| F
F -->|Next close| D
Reviews (3): Last reviewed commit: "Fix blank Settings window on reopen by r..." | Re-trigger Greptile |
| /// window on every open after the first. This value type centralizes that | ||
| /// state machine so the close → reopen path can be exercised in isolation. | ||
| @MainActor | ||
| struct SettingsWindowContentVisibility: Equatable { |
There was a problem hiding this comment.
@MainActor on a pure value type
SettingsWindowContentVisibility holds only value-typed fields (Bool, ObjectIdentifier?) with no AppKit objects and no shared mutable reference semantics. The @MainActor annotation couples the type to the main actor unnecessarily — it cannot be called from non-@MainActor contexts (e.g., a future actor-isolated store, or a plain XCTest target without @MainActor). The cmux actor-isolation rule's preferred shape for pure value types is nonisolated struct; the @MainActor should live on the call sites (already the case — SettingsWindowRootView is @MainActor), not on the model itself.
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| mutating func windowDidBecomeVisible(_ window: ObjectIdentifier, isSettingsWindow: Bool) { | ||
| if observedWindow == nil, isSettingsWindow { | ||
| observedWindow = window | ||
| } | ||
| guard window == observedWindow else { return } | ||
| shouldRenderContent = true | ||
| } |
There was a problem hiding this comment.
SettingsWindowPresenter
windowDidBecomeVisible re-adopts the Settings window by checking isSettingsWindow, which the caller resolves via window.identifier?.rawValue == SettingsWindowPresenter.windowIdentifier. This check only returns true because SettingsWindowPresenter.configure(window:) has already stamped the identifier on the reused NSWindow during the first open. Nothing in SettingsWindowContentVisibility's interface encodes that precondition, so a future refactor that delays or skips configure(window:) would silently break re-adoption: isSettingsWindow would be false, observedWindow would stay nil, and the window would go blank again — exactly the bug this PR fixed — without any compile-time or runtime signal.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
teamleaderleo
added
area: settings
This branch has not been deployed
2 participants
Summary
SettingsWindowRootView(Sources/cmuxApp.swift) chooses between the real settings content and aColor.clearplaceholder via a@Stateflag plus a weak observed-window reference (the CPU optimization from Fix hidden Settings CPU during Codex output #4661). OnwillCloseit both hid content and clearedwindowReference.window. SwiftUI reuses the SettingsWindowscene across close/reopen, so the@Stateplaceholder flag persists and theWindowAccessor(which dedupes by the reusedNSWindow) never fires again. On reopen,didBecomeKey/didBecomeMainfire but are gated byisObservedWindow(...), which returnsfalsebecause the reference was cleared — so content visibility never flips back on. Result: a permanently blank Settings window until app restart.Fixes #4964.
Approach
SettingsWindowContentVisibilitymodel (Sources/SettingsWindowContentVisibility.swift).cmux.settings) if a prior close cleared the reference — the only signal available, since the accessor doesn't re-fire for the reused scene.SettingsWindowRootViewto delegate to the model.Testing
reload.sh): open Settings → ⌘W → reopen → blank window (titlebar only, empty content area).cmuxTests/SettingsWindowContentVisibilityTests.swift(6 Swift Testing cases) covering the exact close→reopen repro, miniaturize/deminiaturize, repeated cycles, and unrelated-window guards. Wired intoproject.pbxproj; passesscripts/lint-pbxproj-test-wiring.shandscripts/check-pbxproj.sh.xcodebuild -scheme cmux-unit -only-testing:cmuxTests/SettingsWindowContentVisibilityTests test→ TEST SUCCEEDED (6/6).restoresContentWhenReusedWindowReopensAfterCloseandsurvivesRepeatedCloseReopenCyclesfail); the second commit adds the fix and turns them green — so the Commits tab proves the test catches the bug.Demo Video
Before
CleanShot.2026-05-28.at.17.00.31.mp4
After
CleanShot.2026-05-28.at.17.09.01.mp4
Review Trigger (Copy/Paste as PR comment)
Checklist
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes #4964 by re-adopting the reused Settings window so reopening always shows the full Settings UI. Centralizes window lifecycle logic to prevent the blank window after the first open.
SettingsWindowContentVisibilityto control content rendering based on window lifecycle.cmux.settingson become-visible when the scene is reused.SettingsWindowRootViewwith the model; simplified event handling.Written for commit 9dd49a7. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
Bug Fixes
Tests