fix(app-router): forbidden()/unauthorized() escalate past intermediate layouts (#1547)#1647
Conversation
…intermediate layouts (#1547) When a deep page called `forbidden()`/`unauthorized()`/`notFound()` and the intermediate layout segments had no matching boundary file, vinext's page-error fast path wrapped the resolved root boundary in *all* of the route's layouts. That left the intermediate layout's chrome ("Dynamic with Layout") rendered next to the boundary fallback, diverging from Next.js where the nearest ancestor boundary owns the entire fallback subtree. `renderPageSpecialError` now resolves the deepest layout-level boundary via `resolveAppPageParentHttpAccessBoundary` and slices the layouts to `[0..boundaryLayoutIndex]`, so layouts beneath the boundary owner stop rendering. When the route-level boundary file lives next to the page (not aligned to a layout), all layouts are kept as before. Ported the Next.js regression cases from `test/e2e/app-dir/forbidden/basic/forbidden-basic.test.ts` and `test/e2e/app-dir/unauthorized/basic/unauthorized-basic.test.ts`.
|
/bigbonk review |
commit: |
|
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean fix that correctly addresses boundary escalation for forbidden()/unauthorized()/notFound() thrown from pages.
What I verified
The core fix is correct. The key insight is that discoverBoundaryFilePerLayout maps boundary files per-layout directory, and when the root forbidden.tsx lives alongside app/layout.tsx, it appears at forbiddens[0]. So resolveAppPageParentHttpAccessBoundary returns layoutIndex: 0 (not null), and the layout slice [0, boundaryLayoutIndex + 1] correctly keeps only the root layout, excluding intermediate layouts that lack a boundary.
The useLayoutAlignedBoundary guard is sound. When a boundary file sits next to the page (not at a layout level), routeBoundaryModule !== parentBoundaryModule causes the code to fall through to the default path (all layouts render), which matches Next.js behavior where a page-level boundary wraps just the page subtree.
Reference equality works here because route.forbidden (from discoverBoundaryFile) and route.forbiddens[0] (from discoverBoundaryFilePerLayout) resolve to the same import variable when they point to the same file, so they're the same module object at runtime.
Tests are well-structured. The fixtures are faithful ports of the Next.js test cases, and the assertions correctly verify both positive content (boundary renders) and negative content (intermediate layout UI is suppressed).
Minor observations
The existing notFound() escalation test in tests/nextjs-compat/not-found.test.ts:118 ("notFound() escalates to root not-found when no local boundary exists") exercises the same scenario but doesn't assert that the intermediate layout's "Dynamic with Layout" text is suppressed. With this PR's fix, that suppression now happens for notFound() too (the resolveAppPageRouteBoundaryModule handles status 404). Consider adding expect(html).not.toContain("Dynamic with Layout") to that test in a follow-up — it would strengthen the regression coverage for the notFound() case specifically.
No blocking issues.
|
Review posted. Approved PR #1647. Summary: The fix correctly resolves the boundary escalation bug by introducing The one minor follow-up I noted: the existing |
1 participant
Summary
forbidden(),unauthorized(), ornotFound()and the intermediate layout segments had no matching boundary file, vinext's page-error fast path wrapped the resolved root boundary in all of the route's layouts. That left the intermediate layout's chrome ("Dynamic with Layout") rendered next to the boundary fallback, diverging from Next.js where the nearest ancestor boundary owns the entire fallback subtree.renderPageSpecialErrornow resolves the deepest layout-level boundary via a newresolveAppPageParentHttpAccessBoundaryhelper and slices the layouts to[0..boundaryLayoutIndex], so layouts beneath the boundary owner stop rendering. When the route-level boundary file lives next to the page (not aligned to a layout), all layouts are kept as before.test/e2e/app-dir/forbidden/basic/forbidden-basic.test.tsandtest/e2e/app-dir/unauthorized/basic/unauthorized-basic.test.ts.Fixes #1547.
Test plan
pnpm test tests/app-router.test.ts -t "1547"— new escalation cases passpnpm test tests/app-router.test.ts -t "boundary|escalate|deep|access|nearest"— all 11 related tests pass (incl. the existing layout-thrown forbidden/unauthorized tests andnotFound()escalation)pnpm test tests/app-page-dispatch.test.ts tests/app-fallback-renderer.test.ts tests/error-boundary.test.tspnpm run check(format + lint + types)