feat: enable support for native reactivity in solid vue adapters#6298
Open
riccardoperra wants to merge 4 commits into
Open
feat: enable support for native reactivity in solid vue adapters#6298riccardoperra wants to merge 4 commits into
riccardoperra wants to merge 4 commits into
Conversation
Contributor
|
PR changed again? Review this PR in Change Stack to compare snapshots and stay oriented. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (26)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (21)
📝 WalkthroughWalkthroughThis PR replaces example and UI reads of table.state with table.store.get(), and simplifies Solid and Vue adapter APIs to provide table.atoms via a single Subscribe callback while removing selector-based selected-state flows. ChangesExample state access updates
Core adapter and API changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
riccardoperra
force-pushed
the
feat/native-reactivity-solid-vue-frameworks
branch
from
5f299d1 to
663abeb
Compare
|
View your CI Pipeline Execution ↗ for commit 663abeb
💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗. ☁️ Nx Cloud last updated this comment at |
![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: 2
🧹 Nitpick comments (6)
examples/vue/kitchen-sink/src/App.vue (1)
344-344: 💤 Low valueMultiple store.get() calls in template.
The template calls
table.store.get()five times in different bindings (lines 344, 741, 751, 763, 786). If each call creates a new snapshot object, this could have minor performance implications. Consider whether these calls are properly cached or if a single reactive reference would be more efficient.Also applies to: 741-741, 751-751, 763-763, 786-786
🤖 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 `@examples/vue/kitchen-sink/src/App.vue` at line 344, The template repeatedly calls table.store.get() (e.g., to read globalFilter) which may create multiple snapshot objects; replace repeated calls by capturing the snapshot once and exposing a single reactive/computed reference (e.g., in setup create const storeSnapshot = computed(() => table.store.get()) or const store = ref(table.store.get()) and update as needed) and then bind to store.value.globalFilter (or store.globalFilter if computed) in the template; update all occurrences that use table.store.get() (including reads of globalFilter and the other bindings) to use this single cached reference to avoid repeated snapshot creation.examples/solid/grouping/src/App.tsx (1)
207-228: ⚡ Quick winConsider memoizing repeated
table.store.get()calls.The pagination state is accessed three times via separate
table.store.get()calls (lines 207, 217, 228). Following the pattern fromexamples/solid/composable-tables/src/components/table-components.tsx, consider creating a memo to access the pagination state once:const pagination = createMemo(() => table.store.get().pagination)Then use
pagination().pageIndexandpagination().pageSizein the UI bindings. This improves both performance and code clarity.♻️ Suggested refactor
function App() { const [data, setData] = createSignal(makeData(10_000)) const refreshData = () => setData(makeData(10_000)) const stressTest = () => setData(makeData(200_000)) const table = createAppTable( { columns, get data() { return data() }, debugTable: true, }, (state) => state, ) + const pagination = createMemo(() => table.store.get().pagination) + return ( <div class="demo-root"> ... <span class="inline-controls"> <div>Page</div> <strong> - {(table.store.get().pagination.pageIndex + 1).toLocaleString()} of{' '} + {(pagination().pageIndex + 1).toLocaleString()} of{' '} {table.getPageCount().toLocaleString()} </strong> </span> <span class="inline-controls"> | Go to page: <input type="number" min="1" max={table.getPageCount()} - value={table.store.get().pagination.pageIndex + 1} + value={pagination().pageIndex + 1} onInput={(e) => { const page = e.currentTarget.value ? Number(e.currentTarget.value) - 1 : 0 table.setPageIndex(page) }} class="page-size-input" /> </span> <select - value={table.store.get().pagination.pageSize} + value={pagination().pageSize} onChange={(e) => table.setPageSize(Number(e.currentTarget.value))} >🤖 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 `@examples/solid/grouping/src/App.tsx` around lines 207 - 228, Memoize the repeated table.store.get().pagination access by creating a memo like const pagination = createMemo(() => table.store.get().pagination) and then replace uses of table.store.get().pagination.pageIndex and table.store.get().pagination.pageSize with pagination().pageIndex and pagination().pageSize; keep existing bindings that call table.getPageCount() and table.setPageIndex(page) but use the memoized pagination for value, min/max and display to avoid multiple table.store.get() calls.examples/solid/with-tanstack-form/src/App.tsx (1)
310-331: ⚡ Quick winConsider memoizing repeated
table.store.get()calls.The pagination state is accessed three times via separate
table.store.get()calls (lines 310, 320, 331). Following the pattern fromexamples/solid/composable-tables/src/components/table-components.tsx, consider creating a memo:const pagination = createMemo(() => table.store.get().pagination)Then use
pagination().pageIndexandpagination().pageSizein the pagination controls. This improves both performance and code clarity.♻️ Suggested refactor
function App() { // Initialize form with makeData const form = useAppForm(() => ({ defaultValues: { data: makeData(100), }, onSubmit: ({ value }: { value: FormData }) => { alert( `Submitted ${value.data.length} records!\n\nFirst record: ${JSON.stringify(value.data[0], null, 2)}`, ) }, validators: { onChange: formSchema, }, })) // Create columns with form fields for editing // Use createMemo since columns depend on `form` (which is reactive) const columns = createMemo(() => columnHelper.columns([ // ... columns definition ]), ) // Create table using form state as data source const table = createTable({ _features, _rowModels: { filteredRowModel: createFilteredRowModel(filterFns), paginatedRowModel: createPaginatedRowModel(), }, get columns() { return columns() }, get data() { return form.state.values.data }, debugTable: true, }) + const pagination = createMemo(() => table.store.get().pagination) + const refreshData = () => { form.reset({ data: makeData(100) }) } // ... rest of component return ( <div class="demo-root"> ... <span class="inline-controls"> <div>Page</div> <strong> - {(table.store.get().pagination.pageIndex + 1).toLocaleString()} of{' '} + {(pagination().pageIndex + 1).toLocaleString()} of{' '} {table.getPageCount().toLocaleString()} </strong> </span> <span class="inline-controls"> | Go to page: <input type="number" min="1" max={table.getPageCount()} - value={table.store.get().pagination.pageIndex + 1} + value={pagination().pageIndex + 1} onInput={(e) => { const page = e.currentTarget.value ? Number(e.currentTarget.value) - 1 : 0 table.setPageIndex(page) }} class="page-size-input" /> </span> <select - value={table.store.get().pagination.pageSize} + value={pagination().pageSize} onChange={(e) => { table.setPageSize(Number(e.currentTarget.value)) }} >🤖 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 `@examples/solid/with-tanstack-form/src/App.tsx` around lines 310 - 331, Replace repeated calls to table.store.get() in the pagination controls by creating a memoized accessor for pagination (e.g., const pagination = createMemo(() => table.store.get().pagination) at the top of the component) and then use pagination().pageIndex and pagination().pageSize wherever pageIndex/pageSize are currently read (including the value prop, display text, and the max prop), while keeping the existing table.setPageIndex(...) and table.getPageCount() calls unchanged.examples/solid/pagination/src/App.tsx (1)
149-170: ⚡ Quick winConsider memoizing repeated
table.store.get()calls.The pagination state is accessed three times via separate
table.store.get()calls (lines 149, 159, 170). Following the pattern fromexamples/solid/composable-tables/src/components/table-components.tsx, consider creating a memo:const pagination = createMemo(() => table.store.get().pagination)Then use
pagination().pageIndexandpagination().pageSizethroughout. This improves both performance and code clarity.♻️ Suggested refactor
function MyTable(props: { data: Array<Person> columns: ReturnType<typeof columnHelper.columns> }) { const table = createTable({ _features, _rowModels: { paginatedRowModel: createPaginatedRowModel(), }, columns: props.columns, get data() { return props.data }, debugTable: true, }) + const pagination = createMemo(() => table.store.get().pagination) + return ( <div class="demo-root"> ... <span class="inline-controls"> <div>Page</div> <strong> - {(table.store.get().pagination.pageIndex + 1).toLocaleString()} of{' '} + {(pagination().pageIndex + 1).toLocaleString()} of{' '} {table.getPageCount().toLocaleString()} </strong> </span> <span class="inline-controls"> | Go to page: <input type="number" min="1" max={table.getPageCount()} - value={table.store.get().pagination.pageIndex + 1} + value={pagination().pageIndex + 1} onInput={(e) => { const page = e.currentTarget.value ? Number(e.currentTarget.value) - 1 : 0 table.setPageIndex(page) }} class="page-size-input" /> </span> <select - value={table.store.get().pagination.pageSize} + value={pagination().pageSize} onChange={(e) => { table.setPageSize(Number(e.currentTarget.value)) }} >🤖 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 `@examples/solid/pagination/src/App.tsx` around lines 149 - 170, The code repeatedly calls table.store.get() for pagination; create a memoized accessor (e.g., const pagination = createMemo(() => table.store.get().pagination) as in the composable-tables example) and replace uses of table.store.get().pagination/pageIndex/pageSize with pagination().pageIndex and pagination().pageSize; keep table.getPageCount() and table.setPageIndex(...) unchanged and update the input value/onInput and select value to read from pagination() to improve performance and clarity.examples/solid/column-resizing/src/App.tsx (1)
100-100: Usetable.atoms.columnResizing.get()fordeltaOffsetin Solid to avoid coarse dependency tracking
table.store.get()is backed by a Solid memo, sodeltaOffsetupdates in"onEnd"mode should still propagate. However,table.storeis computed from all table state slices (table.atoms[key].get()), so readingtable.store.get().columnResizing.deltaOffsetover-subscribes and can trigger unnecessary recomputation. Switch the read to the slice-level atom, e.g.table.atoms.columnResizing.get().deltaOffset ?? 0.🤖 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 `@examples/solid/column-resizing/src/App.tsx` at line 100, The current code reads deltaOffset via table.store.get().columnResizing.deltaOffset which over-subscribes Solid's memo; change the read to the slice-level atom by using table.atoms.columnResizing.get().deltaOffset ?? 0 instead. Locate the usage of deltaOffset (the expression referencing table.store.get()) and replace it with a call to table.atoms.columnResizing.get(), keeping the nullish fallback (?? 0) so the value remains safe when undefined. Ensure you only change the read site and do not modify other table.store or atom writes.packages/solid-table/tests/unit/reactivity.test.ts (1)
70-88: ⚡ Quick winExercise a real
Subscribeupdate here.This only proves that
table.atomsis passed through. It would still pass if the callback body was untracked andconst pagination = atoms.pagination.get()returned a stale snapshot. Please drive a table state change here and assert the child recomputes.🤖 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-table/tests/unit/reactivity.test.ts` around lines 70 - 88, The test currently only checks that table.atoms is passed to children but doesn't exercise reactivity; modify the test to drive a real state change and assert the child recomputes: inside the table.Subscribe children capture the initial value via atoms.pagination.get() (or atoms.pagination()) and store it, then after Subscribe invoke a state mutation (either call the table API to change pagination like table.setPageIndex(...) or directly update the atom via table.atoms.pagination.set(...) / atoms.pagination.set(...)) and assert the received value updates to the new pagination value (and differs from the initial snapshot). Ensure you reference table.Subscribe, table.atoms and atoms.pagination.get()/set() in the test so the change triggers the Subscribe child recomputation.
🤖 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-table/src/createTableHook.ts`:
- Around line 423-429: The default slot call in AppTable's setup currently
invokes slots.default with no args which causes consumers using v-slot="{ state
}" to destructure undefined; update the slot invocation in the AppTable
component (where provide(TableContext, table) is used and slots.default is
called) to pass an empty object as the first parameter (e.g., call
slots.default?.({})) so destructuring in consumer templates is safe and remains
backward-compatible.
In `@packages/vue-table/src/reactivity.ts`:
- Around line 58-69: The vueReactivity() implementation creates a local
subscriptions Set and exposes unmount() but never wires it into Vue’s lifecycle;
update either vueReactivity() or useTable to attach cleanup to the current Vue
scope: in vueReactivity() import onScopeDispose (or onUnmounted) and call
onScopeDispose(() => { subscriptions.forEach(s=>s.unsubscribe());
subscriptions.clear(); }), keep exporting the existing unmount() for external
use (or if you prefer change useTable.ts to call _reactivity.unmount?.() inside
its own onScopeDispose), ensuring the symbols vueReactivity(), subscriptions,
and unmount are used so subscriptions are unsubscribed when the component scope
is destroyed.
---
Nitpick comments:
In `@examples/solid/column-resizing/src/App.tsx`:
- Line 100: The current code reads deltaOffset via
table.store.get().columnResizing.deltaOffset which over-subscribes Solid's memo;
change the read to the slice-level atom by using
table.atoms.columnResizing.get().deltaOffset ?? 0 instead. Locate the usage of
deltaOffset (the expression referencing table.store.get()) and replace it with a
call to table.atoms.columnResizing.get(), keeping the nullish fallback (?? 0) so
the value remains safe when undefined. Ensure you only change the read site and
do not modify other table.store or atom writes.
In `@examples/solid/grouping/src/App.tsx`:
- Around line 207-228: Memoize the repeated table.store.get().pagination access
by creating a memo like const pagination = createMemo(() =>
table.store.get().pagination) and then replace uses of
table.store.get().pagination.pageIndex and table.store.get().pagination.pageSize
with pagination().pageIndex and pagination().pageSize; keep existing bindings
that call table.getPageCount() and table.setPageIndex(page) but use the memoized
pagination for value, min/max and display to avoid multiple table.store.get()
calls.
In `@examples/solid/pagination/src/App.tsx`:
- Around line 149-170: The code repeatedly calls table.store.get() for
pagination; create a memoized accessor (e.g., const pagination = createMemo(()
=> table.store.get().pagination) as in the composable-tables example) and
replace uses of table.store.get().pagination/pageIndex/pageSize with
pagination().pageIndex and pagination().pageSize; keep table.getPageCount() and
table.setPageIndex(...) unchanged and update the input value/onInput and select
value to read from pagination() to improve performance and clarity.
In `@examples/solid/with-tanstack-form/src/App.tsx`:
- Around line 310-331: Replace repeated calls to table.store.get() in the
pagination controls by creating a memoized accessor for pagination (e.g., const
pagination = createMemo(() => table.store.get().pagination) at the top of the
component) and then use pagination().pageIndex and pagination().pageSize
wherever pageIndex/pageSize are currently read (including the value prop,
display text, and the max prop), while keeping the existing
table.setPageIndex(...) and table.getPageCount() calls unchanged.
In `@examples/vue/kitchen-sink/src/App.vue`:
- Line 344: The template repeatedly calls table.store.get() (e.g., to read
globalFilter) which may create multiple snapshot objects; replace repeated calls
by capturing the snapshot once and exposing a single reactive/computed reference
(e.g., in setup create const storeSnapshot = computed(() => table.store.get())
or const store = ref(table.store.get()) and update as needed) and then bind to
store.value.globalFilter (or store.globalFilter if computed) in the template;
update all occurrences that use table.store.get() (including reads of
globalFilter and the other bindings) to use this single cached reference to
avoid repeated snapshot creation.
In `@packages/solid-table/tests/unit/reactivity.test.ts`:
- Around line 70-88: The test currently only checks that table.atoms is passed
to children but doesn't exercise reactivity; modify the test to drive a real
state change and assert the child recomputes: inside the table.Subscribe
children capture the initial value via atoms.pagination.get() (or
atoms.pagination()) and store it, then after Subscribe invoke a state mutation
(either call the table API to change pagination like table.setPageIndex(...) or
directly update the atom via table.atoms.pagination.set(...) /
atoms.pagination.set(...)) and assert the received value updates to the new
pagination value (and differs from the initial snapshot). Ensure you reference
table.Subscribe, table.atoms and atoms.pagination.get()/set() in the test so the
change triggers the Subscribe child recomputation.
🪄 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: 0d1548d4-6888-4db9-b6c5-3bb98203a401
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (84)
examples/angular/basic-external-atoms/src/app/app.tsexamples/angular/basic-external-state/src/app/app.tsexamples/angular/column-groups/src/app/app.tsexamples/angular/column-ordering/src/app/app.tsexamples/angular/column-pinning-split/src/app/app.tsexamples/angular/column-pinning-sticky/src/app/app.tsexamples/angular/column-pinning/src/app/app.tsexamples/angular/column-resizing-performant/src/app/app.tsexamples/angular/column-resizing/src/app/app.tsexamples/angular/column-sizing/src/app/app.tsexamples/angular/column-visibility/src/app/app.tsexamples/angular/composable-tables/src/app/components/table-components.tsexamples/angular/expanding/src/app/app.tsexamples/angular/filters-faceted/src/app/app.tsexamples/angular/filters-fuzzy/src/app/app.tsexamples/angular/filters/src/app/app.tsexamples/angular/grouping/src/app/app.tsexamples/angular/kitchen-sink/src/app/app.tsexamples/angular/pagination/src/app/app.tsexamples/angular/row-pinning/src/app/app.tsexamples/angular/row-selection-signal/src/app/app.component.tsexamples/angular/row-selection/src/app/app.tsexamples/angular/sorting/src/app/app.tsexamples/angular/virtualized-rows/src/app/app.tsexamples/angular/with-tanstack-form/src/app/app.tsexamples/angular/with-tanstack-query/src/app/app.tsexamples/solid/basic-external-atoms/src/App.tsxexamples/solid/basic-external-state/src/App.tsxexamples/solid/column-ordering/src/App.tsxexamples/solid/column-pinning-split/src/App.tsxexamples/solid/column-pinning-sticky/src/App.tsxexamples/solid/column-pinning/src/App.tsxexamples/solid/column-resizing-performant/src/App.tsxexamples/solid/column-resizing/src/App.tsxexamples/solid/column-sizing/src/App.tsxexamples/solid/column-visibility/src/App.tsxexamples/solid/composable-tables/src/components/table-components.tsxexamples/solid/expanding/src/App.tsxexamples/solid/filters-faceted/src/App.tsxexamples/solid/filters-fuzzy/src/App.tsxexamples/solid/filters/src/App.tsxexamples/solid/grouping/src/App.tsxexamples/solid/kitchen-sink/src/App.tsxexamples/solid/pagination/src/App.tsxexamples/solid/row-pinning/src/App.tsxexamples/solid/row-selection/src/App.tsxexamples/solid/sorting/src/App.tsxexamples/solid/virtualized-rows/src/App.tsxexamples/solid/with-tanstack-form/src/App.tsxexamples/solid/with-tanstack-query/src/App.tsxexamples/solid/with-tanstack-router/src/components/table.tsxexamples/vue/basic-external-atoms/src/App.tsxexamples/vue/basic-external-state/src/App.tsxexamples/vue/column-ordering/src/App.vueexamples/vue/column-pinning-split/src/App.vueexamples/vue/column-pinning-sticky/src/App.vueexamples/vue/column-pinning/src/App.vueexamples/vue/column-resizing-performant/src/App.vueexamples/vue/column-resizing/src/App.vueexamples/vue/column-sizing/src/App.vueexamples/vue/column-visibility/src/App.tsxexamples/vue/expanding/src/App.tsxexamples/vue/filters-faceted/src/App.vueexamples/vue/filters-fuzzy/src/App.vueexamples/vue/filters/src/App.vueexamples/vue/grouping/src/App.vueexamples/vue/kitchen-sink/src/App.vueexamples/vue/pagination/src/App.vueexamples/vue/row-pinning/src/App.vueexamples/vue/sorting/src/App.vueexamples/vue/with-tanstack-form/src/App.vueexamples/vue/with-tanstack-query/src/App.tsxpackages/angular-table/src/injectTable.tspackages/angular-table/tests/angularReactivityFeature.test.tspackages/solid-table/package.jsonpackages/solid-table/src/createTable.tspackages/solid-table/src/createTableHook.tsxpackages/solid-table/src/reactivity.tspackages/solid-table/tests/unit/reactivity.test.tspackages/vue-table/package.jsonpackages/vue-table/src/createTableHook.tspackages/vue-table/src/reactivity.tspackages/vue-table/src/useTable.tspackages/vue-table/tests/unit/signals.test.ts
Comment on lines
423
to
429
| const AppTable = defineComponent({ | ||
| name: 'AppTable', | ||
| props: { | ||
| selector: { | ||
| type: Function as PropType<(state: TableState<TFeatures>) => unknown>, | ||
| default: undefined, | ||
| }, | ||
| }, | ||
| setup(props, { slots }) { | ||
| setup(_, { slots }) { | ||
| provide(TableContext, table) | ||
| const selected = props.selector | ||
| ? useSelector(table.store, props.selector) | ||
| : undefined | ||
|
|
||
| return () => { | ||
| if (!props.selector) { | ||
| return slots.default?.() | ||
| } | ||
|
|
||
| return slots.default?.({ state: selected?.value }) | ||
| return slots.default?.() | ||
| } |
Contributor
There was a problem hiding this comment.
Pass {} to the AppTable default slot.
Line 428 now calls the slot with no argument. Vue compiles v-slot="{ state }" to destructure the first parameter, so existing consumers will throw during render instead of just seeing state === undefined after this API removal.
Suggested fix
const AppTable = defineComponent({
name: 'AppTable',
setup(_, { slots }) {
provide(TableContext, table)
return () => {
- return slots.default?.()
+ return slots.default?.({})
}
},
})🤖 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-table/src/createTableHook.ts` around lines 423 - 429, The
default slot call in AppTable's setup currently invokes slots.default with no
args which causes consumers using v-slot="{ state }" to destructure undefined;
update the slot invocation in the AppTable component (where
provide(TableContext, table) is used and slots.default is called) to pass an
empty object as the first parameter (e.g., call slots.default?.({})) so
destructuring in consumer templates is safe and remains backward-compatible.
Comment on lines
58
to
+69
| export function vueReactivity(): TableReactivityBindings { | ||
| const subscriptions = new Set<Subscription>() | ||
|
|
||
| return { | ||
| createOptionsStore: true, | ||
| wrapExternalAtoms: false, | ||
| addSubscription: () => { | ||
| throw new Error( | ||
| 'Feature not supported in current reactivity implementation', | ||
| ) | ||
| wrapExternalAtoms: true, | ||
| addSubscription: (subscription) => { | ||
| subscriptions.add(subscription) | ||
| }, | ||
| unmount: () => { | ||
| throw new Error( | ||
| 'Feature not supported in current reactivity implementation', | ||
| ) | ||
| subscriptions.forEach((s) => s.unsubscribe()) | ||
| subscriptions.clear() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Reactivity binding contract / relevant symbols:"
rg -n -C2 'TableReactivityBindings|wrapExternalAtoms|addSubscription|unmount'
echo
echo "Vue lifecycle cleanup hooks still present in the adapter:"
rg -n -C2 'onScopeDispose|onUnmounted|getCurrentScope'
echo
echo "Call sites that invoke an unmount method:"
rg -n -C2 '\.unmount\s*\('Repository: TanStack/table
Length of output: 36432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Vue adapter teardown hooks (vue-table) =="
rg -n --hidden --no-ignore -S "onScopeDispose|getCurrentScope|onUnmounted|onBeforeUnmount" packages/vue-table -g'*.ts' -g'*.tsx' -g'*.vue'
echo
echo "== Call sites invoking unmount from reactivity bindings =="
rg -n --hidden --no-ignore -S "reactivity\.unmount\?\.|coreReativityFeature:|_reactivity\.unmount\?\." packages -g'*.ts' -g'*.tsx'
echo
echo "== Direct calls to unmount() in vue-table =="
rg -n --hidden --no-ignore -S "unmount\?\.\(\)|unmount\(" packages/vue-table -g'*.ts' -g'*.tsx'
echo
echo "== Check table-core reactivity binding invocation =="
rg -n --hidden --no-ignore -S "_reactivity\.unmount|\.unmount\?\." packages/table-core -g'*.ts'
echo
echo "== Inspect vue-table useTable implementation =="
ls -la packages/vue-table/src
rg -n "function useTable|export function useTable" packages/vue-table/src -g'*.ts' -g'*.tsx'
# Print relevant sections around useTable
FILE=$(rg -l "function useTable|export function useTable" packages/vue-table/src -g'*.ts' -g'*.tsx' | head -n 1)
echo "Using file: $FILE"
sed -n '1,220p' "$FILE" | nl -ba | head -n 220Repository: TanStack/table
Length of output: 103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Vue adapter teardown hooks (vue-table) =="
rg -n --hidden --no-ignore -S "onScopeDispose|getCurrentScope|onUnmounted|onBeforeUnmount" packages/vue-table -g'*.ts' -g'*.tsx' -g'*.vue'
echo
echo "== Call sites invoking unmount from reactivity bindings =="
rg -n --hidden --no-ignore -S "reactivity\.unmount\?\.|_reactivity\.unmount\?\." packages -g'*.ts' -g'*.tsx'
echo
echo "== Direct calls to unmount in vue-table =="
rg -n --hidden --no-ignore -S "unmount\?\.\(\)|unmount\(" packages/vue-table -g'*.ts' -g'*.tsx'
echo
echo "== Table-core invocation of reactivity.unmount =="
rg -n --hidden --no-ignore -S "_reactivity\.unmount|\.unmount\?\." packages/table-core -g'*.ts'
echo
echo "== Inspect vue-table useTable implementation =="
ls -la packages/vue-table/src
FILE=$(rg -l "function useTable|export function useTable" packages/vue-table/src -g'*.ts' -g'*.tsx' | head -n 1)
echo "Using file: $FILE"
sed -n '1,260p' "$FILE" | nl -ba | head -n 260Repository: TanStack/table
Length of output: 103
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "== Vue adapter teardown hooks (vue-table) =="
rg -n --hidden --no-ignore -S "onScopeDispose|getCurrentScope|onUnmounted|onBeforeUnmount" packages/vue-table -g'*.ts' -g'*.tsx' -g'*.vue'
echo "exit=$?"
echo
echo "== Call sites invoking unmount from reactivity bindings =="
rg -n --hidden --no-ignore -S "reactivity\.unmount\?\.|_reactivity\.unmount\?\." packages -g'*.ts' -g'*.tsx'
echo "exit=$?"
echo
echo "== Direct calls to unmount in vue-table =="
rg -n --hidden --no-ignore -S "unmount\?\.\(\)|unmount\(" packages/vue-table -g'*.ts' -g'*.tsx'
echo "exit=$?"
echo
echo "== Table-core invocation of reactivity.unmount =="
rg -n --hidden --no-ignore -S "_reactivity\.unmount|\.unmount\?\." packages/table-core -g'*.ts'
echo "exit=$?"
echo
echo "== Inspect vue-table useTable implementation =="
FILE=$(rg -l "function useTable|export function useTable" packages/vue-table/src -g'*.ts' -g'*.tsx' | head -n 1)
echo "Using file: $FILE"
if [ -n "${FILE:-}" ]; then
sed -n '1,260p' "$FILE" | nl -ba
fiRepository: TanStack/table
Length of output: 614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search .unmount usage in vue-table =="
rg -n --hidden --no-ignore -S "\.unmount\s*\?\." packages/vue-table -g'*.ts' -g'*.tsx' -g'*.vue'
rg -n --hidden --no-ignore -S "\.unmount\s*\(" packages/vue-table -g'*.ts' -g'*.tsx' -g'*.vue'
rg -n --hidden --no-ignore -S "onScopeDispose|getCurrentScope|onUnmounted|onBeforeUnmount" packages/vue-table -g'*.ts' -g'*.tsx' -g'*.vue'
echo
FILE="packages/vue-table/src/useTable.ts"
echo "== Inspect $FILE (first 260 lines) =="
wc -l "$FILE"
sed -n '1,260p' "$FILE" | cat -n
echo
echo "== Inspect sections mentioning lifecycle/cleanup/unmount (whole file, filtered) =="
rg -n -S "unmount|onScopeDispose|getCurrentScope|onUnmounted|dispose|cleanup" "$FILE"Repository: TanStack/table
Length of output: 99
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "== Search .unmount usage in vue-table =="
rg -n --hidden --no-ignore -S "\.unmount\s*\?\." packages/vue-table -g'*.ts' -g'*.tsx' -g'*.vue'
echo "exit_unmount_optional=$?"
rg -n --hidden --no-ignore -S "\.unmount\s*\(" packages/vue-table -g'*.ts' -g'*.tsx' -g'*.vue'
echo "exit_unmount_call=$?"
rg -n --hidden --no-ignore -S "onScopeDispose|getCurrentScope|onUnmounted|onBeforeUnmount" packages/vue-table -g'*.ts' -g'*.tsx' -g'*.vue'
echo "exit_vue_lifecycle=$?"
echo
FILE="packages/vue-table/src/useTable.ts"
echo "== Inspect $FILE =="
wc -l "$FILE"
sed -n '1,260p' "$FILE" | cat -n
echo
echo "== Filtered lines for cleanup/lifecycle =="
rg -n -S "unmount|onScopeDispose|getCurrentScope|onUnmounted|onBeforeUnmount|dispose|cleanup|stop\(" "$FILE"
echo "exit_filtered=$?"Repository: TanStack/table
Length of output: 6631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/vue-table/src/reactivity.ts"
echo "== $FILE =="
wc -l "$FILE"
echo
sed -n '1,120p' "$FILE" | cat -n
echo
echo "== vueReactivity() section =="
sed -n '40,120p' "$FILE" | cat -nRepository: TanStack/table
Length of output: 5349
Wire vueReactivity() subscription cleanup to Vue teardown.
packages/vue-table/src/reactivity.ts keeps external atom subscriptions in a local subscriptions set and only clears them in unmount. That unmount isn’t connected to Vue lifecycle: vueReactivity() imports no onScopeDispose/onUnmounted, and packages/vue-table/src/useTable.ts contains no teardown hook or call to _reactivity.unmount?.(). Since table-core also doesn’t invoke it, wrapped external subscriptions can outlive the component.
Possible fix (attach cleanup to the current Vue scope)
-import { computed, shallowRef, watch } from 'vue'
+import { computed, getCurrentScope, onScopeDispose, shallowRef, watch } from 'vue'
import { batch } from '`@tanstack/store`'
@@
export function vueReactivity(): TableReactivityBindings {
const subscriptions = new Set<Subscription>()
+
+ if (getCurrentScope()) {
+ onScopeDispose(() => {
+ subscriptions.forEach((s) => s.unsubscribe())
+ subscriptions.clear()
+ })
+ }
return {
createOptionsStore: true,
wrapExternalAtoms: true,🤖 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-table/src/reactivity.ts` around lines 58 - 69, The
vueReactivity() implementation creates a local subscriptions Set and exposes
unmount() but never wires it into Vue’s lifecycle; update either vueReactivity()
or useTable to attach cleanup to the current Vue scope: in vueReactivity()
import onScopeDispose (or onUnmounted) and call onScopeDispose(() => {
subscriptions.forEach(s=>s.unsubscribe()); subscriptions.clear(); }), keep
exporting the existing unmount() for external use (or if you prefer change
useTable.ts to call _reactivity.unmount?.() inside its own onScopeDispose),
ensuring the symbols vueReactivity(), subscriptions, and unmount are used so
subscriptions are unsubscribed when the component scope is destroyed.
KevinVandy
reviewed
Jun 6, 2026
| type="number" | ||
| min="1" | ||
| [max]="table().getPageCount()" | ||
| [value]="table().state.pagination.pageIndex + 1" |
Member
There was a problem hiding this comment.
These should probably just be atom lookups, but fine either way
KevinVandy
reviewed
Jun 6, 2026
| ) => { | ||
| if (columnResizeMode() === 'onEnd' && header.column.getIsResizing()) { | ||
| const delta = table.state().columnResizing.deltaOffset ?? 0 | ||
| const delta = table.store.get().columnResizing.deltaOffset ?? 0 |
Member
There was a problem hiding this comment.
makes more sense for atom read for this line since it's one slice of state
KevinVandy
reviewed
Jun 6, 2026
| const table = useTableContext() | ||
|
|
||
| const pagination = createMemo(() => table.state().pagination) | ||
| const pagination = createMemo(() => table.store.get().pagination) |
Member
There was a problem hiding this comment.
read pagination atom instead of store
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
2 participants
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.
Updated Vue and Solid table reactivity to rely on framework-native primitives and table.atoms instead of the framework-specific TanStack Store adapters.
Summary:
Summary by CodeRabbit
Refactor
Subscribeto provide direct atom access.Chores
@tanstack/storepackage for Solid and Vue adapters.