fix(metadata): apply ancestor templates to title defaults#1256
fix(metadata): apply ancestor templates to title defaults#1256NathanDrake2406 wants to merge 1 commit into
Conversation
commit: |
Metadata title defaults currently render without the active ancestor template. That diverges from Next.js when a child layout or page provides title.default under a parent layout title.template. The merge path resolved only the final title after collecting templates, so object defaults skipped the stashed template used for that segment. Resolve each title as it is encountered against the current ancestor template, then stash the current layout template for descendants. Covers the child-layout default regression and updates page-default expectations to match Next.js resolveTitle semantics.
8c7ee75 to
1809012
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(metadata): apply ancestor templates to title defaults
This is a clean, well-scoped fix. The core insight is correct: the old code deferred all title resolution to a single post-loop pass, which meant title.default in a child layout or page never got wrapped by the ancestor template that was active before that segment. Moving to per-segment resolution inside the loop (resolve first, then stash the current segment's template for descendants) matches the Next.js resolveTitle() semantics documented in resolve-title.ts.
I traced through all the key scenarios:
| Scenario | Result | Correct? |
|---|---|---|
Root { template: "%s | Site", default: "Site" } + child { default: "Blog" } |
"Blog | Site" |
Yes |
Root template + page { default: "Page", template: "..." } |
"Page | Root" (page template ignored for own default) |
Yes |
Root template + page string title "About" |
"About | Site" |
Yes |
title.absolute skips templates |
"Custom Title" |
Yes |
Template-only layout { template: "..." } (no default) |
undefined (clears title, consistent with Next.js) |
Yes |
Single page { template: "%s | Page", default: "Page" } |
"Page" (no ancestor template to apply) |
Yes |
The twitter.images guard is a narrow, correct cleanup — prevents materializing an own twitter.images: undefined property when OG has no images.
Two small suggestions for test coverage below, neither blocking.
| } | ||
|
|
||
| function applyTitleTemplate(template: string | undefined, title: string): string { | ||
| return template ? template.replace(/%s/g, title) : title; |
There was a problem hiding this comment.
Nit: this uses /%s/g (global replace), so a template like "%s - %s" would produce "Blog - Blog". The old code used a single .replace("%s", ...). Next.js's resolveTitleTemplate also does a single replacement:
// Next.js: resolve-title.ts
return template.replace(/%s/g, title)Actually, checking again — Next.js does use /%s/g too (source). So this matches. Never mind, carry on.
| return template ? template.replace(/%s/g, title) : title; | ||
| } | ||
|
|
||
| function resolveTitle(title: Metadata["title"], stashedTemplate: string | undefined) { |
There was a problem hiding this comment.
The return type here is implicitly string | undefined. Consider annotating it explicitly as resolveTitle(...): string | undefined for clarity, since it feeds directly into merged.title which accepts both strings and title objects but the resolved value should only ever be a string or undefined at this point.
| }, | ||
| ]); | ||
|
|
||
| expect(result.title).toBe("Blog | Site"); |
There was a problem hiding this comment.
Good test. Consider adding a companion test for the template-only (no default) edge case to lock in the intentional title-clearing behavior:
| expect(result.title).toBe("Blog | Site"); | |
| expect(result.title).toBe("Blog | Site"); | |
| }); | |
| it("template-only layout (no default) clears previously set title", () => { | |
| const result = mergeMetadataEntries([ | |
| { | |
| metadata: { title: { template: "%s | Site", default: "Site" } }, | |
| }, | |
| { | |
| metadata: { title: { template: "%s | Blog" } }, | |
| }, | |
| ]); | |
| expect(result.title).toBeUndefined(); |
| @@ -2150,16 +2165,19 @@ describe("metadata title templates", () => { | |||
| expect(result.title).toBe("Hello World - Blog"); | |||
There was a problem hiding this comment.
Nice — this correctly validates that a 3-layer scenario (root template → middle template → page string) applies the nearest template. The fact that the middle layout's default ("Blog") now also gets the root template applied is the core fix.
|
Review submitted. Here's a summary: Approved the PR. The fix correctly moves title resolution from a deferred post-loop pass to per-segment resolution inside the loop, matching Next.js's I traced through all critical scenarios (string titles, object defaults, absolute titles, template-only layouts, single-page templates, nested templates) and confirmed they produce the correct output consistent with Next.js behavior. Two non-blocking suggestions:
|
Overview
title.defaultis provided under an ancestortitle.template.packages/vinext/src/shims/metadata.tsx,tests/features.test.ts,tests/app-page-head.test.tstitle.defaultvalues now render with the same ancestor template semantics as Next.js.Why
Metadata title resolution has a segment-local invariant: a layout template applies to descendant titles, while the segment's own
title.defaultresolves against the ancestor template that was active before that segment. vinext collected layout templates first and resolved only the final title later, which meant object defaults skipped the stashed template for their own segment.What changed
plus child layout{ default: "Blog" }`Blogtitle.defaultunder ancestor templatetitle.templatetwitter.images: undefinedproperty in resolved metadatatwitter.imagesabsent unless there are images to inheritNotes
The
twitter.imagesguard is a narrow object-shape cleanup found while updating the app-head regression for the title fix. It avoids preserving an own undefined field when OpenGraph has no images; it is not intended to be a broader metadata object normalization pass.This PR also does not attempt the deeper resolved-title architecture change where vinext would carry
{ absolute, template }through the full metadata pipeline like Next.js. The scope here is the visible final-title compatibility bug.Validation
vp test run tests/features.test.ts -t "applies ancestor title template to child layout default title"first failed withReceived: "Blog".vp test run tests/app-page-head.test.ts -t "keeps primary page title handling independent from active parallel route metadata"vp test run tests/features.test.ts -t "metadata title templates"vp test run tests/nextjs-compat/metadata.test.tsvp checkReferences
resolve-title.tstitle.default.resolve-title.test.tstitle.defaultandtitle.templatesemantics.