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

Android - White screen displayed after log out #6932

Closed
kavimuru opened this issue Dec 29, 2021 · 12 comments
Closed

Android - White screen displayed after log out #6932

kavimuru opened this issue Dec 29, 2021 · 12 comments
Assignees

Comments

@kavimuru
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Launch the app
  2. Log in with any account
  3. Send some messages or request money
  4. Log out

Expected Result:

User should log out from the account and login screen appears

Actual Result:

White screen displayed after log out

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Android

Version Number: v1.1.24.0

Reproducible in staging?: Yes

Reproducible in production?: No

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

Notes/Photos/Videos:

Bug5390623_Screenshot_20211229-101859_New_Expensify

Bug5390623_20211229_172356.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause

Slack conversation:

View all open jobs on GitHub

@kavimuru kavimuru added the DeployBlockerCash This issue or pull request should block deployment label Dec 29, 2021
@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.

@MelvinBot
Copy link

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

@mananjadhav
Copy link
Collaborator

mananjadhav commented Dec 29, 2021

This has cropped up in #6898. The undefined check for the appStateSubscription is missing.

const appStateChangeSubscription = AppState.addEventListener('change', appStateChangeCallback);
return () => {
appStateChangeSubscription.remove();
};

Change it to:

if (!appStateChangeSubscription) {
    return;
}
appStateChangeSubscription.remove();

As I commented in slack, there's a follow up PR being worked on to upgrade RN which would be the ideal fix.

cc - @aswin-s @parasharrajat Since you guys were involved in the PR.

@aldo-expensify
Copy link
Contributor

Is it possible for AppState.addEventListener('change', appStateChangeCallback); to return undefined? I failed to find what it returns in the documentation :(

Documentation: https://reactnative.dev/docs/appstate#addeventlistener

@parasharrajat
Copy link
Member

Unfortunately, PR was merged before I was able to test it.
But we are planning to upgrade the RN to v65 and this problem will be resolved there. cc: @roryabraham

@aswin-s
Copy link
Contributor

aswin-s commented Dec 29, 2021

Thanks @parasharrajat. Yes as explained over here, removeEventListener was deprecated only in version 0.65 and we are still on 0.64.x. So the real fix is to upgrade react native version. I already tested @roryabraham's PR on iOS and everything looks good. We might want more eyes on that PR.

@mananjadhav
Copy link
Collaborator

@parasharrajat @roryabraham Are we going to merge the RN upgrade PR soon? If not then we should merge the null check PR so that it can be removed from DeployBlocker. If we'll merge soon then that's the ideal fix.

@aldo-expensify
Copy link
Contributor

Yes as explained over here, removeEventListener was deprecated only in version 0.65 and we are still on 0.64.x.

But... removeEventListener doesn't seem to be used here, the new way .remove() is being used.

Are we going to merge the RN upgrade PR soon

I don't think that the PR upgrading RN will be cherry picked to staging, so I agree with we should merge the null check PR so that it can be removed from DeployBlocker.

@aldo-expensify
Copy link
Contributor

Unfortunately, PR was merged before I was able to test it.

Which PR? #6898?

@parasharrajat
Copy link
Member

Yup. 6898.

@aldo-expensify
Copy link
Contributor

PR with fixed merged and CP'd to staging.

@mvtglobally
Copy link

Retested. Issue is Fixed.

@francoisl francoisl removed the DeployBlockerCash This issue or pull request should block deployment label Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants