[web][native] Reset the state iff current user ID changes
ClosedPublic
Actions

Authored by tomek on Apr 3 2024, 4:23 AM.

Details

Reviewers

kamil
inka
varun
ashoat

Commits

rCOMMa384021d4934: [web][native] Reset the state iff current user ID changes

Summary

We don't need to clear the state if the ID remains the same - it means that user-specific data can be still used. Every time the ID changes we have to clear the user specific data - it is safer to do that regardless of the action reduced.

In this approach we're reducing currentUserInfo twice - the reason is explained in code comments.

https://linear.app/comm/issue/ENG-7075/clear-redux-state-only-when-current-user-id-changes

Depends on D11532

Test Plan

Checked if the state is cleared after logging out.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

tomek created this revision.Apr 3 2024, 4:23 AM

Herald added a subscriber: ashoat. · View Herald TranscriptApr 3 2024, 4:23 AM

Harbormaster completed remote builds in B27940: Diff 38719.Apr 3 2024, 4:39 AM

tomek requested review of this revision.Apr 3 2024, 4:39 AM

tomek added a child revision: D11534: [web][native] Extract the common logic determining when to clear the data.Apr 3 2024, 4:57 AM

inka added inline comments.Apr 4 2024, 3:46 AM

native/redux/redux-setup.js
275–279	Placing this after native specific actions are reduced means that it is not possible for a native specific action to override a reset field. It this necessary? It looks like currently the code is not incorrect, but I fear that this makes introducing bugs in the future more likely

inka added inline comments.Apr 4 2024, 3:59 AM

native/redux/redux-setup.js
272–273	What about null user info?

ashoat added inline comments.Apr 5 2024, 8:57 AM

native/redux/redux-setup.js
263–266	This comment seems confusing to me: "Once here - from the previous state, and once in baseReducer." seems to imply that `baseReducer` does not operate on the previous state. It usually does... the only exception is if `resetUserSpecificState` runs "This approach protects us against possible bugs in currentUserInfo reducer." it's not immediately clear how it protects us "BaseReducer acts on cleared currentUserInfo" this is only true if `resetUserSpecificState` runs "so there's no risk of keeping the old info with a new ID." I don't understand what risk this is referring to, what "old info with a new ID" means (mismatched username/ID? that shouldn't be possible...), and how the risk is mitigated

tomek added inline comments.Apr 8 2024, 7:05 AM

native/redux/redux-setup.js
263–266	Agree with all the points. I'm going to update the comment with a better explanation. Also, here is a description of what I'm trying to protect us against. To know that the current user ID changes, we're reducing current user info using the reducer. There are two possible outcomes: either the ID changes or it doesn't. If the ID changes, we need to clear the content of the store, including the current user info. We have some options for how to determine the new current user info: We can take the just reduced value. This is correct but also fragile. If we have one branch in the current user info reducer that changes the ID without clearing / overriding the rest of the store, we will keep the data that doesn't belong to a new current user - and this is a risk I'm trying to protect us against. Also, if we take this approach, the `currentUserInfo` prop of the result of `resetUserSpecificState` call is never used - which is concerning. We can ignore the reduced value, call `resetUserSpecificState`, and reduce `currentUserInfo` based on the returned result. This is a little paranoid but is as safe as it could be. If the ID doesn't change, we don't need to clear the store and there's no risk of keeping the data of the previous user. mismatched username/ID? that shouldn't be possible... Yeah, for example. I wanted to go from `shouldn't be possible` to `isn't possible`.

tomek added inline comments.Apr 8 2024, 8:36 AM

native/redux/redux-setup.js
275–279	Makes sense - going to move it to make it safer.

Requesting changes to put it back on your queue, and to add myself to the reviewers list so I can review the updated comment

native/redux/redux-setup.js
263–266	Thanks for explaining!

This revision now requires changes to proceed.Apr 8 2024, 7:53 PM

Handle null user and update the comment

native/redux/redux-setup.js
272–273	In the case where `null` becomes `non-null` we will reset the state - which is incorrect. Nice catch!
275–279	Thought about this more and I think that the current solution makes sense. We reset the state only when the current user ID changes. It can change only when the current user reducer returns a new value. The reducer accepts only lib actions - it can't reduce platform-specific actions. So none of the platform-specific actions could cause the state reset. If we really want to move this code up, we would need to filter out all the platform-specific actions, because the reducer doesn't understand them. And at that point, the refactoring won't make sense, because the code wouldn't be safer and would be more complicated.

Harbormaster completed remote builds in B28084: Diff 38934.Apr 9 2024, 5:09 AM

ashoat accepted this revision.Apr 9 2024, 3:39 PM

ashoat added inline comments.

native/redux/redux-setup.js
270 ↗	(On Diff #38934)	I might prefer to use `baseReducer` so the reader can search for it

This revision is now accepted and ready to land.Apr 9 2024, 3:39 PM

inka accepted this revision.Apr 10 2024, 2:46 AM

inka added inline comments.

native/redux/redux-setup.js
275–279	We don't have a guarantee that no native specific action will ever change the `currentUserInfo`. But I see that this is just not worth doing now.