Conversation
Upstream's metadata suite expects /title-template to use its page title without the template declared by the layout in that same segment. A nested page should use the nearest ancestor-segment template, not its colocated layout's template. Vinext applied the last layout template in both cases. Carry an explicit template-contribution flag on metadata merge entries and set it from the layout tree position versus the page's route segments. Layout metadata still contributes its own title/default and all other fields. Keep ordered-list callers without segment metadata compatible with their existing merge contract. Dev and production use the same app-page-head resolver. Add three route-aware regressions: same-segment layout, nested colocated layout, and a deeper descendant without its own layout. The first two failed before this change and all now pass. Validation: - Original v16.2.6 title-template, stashed-title, and parent-layout-title cases: 4 passed. The two baseline failures are now passing; the remaining metadata cases were not selected in this focused run. - Full app-page-head and features suites: 377 passed. - Targeted vp check and vinext package build pass.
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Next.js v16.2.6 expects the same-segment page title to remain Page. Update SSR and browser compatibility assertions, retaining descendant template coverage and linking the upstream test.
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| // page sharing that layout's segment. Keep absent source positions | ||
| // compatible with callers that supply only an ordered module list. | ||
| contributesTitleTemplate: | ||
| options.routeSegments === undefined || |
There was a problem hiding this comment.
[P2] Preserve callers when route-position data is absent
This fallback tests only routeSegments === undefined, although both routeSegments and layoutTreePositions are independently optional (and routeSegments explicitly permits null). For example, a caller with routeSegments: null and no positions previously received ordered-list template merging, but this now normalizes the route to [] and suppresses every layout template. Conversely, supplying segments without positions assigns every layout position 0, so same-segment templates are not scoped correctly. Please apply depth scoping only when both values (including the corresponding position) are present; otherwise retain the ordered merge behavior promised by the comment.
| : [], | ||
| ), | ||
| ...(pageMetadata ? [{ isPage: true, metadata: pageMetadata }] : []), | ||
| ...parallelMetadataResults |
There was a problem hiding this comment.
[P2] Apply the same segment scoping to parallel-route metadata
resolveParallelRouteMetadata() returns a flat layout/page result list, and these entries still have neither isPage nor contributesTitleTemplate set. If the primary page has no title (a supported path covered later in this test file), an @slot/layout title template will therefore wrap the title from a colocated @slot/page, reproducing the same bug on the active parallel branch. Preserve each parallel entry’s page/layout role and tree position so the template can be suppressed for the slot page’s own segment too.
|
Posted 2 actionable inline findings. |
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
| contributesTitleTemplate: | ||
| parallelRoute.routeSegments == null || | ||
| layoutTreePositions[index] === undefined || | ||
| layoutTreePositions[index] < routeSegments.length, |
There was a problem hiding this comment.
[P2] Compare parallel positions in slot-local coordinates
For an ordinary direct slot, slot.routeSegments is null, so resolveActiveParallelRouteHeadInputs() substitutes the primary route full segments, while the slot-root layout keeps its slot-local position 0. In the new fixture this makes the condition effectively 0 < 2, so the @slot/layout.tsx template is still enabled and the result is Slot Page | Slot, not the asserted Slot Page. The unit test avoids this by supplying synthetic routeSegments: ["dashboard", "@slot"] with position 2, which is not the shape produced for a direct slot. Please retain the slot-local page depth (or translate both values to the same coordinate space) before deciding whether the layout is colocated.
|
Posted 1 actionable inline finding. |
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
| contributesTitleTemplate: | ||
| parallelRoute.routeSegments == null || | ||
| layoutTreePositions[index] === undefined || | ||
| layoutTreePositions[index] < routeSegments.length, |
There was a problem hiding this comment.
[P2] Use target-local positions for intercepted layouts
This comparison is still wrong for interception branches. resolveActiveParallelRouteHeadInputs() assigns every intercept layout options.routeSegments.length (the source route depth), while routeSegments here is interceptSourcePageSegments (the target filesystem path, including the slot/interception segments). For example, a one-segment source opening an intercepted page with a layout colocated beside that page gets 1 < targetPath.length, so its template wraps the intercepted page title even though this PR is meant to suppress same-segment templates. interceptLayoutSegments already carries each layout’s target-branch depth; please preserve/translate that depth so both sides of this test use the same coordinate system, and add an intercepted colocated-layout regression.
|
Posted 1 actionable inline finding. |
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
| contributesTitleTemplate: | ||
| parallelRoute.routeSegments == null || | ||
| layoutTreePositions[index] === undefined || | ||
| layoutTreePositions[index] < routeSegments.length, |
There was a problem hiding this comment.
[P2] Supply positions for slot-less sibling interceptions
The sibling-intercept path does not pass through the corrected position calculation: primaryParallelRouteHeadInput in app-page-element-builder.ts builds layoutModules and routeSegments but omits layoutTreePositions. Every sibling-intercept layout therefore takes the layoutTreePositions[index] === undefined compatibility branch here, so a template from a layout colocated with the intercepted page still wraps that page title. The new interception regression exercises a named @modal branch only. Please derive/pass target-local positions for the sibling-intercept head as well and cover that path.
| // A layout template applies to descendant route segments, never the | ||
| // page sharing that layout's segment. Keep absent source positions | ||
| // compatible with callers that supply only an ordered module list. | ||
| contributesTitleTemplate: |
There was a problem hiding this comment.
[P2] Scope templates in HTTP-access fallback metadata too
This scoping is only applied by prepareAppPageHeadInner; 404/403/401 rendering goes through resolveHttpAccessFallbackMetadata -> resolveOrderedAppPageMetadata, which still converts every source to a plain { metadata } entry. Thus a /foo/layout.tsx title template still wraps the title exported by a colocated /foo/not-found.tsx, although Next.js excludes the leaf layout template for the error-convention leaf as well. Please carry layout/page role and position through the ordered fallback plan (while preserving the documented no-position fallback) and add a not-found regression.
There was a problem hiding this comment.
Verified against an isolated Next.js 16.2.7 app: with a colocated layout template and a not-found title, a real HTTP 404 renders <title>Fallback | Leaf</title>. vinext already matches that result. The error convention is an additional metadata leaf, so suppressing its layout template here would introduce a parity regression. I left this path unchanged and addressed the sibling-intercept finding separately in 5b4a72f.
|
Posted 2 actionable inline findings. |
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
|
LGTM! |
A layout's title template currently applies to the page in its own route segment. Restrict template inheritance to descendant segments so a page uses the nearest ancestor segment's template. Layout titles, defaults, and other metadata continue to contribute, and callers supplying only an ordered module list retain their existing merge behavior. Development and production share this resolver.
Adds three route-aware regressions covering a colocated layout, a nested colocated layout, and a deeper descendant without its own layout, based on the Next.js metadata suite.
The source commit records all 4 focused upstream title cases passing, 377 app-page-head and feature tests passing, and passing targeted checks and package build. Tests were not rerun in this worktree because its local
vite-plusdependency is not installed.