feat: add autoResetSorting #5414
Conversation
|
View your CI Pipeline Execution ↗ for commit 419bec9
☁️ Nx Cloud last updated this comment at |
|
What's the use-case for this? Usually changing the sorting state without user-input is bad UX, right? |
Personally I agree that changing the sorting state without user-input is bad UX. But there's still some designers prefer automatically resetting the sorting state when data changes. |
|
Also, see #5217 |
|
ok, open to merging this, but need to test more. Definitely agree that this should not be enabled by default like some of the other autoReset options though. |
|
WalkthroughAdds an autoResetSorting option to the sorting feature, exposes a new internal _autoResetSorting hook on the table instance, and wires it to run during sorted row model memoization alongside _autoResetPageIndex. Documentation updated to reflect the new option at both table and column levels. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User/Consumer
participant T as Table Instance
participant RS as RowSorting Feature
participant GM as getSortedRowModel Memo
U->>T: setOptions / setState (sorting change)
T->>GM: compute sorted row model (memo)
GM->>T: _autoResetSorting() Note over GM,T: New side-effect
T->>RS: _autoResetSorting()
RS->>RS: if (autoResetAll || autoResetSorting) && !manualSorting
alt should auto-reset
RS-->>T: queue resetSorting() after current cycle
else no reset
RS-->>T: do nothing
end
GM->>T: _autoResetPageIndex() Note over GM,T: Existing pattern
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/api/features/sorting.md(2 hunks)packages/table-core/src/features/RowSorting.ts(3 hunks)packages/table-core/src/utils/getSortedRowModel.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/table-core/src/utils/getSortedRowModel.ts (1)
packages/table-core/src/utils.ts (1)
getMemoOptions(202-219)
🪛 LanguageTool
docs/api/features/sorting.md
[grammar] ~327-~327: There might be a mistake here.
Context: ...ly reset the sorting state of the table when sorting state changes. ### `sortDescFi...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (2)
packages/table-core/src/features/RowSorting.ts (1)
255-255: LGTM!The internal
_autoResetSortingmethod is correctly declared and follows the established pattern for auto-reset hooks.packages/table-core/src/utils/getSortedRowModel.ts (1)
117-120: LGTM!The integration of
_autoResetSorting()correctly follows the same pattern as_autoResetPageIndex(). The callback is triggered when the memoized row model changes, which occurs when either the sorting state or the underlying data (pre-sorted row model) changes.
| ### `autoResetSorting` | ||
| ```tsx | ||
| autoResetSorting?: boolean | ||
| ``` | ||
| Enable this setting to automatically reset the sorting state of the table when sorting state changes. | ||
There was a problem hiding this comment.
Fix the misleading description to accurately describe when auto-reset occurs.
The description states "when sorting state changes," which is circular and inaccurate. The auto-reset is triggered when the underlying data changes (when getPreSortedRowModel() returns a new value), not when the sorting state itself changes.
Apply this diff to fix the documentation:
-Enable this setting to automatically reset the sorting state of the table when sorting state changes.
+Enable this setting to automatically reset the sorting state of the table when the underlying data changes.Based on static analysis hints.
📝 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.
| ### `autoResetSorting` | |
| ```tsx | |
| autoResetSorting?: boolean | |
| ``` | |
| Enable this setting to automatically reset the sorting state of the table when sorting state changes. | |
| ### `autoResetSorting` | |
🧰 Tools
🪛 LanguageTool
[grammar] ~327-~327: There might be a mistake here.
Context: ...ly reset the sorting state of the table when sorting state changes. ### `sortDescFi...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In docs/api/features/sorting.md around lines 321 to 328, the description for
`autoResetSorting` is misleading: it says it resets "when sorting state changes"
but actually triggers when the underlying data changes (i.e., when
getPreSortedRowModel() returns a new value). Update the wording to state that
enabling `autoResetSorting` will automatically reset the table's sorting state
when the underlying row data changes (pre-sorted row model changes), not when
the sorting state itself changes, and keep the prop signature and example
intact.
| /** | ||
| * Enable this setting to automatically reset the sorting state of the table when sorting state changes. | ||
| * @link [API Docs](https://tanstack.com/table/v8/docs/api/features/sorting#autoresetsorting) | ||
| * @link [Guide](https://tanstack.com/table/v8/docs/guide/sorting) | ||
| */ | ||
| autoResetSorting?: boolean |
There was a problem hiding this comment.
Clarify the documentation to accurately describe when auto-reset occurs.
The comment states "when sorting state changes," which is circular and misleading. Based on the implementation, this feature resets sorting when the underlying data changes (specifically when getPreSortedRowModel() returns a new value), not when the sorting state itself changes.
Apply this diff to clarify the documentation:
/**
- * Enable this setting to automatically reset the sorting state of the table when sorting state changes.
+ * Enable this setting to automatically reset the sorting state of the table when the underlying data changes.
* @link [API Docs](https://tanstack.com/table/v8/docs/api/features/sorting#autoresetsorting)
* @link [Guide](https://tanstack.com/table/v8/docs/guide/sorting)
*/📝 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.
| /** | |
| * Enable this setting to automatically reset the sorting state of the table when sorting state changes. | |
| * @link [API Docs](https://tanstack.com/table/v8/docs/api/features/sorting#autoresetsorting) | |
| * @link [Guide](https://tanstack.com/table/v8/docs/guide/sorting) | |
| */ | |
| autoResetSorting?: boolean | |
| /** | |
| * Enable this setting to automatically reset the sorting state of the table when the underlying data changes. | |
| * @link [API Docs](https://tanstack.com/table/v8/docs/api/features/sorting#autoresetsorting) | |
| * @link [Guide](https://tanstack.com/table/v8/docs/guide/sorting) | |
| */ | |
| autoResetSorting?: boolean |
🤖 Prompt for AI Agents
In packages/table-core/src/features/RowSorting.ts around lines 198 to 203, the
JSDoc for autoResetSorting is misleading because it says "when sorting state
changes" — update the comment to state that autoResetSorting controls whether
sorting is automatically reset when the underlying row data changes
(specifically when getPreSortedRowModel() returns a new value), not when the
sorting state itself changes; keep the existing API Docs and Guide links and
wording about enabling the setting but replace the phrase to reference
data/pre-sorted row model changes.
| let registered = false | ||
| let queued = false | ||
|
|
||
| table._autoResetSorting = () => { | ||
| if (!registered) { | ||
| table._queue(() => { | ||
| registered = true | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| if ( | ||
| table.options.autoResetAll ?? | ||
| table.options.autoResetSorting ?? | ||
| !table.options.manualSorting | ||
| ) { | ||
| if (queued) return | ||
| queued = true | ||
| table._queue(() => { | ||
| table.resetSorting() | ||
| queued = false | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The default behavior contradicts the requirement that auto-reset should not be enabled by default.
The condition on lines 543-546 evaluates to true by default when neither autoResetAll nor autoResetSorting is set and manualSorting is not enabled. This means the feature is enabled by default, which contradicts the maintainer's explicit requirement in the PR comments that "the feature should not be enabled by default, similar to other autoReset options."
Apply this diff to disable the feature by default:
if (
table.options.autoResetAll ??
- table.options.autoResetSorting ??
- !table.options.manualSorting
+ table.options.autoResetSorting
) {This change ensures that auto-reset only occurs when:
autoResetAllis explicitly set totrue, ORautoResetSortingis explicitly set totrue
Without this change, existing applications not using manualSorting will experience unexpected sorting resets when data changes, which could be a breaking change.
📝 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.
| let registered = false | |
| let queued = false | |
| table._autoResetSorting = () => { | |
| if (!registered) { | |
| table._queue(() => { | |
| registered = true | |
| }) | |
| return | |
| } | |
| if ( | |
| table.options.autoResetAll ?? | |
| table.options.autoResetSorting ?? | |
| !table.options.manualSorting | |
| ) { | |
| if (queued) return | |
| queued = true | |
| table._queue(() => { | |
| table.resetSorting() | |
| queued = false | |
| }) | |
| } | |
| } | |
| let registered = false | |
| let queued = false | |
| table._autoResetSorting = () => { | |
| if (!registered) { | |
| table._queue(() => { | |
| registered = true | |
| }) | |
| return | |
| } | |
| if ( | |
| table.options.autoResetAll ?? | |
| table.options.autoResetSorting | |
| ) { | |
| if (queued) return | |
| queued = true | |
| table._queue(() => { | |
| table.resetSorting() | |
| queued = false | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/table-core/src/features/RowSorting.ts around lines 532 to 555, the
current condition enables auto-reset by default when neither autoResetAll nor
autoResetSorting is set and manualSorting is false; change the conditional so
auto-reset only runs when autoResetAll or autoResetSorting are explicitly true
(e.g. replace the truthy-coalescing check with an explicit equality check:
table.options.autoResetAll === true || table.options.autoResetSorting === true),
leaving the registered/queued logic and table._queue call intact.
2 participants
Summary by CodeRabbit
New Features
Documentation