Device Sync Tweaks #2053
Device Sync Tweaks #2053
Conversation
e33a024 to
15ef977
Compare
There was a problem hiding this comment.
Great change and unit test 👍
Just want to make sure we have consensus on the user experience we want before landing, cc @nplasterer
| @@ -518,7 +518,7 @@ where | |||
|
|
|||
| pub async fn create_group_with_inbox_ids( | |||
| &self, | |||
| inbox_ids: &[InboxId], | |||
| inbox_ids: &[impl AsIdRef], | |||
There was a problem hiding this comment.
What is the idea behind this change?
There was a problem hiding this comment.
It's so that we have the option to pass in inbox_id as a &str
Before we had to call .to_string() on the .inbox_id() function like so: client.create_group_with_inbox_ids(bo.inbox_id().to_string())
Now the .to_string() is no longer required, and we can just client.create_group_with_inbox_ids(bo.inbox_id())
| @@ -278,7 +281,11 @@ where | |||
| /// This should be triggered when a new sync group appears, | |||
| /// indicating the presence of a new installation. | |||
| pub async fn add_new_installation_to_groups(&self) -> Result<(), DeviceSyncError> { | |||
| let groups = self.mls_store.find_groups(GroupQueryArgs::default())?; | |||
| let groups = self.mls_store.find_groups(GroupQueryArgs { | |||
| activity_after_ns: Some(now_ns() - NS_IN_DAY * 90), | |||
There was a problem hiding this comment.
Are we sure we want to throw away groups older than 90 days? I can imagine at least some people who would want to keep their historical conversations for record-keeping purposes.
There was a problem hiding this comment.
Ya I think if they are allowed groups we should sync all of them. @rygine has a proposal for a more advanced history transfer that allows users to have optionality of how much they want to preserver or not. But I don't think we need that in this PR.
xmtp_mls/src/groups/device_sync.rs
Outdated
| let groups = self.mls_store.find_groups(GroupQueryArgs::default())?; | ||
| let groups = self.mls_store.find_groups(GroupQueryArgs { | ||
| activity_after_ns: Some(now_ns() - NS_IN_DAY * 90), | ||
| consent_states: Some(vec![ConsentState::Allowed, ConsentState::Unknown]), |
There was a problem hiding this comment.
I'm wondering how we would feel about throwing away ConsentState::Unknown? This corresponds to the 'security line' or message requests tab, and is likely to be a backlog of spam conversations, and is likely where we can throw away the most conversations. If the user later goes on to approve the conversation, normal device syncing will probably add the new device anyway (although the message history from before the approval will likely be missing on the new device).
There was a problem hiding this comment.
Yep aligned. Only add me to my allowed groups for sure.
| let groups = self.mls_store.find_groups(GroupQueryArgs::default())?; | ||
| let groups = self.mls_store.find_groups(GroupQueryArgs { | ||
| activity_after_ns: Some(now_ns() - NS_IN_DAY * 90), | ||
| consent_states: Some(vec![ConsentState::Allowed]), |
There was a problem hiding this comment.
I was just thinking about this a bit more I'm wondering if we are being too restrictive with only Allowed maybe we should also include Unknown... In the case where I have a few conversations that are unknown that I then approve will I still be able to add my own installation to that group. 🤔 @codabrink maybe we should open this up a little to expand to unknown?
Filter device sync group additions to only include groups active within 90 days and with allowed or unknown consent states in
add_new_installation_to_groupsmethodThe
add_new_installation_to_groupsmethod in device_sync.rs now filters groups based on activity within the last 90 days and consent states ofAllowedorUnknown. The method signature forcreate_group_with_inbox_idsin client.rs changes to accept any type implementingAsIdReftrait instead of specifically requiringInboxIdobjects. New test coverage verifies the filtering behavior in tests.rs.📍Where to Start
Start with the
add_new_installation_to_groupsmethod in device_sync.rs to understand the new filtering logic for group selection.Changes since #2053 opened
Macroscope summarized f51c1eb.