fix: move NotFoundBoundary inside Template in per-segment wiring#784
Conversation
Add a structural element-tree test that verifies NotFoundBoundary is nested inside the Template component (not outside it) in the per-segment wrapping chain. Walks the React element tree to compare component depths, asserting Template appears at a shallower depth than NotFoundBoundary.
The per-segment wrapping loop was applying NotFoundBoundary after the Template wrap (building bottom-up), which placed it outside the Template in the final tree: Layout > NotFoundBoundary > Template > Error > Page (wrong) When notFound() fires, NotFoundBoundary catches the error and renders its fallback — but because it was outside Template, the Template was no longer in the tree. Next.js's correct order is: Layout > Template > Error > NotFoundBoundary > Page (correct) Fix: move the layoutNotFoundComponent wrapping block to the start of the loop body (before the ErrorBoundary wrap), so it becomes the innermost wrapper. Bottom-up build order is now: 1st wrap: NotFoundBoundary (innermost) 2nd wrap: ErrorBoundary 3rd wrap: Template 4th wrap: Layout (outermost) This matches Next.js semantics: templateValue wraps ErrorBoundary > NotFoundBoundary > Page, and the Template component receives that subtree as children.
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Fix is correct. Verified against Next.js source (packages/next/src/client/components/layout-router.tsx) — the per-segment nesting order is Layout > Template > ErrorBoundary > LoadingBoundary > NotFoundBoundary > children. The old code had NotFoundBoundary outside Template, which meant notFound() would replace the entire Template subtree. Moving it inside ErrorBoundary/Template is the right fix.
The code change is minimal and targeted — just moving the layoutNotFoundComponent wrapping to the top of the loop body so it becomes the innermost wrapper when building bottom-up. The test is well-structured and validates the element tree nesting order.
Two minor nits below, neither blocking.
Not in this diff but worth a follow-up: The doc comment on NotFoundBoundaryInner in error-boundary.tsx:107-108 says "This must be placed above the ErrorBoundary in the component tree" — that was the old (incorrect) behavior. With this fix, NotFoundBoundary is now correctly placed below (inside) ErrorBoundary. The ErrorBoundary.getDerivedStateFromError re-throw ensures notFound errors propagate regardless, so no correctness issue, but the comment could confuse future readers.
| const templateDepth = depthMap.get("RootTemplate"); | ||
| // NotFoundBoundary renders as NotFoundBoundaryInner at the class level. | ||
| // We search for the class component by its name. | ||
| const notFoundDepth = depthMap.get("NotFoundBoundaryInner") ?? depthMap.get("NotFoundBoundary"); |
There was a problem hiding this comment.
Nit: This comment is slightly inaccurate. Since walkDepth operates on the static React element tree (not the rendered component tree), it will find "NotFoundBoundary" (the function wrapper) but never "NotFoundBoundaryInner" (which is only instantiated when NotFoundBoundary renders). The ?? fallback here will always resolve to the right-hand side depthMap.get("NotFoundBoundary").
Not a correctness issue — the test works as intended — but the comment on line 264 implies NotFoundBoundaryInner is the primary lookup target when it's actually unreachable in a static tree walk.
| const notFoundDepth = depthMap.get("NotFoundBoundaryInner") ?? depthMap.get("NotFoundBoundary"); | |
| // NotFoundBoundary is a function component wrapping the NotFoundBoundaryInner class. | |
| // In the static element tree (before render), we see the outer NotFoundBoundary. | |
| const notFoundDepth = depthMap.get("NotFoundBoundaryInner") ?? depthMap.get("NotFoundBoundary"); |
|
Review submitted. Here's a summary of my findings: Approved — the fix is correct and matches Next.js's actual component nesting order. Key findings:
|
Summary
NotFoundBoundarywas placed outsideTemplatein the per-segment layout loop, producingLayout > NotFound > Template > Error > PageTemplateContext— the correct order isLayout > Template > Error > NotFound > PagenotFound()fired, the NotFound fallback replaced the entire Template subtree instead of rendering inside itMoves the
layoutNotFoundComponentwrapping to the top of the loop body (beforeErrorBoundary), making it the innermost wrapper. Since the loop builds bottom-up, this produces the correct outer-to-inner nesting.This is a pre-existing bug surfaced by the comment introduced in #780.
Test plan
NotFoundBoundary is nested inside Template) walks the React element tree and assertsRootTemplateappears at a shallower depth thanNotFoundBoundaryInnertests/app-page-route-wiring.test.tspass after fixvp checkpasses with no type errors