fix: rewritten pathnames and metadata title resolution

[JaredStowell]

This fixes two App Router parity gaps with Next.js and hardens the related coverage.

• Preserve the canonical requested pathname in navigation context after rewrites so usePathname() reflects the source URL, not the rewritten destination.
• Fix metadata title resolution so nested defaults, ancestor templates, and multi-placeholder templates match Next.js behavior.
• Preserve resolved parent title state for generateMetadata(_, parent), including title.absolute and title.template, instead of collapsing parent titles to a plain string.
• Add regression coverage for middleware rewrites, parent metadata title shape, title uniqueness, and multi-placeholder templates.
• Refresh generated App Router entry snapshots for the updated parent metadata merge path.

Testing

• pnpm test tests/features.test.ts -t "metadata title templates"
• pnpm test tests/nextjs-compat/hooks.test.ts tests/nextjs-compat/metadata.test.ts tests/entry-templates.test.ts
• pnpm run fmt
• pnpm run typecheck

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@486

commit: 271b0ba

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c58fc73188

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[james-elicx]

/bigbonk review

[ask-bonk]

Review

Good work fixing the pathname semantics for usePathname() after rewrites and improving the metadata title resolution. The approach of capturing navigationPathname before rewrites mutate cleanPathname is the right pattern, and the new resolveTitleState / mergeMetadataForParent split is cleaner than the old inline logic. Tests are thorough and well-structured.

However, there's one scoping bug that needs fixing before merge, plus a couple of minor observations.

Blocking issue

__cachePathname is out of scope for the RSC ISR cache write path. The first const __cachePathname = navigationPathname at line 2163 is inside an if block (lines 2158–2309). The RSC cache write at line 2675 references __cachePathname but lives outside that block scope. Since ES modules use strict mode, this will throw a ReferenceError at runtime for ISR-eligible RSC requests. The old code used cleanPathname (a let in function scope), so this is a regression introduced by this PR. See inline comment for fix.

Non-blocking observations

1. template: null doesn't reset stashedTemplate. If a layout sets title: { template: null }, the current resolveTitleState ignores it (null is falsy), so an ancestor's template bleeds through to deeper descendants. In Next.js, template: null should block ancestor template inheritance. This is pre-existing behavior and out of scope for this PR, but worth a follow-up.

2. The __cachePathname pattern of re-declaring const __cachePathname = navigationPathname in each ISR block is fragile. Consider declaring it once alongside navigationPathname at line 1524 to avoid this class of scoping issues.

[ask-bonk]

Bug: __cachePathname is block-scoped but referenced outside this block.

This const declaration lives inside the ISR cache-read if block (lines 2158–2309). However, __cachePathname is also referenced at lines 2675 and 2680 in the RSC cache-write path, which is outside this block. Since const is block-scoped and ES modules are strict mode, this will throw a ReferenceError at runtime for ISR-eligible RSC requests (e.g. client-side navigation to an ISR page).

The old code used cleanPathname (a let in function scope), so this is a regression.

Fix: Declare __cachePathname once in the function scope alongside navigationPathname, rather than inside each ISR block:

Suggested change

	const __cachePathname = navigationPathname;
	const __cachePathname = navigationPathname;

Or better yet, hoist it to line 1524 right after const navigationPathname = cleanPathname;:

const __cachePathname = navigationPathname;

and remove the duplicate declarations at lines 2163 and 2854.

[ask-bonk]

Nit (non-blocking): When title.template is explicitly null (which the updated Metadata type now allows), this line sets titleTemplate = null. Since null is falsy, it won't update stashedTemplate at line 312, meaning an ancestor's template bleeds through to deeper descendants. In Next.js, template: null is the mechanism to explicitly opt out of template inheritance.

This is pre-existing behavior and out of scope for this PR, but worth noting for a follow-up. The fix would be:

if (titleTemplate !== undefined) {
  stashedTemplate = titleTemplate ?? undefined;
}

[ask-bonk]

Nit (non-blocking): title.absolute is typed as string | undefined. An empty string "" is falsy and would be skipped here, falling back to the title.default branch. In Next.js, title: { absolute: "" } produces an empty <title></title> (explicitly clearing the title). Consider using title.absolute !== undefined instead of a truthiness check.

[ask-bonk]

The fallback || "" means that if resolvedAbsolute is undefined (no ancestor set a title), the parent object will have absolute: "". This is a reasonable default for the parent accumulator, but it means generateMetadata(_, parent) will see (await parent).title.absolute === "" even when no title was set. Not a correctness issue, just noting for awareness.

[ask-bonk]

Review submitted. Here's a summary of what I flagged:

Blocking issue

__cachePathname scoping bug — The const __cachePathname = navigationPathname at app-rsc-entry.ts:2163 is inside an if block (ISR cache read, lines 2158–2309), but it's referenced at line 2675 in the RSC ISR cache write path which is outside that block scope. Since const is block-scoped and ES modules run in strict mode, this will throw a ReferenceError at runtime for ISR-eligible RSC requests (client-side navigation to ISR pages). The old code used cleanPathname (function-scoped let), so this is a regression. Fix: hoist the declaration to function scope alongside navigationPathname.

Non-blocking observations

1. template: null doesn't reset stashedTemplate in resolveTitleState — ancestor templates bleed through, which differs from Next.js behavior where template: null explicitly blocks inheritance. Pre-existing, out of scope.

2. title.absolute empty string — truthiness check at line 304 skips absolute: "", which Next.js treats as an explicit empty title. Minor edge case.

3. mergeMetadataForParent fallback — absolute: resolvedAbsolute || "" means generateMetadata sees title.absolute === "" when no title is set, which is a reasonable default but worth awareness.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review

Good work. The two core fixes are correct and well-tested:

1. Pathname preservation after rewrites: Capturing navigationPathname before rewrites mutate cleanPathname is the right approach. Using it for setNavigationContext ensures usePathname() returns the canonical URL. Using __cachePathname (same value) for ISR cache keys is also correct — the cache should be keyed by the source URL, not the rewrite target, since cache lookups happen before rewrites are applied.

2. Metadata title resolution: The new resolveTitleState function cleanly separates the title resolution algorithm from the merge logic. The terminal flag correctly distinguishes page-level merges (where the last entry's template is ignored) from parent accumulation (where all templates are preserved). The mergeMetadataForParent function correctly exposes { absolute, template } to generateMetadata(_, parent) consumers.

The ISR rewrite test at app-router.test.ts:1689 is thorough — it validates the full MISS → HIT → STALE → background regen → HIT cycle and confirms source vs. target cache independence.

Note on the ask-bonk __cachePathname scoping concern: That was a false alarm. The reviewer analyzed the old code where __cachePathname was declared inside if blocks. In this PR, __cachePathname is declared at function scope (line 1525 of the template), alongside navigationPathname. All 18 references are within the same _handleRequest function body. No scoping issue exists.

Two minor suggestions inline, neither blocking.

[ask-bonk]

Nit: resolvedTitle.absolute || "" collapses undefined and "" to the same value, which is fine for the parent accumulator. But if you ever want to distinguish "no title was set" from "title was explicitly cleared to empty string via absolute: """, you'd want resolvedTitle.absolute ?? "" instead. Not blocking — the current behavior is a reasonable default for generateMetadata(_, parent) consumers.

[ask-bonk]

Minor perf nit: mergeMetadata already calls resolveTitleState(metadataList) internally (line 347), and then this function calls resolveTitleState(metadataList, { terminal: false }) again. That's two passes over the metadata list. You could avoid this by extracting the non-title merge loop into a shared helper and calling resolveTitleState only once per function. Not blocking — the metadata lists are tiny (typically 2-4 entries), so this is negligible in practice.

[ask-bonk]

Review posted successfully. Here's a summary of my findings:

Overall verdict: The PR is correct and well-tested. No blocking issues.

Key findings:

1. Pathname preservation (navigationPathname / __cachePathname): Both are declared at function scope (line 1525) before any rewrites can mutate cleanPathname. The previous reviewer's scoping concern was a false alarm — they were analyzing the old code structure. The ISR cache key correctly uses the pre-rewrite pathname, since cache lookups happen based on incoming request URLs.

2. Metadata title resolution (resolveTitleState): The algorithm correctly handles template application, absolute bypass, template: null inheritance blocking, and the terminal/non-terminal distinction for page vs parent accumulation. I traced through the template: null test case manually and confirmed the logic is correct.

3. Test coverage: Comprehensive — 15 new unit tests for title resolution, integration tests for pathname preservation across the full ISR lifecycle (MISS → HIT → STALE → regen → HIT), config-level and middleware-level rewrite tests, and additional fixtures for grouped routes, cookies().has(), permanentRedirect(), social metadata, and the /not-found route conflict edge case.

4. Two non-blocking suggestions: (a) using ?? instead of || in mergeMetadataForParent to preserve the semantic distinction between "no title" and "empty title", (b) avoiding the double resolveTitleState call in mergeMetadataForParent.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review

This PR correctly fixes two parity gaps with Next.js and the test coverage is thorough. The approach is clean and well-structured.

Pathname preservation after rewrites

Capturing navigationPathname (and its alias __cachePathname) at function scope before middleware/config rewrites can mutate cleanPathname is the right design. This ensures:

• usePathname() reflects the canonical source URL, not the rewrite destination
• ISR cache keys use the source URL (correct, since cache lookups happen before rewrites)
• Background regeneration also uses the correct navigation context

The pageSearchParams split is also correct: setNavigationContext uses url.searchParams (original request) for useSearchParams(), while pageSearchParams (potentially rewritten) is passed to buildPageElement for the page's searchParams prop.

Note on the earlier scoping concern: __cachePathname is declared at line 1525, in the _handleRequest function body, not inside any block. No scoping issue.

Metadata title resolution

The new resolveTitleState function is a well-factored algorithm that correctly handles:

• Template application via stashedTemplate (accumulated from ancestors)
• title.absolute bypassing all templates
• The terminal flag distinguishing page-level merges (where the last entry's template is ignored) from parent accumulation
• template: null blocking ancestor template inheritance via stashedTemplate = titleTemplate ?? undefined
• Multi-placeholder templates (%s | %s) via replace(/%s/g, ...)

The separation into mergeMetadata (terminal, returns flat string title) and mergeMetadataForParent (non-terminal, returns { absolute, template } object) is the right design — it preserves the structured title state that generateMetadata(_, parent) consumers need.

Test coverage

Excellent coverage: 15 unit tests for title resolution edge cases, integration tests for pathname preservation across the full ISR lifecycle (MISS → HIT → STALE → regen → HIT), config-level and middleware-level rewrite pathname tests, and additional fixtures for grouped routes, cookies().has(), permanentRedirect(), social metadata, and the /not-found route conflict edge case.

Two minor non-blocking observations inline.

[ask-bonk]

Nit (non-blocking): __cachePathname is an alias for navigationPathname with no divergence point — they're always the same value. Consider whether the separate name adds clarity or whether using navigationPathname directly for cache keys would be simpler. The separate name does communicate intent ("this is for cache key computation"), so it's fine either way.

[ask-bonk]

Nit (non-blocking): resolvedTitle.absolute ?? "" collapses undefined to "", which is a reasonable default for the parent accumulator. But if a future consumer needs to distinguish "no title was set" from "title was explicitly cleared", consider ?? vs || semantics. Current behavior is correct for generateMetadata(_, parent) consumers.

Also, as noted by a previous reviewer, mergeMetadataForParent calls resolveTitleState(metadataList, { terminal: false }) and then mergeMetadata (called separately on the final list) calls resolveTitleState(metadataList) — two passes. Since metadata lists are tiny (2-4 entries), this is negligible, but if you ever refactor the merge pipeline, you could extract a single resolveTitleState call that returns both terminal and non-terminal results.

[ask-bonk]

Review submitted with approval. Here's a summary:

Verdict: Approved

The PR correctly fixes two Next.js parity gaps:

1. Pathname preservation after rewrites — navigationPathname is captured at function scope before rewrites mutate cleanPathname, ensuring usePathname() returns the canonical source URL. ISR cache keys also correctly use the pre-rewrite pathname. The pageSearchParams split properly distinguishes navigation context (original request params for useSearchParams()) from page props (potentially rewritten params for the searchParams prop).

2. Metadata title resolution — The new resolveTitleState function cleanly separates the title resolution algorithm from the merge logic. The terminal flag correctly distinguishes page-level merges from parent accumulation, and mergeMetadataForParent preserves the structured { absolute, template } shape that generateMetadata(_, parent) consumers expect.

I left two minor non-blocking observations: one about whether __cachePathname adds clarity as a separate name vs. using navigationPathname directly, and one about the ?? "" fallback semantics in mergeMetadataForParent.

github run