Repository navigation
iOS: native drag & drop in workspace list + create workspace in group - #7384
Merged
Merged
Conversation
WorkspaceReorderCoordinator.workspaceReorderPlan(tabId:before:) plans toIndex as the before-target's index with the dragged row still present, so remove-then-insert places the dragged workspace after the target for any downward move. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds a mobile workspace.move verb (dispatch in TerminalController, body in TerminalController+WorkspaceMove.swift, ticket auth in MobileHostService) that applies group membership and ordering via the existing reorder/group logic, and threads an optional group_id through the mobile workspace.create path. iOS: long-press drag on workspace rows with drop targets on rows and group headers (reorder, move into group at position, ungroup), drop intent computed by a pure MobileWorkspaceDropIntentResolver in CmuxMobileShellModel (unit tested), disabled during search/filter and when disconnected. Group headers gain a context menu with New Workspace in Group. Strings localized EN+JA. Fixes the downward before-move off-by-one in WorkspaceReorderCoordinator (regression test in previous commit). Co-Authored-By: Claude Fable 5 <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 adds a ChangesWorkspace move flow
Estimated code review effort: 4 (Complex) | ~60 minutes macOS workspace reorder planning fix
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant WorkspaceListView
participant MobileShellComposite
participant MobileCoreRPCClient
participant TerminalController
participant MobileHostService
WorkspaceListView->>MobileShellComposite: moveWorkspace(id, groupID, beforeID)
MobileShellComposite->>MobileCoreRPCClient: sendWorkspaceMutation("workspace.move")
MobileCoreRPCClient->>TerminalController: mobileHostHandleRPC("workspace.move")
TerminalController->>MobileHostService: ticketAuthorizationError(workspace.move)
MobileHostService-->>TerminalController: authorization result
TerminalController->>TerminalController: v2MobileWorkspaceMove(params)
TerminalController-->>MobileCoreRPCClient: v2MobileWorkspaceList result
MobileCoreRPCClient-->>MobileShellComposite: updated workspace list
MobileShellComposite-->>WorkspaceListView: refreshed workspace order
Estimated code review effort: 4 (Complex) | ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors)
✅ Passed checks (21 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 SummaryThis PR adds native iOS drag-and-drop reordering to the mobile workspace list and the ability to create workspaces inside groups, backed by two new Mac-side RPC verbs (
Confidence Score: 5/5Safe to merge — authorization logic is correct, off-by-one fix is verified, no data-loss or security issues found. All authorization paths handle missing/stale tokens correctly. The off-by-one fix in WorkspaceReorderCoordinator is mathematically sound. Optimistic state is properly cleared on server reconciliation. Synthetic groupFooter placement is correct (anchor continue guard prevents double-emit). Full EN+JA localization with correct positional format specifiers. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User as iOS User
participant LV as WorkspaceListView
participant R as MoveIntentResolver
participant Shell as MobileShellComposite
participant RPC as MobileCoreRPCClient
participant Auth as MobileHostService
participant TC as TerminalController
participant TM as TabManager
User->>LV: Drag workspace row (onMove)
LV->>R: moveIntent(sourceOffsets:destination:)
R-->>LV: MobileWorkspaceMoveIntent
LV->>LV: Apply optimistic order (MobileWorkspaceOrderMoveApplier)
LV->>LV: "isWorkspaceMovePending = true"
LV->>Shell: moveWorkspace(intent)
Shell->>RPC: workspace.move RPC (with attachTicket)
RPC->>Auth: ticketAuthorizationResultIfNeeded(request)
Auth->>Auth: Check Mac-scoped attach ticket
Auth-->>RPC: nil (authorized)
RPC->>TC: v2MobileWorkspaceMove
TC->>TM: addWorkspaceToGroup / moveWorkspace
TM-->>TC: Updated tab order
TC-->>RPC: Success response
RPC-->>Shell: Result
Shell-->>LV: Reconciled workspace list
LV->>LV: onChange clears optimistic state
%%{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"}}}%%
sequenceDiagram
participant User as iOS User
participant LV as WorkspaceListView
participant R as MoveIntentResolver
participant Shell as MobileShellComposite
participant RPC as MobileCoreRPCClient
participant Auth as MobileHostService
participant TC as TerminalController
participant TM as TabManager
User->>LV: Drag workspace row (onMove)
LV->>R: moveIntent(sourceOffsets:destination:)
R-->>LV: MobileWorkspaceMoveIntent
LV->>LV: Apply optimistic order (MobileWorkspaceOrderMoveApplier)
LV->>LV: "isWorkspaceMovePending = true"
LV->>Shell: moveWorkspace(intent)
Shell->>RPC: workspace.move RPC (with attachTicket)
RPC->>Auth: ticketAuthorizationResultIfNeeded(request)
Auth->>Auth: Check Mac-scoped attach ticket
Auth-->>RPC: nil (authorized)
RPC->>TC: v2MobileWorkspaceMove
TC->>TM: addWorkspaceToGroup / moveWorkspace
TM-->>TC: Updated tab order
TC-->>RPC: Success response
RPC-->>Shell: Result
Shell-->>LV: Reconciled workspace list
LV->>LV: onChange clears optimistic state
Reviews (24): Last reviewed commit: "Make workspace group pending action equa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+2
to
+4
| public struct MobileWorkspaceDropIntentResolver: Sendable { | ||
| /// Creates a resolver. | ||
| public init() {} |
Contributor
There was a problem hiding this comment.
MobileWorkspaceDropIntentResolver holds zero stored properties and its public init() body is empty. Every call site is MobileWorkspaceDropIntentResolver().intent(...) — the instance is constructed and immediately discarded. The three private helpers (validGroupID, workspaceAfterGroup, changesOrder) also operate purely on their arguments with no self access. Per the cmux no-ambient-global-state rule, an empty struct whose entire API is instance methods on stateless data is the same anti-pattern as a static-only namespace type. The canonical fix is to promote intent to public static func intent(workspaces:groups:draggedWorkspaceID:target:) and the three helpers to private static func, or expose the single public entry-point as a file-scoped free function — both are explicitly permitted by the rule and eliminate the meaningless init() allocation at every drop site.
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!
Collaborator
Author
There was a problem hiding this comment.
Fixed in 92ec77e: resolver is now an enum with static intent/helpers; call sites no longer allocate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swift (1)
193-205: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdjust the
afterbranch for downward moves
beforealready compensates for the removal shift;afterstill doesidx + 1unconditionally, soreorderWorkspace(tabId: a, after: b)can landaafter the wrong sibling whenastarts beforeb.Suggested fix
if let afterId { guard let idx = model.tabs.firstIndex(where: { $0.id == afterId }) else { return nil } - return workspaceReorderPlan(tabId: tabId, toIndex: idx + 1) + let targetIndex = currentIndex < idx ? idx : idx + 1 + return workspaceReorderPlan(tabId: tabId, toIndex: targetIndex) }🤖 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/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swift` around lines 193 - 205, The after-branch in workspaceReorderPlan(tabId:before:after:) is still using idx + 1 unconditionally, which can misplace items when the moved tab starts before the afterId tab. Update the WorkspaceReorderCoordinator logic to mirror the before-case adjustment by compensating for the removal shift when currentIndex is less than the target index, then pass the corrected index into workspaceReorderPlan(tabId:toIndex:).
🤖 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 `@cmuxTests/MobileHostAuthorizationTests.swift`:
- Around line 433-465: The new authorization tests in
MobileHostAuthorizationTests and the workspaceMoveAuthorizationError helper only
verify ticket scoping, but they do not cover the actual workspace move behavior
in TerminalController+WorkspaceMove. Add a behavior-level test for
v2MobileWorkspaceMove that exercises a reorder/move without group_id and asserts
the existing group membership is preserved, so the mutation logic is validated
directly instead of only the auth gate.
In
`@Packages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileWorkspaceListItemTests.swift`:
- Around line 181-246: Add test coverage for the untested .afterWorkspace path
in MobileWorkspaceDropIntentResolver.intent(), since current cases only cover
.beforeWorkspace and .groupHeader. Extend MobileWorkspaceListItemTests with
scenarios that exercise dropping after a workspace in both mid-list and
end-of-list positions, verifying the computed
MobileWorkspaceMoveIntent.beforeWorkspaceID uses the next item or nil when
appending. Use the existing helpers and symbols
MobileWorkspaceDropIntentResolver, MobileWorkspaceMoveIntent, and
.afterWorkspace to place the new tests alongside the current drop intent cases.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDragDropModifier.swift`:
- Around line 10-27: Replace the overlay-based GeometryReader in
WorkspaceDragDropModifier.body with onGeometryChange for the height measurement
used by dropTarget. Update the WorkspaceDragDropModifier view so it captures the
row height via onGeometryChange(for:of:action:) and uses that value inside the
dropDestination handler, keeping the existing isEnabled, draggable, and
performDrop flow intact. Avoid introducing any extra layout wrapper just to read
proxy.size.height.
In
`@Packages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceCoordinatorTests.swift`:
- Around line 153-164: Add a sibling regression test in
WorkspaceCoordinatorTests for the downward after case handled by
reorderWorkspace(tabId:after:), mirroring
reorderWorkspaceBeforeDownwardMoveInsertsAtExpectedSlot. Use the same setup with
CoordinatorStubTab instances a, b, and c, call reorder.reorderWorkspace(tabId:
a.id, after: b.id), and assert the final tab order matches the expected
downward-move slot. This will cover the remaining off-by-one path in
WorkspaceReorderCoordinator.swift and lock in the fix for the after branch.
---
Outside diff comments:
In
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swift`:
- Around line 193-205: The after-branch in
workspaceReorderPlan(tabId:before:after:) is still using idx + 1
unconditionally, which can misplace items when the moved tab starts before the
afterId tab. Update the WorkspaceReorderCoordinator logic to mirror the
before-case adjustment by compensating for the removal shift when currentIndex
is less than the target index, then pass the corrected index into
workspaceReorderPlan(tabId:toIndex:).
🪄 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: d5e7d668-05d6-4e9b-bd91-eca7a09871c5
📒 Files selected for processing (23)
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+WorkspaceActions.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileWorkspaceDropIntentResolver.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileWorkspaceDropTarget.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileWorkspaceMoveIntent.swiftPackages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileWorkspaceListItemTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDragDropModifier.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceGroupHeaderRow.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView+Actions.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView+DragDrop.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView+WorkspaceActions.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceCoordinatorTests.swiftSources/Mobile/MobileHostService.swiftSources/TerminalController+WorkspaceMove.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MobileHostAuthorizationTests.swiftios/cmux/Resources/Localizable.xcstrings
Collaborator
Author
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The after branch planned toIndex in pre-removal index space; downward moves landed one row too low. Mirror the before-branch fix by compensating for the dragged tab's removal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… afterWorkspace drops Greptile: the resolver held no state, so its instance API was a hidden static namespace; promote intent and helpers to static on an enum. CodeRabbit: add tests for the previously uncovered afterWorkspace branch (mid-list insert and end-of-list append). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Read the post-mutation groupId from the model instead of the captured reference so the anchor-protection guard stays correct even if the coordinator replaces the workspace instance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines
+1448
to
1455
| private static func ticketWorkspaceAuthorizationError(authorization: MobileAttachTicketAuthorization, workspaceSelection: String?) -> MobileHostRPCError? { | ||
| if let workspaceSelection, authorization.createdWorkspaceIDs.contains(workspaceSelection) { return nil } | ||
| let ticket = authorization.ticket | ||
| if !ticket.workspaceID.isEmpty { | ||
| guard let workspaceSelection, workspaceSelection == ticket.workspaceID else { return scopedTicketError } | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
ticketWorkspaceAuthorizationError does not trim ticket.workspaceID before comparing it against workspaceSelection, while the existing ticketTerminalAuthorizationError — which this function mirrors — always trims first. Two concrete failure modes follow: (1) a ticket whose workspaceID is pure whitespace (e.g. " ") is not caught by the isEmpty guard, so the comparison workspaceSelection == " " never matches a real UUID and every workspace.move on that ticket gets a forbidden response; (2) a ticket with trailing whitespace in workspaceID fails to match the correctly-formed workspaceSelection for the same reason. Both cases would surface as silent authorization failures that are hard to trace back to the missing trim.
Suggested change
| private static func ticketWorkspaceAuthorizationError(authorization: MobileAttachTicketAuthorization, workspaceSelection: String?) -> MobileHostRPCError? { | |
| if let workspaceSelection, authorization.createdWorkspaceIDs.contains(workspaceSelection) { return nil } | |
| let ticket = authorization.ticket | |
| if !ticket.workspaceID.isEmpty { | |
| guard let workspaceSelection, workspaceSelection == ticket.workspaceID else { return scopedTicketError } | |
| } | |
| return nil | |
| } | |
| private static func ticketWorkspaceAuthorizationError(authorization: MobileAttachTicketAuthorization, workspaceSelection: String?) -> MobileHostRPCError? { | |
| if let workspaceSelection, authorization.createdWorkspaceIDs.contains(workspaceSelection) { return nil } | |
| let ticket = authorization.ticket | |
| let ticketWorkspaceID = ticket.workspaceID.trimmingCharacters(in: .whitespacesAndNewlines) | |
| if !ticketWorkspaceID.isEmpty { | |
| guard let workspaceSelection, workspaceSelection == ticketWorkspaceID else { return scopedTicketError } | |
| } | |
| return nil | |
| } |
Collaborator
Author
There was a problem hiding this comment.
Fixed in 32881e7: workspaceID is trimmed before the empty check and comparison, mirroring ticketTerminalAuthorizationError, with tests for padded and whitespace-only ticket workspaceIDs.
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)
Sources/TerminalController+WorkspaceMove.swift (1)
12-15: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty
group_idis rejected even though it's meant to mean "no group".
mobileWorkspaceMoveGroupIDintentionally treats a present-but-empty/whitespacegroup_idasnil(line 122). But the caller at Line 13 can't tell that apart from a malformed UUID string — both producetargetGroupID == nilwhilev2HasNonNullParamistrue, so any client that sendsgroup_id: ""to mean "ungroup" getsinvalid_paramsinstead of being ungrouped. Right now the only caller (iOS composite) omits the key entirely for ungroup, so this is a latent bug rather than an active one, but it contradicts the helper's own documented behavior and will misfire for any other caller/test that sends an explicit empty string.🐛 Proposed fix to only flag genuinely malformed values
- let targetGroupID = mobileWorkspaceMoveGroupID(params: params) - if v2HasNonNullParam(params, "group_id"), targetGroupID == nil { - return .err(code: "invalid_params", message: "Missing or invalid group_id", data: nil) - } + let targetGroupID = mobileWorkspaceMoveGroupID(params: params) + if v2HasNonNullParam(params, "group_id"), targetGroupID == nil, + let rawGroupID = v2RawString(params, "group_id")?.trimmingCharacters(in: .whitespacesAndNewlines), + !rawGroupID.isEmpty { + return .err(code: "invalid_params", message: "Missing or invalid group_id", data: nil) + }Also applies to: 119-126
🤖 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 `@Sources/TerminalController`+WorkspaceMove.swift around lines 12 - 15, The current validation in TerminalController+WorkspaceMove wrongly rejects an explicitly empty or whitespace group_id even though mobileWorkspaceMoveGroupID treats that as “no group”. Update the check around mobileWorkspaceMoveGroupID(params:) and v2HasNonNullParam(params, "group_id") so only genuinely malformed non-empty values return invalid_params, while an empty/blank group_id is allowed to flow through as nil and ungroup the workspace. Keep the nil-handling behavior aligned with mobileWorkspaceMoveGroupID and adjust any related validation logic in the same workspace-move path so explicit empty strings are not treated as errors.
🤖 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/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileWorkspaceDropIntentResolver.swift`:
- Line 2: MobileWorkspaceDropIntentResolver is a caseless enum used only as a
static namespace, which conflicts with the no-ambient-global-state guideline.
Replace the enum with a constructable type and move the static helper(s) into
instance methods, then update the CmuxMobileShellUI call site(s) to use an
injected/resolved instance of MobileWorkspaceDropIntentResolver instead of
calling it statically.
---
Outside diff comments:
In `@Sources/TerminalController`+WorkspaceMove.swift:
- Around line 12-15: The current validation in TerminalController+WorkspaceMove
wrongly rejects an explicitly empty or whitespace group_id even though
mobileWorkspaceMoveGroupID treats that as “no group”. Update the check around
mobileWorkspaceMoveGroupID(params:) and v2HasNonNullParam(params, "group_id") so
only genuinely malformed non-empty values return invalid_params, while an
empty/blank group_id is allowed to flow through as nil and ungroup the
workspace. Keep the nil-handling behavior aligned with
mobileWorkspaceMoveGroupID and adjust any related validation logic in the same
workspace-move path so explicit empty strings are not treated as errors.
🪄 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: 227d9357-c193-41aa-a99c-edfe719211cb
📒 Files selected for processing (6)
Packages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileWorkspaceDropIntentResolver.swiftPackages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileWorkspaceListItemTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView+DragDrop.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceCoordinatorTests.swiftSources/TerminalController+WorkspaceMove.swift
…-safe insertion mapping On iPhone compact List, .dropDestination(for: String.self) never participated as a drop target (drops ended with operation=0). Replace with onDrag NSItemProvider plain-text vending, ForEach.onInsert(of:) for positional drops, and onDrop(of:) on group headers. The insertion-index to drop-target mapping lives in CmuxMobileShellModel as MobileWorkspaceListItem.insertionDropTarget, never crosses group headers (ambiguous header-adjacent gaps are no-ops), and is unit tested for all header-adjacent edge cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Mirror ticketTerminalAuthorizationError: a whitespace-padded ticket workspaceID no longer causes silent forbidden responses, and a whitespace-only workspaceID is treated as Mac-scoped. Adds tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileWorkspaceListItemTests.swift`:
- Around line 268-316: Add a missing test for the header-to-workspace boundary
in MobileWorkspaceListItemTests. The current insertionTarget coverage skips the
case where a drop occurs immediately after an expanded group header and before
its first member, which should resolve through insertionDropTarget’s (_,
.some(.workspace)) path. Add a test near the existing insertionTarget cases that
uses a groupHeader followed by its first indented workspace and asserts the
target is before that workspace.
🪄 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: cea38aa5-63b7-4444-8a68-98d919b84c89
📒 Files selected for processing (8)
Packages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileWorkspaceListItem+DropTarget.swiftPackages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileWorkspaceListItemTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileWorkspaceDragPayload.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDragDropModifier.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView+DragDrop.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceCoordinatorTests.swift
iOS List never delivers drops of its own rows to custom drop targets (dropDestination, onDrop, onInsert all end with operation=0), so the provider-based plumbing could not work. Use ForEach.onMove in both flat and grouped presentations: MobileWorkspaceListItem.moveIntent maps onMove's pre-removal (source, destination) to a move intent, derives group membership from the landing gap, rejects header moves and identity drops, and is unit tested. Optimistic row order applies locally and reconciles after the authoritative Mac resync, including rollback on failed moves. Removes the dead drop-target plumbing and its localization key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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/WorkspaceListView`+DragDrop.swift:
- Around line 27-51: The drag-and-drop intent calculation is mixing snapshots:
`moveFlatRows` builds `items` from `sourceWorkspaces` but still passes
`filteredWorkspaces` into `MobileWorkspaceListItem.moveIntent`, which can cause
anchor resolution and no-op detection to use stale order. Update `moveFlatRows`
to pass the same snapshot used for `items` (and apply the same fix in
`moveGroupedRows`) so `moveIntent` evaluates against the optimistic/current
ordering consistently.
🪄 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: 0f2794ab-f415-4b23-abc7-4090698f59bd
📒 Files selected for processing (7)
Packages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileWorkspaceListItem+MoveIntent.swiftPackages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileWorkspaceListItemTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView+DragDrop.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView+WorkspaceActions.swiftcmuxTests/MobileHostAuthorizationTests.swiftios/cmux/Resources/Localizable.xcstrings
💤 Files with no reviewable changes (1)
- ios/cmux/Resources/Localizable.xcstrings
Group headers now carry the full workspace-style context menu: Pin/Unpin Group, Rename Group (sheet), New Workspace in Group, and Ungroup / Delete Group behind confirmations, backed by a new mobile workspace.group.action verb (dispatch in TerminalController, body in TerminalController+WorkspaceGroupAction.swift, anchor-scoped ticket auth with tests) routed to the existing desktop group logic. Workspace actions stay enabled while disconnected: every list mutation now returns a typed Result, and failures surface in a shared bottom capsule toast (X to dismiss, finger-tracking drag with 50% threshold and spring-back, Clock-injected auto-dismiss with cancellation, one at a time) instead of silently disabling affordances. Create keeps single-flight dedup across both entrypoints. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
# Conflicts: # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+MenuState.swift # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift
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 8f3a563. Configure here.
| return .failure(.rejected(hostDisplayName: connectedHostName)) | ||
| } | ||
| } | ||
| return .failure(.rejected(hostDisplayName: connectedHostName)) |
There was a problem hiding this comment.
Duplicated error-to-failure mapping across two files
Low Severity
The MobileShellConnectionError-to-MobileWorkspaceMutationFailure mapping in createRemoteWorkspace's catch block is a near-exact copy of the workspaceMutationFailure helper in MobileShellComposite+WorkspaceActions.swift. The same switch over .connectionClosed, .requestTimedOut, .rpcError normalization list, etc. is inlined in both places. If a new error case or RPC code is added, both must be updated in lockstep or they'll silently diverge.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8f3a563. Configure here.
This was referenced Jul 8, 2026
This was referenced Jul 16, 2026
Merged
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
1 participant
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.
Adds native drag and drop to the iOS workspace list and a group-header context menu to create a workspace inside a group.
Mac side: new mobile
workspace.moveverb (dispatch inSources/TerminalController.swift, implementation inSources/TerminalController+WorkspaceMove.swift, ticket authorization inSources/Mobile/MobileHostService.swift) applying group membership + ordering through the existing reorder/group logic, and an optionalgroup_idthreaded through the mobileworkspace.createpath. Also fixes a pre-existing downward before-move off-by-one inWorkspaceReorderCoordinator.workspaceReorderPlan(tabId:before:); the first commit adds the failing regression test, the second the fix (CI red→green on the Commits tab).iOS side: long-press drag on workspace rows with drop targets on rows and group headers (reorder, move into a group at a position, drag out to ungroup). Drop intent is computed by a pure
MobileWorkspaceDropIntentResolverin CmuxMobileShellModel with unit tests (reorder, header append, between members, ungroup, self-drop no-op), restricted to the displayed Mac's rows, disabled during search/filter and when disconnected. Group headers get a context menu "New Workspace in Group" which creates and selects the workspace in that group. New strings localized EN+JA with accessibility hints.Tests: CmuxMobileShellModel, CmuxMobileRPC, CmuxMobileShell, CmuxWorkspaces package tests;
cmuxTests/MobileHostAuthorizationTestsgains fourworkspace.moveticket-scope cases. Note:MobileCoreRPCConnectWaiterTests.cancelledPostConnectWaiterDoesNotCloseTransportForSurvivoris a pre-existing flake under full-suite load (file untouched here; passes 5/5 in isolation).🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds native drag‑and‑drop to the iOS workspace list and a full group‑header menu, including creating a workspace in a group. Actions are capability‑ and Mac‑scoped‑ticket gated, failures show in a single toast, and pending moves persist across refreshes.
New Features
List.onMovein flat and grouped lists viaMobileWorkspaceListItem.moveIntent; supports reorder, move into a group, ungroup, and header‑anchor moves; disabled when filtered or when the snapshot spans multiple/unknown windows; group footers clarify in‑group vs top‑level drop zones.workspace.moveandworkspace.group.action;workspace.createaccepts optionalgroup_id. The host advertisesworkspace.move.v1,workspace.group_actions.v1, andworkspace.create_in_group.v1. The app shows move/group/create‑in‑group only when those are supported and a valid Mac‑scoped attach ticket is present;MobileCoreRPCClientexposesattachTicketand preserves it forworkspace.move/workspace.group.action.Bug Fixes
workspace.move/workspace.group.action; ignore stale/unknown tickets for broad RPCs likeworkspace.list/workspace.create; enforce Mac‑scoped tickets for create‑in‑group; authorizeworkspace.action/workspace.closewith workspace‑scoped tickets for the targeted workspace; trim and accept whitespace‑onlyworkspaceIDas Mac‑scoped; add scoped‑ticket regression tests and fix workspace‑ticket auth test compile.createWorkspaceRequestreturnsnotConnectedand never creates locally; single‑flight is target‑aware; blankgroup_idungroups; prevent duplicate error toasts for workspace create.Written for commit 8f3a563. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Note
Medium Risk
Broad changes to workspace mutation RPCs and attach-ticket authorization across shell, RPC client, and UI; incorrect gating or routing could allow unauthorized Mac-wide edits or mis-target secondary Macs, though coverage is heavy on ticket scope and encoding tests.
Overview
Adds native list reordering on iOS for flat and grouped workspace lists:
List.onMovedrivesworkspace.moveon the Mac, with optimistic ordering, group footer rows for clearer in-group vs top-level drops, and a puremoveIntentresolver plus local order appliers in the shell model. Reorder is disabled while searching/filtering, when the snapshot isn’t a single known window, or when move capability / Mac-scoped attach ticket policy blocks it.Extends Mac-backed workspace mutations beyond rename/pin/read/close: move, group actions (
workspace.group.action), and create in group (group_idonworkspace.create). Shell APIs now returnResult<Void, MobileWorkspaceMutationFailure>instead of failing silently; the shell maps RPC/connection errors and surfaces failures via a bottom toast (create/move/group/detail actions share the same path). Group headers gain pin/rename, new workspace in group, ungroup/delete (with confirmations); workspace detail menus wire through the same capability-aware closures.Authorization and RPC framing change for Mac-wide mutations:
MobileShellWorkspaceMutationTicketPolicyallows move/group/create-in-group only when the attach ticket is non-expired and has an emptyworkspaceID(Mac-wide scope).MobileCoreRPCClientexposesattachTicketand always keeps attach-ticket context onworkspace.moveandworkspace.group.actionso workspace-scoped tickets are rejected on the host instead of falling back to Stack-only auth. Host capability flagsworkspace.move.v1,workspace.group_actions.v1, andworkspace.create_in_group.v1are ANDed with that ticket policy for UI and per-rowactionCapabilities.Create workspace is refactored into
createWorkspaceRequestwith single-flight coalescing (same group only; different group returns busy), no local create when disconnected or for in-group without connection, and create failures no longer set global pairingconnectionError.Reviewed by Cursor Bugbot for commit 8f3a563. Bugbot is set up for automated code reviews on this repo. Configure here.