From 3995c396f88d7eb23d43e4c25e137ead99ec5e49 Mon Sep 17 00:00:00 2001 From: sudacode Date: Thu, 19 Mar 2026 23:13:43 -0700 Subject: [PATCH] fix(review): address latest CodeRabbit comments --- launcher/commands/stats-command.ts | 97 ++++----- src/anki-integration.ts | 25 ++- src/anki-integration/card-creation.test.ts | 198 ++++++++++++++++++ src/anki-integration/card-creation.ts | 14 +- src/anki-integration/known-word-cache.test.ts | 95 +++++++++ src/anki-integration/known-word-cache.ts | 30 +-- 6 files changed, 379 insertions(+), 80 deletions(-) diff --git a/launcher/commands/stats-command.ts b/launcher/commands/stats-command.ts index 7ce11cb..6edb6a2 100644 --- a/launcher/commands/stats-command.ts +++ b/launcher/commands/stats-command.ts @@ -29,6 +29,11 @@ type StatsCommandDeps = { const STATS_STARTUP_RESPONSE_TIMEOUT_MS = 12_000; +type StatsResponseWait = { + controller: AbortController; + promise: Promise<{ kind: 'response'; response: StatsCommandResponse }>; +}; + const defaultDeps: StatsCommandDeps = { createTempDir: (prefix) => fs.mkdtempSync(path.join(os.tmpdir(), prefix)), joinPath: (...parts) => path.join(...parts), @@ -62,6 +67,41 @@ const defaultDeps: StatsCommandDeps = { }, }; +async function performStartupHandshake( + createResponseWait: () => StatsResponseWait, + attachedExitPromise: Promise, +): Promise { + const responseWait = createResponseWait(); + const startupResult = await Promise.race([ + responseWait.promise, + attachedExitPromise.then((status) => ({ kind: 'exit' as const, status })), + ]); + + if (startupResult.kind === 'exit') { + if (startupResult.status !== 0) { + responseWait.controller.abort(); + throw new Error(`Stats app exited before startup response (status ${startupResult.status}).`); + } + + const response = await responseWait.promise.then((result) => result.response); + if (!response.ok) { + throw new Error(response.error || 'Stats dashboard failed to start.'); + } + return true; + } + + if (!startupResult.response.ok) { + throw new Error(startupResult.response.error || 'Stats dashboard failed to start.'); + } + + const exitStatus = await attachedExitPromise; + if (exitStatus !== 0) { + throw new Error(`Stats app exited with status ${exitStatus}.`); + } + + return true; +} + export async function runStatsCommand( context: LauncherCommandContext, deps: Partial = {}, @@ -120,62 +160,7 @@ export async function runStatsCommand( return true; } - if (!args.statsCleanup && !args.statsStop) { - const responseWait = createResponseWait(); - const startupResult = await Promise.race([ - responseWait.promise, - attachedExitPromise.then((status) => ({ kind: 'exit' as const, status })), - ]); - if (startupResult.kind === 'exit') { - if (startupResult.status !== 0) { - responseWait.controller.abort(); - throw new Error( - `Stats app exited before startup response (status ${startupResult.status}).`, - ); - } - const response = await responseWait.promise.then((result) => result.response); - if (!response.ok) { - throw new Error(response.error || 'Stats dashboard failed to start.'); - } - return true; - } - if (!startupResult.response.ok) { - throw new Error(startupResult.response.error || 'Stats dashboard failed to start.'); - } - const exitStatus = await attachedExitPromise; - if (exitStatus !== 0) { - throw new Error(`Stats app exited with status ${exitStatus}.`); - } - return true; - } - const attachedExitPromiseCleanup = attachedExitPromise; - const responseWait = createResponseWait(); - - const startupResult = await Promise.race([ - responseWait.promise, - attachedExitPromiseCleanup.then((status) => ({ kind: 'exit' as const, status })), - ]); - if (startupResult.kind === 'exit') { - if (startupResult.status !== 0) { - responseWait.controller.abort(); - throw new Error( - `Stats app exited before startup response (status ${startupResult.status}).`, - ); - } - const response = await responseWait.promise.then((result) => result.response); - if (!response.ok) { - throw new Error(response.error || 'Stats dashboard failed to start.'); - } - return true; - } - if (!startupResult.response.ok) { - throw new Error(startupResult.response.error || 'Stats dashboard failed to start.'); - } - const exitStatus = await attachedExitPromiseCleanup; - if (exitStatus !== 0) { - throw new Error(`Stats app exited with status ${exitStatus}.`); - } - return true; + return await performStartupHandshake(createResponseWait, attachedExitPromise); } finally { resolvedDeps.removeDir(tempDir); } diff --git a/src/anki-integration.ts b/src/anki-integration.ts index b57037c..1eda157 100644 --- a/src/anki-integration.ts +++ b/src/anki-integration.ts @@ -199,6 +199,25 @@ export class AnkiIntegration { }); } + private recordCardsMinedSafely( + count: number, + noteIds: number[] | undefined, + source: string, + ): void { + if (!this.recordCardsMinedCallback) { + return; + } + + try { + this.recordCardsMinedCallback(count, noteIds); + } catch (error) { + log.warn( + `recordCardsMined callback failed during ${source}:`, + (error as Error).message, + ); + } + } + private createKnownWordCache(knownWordCacheStatePath?: string): KnownWordCacheManager { return new KnownWordCacheManager({ client: { @@ -221,7 +240,7 @@ export class AnkiIntegration { shouldAutoUpdateNewCards: () => this.config.behavior?.autoUpdateNewCards !== false, processNewCard: (noteId) => this.processNewCard(noteId), recordCardsAdded: (count, noteIds) => { - this.recordCardsMinedCallback?.(count, noteIds); + this.recordCardsMinedSafely(count, noteIds, 'polling'); }, isUpdateInProgress: () => this.updateInProgress, setUpdateInProgress: (value) => { @@ -245,7 +264,7 @@ export class AnkiIntegration { shouldAutoUpdateNewCards: () => this.config.behavior?.autoUpdateNewCards !== false, processNewCard: (noteId: number) => this.processNewCard(noteId), recordCardsAdded: (count, noteIds) => { - this.recordCardsMinedCallback?.(count, noteIds); + this.recordCardsMinedSafely(count, noteIds, 'proxy'); }, getDeck: () => this.config.deck, findNotes: async (query, options) => @@ -345,7 +364,7 @@ export class AnkiIntegration { this.previousNoteIds.add(noteId); }, recordCardsMinedCallback: (count, noteIds) => { - this.recordCardsMinedCallback?.(count, noteIds); + this.recordCardsMinedSafely(count, noteIds, 'card creation'); }, }); } diff --git a/src/anki-integration/card-creation.test.ts b/src/anki-integration/card-creation.test.ts index 81bdc71..abfab36 100644 --- a/src/anki-integration/card-creation.test.ts +++ b/src/anki-integration/card-creation.test.ts @@ -85,3 +85,201 @@ test('CardCreationService counts locally created sentence cards', async () => { assert.equal(created, true); assert.deepEqual(minedCards, [{ count: 1, noteIds: [42] }]); }); + +test('CardCreationService keeps updating after trackLastAddedNoteId throws', async () => { + const calls = { + notesInfo: 0, + updateNoteFields: 0, + }; + const service = new CardCreationService({ + getConfig: () => + ({ + deck: 'Mining', + fields: { + sentence: 'Sentence', + audio: 'SentenceAudio', + }, + media: { + generateAudio: false, + generateImage: false, + }, + behavior: {}, + ai: false, + }) as AnkiConnectConfig, + getAiConfig: () => ({}), + getTimingTracker: () => ({}) as never, + getMpvClient: () => + ({ + currentVideoPath: '/video.mp4', + currentSubText: '字幕', + currentSubStart: 1, + currentSubEnd: 2, + currentTimePos: 1.5, + currentAudioStreamIndex: 0, + }) as never, + client: { + addNote: async () => 42, + addTags: async () => undefined, + notesInfo: async () => { + calls.notesInfo += 1; + return [ + { + noteId: 42, + fields: { + Sentence: { value: 'existing' }, + }, + }, + ]; + }, + updateNoteFields: async () => { + calls.updateNoteFields += 1; + }, + storeMediaFile: async () => undefined, + findNotes: async () => [], + retrieveMediaFile: async () => '', + }, + mediaGenerator: { + generateAudio: async () => null, + generateScreenshot: async () => null, + generateAnimatedImage: async () => null, + }, + showOsdNotification: () => undefined, + showUpdateResult: () => undefined, + showStatusNotification: () => undefined, + showNotification: async () => undefined, + beginUpdateProgress: () => undefined, + endUpdateProgress: () => undefined, + withUpdateProgress: async (_message, action) => action(), + resolveConfiguredFieldName: () => null, + resolveNoteFieldName: () => null, + getAnimatedImageLeadInSeconds: async () => 0, + extractFields: () => ({}), + processSentence: (sentence) => sentence, + setCardTypeFields: (updatedFields) => { + updatedFields.CardType = 'sentence'; + }, + mergeFieldValue: (_existing, newValue) => newValue, + formatMiscInfoPattern: () => '', + getEffectiveSentenceCardConfig: () => ({ + model: 'Sentence', + sentenceField: 'Sentence', + audioField: 'SentenceAudio', + lapisEnabled: false, + kikuEnabled: false, + kikuFieldGrouping: 'disabled', + kikuDeleteDuplicateInAuto: false, + }), + getFallbackDurationSeconds: () => 10, + appendKnownWordsFromNoteInfo: () => undefined, + isUpdateInProgress: () => false, + setUpdateInProgress: () => undefined, + trackLastAddedNoteId: () => { + throw new Error('track failed'); + }, + }); + + const created = await service.createSentenceCard('テスト', 0, 1); + + assert.equal(created, true); + assert.equal(calls.notesInfo, 1); + assert.equal(calls.updateNoteFields, 1); +}); + +test('CardCreationService keeps updating after recordCardsMinedCallback throws', async () => { + const calls = { + notesInfo: 0, + updateNoteFields: 0, + }; + const service = new CardCreationService({ + getConfig: () => + ({ + deck: 'Mining', + fields: { + sentence: 'Sentence', + audio: 'SentenceAudio', + }, + media: { + generateAudio: false, + generateImage: false, + }, + behavior: {}, + ai: false, + }) as AnkiConnectConfig, + getAiConfig: () => ({}), + getTimingTracker: () => ({}) as never, + getMpvClient: () => + ({ + currentVideoPath: '/video.mp4', + currentSubText: '字幕', + currentSubStart: 1, + currentSubEnd: 2, + currentTimePos: 1.5, + currentAudioStreamIndex: 0, + }) as never, + client: { + addNote: async () => 42, + addTags: async () => undefined, + notesInfo: async () => { + calls.notesInfo += 1; + return [ + { + noteId: 42, + fields: { + Sentence: { value: 'existing' }, + }, + }, + ]; + }, + updateNoteFields: async () => { + calls.updateNoteFields += 1; + }, + storeMediaFile: async () => undefined, + findNotes: async () => [], + retrieveMediaFile: async () => '', + }, + mediaGenerator: { + generateAudio: async () => null, + generateScreenshot: async () => null, + generateAnimatedImage: async () => null, + }, + showOsdNotification: () => undefined, + showUpdateResult: () => undefined, + showStatusNotification: () => undefined, + showNotification: async () => undefined, + beginUpdateProgress: () => undefined, + endUpdateProgress: () => undefined, + withUpdateProgress: async (_message, action) => action(), + resolveConfiguredFieldName: () => null, + resolveNoteFieldName: () => null, + getAnimatedImageLeadInSeconds: async () => 0, + extractFields: () => ({}), + processSentence: (sentence) => sentence, + setCardTypeFields: (updatedFields) => { + updatedFields.CardType = 'sentence'; + }, + mergeFieldValue: (_existing, newValue) => newValue, + formatMiscInfoPattern: () => '', + getEffectiveSentenceCardConfig: () => ({ + model: 'Sentence', + sentenceField: 'Sentence', + audioField: 'SentenceAudio', + lapisEnabled: false, + kikuEnabled: false, + kikuFieldGrouping: 'disabled', + kikuDeleteDuplicateInAuto: false, + }), + getFallbackDurationSeconds: () => 10, + appendKnownWordsFromNoteInfo: () => undefined, + isUpdateInProgress: () => false, + setUpdateInProgress: () => undefined, + recordCardsMinedCallback: () => { + throw new Error('record failed'); + }, + }); + + const created = await service.createSentenceCard('テスト', 0, 1); + + assert.equal(created, true); + assert.equal(calls.notesInfo, 1); + assert.equal(calls.updateNoteFields, 1); +}); diff --git a/src/anki-integration/card-creation.ts b/src/anki-integration/card-creation.ts index c9c830b..85bd9c3 100644 --- a/src/anki-integration/card-creation.ts +++ b/src/anki-integration/card-creation.ts @@ -551,14 +551,24 @@ export class CardCreationService { this.getConfiguredAnkiTags(), ); log.info('Created sentence card:', noteId); - this.deps.trackLastAddedNoteId?.(noteId); - this.deps.recordCardsMinedCallback?.(1, [noteId]); } catch (error) { log.error('Failed to create sentence card:', (error as Error).message); this.deps.showUpdateResult(`Sentence card failed: ${(error as Error).message}`, false); return false; } + try { + this.deps.trackLastAddedNoteId?.(noteId); + } catch (error) { + log.warn('Failed to track last added note:', (error as Error).message); + } + + try { + this.deps.recordCardsMinedCallback?.(1, [noteId]); + } catch (error) { + log.warn('Failed to record mined card:', (error as Error).message); + } + try { const noteInfoResult = await this.deps.client.notesInfo([noteId]); const noteInfos = noteInfoResult as CardCreationNoteInfo[]; diff --git a/src/anki-integration/known-word-cache.test.ts b/src/anki-integration/known-word-cache.test.ts index 48c9815..25bf14d 100644 --- a/src/anki-integration/known-word-cache.test.ts +++ b/src/anki-integration/known-word-cache.test.ts @@ -263,6 +263,101 @@ test('KnownWordCacheManager refresh incrementally reconciles deleted and edited } }); +test('KnownWordCacheManager preserves cache state key captured before refresh work', async () => { + const config: AnkiConnectConfig = { + fields: { + word: 'Word', + }, + knownWords: { + highlightEnabled: true, + refreshMinutes: 1, + }, + }; + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-known-word-cache-key-')); + const statePath = path.join(stateDir, 'known-words-cache.json'); + let notesInfoStarted = false; + let releaseNotesInfo!: () => void; + const notesInfoGate = new Promise((resolve) => { + releaseNotesInfo = resolve; + }); + const manager = new KnownWordCacheManager({ + client: { + findNotes: async () => [1], + notesInfo: async () => { + notesInfoStarted = true; + await notesInfoGate; + return [ + { + noteId: 1, + fields: { + Word: { value: '猫' }, + }, + }, + ]; + }, + }, + getConfig: () => config, + knownWordCacheStatePath: statePath, + showStatusNotification: () => undefined, + }); + + try { + const refreshPromise = manager.refresh(true); + await waitForCondition(() => notesInfoStarted); + + config.fields = { + ...config.fields, + word: 'Expression', + }; + releaseNotesInfo(); + await refreshPromise; + + const persisted = JSON.parse(fs.readFileSync(statePath, 'utf-8')) as { + scope: string; + words: string[]; + }; + assert.equal( + persisted.scope, + '{"refreshMinutes":1,"scope":"is:note","fieldsWord":"Word"}', + ); + assert.deepEqual(persisted.words, ['猫']); + } finally { + fs.rmSync(stateDir, { recursive: true, force: true }); + } +}); + +test('KnownWordCacheManager does not borrow fields from other decks during refresh', async () => { + const config: AnkiConnectConfig = { + knownWords: { + highlightEnabled: true, + decks: { + Mining: [], + Reading: ['AltWord'], + }, + }, + }; + const { manager, clientState, cleanup } = createKnownWordCacheHarness(config); + + try { + clientState.findNotesByQuery.set('deck:"Mining"', [1]); + clientState.findNotesByQuery.set('deck:"Reading"', []); + clientState.notesInfoResult = [ + { + noteId: 1, + fields: { + AltWord: { value: '猫' }, + }, + }, + ]; + + await manager.refresh(true); + + assert.equal(manager.isKnownWord('猫'), false); + } finally { + cleanup(); + } +}); + test('KnownWordCacheManager invalidates persisted cache when per-deck fields change', () => { const config: AnkiConnectConfig = { fields: { diff --git a/src/anki-integration/known-word-cache.ts b/src/anki-integration/known-word-cache.ts index 07c55c3..176364b 100644 --- a/src/anki-integration/known-word-cache.ts +++ b/src/anki-integration/known-word-cache.ts @@ -227,6 +227,7 @@ export class KnownWordCacheManager { return; } + const frozenStateKey = this.getKnownWordCacheStateKey(); this.isRefreshingKnownWords = true; try { const noteFieldsById = await this.fetchKnownWordNoteFieldsById(); @@ -257,7 +258,7 @@ export class KnownWordCacheManager { } this.knownWordsLastRefreshedAtMs = Date.now(); - this.knownWordsStateKey = this.getKnownWordCacheStateKey(); + this.knownWordsStateKey = frozenStateKey; this.persistKnownWordCacheState(); log.info( 'Known-word cache refreshed', @@ -284,6 +285,11 @@ export class KnownWordCacheManager { return getKnownWordCacheRefreshIntervalMinutes(this.deps.getConfig()) * 60_000; } + private getDefaultKnownWordFields(): string[] { + const configuredWordField = getConfiguredWordFieldName(this.deps.getConfig()); + return [...new Set([configuredWordField, 'Word', 'Reading', 'Word Reading'])]; + } + private getKnownWordDecks(): string[] { const configuredDecks = this.deps.getConfig().knownWords?.decks; if (configuredDecks && typeof configuredDecks === 'object' && !Array.isArray(configuredDecks)) { @@ -297,20 +303,7 @@ export class KnownWordCacheManager { } private getConfiguredFields(): string[] { - const configuredDecks = this.deps.getConfig().knownWords?.decks; - if (configuredDecks && typeof configuredDecks === 'object' && !Array.isArray(configuredDecks)) { - const allFields = new Set(); - for (const fields of Object.values(configuredDecks)) { - if (Array.isArray(fields)) { - for (const f of fields) { - if (typeof f === 'string' && f.trim()) allFields.add(f.trim()); - } - } - } - if (allFields.size > 0) return [...allFields]; - } - const configuredWordField = getConfiguredWordFieldName(this.deps.getConfig()); - return [...new Set([configuredWordField, 'Word', 'Reading', 'Word Reading'])]; + return this.getDefaultKnownWordFields(); } private getImmediateAppendFields(): string[] | null { @@ -344,8 +337,7 @@ export class KnownWordCacheManager { } } - const configuredWordField = getConfiguredWordFieldName(this.deps.getConfig()); - return [...new Set([configuredWordField, 'Word', 'Reading', 'Word Reading'])]; + return this.getDefaultKnownWordFields(); } return this.getConfiguredFields(); @@ -365,7 +357,7 @@ export class KnownWordCacheManager { : []; scopes.push({ query: `deck:"${escapeAnkiSearchValue(trimmedDeckName)}"`, - fields: normalizedFields.length > 0 ? normalizedFields : this.getConfiguredFields(), + fields: normalizedFields.length > 0 ? normalizedFields : this.getDefaultKnownWordFields(), }); } if (scopes.length > 0) { @@ -373,7 +365,7 @@ export class KnownWordCacheManager { } } - return [{ query: this.buildKnownWordsQuery(), fields: this.getConfiguredFields() }]; + return [{ query: this.buildKnownWordsQuery(), fields: this.getDefaultKnownWordFields() }]; } private buildKnownWordsQuery(): string {