Skip to content

Fix terminal file drops inserting paths - #3900

Closed
austinywang wants to merge 2 commits into
mainfrom
issue-3729-dnd-restore-path-insert
Closed

austinywang wants to merge 2 commits into
mainfrom
issue-3729-dnd-restore-path-insert

Conversation

@austinywang

@austinywang austinywang commented May 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Restore terminal file drops to the file-path insertion path by refusing inactive terminal pane drop targets.
  • Add a regression test for the portal lookup invariant that let viewer-oriented pane routing shadow terminal text insertion, with both active-context and cleared-context assertions.

Local repro

  1. On current main, construct a GhosttySurfaceScrollView and install an active TerminalPaneDropContext.
  2. Query paneDropTargetForDrop(at:) at the center of the hosted terminal; active pane routing is available.
  3. Clear the pane drop context with setPaneDropContext(nil), mirroring a stale/inactive terminal portal lookup during a Finder file drop.
  4. Observed on the failing regression commit: lookup still returns a TerminalPaneDropTargetView, even though that target cannot handle the drop because its context is nil. This can route the file drop toward the built-in preview path instead of terminal text insertion.
  5. Expected: lookup returns nil after context is cleared, allowing terminal file-path insertion to receive the drop.

Commits

  • 47b730493 adds the failing regression test only.
  • 6d13e2b09 applies the fix.

Testing

  • Not run locally by request; direct xcodebuild/local test execution is intentionally avoided before CI.
  • CI/CD is being driven with $iterate-pr until fully green.

Closes #3729


Note

Low Risk
Low risk: a small guard condition changes drop-target discovery only when dropContext is nil, plus a focused regression test to lock the behavior in.

Overview
Fixes Finder file drops being misrouted by making paneDropTargetForDrop(at:) return nil unless paneDropTargetView.dropContext is set, so inactive pane-drop targets no longer shadow file-path insertion.

Adds a regression test ensuring drop-target lookup succeeds with an active context and returns nil after setPaneDropContext(nil).

Reviewed by Cursor Bugbot for commit 6d13e2b. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • Bug Fixes

    • Prevented inactive/no-drop contexts from participating in drop hit-testing so they no longer interfere with file-path drop insertion.
  • Tests

    • Added a test ensuring drop target lookup returns no target when the drop context is inactive, guarding against accidental drop handling.

Review Change Stack

Review Change Stack

@vercel

vercel Bot commented May 12, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Canceled Canceled May 12, 2026 3:44am
cmux-staging Building Building Preview, Comment May 12, 2026 3:44am

@coderabbitai

coderabbitai Bot commented May 12, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

A guard in GhosttySurfaceScrollView.paneDropTargetForDrop(at:) now returns nil when the pane drop target view's dropContext is nil, preventing hit-testing and selection; a regression test verifies the lookup returns nil after clearing the drop context.

Changes

Drop-target detection guard

Layer / File(s) Summary
Drop-target context guard and validation
Sources/GhosttyTerminalView.swift, cmuxTests/TerminalAndGhosttyTests.swift
Added an early-return guard in paneDropTargetForDrop(at:) to skip drop-target detection when paneDropTargetView.dropContext is nil. Added testTerminalPaneDropTargetLookupRequiresActiveDropContext which sets a non-nil drop context and verifies paneDropTargetForDrop(at:) returns a target, then clears the context, forces layout, and asserts the lookup returns nil.

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly Related PRs

Poem

🐰 I hopped to the gate where the droppings lie,
If context is missing, I pass them by,
No phantom targets to trip the display,
The terminal keeps its rightful way,
Tests nod and nibble — hip hop, hooray!

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (14 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix terminal file drops inserting paths' accurately describes the main change: restoring terminal file drops to use the file-path insertion path instead of being misrouted to the built-in viewer.
Linked Issues check ✅ Passed The PR directly addresses #3729 by adding a guard to prevent inactive pane drop targets from intercepting file drops, allowing terminal file-path insertion to receive drops as intended.
Out of Scope Changes check ✅ Passed All changes are scoped to the drag-and-drop regression fix: one guard condition in the drop-target discovery and a focused regression test verifying the expected behavior.
Cmux Swift Actor Isolation ✅ Passed Production code change adds a guard nil-check to a MainActor-inherited NSView method. No new types, Sendable violations, or background context access introduced. Test file excluded per check rules.
Cmux Swift Blocking Runtime ✅ Passed No blocking patterns introduced. Production code adds a simple guard check. Test uses deterministic scaffolding without sleep or timing delays. Complies with swift-blocking-runtime.md rules.
Cmux No Hacky Sleeps ✅ Passed PR only modifies Swift files. The check explicitly states: "Swift timing and blocking primitives are covered by swift-blocking-runtime.md." Therefore, this PR is outside the scope of this check.
Cmux Swift Concurrency ✅ Passed Adds a synchronous guard and XCTest without legacy concurrency patterns. No DispatchQueue, Combine, completion handlers, or Tasks.
Cmux Swift @Concurrent ✅ Passed PR adds a synchronous guard check in paneDropTargetForDrop() with no async/await, @concurrent annotations, or heavy operations. All changes comply with swift-concurrent-annotation.md rules.
Cmux Swift File And Package Boundaries ✅ Passed Adds 1-line guard to paneDropTargetForDrop method and 27-line regression test. Focused bug fix; small AppKit glue code. Test code allowed per rules. Preserves single responsibility.
Cmux Swift Logging ✅ Passed All logging in the PR complies with Swift logging rules. Debug logging is properly guarded by #if DEBUG, and no unguarded print/NSLog statements exist in production code.
Cmux Swiftui State Layout ✅ Passed No new @Published/@Observable/@State patterns. Changes only in NSView subclass (AppKit), which is allowed per rules for bridge views not owned by SwiftUI.
Cmux Architecture Rethink ✅ Passed Small correctness fix. Adds single guard requiring active dropContext in paneDropTargetForDrop. No prohibited patterns. Clear owner and invariant. Includes regression test.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR modifies terminal pane helper views (NSView subclasses), not standalone windows. No NSWindow, NSPanel, or WindowGroup created. Test additions are in test-only fixtures.
Description check ✅ Passed The PR description is comprehensive and well-structured, covering all required sections from the template: summary, local reproduction steps, commits, and testing approach.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-3729-dnd-restore-path-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 and usage tips.

@austinywang
austinywang force-pushed the issue-3729-dnd-restore-path-insert branch from a3c1507 to be31f19 Compare May 12, 2026 02:55
@greptile-apps

greptile-apps Bot commented May 12, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a drag-and-drop routing bug where GhosttySurfaceScrollView.paneDropTargetForDrop(at:) would return a TerminalPaneDropTargetView even when its dropContext was nil, causing Finder file drops to be misrouted toward the built-in preview path instead of terminal text insertion.

  • Production fix: A single guard paneDropTargetView.dropContext != nil else { return nil } line added at the top of paneDropTargetForDrop(at:) ensures inactive targets are invisible to drop hit-testing.
  • Regression test: testTerminalPaneDropTargetLookupRequiresActiveDropContext now covers both the positive path (active context → non-nil result) and the negative path (nil context → nil result), fully pinning the invariant against future regressions.

Confidence Score: 5/5

Safe to merge — the guard is a minimal, correctly ordered early exit with no side effects on any path other than the broken nil-context one.

The production change is a single guard line that short-circuits an existing function only when dropContext is already nil; it cannot affect pane drop routing when a real context is present. The regression test now covers both the active-context and cleared-context paths, making the fix observable and falsifiable. No concurrency, state, or architectural concerns are introduced.

No files require special attention.

Important Files Changed

Filename Overview
Sources/GhosttyTerminalView.swift Adds a one-line nil-context guard to paneDropTargetForDrop(at:); fix is minimal, correctly ordered, and closes the drop-routing shadow bug.
cmuxTests/TerminalAndGhosttyTests.swift Adds a regression test covering both active-context (non-nil) and cleared-context (nil) paths, making the guard's effect observable and the test falsifiable.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Finder file drop"] --> B["paneDropTargetForDrop(at:)"]
    B --> C{"dropContext != nil"}
    C -- "false — NEW GUARD" --> D["Return nil → terminal text insertion"]
    C -- "true" --> E{"bounds check"}
    E -- "fail" --> D
    E -- "pass" --> F{"converted point check"}
    F -- "fail" --> D
    F -- "pass" --> G{"shouldDeferToPaneTabBar"}
    G -- "true" --> D
    G -- "false" --> H["Return TerminalPaneDropTargetView → pane routing"]
Loading

Reviews (2): Last reviewed commit: "Let terminal file drops reach path inser..." | Re-trigger Greptile

Comment thread cmuxTests/TerminalAndGhosttyTests.swift
A terminal portal can currently advertise a pane drop target after its drop context has been cleared. That lets the overlay route a Finder file drop into the pane-drop/viewer path instead of allowing the terminal text insertion path to own the file path.

Constraint: The bugfix commit must be separate, so this commit intentionally adds only the failing regression test.

Confidence: high

Scope-risk: narrow

Tested: Not run locally; user explicitly forbade direct xcodebuild and CI will exercise this regression before the fix commit.

Not-tested: Local XCTest execution.
Terminal portal lookup now applies the same active-context invariant as pane hit testing and browser portal lookup. When a terminal pane has no drop context, it is no longer advertised as a pane drop target, so Finder file drops can continue to the terminal text insertion path instead of being intercepted by preview routing.

Constraint: User required the fix in a separate commit after the failing regression test commit.

Rejected: Falling back after a pane target rejects the drop | broader overlay behavior change that could hide legitimate pane-drop failures.

Confidence: high

Scope-risk: narrow

Directive: Keep terminal and browser pane drop lookup aligned; a pane target without context must not be discoverable through portal lookup.

Tested: Not run locally; user explicitly forbade direct xcodebuild before CI. Regression is covered by testTerminalPaneDropTargetLookupRequiresActiveDropContext.

Not-tested: Local XCTest execution and manual drag/drop before CI.

This branch was successfully deployed

1 active deployment
Preview – cmux — 6d13e2b0 Deployed May 12, 2026 by vercel[bot]
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.

Regression: file drag-and-drop opens built-in viewer instead of referencing path

1 participant