fix: clear multi-sort when clicking column without shift key #6084
Conversation
- Fixed sorting logic in RowSorting.ts to properly clear existing multi-sort when clicking without shift key - Added comprehensive tests to verify the fix works correctly - Ensures single column click replaces all existing sorts - Maintains existing behavior for shift+click multi-sort Fixes TanStack#6070
WalkthroughRefactors non-multi-sort behavior in RowSorting to clear existing multi-column sorts on normal clicks unless exactly one sort exists, in which case it toggles that column. Adds a new Vitest test suite validating single- and multi-sort transitions, shift vs non-shift clicks, and handler behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as ColumnHeader
participant Table as Table Core
participant RS as RowSorting
User->>UI: Click column header (with/without Shift)
UI->>Table: onClick({ columnId, shiftKey })
Table->>RS: toggleSorting(columnId, shiftKey)
alt shiftKey = true (multi-sort)
RS->>Table: Add/Toggle column in sorting array (preserve others)
else shiftKey = false (single-sort)
alt existing sorts length == 1 and same column
RS->>Table: Toggle sort direction for column
else existing sorts length != 1 or different column
RS->>Table: Replace sorting with [column] only
end
end
Table-->>UI: Updated sorting state
UI-->>User: Re-rendered rows with new order
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/table-core/src/features/RowSorting.ts (1)
369-372: Pass the multi flag to getNextSortingOrder to respect enableMultiRemoveWithout passing multi, removal in multi-sort mode may be allowed even when enableMultiRemove is false.
- const nextSortingOrder = column.getNextSortingOrder() + const nextSortingOrder = column.getNextSortingOrder(multi)
🧹 Nitpick comments (3)
packages/table-core/src/features/RowSorting.ts (2)
375-376: Remove unused variable existingIndexMinor cleanup; it’s computed but never used.
- const existingIndex = old?.findIndex(d => d.id === column.id)
391-396: Refresh the comment to reflect the refined behaviorAfter the fix above, “always replace” is no longer accurate when nextSortingOrder is false; we now “remove”. Tweak the comment accordingly.
packages/table-core/tests/RowSorting.test.ts (1)
27-206: Add a test to respect enableMultiRemove=false with shift togglesEnsures getNextSortingOrder receives multi=true and honors the gating.
Proposed test:
it('does not remove sort on shift toggle when enableMultiRemove is false', () => { let sorting = [ { id: 'firstName', desc: false }, // first sort dir for strings is 'asc' { id: 'lastName', desc: true }, ] const table = createTable<Person>({ data: defaultData, columns: defaultColumns, getCoreRowModel: getCoreRowModel(), getSortedRowModel: getSortedRowModel(), onStateChange() {}, renderFallbackValue: '', state: { sorting }, enableMultiRemove: false, onSortingChange: updater => { sorting = typeof updater === 'function' ? updater(sorting) : updater }, }) const firstName = table.getColumn('firstName') firstName?.toggleSorting(undefined, true) // shift toggle // Should toggle to 'desc' (not remove), and keep lastName expect(sorting).toEqual([ { id: 'firstName', desc: true }, { id: 'lastName', desc: true }, ]) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/table-core/src/features/RowSorting.ts(1 hunks)packages/table-core/tests/RowSorting.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/table-core/tests/RowSorting.test.ts (3)
packages/table-core/src/core/table.ts (1)
createTable(283-527)packages/table-core/src/utils/getCoreRowModel.ts (1)
getCoreRowModel(5-82)packages/table-core/src/utils/getSortedRowModel.ts (1)
getSortedRowModel(5-121)
🔇 Additional comments (7)
packages/table-core/tests/RowSorting.test.ts (7)
27-58: LGTM: verifies clearing multi-sort on simple clickThis test accurately captures the intended UX regression and the fix. Nice.
60-88: LGTM: shift+click multi-sort path preservedCovers additive behavior; assertions look good.
90-114: LGTM: single-sort toggling worksGood sanity check for the non-multi path.
115-142: LGTM: replacing multi-sort by clicking a different columnConfirms the primary UX expectation.
144-173: Handler test is solid; relies on inferred first sort directionLooks correct given age is numeric (desc first). No change needed.
175-206: LGTM: handler with shift maintains existing sortsCovers the event-based multi path.
27-206: Add test for cycling to “none” in multi-sort clear scenario
Ensure toggling a column transitions to an empty sort array when the next state is “none” during a multi→single click. Example to add at packages/table-core/tests/RowSorting.test.ts:it('clears to no sorting when next order is remove on normal click', () => { // age defaults to 'desc' first; set it to 'asc' so next is 'none' let sorting = [ { id: 'age', desc: false }, // asc { id: 'lastName', desc: true }, ] const table = createTable<Person>({ data: defaultData, columns: defaultColumns, getCoreRowModel: getCoreRowModel(), getSortedRowModel: getSortedRowModel(), onStateChange() {}, renderFallbackValue: '', state: { sorting }, onSortingChange: updater => { sorting = typeof updater === 'function' ? updater(sorting) : updater }, }) const ageColumn = table.getColumn('age') ageColumn?.toggleSorting(undefined, false) // normal click expect(sorting).toEqual([]) // should remove all sorts })
| // Normal mode - always replace when not in multi-sort mode | ||
| // This ensures that clicking without shift key clears existing multi-sort | ||
| if (existingSorting && old?.length === 1) { | ||
| // Only one column sorted, so we can toggle | ||
| sortAction = 'toggle' | ||
| } else { | ||
| // Multiple columns sorted or no existing sort, replace all | ||
| sortAction = 'replace' | ||
| } |
There was a problem hiding this comment.
Honor “remove” (none) state when clearing multi-sort with a normal click
When nextSortingOrder is false (enableSortingRemoval path), the non-multi branch always “replace”s and ends up forcing a sort with desc=false instead of removing sorting entirely. That breaks the expected cycle 'desc' → 'asc' → 'none' in the multi→single transition.
Apply this diff to respect the remove state:
- // Normal mode - always replace when not in multi-sort mode
- // This ensures that clicking without shift key clears existing multi-sort
- if (existingSorting && old?.length === 1) {
- // Only one column sorted, so we can toggle
- sortAction = 'toggle'
- } else {
- // Multiple columns sorted or no existing sort, replace all
- sortAction = 'replace'
- }
+ // Normal mode - clear existing multi-sort on simple click
+ if (existingSorting && old?.length === 1) {
+ // Only one column sorted, so we can toggle
+ sortAction = 'toggle'
+ } else {
+ // Multiple columns sorted or no existing sort
+ // If next state is "remove", clear all sorting rather than forcing a sort
+ sortAction =
+ !hasManualValue && nextSortingOrder === false ? 'remove' : 'replace'
+ }📝 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.
| // Normal mode - always replace when not in multi-sort mode | |
| // This ensures that clicking without shift key clears existing multi-sort | |
| if (existingSorting && old?.length === 1) { | |
| // Only one column sorted, so we can toggle | |
| sortAction = 'toggle' | |
| } else { | |
| // Multiple columns sorted or no existing sort, replace all | |
| sortAction = 'replace' | |
| } | |
| // Normal mode - clear existing multi-sort on simple click | |
| if (existingSorting && old?.length === 1) { | |
| // Only one column sorted, so we can toggle | |
| sortAction = 'toggle' | |
| } else { | |
| // Multiple columns sorted or no existing sort | |
| // If next state is "remove", clear all sorting rather than forcing a sort | |
| sortAction = | |
| !hasManualValue && nextSortingOrder === false ? 'remove' : 'replace' | |
| } |
🤖 Prompt for AI Agents
In packages/table-core/src/features/RowSorting.ts around lines 391 to 399, the
non-multi branch always sets sortAction = 'replace' which forces a sort instead
of honoring the "remove" (none) state when nextSortingOrder is false; change the
logic so that when existingSorting && old?.length === 1 you choose between
'toggle' and 'remove' based on nextSortingOrder (i.e., if nextSortingOrder ===
false use 'remove', otherwise use 'toggle'), and otherwise keep 'replace' for
multi/no-existing-sort cases so the desc→asc→none cycle is preserved during
multi→single transitions.
Fixes #6070
Summary by CodeRabbit
New Features
Tests