fix: bust stale browser cache for replaced images via content-hash URLs#154
fix: bust stale browser cache for replaced images via content-hash URLs#154rohilsurana wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces content-hash-based asset versioning across the image pipeline. A new ChangesAsset versioning and cache-aware image pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ContentRoute as _content route
participant AssetVersion as getAssetVersion
participant FileSystem
Client->>ContentRoute: GET /_content/path?v=...
ContentRoute->>AssetVersion: compute currentVersion from file
AssetVersion-->>ContentRoute: content hash
ContentRoute->>ContentRoute: compute ETag, Cache-Control
alt If-None-Match matches ETag
ContentRoute-->>Client: 304 Not Modified
else
ContentRoute->>FileSystem: read file
FileSystem-->>ContentRoute: file data
ContentRoute-->>Client: 200 with ETag, Cache-Control
end
sequenceDiagram
participant Content as Markdown/MDX content
participant Remark as remark-resolve-images
participant Version as getAssetVersion
participant Render as MDXImage/page-context
Content->>Remark: processUrl(src)
Remark->>Version: versionFor(resolved local path)
Version-->>Remark: hash or null
Remark-->>Content: finalizeUrl with ?v=hash
Render->>Render: splitVersion(src) -> base, version
Render->>Render: buildOptimizedUrl(base, width, undefined, version)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
| */ | ||
| export function getAssetVersion(filePath: string): string | null { | ||
| try { | ||
| const stat = fs.statSync(filePath); |
There was a problem hiding this comment.
Converted asset-version.ts to fs/promises in 9ffc5fc. The remark plugin now runs as an async transformer (nodes collected in sync visits, then processed with awaits), and the image handler, _content route, warmup, and static scanner all await it.
| if (cached && cached.mtimeMs === stat.mtimeMs && cached.size === stat.size) { | ||
| return cached.hash; | ||
| } | ||
| const hash = hashContent(fs.readFileSync(filePath)); |
There was a problem hiding this comment.
Same as above, use promise-based fs utils
There was a problem hiding this comment.
Done in 9ffc5fc — same async conversion covers this call site.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/chronicle/src/lib/asset-version.ts (1)
22-35: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSynchronous fs calls used in request-serving paths.
fs.statSync/fs.readFileSyncblock the event loop; this was already flagged in a prior review (rsbh: can we use promise based fs utils,rsbh: Same as above, use promise-based fs utils) and remains unaddressed. It's more impactful than a style nit here: downstream evidence showsgetAssetVersionis called directly inside request handlers (packages/chronicle/src/server/api/image.ts:104andpackages/chronicle/src/server/routes/_content/[...path].ts:31), so a cache miss triggers a blocking disk read on the request thread, stalling all concurrent requests under load.Consider converting to
fs.promises.stat/readFile(requires makinggetAssetVersionasync and updating its callers).🤖 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/asset-version.ts` around lines 22 - 35, The getAssetVersion helper still uses blocking fs.statSync and fs.readFileSync in a request-serving path, which can stall the event loop on cache misses. Update getAssetVersion in asset-version.ts to use promise-based fs APIs (fs.promises.stat/readFile), make it async, and propagate the async change to its callers such as the image API handler and the _content route so they await the version lookup instead of blocking the request thread.
🧹 Nitpick comments (3)
packages/chronicle/src/lib/remark-resolve-images.ts (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRelative import instead of path alias.
This new import uses
./asset-versionwhile every other new import added in this PR cohort (image.tsx,page-context.tsx,entry-server.tsx,static-generate.ts) uses the@/lib/...alias.-import { getAssetVersion } from './asset-version' +import { getAssetVersion } from '`@/lib/asset-version`'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/remark-resolve-images.ts` at line 9, The new import in remark-resolve-images should use the project path alias instead of a relative path to match the rest of the PR cohort and the repo’s tsconfig/vite conventions. Update the getAssetVersion import in remark-resolve-images to use the `@/lib` alias, consistent with image.tsx, page-context.tsx, entry-server.tsx, and static-generate.ts, and keep the symbol reference unchanged.Source: Coding guidelines
packages/chronicle/src/lib/page-context.tsx (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect, but consider extracting the shared version/optimize branching.
The
splitVersion→isLocalImage(base) && !isSvg(base)→ (webpUrlif static /buildOptimizedUrlotherwise) branching here is nearly identical to the logic inimage.tsx(lines 21-27) andentry-server.tsx(lines 158-161). Three independent copies increase the risk of divergence — which has already happened, sinceentry-server.tsx's copy omits theisStaticMode()branch that this file andimage.tsxboth have (see comment there). Extracting a shared helper (e.g.resolveImageDisplayUrl(src, isStatic)) intoimage-utils.tswould remove the duplication and prevent future drift.Also applies to: 137-144
🤖 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/page-context.tsx` at line 19, The image URL resolution logic in this page-context file is duplicated across `splitVersion`, `isLocalImage(base) && !isSvg(base)`, and the `webpUrl`/`buildOptimizedUrl` branching, which should be centralized to avoid divergence. Extract the shared behavior into a helper in `image-utils.ts` (for example, a resolver like `resolveImageDisplayUrl`) and update `page-context.tsx`, `image.tsx`, and `entry-server.tsx` to use it, keeping the `isStaticMode()` handling consistent wherever `buildOptimizedUrl` is chosen.packages/chronicle/src/lib/remark-resolve-images.test.ts (1)
80-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for path traversal once
versionForis hardened.Tied to the containment-check fix suggested in
remark-resolve-images.ts— consider a test asserting that an image reference like(pointing outside the content root) doesn't get versioned/read from outsidecontentRoot.🤖 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/remark-resolve-images.test.ts` around lines 80 - 89, Add a regression test in remark-resolve-images.test.ts around transform()/versionFor path handling to cover path traversal. Assert that an image reference like ../../outside.png does not get versioned or resolved from outside contentRoot, and that the resulting tree/file.data.images only includes safe in-root image URLs. Focus the test near the existing img-tag and plain-URL coverage so the containment behavior is validated alongside version stamping.
🤖 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/lib/asset-version.ts`:
- Around line 22-31: getAssetVersion is caching by mtimeMs and size only, which
can return a stale hash after same-size rewrites or timestamp-tick collisions.
Update the invalidation logic in getAssetVersion to use a stronger change signal
than metadata alone, or remove the memoization when metadata cannot reliably
distinguish file content changes; make sure the cache lookup and refresh path in
getAssetVersion always recompute the hash when file contents may have changed.
In `@packages/chronicle/src/lib/remark-resolve-images.ts`:
- Around line 80-97: The version lookup in versionFor is not restricted to
contentRoot, so traversal-style URLs from resolveUrl can escape the content
tree. Update versionFor in remark-resolve-images.ts to verify the decoded rel
path stays contained under contentRoot before calling getAssetVersion, and
return undefined for anything outside that root. Keep the fix localized to
versionFor/path.join handling so processUrl continues to use the sanitized
version value.
In `@packages/chronicle/src/server/api/image.ts`:
- Around line 104-105: The image request path in the API handler is still doing
synchronous filesystem work through getAssetVersion, which blocks the event loop
under load. Update getAssetVersion and its callers, including the image
handler’s cache key path and the warmup flow, to use fs.promises-based async I/O
instead of fs.statSync/fs.readFileSync. Thread the async change through the
relevant functions in image.ts so the currentVersion lookup is awaited before
computing cacheKey.
---
Duplicate comments:
In `@packages/chronicle/src/lib/asset-version.ts`:
- Around line 22-35: The getAssetVersion helper still uses blocking fs.statSync
and fs.readFileSync in a request-serving path, which can stall the event loop on
cache misses. Update getAssetVersion in asset-version.ts to use promise-based fs
APIs (fs.promises.stat/readFile), make it async, and propagate the async change
to its callers such as the image API handler and the _content route so they
await the version lookup instead of blocking the request thread.
---
Nitpick comments:
In `@packages/chronicle/src/lib/page-context.tsx`:
- Line 19: The image URL resolution logic in this page-context file is
duplicated across `splitVersion`, `isLocalImage(base) && !isSvg(base)`, and the
`webpUrl`/`buildOptimizedUrl` branching, which should be centralized to avoid
divergence. Extract the shared behavior into a helper in `image-utils.ts` (for
example, a resolver like `resolveImageDisplayUrl`) and update
`page-context.tsx`, `image.tsx`, and `entry-server.tsx` to use it, keeping the
`isStaticMode()` handling consistent wherever `buildOptimizedUrl` is chosen.
In `@packages/chronicle/src/lib/remark-resolve-images.test.ts`:
- Around line 80-89: Add a regression test in remark-resolve-images.test.ts
around transform()/versionFor path handling to cover path traversal. Assert that
an image reference like ../../outside.png does not get versioned or resolved
from outside contentRoot, and that the resulting tree/file.data.images only
includes safe in-root image URLs. Focus the test near the existing img-tag and
plain-URL coverage so the containment behavior is validated alongside version
stamping.
In `@packages/chronicle/src/lib/remark-resolve-images.ts`:
- Line 9: The new import in remark-resolve-images should use the project path
alias instead of a relative path to match the rest of the PR cohort and the
repo’s tsconfig/vite conventions. Update the getAssetVersion import in
remark-resolve-images to use the `@/lib` alias, consistent with image.tsx,
page-context.tsx, entry-server.tsx, and static-generate.ts, and keep the symbol
reference unchanged.
🪄 Autofix (Beta)
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
Run ID: bf6ce132-8d3c-48a9-a591-4a6e95c43324
📒 Files selected for processing (16)
packages/chronicle/src/cli/commands/static-generate.tspackages/chronicle/src/components/mdx/image.tsxpackages/chronicle/src/lib/asset-version.test.tspackages/chronicle/src/lib/asset-version.tspackages/chronicle/src/lib/image-utils.test.tspackages/chronicle/src/lib/image-utils.tspackages/chronicle/src/lib/page-context.tsxpackages/chronicle/src/lib/remark-resolve-images.test.tspackages/chronicle/src/lib/remark-resolve-images.tspackages/chronicle/src/server/api/image.test.tspackages/chronicle/src/server/api/image.tspackages/chronicle/src/server/entry-server.tsxpackages/chronicle/src/server/routes/_content/[...path].tspackages/chronicle/src/server/utils/asset-cache.test.tspackages/chronicle/src/server/utils/asset-cache.tspackages/chronicle/src/types/globals.d.ts
…ookups to content root, stronger cache invalidation
2 participants
Problem
When an image file is replaced but keeps the same filename, browsers keep showing the old cached image until users hard-refresh.
Two things caused this:
/api/imageresponses were sent withCache-Control: public, max-age=31536000, immutable. The URL never changed when the file changed, so browsers never asked again for a year./_content/*responses were sent withmax-age=86400and noETagorLast-Modified, so they could stay stale for a day with no cheap way to revalidate.Fix
Stamp a short content hash on every resolved local image URL at MDX transform time, and make cache headers depend on whether the requested version matches the file on disk.
remark-resolve-imagesnow appends?v=<10-char sha256>to resolved/_content/URLs and threads it into/api/image?...&v=URLs. Replacing an image produces a new URL on the next build, so every browser fetches the new file./api/imageand/_content/*servepublic, max-age=31536000, immutableonly when the requestedvmatches the current file hash. Anything else (no version, stale version) getspublic, no-cacheplus anETag, so clients revalidate and get cheap 304s. Old HTML self-heals instead of staying stale.no-cache(viaimport.meta.dev), since Vite does not re-transform MDX when only an image file changes./api/imageserver-side cache key now uses the content hash instead of mtime, so a fresh checkout (where all mtimes change) no longer invalidates the whole optimizer cache.data/pages/*.json, while files on disk keep their original names, so any static host serves them unchanged.<img>tags, so hints hit the same cache entries. The client prefetch also uses the webp variant in static mode, matching whatMDXImagerenders.New helpers:
asset-version.ts(memoized content hash of a file) andserver/utils/asset-cache.ts(cache-control / ETag / conditional request logic).Behavior
?v=matches filepublic, max-age=31536000, immutable?v=stale or missingpublic, no-cache+ETag(304 on match)public, no-cache+ETagNote for CDN users: edge caches must include the query string in their cache key for versioned URLs to bust correctly.
Testing
no-cache+ working 304s; production build sendsimmutablefor matchingv,no-cachefor stale/missingv; replacing the image file and rebuilding produced a newv=hash in both server and client bundles; static build copies originals + webp and stamps versioned URLs in page data.