[$1000] Long messages disappear when scrolling through chat history

[Julesssss]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Locate a chat with many long messages
2. Scroll up for a while
3. More and more large blank spaces begin to appear

Expected Result:

• Chats should be visible, no matter how much you scroll back

Actual Result:

• After scrolling back for a while, chats are not visible

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Yes, use mobile

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] Android / native
• [x] Android / Chrome
• [ ] iOS / native
• [x] iOS / Safari
• [x] MacOS / Chrome / Safari
• [x] MacOS / Desktop

Version Number: v1.3.7-0 Reproducible in staging?: Yes Reproducible in production?: Yes Issue reported by: me Slack conversation: link

https://user-images.githubusercontent.com/10736861/234807831-13681904-4fed-4c17-a724-2487c32c1839.mp4

https://user-images.githubusercontent.com/10736861/234807846-41d59a4f-3659-4d47-8bed-2df7ec66095d.mov

https://user-images.githubusercontent.com/10736861/234807899-fabfe654-6df6-471e-94bc-ec9392946950.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01dc99163ab590ca90
• Upwork Job ID: 1651521261601800192
• Last Price Increase: 2023-04-27

[MelvinBot]

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [x] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [x] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[jliexpensify]

Damn, I can repro this. It's very evident with Chronos, but takes more effort with actual DM's.

@Julesssss did you want someone external to take this, and you act as the Supervising Engineer?

[Julesssss]

Yeah sounds good. I've only ever noticed this when scrolling through Chronos so we'll have to get contributors to create lots of long messages

On Thu, Apr 27, 2023 at 10:25 AM Jason Li @.***> wrote:

Damn, I can repro this. It's very evident with Chronos, but takes more effort with actual DM's.

@Julesssss https://github.com/Julesssss did you want someone external to take this, and you act as the Supervising Engineer?

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/18067#issuecomment-1525248937, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACR5JXMKWINJWIG4NJZZNLTXDI3PTANCNFSM6AAAAAAXNQZWSM . You are receiving this because you were mentioned.Message ID: @.***>

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01dc99163ab590ca90

[MelvinBot]

Current assignee @jliexpensify is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

[MelvinBot]

Current assignee @Julesssss is eligible for the External assigner, not assigning anyone new.

[gitfullstacker]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Long messages disappear when scrolling through chat history

What is the root cause of that problem?

If the application is not optimized to handle a large number of messages or if it is limited by the amount of memory available on the device, it may struggle to display all the messages properly. This can result in the appearance of large blank spaces or even the disappearance of some messages altogether.

To prevent this issue, chat applications should be designed to efficiently handle large amounts of chat history data. They should also be optimized to make the most of the available resources on the device, such as memory and processing power. Additionally, the application should be regularly updated and maintained to fix any bugs or performance issues that may arise.

What changes do you think we should make in order to solve the problem?

Improve the loading and display of chat history: The chat application could be optimized to more efficiently load and display chat history, particularly for long messages. This could involve implementing a more efficient algorithm for loading and rendering messages or using pagination to limit the amount of data loaded at once.

Increase the amount of memory available: If the issue is related to insufficient memory on the device, the chat application could be optimized to make the most of the available memory. This could involve implementing caching mechanisms or using compression techniques to reduce the amount of memory needed to store messages.

Implement better error handling: The chat application could be updated to handle errors more gracefully, such as when messages fail to load or display properly. This could involve providing users with clear error messages or allowing them to retry loading messages if they fail to load.

Regularly update and maintain the application: The chat application should be regularly updated and maintained to address any bugs or performance issues that may arise. This could involve implementing new features, fixing bugs, or optimizing the application for new devices or operating systems.

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[MelvinBot]

📣 @chenxidev1129! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[gitfullstacker]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~015d50d1ac51670370

[MelvinBot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[Marlboro11526]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~01b551a83e11e05d6a

[MelvinBot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[sobitneupane]

@chenxidev1129 Thanks for your proposal.

Looks like you are a new contributor. Please go through the contributing guidelines if you have not already. We expect some concrete proposal. Your proposal is quite generic, please try to be specific. Also please make use of permalink to point where the issue exists in app whenever possible. You can go through some other issues to know about the expectations in a proposal.

[gitfullstacker]

Okay. Thanks. I will check again @sobitneupane

[chafikchaban]

Proposal

Please re-state the problem that we are trying to solve in this issue.

React Native Flatlist Performance issue. Long messages are randomly not rendered in a large messages list

What is the root cause of that problem?

InvertedFlatList used in ReportActionsList is not well configured:

• the initialNumToRender which we calculate to return the maximum number of smallest space taking items that could fit the available visible space ( which imo is a little random )

According to the React Native docs: initialnumtorender:

"Setting a low initialNumToRender may cause blank areas, especially if it's too small to cover the viewport on initial render."

• that is considering BaseInvertedFlatList's default configuration. more specifically maxtorenderperbatch which is set to 1. which was set in this commit by @marcaaron in order to improve overall performance

According to the React Native docs: maxToRenderPerBatch:

"Setting a bigger number means less visual blank areas when scrolling (increases the fill rate)."

What changes do you think we should make in order to solve the problem?

we need to tweak these values. the best way to do it is:

• setting initialnumtorender to the length of the array ( this does actually affects the list's initial rendering performance just a little bit, but is optimal for scrolling and guarantees a smooth experience with no jumping on scroll )

What alternative solutions did you explore? (Optional)

• using a higher value of maxToRenderPerBatch ( even though just setting it to 2 solves our issue, the jumping of next batch being rendered is actualy visible and annoying tbh)

[romulo114]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When scrolling chat history, messages sometimes disappear.

What is the root cause of that problem?

We're using FlatList component of react-native for displaying messages history. This component uses getItemLayout props to calculate the item's offset and height. In our project, We calculate the offset of the item using the following code.

https://github.com/Expensify/App/blob/a7863493a03970cdc0687621ec40dc75aea99477/src/components/InvertedFlatList/BaseInvertedFlatList.js#L57-L112

This algorithm assumes that the measureItemLayout is called in order, but this is not the case. Actually, measureItemLayout callback is called in different order like belows. This is the main reason.

What changes do you think we should make in order to solve the problem?

In order to solve this issue, we have to update the getItemLayout and measureItemLayout method so that get the offset of the item correctly. I was able to fix the issue successfully by assuming that measureItemLayout is called in any order.

Result

https://user-images.githubusercontent.com/117526965/235340567-9263b66f-2eb8-41e3-901e-602e12633d15.mp4

What alternative solutions did you explore? (Optional)

NA

[huzaifa-99]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We are trying to fix disappearing messages when scrolling in a long chat history

What is the root cause of that problem?

In this commit we made a tradeoff to have faster chat switching instead of having faster message rendering. The maxToRenderPerBatch={1} is not rendering as fast as we are scrolling.

What changes do you think we should make in order to solve the problem?

We need to render more items on scroll, just enough so the user doesn't see blank areas.

We are calculating the initialNumToRender here based on font (size/scale), minimum message height and available height of the message list.

We could use the same value for maxToRenderPerBatch as we do for initialNumToRender here

maxToRenderPerBatch={calculateInitialNumToRender()}

We can also use some value of updateCellsBatchingPeriod to have some delay in batch renders

This will still show some blank areas if scrolled really fast, we could increase the maxToRenderPerBatch but it will slow list rendering.

What alternative solutions did you explore? (Optional)

N/A

[Julesssss]

Thanks all! All are well explained and make sense.

@romulo114 I'm curious what your solution would look like?

[romulo114]

@Julesssss Please take a look.

https://github.com/Expensify/App/blob/a7863493a03970cdc0687621ec40dc75aea99477/src/components/InvertedFlatList/BaseInvertedFlatList.js#L93-L112

We can get the item's height correctly but the offset isn't correct. As you can see, we calculate the offset of the item based on previous offset, which is assuming that this callback is called in order. Actually this callback isn't called in order.

In order to solve this issue, I get the item's length(height) only in this callback(by removing the line 110, and get the offset of the item when getItemLayout is called.

In getItemLayout function, I get the offset by cumulating the heights previous items and put them in sizeMap when offset isn't available. Of course, offset is available, I get the offset from sizeMap. If height of an item isn't available yet, I use initialRowHeight prop of the component.

[bernhardoj]

Based on this comment from Pagination tracking https://github.com/Expensify/App/issues/12054#issuecomment-1518207853, getItemLayout could be removed.

[romulo114]

Based on this comment from Pagination tracking #12054 (comment), getItemLayout could be removed.

As you can see here, it is more performant to use when there is a lot of messages. Can you specify the clear reason that getItemLayout should be removed. IMO, removing getItemLayout isn't a solution.

[Julesssss]

Thanks @romulo114! @sobitneupane please review this proposal and let me know what you think.

[sobitneupane]

Overall the proposal looks good to me. But I was able to reproduce the issue with the proposed changes as well. @romulo114 Can you please share the code diff. I might have missed some changes.

[Julesssss]

If we like the proposal it's probably fine to not see the whole diff, but if you do have it written already please share.

[chafikchaban]

If we're going that route, i think the biggest challenge is to find a way to make sure that the measuring occurs first, otherwise we're just assuming and estimating item sizes anyway. which to my understanding is a big no-no to pass to the Flatlist's getItemLayout

also: the order in which measureItemLayout is called doesn't really matter since we store the measurements in the sizeMap accordingly to the measured item's index on the initial list.

Concern: order of execution between the Flatlist's getItemLayout and the item's onLayout

---

But also digging deeper, does it even make sense in our case to use getItemLayout ? According to the docs here :

"getItemLayout should only be used when all your list item components have the same height (or width, for a horizontal list)"

and here:

"getItemLayout is an optional optimisation that allows skipping the measurement of dynamic content if you know the size (height or width) of items ahead of time. getItemLayout is efficient if you have fixed size items_"

which is not the case here 🤔

@Julesssss @sobitneupane thoughts ?

[romulo114]

But also digging deeper, does it even make sense in our case to use getItemLayout ? According to the docs here :

"getItemLayout should only be used when all your list item components have the same height (or width, for a horizontal list)"

and here:

"getItemLayout is an optional optimisation that allows skipping the measurement of dynamic content if you know the size (height or width) of items ahead of time. getItemLayout is efficient if you have fixed size items_"

@chafikchaban

They are saying getItemLayout can be used to optimize the performance especially when items have the same height. This doesn't mean we should not use this when heights are different. For example, getItemLayout would work great when we can get the items' heights easily.

also: the order in which measureItemLayout is called doesn't really matter since we store the measurements in the sizeMap accordingly to the measured item's index on the initial list.

If you took a look at the source, you can notice that the order is important in calculating offsets.

[jliexpensify]

Just giving this a quick bump @Julesssss and @sobitneupane - I tested this again with v1.3.9-19 and it's still happening, albeit not as bad as it was before. Would any of the above proposals work?

[romulo114]

@jliexpensify @Julesssss @sobitneupane Here are my fixes. They work perfect.