Repository navigation
Fix main window not refitting after a display reconfiguration (Screen Sharing / resolution change) - #5193
Conversation
cmux main windows set `isMovable = false` for the custom titlebar drag handling (WindowDragHandleView / configureCmuxMainWindowDragBehavior). A non-movable NSWindow is excluded from AppKit's automatic on-screen constraining when the display configuration changes, so after a resolution change, a display connect/disconnect, or a Screen Sharing virtual display being resized, the system leaves the window where it was. A window sized for the previous geometry then overflows the new screen and stays pinned to an edge, while every ordinary (movable) app refits. Observe NSApplication.didChangeScreenParametersNotification and reconcile each registered main window onto the visible frame of the display it now belongs to, reusing the existing geometry helpers (currentDisplayGeometries, display(for:in:), clampFrame). The pass is a no-op when a window already fits, so windows on unaffected displays are untouched; notifications are coalesced so one reconfiguration triggers one pass. Adds Swift Testing coverage for the pure reconciliation decision.
|
@domsilvestre is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAppDelegate now observes macOS screen-geometry changes and reconciles main-window frames to respect display bounds. When screen parameters change, a coalesced deferred pass iterates all tracked windows, selecting the appropriate target display and computing clamped or remapped frames while preserving frames that already fit. Comprehensive tests validate oversized/displaced windows, fitting windows, and nil-display edge cases. ChangesScreen Parameter Reconciliation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 passed)
✨ 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 |
Greptile SummaryFixes the long-standing bug where cmux main windows (which set
Confidence Score: 5/5Safe to merge — the reconcile pass is idempotent and runs only when NSScreen geometry actually changes; windows that already fit are untouched. The core reconcile logic is a well-guarded, idempotent pass over mainWindowContexts: it only calls setFrame when the clamped result differs from the current frame. The pure geometry helpers are covered by four targeted unit tests. The coalescing token prevents double execution within a notification burst. The only observation is a style concern about adding pure geometry helpers to an already very large AppDelegate rather than a bug or behavioral regression. Sources/AppDelegate.swift — the growing collection of nonisolated static geometry helpers now exceeds 3600 lines in the file and would benefit from extraction to a dedicated geometry module. Important Files Changed
Sequence DiagramsequenceDiagram
participant macOS
participant AppDelegate
participant NSWindow
macOS->>AppDelegate: didChangeScreenParametersNotification (burst)
Note over AppDelegate: screenParametersReconcileToken &+= 1
macOS->>AppDelegate: didChangeScreenParametersNotification (burst)
Note over AppDelegate: token incremented again
Note over AppDelegate: 0.1s elapses, only latest token survives
AppDelegate->>AppDelegate: reconcileMainWindowFramesAfterScreenChange()
loop each mainWindowContext
AppDelegate->>NSWindow: window.frame, window.screen?.cmuxDisplayID
AppDelegate->>AppDelegate: reconciledFrameAfterScreenChange()
alt "frame changed by >0.5pts"
AppDelegate->>NSWindow: setFrame(reconciledFrame, display: true)
else already fits
Note over AppDelegate: no-op
end
end
Reviews (2): Last reviewed commit: "Address review: snapshot window contexts..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { [weak self] in | ||
| guard let self, self.screenParametersReconcileToken == token else { return } | ||
| self.reconcileMainWindowFramesAfterScreenChange() | ||
| } | ||
| } |
There was a problem hiding this comment.
DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) is a wall-clock wait used to let NSScreen geometry settle after didChangeScreenParametersNotification fires — this is the pattern the cmux-swift-blocking-runtime rule flags as a failure even when the delay is small. The allowed exception in that rule is specifically for init-time / passive-callback deferrals used to avoid AppKit constraint-pass crashes with a documented reason; settling geometry is a different case.
The 0.1 s is also untested: on a heavily loaded Mac or an unusual display configuration the geometry might not yet be final, and the only signal the code can wait for is another notification that never comes. If a second reconfiguration races in slightly past the 100 ms window, the token comparison guards against double-execution, but the geometry read on whichever pass fires first may still be from an intermediate state.
The cmux-swift-blocking-runtime rule asks for "the real event that should replace it." Here the burst-coalescing token already handles multiple rapid firings correctly; the remaining issue is that NSApplication.didChangeScreenParametersNotification can precede the final NSScreen update. A runloop-based observation via CGDisplayRegisterReconfigurationCallback (which fires after reconfiguration is complete) would be the cleaner signal. At minimum the delay constant should be extracted and documented as a // FIXME: with the tracking issue, and an explicit comment noting that no synchronous AppKit finalization signal exists should be added per the rule's documentation requirement.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/AppDelegate.swift`:
- Around line 3280-3281: The loop iterates mainWindowContexts.values while
calling resolvedWindow(for:), but resolvedWindow(for:) can mutate
mainWindowContexts causing mutation-during-enumeration crashes; fix by taking a
stable snapshot before iterating (e.g., capture Array(mainWindowContexts.values)
into a local variable) and iterate that snapshot when calling
resolvedWindow(for:) so mutations to mainWindowContexts won't affect the
enumeration; update the loop that references mainWindowContexts.values and
ensure resolvedWindow(for:) is still called with each snapped context.
🪄 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: 126d1d49-eac9-49ad-9f56-37fe1ecc0512
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/SessionPersistenceTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ebounce resolvedWindow(for:) can reindex mainWindowContexts, so iterating mainWindowContexts.values directly risks a mutate-during-enumeration crash. Snapshot into an Array first, matching the existing pattern used elsewhere in the file. Also document the coalescing asyncAfter as a debounce rather than a correctness dependency, and add docstrings to the reconcile pass and the frame-equality helper.
|
Thanks for the reviews. Addressed in e7a8920:
Verified locally: app builds and the 4 Swift Testing cases for |
CodeRabbit now posts non-blocking comment reviews (request_changes_workflow=false, #5538).
|
Thanks for this! Windows refit after display reconfiguration landed on main in #6913. You opened this first, so you got there first. Closing since main covers it now. |
2 participants
Problem
On a Mac accessed via macOS Screen Sharing (for example a headless Mac mini reached from a laptop), connecting or resizing the viewer resizes the host's virtual display. macOS refits ordinary windows to the new geometry, but the cmux window stays where it was and ends up pinned against a screen edge or overflowing the display. The same happens on a plain resolution change or when an external display is connected or disconnected.
Root cause
cmux main windows set
isMovable = falsefor the custom titlebar drag handling (WindowDragHandleView,configureCmuxMainWindowDragBehavior). A non-movableNSWindowis excluded from AppKit's automatic on-screen constraining on display reconfiguration, so nothing repositions or resizes the window whenNSApplication.didChangeScreenParametersNotificationfires. The geometry-resolution helpers already exist (resolvedWindowFrame,clampFrame,display(for:in:)) but only run at launch and session restore, never reactively while the app is open.Fix
NSApplication.didChangeScreenParametersNotificationinAppDelegate.currentDisplayGeometries,display(for:in:),clampFrame).NSScreengeometry is current when read back.This is intentionally scoped to the missing reactive refit: it clamps the window to fit, it does not restore previously saved per-display geometry (that broader behavior is what #2667 is working toward).
Tests
Adds a Swift Testing suite for the pure decision (
AppDelegate.reconciledFrameAfterScreenChange): an oversized window on a shrunk display is refit, an already-fitting window is untouched, an off-screen window is pulled back on-screen, and an empty display list returns nil.Verification
Related: #2666, #2824, #2667 (same area: window geometry on display configuration change).
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes main window not refitting after display changes (Screen Sharing, resolution changes, display connect/disconnect). Windows now clamp to the visible screen so they stay fully on-screen.
NSApplication.didChangeScreenParametersNotificationand run one coalesced reconcile pass after changes.currentDisplayGeometries,display(for:in:), andclampFrame(no-op if it already fits).AppDelegate.reconciledFrameAfterScreenChangewith unit tests for oversized, already-fitting, off-screen, and no-display scenarios.mainWindowContextsbefore iterating to avoid mutate-during-enumeration crashes, and skip redundantsetFramecalls when the frame is unchanged.Written for commit e7a8920. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests