fix: serve metadata routes in dynamic segments#500
fix: serve metadata routes in dynamic segments#500james-elicx merged 2 commits intocloudflare:mainfrom
Conversation
…, etc.)
Metadata routes like opengraph-image.tsx inside dynamic segments
(e.g., app/blog/[slug]/opengraph-image.tsx) returned 404 because the
runtime matching used strict equality against the servedUrl which
contains literal bracket notation ([slug]).
Two fixes:
- Generate patternParts for metadata routes with dynamic segments so
the runtime uses matchPattern() instead of strict equality
- Pass extracted params to the metadata function via makeThenableParams()
matching Next.js behavior where dynamic metadata routes receive { params }
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good fix — the root cause analysis is correct and the approach of reusing matchPattern() with code-gen'd patternParts is clean and consistent with how intercept routes already work. The tests cover the right cases.
A few items to consider:
-
Indentation issue in generated code — The
try/catchblock for static metadata files (lines 1730-1738) has mis-indented closing braces inherited from the original code. Thecatchis indented to look like it's inside theResponseconstructor. Since this PR restructures this block (removing a nesting level), it's a good opportunity to fix the indentation. -
patternPartson static metadata routes — Static metadata files are physical assets served from embedded base64. In practice they won't appear inside dynamic segments (a literalfavicon.icoinsideapp/blog/[slug]/wouldn't be meaningful). GeneratingpatternPartsfor them is harmless but unused. Consider guarding withif (mr.isDynamic && patternParts)or adding a comment. -
Segment-to-pattern conversion duplication — The
[param]→:paramconversion logic at codegen time is duplicated from the route scanner. A// Keep in sync with routing/app-router.tscomment would help future maintainers.
| servedUrl: ${JSON.stringify(mr.servedUrl)}, | ||
| contentType: ${JSON.stringify(mr.contentType)}, | ||
| module: ${getImportVar(mr.filePath)}, | ||
| module: ${getImportVar(mr.filePath)},${patternParts ? `\n patternParts: ${patternParts},` : ""} |
There was a problem hiding this comment.
The patternParts property is only useful for dynamic metadata routes (where module.default is called). For static metadata routes, the file is served from embedded base64 and doesn't have URL params to extract. Consider only emitting patternParts for dynamic routes, or add a comment explaining if future-proofing is intentional:
| module: ${getImportVar(mr.filePath)},${patternParts ? `\n patternParts: ${patternParts},` : ""} | |
| module: ${getImportVar(mr.filePath)},${patternParts ? `\n patternParts: ${patternParts},` : ""} | |
| }`; | |
| } | |
| // Static: read file and embed as base64 |
(The suggestion keeps the current behavior — this is a nit, not a blocker.)
| // Convert dynamic segments in servedUrl to matchPattern format: | ||
| // [param] → :param | ||
| // [...param] → :param+ | ||
| // [[...param]] → :param* | ||
| const patternParts = mr.servedUrl.includes("[") | ||
| ? JSON.stringify( | ||
| mr.servedUrl | ||
| .split("/") | ||
| .filter(Boolean) | ||
| .map((seg) => { | ||
| if (seg.startsWith("[[...") && seg.endsWith("]]")) | ||
| return ":" + seg.slice(5, -2) + "*"; | ||
| if (seg.startsWith("[...") && seg.endsWith("]")) return ":" + seg.slice(4, -1) + "+"; | ||
| if (seg.startsWith("[") && seg.endsWith("]")) return ":" + seg.slice(1, -1); | ||
| return seg; | ||
| }), | ||
| ) |
There was a problem hiding this comment.
This [param] → :param conversion duplicates the pattern format used in routing/app-router.ts. If the format ever changes, both would need to stay in sync. Consider adding a comment:
// Keep in sync with routing/app-router.ts patternParts generation
| // Dynamic metadata route — call the default export and serialize | ||
| const metaFn = metaRoute.module.default; | ||
| if (typeof metaFn === "function") { | ||
| const result = await metaFn({ params: makeThenableParams(_metaParams || {}) }); |
There was a problem hiding this comment.
Nice — passing { params: makeThenableParams(_metaParams || {}) } matches the Next.js convention where metadata functions receive { params }. The || {} fallback handles the non-dynamic-segment case cleanly (static paths matched by equality where _metaParams stays null).
| // to avoid Satori/Resvg dependencies in the test environment. | ||
| export default async function OGImage({ params }: { params: Promise<{ slug: string }> }) { | ||
| const { slug } = await params; | ||
| return new Response(`og:${slug}`, { |
There was a problem hiding this comment.
Smart approach — returning a plain Response instead of ImageResponse/satori avoids heavy native deps in tests while still exercising the full metadata route matching + param extraction pipeline.
| it("serves dynamic opengraph-image in dynamic segment with params", async () => { | ||
| const res = await fetch(`${baseUrl}/blog/hello-world/opengraph-image`); | ||
| expect(res.status).toBe(200); | ||
| expect(res.headers.get("content-type")).toContain("image/png"); |
There was a problem hiding this comment.
Consider also asserting the content-type is exactly image/png (not just contains) since the fixture explicitly sets it. Also, you might want a test for a metadata route in a non-dynamic segment to ensure the patternParts === null / strict-equality path still works — though the existing 20 passing tests likely cover that already.
|
Review posted successfully. Here's a summary of the review: Overall assessment: Good fix. The root cause (strict equality matching against bracket-notation Key feedback points:
|
…tation - Only generate patternParts for dynamic metadata routes (static files don't use params) - Add "keep in sync" comment for segment-to-pattern conversion - Fix mis-indented try/catch braces in generated static metadata code
|
/bigbonk review |
There was a problem hiding this comment.
Good fix. The root cause (strict equality against bracket-notation servedUrl for dynamic segments) is correctly identified, and reusing the existing matchPattern() infrastructure is the right call.
The code-gen/runtime split is clean: patternParts is computed at build time and emitted into the metadata route table, then at runtime the same matchPattern() used for intercept routes handles the matching. Params flow through makeThenableParams() just like regular routes, matching Next.js 15+ behavior.
Tests are well-structured — scanner test confirms discovery, two integration tests with different param values confirm runtime matching and param extraction. The fixture cleverly avoids satori/resvg deps.
A few minor items below, none blocking.
| // [...param] → :param+ | ||
| // [[...param]] → :param* | ||
| const patternParts = | ||
| mr.isDynamic && mr.servedUrl.includes("[") |
There was a problem hiding this comment.
Nit: The mr.isDynamic guard here is good — it prevents generating patternParts for static metadata files (which can't meaningfully exist in dynamic segments). But the mr.servedUrl.includes("[") check is redundant when isDynamic is true, since isDynamic is set based on file extension (.ts/.tsx), not URL shape.
Consider: a dynamic metadata route at the root (e.g., app/opengraph-image.tsx) has isDynamic: true but no brackets in servedUrl. In that case patternParts correctly stays null and the strict-equality path is used. So the includes("[") check is doing useful work — it distinguishes "dynamic file in static segment" from "dynamic file in dynamic segment". This is subtle enough to warrant a one-line comment explaining why both conditions are needed.
| const patternParts = | ||
| mr.isDynamic && mr.servedUrl.includes("[") | ||
| ? JSON.stringify( | ||
| mr.servedUrl | ||
| .split("/") | ||
| .filter(Boolean) | ||
| .map((seg) => { | ||
| if (seg.startsWith("[[...") && seg.endsWith("]]")) | ||
| return ":" + seg.slice(5, -2) + "*"; | ||
| if (seg.startsWith("[...") && seg.endsWith("]")) | ||
| return ":" + seg.slice(4, -1) + "+"; | ||
| if (seg.startsWith("[") && seg.endsWith("]")) return ":" + seg.slice(1, -1); | ||
| return seg; | ||
| }), | ||
| ) | ||
| : null; |
There was a problem hiding this comment.
The segment-to-pattern conversion duplicates convertSegmentsToRouteParts() in routing/app-router.ts. I see the // Keep in sync comment (good), but there's a subtle behavioral difference: convertSegmentsToRouteParts uses regexes to extract param names (e.g., /^\[\.\.\.(\w+)\]$/) and also handles invisible segments (route groups, parallel slots) via isInvisibleSegment(). This codegen version uses startsWith/endsWith string checks and doesn't handle invisible segments.
For metadata routes this is fine today — servedUrl is a cleaned URL path that won't contain route groups or @ slots. But if servedUrl format ever changes, this could silently diverge. Consider extracting a shared segmentToPatternPart(seg: string): string utility that both call sites use, or at minimum documenting that this version intentionally omits invisible-segment handling because servedUrl is already cleaned.
| // strict equality for static paths. | ||
| var _metaParams = null; | ||
| if (metaRoute.patternParts) { | ||
| var _metaUrlParts = cleanPathname.split("/").filter(Boolean); |
There was a problem hiding this comment.
Minor perf nit: cleanPathname.split("/").filter(Boolean) is recomputed on every metadata route iteration that has patternParts. Since cleanPathname doesn't change within the loop, this could be hoisted above the for loop (or computed once lazily). Not a real issue for typical metadata route counts, but worth noting for consistency with other loop-hoisted variables in this function.
| it("serves dynamic opengraph-image in dynamic segment with params", async () => { | ||
| const res = await fetch(`${baseUrl}/blog/hello-world/opengraph-image`); | ||
| expect(res.status).toBe(200); | ||
| expect(res.headers.get("content-type")).toContain("image/png"); |
There was a problem hiding this comment.
Consider tightening the content-type assertion to an exact match rather than .toContain("image/png"), since the fixture explicitly sets "Content-Type": "image/png". Using toBe("image/png") would catch if something downstream accidentally appends a charset or other parameter.
(Minor — toContain is fine for a first pass since real ImageResponse objects may include additional parameters.)
| @@ -0,0 +1,8 @@ | |||
| // Dynamic OG image in a dynamic segment — returns a plain Response | |||
| // to avoid Satori/Resvg dependencies in the test environment. | |||
| export default async function OGImage({ params }: { params: Promise<{ slug: string }> }) { | |||
There was a problem hiding this comment.
Nice — using Promise<{ slug: string }> for the params type correctly models the Next.js 15+ async params convention, and the await params on line 4 exercises the makeThenableParams() thenable path.
|
Review posted. Here's a summary of my review: Verdict: Approved -- this is a correct fix with good test coverage. What the PR does right:
Minor feedback (none blocking):
|
2 participants
Summary
app/blog/[slug]/opengraph-image.tsx→ requesting/blog/hello-world/opengraph-image→ 404cleanPathname === metaRoute.servedUrl) butservedUrlcontains literal bracket notation (/blog/[slug]/opengraph-image){ params }so the function can generate per-page imagesFix
servedUrland generatepatternParts(reusing the existingmatchPattern()function already used for intercept routes)matchPattern()for metadata routes withpatternPartsto extract paramsmakeThenableParams(), matching Next.js behaviorTest plan
scanMetadataFilesdiscovers opengraph-image in dynamic segments/blog/hello-world/opengraph-imagereturns 200 with correct params