refactor(table-core): centralize no-op state update guarding in setStateSlice - #6532
Merged
Conversation
…ateSlice Route every table.setX state router through a single setStateSlice util that resolves the updater once against the slice's current value, structurally compares the result, and skips the onXChange handler entirely when nothing changed. This removes the class of render loops where auto resets fired change handlers with freshly allocated but semantically identical values (e.g. autoResetExpanded after a data reference change), and deletes the scattered ad-hoc guards that previously protected individual slices. For uncontrolled slices the default handler receives the pre-resolved value, so updaters run exactly once. User handlers and externally owned slices receive the original updater untouched to preserve functional update composition and independent base-atom fallback semantics. Supersedes #6519. Co-Authored-By: Dmitrii Kartashev <dikartashev@gmail.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
View your CI Pipeline Execution ↗ for commit fc79eaa
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview1 package(s) bumped directly, 17 bumped as dependents. 🟩 Patch bumps
|
Contributor
|
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:
📝 WalkthroughWalkthroughThe pull request centralizes table state updates through ChangesCentralized state no-op handling
Date-range filter examples
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant FilterInput
participant TableFilter
participant TableState
participant RowModel
FilterInput->>TableFilter: Set minimum or maximum birth date
TableFilter->>TableState: Update inDateRange filter bounds
TableState->>RowModel: Apply date-range filter
RowModel-->>TableFilter: Return filtered rows
Possibly related PRs
🚥 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 |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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: 3
🤖 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 @.changeset/central-noop-state-guard.md:
- Line 11: Update the `onXChange` behavior statement in the changeset to remove
the incorrect `useState` identity-bail comparison and document that
`setStateSlice` suppresses events for structurally equal values, including newly
allocated equivalent objects.
In `@packages/table-core/src/utils.ts`:
- Around line 163-204: Update stateSlicesEqual’s array and object comparison
logic to distinguish sparse holes from explicitly stored undefined values by
checking ownership of each array index before recursive comparison. Include all
own enumerable keys, including symbol keys, when comparing object-like values
instead of relying only on Object.keys, while preserving the existing length,
prototype, and recursive equality checks.
- Around line 254-257: Update the state-update flow around functionalUpdate and
stateSlicesEqual to determine slice handler ownership before reading or
comparing the atom snapshot. Apply the local no-op guard only when the slice is
internally owned and uses the default state handler; for controlled or custom
handlers, delegate the original updater unchanged so queued host updates are
preserved. Add a test covering a queued controlled host update followed by a
locally apparent no-op.
🪄 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: a4cd23f3-7934-4442-b83a-4263144cf36d
📒 Files selected for processing (34)
.changeset/central-noop-state-guard.mdpackage.jsonpackages/alpine-table/tests/unit/adapterCoverage.test.tspackages/lit-table/tests/unit/adapterLifecycle.test.tspackages/solid-table/tests/unit/adapterReactivity.test.tspackages/table-core/src/features/cell-selection/cellSelectionFeature.utils.tspackages/table-core/src/features/column-filtering/columnFilteringFeature.utils.tspackages/table-core/src/features/column-grouping/columnGroupingFeature.utils.tspackages/table-core/src/features/column-ordering/columnOrderingFeature.utils.tspackages/table-core/src/features/column-pinning/columnPinningFeature.utils.tspackages/table-core/src/features/column-resizing/columnResizingFeature.utils.tspackages/table-core/src/features/column-sizing/columnSizingFeature.utils.tspackages/table-core/src/features/column-visibility/columnVisibilityFeature.utils.tspackages/table-core/src/features/global-filtering/globalFilteringFeature.utils.tspackages/table-core/src/features/row-expanding/rowExpandingFeature.utils.tspackages/table-core/src/features/row-pagination/rowPaginationFeature.utils.tspackages/table-core/src/features/row-pinning/rowPinningFeature.utils.tspackages/table-core/src/features/row-selection/rowSelectionFeature.utils.tspackages/table-core/src/features/row-sorting/rowSortingFeature.utils.tspackages/table-core/src/utils.tspackages/table-core/tests/unit/features/column-filtering/columnFilteringFeature.utils.test.tspackages/table-core/tests/unit/features/column-grouping/columnGroupingFeature.utils.test.tspackages/table-core/tests/unit/features/column-ordering/columnOrderingFeature.utils.test.tspackages/table-core/tests/unit/features/column-pinning/columnPinningFeature.utils.test.tspackages/table-core/tests/unit/features/column-resizing/columnResizingFeature.utils.test.tspackages/table-core/tests/unit/features/column-sizing/columnSizingFeature.utils.test.tspackages/table-core/tests/unit/features/column-visibility/columnVisibilityFeature.utils.test.tspackages/table-core/tests/unit/features/global-filtering/globalFilteringFeature.utils.test.tspackages/table-core/tests/unit/features/row-expanding/rowExpandingFeature.utils.test.tspackages/table-core/tests/unit/features/row-pagination/rowPaginationFeature.utils.test.tspackages/table-core/tests/unit/features/row-pinning/rowPinningFeature.utils.test.tspackages/table-core/tests/unit/features/row-selection/rowSelectionFeature.utils.test.tspackages/table-core/tests/unit/features/row-sorting/rowSortingFeature.utils.test.tspackages/table-core/tests/unit/setStateSlice.test.ts
Contributor
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)
docs/reference/index/functions/makeStateUpdater.md (1)
14-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument the configured atom fallback.
makeStateUpdaterfirst usesoptions.atoms[key]and falls back tobaseAtoms[key]. The text at Lines 14-16 says that the updater always writes through the table base atom. Update it to describe both targets.🤖 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 `@docs/reference/index/functions/makeStateUpdater.md` around lines 14 - 16, Update the makeStateUpdater documentation to state that it writes through options.atoms[key] when configured, otherwise falling back to baseAtoms[key], while preserving the description of value and functional updater forms.
🤖 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 `@docs/reference/index/functions/setStateSlice.md`:
- Around line 72-87: Fix the generated Updater type reference in the
setStateSlice documentation so the indexed state lookup uses K directly rather
than the invalid K<K> expression. Regenerate the page or update the emitted
Markdown while preserving the existing state-key union and resulting indexed
state type.
---
Outside diff comments:
In `@docs/reference/index/functions/makeStateUpdater.md`:
- Around line 14-16: Update the makeStateUpdater documentation to state that it
writes through options.atoms[key] when configured, otherwise falling back to
baseAtoms[key], while preserving the description of value and functional updater
forms.
🪄 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: e5d91206-4edf-4039-8ff7-21f9e6bc4820
📒 Files selected for processing (162)
docs/reference/index/functions/assignPrototypeAPIs.mddocs/reference/index/functions/assignTableAPIs.mddocs/reference/index/functions/callMemoOrStaticFn.mddocs/reference/index/functions/flattenBy.mddocs/reference/index/functions/getFunctionNameInfo.mddocs/reference/index/functions/isFunction.mddocs/reference/index/functions/makeStateUpdater.mddocs/reference/index/functions/memo.mddocs/reference/index/functions/setStateSlice.mddocs/reference/index/functions/skipFirstRun.mddocs/reference/index/functions/stateSlicesEqual.mddocs/reference/index/functions/tableMemo.mddocs/reference/index/index.mddocs/reference/index/interfaces/API.mddocs/reference/index/interfaces/PrototypeAPI.mddocs/reference/index/type-aliases/APIObject.mddocs/reference/index/type-aliases/PrototypeAPIObject.mddocs/reference/static-functions/functions/cell_getCanSelect.mddocs/reference/static-functions/functions/cell_getIsFocused.mddocs/reference/static-functions/functions/cell_getIsSelected.mddocs/reference/static-functions/functions/cell_getSelectionEdges.mddocs/reference/static-functions/functions/cell_getSelectionExtendHandler.mddocs/reference/static-functions/functions/cell_getSelectionStartHandler.mddocs/reference/static-functions/functions/cell_getTabIndex.mddocs/reference/static-functions/functions/column_clearSorting.mddocs/reference/static-functions/functions/column_getAfter.mddocs/reference/static-functions/functions/column_getAutoFilterFn.mddocs/reference/static-functions/functions/column_getAutoSortDir.mddocs/reference/static-functions/functions/column_getAutoSortFn.mddocs/reference/static-functions/functions/column_getCanFilter.mddocs/reference/static-functions/functions/column_getCanHide.mddocs/reference/static-functions/functions/column_getCanMultiSort.mddocs/reference/static-functions/functions/column_getCanSort.mddocs/reference/static-functions/functions/column_getFilterFn.mddocs/reference/static-functions/functions/column_getFilterIndex.mddocs/reference/static-functions/functions/column_getFilterValue.mddocs/reference/static-functions/functions/column_getFirstSortDir.mddocs/reference/static-functions/functions/column_getIndex.mddocs/reference/static-functions/functions/column_getIsFiltered.mddocs/reference/static-functions/functions/column_getIsFirstColumn.mddocs/reference/static-functions/functions/column_getIsLastColumn.mddocs/reference/static-functions/functions/column_getIsSorted.mddocs/reference/static-functions/functions/column_getIsVisible.mddocs/reference/static-functions/functions/column_getNextSortingOrder.mddocs/reference/static-functions/functions/column_getSize.mddocs/reference/static-functions/functions/column_getSortFn.mddocs/reference/static-functions/functions/column_getSortIndex.mddocs/reference/static-functions/functions/column_getStart.mddocs/reference/static-functions/functions/column_getToggleSortingHandler.mddocs/reference/static-functions/functions/column_getToggleVisibilityHandler.mddocs/reference/static-functions/functions/column_resetSize.mddocs/reference/static-functions/functions/column_setFilterValue.mddocs/reference/static-functions/functions/column_toggleSorting.mddocs/reference/static-functions/functions/column_toggleVisibility.mddocs/reference/static-functions/functions/getDefaultCellSelectionState.mddocs/reference/static-functions/functions/getDefaultColumnFiltersState.mddocs/reference/static-functions/functions/getDefaultColumnOrderState.mddocs/reference/static-functions/functions/getDefaultColumnSizingColumnDef.mddocs/reference/static-functions/functions/getDefaultColumnSizingState.mddocs/reference/static-functions/functions/getDefaultColumnVisibilityState.mddocs/reference/static-functions/functions/getDefaultRowSelectionState.mddocs/reference/static-functions/functions/header_getSize.mddocs/reference/static-functions/functions/header_getStart.mddocs/reference/static-functions/functions/isRowSelected.mddocs/reference/static-functions/functions/isSubRowSelected.mddocs/reference/static-functions/functions/orderColumns.mddocs/reference/static-functions/functions/row_getCanExpand.mddocs/reference/static-functions/functions/row_getCanMultiSelect.mddocs/reference/static-functions/functions/row_getCanSelect.mddocs/reference/static-functions/functions/row_getCanSelectSubRows.mddocs/reference/static-functions/functions/row_getIsAllParentsExpanded.mddocs/reference/static-functions/functions/row_getIsAllSubRowsSelected.mddocs/reference/static-functions/functions/row_getIsExpanded.mddocs/reference/static-functions/functions/row_getIsSelected.mddocs/reference/static-functions/functions/row_getIsSomeSelected.mddocs/reference/static-functions/functions/row_getToggleExpandedHandler.mddocs/reference/static-functions/functions/row_getToggleSelectedHandler.mddocs/reference/static-functions/functions/row_getVisibleCells.mddocs/reference/static-functions/functions/row_getVisibleCellsByColumnId.mddocs/reference/static-functions/functions/row_toggleExpanded.mddocs/reference/static-functions/functions/row_toggleSelected.mddocs/reference/static-functions/functions/selectRowsFn.mddocs/reference/static-functions/functions/shouldAutoRemoveFilter.mddocs/reference/static-functions/functions/table_autoResetCellSelection.mddocs/reference/static-functions/functions/table_autoResetSorting.mddocs/reference/static-functions/functions/table_extendCellSelection.mddocs/reference/static-functions/functions/table_firstPage.mddocs/reference/static-functions/functions/table_getCanLastPage.mddocs/reference/static-functions/functions/table_getCanNextPage.mddocs/reference/static-functions/functions/table_getCanPreviousPage.mddocs/reference/static-functions/functions/table_getCanSomeRowsExpand.mddocs/reference/static-functions/functions/table_getCellSelectionBounds.mddocs/reference/static-functions/functions/table_getCellSelectionColumnIds.mddocs/reference/static-functions/functions/table_getCellSelectionColumnIndexes.mddocs/reference/static-functions/functions/table_getCellSelectionMergeBounds.mddocs/reference/static-functions/functions/table_getCellSelectionRowIds.mddocs/reference/static-functions/functions/table_getCenterTotalSize.mddocs/reference/static-functions/functions/table_getColumnIndexes.mddocs/reference/static-functions/functions/table_getColumnOffsets.mddocs/reference/static-functions/functions/table_getEndTotalSize.mddocs/reference/static-functions/functions/table_getExpandedDepth.mddocs/reference/static-functions/functions/table_getFilteredSelectedRowModel.mddocs/reference/static-functions/functions/table_getFocusedCell.mddocs/reference/static-functions/functions/table_getGroupedSelectedRowModel.mddocs/reference/static-functions/functions/table_getIsAllColumnsVisible.mddocs/reference/static-functions/functions/table_getIsAllPageRowsSelected.mddocs/reference/static-functions/functions/table_getIsAllRowsExpanded.mddocs/reference/static-functions/functions/table_getIsAllRowsSelected.mddocs/reference/static-functions/functions/table_getIsSomeColumnsVisible.mddocs/reference/static-functions/functions/table_getIsSomePageRowsSelected.mddocs/reference/static-functions/functions/table_getIsSomeRowsExpanded.mddocs/reference/static-functions/functions/table_getIsSomeRowsSelected.mddocs/reference/static-functions/functions/table_getOrderColumnsFn.mddocs/reference/static-functions/functions/table_getPageCount.mddocs/reference/static-functions/functions/table_getPageOptions.mddocs/reference/static-functions/functions/table_getPreSelectedRowModel.mddocs/reference/static-functions/functions/table_getRowCount.mddocs/reference/static-functions/functions/table_getSelectedCellCount.mddocs/reference/static-functions/functions/table_getSelectedCellIds.mddocs/reference/static-functions/functions/table_getSelectedCellRangesData.mddocs/reference/static-functions/functions/table_getSelectedRowIds.mddocs/reference/static-functions/functions/table_getSelectedRowModel.mddocs/reference/static-functions/functions/table_getStartTotalSize.mddocs/reference/static-functions/functions/table_getToggleAllColumnsVisibilityHandler.mddocs/reference/static-functions/functions/table_getToggleAllPageRowsSelectedHandler.mddocs/reference/static-functions/functions/table_getToggleAllRowsExpandedHandler.mddocs/reference/static-functions/functions/table_getToggleAllRowsSelectedHandler.mddocs/reference/static-functions/functions/table_getTotalSize.mddocs/reference/static-functions/functions/table_getVisibleFlatColumns.mddocs/reference/static-functions/functions/table_getVisibleLeafColumns.mddocs/reference/static-functions/functions/table_lastPage.mddocs/reference/static-functions/functions/table_moveCellSelection.mddocs/reference/static-functions/functions/table_nextPage.mddocs/reference/static-functions/functions/table_previousPage.mddocs/reference/static-functions/functions/table_resetCellSelection.mddocs/reference/static-functions/functions/table_resetColumnFilters.mddocs/reference/static-functions/functions/table_resetColumnOrder.mddocs/reference/static-functions/functions/table_resetColumnSizing.mddocs/reference/static-functions/functions/table_resetColumnVisibility.mddocs/reference/static-functions/functions/table_resetExpanded.mddocs/reference/static-functions/functions/table_resetPageIndex.mddocs/reference/static-functions/functions/table_resetPageSize.mddocs/reference/static-functions/functions/table_resetPagination.mddocs/reference/static-functions/functions/table_resetRowSelection.mddocs/reference/static-functions/functions/table_resetSorting.mddocs/reference/static-functions/functions/table_selectAllCells.mddocs/reference/static-functions/functions/table_selectCellRange.mddocs/reference/static-functions/functions/table_setCellSelection.mddocs/reference/static-functions/functions/table_setColumnFilters.mddocs/reference/static-functions/functions/table_setColumnOrder.mddocs/reference/static-functions/functions/table_setColumnSizing.mddocs/reference/static-functions/functions/table_setColumnVisibility.mddocs/reference/static-functions/functions/table_setFocusedCell.mddocs/reference/static-functions/functions/table_setPageIndex.mddocs/reference/static-functions/functions/table_setPageSize.mddocs/reference/static-functions/functions/table_setPagination.mddocs/reference/static-functions/functions/table_setRowSelection.mddocs/reference/static-functions/functions/table_setSorting.mddocs/reference/static-functions/functions/table_toggleAllColumnsVisible.mddocs/reference/static-functions/functions/table_toggleAllPageRowsSelected.mddocs/reference/static-functions/functions/table_toggleAllRowsSelected.mdpackages/table-core/src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/table-core/src/utils.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@examples/alpine/filters/src/makeData.ts`:
- Line 34: Update the row-generation logic in makeData.ts so age and birthDate
are derived from the same generated value rather than independently. Apply the
same change at examples/alpine/filters/src/makeData.ts:34,
examples/angular/filters/src/app/makeData.ts:34,
examples/octane/filters/src/makeData.ts:34,
examples/react/filters/src/makeData.ts:34, and
examples/solid/filters/src/makeData.ts:34, preserving the 18–65 age range and
ensuring each row’s age matches its birthDate.
In `@examples/preact/filters/tests/e2e/smoke.spec.ts`:
- Around line 309-320: Make the maximum-bound assertions discriminating in the
smoke-test range-filter cases: in
examples/preact/filters/tests/e2e/smoke.spec.ts lines 309-320,
examples/lit/filters/tests/e2e/smoke.spec.ts lines 155-166,
examples/octane/filters/tests/e2e/smoke.spec.ts lines 300-311, and
examples/vue/filters/tests/e2e/smoke.spec.ts lines 307-318, set maxDate to
minDate, assert the bounded result is non-empty, and require every rendered date
to equal minDate while preserving the existing filter-value assertion.
In `@examples/react/row-selection/src/main.tsx`:
- Around line 307-309: Remove the orphaned State label and its empty container
alongside the commented-out table.state preview in the main component, or
restore a lightweight state preview within that container; ensure no standalone
State text remains when the preview is disabled.
🪄 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: e098f036-0c5e-464d-86fa-a5cf6cb1a234
📒 Files selected for processing (36)
.changeset/central-noop-state-guard.mdexamples/alpine/filters/index.htmlexamples/alpine/filters/src/main.tsexamples/alpine/filters/src/makeData.tsexamples/alpine/filters/tests/e2e/smoke.spec.tsexamples/angular/filters/src/app/app.tsexamples/angular/filters/src/app/makeData.tsexamples/angular/filters/src/app/table-filter/table-filter.tsexamples/angular/filters/tests/e2e/smoke.spec.tsexamples/ember/filters/app/templates/application.gtsexamples/ember/filters/app/utils/make-data.tsexamples/ember/filters/tests/e2e/smoke.spec.tsexamples/lit/filters/src/main.tsexamples/lit/filters/src/makeData.tsexamples/lit/filters/tests/e2e/smoke.spec.tsexamples/octane/filters/src/main.tsrxexamples/octane/filters/src/makeData.tsexamples/octane/filters/tests/e2e/smoke.spec.tsexamples/preact/filters/src/main.tsxexamples/preact/filters/src/makeData.tsexamples/preact/filters/tests/e2e/smoke.spec.tsexamples/react/filters/src/main.tsxexamples/react/filters/src/makeData.tsexamples/react/filters/tests/e2e/smoke.spec.tsexamples/react/row-selection/src/main.tsxexamples/solid/filters/src/App.tsxexamples/solid/filters/src/ColumnFilter.tsxexamples/solid/filters/src/makeData.tsexamples/solid/filters/tests/e2e/smoke.spec.tsexamples/vue/filters/src/App.vueexamples/vue/filters/src/Filter.vueexamples/vue/filters/src/makeData.tsexamples/vue/filters/src/tableHelper.tsexamples/vue/filters/tests/e2e/smoke.spec.tspackages/table-core/src/utils.tspackages/table-core/tests/unit/setStateSlice.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/central-noop-state-guard.md
- packages/table-core/tests/unit/setStateSlice.test.ts
| 'complicated', | ||
| 'single', | ||
| ])[0], | ||
| birthDate: faker.date.birthdate({ min: 18, max: 65, mode: 'age' }), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep age consistent with birthDate.
Each generator assigns age and birthDate independently. A row can show an age below 18 with a birth date for a person aged 18–65. Generate one value from the other.
examples/alpine/filters/src/makeData.ts#L34-L34: Deriveagefrom the generatedbirthDate, or derivebirthDatefromage.examples/angular/filters/src/app/makeData.ts#L34-L34: Deriveagefrom the generatedbirthDate, or derivebirthDatefromage.examples/octane/filters/src/makeData.ts#L34-L34: Deriveagefrom the generatedbirthDate, or derivebirthDatefromage.examples/react/filters/src/makeData.ts#L34-L34: Deriveagefrom the generatedbirthDate, or derivebirthDatefromage.examples/solid/filters/src/makeData.ts#L34-L34: Deriveagefrom the generatedbirthDate, or derivebirthDatefromage.
📍 Affects 5 files
examples/alpine/filters/src/makeData.ts#L34-L34(this comment)examples/angular/filters/src/app/makeData.ts#L34-L34examples/octane/filters/src/makeData.ts#L34-L34examples/react/filters/src/makeData.ts#L34-L34examples/solid/filters/src/makeData.ts#L34-L34
🤖 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/alpine/filters/src/makeData.ts` at line 34, Update the
row-generation logic in makeData.ts so age and birthDate are derived from the
same generated value rather than independently. Apply the same change at
examples/alpine/filters/src/makeData.ts:34,
examples/angular/filters/src/app/makeData.ts:34,
examples/octane/filters/src/makeData.ts:34,
examples/react/filters/src/makeData.ts:34, and
examples/solid/filters/src/makeData.ts:34, preserving the 18–65 age range and
ensuring each row’s age matches its birthDate.
Comment on lines
+309
to
+320
| // The largest visible date keeps the range ordered and non-empty. | ||
| const maxDate = dates[dates.length - 1]! | ||
| await dateRangeFilter(page, 'max').fill(maxDate) | ||
|
|
||
| await expectColumnFilters(page, [ | ||
| { id: 'birthDate', value: [minDate, maxDate] }, | ||
| ]) | ||
|
|
||
| for (const date of await readBodyColumn(page, 'birthDate')) { | ||
| expect(date >= minDate).toBe(true) | ||
| expect(date <= maxDate).toBe(true) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the maximum-bound test discriminating.
The selected maxDate is the largest date from the original page. A broken maximum bound can pass when no rendered min-filtered row exceeds that value.
Use maxDate = minDate. Assert that the bounded result is non-empty and that every rendered date equals minDate. This verifies the inclusive maximum bound.
examples/preact/filters/tests/e2e/smoke.spec.ts#L309-L320: useminDateasmaxDateand assert exact bounded dates.examples/lit/filters/tests/e2e/smoke.spec.ts#L155-L166: useminDateasmaxDateand assert exact bounded dates.examples/octane/filters/tests/e2e/smoke.spec.ts#L300-L311: useminDateasmaxDateand assert exact bounded dates.examples/vue/filters/tests/e2e/smoke.spec.ts#L307-L318: useminDateasmaxDateand assert exact bounded dates.
📍 Affects 4 files
examples/preact/filters/tests/e2e/smoke.spec.ts#L309-L320(this comment)examples/lit/filters/tests/e2e/smoke.spec.ts#L155-L166examples/octane/filters/tests/e2e/smoke.spec.ts#L300-L311examples/vue/filters/tests/e2e/smoke.spec.ts#L307-L318
🤖 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/preact/filters/tests/e2e/smoke.spec.ts` around lines 309 - 320, Make
the maximum-bound assertions discriminating in the smoke-test range-filter
cases: in examples/preact/filters/tests/e2e/smoke.spec.ts lines 309-320,
examples/lit/filters/tests/e2e/smoke.spec.ts lines 155-166,
examples/octane/filters/tests/e2e/smoke.spec.ts lines 300-311, and
examples/vue/filters/tests/e2e/smoke.spec.ts lines 307-318, set maxDate to
minDate, assert the bounded result is non-empty, and require every rendered date
to equal minDate while preserving the existing filter-value assertion.
Contributor
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/table-core/src/features/row-selection/rowSelectionFeature.utils.ts`:
- Around line 53-58: Document in the central table state API contract that
table_setRowSelection intentionally bypasses the setStateSlice no-op guard and
may invoke onRowSelectionChange for structurally unchanged selections. Preserve
the existing row-selection behavior and explain that this applies to user
updates and explicit resets.
🪄 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: edb74594-79ef-435e-815a-a15d1fa64e33
📒 Files selected for processing (3)
.changeset/central-noop-state-guard.mdpackages/table-core/src/features/row-selection/rowSelectionFeature.utils.tspackages/table-core/tests/unit/features/row-selection/rowSelectionFeature.utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/central-noop-state-guard.md
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We fix the test:types failure by adding as RowSource to the rowSource initial value in Alpine.reactive(), which narrows the inferred type from string to the expected 'all' | 'custom' | 'filtered' | 'page' | 'selected' union. Without this assertion, Alpine's reactive wrapper widens the literal to string, causing both TS2322 errors where { rowSource: string } fails assignability to AggregationTableMeta. This aligns the alpine example with the pattern used in the vanilla and lit aggregation examples.
Tip
✅ We verified this fix by re-running tanstack-alpine-table-example-aggregation:test:types.
diff --git a/examples/alpine/aggregation/src/main.ts b/examples/alpine/aggregation/src/main.ts
index a495306a..d486d946 100644
--- a/examples/alpine/aggregation/src/main.ts
+++ b/examples/alpine/aggregation/src/main.ts
@@ -91,7 +91,7 @@ const columns = columnHelper.columns([
Alpine.data('table', () => {
const local = Alpine.reactive({
data: makeData(10_000),
- rowSource: 'filtered',
+ rowSource: 'filtered' as RowSource,
})
const table = createTable({
features,
Or Apply changes locally with:
npx nx-cloud apply-locally v22x-f6cr
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
…, call unguarded handlers directly setStateSlice now always guards, with isEqual as an override hook for custom feature slices. Slices that deliberately skip guarding (rowSelection, columnSizing, columnResizing, globalFilter, and the expanded/cellSelection setters) call their change handler directly instead of routing through a no-op wrapper, keeping pointer-frequency paths free of the routing overhead. rowPinning picks up the guard via the new default, matching columnPinning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
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
1 participant
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.
Summary
Supersedes #6519 (thanks @Faithfinder, the analysis and measurements there motivated this, and the commit is co-authored accordingly). Instead of adding a per-slice guard to
table_resetExpanded, this centralizes no-op suppression in one shared util that every guarded state slice routes through; the handful of hot-path slices that deliberately skip guarding call their change handler directly and say why in a comment.Guarded state slices now route through a single
setStateSlice(table, key, updater)util that guards with the structuralstateSlicesEqualcompare by default (isEqualremains an optional override for custom feature slices that want a cheaper or semantic-aware policy). The slice'son<Slice>Changehandler receives a guarded updater:Equality is evaluated inside the state owner's container, against the owner's own current value, never against the table's potentially stale snapshot. A structural no-op returns the owner's existing reference, and suppression rides on the owner's identity bailout:
makeStateUpdater) writes through the base atom, and@tanstack/storeatoms skip propagation entirely whenObject.is(old, next)holds. No notification, no re-render.setState(and the equivalent identity-bailing primitives in Solid, Vue, Svelte 5, Angular, and Preact) resolve the guarded updater against their own latest queued value and bail on the preserved reference, so no re-render and no newdatareference to re-trigger auto resets.Because the original updater only ever runs inside the owner, it runs exactly once per application and same-tick queued updates stay composable. An earlier revision resolved updaters eagerly against the table's snapshot; that could wrongly suppress a real update when the controlled owner had newer queued state, so the final design never resolves outside the owner.
stateSlicesEqualis a new depth-capped (3 container levels) structural compare covering every stock slice shape: arrays of fresh objects (sorting,columnFilters), nested arrays (columnPinning), array-valued filter values, null-prototype maps vs plain objects, enumerable symbols, and sparse arrays. Non-plain values (dates, class instances) and deeper nesting compare by reference; a false negative just lets the update proceed, so the compare always fails safe.Which slices are guarded
Guarded via
setStateSlice(all column-count-bounded; covers all four auto-reset slices):sorting,columnFilters,grouping,columnOrder,columnPinning,rowPinning,columnVisibility,paginationexpanded,cellSelectionDeliberately unguarded slices call their change handler directly (
table.options.on<Slice>Change?.(updater)) rather than routing throughsetStateSlice, so the guarded-or-not decision is visible in the call-site idiom and hot paths pay no routing overhead. Each site carries its rationale in a comment:rowSelection: no auto reset, and selection maps scale with row count; an O(n) compare on every gesture would be pure overheadcolumnSizing/columnResizing: pointer-frequency transient writes during resizeglobalFilter: a scalar, so the owner'sObject.isbailout already provides value equalityexpandedsetter: the sentinel-aware toggles already guard membership, a full compare could hit large row-id maps, and no structural compare can know thetruesentinel and a materialized row-id map are semantically interchangeablecellSelectionsetter: pointer-driven with potentially large ranges; the drag-extend focus short-circuit is keptThis makes every guarded setter, reset, and auto reset loop-proof: the #6499-style cycle (
dataidentity change → auto reset →onExpandedChange→ controlled consumer re-render → newdatareference → repeat) is broken for any state owner with identity-bailout semantics, which includes the table's own atoms and the state primitives of every first-party adapter framework.Removed code
table_resetPageIndex/table_resetPageSize(subsumed by the pagination guard; see behavior changes below)safeUpdaterwrapper intable_setPaginationKept deliberately:
row_toggleExpanded's membership guard andtable_toggleAllRowsExpanded's sentinel/emptiness guards. Without them, a no-op toggle under theexpanded: truesentinel would materialize the sentinel into a row-id map, which is a real state change the central compare would (correctly) let through. The cell-selection drag-extend focus comparison is also kept as a cheap allocation-avoiding short-circuit.Behavior changes
on<Slice>Changestill runs for a structural no-op (only that handler's state container can know its latest queued value), but the guarded updater preserves the container's existing reference, so no state write, re-render, or downstream notification occurs. Handler bodies with unconditional side effects will still see those side effects run.table_resetPageIndex/table_resetPageSizeno longer early-return before invoking the handler; the centralized guard turns the write into a no-op instead. The auto-reset path keeps a dedicated pre-check:table_autoResetPageIndexskips the reset entirely when already on the default page, so a user-providedonPaginationChangeis never invoked for an auto-reset no-op. This preserves call-level suppression where it matters most, since auto resets fire on every data, filter, sort, and grouping change, and side-effectful handlers (for example a refetch undermanualPaginationwithautoResetPageIndex: true) could otherwise self-sustain a refetch cycle. The check reads the pagination atom inside the scheduled, untracked auto-reset hook, so it registers no reactive dependencies.Updater<T>has always been value-or-function, so handlers that resolve withfunctionalUpdateare unaffected.@tracked, Svelte 4 writable stores) do not get the bail; the table's own atoms and all modern framework primitives do.Testing
tests/unit/setStateSlice.test.ts: compare semantics, no-op suppression for uncontrolled and controlled state, single execution of updaters, handler-invoked-but-reference-preserved semantics for controlled no-ops, the structural default guard and customisEqualoverrides, external-atom owners (ownership release composability), and clamped page navigation no-ops.packages/react-table/tests/expandedAutoResetLoop.test.tsx: the fix(table-core): wire expansion auto-reset into the core row model and guard first runs #6499/fix(table-core): don't fire onExpandedChange when resetExpanded is a no-op #6519 regression case with controlledexpandedand an unstabledatareference.table_autoResetPageIndexcases: the handler is not invoked when already on the default page, and still fires a real reset when off it.state/initialState) so they still cover their original semantics.rowPinningtests updated for its newly guarded setter (handlers now receive function-form updaters, resolved via the sharedgetUpdaterResulthelper).test:lib,test:types,test:eslint,test:knip, andbuild(size-limit passes at 24.77 kB) pass.🤖 Generated with Claude Code
Summary by CodeRabbit