From 548e4c8ce0be7e7ed8ab3eace07211004def3a8d Mon Sep 17 00:00:00 2001 From: afiqzudinhadi Date: Mon, 3 Aug 2026 22:09:04 +0800 Subject: [PATCH] fix(library): isolate per-type sync failures, suppress dry-run notices syncAll() ran each enabled type's sync in sequence with no per-type error handling, so one type throwing (e.g. its vault folder missing) aborted the whole run and skipped libraryLastSync -- every other enabled type silently never synced. Each type's runSync is now wrapped individually: failures are logged (and surfaced via Notice unless quiet) and the loop continues; libraryLastSync still updates after the loop regardless. Also: dry-run sync previews now route spec-level flip notifications (the "chapter/issue out" Notices) through the log instead of a real Notice, since a dry run never actually writes the change it's describing. --- .../obsidian/src/library/LibraryController.ts | 30 +++- tests/library-controller.test.ts | 147 ++++++++++++++++++ 2 files changed, 170 insertions(+), 7 deletions(-) diff --git a/packages/obsidian/src/library/LibraryController.ts b/packages/obsidian/src/library/LibraryController.ts index fba6a85..21b5a21 100644 --- a/packages/obsidian/src/library/LibraryController.ts +++ b/packages/obsidian/src/library/LibraryController.ts @@ -91,17 +91,25 @@ export class LibraryController { }; } - private makeSpecDeps(): SpecDeps { + /** dryRun=true routes flip notifications to the log instead of a real Notice -- a dry-run + * preview shouldn't pop user-facing notices for changes that were never actually written. */ + private makeSpecDeps(dryRun = false): SpecDeps { return { http: this.makeHttp(), httpText: this.makeHttpText(), getKey: name => this.getKey(name), log: msg => console.log(`[media-db-library] ${msg}`), - notify: msg => this.notify(msg), + notify: msg => { + if (dryRun) { + console.log(`[media-db-library] [dry] ${msg}`); + } else { + this.notify(msg); + } + }, }; } - private makeDeps(spec: MediaTypeSpec): LibraryEngineDeps { + private makeDeps(spec: MediaTypeSpec, dryRun = false): LibraryEngineDeps { const { app } = this.plugin; const folderPath = this.folderFor(spec); const folder = app.vault.getAbstractFileByPath(folderPath); @@ -122,7 +130,7 @@ export class LibraryController { }, sleep: ms => new Promise(r => setTimeout(r, ms)), log: msg => console.log(`[media-db-library] ${msg}`), - specDeps: this.makeSpecDeps(), + specDeps: this.makeSpecDeps(dryRun), }; } @@ -140,7 +148,7 @@ export class LibraryController { private async runSync(spec: MediaTypeSpec, full: boolean, dryRun: boolean, quiet: boolean): Promise { this.warnMissingKey(spec, quiet); - const deps = this.makeDeps(spec); + const deps = this.makeDeps(spec, dryRun); const report = await libraryFolderSync(spec, deps, { full, dryRun }); const mode = dryRun ? 'DRY-RUN ' : ''; if (shouldNotifySync(quiet, report.written, report.errors.length)) { @@ -172,7 +180,7 @@ export class LibraryController { } this.syncing = true; try { - const deps = this.makeDeps(spec); + const deps = this.makeDeps(spec, dryRun); const report = await libraryFolderResolve(spec, deps, { dryRun }); const mode = dryRun ? 'DRY-RUN ' : ''; this.notify( @@ -194,7 +202,15 @@ export class LibraryController { try { for (const spec of this.specs) { if (!this.enabledFor(spec)) continue; - await this.runSync(spec, full, false, quiet); + try { + await this.runSync(spec, full, false, quiet); + } catch (e) { + // one type's failure (e.g. its vault folder is missing) must not abort the + // rest of the run -- log + surface it, then keep going with the other types + const msg = `Library sync failed for ${spec.typeName}: ${e instanceof Error ? e.message : String(e)}`; + console.log(`[media-db-library] ${msg}`); + if (!quiet) this.notify(msg); + } } if (!full) { this.plugin.settings.libraryLastSync = Date.now(); diff --git a/tests/library-controller.test.ts b/tests/library-controller.test.ts index f61697e..4452c1c 100644 --- a/tests/library-controller.test.ts +++ b/tests/library-controller.test.ts @@ -274,3 +274,150 @@ describe('missing API key handling', () => { expect(notices.filter(m => m.includes('API key not configured')).length).toBe(1); }); }); + +describe('syncAll error isolation (I5)', () => { + test('first spec throws (e.g. missing vault folder) -> remaining specs still synced, lastSync updated', async () => { + const c = new LibraryController(fakePlugin()); + const notices: string[] = []; + (c as any).notify = (msg: string) => notices.push(msg); + const calledTypes: string[] = []; + (c as any).makeDeps = (spec: { typeName: string }) => { + calledTypes.push(spec.typeName); + if (spec.typeName === 'manga') { + return { + listNotes: async () => { + throw new Error('Library folder not found: Mangas'); + }, + readNote: async () => '', + writeNote: async () => {}, + sleep: async () => {}, + log: () => {}, + specDeps: fakeSpecDeps(), + }; + } + return { + listNotes: async () => [], + readNote: async () => '', + writeNote: async () => {}, + sleep: async () => {}, + log: () => {}, + specDeps: fakeSpecDeps(), + }; + }; + + const before = (c as any).plugin.settings.libraryLastSync; + await c.syncAll(false, false); + + // manga (first in LIBRARY_TYPES) threw, but book/game/comic still got their turn + expect(calledTypes.sort()).toEqual(['book', 'comic', 'game', 'manga']); + expect(notices.some(m => m.includes('Library sync failed for manga'))).toBe(true); + expect((c as any).plugin.settings.libraryLastSync).toBeGreaterThan(before); + }); + + test('quiet sync: per-type failure logged only, no Notice', async () => { + const c = new LibraryController(fakePlugin()); + const notices: string[] = []; + (c as any).notify = (msg: string) => notices.push(msg); + const logs: string[] = []; + const origLog = console.log; + console.log = (msg: string) => { + logs.push(msg); + }; + (c as any).makeDeps = (spec: { typeName: string }) => ({ + listNotes: async () => { + if (spec.typeName === 'book') throw new Error('boom'); + return []; + }, + readNote: async () => '', + writeNote: async () => {}, + sleep: async () => {}, + log: () => {}, + specDeps: fakeSpecDeps(), + }); + + try { + await c.syncAll(false, true); + } finally { + console.log = origLog; + } + + expect(notices.some(m => m.includes('Library sync failed for book'))).toBe(false); + expect(logs.some(m => m.includes('Library sync failed for book'))).toBe(true); + }); +}); + +describe('dry-run notify suppression (minor)', () => { + test('makeSpecDeps(dryRun=true) -> notify logs instead of raising a Notice', () => { + const c = new LibraryController(fakePlugin()); + const notices: string[] = []; + (c as any).notify = (msg: string) => notices.push(msg); + const logs: string[] = []; + const origLog = console.log; + console.log = (msg: string) => { + logs.push(msg); + }; + try { + const specDeps = (c as any).makeSpecDeps(true); + specDeps.notify('«Test Manga» ch. 5 out'); + } finally { + console.log = origLog; + } + expect(notices).toEqual([]); + expect(logs.some(m => m.includes('«Test Manga» ch. 5 out'))).toBe(true); + }); + + test('makeSpecDeps(dryRun=false) -> notify raises a Notice normally', () => { + const c = new LibraryController(fakePlugin()); + const notices: string[] = []; + (c as any).notify = (msg: string) => notices.push(msg); + const specDeps = (c as any).makeSpecDeps(false); + specDeps.notify('«Test Manga» ch. 5 out'); + expect(notices).toEqual(['«Test Manga» ch. 5 out']); + }); + + test('dry-run manga flip scenario end-to-end: syncType(dryRun=true) -> no Notice, log entry present', async () => { + const c = new LibraryController(fakePlugin()); + const notices: string[] = []; + (c as any).notify = (msg: string) => notices.push(msg); + const logs: string[] = []; + const origLog = console.log; + console.log = (msg: string) => { + logs.push(msg); + }; + + const flipSpec = { + typeName: 'manga' as const, + itemType: 'manga_item', + folderSettingKey: 'libraryMangaFolder', + enabledSettingKey: 'libraryMangaEnabled', + throttleMs: 0, + hasId: () => true, + isActive: () => true, + resolve: async () => null, + sync: async (_ctx: unknown, deps: { notify: (msg: string) => void }) => { + deps.notify('«Test Manga» ch. 5 out'); + return { content: 'updated content', flipped: true }; + }, + }; + + (c as any).makeDeps = (spec: unknown, dryRun: boolean) => ({ + listNotes: async () => [{ path: 'Mangas/Test Manga.md' }], + readNote: async () => 'orig content', + writeNote: async () => {}, + sleep: async () => {}, + log: () => {}, + specDeps: (c as any).makeSpecDeps(dryRun), + }); + + try { + await c.syncType(flipSpec as any, false, true); // dryRun = true + } finally { + console.log = origLog; + } + + // the run-summary Notice (dry-run or not) is unrelated existing behavior and still fires; + // what must NOT happen is the spec's flip-notify reaching a real Notice + expect(notices.some(m => m.includes('«Test Manga» ch. 5 out'))).toBe(false); + expect(logs.some(m => m.includes('«Test Manga» ch. 5 out'))).toBe(true); + }); +});