Page MenuHomePhabricator
Differential D7175

[lib] Include an isPinned flag in ChatMessageInfoItem
ClosedPublic
Actions

Authored by rohan on Mar 25 2023, 6:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 31, 5:11 PM
Unknown Object (File)
Tue, Mar 25, 2:06 AM
Unknown Object (File)
Fri, Mar 21, 2:06 AM
Unknown Object (File)
Mon, Mar 17, 6:05 AM
Unknown Object (File)
Mon, Mar 17, 6:05 AM
Unknown Object (File)
Mon, Mar 17, 6:05 AM
Unknown Object (File)
Mon, Mar 17, 6:05 AM
Unknown Object (File)
Mon, Mar 17, 6:05 AM
View All Files
Subscribers
ashoat
ginsu
• kuba

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
Branch
pinned_messages
Lint
No Lint Coverage
Unit
No Test Coverage

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

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)

rohan attached a referenced file: F444849: Screenshot 2023-03-25 at 10.14.10 AM.png. (Show Details)Mar 25 2023, 7:14 AM
ashoat added a reviewer: • kuba.Mar 25 2023, 10:42 AM
ashoat added a subscriber: • kuba.

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

tomek accepted this revision.EditedMar 27 2023, 2:48 AM

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

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
rohan added a comment.Mar 28 2023, 9:09 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)?

rohan updated this revision to Diff 24288.Mar 28 2023, 9:10 AM

Simplify isPinned calculation

tomek added a comment.Mar 28 2023, 9:16 AM
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
rohan added a comment.Mar 28 2023, 10:55 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

rohan updated this revision to Diff 24497.Mar 31 2023, 4:16 AM

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
rohan updated this revision to Diff 24809.Apr 7 2023, 8:52 AM

Rebase

Harbormaster completed remote builds in B18109: Diff 24809.Apr 7 2023, 9:13 AM
rohan updated this revision to Diff 24852.Apr 7 2023, 2:26 PM

Rebase before landing

Harbormaster completed remote builds in B18150: Diff 24852.Apr 7 2023, 2:47 PM
rohan updated this revision to Diff 24987.Apr 11 2023, 8:45 AM

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.
RohanK6 <rohan.khanderia@gmail.com> added a commit: rCOMM4deca77f34e7: [lib] Include an isPinned flag in ChatMessageInfoItem.
Harbormaster completed remote builds in B18244: Diff 24987.Apr 11 2023, 9:09 AM

Revision Contents
Changeset List

PathSize
lib/
selectors/
28 lines

Diff 24135

View Options

lib/selectors/chat-selectors.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines