Fix "Tried to use NavigationService before appContainerRef was set". #4454
Conversation
chrisbobbe
added
severe: crash
chrisbobbe
force-pushed
the
pr-fix-navigation-service-crash
branch
from
c3373e0 to
b3b6259
Compare
|
@gnprice, I've just pushed a new revision, following our conversations yesterday (here and here). Thanks for your help!
|
chrisbobbe
force-pushed
the
pr-fix-navigation-service-crash
branch
2 times, most recently
from
c6ae80a to
2274394
Compare
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe for this fix! Just a few comments below.
src/message/fetchActions.js
Outdated
| // conditional accordingly, if we found out we're not depending on | ||
| // the more general condition; see | ||
| // https://github.com/zulip/zulip-mobile/pull/4274#discussion_r505941875 | ||
| if (getNavigationRoutes().some(navigationRoute => navigationRoute.name === 'loading')) { |
There was a problem hiding this comment.
Could this drop the some, and instead look just at the root route, i.e. getNavigationRoutes()[0]?
That would make the reasoning quoted in the commit message apply more directly -- it says
* The possible roots of the nav stack are main-tabs, loading,
account-pick, and realm-input. Those are the routes that we either
reset to, or set as the initial route.
* So we're talking about a possible change in behavior when the root
route is account-pick or realm-input.
after concluding the old logic is equivalent to "root route is not main-tabs". Implicitly it's using that the new logic is equivalent to "root route is loading", in order to say that the remaining two possible root routes are the only cases where the behavior might change.
There was a problem hiding this comment.
Yep, sounds right!
src/main/MainTabsScreen.js
Outdated
| // When our other `connect` callsites are gone, or at least the ones | ||
| // in `MainTabsScreen`'s descendants, we expect that we can safely | ||
| // switch this to a `useSelector`. |
There was a problem hiding this comment.
I think the conclusion is actually that we'll likely want to keep this here even in a future when all its descendants are using useSelector, because it's doing something useful for us that useSelector doesn't do: it interposes a new ReactReduxContext.Provider component, which proxies subscriptions so that the descendant components only rerender if this one continues to say their subtree should be kept around.
I guess right now it seems to break as long as a relevant descendant uses connect, and works if it's converted to useSelector too. After investigation, I wound up puzzled that useSelector would work here when connect doesn't:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/converting.20to.20Hooks/near/1111970
So clearly there is something I still don't understand. But, the job connect is doing for us here is one that makes sense to me as being a useful one.
There was a problem hiding this comment.
Ah OK, thanks! If I haven't got it quite right in this revision (which borrows heavily from your comment here), I'd appreciate it if you'd jump in and make any necessary changes to set the record straight. 🙂
In e181248, we forgot to change this instance of 'main' to 'main-tabs'. The symptom was that you could navigate somewhere in the app (e.g., to the message list) while the "Connecting..." banner was showing, then be interrupted when that banner disappeared by having the nav reset back to the home tab -- which was annoying.
chrisbobbe
force-pushed
the
pr-fix-navigation-service-crash
branch
from
2274394 to
34ae49d
Compare
|
Thanks for the review! Revision pushed. |
Greg validates this as follows [1]:
"""
* The main-tabs route can only be the root; we never push it onto
the stack, only reset to it or set it as initial. So the existing
logic is equivalent to checking just whether the root route is
main-tabs.
* The possible roots of the nav stack are main-tabs, loading,
account-pick, and realm-input. Those are the routes that we either
reset to, or set as the initial route.
* So we're talking about a possible change in behavior when the root
route is account-pick or realm-input.
* That case may indeed be possible -- either because of DEAD_QUEUE,
or because of navigation between when the initial fetch starts and
completes.
* In particular if when the app starts up, or you've just switched
accounts, you promptly go and hit "logout" before the initial
fetch completes, I think that will trigger this case.
* But if it does happen, it seems better to stick around in the flow
you're already in, rather than yank over to the main-tabs screen.
"""
[1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4453.20.22Tried.20to.20use.20NavigationService.20before.20appContaine.2E.2E.2E/near/1111824
And explain an annoying problem this solves.
…turn. This is a first step toward removing the 'loading' route, which will lead to some simplifications and a fix for an urgent bug. After this change, code that has been navigating to 'loading' can instead navigate to `MainTabsScreen`, and the user will still see the loading screen. See discussion [1]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4453.20.22Tried.20to.20use.20NavigationService.20before.20appContaine.2E.2E.2E/near/1111875
We've just made it safe to do this by having `MainTabsScreen` show the full-screen loading indicator if it doesn't have server data. Now, the time between LOGIN_SUCCESS/ACCOUNT_SWITCH and seeing the server data on MainTabsScreen is spent on MainTabsScreen itself, but in the branch that shows the loading indicator. Before, it was spent on LoadingScreen, which meant some confusing [1] logic about navigating to and from the 'loading' route. The 'loading' route ceases to be something we'd ever want or need to navigate to, and we'll remove it in an upcoming commit. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4453.20.22Tried.20to.20use.20NavigationService.20before.20appContaine.2E.2E.2E/near/1111808
The need for this was removed in a recent commit.
This is fine because we've just changed all the places where we were navigating to 'loading', to instead navigate to 'main-tabs'. This removes a `NavigationService` callsite that's been throwing quite a lot; that's zulip#4453. We still don't know why it's throwing those errors, and we haven't managed to reproduce them yet. But it means we should continue to prioritize removing `NavigationService`. See discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4453.20.22Tried.20to.20use.20NavigationService.20before.20appContaine.2E.2E.2E/near/1111875. Fixes: zulip#4453
It was previously a thunk action in order to ensure that a navigation action wouldn't be executed until after the store had rehydrated. But we've just stopped doing that navigation.
We recently stopped navigating to the 'loading' route at all, which means we can stop listing it in `AppNavigator`. When we do so, `LoadingScreen` loses all its former claims to a special status as a React Navigation screen component. So, remove `route` and `navigation` altogether from its props, rename the component, and put it in src/common along with our other loading-indicator components.
gnprice
force-pushed
the
pr-fix-navigation-service-crash
branch
from
34ae49d to
350ebcd
Compare
|
Merged, thanks! I made one fix, in an intermediate commit in some code a later commit deletes anyway: export const initialFetchComplete = () => async (dispatch: Dispatch, getState: GetState) => {
- if (getNavigationRoutes()[0] === 'loading') {
+ /* flowlint-next-line unnecessary-optional-chain:off */ // [0] may not exist
+ if (getNavigationRoutes()[0]?.name === 'loading') { |
|
Thanks for that catch! 🙂 |
2 participants
See discussion.
@gnprice, feel free to make any tweaks as you see fit (or ask me to, and I will). Also, let me know if you'd like to try a different approach, of course. 🙂
Fixes: #4453