[BUG] Simple header breaks restore state location

[phazei]

Describe the bug If restoreStateFrom is used along with components Header, then the restoreStateFrom does not restore to the correct location

Reproduction https://codesandbox.io/p/sandbox/sandpack-project-forked-x869rf?file=%2FApp.js%3A193%2C15

To Reproduce Steps to reproduce the behavior: Scroll up, click the "save" button. Click the "restore" button. See the location jump.

Expected behavior Comment out line 194.

            components={{
                 //Header: Header,
            }}

Scroll up, click the "save" button. Click the "restore" button. See the location not jump.

[phazei]

I tried to debug this issue myself for a few hours, but honestly I just don't understand the urx streams at all, I can't even manage to figure out how to follow the flow.

In the stateLoadSystem, I found this which I think is the getState callback

    u.subscribe(
      u.pipe(getState, u.withLatestFrom(sizes, scrollTop, useWindowScroll, statefulWindowScrollContainerState, statefulWindowViewportRect)),
      ([callback, sizes, scrollTop, useWindowScroll, windowScrollContainerState, windowViewportRect]) => {
        const ranges = sizeTreeToRanges(sizes.sizeTree)
        if (useWindowScroll && windowScrollContainerState !== null && windowViewportRect !== null) {
          scrollTop = windowScrollContainerState.scrollTop - windowViewportRect.offsetTop
        }
        callback({ ranges, scrollTop })
      }
    )

And it seems that when I log the scrollTop is saving the correct position. I tried modifying the state.tsx example by adding a 200px height header. If I am at the very top and I save, the scrollTop is 0, when I restore it seems to compensate for the header a second time and it scrolls down 200px in addition to the scrollTop value. So if I scroll to item 0, and save, scrollTop is already 200px, and when I restore, it goes to item 8, which is 400px. It works perfectly if I manually subtract the header height from scrollTop before I save. Though I tried to retrieve the value from the stream it seems to be in, I see it grabbed from useEmitterValue in Virtuoso.tsx but whatever I tried I couldn't get it. I'd really love to help and debug it myself, but I can't even figure out where it's restoring it from, I think it's getting piped to initialTopMostItemIndex, but I can't make heads or tails of it.

[petyosi]

This is most likely a legit issue, but I am stretched very thin with my time; it's unlikely for me to be able to get to it at any point soon.

[phazei]

I think I figured out a solution.

The core of the issue is the scrolling restoration uses: initialTopMostItemIndex({ offset: snapshot!.scrollTop, index: 0, align: 'start' }

It's off-setting the saved scrollTop from the first indexed item, but that doesn't account for the Header height.

I attempted to grab the header height from domIOsystem in the tup in stateLoadSystem.ts:

    u.connect(
      u.pipe(
        u.combineLatest(restoreStateFrom, headerHeight),
        u.filter(([snapshot, height]) => u.isDefined(snapshot)),
        u.map(([snapshot, headerHeight]) => {
          return locationFromSnapshot(snapshot, headerHeight)
        })
      ),
      initialTopMostItemIndex
    )
    ----
    function locationFromSnapshot(snapshot: StateSnapshot | undefined, headerHeight: number) {
  return { offset: snapshot!.scrollTop - headerHeight, index: 0, align: 'start' }
}

After lots of debugging and adding a new isHeaderHeightSet bool in the Header: component inside Virtuoso.tsx because there was no way to tell if it was 0 or if it wasn't set yet, I realized that even after a stream var is set, for the first few cycles of the app they are stuck with their default values. I think maybe all the defined streams should be set on a reload before attempting to process anything else. I didn't need that boolean anyway since I realized I needed to check if a Header component was even set, but listComponentPropsSystem isn't exported so I couldn't find a way to check that or the props outside of the main Virtuoso.tsx.

While debugging I found I can add && height > 0 to the u.filter, but by the time headerHeight is available, the filter from initialTopMostItemIndexSystem:

        u.filter(([[, didMount], scrolledToInitialItem, { sizeTree }, defaultItemSize, scrollScheduled]) => {
          return didMount && (!empty(sizeTree) || u.isDefined(defaultItemSize)) && !scrolledToInitialItem && !scrollScheduled
        }),

and that fails because scrolledToInitialItem && scrollScheduled are already true.

This is a solution in stateLoadSystem that works:

    u.connect(
      u.pipe(
        u.combineLatest(restoreStateFrom, headerHeight),
        u.filter(([snapshot, height]) => u.isDefined(snapshot) && height > 0),
        u.map(([snapshot, headerHeight]) => {
          return locationFromSnapshot(snapshot, headerHeight)
        })
      ),
      scrollToIndex
    )

But it bypasses the initialTopMostItemIndexSystem, and also always expects the height so it's not a good solution.

So if the headerHeight wasn't available before all the checks, then maybe after would be the ideal time to get it.

So this is the final solution:

    u.subscribe(
      u.pipe(
        u.combineLatest(listRefresh, didMount),
        u.withLatestFrom(scrolledToInitialItem, sizes, defaultItemSize, initialItemFinalLocationReached),
        u.filter(([[, didMount], scrolledToInitialItem, { sizeTree }, defaultItemSize, scrollScheduled]) => {
          return didMount && (!empty(sizeTree) || u.isDefined(defaultItemSize)) && !scrolledToInitialItem && !scrollScheduled
        }),
        u.withLatestFrom(initialTopMostItemIndex)
      ),
      ([, initialTopMostItemIndex]) => {
        u.handleNext(scrollTargetReached, () => {
          u.publish(initialItemFinalLocationReached, true)
        })

        skipFrames(4, () => {
          u.handleNext(scrollTop, () => {
            u.publish(scrolledToInitialItem, true)
          })

          //New Code:
          //by the 3rd skipped frame the values has loaded if it exists
          const currentHeaderHeight = u.getValue(headerHeight)
          if (typeof initialTopMostItemIndex === 'object' && typeof initialTopMostItemIndex.offset === 'number') {
            initialTopMostItemIndex.offset -= currentHeaderHeight
          }
          //End New Code

          u.publish(scrollToIndex, initialTopMostItemIndex)
        })
      }
    )

I played with the skipFrames, and it needs to be at least 3 before the headerHeight value is properly populated, it was 4 to begin with, so all good now.