Page MenuHomePhabricator

[lib] Calculate currentAsOf for every keyserver in mergeNewMessages
ClosedPublic

Authored by inka on Nov 14 2023, 6:31 AM.
Tags
None
Referenced Files
F3349756: D9882.id33210.diff
Fri, Nov 22, 7:00 PM
Unknown Object (File)
Mon, Oct 28, 12:14 AM
Unknown Object (File)
Oct 15 2024, 10:09 PM
Unknown Object (File)
Sep 28 2024, 3:55 AM
Unknown Object (File)
Sep 28 2024, 3:55 AM
Unknown Object (File)
Sep 28 2024, 3:55 AM
Unknown Object (File)
Sep 28 2024, 3:54 AM
Unknown Object (File)
Sep 28 2024, 3:49 AM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-5723/update-mergenewmessages-function
We need to calculate currentAsOf for every keyserver. Previously, we calculated max of orderedNewMessageInfos[0].time and updatedMessageStore.currentAsOf[keyserverID].
orderedNewMessageInfos are messages ordered by time, and the first element was the newest message. We take orderedNewMessageInfos and find the newest message for each keyserver with findNewestMessageTimePerKeyserver.
updatedMessageStore.currentAsOf is currently the same as processedMessageStore.currentAsOf - processedMessageStore is calculated from updatedMessageStore by applying ops with processMessageStoreOperations. But if in the future we added to ops something that changes currentAsOf, we would want to use that value. So this stays correct, and is easier to maintain

Test Plan

Logged the created messageStore and checked that currentAsOf is a "map" from keyserverID to timestamp.
Checked that the value calculated is the timestamp of the newest message from that keyserver. Tested that if a new message arrives it gets updated correctly. Tested that if older messages are fetched the value doesn't change

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.Nov 14 2023, 6:58 AM
michal added inline comments.
lib/reducers/message-reducer.js
610–613 ↗(On Diff #33210)

Could we just do this? processedMessageStore is included in currentAsOf

This revision is now accepted and ready to land.Nov 14 2023, 7:13 AM
lib/reducers/message-reducer.js
610–613 ↗(On Diff #33210)

currentAsOf has values only for keyservers that were present in orderedNewMessageInfos (you can see we iterate over newestMessageTimePerKeyserver that is based on orderedNewMessageInfos). This may not include all keyservers present in processedMessageStore

In other words some messages are passed to mergeNewMessages, and we calculate the new currentAsOf based on them, but they may not include some keyservers that are present in our store.

This may be the wrong diff to ask this question on, but I'm wondering – how will the MessageStore (and other stores such as ThreadStore) be affected when a keyserver is dropped from the KeyserverStore?

This may be the wrong diff to ask this question on, but I'm wondering – how will the MessageStore (and other stores such as ThreadStore) be affected when a keyserver is dropped from the KeyserverStore?

I actually asked @ginsu about this regarding the D9829, and as far as I understand for now we are just leaving all data related to a keyserver when we leave it. @ginsu is that right?

and as far as I understand for now we are just leaving all data related to a keyserver when we leave it. @ginsu is that right?

That seems very concerning... so when you leave a keyserver, you still see all of that keyserver's communities in the navigation side drawer, and all of the chats still appear in your inbox?

I sometimes worry that if I stop monitoring every single diff, people on the team will make the weirdest product decisions without surfacing them to me. I'm not sure how this decision was made (@ginsu?) but in the future, this sort of decision ABSOLUTELY must be run by me.

Please make a task and link it here before landing.

lib/reducers/message-reducer.js
595 ↗(On Diff #33210)

Please add a type to this collection. The new Flow requires it

as far as I understand for now we are just leaving all data related to a keyserver when we leave it. @ginsu is that right?

Sorry if I caused any miscommunication but this assumption should just live in the context of D9829 which just handles removing a disconnected server from the keyserver store since these keyservers are not getting connected to anything or holds any community data atm.