We only need to do a "spot check" on this value below. To avoid regenerating performRecovery whenever it changes, I assign to a ref here

By assigning to a ref, I avoid using the value of preRequestUserState at the time performRecovery is bound, and instead am able to use the value of preRequestUserState right before resolveKeyserverSessionInvalidation is called below

One difference in the code before my recent changes: previously, if the session recovery failed, we would commit the sessionChange that came in the initial failed request to Redux.

After my changes, we are using this genericCookieInvalidation instead.

This means we won't keep the anonymous cookie we are passed. But these anonymous cookies are useless, and are due to be deprecated in ENG-4653. Furthermore, trying to pass the sessionChange from the failed request would be tough... we would need to find some way to get it from the CallKeyserverEndpointProvider to the KeyserverConnectionHandler.

Without this change, invalidSessionRecovery was filtering out the SET_NEW_SESSION call.

I think that prior to my recent changes, SET_NEW_SESSION with cookieInvalidated would only be responsible for changing the cookie, and it would contain the same currentUserInfo that was currently in the Redux store.

In my new system, SET_NEW_SESSION with cookieInvalidated is responsible for clearing the currentUserInfo, so it contains a LoggedOutCurrentUserInfo here.

I think this change is safe. If it's a LoggedOutCurrentUserInfo, it should not count as a session recovery, since the session is being thrown away. Meanwhile, I think invalidSessionDowngrade should handle making sure that the SET_NEW_SESSION is ignored if something has changed in the meantime.

This change was necessary because if we simply don't include cookie, the the code in keyserver-reducer.js won't clear it. In order to make sure the cookie is cleared, we need to specify null.

This wasn't necessary before because the cookie would be set to a string (anonymous cookie).

Can we use sessionIDSelector?

actionCurrentUserInfo can be null/undefined according to our types. Should we return in such case as well?

We wanted to pass preRequestUserState.currentUserInfo for set new session action, instead of sessionChange.currentUserInfo ENG-6126
This was intended to fix the bug described in ENG-5648, in which a set new session was trying to downgrade the session, but would fail because of this functions currentReduxState.currentUserInfo?.id !== actionCurrentUserInfo.id check.

But I think this code you introduce here is better - an action cannot be a recovery if it has currentUserInfo = {anonymous: true}. And this should fix the issue from ENG-5648

Will make this change

This was intended to fix the bug described in ENG-5648, in which a set new session was trying to downgrade the session, but would fail because of this functions currentReduxState.currentUserInfo?.id !== actionCurrentUserInfo.id check.

But I think this code you introduce here is better - an action cannot be a recovery if it has currentUserInfo = {anonymous: true}. And this should fix the issue from ENG-5648

That makes sense, and happy to hear it will solve ENG-5648.

actionCurrentUserInfo can be null/undefined according to our types. Should we return in such case as well?

Are you sure we need to consider the case where actionCurrentUserInfo is null/undefined? There is an invariant directly above this line that would catch this case, and in fact there is a check in native/redux/redux-setup.js as well.

We wanted to pass preRequestUserState.currentUserInfo for set new session action, instead of sessionChange.currentUserInfo ENG-6126

I think we still need to fix invalidSessionRecovery, as it does not currently do what it is supposed to (prevent a session recovery from overwriting a logout). I did some thinking about this and added a comment here.

Are you sure we need to consider the case where actionCurrentUserInfo is null/undefined? There is an invariant directly above this line that would catch this case, and in fact there is a check in native/redux/redux-setup.js as well.

Ohh, that's right, sorry.

I definitely think the effect approach is worse, as it forces us to wait on React to run the effect.

I can add a code comment.

Yes – that would additionally allow an explicit cookie: undefined to be specified. I'm not opposed to it, but I opted for this approach for consistency with the sessionID line directly above