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.14k stars 2.63k forks source link

[HOLD for payment 2021-12-15] [$4000] Web app does not connect on Chomebook device #4910

Closed isagoico closed 2 years ago

isagoico commented 2 years 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!


Action Performed:

  1. Connect Chromebook to internet via Instant Tether to a Pixel device
  2. On a web browser, navigate to new.expensify.com
  3. Notice that you are able to open the new.expensify.com page, however it does not recognize you as being online.

Expected Result:

User should be able to navigate the app in any device

Actual Result:

User is unable to use the webApp in Chromebook. Here's some extra info from @quinthar

Aha, I figured it out. Chromebook has two ways to connect, wifi, or some kind of "direct connect" to your Pixel phone. When you connect to wifi like normal, we recognize it. When you connect to wifi via the direct Pixel connect (not sure what it is -- bluetooth? quantum tunneling?), we do not recognize you as being online.

Workaround:

None

Platform:

Where is this issue occurring?

Version Number: 1.0.88-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation image

Expensify/Expensify Issue URL:

View all open jobs on GitHub


From @quinthar https://expensify.slack.com/archives/C01GTK53T8Q/p1629946809006100

Cannot go online with Chromebook. No errors. Just.... doesn't connect.

MelvinBot commented 2 years ago

Triggered auto assignment to @kadiealexander (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

isagoico commented 2 years ago

@kadiealexander this was my bad, I created the issue and it still had the AutoAssignerTriage label. Adding the engineering label now.

MelvinBot commented 2 years ago

Triggered auto assignment to @jasperhuangg (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

mallenexpensify commented 2 years ago

Outta curiosity I tested tethering from an iphone and didn't have any issues

jasperhuangg commented 2 years ago

I don't have a Chromebook to reproduce this error, so I'm going to unassign myself and reach out in #engineering-chat for an assist. My take is that we need to update how we detect a user's offline status to account for "Pixel Connect"?

roryabraham commented 2 years ago

This feels more like a weekly to me.

MelvinBot commented 2 years ago

Eep! 4 days overdue now. Issues have feelings too...

jasperhuangg commented 2 years ago

Demoting to a weekly since this seems like an uncommon edge case.

MelvinBot commented 2 years ago

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

stephanieelliott commented 2 years ago

Posted job to Upwork: https://www.upwork.com/jobs/~01855e280c1fa3ca00

MelvinBot commented 2 years ago

Triggered auto assignment to @marcaaron (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

marcaaron commented 2 years ago

I also do not have a Chromebook so unsure how to verify if any proposals work or not.

marcaaron commented 2 years ago

I’m thinking we can maybe just set up a tunnel to a local build and have someone who does have a Chromebook test any proposals that come in? Really could be anyone with a Chromebook just signing into an ngrok web build or something.

mallenexpensify commented 2 years ago

Doubled price to $500 a day early cuz I'm in here for the Weekly resync

quinthar commented 2 years ago

I'm happy to test and confirm. Also, the title seems a bit off -- it does open, it just doesn't connect.

stephanieelliott commented 2 years ago

This is on hold while we prioritize N6 issues.

marcaaron commented 2 years ago

Still on hold

kadiealexander commented 2 years ago

Please refer to this post for updated information on the n6 hold, we've raised the bonus to $250 for all issues where a PR is created before the N6 hold is lifted.

stephanieelliott commented 2 years ago

Still on hold for N6

stephanieelliott commented 2 years ago

The original Upwork post expired during the N6 hold period. Just re-posted to Upwork and doubled to $500: https://www.upwork.com/jobs/~011b7f8abcaf97ca2f

stephanieelliott commented 2 years ago

Price doubled again, now $1,000.

stephanieelliott commented 2 years ago

This has been open for a few months now with no proposals, took a swing at updating the issue description to make the problem a bit more clear. I think the "pixel direct connect" is the Instant Tether feature described here, which would mean issue occurs only when using a Chromebook which is tethered to a Pixel.

Updated the action steps, @quinthar does this accurately describe what you are running into:

  1. Connect Chromebook to internet via Instant Tether to a Pixel device
  2. On a web browser, navigate to new.expensify.com
  3. Notice that you are able to open the new.expensify.com page, however it does not recognize you as being online.
mallenexpensify commented 2 years ago

We are placing all non-critical issues on hold while we're on a company offsite. The hold is expected to be lifted on 11/19 (but could go longer). For any PRs that are submitted before or during the hold, we will add a $250 bonus payment.

kadiealexander commented 2 years ago

Doubled the job price again on behalf of the team here.

stephanieelliott commented 2 years ago

Doubled to again, to $4,000!

eVoloshchak commented 2 years ago

I was able to reproduce this issue using HP Chromebook 11 G6 and OnePlus 6

We use @react-native-community/netinfo to retrieve information abount current network state. For browser this library uses Network Information API . We are iterested in this file. As you can see, it correctly uses navigator interface to see if we are connected to the internet: const isConnected = navigator.onLine; When we are connected via Instant Tethering, connection type is 'unknown'. The problem occured because react-native-netinfo treats connection of type 'unknown' as not connected, no matter what the real connection state is. You can see this problem exactly here

 else if (type === NetInfoStateType.unknown) {
    const state: NetInfoUnknownState = {
      ...baseState,
      isConnected: false,
      isInternetReachable: false,
      type,
      details: null,
    };
    return state;
  }

If connection has an 'unknown' type, isConnected will always be false.

So my proposal is:

1) Fork react-native-netinfo 2) Replace the code i was talking about earlier with

 else if (type === NetInfoStateType.unknown) {
    const state: NetInfoUnknownState = {
      ...baseState,
      isConnected: isConnected,
      isInternetReachable: false,
      type,
      details: null,
    };
    return state;
  }

That way if connection type is 'unknown', we will still get the real network state in 'isConnected' property

I've already forked this library and applied the described fix and it works as expected. Attaching videos for demonstration:

https://user-images.githubusercontent.com/9059945/142476970-77e40c6e-d884-4411-afb0-b20324d1d7d5.mp4

https://user-images.githubusercontent.com/9059945/142477076-22aae72c-eb69-459b-bd64-05c1087045c4.mp4

mallenexpensify commented 2 years ago

Thanks @eVoloshchak . @marcaaron is out of the office til 11/22, I'm going to see if I can get another CME (Contributor Manager Engineer) to review

Julesssss commented 2 years ago

Nice work @eVoloshchak.

My only concern is that there might be a valid reason for isConnected to default to false in this case. However, the only info I could find on this was an unanswered question from our own @marcaaron.

Julesssss commented 2 years ago

Actually, as PRs seem to be more active, let's go ahead and try to submit the change as a PR on the original library. If they reject or ignore the change then we can use your fork instead.

Does that sound okay to you @eVoloshchak? If so, I'm happy to approve the proposal.

Julesssss commented 2 years ago

Okay, it sounds like we've got the go-ahead to submit a PR! The authors have specifically asked that we include some release notes that explain the change so that other users are aware that the behavior of NetInfoStateType.unknown has changed.

Are you happy to submit a few sentences along with the PR @eVoloshchak? We can help with this too if necessary.

eVoloshchak commented 2 years ago

Okay, it sounds like we've got the go-ahead to submit a PR! The authors have specifically asked that we include some release notes that explain the change so that other users are aware that the behavior of NetInfoStateType.unknown has changed.

Are you happy to submit a few sentences along with the PR @eVoloshchak? We can help with this too if necessary.

1) We are using version 5.9.10, while the latest is 7.1.2. Do i submit a PR and we migrate to the latest version? 2) I'm leaving my country in a couple of hours and won't have access to computer till november 27. I'll have a phone, but that is not sufficient to test the changes. Could this wait or Is this critical? If that is an issue, I could try and find one to rent/buy

stephanieelliott commented 2 years ago

Thanks @eVoloshchak! Hired you on the job in Upwork.

I'm leaving my country in a couple of hours and won't have access to computer till november 27.

This issue is non-critical, I think this is fine to wait and test changes once you return. Thanks!

Julesssss commented 2 years ago
  1. We are using version 5.9.10, while the latest is 7.1.2. Do I submit a PR and we migrate to the latest version?

Yeah, we should make a note on the PR so that we can pay attention to these additional version changes when testing. But if any issues arise here then don't worry, we can treat those as a separate issue.

  1. I'm leaving my country in a couple of hours and won't have access to computer till november 27. I'll have a phone, but that is not sufficient to test the changes. Could this wait or Is this critical? If that is an issue, I could try and find one to rent/buy

As Stephanie said, the delay is no problem.

MelvinBot commented 2 years ago

📣 @eVoloshchak You have been assigned to this job by @jliexpensify! Please apply to this job in Upwork and let us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

eVoloshchak commented 2 years ago

Submitted a pull-reqest to react-native-netinfo

Julesssss commented 2 years ago

Nice, I'll follow along with the PR review. It seems like you've explained the changes well.

eVoloshchak commented 2 years ago

The PR was merged and is now included in version 7.1.3. Do I create a PR for the app with @react-native-community/netinfo version changed from ^5.9.10 to ^7.1.3 in package.json?

Julesssss commented 2 years ago

@eVoloshchak yes, please

kidroca commented 2 years ago

Here's a list of issues that might be affected positively by the update of netinfo that was merged:

Might be that the updated netinfo library has a workaround for navigator.onLine being incorrectly reported as false ?

eVoloshchak commented 2 years ago

Here's a list of issues that might be affected positively by the update of netinfo that was merged:

Might be that the updated netinfo library has a workaround for navigator.onLine being incorrectly reported as false ?

I'm guessing the root cause of both of these issues is that the library does not detect if internet is actually reachable, it just relies on device being connected to some kind of a network. I was able to reproduce the first issue with the updated netinfo and it seems that the second issue is the same as the first (but instead of not detecting that internet is gone, it is not detecting, that internet is available again).

There is an issue describing this problem, but it wasn't resolved. I've tried to play around with reachability timeout of NetInfo's configure settings, but that didn't help.

I would propose setting up some sort of a task that would ping some endpoint periodickally, similar to what is described in this comment. We could even use the same endpoint library is using: https://clients3.google.com/generate_204

UPD So it seems that react-native-netinfo actually tries to periodickally make a call to https://clients3.google.com/generate_204, which obviously fails, if we turn off mobile data. However, that just results in error being displayed in console, while network state listener does not get called and network state is not updated. Screenshot from 2021-12-01 14-29-08

I can take a look at why library does not hadle this requests failing if this approach is something we can use.

botify commented 2 years ago

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

If no regressions arise, payment will be issued on 2021-12-15. :confetti_ball:

mallenexpensify commented 2 years ago

Apologies for the delay @eVoloshchak , I just paid you $4000 in Upwork! Thanks for the help!