Repository navigation
fix(app-router): render templates with children only - #2322
Conversation
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk review
Verdict: LGTM. The change is correct and well-targeted.
Root cause & fix are correct
Verified against Next.js v16.2.6 source. create-component-tree.tsx constructs the template node as:
const templateNode = createElement(
Template,
null, // <-- no props, no params
createElement(RenderFromTemplateContext, null)
)The PR's change from <TemplateComponent params={options.matchedParams}> to <TemplateComponent> exactly matches Next.js's children-only template contract. This is the right fix for the reported bug: client template.tsx modules failing because matchedParams is a thenable/non-plain object that React Flight rejects.
Verification performed
options.matchedParamsis still used in 8 other locations (layouts, slots, route state key, etc.) — no dead code introduced; change is correctly scoped to templates only.app-page-route-wiring.tsx:680-703is the single construction site for templates. The App Router prod server delegates to this shared wiring, so there's no dev/prod parity gap to chase elsewhere.- Ran
vitest run tests/app-page-route-wiring.test.tslocally — 38 passed. The newexpect(lastGroupTemplateProps).not.toHaveProperty("params")is a solid regression guard, andchildrenis still threaded through (readChildrencontinues to work).
Minor / non-blocking observations
- Client spec lines 21-24 (
template-navigation.spec.ts): line 21 increments then line 22 immediately clicks the link without asserting the intermediate count. The intent (prove count resets to 0 after navigating back via a remounted template) holds because line 24 asserts the freshly-mounted template starts at 0. Not a bug, but adding an intermediatetoHaveText("Template 1")assertion before navigating would make the test's intent more legible and rule out a no-op increment masking a regression. - Shared testid
client-template-linkacross bothpage.tsxandother/page.tsxis fine since only one page is mounted at a time, but worth noting for future maintainers.
Both are optional polish; nothing blocks merge.
|
Review posted to PR #2322. SummaryVerdict: LGTM — the change is correct and well-scoped. What I verified:
Non-blocking observations (posted in the review):
Nothing blocks merge. |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review — independent verification
Verdict: LGTM. Verified the fix against Next.js v16.2.6 source and ran the tests locally.
Fix is correct
Confirmed against packages/next/src/server/app-render/create-component-tree.tsx at tag v16.2.6. Next.js builds the template node as:
const templateNode = createElement(
Template,
null, // no props, no params
createElement(RenderFromTemplateContext, null)
)The PR's change from <TemplateComponent params={options.matchedParams}> to <TemplateComponent> (app-page-route-wiring.tsx:689,695) exactly matches this children-only contract. The stated root cause is accurate: matchedParams is a thenable/non-plain object that React Flight rejects before a client template can mount.
Verification performed
- Single construction site. Lines 689 and 695 are the two branches of the same template loop and both were updated. The App Router prod server delegates to
buildAppPageElements, so there is no dev/prod parity gap. - No dead code.
options.matchedParamsis still consumed for layouts, slots, and state keys. - Unit tests:
vp test run tests/app-page-route-wiring.test.ts→ 38 passed. The newexpect(lastGroupTemplateProps).not.toHaveProperty("params")is a good regression guard, andchildrenis still threaded through. - E2E: ran
template-navigation.spec.ts(project app-router, 1 worker) → 2 passed. Verified the subtle server-template assertion: I initially suspected the parent-segment template would not remount onalpha→beta, but the template'stemplateTreePositionkeys it to the[section]boundary viasegmentResetKey, so the reset-to-0 assertion is correct.
Non-blocking observations
- Client spec lines 21-24 (
template-navigation.spec.ts): line 21 increments then line 22 immediately navigates without an intermediate count assertion. An addedtoHaveText("Template 1")before navigating would make the intent clearer and rule out a no-op increment masking a regression. - Shared
client-template-linktestid acrosspage.tsxandother/page.tsxis fine (one page mounted at a time) but worth a note for future maintainers.
Nothing blocks merge.
Co-authored-by: james-elicx <james-elicx@users.noreply.github.com>
|
Review posted to PR #2322. SummaryVerdict: LGTM — the change is correct and well-scoped. What I verified:
Notable: I initially suspected the server-template parity assertion (count resets Non-blocking observations (posted in review): intermediate count assertion in the client spec, and the shared |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review for issues
Verdict: LGTM. No blocking issues. The fix is correct and matches Next.js v16.2.6 exactly.
Fix is correct
Verified against packages/next/src/server/app-render/create-component-tree.tsx at tag v16.2.6. Next.js builds the template node with no props:
const templateNode = createElement(
Template,
null, // no props, no params
createElement(RenderFromTemplateContext, null)
)Dropping params={options.matchedParams} from both branches of TemplateComponent (app-page-route-wiring.tsx:689,695) is the correct children-only contract. The stated root cause holds: matchedParams is a thenable produced by makeThenableParams, a non-plain object React Flight rejects before a client template.tsx can mount.
Verification performed
- No dead code.
options.matchedParamsis still consumed in 7 other spots —resolveAppPageRouteStateKey(route reset key, L545), slot param override (L638), layout params (L714), slot params (L771), and segment reset keys (L1010, L1084). The change is scoped to templates only. - Single construction site / no dev-prod gap. Lines 687–698 are the only place templates are built; the App Router prod server delegates to
buildAppPageElements, so there is no Pages-Router-style parity gap to chase inprod-server.ts. - Unit test passes locally.
vitest run tests/app-page-route-wiring.test.ts→ 38 passed. The newexpect(lastGroupTemplateProps).not.toHaveProperty("params")is a solid regression guard, andchildrenis still threaded through (readChildrenkeeps working). - E2E green in CI. All three
app-routershards pass, which exercise the newtemplate-navigation.spec.ts.
On the subtle server-template assertion (L42-44)
I dug into whether alpha→beta should reset the server template's client identity to 0, since the template.tsx lives at the static template-server/ segment, not at [section]. vinext's segmentResetKey for that template resolves to a constant (resolveAppPageSegmentStateKey returns the first non-group segment from the template's own treePosition, i.e. "template-server"), so the <Slot key=...> does not change on a param-only navigation. The reset to 0 therefore comes from the full subtree re-render on param navigation, not from the key — and CI confirms it matches Next.js empirically. Worth a one-line comment in the spec noting the reset is driven by the subtree rebuild, not the template key, so a future refactor of segmentResetKey doesn't silently regress this guarantee. Non-blocking.
Minor / non-blocking
- Unused fixture link.
template-server/page.tsxdefinesdata-testid="server-template-link"that the spec never clicks (the test starts directly at/template-server/alpha). Harmless, but it's dead test scaffolding. - Client spec L21-24. Line 21 increments then L22 navigates without asserting the intermediate count — adding
toHaveText("Template 1")before navigating back would make the reset-on-remount intent clearer and rule out a no-op increment masking a regression. - Shared
client-template-linktestid acrosspage.tsxandother/page.tsxis fine (one page mounted at a time) but worth a note for future maintainers.
Nothing blocks merge.
Co-authored-by: james-elicx <james-elicx@users.noreply.github.com>
|
Review posted to PR #2322. SummaryVerdict: LGTM — the change is correct and well-scoped. No blocking issues. What I verified:
Notable finding I investigated: the server-template parity assertion ( Non-blocking observations posted: unused |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review for issues
Verdict: LGTM. The core fix is correct and matches Next.js v16.2.6 exactly. One genuine latent-correctness finding on the new server-template test that's worth recording, plus minor test-scaffolding cleanup. Nothing blocks merge.
Core fix is correct
Verified against packages/next/src/server/app-render/create-component-tree.tsx at v16.2.6. Next.js builds the template node with no props:
const templateNode = createElement(Template, null, createElement(RenderFromTemplateContext, null))Dropping params={options.matchedParams} from both branches of TemplateComponent (app-page-route-wiring.tsx:689,695) is the correct children-only contract. Root cause is accurate — matchedParams is a makeThenableParams thenable React Flight rejects for client templates.
- Single construction site / no dev-prod gap.
buildAppPageElements(L680–703) is the only place templates are built; I grepped the wholepackages/vinext/srctree — there is no secondTemplateComponent/template-with-params construction inprod-server.tsorapp-rsc-entry.ts. Prod delegates to this shared wiring. - No dead code.
options.matchedParamsstill feeds layouts (L714), slots (L771), and segment/route reset keys. - Unit tests pass locally —
vitest run tests/app-page-route-wiring.test.ts→ 38 passed.not.toHaveProperty("params")is a solid regression guard.
Finding: the server-template assertion is right per the docs, but vinext's key mechanism is not what makes it pass (latent risk)
The new test asserts that navigating template-server/alpha → template-server/beta resets server-template-identity to 0 (template-navigation.spec.ts:42-44). I checked this against the Next.js template.js docs:
Templates receive a unique key for their own segment level. They remount when that segment (including its dynamic params) changes. Navigations within deeper segments do not remount higher-level templates.
And the concrete blog example: app/blog/template.tsx remounts on /blog/first-post → /blog/second-post because its key goes from /blog/first-post to /blog/second-post — i.e. the template's key includes the changing child dynamic segment. So the assertion that the template-server template remounts on alpha→beta is correct Next.js behavior, and the test is asserting the right thing. Good.
But here's the catch: vinext's segmentResetKey for that template comes from resolveAppPageSegmentStateKey(routeSegments, treePosition, params) (app-page-segment-state.ts:117-128), which returns the first non-group segment at the template's tree position — i.e. the constant "template-server". It does not include the child [section] param. So vinext's <Slot key={segmentResetKey}> (app-page-route-wiring.tsx:1068) does not change on alpha→beta. The reset-to-0 the test observes comes from the full subtree re-render on param navigation, not from the template key changing as Next.js's blog example shows.
That means this green test is a coincidental pass over a real keying divergence: per the docs a parent template's key should track its changing child segment, but vinext keys it to a constant. A future refactor that relies on the Slot key for remount (instead of subtree rebuild) would silently regress this guarantee, and the test wouldn't catch the mechanism breaking. This keying gap is pre-existing (not introduced by this PR, which is scoped to the children-only contract and defers soft-nav/persistence to #2316), so it does not block. Recommend either (a) a one-line comment in the spec noting the reset is driven by subtree rebuild, not the template key, or (b) filing a follow-up issue to align resolveAppPageSegmentStateKey for templates with Next.js's child-segment-inclusive keying.
Minor / non-blocking
- Dead test scaffolding.
template-server/page.tsxand itsdata-testid="server-template-link"are never referenced — the spec navigates directly to/template-server/alpha. Confirmed via grep:server-template-linkhas exactly one occurrence (the fixture) and zero in tests. Either drop the fixture page or add a test that starts at/template-serverand clicks through it. - Client spec L21-24. L21 increments then L22 navigates without asserting the intermediate count. An added
toHaveText("Template 1")before navigating would rule out a no-op increment masking a regression. - Shared
client-template-linktestid acrosspage.tsxandother/page.tsxis fine (one page mounted at a time) but worth a note for maintainers.
Nothing blocks merge.
|
Review posted to PR #2322. SummaryVerdict: LGTM — the core fix is correct and well-scoped. Nothing blocks merge. What I verified:
The one substantive finding (which I dug deeper on than prior reviews): the server-template assertion ( Minor/non-blocking: dead |
Summary
paramsto App Router templates, matching Next.js's children-only template contracttemplate.tsxmodules to serialize and mount instead of failing on vinext's thenable params objectRoot cause
vinext rendered every template as
<TemplateComponent params={options.matchedParams}>. For client templates,matchedParamsis a thenable/non-plain object and React Flight rejects it before the template can mount. Next.js constructs templates with only the template context child.This track intentionally avoids the layout persistence and soft back/forward work already covered by #2316.
Next.js reference
test/e2e/app-dir/app/index.test.tsat tagv16.2.6packages/next/src/server/app-render/create-component-tree.tsxat tagv16.2.6Validation
vp test run tests/app-page-route-wiring.test.ts— 38 passedtemplate-navigation.spec.tswith one worker — 2 passedlayout-persistence.spec.ts --grep "Template remount"with one worker — 3 passedvp check— cleanvp run vinext#build— passed