feat: add better handling when play key fails to fetch #275
Conversation
this could happen in the case where someone has no internet access or Slippi is down. Previously it would show the online activation step which was pretty confusing to users.
| loading: boolean; | ||
| }> = ({ uid, displayName, playKey, loading }) => { | ||
| }> = ({ uid, displayName, playKey, isServerError, loading }) => { | ||
| const isErrorText = isServerError || !playKey; |
There was a problem hiding this comment.
| const isErrorText = isServerError || !playKey; | |
| const showErrorText = serverError || !playKey; |
src/renderer/lib/hooks/useAccount.ts
Outdated
| .catch((err) => { | ||
| console.warn("Error fetching play key: ", err); | ||
| set({ playKey: null }); | ||
| set({ playKey: null, isServerError: true }); |
There was a problem hiding this comment.
Ok so we actually need to think of another way to do this. A user with a brand new account triggers this bit of code and thus they are never able to set their connect code.
There was a problem hiding this comment.
Huh interesting. In all likelihood I feel like an account without a play key should return a successful result with an empty play key, not an error. I can take a look tomorrow too.
There was a problem hiding this comment.
Addressed by ac47b41
The connectCode and private objects on getUser were null for a new user.
| @@ -100,12 +100,20 @@ export async function fetchPlayKey(): Promise<PlayKey> { | |||
|
|
|||
| handleErrors(res.errors); | |||
|
|
|||
| const connectCode = res.data.getUser?.connectCode?.code; | |||
| const playKey = res.data.getUser?.private?.playKey; | |||
| if (!connectCode || !playKey) { | |||
There was a problem hiding this comment.
| if (!connectCode || !playKey) { | |
| const displayName = res.data.getUser?.displayName; | |
| const latestVersion = res.data.getLatestDolphin?.version; | |
| if (!connectCode || !playKey || !displayName || !latestVersion) { |
The current use of optional chaining means that displayName and latestVersion can be undefined. This does not actually meet the PlayKey interface as defined:
export interface PlayKey {
uid: string;
playKey: string;
connectCode: string;
displayName: string;
latestVersion: string;
}So we should ensure those values also exist.
There was a problem hiding this comment.
I agree with this from a type perspective, but not a functionality perspective. If we fail to get the version, we shouldn't be telling the user that they need to initialize their connect code.
I'd rather add null as a possible type for latest version and handle that correctly in the code.
There was a problem hiding this comment.
Ah right. In that case the type needs to be updated to latestVersion: string | null
There was a problem hiding this comment.
If an optional chain like that fails, does the value become null or undefined? Do ts types differentiate between the two?
src/renderer/lib/slippiBackend.ts
Outdated
| connectCode: connectCode, | ||
| playKey: playKey, | ||
| displayName: res.data.getUser?.displayName, | ||
| latestVersion: res.data.getLatestDolphin?.version, |
There was a problem hiding this comment.
| connectCode: connectCode, | |
| playKey: playKey, | |
| displayName: res.data.getUser?.displayName, | |
| latestVersion: res.data.getLatestDolphin?.version, | |
| connectCode, | |
| playKey, | |
| displayName, | |
| latestVersion, |
This repo uses object shorthand notation so we should stick with that.
GQL is never type unsafe by definition. |
src/renderer/lib/hooks/useAccount.ts
Outdated
| @@ -11,6 +11,7 @@ export const useAccount = create( | |||
| user: null as firebase.User | null, | |||
| loading: false, | |||
| playKey: null as PlayKey | null, | |||
| serverError: false as boolean | null, | |||
There was a problem hiding this comment.
I don't see serverError ever being set to null or being set ambiguously, can we remove that extra type?
Previously when the user had no internet access or Slippi servers were down we would show the online activation step which was pretty confusing to users.
New behavior:
https://user-images.githubusercontent.com/1534726/153937946-e748898c-87fd-4f17-a0fc-5bd83fbbaacb.mp4