Repository navigation
iOS workspace list: divider cleanup, + long-press group create, principled drag-drop - #7671
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:
📝 WalkthroughWalkthroughThis PR removes visual divider elements from the workspace list UI in the iOS shell. The leading overlay divider on indented workspace rows is removed, and the drawn end-of-group marks in the group footer row are replaced with an invisible spacer, while existing insets and accessibility behavior remain unchanged. ChangesRemove list divider marks
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 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 SummaryFour workspace-list improvements: indent-guide chrome removed, long-press
Confidence Score: 5/5Safe to merge; the group-create RPC is capability-gated and ticket-authed identically to existing Mac-scoped mutations, and the pipelining rewrite is backed by 109 tests including a host-order parity oracle. All new types are pure value-type Sendable structs with no actor isolation concerns. The reconciler and policy logic are well-tested against the host simulator. The three nits (O(n²) scan in MobileWorkspaceOptimisticOrder.swift — the Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI as WorkspaceListView
participant Rec as OptimisticOrderReconciler
participant Chain as Task Chain
participant Shell as MobileShellComposite
participant RPC as MobileCoreRPCClient
participant Mac as MobileHostService
UI->>Rec: Build Reconciler with optimisticOrder and pendingBases
UI->>Chain: pendingWorkspaceMoveTask chained on previousMove
Chain->>RPC: workspace.move with attach ticket
RPC->>Mac: ticketAuthorizationResultIfNeeded check
Mac-->>Chain: result Bool
Chain->>UI: pendingWorkspaceMoveCount decrement
UI->>Rec: reconciling on onChange
alt authoritative matches optimisticOrder
Rec-->>UI: clear optimism fulfilled
else authoritative matches a pendingBase
Rec-->>UI: keep optimisticOrder prune older bases
else no match supersede or failure
Rec-->>UI: clear bump epoch detach chain
end
Note over UI,Mac: Group create flow
UI->>Shell: createWorkspaceGroup with optional title
Shell->>Shell: check workspace.group_create.v1 capability
Shell->>RPC: workspace.group.create
RPC->>Mac: requiresCurrentAttachTicket check
Mac-->>Shell: Result success or failure
Shell-->>UI: handleWorkspaceActionResult createWorkspaceGroup
%%{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 UI as WorkspaceListView
participant Rec as OptimisticOrderReconciler
participant Chain as Task Chain
participant Shell as MobileShellComposite
participant RPC as MobileCoreRPCClient
participant Mac as MobileHostService
UI->>Rec: Build Reconciler with optimisticOrder and pendingBases
UI->>Chain: pendingWorkspaceMoveTask chained on previousMove
Chain->>RPC: workspace.move with attach ticket
RPC->>Mac: ticketAuthorizationResultIfNeeded check
Mac-->>Chain: result Bool
Chain->>UI: pendingWorkspaceMoveCount decrement
UI->>Rec: reconciling on onChange
alt authoritative matches optimisticOrder
Rec-->>UI: clear optimism fulfilled
else authoritative matches a pendingBase
Rec-->>UI: keep optimisticOrder prune older bases
else no match supersede or failure
Rec-->>UI: clear bump epoch detach chain
end
Note over UI,Mac: Group create flow
UI->>Shell: createWorkspaceGroup with optional title
Shell->>Shell: check workspace.group_create.v1 capability
Shell->>RPC: workspace.group.create
RPC->>Mac: requiresCurrentAttachTicket check
Mac-->>Shell: Result success or failure
Shell-->>UI: handleWorkspaceActionResult createWorkspaceGroup
Reviews (20): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
| splitDividerHex: splitDividerHex | ||
| ) |
There was a problem hiding this comment.
Configured Divider Color Can Be Ignored
When paneBorderColorHex is set, this call now leaves borderHex on the derived separator color and puts the configured value only in splitDividerHex. If the vendored bonsplit renderer still reads borderHex for split dividers on any path, the user setting is silently ignored and split panes render with the default separator color instead of the configured color.
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 `@vendor/bonsplit`:
- Line 1: The bonsplit submodule pointer is being updated to a SHA that is only
reachable from origin/feat-split-divider-color, so update the bonsplit submodule
commit first by landing it on origin/main and then bump the parent repo pointer.
Make sure the submodule update references the bonsplit submodule commit/checkout
flow rather than directly pinning the parent to the orphaned SHA, so the new
submodule commit is reachable from origin/main before the pointer change.
🪄 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: 8a5ded02-7bdc-4bf2-b930-dcddb72a0c82
📒 Files selected for processing (2)
Sources/Workspace.swiftvendor/bonsplit
| @@ -1 +1 @@ | |||
| Subproject commit fe53e04c5ec195f5a11f31374368c674769d2630 | |||
| Subproject commit 82260c92a9ca4025f7c288036a6bcb685e67c11d | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify bonsplit submodule commit contains splitDividerHex and is pushed to remote main.
BONSPLIT_DIR="vendor/bonsplit"
NEW_COMMIT="82260c92a9ca4025f7c288036a6bcb685e67c11d"
# Check that splitDividerHex exists in the ChromeColors type
echo "=== Searching for splitDividerHex in bonsplit sources ==="
rg -n 'splitDividerHex' "$BONSPLIT_DIR" --type=swift || echo "WARNING: splitDividerHex not found in bonsplit sources"
# Check that the new commit is an ancestor of origin/main (i.e., pushed to remote)
echo ""
echo "=== Checking remote ancestry ==="
cd "$BONSPLIT_DIR" && git fetch origin 2>/dev/null
if git merge-base --is-ancestor "$NEW_COMMIT" origin/main 2>/dev/null; then
echo "OK: $NEW_COMMIT is on origin/main"
else
echo "WARNING: $NEW_COMMIT is NOT on origin/main — the pointer may be orphaned"
fi
# Show the commit message for context
echo ""
echo "=== Commit details ==="
git log -1 --oneline "$NEW_COMMIT" 2>/dev/null || echo "Commit not found locally"Repository: manaflow-ai/cmux
Length of output: 499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SUBMODULE=vendor/bonsplit
COMMIT=82260c92a9ca4025f7c288036a6bcb685e67c11d
echo "== Submodule HEAD =="
git -C "$SUBMODULE" rev-parse HEAD
git -C "$SUBMODULE" status --short
echo
echo "== Commit summary =="
git -C "$SUBMODULE" show --stat --oneline --decorate=short --no-renames --summary "$COMMIT" | sed -n '1,120p'
echo
echo "== Tree search for splitDividerHex at commit =="
git -C "$SUBMODULE" grep -n 'splitDividerHex' "$COMMIT" -- || true
echo
echo "== Remote refs containing commit =="
git -C "$SUBMODULE" for-each-ref --format='%(refname:short) %(objectname)' refs/remotes | sed -n '1,120p'
git -C "$SUBMODULE" branch -r --contains "$COMMIT" || true
echo
echo "== origin/main ancestry check =="
git -C "$SUBMODULE" fetch origin >/dev/null 2>&1 || true
if git -C "$SUBMODULE" merge-base --is-ancestor "$COMMIT" origin/main; then
echo "ancestor-of-origin/main"
else
echo "not-ancestor-of-origin/main"
fiRepository: manaflow-ai/cmux
Length of output: 9764
Move the bonsplit submodule commit onto origin/main before bumping this pointer. The new SHA is only reachable from origin/feat-split-divider-color, so the parent repo can end up pinned to an orphaned submodule commit.
🤖 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 `@vendor/bonsplit` at line 1, The bonsplit submodule pointer is being updated
to a SHA that is only reachable from origin/feat-split-divider-color, so update
the bonsplit submodule commit first by landing it on origin/main and then bump
the parent repo pointer. Make sure the submodule update references the bonsplit
submodule commit/checkout flow rather than directly pinning the parent to the
orphaned SHA, so the new submodule commit is reachable from origin/main before
the pointer change.
| @@ -1 +1 @@ | |||
| Subproject commit fe53e04c5ec195f5a11f31374368c674769d2630 | |||
| Subproject commit 82260c92a9ca4025f7c288036a6bcb685e67c11d | |||
There was a problem hiding this comment.
The parent app now depends on this bonsplit revision for ChromeColors.splitDividerHex, but the pinned submodule object is not available from the configured vendor/bonsplit remote in this checkout. A clean clone that runs git submodule update --init vendor/bonsplit can fail before the app builds, leaving import Bonsplit and the new splitDividerHex API unresolved. Please point the submodule at a commit that is fetchable from https://github.com/manaflow-ai/bonsplit.git and contains the divider-color changes.
The grouped-workspace rows drew a 1px vertical indent line at their leading edge, and WorkspaceGroupFooterRow drew an L-shaped end-of-group corner mark. Both are the dividers the user reported. The footer row stays as an invisible 12pt spacer so its drag-drop target (drop above = into group, below = top level) and accessibility element keep working. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4070cfe to
43f1fb0
Compare
Tap keeps creating a workspace; press-and-hold now opens a menu with New Workspace and New Workspace Group. Group creation from mobile is a new RPC: the Mac host advertises workspace.group_create.v1 and handles workspace.group.create (trimmed optional title; blank falls back to the desktop's localized auto-name; anchor workspace created without stealing Mac focus, matching mobile workspace.create). The client gates the menu item and the send path on the capability plus mac-scoped mutation authorization, using the same mutation plumbing as the other group actions; old clients and workspace.group.action are unchanged. Over-cap host files (TerminalController.swift, MobileHostService.swift) changed at net-zero lines via extended case labels. New strings have en+ja entries in the iOS app catalog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The invisible 12pt group-footer drop-target row was inflated to the iOS List default minimum row height (~44pt), leaving a large blank band after every group. Override defaultMinListRowHeight on the list so the footer renders at its actual 12pt; all real rows exceed the floor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
All drag rules now live in one policy point (MobileWorkspaceMovePolicy), mirroring the macOS sidebar resolver/planner semantics: group-header drops normalize to whole-group boundaries (never interleave another group, matching the host's mobileWorkspaceMoveTopLevelBeforeID normalization), pinned tiers clamp at top level and within groups (anchor first, pinned members, then unpinned), anchors cannot leave or reorder within their group, footer/collapsed/end slots and stale-ID cases are explicit rules. The optimistic applier uses the same policy, so the predicted order always equals the host's post-move order; a test-target-independent host-order simulator (reimplemented from the host code, not shared with the policy) sweeps every source row x destination across fixtures to prove parity. Haptic churn root cause: a successful move RPC cleared the optimistic order before the authoritative snapshot arrived, snapping rows back under the active UIKit reorder interaction (continuous retarget haptics); the pending flag also toggled moveDisabled mid-gesture. Moves now reconcile on snapshot (keep optimism until the authoritative order matches or supersedes; roll back only on a failed move) via a pure tested reconciler, and reorder stays enabled during pending moves with pipelined drops based on the prior optimistic order. 109 CmuxMobileShellModel tests pass; CmuxMobileShellUI iOS build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding: with reorder enabled during pending moves, two in-flight move RPCs had no ordering guarantee, so a rapid second drag could reach the Mac first and the authoritative snapshot would diverge from the predicted order, dropping pipelined optimism mid-gesture. Each move now chains on the previous send (single tail Task) so the host applies moves in UI order, and the pending flag is a counter so overlapping moves cannot clear each other's pending state early. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Policy check flagged the reconciler's static methods as static-as-namespace; the snapshots are now constructor-injected state and the order-signature factory lives on MobileWorkspaceOrderSignature, the type that owns it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
check-package-resolved-policy.py rejects this location; it was created by an xcodebuild verification run, not a resolution change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review P1s: the serial send chain had no size bound (a slow or offline Mac let drags enqueue unbounded work), and a chained move could not see its predecessor's outcome, so an intent computed against a rejected prediction was still sent and produced a wrong final order. The chain now carries acceptance (Task<Bool, Never>): a successor aborts without sending when any predecessor failed (the failure rollback already restored the authoritative order), and reorder pauses once 8 moves are pending, a depth normal round-trips never reach. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dogfood follow-up: the flap pile lived in stacks of header-only groups, whose 12pt footers created adjacent razor-thin reorder targets, but the end-of-group slot is still wanted for populated groups so both boundary targets are directly draggable. items() now emits the slot only for expanded groups with at least one member row (16pt, invisible, per-group unique even for non-contiguous runs); dropping before it joins the group at its end (members and external workspaces alike) and dropping after it lands at root. Empty and collapsed groups stay slot-free — their below-header gap already joins/roots — so the header stacks that flapped have no thin targets at all. The round-5 membership-inference rule remains as the documented fallback for boundaries without a slot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review P1: a failed move left its completed false-valued task as the chain tail, so every later drag chained on it and silently aborted forever (and left fresh optimistic state visible). The failure handler now detaches the tail: queued dependents still hold their captured reference and drain by aborting, while drags started after the failure chain on nothing and proceed normally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Judge findings: the identity-drop test's source index selected the new end-of-group slot (rejected by the footer-source guard before the identity check ran), leaving identity rejection untested; it now drags a real row into its own gap, with a separately named footer-source rejection test and restored collapsed-group no-slot coverage. The items() doc bullet now states the actual end-of-group slot emission rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two review findings on pipelined optimism. First, the reconciler only remembered the latest move's base, so a three-deep drag chain snapped through intermediate orders when the first refresh landed; every move now records its source order and a snapshot matching any pending intermediate keeps the displayed prediction (pruning strictly older bases), the displayed order drains the chain, an unmatched order supersedes, and failure clears everything. Second, held optimism froze full row snapshots; optimistic state now stores only workspace IDs plus intended membership and every render overlays the current authoritative content, so titles, unread state, capabilities, collapse/expand, deletions, and arrivals stay live mid-chain. 120 model tests including the three-move chain, out-of-order supersede, mid-chain failure, and live-content scenarios. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review findings: the optimistic entries dropped pin state, so a Mac-side workspace or group pin change mid-chain could clamp the move to a no-op while the stale base still matched, pinning an invalid order on screen; predictions now capture workspace and group pin state and any change supersedes optimism. The move policy also rebuilt its group lookup dictionaries on every access inside per-workspace loops; they are now built once at init. The anchor-only-group drop-slot finding is consciously rejected: a synthetic slot there recreates the flapping thin-row pile in header stacks that started this work, every other row still joins via the below-header gap, and only the row directly beneath an empty group needs a two-step move. 122 model tests including workspace- and group-pin supersede cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review triage. A supersede cleared optimism but queued dependent RPCs still sent intents computed against the overruled predictions; the chain now carries an epoch that a supersede or failure bumps, so not-yet-sent moves abort. The list-wide reduced minimum row height (needed for the invisible end-of-group spacer) let group header rows shrink below the 44pt tap target; header rows now pin an explicit minimum. The pipeline cap drops from 8 to 3 because every queued move currently costs a full workspace refresh on reply; applying the mutation's returned list instead of re-aggregating is noted as a follow-up. Consciously accepted: a workspace arriving mid-chain may transiently render outside its pin tier until the chain drains or a supersede corrects it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Native onMove is index-only, so the group/root boundary could only be encoded as invisible rows: too thin to hit (the root target was practically unreachable under a finger) or too thick not to read as dead space. The gesture layer is now onDrag/onDrop with a pure MobileWorkspaceDropResolver porting the Mac resolver contract: hit row plus top/middle/bottom bands decide the slot, a horizontal lane at a group's end boundary picks group (right half) or root (left half), dropping on a header's middle band appends into that group including anchor-only groups, and group drags are offered whole-group boundaries only. A single overlay draws the policy-clamped insertion line (indented for in-group targets) or a header highlight, so the destination is visible before the drop; haptics fire once per semantic target change. All synthetic footer rows and the min-row-height override are deleted; intents flow into the unchanged policy, optimistic-chain, and epoch pipeline through one shared commit path. 131 model tests (11 new resolver rules; parity oracle sweep intact). Drag feel, autoscroll cadence, and context-menu coexistence need device dogfood; iPad pointer and VoiceOver reorder are follow-ups. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This reverts commit 78cbf1f.
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 3281bc9. Configure here.
| // reference and drain by aborting above. | ||
| pendingWorkspaceMoveTask = nil | ||
| } | ||
| return accepted |
There was a problem hiding this comment.
In-flight move ignores epoch
Medium Severity
Pipelined drag tasks only compare workspaceMoveEpoch before awaiting the move RPC. When syncOptimisticWorkspaceOrder clears optimism and bumps the epoch (matching snapshot or supersede), tasks already awaiting moveWorkspace still complete and can send a move computed against an invalidated prediction, so the Mac may apply a stale reorder after the list already converged.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3281bc9. Configure here.


Four iOS workspace-list changes from one dogfood session.
Indent guides removed. Grouped rows drew a 1px leading line and
WorkspaceGroupFooterRowdrew an end-of-group corner mark; both removed. The footer stays as an invisible 12pt drop target (drag semantics + accessibility unchanged), anddefaultMinListRowHeightis overridden so the iOS List minimum (~44pt) no longer inflates it into a dead band after each group.Long-press "+" creates a workspace group. Tap keeps creating a workspace; hold opens a menu with New Workspace / New Workspace Group. New capability-gated RPC
workspace.group.create(host advertisesworkspace.group_create.v1; trimmed optional title, blank falls back to the desktop's localized auto-name; anchor created without stealing Mac focus). Old clients byte-identical; over-cap host files edited at net-zero lines; en+ja strings in the iOS catalog.Principled drag-drop policy (Mac parity). All drag rules now live in
MobileWorkspaceMovePolicy, mirroring the macOS sidebar resolver/planner: group-header drops normalize to whole-group boundaries (groups can never nest or interleave, matching the host'smobileWorkspaceMoveTopLevelBeforeIDnormalization), pinned tiers clamp at top level and within groups (anchor first, pinned members, then unpinned), anchors cannot leave or reorder within their group, and footer/collapsed/end/stale-ID slots are explicit rules. A test-target-independent host-order simulator (reimplemented from the host code, with line citations) sweeps every source row × destination across fixtures proving predicted order == host order.Continuous drag haptics fixed. Root cause: a successful move RPC cleared the optimistic order before the authoritative snapshot arrived, snapping rows back under the active reorder interaction (continuous UIKit retarget haptics); the pending flag also toggled
moveDisabledmid-gesture. Moves now reconcile on snapshot via a pure tested reconciler (keep optimism until the authoritative order matches or supersedes; roll back only on failure), reorder stays enabled while pending, and pipelined sends are chained so the Mac applies rapid drags in UI order.109 CmuxMobileShellModel tests (incl. the independent parity oracle); structured review + policy checks clean.
🤖 Generated with Claude Code
Note
Medium Risk
Touches Mac-scoped RPC auth, workspace ordering semantics, and pipelined optimistic UI state; regressions could mis-order workspaces or reject valid group creates, but behavior is heavily covered by new parity and shell tests.
Overview
Adds creating workspace groups from iOS via capability
workspace.group_create.v1and RPCworkspace.group.create(optional trimmed title, Mac auto-name when blank), wired through shell/RPC auth like other Mac-scoped mutations; the + control becomes a menu (tap = new workspace, menu item = new group) with failure toasts and localization.Reorder/drag is centralized in
MobileWorkspaceMovePolicyso list drags normalize like the Mac sidebar (group boundaries, pinned tiers, anchor rules, end-of-group slots). List rendering now emits group footers only after expanded groups with visible member rows; the footer is an invisible drop slot (guides removed). Optimistic ordering usesMobileWorkspaceOptimisticOrderReconcilerso the list does not snap back before the host snapshot; moves can pipeline (chained RPCs, epoch abort on failure/supersede, cap of 3) while reorder stays enabled during pending moves;moveWorkspacereports success/failure to the UI layer.Mac host advertises the capability, requires attach tickets for
workspace.group.create, and implementsv2MobileWorkspaceGroupCreatereturning an updated workspace list.Reviewed by Cursor Bugbot for commit 3281bc9. Bugbot is set up for automated code reviews on this repo. Configure here.