fix(router-core): lane match loader rewrite#7805
Open
Sheraff wants to merge 22 commits into
Open
Conversation
Sheraff
changed the title
Contributor
|
View your CI Pipeline Execution ↗ for commit 3d252e6
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces legacy router match loading with client/server lane pipelines, shared SSR hydration, simplified stores, updated framework rendering and transition synchronization, targeted HMR refreshes, and extensive regression coverage across router-core, React, Solid, Vue, SSR, and benchmarks. ChangesRouter loading, hydration, and SSR
Framework adapters and tooling
Regression coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem client preload-churn (react) |
610 KB | 2,448.1 KB | -75.08% |
| ❌ | Memory | mem client preload-churn (solid) |
577.3 KB | 2,285 KB | -74.74% |
| ❌ | Memory | mem client preload-churn (vue) |
667.9 KB | 2,451.2 KB | -72.75% |
| ❌ | Memory | mem server error-paths not-found (solid) |
423.3 KB | 958.8 KB | -55.85% |
| ❌ | Memory | mem server error-paths redirect (vue) |
304.3 KB | 566.7 KB | -46.31% |
| ❌ | Memory | mem client unique-location-churn (react) |
729 KB | 1,259.9 KB | -42.14% |
| ❌ | Memory | mem server error-paths redirect (solid) |
441 KB | 757.4 KB | -41.77% |
| ❌ | Memory | mem client navigation-churn (react) |
429.2 KB | 715.7 KB | -40.03% |
| ❌ | Memory | mem client mount-unmount (react) |
510.9 KB | 828.1 KB | -38.3% |
| ❌ | Memory | mem server request-churn (react) |
492.9 KB | 709.9 KB | -30.58% |
| ❌ | Memory | mem client unique-location-churn (vue) |
1.1 MB | 1.5 MB | -26.59% |
| ❌ | Memory | mem client loader-data-retention (vue) |
594.9 KB | 773.8 KB | -23.12% |
| ❌ | Memory | mem client navigation-churn (solid) |
1.4 MB | 1.8 MB | -20.53% |
| ❌ | Memory | mem client navigation-churn (vue) |
989.2 KB | 1,238.4 KB | -20.12% |
| ❌ | Memory | mem server error-paths not-found (react) |
266.8 KB | 312.5 KB | -14.63% |
| ❌ | Simulation | client-async-pipeline navigation loop (vue) |
45.8 ms | 53.2 ms | -13.83% |
| ❌ | Memory | mem client unique-location-churn (solid) |
841 KB | 955.8 KB | -12.01% |
| ❌ | Simulation | client-rewrites navigation loop (vue) |
67.2 ms | 75.8 ms | -11.37% |
| ❌ | Memory | mem client interrupted-navigations (vue) |
870.7 KB | 973.1 KB | -10.52% |
| ❌ | Simulation | client-preload interaction loop (vue) |
55 ms | 58.3 ms | -5.74% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix-router-core-lane-match-loader (4150cdf) with main (11a6a0d)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vue-router/src/useMatch.tsx (1)
137-154: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPreserve the generic result type instead of erasing it with
any.
Vue.computed<any>allows the implementation to violate the declaredThrowOrOptional<UseMatchResult<...>>contract without a compiler error. Typematchandresultfrom the existing generic aliases.As per coding guidelines, “Use TypeScript strict mode with extensive type safety.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vue-router/src/useMatch.tsx` around lines 137 - 154, Update the computed result in the useMatch flow to preserve the existing generic result type instead of using Vue.computed<any>. Type match and result with the existing generic aliases, including the declared ThrowOrOptional<UseMatchResult<...>> contract, so incorrect return values are caught by TypeScript while preserving the current selection and missing-match behavior.Source: Coding guidelines
🧹 Nitpick comments (7)
e2e/solid-start/selective-ssr/tests/issue-7283-dev-hydration.spec.ts (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse braces for all conditional bodies.
These one-line
ifstatements violate the repository’s TypeScript control-flow convention.As per coding guidelines,
**/*.{ts,tsx,js,jsx}must use curly braces for control statements.Also applies to: 79-79, 84-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/solid-start/selective-ssr/tests/issue-7283-dev-hydration.spec.ts` at line 27, Update the one-line conditional statements in issue-7283-dev-hydration.spec.ts, including the checks around lines 27, 79, and 84, to wrap each conditional body in curly braces while preserving their existing behavior.Source: Coding guidelines
packages/solid-router/tests/transitioner-remount.test.tsx (1)
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid discarding location-state type safety.
Assert the partial state shape directly instead of casting to
any.Proposed change
- expect((router.state.location.state as any).remounted).toBe(true) + expect(router.state.location.state).toMatchObject({ remounted: true })As per coding guidelines, TypeScript files must use strict mode with extensive type safety.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/solid-router/tests/transitioner-remount.test.tsx` at line 54, Replace the any cast in the remounted-state assertion with a type-safe assertion of the partial location-state shape, using an appropriate typed access or narrowing approach so router.state.location.state remains checked by TypeScript.Source: Coding guidelines
packages/router-core/tests/fatal-load-rejection.test.ts (1)
25-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModel the error component without bypassing its route contract.
as anyprevents this test from detecting changes to theerrorComponentinterface. Use a callable mock component with itspreloadproperty attached, matching the runtime shape used elsewhere.Proposed fix
+ const ErrorComponent = Object.assign(() => null, { + preload: errorComponentPreload, + }) + const rootRoute = new BaseRootRoute({ loader: () => 'root data', - errorComponent: { preload: errorComponentPreload } as any, + errorComponent: ErrorComponent, })As per coding guidelines, TypeScript files must use strict mode with extensive type safety.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/tests/fatal-load-rejection.test.ts` around lines 25 - 30, Update the errorComponent setup in the BaseRootRoute test to use a typed callable mock component with its preload property attached, matching the runtime component shape. Remove the as any cast so TypeScript validates the errorComponent contract and the test catches interface changes.Source: Coding guidelines
packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts (1)
60-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the canonical replacement setup type-checked.
Both
as anycasts allow this regression test to keep compiling ifbuildLocationorcommitLocationchanges incompatibly. Please use their declared/internal option types or a typed helper for the Transitioner operation being reproduced.As per coding guidelines, TypeScript files must use strict mode with extensive type safety.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts` around lines 60 - 75, Remove the as any casts from the buildLocation and commitLocation calls in this regression test. Use the declared option types or a typed helper representing the Transitioner operation so both canonical replacement setup calls remain strictly type-checked while preserving their existing behavior.Source: Coding guidelines
packages/router-core/tests/issue-4444-param-parse-error-lazy-child.test.ts (1)
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the lazy child chunk was never requested.
The test can currently pass even if
lazyFnruns before the child is trimmed, leaving the documented regression partially uncovered.Proposed assertion
expect( router.state.matches.find((match) => match.status === 'pending'), ).toBeUndefined() +expect(lazyFn).not.toHaveBeenCalled() expect(router.state.isLoading).toBe(false)Also applies to: 60-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/tests/issue-4444-param-parse-error-lazy-child.test.ts` around lines 19 - 21, Update the test using the lazyFn mock so it explicitly asserts lazyFn was never called when the child route is trimmed after the parameter parse error. Place the assertion after the relevant navigation or error-handling flow, preserving the existing test behavior while verifying the lazy child chunk is not requested.packages/react-router/tests/transactional-loading.test.tsx (1)
130-148: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCapture every committed pair to verify atomic publication.
The final sequential assertions can miss a transient mixed-generation render after
childRefreshresolves. Record parent/child values on each committed render and assert that neitherparent-v2/child-v1norparent-v1/child-v2occurs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-router/tests/transactional-loading.test.tsx` around lines 130 - 148, Update the transactional loading test around the final refresh sequence to record the parent and child values on every committed render, then assert no mixed-generation pair occurs: parent-v2 with child-v1 or parent-v1 with child-v2. Keep the existing final-state assertions and use the render-observation mechanism already provided by the test setup.packages/router-core/src/router.ts (1)
1330-1338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent
catch {}hides listener failures.Swallowing all listener exceptions keeps one bad listener from breaking others, but the fully empty catch also hides real bugs during development. Consider logging in non-production.
♻️ Suggested dev logging
if (listener.eventType === routerEvent.type) { try { listener.fn(routerEvent) - } catch {} + } catch (err) { + if (process.env.NODE_ENV !== 'production') { + console.error( + `Error in router event listener for "${routerEvent.type}":`, + err, + ) + } + } }If tests assert fully-silent behavior, feel free to disregard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/src/router.ts` around lines 1330 - 1338, Update the catch block in the emit method to retain listener isolation while reporting listener exceptions in non-production environments. Preserve the existing behavior of continuing to notify other subscribers and keeping production logging silent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/solid-start/selective-ssr/src/routes/ssr-false-pending-min.tsx`:
- Around line 6-13: Update recordEvent to avoid casting window to any by
declaring a local Window augmentation for the __events event buffer, then
initialize and append through window.__events. Preserve the existing server
guard and event shape, including the performance timestamp.
In `@packages/react-router/tests/router.test.tsx`:
- Around line 2463-2467: Update the navigation assertion in the router test to
target the parent route’s specific not-found boundary, using its unique test
identifier or boundary-specific text instead of the broad “Not Found” match;
keep the existing home-page and navigation steps unchanged.
In `@packages/router-core/tests/issue-6221-head-waits-for-loader.test.ts`:
- Line 23: Update the authentication check in the relevant test to wrap the
notFound call inside curly braces, preserving the existing condition and
behavior.
In `@packages/solid-router/src/Transitioner.tsx`:
- Around line 47-65: Move the Solid.onCleanup registration for unsub immediately
after router.history.subscribe(router.load), before the early return in the
Transitioner setup. Keep the existing cleanup callback unchanged and remove the
later registration so every execution path disposes the history subscription.
In `@packages/solid-router/tests/on-rendered-change-info.test.tsx`:
- Around line 44-61: Update the onRendered transition handling to call
getLocationChangeInfo with the previously rendered location as the first
argument and the latest resolved location as the second. Ensure fromLocation,
toLocation, and change flags reflect the actual transition, including same-href
state changes; locate the change in the code that constructs the onRendered
event.
In `@packages/vue-router/tests/pending-fallback-promise-replacement.test.tsx`:
- Around line 13-18: Update the afterEach cleanup hook to drain and await all
testCleanups while fake timers remain active, then restore real timers in a
finally block so timer state is reset even if cleanup fails. Preserve the
existing cleanup() call after the callbacks settle.
---
Outside diff comments:
In `@packages/vue-router/src/useMatch.tsx`:
- Around line 137-154: Update the computed result in the useMatch flow to
preserve the existing generic result type instead of using Vue.computed<any>.
Type match and result with the existing generic aliases, including the declared
ThrowOrOptional<UseMatchResult<...>> contract, so incorrect return values are
caught by TypeScript while preserving the current selection and missing-match
behavior.
---
Nitpick comments:
In `@e2e/solid-start/selective-ssr/tests/issue-7283-dev-hydration.spec.ts`:
- Line 27: Update the one-line conditional statements in
issue-7283-dev-hydration.spec.ts, including the checks around lines 27, 79, and
84, to wrap each conditional body in curly braces while preserving their
existing behavior.
In `@packages/react-router/tests/transactional-loading.test.tsx`:
- Around line 130-148: Update the transactional loading test around the final
refresh sequence to record the parent and child values on every committed
render, then assert no mixed-generation pair occurs: parent-v2 with child-v1 or
parent-v1 with child-v2. Keep the existing final-state assertions and use the
render-observation mechanism already provided by the test setup.
In `@packages/router-core/src/router.ts`:
- Around line 1330-1338: Update the catch block in the emit method to retain
listener isolation while reporting listener exceptions in non-production
environments. Preserve the existing behavior of continuing to notify other
subscribers and keeping production logging silent.
In `@packages/router-core/tests/fatal-load-rejection.test.ts`:
- Around line 25-30: Update the errorComponent setup in the BaseRootRoute test
to use a typed callable mock component with its preload property attached,
matching the runtime component shape. Remove the as any cast so TypeScript
validates the errorComponent contract and the test catches interface changes.
In `@packages/router-core/tests/issue-4444-param-parse-error-lazy-child.test.ts`:
- Around line 19-21: Update the test using the lazyFn mock so it explicitly
asserts lazyFn was never called when the child route is trimmed after the
parameter parse error. Place the assertion after the relevant navigation or
error-handling flow, preserving the existing test behavior while verifying the
lazy child chunk is not requested.
In
`@packages/router-core/tests/issue-6371-search-default-normalization-abort.test.ts`:
- Around line 60-75: Remove the as any casts from the buildLocation and
commitLocation calls in this regression test. Use the declared option types or a
typed helper representing the Transitioner operation so both canonical
replacement setup calls remain strictly type-checked while preserving their
existing behavior.
In `@packages/solid-router/tests/transitioner-remount.test.tsx`:
- Line 54: Replace the any cast in the remounted-state assertion with a
type-safe assertion of the partial location-state shape, using an appropriate
typed access or narrowing approach so router.state.location.state remains
checked by TypeScript.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9eff563-1e9d-4b5e-9e0d-17c99dcf056c
📒 Files selected for processing (162)
benchmarks/memory/client/scenarios/interrupted-navigations/shared.tse2e/solid-start/selective-ssr/src/routeTree.gen.tse2e/solid-start/selective-ssr/src/routes/ssr-false-pending-min.tsxe2e/solid-start/selective-ssr/tests/issue-7283-dev-hydration.spec.tspackages/react-router/src/Match.tsxpackages/react-router/src/Matches.tsxpackages/react-router/src/Transitioner.tsxpackages/react-router/src/lazyRouteComponent.tsxpackages/react-router/src/ssr/renderRouterToStream.tsxpackages/react-router/src/ssr/renderRouterToString.tsxpackages/react-router/tests/Matches.test.tsxpackages/react-router/tests/ancestor-loader-child-pending-min.test.tsxpackages/react-router/tests/component-preload-retry-pending-min.test.tsxpackages/react-router/tests/component-preload-retry.test.tsxpackages/react-router/tests/errorComponent.test.tsxpackages/react-router/tests/hydration-capped-boundary-pending.test.tsxpackages/react-router/tests/hydration-terminal-lane.test.tsxpackages/react-router/tests/issue-4759-pending-frame.test.tsxpackages/react-router/tests/issue-7051-force-pending-suspense.test.tsxpackages/react-router/tests/issue-7367-pending-min-redirect.test.tsxpackages/react-router/tests/issue-7457-redirect-chain-first-load.test.tsxpackages/react-router/tests/issue-7638-invalidate-transition-error.test.tsxpackages/react-router/tests/loaders.test.tsxpackages/react-router/tests/on-rendered-same-href-state.test.tsxpackages/react-router/tests/pending-fallback-promise-replacement.test.tsxpackages/react-router/tests/preloaded-mount-resolution.test.tsxpackages/react-router/tests/redirect.test.tsxpackages/react-router/tests/root-pending-min.test.tsxpackages/react-router/tests/router.test.tsxpackages/react-router/tests/store-updates-during-navigation.test.tsxpackages/react-router/tests/transactional-loading.test.tsxpackages/react-router/tests/transitioner-listener-errors.test.tsxpackages/react-router/tests/transitioner-render-ack.test.tsxpackages/react-router/tests/transitioner-router-swap.test.tsxpackages/react-router/tests/useNavigate.test.tsxpackages/router-core/src/Matches.tspackages/router-core/src/hydrate.tspackages/router-core/src/isServer/client.tspackages/router-core/src/isServer/development.tspackages/router-core/src/isServer/server.tspackages/router-core/src/load-matches.tspackages/router-core/src/load.client.tspackages/router-core/src/load.server.tspackages/router-core/src/redirect.tspackages/router-core/src/route-chunks.tspackages/router-core/src/route.tspackages/router-core/src/router.tspackages/router-core/src/ssr/createRequestHandler.tspackages/router-core/src/ssr/ssr-client.tspackages/router-core/src/ssr/ssr-server.tspackages/router-core/src/stores.tspackages/router-core/tests/background-assets-stale.test.tspackages/router-core/tests/background-trim-abort.test.tspackages/router-core/tests/blocked-navigation-current-load.test.tspackages/router-core/tests/boundary-component-chunk.test.tspackages/router-core/tests/callbacks.test.tspackages/router-core/tests/chunk-failure-lifecycle.test.tspackages/router-core/tests/client-lane-adversarial.test.tspackages/router-core/tests/fatal-load-rejection.test.tspackages/router-core/tests/granular-stores.test.tspackages/router-core/tests/hydrate.test.tspackages/router-core/tests/hydrated-stay-match-data.test.tspackages/router-core/tests/hydration-asset-context-order.test.tspackages/router-core/tests/hydration-boundary-chunks.test.tspackages/router-core/tests/hydration-currentness.test.tspackages/router-core/tests/invalidate-pre-rematch-failure.test.tspackages/router-core/tests/issue-3928-rapid-reload-abort.test.tspackages/router-core/tests/issue-4078-loader-notfound-root-boundary.test.tspackages/router-core/tests/issue-4444-param-parse-error-lazy-child.test.tspackages/router-core/tests/issue-4476-preload-navigate-abort.test.tspackages/router-core/tests/issue-4572-preload-root-beforeload-flags.test.tspackages/router-core/tests/issue-4684-head-on-beforeload-error.test.tspackages/router-core/tests/issue-5106-hydrated-notfound-boundary.test.tspackages/router-core/tests/issue-6107-preload-chunk-failure.test.tspackages/router-core/tests/issue-6221-head-waits-for-loader.test.tspackages/router-core/tests/issue-6351-fuzzy-notfound-layout.test.tspackages/router-core/tests/issue-6371-search-default-normalization-abort.test.tspackages/router-core/tests/issue-7379-head-matches-direct-load.test.tspackages/router-core/tests/issue-7635-dehydrated-error-child-head.test.tspackages/router-core/tests/load-route-chunk.test.tspackages/router-core/tests/load.test.tspackages/router-core/tests/preflight-reentrant-context.test.tspackages/router-core/tests/preload-adoption.test.tspackages/router-core/tests/preload-background-parent-coherence.test.tspackages/router-core/tests/preload-public-cache-behavior.test.tspackages/router-core/tests/preload-public-signal-lifetime.test.tspackages/router-core/tests/routerTestUtils.tspackages/router-core/tests/serial-failure-foreground-prefix.test.tspackages/router-core/tests/server-async-headers-decorative-hang.test.tspackages/router-core/tests/server-beforeload-preload-flag.test.tspackages/router-core/tests/server-chunk-failure-lifecycle.test.tspackages/router-core/tests/server-concurrent-error-notfound.test.tspackages/router-core/tests/server-headers-asset-failure.test.tspackages/router-core/tests/server-loader-abort-error.test.tspackages/router-core/tests/server-serial-ssr-notfound.test.tspackages/router-core/tests/server-ssr-false-assets.test.tspackages/router-core/tests/server-ssr-option-error.test.tspackages/router-core/tests/stay-match-abort.test.tspackages/router-core/tests/superseded-load-await.test.tspackages/router-devtools-core/src/AgeTicker.tsxpackages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsxpackages/router-plugin/src/core/hmr/handle-route-update.tspackages/router-plugin/tests/add-hmr.test.tspackages/router-plugin/tests/add-hmr/snapshots/react/arrow-function@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/arrow-function@webpack-hot.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/createRootRoute-inline-component@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/createRootRoute@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/createRootRouteWithContext-type-args@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/explicit-undefined-component@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/function-declaration@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/multi-component@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/react/string-literal-keys@true.tsxpackages/router-plugin/tests/add-hmr/snapshots/solid/arrow-function@true.tsxpackages/router-plugin/tests/handle-route-update.test.tspackages/solid-router/src/ClientOnly.tsxpackages/solid-router/src/Match.tsxpackages/solid-router/src/Matches.tsxpackages/solid-router/src/Transitioner.tsxpackages/solid-router/src/lazyRouteComponent.tsxpackages/solid-router/src/matchContext.tsxpackages/solid-router/src/routerStores.tspackages/solid-router/src/ssr/renderRouterToStream.tsxpackages/solid-router/src/ssr/renderRouterToString.tsxpackages/solid-router/src/useMatch.tsxpackages/solid-router/tests/Transitioner.test.tsxpackages/solid-router/tests/component-preload-retry.test.tsxpackages/solid-router/tests/link.test.tsxpackages/solid-router/tests/loaders.test.tsxpackages/solid-router/tests/on-rendered-change-info.test.tsxpackages/solid-router/tests/pending-fallback-promise-replacement.test.tsxpackages/solid-router/tests/redirect.test.tsxpackages/solid-router/tests/router.test.tsxpackages/solid-router/tests/same-route-pending-blank.test.tsxpackages/solid-router/tests/server/errorComponent.test.tsxpackages/solid-router/tests/store-updates-during-navigation.test.tsxpackages/solid-router/tests/transitioner-listener-errors.test.tsxpackages/solid-router/tests/transitioner-remount.test.tsxpackages/solid-router/tests/transitioner-render-ack.test.tsxpackages/solid-router/tests/use-match-outgoing-transition.test.tsxpackages/start-server-core/src/createStartHandler.tspackages/vue-router/src/Match.tsxpackages/vue-router/src/Matches.tsxpackages/vue-router/src/Transitioner.tsxpackages/vue-router/src/lazyRouteComponent.tsxpackages/vue-router/src/matchContext.tsxpackages/vue-router/src/routerStores.tspackages/vue-router/src/ssr/renderRouterToStream.tsxpackages/vue-router/src/ssr/renderRouterToString.tsxpackages/vue-router/src/useMatch.tsxpackages/vue-router/tests/Matches.test.tsxpackages/vue-router/tests/Transitioner.test.tsxpackages/vue-router/tests/component-preload-retry.test.tsxpackages/vue-router/tests/hydration-capped-boundary-pending.test.tsxpackages/vue-router/tests/link.test.tsxpackages/vue-router/tests/loaders.test.tsxpackages/vue-router/tests/pending-fallback-promise-replacement.test.tsxpackages/vue-router/tests/redirect.test.tsxpackages/vue-router/tests/router.test.tsxpackages/vue-router/tests/store-updates-during-navigation.test.tsxpackages/vue-router/tests/transitioner-idle-after-render.test.tsxpackages/vue-router/tests/transitioner-listener-errors.test.tsxpackages/vue-router/tests/transitioner-remount-rendered.test.tsx
💤 Files with no reviewable changes (13)
- packages/router-plugin/tests/add-hmr.test.ts
- packages/solid-router/src/matchContext.tsx
- packages/router-core/src/load-matches.ts
- packages/solid-router/tests/server/errorComponent.test.tsx
- packages/solid-router/tests/Transitioner.test.tsx
- packages/router-core/src/redirect.ts
- packages/vue-router/src/routerStores.ts
- packages/router-core/src/ssr/ssr-server.ts
- packages/solid-router/src/routerStores.ts
- packages/router-core/tests/callbacks.test.ts
- packages/vue-router/tests/redirect.test.tsx
- packages/solid-router/tests/redirect.test.tsx
- benchmarks/memory/client/scenarios/interrupted-navigations/shared.ts
Comment on lines
+6
to
+13
| function recordEvent(type: string) { | ||
| if (typeof window === 'undefined') { | ||
| return | ||
| } | ||
|
|
||
| const global = window as any | ||
| global.__events ??= [] | ||
| global.__events.push({ type, t: performance.now() }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- target file ---'
cat -n e2e/solid-start/selective-ssr/src/routes/ssr-false-pending-min.tsx
echo '--- search __events ---'
rg -n "__events|recordEvent|interface Window|declare global" e2e/solid-start/selective-ssr/src/routes -S
echo '--- search repo-wide Window augmentation ---'
rg -n "interface Window|declare global" -S .Repository: TanStack/router
Length of output: 4375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,80p' e2e/solid-start/selective-ssr/src/routes/ssr-false-pending-min.tsx | cat -n
printf '\n--- related typings ---\n'
rg -n "window\\.__(events|events)|__events|interface Window|declare global" -S e2e/solid-start/selective-ssr src . \
--glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'Repository: TanStack/router
Length of output: 3954
Type the event buffer instead of casting window to any. window as any bypasses strict TS here; add a local Window augmentation for __events and push through window.__events.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/solid-start/selective-ssr/src/routes/ssr-false-pending-min.tsx` around
lines 6 - 13, Update recordEvent to avoid casting window to any by declaring a
local Window augmentation for the __events event buffer, then initialize and
append through window.__events. Preserve the existing server guard and event
shape, including the performance timestamp.
Source: Coding guidelines
Comment on lines
+44
to
+61
| const event = onRendered.mock.calls[0]![0] | ||
| expect(event.fromLocation?.pathname).toBe('/') | ||
| expect(event.toLocation.pathname).toBe('/next') | ||
| expect(event.pathChanged).toBe(true) | ||
| expect(event.hrefChanged).toBe(true) | ||
|
|
||
| await router.navigate({ | ||
| to: '/next', | ||
| state: { sameHrefState: true } as any, | ||
| }) | ||
| await waitFor(() => expect(onRendered).toHaveBeenCalledTimes(2)) | ||
|
|
||
| const stateEvent = onRendered.mock.calls[1]![0] | ||
| expect(stateEvent.fromLocation?.state.sameHrefState).toBeUndefined() | ||
| expect(stateEvent.toLocation.state.sameHrefState).toBe(true) | ||
| expect(stateEvent.fromLocation?.href).toBe('/next') | ||
| expect(stateEvent.toLocation.href).toBe('/next') | ||
| expect(stateEvent.hrefChanged).toBe(false) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
file="$(fd -a '^Transitioner\.tsx$' packages/solid-router/src | head -n1)"
ast-grep outline "$file" --items all --match 'Transitioner'
rg -n -C5 '\bonRendered\b|getLocationChangeInfo\s*\(' "$file"Repository: TanStack/router
Length of output: 648
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first, then inspect the helper and its call sites.
ast-grep outline packages/solid-router/src/Transitioner.tsx --view expanded
ast-grep outline packages/solid-router/src --view expanded | sed -n '1,220p'
rg -n -C 4 'getLocationChangeInfo|type: .onRendered.|latestLocation|previous.*location|rendered' packages/solid-router/src packages/solid-router/testsRepository: TanStack/router
Length of output: 38858
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper implementation and nearby tests/usages that define its semantics.
rg -n -C 6 'export function getLocationChangeInfo|function getLocationChangeInfo|getLocationChangeInfo\(' packages -g '!**/dist/**' -g '!**/build/**'
# If the implementation lives in router-core, map that file before reading it.
helper_file="$(rg -l 'function getLocationChangeInfo|export function getLocationChangeInfo' packages -g '!**/dist/**' -g '!**/build/**' | head -n1 || true)"
if [ -n "${helper_file:-}" ]; then
wc -l "$helper_file"
cat -n "$helper_file" | sed -n '1,240p'
fiRepository: TanStack/router
Length of output: 16905
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the helper definition and the onRendered event shape to resolve the semantics.
sed -n '918,955p' packages/router-core/src/router.ts
rg -n -C 4 "onRendered|LocationChangeInfo|fromLocation|toLocation|pathChanged|hrefChanged" packages/router-core/src/router.ts packages/solid-router/tests/on-rendered-change-info.test.tsx packages/solid-router/src/Transitioner.tsxRepository: TanStack/router
Length of output: 16395
Pass the latest location into getLocationChangeInfo
onRendered should report the transition from the previously rendered location to the new one. getLocationChangeInfo(resolvedLocation, resolvedLocation) makes both sides identical, so fromLocation/toLocation and the change flags no longer match the expected event shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/solid-router/tests/on-rendered-change-info.test.tsx` around lines 44
- 61, Update the onRendered transition handling to call getLocationChangeInfo
with the previously rendered location as the first argument and the latest
resolved location as the second. Ensure fromLocation, toLocation, and change
flags reflect the actual transition, including same-href state changes; locate
the change in the code that constructs the onRendered event.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/router-core/tests/preload-beforeload-reuse.test.ts (1)
222-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a same-route
loaderDepscase. This test only changes the parent match; it doesn’t cover reusing preloaded child context when the route’s own deps change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/tests/preload-beforeload-reuse.test.ts` around lines 222 - 280, Extend the test around childBeforeLoad to add a same-route loaderDeps scenario where the child route’s own dependencies change between preload and navigation. Configure childRoute with loaderDeps and assert the preloaded child context is not reused, including updated context values and the expected preload flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router-core/tests/preload-beforeload-reuse.test.ts`:
- Around line 242-245: Update the regression case around loaderDeps and context
to preserve type checking for the version contract: add a typed validateSearch
definition for version, replace Record<string, any> with the inferred or
explicit validated search type, and remove the as any casts around preloadRoute,
navigate, and beforeLoad replacement by using typed route refresh helpers or a
type-compatible mock.
---
Nitpick comments:
In `@packages/router-core/tests/preload-beforeload-reuse.test.ts`:
- Around line 222-280: Extend the test around childBeforeLoad to add a
same-route loaderDeps scenario where the child route’s own dependencies change
between preload and navigation. Configure childRoute with loaderDeps and assert
the preloaded child context is not reused, including updated context values and
the expected preload flags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: beb16cf3-124d-4a1b-ae41-86535af7b3c1
📒 Files selected for processing (3)
docs/router/guide/preloading.mdpackages/router-core/src/load-client.tspackages/router-core/tests/preload-beforeload-reuse.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-core/src/load-client.ts
Comment on lines
+242
to
+245
| loaderDeps: ({ search }: { search: Record<string, any> }) => ({ | ||
| version: Number(search.version), | ||
| }), | ||
| context: ({ deps }: { deps: { version: number } }) => ({ |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='packages/router-core/tests/preload-beforeload-reuse.test.ts'
# Locate all explicit any escapes in this test.
ast-grep run --pattern '$EXPR as any' --lang typescript "$file"
rg -n '\bany\b' "$file"
# Find established typed search-validation patterns in adjacent tests.
rg -n -C8 --type ts 'validateSearch:|loaderDeps:' packages/router-core/testsRepository: TanStack/router
Length of output: 40432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='packages/router-core/tests/preload-beforeload-reuse.test.ts'
# Read the relevant sections around the flagged lines.
sed -n '220,380p' "$file"
# Look for local helpers or typed fixtures used in this file.
rg -n -C4 'createTestRouter|as any|validateSearch|loaderDeps|beforeLoad|preloadRoute|navigate' "$file"Repository: TanStack/router
Length of output: 21621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find typed route-option replacement patterns and search validation in nearby tests.
rg -n -C3 '_refreshRoute|options\.beforeLoad\s*=|validateSearch:' packages/router-core/tests/*.test.ts packages/router-core/tests/*.test-d.ts
# Inspect the router test utilities for any helper that already handles route refreshes or typed route replacement.
ast-grep outline packages/router-core/tests/routerTestUtils.ts --view expanded
sed -n '1,260p' packages/router-core/tests/routerTestUtils.tsRepository: TanStack/router
Length of output: 16839
Keep this regression case type-checked. Record<string, any> and the as any casts around preloadRoute/navigate and beforeLoad replacement bypass the version contract. Add a typed validateSearch for version, then use typed route refresh helpers or assign a type-compatible mock directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router-core/tests/preload-beforeload-reuse.test.ts` around lines 242
- 245, Update the regression case around loaderDeps and context to preserve type
checking for the version contract: add a typed validateSearch definition for
version, replace Record<string, any> with the inferred or explicit validated
search type, and remove the as any casts around preloadRoute, navigate, and
beforeLoad replacement by using typed route refresh helpers or a type-compatible
mock.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router-core/tests/hydration-currentness.test.ts`:
- Around line 205-232: Extract the repeated mockWindow.$_TSR scaffolding in the
hydration currentness tests into a helper such as mockDehydratedRouter,
accepting only the matches array and preserving the shared manifest,
dehydratedData, callbacks, buffer, and initialized values. Replace the
duplicated setups at the referenced test blocks with this helper while keeping
each test’s matches payload unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dae5e328-1744-4525-a06b-e2022da66f47
📒 Files selected for processing (10)
e2e/react-start/basic/src/routes/specialChars/search.tsxe2e/react-start/basic/tests/special-characters.spec.tspackages/react-router/src/Transitioner.tsxpackages/router-core/src/load-client.tspackages/router-core/src/router.tspackages/router-core/tests/hydrated-stay-match-data.test.tspackages/router-core/tests/hydration-currentness.test.tspackages/solid-router/src/Transitioner.tsxpackages/solid-router/tests/transitioner-render-ack.test.tsxpackages/vue-router/src/Transitioner.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/solid-router/tests/transitioner-render-ack.test.tsx
- packages/react-router/src/Transitioner.tsx
- packages/router-core/tests/hydrated-stay-match-data.test.ts
- packages/solid-router/src/Transitioner.tsx
- packages/vue-router/src/Transitioner.tsx
- packages/router-core/src/load-client.ts
- packages/router-core/src/router.ts
Comment on lines
+205
to
+232
| mockWindow.$_TSR = { | ||
| router: { | ||
| manifest: testManifest, | ||
| dehydratedData: {}, | ||
| matches: [ | ||
| { | ||
| i: oldMatches[0]!.id, | ||
| s: 'success', | ||
| ssr: true, | ||
| u: Date.now(), | ||
| }, | ||
| { | ||
| i: oldMatches[1]!.id, | ||
| s: 'success', | ||
| ssr: 'data-only', | ||
| b: { source: 'old-server' }, | ||
| l: 'old-server', | ||
| u: Date.now(), | ||
| }, | ||
| ], | ||
| }, | ||
| h: vi.fn(), | ||
| e: vi.fn(), | ||
| c: vi.fn(), | ||
| p: vi.fn(), | ||
| buffer: [], | ||
| initialized: false, | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate mockWindow.$_TSR scaffolding across tests.
Each test rebuilds an almost identical { router: { manifest, dehydratedData, matches: [...] }, h, e, c, p, buffer, initialized } shell, differing only in the matches payload. Consider extracting a small helper (e.g. mockDehydratedRouter(matches)) that returns the boilerplate object, taking only the matches array as a parameter, to cut repetition across this (and likely other) tests in the file.
♻️ Example helper extraction
function mockDehydratedRouter(matches: TsrSsrGlobal['router']['matches']) {
return {
router: { manifest: testManifest, dehydratedData: {}, matches },
h: vi.fn(),
e: vi.fn(),
c: vi.fn(),
p: vi.fn(),
buffer: [],
initialized: false,
}
}Also applies to: 268-295, 325-351, 388-414
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router-core/tests/hydration-currentness.test.ts` around lines 205 -
232, Extract the repeated mockWindow.$_TSR scaffolding in the hydration
currentness tests into a helper such as mockDehydratedRouter, accepting only the
matches array and preserving the shared manifest, dehydratedData, callbacks,
buffer, and initialized values. Replace the duplicated setups at the referenced
test blocks with this helper while keeping each test’s matches payload
unchanged.
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…match-loader # Conflicts: # packages/router-core/tests/issue-4684-head-on-beforeload-error.test.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
We identified the failure in tanstack-react-start-e2e-basic:test:e2e--rsbuild-spa as an environment-level port conflict: both the dummy external server and the app server are configured to bind to port 43301, causing an EADDRINUSE error unrelated to our code changes. Our PR only modifies UI components and test assertions, with no changes to server or port configuration, so this failure cannot be attributed to this PR.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2980
Fixes #3179
Fixes #4444
Fixes #4572
Fixes #4684
Fixes #4759
Fixes #5106
Fixes #6107
Fixes #6221
Fixes #6351
Fixes #7283
Fixes #7379
Fixes #7602
Fixes #7635
Fixes #7120
Fixes #4614
Fixes #7673
Fixes #7457
Should fix (to verify) #3928
In-scope, but should be fixed already #2182
In-scope, but should be fixed already #2905
In-scope, but should be fixed already #3293
In-scope, but should be fixed already #4476
In-scope, but should be fixed already #4696
In-scope, but should be fixed already #6371
In-scope, but should be fixed already #7367
In-scope, but should be fixed already #7638
Not a bug #7110
In-scope, but not fixed, maybe we should #5778 (explicitly documented as a known expected failure.)
Not in scope #7633
Summary by CodeRabbit
Summary
New Features
/ssr-false-pending-min) and related E2E coverage.Bug Fixes
Documentation
beforeLoadand context reuse rules.