Repository navigation
Fix iOS toolbar glass and lifecycle - #7116
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:
📝 WalkthroughWalkthroughWorkspaceChatPane is now chat-only, WorkspaceDetailView owns toolbar and title wiring, title width math moved to a new leading-toolbar helper, compact-toolbar UI tests were expanded, and compact-stack auto-open timing changed in WorkspaceShellView. ChangesWorkspace chat pane and toolbar refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 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 iOS toolbar glass and lifecycle regressions by consolidating all workspace toolbar ownership (back button, title menu, trailing cluster, dialogs, sheets) onto the stable
Confidence Score: 5/5Safe to merge; changes are well-scoped toolbar ownership and keyboard chrome refactors with no runtime-critical regressions found. The toolbar consolidation is architecturally clean — one stable owner, no duplicate modifier stacks, and the width-math replacement correctly adjusts the formula for a leading vs. centered placement. The keyboard chrome container insertion is carefully mirrored across constraints, animation cancellation, and scroll-edge interaction. The only issue is a new #if DEBUG accessibility seam that extends an existing pattern in ChatTranscriptUITableView, which is a code-organisation concern but does not affect shipping behaviour. No files require special attention for merge safety. Important Files Changed
Reviews (12): Last reviewed commit: "Gate terminal text sheet to terminal mod..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| in: app, | ||
| context: "fresh no-agent workspace immediately after create" | ||
| ) | ||
| RunLoop.current.run(until: Date().addingTimeInterval(5)) |
There was a problem hiding this comment.
RunLoop.current.run(until: Date().addingTimeInterval(5)) measures calendar time starting from whenever the previous assertions finish, not from the moment the workspace finishes its initial settle. On a CI runner under load, assertWorkspaceToolbarVisible for "immediately after create" can itself take up to 4 seconds (each waitForExistence(timeout: 4)), so the 5-second window might expire just as toolbar state is stabilising rather than 5 seconds after it was first stable. Using XCTestExpectation with fulfillmentTimeout: or XCTest's built-in wait(for:timeout:enforceOrder:) would give a deterministic 5-second window relative to a known event rather than to an arbitrary point in the test timeline.
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: 2
🤖 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 `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 304-322: The test in cmuxUITests should stop using a fixed
5-second wait between two toolbar checks and instead poll continuously for the
full window. Replace the RunLoop.current.run pause with repeated validation
using assertWorkspaceToolbarVisible (or equivalent state checks on
freshBackButton, freshTitleMenu, and freshTerminalDropdown) so the test fails
immediately if the toolbar disappears or changes during the interval. Keep the
final assertBackButtonFrameStaysCompactAroundPress check, but make the waiting
logic causality-based rather than a blind sleep.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 174-181: The browser-title branch in WorkspaceDetailView is
duplicating title styling instead of using the shared WorkspaceToolbarTitleView.
Update the activeBrowser path to route browser.title through
WorkspaceToolbarTitleView the same way the default branch does, preserving the
existing fallback to workspace.name and selectedToolbarSubtitle where
appropriate. This keeps truncation and typography consistent and avoids
diverging styling between the browser and non-browser states.
🪄 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: 53064651-548c-4a66-a867-9fec3e667982
📒 Files selected for processing (4)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceChatPane.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+AgentChat.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftios/cmuxUITests/cmuxUITests.swift
💤 Files with no reviewable changes (1)
- Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+AgentChat.swift
| } else if let browser = activeBrowser { | ||
| Text(browser.title ?? workspace.name) | ||
| .font(.headline) | ||
| .lineLimit(1) | ||
| .truncationMode(.tail) | ||
| .foregroundStyle(TerminalPalette.foreground) | ||
| } else { | ||
| WorkspaceToolbarTitleView(title: workspace.name, subtitle: selectedToolbarSubtitle) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Reuse WorkspaceToolbarTitleView for the browser title instead of ad hoc styling.
The browser branch hand-rolls Text(...).font(.headline)...foregroundStyle(...) while the default branch uses WorkspaceToolbarTitleView. Routing the browser title through the same component would keep truncation/typography consistent across modes and avoid drift if the shared title view's styling changes later.
♻️ Possible consolidation
- } else if let browser = activeBrowser {
- Text(browser.title ?? workspace.name)
- .font(.headline)
- .lineLimit(1)
- .truncationMode(.tail)
- .foregroundStyle(TerminalPalette.foreground)
- } else {
+ } else if let browser = activeBrowser {
+ WorkspaceToolbarTitleView(title: browser.title ?? workspace.name, subtitle: nil)
+ } else {
WorkspaceToolbarTitleView(title: workspace.name, subtitle: selectedToolbarSubtitle)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if let browser = activeBrowser { | |
| Text(browser.title ?? workspace.name) | |
| .font(.headline) | |
| .lineLimit(1) | |
| .truncationMode(.tail) | |
| .foregroundStyle(TerminalPalette.foreground) | |
| } else { | |
| WorkspaceToolbarTitleView(title: workspace.name, subtitle: selectedToolbarSubtitle) | |
| } else if let browser = activeBrowser { | |
| WorkspaceToolbarTitleView(title: browser.title ?? workspace.name, subtitle: nil) | |
| } else { | |
| WorkspaceToolbarTitleView(title: workspace.name, subtitle: selectedToolbarSubtitle) |
🤖 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/WorkspaceDetailView.swift`
around lines 174 - 181, The browser-title branch in WorkspaceDetailView is
duplicating title styling instead of using the shared WorkspaceToolbarTitleView.
Update the activeBrowser path to route browser.title through
WorkspaceToolbarTitleView the same way the default branch does, preserving the
existing fallback to workspace.name and selectedToolbarSubtitle where
appropriate. This keeps truncation and typography consistent and avoids
diverging styling between the browser and non-browser states.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| } | ||
| store.selectedWorkspaceID = selectedWorkspaceID | ||
| } | ||
| .onChange(of: store.workspaces.map(\.id)) { _, workspaceIDs in | ||
| compactNavigationPath.removeAll { !workspaceIDs.contains($0) } | ||
| autoOpenSelectedWorkspaceForSoakIfNeeded() | ||
| } | ||
| .onAppear { | ||
| autoOpenSelectedWorkspaceForSoakIfNeeded() | ||
| } |
There was a problem hiding this comment.
The onChange(of: store.workspaces.map(\.id)) block that contained compactNavigationPath.removeAll { !workspaceIDs.contains($0) } was removed without a replacement. When a workspace is deleted on the Mac while it is the current destination in compactNavigationPath, the stale ID remains in the path unless store.selectedWorkspaceID also changes at the same moment. If the Mac does not update selectedWorkspaceID (e.g. the user's selection was already on a different workspace on the Mac side, or there are no remaining workspaces to select), the onChange(of: store.selectedWorkspaceID) handler never fires, and WorkspaceDetailContainer is left rendering a workspace ID that no longer exists in the store. The user must manually press the back button to escape; no automatic recovery occurs.
This reverts commit c917839.
azooz2003-bit
changed the title
Second sync with origin/main (6 commits, incl. #7116 iOS toolbar glass/lifecycle). GhosttySurfaceView.swift auto-merged cleanly on top of the prior coordinator-based resolution; multi-row invariants verified intact (instance persistentToolbarHeight flows into the viewport snapshot's toolbarFrameHeight; no stale Self. static reference). Only conflict was .github/swift-file-length-budget.tsv, regenerated via scripts/swift_file_length_budget.py --write-budget (not hand-edited).
This branch was successfully deployed
1 participant
Summary
.topBarLeadingtoolbar items for Back and the workspace title, so SwiftUI owns their native toolbar identity and presentation.ToolbarSpacer(.fixed, placement: .topBarLeading)between Back and title on iOS 26+, so SwiftUI creates a real toolbar break instead of visually joining adjacent leading items.Related
Testing
git diff --check.xcodebuild -workspace ios/cmux.xcworkspace -scheme CmuxMobileShellUI -destination "platform=iOS Simulator,name=iPhone 17" -derivedDataPath /tmp/cmux-itbar-toolbarspacer build../scripts/reload-cloud.sh --tag itbarfell back to local./scripts/reload.sh --tag itbarand built/Users/abdulazizalbahar/Library/Developer/Xcode/DerivedData/cmux-itbar/Build/Products/Debug/cmux DEV itbar.appat commit4bd42935b0.ios/scripts/reload.sh --tag itbar --simulator "iPhone 17"built, installed, launched, signed in, and auto-paireddev.cmux.ios.itbaron iPhone 17 simulator at commit4bd42935b0.ios/scripts/reload.sh --tag itbar --device-only --device-id 4A52829D-6427-599F-A166-4058881D2DF4 --team 7WLXT3NR37built, signed, installed, launched, signed in, and auto-paireddev.cmux.ios.itbaron Aziz at commit4bd42935b0.swift test --package-path Packages/iOS/CmuxMobileShellUI --filter MobileLeadingToolbarTitleWidthTestsis blocked by SwiftPM package platform inference for this iOS package.Notes
Summary by CodeRabbit
New Features
Bug Fixes