[lib] update message reducer to use ops approach for local field
ClosedPublic
Actions

Authored by ginsu on Fri, Jun 7, 5:02 PM.

Details

Reviewers

will
tomek
kamil

Commits

rCOMMa86fb7ba13a4: [lib] update message reducer to use ops approach for local field

Summary

Refactor the local field in the message reducer + relevant util functions in this file to use the ops approach

Depends on D12357

Test Plan

Tested each action type w/ @will during the hackathon, and we were able to confirm that we were getting the expected behavior for each action

Some of the main user flows we tested:

fail sending a message
successfully retry sending a message
closing the app where a thread had a bunch of sent failed local messages
fail sending a reaction message
deleting a thread with a bunch of sent failed messages

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Fri, Jun 7, 5:02 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptFri, Jun 7, 5:02 PM

ginsu edited the summary of this revision. (Show Details)Fri, Jun 7, 5:03 PM

ginsu added reviewers: will, tomek, kamil.

ginsu added a parent revision: D12357: [web] handle message store local ops types in processMessageStoreOperations.

Harbormaster completed remote builds in B29546: Diff 41133.Fri, Jun 7, 5:20 PM

ginsu requested review of this revision.Fri, Jun 7, 5:20 PM

tomek requested changes to this revision.Mon, Jun 10, 3:40 AM

tomek added inline comments.

lib/reducers/message-reducer.js
791–792 ↗	(On Diff #41133)	Why don't we set the local here? Shouldn't we set it for all the actions? Maybe we can even spread the processed store?
901 ↗	(On Diff #41133)	Shouldn't we also set the local? Could you explain here, and in all the other places, why we're not setting the local?
1583–1586 ↗	(On Diff #41133)	It is surprising to see an action where we're adding some ops without removing the old logic. Originally, we weren't modifying the `messageStore.local`, and now we do. What is the reason?

This revision now requires changes to proceed.Mon, Jun 10, 3:40 AM

We should also check for store inconsistencies in setClientDBStoreActionType - but I guess that extending this action will be done later in the stack.

ginsu added a child revision: D12370: [native/web] introduce migration to move local message info to sqlite.Mon, Jun 10, 11:15 PM

ginsu added inline comments.Tue, Jun 11, 10:26 PM

lib/reducers/message-reducer.js
791–792 ↗	(On Diff #41133)	Was thinking that since `messageStore.local` was not being touched it would be fine to not explicitly set local here, but now I can see that it would be best to be explicit here and also set local
1583–1586 ↗	(On Diff #41133)	I think initially during the hackathon the thinking was that since these are the ops for the `createLocalMessageActionType` it might make sense to add a "shell" local message info; however, after thinking about it some more, this is unnecessary/redundant, and can be removed

We should also check for store inconsistencies in setClientDBStoreActionType - but I guess that extending this action will be done later in the stack.

D12384

update

ginsu edited the test plan for this revision. (Show Details)Tue, Jun 11, 10:32 PM

ginsu added inline comments.Tue, Jun 11, 10:37 PM

lib/reducers/message-reducer.js
1583 ↗	(On Diff #41228)	Noticed we were missing setting threads here so snuck this change in

Harbormaster completed remote builds in B29616: Diff 41228.Tue, Jun 11, 10:44 PM

tomek accepted this revision.Wed, Jun 12, 4:09 AM

tomek added inline comments.

lib/reducers/message-reducer.js
791–792 ↗	(On Diff #41133)	For me, the benefit of having local here is that the logic from the reducer and from the ops processing get decoupled - the reducer doesn't need to know any detail about how the result of ops processing will look.