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.
Details
Details
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
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-1350
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
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? |
Comment Actions
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. |