fix: keep every frame when optimizing animated images - #168
Conversation
sharp decodes only the first frame unless constructed with
`{ animated: true }`, so animated GIFs were served as still images by both
image pipelines — the /api/image handler (dev and server builds) and the
static build's .webp generation, which MDXImage always prefers via
<picture><source>.
Detect multi-frame sources with a header-only metadata probe, gated on the
two formats that can animate, and pass the flag through to sharp. AVIF
output is skipped for animated input because sharp flattens it; those
requests get animated WebP instead, which every AVIF-capable browser
decodes. The flag is folded into the cache key and ETag only when set, so
still-image keys stay stable while stale single-frame entries are busted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe image pipeline now detects animated GIF and WebP sources. It avoids AVIF for animated inputs, preserves frames during Sharp processing, differentiates animated cache entries, and applies the same handling during static generation. ChangesAnimated image support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ImageRequest
participant isAnimatedImage
participant negotiateFormat
participant Sharp
ImageRequest->>isAnimatedImage: Inspect source file
isAnimatedImage-->>ImageRequest: Return animated status
ImageRequest->>negotiateFormat: Select output format
negotiateFormat-->>ImageRequest: Return WebP or selected format
ImageRequest->>Sharp: Optimize with animation option
Sharp-->>ImageRequest: Return optimized image
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/chronicle/src/lib/image-animation.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse configured aliases for changed internal imports.
Replace relative internal imports with the configured
@/*aliases.
packages/chronicle/src/lib/image-animation.ts#L2-L2: importisAnimatablefrom@/lib/image-utils.packages/chronicle/src/lib/image-utils.test.ts#L5-L5: change theimage-utilsmodule specifier to@/lib/image-utils.packages/chronicle/src/lib/image-animation.test.ts#L6-L6: importisAnimatedImagefrom@/lib/image-animation.packages/chronicle/src/server/api/image.test.ts#L6-L6: import image API exports from@/server/api/image.As per coding guidelines, use path alias
@/*→./src/*configured in tsconfig and vite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/chronicle/src/lib/image-animation.ts` at line 2, Update the internal imports to use the configured `@/` aliases instead of relative paths: in packages/chronicle/src/lib/image-animation.ts#L2-L2 import isAnimatable from `@/lib/image-utils`, in packages/chronicle/src/lib/image-utils.test.ts#L5-L5 switch the image-utils module specifier to `@/lib/image-utils`, in packages/chronicle/src/lib/image-animation.test.ts#L6-L6 import isAnimatedImage from `@/lib/image-animation`, and in packages/chronicle/src/server/api/image.test.ts#L6-L6 import the image API exports from `@/server/api/image`. Keep the exported symbols and test behavior unchanged while only updating the module specifiers.Source: Coding guidelines
packages/chronicle/src/lib/image-animation.test.ts (1)
33-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd animated WebP coverage.
These tests validate GIF behavior only. The feature also detects and processes animated WebP inputs. Add one animated WebP fixture and assert metadata detection plus WebP and original-format frame preservation.
packages/chronicle/src/lib/image-animation.test.ts#L33-L48: assertisAnimatedImagereturnstruefor a multi-frame WebP.packages/chronicle/src/server/api/image.test.ts#L108-L142: pass that WebP fixture tooptimizeImageand assert each animated output retains all pages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/chronicle/src/lib/image-animation.test.ts` around lines 33 - 48, Add animated WebP coverage at both affected test sites: in packages/chronicle/src/lib/image-animation.test.ts#L33-L48, extend isAnimatedImage assertions with a multi-frame WebP fixture and verify it returns true; in packages/chronicle/src/server/api/image.test.ts#L108-L142, pass the same WebP fixture through optimizeImage and assert both the WebP output and the original-format output preserve all pages for animated inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/chronicle/src/cli/commands/static-generate.ts`:
- Around line 721-725: Update the output handling around the optimized buffer
write and fallback copy so the original source is copied only when origDest and
destPath differ. Preserve the optimized WebP output when relativePath already
ends in .webp, while retaining the fallback copy behavior for distinct
destinations.
In `@packages/chronicle/src/server/api/image.ts`:
- Around line 119-125: Update buildOptimizedUrl to append the image-pipeline
revision as the v query parameter on generated /api/image URLs, using the
existing revision source rather than changing handler caching. Add or update
coverage to verify a cached pre-fix flattened image is bypassed when the
revision changes and the new response is returned.
---
Nitpick comments:
In `@packages/chronicle/src/lib/image-animation.test.ts`:
- Around line 33-48: Add animated WebP coverage at both affected test sites: in
packages/chronicle/src/lib/image-animation.test.ts#L33-L48, extend
isAnimatedImage assertions with a multi-frame WebP fixture and verify it returns
true; in packages/chronicle/src/server/api/image.test.ts#L108-L142, pass the
same WebP fixture through optimizeImage and assert both the WebP output and the
original-format output preserve all pages for animated inputs.
In `@packages/chronicle/src/lib/image-animation.ts`:
- Line 2: Update the internal imports to use the configured `@/` aliases instead
of relative paths: in packages/chronicle/src/lib/image-animation.ts#L2-L2 import
isAnimatable from `@/lib/image-utils`, in
packages/chronicle/src/lib/image-utils.test.ts#L5-L5 switch the image-utils
module specifier to `@/lib/image-utils`, in
packages/chronicle/src/lib/image-animation.test.ts#L6-L6 import isAnimatedImage
from `@/lib/image-animation`, and in
packages/chronicle/src/server/api/image.test.ts#L6-L6 import the image API
exports from `@/server/api/image`. Keep the exported symbols and test behavior
unchanged while only updating the module specifiers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 81a6d7b0-a304-4208-9870-385fb0da7dd7
📒 Files selected for processing (7)
packages/chronicle/src/cli/commands/static-generate.tspackages/chronicle/src/lib/image-animation.test.tspackages/chronicle/src/lib/image-animation.tspackages/chronicle/src/lib/image-utils.test.tspackages/chronicle/src/lib/image-utils.tspackages/chronicle/src/server/api/image.test.tspackages/chronicle/src/server/api/image.ts
| // Animated sources must keep every frame — the <picture> element in | ||
| // MDXImage always prefers this .webp, so a flattened one would render | ||
| // an animated GIF as a still image | ||
| const animated = await isAnimatedImage(srcPath); | ||
| const optimizedBuf = await sharp(source, { animated }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not overwrite the optimized WebP output.
When relativePath already ends in .webp, webpRelative at Line 715 equals relativePath. destPath and origDest then point to the same file. Line 729 writes the optimized buffer, but Line 734 copies the original source over it. This bypasses resizing and quality settings for animated WebP inputs.
Skip the fallback copy when origDest === destPath.
Proposed fix
const origDest = path.join(outputDir, '_content', relativePath);
- await fs.mkdir(path.dirname(origDest), { recursive: true });
- await fs.copyFile(srcPath, origDest);
+ if (origDest !== destPath) {
+ await fs.mkdir(path.dirname(origDest), { recursive: true });
+ await fs.copyFile(srcPath, origDest);
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/chronicle/src/cli/commands/static-generate.ts` around lines 721 -
725, Update the output handling around the optimized buffer write and fallback
copy so the original source is copied only when origDest and destPath differ.
Preserve the optimized WebP output when relativePath already ends in .webp,
while retaining the fallback copy behavior for distinct destinations.
| const requestedVersion = event.url.searchParams.get('v') | ||
| const cacheControl = import.meta.dev | ||
| ? REVALIDATE_CACHE | ||
| : assetCacheControl(requestedVersion, currentVersion) | ||
| const etag = etagFor(currentVersion ?? String(stat.mtimeMs), String(w), String(q), format) | ||
| // `animated` is part of the ETag so browsers holding a pre-fix single-frame | ||
| // response revalidate instead of getting a 304 | ||
| const etag = etagFor(currentVersion ?? String(stat.mtimeMs), String(w), String(q), format, ...(animated ? ['animated'] : [])) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
ast-grep outline packages/chronicle/src/server/utils/asset-cache.ts --items all
sed -n '1,100p' packages/chronicle/src/server/utils/asset-cache.ts
ast-grep outline packages/chronicle/src/lib/image-utils.ts --items all
rg -n -C 4 'buildOptimizedUrl|assetCacheControl|IMMUTABLE_CACHE' packages/chronicle/srcRepository: raystack/chronicle
Length of output: 21607
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '--- packages/chronicle/src/server/api/image.ts ---\n'
wc -l packages/chronicle/src/server/api/image.ts
sed -n '1,180p' packages/chronicle/src/server/api/image.ts
printf '\n--- getAssetVersion usages ---\n'
rg -n -C 3 'getAssetVersion|version' packages/chronicle/src/lib packages/chronicle/src/server | sed -n '1,220p'
printf '\n--- package/tooling version context ---\n'
sed -n '1,120p' package.json
fd -e toml -e json -e lock -e lockfile -e yarn.lock -e pnpm-lock.yaml . | sed -n '1,80p'Repository: raystack/chronicle
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat <<'EOF'
--- packages/chronicle/src/server/api/image.ts ---
EOF
wc -l packages/chronicle/src/server/api/image.ts
sed -n '1,180p' packages/chronicle/src/server/api/image.ts
printf '%s\n' ''
printf '%s\n' '--- getAssetVersion usages ---'
rg -n -C 3 'getAssetVersion|version' packages/chronicle/src/lib packages/chronicle/src/server | sed -n '1,220p'
printf '%s\n' ''
printf '%s\n' '--- package/tooling version context ---'
sed -n '1,120p' package.json
fd -e toml -e json -e lock -e lockfile -e yarn.lock -e pnpm-lock.yaml . | sed -n '1,80p'Repository: raystack/chronicle
Length of output: 25063
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- asset-version source/usages ---'
fd -a 'asset-version' packages/chronicle/src
sed -n '1,180p' packages/chronicle/src/lib/asset-version.ts
printf '%s\n' ''
printf '%s\n' '--- version/currentVersion search ---'
rg -n -C 4 'currentVersion|RECOMMENDED_IMAGE_PIPELINE_VERSION|IMAGE.*VERSION|pipeline|version' packages/chronicle/src packages/chronicle/package.json | sed -n '1,260p'
printf '%s\n' ''
printf '%s\n' '--- precise image handler cache paths ---'
sed -n '88,132p' packages/chronicle/src/server/api/image.ts
printf '%s\n' ''
printf '%s\n' '--- image URL call sites and version passing ---'
python3 - <<'PY'
from pathlib import Path
for p in Path('packages/chronicle/src').rglob('*'):
if p.is_file() and p.name.endswith(('.ts', '.tsx')):
text = p.read_text(errors='ignore')
if 'buildOptimizedUrl' in text:
for i,l in enumerate(text.splitlines(), 1):
if 'buildOptimizedUrl' in l:
print(f'{p}:{i}:{l.strip()}')
PYRepository: raystack/chronicle
Length of output: 27532
Invalidate image pipeline revisions in the URL.
buildOptimizedUrl currently omits ?v, so browsers can cache a pre-fix flattened image as public, max-age=31536000, immutable and never hit the handler to see the new ETag. Include an image-pipeline revision in generated /api/image URLs, and test a pre-fix cache hit does not reuse the old response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/chronicle/src/server/api/image.ts` around lines 119 - 125, Update
buildOptimizedUrl to append the image-pipeline revision as the v query parameter
on generated /api/image URLs, using the existing revision source rather than
changing handler caching. Add or update coverage to verify a cached pre-fix
flattened image is bypassed when the revision changes and the new response is
returned.
rohilsurana
left a comment
rohilsurana
left a comment
There was a problem hiding this comment.
Overview
Fixes animated GIFs (and animated WebP) rendering as still images. sharp only decodes the first frame unless built with { animated: true }, and two pipelines flattened animated sources: the /api/image handler (dev + server) and the static build. The fix adds a cheap isAnimatable() extension gate, a server-only isAnimatedImage() probe, threads an animated flag through optimizeImage, negotiateFormat, the cache key, and the ETag, and downgrades AVIF to WebP for animated input.
What looks good
- The AVIF downgrade is correct. Every AVIF-capable browser also decodes WebP, so returning WebP for an AVIF-only
Acceptis safe. - The cache key and ETag only gain the
animatedmarker when it is set. Still-image keys stay stable, so the on-disk cache and browser caches survive, while stale single-frame animated entries get busted. sharpstays out of the client bundle by keeping the probe inimage-animation.tsand the plain extension gate inimage-utils.ts. Nice split.- Static build writes both the animated
.webpand copies the original as the<picture>fallback, so the fix holds even with JS off. - Solid test coverage:
negotiateFormatanimated cases, cache-key stability,isAnimatableunits, and real frame-count assertions on a generated fixture (no binary committed).
Notes
Only nit-level items, none blocking. Left two inline. Approving.
| // metadata probe in isAnimatedImage() | ||
| export function isAnimatable(url: string): boolean { | ||
| const base = url.split('?')[0].toLowerCase(); | ||
| return base.endsWith('.gif') || base.endsWith('.webp'); |
There was a problem hiding this comment.
nit: this gate covers .gif and .webp only. Animated PNG (APNG) and animated AVIF also carry multiple pages, so an APNG dropped into content would still get flattened. Adding .png here would make every PNG pay the metadata probe, so leaving it out is a fair tradeoff. A one-line comment noting APNG is intentionally out of scope would stop the next person from treating it as a bug.
| throw new HTTPError({ status: StatusCodes.NOT_FOUND, message: 'Not Found' }) | ||
| } | ||
|
|
||
| const animated = await isAnimatedImage(filePath) |
There was a problem hiding this comment.
nit: isAnimatedImage now runs on every request before the cache lookup. It is a header-only sharp().metadata() read, and isAnimatable short-circuits png/jpg without touching sharp, so the extra cost lands only on .gif/.webp. Fine at docs traffic, but note that cached hits for those two formats now pay one extra file open plus header parse each time. No change needed, just flagging the added work on the hot path.
2 participants
Animated GIFs in content rendered as still images in dev, server builds, and static builds.
Root cause
sharp()decodes only the first frame unless constructed with{ animated: true }. Two independent pipelines flattened animated sources:src/server/api/image.ts—optimizeImage()(dev + server builds).negotiateFormat()also picked AVIF whenever the browser advertised it, which flattens animation even withanimated: true.src/cli/commands/static-generate.ts— wrote a single-frame.webp;MDXImageemits<picture><source srcSet={webpUrl(src)} type="image/webp">, so the browser always takes that still WebP and never falls back to the original GIF.Changes
src/lib/image-utils.tsisAnimatable(url)— cheap.gif/.webpextension gate (client-safe, no sharp)src/lib/image-animation.ts(new)isAnimatedImage(filePath)—sharp().metadata().pages > 1, header-only probe; server-only so sharp stays out of the client bundlesrc/server/api/image.tsoptimizeImage(..., animated)→sharp(source, { animated });negotiateFormat(accept, animated)downgrades AVIF→WebP for animated input; animated flag folded into cache key + ETag;warmupImageCache()wired throughsrc/cli/commands/static-generate.tssharp(source, { animated })for the generated.webpAVIF is skipped for animated input because sharp flattens animated AVIF output; those requests get animated WebP instead, which every AVIF-capable browser decodes.
The cache key and ETag only gain the
animatedmarker when it is set, so still-image keys stay stable and the on-disk.cache/imagessurvives, while stale single-frame animated entries and pre-fix browser caches are busted (verified: old ETag →200, new ETag →304).Verification
End-to-end with a real 13-frame, 4.7 MB GIF placed in
examples/basic:bun test→ 264 pass, 0 fail. Added tests:negotiateFormatanimated cases, cache-key animated/still stability,isAnimatableunits, andoptimizeImageframe-count assertions using a sharp-generated 3-frame fixture (no binary committed). Biome clean on touched files;tscreports no new errors.🤖 Generated with Claude Code