Align titlebar accessory hints#5059
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR refactors titlebar shortcut-hint layout from left-edge and right-edge positioning to center-based positioning. A new metric constant is introduced, ChangesTitlebar hint layout refactoring to center-based positioning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (13 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 |
Contributor
Greptile SummaryThis PR fixes a three-way misalignment in the minimal-mode titlebar: shortcut-hint pills, visible accessory buttons, and the hidden AppKit hit layer now all share the same leading coordinate, sourced from the new
Confidence Score: 4/5Safe to merge; the layout math is consistent and all changed invariants are covered by updated unit and UI tests. The core alignment arithmetic is correct — buttonCenterX in the SwiftUI overlay and buttonXRanges in the AppKit hit layer now produce the same leading coordinate once the outer hintLeadingPadding offset is accounted for. The three cosmetic issues (dead trafficLightGap constant, stale planner comment, ignored trafficLightFrame parameter still in the function signature) are minor cleanup items that pose no runtime risk. Sources/Update/UpdateTitlebarAccessory.swift — the dead trafficLightGap constant, stale comment, and ignored parameter are all in this file. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[titlebarControlsLeadingPadding = 4] --> B[TitlebarControlsHitRegions.outerLeadingPadding]
A --> C[TitlebarControlsLayoutMetrics.hintLeadingPadding]
B --> D[AppKit hit-region buttonXRanges
minX = outerLeadingPadding + groupPadding.leading]
C --> E[SwiftUI outer .padding leading
shifts paddedContent by 4 pt]
E --> F[titlebarShortcutHintOverlay
.position x: buttonCenterX
centered = groupPadding.leading + i*step + size/2]
D --> G{Same leading coordinate?}
F --> G
G --> H[YES - pills, icons, hit targets aligned]
I[leadingOffset] --> J[ignores trafficLightFrame
AppKit already offsets accessory past traffic lights]
J --> K[hostingView.frame.x = 0
containerWidth = contentSize.width]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static let outerLeadingPadding: CGFloat = TitlebarControlsHitRegions.outerLeadingPadding | ||
| static let hintRightSafetyShift: CGFloat = 10 | ||
| static let hintTrailingBaseInset: CGFloat = 8 | ||
| static let trafficLightGap: CGFloat = 2 |
Contributor
There was a problem hiding this comment.
leadingOffset refactor
trafficLightGap is now defined but referenced nowhere in production code. The only call sites were the old leadingOffset body (which now ignores the traffic-light frame entirely) and the test that was updated to expect 0. Leaving an unreachable constant here risks future callers picking it up and re-introducing the double-offset that this PR is specifically removing.
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!
Comment on lines
555
to
562
| static func leadingOffset( | ||
| trafficLightFrame: NSRect?, | ||
| trafficLightFrame _: NSRect?, | ||
| debugSnapshot: MinimalModeTitlebarDebugSnapshot | ||
| ) -> CGFloat { | ||
| let debugOffset = MinimalModeTitlebarDebugSettings.leftControlsXOffset( | ||
| MinimalModeTitlebarDebugSettings.leftControlsXOffset( | ||
| leadingInset: debugSnapshot.leftControlsLeadingInset | ||
| ) | ||
| guard let trafficLightFrame, !trafficLightFrame.isEmpty else { | ||
| return debugOffset | ||
| } | ||
| return max(debugOffset, trafficLightFrame.maxX + trafficLightGap) | ||
| } |
Contributor
There was a problem hiding this comment.
trafficLightFrame parameter is now completely ignored — its internal label is _. Since AppKit already positions the titlebar accessory past the traffic lights, the frame is no longer needed for the x-offset calculation. Keeping the parameter in the signature means every call site still computes the frame unnecessarily and future readers may assume the argument influences the result. Removing it makes the contract explicit.
Suggested change
| static func leadingOffset( | |
| trafficLightFrame: NSRect?, | |
| trafficLightFrame _: NSRect?, | |
| debugSnapshot: MinimalModeTitlebarDebugSnapshot | |
| ) -> CGFloat { | |
| let debugOffset = MinimalModeTitlebarDebugSettings.leftControlsXOffset( | |
| MinimalModeTitlebarDebugSettings.leftControlsXOffset( | |
| leadingInset: debugSnapshot.leftControlsLeadingInset | |
| ) | |
| guard let trafficLightFrame, !trafficLightFrame.isEmpty else { | |
| return debugOffset | |
| } | |
| return max(debugOffset, trafficLightFrame.maxX + trafficLightGap) | |
| } | |
| static func leadingOffset( | |
| debugSnapshot: MinimalModeTitlebarDebugSnapshot | |
| ) -> CGFloat { | |
| MinimalModeTitlebarDebugSettings.leftControlsXOffset( | |
| leadingInset: debugSnapshot.leftControlsLeadingInset | |
| ) | |
| } |
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!
hhsw2015
pushed a commit
to hhsw2015/cmux
that referenced
this pull request
Jun 1, 2026
- Add Send Ctrl-F to Terminal passthrough (manaflow-ai#5011, force-stop CC agents) - Fix sidebar worktree spawn worktree-setup-as-input bug (manaflow-ai#5032) - Add boundary-aware ranking layer for command-palette fuzzy search - Open extension browser as pane tab + polish (manaflow-ai#5053) - Move sidebar kind selection to titlebar menu, fix clipped tooltip and floor sidebar width (manaflow-ai#5045) - Extract CmuxFoundation package — modular refactor wave 1 (manaflow-ai#5055) - Center empty sidebar-extension state, fade host bottom edge (manaflow-ai#5057) - Align titlebar accessory hints (manaflow-ai#5059) - Restore sidebar minimum width (manaflow-ai#5062) Conflicts resolved: - cmux.xcodeproj/project.pbxproj: merge fork's CMUXSettingsCore + CMUXSessionDaemon package refs with upstream's new CmuxFoundation package reference and product dependency.
austinywang
added a commit
that referenced
this pull request
Jun 1, 2026
Branch had diverged behind main, which reworked the titlebar shortcut-hint layout (#5045 clipping fix, #5059 hint alignment with defaultTitlebarHintX 4->0) and shrank header chrome sizing. Resolution preserves the branch's intent (restored release sizes 24/15/17/8, sidebar.left symbol, 3-slot sidebar chrome button-count plumbing) while adopting main's newer hint-layout machinery: - WindowChromeMetrics: keep restored sizes; add main's titlebarControlsLeadingPadding. - TitlebarControlsHitRegions: keep sidebarChromeButtonCount/allTitlebarButtonCount (referenced widely by auto-merged code); adopt named outerLeadingPadding. - contentSize: combine main's two-reservation clipping fix with the branch's buttonCount/reservesShortcutHintOverflow threading; bound titlebarHintLayoutRightmostExtent to the active slot count for sidebar chrome. - Drop branch's superseded hint positioning (titlebarButtonRightEdge, hintRightSafetyShift) in favor of main's planner (hintInterval/titlebarHintPillWidth). - Update regression anchors to merged behavior (defaultTitlebarHintX=0): classicWithShortcutHints 186->172, sidebar 118->104, roomy fitted spacing 7->14. vendor/bonsplit working tree checked out to the merged pointer (adds isAudioMuted, forkConversation* APIs that Workspace.swift now uses). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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
Align the titlebar shortcut hint pills, titlebar icons, and accessory button hit targets to the same center X.
Root cause: the shortcut hint overlay positioned pills by right-edge spacer math, while the visible accessory buttons and hidden AppKit hit layer used separate leading offsets. The titlebar accessory host also reapplied traffic-light spacing that AppKit had already handled.
Validation
./scripts/reload.sh --tag titlebarx --swift-frontend-workaroundtitlebarxvisually against the misaligned top-left titlebar accessory controls.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Aligns titlebar shortcut hint pills, accessory icons, and hidden hit regions to the same center X for crisp visuals and correct click targets. Removes duplicate traffic‑light spacing and ad‑hoc offsets for a consistent layout.
HeaderChromeControlMetrics.titlebarControlsLeadingPaddingfor both visible controls and hit regions.Written for commit 22c62a5. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores