test: regression tests for inherited intercepting route slot deduplication#721
Conversation
| const uniqueInterceptTargetPatterns = new Map<string, string>(); | ||
| for (const route of routes) { | ||
| for (const slot of route.parallelSlots) { | ||
| for (const intercept of slot.interceptingRoutes) { | ||
| // Inherited slots can surface the same intercepting page on multiple | ||
| // child routes. De-dupe by page path so we only validate distinct | ||
| // physical intercept definitions while still rejecting real conflicts. | ||
| uniqueInterceptTargetPatterns.set(intercept.pagePath, intercept.targetPattern); | ||
| } | ||
| } | ||
| } | ||
| validateRoutePatterns(Array.from(uniqueInterceptTargetPatterns.values())); |
There was a problem hiding this comment.
The old approach could have been wrapped in a set to achieve the same result I imagine.
I wonder if it could potentially be possible to have two conflicting pagePath values and therefore one overwrites the others targetPattern 🤔
There was a problem hiding this comment.
@james-elicx I changed it a bit. Mind taking another look?
There was a problem hiding this comment.
I think it could potentially be possible for two interceptions to target the same route pattern
What I meant when I wrote this was that you could potentially achieve the same desired outcome with:
[...new Set(routes.flatMap((route) =>
route.parallelSlots.flatMap((slot) =>
slot.interceptingRoutes.map((intercept) => intercept.targetPattern),
),
))]just deduping the array we originally created
what do you think?
There was a problem hiding this comment.
I updated the PR to follow your suggestion.
After discussing it a bit more with AI, I think the targetPattern dedupe is probably fine for this PR. What I still want to validate is the behavior when there are multiple intercepting routes targeting the same route, for example:
app/feed/page.tsx
app/feed/@modal/(..)photo/[id]/page.tsx
app/profile/page.tsx
app/profile/@modal/(..)photo/[id]/page.tsx
app/photo/[id]/page.tsx
I’m going to put together a small demo around this case to compare the behavior in Next.js and vinext, just to make sure we’re matching the intended semantics.
There was a problem hiding this comment.
No worries, I'll mark as draft for the moment while you're looking at that
There was a problem hiding this comment.
Ok, I opened #730 to track this issue separately.
There was a problem hiding this comment.
The Set dedupe was merging in another PR so I'm going to merge this as additional regression tests.
|
/bigbonk review |
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good bug find and clean fix. The root cause analysis is correct — inherited slots cause the same physical intercepting page to appear on multiple discovered routes, and the old flatMap approach counted them as separate entries.
The fix is sound for the problem it solves. I have one concern about a remaining edge case and a few minor nits.
Remaining edge case (not a blocker for this PR): Two different physical intercepting pages (different pagePath) that happen to target the same pattern (e.g. app/a/@modal/(...)photo/[id]/page.tsx and app/b/@modal/(...)photo/[id]/page.tsx both targeting /photo/:id) would still produce duplicate values from Array.from(uniqueInterceptTargetPatterns.values()), which validateRoutePatterns would reject. In Next.js, multiple intercepting routes from different locations can intercept the same target — interception is scoped to the navigation context. But this is a pre-existing issue (the old flatMap code had the same behavior), so it's not a regression introduced here.
| } | ||
|
|
||
| export function collectInterceptTargetPatterns(routes: readonly AppRoute[]): string[] { | ||
| const uniqueInterceptTargetPatterns = new Map<string, string>(); |
There was a problem hiding this comment.
To address @james-elicx's review comment: the Map is keyed by pagePath (physical file) with targetPattern as the value. So two different physical intercepting pages that target the same pattern would both be stored (separate keys), and Array.from(values()) would emit that pattern twice — causing validateRoutePatterns to throw.
This isn't a regression (the old flatMap had the same problem), but worth noting that de-duplicating by targetPattern in the returned array would be more correct for the general case. The simplest approach would be to return values through a Set:
| const uniqueInterceptTargetPatterns = new Map<string, string>(); | |
| const uniqueInterceptTargetPatterns = new Map<string, string>(); |
Actually the Map type is fine as-is — but consider de-duping the return value at line 159:
return [...new Set(uniqueInterceptTargetPatterns.values())];
This would handle the case where two different physical pages legitimately intercept the same target pattern (e.g. via (...) from different subtrees). Not a blocker for this PR, but a follow-up improvement.
| } | ||
| } | ||
|
|
||
| return Array.from(uniqueInterceptTargetPatterns.values()); |
There was a problem hiding this comment.
Consider de-duplicating the returned values to handle the case where two different physical pages (different pagePath) legitimately intercept the same targetPattern:
| return Array.from(uniqueInterceptTargetPatterns.values()); | |
| return Array.from(new Set(uniqueInterceptTargetPatterns.values())); |
Without this, two different @modal slots in sibling subtrees both using (...) to intercept the same root-level route would still trigger a false positive from validateRoutePatterns.
| const builder = await createBuilder({ | ||
| root, | ||
| configFile: false, | ||
| plugins: [vinext({ appDir: root })], |
There was a problem hiding this comment.
Passing appDir: root means appDir is the project root, not the app/ subdirectory. Is this intentional? I'd expect appDir: path.join(root, 'app') based on how other tests configure vinext — unless vinext() defaults to <root>/app when appDir is the project root.
Worth double-checking this doesn't accidentally work by falling through to a default.
| import { afterEach, describe, expect, it } from "vite-plus/test"; | ||
| import vinext from "../packages/vinext/src/index.js"; | ||
|
|
||
| const tmpDirs: string[] = []; |
There was a problem hiding this comment.
Nit: The tmpDirs array + afterEach cleanup pattern works, but the withTempDir helper from routing.test.ts is cleaner (auto-cleans in finally). Consider extracting it to a shared test utility if you want to reuse it, or just use the same pattern inline here. Not a blocker.
tests/routing.test.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| it("rejects a single intercepting page resolving to multiple target patterns", () => { |
There was a problem hiding this comment.
Good test — this directly validates the new collectInterceptTargetPatterns function with a synthetic conflict. The test constructs a scenario where the same pagePath maps to two different targetPattern values and verifies it throws.
One thing to consider: it would also be valuable to test the inverse — two different pagePath values that map to the same targetPattern — to document the current behavior (which would currently throw, as discussed in my comment on the implementation).
| was intercepted by the parallel slot. | ||
| </p> | ||
| <div className="font-mono text-sm text-cyan-300"> | ||
| ${product.price.toFixed(2)} |
There was a problem hiding this comment.
Nit: This renders a literal $ followed by the price. In JSX, $ is not interpolated, so this renders correctly as $1.99 etc. — but it reads confusingly like a template literal that's missing backticks. Consider wrapping in braces for clarity:
| ${product.price.toFixed(2)} | |
| {`$${product.price.toFixed(2)}`} |
| Loading this URL directly should render the standalone page. Navigating | ||
| from the gallery should keep the gallery visible and render this content | ||
| in the parallel modal slot instead. | ||
| </p> |
There was a problem hiding this comment.
Same nit as the modal page — this JSX $ works but reads like a broken template literal:
| </p> | |
| {`$${product.price.toFixed(2)}`} |
|
Review posted successfully. Here's a summary of the feedback I provided: Overall assessment: The fix is correct and well-motivated. The root cause (inherited slots causing the same physical intercepting page to appear multiple times) is accurately diagnosed, and de-duplicating by Key feedback points:
|
|
/bigbonk review |
There was a problem hiding this comment.
Good bug fix. The root cause (inherited slots carrying interceptingRoutes to child routes, causing duplicates in validation) is correctly diagnosed and the fix is minimal. A few notes below.
The PR is marked as draft while the author investigates cross-subtree interception semantics (e.g. app/feed/@modal/(..)photo/[id] + app/profile/@modal/(..)photo/[id] both targeting /photo/:id). That's the right call — the Set de-dupe on targetPattern silently allows that case, and it's worth confirming Next.js behavior before merging.
| cachedPageExtensionsKey = null; | ||
| } | ||
|
|
||
| export function collectInterceptTargetPatterns(routes: readonly AppRoute[]): string[] { |
There was a problem hiding this comment.
Exporting this is good — it makes the de-duplication logic directly unit-testable (which the new test exercises). One thing to consider: the function name says "collect" but it also de-duplicates. A name like collectUniqueInterceptTargetPatterns would make the contract clearer, though this is a minor nit.
| const builder = await createBuilder({ | ||
| root, | ||
| configFile: false, | ||
| plugins: [vinext({ appDir: root })], |
There was a problem hiding this comment.
The previous bonk review flagged this as potentially wrong (appDir: root instead of path.join(root, 'app')), but this is actually correct. The appDir option is used as baseDir in the plugin, and path.join(baseDir, 'app') is derived internally (see index.ts:1200). So passing the project root is the intended usage.
However, I'd argue this test would be clearer without the appDir option at all — since the fixture has app/ at the root, the auto-detection logic (index.ts:1185-1196) would find it. Removing appDir: root would test the more common configuration path and avoid this confusion.
| import { afterEach, describe, expect, it } from "vite-plus/test"; | ||
| import vinext from "../packages/vinext/src/index.js"; | ||
|
|
||
| const tmpDirs: string[] = []; |
There was a problem hiding this comment.
The routing.test.ts file has a withTempDir helper that handles cleanup in a finally block, which is safer — if the test throws before tmpDirs.push(root), cleanup never happens. Consider using the same pattern:
async function withTempDir<T>(prefix: string, fn: (dir: string) => Promise<T>): Promise<T> {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix));
try {
return await fn(dir);
} finally {
fs.rmSync(dir, { recursive: true, force: true });
}
}Or better yet, extract the one from routing.test.ts into a shared test utility.
tests/routing.test.ts
Outdated
| }, | ||
| ]; | ||
|
|
||
| expect(collectInterceptTargetPatterns([firstRoute, secondRoute])).toEqual(["/photo/:id"]); |
There was a problem hiding this comment.
This test directly validates that two different physical pages targeting the same targetPattern produce a single entry — good coverage for the Set de-duplication.
Worth noting: this also documents that vinext currently allows two different physical intercepting pages to target the same pattern (the Set silently merges them). If that turns out to differ from Next.js behavior, this test will need updating.
| EMPTY_PAGE, | ||
| ); | ||
|
|
||
| invalidateAppRouteCache(); |
There was a problem hiding this comment.
This is the key regression test — it exercises the exact scenario from the bug report (inherited @modal slot causing the same intercepting page to appear on both /intercepting-routes and /intercepting-routes/photo/:id, which the old code rejected).
The assertions at lines 1122-1130 nicely verify that both routes see the same targetPattern and pagePath, confirming the inheritance behavior.
| @@ -0,0 +1,46 @@ | |||
| 'use cache'; | |||
There was a problem hiding this comment.
Is 'use cache' intentional here? This layout receives modal as a prop (a React node from a parallel slot). Caching a layout that renders dynamic slot content could produce stale modal content or cache key explosions depending on how the cache boundary interacts with RSC streaming.
Other layouts in the playground (e.g. parallel-routes/layout.tsx) don't use 'use cache'. If this was copied from another layout template, consider removing it.
|
Review posted successfully. Here's a summary of the feedback I provided: Overall assessment: Good bug fix. The root cause is correctly diagnosed — inherited parallel slots carry their The PR is correctly marked as draft while the author investigates cross-subtree interception semantics. Key feedback points:
|
- Merge origin/main into fix/intercepting-routes - Rename collectInterceptTargetPatterns → collectUniqueInterceptTargetPatterns - Fix $price JSX template literals in example modal and photo pages - Remove 'use cache' from intercepting-routes layout (parallel slot renders dynamic content) - Refactor intercepting-routes-build.test.ts: use withTempDir helper, remove appDir:root
james-elicx
changed the title
|
/bigbonk review |
There was a problem hiding this comment.
This PR adds solid regression tests for the inherited intercepting route slot deduplication fix. The test coverage is well-targeted and the playground example is a good addition. A few things to clean up before merge.
Summary:
- The two regression tests (unit + build) are well-structured and cover the exact scenario from the bug report
- The playground example is clean and follows existing patterns (and the
'use cache'issue from prior reviews was addressed) - The
.gitignorechanges are unrelated to this PR and should be dropped - The formatting-only change in
app-router.tsis noise — consider dropping it or mentioning it in the PR description - The duplicated
withTempDirhelper should ideally be shared, but that can be a follow-up
| coverage/ | ||
|
|
||
| # Worktrees | ||
| .worktrees/ |
There was a problem hiding this comment.
This PR removes .claude (without trailing slash) and .vinext from .gitignore. These are unrelated to the intercepting routes regression tests and shouldn't be part of this PR.
- Removing
.claude(the file variant) while keeping.claude/(the directory variant) is an odd partial cleanup. If the intent is to stop ignoring.claudefiles, that should be its own commit/PR. - Removing
.vinextmay cause it to show up as untracked if anyone has a.vinextfile locally.
Please revert the .gitignore changes — they're accidental noise from the branch.
| converted: { | ||
| urlSegments: string[]; |
There was a problem hiding this comment.
This is a formatting-only change (expanding the inline type literal to multi-line). It's fine stylistically but unrelated to the regression tests. Since the maintainer already noted the Set dedupe is merging in another PR, this formatting change may cause a merge conflict with that work. Consider dropping it to keep this PR focused on tests only.
| import { describe, expect, it } from "vite-plus/test"; | ||
| import vinext from "../packages/vinext/src/index.js"; | ||
|
|
||
| async function withTempDir<T>(prefix: string, run: (tmpDir: string) => Promise<T>): Promise<T> { |
There was a problem hiding this comment.
This is a duplicate of the withTempDir helper in routing.test.ts:22. Prior reviews flagged this too. Not a blocker for this PR, but worth extracting to a shared tests/_helpers.ts or similar as a follow-up to avoid drift between the two copies.
| it("allows inherited intercepting slots to reuse the same target pattern", async () => { | ||
| await withTempDir("vinext-app-intercept-inherited-slot-", async (tmpDir) => { | ||
| const appDir = path.join(tmpDir, "app"); | ||
|
|
||
| await mkdir(path.join(appDir, "intercepting-routes", "@modal", "(.)photo", "[id]"), { | ||
| recursive: true, | ||
| }); | ||
| await mkdir(path.join(appDir, "intercepting-routes", "photo", "[id]"), { | ||
| recursive: true, | ||
| }); | ||
|
|
||
| await writeFile(path.join(appDir, "layout.tsx"), EMPTY_PAGE); | ||
| await writeFile(path.join(appDir, "page.tsx"), EMPTY_PAGE); | ||
| await writeFile(path.join(appDir, "intercepting-routes", "layout.tsx"), EMPTY_PAGE); | ||
| await writeFile(path.join(appDir, "intercepting-routes", "page.tsx"), EMPTY_PAGE); | ||
| await writeFile( | ||
| path.join(appDir, "intercepting-routes", "photo", "[id]", "page.tsx"), | ||
| EMPTY_PAGE, | ||
| ); | ||
| await writeFile( | ||
| path.join(appDir, "intercepting-routes", "@modal", "default.tsx"), | ||
| EMPTY_PAGE, | ||
| ); | ||
| await writeFile( | ||
| path.join(appDir, "intercepting-routes", "@modal", "(.)photo", "[id]", "page.tsx"), | ||
| EMPTY_PAGE, | ||
| ); | ||
|
|
||
| invalidateAppRouteCache(); | ||
| const routes = await appRouter(appDir); | ||
|
|
||
| const galleryRoute = routes.find((route) => route.pattern === "/intercepting-routes"); | ||
| const detailRoute = routes.find( | ||
| (route) => route.pattern === "/intercepting-routes/photo/:id", | ||
| ); | ||
|
|
||
| expect(galleryRoute).toBeDefined(); | ||
| expect(detailRoute).toBeDefined(); | ||
|
|
||
| const galleryModal = galleryRoute!.parallelSlots.find((slot) => slot.name === "modal"); | ||
| const detailModal = detailRoute!.parallelSlots.find((slot) => slot.name === "modal"); | ||
|
|
||
| expect(galleryModal?.interceptingRoutes[0]?.targetPattern).toBe( | ||
| "/intercepting-routes/photo/:id", | ||
| ); | ||
| expect(detailModal?.interceptingRoutes[0]?.targetPattern).toBe( | ||
| "/intercepting-routes/photo/:id", | ||
| ); | ||
| expect(detailModal?.interceptingRoutes[0]?.pagePath).toBe( | ||
| galleryModal?.interceptingRoutes[0]?.pagePath, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Good regression test — this exercises exactly the scenario from the bug report: inherited @modal slot causing the same intercepting page to appear on both /intercepting-routes and /intercepting-routes/photo/:id. The assertions at lines 1121-1129 clearly verify that both routes see the same targetPattern and pagePath, confirming inheritance behavior and that the deduplication works correctly.
| it("builds when an inherited modal slot intercepts the same target route as a standalone page", async () => { | ||
| // Ported from Next.js route interception behavior: | ||
| // test/e2e/app-dir/parallel-routes-and-interception/parallel-routes-and-interception.test.ts | ||
| // https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/parallel-routes-and-interception/parallel-routes-and-interception.test.ts | ||
| await withTempDir("vinext-intercept-build-", async (root) => { | ||
| fs.symlinkSync( | ||
| path.resolve(import.meta.dirname, "../node_modules"), | ||
| path.join(root, "node_modules"), | ||
| "junction", | ||
| ); | ||
|
|
||
| writeFixtureFile( | ||
| root, | ||
| "package.json", | ||
| JSON.stringify({ name: "vinext-intercept-build", private: true, type: "module" }, null, 2), | ||
| ); | ||
| writeFixtureFile( | ||
| root, | ||
| "tsconfig.json", | ||
| JSON.stringify( | ||
| { | ||
| compilerOptions: { | ||
| target: "ES2022", | ||
| module: "ESNext", | ||
| moduleResolution: "bundler", | ||
| jsx: "react-jsx", | ||
| strict: true, | ||
| skipLibCheck: true, | ||
| types: ["vite/client", "@vitejs/plugin-rsc/types"], | ||
| }, | ||
| include: ["app", "*.ts", "*.tsx"], | ||
| }, | ||
| null, | ||
| 2, | ||
| ), | ||
| ); | ||
| writeFixtureFile( | ||
| root, | ||
| "app/layout.tsx", | ||
| `import type { ReactNode } from "react"; | ||
|
|
||
| export default function RootLayout({ children }: { children: ReactNode }) { | ||
| return ( | ||
| <html lang="en"> | ||
| <body>{children}</body> | ||
| </html> | ||
| ); | ||
| } | ||
| `, | ||
| ); | ||
| writeFixtureFile( | ||
| root, | ||
| "app/page.tsx", | ||
| `export default function HomePage() { | ||
| return <main>home</main>; | ||
| } | ||
| `, | ||
| ); | ||
| writeFixtureFile( | ||
| root, | ||
| "app/intercepting-routes/layout.tsx", | ||
| `import type { ReactNode } from "react"; | ||
|
|
||
| export default function InterceptingLayout({ | ||
| children, | ||
| modal, | ||
| }: { | ||
| children: ReactNode; | ||
| modal: ReactNode; | ||
| }) { | ||
| return ( | ||
| <> | ||
| <div data-testid="gallery-shell">{children}</div> | ||
| {modal} | ||
| </> | ||
| ); | ||
| } | ||
| `, | ||
| ); | ||
| writeFixtureFile( | ||
| root, | ||
| "app/intercepting-routes/page.tsx", | ||
| `export default function GalleryPage() { | ||
| return <main>gallery</main>; | ||
| } | ||
| `, | ||
| ); | ||
| writeFixtureFile( | ||
| root, | ||
| "app/intercepting-routes/photo/[id]/page.tsx", | ||
| `export default function PhotoPage() { | ||
| return <main>standalone photo page</main>; | ||
| } | ||
| `, | ||
| ); | ||
| writeFixtureFile( | ||
| root, | ||
| "app/intercepting-routes/@modal/default.tsx", | ||
| `export default function ModalDefault() { | ||
| return null; | ||
| } | ||
| `, | ||
| ); | ||
| writeFixtureFile( | ||
| root, | ||
| "app/intercepting-routes/@modal/(.)photo/[id]/page.tsx", | ||
| `export default function PhotoModalPage() { | ||
| return <div>photo modal</div>; | ||
| } | ||
| `, | ||
| ); | ||
|
|
||
| await buildApp(root); | ||
|
|
||
| expect(fs.existsSync(path.join(root, "dist", "server", "index.js"))).toBe(true); | ||
| expect(fs.existsSync(path.join(root, "dist", "server", "ssr", "index.js"))).toBe(true); | ||
| expect(fs.existsSync(path.join(root, "dist", "client"))).toBe(true); | ||
| }); | ||
| }, 60_000); |
There was a problem hiding this comment.
This is a thorough integration test — it creates a full fixture from scratch, symlinks node_modules, and runs a real createBuilder + buildApp(). The 60-second timeout is appropriate for a production build test.
One minor observation: the symlink + writeFixtureFile pattern for creating fixtures inline is verbose but clear. If more build tests like this are added in the future, consider putting the fixture on disk in tests/fixtures/ instead — it would be easier to maintain and wouldn't need the writeFixtureFile calls.
|
Review posted. Here's a summary of my feedback: Overall: The PR is in good shape. The regression tests are well-targeted and the playground example is a clean addition. Key feedback:
|
2 participants
Summary
The fix for this bug (inherited parallel slots causing the same intercepting page to be counted multiple times during validation) has already been merged into main. This PR retains the regression tests and the
intercepting-routesplayground example so the behavior stays covered going forward.What's included
tests/intercepting-routes-build.test.ts— production build regression test: verifies a route structure with an inherited@modalslot and an intercepting(.)photo/[id]route builds successfullytests/routing.test.ts— unit regression test: verifies that inherited intercepting slots sharing the same target pattern are correctly allowed (not rejected as duplicates)examples/app-router-playground/app/intercepting-routes/— playground demo showing the intercepting routes pattern with a modal slotBackground
When a parallel slot is inherited by child routes, the same intercepting page can appear on multiple discovered routes. The old validation flattened all intercepted target patterns and treated repeated entries from inherited slots as separate route definitions, incorrectly throwing:
You cannot have two routes that resolve to the same pathThe fix (de-duplicating by target pattern before validation) landed in main separately. These tests guard against regressions.