Page MenuHomePhabricator

[lib] move calling `getThreadUpdatesForNewMessages` to `reduceThreadInfos`
ClosedPublic

Authored by kamil on Wed, Oct 9, 7:07 AM.
Tags
None
Referenced Files
F2989231: D13661.diff
Wed, Oct 16, 8:32 PM
Unknown Object (File)
Tue, Oct 15, 5:27 AM
Unknown Object (File)
Mon, Oct 14, 12:28 PM
Unknown Object (File)
Mon, Oct 14, 7:07 AM
Unknown Object (File)
Sun, Oct 13, 1:02 PM
Unknown Object (File)
Sun, Oct 13, 12:20 AM
Unknown Object (File)
Sat, Oct 12, 7:11 AM
Unknown Object (File)
Fri, Oct 11, 3:36 PM
Subscribers

Details

Summary

This is a fix for ENG-9453.

Alternative is introducing another queue in useSendComposableDMOperation to make sure we're always calculating replies count on the newest thread info, but I prefer this solution and it's cleaner.

Test Plan

Send a lot of messages and checking replies count - did test multiple times as there is no single way to reproduce it.

Diff Detail

Repository
rCOMM Comm
Branch
fix-count
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil added inline comments.
lib/reducers/master-reducer.js
59–62

done the same way as in useSendDMOperationUtils

lib/shared/dm-ops/dm-op-utils.js
358–372

we can remove it because this is done before calling this function, see thread-reducer.js

lib/shared/dm-ops/process-dm-ops.js
220–226

this is now done in reducer

kamil published this revision for review.Wed, Oct 9, 7:11 AM
lib/reducers/message-reducer.js
1888–1890 ↗(On Diff #45015)

Is it possible to expand a little bit on this? I understand that you're just moving the code around here but it would help to be more specific about "dedicated actions", and to explain why we do it this way

lib/reducers/message-reducer.js
1888–1890 ↗(On Diff #45015)

Do you think this is better now?

tomek added inline comments.
lib/reducers/thread-reducer.js
605 ↗(On Diff #45015)

We should also return threadStoreOperations

This revision is now accepted and ready to land.Thu, Oct 10, 5:14 AM
lib/reducers/message-reducer.js
1888–1890 ↗(On Diff #45015)

A bit better, but some things are still confusing to me:

  1. Do we use processDMOpsActionType to schedule messages to other peers, even for composable DM messages? Or are they two different cases?
  2. Do we only care about calculating the reply count during processDMOpsActionType for non-composable messages? Is that because it's handled already for composable messages by a different action?
lib/reducers/message-reducer.js
1888–1890 ↗(On Diff #45015)
  1. We always use processDMOpsActionType to schedule messages to other peers (this is the only way to do it), that's why dedicated actions are not enough here (for the DMs) and we have processDMOpsActionType too.
  2. We have one mechanism for calculating the reply count for DM messages (implemented here in thread reducer) which works regardless of message type (composable or not), and side (sender or receiver).

Looks like this is confusing so sharing some more context:
Non-composable messages, receiver
processDMOpsActionType is responsible for adding messages to the store, and updating the reply count in the thread reducer (getThreadUpdatesForNewMessages), no messages to other peers are scheduled because this is the receiver.

Non-composable messages, sender
processDMOpsActionType is responsible for adding messages to the store, and scheduling messages to other peers, and updating the reply count in the thread reducer (getThreadUpdatesForNewMessages).

Composable messages, receiver
processDMOpsActionType is responsible for adding messages to the store, and updating the reply count in the thread reducer (getThreadUpdatesForNewMessages), no messages to other peers are scheduled because this is the receiver. There is no additional action.

Composable messages, sender
sendTextMessageActionTypes or sendMultimediaMessageActionTypes is responsible for actual sending logic the same way as for the keyserver, we didn't have to rewrite InputStateContatiner, we have tracking status, and retrying implemented. This action also adds a message to the message store.
The only difference is that instead of calling keyserver it dispatches processDMOpsActionType which:

  • schedule messages to other peers
  • await sending messages to mark the message as a success/failure
  • update reply count as all above using getThreadUpdatesForNewMessages
  • it can not add a message to the store, it was already added using sendTextMessageActionTypes or sendMultimediaMessageActionTypes, this is why we have the code here (in theory, the message will be merged anyway but for the short period of time we can show two messages on the screen)

I'm going to land it now to fix it - but if the comment is still not okay, I can update this later as a follow-up.

Thanks for the detailed explanation!!