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 9/26] [$250] CRITICAL: [UX Reliability] Old messages show up as most recent, and the new messages didn't load for several minutes #43656

Closed
6 tasks
muttmuure opened this issue Jun 13, 2024 · 89 comments
Assignees
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@muttmuure
Copy link
Contributor

muttmuure commented Jun 13, 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: v1.4.82-4
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: @kevinksullivan
Slack conversation: https://expensify.slack.com/archives/C05LX9D6E07/p1717013309521369

Action Performed:

Break down in numbered steps

As part of the OpenApp, we load a parent reportAction for all reports stored locally. When the app executes OpenReport, if present, we load the parent reportAction first.
OpenApp command now returns the last report action for each chat in the LHN. We want to show it instantly when the user opens a report from the LHN, and the loading skeleton above it.

  1. Alice texts message A to Bob in their DM
  2. Bob replies in a thread to the message A, which creates a new chat in their LHN
  3. Alice and Bob then continue to chat in their DM and end the conversation with the message B
  4. Bob signs out and then signs in again
  5. Bob now sees the DM and the thread of the message A in the LHN
  6. The DM has the preview of the message B in the LHN
  7. Bob opens the DM

Expected Result:

Bob should see the message B as the last message on the report page and a loading skeleton above it, indicating that the report actions are being fetched

Actual Result:

Bob sees the message A on the report page, with no loading spinner, until the OpenReport command returns the report actions. Sometimes Bob sees no message but a full page loading skeleton.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019999ccc983033009
  • Upwork Job ID: 1803425149604648150
  • Last Price Increase: 2024-08-26
  • Automatic offers:
    • CyberAndrii | Contributor | 103753081
Issue OwnerCurrent Issue Owner: @Kureev
@muttmuure muttmuure added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. AutoAssignerNewDotQuality Used to assign quality issues to engineers labels Jun 13, 2024
Copy link

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

Copy link

melvin-bot bot commented Jun 13, 2024

Triggered auto assignment to @cristipaval (AutoAssignerNewDotQuality)

@melvin-bot melvin-bot bot added the Weekly KSv2 label Jun 13, 2024
@muttmuure
Copy link
Contributor Author

From @mountiny -

We need to explore what is the consequence of not returning the parent report actions in the OpenApp for the app. It should solve this issue but I am not sure about its side effects

@cristipaval
Copy link
Contributor

cristipaval commented Jun 17, 2024

Just started to look into this one. I'll first start with the Slack convo about the issue.

@melvin-bot melvin-bot bot removed the Overdue label Jun 17, 2024
@cristipaval
Copy link
Contributor

Yes, I agree with @mountiny's suggested solution. I'll investigate if it is feasible not to return the parent report action in the parent report.

@melvin-bot melvin-bot bot added the Overdue label Jun 19, 2024
@cristipaval cristipaval added the External Added to denote the issue can be worked on by a contributor label Jun 19, 2024
@melvin-bot melvin-bot bot changed the title CRITICAL: [UX Reliability] Old messages show up as most recent, and the new messages didn't load for several minutes [$250] CRITICAL: [UX Reliability] Old messages show up as most recent, and the new messages didn't load for several minutes Jun 19, 2024
Copy link

melvin-bot bot commented Jun 19, 2024

Job added to Upwork: https://www.upwork.com/jobs/~019999ccc983033009

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

melvin-bot bot commented Jun 19, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Jun 19, 2024
@cristipaval cristipaval changed the title [$250] CRITICAL: [UX Reliability] Old messages show up as most recent, and the new messages didn't load for several minutes [$500] CRITICAL: [UX Reliability] Old messages show up as most recent, and the new messages didn't load for several minutes Jun 19, 2024
Copy link

melvin-bot bot commented Jun 19, 2024

Upwork job price has been updated to $500

@cristipaval
Copy link
Contributor

cristipaval commented Jun 19, 2024

The bounty for the Critical issues is $500, correct?

@cristipaval
Copy link
Contributor

Yes, I agree with @mountiny's suggested solution. I'll investigate if it is feasible not to return the parent report action in the parent report.

Unfortunately, this doesn't seem feasible. I added the External label to get the contributors' help checking if we could update the existing logic around the loading spinner showing at the bottom of the chat when the App fetches report actions.

@CortneyOfstad
Copy link
Contributor

Sounds good and thank you for looking at @cristipaval!

@cristipaval
Copy link
Contributor

FYI @CortneyOfstad, we're discussing more about this issue here

Copy link

melvin-bot bot commented Aug 30, 2024

Current assignee @cristipaval is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@rushatgabhane
Copy link
Member

rushatgabhane commented Aug 30, 2024

(and assign me)

Copy link

melvin-bot bot commented Aug 30, 2024

📣 @CyberAndrii 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@dylanexpensify
Copy link
Contributor

assigned all the peeps!

@dangrous
Copy link
Contributor

Proposal looks good to me too, my only note is that last time we migrated something to useOnyx it ended up breaking everything haha so we should just make sure to test a lot, even unrelated flows!

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 31, 2024
@rushatgabhane
Copy link
Member

rushatgabhane commented Sep 9, 2024

@CyberAndrii our PR has been reverted because it caused a bug.

Bug: Old reports don't open because openReport command is not called.

Steps to reproduce

  1. Go to Onyx, and manually clear report_[reportID].
  2. Navigate to the reportID - r/[reportID]
  3. Verify that the report opens. i.e. openReport command is called from network tab

See more here - https://expensify.slack.com/archives/C01GTK53T8Q/p1725902444282339?thread_ts=1725890512.566159&cid=C01GTK53T8Q

@rushatgabhane
Copy link
Member

Alright we're back to where we started.

@CyberAndrii @kirillzyusko @allroundexperts i could use all your help to fix the app crash when we switch accounts as a copilot.

@CyberAndrii
Copy link
Contributor

CyberAndrii commented Sep 9, 2024

@rushatgabhane This line is causing the issue

parameters.clientLastReadTime = ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.lastReadTime ?? '';

Before my PR, the clientLastReadTime property was set to an empty string because ${ONYXKEYS.COLLECTION.REPORT}${reportID} was not yet loaded at that point. However since OpenReport is now called slightly later, it receives a value like 2024-09-09 19:35:18.842 which causes the backend to return a 403 error.

@rushatgabhane
Copy link
Member

which causes the backend to return a 403 error

not sure how did you get 403 error. Do you have any reproducible steps?

@CyberAndrii
Copy link
Contributor

Nevermind, it was related to #46576.

Moving the empty object check from isLoadingReportOnyx to shouldShowSkeleton fixes the loading issue for me.

1a49632

@rushatgabhane
Copy link
Member

cool, we can test it in the PR. it won't cause this bug - #43656 (comment) right?

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Sep 11, 2024
@CortneyOfstad
Copy link
Contributor

PR went into production 4 days ago, making the payment date Sept. 26! Changing the title now!

@CortneyOfstad CortneyOfstad changed the title [$250] CRITICAL: [UX Reliability] Old messages show up as most recent, and the new messages didn't load for several minutes [HOLD FOR PAYMENT 9/26] [$250] CRITICAL: [UX Reliability] Old messages show up as most recent, and the new messages didn't load for several minutes Sep 23, 2024
@CortneyOfstad
Copy link
Contributor

CortneyOfstad commented Sep 27, 2024

Okay, so this one is pretty lengthy with a lot of back and forth in terms of assignments. Please correct me if I am wrong (CC @cristipaval) but it looks as though the payment should be this:

@allroundexperts — to be paid $250 via NewDot (due to regression)
@kirillzyusko — not paid as they are contracted
@CyberAndrii — to be paid $250 via Upwork (offer sent here, please let me know once you accept!)
@rushatgabhane — to be paid $250 via NewDot

Does that look correct to everyone?

@CyberAndrii
Copy link
Contributor

Accepted the offer @CortneyOfstad

@CortneyOfstad
Copy link
Contributor

Since there are no objections or changes to the comment here, below is the payment summary!

Payment Summary

@allroundexperts — to be paid $250 via NewDot
@CyberAndrii — paid $250 via Upwork
@rushatgabhane — to be paid $250 via NewDot
@kirillzyusko — contracted position

@github-project-automation github-project-automation bot moved this from CRITICAL to Done in [#whatsnext] #quality Oct 1, 2024
@JmillsExpensify
Copy link

$250 approved for @allroundexperts

@garrettmknight
Copy link
Contributor

$250 approved for @rushatgabhane

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Development

No branches or pull requests