Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] Chat - Infinite loading occurs after enabling the internet #34577

Closed
2 of 6 tasks
kbecciv opened this issue Jan 16, 2024 · 46 comments
Closed
2 of 6 tasks

[$500] Chat - Infinite loading occurs after enabling the internet #34577

kbecciv opened this issue Jan 16, 2024 · 46 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff

Comments

@kbecciv
Copy link

kbecciv commented Jan 16, 2024

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.25-4
Reproducible in staging?: y
Reproducible in production?: n
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. Log in with 2 separate accounts in the main testing device and a secondary device
  2. Disable the internet connection in the main testing device
  3. From the main + menu, create a money request with the account logged in the secondary device
  4. In the secondary device request money to the main testing device account
  5. Enable the internet connection in in the main testing device, the money request request will be sent and will get a response an error.
  6. Verify that the chat row in the LHN appears greyed out with a solid red dot icon
  7. Click into the chat that failed to create that have a red dot
  8. Verify you see an error message in-line beneath the welcome message because it failed to be created.
  9. Tap/click the X on the Report no longer exists error to dismiss the error message
  10. Verify the chat is deleted and removed from the LHN
  11. Verify you are routed to your chat with Concierge

Expected Result:

After enabled the internet connection in in the main testing device, the money request request will be sent and will get a response an error.

Actual Result:

Infinite loading occurs after enabling the internet

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

Add any screenshot/video evidence

Bug6344248_1705408428309.2024-01-16_02-38-32.1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0115b2b43553abb055
  • Upwork Job ID: 1747315684569538560
  • Last Price Increase: 2024-02-06
  • Automatic offers:
    • aimane-chnaif | Contributor | 28124444
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Jan 16, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Jan 16, 2024

Triggered auto assignment to @arosiclair (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@arosiclair
Copy link
Contributor

arosiclair commented Jan 16, 2024

Attempting to reproduce on prod...

Works correctly on prod v1.4.24-8. Note that this scenario only occurs when there is no pre-existing chat. If there is a pre-existing chat, the offline money request should succeed without issue.

@arosiclair
Copy link
Contributor

Reproduced in staging v1.4.25-7

Screenshot 2024-01-16 at 10 36 33 AM

@arosiclair
Copy link
Contributor

Nothing obvious is jumping out to me in the deploy's PR list or diff. This may be a backend issue

@arosiclair
Copy link
Contributor

arosiclair commented Jan 16, 2024

Okay it seems the RequestMoney command fails and correctly sets the error for the report already existing. However, the following OpenReport command clears the report by setting it to null. This triggers the loading view since isReportReadyForDisplay would evaluate to false here.

Screenshot 2024-01-16 at 11 18 18 AM Screenshot 2024-01-16 at 11 19 24 AM

It seems that the OpenReport call is coming from the ReportActionsView here. Calling OpenReport there seems unnecessary since it's already handled in the ReportScreen. I think we should probably get rid of it, but I'm not sure.

@arosiclair
Copy link
Contributor

Can't find anything in the current deploy that might have caused this. This is also an edge case so I don't think this needs to be a blocker so removing the label

@arosiclair arosiclair added Bug Something is broken. Auto assigns a BugZero manager. and removed DeployBlockerCash This issue or pull request should block deployment Engineering Hourly KSv2 labels Jan 16, 2024
Copy link

melvin-bot bot commented Jan 16, 2024

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Jan 16, 2024
@arosiclair arosiclair added the External Added to denote the issue can be worked on by a contributor label Jan 16, 2024
@melvin-bot melvin-bot bot changed the title Chat - Infinite loading occurs after enabling the internet [$500] Chat - Infinite loading occurs after enabling the internet Jan 16, 2024
Copy link

melvin-bot bot commented Jan 16, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0115b2b43553abb055

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 16, 2024
Copy link

melvin-bot bot commented Jan 16, 2024

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

@DylanDylann
Copy link
Contributor

DylanDylann commented Jan 17, 2024

Proposal

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

  • Chat - Infinite loading occurs after enabling the internet

What is the root cause of that problem?

  • After turning on the network, in main device, calling "RequestMoney" returns the error "There is a previously existing chat between these users.". (It is because in secondary device, we have created the chat between two user)

  • Then, in the main device, call "OpenReport" will set the current report to null so the isReportReadyForDisplay is false.

  • In ReportScreen, we display the "loading indicator" because the isReportReadyForDisplay is false and shouldShowNotFoundPage is false (If shouldShowNotFoundPage is true, it will display the FullPageNotFoundView rather than "loading indicator":

  • Additionally, shouldShowNotFoundPage is false because wasReportAccessibleRef is set to true in the below:

    useEffect(() => {
    if (!report || !report.reportID || shouldHideReport) {
    return;
    }
    wasReportAccessibleRef.current = true;
    }, [shouldHideReport, report]);

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

  • We can display the FullPageNotFoundView in this case. Do it by create a ref called wasReportAccessibleButNowNotRef = useRef(false) (This is used to track if the report was accessible but now is not) and update it to true in the below logic:
    useEffect(() => {
        if (!prevReport.reportID && wasReportAccessibleRef.current && !wasReportAccessibleButNowNotRef.current) {
            wasReportAccessibleButNowNotRef.current = true;
        }
    }, [prevReport.reportID, wasReportAccessibleRef]);
   const shouldShowNotFoundPage = useMemo(
        () =>
            (!wasReportAccessibleRef.current &&
                !firstRenderRef.current &&
                !report.reportID &&
                !isOptimisticDelete &&
                !reportMetadata.isLoadingInitialReportActions &&
                !isLoading &&
                !userLeavingStatus) ||
            shouldHideReport ||
+          wasReportAccessibleButNowNotRef.current,
        [report, reportMetadata, isLoading, shouldHideReport, isOptimisticDelete, userLeavingStatus],
    );

What alternative solutions did you explore? (Optional)

  • NA

@arosiclair
Copy link
Contributor

We can display the FullPageNotFoundView in this case.

We should not display the Not Found page. The correct behavior is to display the error in the chat.

@DylanDylann
Copy link
Contributor

DylanDylann commented Jan 18, 2024

@arosiclair When we refresh the page, it will display the FullPageNotFoundView (in staging), so I think displaying the FullPageNotFoundView can also be an option

@arosiclair
Copy link
Contributor

@arosiclair When we refresh the page, it will display the FullPageNotFoundView (in staging), so I think displaying the FullPageNotFoundView can also be an option

Yes that is fine after refreshing the page, but before that, the user should be able to see the error explaining why the request they made disappeared.

@melvin-bot melvin-bot bot added the Overdue label Jan 22, 2024
@aimane-chnaif
Copy link
Contributor

Same ^

@melvin-bot melvin-bot bot removed the Overdue label Feb 12, 2024
Copy link

melvin-bot bot commented Feb 13, 2024

@arosiclair @kadiealexander @aimane-chnaif this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

  • Decide whether any proposals currently meet our guidelines and can be approved as-is today
  • If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@melvin-bot melvin-bot bot added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Feb 13, 2024
Copy link

melvin-bot bot commented Feb 13, 2024

Current assignee @aimane-chnaif is eligible for the Internal assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added the Overdue label Feb 14, 2024
@arosiclair
Copy link
Contributor

Alright took a second look at this and the root problem seems to be in ReportUtils.buildOptimisticChatReport where we do not set isOptimisticReport. Since it is false, on reconnect the ReportActionsView incorrectly calls OpenReport for the optimistic report here which causes the report to get deleted since it doesn't exist.

Setting isOptimisticReport fixes the issue. I'm also going to update the OptimisticChatReport type to require it to be set.

@melvin-bot melvin-bot bot removed the Overdue label Feb 14, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Feb 14, 2024
@arosiclair arosiclair removed the retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause label Feb 14, 2024
Copy link

melvin-bot bot commented Feb 20, 2024

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

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Mar 11, 2024
Copy link

melvin-bot bot commented Mar 11, 2024

This issue has not been updated in over 15 days. @arosiclair, @kadiealexander, @aimane-chnaif 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!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Mar 11, 2024
@aimane-chnaif
Copy link
Contributor

Reviewing should be replaced with Awaiting payment

@arosiclair arosiclair added Daily KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review Engineering Monthly KSv2 labels Mar 12, 2024
@arosiclair
Copy link
Contributor

@kadiealexander can you just do a C+ payment for @aimane-chnaif? PR was deployed a couple weeks ago. Thanks

@melvin-bot melvin-bot bot added the Overdue label Mar 14, 2024
@kadiealexander
Copy link
Contributor

Payouts due:

Upwork job is here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff
Projects
No open projects
Status: CRITICAL
Development

No branches or pull requests

10 participants