Bug: shift causes render artifacts when items are added/removed mid-list

[mattwondra]

Describe the bug
shift works great when prepending items, but causes lots of rendering artifacts when adding/removing items from anywhere else in the list. To see it, enable shift with a list of items of varying heights and then:

• Add items to the middle of the visible list — the items above those inserted will have the wrong heights, as if the cached heights were not updated
• Remove items from the middle of the visible list — the same thing happens
• Scroll to the top, and prepend a small number of items to the top of the list, several times without scrolling — when you scroll up, there can be overlap and blank spaces

To Reproduce

Use the story in my branch and check out this demo for examples:

Screen.Recording.2023-12-12.at.4.50.54.PM.mp4

Expected behavior
Rendering glitches should not happen with shift enabled, no matter where in the list items are being added / removed.

Platform:

• OS: MacOS
• Browser: Chrome, Firefox, Safari
• Version of react: 18.2.0
• Version of this package: 0.17.5

Additional context
Add any other context about the problem here.

[inokawa]

It is not well documented, but by design, shift prop should be true when items are increased/decreased from top. And it should be false when you do other actions which increase/decrease items. Both true and false is OK when you do not increase/decrease items.

That is based on an assumption that increase/decrease at top and increase/decrease at mid/bottom will not happen at the same one render. In that shuffling/refreshing like case it might be better to set false to shift.

[mattwondra]

Got it... I wonder if I could give some more feedback that might adjust your thinking on how shift currently works then?

My concern is that shift can only be true in very specific circumstances, otherwise it causes obvious and hard-to-debug visual glitches. Using it correctly requires very fine-grained control over your render cycles and it's easy for users — myself included! — to do the wrong thing (see: footgun).

The examples in Storybook that use shift are very simplistic, knowing ahead of time where items will be added. But even these can be broken — for example, in Bi Directional Infinite Scrolling scroll all the way up (to initiate prepend and setShifting(true)), then all the way down (to initiate append with setShifting(false)). By the time the prepend actually happens, shift is false, meaning you get scrolled away from your position instead of staying the same spot as items are added. (See demo below.)

The solution to this (and I think a much more common use case) is to check whether to shift on the fly whenever items are changed. But this also seems error-prone. For example, if you keep a ref to the key of the first item, you could compare it in the render cycle and set shift if the key has changed. However if the component goes through multiple renders, this un-intuitively breaks. Here's a branch to go along with the following demo:

https://cloudup.com/c_Nm48VG4y2

Possible solution

Is it possible for Virtua to prevent the wrong thing from happening, by deciding itself whether or not to shift? Whenever it makes scroll adjustments, it checks whether the first items have changed (by comparing keys). This offloads that management from users, and also ensures that it's checking at the moment any jumps happen, to theoretically avoid double-render issues (like those highlighted in my demo).

What are your thoughts @inokawa? shift could still be a prop to enable this "shifting mode", but it would just stay true or false based on the user's preference, rather than changing every time items are changed.

[inokawa]

Thank you for your feedback! I agree with that staying true or false would be better, as long as its code can be small.

Contribution is welcome but I'm not sure it's possible. I didn't find out a way to do it completely when I implemented shift prop. Actually I started it with comparing key and length of the children on each render. However if users do sorting/refreshing/shuffling/filtering children it was hard to guess from them what users wanted to do. Checking start or end key was not enough but searching key from list on each children change will cost. That's the reason I took just passing direction of update with shift prop approach. And key is generated after ReactElement is created so we may not be able to rely on key if users choose render prop (implemented recently).

[LookRain]

hi @mattwondra , curious if you found any workaround for this issue?

[mattwondra]

@LookRain Hiya! We rolled our own checking to see whether shift should be enabled. It was kind of complex to set up, but seems to be working OK and is performant enough for us. Essentially:

• We keep a boolean ref called shouldShift that gets set, and passed into Virtua
  • It's a ref instead of state, because on the same render pass where a new message is added, we need to also correctly set shouldShift. Setting state wouldn't update the shift param until the next render pass, by which time Virtua would have already messed up the rendering with the wrong shift value.
• The array of messages that will be rendered is heavily memoized, so its reference only changes when an item has been added or removed.
• We also keep a ref for the previous list of messages, so that we can compare them whenever the list changes
• On every render pass (oof) we check whether the list has changed by comparing previousListData to listData
  • If they match, we do nothing — the list hasn't changed and we should keep the same shouldShift value
• If the list has changed, we check whether the first items match (by an id)
  • If they don't match, we know we're pre-pending and we set shouldShift.current = true
  • If they do match, we know an item was added somewhere else in the array and set shouldShift.current = false
• Then we update previousListData.current = listData for the next render pass

This ensures that every time listData changes, shouldShift also changes in the same render pass — but if another render happens soon after, shouldShift only changes back when necessary. Here's the hook code, hopefully it's useful for you with some modification:

/**
 * Virtua's `shift` prop helps maintain scroll position when prepending items (like message history).
 * Unfortunately it's finicky and must only be `true` when the beginning of the list changes, otherwise
 * rendering gets broken (see: https://github.com/inokawa/virtua/issues/284).
 *
 * Virtua also requires `shift` to be correct on the same render pass when items are updated — so we can't
 * just use `useEffect` and `useState` to monitor items and update `shift` since those will update _after_ the
 * render pass. Instead, we use refs to check if the underlying data has changed on each render pass, and
 * update a `shift` ref in the same pass.
 *
 * That's all encapsulated in this handy hook, to keep the logic out of the component.
 */
const useShift = (listData?: MessageListData) => {
  const previousListData = useRef<MessageListData | undefined>()
  const shouldShift = useRef<boolean>()
  if (listData !== previousListData.current) {
    if (listData?.messages[0]?.id !== previousListData.current?.messages[0]?.id) {
      shouldShift.current = true
    } else {
      shouldShift.current = false
    }
    previousListData.current = listData
  }
  return shouldShift.current
}