[PB-6681] change getUser to async - #2095
Conversation
Deploying drive-web with
|
| Latest commit: |
6197f49
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://03deb857.drive-web.pages.dev |
| Branch Preview URL: | https://get-user-async.drive-web.pages.dev |
sg-gs
left a comment
sg-gs
left a comment
There was a problem hiding this comment.
ℹ️ Just reviewed the network part.
| }, [isUserLoaded, user]); | ||
|
|
||
| if (!isUserLoaded || !user) return <></>; | ||
|
|
||
| useEffect(() => { | ||
| if (!user) { | ||
| const params = urlParams.toString(); |
There was a problem hiding this comment.
this will crash, remember to test all that changes. no add hooks after conditional returns https://react.dev/warnings/invalid-hook-call-warning#breaking-rules-of-hooks
There was a problem hiding this comment.
Was dead code; removed it
| const initializeThunk = createAsyncThunk<void, undefined, { state: RootState }>( | ||
| 'user/initialize', | ||
| async (_, { dispatch }) => { | ||
| const user = await encryptedStorageService.getUser(); | ||
| if (user) { | ||
| dispatch(userActions.setUser(user)); | ||
| } | ||
| dispatch(userActions.setIsUserInitialized(true)); | ||
| }, | ||
| ); | ||
|
|
There was a problem hiding this comment.
initializeThunk uses the same string ‘user/initialize’ as initializeUserThunk. As extraReducers are matched by action.type, the addCase(initializeUserThunk.pending/fulfilled/rejected, …) calls are also triggered when initializeThunk is executed in the bootstrap (index.tsx). If that fails, the error message is displayed and the user is redirected to /login for no real reason. Change the prefix of initializeThunk to something unique
There was a problem hiding this comment.
changed the prefix
|
|
||
| const handleGoToLogin = () => { | ||
| dispatch(userThunks.logoutThunk()); | ||
| authService.logOut(); |
There was a problem hiding this comment.
the effect of this logout is the same as the userThunks logout?
There was a problem hiding this comment.
Hmm, I revert this change just to be safe
| let sessionRef: VideoStreamingSession | undefined; | ||
| setupStreaming().then((session) => { | ||
| sessionRef = session; | ||
| }); | ||
|
|
||
| return () => { | ||
| session.destroy(); | ||
| sessionRef?.destroy(); | ||
| setCanPlay(false); |
There was a problem hiding this comment.
sessionRef is assigned within the .then() of setupStreaming(). If the effect is cleared (video change, unmount) before that promise resolves, the cleanup runs whilst sessionRef is still undefined and .destroy() is never called, consequently, the streaming session becomes orphaned. A ‘cancelled’ flag is needed to destroy the session if it is terminated prematurely
There was a problem hiding this comment.
Added canceled flag
| try { | ||
| metaService.trackPurchase(); | ||
| gaService.trackPurchase(); | ||
| await gaService.trackPurchase(); |
There was a problem hiding this comment.
No need to await this call. Can be resolved later.
There was a problem hiding this comment.
But the last line removePaymentsStorage(); cleans up payment storage and if trackPurchase puts data after that, it won't be cleaned, no?
|
All comments are addressed; QA passed
5 participants
Description
Makes getUser async and fully switches to the encrypted version of the user
Related Issues
Relates to PB-6681
Related Pull Requests
Checklist
Testing Process
Check that:
Additional Notes
PR on top of another PR because together it's too many changes