Details

Reviewers

ginsu
atul
tomek
• kuba

Commits

rCOMM4deca77f34e7: [lib] Include an isPinned flag in ChatMessageInfoItem

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 created this revision.Mar 25 2023, 6:44 AM

Herald added a subscriber: ashoat. · View Herald TranscriptMar 25 2023, 6:44 AM

rohan edited the summary of this revision. (Show Details)Mar 25 2023, 6:45 AM

rohan edited the test plan for this revision. (Show Details)

rohan attached a referenced file: F444831: Screenshot 2023-03-25 at 9.35.13 AM.png. (Show Details)

rohan attached a referenced file: F444832: Screenshot 2023-03-25 at 9.34.37 AM.png. (Show Details)

Harbormaster completed remote builds in B17633: Diff 24135.Mar 25 2023, 6:59 AM

rohan requested review of this revision.Mar 25 2023, 6:59 AM

rohan added inline comments.Mar 25 2023, 7:14 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`.

rohan attached a referenced file: F444849: Screenshot 2023-03-25 at 10.14.10 AM.png. (Show Details)Mar 25 2023, 7:14 AM

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

rohan added a child revision: D7199: [keyserver] Add pinned_count column to MariaDB threads table.Mar 27 2023, 9:50 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.

Harbormaster completed remote builds in B17748: Diff 24288.Mar 28 2023, 9:25 AM

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

Rebase

Harbormaster completed remote builds in B17882: Diff 24497.Mar 31 2023, 4:41 AM

rohan edited child revisions, added: D7202: [native] Add pinned_count column to SQLite threads table; removed: D7199: [keyserver] Add pinned_count column to MariaDB threads table.Apr 7 2023, 8:27 AM

Rebase

Harbormaster completed remote builds in B18109: Diff 24809.Apr 7 2023, 9:13 AM

Rebase before landing

Harbormaster completed remote builds in B18150: Diff 24852.Apr 7 2023, 2:47 PM

Rebase before landing

This revision was landed with ongoing or failed builds.Apr 11 2023, 9:00 AM

Closed by commit rCOMM4deca77f34e7: [lib] Include an isPinned flag in ChatMessageInfoItem (authored by RohanK6 <rohan.khanderia@gmail.com>). · Explain Why

This revision was automatically updated to reflect the committed changes.