[Session] Align state and global agent switchpoints (#3845)

* Adopt synced accounts unconditionally * Remove try/catch around resuming session * Move to login form on resume failure * Restructure code flow for easier reading --------- Co-authored-by: Eric Bailey <[email protected]>
bluesky-social · May 3, 2024 · 4a2d425 · 4a2d425
1 parent 85b3441
commit 4a2d425
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 70 deletions.
diff --git a/src/lib/hooks/useAccountSwitcher.ts b/src/lib/hooks/useAccountSwitcher.ts
@@ -15,7 +15,7 @@ export function useAccountSwitcher() {
   const [pendingDid, setPendingDid] = useState<string | null>(null)
   const {_} = useLingui()
   const {track} = useAnalytics()
-  const {initSession, clearCurrentAccount} = useSessionApi()
+  const {initSession} = useSessionApi()
   const {requestSwitchToAccount} = useLoggedOutViewControls()
 
   const onPressSwitchAccount = useCallback(
@@ -53,19 +53,11 @@ export function useAccountSwitcher() {
         logger.error(`switch account: selectAccount failed`, {
           message: e.message,
         })
-        clearCurrentAccount() // back user out to login
       } finally {
         setPendingDid(null)
       }
     },
-    [
-      _,
-      track,
-      clearCurrentAccount,
-      initSession,
-      requestSwitchToAccount,
-      pendingDid,
-    ],
+    [_, track, initSession, requestSwitchToAccount, pendingDid],
   )
 
   return {onPressSwitchAccount, pendingDid}

diff --git a/src/screens/Login/ChooseAccountForm.tsx b/src/screens/Login/ChooseAccountForm.tsx
@@ -39,31 +39,33 @@ export const ChooseAccountForm = ({
         // The session API isn't resilient to race conditions so let's just ignore this.
         return
       }
-      if (account.accessJwt) {
-        if (account.did === currentAccount?.did) {
-          setShowLoggedOut(false)
-          Toast.show(_(msg`Already signed in as @${account.handle}`))
-        } else {
-          try {
-            setPendingDid(account.did)
-            await initSession(account)
-            logEvent('account:loggedIn', {
-              logContext: 'ChooseAccountForm',
-              withPassword: false,
-            })
-            track('Sign In', {resumedSession: true})
-            Toast.show(_(msg`Signed in as @${account.handle}`))
-          } catch (e: any) {
-            logger.error('choose account: initSession failed', {
-              message: e.message,
-            })
-            onSelectAccount(account)
-          } finally {
-            setPendingDid(null)
-          }
-        }
-      } else {
+      if (!account.accessJwt) {
+        // Move to login form.
         onSelectAccount(account)
+        return
+      }
+      if (account.did === currentAccount?.did) {
+        setShowLoggedOut(false)
+        Toast.show(_(msg`Already signed in as @${account.handle}`))
+        return
+      }
+      try {
+        setPendingDid(account.did)
+        await initSession(account)
+        logEvent('account:loggedIn', {
+          logContext: 'ChooseAccountForm',
+          withPassword: false,
+        })
+        track('Sign In', {resumedSession: true})
+        Toast.show(_(msg`Signed in as @${account.handle}`))
+      } catch (e: any) {
+        logger.error('choose account: initSession failed', {
+          message: e.message,
+        })
+        // Move to login form.
+        onSelectAccount(account)
+      } finally {
+        setPendingDid(null)
       }
     },
     [

diff --git a/src/state/session/index.tsx b/src/state/session/index.tsx
@@ -337,35 +337,22 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
       if (isSessionExpired(account)) {
         logger.debug(`session: attempting to resume using previous session`)
 
-        try {
-          const freshAccount = await resumeSessionWithFreshAccount()
-          __globalAgent = agent
-          await fetchingGates
-          setState(s => {
-            return {
-              accounts: [
-                freshAccount,
-                ...s.accounts.filter(a => a.did !== freshAccount.did),
-              ],
-              currentAgentState: {
-                did: freshAccount.did,
-                agent: agent,
-              },
-              needsPersist: true,
-            }
-          })
-        } catch (e) {
-          /*
-           * Note: `agent.persistSession` is also called when this fails, and
-           * we handle that failure via `createPersistSessionHandler`
-           */
-          logger.info(`session: resumeSessionWithFreshAccount failed`, {
-            message: e,
-          })
-
-          __globalAgent = PUBLIC_BSKY_AGENT
-          // TODO: This needs a setState.
-        }
+        const freshAccount = await resumeSessionWithFreshAccount()
+        __globalAgent = agent
+        await fetchingGates
+        setState(s => {
+          return {
+            accounts: [
+              freshAccount,
+              ...s.accounts.filter(a => a.did !== freshAccount.did),
+            ],
+            currentAgentState: {
+              did: freshAccount.did,
+              agent: agent,
+            },
+            needsPersist: true,
+          }
+        })
       } else {
         logger.debug(`session: attempting to reuse previous session`)
 
@@ -480,6 +467,11 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
       const persistedSession = persisted.get('session')
 
       logger.debug(`session: persisted onUpdate`, {})
+      setState(s => ({
+        accounts: persistedSession.accounts,
+        currentAgentState: s.currentAgentState,
+        needsPersist: false, // Synced from another tab. Don't persist to avoid cycles.
+      }))
 
       const selectedAccount = persistedSession.accounts.find(
         a => a.did === persistedSession.currentAccount?.did,
@@ -531,15 +523,9 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
             did: undefined,
             agent: PUBLIC_BSKY_AGENT,
           },
-          needsPersist: true, // TODO: This seems bad in this codepath. Existing behavior.
+          needsPersist: false, // Synced from another tab. Don't persist to avoid cycles.
         }))
       }
-
-      setState(s => ({
-        accounts: persistedSession.accounts,
-        currentAgentState: s.currentAgentState,
-        needsPersist: false, // Synced from another tab. Don't persist to avoid cycles.
-      }))
     })
   }, [state, setState, initSession])