Repository navigation
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds special-case handling in ChangesPathless Layout Route Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 |
|
Danger is currently blocked on labels. I do not have permission to add them from the fork; the requested sets appear to be: category \�ug, CI \ci:normal, and QA \qa:needed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/frameworks/tanstack-react/src/routing/duplicate-tree.ts (1)
64-98: ⚖️ Poor tradeoffConsider investigating stricter typing for route options (optional).
The function uses several
anytype assertions (lines 64, 76-81, 84) to work with TanStack Router's internal option types. While this is functional and may be necessary given TanStack's type system, stricter typing could improve type safety and IDE support if TanStack exports suitable utility types for route manipulation.However, this appears to be a broader pattern in the codebase and would require coordination with TanStack Router's type system design, so it's reasonable to defer.
🤖 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.ts` around lines 64 - 98, The code relies on repeated any casts for route options and cloning which weakens type safety; replace the broad any usages by importing/using TanStack Router's specific option types (e.g., the FileRoute or Route option/interface types) for variables like options, the createRoute call payload, cloned, children and cloneChild parameters, and the byId map value (instead of AnyRoute any-casts), and adjust the getParentRoute override and update/addChildren calls to use those concrete types; specifically, change the declarations that currently cast (oldRoute as any).options, the createRoute argument, the cloned variable, children typing, and cloneChild signature to use the library's exported route option/route instance types and update getOverrideFor/overrides to return correctly typed partial option overrides so you can remove the casts around createRoute, cloned.update, cloned.addChildren and byId.set.
🤖 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.
Nitpick comments:
In `@code/frameworks/tanstack-react/src/routing/duplicate-tree.ts`:
- Around line 64-98: The code relies on repeated any casts for route options and
cloning which weakens type safety; replace the broad any usages by
importing/using TanStack Router's specific option types (e.g., the FileRoute or
Route option/interface types) for variables like options, the createRoute call
payload, cloned, children and cloneChild parameters, and the byId map value
(instead of AnyRoute any-casts), and adjust the getParentRoute override and
update/addChildren calls to use those concrete types; specifically, change the
declarations that currently cast (oldRoute as any).options, the createRoute
argument, the cloned variable, children typing, and cloneChild signature to use
the library's exported route option/route instance types and update
getOverrideFor/overrides to return correctly typed partial option overrides so
you can remove the casts around createRoute, cloned.update, cloned.addChildren
and byId.set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbb90587-e03a-47cc-b290-e421206494ee
📒 Files selected for processing (2)
code/frameworks/tanstack-react/src/routing/duplicate-tree.test.tscode/frameworks/tanstack-react/src/routing/duplicate-tree.ts
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for your PR @MPiland!
Could you please help us to review it by providing a reproduction repository (e.g. using http://storybook.new/)?
The testing steps you provided are great (thanks!) but we still get a lot of PRs, so manually recreating reproductions makes it harder for us to review PRs quickly and with confidence.
Also, it looks like you could actually create config and stories with play functions to directly test your change within the repo. Stories and E2E tests are an acceptable form of reproduction for us, so you could also submit a story in the tanstack framework stories that matches your manual testing setup, and asserts everything renders as intended.
Thanks 🙏
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 72 | 0 |
| Self size | 21.07 MB | 21.07 MB | 0 B |
| Dependency size | 36.41 MB | 36.43 MB | 🚨 +25 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 204 | 204 | 0 |
| Self size | 802 KB | 802 KB | 0 B |
| Dependency size | 91.40 MB | 91.43 MB | 🚨 +25 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 197 | 197 | 0 |
| Self size | 32 KB | 32 KB | 🎉 -36 B 🎉 |
| Dependency size | 89.90 MB | 89.92 MB | 🚨 +25 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 1.08 MB | 1.08 MB | 🎉 -66 B 🎉 |
| Dependency size | 57.48 MB | 57.51 MB | 🚨 +25 KB 🚨 |
| Bundle Size Analyzer | node | node |
Thanks! I added an in-repo reproduction story under the TanStack framework template:
It covers the pathless layout route case with a nested child route and
I validated the package with:
I also generated the |
huang-julien
left a comment
huang-julien
left a comment
There was a problem hiding this comment.
While the fix works, the PR has unrelated changes.
Please disclose if/how did you use codex/AI agent on this PR.
723b09d to
97062f8
Compare
|
Hi, i'm closing this PR as it still involves multiple differents fixes. Please open smaller PRs that fixes a single bug. |
3 participants
What I did
Preserved pure pathless TanStack file routes, such as
/_authenticated, as id-only routes in the router mock instead of normalizing them topath: '/'.This keeps pathless layout routes in the duplicated story router and preserves route ids used by APIs like
getRouteApi('/_authenticated/...').Updated route tree duplication so id-only pathless layout routes stay id-only when cloned, while normal path routes still preserve their original file-route ids after being recreated under the duplicated parent.
The duplicated
__root__route now stays a fresh Storybook preview root and only applies explicitrouteOverrides.__root__options. It no longer copies source app root options, which can include TanStack Start document components such ashtml,head,body,HeadContent,Scripts, and app providers that are invalid inside Storybook's preview root.Added an in-repo TanStack framework story that reproduces both the pathless layout route case and the TanStack Start-style document root case.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
The new TanStack framework story covers the manual reproduction path:
PathlessLayoutRoutes.stories.tsxin the generated TanStack React Storybook.getRouteApi('/_authenticated/...')can read route context and params from the nested route.html,head,body,HeadContent,Scripts, and app providers) is not copied into Storybook's preview root.Documentation
MIGRATION.MD
Summary by CodeRabbit
New Features
Bug Fixes
Tests