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

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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:

1. Go to staging.new.expensify.com
2. Navigate to FAB> Request money
3. 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?

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

Screenshots/Videos

Bug6287893_1700694798230.Recording__1396.mp4

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]

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

[melvin-bot]

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

[melvin-bot]

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]

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

[felix-lambert]

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

Screen.Recording.2023-11-23.at.13.05.06.mov

[melvin-bot]

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

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>

[felix-lambert]

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

[melvin-bot]

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

[jliexpensify]

@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]

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]

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

[jliexpensify]

Bumping @ntdiary for reviews please!

[ntdiary]

Bumping @ntdiary for reviews please!

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

[ntdiary]

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]

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]

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]

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]

@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]

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]

@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]

@brunovjk thanks I did :)

Proposal

Updated

[ntdiary]

What do you guys think?

App/src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js

Line 322 in 4239252

shouldShowOptions={isOptionsDataReady}

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]

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

[brunovjk]

@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]

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]

We're still investigating, PR

[melvin-bot]

⚠️ 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]

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]

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?

[brunovjk]

Exactly. Awesome! Thank you @jliexpensify

[jliexpensify]

Payment Summary

• Contributor: @brunovjk - $500
• C+: @ntdiary - $500

@ntdiary anything on the checklist that needs to be completed?

NEW UPWORKS JOB

[ntdiary]

BugZero Checklist:

• [@ntdiary] The PR that introduced the bug has been identified. Link to the PR: N/A
• [@ntdiary] 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
• [@ntdiary] 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
• [@ntdiary] Determine if we should create a regression test for this bug. N/A
• [@ntdiary] 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. N/A
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@jliexpensify, we didn't add the skeleton animation for transition on this selector page before, so this issue can be considered as UI/UX optimization rather than a bug, and it doesn't require regression test. :)

[jliexpensify]

Hi @ntdiary , can you accept the offer so I can pay you?

[jliexpensify]

Paid @brunovjk

[ntdiary]

Ah, @jliexpensify, sorry for the delay, have just accepted it. 😂

[jliexpensify]

Hey @ntdiary - there was an error when I tried paying you, can you confirm you got the payment?

[jliexpensify]

Actually I think I know the issue - I may have accidentally created a per-hour contract instead of one for the full amount. Going to end this contract and re-hire you, sorry!

[jliexpensify]

@ntdiary sorry, re-hired you!

[ntdiary]

@ntdiary sorry, re-hired you!

@jliexpensify, re-accepted. 😄

[jliexpensify]

Paid and job closed!

Upworks is so frustrating, there's no way to change a contract from hourly to fixed amount after it's been accepted!