[lib] introduce sendReactionMessage success case to message reducers
ClosedPublic
Actions

Authored by ginsu on Nov 30 2022, 4:45 PM.

Details

Reviewers

atul
rohan
tomek

Commits

rCOMM7116755f56ed: [lib] introduce sendReactionMessage success case to message reducers

Summary

Introduced a new case to reduceMessageStore which was when the sendReactionMessageActionTypes success type gets dispatched. Like a lot of the other cases in reduceMessageStore when this case is true, it returns the new message store with the merged messages in them, so in this case the newly created reaction message

Depends on D5780

Linear Task: ENG-2246

Test Plan

Will continue testing in subsequent diffs. Ran this locally on native/web and nothing crashed. When I tried liking a message on my local environment this is the interaction I got, which I felt was a really strong indicator that things are working:

Please note that the reaction message in this demo is considered an "unsupported message" type and will be resolved in subsequent diffs

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Nov 30 2022, 4:45 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptNov 30 2022, 4:45 PM

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

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

ginsu added reviewers: atul, rohan, tomek.

ginsu added a parent revision: D5780: [native] introduce onPressReact to reaction message utils.

ginsu attached a referenced file: F267709: Screen Recording 2022-11-30 at 7.41.13 PM.mov. (Show Details)

Harbormaster completed remote builds in B13905: Diff 19057.Nov 30 2022, 4:57 PM

ginsu requested review of this revision.Nov 30 2022, 4:57 PM

I have a strong feeling that we don't want to show it as a robotext - it should be an icon with a number and a message shouldn't appear on a list at all.

lib/reducers/message-reducer.js
848–868 ↗	(On Diff #19057)	Is there a reason behind not merging it with the previous `if`?

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

The robotext thing is confusing... I get that that's probably meant as some sort of intermediate state, but I don't think we should be treating reactions as a "robotext" type

@tomek @ashoat

Should have made this more clear in my test plan, but the demo still has reaction messages as a shimmed "unsupported message", and that is why it is a robotext. I am currently working on the unshimming and supporting the reaction message, and when that is done, liking a message will look something like this

ginsu attached a referenced file: F268655: Screen Recording 2022-12-01 at 12.43.58 PM.mov. (Show Details)Dec 1 2022, 9:50 AM

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

Looks great!! Thanks for clarifying

ginsu added a child revision: D5783: [native] introduced react option to the message tooltip.Dec 5 2022, 12:11 PM

addressed tomek's comments

ginsu added inline comments.Dec 5 2022, 2:09 PM

lib/reducers/message-reducer.js
848–868 ↗	(On Diff #19057)	Was initially thinking I would need to do some logic before merging, but now, after getting most of the stack done locally I don't think that is the case

Harbormaster completed remote builds in B13974: Diff 19147.Dec 5 2022, 2:19 PM

ginsu added a child revision: D5811: [lib] introduced reactions field to chatMessageItems.Dec 5 2022, 5:17 PM

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

tomek added inline comments.

lib/reducers/message-reducer.js
855	At some point we can consider displaying reaction optimistically on start

ashoat added inline comments.Dec 6 2022, 7:01 AM

lib/reducers/message-reducer.js
855	I'd agree that showing it optimistically would be good. Not sure how easy that would be... happy to have it covered by a follow-up task on Linear (please create before landing)