schrodinger/fixed-data-table-2

Error when refreshing data in table with "smaller" data

scott-lewis-nsidc opened this issue · 5 comments

scott-lewis-nsidc commented

In a project I am part of, we have several "tabs" each with a separate bucket of data being pulled in. When a tab is pressed, it refreshes the data table with data from the new bucket (the data itself is all the same format, just coming from a different "source", so to speak). We had to update to fixed-data-table-2 recently because fixed-data-table is apparently not maintained anymore. But in fixed-data-table, when the "new" data list is shorter than the "old" one and you've scrolled down, it would just gracefully keep the scroll at the end of the table. But with fixed-data-table-2, when the "new" data is shorter, it throws an error in our components, because it's passing in data rows that don't exist.

Expected Behavior

Refreshing a data table with new "shorter" data should not throw an error (perhaps just be at the bottom). Gracefully adjust to new data size.

Current Behavior

If the current scroll position would be beyond the end of the "new" data table, it tries to pass the non-existent row information to the table components, causing an error.

Possible Solution

When refreshing the data, if trying to look at rows beyond the end of the "new" data, adjust the row count to show the end of the table instead.

Steps to Reproduce (for bugs)

I will try to set up a sandbox if I can (the application itself is an internal one with no external way to view).

Context

More annoying than problematic in our particular case, as refreshing the page just causes things to reload properly.

Your Environment

  • Version used: 1.2.17
  • Browser Name and version: Firefox 112.0.2 64-bit (but I get the issue on other browsers).
  • Operating System and version (desktop or mobile): Ubuntu 22
AmanGupta2708 commented

Thanks @scott-lewis-nsidc for reporting this issue.

When rowCount changes, we change the scroll-position which set scrolling to true in redux store, (https://github.com/schrodinger/fixed-data-table-2/blob/master/src/reducers/index.js#L152) which looks wrong as we are not actually scrolling.

Because scrolling is true, in FixedDataBufferedRows, this.staticRowArray can have some rows before refreshing (https://github.com/schrodinger/fixed-data-table-2/blob/master/src/FixedDataTableBufferedRows.js#L97) if rowsCounts decreases, which could leads to NPE (as we try to render rows which no longer exist).

We can either reset this.staticRowArray, or not let scrolling to true in that case.

@pradeepnschrodinger

scott-lewis-nsidc commented

My vote would be to reset this.staticRowArray (which I believe would basically just be removing line https://github.com/schrodinger/fixed-data-table-2/blob/master/src/FixedDataTableBufferedRows.js#L98 and always just using the new row count).

pradeepnschrodinger commented

@AmanGupta2708, @scott-lewis-nsidc , good find and thanks for the explanations/context!

If it helps, here's a codesandbox link to reproduce the issue - https://codesandbox.io/s/dynamic-row-test-fixed-data-table-2-pbpc9p

The issue here is a bit tricky.
Unfortunately, we can't simply reset the this.staticRowArray list during scrolls since it can impact performance.

And the scrolling state should be true in this case since the scroll offsets indeed change.
The scroll state is also tied to handlers like onScrollStart and onScrollEnd which is used to alert changes in scroll offsets, so we need the state flip here.

pradeepnschrodinger commented

Unfortunately, we can't simply reset the this.staticRowArray list during scrolls since it can impact performance.

Context:

As you scroll down a table with variable row heights, the total no:of rows rendered at a given instant can change rapidly.
IF we only render the exact amount of rows, then React will have to keep mounting/unmounting new/old rows on every rendered frame.
This also leads to ALL the cells under these new/old rows being mounted/unmounted, which can slow down performance especially if the user has heavy cells.

So to avoid this, the logic in FixedDataBufferedRows lets the row list keep growing but never shrink during the entirety of the scroll.
This avoids unmounting of rows in most scenarios.
We then have the new rows directly replace the old rows in the list.

The bug here is that when rows are truncated during a scroll, there's not enough new rows to replace the old rows which are potentially "stale".
So this then results in at least 1 render frame where the cell renderer will be called with a stale row index, and depending on how the user defines the cell renderer it can crash the app.

Fix Guide:

We'll have to figure out a solution which keeps the performance benefit above as much as possible but still avoids stale rows being present in the list.

A quick fix would be to explicitly filter out (or unmount) the stale rows:

for (let i = 0; i < this._staticRowArray.length; i++) {
      let rowIndex = rowsToRender[i];

      ...

      if (!inRange(rowIndex, 0, rowSettings.rowsCount)) {
        this._staticRowArray[i] = null;
        continue;
      }
AmanGupta2708 commented

@scott-lewis-nsidc , this is fixed with #677 , and is available on v1.2.18.
Thanks for filing the issue.

I'm closing this off but feel free to post questions here if you face any trouble.

  • 🎉1