fix(app-router): prevent repeated hard-navigation loops

[NathanDrake2406]

What this changes

Implements #726-COMPAT-04/05 from #726.

This adds a browser-side hard-navigation loop guard for App Router root-boundary hard navigation. When a hard navigation has already been attempted for a target and the browser is already on that target URL, vinext now blocks the repeated attempt, logs the recovery decision, clears the guard, and leaves the current document in place instead of reloading forever.

It also adds explicit old-client/new-server coverage for future compatibility metadata. A payload with a future artifact compatibility schema still degrades to unknown compatibility and renderFresh; it does not become positive reuse proof.

Bonk: please read issue #726 for the big-picture architecture before reviewing this slice.

Why

Issue #726 calls out deployment compatibility as algebra rather than strict equality everywhere. From first principles, a router should only reuse or visibly commit work when it has proof that the work belongs to the same semantic world: compatible graph, compatible deployment, compatible root boundary, compatible payload schema, and a still-current lifecycle token. When that proof is missing, the fallback can be a hard navigation, but the fallback itself also needs a bounded safety contract.

A hard navigation is a recovery effect, not semantic proof. If the same client/server skew keeps producing the same hard-navigation decision for the same URL, repeating the effect no longer moves the system toward a safer state. It just burns the current document and starts over. The correct invariant is: try the hard navigation once for the target, then stop if the freshly loaded document reaches the same decision again for that same target.

For example, an old client on /dashboard can request RSC from a new server whose root-boundary metadata is incompatible with the visible tree. The client correctly chooses a hard navigation to /dashboard. If the loaded document still has the old client bundle and receives the same incompatible payload again, an unguarded window.location.assign('/dashboard') loops forever. With this guard, the first hard navigation is allowed, the repeated same-target hard navigation is blocked, and the server-rendered document remains visible.

The existing root-boundary path had a direct window.location.assign(...) side effect with no persisted memory of the attempted target. That preserved current root-boundary behavior, but it did not bound the recovery path if the same client/server skew kept producing the same hard-navigation decision for the same URL.

Approach

• Route App Router hard-navigation effects through a small loop-guard helper in the browser navigation controller.
• Persist the last attempted hard-navigation target in sessionStorage.
• Block only when the stored target matches and the browser is already on that same target URL, so ordinary cross-root navigations to the same destination from a different page still work.
• Clear the guard after successful initial RSC bootstrap, matching the existing one-shot initial RSC reload guard lifecycle.
• Keep compatibility fallback conservative: future or malformed compatibility metadata remains unknown proof and falls back to fresh render.
• Document the intentionally ignored same-URL server-action guard result and the storage-unavailable cross-URL asymmetry.

Non-goals for this PR match #726-COMPAT-04/05: no full rolling deploy protocol, no skip transport, and no cache/reuse authority changes.

Validation

Next.js reference checked: test/e2e/app-dir/segment-cache/deployment-skew/deployment-skew.test.ts in the local Next.js canary clone, which covers deployment skew hard-navigation behavior.

• vp test run tests/app-browser-entry.test.ts tests/app-elements.test.ts passed: 126 tests.
• vp check tests/app-browser-entry.test.ts packages/vinext/src/server/app-browser-navigation-controller.ts passed with no warnings after the Bonk follow-up.
• vp check tests/app-browser-entry.test.ts tests/app-elements.test.ts packages/vinext/src/server/app-browser-navigation-controller.ts packages/vinext/src/server/app-browser-entry.ts passed with no warnings before the Bonk follow-up.
• vp check exited successfully. It reported one existing warning outside this PR in packages/vinext/src/server/request-pipeline.ts for unknown | undefined.

[pkg-pr-new]

Open in StackBlitz

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

commit: 04307e9

[james-elicx]

/bigbonk review

[ask-bonk]

Review: fix(app-router): prevent repeated hard-navigation loops

This is a clean, well-scoped implementation of #726-COMPAT-04/05. The loop guard is correctly bounded (one attempt per target, cleared on successful bootstrap), the sessionStorage fallback is handled gracefully, and the DI seam via performHardNavigation keeps the controller testable.

The test assertion fixes for trace entry indices are a legitimate fix — #1101 prepended transaction codes to traces but didn't update these four assertions. The future-schema compatibility test in app-elements.test.ts is a good addition to lock the conservative fallback behavior.

All 125 tests pass locally (vp test run tests/app-browser-entry.test.ts tests/app-elements.test.ts).

Observations (none blocking)

1. Ignored return value in commitSameUrlNavigatePayload (app-browser-navigation-controller.ts:575, :590)

Both hard-navigate paths in commitSameUrlNavigatePayload ignore the return value of performHardNavigation. If the loop guard blocks the navigation and returns false, the function still returns undefined. That's the correct degraded state for the same-URL action path (the server-rendered document stays visible), but a one-line comment explaining the ignored return is deliberate would help a future reader who might wonder if this should propagate differently from the renderNavigationPayload path (which maps false → "no-commit").

2. Asymmetric sessionStorage-unavailable behavior (app-browser-navigation-controller.ts:155-162)

When !guardPersisted && currentHref === targetHref, the navigation is correctly blocked. When !guardPersisted && currentHref !== targetHref, the navigation proceeds without the guard. That's correct — cross-URL hard navigations can't loop — but a brief comment explaining the asymmetry would help readers.

3. Unused HardNavigationMode "replace" path (app-browser-navigation-controller.ts:30, :164)

HardNavigationMode supports "replace" but all three call sites use the default "assign". The replace branch at line 164 is dead code today. Fine as forward-compatible design — just noting it.

4. Suggested additional test case

The loop-guard test (line 1973) covers the core scenario well. One case that would strengthen coverage: verify a cross-page hard navigation to the same target still works. E.g., the browser is on /settings (not /dashboard), the guard has /dashboard stored, and a hard navigation to /dashboard should still be allowed because currentHref !== targetHref. This exercises the currentHref === targetHref conjunct and confirms the guard doesn't over-block.

Summary

Solid work. The guard design mirrors the existing recoverFromBadInitialRscResponse pattern (sessionStorage-backed one-shot with persistence verification), which is good architectural consistency. The clearHardNavigationLoopGuard() placement alongside clearReloadFlag() in both initial-RSC success paths is correct — both guards share the same lifecycle.

[ask-bonk]

Review posted successfully. Here's a summary of the review:

PR #1111 — fix(app-router): prevent repeated hard-navigation loops

This is a well-scoped implementation of #726-COMPAT-04/05. The core change routes hard-navigation side effects through a sessionStorage-backed loop guard that allows one attempt per target URL and blocks repeated same-target navigations. The guard is cleared on successful RSC bootstrap (alongside the existing reload flag), which gives it the correct one-shot lifecycle.

Key points from the review:

1. All 125 tests pass — both app-browser-entry.test.ts and app-elements.test.ts.
2. The trace assertion fixes are legitimate — #1101 prepended transaction codes but missed updating 4 existing assertions.
3. The performHardNavigation DI seam keeps the controller testable.
4. Suggested a comment for the intentionally-ignored return value in commitSameUrlNavigatePayload, a comment for the asymmetric sessionStorage-unavailable behavior, noted the unused "replace" mode, and suggested an additional test for cross-page hard navigation to the same target.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review: fix(app-router): prevent repeated hard-navigation loops

Clean implementation. The loop guard design is sound: sessionStorage-backed one-shot with persistence verification, cleared alongside clearReloadFlag() in both initial-RSC success paths. The DI seam via performHardNavigation keeps the controller testable without needing to mock window.location.assign separately from the guard logic.

The trace assertion fixes (entries[0] to entries[1]) are a legitimate catch. The future-schema compatibility test locks down the conservative fallback, which is a good addition.

The cross-page guard test (line 2040) directly addresses the earlier review feedback about verifying the guard does not over-block when currentHref !== targetHref. Good coverage.

One minor observation inline, otherwise LGTM.

[ask-bonk]

Inline note for app-browser-navigation-controller.ts:167-172:

Nit: location.assign(href) and location.replace(href) pass the original (potentially relative) href, while the guard stores the normalized targetHref. This is correct — the browser will resolve the relative URL the same way normalizeBrowserHref does, so the guard comparison on the next page load will match. Just noting that the asymmetry is intentional for anyone reading this later.

[ask-bonk]

Review posted. Here's a summary:

PR #1111 — fix(app-router): prevent repeated hard-navigation loops

This is a well-scoped, clean implementation of #726-COMPAT-04/05. The core changes:

1. Loop guard (performHardNavigationWithLoopGuard): sessionStorage-backed one-shot guard that allows one hard navigation per target URL, blocks on repeated same-target navigation, and degrades gracefully when sessionStorage is unavailable. The design mirrors the existing recoverFromBadInitialRscResponse pattern — good architectural consistency.

2. DI seam (performHardNavigation in deps): Keeps the controller testable by injecting the navigation function rather than directly calling window.location.assign.

3. Guard clearing: clearHardNavigationLoopGuard() placed alongside clearReloadFlag() in both initial-RSC success paths (embedded RSC and fetched RSC) — correct shared lifecycle.

4. Trace assertion fixes: Four test assertions updated from entries[0] to entries[1] to account for feat(app-router): trace approved visible commit outcomes #1101 prepending transaction codes. Legitimate fix.

5. Future-schema compatibility test: Locks down the conservative behavior — a payload with a future schemaVersion is treated as unknown proof and falls back to renderFresh, not positive reuse proof.

6. Cross-page guard test: Directly addresses the earlier review suggestion about verifying the guard doesn't over-block when currentHref !== targetHref.

One minor observation posted about the intentional asymmetry between normalized URLs in the guard vs raw URLs in location.assign/location.replace.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review: fix(app-router): prevent repeated hard-navigation loops

Verified all 126 tests pass locally. The implementation is correct, well-bounded, and architecturally consistent with the existing recoverFromBadInitialRscResponse pattern.

What I verified

1. Guard logic is sound. The performHardNavigationWithLoopGuard function correctly handles the four cases: (a) stored guard matches + same URL → block, (b) storage unavailable + same URL → block, (c) storage unavailable + different URL → allow (forward progress), (d) normal case → write guard + navigate. The two-conjunct check (storedTarget === targetHref && currentHref === targetHref) is the right predicate — it prevents over-blocking cross-page navigations to the same destination.

2. Guard lifecycle is correct. clearHardNavigationLoopGuard() is placed alongside clearReloadFlag() in both initial-RSC success paths (embedded RSC at line 698, fetched RSC at line 762). The guard is not cleared in recoverFromBadInitialRscResponse, which is correct — that path handles bad RSC fetches before the client starts doing client-side navigations.

3. DI seam works well. The performHardNavigation dependency injection keeps the controller testable. The tests correctly use the real guard implementation (via stubWindow) rather than mocking it away, which exercises the actual sessionStorage interaction.

4. Outcome mapping is correct. The renderNavigationPayload hard-navigate path (line 509) maps performHardNavigation returning false to "no-commit", which is the right degraded state — the current document stays visible. The commitSameUrlNavigatePayload paths (lines 581, 598) intentionally ignore the return value and return undefined, matching the existing server-action return contract.

5. Future-schema test is good. The app-elements.test.ts addition (line 726) correctly exercises the end-to-end path: parseArtifactCompatibilityEnvelope returns null for a future schemaVersion (via the hasCurrentSchemaVersions check), readArtifactCompatibilityMetadata falls back to the empty envelope, and evaluateArtifactCompatibility produces { kind: "unknown", fallback: "renderFresh" }. This locks down the conservative behavior for forward compatibility.

6. Cross-page guard test (line 2076) directly validates the guard doesn't over-block when currentHref !== targetHref, which was the right additional test case to include.

No blocking issues. One minor inline note.

[ask-bonk]

Extremely minor: location.assign(href) and location.replace(href) pass the original (potentially relative) href, while the guard stores normalizeBrowserHref(href). This is correct — the browser resolves relative URLs the same way new URL(href, location.href) does, so the guard comparison on the next page load will match. But if you ever add query-stripping or hash-stripping to normalizeBrowserHref, these would silently diverge. A one-line comment noting the intentional asymmetry would help a future reader:

Suggested change

	if (mode === "replace") {
	window.location.replace(href);
	} else {
	window.location.assign(href);
	}
	return true;
	// The guard stores the normalized targetHref, but we pass the original href
	// to the browser. The browser resolves relative URLs identically to
	// normalizeBrowserHref, so the guard comparison on the next page load
	// matches. If normalizeBrowserHref ever strips query/hash, this must be
	// revisited.
	if (mode === "replace") {
	window.location.replace(href);
	} else {
	window.location.assign(href);
	}
	return true;

[ask-bonk]

Review posted and approved. Here's the summary:

PR #1111 — APPROVED

All 126 tests pass. The implementation is correct and architecturally consistent with the existing recoverFromBadInitialRscResponse pattern. Key points verified:

1. Guard logic handles all four cases correctly — stored match + same URL blocks, storage unavailable + same URL blocks, storage unavailable + different URL allows forward progress, normal case writes guard and navigates.
2. Guard lifecycle is right — cleared alongside clearReloadFlag() in both initial-RSC success paths, not cleared in the bad-RSC recovery path (which is correct since hard-navigation loops only happen after successful bootstrap).
3. DI seam via performHardNavigation keeps the controller testable while tests exercise the real sessionStorage interaction through stubWindow.
4. Outcome mapping is correct — renderNavigationPayload maps blocked navigation to "no-commit", commitSameUrlNavigatePayload intentionally ignores the return (matching existing contract).
5. Future-schema compatibility test locks down conservative fallback behavior end-to-end.
6. Cross-page guard test confirms the guard doesn't over-block.

One minor suggestion: add a comment noting the intentional asymmetry between normalized URLs in the guard vs raw URLs passed to location.assign/location.replace.

github run