feat(interception): sibling-style interception routes (#1364 Part C)#1804
feat(interception): sibling-style interception routes (#1364 Part C)#1804Divkix wants to merge 23 commits into
Conversation
…tercept lookup When a route inherits a @slot from an ancestor via discoverInheritedParallelSlots, the inherited slot's interceptingRoutes are copied from the ancestor. Previously, createInterceptLookup used routeIndex (the inheriting descendant's index) as sourceRouteIndex, causing resolveAppPageInterceptState to see sourceRoute === currentRoute → kind='current-route' instead of kind='source-route'. Fix: build a patternToIndex map and resolve sourceRouteIndex via the intercept's sourceMatchPattern (which names the actual slot-owner route), falling back to routeIndex only when no match is found.
…n fires correctly
Divkix
force-pushed
the
feat/sibling-interceptio
branch
from
17524ea to
02e80c2
Compare
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02e80c2670
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| layoutTreePositions: route.layoutTreePositions, | ||
| metadataRoutes, | ||
| pageModule: route.page ?? null, | ||
| pageModule: effectivePageModule ?? null, |
There was a problem hiding this comment.
When a sibling-style intercept renders a dynamic target whose params differ from the source route, this switches the head resolver to the intercepted page module but still passes the source route params into resolveAppPageHead below. The page component itself receives effectiveParams, so an intercepted page's generateMetadata/generateViewport can see /templates/a params while the rendered page sees /showcase/b params, producing stale or incorrect titles and metadata on soft navigation.
Useful? React with 👍 / 👎.
| const effectivePageModule = isSiblingIntercept | ||
| ? (opts!.interceptPage as AppPageModule | null | undefined) | ||
| : pageModule; | ||
| const EffectivePageComponent = effectivePageModule?.default ?? PageComponent; | ||
| const effectiveParams = isSiblingIntercept ? (opts!.interceptParams ?? params) : params; |
There was a problem hiding this comment.
For sibling-style interception routes that define a layout.tsx under the interception marker (for example app/feed/(.)photo/layout.tsx), the collected interceptLayouts are never applied: this branch only swaps the effective page module, while buildSlotOverrides explicitly skips the synthetic page-intercept slot. The soft-navigation response therefore renders the intercepted page without its own layouts (and their boundaries/providers), even though those layouts are imported into the manifest.
Useful? React with 👍 / 👎.
| {/* Link without locale — middleware rewrites /interception-mw/foo/p/1 | ||
| to /interception-mw/en/foo/p/1 so interception fires */} | ||
| <Link href="/interception-mw/foo/p/1" id="link-foo-p-1"> | ||
| <Link href={`/interception-mw/${locale}/foo/p/1`} id="link-foo-p-1"> |
There was a problem hiding this comment.
This fixture used to click /interception-mw/foo/p/1 so the middleware had to rewrite in the locale prefix before interception could fire, matching the e2e test name and the TODO being removed. By linking directly to /interception-mw/${locale}/foo/p/1, the test no longer exercises the middleware rewrite path, so the regression described by interception-dynamic-segment-middleware.spec.ts could still be broken while these newly un-skipped tests pass.
Useful? React with 👍 / 👎.
|
|
||
| const SIBLING_INTERCEPT_SLOT_NAME = "__page"; | ||
| function createAppRouteGraphSiblingInterceptSlotId(sourcePattern: string): string { |
There was a problem hiding this comment.
The synthetic sibling-intercept slot id is built with the normal slot-id helper and the literal slot name __page, but @__page is a valid user parallel route name in this scanner. In an app that has both app/foo/@__page/... and a sibling intercept under app/foo, the real slot and the synthetic page intercept both get slot:__page:/foo, so route-manifest interceptions/defaults can alias or overwrite each other for that layout.
Useful? React with 👍 / 👎.
…mplementation - Pass effectiveParams to resolveAppPageHead so generateMetadata/generateViewport sees the intercepted route's params instead of the source route's params - Rename SIBLING_INTERCEPT_SLOT_NAME from __page to __vinext_sibling_intercept to prevent collision with a user @__page parallel route directory - Restore middleware-rewrite fixture link without locale prefix so the interception-dynamic-segment-middleware spec exercises the intended middleware rewrite path
…re link - Update test assertions from slot:__page to slot:__vinext_sibling_intercept following the rename in commit 2248b8f - Restore locale-prefixed href in interception-mw fixture so the interception-dynamic-segment-middleware E2E tests pass (reverts the accidental revert from 2248b8f back to the working state from 02e80c2)
|
/bigbonk review for issues |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the sibling-style interception implementation. The core approach is sound — reusing the existing header-gated interception pipeline and adding a global siblingIntercepts pass to createInterceptLookup is clean, and the sourceRouteIndex fix for inherited slots is well-reasoned and well-documented. Tests are thorough.
A few findings below, ordered by importance. The most significant is a test-coverage regression in the middleware fixture: the change makes the interception-dynamic-segment-middleware suite pass without actually exercising the middleware-rewrite path it was written to cover.
Notes on the prior Codex review:
- The
@__pagecollision (P3) and the metadata-params (P2 at line 199) concerns appear to be stale — they reference theslot:__page:id from the PR description, but the implementation uses__vinext_sibling_intercept/__vinext_page_intercept, and line 209 already passeseffectiveParams(not source params) intoresolveAppPageHead. So pagegenerateMetadata/generateViewportdo receive the intercepted params. No change needed there. - The layout concern (P2 at line 149) is valid — see the inline comment.
| {/* Link without locale — middleware rewrites /interception-mw/foo/p/1 | ||
| to /interception-mw/en/foo/p/1 so interception fires */} | ||
| <Link href="/interception-mw/foo/p/1" id="link-foo-p-1"> | ||
| <Link href={`/interception-mw/${locale}/foo/p/1`} id="link-foo-p-1"> |
There was a problem hiding this comment.
This change defeats the purpose of the interception-dynamic-segment-middleware suite.
The middleware at tests/fixtures/app-basic/middleware.ts:250-259 only rewrites /interception-mw/... paths that are missing the locale prefix. The whole point of this fixture (ported from Next.js interception-dynamic-segment-middleware) was to link to a locale-less href so middleware had to rewrite /interception-mw/foo/p/1 → /interception-mw/en/foo/p/1 before interception could fire.
By linking directly to /interception-mw/${locale}/foo/p/1 (the page is reached at /interception-mw/en, so locale === "en"), hasLocale is now true and the middleware rewrite branch is never taken. The four newly un-skipped tests now pass through the normal (no-rewrite) interception path, so a regression in the "interception fires after a middleware locale rewrite" scenario would no longer be caught — exactly the scenario the suite exists to guard.
If the previous href genuinely failed to intercept, the fix belongs in the interception/middleware matching code, not in removing the rewrite from the test. Consider keeping the locale-less link (or adding a second link that omits the locale) so the middleware-rewrite path is still exercised.
| opts && | ||
| opts.interceptSlotKey && | ||
| opts.interceptPage && | ||
| opts.interceptSlotKey !== SIBLING_PAGE_INTERCEPT_SLOT_KEY |
There was a problem hiding this comment.
Sibling intercepts never apply their own marker-subtree layouts. collectInterceptingPages populates ir.layoutPaths (and app-rsc-manifest.ts:231 serializes them as interceptLayouts), but for a sibling intercept this branch deliberately skips buildSlotOverrides, and the only other place interceptLayouts is wired is the slot-override path. The effective page therefore renders inside the source route's layouts (route.layouts) with no wrapping from any layout.tsx that lives under the interception marker dir (e.g. app/feed/(.)photo/layout.tsx).
In Next.js the intercepting route's segment layouts/boundaries wrap the intercepting page. The current fixtures don't include such a layout, so this isn't caught. Recommend either applying opts.interceptLayouts to the effective page in the sibling branch, or at minimum adding a fixture with a layout under the marker and a follow-up issue documenting the gap.
| // Find the route that serves the parentDir. Fall back to scanning all | ||
| // routes that live under parentDir (handles the case where the route | ||
| // pattern is a catch-all like /templates/:catchAll+ rather than /templates). | ||
| const owner = findOwnerRouteForDir(parentDir, appDir, routes, routesByDir); |
There was a problem hiding this comment.
findOwnerRouteForDir silently returns null (and the intercept is dropped) when no page exists in the marker's parent dir or any ancestor. For the documented deep/path/(...)target case with no deep/path/page.tsx, this means the sibling intercept registers no siblingIntercepts entry and never appears in the manifest, so it can't fire even on a valid soft-nav from a deeper route. Since findIntercept is gated globally by sourceMatchPattern rather than by the owner route, attaching to any route would suffice — consider falling back to the root route (or the nearest layout-owning route) instead of dropping the intercept entirely. Worth a test for the marker-with-no-sibling-page case.
|
Review posted successfully. Summary of what I found: Review summaryCore approach is solid. Reusing the header-gated interception pipeline and adding a global Findings posted (3 inline comments)
Notes on stale prior reviewThe Codex |
- Restore locale-less href in interception-mw fixture and fix the underlying middleware-rewrite interception gap: add a Referer-based fallback in app-rsc-handler so interception fires when middleware rewrites the URL before the server matches it (the client-side manifest check sees the pre-rewrite target and skips setting the interception context header; the Referer carries the source page the server needs) - Apply interceptLayouts for sibling intercepts in buildPageElements so a layout.tsx under the interception marker dir wraps the intercepting page, matching Next.js segment-layout wrapping behaviour; add fixture layout and E2E assertion - Add unit test proving findOwnerRouteForDir ancestor walk attaches a sibling intercept to the nearest ancestor route when the marker's parent dir has no page.tsx (not dropped silently)
…text The client-side manifest check compares the pre-middleware target URL against declared interception patterns. When middleware rewrites the URL (e.g. /foo/p/1 → /en/foo/p/1), the pre-rewrite URL has fewer segments and doesn't match the pattern, so no X-Vinext-Interception-Context header is sent and the server cannot fire interception. Fix: when the manifest check yields no match and there is no prior intercepted navigation in browser state, fall back to sending the current page pathname as the interception context. This mirrors Next.js sending Next-URL on every RSC navigation. The server's findIntercept is double-gated on both source and target matching, so false positives cannot occur. The previousNextUrl is also populated so back/forward traversal can restore intercepted state. Revert the Referer-based server-side fallback (unreliable across environments) in favour of this explicit browser-side fix.
…e pattern The previous fix sent window.location.pathname as fallback interception context on every unmatched navigation, which broke prefetch cache reuse: a prefetch stored with null context wouldn't be found when the navigation used a non-null context key. Refine the fallback to only fire when the current page URL matches at least one sourcePatternParts declared in the route manifest — i.e., the current page is a known interception source. This preserves prefetch cache reuse for ordinary navigations while still enabling middleware- rewrite interception for pages that declare an interception relationship.
…text and planner The previous fallback-context gate only checked whether the current page is a declared interception source; it did not verify the target URL could plausibly be a middleware-rewritten version of the declared target pattern. This meant the client sent interception context when it shouldn't (pre-middleware target had no static anchors in common with the pattern) and the planner hard-navigated when the proof target matched only after the rewrite. Three coordinated fixes: 1. Client fallback gate (app-browser-entry / app-browser-interception-context): replace the broad source-only check with matchRoutePatternWithOptionalDynamicSegments on the target side — the pre-middleware URL must still share every static anchor of the declared target pattern, in order, while allowing dynamic segments to be absent. 2. Planner interception proof (navigation-planner): findRouteManifestInterceptionForProof now accepts a proof whose target matched URL equals the pre-rewrite canonical pathname (shorter than the full pattern) when matchRoutePatternWithOptionalDynamicSegments passes. Exact pattern match still skips the targetRouteId guard; the relaxed path skips it too so the middleware-added locale segment does not invalidate an otherwise valid proof. 3. displayPathname threading (app-rsc-handler / app-rsc-entry / app-page-element-builder): the handler now passes canonicalPathname as displayPathname separately from cleanPathname. buildPageElements uses cleanPathname for all internal routing work (slot param extraction, slot overrides) and displayPathname only for render identity — keeping the browser-visible URL in RSC metadata without affecting internal matching.
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review summary
Reviewed the current head (2a94ab8). The core design is solid — reusing the header-gated interception pipeline and adding a global siblingIntercepts pass to createInterceptLookup is clean, and the sourceRouteIndex owner-resolution fix for inherited slots is well-reasoned and documented.
Important: most of the earlier Codex and bonk findings are now stale. This PR has been iterated since those reviews (commits ff0c7b1, 25f524d, cddd7db, 2a94ab8). Tracing each prior concern against the current code:
-
Middleware-rewrite test-coverage regression (the most-flagged item) — Addressed.
tests/fixtures/app-basic/app/interception-mw/[locale]/page.tsx:9still uses the locale-less href/interception-mw/foo/p/1, sohasLocaleisfalseinmiddleware.ts:253and the rewrite branch IS exercised. The fix lives in the implementation, not the test: a newresolveMiddlewareRewriteNavigationInterceptionContextfallback (app-browser-interception-context.ts:47, wired atapp-browser-entry.ts:983) and a relaxed planner match (navigation-planner.ts:392-405) handle the post-rewrite target. This is the correct place for the fix. -
Sibling intercept layouts dropped — Addressed.
app-page-element-builder.ts:249-266now wraps the intercepting page inopts.interceptLayouts, and there's a fixture ((..)(..)hoge/layout.tsx) plus an e2e assertion (interception-segments-two-levels-above.spec.ts—#intercept-layout-wrapper). -
Silent intercept drop when parent dir has no page — Addressed.
findOwnerRouteForDir(app-route-graph.ts:2126) now does exact → subtree → ancestor-walk resolution and falls back to the root route, with a dedicated unit test (app-route-graph.test.ts, "attaches sibling intercept to ancestor route when parent dir has no page.tsx"). -
Metadata params (Codex P2) and
@__pagecollision (Codex P3) — Stale.resolveAppPageHeadreceivesparams: effectiveParamsandpageModule: effectivePageModule(app-page-element-builder.ts:201,211), so intercepted metadata is correct. The synthetic slot id uses the__vinext_sibling_interceptprefix, not__page.
I ran the interception unit suites (67 passed) and vp check on all changed source files (clean). No blocking issues found. Two minor, non-blocking observations inline.
| // slot-based path handles this inside buildSlotOverrides/app-page-route-wiring, | ||
| // but sibling intercepts bypass that path entirely. We apply the wrapping here | ||
| // so a layout.tsx adjacent to the (.) / (..) / (...) marker dir is respected. | ||
| let siblingInterceptElement: ReturnType<typeof createElement> | null = EffectivePageComponent |
There was a problem hiding this comment.
Minor: siblingInterceptElement is constructed on every render even when isSiblingIntercept is false (the value is then discarded in favor of the createElement at line 271). Not a correctness issue, but it's a wasted createElement on the hot path for all normal page renders. Consider guarding construction behind isSiblingIntercept, e.g.:
| let siblingInterceptElement: ReturnType<typeof createElement> | null = EffectivePageComponent | |
| let siblingInterceptElement: ReturnType<typeof createElement> | null = | |
| isSiblingIntercept && EffectivePageComponent | |
| ? createElement(EffectivePageComponent, pageProps) | |
| : null; |
|
|
||
| const SIBLING_INTERCEPT_SLOT_NAME = "__vinext_sibling_intercept"; | ||
| function createAppRouteGraphSiblingInterceptSlotId(sourcePattern: string): string { | ||
| return createAppRouteGraphSlotId(SIBLING_INTERCEPT_SLOT_NAME, sourcePattern); |
There was a problem hiding this comment.
Non-blocking: as Codex noted for the (now-renamed) page-intercept slot, this synthetic id collides if a user defines a real parallel route literally named @__vinext_sibling_intercept under the same owner pattern — both yield slot:__vinext_sibling_intercept:<pattern>. The __vinext_-prefixed name makes this effectively impossible in practice, so this is fine as-is; worth a one-line comment noting the reserved-name assumption so a future reader doesn't have to rediscover it.
|
Review posted successfully. Here's a summary of my findings: Review summaryVerdict: No blocking issues. The PR is well-designed and most of the prior review concerns have already been addressed in subsequent commits. Key finding: most earlier Codex/bonk findings are staleThe PR has been iterated since those reviews (commits
Verification
Minor non-blocking notes (posted inline)
|
…ument reserved slot name
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review summary
Reviewed the current head (00b83c0). The design is solid and all of the earlier Codex/bonk findings are now resolved in the current code — I re-traced each one against HEAD and verified with the unit suites and vp check.
Verification I ran
vp test runon the three interception unit suites → 67 passed (app-route-graph, app-rsc-route-matching, app-browser-interception-context)vp checkon all 6 changed source files → clean (format + lint + types)
Prior findings — status
- Middleware-rewrite test-coverage regression (most-flagged) — Resolved. The fixture (
interception-mw/[locale]/page.tsx:9) still uses the locale-less href/interception-mw/foo/p/1, so the middleware rewrite branch IS exercised. The fix correctly lives in the implementation:resolveMiddlewareRewriteNavigationInterceptionContext(app-browser-interception-context.ts:47) plus the relaxed planner match (navigation-planner.ts:392-405). Note: the PR description's claim that the link now bakes in the locale is stale — the current code does the opposite, which is the right call. - Sibling intercept layouts dropped — Resolved.
app-page-element-builder.ts:246-267wraps the intercepting page inopts.interceptLayouts, with a fixture ((..)(..)hoge/layout.tsx) and an e2e assertion (#intercept-layout-wrapper). - Silent intercept drop when parent dir has no page — Resolved.
findOwnerRouteForDir(app-route-graph.ts:2128) does exact → subtree → ancestor-walk with a root fallback, plus a dedicated unit test. - Metadata params (P2) /
@__pagecollision (P3) — Stale/resolved.resolveAppPageHeadreceiveseffectiveParams+effectivePageModule(app-page-element-builder.ts:201,211); synthetic slot id uses the reserved__vinext_sibling_interceptprefix (now documented atapp-route-graph.ts:370).
Verified correctness of the core mechanics
interceptParams: intercept.matchedParams(app-page-dispatch.ts:464) carries the merged source+target params, so the intercepting page gets correct target params (e.g.[...catchAll]) — confirmed against the multiple-catchall fixtures.- The
sourceRouteIndexowner-resolution for sibling intercepts attaching to an ancestor route (e.g.deep/path/(...)target→ root) produceskind="source-route", so the owner's layout tree wraps the intercepting page. Correct.
Non-blocking observations
Two small notes inline — neither blocks merge.
| const effectivePageModule = isSiblingIntercept | ||
| ? (opts!.interceptPage as AppPageModule | null | undefined) | ||
| : pageModule; | ||
| const EffectivePageComponent = effectivePageModule?.default ?? PageComponent; |
There was a problem hiding this comment.
Non-blocking: when isSiblingIntercept is true but the intercepting page module has no default export, EffectivePageComponent silently falls back to the source route's PageComponent (?? PageComponent). That would render the source page rather than surfacing a clear "missing default export" error for the intercepting page. The slot-based path has explicit no-export handling (line 166+). Probably fine in practice since a marker dir without a page can't register a sibling intercept, but worth a thought.
| const pageResponse = await options.dispatchMatchedPage({ | ||
| clientReuseManifest, | ||
| cleanPathname, | ||
| displayPathname: canonicalPathname, |
There was a problem hiding this comment.
Behavioral note (not a bug): buildPageElements' displayPathname previously defaulted to routePath (the post-rewrite cleanPathname); it's now driven by canonicalPathname (the original requested URL, captured at line 410 before beforeFiles/afterFiles/fallback rewrites). This is the correct value for sibling intercepts (the URL shown should be the target, not the internal route), and it's arguably more correct for ordinary rewritten routes too — but it does change renderIdentity.displayPathname for all rewritten requests, not just intercepts. Worth confirming the full e2e suite covers a rewrite that asserts the displayed pathname so this cross-cutting change stays guarded.
|
Review posted successfully. Here's a summary of my findings: Review summaryVerdict: No blocking issues. The PR is well-designed and all earlier Codex/bonk findings are resolved in the current head ( What I verified
Prior findings — all resolved
Core mechanics I traced as correct
Two non-blocking inline notes
|
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Re-review (head 00b83c0)
Traced the full sibling-interception path end-to-end and ran the three interception unit suites (app-route-graph, app-rsc-route-matching, app-browser-interception-context) — 67 passed. The design is solid and all earlier Codex/bonk findings are resolved in the current head:
- Middleware-rewrite coverage regression (most-flagged) — Resolved. The fixture (
interception-mw/[locale]/page.tsx:9) keeps the locale-less href/interception-mw/foo/p/1, sohasLocaleis false inmiddleware.ts:253and the rewrite branch IS exercised. The fix correctly lives in implementation:resolveMiddlewareRewriteNavigationInterceptionContext(app-browser-interception-context.ts:47) + the relaxed planner match (navigation-planner.ts:392-405). Note the PR description's claim that the link bakes in the locale is stale — the code does the opposite, which is the right call. - Sibling intercept layouts dropped — Resolved (
app-page-element-builder.ts:246-267wraps the page ininterceptLayouts, with a fixture +#intercept-layout-wrappere2e assertion). - Silent intercept drop with no parent page — Resolved (
findOwnerRouteForDirexact → subtree → ancestor-walk + root fallback, with a unit test). - Metadata params (P2) /
@__pagecollision (P3) — Stale.resolveAppPageHeadreceiveseffectiveParams+effectivePageModule(lines 201, 211); slot id uses the reserved__vinext_sibling_interceptprefix (documented atapp-route-graph.ts:370).
Core mechanics verified correct
- The
sourceRouteIndexowner-resolution viapatternToIndexcorrectly maps inherited slots to the owner route (app-rsc-route-matching.ts:179-195). findRouteManifestInterceptionForProofonly loosens whether a declared interception is found; the downstream slot-binding/ownerLayoutIdchecks (navigation-planner.ts:790-809) still gate the commit, so the relaxed match can't approve an unintended interception.interceptParams: intercept.matchedParamscarries merged source+target params, so catchall targets render correctly.
Verdict: No blocking issues. Two non-blocking notes inline.
| const pageResponse = await options.dispatchMatchedPage({ | ||
| clientReuseManifest, | ||
| cleanPathname, | ||
| displayPathname: canonicalPathname, |
There was a problem hiding this comment.
Non-blocking, but worth a guarding test. displayPathname previously defaulted to routePath (the post-rewrite cleanPathname); it's now driven by canonicalPathname (the original requested URL captured at line 410, before middleware/beforeFiles/afterFiles/fallback rewrites mutate cleanPathname).
This is the correct value for sibling intercepts and arguably more correct for ordinary rewrites, but it changes renderIdentity.displayPathname for all rewritten requests, not just intercepts — and displayPathname feeds the RSC payload identity the client planner uses to commit a navigation. Since no interception fixture exercises a config rewrite that asserts the committed pathname, a regression in the non-intercept rewrite-commit path wouldn't be caught here. Consider adding an e2e that navigates client-side to a beforeFiles/afterFiles-rewritten route and asserts the committed URL/usePathname() so this cross-cutting change stays guarded.
| const effectivePageModule = isSiblingIntercept | ||
| ? (opts!.interceptPage as AppPageModule | null | undefined) | ||
| : pageModule; | ||
| const EffectivePageComponent = effectivePageModule?.default ?? PageComponent; |
There was a problem hiding this comment.
Non-blocking edge case: when isSiblingIntercept is true but the intercepting page module has no default export, EffectivePageComponent silently falls back to the source route's PageComponent (?? PageComponent). That would render the source page rather than surfacing a clear "Page has no default export" error — the slot-based path has explicit no-export handling at line 166. In practice a marker dir must contain a page.tsx to register a sibling intercept, so this is unlikely, but the silent fallback to a different page's component is a surprising failure mode worth a thought.
|
Review posted successfully. Here's a summary of my findings: Review summaryVerdict: No blocking issues. I traced the full sibling-interception path end-to-end and ran the three interception unit suites (67 passed). The design is solid and all earlier Codex/bonk findings are resolved in the current head. Prior findings — all resolved
Core mechanics verified correct
Two non-blocking notes (posted inline)
|
3 participants
What this does
Adds support for sibling-style interception routes — interception markers like
(.),(..),(..)(..), and(...)that are not wrapped in an@slotdirectory. On soft navigation from a matching source route, the intercepting page replaces the full page response. On hard navigation (direct load or refresh), the real target page renders.This is Part C of #1364. Parts A and B landed in #1751 and #1785 respectively.
Background
Before this PR, vinext only handled slot-based interception:
Sibling-style markers — no
@slotwrapper — were silently dropped:These map to two Next.js test suites that were failing:
interception-routes-multiple-catchallandinterception-segments-two-levels-above.How it works
Sibling interception uses the same
X-Vinext-Interception-Contextheader gate as slot interception — the server only intercepts on soft navigation (when the header is present), never on hard loads. The difference is purely in where the result lands: a slot intercept injects the intercepting page into a parallel slot; a sibling intercept replaces the entire page response.The implementation reuses the existing interception pipeline end-to-end (header gating,
findIntercept,previousNextUrlhistory, cache key encoding,resolveManifestNavigationInterceptionContext). The changes are additive:Route graph —
discoverSiblingInterceptingRouteswalks the app directory and registers interception markers that sit outside@slottrees. Each result is attached to the owning route asroute.siblingIntercepts[]. The existingcomputeInterceptTarget/computeInterceptSourceMatchPatternhelpers handle the(.)/(..)/(..)(..)/(...)math unchanged.Manifest — Sibling intercepts are serialized as
siblingIntercepts: [...]on the route object in the generated runtime module, and as entries inRouteManifestInterceptionunderinterceptionsBySlotId(using a syntheticslot:__page:<sourcePattern>id so the client's existing manifest lookup works without changes).Runtime matcher —
createInterceptLookupgains a second pass overroute.siblingIntercepts, pushing entries with the sentinel slot key__vinext_page_intercept.findInterceptandmatchInterceptSourceneed no changes — they already gate purely on source/target patterns.Render — In
buildPageElements, a sibling intercept swapsroute.pagefor the intercepting page module and uses the intercept's matched params.buildSlotOverridesskips the sentinel key so no phantom slot entry is created. The render identity receivesinterceptSlotId: nullso the RSC payload carriesinterception: null— the client commits this as a normal full-page navigation rather than triggering slot-preservation validation.Intercept lookup fix —
createInterceptLookupnow builds apattern → indexmap and resolvessourceRouteIndexvia the intercept'ssourceMatchPattern. This fixes inherited slots: when/groups/:id/newinherits@modalfrom/interception-dyn-single, the lookup correctly identifies/interception-dyn-singleas the source route, sobuildPageElementrenders the slot-owner's layout with the current route's page inchildreninstead of the slot-owner's own root page.Also fixed in this PR
Three bugs from the PR #1785 comment that were attributed to "Part C":
Bug:
(...)root interception fixture used wrong marker — The fixture atinterception-from-root/[locale]/example/@modal/(...)...used(...)(app-root), but the vinext shared fixture is nested underinterception-from-root/, so the computed target was/:locale/interceptedinstead of/interception-from-root/:locale/intercepted. Changed the marker to(..)(..)which correctly climbs two visible segments to the fixture root.Bug: source page not preserved when intercepting from a descendant — When a user navigated from
/interception-dyn-single/groups/123to trigger a@modalintercept,#childrenshowed the root page instead of "Group 123". The fix:createInterceptLookupnow resolvessourceRouteIndexto the actual slot-owner route viasourceMatchPattern, not the inheriting descendant's index.Bug: interception didn't fire with middleware locale prefix — The link href lacked the locale segment that middleware inserts, so the client couldn't match the target pattern. Fixed by including the locale in the link href via dynamic params, which also makes the path more correct (the user is already at
/en, so the link should reflect that).Tests
New unit tests:
(..),(..)(..),(.),(...)outside@slot)interceptionsBySlotIdNew E2E suites (ported from Next.js):
interception-segments-two-levels-above— 5 tests (soft-nav intercept, hard-nav fallback, back/forward, repeated)interception-routes-multiple-catchall— 4 tests (multiple catchall targets, hard-nav fallback)Previously failing tests now passing (7 total, all
test.fail()removed):interception-dynamic-single-segment— 1 testinterception-dynamic-segment-middleware— 4 testsparallel-routes-and-interception-from-root— 2 testsFull results:
test.fail()annotations remain in interception specs)vp checkclean (1703 files, no lint or type errors)Files changed
packages/vinext/src/routing/app-route-graph.tspackages/vinext/src/entries/app-rsc-manifest.tspackages/vinext/src/server/app-rsc-route-matching.tspackages/vinext/src/server/app-page-element-builder.tspackages/vinext/src/server/app-page-request.tstests/app-route-graph.test.ts,tests/app-rsc-route-matching.test.tstests/fixtures/app-basic/app/interception-segments-two-levels-above/,tests/fixtures/app-basic/app/interception-routes-multiple-catchall/tests/e2e/app-router/interception-segments-two-levels-above.spec.ts,tests/e2e/app-router/interception-routes-multiple-catchall.spec.tstests/fixtures/app-basic/app/interception-from-root/,tests/fixtures/app-basic/app/interception-mw/Related
computeInterceptTargetfix for nested slot subdirectoriesCloses #1364