From 040741cf57ab2ef3c665af23a09a9cd0c2572595 Mon Sep 17 00:00:00 2001 From: sudacode Date: Sun, 3 May 2026 21:07:02 -0700 Subject: [PATCH] fix: address coderabbit feedback --- ...unresolved-CodeRabbit-comments-on-PR-57.md | 37 ++++++++++++++++ plugin/subminer/lifecycle.lua | 14 ++++-- plugin/subminer/process.lua | 4 +- scripts/test-plugin-start-gate.lua | 43 +++++++++++++++++++ .../card-creation-manual-update.test.ts | 34 +++++++++++++++ src/anki-integration/card-creation.ts | 9 +++- .../services/anilist/anilist-updater.test.ts | 12 ++++++ src/core/services/anilist/anilist-updater.ts | 5 ++- .../tokenizer/annotation-stage.test.ts | 13 ++++++ .../tokenizer/subtitle-annotation-filter.ts | 1 - .../runtime/mpv-main-event-actions.test.ts | 35 +++++++++++++++ src/main/runtime/mpv-main-event-main-deps.ts | 20 ++++----- src/renderer/subtitle-render.test.ts | 1 + 13 files changed, 207 insertions(+), 21 deletions(-) create mode 100644 backlog/tasks/task-331 - Address-unresolved-CodeRabbit-comments-on-PR-57.md diff --git a/backlog/tasks/task-331 - Address-unresolved-CodeRabbit-comments-on-PR-57.md b/backlog/tasks/task-331 - Address-unresolved-CodeRabbit-comments-on-PR-57.md new file mode 100644 index 00000000..1aacb8e7 --- /dev/null +++ b/backlog/tasks/task-331 - Address-unresolved-CodeRabbit-comments-on-PR-57.md @@ -0,0 +1,37 @@ +--- +id: TASK-331 +title: Address unresolved CodeRabbit comments on PR 57 +status: Done +assignee: + - codex +created_date: '2026-05-04 03:21' +updated_date: '2026-05-04 03:27' +labels: + - pr-feedback + - coderabbit +dependencies: [] +references: + - 'https://github.com/ksyasuda/SubMiner/pull/57' +priority: medium +--- + +## Description + + +Assess and fix unresolved CodeRabbit review comments on PR #57 after rebasing tokenizer-updates. Scope includes manual clipboard SentenceAudio guard, tokenizer standalone particle blacklist, AniList guessit fallback confidence, startup gate duplicate auto-start, and small regression-test hardening where applicable. + + +## Acceptance Criteria + +- [x] #1 Each unresolved 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. + + +## Implementation Notes + + +Fixed all verified actionable CodeRabbit comments from PR #57: manual clipboard updates no longer fall back to ExpressionAudio when SentenceAudio is absent, connective particle phrases no longer suppress lexical verb readings like 立って, guessit output only borrows parser season/episode from non-low-confidence parses, duplicate auto-start no longer releases an active pause-until-ready gate, JLPT CSS tests block text-decoration shorthand underlines, post-watch update rejection logging is covered, and duplicate quit-on-disconnect predicate code is shared. + +Verification: bun test src/anki-integration/card-creation-manual-update.test.ts src/core/services/tokenizer/annotation-stage.test.ts src/core/services/anilist/anilist-updater.test.ts src/main/runtime/mpv-main-event-actions.test.ts src/renderer/subtitle-render.test.ts; lua scripts/test-plugin-start-gate.lua; bun run typecheck; bun run test:fast. + diff --git a/plugin/subminer/lifecycle.lua b/plugin/subminer/lifecycle.lua index 2a8899a5..fcfedb93 100644 --- a/plugin/subminer/lifecycle.lua +++ b/plugin/subminer/lifecycle.lua @@ -83,11 +83,17 @@ function M.create(ctx) return end - aniskip.clear_aniskip_state() - process.disarm_auto_play_ready_gate() - local has_matching_socket = rearm_managed_subtitle_defaults() - local should_auto_start = resolve_auto_start_enabled() + local has_matching_socket = process.has_matching_mpv_ipc_socket(opts.socket_path) + local preserve_active_auto_start_gate = ( + state.overlay_running and state.auto_play_ready_gate_armed and should_auto_start and has_matching_socket + ) + aniskip.clear_aniskip_state() + if not preserve_active_auto_start_gate then + process.disarm_auto_play_ready_gate() + end + has_matching_socket = rearm_managed_subtitle_defaults() + if should_auto_start then if not has_matching_socket then subminer_log( diff --git a/plugin/subminer/process.lua b/plugin/subminer/process.lua index b3ba1bac..ab73799f 100644 --- a/plugin/subminer/process.lua +++ b/plugin/subminer/process.lua @@ -299,7 +299,9 @@ function M.create(ctx) if overrides.auto_start_trigger == true then subminer_log("debug", "process", "Auto-start ignored because overlay is already running") local socket_path = overrides.socket_path or opts.socket_path - disarm_auto_play_ready_gate() + if not state.auto_play_ready_gate_armed then + disarm_auto_play_ready_gate() + end local visibility_action = resolve_visible_overlay_startup() and "show-visible-overlay" or "hide-visible-overlay" diff --git a/scripts/test-plugin-start-gate.lua b/scripts/test-plugin-start-gate.lua index 70b26bdc..0656e777 100644 --- a/scripts/test-plugin-start-gate.lua +++ b/scripts/test-plugin-start-gate.lua @@ -559,6 +559,49 @@ do ) end +do + local recorded, err = run_plugin_scenario({ + process_list = "", + option_overrides = { + binary_path = binary_path, + auto_start = "yes", + auto_start_visible_overlay = "yes", + auto_start_pause_until_ready = "yes", + socket_path = "/tmp/subminer-socket", + }, + input_ipc_server = "/tmp/subminer-socket", + media_title = "Random Movie", + files = { + [binary_path] = true, + }, + }) + assert_true(recorded ~= nil, "plugin failed to load for pre-ready duplicate auto-start scenario: " .. tostring(err)) + fire_event(recorded, "file-loaded") + fire_event(recorded, "file-loaded") + assert_true(recorded.script_messages["subminer-autoplay-ready"] ~= nil, "subminer-autoplay-ready script message not registered") + assert_true( + count_start_calls(recorded.async_calls) == 1, + "pre-ready duplicate auto-start should not issue duplicate --start commands" + ) + assert_true( + count_property_set(recorded.property_sets, "pause", true) == 1, + "pre-ready duplicate auto-start should not repeat the pause gate" + ) + assert_true( + count_property_set(recorded.property_sets, "pause", false) == 0, + "pre-ready duplicate auto-start should not resume playback before tokenization is ready" + ) + assert_true( + count_osd_message(recorded.osd, "SubMiner: Loading subtitle tokenization...") == 1, + "pre-ready duplicate auto-start should not repeat the loading OSD" + ) + recorded.script_messages["subminer-autoplay-ready"]() + assert_true( + count_property_set(recorded.property_sets, "pause", false) == 1, + "autoplay-ready should resume the original pre-ready gate" + ) +end + do local recorded, err = run_plugin_scenario({ process_list = "", diff --git a/src/anki-integration/card-creation-manual-update.test.ts b/src/anki-integration/card-creation-manual-update.test.ts index 8ac6efe2..e33de200 100644 --- a/src/anki-integration/card-creation-manual-update.test.ts +++ b/src/anki-integration/card-creation-manual-update.test.ts @@ -141,3 +141,37 @@ test('manual clipboard subtitle update replaces sentence audio without touching [true], ); }); + +test('manual clipboard subtitle update skips audio when sentence audio field is missing', async () => { + const { service, updatedFields, mergeCalls, storedMedia } = createManualUpdateService({ + client: { + addNote: async () => 0, + addTags: async () => undefined, + notesInfo: async () => [ + { + noteId: 42, + fields: { + Expression: { value: '単語' }, + Sentence: { value: '' }, + ExpressionAudio: { value: '[sound:auto-expression.mp3]' }, + }, + }, + ], + updateNoteFields: async (_noteId, fields) => { + updatedFields.push(fields); + }, + storeMediaFile: async (filename) => { + storedMedia.push(filename); + }, + findNotes: async () => [42], + retrieveMediaFile: async () => '', + }, + }); + + await service.updateLastAddedFromClipboard('字幕'); + + assert.equal(storedMedia.length, 1); + assert.equal(updatedFields.length, 1); + assert.deepEqual(updatedFields[0], { Sentence: '字幕' }); + assert.equal(mergeCalls.length, 0); +}); diff --git a/src/anki-integration/card-creation.ts b/src/anki-integration/card-creation.ts index 43bb40a1..a5349fd8 100644 --- a/src/anki-integration/card-creation.ts +++ b/src/anki-integration/card-creation.ts @@ -218,7 +218,7 @@ export class CardCreationService { fields, this.deps.getConfig(), ); - const sentenceAudioField = this.getResolvedSentenceAudioFieldName(noteInfo); + const sentenceAudioField = this.getResolvedSentenceOnlyAudioFieldName(noteInfo); const sentenceField = this.deps.getEffectiveSentenceCardConfig().sentenceField; const sentence = blocks.join(' '); @@ -721,6 +721,13 @@ export class CardCreationService { ); } + private getResolvedSentenceOnlyAudioFieldName(noteInfo: CardCreationNoteInfo): string | null { + return this.deps.resolveNoteFieldName( + noteInfo, + this.deps.getEffectiveSentenceCardConfig().audioField || 'SentenceAudio', + ); + } + private createPendingNoteInfo(fields: Record): CardCreationNoteInfo { return { noteId: -1, diff --git a/src/core/services/anilist/anilist-updater.test.ts b/src/core/services/anilist/anilist-updater.test.ts index c3b6d480..db4109bf 100644 --- a/src/core/services/anilist/anilist-updater.test.ts +++ b/src/core/services/anilist/anilist-updater.test.ts @@ -34,6 +34,18 @@ test('guessAnilistMediaInfo fills missing guessit episode from filename parser', }); }); +test('guessAnilistMediaInfo ignores low-confidence parser details when guessit omits them', async () => { + const result = await guessAnilistMediaInfo('/tmp/Season 2/Guessit Title.mkv', null, { + runGuessit: async () => JSON.stringify({ title: 'Guessit Title' }), + }); + assert.deepEqual(result, { + title: 'Guessit Title', + season: null, + episode: null, + source: 'guessit', + }); +}); + test('guessAnilistMediaInfo parses Little Witch Academia release filename', async () => { const filename = '/tmp/Little Witch Academia (2017) - S01E02 - 002 - Papiliodia [Bluray-1080p][10bit][h265][AC3 2.0][JA].mkv'; diff --git a/src/core/services/anilist/anilist-updater.ts b/src/core/services/anilist/anilist-updater.ts index 896bc584..462e9379 100644 --- a/src/core/services/anilist/anilist-updater.ts +++ b/src/core/services/anilist/anilist-updater.ts @@ -237,12 +237,13 @@ export async function guessAnilistMediaInfo( const year = firstYear(parsed.year); if (title) { const fallback = parseMediaInfo(target); + const canUseFallbackDetails = fallback.confidence !== 'low'; return { title: buildGuessitTitle(title, alternativeTitle), ...(alternativeTitle ? { alternativeTitle } : {}), ...(year ? { year } : {}), - season: season ?? fallback.season, - episode: episode ?? fallback.episode, + season: season ?? (canUseFallbackDetails ? fallback.season : null), + episode: episode ?? (canUseFallbackDetails ? fallback.episode : null), source: 'guessit', }; } diff --git a/src/core/services/tokenizer/annotation-stage.test.ts b/src/core/services/tokenizer/annotation-stage.test.ts index 77b0f147..68d8c996 100644 --- a/src/core/services/tokenizer/annotation-stage.test.ts +++ b/src/core/services/tokenizer/annotation-stage.test.ts @@ -567,6 +567,19 @@ test('shouldExcludeTokenFromSubtitleAnnotations excludes standalone connective p assert.equal(shouldExcludeTokenFromSubtitleAnnotations(token), true); }); +test('shouldExcludeTokenFromSubtitleAnnotations keeps lexical verbs whose reading matches connective particles', () => { + const token = makeToken({ + surface: '立って', + headword: '立つ', + reading: 'タッテ', + partOfSpeech: PartOfSpeech.verb, + pos1: '動詞', + pos2: '自立', + }); + + assert.equal(shouldExcludeTokenFromSubtitleAnnotations(token), false); +}); + test('shouldExcludeTokenFromSubtitleAnnotations excludes rhetorical もんか grammar particle phrases', () => { for (const surface of ['もんか', 'ものか']) { const token = makeToken({ diff --git a/src/core/services/tokenizer/subtitle-annotation-filter.ts b/src/core/services/tokenizer/subtitle-annotation-filter.ts index 508dc98e..72cdd64b 100644 --- a/src/core/services/tokenizer/subtitle-annotation-filter.ts +++ b/src/core/services/tokenizer/subtitle-annotation-filter.ts @@ -57,7 +57,6 @@ export const SUBTITLE_ANNOTATION_EXCLUDED_TERMS = new Set([ '貴方', 'もんか', 'ものか', - ...STANDALONE_GRAMMAR_PARTICLE_PHRASES, ]); const SUBTITLE_ANNOTATION_EXCLUDED_TRAILING_PARTICLE_SUFFIXES = new Set([ 'って', diff --git a/src/main/runtime/mpv-main-event-actions.test.ts b/src/main/runtime/mpv-main-event-actions.test.ts index 4665fe7c..dc862687 100644 --- a/src/main/runtime/mpv-main-event-actions.test.ts +++ b/src/main/runtime/mpv-main-event-actions.test.ts @@ -223,6 +223,41 @@ test('time-pos and pause handlers report progress with correct urgency', () => { ]); }); +test('time-pos handler logs post-watch update rejection without blocking later handlers', async () => { + const calls: string[] = []; + const timeHandler = createHandleMpvTimePosChangeHandler({ + recordPlaybackPosition: (time) => calls.push(`time:${time}`), + reportJellyfinRemoteProgress: (force) => calls.push(`progress:${force ? 'force' : 'normal'}`), + refreshDiscordPresence: () => calls.push('presence'), + maybeRunAnilistPostWatchUpdate: async () => { + calls.push('post-watch'); + throw new Error('boom'); + }, + logError: (message, error) => calls.push(`error:${message}:${(error as Error).message}`), + }); + const pauseHandler = createHandleMpvPauseChangeHandler({ + recordPauseState: (paused) => calls.push(`pause:${paused ? 'yes' : 'no'}`), + reportJellyfinRemoteProgress: (force) => calls.push(`progress:${force ? 'force' : 'normal'}`), + refreshDiscordPresence: () => calls.push('presence'), + }); + + timeHandler({ time: 12.5 }); + pauseHandler({ paused: true }); + await Promise.resolve(); + await Promise.resolve(); + + assert.deepEqual(calls, [ + 'time:12.5', + 'progress:normal', + 'presence', + 'post-watch', + 'pause:yes', + 'progress:force', + 'presence', + 'error:AniList post-watch update failed unexpectedly:boom', + ]); +}); + test('subtitle metrics change handler forwards patch payload', () => { let received: Record | null = null; const handler = createHandleMpvSubtitleMetricsChangeHandler({ diff --git a/src/main/runtime/mpv-main-event-main-deps.ts b/src/main/runtime/mpv-main-event-main-deps.ts index 44399d58..a703ba13 100644 --- a/src/main/runtime/mpv-main-event-main-deps.ts +++ b/src/main/runtime/mpv-main-event-main-deps.ts @@ -78,23 +78,19 @@ export function createBuildBindMpvMainEventHandlersMainDepsHandler(deps: { deps.ensureImmersionTrackerInitialized(); deps.appState.immersionTracker?.recordPlaybackPosition?.(normalizedTimeSec); }; + const hasInitialPlaybackQuitOnDisconnectArg = (): boolean => + Boolean( + deps.appState.initialArgs?.managedPlayback || + deps.appState.initialArgs?.jellyfinPlay || + deps.appState.initialArgs?.youtubePlay, + ); return () => ({ reportJellyfinRemoteStopped: () => deps.reportJellyfinRemoteStopped(), syncOverlayMpvSubtitleSuppression: () => deps.syncOverlayMpvSubtitleSuppression(), - hasInitialPlaybackQuitOnDisconnectArg: () => - Boolean( - deps.appState.initialArgs?.managedPlayback || - deps.appState.initialArgs?.jellyfinPlay || - deps.appState.initialArgs?.youtubePlay, - ), + hasInitialPlaybackQuitOnDisconnectArg, isOverlayRuntimeInitialized: () => deps.appState.overlayRuntimeInitialized, - shouldQuitOnDisconnectWhenOverlayRuntimeInitialized: () => - Boolean( - deps.appState.initialArgs?.managedPlayback || - deps.appState.initialArgs?.jellyfinPlay || - deps.appState.initialArgs?.youtubePlay, - ), + shouldQuitOnDisconnectWhenOverlayRuntimeInitialized: hasInitialPlaybackQuitOnDisconnectArg, isQuitOnDisconnectArmed: () => deps.getQuitOnDisconnectArmed(), scheduleQuitCheck: (callback: () => void) => deps.scheduleQuitCheck(callback), isMpvConnected: () => Boolean(deps.appState.mpvClient?.connected), diff --git a/src/renderer/subtitle-render.test.ts b/src/renderer/subtitle-render.test.ts index 3fa91ed2..49619b68 100644 --- a/src/renderer/subtitle-render.test.ts +++ b/src/renderer/subtitle-render.test.ts @@ -909,6 +909,7 @@ test('subtitle annotation CSS underlines JLPT tokens without changing token colo // popup/selection state. assert.doesNotMatch(plainJlptBlock, /(?:^|\n)\s*color\s*:/m); assert.doesNotMatch(plainJlptBlock, /text-decoration-line:\s*underline;/); + assert.doesNotMatch(plainJlptBlock, /text-decoration\s*:[^;]*\bunderline\b/i); assert.match( plainJlptBlock, new RegExp(`border-bottom:\\s*2px\\s+solid\\s+var\\(--subtitle-jlpt-n${level}-color,`),