Repository navigation
fix(feedback): optimize oversized screenshots without losing detail - #23664
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 5ed3b1e8d78d3c4f2f18db295b802de1bc3182a1 and 3bbd62b2cad4a15d2c7f4477e8ac5f6a5381f557. 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughFeedback image attachment processing now shrinks eligible oversized images and returns compression notices with images and errors. The shrink process tries PNG and JPEG encodings in a defined order. Sidebar image batches are read sequentially and use the committed image count and byte total when each read starts. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Oversized screenshots are now shrunk to fit the attachment limit, and the user is told when this happens. Animated GIF, WebP and APNG images are still refused rather than flattened. No merge-blocking risk was identified. Architecture SummaryArchitecture risk: 🟡 Medium · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
Reliability and maintainability
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue [ Comment |
22c4e73 to
0a75606
Compare
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one rough edge inline.
Reviewed changes
- Attach-time shrink pipeline — new
feedback-image-shrink.tsdecodes once (createImageBitmap) and re-encodes down a PNG-then-JPEG ladder, returning the first blob that fits the space left. - Budget accounting —
readFeedbackImageFilesadmits an oversized file only when there is a plausible shrink target, subtracts the shrunk size from the budget, and returns compressionnoticesalongside errors. - Serialized read queue —
use-sidebar-feedback-images.tsnow reads batches one at a time against committed drafts, so a later add is measured against the earlier batch's shrunk size rather than its raw file size; notices surface viatoast.info. - Gates & copy —
canAttachWithinlets the paste and attach gates admit shrinkable images;en.jsongains thecompressedstring; four test suites updated/added.
I verified the main-process handoff is safe (shrunk contentType can only be image/png/image/jpeg, both accepted; bytes/data derive from the same Blob and main validates data.byteLength), and that en-runtime-required.json does not need the new key because its call-site default matches en.json.
ℹ️ Animation guard excludes only GIF, but image/webp is an accepted animation-capable type
canAttachWithin protects animation only by rejecting image/gif, yet image/webp is in SUPPORTED_FEEDBACK_IMAGE_TYPES and can be animated. An oversized animated WebP therefore takes the shrink branch, where createImageBitmap yields a single frame, so the animation is silently flattened — the exact "degrade an attachment without telling the user" outcome this PR exists to prevent. The compression notice only reports a size change.
Technical details
# Animated WebP is silently flattened by the shrink path
## Affected sites
- `src/renderer/src/lib/feedback-image-attachments.ts:62` — `file.type !== 'image/gif'` is the only animation guard; `image/webp` passes.
- `src/renderer/src/lib/feedback-image-shrink.ts:56` — `createImageBitmap(image)` is spec'd to decode only the default/first frame of an animated image.
- `src/renderer/src/lib/feedback-image-attachments.test.ts:198` — the only animation test is GIF-specific; there is no WebP case.
## Required outcome
A file whose animation would be lost must not be silently re-encoded: exclude animation-capable accepted types (`image/webp`, and ideally animated PNG) from shrink, or refuse them with the existing error path as GIF already does.
## Suggested approach
Extend the guard to `image/webp` and detect APNG (or simply refuse `image/png` files that sniff as APNG) before shrinking, mirroring the GIF rationale. `createImageBitmap` frame semantics: https://html.spec.whatwg.org/multipage/imagebitmap-and-animations.html#dom-createimagebitmap; animated WebP support: https://developer.mozilla.org/en-US/docs/Web/Media/Guides/Formats/Image_typesDeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ New commit is clean — no new issues; the earlier animated-WebP suggestion remains open.
Reviewed changes
Reviewed the single commit added since the prior pullfrog review, against a rebase of the original head.
- Lazy read-queue seed —
readQueueRefnow initializes asuseRef<Promise<void>>(undefined!)and seeds withreadQueueRef.current ??= Promise.resolve(). This satisfies the rendereruseReflazy-initialization ratchet (src/renderer/src/lazy-use-ref-ratchet.test.tsrejectsuseRef(Promise.resolve())because the argument runs on every render) and is behaviour-neutral:currentis always seeded beforehandleAddFilescan run, and the??=guard keeps the same promise for the component's life. Theundefined!marker is explicitly allow-listed by that ratchet.
Nothing else changed in the delta. The previously flagged animation guard (only image/gif excluded, image/webp still shrinkable) is unaffected by this commit and remains a minor, non-blocking suggestion.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
1911db6 to
5761297
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the net delta since the prior pullfrog review (0a75606), centered on the new animation guard, against the full PR.
- Byte-level animation detection —
hasAnimationFramesreads theVP8Xanimation flag (0x02) for WebP and walks PNG chunks foracTLbefore the firstIDAT, so an oversized animated WebP or APNG is refused with the size error instead of being flattened to one frame bycreateImageBitmap. - Guard placement — the check runs in
readFeedbackImageFilesonly for files larger than the space left, after the dimension check and before shrinking; the by-type GIF guard still short-circuits before any read. - Coverage — new unit tests exercise the chunk/flag sniffing (APNG, animated and static WebP, truncated PNG) and the refusal path; the existing animated GIF/WebP read test now also asserts
shrinkFeedbackImageis never called.
I verified the VP8X animation bit against the WebP RIFF container spec (|Rsv|I|L|E|X|A|R|, MSB-first, so animation = 0x02) and the PNG acTL-before-IDAT rule, and ran the four feedback suites locally — 60 tests pass.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes
Reviewed the commit that superseded 1911db6 on this branch (the branch moved while the previous review was posting).
- Swapped byte sniffing for a type guard — the read-time
hasAnimationFramescheck is gone;canAttachWithinnow refusesimage/webpby type alongsideimage/gif, so an oversized WebP never reachesshrinkFeedbackImage. - Removed the sniffing helpers —
hasAnimationFrames/hasAsciiand their unit tests are deleted; the GIF and WebP refusal cases are folded into one read-path test. - Net effect — oversized WebP (static or animated) is refused outright; only PNG and JPEG are still shrunk.
The animation guard now matches the PR description's stated rule for GIF and WebP. The one gap left is APNG, noted inline. All four feedback suites pass locally (56 tests).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
5761297 to
5ed3b1e
Compare
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
5ed3b1e to
3bbd62b
Compare
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes
Reviewed the delta since the prior pullfrog review (5761297): the acTL APNG refusal.
- Refused oversized animated PNG —
isAnimatedPngsniffs theacTLchunk in the first 64 KB and routes an oversizedimage/pngto the existing size error instead of the shrink path, matching the by-type GIF/WebP rule; a read-path test and an updated paste test come with it.
The APNG gap raised in the prior review is closed and its thread resolved. The remaining note is that the new refusal is not mirrored in the paste gate, which cannot sniff bytes synchronously.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
66b2dda to
49e2581
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the net delta since the prior pullfrog review (3bbd62b): the localized compression notice plus the narrowed paste-gate contract.
- Added
compressedtranslations — Spanish, French, Japanese, Korean, and Simplified Chinese now carry the compression notice, each preserving all three placeholders ({{fileName}},{{originalSize}},{{size}}).pnpm run verify:localization-catalogspasses (exit 0). - Narrowed the
hasAttachableFeedbackImagecontract — the doc comment now scopes the mirroring invariant to the type/size limits the gate can evaluate and states that byte-dependent checks (dimensions, APNG, whether a shrink fits) cannot run synchronously, resolving the last inline thread.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the three commits added since the prior pullfrog review (49e2581): the shrink-ladder split, the per-step canvas release, the read-queue settle hardening, and the pending-attachment hint.
- Split the shrink ladder by source type —
feedbackImageShrinkSteps(sourceType)returns the JPEG-only ladder for animage/jpegsource (skipping both lossless PNG encodes, since a full-size PNG of decoded JPEG noise runs larger than the source it must undercut), while PNG still tries PNG sizes before the lossy ones. - Released each shrink step's canvas —
encodeBitmapis now async and zeroescanvas.width/heightin afinallyaftertoBlobhas answered, so full-size canvases no longer accumulate against the renderer's canvas memory budget (which hands back a blank canvas, silently uploading an empty screenshot). - Hardened the serialized read queue — the queue tail is now
settled.catch(...), so a throw inside either settle callback can no longer leave the tail rejected and make every later attach report "Could not read the attached images". - Added a "Preparing attachments…" hint — the attachment box shows it while
pendingImageReadCount > 0, so a slow shrink does not read as a dropped file;preparingwas added to every locale (no placeholders, so catalog parity holds). - Tests — new coverage for the PNG/JPEG step split, batch sizing against the shrunk size, the compression notice, and the pending hint.
The JPEG-source shortcut is a deliberate resolution/time trade-off documented in feedback-image-shrink.ts and the PR body (greptile's open P2 raises the clarity side of it). Feedback suites (60 tests), oxlint on the changed sources, and verify:localization-catalogs all pass locally at this head.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Thanks, Kelvin. Shrinking at attach time in the dialog is the right call, and the reasoning against doing it in main is solid. We tested a real build with several full-screen Retina screenshots and pushed some changes on top (plus a merge of the latest
Could you update the description to match? Specifically:
|
There was a problem hiding this comment.
Important
The new paste-gate reservation can under-count a mixed batch, so a paste is swallowed and its text dropped even though the reader then refuses the image. Details inline.
Reviewed changes
Reviewed the 14 commits added since the prior pullfrog review (22d2722) — the half-budget shrink cap and its accounting, the paste-gate reservation, the chunk-walk APNG sniff, the delayed preparing hint, the unmount skip, and supporting tests/docs.
- Capped every shrunk screenshot at half the total budget —
MAX_FEEDBACK_IMAGE_SHRINK_TARGET_BYTES = TOTAL/2; an image that already fits is still never re-encoded, so the first re-encode leaves room for a second. - Reserved a shrinking batch's capped size for the paste gate —
maxFeedbackImageBatchBytesreplaces the hook's raw file-size sum so a screenshot still shrinking no longer turns the next paste away. - Blamed the shared budget vs. the file size —
addSizeError(fitsWholeBudget)now picks "would bring the attachments over 4.0 MB in total" only when the whole budget could not have taken the file. - Walked PNG chunks for APNG —
isAnimatedPngfollows chunk headers, soacTLpast a large metadata chunk is found and bytes that merely spellacTLare not misread; a corrupt length ends the walk. - Reported an undecodable oversized image as invalid — a
shrinkFeedbackImagerejection now yields "is not a valid supported image" instead of a size message. - Delayed the preparing hint —
SidebarFeedbackImageAttachmentsusesuseDelayedStatuswith a 250 ms show delay so an image that reads at once does not flash it; queued reads are skipped once the dialog unmounts. - Tests/docs — coverage of the real decode/downscale/release path, the cap, the APNG walk, and the paste race; corrected
toBlob/canvas comments.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ New commit is clean — no new issues. The earlier
maxFeedbackImageBatchBytesfinding remains open and untouched by this commit.
Reviewed changes
Reviewed the single commit added since the prior pullfrog review (d202596): a revert of e2272b825e.
- Reverted the direct read-queue seed —
readQueueRefis back to the lazy form (useRef<Promise<void>>(undefined!)plusreadQueueRef.current ??= Promise.resolve()). The directuseRef(Promise.resolve())form evaluates its argument on every render and trips the rendereruseReflazy-initialization ratchet (src/renderer/src/lazy-use-ref-ratchet.test.ts). Behavior is unchanged:currentis seeded before any handler can run, and??=keeps the same promise for the component's life.
I ran lazy-use-ref-ratchet.test.ts, use-sidebar-feedback-images.test.tsx, and the three feedback suites (73 tests) at this head — all pass. The one open pullfrog thread (maxFeedbackImageBatchBytes under-reserving a mixed batch) anchors to feedback-image-attachments.ts, which this revert does not touch, so it stays open.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
72304c5 to
5a33db9
Compare
nwparker
left a comment
nwparker
left a comment
There was a problem hiding this comment.
Repaired removed-hook integration, silent loss of text co-pasted with oversized images and render-time queue-ref mutation found by fresh CI. 80 tests, web typecheck, changed-quality and standalone React Doctor pass; paste preservation tests verified red/green. Remaining tradeoff: resizing/JPEG can reduce screenshot detail, disclosed in UI.
Required PR Checks verify passed on this exact commit: 72a1761bf4f98e3b68ae9c44ee739f890ffb663c.
nwparker
changed the title
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the net delta since the prior pullfrog review (72304c5): the mixed-paste gate option and read-queue seed (72a1761), then the full-resolution lossless-only rewrite and the independent batch reservation (b47911c).
- Replaced the downscale/JPEG ladder with a single full-resolution PNG encode —
feedback-image-shrink.tsdraws the decoded bitmap at native size and encodes once asimage/png; an oversized image that cannot fit is refused with the existing size message, and an image already within budget stays byte-for-byte. GIF/WebP stay refused by type, APNG by theacTLchunk walk. - Made the paste-gate reservation an upper bound —
maxFeedbackImageBatchBytesnow sums independent per-file bounds (file size when it fits, else the capped shrink target) and stops at the total budget, so a shrink's freed slack, a rejected file, or a removed draft can no longer make a queued batch under-reserve and swallow co-pasted text. The oldretina.png+anim.gifcounterexample now reserves the full 4 MiB and is pinned by a regression. - Preserved mixed-paste text — the gate only consumes a clipboard carrying text when an image fits as-is (
allowShrinking: clipboardData.getData('text/plain').length === 0), whilehandleAddFilesstill runs; the read queue also skips queued batches after unmount. - Tests — new regressions for the GIF-admitted-by-slack reservation, the budget-capped reservation, the full-resolution PNG policy for both PNG and JPEG sources, and a
userEventpaste that preserves text, caret, and subsequent typing when the attached image is refused.
I traced the new reservation as a true upper bound on what the reader can commit (the predictor always uses the largest fitBytes, and each reader commit is <= its predicted per-file bound), re-checked the addSizeError budget-vs-file-cap attribution across gif/APNG/capped-target cases, and ran the four focused feedback suites — 77 tests pass at b47911c. The PRRT_kwDORpCz1s6oPcAl thread is already resolved by the author's reply, and no new issue replaces it.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
… a JPEG Two defects in the attach-time shrink ladder: - Every step allocated a fresh full-size canvas and left it for the garbage collector, so a 32-megapixel source could keep six of them alive at once. Past the renderer's canvas memory budget Chromium hands back a blank canvas, which would upload as a blank screenshot with nothing to notice it. Each step's canvas is now released once toBlob has answered. - A JPEG source ran two full-size PNG encodes first. A PNG of decoded JPEG noise is many times larger than the source it has to undercut, so those steps could only ever burn time and a large buffer before the first JPEG attempt. A lossy source now starts at the JPEG steps.
…ck throws The queue tail is what the next batch awaits. A throw in either settle callback left it rejected, so every later attach short-circuited into "Could not read the attached images" without reading anything — and the rejection went unhandled.
Shrinking a full-screen capture takes long enough that the gap between picking the file and its thumbnail appearing reads as a dropped attachment: the only other signal was the Send button quietly staying disabled. The attachment hint now reads "Preparing attachments…" while a batch is in flight.
…fused image's budget
…inkable image has no room
A shrink kept the largest step that fit everything left, so the first full-screen screenshot took a 3.8 MB PNG, the second fell to a barely readable JPEG and the third was refused. Re-encodes now target at most 2 MB; an image that already fits is still attached untouched. The "larger than 4.0 MB" refusal now applies whenever the capped target an empty budget would give was the one that refused it.
…file size, for the paste gate A 6 MB screenshot still shrinking reserved 6 MB, more than the whole budget, so a second paste during the shrink fell through to the textarea even though it attached. Each pending file now reserves what the reader can commit under the same fit and shrink-target rules. Also corrects the minimum shrink target comment to the measured smallest step.
…ents; mock toast.info in the dialog test
This reverts commit e2272b825eb2d26286aee3a99ca28f3ea984508e.
Optimize oversized attachments only as full-resolution lossless PNG; refuse them with existing warnings when that cannot fit. Bound each pending batch independently so earlier optimization slack, rejected files, or removed drafts cannot under-reserve bytes and swallow co-pasted text. Regression tests demonstrate the old mixed-batch text loss.
b47911c to
91e7548
Compare
|
Addressed the older review’s two summary warnings on the current head,
The two concrete inline findings (JPEG detail loss and pending-batch under-reservation) are separately fixed, regression-tested, replied to and resolved. Latest-head CI remains the merge gate. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the delta since the prior pullfrog review (b47911c): a rebase onto newer main plus three test-only commits. The feedback production code is unchanged from the previously approved head; the new commits add test coverage and one test-only dependency.
- De-duplicated the journal provider-handle import —
journal-submission-positions.test.tskeeps a singlecodexProviderHandleimport (moved to alphabetical position); no behavioural change. - Covered the encoded Windows launch line —
startup-line-prompt-carry.test.tsadds a win32 case asserting a short multi-line prompt rides the launch line with no raw\nand a`nescape, and moves the "not carried" control todarwin(a short win32 prompt is now carried). The encoding lives inbuildAgentStartupPlan, not the gate under test — the distinction the new control relies on is correct. - Pinned the released journal parser dependency —
@streamparser/json@0.0.26is added todevDependencieswith a comment aboveCHECKOUT_FORMAT = 4.release-checkout-tree.tsarchives onlysrc/**and runs no install, so the extracted released sources resolve the bare import from the repo root'snode_modules; no format bump is needed because extraction is unchanged.
I ran the two changed suites at 725ee66 (35 tests) — both pass. All prior pullfrog threads remain resolved.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
nwparker
left a comment
nwparker
left a comment
There was a problem hiding this comment.
Low merge risk after the final repairs. Oversized screenshots are optimized only through full-resolution lossless PNG encoding. The original attachment limits remain enforced before decode, and independent paste reservations preserve eligible images and accompanying text. 156 focused tests, both typechecks, and quality gates pass. Fresh CI on this exact head has passed, and review findings are addressed.
ELI5
Feedback screenshots must fit the existing 4 MB attachment budget. Orca now tries a full-resolution lossless PNG re-encode when an image is too large, and attaches it only if that fits. It never downsizes the screenshot or adds JPEG compression. Images that still cannot fit receive the existing size warning. The original file is unchanged.
What Changed
Before: oversized screenshots were refused. This branch's earlier compression ladder could fit more images by reducing resolution or using JPEG, which could make small text harder to read.
After: an oversized PNG or JPEG gets one full-resolution PNG encode. If the result fits its remaining capped budget, it appears as an attachment and a toast reports the size reduction. If it cannot fit, the image is refused rather than losing detail. Images already within the budget are attached byte-for-byte.
Why
Feedback screenshots often contain small text. Full-resolution PNG preserves displayed detail without introducing JPEG artifacts. A single encode bounds the work more tightly than the old multi-step ladder. This deliberately leaves some oversized photographs or already efficiently encoded PNGs refused, matching the previous main behavior instead of attaching a less readable image.
Preparation stays in the dialog so the user sees the result or warning before sending. Browser clients use the same path. Conservative pending reservations can leave co-pasted text in the textarea even when an earlier batch later frees space; that is preferable to silently discarding the report.
Linked Issue
Related to #22776: automatic lossless optimization only; resolution reduction and lossy JPEG fallback are intentionally excluded.
Visual Proof
Before:
After:
The existing proof shows the attachment/preparation flow; the final compression policy now uses full-resolution PNG only.
Testing
ORCA_BACKGROUND_LAUNCH=1. No visible Electron app, native clipboard, live feedback submission or Linux/Windows manual validation was run for this repair. Fresh CI must pass on the final head before merge.Review
The earlier Medium risk caused by downscaling/JPEG detail loss is removed by the full-resolution lossless-only policy. Both open bot findings have concrete code fixes and regression coverage. Remaining size refusals and animated-image restrictions match baseline safeguards; pending reservations err toward preserving report text.
Agent skill upstream boundary
Checklist