From bf06463bb36197280dafbec3358dd2d1a9b9558f Mon Sep 17 00:00:00 2001 From: sudacode Date: Fri, 3 Apr 2026 01:14:57 -0700 Subject: [PATCH] fix: address CodeRabbit follow-ups for PR #40 --- README.md | 2 +- ...ress-PR-40-CodeRabbit-review-follow-ups.md | 39 +++++++++ changes/268-windows-mpv-shortcut-defaults.md | 1 + docs-site/installation.md | 2 +- docs-site/usage.md | 6 +- launcher/mpv.test.ts | 86 ++++++++++++++----- launcher/mpv.ts | 63 ++++++++------ src/main-entry-runtime.test.ts | 20 +++++ src/main-entry-runtime.ts | 62 ++++++------- src/main/runtime/first-run-setup-service.ts | 18 ++++ .../runtime/first-run-setup-window.test.ts | 45 ++++++++++ src/main/runtime/first-run-setup-window.ts | 27 ++++-- src/main/runtime/windows-mpv-launch.test.ts | 59 +++++++++---- src/main/runtime/windows-mpv-launch.ts | 27 +++++- 14 files changed, 341 insertions(+), 116 deletions(-) create mode 100644 backlog/completed/task-272 - Assess-and-address-PR-40-CodeRabbit-review-follow-ups.md diff --git a/README.md b/README.md index f781512b..11c4ae1d 100644 --- a/README.md +++ b/README.md @@ -228,7 +228,7 @@ See the [build-from-source guide](https://docs.subminer.moe/installation#from-so ### 2. First Launch -Run the app. On first launch SubMiner starts in the system tray, creates a default config, and opens a setup popup to install the mpv plugin and configure Yomitan dictionaries. +Run the app. On first launch SubMiner starts in the system tray, creates a default config, and opens a setup popup to finish config, install the mpv plugin, and configure Yomitan dictionaries. ### 3. Mine diff --git a/backlog/completed/task-272 - Assess-and-address-PR-40-CodeRabbit-review-follow-ups.md b/backlog/completed/task-272 - Assess-and-address-PR-40-CodeRabbit-review-follow-ups.md new file mode 100644 index 00000000..dfe53ddf --- /dev/null +++ b/backlog/completed/task-272 - Assess-and-address-PR-40-CodeRabbit-review-follow-ups.md @@ -0,0 +1,39 @@ +--- +id: TASK-272 +title: 'Assess and address PR #40 CodeRabbit review follow-ups' +status: Done +assignee: [] +created_date: '2026-04-03 07:52' +updated_date: '2026-04-03 08:04' +labels: + - coderabbit + - review + - launcher +milestone: 'PR #40' +dependencies: [] +references: + - 'https://github.com/ksyasuda/SubMiner/pull/40' +priority: medium +--- + +## Description + + +Implement the valid CodeRabbit findings on PR #40 and keep the Windows mpv shortcut / first-run setup flow consistent end to end. + + +## Acceptance Criteria + +- [ ] #1 Windows binary resolution does not return install directories as executable candidates +- [ ] #2 Launch-mpv arg parsing preserves space-separated mpv option values and target separation +- [ ] #3 Windows mpv launch args keep the final input-ipc-server and script-opts socket path in sync when custom values are supplied +- [ ] #4 First-run setup navigation swallows stale or invalid custom-scheme actions without navigating away +- [ ] #5 Setup messaging and footer copy reflect configReady, plugin, and dictionary gates consistently +- [ ] #6 Regression tests cover the fixed behaviors + + +## Final Summary + + +Addressed CodeRabbit follow-ups for PR #40. Hardened launcher binary discovery on Windows and PATH resolution, fixed launch-mpv argument parsing for value-bearing flags, synced custom Windows mpv IPC socket values into script opts, and tightened first-run setup messaging/navigation to handle stale actions and blocker copy. Verified with `bun test src/main-entry-runtime.test.ts src/main/runtime/windows-mpv-launch.test.ts src/main/runtime/first-run-setup-window.test.ts launcher/mpv.test.ts`. + diff --git a/changes/268-windows-mpv-shortcut-defaults.md b/changes/268-windows-mpv-shortcut-defaults.md index 18e41f2a..85a5f657 100644 --- a/changes/268-windows-mpv-shortcut-defaults.md +++ b/changes/268-windows-mpv-shortcut-defaults.md @@ -3,3 +3,4 @@ area: launcher - Fixed the Windows `SubMiner mpv` shortcut and `SubMiner.exe --launch-mpv` flow to launch mpv with SubMiner's required default args directly instead of requiring an `mpv.conf` profile named `subminer`. - Clarified the Windows install and usage docs so the shortcut path is documented as self-contained, while the optional `subminer` mpv profile remains available for manual mpv launches. +- Hardened the first-run setup blocker copy and stale custom-scheme handling so setup messages stay aligned with config, plugin, and dictionary readiness. diff --git a/docs-site/installation.md b/docs-site/installation.md index c4faa9b0..e567e8d8 100644 --- a/docs-site/installation.md +++ b/docs-site/installation.md @@ -172,7 +172,7 @@ Install `mpv` separately and ensure `mpv.exe` is on `PATH`. `ffmpeg` is still re ### Windows Usage Notes - Launch `SubMiner.exe` once to let the first-run setup flow seed `%APPDATA%\\SubMiner\\config.jsonc`, require mpv plugin installation, and open bundled Yomitan settings. The optional `SubMiner mpv` Start Menu/Desktop shortcut can also be created during setup, and on Windows it is the recommended way to launch mpv playback with SubMiner defaults. -- `SubMiner.exe --launch-mpv` and the optional `SubMiner mpv` shortcut pass SubMiner's default mpv socket/subtitle args directly, including the Windows-safe subtitle search paths that skip the extra current-directory scan; they do not require an `mpv.conf` profile named `subminer`. +- `SubMiner.exe --launch-mpv` and the optional `SubMiner mpv` shortcut pass SubMiner's default mpv socket/subtitle args directly and do not require an `mpv.conf` profile named `subminer`. - First-run mpv plugin installs pin `binary_path` to the current `SubMiner.exe` automatically. Manual plugin configs can leave `binary_path` empty unless SubMiner is installed in a non-standard location. - Windows plugin installs rewrite `socket_path` to `\\.\pipe\subminer-socket`; do not keep `/tmp/subminer-socket` on Windows. - Native window tracking is built in on Windows; no `xdotool`, `xwininfo`, or compositor-specific helper is required. diff --git a/docs-site/usage.md b/docs-site/usage.md index 565e7ead..9bc75b7b 100644 --- a/docs-site/usage.md +++ b/docs-site/usage.md @@ -117,7 +117,7 @@ SubMiner.AppImage --help # Show all options ### Windows mpv Shortcut -First-run setup requires the mpv plugin before it can finish. +First-run setup creates the config file, then requires the mpv plugin and Yomitan dictionaries before it can finish. If you enabled the optional Windows shortcut during install, SubMiner creates a `SubMiner mpv` shortcut in the Start menu and/or on the desktop. On Windows, that shortcut is the recommended way to launch local files with SubMiner because it starts `mpv.exe` with the right defaults directly. After setup completes, the shortcut is the normal Windows playback entry point. @@ -162,10 +162,10 @@ Setup flow: - config file: create the default config directory and prefer `config.jsonc` - plugin status: install the bundled mpv plugin before finishing setup - Yomitan shortcut: open bundled Yomitan settings directly from the setup window -- dictionary check: ensure at least one bundled Yomitan dictionary is available +- dictionary check: ensure at least one bundled Yomitan dictionary is available, unless an external Yomitan profile is configured - Windows: optionally create or remove `SubMiner mpv` Start Menu/Desktop shortcuts (`SubMiner.exe --launch-mpv`) - refresh: re-check plugin + dictionary state without restarting -- `Finish setup` stays disabled until the mpv plugin is installed and dictionary availability is detected +- `Finish setup` stays disabled until the config, plugin, and dictionary gates are satisfied - finish action writes setup completion state and suppresses future auto-open prompts AniList character dictionary auto-sync (optional): diff --git a/launcher/mpv.test.ts b/launcher/mpv.test.ts index 3e8a2df2..2eb6ee77 100644 --- a/launcher/mpv.test.ts +++ b/launcher/mpv.test.ts @@ -427,11 +427,14 @@ function withFindAppBinaryEnvSandbox(run: () => void): void { } } -function withFindAppBinaryPlatformSandbox(platform: NodeJS.Platform, run: () => void): void { +function withFindAppBinaryPlatformSandbox( + platform: NodeJS.Platform, + run: (pathModule: typeof path) => void, +): void { const originalPlatform = process.platform; try { Object.defineProperty(process, 'platform', { value: platform, configurable: true }); - withFindAppBinaryEnvSandbox(run); + withFindAppBinaryEnvSandbox(() => run(platform === 'win32' ? (path.win32 as typeof path) : path)); } finally { Object.defineProperty(process, 'platform', { value: originalPlatform, configurable: true }); } @@ -457,7 +460,7 @@ function withAccessSyncStub( } } -test('findAppBinary resolves ~/.local/bin/SubMiner.AppImage when it exists', () => { +test('findAppBinary resolves ~/.local/bin/SubMiner.AppImage when it exists', { concurrency: false }, () => { const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-test-home-')); const originalHomedir = os.homedir; try { @@ -465,8 +468,8 @@ test('findAppBinary resolves ~/.local/bin/SubMiner.AppImage when it exists', () const appImage = path.join(baseDir, '.local/bin/SubMiner.AppImage'); makeExecutable(appImage); - withFindAppBinaryPlatformSandbox('linux', () => { - const result = findAppBinary('/some/other/path/subminer'); + withFindAppBinaryPlatformSandbox('linux', (pathModule) => { + const result = findAppBinary('/some/other/path/subminer', pathModule); assert.equal(result, appImage); }); } finally { @@ -475,16 +478,16 @@ test('findAppBinary resolves ~/.local/bin/SubMiner.AppImage when it exists', () } }); -test('findAppBinary resolves /opt/SubMiner/SubMiner.AppImage when ~/.local/bin candidate does not exist', () => { +test('findAppBinary resolves /opt/SubMiner/SubMiner.AppImage when ~/.local/bin candidate does not exist', { concurrency: false }, () => { const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-test-home-')); const originalHomedir = os.homedir; try { os.homedir = () => baseDir; - withFindAppBinaryPlatformSandbox('linux', () => { + withFindAppBinaryPlatformSandbox('linux', (pathModule) => { withAccessSyncStub( (filePath) => filePath === '/opt/SubMiner/SubMiner.AppImage', () => { - const result = findAppBinary('/some/other/path/subminer'); + const result = findAppBinary('/some/other/path/subminer', pathModule); assert.equal(result, '/opt/SubMiner/SubMiner.AppImage'); }, ); @@ -495,7 +498,7 @@ test('findAppBinary resolves /opt/SubMiner/SubMiner.AppImage when ~/.local/bin c } }); -test('findAppBinary finds subminer on PATH when AppImage candidates do not exist', () => { +test('findAppBinary finds subminer on PATH when AppImage candidates do not exist', { concurrency: false }, () => { const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-test-path-')); const originalHomedir = os.homedir; const originalPath = process.env.PATH; @@ -507,12 +510,12 @@ test('findAppBinary finds subminer on PATH when AppImage candidates do not exist makeExecutable(wrapperPath); process.env.PATH = `${binDir}${path.delimiter}${originalPath ?? ''}`; - withFindAppBinaryPlatformSandbox('linux', () => { + withFindAppBinaryPlatformSandbox('linux', (pathModule) => { withAccessSyncStub( (filePath) => filePath === wrapperPath, () => { // selfPath must differ from wrapperPath so the self-check does not exclude it - const result = findAppBinary(path.join(baseDir, 'launcher', 'subminer')); + const result = findAppBinary(path.join(baseDir, 'launcher', 'subminer'), pathModule); assert.equal(result, wrapperPath); }, ); @@ -524,20 +527,27 @@ test('findAppBinary finds subminer on PATH when AppImage candidates do not exist } }); -test('findAppBinary resolves Windows install paths when present', () => { +test('findAppBinary resolves Windows install paths when present', { concurrency: false }, () => { const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-test-win-')); const originalHomedir = os.homedir; const originalLocalAppData = process.env.LOCALAPPDATA; try { os.homedir = () => baseDir; - process.env.LOCALAPPDATA = path.join(baseDir, 'AppData', 'Local'); - const appExe = path.join(baseDir, 'AppData', 'Local', 'Programs', 'SubMiner', 'SubMiner.exe'); + process.env.LOCALAPPDATA = path.win32.join(baseDir, 'AppData', 'Local'); + const appExe = path.win32.join( + baseDir, + 'AppData', + 'Local', + 'Programs', + 'SubMiner', + 'SubMiner.exe', + ); - withFindAppBinaryPlatformSandbox('win32', () => { + withFindAppBinaryPlatformSandbox('win32', (pathModule) => { withAccessSyncStub( (filePath) => filePath === appExe, () => { - const result = findAppBinary(path.join(baseDir, 'launcher', 'SubMiner.exe')); + const result = findAppBinary(pathModule.join(baseDir, 'launcher', 'SubMiner.exe'), pathModule); assert.equal(result, appExe); }, ); @@ -553,22 +563,22 @@ test('findAppBinary resolves Windows install paths when present', () => { } }); -test('findAppBinary resolves SubMiner.exe on PATH on Windows', () => { +test('findAppBinary resolves SubMiner.exe on PATH on Windows', { concurrency: false }, () => { const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-test-win-path-')); const originalHomedir = os.homedir; const originalPath = process.env.PATH; try { os.homedir = () => baseDir; - const binDir = path.join(baseDir, 'bin'); - const wrapperPath = path.join(binDir, 'SubMiner.exe'); + const binDir = path.win32.join(baseDir, 'bin'); + const wrapperPath = path.win32.join(binDir, 'SubMiner.exe'); makeExecutable(wrapperPath); - process.env.PATH = `${binDir}${path.delimiter}${originalPath ?? ''}`; + process.env.PATH = `${binDir}${path.win32.delimiter}${originalPath ?? ''}`; - withFindAppBinaryPlatformSandbox('win32', () => { + withFindAppBinaryPlatformSandbox('win32', (pathModule) => { withAccessSyncStub( (filePath) => filePath === wrapperPath, () => { - const result = findAppBinary(path.join(baseDir, 'launcher', 'SubMiner.exe')); + const result = findAppBinary(pathModule.join(baseDir, 'launcher', 'SubMiner.exe'), pathModule); assert.equal(result, wrapperPath); }, ); @@ -579,3 +589,35 @@ test('findAppBinary resolves SubMiner.exe on PATH on Windows', () => { fs.rmSync(baseDir, { recursive: true, force: true }); } }); + +test('findAppBinary resolves a Windows install directory to SubMiner.exe', { concurrency: false }, () => { + const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-test-win-dir-')); + const originalHomedir = os.homedir; + const originalSubminerBinaryPath = process.env.SUBMINER_BINARY_PATH; + try { + os.homedir = () => baseDir; + const installDir = path.win32.join(baseDir, 'Programs', 'SubMiner'); + const appExe = path.win32.join(installDir, 'SubMiner.exe'); + process.env.SUBMINER_BINARY_PATH = installDir; + fs.mkdirSync(installDir, { recursive: true }); + fs.writeFileSync(appExe, '#!/bin/sh\nexit 0\n'); + fs.chmodSync(appExe, 0o755); + + const originalPlatform = process.platform; + try { + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + const result = findAppBinary(path.win32.join(baseDir, 'launcher', 'SubMiner.exe'), path.win32); + assert.equal(result, appExe); + } finally { + Object.defineProperty(process, 'platform', { value: originalPlatform, configurable: true }); + } + } finally { + os.homedir = originalHomedir; + if (originalSubminerBinaryPath === undefined) { + delete process.env.SUBMINER_BINARY_PATH; + } else { + process.env.SUBMINER_BINARY_PATH = originalSubminerBinaryPath; + } + fs.rmSync(baseDir, { recursive: true, force: true }); + } +}); diff --git a/launcher/mpv.ts b/launcher/mpv.ts index fde57c74..7c4e4330 100644 --- a/launcher/mpv.ts +++ b/launcher/mpv.ts @@ -14,7 +14,6 @@ import { isExecutable, resolveBinaryPathCandidate, resolveCommandInvocation, - realpathMaybe, isYoutubeTarget, uniqueNormalizedLangCodes, sleep, @@ -35,6 +34,8 @@ type SpawnTarget = { args: string[]; }; +type PathModule = Pick; + const DETACHED_IDLE_MPV_PID_FILE = path.join(os.tmpdir(), 'subminer-idle-mpv.pid'); const OVERLAY_START_SOCKET_READY_TIMEOUT_MS = 900; const OVERLAY_START_COMMAND_SETTLE_TIMEOUT_MS = 700; @@ -243,29 +244,30 @@ export function detectBackend(backend: Backend): Exclude { fail('Could not detect display backend'); } -function resolveAppBinaryCandidate(candidate: string): string { +function resolveAppBinaryCandidate(candidate: string, pathModule: PathModule = path): string { const direct = resolveBinaryPathCandidate(candidate); if (!direct) return ''; - if (isExecutable(direct)) { - return direct; - } - if (process.platform === 'win32') { try { if (fs.existsSync(direct) && fs.statSync(direct).isDirectory()) { for (const candidateBinary of ['SubMiner.exe', 'subminer.exe']) { - const nestedCandidate = path.join(direct, candidateBinary); + const nestedCandidate = pathModule.join(direct, candidateBinary); if (isExecutable(nestedCandidate)) { return nestedCandidate; } } + return ''; } } catch { // ignore } - if (!path.extname(direct)) { + if (isExecutable(direct)) { + return direct; + } + + if (!pathModule.extname(direct)) { for (const extension of ['.exe', '.cmd', '.bat']) { const withExtension = `${direct}${extension}`; if (isExecutable(withExtension)) { @@ -277,6 +279,10 @@ function resolveAppBinaryCandidate(candidate: string): string { return ''; } + if (isExecutable(direct)) { + return direct; + } + if (process.platform !== 'darwin') { return ''; } @@ -291,8 +297,8 @@ function resolveAppBinaryCandidate(candidate: string): string { if (!appPath) return ''; const candidates = [ - path.join(appPath, 'Contents', 'MacOS', 'SubMiner'), - path.join(appPath, 'Contents', 'MacOS', 'subminer'), + pathModule.join(appPath, 'Contents', 'MacOS', 'SubMiner'), + pathModule.join(appPath, 'Contents', 'MacOS', 'subminer'), ]; for (const candidateBinary of candidates) { @@ -304,20 +310,20 @@ function resolveAppBinaryCandidate(candidate: string): string { return ''; } -function findCommandOnPath(candidates: string[]): string { - const pathDirs = getPathEnv().split(path.delimiter); +function findCommandOnPath(candidates: string[], pathModule: PathModule = path): string { + const pathDirs = getPathEnv().split(pathModule.delimiter); for (const candidateName of candidates) { for (const dir of pathDirs) { if (!dir) continue; - const directCandidate = path.join(dir, candidateName); + const directCandidate = pathModule.join(dir, candidateName); if (isExecutable(directCandidate)) { return directCandidate; } - if (process.platform === 'win32' && !path.extname(candidateName)) { + if (process.platform === 'win32' && !pathModule.extname(candidateName)) { for (const extension of ['.exe', '.cmd', '.bat']) { - const extendedCandidate = path.join(dir, `${candidateName}${extension}`); + const extendedCandidate = pathModule.join(dir, `${candidateName}${extension}`); if (isExecutable(extendedCandidate)) { return extendedCandidate; } @@ -329,13 +335,13 @@ function findCommandOnPath(candidates: string[]): string { return ''; } -export function findAppBinary(selfPath: string): string | null { +export function findAppBinary(selfPath: string, pathModule: PathModule = path): string | null { const envPaths = [process.env.SUBMINER_APPIMAGE_PATH, process.env.SUBMINER_BINARY_PATH].filter( (candidate): candidate is string => Boolean(candidate), ); for (const envPath of envPaths) { - const resolved = resolveAppBinaryCandidate(envPath); + const resolved = resolveAppBinaryCandidate(envPath, pathModule); if (resolved) { return resolved; } @@ -345,36 +351,37 @@ export function findAppBinary(selfPath: string): string | null { if (process.platform === 'win32') { const localAppData = process.env.LOCALAPPDATA?.trim() || - (process.env.APPDATA?.trim() || '').replace(/[\\/]Roaming$/i, `${path.sep}Local`) || - path.join(os.homedir(), 'AppData', 'Local'); + (process.env.APPDATA?.trim() || '').replace(/[\\/]Roaming$/i, `${pathModule.sep}Local`) || + pathModule.join(os.homedir(), 'AppData', 'Local'); const programFiles = process.env.ProgramFiles?.trim() || 'C:\\Program Files'; const programFilesX86 = process.env['ProgramFiles(x86)']?.trim() || 'C:\\Program Files (x86)'; - candidates.push(path.join(localAppData, 'Programs', 'SubMiner', 'SubMiner.exe')); - candidates.push(path.join(programFiles, 'SubMiner', 'SubMiner.exe')); - candidates.push(path.join(programFilesX86, 'SubMiner', 'SubMiner.exe')); + candidates.push(pathModule.join(localAppData, 'Programs', 'SubMiner', 'SubMiner.exe')); + candidates.push(pathModule.join(programFiles, 'SubMiner', 'SubMiner.exe')); + candidates.push(pathModule.join(programFilesX86, 'SubMiner', 'SubMiner.exe')); candidates.push('C:\\SubMiner\\SubMiner.exe'); } else if (process.platform === 'darwin') { candidates.push('/Applications/SubMiner.app/Contents/MacOS/SubMiner'); candidates.push('/Applications/SubMiner.app/Contents/MacOS/subminer'); - candidates.push(path.join(os.homedir(), 'Applications/SubMiner.app/Contents/MacOS/SubMiner')); - candidates.push(path.join(os.homedir(), 'Applications/SubMiner.app/Contents/MacOS/subminer')); + candidates.push(pathModule.join(os.homedir(), 'Applications/SubMiner.app/Contents/MacOS/SubMiner')); + candidates.push(pathModule.join(os.homedir(), 'Applications/SubMiner.app/Contents/MacOS/subminer')); } else { - candidates.push(path.join(os.homedir(), '.local/bin/SubMiner.AppImage')); + candidates.push(pathModule.join(os.homedir(), '.local/bin/SubMiner.AppImage')); candidates.push('/opt/SubMiner/SubMiner.AppImage'); } for (const candidate of candidates) { - const resolved = resolveAppBinaryCandidate(candidate); + const resolved = resolveAppBinaryCandidate(candidate, pathModule); if (resolved) return resolved; } const fromPath = findCommandOnPath( process.platform === 'win32' ? ['SubMiner', 'subminer'] : ['subminer'], + pathModule, ); if (fromPath) { - const resolvedSelf = realpathMaybe(selfPath); - const resolvedCandidate = realpathMaybe(fromPath); + const resolvedSelf = pathModule.resolve(selfPath); + const resolvedCandidate = pathModule.resolve(fromPath); if (resolvedSelf !== resolvedCandidate) return fromPath; } diff --git a/src/main-entry-runtime.test.ts b/src/main-entry-runtime.test.ts index df535e45..f50900bc 100644 --- a/src/main-entry-runtime.test.ts +++ b/src/main-entry-runtime.test.ts @@ -71,6 +71,26 @@ test('launch-mpv entry helpers detect and normalize targets', () => { assert.deepEqual(normalizeLaunchMpvTargets(['SubMiner.exe', '--launch-mpv', 'C:\\a.mkv']), [ 'C:\\a.mkv', ]); + assert.deepEqual( + normalizeLaunchMpvExtraArgs([ + 'SubMiner.exe', + '--launch-mpv', + '--sub-file', + 'track.srt', + 'C:\\a.mkv', + ]), + ['--sub-file', 'track.srt'], + ); + assert.deepEqual( + normalizeLaunchMpvTargets([ + 'SubMiner.exe', + '--launch-mpv', + '--sub-file', + 'track.srt', + 'C:\\a.mkv', + ]), + ['C:\\a.mkv'], + ); assert.deepEqual( normalizeLaunchMpvExtraArgs([ 'SubMiner.exe', diff --git a/src/main-entry-runtime.ts b/src/main-entry-runtime.ts index a8695e3b..6b7eb101 100644 --- a/src/main-entry-runtime.ts +++ b/src/main-entry-runtime.ts @@ -127,18 +127,6 @@ export function normalizeLaunchMpvTargets(argv: string[]): string[] { } const targets: string[] = []; - const flagValueArgs = new Set([ - '--alang', - '--input-ipc-server', - '--log-file', - '--profile', - '--script', - '--script-opts', - '--scripts', - '--slang', - '--sub-file-paths', - '--ytdl-format', - ]); let parsingTargets = false; for (let i = launchMpvIndex + 1; i < argv.length; i += 1) { @@ -155,13 +143,20 @@ export function normalizeLaunchMpvTargets(argv: string[]): string[] { continue; } - if (token.startsWith('-')) { - if (!token.includes('=') && flagValueArgs.has(token)) { - i += 1; + if (token.startsWith('--')) { + if (!token.includes('=') && i + 1 < argv.length) { + const value = argv[i + 1]; + if (value && !value.startsWith('-')) { + i += 1; + } } continue; } + if (token.startsWith('-')) { + continue; + } + parsingTargets = true; targets.push(token); } @@ -175,19 +170,6 @@ export function normalizeLaunchMpvExtraArgs(argv: string[]): string[] { return []; } - const flagValueArgs = new Set([ - '--alang', - '--input-ipc-server', - '--log-file', - '--profile', - '--script', - '--script-opts', - '--scripts', - '--slang', - '--sub-file-paths', - '--ytdl-format', - ]); - const extraArgs: string[] = []; for (let i = launchMpvIndex + 1; i < argv.length; i += 1) { const token = argv[i]; @@ -195,18 +177,24 @@ export function normalizeLaunchMpvExtraArgs(argv: string[]): string[] { if (token === '--') { break; } + if (token.startsWith('--')) { + extraArgs.push(token); + if (!token.includes('=') && i + 1 < argv.length) { + const value = argv[i + 1]; + if (value && !value.startsWith('-')) { + extraArgs.push(value); + i += 1; + } + } + continue; + } + if (token.startsWith('-')) { + extraArgs.push(token); + continue; + } if (!token.startsWith('-')) { break; } - - extraArgs.push(token); - if (!token.includes('=') && flagValueArgs.has(token)) { - const value = argv[i + 1]; - if (value && value !== '--') { - extraArgs.push(value); - i += 1; - } - } } return extraArgs; } diff --git a/src/main/runtime/first-run-setup-service.ts b/src/main/runtime/first-run-setup-service.ts index 42af4aee..4accf574 100644 --- a/src/main/runtime/first-run-setup-service.ts +++ b/src/main/runtime/first-run-setup-service.ts @@ -149,6 +149,24 @@ function isYomitanSetupSatisfied(options: { return options.externalYomitanConfigured || options.dictionaryCount >= 1; } +export function getFirstRunSetupCompletionMessage(snapshot: { + configReady: boolean; + dictionaryCount: number; + externalYomitanConfigured: boolean; + pluginStatus: SetupStatusSnapshot['pluginStatus']; +}): string | null { + if (!snapshot.configReady) { + return 'Create or provide the config file before finishing setup.'; + } + if (snapshot.pluginStatus !== 'installed') { + return 'Install the mpv plugin before finishing setup.'; + } + if (!snapshot.externalYomitanConfigured && snapshot.dictionaryCount < 1) { + return 'Install at least one Yomitan dictionary before finishing setup.'; + } + return null; +} + async function resolveYomitanSetupStatus(deps: { configFilePaths: { jsoncPath: string; jsonPath: string }; getYomitanDictionaryCount: () => Promise; diff --git a/src/main/runtime/first-run-setup-window.test.ts b/src/main/runtime/first-run-setup-window.test.ts index 9eb07d77..6278b7c7 100644 --- a/src/main/runtime/first-run-setup-window.test.ts +++ b/src/main/runtime/first-run-setup-window.test.ts @@ -55,6 +55,32 @@ test('buildFirstRunSetupHtml switches plugin action to reinstall when already in }); assert.match(html, /Reinstall mpv plugin/); + assert.match( + html, + /Finish stays unlocked once the mpv plugin is installed and Yomitan reports at least one installed dictionary\./, + ); +}); + +test('buildFirstRunSetupHtml explains the config blocker when setup is missing config', () => { + const html = buildFirstRunSetupHtml({ + configReady: false, + dictionaryCount: 0, + canFinish: false, + externalYomitanConfigured: false, + pluginStatus: 'required', + pluginInstallPathSummary: null, + windowsMpvShortcuts: { + supported: false, + startMenuEnabled: true, + desktopEnabled: true, + startMenuInstalled: false, + desktopInstalled: false, + status: 'optional', + }, + message: null, + }); + + assert.match(html, /Create or provide the config file before finishing setup\./); }); test('buildFirstRunSetupHtml explains external yomitan mode and keeps finish enabled', () => { @@ -120,6 +146,25 @@ test('first-run setup navigation handler prevents default and dispatches action' assert.deepEqual(calls, ['preventDefault', 'install-plugin']); }); +test('first-run setup navigation handler swallows stale custom-scheme actions', () => { + const calls: string[] = []; + const handleNavigation = createHandleFirstRunSetupNavigationHandler({ + parseSubmissionUrl: (url) => parseFirstRunSetupSubmissionUrl(url), + handleAction: async (submission) => { + calls.push(submission.action); + }, + logError: (message) => calls.push(message), + }); + + const prevented = handleNavigation({ + url: 'subminer://first-run-setup?action=skip-plugin', + preventDefault: () => calls.push('preventDefault'), + }); + + assert.equal(prevented, true); + assert.deepEqual(calls, ['preventDefault']); +}); + test('closing incomplete first-run setup quits app outside background mode', async () => { const calls: string[] = []; let closedHandler: (() => void) | undefined; diff --git a/src/main/runtime/first-run-setup-window.ts b/src/main/runtime/first-run-setup-window.ts index dbcc9626..401c6c1d 100644 --- a/src/main/runtime/first-run-setup-window.ts +++ b/src/main/runtime/first-run-setup-window.ts @@ -1,3 +1,5 @@ +import { getFirstRunSetupCompletionMessage } from './first-run-setup-service'; + type FocusableWindowLike = { focus: () => void; }; @@ -123,11 +125,14 @@ export function buildFirstRunSetupHtml(model: FirstRunSetupHtmlModel): string { : model.dictionaryCount >= 1 ? 'ready' : 'warn'; - const footerMessage = model.externalYomitanConfigured - ? model.pluginStatus === 'installed' - ? 'Finish stays unlocked while SubMiner is reusing an external Yomitan profile. If you later launch without yomitan.externalProfilePath, setup will require at least one internal dictionary.' - : 'Finish stays locked until the mpv plugin is installed. If you later launch without yomitan.externalProfilePath, setup will also require at least one internal dictionary.' - : 'Finish stays locked until the mpv plugin is installed and Yomitan reports at least one installed dictionary.'; + const blockerMessage = getFirstRunSetupCompletionMessage(model); + const footerMessage = blockerMessage + ? blockerMessage + : model.canFinish + ? model.externalYomitanConfigured + ? 'Finish stays unlocked while SubMiner is reusing an external Yomitan profile. If you later launch without yomitan.externalProfilePath, setup will require at least one internal dictionary.' + : 'Finish stays unlocked once the mpv plugin is installed and Yomitan reports at least one installed dictionary.' + : 'Finish stays locked until the mpv plugin is installed and Yomitan reports at least one installed dictionary.'; return ` @@ -333,9 +338,17 @@ export function createHandleFirstRunSetupNavigationHandler(deps: { logError: (message: string, error: unknown) => void; }) { return (params: { url: string; preventDefault: () => void }): boolean => { - const submission = deps.parseSubmissionUrl(params.url); - if (!submission) return false; + if (!params.url.startsWith('subminer://first-run-setup')) { + return false; + } params.preventDefault(); + let submission: FirstRunSetupSubmission | null; + try { + submission = deps.parseSubmissionUrl(params.url); + } catch { + return true; + } + if (!submission) return true; void deps.handleAction(submission).catch((error) => { deps.logError('Failed handling first-run setup action', error); }); diff --git a/src/main/runtime/windows-mpv-launch.test.ts b/src/main/runtime/windows-mpv-launch.test.ts index 0af9b271..b1f39f17 100644 --- a/src/main/runtime/windows-mpv-launch.test.ts +++ b/src/main/runtime/windows-mpv-launch.test.ts @@ -49,21 +49,22 @@ test('buildWindowsMpvLaunchArgs uses explicit SubMiner defaults and targets', () 'C:\\Program Files\\SubMiner\\resources\\plugin\\subminer\\main.lua', ), [ - '--player-operation-mode=pseudo-gui', - '--force-window=immediate', - '--script=C:\\Program Files\\SubMiner\\resources\\plugin\\subminer\\main.lua', - '--input-ipc-server=\\\\.\\pipe\\subminer-socket', - '--alang=ja,jp,jpn,japanese,en,eng,english,enus,en-us', - '--slang=ja,jp,jpn,japanese,en,eng,english,enus,en-us', - '--sub-auto=fuzzy', - '--sub-file-paths=subs;subtitles', - '--sid=auto', - '--secondary-sid=auto', - '--secondary-sub-visibility=no', - '--script-opts=subminer-binary_path=C:\\SubMiner\\SubMiner.exe,subminer-socket_path=\\\\.\\pipe\\subminer-socket', - 'C:\\a.mkv', - 'C:\\b.mkv', - ]); + '--player-operation-mode=pseudo-gui', + '--force-window=immediate', + '--script=C:\\Program Files\\SubMiner\\resources\\plugin\\subminer\\main.lua', + '--input-ipc-server=\\\\.\\pipe\\subminer-socket', + '--alang=ja,jp,jpn,japanese,en,eng,english,enus,en-us', + '--slang=ja,jp,jpn,japanese,en,eng,english,enus,en-us', + '--sub-auto=fuzzy', + '--sub-file-paths=subs;subtitles', + '--sid=auto', + '--secondary-sid=auto', + '--secondary-sub-visibility=no', + '--script-opts=subminer-binary_path=C:\\SubMiner\\SubMiner.exe,subminer-socket_path=\\\\.\\pipe\\subminer-socket', + 'C:\\a.mkv', + 'C:\\b.mkv', + ], + ); }); test('buildWindowsMpvLaunchArgs keeps shortcut-only launches in idle mode', () => { @@ -92,6 +93,34 @@ test('buildWindowsMpvLaunchArgs keeps shortcut-only launches in idle mode', () = ); }); +test('buildWindowsMpvLaunchArgs mirrors a custom input-ipc-server into script opts', () => { + assert.deepEqual( + buildWindowsMpvLaunchArgs( + ['C:\\video.mkv'], + ['--input-ipc-server', '\\\\.\\pipe\\custom-subminer-socket'], + 'C:\\SubMiner\\SubMiner.exe', + 'C:\\Program Files\\SubMiner\\resources\\plugin\\subminer\\main.lua', + ), + [ + '--player-operation-mode=pseudo-gui', + '--force-window=immediate', + '--script=C:\\Program Files\\SubMiner\\resources\\plugin\\subminer\\main.lua', + '--input-ipc-server=\\\\.\\pipe\\custom-subminer-socket', + '--alang=ja,jp,jpn,japanese,en,eng,english,enus,en-us', + '--slang=ja,jp,jpn,japanese,en,eng,english,enus,en-us', + '--sub-auto=fuzzy', + '--sub-file-paths=subs;subtitles', + '--sid=auto', + '--secondary-sid=auto', + '--secondary-sub-visibility=no', + '--script-opts=subminer-binary_path=C:\\SubMiner\\SubMiner.exe,subminer-socket_path=\\\\.\\pipe\\custom-subminer-socket', + '--input-ipc-server', + '\\\\.\\pipe\\custom-subminer-socket', + 'C:\\video.mkv', + ], + ); +}); + test('launchWindowsMpv reports missing mpv path', () => { const errors: string[] = []; const result = launchWindowsMpv( diff --git a/src/main/runtime/windows-mpv-launch.ts b/src/main/runtime/windows-mpv-launch.ts index e6970f4e..44a71be3 100644 --- a/src/main/runtime/windows-mpv-launch.ts +++ b/src/main/runtime/windows-mpv-launch.ts @@ -33,6 +33,27 @@ export function resolveWindowsMpvPath(deps: WindowsMpvLaunchDeps): string { return ''; } +const DEFAULT_WINDOWS_MPV_SOCKET = '\\\\.\\pipe\\subminer-socket'; + +function readExtraArgValue(extraArgs: string[], flag: string): string | undefined { + let value: string | undefined; + for (let i = 0; i < extraArgs.length; i += 1) { + const arg = extraArgs[i]; + if (arg === flag) { + const next = extraArgs[i + 1]; + if (next && !next.startsWith('-')) { + value = next; + i += 1; + } + continue; + } + if (arg?.startsWith(`${flag}=`)) { + value = arg.slice(flag.length + 1); + } + } + return value; +} + export function buildWindowsMpvLaunchArgs( targets: string[], extraArgs: string[] = [], @@ -40,9 +61,11 @@ export function buildWindowsMpvLaunchArgs( pluginEntrypointPath?: string, ): string[] { const launchIdle = targets.length === 0; + const inputIpcServer = + readExtraArgValue(extraArgs, '--input-ipc-server') ?? DEFAULT_WINDOWS_MPV_SOCKET; const scriptOpts = typeof binaryPath === 'string' && binaryPath.trim().length > 0 - ? `--script-opts=subminer-binary_path=${binaryPath.trim().replace(/,/g, '\\,')},subminer-socket_path=\\\\.\\pipe\\subminer-socket` + ? `--script-opts=subminer-binary_path=${binaryPath.trim().replace(/,/g, '\\,')},subminer-socket_path=${inputIpcServer.replace(/,/g, '\\,')}` : null; const scriptEntrypoint = typeof pluginEntrypointPath === 'string' && pluginEntrypointPath.trim().length > 0 @@ -54,7 +77,7 @@ export function buildWindowsMpvLaunchArgs( '--force-window=immediate', ...(launchIdle ? ['--idle=yes'] : []), ...(scriptEntrypoint ? [scriptEntrypoint] : []), - '--input-ipc-server=\\\\.\\pipe\\subminer-socket', + `--input-ipc-server=${inputIpcServer}`, '--alang=ja,jp,jpn,japanese,en,eng,english,enus,en-us', '--slang=ja,jp,jpn,japanese,en,eng,english,enus,en-us', '--sub-auto=fuzzy',