Fix sibling intercepted App Router routes#867
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
Fixes App Router sibling intercepted-route navigation when the source route is a layout-owned, slot-only route (no page.tsx), aligning vinext behavior with Next.js’ intercepting-parallel-modal patterns and preserving source-slot params across sibling navigations.
Changes:
- Extend App Router route discovery to include layout segments whose visible content is provided entirely by parallel slot pages (even without a segment
page.tsx). - Update the App RSC entry template to (a) allow slot-only routes to render without “Page has no default export” and (b) merge source interception-context params with target URL params.
- Add unit + e2e regression fixtures/tests for sibling
(..)interception, top→film modal interception, and the@feed+@modal/(..)photo/[id]reference shape.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/vinext/src/routing/app-router.ts | Adds discovery of “slot-only” layout routes and refactors route construction to support null pagePath. |
| packages/vinext/src/entries/app-rsc-entry.ts | Updates intercept param resolution and permits rendering routes with no page module (slot-only routes). |
| tests/routing.test.ts | Adds a unit test asserting discovery of a layout route whose renderable content is parallel-slot pages. |
| tests/e2e/app-router/advanced.spec.ts | Adds e2e coverage for sibling (..) interception scenarios and param preservation with parallel slots. |
| tests/snapshots/entry-templates.test.ts.snap | Updates snapshot for the entry template changes (intercept param merging + slot-only route rendering). |
| tests/fixtures/app-basic/app/top/page.tsx | Adds a “top” listing page used as an interception source. |
| tests/fixtures/app-basic/app/top/layout.tsx | Adds layout with a modal slot for the top-level sibling interception fixture. |
| tests/fixtures/app-basic/app/top/@modal/default.tsx | Adds default content for the @modal slot. |
| tests/fixtures/app-basic/app/top/@modal/(..)film/[imdbId]/page.tsx | Adds an intercepted modal page for /film/[imdbId] from /top. |
| tests/fixtures/app-basic/app/film/[imdbId]/page.tsx | Adds the direct (non-intercepted) film detail page for assertions. |
| tests/fixtures/app-basic/app/sibling-source/page.tsx | Adds a basic sibling interception source page. |
| tests/fixtures/app-basic/app/sibling-source/layout.tsx | Adds layout with a modal slot for sibling-source fixture. |
| tests/fixtures/app-basic/app/sibling-source/@modal/default.tsx | Adds default content for the sibling-source modal slot. |
| tests/fixtures/app-basic/app/sibling-source/@modal/(..)sibling-target/[id]/page.tsx | Adds the intercepted modal page that targets the sibling route. |
| tests/fixtures/app-basic/app/sibling-target/[id]/page.tsx | Adds the direct sibling target page (should stay hidden during interception). |
| tests/fixtures/app-basic/app/parallel-sibling-modal/photo/[id]/page.tsx | Adds the direct photo page for the parallel-slot sibling-modal fixture. |
| tests/fixtures/app-basic/app/parallel-sibling-modal/[username]/layout.tsx | Adds a layout that renders @feed and @modal slots (slot-only route shape). |
| tests/fixtures/app-basic/app/parallel-sibling-modal/[username]/@feed/page.tsx | Adds feed slot page that provides visible source content + link to sibling target. |
| tests/fixtures/app-basic/app/parallel-sibling-modal/[username]/@feed/default.tsx | Adds feed slot default (calls notFound()) to model Next.js reference behavior. |
| tests/fixtures/app-basic/app/parallel-sibling-modal/[username]/@modal/default.tsx | Adds modal slot default content. |
| tests/fixtures/app-basic/app/parallel-sibling-modal/[username]/@modal/(..)photo/[id]/page.tsx | Adds intercepted modal page for /photo/[id] from the username route. |
Comments suppressed due to low confidence (1)
packages/vinext/src/entries/app-rsc-entry.ts:2209
- This change allows routes with no page module (slot-only layout routes) to proceed past the default-export check. However, later in this handler
probePage()callsPageComponent(...)unconditionally; whenroute.pageis undefined this will throw on every request (currently caught and ignored, but it adds avoidable exceptions/overhead and can interfere with future special-error handling). Consider guarding the probe path (and any other directPageComponent(...)call sites) when there is no page component.
const PageComponent = route.page?.default;
if (hasPageModule && !PageComponent) {
setHeadersContext(null);
setNavigationContext(null);
return new Response("Page has no default export", { status: 500 });
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let sourceParams = {}; | ||
| if (sourcePathname !== null) { | ||
| const sourceRoute = routes[entry.sourceRouteIndex]; | ||
| const sourceParts = sourcePathname.split("/").filter(Boolean); | ||
| const matchedSourceParams = sourceRoute |
There was a problem hiding this comment.
matchPattern() builds a null-prototype params object (Object.create(null)), but findIntercept() then merges into normal objects ({} + spread). That loses the null-prototype guarantee and can reintroduce prototype edge cases (e.g. a param named __proto__) and inconsistency vs the rest of the matcher. Consider keeping sourceParams and the merged matchedParams as null-prototype objects when combining source + target params.
There was a problem hiding this comment.
Fixed in the follow-up commit — mergeMatchedParams and sourceParams both use Object.create(null).
|
/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
The approach is sound — discovering layout routes whose content comes entirely from parallel slots, allowing null page modules through the render path, and merging source params into interception results are the right changes for this feature. The test coverage is good, with three distinct interception shapes and both unit and e2e tests. A few issues to address.
Summary
- Null-prototype loss on param merging (echoing Copilot's comment — this is real).
matchPattern()returnsObject.create(null)but the spread into{}creates normal objects, reintroducing prototype properties. A param named__proto__orconstructorcould cause subtle bugs. probePage()throws unconditionally for slot-only routes. WhenPageComponentisundefined(slot-only route with no page module),probePage()at line 2528 callsPageComponent(...)which throws aTypeError. It's caught byprobeAppPageComponent's try-catch so it doesn't crash, but it's avoidable overhead on every request and can confuse error monitoring. Consider guarding:if (!PageComponent) return null;at the top ofprobePage().- Missing test for direct (hard) navigation to a slot-only route. The e2e tests only cover client-side navigation to the interception target. A
page.goto("/parallel-sibling-modal/vercel")that asserts the feed slot renders correctly via full-page SSR would exercise the_handleRequestcode path wherehasPageModuleis false andPageComponentis null. That's the most novel runtime path in this PR.
| sourceParams = matchedSourceParams; | ||
| } | ||
| } | ||
| return { ...entry, matchedParams: { ...sourceParams, ...params } }; |
There was a problem hiding this comment.
matchPattern() returns Object.create(null) objects (line 905), but both sourceParams (initialized as {}) and the merged result ({ ...sourceParams, ...params }) are plain objects with Object.prototype. This loses the null-prototype guarantee and reintroduces prototype pollution risk — e.g. a param named __proto__ or constructor could shadow inherited properties.
Keep the null-prototype shape throughout:
| return { ...entry, matchedParams: { ...sourceParams, ...params } }; | |
| return { ...entry, matchedParams: Object.assign(Object.create(null), sourceParams, params) }; |
There was a problem hiding this comment.
Fixed in the latest commit before this review — mergeMatchedParams uses Object.assign(Object.create(null), ...) and sourceParams is initialized with Object.create(null).
| const params = matchPattern(urlParts, entry.targetPatternParts); | ||
| if (params !== null) { | ||
| return { ...entry, matchedParams: params }; | ||
| let sourceParams = {}; |
There was a problem hiding this comment.
Same issue — initialize as null-prototype to stay consistent with matchPattern() output:
| let sourceParams = {}; | |
| let sourceParams = Object.create(null); |
There was a problem hiding this comment.
Fixed in the same commit — let sourceParams = Object.create(null) at the initialization site.
| "Feed for vercel", | ||
| ); | ||
| await expect(page.locator('[data-testid="parallel-photo-modal"]')).toHaveText("Photo MODAL 42"); | ||
| await expect(page.locator('[data-testid="parallel-photo-page"]')).not.toBeVisible(); |
There was a problem hiding this comment.
Good test coverage for the client-side interception path. However, there's no test for direct navigation (full page load) to the slot-only route — i.e. a page.goto("/parallel-sibling-modal/vercel") that asserts the feed slot renders correctly via SSR without any prior client-side navigation. That exercises the _handleRequest code path where hasPageModule is now false and PageComponent is null, which is the most novel part of this PR.
Worth adding a standalone test like:
test("direct navigation to slot-only layout route renders slot content", async ({ page }) => {
await page.goto(`${BASE}/parallel-sibling-modal/vercel`);
await expect(page.locator('[data-testid="parallel-feed-page"]')).toContainText("Feed for vercel");
await expect(page.locator('[data-testid="parallel-modal-default"]')).toBeVisible();
});There was a problem hiding this comment.
Added — "direct navigation to slot-only parallel source route renders feed slot" at line 242 of advanced.spec.ts. It does a cold page.goto to /parallel-sibling-modal/vercel and asserts parallel-feed-page, parallel-modal-default visible, and photo elements absent.
| )) { | ||
| const dir = path.dirname(file); | ||
| const routeDir = dir === "." ? appDir : path.join(appDir, dir); | ||
| if (discoverParallelSlots(routeDir, appDir, matcher).length === 0) continue; |
There was a problem hiding this comment.
Minor perf note: this iterates every layout file then calls discoverParallelSlots (which does sync readdirSync) for each one. For apps with many layouts, the sync I/O per layout could add up. Not a blocker for this PR, but a fast readdirSync check for any @-prefixed subdirectory before the full discoverParallelSlots call would short-circuit cheaply for the majority of layouts that don't have slots.
There was a problem hiding this comment.
Already guarded — hasParallelSlotDirectory(routeDir) at line 202 does exactly this fast-path check (readdirSync looking for any @-prefixed entry) and skips the full discoverParallelSlots call for layouts without slots.
|
The review was submitted successfully. Here's a summary of what I posted: Review submitted on PR #867 with 4 inline comments and a summary covering 3 key issues:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review
The approach is solid — discovering layout routes with parallel slots but no page, allowing null page modules through the render path, and merging source params into interception results are the right changes for this feature. The test coverage is good with three distinct interception shapes, unit tests, and e2e tests including the direct navigation SSR test.
The null-prototype concern from the prior review is already addressed in this code (mergeMatchedParams uses Object.assign(Object.create(null), ...) and sourceParams is initialized with Object.create(null)).
One remaining issue: probePage() throws TypeError on every request to a slot-only route
When a slot-only route is hit (no page.tsx, so route.page is null), PageComponent is undefined (line 2208). The hasPageModule && !PageComponent guard at line 2209 correctly skips the "no default export" error, and element: PageComponent ? createElement(PageComponent, pageProps) : null at line 1118 correctly handles the null case during rendering.
But probePage() at line 2532 unconditionally calls PageComponent({...}), which throws TypeError: PageComponent is not a function. This doesn't crash — probeAppPageComponent in app-page-execution.ts:237 catches it, and onError returns null for non-special errors — but it's avoidable overhead on every request to a slot-only route and can confuse error monitoring.
Suggested fix in the generated probePage() body (line 2532 of app-rsc-entry.ts):
probePage() {
if (!PageComponent) return null;
// ... rest of probe logic
}Or inline: return PageComponent ? PageComponent({...}) : null;
This was flagged in the prior bonk review as well — worth addressing before merge.
|
Review submitted. Here's a summary of what I posted: Review on PR #867 — The overall approach is solid. The null-prototype concern from the prior bonk review is already addressed in the code. I flagged one remaining issue:
|
probePage() calls PageComponent(...) unconditionally. When a layout-owned slot-only route is hit, route.page is null so PageComponent is undefined, causing probePage() to throw TypeError on every request. probeAppPageComponent catches it and returns null, so it does not crash, but it fires an avoidable exception on every request to a slot-only URL and can confuse error monitoring. probeLayoutAt() already uses the same guard pattern: if (!LayoutComp) return null. Apply the same guard to probePage().
|
/bigbonk review |
There was a problem hiding this comment.
Review
This is a well-structured fix for sibling intercepted routes. The three logical changes — slot-only route discovery, null page module handling, and source param merging — are minimal and correctly scoped. The test coverage is good: unit test for route discovery, e2e tests for three distinct interception shapes (simple sibling, top-level film, parallel feed+modal), and the direct-navigation SSR test that exercises the novel hasPageModule=false code path.
Prior review feedback (null-prototype params, probePage() guard, direct nav test, fast @-prefix check) has all been addressed. The code looks correct.
Two minor observations below (neither blocking).
| sourceParams = matchedSourceParams; | ||
| } | ||
| } | ||
| return { ...entry, matchedParams: mergeMatchedParams(sourceParams, params) }; |
There was a problem hiding this comment.
Nit: the spread { ...entry, matchedParams: ... } creates a plain Object.prototype result object, but that's fine — this is the intercept result envelope (with sourceRouteIndex, slotKey, etc.), not the params object itself. The matchedParams value inside correctly has a null prototype via mergeMatchedParams. Just noting this for future readers so it doesn't get flagged again.
| // Layouts with parallel slot pages are valid route entries even when the | ||
| // segment has no children page. Next.js uses this for modal/feed patterns | ||
| // like app/user/[id]/layout + @feed/page + @modal/default. | ||
| const routePatterns = new Set(routes.map((route) => route.pattern)); |
There was a problem hiding this comment.
Very minor: this second scan of **/layout re-walks the filesystem after the page/route scans. For large apps this adds a third directory traversal. Not a concern for correctness or for typical app sizes, but if perf profiling ever flags appRouter, this is a candidate for folding into the first page scan pass (collecting layout directories as a side effect).
|
Review submitted. Here's a summary: Approved PR #867 with two non-blocking inline observations. The PR makes three well-scoped changes to fix sibling intercepted App Router routes:
All prior review feedback has been addressed. Test coverage spans unit (route discovery), e2e (three interception shapes), and direct-navigation SSR. |
Summary
Fixes sibling intercepted App Router navigation when the source route is a layout-owned parallel-slot route.
This now matches the Next.js
intercepting-parallel-modalreference shape where/[username]renders@feedand@modal, and navigating to a sibling target mounts the modal while preserving source content.Fixes #866
What Changed
page.tsx.(..), the reported/topto/film/[imdbId]shape, and the Next.js@feed+@modal/(..)photo/[id]shape.Why It Matters
Apps using modal/detail routes from a listing page can now keep the listing tree mounted while rendering the intercepted sibling route. Source params such as
usernameno longer disappear when they are not present in the target URL.Risks Or Limits
This changes route discovery for layout segments with parallel slots but no child page. The added route is only created when that layout owns parallel slots and no existing route pattern already exists.
Verification
npx vp test run tests/routing.test.ts -t "layout routes whose own content is parallel slot pages"npx vp test run tests/app-router.test.ts -t "intercept"PLAYWRIGHT_PROJECT=app-router npx playwright test tests/e2e/app-router/advanced.spec.ts -g "sibling|top-level"npx vp check packages/vinext/src/routing/app-router.ts packages/vinext/src/entries/app-rsc-entry.ts tests/routing.test.ts tests/e2e/app-router/advanced.spec.ts tests/fixtures/app-basic/app/sibling-source tests/fixtures/app-basic/app/sibling-target tests/fixtures/app-basic/app/top tests/fixtures/app-basic/app/film tests/fixtures/app-basic/app/parallel-sibling-modal