fix(router): reuse resolved lazy route components - #8054
Conversation
|
View your CI Pipeline Execution ↗ for commit 8d93118
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview4 package(s) bumped directly, 19 bumped as dependents. 🟩 Patch bumps
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change reuses resolved client-side lazy route components during preload. React, Solid, and Vue router types accept undefined preload results. Tests cover repeated preloads and route revisits without pending UI. ChangesLazy route preload reuse
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Router
participant LazyRouteComponent
participant Importer
User->>Router: Navigate to code-split route
Router->>LazyRouteComponent: Preload route component
LazyRouteComponent->>LazyRouteComponent: Check resolved client component
alt Component already resolved
LazyRouteComponent-->>Router: Return without importing
else Component unresolved
LazyRouteComponent->>Importer: Load component
Importer-->>LazyRouteComponent: Resolve component
LazyRouteComponent-->>Router: Complete preload
end
Router-->>User: Render route without pending flicker
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bundle Size Benchmarks
The following scenarios have bundle-size changes compared with the baseline:
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. |
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/vue-router/src/lazyRouteComponent.tsx`:
- Around line 69-70: Update the preload logic around the preload function and
load so SSR retains the resolved loadPromise instead of invoking importer()
again; only clear the promise after successful browser-side loading, while
preserving unconditional clearing on rejection to allow failed-import retries.
🪄 Autofix
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 Plus
Run ID: be92081d-50d3-48fd-a0bc-a549d1747866
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.changeset/fuzzy-pandas-smile.mde2e/react-start/issue-8049/package.jsone2e/react-start/issue-8049/playwright.config.tse2e/react-start/issue-8049/src/routeTree.gen.tse2e/react-start/issue-8049/src/router.tsxe2e/react-start/issue-8049/src/routes/__root.tsxe2e/react-start/issue-8049/src/routes/index.tsxe2e/react-start/issue-8049/src/routes/test.tsxe2e/react-start/issue-8049/tests/issue-8049.spec.tse2e/react-start/issue-8049/tsconfig.jsone2e/react-start/issue-8049/vite.config.tspackages/react-router/src/lazyRouteComponent.tsxpackages/react-router/src/route.tsxpackages/react-router/tests/component-preload-retry.test.tsxpackages/router-core/src/route.tspackages/solid-router/src/lazyRouteComponent.tsxpackages/solid-router/src/route.tsxpackages/solid-router/tests/component-preload-retry.test.tsxpackages/vue-router/src/lazyRouteComponent.tsxpackages/vue-router/src/route.tspackages/vue-router/tests/component-preload-retry.test.tsx
Merging this PR will degrade performance by 5.02%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server error-paths not-found (vue) |
467.2 KB | 2,279.9 KB | -79.51% |
| ❌ | Memory | mem server peak-large-page (react) |
1.2 MB | 2.1 MB | -43.73% |
| ❌ | Memory | mem client unique-location-churn (solid) |
346.5 KB | 449.3 KB | -22.87% |
| ❌ | Memory | mem client unique-location-churn (react) |
667.3 KB | 757.6 KB | -11.92% |
| ❌ | Memory | mem client navigation-churn (vue) |
1.5 MB | 1.7 MB | -8.97% |
| ❌ | Simulation | client-nested-params navigation loop (react) |
212.5 ms | 228.4 ms | -6.99% |
| ❌ | Memory | mem client preload-churn (vue) |
758.9 KB | 802.1 KB | -5.39% |
| ❌ | Memory | mem client interrupted-navigations (vue) |
435.8 KB | 459.9 KB | -5.24% |
| ❌ | Memory | mem server peak-large-page (vue) |
1 MB | 1.1 MB | -4.93% |
| ❌ | Memory | mem server peak-large-page (solid) |
1 MB | 1.1 MB | -4.27% |
| ❌ | Memory | mem client navigation-churn (solid) |
650.1 KB | 672.4 KB | -3.32% |
| ❌ | Memory | mem server aborted-requests (vue) |
1,012.4 KB | 1,046.7 KB | -3.28% |
| ❌ | Memory | mem server error-paths error (vue) |
945.3 KB | 974.9 KB | -3.04% |
| ⚡ | Memory | mem server error-paths redirect (vue) |
648.5 KB | 401.6 KB | +61.48% |
| ⚡ | Memory | mem server error-paths not-found (solid) |
911.2 KB | 580.5 KB | +56.97% |
| ⚡ | Memory | mem server error-paths unmatched (react) |
629.2 KB | 435.7 KB | +44.41% |
| ⚡ | Memory | mem client unique-location-churn (vue) |
576.1 KB | 523.1 KB | +10.12% |
| ⚡ | Memory | mem server request-churn (react) |
710.7 KB | 666.5 KB | +6.62% |
| ⚡ | Simulation | client-route-tree-scale navigation loop (react) |
172.3 ms | 164.1 ms | +4.97% |
| ⚡ | Memory | mem server aborted-requests (solid) |
1.2 MB | 1.2 MB | +4.34% |
| ... | ... | ... | ... | ... | ... |
ℹ️ 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/issue-8049-lazy-route-revisit (8d93118) with main (3848503)
| defaultPendingMinMs: 0, | ||
| defaultPendingComponent: () => <div role="status">Loading page</div>, | ||
| }) | ||
|
|
There was a problem hiding this comment.
can we please use data-testid instead of roles?
also, how does this assert that the pending component is NOT shown?
can we use faketimers and a defaultPendingMinMs > 0 and then check inbetween that the pending component (by id) is not visible?
Summary
Testing
CI=1 NX_DAEMON=false pnpm nx run @tanstack/react-router:test:unit --outputStyle=stream --skipRemoteCache -- tests/component-preload-retry.test.tsxCI=1 NX_DAEMON=false pnpm nx run @tanstack/solid-router:test:unit --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run @tanstack/vue-router:test:unit --outputStyle=stream --skipRemoteCache -- tests/component-preload-retry.test.tsxCI=1 NX_DAEMON=false pnpm nx run-many --target=test:types --projects=@tanstack/router-core,@tanstack/react-router,@tanstack/solid-router,@tanstack/vue-router --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run-many --target=test:eslint --projects=@tanstack/router-core,@tanstack/react-router,@tanstack/solid-router,@tanstack/vue-router --outputStyle=stream --skipRemoteCacheCI=1 pnpm test:e2eine2e/react-start/issue-8049pnpm buildine2e/react-start/issue-8049Fixes #8049
Summary by CodeRabbit
Bug Fixes
Tests