[HOLD for payment 2024-07-10] [$250] [CRITICAL] [UX Reliability] When deeplinking to expense report from email empty screen is shown

[mountiny]

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: 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: @danielrvidal Slack conversation:

Action Performed:

1. daniel@expensify.com submitted danrvidal+upgradetest7@gmail.com an IOU.
2. danrvidal+upgradetest7@gmail.com got the email and clicked Pay , is taken to the sign in page, gets the magic code and logs in
3. danrvidal+upgradetest7@gmail.com is taken to directly to the expense.
4. The subheader is not loading right away and LHN chats are not loading right away.
5. After letting it load, I click into the DM and there is nothing showing. a. I let it load for a bit, nothing. b. I refresh and see the expense show up.

This makes for a confusing experience. As a side note, we’ll be updating the flow so we don’t send users to the expense directly but rather the DM (GH) but I still experienced this bug so it felt like we should update it.

Expected Result:

The DM chat should load immediately as well as the chats should be available in LHN first, we should wait for the openApp response and show the skeleton if its not available

Actual Result:

• Subheader not showing when I’m brought into a report for the first time as a new user, and the DM the IOU sent to me is not loading.
• The DM chat is showing empty screen

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
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

In slack here https://expensify.slack.com/archives/C05LX9D6E07/p1716940223060999?thread_ts=1716939884.285849&cid=C05LX9D6E07

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0167ada75a878e8b10
• Upwork Job ID: 1795785341708906496
• Last Price Increase: 2024-05-29
• Automatic offers:
• c3024 | Reviewer | 102542821

Issue Owner

Current Issue Owner: @johncschuster

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0167ada75a878e8b10

[melvin-bot[bot]]

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

[Skakruk]

Proposal

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

Subheader not showing when user brought into a report for the first time as a new user, and the DM the IOU sent to me is not loading. Also reproducible for any user, who received IOU link, on the first app load (after clearing DB) when the user navigates to IOU report directly.

What is the root cause of that problem?

On the first app load, we're making OpenApp request which blocks any other updates to Onyx, until it finishes. https://github.com/Expensify/App/blob/15600b0d29b1bb745e0c2ea2cf7020bf6e240e96/src/libs/actions/OnyxUpdateManager/index.ts#L74-L80 Since OpenReport request is established before the OpenApp, the Report data is skipped and not saved into IndexedDB.

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

We need to delay OpenReport request until OpenApp is made.

In ReportScreen.tsx we add

const [isLoadingApp] = useOnyx(ONYXKEYS.IS_LOADING_APP);

and add checks for isLoadingApp in a few places:

1. don't make fetchReportIfNeeded() call https://github.com/Expensify/App/blob/21d7884c021fd9f8913d4f317d7d72bfdac9651c/src/pages/home/ReportScreen.tsx#L472-L480
2. for showing skeleton while the app is still loading https://github.com/Expensify/App/blob/21d7884c021fd9f8913d4f317d7d72bfdac9651c/src/pages/home/ReportScreen.tsx#L377
3. to condition shouldShowNotFoundPage for not showing "Report not found" https://github.com/Expensify/App/blob/21d7884c021fd9f8913d4f317d7d72bfdac9651c/src/pages/home/ReportScreen.tsx#L388

Also add condition

if (report.reportID === reportIDFromRoute) {
    return;
}

to https://github.com/Expensify/App/blob/21d7884c021fd9f8913d4f317d7d72bfdac9651c/src/pages/home/ReportScreen.tsx#L417 to make sure we don't make a duplicate request for already loaded report.

https://github.com/Expensify/App/assets/1768919/239b517d-c79b-4a56-959a-bb80e27d5143

[arosiclair]

I can manage this. Looks like we're having @kacper-mikolajczak take a look

[kacper-mikolajczak]

Hi! While trying to reproduce the issue I've gotten the infinite loading spinner on LHN with error message for the report:

https://github.com/Expensify/App/assets/62747088/b247b91c-41ac-4b85-820a-596990fb63a6

It hangs even after OpenApp call has ended. Is it a known issue?

[Skakruk]

@kacper-mikolajczak I got the same issue, and it also related to requests made prior the OpenApp

[kacper-mikolajczak]

Understandable, I've checked your approach and it looks fine. One thing that we could improve is making a OpenReport request in parallel to OpenApp but as you said, such change would not be easy with current architecture.

@mountiny What do you think about @Skakruk taking over the PR as he already proposed a solid solution?

[kacper-mikolajczak]

What also I've noticed while testing, we are making an OpenReport request even for user who was not logged in. Is it intended behaviour?

https://github.com/Expensify/App/assets/62747088/62dcf047-ed9b-4794-b9c4-538f7950f076

[Skakruk]

appears to be:

https://github.com/Expensify/App/blob/b4c7110dd7a6976c492aedd8c9a660e0076b3aad/src/libs/actions/Report.ts#L2487-L2489

[arosiclair]

@mountiny What do you think about @Skakruk taking over the PR as he already proposed a solid solution?

@Skakruk's proposal looks promising to me as well. Let's implement it 👍🏽

[melvin-bot[bot]]

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

Offer link Upwork job

[melvin-bot[bot]]

📣 @Skakruk You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

[arosiclair]

@Skakruk actually this is still up for discussion please hold.

[arosiclair]

Alright, upon some further discussion we realized this requires a backend fix to address. The root problem is that the subtitle in the header needs data from the parent report to be populated (that's done here) and OpenReport does not return that data. So we just need to include parent report data in the OpenReport response.

I'll take this internal and post a PR to implement that. @Skakruk appreciate your proposal but it actually won't be necessary anymore.

[Skakruk]

That’s strange, from the logging, I noticed that OpenApp is blocking and skipping any merges in Onyx until is processed. And changes to api might not fix the issue.

On Fri, May 31, 2024 at 19:48 Andrew Rosiclair @.***> wrote:

Alright, upon some further discussion we realized this requires a backend fix to address. The root problem is that the subtitle in the header needs data from the parent report to be populated (that's done here https://github.com/Expensify/App/blob/a51a245ac4a3f4cae0635e1564d530b6c186e3c4/src/libs/ReportUtils.ts#L3303) and OpenReport does not return that data. So we just need to include parent report data in the OpenReport response.

I'll take this internal and post a PR to implement that.

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/42753#issuecomment-2142635106, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANP3V6FMYVQCYMOJJNPOFLZFCSWJAVCNFSM6AAAAABIOYYDGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBSGYZTKMJQGY . You are receiving this because you were mentioned.Message ID: @.***>

[arosiclair]

To clarify, your RCA wasn't accurate. The log you're seeing here is referring to Onyx updates from Pusher not from API commands. API commands like OpenReport can merge onyx updates without issue before OpenApp completes.

[Skakruk]

From what I traced, API calls are saving data in Onyx via SaveResponseInOnyx, which calls saveUpdateInformation. Inside of it, there is Onyx.set(ONYXKEYS.ONYX_UPDATES_FROM_SERVER, updateParams); We have global listener https://github.com/Expensify/App/blob/15600b0d29b1bb745e0c2ea2cf7020bf6e240e96/src/libs/actions/OnyxUpdateManager/index.ts#L147-L152

Here it has callback handleOnyxUpdateGap which is referred to in the first place, that is blocking saving API response data into Onyx and IDB as a consequence.

On first app load (when OpenApp is called):

After page refresh (when OpenApp is not called):

[c3024]

I tried to repro the bug on dev a few times and this is what I have been seeing

3. danrvidal+upgradetest7@gmail.com is taken to directly to the expense.

I am not taken to the expense but seeing the not found page similar to this

4. The subheader is not loading right away and LHN chats are not loading right away.

LHN shows loading while the chats are loading with center pane showing the not found page.

5. After letting it load, I click into the DM and there is nothing showing. a. I let it load for a bit, nothing.

I think this is due to the issues in comment linking. I saw this a few times in other places as well.

https://github.com/Expensify/App/assets/102477862/46e1840c-c23a-4b7e-b45e-8848d9e11734

[arosiclair]

From what I traced, API calls are saving data in Onyx via SaveResponseInOnyx, which calls saveUpdateInformation. Inside of it, there is Onyx.set(ONYXKEYS.ONYX_UPDATES_FROM_SERVER, updateParams); We have global listener

Ah thanks I haven't seen that code flow before. This seems to only apply to writes which is where my confusion is coming from. Regardless, there's some missing data in OpenReport that we need to fix first.

I have not run into the not found page yet but I need to do some additional testing with OpenApp intentionally slowed down.

[arosiclair]

With a 10 second delay added to OpenApp I can also reproduce the Not Found screen. Some findings:

• Daniel's original bug is a little different from what I'm reproducing. You can see that his report clearly loads before OpenApp finishes. This should be impossible per this breakdown
• Adding thread ancestry to OpenReport's response helps populate the header but doesn't fix the NotFound page
• Preventing the NotFound page from appearing while ONYXKEYS.IS_LOADING_APP is true helps, but leaves a permanent skeleton loading page

This is very difficult to debug. @Skakruk's RCA actually seems accurate but the solution is a bit of a workaround. This problem of dropping Onyx updates while OpenApp is running is similar to the problem of dropping updates while we handle gaps. We fixed that with the deferred updates queue, so I think we should probably use the queue again in this scenario. I'm going to test queuing OpenReport's updates and see if that fixes the issue tomorrow.

[arosiclair]

Alright the deferred updates approach was very promising but ultimately doesn't work. The root problem is that we're performing a write command before OpenApp finishes and dropping the update. If we defer updates for the write command, we still end up ignoring them because the lastUpdateID from OpenApp will already include the lastUpdateID for the write command.

For example if the lastUpdateID for OpenReport is 50, OpenApp will definitely return a lastUpdateID >= 50. Since the clientLastUpdateID is now >= 50, we'll ignore the deferred OpenReport update (even when OpenApp doesn't include that report data).

So I think the best path forward is to block OpenReport on OpenApp. This might be a recurring problem for any write command you can deep link to, but for now OpenReport is the only problem I'm aware of so we can just patch that directly in the ReportScreen.

[Skakruk]

@arosiclair so what are the next actions?

[arosiclair]

@arosiclair so what are the next actions?

I'm drafting and testing changes both on the backend and frontend. The frontend changes are similar to your proposal so you'll get payment for that assuming everything pans out well.

[arosiclair]

To recap I'm solving two problems

1. OpenReport doesn't return parent report data for money requests
2. Deep linking to a report while signed out fails to correctly load the report

I posted https://github.com/Expensify/Auth/pull/11174 to solve 1️⃣

• This fixes the issue
• I removed a restriction on getting thread ancestry in DMs for this. This seemed to reveal a bug where clicking on parent comments from a thread leads to a not found page (the reason why it was restricted in the first place). I need to take a closer look at this before moving forward.

I posted https://github.com/Expensify/App/pull/43307 to solve 2️⃣

• This fixes the issue
• Seems to be causing repeated unnecessary calls to OpenReport. I need to look into this before posting the PR.

[johncschuster]

Thanks for that great update, @arosiclair!

[arosiclair]

Retested and addressed issues in https://github.com/Expensify/Auth/pull/11174. It's approved and ready to be merged as soon as we get approval from the fire room here.

No extra progress on problem 2️⃣ today. I'll continue work on that tomorrow.

[arosiclair]

EOD update:

• https://github.com/Expensify/Auth/pull/11174 is deployed to prod. 1️⃣ is done.
• On main, deep linking while signed out is even more broken now than before 🙃
  • After signing out, deep linking to a money request and signing in, OpenApp doesn't run and the app never loads
  • This is not reproducible on staging atm
  • The root of this problem is a call to OpenReport before OpenApp. An Onyx update gap with the response is detected which pauses the SequentialQueue and prevents OpenApp from running
  • I'm not sure what new change specifically is causing this

I'm now thinking this PR might not be the way to go for solving problem 2️⃣.

• To recap, it attempts to fix deep linking by blocking OpenReport until OpenApp completes
• This doesn't work reliably because we call these API commands when the AuthScreens and ReportScreen components mount. Since the ReportScreen is a descendent of AuthScreens, it will mount first and will not know if the app is being initialized.

Some ideas on a better solution

• Defer any write commands that are queued before OpenApp. This would probably need to be done at the API or SequentialQueue level
• Add some kind of interstitial "setup/initializing" page between the SignInPage and the LHN. The setup page would completely process OpenApp before navigating to the LHN and the initial ReportScreen

Unfortunately, I'm going OOO for the rest of the week. I'll be back on Monday to keep working on this if no progress is made.

[johncschuster]

Thanks for the update, @arosiclair! I'll keep Melvin at bay while you're OOO.

[melvin-bot[bot]]

@johncschuster @arosiclair @c3024 this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[johncschuster]

Yes, Melvin, Andrew is working on it.

[johncschuster]

Pre-emptive bump while @arosiclair is OOO 👍

[johncschuster]

And one more before the weekend 👍

[melvin-bot[bot]]

@johncschuster, @arosiclair, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[arosiclair]

Since this is critical functionality and https://github.com/Expensify/App/pull/43307 does work, I'm moving forward with it so we can get the fix out ASAP. I'll create a follow up to look into some of the better ideas I mentioned later. ETA on the PR is EOW.

[johncschuster]

@arosiclair how's this one going?

[arosiclair]

PR just hit staging yesterday.

[melvin-bot[bot]]

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

[johncschuster]

How's this one going, @arosiclair?

[arosiclair]

We're just waiting on the prod deploy

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/43307

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

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

• @c3024 requires payment automatic offer (Reviewer)

[melvin-bot[bot]]

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.
• [ ] [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[johncschuster]

Acknowledged! I will issue payment after the regression window 👍

[johncschuster]

@c3024 can you complete the BZ Checklist above when you get a moment? Thank you!

[c3024]

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:

• [x] [@c3024] The PR that introduced the bug has been identified. Link to the PR: No specific PR can be held responsible for this bug.
• [x] [@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
• [x] [@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: Not started as this bug cannot be caught earlier.
• [x] [@c3024] Determine if we should create a regression test for this bug. Yes
• [x] [@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. Test here in the author checklist Tests

1. Log into NewDot as user A
2. Verify the app and the initial report load without issue
3. Open a DM to user B
4. Submit an expense in the DM
5. Open the report and copy the link
6. Sign out
7. Open the link and sign in as user B
8. Verify
   • the app loads
   • the expense report loads
   • the header includes a link to the parent report