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] Conversation - "Chat Skeleton" is blinking on Staging and Chat crashes on PROD site #28799

Closed
6 tasks done
izarutskaya opened this issue Oct 4, 2023 · 34 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@izarutskaya
Copy link

izarutskaya commented Oct 4, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Access staging.new.expensify.com
  2. Sign into a specific account (emilio.utest@gmail.com)
  3. Go to "I am user B" chat (It is an unavailable workspace chat)

Expected Result:

User expects the chat to load normally or some type of message appear stating that the chat can no longer be accesed

Actual Result:

The chat never loads, the "Chat skeleton" keeps on blinking and on the PROD site the conversation crashes the site and forces the user to relogin.

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: v1.3.77-2

Reproducible in staging?: Y

Reproducible in production?: No, on PROD the chat Crashes

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6224057_1696369362998.Crash_on_PROD_site.mp4
Bug6224057_1696369363000.Skeleton_is_blinking_on_staging.mp4

utest-dl.s3.amazonaws.com_12102_26469_432782_6224057_bugAttachment_Bug6224057_1696369362991%21Logs_from_prod_site.txt_X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20231004T110923Z&X-Amz-SignedHeaders=host&X-Amz-Expires=86400&X-Amz-Cr.txt

utest-dl.s3.amazonaws.com_12102_26469_432782_6224057_bugAttachment_Bug6224057_1696369363099%21Logs_on_staging.txt_X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20231004T110908Z&X-Amz-SignedHeaders=host&X-Amz-Expires=86400&X-Amz-Creden.txt

Expensify/Expensify Issue URL:

Issue reported by: Applause-Internal Team

Slack conversation: @

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01544895bdb4802211
  • Upwork Job ID: 1709526511894474752
  • Last Price Increase: 2023-10-25
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 4, 2023
@melvin-bot melvin-bot bot changed the title Conversation - "Chat Skeleton" is blinking on Staging and Chat crashes on PROD site [$500] Conversation - "Chat Skeleton" is blinking on Staging and Chat crashes on PROD site Oct 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01544895bdb4802211

@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

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
Copy link

melvin-bot bot commented Oct 4, 2023

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

@izarutskaya
Copy link
Author

Not repro on Prod

crash.mp4

@OSBotify
Copy link
Contributor

OSBotify commented Oct 4, 2023

👋 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.

@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

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

@mountiny
Copy link
Contributor

mountiny commented Oct 4, 2023

this is probably whats causing the crash in production

18:42:11.952 react-dom.production.min.js:186 TypeError: Cannot read properties of undefined (reading 'participantAccountIDs')
    at Za (ReportUtils.js:660:19)
    at Ba (ReportUtils.js:856:13)
    at Object.Va (ReportUtils.js:887:39)
    at ReportActionCompose.js:153:15
    at Object.useMemo (react-dom.production.min.js:182:189)
    at t.useMemo (react.production.min.js:25:208)
    at Ui (ReportActionCompose.js:152:48)
    at br (react-dom.production.min.js:166:137)
    at ys (react-dom.production.min.js:289:386)
    at fl (react-dom.production.min.js:279:389)

@mountiny
Copy link
Contributor

mountiny commented Oct 4, 2023

I am not sure how to treat this, we dont have access to the account you have linked for testing, seems like production has crash and staging handles this better

@dukenv0307
Copy link
Contributor

dukenv0307 commented Oct 4, 2023

Proposal

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

Conversation - "Chat Skeleton" is blinking on Staging and Chat crashes on PROD site

What is the root cause of that problem?

If the report hasn't been loaded yet, this function will make the app crashes when we call canShowReportRecipientLocalTime in ReportActionCompose

App/src/libs/ReportUtils.js

Lines 659 to 661 in ab821e5

function hasSingleParticipant(report) {
return report && report.participantAccountIDs && report.participantAccountIDs.length === 1;
}

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

We should use lodashGet or safely check whether the report is undefined or not in hasSingleParticipant function

What alternative solutions did you explore? (Optional)

@fedirjh
Copy link
Contributor

fedirjh commented Oct 4, 2023

this function will make the app crashes when we call canShowReportRecipientLocalTime in ReportActionCompose

@dukenv0307 can you please show me where hasSingleParticipant is used inside ReportActionCompose?

FWIW hasSingleParticipant seems to have the necessary safety check ...

@dukenv0307
Copy link
Contributor

dukenv0307 commented Oct 4, 2023

@fedirjh
In canShowReportRecipientLocalTime, we call getReportRecipientAccountIDs function

const reportRecipientAccountIDs = getReportRecipientAccountIDs(report, accountID);

And then we call hasSingleParticipant here

if (hasSingleParticipant(parentReport)) {

I've encountered crashes like this before, but I don't remember how to reproduce it. So posting the proposal here may be the root cause

@mountiny
Copy link
Contributor

mountiny commented Oct 4, 2023

@dukenv0307 I think report && this is safe enough isnt it?

I am not sure about this ad deploy blocker, seems more so as something odd with this specific account for testing

I am going to demote this now, but leave this open for furhter investigation, if there are better repo steps not related to that specific account maybe

@mountiny mountiny added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Oct 4, 2023
@alexpensify
Copy link
Contributor

To confirm, should the proposal here be reviewed, or wait for more proposals? Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Oct 11, 2023
@alexpensify
Copy link
Contributor

Still waiting for proposals. If no movement next week, I'll start asking around for help.

@melvin-bot melvin-bot bot added the Overdue label Oct 16, 2023
@alexpensify
Copy link
Contributor

Still waiting for proposals

@melvin-bot melvin-bot bot removed the Overdue label Oct 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2023

@madmax330 @alexpensify @fedirjh this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2023

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

@alexpensify
Copy link
Contributor

We need proposals here

@akamefi202
Copy link
Contributor

Proposal

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

"Chat Skeleton" is blinking on Staging and Chat crashes on PROD site.

What is the root cause of that problem?

In my experience, blinking loop error occurs when the back-end returns an empty reportActions array on OpenReport API Request.
This is unexpected error from the back-end because reportActions array usually contains at least one item("Created").

const isLoadingInitialReportActions = _.isEmpty(reportActions) && reportMetadata.isLoadingReportActions;

In more details, if reportActions array is empty, the value of isLoadingInitialReportActions is equal to reportMetadata.isLoadingReportActions.

{isReportReadyForDisplay && !isLoadingInitialReportActions && !isLoading && (
<ReportActionsView

{(!isReportReadyForDisplay || isLoadingInitialReportActions || isLoading) && <ReportActionsSkeletonView />}

By the value of isLoadingInitialReportActions, the report screen shows skeleton view or report actions view.
If the value of isLoadingInitialReportActions is changed from false to true, it mounts skeleton view.
If the value is changed from true to false, it mounts report actions view.

Report.openReport(reportID);

When ReportActionsView component mounts, it calls OpenReport function to get data such as reportActions array.

isLoadingReportActions: false,

isLoadingReportActions: true,

Inside OpenReport function, it sets reportMetadata.isLoadingReportActions as true in optimistic data and false in failure data.
And this updates the value of isLoadingInitialReportActions directly and it causes limitless mounting of ReportActionsView component.

Because mounting of ReportActionsView component calls OpenReport function again.
And OpenReport function causes another mounting by updating isLoadingInitialReportActions.

Usually, this doesn't happen because reportActions array has more than one item at least.
And it keeps the value of isLoadingInitialReportActions as false.
So that ReportActionsView doesn't mount again, although OpenReport function is called.

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

This is an error from the back-end but we might add an error handler for this.

errorFields: {
    openReport: ErrorUtils.getMicroSecondOnyxError('report.genericOpenReportFailureMessage'),
},

We can add openReport error if OpenReport API Request is failed.
Of course, we should clear this error if the request is succeeded.

<FullPageNotFoundView
shouldShow={shouldShowNotFoundPage}
subtitleKey="notFound.noAccess"
shouldShowCloseButton={false}
shouldShowBackButton={isSmallScreenWidth}
onBackButtonPress={Navigation.goBack}
shouldShowLink={false}
>

And by updating above code part, we can show appropriate error message if there is openReport error & reportActions array is empty.
As a result, we can break the loop and the error message will be shown in the report screen.

What alternative solutions did you explore? (Optional)

N/A

@alexpensify
Copy link
Contributor

@fedirjh - can you review if this proposal will fix this issue? Thanks!

@fedirjh
Copy link
Contributor

fedirjh commented Oct 20, 2023

In my experience, blinking loop error occurs when the back-end returns an empty reportActions array on OpenReport API Request.

@akamefi202 Can you share the response of the OpenReport API request that caused the error?

Because mounting of ReportActionsView component calls OpenReport function again.
And OpenReport function causes another mounting by updating isLoadingInitialReportActions.

Please verify from your devtools network tab whether the request is being retried due to these race conditions. Screenshots are appreciated in this case.

@akamefi202 Were you able to reproduce the crash?

@akamefi202
Copy link
Contributor

@fedirjh I'm not able to reproduce this bug.
As you can see in reproduction steps, this error is shown in very specific situation.
I found this error while I was working on other issue, but we fixed the problem from back-end side in that case.

I think the reporter can confirm response of OpenReport API Request using emilio.utest@gmail.com and "I am user B" user.
I don't have access to emilio.utest@gmail.com so I can't follow reproduction steps.

@melvin-bot melvin-bot bot added the Overdue label Oct 23, 2023
@fedirjh
Copy link
Contributor

fedirjh commented Oct 23, 2023

@izarutskaya Can you please retest if this bug still exists?

@melvin-bot melvin-bot bot removed the Overdue label Oct 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 25, 2023

@madmax330 @alexpensify @fedirjh this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

@melvin-bot melvin-bot bot added the Overdue label Oct 25, 2023
@madmax330
Copy link
Contributor

@izarutskaya can you check if this is happening? If it's not let's close this out.

@melvin-bot melvin-bot bot removed the Overdue label Oct 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 25, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Oct 30, 2023
@fedirjh
Copy link
Contributor

fedirjh commented Oct 30, 2023

@izarutskaya Little bump for #28799 (comment)

@melvin-bot melvin-bot bot removed the Overdue label Oct 30, 2023
Copy link

melvin-bot bot commented Nov 1, 2023

@madmax330 @alexpensify @fedirjh 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 Nov 1, 2023
Copy link

melvin-bot bot commented Nov 1, 2023

Current assignee @fedirjh is eligible for the Internal assigner, not assigning anyone new.

@madmax330
Copy link
Contributor

I'm just going to close this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

8 participants