Check active account, too, in getHaveServerData#4961
Merged
chrisbobbe merged 4 commits intozulip:masterfrom Aug 16, 2021
Merged
Check active account, too, in getHaveServerData#4961chrisbobbe merged 4 commits intozulip:masterfrom
chrisbobbe merged 4 commits intozulip:masterfrom
Conversation
gnprice
added
a-redux
severe: crash
Member
Author
|
I tested manually that this resolves a possible crash:
Without these changes, the result was a crash upon logging in. (Also at the logout, step 2; but killing the app and restarting it got me to the account picker for step 3.) With these changes, the result was the loading screen followed by the normal main app UI. |
chrisbobbe
force-pushed
the
pr-check-have-account
branch
from
8961b40 to
3278eec
Compare
Some care is needed when adding checks here. Too much care, really; I'd love to find a way to arrange things to make these delicate properties be more automatically true. Pending that, add some comments to make the needed reasoning more explicit, so that at least it's easier to re-confirm without reasoning back through everything from scratch. While here, also add a pointer to the issue we more recently filed for the underlying problem this function exists to work around.
This isn't itself server data -- it's what we use for acquiring server data -- which is why we didn't cover it when expanding these checks in 916dc4b. But like the server data, it's something we assume is present when rendering the main UI of the app. And because of the same issue zulip#4841, the same lack of consistency in our data-storage model with redux-persist that caused zulip#4587 and made that fix necessary, it's possible for it to be lacking data when the existing checks in this function all pass. So, add a check for this too. Build on the comments added in the previous commit to reason through why this won't get us stuck at the loading screen; and add a comment at the otherwise inessential-looking bit of code that provides one of the assumptions we rely on for that reasoning. Also correct a related point in the comment at the end of the function.
There probably isn't any UI that would actually crash as a consequence of missing an API key for the active account. But certainly it's something we assume throughout the main UI, and it's easy to check for. So in addition to checking there's an active account at all, check it's logged in for good measure.
I don't see an immediate way we could check right now whether any server data we have is actually for the current active account, or for some other account (presumably for a previous active account, and left over due to our consistency-lacking data storage model.) The reasoning in the comment below this, about why it's mostly fine for this function not to spot inconsistencies between pieces of the server data, probably also holds between the server data and accounts. But there are also some plausible future changes, including at least one we actually intend to make soon anyway, that would make it easy to add some such checks, and at that point it'd be nice to do so. So add a comment about those.
chrisbobbe
force-pushed
the
pr-check-have-account
branch
from
3278eec to
9d71508
Compare
chrisbobbe
approved these changes
Aug 16, 2021
Contributor
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks, this LGTM! See some tiny comments below, which I've addressed before merging.
|
|
||
| import type { GlobalState, UserOrBot, Selector, User, UserId } from '../types'; | ||
| import { getUsers, getCrossRealmBots, getNonActiveUsers } from '../directSelectors'; | ||
| import { tryGetActiveAccount } from '../account/accountsSelectors'; |
Contributor
There was a problem hiding this comment.
This import isn't needed until the following commit.
| // subtrees but not others. See #4587 for an example, and #4841 overall. | ||
|
|
||
| // It's important that we never stick around in a state where we're trying | ||
| // to show the main UI but this function returns false, When in that |
Contributor
There was a problem hiding this comment.
Nit: period instead of comma
Member
Author
|
Thanks for the review and fixes! |
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This isn't itself server data -- it's what we use for acquiring server data -- which is why we didn't cover it when expanding these checks in 916dc4b / #4588.
But like the server data, it's something we assume is present when rendering the main UI of the app. And because of the same issue #4841, the same lack of consistency in our data-storage model with redux-persist that caused #4587 and made that fix necessary, it's possible for it to be lacking data when the existing checks in this function all pass.
So, add a check for this too.
Also expand the comments in this function, particularly on the tricky balance it has to maintain. Some care is needed when adding checks here -- too much care, really, and I'd love to find a way to arrange things to make these delicate properties be more automatically true. Pending that, add some comments to make the needed reasoning more explicit, so that at least it's easier to re-confirm without reasoning back through everything from scratch.