Repository navigation
Make file drops default to path text - #3684
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWhen files are dragged, a new routing policy can route file URLs into text insertion (editor or terminal) instead of preview/transfer. An overlay hint badge shows the resolved destination kind; insertion routes use shell-escaped paths via TerminalImageTransfer and JS insertion for web editors. Settings, localization, and tests were added. ChangesShift+file-drop-to-text routing
Sequence Diagram(s)sequenceDiagram
participant User
participant OverlayView
participant RoutingPolicy
participant TargetHost
participant TerminalSurface
User->>OverlayView: drag with file URLs
OverlayView->>RoutingPolicy: shouldRouteFileDropToTextDestination(types, flags)
RoutingPolicy-->>OverlayView: true/false
alt text routing
OverlayView->>TargetHost: performFileDropAsText(urls)
TargetHost->>TerminalSurface: sendText(shell-escaped paths) // or
TargetHost->>WKWebView: evaluate JS to insert text
else preview/transfer
OverlayView->>Workspace: handle preview/transfer flow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (11 passed)
✨ Finishing Touches📝 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 |
Greptile SummaryThis PR changes the default file-drop behavior to insert shell-escaped path text into interactive destinations (terminals, browser web views, file-preview text editors), with Shift inverting to preview/split. It extracts the monolithic
Confidence Score: 4/5Safe to merge with the The refactoring and new routing logic are well-structured and most previously-flagged issues have been addressed. One confirmed defect in the new file-preview-drag text-insertion path:
Important Files Changed
Sequence DiagramsequenceDiagram
participant Finder
participant FileDropOverlayView
participant DragOverlayRoutingPolicy
participant PaneDropTargetView
participant BrowserPaneDropTargetView
participant FileDropTextDropController
Finder->>FileDropOverlayView: draggingEntered/Updated
FileDropOverlayView->>DragOverlayRoutingPolicy: resolvedFileDropBehavior(modifierFlags)
alt "behavior == .text (default)"
FileDropOverlayView->>FileDropOverlayView: paneDropTargetForTextDrop
FileDropOverlayView->>PaneDropTargetView: fileDropDraggingEntered
PaneDropTargetView->>DragOverlayRoutingPolicy: shouldRouteFileDropToTextDestination
FileDropOverlayView->>FileDropOverlayView: updateHintBadge (show Shift hint)
else "behavior == .preview (Shift held)"
FileDropOverlayView->>PaneDropTargetView: fileDropDraggingEntered (split UI)
end
Finder->>FileDropOverlayView: performDragOperation
alt text path
FileDropOverlayView->>PaneDropTargetView: fileDropPerformDragOperation
PaneDropTargetView->>PaneDropTargetView: handleFileDropAsText
PaneDropTargetView->>FileDropTextDropController: performPanelTextDrop
FileDropTextDropController->>FileDropTextDropController: insert() + focusPanel
else browser web view
FileDropOverlayView->>BrowserPaneDropTargetView: fileDropPerformDragOperation
BrowserPaneDropTargetView->>BrowserPaneDropTargetView: shouldRouteFileDropToHostedWebView
BrowserPaneDropTargetView->>FileDropTextDropController: focusBrowserPanel
else preview path
FileDropOverlayView->>PaneDropTargetView: fileDropPerformDragOperation (split)
end
Finder->>FileDropOverlayView: concludeDragOperation
FileDropOverlayView->>PaneDropTargetView: fileDropConcludeDragOperation
Reviews (14): Last reviewed commit: "Fix browser drop review issues" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| webView.evaluateJavaScript(script) | ||
| return true |
There was a problem hiding this comment.
evaluateJavaScript(_:) is the fire-and-forget overload — its return value (the IIFE's true/false) is discarded, and the Swift function unconditionally returns true. If the cursor lands over a non-editable element in the web view, the JS IIFE returns false, no text is inserted, but AppKit receives a success signal and animates the drag image away as though the drop worked. The user loses the file path with no feedback. Use evaluateJavaScript(_:completionHandler:) and propagate the JS boolean back to the caller (the completion runs on the main thread so a @MainActor callback is safe here).
| private static func insertedText(for fileURLs: [URL]) -> String { | ||
| insertedText(forFileURLs: fileURLs) | ||
| } |
There was a problem hiding this comment.
insertedText(for:) wrapper now only delegates to insertedText(forFileURLs:) and has no remaining callers. It was kept to preserve existing internal call sites, but after the rename those sites should use forFileURLs directly so the private shim can be removed.
| private static func insertedText(for fileURLs: [URL]) -> String { | |
| insertedText(forFileURLs: fileURLs) | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| func hide() { | ||
| guard !isHidden else { return } | ||
| alphaValue = 0 | ||
| isHidden = true | ||
| } |
There was a problem hiding this comment.
hide() path clears isHidden immediately without any animation, making the badge pop away abruptly. show() fades in via animator(), so a matching fade-out on hide() would keep the transition consistent and less jarring when Shift is released mid-drag.
| func hide() { | |
| guard !isHidden else { return } | |
| alphaValue = 0 | |
| isHidden = true | |
| } | |
| func hide() { | |
| guard !isHidden else { return } | |
| NSAnimationContext.runAnimationGroup { context in | |
| context.duration = 0.12 | |
| animator().alphaValue = 0 | |
| } completionHandler: { [weak self] in | |
| self?.isHidden = true | |
| } | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 559-570: The fallback that treats a window firstResponder field
editor as an editor in textDropDestinationKindUnderPoint is causing mis-routed
drops and incorrect hints; update textDropDestinationKindUnderPoint (and the
related fallback logic used by editableTextViewUnderPoint) to only return
.editor when the resolved field editor/view's frame actually contains the
supplied windowPoint (i.e., hit-test the field editor's convertToWindow/bounds
or use hitTest on its superview) or remove the fallback entirely so only direct
hit-tested editableTextViews/webViews/terminals (editableTextViewUnderPoint,
webViewUnderPoint, terminalUnderPoint) are considered; ensure
performFileDropAsText uses the same point-validated target to avoid inserting
into an off-point firstResponder.
- Around line 348-353: When routing the drop to text
(shouldRouteFileDropToTextDestination(sender)) ensure we call draggingExited on
any activeDragWebView and activePaneDropTarget before nulling them and
returning; update the early-return paths in prepareForDragOperation and
performDragOperation to invoke activeDragWebView?.draggingExited(sender) and
activePaneDropTarget?.draggingExited(sender) (mirroring the cleanup in
draggingUpdated), then clear
preparedDragWebView/preparedPaneDropTarget/activePaneDropTarget and return;
likewise, in concludeDragOperation call
activeDragWebView?.draggingExited(sender) and
activePaneDropTarget?.draggingExited(sender) before the defer that clears
activeDragWebView so the web view receives a matching draggingExited/conclude
sequence.
- Around line 10-132: The ContentView file has ~120 lines of self-contained
AppKit code that should be extracted: move the FileDropHintBadgeView class and
FileDropTextDestinationKind enum into their own Swift source file, update
imports if needed, and remove the file-private/private modifier so they are
internal by omission (or explicitly internal) in the new file; ensure references
to FileDropHintBadgeView and FileDropTextDestinationKind in ContentView remain
unchanged and build by adding the new file to the target.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 9254-9259: The method handleDroppedFileURLsAsText currently
returns true even if terminalSurface is nil and sendText no-ops; update it to
return false when there's no active terminal surface: after computing text
(TerminalImageTransferPlanner.insertedText(forFileURLs:)), ensure
terminalSurface is present (e.g., guard let surface = terminalSurface else {
return false }) before calling sendText on that surface and then return true;
reference the handleDroppedFileURLsAsText function and terminalSurface property
when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e06a849-23ed-4a30-ae42-b12ed2592903
📒 Files selected for processing (7)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/DragOverlayRoutingPolicy.swiftSources/GhosttyTerminalView.swiftSources/TerminalImageTransfer.swiftSources/TerminalPaneDropTargetView.swiftcmuxTests/FinderFileDropRegressionTests.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
lawrencecchen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/FileExplorerTerminalPathInsertion.swift (1)
12-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
relativePathfallback returns un-normalized path, defeating the newmacOSDisplayPathrewriteIn the non-matching branch (line 21), the function returns the caller's original
pathrather thannormalizedPath. Before this PR,normalizedFileSystemPathwas nearly idempotent on real absolute paths (just resolves./..), so the gap was negligible. Now thatmacOSDisplayPathcan materially rewrite strings (e.g./private/tmp/foo→/tmp/foo), the fallback silently yields the/private/…form even though the matching branch would have yielded the display-normalized form.🐛 Proposed fix
if normalizedPath.hasPrefix(normalizedRoot) { return String(normalizedPath.dropFirst(normalizedRoot.count)) } - return path + return normalizedPath }🤖 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/FileExplorerTerminalPathInsertion.swift` around lines 12 - 22, The fallback in relativePath(for:rootPath:) returns the original caller `path` which bypasses the `normalizedFileSystemPath`/`macOSDisplayPath` rewrite; change the non-matching return to return `normalizedPath` (and likewise return `normalizedPath` when `rootPath` is empty) so all branches consistently use the normalized/display path produced by `normalizedFileSystemPath`/`macOSDisplayPath` (refer to relativePath, normalizedFileSystemPath, normalizedPath, normalizedRootPath).
🤖 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.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 19-20: The Japanese text uses the ambiguous term "パステキスト" which
can be read as "paste text"; update the Japanese localizations for the resource
keys settings.app.fileDrop.defaultBehavior.preview.subtitle and
settings.app.fileDrop.defaultBehavior.text to use a clearer translation for
"path text" such as "パス文字列" (or "パスのテキスト") in both entries so the intent is
unambiguous.
In `@Sources/ContentView.swift`:
- Around line 575-576: Replace the unconditional WebView-based drop routing with
a cached editable-target probe: stop returning .editor based solely on
webViewUnderPoint(...) (the code in webViewUnderPoint(...) and the drop-routing
branch); instead, during drag update use the preparedDragWebView pattern to call
evaluateJavaScript(script, completionHandler:) that runs elementFromPoint(...)
and determines whether the element is editable, store that boolean on
preparedDragWebView (or a related cached flag), and in the actual drop handler
consult that cached editable flag to decide .editor routing; also ensure the
evaluateJavaScript completion handler updates the cache before the drop and
remove reliance on immediate synchronous evaluateJavaScript calls at drop time.
In `@Sources/SettingsNavigation.swift`:
- Line 299: The settingsPathAnchorIDs map is missing an entry for the new "File
Drops" setting, so add a mapping from the settings key (likely
"app.fileDropDefaultBehavior") to the anchor id produced by settingID(for: .app,
idSuffix: "file-drops") in the settingsPathAnchorIDs collection (and update any
related search/index wiring if there is a parallel mapping), ensuring the new
entry mirrors how other settings are wired so deep-links and jump-to behavior
work correctly.
---
Outside diff comments:
In `@Sources/FileExplorerTerminalPathInsertion.swift`:
- Around line 12-22: The fallback in relativePath(for:rootPath:) returns the
original caller `path` which bypasses the
`normalizedFileSystemPath`/`macOSDisplayPath` rewrite; change the non-matching
return to return `normalizedPath` (and likewise return `normalizedPath` when
`rootPath` is empty) so all branches consistently use the normalized/display
path produced by `normalizedFileSystemPath`/`macOSDisplayPath` (refer to
relativePath, normalizedFileSystemPath, normalizedPath, normalizedRootPath).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d44aee2-1f53-4bfd-82c1-ee77723dd25f
📒 Files selected for processing (8)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/DragOverlayRoutingPolicy.swiftSources/FileExplorerTerminalPathInsertion.swiftSources/SettingsNavigation.swiftSources/TerminalPaneDropTargetView.swiftSources/cmuxApp.swiftcmuxTests/FinderFileDropRegressionTests.swift
| case .browser: | ||
| return .editor |
There was a problem hiding this comment.
handleFileDropAsText has no browser branch
fileDropTextDestinationKind returns .editor for .browser panels (line 541–542), causing shouldRouteFileDropToTextDestination to return true and performDragOperation to call handleFileDropAsText. But handleFileDropAsText only has branches for TerminalPanel and FilePreviewPanel; a browser panel falls through to return false (line 521). The drag operation is accepted with .copy at the updateDragState level, then silently dropped — the user loses the file path with no feedback.
Either add a BrowserPanel branch in handleFileDropAsText that routes through WindowBrowserSlotView.handleDroppedFileURLsAsText, or return nil from fileDropTextDestinationKind for .browser here since browser panes already have their own BrowserPaneDropTargetView path that handles text insertion directly.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/BrowserWindowPortal.swift (1)
1418-1881: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSplit this pane-drop block into a dedicated Swift file before merge.
CI is already red on the Swift file-length budget (
4362 > 4305), and this new pane-drop target / slot-helper code is cohesive enough to move without changing behavior. Pulling it into its own file underSources/should unblock the budget and keep future portal changes reviewable. Based on learnings: "This repo’s CI enforces a Swift file-length budget for large view files ... extract the subview into a dedicated Swift file under Sources."🤖 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/BrowserWindowPortal.swift` around lines 1418 - 1881, This file is over the Swift file-length budget; extract the cohesive pane-drop/slot helper classes into a new Swift source file: move the final class BrowserPaneDropTargetView and the final class WindowBrowserSlotView (including their private vars, methods like setPaneDropContext(_:), paneDropTargetForDrop(_:), setPortalDragDropZone(_:), setDropZoneOverlay(zone:), handleDroppedFileURLsAsText(_:at:), and any DEBUG-only helpers such as logHitTestDecision and clearDragState) into a new Sources/Swift file, preserve all access levels, imports, and conditional compilation blocks, remove their definitions from the original file, ensure the new file is added to the build target, and run a build to fix any missing references (update any fileprivate/internal usage if needed to maintain visibility across files).
♻️ Duplicate comments (6)
Resources/Localizable.xcstrings (1)
20-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse unambiguous Japanese for “path text” in this label
パステキストis ambiguous in Japanese; this should match the clearer terminology already used in Line 19 (for example,パス文字列).🤖 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 `@Resources/Localizable.xcstrings` at line 20, The Japanese localization for "settings.app.fileDrop.defaultBehavior.text" uses the ambiguous term "パステキスト"; update the Japanese "value" to the clearer term "パス文字列" to match the adjacent localization (line 19) so the label is unambiguous for Japanese users.Sources/GhosttyTerminalView.swift (1)
9254-9259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn
falsewhen text insertion has no target.On Line 9257,
terminalSurface?.sendText(text)can no-op while the method still returnstrue, which misreports drop handling success.Suggested fix
func handleDroppedFileURLsAsText(_ urls: [URL]) -> Bool { let text = TerminalImageTransferPlanner.insertedText(forFileURLs: urls) guard !text.isEmpty else { return false } - terminalSurface?.sendText(text) + guard let terminalSurface else { return false } + terminalSurface.sendText(text) return true }🤖 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/GhosttyTerminalView.swift` around lines 9254 - 9259, The method handleDroppedFileURLsAsText currently returns true even when terminalSurface is nil and sendText is a no-op; change it to verify a real target before claiming success: compute text with TerminalImageTransferPlanner.insertedText(forFileURLs:), guard that text is non-empty and that terminalSurface is non-nil (or otherwise call a sendText-returning API and propagate its Bool), then call terminalSurface.sendText(text) and return true only when a valid surface handled the text—return false if terminalSurface is missing or the send did not actually occur.Sources/ContentView.swift (4)
358-363:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winShift-routing early-returns still leave
activeDragWebView/activePaneDropTargetwith an unbalanceddraggingEntered.
draggingUpdated(hunk 10, 474–483) correctly callsprev.draggingExited(sender)on bothactiveDragWebViewandactivePaneDropTargetwhen transitioning into text-routing mode. However, if the user only adds Shift at the moment of drop (i.e., the lastdraggingUpdatedran in non-text mode and set up anactiveDragWebView/activePaneDropTarget), then:
prepareForDragOperation(358–363) clearspreparedPaneDropTargetandactivePaneDropTarget = nilbut never notifies them viadraggingExited, and ignoresactiveDragWebViewentirely.performDragOperation(400–406) has the same gap.concludeDragOperation(443–462)defers the nil-out but does not calldraggingExitedon either before clearing.Net effect: the previously hovered web view / pane target receives an unbalanced
draggingEnteredand can leak stale drop indicators or hover state. Mirror the cleanup thatdraggingUpdatedalready performs at all three sites:🛠 Apply at all three early-return sites
if shouldRouteFileDropToTextDestination(sender) { + if let prev = activeDragWebView { + prev.draggingExited(sender) + activeDragWebView = nil + } + if let prev = activePaneDropTarget { + prev.fileDropDraggingExited(sender) + activePaneDropTarget = nil + } preparedDragWebView = nil preparedPaneDropTarget = nil - activePaneDropTarget = nil ... }In
concludeDragOperation, perform the samedraggingExitedcalls before thedeferclears the references.Also applies to: 400-406, 443-462
🤖 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/ContentView.swift` around lines 358 - 363, When early-returning to route the drop to text in prepareForDragOperation, performDragOperation, and concludeDragOperation (where you currently set preparedDragWebView/preparedPaneDropTarget/activePaneDropTarget to nil), first call draggingExited(sender) on any existing activeDragWebView and activePaneDropTarget (and on preparedPaneDropTarget if set) to mirror the cleanup done in draggingUpdated; then clear those references (and only then return). This ensures any prior draggingEntered is balanced and avoids leaking hover state; locate the cleanup in prepareForDragOperation, performDragOperation, and the defer block in concludeDragOperation and add the draggingExited(sender) calls before nil-ing activeDragWebView/activePaneDropTarget/preparedPaneDropTarget.
10-142:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCI blocker: Swift file length budget exceeded — extract
FileDropHintBadgeView,FileDropPaneTarget, and the pane-target extensions to dedicated files.The
workflow-guard-testsjob is now failing withactual=16182, budget=15956(226 lines over). The newly addedFileDropHintBadgeView(~105 lines, 10–114),FileDropPaneTargetprotocol (116–124), and thePaneDropTargetView/BrowserPaneDropTargetViewconformance extensions (126–142) together account for more than the overage and are entirely self-contained AppKit types with no need to live inContentView.swift.Move them into dedicated files under
Sources/(e.g.,Sources/FileDropHintBadgeView.swiftandSources/FileDropPaneTarget.swift). Drop theprivatemodifiers on the moved declarations so they default to internal visibility, sinceFileDropOverlayView(still in this file) needs to reference them.Based on learnings from PR 3502: extract helper subviews into a dedicated Swift file under
Sourcesto stay under the CI file-length budget; mark theminternal(omitprivate) so other files can reference them.🤖 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/ContentView.swift` around lines 10 - 142, The file is over budget; extract FileDropHintBadgeView, FileDropPaneTarget protocol, and the PaneDropTargetView/BrowserPaneDropTargetView extensions into separate source files (e.g., Sources/FileDropHintBadgeView.swift and Sources/FileDropPaneTarget.swift), remove the leading private modifiers so they are internal (omit private) so FileDropOverlayView and other types can access them, and ensure the new files import AppKit if needed and preserve all method and type names (FileDropHintBadgeView, configureNativeGlassIfNeeded, FileDropPaneTarget, and the two extensions) exactly so existing references continue to compile.
564-566:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
textDropDestinationKindUnderPointreturns.editorfor anyWKWebViewregardless of editability.Line 564 returns
.editorwhenever aWKWebViewis under the cursor. This drives three behaviors that all assume a real editable target exists:
updateHintBadge(542–558) — shows a "drop into editor" hint over web views with no editable focus.shouldRouteFileDropToTextDestination(533–540) — tellsprepareForDragOperationto consume the drop instead of letting the web view's native drop handling run.performFileDropAsText(583–585) — delegates toFileDropTextInsertion.insert(...), but since the upstream gate already returned true, the drop is consumed even when the JS finds no editable element.Probe editability during
draggingUpdated(using thepreparedDragWebViewcaching pattern that already exists for the non-text route) by runningevaluateJavaScriptwith a completion handler that checks whetherelementFromPointis editable, cache that boolean, and consult the cache intextDropDestinationKindUnderPointso non-editable web content falls through to terminal/native drop handling.🤖 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/ContentView.swift` around lines 564 - 566, textDropDestinationKindUnderPoint currently returns .editor whenever webViewUnderPoint(windowPoint) != nil, causing editor-specific flows to run for non-editable WKWebView content; change the logic to consult a cached “webViewIsEditable” boolean that you populate during draggingUpdated by using the existing preparedDragWebView pattern and calling preparedDragWebView.evaluateJavaScript("document.elementFromPoint(x,y)...") (or equivalent elementFromPoint check) with a completion handler to determine if the element is editable, store that result on the drag state (cache), and then have textDropDestinationKindUnderPoint return .editor only when webViewUnderPoint(...) != nil AND the cached webViewIsEditable is true; update related callers (updateHintBadge, shouldRouteFileDropToTextDestination, performFileDropAsText / FileDropTextInsertion.insert) to rely on that cached value so non-editable web content falls through to native/terminal drop handling.
614-619:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFirst-responder fallback in
editableTextViewUnderPointstill routes drops to off-cursor field editors.The fallback at 614–618 returns the window's firstResponder field editor whenever the hit-test walk finds nothing, regardless of where the cursor actually is. Because
textDropDestinationKindUnderPoint(560–571) is the gate for both the floating Shift hint badge and the entire shift-routing path:
- The badge can show "drop into editor" when the cursor is over a non-editable area while a search field elsewhere holds focus.
performFileDropAsTextwill insert the path into that off-cursor field editor — a surprising outcome for a positional gesture.Either drop the fallback entirely (rely on the hit-test walk), or gate it on the field editor's frame actually containing
windowPoint:🛠 Proposed fix
- if let fieldEditor = window?.firstResponder as? NSTextView, - fieldEditor.isFieldEditor, - fieldEditor.isEditable { - return fieldEditor - } - return nil + if let fieldEditor = window?.firstResponder as? NSTextView, + fieldEditor.isFieldEditor, + fieldEditor.isEditable, + let frameInWindow = fieldEditor.superview?.convert(fieldEditor.frame, to: nil), + frameInWindow.contains(windowPoint) { + return fieldEditor + } + return nil🤖 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/ContentView.swift` around lines 614 - 619, The fallback in editableTextViewUnderPoint currently returns the window's firstResponder field editor unconditionally, which routes drops to off-cursor editors; change the fallback to only return the field editor if its frame actually contains the windowPoint (i.e., convert the fieldEditor’s bounds to window coordinates and check contains(windowPoint)), otherwise return nil—this keeps textDropDestinationKindUnderPoint and performFileDropAsText driven by the hit-test walk and prevents inserting into an off-cursor NSTextView.
🤖 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.
Inline comments:
In `@Sources/BrowserWindowPortal.swift`:
- Around line 1445-1457: The current guard treats any file-URL drag with
fileDropBehavior == .copy as captured, but handleDroppedFileURLsAsText(...) can
still fail for non-editable targets or unreadable payloads; change the capture
logic so that when fileDropBehavior == .copy you actually attempt
handleDroppedFileURLsAsText(...) and, if it returns false, treat the drop as not
captured (so it can fall back to preview/split handling). Update the
variables/branching around fileDropBehavior/resolvedFileDropBehavior,
shouldCaptureFileDrop and the guard in BrowserWindowPortal (and the analogous
blocks at the other locations) to consult the result of
handleDroppedFileURLsAsText(...) and only mark the drop captured when that call
succeeds; otherwise allow
shouldCaptureFilePreviewTransfer/shouldCaptureBonsplitTransfer to proceed.
In `@Sources/TerminalPaneDropTargetView.swift`:
- Around line 515-521: The browser pane case is missing so drops routed as text
are incorrectly rejected; update TerminalPaneDropTargetView to handle browser
panes by adding a branch that checks for the browser pane type (e.g.
BrowserPanel or your BrowserPane class used for browser tabs) before the
FilePreviewPanel branch and return false (or call an appropriate
browser-specific handler if wired) so browser drops advertised as text do not
block fallback behavior; apply the same change to the other occurrence around
the 538-543 region where handleFileDropAsText is dispatched.
- Line 125: Add explicit deinit implementations for both
PaneDropZoneOverlayAnimator and PaneDropTargetView to satisfy the
required_deinit lint rule; locate the class declarations for
PaneDropZoneOverlayAnimator and PaneDropTargetView and add a deinit { } block
(perform any necessary teardown there or leave empty if none), ensuring any
retained resources or observers are cleaned up and superclass deinit is
implicitly handled.
- Around line 125-236: The file is too large for the repo guard—extract the
PaneDropZoneOverlayAnimator and its helper methods (class
PaneDropZoneOverlayAnimator, static func applyStyle, func setZone, func
hideImmediately, private func applyFrame, private static func
rectApproximatelyEqual) into a new Sources/<DescriptiveName>.swift file; keep
the class and method signatures unchanged (same access level and final
modifier), import AppKit if needed, remove the moved class from the original
file, and ensure the new file is included in the Sources target so builds and
tests remain unchanged.
---
Outside diff comments:
In `@Sources/BrowserWindowPortal.swift`:
- Around line 1418-1881: This file is over the Swift file-length budget; extract
the cohesive pane-drop/slot helper classes into a new Swift source file: move
the final class BrowserPaneDropTargetView and the final class
WindowBrowserSlotView (including their private vars, methods like
setPaneDropContext(_:), paneDropTargetForDrop(_:), setPortalDragDropZone(_:),
setDropZoneOverlay(zone:), handleDroppedFileURLsAsText(_:at:), and any
DEBUG-only helpers such as logHitTestDecision and clearDragState) into a new
Sources/Swift file, preserve all access levels, imports, and conditional
compilation blocks, remove their definitions from the original file, ensure the
new file is added to the build target, and run a build to fix any missing
references (update any fileprivate/internal usage if needed to maintain
visibility across files).
---
Duplicate comments:
In `@Resources/Localizable.xcstrings`:
- Line 20: The Japanese localization for
"settings.app.fileDrop.defaultBehavior.text" uses the ambiguous term "パステキスト";
update the Japanese "value" to the clearer term "パス文字列" to match the adjacent
localization (line 19) so the label is unambiguous for Japanese users.
In `@Sources/ContentView.swift`:
- Around line 358-363: When early-returning to route the drop to text in
prepareForDragOperation, performDragOperation, and concludeDragOperation (where
you currently set
preparedDragWebView/preparedPaneDropTarget/activePaneDropTarget to nil), first
call draggingExited(sender) on any existing activeDragWebView and
activePaneDropTarget (and on preparedPaneDropTarget if set) to mirror the
cleanup done in draggingUpdated; then clear those references (and only then
return). This ensures any prior draggingEntered is balanced and avoids leaking
hover state; locate the cleanup in prepareForDragOperation,
performDragOperation, and the defer block in concludeDragOperation and add the
draggingExited(sender) calls before nil-ing
activeDragWebView/activePaneDropTarget/preparedPaneDropTarget.
- Around line 10-142: The file is over budget; extract FileDropHintBadgeView,
FileDropPaneTarget protocol, and the
PaneDropTargetView/BrowserPaneDropTargetView extensions into separate source
files (e.g., Sources/FileDropHintBadgeView.swift and
Sources/FileDropPaneTarget.swift), remove the leading private modifiers so they
are internal (omit private) so FileDropOverlayView and other types can access
them, and ensure the new files import AppKit if needed and preserve all method
and type names (FileDropHintBadgeView, configureNativeGlassIfNeeded,
FileDropPaneTarget, and the two extensions) exactly so existing references
continue to compile.
- Around line 564-566: textDropDestinationKindUnderPoint currently returns
.editor whenever webViewUnderPoint(windowPoint) != nil, causing editor-specific
flows to run for non-editable WKWebView content; change the logic to consult a
cached “webViewIsEditable” boolean that you populate during draggingUpdated by
using the existing preparedDragWebView pattern and calling
preparedDragWebView.evaluateJavaScript("document.elementFromPoint(x,y)...") (or
equivalent elementFromPoint check) with a completion handler to determine if the
element is editable, store that result on the drag state (cache), and then have
textDropDestinationKindUnderPoint return .editor only when
webViewUnderPoint(...) != nil AND the cached webViewIsEditable is true; update
related callers (updateHintBadge, shouldRouteFileDropToTextDestination,
performFileDropAsText / FileDropTextInsertion.insert) to rely on that cached
value so non-editable web content falls through to native/terminal drop
handling.
- Around line 614-619: The fallback in editableTextViewUnderPoint currently
returns the window's firstResponder field editor unconditionally, which routes
drops to off-cursor editors; change the fallback to only return the field editor
if its frame actually contains the windowPoint (i.e., convert the fieldEditor’s
bounds to window coordinates and check contains(windowPoint)), otherwise return
nil—this keeps textDropDestinationKindUnderPoint and performFileDropAsText
driven by the hit-test walk and prevents inserting into an off-cursor
NSTextView.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 9254-9259: The method handleDroppedFileURLsAsText currently
returns true even when terminalSurface is nil and sendText is a no-op; change it
to verify a real target before claiming success: compute text with
TerminalImageTransferPlanner.insertedText(forFileURLs:), guard that text is
non-empty and that terminalSurface is non-nil (or otherwise call a
sendText-returning API and propagate its Bool), then call
terminalSurface.sendText(text) and return true only when a valid surface handled
the text—return false if terminalSurface is missing or the send did not actually
occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8b56e3f-a016-4e12-b3c1-b3d818160ae5
📒 Files selected for processing (9)
Resources/Localizable.xcstringsSources/BrowserWindowPortal.swiftSources/ContentView.swiftSources/DragOverlayRoutingPolicy.swiftSources/GhosttyTerminalView.swiftSources/Panels/FilePreviewPanel.swiftSources/TerminalPaneDropTargetView.swiftcmuxTests/BrowserPaneDropRoutingTests.swiftcmuxTests/FinderFileDropRegressionTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 361-366: Cache the result of textDropDestinationKindUnderPoint for
each drag tick and thread it into callers instead of calling it twice;
specifically, compute let cachedKind =
textDropDestinationKindUnderPoint(sender.draggingLocation) once in
prepareForDragOperation and in draggingUpdated, pass cachedKind into
shouldRouteFileDropToTextDestination and updateHintBadge (or add an optional
parameter/overload to shouldRouteFileDropToTextDestination and updateHintBadge
to accept a precomputed drop kind) so they use cachedKind rather than invoking
textDropDestinationKindUnderPoint again; ensure the side effect that toggles
isHidden and the hitTest traversal only runs once per tick by using the cached
value everywhere within that drag event.
- Around line 109-114: The call to the private-API IMP via unsafeBitCast
(selector "setCornerRadius:" / CornerRadiusSetter) is redundant or unsafe;
either remove the block that looks up and calls the IMP in
configureNativeGlassIfNeeded (rely on layer?.cornerRadius = 13 set earlier) or,
if empirical testing shows NSGlassEffectView requires the direct setter, keep
the IMP path but add a short comment above it explaining why layer.cornerRadius
is insufficient for NSGlassEffectView rendering and that the `@convention`(c)
(AnyObject, Selector, CGFloat) signature is a best-effort assumption for 64-bit
macOS so future maintainers understand the risk.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45ab3403-b850-4f13-a560-ce2a304a3565
📒 Files selected for processing (1)
Sources/ContentView.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
Sources/ContentView.swift (4)
109-115: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRedundant private-API
setCornerRadius:IMP path.
effectView.layer?.cornerRadius = 13is already applied at line 39 immediately beforeconfigureNativeGlassIfNeeded(_:). IfNSGlassEffectViewhonors the backing layer'scornerRadius, theunsafeBitCast-of-IMP block is dead code with a fragile signature assumption (@convention(c) (AnyObject, Selector, CGFloat) -> Void). If empirical testing actually requires it, leave a one-line comment explaining why the layer property is insufficient so this isn't accidentally removed later.🤖 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/ContentView.swift` around lines 109 - 115, The IMP invocation for "setCornerRadius:" in Sources/ContentView.swift is redundant and fragile because effectView.layer?.cornerRadius = 13 is already set (see configureNativeGlassIfNeeded(_:)) and the unsafeBitCast assumes a specific C calling convention and signature; remove the entire NSSelectorFromString/unsafeBitCast block (including the CornerRadiusSetter usage and setter call). If testing shows the layer property truly doesn't affect NSGlassEffectView, keep the IMP path but replace it with a single-line comment above the block explaining why the layer setter is insufficient and why the private-API IMP call is required.
565-576:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
textDropDestinationKindUnderPointreturns.editorfor anyWKWebView, regardless of whether the element under the cursor is editable.Already raised previously:
WKWebView.evaluateJavaScript(_:completionHandler:)is asynchronous, soFileDropTextInsertion.insertcannot synchronously confirm editability. As written, both the hint badge andperformFileDropAsTextwill treat any browser-pane web view (e.g., a non-editable page) as an editor, consuming the drop and breaking the page's native drop handling. Probe editability duringdraggingUpdatedviaevaluateJavaScript(_:completionHandler:)and cache the result onpreparedDragWebView(or a sibling flag), then use the cached editability at drop time instead of re-runningwebViewUnderPoint(...).🤖 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/ContentView.swift` around lines 565 - 576, textDropDestinationKindUnderPoint currently treats any WKWebView as editable because webViewUnderPoint is synchronous; update the logic to rely on a cached editability flag set during draggingUpdated instead of calling webViewUnderPoint directly: in draggingUpdated, call WKWebView.evaluateJavaScript(_:completionHandler:) to probe document activeElement or isContentEditable and store the result on preparedDragWebView (or a sibling Bool), then change textDropDestinationKindUnderPoint to check editableTextViewUnderPoint(windowPoint), the cached preparedDragWebView editability flag for the web view under point, and terminalUnderPoint in that order; also ensure FileDropTextInsertion.insert uses the same cached flag rather than re-evaluating editability synchronously.
10-117: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winFile-length budget — extract
FileDropHintBadgeViewto its own file.This was raised previously and the badge view (plus its private-API glass setup) is still inlined here. Per the established pattern for
Sources/ContentView.swift, moveFileDropHintBadgeView(andFileDropTextDestinationKindif file-private here) into a dedicatedSources/FileDropHintBadgeView.swiftto stay under the CI file-length threshold.Based on learnings: this repo's CI enforces a Swift file-length budget for large view files (e.g.,
Sources/ContentView.swift); helper subviews/components should be extracted to a dedicated file underSources/(PR 3502).🤖 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/ContentView.swift` around lines 10 - 117, Extract the FileDropHintBadgeView class (and any related file-private types like FileDropTextDestinationKind if present) into a new Sources/FileDropHintBadgeView.swift file: create the new file, add the necessary imports (AppKit/Foundation as used), copy the full FileDropHintBadgeView implementation including configureNativeGlassIfNeeded and preserve its access level and `@available` annotations, and remove the inlined class from Sources/ContentView.swift so ContentView references the type from the new file; ensure any file-private helpers used only by that class remain file-private in the new file and update any unresolved references in ContentView (none if you removed only the class) so the project builds.
361-366: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
textDropDestinationKindUnderPointis still invoked multiple times per drag tick.Already raised previously and still present:
prepareForDragOperation(lines 361 and 365) hit-tests twice.draggingUpdatedcallsupdateHintBadge(line 475) — which hit-tests at line 551 — and thenshouldRouteFileDropToTextDestination(line 477) — which hit-tests again at line 539.- The
nil-vs-non-nilbranch on line 486 redundantly re-runs the same hit-test, and is unreachable in thenilbranch (the routing predicate already requiredcanDropAsText).Each invocation toggles
isHiddenon the overlay and walkscontentView.hitTestat ~60 Hz. Cache the resolvedFileDropTextDestinationKind?once per call and thread it intoshouldRouteFileDropToTextDestinationandupdateHintBadge(e.g., overloads accepting a precomputedcanDropAsText/kind).Also applies to: 475-489, 538-545
🤖 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/ContentView.swift` around lines 361 - 366, The hit-test FileDropTextDestinationKind? is being computed multiple times per drag tick (textDropDestinationKindUnderPoint called from prepareForDragOperation, draggingUpdated, shouldRouteFileDropToTextDestination, and updateHintBadge); compute it once at the start of the drag-handling path (e.g., in prepareForDragOperation/draggingUpdated), store it in a local FileDropTextDestinationKind? variable, and thread that cached value into shouldRouteFileDropToTextDestination and updateHintBadge by adding overloads or optional parameters that accept the precomputed kind (or canDropAsText Bool), then remove the duplicate calls to textDropDestinationKindUnderPoint and stop toggling overlay.isHidden based on redundant hit-tests. Ensure all call sites (prepareForDragOperation, draggingUpdated, and any branches that previously re-run the test) use the cached value.
🤖 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.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 446-465: concludeDragOperation is recomputing the routing decision
and can diverge from what performDragOperation actually did; fix by having
performDragOperation record the routing outcome (e.g. set a Bool property like
didRouteFileDropToTextDestination or an enum on the view/controller) when it
chooses the text vs pane path, and then have concludeDragOperation consume that
stored decision instead of calling shouldRouteFileDropToTextDestination(sender)
again; update performDragOperation to set the flag when it calls text drop
handling or paneDropTarget.fileDropPerformDragOperation, and change
concludeDragOperation to guard/branch on that stored flag and then call
paneDropTarget.fileDropConcludeDragOperation(sender) only if the stored decision
indicates the pane path was used, clearing the stored flag as part of the
existing defer cleanup.
---
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 109-115: The IMP invocation for "setCornerRadius:" in
Sources/ContentView.swift is redundant and fragile because
effectView.layer?.cornerRadius = 13 is already set (see
configureNativeGlassIfNeeded(_:)) and the unsafeBitCast assumes a specific C
calling convention and signature; remove the entire
NSSelectorFromString/unsafeBitCast block (including the CornerRadiusSetter usage
and setter call). If testing shows the layer property truly doesn't affect
NSGlassEffectView, keep the IMP path but replace it with a single-line comment
above the block explaining why the layer setter is insufficient and why the
private-API IMP call is required.
- Around line 565-576: textDropDestinationKindUnderPoint currently treats any
WKWebView as editable because webViewUnderPoint is synchronous; update the logic
to rely on a cached editability flag set during draggingUpdated instead of
calling webViewUnderPoint directly: in draggingUpdated, call
WKWebView.evaluateJavaScript(_:completionHandler:) to probe document
activeElement or isContentEditable and store the result on preparedDragWebView
(or a sibling Bool), then change textDropDestinationKindUnderPoint to check
editableTextViewUnderPoint(windowPoint), the cached preparedDragWebView
editability flag for the web view under point, and terminalUnderPoint in that
order; also ensure FileDropTextInsertion.insert uses the same cached flag rather
than re-evaluating editability synchronously.
- Around line 10-117: Extract the FileDropHintBadgeView class (and any related
file-private types like FileDropTextDestinationKind if present) into a new
Sources/FileDropHintBadgeView.swift file: create the new file, add the necessary
imports (AppKit/Foundation as used), copy the full FileDropHintBadgeView
implementation including configureNativeGlassIfNeeded and preserve its access
level and `@available` annotations, and remove the inlined class from
Sources/ContentView.swift so ContentView references the type from the new file;
ensure any file-private helpers used only by that class remain file-private in
the new file and update any unresolved references in ContentView (none if you
removed only the class) so the project builds.
- Around line 361-366: The hit-test FileDropTextDestinationKind? is being
computed multiple times per drag tick (textDropDestinationKindUnderPoint called
from prepareForDragOperation, draggingUpdated,
shouldRouteFileDropToTextDestination, and updateHintBadge); compute it once at
the start of the drag-handling path (e.g., in
prepareForDragOperation/draggingUpdated), store it in a local
FileDropTextDestinationKind? variable, and thread that cached value into
shouldRouteFileDropToTextDestination and updateHintBadge by adding overloads or
optional parameters that accept the precomputed kind (or canDropAsText Bool),
then remove the duplicate calls to textDropDestinationKindUnderPoint and stop
toggling overlay.isHidden based on redundant hit-tests. Ensure all call sites
(prepareForDragOperation, draggingUpdated, and any branches that previously
re-run the test) use the cached value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 927a39b4-184f-44b1-8562-14fe7ed71a5b
📒 Files selected for processing (6)
Sources/BrowserWindowPortal.swiftSources/ContentView.swiftSources/DragOverlayRoutingPolicy.swiftSources/Panels/FilePreviewPanel.swiftSources/TerminalPaneDropTargetView.swiftcmuxTests/FinderFileDropRegressionTests.swift
Stale bot review addressed in later commits on #3684; current checks are green and current-head review threads are resolved.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 59e88e0. Configure here.
| // The window overlay delegates Finder/sidebar files to pane-level Bonsplit targets. | ||
| _ = hasLocalDraggingSource | ||
| guard hasFileURL(pasteboardTypes) else { return false } | ||
| guard hasFileDropPayload(pasteboardTypes) else { return false } |
There was a problem hiding this comment.
Overlay captures file preview drags it cannot process
Medium Severity
shouldCaptureFileDropDestination now uses hasFileDropPayload which returns true for filePreviewTransferType, but FileDropOverlayView only registers for PasteboardFileURLReader.fileURLPasteboardTypes. When a pure internal file-preview drag (no real file URLs) is active, shouldCaptureFileDropOverlay returns true (capturing the hit test), but AppKit won't deliver drag destination methods to the overlay since it's unregistered for that type. This can block BrowserPaneDropTargetView from receiving the drag, potentially causing file-preview drags to silently fail when the overlay is installed.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 59e88e0. Configure here.
| if let terminal = terminalUnderPoint(windowPoint) { | ||
| return insert(urls, into: terminal) | ||
| } | ||
| return false |
There was a problem hiding this comment.
Redundant text computation in performFileDropAsText terminal path
Low Severity
performFileDropAsText computes text via TerminalImageTransferPlanner.insertedText(forFileURLs:) and guards on it being non-empty, but for the terminal path it calls insert(urls, into: terminal) which calls handleDroppedFileURLsAsText that recomputes the same text internally. The pre-computed text variable is unused in the terminal branch, wasting the string allocation and shell-escaping work.
Reviewed by Cursor Bugbot for commit 59e88e0. Configure here.
| guard let dragId = FilePreviewDragPasteboardWriter.dragID(from: pasteboard), | ||
| let entry = FilePreviewDragRegistry.shared.entry(id: dragId) else { | ||
| return [] | ||
| } | ||
| return [URL(fileURLWithPath: entry.filePath).standardizedFileURL] | ||
| } |
There was a problem hiding this comment.
/private/… paths inserted into the terminal
fileURLs(from:) constructs file-preview-transfer URLs as URL(fileURLWithPath: entry.filePath).standardizedFileURL. standardizedFileURL resolves the /tmp → /private/tmp symlink, so a file at /tmp/foo.txt yields .path == "/private/tmp/foo.txt". That URL is then passed straight to TerminalImageTransferPlanner.insertedText(forFileURLs:) which uses url.path without any display-path normalization. The result is that dragging a file-preview tab with a /tmp/… path into a terminal inserts /private/tmp/foo.txt instead of the user-visible /tmp/foo.txt. This is a new code path introduced by this PR (file-preview drags previously never inserted text). The same macOSDisplayPath normalization already applied in FileExplorerTerminalPathInsertion should be applied here, either inside fileURLs(from:) or inside insertedText(forFileURLs:).
Merged manaflow-ai/cmux#3684. Implements configurable file-drop routing so text-capable panes default to text insertion while Shift creates the split preview path, with shared pane-drop routing, focus restoration, and localized strings across all supported languages.
This branch was successfully deployed
1 participant
Summary
Testing
jq empty Resources/Localizable.xcstringsgit diff --checkxcodebuild test -quiet -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-dropdry-unit -only-testing:cmuxTests/FinderFileDropRegressionTests -only-testing:cmuxTests/BrowserPaneDropRoutingTests -only-testing:cmuxTests/FileDropOverlayViewTests -only-testing:cmuxTests/PortalTabDragRoutingTests\n-./scripts/reload.sh --tag dropdry\n\n## Issues\n- Task: default file drops should insert path text into interactive destinations, Shift should create the preview/split, non-interactive destinations should show the blue split target, dropped text should focus its destination, and the behavior should be configurable in Settings.\n\n\n\n\n## Summary by CodeRabbit\n\n* New Features\n * Configurable "File Drops" setting to choose default handling.\n * Option to route file drops as plain-text paths into editor or terminal; Shift toggles behavior.\n * Drag hint badge shows resolved drop destination (editor vs terminal).\n * File paths inserted from explorer are normalized to macOS-friendly display paths.\n\n* Documentation\n * Localized hint strings added for English and Japanese.\n\n* Tests\n * New tests covering file-drop routing, Shift behavior, modifier merging, and path insertion.\n\nNote
Medium Risk
Moderate risk because it rewires drag-and-drop hit-testing/routing across terminal panes, browser panes, and the window overlay, which can easily regress drop handling and focus behavior.
Overview
Changes file-drop behavior to default to inserting shell-escaped path text into interactive destinations (terminal surfaces, browser web views/editable inputs, and text file-preview editors), with Shift inverting to the preview/split behavior.
This refactors drag/drop plumbing by extracting shared pane drop routing utilities (
PaneDropRoutingSupport), adding a dedicatedBrowserPaneDropTargetView, and reworkingFileDropOverlayViewto route between pane targets and web views, show a new hint badge, and restore focus after successful text drops.Adds an app setting (
File Drops) with new localizations, extends file-preview drag registry/pasteboard handling to resolve URLs, normalizes file path display (/private/...to/...), and updates/expands unit tests for the new routing and web-view lifecycle behavior.Reviewed by Cursor Bugbot for commit 59e88e0. Bugbot is set up for automated code reviews on this repo. Configure here.