perf(og): render blog OG/Instagram images on-demand, not at build - #4709
isaaclombardssw wants to merge 7 commits into
Conversation
The dynamic blog OG and Instagram image routes were force-static, so `next build` satori-rendered one image per post per locale at build time — 436 renders (2 image types x 2 locales x 109 posts), each also firing a Tina query. That pushed the Vercel build from ~9 to ~14 minutes. Move image generation off the build path: - All four routes now defer to on-demand ISR (first request renders + caches) on the server build, prerendering nothing. A static export (`output: 'export'`) can't generate on-demand, so it still prebuilds every image — gated on the same EXPORT_MODE switch next.config.js uses. - ogAssets loads fonts/logo/images via `fetch(new URL(..., import.meta.url))` (the Next/@vercel/og-documented pattern) instead of fs reads of public/. Webpack emits each asset next to the function bundle, so they resolve at build or runtime with no filesystem/tracing dependency. Author photos stay optional (only the 5 on disk are mapped; others fall back to a llama). OG unit tests pass; typecheck and biome clean on the touched files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P9n7Dc3rtjpt4QeujTd3r2
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The import.meta.url asset pattern relies on Next output file tracing to bundle the emitted files into the function — but this app sets outputFileTracing: false, so at runtime the fonts weren't in the bundle and satori threw "No fonts are loaded" (500 on on-demand OG images). Read assets from public/ on disk (works at build and any runtime where public/ is on the function fs); if that misses at runtime, fetch the asset from the live deployment (VERCEL_URL). No global tracing-config change, no request threading. Unmapped author photos still fall back to a llama. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P9n7Dc3rtjpt4QeujTd3r2
Actioning /ssw-review-pr round 1: - Runtime asset fetch now prefers VERCEL_PROJECT_PRODUCTION_URL (a stable public alias, never deployment-protected) over the per-deployment VERCEL_URL, which Deployment Protection can 401 -> empty fonts -> 500. - Memoize asset buffers per path so warm on-demand renders don't re-read or re-fetch identical fonts/logo each time. - Fix the OG route header comment that still claimed import.meta.url loading (the shipped path is disk + live-origin fallback). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P9n7Dc3rtjpt4QeujTd3r2
Actioning /ssw-review-pr round 2: - loadPublic evicts null misses from the memo cache so a transient font/ image fetch failure can't poison the warm instance (round-1 memoization regression flagged by the re-review). - The four image routes now 404 an unknown slug instead of rendering and ISR-caching a generic "TinaCMS Blog" fallback (restores the prior dynamicParams:false behaviour, closes a cache-fill/compute amplification surface). - ogFonts collapsed to a spec table + flatMap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P9n7Dc3rtjpt4QeujTd3r2
|
No blocking issues. CI is green and both round-1 Material items are resolved across the follow-up commits. Notable: CONTRIBUTING.md asks for a tracking issue on technical debt, not just an in-code comment. The Callouts:
Smaller:
|
joshbermanssw
left a comment
There was a problem hiding this comment.
Reviewed the diff and traced the runtime paths against the codebase. The core idea is right and well-argued — moving 436 build-time satori renders off the critical path is clearly correct work; an OG card fetched once by a crawler has no business blocking every deploy. My comments are all about the new runtime paths, which is where the risk moved when you took them off the build.
Two I'd fix before merging
- Unknown slugs 500 instead of 404 — the new
if (!post)guard is unreachable; Tina's client defaults toerrorPolicy: 'throw'.app/blog/[...slug]/page.tsxalready wraps this in try/catch for exactly this reason. Inline detail on the og route. - A partial font-load failure gets ISR-cached until the next deploy — satori only throws at zero fonts, so dropping one silently ships a valid-but-wrong 200 that then sticks. Inline detail on
ogFonts.
Both are small (a try/catch, and turning a silent drop into a throw).
Other notes (non-blocking)
- The
EXPORT_MODEgate is dead code —package.json:13uses a comma, not a space, soEXPORT_MODEis never"static". Pre-existing, and self-consistent withnext.config.js:7(both dead, both flip together), so nothing breaks — but the "static export still prebuilds" path can't currently execute or be tested. Separate follow-up. - No test coverage on the new logic — "OG unit tests pass (21/21)" is true but
ogShared.test.ts/authorImages.test.tsdon't importogAssetsat all. The rewritten fallback/memoization/font-assembly code is the highest-risk code in the PR and is untested; a mockedfs/fetchtest would have caught #2's partial-failure semantics. - New amplification vector (low) —
/blog/og/<anything>now runs a Tina query per request and unknown slugs aren't cached, so each hit is fresh backend work. Previously 404'd for free at the routing layer. Validating the slug againstgenerateBlogStaticParams()before querying closes this and fixes #1. - No
revalidate— cached until next deploy. Fine given content commits trigger rebuilds, but an explicitrevalidatewould bound the blast radius of #2.
What's good
The perf diagnosis is sharp, and the comments are genuinely excellent — they explain why (the outputFileTracing: false constraint forcing the fetch fallback, the prod-alias-vs-401 reasoning), which is the hard part to reconstruct in six months. Memoization with miss-eviction shows real care, as const + flatMap is the right call over filter, and verifying the fallback on preview before claiming it works is exactly right.
| ) { | ||
| const slugPath = params.slug.join('/'); | ||
| const { post } = await getBlogPost('en', slugPath); | ||
| if (!post) { |
There was a problem hiding this comment.
This 404 guard is unreachable — unknown slugs will 500 instead.
getBlogPost throws on a missing record rather than returning { post: null }:
tina/__generated__/client.ts:3callscreateClient({ url, token, queries })with noerrorPolicytinacms/dist/client.js:159—this.errorPolicy = errorPolicy || "throw"tinacms/dist/client.js:248— throws whenjson.errorsis present@tinacms/graphql/dist/index.js:6837— a missing record throwsNotFoundError, which lands injson.errors
So the throw propagates out of GET → 500, and this block never runs.
This path didn't exist before: dynamicParams = false meant Next 404'd at the routing layer before the handler ran. Making params dynamic created it. Realistic trigger is a deleted or renamed post whose OG URL is still live on social — the crawler gets a 500 and it shows up as error noise.
app/blog/[...slug]/page.tsx:42-56 already handles exactly this with try/catch → notFound(), so there's an established convention to follow:
let post;
try {
({ post } = await getBlogPost('en', slugPath));
} catch {
post = null;
}
if (!post) {
return new Response(null, { status: 404 });
}Alternatively, validating slugPath against generateBlogStaticParams() before querying gives a true 404 and closes the amplification vector noted in the summary. Applies to all four routes.
There was a problem hiding this comment.
✅ Done. Wrapped getBlogPost in try/catch → post = null → 404, matching the app/blog/[...slug]/page.tsx convention. Went with the try/catch over pre-validating against generateBlogStaticParams() to keep it minimal; the amplification vector is noted but low.
| import { getBlogPost } from 'utils/blog/getBlogPost'; | ||
| import { renderBlogOgImage } from 'utils/og/blogOgImage'; | ||
|
|
||
| const IS_EXPORT = process.env.EXPORT_MODE === 'static'; |
There was a problem hiding this comment.
Heads up: this gate can never be true — pre-existing bug, not introduced here.
package.json:13 is:
"export": "tinacms build && EXPORT_MODE=static,UNOPTIMIZED_IMAGES=true next build"
The comma makes that a single shell assignment, so EXPORT_MODE is the literal string "static,UNOPTIMIZED_IMAGES=true" and UNOPTIMIZED_IMAGES is never set at all. Verified:
$ sh -c 'EXPORT_MODE=static,UNOPTIMIZED_IMAGES=true node -e "console.log(process.env.EXPORT_MODE)"'
static,UNOPTIMIZED_IMAGES=true
So IS_EXPORT is always false — but so is isStatic in next.config.js:7, which means output: 'export' is never set either. Your gate is therefore self-consistent with next.config.js (both dead, both flip together if someone fixes the comma), so nothing breaks here and I wouldn't change it in this PR.
Worth knowing though: the PR description's "a static export still prebuilds every image" describes a path that currently can't execute or be tested. It also implies pnpm export / static-start's serve out hasn't worked for a while. Probably a separate follow-up (space, not comma).
There was a problem hiding this comment.
Agreed, pre-existing and self-consistent with next.config.js, so leaving it here. Separate follow-up to fix the comma (static UNOPTIMIZED_IMAGES=true) and confirm export/static-start still work.
| const bufs = await Promise.all(specs.map((s) => loadPublic(s.path))); | ||
| // flatMap drops any font that failed to load (satori renders with whatever's | ||
| // present) without tripping filter()'s type-narrowing on the buffer. | ||
| return specs.flatMap((s, i) => { |
There was a problem hiding this comment.
Silently dropping a font can cache a wrong-typography image until the next deploy.
satori only throws when zero fonts load (next/dist/compiled/@vercel/og/index.node.js:16262 — "No fonts are loaded"). That splits into two very different outcomes:
- All three fail → throws → 500 → not cached → self-heals on retry. Fine.
- One fails (Inter fetch blips, IBM Plex succeeds) → a valid 200 with wrong typography → and since these routes are
force-staticwith norevalidate, that bad image is ISR-cached for the life of the deployment.
The miss-eviction in loadPublic doesn't help here — by that point the degraded render is already in the ISR cache, not the asset cache. Every cold render makes ~5 network fetches to production; across 436 images that's a lot of chances for one blip to mint a permanently-wrong card, silently.
Suggest failing loudly instead — a 500 isn't cached, and the next request retries:
const bufs = await Promise.all(specs.map((s) => loadPublic(s.path)));
return specs.map((s, i) => {
const data = bufs[i];
if (!data) {
throw new Error(`OG font failed to load: ${s.path}`);
}
return { name: s.name, data, weight: s.weight, style: 'normal' as const };
});Same reasoning applies to logoDataUri() returning null, and to a mapped author's photo failing → silently falling back to a llama, indistinguishable from the legitimate unmapped-author fallback. Worth distinguishing "asset legitimately absent" (llama is correct) from "load failed" (should throw).
There was a problem hiding this comment.
✅ Done. ogFonts now throws if any font fails (500, not cached, so it retries). Also fixed the root: fetchPublic now distinguishes a genuine 404 (→ null, unmapped author falls back to a llama) from a transient/non-404 failure (→ throw). So the mapped author-photo and logo cases fail loud on a blip instead of silently degrading, while real absence still gets the llama. Unit test covers both.
| // never deployment-protected, so the self-fetch below can't 401 (the | ||
| // per-deployment VERCEL_URL can be gated by Vercel Deployment Protection). | ||
| // Fall back to VERCEL_URL, then null (local/build, where the disk read is used). | ||
| const ASSET_HOST = |
There was a problem hiding this comment.
Nice comment — the 401 reasoning is the non-obvious part and I'm glad it's written down.
One trade-off worth noting: VERCEL_PROJECT_PRODUCTION_URL points at production even on a preview deploy. So any PR that adds a new font, llama, or author photo will, on its own preview, fetch that asset from production → 404 → silently dropped. That defeats the one case you'd most want a preview to validate.
Not a blocker, and your reasoning for preferring the prod alias is sound. If it matters later, trying VERCEL_URL first with a prod fallback, or using VERCEL_AUTOMATION_BYPASS_SECRET, would close it.
There was a problem hiding this comment.
Good call-out. Leaving the prod alias as-is for the 401 reason, so a preview that adds a brand-new asset will fetch it from prod (404 → and now, after the fail-loud change, that 404 is treated as genuine absence → falls back, not a 500). If validating new assets on preview matters later, VERCEL_URL-first with a prod fallback would close it.
| // Only successful buffers stay cached. Evict a miss so a transient fetch | ||
| // failure can't poison the warm instance (it would otherwise strip that | ||
| // font/image from every later render); the next render retries. | ||
| cached.then((buf) => { |
There was a problem hiding this comment.
Minor / robustness only. This is correct today — the eviction .then is registered at creation so it runs before any awaiting caller resumes, and fetchPublic catches everything so it can't reject.
But it's load-bearing on fetchPublic never rejecting. If that ever changes, the rejected promise stays in assetCache and every later caller re-throws it for the life of the instance — the exact poisoning this eviction exists to prevent. A .catch(() => assetCache.delete(webPath)) would make it robust to that without changing current behavior.
There was a problem hiding this comment.
✅ Added .catch(() => assetCache.delete(webPath)). Now load-bearing rather than just defensive, since fetchPublic can reject after the change above — a rejected promise would otherwise stay pinned and re-throw for the life of the instance.
| ) { | ||
| const slugPath = params.slug.join('/'); | ||
| const { post } = await getBlogPost('en', slugPath); | ||
| if (!post) { |
There was a problem hiding this comment.
Same as the og route: this guard is unreachable because getBlogPost throws on an unknown slug (Tina's client defaults to errorPolicy: 'throw'), so the request 500s rather than 404s. Needs the same try/catch that app/blog/[...slug]/page.tsx:42-56 already uses.
There was a problem hiding this comment.
✅ Done, same fix.
| ) { | ||
| const slugPath = params.slug.join('/'); | ||
| const { post } = await getBlogPost('zh', slugPath); | ||
| if (!post) { |
There was a problem hiding this comment.
Same as the og route: this guard is unreachable because getBlogPost throws on an unknown slug (Tina's client defaults to errorPolicy: 'throw'), so the request 500s rather than 404s. Needs the same try/catch that app/blog/[...slug]/page.tsx:42-56 already uses.
There was a problem hiding this comment.
✅ Done, same fix.
| ) { | ||
| const slugPath = params.slug.join('/'); | ||
| const { post } = await getBlogPost('zh', slugPath); | ||
| if (!post) { |
There was a problem hiding this comment.
Same as the og route: this guard is unreachable because getBlogPost throws on an unknown slug (Tina's client defaults to errorPolicy: 'throw'), so the request 500s rather than 404s. Needs the same try/catch that app/blog/[...slug]/page.tsx:42-56 already uses.
There was a problem hiding this comment.
✅ Done, same fix.
- Wrap getBlogPost in try/catch on all four OG/Instagram routes: Tina's client throws on an unknown slug, so the if (!post) 404 guard was unreachable and unknown slugs 500'd. Matches app/blog/[...slug]/page.tsx. - fetchPublic distinguishes a genuine 404 (return null, so an unmapped author still falls back to a llama) from a transient/non-404 failure (throw). ogFonts throws on any missing font. A partial asset failure now 500s (not ISR-cached, retries) instead of silently caching a degraded image for the deploy's life. - Evict rejected promises from assetCache too, so a transient failure can't pin a rejected promise that re-throws for the instance's life. - Add ogAssets unit test for the 404-vs-transient fail-loud semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDKDBztP73a864vaaHhBYY
Resolve conflicts in utils/og/ogAssets.tsx and app/blog/instagram/[...slug]/route.tsx. Main's #4668 added synchronous disk-only OG loaders; this PR supersedes them with the async disk + origin-fetch loaders (with memoization and fail-loud semantics) that on-demand rendering requires. svgDataUri now comes from ogShared (main factored it out); callers already await ogFonts()/logoDataUri(). OG unit tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0152WnQMtodVWKQWmqeaJoRE
Why
Merging the dynamic blog OG + Instagram image routes pushed the Vercel build from ~9 to ~14 minutes.
The routes were
force-static, sonext buildsatori-rendered one image per post per locale at build time — 436 renders (2 image types × 2 locales × 109 posts), each also firing a Tina query. That work sits on the critical build path for no real benefit: an OG card is fetched once by a crawler when a post is first shared, and the Instagram images aren't referenced anywhere (manual social use only).What changed
1. Routes → on-demand ISR. All four routes prerender nothing on the server build; each image renders on first request and is cached thereafter. A static export (
output: 'export', used bypnpm export) can't generate on-demand, so it still prebuilds every image — gated on the sameEXPORT_MODEswitchnext.config.jsalready uses, so both paths stay correct.2.
ogAssetsasset loading reworked. Reads fonts/logo/images frompublic/on disk, and if that misses at runtime, fetches the asset from the live production host (VERCEL_PROJECT_PRODUCTION_URL, falling back toVERCEL_URL). This is needed because the app setsoutputFileTracing: false, sopublic/is not bundled into the on-demand serverless function and theimport.meta.url/ webpack-emit pattern doesn't resolve at runtime here (it 500'd with "No fonts are loaded"). The production host is a public alias, so the fetch isn't gated by deployment protection. Author photos stay optional (missing ones fall back to a llama). Buffers are memoized per asset so warm renders don't re-fetch.Files:
utils/og/ogAssets.tsx(rewritten),blogOgImage.tsx+blogInstagramImage.tsx(await the now-async loaders), and the 4 route handlers.Expected effect
Server build drops the 436 image renders → back toward ~9 min.
Testing
🤖 Generated with Claude Code
https://claude.ai/code/session_01P9n7Dc3rtjpt4QeujTd3r2
Closes #4761