Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: more lenient initial sync #839

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

EvanHahn
Copy link
Contributor

@EvanHahn EvanHahn commented Sep 18, 2024

Fixes the weird invite bug we've been seeing (digidem/comapeo-mobile#671):

video.mp4

When you join a project, you wait for some data to sync, such as the project config. That effectively happens in two steps:

  1. If sync is already done, return.
  2. Otherwise, listen for sync state updates, and return when sync is done.

Before this change, these two steps answered "is sync done?" in slightly different ways. After this change, they answer that in the same way.

Here's a diff in pseudocode:

 function waitForInitialSync() {
   if (!wantsDataFromOtherPeers()) return 'done'

   onSyncState(() => {
-    if (isSyncCompletelyFinished()) {
+    if (!wantsDataFromOtherPeers()) {
       return 'done'
     }
   })
 }

More specifically, this makes both parts check "is sync done?" by looking at the auth and config namespaces. If they want nothing but have something, we consider them done with initial sync.

When you join a project, you wait for some data to sync, such as the
project config. That effectively happens in two steps:

1. If sync is already done, return.
2. Otherwise, listen for sync state updates, and return when sync is
  done.

Before this change, these two steps answered "is sync done?" in slightly
different ways. After this change, they answer that in the same way.

Here's a diff in pseudocode:

```diff
 function waitForInitialSync() {
   if (!wantsDataFromOtherPeers()) return 'done'

   onSyncState(() => {
-    if (isSyncCompletelyFinished()) {
+    if (!wantsDataFromOtherPeers()) {
       return 'done'
     }
   })
 }
```

More specifically, this makes both parts check "is sync done?" by
looking at the `auth` and `config` namespaces. If they want nothing but
have something, we consider them done with initial sync.
Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is correct. Not sure if the naming and the nested function is the clearest.

Maybe something like hasRrceivedAllData(syncSyate.auth) && hasRrceivedAllData(syncState.config)

But much of a muchness really.

Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe hold on this given #640, and we can change this in the other direction -> less lenient initial sync to actually check everything has synced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants