fix(pty): prevent duplicate clipboard image paste#2273
Conversation
Greptile SummaryThis PR fixes double-pasting of clipboard images by introducing two layered defenses: a
Confidence Score: 4/5Safe to merge; the fix correctly prevents duplicate image injection without regressing text paste or single-image paste flows. The dual-layer approach (fingerprint dedup + time-gate) is well-reasoned and the tests cover the main boundaries. The small open questions — item ordering in slice(0,1), lastSystemPasteAtRef being stamped for text pastes, and the missing isNearDuplicatePaste(0) test — are all speculative edge cases that are extremely unlikely to manifest in practice. terminal-image-paths.ts is worth a second look for the slice(0, 1) ordering assumption; pty-pane.tsx is otherwise straightforward.
|
| Filename | Overview |
|---|---|
| src/renderer/lib/pty/terminal-image-paths.ts | Adds dedup fingerprinting in extractClipboardImageFiles (seen Set + slice(0,1) when types include image/*) and the new isNearDuplicatePaste time-gate helper; logic is correct but item ordering in slice(0,1) is browser-defined |
| src/renderer/lib/pty/pty-pane.tsx | Introduces two timestamp refs (lastDomImagePasteAtRef / lastSystemPasteAtRef) and mutual 250ms dedup guards between handlePaste and handleSystemPaste; also adds stopImmediatePropagation on the native event to cut off xterm's listener |
| src/renderer/tests/terminal-image-injection.test.ts | Adds three new unit tests covering dedup, alternate-representation collapsing, and the 250ms boundary; tests are correct and match implementation behavior |
Sequence Diagram
sequenceDiagram
participant User
participant OuterDiv as Outer Div (onPasteCapture)
participant handlePaste
participant xterm as xterm.js listener
participant handleSystemPaste
User->>OuterDiv: cmd+v (paste event, capture phase)
OuterDiv->>handlePaste: fires first (capture, outer→inner)
alt clipboard has image files
handlePaste->>handlePaste: stopPropagation + stopImmediatePropagation
handlePaste->>handlePaste: isNearDuplicatePaste(lastSystemPasteAtRef)?
alt not a duplicate
handlePaste->>handlePaste: "set lastDomImagePasteAtRef = now"
handlePaste->>handlePaste: injectImageFiles / pasteClipboardImageOrText
Note over xterm,handleSystemPaste: event stopped — never fires
else near-duplicate (handleSystemPaste ran recently)
handlePaste-->>OuterDiv: return (suppressed)
end
else no image files but clipboardDataMayContainImage
handlePaste->>handlePaste: stopPropagation + stopImmediatePropagation
handlePaste->>handlePaste: isNearDuplicatePaste(lastSystemPasteAtRef)?
alt not a duplicate
handlePaste->>handlePaste: "set lastDomImagePasteAtRef = now"
handlePaste->>handlePaste: pasteClipboardImageOrText (IPC path)
else near-duplicate
handlePaste-->>OuterDiv: return (suppressed)
end
else no image at all
handlePaste-->>OuterDiv: return (no-op)
OuterDiv->>xterm: event continues to target
xterm->>handleSystemPaste: onPasteFromClipboard
handleSystemPaste->>handleSystemPaste: isNearDuplicatePaste(lastDomImagePasteAtRef)?
alt not a duplicate
handleSystemPaste->>handleSystemPaste: "set lastSystemPasteAtRef = now"
handleSystemPaste->>handleSystemPaste: pasteClipboardImageOrText (preferText)
else near-duplicate
handleSystemPaste-->>xterm: return (suppressed)
end
end
Comments Outside Diff (1)
-
src/renderer/lib/pty/terminal-image-paths.ts, line 40-42 (link)The
slice(0, 1)collapse relies on the browser-defined ordering ofclipboardData.items. On macOS, Chromium typically places the PNG representation first, but this is not guaranteed across platforms or browser versions. If the OS happens to supply TIFF (or another lossy/heavier format) before PNG,slice(0, 1)would silently prefer it. Sorting by a quality heuristic (preferimage/png>image/jpeg> everything else) before slicing would make this deterministic.Prompt To Fix With AI
This is a comment left during a code review. Path: src/renderer/lib/pty/terminal-image-paths.ts Line: 40-42 Comment: The `slice(0, 1)` collapse relies on the browser-defined ordering of `clipboardData.items`. On macOS, Chromium typically places the PNG representation first, but this is not guaranteed across platforms or browser versions. If the OS happens to supply TIFF (or another lossy/heavier format) before PNG, `slice(0, 1)` would silently prefer it. Sorting by a quality heuristic (prefer `image/png` > `image/jpeg` > everything else) before slicing would make this deterministic. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/renderer/lib/pty/terminal-image-paths.ts:40-42
The `slice(0, 1)` collapse relies on the browser-defined ordering of `clipboardData.items`. On macOS, Chromium typically places the PNG representation first, but this is not guaranteed across platforms or browser versions. If the OS happens to supply TIFF (or another lossy/heavier format) before PNG, `slice(0, 1)` would silently prefer it. Sorting by a quality heuristic (prefer `image/png` > `image/jpeg` > everything else) before slicing would make this deterministic.
```suggestion
if (clipboardData.types.some((type) => type.toLowerCase().startsWith('image/'))) {
const preferredOrder = ['image/png', 'image/jpeg', 'image/gif', 'image/webp'];
imageFiles.sort(
(a, b) => {
const ai = preferredOrder.indexOf(a.type);
const bi = preferredOrder.indexOf(b.type);
return (ai === -1 ? Infinity : ai) - (bi === -1 ? Infinity : bi);
}
);
return imageFiles.slice(0, 1);
}
```
### Issue 2 of 3
src/renderer/lib/pty/pty-pane.tsx:134-135
`lastSystemPasteAtRef` is stamped on every `handleSystemPaste` invocation, regardless of whether the clipboard actually contains an image. On a slow or busy machine, if a plain-text `cmd+v` triggers `handleSystemPaste` and — within 250 ms — the same keypress also delivers a DOM paste event carrying image files to `handlePaste`, `isNearDuplicatePaste(lastSystemPasteAtRef.current)` would return `true` and silently drop the image injection. The guard should only be set (and checked) when the paste path actually carries image content.
### Issue 3 of 3
src/renderer/tests/terminal-image-injection.test.ts:62-65
The test suite doesn't cover `isNearDuplicatePaste(0, now)` — the initial state of both timestamp refs. Because `useRef(0)` is the starting value, this path is exercised on every first paste in a freshly-mounted component; a test would guard against accidentally removing the `recentPasteAt > 0` guard in the future.
```suggestion
it('detects near-duplicate paste paths from the same user gesture', () => {
expect(isNearDuplicatePaste(1_000, 1_249)).toBe(true);
expect(isNearDuplicatePaste(1_000, 1_250)).toBe(false);
// initial ref value (0) must never be treated as a recent paste
expect(isNearDuplicatePaste(0, 100)).toBe(false);
});
```
Reviews (1): Last reviewed commit: "fix(pty): prevent duplicate clipboard im..." | Re-trigger Greptile

summary
demo of the fix:
https://streamable.com/fdx3s7