Repository navigation
Fix file preview drop routing - #3539
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughWindow-level file-URL drags can now route to pane-level drop targets; terminal-specific pane types were generalized to Pane* types. File-drop overlay hit-testing, portal-hosted resolution, pane-target lifecycle, drop routing, and file-preview theming/editor were added or extended. ChangesPane & File-Drop Routing, Portal Hit-Testing, and File-Preview Theming
Sequence DiagramsequenceDiagram
participant User as "User"
participant Overlay as "FileDropOverlayView"
participant Portal as "TerminalWindowPortal"
participant Pane as "PaneDropTargetView"
participant Workspace as "Workspace"
participant Preview as "FilePreviewPanel"
User->>Overlay: Drag file(s) over window
Overlay->>Overlay: hitTest() / shouldDeferFileDropOverlayToBonsplitTabBar
alt Defer to tab bar
Overlay-->>User: no capture (deferred)
else Capture file drop
Overlay->>Portal: paneDropTargetUnderPoint(windowPoint)
Portal->>Pane: paneDropTargetForDrop(localPoint)
Pane-->>Portal: target or nil
Portal-->>Overlay: resolved PaneDropTargetView
User->>Overlay: draggingEntered / prepareForDragOperation
Overlay->>Pane: prepareForDragOperation(sender)
Pane->>Pane: extract fileURLs, compute zone, activate overlay
User->>Overlay: performDragOperation (drop)
Overlay->>Pane: performDragOperation(sender)
Pane->>Workspace: handleExternalFileDrop(fileURLs, destination)
Workspace-->>Pane: success/failure
Overlay->>Pane: concludeDragOperation
Pane->>Pane: clear overlay/state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 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 |
Contributor
Greptile SummaryThis PR introduces a shared pane-level file-drop adapter (
Confidence Score: 4/5Safe to merge for Finder-to-pane drops; in-app file-preview tab drags to non-pane targets (sidebar pins, new-window edges) may be silently blocked. The core pane-level routing for Finder drops and the terminal snapshot fix look correct and are well-tested. The main concern is in Sources/DragOverlayRoutingPolicy.swift — the Important Files Changed
Sequence DiagramsequenceDiagram
participant Finder as Finder / Sidebar
participant Overlay as FileDropOverlayView
participant Policy as DragOverlayRoutingPolicy
participant PaneDT as PaneDropTargetView
participant Portal as TerminalWindowPortal
participant WS as Workspace
Finder->>Overlay: file URL drag (hitTest)
Overlay->>Policy: shouldCaptureFileDropDestination(types, hasLocal)
Policy-->>Overlay: true (all file-URL drags captured)
Overlay->>Overlay: shouldDeferToBonsplitTabBar?
alt over tab bar
Overlay-->>Finder: nil (defer to bonsplit)
else over pane
Overlay->>Overlay: inlinePaneDropTargetUnderPoint
alt found SwiftUI PaneDropTargetView
Overlay->>PaneDT: draggingEntered / draggingUpdated
PaneDT-->>Overlay: .copy
else terminal pane
Overlay->>Portal: terminalPaneDropTargetAtWindowPoint
Portal-->>Overlay: PaneDropTargetView
Overlay->>PaneDT: draggingEntered / draggingUpdated
end
Overlay->>PaneDT: performDragOperation
PaneDT->>WS: handleExternalFileDrop(urls, destination)
WS-->>PaneDT: handled
end
Reviews (4): Last reviewed commit: "Clear stale pane drop state" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
443
to
+448
| } | ||
| let webView = preparedDragWebView ?? activeDragWebView ?? webViewUnderPoint(sender.draggingLocation) | ||
| webView?.concludeDragOperation(sender) | ||
| if let paneDropTarget = preparedPaneDropTarget ?? activePaneDropTarget { | ||
| paneDropTarget.draggingExited(sender) | ||
| } |
Contributor
There was a problem hiding this comment.
concludeDragOperation calls draggingExited on a semantically wrong path — and the path is dead in practice.
performDragOperation always nils out both preparedPaneDropTarget and activePaneDropTarget before delegating to the pane target, so preparedPaneDropTarget ?? activePaneDropTarget is always nil by the time concludeDragOperation runs after a successful drop. The draggingExited call is therefore unreachable via the normal AppKit lifecycle (prepare → perform → conclude). Even if the guard were reachable, draggingExited is the wrong signal here — the correct delegate call on conclusion of a successful drop is concludeDragOperation, not draggingExited. PaneDropTargetView already handles its own cleanup in performDragOperation via defer { clearDragState(...) }, so this block does nothing useful.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/Panels/FilePreviewPanel.swift (1)
928-1004: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExtract
FilePreviewTextEditorinto its own Swift file.This file is now over the enforced CI budget (
3965 > 3933). Moving the representable plus its theming helper out ofFilePreviewPanel.swiftkeeps the feature intact and unblocks the PR.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/Panels/FilePreviewPanel.swift` around lines 928 - 1004, Create a new Swift file and move the entire FilePreviewTextEditor NSViewRepresentable (including its nested Coordinator type and the private static applyTheme helper) into it, adding required imports (SwiftUI, AppKit) and keeping the same access level so references from FilePreviewPanel (e.g., panel: FilePreviewPanel, SavingTextView, makeCoordinator, applyTheme) still compile; then remove the struct from FilePreviewPanel.swift and ensure FilePreviewPanel.attachTextView/textContent usages remain unchanged so the build passes under the CI file-length budget.cmuxTests/WindowAndDragTests.swift (1)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCI blocker: trim 4 lines to satisfy the Swift file-length budget.
The pipeline reports:
Swift file length budget exceeded for this file group: actual=2867, budget=2863 (+4).The net line addition from this PR's two updated test segments is exactly 4 lines over the budget. The quickest fix is to collapse any redundant blank lines (the file has several consecutive double-blank separators between classes/test suites). For example, reducing the four double-blank separators immediately surrounding the new
FileDropOverlayViewTestsblock to single-blank separators would recover the needed lines without changing any logic.🔧 Example trimming approach (representative, not exhaustive)
// ... end of FilePreviewDragPasteboardWriterTests } - `@MainActor` final class FilePreviewPanelTextSavingTests: XCTestCase {Apply the same pattern to the other double-blank separators in the file until 4 lines are recovered.
🤖 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 `@cmuxTests/WindowAndDragTests.swift` at line 1, Trim four redundant blank lines around the new test block to satisfy the Swift file-length budget: collapse the four double-blank separators immediately surrounding the FileDropOverlayViewTests test suite into single blank lines (reduce consecutive empty lines to a single empty line) so the file length drops by four lines without changing any code or test logic; search for the FileDropOverlayViewTests declaration and adjust the blank-line separators before and after that class (and any immediately adjacent double-blank separators) accordingly.
🤖 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 37-52: Extract the standalone drop-routing helpers (notably the
static func fileURLs(from:) and the related helpers referenced around the
676–712 region) out of ContentView.swift into their own Swift source file as a
same-type extension so ContentView.swift stays under the file-length budget;
move the functions verbatim, keep their signatures and behavior unchanged, and
do not mark them private if they must be accessed from other files.
- Around line 377-382: preparedPaneDropTarget and activePaneDropTarget are being
cleared too early and prepareForDragOperation(_:) is not being forwarded to the
pane target, breaking the NSDraggingDestination lifecycle; update the drag
handling so that when a paneDropTarget is found you call its
prepareForDragOperation(_:) (matching the webView path) and do NOT nil
preparedPaneDropTarget/activePaneDropTarget before invoking the pane target's
performDragOperation(_:) — instead defer clearing these references until after
concludeDragOperation(_:) completes (or defer/perform the target's cleanup
before you nil the refs) so draggingExited(sender) on TerminalPaneDropTargetView
will be called.
In `@Sources/Panels/PanelContentView.swift`:
- Around line 21-26: The pane-drop overlay (PaneDropTargetRepresentable) is
being mounted even for offscreen panels; update PanelContentView so the overlay
is only attached when the panel is visible by gating on isVisibleInUI — e.g.
replace the unconditional .overlay { paneDropTargetOverlay } with a conditional
overlay using if isVisibleInUI { paneDropTargetOverlay } (or return an EmptyView
otherwise). Apply the same visibility check to the other occurrence around lines
81–99 where PaneDropTargetRepresentable is mounted so hidden panels no longer
participate in AppKit drag hit-testing.
In `@Sources/TerminalWindowPortal.swift`:
- Around line 2119-2125: Move the newly added pane-drop-target logic out of
Sources/TerminalWindowPortal.swift into a new extension file (e.g.
Sources/TerminalWindowPortal+PaneDropTarget.swift) to satisfy the file-length
budget: create an extension on WindowTerminalPortal that implements
terminalPaneDropTargetAtWindowPoint(_:) (keeping the same implementation that
converts window point to hostView coordinates, iterates
hostView.subviews.reversed(), filters GhosttySurfaceScrollView entries via
entriesByHostedId, checks isHidden and frame.contains, converts to localPoint
and calls paneDropTargetForDrop(at:)), and add the static registry wrapper
extension on TerminalWindowPortalRegistry that delegates
terminalPaneDropTargetAtWindowPoint(_:in:) to portal(for:). Ensure imports
(AppKit) and symbol names remain unchanged so behavior is identical.
In `@Sources/WorkspaceContentView.swift`:
- Around line 235-238: WorkspaceContentView.swift exceeds the CI line-count by
one; extract a small nested helper into its own file to reduce length —
specifically move the PanelContentView declaration out of
WorkspaceContentView.swift into a new Sources/PanelContentView.swift, preserve
its signature and any used properties (workspaceId, paneId), update access
level/imports as needed, and remove the nested type from WorkspaceContentView so
all references (PanelContentView(...) usage) continue to compile.
In `@vendor/bonsplit`:
- Line 1: The submodule "bonsplit" is pinned to the unmerged PR HEAD commit
aa4f69723ea1a5a59df9dc0c9e7bbdfb1f96abc1 (PR `#115`) which is not reachable from
main; fix by repinning the submodule to a stable commit on the default branch
(main) or by first merging PR `#115` into main and then updating the submodule
reference, i.e., update the gitlink to a commit that exists on main (or merge
and re-run git submodule update --init && git add the submodule change and
commit) so the repository no longer references an open-PR-only SHA.
---
Outside diff comments:
In `@cmuxTests/WindowAndDragTests.swift`:
- Line 1: Trim four redundant blank lines around the new test block to satisfy
the Swift file-length budget: collapse the four double-blank separators
immediately surrounding the FileDropOverlayViewTests test suite into single
blank lines (reduce consecutive empty lines to a single empty line) so the file
length drops by four lines without changing any code or test logic; search for
the FileDropOverlayViewTests declaration and adjust the blank-line separators
before and after that class (and any immediately adjacent double-blank
separators) accordingly.
In `@Sources/Panels/FilePreviewPanel.swift`:
- Around line 928-1004: Create a new Swift file and move the entire
FilePreviewTextEditor NSViewRepresentable (including its nested Coordinator type
and the private static applyTheme helper) into it, adding required imports
(SwiftUI, AppKit) and keeping the same access level so references from
FilePreviewPanel (e.g., panel: FilePreviewPanel, SavingTextView,
makeCoordinator, applyTheme) still compile; then remove the struct from
FilePreviewPanel.swift and ensure FilePreviewPanel.attachTextView/textContent
usages remain unchanged so the build passes under the CI file-length budget.
🪄 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: b4601856-c665-4bb7-91f3-d49f1235d194
📒 Files selected for processing (12)
Sources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/Panels/FilePreviewPanel.swiftSources/Panels/PanelContentView.swiftSources/Panels/TerminalPanelView.swiftSources/TerminalPaneDropTargetView.swiftSources/TerminalWindowPortal.swiftSources/Workspace.swiftSources/WorkspaceContentView.swiftcmuxTests/PortalTabDragRoutingTests.swiftcmuxTests/WindowAndDragTests.swiftvendor/bonsplit
Comment on lines
+2119
to
+2125
| static func terminalPaneDropTargetAtWindowPoint( | ||
| _ windowPoint: NSPoint, | ||
| in window: NSWindow | ||
| ) -> TerminalPaneDropTargetView? { | ||
| let portal = portal(for: window) | ||
| return portal.terminalPaneDropTargetAtWindowPoint(windowPoint) | ||
| } |
There was a problem hiding this comment.
CI blocker: Swift file-length budget exceeded after adding this API.
The current CI run fails on file-length budget for Sources/TerminalWindowPortal.swift (+27 lines). Please move the newly added pane-drop-target methods into a separate extension file (same module) to unblock merge without changing behavior.
Suggested extraction sketch
--- a/Sources/TerminalWindowPortal.swift
+++ b/Sources/TerminalWindowPortal.swift
@@
- func terminalPaneDropTargetAtWindowPoint(_ windowPoint: NSPoint) -> TerminalPaneDropTargetView? {
- guard ensureInstalled() else { return nil }
- let point = hostView.convert(windowPoint, from: nil)
-
- for subview in hostView.subviews.reversed() {
- guard let hostedView = subview as? GhosttySurfaceScrollView else { continue }
- let hostedId = ObjectIdentifier(hostedView)
- guard entriesByHostedId[hostedId] != nil else { continue }
- guard !hostedView.isHidden else { continue }
- guard hostedView.frame.contains(point) else { continue }
- let localPoint = hostedView.convert(point, from: hostView)
- if let target = hostedView.paneDropTargetForDrop(at: localPoint) {
- return target
- }
- }
-
- return nil
- }
@@
- static func terminalPaneDropTargetAtWindowPoint(
- _ windowPoint: NSPoint,
- in window: NSWindow
- ) -> TerminalPaneDropTargetView? {
- let portal = portal(for: window)
- return portal.terminalPaneDropTargetAtWindowPoint(windowPoint)
- }// New file: Sources/TerminalWindowPortal+PaneDropTarget.swift
import AppKit
extension WindowTerminalPortal {
func terminalPaneDropTargetAtWindowPoint(_ windowPoint: NSPoint) -> TerminalPaneDropTargetView? {
guard ensureInstalled() else { return nil }
let point = hostView.convert(windowPoint, from: nil)
for subview in hostView.subviews.reversed() {
guard let hostedView = subview as? GhosttySurfaceScrollView else { continue }
let hostedId = ObjectIdentifier(hostedView)
guard entriesByHostedId[hostedId] != nil else { continue }
guard !hostedView.isHidden else { continue }
guard hostedView.frame.contains(point) else { continue }
let localPoint = hostedView.convert(point, from: hostView)
if let target = hostedView.paneDropTargetForDrop(at: localPoint) {
return target
}
}
return nil
}
}
extension TerminalWindowPortalRegistry {
static func terminalPaneDropTargetAtWindowPoint(
_ windowPoint: NSPoint,
in window: NSWindow
) -> TerminalPaneDropTargetView? {
let portal = portal(for: window)
return portal.terminalPaneDropTargetAtWindowPoint(windowPoint)
}
}Based on learnings: “This repo’s CI enforces a Swift file-length budget for large view files… extract the subview/type 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/TerminalWindowPortal.swift` around lines 2119 - 2125, Move the newly
added pane-drop-target logic out of Sources/TerminalWindowPortal.swift into a
new extension file (e.g. Sources/TerminalWindowPortal+PaneDropTarget.swift) to
satisfy the file-length budget: create an extension on WindowTerminalPortal that
implements terminalPaneDropTargetAtWindowPoint(_:) (keeping the same
implementation that converts window point to hostView coordinates, iterates
hostView.subviews.reversed(), filters GhosttySurfaceScrollView entries via
entriesByHostedId, checks isHidden and frame.contains, converts to localPoint
and calls paneDropTargetForDrop(at:)), and add the static registry wrapper
extension on TerminalWindowPortalRegistry that delegates
terminalPaneDropTargetAtWindowPoint(_:in:) to portal(for:). Ensure imports
(AppKit) and symbol names remain unchanged so behavior is identical.
| @@ -1 +1 @@ | |||
| Subproject commit f9b7618634677618c225120d5dd4d574ffaec2d3 | |||
| Subproject commit aa4f69723ea1a5a59df9dc0c9e7bbdfb1f96abc1 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the new bonsplit commit belongs to a merged PR (`#115`)
# and is reachable from the default branch.
# 1. Check PR `#115` state and merge commit
gh pr view 115 --repo manaflow-ai/bonsplit --json state,mergeCommit,headRefOid,mergedAt \
| jq '{state, mergedAt, mergeCommit: .mergeCommit.oid, headSha: .headRefOid}'
# 2. Verify the pinned SHA exists in the repo and show its commit message
gh api repos/manaflow-ai/bonsplit/commits/aa4f69723ea1a5a59df9dc0c9e7bbdfb1f96abc1 \
--jq '{sha: .sha, message: .commit.message, date: .commit.author.date}'
# 3. Check whether this SHA is reachable from the default branch
DEFAULT_BRANCH=$(gh api repos/manaflow-ai/bonsplit --jq '.default_branch')
echo "Default branch: $DEFAULT_BRANCH"
gh api "repos/manaflow-ai/bonsplit/compare/${DEFAULT_BRANCH}...aa4f69723ea1a5a59df9dc0c9e7bbdfb1f96abc1" \
--jq '{status, ahead_by, behind_by}'Repository: manaflow-ai/cmux
Length of output: 383
Submodule is pinned to an unmerged PR feature branch commit; this will break silently if the PR is rebased, force-pushed, or abandoned.
The new SHA aa4f69723ea1a5a59df9dc0c9e7bbdfb1f96abc1 is the HEAD of PR #115, which is still OPEN and not merged into the default branch (main). The commit is currently not reachable from main (diverged status). If the PR is later rebased, force-pushed, or abandoned, this submodule reference will point to a non-existent or unreachable commit, silently breaking builds.
Either merge PR #115 into manaflow-ai/bonsplit main first, or pin the submodule to a stable commit already on the default branch.
🤖 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 `@vendor/bonsplit` at line 1, The submodule "bonsplit" is pinned to the
unmerged PR HEAD commit aa4f69723ea1a5a59df9dc0c9e7bbdfb1f96abc1 (PR `#115`) which
is not reachable from main; fix by repinning the submodule to a stable commit on
the default branch (main) or by first merging PR `#115` into main and then
updating the submodule reference, i.e., update the gitlink to a commit that
exists on main (or merge and re-run git submodule update --init && git add the
submodule change and commit) so the repository no longer references an
open-PR-only SHA.
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 `@cmuxTests/FileDropOverlayViewTests.swift`:
- Around line 179-182: This test binds a CmuxWebView to an anchor via
BrowserWindowPortalRegistry.bind but never detaches it, leaving global registry
state that can affect other tests; after
BrowserWindowPortalRegistry.synchronizeForAnchor(anchor) add a teardown call to
remove the binding—e.g., call BrowserWindowPortalRegistry.unbind(webView:
webView, from: anchor) (or the registry's appropriate remove/detach API) to
ensure the CmuxWebView binding is cleared at the end of the test.
In `@Sources/ContentView.swift`:
- Around line 286-289: The pane-target references (preparedPaneDropTarget and
activePaneDropTarget) are being cleared before delegation, so
concludeDragOperation has nothing to notify and the conclude path incorrectly
calls draggingExited; instead, preserve the paneDropTarget until the conclude
phase by not nil-ing preparedPaneDropTarget/activePaneDropTarget before calling
paneDropTarget.performDragOperation(sender), and ensure the conclude path
invokes paneDropTarget.concludeDragOperation (not draggingExited) so the
post-drop callback runs; apply the same change for the analogous block at the
later occurrence (the 313-315 area) referencing the same
preparedPaneDropTarget/activePaneDropTarget and paneDropTarget methods.
- Around line 244-246: In the prepareForDragOperation(_: ) branch, forward the
call to the pane destination instead of unconditionally marking it prepared:
call paneDropTarget.prepareForDragOperation(info) and use its boolean result to
decide whether to set preparedPaneDropTarget and return true; if the pane target
returns false, do not set preparedPaneDropTarget and return false. This uses the
existing symbols paneDropTarget, preparedPaneDropTarget and the
prepareForDragOperation(_:) method to match the WKWebView path.
In `@Sources/Panels/FilePreviewTextEditor.swift`:
- Around line 82-95: The Coordinator class and SavingTextView type are missing
explicit deinit methods required by the required_deinit rule; add an explicit
deinit to each (e.g., in final class Coordinator and in SavingTextView) —
implement deinit { /* cleanup if needed */ } for both, and if either holds
delegates or observers (e.g., NSTextView.delegate or NotificationCenter
observers) perform the appropriate removal inside their deinit to avoid leaks.
🪄 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: a25eb333-3e5c-4660-a944-dd484b532a46
📒 Files selected for processing (9)
GhosttyTabs.xcodeproj/project.pbxprojSources/ContentView.swiftSources/DragOverlayRoutingPolicy.swiftSources/Panels/FilePreviewPanel.swiftSources/Panels/FilePreviewTextEditor.swiftSources/TerminalWindowPortal.swiftSources/WorkspaceContentView.swiftcmuxTests/FileDropOverlayViewTests.swiftcmuxTests/WindowAndDragTests.swift
…-drop # Conflicts: # Sources/ContentView.swift # Sources/Workspace.swift # cmuxTests/WindowAndDragTests.swift # vendor/bonsplit
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
235-238:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear pane drop-target state when capture is declined
Line 235 and Line 277 return early on
!shouldCapture, but pane-target state is left untouched. That can keep stale routing state alive across drag sessions.💡 Suggested fix
guard shouldCapture else { preparedDragWebView = nil + preparedPaneDropTarget = nil + activePaneDropTarget = nil return false }guard shouldCapture else { preparedDragWebView = nil activeDragWebView = nil + preparedPaneDropTarget = nil + activePaneDropTarget = nil return false }Also applies to: 277-281
🤖 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 235 - 238, The early returns that check shouldCapture currently only nil out preparedDragWebView but leave pane drop-target routing state intact; update both guard branches (the one using shouldCapture around preparedDragWebView and the other at lines ~277-281) to also reset whatever variable(s) track the pane drop-target (clear the pane-target routing state alongside preparedDragWebView) so stale drop-target state is cleared when capture is declined.
🤖 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.
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 235-238: The early returns that check shouldCapture currently only
nil out preparedDragWebView but leave pane drop-target routing state intact;
update both guard branches (the one using shouldCapture around
preparedDragWebView and the other at lines ~277-281) to also reset whatever
variable(s) track the pane drop-target (clear the pane-target routing state
alongside preparedDragWebView) so stale drop-target state is cleared when
capture is declined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b18afd7-d5bb-4216-ba62-36b342a31c55
📒 Files selected for processing (9)
GhosttyTabs.xcodeproj/project.pbxprojSources/ContentView.swiftSources/DragOverlayRoutingPolicy.swiftSources/GhosttyTerminalView.swiftSources/Panels/FilePreviewTextEditor.swiftSources/Panels/PanelContentView.swiftSources/Workspace.swiftcmuxTests/FileDropOverlayViewTests.swiftcmuxTests/FinderFileDropRegressionTests.swift
Comment on lines
+37
to
+39
| _ = hasLocalDraggingSource | ||
| guard hasFileURL(pasteboardTypes) else { return false } | ||
| return true |
Contributor
There was a problem hiding this comment.
_ = hasLocalDraggingSource removes the guard that previously prevented the overlay from capturing in-app drags. Any drag that carries .fileURL — including file-preview tab drags (which advertise filePreviewTransferType + bonsplitTransferType + .fileURL) — now always hits shouldCaptureFileDropDestination = true. The overlay then calls paneDropTargetUnderPoint, which returns nil for non-pane areas like the sidebar, title bar, or window edges. With no target found and shouldDeferFileDropOverlayToBonsplitTabBar only covering the tab bar, the overlay returns [] and swallows the drag. File-preview tabs dragged to the sidebar to pin them or to a window edge to create a new window will silently fail to drop instead of being routed through bonsplit's native handling.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/ContentView.swift (1)
551-587: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExtract these drop-routing helpers out of
ContentView.swift.This helper block is self-contained and adds more non-view glue code to an already oversized file. Please move it into a same-type extension under
Sources/so this PR doesn’t reintroduce the file-length-budget problem onContentView.swift.Based on learnings: “This repo’s CI enforces a Swift file-length budget for large view files (e.g., Sources/ContentView.swift). When adding helper views/small components, avoid bloating the existing file: 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/ContentView.swift` around lines 551 - 587, Extract the drop-routing helpers (shouldDeferFileDropOverlayToBonsplitTabBar(at:), paneDropTargetUnderPoint(_:), inlinePaneDropTargetUnderPoint(_:), paneDropTarget(in:at:)) out of ContentView.swift into a new Swift file under Sources as a same-type extension (e.g., extension ContentView { ... }), preserving their access level (private) and any implicit use of window/contentView; import AppKit if needed and keep the same function signatures so callers compile; remove these methods from ContentView.swift and run a build to fix any missing references.
🤖 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.
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 551-587: Extract the drop-routing helpers
(shouldDeferFileDropOverlayToBonsplitTabBar(at:), paneDropTargetUnderPoint(_:),
inlinePaneDropTargetUnderPoint(_:), paneDropTarget(in:at:)) out of
ContentView.swift into a new Swift file under Sources as a same-type extension
(e.g., extension ContentView { ... }), preserving their access level (private)
and any implicit use of window/contentView; import AppKit if needed and keep the
same function signatures so callers compile; remove these methods from
ContentView.swift and run a build to fix any missing references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b8e46534-da96-4893-8993-47e6cec971e4
📒 Files selected for processing (1)
Sources/ContentView.swift
lawrencecchen
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
May 6, 2026 01:12
Superseded by latest CodeRabbit pass on a725315 after addressing the comments.
|
how to upload a screenshot to claude now?! |
austinywang
added a commit
that referenced
this pull request
May 6, 2026
Agent-hosted terminal panes were routed away from file preview, but their drops still flowed through the generic Ghostty file-drop handler. That handler pastes shell-escaped paths in a way that can leave Claude Code/Codex-style TUIs with literal path text instead of attachment state. This gives agent terminals a distinct pane routing result and uses the existing file URL parsing and remote-upload planner while delivering the final path text as an explicit bracketed paste. Plain shell terminals and non-terminal panels keep the file-preview route from PR #3539. Constraint: Issue #3615 requires agent terminal drops to attach images to the prompt while plain shell drops still open file preview Constraint: Local tests and builds are disallowed before CI passes Rejected: Reuse the generic terminal input route | dogfood showed it inserts literal paths instead of prompt attachments Confidence: medium Scope-risk: moderate Directive: Agent terminal file drops must stay separate from plain shell/file-preview routing Tested: git diff --check Not-tested: Local tests/builds not run per task policy; CI pending
This was referenced May 8, 2026
This was referenced May 18, 2026
5 tasks done
5 of 6 tasks
This branch was successfully deployed
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
2 participants
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.
Summary
Tests
Dogfood
Summary by cubic
Fixes file preview drag‑and‑drop so all file URL drags (Finder, sidebar, and in‑app) land in the hovered pane with zone-based insert/split, while browser uploads still hit the WKWebView. Also themes the text preview editor and preserves effective working directories when restoring terminal tabs.
Bug Fixes
bonsplittab drags.Dependencies
bonsplitto support inserting preview tabs from tab‑bar file drops.Written for commit a725315. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests