Repository navigation
Add didReorderTabsInPane delegate for within-pane tab reorder - #143
azooz2003-bit merged 3 commits into
Conversation
Fire a delegate callback after the user drag-reorders tabs within a single pane (the engine previously only notified on cross-pane moves). cmux's remote tmux mirror uses this to propagate window reordering to tmux swap-window. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new BonsplitDelegate callback for same-pane tab reorder and updates TabBarView to call it after manual drag-finish and drag-and-drop moves only when the pane's tab order actually changes. ChangesTab Reorder Delegate Notification
sequenceDiagram
participant TabBarView
participant Pane
participant BonsplitController
participant BonsplitDelegate
TabBarView->>Pane: captureOrderedTabIds()
TabBarView->>Pane: moveTab(from:to:) (no animation)
Pane-->>TabBarView: newOrderedTabIds
TabBarView->>BonsplitController: notify reorder if changed
BonsplitController->>BonsplitDelegate: splitTabBar(_:didReorderTabsInPane:orderedTabIds:)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 SummaryThis PR adds
Confidence Score: 4/5Safe to merge; the new callback is non-breaking and the two call sites are functionally correct for all known move semantics. The two call sites use slightly different guard strategies — the manual-drag path explicitly compares tab order before and after mutation, while the drop path relies on index-equality checks without verifying the actual resulting order. Under current Sources/Bonsplit/Internal/Views/TabBarView.swift — specifically the Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant TabBarManualReorder as Manual Drag Tracker
participant TabDropDelegate as SwiftUI DropDelegate
participant Pane
participant Delegate as BonsplitDelegate
User->>TabBarManualReorder: drag tab (within pane)
TabBarManualReorder->>Pane: capture orderBefore
TabBarManualReorder->>Pane: moveTab(from:to:) inside withTransaction
TabBarManualReorder->>Pane: read pane.tabs (outside transaction)
alt order actually changed
TabBarManualReorder->>Delegate: didReorderTabsInPane(pane, orderedTabIds)
end
User->>TabDropDelegate: drop tab (within pane)
TabDropDelegate->>TabDropDelegate: guard targetIndex ≠ sourceIndex / sourceIndex+1
TabDropDelegate->>Pane: moveTab(from:to:) inside withTransaction
TabDropDelegate->>Delegate: didReorderTabsInPane(pane, orderedTabIds) [inside withTransaction]
Reviews (1): Last reviewed commit: "Add didReorderTabsInPane delegate for wi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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)
Sources/Bonsplit/Public/BonsplitDelegate.swift (1)
2411-2415: 💤 Low valueDelegate called inside transaction in drop path but outside in manual path.
The drop delegate calls
splitTabBar(_:didReorderTabsInPane:orderedTabIds:)inside thewithTransaction(Transaction(animation: nil))block, while the manual reorder path (lines 2357–2363) calls it after the transaction closes. This inconsistency could lead to different animation behavior if the delegate makes UI changes, though in practice delegates typically sync external state rather than update UI.♻️ Optional refactor to match manual reorder pattern
Move the delegate call outside the transaction:
pane.moveTab(from: sourceIndex, to: targetIndex) - bonsplitController.delegate?.splitTabBar( - bonsplitController, - didReorderTabsInPane: pane.id, - orderedTabIds: pane.tabs.map { TabID(id: $0.id) } - ) } else { _ = bonsplitController.moveTab( TabID(id: draggedTab.id), toPane: pane.id, atIndex: targetIndex ) } } } applyMove() + +if sourcePaneId == pane.id { + bonsplitController.delegate?.splitTabBar( + bonsplitController, + didReorderTabsInPane: pane.id, + orderedTabIds: pane.tabs.map { TabID(id: $0.id) } + ) +}🤖 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/Public/BonsplitDelegate.swift` around lines 2411 - 2415, The drop path currently invokes the delegate method splitTabBar(_:didReorderTabsInPane:orderedTabIds:) from inside the withTransaction(Transaction(animation: nil)) block, while the manual reorder path invokes it after the transaction closes; move the delegate invocation out of the withTransaction block in the drop handling code so it runs after the transaction completes (compute and capture orderedTabIds inside the transaction if needed, then call splitTabBar(_:didReorderTabsInPane:orderedTabIds:) immediately after the withTransaction closure ends) to match the manual reorder pattern and ensure consistent animation behavior.
🤖 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 `@Sources/Bonsplit/Public/BonsplitDelegate.swift`:
- Around line 2411-2415: The drop path currently invokes the delegate method
splitTabBar(_:didReorderTabsInPane:orderedTabIds:) from inside the
withTransaction(Transaction(animation: nil)) block, while the manual reorder
path invokes it after the transaction closes; move the delegate invocation out
of the withTransaction block in the drop handling code so it runs after the
transaction completes (compute and capture orderedTabIds inside the transaction
if needed, then call splitTabBar(_:didReorderTabsInPane:orderedTabIds:)
immediately after the withTransaction closure ends) to match the manual reorder
pattern and ensure consistent animation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe1566ce-bf25-4f9b-9e4c-71512b1502cd
📒 Files selected for processing (2)
Sources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Public/BonsplitDelegate.swift
The drag-and-drop reorder path fired `splitTabBar(_:didReorderTabsInPane:orderedTabIds:)` unconditionally after `pane.moveTab`, unlike the manual-drag path which captures the order beforehand and only notifies when it actually changed. A `moveTab` that clamps or no-ops would push a spurious reorder to consumers (e.g. a redundant tmux `swap-window`). Capture the order before the move and fire the delegate only when `pane.tabs` order differs, mirroring the manual-drag path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/Bonsplit/Internal/Views/TabBarView.swift (1)
3410-3421:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffDelegate call still inside transaction; inconsistent with manual drag path.
The past review comment requested moving the delegate call outside the transaction, but the current code only added order verification while leaving the delegate call inside
withTransaction(lines 3403-3429). This creates an inconsistency with the manual drag path (lines 2357-2363), which calls the delegate after the transaction completes.Calling the delegate inside a transaction can trigger state changes mid-transaction. Both paths should follow the same pattern: capture order, perform move in transaction, then verify and notify outside the transaction.
🔄 Proposed fix to move delegate call outside transaction
let orderBeforeReorder = pane.tabs.map { $0.id } pane.moveTab(from: sourceIndex, to: targetIndex) - if pane.tabs.map({ $0.id }) != orderBeforeReorder { - bonsplitController.delegate?.splitTabBar( - bonsplitController, - didReorderTabsInPane: pane.id, - orderedTabIds: pane.tabs.map { TabID(id: $0.id) } - ) - } } else { _ = bonsplitController.moveTab( TabID(id: draggedTab.id), toPane: pane.id, atIndex: targetIndex ) } } } applyMove() + + if sourcePaneId == pane.id, pane.tabs.map({ $0.id }) != orderBeforeReorder { + bonsplitController.delegate?.splitTabBar( + bonsplitController, + didReorderTabsInPane: pane.id, + orderedTabIds: pane.tabs.map { TabID(id: $0.id) } + ) + }Note:
orderBeforeReordermust be declared outsideapplyMoveso it's accessible after the closure executes. Move the declaration to just before line 3401.🤖 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 3410 - 3421, The delegate notification is still being invoked inside the transaction; move the reorder notification so it runs after the transaction completes to match the manual-drag path. Capture orderBeforeReorder before calling the transactional closure that calls pane.moveTab(from:to:), declare that variable outside the transaction/applyMove block, perform pane.moveTab inside the transaction, then after the transaction finishes compare pane.tabs.map { $0.id } to orderBeforeReorder and, only if changed, call bonsplitController.delegate?.splitTabBar(... didReorderTabsInPane: ..., orderedTabIds: ...). Ensure no delegate calls occur inside the withTransaction/applyMove closure so state changes happen outside the transaction.
🤖 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.
Duplicate comments:
In `@Sources/Bonsplit/Internal/Views/TabBarView.swift`:
- Around line 3410-3421: The delegate notification is still being invoked inside
the transaction; move the reorder notification so it runs after the transaction
completes to match the manual-drag path. Capture orderBeforeReorder before
calling the transactional closure that calls pane.moveTab(from:to:), declare
that variable outside the transaction/applyMove block, perform pane.moveTab
inside the transaction, then after the transaction finishes compare
pane.tabs.map { $0.id } to orderBeforeReorder and, only if changed, call
bonsplitController.delegate?.splitTabBar(... didReorderTabsInPane: ...,
orderedTabIds: ...). Ensure no delegate calls occur inside the
withTransaction/applyMove closure so state changes happen outside the
transaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47bf1656-e780-486b-865f-5d829dd46abe
📒 Files selected for processing (1)
Sources/Bonsplit/Internal/Views/TabBarView.swift
Move the within-pane drop-reorder delegate call out of `withTransaction` (capture the pre-move order in the closure scope, notify after the transaction completes) so it matches the manual-drag path exactly instead of firing mid-transaction. Addresses review feedback on the original guard-only change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
manaflow-ai/bonsplit#143 (didReorderTabsInPane delegate) is merged, so the submodule no longer needs the fork commit: bump c209a0db3 -> 5728c21fd, the merge commit on manaflow-ai/bonsplit main. The bump also picks up the divider-thickness work (bonsplit#139) already merged on bonsplit main. .gitmodules already pointed at manaflow-ai/bonsplit; this PR now builds standalone with no unmerged dependencies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Note
Required by manaflow-ai/cmux#5553 (the remote tmux
-CCmirror). That PR'svendor/bonsplitsubmodule points at this delegate and cannot build until thisPR is merged. Please merge this first.
What
Adds one optional
BonsplitDelegatemethod, fired after the user drag-reorders tabs within a single pane:Bonsplit already notified on cross-pane moves (
didMoveTab…fromPane…toPane) but had no callback for a within-pane reorder. This fires it on both reorder paths (manual-drag tracking + the SwiftUI drop delegate), passing the pane's new full tab order.Why
Consumers that mirror external state to tab order need to know when tabs are reordered within a pane, not only moved across panes. (cmux's remote-tmux mirror uses it to propagate window reordering to tmux
swap-window.)Compatibility — non-breaking: a default no-op is added in the protocol extension, so existing conformers are unaffected.
Scope — 18 lines, 2 files (
TabBarView.swift,BonsplitDelegate.swift).Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Add optional
BonsplitDelegatecallbacksplitTabBar(_:didReorderTabsInPane:orderedTabIds:)to report within‑pane tab reorders, complementing the cross‑pane move callback. Fires for manual drag and SwiftUI drop only when the order changes, and now fires after the transaction to match manual‑drag timing; non‑breaking via a default no‑op.Written for commit c209a0d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes