Repository navigation
iOS #6271: top-right "+" adds a terminal to the current workspace - #6865
austinywang wants to merge 5 commits into
Conversation
… affordance Regression test (red) for the TestFlight complaint that the prominent "+" button next to the iOS terminal picker spins up a whole new WORKSPACE instead of adding a terminal to the current one. Models the two add affordances (new-terminal-in-current-workspace vs new-workspace) as a pure value type in CMUXMobileCore — the only mobile package CI exercises headlessly via `swift test` — so the contract is guarded by a real CI gate. This commit ships the test with the affordance deliberately pinned to the buggy `.newWorkspace`; the next commit flips it and wires the button. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a public mobile terminal add-affordance enum, wires the iOS top-right “+” button to create a terminal in the current workspace, keeps the alternate new-workspace action available, updates trailing title width reservation, and adds tests for the affordance and icon mapping. ChangesMobile terminal add flow
Sequence Diagram(s)sequenceDiagram
participant User
participant newTerminalToolbarButton
participant performPrimaryAdd
participant MobileTerminalAddAffordance
participant WorkspaceDetailView
User->>newTerminalToolbarButton: tap "+"
newTerminalToolbarButton->>performPrimaryAdd: invoke action
performPrimaryAdd->>MobileTerminalAddAffordance: read primaryNavbarButton
MobileTerminalAddAffordance-->>performPrimaryAdd: newTerminalInCurrentWorkspace
alt primaryNavbarButton == newTerminalInCurrentWorkspace
performPrimaryAdd->>WorkspaceDetailView: createTerminalFromToolbar()
WorkspaceDetailView->>WorkspaceDetailView: close browser / clear chat state
else primaryNavbarButton == newWorkspace
performPrimaryAdd->>WorkspaceDetailView: createWorkspaceFromToolbar()
end
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 SummaryRestores an always-present "+" toolbar button in all three iOS workspace navbars (terminal/browser/chat) that adds a terminal to the current workspace, fixing issue #6271 where it was creating a whole new workspace instead. The action is guarded by a core unit test that pins
Confidence Score: 5/5Safe to merge — the change is well-scoped iOS UI wiring with no shared-state mutations or concurrency hazards. The new terminal button is pure UI, the core affordance contract is guarded by a headless unit test, No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User taps '+' (newTerminalToolbarButton)"] --> B["performPrimaryAdd()"]
B --> C{"MobileTerminalAddAffordance\n.primaryNavbarButton"}
C -->|".newTerminalInCurrentWorkspace\n(pinned by unit test)"| D["createTerminalFromToolbar()"]
C -->|".newWorkspace"| E["createWorkspaceFromToolbar()"]
D --> F["createTerminal() → Bool"]
F -->|"false: no workspace / task in-flight"| G["Early return — overlays stay open"]
F -->|"true: terminal created / dispatched"| H["dismissTerminalKeyboardForChrome()"]
H --> I["browserStore.closeBrowser()"]
I --> J["withAnimation: isChatMode = false"]
J --> K["pinnedChatSessionID = nil"]
K --> L["Terminal surfaces in detailSurfaceContent"]
M["Picker menu 'New Terminal'"] --> D
N["Picker menu 'New Workspace'"] --> E
style C fill:#f0a,color:#fff
style G fill:#faa
style L fill:#afa
%%{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["User taps '+' (newTerminalToolbarButton)"] --> B["performPrimaryAdd()"]
B --> C{"MobileTerminalAddAffordance\n.primaryNavbarButton"}
C -->|".newTerminalInCurrentWorkspace\n(pinned by unit test)"| D["createTerminalFromToolbar()"]
C -->|".newWorkspace"| E["createWorkspaceFromToolbar()"]
D --> F["createTerminal() → Bool"]
F -->|"false: no workspace / task in-flight"| G["Early return — overlays stay open"]
F -->|"true: terminal created / dispatched"| H["dismissTerminalKeyboardForChrome()"]
H --> I["browserStore.closeBrowser()"]
I --> J["withAnimation: isChatMode = false"]
J --> K["pinnedChatSessionID = nil"]
K --> L["Terminal surfaces in detailSurfaceContent"]
M["Picker menu 'New Terminal'"] --> D
N["Picker menu 'New Workspace'"] --> E
style C fill:#f0a,color:#fff
style G fill:#faa
style L fill:#afa
Reviews (7): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public enum MobileTerminalPrimaryAddButton { | ||
| /// What tapping the prominent "+" does. | ||
| public static let affordance: MobileTerminalAddAffordance = .newTerminalInCurrentWorkspace | ||
| } |
There was a problem hiding this comment.
MobileTerminalPrimaryAddButton is a caseless enum whose only purpose is to hold a single static let — exactly the pattern the no-ambient-global-state rule flags. The same constant can live as a static let member on MobileTerminalAddAffordance directly (via an extension), which avoids introducing a second uninhabitable type and keeps the test surface on the same type that owns the cases.
Rule Used: Flag new ambient global state in production Swift:... (source)
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!
There was a problem hiding this comment.
Already addressed by the current shape: the primary binding lives directly on MobileTerminalAddAffordance.primaryNavbarButton; the separate caseless namespace enum is gone.
— Claude Code
| func performPrimaryAdd() { | ||
| switch MobileTerminalPrimaryAddButton.affordance { | ||
| case .newTerminalInCurrentWorkspace: | ||
| createTerminalFromToolbar() | ||
| case .newWorkspace: | ||
| createWorkspaceFromToolbar() | ||
| } | ||
| } |
There was a problem hiding this comment.
Both toggleChatMode() and the onExitChat closure wrap isChatMode = false in withAnimation(.snappy(duration: 0.28)), so the .move(edge: .top).combined(with: .opacity) transition plays. Here the assignment is bare, so tapping "+" while in chat mode instantly snaps the pane away rather than easing it out.
| func performPrimaryAdd() { | |
| switch MobileTerminalPrimaryAddButton.affordance { | |
| case .newTerminalInCurrentWorkspace: | |
| createTerminalFromToolbar() | |
| case .newWorkspace: | |
| createWorkspaceFromToolbar() | |
| } | |
| } | |
| func performPrimaryAdd() { | |
| withAnimation(.snappy(duration: 0.28)) { | |
| switch MobileTerminalPrimaryAddButton.affordance { | |
| case .newTerminalInCurrentWorkspace: | |
| createTerminalFromToolbar() | |
| case .newWorkspace: | |
| createWorkspaceFromToolbar() | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Addressed in 4f321d0: the chat-mode exit path in createTerminalFromToolbar() now wraps isChatMode = false in withAnimation(.snappy(duration: 0.28)) after terminal creation is accepted.
— Claude Code
d4dd49b to
0f75651
Compare
![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 843-852: Add a guard in createTerminalFromToolbar before tearing
down browser/chat state, because createTerminal() can no-op when workspace
lookup fails or a terminal creation task is already in flight. Use a
canCreateTerminal-style check (mirroring createWorkspaceFromToolbar and the
createTerminal(in:) early-return conditions) to confirm terminal creation can
actually proceed, and only then call browserStore.closeBrowser, clear
isChatMode/pinnedChatSessionID, and invoke createTerminal().
🪄 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: 5412db22-dde6-477b-af17-3bb1ec9ccf52
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileTerminalAddAffordance.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/MobileTerminalAddAffordanceTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileNavTitleWidth.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+TerminalAdd.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift
TestFlight feedback: the prominent "+" button next to the iOS terminal picker spun up a whole new WORKSPACE, which testers found unintuitive — they expected it to add a terminal to the workspace they were already in (the macOS default). - Flip `MobileTerminalPrimaryAddButton.affordance` to `.newTerminalInCurrentWorkspace`, turning the red regression test green. - Add a prominent top-bar "+" (`newTerminalToolbarButton`) immediately left of the terminal picker in all three iOS navbars (terminal / browser / chat), routed through the pinned affordance to `createTerminalFromToolbar`. New Workspace stays reachable from the picker menu (its layered `plus.square.on.square` glyph), so no capability is lost. - Surface the new terminal even from an overlay: `createTerminalFromToolbar` now also exits agent-chat mode (it already closed the browser). - Reserve trailing width for the always-present "+" in `MobileNavTitleWidth` so a long workspace title can't underlap the bar buttons. Reuses the already-localized `mobile.terminal.new` string (en + ja); no new user-facing copy. Grows WorkspaceDetailView.swift by 5 lines; budget refreshed. Fixes #6271 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0f75651 to
63c4582
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
This comment has been minimized.
This comment has been minimized.
…ght-button-makes-a # Conflicts: # .github/swift-file-length-budget.tsv
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Fixes #6271
Problem
From TestFlight feedback (cmux BETA, iPhone): the prominent "+" button in the
top-right of the iOS workspace navbar — next to the terminal (
>_) picker — spunup a whole new workspace with a terminal. Testers found this unintuitive and
expected it to add a terminal to the current workspace (the macOS default).
The misleading button was the overlapping-cards
plus.square.on.squareaffordance. A prior change (#6362) removed that top-level button and buried both
"New Workspace" and "New Terminal" in the picker menu, so on current
maintherewas no obvious one-tap way to add a terminal at all.
Fix
newTerminalToolbarButton) immediatelyto the left of the terminal picker, in all three iOS navbars (terminal /
browser / chat). It uses the plain
plusglyph (macOS "new tab") and adds aterminal to the current workspace.
MobileTerminalPrimaryAddButton, inCMUXMobileCore(the one mobile package CI exercises headlessly viaswift test), so the wiring is guarded by a real CI gate and can't silentlyregress back to "new workspace".
plus.square.on.squareglyph) — no capability lost.createTerminalFromToolbarnow also exits agent-chat mode (it already closedthe browser) so the freshly-created terminal is actually surfaced from any
overlay pane.
MobileNavTitleWidthreserves trailing width for the always-present "+" so along workspace title can't underlap the bar buttons.
Tests
Two-commit red→green (verified locally):
MobileTerminalAddAffordanceTestswith the affordancepinned to the buggy
.newWorkspace→swift test --package-path Packages/Shared/CMUXMobileCorefails (3 expectations)..newTerminalInCurrentWorkspaceand wires the button→ green.
The test runs in CI's
mobile-core-packagejob (Packages/Shared/**pathfilter). The SwiftUI toolbar wiring itself isn't headlessly unit-testable
(
CmuxMobileShellUIis UIKit-bound and not in any CI test scheme), so theguarded seam is the pure affordance contract plus the new
MobileTerminalNewTerminalButtonaccessibility identifier.Localization
No new user-facing strings — the button reuses the already-localized
mobile.terminal.new("New Terminal" / "新規ターミナル", en + ja).🤖 Generated with Claude Code
Summary by cubic
Restores a top-bar “+” on iOS that adds a terminal to the current workspace, matching macOS and fixing #6271. Overlays only close when creation succeeds, and the new terminal is shown immediately.
CmuxMobileShellUI).createTerminalnow returnsBool; we only close the browser and exit agent chat after a successful creation (with preview tests for success/missing workspace).CMUXMobileCore(MobileTerminalAddAffordance) and pinned the primary action and glyphs with a unit test to prevent regressions.Written for commit cb26337. Summary will update on new commits.
Summary by CodeRabbit