mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-05-04 00:41:33 -07:00
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
This commit is contained in:
@@ -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
|
||||||
|
|
||||||
|
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||||
|
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.
|
||||||
|
<!-- SECTION:DESCRIPTION:END -->
|
||||||
|
|
||||||
|
## Acceptance Criteria
|
||||||
|
<!-- AC:BEGIN -->
|
||||||
|
- [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.
|
||||||
|
<!-- AC:END -->
|
||||||
|
|
||||||
|
## Implementation Plan
|
||||||
|
|
||||||
|
<!-- SECTION:PLAN:BEGIN -->
|
||||||
|
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.
|
||||||
|
<!-- SECTION:PLAN:END -->
|
||||||
|
|
||||||
|
## Implementation Notes
|
||||||
|
|
||||||
|
<!-- SECTION:NOTES:BEGIN -->
|
||||||
|
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.
|
||||||
|
<!-- SECTION:NOTES:END -->
|
||||||
|
|
||||||
|
## Final Summary
|
||||||
|
|
||||||
|
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
|
||||||
|
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.
|
||||||
|
<!-- SECTION:FINAL_SUMMARY:END -->
|
||||||
@@ -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.
|
||||||
@@ -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', () => {
|
test('KnownWordCacheManager skips immediate append when addMinedWordsImmediately is disabled', () => {
|
||||||
const config: AnkiConnectConfig = {
|
const config: AnkiConnectConfig = {
|
||||||
knownWords: {
|
knownWords: {
|
||||||
|
|||||||
@@ -170,8 +170,10 @@ export class KnownWordCacheManager {
|
|||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
let didMutateCache = false;
|
||||||
const currentStateKey = this.getKnownWordCacheStateKey();
|
const currentStateKey = this.getKnownWordCacheStateKey();
|
||||||
if (this.knownWordsStateKey && this.knownWordsStateKey !== currentStateKey) {
|
if (this.knownWordsStateKey && this.knownWordsStateKey !== currentStateKey) {
|
||||||
|
didMutateCache = this.knownWords.size > 0 || this.noteWordsById.size > 0;
|
||||||
this.clearKnownWordCacheState();
|
this.clearKnownWordCacheState();
|
||||||
}
|
}
|
||||||
if (!this.knownWordsStateKey) {
|
if (!this.knownWordsStateKey) {
|
||||||
@@ -180,13 +182,13 @@ export class KnownWordCacheManager {
|
|||||||
|
|
||||||
const preferredFields = this.getImmediateAppendFields();
|
const preferredFields = this.getImmediateAppendFields();
|
||||||
if (!preferredFields) {
|
if (!preferredFields) {
|
||||||
return false;
|
return didMutateCache;
|
||||||
}
|
}
|
||||||
|
|
||||||
const nextWords = this.extractNormalizedKnownWordsFromNoteInfo(noteInfo, preferredFields);
|
const nextWords = this.extractNormalizedKnownWordsFromNoteInfo(noteInfo, preferredFields);
|
||||||
const changed = this.replaceNoteSnapshot(noteInfo.noteId, nextWords);
|
const changed = this.replaceNoteSnapshot(noteInfo.noteId, nextWords);
|
||||||
if (!changed) {
|
if (!changed) {
|
||||||
return false;
|
return didMutateCache;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (this.knownWordsLastRefreshedAtMs <= 0) {
|
if (this.knownWordsLastRefreshedAtMs <= 0) {
|
||||||
|
|||||||
@@ -326,6 +326,38 @@ test('registerIpcHandlers runs AniList update after manual mark watched succeeds
|
|||||||
assert.deepEqual(calls, ['mark', 'anilist']);
|
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 () => {
|
test('registerIpcHandlers skips AniList update when manual mark watched has no active session', async () => {
|
||||||
const { registrar, handlers } = createFakeIpcRegistrar();
|
const { registrar, handlers } = createFakeIpcRegistrar();
|
||||||
const calls: string[] = [];
|
const calls: string[] = [];
|
||||||
|
|||||||
@@ -390,7 +390,14 @@ export function registerIpcHandlers(deps: IpcServiceDeps, ipc: IpcMainRegistrar
|
|||||||
ipc.handle(IPC_CHANNELS.command.markActiveVideoWatched, async () => {
|
ipc.handle(IPC_CHANNELS.command.markActiveVideoWatched, async () => {
|
||||||
const marked = (await deps.immersionTracker?.markActiveVideoWatched()) ?? false;
|
const marked = (await deps.immersionTracker?.markActiveVideoWatched()) ?? false;
|
||||||
if (marked) {
|
if (marked) {
|
||||||
|
try {
|
||||||
await deps.runAnilistPostWatchUpdateOnManualMark?.();
|
await deps.runAnilistPostWatchUpdateOnManualMark?.();
|
||||||
|
} catch (error) {
|
||||||
|
console.warn(
|
||||||
|
'Failed to run AniList post-watch update after manual watched mark:',
|
||||||
|
(error as Error).message,
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
return marked;
|
return marked;
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -121,6 +121,63 @@ test('createMaybeRunAnilistPostWatchUpdateHandler force-runs manual watched upda
|
|||||||
assert.ok(calls.includes('osd:updated ok'));
|
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<number>((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 () => {
|
test('createMaybeRunAnilistPostWatchUpdateHandler skips youtube playback entirely', async () => {
|
||||||
const calls: string[] = [];
|
const calls: string[] = [];
|
||||||
const handler = createMaybeRunAnilistPostWatchUpdateHandler({
|
const handler = createMaybeRunAnilistPostWatchUpdateHandler({
|
||||||
|
|||||||
@@ -144,12 +144,17 @@ export function createMaybeRunAnilistPostWatchUpdateHandler(deps: {
|
|||||||
deps.resetTrackedMedia(mediaKey);
|
deps.resetTrackedMedia(mediaKey);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
let watchedSeconds = 0;
|
||||||
if (!force) {
|
if (!force) {
|
||||||
const watchedSeconds = deps.getWatchedSeconds();
|
watchedSeconds = deps.getWatchedSeconds();
|
||||||
if (!Number.isFinite(watchedSeconds) || watchedSeconds < deps.minWatchSeconds) {
|
if (!Number.isFinite(watchedSeconds) || watchedSeconds < deps.minWatchSeconds) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
deps.setInFlight(true);
|
||||||
|
try {
|
||||||
|
if (!force) {
|
||||||
const duration = await deps.maybeProbeAnilistDuration(mediaKey);
|
const duration = await deps.maybeProbeAnilistDuration(mediaKey);
|
||||||
if (!duration || duration <= 0) {
|
if (!duration || duration <= 0) {
|
||||||
return;
|
return;
|
||||||
@@ -169,8 +174,6 @@ export function createMaybeRunAnilistPostWatchUpdateHandler(deps: {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
deps.setInFlight(true);
|
|
||||||
try {
|
|
||||||
await deps.processNextAnilistRetryUpdate();
|
await deps.processNextAnilistRetryUpdate();
|
||||||
if (deps.hasAttemptedUpdateKey(attemptKey)) {
|
if (deps.hasAttemptedUpdateKey(attemptKey)) {
|
||||||
return;
|
return;
|
||||||
|
|||||||
Reference in New Issue
Block a user