From c9ce337c1a75b7ce4d49390364e0e70b39502be5 Mon Sep 17 00:00:00 2001 From: Kyle Date: Fri, 10 Apr 2026 19:22:01 -0700 Subject: [PATCH] fix: address latest CodeRabbit review --- src/main.ts | 41 ++++++++----------- src/main/overlay-runtime.ts | 3 ++ .../runtime/app-lifecycle-actions.test.ts | 4 +- src/main/runtime/app-lifecycle-actions.ts | 2 + .../app-lifecycle-main-cleanup.test.ts | 1 + src/main/runtime/runtime-options-open.test.ts | 17 ++++++-- src/main/runtime/runtime-options-open.ts | 20 ++++++--- 7 files changed, 54 insertions(+), 34 deletions(-) diff --git a/src/main.ts b/src/main.ts index c947fde5..96abf216 100644 --- a/src/main.ts +++ b/src/main.ts @@ -133,13 +133,12 @@ import { } from './logger'; import { createWindowTracker as createWindowTrackerCore } from './window-trackers'; import { - bindWindowsOverlayAboveMpvNative, - clearWindowsOverlayOwnerNative, - ensureWindowsOverlayTransparencyNative, - getWindowsForegroundProcessNameNative, - queryWindowsTargetWindowHandle, - setWindowsOverlayOwnerNative, - shouldUseWindowsTrackerPowershellFallback, + bindWindowsOverlayAboveMpv, + clearWindowsOverlayOwner, + ensureWindowsOverlayTransparency, + findWindowsMpvTargetWindowHandle, + getWindowsForegroundProcessName, + setWindowsOverlayOwner, } from './window-trackers/windows-helper'; import { commandNeedsOverlayStartupPrereqs, @@ -1940,18 +1939,9 @@ function resolveWindowsOverlayBindTargetHandle(targetMpvSocketPath?: string | nu return null; } - if (shouldUseWindowsTrackerPowershellFallback()) { - 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; + void targetMpvSocketPath; + return findWindowsMpvTargetWindowHandle(); } catch { return null; } @@ -1990,7 +1980,7 @@ async function syncWindowsVisibleOverlayToMpvZOrder(): Promise { const overlayHwnd = getWindowsNativeWindowHandleNumber(mainWindow); const targetWindowHwnd = resolveWindowsOverlayBindTargetHandle(appState.mpvSocketPath); - if (targetWindowHwnd !== null && bindWindowsOverlayAboveMpvNative(overlayHwnd, targetWindowHwnd)) { + if (targetWindowHwnd !== null && bindWindowsOverlayAboveMpv(overlayHwnd, targetWindowHwnd)) { (mainWindow as BrowserWindow & { setOpacity?: (opacity: number) => void }).setOpacity?.(1); return true; } @@ -2082,7 +2072,7 @@ function maybePollWindowsVisibleOverlayForegroundProcess(): void { return; } - const processName = getWindowsForegroundProcessNameNative(); + const processName = getWindowsForegroundProcessName(); const normalizedProcessName = processName?.trim().toLowerCase() ?? null; const previousProcessName = lastWindowsVisibleOverlayForegroundProcessName; lastWindowsVisibleOverlayForegroundProcessName = normalizedProcessName; @@ -2234,6 +2224,9 @@ function openRuntimeOptionsPalette(): void { if (!opened) { showMpvOsd('Runtime options overlay unavailable.'); } + }).catch((error) => { + logger.error('Failed to open runtime options overlay.', error); + showMpvOsd('Runtime options overlay unavailable.'); }); } @@ -4106,7 +4099,7 @@ function createMainWindow(): BrowserWindow { const window = createMainWindowHandler(); if (process.platform === 'win32') { const overlayHwnd = getWindowsNativeWindowHandleNumber(window); - if (!ensureWindowsOverlayTransparencyNative(overlayHwnd)) { + if (!ensureWindowsOverlayTransparency(overlayHwnd)) { logger.warn('Failed to eagerly extend Windows overlay transparency via koffi'); } } @@ -5149,7 +5142,7 @@ const { initializeOverlayRuntime: initializeOverlayRuntimeHandler } = if (process.platform !== 'win32' || !mainWindow || mainWindow.isDestroyed()) return; const overlayHwnd = getWindowsNativeWindowHandleNumber(mainWindow); const targetWindowHwnd = resolveWindowsOverlayBindTargetHandle(appState.mpvSocketPath); - if (targetWindowHwnd !== null && bindWindowsOverlayAboveMpvNative(overlayHwnd, targetWindowHwnd)) { + if (targetWindowHwnd !== null && bindWindowsOverlayAboveMpv(overlayHwnd, targetWindowHwnd)) { return; } const tracker = appState.windowTracker; @@ -5166,7 +5159,7 @@ const { initializeOverlayRuntime: initializeOverlayRuntimeHandler } = })() : null; if (!mpvResult) return; - if (!setWindowsOverlayOwnerNative(overlayHwnd, mpvResult.hwnd)) { + if (!setWindowsOverlayOwner(overlayHwnd, mpvResult.hwnd)) { logger.warn('Failed to set overlay owner via koffi'); } }, @@ -5174,7 +5167,7 @@ const { initializeOverlayRuntime: initializeOverlayRuntimeHandler } = const mainWindow = overlayManager.getMainWindow(); if (process.platform !== 'win32' || !mainWindow || mainWindow.isDestroyed()) return; const overlayHwnd = getWindowsNativeWindowHandleNumber(mainWindow); - if (!clearWindowsOverlayOwnerNative(overlayHwnd)) { + if (!clearWindowsOverlayOwner(overlayHwnd)) { logger.warn('Failed to clear overlay owner via koffi'); } }, diff --git a/src/main/overlay-runtime.ts b/src/main/overlay-runtime.ts index 2a768c65..a14c4adb 100644 --- a/src/main/overlay-runtime.ts +++ b/src/main/overlay-runtime.ts @@ -89,6 +89,9 @@ export function createOverlayModalRuntimeService( }; const isWindowReadyForIpc = (window: BrowserWindow): boolean => { + if (window.isDestroyed()) { + return false; + } if (window.webContents.isLoading()) { return false; } diff --git a/src/main/runtime/app-lifecycle-actions.test.ts b/src/main/runtime/app-lifecycle-actions.test.ts index 4c08ead1..5fa4fd0e 100644 --- a/src/main/runtime/app-lifecycle-actions.test.ts +++ b/src/main/runtime/app-lifecycle-actions.test.ts @@ -16,6 +16,7 @@ test('on will quit cleanup handler runs all cleanup steps', () => { unregisterAllGlobalShortcuts: () => calls.push('unregister-shortcuts'), stopSubtitleWebsocket: () => calls.push('stop-ws'), stopTexthookerService: () => calls.push('stop-texthooker'), + clearWindowsVisibleOverlayForegroundPollLoop: () => calls.push('clear-windows-visible-overlay-poll'), destroyMainOverlayWindow: () => calls.push('destroy-main-overlay-window'), destroyModalOverlayWindow: () => calls.push('destroy-modal-overlay-window'), destroyYomitanParserWindow: () => calls.push('destroy-yomitan-window'), @@ -40,9 +41,10 @@ test('on will quit cleanup handler runs all cleanup steps', () => { }); cleanup(); - assert.equal(calls.length, 28); + assert.equal(calls.length, 29); assert.equal(calls[0], 'destroy-tray'); assert.equal(calls[calls.length - 1], 'stop-discord-presence'); + assert.ok(calls.includes('clear-windows-visible-overlay-poll')); assert.ok(calls.indexOf('flush-mpv-log') < calls.indexOf('destroy-socket')); }); diff --git a/src/main/runtime/app-lifecycle-actions.ts b/src/main/runtime/app-lifecycle-actions.ts index 4dd08be7..5e90c0e7 100644 --- a/src/main/runtime/app-lifecycle-actions.ts +++ b/src/main/runtime/app-lifecycle-actions.ts @@ -6,6 +6,7 @@ export function createOnWillQuitCleanupHandler(deps: { unregisterAllGlobalShortcuts: () => void; stopSubtitleWebsocket: () => void; stopTexthookerService: () => void; + clearWindowsVisibleOverlayForegroundPollLoop: () => void; destroyMainOverlayWindow: () => void; destroyModalOverlayWindow: () => void; destroyYomitanParserWindow: () => void; @@ -36,6 +37,7 @@ export function createOnWillQuitCleanupHandler(deps: { deps.unregisterAllGlobalShortcuts(); deps.stopSubtitleWebsocket(); deps.stopTexthookerService(); + deps.clearWindowsVisibleOverlayForegroundPollLoop(); deps.destroyMainOverlayWindow(); deps.destroyModalOverlayWindow(); deps.destroyYomitanParserWindow(); diff --git a/src/main/runtime/app-lifecycle-main-cleanup.test.ts b/src/main/runtime/app-lifecycle-main-cleanup.test.ts index 48290773..e2bd0b3e 100644 --- a/src/main/runtime/app-lifecycle-main-cleanup.test.ts +++ b/src/main/runtime/app-lifecycle-main-cleanup.test.ts @@ -87,6 +87,7 @@ test('cleanup deps builder returns handlers that guard optional runtime objects' assert.ok(calls.includes('destroy-yomitan-settings-window')); assert.ok(calls.includes('stop-jellyfin-remote')); assert.ok(calls.includes('stop-discord-presence')); + assert.ok(calls.includes('clear-windows-visible-overlay-foreground-poll-loop')); assert.equal(reconnectTimer, null); assert.equal(immersionTracker, null); }); diff --git a/src/main/runtime/runtime-options-open.test.ts b/src/main/runtime/runtime-options-open.test.ts index 55a750c4..28ec2ac1 100644 --- a/src/main/runtime/runtime-options-open.test.ts +++ b/src/main/runtime/runtime-options-open.test.ts @@ -21,7 +21,11 @@ test('runtime options open prefers dedicated modal window on first attempt', asy }); return true; }, - waitForModalOpen: async () => true, + waitForModalOpen: async (modal, timeoutMs) => { + assert.equal(modal, 'runtime-options'); + assert.equal(timeoutMs, 1500); + return true; + }, logWarn: () => { throw new Error('should not warn on first-attempt success'); }, @@ -52,7 +56,9 @@ test('runtime options open retries after an open timeout', async () => { }); return true; }, - waitForModalOpen: async () => { + waitForModalOpen: async (modal, timeoutMs) => { + assert.equal(modal, 'runtime-options'); + assert.equal(timeoutMs, 1500); waitCalls += 1; return waitCalls === 2; }, @@ -76,13 +82,18 @@ test('runtime options open retries after an open timeout', async () => { }); test('runtime options open fails when no overlay window can be targeted', async () => { + let waitCalls = 0; const opened = await openRuntimeOptionsModal({ ensureOverlayStartupPrereqs: () => {}, ensureOverlayWindowsReadyForVisibilityActions: () => {}, sendToActiveOverlayWindow: () => false, - waitForModalOpen: async () => true, + waitForModalOpen: async () => { + waitCalls += 1; + return true; + }, logWarn: () => {}, }); assert.equal(opened, false); + assert.equal(waitCalls, 0); }); diff --git a/src/main/runtime/runtime-options-open.ts b/src/main/runtime/runtime-options-open.ts index aa3f1665..3296cfd3 100644 --- a/src/main/runtime/runtime-options-open.ts +++ b/src/main/runtime/runtime-options-open.ts @@ -1,4 +1,5 @@ import type { OverlayHostedModal } from '../../shared/ipc/contracts'; +import { openOverlayHostedModal } from './overlay-hosted-modal-open'; const RUNTIME_OPTIONS_MODAL: OverlayHostedModal = 'runtime-options'; const RUNTIME_OPTIONS_OPEN_TIMEOUT_MS = 1500; @@ -18,12 +19,19 @@ export async function openRuntimeOptionsModal(deps: { 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, - }); + return openOverlayHostedModal( + { + ensureOverlayStartupPrereqs: deps.ensureOverlayStartupPrereqs, + ensureOverlayWindowsReadyForVisibilityActions: + deps.ensureOverlayWindowsReadyForVisibilityActions, + sendToActiveOverlayWindow: deps.sendToActiveOverlayWindow, + }, + { + channel: 'runtime-options:open', + modal: RUNTIME_OPTIONS_MODAL, + preferModalWindow: true, + }, + ); }; if (!sendOpen()) {