feat(tui): paste images from clipboard via cmd+v#2887
Conversation
- Add clipboard_image_darwin.go: reads PNG image data from the macOS clipboard using osascript and writes it to a temp file - Add clipboard_image_other.go: no-op stub for non-Darwin platforms - Wire readClipboardImage() into handleClipboardPaste() in editor.go so that cmd+v first checks for an image, attaches it if found, and falls through to normal text-paste behaviour otherwise - Update banner.go and styles.go to support image attachment display
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
This PR adds macOS clipboard image paste via cmd+v, which is a great UX improvement. Three medium-severity bugs were found in the newly added code that need attention before merging.
Summary of findings:
- Temp file leaked when
AttachFilefails — no cleanup on error path - Silent failure — paste handler exits early even on
AttachFileerror, skipping the text-paste fallback - Binary PNG corrupted — raw PNG bytes flow through the text-attachment path and will be UTF-8-corrupted when JSON-serialized to the model
| if attachErr := e.AttachFile(imgPath); attachErr == nil && len(e.attachments) > 0 { | ||
| e.attachments[len(e.attachments)-1].isTemp = true | ||
| } | ||
| return e, textarea.Blink |
There was a problem hiding this comment.
[MEDIUM] Temp clipboard image file leaked when AttachFile fails
When readClipboardImage() returns a non-empty imgPath and AttachFile subsequently fails (e.g., file too large, validation error), the PNG temp file at imgPath is silently leaked — there is no os.Remove(imgPath) on the error branch, and isTemp is only set on success so collectAttachments cleanup is never triggered either.
if imgPath, err := readClipboardImage(); err == nil && imgPath != "" {
if attachErr := e.AttachFile(imgPath); attachErr == nil && len(e.attachments) > 0 {
e.attachments[len(e.attachments)-1].isTemp = true
}
return e, textarea.Blink // temp file leaked if AttachFile failed
}Fix: add cleanup on the error path:
if attachErr := e.AttachFile(imgPath); attachErr == nil && len(e.attachments) > 0 {
e.attachments[len(e.attachments)-1].isTemp = true
} else if attachErr != nil {
_ = os.Remove(imgPath)
}| if attachErr := e.AttachFile(imgPath); attachErr == nil && len(e.attachments) > 0 { | ||
| e.attachments[len(e.attachments)-1].isTemp = true | ||
| } | ||
| return e, textarea.Blink |
There was a problem hiding this comment.
[MEDIUM] Silent failure: AttachFile rejection skips text-paste fallback
The return e, textarea.Blink at line 887 is inside the outer if imgPath != "" block but outside the inner if attachErr == nil block. This means the function always returns early whenever the clipboard contains an image — even if AttachFile fails. The text-paste fallback below is never reached, so if the image is rejected (e.g., >5 MB size limit), the user gets a silent no-op with no feedback and no text-paste attempt.
if imgPath, err := readClipboardImage(); err == nil && imgPath != "" {
if attachErr := e.AttachFile(imgPath); attachErr == nil && len(e.attachments) > 0 {
e.attachments[len(e.attachments)-1].isTemp = true
}
return e, textarea.Blink // ← exits even when AttachFile failed
}
// text paste — unreachable when clipboard has an imageFix: Only return early on successful attachment; fall through to text paste (or show an error) on AttachFile failure:
if imgPath, err := readClipboardImage(); err == nil && imgPath != "" {
if attachErr := e.AttachFile(imgPath); attachErr == nil && len(e.attachments) > 0 {
e.attachments[len(e.attachments)-1].isTemp = true
return e, textarea.Blink
}
// AttachFile failed — clean up and fall through to text paste
_ = os.Remove(imgPath)
}|
|
||
| func (e *editor) handleClipboardPaste() (layout.Model, tea.Cmd) { | ||
| if imgPath, err := readClipboardImage(); err == nil && imgPath != "" { | ||
| if attachErr := e.AttachFile(imgPath); attachErr == nil && len(e.attachments) > 0 { |
There was a problem hiding this comment.
[MEDIUM] Binary PNG data routed through a text-only attachment path — will corrupt image on send
When AttachFile succeeds and isTemp = true is set, the image attachment is handled by the pre-existing collectAttachments() code which reads the temp file and stores it as:
result = append(result, messages.Attachment{
Name: strings.TrimPrefix(att.placeholder, "@"),
Content: string(data), // raw PNG bytes cast to string
})collectAttachments was designed for text-paste temp files. For a clipboard PNG, data will be raw binary bytes. Go's string(data) preserves the bytes in memory, but Go's encoding/json encoder replaces invalid UTF-8 sequences with the Unicode replacement character (U+FFFD), corrupting the image before it reaches the model. File-reference attachments use the FilePath field which avoids this issue entirely — clipboard images should follow the same path, or PNG bytes should be base64-encoded into Content.
What
Adds support for pasting images from the clipboard (cmd+v) directly into the conversation editor.
How
clipboard_image_darwin.go— macOS-only implementation that usesosascriptto read PNG image data from the system clipboard and writes it to a temp file in the data directory.clipboard_image_other.go— no-op stub (build tag!darwin) so the package compiles on Linux/Windows without changes.editor.go—handleClipboardPaste()now callsreadClipboardImage()first. If an image is found it is attached via the existingAttachFilepath (and markedisTemp = truefor auto-cleanup); otherwise the function falls through to the existing text-paste behaviour unchanged.banner.go/styles.go— minor display tweaks to correctly surface image attachments in the attachment banner.Behaviour
@/tmp/clipboard-image-<ts>.png→ shown in banner"", nil)