From 26cb4704f17b2429f3e3a4eccd73ddbe9eb50c46 Mon Sep 17 00:00:00 2001 From: Kyle Date: Fri, 10 Apr 2026 19:11:16 -0700 Subject: [PATCH] fix: address CodeRabbit review and ci --- src/core/services/overlay-visibility.ts | 3 +- src/core/services/overlay-window-flags.ts | 1 + src/core/services/overlay-window.ts | 3 +- src/main.ts | 53 ++++++----- src/main/overlay-runtime.test.ts | 12 +-- src/main/overlay-runtime.ts | 2 +- .../app-lifecycle-main-cleanup.test.ts | 3 + .../runtime/app-lifecycle-main-cleanup.ts | 3 + .../startup-lifecycle-composer.test.ts | 1 + src/main/runtime/runtime-options-open.test.ts | 88 +++++++++++++++++++ src/main/runtime/runtime-options-open.ts | 45 ++++++++++ src/window-trackers/windows-helper.test.ts | 36 ++++++++ src/window-trackers/windows-helper.ts | 15 ++++ src/window-trackers/windows-tracker.ts | 17 ++-- 14 files changed, 239 insertions(+), 43 deletions(-) create mode 100644 src/core/services/overlay-window-flags.ts create mode 100644 src/main/runtime/runtime-options-open.test.ts create mode 100644 src/main/runtime/runtime-options-open.ts diff --git a/src/core/services/overlay-visibility.ts b/src/core/services/overlay-visibility.ts index 0e1cf005..f106783e 100644 --- a/src/core/services/overlay-visibility.ts +++ b/src/core/services/overlay-visibility.ts @@ -1,14 +1,13 @@ import type { BrowserWindow } from 'electron'; import { BaseWindowTracker } from '../../window-trackers'; import { WindowGeometry } from '../../types'; +import { OVERLAY_WINDOW_CONTENT_READY_FLAG } from './overlay-window-flags'; const WINDOWS_OVERLAY_REVEAL_DELAY_MS = 48; const pendingWindowsOverlayRevealTimeoutByWindow = new WeakMap< BrowserWindow, ReturnType >(); -const OVERLAY_WINDOW_CONTENT_READY_FLAG = '__subminerOverlayContentReady'; - function setOverlayWindowOpacity(window: BrowserWindow, opacity: number): void { const opacityCapableWindow = window as BrowserWindow & { setOpacity?: (opacity: number) => void; diff --git a/src/core/services/overlay-window-flags.ts b/src/core/services/overlay-window-flags.ts new file mode 100644 index 00000000..d8c53372 --- /dev/null +++ b/src/core/services/overlay-window-flags.ts @@ -0,0 +1 @@ +export const OVERLAY_WINDOW_CONTENT_READY_FLAG = '__subminerOverlayContentReady'; diff --git a/src/core/services/overlay-window.ts b/src/core/services/overlay-window.ts index 62bcc34c..29406ef8 100644 --- a/src/core/services/overlay-window.ts +++ b/src/core/services/overlay-window.ts @@ -10,11 +10,12 @@ import { } from './overlay-window-input'; import { buildOverlayWindowOptions } from './overlay-window-options'; import { normalizeOverlayWindowBoundsForPlatform } from './overlay-window-bounds'; +import { OVERLAY_WINDOW_CONTENT_READY_FLAG } from './overlay-window-flags'; +export { OVERLAY_WINDOW_CONTENT_READY_FLAG } from './overlay-window-flags'; const logger = createLogger('main:overlay-window'); const overlayWindowLayerByInstance = new WeakMap(); const overlayWindowContentReady = new WeakSet(); -const OVERLAY_WINDOW_CONTENT_READY_FLAG = '__subminerOverlayContentReady'; export function isOverlayWindowContentReady(window: BrowserWindow): boolean { if (window.isDestroyed()) { diff --git a/src/main.ts b/src/main.ts index 24f82fd2..c947fde5 100644 --- a/src/main.ts +++ b/src/main.ts @@ -137,9 +137,9 @@ import { clearWindowsOverlayOwnerNative, ensureWindowsOverlayTransparencyNative, getWindowsForegroundProcessNameNative, - queryWindowsForegroundProcessName, queryWindowsTargetWindowHandle, setWindowsOverlayOwnerNative, + shouldUseWindowsTrackerPowershellFallback, } from './window-trackers/windows-helper'; import { commandNeedsOverlayStartupPrereqs, @@ -453,6 +453,7 @@ import { handleCliCommandRuntimeServiceWithContext } from './main/cli-runtime'; import { createOverlayModalRuntimeService } from './main/overlay-runtime'; import { createOverlayModalInputState } from './main/runtime/overlay-modal-input-state'; import { openYoutubeTrackPicker } from './main/runtime/youtube-picker-open'; +import { openRuntimeOptionsModal as openRuntimeOptionsModalRuntime } from './main/runtime/runtime-options-open'; import { createPlaylistBrowserIpcRuntime } from './main/runtime/playlist-browser-ipc'; import { writeSessionBindingsArtifact } from './main/runtime/session-bindings-artifact'; import { openOverlayHostedModal } from './main/runtime/overlay-hosted-modal-open'; @@ -1903,7 +1904,6 @@ let windowsVisibleOverlayZOrderRetryTimeouts: Array | null = null; -let windowsVisibleOverlayForegroundPollInFlight = false; let lastWindowsVisibleOverlayForegroundProcessName: string | null = null; let lastWindowsVisibleOverlayBlurredAtMs = 0; @@ -1940,9 +1940,11 @@ function resolveWindowsOverlayBindTargetHandle(targetMpvSocketPath?: string | nu return null; } - const helperTargetHwnd = queryWindowsTargetWindowHandle({ targetMpvSocketPath }); - if (helperTargetHwnd !== null) { - return helperTargetHwnd; + if (shouldUseWindowsTrackerPowershellFallback()) { + const helperTargetHwnd = queryWindowsTargetWindowHandle({ targetMpvSocketPath }); + if (helperTargetHwnd !== null) { + return helperTargetHwnd; + } } try { @@ -2103,6 +2105,15 @@ function ensureWindowsVisibleOverlayForegroundPollLoop(): void { }, WINDOWS_VISIBLE_OVERLAY_FOREGROUND_POLL_INTERVAL_MS); } +function clearWindowsVisibleOverlayForegroundPollLoop(): void { + if (windowsVisibleOverlayForegroundPollInterval === null) { + return; + } + + clearInterval(windowsVisibleOverlayForegroundPollInterval); + windowsVisibleOverlayForegroundPollInterval = null; +} + function scheduleVisibleOverlayBlurRefresh(): void { if (process.platform !== 'win32') { return; @@ -2211,23 +2222,19 @@ function setOverlayDebugVisualizationEnabled(enabled: boolean): void { } function openRuntimeOptionsPalette(): void { - const opened = openOverlayHostedModal( - { - ensureOverlayStartupPrereqs: () => ensureOverlayStartupPrereqs(), - ensureOverlayWindowsReadyForVisibilityActions: () => - ensureOverlayWindowsReadyForVisibilityActions(), - sendToActiveOverlayWindow: (channel, payload, runtimeOptions) => - sendToActiveOverlayWindow(channel, payload, runtimeOptions), - }, - { - channel: IPC_CHANNELS.event.runtimeOptionsOpen, - modal: 'runtime-options', - preferModalWindow: true, - }, - ); - if (!opened) { - showMpvOsd('Runtime options overlay unavailable.'); - } + void openRuntimeOptionsModalRuntime({ + ensureOverlayStartupPrereqs: () => ensureOverlayStartupPrereqs(), + ensureOverlayWindowsReadyForVisibilityActions: () => + ensureOverlayWindowsReadyForVisibilityActions(), + sendToActiveOverlayWindow: (channel, payload, runtimeOptions) => + sendToActiveOverlayWindow(channel, payload, runtimeOptions), + waitForModalOpen: (modal, timeoutMs) => overlayModalRuntime.waitForModalOpen(modal, timeoutMs), + logWarn: (message) => logger.warn(message), + }).then((opened) => { + if (!opened) { + showMpvOsd('Runtime options overlay unavailable.'); + } + }); } function openJimakuOverlay(): void { @@ -3034,6 +3041,8 @@ const { annotationSubtitleWsService.stop(); }, stopTexthookerService: () => texthookerService.stop(), + clearWindowsVisibleOverlayForegroundPollLoop: () => + clearWindowsVisibleOverlayForegroundPollLoop(), getMainOverlayWindow: () => overlayManager.getMainWindow(), clearMainOverlayWindow: () => overlayManager.setMainWindow(null), getModalOverlayWindow: () => overlayManager.getModalWindow(), diff --git a/src/main/overlay-runtime.test.ts b/src/main/overlay-runtime.test.ts index 307b3650..3f2e17dd 100644 --- a/src/main/overlay-runtime.test.ts +++ b/src/main/overlay-runtime.test.ts @@ -492,7 +492,7 @@ test('notifyOverlayModalOpened enables input on visible main overlay window when assert.equal(mainWindow.webContentsFocused, true); }); -test('handleOverlayModalClosed resets modal state even when modal window does not exist', () => { +test('handleOverlayModalClosed is a no-op when no modal window can be targeted', () => { const state: boolean[] = []; const runtime = createOverlayModalRuntimeService( { @@ -509,13 +509,14 @@ test('handleOverlayModalClosed resets modal state even when modal window does no }, ); - runtime.sendToActiveOverlayWindow('runtime-options:open', undefined, { + const sent = runtime.sendToActiveOverlayWindow('runtime-options:open', undefined, { restoreOnModalClose: 'runtime-options', }); + assert.equal(sent, false); runtime.notifyOverlayModalOpened('runtime-options'); runtime.handleOverlayModalClosed('runtime-options'); - assert.deepEqual(state, [true, false]); + assert.deepEqual(state, []); }); test('handleOverlayModalClosed hides modal window for single kiku modal', () => { @@ -637,10 +638,11 @@ test('warm modal window reopen becomes interactive immediately on the second ope }); test('waitForModalOpen resolves true after modal acknowledgement', async () => { + const modalWindow = createMockWindow(); const runtime = createOverlayModalRuntimeService({ getMainWindow: () => null, - getModalWindow: () => null, - createModalWindow: () => null, + getModalWindow: () => modalWindow as never, + createModalWindow: () => modalWindow as never, getModalGeometry: () => ({ x: 0, y: 0, width: 400, height: 300 }), setModalWindowBounds: () => {}, }); diff --git a/src/main/overlay-runtime.ts b/src/main/overlay-runtime.ts index 1e3a5720..2a768c65 100644 --- a/src/main/overlay-runtime.ts +++ b/src/main/overlay-runtime.ts @@ -1,9 +1,9 @@ import type { BrowserWindow } from 'electron'; import type { OverlayHostedModal } from '../shared/ipc/contracts'; import type { WindowGeometry } from '../types'; +import { OVERLAY_WINDOW_CONTENT_READY_FLAG } from '../core/services/overlay-window-flags'; const MODAL_REVEAL_FALLBACK_DELAY_MS = 250; -const OVERLAY_WINDOW_CONTENT_READY_FLAG = '__subminerOverlayContentReady'; export interface OverlayWindowResolver { getMainWindow: () => BrowserWindow | null; diff --git a/src/main/runtime/app-lifecycle-main-cleanup.test.ts b/src/main/runtime/app-lifecycle-main-cleanup.test.ts index d6814385..48290773 100644 --- a/src/main/runtime/app-lifecycle-main-cleanup.test.ts +++ b/src/main/runtime/app-lifecycle-main-cleanup.test.ts @@ -18,6 +18,8 @@ test('cleanup deps builder returns handlers that guard optional runtime objects' unregisterAllGlobalShortcuts: () => calls.push('unregister-shortcuts'), stopSubtitleWebsocket: () => calls.push('stop-ws'), stopTexthookerService: () => calls.push('stop-texthooker'), + clearWindowsVisibleOverlayForegroundPollLoop: () => + calls.push('clear-windows-visible-overlay-foreground-poll-loop'), getMainOverlayWindow: () => ({ isDestroyed: () => false, destroy: () => calls.push('destroy-main-overlay-window'), @@ -99,6 +101,7 @@ test('cleanup deps builder skips destroyed yomitan window', () => { unregisterAllGlobalShortcuts: () => {}, stopSubtitleWebsocket: () => {}, stopTexthookerService: () => {}, + clearWindowsVisibleOverlayForegroundPollLoop: () => {}, getMainOverlayWindow: () => ({ isDestroyed: () => true, destroy: () => calls.push('destroy-main-overlay-window'), diff --git a/src/main/runtime/app-lifecycle-main-cleanup.ts b/src/main/runtime/app-lifecycle-main-cleanup.ts index 803a0a18..6d4bbb9a 100644 --- a/src/main/runtime/app-lifecycle-main-cleanup.ts +++ b/src/main/runtime/app-lifecycle-main-cleanup.ts @@ -25,6 +25,7 @@ export function createBuildOnWillQuitCleanupDepsHandler(deps: { unregisterAllGlobalShortcuts: () => void; stopSubtitleWebsocket: () => void; stopTexthookerService: () => void; + clearWindowsVisibleOverlayForegroundPollLoop: () => void; getMainOverlayWindow: () => DestroyableWindow | null; clearMainOverlayWindow: () => void; getModalOverlayWindow: () => DestroyableWindow | null; @@ -64,6 +65,8 @@ export function createBuildOnWillQuitCleanupDepsHandler(deps: { unregisterAllGlobalShortcuts: () => deps.unregisterAllGlobalShortcuts(), stopSubtitleWebsocket: () => deps.stopSubtitleWebsocket(), stopTexthookerService: () => deps.stopTexthookerService(), + clearWindowsVisibleOverlayForegroundPollLoop: () => + deps.clearWindowsVisibleOverlayForegroundPollLoop(), destroyMainOverlayWindow: () => { const window = deps.getMainOverlayWindow(); if (!window) return; diff --git a/src/main/runtime/composers/startup-lifecycle-composer.test.ts b/src/main/runtime/composers/startup-lifecycle-composer.test.ts index 0c2cf22c..f3ddad99 100644 --- a/src/main/runtime/composers/startup-lifecycle-composer.test.ts +++ b/src/main/runtime/composers/startup-lifecycle-composer.test.ts @@ -21,6 +21,7 @@ test('composeStartupLifecycleHandlers returns callable startup lifecycle handler unregisterAllGlobalShortcuts: () => {}, stopSubtitleWebsocket: () => {}, stopTexthookerService: () => {}, + clearWindowsVisibleOverlayForegroundPollLoop: () => {}, getMainOverlayWindow: () => null, clearMainOverlayWindow: () => {}, getModalOverlayWindow: () => null, diff --git a/src/main/runtime/runtime-options-open.test.ts b/src/main/runtime/runtime-options-open.test.ts new file mode 100644 index 00000000..55a750c4 --- /dev/null +++ b/src/main/runtime/runtime-options-open.test.ts @@ -0,0 +1,88 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { openRuntimeOptionsModal } from './runtime-options-open'; + +test('runtime options open prefers dedicated modal window on first attempt', async () => { + const calls: string[] = []; + + const opened = await openRuntimeOptionsModal({ + ensureOverlayStartupPrereqs: () => { + calls.push('ensure-startup'); + }, + ensureOverlayWindowsReadyForVisibilityActions: () => { + calls.push('ensure-windows'); + }, + sendToActiveOverlayWindow: (channel, payload, options) => { + calls.push(`send:${channel}`); + assert.equal(payload, undefined); + assert.deepEqual(options, { + restoreOnModalClose: 'runtime-options', + preferModalWindow: true, + }); + return true; + }, + waitForModalOpen: async () => true, + logWarn: () => { + throw new Error('should not warn on first-attempt success'); + }, + }); + + assert.equal(opened, true); + assert.deepEqual(calls, ['ensure-startup', 'ensure-windows', 'send:runtime-options:open']); +}); + +test('runtime options open retries after an open timeout', async () => { + const calls: string[] = []; + const warnings: string[] = []; + let waitCalls = 0; + + const opened = await openRuntimeOptionsModal({ + ensureOverlayStartupPrereqs: () => { + calls.push('ensure-startup'); + }, + ensureOverlayWindowsReadyForVisibilityActions: () => { + calls.push('ensure-windows'); + }, + sendToActiveOverlayWindow: (channel, payload, options) => { + calls.push(`send:${channel}`); + assert.equal(payload, undefined); + assert.deepEqual(options, { + restoreOnModalClose: 'runtime-options', + preferModalWindow: true, + }); + return true; + }, + waitForModalOpen: async () => { + waitCalls += 1; + return waitCalls === 2; + }, + logWarn: (message) => { + warnings.push(message); + }, + }); + + assert.equal(opened, true); + assert.deepEqual(calls, [ + 'ensure-startup', + 'ensure-windows', + 'send:runtime-options:open', + 'ensure-startup', + 'ensure-windows', + 'send:runtime-options:open', + ]); + assert.deepEqual(warnings, [ + 'Runtime options modal did not acknowledge modal open on first attempt; retrying dedicated modal window.', + ]); +}); + +test('runtime options open fails when no overlay window can be targeted', async () => { + const opened = await openRuntimeOptionsModal({ + ensureOverlayStartupPrereqs: () => {}, + ensureOverlayWindowsReadyForVisibilityActions: () => {}, + sendToActiveOverlayWindow: () => false, + waitForModalOpen: async () => true, + logWarn: () => {}, + }); + + assert.equal(opened, false); +}); diff --git a/src/main/runtime/runtime-options-open.ts b/src/main/runtime/runtime-options-open.ts new file mode 100644 index 00000000..aa3f1665 --- /dev/null +++ b/src/main/runtime/runtime-options-open.ts @@ -0,0 +1,45 @@ +import type { OverlayHostedModal } from '../../shared/ipc/contracts'; + +const RUNTIME_OPTIONS_MODAL: OverlayHostedModal = 'runtime-options'; +const RUNTIME_OPTIONS_OPEN_TIMEOUT_MS = 1500; + +export async function openRuntimeOptionsModal(deps: { + ensureOverlayStartupPrereqs: () => void; + ensureOverlayWindowsReadyForVisibilityActions: () => void; + sendToActiveOverlayWindow: ( + channel: string, + payload?: unknown, + runtimeOptions?: { + restoreOnModalClose?: OverlayHostedModal; + preferModalWindow?: boolean; + }, + ) => boolean; + waitForModalOpen: (modal: OverlayHostedModal, timeoutMs: number) => Promise; + logWarn: (message: string) => void; +}): Promise { + const sendOpen = (): boolean => { + deps.ensureOverlayStartupPrereqs(); + deps.ensureOverlayWindowsReadyForVisibilityActions(); + return deps.sendToActiveOverlayWindow('runtime-options:open', undefined, { + restoreOnModalClose: RUNTIME_OPTIONS_MODAL, + preferModalWindow: true, + }); + }; + + if (!sendOpen()) { + return false; + } + + if (await deps.waitForModalOpen(RUNTIME_OPTIONS_MODAL, RUNTIME_OPTIONS_OPEN_TIMEOUT_MS)) { + return true; + } + + deps.logWarn( + 'Runtime options modal did not acknowledge modal open on first attempt; retrying dedicated modal window.', + ); + if (!sendOpen()) { + return false; + } + + return await deps.waitForModalOpen(RUNTIME_OPTIONS_MODAL, RUNTIME_OPTIONS_OPEN_TIMEOUT_MS); +} diff --git a/src/window-trackers/windows-helper.test.ts b/src/window-trackers/windows-helper.test.ts index b690d0d2..ea08532d 100644 --- a/src/window-trackers/windows-helper.test.ts +++ b/src/window-trackers/windows-helper.test.ts @@ -1,5 +1,6 @@ import test from 'node:test'; import assert from 'node:assert/strict'; +import * as windowsHelper from './windows-helper'; import { lowerWindowsOverlayInZOrder, parseWindowTrackerHelperForegroundProcess, @@ -204,6 +205,41 @@ test('queryWindowsTargetWindowHandle resolves the selected hwnd from the powersh assert.deepEqual(capturedArgs, ['\\\\.\\pipe\\subminer-socket']); }); +test('shouldUseWindowsTrackerPowershellFallback returns true for explicit powershell mode', () => { + assert.equal( + windowsHelper.shouldUseWindowsTrackerPowershellFallback({ + helperModeEnv: 'powershell', + }), + true, + ); +}); + +test('shouldUseWindowsTrackerPowershellFallback returns true for ps1 helper path override', () => { + assert.equal( + windowsHelper.shouldUseWindowsTrackerPowershellFallback({ + helperPathEnv: 'C:\\custom\\tracker.ps1', + }), + true, + ); +}); + +test('shouldUseWindowsTrackerPowershellFallback returns false for default and native modes', () => { + assert.equal( + windowsHelper.shouldUseWindowsTrackerPowershellFallback({ + helperModeEnv: 'auto', + helperPathEnv: undefined, + }), + false, + ); + assert.equal( + windowsHelper.shouldUseWindowsTrackerPowershellFallback({ + helperModeEnv: 'native', + helperPathEnv: 'C:\\custom\\tracker.exe', + }), + false, + ); +}); + test('resolveWindowsTrackerHelper auto mode prefers native helper when present', () => { const helper = resolveWindowsTrackerHelper({ dirname: 'C:\\repo\\dist\\window-trackers', diff --git a/src/window-trackers/windows-helper.ts b/src/window-trackers/windows-helper.ts index 50340c1d..856612a2 100644 --- a/src/window-trackers/windows-helper.ts +++ b/src/window-trackers/windows-helper.ts @@ -64,6 +64,21 @@ function normalizeHelperMode(value: string | undefined): WindowsTrackerHelperMod return 'auto'; } +export function shouldUseWindowsTrackerPowershellFallback(options: { + helperModeEnv?: string | undefined; + helperPathEnv?: string | undefined; +} = {}): boolean { + const mode = normalizeHelperMode( + options.helperModeEnv ?? process.env.SUBMINER_WINDOWS_TRACKER_HELPER, + ); + if (mode === 'powershell') { + return true; + } + + const helperPath = options.helperPathEnv ?? process.env.SUBMINER_WINDOWS_TRACKER_HELPER_PATH; + return helperPath?.trim().toLowerCase().endsWith('.ps1') ?? false; +} + function inferHelperKindFromPath(helperPath: string): WindowsTrackerHelperKind | null { const normalized = helperPath.trim().toLowerCase(); if (normalized.endsWith('.exe')) return 'native'; diff --git a/src/window-trackers/windows-tracker.ts b/src/window-trackers/windows-tracker.ts index c13c43f0..648d8de5 100644 --- a/src/window-trackers/windows-tracker.ts +++ b/src/window-trackers/windows-tracker.ts @@ -19,7 +19,10 @@ import { BaseWindowTracker } from './base-tracker'; import type { WindowGeometry } from '../types'; import type { MpvPollResult } from './win32'; -import { queryWindowsTrackerMpvWindows } from './windows-helper'; +import { + queryWindowsTrackerMpvWindows, + shouldUseWindowsTrackerPowershellFallback, +} from './windows-helper'; import { createLogger } from '../logger'; const log = createLogger('tracker').child('windows'); @@ -32,18 +35,8 @@ type WindowsTrackerDeps = { now?: () => number; }; -function shouldUsePowershellTrackerFallback(): boolean { - const helperMode = process.env.SUBMINER_WINDOWS_TRACKER_HELPER?.trim().toLowerCase(); - if (helperMode === 'powershell') { - return true; - } - - const helperPath = process.env.SUBMINER_WINDOWS_TRACKER_HELPER_PATH?.trim().toLowerCase(); - return helperPath?.endsWith('.ps1') ?? false; -} - function defaultPollMpvWindows(targetMpvSocketPath?: string | null): MpvPollResult { - if (targetMpvSocketPath && shouldUsePowershellTrackerFallback()) { + if (targetMpvSocketPath && shouldUseWindowsTrackerPowershellFallback()) { const helperResult = queryWindowsTrackerMpvWindows({ targetMpvSocketPath, });