test: expand Next.js compat coverage and fix parity bugs#578
test: expand Next.js compat coverage and fix parity bugs#578dmmulroy wants to merge 23 commits intocloudflare:mainfrom
Conversation
Manifest of 379 Next.js test/e2e/app-dir directories, prioritized: - P1: 26 error/validation directories - P2: 76 edge case directories - P3: 140 core feature directories - P4: 103 other directories - Already covered: 14, skip: 21 Ref: cloudflare#204
…autoresearch.sh ANSI stripping.\n\nResult: {"status":"keep","passing_compat_tests":233,"test_files":21,"dirs_covered":13,"skipped_tests":2}
…tch errors in generateMetadata and page component\n\nResult: {"status":"keep","passing_compat_tests":236,"test_files":22,"dirs_covered":14,"skipped_tests":2}
…+ root escalation. Also triaged 6 P1 dirs as skip (Playwright/CLI/build-specific).\n\nResult: {"status":"keep","passing_compat_tests":241,"test_files":23,"dirs_covered":15,"skipped_tests":2}
…coped + root escalation. Skip proxy-missing-export (covered by PR cloudflare#203).\n\nResult: {"status":"keep","passing_compat_tests":246,"test_files":24,"dirs_covered":16,"skipped_tests":2}
…h-triage 14 more P1+P2 dirs as skip. 249 total tests.\n\nResult: {"status":"keep","passing_compat_tests":249,"test_files":25,"dirs_covered":17,"skipped_tests":2}
…nstead of NextRequest (.nextUrl undefined). Wrapped in NextRequest in app-rsc-entry.ts.\n\nResult: {"status":"keep","passing_compat_tests":251,"test_files":26,"dirs_covered":18,"skipped_tests":2}
…d RSC redirect behavioral difference (307 vs 200+stream). Triage 4 more dirs.\n\nResult: {"status":"keep","passing_compat_tests":254,"test_files":27,"dirs_covered":19,"skipped_tests":2}
…tRedirect status codes + client component redirect SSR. Triage 5 more dirs.\n\nResult: {"status":"keep","passing_compat_tests":259,"test_files":28,"dirs_covered":20,"skipped_tests":2}
…rams instead of scoped per-segment params. Fixed 4 rendering loops in app-rsc-entry.ts + buildPageElement. Added cheerio + fetchDom helper.\n\nResult: {"status":"keep","passing_compat_tests":265,"test_files":29,"dirs_covered":21,"skipped_tests":2}
…lternates/images/videos), manifest.webmanifest, icon.tsx file convention routes.\n\nResult: {"status":"keep","passing_compat_tests":275,"test_files":30,"dirs_covered":22,"skipped_tests":2}
…client components, passthrough from server→client, and no-use pages. Triage 2 more dirs.\n\nResult: {"status":"keep","passing_compat_tests":280,"test_files":31,"dirs_covered":23,"skipped_tests":2}
… conflicting-search-and-route-params (2 tests: same-name route vs search param). Triage 3 more dirs.\n\nResult: {"status":"keep","passing_compat_tests":285,"test_files":33,"dirs_covered":25,"skipped_tests":2}
…re. Added APP_UNDERSCORED_ROOT_FIXTURE_DIR and a minimal fixture to verify private underscore folders are hidden while %5F-decoded folders remain routable.\n\nResult: {"status":"keep","passing_compat_tests":288,"test_files":34,"dirs_covered":26,"skipped_tests":2}
…ndler-only fixture. Verified function-level "use cache" in route handlers and revalidatePath() invalidation without any pages present.\n\nResult: {"status":"keep","passing_compat_tests":290,"test_files":35,"dirs_covered":27,"skipped_tests":2}
…, force-static, and client-page request API usage. BUG FOUND+FIXED: force-static pages leaked real searchParams instead of empty object.\n\nResult: {"status":"keep","passing_compat_tests":294,"test_files":36,"dirs_covered":28,"skipped_tests":2}
…rects in next.config (itms-apps with and without //).\n\nResult: {"status":"keep","passing_compat_tests":296,"test_files":37,"dirs_covered":29,"skipped_tests":2}
…stead of grepping for 'passed', preventing false positives when some compat tests fail. Metric unchanged but benchmark correctness improved.\n\nResult: {"status":"keep","passing_compat_tests":296,"test_files":37,"dirs_covered":29,"skipped_tests":2}
… fixture. Verified route handlers redirect to slash-canonical URLs and preserve the slash in both url.pathname and req.nextUrl.pathname.\n\nResult: {"status":"keep","passing_compat_tests":298,"test_files":38,"dirs_covered":30,"skipped_tests":2}
…. Fixed a real runtime gap: default 404 pages now fall back to next/error and stay wrapped in root/ancestor layouts when no explicit not-found.tsx exists.\n\nResult: {"status":"keep","passing_compat_tests":301,"test_files":39,"dirs_covered":31,"skipped_tests":2}
…d hybrid app+pages fixture. Fixed real runtime gaps: middleware now has a next/headers context across dev/app/prod execution paths, and draftMode() cookies set in middleware are captured before the ALS scope unwinds. Updated generated-entry snapshots accordingly.\n\nResult: {"status":"keep","passing_compat_tests":312,"test_files":40,"dirs_covered":31,"skipped_tests":2}
commit: |
|
/big-bonk review |
|
@dmmulroy just curious, did you use the hyped Karpathy's autoresearch for this? |
@NathanDrake2406 yeah! well more specifically I used https://github.com/davebcn87/pi-autoresearch |
Damn that's really cool man, thanks. let me have a look. Output looks good also! |
|
/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: test: expand Next.js compat coverage and fix parity bugs
This is excellent work. The autoresearch loop produced real, testable bugs and the fixes are correct and well-placed. I reviewed all five runtime fixes, checked dev/prod parity across the entry points, verified the __scopeParamsForLayout loop bounds, and read through the 40+ new test files. Here's my detailed review.
Runtime fixes — all look correct
1. Layout params scoping (__scopeParamsForLayout): The loop bound for (i = 0; i < treePosition; i++) is correct. treePosition is the directory depth (not the segment index), so looping to treePosition - 1 includes the layout's own dynamic segment. Verified against the computeLayoutTreePositions implementation in routing/app-router.ts. The fix is applied consistently across all 5 layout-wrapping loops (RSC+HTML paths in renderHTTPAccessFallbackPage, renderErrorBoundaryPage, and buildPageElement).
2. Default 404 rendering: Using DefaultHttpErrorComponent from next/error as the fallback when no not-found.tsx exists is the right approach and matches Next.js behavior. The noindex meta tag is already unconditionally added at line 813 for all fallback pages, so this is covered.
3. force-static searchParams suppression: Clean implementation — replacing spObj with {} early in the pipeline means both searchParams props and metadata resolution see empty params. One thing worth verifying: does force-static also suppress headers() and cookies() in the headers context? The fixture test accesses them, so if the tests pass that confirms it works.
4. Route handler NextRequest wrapping: Correct. The instanceof guard prevents double-wrapping.
5. Middleware headers context: The runWithHeadersContext + getDraftModeCookieHeader pattern is correctly duplicated across all three middleware execution sites. The SYNC comments with the middleware-headers-context tag are a good practice.
Dev/prod parity — covered
The App Router prod server and Cloudflare Worker entry both delegate to the built RSC entry, so they inherit the fixes automatically. The Pages Router prod server calls the generated entry's exported runMiddleware. No additional fixes needed in prod-server.ts or app-router-entry.ts.
Test coverage — thorough
41 test files covering routing, request APIs, metadata, error boundaries, streaming, middleware, caching, and more. The fetchDom/Cheerio helper is a good addition for DOM-structured assertions. Dedicated fixtures for cases that can't be expressed in the shared fixture is the right call.
Nits and observations
See inline comments below.
| // Compute the subset of params that a layout at a given tree position should receive. | ||
| // In Next.js, each layout only sees params from its own segment and ancestor segments — | ||
| // NOT from child segments deeper in the tree. For example, with | ||
| // /base/[param1]/[param2]/page.tsx, the layout at [param1]/ gets {param1} but not {param2}. |
There was a problem hiding this comment.
The function is well-structured and the loop bounds are correct. Verified: treePosition = directory depth from appDir, so for (i = 0; i < treePosition) correctly includes the layout's own dynamic segment.
Minor observation: the segment-type detection logic (optional catch-all, catch-all, dynamic) is duplicated between __scopeParamsForLayout and __resolveChildSegments. Not worth extracting now since the patterns are small and stable, but flagging it in case this grows.
| // force-static pages receive empty searchParams rather than real request data. | ||
| // This mirrors Next.js, which strips dynamic request state from force-static | ||
| // render paths instead of exposing live values. | ||
| const isForceStatic = route.page?.dynamic === "force-static"; |
There was a problem hiding this comment.
Good fix. The effectiveSpObj / effectiveHasSearchParams pattern is clean — both metadata resolution and page props consistently see the empty state for force-static pages.
| const _prevHeadersPhase = setHeadersAccessPhase("middleware"); | ||
| try { | ||
| const _middlewareResponse = await middlewareFn(nextRequest, mwFetchEvent); | ||
| _mwDraftCookie = getDraftModeCookieHeader(); |
There was a problem hiding this comment.
Nit: runWithHeadersContext always initializes phase: "render" (see headers.ts line 224), which is immediately overridden by setHeadersAccessPhase("middleware") on the next line. This works fine but the initial phase set is wasted. Very minor.
| setHeadersAccessPhase(_prevHeadersPhase); | ||
| } | ||
| }); | ||
| if (_mwDraftCookie && mwResponse) { |
There was a problem hiding this comment.
Capturing getDraftModeCookieHeader() inside the runWithHeadersContext scope (before it unwinds) is critical — good catch. The draft mode cookie would be lost if captured after the ALS scope unwinds.
| const response = await handlerFn(request, { params }); | ||
| // Wrap in NextRequest so route handlers get .nextUrl, .cookies, .geo, .ip, etc. | ||
| // Next.js passes NextRequest to route handlers, not plain Request. | ||
| const routeRequest = request instanceof NextRequest ? request : new NextRequest(request); |
There was a problem hiding this comment.
Correct fix. Note that unlike page components which receive makeThenableParams(params), route handlers receive plain params. This is actually correct — Next.js route handlers receive plain params objects, not Promise-wrapped ones.
| const fetchEvent = new NextFetchEvent({ page: normalizedPathname }); | ||
|
|
||
| // Execute the middleware | ||
| // SYNC: middleware-headers-context — this pattern is duplicated in |
There was a problem hiding this comment.
The SYNC tag middleware-headers-context is a good practice for keeping the three copies in sync. I confirmed the pattern is consistently applied across all three files (server/middleware.ts, entries/app-rsc-entry.ts, entries/pages-server-entry.ts).
| "@types/react-dom": "catalog:", | ||
| "@typescript/native-preview": "catalog:", | ||
| "@vitejs/plugin-rsc": "catalog:", | ||
| "cheerio": "^1.2.0", |
There was a problem hiding this comment.
Nit: ^1.2.0 is a wide range for a test utility. Consider pinning to 1.2.0 to avoid surprise breakage from semver-minor bumps, though this is very low risk for a test-only dep.
|
Review posted successfully. Here's a summary of what I found: Review Summary for PR #578Overall verdict: The PR is solid. All 5 runtime fixes are correct, dev/prod parity is maintained, and the 41 new test files provide substantial Next.js compat coverage. Runtime fixes verified:
Dev/prod parity: No gaps. Prod server and Worker entry delegate to the code-generated entries that already contain the fixes. Inline comments posted: 7 comments covering nits (shadowed variable, wasted initial phase, cheerio version range) and confirmations of correctness on the key changes. |
|
it's quite insane how it's a 3k diff and it's approved on the first pass. @dmmulroy |
please do not credit anything but the idea to try autoresearch to me here haha. I helped steer it a few times (pi has a great steering mechanism) and i helped iterate on the metrics/validation strategy but beyond that the credit all goes to the folks at shopify and andrej karpathy |
|
I might need to branch out from CC and try pi! |
3 participants
Summary
This PR ports a large batch of HTTP-testable Next.js App Router compatibility coverage into
tests/nextjs-compat/and fixes several real runtime parity bugs uncovered along the way.Highlights:
Included coverage
Notable compat additions in this branch include:
fetch-deduping-errorsforbiddenunauthorizedsearchParamsstatic bailout coverageuseParamsuse cachetrailingSlashnot-foundbehaviorRuntime fixes uncovered by the compat ports
NextRequestsoreq.nextUrlexistsforce-staticpages from receiving livesearchParamsnot-found.tsxexistsnext/headerscontext and capture draft-mode cookies before the ALS scope unwindsValidation
Primary validation for this branch came from the compat benchmark loop plus the automatic backpressure checks required by autoresearch mode.
Latest kept result before removing autoresearch assets from the branch:
Autoresearch findings summary
Bugs found and fixed
Requestinstead ofNextRequest, soreq.nextUrlwas undefined.force-staticpages still saw livesearchParams.not-found.tsxexisted.next/headersrequest context, and middleware draft-mode cookies could be lost after the ALS scope unwound.Behavioral differences documented
Process/tooling improvements from the loop
fetchDom/Cheerio-based assertions for more DOM-structured compat testsAutoresearch metric progression
Autoresearch notes intentionally left out of the diff
This PR intentionally excludes files such as:
autoresearch.mdautoresearch.shautoresearch.checks.shautoresearch.manifest.jsonautoresearch.jsonlautoresearch.ideas.mdThose assets were useful during the audit loop but are not needed to review or merge the runtime/test changes.
Reviewer notes
This branch is best reviewed commit-by-commit or by file grouping:
packages/vinext/src/**