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.

Sign up for GitHub

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

Jump to bottom

Logout gives endless loading screen #4723

Closed
gnprice opened this issue May 5, 2021 · 1 comment · Fixed by #4733
Closed

Logout gives endless loading screen #4723

gnprice opened this issue May 5, 2021 · 1 comment · Fixed by #4733
Labels
a-multi-org P1 high-priority

Comments

@gnprice
Copy link
Member

gnprice commented May 5, 2021 •
edited
Loading

This seems to reproduce reliably for me, on v27.162, on both iOS and Android. Not sure if it's new in v27.162 or was present earlier.

Repro steps:

  • I'm logged into an account.
  • I hit the "Log out" button. Get a confirmation modal, and confirm.

Expected result:

  • Navigate to the account-pick screen. During the nav transition animation, the old account's UI is replaced with a loading screen.

Actual result:

  • See just the loading screen -- no nav transition happening.
  • Sometimes, this goes on apparently indefinitely, at least for a good few seconds, until I kill the app. (Ken Clary observed this continuing for 30 seconds or so before killing the app.)
  • Sometimes, it turns into an error screen, the one our new React error boundary displays. The error message is "Error: Active account not logged in".

We probably want to change what the "Log out" button does entirely. But that isn't a trivial change to make, and I suspect that taking care of it will involve basically debugging this issue anyway -- the key thing that should be happening here and doesn't seem to be is the navigation, and we'll still want to make the same navigation after that change.
@chrisbobbe
Copy link
Contributor

chrisbobbe commented May 6, 2021 •
edited
Loading

From a bisect (luckily starting with the first and last commits in #4603) I think a80b4e8 may have introduced the issue; I'm looking into it.

EDIT: I think I've found a clue; discussion in chat.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 7, 2021
@chrisbobbe
AppNavigator: Stop changing what we pass for Stack.Screens' `compon…
d0ae7a6 
…ent`.

Before, whenever `AppNavigator` was called, a new value was passed
to these `Stack.Screen`s as `component`. That's because
`withHaveServerDataGate` returns a fresh component made by
react-redux's `connect`.

We found with a bisect that something in a80b4e8 was causing

Here, we stop letting that value change, and we see that it
fixes zulip#4603.

Alternatively, we might have put the `withHaveServerDataGate` call
in the same file as `MainTabsScreen`, etc. -- after all, that's our
usual practice for higher-order components. But the choice to put it
in AppNavigator (in a80b4e8 and f53d4f6) was intentional: we
wanted to make it easy to scan through and spot any that should be
treated with `withHaveServerDataGate` and aren't.

This approach lets us keep the calls in AppNavigator, but it adds
some ugly boilerplate. Ah, well.

We considered `useMemo` instead of `useRef`, but the
documentation [1] cautions against that for cases like these:

> **You may rely on useMemo as a performance optimization, not as a
> semantic guarantee.** In the future, React may choose to “forget”
> some previously memoized values and recalculate them on next
> render, e.g. to free memory for offscreen components. Write your
> code so that it still works without `useMemo` — and then add it to
> optimize performance.

So, use `useRef`.

[1] https://reactjs.org/docs/hooks-reference.html#useref

Fixes: zulip#4723
@chrisbobbe chrisbobbe mentioned this issue May 7, 2021
Merged
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 7, 2021
@chrisbobbe
AppNavigator: Stop changing what we pass for Stack.Screens' `compon…
08268a2 
…ent`.

Before, whenever `AppNavigator` was called, a new value was passed
to these `Stack.Screen`s as `component`. That's because
`withHaveServerDataGate` returns a fresh component made by
react-redux's `connect`.

We found with a bisect that something in a80b4e8 was causing

Here, we stop letting that value change, and we see that it
fixes zulip#4603.

Alternatively, we might have put the `withHaveServerDataGate` call
in the same file as `MainTabsScreen`, etc. -- after all, that's our
usual practice for higher-order components. But the choice to put it
in AppNavigator (in a80b4e8 and f53d4f6) was intentional: we
wanted to make it easy to scan through and spot any that should be
treated with `withHaveServerDataGate` and aren't.

This approach lets us keep the calls in AppNavigator, but it adds
some ugly boilerplate. Ah, well.

We considered `useMemo` instead of `useRef`, but the
documentation [1] cautions against that for cases like these:

> **You may rely on useMemo as a performance optimization, not as a
> semantic guarantee.** In the future, React may choose to “forget”
> some previously memoized values and recalculate them on next
> render, e.g. to free memory for offscreen components. Write your
> code so that it still works without `useMemo` — and then add it to
> optimize performance.

So, use `useRef`.

[1] https://reactjs.org/docs/hooks-reference.html#usememo

Fixes: zulip#4723
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 7, 2021
@chrisbobbe
AppNavigator: Stop changing what we pass for Stack.Screens' `compon…
a244e5c 
…ent`.

Before, whenever `AppNavigator` was called, a new value was passed
to these `Stack.Screen`s as `component`. That's because
`withHaveServerDataGate` returns a fresh component made by
react-redux's `connect`.

We found with a bisect that something in a80b4e8 was
causing zulip#4723.

Here, we stop letting that value change, and we see that it fixes
that issue.

Alternatively, we might have put the `withHaveServerDataGate` call
in the same file as `MainTabsScreen`, etc. -- after all, that's our
usual practice for higher-order components. But the choice to put it
in AppNavigator (in a80b4e8 and f53d4f6) was intentional: we
wanted to make it easy to scan through and spot any that should be
treated with `withHaveServerDataGate` and aren't.

This approach lets us keep the calls in AppNavigator, but it adds
some ugly boilerplate. Ah, well.

We considered `useMemo` instead of `useRef`, but the
documentation [1] cautions against that for cases like these:

> **You may rely on useMemo as a performance optimization, not as a
> semantic guarantee.** In the future, React may choose to “forget”
> some previously memoized values and recalculate them on next
> render, e.g. to free memory for offscreen components. Write your
> code so that it still works without `useMemo` — and then add it to
> optimize performance.

So, use `useRef`.

[1] https://reactjs.org/docs/hooks-reference.html#usememo

Fixes: zulip#4723
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
a-multi-org P1 high-priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix: Logout gives endless loading screen chrisbobbe/zulip-mobile
2 participants
@gnprice @chrisbobbe