[lib][native][web] Add support for rendering array of messages in RobotextChatMessageInfoItem
ClosedPublic
Actions

Authored by ashoat on Oct 10 2024, 3:32 AM.

Details

Reviewers

tomek

Commits

rCOMMe5fadce0c2a0: [lib][native][web] Add support for rendering array of messages in…

Summary

This diff makes it possible to render a robotext that corresponds to a list of messages.

We don't actually render a list of messages anywhere yet.

Depends on D13685

Test Plan

Tested in combination with the following diffs, where I construct RobotextChatMessageInfoItem with an array of messageInfos to represent combined messages (ENG-9559)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Oct 10 2024, 3:32 AM

ashoat added a child revision: D13687: [lib] Introduce MessageSpec.mergeIntoPrecedingRobotextMessageItem.

Harbormaster completed remote builds in B32170: Diff 45045.Oct 10 2024, 5:04 AM

ashoat requested review of this revision.Oct 10 2024, 5:04 AM

ashoat added inline comments.Oct 10 2024, 2:13 PM

lib/shared/edit-messages-utils.js
103 ↗	(On Diff #45045)	We made it optional here because for the first time, some messages aren't targetable (merged messages can't be targeted)
lib/shared/reaction-utils.js
79 ↗	(On Diff #45045)	We made it optional here because for the first time, some messages aren't targetable (merged messages can't be targeted)
82–84 ↗	(On Diff #45045)	This stuff only matters when the message is targetable anyways, since we will return `false` if not
lib/shared/sidebar-utils.js
200 ↗	(On Diff #45045)	We made it optional here because for the first time, some messages aren't targetable (merged messages can't be targeted)
203–205 ↗	(On Diff #45045)	This stuff only matters when the message is targetable anyways, since we will return `false` if not
lib/utils/message-pinning-utils.js
30 ↗	(On Diff #45045)	We made it optional here because for the first time, some messages aren't targetable (merged messages can't be targeted)
native/chat/utils.js
156–159 ↗	(On Diff #45045)	We only need to show usernames for composable messages. Robotext messages don't show usernames
native/search/message-search.react.js
229–249 ↗	(On Diff #45045)	Message search currently only returns text messages, but I added this code here for future-proofing
web/components/message-result.react.js
40 ↗	(On Diff #45045)	We only need to show usernames for composable messages. Robotext messages don't show usernames
web/selectors/thread-selectors.js
67 ↗	(On Diff #45045)	We made it optional here because for the first time, some messages aren't targetable (merged messages can't be targeted)
web/tooltips/tooltip-action-utils.js
187–194 ↗	(On Diff #45045)	We only care about text messages for replies, which are always composable and never combined
226–232 ↗	(On Diff #45045)	We only care about text messages for copies, which are always composable and never combined

tomek accepted this revision.Oct 11 2024, 2:40 PM

tomek added inline comments.

lib/shared/chat-message-item-utils.js
46 ↗	(On Diff #45045)	I think it is risky to use a separator that can be already a part of the message key - we're using the same character to separate the keyserver ID from the message ID. Can we use a different separator? Should we also sort the keys?
58–72 ↗	(On Diff #45045)	I think there are a couple of edge cases that should be handled: Someone added a user and created a sidebar from the robotext. Then another user was added. Two users were added after each other. We have only the second operation displayed on the client. Then a sidebar was created from the robotext. And then, the viewer scrolled up and fetched the second operation. So basically, both options are possible: There are two robotext messages next to each other and there's a sidebar created from the first one. There are two robotext messages next to each other and there's a sidebar created from the second one. And, after combining scenarios 1 and 2 it is possible that There are two robotext messages next to each other and there are sidebars created from both of them. All of these can be handled by making sure we're collapsing only messages that don't have reactions / sidebars. Just wanted to highlight the possible issues.

This revision is now accepted and ready to land.Oct 11 2024, 2:40 PM

ashoat added inline comments.Oct 14 2024, 12:07 AM

lib/shared/chat-message-item-utils.js
46 ↗	(On Diff #45045)	I think it is risky to use a separator that can be already a part of the message key - we're using the same character to separate the keyserver ID from the message ID. Can we use a different separator? No problem, I'll find a better one. Should we also sort the keys? `chat-selectors.js` calls `mergeIntoPrecedingRobotextMessageItem` on the messages in the order to oldest to newest. `mergeIntoPrecedingRobotextMessageItem` is then responsible for maintaing an expected / consistent order. I think it's okay to skip sorting here.
58–72 ↗	(On Diff #45045)	All of these can be handled by making sure we're collapsing only messages that don't have reactions / sidebars. I had the same thought, and made sure we don't collapse messages with reactions / sidebars in D13687.