Page MenuHomePhabricator

[lib] introduced sendReactionMessageActionTypes
ClosedPublic

Authored by ginsu on Nov 30 2022, 7:17 AM.
Tags
None
Referenced Files
Unknown Object (File)
Apr 4 2024, 5:33 AM
Unknown Object (File)
Mar 7 2024, 1:31 PM
Unknown Object (File)
Mar 7 2024, 1:05 PM
Unknown Object (File)
Mar 7 2024, 12:39 PM
Unknown Object (File)
Mar 7 2024, 12:17 PM
Unknown Object (File)
Mar 7 2024, 11:54 AM
Unknown Object (File)
Mar 7 2024, 11:26 AM
Unknown Object (File)
Mar 7 2024, 10:37 AM
Subscribers

Details

Summary

introduced sendReactionMessageActionTypes. These will be used in subsequent diffs mainly in message-reducer


Linear Task: ENG-2248

Test Plan

flow also ran app locally on web/native and nothing crashed and there were no regressions. Will continue testing in subsequent diffs

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

updated reaction message success payload type

ginsu requested review of this revision.Nov 30 2022, 7:36 AM

working on message reducer stuff right now and I have a strong intuition that SendReactionMessagePayload will need to change

added newMessageInfos field to SendReactionMessagePayload type

tomek requested changes to this revision.Dec 1 2022, 2:58 AM
tomek added inline comments.
lib/types/message-types.js
521–536 ↗(On Diff #19048)

Not sure about the plan here, but it seems inconsistent:

  1. SendReactionMessagePayload contains targetMessageID but not reaction. It seems we should either include both or none. Also, why do we need targetMessageID when newMessageInfos is there?
  2. SendMessagePayload has localID. It seems like we can use a similar approach when sending reaction message - that id can be used to check what message was sent.
  3. SendMessagePayload doesn't need newMessageInfos. Why this is different comparing to SendReactionMessagePayload?

It seems like using SendMessagePayload instead should be possible, but maybe I'm missing something.

lib/types/redux-types.js
536–537 ↗(On Diff #19048)

It doesn't seem to scale well when we have different reaction types. It shouldn't be a big issue now, but we have to think about a case where a user has two devices: one which supports only liking and one with more reaction types.

This revision now requires changes to proceed.Dec 1 2022, 2:58 AM
lib/types/message-types.js
521–536 ↗(On Diff #19048)

Sorry this took me a while to address

Addressed the first point, your feedback makes sense, and having a separate targetMessageID field is unnecessary

I believe that since reaction messages aren't being shown optimistically at the moment, we need to have a separate reaction message payload type since we are only showing reactions if it was successfully sent. When we eventually work on displaying reaction messages optimistically, I think then using SendMessagePayload should be possible.

lib/types/redux-types.js
536–537 ↗(On Diff #19048)

I am honestly still a little confused about this feedback here; again, sorry this took me a while to address. Please correct me if I am wrong, but regarding the case that you provided about a user having two devices, targetMessageID and threadID will always be present on both devices because both liking and future reaction types will need a targetMessageID field and threadID field. Could you please elaborate more on what you mean regarding that this won't scale well?

tomek added inline comments.
lib/types/message-types.js
521–536 ↗(On Diff #19048)

Ok, that makes sense!

lib/types/redux-types.js
539 ↗(On Diff #19296)

Why this is optional?

536–537 ↗(On Diff #19048)

I think I was concerned about this because error payload doesn't contain an info about which reaction type failed. I don't think it is a valid concern because of how the rest of a stack looks like: in this code version we can assume like action and this code version would not receive a failed request about different reaction type - because it can't send it.

lib/types/redux-types.js
536–537 ↗(On Diff #19048)

Oh gotcha, thank you for elaborating this makes sense now, I will update the diff to have the reaction type in the error payload

lib/types/redux-types.js
539 ↗(On Diff #19296)

I was initially basing this type from the other send message redux types (SEND_TEXT_MESSAGE_STARTED, SEND_TEXT_MESSAGE_FAILED, SEND_TEXT_MESSAGE_SUCCESS, SEND_MULTIMEDIA_MESSAGE_STARTED, etc), but after taking a second look and based on what I did on D5782, it seems like it would be better to make loadingInfo required

atul requested changes to this revision.Dec 15 2022, 9:05 AM

Some questions about SendReactionMessagePayload

lib/types/message-types.js
547 ↗(On Diff #19332)

Can you explain why we have this newMessageInfos field?

  1. It looks like SendMessagePayload doesn't include any equivalent field? It looks like anything regarding message contents are in SendTextMessageRequest or SendMultimediaMessageRequest, etc.
  2. Why do we need to be sending an array of RawMessageInfos? Isn't it only possible to send one reaction at a time?
  3. Are you sure we need to be sending over the entire contents of RawMessageInfo?
  4. Can we narrow the type to RawReactionMessageInfo?
This revision now requires changes to proceed.Dec 15 2022, 9:05 AM
lib/types/message-types.js
547 ↗(On Diff #19332)

The reason we need to have newMessageInfos here, is because in D5782, we are using the mergeNewMessages function and it needs an array of RawMessageInfos as one of its parameters

Screenshot 2022-12-15 at 5.25.34 PM.png (476×1 px, 107 KB)

@ginsu you should Re-Request Review to move this back to @atul's queue if you think his question has been addressed

ginsu requested review of this revision.Dec 16 2022, 7:05 AM

Accepting but I think the creation of the RawMessageInfos array can be pushed down to message-reducer to keep things cleaner.

lib/types/message-types.js
547 ↗(On Diff #19332)

Can we handle the creation of array of RawMessageInfos in message-reducer instead? I think that'd be cleaner than creating the array here just because a function called down the line needs it in that format.

This revision is now accepted and ready to land.Dec 18 2022, 8:08 PM
In D5769#176948, @atul wrote:

Accepting but I think the creation of the RawMessageInfos array can be pushed down to message-reducer to keep things cleaner.

@atul would you be okay if we punted this down to when I work on the displaying reactions optimistically task later in the week? I feel like this will go hand in hand with that task

In D5769#177981, @ginsu wrote:
In D5769#176948, @atul wrote:

Accepting but I think the creation of the RawMessageInfos array can be pushed down to message-reducer to keep things cleaner.

@atul would you be okay if we punted this down to when I work on the displaying reactions optimistically task later in the week? I feel like this will go hand in hand with that task

I mean we're defining the interface here with SendReactionMessagePayload so ideally we'd correct it right now, but I get that it'd be helpful to get as much of this stack landed as soon as possible so it's probably fine to punt it for now.