fix: address claude review feedback on overlay refactor

This commit is contained in:
2026-02-26 18:47:51 -08:00
parent 75442a4648
commit a03388a38f
28 changed files with 95 additions and 197 deletions

View File

@@ -50,7 +50,7 @@ function createMockWindow(): MockWindow & {
url: 'file:///overlay/index.html?layer=modal',
loadCallbacks: [],
};
return {
const window = {
...state,
isDestroyed: () => state.destroyed,
isVisible: () => state.visible,
@@ -92,6 +92,29 @@ function createMockWindow(): MockWindow & {
},
},
};
Object.defineProperty(window, 'loading', {
get: () => state.loading,
set: (value: boolean) => {
state.loading = value;
},
});
Object.defineProperty(window, 'url', {
get: () => state.url,
set: (value: string) => {
state.url = value;
},
});
Object.defineProperty(window, 'ignoreMouseEvents', {
get: () => state.ignoreMouseEvents,
set: (value: boolean) => {
state.ignoreMouseEvents = value;
},
});
return window;
}
test('sendToActiveOverlayWindow targets modal window with full geometry and tracks close restore', () => {

View File

@@ -1,6 +1,8 @@
import type { BrowserWindow } from 'electron';
import type { WindowGeometry } from '../types';
const MODAL_REVEAL_FALLBACK_DELAY_MS = 250;
type OverlayHostedModal = 'runtime-options' | 'subsync' | 'jimaku' | 'kiku';
export interface OverlayWindowResolver {
@@ -67,14 +69,7 @@ export function createOverlayModalRuntimeService(
if (window.webContents.isLoading()) {
return false;
}
const getURL = window.webContents.getURL;
if (typeof getURL !== 'function') {
return true;
}
const currentURL = getURL.call(window.webContents);
return currentURL !== '' && currentURL !== 'about:blank';
return window.webContents.getURL() !== '' && window.webContents.getURL() !== 'about:blank';
};
const sendOrQueueForWindow = (
@@ -142,7 +137,7 @@ export function createOverlayModalRuntimeService(
return;
}
showModalWindow(targetWindow, { passThroughMouseEvents: true });
}, 250);
}, MODAL_REVEAL_FALLBACK_DELAY_MS);
};
const sendToActiveOverlayWindow = (
@@ -208,8 +203,6 @@ export function createOverlayModalRuntimeService(
if (restoreVisibleOverlayOnModalClose.size === 0) {
clearPendingModalWindowReveal();
notifyModalStateChange(false);
}
if (restoreVisibleOverlayOnModalClose.size === 0) {
modalWindow.hide();
}
};

View File

@@ -12,7 +12,7 @@ test('on will quit cleanup handler runs all cleanup steps', () => {
destroyTray: () => calls.push('destroy-tray'),
stopConfigHotReload: () => calls.push('stop-config'),
restorePreviousSecondarySubVisibility: () => calls.push('restore-sub'),
restoreMpvSubVisibilityForInvisibleOverlay: () => calls.push('restore-mpv-sub'),
restoreMpvSubVisibility: () => calls.push('restore-mpv-sub'),
unregisterAllGlobalShortcuts: () => calls.push('unregister-shortcuts'),
stopSubtitleWebsocket: () => calls.push('stop-ws'),
stopTexthookerService: () => calls.push('stop-texthooker'),

View File

@@ -2,7 +2,7 @@ export function createOnWillQuitCleanupHandler(deps: {
destroyTray: () => void;
stopConfigHotReload: () => void;
restorePreviousSecondarySubVisibility: () => void;
restoreMpvSubVisibilityForInvisibleOverlay: () => void;
restoreMpvSubVisibility: () => void;
unregisterAllGlobalShortcuts: () => void;
stopSubtitleWebsocket: () => void;
stopTexthookerService: () => void;
@@ -26,7 +26,7 @@ export function createOnWillQuitCleanupHandler(deps: {
deps.destroyTray();
deps.stopConfigHotReload();
deps.restorePreviousSecondarySubVisibility();
deps.restoreMpvSubVisibilityForInvisibleOverlay();
deps.restoreMpvSubVisibility();
deps.unregisterAllGlobalShortcuts();
deps.stopSubtitleWebsocket();
deps.stopTexthookerService();

View File

@@ -14,7 +14,7 @@ test('cleanup deps builder returns handlers that guard optional runtime objects'
destroyTray: () => calls.push('destroy-tray'),
stopConfigHotReload: () => calls.push('stop-config'),
restorePreviousSecondarySubVisibility: () => calls.push('restore-sub'),
restoreMpvSubVisibilityForInvisibleOverlay: () => calls.push('restore-mpv-sub'),
restoreMpvSubVisibility: () => calls.push('restore-mpv-sub'),
unregisterAllGlobalShortcuts: () => calls.push('unregister-shortcuts'),
stopSubtitleWebsocket: () => calls.push('stop-ws'),
stopTexthookerService: () => calls.push('stop-texthooker'),
@@ -73,7 +73,7 @@ test('cleanup deps builder skips destroyed yomitan window', () => {
destroyTray: () => {},
stopConfigHotReload: () => {},
restorePreviousSecondarySubVisibility: () => {},
restoreMpvSubVisibilityForInvisibleOverlay: () => {},
restoreMpvSubVisibility: () => {},
unregisterAllGlobalShortcuts: () => {},
stopSubtitleWebsocket: () => {},
stopTexthookerService: () => {},

View File

@@ -21,7 +21,7 @@ export function createBuildOnWillQuitCleanupDepsHandler(deps: {
destroyTray: () => void;
stopConfigHotReload: () => void;
restorePreviousSecondarySubVisibility: () => void;
restoreMpvSubVisibilityForInvisibleOverlay: () => void;
restoreMpvSubVisibility: () => void;
unregisterAllGlobalShortcuts: () => void;
stopSubtitleWebsocket: () => void;
stopTexthookerService: () => void;
@@ -52,8 +52,8 @@ export function createBuildOnWillQuitCleanupDepsHandler(deps: {
destroyTray: () => deps.destroyTray(),
stopConfigHotReload: () => deps.stopConfigHotReload(),
restorePreviousSecondarySubVisibility: () => deps.restorePreviousSecondarySubVisibility(),
restoreMpvSubVisibilityForInvisibleOverlay: () =>
deps.restoreMpvSubVisibilityForInvisibleOverlay(),
restoreMpvSubVisibility: () =>
deps.restoreMpvSubVisibility(),
unregisterAllGlobalShortcuts: () => deps.unregisterAllGlobalShortcuts(),
stopSubtitleWebsocket: () => deps.stopSubtitleWebsocket(),
stopTexthookerService: () => deps.stopTexthookerService(),

View File

@@ -75,7 +75,7 @@ test('composeMpvRuntimeHandlers returns callable handlers and forwards to inject
onSubtitleChange: () => {},
refreshDiscordPresence: () => {},
updateCurrentMediaPath: () => {},
restoreMpvSubVisibilityForInvisibleOverlay: () => {},
restoreMpvSubVisibility: () => {},
getCurrentAnilistMediaKey: () => null,
resetAnilistMediaTracking: () => {},
maybeProbeAnilistDuration: () => {},

View File

@@ -16,7 +16,7 @@ test('composeStartupLifecycleHandlers returns callable startup lifecycle handler
destroyTray: () => {},
stopConfigHotReload: () => {},
restorePreviousSecondarySubVisibility: () => {},
restoreMpvSubVisibilityForInvisibleOverlay: () => {},
restoreMpvSubVisibility: () => {},
unregisterAllGlobalShortcuts: () => {},
stopSubtitleWebsocket: () => {},
stopTexthookerService: () => {},

View File

@@ -51,7 +51,7 @@ test('media path change handler reports stop for empty path and probes media key
const handler = createHandleMpvMediaPathChangeHandler({
updateCurrentMediaPath: (path) => calls.push(`path:${path}`),
reportJellyfinRemoteStopped: () => calls.push('stopped'),
restoreMpvSubVisibilityForInvisibleOverlay: () => calls.push('restore-mpv-sub'),
restoreMpvSubVisibility: () => calls.push('restore-mpv-sub'),
getCurrentAnilistMediaKey: () => 'show:1',
resetAnilistMediaTracking: (mediaKey) => calls.push(`reset:${String(mediaKey)}`),
maybeProbeAnilistDuration: (mediaKey) => calls.push(`probe:${mediaKey}`),

View File

@@ -33,7 +33,7 @@ export function createHandleMpvSecondarySubtitleChangeHandler(deps: {
export function createHandleMpvMediaPathChangeHandler(deps: {
updateCurrentMediaPath: (path: string) => void;
reportJellyfinRemoteStopped: () => void;
restoreMpvSubVisibilityForInvisibleOverlay: () => void;
restoreMpvSubVisibility: () => void;
getCurrentAnilistMediaKey: () => string | null;
resetAnilistMediaTracking: (mediaKey: string | null) => void;
maybeProbeAnilistDuration: (mediaKey: string) => void;
@@ -45,7 +45,7 @@ export function createHandleMpvMediaPathChangeHandler(deps: {
deps.updateCurrentMediaPath(path);
if (!path) {
deps.reportJellyfinRemoteStopped();
deps.restoreMpvSubVisibilityForInvisibleOverlay();
deps.restoreMpvSubVisibility();
}
const mediaKey = deps.getCurrentAnilistMediaKey();
deps.resetAnilistMediaTracking(mediaKey);

View File

@@ -36,7 +36,7 @@ test('main mpv event binder wires callbacks through to runtime deps', () => {
broadcastSecondarySubtitle: (text) => calls.push(`broadcast-secondary:${text}`),
updateCurrentMediaPath: (path) => calls.push(`media-path:${path}`),
restoreMpvSubVisibilityForInvisibleOverlay: () => calls.push('restore-mpv-sub'),
restoreMpvSubVisibility: () => calls.push('restore-mpv-sub'),
getCurrentAnilistMediaKey: () => 'media-key',
resetAnilistMediaTracking: (key) => calls.push(`reset-media:${String(key)}`),
maybeProbeAnilistDuration: (mediaKey) => calls.push(`probe:${mediaKey}`),

View File

@@ -43,7 +43,7 @@ export function createBindMpvMainEventHandlersHandler(deps: {
broadcastSecondarySubtitle: (text: string) => void;
updateCurrentMediaPath: (path: string) => void;
restoreMpvSubVisibilityForInvisibleOverlay: () => void;
restoreMpvSubVisibility: () => void;
getCurrentAnilistMediaKey: () => string | null;
resetAnilistMediaTracking: (mediaKey: string | null) => void;
maybeProbeAnilistDuration: (mediaKey: string) => void;
@@ -97,8 +97,8 @@ export function createBindMpvMainEventHandlersHandler(deps: {
const handleMpvMediaPathChange = createHandleMpvMediaPathChangeHandler({
updateCurrentMediaPath: (path) => deps.updateCurrentMediaPath(path),
reportJellyfinRemoteStopped: () => deps.reportJellyfinRemoteStopped(),
restoreMpvSubVisibilityForInvisibleOverlay: () =>
deps.restoreMpvSubVisibilityForInvisibleOverlay(),
restoreMpvSubVisibility: () =>
deps.restoreMpvSubVisibility(),
getCurrentAnilistMediaKey: () => deps.getCurrentAnilistMediaKey(),
resetAnilistMediaTracking: (mediaKey) => deps.resetAnilistMediaTracking(mediaKey),
maybeProbeAnilistDuration: (mediaKey) => deps.maybeProbeAnilistDuration(mediaKey),

View File

@@ -41,7 +41,7 @@ test('mpv main event main deps map app state updates and delegate callbacks', as
calls.push(`broadcast:${channel}:${String(payload)}`),
onSubtitleChange: (text) => calls.push(`subtitle-change:${text}`),
updateCurrentMediaPath: (path) => calls.push(`path:${path}`),
restoreMpvSubVisibilityForInvisibleOverlay: () => calls.push('restore-mpv-sub'),
restoreMpvSubVisibility: () => calls.push('restore-mpv-sub'),
getCurrentAnilistMediaKey: () => 'media-key',
resetAnilistMediaTracking: (mediaKey) => calls.push(`reset:${mediaKey}`),
maybeProbeAnilistDuration: (mediaKey) => calls.push(`probe:${mediaKey}`),
@@ -75,7 +75,7 @@ test('mpv main event main deps map app state updates and delegate callbacks', as
deps.broadcastSubtitleAss('ass');
deps.broadcastSecondarySubtitle('sec');
deps.updateCurrentMediaPath('/tmp/video');
deps.restoreMpvSubVisibilityForInvisibleOverlay();
deps.restoreMpvSubVisibility();
assert.equal(deps.getCurrentAnilistMediaKey(), 'media-key');
deps.resetAnilistMediaTracking('media-key');
deps.maybeProbeAnilistDuration('media-key');

View File

@@ -27,7 +27,7 @@ export function createBuildBindMpvMainEventHandlersMainDepsHandler(deps: {
broadcastToOverlayWindows: (channel: string, payload: unknown) => void;
onSubtitleChange: (text: string) => void;
updateCurrentMediaPath: (path: string) => void;
restoreMpvSubVisibilityForInvisibleOverlay: () => void;
restoreMpvSubVisibility: () => void;
getCurrentAnilistMediaKey: () => string | null;
resetAnilistMediaTracking: (mediaKey: string | null) => void;
maybeProbeAnilistDuration: (mediaKey: string) => void;
@@ -71,8 +71,8 @@ export function createBuildBindMpvMainEventHandlersMainDepsHandler(deps: {
broadcastSecondarySubtitle: (text: string) =>
deps.broadcastToOverlayWindows('secondary-subtitle:set', text),
updateCurrentMediaPath: (path: string) => deps.updateCurrentMediaPath(path),
restoreMpvSubVisibilityForInvisibleOverlay: () =>
deps.restoreMpvSubVisibilityForInvisibleOverlay(),
restoreMpvSubVisibility: () =>
deps.restoreMpvSubVisibility(),
getCurrentAnilistMediaKey: () => deps.getCurrentAnilistMediaKey(),
resetAnilistMediaTracking: (mediaKey: string | null) =>
deps.resetAnilistMediaTracking(mediaKey),

View File

@@ -8,14 +8,12 @@ import {
type VisibilityState = {
savedSubVisibility: boolean | null;
savedSecondarySubVisibility: boolean | null;
revision: number;
};
test('ensure overlay mpv subtitle suppression captures previous visibility then hides subtitles', async () => {
const state: VisibilityState = {
savedSubVisibility: null,
savedSecondarySubVisibility: null,
revision: 0,
};
const calls: boolean[] = [];
@@ -29,10 +27,6 @@ test('ensure overlay mpv subtitle suppression captures previous visibility then
setSavedSubVisibility: (visible) => {
state.savedSubVisibility = visible;
},
getSavedSecondarySubVisibility: () => state.savedSecondarySubVisibility,
setSavedSecondarySubVisibility: (visible) => {
state.savedSecondarySubVisibility = visible;
},
getRevision: () => state.revision,
setRevision: (revision) => {
state.revision = revision;
@@ -40,24 +34,19 @@ test('ensure overlay mpv subtitle suppression captures previous visibility then
setMpvSubVisibility: (visible) => {
calls.push(visible);
},
setMpvSecondarySubVisibility: (visible) => {
calls.push(visible);
},
logWarn: () => {},
});
await ensureHidden();
assert.equal(state.savedSubVisibility, false);
assert.equal(state.savedSecondarySubVisibility, false);
assert.equal(state.revision, 1);
assert.deepEqual(calls, [false, false]);
assert.deepEqual(calls, [false]);
});
test('restore overlay mpv subtitle suppression restores saved visibility', () => {
const state: VisibilityState = {
savedSubVisibility: false,
savedSecondarySubVisibility: true,
revision: 4,
};
const calls: boolean[] = [];
@@ -67,10 +56,6 @@ test('restore overlay mpv subtitle suppression restores saved visibility', () =>
setSavedSubVisibility: (visible) => {
state.savedSubVisibility = visible;
},
getSavedSecondarySubVisibility: () => state.savedSecondarySubVisibility,
setSavedSecondarySubVisibility: (visible) => {
state.savedSecondarySubVisibility = visible;
},
getRevision: () => state.revision,
setRevision: (revision) => {
state.revision = revision;
@@ -80,23 +65,18 @@ test('restore overlay mpv subtitle suppression restores saved visibility', () =>
setMpvSubVisibility: (visible) => {
calls.push(visible);
},
setMpvSecondarySubVisibility: (visible) => {
calls.push(visible);
},
});
restore();
assert.equal(state.savedSubVisibility, null);
assert.equal(state.savedSecondarySubVisibility, null);
assert.equal(state.revision, 5);
assert.deepEqual(calls, [false, true]);
assert.deepEqual(calls, [false]);
});
test('restore keeps mpv subtitles hidden when visible-overlay binding still requires suppression', () => {
const state: VisibilityState = {
savedSubVisibility: true,
savedSecondarySubVisibility: true,
revision: 9,
};
const calls: boolean[] = [];
@@ -106,10 +86,6 @@ test('restore keeps mpv subtitles hidden when visible-overlay binding still requ
setSavedSubVisibility: (visible) => {
state.savedSubVisibility = visible;
},
getSavedSecondarySubVisibility: () => state.savedSecondarySubVisibility,
setSavedSecondarySubVisibility: (visible) => {
state.savedSecondarySubVisibility = visible;
},
getRevision: () => state.revision,
setRevision: (revision) => {
state.revision = revision;
@@ -119,23 +95,18 @@ test('restore keeps mpv subtitles hidden when visible-overlay binding still requ
setMpvSubVisibility: (visible) => {
calls.push(visible);
},
setMpvSecondarySubVisibility: (visible) => {
calls.push(visible);
},
});
restore();
assert.equal(state.savedSubVisibility, true);
assert.equal(state.savedSecondarySubVisibility, true);
assert.equal(state.revision, 10);
assert.deepEqual(calls, [false, false]);
assert.deepEqual(calls, [false]);
});
test('restore defers mpv subtitle restore while mpv is disconnected', () => {
const state: VisibilityState = {
savedSubVisibility: true,
savedSecondarySubVisibility: false,
revision: 2,
};
const calls: boolean[] = [];
@@ -145,10 +116,6 @@ test('restore defers mpv subtitle restore while mpv is disconnected', () => {
setSavedSubVisibility: (visible) => {
state.savedSubVisibility = visible;
},
getSavedSecondarySubVisibility: () => state.savedSecondarySubVisibility,
setSavedSecondarySubVisibility: (visible) => {
state.savedSecondarySubVisibility = visible;
},
getRevision: () => state.revision,
setRevision: (revision) => {
state.revision = revision;
@@ -158,9 +125,6 @@ test('restore defers mpv subtitle restore while mpv is disconnected', () => {
setMpvSubVisibility: (visible) => {
calls.push(visible);
},
setMpvSecondarySubVisibility: (visible) => {
calls.push(visible);
},
});
restore();

View File

@@ -3,10 +3,6 @@ type MpvVisibilityClient = {
requestProperty: (name: string) => Promise<unknown>;
};
type RestoreOptions = {
respectVisibleOverlayBinding?: boolean;
};
function parseSubVisibility(value: unknown): boolean {
if (typeof value === 'string') {
const normalized = value.trim().toLowerCase();
@@ -33,12 +29,9 @@ export function createEnsureOverlayMpvSubtitlesHiddenHandler(deps: {
getMpvClient: () => MpvVisibilityClient | null;
getSavedSubVisibility: () => boolean | null;
setSavedSubVisibility: (visible: boolean | null) => void;
getSavedSecondarySubVisibility: () => boolean | null;
setSavedSecondarySubVisibility: (visible: boolean | null) => void;
getRevision: () => number;
setRevision: (revision: number) => void;
setMpvSubVisibility: (visible: boolean) => void;
setMpvSecondarySubVisibility: (visible: boolean) => void;
logWarn: (message: string, error: unknown) => void;
}) {
return async (): Promise<void> => {
@@ -50,9 +43,17 @@ export function createEnsureOverlayMpvSubtitlesHiddenHandler(deps: {
return;
}
if (deps.getSavedSubVisibility() === null) {
const shouldCaptureSavedVisibility = deps.getSavedSubVisibility() === null;
const savedVisibilityPromise = shouldCaptureSavedVisibility
? mpvClient.requestProperty('sub-visibility')
: null;
// Hide immediately on overlay toggle; capture/restore logic is handled separately.
deps.setMpvSubVisibility(false);
if (shouldCaptureSavedVisibility && savedVisibilityPromise) {
try {
const currentSubVisibility = await mpvClient.requestProperty('sub-visibility');
const currentSubVisibility = await savedVisibilityPromise;
if (revision !== deps.getRevision()) {
return;
}
@@ -68,64 +69,28 @@ export function createEnsureOverlayMpvSubtitlesHiddenHandler(deps: {
deps.setSavedSubVisibility(true);
}
}
if (deps.getSavedSecondarySubVisibility() === null) {
try {
const currentSecondarySubVisibility = await mpvClient.requestProperty('secondary-sub-visibility');
if (revision !== deps.getRevision()) {
return;
}
deps.setSavedSecondarySubVisibility(parseSubVisibility(currentSecondarySubVisibility));
} catch (error) {
if (revision !== deps.getRevision()) {
return;
}
deps.logWarn(
'[overlay] Failed to capture secondary mpv sub-visibility; falling back to visible restore',
error,
);
deps.setSavedSecondarySubVisibility(true);
}
}
if (revision !== deps.getRevision()) {
return;
}
deps.setMpvSubVisibility(false);
deps.setMpvSecondarySubVisibility(false);
};
}
export function createRestoreOverlayMpvSubtitlesHandler(deps: {
getSavedSubVisibility: () => boolean | null;
setSavedSubVisibility: (visible: boolean | null) => void;
getSavedSecondarySubVisibility: () => boolean | null;
setSavedSecondarySubVisibility: (visible: boolean | null) => void;
getRevision: () => number;
setRevision: (revision: number) => void;
isMpvConnected: () => boolean;
shouldKeepSuppressedFromVisibleOverlayBinding: () => boolean;
setMpvSubVisibility: (visible: boolean) => void;
setMpvSecondarySubVisibility: (visible: boolean) => void;
}) {
return (options?: RestoreOptions): void => {
return (): void => {
deps.setRevision(deps.getRevision() + 1);
const savedVisibility = deps.getSavedSubVisibility();
const respectVisibleOverlayBinding = options?.respectVisibleOverlayBinding ?? true;
if (
respectVisibleOverlayBinding &&
deps.shouldKeepSuppressedFromVisibleOverlayBinding()
) {
if (deps.shouldKeepSuppressedFromVisibleOverlayBinding()) {
deps.setMpvSubVisibility(false);
deps.setMpvSecondarySubVisibility(false);
return;
}
const hasSecondarySavedVisibility = deps.getSavedSecondarySubVisibility() !== null;
if (savedVisibility === null && !hasSecondarySavedVisibility) {
if (savedVisibility === null) {
return;
}
@@ -136,12 +101,7 @@ export function createRestoreOverlayMpvSubtitlesHandler(deps: {
if (savedVisibility !== null) {
deps.setMpvSubVisibility(savedVisibility);
}
const savedSecondaryVisibility = deps.getSavedSecondarySubVisibility();
if (savedSecondaryVisibility !== null) {
deps.setMpvSecondarySubVisibility(savedSecondaryVisibility);
}
deps.setSavedSubVisibility(null);
deps.setSavedSecondarySubVisibility(null);
};
}

View File

@@ -172,7 +172,6 @@ export interface AppState {
lastSecondarySubToggleAtMs: number;
previousSecondarySubVisibility: boolean | null;
overlaySavedMpvSubVisibility: boolean | null;
overlaySavedSecondaryMpvSubVisibility: boolean | null;
overlayMpvSubVisibilityRevision: number;
mpvSubtitleRenderMetrics: MpvSubtitleRenderMetrics;
shortcutsRegistered: boolean;
@@ -247,7 +246,6 @@ export function createAppState(values: AppStateInitialValues): AppState {
lastSecondarySubToggleAtMs: 0,
previousSecondarySubVisibility: null,
overlaySavedMpvSubVisibility: null,
overlaySavedSecondaryMpvSubVisibility: null,
overlayMpvSubVisibilityRevision: 0,
mpvSubtitleRenderMetrics: {
...DEFAULT_MPV_SUBTITLE_RENDER_METRICS,