refactor: improve sync reliability

[tyler-dot-earth]

sync changes

Primarily:

1. Make refreshBookExport async
   • There is no reason for it not to be, though that's largely because it is now debounced. (keep reading)
2. Remove debounce from refreshBookExport
   • I suspect this only ever existed because the 'delete' listener fires for each file that is deleted, which in turn was firing the refreshBookExport function a ton of times. (it isn't called in the 'delete' listener anymore - keep reading)
3. Defer refreshBookExport to sync events
   • aka don't call this when deleting files
   • ensure refreshBookExport called when syncing - both on scheduled and on-demand
4. Generally ensure all syncing happens via refreshBookExport
   • ensures that the various sync methods won't get tangled up very easily (eg writing to settings via various sync operations in tandem)

All of these combined should ensure a much more reliably sync operation.

Extensive testing instructions found at bottom of this post ⤵

notes dump

clean plugin install happens

default data.json (aka settings):

{
  "token": "",
  "readwiseDir": "Readwise",
  "frequency": "0",
  "triggerOnLoad": true,
  "isSyncing": false,
  "lastSyncFailed": false,
  "lastSavedStatusID": 0,
  "currentSyncStatusID": 0,
  "refreshBooks": false,
  "booksToRefresh": [],
  "booksIDsMap": {},
  "reimportShowConfirmation": true
}

user clicks "connect"

• getUserAuthToken() → saves to settings.token
• web browser opens to https://readwise.io/export/obsidian/preferences
• obsidian plugin UI refreshes to show all options now that user is authenticated

note that sync frequency is manual/0 at this point

user initiates initial/first sync — this can happen one of several ways:

1. click "initiate sync"
   • refreshBookExport
2. Sync your data now command
   • refreshBookExport
3. toggle on "resync deleted files"
   • refreshBookExport
4. reload workspace
   • onload → refreshBookExport
5. interval is triggered
   • configureSchedule → refreshBookExport

after the initial sync, settings.lastSavedStatusID and settings.booksIDsMap are populated

at this point, the user may also use the Delete and reimport this document command within a specific Readwise export — that also simply uses refreshBookExport

notes about refreshBookExport

• all syncing goes through this. ==very important== because it ensures that the various sync methods won't get tangled up very easily (eg writing to settings via various sync operations in tandem)
• refreshBookExport will always trigger a requestArchive (either directly or via startSync), regardless of if it was provided specific books to refresh, so new highlights should always appear

notes about "resync deleted files" (aka settings.refreshBooks):

• if disabled:
  • the entry will remain in booksIDsMap so we have a copy of its book ID
  • book IDs will NOT be added to booksToRefresh
• if enabled:
  • book IDs will be added to booksToRefresh as they are deleted
  • the entry will remain in booksIDsMap
  • refreshBookExport will look for a difference in the filesystem vs what is in booksIDsMap — seemed like the only way to handle filesystem deletions (outside of Obsidian), unknown performance when there are many many readwise files. could be moved behind a toggle if it's particularly slow.

other notes:

• auto query string seems to indicate that the sync wasn't triggered by a user (eg interval sync)
• when settings.readwiseDir isn't there, the requestArchive will use the parentPageDeleted querystring (like /api/obsidian/init?parentPageDeleted=true) — there's no documentation on this, but i did test it and it does successfully sync the files in a clean install

install for testing

you can download and build the plugin yourself, or use BRAT to install a pre-built beta:

1. disable the current Readwise plugin in your Obsidian
2. install BRAT via Obsidian community plugins and enable the BRAT plugin
3. within BRAT's plugin settings, click the "Add Beta plugin with frozen version" button
4. fill the fields in with the beta info Repository: https://github.com/tyler-dot-earth/dev-obsidian-readwise Latest pre-built version: refactor-improve-sync-reliability-beta-1 Should look a bit like this:
5. use the plugin like normal

how to test thoroughly

:warning: perform these in listed order

• [ ] confirm clean plugin install syncs notes as expected (this isn't automatic; you must click "initiate sync" or reload the workspace after authenticating)

• enable Resync deleted files

• delete a synced file

• use command palette to Readwise Official: Sync your data now

• [ ] confirm deleted file is re-synced

• delete a synced file

• open plugin settings and click Initiate sync

• [ ] confirm deleted file is re-synced

• open a synced file

• use command palette to Readwise Official: Delete and reimport this document

• [ ] confirm file is deleted and is re-synced

• delete synced file

• use command palette to Reload app without saving (or re-open Obsidian)

• [ ] confirm deleted file is re-synced

• disable Resync deleted files

• delete a synced file

• use command palette to Readwise Official: Sync your data now

• [ ] confirm file is NOT re-synced

• delete a synced file

• open plugin settings and click Initiate sync

• [ ] confirm file is NOT re-synced

• open a synced file

• use command palette to Readwise Official: Delete and reimport this document

• [ ] confirm file is re-synced

• delete synced file

• use command palette to Reload app without saving (or re-open Obsidian)

• [ ] confirm deleted file is re-synced

• delete synced file

• enable Resync deleted files

• [ ] confirm deleted file is re-synced

• and finally, for good measure:

• [ ] ensure adding new highlights continues to work as expected

• [ ] ensure syncing on interval continues to work as expected

other changes

• a few random, small bugfixes and refactors. i'm happy to split into their own PR, but felt like more friction than it was worth for both me and reviewer(s).

[TristanH]

Remove debounce from refreshBookExport I suspect this only ever existed because the 'delete' listener fires for each file that is deleted, which in turn was firing the refreshBookExport function a ton of times. (it isn't called in the 'delete' listener anymore - keep reading)

I'd definitely like us to confirm this... @tadeoos do you recollect the original rationale?

[tyler-dot-earth]

@TristanH

Remove debounce from refreshBookExport I suspect this only ever existed because the 'delete' listener fires for each file that is deleted, which in turn was firing the refreshBookExport function a ton of times. (it isn't called in the 'delete' listener anymore - keep reading)

I'd definitely like us to confirm this... @tadeoos do you recollect the original rationale?

Please do! I could only guess based on the code and behavior.

Given the debounce was 800ms, I can't imagine it was to compensate for some backend behavior (eg a slow sync process that would get confused by "outdated" refreshBookExport request)

The relevant changes call refreshBookExport significantly less as it is simply no longer called immediately upon deletion of every single file, instead deferring to the normal sync schedule or manual re-sync.

So beyond spamming the request, which this PR significantly reduces, i suppose the question to answer is: is there any point in keeping the debounce?

That said, I can also say:

• this syncing behavior is working really well from my testing
• requestSync was never similarly debounced (though perhaps that's a problem of its own)
• rate limiting may be a more generalized solution to this problem

[tyler-dot-earth]

@TristanH alright, i tested the heck out of this today — behavior seems solid, and I think I fixed a few more related bugs along the way.

added a ton of notes to the original post, and a big list of steps for testing.

enjoy!

[tyler-dot-earth]

added an install for testing section to OP with instructions on testing via BRAT instead of having to manually clone & install.

[tyler-dot-earth]

Noticing that null is being added to books to refresh.

Not necessarily a problem - this happened before, too - but will see if there's something specific causing it. Would be easy to filter them out, but would be better if it didn't happen to begin with.