Page MenuHomePhabricator
Differential D6875

[lib/native/web] switch reactions data structure from map to plain object
ClosedPublic
Actions

Authored by ginsu on Feb 23 2023, 2:00 PM.
Tags
None
Referenced Files
F1803816: D6875.diff
Mon, May 20, 6:48 PM
Unknown Object (File)
Fri, May 10, 11:37 PM
Unknown Object (File)
Fri, May 10, 11:37 PM
Unknown Object (File)
Fri, May 10, 11:36 PM
Unknown Object (File)
Fri, May 10, 11:36 PM
Unknown Object (File)
Fri, May 10, 11:36 PM
Unknown Object (File)
Fri, May 10, 11:36 PM
Unknown Object (File)
Fri, May 10, 11:36 PM
View All 76 Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
atul
tomek
ashoat
rohan
Commits
rCOMMbe977b7db9fb: [lib/native/web] switch reactions data structure from map to plain object
Summary

In dev mode the app was warning whenever a user pressed a message that "Non-serializble values were found in the navigation state". This was caused becasue reactions were stored in a map. To fix this, I switched the data structure from a map to a plain object

Linear Task: ENG-2851

Test Plan

When a user taps on a message now the warning message about "non-serializable..." no longer shows up

Before:

After:

In addition tested on both native and web the following to make sure there were no regressions with reactions:

  • reacting to a message (text/multimedia/robotext)
  • unreacting to a message (text/multimedia/robotext)
  • viewing who has liked a message
  • sending multiple reactions to the same message

also flow

Diff Detail

Repository
rCOMM Comm
Branch
eng-2851
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu created this revision.Feb 23 2023, 2:00 PM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptFeb 23 2023, 2:00 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Feb 23 2023, 2:02 PM
Harbormaster failed remote builds in B16859: Diff 23038!
ginsu edited the summary of this revision. (Show Details)Feb 23 2023, 2:14 PM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, tomek, ashoat, rohan.
ginsu attached a referenced file: F399481: Screen Recording 2023-02-23 at 4.49.23 PM.mov. (Show Details)
ginsu attached a referenced file: F399480: Screen Recording 2023-02-23 at 4.47.42 PM.mov. (Show Details)
ginsu updated this revision to Diff 23041.Feb 23 2023, 2:42 PM
ginsu edited the test plan for this revision. (Show Details)

rebase

Harbormaster completed remote builds in B16862: Diff 23041.Feb 23 2023, 2:57 PM
ginsu requested review of this revision.Feb 23 2023, 2:57 PM
ashoat requested changes to this revision.Feb 24 2023, 8:39 AM
ashoat added inline comments.
lib/selectors/chat-selectors.js
297 ↗(On Diff #23041)

Make this $ReadOnly

470 ↗(On Diff #23041)

If we're going to be storing an object anyways, should we use an object throughout? It would allow us to avoid this marshalling step

lib/shared/reaction-utils.test.js
24–27 ↗(On Diff #23041)

There is literally no point to constructing a Map and then immediately converting it to an object... please just use an object from the start...

native/chat/reaction-message-utils.js
48–51 ↗(On Diff #23041)

Would this be cleaner as a ternary?

web/chat/message-tooltip.react.js
124–127 ↗(On Diff #23041)

Would this be cleaner as a ternary?

This revision now requires changes to proceed.Feb 24 2023, 8:39 AM
ginsu updated this revision to Diff 23093.Feb 24 2023, 4:48 PM

address ashoat's comments

ginsu edited the test plan for this revision. (Show Details)Feb 24 2023, 4:49 PM
Harbormaster completed remote builds in B16902: Diff 23093.Feb 24 2023, 5:04 PM
ashoat accepted this revision.Feb 26 2023, 4:32 AM

Perfect! Love the small extra improvements you made

This revision is now accepted and ready to land.Feb 26 2023, 4:32 AM
ginsu updated this revision to Diff 23101.Feb 26 2023, 11:04 AM

rebase before landing

Harbormaster completed remote builds in B16910: Diff 23101.Feb 26 2023, 11:19 AM
Closed by commit rCOMMbe977b7db9fb: [lib/native/web] switch reactions data structure from map to plain object (authored by ginsu). · Explain WhyFeb 26 2023, 1:00 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMMbe977b7db9fb: [lib/native/web] switch reactions data structure from map to plain object.

Revision Contents
Changeset List

Diff 23038

View Options

lib/selectors/chat-selectors.js

Loading...
View Options

lib/shared/reaction-utils.js

Loading...
View Options

lib/shared/reaction-utils.test.js

Loading...
View Options

native/chat/composed-message.react.js

Loading...
View Options

native/chat/inline-engagement.react.js

Loading...
View Options

native/chat/message-reactions-modal.react.js

Loading...
View Options

native/chat/multimedia-message-utils.js

Loading...
View Options

native/chat/reaction-message-utils.js

Loading...
View Options

native/chat/robotext-message.react.js

Loading...
View Options

native/chat/utils.js

Loading...
View Options

native/types/chat-types.js

Loading...
View Options

web/chat/composed-message.react.js

Loading...
View Options

web/chat/inline-engagement.react.js

Loading...
View Options

web/chat/message-tooltip.react.js

Loading...
View Options

web/chat/robotext-message.react.js

Loading...
View Options

web/modals/chat/message-reactions-modal.react.js

Loading...