fix: address CodeRabbit review feedback

This commit is contained in:
2026-03-31 22:44:43 -07:00
parent a5faef5aee
commit 233bde5861
8 changed files with 199 additions and 8 deletions

View File

@@ -487,3 +487,88 @@ test('CardCreationService tracks pre-add duplicate note ids for kiku sentence ca
assert.deepEqual(duplicateLookupExpressions, ['重複文']);
assert.deepEqual(trackedDuplicates, [{ noteId: 42, duplicateNoteIds: [7, 18, 30] }]);
});
test('CardCreationService does not track duplicate ids when pre-add lookup returns none', async () => {
const trackedDuplicates: Array<{ noteId: number; duplicateNoteIds: number[] }> = [];
const service = new CardCreationService({
getConfig: () =>
({
deck: 'Mining',
fields: {
word: 'Expression',
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 () => [],
updateNoteFields: async () => undefined,
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: () => undefined,
mergeFieldValue: (_existing, newValue) => newValue,
formatMiscInfoPattern: () => '',
getEffectiveSentenceCardConfig: () => ({
model: 'Sentence',
sentenceField: 'Sentence',
audioField: 'SentenceAudio',
lapisEnabled: false,
kikuEnabled: true,
kikuFieldGrouping: 'manual',
kikuDeleteDuplicateInAuto: false,
}),
getFallbackDurationSeconds: () => 10,
appendKnownWordsFromNoteInfo: () => undefined,
isUpdateInProgress: () => false,
setUpdateInProgress: () => undefined,
trackLastAddedNoteId: () => undefined,
findDuplicateNoteIds: async () => [],
trackLastAddedDuplicateNoteIds: (noteId, duplicateNoteIds) => {
trackedDuplicates.push({ noteId, duplicateNoteIds });
},
});
const created = await service.createSentenceCard('重複なし', 0, 1);
assert.equal(created, true);
assert.deepEqual(trackedDuplicates, []);
});

View File

@@ -602,10 +602,12 @@ export class CardCreationService {
log.warn('Failed to track last added note:', (error as Error).message);
}
try {
this.deps.trackLastAddedDuplicateNoteIds?.(noteId, duplicateNoteIds);
} catch (error) {
log.warn('Failed to track duplicate note ids:', (error as Error).message);
if (duplicateNoteIds.length > 0) {
try {
this.deps.trackLastAddedDuplicateNoteIds?.(noteId, duplicateNoteIds);
} catch (error) {
log.warn('Failed to track duplicate note ids:', (error as Error).message);
}
}
try {

View File

@@ -267,3 +267,33 @@ test('findDuplicateNote does not disable retries on findNotes calls', async () =
assert.ok(seenOptions.length > 0);
assert.ok(seenOptions.every((options) => options?.maxRetries !== 0));
});
test('findDuplicateNote stops after the first exact-match chunk', async () => {
const currentNote: NoteInfo = {
noteId: 100,
fields: {
Expression: { value: '貴様' },
},
};
let notesInfoCalls = 0;
const candidateIds = Array.from({ length: 51 }, (_, index) => 200 + index);
const duplicateId = await findDuplicateNote('貴様', 100, currentNote, {
findNotes: async () => candidateIds,
notesInfo: async (noteIds) => {
notesInfoCalls += 1;
return noteIds.map((noteId) => ({
noteId,
fields: {
Expression: { value: noteId === 200 ? '貴様' : `別単語-${noteId}` },
},
}));
},
getDeck: () => 'Japanese::Mining',
resolveFieldName: (noteInfo, preferredName) => createFieldResolver(noteInfo, preferredName),
logWarn: () => {},
});
assert.equal(duplicateId, 200);
assert.equal(notesInfoCalls, 1);
});

View File

@@ -24,7 +24,13 @@ export async function findDuplicateNote(
noteInfo: NoteInfo,
deps: DuplicateDetectionDeps,
): Promise<number | null> {
const duplicateNoteIds = await findDuplicateNoteIds(expression, excludeNoteId, noteInfo, deps);
const duplicateNoteIds = await findDuplicateNoteIds(
expression,
excludeNoteId,
noteInfo,
deps,
1,
);
return duplicateNoteIds[0] ?? null;
}
@@ -33,6 +39,7 @@ export async function findDuplicateNoteIds(
excludeNoteId: number,
noteInfo: NoteInfo,
deps: DuplicateDetectionDeps,
maxMatches?: number,
): Promise<number[]> {
const configuredWordFieldCandidates = deps.getWordFieldCandidates?.() ?? ['Expression', 'Word'];
const sourceCandidates = getDuplicateSourceCandidates(
@@ -99,6 +106,7 @@ export async function findDuplicateNoteIds(
sourceCandidates.map((candidate) => candidate.value),
configuredWordFieldCandidates,
deps,
maxMatches,
);
} catch (error) {
deps.logWarn('Duplicate search failed:', error);
@@ -112,6 +120,7 @@ function findExactDuplicateNoteIds(
sourceValues: string[],
candidateFieldNames: string[],
deps: DuplicateDetectionDeps,
maxMatches?: number,
): Promise<number[]> {
const candidates = Array.from(candidateNoteIds).filter((id) => id !== excludeNoteId);
deps.logDebug?.(`[duplicate] candidateIds=${candidates.length} exclude=${excludeNoteId}`);
@@ -145,6 +154,9 @@ function findExactDuplicateNoteIds(
);
deps.logInfo?.(`[duplicate] matched noteId=${noteInfo.noteId} field=${resolvedField}`);
matches.push(noteInfo.noteId);
if (maxMatches !== undefined && matches.length >= maxMatches) {
return matches;
}
break;
}
}

View File

@@ -1390,3 +1390,32 @@ test('addYomitanNoteViaSearch returns note and duplicate ids from the bridge pay
duplicateNoteIds: [18, 7, 18],
});
});
test('addYomitanNoteViaSearch rejects invalid numeric note ids from the bridge shortcut', async () => {
const deps = createDeps(async () => NaN);
const result = await addYomitanNoteViaSearch('食べる', deps, {
error: () => undefined,
});
assert.deepEqual(result, {
noteId: null,
duplicateNoteIds: [],
});
});
test('addYomitanNoteViaSearch sanitizes invalid payload note ids while keeping valid duplicate ids', async () => {
const deps = createDeps(async (_script) => ({
noteId: -1,
duplicateNoteIds: [18, 0, 7.5, 7],
}));
const result = await addYomitanNoteViaSearch('食べる', deps, {
error: () => undefined,
});
assert.deepEqual(result, {
noteId: null,
duplicateNoteIds: [18, 7],
});
});

View File

@@ -2010,7 +2010,10 @@ export async function addYomitanNoteViaSearch(
try {
const result = await parserWindow.webContents.executeJavaScript(script, true);
if (typeof result === 'number') {
return { noteId: result, duplicateNoteIds: [] };
return {
noteId: Number.isInteger(result) && result > 0 ? result : null,
duplicateNoteIds: [],
};
}
if (result && typeof result === 'object' && !Array.isArray(result)) {
const envelope = result as {
@@ -2018,7 +2021,12 @@ export async function addYomitanNoteViaSearch(
duplicateNoteIds?: unknown;
};
return {
noteId: typeof envelope.noteId === 'number' ? envelope.noteId : null,
noteId:
typeof envelope.noteId === 'number' &&
Number.isInteger(envelope.noteId) &&
envelope.noteId > 0
? envelope.noteId
: null,
duplicateNoteIds: Array.isArray(envelope.duplicateNoteIds)
? envelope.duplicateNoteIds.filter(
(entry): entry is number => typeof entry === 'number' && Number.isInteger(entry) && entry > 0,

View File

@@ -549,6 +549,31 @@ test('paused configured subtitle-jump keybinding re-applies pause after backward
}
});
test('configured subtitle-jump keybinding preserves pause when pause state is unknown', async () => {
const { handlers, testGlobals } = createKeyboardHandlerHarness();
try {
await handlers.setupMpvInputForwarding();
handlers.updateKeybindings([
{
key: 'Shift+KeyH',
command: ['sub-seek', -1],
},
] as never);
testGlobals.setPlaybackPausedResponse(null);
testGlobals.dispatchKeydown({ key: 'H', code: 'KeyH', shiftKey: true });
await wait(0);
assert.deepEqual(testGlobals.mpvCommands.slice(-2), [
['sub-seek', -1],
['set_property', 'pause', 'yes'],
]);
} finally {
testGlobals.restore();
}
});
test('visible-layer y-t dispatches mpv plugin toggle while overlay owns focus', async () => {
const { handlers, testGlobals } = createKeyboardHandlerHarness();

View File

@@ -376,7 +376,7 @@ export function createKeyboardHandlers(
.getPlaybackPaused()
.then((paused) => {
window.electronAPI.sendMpvCommand(command);
if (paused === true) {
if (paused !== false) {
window.electronAPI.sendMpvCommand(['set_property', 'pause', 'yes']);
}
})