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

[Bug] No Offline Indicator displayed after going offline #10524

Closed
kbecciv opened this issue Aug 24, 2022 · 43 comments
Closed

[Bug] No Offline Indicator displayed after going offline #10524

kbecciv opened this issue Aug 24, 2022 · 43 comments
Assignees
Labels
Daily KSv2 Engineering Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Aug 24, 2022

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. Launch the app
  2. Log in with any account
  3. Go to any chat
  4. Go offline

Expected Result:

offline message displayed below after go offline

Actual Result:

No offline message displayed below after go offline

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • iOS
  • Android
  • Desktop App

Version Number: 1.1.89.0

Reproducible in staging?: Yes

Reproducible in production?: No

Email or phone of affected tester (no customers): any

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

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Aug 24, 2022
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Aug 24, 2022

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

@neil-marcellini
Copy link
Contributor

Not reproducible on iOS staging so I'll remove the deploy blocker label.

@sketchydroide
Copy link
Contributor

this might be duplicated with the one I just linked

@neil-marcellini
Copy link
Contributor

I'm unable to test on Android right now, it keeps crashing.

@neil-marcellini
Copy link
Contributor

I think this issue title and description is better so I'm going to close the other issue and update this one to include the desktop platform.

@neil-marcellini neil-marcellini changed the title IOS/Android - Offline - No offline message displayed below after go offline Offline - No offline message displayed below after go offline Aug 29, 2022
@neil-marcellini neil-marcellini changed the title Offline - No offline message displayed below after go offline Offline - No offline message displayed below after going offline Aug 29, 2022
@melvin-bot

This comment was marked as off-topic.

@neil-marcellini
Copy link
Contributor

I'm un-assigning and posting this to n7 in Slack so that someone can take it on with a higher priority. I'll also make it daily. This issue is causing a lot of QA to fail.

@melvin-bot
Copy link

melvin-bot bot commented Aug 30, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION 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 production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

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.

@grgia grgia self-assigned this Aug 30, 2022
@grgia
Copy link
Contributor

grgia commented Aug 30, 2022

Offline message seems to be working on staging web/mobile ios/desktop so far, will continue trying to replicate

@grgia
Copy link
Contributor

grgia commented Aug 30, 2022

v1.1.94-2 - iOS is good
v1.1.94-3 - Desktop is good
v1.1.94-3 - Android is good

@grgia grgia added the Needs Reproduction Reproducible steps needed label Aug 30, 2022
@PauloGasparSv PauloGasparSv changed the title Offline - No Offline Indicator displayed after going offline No Offline Indicator displayed after going offline Sep 26, 2022
@grgia grgia added Daily KSv2 and removed Weekly KSv2 labels Sep 27, 2022
@grgia
Copy link
Contributor

grgia commented Sep 27, 2022

Changing label to daily seeing as there's indication of a regression.

@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION 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 production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

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.

@JmillsExpensify JmillsExpensify changed the title No Offline Indicator displayed after going offline [Bug] No Offline Indicator displayed after going offline Sep 27, 2022
@melvin-bot melvin-bot bot added the Overdue label Sep 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2022

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

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@grgia
Copy link
Contributor

grgia commented Oct 3, 2022

I'm not able to reproduce the sign-on offline issue on staging/web/mobile. @neil-marcellini are you still able to? Otherwise, we can probably close out for now.
That said, I do still notice that when you log off while offline, you can't sign back in until you refresh or close/reopen the app (which I'm assuming is because of how we clear onyx values on sign-out). This doesn't happen when you go offline and online from the sign-on page otherwise, only after signing out while offline.

@melvin-bot melvin-bot bot removed the Overdue label Oct 3, 2022
@neil-marcellini
Copy link
Contributor

I'll see if I can still reproduce this.

That said, I do still notice that when you log off while offline, you can't sign back in until you refresh or close/reopen the app (which I'm assuming is because of how we clear onyx values on sign-out). This doesn't happen when you go offline and online from the sign-on page otherwise, only after signing out while offline.

That sounds like another bug, please create a separate issue for it.

@neil-marcellini
Copy link
Contributor

I can reproduce this.

Screen.Recording.2022-10-04.at.9.54.57.AM.mov

@neil-marcellini neil-marcellini removed the Needs Reproduction Reproducible steps needed label Oct 4, 2022
@neil-marcellini
Copy link
Contributor

After looking at the logs more, I don't think this has anything to do with clearing Onyx.

If I comment out this line then it works. I think the problem is that when we log out we stop listening to the NetInfo here, and when we log back in we never start listening again. I think we should always listen to the net info.

NetworkConnection.stopListeningForReconnect();

@melvin-bot melvin-bot bot added the Overdue label Oct 6, 2022
@grgia
Copy link
Contributor

grgia commented Oct 6, 2022

I'll see if I can replicate it today and if so, I can look more into that suggestion.

@grgia
Copy link
Contributor

grgia commented Oct 7, 2022

I met with Neil and this is what we discussed:

We use NetworkConnection functions to handle our listeners.
We start listening for Network Connection when we build the App here, which was why removing the listeners during sign-off required a refresh in order to see the indicators again.
For now, a simple solution in this PR is to not stop listening for Network Connection, since the offline indicator is used on the sign-in page.

@melvin-bot melvin-bot bot added the Overdue label Oct 10, 2022
@grgia
Copy link
Contributor

grgia commented Oct 10, 2022

Not Overdue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

8 participants