Skip to content

fix: clear multi-sort when clicking column without shift key #6084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: clear multi-sort when clicking column without shift key #6084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions packages/table-core/src/features/RowSorting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,12 +388,13 @@ export const RowSorting: TableFeature = {
sortAction = 'add'
}
} else {
// Normal mode
if (old?.length && existingIndex !== old.length - 1) {
sortAction = 'replace'
} else if (existingSorting) {
// 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'
}
Comment on lines +391 to 399
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
// 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.

}
Expand Down
207 changes: 207 additions & 0 deletions packages/table-core/tests/RowSorting.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,207 @@
import { describe, expect, it } from 'vitest'
import { createTable, getCoreRowModel, getSortedRowModel } from '../src'
import type { Person } from './makeTestData'
import { makeData } from './makeTestData'

const defaultData = makeData(3)

const defaultColumns = [
{
accessorKey: 'firstName' as keyof Person,
id: 'firstName',
},
{
accessorKey: 'lastName' as keyof Person,
id: 'lastName',
},
{
accessorKey: 'age' as keyof Person,
id: 'age',
},
{
accessorKey: 'visits' as keyof Person,
id: 'visits',
},
]

describe('RowSorting', () => {
it('should clear multi-sort when clicking column without shift key', () => {
let sorting = [
{ id: 'firstName', desc: false },
{ 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
},
})

expect(sorting).toHaveLength(2)
expect(sorting[0]).toEqual({ id: 'firstName', desc: false })
expect(sorting[1]).toEqual({ id: 'lastName', desc: true })

const ageColumn = table.getColumn('age')
ageColumn?.toggleSorting(false, false)

expect(sorting).toHaveLength(1)
expect(sorting[0]).toEqual({ id: 'age', desc: false })
})

it('should maintain multi-sort when clicking column with shift key', () => {
let sorting = [
{ id: 'firstName', desc: false },
{ 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(false, true)

expect(sorting).toHaveLength(3)
expect(sorting[0]).toEqual({ id: 'firstName', desc: false })
expect(sorting[1]).toEqual({ id: 'lastName', desc: true })
expect(sorting[2]).toEqual({ id: 'age', desc: false })
})

it('should toggle sort direction when clicking same column without shift key in single sort mode', () => {
let sorting = [{ id: 'firstName', desc: false }]

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 firstNameColumn = table.getColumn('firstName')
firstNameColumn?.toggleSorting(undefined, false)

expect(sorting).toHaveLength(1)
expect(sorting[0]).toEqual({ id: 'firstName', desc: true })
})

it('should replace multi-sort when clicking different column without shift key', () => {
let sorting = [
{ id: 'firstName', desc: false },
{ id: 'lastName', desc: true },
{ id: 'age', desc: false },
]

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 visitsColumn = table.getColumn('visits')
visitsColumn?.toggleSorting(false, false)

expect(sorting).toHaveLength(1)
expect(sorting[0]).toEqual({ id: 'visits', desc: false })
})

it('should work with getToggleSortingHandler', () => {
let sorting = [
{ id: 'firstName', desc: false },
{ 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')
const handler = ageColumn?.getToggleSortingHandler()

const mockEvent = { shiftKey: false }
handler?.(mockEvent)

expect(sorting).toHaveLength(1)
expect(sorting[0]).toEqual({ id: 'age', desc: true })
})

it('should work with getToggleSortingHandler with shift key', () => {
let sorting = [
{ id: 'firstName', desc: false },
{ 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')
const handler = ageColumn?.getToggleSortingHandler()

const mockEvent = { shiftKey: true }
handler?.(mockEvent)

expect(sorting).toHaveLength(3)
expect(sorting[0]).toEqual({ id: 'firstName', desc: false })
expect(sorting[1]).toEqual({ id: 'lastName', desc: true })
expect(sorting[2]).toEqual({ id: 'age', desc: true })
})
})