[web] Fix connection field missing
ClosedPublic
Actions

Authored by inka on Dec 22 2023, 5:01 AM.

Details

Reviewers

kamil
michal
ashoat

Commits

rCOMM228d352228f6: [web] Fix connection field missing

Summary

issue: ENG-6104
See this discussion for all details, but here are the most important things:

migrateStorageToSQLite can be called on current and later version of the app, because the db might not be available sometimes and then we default to using local storage.
a transform changing is also a problem in the usual case - we persist state with one transform and then restore it with a changed one - this will cause issues. So if we ever change the transforms, they should be responsible themselves for making sure that the old persisted state isn't broken (this is in reference to the comment I deleted).
This diff doesn't change the behaviour when oldStorage?._persist?.version === 4

Test Plan

tested that the connection filed is no longer missing in the following scenario:

login
kill the keyserver
logout
refresh (app not working)
run keyserver
refresh -> connection is missing

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

inka created this revision.Dec 22 2023, 5:01 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 22 2023, 5:01 AM

inka added a child revision: D10442: [lib] Remove cookie on logout.Dec 22 2023, 5:13 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 22 2023, 5:27 AM

Harbormaster failed remote builds in B25315: Diff 34969!

Harbormaster completed remote builds in B25315: Diff 34969.Dec 22 2023, 5:35 AM

inka requested review of this revision.Dec 22 2023, 5:35 AM

Could we apply all transforms in persistConfig.transforms dynamically instead of "hardcoding" them in migrateStorageToSQLite?

a transform changing is also a problem in the usual case - we persist state with one transform and then restore it with a changed one - this will cause issues. So if we ever change the transforms, they should be responsible themselves for making sure that the old persisted state isn't broken (this is in reference to the comment I deleted).

In that case should we move the oldStorage?._persist?.version >= 4 condition inside of the keyserverStoreTransform? If someone adds a new transform then it would have it's own condition inside it, from which state version it should modify the state data?

Iterate all transforms
The if was removed altogether, because this particular transform is always correct

Harbormaster completed remote builds in B25346: Diff 35016.Dec 27 2023, 3:52 AM

I'm not as familiar with the redux-persist transforms code as @michal, but since this is resolving an urgent issue and @michal is out of office, I'll go ahead and accept it on his behalf.

@inka, might also be good to set a reminder to ping @michal about this diff to confirm that there are no issues after he's back.

This revision is now accepted and ready to land.Dec 27 2023, 6:21 AM