[lib] Create a store in redux for the invite links
ClosedPublic
Actions

Authored by tomek on May 19 2023, 9:44 AM.

Details

Reviewers

bartek
kamil
inka

Commits

rCOMM69d623f52dfa: [lib] Create a store in redux for the invite links

Summary

A couple of screens require invite links info, and it wasn't an issue until I added an option to update a link - then the data displayed on the screens was inconsistent. The approach with redux should be used from the beginning.

It will also significantly simplify introduction of invite link updates (if we decide to have them).

Depends on D7848

Test Plan

Tested in combination with the rest of the stack: updating a link on one screen and opening another screen where that link is displayed should show the current value.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

tomek created this revision.May 19 2023, 9:44 AM

Herald added subscribers: atul, ashoat. · View Herald TranscriptMay 19 2023, 9:44 AM

tomek added a child revision: D7730: [native] Create a modal where invite links are displayed.May 19 2023, 9:47 AM

tomek edited parent revisions, added: D7876: [lib] Update invite link type; removed: D7848: [keyserver] Validate fetchPrimaryInviteLinksResponder output.

tomek edited the summary of this revision. (Show Details)May 19 2023, 10:01 AM

tomek added a parent revision: D7848: [keyserver] Validate fetchPrimaryInviteLinksResponder output.

Harbormaster completed remote builds in B19509: Diff 26674.May 19 2023, 10:02 AM

tomek requested review of this revision.May 19 2023, 10:02 AM

kamil requested changes to this revision.May 22 2023, 1:34 AM

kamil added inline comments.

lib/types/link-types.js
39	This is used twice in this diff, and probably will be used in the future in different places, so I think it will be more readable to make this a separate type, but up to you.
native/redux/persist.js
536	At this point, the state is not a type of `AppState`, because it does not have required prop `inviteLinksStore`. In other migrations this thing is common so probably it's okay - just it doesn't feel correct to me.
536–542	are you sure this is needed? I think we use by default autoMergeLevel1 - so rehydrated state should be merged with the initial state, and this will have no effect anyway.
web/redux/redux-setup.js
100	It's not optional so you have to add this to the initial state on the web as the client will expect this object to be defined.

This revision now requires changes to proceed.May 22 2023, 1:34 AM

tomek added inline comments.May 23 2023, 4:57 AM

lib/types/link-types.js
39	This is quite similar to what we do e.g. with threads or messages. But I'll add this new type.
native/redux/persist.js
536	That's true, but following this convention is convenient.
536–542	Interesting. I added it after some errors I've seen. I can test and remove if not necessary, but also I'm not sure if spending too much time on it makes sense.
web/redux/redux-setup.js
100	I guess it's ok - it will be introduced on web soon. On the other hand, we could wait with introducing it on web until the diffs are up - but it shouldn't matter.

tomek added inline comments.May 23 2023, 5:07 AM

native/redux/persist.js
536–542	I've tested it by: Checking out a version before this migration was introduced Wiping out the state Opening and closing the app Checking out this version with migration deleted Opening the app - the store contained `{"links": {}}` which proves that it works

Delete unnecessary migration

Harbormaster completed remote builds in B19663: Diff 26871.May 23 2023, 5:33 AM

tomek removed a parent revision: D7876: [lib] Update invite link type.May 23 2023, 6:06 AM

tomek added a parent revision: D7941: [keyserver] Convert flag to a boolean.

ashoat added inline comments.May 23 2023, 7:28 AM

lib/selectors/invite-links-selectors.js
9 ↗	(On Diff #26871)	We should avoid returning mutable objects from `createSelector` calls, as the object is memoized by `createSelector`, and mutating it will affect future invocations of the selector

Use readonly type

Harbormaster completed remote builds in B19684: Diff 26892.May 23 2023, 7:58 AM

kamil accepted this revision.May 23 2023, 4:08 PM

kamil added inline comments.

native/redux/persist.js
536–542	Interesting. I added it after some errors I've seen. Not sure what exactly errors you saw, but I suspect that it was an alert saying that `EXPECTED KEYS NOT REHYDRATED: ["inviteLinksStore"]`, this should be shown only once and I don't think it's a problem - just logging that there is a new key in the default store, not persisted yet.
web/redux/redux-setup.js
100	I still do not feel comfortable with the fact, that we create places (like this one in this particular case) where we assume that variable is of a given type but it isn't, but agree, it shouldn't matter

This revision is now accepted and ready to land.May 23 2023, 4:08 PM

tomek added inline comments.May 24 2023, 1:07 AM

native/redux/persist.js
536–542	No, these were exceptions saying something like "cannot read property `links` of undefined". But maybe they were connected to requests and not the state.
web/redux/redux-setup.js
100	Yeah, that's not great, but a possible solution would require a lot of work and maintenance. For one migration it doesn't sound bad: we can have a type without one field. But then, when another migration adds yet another field, we would need three types: one for AppState, one without one field, and one without two. It feels like it's really not worth it... maybe if we frequently encounter bugs caused by these types we could investigate other options, but up to this point I don't remember any accident caused by this.