Page MenuHomePhabricator

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

Authored by tomek on Apr 3 2024, 4:23 AM.
Tags
None
Referenced Files
F3374465: D11533.id39050.diff
Tue, Nov 26, 3:40 PM
Unknown Object (File)
Sun, Nov 24, 4:10 AM
Unknown Object (File)
Sun, Nov 24, 12:45 AM
Unknown Object (File)
Sun, Nov 24, 12:05 AM
Unknown Object (File)
Tue, Nov 12, 10:49 AM
Unknown Object (File)
Sun, Nov 3, 11:23 AM
Unknown Object (File)
Oct 13 2024, 12:07 AM
Unknown Object (File)
Oct 13 2024, 12:07 AM
Subscribers

Details

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 requested review of this revision.Apr 3 2024, 4:39 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

native/redux/redux-setup.js
272–273

What about null user info?

native/redux/redux-setup.js
263–266

This comment seems confusing to me:

  1. "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
  2. "This approach protects us against possible bugs in currentUserInfo reducer." it's not immediately clear how it protects us
  3. "BaseReducer acts on cleared currentUserInfo" this is only true if resetUserSpecificState runs
  4. "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
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.

  1. 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:
    1. 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.
    2. 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.
  2. 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.

native/redux/redux-setup.js
275–279

Makes sense - going to move it to make it safer.

ashoat requested changes to this revision.Apr 8 2024, 7:53 PM

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
tomek marked 4 inline comments as done.

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.

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 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.

tomek marked 2 inline comments as done.

Update the comment