fix: upgrade TanStack Store to latest#7824
Conversation
📝 WalkthroughWalkthroughTanStack Store dependencies are upgraded, React Router store reads migrate to ChangesStore selector migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
|
View your CI Pipeline Execution ↗ for commit f62402e
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-router/package.json (1)
96-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlan the
useStoremigration with this bump.packages/react-router/srcstill usesuseStorein many places, and@tanstack/react-store0.11.x deprecates it in favor ofuseSelector.🤖 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/package.json` at line 96, Plan and begin migrating the useStore calls throughout packages/react-router/src to the useSelector API required by `@tanstack/react-store` 0.11.x, updating each affected component or hook while preserving existing selected state and subscription behavior.
🤖 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 `@packages/react-router/package.json`:
- Line 96: Plan and begin migrating the useStore calls throughout
packages/react-router/src to the useSelector API required by
`@tanstack/react-store` 0.11.x, updating each affected component or hook while
preserving existing selected state and subscription behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1052d019-c40d-499f-a83f-e5c4f94c4699
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
packages/react-router/package.jsonpackages/react-router/src/Match.tsxpackages/router-core/package.jsonpackages/vue-router/package.jsonpackages/vue-router/src/Match.tsxpackages/vue-router/src/Matches.tsxpackages/vue-router/src/Scripts.tsxpackages/vue-router/src/Transitioner.tsxpackages/vue-router/src/headContentUtils.tsxpackages/vue-router/src/link.tsxpackages/vue-router/src/not-found.tsxpackages/vue-router/src/useCanGoBack.tspackages/vue-router/src/useLocation.tsxpackages/vue-router/src/useMatch.tsxpackages/vue-router/src/useRouterState.tsxpackages/vue-router/src/useStore.ts
Merging this PR will improve performance by 20.62%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server error-paths not-found (solid) |
422.4 KB | 1,068.4 KB | -60.47% |
| ❌ | Memory | mem server error-paths unmatched (react) |
264.2 KB | 364.9 KB | -27.61% |
| ❌ | Memory | mem client preload-churn (vue) |
666.7 KB | 842.3 KB | -20.85% |
| ❌ | Memory | mem server peak-large-page (solid) |
771.4 KB | 877.2 KB | -12.06% |
| ❌ | Memory | mem server error-paths not-found (react) |
267 KB | 301 KB | -11.29% |
| ❌ | Simulation | client-history navigation loop (react) |
38.9 ms | 41.5 ms | -6.2% |
| ❌ | Simulation | client-route-tree-scale navigation loop (react) |
74 ms | 77.4 ms | -4.44% |
| ❌ | Simulation | ssr dehydrate rich types (solid) |
73.3 ms | 75.7 ms | -3.12% |
| ⚡ | Memory | mem server server-fn-churn (solid) |
3,825.7 KB | 273.9 KB | ×14 |
| ⚡ | Memory | mem client navigation-churn (vue) |
985.9 KB | 379 KB | ×2.6 |
| ⚡ | Memory | mem server error-paths redirect (vue) |
611.4 KB | 308.9 KB | +97.91% |
| ⚡ | Memory | mem server server-fn-churn (vue) |
3.9 MB | 2.4 MB | +65.39% |
| ⚡ | Memory | mem client unique-location-churn (vue) |
1,160.3 KB | 736.3 KB | +57.59% |
| ⚡ | Simulation | client-links navigation loop (vue) |
173.1 ms | 122.4 ms | +41.42% |
| ⚡ | Simulation | ssr head (vue) |
326.7 ms | 236.4 ms | +38.2% |
| ⚡ | Simulation | client-side navigation loop (vue) |
77.2 ms | 56.8 ms | +35.75% |
| ⚡ | Simulation | client-head navigation loop (vue) |
97.3 ms | 74.3 ms | +30.92% |
| ⚡ | Simulation | ssr assets linked-css control (vue) |
215.4 ms | 165.3 ms | +30.28% |
| ⚡ | Simulation | ssr assets inline-css cdn (vue) |
188.3 ms | 145 ms | +29.85% |
| ⚡ | Simulation | ssr before-load chain (vue) |
184.4 ms | 142.7 ms | +29.28% |
| ... | ... | ... | ... | ... | ... |
ℹ️ 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 upgrade-store (f62402e) with main (0b178a7)
There was a problem hiding this comment.
Actionable comments posted: 1
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/useSelector.ts (1)
27-37: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMemory leak when called inside functional components or render functions.
This fallback creates a new detached
effectScopeand registers anonUnmountedhook on the parentinstance. While this correctly cleans up subscriptions for one-off calls outside ofsetup()(e.g., inonMounted), it creates a severe memory leak ifuseSelectoris used inside a functional component (such asCatchNotFoundinnot-found.tsxshown in the graph context).Because functional components and render functions execute on every component update, and
Vue.getCurrentScope()is falsy during render, this fallback is triggered repeatedly. On every re-render, it creates a new store subscription and appends anotheronUnmountedhook to the parent component, neither of which are cleaned up until the parent unmounts.Since Vue explicitly advises against using stateful composition API hooks in functional components, consider documenting that
useSelectormust be used strictly withinsetup(), and convert existing functional components likeCatchNotFoundto stateful components usingVue.defineComponent(() => { ... }).🤖 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/useSelector.ts` around lines 27 - 37, Prevent repeated render-time subscriptions by documenting that useSelector must only be called within setup(), not functional components or render functions. Convert functional consumers such as CatchNotFound to stateful components using Vue.defineComponent with setup-based usage, while preserving existing one-off calls outside setup that rely on the scoped cleanup.
🤖 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/tests/useSelector.test.tsx`:
- Around line 32-37: Rewrite the test’s functional Component as a stateful
component using Vue.defineComponent with a setup function that calls useSelector
once. Exercise a re-render before unmounting, then verify the subscription is
cleaned up without creating additional subscriptions; keep the existing cleanup
assertion and store setup intact.
---
Outside diff comments:
In `@packages/vue-router/src/useSelector.ts`:
- Around line 27-37: Prevent repeated render-time subscriptions by documenting
that useSelector must only be called within setup(), not functional components
or render functions. Convert functional consumers such as CatchNotFound to
stateful components using Vue.defineComponent with setup-based usage, while
preserving existing one-off calls outside setup that rely on the scoped cleanup.
🪄 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: 2c335bdb-870b-4164-a649-bc9b5c1a9669
📒 Files selected for processing (25)
packages/react-router/src/Match.tsxpackages/react-router/src/Matches.tsxpackages/react-router/src/Scripts.tsxpackages/react-router/src/Transitioner.tsxpackages/react-router/src/headContentUtils.tsxpackages/react-router/src/link.tsxpackages/react-router/src/not-found.tsxpackages/react-router/src/useCanGoBack.tspackages/react-router/src/useLocation.tsxpackages/react-router/src/useMatch.tsxpackages/react-router/src/useRouterState.tsxpackages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsxpackages/vue-router/src/Match.tsxpackages/vue-router/src/Matches.tsxpackages/vue-router/src/Scripts.tsxpackages/vue-router/src/Transitioner.tsxpackages/vue-router/src/headContentUtils.tsxpackages/vue-router/src/link.tsxpackages/vue-router/src/not-found.tsxpackages/vue-router/src/useCanGoBack.tspackages/vue-router/src/useLocation.tsxpackages/vue-router/src/useMatch.tsxpackages/vue-router/src/useRouterState.tsxpackages/vue-router/src/useSelector.tspackages/vue-router/tests/useSelector.test.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/vue-router/src/useLocation.tsx
- packages/vue-router/src/Scripts.tsx
- packages/vue-router/src/Transitioner.tsx
- packages/react-router/src/Match.tsx
| it('cleans up a functional component subscription on unmount', () => { | ||
| const { store, unsubscribe } = createStore({ count: 0 }) | ||
| const Component = () => { | ||
| const count = useSelector(store, (state) => state.count) | ||
| return <div>{count.value}</div> | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Test masks a memory leak for functional components.
This test verifies a single mount and unmount of a functional component. However, functional components re-run their entire function body on every render. If Component were to re-render (e.g., by reacting to props or parent state), useSelector would execute again.
As there is no active effect scope during render, useSelector falls back to creating a new detached effectScope and adding an onUnmounted listener to the wrapper instance on every render, causing a memory leak.
Stateful composition APIs should not be used in functional components. Consider rewriting this test using a stateful component (Vue.defineComponent(() => { ... })) to promote correct usage patterns, and verify that re-renders do not leak subscriptions.
♻️ Proposed fix to use a stateful component
- it('cleans up a functional component subscription on unmount', () => {
+ it('cleans up a stateful component subscription on unmount', () => {
const { store, unsubscribe } = createStore({ count: 0 })
- const Component = () => {
+ const Component = Vue.defineComponent(() => {
const count = useSelector(store, (state) => state.count)
- return <div>{count.value}</div>
- }
+ return () => <div>{count.value}</div>
+ })
const view = render(Component)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('cleans up a functional component subscription on unmount', () => { | |
| const { store, unsubscribe } = createStore({ count: 0 }) | |
| const Component = () => { | |
| const count = useSelector(store, (state) => state.count) | |
| return <div>{count.value}</div> | |
| } | |
| it('cleans up a stateful component subscription on unmount', () => { | |
| const { store, unsubscribe } = createStore({ count: 0 }) | |
| const Component = Vue.defineComponent(() => { | |
| const count = useSelector(store, (state) => state.count) | |
| return () => <div>{count.value}</div> | |
| }) |
🤖 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/tests/useSelector.test.tsx` around lines 32 - 37, Rewrite
the test’s functional Component as a stateful component using
Vue.defineComponent with a setup function that calls useSelector once. Exercise
a re-render before unmounting, then verify the subscription is cleaned up
without creating additional subscriptions; keep the existing cleanup assertion
and store setup intact.
1 participant
This PR upgrades TanStack Store to the latest version.
Something worth note before merging is that we may want to consider documenting a shift away from
useStoreand towardsuseSelector, as upstream Store has markeduseStoreas deprecated.Summary by CodeRabbit
Bug Fixes
Tests