[Session] Use flag on state for persistence

[gaearon]

See individual commits.

• cf360ed and 5d0d740 move isInitialLoad and isSwitchingAccounts out of the main state object, similar to how they're taken out in [Session] V2 (V2) #3728.
• 5d0d740 is just a visual cleanup so we always see the state object shape.
• Now, 9e0dd84 is the interesting bit. This is a change in strategy both from what's in main and what's in [Session] V2 (V2) #3728. If this change looks good, I'll update the other PR to match it. See below.
• 2cdab86 is just a line reorder.

Persistence change

Whenever we update accounts or currentAccount, we need to persist that change to the local storage. There is only one exception to that — we don't want to do that if the update is coming from another tab (which means it was already persisted — and we want to avoid two tabs entering a cycle of persisting and notifying each other).

Well, and we also don't want to persist the initial state.

So how do we do this?

On main, persistence is achieved with an explicit call like this:

setStateAndPersist(prev => ({
  accounts: [account, ...prev.accounts.filter(...)],
  currentAccount: prev.currentAccount
})

which by itself sets isDirty.current = true, and then an Effect checks it.

On #3728, the strategy is different because there's no single state object so there's no setStateAndPersist function. Instead there's a function called persistNextUpdate which sets the same isDirty.current = true ref. I'm not a big fan of this approach because it's a bit hard to tell which exactly state update will be persisted. It also seems easy to add a setState call somewhere in a different function, assume it's taken care of by an earlier persistNextUpdate call, and then to lose that earlier call due to some refactoring. So I'd like to enforce that the decision to persist is close to the state update.

The strategy I'm taking in this PR is different from both of these approaches. Instead of tracking a separate ref, I track the need to persist in the state object itself as a mutable field.

- setStateAndPersist(prev => ({
+ setState(prev => ({
  accounts: [account, ...prev.accounts.filter(...)],
  currentAccount: prev.currentAccount,
+  needsPersist: true
})

There is no isDirty ref. Instead, the effect checks the latest committed state:

  React.useEffect(() => {
    if (state.needsPersist) {
      state.needsPersist = false
      persisted.write('session', {
        accounts: state.accounts,
        currentAccount: state.currentAccount,
      })
    }
  }, [state])

This ensures that any state updates to state.accounts and state.currentAccount get persisted as long as they have the needsPersist: true tag. I'm keeping it false on updates originating from the tab sync.

Note that if you have a needsPersist: true update that hasn't been processed yet, immediately followed by a needsPersist: false update, neither of them will get persisted because the latest version wins. This is different from the current behavior where isDirty.current being true will cause the next committed update to be persisted. However I think that's fine (and maybe even good?) because the only reason you could get a needsPersist: false update is if you did something in another tab — and in that case the other-tab-originating update is fresher anyway. So it kind of makes sense that we only ever want to persist the latest state — and only if that specific state is marked to be persisted.

I plan to port this change to #3728 as well if we get it into main.

Test Plan

Put logs next to persistence. Verify changes get persisted as before. Verify switching account, logging out, logging in works. Verify logging out or switching accounts in another tab switches the current tab.

[render]

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

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

[github-actions]

Old size	New size	Diff
6.49 MB	6.49 MB	-96 B (-0.00%)

---

[estrattonbailey]

Tested with two tabs open via #3795, code LGTM