Repository navigation
Fix blank Settings window on every open after the first #4965
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,66 @@ | ||||||||||||||||||||||
| /// Decides whether the Settings window renders its full content or a lightweight | ||||||||||||||||||||||
| /// placeholder, based on the lifecycle of the underlying `NSWindow`. | ||||||||||||||||||||||
| /// | ||||||||||||||||||||||
| /// SwiftUI reuses the Settings `Window` scene across close / reopen. The | ||||||||||||||||||||||
| /// `WindowAccessor` that supplies the window dedupes by the reused `NSWindow`, | ||||||||||||||||||||||
| /// so it does not fire again on the second open, and the scene's view state | ||||||||||||||||||||||
| /// persists. Without re-adoption, a window that was forgotten on close (to stop | ||||||||||||||||||||||
| /// observing it) can never flip back to rendering content — producing a blank | ||||||||||||||||||||||
| /// 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 { | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment.
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! |
||||||||||||||||||||||
| /// Whether the settings content should render. When `false`, callers show a | ||||||||||||||||||||||
| /// cheap placeholder instead of the full settings hierarchy. | ||||||||||||||||||||||
| private(set) var shouldRenderContent: Bool | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// Identity of the window currently being observed, if any. Cleared on close. | ||||||||||||||||||||||
| private var observedWindow: ObjectIdentifier? | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// Creates a coordinator that renders content until a lifecycle event hides it. | ||||||||||||||||||||||
| init() { | ||||||||||||||||||||||
| shouldRenderContent = true | ||||||||||||||||||||||
| observedWindow = nil | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// Adopts the window the scene attached to. | ||||||||||||||||||||||
| /// | ||||||||||||||||||||||
| /// - Parameters: | ||||||||||||||||||||||
| /// - window: Identity of the attached window. | ||||||||||||||||||||||
| /// - isMiniaturized: Whether that window is currently miniaturized; content | ||||||||||||||||||||||
| /// stays hidden while miniaturized to avoid wasted rendering. | ||||||||||||||||||||||
| mutating func windowConfigured(_ window: ObjectIdentifier, isMiniaturized: Bool) { | ||||||||||||||||||||||
| observedWindow = window | ||||||||||||||||||||||
| shouldRenderContent = !isMiniaturized | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+32
to
+35
Contributor
There was a problem hiding this comment.
Suggested change
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! |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// Hides content while the observed window is miniaturized. | ||||||||||||||||||||||
| mutating func windowDidMiniaturize(_ window: ObjectIdentifier) { | ||||||||||||||||||||||
| guard window == observedWindow else { return } | ||||||||||||||||||||||
| shouldRenderContent = false | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// Restores content when the window becomes visible again. | ||||||||||||||||||||||
| /// | ||||||||||||||||||||||
| /// Re-adopts the window when the previous observation was cleared on close | ||||||||||||||||||||||
| /// (the reused-scene case where `WindowAccessor` never fires a second time). | ||||||||||||||||||||||
| /// | ||||||||||||||||||||||
| /// - Parameters: | ||||||||||||||||||||||
| /// - window: Identity of the window that became visible. | ||||||||||||||||||||||
| /// - isSettingsWindow: Whether that window is the Settings window, used to | ||||||||||||||||||||||
| /// re-adopt it after a prior close cleared the reference. | ||||||||||||||||||||||
| mutating func windowDidBecomeVisible(_ window: ObjectIdentifier, isSettingsWindow: Bool) { | ||||||||||||||||||||||
| if observedWindow == nil, isSettingsWindow { | ||||||||||||||||||||||
| observedWindow = window | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| guard window == observedWindow else { return } | ||||||||||||||||||||||
| shouldRenderContent = true | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+52
to
+58
Contributor
There was a problem hiding this comment.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source) |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// Hides content and forgets the window when it closes. | ||||||||||||||||||||||
| mutating func windowWillClose(_ window: ObjectIdentifier) { | ||||||||||||||||||||||
| guard window == observedWindow else { return } | ||||||||||||||||||||||
| shouldRenderContent = false | ||||||||||||||||||||||
| observedWindow = nil | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| import Testing | ||
|
|
||
| #if canImport(cmux_DEV) | ||
| @testable import cmux_DEV | ||
| #elseif canImport(cmux) | ||
| @testable import cmux | ||
| #endif | ||
|
|
||
| /// Regression coverage for the Settings window rendering a blank placeholder on | ||
| /// every open after the first within a session. | ||
| /// | ||
| /// SwiftUI reuses the Settings `Window` scene across close / reopen, so the | ||
| /// scene's content-visibility state persists and the `WindowAccessor` that | ||
| /// supplies the window does not fire a second time. The original implementation | ||
| /// cleared the observed window on close and then rejected every subsequent | ||
| /// "became visible" notification, leaving the body stuck on the placeholder. | ||
| @MainActor | ||
| @Suite struct SettingsWindowContentVisibilityTests { | ||
| /// Distinct object identities standing in for `NSWindow` instances. | ||
| private final class WindowToken {} | ||
|
|
||
| @Test func rendersContentOnFirstConfigure() { | ||
| var visibility = SettingsWindowContentVisibility() | ||
| let window = WindowToken() | ||
|
|
||
| visibility.windowConfigured(ObjectIdentifier(window), isMiniaturized: false) | ||
|
|
||
| #expect(visibility.shouldRenderContent) | ||
| } | ||
|
Comment on lines
+22
to
+29
Contributor
There was a problem hiding this comment.
The suite has no case that calls |
||
|
|
||
| @Test func hidesContentWhileMiniaturizedAndRestoresOnDeminiaturize() { | ||
| var visibility = SettingsWindowContentVisibility() | ||
| let window = WindowToken() | ||
| let id = ObjectIdentifier(window) | ||
| visibility.windowConfigured(id, isMiniaturized: false) | ||
|
|
||
| visibility.windowDidMiniaturize(id) | ||
| #expect(!visibility.shouldRenderContent) | ||
|
|
||
| visibility.windowDidBecomeVisible(id, isSettingsWindow: true) | ||
| #expect(visibility.shouldRenderContent) | ||
| } | ||
|
|
||
| /// The core regression: close then reopen the reused window. Because the | ||
| /// reopen arrives only as a "became visible" notification (the accessor does | ||
| /// not fire again), the model must re-adopt the Settings window and restore | ||
| /// content. Before the fix this stayed `false` — a permanently blank window. | ||
| @Test func restoresContentWhenReusedWindowReopensAfterClose() { | ||
| var visibility = SettingsWindowContentVisibility() | ||
| let window = WindowToken() | ||
| let id = ObjectIdentifier(window) | ||
| visibility.windowConfigured(id, isMiniaturized: false) | ||
|
|
||
| visibility.windowWillClose(id) | ||
| #expect(!visibility.shouldRenderContent) | ||
|
|
||
| // Reopen: same reused NSWindow, only a become-visible notification. | ||
| visibility.windowDidBecomeVisible(id, isSettingsWindow: true) | ||
| #expect(visibility.shouldRenderContent) | ||
| } | ||
|
|
||
| @Test func survivesRepeatedCloseReopenCycles() { | ||
| var visibility = SettingsWindowContentVisibility() | ||
| let window = WindowToken() | ||
| let id = ObjectIdentifier(window) | ||
| visibility.windowConfigured(id, isMiniaturized: false) | ||
|
|
||
| for _ in 0..<5 { | ||
| visibility.windowWillClose(id) | ||
| #expect(!visibility.shouldRenderContent) | ||
| visibility.windowDidBecomeVisible(id, isSettingsWindow: true) | ||
| #expect(visibility.shouldRenderContent) | ||
| } | ||
| } | ||
|
|
||
| @Test func ignoresUnrelatedWindowsAfterClose() { | ||
| var visibility = SettingsWindowContentVisibility() | ||
| let settingsWindow = WindowToken() | ||
| let otherWindow = WindowToken() | ||
| visibility.windowConfigured(ObjectIdentifier(settingsWindow), isMiniaturized: false) | ||
| visibility.windowWillClose(ObjectIdentifier(settingsWindow)) | ||
|
|
||
| // A non-settings window becoming key must not flip content back on. | ||
| visibility.windowDidBecomeVisible(ObjectIdentifier(otherWindow), isSettingsWindow: false) | ||
| #expect(!visibility.shouldRenderContent) | ||
| } | ||
|
|
||
| @Test func miniaturizeNotificationForUnrelatedWindowIsIgnored() { | ||
| var visibility = SettingsWindowContentVisibility() | ||
| let settingsWindow = WindowToken() | ||
| let otherWindow = WindowToken() | ||
| visibility.windowConfigured(ObjectIdentifier(settingsWindow), isMiniaturized: false) | ||
|
|
||
| visibility.windowDidMiniaturize(ObjectIdentifier(otherWindow)) | ||
| #expect(visibility.shouldRenderContent) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SettingsWindowContentVisibilityis a pure value type whose fields areBoolandObjectIdentifier?— it holds no UI state and has no dependency on the main actor. Marking it@MainActorcouples 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 benonisolated(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.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!