fix windows launcher review feedback

This commit is contained in:
2026-04-03 13:04:01 -07:00
parent ea22fda16b
commit 2d01dc097b
8 changed files with 142 additions and 40 deletions

View File

@@ -91,8 +91,8 @@ if (shouldHandleHelpOnlyAtEntry(process.argv, process.env)) {
if (shouldHandleLaunchMpvAtEntry(process.argv, process.env)) { if (shouldHandleLaunchMpvAtEntry(process.argv, process.env)) {
const sanitizedEnv = sanitizeLaunchMpvEnv(process.env); const sanitizedEnv = sanitizeLaunchMpvEnv(process.env);
applySanitizedEnv(sanitizedEnv); applySanitizedEnv(sanitizedEnv);
void app.whenReady().then(() => { void app.whenReady().then(async () => {
const result = launchWindowsMpv( const result = await launchWindowsMpv(
normalizeLaunchMpvTargets(process.argv), normalizeLaunchMpvTargets(process.argv),
createWindowsMpvLaunchDeps({ createWindowsMpvLaunchDeps({
getEnv: (name) => process.env[name], getEnv: (name) => process.env[name],

View File

@@ -367,7 +367,11 @@ import {
detectWindowsMpvShortcuts, detectWindowsMpvShortcuts,
resolveWindowsMpvShortcutPaths, resolveWindowsMpvShortcutPaths,
} from './main/runtime/windows-mpv-shortcuts'; } from './main/runtime/windows-mpv-shortcuts';
import { createWindowsMpvLaunchDeps, launchWindowsMpv } from './main/runtime/windows-mpv-launch'; import {
createWindowsMpvLaunchDeps,
getConfiguredWindowsMpvPathStatus,
launchWindowsMpv,
} from './main/runtime/windows-mpv-launch';
import { createWaitForMpvConnectedHandler } from './main/runtime/jellyfin-remote-connection'; import { createWaitForMpvConnectedHandler } from './main/runtime/jellyfin-remote-connection';
import { createPrepareYoutubePlaybackInMpvHandler } from './main/runtime/youtube-playback-launch'; import { createPrepareYoutubePlaybackInMpvHandler } from './main/runtime/youtube-playback-launch';
import { shouldEnsureTrayOnStartupForInitialArgs } from './main/runtime/startup-tray-policy'; import { shouldEnsureTrayOnStartupForInitialArgs } from './main/runtime/startup-tray-policy';
@@ -2216,6 +2220,7 @@ const openFirstRunSetupWindowHandler = createOpenFirstRunSetupWindowHandler({
}), }),
getSetupSnapshot: async () => { getSetupSnapshot: async () => {
const snapshot = await firstRunSetupService.getSetupStatus(); const snapshot = await firstRunSetupService.getSetupStatus();
const mpvExecutablePath = getResolvedConfig().mpv.executablePath;
return { return {
configReady: snapshot.configReady, configReady: snapshot.configReady,
dictionaryCount: snapshot.dictionaryCount, dictionaryCount: snapshot.dictionaryCount,
@@ -2223,7 +2228,8 @@ const openFirstRunSetupWindowHandler = createOpenFirstRunSetupWindowHandler({
externalYomitanConfigured: snapshot.externalYomitanConfigured, externalYomitanConfigured: snapshot.externalYomitanConfigured,
pluginStatus: snapshot.pluginStatus, pluginStatus: snapshot.pluginStatus,
pluginInstallPathSummary: snapshot.pluginInstallPathSummary, pluginInstallPathSummary: snapshot.pluginInstallPathSummary,
mpvExecutablePath: getResolvedConfig().mpv.executablePath, mpvExecutablePath,
mpvExecutablePathStatus: getConfiguredWindowsMpvPathStatus(mpvExecutablePath),
windowsMpvShortcuts: snapshot.windowsMpvShortcuts, windowsMpvShortcuts: snapshot.windowsMpvShortcuts,
message: firstRunSetupMessage, message: firstRunSetupMessage,
}; };

View File

@@ -17,6 +17,7 @@ test('buildFirstRunSetupHtml renders macchiato setup actions and disabled finish
pluginStatus: 'required', pluginStatus: 'required',
pluginInstallPathSummary: null, pluginInstallPathSummary: null,
mpvExecutablePath: '', mpvExecutablePath: '',
mpvExecutablePathStatus: 'blank',
windowsMpvShortcuts: { windowsMpvShortcuts: {
supported: false, supported: false,
startMenuEnabled: true, startMenuEnabled: true,
@@ -45,6 +46,7 @@ test('buildFirstRunSetupHtml switches plugin action to reinstall when already in
pluginStatus: 'installed', pluginStatus: 'installed',
pluginInstallPathSummary: '/tmp/mpv', pluginInstallPathSummary: '/tmp/mpv',
mpvExecutablePath: 'C:\\Program Files\\mpv\\mpv.exe', mpvExecutablePath: 'C:\\Program Files\\mpv\\mpv.exe',
mpvExecutablePathStatus: 'configured',
windowsMpvShortcuts: { windowsMpvShortcuts: {
supported: true, supported: true,
startMenuEnabled: true, startMenuEnabled: true,
@@ -65,6 +67,31 @@ test('buildFirstRunSetupHtml switches plugin action to reinstall when already in
); );
}); });
test('buildFirstRunSetupHtml marks an invalid configured mpv path as invalid', () => {
const html = buildFirstRunSetupHtml({
configReady: true,
dictionaryCount: 1,
canFinish: true,
externalYomitanConfigured: false,
pluginStatus: 'installed',
pluginInstallPathSummary: '/tmp/mpv',
mpvExecutablePath: 'C:\\Broken\\mpv.exe',
mpvExecutablePathStatus: 'invalid',
windowsMpvShortcuts: {
supported: true,
startMenuEnabled: true,
desktopEnabled: true,
startMenuInstalled: false,
desktopInstalled: false,
status: 'optional',
},
message: null,
});
assert.match(html, />Invalid</);
assert.match(html, /Current: C:\\Broken\\mpv\.exe \(invalid; file not found\)/);
});
test('buildFirstRunSetupHtml explains the config blocker when setup is missing config', () => { test('buildFirstRunSetupHtml explains the config blocker when setup is missing config', () => {
const html = buildFirstRunSetupHtml({ const html = buildFirstRunSetupHtml({
configReady: false, configReady: false,
@@ -74,6 +101,7 @@ test('buildFirstRunSetupHtml explains the config blocker when setup is missing c
pluginStatus: 'required', pluginStatus: 'required',
pluginInstallPathSummary: null, pluginInstallPathSummary: null,
mpvExecutablePath: '', mpvExecutablePath: '',
mpvExecutablePathStatus: 'blank',
windowsMpvShortcuts: { windowsMpvShortcuts: {
supported: false, supported: false,
startMenuEnabled: true, startMenuEnabled: true,
@@ -97,6 +125,7 @@ test('buildFirstRunSetupHtml explains external yomitan mode and keeps finish ena
pluginStatus: 'installed', pluginStatus: 'installed',
pluginInstallPathSummary: null, pluginInstallPathSummary: null,
mpvExecutablePath: '', mpvExecutablePath: '',
mpvExecutablePathStatus: 'blank',
windowsMpvShortcuts: { windowsMpvShortcuts: {
supported: false, supported: false,
startMenuEnabled: true, startMenuEnabled: true,
@@ -208,6 +237,7 @@ test('closing incomplete first-run setup quits app outside background mode', asy
pluginStatus: 'required', pluginStatus: 'required',
pluginInstallPathSummary: null, pluginInstallPathSummary: null,
mpvExecutablePath: '', mpvExecutablePath: '',
mpvExecutablePathStatus: 'blank',
windowsMpvShortcuts: { windowsMpvShortcuts: {
supported: false, supported: false,
startMenuEnabled: true, startMenuEnabled: true,

View File

@@ -39,6 +39,7 @@ export interface FirstRunSetupHtmlModel {
pluginStatus: 'installed' | 'required' | 'failed'; pluginStatus: 'installed' | 'required' | 'failed';
pluginInstallPathSummary: string | null; pluginInstallPathSummary: string | null;
mpvExecutablePath: string; mpvExecutablePath: string;
mpvExecutablePathStatus: 'blank' | 'configured' | 'invalid';
windowsMpvShortcuts: { windowsMpvShortcuts: {
supported: boolean; supported: boolean;
startMenuEnabled: boolean; startMenuEnabled: boolean;
@@ -94,8 +95,23 @@ export function buildFirstRunSetupHtml(model: FirstRunSetupHtmlModel): string {
? 'muted' ? 'muted'
: 'warn'; : 'warn';
const mpvExecutablePathLabel = const mpvExecutablePathLabel =
model.mpvExecutablePath.trim().length > 0 ? 'Configured' : 'Blank'; model.mpvExecutablePathStatus === 'configured'
const mpvExecutablePathTone = model.mpvExecutablePath.trim().length > 0 ? 'ready' : 'muted'; ? 'Configured'
: model.mpvExecutablePathStatus === 'invalid'
? 'Invalid'
: 'Blank';
const mpvExecutablePathTone =
model.mpvExecutablePathStatus === 'configured'
? 'ready'
: model.mpvExecutablePathStatus === 'invalid'
? 'danger'
: 'muted';
const mpvExecutablePathCurrent =
model.mpvExecutablePathStatus === 'blank'
? 'blank (PATH discovery)'
: model.mpvExecutablePathStatus === 'invalid'
? `${model.mpvExecutablePath} (invalid; file not found)`
: model.mpvExecutablePath;
const mpvExecutablePathCard = model.windowsMpvShortcuts.supported const mpvExecutablePathCard = model.windowsMpvShortcuts.supported
? ` ? `
<div class="card block"> <div class="card block">
@@ -103,7 +119,7 @@ export function buildFirstRunSetupHtml(model: FirstRunSetupHtmlModel): string {
<div> <div>
<strong>mpv executable path</strong> <strong>mpv executable path</strong>
<div class="meta">Leave blank to auto-discover mpv.exe from PATH.</div> <div class="meta">Leave blank to auto-discover mpv.exe from PATH.</div>
<div class="meta">Current: ${escapeHtml(model.mpvExecutablePath.trim().length > 0 ? model.mpvExecutablePath : 'blank (PATH discovery)')}</div> <div class="meta">Current: ${escapeHtml(mpvExecutablePathCurrent)}</div>
</div> </div>
${renderStatusBadge(mpvExecutablePathLabel, mpvExecutablePathTone)} ${renderStatusBadge(mpvExecutablePathLabel, mpvExecutablePathTone)}
</div> </div>

View File

@@ -12,7 +12,7 @@ function createDeps(overrides: Partial<WindowsMpvLaunchDeps> = {}): WindowsMpvLa
getEnv: () => undefined, getEnv: () => undefined,
runWhere: () => ({ status: 1, stdout: '' }), runWhere: () => ({ status: 1, stdout: '' }),
fileExists: () => false, fileExists: () => false,
spawnDetached: () => undefined, spawnDetached: async () => undefined,
showError: () => undefined, showError: () => undefined,
...overrides, ...overrides,
}; };
@@ -134,9 +134,9 @@ test('buildWindowsMpvLaunchArgs mirrors a custom input-ipc-server into script op
); );
}); });
test('launchWindowsMpv reports missing mpv path', () => { test('launchWindowsMpv reports missing mpv path', async () => {
const errors: string[] = []; const errors: string[] = [];
const result = launchWindowsMpv( const result = await launchWindowsMpv(
[], [],
createDeps({ createDeps({
showError: (_title, content) => errors.push(content), showError: (_title, content) => errors.push(content),
@@ -148,14 +148,14 @@ test('launchWindowsMpv reports missing mpv path', () => {
assert.match(errors[0] ?? '', /mpv\.executablePath/i); assert.match(errors[0] ?? '', /mpv\.executablePath/i);
}); });
test('launchWindowsMpv spawns detached mpv with targets', () => { test('launchWindowsMpv spawns detached mpv with targets', async () => {
const calls: string[] = []; const calls: string[] = [];
const result = launchWindowsMpv( const result = await launchWindowsMpv(
['C:\\video.mkv'], ['C:\\video.mkv'],
createDeps({ createDeps({
getEnv: (name) => (name === 'SUBMINER_MPV_PATH' ? 'C:\\mpv\\mpv.exe' : undefined), getEnv: (name) => (name === 'SUBMINER_MPV_PATH' ? 'C:\\mpv\\mpv.exe' : undefined),
fileExists: (candidate) => candidate === 'C:\\mpv\\mpv.exe', fileExists: (candidate) => candidate === 'C:\\mpv\\mpv.exe',
spawnDetached: (command, args) => { spawnDetached: async (command, args) => {
calls.push(command); calls.push(command);
calls.push(args.join('|')); calls.push(args.join('|'));
}, },
@@ -173,14 +173,14 @@ test('launchWindowsMpv spawns detached mpv with targets', () => {
]); ]);
}); });
test('launchWindowsMpv reports spawn failures with path context', () => { test('launchWindowsMpv reports spawn failures with path context', async () => {
const errors: string[] = []; const errors: string[] = [];
const result = launchWindowsMpv( const result = await launchWindowsMpv(
[], [],
createDeps({ createDeps({
getEnv: (name) => (name === 'SUBMINER_MPV_PATH' ? 'C:\\mpv\\mpv.exe' : undefined), getEnv: (name) => (name === 'SUBMINER_MPV_PATH' ? 'C:\\mpv\\mpv.exe' : undefined),
fileExists: (candidate) => candidate === 'C:\\mpv\\mpv.exe', fileExists: (candidate) => candidate === 'C:\\mpv\\mpv.exe',
spawnDetached: () => { spawnDetached: async () => {
throw new Error('spawn failed'); throw new Error('spawn failed');
}, },
showError: (_title, content) => errors.push(content), showError: (_title, content) => errors.push(content),
@@ -192,3 +192,21 @@ test('launchWindowsMpv reports spawn failures with path context', () => {
assert.match(errors[0] ?? '', /Failed to launch mpv/i); assert.match(errors[0] ?? '', /Failed to launch mpv/i);
assert.match(errors[0] ?? '', /C:\\mpv\\mpv\.exe/i); assert.match(errors[0] ?? '', /C:\\mpv\\mpv\.exe/i);
}); });
test('launchWindowsMpv reports async spawn failures with path context', async () => {
const errors: string[] = [];
const result = await launchWindowsMpv(
[],
createDeps({
getEnv: (name) => (name === 'SUBMINER_MPV_PATH' ? 'C:\\mpv\\mpv.exe' : undefined),
fileExists: (candidate) => candidate === 'C:\\mpv\\mpv.exe',
spawnDetached: () => Promise.reject(new Error('async spawn failed')),
showError: (_title, content) => errors.push(content),
}),
);
assert.equal(result.ok, false);
assert.equal(result.mpvPath, 'C:\\mpv\\mpv.exe');
assert.match(errors[0] ?? '', /Failed to launch mpv/i);
assert.match(errors[0] ?? '', /async spawn failed/i);
});

View File

@@ -5,23 +5,45 @@ export interface WindowsMpvLaunchDeps {
getEnv: (name: string) => string | undefined; getEnv: (name: string) => string | undefined;
runWhere: () => { status: number | null; stdout: string; error?: Error }; runWhere: () => { status: number | null; stdout: string; error?: Error };
fileExists: (candidate: string) => boolean; fileExists: (candidate: string) => boolean;
spawnDetached: (command: string, args: string[]) => void; spawnDetached: (command: string, args: string[]) => Promise<void>;
showError: (title: string, content: string) => void; showError: (title: string, content: string) => void;
} }
export type ConfiguredWindowsMpvPathStatus = 'blank' | 'configured' | 'invalid';
function normalizeCandidate(candidate: string | undefined): string { function normalizeCandidate(candidate: string | undefined): string {
return typeof candidate === 'string' ? candidate.trim() : ''; return typeof candidate === 'string' ? candidate.trim() : '';
} }
function defaultWindowsMpvFileExists(candidate: string): boolean {
try {
return fs.statSync(candidate).isFile();
} catch {
return false;
}
}
export function getConfiguredWindowsMpvPathStatus(
configuredMpvPath = '',
fileExists: (candidate: string) => boolean = defaultWindowsMpvFileExists,
): ConfiguredWindowsMpvPathStatus {
const configPath = normalizeCandidate(configuredMpvPath);
if (!configPath) {
return 'blank';
}
return fileExists(configPath) ? 'configured' : 'invalid';
}
export function resolveWindowsMpvPath( export function resolveWindowsMpvPath(
deps: WindowsMpvLaunchDeps, deps: WindowsMpvLaunchDeps,
configuredMpvPath = '', configuredMpvPath = '',
): string { ): string {
const configPath = normalizeCandidate(configuredMpvPath); const configPath = normalizeCandidate(configuredMpvPath);
if (configPath) { const configuredPathStatus = getConfiguredWindowsMpvPathStatus(configPath, deps.fileExists);
if (deps.fileExists(configPath)) { if (configuredPathStatus === 'configured') {
return configPath; return configPath;
} }
if (configuredPathStatus === 'invalid') {
return ''; return '';
} }
@@ -102,14 +124,14 @@ export function buildWindowsMpvLaunchArgs(
]; ];
} }
export function launchWindowsMpv( export async function launchWindowsMpv(
targets: string[], targets: string[],
deps: WindowsMpvLaunchDeps, deps: WindowsMpvLaunchDeps,
extraArgs: string[] = [], extraArgs: string[] = [],
binaryPath?: string, binaryPath?: string,
pluginEntrypointPath?: string, pluginEntrypointPath?: string,
configuredMpvPath?: string, configuredMpvPath?: string,
): { ok: boolean; mpvPath: string } { ): Promise<{ ok: boolean; mpvPath: string }> {
const normalizedConfiguredPath = normalizeCandidate(configuredMpvPath); const normalizedConfiguredPath = normalizeCandidate(configuredMpvPath);
const mpvPath = resolveWindowsMpvPath(deps, normalizedConfiguredPath); const mpvPath = resolveWindowsMpvPath(deps, normalizedConfiguredPath);
if (!mpvPath) { if (!mpvPath) {
@@ -123,7 +145,7 @@ export function launchWindowsMpv(
} }
try { try {
deps.spawnDetached( await deps.spawnDetached(
mpvPath, mpvPath,
buildWindowsMpvLaunchArgs(targets, extraArgs, binaryPath, pluginEntrypointPath), buildWindowsMpvLaunchArgs(targets, extraArgs, binaryPath, pluginEntrypointPath),
); );
@@ -155,21 +177,31 @@ export function createWindowsMpvLaunchDeps(options: {
}, },
fileExists: fileExists:
options.fileExists ?? options.fileExists ??
((candidate) => { defaultWindowsMpvFileExists,
spawnDetached: (command, args) =>
new Promise((resolve, reject) => {
try { try {
return fs.statSync(candidate).isFile();
} catch {
return false;
}
}),
spawnDetached: (command, args) => {
const child = spawn(command, args, { const child = spawn(command, args, {
detached: true, detached: true,
stdio: 'ignore', stdio: 'ignore',
windowsHide: true, windowsHide: true,
}); });
let settled = false;
child.once('error', (error) => {
if (settled) return;
settled = true;
reject(error);
});
child.once('spawn', () => {
if (settled) return;
settled = true;
child.unref(); child.unref();
}, resolve();
});
} catch (error) {
reject(error);
}
}),
showError: options.showError, showError: options.showError,
}; };
} }

View File

@@ -29,7 +29,7 @@ test('youtube playback runtime resets flow ownership after a successful run', as
resolveYoutubePlaybackUrl: async () => { resolveYoutubePlaybackUrl: async () => {
throw new Error('linux path should not resolve direct playback url'); throw new Error('linux path should not resolve direct playback url');
}, },
launchWindowsMpv: () => ({ ok: false }), launchWindowsMpv: async () => ({ ok: false }),
waitForYoutubeMpvConnected: async (timeoutMs) => { waitForYoutubeMpvConnected: async (timeoutMs) => {
calls.push(`wait-connected:${timeoutMs}`); calls.push(`wait-connected:${timeoutMs}`);
return true; return true;
@@ -105,7 +105,7 @@ test('youtube playback runtime resolves the socket path lazily for windows start
calls.push(`resolve:${url}:${format}`); calls.push(`resolve:${url}:${format}`);
return 'https://example.com/direct'; return 'https://example.com/direct';
}, },
launchWindowsMpv: (_playbackUrl, args) => { launchWindowsMpv: async (_playbackUrl, args) => {
calls.push(`launch:${args.join(' ')}`); calls.push(`launch:${args.join(' ')}`);
return { ok: true, mpvPath: '/usr/bin/mpv' }; return { ok: true, mpvPath: '/usr/bin/mpv' };
}, },

View File

@@ -17,7 +17,7 @@ export type YoutubePlaybackRuntimeDeps = {
setAppOwnedFlowInFlight: (next: boolean) => void; setAppOwnedFlowInFlight: (next: boolean) => void;
ensureYoutubePlaybackRuntimeReady: () => Promise<void>; ensureYoutubePlaybackRuntimeReady: () => Promise<void>;
resolveYoutubePlaybackUrl: (url: string, format: string) => Promise<string>; resolveYoutubePlaybackUrl: (url: string, format: string) => Promise<string>;
launchWindowsMpv: (playbackUrl: string, args: string[]) => LaunchResult; launchWindowsMpv: (playbackUrl: string, args: string[]) => Promise<LaunchResult>;
waitForYoutubeMpvConnected: (timeoutMs: number) => Promise<boolean>; waitForYoutubeMpvConnected: (timeoutMs: number) => Promise<boolean>;
prepareYoutubePlaybackInMpv: (request: { url: string }) => Promise<boolean>; prepareYoutubePlaybackInMpv: (request: { url: string }) => Promise<boolean>;
runYoutubePlaybackFlow: (request: { runYoutubePlaybackFlow: (request: {
@@ -77,7 +77,7 @@ export function createYoutubePlaybackRuntime(deps: YoutubePlaybackRuntimeDeps) {
if (deps.platform === 'win32' && !deps.getMpvConnected()) { if (deps.platform === 'win32' && !deps.getMpvConnected()) {
const socketPath = deps.getSocketPath(); const socketPath = deps.getSocketPath();
const launchResult = deps.launchWindowsMpv(playbackUrl, [ const launchResult = await deps.launchWindowsMpv(playbackUrl, [
'--pause=yes', '--pause=yes',
'--ytdl=yes', '--ytdl=yes',
`--ytdl-format=${deps.mpvYtdlFormat}`, `--ytdl-format=${deps.mpvYtdlFormat}`,