fix: pass parent promise to generateMetadata() for ancestor metadata access#376
Conversation
…access Implements Next.js's eager-execution-with-serial-resolution pattern for generateMetadata(): all calls kick off concurrently but each segment's parent promise resolves serially (root → leaf), so `await parent` inside a child's generateMetadata correctly gets the accumulated ancestor metadata. Fixes #375
commit: |
|
|
/bigbonk review this pr please! |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix: pass parent promise to generateMetadata() for ancestor metadata access
Good fix. The implementation correctly matches Next.js's eager-execution-with-serial-resolution pattern, the generated code is clean JS (no leaked TS syntax), and the tests cover the key regression. A few observations and one thing worth checking:
Correctness
The parent promise chain logic is correct. Each layout's generateMetadata() is kicked off immediately (concurrent I/O), but each segment's parent promise only resolves after the preceding segment merges — matching the Next.js approach.
The resolveModuleMetadata() signature change is backward-compatible: parent has a default value of Promise.resolve({}), so existing call sites that don't pass it continue to work.
Dev/prod parity
prod-server.ts and cloudflare/worker-entry.ts don't contain any resolveModuleMetadata or generateMetadata calls — the App Router production path delegates to the RSC entry built from app-dev-server.ts. So this fix propagates to production automatically. No parity issue here.
Template literal safety
Confirmed: no TypeScript type annotations (Promise<Metadata>, generics, etc.) appear inside the generated JS template string. The parent parameter in the generated code is just a plain JS value. Good catch on the root cause.
Minor observations
-
searchParamsnot passed to page'sgenerateMetadatainbuildPageElement(line 910): The page'sresolveModuleMetadatacall passesundefinedforsearchParams:resolveModuleMetadata(route.page, params, undefined, pageParentPromise)
This was already the case before this PR (the old code also passed
(route.page, params)with no searchParams), so it's not a regression. But in Next.js,generateMetadatafor pages receives{ params, searchParams }as its first argument. Might be worth a follow-up. -
layoutParentPromisesarray is populated but never read (line 889-894): The array is built up withlayoutParentPromises.push(parentForThisLayout)but is never used after the loop. It's harmless but dead code. Consider removing it for clarity. -
resolveModuleViewportinrenderHTTPAccessFallbackPagepasses no params (line 539):resolveModuleViewport(mod)is called withoutparams, while inbuildPageElementit'sresolveModuleViewport(mod, params). This was pre-existing, but sincerenderHTTPAccessFallbackPagepasses{}for params toresolveModuleMetadata, it's consistent. Just noting it.
Tests
The three new tests cover the essential cases well:
- Parent OG images are inherited by child
- Page title renders correctly when using parent
await parentdoesn't throw
The fixture is clean and follows the project convention of linking to the ported Next.js test source.
Verdict
LGTM. The core logic is correct and well-documented with thorough inline comments explaining the approach. The layoutParentPromises dead code is the only cleanup worth doing (nit, not blocking).
|
Review posted on PR #376. Here's a summary: LGTM. The implementation correctly matches Next.js's eager-execution-with-serial-resolution pattern for the
One minor nit: |
…o generateMetadata - Remove unused layoutParentPromises[] array (dead code pointed out in review) - Pass searchParams to page's resolveModuleMetadata() — the URL query string was previously always passed as undefined, so generateMetadata() could never read searchParams from the URL - Reuse the already-computed spObj for pageProps.searchParams to avoid double iteration - Add 2 regression tests for searchParams in generateMetadata
|
/bigbonk re-review the pr please |
There was a problem hiding this comment.
Re-review: fix: pass parent promise to generateMetadata() for ancestor metadata access
Took a closer look at the full change set. The core implementation is correct and well-structured. Upgrading from the first review's observations with more detailed analysis:
Correctness: Parent Promise Chain
The eager-execution-with-serial-resolution pattern is implemented correctly in both buildPageElement and renderHTTPAccessFallbackPage. Traced through several scenarios:
-
No double-merge of title templates. The accumulator chain (for
parent) uses merged/resolved metadata (templates already applied), while the finalmergeMetadatacall uses the raw per-segment results (templates still intact). These are independent — the accumulator feedsparentpromises, the raw results feed the final head tags. This exactly matches Next.js's separation ofResolvingMetadata(whatawait parentgives you) vs the final metadata resolution pipeline. -
No duplicate OG images. When a page reads
(await parent).openGraph?.imagesand spreads them into its return value, the finalmergeMetadatashallow-replaces the layout'sopenGraphwith the page'sopenGraphentirely. The parent images appear only once — inside the page's returned array. No concatenation happens.
Correction to Previous Review
The previous review flagged "layoutParentPromises array is populated but never read (dead code)". This is incorrect — layoutParentPromises only appears in comments (lines 870-871) describing the conceptual model. The actual variable is layoutMetaPromises, which IS used at line 920 (Promise.all(layoutMetaPromises)). No dead code.
searchParams Reorder
The spObj computation was moved before the Promise.all. This is safe — it's a synchronous operation on the already-constructed URLSearchParams, with no dependency on async results. The reorder was necessary to pass spObj to resolveModuleMetadata(route.page, params, spObj, pageParentPromise) inside the same Promise.all.
Additionally, pageProps.searchParams = makeThenableParams(spObj) is now set unconditionally when searchParams is truthy (even if empty). The comment explains why: pages that do await searchParams need a thenable, not undefined. Previously searchParams was only set if the query string was non-empty, which would cause await searchParams to throw on URLs with no query params. Good fix.
Dev/Prod Parity
Confirmed: prod-server.ts and worker-entry.ts contain zero references to resolveModuleMetadata, generateMetadata, or mergeMetadata. App Router production delegates to the RSC entry built from app-dev-server.ts. No parity gap.
Template Literal Safety
Confirmed: the generated JS string contains only plain JS. The parent parameter in the generated code is a runtime value (parentForThisLayout, pageParentPromise) — no TypeScript type annotations leak into the template literal. The Promise<Metadata> type annotation exists only in the outer .ts file (resolveModuleMetadata signature in metadata.tsx), not in the emitted virtual module.
One Real Issue (Pre-existing, Not Blocking)
renderHTTPAccessFallbackPage passes {} for params to metadata resolution (line 530) but uses actual matched params for layout component props (line 567). If a layout's generateMetadata accesses dynamic route params, it'll get undefined values. The .catch() prevents crashes, and error/not-found pages have noindex so SEO impact is nil, but for correctness the metadata resolution should use opts?.matchedParams ?? route?.params ?? {} instead of {}. Same pre-existing issue as the first review noted — fine as a follow-up.
Tests
Three well-targeted regression tests. The metadata-generate-searchparams fixture is a nice bonus — it validates the searchParams forwarding fix that came along with this PR.
Verdict
LGTM. The implementation is correct, the architecture cleanly separates the parent chain from the final merge, and no TypeScript leaks into generated code. The previous review's dead-code nit was a false positive.
|
Review posted on PR #376. Here's a summary: LGTM. The implementation correctly matches Next.js's eager-execution-with-serial-resolution pattern. Key findings from the re-review:
|
…k render
renderHTTPAccessFallbackPage was always passing {} as params to
resolveModuleMetadata(), so layouts with generateMetadata({ params })
would get undefined for all dynamic route segments (e.g. params.slug)
when rendering not-found/forbidden/unauthorized fallback pages.
Fix: use opts?.matchedParams ?? route?.params ?? {} (same expression
already used for the layout component props just below) so that
generateMetadata() in fallback layouts receives the real matched params.
Adds two regression tests to not-found.test.ts: hit an invalid slug that
triggers notFound() and assert the title contains the actual slug value.
Also updates entry-templates snapshots.
|
/bigbonk re-review please |
There was a problem hiding this comment.
Re-review: fix: pass parent promise to generateMetadata() for ancestor metadata access
Read through the full diff and both call sites in detail. All tests pass (38/38 metadata, 26/26 not-found). Here's what I found:
Core implementation: Correct
The parent promise chain in both buildPageElement and renderHTTPAccessFallbackPage is correctly structured:
accumulatedMetaPromisestarts asPromise.resolve({})(root has no parent).- Each layout's
resolveModuleMetadata()is kicked off immediately with the currentaccumulatedMetaPromiseas itsparent— all calls are concurrent for I/O. - The accumulator advances to a new promise that merges the previous accumulated result with the current layout's result — so
await parentinside layout N gives merged metadata from layouts 0..N-1. - The page gets
pageParentPromise = accumulatedMetaPromise(all layouts merged).
This matches Next.js's eager-execution-with-serial-resolution pattern.
resolveModuleMetadata() signature change: Safe
The parent parameter defaults to Promise.resolve({}), so all existing call sites that don't pass it (there are none in the codebase, but in general) continue to work. The params default changing from required to = {} is also safe — the only call site that previously relied on implicit undefined was renderHTTPAccessFallbackPage, which now passes _fallbackParams explicitly.
No double-merge issue
Confirmed: the accumulator chain produces resolved/merged metadata for the parent parameter, while layoutMetaResults (the raw per-segment results) are what feed into the final mergeMetadata([...layoutMetaResults, pageMeta]). These are independent data paths — no double counting of title templates or OG images.
searchParams reorder: Correct
The spObj computation was moved above Promise.all so it can be passed to resolveModuleMetadata(route.page, params, spObj, pageParentPromise). This is safe — it's synchronous iteration over URLSearchParams. The pageProps.searchParams = makeThenableParams(spObj) assignment now happens unconditionally when searchParams is truthy (fixing pages that do await searchParams on URLs with no query string).
renderHTTPAccessFallbackPage params: Fixed pre-existing issue
The diff shows _fallbackParams is now opts?.matchedParams ?? route?.params ?? {}, and all callers pass matchedParams: params. This fixes the pre-existing issue where metadata resolution got {} instead of actual route params. Good — the not-found tests at layout-params-notfound/bad-slug confirm this works correctly.
One remaining gap: resolveModuleViewport in renderHTTPAccessFallbackPage
Line 540:
Promise.all(_filteredLayouts.map((mod) => resolveModuleViewport(mod).catch(...)))This still doesn't pass _fallbackParams to resolveModuleViewport(). While resolveModuleViewport defaults params to undefined (which won't crash since makeThenableParams(undefined) would fail), checking the signature shows it defaults to Record<string, string | string[]> — but resolveModuleViewport(mod) passes nothing, so params is undefined. If a layout has generateViewport({ params }) that accesses params.slug, it would get undefined.
This is pre-existing and not introduced by this PR, so not blocking. But since you're already passing _fallbackParams to resolveModuleMetadata in the same function, it'd be a one-line fix to also pass it to resolveModuleViewport:
Promise.all(_filteredLayouts.map((mod) => resolveModuleViewport(mod, _fallbackParams).catch(...)))Template literal safety: Confirmed
No TypeScript type annotations in the generated JS string. The parent parameter flows as a plain runtime JS value (parentForThisLayout, pageParentPromise). The Promise<Metadata> annotation exists only in metadata.tsx, not in the emitted virtual module.
Test coverage: Good
- Three parent-metadata tests cover the core regression (OG image inheritance, title rendering,
await parentnot throwing). - Two searchParams tests validate the forwarding fix.
- Two layout-params-notfound tests validate the params-to-metadata fix in
renderHTTPAccessFallbackPage. - Entry template snapshot updated.
Verdict
LGTM. The implementation is correct and well-documented. The resolveModuleViewport params gap in renderHTTPAccessFallbackPage (line 540) is pre-existing and low-impact — fine as a follow-up.
|
Review posted on PR #376. Summary: LGTM. The parent promise chain implementation is correct — all
|
…ackPage
generateViewport({ params }) in a layout wrapping a not-found/error
boundary now receives the actual route params instead of an empty object.
Mirrors the same fix already applied to resolveModuleMetadata.
Adds generateViewport export to the layout-params-notfound fixture and
two regression tests confirming the themeColor includes the real slug.
|
/bigbonk re-review |
There was a problem hiding this comment.
Re-review: fix: pass parent promise to generateMetadata() for ancestor metadata access
Read through the full diff in detail. All three previous reviews were thorough and correct. Here's what I found on this pass:
Core logic: Correct
The parent promise chain in both buildPageElement (lines 888-902) and renderHTTPAccessFallbackPage (lines 528-537) is sound:
accumulatedMetaPromisestarts asPromise.resolve({})— root layout has no parent.- Each layout's
resolveModuleMetadata()is called immediately with the current accumulator asparent— all calls are concurrent. - The accumulator advances via
.then()to merge the previous accumulated result with the current layout's result —await parentinside layout N gives merged metadata from layouts 0..N-1. - The page gets
pageParentPromise = accumulatedMetaPromise(all layouts merged).
Edge cases I traced:
- 0 layouts:
layoutMetaPromisesis empty,accumulatedMetaPromisestaysPromise.resolve({}),pageParentPromiseresolves to{}. Correct. - 1 layout: Single layout gets
parent = Promise.resolve({}), page gets that layout's merged result. Correct. - Layout
generateMetadata()throws:.catch()returnsnull, accumulator falls through toawait parentForThisLayout(previous accumulated value). The error doesn't propagate — subsequent layouts and the page still get valid parent metadata. Correct. - Page
generateMetadata()throws: No.catch()on the page's call (line 923). The error propagates out ofbuildPageElement()where it's caught by therenderErrorBoundaryPagehandler (lines 2026-2029). Correct — matches the PR description's intent that page metadata errors are NOT swallowed.
resolveModuleMetadata() signature: Safe
The parent parameter defaults to Promise.resolve({}) (line 286). The params default changed from implicit undefined to = {} (line 284). Both are backward-compatible — no external callers exist outside this codebase, and the internal call sites all pass explicit values now.
No double-merge issue
Confirmed: layoutMetaResults (raw per-segment results, line 921) feeds into the final mergeMetadata([...layoutMetaResults.filter(Boolean), ...pageMeta]) (line 927). The accumulator chain feeds only the parent promises. These are independent data paths — title templates and OG images won't be double-counted.
searchParams fix: Correct
spObj computation moved before Promise.all (lines 907-918) — this is purely synchronous iteration over URLSearchParams, no dependency on async results. The unconditional pageProps.searchParams = makeThenableParams(spObj) when searchParams is truthy (line 942) fixes the latent bug where pages doing await searchParams on no-query URLs would get undefined. The markDynamicUsage() call remains gated on hasSearchParams (line 951) — correct, since empty query strings shouldn't trigger dynamic.
renderHTTPAccessFallbackPage params fix: Good
_fallbackParams is now opts?.matchedParams ?? route?.params ?? {} (line 526), and callers pass matchedParams: params (e.g., line 2018). This fixes the pre-existing issue from previous reviews. The resolveModuleViewport call on line 540 now also receives _fallbackParams — previous reviews noted this was missing, and it's now fixed. Well done.
Template literal safety: Confirmed
Scanned the generated code template (lines 218-2425). No TypeScript type annotations appear in the template literal. All type-annotated code lives in the outer .ts files (metadata.tsx, app-dev-server.ts), not in the emitted virtual module string.
Test fixtures: Clean
metadata-parent-generate/layout.tsx— returns base OG image viagenerateMetadata(). Simple and targeted.metadata-parent-generate/page.tsx— reads(await parent).openGraph?.imagesand prepends. Directly exercises the bug.metadata-generate-searchparams/page.tsx— reads(await searchParams).qingenerateMetadata. Good coverage for the searchParams forwarding fix.layout-params-notfound/[slug]/layout.tsx— tests bothgenerateMetadataandgenerateViewportreceiving actual params in fallback render.
Test coverage: Solid
The three parent-metadata tests (lines 391-416) cover:
- OG image inheritance from parent layout
- Page title rendering when using parent
await parentnot throwing (200 status check)
Two searchParams tests (lines 423-437) validate both with and without query strings. Two layout-params-notfound tests (lines 206-227) validate params forwarding in fallback render. Two generateViewport tests (lines 237-258) validate viewport params forwarding.
One minor observation (not blocking)
In buildPageElement, the page's resolveModuleMetadata call (line 923) is inside Promise.all alongside layoutMetaPromises. This means the page's metadata resolution starts concurrently with layout metadata. If the page's generateMetadata calls await parent early, it'll block until all layout metadata is resolved — which is correct. But if a layout's generateMetadata is slow AND the page doesn't use await parent, the page metadata resolves independently. This matches Next.js behavior (concurrent kickoff, serial resolution only when parent is actually awaited).
Verdict
LGTM. The implementation is correct, edge cases are handled properly, no double-merge issues, template literal is clean JS, and test coverage is thorough. The searchParams and fallback params fixes are nice bonuses. Ship it.
|
Review posted on PR #376. Here's the summary: LGTM. After reading all 9 changed files in detail, the implementation is correct. Key findings:
|
1 participant
Summary
Fixes #375 —
generateMetadata()was never passed theparentparameter, causing any app that usesawait parentto throw a TypeError.What changed
shims/metadata.tsx—resolveModuleMetadata()now accepts aparent: Promise<Metadata>parameter (defaults toPromise.resolve({})) and passes it as the second argument tomod.generateMetadata().server/app-dev-server.ts— Both call sites (buildPageElementandrenderHTTPAccessFallbackPage) now build a serial parent promise chain matching Next.js's eager-execution-with-serial-resolution approach:generateMetadata()calls are kicked off concurrently (good for I/O-bound work).parentpromise only resolves after the preceding segment's metadata is merged, soawait parentgives the correct accumulated ancestor metadata.Tests
Three new regression tests in
tests/nextjs-compat/metadata.test.ts, backed by a new fixture attests/fixtures/app-basic/app/nextjs-compat/metadata-parent-generate/:should pass parent metadata to generateMetadata via parent parameter— OG images from parent layout are visible to childshould render page title from generateMetadata that uses parent— page title rendered correctly when using parentparent parameter should not be undefined (await parent must not throw)—await parentnever throwsRoot cause of the previous crash (all tests 500ing)
The initial implementation accidentally included TypeScript type annotations (
Promise<Metadata>[]) inside the template-literal string that generates the virtual RSC entry module. That virtual module is resolved as plain JavaScript (no.tsextension), so the generic syntax caused a parse error, crashing every page request. The fix was to remove the type annotations from the generated code (they're only needed in the outer.tsfile, not in the emitted JS string).