Page MenuHomePhabricator

Populate messageIDs fileds of threads with new messages in messageStore
ClosedPublic

Authored by marcin on Jul 14 2022, 4:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 10:33 AM
Unknown Object (File)
Oct 22 2024, 3:39 PM
Unknown Object (File)
Oct 20 2024, 6:52 AM
Unknown Object (File)
Oct 18 2024, 7:59 AM
Unknown Object (File)
Oct 17 2024, 1:56 AM
Unknown Object (File)
Oct 7 2024, 2:14 PM
Unknown Object (File)
Oct 7 2024, 2:14 PM
Unknown Object (File)
Oct 7 2024, 2:14 PM

Details

Summary

This differential checks for any messages in messageStore that are not yet present in messageIDs fields in threads field of messageStore and inserts them. This way new messages that are already stored in SQLite can be directly displayed without interaction with server.

Test Plan

Build the Android app, launch it, log in and then kill it. Open web app with another client and send several messages to android client and then kill keyserver (Ctrl + C in terminal). Open Android app and ensure that despite being disconnected from server those new messages delivered via notification are correctly displayed. It needs to ba Android machine since we do not yet store notification in SQLite as they are delivered on iOS.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

At a high level this seems like an okay approach for now.


At some point, I think it would be better to either

A. persist threads in SQLite
B. compute messageStore.threads on the C++ side (something SQLite probably lends itself well to) and pass it over (maybe setMessageStoreMessages => setMessageStore)

Personally leaning towards option B.

Either way, I think it would be good to move this complexity away from the setMessageStoreMessages handler.

lib/reducers/message-reducer.js
1290–1291 ↗(On Diff #14460)

We should remove this, and move it to where it's more relevant

1298–1303 ↗(On Diff #14460)

Something like this would allow us to avoid having to index in on threads with threadID.

I think it's slightly cleaner, but up to you on what you prefer.


Also think something like existingMessageIDs might be a better name for the Set() than displayedMessages, but again up to whatever you prefer.

1304 ↗(On Diff #14460)

Eh, it's not really the threadIDs that need to be resorted.

Maybe something like threadsThatNeedMsgIDsResorted? That's kind of verbose, but anything that makes it more clear that it's not threadIDs being sorted should be good.

1308 ↗(On Diff #14460)

This seems like a reasonable position to move the comment to?

This revision is now accepted and ready to land.Jul 18 2022, 3:08 PM
In D4532#130110, @atul wrote:

At a high level this seems like an okay approach for now.


At some point, I think it would be better to either

A. persist threads in SQLite
B. compute messageStore.threads on the C++ side (something SQLite probably lends itself well to) and pass it over (maybe setMessageStoreMessages => setMessageStore)

Personally leaning towards option B.

Either way, I think it would be good to move this complexity away from the setMessageStoreMessages handler.

Agree that B makes the most sense! I don't think it would be too difficult. Could be a good starter task – @atul, would you mind creating a Linear task and adding the "Starter" label? cc @palys-swm

By the way, I think it would probably be just as reasonable to compute messageStore.threads on the JS side, right? I think in general we'd rather have code in JS than in C++ / Rust.

lib/reducers/message-reducer.js
1298–1303 ↗(On Diff #14460)

We need to iterate this way:

for (const threadID in threads) {
      threads[threadID].messageIDs.forEach(msgID => {
        existingMessageIDs.add(msgID);
    });
}

Attempt to iterate over threads with of semantics results in flow error.

Reminder for somebody to create task above! I requested @atul, but @marcin could do it too – either way works

Reminder for somebody to create task above! I requested @atul, but @marcin could do it too – either way works

https://linear.app/comm/issue/ENG-1430/compute-messagestorethreads-on-the-c-side

Rebase master before landing