Page MenuHomePhabricator

[lib] add relative user info to users in messageReactionInfo
ClosedPublic

Authored by ginsu on Jan 18 2023, 9:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, May 13, 5:42 AM
Unknown Object (File)
Apr 17 2024, 11:00 AM
Unknown Object (File)
Apr 17 2024, 11:00 AM
Unknown Object (File)
Apr 17 2024, 11:00 AM
Unknown Object (File)
Apr 17 2024, 11:00 AM
Unknown Object (File)
Apr 17 2024, 11:00 AM
Unknown Object (File)
Apr 17 2024, 11:00 AM
Unknown Object (File)
Apr 17 2024, 11:00 AM
Subscribers

Details

Summary

Right now users in messageReactionInfo returns a set of user ids of all the users who have liked a message. Now that we want to show information about the user like their username, I needed to convert users from a set of user IDs to an array of RelativeUserInfos. Also needed to make some changes to the reaction-utils test suite since we are no longer using a set as our data structure.


Linear Task: ENG-2718

Test Plan

Logged out messageReactionInfo to show that my changes are behaving as expected (see screenshots below):

Before:

Screenshot 2023-01-18 at 11.42.56 AM.png (1×3 px, 1 MB)

After:

Screenshot 2023-01-18 at 11.41.10 AM.png (1×3 px, 1 MB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: tomek, atul.
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 18 2023, 9:07 AM
Harbormaster failed remote builds in B15440: Diff 21049!
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 18 2023, 9:16 AM
Harbormaster failed remote builds in B15441: Diff 21051!
ginsu requested review of this revision.Jan 18 2023, 9:59 AM
tomek requested changes to this revision.Jan 19 2023, 2:39 AM
tomek added inline comments.
lib/selectors/chat-selectors.js
433–435 ↗(On Diff #21052)

It should never happen that we don't have an entry in the map, but Flow doesn't know that. In this case I may even consider adding an invariant, as falsy value here would be a programmer error. But I think it is a better idea to analyze why we have to check this condition.
It seems that we're maintaining two collections reactionUsersSet and reactionsUserInfoMap independently, while they are really closely related. So how about removing reactionUsersSet completely and instead add or delete an entry in reactionsUserInfoMap depending on like / unlike action? So this:

if (messageInfo.action === 'add_reaction') {
  messageReactionUsersSet.add(messageInfo.creator.id);
} else {
  messageReactionUsersSet.delete(messageInfo.creator.id);
}
if (!reactionsUserInfoMap.has(messageInfo.creator.id)) {
  reactionsUserInfoMap.set(messageInfo.creator.id, messageInfo.creator);
}

should be replaced with:

if (messageInfo.action === 'add_reaction') {
  reactionsUserInfoMap.set(messageInfo.creator.id, messageInfo.creator);
} else {
  reactionsUserInfoMap.delete(messageInfo.creator.id);
}

And targetMessageReactionsMap would become new Map<string, Map<string, Map<string, RelativeUserInfo>>>() (we can consider introducing some named types to make this more readable).

What do you think about it?

This revision now requires changes to proceed.Jan 19 2023, 2:39 AM
lib/selectors/chat-selectors.js
433–435 ↗(On Diff #21052)

Makes sense! My initial approach was based on this comment from when I first introduced the reactions field to chatMessageItems, but what you have suggested now seems like a better solution overall

tomek added inline comments.
lib/selectors/chat-selectors.js
334 ↗(On Diff #21083)

Wondering if we can use TargetMessageReactions here

This revision is now accepted and ready to land.Jan 20 2023, 7:30 AM