Skip to content

Commit 6b54a3a

Browse files
committed
getHaveServerData: Fix unsafe zulipVersion access causing crash on login
This fixes a regression introduced in 3b811d6 that made it impossible to log into a new account, by crashing just after completing authentication with the message "zulipVersion must be non-null". Oops: getHaveServerData was not an OK place to call getServerVersion. From the jsdoc: * This function assumes we have server data for this account, and if not it * may throw. […] So, abandon that selector and access zulipVersion safely. While I was at it, I double-checked `Account['zulipVersion']` and `Account['zulipFeatureLevel']` …and found that their jsdocs weren't right about when those fields were null. Fix that, copying some text from the jsdoc of `Account['userId']`.
1 parent cc53dd5 commit 6b54a3a

File tree

2 files changed

+19
-12
lines changed

2 files changed

+19
-12
lines changed

src/haveServerDataSelectors.js

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,7 @@
22

33
import type { GlobalState, PerAccountState } from './types';
44
import { getUsers } from './directSelectors';
5-
import {
6-
tryGetAuth,
7-
tryGetActiveAccountState,
8-
getServerVersion,
9-
} from './account/accountsSelectors';
5+
import { tryGetAuth, tryGetActiveAccountState, getAccount } from './account/accountsSelectors';
106
import { getUsersById } from './users/userSelectors';
117
import { kMinAllowedServerVersion } from './api/apiErrors';
128

@@ -119,9 +115,16 @@ export const getHaveServerData = (state: PerAccountState): boolean => {
119115
return false;
120116
}
121117

118+
const { zulipVersion } = getAccount(state);
119+
120+
// The server version comes with the rest of the server data.
121+
if (!zulipVersion) {
122+
return false;
123+
}
124+
122125
// We may have server data, but it would be from an ancient server that we
123126
// don't support, so it might be malformed.
124-
if (!getServerVersion(state).isAtLeast(kMinAllowedServerVersion)) {
127+
if (!zulipVersion.isAtLeast(kMinAllowedServerVersion)) {
125128
return false;
126129
}
127130

src/types.js

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,11 @@ export type Account = {|
8686
* Because a server deploy invalidates event queues, this means the value
8787
* is always up to date for a server we have an active event queue on.
8888
*
89-
* This is `null` just when representing an account which was last used on
90-
* a version of the app which didn't record this information. It's never
91-
* `null` for an account for which we have server data.
89+
* This is `null` briefly when we've logged in but not yet completed our
90+
* first initial fetch on the account. It's also `null` when representing
91+
* an account which was last used on a version of the app which didn't
92+
* record this information. It's never `null` for an account for which we
93+
* have server data.
9294
*
9395
* For use in:
9496
* * how we make some API requests, in order to keep the logic isolated
@@ -107,9 +109,11 @@ export type Account = {|
107109
* This is designed to provide a simple way for mobile apps to decide
108110
* whether the server supports a given feature or API change.
109111
*
110-
* This is `null` just when representing an account which was last used on
111-
* a version of the app which didn't record this information. It's never
112-
* `null` for an account for which we have server data.
112+
* This is `null` briefly when we've logged in but not yet completed our
113+
* first initial fetch on the account. It's also `null` when representing
114+
* an account which was last used on a version of the app which didn't
115+
* record this information. It's never `null` for an account for which we
116+
* have server data.
113117
*
114118
* Like zulipVersion, we learn the feature level from /server_settings
115119
* at the start of the login process, and again from /register when

0 commit comments

Comments
 (0)