fix: address PR #49 CodeRabbit follow-ups

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.

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)

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,

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

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

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(),

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;

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<typeof compileSessionBindings>['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',

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();
 

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,

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

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');
     });
   });

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 {