From 55c577e911c713c333684c565b94ceff2d9ce1e8 Mon Sep 17 00:00:00 2001 From: sudacode Date: Sat, 28 Feb 2026 20:07:42 -0800 Subject: [PATCH] fix: address latest claude review findings --- .../anki-connect-proxy.test.ts | 30 ++++++++ src/anki-integration/anki-connect-proxy.ts | 3 +- .../services/tokenizer/annotation-stage.ts | 1 + src/main/runtime/startup-warmups.test.ts | 68 +++++++++++++++++++ src/main/runtime/startup-warmups.ts | 17 ++--- .../subtitle-tokenization-main-deps.test.ts | 26 +++++++ .../subtitle-tokenization-main-deps.ts | 2 +- 7 files changed, 137 insertions(+), 10 deletions(-) diff --git a/src/anki-integration/anki-connect-proxy.test.ts b/src/anki-integration/anki-connect-proxy.test.ts index 9a8de47..c830921 100644 --- a/src/anki-integration/anki-connect-proxy.test.ts +++ b/src/anki-integration/anki-connect-proxy.test.ts @@ -265,6 +265,36 @@ test('proxy does not fallback-enqueue latest note for multi requests without add assert.deepEqual(processed, []); }); +test('proxy fallback-enqueues latest note for addNote responses without note IDs and escapes deck quotes', async () => { + const processed: number[] = []; + const findNotesQueries: string[] = []; + const proxy = new AnkiConnectProxyServer({ + shouldAutoUpdateNewCards: () => true, + processNewCard: async (noteId) => { + processed.push(noteId); + }, + getDeck: () => 'My "Japanese" Deck', + findNotes: async (query) => { + findNotesQueries.push(query); + return [500, 501]; + }, + logInfo: () => undefined, + logWarn: () => undefined, + logError: () => undefined, + }); + + (proxy as unknown as { + maybeEnqueueFromRequest: (request: Record, responseBody: Buffer) => void; + }).maybeEnqueueFromRequest( + { action: 'addNote' }, + Buffer.from(JSON.stringify({ result: 0, error: null }), 'utf8'), + ); + + await waitForCondition(() => processed.length === 1); + assert.deepEqual(findNotesQueries, ['"deck:My \\"Japanese\\" Deck" added:1']); + assert.deepEqual(processed, [501]); +}); + test('proxy detects self-referential loop configuration', () => { const proxy = new AnkiConnectProxyServer({ shouldAutoUpdateNewCards: () => true, diff --git a/src/anki-integration/anki-connect-proxy.ts b/src/anki-integration/anki-connect-proxy.ts index 4d75df9..685fa17 100644 --- a/src/anki-integration/anki-connect-proxy.ts +++ b/src/anki-integration/anki-connect-proxy.ts @@ -233,7 +233,8 @@ export class AnkiConnectProxyServer { try { const deck = this.deps.getDeck ? this.deps.getDeck() : undefined; - const query = deck ? `"deck:${deck}" added:1` : 'added:1'; + const escapedDeck = deck ? deck.replace(/"/g, '\\"') : undefined; + const query = escapedDeck ? `"deck:${escapedDeck}" added:1` : 'added:1'; const noteIds = await findNotes(query, { maxRetries: 0 }); if (!noteIds || noteIds.length === 0) { return; diff --git a/src/core/services/tokenizer/annotation-stage.ts b/src/core/services/tokenizer/annotation-stage.ts index 922f873..52d36c0 100644 --- a/src/core/services/tokenizer/annotation-stage.ts +++ b/src/core/services/tokenizer/annotation-stage.ts @@ -81,6 +81,7 @@ function isExcludedByTagSet( if (parts.length === 0) { return false; } + // Composite tags like "助詞|名詞" stay eligible unless every component is excluded. return parts.every((part) => exclusions.has(part)); } diff --git a/src/main/runtime/startup-warmups.test.ts b/src/main/runtime/startup-warmups.test.ts index f4cf45d..8b6c6c7 100644 --- a/src/main/runtime/startup-warmups.test.ts +++ b/src/main/runtime/startup-warmups.test.ts @@ -13,6 +13,17 @@ function shouldAutoConnectJellyfinRemote(config: { return config.enabled && config.remoteControlEnabled && config.remoteControlAutoConnect; } +function createDeferred(): { + promise: Promise; + resolve: () => void; +} { + let resolve!: () => void; + const promise = new Promise((nextResolve) => { + resolve = nextResolve; + }); + return { promise, resolve }; +} + test('launchBackgroundWarmupTask logs completion timing', async () => { const debugLogs: string[] = []; const launchTask = createLaunchBackgroundWarmupTaskHandler({ @@ -194,3 +205,60 @@ test('startBackgroundWarmups logs per-stage progress for enabled tokenization wa assert.ok(debugLogs.includes('[startup-warmup] stage start: jellyfin-remote-session')); assert.ok(debugLogs.includes('[startup-warmup] stage ready: jellyfin-remote-session')); }); + +test('startBackgroundWarmups starts mecab and dictionary warmups without waiting for yomitan warmup', async () => { + const startedStages: string[] = []; + let started = false; + let subtitleTokenizationTask: Promise | null = null; + const yomitanDeferred = createDeferred(); + const mecabDeferred = createDeferred(); + const subtitleDictionariesDeferred = createDeferred(); + + const startWarmups = createStartBackgroundWarmupsHandler({ + getStarted: () => started, + setStarted: (value) => { + started = value; + }, + isTexthookerOnlyMode: () => false, + launchTask: (label, task) => { + if (label === 'subtitle-tokenization') { + subtitleTokenizationTask = task(); + } + }, + createMecabTokenizerAndCheck: async () => { + startedStages.push('mecab'); + await mecabDeferred.promise; + }, + ensureYomitanExtensionLoaded: async () => { + startedStages.push('yomitan-extension'); + await yomitanDeferred.promise; + }, + prewarmSubtitleDictionaries: async () => { + startedStages.push('subtitle-dictionaries'); + await subtitleDictionariesDeferred.promise; + }, + shouldWarmupMecab: () => true, + shouldWarmupYomitanExtension: () => true, + shouldWarmupSubtitleDictionaries: () => true, + shouldWarmupJellyfinRemoteSession: () => false, + shouldAutoConnectJellyfinRemote: () => false, + startJellyfinRemoteSession: async () => {}, + }); + + startWarmups(); + await Promise.resolve(); + await Promise.resolve(); + + assert.ok(subtitleTokenizationTask); + assert.equal(startedStages.includes('yomitan-extension'), true); + assert.equal(startedStages.includes('mecab'), true); + assert.equal(startedStages.includes('subtitle-dictionaries'), true); + + yomitanDeferred.resolve(); + mecabDeferred.resolve(); + subtitleDictionariesDeferred.resolve(); + if (!subtitleTokenizationTask) { + throw new Error('Expected subtitle tokenization warmup task'); + } + await subtitleTokenizationTask; +}); diff --git a/src/main/runtime/startup-warmups.ts b/src/main/runtime/startup-warmups.ts index 00381f7..fadc8ba 100644 --- a/src/main/runtime/startup-warmups.ts +++ b/src/main/runtime/startup-warmups.ts @@ -47,15 +47,16 @@ export function createStartBackgroundWarmupsHandler(deps: { warmupMecab || warmupYomitanExtension || warmupSubtitleDictionaries; if (shouldWarmupTokenization) { deps.launchTask('subtitle-tokenization', async () => { - if (warmupYomitanExtension) { - deps.logDebug?.('[startup-warmup] stage start: yomitan-extension'); - await deps.ensureYomitanExtensionLoaded(); - deps.logDebug?.('[startup-warmup] stage ready: yomitan-extension'); - } else { - deps.logDebug?.('[startup-warmup] stage skipped: yomitan-extension'); - } - await Promise.all([ + warmupYomitanExtension + ? (async () => { + deps.logDebug?.('[startup-warmup] stage start: yomitan-extension'); + await deps.ensureYomitanExtensionLoaded(); + deps.logDebug?.('[startup-warmup] stage ready: yomitan-extension'); + })() + : Promise.resolve().then(() => { + deps.logDebug?.('[startup-warmup] stage skipped: yomitan-extension'); + }), warmupMecab ? (async () => { deps.logDebug?.('[startup-warmup] stage start: mecab'); diff --git a/src/main/runtime/subtitle-tokenization-main-deps.test.ts b/src/main/runtime/subtitle-tokenization-main-deps.test.ts index d951012..aa1654e 100644 --- a/src/main/runtime/subtitle-tokenization-main-deps.test.ts +++ b/src/main/runtime/subtitle-tokenization-main-deps.test.ts @@ -180,3 +180,29 @@ test('dictionary prewarm does not show OSD when notifications are disabled', asy assert.deepEqual(osdMessages, []); }); + +test('dictionary prewarm clears loading OSD timer even if notifications are disabled before completion', async () => { + const clearedTimers: unknown[] = []; + const jlptDeferred = createDeferred(); + const freqDeferred = createDeferred(); + let shouldShowNotification = true; + + const prewarm = createPrewarmSubtitleDictionariesMainHandler({ + ensureJlptDictionaryLookup: async () => jlptDeferred.promise, + ensureFrequencyDictionaryLookup: async () => freqDeferred.promise, + shouldShowOsdNotification: () => shouldShowNotification, + showMpvOsd: () => undefined, + setInterval: () => 'loading-timer', + clearInterval: (timer) => { + clearedTimers.push(timer); + }, + }); + + const prewarmPromise = prewarm({ showLoadingOsd: true }); + shouldShowNotification = false; + jlptDeferred.resolve(); + freqDeferred.resolve(); + await prewarmPromise; + + assert.deepEqual(clearedTimers, ['loading-timer']); +}); diff --git a/src/main/runtime/subtitle-tokenization-main-deps.ts b/src/main/runtime/subtitle-tokenization-main-deps.ts index 7511c38..d047b42 100644 --- a/src/main/runtime/subtitle-tokenization-main-deps.ts +++ b/src/main/runtime/subtitle-tokenization-main-deps.ts @@ -113,7 +113,7 @@ export function createPrewarmSubtitleDictionariesMainHandler(deps: { }; const endLoadingOsd = (): void => { - if (!showMpvOsd || !shouldShowOsdNotification()) { + if (!showMpvOsd) { return; }