[HOLD] DEV - `OpenPaymentsPage` is called many times when opening a workspace chat

m-natarajan commented 6 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: Only in Dev Reproducible in staging?: n/a Reproducible in production?: n/a 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: @youssef-lr Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1710976232831539

Action Performed:

Open app
Open any workspace chat with IOU
Expected Result:

There should be one api call for OpenPaymentsPage

Actual Result:

OpenPaymentsPage called many times

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?
- [ ] Android: Native
- [ ] Android: mWeb Chrome
- [ ] iOS: Native
- [ ] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/38435837/01b842ec-622e-43b2-88f7-2e5b80ac40db

View all open jobs on GitHub

melvin-bot[bot] commented 6 months ago

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

ShridharGoel commented 6 months ago

Proposal

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

OpenPaymentsPage is called many times when opening a workspace chat.

What is the root cause of that problem?

This is being called for each SettlementButton.

useEffect(() => {
    PaymentMethods.openWalletPage();
}, []);

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

It doesn't seem needed, we can simply remove the useEffect.

nkdengineer commented 6 months ago

Proposal

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

DEV - OpenPaymentsPage is called many times when opening a workspace chat

What is the root cause of that problem?
We have a logic to call openWalletPage foreach SettlementButton: https://github.com/Expensify/App/blob/47bba81007ae9a4a5dfd6008c626efecb895153d/src/components/SettlementButton.tsx#L134-L136

What changes do you think we should make in order to solve the problem?
We should move this logic to the report that contains these ```SettlementButton. To do that:

First, add the below logic to ReportScreen:

 const didCallOpenWalletPage = useRef(false)
useEffect(() => {
    if(didCallOpenWalletPage.current){
        return
    }
    const shouldCallOpenWalletPage = some(Object.values(reportActions), (action) => {
        if (action.actionName !== CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW) {
            return false;
        }
        const iouReportID = ReportActionsUtils.getIOUReportIDFromReportActionPreview(action);
        const iouReport = ReportUtils.getReport(iouReportID);
        if(ReportUtils.isClosedExpenseReportWithNoExpenses(iouReport)){
            return false
        }
        const shouldShowPayButton = IOU.canIOUBePaid(iouReport, report, policy);
        const shouldShowApproveButton = IOU.canApproveIOU(iouReport, report, policy);
        return shouldShowPayButton || shouldShowApproveButton
    });
    if(shouldCallOpenWalletPage && !didCallOpenWalletPage.current){
        PaymentMethods.openWalletPage();
        didCallOpenWalletPage.current =  true
    }
}, [reportActions, report]);

In the above useEffect, we will check if the report contains at least one ReportPreview action that should show pay button or approve button. If true, call the PaymentMethods.openWalletPage()

What alternative solutions did you explore? (Optional)
NA

bernhardoj commented 6 months ago

This is caused by https://github.com/Expensify/App/pull/30269 where the chat FlatList is remounted multiple times because the listID is renewed multiple times. https://github.com/Expensify/App/blob/9c8ac2975aa898a2fe90da287a4e11b187b4e5b1/src/pages/home/report/ReportActionsView.tsx#L129-L135

youssef-lr commented 6 months ago

cc @perunt @ishpaul777

nkdengineer commented 6 months ago

This issue appear in prod as well

youssef-lr commented 6 months ago

Here's staging @nkdengineer, do you have a recording for prod?

https://github.com/Expensify/App/assets/9680864/9eb28e18-ea66-4108-8501-287ba5c3c0b0

nkdengineer commented 6 months ago

@youssef-lr here is the video in prod (https://new.expensify.com/):

https://github.com/Expensify/App/assets/161821005/1aa2e86e-9a81-4736-abcf-d8a918c2ae80

youssef-lr commented 6 months ago

Can you try staging please?

nkdengineer commented 6 months ago

@youssef-lr Here is staging:

https://github.com/Expensify/App/assets/161821005/3e8308b2-a451-4d8a-bedd-7f26753b1afa

bernhardoj commented 6 months ago

I think it's not deployed to staging yet. On dev, even if you have one button only, it will trigger multiple OpenPaymentsPage calls.

On staging/prod, it will trigger once for each button.

sakluger commented 6 months ago

@bernhardoj if this is related to https://github.com/Expensify/App/pull/30269 as you suspect, that PR has a bunch of comments around potential regressions it has caused. I've asked here in Slack if we should revert it, and I'm going to put this issue on hold while we discuss.\

sakluger commented 6 months ago

Rory said that we will not revert that PR and instead should fix any new issues that arose from it.

@roryabraham @perunt @ishpaul777 could you take a look at this one please?

sakluger commented 6 months ago

Bump @roryabraham @perunt @ishpaul777

perunt commented 6 months ago

There were some fixes implemented related to re-rendering the chat, which should help in this situation as well. Also, since the bug was captured two weeks ago on production, I can carefully assume that you're receiving so many requests because there are a lot of MoneyRequest instances on the page? I mean, are we sure that these were batched before? It's definitely a bad way to handle it right now, but I can assume that this is how we are handling it currently, and it's not a bug, but it is definitely the wrong approach

melvin-bot[bot] commented 6 months ago

@sakluger Eep! 4 days overdue now. Issues have feelings too...

sakluger commented 5 months ago

Adding a hold and moving to weekly while we figure out what to do with the issue.

sakluger commented 5 months ago

Still on hold. Any thoughts on how we should proceed @roryabraham? Should this be added to a project?

sakluger commented 5 months ago

Still on hold I guess.

sakluger commented 5 months ago

Asked again in Slack which project we should add this to.

sakluger commented 4 months ago

No answer, but I'm going to add to #wave-collect since that makes the most sense to me.

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @stephanieelliott (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.

sakluger commented 4 months ago

Hey @stephanieelliott, assigning to you while I'm OOO until May 31. I'm not really sure what to do with this issue - I've posted in Slack a few times asking how to move forward and people agree that we should fix it, but I haven't gotten consensus on how to move it forward. I think we can probably just leave it for now and bump in Slack every few weeks.

stephanieelliott commented 4 months ago

Roger that, will do @sakluger!

stephanieelliott commented 4 months ago

Still holding on this, we're not quite at the polish stage of this wave, we'll prioritize it at that time

sakluger commented 3 months ago

Still on hold

sakluger commented 3 months ago

Still on hold.

sakluger commented 2 months ago

I guess we're still on hold.

sakluger commented 2 months ago

I asked again in Slack how we can move the issue forward.

roryabraham commented 2 months ago

I just retested this on prod. OpenPaymentsPage is only used in the Wallet page. Opening that page, I see OpenPaymentsPage being called only once. So I'm going to close this.

Expensify / App