Page MenuHomePhabricator

[lib][web][native] handle failed redux migrations
ClosedPublic

Authored by kamil on Feb 5 2024, 1:15 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 10:01 AM
Unknown Object (File)
Wed, Nov 27, 9:28 AM
Unknown Object (File)
Sat, Nov 23, 5:15 PM
Unknown Object (File)
Sat, Nov 23, 3:59 PM
Unknown Object (File)
Sat, Nov 23, 3:52 PM
Unknown Object (File)
Sat, Nov 23, 1:45 PM
Unknown Object (File)
Wed, Nov 6, 4:18 PM
Unknown Object (File)
Oct 15 2024, 10:47 AM
Subscribers
None

Details

Summary

Context here: https://phab.comm.dev/D10784#inline-66023.

This code provides functions that handle resetting the user state after migration failure. As a result, this code should also cause the deletion database and clear auth metadata.
Right now this code causes logging out - recovering on native is tracked in ENG-6546.

Why do we need different nonUserSpecificFields here:
The result of handleReduxMigrationFailure is passed directly to the REHYDRATE action, and redux does not merge modified keys with the default state. And we have this flow:

  1. redux-persist rehydrates only persisted state - not full AppState
  2. We call this code on the failure, it goes through the state and reasign nonUserSpecificFields.
  3. If the field is nonUserSpecificFields but it hasn't persisted it gets overridden with undefined - an example is loadingStatuses.
  4. REHYDRATE action skips e.g. loadingStatuses because it was modified without merging with default state.
  5. We end up with loadingStatuses: undefined which is not what is described by types and causes errors in selectors.

To mitigate that, in this particular case when we reset the state after a failed migration we want to reset only persisted fields, for others we should use default ones (those are always used as default on app start).
Not sure if this is the cleanest solution but couldn't think of anything better.

Test Plan

Add migration that fails and run it, make sure it reset state, clear database and on native clears also auth metadata.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Feb 5 2024, 1:27 PM
kamil edited the summary of this revision. (Show Details)
kamil added inline comments.
web/redux/persist.js
36 ↗(On Diff #36655)

this function will be used in next diff in the stack

native/redux/persist.js
117 ↗(On Diff #36655)

I worry that we might modify the list of persisted fields without thinking about this list here. What can we do to make sure we don't accidentally add something new that is persisted in redux-persist, without considering whether it should be added here?

If we want to only use persisted fields, can we automatically calculate what those are?

web/redux/persist.js
41 ↗(On Diff #36655)

Same question here

kamil edited the summary of this revision. (Show Details)
  • automatically calculate perssited fields
  • resetUserSpecificStateOnIdentityActions -> resetUserSpecificState
ashoat added 1 blocking reviewer(s): inka.

Looks good to me, but I think @inka should take a look too

  • wipe keyserver store, not only cookie
  • fix updateClientDBThreadStoreThreadInfos function
lib/utils/keyserver-store-utils.js
6–23 ↗(On Diff #36774)

this change was suggested by @inka, it better to wipe all keyservers - not only set cookie to null

native/redux/client-db-utils.js
51–54 ↗(On Diff #36774)

updateClientDBThreadStoreThreadInfos was used also before migration 44 (introducing keyserver store) so the previous version wasn't correct if someone was migrating from a very old state version.

handleMigrationFailure defined - for migrations after 44
handleMigrationFailure undefined - older migrations, in that case, we still have a cookie in the top level so we set it to null (we need to cast via any because right now types are defined differently)

This revision is now accepted and ready to land.Feb 7 2024, 4:32 AM
This revision was landed with ongoing or failed builds.Feb 8 2024, 10:52 AM
This revision was automatically updated to reflect the committed changes.