Paths

Table of Contentst

-
lib/reducers/
-
reducers/
1
message-reducer.js

[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	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	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 ↗	(On Diff #19147)	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 ↗	(On Diff #19147)	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)

Looks good, please make sure to create a task for displaying reactions optimistically before landing

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

ginsu mentioned this in D5769: [lib] introduced sendReactionMessageActionTypes.Dec 13 2022, 9:53 AM

ginsu added inline comments.Dec 18 2022, 7:32 PM

lib/reducers/message-reducer.js
855 ↗	(On Diff #19147)	Follow up task

rebase before landing

Harbormaster completed remote builds in B14596: Diff 19908.Dec 20 2022, 4:29 PM

Closed by commit rCOMM7116755f56ed: [lib] introduce sendReactionMessage success case to message reducers (authored by ginsu). · Explain WhyDec 20 2022, 4:31 PM

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMM7116755f56ed: [lib] introduce sendReactionMessage success case to message reducers.

Revision Contents
Changeset List

Path

Size

lib/

reducers/

message-reducer.js

8 lines

Diff 19057

View Options

lib/reducers/message-reducer.js

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines	networks:
- services-net		- services-net
build:		build:
dockerfile: services/identity/Dockerfile		dockerfile: services/identity/Dockerfile
context: ../		context: ../
image: commapp/identity-server:0.1		image: commapp/identity-server:0.1
container_name: identity-server		container_name: identity-server
ports:		ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051"		- "${COMM_SERVICES_PORT_IDENTITY}:50051"
		# localstack
		localstack:
		image: localstack/localstack
		container_name: localstack
		hostname: localstack
		ports:
		- "4566:4566"
		environment:
		- SERVICES=s3,dynamodb
		- DATA_DIR=${TMPDIR}/localstack/data
		- HOSTNAME_EXTERNAL=localstack
		volumes:
		jimUnsubmitted Not Done Inline Actions Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea. jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
		- ${TMPDIR}:/tmp/localstack
		networks:
		- services-net

[lib] introduce sendReactionMessage success case to message reducersClosedPublicActions