Repository navigation
Fix minimal-mode sidebar titlebar icon alignment - #4481
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCompute an optical Y offset and window-aware controls frame for minimal-mode titlebar controls; apply that as top padding in VerticalTabsSidebar and use it in WindowDecorationsController click-target placement; add a regression test asserting alignment with the traffic-light button. ChangesMinimal Mode Sidebar Icon Vertical Alignment
Sequence DiagramsequenceDiagram
participant ContentView
participant VerticalTabsSidebar
participant WindowDragHandleView
participant WindowDecorationsController
participant NSWindow
ContentView->>VerticalTabsSidebar: pass observedWindow
VerticalTabsSidebar->>WindowDragHandleView: request controls top inset/frame
WindowDragHandleView->>NSWindow: minimalModeTrafficLightFrameInContentCoordinates(for:)
NSWindow-->>WindowDragHandleView: traffic-light frame or nil
WindowDragHandleView-->>VerticalTabsSidebar: return controls frame / top inset
WindowDecorationsController->>WindowDragHandleView: compute click-target frame via minimalModeSidebarTitlebarControlsFrame(...)
WindowDecorationsController-->>NSWindow: set target.frame
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (15 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 SummaryFixes minimal-mode sidebar titlebar control misalignment (#4475) by replacing a fixed
Confidence Score: 5/5Safe to merge; the geometry math is correct for both flipped and non-flipped coordinate systems, the legacy fallback is preserved, and a regression test validates the key invariant. The centralized frame calculation correctly handles flipped vs. non-flipped AppKit coordinate systems, the optical adjustment applies symmetrically in both orientations, the nil-close-button fallback retains previous behavior, and the SwiftUI padding derives from the same helper as the AppKit click target so the two cannot drift apart. No files require special attention; all four changed files are focused and internally consistent. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["applyMinimalModeSidebarTitlebarClickTarget(to:)"] --> B["minimalModeTrafficLightFrameInContentCoordinates(window:contentView:)"]
B --> C{closeButton found?}
C -- Yes --> D["closeButtonSuperview.convert(frame, to: contentView)"]
C -- No --> E["nil → fallback path"]
D --> F["minimalModeSidebarTitlebarControlsFrame(contentBounds:contentViewIsFlipped:trafficLightFrameInContent:visualDownwardAdjustment:)"]
E --> F
F --> G["target.frame = NSRect centered on traffic-light midY ± opticalYOffset"]
H["VerticalTabsSidebar.minimalModeSidebarTitlebarControlsTopPadding"] --> I["minimalModeSidebarTitlebarControlsTopInset(in: observedWindow)"]
I --> J["minimalModeSidebarTitlebarControlsFrame(in: window)"]
J --> F
I --> K[".padding(.top, inset) on TitlebarControlsView"]
style F fill:#d4edda,stroke:#28a745
style G fill:#cce5ff,stroke:#004085
style K fill:#cce5ff,stroke:#004085
Reviews (5): Last reviewed commit: "fix: share traffic light frame helper" | Re-trigger Greptile |
|
Addressed the remaining Greptile note in |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes 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 9d89c4b. Configure here.
|
Follow-up pushed in |
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/WindowDecorationsController.swift`:
- Around line 415-431: Extract the duplicated traffic-light frame conversion
into a shared helper and call it from both WindowDecorationsController and
WindowDragHandleView: replace the inline closure assigned to
trafficLightFrameInContent in WindowDecorationsController with a call to a new
internal function trafficLightFrameInContentCoordinates(window:contentView:) (or
make minimalModeTrafficLightFrameInContentCoordinates internal) that
encapsulates the guard-for-closeButton + superview.convert(closeButton.frame,
to: contentView) logic, then pass its result into
minimalModeSidebarTitlebarControlsFrame; this removes the duplicate logic that
currently mirrors minimalModeTrafficLightFrameInContentCoordinates and keeps
behavior unchanged while avoiding `@MainActor` issues.
🪄 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: 0469b6e8-d87a-489f-b807-41eeb1c76aef
📒 Files selected for processing (2)
Sources/WindowDecorationsController.swiftSources/WindowDragHandleView.swift
|
@coderabbitai review The requested shared traffic-light frame helper has been pushed in |
|
✅ Actions performedReview triggered.
|

Closes #4475.
What changed
leftControlsTopInset.TitlebarControlsViewicon padding.Regression source
Likely introduced by the minimal-mode toolbar removal path:
d13674f93 Hide window toolbar in minimal mode to eliminate titlebar gapf17415702 Remove toolbar entirely in minimal mode instead of just hidingThose commits moved minimal mode onto a separate overlay path while the standard titlebar accessory still used native titlebar metrics.
Sibling #4469 / #4471 coordination
I checked
origin/issue-4469-fullscreen-titlebar-alignbefore editing. That work has already landed onorigin/mainas296060b95 Align titlebar controls with traffic lights (#4471). This PR layers on top of that and does not re-fix #4469; it fixes the remaining minimal-mode-only overlay path.Screenshots
Sanitized titlebar crops captured from the tagged dev build:
/tmp/cmux-issue-4475-screens/minimal-hover-before-titlebar.png/tmp/cmux-issue-4475-screens/minimal-hover-after-titlebar.pngFull local captures are also available at:
/tmp/cmux-issue-4475-screens/minimal-hover-before.png/tmp/cmux-issue-4475-screens/minimal-hover-after.pngI attempted to upload the cropped PNGs for inline PR rendering, but the available
gh gistpath rejects binary files and the unauthenticated upload fallbacks were unavailable. The after image was visually verified with the user before opening this PR.Test approach
Failing-test-first structure:
c78167d17 test: cover minimal titlebar control alignment278cbb695 fix: align minimal titlebar controls to traffic lightsThe regression test is in
cmuxTests/WindowAndDragTests.swiftand exercises the realWindowDecorationsControllerminimal click-target installation against AppKit traffic-light geometry.Verification
CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag issue-4475-minimal-mode-sidebar-icon-offset --launch.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes minimal-mode titlebar control geometry and click-target placement based on live AppKit traffic-light frames, which could affect window interactions across macOS versions and scale factors.
Overview
Fixes minimal-mode sidebar titlebar icon/click-target alignment by deriving the controls host frame from the window’s live traffic-light (close button) geometry instead of a fixed top inset, with a one-backing-pixel optical Y compensation.
Centralizes the frame/top-inset calculation in shared helpers and reuses it in both the AppKit
WindowDecorationsControllerclick target and the SwiftUIVerticalTabsSidebarpadding (now passedobservedWindow). Adds a regression XCTest that creates a realNSWindow, installs the minimal-mode click target, and asserts its center aligns to the traffic-light center plus the optical offset.Reviewed by Cursor Bugbot for commit 79adecf. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes #4475 by aligning minimal-mode sidebar titlebar controls to the macOS close button using live geometry. Click targets and padding now match at all scales, and window decoration updates run on the main actor.
minimalModeSidebarTitlebarControlsFrameand reuse it for both the AppKit click target and SwiftUI padding;ContentViewnow passesobservedWindowsoVerticalTabsSidebarcomputes top padding viaminimalModeSidebarTitlebarControlsTopInset(in:).minimalModeTrafficLightFrameInContentCoordinateshelper to convert the close-button frame into content coordinates for both AppKit and tests.WindowDecorationsControllerand helpers@MainActor; add a self-contained regression test that asserts the host center tracks the traffic-light center plus the optical offset, without introducing new warnings.Written for commit 79adecf. Summary will update on new commits. Review in cubic
Summary by CodeRabbit