Conversation
There was a problem hiding this comment.
🟡 Changes recommended
APNG detection can misclassify valid animated PNGs and silently flatten them during conversion.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds WebP re-encoding to reduce the size of images embedded in generated documentation.
Changes:
- Converts PNG, GIF, and JPEG images to WebP with safety fallbacks.
- Adds encoder dependencies to development, CI, and container environments.
- Updates tests, filter references, and browser requirements.
File summaries
| File | Description |
|---|---|
lib/docs/filters/core/images.rb |
Implements WebP conversion and MIME handling. |
test/lib/docs/filters/core/images_test.rb |
Tests conversion and fallback behavior. |
test/files/image.gif |
Adds a GIF conversion fixture. |
test/files/image.jpg |
Adds a JPEG conversion fixture. |
README.md |
Updates browser compatibility requirements. |
docs/scraper-reference.md |
Documents the default image filter. |
docs/filter-reference.md |
Adds the image filter reference. |
Dockerfile |
Installs WebP tools. |
Dockerfile-alpine |
Installs Alpine WebP tools. |
.github/workflows/test.yml |
Installs WebP tools in CI. |
.devcontainer/devcontainer.json |
Installs WebP tools in the dev container. |
Review details
- Files reviewed: 9/11 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+57
to
+62
| def self.png?(data) | ||
| return false unless starts_with?(data, PNG_SIGNATURE) | ||
| # cwebp silently keeps the first frame of an animated PNG | ||
| idat = data.index('IDAT'.b) | ||
| idat.nil? || !data.byteslice(0, idat).include?('acTL'.b) | ||
| end |
Comment on lines
+169
to
+173
| it "keeps the PNG when the conversion doesn't pay off" do | ||
| @body = IMG_BODY | ||
| data = png_data | ||
| stub_request make_response(body: data, content_length: data.bytesize) | ||
| stub(Docs::ImagesFilter).convert_to_webp(data) { nil } |
| JPEG_SIGNATURE = "\xff\xd8\xff".b | ||
|
|
||
| # WebP q=80 is roughly equivalent to JPEG q=90, and -sharp_yuv keeps the | ||
| # edges of the screenshots and diagrams documentation is full of crisp. |
Pipe PNGs through `cwebp -lossless` after image_optim, before the final size check. Roughly 15% smaller across the PNGs in the existing docs. Falls back to the PNG when the data isn't a PNG, when it's animated (cwebp would silently drop all but the first frame), when the WebP comes out bigger, or when cwebp isn't installed. WebP needs Safari 14+, Edge 18+ and iOS 14+, well below the versions the app already requires.
Route GIFs through gif2webp, sharing the size comparison and fallbacks with the PNG path. The wins are larger than for PNGs -- the GIFs in the existing docs drop to roughly a quarter of their size -- since GIF's LZW leaves a lot on the table. gif2webp keeps every frame, so animations survive the conversion.
Re-encode JPEGs with `cwebp -q 80 -sharp_yuv`, which takes the JPEGs in the existing docs down to 73% of their size. Quality 80 is the knee of the size/SSIM curve measured over those images (mean SSIM 0.9935, worst 0.984); 85 gives up most of the savings for little quality. -sharp_yuv keeps text edges in screenshots and diagrams crisp. The pipeline was already lossy for PNGs via pngquant, and jpegoptim already strips EXIF, so dropping metadata here loses nothing new. Move the GIF test fixture to test/files/ next to the new JPEG one.
Pipe ChunkyPNG's output straight into `cwebp -lossless` instead of writing a PNG and running OptiPNG over it. No PNG is produced anymore and the two sheets go from 462 to 302 kilobytes. `--disable-optimization` now picks cwebp's fastest setting rather than skipping OptiPNG, keeping dev boots cheap. Sprockets 4 resolves `assets_compile` entries as paths, not globs, so the `*.png` entry never matched the sprites and they were left out of the manifest. List them explicitly.
The task compiles every scss.erb template, and _environment.scss.erb read the `App` constant, which only exists once the app is loaded: invoking the task directly died with a NameError. Pass the templates a binding carrying `environment`, resolved from APP_ENV and RACK_ENV the way Sinatra resolves it.
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.
What
Convert images to WebP as they're inlined by
ImagesFilter:cwebp -lossless -z 9gif2webpcwebp -q 80 -sharp_yuvWhy
Measured over the images in my locally scraped docs:
Images are base64-inlined into
db.json, so the saving carries straightinto what users download.
Choosing the JPEG quality
JPEG is the only lossy conversion here. Quality 80 is the knee of the
size/SSIM curve over those 22 images (
cwebp -print_ssim):That's a mean SSIM of 0.9935, worst 0.984. Quality 85 gives up most of
the savings for little in return. It also matches the floor
pngquantalready uses in
.image_optim.yml, which setsallow_lossy: true— thepipeline was already lossy for PNGs.
-sharp_yuvis on because documentation images are mostly screenshotsand diagrams, where chroma subsampling artifacts on text edges are the
visible failure mode.
Fallbacks
Conversion is skipped, leaving the original untouched, when:
Content-Typeheader, which is often wrong)cwebpsilently keeps only the first frame, soan
acTLchunk beforeIDATmeans skip. (gif2webphas no suchproblem and keeps every frame.)
and 3 of 22 JPEGs grew, one PNG by 89%.
cwebp/gif2webparen't installed —Errno::ENOENTis rescued, soscraping still works, just without conversion.
Breaking
WebP raises the browser requirements to Safari 14+, Edge 18+ and
iOS 14+ (from 11.1+, 17+ and 11.3+). README updated.
libwebpis a new scrape-time dependency; it isn't part ofimage_optim_pack. Added to both Dockerfiles, CI and the devcontainer.Notes
ImagesFilterwas missing from both filter reference lists — added.chunky_png; the GIF andJPEG are small files under
test/files/.