From 230ecb06535a37b3ba5a940dd354fc800902177e Mon Sep 17 00:00:00 2001 From: Tareef Kawaf Date: Sun, 13 Sep 2026 22:01:49 -0500 Subject: [PATCH 01/10] fix: scrape the entire Kindle library, not just the first rendered batch 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. --- src/scraper/loadRemoteDom.ts | 109 +++++++++++++++++++++++--- src/scraper/scrapeBooks.ts | 27 ++++--- src/scraper/scrollUntilStable.spec.ts | 64 +++++++++++++++ src/scraper/scrollUntilStable.ts | 45 +++++++++++ 4 files changed, 223 insertions(+), 22 deletions(-) create mode 100644 src/scraper/scrollUntilStable.spec.ts create mode 100644 src/scraper/scrollUntilStable.ts diff --git a/src/scraper/loadRemoteDom.ts b/src/scraper/loadRemoteDom.ts index 9ed3e7a..976c727 100644 --- a/src/scraper/loadRemoteDom.ts +++ b/src/scraper/loadRemoteDom.ts @@ -3,6 +3,9 @@ import { BrowserWindow, remote } from 'electron'; import { ee } from '~/eventEmitter'; +import type { ScrollUntilStableOptions } from './scrollUntilStable'; +import { scrollUntilStable } from './scrollUntilStable'; + const { BrowserWindow: RemoteBrowserWindow } = remote; type DomResult = { @@ -13,8 +16,20 @@ type DomResult = { type LoadRemoteDomOptions = { log?: boolean; label?: string; + /** + * Some pages (like Amazon's Kindle notebook library list) only render items into the DOM as + * the page is scrolled. When set, the page is repeatedly scrolled to the bottom and `itemSelector` + * is counted until the count stops growing, instead of just waiting out a fixed `timeout`. + */ + scrollToLoadAll?: { itemSelector: string } & ScrollUntilStableOptions; + /** Give up and reject if the page hasn't resolved within this many ms (default 2 minutes) */ + loadTimeoutMs?: number; }; +// Chromium's ERR_ABORTED. Fires for perfectly normal cases (a redirect that interrupts the +// original navigation, for instance) so it's not treated as a real load failure. +const ERR_ABORTED = -3; + export const loadRemoteDom = async ( targetUrl: string, timeout = 0, @@ -34,11 +49,46 @@ export const loadRemoteDom = async ( show: false, }); - // eslint-disable-next-line @typescript-eslint/no-floating-promises - window.loadURL(targetUrl); + const destroyWindow = (): void => { + if (!window.isDestroyed()) { + window.destroy(); + } + }; - return new Promise((resolveWrapper) => { + return new Promise((resolveWrapper, rejectWrapper) => { let didNavigateUrl: string = null; + let settled = false; + + const settle = (fn: () => void): void => { + if (settled) { + return; + } + settled = true; + clearTimeout(safetyTimeout); + fn(); + }; + + // Without this, a network hang, a page that never fires did-finish-load, or a page load + // error would leak this hidden window forever and hang whatever book/page was waiting on it, + // stalling the whole sync instead of just failing that one item. + const safetyTimeout = setTimeout(() => { + settle(() => { + destroyWindow(); + rejectWrapper(new Error(`${labelPrefix}Timed out loading ${targetUrl}`)); + }); + }, options.loadTimeoutMs ?? 120000); + + window.webContents.on('did-fail-load', (_event, errorCode: number, errorDescription: string) => { + if (errorCode === ERR_ABORTED) { + return; + } + settle(() => { + destroyWindow(); + rejectWrapper( + new Error(`${labelPrefix}Failed to load ${targetUrl}: ${errorDescription} (${errorCode})`) + ); + }); + }); window.webContents.on('did-navigate', (_event, url) => { didNavigateUrl = url; @@ -57,6 +107,34 @@ export const loadRemoteDom = async ( // eslint-disable-next-line @typescript-eslint/no-floating-promises Promise.resolve() .then(() => { + if (options.scrollToLoadAll) { + const { itemSelector, ...scrollOptions } = options.scrollToLoadAll; + + return scrollUntilStable( + async () => { + await window.webContents.executeJavaScript( + 'window.scrollTo(0, document.body.scrollHeight)' + ); + }, + async () => { + const count: unknown = await window.webContents.executeJavaScript( + `document.querySelectorAll(${JSON.stringify(itemSelector)}).length` + ); + return Number(count); + }, + (ms) => new Promise((resolve) => setTimeout(resolve, ms)), + { + ...scrollOptions, + onProgress: (count) => { + if (shouldLog) { + ee.emit('syncLog', `${labelPrefix}Found ${count} item(s) so far…`); + } + scrollOptions.onProgress?.(count); + }, + } + ); + } + if (timeout > 0) { if (shouldLog) { ee.emit( @@ -83,17 +161,28 @@ export const loadRemoteDom = async ( } const $ = cheerio.load(html); - window.destroy(); + settle(() => { + destroyWindow(); - if (shouldLog) { - ee.emit('syncLog', `${labelPrefix}Page ready`); - } + if (shouldLog) { + ee.emit('syncLog', `${labelPrefix}Page ready`); + } - resolveWrapper({ - dom: $, - didNavigateUrl: didNavigateUrl, + resolveWrapper({ + dom: $, + didNavigateUrl: didNavigateUrl, + }); + }); + }) + .catch((error) => { + settle(() => { + destroyWindow(); + rejectWrapper(error instanceof Error ? error : new Error(String(error))); }); }); }); + + // eslint-disable-next-line @typescript-eslint/no-floating-promises + window.loadURL(targetUrl); }); }; diff --git a/src/scraper/scrapeBooks.ts b/src/scraper/scrapeBooks.ts index 84f60ee..b5cae10 100644 --- a/src/scraper/scrapeBooks.ts +++ b/src/scraper/scrapeBooks.ts @@ -65,23 +65,26 @@ export const parseBooks = ($: Root): Book[] => { }); }; +// Amazon's Kindle notebook page lazily renders books into the DOM as the page is scrolled, +// so a single unscrolled read only ever sees the first batch (regardless of library size). +const NotebookBookSelector = '.kp-notebook-library-each-book'; + const scrapeBooks = async (): Promise => { const region = currentAmazonRegion(); ee.emit('syncLog', `Loading Kindle notebook from ${region.hostname}…`); - const { dom } = await loadRemoteDom(region.notebookUrl, 30000); + const { dom } = await loadRemoteDom(region.notebookUrl, 0, { + scrollToLoadAll: { + itemSelector: NotebookBookSelector, + // Large libraries (hundreds+ of books) need more, and slower-loading, scrolls than the + // defaults tuned for a typical page + scrollDelayMs: 1500, + maxIterations: 150, + stableIterations: 3, + }, + }); const books = parseBooks(dom); - // Amazon's Kindle notebook page typically shows up to 54 books per page - // If we get exactly 54, there may be more books on additional pages - // However, pagination requires complex interaction with Amazon's interface - // Users with more than 54 books may need to sync multiple times - // The sync process is intelligent and will only sync new/changed books - if (books.length === 54) { - console.log( - 'Found 54 books. If you have more books, you may need to sync multiple times to get them all.' - ); - ee.emit('syncLog', 'Note: Kindle notebook may show only 54 books per page'); - } + ee.emit('syncLog', `Found ${books.length} book(s) in Kindle notebook`); return books; }; diff --git a/src/scraper/scrollUntilStable.spec.ts b/src/scraper/scrollUntilStable.spec.ts new file mode 100644 index 0000000..af185bb --- /dev/null +++ b/src/scraper/scrollUntilStable.spec.ts @@ -0,0 +1,64 @@ +import { scrollUntilStable } from './scrollUntilStable'; + +const noopWait = (): Promise => Promise.resolve(); + +describe('scrollUntilStable', () => { + it('stops as soon as the count is stable for the configured number of rounds', async () => { + const counts = [10, 25, 40, 40, 40, 40, 999]; // extra values should never be reached + const countItems = jest.fn, []>(() => Promise.resolve(counts.shift())); + const scrollToBottom = jest.fn, []>(() => Promise.resolve()); + + const result = await scrollUntilStable(scrollToBottom, countItems, noopWait, { + stableIterations: 3, + }); + + expect(result).toBe(40); + // Initial read (10) + scrolls (25, then three consecutive 40s to satisfy stableIterations) + expect(countItems).toHaveBeenCalledTimes(6); + }); + + it('gives up after maxIterations even if the count is still growing', async () => { + let n = 0; + const countItems = jest.fn, []>(() => { + n += 10; + return Promise.resolve(n); + }); + const scrollToBottom = jest.fn, []>(() => Promise.resolve()); + + const result = await scrollUntilStable(scrollToBottom, countItems, noopWait, { + maxIterations: 5, + stableIterations: 100, + }); + + // Initial read + 5 scroll iterations, count never stabilizes + expect(countItems).toHaveBeenCalledTimes(6); + expect(result).toBe(60); + }); + + it('reports progress after every scroll attempt', async () => { + const counts = [5, 12, 12, 12]; + const countItems = jest.fn, []>(() => Promise.resolve(counts.shift())); + const scrollToBottom = jest.fn, []>(() => Promise.resolve()); + const onProgress = jest.fn(); + + await scrollUntilStable(scrollToBottom, countItems, noopWait, { + stableIterations: 2, + onProgress, + }); + + // Initial read, then one per scroll: 12 (changed), 12, 12 (two consecutive stable reads) + expect(onProgress.mock.calls.map((call) => call[0])).toEqual([5, 12, 12, 12]); + }); + + it('never scrolls if the list is already stable on first read', async () => { + const countItems = jest.fn, []>(() => Promise.resolve(7)); + const scrollToBottom = jest.fn, []>(() => Promise.resolve()); + + const result = await scrollUntilStable(scrollToBottom, countItems, noopWait, { + stableIterations: 1, + }); + + expect(result).toBe(7); + expect(scrollToBottom).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/scraper/scrollUntilStable.ts b/src/scraper/scrollUntilStable.ts new file mode 100644 index 0000000..6a16dd2 --- /dev/null +++ b/src/scraper/scrollUntilStable.ts @@ -0,0 +1,45 @@ +export type ScrollUntilStableOptions = { + /** Give up after this many scroll attempts, even if the count is still growing */ + maxIterations?: number; + /** How long to wait after each scroll for lazily-loaded content to render */ + scrollDelayMs?: number; + /** How many consecutive unchanged counts before we consider the list fully loaded */ + stableIterations?: number; + /** Called after each scroll attempt with the item count seen so far */ + onProgress?: (count: number) => void; +}; + +/** + * Repeatedly scrolls a lazily-loaded list and counts its items until the count stops growing + * (or `maxIterations` is reached), then returns the final count. Amazon's Kindle notebook page + * only renders books into the DOM as the page is scrolled, so a single unscrolled read only + * ever sees the first batch. + * + * Scroll/count/wait are injected so this can be unit tested without a real browser. + */ +export const scrollUntilStable = async ( + scrollToBottom: () => Promise, + countItems: () => Promise, + wait: (ms: number) => Promise, + options: ScrollUntilStableOptions = {} +): Promise => { + const { maxIterations = 60, scrollDelayMs = 1000, stableIterations = 3, onProgress } = options; + + let lastCount = await countItems(); + onProgress?.(lastCount); + + let stableRounds = 0; + + for (let i = 0; i < maxIterations && stableRounds < stableIterations; i++) { + await scrollToBottom(); + await wait(scrollDelayMs); + + const count = await countItems(); + onProgress?.(count); + + stableRounds = count === lastCount ? stableRounds + 1 : 0; + lastCount = count; + } + + return lastCount; +}; From 71fce77a1c3c09ca8d059a1d35ab2d5b20e48475 Mon Sep 17 00:00:00 2001 From: Tareef Kawaf Date: Sun, 13 Sep 2026 22:05:11 -0500 Subject: [PATCH 02/10] fix: stop creating duplicate notes and reading stale legacy frontmatter 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 #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 #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). --- src/fileManager/index.ts | 125 ++++++++++++++++++---------------- src/fileManager/mappers.ts | 4 ++ src/models.ts | 8 +++ src/utils/frontmatter.spec.ts | 21 ++++++ src/utils/frontmatter.ts | 9 ++- 5 files changed, 109 insertions(+), 58 deletions(-) diff --git a/src/fileManager/index.ts b/src/fileManager/index.ts index df054c9..a79ae48 100644 --- a/src/fileManager/index.ts +++ b/src/fileManager/index.ts @@ -42,24 +42,27 @@ export default class FileManager { const fileCache = this.metadataCache.getFileCache(file); // File cache can be undefined if this file was just created and not yet cached by Obsidian - // Try both nested format (legacy) and flat format (new) + // Every sync writes the flat format and is supposed to strip the legacy nested block, but + // older files may still carry a stale nested block alongside now-current flat properties + // (e.g. if they were written by a version of the plugin that merged forward without + // removing it). Prefer the flat format - the one the plugin actively keeps up to date - + // and only fall back to the nested one for files that predate the flat format entirely. let kindleFrontmatter: KindleFrontmatter | undefined; - + if (fileCache?.frontmatter) { - // Try nested format first (legacy support) - kindleFrontmatter = fileCache.frontmatter[SyncingStateKey] as KindleFrontmatter | undefined; - - // If not found, try flat format (new Obsidian properties format) - if (!kindleFrontmatter && fileCache.frontmatter[`${PropertyPrefix}bookId`]) { + if (fileCache.frontmatter[`${PropertyPrefix}bookId`]) { kindleFrontmatter = { bookId: fileCache.frontmatter[`${PropertyPrefix}bookId`] as string, title: fileCache.frontmatter[`${PropertyPrefix}title`] as string, author: fileCache.frontmatter[`${PropertyPrefix}author`] as string, asin: fileCache.frontmatter[`${PropertyPrefix}asin`] as string | undefined, lastAnnotatedDate: fileCache.frontmatter[`${PropertyPrefix}lastAnnotatedDate`] as string | undefined, + lastChecked: fileCache.frontmatter[`${PropertyPrefix}lastChecked`] as string | undefined, bookImageUrl: fileCache.frontmatter[`${PropertyPrefix}bookImageUrl`] as string | undefined, highlightsCount: fileCache.frontmatter[`${PropertyPrefix}highlightsCount`] as number | undefined, }; + } else { + kindleFrontmatter = fileCache.frontmatter[SyncingStateKey] as KindleFrontmatter | undefined; } } @@ -96,9 +99,28 @@ export default class FileManager { // Get files directly from the highlights folder if possible const folderPath = normalizePath(highlightsFolder === '/' ? '' : highlightsFolder); - + + // Scans every markdown file in the vault and filters by path prefix. Used for the root + // folder case, and as a fallback whenever direct folder resolution below doesn't pan out + // (wrong case, a stray slash, or the folder not being resolvable yet) so that a lookup + // hiccup can't make every book in the folder look "missing" and get duplicated. + const scanAllFilesFallback = (): TFile[] => { + const allMarkdownFiles = this.vault.getMarkdownFiles(); + return allMarkdownFiles.filter((file) => { + try { + const filePath = normalizePath(file.path); + if (folderPath === '') { + return !filePath.includes('/'); + } + return filePath === folderPath || filePath.startsWith(folderPath + '/'); + } catch { + return false; + } + }); + }; + let filesInFolder: TFile[] = []; - + try { // Try to get the folder directly and its files if (folderPath !== '') { @@ -117,50 +139,31 @@ export default class FileManager { return files; }; filesInFolder = getAllFilesInFolder(folder); + } else { + // Configured folder didn't resolve to a folder (case/slash mismatch, or it hasn't + // been indexed yet) - fall back rather than silently treating the vault as empty + filesInFolder = scanAllFilesFallback(); } } else { - // For root folder, we need to scan all files but filter for root only - // This is less efficient but necessary for root folder - const allMarkdownFiles = this.vault.getMarkdownFiles(); - filesInFolder = allMarkdownFiles.filter((file) => { - const filePath = normalizePath(file.path); - // File is in root if it has no '/' separator (just filename) - return !filePath.includes('/'); - }); + filesInFolder = scanAllFilesFallback(); } } catch (error) { // Fallback: if direct folder access fails, use filtered approach console.warn('Error accessing folder directly, using fallback method:', error); try { - const allMarkdownFiles = this.vault.getMarkdownFiles(); - filesInFolder = allMarkdownFiles.filter((file) => { - try { - const filePath = normalizePath(file.path); - if (folderPath === '') { - return !filePath.includes('/'); - } - return filePath.startsWith(folderPath + '/') || filePath === folderPath; - } catch { - return false; - } - }); + filesInFolder = scanAllFilesFallback(); } catch (fallbackError) { console.warn('Fallback method also failed:', fallbackError); return []; } } - + // If no files, return early if (filesInFolder.length === 0) { return []; } - - // Limit the number of files we process to prevent blocking - // Process files in smaller batches - const maxFilesToProcess = 1000; - const filesToProcess = filesInFolder.slice(0, maxFilesToProcess); - - return filesToProcess + + return filesInFolder .map((file) => { try { return this.mapToKindleFile(file); @@ -182,9 +185,26 @@ export default class FileManager { content: string, highlightsCount: number ): Promise { - const filePath = this.generateUniqueFilePath(book, metadata); + const filePath = normalizePath(bookFilePath(book, metadata)); const frontmatterContent = this.generateBookContent(book, content, highlightsCount); + // A file can already exist at this book's canonical path even though it wasn't found by its + // stored bookId (e.g. Obsidian's metadata cache hasn't caught up with a recent write yet, or + // the configured highlights folder briefly failed to resolve). Update that file in place + // instead of creating a timestamp-suffixed duplicate alongside it. + const existingFile = this.vault.getAbstractFileByPath(filePath); + + if (existingFile instanceof TFile) { + try { + await this.vault.modify(existingFile, frontmatterContent); + await new Promise((resolve) => setTimeout(resolve, 50)); + } catch (error) { + console.error(`Error updating existing file (path="${filePath})"`); + throw error; + } + return; + } + try { await this.vault.create(filePath, frontmatterContent); // Give Obsidian time to process the new file and update metadata cache @@ -221,16 +241,20 @@ export default class FileManager { * Uses flat properties format compatible with Obsidian's properties system */ private generateBookContent(book: Book, content: string, highlightsCount: number): string { - const frontmatter = bookToFrontMatter(book, highlightsCount); - + // lastChecked always means "the plugin wrote this file just now" - it's our own bookkeeping, + // not something scraped from Amazon, so it's stamped fresh on every write rather than carried + // over from whatever was on the book object passed in. + const frontmatter = bookToFrontMatter({ ...book, lastChecked: new Date() }, highlightsCount); + // Use flat properties format for better Obsidian compatibility const flatProperties: Record = { [`${PropertyPrefix}bookId`]: frontmatter.bookId, [`${PropertyPrefix}title`]: frontmatter.title, [`${PropertyPrefix}author`]: frontmatter.author, [`${PropertyPrefix}highlightsCount`]: frontmatter.highlightsCount, + [`${PropertyPrefix}lastChecked`]: frontmatter.lastChecked, }; - + // Only add optional fields if they exist if (frontmatter.asin) { flatProperties[`${PropertyPrefix}asin`] = frontmatter.asin; @@ -242,22 +266,9 @@ export default class FileManager { flatProperties[`${PropertyPrefix}bookImageUrl`] = frontmatter.bookImageUrl; } - return mergeFrontmatter(content, flatProperties); - } - - private generateUniqueFilePath(book: Book, metadata: BookMetadata): string { - const filePath = bookFilePath(book, metadata); - - const isDuplicate = this.vault - .getMarkdownFiles() - .some((v) => v.path === normalizePath(filePath)); - - if (isDuplicate) { - const currentTime = new Date().getTime().toString(); - return filePath.replace('.md', `-${currentTime}.md`); - } - - return filePath; + // Strip any stale legacy nested block so it can't keep shadowing these freshly-written flat + // properties on a future read (see the priority comment in mapToKindleFile) + return mergeFrontmatter(content, flatProperties, [SyncingStateKey]); } /** diff --git a/src/fileManager/mappers.ts b/src/fileManager/mappers.ts index 7ce36c6..99dfcbd 100644 --- a/src/fileManager/mappers.ts +++ b/src/fileManager/mappers.ts @@ -24,6 +24,7 @@ export const bookToFrontMatter = (book: Book, highlightsCount: number): KindleFr lastAnnotatedDate: book.lastAnnotatedDate ? moment(book.lastAnnotatedDate).format('YYYY-MM-DD') : null, + lastChecked: book.lastChecked ? moment(book.lastChecked).format('YYYY-MM-DD') : null, bookImageUrl: book.imageUrl, highlightsCount, }; @@ -39,6 +40,9 @@ export const frontMatterToBook = (frontmatter: KindleFrontmatter): Book => { lastAnnotatedDate: frontmatter.lastAnnotatedDate ? moment(frontmatter.lastAnnotatedDate, formats).toDate() : null, + lastChecked: frontmatter.lastChecked + ? moment(frontmatter.lastChecked, formats).toDate() + : null, imageUrl: frontmatter.bookImageUrl, }; }; diff --git a/src/models.ts b/src/models.ts index 51fd33f..a298e4c 100644 --- a/src/models.ts +++ b/src/models.ts @@ -8,6 +8,13 @@ export type Book = { url?: string; imageUrl?: string; lastAnnotatedDate?: Date; + /** + * When the plugin itself last checked this book's highlights. Plugin-owned bookkeeping (not + * something Amazon provides) - used to decide whether Amazon's own "last accessed" date + * reflects new activity since we last looked, without being fooled by the fact that checking + * a book is itself an access. + */ + lastChecked?: Date; }; export type Highlight = { @@ -61,6 +68,7 @@ export type KindleFrontmatter = { author: string; asin: string; lastAnnotatedDate?: string; // Not set for My Clipping annotations + lastChecked?: string; bookImageUrl: string; highlightsCount: number; }; diff --git a/src/utils/frontmatter.spec.ts b/src/utils/frontmatter.spec.ts index 1864100..9bb6cbc 100644 --- a/src/utils/frontmatter.spec.ts +++ b/src/utils/frontmatter.spec.ts @@ -104,4 +104,25 @@ bookName: New Book }); }).not.toThrow(); }); + + it('Removes stale legacy keys instead of leaving them to shadow the new values', () => { + const originalYamlContent = `--- +kindle-sync: + bookId: OldId123 + lastAnnotatedDate: Jan 01, 2020 +kindle-bookId: OldId123 +--- + +# Content +`; + + const actual = mergeFrontmatter( + originalYamlContent, + { 'kindle-bookId': 'OldId123', 'kindle-lastAnnotatedDate': '2024-05-01' }, + ['kindle-sync'] + ); + + expect(actual).not.toContain('kindle-sync'); + expect(actual).toContain("kindle-lastAnnotatedDate: '2024-05-01'"); + }); }); diff --git a/src/utils/frontmatter.ts b/src/utils/frontmatter.ts index f2ad310..d6239ab 100644 --- a/src/utils/frontmatter.ts +++ b/src/utils/frontmatter.ts @@ -16,12 +16,19 @@ const sanitize = (obj: any): any => { export const mergeFrontmatter = ( textWithFrontMatter: string, // eslint-disable-next-line @typescript-eslint/no-explicit-any - newFrontMatter: Record + newFrontMatter: Record, + keysToRemove: string[] = [] ): string => { // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment const cleanFrontmatter = sanitize(newFrontMatter); const { data, content } = matter(textWithFrontMatter); // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment const mergedFrontMatter = Object.assign({}, data, cleanFrontmatter); + + for (const key of keysToRemove) { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete, @typescript-eslint/no-unsafe-member-access + delete mergedFrontMatter[key]; + } + return matter.stringify(content, mergedFrontMatter); }; From c09b9754c3434059abf56d216cf22f55749a4a7e Mon Sep 17 00:00:00 2001 From: Tareef Kawaf Date: Sun, 13 Sep 2026 22:08:36 -0500 Subject: [PATCH 03/10] fix: stop treating Amazon's "last accessed" date as an annotation signal 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. --- src/sync/syncManager/diffBooks.spec.ts | 75 +++++++++++++++++--------- src/sync/syncManager/diffBooks.ts | 48 ++++++++--------- src/sync/syncManager/index.ts | 17 +++--- 3 files changed, 80 insertions(+), 60 deletions(-) diff --git a/src/sync/syncManager/diffBooks.spec.ts b/src/sync/syncManager/diffBooks.spec.ts index 1c514e7..71a5d98 100644 --- a/src/sync/syncManager/diffBooks.spec.ts +++ b/src/sync/syncManager/diffBooks.spec.ts @@ -1,67 +1,90 @@ import faker from 'faker'; +import moment from 'moment'; import type { Book } from '~/models'; import { diffBooks } from './diffBooks'; -const book = (id: string, lastAnnotatedDate?: Date): Book => { +const book = (id: string, lastAnnotatedDate?: Date, lastChecked?: Date): Book => { return { id, title: faker.lorem.words(3), author: faker.name.findName(), lastAnnotatedDate, + lastChecked, }; }; describe('diffBooks', () => { it('New remote books are always filtered for sync', () => { const remoteBooks = [book('1'), book('2'), book('3')]; - const vaultBooks = [book('1')]; + const vaultBooks = [book('1', undefined, new Date())]; - const actualBooks = diffBooks(remoteBooks, vaultBooks, new Date()); + const actualBooks = diffBooks(remoteBooks, vaultBooks); expect(actualBooks.map((a) => a.id)).toEqual(['2', '3']); }); - it('No books to sync if remote and vault are identical', () => { - const remoteBooks = [book('1'), book('2'), book('3')]; - const vaultBooks = [book('1'), book('2'), book('3')]; + it('No books to sync if remote and vault are identical and were checked today', () => { + const today = new Date(); + const remoteBooks = [book('1', today), book('2', today), book('3', today)]; + const vaultBooks = [book('1', today, today), book('2', today, today), book('3', today, today)]; - const actualBooks = diffBooks(remoteBooks, vaultBooks, new Date()); + const actualBooks = diffBooks(remoteBooks, vaultBooks); expect(actualBooks).toHaveLength(0); }); - it('Books with same last annotated date on same day as last sync will always be filtered for sync', () => { - const remoteBooks = [book('1', new Date('October 25, 2021'))]; - const vaultBooks = [book('1', new Date('October 25, 2021'))]; - const lastSyncDate = new Date('Tue Oct 25 2021 18:58:18 GMT+1300'); + it('A vault book that has never been checked (lastChecked unset) is always synced', () => { + const remoteBooks = [book('1', new Date('October 19, 2018'))]; + const vaultBooks = [book('1', new Date('October 19, 2018'), undefined)]; - const actualBooks = diffBooks(remoteBooks, vaultBooks, lastSyncDate); + const actualBooks = diffBooks(remoteBooks, vaultBooks); expect(actualBooks.map((a) => a.id)).toEqual(['1']); }); - it('Books with same last annotated date a day before last sync will always be filtered for sync', () => { + it("Amazon's date moving past our last check means new activity happened and we resync", () => { const remoteBooks = [book('1', new Date('October 25, 2021'))]; - const vaultBooks = [book('1', new Date('October 25, 2021'))]; - const lastSyncDate = new Date('Tue Oct 26 2021 18:58:18 GMT+1300'); + const vaultBooks = [book('1', new Date('October 20, 2021'), new Date('October 20, 2021'))]; - const actualBooks = diffBooks(remoteBooks, vaultBooks, lastSyncDate); + const actualBooks = diffBooks(remoteBooks, vaultBooks); expect(actualBooks.map((a) => a.id)).toEqual(['1']); }); - it('Books with same last annotated date two days before last sync will not be filtered for sync', () => { - const remoteBooks = [book('1', new Date('October 25, 2021'))]; - const vaultBooks = [book('1', new Date('October 25, 2021'))]; - const lastSyncDate = new Date('Tue Oct 27 2021 18:58:18 GMT+1300'); + it("Amazon's date matching our own last check (e.g. from us checking it) does not force a resync", () => { + // Simulates the exact trap this logic exists to avoid: our own check of the book bumped + // Amazon's "last accessed" date to the same day we recorded as lastChecked. That must not, + // by itself, look like new activity on the next sync. Uses "now" (rather than a fixed past + // date) so this test isn't also tripping the 90-day safety net below. + const checkedOn = new Date(); + const remoteBooks = [book('1', checkedOn)]; + const vaultBooks = [book('1', checkedOn, checkedOn)]; - const actualBooks = diffBooks(remoteBooks, vaultBooks, lastSyncDate); - expect(actualBooks.map((a) => a.id)).toHaveLength(0); + const actualBooks = diffBooks(remoteBooks, vaultBooks); + expect(actualBooks).toHaveLength(0); }); - it('Books with different last annotated dates are filtered for sync', () => { - const remoteBooks = [book('1', new Date('October 19, 2018'))]; - const vaultBooks = [book('1', new Date('August 6, 2018'))]; + it('A book untouched on Amazon since well before our last check is not resynced', () => { + const remoteBooks = [book('1', new Date('August 6, 2018'))]; + const vaultBooks = [book('1', new Date('August 6, 2018'), new Date())]; + + const actualBooks = diffBooks(remoteBooks, vaultBooks); + expect(actualBooks).toHaveLength(0); + }); - const actualBooks = diffBooks(remoteBooks, vaultBooks, new Date()); + it('Forces a recheck once more than 90 days have passed since we last checked, even with no new Amazon activity', () => { + const longAgo = moment().subtract(91, 'days').toDate(); + const remoteBooks = [book('1', longAgo)]; + const vaultBooks = [book('1', longAgo, longAgo)]; + + const actualBooks = diffBooks(remoteBooks, vaultBooks); expect(actualBooks.map((a) => a.id)).toEqual(['1']); }); + + it('Does not force a recheck before the 90-day safety net elapses', () => { + const recently = moment().subtract(89, 'days').toDate(); + const remoteBooks = [book('1', recently)]; + const vaultBooks = [book('1', recently, recently)]; + + const actualBooks = diffBooks(remoteBooks, vaultBooks); + expect(actualBooks).toHaveLength(0); + }); }); diff --git a/src/sync/syncManager/diffBooks.ts b/src/sync/syncManager/diffBooks.ts index 1a053be..e86a44f 100644 --- a/src/sync/syncManager/diffBooks.ts +++ b/src/sync/syncManager/diffBooks.ts @@ -3,40 +3,40 @@ import moment from 'moment'; import type { Book } from '~/models'; +// Safety net: force a full recheck at least this often, in case Amazon's "last accessed" date +// ever doesn't track a real content change the way we expect (e.g. a highlight swapped out for +// another on the same day, synced from a device that behaves differently, etc). +const REFRESH_INTERVAL_DAYS = 90; + const isEqual = (book1: Book, book2: Book): boolean => { return book1.id === book2.id; }; -const isSameDate = (date1: Date | undefined, date2: Date | undefined): boolean => { - return date1?.getTime() === date2?.getTime(); -}; +// Amazon's "last annotated date" is really a "last accessed" date - it updates whenever the +// book's notebook page is viewed, including by our own scrape. Comparing it against the date we +// last stored from a *previous* scrape means checking a book once poisons it forever: every +// subsequent sync sees "changed" (because we're the ones who changed it) and re-checks it again, +// which poisons it again. Comparing against our own `lastChecked` bookkeeping instead breaks the +// loop, since both values land on "today" at the moment we check, and only genuine outside +// activity after that point will move Amazon's date past it again. +const needsRecheck = (remote: Book, vault: Book): boolean => { + if (vault.lastChecked == null) { + return true; + } -const updatedSince = (book: Book, lastSyncDate: Date): boolean => { - if (book.lastAnnotatedDate != null) { - return moment(lastSyncDate) - .startOf('day') - .subtract(1, 'd') - .isSameOrBefore(book.lastAnnotatedDate); + if (remote.lastAnnotatedDate != null && moment(remote.lastAnnotatedDate).isAfter(vault.lastChecked, 'day')) { + return true; } - return false; + + return moment().diff(vault.lastChecked, 'days') > REFRESH_INTERVAL_DAYS; }; -export const diffBooks = ( - remoteBooks: Book[], - vaultBooks: Book[], - lastSyncDate: Date -): Book[] => { +export const diffBooks = (remoteBooks: Book[], vaultBooks: Book[]): Book[] => { const newBooks = remoteBooks.filter((remote) => !vaultBooks.some((v) => isEqual(v, remote))); - const diffAnnotatedDates = remoteBooks.filter((remote) => - vaultBooks.some( - (v) => isEqual(v, remote) && !isSameDate(remote.lastAnnotatedDate, v.lastAnnotatedDate) - ) - ); - - const updatedBooks = remoteBooks.filter((remote) => - vaultBooks.some((v) => isEqual(v, remote) && updatedSince(remote, lastSyncDate)) + const booksNeedingRecheck = remoteBooks.filter((remote) => + vaultBooks.some((v) => isEqual(v, remote) && needsRecheck(remote, v)) ); - return _.uniqBy([...newBooks, ...diffAnnotatedDates, ...updatedBooks], (book) => book.id); + return _.uniqBy([...newBooks, ...booksNeedingRecheck], (book) => book.id); }; diff --git a/src/sync/syncManager/index.ts b/src/sync/syncManager/index.ts index 68e6b16..b3664d7 100644 --- a/src/sync/syncManager/index.ts +++ b/src/sync/syncManager/index.ts @@ -17,14 +17,12 @@ export default class SyncManager { } public filterBooksToSync(remoteBooks: Book[]): Book[] { - const lastSyncDate = get(settingsStore).lastSyncDate; const ignoredBooks = get(settingsStore).ignoredBooks ?? []; const vaultBooks = this.fileManager.getKindleFiles(); const booksToSync = diffBooks( remoteBooks, - vaultBooks.map((v) => v.book), - lastSyncDate + vaultBooks.map((v) => v.book) ); if (ignoredBooks.length === 0) { @@ -39,13 +37,12 @@ export default class SyncManager { } public async syncBook(book: Book, highlights: Highlight[]): Promise { - if (highlights.length === 0) { - return; // No highlights for book. Skip sync - } - const file = this.fileManager.getKindleFile(book); if (file == null) { + if (highlights.length === 0) { + return; // Nothing to create yet + } await this.createBook(book, highlights); } else { await this.resyncBook(file, book, highlights); @@ -61,9 +58,9 @@ export default class SyncManager { const diffs = diffManager.diff(remoteHighlights); - if (diffs.length > 0) { - await diffManager.applyDiffs(remoteBook, remoteHighlights, diffs); - } + // Always write, even with zero diffs - this is what stamps lastChecked with today's date, so + // a book we just confirmed is unchanged doesn't look perpetually stale to diffBooks + await diffManager.applyDiffs(remoteBook, remoteHighlights, diffs); return diffs; } From f8e99c2d1ce0da4f9851a7e1b82aadbe518ee300 Mon Sep 17 00:00:00 2001 From: Tareef Kawaf Date: Sun, 13 Sep 2026 22:14:51 -0500 Subject: [PATCH 04/10] fix: wait for the metadata cache to catch up instead of guessing 50ms 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). --- src/fileManager/index.ts | 46 +++++++++++++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/src/fileManager/index.ts b/src/fileManager/index.ts index a79ae48..0f7fdfb 100644 --- a/src/fileManager/index.ts +++ b/src/fileManager/index.ts @@ -18,6 +18,40 @@ export default class FileManager { return await this.vault.cachedRead(file.file); } + /** + * Waits for Obsidian's metadataCache to actually reprocess a file we just wrote, instead of + * guessing with a fixed delay. Every read in this file (getKindleFiles/mapToKindleFile) goes + * through that same cache, so a fixed delay that's too short leaves a window where the very + * next sync reads pre-write frontmatter - which looks identical to "we never checked this book" + * and makes it look permanently stale no matter how many times it's actually corrected on disk. + * Falls back to a timeout so a write that Obsidian never reports as changed (e.g. no actual + * byte difference) can't hang the sync. + */ + private waitForMetadataCache(file: TFile, timeoutMs = 5000): Promise { + return new Promise((resolve) => { + let settled = false; + + const finish = (): void => { + if (settled) { + return; + } + settled = true; + clearTimeout(timeoutHandle); + this.metadataCache.off('changed', onChanged); + resolve(); + }; + + const onChanged = (changedFile: TFile): void => { + if (changedFile.path === file.path) { + finish(); + } + }; + + this.metadataCache.on('changed', onChanged); + const timeoutHandle = setTimeout(finish, timeoutMs); + }); + } + public getKindleFile(book: Book): KindleFile | undefined { const allSyncedFiles = this.getKindleFiles(); @@ -197,7 +231,7 @@ export default class FileManager { if (existingFile instanceof TFile) { try { await this.vault.modify(existingFile, frontmatterContent); - await new Promise((resolve) => setTimeout(resolve, 50)); + await this.waitForMetadataCache(existingFile); } catch (error) { console.error(`Error updating existing file (path="${filePath})"`); throw error; @@ -206,10 +240,8 @@ export default class FileManager { } try { - await this.vault.create(filePath, frontmatterContent); - // Give Obsidian time to process the new file and update metadata cache - // This helps prevent performance issues when syncing many books - await new Promise((resolve) => setTimeout(resolve, 50)); + const createdFile = await this.vault.create(filePath, frontmatterContent); + await this.waitForMetadataCache(createdFile); } catch (error) { console.error(`Error writing new file (path="${filePath})"`); throw error; @@ -226,9 +258,7 @@ export default class FileManager { try { await this.vault.modify(kindleFile.file, frontmatterContent); - // Give Obsidian time to process the file update and update metadata cache - // This helps prevent performance issues when syncing many books - await new Promise((resolve) => setTimeout(resolve, 50)); + await this.waitForMetadataCache(kindleFile.file); } catch (error) { console.error(`Error modifying e file (path="${kindleFile.file.path})"`); throw error; From b7a780a6138df61aaecadc051ea483b20ead3d6b Mon Sep 17 00:00:00 2001 From: Tareef Kawaf Date: Sun, 13 Sep 2026 22:31:50 -0500 Subject: [PATCH 05/10] fix: pick duplicate notes for the same book deterministically 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. --- src/fileManager/index.ts | 30 ++++++++++++++-- .../selectCanonicalDuplicate.spec.ts | 34 ++++++++++++++++++ src/fileManager/selectCanonicalDuplicate.ts | 36 +++++++++++++++++++ 3 files changed, 98 insertions(+), 2 deletions(-) create mode 100644 src/fileManager/selectCanonicalDuplicate.spec.ts create mode 100644 src/fileManager/selectCanonicalDuplicate.ts diff --git a/src/fileManager/index.ts b/src/fileManager/index.ts index 0f7fdfb..69e6d14 100644 --- a/src/fileManager/index.ts +++ b/src/fileManager/index.ts @@ -7,6 +7,8 @@ import { settingsStore } from '~/store'; import { mergeFrontmatter } from '~/utils'; import { bookFilePath, bookToFrontMatter, frontMatterToBook } from './mappers'; +import type { DuplicateCandidate } from './selectCanonicalDuplicate'; +import { pickBetterDuplicate } from './selectCanonicalDuplicate'; const SyncingStateKey = 'kindle-sync'; const PropertyPrefix = 'kindle-'; @@ -55,9 +57,33 @@ export default class FileManager { public getKindleFile(book: Book): KindleFile | undefined { const allSyncedFiles = this.getKindleFiles(); - const kindleFile = allSyncedFiles.find((file) => file.frontmatter.bookId === book.id); + const matches = allSyncedFiles.filter((file) => file.frontmatter.bookId === book.id); - return kindleFile == null ? undefined : { ...kindleFile, book }; + if (matches.length === 0) { + return undefined; + } + + // Under normal operation there's exactly one match. If a duplicate ever exists anyway (a + // leftover from a bug, a sync interrupted mid-write), pick deterministically instead of + // depending on vault enumeration order - otherwise different syncs can read and write + // different copies of "the same" book, which never converges. + const asDuplicateCandidate = (kindleFile: KindleFile): DuplicateCandidate => ({ + path: kindleFile.file.path, + highlightsCount: kindleFile.frontmatter.highlightsCount, + createdAt: kindleFile.file.stat.ctime, + }); + + const kindleFile = + matches.length === 1 + ? matches[0] + : matches.reduce((best, candidate) => + pickBetterDuplicate(asDuplicateCandidate(best), asDuplicateCandidate(candidate)).path === + best.file.path + ? best + : candidate + ); + + return { ...kindleFile, book }; } public mapToKindleFile(fileOrFolder: TAbstractFile): KindleFile | undefined { diff --git a/src/fileManager/selectCanonicalDuplicate.spec.ts b/src/fileManager/selectCanonicalDuplicate.spec.ts new file mode 100644 index 0000000..90e9ebe --- /dev/null +++ b/src/fileManager/selectCanonicalDuplicate.spec.ts @@ -0,0 +1,34 @@ +import { pickBetterDuplicate } from './selectCanonicalDuplicate'; + +describe('pickBetterDuplicate', () => { + it('prefers the file with more highlights', () => { + const a = { path: 'Book-111.md', highlightsCount: 93, createdAt: 1 }; + const b = { path: 'Book-222.md', highlightsCount: 164, createdAt: 2 }; + + expect(pickBetterDuplicate(a, b)).toBe(b); + expect(pickBetterDuplicate(b, a)).toBe(b); + }); + + it('prefers the canonical (non-suffixed) filename when highlight counts match', () => { + const suffixed = { path: 'Book-111.md', highlightsCount: 50, createdAt: 1 }; + const canonical = { path: 'Book.md', highlightsCount: 50, createdAt: 2 }; + + expect(pickBetterDuplicate(suffixed, canonical)).toBe(canonical); + expect(pickBetterDuplicate(canonical, suffixed)).toBe(canonical); + }); + + it('prefers the oldest file when counts match and neither (or both) has a suffix', () => { + const older = { path: 'Book-111.md', highlightsCount: 50, createdAt: 100 }; + const newer = { path: 'Book-222.md', highlightsCount: 50, createdAt: 200 }; + + expect(pickBetterDuplicate(older, newer)).toBe(older); + expect(pickBetterDuplicate(newer, older)).toBe(older); + }); + + it('treats a missing highlightsCount as worse than any real count', () => { + const unknown = { path: 'Book-111.md', highlightsCount: undefined, createdAt: 1 }; + const known = { path: 'Book-222.md', highlightsCount: 0, createdAt: 2 }; + + expect(pickBetterDuplicate(unknown, known)).toBe(known); + }); +}); diff --git a/src/fileManager/selectCanonicalDuplicate.ts b/src/fileManager/selectCanonicalDuplicate.ts new file mode 100644 index 0000000..5072465 --- /dev/null +++ b/src/fileManager/selectCanonicalDuplicate.ts @@ -0,0 +1,36 @@ +export type DuplicateCandidate = { + path: string; + highlightsCount?: number; + createdAt: number; +}; + +const hasNumericSuffix = (path: string): boolean => /-\d+\.md$/.test(path); + +/** + * Picks which of two files claiming the same bookId should be treated as canonical. Exists so + * that even if a duplicate note ever slips back in (a leftover from a bug, a sync interrupted + * mid-write, a manual copy), lookups stay consistent instead of depending on whatever order the + * vault happens to enumerate files in - which otherwise lets different syncs read and write + * different copies of the "same" book, permanently confusing anything that compares state between + * syncs (see the lastChecked bookkeeping this exists to protect). + * + * Preference order: most highlights (the most complete copy), then no numeric-suffix filename + * (the canonical name a fresh write would use), then oldest (the original, not a later copy). + */ +export const pickBetterDuplicate = (a: T, b: T): T => { + const aCount = a.highlightsCount ?? -1; + const bCount = b.highlightsCount ?? -1; + + if (aCount !== bCount) { + return aCount > bCount ? a : b; + } + + const aHasSuffix = hasNumericSuffix(a.path); + const bHasSuffix = hasNumericSuffix(b.path); + + if (aHasSuffix !== bHasSuffix) { + return aHasSuffix ? b : a; + } + + return a.createdAt <= b.createdAt ? a : b; +}; From 58d1d776802eb8c09b552d2c8de4eb2fa0991385 Mon Sep 17 00:00:00 2001 From: Tareef Kawaf Date: Sun, 13 Sep 2026 22:48:16 -0500 Subject: [PATCH 06/10] fix: create a note for books with zero highlights too 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. --- src/sync/syncManager/index.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/sync/syncManager/index.ts b/src/sync/syncManager/index.ts index b3664d7..f4a4d65 100644 --- a/src/sync/syncManager/index.ts +++ b/src/sync/syncManager/index.ts @@ -40,9 +40,10 @@ export default class SyncManager { const file = this.fileManager.getKindleFile(book); if (file == null) { - if (highlights.length === 0) { - return; // Nothing to create yet - } + // Still create a file even with zero highlights - a book with no note has no lastChecked + // to compare against, so it looks "new" again on every single sync, forever, exactly like + // the bugs fixed elsewhere in this codebase. Recording that we checked it (even if there's + // nothing to show yet) is what lets it drop out of the diff once confirmed empty. await this.createBook(book, highlights); } else { await this.resyncBook(file, book, highlights); From 6548974815f63f889490ecc65226c1f969a2e77e Mon Sep 17 00:00:00 2001 From: Tareef Kawaf Date: Mon, 14 Sep 2026 09:06:59 -0500 Subject: [PATCH 07/10] fix: don't discard a note's own frontmatter in the path-collision recovery 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. --- src/fileManager/index.ts | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/fileManager/index.ts b/src/fileManager/index.ts index 69e6d14..147d675 100644 --- a/src/fileManager/index.ts +++ b/src/fileManager/index.ts @@ -246,7 +246,6 @@ export default class FileManager { highlightsCount: number ): Promise { const filePath = normalizePath(bookFilePath(book, metadata)); - const frontmatterContent = this.generateBookContent(book, content, highlightsCount); // A file can already exist at this book's canonical path even though it wasn't found by its // stored bookId (e.g. Obsidian's metadata cache hasn't caught up with a recent write yet, or @@ -256,6 +255,13 @@ export default class FileManager { if (existingFile instanceof TFile) { try { + const existingContent = await this.vault.cachedRead(existingFile); + const frontmatterContent = this.generateBookContent( + book, + content, + highlightsCount, + existingContent + ); await this.vault.modify(existingFile, frontmatterContent); await this.waitForMetadataCache(existingFile); } catch (error) { @@ -266,6 +272,7 @@ export default class FileManager { } try { + const frontmatterContent = this.generateBookContent(book, content, highlightsCount); const createdFile = await this.vault.create(filePath, frontmatterContent); await this.waitForMetadataCache(createdFile); } catch (error) { @@ -296,12 +303,26 @@ export default class FileManager { * (b) rendered book highlights * Uses flat properties format compatible with Obsidian's properties system */ - private generateBookContent(book: Book, content: string, highlightsCount: number): string { + private generateBookContent( + book: Book, + content: string, + highlightsCount: number, + preserveFrontmatterFrom?: string + ): string { // lastChecked always means "the plugin wrote this file just now" - it's our own bookkeeping, // not something scraped from Amazon, so it's stamped fresh on every write rather than carried // over from whatever was on the book object passed in. const frontmatter = bookToFrontMatter({ ...book, lastChecked: new Date() }, highlightsCount); + // When rewriting a file we found by path rather than by id (see createFile), `content` is a + // freshly rendered body with no frontmatter of its own. Carrying over any frontmatter that + // already existed on disk - including fields the plugin knows nothing about, like a user's + // own rating or genre - before merging in the plugin's own fields, so a lookup miss can't + // silently discard someone's own data along with fixing the duplicate. + const baseContent = preserveFrontmatterFrom + ? matter.stringify(content, matter(preserveFrontmatterFrom).data) + : content; + // Use flat properties format for better Obsidian compatibility const flatProperties: Record = { [`${PropertyPrefix}bookId`]: frontmatter.bookId, @@ -324,7 +345,7 @@ export default class FileManager { // Strip any stale legacy nested block so it can't keep shadowing these freshly-written flat // properties on a future read (see the priority comment in mapToKindleFile) - return mergeFrontmatter(content, flatProperties, [SyncingStateKey]); + return mergeFrontmatter(baseContent, flatProperties, [SyncingStateKey]); } /** From c114f8e6a6622d0fbf46746a4c854f8ada51903f Mon Sep 17 00:00:00 2001 From: Tareef Kawaf Date: Fri, 2 Oct 2026 12:15:38 -0400 Subject: [PATCH 08/10] fix: retry vault reads that fail transiently on cloud-synced vaults 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. --- src/fileManager/index.ts | 9 ++++++--- src/utils/index.ts | 1 + src/utils/withRetry.spec.ts | 35 +++++++++++++++++++++++++++++++++++ src/utils/withRetry.ts | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 3 deletions(-) create mode 100644 src/utils/withRetry.spec.ts create mode 100644 src/utils/withRetry.ts diff --git a/src/fileManager/index.ts b/src/fileManager/index.ts index 147d675..4c58c4e 100644 --- a/src/fileManager/index.ts +++ b/src/fileManager/index.ts @@ -4,7 +4,7 @@ import { get } from 'svelte/store'; import type { Book, BookMetadata, KindleFile, KindleFrontmatter } from '~/models'; import { settingsStore } from '~/store'; -import { mergeFrontmatter } from '~/utils'; +import { mergeFrontmatter, withRetry } from '~/utils'; import { bookFilePath, bookToFrontMatter, frontMatterToBook } from './mappers'; import type { DuplicateCandidate } from './selectCanonicalDuplicate'; @@ -17,7 +17,10 @@ export default class FileManager { constructor(private vault: Vault, private metadataCache: MetadataCache) {} public async readFile(file: KindleFile): Promise { - return await this.vault.cachedRead(file.file); + // Reading from a cloud-synced vault (iCloud Drive, Dropbox, ...) can transiently time out if + // the file was evicted locally and needs to be fetched on demand, especially under the heavy, + // rapid I/O of syncing a large library. Retry a few times before giving up on this book. + return await withRetry(() => this.vault.cachedRead(file.file)); } /** @@ -255,7 +258,7 @@ export default class FileManager { if (existingFile instanceof TFile) { try { - const existingContent = await this.vault.cachedRead(existingFile); + const existingContent = await withRetry(() => this.vault.cachedRead(existingFile)); const frontmatterContent = this.generateBookContent( book, content, diff --git a/src/utils/index.ts b/src/utils/index.ts index bd99823..80da2cb 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -4,3 +4,4 @@ export * from './br2ln'; export * from './hash'; export * from './stringBuffer'; export * from './parseAuthor'; +export * from './withRetry'; diff --git a/src/utils/withRetry.spec.ts b/src/utils/withRetry.spec.ts new file mode 100644 index 0000000..38e1bed --- /dev/null +++ b/src/utils/withRetry.spec.ts @@ -0,0 +1,35 @@ +import { withRetry } from './withRetry'; + +describe('withRetry', () => { + it('returns the result on the first success without retrying', async () => { + const fn = jest.fn, []>(() => Promise.resolve('ok')); + + const result = await withRetry(fn, { delayMs: 0 }); + + expect(result).toBe('ok'); + expect(fn).toHaveBeenCalledTimes(1); + }); + + it('retries after a failure and returns the eventual success', async () => { + let calls = 0; + const fn = jest.fn, []>(() => { + calls++; + if (calls < 3) { + return Promise.reject(new Error('ETIMEDOUT')); + } + return Promise.resolve('ok'); + }); + + const result = await withRetry(fn, { attempts: 3, delayMs: 0 }); + + expect(result).toBe('ok'); + expect(fn).toHaveBeenCalledTimes(3); + }); + + it('throws the last error once all attempts are exhausted', async () => { + const fn = jest.fn, []>(() => Promise.reject(new Error('still failing'))); + + await expect(withRetry(fn, { attempts: 2, delayMs: 0 })).rejects.toThrow('still failing'); + expect(fn).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/utils/withRetry.ts b/src/utils/withRetry.ts new file mode 100644 index 0000000..83935f0 --- /dev/null +++ b/src/utils/withRetry.ts @@ -0,0 +1,32 @@ +export type WithRetryOptions = { + attempts?: number; + delayMs?: number; +}; + +/** + * Retries a failing async operation with a linearly increasing delay between attempts. Exists for + * operations reading from a cloud-synced vault (iCloud Drive, Dropbox, OneDrive, ...), where a + * file that's been evicted locally has to be fetched on demand before it can be read - something + * that can transiently time out or fail under heavy, rapid file I/O (e.g. syncing a large + * library) without there being anything actually wrong with the file itself. + */ +export const withRetry = async ( + fn: () => Promise, + { attempts = 3, delayMs = 500 }: WithRetryOptions = {} +): Promise => { + let lastError: unknown; + + for (let attempt = 0; attempt < attempts; attempt++) { + try { + return await fn(); + } catch (error) { + lastError = error; + + if (attempt < attempts - 1) { + await new Promise((resolve) => setTimeout(resolve, delayMs * (attempt + 1))); + } + } + } + + throw lastError; +}; From 6fbdaa92b3a8d41a0d446699e76aebfd777acdf6 Mon Sep 17 00:00:00 2001 From: Tareef Kawaf Date: Fri, 2 Oct 2026 12:34:51 -0400 Subject: [PATCH 09/10] fix: surface per-book sync errors in the completion screen, not just 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. --- src/components/syncModal/store.ts | 10 ++++++++-- .../syncModal/views/SyncingView.svelte | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/components/syncModal/store.ts b/src/components/syncModal/store.ts index 8db3e28..b8774a1 100644 --- a/src/components/syncModal/store.ts +++ b/src/components/syncModal/store.ts @@ -1,4 +1,4 @@ -import { writable } from 'svelte/store'; +import { get, writable } from 'svelte/store'; import { ee } from '~/eventEmitter'; import type { Book, Highlight, KindleFile, SyncMode } from '~/models'; @@ -169,7 +169,10 @@ const createSyncModalStore = () => { }); ee.on('syncSessionSuccess', () => { - addRootLog('Sync complete'); + const erroredCount = get(store).erroredJobs.length; + addRootLog( + erroredCount > 0 ? `Sync complete with ${erroredCount} error${erroredCount === 1 ? '' : 's'}` : 'Sync complete' + ); store.update((state) => ({ ...InitialState, status: 'sync:complete' as const, @@ -178,6 +181,9 @@ const createSyncModalStore = () => { highlightsSynced: state.highlightsSynced, syncDurationMs: state.syncStartedAt ? Date.now() - state.syncStartedAt : undefined, activityLog: state.activityLog, + // Carried over rather than reset - a session can complete overall while individual books + // failed along the way, and that's otherwise invisible outside the console. + erroredJobs: state.erroredJobs, })); }); diff --git a/src/components/syncModal/views/SyncingView.svelte b/src/components/syncModal/views/SyncingView.svelte index 45214c2..e5281dc 100644 --- a/src/components/syncModal/views/SyncingView.svelte +++ b/src/components/syncModal/views/SyncingView.svelte @@ -119,6 +119,7 @@ $: highlights = $store.highlightsSynced ?? 0; $: duration = $store.syncDurationMs; + $: errorCount = $store.erroredJobs.length; $: details = $store.activityLog; @@ -199,6 +200,13 @@ {:else} All books up to date {/if} + {#if errorCount > 0} +
+ {errorCount} {errorCount === 1 ? 'book' : 'books'} could not be synced due to errors. + Open Developer Tools (Cmd+Option+I, or View → Toggle Developer Tools) and check the + Console tab for details, or see "Details" below. +
+ {/if} {#if details.length > 0} @@ -576,4 +584,12 @@ font-size: 0.85em; margin-top: 6px; } + + .kp-syncmodal--complete-errors { + color: var(--text-error); + font-size: 0.85em; + margin-top: 14px; + max-width: 420px; + text-align: center; + } From 84da0011c66f6c4bbf8001299bd33491650719ef Mon Sep 17 00:00:00 2001 From: Tareef Kawaf Date: Fri, 2 Oct 2026 12:52:26 -0400 Subject: [PATCH 10/10] fix: stop logging the same scroll count repeatedly in the sync log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/scraper/loadRemoteDom.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/scraper/loadRemoteDom.ts b/src/scraper/loadRemoteDom.ts index 976c727..892ae01 100644 --- a/src/scraper/loadRemoteDom.ts +++ b/src/scraper/loadRemoteDom.ts @@ -58,6 +58,7 @@ export const loadRemoteDom = async ( return new Promise((resolveWrapper, rejectWrapper) => { let didNavigateUrl: string = null; let settled = false; + let lastLoggedCount: number | undefined; const settle = (fn: () => void): void => { if (settled) { @@ -126,8 +127,13 @@ export const loadRemoteDom = async ( { ...scrollOptions, onProgress: (count) => { - if (shouldLog) { + // scrollUntilStable reports every attempt, including the repeated identical + // reads it needs to confirm the count has actually stopped growing - logging + // every one of those just prints the same number several times in a row with + // nothing visibly happening. Only log when the count actually changes. + if (shouldLog && count !== lastLoggedCount) { ee.emit('syncLog', `${labelPrefix}Found ${count} item(s) so far…`); + lastLoggedCount = count; } scrollOptions.onProgress?.(count); },