diff --git a/changes/fix-pr-49-coderabbit-review-follow-ups.md b/changes/fix-pr-49-coderabbit-review-follow-ups.md new file mode 100644 index 00000000..320cb661 --- /dev/null +++ b/changes/fix-pr-49-coderabbit-review-follow-ups.md @@ -0,0 +1,4 @@ +type: fixed +area: overlay + +- Addressed the latest CodeRabbit follow-ups on PR #49, including generation-scoped Lua session binding names, stricter session command validation, session-help shortcut visibility, the numeric-selection key guard, stats-overlay startup classification, and safer session-binding persistence. diff --git a/plugin/subminer/session_bindings.lua b/plugin/subminer/session_bindings.lua index de6fdbed..718a1041 100644 --- a/plugin/subminer/session_bindings.lua +++ b/plugin/subminer/session_bindings.lua @@ -313,12 +313,14 @@ function M.create(ctx) local previous_binding_names = state.session_binding_names local next_binding_names = {} + state.session_binding_generation = (state.session_binding_generation or 0) + 1 + local generation = state.session_binding_generation local timeout_ms = tonumber(artifact.numericSelectionTimeoutMs) or 3000 for index, binding in ipairs(artifact.bindings) do local key_name = key_spec_to_mpv_binding(binding.key) if key_name then - local name = "subminer-session-binding-" .. tostring(index) + local name = "subminer-session-binding-" .. tostring(generation) .. "-" .. tostring(index) next_binding_names[#next_binding_names + 1] = name mp.add_forced_key_binding(key_name, name, function() handle_binding(binding, timeout_ms) diff --git a/plugin/subminer/state.lua b/plugin/subminer/state.lua index 285450e9..6573f8c0 100644 --- a/plugin/subminer/state.lua +++ b/plugin/subminer/state.lua @@ -33,6 +33,7 @@ function M.new() auto_play_ready_timeout = nil, auto_play_ready_osd_timer = nil, suppress_ready_overlay_restore = false, + session_binding_generation = 0, session_binding_names = {}, session_numeric_binding_names = {}, session_numeric_selection = nil, diff --git a/src/cli/args.test.ts b/src/cli/args.test.ts index 6b057412..9df171d0 100644 --- a/src/cli/args.test.ts +++ b/src/cli/args.test.ts @@ -231,6 +231,9 @@ test('hasExplicitCommand and shouldStartApp preserve command intent', () => { assert.equal(shouldStartApp(cycleRuntimeOption), true); assert.equal(commandNeedsOverlayRuntime(cycleRuntimeOption), true); + const toggleStatsOverlayRuntime = parseArgs(['--toggle-stats-overlay']); + assert.equal(commandNeedsOverlayRuntime(toggleStatsOverlayRuntime), true); + const dictionary = parseArgs(['--dictionary']); assert.equal(dictionary.dictionary, true); assert.equal(hasExplicitCommand(dictionary), true); diff --git a/src/cli/args.ts b/src/cli/args.ts index b02c09b5..32a3d593 100644 --- a/src/cli/args.ts +++ b/src/cli/args.ts @@ -686,6 +686,7 @@ export function commandNeedsOverlayRuntime(args: CliArgs): boolean { args.mineSentenceMultiple || args.updateLastCardFromClipboard || args.toggleSecondarySub || + args.toggleStatsOverlay || args.toggleSubtitleSidebar || args.triggerFieldGrouping || args.triggerSubsync || diff --git a/src/core/services/session-bindings.test.ts b/src/core/services/session-bindings.test.ts index f29f3a2a..a6edd045 100644 --- a/src/core/services/session-bindings.test.ts +++ b/src/core/services/session-bindings.test.ts @@ -230,6 +230,23 @@ test('compileSessionBindings rejects malformed command arrays', () => { ]); }); +test('compileSessionBindings rejects non-string command heads and extra args on special commands', () => { + const result = compileSessionBindings({ + shortcuts: createShortcuts(), + keybindings: [ + createKeybinding('Ctrl+J', [42] as never), + createKeybinding('Ctrl+K', [SPECIAL_COMMANDS.JIMAKU_OPEN, 'extra'] as never), + ], + platform: 'linux', + }); + + assert.deepEqual(result.bindings, []); + assert.deepEqual(result.warnings.map((warning) => `${warning.kind}:${warning.path}`), [ + 'unsupported:keybindings[0].key', + 'unsupported:keybindings[1].key', + ]); +}); + test('compileSessionBindings warns on deprecated toggleVisibleOverlayGlobal config', () => { const result = compileSessionBindings({ shortcuts: createShortcuts(), diff --git a/src/core/services/session-bindings.ts b/src/core/services/session-bindings.ts index 3d73eaad..950f4e45 100644 --- a/src/core/services/session-bindings.ts +++ b/src/core/services/session-bindings.ts @@ -268,40 +268,49 @@ function resolveCommandBinding( const first = command[0]; if (typeof first !== 'string') { - return { - actionType: 'mpv-command', - command, - }; + return null; } if (first === SPECIAL_COMMANDS.SUBSYNC_TRIGGER) { + if (command.length !== 1) return null; return { actionType: 'session-action', actionId: 'triggerSubsync' }; } if (first === SPECIAL_COMMANDS.RUNTIME_OPTIONS_OPEN) { + if (command.length !== 1) return null; return { actionType: 'session-action', actionId: 'openRuntimeOptions' }; } if (first === SPECIAL_COMMANDS.JIMAKU_OPEN) { + if (command.length !== 1) return null; return { actionType: 'session-action', actionId: 'openJimaku' }; } if (first === SPECIAL_COMMANDS.YOUTUBE_PICKER_OPEN) { + if (command.length !== 1) return null; return { actionType: 'session-action', actionId: 'openYoutubePicker' }; } if (first === SPECIAL_COMMANDS.PLAYLIST_BROWSER_OPEN) { + if (command.length !== 1) return null; return { actionType: 'session-action', actionId: 'openPlaylistBrowser' }; } if (first === SPECIAL_COMMANDS.REPLAY_SUBTITLE) { + if (command.length !== 1) return null; return { actionType: 'session-action', actionId: 'replayCurrentSubtitle' }; } if (first === SPECIAL_COMMANDS.PLAY_NEXT_SUBTITLE) { + if (command.length !== 1) return null; return { actionType: 'session-action', actionId: 'playNextSubtitle' }; } if (first === SPECIAL_COMMANDS.SHIFT_SUB_DELAY_TO_PREVIOUS_SUBTITLE_START) { + if (command.length !== 1) return null; return { actionType: 'session-action', actionId: 'shiftSubDelayPrevLine' }; } if (first === SPECIAL_COMMANDS.SHIFT_SUB_DELAY_TO_NEXT_SUBTITLE_START) { + if (command.length !== 1) return null; return { actionType: 'session-action', actionId: 'shiftSubDelayNextLine' }; } if (first.startsWith(SPECIAL_COMMANDS.RUNTIME_OPTION_CYCLE_PREFIX)) { + if (command.length !== 1) { + return null; + } const parts = first.split(':'); if (parts.length !== 3) { return null; diff --git a/src/main.ts b/src/main.ts index d0cabed8..3e019b8d 100644 --- a/src/main.ts +++ b/src/main.ts @@ -1944,7 +1944,17 @@ function resolveWindowsOverlayBindTargetHandle(targetMpvSocketPath?: string | nu } try { - void targetMpvSocketPath; + if (targetMpvSocketPath) { + const windowTracker = appState.windowTracker as + | { + getTargetWindowHandle?: () => number | null; + } + | null; + const trackedHandle = windowTracker?.getTargetWindowHandle?.(); + if (typeof trackedHandle === 'number' && Number.isFinite(trackedHandle)) { + return trackedHandle; + } + } return findWindowsMpvTargetWindowHandle(); } catch { return null; @@ -4244,15 +4254,13 @@ function persistSessionBindings( bindings: CompiledSessionBinding[], warnings: ReturnType['warnings'] = [], ): void { + const artifact = buildPluginSessionBindingsArtifact({ + bindings, + warnings, + numericSelectionTimeoutMs: getConfiguredShortcuts().multiCopyTimeoutMs, + }); + writeSessionBindingsArtifact(CONFIG_DIR, artifact); appState.sessionBindings = bindings; - writeSessionBindingsArtifact( - CONFIG_DIR, - buildPluginSessionBindingsArtifact({ - bindings, - warnings, - numericSelectionTimeoutMs: getConfiguredShortcuts().multiCopyTimeoutMs, - }), - ); if (appState.mpvClient?.connected) { sendMpvCommandRuntime(appState.mpvClient, [ 'script-message', diff --git a/src/renderer/handlers/keyboard.test.ts b/src/renderer/handlers/keyboard.test.ts index aeb736d6..59f86aae 100644 --- a/src/renderer/handlers/keyboard.test.ts +++ b/src/renderer/handlers/keyboard.test.ts @@ -389,6 +389,88 @@ function createKeyboardHandlerHarness() { }; } +test('session help chord resolver follows remapped session bindings', async () => { + const { handlers, testGlobals } = createKeyboardHandlerHarness(); + + try { + await handlers.setupMpvInputForwarding(); + + assert.deepEqual(handlers.getSessionHelpOpeningInfo(), { + bindingKey: 'KeyH', + fallbackUsed: false, + fallbackUnavailable: false, + }); + + handlers.updateSessionBindings([ + { + sourcePath: 'keybindings[0].key', + originalKey: 'KeyH', + key: { code: 'KeyH', modifiers: [] }, + actionType: 'session-action', + actionId: 'openJimaku', + }, + { + sourcePath: 'keybindings[1].key', + originalKey: 'KeyJ', + key: { code: 'KeyJ', modifiers: [] }, + actionType: 'mpv-command', + command: ['cycle', 'pause'], + }, + ] as never); + + assert.deepEqual(handlers.getSessionHelpOpeningInfo(), { + bindingKey: 'KeyK', + fallbackUsed: true, + fallbackUnavailable: false, + }); + + handlers.updateSessionBindings([ + { + sourcePath: 'keybindings[0].key', + originalKey: 'KeyH', + key: { code: 'KeyH', modifiers: [] }, + actionType: 'session-action', + actionId: 'openSessionHelp', + }, + { + sourcePath: 'keybindings[1].key', + originalKey: 'KeyK', + key: { code: 'KeyK', modifiers: [] }, + actionType: 'session-action', + actionId: 'openControllerSelect', + }, + ] as never); + + assert.deepEqual(handlers.getSessionHelpOpeningInfo(), { + bindingKey: 'KeyK', + fallbackUsed: true, + fallbackUnavailable: true, + }); + } finally { + testGlobals.restore(); + } +}); + +test('numeric selection ignores non-digit keys instead of falling through to other shortcuts', async () => { + const { handlers, testGlobals, ctx } = createKeyboardHandlerHarness(); + + try { + await handlers.setupMpvInputForwarding(); + handlers.beginSessionNumericSelection('copySubtitleMultiple'); + + testGlobals.dispatchKeydown({ key: 'y', code: 'KeyY' }); + + assert.equal(ctx.state.chordPending, false); + assert.deepEqual(testGlobals.sessionActions, []); + assert.equal( + testGlobals.commandEvents.some((event) => event.type === 'forwardKeyDown'), + false, + ); + } finally { + testGlobals.restore(); + } +}); + test('keyboard mode: left and right move token selection while popup remains open', async () => { const { ctx, handlers, testGlobals } = createKeyboardHandlerHarness(); diff --git a/src/renderer/handlers/keyboard.ts b/src/renderer/handlers/keyboard.ts index b4ad5dd5..93e735b9 100644 --- a/src/renderer/handlers/keyboard.ts +++ b/src/renderer/handlers/keyboard.ts @@ -160,8 +160,9 @@ export function createKeyboardHandlers( return true; } - if (!/^[1-9]$/.test(e.key) || e.ctrlKey || e.metaKey || e.altKey) { - return false; + if (!/^[1-9]$/.test(e.key) || e.ctrlKey || e.metaKey || e.altKey || e.shiftKey) { + e.preventDefault(); + return true; } e.preventDefault(); @@ -1115,6 +1116,7 @@ export function createKeyboardHandlers( return { beginSessionNumericSelection, + getSessionHelpOpeningInfo: resolveSessionHelpChordBinding, setupMpvInputForwarding, refreshConfiguredShortcuts, updateSessionBindings, diff --git a/src/renderer/modals/session-help.ts b/src/renderer/modals/session-help.ts index f91210c8..c1c46d0e 100644 --- a/src/renderer/modals/session-help.ts +++ b/src/renderer/modals/session-help.ts @@ -96,6 +96,10 @@ const OVERLAY_SHORTCUTS: Array<{ { key: 'markAudioCard', label: 'Mark audio card' }, { key: 'openRuntimeOptions', label: 'Open runtime options' }, { key: 'openJimaku', label: 'Open jimaku' }, + { key: 'openSessionHelp', label: 'Open session help' }, + { key: 'openControllerSelect', label: 'Open controller select' }, + { key: 'openControllerDebug', label: 'Open controller debug' }, + { key: 'toggleSubtitleSidebar', label: 'Toggle subtitle sidebar' }, { key: 'toggleVisibleOverlayGlobal', label: 'Show/hide visible overlay' }, ]; @@ -104,11 +108,12 @@ function buildOverlayShortcutSections(shortcuts: RuntimeShortcutConfig): Session for (const shortcut of OVERLAY_SHORTCUTS) { const keybind = shortcuts[shortcut.key]; - if (typeof keybind !== 'string') continue; - if (keybind.trim().length === 0) continue; rows.push({ - shortcut: formatKeybinding(keybind), + shortcut: + typeof keybind === 'string' && keybind.trim().length > 0 + ? formatKeybinding(keybind) + : 'Unbound', action: shortcut.label, }); } @@ -591,13 +596,17 @@ export function createSessionHelpModal( priorFocus = document.activeElement; ctx.state.sessionHelpModalOpen = true; + helpSections = []; + helpFilterValue = ''; options.syncSettingsModalSubtitleSuppression(); ctx.dom.overlay.classList.add('interactive'); ctx.dom.sessionHelpModal.classList.remove('hidden'); ctx.dom.sessionHelpModal.setAttribute('aria-hidden', 'false'); ctx.dom.sessionHelpModal.setAttribute('tabindex', '-1'); ctx.dom.sessionHelpFilter.value = ''; - helpFilterValue = ''; + ctx.state.sessionHelpSelectedIndex = 0; + ctx.dom.sessionHelpContent.innerHTML = ''; + ctx.dom.sessionHelpContent.classList.remove('session-help-content-no-results'); if (ctx.platform.shouldToggleMouseIgnore) { window.electronAPI.setIgnoreMouseEvents(false); } diff --git a/src/renderer/renderer.ts b/src/renderer/renderer.ts index 14b22016..9b01546e 100644 --- a/src/renderer/renderer.ts +++ b/src/renderer/renderer.ts @@ -431,11 +431,7 @@ function registerModalOpenHandlers(): void { }); window.electronAPI.onOpenSessionHelp(() => { runGuarded('session-help:open', () => { - sessionHelpModal.openSessionHelpModal({ - bindingKey: 'KeyH', - fallbackUsed: false, - fallbackUnavailable: false, - }); + sessionHelpModal.openSessionHelpModal(keyboardHandlers.getSessionHelpOpeningInfo()); window.electronAPI.notifyOverlayModalOpened('session-help'); }); }); diff --git a/src/window-trackers/windows-tracker.ts b/src/window-trackers/windows-tracker.ts index f7375799..9fb1c7a3 100644 --- a/src/window-trackers/windows-tracker.ts +++ b/src/window-trackers/windows-tracker.ts @@ -51,6 +51,7 @@ export class WindowsWindowTracker extends BaseWindowTracker { private trackingLossStartedAtMs: number | null = null; private targetWindowMinimized = false; private readonly targetMpvSocketPath: string | null; + private currentTargetWindowHwnd: number | null = null; constructor(_targetMpvSocketPath?: string, deps: WindowsTrackerDeps = {}) { super(); @@ -81,6 +82,10 @@ export class WindowsWindowTracker extends BaseWindowTracker { return this.targetWindowMinimized; } + getTargetWindowHandle(): number | null { + return this.currentTargetWindowHwnd; + } + private maybeLogPollError(error: Error): void { const now = Date.now(); const fingerprint = error.message; @@ -122,7 +127,7 @@ export class WindowsWindowTracker extends BaseWindowTracker { private selectBestMatch( result: MpvPollResult, - ): { geometry: WindowGeometry; focused: boolean } | null { + ): { geometry: WindowGeometry; focused: boolean; hwnd: number } | null { if (result.matches.length === 0) return null; const focusedMatch = result.matches.find((m) => m.isForeground); @@ -133,6 +138,7 @@ export class WindowsWindowTracker extends BaseWindowTracker { return { geometry: best.bounds, focused: best.isForeground, + hwnd: best.hwnd, }; } @@ -147,6 +153,7 @@ export class WindowsWindowTracker extends BaseWindowTracker { if (best) { this.resetTrackingLossState(); this.targetWindowMinimized = false; + this.currentTargetWindowHwnd = best.hwnd; this.updateTargetWindowFocused(best.focused); this.updateGeometry(best.geometry); return; @@ -154,18 +161,21 @@ export class WindowsWindowTracker extends BaseWindowTracker { if (result.windowState === 'minimized') { this.targetWindowMinimized = true; + this.currentTargetWindowHwnd = null; this.updateTargetWindowFocused(false); this.registerTrackingMiss(this.minimizedTrackingLossGraceMs); return; } this.targetWindowMinimized = false; + this.currentTargetWindowHwnd = null; this.updateTargetWindowFocused(false); this.registerTrackingMiss(); } catch (error: unknown) { const err = error instanceof Error ? error : new Error(String(error)); this.maybeLogPollError(err); this.targetWindowMinimized = false; + this.currentTargetWindowHwnd = null; this.updateTargetWindowFocused(false); this.registerTrackingMiss(); } finally {