From 47161cd8a56158ae6d7710d97f69b1a9bc8612ed Mon Sep 17 00:00:00 2001 From: sudacode Date: Sun, 3 May 2026 22:15:18 -0700 Subject: [PATCH] fix: address PR #57 CodeRabbit feedback - Acquire AniList post-watch in-flight lock before async gating to prevent duplicate writes - Isolate manual watched mark result from AniList post-watch callback failures - Report known-word cache clears as mutations during immediate append when state existed - Add regression tests for each fix --- ...ddress-PR-57-latest-CodeRabbit-comments.md | 53 +++++++++++++++++ changes/334-coderabbit-followups.md | 4 ++ src/anki-integration/known-word-cache.test.ts | 45 +++++++++++++++ src/anki-integration/known-word-cache.ts | 6 +- src/core/services/ipc.test.ts | 32 +++++++++++ src/core/services/ipc.ts | 9 ++- src/main/runtime/anilist-post-watch.test.ts | 57 +++++++++++++++++++ src/main/runtime/anilist-post-watch.ts | 41 ++++++------- 8 files changed, 225 insertions(+), 22 deletions(-) create mode 100644 backlog/tasks/task-334 - Assess-and-address-PR-57-latest-CodeRabbit-comments.md create mode 100644 changes/334-coderabbit-followups.md diff --git a/backlog/tasks/task-334 - Assess-and-address-PR-57-latest-CodeRabbit-comments.md b/backlog/tasks/task-334 - Assess-and-address-PR-57-latest-CodeRabbit-comments.md new file mode 100644 index 00000000..e1ef92a3 --- /dev/null +++ b/backlog/tasks/task-334 - Assess-and-address-PR-57-latest-CodeRabbit-comments.md @@ -0,0 +1,53 @@ +--- +id: TASK-334 +title: Assess and address PR 57 latest CodeRabbit comments +status: Done +assignee: + - '@codex' +created_date: '2026-05-04 05:03' +updated_date: '2026-05-04 05:07' +labels: + - pr-feedback + - coderabbit +dependencies: [] +references: + - 'https://github.com/ksyasuda/SubMiner/pull/57' +priority: medium +--- + +## Description + + +Assess the latest CodeRabbit review on PR #57 submitted 2026-05-04 and fix verified issues. Current scope: AniList post-watch duplicate-write race, known-word cache mutation return value, and manual-mark AniList rejection isolation with regression coverage. + + +## Acceptance Criteria + +- [x] #1 Each latest CodeRabbit comment is either fixed or explicitly assessed as not applicable against current code. +- [x] #2 Regression tests cover behavior changes where practical. +- [x] #3 Relevant focused tests and typecheck pass, or any blocked verification is documented. + + +## Implementation Plan + + +1. Verify each latest CodeRabbit finding against current code. +2. Update known-word cache append return semantics so cache clears are reported as mutations when state existed. +3. Acquire AniList post-watch in-flight before async gating and release in finally. +4. Isolate manual-mark AniList callback failures in IPC and add a rejection-path regression test. +5. Run focused tests for touched areas plus typecheck; document any blocked verification. + + +## Implementation Notes + + +Verified latest CodeRabbit review submitted 2026-05-04 on PR #57. Fixed all three current items: known-word cache mutation return after cache reset, AniList post-watch concurrent in-flight race, and manual watched mark isolation from AniList callback failures. Added regression tests for each path and a changelog fragment. + + +## Final Summary + + +Fixed latest PR #57 CodeRabbit feedback by reporting known-word cache clears as mutations during immediate append, acquiring AniList post-watch in-flight before awaited gates to prevent duplicate writes, and isolating manual watched mark success from AniList post-watch callback failures. Added focused regression coverage in known-word cache, AniList post-watch, and IPC tests, plus a changelog fragment. + +Verification: bun test src/anki-integration/known-word-cache.test.ts; bun test src/main/runtime/anilist-post-watch.test.ts; bun test src/core/services/ipc.test.ts; bun run typecheck; bun run format:check:src; bun run changelog:lint; bun run test:fast; bun run test:env; bun run build; bun run test:smoke:dist. + diff --git a/changes/334-coderabbit-followups.md b/changes/334-coderabbit-followups.md new file mode 100644 index 00000000..05506611 --- /dev/null +++ b/changes/334-coderabbit-followups.md @@ -0,0 +1,4 @@ +type: fixed +area: anilist + +- AniList: Prevented duplicate post-watch writes during concurrent checks, preserved manual watched marks when post-watch sync fails, and kept known-word cache refresh notifications accurate after cache resets. diff --git a/src/anki-integration/known-word-cache.test.ts b/src/anki-integration/known-word-cache.test.ts index 4db0db9c..6ce4ba23 100644 --- a/src/anki-integration/known-word-cache.test.ts +++ b/src/anki-integration/known-word-cache.test.ts @@ -520,6 +520,51 @@ test('KnownWordCacheManager uses the current deck fields for immediate append', } }); +test('KnownWordCacheManager reports immediate append cache clears as mutations', () => { + const config: AnkiConnectConfig = { + fields: { + word: 'Expression', + }, + knownWords: { + highlightEnabled: true, + refreshMinutes: 60, + }, + }; + const { manager, statePath, cleanup } = createKnownWordCacheHarness(config); + + try { + fs.writeFileSync( + statePath, + JSON.stringify({ + version: 2, + refreshedAtMs: Date.now(), + scope: '{"refreshMinutes":60,"scope":"is:note","fieldsWord":"Expression"}', + words: ['猫'], + notes: { + '1': ['猫'], + }, + }), + 'utf-8', + ); + manager.startLifecycle(); + assert.equal(manager.isKnownWord('猫'), true); + + config.fields = { word: 'Word' }; + const changed = manager.appendFromNoteInfo({ + noteId: 2, + fields: { + Word: { value: '' }, + }, + }); + + assert.equal(changed, true); + assert.equal(manager.isKnownWord('猫'), false); + } finally { + manager.stopLifecycle(); + cleanup(); + } +}); + test('KnownWordCacheManager skips immediate append when addMinedWordsImmediately is disabled', () => { const config: AnkiConnectConfig = { knownWords: { diff --git a/src/anki-integration/known-word-cache.ts b/src/anki-integration/known-word-cache.ts index e2152012..5bc72fd6 100644 --- a/src/anki-integration/known-word-cache.ts +++ b/src/anki-integration/known-word-cache.ts @@ -170,8 +170,10 @@ export class KnownWordCacheManager { return false; } + let didMutateCache = false; const currentStateKey = this.getKnownWordCacheStateKey(); if (this.knownWordsStateKey && this.knownWordsStateKey !== currentStateKey) { + didMutateCache = this.knownWords.size > 0 || this.noteWordsById.size > 0; this.clearKnownWordCacheState(); } if (!this.knownWordsStateKey) { @@ -180,13 +182,13 @@ export class KnownWordCacheManager { const preferredFields = this.getImmediateAppendFields(); if (!preferredFields) { - return false; + return didMutateCache; } const nextWords = this.extractNormalizedKnownWordsFromNoteInfo(noteInfo, preferredFields); const changed = this.replaceNoteSnapshot(noteInfo.noteId, nextWords); if (!changed) { - return false; + return didMutateCache; } if (this.knownWordsLastRefreshedAtMs <= 0) { diff --git a/src/core/services/ipc.test.ts b/src/core/services/ipc.test.ts index f598b8c7..46e2ec80 100644 --- a/src/core/services/ipc.test.ts +++ b/src/core/services/ipc.test.ts @@ -326,6 +326,38 @@ test('registerIpcHandlers runs AniList update after manual mark watched succeeds assert.deepEqual(calls, ['mark', 'anilist']); }); +test('registerIpcHandlers isolates AniList update failures after manual mark watched succeeds', async () => { + const { registrar, handlers } = createFakeIpcRegistrar(); + const calls: string[] = []; + const originalWarn = console.warn; + console.warn = () => undefined; + + try { + registerIpcHandlers( + createRegisterIpcDeps({ + immersionTracker: createFakeImmersionTracker({ + markActiveVideoWatched: async () => { + calls.push('mark'); + return true; + }, + }), + runAnilistPostWatchUpdateOnManualMark: async () => { + calls.push('anilist'); + throw new Error('post-watch failed'); + }, + }), + registrar, + ); + + const result = await handlers.handle.get(IPC_CHANNELS.command.markActiveVideoWatched)?.({}); + + assert.equal(result, true); + assert.deepEqual(calls, ['mark', 'anilist']); + } finally { + console.warn = originalWarn; + } +}); + test('registerIpcHandlers skips AniList update when manual mark watched has no active session', async () => { const { registrar, handlers } = createFakeIpcRegistrar(); const calls: string[] = []; diff --git a/src/core/services/ipc.ts b/src/core/services/ipc.ts index 93db58f8..89c04da1 100644 --- a/src/core/services/ipc.ts +++ b/src/core/services/ipc.ts @@ -390,7 +390,14 @@ export function registerIpcHandlers(deps: IpcServiceDeps, ipc: IpcMainRegistrar ipc.handle(IPC_CHANNELS.command.markActiveVideoWatched, async () => { const marked = (await deps.immersionTracker?.markActiveVideoWatched()) ?? false; if (marked) { - await deps.runAnilistPostWatchUpdateOnManualMark?.(); + try { + await deps.runAnilistPostWatchUpdateOnManualMark?.(); + } catch (error) { + console.warn( + 'Failed to run AniList post-watch update after manual watched mark:', + (error as Error).message, + ); + } } return marked; }); diff --git a/src/main/runtime/anilist-post-watch.test.ts b/src/main/runtime/anilist-post-watch.test.ts index 7ea23798..83923a11 100644 --- a/src/main/runtime/anilist-post-watch.test.ts +++ b/src/main/runtime/anilist-post-watch.test.ts @@ -121,6 +121,63 @@ test('createMaybeRunAnilistPostWatchUpdateHandler force-runs manual watched upda assert.ok(calls.includes('osd:updated ok')); }); +test('createMaybeRunAnilistPostWatchUpdateHandler blocks concurrent runs before async gating', async () => { + const calls: string[] = []; + let inFlight = false; + let resolveDuration!: (duration: number) => void; + const durationPromise = new Promise((resolve) => { + resolveDuration = resolve; + }); + const handler = createMaybeRunAnilistPostWatchUpdateHandler({ + getInFlight: () => inFlight, + setInFlight: (value) => { + inFlight = value; + calls.push(`inflight:${value}`); + }, + getResolvedConfig: () => ({}), + isAnilistTrackingEnabled: () => true, + getCurrentMediaKey: () => '/tmp/video.mkv', + hasMpvClient: () => true, + getTrackedMediaKey: () => '/tmp/video.mkv', + resetTrackedMedia: () => {}, + getWatchedSeconds: () => 1000, + maybeProbeAnilistDuration: async () => { + calls.push('probe'); + return await durationPromise; + }, + ensureAnilistMediaGuess: async () => ({ title: 'Show', season: null, episode: 1 }), + hasAttemptedUpdateKey: () => false, + processNextAnilistRetryUpdate: async () => ({ ok: true, message: 'noop' }), + refreshAnilistClientSecretState: async () => 'token', + enqueueRetry: () => calls.push('enqueue'), + markRetryFailure: () => calls.push('mark-failure'), + markRetrySuccess: () => calls.push('mark-success'), + refreshRetryQueueState: () => calls.push('refresh'), + updateAnilistPostWatchProgress: async () => { + calls.push('update'); + return { status: 'updated', message: 'updated ok' }; + }, + rememberAttemptedUpdateKey: () => calls.push('remember'), + showMpvOsd: (message) => calls.push(`osd:${message}`), + logInfo: (message) => calls.push(`info:${message}`), + logWarn: (message) => calls.push(`warn:${message}`), + minWatchSeconds: 600, + minWatchRatio: 0.85, + }); + + const firstRun = handler(); + assert.deepEqual(calls, ['inflight:true', 'probe']); + + await handler(); + assert.deepEqual(calls, ['inflight:true', 'probe']); + + resolveDuration(1000); + await firstRun; + + assert.equal(calls.filter((call) => call === 'update').length, 1); + assert.equal(calls.at(-1), 'inflight:false'); +}); + test('createMaybeRunAnilistPostWatchUpdateHandler skips youtube playback entirely', async () => { const calls: string[] = []; const handler = createMaybeRunAnilistPostWatchUpdateHandler({ diff --git a/src/main/runtime/anilist-post-watch.ts b/src/main/runtime/anilist-post-watch.ts index bc2fcd8d..47a9a3d4 100644 --- a/src/main/runtime/anilist-post-watch.ts +++ b/src/main/runtime/anilist-post-watch.ts @@ -144,33 +144,36 @@ export function createMaybeRunAnilistPostWatchUpdateHandler(deps: { deps.resetTrackedMedia(mediaKey); } + let watchedSeconds = 0; if (!force) { - const watchedSeconds = deps.getWatchedSeconds(); + watchedSeconds = deps.getWatchedSeconds(); if (!Number.isFinite(watchedSeconds) || watchedSeconds < deps.minWatchSeconds) { return; } - - const duration = await deps.maybeProbeAnilistDuration(mediaKey); - if (!duration || duration <= 0) { - return; - } - if (watchedSeconds / duration < deps.minWatchRatio) { - return; - } - } - - const guess = await deps.ensureAnilistMediaGuess(mediaKey); - if (!guess?.title || !guess.episode || guess.episode <= 0) { - return; - } - - const attemptKey = buildAnilistAttemptKey(mediaKey, guess.episode); - if (deps.hasAttemptedUpdateKey(attemptKey)) { - return; } deps.setInFlight(true); try { + if (!force) { + const duration = await deps.maybeProbeAnilistDuration(mediaKey); + if (!duration || duration <= 0) { + return; + } + if (watchedSeconds / duration < deps.minWatchRatio) { + return; + } + } + + const guess = await deps.ensureAnilistMediaGuess(mediaKey); + if (!guess?.title || !guess.episode || guess.episode <= 0) { + return; + } + + const attemptKey = buildAnilistAttemptKey(mediaKey, guess.episode); + if (deps.hasAttemptedUpdateKey(attemptKey)) { + return; + } + await deps.processNextAnilistRetryUpdate(); if (deps.hasAttemptedUpdateKey(attemptKey)) { return;