fix: Downscale large images before attaching#1987
Merged
charlesvien merged 9 commits intomainfrom May 4, 2026
Merged
Conversation
This was referenced May 3, 2026
Member
Author
343dd63 to
fa4664e
Compare
8d7c1d1 to
d085988
Compare
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/code/src/renderer/features/sessions/components/SessionView.tsx:375-382
**Duplicate drop-handler logic (OnceAndOnlyOnce)**
The async drop-handling loop in `SessionView.tsx` and `TaskInput.tsx` (lines 548–555) is now character-for-character identical. The PR already introduced `resolveDroppedFile` to deduplicate `useTiptapEditor` and the two component drop handlers — this same pattern of iterating files and calling `editorRef.current?.addAttachment` appears verbatim in both components and could be extracted into a shared helper (e.g. `resolveAndAttachDroppedFiles(files, addAttachment)`) to stay consistent with the existing deduplication work.
### Issue 2 of 3
apps/code/src/renderer/features/message-editor/utils/persistFile.ts:71-86
**Inconsistent error handling across drop paths**
When `persistImageFilePath` throws (e.g. image is >50MB or the processor fails), `resolveDroppedFile` silently falls back to the original un-downscaled file path. In `AttachmentMenu.tsx` the same failure surfaces a `toast.error("Failed to attach image")`. For the drop paths in `useTiptapEditor`, `SessionView`, and `TaskInput`, the user gets no feedback that downscaling was skipped and the image may still exceed the upstream dimension limit. At a minimum a toast on the catch branch would align the two paths.
### Issue 3 of 3
apps/code/src/renderer/features/message-editor/tiptap/useTiptapEditor.ts:370-388
**Drop handler now always consumes the event**
`event.preventDefault()` and `return true` are now unconditional. Previously, if every dropped file had no resolvable path (`getFilePath` returns `""`), the handler returned `false`, letting tiptap or the browser handle the drop normally. After this change, any file drop is always swallowed — including cases where all `resolveDroppedFile` calls return `null` — which could silently discard drops of items that don't have an Electron file path attached. This is a minor but real behaviour change worth documenting or guarding.
Reviews (1): Last reviewed commit: "Consolidate image constants and unify dr..." | Re-trigger Greptile |
d085988 to
ab33331
Compare
fa4664e to
00b5fb3
Compare
ab33331 to
6310a52
Compare
c62d6b9 to
6a11acc
Compare
30015b0 to
fa56f04
Compare
2f8d1ae to
9d74bcd
Compare
b77e0c7 to
dd6be59
Compare
dd6be59 to
bd286e7
Compare
9d74bcd to
0587776
Compare
bd286e7 to
813e5c9
Compare
Member
Author
Merge activity
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Problem
Large images (>2000px) fail with a dimension limit error when attached via file picker or drag-and-drop. Cmd+V paste already worked because it pre-downscales through ElectronImageProcessor.
Closes #1889
Changes
How did you test this?
Manually