Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

native: increase channel scroll position stability when loading to anchor #3878

Merged
merged 19 commits into from
Sep 5, 2024

Conversation

davidisaaclee
Copy link
Contributor

@davidisaaclee davidisaaclee commented Sep 3, 2024

Description Before After
Simulated load around anchor – here's the worst case I could find for the "before" case - most of the time it's relatively stable with a small blip in position.
before-load-around-worstcase.mp4
after-load-around.mp4
Load "blitz" – load posts out of order, with anchor not in first page. This is an unlikely scenario, but illustrates what things look like in a worst-case, possibly buggy release.
before-load-blitz.mp4
after-load-blitz.mp4
Load newer posts while locked to anchor – after loading channel with anchor locked to top, I pressed the Load newer button that adds posts to the "new" end of the timeline
before-loading-newer-posts.mp4
after-loading-newer-posts.mp4
  • [extra] Replace all Math.random with a seeded random in fakeData.ts – this ensures that Cosmos fixtures always look the same (unless you change the seed).
  • Added two Channel fixtures: one that simply loads a chat with an unread anchor, and one that has a toolbar for interactively loading posts in different ways.
  • Reorganization of anchor scroll lock code – hidden in this reorg are the following key changes:
    • We previously attempted to scroll to anchor whenever any list item got laid out (and we believed we had an anchorIndex) – removed attempts to scroll when non-anchor list items are laid out. (My guess is that this used to be the case to emulate maintainVisibleContentPosition as list items moved the anchor around, but we can rely on mvcp now.)
    • Previously, when a scroll-to-anchor failed, we scrolled to a "best guess" of the anchor and rested (probably relying on the "did layout" scroll attempt to fix stuff). Now, on a scroll-to-anchor failure, we mark ourselves as explicitly needsScrollToAnchor = true, and wait for an anchor layout to trigger a scroll.

I kept the existing code for autoscrollToTopThreshold, but I believe that that feature is broken in the build of RN that we are using – we should apply facebook/react-native#38245 (tested in a quick-n-dirty patch and appears to work)

Content is still shifting a little bit as things load in: I believe this is due to images in messages resizing themselves, and thus their ChatMessage containers.

I've logged into this client on my live ship and browsed around to channels with lots of unreads, and some with no unreads – things seem fairly stable. (Did the same with current release client and it is less scroll-stable.)

- Rename (flipped) `hasFoundAnchor` -> `needsScrollToAnchor`
- Move scroll-to-anchor to shared callback with built-in checks
- Stop attempting imperative scroll-to-anchor when laying out any item
- Attempt scroll-to-anchor again after failing to scroll
- Disable autoscrollToTopThreshold when user hasn't scrolled yet
- Use enum to explicitly track anchor layout status
- Mark page as ready for display if either (1) scroll-to-anchor
  succeeded, or (2) we gave up waiting for anchor
- Avoid concurrent attempts to scroll to anchor
- Animate scroll when jumping to anchor if we're already showing the
  page; if we're not showing, jump without animation
@galenwp
Copy link

galenwp commented Sep 3, 2024

Heroic

Copy link
Member

@latter-bolden latter-bolden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much easier to read through! Appreciate the copious amount of comments <3

The posts disappearing during search scrollback feels like a regression we should address before merging, but otherwise this feels like a much better base to build off of.

packages/ui/src/components/Channel/Scroller.tsx Outdated Show resolved Hide resolved
@davidisaaclee
Copy link
Contributor Author

davidisaaclee commented Sep 5, 2024

In the before case, I didn't record jumping to the selection anchor from search page – it anchored correctly to the item, but only after scroll position jumped up and down a few times.

Current Testflight client 5817765
before-dagoth.mov
after-dagoth.mp4

There are still some white flashes at 5817765, which I tried to capture but could not – in any case, I think the flashes are much briefer.


starting at 6765d3f...

  • Main change was reducing how many posts we load on each page – we still load enough to fill a page+, and I increased the threshold before we start loading more posts to keep things smooth.
  • I removed a demo seed on the seeded RNG and didn't realize that not providing a seed made the RNG "unseeded", causing a different output on each run – seeded with empty string to pin.
  • Stabilized identities of the simple deps to listRenderItem
  • listRenderItem relies on each post's neighbors to draw dividers etc – to avoid resetting the callback every time posts changes, I derived postsWithNeighborsin a different useMemo. I don't know if this has an effect yet (depends on how much memoization FlatList does internally), but it sets us up for increased memoization.

Copy link
Member

@latter-bolden latter-bolden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes make sense to me. Likewise curious if you'd have feedback on the modified post fetch count @dnbrwstr.

Excellent work on the fixes! Was feeling really good on my dev build.

@davidisaaclee davidisaaclee merged commit 3a2d394 into develop Sep 5, 2024
1 check passed
@davidisaaclee davidisaaclee deleted the dil/anchor-scroll-lock-2 branch September 5, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants