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

Ensure that all imported notes are sent to the server #986

Merged
merged 10 commits into from
Nov 6, 2018
Merged

Conversation

mirka
Copy link
Member

@mirka mirka commented Nov 6, 2018

Importing over 350-400 notes at once can cause the syncing to get stuck. When the syncing is stuck, it can be resumed by calling client.connect().

How can we tell that syncing is stuck? We first need to be able to detect whether the syncing is active or idle. When syncing is idle despite there being unsynced notes left, we can tell that it is stuck. Then, we can resuscitate the transfer by calling client.connect().

This workaround adds a way to detect syncing activity (active/idle), by attaching debounced listeners to the simperium client's send and message events. We can now hook arbitrary functions to these state changes. (This will probably come in handy when we add a syncing indicator in the future.)

Also changed

  • Added the debug module for easier debugging (cc: @adlk)
  • Increased the max file size for SimplenoteImporter to 5 MB

To test

  1. To see the pertinent logs in the console, enter this into the devtools console and reload: localStorage.debug = 'sync:*'.
  2. Try doing a 400+ note import. See that the syncing is reactivated when it goes idle with unsynced notes remaining.
  3. After the sync is over, verify that you can log out without warnings.

Here are some import files of various sizes: mock-simplenote-json.zip

@mirka mirka added this to the 1.3.0 milestone Nov 6, 2018
@mirka mirka requested a review from roundhill November 6, 2018 15:50
Copy link
Contributor

@roundhill roundhill left a comment

Choose a reason for hiding this comment

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

Tested well for me! Just had one thought about a constant but this is good to go.

lib/utils/sync/activity-hooks.js Outdated Show resolved Hide resolved
@roundhill
Copy link
Contributor

I also tried an import while offline, and then connecting up again. It synced up in the same way and I could see the idle reviver working 👍

@mirka mirka merged commit c312d5a into master Nov 6, 2018
@mirka mirka deleted the fix/stuck-sync branch November 6, 2018 18:45
@mirka mirka mentioned this pull request Nov 6, 2018
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