Repository navigation
Fix a crash when a sidebar workspace drag ends (from #20015) - #20018
Merged
Merged
Conversation
Fails on main: #20015 forwards draggingEnded: to the reorder view, which does not implement it, so AppKit raises an unrecognized selector. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Bonsplit overlay forwarded draggingEnded: to the reorder view, which does not implement it. Swallow it for reorder drags, as AppKit did before #18837 when the reorder view was the destination, and type the destination as the reorder view. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The overlay safely completes sidebar reorder drags without forwarding an unsupported callback, and the inspected drop path remains intact. No actionable merge risk is supported. Pre-merge checks ·
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely identifies the primary change: preventing a crash when a sidebar workspace drag ends. Description check The description explains the crash, root cause, fix, regression test, testing limitation, and changelog entry. It omits the template's Proof and Checklist sections, but the core review information is … Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Cmux Cloud Persistent Session And Early Input PASS. The pull request changes only sidebar drag-drop handling and its regression test. It does not create or modify Cloud terminal clients, transports, readiness gates, input ownership, snapshots, au… Cmux Swift Actor Isolation The production diff only narrows reorderDestinationto the existing@MainActorSidebarWorkspaceReorderDropViewand changes drag-end forwarding. It introduces no implicit value-model or service-p…Cmux Swift Blocking Runtime The production diff only changes the reorder destination type and prevents forwarding draggingEnded; it adds no semaphore, blocking wait, sleep, delayed dispatch, polling, main-queue sync, or manual…Cmux Browser Automation Off-Main The pull request changes only sidebar drag-and-drop handling and its regression test. It does not change browser socket automation, browser.*command routing,socketWorkerMethods, `processV2Comman…Cmux Expensive Synchronous Load PASS: The production diff only narrows reorderDestinationtoSidebarWorkspaceReorderDropViewand stops forwardingdraggingEnded. It adds no agent-history loader, file read, directory scan, JSON/…Cmux Cache Substitution Correctness PASS: The production diff only changes drag-destination typing and prevents forwarding draggingEndedto the reorder view. It does not replace an authoritative read with a cached or opportunistic val…Cmux No Hacky Sleeps PASS. The pull request changes only Swift production code and Swift tests. It introduces no TypeScript, JavaScript, shell, or build/runtime-script sleeps, timers, polling, delayed dispatch, or wall-cl… Cmux Algorithmic Complexity PASS — The production diff changes a stored reference type and adds an early-return branch in draggingEnded. It adds no collection scan, nested iteration, sorting, filtering, join, or algorithm chan…Cmux Swift Concurrency PASS. The diff adds no legacy async pattern covered by the check. It changes AppKit drag callbacks, narrows the reorder destination type, and adds a test call to draggingEnded. These are AppKit/XCTe…Cmux Swift @ConcurrentThe diff changes only synchronous drag-routing code and a synchronous @MainActortest. It adds noasync,nonisolated async, or@concurrentdeclarations or call sites. Existing async tests and `D…Cmux Swift Package Boundaries PASS: The only production change is in SidebarBonsplitTabWorkspaceDropView, anNSViewAppKit drag-and-drop bridge. It changes the reorder destination type and prevents forwardingdraggingEndedt…Cmux Swiftpm Lockfiles PASS: The PR changes only two Swift source/test files. It does not modify a Package.swift dependency, any package .gitignore, an Xcode project package reference, or a Package.resolved lockfile. The Sw… Cmux Swift Logging PASS: The pull request adds no production logging and does not materially change existing logging. The Swift diff changes drag routing and adds a test assertion/comment only. Existing dlogcalls rem…Cmux User-Facing Error Privacy The production change affects sidebar drag-event routing, which is reachable through a user sidebar drag, but it adds no user-facing error, alert, command output, API body, or recovery text. The only … Cmux Full Internationalization PASS: The pull request changes drag-routing behavior and adds a regression test. The only added text is developer-only comments, and the test change is explicitly allowed. No user-facing Swift, web, m… Cmux Swiftui State Layout PASS: The pull request changes only SidebarBonsplitTabWorkspaceDropView, anNSViewAppKit bridge, and a test. The diff adds no SwiftUI view,ObservableObject/@Publishedstate, geometry measure…Cmux Architecture Rethink PASS: The diff is a small local AppKit callback fix. It changes the destination type to the concrete SidebarWorkspaceReorderDropViewand prevents forwardingdraggingEnded, which that view does not…Cmux Swift Auxiliary Window Close Shortcuts The PR changes drag-destination routing and a test-only drag fixture. It does not add or materially change a user-visible standalone NSWindow, NSPanel, NSWindowController, SwiftUI Window, or WindowGro… Cmux Source Artifacts The pull request changes only two intentional source-control paths: a hand-written Swift source file and its Swift regression test. The diff adds no logs, screenshots, recordings, temporary or cache d… Cmux No Test Or Debug Seam In Production Source The PR adds no test or debug seam to production source. The only production file changed is Sources/Sidebar/SidebarBonsplitTabWorkspaceDropView.swift; its additions change the reorder destination ty…
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Contributor
Author
|
Correctness review (subagent): this addresses the one critical finding on #20015 (the forwarded |
Contributor
|
Merge receipt for
Labeled |
github-actions
Bot
added
the
merged-unverified
Merged
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.
Follow-up to #20015. That fix made sidebar workspace reorder work again, but it crashes when the drag ends over the sidebar.
Cause
In #20015,
SidebarBonsplitTabWorkspaceDropViewforwardsdraggingEnded(_:)toSidebarWorkspaceReorderDropView. That view doesn't override the method, andNSViewdoesn't implementdraggingEnded:at runtime:NSView.instancesRespond(to: "draggingEnded:")is false. Swift still compiles the call because the optionalNSDraggingDestinationrequirement is imported ontoNSView, so at runtime it throwsNSInvalidArgumentException: unrecognized selector. AppKit sendsdraggingEnded:to the last destination that responds to it, and the Bonsplit overlay does, so ending a row drag over the sidebar hits this path. A correctness review subagent found it after #20015 merged.Fix
draggingEndedinstead of forwarding it. That matches how things worked before Drop a sidebar workspace onto a pane to merge its tabs #18837, when AppKit skipped the reorder view for this call.reorderDestinationis now typed asSidebarWorkspaceReorderDropView.Tests
workspaceRowDragOverBonsplitOverlayStillReordersnow also callsbonsplit.draggingEnded(sender). The test commit comes before the fix. On that commit the call throws. Neither result was run locally, because local xcodebuild tests aren't allowed here. Hosted CI provides the green result.Changelog
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Fixes a crash when ending a workspace row drag over the sidebar. The Bonsplit overlay forwarded
draggingEnded:to the reorder drop view, which doesn't implement it, so AppKit raised an unrecognized selector exception. The overlay now swallows the call for sidebar reorder drags, andreorderDestinationis typed asSidebarWorkspaceReorderDropView.Written for commit 3e3237d. Summary will update on new commits.
Summary by CodeRabbit