refactor: extract app page boundary helpers#634
Conversation
a6dd296 to
d9556ad
Compare
commit: |
|
|
/bigbonk review please |
There was a problem hiding this comment.
Review: refactor: extract app page boundary helpers
Clean, well-structured refactoring that eliminates significant code duplication between renderHTTPAccessFallbackPage and renderErrorBoundaryPage. The four extracted helpers (resolveAppPageHttpAccessBoundaryComponent, resolveAppPageErrorBoundary, wrapAppPageBoundaryElement, renderAppPageBoundaryResponse) each have a clear single responsibility and are well-typed.
Behavioral equivalence verified
I compared the original inline code on main against the new helper-delegating code line by line:
resolveAppPageHttpAccessBoundaryComponent — The status code branching (403/401/fallback-to-not-found) and route-then-root fallback chain are identical to the original inline if/else if/else block.
resolveAppPageErrorBoundary — The page > layout (innermost-first) > global-error resolution order matches the original. The isGlobalError flag correctly replaces the old _isGlobalError variable and is used identically to gate skipLayoutWrapping.
wrapAppPageBoundaryElement — The layout wrapping loop, RSC-only LayoutSegmentProvider wrapping, and RSC-only global error boundary wrapping all match the original branching structure. The old code had separate RSC and HTML paths that both computed _fallbackParams / _fallbackParamsHtml with the same expression (opts?.matchedParams ?? route?.params ?? {}); the new code correctly unifies this into a single matchedParams field.
renderAppPageBoundaryResponse — The RSC path returns a Response with text/x-component content type and the correct status code. The HTML path delegates to createHtmlResponse. The important "do NOT clear context here" invariant is preserved via a comment in the helper, and the actual clearing still happens only inside createHtmlResponse (i.e., the HTML/SSR path).
One subtle improvement
The createHtmlResponse callback pattern defers font data creation and SSR entry loading until actually needed. In the original code, the HTML path computed these eagerly even before checking isRscRequest. The new pattern avoids unnecessary work for RSC requests.
Type design
The heavy generics (5 type params on WrapAppPageBoundaryElementOptions) are the right call here. The helpers are framework-agnostic — they don't import React or any vinext-specific types — which means they can be unit tested with plain strings (as the tests demonstrate). The getDefaultExport, renderLayout, renderErrorBoundary, and renderLayoutSegmentProvider callbacks cleanly separate the "what to do" logic (in the helper) from the "how to create elements" logic (in the generated code).
Test coverage
Good coverage of the pure helpers with string-based assertions that are easy to read. The renderAppPageBoundaryResponse tests verify both the RSC and HTML paths, including that createHtmlResponse is only called for non-RSC requests.
Minor observations (non-blocking)
- The
Varyheader key changed from"Vary"toVary(unquoted object key) — cosmetic only, both are equivalent in JS. - Architectural comments explaining why LayoutSegmentProvider and GlobalErrorBoundary are needed for RSC tree reconciliation were removed from the generated code. The brief comment in
renderAppPageBoundaryResponsecovers the context-clearing invariant. The tree reconciliation rationale might be worth preserving as a comment inwrapAppPageBoundaryElementfor future readers, but this is minor.
LGTM — all CI green, behavior preserved, good test coverage.
|
The review has been posted and the PR has been approved. Here's a summary of the review: PR #634 approved — This is a clean refactoring that extracts duplicated boundary resolution, element wrapping, and response rendering logic from Key findings:
|
Summary
app-page-boundary.tsapp-rsc-entry.tsfocused on route-specific metadata and tree wiring while delegating fallback/error boundary response orchestration to typed helpersTesting
vp check packages/vinext/src/server/app-page-boundary.ts packages/vinext/src/entries/app-rsc-entry.ts tests/app-page-boundary.test.ts tests/app-router.test.tsvp test run tests/app-page-boundary.test.ts tests/app-router.test.ts -t 'app page boundary helpers|generated code delegates page boundary rendering'vp test run tests/app-page-boundary.test.ts tests/app-page-request.test.ts tests/app-page-stream.test.ts tests/app-page-execution.test.ts tests/app-page-cache.test.ts tests/app-page-response.test.ts tests/error-boundary.test.tsvp test run tests/entry-templates.test.ts -uvp test run tests/app-router.test.ts -t 'notFound\(\) inside Suspense boundary preserves digest for not-found UI|renders intercepted photo modal on RSC navigation from feed|renders custom not-found.tsx for unmatched routes|notFound\(\) escalates to nearest ancestor not-found.tsx|forbidden\(\) from Server Component returns 403 with forbidden.tsx|unauthorized\(\) from Server Component returns 401 with unauthorized.tsx|renders error boundary wrapper for routes with error.tsx|generated code delegates page boundary rendering to typed helpers|generated code|dynamicParams = false|page ISR \+ searchParams'vp run vinext#buildWritten by Codex.