Repository navigation
Fix pane tab drags silently never starting (#10033) - #10038
Conversation
Tab drags silently never started when the press arrived with clickCount >= 2 — the common click-to-select-then-immediately-drag flow. Bonsplit PR: manaflow-ai/bonsplit#217 Fixes #10033 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Ends cancelled tab drags immediately instead of after AppKit's snap-back animation, so drag state is revoked before the next press. Bonsplit PR: manaflow-ai/bonsplit#218 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAccepted Bonsplit pane and workspace drops now finish originating tab-drag transfers with the drag pasteboard. Pane transfer lifecycle tests verify native drag and registry cleanup. The vendored Bonsplit revision also updates. ChangesBonsplit tab-drag lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DropTarget
participant PaneTransferDropRouter
participant PaneTransferSourceResolver
participant TabDragTransferRegistry
DropTarget->>PaneTransferDropRouter: perform accepted drop with pasteboard
PaneTransferDropRouter->>PaneTransferSourceResolver: finishAcceptedDrop(source, id, pasteboard)
PaneTransferSourceResolver->>TabDragTransferRegistry: finish originating drag
TabDragTransferRegistry-->>DropTarget: remove transfer registration and end native drag
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches 💡 1📝 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 |
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: Add regression coverage for modifier handling in
TabBarDragAndHoverView.trackTabMouseDown by testing that Control-clicks are
rejected, and add a sidebar workspace reorder test alongside the existing
wide-tab and same-/cross-pane reorder cases. Reuse the current Bonsplit test
helpers and run the full Bonsplit test suite.
🪄 Autofix
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 Plus
Run ID: 4569779e-8418-4390-995f-7c318871f357
📒 Files selected for processing (1)
vendor/bonsplit
| @@ -1 +1 @@ | |||
| Subproject commit 2270bacdf94672384097f6b94864111a54bcf548 | |||
| Subproject commit d689ecb1a299ba4ce21b120afeb654786fce3293 | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
old=2270bacdf94672384097f6b94864111a54bcf548
new=d689ecb1a299ba4ce21b120afeb654786fce3293
git -C vendor/bonsplit cat-file -e "${old}^{commit}"
git -C vendor/bonsplit cat-file -e "${new}^{commit}"
git -C vendor/bonsplit diff --check "$old" "$new"
git -C vendor/bonsplit diff --stat "$old" "$new"
git -C vendor/bonsplit diff "$old" "$new"
git -C vendor/bonsplit grep -n -E \
'trackTabMouseDown|TabBarBackgroundNSView|hit.?test|transfer|snap.?back|Control|workspace.*reorder' \
"$new" -- '*.swift' || trueRepository: manaflow-ai/cmux
Length of output: 50373
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo '--- integration files ---'
fd -t f 'BonsplitTabBarPassThrough.swift|BrowserWindowPortal.swift' . | sort
for f in $(fd -t f 'BonsplitTabBarPassThrough.swift|BrowserWindowPortal.swift' .); do
echo "--- $f ---"
rg -n -C 8 'BonsplitTabBarPassThrough|TabBarBackgroundNSView|hitTest|hitRegion|634|644|tab.?bar|view' "$f" | head -240
done
echo '--- focused changed source symbols ---'
git -C vendor/bonsplit grep -n -E \
'trackTabMouseDown|handleTabMouseDragged|beginNativeTabDrag|nativeTabDragSessionDidEnd|animatesToStartingPositionsOnCancelOrFail|finishDrag|shouldCaptureHitTest|insertionIndex|isNoopSamePaneTarget' \
d689ecb1a299ba4ce21b120afeb654786fce3293 -- '*.swift' || true
echo '--- focused tests ---'
git -C vendor/bonsplit grep -n -E \
'clickCount|control|Control|cancel|snap|animation|wide|Wide|sidebar|workspace|reorder|hitTest|hit.?region|structure|TabBarBackground' \
d689ecb1a299ba4ce21b120afeb654786fce3293 -- 'Tests/**/*.swift' || trueRepository: manaflow-ai/cmux
Length of output: 12750
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo '--- files ---'
fd -t f 'BonsplitTabBarPassThrough.swift|BrowserWindowPortal.swift' . | sort
echo '--- drag implementation ---'
git -C vendor/bonsplit show --format= --no-ext-diff d689ecb1a299ba4ce21b120afeb654786fce3293:Sources/Bonsplit/Internal/Views/TabBarView.swift |
sed -n '2150,2315p'
echo '--- focused tests and test names ---'
git -C vendor/bonsplit ls-tree -r --name-only d689ecb1a299ba4ce21b120afeb654786fce3293 Tests/BonsplitTests |
grep -E 'TabBarDragEventRoutingTests|TabBarDropHandlerTests|TabWideTitleDragTests|workspace|Sidebar|Reorder'
for f in \
Tests/BonsplitTests/TabBarDragEventRoutingTests.swift \
Tests/BonsplitTests/TabBarDropHandlerTests.swift \
Tests/BonsplitTests/TabWideTitleDragTests.swift; do
echo "--- $f ---"
git -C vendor/bonsplit show --format= --no-ext-diff d689ecb1a299ba4ce21b120afeb654786fce3293:"$f" |
rg -n -C 5 'func test|clickCount|control|cancel|snap|animation|wide|sidebar|workspace|reorder|hit' | head -360
doneRepository: manaflow-ai/cmux
Length of output: 22509
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
new=d689ecb1a299ba4ce21b120afeb654786fce3293
old=2270bacdf94672384097f6b94864111a54bcf548
echo '--- drag routing tests ---'
git -C vendor/bonsplit show --format= "$new":Tests/BonsplitTests/TabBarDragEventRoutingTests.swift |
sed -n '1,230p'
echo '--- wide-title tests ---'
git -C vendor/bonsplit show --format= "$new":Tests/BonsplitTests/TabWideTitleDragTests.swift |
sed -n '1,180p'
echo '--- workspace/sidebar reorder references ---'
git -C vendor/bonsplit grep -n -i -E \
'workspace|sidebar|reorder' "$new" -- 'Tests/**/*.swift' 'Sources/**/*.swift' |
head -240 || true
echo '--- tab-bar hierarchy and hit-region changes ---'
git -C vendor/bonsplit diff --unified=35 "$old" "$new" -- \
Sources/Bonsplit/Internal/Views/TabBarView.swift |
rg -n -C 35 \
'TabBarDragAndHoverView|TabBarBackgroundNSView|overlay|background|hitTest|onBeginTabDrag|TabBarDropDestinationView|BonsplitTabItemHitRegionProviding' |
head -520
echo '--- package test definitions ---'
git -C vendor/bonsplit ls-tree -r --name-only "$new" |
grep -E 'Package.swift|Tests/.+\\.swift$' |
head -120Repository: manaflow-ai/cmux
Length of output: 47730
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
new=d689ecb1a299ba4ce21b120afeb654786fce3293
old=2270bacdf94672384097f6b94864111a54bcf548
echo '--- explicit Control-click and sidebar/workspace tests ---'
git -C vendor/bonsplit grep -n -i -E \
'control.?click|modifierFlags|workspace|sidebar.*reorder|reorder.*sidebar|sidebar.*workspace|workspace.*sidebar' \
"$new" -- 'Tests/**/*.swift' 'Sources/**/*.swift' || true
echo '--- relevant TabBarView body before and after ---'
for rev in "$old" "$new"; do
echo "--- $rev ---"
git -C vendor/bonsplit show --format= "$rev":Sources/Bonsplit/Internal/Views/TabBarView.swift |
sed -n '970,1185p'
done
echo '--- direct hierarchy diff ---'
git -C vendor/bonsplit diff --unified=12 "$old" "$new" -- \
Sources/Bonsplit/Internal/Views/TabBarView.swift |
sed -n '1,420p'Repository: manaflow-ai/cmux
Length of output: 46976
Add Control-click and sidebar workspace reorder regression tests.
TabBarDragAndHoverView.trackTabMouseDown rejects Control-clicks at vendor/bonsplit/Sources/Bonsplit/Internal/Views/TabBarView.swift:2186, but no modifier-flag test covers this path. The tests cover wide-tab geometry and same-/cross-pane reorder, but not sidebar workspace reorder. Add both cases and run the Bonsplit test suite.
🤖 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, Add regression coverage for modifier handling in
TabBarDragAndHoverView.trackTabMouseDown by testing that Control-clicks are
rejected, and add a sidebar workspace reorder test alongside the existing
wide-tab and same-/cross-pane reorder cases. Reuse the current Bonsplit test
helpers and run the full Bonsplit test suite.
Advance Bonsplit to the restored legacy pasteboard payload (manaflow-ai/bonsplit#219): pane/browser drop overlays decode tab.id/kind/sourcePaneId/sourceProcessId again, so the blue drop zones render and drops route. Also share one tab-drag capability registry between workspaces and the Dock so bonsplit tab-strip destinations can resolve drags that began in another controller, matching 0.64.22 where any destination could decode the drag. Fixes #10033 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/PaneDropRoutingSupport.swift`:
- Around line 12-15: Replace the namespace-only BonsplitTabDragSharing access
around finishAcceptedDrop with an explicitly owned, injectable drag coordinator
or registry instance. Thread that single instance from the app-owned drag
coordinator through workspace and Dock controllers into drop targets, and invoke
it from finishAcceptedDrop instead of process-wide mutable state. Preserve the
single authoritative registry while making ownership constructable and testable.
In `@Sources/Sidebar/SidebarBonsplitTabWorkspaceDropOverlay.swift`:
- Line 222: Move the BonsplitTabDragSharing.finishAcceptedDrop call out of the
immediate deferred-drop path and invoke it only after
performPendingDropIfPossible() confirms moved == true. Ensure rejected or
unsuccessful pending drops leave the shared drag unfinished, or add an explicit
failure/cancellation path if queuing remains the acceptance boundary.
🪄 Autofix
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 Plus
Run ID: f879e349-7942-4351-8130-6e31e23df94d
📒 Files selected for processing (7)
Sources/BrowserPaneDropTargetView.swiftSources/ContentView+MoveTabToNewWorkspace.swiftSources/PaneDropRoutingSupport.swiftSources/Sidebar/SidebarBonsplitTabDropDelegate.swiftSources/Sidebar/SidebarBonsplitTabWorkspaceDropOverlay.swiftSources/TerminalPaneDropTargetView.swiftvendor/bonsplit
| point: localPoint(sender), | ||
| transfer: transfer | ||
| ) | ||
| BonsplitTabDragSharing.finishAcceptedDrop(from: sender.draggingPasteboard) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not finalize a deferred drop before the move succeeds.
Line 222 calls finishAcceptedDrop immediately after queuing pendingDrop. performPendingDropIfPossible() can later reject the action or return moved == false, so the originating drag finishes even though no tab moved. Complete the drag only after the deferred move returns true. If queuing is the intended acceptance boundary, add an explicit failure or cancellation path instead of using success completion unconditionally.
This follows the PR contract that failed drop paths must not finalize the shared drag.
🤖 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/Sidebar/SidebarBonsplitTabWorkspaceDropOverlay.swift` at line 222,
Move the BonsplitTabDragSharing.finishAcceptedDrop call out of the immediate
deferred-drop path and invoke it only after performPendingDropIfPossible()
confirms moved == true. Ensure rejected or unsuccessful pending drops leave the
shared drag unfinished, or add an explicit failure/cancellation path if queuing
remains the acceptance boundary.
…wide-tabs # Conflicts: # Sources/BrowserPaneDropTargetView.swift # Sources/DockSplitStore.swift # Sources/TerminalPaneDropTargetView.swift # Sources/Workspace.swift # vendor/bonsplit
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
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: Restore the missing drag regression coverage in the Bonsplit host
tests: update TabBarDragEventRoutingTests and its event helper to exercise
Control-click rejection in TabBarView.trackTabMouseDown, including the
appropriate modifier flags, while preserving the existing double-click arming
case. Also add or retain a sidebar/workspace reorder regression test in the
appropriate test suite.
🪄 Autofix
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 Plus
Run ID: 640f3cd8-2f95-4dca-8e3e-cdb5c1a1ce41
📒 Files selected for processing (1)
vendor/bonsplit
| @@ -1 +1 @@ | |||
| Subproject commit 5b61b184336756deb0c435e1f79dd1c839eb0281 | |||
| Subproject commit 18f3f8bc2ac3019d0f30b41586f4785950af6de7 | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Retain the missing drag regression coverage in this revision.
TabBarView.trackTabMouseDown rejects Control-clicks, but TabBarDragEventRoutingTests only covers double-click arming and its event helper uses empty modifier flags. The Bonsplit test tree also has no sidebar/workspace reorder regression case. Add the Control-click test and keep the sidebar/workspace reorder test in the appropriate host test suite. (raw.githubusercontent.com)
This repeats the unresolved test-coverage concern from the previous review.
#!/usr/bin/env bash
set -euo pipefail
rev=18f3f8bc2ac3019d0f30b41586f4785950af6de7
git -C vendor/bonsplit cat-file -e "${rev}^{commit}"
echo "--- drag-event modifier coverage ---"
git -C vendor/bonsplit show "${rev}:Tests/BonsplitTests/TabBarDragEventRoutingTests.swift" |
grep -n -E 'modifierFlags|clickCount|control' || true
echo "--- sidebar/workspace test coverage ---"
git -C vendor/bonsplit grep -n -i -E 'sidebar|workspace|reorder' \
"${rev}" -- 'Tests/**/*.swift' || true🤖 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, Restore the missing drag regression coverage in
the Bonsplit host tests: update TabBarDragEventRoutingTests and its event helper
to exercise Control-click rejection in TabBarView.trackTabMouseDown, including
the appropriate modifier flags, while preserving the existing double-click
arming case. Also add or retain a sidebar/workspace reorder regression test in
the appropriate test suite.
Problem
Since the native tab drag rewrite (after 0.64.22), pane tab drags regressed twice over (#10033):
trackTabMouseDownonly armed onclickCount == 1; AppKit counts a press within the double-click interval near the same point asclickCount == 2, so click-to-select-then-drag never armed. Felt sporadic (pure timing).33969f2replaced thecom.splittabbar.tabtransferJSON payload with a bare capability token, but cmux's pane/browser drop overlays (PaneDragTransfer,BonsplitTabDragPayload,BrowserPaneDragTransfer) resolve drop targets by JSON-parsing that payload. They decoded nothing → drag image rendered but the blue drop zones never appeared and drops did nothing.Fixes
tab.id,tab.kind,sourcePaneId,sourceProcessId) with the capability token embedded; titles stay off the pasteboard, released registrations stay inert (regression test red→green).TabDragTransferRegistrybetween workspaces and the Dock so bonsplit tab-strip destinations resolve drags that began in another controller, matching 0.64.22.Horizontal insert-between-tabs positioning (the one post-0.64.22 behavior to keep) is untouched and covered by
TabDropIndexResolverTests/TabWideTitleDragTests.Validation
tab.dragStartfired,terminal.paneDrop.hitTest capture=1 hasTransfer=1captured, then decode returned nil → no zones, no drop.issue-10033-tab-drag(cloud reload).Fixes #10033
🤖 Generated with Claude Code
Summary by CodeRabbit