Page MenuHomePhabricator

[lib] Include an isPinned flag in ChatMessageInfoItem
ClosedPublic

Authored by rohan on Mar 25 2023, 6:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 8, 1:38 AM
Unknown Object (File)
Fri, Mar 8, 1:38 AM
Unknown Object (File)
Fri, Mar 8, 1:38 AM
Unknown Object (File)
Tue, Mar 5, 11:50 PM
Unknown Object (File)
Tue, Mar 5, 11:50 PM
Unknown Object (File)
Tue, Mar 5, 11:16 PM
Unknown Object (File)
Tue, Mar 5, 11:16 PM
Unknown Object (File)
Tue, Mar 5, 11:15 PM

Details

Summary

Each message on the client needs to be aware if it's pinned or not - the best way I found to do this is in chat-selectors when we compose ChatMessageInfoItem. I purposely didn't include isPinned in RobotextChatMessageInfoItem since robotext messages cannot be pinned.

My main reference was @ginsu's reaction stack for this from D5811. I created a separate for (...) loop to avoid creating conditional checks in the one used for reactions, but if it'll be better to merge the two and perform different actions based on messageInfo.type, I can do that as well.

Linear: https://linear.app/comm/issue/ENG-3392/include-an-ispinned-flag-in-chatmessageinfoitem

Depends on D7148

Test Plan

Open up a thread on web and confirm that the messages that are pinned on the server have a isPinned flag of true on the client

Database:

Screenshot 2023-03-25 at 9.35.13 AM.png (730×2 px, 181 KB)

Client:

Screenshot 2023-03-25 at 9.34.37 AM.png (950×828 px, 228 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan requested review of this revision.Mar 25 2023, 6:59 AM
lib/selectors/chat-selectors.js
437 ↗(On Diff #24135)

This should handle the targetMessagePinStatusMap having the key originalMessageInfo.id, so we'd return true === true or false === true, and also where the key doesn't exist and we have null === true.

Screenshot 2023-03-25 at 10.14.10 AM.png (242×2 px, 67 KB)

ashoat added a subscriber: kuba.

Also throwing in @kuba, since he has been looking at chat-selectors.js recently too

It wasn't initially obvious to me that this is the right approach. But after some time it seems to make sense.

So basically, this code handles only the pin icon next to a message on message list. And it's not intended to provide data for pinned list, right?

Because it is only for message list, we are guaranteed to have all the messages between the most recent and pinned messages, and can conclude the status based on messages in between.

The only concern is that we probably don't pin messages in transaction. That means that we can successfully update pin status in the db, but fail to create TOGGLE_PIN message. A lot more reliable approach would be to add a field to a message saying that it is pinned.

lib/selectors/chat-selectors.js
432–438 ↗(On Diff #24135)

Why this code is so complicated?

This revision is now accepted and ready to land.Mar 27 2023, 2:48 AM
In D7175#213494, @tomek wrote:

So basically, this code handles only the pin icon next to a message on message list. And it's not intended to provide data for pinned list, right?

Yeah, exactly!

The only concern is that we probably don't pin messages in transaction. That means that we can successfully update pin status in the db, but fail to create TOGGLE_PIN message. A lot more reliable approach would be to add a field to a message saying that it is pinned.

You're right, I don't believe we pin the messages in a transaction. So the DB for the message gets updated on pin/unpin (pinned and pin_time), and we separately create a message of type === TOGGLE_PIN. So are you suggesting instead of determining isPinned by the messages, we instead determine isPinned by it's state in the DB (pinned = 1 or pinned = 0)?

Simplify isPinned calculation

In D7175#214618, @rohan wrote:
In D7175#213494, @tomek wrote:

So basically, this code handles only the pin icon next to a message on message list. And it's not intended to provide data for pinned list, right?

Yeah, exactly!

The only concern is that we probably don't pin messages in transaction. That means that we can successfully update pin status in the db, but fail to create TOGGLE_PIN message. A lot more reliable approach would be to add a field to a message saying that it is pinned.

You're right, I don't believe we pin the messages in a transaction. So the DB for the message gets updated on pin/unpin (pinned and pin_time), and we separately create a message of type === TOGGLE_PIN. So are you suggesting instead of determining isPinned by the messages, we instead determine isPinned by it's state in the DB (pinned = 1 or pinned = 0)?

Yeah, that would be more reliable, but also more complicated. We have to decide if it makes sense to spend time on it now. At least we should create a task to track that.

In D7175#214623, @tomek wrote:

Yeah, that would be more reliable, but also more complicated. We have to decide if it makes sense to spend time on it now. At least we should create a task to track that.

Here's the task: https://linear.app/comm/issue/ENG-3421/consider-determining-ispinned-based-on-the-messages-state-in-the-db

This revision was landed with ongoing or failed builds.Apr 11 2023, 9:00 AM
This revision was automatically updated to reflect the committed changes.