fix: dedupe repeated desktop image paste - #43396
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting a real desktop paste path. Current main still sends every clipboard image from apps/desktop/src/app/chat/composer/index.tsx:314-325 to attachImageBlob, which persists each image at apps/desktop/src/app/chat/hooks/use-composer-actions.ts:410-423 without cross-event dedupe.
Problems
apps/desktop/src/app/chat/hooks/use-composer-actions.ts:394records the key beforesaveImageBuffer. When that operation returns no path or throws, the entry remains; an immediate retry is suppressed and returns success without attaching an image. Clear the entry on every persistence failure and cover retry-after-failure.apps/desktop/src/app/chat/hooks/use-composer-actions.ts:49identifies content with only size plus a 32-bit FNV-style hash. Distinct same-size image bytes can collide and be silently dropped within the window. Use a collision-resistant digest or retain/compare the short-lived bytes.
Suggested changes
- Reapply the narrow hook change against the current source and fold the helper tests into the now-expanded test file.
Automated hermes-sweeper review.
| hash = Math.imul(hash, 16777619) >>> 0 | ||
| } | ||
|
|
||
| return [blob.size, hash.toString(16)].join('|') |
There was a problem hiding this comment.
This is not byte-identity dedupe: the key contains only size plus a 32-bit hash, so different same-size images can collide and one will be silently suppressed. Please use a collision-resistant digest or compare the retained short-lived bytes.
| // macOS/Electron can fire the same Cmd+V screenshot through multiple | ||
| // clipboard paths/events. Drop only near-simultaneous byte-identical | ||
| // image blobs so a pasted screenshot attaches once. | ||
| if (!rememberRecentImageBlobPaste(recentImageBlobPastesRef.current, dedupeKey)) { |
There was a problem hiding this comment.
The key is remembered before saveImageBuffer, but neither the no-path return nor the catch clears it. A transient save failure therefore makes an immediate retry return true without attaching anything; remove the key on every save failure and add a retry regression test.
f56f73a to
3e86a0f
Compare
|
Updated on 2026-07-21 against current upstream main at commit 477c08b.
Validation: 22 focused Vitest tests passed. Targeted ESLint and Prettier checks passed. |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Of the two PRs reviewed, only #43396 addresses the reported repeated-image-paste cause by deduplicating byte-identical blobs across separate desktop callbacks at the persistence boundary. Merged PR #56029 changes the same hook but solves an unrelated dropped-folder classification bug and provides only relevant baseline context.
Related pull requests
- #43396
related— (+217/-3) — merge candidate: Adds a 1.5-second SHA-256 content-deduplication window around attachImageBlob, while preserving byte-distinct same-size images and clearing dedupe state after save or attachment failures. This satisfies the keep_open review on #43396: the current diff replaces the collision-prone hash, covers both persistence-failure paths, and adds focused retry and identity tests. - #56029 [merged]
related— (+182/-40) — not a duplicate: This merged PR detects native dropped directories through webkitGetAsEntry and routes them to @folder refs rather than the upload pipeline; it does not deduplicate repeated clipboard images. It remains relevant because it is the merged baseline implementation in the same hook and test file against which #43396 must apply cleanly.
Suggested consolidation
Merge #43396 after confirming its stated focused tests and checks against the current merged baseline; its diff directly fixes the cross-callback byte-identical image duplication and explicitly resolves the concerns in the keep_open review. Do not close or classify merged #56029 as a duplicate, because it addresses dropped-folder routing rather than image-paste duplication.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 21 kB of PR diffs, 2 kB of issue/PR text, 2 kB of discussion (3 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Summary
attachImageBlobcalls within a short window.Related work
#38306 handles mirrored
DataTransfer.itemsandDataTransfer.filesinside one paste event. #55722 works at the composer extraction and drop-event layer by selecting one native image candidate and stopping drop bubbling.This PR covers the downstream persistence boundary across separate desktop callbacks or events. It keeps byte-distinct images even when their metadata matches, collapses byte-identical images even when their metadata differs, and rolls back the key on persistence failure.
Testing
npm --workspace apps/desktop run test:ui -- src/app/chat/hooks/use-composer-actions.test.tsnpm --workspace apps/desktop run typechecknpx eslint src/app/chat/hooks/use-composer-actions.ts src/app/chat/hooks/use-composer-actions.test.ts