Repository navigation
Fix same-pane tab drag fallback reorder - #197
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe tab drop delegate resolves local transfers from active or encoded drag state. It handles same-pane reordering, cross-pane fallback requests, no-op suppression, hover indicators, and centralized cleanup. Tests cover reorder behavior and drop-zone ownership. ChangesTab drop delegate
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant TabBarView
participant TabDropDelegate
participant TabController
participant ExternalDropHandler
TabBarView->>TabDropDelegate: supply location and transfer payload
TabDropDelegate->>TabDropDelegate: resolve target and insertion index
TabDropDelegate->>TabController: reorder or move tab
TabDropDelegate->>ExternalDropHandler: create cross-pane fallback request
TabDropDelegate->>TabBarView: clear drag and hover state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 `@Sources/Bonsplit/Internal/Views/TabBarView.swift`:
- Around line 2024-2025: Separate the manual insertion destination from the
visual dropTargetIndex state in the manual drag handling around
manualDropTargetIndex and the static-target logic near performDrop. Preserve
valid same-pane manual indices, including sourceIndex and sourceIndex + 1, as
the destination while continuing to suppress their indicator during hover
rendering; ensure stale native end drops use that retained manual no-op
destination instead of staticTargetIndex. Add coverage for stale-end cases at
both manual no-op indices.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4d9fa8f-9ee5-4671-9ef1-9e41618fe3cd
📒 Files selected for processing (2)
Sources/Bonsplit/Internal/Views/TabBarView.swiftTests/BonsplitTests/TabDropDelegateSamePaneFallbackTests.swift
Comment on lines
2024
to
2025
| manualDropTargetIndex = targetIndex | ||
| dropTargetIndex = targetIndex |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve an explicit manual no-op target.
Line 2024 receives nil when manual tracking suppresses a target at the source index or the following index. Lines 3408-3414 also replace a retained manual no-op target with staticTargetIndex.
If a stale native end drop reaches performDrop before local drag state is cleared, the static target moves the tab even though the manual drop was a no-op.
Store the manual insertion index separately from the indicator state. Use a valid manual same-pane index as the destination, including a no-op index. Keep no-op indicator suppression in hover rendering. Add stale-end cases for manual targets at sourceIndex and sourceIndex + 1.
Also applies to: 3408-3414
🤖 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/Bonsplit/Internal/Views/TabBarView.swift` around lines 2024 - 2025,
Separate the manual insertion destination from the visual dropTargetIndex state
in the manual drag handling around manualDropTargetIndex and the static-target
logic near performDrop. Preserve valid same-pane manual indices, including
sourceIndex and sourceIndex + 1, as the destination while continuing to suppress
their indicator during hover rendering; ensure stale native end drops use that
retained manual no-op destination instead of staticTargetIndex. Add coverage for
stale-end cases at both manual no-op indices.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Tests/BonsplitTests/TabDropDelegateSamePaneFallbackTests.swift (2)
58-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
onExternalTabDropin the harness is unused.No test in this file triggers a cross-pane drop, so the handler at Lines 72-77 never runs. Either add a cross-pane fallback test that uses it, or remove it from the harness.
🤖 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/TabDropDelegateSamePaneFallbackTests.swift` around lines 58 - 79, The makeHarness method defines an unused onExternalTabDrop handler because these tests never exercise cross-pane drops. Remove that handler setup from makeHarness, unless adding a test that actually triggers the cross-pane fallback is required; keep the existing harness configuration and tab setup unchanged.
6-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests do not cover the stale fallback drop path.
All three tests call the static helpers
TabDropDelegate.samePaneDropTargetandTabDropDelegate.performSamePaneReorderdirectly. None of them exercisesperformDrop,decodeTransfer, orperformSameProcessTransfer. The stale same-pane fallback behavior described in the PR objectives is therefore unverified, and the namestestEarlierTabSwiftUIDropUsesMiddleTargetFromDelegateandtestLaterTabSwiftUIDropUsesMiddleTargetFromDelegateclaim more coverage than the assertions provide.Add a case that applies a reorder, clears the controller drag state, and then invokes the same-pane fallback with an end
targetIndex. That case pins the behavior flagged inSources/Bonsplit/Internal/Views/TabBarView.swiftLines 3092-3102.The reorder expectations themselves are correct against
PaneState.moveTab.🤖 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/TabDropDelegateSamePaneFallbackTests.swift` around lines 6 - 56, Add a test covering the stale same-pane fallback path through the delegate’s drop flow rather than only calling samePaneDropTarget and performSamePaneReorder directly. Reorder a tab, clear the controller drag state, then invoke performDrop using a stale transfer and an end targetIndex, asserting the fallback succeeds and the final tab order matches PaneState.moveTab behavior. Update the existing SwiftUI test names if needed so they accurately describe their direct-helper coverage.
🤖 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/TabBarView.swift`:
- Around line 3092-3102: Update performSameProcessTransfer so same-pane
transfers encountered through the fallback path are ignored and return false
instead of calling performSamePaneReorder. Preserve the existing
allowTabReordering check only if needed for surrounding behavior, and leave
cross-pane transfer handling unchanged.
- Around line 2976-2980: Update the dropExited logic around
samePaneLocalDragSourceIndex so it clears dropTargetIndex only when this
delegate still owns the current target, preserving the existing targetIndex
comparison. Do not unconditionally clear the indicator merely because a
same-pane local drag exists; retain a newly assigned target when dropEntered ran
before dropExited.
- Around line 3129-3155: Update performSamePaneReorder so that after a same-pane
move changes the tab order, it restores the
BonsplitController.moveTab/reorderTab contract by selecting the moved tab,
focusing the pane, notifying the geometry change, and emitting didSelectTab for
the moved tab when the controller can identify it. Keep the existing no-op and
failed-source behavior unchanged.
In `@Tests/BonsplitTests/BonsplitTests.swift`:
- Around line 2449-2498: Strengthen the overlay regression check around
chromeDragZones(at:) by ensuring the candidate drag-zone set is non-empty before
evaluating overlap, so an empty set cannot make the test pass vacuously. Replace
the fixed tabPoint assumption with a point derived from the registered
rendered-tab hit region, or otherwise select a guaranteed point inside that
region, while preserving the existing assertion that no chrome drop destination
overlaps it.
---
Nitpick comments:
In `@Tests/BonsplitTests/TabDropDelegateSamePaneFallbackTests.swift`:
- Around line 58-79: The makeHarness method defines an unused onExternalTabDrop
handler because these tests never exercise cross-pane drops. Remove that handler
setup from makeHarness, unless adding a test that actually triggers the
cross-pane fallback is required; keep the existing harness configuration and tab
setup unchanged.
- Around line 6-56: Add a test covering the stale same-pane fallback path
through the delegate’s drop flow rather than only calling samePaneDropTarget and
performSamePaneReorder directly. Reorder a tab, clear the controller drag state,
then invoke performDrop using a stale transfer and an end targetIndex, asserting
the fallback succeeds and the final tab order matches PaneState.moveTab
behavior. Update the existing SwiftUI test names if needed so they accurately
describe their direct-helper coverage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c956299f-4e9b-4d92-bc92-42d2f489c0b2
📒 Files selected for processing (3)
Sources/Bonsplit/Internal/Views/TabBarView.swiftTests/BonsplitTests/BonsplitTests.swiftTests/BonsplitTests/TabDropDelegateSamePaneFallbackTests.swift
Comment on lines
+2449
to
2498
| let tabPoint = NSPoint(x: 90, y: 30) | ||
| let trailingEmptyPoint = NSPoint(x: 460, y: 30) | ||
| let registrationDeadline = Date().addingTimeInterval(0.5) | ||
| var dropDestinations: [NSView] = [] | ||
| var dragZoneViews: [TabBarDragZoneView.DragNSView] = [] | ||
|
|
||
| func dropDestination(at point: NSPoint) -> NSView? { | ||
| let pointInWindow = hostingView.convert(point, to: nil) | ||
| return dropDestinations.first { view in | ||
| view.convert(view.bounds, to: nil).contains(pointInWindow) | ||
| } | ||
| } | ||
| func chromeDragZones(at point: NSPoint) -> [TabBarDragZoneView.DragNSView] { | ||
| let pointInWindow = hostingView.convert(point, to: nil) | ||
| return dragZoneViews.filter { dragZone in | ||
| dragZone.convert(dragZone.bounds, to: nil).contains(pointInWindow) | ||
| } | ||
| } | ||
|
|
||
| repeat { | ||
| contentView.layoutSubtreeIfNeeded() | ||
| setDragHitTesting(hostingView) | ||
| dropDestinations = tabDropDestinations(in: hostingView) | ||
| dragZoneViews = dragZones(in: hostingView) | ||
| if dragZoneViews.contains(where: { dragZone in | ||
| dropDestinations.contains(where: { framesMatch(dragZone, $0) }) | ||
| }) { | ||
| let tabPointInWindow = hostingView.convert(tabPoint, to: nil) | ||
| if BonsplitTabItemHitRegionRegistry.containsWindowPoint(tabPointInWindow, in: window), | ||
| dropDestination(at: trailingEmptyPoint) != nil { | ||
| break | ||
| } | ||
| RunLoop.current.run(until: Date().addingTimeInterval(0.01)) | ||
| } while Date() < registrationDeadline | ||
|
|
||
| guard dragZoneViews.contains(where: { dragZone in | ||
| dropDestinations.contains(where: { framesMatch(dragZone, $0) }) | ||
| }) else { | ||
| guard dropDestination(at: trailingEmptyPoint) != nil else { | ||
| throw XCTSkip( | ||
| "This SwiftUI runtime does not expose view-local onDrop registration through registeredDraggedTypes" | ||
| ) | ||
| } | ||
|
|
||
| for point in [ | ||
| NSPoint(x: 90, y: 30), | ||
| NSPoint(x: 240, y: 30), | ||
| NSPoint(x: 460, y: 30), | ||
| ] { | ||
| guard let hitView = hostingView.hitTest(point) else { | ||
| XCTFail("Expected tab-bar chrome to hit-test at x=\(point.x)") | ||
| continue | ||
| } | ||
| let pointInWindow = hostingView.convert(point, to: nil) | ||
| let hitFrameInWindow = hitView.convert(hitView.bounds, to: nil) | ||
| let dropDestination = dropDestinations.first { view in | ||
| let destinationFrame = view.convert(view.bounds, to: nil) | ||
| return destinationFrame.contains(pointInWindow) | ||
| && abs(destinationFrame.minX - hitFrameInWindow.minX) <= 0.5 | ||
| && abs(destinationFrame.maxX - hitFrameInWindow.maxX) <= 0.5 | ||
| } | ||
| XCTAssertNotNil( | ||
| dropDestination, | ||
| "Trailing tab-bar chrome at x=\(point.x) should route tab transfers to a registered drop destination" | ||
| let tabPointInWindow = hostingView.convert(tabPoint, to: nil) | ||
| XCTAssertTrue( | ||
| BonsplitTabItemHitRegionRegistry.containsWindowPoint(tabPointInWindow, in: window), | ||
| "The test point should be owned by the rendered pane tab" | ||
| ) | ||
|
|
||
| for dragZone in chromeDragZones(at: tabPoint) { | ||
| XCTAssertFalse( | ||
| dropDestinations.contains(where: { framesMatch(dragZone, $0) }), | ||
| "Empty tab-bar chrome must not register an end-drop destination over a rendered tab" | ||
| ) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The overlay assertion can pass without testing anything.
The loop at Lines 2493-2498 runs only for chrome drag zones that contain tabPoint. If chromeDragZones(at: tabPoint) returns an empty array, the regression assertion is skipped and the test passes. framesMatch also requires a full frame match, so a chrome drop destination that only partially overlaps the tab is not detected.
tabPoint is also a fixed (90, 30) for a 480x60 host view. Tab metrics or tab-bar vertical placement changes turn this into a false failure at Line 2488 instead of a skip.
Assert that the candidate set is non-empty before the frame comparison, or derive tabPoint from the registered tab hit region so the point always lands on a rendered tab.
🔧 Proposed change: make the candidate set explicit
- for dragZone in chromeDragZones(at: tabPoint) {
+ let overlappingChromeZones = chromeDragZones(at: tabPoint)
+ XCTAssertFalse(
+ overlappingChromeZones.isEmpty,
+ "Expected the rendered tab point to be covered by tab-bar chrome drag zones"
+ )
+ for dragZone in overlappingChromeZones {
XCTAssertFalse(
dropDestinations.contains(where: { framesMatch(dragZone, $0) }),
"Empty tab-bar chrome must not register an end-drop destination over a rendered tab"
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let tabPoint = NSPoint(x: 90, y: 30) | |
| let trailingEmptyPoint = NSPoint(x: 460, y: 30) | |
| let registrationDeadline = Date().addingTimeInterval(0.5) | |
| var dropDestinations: [NSView] = [] | |
| var dragZoneViews: [TabBarDragZoneView.DragNSView] = [] | |
| func dropDestination(at point: NSPoint) -> NSView? { | |
| let pointInWindow = hostingView.convert(point, to: nil) | |
| return dropDestinations.first { view in | |
| view.convert(view.bounds, to: nil).contains(pointInWindow) | |
| } | |
| } | |
| func chromeDragZones(at point: NSPoint) -> [TabBarDragZoneView.DragNSView] { | |
| let pointInWindow = hostingView.convert(point, to: nil) | |
| return dragZoneViews.filter { dragZone in | |
| dragZone.convert(dragZone.bounds, to: nil).contains(pointInWindow) | |
| } | |
| } | |
| repeat { | |
| contentView.layoutSubtreeIfNeeded() | |
| setDragHitTesting(hostingView) | |
| dropDestinations = tabDropDestinations(in: hostingView) | |
| dragZoneViews = dragZones(in: hostingView) | |
| if dragZoneViews.contains(where: { dragZone in | |
| dropDestinations.contains(where: { framesMatch(dragZone, $0) }) | |
| }) { | |
| let tabPointInWindow = hostingView.convert(tabPoint, to: nil) | |
| if BonsplitTabItemHitRegionRegistry.containsWindowPoint(tabPointInWindow, in: window), | |
| dropDestination(at: trailingEmptyPoint) != nil { | |
| break | |
| } | |
| RunLoop.current.run(until: Date().addingTimeInterval(0.01)) | |
| } while Date() < registrationDeadline | |
| guard dragZoneViews.contains(where: { dragZone in | |
| dropDestinations.contains(where: { framesMatch(dragZone, $0) }) | |
| }) else { | |
| guard dropDestination(at: trailingEmptyPoint) != nil else { | |
| throw XCTSkip( | |
| "This SwiftUI runtime does not expose view-local onDrop registration through registeredDraggedTypes" | |
| ) | |
| } | |
| for point in [ | |
| NSPoint(x: 90, y: 30), | |
| NSPoint(x: 240, y: 30), | |
| NSPoint(x: 460, y: 30), | |
| ] { | |
| guard let hitView = hostingView.hitTest(point) else { | |
| XCTFail("Expected tab-bar chrome to hit-test at x=\(point.x)") | |
| continue | |
| } | |
| let pointInWindow = hostingView.convert(point, to: nil) | |
| let hitFrameInWindow = hitView.convert(hitView.bounds, to: nil) | |
| let dropDestination = dropDestinations.first { view in | |
| let destinationFrame = view.convert(view.bounds, to: nil) | |
| return destinationFrame.contains(pointInWindow) | |
| && abs(destinationFrame.minX - hitFrameInWindow.minX) <= 0.5 | |
| && abs(destinationFrame.maxX - hitFrameInWindow.maxX) <= 0.5 | |
| } | |
| XCTAssertNotNil( | |
| dropDestination, | |
| "Trailing tab-bar chrome at x=\(point.x) should route tab transfers to a registered drop destination" | |
| let tabPointInWindow = hostingView.convert(tabPoint, to: nil) | |
| XCTAssertTrue( | |
| BonsplitTabItemHitRegionRegistry.containsWindowPoint(tabPointInWindow, in: window), | |
| "The test point should be owned by the rendered pane tab" | |
| ) | |
| for dragZone in chromeDragZones(at: tabPoint) { | |
| XCTAssertFalse( | |
| dropDestinations.contains(where: { framesMatch(dragZone, $0) }), | |
| "Empty tab-bar chrome must not register an end-drop destination over a rendered tab" | |
| ) | |
| } | |
| let tabPoint = NSPoint(x: 90, y: 30) | |
| let trailingEmptyPoint = NSPoint(x: 460, y: 30) | |
| let registrationDeadline = Date().addingTimeInterval(0.5) | |
| var dropDestinations: [NSView] = [] | |
| var dragZoneViews: [TabBarDragZoneView.DragNSView] = [] | |
| func dropDestination(at point: NSPoint) -> NSView? { | |
| let pointInWindow = hostingView.convert(point, to: nil) | |
| return dropDestinations.first { view in | |
| view.convert(view.bounds, to: nil).contains(pointInWindow) | |
| } | |
| } | |
| func chromeDragZones(at point: NSPoint) -> [TabBarDragZoneView.DragNSView] { | |
| let pointInWindow = hostingView.convert(point, to: nil) | |
| return dragZoneViews.filter { dragZone in | |
| dragZone.convert(dragZone.bounds, to: nil).contains(pointInWindow) | |
| } | |
| } | |
| repeat { | |
| contentView.layoutSubtreeIfNeeded() | |
| setDragHitTesting(hostingView) | |
| dropDestinations = tabDropDestinations(in: hostingView) | |
| dragZoneViews = dragZones(in: hostingView) | |
| let tabPointInWindow = hostingView.convert(tabPoint, to: nil) | |
| if BonsplitTabItemHitRegionRegistry.containsWindowPoint(tabPointInWindow, in: window), | |
| dropDestination(at: trailingEmptyPoint) != nil { | |
| break | |
| } | |
| RunLoop.current.run(until: Date().addingTimeInterval(0.01)) | |
| } while Date() < registrationDeadline | |
| guard dropDestination(at: trailingEmptyPoint) != nil else { | |
| throw XCTSkip( | |
| "This SwiftUI runtime does not expose view-local onDrop registration through registeredDraggedTypes" | |
| ) | |
| } | |
| let tabPointInWindow = hostingView.convert(tabPoint, to: nil) | |
| XCTAssertTrue( | |
| BonsplitTabItemHitRegionRegistry.containsWindowPoint(tabPointInWindow, in: window), | |
| "The test point should be owned by the rendered pane tab" | |
| ) | |
| let overlappingChromeZones = chromeDragZones(at: tabPoint) | |
| XCTAssertFalse( | |
| overlappingChromeZones.isEmpty, | |
| "Expected the rendered tab point to be covered by tab-bar chrome drag zones" | |
| ) | |
| for dragZone in overlappingChromeZones { | |
| XCTAssertFalse( | |
| dropDestinations.contains(where: { framesMatch(dragZone, $0) }), | |
| "Empty tab-bar chrome must not register an end-drop destination over a rendered tab" | |
| ) | |
| } |
🤖 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 2449 - 2498, Strengthen
the overlay regression check around chromeDragZones(at:) by ensuring the
candidate drag-zone set is non-empty before evaluating overlap, so an empty set
cannot make the test pass vacuously. Replace the fixed tabPoint assumption with
a point derived from the registered rendered-tab hit region, or otherwise select
a guaranteed point inside that region, while preserving the existing assertion
that no chrome drop destination overlaps it.
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
Testing
Issues
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes same‑pane tab drag reorder by moving it into the drop delegate, honoring live hover targets and ignoring stale SwiftUI fallbacks to stop duplicate moves and flicker. Also tightens cleanup and keeps the end‑of‑bar drop active only in truly empty chrome.
samePaneDropTarget,performSamePaneReorder, and a fallback request planner) and tests for earlier→middle/later→middle, SwiftUI‑owned drops, no‑op suppression, controller move contract (select + geometry), and empty‑chrome end‑drop.Written for commit 529913b. Summary will update on new commits.
Summary by CodeRabbit