petyosi / react-virtuoso

The most powerful virtual list component for React
https://virtuoso.dev
MIT License
5.25k stars 301 forks source link

Scroll jank when prepending async relay pagination data #248

Closed uncvrd closed 3 years ago

uncvrd commented 3 years ago

Hi! Great work on this library - really appreciate the standard support for inverse (chat) style scrolling. I have run in to a small issue when prepending data that comes from an asynchronous source that I believe draws from the small delay between when the data is appended and when firstItemIndex is changed and I was wondering if you had any suggestions?

For example, here's a video of the jank I am experiencing: https://streamable.com/x1q88p

You'll notice at around the 0:02 second mark there is some jitter as the data has been loaded but the start index has not been updated just yet. That animation jitter you see is actually the new data that was loaded before the scroll is pushed back down to where it should be by the firstItemIndex. The code to merge new data into the existing data array handled by Apollo is kind of long which is where I think the problem stems from as I explain below. Is there a way to update both the data and the firstItemIndex at the same time?

Here's my code, I am using Apollo Server relay style pagination (edges and nodes). FETCH_ITEM_COUNT is a constant that is set to 20:

<Virtuoso
    firstItemIndex={firstItemIndex}
    initialTopMostItemIndex={FETCH_ITEM_COUNT - 1}
    data={data?.artistMessages?.edges || []}
    startReached={fetch}
    itemContent={(index, message) => {
    return (
        message?.node && (
        <ChatMessage message={message.node} key={index} />
        )
    );
    }}
/>

I took influence from your prepending example and used useState to initialize a firstItemIndex like so:

const [firstItemIndex, setFirstItemIndex] = useState(START_INDEX);

Where START_INDEX is a constant set to an arbitrarily large value: 1,000,000.

When start is reached, I have a fetch method to that loads the next batch of data (cursor based pagination) like so:

const fetch = useCallback(
async (index: number) => {
    // check to confirm that there is data to query for before running the fetch
    if (fetchMore && data?.artistMessages?.pageInfo.hasPreviousPage) {
    const items = await fetchMore({
        variables: {
            last: FETCH_ITEM_COUNT,
            before: data.artistMessages?.pageInfo.startCursor,
            artistId: parseInt(artistId),
        },
    });
    // items contains the new elements, subtract the new items from the firstItemIndex
    setFirstItemIndex((curr) => {
        return curr - (items.data.artistMessages?.edges?.length || 0);
    });
    }
},
[data, fetchMore, artistId]
);

I call fetch and receive a new batch of data to prepend to my list. Notice how I do not have a useState for the data attribute of Virtuoso? This is because Apollo Server Relay pagination handles updating the data.artistMessages.edges internally. I'm thinking this is where the jank comes from since the code to prepend relay data is kind of lengthy see here. Apollo writes to client cache so that might be where the delay is coming from.

Now with the full scope of this issue, do you think it possible to be able to update the firstItemIndex and data at the same time? I may not fully understand the rendering process in React but I'm led to believe that having two points that need to be updated is causing the temporary misalignment of data on screen.

I thought I could utilize my datasource length to prevent needing a firstItemIndex state hook like so:

firstItemIndex={1000000 - (data?.artistMessages?.edges?.length || 0)}

However this results in an error on page load:

upwardScrollFixSystem.ts:93 Uncaught TypeError: Cannot read property 'originalIndex' of undefined

Let me know if there's anything else I can provide!! Thanks :)

petyosi commented 3 years ago

Sorry - can't help much here. Setting data and firstItemIndex them with subsequent calls seems to work fine in the example. Even if there's an 'incorrect' frame, it is likely a short one. I am not an Apollo user, so I can't advise on how to sync the prop values correctly.

uncvrd commented 3 years ago

@petyosi yes it likely has to do with the delay when writing to apollo's cache which I suppose will not work with this library with the dependance on a firstItemIndex being required on the component. Regardless thanks for advising!

petyosi commented 3 years ago

If you find a solution, please post it here; extending the library examples with data fetching strategies is something I want to do for a long time, but I am intimidated by the number of options out there!

uncvrd commented 3 years ago

No problem - totally understand that! Will do :)

uncvrd commented 3 years ago

@petyosi hey! ok I've recreated this problem in a codesandbox with a generic async request which highlights the scroll jank (Apollo is not included), it doesn't always happen because we may be getting lucky with the render loop. Having two properties (firstItemIndex and data) that need to be updated at the exact same time to prevent render jank is what I imagine is causing problems. Happens most often when scrolling slowly.

Streamable showing the issue: https://streamable.com/u3000q

Link to codesandbox: https://codesandbox.io/s/modern-meadow-24ljr?file=/src/Scroll.js

I hope this helps!!

petyosi commented 3 years ago

Thank you, I see what you mean. IMO, the jump is less severe than the initial recording, so I guess there is a space for optimization in the async scenario, too.

Apart from that, I am not sure that this is completely unavoidable. The prepend implemenation happens in two steps - a react re-render and an element.scrollTo method call. I can't think of how the two can be forced into a single frame, but maybe there is a way.

uncvrd commented 3 years ago

You're welcome! And yes I completely agree with it being less severe in this scenario due to no cache write delay when using Apollo Client. I understand the complication of the two step process, I'll brainstorm some ideas as well

uncvrd commented 3 years ago

I found a vue virtual scroll component that appears to handle prepending async information without the jank. I'm not too familiar with vue so it'll take some time for me to get a handle of how this works, but thought I'd pass this along for inspiration. https://tangbc.github.io/vue-virtual-scroll-list/#/chat-room

github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 1.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

petyosi commented 3 years ago

@uncvrd - I think the change in 1.2.0 improved the behavior. Let me know what you think.

seahorsepip commented 3 years ago

I still see a jump once in a every few load more on scrolls at https://virtuoso.dev/prepend-items/

If I understand correctly the implementation is as following:

  1. Scroll to top
  2. Load more items
  3. Insert items
  4. Wait for item heights
  5. Scroll top is compensated for new item heights

The jump probably happens around step 4.

What about wrapping new items in 0 height containers so they don't affect the list height/offsets until they have been measured? Then it would something like the following:

  1. Scroll to top
  2. Load more items
  3. Insert items (0 height wrapper)
  4. Wait for item heights (height inside the 0 height wrappers)
  5. Scroll top is compensated for new item heights, remove 0 height from item wrappers just before changing the scroll top.
petyosi commented 3 years ago

@seahorsepip can you record a video? I can't see a jump, but I am probably missing something. Introducing measurement containers is not something I want to deal with ATM, as it causes too much complexity.

seahorsepip commented 3 years ago

Jumps at 00:05, 00:18 and 00:21.

https://github.com/seahorsepip/uploads/raw/master/Prepending%20Items%20_%20React%20Virtuoso%20-%20Google%20Chrome%202020-12-28%2011-42-11.mp4

petyosi commented 3 years ago

@seahorsepip Thanks, what's the browser/OS?

seahorsepip commented 3 years ago

@petyosi Window 10 (latest insider slow ring) with latest stable Chrome. It's a 4k screen running most of the time on the intel integrated GPU so performance is a bit more limited unless I force the Nvidia GPU 😅.

seahorsepip commented 3 years ago

@petyosi Also keep in mind that there might be still scroll events emitted when the end is reached due to inertia from most touchpad drivers that might shift the scroll between measure and scroll compensation.

Never mind, also happens when I scroll with a mouse which doesn't have inertia.