Repository navigation
Prioritize mobile toolbar controls over long titles - #7229
azooz2003-bit wants to merge 8 commits into
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:
📝 WalkthroughWalkthroughReplaces width-based toolbar title sizing with role-based layout priorities across workspace toolbars, updates UI regression coverage for title/trailing control spacing, and centralizes delayed-terminal preview workspace construction in shared helpers. ChangesToolbar title and trailing control layout
Preview workspace construction helpers
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
![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 29-33: The `fittedLabel` view in `WorkspaceTitleMenu` applies a
redundant `layoutPriority` that has no effect because it is the only content
inside the `Menu`/`Button`; remove the inner
`.layoutPriority(MobileToolbarItemLayoutRole.compressibleTitle.swiftUILayoutPriority)`
from `fittedLabel` and keep the priority only on `MobileToolbarPriorityHost`,
which is the place that governs toolbar item compression.
🪄 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: 737f236a-9242-42f5-8679-830e8b632e6a
📒 Files selected for processing (7)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileLeadingToolbarTitleWidth.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceTitleMenu.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileLeadingToolbarTitleWidthTests.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceToolbarLayoutPriorityTests.swiftios/cmuxUITests/cmuxUITests.swift
💤 Files with no reviewable changes (2)
- Packages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileLeadingToolbarTitleWidthTests.swift
- Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileLeadingToolbarTitleWidth.swift
Greptile SummaryReplaces the old width-cap geometry approach (measuring
Confidence Score: 5/5The change is safe to merge; it removes stateful geometry measurement code and replaces it with a well-scoped layout-priority HStack, with no regressions on actor isolation, blocking primitives, or ambient global state. The production changes are narrowly scoped to toolbar composition in CmuxMobileShellUI. Removing the onGeometryChange/contentWidth state loop is a net improvement to layout stability. The new MobileWorkspacePriorityToolbar correctly places all three toolbar items as true HStack siblings, so the layoutPriority competition is structurally valid. The only finding is WorkspaceDetailPreviewWorkspaceBuilder landing in production Sources under #if DEBUG with no production caller, which is a style concern consistent with the project's existing preview-view pattern. No files require special attention; WorkspaceDetailPreviewWorkspaceBuilder.swift has a minor placement concern (debug-only type in production Sources) but poses no runtime risk. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WorkspaceDetailView / AgentChatDemoScreen]
A --> B[ToolbarItem placement: .principal]
B --> C[MobileWorkspacePriorityToolbar]
C --> D[back layoutPriority 1 fixedSize]
C --> E[title layoutPriority -1 maxWidth .infinity]
C --> F[trailing layoutPriority 1 fixedSize]
E --> G[WorkspaceTitleMenu]
G --> H[MobileToolbarPriorityHost role: .compressibleTitle]
H --> I[Menu or Button fittedLabel]
%%{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 / AgentChatDemoScreen]
A --> B[ToolbarItem placement: .principal]
B --> C[MobileWorkspacePriorityToolbar]
C --> D[back layoutPriority 1 fixedSize]
C --> E[title layoutPriority -1 maxWidth .infinity]
C --> F[trailing layoutPriority 1 fixedSize]
E --> G[WorkspaceTitleMenu]
G --> H[MobileToolbarPriorityHost role: .compressibleTitle]
H --> I[Menu or Button fittedLabel]
Reviews (5): Last reviewed commit: "fix: move workspace preview builder to o..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private var fittedLabel: some View { | ||
| let cap = MobileLeadingToolbarTitleWidth( | ||
| contentWidth: contentWidth, | ||
| hasBackButton: hasBackButton, | ||
| hasTrailingCluster: hasTrailingCluster, | ||
| hasChatToggle: hasChatToggle | ||
| ).cap | ||
|
|
||
| return label() | ||
| .frame( | ||
| minWidth: min(MobileLeadingToolbarTitleWidth.floor, cap), | ||
| maxWidth: cap, | ||
| alignment: .leading | ||
| ) | ||
| label() | ||
| .layoutPriority(MobileToolbarItemLayoutRole.compressibleTitle.swiftUILayoutPriority) | ||
| .fixedSize(horizontal: false, vertical: true) | ||
| } |
There was a problem hiding this comment.
.layoutPriority(-1) applied to label() here is redundant. fittedLabel is used as the sole child inside a Menu.label or Button.label closure — there are no sibling views in that container to compete with, so the priority modifier has no observable effect. The effective layout priority is already established by MobileToolbarPriorityHost.body (line 62), which applies role.swiftUILayoutPriority to the whole Menu/Button. Removing the inner application keeps the intent readable at a single, correct call site.
| private var fittedLabel: some View { | |
| let cap = MobileLeadingToolbarTitleWidth( | |
| contentWidth: contentWidth, | |
| hasBackButton: hasBackButton, | |
| hasTrailingCluster: hasTrailingCluster, | |
| hasChatToggle: hasChatToggle | |
| ).cap | |
| return label() | |
| .frame( | |
| minWidth: min(MobileLeadingToolbarTitleWidth.floor, cap), | |
| maxWidth: cap, | |
| alignment: .leading | |
| ) | |
| label() | |
| .layoutPriority(MobileToolbarItemLayoutRole.compressibleTitle.swiftUILayoutPriority) | |
| .fixedSize(horizontal: false, vertical: true) | |
| } | |
| private var fittedLabel: some View { | |
| label() | |
| .fixedSize(horizontal: false, vertical: true) | |
| } |
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!
| enum MobileToolbarItemLayoutRole { | ||
| case compressibleTitle | ||
| case fixedTrailingControls | ||
|
|
||
| var swiftUILayoutPriority: Double { | ||
| switch self { | ||
| case .compressibleTitle: | ||
| return -1 | ||
| case .fixedTrailingControls: | ||
| return 1 | ||
| } | ||
| } | ||
| } | ||
|
|
||
| struct MobileToolbarPriorityHost<Content: View>: View { | ||
| let role: MobileToolbarItemLayoutRole | ||
| let content: Content | ||
|
|
||
| init(role: MobileToolbarItemLayoutRole, @ViewBuilder content: () -> Content) { | ||
| self.role = role | ||
| self.content = content() | ||
| } | ||
|
|
||
| @ViewBuilder | ||
| var body: some View { | ||
| content | ||
| .layoutPriority(role.swiftUILayoutPriority) | ||
| } |
There was a problem hiding this comment.
MobileToolbarItemLayoutRole / MobileToolbarPriorityHost may not create the cross-item competition the naming implies. SwiftUI's layoutPriority only establishes relative priority among sibling views inside the same SwiftUI layout container. In an iOS navigation bar, items placed in .topBarLeading (or .principal) and .topBarTrailing are wrapped in separate UIBarButtonItem custom views by UIKit — they are not siblings in any shared SwiftUI layout pass. The host applies layoutPriority(1) to the trailing cluster and layoutPriority(-1) to the title, but because the two live in different UIKit containers there is no SwiftUI container above them to act on those priorities relative to each other.
The title truncates correctly in practice because UIKit's navigation bar layout natively reserves trailing item space before constraining the leading/title area — not because of the SwiftUI priority values. A future author adding a third role value or tweaking the priority number would expect the abstraction to govern the competition, but it doesn't. Consider documenting that the priority modifier here scopes to within each item's own view subtree and that UIKit's inter-placement layout is what prevents title overflow.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceTitleMenu.swift (1)
39-51: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMark the new pure-value
MobileToolbarItemLayoutRoleenumnonisolated.This is a brand-new pure value type (two cases + a computed property, no UI/state dependency). Under this repo's Swift 6 MainActor-by-default isolation, it will implicitly inherit
@MainActorunless explicitly marked otherwise, adding unnecessary actor coupling for a value-only utility.-enum MobileToolbarItemLayoutRole { +nonisolated enum MobileToolbarItemLayoutRole { case compressibleTitle case fixedTrailingControlsAs per coding guidelines: "Report a failure when the diff introduces or materially expands ... pure value model structs that remain implicitly
@MainActorwhen they should be markednonisolated."🤖 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 39 - 51, The new pure-value MobileToolbarItemLayoutRole enum is implicitly inheriting MainActor isolation and should be explicitly marked nonisolated. Update the enum declaration itself, not the swiftUILayoutPriority property, so this value-only type stays decoupled from UI actor isolation under Swift 6 MainActor-by-default rules.Source: Coding guidelines
ios/cmuxUITests/cmuxUITests.swift (1)
2585-2633: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the width-based fallback in this toolbar assertion.
ios/cmuxUITests/cmuxUITests.swift:2585-2633—titleFrameUsesIdealTextWidthcan still pass when the title frame is wider than the space before the trailing controls, so the helper may accept the overlap state it is meant to catch. If this is compensating for a simulator/AX quirk, document that explicitly; otherwise keep only the actual separation check.🤖 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 `@ios/cmuxUITests/cmuxUITests.swift` around lines 2585 - 2633, The toolbar assertion helper in assertToolbarTitleYieldsToTrailingControls is still allowing overlap through the width-based fallback, which can let the bad layout pass. Remove the titleFrameUsesIdealTextWidth condition and rely on the actual separation check between the title frame and the first trailing control; if you must keep any fallback for simulator or AX quirks, make that exception explicit in the helper and document it in-place.
🤖 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 69-92: The MobileWorkspacePriorityToolbar is using
MobileToolbarItemLayoutRole.fixedTrailingControls for the leading back()
control, which is misleading because the role name implies trailing placement.
Update the layout role naming in MobileWorkspacePriorityToolbar to a
position-neutral concept (for example, a fixed control role) and use it
consistently for both back() and trailing() so the semantics match the actual
usage and future maintainers are not confused.
---
Outside diff comments:
In `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 2585-2633: The toolbar assertion helper in
assertToolbarTitleYieldsToTrailingControls is still allowing overlap through the
width-based fallback, which can let the bad layout pass. Remove the
titleFrameUsesIdealTextWidth condition and rely on the actual separation check
between the title frame and the first trailing control; if you must keep any
fallback for simulator or AX quirks, make that exception explicit in the helper
and document it in-place.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceTitleMenu.swift`:
- Around line 39-51: The new pure-value MobileToolbarItemLayoutRole enum is
implicitly inheriting MainActor isolation and should be explicitly marked
nonisolated. Update the enum declaration itself, not the swiftUILayoutPriority
property, so this value-only type stays decoupled from UI actor isolation under
Swift 6 MainActor-by-default rules.
🪄 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: add65eb1-aa7d-48a8-951f-f70567689acb
📒 Files selected for processing (5)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailCreateDelayedTerminalPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailDelayedTerminalPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceTitleMenu.swiftios/cmuxUITests/cmuxUITests.swift
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/MobileWorkspacePriorityToolbar.swift`:
- Around line 4-29: The layout role name fixedTrailingControls is misleading
because MobileWorkspacePriorityToolbar uses it for both the leading back()
control and the trailing control, so rename the enum case to a more general
non-compressing/control-priority name and update the
MobileWorkspacePriorityToolbar body plus the matching usage in
WorkspaceTitleMenu.swift. Keep the behavior the same by preserving the current
swiftUILayoutPriority mapping, but make the symbol name reflect that it applies
to any fixed control, not just trailing ones.
🪄 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: ec7ec4bf-2991-471e-8d2b-ffc34d414f47
📒 Files selected for processing (5)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileToolbarItemLayoutRole.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileToolbarPriorityHost.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileWorkspacePriorityToolbar.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceTitleMenu.swiftios/cmuxUITests/cmuxUITests.swift
💤 Files with no reviewable changes (1)
- Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceTitleMenu.swift
| struct MobileWorkspacePriorityToolbar<Back: View, Title: View, Trailing: View>: View { | ||
| @ViewBuilder let back: () -> Back | ||
| @ViewBuilder let title: () -> Title | ||
| @ViewBuilder let trailing: () -> Trailing | ||
|
|
||
| var body: some View { | ||
| HStack(spacing: 8) { | ||
| back() | ||
| .layoutPriority(MobileToolbarItemLayoutRole.fixedTrailingControls.swiftUILayoutPriority) | ||
| .fixedSize() | ||
| .mobileGlassCompactToolbarControl() | ||
|
|
||
| title() | ||
| .layoutPriority(MobileToolbarItemLayoutRole.compressibleTitle.swiftUILayoutPriority) | ||
| .frame(minWidth: 0, maxWidth: .infinity, alignment: .leading) | ||
| .mobileGlassCompactToolbarControl() | ||
| .clipped() | ||
|
|
||
| trailing() | ||
| .layoutPriority(MobileToolbarItemLayoutRole.fixedTrailingControls.swiftUILayoutPriority) | ||
| .fixedSize() | ||
| .mobileGlassCompactToolbarControl() | ||
| } | ||
| .frame(maxWidth: .infinity, minHeight: 44, maxHeight: 44, alignment: .center) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider renaming fixedTrailingControls for clarity.
The leading back() view is also assigned MobileToolbarItemLayoutRole.fixedTrailingControls (Line 12), even though it's a leading control. Functionally correct today (both leading and trailing controls just need "don't compress" priority), but the name will mislead future readers extending this enum with role-specific behavior beyond priority.
✏️ Optional rename
- case fixedTrailingControls
+ case fixedControl(and update all three call sites in this file plus WorkspaceTitleMenu.swift)
🤖 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/MobileWorkspacePriorityToolbar.swift`
around lines 4 - 29, The layout role name fixedTrailingControls is misleading
because MobileWorkspacePriorityToolbar uses it for both the leading back()
control and the trailing control, so rename the enum case to a more general
non-compressing/control-priority name and update the
MobileWorkspacePriorityToolbar body plus the matching usage in
WorkspaceTitleMenu.swift. Keep the behavior the same by preserving the current
swiftUILayoutPriority mapping, but make the symbol name reflect that it applies
to any fixed control, not just trailing ones.
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileToolbarItemLayoutRole.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileToolbarItemLayoutRole.swift:3">
P3: `fixedTrailingControls` is currently applied to both the leading back control and trailing controls, so the name no longer reflects the role’s actual semantics. A position-agnostic name (for example, `fixedControl`) would make future role-specific behavior easier to reason about and reduce reader confusion.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -0,0 +1,13 @@ | |||
| enum MobileToolbarItemLayoutRole { | |||
| case compressibleTitle | |||
| case fixedTrailingControls | |||
There was a problem hiding this comment.
P3: fixedTrailingControls is currently applied to both the leading back control and trailing controls, so the name no longer reflects the role’s actual semantics. A position-agnostic name (for example, fixedControl) would make future role-specific behavior easier to reason about and reduce reader confusion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileToolbarItemLayoutRole.swift, line 3:
<comment>`fixedTrailingControls` is currently applied to both the leading back control and trailing controls, so the name no longer reflects the role’s actual semantics. A position-agnostic name (for example, `fixedControl`) would make future role-specific behavior easier to reason about and reduce reader confusion.</comment>
<file context>
@@ -0,0 +1,13 @@
+enum MobileToolbarItemLayoutRole {
+ case compressibleTitle
+ case fixedTrailingControls
+
+ var swiftUILayoutPriority: Double {
</file context>
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/WorkspaceDetailCreateDelayedTerminalPreviewView.swift`:
- Around line 92-127: The delayed-terminal preview helper and capability
constant are duplicated here and in WorkspaceDetailDelayedTerminalPreviewView,
so consolidate them into one shared internal helper. Move the common
MobileWorkspaceActionCapabilities and MobileWorkspacePreview construction logic
into a single reusable function/symbol and have both preview views call it,
keeping only any screen-specific differences outside the shared path.
🪄 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: 42981e21-f214-421e-81f4-c49f870266fa
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailCreateDelayedTerminalPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailDelayedTerminalPreviewView.swift
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileToolbarItemLayoutRole.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileToolbarItemLayoutRole.swift:3">
P3: `fixedTrailingControls` is currently applied to both the leading back control and trailing controls, so the name no longer reflects the role’s actual semantics. A position-agnostic name (for example, `fixedControl`) would make future role-specific behavior easier to reason about and reduce reader confusion.</violation>
</file>
<file name="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailCreateDelayedTerminalPreviewView.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailCreateDelayedTerminalPreviewView.swift:92">
P3: This introduces a second, parallel preview-workspace builder with its own action-capabilities constant in a sibling delayed-terminal preview file. Keeping these construction paths duplicated makes preview defaults easier to drift over time; consolidating them behind one shared internal helper would reduce maintenance risk.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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)
ios/cmuxUITests/cmuxUITests.swift (1)
2399-2434: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the duplicated polling scaffolding.
waitForCompactToolbarTitleHeightToFitControlsandassertToolbarTitleYieldsToTrailingControlsboth hand-roll the same deadline/RunLoop.current.runpolling loop pattern (capture frames, check a geometry predicate, sleep 0.05s, repeat until deadline, then fail with a formatted message). Since both were introduced/expanded in this same diff, extracting a small genericpollUntil(timeout:predicate:onTimeout:)helper would avoid the two implementations drifting independently as the toolbar geometry checks evolve.Also applies to: 2544-2587
🤖 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 `@ios/cmuxUITests/cmuxUITests.swift` around lines 2399 - 2434, The polling loop logic is duplicated between waitForCompactToolbarTitleHeightToFitControls and assertToolbarTitleYieldsToTrailingControls; extract the shared deadline/RunLoop.current.run retry pattern into a small reusable helper such as a generic pollUntil(timeout:predicate:onTimeout:) and have both methods delegate to it. Keep the existing geometry-specific checks and XCTFail messages in the callers, but remove the hand-rolled loop from each method so the toolbar assertions stay consistent as they evolve.
🤖 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 `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 2399-2434: The polling loop logic is duplicated between
waitForCompactToolbarTitleHeightToFitControls and
assertToolbarTitleYieldsToTrailingControls; extract the shared
deadline/RunLoop.current.run retry pattern into a small reusable helper such as
a generic pollUntil(timeout:predicate:onTimeout:) and have both methods delegate
to it. Keep the existing geometry-specific checks and XCTFail messages in the
callers, but remove the hand-rolled loop from each method so the toolbar
assertions stay consistent as they evolve.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33b58e9f-5c8a-461a-844f-06c1676e0d55
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swiftios/cmuxUITests/cmuxUITests.swift
Artifact proof for final verificationHead verified: ad2ab57 Final verifier: APPROVED. The verifier inspected the current head, relevant Swift files, UI test assertions, screenshot evidence, focused result bundles, Behavior proven:
Focused evidence:
Tagged dogfood builds from this head:
Residual risk: broad suites were still pending when this proof was added. Focused UI coverage and the final verifier approval cover the toolbar priority regression specifically. |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
Verification
xcodebuild test -scheme CmuxMobileShellUI -destination 'platform=iOS Simulator,id=5AD831AA-8F8C-42F6-BA3E-DA9A0D971C26' -derivedDataPath /tmp/cmux-tprio-pkg -only-testing:CmuxMobileShellUITests/WorkspaceToolbarLayoutPriorityTestsxcodebuild test -workspace ios/cmux.xcworkspace -scheme cmux-ios -configuration Debug -destination 'platform=iOS Simulator,id=5AD831AA-8F8C-42F6-BA3E-DA9A0D971C26' -derivedDataPath /tmp/cmux-ios-tprio-uitests -only-testing:cmuxUITests/cmuxUITests/testInlineWorkspaceLongTitleYieldsToTrailingToolbarControls./scripts/reload.sh --tag tprio./ios/scripts/reload.sh --tag tprio --simulator cmux-tprio-verify --no-setupDogfood
tprioon simulatorcmux-tprio-verify(5AD831AA-8F8C-42F6-BA3E-DA9A0D971C26)/tmp/cmux-tprio-evidence/inline-workspace-long-title-final.pngNotes
WorkspaceMacSelectionTestsfailures around pending machine selection restore state.MobileWorkspaceTitleMenu.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Prioritize mobile toolbar controls over long titles on iOS so chat and terminal buttons always stay visible. Replace width caps with a shared priority toolbar across inline previews and production, with tests that enforce truncation and title height fitting.
Refactors
MobileToolbarPriorityHost,MobileToolbarItemLayoutRole, and sharedMobileWorkspacePriorityToolbar.WorkspaceDetailViewtoolbar into.principalwith prioritized back/title/trailing clusters; inline preview now uses the same toolbar and fixed-size trailing controls.WorkspaceTitleMenuto a compressible title; extractedWorkspaceDetailPreviewWorkspaceBuilderwith stable action capabilities.Bug Fixes
Written for commit ad2ab57. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests