Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.33k stars 2.76k forks source link

[$250] CRITICAL: [UX Reliability] Old messages show up as most recent, and the new messages didn't load for several minutes #43656

Open muttmuure opened 3 months ago

muttmuure commented 3 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: 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
    1. Bob replies in a thread to the message A, which creates a new chat in their LHN
    2. Alice and Bob then continue to chat in their DM and end the conversation with the message B
    3. Bob signs out and then signs in again
    4. Bob now sees the DM and the thread of the message A in the LHN
    5. The DM has the preview of the message B in the LHN
    6. 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?

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
cristipaval commented 1 month ago

@Kureev could you please give us an update?

melvin-bot[bot] commented 1 month ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

kirillzyusko commented 1 month ago

Hi, I am Kiryl from Margelo and I can work on this issue.

Feel free to re-assign this ticket on me πŸ™Œ

CortneyOfstad commented 1 month ago

I'm back from OoO @kadiealexander β€” thanks for holding down the fort!

CortneyOfstad commented 1 month ago

Still waiting for launch to production before closing!

CortneyOfstad commented 3 weeks ago

Still on staging πŸ‘

CortneyOfstad commented 3 weeks ago

Hit production 2 days ago! πŸŽ‰ New date is Aug. 26 based on 7-day holding period

CyberAndrii commented 3 weeks ago

https://github.com/Expensify/App/pull/47243 introduced a new regression: https://github.com/Expensify/App/pull/47338#issuecomment-2309712427

rushatgabhane commented 3 weeks ago

Hello, it'll be great if you could fix the regression with priority because it is crashing the app when using copilot

https://github.com/Expensify/App/pull/47338#issuecomment-2309712427

cristipaval commented 2 weeks ago

@kirillzyusko could you possibly look into it?

rushatgabhane commented 2 weeks ago

Steps to reproduce

https://github.com/Expensify/App/pull/47338#issuecomment-2308560242

kirillzyusko commented 2 weeks ago

@christipaval yes, I'll look into the problem and will try to fix it ASAP! πŸ™Œ

CyberAndrii commented 2 weeks ago

Edited by proposal-police: This proposal was edited at 2024-09-11 07:14:46 UTC.

Proposal

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

The app crashes when switching copilot accounts

What is the root cause of that problem?

It's a bug with withOnyx where it doesn't give the most up-to-date data. In this case, onyx returns undefined for lastVisitedPath.

https://github.com/Expensify/App/blob/f58439cfb116abd338dae880088a26725794108e/src/Expensify.tsx#L336-L338

This later causes the ReportScreen component to receive a route where the route.params.reportID property is set to an empty string.

Then reportIDFromRoute is assigned to route.params.reportID.

https://github.com/Expensify/App/blob/f58439cfb116abd338dae880088a26725794108e/src/pages/home/ReportScreen.tsx#L102

Next, we use the reportIDFromRoute variable to fetch the report data from Onyx.

https://github.com/Expensify/App/blob/f58439cfb116abd338dae880088a26725794108e/src/pages/home/ReportScreen.tsx#L118

However, because reportIDFromRoute is set to an empty string, useOnyx throws this error:

'reportNameValuePairs_991706764973694' key can't be changed to 'reportNameValuePairs_'. useOnyx() only supports dynamic keys if they are both collection member keys from the same collection e.g. from 'collection_id1' to 'collection_id2'.

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

We can check here if the report is loaded. If not, a skeleton component will be shown

const shouldShowSkeleton =
-    (isLinkingToMessage && !isLinkedMessagePageReady) || (!isLinkingToMessage && !isInitialPageReady) || isLoadingReportOnyx || !isCurrentReportLoadedFromOnyx || isLoading;
+    (isLinkingToMessage && !isLinkedMessagePageReady) || (!isLinkingToMessage && !isInitialPageReady) || isEmptyObject(reportOnyx) || isLoadingReportOnyx || !isCurrentReportLoadedFromOnyx || isLoading;
kirillzyusko commented 2 weeks ago

@CyberAndrii the proposal looks good to me! Do you want me to continue to work on this issue or will you fix it?

I'm currently trying to reproduce the problem, but I'm not getting emails and can not proceed further with reproduction πŸ€·β€β™‚οΈ

rushatgabhane commented 2 weeks ago

@cristipaval let's assign @CyberAndrii to fix it because they can reproduce the bug?

CyberAndrii commented 2 weeks ago

@kirillzyusko sure, will do.

I'm not getting emails

Could this be related to https://github.com/Expensify/App/issues/47979?

melvin-bot[bot] commented 2 weeks ago

Upwork job price has been updated to $250

CortneyOfstad commented 2 weeks ago

Adjusted the title due to the Regression πŸ‘

kirillzyusko commented 2 weeks ago

Could this be related to https://github.com/Expensify/App/issues/47979?

I think it's related to https://expensify.slack.com/archives/C049HHMV9SM/p1724645117851719

CyberAndrii commented 2 weeks ago

Hey @allroundexperts can you review my proposal?

CyberAndrii commented 2 weeks ago

@allroundexperts previous steps doesn't work anymore. Also I can't reproduce this in Safari so use Chrome. New steps:

  1. Enable newDotCopilot beta
  2. Go to an account that can act as a copilot for multiple accounts. Or you can add them via oldDot by following instructions on help site.
  3. Go to settings and click the account switcher
  4. Click an account

https://github.com/user-attachments/assets/7c38e985-f666-4d82-b5d5-a09d398c8fba

rushatgabhane commented 2 weeks ago

Let's assign @CyberAndrii

C+ reviewed πŸŽ€ πŸ‘€ πŸŽ€ https://github.com/Expensify/App/issues/43656#issuecomment-2310235866

melvin-bot[bot] commented 2 weeks ago

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

rushatgabhane commented 2 weeks ago

(and assign me)

melvin-bot[bot] commented 2 weeks ago

πŸ“£ @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 commented 2 weeks ago

assigned all the peeps!

dangrous commented 2 weeks ago

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!

rushatgabhane commented 6 days ago

@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 commented 6 days ago

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 commented 6 days ago

@rushatgabhane This line is causing the issue

https://github.com/Expensify/App/blob/a506fa44fd45d4948a9bfbd9c83b0a0312721133/src/libs/actions/Report.ts#L963

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 commented 6 days ago

which causes the backend to return a 403 error

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

CyberAndrii commented 5 days ago

Nevermind, it was related to https://github.com/Expensify/App/issues/46576.

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

https://github.com/Expensify/App/commit/1a496328e6083b896009c8cf3899827226f2f616

rushatgabhane commented 5 days ago

cool, we can test it in the PR. it won't cause this bug - https://github.com/Expensify/App/issues/43656#issuecomment-2338679611 right?