Repository navigation
Fix SSH multi-image image drops - #3795
lawrencecchen wants to merge 2 commits into
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.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR extends clipboard image handling from single-image to multi-image materialization. It refactors ChangesMulti-Image Clipboard Drop & Materialization
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 (3 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 |
|
Closing this split because it still includes the multi-image SSH behavior. The intended first step, single-image SSH drag/upload, already landed in #3755. |
Greptile SummaryThis PR fixes a regression where dropping multiple images into an SSH-connected cmux terminal only uploaded the first image. The root cause was
Confidence Score: 3/5The multi-image extraction logic is well-structured and the regression test directly covers the fixed path, but the new public APIs that create and manage named NSPasteboards are missing @mainactor, leaving a static soundness gap for future callers. The core fix replaces a stop-at-first pattern with a full per-item scan, backed by both a unit regression and a tagged socket integration test. The actor isolation issue on new AppKit-stateful APIs is a real static guarantee gap. A secondary design concern: any single oversized image in a multi-image drop silently discards all valid images in the batch. Sources/GhosttyTerminalView.swift — specifically the new public multi-image APIs and pasteboardFallbackImageRepresentations; the actor isolation annotations and size-rejection semantics both warrant a closer look before merging. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["NSPasteboard (drop)"] --> B["imageRepresentations(in pasteboard:)"]
B --> C{"pasteboardItems?"}
C -- "for each item" --> D["imageRepresentations(in item:)"]
D --> E{"directImageRepresentation?"}
E -- yes --> F["[(data, ext)]"]
E -- no --> G{"rtfdAttachments?"}
G -- yes --> F
G -- no --> H{"fallbackImageRepresentation(item)?"}
H -- yes --> F
H -- no --> I["pasteboardFallbackImageRepresentations(for: item)"]
I --> J["copiedPasteboardItem to new named NSPasteboard"]
J --> K{"direct / rtfd / fallback?"}
K -- found --> F
K -- not found --> L["[]"]
F --> M["flatMap all items to itemRepresentations"]
M --> N{"empty?"}
N -- no --> O["Return all item representations"]
N -- yes --> P["Whole-pasteboard fallback (single-image)"]
O --> Q["materializeImageFileURLs"]
P --> Q
Q --> R{"Each repr <= 10 MB?"}
R -- yes --> S["Write temp file, register ownership"]
R -- "any exceeds" --> T["cleanupTransferred, rejectedImagePayload"]
S --> U[".saved([URL])"]
Reviews (1): Last reviewed commit: "Fix SSH multi-image image data drops" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private static func pasteboardFallbackImageRepresentations( | ||
| for item: NSPasteboardItem | ||
| ) -> [(data: Data, fileExtension: String)] { | ||
| guard let copiedItem = copiedPasteboardItem(from: item) else { return [] } | ||
|
|
||
| let pasteboard = NSPasteboard(name: .init("cmux-single-image-item-\(UUID().uuidString)")) | ||
| pasteboard.clearContents() | ||
| defer { | ||
| pasteboard.clearContents() | ||
| pasteboard.releaseGlobally() | ||
| } | ||
| guard pasteboard.writeObjects([copiedItem]) else { return [] } | ||
|
|
||
| if let directImage = directImageRepresentation(in: pasteboard) { | ||
| imageData = directImage.data | ||
| fileExtension = directImage.fileExtension | ||
| } else if let rtfdAttachment = rtfdAttachmentImageRepresentation(in: pasteboard) { | ||
| imageData = rtfdAttachment.data | ||
| fileExtension = rtfdAttachment.fileExtension | ||
| } else { | ||
| guard hasImageData(in: pasteboard), | ||
| let image = NSImage(pasteboard: pasteboard), | ||
| let tiffData = image.tiffRepresentation, | ||
| let bitmap = NSBitmapImageRep(data: tiffData), | ||
| let pngData = bitmap.representation(using: .png, properties: [:]) else { | ||
| return .noDecodableImagePayload | ||
| return [directImage] | ||
| } | ||
| let rtfdAttachments = rtfdAttachmentImageRepresentations(in: pasteboard) | ||
| if !rtfdAttachments.isEmpty { | ||
| return rtfdAttachments | ||
| } | ||
| if let fallbackImage = fallbackImageRepresentation(in: pasteboard) { | ||
| return [fallbackImage] | ||
| } | ||
| return [] | ||
| } |
There was a problem hiding this comment.
@MainActor on new AppKit-stateful public APIs
saveImageFileURLsIfNeeded, materializeImageFileURLsIfNeeded, and the private pasteboardFallbackImageRepresentations(for:) all reach into AppKit far beyond a simple read: pasteboardFallbackImageRepresentations creates a named NSPasteboard, calls clearContents(), writeObjects(), reads back data, and releases the pasteboard in a defer. copiedPasteboardItem also eagerly resolves lazy/promised pasteboard types via IPC. None of these carries @MainActor, so a future Task {} or DispatchQueue.global caller compiles cleanly but races against the main thread on AppKit internals. The existing single-read helpers already had this gap, but the new code adds full pasteboard lifecycle management (create → write → read → releaseGlobally) off-annotation, materially widening the exposure.
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
| for representation in representations { | ||
| guard representation.data.count <= maxClipboardImageSize else { | ||
| #if DEBUG | ||
| cmuxDebugLog("terminal.paste.image.rejected reason=tooLarge bytes=\(imageData.count)") | ||
| cmuxDebugLog("terminal.paste.image.rejected reason=tooLarge bytes=\(representation.data.count)") | ||
| #endif | ||
| return .rejectedImagePayload | ||
| cleanupTransferredTemporaryImageFiles(fileURLs) | ||
| return .rejectedImagePayload | ||
| } |
There was a problem hiding this comment.
When iterating a multi-image drop, hitting the 10 MB guard on any single representation calls cleanupTransferredTemporaryImageFiles(fileURLs) on all already-written files and returns .rejectedImagePayload for the entire batch. A user who drops two images — one 4 MB and one 12 MB — gets nothing uploaded. The old single-image code had the same early-return semantics but only ever considered one file, so this is a new all-or-nothing behaviour. The more intuitive outcome would be to skip (or reject individually) the oversized item while still returning the valid ones.
This branch was successfully deployed
1 participant
Split out the SSH-safe part from #3769 while the local Claude drag path is still under investigation.
Changes:
Verification:
FinderFileDropRegressionTests/testImagePasteboardDropMaterializesEveryImageForRemoteUploadfailed with1URL instead of2.tests_v2/test_ssh_remote_image_drop_upload.pyuploaded 2 remote files and hash-checked both.Note
Medium Risk
Medium risk because it changes pasteboard image materialization and temporary-file lifecycle, which directly affects drag/drop behavior and remote SSH uploads. Failures could lead to missing uploads or leaked temp files across sessions.
Overview
Fixes remote (SSH) image drops to upload every image when the pasteboard contains multiple image items, instead of only materializing a single image.
Refactors
GhosttyPasteboardHelperto extract image representations perNSPasteboardItem, normalize TIFF payloads to PNG, add APIs for multi-image materialization (materializeImageFileURLsIfNeeded/saveImageFileURLsIfNeeded), and ensure owned temporary clipboard images are cleaned up on app termination.Extends the debug socket drop simulation to support an
image_datapayload (in addition tofile_urls) and updates both XCTest and v2 Python regression tests to validate two-image remote upload behavior end-to-end.Reviewed by Cursor Bugbot for commit 99aea35. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes SSH multi-image drops by materializing every image from the pasteboard and uploading the full set to the remote terminal. Adds a regression test and debug simulation for image-data drops.
Bug Fixes
New Features
payloadparam (image_dataorfile_urls) and returnsrouteandpayload.Written for commit 99aea35. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes