Repository navigation
Fix file drag hit-test amplification - #10783
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughFile-drop hit testing now uses the configured reference view, caches the result for each drag update, and reuses it for routing. Inline traversal avoids hiding the overlay. A performance test verifies one underlying hit test per drag event. ChangesFile-drop hit testing
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR makes drag hit-testing reuse one content lookup per mouse event and avoids unnecessary overlay visibility invalidation, with targeted regression coverage and checks passing; no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
Full details: Description checkExplanation The description provides detailed change rationale, reproduction evidence, regression coverage, verification results, and known upstream blockers. It omits the template headings, demo video, review-trigger block, and checklist, but the core required information is present. Full details: Linked Issues checkExplanation The changes directly address issue Full details: Docstring CoverageExplanation Docstring coverage is 13.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 files. (2 skipped: 1 unsupported, 1 too large.) Full details: Cmux Swift Actor IsolationExplanation PASS. The production diff adds only synchronous state and hit-testing logic to the existing Full details: Cmux Swift Blocking RuntimeExplanation PASS. The PR adds no semaphore, blocking wait, sleep, delayed dispatch, polling loop, main-queue sync, or manual lock. Production changes only cache and restore hit-test state with Full details: Cmux Browser Automation Off-MainExplanation PASS: The pull request changes only file-drag hit testing, Full details: Cmux Expensive Synchronous LoadExplanation PASS: The production diff adds only Full details: Cmux Cache Substitution CorrectnessExplanation PASS — the diff caches only the Full details: Cmux No Hacky SleepsExplanation PASS: The pull request changes four Swift files, one Xcode project file, and adds one Swift test. It does not change TypeScript, JavaScript, shell, or non-Swift build/runtime code covered by Full details: Cmux Algorithmic ComplexityExplanation PASS. The production diff adds one underlying Full details: Cmux Swift ConcurrencyExplanation PASS. The pull request does not introduce or materially expand any listed legacy async pattern. The production diff adds synchronous hit-test state and Full details: Cmux Swift `@Concurrent`Explanation PASS: The PR introduces no Full details: Cmux Swift Package BoundariesExplanation PASS: The production diff is AppKit UI glue, not independently reusable domain logic. Full details: Cmux Swiftpm LockfilesExplanation PASS. The diff changes only Swift source files, adds one test file, and registers that test in Full details: Cmux Swift LoggingExplanation PASS: The PR diff adds no Full details: Cmux User-Facing Error PrivacyExplanation PASS — The production diff adds only hit-testing state and routing logic. It adds no user-facing error, alert, command output, API error body, or recovery copy. The only new strings are in the regression test and are covered by the rule's test exemption. Full details: Cmux Full InternationalizationExplanation PASS. The diff adds no user-facing production text, localization keys, catalogs, Info.plist entries, or web messages. The only added string literals are test name/assertion text in Full details: Cmux Swiftui State LayoutExplanation PASS. The diff adds no Full details: Cmux Architecture RethinkExplanation PASS: The diff introduces no new sleeps, delayed dispatch, polling, locks, observers, or duplicate drag entrypoints. The new hit-test value is a local, event-scoped snapshot owned by Full details: Cmux Swift Auxiliary Window Close ShortcutsExplanation PASS. The diff changes file-drop overlay hit testing and adds a test-only NSWindow fixture. It does not add or materially change a user-visible NSWindow, NSPanel, NSWindowController, SwiftUI Window, or WindowGroup. The only added NSWindow code is in cmuxTests/FileDropOverlayHitTestPerformanceTests.swift, which the rule explicitly allows. No auxiliary window identifier, close-shortcut workaround, or shared close-shortcut registration is required. Full details: Cmux Source ArtifactsExplanation PASS. The diff changes only Swift source, a Swift regression test, and Xcode project configuration. The new Full details: Cmux No Test Or Debug Seam In Production SourceExplanation PASS: The production diff adds no test/debug seam. The only new members are Full details: Cmux No Ambient Global StateExplanation PASS. The production diff adds
✨ 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 |
Closes #7811.
Profile evidence
Before editing, I reproduced the issue against the installed release app with 104 workspaces. The scripted drag used the issue's exact 80 mouse-moves over 4 seconds, from an
AGENTS.mdfile-explorer row to the terminal center.In the 4.41-second drag interval of the Time Profiler trace (
/tmp/cmux-7811-baseline-profile):NSCoreDragReceiveMessageProc, matching the 80-event driver.stepTransactionFlush).NSView._setHidden.The release build strips the cmux method names, but the system stacks align with the current
FileDropOverlayViewsource: each routing query hid the overlay and independently calledcontentView.hitTest.Change
draggingEntered/draggingUpdatedevent and reuse that immutable result for text, web, and terminal routing during that event.isHiddenand triggers AppKit layout/display invalidation.The snapshot is scoped to one mouse event and restored with
defer; it is not cached across cursor positions.Regression coverage
The commits intentionally preserve the required red/green order:
90dd8b6bd0adds the regression test only.6a29623c98adds the fix.FileDropOverlayHitTestPerformanceTestsdrives 80 distinct cursor positions through the realdraggingEntered/draggingUpdatedpath and requires exactly 80 underlying AppKit hit-tests. Before the fix, the same event path performs several full hierarchy hit-tests per event; after the fix, it performs one current-position lookup per event.Verification
ContentView.swift,FileDropOverlayView.swift, andFileDropOverlayViewHitTesting.swift../scripts/lint-pbxproj-test-wiring.sh(706 test files)../scripts/check-pbxproj.sh.git diff --check origin/main...HEAD.FileDropOverlayViewHitTesting.swiftshrinks by seven lines.cmux-unitscheme reaches this branch's files successfully, then current main fails inExtensionWorktreePrototype.swift:44-45because fix: preserve worktree identity initializer contract (follow-up to #10777) #10781 assignsletproperties that retain declaration defaults. This is unrelated to the PR.python3 scripts/swift_file_length_budget.pycannot run because main intentionally deleted that script and.github/swift-file-length-budget.tsvin Remove Swift file length budget #8125. The exact command was attempted and returnedErrno 2; no removed budget infrastructure is reintroduced here.Per the task constraint, I did not run a tagged reload or launch a development app, so no post-fix UI profile is claimed. The baseline trace is real; the regression cap is objective; CI can execute the app-hosted test. No user-facing strings changed, so localization catalogs were audited as unaffected.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
Bug Fixes
Performance
Tests