fix: address CodeRabbit review round 3

This commit is contained in:
2026-04-10 18:57:49 -07:00
parent 659f468bfb
commit aa6903d457
16 changed files with 305 additions and 39 deletions

View File

@@ -89,13 +89,20 @@ function M.create(ctx)
return nil
end
if type(key.code) ~= "string" then
return nil
end
if type(key.modifiers) ~= "table" then
return nil
end
local key_name = key_code_to_mpv_name(key.code)
if not key_name then
return nil
end
local parts = {}
for _, modifier in ipairs(key.modifiers or {}) do
for _, modifier in ipairs(key.modifiers) do
local mapped = MODIFIER_MAP[modifier]
if mapped then
parts[#parts + 1] = mapped
@@ -298,7 +305,6 @@ function M.create(ctx)
local previous_binding_names = state.session_binding_names
local next_binding_names = {}
state.session_binding_names = next_binding_names
local timeout_ms = tonumber(artifact.numericSelectionTimeoutMs) or 3000
for index, binding in ipairs(artifact.bindings) do
@@ -319,6 +325,7 @@ function M.create(ctx)
end
remove_binding_names(previous_binding_names)
state.session_binding_names = next_binding_names
subminer_log(
"info",

View File

@@ -106,6 +106,17 @@ test('parseArgs captures session action forwarding flags', () => {
assert.equal(shouldStartApp(args), true);
});
test('parseArgs ignores non-positive numeric session action counts', () => {
const args = parseArgs([
'--copy-subtitle-count=0',
'--mine-sentence-count',
'-1',
]);
assert.equal(args.copySubtitleCount, undefined);
assert.equal(args.mineSentenceCount, undefined);
});
test('youtube playback does not use generic overlay-runtime bootstrap classification', () => {
const args = parseArgs(['--youtube-play', 'https://youtube.com/watch?v=abc']);

View File

@@ -238,21 +238,19 @@ export function parseArgs(argv: string[]): CliArgs {
args.cycleRuntimeOptionId = parsed.id;
args.cycleRuntimeOptionDirection = parsed.direction;
}
}
else if (arg.startsWith('--copy-subtitle-count=')) {
} else if (arg.startsWith('--copy-subtitle-count=')) {
const value = Number(arg.split('=', 2)[1]);
if (Number.isInteger(value)) args.copySubtitleCount = value;
if (Number.isInteger(value) && value > 0) args.copySubtitleCount = value;
} else if (arg === '--copy-subtitle-count') {
const value = Number(readValue(argv[i + 1]));
if (Number.isInteger(value)) args.copySubtitleCount = value;
if (Number.isInteger(value) && value > 0) args.copySubtitleCount = value;
} else if (arg.startsWith('--mine-sentence-count=')) {
const value = Number(arg.split('=', 2)[1]);
if (Number.isInteger(value)) args.mineSentenceCount = value;
if (Number.isInteger(value) && value > 0) args.mineSentenceCount = value;
} else if (arg === '--mine-sentence-count') {
const value = Number(readValue(argv[i + 1]));
if (Number.isInteger(value)) args.mineSentenceCount = value;
}
else if (arg === '--anilist-status') args.anilistStatus = true;
if (Number.isInteger(value) && value > 0) args.mineSentenceCount = value;
} else if (arg === '--anilist-status') args.anilistStatus = true;
else if (arg === '--anilist-logout') args.anilistLogout = true;
else if (arg === '--anilist-setup') args.anilistSetup = true;
else if (arg === '--anilist-retry-queue') args.anilistRetryQueue = true;

View File

@@ -3,7 +3,11 @@ import assert from 'node:assert/strict';
import { createIpcDepsRuntime, registerIpcHandlers, type IpcServiceDeps } from './ipc';
import { IPC_CHANNELS } from '../../shared/ipc/contracts';
import type { PlaylistBrowserSnapshot, SubtitleSidebarSnapshot } from '../../types';
import type {
PlaylistBrowserSnapshot,
SessionActionDispatchRequest,
SubtitleSidebarSnapshot,
} from '../../types';
interface FakeIpcRegistrar {
on: Map<string, (event: unknown, ...args: unknown[]) => void>;
@@ -860,6 +864,55 @@ test('registerIpcHandlers awaits saveControllerPreference through request-respon
]);
});
test('registerIpcHandlers validates dispatchSessionAction payloads', async () => {
const { registrar, handlers } = createFakeIpcRegistrar();
const dispatched: SessionActionDispatchRequest[] = [];
registerIpcHandlers(
createRegisterIpcDeps({
dispatchSessionAction: async (request) => {
dispatched.push(request);
},
}),
registrar,
);
const dispatchHandler = handlers.handle.get(IPC_CHANNELS.command.dispatchSessionAction);
assert.ok(dispatchHandler);
await assert.rejects(async () => {
await dispatchHandler!({}, { actionId: 'cycleRuntimeOption', payload: { direction: 1 } });
}, /Invalid session action payload/);
await assert.rejects(async () => {
await dispatchHandler!({}, { actionId: 'unknown-action' });
}, /Invalid session action payload/);
await dispatchHandler!({}, {
actionId: 'copySubtitleMultiple',
payload: { count: 3 },
});
await dispatchHandler!({}, {
actionId: 'cycleRuntimeOption',
payload: {
runtimeOptionId: 'anki.autoUpdateNewCards',
direction: -1,
},
});
assert.deepEqual(dispatched, [
{
actionId: 'copySubtitleMultiple',
payload: { count: 3 },
},
{
actionId: 'cycleRuntimeOption',
payload: {
runtimeOptionId: 'anki.autoUpdateNewCards',
direction: -1,
},
},
]);
});
test('registerIpcHandlers rejects malformed controller preference payloads', async () => {
const { registrar, handlers } = createFakeIpcRegistrar();
registerIpcHandlers(

View File

@@ -27,6 +27,7 @@ import {
parseRuntimeOptionDirection,
parseRuntimeOptionId,
parseRuntimeOptionValue,
parseSessionActionDispatchRequest,
parseSubtitlePosition,
parseSubsyncManualRunRequest,
parseYoutubePickerResolveRequest,
@@ -459,22 +460,11 @@ export function registerIpcHandlers(deps: IpcServiceDeps, ipc: IpcMainRegistrar
ipc.handle(
IPC_CHANNELS.command.dispatchSessionAction,
async (_event: unknown, request: unknown) => {
if (!request || typeof request !== 'object') {
const parsedRequest = parseSessionActionDispatchRequest(request);
if (!parsedRequest) {
throw new Error('Invalid session action payload');
}
const actionId =
typeof (request as Record<string, unknown>).actionId === 'string'
? ((request as Record<string, unknown>).actionId as SessionActionDispatchRequest['actionId'])
: null;
if (!actionId) {
throw new Error('Invalid session action id');
}
const payload =
(request as Record<string, unknown>).payload &&
typeof (request as Record<string, unknown>).payload === 'object'
? ((request as Record<string, unknown>).payload as SessionActionDispatchRequest['payload'])
: undefined;
await deps.dispatchSessionAction?.({ actionId, payload });
await deps.dispatchSessionAction?.(parsedRequest);
},
);

View File

@@ -319,7 +319,7 @@ test('shouldActivateOverlayShortcuts preserves non-macOS behavior', () => {
});
test('registerOverlayShortcutsRuntime reports active shortcuts when configured', () => {
const result = registerOverlayShortcutsRuntime({
const deps = {
getConfiguredShortcuts: () => makeShortcuts({ openJimaku: 'Ctrl+J' }),
getOverlayHandlers: () => ({
copySubtitle: () => {},
@@ -336,15 +336,17 @@ test('registerOverlayShortcutsRuntime reports active shortcuts when configured',
}),
cancelPendingMultiCopy: () => {},
cancelPendingMineSentenceMultiple: () => {},
});
};
const result = registerOverlayShortcutsRuntime(deps);
assert.equal(result, true);
assert.equal(unregisterOverlayShortcutsRuntime(result, deps), false);
});
test('unregisterOverlayShortcutsRuntime clears pending shortcut work when active', () => {
const calls: string[] = [];
const result = unregisterOverlayShortcutsRuntime(true, {
getConfiguredShortcuts: () => makeShortcuts(),
const deps = {
getConfiguredShortcuts: () => makeShortcuts({ openJimaku: 'Ctrl+J' }),
getOverlayHandlers: () => ({
copySubtitle: () => {},
copySubtitleMultiple: () => {},
@@ -364,8 +366,10 @@ test('unregisterOverlayShortcutsRuntime clears pending shortcut work when active
cancelPendingMineSentenceMultiple: () => {
calls.push('cancel-mine-sentence-multiple');
},
});
};
assert.equal(registerOverlayShortcutsRuntime(deps), true);
const result = unregisterOverlayShortcutsRuntime(true, deps);
assert.equal(result, false);
assert.deepEqual(calls, ['cancel-multi-copy', 'cancel-mine-sentence-multiple']);
});

View File

@@ -200,6 +200,24 @@ test('compileSessionBindings warns on unsupported shortcut and keybinding syntax
);
});
test('compileSessionBindings rejects malformed command arrays', () => {
const result = compileSessionBindings({
shortcuts: createShortcuts(),
keybindings: [
createKeybinding('Ctrl+J', ['show-text', 3000]),
createKeybinding('Ctrl+K', ['show-text', { bad: true } as never] as never),
],
platform: 'linux',
});
assert.deepEqual(result.bindings.map((binding) => binding.sourcePath), ['keybindings[0].key']);
assert.equal(result.bindings[0]?.actionType, 'mpv-command');
assert.deepEqual(result.bindings[0]?.command, ['show-text', 3000]);
assert.deepEqual(result.warnings.map((warning) => `${warning.kind}:${warning.path}`), [
'unsupported:keybindings[1].key',
]);
});
test('compileSessionBindings warns on deprecated toggleVisibleOverlayGlobal config', () => {
const result = compileSessionBindings({
shortcuts: createShortcuts(),

View File

@@ -53,6 +53,10 @@ function normalizeModifiers(modifiers: SessionKeyModifier[]): SessionKeyModifier
);
}
function isValidCommandEntry(value: unknown): value is string | number {
return typeof value === 'string' || typeof value === 'number';
}
function normalizeCodeToken(token: string): string | null {
const normalized = token.trim();
if (!normalized) return null;
@@ -253,8 +257,19 @@ function resolveCommandBinding(
| Omit<CompiledMpvCommandBinding, 'key' | 'sourcePath' | 'originalKey'>
| Omit<CompiledSessionActionBinding, 'key' | 'sourcePath' | 'originalKey'>
| null {
const command = binding.command ?? [];
const first = typeof command[0] === 'string' ? command[0] : '';
const command = binding.command;
if (!Array.isArray(command) || command.length === 0 || !command.every(isValidCommandEntry)) {
return null;
}
const first = command[0];
if (typeof first !== 'string') {
return {
actionType: 'mpv-command',
command,
};
}
if (first === SPECIAL_COMMANDS.SUBSYNC_TRIGGER) {
return { actionType: 'session-action', actionId: 'triggerSubsync' };
}
@@ -283,7 +298,14 @@ function resolveCommandBinding(
return { actionType: 'session-action', actionId: 'shiftSubDelayNextLine' };
}
if (first.startsWith(SPECIAL_COMMANDS.RUNTIME_OPTION_CYCLE_PREFIX)) {
const [, runtimeOptionId, rawDirection] = first.split(':');
const parts = first.split(':');
if (parts.length !== 3) {
return null;
}
const [, runtimeOptionId, rawDirection] = parts;
if (!runtimeOptionId || (rawDirection !== 'prev' && rawDirection !== 'next')) {
return null;
}
return {
actionType: 'session-action',
actionId: 'cycleRuntimeOption',
@@ -398,7 +420,15 @@ export function compileSessionBindings(
return;
}
const resolved = resolveCommandBinding(binding);
if (!resolved) return;
if (!resolved) {
warnings.push({
kind: 'unsupported',
path: `keybindings[${index}].key`,
value: binding.command,
message: 'Unsupported keybinding command syntax.',
});
return;
}
const compiled: CompiledSessionBinding = {
sourcePath: `keybindings[${index}].key`,
originalKey: binding.key,

View File

@@ -138,6 +138,7 @@ import {
ensureWindowsOverlayTransparencyNative,
getWindowsForegroundProcessNameNative,
queryWindowsForegroundProcessName,
queryWindowsTargetWindowHandle,
setWindowsOverlayOwnerNative,
} from './window-trackers/windows-helper';
import {
@@ -1939,11 +1940,16 @@ function resolveWindowsOverlayBindTargetHandle(targetMpvSocketPath?: string | nu
return null;
}
const helperTargetHwnd = queryWindowsTargetWindowHandle({ targetMpvSocketPath });
if (helperTargetHwnd !== null) {
return helperTargetHwnd;
}
try {
const win32 = require('./window-trackers/win32') as typeof import('./window-trackers/win32');
const poll = win32.findMpvWindows();
const focused = poll.matches.find((m) => m.isForeground);
return focused?.hwnd ?? poll.matches.sort((a, b) => b.area - a.area)[0]?.hwnd ?? null;
return focused?.hwnd ?? [...poll.matches].sort((a, b) => b.area - a.area)[0]?.hwnd ?? null;
} catch {
return null;
}
@@ -5144,7 +5150,7 @@ const { initializeOverlayRuntime: initializeOverlayRuntimeHandler } =
const win32 = require('./window-trackers/win32') as typeof import('./window-trackers/win32');
const poll = win32.findMpvWindows();
const focused = poll.matches.find((m) => m.isForeground);
return focused ?? poll.matches.sort((a, b) => b.area - a.area)[0] ?? null;
return focused ?? [...poll.matches].sort((a, b) => b.area - a.area)[0] ?? null;
} catch {
return null;
}

View File

@@ -225,6 +225,24 @@ test('sendToActiveOverlayWindow creates modal window lazily when absent', () =>
assert.deepEqual(window.sent, [['jimaku:open']]);
});
test('sendToActiveOverlayWindow does not retain restore state when modal creation fails', () => {
const runtime = createOverlayModalRuntimeService({
getMainWindow: () => null,
getModalWindow: () => null,
createModalWindow: () => null,
getModalGeometry: () => ({ x: 0, y: 0, width: 400, height: 300 }),
setModalWindowBounds: () => {},
});
assert.equal(
runtime.sendToActiveOverlayWindow('runtime-options:open', undefined, {
restoreOnModalClose: 'runtime-options',
}),
false,
);
assert.equal(runtime.getRestoreVisibleOverlayOnModalClose().has('runtime-options'), false);
});
test('sendToActiveOverlayWindow waits for blank modal URL before sending open command', () => {
const window = createMockWindow();
window.url = '';
@@ -587,6 +605,37 @@ test('sendToActiveOverlayWindow waits for modal ready-to-show before delivering
assert.deepEqual(window.sent, [['runtime-options:open']]);
});
test('warm modal window reopen becomes interactive immediately on the second open', () => {
const window = createMockWindow();
const runtime = createOverlayModalRuntimeService({
getMainWindow: () => null,
getModalWindow: () => window as never,
createModalWindow: () => window as never,
getModalGeometry: () => ({ x: 0, y: 0, width: 400, height: 300 }),
setModalWindowBounds: () => {},
});
runtime.sendToActiveOverlayWindow('runtime-options:open', undefined, {
restoreOnModalClose: 'runtime-options',
});
runtime.notifyOverlayModalOpened('runtime-options');
runtime.handleOverlayModalClosed('runtime-options');
window.ignoreMouseEvents = true;
window.focused = false;
window.webContentsFocused = false;
runtime.sendToActiveOverlayWindow('runtime-options:open', undefined, {
restoreOnModalClose: 'runtime-options',
});
assert.equal(window.isVisible(), true);
assert.equal(window.ignoreMouseEvents, false);
assert.equal(window.isFocused(), true);
assert.equal(window.webContentsFocused, true);
assert.equal(window.getShowCount(), 2);
});
test('waitForModalOpen resolves true after modal acknowledgement', async () => {
const runtime = createOverlayModalRuntimeService({
getMainWindow: () => null,

View File

@@ -43,6 +43,7 @@ export function createOverlayModalRuntimeService(
let modalActive = false;
let mainWindowMousePassthroughForcedByModal = false;
let mainWindowHiddenByModal = false;
let modalWindowPrimedForImmediateShow = false;
let pendingModalWindowReveal: BrowserWindow | null = null;
let pendingModalWindowRevealTimeout: ReturnType<typeof setTimeout> | null = null;
@@ -272,9 +273,9 @@ export function createOverlayModalRuntimeService(
};
if (restoreOnModalClose) {
restoreVisibleOverlayOnModalClose.add(restoreOnModalClose);
const mainWindow = getTargetOverlayWindow();
if (!preferModalWindow && mainWindow && !mainWindow.isDestroyed() && mainWindow.isVisible()) {
restoreVisibleOverlayOnModalClose.add(restoreOnModalClose);
sendOrQueueForWindow(mainWindow, (window) => {
if (payload === undefined) {
window.webContents.send(channel);
@@ -288,10 +289,15 @@ export function createOverlayModalRuntimeService(
const modalWindow = resolveModalWindow();
if (!modalWindow) return false;
restoreVisibleOverlayOnModalClose.add(restoreOnModalClose);
deps.setModalWindowBounds(deps.getModalGeometry());
const wasVisible = modalWindow.isVisible();
if (!wasVisible) {
scheduleModalWindowReveal(modalWindow);
if (modalWindowPrimedForImmediateShow && isWindowReadyForIpc(modalWindow)) {
showModalWindow(modalWindow);
} else {
scheduleModalWindowReveal(modalWindow);
}
} else if (!modalWindow.isFocused()) {
showModalWindow(modalWindow);
}
@@ -339,6 +345,7 @@ export function createOverlayModalRuntimeService(
if (modalWindow && !modalWindow.isDestroyed()) {
modalWindow.hide();
}
modalWindowPrimedForImmediateShow = true;
mainWindowMousePassthroughForcedByModal = false;
mainWindowHiddenByModal = false;
notifyModalStateChange(false);

View File

@@ -89,6 +89,17 @@ test('shouldAutoOpenFirstRunSetup only for startup/setup intents', () => {
assert.equal(shouldAutoOpenFirstRunSetup(makeArgs({ settings: true })), false);
});
test('shouldAutoOpenFirstRunSetup treats numeric startup counts as explicit commands', () => {
assert.equal(
shouldAutoOpenFirstRunSetup(makeArgs({ start: true, copySubtitleCount: 2 })),
false,
);
assert.equal(
shouldAutoOpenFirstRunSetup(makeArgs({ background: true, mineSentenceCount: 1 })),
false,
);
});
test('setup service auto-completes legacy installs with config and dictionaries', async () => {
await withTempDir(async (root) => {
const configDir = path.join(root, 'SubMiner');

View File

@@ -68,8 +68,10 @@ function hasAnyStartupCommandBeyondSetup(args: CliArgs): boolean {
args.hideVisibleOverlay ||
args.copySubtitle ||
args.copySubtitleMultiple ||
args.copySubtitleCount !== undefined ||
args.mineSentence ||
args.mineSentenceMultiple ||
args.mineSentenceCount !== undefined ||
args.updateLastCardFromClipboard ||
args.refreshKnownWords ||
args.toggleSecondarySub ||

View File

@@ -193,7 +193,10 @@ test('createImmersionTrackerStartupHandler keeps tracker startup alive when mpv
'warn:MPV auto-connect failed during immersion tracker startup; continuing.:socket not ready',
),
);
assert.equal(calls.includes('warn:Immersion tracker startup failed; disabling tracking.'), false);
assert.equal(
calls.some((entry) => entry.startsWith('warn:Immersion tracker startup failed; disabling tracking.')),
false,
);
});
test('createImmersionTrackerStartupHandler disables tracker on failure', () => {

View File

@@ -95,6 +95,7 @@ function withRuntimeOptionsModal(
Object.defineProperty(globalThis, 'window', {
configurable: true,
writable: true,
value: {
electronAPI: {
getRuntimeOptions,
@@ -109,6 +110,7 @@ function withRuntimeOptionsModal(
Object.defineProperty(globalThis, 'document', {
configurable: true,
writable: true,
value: {
createElement: () => createElementStub(),
},
@@ -152,10 +154,12 @@ function withRuntimeOptionsModal(
.finally(() => {
Object.defineProperty(globalThis, 'window', {
configurable: true,
writable: true,
value: previousWindow,
});
Object.defineProperty(globalThis, 'document', {
configurable: true,
writable: true,
value: previousDocument,
});
});

View File

@@ -8,12 +8,37 @@ import type {
import type {
ControllerConfigUpdate,
ControllerPreferenceUpdate,
SessionActionDispatchRequest,
SubsyncManualRunRequest,
} from '../../types/runtime';
import type { RuntimeOptionId, RuntimeOptionValue } from '../../types/runtime-options';
import type { SessionActionId, SessionActionPayload } from '../../types/session-bindings';
import type { SubtitlePosition } from '../../types/subtitle';
import { OVERLAY_HOSTED_MODALS, type OverlayHostedModal } from './contracts';
const SESSION_ACTION_IDS: SessionActionId[] = [
'toggleStatsOverlay',
'toggleVisibleOverlay',
'copySubtitle',
'copySubtitleMultiple',
'updateLastCardFromClipboard',
'triggerFieldGrouping',
'triggerSubsync',
'mineSentence',
'mineSentenceMultiple',
'toggleSecondarySub',
'markAudioCard',
'openRuntimeOptions',
'openJimaku',
'openYoutubePicker',
'openPlaylistBrowser',
'replayCurrentSubtitle',
'playNextSubtitle',
'shiftSubDelayPrevLine',
'shiftSubDelayNextLine',
'cycleRuntimeOption',
];
const RUNTIME_OPTION_IDS: RuntimeOptionId[] = [
'anki.autoUpdateNewCards',
'subtitle.annotation.nPlusOne',
@@ -35,6 +60,43 @@ function isInteger(value: unknown): value is number {
return typeof value === 'number' && Number.isInteger(value);
}
function isSessionActionId(value: unknown): value is SessionActionId {
return typeof value === 'string' && SESSION_ACTION_IDS.includes(value as SessionActionId);
}
function parseSessionActionPayload(
actionId: SessionActionId,
value: unknown,
): SessionActionPayload | undefined | null {
if (actionId === 'copySubtitleMultiple' || actionId === 'mineSentenceMultiple') {
if (value === undefined) return undefined;
if (!isObject(value)) return null;
const keys = Object.keys(value);
if (keys.some((key) => key !== 'count')) return null;
if (value.count === undefined) return null;
if (!isInteger(value.count) || value.count < 1) return null;
return { count: value.count };
}
if (actionId === 'cycleRuntimeOption') {
if (!isObject(value)) return null;
const keys = Object.keys(value);
if (keys.some((key) => key !== 'runtimeOptionId' && key !== 'direction')) return null;
if (typeof value.runtimeOptionId !== 'string' || value.runtimeOptionId.trim().length === 0) {
return null;
}
if (value.direction !== 1 && value.direction !== -1) {
return null;
}
return {
runtimeOptionId: value.runtimeOptionId,
direction: value.direction,
};
}
return value === undefined ? undefined : null;
}
export function parseOverlayHostedModal(value: unknown): OverlayHostedModal | null {
if (typeof value !== 'string') return null;
return OVERLAY_HOSTED_MODALS.includes(value as OverlayHostedModal)
@@ -182,6 +244,17 @@ export function parseRuntimeOptionValue(value: unknown): RuntimeOptionValue | nu
: null;
}
export function parseSessionActionDispatchRequest(
value: unknown,
): SessionActionDispatchRequest | null {
if (!isObject(value)) return null;
if (!isSessionActionId(value.actionId)) return null;
const payload = parseSessionActionPayload(value.actionId, value.payload);
if (payload === null) return null;
return payload === undefined ? { actionId: value.actionId } : { actionId: value.actionId, payload };
}
export function parseMpvCommand(value: unknown): Array<string | number> | null {
if (!Array.isArray(value)) return null;
return value.every((entry) => typeof entry === 'string' || typeof entry === 'number')