[native] introduce localID field to onPressReact function
ClosedPublic
Actions

Authored by ginsu on Dec 30 2022, 4:02 PM.

Details

Reviewers

atul
tomek

Commits

rCOMMf95304880919: [native] introduce localID field to onPressReact function

Summary

introduce localID field to onPressReact function. This diff also handles creating the localID variable on the client using the localIDPrefix and nextLocalID state

Depends on D6119
Linear Task: ENG-2519

Test Plan

Please watch this demo video to show that we were able to get the localID succesfully from the native client to db:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Dec 30 2022, 4:02 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 30 2022, 4:02 PM

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

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

ginsu added reviewers: atul, tomek.

ginsu added a parent revision: D6119: [web] introduce localID field to useOnClickReact hook.

ginsu attached a referenced file: F312096: Screen Recording 2022-12-30 at 7.00.32 PM.mov. (Show Details)

ginsu added a child revision: D6121: [web] add starting payload to send reaction message dispatch action promise.Dec 30 2022, 4:09 PM

ginsu added a child revision: D6122: [native] add starting payload to send reaction message dispatch action promise.Dec 30 2022, 4:11 PM

Harbormaster completed remote builds in B14989: Diff 20458.Dec 30 2022, 4:13 PM

ginsu requested review of this revision.Dec 30 2022, 4:13 PM

ashoat added inline comments.Dec 30 2022, 9:08 PM

native/chat/reaction-message-utils.js
18–31 ↗	(On Diff #20458)	Lots of props... might be better to use an object

tomek requested changes to this revision.Jan 2 2023, 5:37 AM

tomek added inline comments.

native/chat/reaction-message-utils.js
17–28 ↗	(On Diff #20458)	Not sure if this makes a big difference for us, but it would be better if we could make sure that type parameter of route is the same as for navigation. Currently, it is possible to have e.g. `route` for `'TextMessageTooltipModal'` and `navigation` for `MultimediaMessageTooltipModal`.
31–39 ↗	(On Diff #20458)	Can't we make `reactionMessageLocalID` non-nullable so that the invariant isn't necessary?

This revision now requires changes to proceed.Jan 2 2023, 5:37 AM

ginsu added inline comments.Jan 3 2023, 2:31 PM

native/chat/reaction-message-utils.js
18–31 ↗	(On Diff #20458)	Created a task to address this since the changes are a bit more involved and I would consider them outside the scope of this diff
31–39 ↗	(On Diff #20458)	I can definitely do that. My original thinking for this was that I thought it would be a bit weird if the other tooltip onPress action functions like copy and thread had access to this field, but if we still think setting `reactionMessageLocalID` to be non-nullable is the best solution forward, I will make the necessary update

address feedback

Harbormaster completed remote builds in B15058: Diff 20544.Jan 3 2023, 2:46 PM

tomek accepted this revision.Jan 5 2023, 4:14 AM

tomek added inline comments.

native/chat/reaction-message-utils.js
31–39 ↗	(On Diff #20458)	You're right. If we have to choose between this being optional and having to provide this value for unrelated actions, then keeping it optional is better.