From d65575c80dc88c2c0a34dffdfdaf2945fcdef915 Mon Sep 17 00:00:00 2001 From: sudacode Date: Sun, 22 Mar 2026 19:37:49 -0700 Subject: [PATCH] fix: address CodeRabbit review feedback --- README.md | 2 +- docs-site/changelog.md | 7 + launcher/aniskip-metadata.ts | 2 +- launcher/commands/jellyfin-command.ts | 8 +- launcher/log.test.ts | 6 +- launcher/mpv.ts | 7 +- launcher/smoke.e2e.test.ts | 1 + plugin/subminer/process.lua | 3 +- src/anki-integration/media-source.ts | 2 + src/cli/args.ts | 1 - src/core/services/cli-command.test.ts | 15 ++ .../services/overlay-runtime-init.test.ts | 87 +++++++++ src/core/services/startup.test.ts | 74 ++++++++ src/core/services/startup.ts | 1 + src/core/services/youtube/kinds.ts | 1 + src/core/services/youtube/labels.ts | 5 +- src/core/services/youtube/retime.test.ts | 4 - src/core/services/youtube/retime.ts | 2 +- src/core/services/youtube/timedtext.ts | 21 ++- .../services/youtube/track-download.test.ts | 45 +++++ src/core/services/youtube/track-download.ts | 50 ++++- src/core/services/youtube/track-probe.test.ts | 17 +- src/core/services/youtube/track-probe.ts | 22 ++- src/main.ts | 7 +- src/main/runtime/youtube-flow.test.ts | 57 ++++++ src/main/runtime/youtube-flow.ts | 39 +++- src/preload-args.test.ts | 9 + src/renderer/handlers/mouse.ts | 1 - .../modals/youtube-track-picker.test.ts | 176 ++++++++++++++++++ src/renderer/modals/youtube-track-picker.ts | 34 +++- src/shared/ipc/validators.ts | 13 +- src/shared/log-files.test.ts | 4 +- src/types.ts | 22 ++- 33 files changed, 678 insertions(+), 67 deletions(-) create mode 100644 src/core/services/youtube/kinds.ts diff --git a/README.md b/README.md index bfa9024..500ebd3 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ # SubMiner -### Turn mpv into a sentence-mining workstation. +## Turn mpv into a sentence-mining workstation. Look up words with Yomitan, export to Anki in one key, track your immersion — all without leaving mpv. diff --git a/docs-site/changelog.md b/docs-site/changelog.md index 48aa88e..4fc64bc 100644 --- a/docs-site/changelog.md +++ b/docs-site/changelog.md @@ -1,5 +1,12 @@ # Changelog +## v0.9.0 (2026-03-22) +- Added an app-owned YouTube subtitle picker flow that boots mpv paused, opens an overlay picker, and downloads selected subtitles into external files before playback resumes. +- Added explicit launcher/app YouTube subtitle modes `download` and `generate`, with `download` as the default path. +- Disabled mpv native YouTube subtitle auto-loading for the app-owned flow so injected external subtitle files stay authoritative. +- Added OSD status updates covering YouTube playback startup, subtitle acquisition, and subtitle loading. +- Improved sidebar startup/resume behavior and overlay/sidebar subtitle synchronization. + ## v0.8.0 (2026-03-22) - Added a configurable subtitle sidebar feature (`subtitleSidebar`) with overlay/embedded rendering, click-to-seek cue list, and hot-reloadable visibility and behavior controls. - Added a rendered sidebar modal with cue list display, click-to-seek, active-cue highlighting, and embedded layout support. diff --git a/launcher/aniskip-metadata.ts b/launcher/aniskip-metadata.ts index 5f8ed2d..24ce56f 100644 --- a/launcher/aniskip-metadata.ts +++ b/launcher/aniskip-metadata.ts @@ -558,7 +558,7 @@ export function buildSubminerScriptOpts( const parts = [ `subminer-binary_path=${sanitizeScriptOptValue(appPath)}`, `subminer-socket_path=${sanitizeScriptOptValue(socketPath)}`, - ...extraParts, + ...extraParts.map(sanitizeScriptOptValue), ]; if (logLevel !== 'info') { parts.push(`subminer-log_level=${sanitizeScriptOptValue(logLevel)}`); diff --git a/launcher/commands/jellyfin-command.ts b/launcher/commands/jellyfin-command.ts index 3f8fb27..f6696bd 100644 --- a/launcher/commands/jellyfin-command.ts +++ b/launcher/commands/jellyfin-command.ts @@ -75,11 +75,5 @@ export async function runJellyfinCommand(context: LauncherCommandContext): Promi return true; } - return Boolean( - args.jellyfin || - args.jellyfinLogin || - args.jellyfinLogout || - args.jellyfinPlay || - args.jellyfinDiscovery, - ); + return false; } diff --git a/launcher/log.test.ts b/launcher/log.test.ts index 91e2776..615934b 100644 --- a/launcher/log.test.ts +++ b/launcher/log.test.ts @@ -4,6 +4,7 @@ import path from 'node:path'; import { getDefaultLauncherLogFile, getDefaultMpvLogFile } from './types.js'; test('getDefaultMpvLogFile uses APPDATA on windows', () => { + const today = new Date().toISOString().slice(0, 10); const resolved = getDefaultMpvLogFile({ platform: 'win32', homeDir: 'C:\\Users\\tester', @@ -17,13 +18,14 @@ test('getDefaultMpvLogFile uses APPDATA on windows', () => { 'C:\\Users\\tester\\AppData\\Roaming', 'SubMiner', 'logs', - `mpv-${new Date().toISOString().slice(0, 10)}.log`, + `mpv-${today}.log`, ), ), ); }); test('getDefaultLauncherLogFile uses launcher prefix', () => { + const today = new Date().toISOString().slice(0, 10); const resolved = getDefaultLauncherLogFile({ platform: 'linux', homeDir: '/home/tester', @@ -36,7 +38,7 @@ test('getDefaultLauncherLogFile uses launcher prefix', () => { '.config', 'SubMiner', 'logs', - `launcher-${new Date().toISOString().slice(0, 10)}.log`, + `launcher-${today}.log`, ), ); }); diff --git a/launcher/mpv.ts b/launcher/mpv.ts index 7177c3c..406759f 100644 --- a/launcher/mpv.ts +++ b/launcher/mpv.ts @@ -789,7 +789,8 @@ function stopManagedOverlayApp(args: Args): void { const stopArgs = ['--stop']; if (args.logLevel !== 'info') stopArgs.push('--log-level', args.logLevel); - const result = spawnSync(state.appPath, stopArgs, { + const target = resolveAppSpawnTarget(state.appPath, stopArgs); + const result = spawnSync(target.command, target.args, { stdio: 'ignore', env: buildAppEnv(), }); @@ -919,7 +920,7 @@ export function runAppCommandWithInherit(appPath: string, appArgs: string[]): vo proc.once('error', (error) => { fail(`Failed to run app command: ${error.message}`); }); - proc.once('exit', (code) => { + proc.once('close', (code) => { process.exit(code ?? 0); }); } @@ -970,7 +971,7 @@ export function runAppCommandAttached( proc.once('error', (error) => { reject(error); }); - proc.once('exit', (code, signal) => { + proc.once('close', (code, signal) => { if (code !== null) { resolve(code); } else if (signal) { diff --git a/launcher/smoke.e2e.test.ts b/launcher/smoke.e2e.test.ts index e95ed55..82798fd 100644 --- a/launcher/smoke.e2e.test.ts +++ b/launcher/smoke.e2e.test.ts @@ -310,6 +310,7 @@ test( const appStartPath = path.join(smokeCase.artifactsDir, 'fake-app-start.log'); const appStopPath = path.join(smokeCase.artifactsDir, 'fake-app-stop.log'); await waitForJsonLines(appStartPath, 1); + await waitForJsonLines(appStopPath, 1); const appStartEntries = readJsonLines(appStartPath); const appStopEntries = readJsonLines(appStopPath); diff --git a/plugin/subminer/process.lua b/plugin/subminer/process.lua index 54b09b1..532f65f 100644 --- a/plugin/subminer/process.lua +++ b/plugin/subminer/process.lua @@ -4,6 +4,7 @@ local OVERLAY_START_RETRY_DELAY_SECONDS = 0.2 local OVERLAY_START_MAX_ATTEMPTS = 6 local AUTO_PLAY_READY_LOADING_OSD = "Loading subtitle tokenization..." local AUTO_PLAY_READY_READY_OSD = "Subtitle tokenization ready" +local DEFAULT_AUTO_PLAY_READY_TIMEOUT_SECONDS = 15 function M.create(ctx) local mp = ctx.mp @@ -47,7 +48,7 @@ function M.create(ctx) return parsed end end - return 15 + return DEFAULT_AUTO_PLAY_READY_TIMEOUT_SECONDS end local function normalize_socket_path(path) diff --git a/src/anki-integration/media-source.ts b/src/anki-integration/media-source.ts index 13f5306..36adaa2 100644 --- a/src/anki-integration/media-source.ts +++ b/src/anki-integration/media-source.ts @@ -48,6 +48,8 @@ function resolvePreferredUrlFromMpvEdlSource( return typedMatch; } + // mpv EDL sources usually list audio streams first and video streams last, so + // when classifyMediaUrl cannot identify a typed URL we fall back to stream order. return kind === 'audio' ? urls[0] ?? null : urls[urls.length - 1] ?? null; } diff --git a/src/cli/args.ts b/src/cli/args.ts index 08b8efc..eba4946 100644 --- a/src/cli/args.ts +++ b/src/cli/args.ts @@ -423,7 +423,6 @@ export function shouldStartApp(args: CliArgs): boolean { args.stats || args.jellyfin || args.jellyfinPlay || - Boolean(args.youtubePlay) || args.texthooker ) { if (args.launchMpv) { diff --git a/src/core/services/cli-command.test.ts b/src/core/services/cli-command.test.ts index 21f4f30..d59d287 100644 --- a/src/core/services/cli-command.test.ts +++ b/src/core/services/cli-command.test.ts @@ -250,6 +250,21 @@ test('handleCliCommand starts youtube playback flow on initial launch', () => { ]); }); +test('handleCliCommand defaults youtube mode to download when omitted', () => { + const { deps, calls } = createDeps({ + runYoutubePlaybackFlow: async (request) => { + calls.push(`youtube:${request.url}:${request.mode}`); + }, + }); + + handleCliCommand(makeArgs({ youtubePlay: 'https://youtube.com/watch?v=abc' }), 'initial', deps); + + assert.deepEqual(calls, [ + 'initializeOverlayRuntime', + 'youtube:https://youtube.com/watch?v=abc:download', + ]); +}); + test('handleCliCommand processes --start for second-instance when overlay runtime is not initialized', () => { const { deps, calls } = createDeps(); const args = makeArgs({ start: true }); diff --git a/src/core/services/overlay-runtime-init.test.ts b/src/core/services/overlay-runtime-init.test.ts index 63240a6..9fd9ad3 100644 --- a/src/core/services/overlay-runtime-init.test.ts +++ b/src/core/services/overlay-runtime-init.test.ts @@ -152,6 +152,93 @@ test('initializeOverlayAnkiIntegration can initialize Anki transport after overl assert.equal(setIntegrationCalls, 1); }); +test('initializeOverlayAnkiIntegration returns false when integration already exists', () => { + let createdIntegrations = 0; + let startedIntegrations = 0; + let setIntegrationCalls = 0; + + const result = initializeOverlayAnkiIntegration({ + getResolvedConfig: () => ({ + ankiConnect: { enabled: true } as never, + }), + getSubtitleTimingTracker: () => ({}), + getMpvClient: () => ({ + send: () => {}, + }), + getRuntimeOptionsManager: () => ({ + getEffectiveAnkiConnectConfig: (config) => config as never, + }), + getAnkiIntegration: () => ({}), + createAnkiIntegration: () => { + createdIntegrations += 1; + return { + start: () => { + startedIntegrations += 1; + }, + }; + }, + setAnkiIntegration: () => { + setIntegrationCalls += 1; + }, + showDesktopNotification: () => {}, + createFieldGroupingCallback: () => async () => ({ + keepNoteId: 11, + deleteNoteId: 12, + deleteDuplicate: false, + cancelled: false, + }), + getKnownWordCacheStatePath: () => '/tmp/known-words-cache.json', + }); + + assert.equal(result, false); + assert.equal(createdIntegrations, 0); + assert.equal(startedIntegrations, 0); + assert.equal(setIntegrationCalls, 0); +}); + +test('initializeOverlayAnkiIntegration returns false when ankiConnect is disabled', () => { + let createdIntegrations = 0; + let startedIntegrations = 0; + let setIntegrationCalls = 0; + + const result = initializeOverlayAnkiIntegration({ + getResolvedConfig: () => ({ + ankiConnect: { enabled: false } as never, + }), + getSubtitleTimingTracker: () => ({}), + getMpvClient: () => ({ + send: () => {}, + }), + getRuntimeOptionsManager: () => ({ + getEffectiveAnkiConnectConfig: (config) => config as never, + }), + createAnkiIntegration: () => { + createdIntegrations += 1; + return { + start: () => { + startedIntegrations += 1; + }, + }; + }, + setAnkiIntegration: () => { + setIntegrationCalls += 1; + }, + showDesktopNotification: () => {}, + createFieldGroupingCallback: () => async () => ({ + keepNoteId: 11, + deleteNoteId: 12, + deleteDuplicate: false, + cancelled: false, + }), + getKnownWordCacheStatePath: () => '/tmp/known-words-cache.json', + }); + + assert.equal(result, false); + assert.equal(createdIntegrations, 0); + assert.equal(startedIntegrations, 0); + assert.equal(setIntegrationCalls, 0); +}); + test('initializeOverlayRuntime can skip starting Anki integration transport', () => { let createdIntegrations = 0; let startedIntegrations = 0; diff --git a/src/core/services/startup.test.ts b/src/core/services/startup.test.ts index 7e6b098..156c994 100644 --- a/src/core/services/startup.test.ts +++ b/src/core/services/startup.test.ts @@ -195,6 +195,80 @@ test('runAppReadyRuntime headless refresh bootstraps Anki runtime without UI sta ]); }); +test('runAppReadyRuntime loads Yomitan before headless overlay fallback initialization', async () => { + const calls: string[] = []; + + await runAppReadyRuntime({ + ensureDefaultConfigBootstrap: () => { + calls.push('bootstrap'); + }, + loadSubtitlePosition: () => { + calls.push('load-subtitle-position'); + }, + resolveKeybindings: () => { + calls.push('resolve-keybindings'); + }, + createMpvClient: () => { + calls.push('create-mpv'); + }, + reloadConfig: () => { + calls.push('reload-config'); + }, + getResolvedConfig: () => ({}), + getConfigWarnings: () => [], + logConfigWarning: () => {}, + setLogLevel: () => {}, + initRuntimeOptionsManager: () => { + calls.push('init-runtime-options'); + }, + setSecondarySubMode: () => {}, + defaultSecondarySubMode: 'hover', + defaultWebsocketPort: 0, + defaultAnnotationWebsocketPort: 0, + defaultTexthookerPort: 0, + hasMpvWebsocketPlugin: () => false, + startSubtitleWebsocket: () => {}, + startAnnotationWebsocket: () => {}, + startTexthooker: () => {}, + log: () => {}, + createMecabTokenizerAndCheck: async () => {}, + createSubtitleTimingTracker: () => { + calls.push('subtitle-timing'); + }, + createImmersionTracker: () => {}, + startJellyfinRemoteSession: async () => {}, + loadYomitanExtension: async () => { + calls.push('load-yomitan'); + }, + handleFirstRunSetup: async () => {}, + prewarmSubtitleDictionaries: async () => {}, + startBackgroundWarmups: () => {}, + texthookerOnlyMode: false, + shouldAutoInitializeOverlayRuntimeFromConfig: () => false, + setVisibleOverlayVisible: () => {}, + initializeOverlayRuntime: () => { + calls.push('init-overlay'); + }, + handleInitialArgs: () => { + calls.push('handle-initial-args'); + }, + shouldRunHeadlessInitialCommand: () => true, + shouldUseMinimalStartup: () => false, + shouldSkipHeavyStartup: () => false, + }); + + assert.deepEqual(calls, [ + 'bootstrap', + 'reload-config', + 'init-runtime-options', + 'create-mpv', + 'subtitle-timing', + 'load-yomitan', + 'init-overlay', + 'handle-initial-args', + ]); +}); + test('runAppReadyRuntime loads Yomitan before auto-initializing overlay runtime', async () => { const calls: string[] = []; diff --git a/src/core/services/startup.ts b/src/core/services/startup.ts index fcb32b4..722a064 100644 --- a/src/core/services/startup.ts +++ b/src/core/services/startup.ts @@ -194,6 +194,7 @@ export async function runAppReadyRuntime(deps: AppReadyRuntimeDeps): Promise { - if (process.platform === 'win32') { - return; - } - const root = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-youtube-retime-')); try { const primaryPath = path.join(root, 'primary.vtt'); diff --git a/src/core/services/youtube/retime.ts b/src/core/services/youtube/retime.ts index 878bb2e..a1dd29c 100644 --- a/src/core/services/youtube/retime.ts +++ b/src/core/services/youtube/retime.ts @@ -1,7 +1,7 @@ export async function retimeYoutubeSubtitle(input: { primaryPath: string; secondaryPath: string | null; -}): Promise<{ ok: boolean; path: string; strategy: 'none'; message: string }> { +}): Promise<{ ok: boolean; path: string; strategy: 'none' | 'alass' | 'ffsubsync'; message: string }> { return { ok: true, path: input.primaryPath, diff --git a/src/core/services/youtube/timedtext.ts b/src/core/services/youtube/timedtext.ts index 35878cb..905d449 100644 --- a/src/core/services/youtube/timedtext.ts +++ b/src/core/services/youtube/timedtext.ts @@ -74,16 +74,31 @@ export function convertYoutubeTimedTextToVtt(xml: string): string { return 'WEBVTT\n'; } - const blocks = rows.map((row, index) => { + const blocks: string[] = []; + let previousText = ''; + for (let index = 0; index < rows.length; index += 1) { + const row = rows[index]!; const nextRow = rows[index + 1]; const unclampedEnd = row.startMs + row.durationMs; const clampedEnd = nextRow && unclampedEnd > nextRow.startMs ? Math.max(row.startMs, nextRow.startMs - 1) : unclampedEnd; + if (clampedEnd <= row.startMs) { + previousText = row.text; + continue; + } - return `${formatVttTimestamp(row.startMs)} --> ${formatVttTimestamp(clampedEnd)}\n${row.text}`; - }); + const text = + previousText && row.text.startsWith(previousText) + ? row.text.slice(previousText.length).trimStart() + : row.text; + previousText = row.text; + if (!text) { + continue; + } + blocks.push(`${formatVttTimestamp(row.startMs)} --> ${formatVttTimestamp(clampedEnd)}\n${text}`); + } return `WEBVTT\n\n${blocks.join('\n\n')}\n`; } diff --git a/src/core/services/youtube/track-download.test.ts b/src/core/services/youtube/track-download.test.ts index 800dfd0..3c317fe 100644 --- a/src/core/services/youtube/track-download.test.ts +++ b/src/core/services/youtube/track-download.test.ts @@ -470,3 +470,48 @@ test('downloadYoutubeSubtitleTracks prefers direct download URLs when available' ); }); }); + +test('downloadYoutubeSubtitleTracks keeps duplicate source-language direct downloads distinct', async () => { + await withTempDir(async (root) => { + const seen: string[] = []; + await withStubFetch( + async (url) => { + seen.push(url); + return new Response(`WEBVTT\n${url}\n`, { status: 200 }); + }, + async () => { + const result = await downloadYoutubeSubtitleTracks({ + targetUrl: 'https://www.youtube.com/watch?v=abc123', + outputDir: path.join(root, 'out'), + tracks: [ + { + id: 'auto:ja-orig', + language: 'ja', + sourceLanguage: 'ja-orig', + kind: 'auto', + label: 'Japanese (auto)', + downloadUrl: 'https://example.com/subs/ja-auto.vtt', + fileExtension: 'vtt', + }, + { + id: 'manual:ja-orig', + language: 'ja', + sourceLanguage: 'ja-orig', + kind: 'manual', + label: 'Japanese (manual)', + downloadUrl: 'https://example.com/subs/ja-manual.vtt', + fileExtension: 'vtt', + }, + ], + mode: 'download', + }); + + assert.deepEqual(seen, [ + 'https://example.com/subs/ja-auto.vtt', + 'https://example.com/subs/ja-manual.vtt', + ]); + assert.notEqual(result.get('auto:ja-orig'), result.get('manual:ja-orig')); + }, + ); + }); +}); diff --git a/src/core/services/youtube/track-download.ts b/src/core/services/youtube/track-download.ts index 7f446af..90e24ca 100644 --- a/src/core/services/youtube/track-download.ts +++ b/src/core/services/youtube/track-download.ts @@ -7,12 +7,28 @@ import { convertYoutubeTimedTextToVtt, isYoutubeTimedTextExtension } from './tim const YOUTUBE_SUBTITLE_EXTENSIONS = new Set(['.srt', '.vtt', '.ass']); const YOUTUBE_BATCH_PREFIX = 'youtube-batch'; +const YOUTUBE_DOWNLOAD_TIMEOUT_MS = 15_000; -function runCapture(command: string, args: string[]): Promise<{ stdout: string; stderr: string }> { +function createFetchTimeoutSignal(timeoutMs: number): AbortSignal | undefined { + if (typeof AbortSignal !== 'undefined' && typeof AbortSignal.timeout === 'function') { + return AbortSignal.timeout(timeoutMs); + } + return undefined; +} + +function runCapture( + command: string, + args: string[], + timeoutMs = YOUTUBE_DOWNLOAD_TIMEOUT_MS, +): Promise<{ stdout: string; stderr: string }> { return new Promise((resolve, reject) => { const proc = spawn(command, args, { stdio: ['ignore', 'pipe', 'pipe'] }); let stdout = ''; let stderr = ''; + const timer = setTimeout(() => { + proc.kill(); + reject(new Error(`yt-dlp timed out after ${timeoutMs}ms`)); + }, timeoutMs); proc.stdout.setEncoding('utf8'); proc.stderr.setEncoding('utf8'); proc.stdout.on('data', (chunk) => { @@ -21,8 +37,12 @@ function runCapture(command: string, args: string[]): Promise<{ stdout: string; proc.stderr.on('data', (chunk) => { stderr += String(chunk); }); - proc.once('error', reject); + proc.once('error', (error) => { + clearTimeout(timer); + reject(error); + }); proc.once('close', (code) => { + clearTimeout(timer); if (code === 0) { resolve({ stdout, stderr }); return; @@ -35,11 +55,16 @@ function runCapture(command: string, args: string[]): Promise<{ stdout: string; function runCaptureDetailed( command: string, args: string[], + timeoutMs = YOUTUBE_DOWNLOAD_TIMEOUT_MS, ): Promise<{ stdout: string; stderr: string; code: number }> { return new Promise((resolve, reject) => { const proc = spawn(command, args, { stdio: ['ignore', 'pipe', 'pipe'] }); let stdout = ''; let stderr = ''; + const timer = setTimeout(() => { + proc.kill(); + reject(new Error(`yt-dlp timed out after ${timeoutMs}ms`)); + }, timeoutMs); proc.stdout.setEncoding('utf8'); proc.stderr.setEncoding('utf8'); proc.stdout.on('data', (chunk) => { @@ -48,8 +73,12 @@ function runCaptureDetailed( proc.stderr.on('data', (chunk) => { stderr += String(chunk); }); - proc.once('error', reject); + proc.once('error', (error) => { + clearTimeout(timer); + reject(error); + }); proc.once('close', (code) => { + clearTimeout(timer); resolve({ stdout, stderr, code: code ?? 1 }); }); }); @@ -125,8 +154,13 @@ async function downloadSubtitleFromUrl(input: { : YOUTUBE_SUBTITLE_EXTENSIONS.has(`.${ext}`) ? ext : 'vtt'; - const targetPath = path.join(input.outputDir, `${input.prefix}.${input.track.sourceLanguage}.${safeExt}`); - const response = await fetch(input.track.downloadUrl); + const targetPath = path.join( + input.outputDir, + `${input.prefix}.${input.track.sourceLanguage}.${safeExt}`, + ); + const response = await fetch(input.track.downloadUrl, { + signal: createFetchTimeoutSignal(YOUTUBE_DOWNLOAD_TIMEOUT_MS), + }); if (!response.ok) { throw new Error(`HTTP ${response.status} while downloading ${input.track.sourceLanguage}`); } @@ -195,6 +229,8 @@ export async function downloadYoutubeSubtitleTracks(input: { mode: YoutubeFlowMode; }): Promise> { fs.mkdirSync(input.outputDir, { recursive: true }); + const hasDuplicateSourceLanguages = + new Set(input.tracks.map((track) => track.sourceLanguage)).size !== input.tracks.length; for (const name of fs.readdirSync(input.outputDir)) { if (name.startsWith(`${YOUTUBE_BATCH_PREFIX}.`)) { try { @@ -204,12 +240,12 @@ export async function downloadYoutubeSubtitleTracks(input: { } } } - if (input.tracks.every(canDownloadSubtitleFromUrl)) { + if (hasDuplicateSourceLanguages || input.tracks.every(canDownloadSubtitleFromUrl)) { const results = new Map(); for (const track of input.tracks) { const download = await downloadSubtitleFromUrl({ outputDir: input.outputDir, - prefix: YOUTUBE_BATCH_PREFIX, + prefix: track.id.replace(/[^a-z0-9_-]+/gi, '-'), track, }); results.set(track.id, download.path); diff --git a/src/core/services/youtube/track-probe.test.ts b/src/core/services/youtube/track-probe.test.ts index cf05ddc..0c5eda8 100644 --- a/src/core/services/youtube/track-probe.test.ts +++ b/src/core/services/youtube/track-probe.test.ts @@ -16,11 +16,15 @@ async function withTempDir(fn: (dir: string) => Promise): Promise { function makeFakeYtDlpScript(dir: string, payload: unknown): void { const scriptPath = path.join(dir, 'yt-dlp'); + const stdoutBody = typeof payload === 'string' ? payload : JSON.stringify(payload); const script = `#!/usr/bin/env node -process.stdout.write(${JSON.stringify(JSON.stringify(payload))}); +process.stdout.write(${JSON.stringify(stdoutBody)}); `; fs.writeFileSync(scriptPath, script, 'utf8'); - fs.chmodSync(scriptPath, 0o755); + if (process.platform !== 'win32') { + fs.chmodSync(scriptPath, 0o755); + } + fs.writeFileSync(scriptPath + '.cmd', `@echo off\r\nnode "${scriptPath}"\r\n`, 'utf8'); } async function withFakeYtDlp(payload: unknown, fn: () => Promise): Promise { @@ -78,3 +82,12 @@ test('probeYoutubeTracks keeps preferring srt for manual captions', async () => }, ); }); + +test('probeYoutubeTracks reports malformed yt-dlp JSON with context', async () => { + await withFakeYtDlp('not-json', async () => { + await assert.rejects( + async () => await probeYoutubeTracks('https://www.youtube.com/watch?v=abc123'), + /Failed to parse yt-dlp output as JSON/, + ); + }); +}); diff --git a/src/core/services/youtube/track-probe.ts b/src/core/services/youtube/track-probe.ts index bfad523..c1a695a 100644 --- a/src/core/services/youtube/track-probe.ts +++ b/src/core/services/youtube/track-probe.ts @@ -1,10 +1,6 @@ import { spawn } from 'node:child_process'; import type { YoutubeTrackOption } from '../../../types'; -import { - formatYoutubeTrackLabel, - normalizeYoutubeLangCode, - type YoutubeTrackKind, -} from './labels'; +import { formatYoutubeTrackLabel, normalizeYoutubeLangCode, type YoutubeTrackKind } from './labels'; export type YoutubeTrackProbeResult = { videoId: string; @@ -102,7 +98,21 @@ export type { YoutubeTrackOption }; export async function probeYoutubeTracks(targetUrl: string): Promise { const { stdout } = await runCapture('yt-dlp', ['--dump-single-json', '--no-warnings', targetUrl]); - const info = JSON.parse(stdout) as YtDlpInfo; + const trimmedStdout = stdout.trim(); + if (!trimmedStdout) { + throw new Error('yt-dlp returned empty output while probing subtitle tracks'); + } + let info: YtDlpInfo; + try { + info = JSON.parse(trimmedStdout) as YtDlpInfo; + } catch (error) { + const snippet = trimmedStdout.slice(0, 200); + throw new Error( + `Failed to parse yt-dlp output as JSON: ${ + error instanceof Error ? error.message : String(error) + }${snippet ? `; stdout=${snippet}` : ''}`, + ); + } const tracks = [...toTracks(info.subtitles, 'manual'), ...toTracks(info.automatic_captions, 'auto')]; return { videoId: info.id || '', diff --git a/src/main.ts b/src/main.ts index 2dc9daa..afaebcc 100644 --- a/src/main.ts +++ b/src/main.ts @@ -933,7 +933,8 @@ async function runYoutubePlaybackFlowMain(request: { mode: 'download' | 'generate'; source: CliCommandSource; }): Promise { - const shouldResumeWarmupsAfterFlow = appState.youtubePlaybackFlowPending; + const wasYoutubePlaybackFlowPending = appState.youtubePlaybackFlowPending; + appState.youtubePlaybackFlowPending = true; if (process.platform === 'win32' && !appState.mpvClient?.connected) { const launchResult = launchWindowsMpv( [request.url], @@ -964,8 +965,8 @@ async function runYoutubePlaybackFlowMain(request: { }); logger.info(`YouTube playback flow completed from ${request.source}.`); } finally { - if (shouldResumeWarmupsAfterFlow) { - appState.youtubePlaybackFlowPending = false; + appState.youtubePlaybackFlowPending = wasYoutubePlaybackFlowPending; + if (!wasYoutubePlaybackFlowPending) { startBackgroundWarmupsIfAllowed(); } } diff --git a/src/main/runtime/youtube-flow.test.ts b/src/main/runtime/youtube-flow.test.ts index 4d8bd3c..cedf13a 100644 --- a/src/main/runtime/youtube-flow.test.ts +++ b/src/main/runtime/youtube-flow.test.ts @@ -449,3 +449,60 @@ test('youtube flow waits for tokenization readiness before releasing playback', 'focus-overlay', ]); }); + +test('youtube flow cleans up paused picker state when opening the picker throws', async () => { + const commands: Array> = []; + const warns: string[] = []; + const focusOverlayCalls: string[] = []; + + const runtime = createYoutubeFlowRuntime({ + probeYoutubeTracks: async () => ({ + videoId: 'video123', + title: 'Video 123', + tracks: [primaryTrack], + }), + acquireYoutubeSubtitleTracks: async () => new Map(), + acquireYoutubeSubtitleTrack: async () => ({ path: '/tmp/auto-ja-orig.vtt' }), + retimeYoutubePrimaryTrack: async ({ primaryPath }) => primaryPath, + startTokenizationWarmups: async () => {}, + waitForTokenizationReady: async () => {}, + waitForAnkiReady: async () => {}, + waitForPlaybackWindowReady: async () => {}, + waitForOverlayGeometryReady: async () => {}, + focusOverlayWindow: () => { + focusOverlayCalls.push('focus-overlay'); + }, + openPicker: async () => { + throw new Error('picker boom'); + }, + pauseMpv: () => { + commands.push(['set_property', 'pause', 'yes']); + }, + resumeMpv: () => { + commands.push(['set_property', 'pause', 'no']); + }, + sendMpvCommand: (command) => { + commands.push(command); + }, + requestMpvProperty: async () => null, + refreshCurrentSubtitle: () => {}, + wait: async () => {}, + showMpvOsd: () => {}, + warn: (message) => { + warns.push(message); + }, + log: () => {}, + getYoutubeOutputDir: () => '/tmp', + }); + + await runtime.runYoutubePlaybackFlow({ url: 'https://example.com', mode: 'download' }); + + assert.deepEqual(commands, [ + ['set_property', 'pause', 'yes'], + ['script-message', 'subminer-autoplay-ready'], + ['set_property', 'pause', 'no'], + ]); + assert.deepEqual(focusOverlayCalls, ['focus-overlay']); + assert.equal(warns.some((message) => message.includes('picker boom')), true); + assert.equal(runtime.hasActiveSession(), false); +}); diff --git a/src/main/runtime/youtube-flow.ts b/src/main/runtime/youtube-flow.ts index 9dcdd9d..fbe5c1a 100644 --- a/src/main/runtime/youtube-flow.ts +++ b/src/main/runtime/youtube-flow.ts @@ -392,12 +392,26 @@ export function createYoutubeFlowRuntime(deps: YoutubeFlowDeps) { }`, ); }); - const probe = await deps.probeYoutubeTracks(input.url); - const defaults = chooseDefaultYoutubeTrackIds(probe.tracks); - const sessionId = createSessionId(); - const outputDir = normalizeOutputPath(deps.getYoutubeOutputDir()); deps.pauseMpv(); + const outputDir = normalizeOutputPath(deps.getYoutubeOutputDir()); + + let probe: YoutubeTrackProbeResult; + try { + probe = await deps.probeYoutubeTracks(input.url); + } catch (error) { + deps.warn( + `Failed to probe YouTube subtitle tracks: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + releasePlaybackGate(deps); + restoreOverlayInputFocus(deps); + return; + } + + const defaults = chooseDefaultYoutubeTrackIds(probe.tracks); + const sessionId = createSessionId(); const openPayload: YoutubePickerOpenPayload = { sessionId, @@ -416,7 +430,22 @@ export function createYoutubeFlowRuntime(deps: YoutubeFlowDeps) { deps.showMpvOsd('Getting subtitles...'); const pickerSelection = createPickerSelectionPromise(sessionId); void pickerSelection.catch(() => undefined); - const opened = await deps.openPicker(openPayload); + let opened = false; + try { + opened = await deps.openPicker(openPayload); + } catch (error) { + activeSession?.reject( + error instanceof Error ? error : new Error(String(error)), + ); + deps.warn( + `Unable to open YouTube subtitle picker: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + releasePlaybackGate(deps); + restoreOverlayInputFocus(deps); + return; + } if (!opened) { activeSession?.reject(new Error('Unable to open YouTube subtitle picker.')); activeSession = null; diff --git a/src/preload-args.test.ts b/src/preload-args.test.ts index 0eb3150..ec6dd07 100644 --- a/src/preload-args.test.ts +++ b/src/preload-args.test.ts @@ -6,6 +6,14 @@ test('resolveOverlayLayerFromArgv returns null when argv is unavailable', () => assert.equal(resolveOverlayLayerFromArgv(null), null); }); +test('resolveOverlayLayerFromArgv returns null for undefined argv', () => { + assert.equal(resolveOverlayLayerFromArgv(undefined), null); +}); + +test('resolveOverlayLayerFromArgv returns null for empty argv', () => { + assert.equal(resolveOverlayLayerFromArgv([]), null); +}); + test('resolveOverlayLayerFromArgv returns parsed overlay layer when present', () => { assert.equal(resolveOverlayLayerFromArgv(['electron', '--overlay-layer=modal']), 'modal'); assert.equal(resolveOverlayLayerFromArgv(['electron', '--overlay-layer=visible']), 'visible'); @@ -13,4 +21,5 @@ test('resolveOverlayLayerFromArgv returns parsed overlay layer when present', () test('resolveOverlayLayerFromArgv ignores unsupported overlay layers', () => { assert.equal(resolveOverlayLayerFromArgv(['electron', '--overlay-layer=secondary']), null); + assert.equal(resolveOverlayLayerFromArgv(['electron', '--overlay-layer=']), null); }); diff --git a/src/renderer/handlers/mouse.ts b/src/renderer/handlers/mouse.ts index 5f44d9c..56cda6f 100644 --- a/src/renderer/handlers/mouse.ts +++ b/src/renderer/handlers/mouse.ts @@ -259,7 +259,6 @@ export function createMouseHandlers( }); document.addEventListener('mousemove', (e: MouseEvent) => { - updatePointerPosition(e); if (!ctx.state.isDragging) return; const deltaY = ctx.state.dragStartY - e.clientY; diff --git a/src/renderer/modals/youtube-track-picker.test.ts b/src/renderer/modals/youtube-track-picker.test.ts index 0a022a2..c1c3ff0 100644 --- a/src/renderer/modals/youtube-track-picker.test.ts +++ b/src/renderer/modals/youtube-track-picker.test.ts @@ -172,3 +172,179 @@ test('youtube track picker close restores focus and mouse-ignore state', () => { }); } }); + +test('youtube track picker re-acknowledges repeated open requests', () => { + const openedNotifications: string[] = []; + const originalWindow = globalThis.window; + const originalDocument = globalThis.document; + + Object.defineProperty(globalThis, 'document', { + configurable: true, + value: { + createElement: () => createFakeElement(), + }, + }); + + Object.defineProperty(globalThis, 'window', { + configurable: true, + value: { + dispatchEvent: () => true, + focus: () => {}, + electronAPI: { + notifyOverlayModalOpened: (modal: string) => { + openedNotifications.push(modal); + }, + notifyOverlayModalClosed: () => {}, + youtubePickerResolve: async () => ({ ok: true, message: '' }), + setIgnoreMouseEvents: () => {}, + }, + }, + }); + + try { + const state = createRendererState(); + const dom = { + overlay: { + classList: createClassList(), + focus: () => {}, + }, + youtubePickerModal: createFakeElement(), + youtubePickerTitle: createFakeElement(), + youtubePickerPrimarySelect: createFakeElement(), + youtubePickerSecondarySelect: createFakeElement(), + youtubePickerTracks: createFakeElement(), + youtubePickerStatus: createFakeElement(), + youtubePickerContinueButton: createFakeElement(), + youtubePickerCloseButton: createFakeElement(), + }; + + const modal = createYoutubeTrackPickerModal( + { + state, + dom, + platform: { + shouldToggleMouseIgnore: false, + }, + } as never, + { + modalStateReader: { isAnyModalOpen: () => true }, + restorePointerInteractionState: () => {}, + syncSettingsModalSubtitleSuppression: () => {}, + }, + ); + + modal.openYoutubePickerModal({ + sessionId: 'yt-1', + url: 'https://example.com/one', + mode: 'download', + tracks: [], + defaultPrimaryTrackId: null, + defaultSecondaryTrackId: null, + hasTracks: false, + }); + modal.openYoutubePickerModal({ + sessionId: 'yt-2', + url: 'https://example.com/two', + mode: 'generate', + tracks: [], + defaultPrimaryTrackId: null, + defaultSecondaryTrackId: null, + hasTracks: false, + }); + + assert.deepEqual(openedNotifications, ['youtube-track-picker', 'youtube-track-picker']); + assert.equal(state.youtubePickerPayload?.sessionId, 'yt-2'); + } finally { + Object.defineProperty(globalThis, 'window', { configurable: true, value: originalWindow }); + Object.defineProperty(globalThis, 'document', { configurable: true, value: originalDocument }); + } +}); + +test('youtube track picker surfaces rejected resolve calls as modal status', async () => { + const originalWindow = globalThis.window; + const originalDocument = globalThis.document; + + Object.defineProperty(globalThis, 'document', { + configurable: true, + value: { + createElement: () => createFakeElement(), + }, + }); + + Object.defineProperty(globalThis, 'window', { + configurable: true, + value: { + dispatchEvent: () => true, + focus: () => {}, + electronAPI: { + notifyOverlayModalOpened: () => {}, + notifyOverlayModalClosed: () => {}, + youtubePickerResolve: async () => { + throw new Error('resolve failed'); + }, + setIgnoreMouseEvents: () => {}, + }, + }, + }); + + try { + const state = createRendererState(); + const dom = { + overlay: { + classList: createClassList(), + focus: () => {}, + }, + youtubePickerModal: createFakeElement(), + youtubePickerTitle: createFakeElement(), + youtubePickerPrimarySelect: createFakeElement(), + youtubePickerSecondarySelect: createFakeElement(), + youtubePickerTracks: createFakeElement(), + youtubePickerStatus: createFakeElement(), + youtubePickerContinueButton: createFakeElement(), + youtubePickerCloseButton: createFakeElement(), + }; + + const modal = createYoutubeTrackPickerModal( + { + state, + dom, + platform: { + shouldToggleMouseIgnore: false, + }, + } as never, + { + modalStateReader: { isAnyModalOpen: () => true }, + restorePointerInteractionState: () => {}, + syncSettingsModalSubtitleSuppression: () => {}, + }, + ); + + modal.openYoutubePickerModal({ + sessionId: 'yt-1', + url: 'https://example.com', + mode: 'download', + tracks: [ + { + id: 'auto:ja-orig', + language: 'ja', + sourceLanguage: 'ja-orig', + kind: 'auto', + label: 'Japanese (auto)', + }, + ], + defaultPrimaryTrackId: 'auto:ja-orig', + defaultSecondaryTrackId: null, + hasTracks: true, + }); + modal.wireDomEvents(); + + const listeners = dom.youtubePickerContinueButton.listeners.get('click') ?? []; + await Promise.all(listeners.map((listener) => listener())); + + assert.equal(state.youtubePickerModalOpen, true); + assert.equal(dom.youtubePickerStatus.textContent, 'resolve failed'); + } finally { + Object.defineProperty(globalThis, 'window', { configurable: true, value: originalWindow }); + Object.defineProperty(globalThis, 'document', { configurable: true, value: originalDocument }); + } +}); diff --git a/src/renderer/modals/youtube-track-picker.ts b/src/renderer/modals/youtube-track-picker.ts index 0a5d799..9f3136b 100644 --- a/src/renderer/modals/youtube-track-picker.ts +++ b/src/renderer/modals/youtube-track-picker.ts @@ -114,13 +114,26 @@ export function createYoutubeTrackPickerModal( return; } - const response = await window.electronAPI.youtubePickerResolve({ - sessionId: payload.sessionId, - action, - primaryTrackId: action === 'use-selected' ? ctx.dom.youtubePickerPrimarySelect.value || null : null, - secondaryTrackId: - action === 'use-selected' ? ctx.dom.youtubePickerSecondarySelect.value || null : null, - }); + let response; + try { + response = + action === 'use-selected' + ? await window.electronAPI.youtubePickerResolve({ + sessionId: payload.sessionId, + action: 'use-selected', + primaryTrackId: ctx.dom.youtubePickerPrimarySelect.value || null, + secondaryTrackId: ctx.dom.youtubePickerSecondarySelect.value || null, + }) + : await window.electronAPI.youtubePickerResolve({ + sessionId: payload.sessionId, + action: 'continue-without-subtitles', + primaryTrackId: null, + secondaryTrackId: null, + }); + } catch (error) { + setStatus(error instanceof Error ? error.message : String(error), true); + return; + } if (!response.ok) { setStatus(response.message, true); return; @@ -129,7 +142,12 @@ export function createYoutubeTrackPickerModal( } function openYoutubePickerModal(payload: YoutubePickerOpenPayload): void { - if (ctx.state.youtubePickerModalOpen) return; + if (ctx.state.youtubePickerModalOpen) { + options.syncSettingsModalSubtitleSuppression(); + applyPayload(payload); + window.electronAPI.notifyOverlayModalOpened('youtube-track-picker'); + return; + } ctx.state.youtubePickerModalOpen = true; options.syncSettingsModalSubtitleSuppression(); applyPayload(payload); diff --git a/src/shared/ipc/validators.ts b/src/shared/ipc/validators.ts index b0160e5..8ce1001 100644 --- a/src/shared/ipc/validators.ts +++ b/src/shared/ipc/validators.ts @@ -259,6 +259,17 @@ export function parseYoutubePickerResolveRequest(value: unknown): YoutubePickerR if (!isObject(value)) return null; if (typeof value.sessionId !== 'string' || !value.sessionId.trim()) return null; if (value.action !== 'use-selected' && value.action !== 'continue-without-subtitles') return null; + if (value.action === 'continue-without-subtitles') { + if (value.primaryTrackId !== null || value.secondaryTrackId !== null) { + return null; + } + return { + sessionId: value.sessionId, + action: 'continue-without-subtitles', + primaryTrackId: null, + secondaryTrackId: null, + }; + } if (value.primaryTrackId !== null && value.primaryTrackId !== undefined && typeof value.primaryTrackId !== 'string') { return null; } @@ -271,7 +282,7 @@ export function parseYoutubePickerResolveRequest(value: unknown): YoutubePickerR } return { sessionId: value.sessionId, - action: value.action, + action: 'use-selected', primaryTrackId: value.primaryTrackId ?? null, secondaryTrackId: value.secondaryTrackId ?? null, }; diff --git a/src/shared/log-files.test.ts b/src/shared/log-files.test.ts index 797c482..5c4d6c3 100644 --- a/src/shared/log-files.test.ts +++ b/src/shared/log-files.test.ts @@ -10,9 +10,11 @@ import { } from './log-files'; test('resolveDefaultLogFilePath uses app prefix by default', () => { + const now = new Date('2026-03-22T12:00:00.000Z'); const resolved = resolveDefaultLogFilePath('app', { platform: 'linux', homeDir: '/home/tester', + now, }); assert.equal( @@ -22,7 +24,7 @@ test('resolveDefaultLogFilePath uses app prefix by default', () => { '.config', 'SubMiner', 'logs', - `app-${new Date().toISOString().slice(0, 10)}.log`, + `app-${now.toISOString().slice(0, 10)}.log`, ), ); }); diff --git a/src/types.ts b/src/types.ts index 1fed44b..d5e3c39 100644 --- a/src/types.ts +++ b/src/types.ts @@ -17,6 +17,7 @@ */ import type { SubtitleCue } from './core/services/subtitle-cue-parser'; +import type { YoutubeTrackKind } from './core/services/youtube/kinds'; export enum PartOfSpeech { noun = 'noun', @@ -561,7 +562,7 @@ export interface ControllerRuntimeSnapshot { export type JimakuLanguagePreference = 'ja' | 'en' | 'none'; export type YoutubeFlowMode = 'download' | 'generate'; -export type YoutubeTrackKind = 'manual' | 'auto'; +export type { YoutubeTrackKind }; export interface YoutubeTrackOption { id: string; @@ -584,12 +585,19 @@ export interface YoutubePickerOpenPayload { hasTracks: boolean; } -export interface YoutubePickerResolveRequest { - sessionId: string; - action: 'use-selected' | 'continue-without-subtitles'; - primaryTrackId: string | null; - secondaryTrackId: string | null; -} +export type YoutubePickerResolveRequest = + | { + sessionId: string; + action: 'continue-without-subtitles'; + primaryTrackId: null; + secondaryTrackId: null; + } + | { + sessionId: string; + action: 'use-selected'; + primaryTrackId: string | null; + secondaryTrackId: string | null; + }; export interface YoutubePickerResolveResult { ok: boolean;