[Session] Align state and global agent switchpoints

[gaearon]

Extracted from #3728.

There's a few different changes here but they all work together towards the goal of aligning __globalAgent assignments with setState calls that modify currentAgentState. Once they fully align, we can kill the global.

This PR doesn't fully get us all the way there because it leaves some existing timing differences.
I will address them in a follow-up.

Reviewing

These are meant to be safe but slightly riskier than the ones before.

I suggest reading each change in isolation.

97a5304

This moves this setState on persisted.onUpdate callback to the beginning of the function.

The purpose of this setState is to update the current accounts to the persisted.accounts from another tab. We always need to do it, and it doesn't depend on any other data, so it makes sense to do it early.

Note for this change to be safe, we need to make sure that it wasn't clashing with any other setState during this callback. Here's how we verify this (links are to the existing code):

• This other setState does not touch accounts so it doesn't matter whether it runs before or after — they accumulate regardless of order.
• This initSession call happens in the middle of the function but it's not being awaited. Therefore, only the code before the first await would run, which transitively has no setState calls.

Therefore, it is safe to move this setState to the beginning of the function.

af704f7

If another tab logged us out, that has already been persisted to the storage by that tab. So it doesn't make sense to persist anything in the storage here.

I believe this was only set to true due to the previous structure of the code which had helper methods like clearCurrentAccount (which always had persistence on). Now that we've gotten rid of helper methods, we can see more clearly what is actually going on, and can adjust it more precisely.

5923f3f

This removes a try/catch around resumeSessionWithFreshAccount in the "saved session expired during initSession" codepath. The thinking here is similar to #3836 — we can rely on onAgentSessionChange callback to log us out. In fact, we're already relying on it because clearly updating __globalAgent is not enough — it does not update the UI. This code was only updating __globalAgent so it can be removed (updating __globalAgent on expiry is done by #3838).

Another consequence of this change is that initSession would now throw if resuming the session failed. Counter-intuitively, I'm removing the logic inside the existing catches in this commit. Since it did not throw in the past, those were superflouous. However, I'm keeping the catches itself so that the calling code's expectations don't change.

Let's audit its callsites to verify this:

• The InnerApp calls here and here already handle exceptions, no change in behavior.
• useAccountSwitcher has an initSession call which does change the behavior.
  • Previously, we'd show "Signed in as ..." even on failure.
  • After the change, we don't show it on failure. (Technically we still do if the session is resumed optimistically but that's an existing problem.)
  • Again, note I've removed the logic inside the catch because we weren't hitting it in the past either. It's superfluous because both clearing the account and showing the failure toast is already handled by onAgentSessionChange.
• Same story with this call in ChooseAccountForm.
  • Previously, we'd show "Signed in as ..." even on failure.
  • After the change, we don't show it on failure. (Technically we still do if the session is resumed optimistically but that's an existing problem.)
  • Here, too, I've removed the logic inside the catch because it wasn't getting called in the past either. So it's superflouous.

Note how by the end of this change, all __globalAgent assignments have a matching setState next to them. There's sometimes an async gap between them, which I'll fix in a follow-up PR.

Test Plan

Verify syncing between tabs (login / logout / switch) still works.

Verify expiration during resumption is handled (see above and test plan from #3838 for testing scenarios).

[render]

Your Render PR Server URL is https://social-app-pr-3845.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-coqgqu021fec73am9j8g.

[github-actions]

Old size	New size	Diff
6.86 MB	6.86 MB	-114 B (-0.00%)

---

[pfrazee]

Just reviewing the code changes, seems sane

[gaearon]

Pushed a slightly less risky version of the last commit.

[gaearon]

Pushed a few more commits.

• 452514d: I've readded the onSelectAccount check that I removed earlier. Even though it wasn't doing anything in the past (cause we weren't hitting it), it's a good idea to kick the user to the login form if resumption failed instead of keeping them staring at the account list.

• e9c165a: This just makes the control flow clearer.