Repository navigation
iOS: title/subtitle lower layout priority than trailing toolbar controls - #7297
Closed
azooz2003-bit wants to merge 12 commits into
Closed
azooz2003-bit wants to merge 12 commits into
azooz2003-bit wants to merge 12 commits into
Conversation
…controls The workspace title+subtitle lived in a .topBarLeading ToolbarItem and the trailing chat-toggle/terminal-picker cluster in a separate .topBarTrailing ToolbarItem. Across independent toolbar placement groups SwiftUI's .layoutPriority cannot act, so a prior PR reserved trailing space with hand-tuned magic numbers (MobileLeadingToolbarTitleWidth: 44/64/60/84/140) and capped the title with .frame(maxWidth:), which also pinned the title to 140pt even when the bar had room. Put the back button, title/subtitle, a Spacer, and the trailing cluster in one HStack inside a single .principal toolbar item, framed to the already-measured bar width. The title/subtitle carry .layoutPriority(0) and the back/trailing islands .layoutPriority(1), so the title truncates (tail) first and the trailing controls never compress or collapse into the system overflow menu. Each island keeps its own Liquid Glass backing (mobileGlassCircle / compact glass control / glassEffect capsule) inside a GlassEffectContainer, preserving the three-island look. Delete MobileLeadingToolbarTitleWidth and its tests; no per-control constants remain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughThis PR removes width-based toolbar title sizing, moves the iOS workspace toolbar into a separate island-based implementation, and updates UI tests to verify the toolbar stays pinned to the bar edges. ChangesiOS workspace toolbar refactor
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WorkspaceDetailView
participant WorkspaceTitleMenu
participant ChatSessionHeaderView
participant WorkspaceToolbarTitleView
WorkspaceDetailView->>WorkspaceTitleMenu: build title/menu island
WorkspaceTitleMenu->>ChatSessionHeaderView: render chat session header when available
WorkspaceTitleMenu->>WorkspaceToolbarTitleView: render workspace title fallback
Possibly related PRs
🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 SummaryReplaces the magic-number
Confidence Score: 5/5Safe to merge — changes are confined to iOS workspace chrome with no auth, data, networking, or persistence touch points. The refactoring cleanly replaces hand-tuned width constants with principled SwiftUI layout priorities. The glass styling is correctly gated on iOS 26+ availability, the measured-width feedback loop is stable, and the new behavior is covered by both existing overflow-menu regression tests and new island-pinning UI tests. No files require special attention. The toolbar extension and WorkspaceDetailView cross-file state access is minimal and clearly documented. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WorkspaceDetailView.body] --> B[.onGeometryChange measures content width]
A --> C[.toolbar workspaceDetailToolbar]
C --> D[ToolbarItem .principal]
D --> E{iOS 26+?}
E -- Yes --> F[GlassEffectContainer]
E -- No --> G[workspaceToolbarLayout HStack]
F --> G
G --> H[back island layoutPriority 1]
G --> I[title menu layoutPriority 0]
G --> J[Spacer]
G --> K[trailing island layoutPriority 1]
%%{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[WorkspaceDetailView.body] --> B[.onGeometryChange measures content width]
A --> C[.toolbar workspaceDetailToolbar]
C --> D[ToolbarItem .principal]
D --> E{iOS 26+?}
E -- Yes --> F[GlassEffectContainer]
E -- No --> G[workspaceToolbarLayout HStack]
F --> G
G --> H[back island layoutPriority 1]
G --> I[title menu layoutPriority 0]
G --> J[Spacer]
G --> K[trailing island layoutPriority 1]
Reviews (9): Last reviewed commit: "iOS: type erase conditional back toolbar..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| contentWidth > 0 ? contentWidth : nil | ||
| } | ||
|
|
||
| private var workspaceToolbarIslandSpacing: CGFloat { 10 } |
Contributor
There was a problem hiding this comment.
workspaceToolbarIslandSpacing is a computed property but always returns the same value. A static let constant removes the per-access re-computation and makes the intent clearer.
Suggested change
| private var workspaceToolbarIslandSpacing: CGFloat { 10 } | |
| private static let workspaceToolbarIslandSpacing: CGFloat = 10 |
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!
…idth Framing the single toolbar container to the measured full-screen contentWidth overflowed the nav bar's centered .principal region, clipping the back-button circle at the left edge and pushing the trailing capsule flush to the right. Frame to maxWidth: .infinity so the bar fills the width the navigation bar grants the principal item (already inset by its leading/trailing layout margins); the three islands keep their native edge insets and the title still truncates in the leftover space via its lower layout priority. Drops the last geometry measurement (contentWidth) entirely. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the top-bar toolbar (workspaceDetailToolbar and its glass islands) out of WorkspaceDetailView.swift into WorkspaceDetailView+Toolbar.swift, matching the existing +AgentChat / +DerivedState / +MenuState split. Brings the host file back under its Swift file-length budget (641 lines) and keeps the god view shrinking. activeBrowser and workspaceBackToolbarButton become non-private so the extension can read them; behavior is unchanged (verified: identical render at the tightest long-title + chat-toggle state). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…yout-priority # Conflicts: # .github/swift-file-length-budget.tsv
New testWorkspaceDetailToolbarPinsIslandsToBarEdgesWithShortTitle plus a shared assertToolbarIslandsPinnedToBarEdges helper, also wired into both long-title toolbar tests: the back island's leading inset and the trailing island's trailing inset must each stay under 40pt from the nav bar edges (broken short-title state floats at ~80pt). Raises the compact-height tolerance to 3 for the delta=3.0 (title 39pt vs controls 36pt) that the current toolbar design already exhibits before this branch's fix. Test-only commit: the short-title pinning test fails against the unfixed toolbar; the fix lands in the next commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The navigation bar measures a principal item at its ideal width, caps the result to the granted inset region, and centers it. With a short title the island HStack's ideal width was small, so the whole three-island cluster hugged and floated ~80pt off the bar edges; a long title only looked right because its huge ideal width hit the cap. Measure the detail content width with onGeometryChange and report it as the toolbar layout's idealWidth while keeping maxWidth .infinity, so the bar always grants the full inset region and the compressible frame fills it exactly: back island pins at the leading inset and the trailing island at the trailing inset in every title state. A hard frame(width:) cannot work here: a fixed frame is incompressible, so UIKit centers the oversized view and the islands spill past the bar margins (the pre-be334ca8e7 state). Budget TSV regenerated by scripts/swift_file_length_budget.py for the WorkspaceDetailView.swift growth. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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/WorkspaceDetailView.swift`:
- Around line 38-41: `toolbarContentWidth` in `WorkspaceDetailView` is being
mutated only within the same view, while `WorkspaceDetailView+Toolbar.swift`
only needs read access via `workspaceToolbarIdealWidth`; change the `@State`
declaration to use `private(set)` so external files in the module can read it
but cannot mutate it. Keep the geometry update path in `WorkspaceDetailView` as
the sole writer and leave the toolbar extension as a read-only consumer.
🪄 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: d6bc0d45-707c-4bef-b25f-f64f4b234c7a
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+Toolbar.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftios/cmuxUITests/cmuxUITests.swift
Collaborator
Author
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
…yout-priority # Conflicts: # .github/swift-file-length-budget.tsv
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 eeae7f4. Configure here.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
3 participants
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.
What
Replace the magic-number title-width cap on the iOS mobile workspace toolbar with a principled layout-priority fix.
Previously the title+subtitle (
.topBarLeading) and the trailing chat-toggle/terminal-picker cluster (.topBarTrailing) were separate SwiftUI toolbar placement groups, so.layoutPrioritycould not act between them. A prior PR reserved trailing space with hand-tuned constants (MobileLeadingToolbarTitleWidth: 44/64/60/84/140) and.frame(maxWidth:), which also capped the title at 140pt even when the bar had room.Now the back button, title/subtitle, a
Spacer, and the trailing cluster live in oneHStackinside a single.principaltoolbar item, framed to the already-measured bar width. Title/subtitle get.layoutPriority(0); back and trailing islands get.layoutPriority(1). The title truncates (tail) first and the trailing controls never compress or collapse into the system overflow menu. Each island keeps its own Liquid Glass backing (mobileGlassCircle/ compact glass control /glassEffectcapsule) inside aGlassEffectContainer, preserving the three-island look.MobileLeadingToolbarTitleWidthand its tests are deleted; no per-control constants remain.Behavior verified (iPhone 16 Pro, iOS 26.5 simulator, mock data)
Existing regression tests
testWorkspaceDetailToolbarKeepsTerminalPickerVisibleWithLongTitleand...WithLongTitleAndChatToggle(which assertassertToolbarOverflowButtonDoesNotExist) cover this behavior.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes the iOS workspace toolbar so the title/subtitle yield to trailing controls and the back/trailing islands pin to the nav bar edges. The principal toolbar fills the inset region to keep chat/terminal controls visible while the title truncates; the back island expands for unread badges.
Bug Fixes
CMUX_UITEST_WORKSPACE_DETAIL_UNREAD_BACK_COUNTseeds unread fixtures; raised compact-height tolerance to 3.Refactors
MobileLeadingToolbarTitleWidthand its tests; dropped title-width props fromWorkspaceTitleMenuandAgentChatDemoScreen.WorkspaceDetailView+Toolbar.swiftand preserved the three-island glass look viaGlassEffectContainer,.mobileGlassCompactToolbarControl, and.glassEffecton iOS 26; updated the file-length budget.toolbarContentWidthprivate(set)so onlyWorkspaceDetailViewmutates the measured width.Written for commit d28c310. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Note
Low Risk
iOS-only navigation chrome and layout refactor with expanded UI test coverage; no auth, data, or networking changes.
Overview
Reworks the iOS workspace detail nav bar so back, title, and trailing controls share one
.principalHStackinstead of split leading/trailing toolbar placements.The title/subtitle get lower
layoutPrioritythan the back and trailing islands (chat toggle + terminal picker), so long titles truncate while controls stay visible and avoid the system overflow menu. Detail width is fed in as the principal item’sidealWidth(toolbarContentWidth) so the bar can use the full inset region and pin islands to the edges.Removes
MobileLeadingToolbarTitleWidthand its unit tests, plus width props fromWorkspaceTitleMenuand geometry tracking inAgentChatDemoScreen. Toolbar UI moves toWorkspaceDetailView+Toolbar.swiftwith per-island glass styling; the back button uses a badge-width compact control when unread count is non-zero.UI tests add island edge-pinning and unread-badge coverage; the delayed-terminal preview can inject unread fixture workspaces via
CMUX_UITEST_WORKSPACE_DETAIL_UNREAD_BACK_COUNT.Reviewed by Cursor Bugbot for commit d28c310. Bugbot is set up for automated code reviews on this repo. Configure here.