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-05-30] HIGH: Fix extraneous ReconnectApp on refresh #40995

Closed
quinthar opened this issue Apr 25, 2024 · 28 comments
Closed
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@quinthar
Copy link
Contributor

quinthar commented Apr 25, 2024

Being discussed here: https://expensify.slack.com/archives/C049HHMV9SM/p1714059210537139

Often occurs in conjunction with HIGH: [Reliability] [$500] Multiple calls to GetNewerActions on report open and HIGH: [API Reliability] [$250] Multiple calls to OpenReport on signin, but seems to be a different root cause.

Actions Performed:

  1. Go to staging.new.expensify.com and login
  2. Open any report
  3. Refresh the page
  4. Observe the API calls for the OpenApp ReconnectApp and OpenReport

Expected Result:

There should be only one ReconnectApp OpenReport OpenApp calls

Actual Result:

Multiple calls for ReconnectApp OpenReport

Platforms:

Mac OS: Chrome/Safari

Screenshots/Videos:

image (15)

image (14)
image (13)

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01008cff119674ebe1
  • Upwork Job ID: 1787505314359492608
  • Last Price Increase: 2024-05-06
Issue OwnerCurrent Issue Owner: @dylanexpensify
@quinthar quinthar converted this from a draft issue Apr 25, 2024
@quinthar quinthar changed the title HIGH: [Reliability] Fix extraneous ReconnectApp/OpenReport/GetNewerActions on refresh HIGH: [Reliability] Fix extraneous ReconnectApp and OpenReport on refresh Apr 25, 2024
@gedu
Copy link
Contributor

gedu commented Apr 25, 2024

Hey Edu here from Callstack

@gedu
Copy link
Contributor

gedu commented Apr 26, 2024

Updates on the OpenReport call case

Problems

There are 2 cases that I’m trying to prevent:

  1. Multiple calls given different scenarios, happy path (opening the report) and multiple ReportScreen creation (navigating to the report, maybe a refresh) all happening at the same time

  2. Updates on the multiple created ReportScreen, changing to another Report all reports gets updated

Solution

  1. I’m storing a set of the ReportsID calling the openReport so if the reportID is already fetching no need to call it again -> it gets removed when the response comes back

  2. I’m checking if the url reportID matches the active reportID (there always 1 report active), if it is active I can call the openReport

At least for now seems to be working, I'm testing and soon I can make a formal proposal or a PR if you want

@quinthar
Copy link
Contributor Author

I don't think that's the right solution -- we don't want to suppress multiple calls, we want to figure out why the code is even attempting to make multiple calls, and stop it.

@quinthar quinthar changed the title HIGH: [Reliability] Fix extraneous ReconnectApp and OpenReport on refresh HIGH: [API Reliability] Fix extraneous ReconnectApp and OpenReport on refresh Apr 27, 2024
@quinthar
Copy link
Contributor Author

quinthar commented Apr 27, 2024

actually, is this a dupe of: #39673? Can we close this?

@quinthar
Copy link
Contributor Author

Actually, the OpenReport portion of this seems like a dupe; let's reduce the scope of this issue to just be fixing the extraneous calls to ReconnectApp, ok @gedu ?

@quinthar quinthar changed the title HIGH: [API Reliability] Fix extraneous ReconnectApp and OpenReport on refresh HIGH: [API Reliability] Fix extraneous ReconnectApp on refresh Apr 27, 2024
@gedu
Copy link
Contributor

gedu commented Apr 29, 2024

@quinthar ok, The "suppress" is mainly because we create multiple ReportScreen, and even when you switch a report I saw OpenReport being called multiple times.
I will take a look at ReconnectApp

@gedu
Copy link
Contributor

gedu commented Apr 29, 2024

@quinthar
Looking at the code, when the site gets refreshed first it does the normal walk through, checking if the user was logged in already, which is true so it does a reconnectApp

Problem

On the other hand, the app register to the Network status, when it is registered it receives updates
NetwokrConnection.ts file subscribeToNetInfo function

NetInfo.addEventListener((state) => {
   // code
   setOfflineStatus((state.isInternetReachable ?? false) === false);
   });

Given that isInternetReachable is null for a couple of calls before getting the right value, it triggers the reconnection.
Screenshot 2024-04-29 at 12 11 31

Solution

Given that the Lib for web is still experimental and in the description for isInternetReachable if it is unknown is null. Also checking the code for web they send basic information until they collect the real

 const baseState = {
    isInternetReachable: null,
  };

  // If we don't have a connection object, we return minimal information

I suggest to remove the ?? false because we only care when the isInternetReachable has a value and the Lib has created a connection object with the data in it.

@melvin-bot melvin-bot bot added the Monthly KSv2 label Apr 29, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Monthly KSv2 labels May 2, 2024
@jjcoffee
Copy link
Contributor

jjcoffee commented May 6, 2024

I'm temporarily out of the C+ team as I'm about to go OOO, so the auto-assigning isn't working here. Tagging @tgolen on the PR to give it a quick review, otherwise once a BZ is assigned maybe they can find someone?

@mallenexpensify mallenexpensify added Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff labels May 6, 2024
Copy link

melvin-bot bot commented May 6, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01008cff119674ebe1

Copy link

melvin-bot bot commented May 6, 2024

Triggered auto assignment to Contributor Plus for review of internal employee PR - @s77rt (Internal)

Copy link

melvin-bot bot commented May 6, 2024

Triggered auto assignment to @dylanexpensify (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 melvin-bot bot added the Daily KSv2 label May 6, 2024
@quinthar quinthar changed the title HIGH: [API Reliability] Fix extraneous ReconnectApp on refresh HIGH: Fix extraneous ReconnectApp on refresh May 12, 2024
@mountiny mountiny self-assigned this May 13, 2024
@dylanexpensify
Copy link
Contributor

Hiya! @mallenexpensify mind confirming what's needed here?

@jjcoffee
Copy link
Contributor

@dylanexpensify The PR hasn't hit staging yet, but eventually payment will be due for C+ review.

@muttmuure
Copy link
Contributor

What's the next step here? Who's doing it? What is the ETA?

@muttmuure
Copy link
Contributor

So we just need to pay this out?

@mountiny mountiny added Weekly KSv2 and removed Daily KSv2 labels May 20, 2024
@mountiny
Copy link
Contributor

Yep this was only deployed to staging so far though

@dylanexpensify
Copy link
Contributor

Still waiting then!

@dylanexpensify
Copy link
Contributor

Hit prod on 23rd, tomorrow will pay out!

@mountiny mountiny added the Awaiting Payment Auto-added when associated PR is deployed to production label May 29, 2024
@mountiny mountiny changed the title HIGH: Fix extraneous ReconnectApp on refresh [HOLD for payment 2024-05-30] HIGH: Fix extraneous ReconnectApp on refresh May 29, 2024
Copy link

melvin-bot bot commented May 30, 2024

Payment Summary

Upwork Job

  • Contributor: @gedu is from an agency-contributor and not due payment
  • ROLE: @jjcoffee paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@dylanexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1787505314359492608/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@jjcoffee
Copy link
Contributor

jjcoffee commented Jun 4, 2024

@dylanexpensify Friendly bump for payment 🙇

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 4, 2024
@dylanexpensify
Copy link
Contributor

Sorry, on it now! Thanks @jjcoffee!

@dylanexpensify
Copy link
Contributor

Invited to apply!

@jjcoffee
Copy link
Contributor

jjcoffee commented Jun 5, 2024

@dylanexpensify Thank you, I've applied! 🙏

@jjcoffee
Copy link
Contributor

@dylanexpensify Friendly bump 🙇

@dylanexpensify
Copy link
Contributor

@jjcoffee sent offer, sorry!

@jjcoffee
Copy link
Contributor

@dylanexpensify No worries! I've accepted the offer now.

@dylanexpensify
Copy link
Contributor

done!

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. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
Status: Done
Development

No branches or pull requests

8 participants