Sorting state behavior is changed if column data includes undefined values

[mcongrove]

Describe the bug

In some cases, when an undefined cell value is present in a column, the sorting toggle for that column doesn't follow the asc-desc-undefined pattern; instead, it may go asc-undefined, asc-desc, asc-undefined-desc-undefined, and a multitude of other variations.

I'm not sure exactly what's causing the bug to appear, but I did manage to get a somewhat simple repro case. I think this issue occurs when using manual sorting (whether or not passing in the manualSorting: true flag), and when the first row has an undefined value for a column. Interestingly, the problem disappears if you set sortDescFirst: false on the column with the undefined value.

Your minimal, reproducible example

https://codesandbox.io/s/fragrant-framework-2noe5r?file=/src/main.tsx

Steps to reproduce

1. Click on the lastName header, note that sort goes into desc mode
2. Click on the lastName header again, note that sort goes into undefined sort mode
3. Click on the lastName header once more, note that sort goes back into desc mode and skips asc

Expected behavior

As a user, I expect that clicking on a sortable header would toggle between the asc-desc-undefined states (or reverse, if sortDescFirst).

How often does this bug happen?

Every time

Screenshots or Videos

Platform

• OS: Mac
• Browser: Chrome

react-table version

v8.5.11

TypeScript version

v4.7.4

Additional context

No response

Terms & Code of Conduct

• I agree to follow this project's Code of Conduct
• I understand that if my bug cannot be reliable reproduced in a debuggable environment, it will probably not be fixed and this issue may even be closed.

[Dysas]

I had a similar problem. In my case, some data values I was sorting had null values which caused this problem. What I have done is, I iterated through all object values and changed null to an empty string (null -> '')

If not for this issue I guess I would be going crazy... Thanks

[vruffer]

I have also experienced this problem, it isn't just limited to sorting either. If you enable global filtering, filterFn will not be invoked with the columnId of columns with an undefined value in the first row.

[FINDarkside]

There seems to be something more weird going on with undefined values, they're not really sorted properly at all. See repro: https://codesandbox.io/s/exciting-shape-5xgj1q?file=/src/main.tsx:573-592

Or when the column has numbers, the numbers won't even be sorted. Those unfedined values will somehow split these numbers to several different groups and the rows are only sorted inside their own groups. Repro: https://codesandbox.io/s/quizzical-paper-jkeu92?file=/src/main.tsx

E: Seems like that's intended behavior and you need to add sortUndefined option to every column.

[simplecommerce]

I have the same problem here where I have values that are null, I thought by adding manualSorting to true would fix it but it didn't.

Isn't there a way to use manualSorting to true and have the next available sort based on the sortDir instead?

[saevarb]

I also just ran into this issue. However, in my case, the sorting order just goes from nothing to desc and back to nothing. I am using manualSorting (sorting server-side).

TLDR: Workaround at the bottom.

After digging through the code and adding some breakpoints, I think I have figured out what the issue is. The code tries to determine the first sort direction automatically based on the value in the first row:

      getAutoSortDir: () => {
        const firstRow = table.getFilteredRowModel().flatRows[0]

        const value = firstRow?.getValue(column.id)

        if (typeof value === 'string') {
          return 'asc'
        }

        return 'desc'
      },

In my case, on the data I'm testing with, value in this code, for my first row, becomes null(it's undefined in my data but I assume it's being converted to null). So getAutoSortDir returns "desc". Once it has been sorted, value is actually a string, so that gives us "asc" the second time this function is called. This gets used in getFirstSortDir:

      getFirstSortDir: () => {
        const sortDescFirst =
          column.columnDef.sortDescFirst ??
          table.options.sortDescFirst ??
          column.getAutoSortDir() === 'desc'
        return sortDescFirst ? 'desc' : 'asc'
      },

which itself gets used in getNextSortingOrder:

      getNextSortingOrder: (multi?: boolean) => {
        const firstSortDirection = column.getFirstSortDir()
        const isSorted = column.getIsSorted()

        if (!isSorted) {
          return firstSortDirection
        }

        if (
          isSorted !== firstSortDirection &&
          (table.options.enableSortingRemoval ?? true) && // If enableSortRemove, enable in general
          (multi ? table.options.enableMultiRemove ?? true : true) // If multi, don't allow if enableMultiRemove))
        ) {
          return false
        }
        return isSorted === 'desc' ? 'asc' : 'desc'
      },

This means the second if statement here returns false immediately on the second time I sort the column, because the current sort direction isn't equal to the first sort direction, since it changes between invocations.
Here is some output from my logpoints that demonstrates the issue. The first is the output from one column containing numbers without any undefineds(termCount), the other is a column of dates(as strings, publishedDate) that contains undefined values.

first row value for termCount  =  8
sortDescFirst for termCount  =  true
firstSortDir for termCount  =  desc
first row value for termCount  =  1557
sortDescFirst for termCount  =  true
firstSortDir for termCount  =  desc
first row value for termCount  =  0
sortDescFirst for termCount  =  true
firstSortDir for termCount  =  desc

first row value for publishedDate  =  null
sortDescFirst for publishedDate  =  true
firstSortDir for publishedDate  =  desc
first row value for publishedDate  =  2023-02-18T13:53:23.000Z
sortDescFirst for publishedDate  =  false
firstSortDir for publishedDate  =  asc

I would make a pull request to fix this problem, but I feel I don't have the required context to understand why tanner decided to have the special string exception in getAutoSortDir, so I'm not entirely sure what the correct way would be to solve this issue.

Workaround

Thankfully there is a solution, though. One way is to do something like (data) => data.colWithUndefined ?? "" -- this seems to work. But we can also just explicitly set the sortDescFirst in the column's definition, thus avoiding the broken getAutoSortDir code.

[simplecommerce]

I also just ran into this issue. However, in my case, the sorting order just goes from nothing to desc and back to nothing. I am using manualSorting (sorting server-side).

TLDR: Workaround at the bottom.

After digging through the code and adding some breakpoints, I think I have figured out what the issue is. The code tries to determine the first sort direction automatically based on the value in the first row:

      getAutoSortDir: () => {
        const firstRow = table.getFilteredRowModel().flatRows[0]

        const value = firstRow?.getValue(column.id)

        if (typeof value === 'string') {
          return 'asc'
        }

        return 'desc'
      },

In my case, on the data I'm testing with, value in this code, for my first row, becomes null(it's undefined in my data but I assume it's being converted to null). So getAutoSortDir returns "desc". Once it has been sorted, value is actually a string, so that gives us "asc" the second time this function is called. This gets used in getFirstSortDir:

      getFirstSortDir: () => {
        const sortDescFirst =
          column.columnDef.sortDescFirst ??
          table.options.sortDescFirst ??
          column.getAutoSortDir() === 'desc'
        return sortDescFirst ? 'desc' : 'asc'
      },

which itself gets used in getNextSortingOrder:

      getNextSortingOrder: (multi?: boolean) => {
        const firstSortDirection = column.getFirstSortDir()
        const isSorted = column.getIsSorted()

        if (!isSorted) {
          return firstSortDirection
        }

        if (
          isSorted !== firstSortDirection &&
          (table.options.enableSortingRemoval ?? true) && // If enableSortRemove, enable in general
          (multi ? table.options.enableMultiRemove ?? true : true) // If multi, don't allow if enableMultiRemove))
        ) {
          return false
        }
        return isSorted === 'desc' ? 'asc' : 'desc'
      },

This means the second if statement here returns false immediately on the second time I sort the column, because the current sort direction isn't equal to the first sort direction, since it changes between invocations. Here is some output from my logpoints that demonstrates the issue. The first is the output from one column containing numbers without any undefineds(termCount), the other is a column of dates(as strings, publishedDate) that contains undefined values.

first row value for termCount  =  8
sortDescFirst for termCount  =  true
firstSortDir for termCount  =  desc
first row value for termCount  =  1557
sortDescFirst for termCount  =  true
firstSortDir for termCount  =  desc
first row value for termCount  =  0
sortDescFirst for termCount  =  true
firstSortDir for termCount  =  desc

first row value for publishedDate  =  null
sortDescFirst for publishedDate  =  true
firstSortDir for publishedDate  =  desc
first row value for publishedDate  =  2023-02-18T13:53:23.000Z
sortDescFirst for publishedDate  =  false
firstSortDir for publishedDate  =  asc

I would make a pull request to fix this problem, but I feel I don't have the required context to understand why tanner decided to have the special string exception in getAutoSortDir, so I'm not entirely sure what the correct way would be to solve this issue.

Thankfully there is a solution, though. One way is to do something like (data) => data.colWithUndefined ?? "" -- this seems to work. But we can also just explicitly set the sortDescFirst in the column's definition, thus avoiding the broken getAutoSortDir code.

In my project, instead of using getToggleSortingHandler I decided to handle it manually like so:

const handleClick = e => {
		switch (sortDir) {
			case "asc":
				header.column.toggleSorting(true, header.column.getCanMultiSort() ? tableInstance.options.isMultiSortEvent?.(e) : false);
				break;
			case "desc":
				header.column.clearSorting();
				break;
			default:
				header.column.toggleSorting(false, header.column.getCanMultiSort() ? tableInstance.options.isMultiSortEvent?.(e) : false);
				break;
		}
	};

I got inspired by looking at how it handles it inside the code and simply just forced it on my end. It works for me at the oment for my cases.

My thought was, if manualSorting is set to true, that getToggleSortingHandler would just ignore the sorting mechanism and just toggle between the states based on the previous one instead (since you are supposed to handle the sorting manually anyway) which would make sense when I think about it.

[jackmcpickle]

Thanks @saevarb

Summed up nicely here.

Thankfully there is a solution, though. One way is to do something like (data) => data.colWithUndefined ?? "" -- this seems to work. But we can also just explicitly set the sortDescFirst in the column's definition, thus avoiding the broken getAutoSortDir code.

Adding sortDescFirst: true on the ColumnDef was the ticket for me to help with my server side sorting.

[mathiaswillburger]

The most suitable approach I found for my server-side sorting table was to add it to the defaultColumn property, so it's applied to all columns automatically everywhere.

    defaultColumn: {
      sortDescFirst: true,
      manualSorting: true,
    },

[JoepKockelkorn]

This was un extremely unintuitive solution, but sortDescFirst: false worked for me. It forces sorting to always follow the asc-desc-undefined cycle, even when there is undefined data in a cell. This should be the default, in my opinion.

[KevinVandy]

This was un extremely unintuitive solution, but sortDescFirst: false worked for me. It forces sorting to always follow the asc-desc-undefined cycle, even when there is undefined data in a cell. This should be the default, in my opinion.

Hoping this is not seen as "extremely unintuitive" going forward, as I've added a callout to this situation in the sorting guide. It is indeed the official way to control this behavior.