fix(routing): match inherited slot params against routePath, not request.url#1040
Conversation
…est.url Follow-up to #1039. Two related polish items in the inherited-slot mirror runtime: 1. Bug under `basePath`: `buildSlotOverrides` was parsing `request.url` to derive the URL parts to match against `slot.slotPatternParts`. The original request URL still carries the configured `basePath`, but `slotPatternParts` is built from app-relative segments only — so for every basePath-configured app, the pattern match silently failed and slot params fell back to the route's matched params. Use the already normalized `routePath` parameter (basePath stripped, RSC suffix removed) instead. Added a unit test that exercises the regression. 2. Memoize `findSlotSubPages` per build via a `WeakMap` keyed by the matcher. Inherited slots get scanned once per descendant route, so without memoization a route N segments deep paid O(N) full subtree walks for every shared ancestor slot. Cache lifetime tracks the matcher's, so a long-lived dev server doesn't accumulate state across builds. Also dedupes a redundant `convertSegmentsToRouteParts` call inside `findMirroredSlotPage` (both tiers needed the same conversion). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
commit: |
|
/bigbonk review |
|
There was a problem hiding this comment.
Clean follow-up. All three changes are correct and well-tested.
basePath fix — The root cause was straightforward: buildSlotOverrides was parsing request.url to get URL parts for matching against slotPatternParts, but slotPatternParts are app-relative (basePath already stripped during graph build). The routePath parameter is already the normalized form (basePath stripped, RSC suffix removed), so switching to it is the right fix. The new test directly exercises this by setting request.url to http://localhost/base/distinct/alice while routePath is /distinct/alice, proving the old code would have produced ["base","distinct","alice"] and silently failed the match.
Memoization — findSlotSubPages does synchronous fs.readdirSync + fs.existsSync walks, so caching per (matcher, slotDir) is a clear win for routes with deep inherited slot chains. The WeakMap<ValidFileMatcher, ...> keying is appropriate since ValidFileMatcher is an object created once per build (via createValidFileMatcher), and the cache naturally dies when the matcher is GC'd. Both call sites (line 332 in discoverSlotSubRoutes and line 907 in findMirroredSlotPage) only iterate the returned array without mutating it, so sharing the cached reference is safe.
Dedupe — Hoisting convertSegmentsToRouteParts above both tiers in findMirroredSlotPage is a trivial but correct cleanup. The conversion only depends on segmentsBelow which doesn't change between tiers.
CI is all green. Ship it.
|
Approved PR #1040. All three changes are correct:
CI is all green across all 30 checks. |
Follow-up to #1039, addressing two items I missed during review.
Summary
basePath:buildSlotOverrideswas deriving the URL parts to match againstslot.slotPatternPartsfromrequest.url. The original request URL still carries the configuredbasePath, butslotPatternPartsis built from app-relative segments only — so for any basePath-configured app, the pattern match silently failed and slot params fell back to the route's matched params (the exact bug fix(routing): mirror inherited parallel slots to descendant sub-pages #1039 was supposed to fix). Switched to the already-normalizedroutePathparameter (basePath stripped, RSC suffix removed) thatbuildPageElementsalready receives.findSlotSubPages: cached per-build via aWeakMapkeyed by the matcher. Inherited slots get scanned once per descendant route, so without memoization a route N segments deep paid O(N) full subtree walks for every shared ancestor slot. Cache lifetime tracks the matcher (one per build), so a long-lived dev server doesn't accumulate state across builds.findMirroredSlotPagewas callingconvertSegmentsToRouteParts(segmentsBelow)twice when the literal-tier match missed. Hoisted it out so both tiers reuse the conversion.Test plan
tests/app-page-element-builder.test.tsexercises the basePath regression: request URL ishttp://localhost/base/distinct/alicewhileroutePathis/distinct/alice, slot hasslotPatternParts: ["distinct", ":name"]. Without the fix,urlPartswould be["base","distinct","alice"], the match would fail, and the slot's resolvedparamswould silently lose thenamevalue. With the fix,slotParamsresolves to{ name: "alice" }.pnpm exec vp test run --project unit— 3524 tests pass.pnpm check— only pre-existing failure (benchmarks/vinext/vite.config.tscannot resolvevinextuntil the package is built; same onmain).🤖 Generated with Claude Code