Stop using fake blank data at login; and restore #4040 #4078
Conversation
This file contains 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
Empirically these aren't true. To see that:
* Delete this "fake blank state" hack.
* Try logging into a new account. (E.g., log out and then log in
again.) Observe the red-screen error:
Error: Error: Error: No server data found
This error is located at:
in Connect(UnreadCards) (at HomeTab.js:66)
in RCTView (at View.js:45)
in View (at HomeTab.js:37)
in HomeTab (created by Connect(HomeTab))
[...]
Inspecting the code that throws that error, it's caused by
`state.realm.email` being undefined.
* Now instead try switching between accounts, or logging out of an
account. Observe that it works fine with no error.
We're about to fix the remaining, LOGIN_SUCCESS issue too -- so
that means we'll get to remove this hack entirely.
The app's main UI relies on actually having data from the server -- after all, without that there's not much useful it could do anyway -- so we should wait to show that UI until we have that data. The visible effect of this is that on login, instead of the broken-looking state where the lower-right corner where a tab icon should be is missing/blank (because it's meant to be the user's avatar, and we don't know their avatar URL yet) and we wrongly say there are no unreads, we just show the loading screen until we're ready to show the real data. This is already what we do in the case where the user is switching from one account to another. There's no reason we should think we're *more* ready to show the main UI when it's an account we weren't even logged into until just now. In fact the only reason this hasn't been a live crashing bug for a long time is that we have in the realm reducer a hack to provide a couple morsels of fake data in this case, causing much of the UI to simply show nonsense (like that blank avatar) instead of crashing. We'll get to take that hack out next. Fixes: zulip#4077
We've just fixed the navigation issue this comment describes, so now we get to cut out this hack that (as explained at zulip#4077) mitigated it.
The point of this state subtree is to carry miscellaneous data from the server; it doesn't retain any state across a `/register` call, and if it did that would probably be a bug. Drop the spread of the old state, to make that explicit and to get the type-checker to check it for us.
In the type definition and its jsdoc, they're listed in a logical order based on their different semantics. Use that in the reducer file too.
This will let us switch to passing one of these helpers a user ID in the next commit. Also fill in some missing pieces of data in these ill-typed tests, without which they start failing. [This is a cherry-pick of 79c8671, which was reverted in 084d726 because it triggered zulip#4077, causing crashes on login.]
gnprice
added
a-onboarding
chrisbobbe
reviewed
Apr 30, 2020
| @@ -35,7 +35,6 @@ export default (state: RealmState = initialState, action: Action): RealmState => | |||
|
|
|||
| case REALM_INIT: { | |||
| return { | |||
| ...state, | |||
There was a problem hiding this comment.
Since this involves spreading a value and type-checking in a reducer, I was vaguely reminded of this Flow bug.
I verified empirically that it isn't in play after this commit. (Making a property undefined trips Flow.) The next commit helps verify that everything's working as expected by reading the code. So in this case it wasn't necessary to go re-digest what exactly that Flow bug is.
There was a problem hiding this comment.
On re-scanning the Flow issue Ray linked to, it sounds plausible that maybe the bug was happening before this commit? Anyway, the important thing is that it's not happening after it. 😆
There was a problem hiding this comment.
This PR looks good to me, but Ray may have more to say.
Nope! 🙂 Merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 fixes #4077, which was the root cause of the crash bug brought out by #4040; then restores the changes made originally in #4040.
See background in #4077 and details in commit messages.