refactor: extract route wiring from generated entry into typed runtime module#748
refactor: extract route wiring from generated entry into typed runtime module#748NathanDrake2406 wants to merge 7 commits intocloudflare:mainfrom
Conversation
commit: |
NathanDrake2406
changed the title
Code reviewFound 1 issue:
vinext/packages/vinext/src/shims/error-boundary.tsx Lines 25 to 82 in ca40d05 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Extract route wiring from generated entry into typed runtime module
This is a well-structured refactor that aligns with the project's architectural guideline that generated entries should stay thin. The extraction is mechanically faithful with a few intentional improvements, the type definitions are thorough, and the test coverage is solid.
Overall Assessment
The core extraction is correct. The generated buildPageElement() now delegates to __buildAppPageRouteElement() with a clean options bag, and the inline __resolveChildSegments is replaced by the typed resolveAppPageChildSegments. The RSC/SSR boundary is properly maintained — error-boundary.tsx has "use client", so the RSC plugin converts its imports to client references in app-page-route-wiring.tsx.
Behavioral Changes
I identified two intentional behavioral differences in resolveAppPageChildSegments vs the old __resolveChildSegments:
-
== null→=== undefinedfor optional catch-all: The old code skipped bothnullandundefinedparam values; the new code only skipsundefined. This is more correct for theAppPageParamstype which doesn't includenull. -
||→??for fallback values in catch-all and dynamic segments: The old code treated empty string""as falsy and fell back to the raw segment syntax (e.g.,"[slug]"); the new code preserves empty strings. This is a correctness improvement — leaking raw segment syntax touseSelectedLayoutSegments()consumers was a bug.
Both are improvements, but neither is covered by a dedicated test case (see inline comments).
ErrorBoundary Pathname Reset
The getDerivedStateFromError → getDerivedStateFromProps interaction is correctly implemented. Returning Partial<ErrorBoundaryState> from getDerivedStateFromError (without previousPathname) ensures React merges the error into existing state without overwriting the tracked pathname. The test at line 212 validates the critical sequence where an error is caught and then getDerivedStateFromProps runs on the same pathname without clearing it.
The existing PR comment about verifying against Next.js source is valid — the PR description mentions this was "identified as a required fix in #726" but doesn't link to the specific Next.js source/test. The test file header (lines 22-28) references Next.js source paths which is good, but the individual test cases for pathname reset don't have // Ported from Next.js: comments.
| if ( | ||
| segment.startsWith("[[...") && | ||
| segment.endsWith("]]") && | ||
| segment.length > "[[...x]]".length - 1 |
There was a problem hiding this comment.
Nit: this length check is a bit convoluted. "[[...x]]".length - 1 evaluates to 8, so the condition is segment.length > 8, meaning minimum 9 characters. Since "[[...x]]" (the shortest valid optional catch-all) is exactly 9 characters, a clearer way to express this would be >= "[[...x]]".length:
| segment.length > "[[...x]]".length - 1 | |
| segment.length >= "[[...x]]".length |
| if (Array.isArray(paramValue) && paramValue.length === 0) { | ||
| continue; | ||
| } | ||
| if (paramValue === undefined) { |
There was a problem hiding this comment.
Behavioral change: the old code used v == null (loose equality, skips both null and undefined), but this uses === undefined (strict, only skips undefined). This is more correct for the AppPageParams type, but it's not covered by a test. Consider adding a test case for an explicitly null param value to lock down the intentional change:
it("skips null param values for optional catch-all", () => {
expect(
resolveAppPageChildSegments(["docs", "[[...slug]]"], 0, { slug: null as any }),
).toEqual(["docs"]);
});(Or if you'd prefer it to push null, document that as the intentional difference.)
| resolvedSegments.push(paramValue.join("/")); | ||
| continue; | ||
| } | ||
| resolvedSegments.push(paramValue ?? segment); |
There was a problem hiding this comment.
Behavioral change: || → ??. The old code used v2 || seg which fell back to the raw segment for any falsy value (including empty string ""). The new ?? only falls back for null/undefined, preserving empty strings. This is more correct but not tested. Consider adding:
it("preserves empty-string param values instead of falling back to raw segment", () => {
expect(
resolveAppPageChildSegments(["blog", "[...slug]"], 0, { slug: "" }),
).toEqual(["blog", ""]);
});|
|
||
| const routeSlots = options.route.slots ?? {}; | ||
| const layoutEntries = createAppPageLayoutEntries(options.route); | ||
| const routeThenableParams = options.makeThenableParams(options.matchedParams); |
There was a problem hiding this comment.
Minor optimization: the old code called makeThenableParams(params) fresh for every layout iteration, creating a new Promise each time. The new code calls it once and reuses the result. This is fine (all resolve to the same value), but worth noting this is an intentional change if anyone ever depends on referential inequality between layout params promises.
| } | ||
|
|
||
| static getDerivedStateFromError(error: Error): ErrorBoundaryState { | ||
| static getDerivedStateFromError(error: Error): Partial<ErrorBoundaryState> { |
There was a problem hiding this comment.
Good use of Partial<ErrorBoundaryState> here. By not including previousPathname in the return, React merges just { error } into existing state, preserving the tracked pathname. This prevents the immediate-clear bug where getDerivedStateFromProps would see a mismatched pathname and reset the error before it could render.
One concern: React guarantees that getDerivedStateFromProps runs after getDerivedStateFromError during the error re-render, and the merged state (with preserved previousPathname) is passed to it. The test at error-boundary.test.ts:212 validates this sequence correctly. However, the test simulates the merge manually with spread ({ ...baseState, ...getDerivedStateFromError(error) }). Consider adding a rendered-component test (using @testing-library/react or similar) that throws an error inside the boundary and verifies the fallback actually renders, to validate the full React lifecycle integration.
| if (props.pathname !== state.previousPathname && state.error) { | ||
| return { error: null, previousPathname: props.pathname }; | ||
| } | ||
| return { error: state.error, previousPathname: props.pathname }; |
There was a problem hiding this comment.
This always returns a new state object even when nothing changed (same pathname, no error). React will still skip the re-render if the values are the same (shallow comparison), so this isn't a correctness issue, but returning null when props.pathname === state.previousPathname && !state.error would be slightly more idiomatic:
| return { error: state.error, previousPathname: props.pathname }; | |
| if (props.pathname !== state.previousPathname) { | |
| return { error: state.error ? null : state.error, previousPathname: props.pathname }; | |
| } | |
| return null; |
Actually, on reflection, the current approach is simpler and easier to reason about. The always-return-state pattern avoids subtle bugs where the pathname tracking falls out of sync. Feel free to ignore this — it's a style preference, not a correctness issue.
| slug: "post", | ||
| }), | ||
| ).toEqual(["blog", "post", "a/b"]); | ||
| }); |
There was a problem hiding this comment.
Good coverage of the happy path. Missing test cases for edge cases in resolveAppPageChildSegments:
- Optional catch-all with empty array
{ slug: [] }(should skip the segment) - Optional catch-all with
undefined(should skip the segment) - Dynamic segment
[id]withundefinedparam (should fall back to raw segment) - Route group segments like
(auth)(should be passed through unchanged)
Some of these are implicitly covered by the integration test below, but dedicated unit tests for the segment resolver would lock down the behavioral changes from || to ??.
| })); | ||
| // The error boundary is primarily a client-side component. | ||
| // | ||
| // Verified against Next.js source: |
There was a problem hiding this comment.
Good: the file header references the Next.js source paths. Per the existing PR comment from the auto-reviewer, the individual pathname-reset test cases (lines 184-245) should ideally include // Ported from Next.js: comments linking to the specific test file that validates this behavior in Next.js, if one exists.
If Next.js doesn't have an explicit test for pathname-based error boundary reset (they may handle it differently via the router), that's worth documenting too — e.g., // No direct Next.js test equivalent; behavior inferred from packages/next/src/client/components/error-boundary.tsx.
|
Review posted successfully. Here's a summary of the findings: The refactoring is well-done overall. The extraction from the generated entry template into Key findings in the review (8 inline comments):
|
- Fix optional catch-all length check readability (> len-1 → >= len) - Add dedicated tests for behavioral changes: strict undefined check on optional catch-all params, nullish coalescing preserving empty strings instead of falling back to raw segment syntax - Add edge case tests: route groups, empty array, undefined dynamic params - Add Next.js source reference comments on pathname-reset error boundary tests (no direct Next.js test equivalent exists)
There was a problem hiding this comment.
Pull request overview
Refactors the App Router generated RSC entry so it delegates route tree wiring (layouts/templates/boundaries/segment context/parallel slots) to a typed runtime module, and updates the client ErrorBoundary shim to reset on pathname changes.
Changes:
- Extracted page route wiring logic into
packages/vinext/src/server/app-page-route-wiring.tsxand updated the generated entry to call it. - Updated
ErrorBoundaryshim to reset error state whenusePathname()changes, and added focused unit tests. - Added unit tests for the extracted route-wiring helpers and updated entry template snapshots.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/error-boundary.test.ts | Adds regression/behavior tests for pathname-based ErrorBoundary resets and digest classification. |
| tests/app-page-route-wiring.test.ts | New unit tests covering segment resolution, tree paths/layout entries, and end-to-end wiring composition. |
| tests/snapshots/entry-templates.test.ts.snap | Updates snapshots to reflect thinner generated entry delegating to the new runtime helper. |
| packages/vinext/src/shims/error-boundary.tsx | Implements pathname-based reset for ErrorBoundary via an inner class + hook wrapper. |
| packages/vinext/src/server/app-page-route-wiring.tsx | New typed runtime module implementing the extracted wiring logic. |
| packages/vinext/src/entries/app-rsc-entry.ts | Updates generator to import and call the extracted runtime wiring helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (const segment of rawSegments) { | ||
| if ( | ||
| segment.startsWith("[[...") && | ||
| segment.endsWith("]]") && | ||
| segment.length >= "[[...x]]".length | ||
| ) { | ||
| const paramName = segment.slice(5, -2); | ||
| const paramValue = params[paramName]; | ||
| if (Array.isArray(paramValue) && paramValue.length === 0) { | ||
| continue; | ||
| } | ||
| if (paramValue === undefined) { | ||
| continue; | ||
| } | ||
| resolvedSegments.push(Array.isArray(paramValue) ? paramValue.join("/") : paramValue); | ||
| continue; | ||
| } | ||
|
|
||
| if (segment.startsWith("[...") && segment.endsWith("]")) { | ||
| const paramName = segment.slice(4, -1); | ||
| const paramValue = params[paramName]; | ||
| if (Array.isArray(paramValue)) { | ||
| resolvedSegments.push(paramValue.join("/")); | ||
| continue; | ||
| } | ||
| resolvedSegments.push(paramValue ?? segment); | ||
| continue; | ||
| } | ||
|
|
||
| if (segment.startsWith("[") && segment.endsWith("]") && !segment.includes(".")) { | ||
| const paramName = segment.slice(1, -1); | ||
| const paramValue = params[paramName]; | ||
| resolvedSegments.push( | ||
| Array.isArray(paramValue) ? paramValue.join("/") : (paramValue ?? segment), | ||
| ); | ||
| continue; |
There was a problem hiding this comment.
resolveAppPageChildSegments() treats missing params as undefined (and uses ?? fallbacks), but AppPageParams is imported as Record<string, string | string[]>, so params[paramName] is typed as always-present/non-nullish. In strict TS this makes checks like paramValue === undefined and expressions like paramValue ?? segment unreachable/ill-typed. Consider widening the param value type for indexed access (e.g., allow undefined when a key is absent) or updating AppPageParams to include undefined so this helper typechecks and matches runtime behavior.
| type ErrorBoundaryInnerConstructor = { | ||
| getDerivedStateFromError(error: Error): { | ||
| error: Error | null; | ||
| previousPathname: string; | ||
| }; | ||
| getDerivedStateFromProps( | ||
| props: { | ||
| children: React.ReactNode; | ||
| fallback: React.ComponentType<{ error: Error; reset: () => void }>; | ||
| pathname: string; | ||
| }, | ||
| state: { | ||
| error: Error | null; | ||
| previousPathname: string; | ||
| }, | ||
| ): { | ||
| error: Error | null; | ||
| previousPathname: string; | ||
| } | null; | ||
| }; |
There was a problem hiding this comment.
The local ErrorBoundaryInnerConstructor type says getDerivedStateFromError() returns a full { error, previousPathname } state object, but the shim’s ErrorBoundaryInner.getDerivedStateFromError() now returns a partial state (it intentionally preserves previousPathname). Updating the test’s constructor type to match the actual signature would prevent type drift and better reflect the behavior under test.
|
/bigbonk review |
There was a problem hiding this comment.
Review: Route wiring extraction + ErrorBoundary pathname reset
The extraction is well-structured and the generated entry is significantly cleaner. The type system around AppPageRouteWiringRoute, AppPageSlotOverride, etc. is thorough and will make PR 2c much easier to iterate on. Two issues worth fixing, a few minor items.
Summary
Must fix:
- The test's
ErrorBoundaryInnerConstructortype claimsgetDerivedStateFromErrorreturns a fullErrorBoundaryState, but the actual implementation now returnsPartial<ErrorBoundaryState>. This type drift masks the exact behavior the test is validating. getDerivedStateFromPropsunconditionally updatespreviousPathnameeven when the pathname hasn't changed and there's no error — this is fine for correctness but the early-outnullreturn would be more idiomatic. More importantly, line 39 has a subtle condition ordering issue: it only clears the error when both pathname changed AND error exists, but it always updatespreviousPathnameon line 42. If the pathname changes without an error, thepreviousPathnameis silently updated on line 42 through the else branch, which is correct but the condition on line 39 could be simplified.
Should fix:
3. The resolveAppPageChildSegments null passthrough for optional catch-all (strict === undefined vs old == null) is tested but the test asserts ["docs", null] which means null leaks into the segment array. Consider whether downstream consumers of segment arrays (e.g. useSelectedLayoutSegments()) handle null entries correctly.
Nits / observations (no action needed):
4. The routeThenableParams memoization outside the loop (line 235) is a nice improvement over the old per-iteration makeThenableParams(params) calls.
5. Template params correctly receive raw matchedParams (not thenable), matching the old behavior.
tests/error-boundary.test.ts
Outdated
| getDerivedStateFromError(error: Error): { | ||
| error: Error | null; | ||
| previousPathname: string; | ||
| }; |
There was a problem hiding this comment.
The actual getDerivedStateFromError now returns Partial<ErrorBoundaryState> (omitting previousPathname), but this type says it returns a full { error, previousPathname }. This matters because the test at line 232-234 simulates React's state merge with { ...baseState, ...getDerivedStateFromError(error) } — if the test type matched reality, TypeScript would flag that previousPathname is optional in the spread result, making the merge semantics explicit.
| }; | |
| getDerivedStateFromError(error: Error): Partial<{ | |
| error: Error | null; | |
| previousPathname: string; | |
| }>; |
| if (props.pathname !== state.previousPathname && state.error) { | ||
| return { error: null, previousPathname: props.pathname }; | ||
| } | ||
| return { error: state.error, previousPathname: props.pathname }; |
There was a problem hiding this comment.
This always returns a new state object even when nothing changed (same pathname, no error). React will shallow-compare and skip the re-render, so this isn't a correctness bug, but it's worth noting that this branch unconditionally syncs previousPathname from props — meaning if the pathname changes but there's no error, the previousPathname is updated silently here. That's actually correct behavior (it needs to track the current pathname for the next error), but it might be clearer to split the two concerns:
if (props.pathname !== state.previousPathname) {
return { error: state.error ? null : state.error, previousPathname: props.pathname };
}
if (state.error) {
return state; // no change needed
}
return null; // no updateThis is a style nit — the current code is correct. Feel free to ignore.
| if (Array.isArray(paramValue) && paramValue.length === 0) { | ||
| continue; | ||
| } | ||
| if (paramValue === undefined) { |
There was a problem hiding this comment.
The old code used v == null (loose equality, catches both null and undefined), this uses === undefined (strict). The test at app-page-route-wiring.test.ts:101-110 explicitly validates this change and documents that null passes through.
However, the test asserts ["docs", null] — meaning null ends up in the segment array as a value. Downstream, resolveAppPageChildSegments results are passed to LayoutSegmentProvider's segmentMap.children, which is consumed by useSelectedLayoutSegments(). Does that hook (and its consumers) handle null entries in the segments array? If not, this is a latent bug where null params leak through to user-facing hooks.
Since AppPageParams is typed as Record<string, string | string[]> (no null), the null case shouldn't occur at runtime. But the test casts null as unknown as string to test it. If the intent is that null is impossible at runtime, consider adding an early return for null with a comment explaining why, or removing the test case.
| import { ErrorBoundary, NotFoundBoundary } from "../shims/error-boundary.js"; | ||
| import { LayoutSegmentProvider } from "../shims/layout-segment-context.js"; | ||
| import { MetadataHead, ViewportHead, type Metadata, type Viewport } from "../shims/metadata.js"; | ||
| import type { AppPageParams } from "./app-page-boundary.js"; |
There was a problem hiding this comment.
Copilot's review raised a valid point: AppPageParams is Record<string, string | string[]>, so params[paramName] is typed as always-present. The === undefined checks and ?? fallbacks are runtime-correct (indexing a Record can return undefined at runtime) but technically unreachable under strict TS.
This isn't a blocking issue — Record<string, V> returning undefined on missing keys is a well-known TS gap that noUncheckedIndexedAccess addresses. But if this repo has that compiler flag enabled, these checks would be properly typed. If not, a small type adjustment like params: Record<string, string | string[] | undefined> on the function signature (not on the exported type) would make the intent explicit without affecting the public API.
| continue; | ||
| } | ||
| const TemplateComponent = templateComponent; | ||
| element = <TemplateComponent params={options.matchedParams}>{element}</TemplateComponent>; |
There was a problem hiding this comment.
Templates receive raw matchedParams (not thenable), matching the old { children: element, params } behavior. Correct.
However, the old code passed both children and params as named props via createElement(TemplateComponent, { children: element, params }). The JSX here passes children via JSX children syntax and params as a prop. The result is identical in React, but worth noting for anyone comparing the generated output.
|
Review posted successfully. Here's a summary of the findings: The refactoring is solid overall. The extraction from the generated entry template into Key findings (5 inline comments):
|
- Widen resolveAppPageChildSegments params to include undefined, matching runtime behavior of Record indexing without noUncheckedIndexedAccess - Fix ErrorBoundaryInnerConstructor test type to match actual Partial<ErrorBoundaryState> return from getDerivedStateFromError - Remove null param test case (impossible at runtime given AppPageParams type, testing unreachable behavior) - Remove vestigial type cast on empty-string test
|
/bigbonk review |
There was a problem hiding this comment.
Review: route wiring extraction + ErrorBoundary pathname reset
The refactoring is structurally sound and well-motivated. The generated entry is dramatically thinner, the new runtime module is properly typed and unit-testable, and the ErrorBoundary pathname reset matches Next.js's ErrorBoundaryHandler almost line-for-line. The previous two bot reviews covered the mechanical details thoroughly; this review focuses on a few remaining items.
ErrorBoundary: verified against Next.js
I fetched the current Next.js error-boundary.tsx from canary. The vinext implementation faithfully replicates the critical behavior:
getDerivedStateFromErrorreturns{ error }(partial), preservingpreviousPathnamevia React's state mergegetDerivedStateFromPropsclears error only whenpathname !== previousPathname && state.error- The unconditional
previousPathnamesync on line 42 matches Next.js exactly
The one difference is that Next.js uses useUntrackedPathname() (which skips router tracking) while vinext uses usePathname(). This is fine since vinext doesn't have the same router tracking infrastructure, but worth noting as a future divergence point if tracking semantics are added.
Remaining items
Two small issues and one observation below. The behavioral changes (== null → === undefined, || → ??) have been thoroughly discussed in prior reviews and the tests now cover the important cases. The AppPageParams typing issue (indexed access returning undefined at runtime but not in the type) is a pre-existing gap — not introduced by this PR.
Overall this is good work that sets up PR 2c well.
| if (props.pathname !== state.previousPathname && state.error) { | ||
| return { error: null, previousPathname: props.pathname }; | ||
| } | ||
| return { error: state.error, previousPathname: props.pathname }; |
There was a problem hiding this comment.
This unconditional previousPathname sync (even when pathname hasn't changed) is correct and matches Next.js's getDerivedStateFromProps exactly — their implementation at error-boundary.tsx L97-L101 has the identical pattern.
One difference: Next.js has an additional handleHardNavError guard inside getDerivedStateFromProps (gated behind process.env.__NEXT_APP_NAV_FAIL_HANDLING) that clears the error and triggers a hard navigation for certain transient errors. This isn't needed today but may become relevant if vinext adds hard-nav recovery.
| export function resolveAppPageChildSegments( | ||
| routeSegments: readonly string[], | ||
| treePosition: number, | ||
| params: Readonly<Record<string, string | string[] | undefined>>, |
There was a problem hiding this comment.
The function signature correctly widens the param value type to include undefined, which is the right call — it matches runtime behavior even though AppPageParams doesn't include undefined. This local widening is better than changing the exported type.
However, there's an asymmetry: the string in the union means a non-array string param like { slug: "hello" } flows into the optional catch-all branch at line 154 as-is (pushing the raw string). This matches the old code, but is it actually correct for optional catch-all? If params.slug is "hello" (a string, not an array), should that be treated as a single-segment catch-all value or should it be skipped? The old code had the same behavior, so this isn't a regression — just worth thinking about for correctness.
| continue; | ||
| } | ||
| const TemplateComponent = templateComponent; | ||
| element = <TemplateComponent params={options.matchedParams}>{element}</TemplateComponent>; |
There was a problem hiding this comment.
Templates receive raw matchedParams (not thenable). This matches the old code which passed params directly:
element = createElement(TemplateComponent, { children: element, params });But layouts at line 258 receive routeThenableParams (thenable). This asymmetry is intentional and matches the old code, but it's worth a brief comment explaining why templates get raw params while layouts get thenable params — otherwise it looks like an oversight to future contributors.
| expect(state).toMatchObject({ error: e }); | ||
| }); | ||
|
|
||
| // No direct Next.js test equivalent; behavior inferred from |
There was a problem hiding this comment.
Good. These comments adequately address the earlier review feedback about documenting the Next.js verification. The comment correctly notes that Next.js's E2E suite (test/e2e/app-dir/errors/index.test.ts) only tests the button-click reset path, not pathname-based reset, and that the behavior is inferred from their source at getDerivedStateFromProps.
I verified this against the current Next.js canary source — the implementation matches.
| expect(resolveAppPageChildSegments(["blog", "[id]"], 0, {})).toEqual(["blog", "[id]"]); | ||
| }); | ||
|
|
||
| it("preserves empty-string param instead of falling back to raw segment", () => { |
There was a problem hiding this comment.
Good addition covering the || → ?? behavioral change. This test locks down that empty-string params are preserved rather than falling back to raw segment syntax.
Note: the input { slug: "" } is typed as string but the catch-all branch at app-page-route-wiring.tsx:165 does paramValue ?? segment — since "" is not nullish, it pushes the empty string. The old v2 || seg would have pushed the raw [...slug] segment. This is a correctness improvement.
|
Review posted successfully. Here's a summary of the findings: The refactoring is solid. The extraction from the generated entry template into Key findings (5 inline comments):
|
Templates receive raw params (Next.js doesn't pass params to templates at all), while layouts receive thenable params matching the Next.js 15+ async params contract.
|
/bigbonk review |
There was a problem hiding this comment.
Review: route wiring extraction + ErrorBoundary pathname reset
This is a well-executed refactor that delivers on the "thin entries" guideline. The generated entry goes from ~250 lines of inline wiring logic to a clean delegation call, the new runtime module is thoroughly typed, and the ErrorBoundary pathname-reset matches the Next.js canary implementation faithfully.
Three prior bot reviews have covered this thoroughly. I have one issue to flag and a few observations.
One issue
Template params comment is inaccurate. The comment at line 230 says "Next.js doesn't pass params to templates at all" but this isn't clearly sourced and the old vinext code did pass params to templates. If this comment is wrong, it could mislead future contributors into removing the param passing. If it's correct, it should link to the Next.js source that demonstrates it. Either way, it's not blocking — the code does the right thing by preserving the existing behavior.
Observations
-
The extraction is mechanically faithful. I traced the old inline code against the new typed module and the logic matches, with two intentional improvements (
== null→=== undefined,||→??) that are both correctness fixes covered by tests. -
ErrorBoundary implementation is correct. The
getDerivedStateFromError→getDerivedStateFromPropsinteraction withPartial<ErrorBoundaryState>is the right approach. React merges partial state, preservingpreviousPathname, sogetDerivedStateFromPropsdoesn't immediately clear a freshly caught error. The test at line 221 validates exactly this sequence. -
Type compatibility at the
resolveChildSegmentsboundary is clean.resolveAppPageChildSegmentsacceptsRecord<string, string | string[] | undefined>while the existing call sites inapp-page-boundary-render.tsandapp-page-boundary.tspassAppPageParams(Record<string, string | string[]>). This works correctly because the function parameter is contravariant — a function that accepts a wider type can be used where a narrower type is expected. -
The
makeThenableParamsmemoization (called once at line 238 instead of per-layout-iteration) is a nice improvement. Since all layouts in a single route share the same params, there's no reason to create N identical Promise objects.
Overall this is solid work that sets up PR 2c well. The inline comments below are minor.
| const TemplateComponent = templateComponent; | ||
| // Next.js doesn't pass params to templates at all (createElement(Template, null, children)). | ||
| // We pass raw params here for convenience; layouts below receive thenable (Promise) params | ||
| // to match the Next.js 15+ async params contract. |
There was a problem hiding this comment.
Nit: this comment claims "Next.js doesn't pass params to templates at all" but I couldn't verify this against the Next.js source. In packages/next/src/client/components/layout-router.tsx, Next.js renders templates with React.createElement(template, segmentChild) where segmentChild contains childProp — params are passed indirectly through the tree rather than as explicit props.
The old vinext code passed params to templates, and this code preserves that behavior, which is fine. But the comment could mislead a future contributor into removing the param passing. Consider either:
- Linking to the specific Next.js source that shows templates don't receive params as props, or
- Softening the comment to something like: "Templates receive raw params (not thenable) for convenience. Next.js doesn't explicitly pass params as a prop to templates, but we do for backward compat."
| if (paramValue === undefined) { | ||
| continue; | ||
| } | ||
| resolvedSegments.push(Array.isArray(paramValue) ? paramValue.join("/") : paramValue); |
There was a problem hiding this comment.
Good improvement: the old == null loose check treated both null and undefined as "skip", but the AppPageParams type (Record<string, string | string[]>) doesn't include null, so null params shouldn't occur at runtime. The strict === undefined is more correct for the type system.
The test at app-page-route-wiring.test.ts:93-98 covers both the undefined (implicit missing key) and empty array cases. Worth noting for anyone reading the diff: this is an intentional behavioral change, not an oversight.
| }; | ||
|
|
||
| for (const [slotName, slot] of Object.entries(routeSlots)) { | ||
| const targetIndex = slot.layoutIndex >= 0 ? slot.layoutIndex : layoutEntries.length - 1; |
There was a problem hiding this comment.
Subtle behavior: when slot.layoutIndex is -1 (meaning "attach to innermost layout"), this uses layoutEntries.length - 1. The old code used route.layouts.length - 1. These are equivalent because layoutEntries is built from route.layouts with a 1:1 mapping, but it's worth noting the indirection changed — if createAppPageLayoutEntries ever filters or transforms the layout list, this assumption would break.
| error: null, | ||
| previousPathname: "/error-test", | ||
| }; | ||
| const stateAfterError = { |
There was a problem hiding this comment.
This test correctly simulates React's state merge semantics: getDerivedStateFromError returns { error } (partial), and the spread { ...baseState, ...result } mirrors how React merges partial state into the existing state object, preserving previousPathname: "/error-test". Then getDerivedStateFromProps sees matching pathnames and preserves the error.
This is the critical regression test for the Partial<ErrorBoundaryState> return type change. Good coverage.
| }); | ||
| } | ||
| layoutProps[slotName] = slotElement; | ||
| return __buildAppPageRouteElement({ |
There was a problem hiding this comment.
Clean delegation. The options bag makes it very clear what data flows into the wiring logic, and the generated code is now ~15 lines instead of ~200. This is exactly what the "thin entries" guideline calls for.
One thing I noticed: the snapshot at entry-templates.test.ts.snap line 750 shows globalErrorModule: null for the basic fixture (no global-error.tsx), while this source template correctly interpolates the globalErrorVar. The snapshot correctly reflects the fixture state — just confirming the template conditional works both ways.
|
Review posted successfully. Here's a summary: The refactoring is solid. The extraction from the generated entry template into Key findings (5 inline comments):
|
|
@james-elicx The comment is correct I think |
Summary
Part of #726 (PR 2b). Moves boundary/layout/segment wiring logic out of the generated entry template (
app-rsc-entry.ts) into a typed runtime module (server/app-page-route-wiring.tsx). This follows the project's architectural guideline that generated entries should stay thin: codegen glue, not runtime logic.What moved
The generated
buildPageElement()function inapp-rsc-entry.tspreviously contained ~250 lines of inline logic for:LayoutSegmentProviderwith computedsegmentMapat each layout levelErrorBoundary,NotFoundBoundary, andSuspense(loading) at correct tree positionsAll of this is now in
packages/vinext/src/server/app-page-route-wiring.tsxas typed, unit-testable functions:buildAppPageRouteElement()createAppPageLayoutEntries()resolveAppPageChildSegments()[id], catch-all[...slug], optional[[...slug]])createAppPageTreePath()The generated entry now imports and calls
__buildAppPageRouteElement()instead of inlining the logic. Template snapshots updated accordingly, and the generated code is much shorter.ErrorBoundary pathname-based reset
ErrorBoundarynow resets error state on pathname changes, matching the existing pattern inNotFoundBoundary. This was identified as a required fix in #726 because the boundary instance persists at the same tree position across client navigations.Implementation details:
ErrorBoundaryis now a function wrapper that readsusePathname()and passes it toErrorBoundaryInner.getDerivedStateFromProps()clears the caught error when the pathname changes.getDerivedStateFromError()preserves the existingpreviousPathnameinstead of overwriting it, which avoids immediately clearing freshly caught client errors beforeerror.tsxcan render.Important scope note: this reset is pathname-based only. It does not reset on search-param-only navigations because
usePathname()does not include the query string.Why this matters for layout persistence
PR 2c ("The Switch") needs to restructure how the route wiring is built from a single nested tree to flat map entries with
Slotreferences. Having the wiring logic in a typed module instead of embedded in a template string means PR 2c can modifybuildAppPageRouteElement()directly with type safety, IDE support, and focused unit tests.Test plan
tests/app-page-route-wiring.test.ts— unit coverage for the extracted helpers (segment resolution, layout entries, tree paths)tests/error-boundary.test.ts— pathname reset coverage, including a regression test that caught the immediate-clear bug in the client error boundary state machinetests/entry-templates.test.ts— snapshot updated to verify generated entry delegates to the extracted helpervp test run tests/error-boundary.test.ts tests/app-page-route-wiring.test.ts tests/entry-templates.test.ts vp check packages/vinext/src/entries/app-rsc-entry.ts packages/vinext/src/server/app-page-route-wiring.tsx packages/vinext/src/shims/error-boundary.tsx tests/error-boundary.test.ts vp run test:e2e -- tests/e2e/app-router/error-handling.spec.ts tests/e2e/app-router/error-interactive.spec.ts tests/e2e/app-router/nextjs-compat/error-nav.spec.ts tests/e2e/app-router/navigation-flows.spec.ts --project app-routerRefs #726