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

When signing into an account after signing out, you are brought to /r/ instead of a chat #3559

Closed
trjExpensify opened this issue Jun 11, 2021 · 7 comments · Fixed by #3634
Closed
Assignees
Labels
External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@trjExpensify
Copy link
Contributor

trjExpensify commented Jun 11, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Expected Result:

You are brought to a chat or LHN

Actual Result:

You are brought to /r/ (web / desktop), or an infinite spinner (ios / android / mweb)

Action Performed:

  1. Sign into an account
  2. Sign out of the account
  3. Sign into a different account

Workaround:

This can be worked around by refreshing the page or restarting the app

Platform:

All platforms

Version: versions after #3535 is merged
Notes/Photos/Videos: https://recordit.co/Iec1NeubJO
Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/167034
Upwork URL: https://www.upwork.com/jobs/~01ad93ad039876530e


View all open jobs on Upwork

@trjExpensify trjExpensify added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 11, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 11, 2021
@MelvinBot
Copy link

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

@trjExpensify trjExpensify added the External Added to denote the issue can be worked on by a contributor label Jun 11, 2021
@MelvinBot MelvinBot added the Daily KSv2 label Jun 11, 2021
@dklymenk
Copy link
Contributor

The issue is caused by a race condition of some kind. The ReportScreen in MainDrawerNavigator is rendered before the reports are fetched and since initialParams for that screen use last accessed report, the screen is rendered with reportID param equal to an empty string. This causes all sorts funny stuff like creating report_NaN, reportActions_NaN keys in local storage, but most importantly it means that the initial screen route will be /r/ instead of /r/{some_report_id}.

There already is a check in place to prevent that from happening, but it is not sufficient, because it checks intermediate data instead of reportID that is used to render the screen.

So my proposal is to replace _.size(props.reports) === 0 here:
https://github.com/Expensify/Expensify.cash/blob/b115b43292c9497d3ddeb747759da3107b3ad382/src/libs/Navigation/AppNavigator/MainDrawerNavigator.js#L45-L47

with !getInitialReportScreenParams(props.reports).reportID. This will ensure that as long as reportID is undefined or an empty string, the invalid route won't be set.

@isagoico
Copy link

We were able to reproduce this. I this is probably a deploy blocker since this is not reproducible in production. @trjExpensify let me know what you think
image

@johnmlee101
Copy link
Contributor

@dklymenk your solution looks good! cc @trjExpensify

@trjExpensify
Copy link
Contributor Author

Thanks, John! @dklymenk I've assigned the issue to you and sent the contract in Upwork. Feel free to proceed with submitting a PR for review. Thanks! 👍

@trjExpensify trjExpensify added Weekly KSv2 and removed Daily KSv2 labels Jun 16, 2021
dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jun 17, 2021
@dklymenk
Copy link
Contributor

Hello, the PR is ready - #3634

Thanks.

luacmartins added a commit that referenced this issue Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants