[BUG] Sometimes done() is not working when scroll was not needed

[eugene-g13]

Describe the bug

				tableVirtuosoRef.current?.scrollIntoView({
					index: nextIndex,
					behavior: 'auto',
					done: () => {
						setCurrentItemIndex(nextIndex);
					},
				});

done: () => { setCurrent...} is not working when table is just focused, first row is selected , but no scroll was actually needed.

Expected behavior

    /**
     * Will be called when the scroll is done, or immediately if no scroll is needed.
     */
    done?: () => void;

Desktop (please complete the following information):

• OS: iOs
• Browser Chrome
• React 18

[petyosi]

I need a reproduction of this, also ensure that you test across different browsers - the one you're pointing at is quite specific. It might be a browser specific thing, which will help me in my investigation.

[marioparaschiv]

I'm having this issue with the message list component too. A temporary workaround I found that might of use to you:

const { bottomOffset: originalBottomOffset } = ref.current.getScrollLocation();

ref.current.scrollToItem({
    index,
    align: 'center',
    behavior: type,
    done: () => scroller.emit('scroller-jumped', {
        messageId,
        type
    })
});

// If no scroll is required, the done callback will not trigger. We'll check this in the next animation frame.
requestAnimationFrame(() => {
    const { bottomOffset: currentBottomOffset } = ref.current?.getScrollLocation() ?? {};

    if (originalBottomOffset === currentBottomOffset) {
        scroller.emit('scroller-jumped', {
            messageId,
            type
        });
    }
});

[petyosi]

I'll take a look at this for the message list.

[marioparaschiv]

I'll take a look at this for the message list.

Thank you! Have you considered creating a GitHub repo for issues related to the Message List component?

[petyosi]

@marioparaschiv since the message-list is commercially licensed, I'm doing email support for it, you can always file such problems at support@virtuoso.dev.

To your point, I failed reproducing the problem you refer to, maybe there's something more to it. Here's what I tried, can you modify the example, so that I can observe the problem on my side? Thanks!

[marioparaschiv]

@petyosi Thank you for saving me some time with the codesandbox!

I've narrowed down the error to items that are at the end of the list (near the bottom).

My suspicion is an early return since the element cannot be centred in the scroller if its towards the bottom of the list (Turns out align: 'center' does not impact this issue).

Here is a reproduction.

Thank you for providing support here. I will use the email if I run into any other bugs.

[petyosi]

@marioparaschiv released a fix: https://codesandbox.io/p/sandbox/scroll-to-done-forked-xdzj4s.

[marioparaschiv]

Hey @petyosi

I am calling the replace imperative method like so:

ref.current.data.replace(messages, {
		purgeItemSizes: true,
		initialLocation: {
			index: 'LAST',
			align: 'end',
			behavior: 'smooth',
			done: () => {
				onScroll()
			}
		}
});

However, the done callback does not seem to be triggering.

Am I doing something wrong? I've tried without purgeItemSizes and also with supressItemMeasure, but to no avail.

[petyosi]

@marioparaschiv can you help me with a reproduction? https://codesandbox.io/p/devbox/scroll-to-done-forked-9qv3p9?workspaceId=ws_Qs9dF7G8JaG3MfhRri5CdK seems to work as expected, from what I see.

[marioparaschiv]

@marioparaschiv can you help me with a reproduction? codesandbox.io/p/devbox/scroll-to-done-forked-9qv3p9?workspaceId=ws_Qs9dF7G8JaG3MfhRri5CdK seems to work as expected, from what I see.

It turns out that, due to extremely fast updates, I was calling replace twice (once when jumping to latest along with once when loading the latest messages).

Having fixed that, I still notice a bit of some undesired behaviour.

I notice that some done callbacks do not run if another replace is called later.

A potential solution could be locking & queueing imperative data management until the replacement is complete.

Or, all done calls should be batched once the latest one completed. I'm not sure what the best solution would be here.

Here's a code sandbox reproducing this:

https://codesandbox.io/p/devbox/scroll-to-done-forked-l3r2wf?workspaceId=ws_6U2s7DoMiiTU2rEN6ZraVM

Also, one thing:

I'm replacing the data with an empty array to trigger my EmptyPlaceholder component where I show a skeleton until the latest messages are loaded.

If this behaviour is by design, please ignore this report. Thank you for your attention on this matter and I want to say that message list has been amazing. Thank you for your work!

[petyosi]

@marioparaschiv I think the sandbox is private, can you please check?

[marioparaschiv]

@marioparaschiv I think the sandbox is private, can you please check?

Fixed, sorry about that.

[petyosi]

@marioparaschiv examined the sandbox. Indeed, calling data modifications one after the other is problematic, especially when you combine them with scroll locations. Scrolling is asynchronous, and the component does a whole lot of internal juggling to measure and position elements correctly.

An internal queue is a nice idea - certainly something I will think about, but I'm not sure I can make that flexible enough.

In the meantime, something that (I believe) may help you is to construct/await promises for the actions. Here's an example of that, https://codesandbox.io/p/devbox/scroll-to-done-forked-fm8x75?workspaceId=ws_Qs9dF7G8JaG3MfhRri5CdK. Perhaps this would work for your case? The internal scheduling would do something similar, the way I think about it at least.