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

[HOLD for payment 2024-07-10] [$250] Chat - Report in the background loads infinitely after refreshing group confirmation page #44442

Closed
2 of 6 tasks
izarutskaya opened this issue Jun 26, 2024 · 29 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Weekly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Jun 26, 2024

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: 9.0.2-0
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to FAB > Start chat.
  3. Click Add to group next to a few users.
  4. Click Next.
  5. Refresh the confirmation page.
  6. Dismiss the confirmation page.

Expected Result:

In Step 5, after refreshing the confirmation page, the report in the background will load.

Actual Result:

In Step 5, after refreshing the confirmation page, the report in the background shows infinite loading.
In Step 6, after dismissing the confirmation page, the link is https://staging.new.expensify.com/r

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

Bug6524928_1719389626682.20240626_160912.mp4

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @CortneyOfstad
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Jun 26, 2024
Copy link

melvin-bot bot commented Jun 26, 2024

Triggered auto assignment to @flodnv (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link

melvin-bot bot commented Jun 26, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Jun 26, 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.

@izarutskaya
Copy link
Author

@CortneyOfstad I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@izarutskaya
Copy link
Author

We think this issue might be related to the #vip-vsb

@flodnv
Copy link
Contributor

flodnv commented Jun 26, 2024

@luacmartins @truph01 @akinwale I see that we modified the loading skeleton in this PR - #43652 - could this DeployBlocker be related with that PR?

@flodnv
Copy link
Contributor

flodnv commented Jun 26, 2024

Actually, that's wrong. I tested reverting that PR on main and the bug is still there.

I used git bisect which tells me that the first bad commit is af02cb4

Indeed, I tested reverting that PR on main and the bug is fixed.

@Kicu @mountiny @roryabraham @rayane-djouah

@mountiny
Copy link
Contributor

Raised in slack

@Kicu
Copy link
Member

Kicu commented Jun 26, 2024

@flodnv interesting, this PR basically does 3 things:

  • remove shim for IE - highly unlikely to be a culprit
  • remove forwardRef that was not used by any component - same thing
  • rewrite ReportScreen from withOnyx to useOnyx - that one looks fishy

In favor of rolling forward instead of rolling back, I'd suggest we don't revert this PR but instead I will provide the revert just for the ReportScreenIDSetter file and we can test if that fixes it - sounds good?
Also @blazejkustra @roryabraham you might be interested to see if this turns out to be some kind of discrepancy between withOnyx and useOnyx

@flodnv
Copy link
Contributor

flodnv commented Jun 26, 2024

That sounds fine as long as it makes sense along that PR, and as long as reverting those changes fix the bug (I tested reverting the full PR only, not just these specific changes)

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Hourly KSv2 labels Jun 26, 2024
@Kicu
Copy link
Member

Kicu commented Jun 26, 2024

@flodnv this should fix it: #44463

here is me trying to reproduce the bug on on my PR:

rec-report-bug.mp4

@roryabraham
Copy link
Contributor

looking at the logs, it appears that the ReportScreenWrapper is not re-rendering when we call navigation.setParams:

image

@roryabraham
Copy link
Contributor

I figured out that adding only the reports back with withOnyx instead of useOnyx solves this problem, but I'm not sure why yet.

@roryabraham
Copy link
Contributor

This is fixed and on prod, we just need to issue a C+ payment to @c3024 for the review

@roryabraham roryabraham removed DeployBlockerCash This issue or pull request should block deployment Reviewing Has a PR in review labels Jun 28, 2024
@roryabraham roryabraham added the Awaiting Payment Auto-added when associated PR is deployed to production label Jun 28, 2024
@roryabraham roryabraham changed the title Chat - Report in the background loads infinitely after refreshing group confirmation page [Pay 7/4] Chat - Report in the background loads infinitely after refreshing group confirmation page Jun 28, 2024
@roryabraham
Copy link
Contributor

@roryabraham
Copy link
Contributor

Just a standard $250 payout for the review here

@CortneyOfstad
Copy link
Contributor

@c3024 — sent you an offer here. Please let me know once you accept. Thank you!

@c3024
Copy link
Contributor

c3024 commented Jun 28, 2024

@CortneyOfstad accepted, thanks!

Copy link

melvin-bot bot commented Jun 29, 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.

Copy link

melvin-bot bot commented Jul 1, 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.

@CortneyOfstad
Copy link
Contributor

This has not deployed to production, as there is a deploy blocker. Going to adjust the title

@CortneyOfstad CortneyOfstad removed the Awaiting Payment Auto-added when associated PR is deployed to production label Jul 3, 2024
@CortneyOfstad CortneyOfstad changed the title [Pay 7/4] Chat - Report in the background loads infinitely after refreshing group confirmation page [$250] Chat - Report in the background loads infinitely after refreshing group confirmation page Jul 3, 2024
Copy link

melvin-bot bot commented Jul 3, 2024

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 3, 2024
@melvin-bot melvin-bot bot changed the title [$250] Chat - Report in the background loads infinitely after refreshing group confirmation page [HOLD for payment 2024-07-10] [$250] Chat - Report in the background loads infinitely after refreshing group confirmation page Jul 3, 2024
Copy link

melvin-bot bot commented Jul 3, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.3-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-07-10. 🎊

For reference, here are some details about the assignees on this issue:

  • @c3024 requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Jul 3, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

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

@CortneyOfstad
Copy link
Contributor

@c3024 can you please complete the checklist above so there is no delay in issuing payment?

Also, i sent you an offer in Upwork (job post here) Let me know once you accept and I'll get that paid ASAP tomorrow 👍

@c3024
Copy link
Contributor

c3024 commented Jul 10, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@c3024] The PR that introduced the bug has been identified. Link to the PR: No PR can be help responsible.
  • [@c3024] 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: NA
  • [@c3024] 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: No. This could not have been identified earlier.
  • [@c3024] Determine if we should create a regression test for this bug. Yes
  • [@c3024] 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.

Regression test proposal

  1. Open any report
  2. Press on FAB > Start Chat
  3. Press on Add to group on some users
  4. Press Next
  5. Refresh the page.
  6. Verify that the report opened in step 1 correctly loads in the background.

@c3024
Copy link
Contributor

c3024 commented Jul 10, 2024

@CortneyOfstad accepted, thanks!

@CortneyOfstad
Copy link
Contributor

Payment Summary

@c3024 — paid $250 via Upwork

Regression Test

Created here

Copy link

melvin-bot bot commented Jul 16, 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.

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. Engineering Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants