Repository navigation
Fix SSH image drops through terminal text route - #3755
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.
|
📝 WalkthroughWalkthroughThis PR consolidates terminal file-drop routing into a dedicated helper method, widens method visibility to enable cross-module access, exposes file-drop simulation via a new DEBUG RPC handler, and validates the functionality end-to-end with an SSH regression test. ChangesTerminal File Drop Simulation
Sequence Diagram(s)sequenceDiagram
participant TestClient
participant TerminalController
participant FileDropController
participant TerminalSurface
TestClient->>TerminalController: debug.terminal.simulate_file_drop(surface_id, paths, route)
TerminalController->>TerminalController: validate & resolve panel
alt route == terminal/direct
TerminalController->>TerminalSurface: debugSimulateFileDrop(paths)
else route == text_destination
TerminalController->>FileDropController: performTerminalFileDrop(workspace, panelId, hostedView, urls, window)
FileDropController->>TerminalSurface: handleDroppedFileURLs(urls)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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, 1 inconclusive)
✅ Passed checks (12 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 SummaryRoutes all terminal text-drop paths through the new
Confidence Score: 5/5Safe to merge; the core routing change is well-contained and the previously flagged dual-wiring issues are resolved. All three terminal drop entry points (overlay hit-test, pane drop target, debug RPC) now converge on Sources/GhosttyTerminalView.swift — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[File Drop Event] --> B{Drop Source}
B --> |Overlay hit-test| C[FileDropOverlayViewHitTesting\nperformFileDropAsText]
B --> |Pane drop target| D[TerminalPaneDropTargetView\nhandleFileDropAsText]
B --> |Debug RPC| E[TerminalController\nv2DebugSimulateTerminalFileDrop]
C --> F{Target under cursor}
F --> |NSTextView| G[insertedText check\n→ insert text]
F --> |GhosttyNSView terminal| H[performTerminalFileDrop\nterminal: overload]
D --> I[performTerminalFileDrop\nworkspace: overload]
E --> |route=text_destination| I
E --> |route=terminal| J[debugSimulateFileDrop\nNSPasteboard path]
H --> K{Workspace/panel\nresolvable?}
K --> |Yes| L[performPanelTextDrop\n→ focus + insert]
K --> |No fallback| M[handleDroppedFileURLs\ndirect]
I --> L
L --> N[handleDroppedURLs\n→ executePreparedImageTransfer]
N --> O{SSH remote?}
O --> |Yes| P[Upload to remote host]
O --> |No| Q[Local insert / path text]
Reviews (4): Last reviewed commit: "fix: tighten ssh drop debug helper" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static func performTerminalTextDrop( | ||
| workspace: Workspace, | ||
| panelId: UUID, | ||
| hostedView: GhosttySurfaceScrollView, | ||
| urls: [URL], | ||
| window: NSWindow? | ||
| ) -> Bool { | ||
| performPanelTextDrop( | ||
| workspace: workspace, | ||
| panelId: panelId, | ||
| focusIntent: .terminal(.surface), | ||
| window: window, | ||
| insert: { | ||
| hostedView.handleDroppedURLs(urls) | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
performTerminalTextDrop is a function inside FileDropTextDropController (all about text drops), but its closure now calls hostedView.handleDroppedURLs(urls), which routes through the full SSH-aware executePreparedImageTransfer planner — not text insertion. A future engineer reading the name "TextDrop" will assume this path is limited to text insertion and may incorrectly bypass it for binary/image files, or wire another caller through handleDroppedURLsAsText instead. The name should reflect the actual invariant (e.g., performTerminalFileDrop or performSSHAwareTerminalDrop).
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
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!
There was a problem hiding this comment.
Addressed in e8d197a by renaming performTerminalTextDrop to performTerminalFileDrop so the helper name matches the SSH-aware file upload behavior.
— Claude Code
| v2MainSync { | ||
| guard let panel = resolveTerminalPanel(from: surfaceId, tabManager: tabManager) else { | ||
| return | ||
| } | ||
|
|
||
| switch route { | ||
| case "terminal", "direct": | ||
| let handled = panel.hostedView.debugSimulateFileDrop(paths: paths) | ||
| result = handled | ||
| ? .ok(["handled": true, "route": "terminal"]) | ||
| : .err(code: "internal_error", message: "Terminal drop simulation failed", data: nil) | ||
| case "text", "text_destination", "pane_text": | ||
| guard let workspace = tabManager.tabs.first(where: { $0.id == panel.workspaceId }) else { | ||
| result = .err(code: "not_found", message: "Workspace not found", data: [ | ||
| "workspace_id": panel.workspaceId.uuidString | ||
| ]) | ||
| return | ||
| } | ||
| let urls = paths.map { URL(fileURLWithPath: $0).standardizedFileURL } | ||
| let handled = FileDropTextDropController.performTerminalTextDrop( | ||
| workspace: workspace, | ||
| panelId: panel.id, | ||
| hostedView: panel.hostedView, | ||
| urls: urls, | ||
| window: panel.hostedView.window | ||
| ) | ||
| result = handled | ||
| ? .ok(["handled": true, "route": "text_destination"]) | ||
| : .err(code: "internal_error", message: "Text destination drop simulation failed", data: nil) | ||
| default: | ||
| result = .err(code: "invalid_params", message: "Unknown route", data: [ | ||
| "route": route | ||
| ]) | ||
| } | ||
| } |
There was a problem hiding this comment.
The text_destination branch calls FileDropTextDropController.performTerminalTextDrop (which goes through hostedView.handleDroppedURLs → surfaceView.handleDroppedFileURLs), while FileDropOverlayViewHitTesting.insert(_:into:) calls terminal.handleDroppedFileURLs directly with its own separate focus-management block. Both paths invoke the SSH-aware planner, but they arrive there through independent wrappers with independent focus side-effects. If focus logic is updated in one path it must also be kept in sync with the other; the last pre-PR regression was caused by exactly this kind of divergence. The architectural fix would be a single shared entry point that owns both the planner dispatch and the subsequent focus transition — the new performTerminalTextDrop is a step in the right direction, but the FileDropOverlayViewHitTesting path does not yet use it.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
There was a problem hiding this comment.
Fixed in e8d197a by routing overlay terminal drops through FileDropTextDropController.performTerminalFileDrop, so pane, overlay, and debug text-destination drops use the shared terminal file-drop helper and focus path.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests_v2/test_ssh_remote_image_drop_upload.py`:
- Around line 124-125: Change the broad except Exception to catch
json.JSONDecodeError (or ValueError) specifically and re-raise the cmuxError
while preserving the original traceback using "from exc"; locate the except
block that currently does "except Exception as exc:" around the json.loads call
and update it to "except json.JSONDecodeError as exc:" (or ValueError if json
not imported) and then raise cmuxError(f"Invalid JSON output for {'
'.join(args)}: {proc.stdout!r} ({exc})") from exc so the original exception
chain is preserved.
🪄 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: ab4b1e5f-b7c6-4ad1-a647-33a7dc78a278
📒 Files selected for processing (7)
Sources/DragOverlayRoutingPolicy.swiftSources/FileDropOverlayViewHitTesting.swiftSources/GhosttyTerminalView.swiftSources/TerminalController.swiftSources/TerminalPaneDropTargetView.swifttests_v2/cmux.pytests_v2/test_ssh_remote_image_drop_upload.py
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/TerminalController.swift`:
- Line 3024: The capability string "debug.terminal.simulate_file_drop" in
TerminalController.swift is being advertised unconditionally; wrap its inclusion
in the same DEBUG compile condition as the dispatcher and implementation so it’s
only present in debug builds (e.g., enclose the capability entry in the same `#if`
DEBUG / `#endif` block used to register the dispatcher and implement the handler),
ensuring the advertised capability and the handler remain consistent across
build configurations.
In `@tests_v2/test_ssh_remote_image_drop_upload.py`:
- Around line 32-37: The subprocess helper _run currently calls subprocess.run
without a timeout and should be extended to accept a timeout parameter (e.g.,
timeout: float | None = DEFAULT_TIMEOUT) and pass it to subprocess.run, raising
on timeout; likewise update _run_cli_json to accept and forward the timeout.
Update all callers of _run and _run_cli_json in the test file to pass a sensible
default timeout (small value) and override it for slow operations like image
builds or docker pulls (longer timeout). Reference the helper functions _run and
_run_cli_json and ensure the timeout is threadable per-call so CI won't hang on
docker/ssh/cli wedges.
🪄 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: 6bd448bc-2d8c-4ac1-9951-17c416fd887a
📒 Files selected for processing (5)
Sources/DragOverlayRoutingPolicy.swiftSources/FileDropOverlayViewHitTesting.swiftSources/TerminalController.swiftSources/TerminalPaneDropTargetView.swifttests_v2/test_ssh_remote_image_drop_upload.py
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit aab9918. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/TerminalController.swift`:
- Around line 12429-12470: The route string (variable `route`) must be validated
before entering the `v2MainSync` block so an invalid route doesn't get masked by
the `resolveTerminalPanel(from:tabManager:)` guard; move or duplicate the
`switch route { ... default: result = .err(code: "invalid_params", message:
"Unknown route", data: ["route": route]) }` check to run immediately after
computing `route` and before calling `v2MainSync`, returning early on invalid
routes, otherwise proceed into `v2MainSync` and keep the existing panel
resolution and per-route handling (`terminal/direct` and
`text/text_destination/pane_text`) that set `result`.
🪄 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: 83c12790-967a-4047-a5ec-42a17940dc21
📒 Files selected for processing (2)
Sources/TerminalController.swifttests_v2/test_ssh_remote_image_drop_upload.py
This branch was successfully deployed
1 participant
Summary
Verification
Note
Medium Risk
Changes terminal file-drop routing and focus behavior, which could affect drag-and-drop handling across terminals/editors (especially for remote/SSH sessions). Added debug-only RPC and an end-to-end Docker-based regression test reduces but doesn’t eliminate behavioral risk.
Overview
Fixes terminal drop-as-text handling by routing terminal file drops through the existing image/file transfer path (including the SSH-aware upload planner), instead of a separate “insert paths as text” shortcut.
Introduces
FileDropTextDropController.performTerminalFileDrop(...)and updates overlay/pane drop hit-testing to use it, while removing the legacyhandleDropped*AsTextterminal APIs and exposingGhosttyNSView.handleDroppedFileURLsfor shared use.Adds a debug-only socket method
debug.terminal.simulate_file_drop(with selectable routing) plus a Python client wrapper, and a new Docker-backed regression test that validates SSH image drops upload correctly by comparing remote vs local SHA-256.Reviewed by Cursor Bugbot for commit 9c50452. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes image drag-and-drop into SSH terminals by routing all terminal drops through the SSH-aware upload planner. Adds a debug drop simulator and a Docker SSH regression test with bounded timeouts to avoid hangs.
Bug Fixes
FileDropTextDropController.performTerminalFileDrop(...)in both overlay hit-testing and pane drop handlers, so images dropped in SSH sessions upload to the remote host.GhosttyNSView.handleDroppedFileURLs(...)and removed the legacy “as text” path for terminal surfaces.New Features
debug.terminal.simulate_file_dropRPC withrouteselection (terminal/directortext_destination) and a Python wrapper intests_v2/cmux.py; now validates params and returns clear errors.tests_v2/test_ssh_remote_image_drop_upload.pywhich spins up a Docker SSH server and verifies the uploaded image hash on the remote, with bounded subprocess/CLI timeouts.Written for commit 9c50452. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests