fix: address CodeRabbit follow-ups for PR #40

This commit is contained in:
2026-04-03 01:14:57 -07:00
parent 61ab1b76fc
commit bf06463bb3
14 changed files with 341 additions and 116 deletions

View File

@@ -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',

View File

@@ -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;
}

View File

@@ -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<number>;

View File

@@ -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;

View File

@@ -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 `<!doctype html>
<html>
@@ -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);
});

View File

@@ -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(

View File

@@ -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',