Repository navigation
TanStack: Carry lazy route bindings onto cloned routes - #35500
valentinpalkovic merged 2 commits into
Conversation
|
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:
📝 WalkthroughWalkthrough
ChangesLazy route cloning
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 |
472405b to
7c5e4e0
Compare
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 21.39 MB | 21.31 MB | 🎉 -75 KB 🎉 |
| Dependency size | 36.75 MB | 36.75 MB | 🚨 +197 B 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 531 | 531 | 0 |
| Self size | 641 KB | 640 KB | 🎉 -1 KB 🎉 |
| Dependency size | 62.41 MB | 62.42 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 94 | 94 | 0 |
| Self size | 1.42 MB | 1.37 MB | 🎉 -48 KB 🎉 |
| Dependency size | 23.91 MB | 23.92 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 125 | 125 | 0 |
| Self size | 29 KB | 29 KB | 0 B |
| Dependency size | 25.90 MB | 25.91 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 83 | 83 | 0 |
| Self size | 32 KB | 32 KB | 🚨 +18 B 🚨 |
| Dependency size | 21.22 MB | 21.23 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 272 | 272 | 0 |
| Self size | 23 KB | 23 KB | 0 B |
| Dependency size | 48.19 MB | 48.20 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/tanstack-react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 84 | 84 | 0 |
| Self size | 113 KB | 113 KB | 🚨 +136 B 🚨 |
| Dependency size | 21.25 MB | 21.27 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 205 | 205 | 0 |
| Self size | 827 KB | 827 KB | 🎉 -32 B 🎉 |
| Dependency size | 91.60 MB | 91.53 MB | 🎉 -74 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 198 | 198 | 0 |
| Self size | 32 KB | 32 KB | 🚨 +36 B 🚨 |
| Dependency size | 90.08 MB | 90.00 MB | 🎉 -75 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 74 | 74 | 0 |
| Self size | 1.09 MB | 1.09 MB | 🚨 +53 B 🚨 |
| Dependency size | 58.14 MB | 58.06 MB | 🎉 -75 KB 🎉 |
| Bundle Size Analyzer | node | node |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 59 | 59 | 0 |
| Self size | 1.48 MB | 1.49 MB | 🚨 +12 KB 🚨 |
| Dependency size | 12.30 MB | 12.30 MB | 🚨 +137 B 🚨 |
| Bundle Size Analyzer | Link | Link |
Head branch was pushed to by a user without write access
.lazy() bindings live on the route instance as lazyFn, not in options, so carry them over for both nested clones and the rebuilt root, or clones lose their lazily-loaded component.
a5968e0 to
efdebf2
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/frameworks/tanstack-react/src/routing/duplicate-tree.test.ts (1)
121-124: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the provenance claim from this comment.
“Real routeTree.gen output” describes origin rather than maintenance rationale. Describe the required pathless-layout invariant directly.
Proposed revision
- // Real routeTree.gen output for `posts/_archive/archived.tsx`: the nested - // pathless layout keeps its id AND carries the pathful prefix as `path`. + // A nested pathless layout can retain its id while carrying a pathful + // prefix; clones must preserve the resulting composed route ids.As per coding guidelines, comments should explain maintenance-relevant rationale, not provenance claims.
🤖 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 `@code/frameworks/tanstack-react/src/routing/duplicate-tree.test.ts` around lines 121 - 124, Update the comment near the duplicate route-tree fixture to remove the “Real routeTree.gen output” provenance claim and describe only the required pathless-layout invariant: it retains its id while carrying the pathful prefix as path, so cloned routes compose identical ids for strict hook matching.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@code/frameworks/tanstack-react/src/routing/duplicate-tree.test.ts`:
- Around line 121-124: Update the comment near the duplicate route-tree fixture
to remove the “Real routeTree.gen output” provenance claim and describe only the
required pathless-layout invariant: it retains its id while carrying the pathful
prefix as path, so cloned routes compose identical ids for strict hook matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96dd504d-3244-4e48-8ad2-a24a0fcf380a
📒 Files selected for processing (2)
code/frameworks/tanstack-react/src/routing/duplicate-tree.test.tscode/frameworks/tanstack-react/src/routing/duplicate-tree.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/frameworks/tanstack-react/src/routing/duplicate-tree.ts
Follows #35499
What I did
Cloned routes lost their
.lazy()binding, so components from*.lazy.tsxfiles never rendered in stories.routeTree.genchains.lazy(() => import('./page.lazy'))on eager routes, and.lazy()stores the loader on the route instance (lazyFn), not in options,so an options-only clone drops it. The binding is now re-applied on the clone.Manual testing
cd code/frameworks/tanstack-react && yarn vitest run(55 tests).and a TanStack Start app:
https://github.com/unpunnyfuns/storybook-tanstack-conformance. The lazy
file route scenario passes with this fix applied.
AI disclosure
Root-cause analysis, fix, and tests developed with Claude (Claude Code), reviewed and submitted by me.
Summary by CodeRabbit
Summary by CodeRabbit