Repository navigation
Fix iOS workspace toolbar leading glass chrome - #7110
azooz2003-bit wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughIntroduces ChangesLeading Toolbar Chrome Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 SummaryThis PR fixes the iOS workspace toolbar by moving the back button and title into a single leading
Confidence Score: 5/5Safe to merge — the change is a targeted structural fix to a well-scoped iOS toolbar layout problem. The new WorkspaceLeadingToolbarChrome struct correctly groups back button and title in one leading ToolbarItem, the updated MobileNavTitleWidth math is pure and fully covered by unit tests, and all three call sites are consistently migrated. The package targets iOS 18+ only, so platform-guard inconsistencies in WorkspaceDetailView are moot. The dead _ = app parameter and the earlier mobileGlassCompactToolbarIsland platform-guard issue are both covered by prior review threads and not introduced or worsened here. No files require special attention. The ios/cmuxUITests/cmuxUITests.swift change is purely additive test hardening; the horizontalGapRange: 4...18 tolerance is generous but reasonable for the 8pt HStack spacing. Important Files Changed
Reviews (2): Last reviewed commit: "fix: keep iOS workspace toolbar leading" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// Compact standalone glass-looking island for controls that must not be | ||
| /// coalesced by iOS toolbar glass grouping. | ||
| @ViewBuilder | ||
| func mobileGlassCompactToolbarIsland(minHeight: CGFloat = 44) -> some View { | ||
| self | ||
| .frame(minHeight: minHeight) | ||
| .padding(.horizontal, 14) | ||
| .padding(.vertical, 4) | ||
| .background(.ultraThinMaterial, in: Capsule()) | ||
| .overlay(Capsule().stroke(.white.opacity(0.12), lineWidth: 1)) | ||
| } |
There was a problem hiding this comment.
mobileGlassCompactToolbarIsland applies material on iOS 18 — breaks the existing platform-guard contract
Every sibling modifier in this file wraps iOS 26-specific visual behavior in #if os(iOS) + if #available(iOS 26.0, *) and explicitly returns self on iOS 18 (see mobileGlassCompactToolbarControl, mobileGlassCompactNavigationTitle). This new modifier applies .background(.ultraThinMaterial, in: Capsule()) and the stroke overlay unconditionally on all iOS versions. On iOS 18, both the back button (which previously got no backing via the now-removed .mobileGlassCompactToolbarControl() call) and the title pill (which previously used mobileGlassCompactNavigationTitle() — a no-op on iOS 18) will now display an explicit capsule-shaped material layer on top of the nav bar's own translucent material, producing double-layered translucency that no other toolbar control on that surface exhibits.
| /// Compact standalone glass-looking island for controls that must not be | |
| /// coalesced by iOS toolbar glass grouping. | |
| @ViewBuilder | |
| func mobileGlassCompactToolbarIsland(minHeight: CGFloat = 44) -> some View { | |
| self | |
| .frame(minHeight: minHeight) | |
| .padding(.horizontal, 14) | |
| .padding(.vertical, 4) | |
| .background(.ultraThinMaterial, in: Capsule()) | |
| .overlay(Capsule().stroke(.white.opacity(0.12), lineWidth: 1)) | |
| } | |
| /// Compact standalone glass-looking island for controls that must not be | |
| /// coalesced by iOS toolbar glass grouping. | |
| @ViewBuilder | |
| func mobileGlassCompactToolbarIsland(minHeight: CGFloat = 44) -> some View { | |
| #if os(iOS) | |
| if #available(iOS 26.0, *) { | |
| self | |
| .frame(minHeight: minHeight) | |
| .padding(.horizontal, 14) | |
| .padding(.vertical, 4) | |
| .background(.ultraThinMaterial, in: Capsule()) | |
| .overlay(Capsule().stroke(.white.opacity(0.12), lineWidth: 1)) | |
| } else { | |
| self | |
| } | |
| #else | |
| self | |
| #endif | |
| } |
| let deadline = Date().addingTimeInterval(timeout) | ||
| let window = app.windows.firstMatch | ||
| let windowFrame = window.exists ? window.frame : app.frame | ||
| let centerTolerance = max(windowFrame.width * 0.10, 28) | ||
| _ = app | ||
| let horizontalGapRange: ClosedRange<CGFloat> = 18...42 |
There was a problem hiding this comment.
_ = app left after removing window-frame lookup
app was previously used to resolve window.frame for the center-tolerance check, but that whole block was removed. The parameter is no longer referenced in the function body, so _ = app is dead code suppressing an "unused parameter" warning. Either the app parameter should be dropped from the signature or _ = app should be replaced with a comment explaining why the parameter is intentionally kept for API stability.
| let deadline = Date().addingTimeInterval(timeout) | |
| let window = app.windows.firstMatch | |
| let windowFrame = window.exists ? window.frame : app.frame | |
| let centerTolerance = max(windowFrame.width * 0.10, 28) | |
| _ = app | |
| let horizontalGapRange: ClosedRange<CGFloat> = 18...42 | |
| let deadline = Date().addingTimeInterval(timeout) | |
| let horizontalGapRange: ClosedRange<CGFloat> = 18...42 |
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.
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/WorkspaceTitleMenu.swift`:
- Around line 21-26: The title width budget does not account for the horizontal
inset added by mobileGlassCompactToolbarIsland() in WorkspaceTitleMenu, so the
rendered pill can exceed the space reserved by MobileNavTitleWidth.cap. Fix this
by either reducing the cap by the island padding amount or moving the padding
inside the width-clamped fittedLabel so the final rendered width stays within
the intended budget.
🪄 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: 173e3b25-304e-40a1-a929-15ef8a6e11fb
📒 Files selected for processing (11)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileNavTitleWidth.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceBackButton.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceChatPane.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+DerivedState.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceLeadingToolbarChrome.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceTitleMenu.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileNavTitleWidthTests.swiftPackages/iOS/CmuxMobileSupport/Sources/CmuxMobileSupport/View+MobileGlass.swiftios/cmuxUITests/cmuxUITests.swift
| .buttonStyle(.plain) | ||
| .mobileGlassCompactToolbarIsland() | ||
| .accessibilityIdentifier("MobileWorkspaceTitleMenu") | ||
| } else { | ||
| fittedLabel | ||
| .mobileGlassCompactNavigationTitle() | ||
| .mobileGlassCompactToolbarIsland() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reserve the island padding in the title-width budget.
fittedLabel is clamped to MobileNavTitleWidth.cap, then mobileGlassCompactToolbarIsland() adds 14pt of horizontal padding on each side. That makes the rendered title pill 28pt wider than the slot the new width model reserves, so narrow phones can still crowd the trailing controls after this refactor. Either subtract that inset in MobileNavTitleWidth.cap or move the padding inside the width-clamped label.
🤖 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/WorkspaceTitleMenu.swift`
around lines 21 - 26, The title width budget does not account for the horizontal
inset added by mobileGlassCompactToolbarIsland() in WorkspaceTitleMenu, so the
rendered pill can exceed the space reserved by MobileNavTitleWidth.cap. Fix this
by either reducing the cap by the island padding amount or moving the padding
inside the width-clamped fittedLabel so the final rendered width stays within
the intended budget.
680e899 to
5f5ceca
Compare
|
Superseded by #7116 so the iOS toolbar fixes can be tested and reviewed in one PR. |
This branch was successfully deployed
1 participant
Summary
Verification
Notes
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes the iOS workspace toolbar so the back button and title stay locked together on the left and remain stable during transitions. Switches title width to leading-based caps with a safety gap to prevent overlap with trailing controls.
Bug Fixes
Refactors
WorkspaceLeadingToolbarChromeand adopted it in workspace detail, chat pane, and the demo screen to own the leading cluster (back + title).MobileNavTitleWidthfor leading rules with explicitleadingMargin,backButtonReserve,interControlSpacing, andtrailingSafetyGapconstants; updated unit tests accordingly.Written for commit 5f5ceca. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes