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
Details
Details
Tested in combination with the following diffs, where I construct RobotextChatMessageInfoItem with an array of messageInfos to represent combined messages (ENG-9559)
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| 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 |
| 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:
So basically, both options are possible:
And, after combining scenarios 1 and 2 it is possible that
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. |
| lib/shared/chat-message-item-utils.js | ||
|---|---|---|
| 46 ↗ | (On Diff #45045) |
No problem, I'll find a better one.
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) |
I had the same thought, and made sure we don't collapse messages with reactions / sidebars in D13687. |