fix(router): gate intercept matching on Next-URL source pattern#1302
Conversation
|
Bumping to retrigger CI (workflow did not fire on initial push). |
|
Note: GitHub Actions CI workflow has not triggered on any of the pushes to this branch (initial push, two follow-up commits, close/reopen). Only the CodeQL workflow runs. Other PRs from the same branch family (#1295–#1304) all triggered CI normally. Posting /bigbonk for review of the diff; CI status needs a maintainer to look at why workflows aren't dispatching here. /bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid PR. The core behavioral change — gating findIntercept on the source pathname matching the intercepting route's pattern — is the correct fix and aligns with how Next.js implements interception rewrites via Next-URL header regex matching. The layering across scanner → manifest → matcher is clean, and the test coverage is thorough.
Correctness
Scanner (computeInterceptSourceMatchPattern): Correctly derives the intercepting route URL from interceptParentDir relative to appDir, stripping route groups and @slot segments via the existing convertSegmentsToRouteParts / isInvisibleSegment machinery. The fallback path (when convertSegmentsToRouteParts returns null) correctly filters invisible segments manually. Returns / for root, matching Next.js' normalizeAppPath behavior.
Matcher (findIntercept + matchInterceptSource + matchPathPrefix): The early null return when sourcePathname === null correctly mirrors Next.js' "no Next-URL header → no rewrite fires" semantics. matchPathPrefix correctly implements prefix matching with descendants allowed (matching ^<pattern>(?:/.*)?$), and handles :name, :name+, :name* segment types correctly. The ordering of source check before target pattern match in the loop is a minor optimization — it avoids the more expensive matchAppRscRoutePattern call when the source is already disqualified.
Call-site threading: Both dispatchMatchedPage and handleServerActionRequest in app-rsc-entry.ts already close over interceptionContext and pass it to findIntercept(pathname, interceptionContext). No callers are broken by the tighter semantics.
Backwards compat: sourceMatchPattern is optional in AppRscInterceptForMatching — matchInterceptSource returns true when sourceMatchPatternParts is null, so existing manifest entries without the field still work (just gated on non-null source). The InterceptingRoute type in app-route-graph.ts makes it required, which is correct since the scanner always computes it.
One nit
app-rsc-route-matching.ts line 212 has a // trigger ci comment that should be removed before merge.
Test coverage
The test matrix is well-designed:
- Unit tests in
app-rsc-route-matching.test.tscover the key boundary conditions: non-matching source, null source, descendant sources, root pattern, dynamic segments in the intercepting route pattern. - Scanner tests in
app-route-graph.test.tsverifysourceMatchPatterncomputation for root-level(.), static parent(..), dynamic ancestor with@modalstripping(...), and two-levels-up(..)(..). - Integration tests in
app-router.test.tscover the end-to-end behavior: intercept fires with correct context header, does NOT fire without header, does NOT fire with unrelated context. - The existing
findIntercepttests that don't setsourceMatchPatterncontinue to work via the backwards-compat path (legacy behavior accepts any non-null source).
LGTM — just clean up the // trigger ci comment.
|
The inline comment API requires the older Review posted and approved on PR #1302. Here's a summary: Verdict: Approved The PR correctly gates Key findings:
One nit: Line 212 of |
Vinext's app-rsc route matcher previously fired interception on any RSC request whose URL matched an intercept target, regardless of the source pathname. Next.js implements interception as a rewrite that is only activated when the Next-URL header matches the *intercepting route* regex (`^<interceptingRoute>(?:/.*)?$`). Without that gate, a direct `.rsc` fetch to an intercept target would incorrectly render the modal page instead of the underlying route, and unrelated sources could pull in modals they have no slot for. This change: 1. Captures the *intercepting route* URL pattern (`sourceMatchPattern`) alongside each `InterceptingRoute` at scan time. The pattern is the slot owner's path with route groups and `@slot` segments stripped, matching Next.js' `extractInterceptionRouteInformation`. The value is propagated through the App RSC manifest. 2. Adds source-pathname filtering to `createAppRscRouteMatcher`'s `findIntercept`: an entry only matches when the provided source pathname satisfies its `sourceMatchPattern` (with descendants allowed). When no source pathname is provided (no Next-URL / X-Vinext-Interception-Context header), no intercept fires. 3. Adds unit coverage in `app-rsc-route-matching.test.ts` and `app-route-graph.test.ts` for the new behavior, including the patterns used by the failing Next.js deploy suite cases (`parallel-routes-and-interception-basepath`, `parallel-routes-and-interception-from-root`, multi-catchall, and two-levels-up). 4. Updates `app-router.test.ts` integration tests that previously relied on the buggy behavior; intercept fires only when the test sends `X-Vinext-Interception-Context` and that context matches the slot's intercepting-route prefix. Cross-references: - https://github.com/vercel/next.js/blob/canary/packages/next/src/shared/lib/router/utils/interception-routes.ts - https://github.com/vercel/next.js/blob/canary/packages/next/src/lib/generate-interception-routes-rewrites.ts PR #1249 is orthogonal: it carries proof of interception through the planner/lifecycle when the matcher has already fired. This change restricts when the matcher fires in the first place.
james-elicx
force-pushed
the
fix/deploy-suite-interception-precedence
branch
from
c806ea6 to
31ed4f3
Compare
commit: |
|
|
/bigbonk review |
There was a problem hiding this comment.
Approved. The implementation correctly mirrors Next.js' interception rewrite semantics across all three layers (scanner, manifest, matcher), and the test coverage is thorough.
Key observations:
-
computeInterceptSourceMatchPatterncorrectly derives the intercepting route's URL frominterceptParentDir, stripping route groups and@slotsegments viaconvertSegmentsToRouteParts/isInvisibleSegment. The fallback path for edge cases whereconvertSegmentsToRoutePartsreturns null is also correct. -
matchPathPrefixcorrectly implements the^<pattern>(?:/.*)?$prefix matching with dynamic, catch-all, and optional catch-all segment support. The ordering infindIntercept— source check before the more expensive target pattern match — is a nice optimization. -
The secondary gate at lines 110–122 correctly preserves the #1249 safety net for legacy manifest shapes (no
sourceMatchPatternParts) while avoiding over-restriction when the primary gate has already approved the source with descendant semantics. -
The
// trigger cicomment from the previous review has been cleaned up. -
All call sites in
app-rsc-entry.tscorrectly close overinterceptionContextand thread it throughfindIntercept. The intermediate wrapper inapp-page-dispatch.tsdelegates correctly.
One minor note: non-terminal catch-all patterns in matchPathPrefix silently return false rather than throwing. This is fine since computeInterceptSourceMatchPattern upstream already validates terminal position, but if a future code path bypassed that validation, mismatches would be silent. Not a blocker — just something to keep in mind if sourceMatchPattern computation is ever extended.
|
Review posted and approved on PR #1302. Here's the summary: Verdict: Approved The PR correctly gates Key findings:
One minor note flagged: non-terminal catch-all patterns in |
1 participant
Overview
Next-URLheader actually satisfies the intercepting-route prefix, matching Next.js' rewrite semantics.Why
Vinext was modeling interception as "if the URL matches the target pattern, render the modal". Next.js models it as a rewrite gated on the Next-URL header (`^(?:/.*)?$`), so the modal only ever renders when the navigator was already inside the slot's owning route. This PR brings the matcher into parity with that contract.
Categories addressed in the Next.js deploy suite:
The remaining deploy-suite cases (`interception-segments-two-levels-above`, `interception-routes-multiple-catchall`) use interception markers placed outside a `@slot` directory. Vinext does not yet treat those as intercepting routes — fixing that requires scanner-level synthesis of an implicit "children" slot and is left for a follow-up PR.
What Changed
Behaviors verified
References
Validation
```bash
vp check
vp test run tests/app-rsc-route-matching.test.ts tests/app-route-graph.test.ts \
tests/intercepting-routes-build.test.ts tests/app-router.test.ts \
tests/app-page-request.test.ts tests/app-page-dispatch.test.ts \
tests/app-server-action-execution.test.ts tests/route-sorting.test.ts \
tests/entry-templates.test.ts tests/app-browser-entry.test.ts \
tests/app-elements.test.ts tests/app-page-element-builder.test.ts \
tests/navigation-planner.test.ts
vp run vinext#build
```
All targeted tests pass (682 tests across the listed files).
Risk / Compatibility
Non-goals / Follow-ups