Repository navigation
Fix wide-title tab drag hit testing - #215
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe AppKit tab-bar drop destination now ignores stale pasteboard types during passive events, accepts valid external dragged events, and requires active local drag ownership for other event types. Tests cover hit-testing, wide-tab insertion positions, and native drop-region availability. ChangesTab-bar drag handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Caution CodeRabbit couldn't update its existing comment. The review summary may be out of date. Error details |
![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
🧹 Nitpick comments (1)
Tests/BonsplitTests/BonsplitTests.swift (1)
2573-2584: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the tab hit region in the skip condition.
Line 2573 skips the test only when the trailing drop destination is absent. Line 2579 uses
XCTUnwrap(tabPoint), so the test fails instead of skipping when the SwiftUI runtime mounts the destination but not the tab hit region. Both views depend on the same runtime mounting behavior. Move thetabPointcheck into the skip condition to keep the failure mode consistent.💚 Proposed change
- guard dropDestination(at: trailingEmptyPoint) != nil else { + guard dropDestination(at: trailingEmptyPoint) != nil, tabPoint != nil else { throw XCTSkip( - "This SwiftUI runtime did not mount the native tab-bar drop destination" + "This SwiftUI runtime did not mount the native tab-bar drop destination or the pane tab hit region" ) }🤖 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 `@Tests/BonsplitTests/BonsplitTests.swift` around lines 2573 - 2584, Update the setup guard around dropDestination(at:) to also require a non-nil tabPoint, skipping the test when either the trailing drop destination or tab hit region is not mounted. Remove the later XCTUnwrap(tabPoint) and continue using the safely unwrapped tab point for coordinate conversion and registry validation.
🤖 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/Bonsplit/Internal/Views/TabBarDropDestinationNSView.swift`:
- Around line 29-41: Update hitTest(_:) in the tab-bar drop destination so it
returns self only when an active drag session is present, requiring
handler?.hasLiveTabDrag to be true in addition to the existing
shouldCaptureHitTest and operation checks. Ensure stale drag pasteboard types
cannot capture mouseMoved or cursorUpdate when no live tab drag exists.
---
Nitpick comments:
In `@Tests/BonsplitTests/BonsplitTests.swift`:
- Around line 2573-2584: Update the setup guard around dropDestination(at:) to
also require a non-nil tabPoint, skipping the test when either the trailing drop
destination or tab hit region is not mounted. Remove the later
XCTUnwrap(tabPoint) and continue using the safely unwrapped tab point for
coordinate conversion and registry validation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a25cbf8-3fb9-46f6-b3fe-ca9d13678dc8
📒 Files selected for processing (11)
Sources/Bonsplit/Internal/Views/TabBarDropDestinationNSView.swiftSources/Bonsplit/Internal/Views/TabBarDropDestinationView.swiftSources/Bonsplit/Internal/Views/TabBarDropHandler.swiftSources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Internal/Views/TabDropIndexResolver.swiftSources/Bonsplit/Public/BonsplitController.swiftTests/BonsplitTests/BonsplitTests.swiftTests/BonsplitTests/TabBarDropHandlerTests.swiftTests/BonsplitTests/TabDropDelegateSamePaneFallbackTests.swiftTests/BonsplitTests/TabDropIndexResolverTests.swiftTests/BonsplitTests/TabWideTitleDragTests.swift
💤 Files with no reviewable changes (1)
- Tests/BonsplitTests/TabDropDelegateSamePaneFallbackTests.swift
This was referenced Aug 12, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/BonsplitTests/TabWideTitleDragTests.swift (1)
20-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd probes for the final tab midpoint.
The final tab midpoint is x=1070. The current probes do not test the interval from x=936 to x=1070. A resolver that returns index 4 too early can pass this test. Add probes immediately before and at x=1070.
Proposed test addition
(875.9, 2), (936, 3), + (1_069.9, 3), + (1_070, 4), (1_200, 4),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/BonsplitTests/TabWideTitleDragTests.swift` around lines 20 - 30, Extend the probes array in the tab drag test with cases immediately before the final tab midpoint and at x=1070, expecting the index before the midpoint and the final index respectively.
🤖 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.
Nitpick comments:
In `@Tests/BonsplitTests/TabWideTitleDragTests.swift`:
- Around line 20-30: Extend the probes array in the tab drag test with cases
immediately before the final tab midpoint and at x=1070, expecting the index
before the midpoint and the final index respectively.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ea34bad-1e3d-4614-a598-9861ef08eea4
📒 Files selected for processing (3)
Sources/Bonsplit/Internal/Views/TabBarDropDestinationNSView.swiftTests/BonsplitTests/BonsplitTests.swiftTests/BonsplitTests/TabWideTitleDragTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/Bonsplit/Internal/Views/TabBarDropDestinationNSView.swift
- Tests/BonsplitTests/BonsplitTests.swift
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.
Summary
Regression provenance
The regression reported by manaflow-ai/cmux#10033 was introduced by bonsplit PR #197, specifically commit 4b5b563 (vendored by cmux commit manaflow-ai/cmux@2141de5). That change moved same-pane ownership into per-tab SwiftUI drop delegates to prevent duplicate middle-tab reorders, but left pointer-to-insertion resolution distributed across tab bounds. Wide titles make the source tab's large drop region retain routing while the pointer crosses insertion gaps.
This ports the strip-wide midpoint resolver and native destination architecture from bonsplit PR #202 (commits 7d2aa11 and 4da56ab), while retaining the pinned branch's later drag lifecycle and focus fixes.
Regression coverage
The first commit adds wide-title midpoint tests and native drop-handler/destination behavior coverage. The second commit adds the centralized resolver and destination implementation.
Validation
CI should run the Bonsplit Swift package tests. No local Xcode build or XCUITest was run on the development Mac.
Summary by cubic
Fixes drag-and-drop hit testing for wide tab titles by replacing per-tab SwiftUI delegates with one AppKit drop destination and tightening passive hit-test gating. Restores same-/cross-pane moves, keeps the trailing indicator visible in full-width mode, and addresses cmux issue #10033.
Bug Fixes
TabBarDropDestinationNSView+TabBarDropHandler) for consistent hit testing across the strip and trailing chrome.TabDropIndexResolverto compute midpoint-based insertion indices, stabilizing routing with wide titles and translated/partially visible strips; routed same-/cross-pane moves through shared controller APIs and kept the trailing indicator visible.mouseMoved,cursorUpdate,leftMouseUp) only capture with a live local drag;leftMouseDraggedis always accepted for external Finder/cross-process drags to avoid stale pasteboard coverage.Refactors
BonsplitController.reorderTabreports only real reorders.Written for commit cb20b6a. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests