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; + } diff --git a/src/fileManager/index.ts b/src/fileManager/index.ts index df054c9..4c58c4e 100644 --- a/src/fileManager/index.ts +++ b/src/fileManager/index.ts @@ -4,9 +4,11 @@ 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'; +import { pickBetterDuplicate } from './selectCanonicalDuplicate'; const SyncingStateKey = 'kindle-sync'; const PropertyPrefix = 'kindle-'; @@ -15,15 +17,76 @@ 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)); + } + + /** + * 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(); - const kindleFile = allSyncedFiles.find((file) => file.frontmatter.bookId === book.id); + const matches = allSyncedFiles.filter((file) => file.frontmatter.bookId === book.id); + + 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 == null ? undefined : { ...kindleFile, book }; + return { ...kindleFile, book }; } public mapToKindleFile(fileOrFolder: TAbstractFile): KindleFile | undefined { @@ -42,24 +105,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 +162,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 +202,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,14 +248,36 @@ export default class FileManager { content: string, highlightsCount: number ): Promise { - const filePath = this.generateUniqueFilePath(book, metadata); - const frontmatterContent = this.generateBookContent(book, content, highlightsCount); + const filePath = normalizePath(bookFilePath(book, metadata)); + + // 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 { + const existingContent = await withRetry(() => this.vault.cachedRead(existingFile)); + const frontmatterContent = this.generateBookContent( + book, + content, + highlightsCount, + existingContent + ); + await this.vault.modify(existingFile, frontmatterContent); + await this.waitForMetadataCache(existingFile); + } 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 - // This helps prevent performance issues when syncing many books - await new Promise((resolve) => setTimeout(resolve, 50)); + const frontmatterContent = this.generateBookContent(book, content, highlightsCount); + const createdFile = await this.vault.create(filePath, frontmatterContent); + await this.waitForMetadataCache(createdFile); } catch (error) { console.error(`Error writing new file (path="${filePath})"`); throw error; @@ -206,9 +294,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; @@ -220,17 +306,35 @@ 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 { - const frontmatter = bookToFrontMatter(book, highlightsCount); - + 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, [`${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 +346,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(baseContent, 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/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; +}; 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/scraper/loadRemoteDom.ts b/src/scraper/loadRemoteDom.ts index 9ed3e7a..892ae01 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,47 @@ 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; + let lastLoggedCount: number | undefined; + + 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 +108,39 @@ 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) => { + // 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); + }, + } + ); + } + if (timeout > 0) { if (shouldLog) { ee.emit( @@ -83,17 +167,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; +}; 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..f4a4d65 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,13 @@ 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) { + // 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); @@ -61,9 +59,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; } 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); }; 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; +};