Details

Reviewers

atul
rohan
tomek

Commits

rCOMM1822281a8e27: [native] introduce onPressReact to reaction message utils

Summary

introduce onPressReact function to new file called reaction-message-utils. Some things to note about this method, for now, I am hardcoding the like emoji since at this current state we only support liking a message. I am thinking that this will change in subsequent diffs when I introduce "unliking" and different reactions. Also, since we are not doing retry behavior for reactions (at least for now) I figured the best way to handle an error for the user would be to show an alert saying that the reaction couldn't be sent. This is something we also do in message report utils

Depends on D5770 and D5769

Linear Task:
ENG-2246

Test Plan

This will be tested further in subsequent diffs. Ran this locally on native and nothing crashed. Also helped me get to the point where I could push a reaction message into the DB (see screenshot below)

Screenshot 2022-11-28 at 7.16.55 PM.png (2×3 px, 1 MB)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Nov 30 2022, 3:36 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptNov 30 2022, 3:36 PM

ginsu edited the summary of this revision. (Show Details)Nov 30 2022, 3:46 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added a parent revision: D5770: [lib] introduce sendReactionMessage to message actions.

ginsu attached a referenced file: F267584: Screenshot 2022-11-28 at 7.16.55 PM.png. (Show Details)

ginsu added reviewers: atul, rohan, tomek.

Harbormaster completed remote builds in B13901: Diff 19050.Nov 30 2022, 3:48 PM

ginsu requested review of this revision.Nov 30 2022, 3:48 PM

ginsu edited the summary of this revision. (Show Details)Nov 30 2022, 3:55 PM

ginsu added a parent revision: D5769: [lib] introduced sendReactionMessageActionTypes.

ginsu added a child revision: D5782: [lib] introduce sendReactionMessage success case to message reducers.Nov 30 2022, 4:52 PM

ginsu added a child revision: D5783: [native] introduced react option to the message tooltip.Nov 30 2022, 4:57 PM

tomek requested changes to this revision.Dec 1 2022, 4:22 AM

tomek added inline comments.

native/chat/reaction-message-utils.js
14–22 ↗	(On Diff #19050)	This approach seems to be overly complicated. Can't we just write a function that receives `messageID`, `threadID` and `reaction`? This function can be named after an action that is performed, so `react` or `sendReaction`.
41 ↗	(On Diff #19050)	It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails.
46 ↗	(On Diff #19050)	Is there a reason to set cancelable to false?

This revision now requires changes to proceed.Dec 1 2022, 4:22 AM

addressed tomek's comments

ginsu added inline comments.Dec 5 2022, 1:59 PM

native/chat/reaction-message-utils.js
41 ↗	(On Diff #19050)	Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed.
46 ↗	(On Diff #19050)	Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context I also found the alert on report-utils to follow the same patern

Harbormaster completed remote builds in B13973: Diff 19146.Dec 5 2022, 2:05 PM

ashoat added inline comments.Dec 5 2022, 2:48 PM

native/chat/reaction-message-utils.js
69 ↗	(On Diff #19146)	I think any modal with `text: 'OK'` can probably be `cancelable: true`

tomek accepted this revision.Dec 6 2022, 3:04 AM

tomek added inline comments.

native/chat/reaction-message-utils.js
16–17 ↗	(On Diff #19146)	Shouldn't we also include robotext here?
31 ↗	(On Diff #19146)	Not sure how it is used later, but it feels like we might want a toggle behavior. But it is also possible that there will be another function that removes a reaction.
76–79 ↗	(On Diff #19146)	As a first iteration it is ok, but in the future we can consider providing `startingPayload` so that we can optimistically show a reaction immediately.

Looks good, please make sure to answer all of @tomek's questions before landing

native/chat/reaction-message-utils.js
21–25 ↗	(On Diff #19146)	Personally would prefer the following, but doesn't matter at all so whatever you prefer. const messageID = route.params.item.messageInfo.id; invariant(messageID, 'messageID should be set'); const threadID = route.params.item.threadInfo.id; invariant(threadID, 'threadID should be set');
31 ↗	(On Diff #19146)	Believe that this is coming later in the stack, right @ginsu?

This revision is now accepted and ready to land.Dec 6 2022, 5:52 PM

ginsu added a child revision: D5853: [native] introduced useCanCreateReactionFromMessage function to reaction message utils.Dec 12 2022, 1:17 PM

addressed feedback

ginsu added inline comments.Dec 12 2022, 1:29 PM

native/chat/reaction-message-utils.js
31 ↗	(On Diff #19146)	Yup, sorry this took a while to address, but will be putting up a diff with toggle/unliking behavior shortly
76–79 ↗	(On Diff #19146)	Definitely, will be covering displaying reactions optimistically in the future as a follow-up task