refactor: extract app page probe runtime#635
Conversation
|
/bigbonk review |
southpolesteve
force-pushed
the
codex/app-page-probe-runtime
branch
from
e5b7aa2 to
1d33813
Compare
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The extraction of boundary resolution logic and probe orchestration into app-page-boundary.ts and app-page-probe.ts is a clean refactoring that reduces duplication and improves testability. The new helper functions are well-typed and the test coverage is solid.
However, there is one blocking bug that will cause a runtime crash, plus a few minor observations.
Bug: handleRenderError is called but never defined
The old code defined a handleRenderError(err) inline helper inside _handleRequest() that resolved special errors (redirect/notFound/forbidden) during SSR. This PR correctly removed the function definition (it was part of the old inline probe logic), and correctly removed the call from the page probe path. However, the SSR error catch block still calls handleRenderError at what is now line 2729:
} catch (ssrErr) {
const specialResponse = await handleRenderError(ssrErr); // ← undefined!This will throw a ReferenceError: handleRenderError is not defined at runtime whenever an SSR render error occurs (e.g., a notFound() or redirect() thrown inside a Suspense boundary during HTML rendering). The fix is straightforward — inline the same pattern used elsewhere:
} catch (ssrErr) {
const __ssrSpecialError = __resolveAppPageSpecialError(ssrErr);
const specialResponse = __ssrSpecialError
? await __buildAppPageSpecialErrorResponse({
clearRequestContext() {
setHeadersContext(null);
setNavigationContext(null);
},
renderFallbackPage(statusCode) {
return renderHTTPAccessFallbackPage(route, statusCode, isRscRequest, request, {
matchedParams: params,
});
},
requestUrl: request.url,
specialError: __ssrSpecialError,
})
: null;Minor observations
-
Test quality is good. The new tests in
app-page-probe.test.tsandapp-page-boundary.test.tscover the key paths: layout errors before page probe, special vs non-special error routing, loading boundary skipping async awaits, HTTP access boundary component resolution order, error boundary resolution precedence, layout wrapping with/without RSC, and RSC vs HTML response routing. These are meaningful unit tests that exercise the extracted logic. -
Behavioral parity for HTML boundary responses. The old code had separate RSC and HTML wrapping paths — RSC got
LayoutSegmentProvider+GlobalErrorBoundarywrappers while HTML got plain layout wrapping only. The newwrapAppPageBoundaryElementcorrectly gates these additional wrappers onisRscRequest, preserving the behavioral difference. This is correct. -
Snapshot changes are mechanical. The
entry-templates.test.ts.snapdiff is large but appears to be the expected result of the code restructuring — the generated code now uses the extracted helper imports and call sites instead of inline logic.
|
The line number doesn't match because the diff view has different line numbers. The review body already clearly describes the issue. The review has been posted successfully. Review posted on PR #635 with a request for changes. Key finding: There is a blocking bug. The Otherwise the refactoring is clean:
|
|
Addressed the blocking bug in What changed:
Reran locally:
/bigbonk review |
There was a problem hiding this comment.
The previous review flagged a blocking bug with handleRenderError being called but not defined at line 2729. This is incorrect — the PR correctly handles all three former call sites of handleRenderError:
- Layout probe path: Delegated to
__probeAppPageBeforeRendervia therenderLayoutSpecialErrorcallback (lines 2564-2593) - Page probe path: Delegated to
__probeAppPageBeforeRendervia therenderPageSpecialErrorcallback (lines 2595-2608) - SSR error catch block: Inlined at lines 2729-2744 using
__resolveAppPageSpecialError+__buildAppPageSpecialErrorResponsedirectly
The test at tests/app-router.test.ts:3714-3718 explicitly verifies handleRenderError no longer appears in the generated code, and all CI checks pass (Vitest unit + integration, all 7 Playwright E2E projects, deploy previews, and smoke tests).
The refactoring is clean:
app-page-probe.tscorrectly orchestrates layout probes before page probes (matching Next.js execution order), delegating to the existingprobeAppPageLayoutsandprobeAppPageComponentfromapp-page-execution.ts- The
ProbeAppPageBeforeRenderOptionsinterface is well-typed with clear callback contracts — this makes the probe orchestration testable independently of the generated entry code - The
hasLoadingBoundarygate correctly skips awaiting async page probes when a Suspense boundary exists (preserving streaming behavior) - Test coverage in
app-page-probe.test.tscovers the important paths: layout errors preempting page probes, non-special layout errors falling through, special page errors producing immediate responses, and loading boundary skipping async awaits - Snapshot changes in
entry-templates.test.ts.snapare mechanical results of the restructuring
|
Review posted and PR approved. Here's a summary of my findings: The previous review's blocking bug report was a false positive. The
The test at The refactoring itself is clean — it extracts the layout+page probe orchestration into a well-typed |
1 participant
Summary
app-page-probehelperapp-rsc-entry.tsfocused on route-specific wiring while delegating layout/page probe handling to the helperTesting
Written by Codex.