[$250] [Search v1] Search page scroll is not smooth

[luacmartins]

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

---

Version Number: Reproducible in staging?: Reproducible in production?: If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Slack conversation:

Action Performed:

1. Open the search page, Profile > Troubleshoot > New Search Page
2. Scroll through the list of results
3. Notice that the items flicker and on mobile the list jumps around

Expected Result:

Scroll should be smooth

Actual Result:

Scroll is not smooth

Workaround:

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

Platforms:

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

• [ ] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/22219519/d0a06780-395b-4bfb-9dbd-6242be6db2d8

https://github.com/Expensify/App/assets/22219519/89b5fcfd-293b-4373-8687-add4cf8ed808

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fb9b7cf710602d40
• Upwork Job ID: 1786403109998579712
• Last Price Increase: 2024-05-03
• Automatic offers:
• eh2077 | Reviewer | 0
• charles-liang | Contributor | 0

Issue Owner

Current Issue Owner: @eh2077

[melvin-bot[bot]]

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 @nharsh94! 📣 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. Make sure you've read and understood the contributing guidelines.
2. 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.
3. 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.
4. 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>

[eh2077]

@nharsh94 Thanks for your proposal. I'm not following your root cause analysis. Can you elaborate on why the scroll is not smooth?

[eh2077]

@nharsh94 Thanks for the update, but your root cause analysis still seems vague to me.

[eh2077]

Still looking for better proposals

[luacmartins]

Still looking for proposals

[charles-liang]

Proposal

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

[Search v1] Search page scroll is not smooth #41581

What is the root cause of that problem?

1. Using Google's performance analysis tools, it was found that the most time-consuming tasks were creating and removing components.

   

2. By removing all elements of the item and replacing them with a single text element, the delay issue was improved.

3. Adding multiple texts increased the scroll delay issue, but it can basically be determined that it's a problem with rendering child elements.

4. Analyzed using React Dev Tools, as shown in the figure, the time it takes for the TransactionListItem to render child elements is only half the time of the parent element. It is speculated that the problem lies with the parent element itself.

5. All functions returning JSX were removed from TransactionListItem.
6. Analyzed again using React Dev Tools, the previously observed delay has disappeared.

Summary: The root cause is that every time you scroll, it removes the TransactionListItem that leaves the visible area and renders new TransactionListItems. Control by windowSize (Default 5 in BaseSelectionList)(The windowSize default 5 means that, in addition to the visible range in the middle, 2 time of elements of the visible height ((windowsize - 1) / 2) = 2) are kept above and below. When the scrolling speed exceeds the rendering speed, elements are recycled instead of being cached. And updateCellsBatchingPeriod (default 50ms) will render according to your scrolling speed.

Rendering the TransactionListItem is too expensive. the Reason is:

1. because each instance of TransactionListItem creates new anonymous functions like DateCell, UserCell, and TotalCell.
2. the OnSelectRow function passed from Search.

Finally, The render requests are blocked the render queue and event handling.

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

1. Setting windowSize and updateCellsBatchingPeriod to the appropriate value in SelectionList in Search.tsx.

https://github.com/charles-liang/ExpensifyApp/blob/charles/src/components/Search.tsx#L50-L52

2. Move all similar to xxxx from inside TransactionListItem to above TransactionListItem. and applying memo along with keyforlist to decide whether to re-render, avoid re-instantiating these functions.

from https://github.com/Expensify/App/blob/e0b6a06d2f34f35cda96ccc4d05252ede111c732/src/components/SelectionList/TransactionListItem.tsx#L69-L74 to https://github.com/charles-liang/ExpensifyApp/blob/charles/src/components/SelectionList/TransactionListItem.tsx#L71-L81 https://github.com/charles-liang/ExpensifyApp/blob/charles/src/components/SelectionList/TransactionListItem.tsx#L162

3. wrapped with useCallback to prevent multiple copy OnSelectRow in Search

https://github.com/charles-liang/ExpensifyApp/blob/charles/src/components/Search.tsx#L50-L52

What alternative solutions did you explore? (Optional)

N/A

[eh2077]

@charles-liang Thanks for your proposal. It's good to see you tried to pinpoint the root cause by profiling the page.

2. By removing all elements of the item and replacing them with a single text element, the delay issue was improved.

Do you mean the TransactionListItem here?

5. All functions returning JSX were removed from TransactionListItem.
6. Analyzed again using React Dev Tools, the previously observed delay has disappeared.

Can you elaborate on why those functions cause the delay when scrolling?

[charles-liang]

yes. i mean TransactionListItem. The RCA is that the cost of re-rendering the item is too high, not due to a specific function. Instead, each TransactionListItem should share a component, rather than each TransactionListItem retuen a new component using function. Therefore, all returned JSX should be moved to the external scope. For specifics, please refer to:

contributingGuides/PERFORMANCE.md

Use PureComponent, React.memo(), and shouldComponentUpdate() to prevent re-rendering expensive components.

[eh2077]

@charles-liang Thanks for the update. Can you detail your improvement plan in the third/solution part of your proposal?

[charles-liang]

Do you mean this: change react 17 to react 18 using createroot instead of AppRegistry.runApplication in src/setup/platformSetup/index.website.ts to turn on concurrent rendering

Maybe like this.

import React from 'react';
import { createRoot } from 'react-dom/client';
import App from './App';
import { AppRegistry } from 'react-native';
import Config from './config';
import { checkForUpdates } from './updateUtils'; 
import { startCurrentDateUpdater } from './DateUtils';

const container = document.getElementById('root');
if (container) {
    const root = createRoot(container);

    AppRegistry.registerComponent(Config.APP_NAME, () => App);
    AppRegistry.runApplication(Config.APP_NAME, {
        initialProps: {},
        rootTag: container,
    });

    if (Config.IS_IN_PRODUCTION) {
        checkForUpdates(webUpdater());
    }

    startCurrentDateUpdater();
}

But I've tested it and the effect is not very obvious.

[eh2077]

@charles-liang I mean the main solution

removed All functions returning JSX from TransactionListItem.

This part can be more detail.

[charles-liang]

For example remove below from TransactionListItem

const dateCell = (
        <TextWithTooltip
            shouldShowTooltip={showTooltip}
            text={date}
            style={[styles.label, styles.pre, styles.justifyContentCenter, isLargeScreenWidth ? undefined : [styles.textMicro, styles.textSupporting]]}
        />
    );

Add this before TransactionListItem in the same file

const dateCell = (showTooltip, date, styles, isLargeScreenWidth)=>(
        <TextWithTooltip
            shouldShowTooltip={showTooltip}
            text={date}
            style={[styles.label, styles.pre, styles.justifyContentCenter, isLargeScreenWidth ? undefined : [styles.textMicro, styles.textSupporting]]}
        />
    );

[eh2077]

Can you add them to your proposal? Also it'll be very helpful if you can provide a branch to test.

[charles-liang]

ok, I update a proposal after taking a time to make a branch for your test.

[charles-liang]

proposal

updated

@eh2077 i add a new branch with the exmaple code for your test. thanks a lot!

https://github.com/charles-liang/ExpensifyApp/blob/charles/src/components/SelectionList/Search.tsx https://github.com/charles-liang/ExpensifyApp/blob/charles/src/components/SelectionList/TransactionListItem.tsx

I have decoupled the component from TransactionListItem and memo them. This is just a draft for your test and needs further revisions to comply with the code of conduct.

[eh2077]

@charles-liang Can you add a summary about the root cause of the issue and update your proposal? And please elaborate on what changes are needed in the solution part based on https://github.com/Expensify/App/issues/41581#issuecomment-2098206239 and https://github.com/Expensify/App/issues/41581#issuecomment-2098277731. Note please explain them in plain english instead of a test branch.

[charles-liang]

proposal

updated

[eh2077]

@charles-liang Thanks for the udpate.

The root cause is that every time you scroll, it removes the TransactionListItem that leaves the visible area and renders new TransactionListItems.

This seems like the key factor causing the scrolling issue. Can you elaborate on why TransactionListItems are not cached?

[charles-liang]

@eh2077

because each instance of TransactionListItem creates new anonymous functions like DateCell, UserCell, and TotalCell.

As mentioned above, every time TransactionListItems is called, the anonymous functions inside it, such as const DateCell = () => {}, are newly generated. This makes the program treat TransactionListItems as new each time. Using memo along with a shallow comparison of props allows it to be cached.

[eh2077]

@charles-liang Thanks for the udpate.

The root cause is that every time you scroll, it removes the TransactionListItem that leaves the visible area and renders new TransactionListItems.

This seems like the key factor causing the scrolling issue. Can you elaborate on why TransactionListItems are not cached?

You didn't answer the question - Can you explain why the list of TransactionListItems are not cached when scrolling?

[charles-liang]

Alright, I misunderstood your question. The reason is that the default configuration for BaseSelectionList has a windowSize = 5. The function getItemLayout returns the height of each cell, and then it determines how many elements can fit on each screen based on the height of the elements.

The windowSize default 5 means that, in addition to the visible range in the middle, 2 time of elements of the visible height ((windowsize - 1) / 2) = 2 are kept above and below. When the scrolling speed exceeds the rendering speed, elements are recycled instead of being cached.

Additionally, updateCellsBatchingPeriod (default 50ms) will render according to your scrolling speed. Finally, The rendering requests are blocked in the rendering queue.(cannot be cancel)

[eh2077]

@charles-liang Can you update your proposal based on the new findings above?

[charles-liang]

proposal

updated

[eh2077]

@charles-liang Thanks for the update.

Setting windowSize and updateCellsBatchingPeriod to the appropriate value in SelectionList in Search.tsx.

Can we get significant improvement in both the profiling results and the smoothness of scrolling after tuning these parameters?

[charles-liang]

@eh2077

Yes, I have tested it, and changing these two values does indeed improve the situation when item created and cached. such as setting windowSize to 111 (large enough to retain 500 items) and updateCellsBatchingPeriod to 200ms(The updateCellsBatchingPeriod cannot be set too high, otherwise it won't load when the user is not scrolling.).

However, there is still a significant lag and white screen when scrolling to an unrendered area for the first time, due to the expensive rendering issue. The data I profiled earlier was from the initial load. Modifying these two values changes the performance of this part.

[eh2077]

@charles-liang Thanks for your patience during the discussion.

---

I think the root cause of this unsmooth scrolling issue is uncovered

• The current settings of BaseSelectionList.tsx can't efficiently render TransactionListItems when scrolling, see comment and comment
• It's relatively expensive to render TransactionListItems

---

It's great that @charles-liang followed our best practice React Performance Tips to profile the performance issue and adopt tips, like using PureComponent and React.memo(), to optimise it, though they seem new to this project and this is likely to be their first contribution. We can also refer to Optimizing Flatlist Configuration

---

That said, I think we can go with @charles-liang 's proposal. Coding details can be discussed in PR.

🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot[bot]]

📣 @eh2077 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @charles-liang 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[luacmartins]

This issue is yours @charles-liang. Let me know when we can expect a PR.

[eh2077]

@charles-liang Kindly comment an eta for the PR, so we can keep the team updated on the progress. Thanks

[charles-liang]

@eh2077

I am currently testing across all platforms and writing the PR documentation. I can submit the PR by Tuesday at the latest.

Including fixing the regression issues, I think I can complete it this week.

[eh2077]

@charles-liang Thank you. Feel free to let us know if you have any questions.

[eh2077]

Still working on the PR