Repository navigation
fix(cli): include captured raster formats in contact sheets - #5301
miguel-heygen merged 4 commits into
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Main still limits the asset contact sheet to png/jpg/jpeg/webp, while captureImageExtension in captureImageValidation.ts can save .avif, .gif, .heic and .tiff. The new list matches that validator's output set exactly. I checked the one risky entry, .heic: the validator only assigns an extension after a successful resize(1, 1).raw() decode, so a HEIC that lands in assets/ is one Sharp can decode, and it cannot crash createContactSheet, which has no per-image try/catch. The tests write real AVIF, two-frame GIF and TIFF fixtures, sample the center pixel of the first cell (244,270 lines up with the 4-col, 480px, 26px-label layout), and assert the originals are byte-identical. The libvips cache toggle keeps Windows cleanup working. Need: yes · Size: ok · Reuse: reuses the validator's format set and the existing Sharp path · Simpler: already minimal.
— Rames
|
Heads up, @user-github-me: the merge queue removed this PR. Your #5295 merged first and also edits Could you rebase onto the latest |
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up. The merge with main left a single createAssetContactSheet import (contactSheet.test.ts:14-19), so the duplicate identifier that took this out of the merge queue is gone, and the PR's net change is back to the one-line format list in contactSheet.ts:250 plus its tests. CI is green at this head, including the Windows engine/CLI job that runs these tests.
Verdict: APPROVE
Reasoning: The duplicate import that failed the queue is gone, the change is unchanged from the approved version, and the branch merges cleanly with main.
What
Include AVIF, GIF, HEIC, and TIFF assets in captured-image contact sheets, alongside the existing JPEG, PNG, and WebP formats.
Why
Capture already validates and downloads these raster formats, but its asset-sheet file list excludes them. A page with a normal AVIF feature image captures successfully with one asset, zero drops, and zero warnings, yet produces no
assets/contact-sheet.jpg. The generated agent prompt then points at a missing sheet.Related work
#5295 fixes EXIF orientation while decoding and resizing thumbnails in the same module. This PR changes only the format selection and its tests; the two fixes cover separate behavior. No matching AVIF/contact-sheet issue or other implementation found in current source and PR searches.
How
Extend the asset-sheet raster extension list to include formats already admitted by the capture image validator. Use the existing Sharp thumbnail path; animated GIFs show their first frame. SVGs retain their dedicated sheet.
Test plan
cb884c82ac7b6d4fa03b1f687e9e20abc346107095473e9c1d422aaf0de14eaf. Format tests also verify original bytes and the first frame of a two-frame GIF.To reproduce, serve a plain HTML page with an
<img>referencing the pinned AVIF and runhyperframes capture <url> --skip-vision --max-screenshots 1 --json. On main the image is inassets/fox-outdoors.avifwith no asset sheet; with this change its labeled thumbnail appears in the sheet.Windows validation
The first Windows engine/CLI run passed 4,486 tests and failed only when removing the animated GIF fixture (
EBUSYinfinally). The test now clears libvips' cached file handles before removing the temporary directory and restores its prior cache limits afterward. The GIF thumbnail and original-byte assertions remain active on Windows; all 21 related tests and CLI typecheck pass locally after the correction. The corrected signed commit85f67e2b150b0a2bd0e4d441573ee28ce71e2be7passes the Windows engine/CLI job; the Windows render, core, and Studio-2 jobs also pass.Review follow-up
Merged current main (
89081b919) and removed the duplicatecreateAssetContactSheetimport reported by the merge queue. The merge preserves main's EXIF-orientation checks and this PR's AVIF, animated GIF, TIFF, and PNG coverage.The uncorrected merge reproduces TypeScript TS2300 at both import lines; the corrected merge passes CLI typecheck and all 30 related contact-sheet, post-extraction, and image-validation tests. The full workspace build, root lint, formatting, normal commit hooks, and both comment checks also pass. Updated GitHub CI remains to run on the published commit.