Repository navigation
Conversation
Amazon's Kindle notebook page lazily renders books into the DOM as the page is scrolled. A single unscrolled read only ever sees the first batch that happens to render (observed as low as ~150 books on a 900+ book library), regardless of the library's real size. The existing "warn if we see exactly 54 books" check no longer matches Amazon's current behavior and never fired, so this was silently losing the majority of a large library with no indication to the user. scrapeBooks now scrolls the page to the bottom repeatedly and counts `.kp-notebook-library-each-book` elements until the count stops growing (scrollUntilStable), instead of waiting out a fixed timeout. The scroll/count/wait loop is extracted as a small, pure, fully unit tested function (scrollUntilStable.ts) so it doesn't depend on a real Electron window in tests. Also hardens loadRemoteDom, which every book-list and highlights-page load goes through: previously a failed or hung page load (network blip, timeout, unexpected error) would leak the hidden BrowserWindow forever and hang whatever was waiting on it, since there was no did-fail-load handling, no timeout, and no .catch() on the load promise chain. This was unlikely to bite at ~150 books but becomes much more likely across hundreds of book/page loads per sync. Failures now destroy the window and reject quickly, so a single bad page load just fails that one item (already handled per-book by the sync loop) instead of risking a hang or crash partway through a large sync.
Two compounding bugs meant a book already synced correctly could end up permanently duplicated and/or permanently stuck looking "changed": 1. FileManager.getKindleFile() matches an existing note to a book by comparing a bookId stored in frontmatter, read via Obsidian's async metadataCache rather than the file itself. If that lookup ever came back empty for a book that already has a note on disk (a stale metadata cache after a big batch of writes, or the configured highlights folder briefly failing to resolve via getAbstractFileByPath - see hadynz#2), createFile's only response to the resulting filename collision was to append a Date.now() suffix and write a brand new duplicate file next to the original, forever, on every sync that hit the same lookup miss. createFile now checks whether a file already exists at the book's exact canonical path before creating anything, and updates that file in place instead of creating a duplicate alongside it. 2. getKindleFiles()'s folder scan silently produced an empty file list whenever the configured highlights folder didn't resolve to a TFolder via getAbstractFileByPath (case/slash mismatch, or just not indexed yet) - there was already a fallback for when that call *threw*, but not for when it just came back empty, so every book in the folder would look "missing" and get duplicated via bug hadynz#1. getKindleFiles() now falls back to a full vault scan (filtered by path prefix) in that case too. Also removed an arbitrary 1000-file processing cap that would silently drop files from a library this size instead of surfacing an error. Separately, the plugin supports both a legacy nested `kindle-sync: {}` frontmatter block and a newer flat `kindle-*` properties format, but mergeFrontmatter only ever merged new flat properties *into* whatever was already there - it never removed a pre-existing nested block. And mapToKindleFile checked the nested block *first*, only falling back to flat properties if nested was absent. So for any note that predates the flat-format switch, every sync kept adding correct, fresh flat properties right alongside the untouched, increasingly stale nested block - and every read kept preferring that same stale nested data, no matter how many times the flat properties were correctly updated underneath it. mergeFrontmatter now accepts a list of keys to strip, and file writes remove the legacy block; reads now prefer the flat format and only fall back to nested for notes that have never been touched by a sync at all. Also threads a new Book.lastChecked field (day-granularity, mirroring lastAnnotatedDate) through the frontmatter read/write path, in preparation for using it instead of Amazon's own date to decide whether a book needs a closer look (see the following commit).
diffBooks decided whether an already-synced book needed a closer look by comparing Amazon's per-book date against whatever date was stored from the previous sync. That date (scraped from a hidden kp-notebook-annotated-date input on the notebook page) is actually labelled "Last accessed on ..." in Amazon's own UI - it updates whenever the book's notebook page is *viewed*, not only when a highlight is added. Visiting a book's highlights page to scrape it - exactly what syncing a book does - is itself a view, so checking a book at all was guaranteed to make it look "changed" again on the very next sync. Once any book entered this loop (via a genuine update, or via either bug fixed in the previous commit), it could never leave it: every sync re-flagged it, re-scraped it, and re-poisoned its own comparison date, forever, regardless of whether anything was actually read or highlighted in the meantime. Comparing Amazon's date against a plugin-owned "when did *we* last check this book" timestamp instead breaks the loop: after a check, both Amazon's date and our own bookkeeping land on the same day, so they keep matching (and the book is left alone) until something else moves Amazon's date forward again. A 90-day fallback still forces an occasional full recheck regardless, as a safety net in case Amazon's date ever doesn't track a real change the way this assumes (e.g. a highlight swapped for another on the same day). - Book.lastChecked is now stamped with the current date on every file write (FileManager.generateBookContent), not carried over from the book data passed in - it always means "the plugin wrote this file just now". - diffBooks() drops the lastSyncDate parameter (no longer meaningful here) and its needsRecheck() replaces the old isSameDate/ updatedSince comparison against the stored lastAnnotatedDate. - SyncManager.resyncBook() now always writes, even when the highlight diff is empty, since that's what stamps lastChecked - without it, a book confirmed unchanged would never advance its own baseline and would look permanently stale to the next sync.
FileManager writes (createFile/updateFile) only ever waited a fixed 50ms after each vault.create/modify before moving on, on the assumption that's enough time for Obsidian's metadataCache to reprocess the file. Every read of a book's frontmatter (getKindleFiles/mapToKindleFile) goes through that same cache, so if the real reindex takes longer - plausible for a large batch of writes, or a cloud-synced vault (e.g. iCloud) adding extra I/O latency - the very next sync can read stale, pre-write frontmatter for a file that's already correct on disk. Observed concretely: a book's lastChecked (see the "last accessed" commit) was correctly stamped with today's date on disk and confirmed via Obsidian's own Properties panel a short while later, but an immediate back-to-back sync still treated the book as never checked - consistent with diffBooks reading a cache entry that hadn't caught up yet at the moment it ran, moments after the previous sync's writes. Replaces the fixed delay with an actual wait for metadataCache's 'changed' event on the specific file just written, with a 5s timeout fallback in case Obsidian doesn't fire it for some edge case (e.g. no byte-level change).
getKindleFile matched a book to its note with a plain Array.find over whatever order the vault happened to enumerate files in. That's fine when there's exactly one note per book, but if a duplicate ever exists - a leftover from the duplicate-creation bug fixed earlier, or any future edge case that produces one - different syncs could find and update different copies of "the same" book. Confirmed on a real vault with 98 duplicated books: one sync's write and the next sync's read would land on different files, so a book could look permanently "never checked" no matter how many times it was actually corrected on disk, since the copy being read was never the one being written. getKindleFile now picks a single canonical file whenever more than one note claims the same bookId, preferring (in order) the most complete copy (highest highlightsCount), then the canonical non-suffixed filename, then the oldest file. The comparison itself (pickBetterDuplicate) is a small pure function, unit tested in isolation. This doesn't create or clean up duplicates - it only makes lookups stable if one exists. Existing duplicates still need a one-time cleanup; the previous commit's metadata-cache fix and this one together stop new ones from causing lasting damage.
syncBook skipped creating a file entirely for a book with no highlights, on the reasonable-looking assumption there's nothing worth writing yet. But that also means such a book never gets a lastChecked baseline recorded anywhere - so diffBooks has nothing to compare against, and the book falls into newBooks and gets rescraped on every single sync, forever, even though it never has anything to report. Same root shape as the other bugs fixed in this branch: no record of having checked a book means it can never stop looking new. createBook already renders an empty highlights section without issue (FileRenderer just joins an empty array into an empty string), so there's nothing unsafe about creating the note - it'll just show 0 highlights until the book actually has some, at which point diffBooks will treat it exactly like any other update.
…overy path createFile's fallback for when a file already exists at a book's canonical path (see the duplicate-notes commit) rewrote that file using only the freshly rendered body, with a brand new frontmatter block built from scratch. Any frontmatter a user had added to that note themselves - a personal rating, genre, tags, anything not owned by the plugin - would be silently discarded, since nothing carried it forward. The normal update path (FileManager.updateFile, via DiffManager) doesn't have this problem, because the content it writes already contains the existing file's own frontmatter, and mergeFrontmatter only overlays the plugin's own kindle-* fields on top of it. This fallback path never had that same starting point, since it always worked from a fresh render. generateBookContent now takes an optional preserveFrontmatterFrom argument; when a pre-existing file is found in createFile's fallback, its current frontmatter is read and carried over before the plugin's own fields are merged in, matching what the normal update path already does.
Observed on a real vault stored in iCloud Drive: resyncing an existing book failed with errors like "ETIMEDOUT: connection timed out, read" and "Unknown system error -81" from Node's readFileHandle, surfaced as "Error syncing book" for several books in one run. Both errors trace back to FileManager.readFile's vault.cachedRead call (used when loading a book's existing note before diffing its highlights). This isn't a logic bug - it's what happens when a cloud-sync provider (iCloud Drive, Dropbox, OneDrive, ...) has evicted a file locally to save space and has to fetch it on demand before it can be read, which can transiently time out or fail under the kind of heavy, rapid file I/O a large-library sync produces. The existing per-book try/catch already stops one failure from crashing the whole sync, but without a retry, a single transient hiccup permanently fails that book's check for the run. Adds a small, generic withRetry helper (linear backoff, unit tested in isolation) and uses it for the two vault reads that can hit this: the normal existing-note read in readFile, and the existing-file read in createFile's path-collision recovery path from an earlier commit.
…the console
The sync modal already tracked failed books in erroredJobs (populated
via the syncBookFailure/resyncFailure events) and even had a view
branch built to display "N books(s) could not be synced because of
errors" - but syncSessionSuccess's handler reset erroredJobs back to
empty every time, via `{ ...InitialState, ... }` without carrying it
over. Since a session finishing is exactly when a user would want to
know some books failed along the way, this meant that view branch was
dead code for the normal completion path: a sync could silently fail
to check or update several books and the completion screen would just
say "N books synced," with the actual failures only ever visible by
having Developer Tools open at the time.
erroredJobs is now preserved through to the 'sync:complete' state, and
that screen shows a dedicated message when it's non-empty, naming the
count and pointing at Developer Tools' Console tab (and the existing
"Details" log below) for specifics - rather than requiring a user to
have had the console open and scrolled back to catch it.
scrollUntilStable reports progress on every scroll attempt, including the repeated identical reads it needs before it's confident the library's full item count has actually stopped growing (stableIterations defaults to 3). Logging every one of those made the sync log print the same "Found N item(s) so far…" line several times in a row with nothing visibly happening, which reads as the sync being stuck even though it's working as designed. loadRemoteDom now only emits a log line when the count actually changes from the last one logged, so the log shows real progress (874 → 924) without the trailing noise of the stabilization checks.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On a large library (tested with 900+ books), sync had several compounding problems:
Root causes and fixes (6 commits)
Full-library scraping - Amazon's notebook page lazily renders books as the page is scrolled; an unscrolled read only ever saw the first rendered batch. Now scrolls until the count of rendered books stabilizes. Also hardens the underlying page-load helper (
loadRemoteDom) with proper failure/timeout handling, since a hung or failed page load previously leaked a hidden window and could hang the whole sync - unlikely at ~150 books, much more likely across hundreds of book/page loads per sync.Duplicate notes / stale legacy frontmatter -
createFileresponded to a filename collision by appending a timestamp and writing a new file, instead of recognizing the book already had a note (e.g. because Obsidian's metadata cache hadn't caught up yet, or the highlights folder briefly failed to resolve). Also, notes from before the plugin's flat-frontmatter-properties format was introduced kept an untouched legacykindle-sync: {}block that a read path incorrectly preferred over newer, correct data written right next to it.Amazon's "last accessed" date isn't an annotation signal - the field the plugin compared against to decide "did this change" is literally labelled "Last accessed on ..." in Amazon's own UI. It updates on any view, including the view sync itself performs to scrape highlights - so checking a book at all guaranteed it would look "changed" again next sync, forever. Replaced with a plugin-owned
lastCheckedbookkeeping field compared against Amazon's date, plus a 90-day periodic safety-net recheck.Metadata cache race - file writes only waited a fixed 50ms for Obsidian's metadata cache to catch up before considering a write "done." Under real load (large batches, cloud-synced vaults) this wasn't always enough, so the very next sync could read stale pre-write frontmatter. Now waits for the cache's own
changedevent instead of guessing.Non-deterministic duplicate resolution - if a duplicate note ever existed for a book (from before these fixes, or any future edge case), lookup picked whichever file the vault happened to enumerate first, so different syncs could read/write different copies of "the same" book. Now picks deterministically (most complete copy, then canonical filename, then oldest).
Zero-highlight books never got a note - meaning no bookkeeping was ever recorded for them, so they looked "new" and got rescraped every single sync forever, same root cause as Contents of title and highlights escaped #3.
Testing
npm testpasses (156 tests).npm run buildsucceeds cleanly.Note on scope
This PR doesn't include the Amazon-login-window fix from #338 - that's already an independent contribution from another author and shouldn't be duplicated here. The two are unrelated and can land independently.
🤖 Generated with Claude Code