[$16000] Pagination seems broken for chats (please see #12054)

[mvtglobally]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Navigate to chat with Chronos
2. Scroll through multiple messages and check pagination

Expected Result:

Messages should load without any issue

Actual Result:

Messages get stuck and pagination doesn't load properly

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.1.39-1
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

New.Expensify.-.Google.Chrome.2022-02-09.22-29-03.mp4

Expensify/Expensify Issue URL:
Issue reported by: @marcaaron @kidroca
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1644435691436529
https://expensify.slack.com/archives/C01GTK53T8Q/p1641586928143800?thread_ts=1641315371.490700&cid=C01GTK53T8Q

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @neil-marcellini (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[neil-marcellini]

I was able to reproduce this on Web and Desktop, particularly if I scrolled up quickly. As shown in the video, this occurs on any chat, not just with Chronos.

[MelvinBot]

Triggered auto assignment to @adelekennedy (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[MelvinBot]

Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MelvinBot]

Still overdue 6 days?! Let's take care of this!

[MelvinBot]

8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[adelekennedy]

jeez - just getting to this
Internal
External

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

[MelvinBot]

Triggered auto assignment to @AndrewGable (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[adelekennedy]

From this slack convo this may be related to the flatlist fix (purely for context)

[MelvinBot]

Triggered auto assignment to @bfitzexpensify (Internal) because issue was reported by a contributor who needs to be compensated if this issue is fixed

[roryabraham]

I see the links point to react-native. I just want to clarify my proposal targets issues on react-native-web

The VirtualizedList component in react-native-web is just copied over from react-native, with only a few specific changes. Please open all your issues and pull requests against react-native, not react-native-web

[LucioChavezFuentes]

Hey guys! I proposed the changes to the react-native-web upstream repo. I will create an issue on Expensify's App repo for each problem.

[LucioChavezFuentes]

I see the links point to react-native. I just want to clarify my proposal targets issues on react-native-web

The VirtualizedList component in react-native-web is just copied over from react-native, with only a few specific changes. Please open all your issues and pull requests against react-native, not react-native-web

@roryabraham Oh, I didn't know that. Do you know exactly where the VirtualizedList file is in react-native?

[roryabraham]

Yes, it's right here

[LucioChavezFuentes]

@roryabraham Also I'm having trouble finding UIManager/index.js and useElementLayout/index.js implementations in react-native. Do you have an idea where they could be?

[necolas]

I'm having trouble finding UIManager/index.js and useElementLayout/index.js implementations in react-native

react-native doesn't have those implementations. The UIManager module is not supported in the Fabric architecture. And useElementLayout is an implementation detail of react-native-web that backs the onLayout prop on react-native components.

As I mentioned on the react-native-web PR, you shouldn't need to touch those files for web anyway. The question I have is about how much of these code changes should go into react-native instead of react-native-web. And whether the y value of onLayout is different between web and native for inverted lists, as offsetTop is not currently a concept in RN layouts either.

[kidroca]

Something else to have in mind is this (big) refactor of the VirtualizedList that was recently merged in react-native: facebook/react-native@9715993

the large part of the refactor is in the data structure used to represent state
the implementations not only unlocks the enhancements of the new list, but unblocks future changes to VirtualizedList.

Per the above summary it seems any enhancements would be better off to be applied on the latest code

I think ATM react-native-web is using the implementation from 6-7 months ago: necolas/react-native-web@edec979

[necolas]

Yeah I think we should update VirtualizedList and FlatList in the 0.19-dev branch, removing any web-specific patches that might be in those modules in react-native-web, and then I can cut a canary that can be tested here and elsewhere. You might want to migrate your own patches, which will probably be a pain, but that's the price of forking forks.

We really need to get to a point where the upstream VirtualizedList works well enough on web out-of-the-box, and can be installed in react-native-web as a specific package. That would make all our lives easier, and I think there is a good chance of that happening in the context of "How can we make React Native better" (which you should comment on about this topic if you haven't already) react-native-community/discussions-and-proposals#528

[LucioChavezFuentes]

Update: right now I'm testing if the problems and solutions stated in my proposal still apply to the refactor of VirtualizedList.

[LucioChavezFuentes]

@bfitzexpensify @roryabraham Some of my solutions do not apply to the new VirtualizedList refactor in the react-native repo. It will take me time to fully understand the new functionality and come up with a new proposal. I believe the issue will persist in Web using the new refactor but I can't be sure as I can’t test it because these changes are not released in react-native-web yet.
Should I wait for upstream changes to be released on react-native-web or make PR directly to Expensify's forks?

[LucioChavezFuentes]

I believe the issue will persist in Web using the new refactor but I can't be sure as I can’t test it because these changes are not released in react-native-web yet.
Should I wait for upstream changes to be released on react-native-web or make PR directly to Expensify's forks?

I can progress if I create my own branch in react-native-web with react-native latest changes (thanks for the tip necolas).

[LucioChavezFuentes]

Update: I am still applying the react-native's VirtualizedList new changes to react-native-web, there are many files to copy and paths to resolve.

[trjExpensify]

Looks like the discussion on holding on #7925 originated here in relation to bi-directional scrolling. That issue is in-turn held on Comment Linking.

[mallenexpensify]

Apologies @trjExpensify, I hid the comment. I swear a checked the issue numbers multiple times (maybe I had stacked windows/browsers?)

[LucioChavezFuentes]

Update: I managed to run the updated VirtualizedList on Web and test it with my proposal. I confirm the issue still exists with the new refactor and my proposal (with minor changes) still applies to fix the issue. It works great! I want to make tests on native platforms just to make sure I don't break something there. Also, I am working to reproduce this issue with minimal code in a sandbox so I can present it as an issue in react-native code.

[roryabraham]

Thanks for the update @LucioChavezFuentes. Please keep us in the loop and let us know how best we can support you.

[LucioChavezFuentes]

Update: I reproduced this issue in a sandbox here

While doing the sandbox I realized that problems 2, 3, and 4 are not caused by Inverted VirtualizedList but by VirtualizedList's struggle to render expensive items, which affects both Inverted and Normal VirtualizedList. Inverted VirtualizedList only causes the first problem.

Also, I found edge cases that made me update the solution for Problem 4. I made tests on Android, it works fine. I still haven't tested my solution in iOS as I have trouble running the rn-tester on this platform.

[tjferriss]

Following instructions for the weekly update chore:

@roryabraham this is one of the oldest issues in the /App repo. To help us clear out the large backlog of bugs, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is
• For any that can't, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #bug-zero

I recognize this issue is on hold, but given it's age do you think we should move it to internal anyways?

[JmillsExpensify]

@tjferriss I think we're on a good path in this issue, despite it's age. There's a lot of tricky stuff going on, including the HOLD and related issue in the title.

[LucioChavezFuentes]

Update: After further testing in native platforms and some more insights from the react-native maintainers' feedback, this is where I am:

My proposal is applicable only on Web. I can not reproduce the problems stated in my proposal in Native platforms except for Problem 2 but this problem has a low impact.

My proposal fixes the VirtualizedList in Web in a superficial way because I am still failing to understand the root issue. There are new discoveries that question my understanding of the problems:

• The e.target.offsetTop solution (fix for Problem 1) causes Problem 4, for Normal or Inverted VirtualizedList.

• I don't know why using e.target.offsetTop causes Problem 4. I thought the canceled Low Pri updates make onLayout returns offsets equal to zero for items the update pretended to mount, but the truth is, canceled Low Pri never mount elements therefore the onLayout is never executed on the canceled items.

• My ‘offsetTop’ solution for problem 1 might not be merged because, as far I can see, inside react-native-web, ‘offsetTop’ has been used previously and later replaced to fix the following issues: 1037 1151.

• I discovered the Web's onLayout does not behave the same as its Native counterpart: Web's onLayout return different values when there are scale transformations while Native's onLayout ignore scale transformations. VirtualizedList is build based on Native's onLayout behaviour. Web's onLayout 'y' does not cause Problem 4 but will cause Problem 1 and a weird bug: on scroll very fast (dragging bar scroll bar up and down) some items get negative offset measure.

• The inverted scroll wheel patch inside react-native-web causes Problem 3.

• Problem 2 still needs further investigation: need to discover why we get negative values in the first place.

These discoveries just make more questions:

Why does onLayout's e.target.offsetTop return zero for offset on an element that is supposed to be mounted and definitely have an offset different to zero?

If offsetTop is unreliable, Is there a solution for the Web’s onLayout to behave the same as its native counterpart without using offsetTop (or at least in a way that doesn’t cause issues)?

Why does the inverted scroll wheel patch cause Problem 3? Does it cause race conditions on mounting new items in VirtualizedList?

Why using Web's onLayout 'y' returns negative offsets on scroll fast?

Do the answers to these questions are inside of react-native or react-native-web, or both?

I am curious to answer the new questions and find out the true root issue but it will take time. If there is urgency on fixing issue #7860 we can merge my solutions in react-native-web fork. I don't think uploading the solutions to react-native fork will help, except for Problem 2.

Something about issue #2545

I don't think my solutions will help to fix the issue #2545, however, let me share something that could help to fix this issue:

Not long ago I reported a bug where the jpg images get values equal to zero for width and height. This bug caused a little movement among items on lists because the initial state of the images rendered a tiny element and the next moment the image shows.
The bug is fixed but only for images uploaded after fix. I notice that jpg images uploaded before the bug fix still receive zero for width and height.

Perhaps the issue exists only in lists with jpg images uploaded before the bug fix.

[JmillsExpensify]

A couple of housekeeping items as I circle back on this issue:

• Shouldn't @LucioChavezFuentes be assigned to this issue? His proposal has been accepted as far as I can see. Further if that's the case, we should remove the Help Wanted label.
• Second, @AndrewGable any thoughts on the questions and comments immediately above?
• Finally, because this is a larger project, I'm filing this outside of our more pressing bug initiative. We can revisit the right place internally down the line.

[bfitzexpensify]

We opened #12054 as a kind of 'reset' since this issue had grown long and a bit unwieldy. @LucioChavezFuentes is assigned over there, but it does seem like there's some confusion on which issue is the better one to communicate on. I think we can probably close this in favour of the other to eliminate that confusion and keep progress and questions/answers in the one place.

[JmillsExpensify]

Ah, thanks. I'm not passionate, so up to you and @roryabraham.

[bfitzexpensify]

Cool, I'll go ahead and close then. Anyone coming here, please head to #12054 instead. Title also updated.