Implement bidirectional scrolling in ecash

[jboniface]

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

---

Problem

We're trying to enable comment linking in Cash, and while developing the solution, we've hit a snag : in short, we're unsure how to load the specific report comment. At the moment, because of pagination when the fetchReportActions we’re just fetching 50 results. So if the report had 300 comments and we want the comment link to go to comment sequence number 100 it gets tricky because we have two options.

1. Have fetch report actions load actions 50 to 100 than 250 to 300. Or,
2. Load 100 to 300 actions.

If we go with option 2 it would be extremely slow since we load bottom to top so it would be a bad user experience to load the desired comment action last.

Option 1 sounds like the better user experience but then our current FlatList only loads more comments if you scroll up. This means the user can scroll up to load report comments between 1-50 but they will never be able to scroll down to load report comments between 100 - 150.

Solution ?

Create a PR to build bidirectional scrolling so that we can load comments onStart or onEnd and then we continue with comment linking. This way even if the comment is pretty old the user will be able to scroll up and down to load additional comments using pagination.

Discussion

As per above, we're exploring whether we want to build our own FlatList component to support bidirectional scrolling, but we need to plot out the actual approach for implementation to determine if it would be feasible.

Ultimately, the question is, does it make sense to build the features we want into FlatList, or should we build our own component?

cc @chiragsalian

I've posted a job for the research side of this discussion, you can view it here https://www.upwork.com/jobs/~01b0e6a74cd4c83a19

[MelvinBot]

Triggered auto assignment to @michaelhaxhiu (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[roryabraham]

cc @marcaaron I know you showed me a gist or draft PR a while back with an outline of an implementation. Do you still have that?

[kidroca]

Do you know the total size of chat items before or when the chat loads?
E.g. do you know you have a million messages in a chat or it's unknown until you fetch all of them

but they will never be able to scroll down to load report comments between 100 - 150.

What's the problem with starting in the middle and then append/prepend to the FlatList - you can (fetch and) insert more items to the bottom when you start to reach the bottom or insert more items to the top when you start to reach the top

[roryabraham]

Do you know the total size of chat items before or when the chat loads?

I'm 95% sure we could easily provide this information from the API.

[marcaaron]

Yes, we already know since the entire reportActionList gets downloaded when the app inits 🙃
(but the pagination and storage of messages happen in Report_GetHistory)

[marcaaron]

What's the problem with starting in the middle and then append/prepend to the FlatList

append works fine since there is an onEndReached method and we already use it for the one direction

prepend not so much for a couple of reasons:

1. There is no onStartReached - make could create our own by wrapping FlatList and looking at the scroll metrics to detect when the ScrollView has reached the "top" (or bottom rather in the inverted style)
2. When prepending items the content shifts. We might be able to solve this if we knew the total height of all items that we are prepending and could offset the scroll position. Perhaps there is a simpler way or something I'm missing.

[marcaaron]

This is something new I just noticed in the past few months that looks interesting

https://github.com/GetStream/react-native-bidirectional-infinite-scroll

Have no idea if it supports web They are working on web support - but I think we are basically trying to achieve the same thing as this package.

[kidroca]

This library seems to support everything you need at the moment

• onStartReached
• onEndReached
• virtualization
• maintain correct scroll position
• potentially fix the a11y and content being copied in reverse if you don't use the inverted version

It also supports the same virtualization props as the FlatList

This package is a wrapper around react-native's FlatList. So it accepts all the props from FlatList

Is it possible to create a new ChatList component based on that library and use it for Android and iOS, while desktop and web continue to use the existing FlatList (ReportActionsView) implementation.
When web support is added - the old FlatList can be dropped altogether

The point is if you try to create your own solution you'll basically create the same thing and have the same problems e.g. make it work for web

[marcaaron]

I can't speak for everyone, but I'm not sure if we would commit to using a library on the mobile side with only the hope that the web side would work out for us eventually (maybe if we can also test the web PR in progress and deem that it's heading in the right direction).

In any case, the first step would be to evaluate if such a library exists and can actually work for us (cross platform or not).

The point is if you try to create your own solution you'll basically create the same thing and have the same problems e.g. make it work for web

An alternative way to frame this is that if we create our own solution, we might have the same problems, but nothing can stop us from solving them.

[MelvinBot]

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

[MelvinBot]

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

[kidroca]

What are the limits for this ticket. Can we use a library?

@marcaaron The library that you found seems to be the real deal
If it doesn't work for web, we could tailor just the web implementation ourselves - it's steal beats trying to cover all platforms ourselves. And in fact that's exactly what's the PR you've shared does - for web they are were working on a standalone web solution that is not using a FlatList but exposes the same interface, and is virtualized, but using react-virtuoso for virtualization

Create a PR to build bidirectional scrolling...

I think this is a leftover from when the ticket was internal, as an external contributor, I can spend a lot of time to do a PR only to be rejected.

How should this ticket be handled?
It's hard making proposal without starting to work on a bi-direction list, but it's also very time consuming to do so

[kidroca]

Using the library and making a web specific implementation (that we can contribute to that library) can be a lot less coding:
see their attempt: https://github.com/GetStream/react-native-bidirectional-infinite-scroll/blob/a782467396b3143b43ff2ca15b4ada3f46a7da20/src/BidirectionalFlatList.tsx ~300 lines of code

This is because we have much more flexibility coding for web

[roryabraham]

@kidroca I think what you've suggested sounds pretty reasonable. I would love to see a solution that forks https://github.com/GetStream/react-native-bidirectional-infinite-scroll and implements a shim to make it compatible with react-native-web. We can use that fork ourselves and try to get it merged into the parent repo. Alternatively, we can just create the shim directly in Expensify.cash but I would prefer the first option.

Given your history of excellent work, I would feel comfortable hiring you for that task. Then if you produce a proof-of-concept and we decide that we'd still rather implement a bidirectional paginated FlatList from scratch, you'd still be paid for that work. It would still be valuable research to push this issue forward.

cc @jboniface @marcaaron @chiragsalian – sound reasonable to you?

[quinthar]

Yes, I love the idea of forking and contributing a RN4W version of https://github.com/GetStream/react-native-bidirectional-infinite-scroll. Great!

[marcaaron]

As for proposing a web implementation it might be worth inquiring on the status of this PR -> GetStream/react-native-bidirectional-infinite-scroll#16

Maybe we can build on that work, propose an alternative, or help get it prioritized.

But verifying if this project suits our needs sounds like a good first step.

[kidroca]

Sounds great!
They are indeed working on another PR for web, so that's good news as well.

@roryabraham
I guess you'd want to see how this lib will integrate with E.cash so you can decide is it worth it to continue with it. Did I get that right?

I can make a POC implementation where we can see how it goes for native and make further plans if it's worth it

Setting:

• a chat with 300+ messages
• initially load the last 50 messages
• then jump to message 100th -> load 25-30 messages above and below
• then scrolling up or down should load and append data correctly for both directions and not cause any flashing or jerk

[roryabraham]

@kidroca Sounds great as a first step. So let's do that first and then discuss.

However, I think it's also in-scope for this job to test the web beta (i.e: point E.cash at the latest commit on this PR)? If it doesn't work and we can create a reproducible test case, then that's valuable information to provide to the PR author.

Let me know if you have any other questions.

[kidroca]

However, I think it's also in-scope for this job to test the web beta (i.e: point E.cash at the latest commit on this PR)? If it doesn't work and we can create a reproducible test case, then that's valuable information to provide to the PR author.

Good idea
Can we just make the scope of the current task clear.
I'll will use the latest code (from the PR) to make a POC, it should work for web, but "If it doesn't work and we can create a reproducible test case" - I'm not sure what that involves and who's expected to do it, but it sounds like extra work

• is it a part of this ticket?
• create a reproducible test case -> does this mean to crate a react-native project that demonstrates the problem? (Expo snack might do the trick as well: https://snack.expo.io/)

[jboniface]

@kidroca tbh, I was going to raise the value on this job right before you accepted it, and it does sound like more work than i originally proposed, so I'll just throw a bonus on to double the payout for you when it's done. Just link back to this comment if I forget somehow and we'll figure it out.

[roryabraham]

Thanks for your continued work and interest @azimgd. Here's are my thoughts:

Working on this issue has been a humbling experience for me – I've tried many different approaches over several months and unfortunately have not been able to successfully implement the feature. During that time, I have successfully produced several POC's that, like the one you're showing us, work in a sandboxed environment using mock data and simple square components. Each time, I have discovered some unforeseen bug or bugs that surfaced at a later step that invalidated my POC to some degree.

It's totally possible that the solution you've been working on solves all the problems we've faced in implementing bidirectional scrolling in this repo and doesn't introduce other unseen ones, but there's no way I can reliably validate that without implementing your solution in our product. I for one still have greater confidence that fixing this bug and completing the onStartReached support directly in React Native's FlatList is the best path forward we have.

If you believe that your react-virtual approach can solve this issue on all platforms without introducing other bugs, that is great. But if you want to pursue that route, you'll need to provide not just a proof-of-concept, but a full-fledged implementation of your approach in our app. However, I unfortunately have not seen enough to make me confident that your POC will be more successful than my several others (though I would love to be proven wrong). So given that, I am not currently comfortable assigning this issue to you or agreeing on a price for that work at this time. I will say that, for a full-fledged solution we would be happy to pay at least the sticker price for this issue, provided that we have not hired someone else for that issue at the time the solution is delivered.

So if you want to keep working on that, I encourage you to do so, as long as you understand that such work is not officially sanctioned at this time, so there is some risk that it won't be used or you won't be paid for it. Hopefully that all makes sense and transparently expresses our position here.

[mallenexpensify]

Guess we'll keep this one open for now, the main issue we're addressing now is #7925

[mallenexpensify]

Happy anniversary!

#7925 is moving along (but also on hold...)

[mallenexpensify]

Rory "Made some great progress on Android implementation last week, posted update here"
#7925 (comment)

[iwiznia]

Making a note here that whenever this is done, there's this other initial consideration we punted.

[roryabraham]

Meeting with Margelo today, we are making slow-and-steady progress. Almost all the pieces are in place now

[mallenexpensify]

@roryabraham , can you provide a quick update when ya can? (Mostly cuz i'm curious how close we are)

[roryabraham]

Bidirectional scrolling features are live in NewDot production. Now we are waiting for @chiragsalian to re-implement comment linking using the new bidirectional scrolling features so we can battle-test them. Last I heard, he was prioritizing finishing API refactors first.

[chiragsalian]

Yup its what rory said. I went ahead and closed my previous PR. Mostly because there was a lot that wasn't ideal about it and it wasn't in any super rush like it was when we originally created the issue so it would be nice to do it better this round.

I'm currently focusing on API refactors but will work on this right after. This time i'll focus on doing very small parts at a time so that reviewing and testing becomes a lot easier.

Additionally i wanted to ask. Do we feel like a design doc is needed to discuss technical implementation using bidirectional scrolling to get more eyes on this or for each task should i just post a plan of action to have it approved before implementing or just implement each task and discuss improvements during review? Any preferences?

[mallenexpensify]

Missed your comment @chiragsalian , thanks for the ping @michaelhaxhiu

Chirag, any update since your last one?
Looks like the GH for comment linking got bumped back to Weekly https://github.com/Expensify/Expensify/issues/147480

Do we feel like a design doc is needed to discuss technical implementation using bidirectional scrolling to get more eyes

Has much changed since the comment linking portion in the comment actions design doc was created? It's been a bit. Maybe a mini doc is in order? Maybe a post in #expensify-open-source with CMEs tagged if you/we are unsure?
https://docs.google.com/document/d/1vfTkJEOqCu0bVKP9ATKhMqAfLslYMuoL6W80lLCB5U4/edit#heading=h.my7t93ltzxp1

[chiragsalian]

Has much changed since the comment linking portion in the comment actions design doc was created?

Well the design doc was pretty light on the technical details for comment linking. So from just that high-level idea, it hasn't changed much. But comment linking has a lot of nuances that I feel should be discussed either in more detail in a doc or in a GH plan of action comment.

Sure okay, I can ask CMEs. I'm not sure why to post in #expensify-open-source just yet though since atm the task is internal. If CMEs decide its external then I think it makes more sense to post in #expensify-open-source.

[mallenexpensify]

I think I maybe accidentally said #expensify-open-source when I meant #engineering.

If there hasn't been a detailed section written to specifically address comment linking, it sounds like it's be useful. Maybe it'll help with splitting up internal/external issues too

[JmillsExpensify]

For clarity bidirectional scrolling already exists in /App, and more recently, we've confirmed it'll be compatible with the Fabric upgrade. Right? Can we rename this issue to focus on comment linking as a result?

[mallenexpensify]

My understanding is this issue is still open since we can't prove everything works with bidirectional scrolling yet, even through it should. This is the main tracking issue for Comment Linking, we could close this then move discussion there, if/when needed. ¯_(ツ)_/¯

[JmillsExpensify]

@roryabraham @chiragsalian I stumbled upon this issue again and I still feel we should close it since nothing is actually happening here. As noted, we already have an issue for comment linking in https://github.com/Expensify/Expensify/issues/147480. Re-open if you disagree.