fix(app-router): pass searchParams to layout generateMetadata#793
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
Fixes App Router metadata resolution so searchParams from the URL query string are passed consistently to layout generateMetadata() (previously layouts always received undefined, while pages received the real params), and adds a regression fixture + test to prevent regressions.
Changes:
- Hoist
URLSearchParams→ plain object conversion so layout metadata resolution receives the samesearchParamsobject as the page. - Add a new
layout-metadata-searchfixture whose layoutgenerateMetadata()readssearchParams.tab. - Add an integration test asserting the layout-derived
<title>reflects the query string value.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/vinext/src/entries/app-rsc-entry.ts |
Hoists searchParams object construction before layout metadata resolution and passes it into resolveModuleMetadata() for layouts. |
tests/fixtures/app-basic/app/layout-metadata-search/layout.tsx |
New regression fixture: layout generateMetadata() reads searchParams.tab and sets the title accordingly. |
tests/fixtures/app-basic/app/layout-metadata-search/page.tsx |
New fixture page to complete the route under the test layout. |
tests/app-router.test.ts |
New regression test asserting layout metadata uses query string search params. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const spObj = {}; | ||
| let hasSearchParams = false; | ||
| if (searchParams && searchParams.forEach) { | ||
| searchParams.forEach(function(v, k) { | ||
| hasSearchParams = true; | ||
| if (k in spObj) { |
There was a problem hiding this comment.
When converting URLSearchParams into spObj, using a normal {} plus if (k in spObj) allows inherited/prototype keys (e.g. __proto__, constructor, toString) to be treated as existing properties. This can lead to surprising behavior and can open up prototype-related issues when passing spObj into userland generateMetadata().
Consider creating spObj with Object.create(null) and using Object.prototype.hasOwnProperty.call(spObj, k) (or checking spObj[k] !== undefined) instead of the in operator when handling duplicates.
| const spObj = {}; | |
| let hasSearchParams = false; | |
| if (searchParams && searchParams.forEach) { | |
| searchParams.forEach(function(v, k) { | |
| hasSearchParams = true; | |
| if (k in spObj) { | |
| const spObj = Object.create(null) as Record<string, string | string[]>; | |
| let hasSearchParams = false; | |
| if (searchParams && searchParams.forEach) { | |
| searchParams.forEach(function(v, k) { | |
| hasSearchParams = true; | |
| if (Object.prototype.hasOwnProperty.call(spObj, k)) { |
Layout `generateMetadata()` was always called with `undefined` for `searchParams` because `spObj` (the URLSearchParams-to-plain-object conversion) was constructed after the layout metadata loop. Hoist the `spObj` construction to before the loop so every layout receives the real query parameters — matching the behaviour already correct for the page segment. Adds a regression fixture (`layout-metadata-search`) and a test that requests `/layout-metadata-search?tab=settings` and asserts the layout's `generateMetadata` title reflects the query param value rather than the undefined-fallback default.
2065169 to
477032f
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review Summary
The bug identification is correct — spObj was constructed after the layout metadata loop, so layouts always received undefined for searchParams. The fix (hoisting spObj construction) is the right mechanical change, and the test + fixture are well-structured.
However, the fix introduces a Next.js behavioral divergence: in Next.js, layouts do not receive searchParams in generateMetadata() — only pages do. The original code was accidentally correct for the wrong reason (passing undefined). This PR makes it intentionally wrong by passing real search params to layouts.
Next.js behavior (authoritative source)
In packages/next/src/lib/metadata/resolve-metadata.ts, the props construction is:
const props: SegmentProps = isPage ? { params, searchParams } : { params }Layouts get { params } only. Pages get { params, searchParams }. This applies to both generateMetadata() and generateViewport().
See also the Next.js docs for generateMetadata:
searchParams- An object containing the current URL's search parameters. Only available in pages.
What should happen
- The
spObjhoisting is still correct and necessary — it should be constructed before the layout loop so it's available for both the layout and page metadata calls. - But the layout metadata call should pass
undefined(notspObj) to match Next.js semantics. - Only the page's
resolveModuleMetadatacall (line 1000) should receivespObj. - The test fixture should be updated to test the page's
generateMetadatareceivingsearchParams, or to verify that the layout does not receive them (matching Next.js).
The spObj hoisting is still useful for the page call and for pageProps.searchParams, so the structural change is fine — just don't pass it to layout metadata resolution.
| const parentForThisLayout = accumulatedMetaPromise; | ||
| // Kick off this layout's metadata resolution now (concurrent with others). | ||
| const metaPromise = resolveModuleMetadata(layoutMods[i], params, undefined, parentForThisLayout) | ||
| const metaPromise = resolveModuleMetadata(layoutMods[i], params, spObj, parentForThisLayout) |
There was a problem hiding this comment.
Next.js parity issue: In Next.js, generateMetadata() for layouts does not receive searchParams — only pages do. The props construction in Next.js's resolve-metadata.ts is:
const props = isPage ? { params, searchParams } : { params }Passing spObj here diverges from Next.js behavior. The original undefined was accidentally correct (though for the wrong reason — it should have been an intentional choice, not a sequencing bug).
| const metaPromise = resolveModuleMetadata(layoutMods[i], params, spObj, parentForThisLayout) | |
| const metaPromise = resolveModuleMetadata(layoutMods[i], params, undefined, parentForThisLayout) |
The spObj hoisting is still the right structural change — it needs to be available for the page metadata call on line 1000 and for pageProps.searchParams below. Just don't pass it to layouts.
| * for searchParams — only page generateMetadata() received the real value. | ||
| */ | ||
|
|
||
| export async function generateMetadata({ |
There was a problem hiding this comment.
Since Next.js does not pass searchParams to layout generateMetadata(), this fixture tests behavior that diverges from Next.js.
Consider either:
- Converting this into a page-level
generateMetadatatest (testing that the page receivessearchParamscorrectly), or - Keeping this as a layout fixture but asserting the opposite — that the layout's metadata falls back to
"home"becausesearchParamsis not provided to layouts (matching Next.js behavior).
Option 2 would be a useful parity test to prevent future accidental passing of searchParams to layouts.
tests/app-router.test.ts
Outdated
| // The layout's generateMetadata reads searchParams.tab — should produce | ||
| // "Layout Section: settings", not "Layout Section: home" (the fallback for | ||
| // undefined searchParams). | ||
| expect(html).toContain("<title>Layout Section: settings</title>"); |
There was a problem hiding this comment.
This assertion expects searchParams to be available in the layout's generateMetadata, which contradicts Next.js behavior. In Next.js, this would produce <title>Layout Section: home</title> (the fallback) because layouts don't receive searchParams.
If the goal is Next.js parity, this should assert the fallback:
| expect(html).toContain("<title>Layout Section: settings</title>"); | |
| expect(html).toContain("<title>Layout Section: home</title>"); |
Alternatively, if you want to test that searchParams works for page metadata, add a generateMetadata to the page fixture instead and assert on that.
|
Review submitted. Here's a summary of the findings: The core bug identification is correct — However, this PR introduces a Next.js behavioral divergence. In Next.js, The review requests changes:
|
In Next.js, layout generateMetadata() does NOT receive searchParams. Only page generateMetadata() does (resolve-metadata.ts:777). The spObj hoisting from the first commit is still the right structural change (needed for the page metadata call and pageProps.searchParams), but layouts should receive undefined, not spObj. Also uses Object.create(null) for spObj to prevent prototype pollution when userland generateMetadata() accesses keys like __proto__. Converts the test to a parity assertion: the layout falls back to "home" because it never receives searchParams.
|
/bigbonk review |
There was a problem hiding this comment.
The PR looks good. The prior review feedback from ask-bonk has been fully addressed:
- Next.js parity is correct — layout
resolveModuleMetadata()passesundefinedforsearchParams(line 987), matching Next.js'sisPage ? { params, searchParams } : { params }pattern. Only the page call (line 1001) receivesspObj. Object.create(null)is the right call — this addresses the Copilot prototype pollution concern. With a null-prototype object,k in spObjis safe since there are no inherited properties.- The
spObjhoisting is structurally correct — it needed to move before the layout loop anyway becausepageProps.searchParams(line 1018) references it, and the page metadata call (line 1001) needs it. - Test and fixture are well-structured — the test correctly asserts the fallback title
"Layout Section: home", confirming layouts do NOT receivesearchParams. - Consistency with
app-page-boundary-render.ts— the extracted runtime helper atserver/app-page-boundary-render.ts:124already passesundefinedfor layouts, so both code paths are aligned.
One minor nit on the fixture page copy (non-blocking).
| return ( | ||
| <main data-testid="layout-metadata-search-page"> | ||
| <h1>Layout Metadata Search Test</h1> | ||
| <p>This page tests that layout generateMetadata receives searchParams.</p> |
There was a problem hiding this comment.
Nit: This copy is stale after the parity fix — the test now verifies that layout generateMetadata does not receive searchParams.
| <p>This page tests that layout generateMetadata receives searchParams.</p> | |
| <p>This page tests that layout generateMetadata does NOT receive searchParams.</p> |
|
Review submitted. Here's a summary: Approved — the PR correctly addresses the original review feedback and is ready to merge. Key findings:
One non-blocking nit: the fixture page's |
Summary
generateMetadata()was always called withundefinedforsearchParamsbecause thespObj(URLSearchParams → plain object conversion) was constructed after the layout metadata loop — only the page segment received the real query parameters.spObjconstruction to before the layout loop so all layouts receive the same search params object already correctly used by the page.tests/fixtures/app-basic/app/layout-metadata-search/) and a test asserting thatlayout generateMetadata()title reflects the URL query parameter value, not the undefined-fallback default.Root Cause
In
packages/vinext/src/entries/app-rsc-entry.ts, the layout loop called:while the page call (a few lines later) correctly used
spObj. ThespObjvariable simply wasn't constructed yet when the loop ran.Test Plan
layout generateMetadata() receives searchParams from URL query string— verifies?tab=settingsproduces<title>Layout Section: settings</title>from the layout'sgenerateMetadatavp test run tests/app-router.test.ts -t "metadata|generateMetadata|viewport"— 26 tests green)vp checkpasses (format, lint, types)