From 274b0619acd493364ac50fa48fb20a15a4f31e61 Mon Sep 17 00:00:00 2001 From: sudacode Date: Thu, 19 Mar 2026 08:47:31 -0700 Subject: [PATCH] fix(anki): address latest PR 19 review follow-ups --- launcher/mpv.test.ts | 72 ++++++++--- src/anki-integration.test.ts | 28 +++++ src/anki-integration.ts | 2 +- src/anki-integration/known-word-cache.test.ts | 112 ++++++++++++++++++ src/anki-integration/known-word-cache.ts | 102 +++++++++++----- src/anki-integration/runtime.test.ts | 33 ++++++ src/anki-integration/runtime.ts | 50 +++----- 7 files changed, 315 insertions(+), 84 deletions(-) create mode 100644 src/anki-integration/known-word-cache.test.ts diff --git a/launcher/mpv.test.ts b/launcher/mpv.test.ts index c61ce4e..8eef0c6 100644 --- a/launcher/mpv.test.ts +++ b/launcher/mpv.test.ts @@ -245,6 +245,44 @@ function makeExecutable(filePath: string): void { fs.chmodSync(filePath, 0o755); } +function withFindAppBinaryEnvSandbox(run: () => void): void { + const originalAppImagePath = process.env.SUBMINER_APPIMAGE_PATH; + const originalBinaryPath = process.env.SUBMINER_BINARY_PATH; + try { + delete process.env.SUBMINER_APPIMAGE_PATH; + delete process.env.SUBMINER_BINARY_PATH; + run(); + } finally { + if (originalAppImagePath === undefined) { + delete process.env.SUBMINER_APPIMAGE_PATH; + } else { + process.env.SUBMINER_APPIMAGE_PATH = originalAppImagePath; + } + if (originalBinaryPath === undefined) { + delete process.env.SUBMINER_BINARY_PATH; + } else { + process.env.SUBMINER_BINARY_PATH = originalBinaryPath; + } + } +} + +function withAccessSyncStub(isExecutablePath: (filePath: string) => boolean, run: () => void): void { + const originalAccessSync = fs.accessSync; + try { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (fs as any).accessSync = (filePath: string): void => { + if (isExecutablePath(filePath)) { + return; + } + throw Object.assign(new Error(`EACCES: ${filePath}`), { code: 'EACCES' }); + }; + run(); + } finally { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (fs as any).accessSync = originalAccessSync; + } +} + test('findAppBinary resolves ~/.local/bin/SubMiner.AppImage when it exists', () => { const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-test-home-')); const originalHomedir = os.homedir; @@ -253,8 +291,10 @@ test('findAppBinary resolves ~/.local/bin/SubMiner.AppImage when it exists', () const appImage = path.join(baseDir, '.local/bin/SubMiner.AppImage'); makeExecutable(appImage); - const result = findAppBinary('/some/other/path/subminer'); - assert.equal(result, appImage); + withFindAppBinaryEnvSandbox(() => { + const result = findAppBinary('/some/other/path/subminer'); + assert.equal(result, appImage); + }); } finally { os.homedir = originalHomedir; fs.rmSync(baseDir, { recursive: true, force: true }); @@ -264,22 +304,16 @@ test('findAppBinary resolves ~/.local/bin/SubMiner.AppImage when it exists', () test('findAppBinary resolves /opt/SubMiner/SubMiner.AppImage when ~/.local/bin candidate does not exist', () => { const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-test-home-')); const originalHomedir = os.homedir; - const originalAccessSync = fs.accessSync; try { os.homedir = () => baseDir; - // No ~/.local/bin/SubMiner.AppImage; patch accessSync so only /opt path is executable - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (fs as any).accessSync = (filePath: string, mode?: number): void => { - if (filePath === '/opt/SubMiner/SubMiner.AppImage') return; - throw Object.assign(new Error(`EACCES: ${filePath}`), { code: 'EACCES' }); - }; - - const result = findAppBinary('/some/other/path/subminer'); - assert.equal(result, '/opt/SubMiner/SubMiner.AppImage'); + withFindAppBinaryEnvSandbox(() => { + withAccessSyncStub((filePath) => filePath === '/opt/SubMiner/SubMiner.AppImage', () => { + const result = findAppBinary('/some/other/path/subminer'); + assert.equal(result, '/opt/SubMiner/SubMiner.AppImage'); + }); + }); } finally { os.homedir = originalHomedir; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (fs as any).accessSync = originalAccessSync; fs.rmSync(baseDir, { recursive: true, force: true }); } }); @@ -296,9 +330,13 @@ test('findAppBinary finds subminer on PATH when AppImage candidates do not exist makeExecutable(wrapperPath); process.env.PATH = `${binDir}${path.delimiter}${originalPath ?? ''}`; - // selfPath must differ from wrapperPath so the self-check does not exclude it - const result = findAppBinary(path.join(baseDir, 'launcher', 'subminer')); - assert.equal(result, wrapperPath); + withFindAppBinaryEnvSandbox(() => { + withAccessSyncStub((filePath) => filePath === wrapperPath, () => { + // selfPath must differ from wrapperPath so the self-check does not exclude it + const result = findAppBinary(path.join(baseDir, 'launcher', 'subminer')); + assert.equal(result, wrapperPath); + }); + }); } finally { os.homedir = originalHomedir; process.env.PATH = originalPath; diff --git a/src/anki-integration.test.ts b/src/anki-integration.test.ts index 6a6feae..a3fbf85 100644 --- a/src/anki-integration.test.ts +++ b/src/anki-integration.test.ts @@ -250,6 +250,34 @@ test('AnkiIntegration does not allocate proxy server when proxy transport is dis assert.equal(privateState.runtime.proxyServer, null); }); +test('AnkiIntegration marks partial update notifications as failures in OSD mode', async () => { + const osdMessages: string[] = []; + const integration = new AnkiIntegration( + { + behavior: { + notificationType: 'osd', + }, + }, + {} as never, + {} as never, + (text) => { + osdMessages.push(text); + }, + ); + + await ( + integration as unknown as { + showNotification: ( + noteId: number, + label: string | number, + errorSuffix?: string, + ) => Promise; + } + ).showNotification(42, 'taberu', 'image failed'); + + assert.deepEqual(osdMessages, ['x Updated card: taberu (image failed)']); +}); + test('FieldGroupingMergeCollaborator synchronizes ExpressionAudio from merged SentenceAudio', async () => { const collaborator = createFieldGroupingMergeCollaborator(); diff --git a/src/anki-integration.ts b/src/anki-integration.ts index 7654407..9752887 100644 --- a/src/anki-integration.ts +++ b/src/anki-integration.ts @@ -913,7 +913,7 @@ export class AnkiIntegration { const type = this.config.behavior?.notificationType || 'osd'; if (type === 'osd' || type === 'both') { - this.showUpdateResult(message, true); + this.showUpdateResult(message, errorSuffix === undefined); } else { this.clearUpdateProgress(); } diff --git a/src/anki-integration/known-word-cache.test.ts b/src/anki-integration/known-word-cache.test.ts new file mode 100644 index 0000000..14c3946 --- /dev/null +++ b/src/anki-integration/known-word-cache.test.ts @@ -0,0 +1,112 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +import type { AnkiConnectConfig } from '../types'; +import { KnownWordCacheManager } from './known-word-cache'; + +function createKnownWordCacheHarness(config: AnkiConnectConfig): { + manager: KnownWordCacheManager; + cleanup: () => void; +} { + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-known-word-cache-')); + const statePath = path.join(stateDir, 'known-words-cache.json'); + const manager = new KnownWordCacheManager({ + client: { + findNotes: async () => [], + notesInfo: async () => [], + }, + getConfig: () => config, + knownWordCacheStatePath: statePath, + showStatusNotification: () => undefined, + }); + + return { + manager, + cleanup: () => { + fs.rmSync(stateDir, { recursive: true, force: true }); + }, + }; +} + +test('KnownWordCacheManager invalidates persisted cache when fields.word changes', () => { + const config: AnkiConnectConfig = { + deck: 'Mining', + fields: { + word: 'Word', + }, + knownWords: { + highlightEnabled: true, + }, + }; + const { manager, cleanup } = createKnownWordCacheHarness(config); + + try { + manager.appendFromNoteInfo({ + noteId: 1, + fields: { + Word: { value: '猫' }, + }, + }); + assert.equal(manager.isKnownWord('猫'), true); + + config.fields = { + ...config.fields, + word: 'Expression', + }; + + ( + manager as unknown as { + loadKnownWordCacheState: () => void; + } + ).loadKnownWordCacheState(); + + assert.equal(manager.isKnownWord('猫'), false); + } finally { + cleanup(); + } +}); + +test('KnownWordCacheManager invalidates persisted cache when per-deck fields change', () => { + const config: AnkiConnectConfig = { + fields: { + word: 'Word', + }, + knownWords: { + highlightEnabled: true, + decks: { + Mining: ['Word'], + }, + }, + }; + const { manager, cleanup } = createKnownWordCacheHarness(config); + + try { + manager.appendFromNoteInfo({ + noteId: 1, + fields: { + Word: { value: '猫' }, + }, + }); + assert.equal(manager.isKnownWord('猫'), true); + + config.knownWords = { + ...config.knownWords, + decks: { + Mining: ['Expression'], + }, + }; + + ( + manager as unknown as { + loadKnownWordCacheState: () => void; + } + ).loadKnownWordCacheState(); + + assert.equal(manager.isKnownWord('猫'), false); + } finally { + cleanup(); + } +}); diff --git a/src/anki-integration/known-word-cache.ts b/src/anki-integration/known-word-cache.ts index b67ec27..06ffd77 100644 --- a/src/anki-integration/known-word-cache.ts +++ b/src/anki-integration/known-word-cache.ts @@ -8,6 +8,57 @@ import { createLogger } from '../logger'; const log = createLogger('anki').child('integration.known-word-cache'); +function trimToNonEmptyString(value: unknown): string | null { + if (typeof value !== 'string') return null; + const trimmed = value.trim(); + return trimmed.length > 0 ? trimmed : null; +} + +export function getKnownWordCacheRefreshIntervalMinutes(config: AnkiConnectConfig): number { + const refreshMinutes = config.knownWords?.refreshMinutes; + return typeof refreshMinutes === 'number' && Number.isFinite(refreshMinutes) && refreshMinutes > 0 + ? refreshMinutes + : DEFAULT_ANKI_CONNECT_CONFIG.knownWords.refreshMinutes; +} + +export function getKnownWordCacheScopeForConfig(config: AnkiConnectConfig): string { + const configuredDecks = config.knownWords?.decks; + if (configuredDecks && typeof configuredDecks === 'object' && !Array.isArray(configuredDecks)) { + const normalizedDecks = Object.entries(configuredDecks) + .map(([deckName, fields]) => { + const name = trimToNonEmptyString(deckName); + if (!name) return null; + const normalizedFields = Array.isArray(fields) + ? [ + ...new Set( + fields + .map(String) + .map(trimToNonEmptyString) + .filter((field): field is string => Boolean(field)), + ), + ].sort() + : []; + return [name, normalizedFields]; + }) + .filter((entry): entry is [string, string[]] => entry !== null) + .sort(([a], [b]) => a.localeCompare(b)); + if (normalizedDecks.length > 0) { + return `decks:${JSON.stringify(normalizedDecks)}`; + } + } + + const configuredDeck = trimToNonEmptyString(config.deck); + return configuredDeck ? `deck:${configuredDeck}` : 'is:note'; +} + +export function getKnownWordCacheLifecycleConfig(config: AnkiConnectConfig): string { + return JSON.stringify({ + refreshMinutes: getKnownWordCacheRefreshIntervalMinutes(config), + scope: getKnownWordCacheScopeForConfig(config), + fieldsWord: trimToNonEmptyString(config.fields?.word) ?? '', + }); +} + export interface KnownWordCacheNoteInfo { noteId: number; fields: Record; @@ -39,7 +90,7 @@ interface KnownWordCacheDeps { export class KnownWordCacheManager { private knownWordsLastRefreshedAtMs = 0; - private knownWordsScope = ''; + private knownWordsStateKey = ''; private knownWords: Set = new Set(); private knownWordsRefreshTimer: ReturnType | null = null; private isRefreshingKnownWords = false; @@ -73,7 +124,7 @@ export class KnownWordCacheManager { } const refreshMinutes = this.getKnownWordRefreshIntervalMs() / 60_000; - const scope = this.getKnownWordCacheScope(); + const scope = getKnownWordCacheScopeForConfig(this.deps.getConfig()); log.info( 'Known-word cache lifecycle enabled', `scope=${scope}`, @@ -101,12 +152,12 @@ export class KnownWordCacheManager { return; } - const currentScope = this.getKnownWordCacheScope(); - if (this.knownWordsScope && this.knownWordsScope !== currentScope) { + const currentStateKey = this.getKnownWordCacheStateKey(); + if (this.knownWordsStateKey && this.knownWordsStateKey !== currentStateKey) { this.clearKnownWordCacheState(); } - if (!this.knownWordsScope) { - this.knownWordsScope = currentScope; + if (!this.knownWordsStateKey) { + this.knownWordsStateKey = currentStateKey; } let addedCount = 0; @@ -127,7 +178,7 @@ export class KnownWordCacheManager { log.info( 'Known-word cache updated in-session', `added=${addedCount}`, - `scope=${currentScope}`, + `scope=${getKnownWordCacheScopeForConfig(this.deps.getConfig())}`, ); } } @@ -135,7 +186,7 @@ export class KnownWordCacheManager { clearKnownWordCacheState(): void { this.knownWords = new Set(); this.knownWordsLastRefreshedAtMs = 0; - this.knownWordsScope = this.getKnownWordCacheScope(); + this.knownWordsStateKey = this.getKnownWordCacheStateKey(); try { if (fs.existsSync(this.statePath)) { fs.unlinkSync(this.statePath); @@ -188,7 +239,7 @@ export class KnownWordCacheManager { this.knownWords = nextKnownWords; this.knownWordsLastRefreshedAtMs = Date.now(); - this.knownWordsScope = this.getKnownWordCacheScope(); + this.knownWordsStateKey = this.getKnownWordCacheStateKey(); this.persistKnownWordCacheState(); log.info( 'Known-word cache refreshed', @@ -208,12 +259,7 @@ export class KnownWordCacheManager { } private getKnownWordRefreshIntervalMs(): number { - const minutes = this.deps.getConfig().knownWords?.refreshMinutes; - const safeMinutes = - typeof minutes === 'number' && Number.isFinite(minutes) && minutes > 0 - ? minutes - : DEFAULT_ANKI_CONNECT_CONFIG.knownWords.refreshMinutes; - return safeMinutes * 60_000; + return getKnownWordCacheRefreshIntervalMinutes(this.deps.getConfig()) * 60_000; } private getKnownWordDecks(): string[] { @@ -259,19 +305,15 @@ export class KnownWordCacheManager { return `(${deckQueries.join(' OR ')})`; } - private getKnownWordCacheScope(): string { - const decks = this.getKnownWordDecks(); - if (decks.length === 0) { - return 'is:note'; - } - return `decks:${JSON.stringify(decks)}`; + private getKnownWordCacheStateKey(): string { + return getKnownWordCacheLifecycleConfig(this.deps.getConfig()); } private isKnownWordCacheStale(): boolean { if (!this.isKnownWordCacheEnabled()) { return true; } - if (this.knownWordsScope !== this.getKnownWordCacheScope()) { + if (this.knownWordsStateKey !== this.getKnownWordCacheStateKey()) { return true; } if (this.knownWordsLastRefreshedAtMs <= 0) { @@ -285,7 +327,7 @@ export class KnownWordCacheManager { if (!fs.existsSync(this.statePath)) { this.knownWords = new Set(); this.knownWordsLastRefreshedAtMs = 0; - this.knownWordsScope = this.getKnownWordCacheScope(); + this.knownWordsStateKey = this.getKnownWordCacheStateKey(); return; } @@ -293,7 +335,7 @@ export class KnownWordCacheManager { if (!raw.trim()) { this.knownWords = new Set(); this.knownWordsLastRefreshedAtMs = 0; - this.knownWordsScope = this.getKnownWordCacheScope(); + this.knownWordsStateKey = this.getKnownWordCacheStateKey(); return; } @@ -301,14 +343,14 @@ export class KnownWordCacheManager { if (!this.isKnownWordCacheStateValid(parsed)) { this.knownWords = new Set(); this.knownWordsLastRefreshedAtMs = 0; - this.knownWordsScope = this.getKnownWordCacheScope(); + this.knownWordsStateKey = this.getKnownWordCacheStateKey(); return; } - if (parsed.scope !== this.getKnownWordCacheScope()) { + if (parsed.scope !== this.getKnownWordCacheStateKey()) { this.knownWords = new Set(); this.knownWordsLastRefreshedAtMs = 0; - this.knownWordsScope = this.getKnownWordCacheScope(); + this.knownWordsStateKey = this.getKnownWordCacheStateKey(); return; } @@ -322,12 +364,12 @@ export class KnownWordCacheManager { this.knownWords = nextKnownWords; this.knownWordsLastRefreshedAtMs = parsed.refreshedAtMs; - this.knownWordsScope = parsed.scope; + this.knownWordsStateKey = parsed.scope; } catch (error) { log.warn('Failed to load known-word cache state:', (error as Error).message); this.knownWords = new Set(); this.knownWordsLastRefreshedAtMs = 0; - this.knownWordsScope = this.getKnownWordCacheScope(); + this.knownWordsStateKey = this.getKnownWordCacheStateKey(); } } @@ -336,7 +378,7 @@ export class KnownWordCacheManager { const state: KnownWordCacheState = { version: 1, refreshedAtMs: this.knownWordsLastRefreshedAtMs, - scope: this.knownWordsScope, + scope: this.knownWordsStateKey, words: Array.from(this.knownWords), }; fs.writeFileSync(this.statePath, JSON.stringify(state), 'utf-8'); diff --git a/src/anki-integration/runtime.test.ts b/src/anki-integration/runtime.test.ts index ec2a271..017686f 100644 --- a/src/anki-integration/runtime.test.ts +++ b/src/anki-integration/runtime.test.ts @@ -151,3 +151,36 @@ test('AnkiIntegrationRuntime restarts known-word lifecycle when known-word setti assert.deepEqual(calls, ['known:start']); }); + +test('AnkiIntegrationRuntime does not stop lifecycle when disabled while runtime is stopped', () => { + const { runtime, calls } = createRuntime({ + knownWords: { + highlightEnabled: true, + }, + }); + + runtime.applyRuntimeConfigPatch({ + knownWords: { + highlightEnabled: false, + }, + }); + + assert.deepEqual(calls, ['known:clear']); +}); + +test('AnkiIntegrationRuntime does not restart known-word lifecycle for config changes while stopped', () => { + const { runtime, calls } = createRuntime({ + knownWords: { + highlightEnabled: true, + refreshMinutes: 90, + }, + }); + + runtime.applyRuntimeConfigPatch({ + knownWords: { + refreshMinutes: 120, + }, + }); + + assert.deepEqual(calls, []); +}); diff --git a/src/anki-integration/runtime.ts b/src/anki-integration/runtime.ts index 9be7f50..2661d02 100644 --- a/src/anki-integration/runtime.ts +++ b/src/anki-integration/runtime.ts @@ -1,5 +1,10 @@ import { DEFAULT_ANKI_CONNECT_CONFIG } from '../config'; import type { AnkiConnectConfig } from '../types'; +import { + getKnownWordCacheLifecycleConfig, + getKnownWordCacheRefreshIntervalMinutes, + getKnownWordCacheScopeForConfig, +} from './known-word-cache'; export interface AnkiIntegrationRuntimeProxyServer { start(options: { host: string; port: number; upstreamUrl: string }): void; @@ -196,12 +201,15 @@ export class AnkiIntegrationRuntime { this.deps.onConfigChanged?.(this.config); const nextKnownWordCacheEnabled = this.config.knownWords?.highlightEnabled === true; - if (wasKnownWordCacheEnabled && this.config.knownWords?.highlightEnabled === false) { - this.deps.knownWordCache.stopLifecycle(); + if (wasKnownWordCacheEnabled && !nextKnownWordCacheEnabled) { + if (this.started) { + this.deps.knownWordCache.stopLifecycle(); + } this.deps.knownWordCache.clearKnownWordCacheState(); - } else if (!wasKnownWordCacheEnabled && nextKnownWordCacheEnabled) { + } else if (this.started && !wasKnownWordCacheEnabled && nextKnownWordCacheEnabled) { this.deps.knownWordCache.startLifecycle(); } else if ( + this.started && wasKnownWordCacheEnabled && nextKnownWordCacheEnabled && previousKnownWordCacheConfig !== null && @@ -218,45 +226,15 @@ export class AnkiIntegrationRuntime { } private getKnownWordCacheLifecycleConfig(config: AnkiConnectConfig): string { - return JSON.stringify({ - refreshMinutes: this.getKnownWordRefreshIntervalMinutes(config), - scope: this.getKnownWordCacheScopeForConfig(config), - fieldsWord: trimToNonEmptyString(config.fields?.word) ?? '', - }); + return getKnownWordCacheLifecycleConfig(config); } private getKnownWordRefreshIntervalMinutes(config: AnkiConnectConfig): number { - const refreshMinutes = config.knownWords?.refreshMinutes; - return typeof refreshMinutes === 'number' && Number.isFinite(refreshMinutes) && refreshMinutes > 0 - ? refreshMinutes - : DEFAULT_ANKI_CONNECT_CONFIG.knownWords.refreshMinutes; + return getKnownWordCacheRefreshIntervalMinutes(config); } private getKnownWordCacheScopeForConfig(config: AnkiConnectConfig): string { - const configuredDecks = config.knownWords?.decks; - if (configuredDecks && typeof configuredDecks === 'object' && !Array.isArray(configuredDecks)) { - const normalizedDecks = Object.entries(configuredDecks) - .map(([deckName, fields]) => { - const name = trimToNonEmptyString(deckName); - if (!name) return null; - const normalizedFields = Array.isArray(fields) - ? [ - ...new Set( - fields.map(String).map(trimToNonEmptyString).filter((field): field is string => Boolean(field)), - ), - ].sort() - : []; - return [name, normalizedFields]; - }) - .filter((entry): entry is [string, string[]] => entry !== null) - .sort(([a], [b]) => a.localeCompare(b)); - if (normalizedDecks.length > 0) { - return `decks:${JSON.stringify(normalizedDecks)}`; - } - } - - const configuredDeck = trimToNonEmptyString(config.deck); - return configuredDeck ? `deck:${configuredDeck}` : 'is:note'; + return getKnownWordCacheScopeForConfig(config); } getOrCreateProxyServer(): AnkiIntegrationRuntimeProxyServer {