Repository navigation
Fit main windows after display topology changes - #7308
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a macOS visible-frame fitting core, a screen-change rescue pass, AppDelegate integration, project/test wiring, and iOS boundary edits. ChangesWindow frame fitting
Estimated code review effort: 4 (Complex) | ~60 minutes iOS WorkspaceDetailView boundary edits
Estimated code review effort: 1 (Trivial) | ~3 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 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 |
Greptile SummaryAdds
Confidence Score: 5/5Safe to merge; the new fit logic is gated on a real topology change, leaves fully-visible windows untouched, and complements rather than conflicts with the existing stranding-rescue pass. The core is a pure, side-effect-free struct with thorough unit coverage including red/green regression pairs. The runtime wiring correctly sequences remembered-frame restore → titlebar stranding → visible-fit rescue, so each pass sees already-corrected frames. The only finding is a dead-code fallback that can never fire. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[NSNotification: didChangeScreenParameters] --> B[reconcileMainWindowFramesAfterScreenChange]
B --> C{displayConfig\nsignatureChanged?}
C -- yes --> D[restoreRememberedFrames\n→ resolvedWindowFrame\n→ fittedFrame multi-display]
C -- no --> E[skip restore]
D --> F
E --> F[Titlebar stranding rescue loop\nreconciledFrameAfterScreenChange per window]
F --> G{visibleFrameFit\ntopologyChanged?}
G -- yes --> H[MainWindowVisibleFrameFitRescue\n.performFitIfNeeded]
G -- no --> I[done]
H --> J{fittedFrame per window}
J -- nil: already fully visible --> K[no-op]
J -- rect: needs clamp/shrink --> L[NSWindow.setFrame]
K --> I
L --> I
subgraph fittedFrame logic
M[isFullyCovered by union\nof visible displays?] -- yes --> N[return nil]
M -- no --> O[targetDisplay:\nmax overlap area\nor nearest edge]
O --> P[clampFrame within\ntargetDisplay.visibleFrame]
P --> Q{approx equal\nto original?}
Q -- yes --> N
Q -- no --> R[return fitted]
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[NSNotification: didChangeScreenParameters] --> B[reconcileMainWindowFramesAfterScreenChange]
B --> C{displayConfig\nsignatureChanged?}
C -- yes --> D[restoreRememberedFrames\n→ resolvedWindowFrame\n→ fittedFrame multi-display]
C -- no --> E[skip restore]
D --> F
E --> F[Titlebar stranding rescue loop\nreconciledFrameAfterScreenChange per window]
F --> G{visibleFrameFit\ntopologyChanged?}
G -- yes --> H[MainWindowVisibleFrameFitRescue\n.performFitIfNeeded]
G -- no --> I[done]
H --> J{fittedFrame per window}
J -- nil: already fully visible --> K[no-op]
J -- rect: needs clamp/shrink --> L[NSWindow.setFrame]
K --> I
L --> I
subgraph fittedFrame logic
M[isFullyCovered by union\nof visible displays?] -- yes --> N[return nil]
M -- no --> O[targetDisplay:\nmax overlap area\nor nearest edge]
O --> P[clampFrame within\ntargetDisplay.visibleFrame]
P --> Q{approx equal\nto original?}
Q -- yes --> N
Q -- no --> R[return fitted]
end
Reviews (11): Last reviewed commit: "Fit restored windows using all current d..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func distanceSquared(_ rect: CGRect, _ point: CGPoint) -> CGFloat { | ||
| let dx = rect.midX - point.x | ||
| let dy = rect.midY - point.y | ||
| return (dx * dx) + (dy * dy) | ||
| } |
There was a problem hiding this comment.
distanceSquared measures from the display's center (rect.midX/rect.midY) to the window's center. When a window is completely off all screens — the only case this fallback fires — a window parked far off the right edge of the main display could score farther from the nearby display's center than from a second display that happens to be centered closer, producing a counterintuitive target. Using the distance from the window center to the nearest point on each visible frame would give a geometrically correct answer regardless of display layout.
| final class MainWindowVisibleFrameFitRescue: NSObject { | ||
| private let coalescedFitNotification = Notification.Name("cmux.mainWindowVisibleFrameFitRescue.perform") | ||
| private let fitCore: MainWindowVisibleFrameFitCore | ||
| private var cachedSignature: [MainWindowVisibleFrameTopologySignatureEntry] = [] | ||
| private var isInstalled = false | ||
|
|
||
| /// Owned and installed by `AppDelegate`. | ||
| init(fitCore: MainWindowVisibleFrameFitCore = MainWindowVisibleFrameFitCore()) { | ||
| self.fitCore = fitCore | ||
| super.init() | ||
| } |
There was a problem hiding this comment.
removeObserver on deallocation
NotificationCenter.default.addObserver(self, ...) is called in install() but there is no deinit { NotificationCenter.default.removeObserver(self) }. Because AppDelegate owns this object for the full app lifetime this is not a practical leak, but if MainWindowVisibleFrameFitRescue were ever released early the NotificationCenter would hold a dangling reference to a deallocated object, which crashes on the next notification delivery. Adding a deinit now keeps the invariant explicit and prevents future surprises if the ownership changes.
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}
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)
Packages/macOS/CmuxWindowing/Sources/CmuxWindowing/Geometry/MainWindowVisibleFrameFitCore.swift (1)
86-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal constant shadows the private method name.
let targetDisplay = targetDisplay(...)shadows thetargetDisplay(for:in:)method with a same-named local constant. It compiles correctly since the call is resolved before the binding completes, but it hurts readability at the call site.♻️ Suggested rename
- let targetDisplay = targetDisplay(for: standardizedFrame, in: usableDisplays) + let selectedDisplay = targetDisplay(for: standardizedFrame, in: usableDisplays) ?? usableDisplays[0] let fitted = clampFrame( standardizedFrame, - within: targetDisplay.visibleFrame, + within: selectedDisplay.visibleFrame, minimumWidth: minimumWidth, minimumHeight: minimumHeight )🤖 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 `@Packages/macOS/CmuxWindowing/Sources/CmuxWindowing/Geometry/MainWindowVisibleFrameFitCore.swift` around lines 86 - 87, Rename the local binding in MainWindowVisibleFrameFitCore’s target selection logic so it does not shadow the private targetDisplay(for:in:) method. Update the let targetDisplay = targetDisplay(...) binding to a clearer name like selectedDisplay or resolvedDisplay, and adjust the subsequent uses in the same scope to match.
🤖 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
`@Packages/macOS/CmuxWindowing/Sources/CmuxWindowing/Geometry/MainWindowVisibleFrameFitCore.swift`:
- Around line 86-87: Rename the local binding in MainWindowVisibleFrameFitCore’s
target selection logic so it does not shadow the private targetDisplay(for:in:)
method. Update the let targetDisplay = targetDisplay(...) binding to a clearer
name like selectedDisplay or resolvedDisplay, and adjust the subsequent uses in
the same scope to match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0aa655f6-1caa-4849-8500-99e5ba840516
📒 Files selected for processing (3)
Packages/macOS/CmuxWindowing/Sources/CmuxWindowing/Geometry/MainWindowVisibleFrameFitCore.swiftSources/App/MainWindowVisibleFrameFitRescue.swiftcmuxTests/MainWindowVisibleFrameFitCoreTests.swift
…tor-window-cutoff # Conflicts: # Sources/AppDelegate.swift # cmux.xcodeproj/project.pbxproj
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 53-55: Make the SwiftUI view state private by narrowing the access
of the `@State` properties in WorkspaceDetailView. Update the terminalPickerRows
and isChatMode declarations so they are private to the view, keeping the state
implementation detail encapsulated and aligned with SwiftLint expectations.
🪄 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: f8d0625d-cd37-4457-b85e-187f5718b82a
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/macOS/CmuxWindowing/Sources/CmuxWindowing/Geometry/MainWindowVisibleFrameFitCore.swift
💤 Files with no reviewable changes (1)
- Packages/macOS/CmuxWindowing/Sources/CmuxWindowing/Geometry/MainWindowVisibleFrameFitCore.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 53-55: Make the SwiftUI view state private by narrowing the access
of the `@State` properties in WorkspaceDetailView. Update the terminalPickerRows
and isChatMode declarations so they are private to the view, keeping the state
implementation detail encapsulated and aligned with SwiftLint expectations.
🪄 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: f8d0625d-cd37-4457-b85e-187f5718b82a
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/macOS/CmuxWindowing/Sources/CmuxWindowing/Geometry/MainWindowVisibleFrameFitCore.swift
💤 Files with no reviewable changes (1)
- Packages/macOS/CmuxWindowing/Sources/CmuxWindowing/Geometry/MainWindowVisibleFrameFitCore.swift
🛑 Comments failed to post (1)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift (1)
53-55: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the SwiftUI state private.
terminalPickerRowsandisChatModeare@State-backed view state; keeping them internal widens the surface unnecessarily. SwiftLint already flags this.♻️ Suggested fix
- `@State` var terminalPickerRows: [TerminalPickerMenuRow] = [] + `@State` private var terminalPickerRows: [TerminalPickerMenuRow] = [] @@ - `@State` var isChatMode = false + `@State` private var isChatMode = false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`@State` private var terminalPickerRows: [TerminalPickerMenuRow] = [] /// Chat-mode toggle for inline agent chat in place of the terminal. `@State` private var isChatMode = false🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 53-53: SwiftUI state properties should be private
(private_swiftui_state)
[Warning] 55-55: SwiftUI state properties should be private
(private_swiftui_state)
🤖 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 `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift` around lines 53 - 55, Make the SwiftUI view state private by narrowing the access of the `@State` properties in WorkspaceDetailView. Update the terminalPickerRows and isChatMode declarations so they are private to the view, keeping the state implementation detail encapsulated and aligned with SwiftLint expectations.Source: Linters/SAST tools
…tor-window-cutoff
…tor-window-cutoff
…tor-window-cutoff # Conflicts: # .github/workflows/ci.yml # tests/test_ci_change_areas.py
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 23cd0ca. Configure here.
This branch was successfully deployed
1 participant
Summary
MainWindowVisibleFrameFitCore, a pureCmuxWindowingdecision core that preserves frames already fully covered by the union of current visible displays and otherwise clamps/shrinks into the best current display.didChangeScreenParametersNotification, so live display changes can rescue reachable-titlebar-but-cut-off main windows without reacting to Dock side/bottom resizes.WorkspaceDetailView.swiftto clear an existing file-budget overage without touching unrelated code.origin/maininto the branch; the PR diff against main remains the focused window-fit change.Fixes #7306
Related
Tests
./scripts/check-pbxproj.sh../scripts/lint-pbxproj-test-wiring.sh.git diff --check origin/main...HEAD.python3 scripts/swift_file_length_budget.py.python3 scripts/check-package-resolved-policy.py.python3 scripts/check-workspace-package-groups.py --check.Localization
No user-facing strings were added by this PR diff. New strings in the PR diff are test names, debug log identifiers, and Sentry breadcrumb metadata. After merging current
origin/main, parsedweb/messages/en.jsonandweb/messages/ja.jsonsuccessfully and verified the existing platform FAQ waitlist copy is present in both locales.Note
Medium Risk
Changes window geometry on display topology changes and session restore; incorrect gating could move windows on Dock-only resizes or fail to rescue clipped frames, but logic is heavily unit-tested and gated separately from per-monitor config restore.
Overview
Adds
MainWindowVisibleFrameFitCoreinCmuxWindowingto decide when a main-window frame is already fully covered by current visible displays (no move) and otherwise clamp/shrink it onto the best display, including multi-monitor overlap and nearest-display fallback.On live display changes,
AppDelegatetracks a separate topology signature (stable display IDs, quantized frames, top menu-bar inset only—not Dock side/bottom resizes) and runsMainWindowVisibleFrameFitRescueonly when that signature changes, alongside the existing titlebar-stranding reconcile pass.Session restore now routes intersecting saved frames through the same fit core instead of preserving any frame with a reachable titlebar, while still leaving fully visible frames untouched (including windows spanning adjacent displays).
Also removes three blank lines in
WorkspaceDetailView.swiftfor file-length budget and wires tests plusCmuxWindowinginto the app/test targets.Reviewed by Cursor Bugbot for commit c2ce925. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests