[PAY WENTAO - FEB 7th][$500] BNP - Black screen appears on transfer from BNP to Attendee screen

izarutskaya commented 11 months ago

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: 1.4.2-0 Reproducible in staging?: Y Reproducible in production?: Y 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: Applause-Internal Team Slack conversation: @

Action Performed:

Go to staging.new.expensify.com
Navigate to FAB> Request money
Add amount and click Next

Expected Result:

Attendee screen should be display and no lagging and transfer error should be present

Actual Result:

Black screen appears on transfer from BNP to Attendee screen

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/596d1b2e-85c1-4425-bb96-7c157c450104

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01d583113b9a89a85a
Upwork Job ID: 1727650719394332672
Last Price Increase: 2023-11-30
Automatic offers:

ntdiary | Reviewer | 27979335

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] 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
[ ] 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.
[ ] 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

melvin-bot[bot] commented 11 months ago

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

felix-lambert commented 11 months ago

I cannot reproduce the bug. Is there something i have been missing?

https://github.com/Expensify/App/assets/6106055/7be93b57-4b80-457e-85e7-8fcf0b6951be

melvin-bot[bot] commented 11 months ago

📣 @felix-lambert! 📣 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:

Make sure you've read and understood the contributing guidelines.
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.
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.
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>
```

felix-lambert commented 11 months ago

Contributor details Your Expensify account email: totoflavien45@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01b7ffb7510bab69aa?viewMode=1

melvin-bot[bot] commented 11 months ago

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

jliexpensify commented 11 months ago

@izarutskaya curioous why you're testing on 1.4.2.0? I'm on staging and can see this issue on v1.4.3-1 - shouldn't you be testing on the most recent versions?

Anyway, I can repro this one - the lag is about 1 second.

felix-lambert commented 11 months ago

Proposal

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

We are experiencing a lag in transition effects within our application, most noticeably when interacting with a large number of contact entries. This issue affects the fluidity and responsiveness of the user interface, particularly during operations that involve extensive contact lists.

What is the root cause of that problem?

The root cause of this lag is the intensive computational demand of the getOptions function when processing a large dataset of contacts.

The performance lag in theOptionsListUtils.js file is caused by the way the personalDetails object is handled. At line 433, there's a loop where personalDetails is repeatedly accessed within each iteration. This object, being very large, leads to a heavy computational load. The issue is exacerbated because each loop iteration requires accessing this large object to retrieve values for the createOption function, resulting in significant performance degradation due to the intensive and repeated data processing involved.

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

To address this issue fast, I propose the following changes:

    useEffect(() => {
        setTimeout(() => {
            // Code to execute after transition ends
            const chatOptions = OptionsListUtils.getFilteredOptionsClone(
                reports,
                personalDetails,
                betas,
                searchTerm,
                participants,
                CONST.EXPENSIFY_EMAILS,

                // If we are using this component in the "Request money" flow then we pass the includeOwnedWorkspaceChats argument so that the current user
                // sees the option to request money from their admin on their own Workspace Chat.
                iouType === CONST.IOU.TYPE.REQUEST,

                // We don't want to include any P2P options like personal details or reports that are not workspace chats for certain features.
                !isDistanceRequest,
                // We don't want the user to be able to invite individuals when they are in the "Distance request" flow for now.
                // This functionality is being built here: https://github.com/Expensify/App/issues/23291
                !isDistanceRequest,
                true,
            );

            setNewChatOptions({
                recentReports: chatOptions.recentReports,
                personalDetails: chatOptions.personalDetails,
                userToInvite: chatOptions.userToInvite,
            });
        }, CONST.ANIMATED_TRANSITION);
    }, []);

I'm basically detecting when the animation of the transition is finished, ensuring UI responsiveness is maintained :)

What alternative solutions did you explore? (Optional)

To go deeper, I have a few options to propose for addressing the performance issue.

Necessity of All createOption Values: Assess whether it's essential to use all values from the createOption function for displaying the contact list. If not all values are necessary, this could reduce the computational load.
Alternative Data Handling Strategies: Avoiding personalDetails in the Loop: Explore methods to access the required values without passing the personalDetails object within the map loop. This could potentially alleviate the performance issue.
Server-side Data Flattening: Consider flattening the data on the server side before it's sent to the client. This approach could simplify the data structure, making it more efficient to process on the client side.
Implementing Pagination: Loading Contacts in Chunks: Implement a pagination system where only a subset of the contact list, say 50 contacts, is loaded initially. This can significantly reduce the initial data load.

melvin-bot[bot] commented 11 months ago

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

jliexpensify commented 11 months ago

Bumping @ntdiary for reviews please!

ntdiary commented 11 months ago

Bumping @ntdiary for reviews please!

@jliexpensify, thank you for the reminder! Will review it today. :)

ntdiary commented 11 months ago

The getOptions function has complex logic, and I'm concerned that wrapping it with memoize may not be safe enough. If this function references other external variables, it could easily introduce potential issues.

Maybe we can explore how to reliably delay its invocation to avoid this transition lag.

felix-lambert commented 11 months ago

Yes I agree. We can see how to add a delay so that the animation of the modal is finished before the invocation of the getOptions function. Do you want me to work on this issue?

brunovjk commented 11 months ago

Proposal

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

The issue at hand involves a lag and the display of a black screen instead of the expected skeleton loading during the rendering of the contact list.

What is the root cause of that problem?

We notive some points here:

1- Resource-intensive getOptions Function: The getOptions function is resource-intensive, especially when dealing with a large dataset of contacts.

2 - Delayed Rendering of Contact List: The page is waiting for the complete contact list before initiating the rendering process.

3 - Lack of Skeleton Loading Display: Skeleton loading is not visible during the loading of the contact list.

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

1 - Asynchronous Processing of getOptions: The proposed solution involves introducing asynchronous processing for the getOptions function to alleviate its performance impact. Tests are ongoing, and any assistance is appreciated.

2 - Utilizing didScreenTransitionEnd: Use the didScreenTransitionEnd property from MoneyRequestParticipantsPage to trigger the retrieval of new options (contacts) only after the page has completed loading.

3 - State Management for isOptionsDataReady: Move isOptionsDataReady to a useState hook and ensure that it is evaluated only after the page has loaded.

Step 2 and 3 we modify the following sections in src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js:

...
        <ScreenWrapper
            includeSafeAreaPaddingBottom={false}
            shouldEnableMaxHeight={DeviceCapabilities.canUseTouchScreen()}
            onEntryTransitionEnd={() => optionsSelectorRef.current && optionsSelectorRef.current.focus()}
            testID={MoneyRequestParticipantsPage.displayName}
        >
            {({safeAreaPaddingBottomStyle, didScreenTransitionEnd}) => (
...
         <MoneyRequestParticipantsSelector
...
         didScreenTransitionEnd={didScreenTransitionEnd}

and src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js:

...
    useEffect(() => {
        if(didScreenTransitionEnd){
            const chatOptions = OptionsListUtils.getFilteredOptions(
...
            });
            setIsOptionsDataReady(ReportUtils.isReportDataReady() && OptionsListUtils.isPersonalDetailsReady(personalDetails));
        }
    }, [betas, reports, participants, personalDetails, translate, searchTerm, setNewChatOptions, iouType, isDistanceRequest, didScreenTransitionEnd]);
...

What alternative solutions did you explore? (Optional)

ntdiary commented 11 months ago

Yes I agree. We can see how to add a delay so that the animation of the modal is finished before the invocation of the getOptions function. Do you want me to work on this issue?

@felix-lambert, that is just my general idea (also just one of the possible solutions). Typically, we need a concrete and feasible solution before we can start assigning and raising a PR. :)

ntdiary commented 11 months ago

@brunovjk, if you recommend asynchronous operations, can you pelease provide more details? Btw, It seems that we haven't used the worker in our app yet.

felix-lambert commented 11 months ago

Hey @ntdiary sorry just discovering this great project :)

Was thinking of doing something like this >

useEffect(() => {
        const unsubscribe = navigation.addListener('transitionEnd', () => {

            // Code to execute after transition ends
            const res = OptionsListUtils.getFilteredOptions(
                reports,
                personalDetails,
                betas,
                searchTerm,
                participants,
            );

            setNewChatOptions({
                recentReports: res.recentReports,
                personalDetails: res.personalDetails,
                userToInvite: res.userToInvite,
            });
        });

        return unsubscribe;
    }, [navigation]);

Don't hesitate if you need more details. I'm basically detecting when the animation of the transition is finished, ensuring UI responsiveness is maintained :)

brunovjk commented 11 months ago

@ntdiary, I appreciate your answer. I'm currently in the process of testing approaches to ensure a seamless integration without introducing new bugs. I'll provide a detailed update as soon as I have a concrete async implementation.

I noticed that the skeleton placeholder, which is typically rendered in other parts, like in src/pages/workspace/WorkspaceInvitePage.js of the application during the processing of getOptions, is not appearing as expected in this specific issue src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js. Do you have an idea why? Is it a expected behavior?

@felix-lambert update your proposal :D

felix-lambert commented 11 months ago

@brunovjk thanks I did :)

Proposal

Updated

brunovjk commented 11 months ago

Proposal

Updated

melvin-bot[bot] commented 11 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

jliexpensify commented 11 months ago

@ntdiary bump for reviews, thanks!

ntdiary commented 11 months ago

In testing. :)

ntdiary commented 11 months ago

https://github.com/Expensify/App/assets/8579651/d9d6dbd3-1197-49be-bc0b-35bbcf61a8b4

The two proposals share some similarities. Additionally, as seen in the videos above, using either transitionEnd or didScreenTransitionEnd has already mitigated the transition lag. However, I think it would be better if we can optimize getOptions function.

By the way, I actually tested didScreenTransitionEnd two days ago. When the page is refreshed, both transitionEnd and didScreenTransitionEnd within the component does not work, causing the options not to be loaded.

brunovjk commented 11 months ago

Thank you very much for the review @ntdiary. What do you think about using the worker in our app? I see, I also think it would be better if we can optimize getOptions function. I'm still working on it, I'll get back to you as soon as possible.

By the way, I actually tested didScreenTransitionEnd two days ago. When the page is refreshed, both transitionEnd and didScreenTransitionEnd within the component does not work, causing the options not to be loaded.

Interesting, it makes sense, I'll continue with getOptions but I believe that the MoneyRequestParticipantsPage and MoneyRequestParticipantsSelector logic should be reviewed, WorkspaceInvitePage has a similar job and LGTM: https://github.com/Expensify/App/blob/27fd4e24a5dfc859bcafcb4e749f9b857e6813d5/src/pages/workspace/WorkspaceInvitePage.js#L230-L236

felix-lambert commented 11 months ago

To make sure getOptions is being called after refresh maybe we could do that

    useEffect(() => {
        setTimeout(() => {
            // Code to execute after transition ends
            const chatOptions = OptionsListUtils.getFilteredOptionsClone(
                reports,
                personalDetails,
                betas,
                searchTerm,
                participants,
                CONST.EXPENSIFY_EMAILS,

                // If we are using this component in the "Request money" flow then we pass the includeOwnedWorkspaceChats argument so that the current user
                // sees the option to request money from their admin on their own Workspace Chat.
                iouType === CONST.IOU.TYPE.REQUEST,

                // We don't want to include any P2P options like personal details or reports that are not workspace chats for certain features.
                !isDistanceRequest,
                // We don't want the user to be able to invite individuals when they are in the "Distance request" flow for now.
                // This functionality is being built here: https://github.com/Expensify/App/issues/23291
                !isDistanceRequest,
                true,
            );

            setNewChatOptions({
                recentReports: chatOptions.recentReports,
                personalDetails: chatOptions.personalDetails,
                userToInvite: chatOptions.userToInvite,
            });
        }, CONST.ANIMATED_TRANSITION);
    }, []);

The performance lag in theOptionsListUtils.js file is caused by the way the personalDetails object is handled. At line 433, there's a loop where personalDetails is repeatedly accessed within each iteration. This object, being very large, leads to a heavy computational load. The issue is exacerbated because each loop iteration requires accessing this large object to retrieve values for the createOption function, resulting in significant performance degradation due to the intensive and repeated data processing involved.

To optimize the display of the contact list, I have a few options to propose for addressing the performance issue.

Necessity of All createOption Values: Assess whether it's essential to use all values from the createOption function for displaying the contact list. If not all values are necessary, this could reduce the computational load.
Alternative Data Handling Strategies: Avoiding personalDetails in the Loop: Explore methods to access the required values without passing the personalDetails object within the map loop. This could potentially alleviate the performance issue.
Server-side Data Flattening: Consider flattening the data on the server side before it's sent to the client. This approach could simplify the data structure, making it more efficient to process on the client side.
Implementing Pagination: Loading Contacts in Chunks: Implement a pagination system where only a subset of the contact list, say 50 contacts, is loaded initially. This can significantly reduce the initial data load.

Which of these approaches do you think would be the best to explore further?

Proposal

Updated

brunovjk commented 11 months ago

Thank you for the review, @ntdiary.

I agree that optimizing the getOptions function would be beneficial. However, there's a challenge – the loops inside getOptions seem to be causing lag, and their performance is dependent on the size of the contact list. While attempting to enhance the efficiency of getOptions and these loops, I've encountered difficulties. It's expected that the function may take longer if the database is extensive, and I've been exploring optimizations without much success. I believe by using the skeleton loading approach, as suggested, we can solve this issue.

However, we still face the problem of page reloading:

By the way, I actually tested didScreenTransitionEnd two days ago. When the page is refreshed, both transitionEnd and didScreenTransitionEnd within the component do not work, causing the options not to load.

I've observed a similar situation in other parts of our app, where the contact list fails to load after a page refresh. For instance:

This seems peculiar since:

return (
    <ScreenWrapper
    ...
    {({safeAreaPaddingBottomStyle, didScreenTransitionEnd}) => (

successfully loads when the page is refreshed. Any insights, @ntdiary and @felix-lambert?

@felix-lambert, I tested your latest proposal. IMP using a hardcoded delay with setTimeout introduces an arbitrary delay (CONST.ANIMATED_TRANSITION). While it may work in most cases, it's not foolproof, as the duration of the transition can vary. Depending on the timing of the transition, network conditions, or other factors, this solution may not be reliable in all scenarios.

melvin-bot[bot] commented 11 months ago

@ntdiary, @jliexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

jliexpensify commented 11 months ago

Bumping @ntdiary

ntdiary commented 11 months ago

Still reviewing the updated proposal.

brunovjk commented 11 months ago

Still reviewing the updated proposal.

@ntdiary did you encounter the same problem, when we refresh the page and the list does not load, in other parts of the app? Thanks.

ntdiary commented 11 months ago

Still reviewing the updated proposal.

@ntdiary did you encounter the same problem, when we refresh the page and the list does not load, in other parts of the app? Thanks.

Certainly, I had noticed this problem earlier as well. :)

Thank you very much for all the discussions! Let's summarize briefly here: in this issue, we discussed three points:

Black screen during the transition.
Improve the performance of getOption.
When refreshing the page, the transitionEnd event cannot be detected, causing the page not to load.

For the 1st point, I prefer @brunovjk's proposal, which is to reuse the existing didScreenTransitionEnd variable.

For the 2nd point, I have to say that it is a difficult task to optimize this function now, its logic is so complex and called from multiple places, so it may be better to create a separate optimization issue for it.

As for the 3rd point, it is more like an edge case and already exists in a few other scenarios, so I feel it's okay to create a separate issue as well. Alternatively, we could simply use InteractionManager.runAfterInteractions(() => { setDidScreenTransitionEnd(true); }); ( though this is not a perfect way).

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 11 months ago

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

lakchote commented 11 months ago

Thank you @ntdiary for your review! I do prefer to reuse the existing didScreenTransitionEnd variable too.

Let's focus on the first point which is the black screen during the transition.

Feel free to create separate issues for the two other points already mentioned.

@brunovjk's proposal LGTM 👍

melvin-bot[bot] commented 11 months ago

📣 @ntdiary 🎉 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] commented 11 months ago

📣 @brunovjk You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

brunovjk commented 11 months ago

Thank you for the review @ntdiary and @lakchote.

I create a PR to use didScreenTransitionEnd.

But we still have the skeleton loading issue, it's not being triggered. I checked and isOptionsDataReady looks always true:

https://github.com/Expensify/App/blob/bbe73efdfab056d5ab6d864941fc3da44ec1c084/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js#L225

I was thinking in move to a useState and control it only after transition end:

...
const [isOptionsDataReady, setIsOptionsDataReady] = useState(false);
...
     useEffect(() => {
        if (didScreenTransitionEnd) {
...
     setIsOptionsDataReady(ReportUtils.isReportDataReady() && OptionsListUtils.isPersonalDetailsReady(personalDetails))        
...
        }
    }, [betas, reports, participants, personalDetails, translate, searchTerm, setNewChatOptions, iouType, isDistanceRequest, didScreenTransitionEnd, setIsOptionsDataReady]);

What do you guys think?

situchan commented 11 months ago

Sorry for late. We should hold this for money request navigation refactor

brunovjk commented 10 months ago

Sorry for late. We should hold this for money request navigation refactor

Let's wait then, but the PR is almost ready for review, still testing on other platforms.

lakchote commented 10 months ago

What do you guys think?

I'm fine with that, but let's hear @ntdiary's opinion on it too!

ntdiary commented 10 months ago

What do you guys think?

https://github.com/Expensify/App/blob/4239252d5ace1039fa2aaf8d707f4e5de8523309/src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js#L322 We can also change the above condition to didScreenTransitionEnd && isOptionsDataReady. Additionally, although the refactor PR has been merged, there are still some blockers that need to be resolved, so it would be better to put this issue on hold for a few days. 😂

brunovjk commented 10 months ago

Thank you for the feedback @ntdiary, makes senses, I liked this shouldShowOptions={didScreenTransitionEnd && isOptionsDataReady} approach. Lets wait then :D

brunovjk commented 10 months ago

@ntdiary and @lakchote the refactored money request navigation was merged but the initial issue remains.

I think we can apply the same solution proposed previously.

I updated the PR to apply changes at new components used. I still need to make videos on the rest of the platforms.

melvin-bot[bot] commented 9 months ago

This issue has not been updated in over 15 days. @ntdiary, @lakchote, @jliexpensify, @brunovjk eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

brunovjk commented 9 months ago

We're still investigating, PR

melvin-bot[bot] commented 9 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

brunovjk commented 9 months ago

Hello @jliexpensify the PR that fixed this issue was deployed today, but I didn't link the issue correctly over there, so I think the automation was broken, and it didn't receive the HOLD payment for 7 days tag here. Do you think I should do something or is everything ok? Thanks for your time.

jliexpensify commented 9 months ago

Hi @brunovjk - we generally pay 7 days after deployment to Production and I'll do a payment summary as well.

So I would say the payout date would be the 7th Feb?

Expensify / App