Fix window collapsing to sliver after disconnecting external displays (#2666) - #2667
Closed
austinywang wants to merge 6 commits into
Closed
austinywang wants to merge 6 commits into
austinywang wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
|
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:
📝 WalkthroughWalkthroughIntroduces a versioned, per-display persisted window-geometry payload with fingerprinted selection, serialized read-merge-write via a persistence queue to avoid lost updates, LRU-capped per-fingerprint storage, live screen-change reconciliation, centralized validated frame application, and accompanying tests plus a small Workspace API overload. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant App as Application
participant Observer as ScreenObserver
participant Queue as PersistQueue
participant Storage as Storage
User->>Observer: Display parameters change
Observer->>App: didChangeScreenParametersNotification
App->>App: Select preferred primary window
App->>App: Compute resolved & validated frame
App->>App: Apply validated frame to window
App->>App: Compute displayConfigurationFingerprint
App->>Queue: Enqueue PendingGeometryWrite (fingerprint + geometry)
Queue->>Storage: Read existing persisted geometry
Queue->>Queue: Merge displayConfigurations, evict LRU if > max
Queue->>Storage: Encode & write updated persisted geometry
Storage-->>Queue: Write complete
Queue-->>App: Persistence complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Contributor
Greptile SummaryThis PR fixes window-collapsing-to-sliver after external display disconnect by adding per-display-configuration geometry keying (fingerprinted by display ID + frame dimensions), a
Confidence Score: 4/5Safe to merge; core fix is correct and well-tested, with two P2 edge cases that don't affect the primary bug-fix path The sliver-detection and per-display-config fingerprinting logic is sound and covered by behavioral regression tests. Both P2 findings are latent rather than regressions: unbounded dict growth is a long-horizon concern, and the miniaturized-frame overwrite requires a specific sequence (miniaturize → disconnect display → reconnect → restart) to trigger. Sources/AppDelegate.swift — encodedPersistedWindowGeometryData (dict growth, lines ~3819–3830) and the tail of handleScreenParametersDidChange (unconditional persist of possibly-stale frame, lines ~3911–3913) Important Files Changed
Sequence DiagramsequenceDiagram
participant NS as NSApp
participant AD as AppDelegate
participant UD as UserDefaults
participant W as NSWindow
Note over AD: Startup restore
AD->>UD: persistedWindowGeometry()
AD->>AD: displayConfigurationFingerprint(current displays)
AD->>AD: persistedWindowGeometryEntry(matchingOnly:false)
AD->>AD: resolvedWindowFrame(frame, display, availableDisplays)
AD->>W: applyValidatedMainWindowFrame(_:to:display:)
Note over NS,AD: Display connect / disconnect
NS-->>AD: didChangeScreenParametersNotification
AD->>AD: handleScreenParametersDidChange()
AD->>UD: persistedWindowGeometry()
AD->>AD: displayConfigurationFingerprint(new displays)
AD->>AD: persistedWindowGeometryEntry(matchingOnly:true)
loop each mainWindowContext
AD->>W: shouldReconcileMainWindowFrameOnScreenParameterChange?
alt primary window + matching persisted geometry
AD->>AD: resolvedWindowFrame(persistedFrame)
else other / non-reconcilable window
AD->>AD: resolvedWindowFrame(liveFrame, display:nil)
end
AD->>W: applyValidatedMainWindowFrame(_:to:display:)
end
AD->>UD: persistWindowGeometry(from: primaryWindow)
Reviews (1): Last reviewed commit: "Fix window restore after display collaps..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (2)
Sources/Workspace.swift (1)
6793-6848: Collapse the config-based overload into the raw-value overload.Now that the tab-title font path is gone, both
applyGhosttyChromeoverloads do the same background-hex / no-op / logging update flow. Routing theGhosttyConfigvariant through the raw-color variant would remove an easy drift point.♻️ Suggested simplification
func applyGhosttyChrome(from config: GhosttyConfig, reason: String = "unspecified") { - let nextHex = Self.bonsplitChromeHex( - backgroundColor: config.backgroundColor, - backgroundOpacity: config.backgroundOpacity - ) - let currentAppearance = bonsplitController.configuration.appearance - let currentBackgroundHex = currentAppearance.chromeColors.backgroundHex - let backgroundChanged = currentBackgroundHex != nextHex - let isNoOp = !backgroundChanged - - if GhosttyApp.shared.backgroundLogEnabled { - GhosttyApp.shared.logBackground( - "theme apply workspace=\(id.uuidString) reason=\(reason) " + - "currentBg=\(currentBackgroundHex ?? "nil") nextBg=\(nextHex) " + - "noop=\(isNoOp)" - ) - } - - guard !isNoOp else { return } - - if backgroundChanged { - bonsplitController.configuration.appearance.chromeColors.backgroundHex = nextHex - } - - if GhosttyApp.shared.backgroundLogEnabled { - GhosttyApp.shared.logBackground( - "theme applied workspace=\(id.uuidString) reason=\(reason) " + - "resultingBg=\(bonsplitController.configuration.appearance.chromeColors.backgroundHex ?? "nil")" - ) - } + applyGhosttyChrome( + backgroundColor: config.backgroundColor, + backgroundOpacity: config.backgroundOpacity, + reason: reason + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 6793 - 6848, The config-based overload applyGhosttyChrome(from config: GhosttyConfig, reason: String) duplicates the raw-value overload; replace its body with a single call to the raw overload so there is one canonical implementation. Concretely, in applyGhosttyChrome(from:reason:) call applyGhosttyChrome(backgroundColor: config.backgroundColor, backgroundOpacity: config.backgroundOpacity, reason: reason) and remove the duplicate backgroundHex/no-op/logging logic from the config overload so all behavior (including GhosttyApp.shared.backgroundLogEnabled logging and the early return on no-op) is performed only by applyGhosttyChrome(backgroundColor:backgroundOpacity:reason:).cmuxTests/SessionPersistenceTests.swift (1)
794-836: Consider asserting fallback geometry on fingerprint miss (non-matchingOnly path).You already verify the strict
matchingOnly: truemiss case; adding the default miss-path assertion would lock in fallback behavior and prevent regressions.Proposed test addition
XCTAssertNil( AppDelegate.persistedWindowGeometryEntry( from: payload, displayConfigurationFingerprint: "missing-fingerprint", matchingOnly: true ) ) + let fallbackResolved = AppDelegate.persistedWindowGeometryEntry( + from: payload, + displayConfigurationFingerprint: "missing-fingerprint" + ) + XCTAssertEqual(fallbackResolved?.frame.x, fallbackGeometry.x, accuracy: 0.001) + XCTAssertEqual(fallbackResolved?.frame.y, fallbackGeometry.y, accuracy: 0.001) + XCTAssertEqual(fallbackResolved?.frame.width, fallbackGeometry.width, accuracy: 0.001) + XCTAssertEqual(fallbackResolved?.frame.height, fallbackGeometry.height, accuracy: 0.001)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/SessionPersistenceTests.swift` around lines 794 - 836, Add an assertion that when persistedWindowGeometryEntry is called with a missing fingerprint but without matchingOnly (the default path), it returns the payload.frame fallback geometry; call AppDelegate.persistedWindowGeometryEntry(from: payload, displayConfigurationFingerprint: "missing-fingerprint") and assert the returned entry's frame.x, frame.y, frame.width, and frame.height equal the fallbackGeometry's corresponding values (with same accuracy checks already used), so fallback behavior is locked in alongside the existing matchingOnly:true nil check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 3797-3809: The persisted display-configuration fingerprint map is
read on the main actor (persistedWindowGeometry(...).displayConfigurations) but
written later on sessionPersistenceQueue, causing a lost-update race between
saveSessionSnapshot(...) and persistWindowGeometry(...); fix by moving the
read/merge/write into the same sessionPersistenceQueue write path: in
encodedPersistedWindowGeometryData / or in the code that currently calls
defaults.set(...), fetch the current persistedWindowGeometry from UserDefaults
on sessionPersistenceQueue, merge the new per-display entry into its
displayConfigurations map (preserving other entries), then write the merged data
back to defaults; alternatively, ensure all geometry writes (saveSessionSnapshot
and persistWindowGeometry) are dispatched to and serialized on
sessionPersistenceQueue so reads and merges happen at write time and cannot be
overwritten.
- Around line 3891-3896: The call to Self.resolvedWindowFrame(from:
SessionRectSnapshot(window.frame), display: nil, availableDisplays:
displays.available, fallbackDisplay: displays.fallback) uses the `display: nil`
path which collapses visibility to a single bounding box and can force spanning
windows into one monitor; update the logic so
`resolvedWindowFrame(from:display:availableDisplays:fallbackDisplay:)` (or the
caller) evaluates visibility per display: compute each display's intersection
with the saved frame (or the true union geometry), determine if the frame is
"sufficiently usable" on multiple displays (e.g. width/height above a threshold
on more than one display) and preserve the spanning frame in that case,
otherwise clamp to the single display's visibleFrame (or pass the specific
display instead of nil); apply the same per-display visibility fix for the other
call sites (the ranges noted: ~4256-4270 and ~4387-4411).
---
Nitpick comments:
In `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 794-836: Add an assertion that when persistedWindowGeometryEntry
is called with a missing fingerprint but without matchingOnly (the default
path), it returns the payload.frame fallback geometry; call
AppDelegate.persistedWindowGeometryEntry(from: payload,
displayConfigurationFingerprint: "missing-fingerprint") and assert the returned
entry's frame.x, frame.y, frame.width, and frame.height equal the
fallbackGeometry's corresponding values (with same accuracy checks already
used), so fallback behavior is locked in alongside the existing
matchingOnly:true nil check.
In `@Sources/Workspace.swift`:
- Around line 6793-6848: The config-based overload applyGhosttyChrome(from
config: GhosttyConfig, reason: String) duplicates the raw-value overload;
replace its body with a single call to the raw overload so there is one
canonical implementation. Concretely, in applyGhosttyChrome(from:reason:) call
applyGhosttyChrome(backgroundColor: config.backgroundColor, backgroundOpacity:
config.backgroundOpacity, reason: reason) and remove the duplicate
backgroundHex/no-op/logging logic from the config overload so all behavior
(including GhosttyApp.shared.backgroundLogEnabled logging and the early return
on no-op) is performed only by
applyGhosttyChrome(backgroundColor:backgroundOpacity:reason:).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15d582fd-f17b-4e7e-b287-597acf636966
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
Lost-update race: persistWindowGeometry and saveSessionSnapshot both wrote the displayConfigurations map with their own copies, racing on UserDefaults. Both writers now go through sessionPersistenceQueue and do read-merge-encode-write inside the queue (writePersistedWindowGeometry). Per-display visibility: hasSufficientVisibleFrame previously unioned intersections into a bbox, which falsely passed two disjoint slivers across two displays. It now checks each display individually so a window is only "reachable" if at least one display has a usable chunk. LRU cap: bound the per-display geometry map at 8 entries so users cycling through many docks/displays don't accumulate UserDefaults indefinitely. The just-written fingerprint is always preserved. Gate post-reconcile persist: handleScreenParametersDidChange now only persists the primary window's frame after the new display set if it actually reconciled the primary (skipping miniaturized/fullscreen windows), so a stale off-screen frame can't get baked into the new fingerprint. Restore tab title font size plumbing: the prior commit removed the surfaceTabBarFontSize wiring in Workspace.swift, silently reverting PR #2645. The bonsplit API still exposes tabTitleFontSize, so the plumbing is restored. Add regression tests for the per-display visibility check and the LRU eviction so the contracts are pinned. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
4336-4350:⚠️ Potential issue | 🟠 MajorPreserve already-usable spanning frames here.
When
displaySnapshotisniland the live frame is already sufficiently visible on the current display set, this branch still picksbestIntersectingDisplayand clamps into that singlevisibleFrame. DuringNSApplication.didChangeScreenParametersNotification, that still collapses legitimate multi-monitor windows to one screen even though no fallback is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 4336 - 4350, When displaySnapshot is nil and hasSufficientVisibleFrame(...) already returns true for the current frame, avoid forcing a single-display clamp by returning the original frame unchanged instead of calling bestIntersectingDisplay(...) and clampFrame(...); update the branch that currently calls bestIntersectingDisplay and clampFrame so it first checks displaySnapshot == nil (or the appropriate sentinel) and if so returns frame directly, otherwise proceed to pick intersecting display and clamp as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 4336-4350: When displaySnapshot is nil and
hasSufficientVisibleFrame(...) already returns true for the current frame, avoid
forcing a single-display clamp by returning the original frame unchanged instead
of calling bestIntersectingDisplay(...) and clampFrame(...); update the branch
that currently calls bestIntersectingDisplay and clampFrame so it first checks
displaySnapshot == nil (or the appropriate sentinel) and if so returns frame
directly, otherwise proceed to pick intersecting display and clamp as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db854aa8-44ca-4364-8a8f-0d70ee4c1edc
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/SessionPersistenceTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/SessionPersistenceTests.swift
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/AppDelegate.swift (2)
5143-5146:⚠️ Potential issue | 🟠 MajorThe quit-time “sync” save still bypasses the geometry-write serializer.
When
synchronouslyistrue,writeBlock()runs on the caller thread whilepersistWindowGeometry(...)continues to enqueue read/merge/write work onsessionPersistenceQueue. That reopens the same race during quit: a queued writer can merge against stale defaults and overwrite the geometry saved here. The synchronous path still needs to execute throughsessionPersistenceQueue.🛠️ Suggested change
- if synchronously { - writeBlock() - } else { - sessionPersistenceQueue.async(execute: writeBlock) - } + if synchronously { + sessionPersistenceQueue.sync(execute: writeBlock) + } else { + sessionPersistenceQueue.async(execute: writeBlock) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 5143 - 5146, persistWindowGeometry's synchronous branch currently calls writeBlock() directly, which bypasses the sessionPersistenceQueue and reintroduces a race; change the synchronous path to run the writeBlock on sessionPersistenceQueue synchronously (e.g., use sessionPersistenceQueue.sync { ... } or an equivalent barrier) so both synchronous and asynchronous flows serialize via sessionPersistenceQueue, ensuring writeBlock and any read/merge/write work are ordered; update references in AppDelegate to call sessionPersistenceQueue.sync with writeBlock instead of invoking writeBlock() directly.
4046-4052:⚠️ Potential issue | 🟠 MajorDon't collapse preserved spanning frames back to one display.
resolvedWindowFrame(...)now intentionally preserves already-usable multi-display frames, but this branch always picks a singleNSScreenand constrains the rect to it beforesetFrame. A legitimate window spanning two monitors will still get squeezed onto one screen during restore/reconciliation. The single-screen clamp needs to be skipped for frames that were preserved because they are already sufficiently usable across the current display set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 4046 - 4052, The branch that calls Self.screenForConstrainingFrame(...) then constrains targetFrame with window.constrainFrameRect(...) always forces the rect onto a single NSScreen and therefore collapses legitimately multi-display frames; change it to skip the single-screen clamp when resolvedWindowFrame(...) indicated the frame was preserved as a usable multi-display rect (i.e., only run Self.screenForConstrainingFrame and the subsequent constrainFrameRect + min-size adjustments when the frame was NOT the preserved multi-display frame). Locate the code paths that produce or tag preserved multi-display frames (the resolvedWindowFrame logic) and use that predicate/flag when deciding whether to call Self.screenForConstrainingFrame, window.constrainFrameRect, and the min-size re-clamp so restored spanning windows are left across displays.
🧹 Nitpick comments (2)
Sources/AppDelegate.swift (1)
3870-3893: This capped history isn't actually recency-based.Updating an existing fingerprint doesn't refresh any recency information, and eviction just removes the first non-current entry. A recently reused display config can still disappear while an older one survives. If this history is meant to keep the most recent dock/undock layouts, track recency explicitly and evict the true least-recently-used entry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 3870 - 3893, The mergedDisplayConfigurations function currently treats the stored configurations as an unordered set and evicts an arbitrary non-current key; instead make the storage recency-aware by adding and updating a last-used timestamp or explicit recency list whenever a fingerprint is created or reused (e.g. attach a lastAccess/lastUsed property to PersistedWindowGeometry.StoredGeometry or maintain a separate [String:Date] or ordered keys array), update that timestamp/recency entry inside nonisolated static func mergedDisplayConfigurations when fingerprint is added or found, and change the eviction logic that uses maxStoredDisplayConfigurations to remove the entry with the oldest lastAccess (the true LRU) rather than merged.keys.first to ensure recently reused display configs are retained.cmuxTests/SessionPersistenceTests.swift (1)
808-810: Fix typo and cap wording mismatch in test docs/identifier.For clarity: Line 810 uses
stradlingFrame(typo), and the comment at Line 853 says “one more than the cap” while the setup loop at Line 858 creates exactlycapentries before merge.✏️ Proposed cleanup
- // Pre-populate the map with one more than the cap, all under + // Pre-populate the map with exactly the cap entries, all under // distinct fingerprints. The newly-written fingerprint must survive; // the map size must be capped to maxStoredDisplayConfigurations. let cap = AppDelegate.maxStoredDisplayConfigurations var existing: [String: AppDelegate.PersistedWindowGeometry.StoredGeometry] = [:] for index in 0..<cap { @@ - let stradlingFrame = CGRect(x: 950, y: 200, width: 1_100, height: 600) + let straddlingFrame = CGRect(x: 950, y: 200, width: 1_100, height: 600) @@ - stradlingFrame, + straddlingFrame,Also applies to: 853-859
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/SessionPersistenceTests.swift` around lines 808 - 810, Rename the misspelled variable `stradlingFrame` to `straddlingFrame` everywhere it’s used (e.g., in the test that constructs the frame and any assertions referencing it) and update the nearby test comment that currently reads “one more than the cap” to accurately describe the code path (the setup loop creates exactly `cap` entries before the merge) — either change the comment to say “equal to the cap” or, if the intent was to test cap+1 behavior, change the setup loop to create `cap + 1` entries; keep all identifier references consistent (use `straddlingFrame` and `cap`) so the test compiles and the docs match the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 5143-5146: persistWindowGeometry's synchronous branch currently
calls writeBlock() directly, which bypasses the sessionPersistenceQueue and
reintroduces a race; change the synchronous path to run the writeBlock on
sessionPersistenceQueue synchronously (e.g., use sessionPersistenceQueue.sync {
... } or an equivalent barrier) so both synchronous and asynchronous flows
serialize via sessionPersistenceQueue, ensuring writeBlock and any
read/merge/write work are ordered; update references in AppDelegate to call
sessionPersistenceQueue.sync with writeBlock instead of invoking writeBlock()
directly.
- Around line 4046-4052: The branch that calls
Self.screenForConstrainingFrame(...) then constrains targetFrame with
window.constrainFrameRect(...) always forces the rect onto a single NSScreen and
therefore collapses legitimately multi-display frames; change it to skip the
single-screen clamp when resolvedWindowFrame(...) indicated the frame was
preserved as a usable multi-display rect (i.e., only run
Self.screenForConstrainingFrame and the subsequent constrainFrameRect + min-size
adjustments when the frame was NOT the preserved multi-display frame). Locate
the code paths that produce or tag preserved multi-display frames (the
resolvedWindowFrame logic) and use that predicate/flag when deciding whether to
call Self.screenForConstrainingFrame, window.constrainFrameRect, and the
min-size re-clamp so restored spanning windows are left across displays.
---
Nitpick comments:
In `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 808-810: Rename the misspelled variable `stradlingFrame` to
`straddlingFrame` everywhere it’s used (e.g., in the test that constructs the
frame and any assertions referencing it) and update the nearby test comment that
currently reads “one more than the cap” to accurately describe the code path
(the setup loop creates exactly `cap` entries before the merge) — either change
the comment to say “equal to the cap” or, if the intent was to test cap+1
behavior, change the setup loop to create `cap + 1` entries; keep all identifier
references consistent (use `straddlingFrame` and `cap`) so the test compiles and
the docs match the logic.
In `@Sources/AppDelegate.swift`:
- Around line 3870-3893: The mergedDisplayConfigurations function currently
treats the stored configurations as an unordered set and evicts an arbitrary
non-current key; instead make the storage recency-aware by adding and updating a
last-used timestamp or explicit recency list whenever a fingerprint is created
or reused (e.g. attach a lastAccess/lastUsed property to
PersistedWindowGeometry.StoredGeometry or maintain a separate [String:Date] or
ordered keys array), update that timestamp/recency entry inside nonisolated
static func mergedDisplayConfigurations when fingerprint is added or found, and
change the eviction logic that uses maxStoredDisplayConfigurations to remove the
entry with the oldest lastAccess (the true LRU) rather than merged.keys.first to
ensure recently reused display configs are retained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9730c808-3c61-4533-9405-f84fdf2291a8
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/SessionPersistenceTests.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is kicking off a free cloud agent to fix this issue. This run is complimentary, but you can enable autofix for all future PRs in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1a1045a. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Sources/Workspace.swift (1)
6828-6830: Optional: extract the font delta epsilon into a named constant.This would make the threshold easier to tune and keep comparisons consistent.
♻️ Proposed refactor
private func applyGhosttyChrome( backgroundColor: NSColor, backgroundOpacity: Double, tabTitleFontSize: CGFloat?, reason: String ) { + let fontDeltaEpsilon: CGFloat = 0.0001 let nextHex = Self.bonsplitChromeHex( backgroundColor: backgroundColor, backgroundOpacity: backgroundOpacity ) let currentAppearance = bonsplitController.configuration.appearance let currentBackgroundHex = currentAppearance.chromeColors.backgroundHex let currentTabTitleFontSize = currentAppearance.tabTitleFontSize let backgroundChanged = currentBackgroundHex != nextHex let fontSizeChanged = tabTitleFontSize.map { - abs(currentTabTitleFontSize - $0) > 0.0001 + abs(currentTabTitleFontSize - $0) > fontDeltaEpsilon } ?? falseAlso applies to: 6850-6851
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 6828 - 6830, Extract the magic threshold used when comparing tab title font sizes into a named constant (e.g., fontSizeEpsilon) and use it in the comparisons that set fontSizeChanged (the expression using tabTitleFontSize and currentTabTitleFontSize) and the other similar comparison around the second occurrence; replace the literal 0.0001 with the constant to centralize tuning and ensure both comparisons use the same epsilon.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 924-925: The test currently uses
XCTAssertLessThanOrEqual(resolved.count, cap) which allows over-eviction to
pass; change the assertion to assert exact size by replacing that line with
XCTAssertEqual(resolved.count, cap) so the test fails if more than cap entries
were evicted (keep the subsequent XCTAssertNil(resolved["existing-0"]) check
as-is to verify the intended eviction target).
In `@Sources/AppDelegate.swift`:
- Around line 3967-3969: The loop iterates mainWindowContexts.values directly
which can crash if resolvedWindow(for:) mutates mainWindowContexts; fix by
snapshotting the collection before iterating (e.g., let contexts =
Array(mainWindowContexts.values)) and then iterate over that snapshot, using
resolvedWindow(for:) and
shouldReconcileMainWindowFrameOnScreenParameterChange(_:) against the snapshot
to avoid concurrent mutation during the loop.
---
Nitpick comments:
In `@Sources/Workspace.swift`:
- Around line 6828-6830: Extract the magic threshold used when comparing tab
title font sizes into a named constant (e.g., fontSizeEpsilon) and use it in the
comparisons that set fontSizeChanged (the expression using tabTitleFontSize and
currentTabTitleFontSize) and the other similar comparison around the second
occurrence; replace the literal 0.0001 with the constant to centralize tuning
and ensure both comparisons use the same epsilon.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4074ef83-9bde-497c-9f72-253d4b6308d0
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
lawrencecchen
added
the
stale-revisit
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
3 participants
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.
Summary
contentMinSize/minSizeand run restored frames back throughconstrainFrameRect(_:to:)before applying themCode paths changed
Sources/AppDelegate.swiftNSApplication.didChangeScreenParametersNotificationobserver to validate or restore live window frames after monitor changesresolvedWindowFrame(...)so bad intersections default to a sane centered frame instead of reopening as a slivercmuxTests/SessionPersistenceTests.swiftSources/Workspace.swifttabTitleFontSizebonsplit calls so this branch builds against the current checked-outvendor/bonsplitAPIValidation
./scripts/reload.sh --tag issue-2666-window-collapse-clamshellNote
Medium Risk
Changes core window restore/persistence logic and adds live reconciliation on display-configuration changes; mistakes could still place windows off-screen or persist incorrect geometry across monitor setups.
Overview
Fixes main-window restore and live monitor-change behavior to avoid windows reopening as thin off-screen slivers after docking/undocking.
Window geometry persistence is extended to store an LRU-capped set of per-display configuration entries keyed by a fingerprint, with all read/merge/write operations serialized on
sessionPersistenceQueueto avoid lost updates. Restore logic now validates visibility per display, preserves legitimately spanning windows, and falls back to a centered default size when frames are undersized or only minimally visible, while enforcing minimum window sizes and re-constraining frames before applying.Adds an
NSApplication.didChangeScreenParametersNotificationobserver to reconcile window frames when displays change (preferring usable live frames, otherwise restoring the matching fingerprinted geometry), updatesWorkspace.applyGhosttyChrometo avoid unintended font-size changes, and adds extensive regression tests for these scenarios.Reviewed by Cursor Bugbot for commit 8feca44. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes the main window collapsing to a sliver after disconnecting or rearranging displays by validating visibility per display, preserving legitimate spanning windows, preferring the live frame only when it’s usable, and centering at a sane size when needed. Geometry saves/restores are keyed per-display configuration and serialized to avoid lost updates.
sessionPersistenceQueuewith read-merge-encode-write; unify snapshot and ad‑hoc writers; cap at 8 entries and keep the newest fingerprint.constrainFrameRect.bonsplitand add a background-onlyapplyGhosttyChromeoverload; add tests for sliver/undersize fallbacks, per‑display visibility and spanning, LRU eviction/recency, live-vs-persisted selection, and fingerprinted-geometry restores.Written for commit 8feca44. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests