[lib] Fix Logout not working and state difference after invalid session downgrade
ClosedPublic
Actions

Authored by inka on Dec 4 2023, 7:06 AM.

Details

Reviewers

kamil
tomek
atul
michal

Commits

rCOMM24a8f3757ba9: [lib] Fix Logout not working and state difference after invalid session…

Summary

issue: https://linear.app/comm/issue/ENG-6001/logout-not-working-and-state-difference-after-invalid-session
There were two problems:

Logout doesn't work because logOutLoadingStatusSelector returns 'loading'. This is probably because logout.success is discarded so reduceLoadingStatuses doesn't update the loading status for the logout action. If this is the issue, it was there for a very long time

This we are fixing by reducing loading statuses before discarding an action

logOutActionTypes.started, setNewSessionActionType and logInActionTypes.success don't clear the redux user store. logOutActionTypes.success does. So if the user logged in before logout succedeed, they were seeing the same userStore in the state and from the login action, so the ops were being discarded, but the db had been deleted. This resulted in state difference error. For more details see this comment: https://linear.app/comm/issue/ENG-6001/logout-not-working-and-state-difference-after-invalid-session#comment-58f94aae

This we are fixing by never discarding ops for login actions

Test Plan

Checked that on login LOG_OUT statuses are being cleared.
Checked that it is possible to logout after an invalidSessionDowngrade

Checked that the state difference error doesn't appear anymore in the scenario described in the issue. Checked that on login action the ops for user store are being executed.

Diff Detail

Repository

rCOMM Comm

Branch

inka/login_and_state_error

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

inka created this revision.Dec 4 2023, 7:06 AM

Herald added a subscriber: ashoat. · View Herald TranscriptDec 4 2023, 7:06 AM

inka added a parent revision: D10156: [lib] Move mergeUserInfos.Dec 4 2023, 7:06 AM

inka edited the summary of this revision. (Show Details)

I don't want to remove all loading statuses on login, because login can be dispatched while the app is running, during a session recovery attempt. So I don't want other actions to loose the possibility to set an 'error' loading status, since that has some effects in the app

Harbormaster completed remote builds in B24733: Diff 34191.Dec 4 2023, 8:40 AM

inka requested review of this revision.Dec 4 2023, 8:40 AM

ashoat added inline comments.Dec 4 2023, 8:55 AM

lib/reducers/loading-reducer.js
20–31 ↗	(On Diff #34191)	These variables have similar names, but it appears that their contents are a little different. Should the names be updated to reflect that?
31 ↗	(On Diff #34191)	We create this `Set` twice in this diff. Should we just export a `Set` instead of an `Array` in the first place?

Update names to reflect differences, change arrays to sets

Harbormaster completed remote builds in B24795: Diff 34266.Dec 5 2023, 3:18 AM

This solution is hacky - we're discarding loading statuses of requests that will be resolved later. Instead, we should reduce loading statuses before we check invalidSessionDowngrade.

lib/reducers/loading-reducer.js
23–26	I guess it can be simplified
37–41	It is possible that there are no logout actions in the state, in which case we would like to avoid modifying the state here

This revision now requires changes to proceed.Dec 5 2023, 5:40 AM

Reduce loading statuses before discarding

inka edited the summary of this revision. (Show Details)Dec 15 2023, 2:09 AM

I wanted to move reducing loading statuses to web and native reducers altogether, but I couldn't overcome a flow problem:
I don't want to copy the reduceLoadingStatuses reducer to web and native, so it is located in lib. It takes an action: BaseAction. I was unable to type it in a way that wouldn't cause errors. If I make the action be an inexact BaseAction, like flow docs suggest: action: {...BaseAction, ...}, then I get errors about read-only properties, as the action I'm passing into the function has nested read-only fields, and the action that is being taken by the reducer was stripped of all read-only.
This works, because setNewSessionActionType, logOutActionTypes and deleteAccountActionTypes are in BaseAction
I know this is not ideal, but it fixes the issue, and is way less hacky than the previous solution.

inka added a reviewer: michal.Dec 15 2023, 2:18 AM

Looks a lot cleaner!

This revision is now accepted and ready to land.Dec 15 2023, 2:23 AM

In D10157#299660, @inka wrote:

I wanted to move reducing loading statuses to web and native reducers altogether, but I couldn't overcome a flow problem:
I don't want to copy the reduceLoadingStatuses reducer to web and native, so it is located in lib. It takes an action: BaseAction. I was unable to type it in a way that wouldn't cause errors. If I make the action be an inexact BaseAction, like flow docs suggest: action: {...BaseAction, ...}, then I get errors about read-only properties, as the action I'm passing into the function has nested read-only fields, and the action that is being taken by the reducer was stripped of all read-only.
This works, because setNewSessionActionType, logOutActionTypes and deleteAccountActionTypes are in BaseAction
I know this is not ideal, but it fixes the issue, and is way less hacky than the previous solution.

Just a quick question: does action: $ReadOnly<{...BaseAction, ...}> solve the issue?

Harbormaster completed remote builds in B25109: Diff 34697.Dec 15 2023, 2:27 AM

Just a quick question: does action: $ReadOnly<{...BaseAction, ...}> solve the issue?

No, I get

Cannot call `reduceLoadingStatuses` with `action` bound to `action` because  object type [1] is incompatible with  `$ReadOnly` [2].Flow(incompatible-call)

where "object type" is Action from web/native, and it doesn't help if I set all fields of all variants Action to read-only. It also doesn't help if I type action taken by reducer as $ReadOnly<Action>. I get the same error....