Repository navigation
Flatten mobile terminal switcher menu - #7087
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:
📝 WalkthroughWalkthrough
ChangesWorkspace toolbar and picker flattening
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 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 |
Greptile SummaryThis PR flattens the mobile nav by making the workspace title pill non-interactive (
Confidence Score: 5/5Safe to merge: all three toolbar contexts (terminal, browser, chat) reach workspace actions through the terminal picker, localization is complete for the new button label, and the tests correctly validate the non-interactive title element and the flattened menu structure. The workspace action surface is preserved across all toolbar modes via the terminal picker's embedded titleMenuContent. The non-interactive title control is consistently handled by mobileGlassCompactNavigationTitle() on iOS 26+ (glass button with hit testing and .isButton removed) and falls back to a plain view on earlier OS versions. Tests are updated to match the new accessibility element type and menu location, and the mock server now correctly seeds host capabilities so action items are visible in connected-app tests. No production regressions or logic gaps were identified. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
WDV[WorkspaceDetailView] -->|terminal mode| TC[WorkspaceToolbarTitleControl non-interactive title pill]
WDV -->|browser mode| TC
WDV -->|chat mode via WorkspaceDetailView+AgentChat| TC
WDV -->|all modes trailing cluster| TPTB[terminalPickerToolbarButton MobileTerminalDropdown]
TPTB --> TPMC[terminalPickerMenuContent]
TPMC --> TS[Section Terminals ForEach workspace.terminals]
TPMC --> TMC[titleMenuContent WorkspaceTitleMenuContent Rename / Read State / Close]
TPMC --> NS[Section New Workspace / New Terminal / New Browser]
TPMC --> DS[Section View as Text / Send Feedback]
TC -.->|accessibilityIdentifier| AID[MobileWorkspaceTitleMenu not a button hit testing off .isButton removed on iOS 26+]
%%{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
WDV[WorkspaceDetailView] -->|terminal mode| TC[WorkspaceToolbarTitleControl non-interactive title pill]
WDV -->|browser mode| TC
WDV -->|chat mode via WorkspaceDetailView+AgentChat| TC
WDV -->|all modes trailing cluster| TPTB[terminalPickerToolbarButton MobileTerminalDropdown]
TPTB --> TPMC[terminalPickerMenuContent]
TPMC --> TS[Section Terminals ForEach workspace.terminals]
TPMC --> TMC[titleMenuContent WorkspaceTitleMenuContent Rename / Read State / Close]
TPMC --> NS[Section New Workspace / New Terminal / New Browser]
TPMC --> DS[Section View as Text / Send Feedback]
TC -.->|accessibilityIdentifier| AID[MobileWorkspaceTitleMenu not a button hit testing off .isButton removed on iOS 26+]
Reviews (5): Last reviewed commit: "Assert mobile title control is nonintera..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 298-300: Finish migrating the remaining title-pill UI tests by
replacing any `app.buttons["MobileWorkspaceTitleMenu"]` lookup and tap in the
old dropdown flows with the flattened terminal-picker path used by
`WorkspaceToolbarTitleControl`, so the tests no longer depend on the
non-hit-testable title control. Update the affected test helpers/cases around
the existing menu assertions to use the new picker interaction and keep the
rename/read-state/close menu checks aligned with the migrated path.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 442-443: The rectangle-stack picker in WorkspaceDetailView is
still exposed with the misleading “Terminals” label even though it now opens
both workspace and terminal actions. Update the picker’s accessibility/title
label to a combined workspace+terminal menu name, and route it through the
localized string API used elsewhere in the view so the new text has matching
string-catalog entries for all supported locales. Use the WorkspaceDetailView
picker/title menu symbols to locate and replace the current label source.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceTitleMenu.swift`:
- Around line 20-22: The iOS fallback in WorkspaceTitleMenu currently puts the
MobileWorkspaceTitleMenu accessibility identifier on fittedLabel, so it is no
longer exposed as a button. Update the fallback branch in WorkspaceTitleMenu to
keep the same accessibility container/traits as the supported OS path, or adjust
the element so app.buttons["MobileWorkspaceTitleMenu"] still resolves correctly
for callers.
🪄 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: e11dfc67-db7c-4544-88ad-76714750272c
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceChatPane.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+AgentChat.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceRenameDialog.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceTitleMenu.swiftios/cmuxUITests/cmuxUITests.swift
💤 Files with no reviewable changes (1)
- Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+AgentChat.swift
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)
3484-3485: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the UI-test
mobile.host.statuspayload aligned with production.
Sources/Mobile/MobileHostService.swift:296-310documents the publicmobile.host.statusreply as onlyroutes,terminal_fidelity, andcapabilities. Addingmac_device_id/mac_display_namehere means the test server accepts a shape the real handler never emits, so client code can accidentally start depending on impossible fields and still pass CI. MirrorpublicStatusPayload(...)instead, and leave Mac identity on the authenticated attach/network-status path.Also applies to: 3607-3618
🤖 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 3484 - 3485, The `mobile.host.status` UI-test response is exposing extra Mac identity fields that the production `publicStatusPayload(...)` never returns. Update `mobileHostStatusResult()` (and the related `mobile.host.status` test cases) so the payload matches the production `MobileHostService` shape exactly, keeping only `routes`, `terminal_fidelity`, and `capabilities`, and leave `mac_device_id` / `mac_display_name` on the authenticated attach/network-status path.
🤖 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 3484-3485: The `mobile.host.status` UI-test response is exposing
extra Mac identity fields that the production `publicStatusPayload(...)` never
returns. Update `mobileHostStatusResult()` (and the related `mobile.host.status`
test cases) so the payload matches the production `MobileHostService` shape
exactly, keeping only `routes`, `terminal_fidelity`, and `capabilities`, and
leave `mac_device_id` / `mac_display_name` on the authenticated
attach/network-status path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1313316-f544-4614-ab70-1d5c1013555c
📒 Files selected for processing (5)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AgentChatDemoScreen.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceTitleMenu.swiftios/cmux/Resources/Localizable.xcstringsios/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 `@ios/cmuxUITests/cmuxUITests.swift`:
- Line 289: The UI test is querying MobileWorkspaceTitleMenu through
app.buttons, but WorkspaceToolbarTitleControl now assigns that identifier to a
non-interactive view, so the lookup will never succeed. Update the title-control
existence check in cmuxUITests to query the correct element type for
MobileWorkspaceTitleMenu, then keep the subsequent picker assertions unchanged
so the test reaches them.
🪄 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: 2df0b2af-5e34-44be-8389-4596d5a93c32
⛔ 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/WorkspaceChatPane.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftios/cmuxUITests/cmuxUITests.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@ios/cmuxUITests/cmuxUITests.swift`:
- Line 289: The UI test is querying MobileWorkspaceTitleMenu through
app.buttons, but WorkspaceToolbarTitleControl now assigns that identifier to a
non-interactive view, so the lookup will never succeed. Update the title-control
existence check in cmuxUITests to query the correct element type for
MobileWorkspaceTitleMenu, then keep the subsequent picker assertions unchanged
so the test reaches them.
🪄 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: 2df0b2af-5e34-44be-8389-4596d5a93c32
⛔ 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/WorkspaceChatPane.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftios/cmuxUITests/cmuxUITests.swift
🛑 Comments failed to post (1)
ios/cmuxUITests/cmuxUITests.swift (1)
289-289: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stop querying
MobileWorkspaceTitleMenuas a button.
WorkspaceToolbarTitleControlnow exposes that identifier on a non-interactive view, soapp.buttons["MobileWorkspaceTitleMenu"]will stay false even when the title control is present. This test will fail before it reaches the flattened picker assertions.Proposed fix
- XCTAssertTrue(app.buttons["MobileWorkspaceTitleMenu"].waitForExistence(timeout: 4)) + XCTAssertTrue(app.descendants(matching: .any)["MobileWorkspaceTitleMenu"].waitForExistence(timeout: 4))📝 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.XCTAssertTrue(app.descendants(matching: .any)["MobileWorkspaceTitleMenu"].waitForExistence(timeout: 4))🤖 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` at line 289, The UI test is querying MobileWorkspaceTitleMenu through app.buttons, but WorkspaceToolbarTitleControl now assigns that identifier to a non-interactive view, so the lookup will never succeed. Update the title-control existence check in cmuxUITests to query the correct element type for MobileWorkspaceTitleMenu, then keep the subsequent picker assertions unchanged so the test reaches them.
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 (1)
ios/cmuxUITests/cmuxUITests.swift (1)
3519-3520: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the UI-test
mobile.host.statusmock aligned with the production payload.The new mock status route appears to hand-build a partial capability list and envelope, but production derives capabilities from
MobileHostService.mobileHostCapabilitiesand returns the fullerv2MobileHostStatusshape. That creates a second source of truth, so capability-gating regressions can be masked in UI tests when the real host contract changes. Mirror the production payload shape/capability source here instead of hardcoding a subset. As per path instructions,reliability-single-source-of-truth.mdsays “tests/mocks should reflect that single source of truth.”Also applies to: 3642-3653
🤖 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 3519 - 3520, The UI-test mock for mobile.host.status is diverging from the production contract by hardcoding a partial payload instead of mirroring the real status shape and capability source. Update mobileHostStatusResult and the related mock route handling to build the response from the same source as production, namely MobileHostService.mobileHostCapabilities and the v2MobileHostStatus envelope, so the mock stays aligned when the production contract changes.Source: Path instructions
🤖 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 289-290: The current assertions around MobileWorkspaceTitleMenu
only verify that the element exists, so they do not catch regressions where the
title pill becomes tappable again. Update the affected UI tests to keep the
existence check via titleControl/MobileWorkspaceTitleMenu, and add a negative
assertion that app.buttons["MobileWorkspaceTitleMenu"] does not exist (or an
equivalent non-interactive assertion) so the title menu is confirmed to be
non-interactive in each of the listed test blocks.
---
Outside diff comments:
In `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 3519-3520: The UI-test mock for mobile.host.status is diverging
from the production contract by hardcoding a partial payload instead of
mirroring the real status shape and capability source. Update
mobileHostStatusResult and the related mock route handling to build the response
from the same source as production, namely
MobileHostService.mobileHostCapabilities and the v2MobileHostStatus envelope, so
the mock stays aligned when the production contract changes.
🪄 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: 068da6ec-6d2b-4315-aa5c-273a2fdb91c8
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceToolbarTitleControl.swiftios/cmuxUITests/cmuxUITests.swift
💤 Files with no reviewable changes (1)
- Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceToolbarTitleControl.swift
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)
3522-3523: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the
mobile.host.statusmock aligned with the public-status contract.This mock now advertises
mac_display_name/mac_device_idonmobile.host.status, butSources/Mobile/MobileHostService.swift:296-331defines that route’s public payload as identity-free; those fields only belong on the verified identity-status path. Leaving them here lets UI tests pass against a payload production should never serve, which can hide incorrect host-identity reads. Restrict this mock to the public status fields (capabilities,terminal_fidelity, etc.), or add a separate mock for the authenticated identity-bearing route if a test actually needs Mac identity. As per path instructions, "flag more than one disagreeing source of truth for the same fact" and fail closed when the reliable signal is missing.Also applies to: 3645-3656
🤖 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 3522 - 3523, The `mobile.host.status` mock in `cmuxUITests` is returning Mac identity fields that do not belong in the public-status contract. Update `mobileHostStatusResult()` and the `case "mobile.host.status"` handling so this mock only serves the public fields defined by `MobileHostService`’s public status route (such as capabilities and terminal fidelity), and move any `mac_display_name` / `mac_device_id` coverage to a separate authenticated identity-status mock if needed.Source: Path instructions
🤖 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 3522-3523: The `mobile.host.status` mock in `cmuxUITests` is
returning Mac identity fields that do not belong in the public-status contract.
Update `mobileHostStatusResult()` and the `case "mobile.host.status"` handling
so this mock only serves the public fields defined by `MobileHostService`’s
public status route (such as capabilities and terminal fidelity), and move any
`mac_display_name` / `mac_device_id` coverage to a separate authenticated
identity-status mock if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4379136d-ef65-4c5e-a1f3-8a42463207b3
📒 Files selected for processing (1)
ios/cmuxUITests/cmuxUITests.swift
* Revert "Flatten mobile terminal switcher menu (#7087)" This reverts commit c259e38. * Keep mobile terminal picker stable * Reuse title menu in mobile chat chrome * Gate mobile title menu on actions * Preserve terminal picker fallback selection * Address mobile picker review feedback * Satisfy mobile picker policy review
This branch was successfully deployed
1 participant
Summary
MobileTerminalDropdown.Closes #7085
Verification
git diff --checkResources/Localizable.xcstringsandios/cmux/Resources/Localizable.xcstrings; verified existing mobile menu/action keys have English and Japanese entries inios/cmux/Resources/Localizable.xcstrings.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Flattened the mobile nav by making the workspace title pill non-interactive and moving workspace actions into the terminal picker. This reduces menu layers on mobile and addresses #7085.
WorkspaceToolbarTitleControl; removedWorkspaceTitleMenu; retained the "MobileWorkspaceTitleMenu" accessibility identifier, and UI tests now assert it is non-interactive.mobile.terminal.picker.menuTitle("Workspace and Terminals").WorkspaceDetailView,WorkspaceChatPane, and inline chat/browser views to use the new title control and inject workspace actions into the picker.mobile.host.status.Written for commit 0d5ad7d. Summary will update on new commits.
Summary by CodeRabbit
WorkspaceToolbarTitleControlto unify the iOS workspace title control across browser, terminal, and inline chat, including chat-toggle and back-button behavior.