Skip to content

Fix horizontal tab strip insertion positions - #9765

Merged
austinywang merged 2 commits into
mainfrom
issue-9746-horizontal-tab-insert
Aug 7, 2026
Merged

austinywang merged 2 commits into
mainfrom
issue-9746-horizontal-tab-insert

Conversation

@austinywang

@austinywang austinywang commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • update bonsplit to use one pane-local drop destination across the horizontal tab strip
  • compute insertion indices from live tab-frame midpoints so every between-tab slot is reachable
  • preserve exact insertion behavior in translated split panes and for cross-pane moves
  • keep tab sizing and overflow clipping unchanged

Upstream implementation: manaflow-ai/bonsplit#202

Regression coverage

The bonsplit PR preserves the required red/green history:

  1. failing pointer-to-index regression tests
  2. the AppKit drop destination, shared mutation path, and behavior tests

Follow-up review coverage rejects stale pasteboard-only transfers that cannot complete, validates model/visual frame order, and exercises translated AppKit hit testing.

Testing

Review follow-up

  • removed sorting from the drag-update hot path; insertion lookup is linear over already pane-ordered frames
  • changed the resolver from static helper APIs to an instance collaborator
  • require an actionable cross-pane fallback handler before advertising a pasteboard-only move

Issue: #9746

Closes #9746

@coderabbitai

coderabbitai Bot commented Aug 7, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR updates the vendor/bonsplit submodule reference from one commit to another. No exported or public declarations change.

Changes

Bonsplit vendor update

Layer / File(s) Summary
Update submodule commit reference
vendor/bonsplit
The submodule reference changes to commit 7d2aa113169d0183b00cae0ec4d97e3a1605b102.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested reviewers: lawrencecchen, azooz2003-bit


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (2 errors, 1 inconclusive)

Check name Status Explanation Resolution
Cmux Algorithmic Complexity ❌ Error New production code sorts all live tab frames in TabDropIndexResolver.swift:41 on every AppKit draggingUpdated call (TabBarDropDestinationNSView.swift:101-125); tabs are an unbounded array and no c... Cache the pane's visually ordered frame midpoints when geometry changes, then use a linear lookup during draggingUpdated; otherwise add a measured, explicit tab-count bound and benchmark.
Cmux No Ambient Global State ❌ Error New Sources/Bonsplit/Internal/Views/TabDropIndexResolver.swift:4 defines a type whose only behavior is static insertionIndex APIs at lines 10 and 23, creating a static-helper namespace. Move insertion behavior to an instance-based, constructable TabDropIndexResolver injected into TabBarDropDestinationNSView, or use private/fileprivate file-scope pure helpers.
Cmux Swift Blocking Runtime ❓ Inconclusive Placeholder while gathering evidence. Continue repository inspection.
✅ Passed checks (22 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The Bonsplit update addresses precise tab insertion, all tab-group indices, and consistent behavior across panes and splits while preserving out-of-scope behavior.
Out of Scope Changes check ✅ Passed The changes are limited to the Bonsplit update for horizontal tab reordering and do not alter tab sizing or overflow clipping.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Cmux Swift Actor Isolation ✅ Passed Bonsplit PR marks new AppKit/SwiftUI drop types and handler @MainActor; the pure resolver is unisolated, and no new Sendable reference or background UI access appears.
Cmux Browser Automation Off-Main ✅ Passed The PR changes only the vendor/bonsplit gitlink; TerminalController.swift, the execution policy, and policy tests are unchanged, so no browser socket automation routing is introduced.
Cmux Expensive Synchronous Load ✅ Passed The Bonsplit diff adds only @MainActor tab-drop UI/controller logic; its two JSONDecoder calls parse bounded TabTransferData from the drag pasteboard, with no agent-history files, scans, or disk lo...
Cmux Cache Substitution Correctness ✅ Passed The diff only updates the Bonsplit submodule; its affected path uses live tab geometry for transient drop UI and in-memory reordering, not persistence, history, undo, or durable snapshots.
Cmux No Hacky Sleeps ✅ Passed The PR changes only the vendor/bonsplit submodule pointer; no TypeScript, JavaScript, shell, or runtime-script diff introduces a sleep or fixed delay.
Cmux Swift Concurrency ✅ Passed The diff changes only the vendor/bonsplit submodule pointer; no cmux-owned Swift files or app concurrency patterns are changed. Third-party code is outside this check.
Cmux Swift @Concurrent ✅ Passed The Bonsplit update adds synchronous @MainActor drop views/handlers and removes synchronous main-queue bridging; it adds no async, nonisolated async, or @concurrent work.
Cmux Swift Package Boundaries ✅ Passed The diff changes only the mode-160000 vendor/bonsplit gitlink; no app Swift or package files change. The boundary rule explicitly allows vendored code.
Cmux Swiftpm Lockfiles ✅ Passed PR changes only the vendored vendor/bonsplit submodule pointer; no cmux SwiftPM or Xcode package files changed, and Bonsplit preserves its upstream Package.resolved ignore policy.
Cmux Swift Logging ✅ Passed The Bonsplit patch adds no print, debugPrint, dump, NSLog, ad hoc logging, or Logger constants; it removes the prior DEBUG NSLog/dlog statements from TabBarView.
Cmux User-Facing Error Privacy ✅ Passed The submodule diff adds tab-drop handling and tests only; review found no new user-facing errors, alerts, raw upstream messages, or sensitive diagnostics.
Cmux Full Internationalization ✅ Passed The submodule diff changes tab-drop logic and tests only; it adds no production user-facing text or catalog/message files. Added literals are test names, fixtures, and diagnostics.
Cmux Swiftui State Layout ✅ Passed The Bonsplit diff adds an NSViewRepresentable/AppKit drop bridge and reuses existing @Observable state; it adds no banned state, GeometryReader, lazy-row store reference, or render-time write.
Cmux Architecture Rethink ✅ Passed The diff centralizes tab-strip drag handling in one AppKit destination and shared controller path; the pure midpoint resolver states its invariant, with no new sleeps, polling, locks, observers, or...
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The PR only updates the Bonsplit gitlink; the referenced diff changes tab-drop views, handlers, index logic, and tests. It adds no standalone cmux window or close-shortcut code.
Cmux Source Artifacts ✅ Passed The only changed path is the existing vendor/bonsplit gitlink, updated for the stated product fix; no artifact, scratch directory, or generated file entered source control.
Cmux No Test Or Debug Seam In Production Source ✅ Passed The Bonsplit production diff adds no DEBUG/test-only seams or test-named members; new helpers are called by production drop handling, and tests use @testable import.
Title check ✅ Passed The title clearly and concisely describes the primary change to horizontal tab strip insertion positions.
Description check ✅ Passed The description explains the change, motivation, regression coverage, testing results, and linked issue, but omits the template checklist and demo video section.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-9746-horizontal-tab-insert

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@austinywang

Copy link
Copy Markdown
Contributor Author

Review follow-up on the current bonsplit head 4da56ab:

  • Algorithmic complexity: removed the per-draggingUpdated sort. The destination supplies frames in pane-model order, the resolver validates model/visual ordering in O(n), then performs a linear midpoint scan.
  • Ambient global state: TabDropIndexResolver is now an instance collaborator owned by TabBarDropDestinationNSView; its insertion APIs are no longer static.
  • Pasteboard fallback: a no-live-state transfer advertises .move only for a cross-pane source with an installed external handler.

The follow-up behavior tests cover unordered frame rejection, same-pane stale transfers, and cross-pane transfers without a handler. Local validation is 216 XCTest cases plus 7 Swift Testing cases, all passing with no warnings observed.

@austinywang
austinywang merged commit c5bf3ca into main Aug 7, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Horizontal tab strip: dragged tab cannot be inserted between two tabs — drop always lands at the end

1 participant