[HOLD for payment 2024-06-06] [$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

327270767-f00e25f1-47a9-4095-8871-158eefed349b.mov

327281632-1fa5a858-115a-4e45-948b-c8726e76a67e.mp4

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: @muttmuure

[melvin-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]

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

[melvin-bot]

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

[melvin-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

App/src/components/SelectionList/TransactionListItem.tsx

Lines 69 to 74 in e0b6a06

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

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]

Still working on the PR

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.77-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: [Search v1] Search page scroll is not smooth #42170

If no regressions arise, payment will be issued on 2024-06-06. 🎊

For reference, here are some details about the assignees on this issue:

• @charles-liang requires payment automatic offer (Contributor)
• @eh2077 requires payment automatic offer (Reviewer)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@eh2077] The PR that introduced the bug has been identified. Link to the PR:
• [@eh2077] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@eh2077] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@eh2077] Determine if we should create a regression test for this bug.
• [@eh2077] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@muttmuure] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

@charles-liang, @luacmartins, @muttmuure, @eh2077 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[eh2077]

Checklist

• [@eh2077] The PR that introduced the bug has been identified. Link to the PR: This is a performance issue not a bug, so there isn't a specific PR responsible for this issue
• [@eh2077] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
• [@eh2077] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A, we already have our best practices documented here React Performance Tips
• [@eh2077] Determine if we should create a regression test for this bug. No

[melvin-bot]

@charles-liang, @luacmartins, @muttmuure, @eh2077 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[luacmartins]

Are we just missing payment here?

[eh2077]

@luacmartins Yes, that's right. Just asking, is it possible to increase the bounty a little bit as it needed more effort than usual bug fixes. I'm also fine if we just do with the default bounty. Thanks

[luacmartins]

Thanks for confirming! I think the $250 bounty is still fair in this case.

[eh2077]

No problem! Let's just wait for BZ team to issue the payment.

cc @muttmuure

[melvin-bot]

@charles-liang, @luacmartins, @muttmuure, @eh2077 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[muttmuure]

Looking

[muttmuure]

Paid