fix(library): surface comic vine api errors + sync summary transparency
This commit is contained in:
parent
f146de1b76
commit
d76ed2631b
5 changed files with 98 additions and 6 deletions
|
|
@ -146,17 +146,25 @@ export class LibraryController {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Surfaces every counter, not just synced/written/flipped -- a run where fetches are
|
||||||
|
* silently failing (e.g. Comic Vine error envelope) must not read as an empty-but-clean
|
||||||
|
* success just because skippedNoData/errors were left out of the message. */
|
||||||
|
private buildSyncSummary(spec: MediaTypeSpec, mode: string, report: LibraryReport): string {
|
||||||
|
let msg = `Library ${mode}sync (${spec.typeName}): ${report.scanned} scanned, ${report.synced} ok, ${report.written} updated, ${report.flipped.length} flipped`;
|
||||||
|
if (report.skippedStatic) msg += `, ${report.skippedStatic} static`;
|
||||||
|
if (report.skippedNoId) msg += `, ${report.skippedNoId} no-id`;
|
||||||
|
if (report.skippedNoData) msg += `, ${report.skippedNoData} no-data (see console)`;
|
||||||
|
if (report.errors.length) msg += `, ${report.errors.length} errors (see console)`;
|
||||||
|
return msg;
|
||||||
|
}
|
||||||
|
|
||||||
private async runSync(spec: MediaTypeSpec, full: boolean, dryRun: boolean, quiet: boolean): Promise<LibraryReport> {
|
private async runSync(spec: MediaTypeSpec, full: boolean, dryRun: boolean, quiet: boolean): Promise<LibraryReport> {
|
||||||
this.warnMissingKey(spec, quiet);
|
this.warnMissingKey(spec, quiet);
|
||||||
const deps = this.makeDeps(spec, dryRun);
|
const deps = this.makeDeps(spec, dryRun);
|
||||||
const report = await libraryFolderSync(spec, deps, { full, dryRun });
|
const report = await libraryFolderSync(spec, deps, { full, dryRun });
|
||||||
const mode = dryRun ? 'DRY-RUN ' : '';
|
const mode = dryRun ? 'DRY-RUN ' : '';
|
||||||
if (shouldNotifySync(quiet, report.written, report.errors.length)) {
|
if (shouldNotifySync(quiet, report.written, report.errors.length)) {
|
||||||
this.notify(
|
this.notify(this.buildSyncSummary(spec, mode, report), 0);
|
||||||
`Library ${mode}sync (${spec.typeName}): ${report.synced} checked, ${report.written} updated, ${report.flipped.length} flipped` +
|
|
||||||
(report.errors.length ? `, ${report.errors.length} errors (see console)` : ''),
|
|
||||||
0,
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
return report;
|
return report;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -139,9 +139,27 @@ export function renderComic(r: ComicRecord, myNotes: string, customSections: Cus
|
||||||
return fm.join('\n') + '\n' + b.join('\n');
|
return fm.join('\n') + '\n' + b.join('\n');
|
||||||
}
|
}
|
||||||
|
|
||||||
async function fetchVolumeResults(query: string, key: string, deps: SpecDeps): Promise<any[]> {
|
/**
|
||||||
|
* Comic Vine returns HTTP 200 even on auth/request failures -- the real signal is the
|
||||||
|
* envelope's `status_code` (1 = OK; anything else carries a human-readable `error`, e.g.
|
||||||
|
* "Invalid API Key"). Left un-checked, callers see empty `results` and treat it as
|
||||||
|
* "nothing found" instead of a real failure. `status_code` absent entirely (as in older
|
||||||
|
* hand-built test fixtures) is treated as OK -- only an explicit non-1 code trips this.
|
||||||
|
*/
|
||||||
|
function checkEnvelope(data: any, deps: SpecDeps): boolean {
|
||||||
|
if (data && data.status_code !== undefined && data.status_code !== 1) {
|
||||||
|
const msg = data.error || 'Unknown error';
|
||||||
|
deps.log(`Comic Vine error: ${msg}`);
|
||||||
|
deps.notify(`Comic Vine: ${msg}`);
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
async function fetchVolumeResults(query: string, key: string, deps: SpecDeps): Promise<any[] | null> {
|
||||||
const qs = new URLSearchParams({ api_key: key, format: 'json', filter: `name:${query}`, limit: '10' });
|
const qs = new URLSearchParams({ api_key: key, format: 'json', filter: `name:${query}`, limit: '10' });
|
||||||
const res = await deps.http(`${COMICVINE_BASE}/volumes/?${qs.toString()}`, {});
|
const res = await deps.http(`${COMICVINE_BASE}/volumes/?${qs.toString()}`, {});
|
||||||
|
if (!checkEnvelope(res, deps)) return null;
|
||||||
return Array.isArray(res?.results) ? res.results : [];
|
return Array.isArray(res?.results) ? res.results : [];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -183,6 +201,7 @@ export const comicSpec: MediaTypeSpec = {
|
||||||
if (!query) return null;
|
if (!query) return null;
|
||||||
try {
|
try {
|
||||||
const results = await fetchVolumeResults(query, key, deps);
|
const results = await fetchVolumeResults(query, key, deps);
|
||||||
|
if (results === null) return null; // envelope error already logged/notified
|
||||||
if (results.length === 0) return null;
|
if (results.length === 0) return null;
|
||||||
const q = query.toLowerCase();
|
const q = query.toLowerCase();
|
||||||
const exacts = results.filter(r => String(r.name ?? '').toLowerCase() === q);
|
const exacts = results.filter(r => String(r.name ?? '').toLowerCase() === q);
|
||||||
|
|
@ -218,6 +237,8 @@ export const comicSpec: MediaTypeSpec = {
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (!checkEnvelope(json, deps)) return null;
|
||||||
|
|
||||||
const result = json?.results;
|
const result = json?.results;
|
||||||
if (!result || !result.name || (result.id != null && String(result.id) !== id)) {
|
if (!result || !result.name || (result.id != null && String(result.id) !== id)) {
|
||||||
// identity-guard: never silently swap to a different volume's data; leave the note
|
// identity-guard: never silently swap to a different volume's data; leave the note
|
||||||
|
|
|
||||||
2
tests/fixtures/comicvine-volume.json
vendored
2
tests/fixtures/comicvine-volume.json
vendored
|
|
@ -1,4 +1,6 @@
|
||||||
{
|
{
|
||||||
|
"error": "OK",
|
||||||
|
"status_code": 1,
|
||||||
"results": {
|
"results": {
|
||||||
"id": 195824,
|
"id": 195824,
|
||||||
"name": "Absolute Batman",
|
"name": "Absolute Batman",
|
||||||
|
|
|
||||||
|
|
@ -329,6 +329,16 @@ describe('comicSpec.resolve', () => {
|
||||||
const result = await comicSpec.resolve(ctx, deps);
|
const result = await comicSpec.resolve(ctx, deps);
|
||||||
expect(result).toBeNull();
|
expect(result).toBeNull();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('Comic Vine error envelope (invalid key) -> null, logged', async () => {
|
||||||
|
const deps = makeDeps({
|
||||||
|
http: async () => ({ error: 'Invalid API Key', status_code: 100, results: [] }),
|
||||||
|
getKey: () => 'bad-key',
|
||||||
|
});
|
||||||
|
const result = await comicSpec.resolve(ctxFor({ title: 'Absolute Batman' }), deps);
|
||||||
|
expect(result).toBeNull();
|
||||||
|
expect(deps.logCalls.some(m => m.includes('Invalid API Key'))).toBe(true);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('comicSpec.sync — no id / no key', () => {
|
describe('comicSpec.sync — no id / no key', () => {
|
||||||
|
|
@ -385,6 +395,17 @@ describe('comicSpec.sync — comicvine enrich', () => {
|
||||||
expect(deps.notifyCalls).toEqual([]);
|
expect(deps.notifyCalls).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('Comic Vine error envelope (invalid key) -> null, notify + log', async () => {
|
||||||
|
const deps = makeDeps({
|
||||||
|
http: async () => ({ error: 'Invalid API Key', status_code: 100, results: [] }),
|
||||||
|
getKey: () => 'bad-key',
|
||||||
|
});
|
||||||
|
const result = await comicSpec.sync(ctxFor({ comicvine_id: '195824' }), deps);
|
||||||
|
expect(result).toBeNull();
|
||||||
|
expect(deps.notifyCalls.some(m => m.includes('Invalid API Key'))).toBe(true);
|
||||||
|
expect(deps.logCalls.some(m => m.includes('Invalid API Key'))).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
test('fetches from GET /volume/4050-{id}/ with api_key + format=json', async () => {
|
test('fetches from GET /volume/4050-{id}/ with api_key + format=json', async () => {
|
||||||
let calledUrl = '';
|
let calledUrl = '';
|
||||||
const deps = makeDeps({
|
const deps = makeDeps({
|
||||||
|
|
|
||||||
|
|
@ -1,4 +1,5 @@
|
||||||
import { describe, expect, test } from 'bun:test';
|
import { describe, expect, test } from 'bun:test';
|
||||||
|
import type { MediaTypeSpec } from 'packages/obsidian/src/library/types';
|
||||||
import { bookSpec } from 'packages/obsidian/src/library/book';
|
import { bookSpec } from 'packages/obsidian/src/library/book';
|
||||||
import { gameSpec } from 'packages/obsidian/src/library/game';
|
import { gameSpec } from 'packages/obsidian/src/library/game';
|
||||||
import { LibraryController } from 'packages/obsidian/src/library/LibraryController';
|
import { LibraryController } from 'packages/obsidian/src/library/LibraryController';
|
||||||
|
|
@ -421,3 +422,42 @@ describe('dry-run notify suppression (minor)', () => {
|
||||||
expect(logs.some(m => m.includes('«Test Manga» ch. 5 out'))).toBe(true);
|
expect(logs.some(m => m.includes('«Test Manga» ch. 5 out'))).toBe(true);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('sync summary transparency', () => {
|
||||||
|
test('scanned + no-data counts surface in summary notice, not hidden behind a silent-looking 0/0', async () => {
|
||||||
|
const c = new LibraryController(fakePlugin());
|
||||||
|
const notices: string[] = [];
|
||||||
|
(c as any).notify = (msg: string) => notices.push(msg);
|
||||||
|
(c as any).makeDeps = () => ({
|
||||||
|
listNotes: async () => [{ path: 'Mangas/Test.md' }],
|
||||||
|
readNote: async () => 'content',
|
||||||
|
writeNote: async () => {},
|
||||||
|
sleep: async () => {},
|
||||||
|
log: () => {},
|
||||||
|
specDeps: fakeSpecDeps(),
|
||||||
|
});
|
||||||
|
|
||||||
|
// simulates e.g. a Comic Vine error-envelope: fetch "succeeds" (no throw) but spec.sync
|
||||||
|
// has no usable data -> counted as skippedNoData rather than a written/errors change
|
||||||
|
const noDataSpec: MediaTypeSpec = {
|
||||||
|
typeName: 'manga',
|
||||||
|
itemType: 'manga_item',
|
||||||
|
folderSettingKey: 'libraryMangaFolder',
|
||||||
|
enabledSettingKey: 'libraryMangaEnabled',
|
||||||
|
throttleMs: 0,
|
||||||
|
hasId: () => true,
|
||||||
|
isActive: () => true,
|
||||||
|
resolve: async () => null,
|
||||||
|
sync: async () => null,
|
||||||
|
};
|
||||||
|
|
||||||
|
const report = await c.syncType(noDataSpec, false);
|
||||||
|
|
||||||
|
expect(report.scanned).toBe(1);
|
||||||
|
expect(report.skippedNoData).toBe(1);
|
||||||
|
expect(notices.length).toBe(1);
|
||||||
|
expect(notices[0]).toContain('1 scanned');
|
||||||
|
expect(notices[0]).toContain('0 ok');
|
||||||
|
expect(notices[0]).toContain('1 no-data (see console)');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue