fix: address coderabbit review findings

This commit is contained in:
2026-03-15 14:26:30 -07:00
parent 5c31be99b5
commit 87bf3cef0c
18 changed files with 330 additions and 65 deletions

View File

@@ -71,6 +71,8 @@ test('mpv event bindings register all expected events', () => {
onSubtitleChange: () => {},
onSubtitleAssChange: () => {},
onSecondarySubtitleChange: () => {},
onSubtitleTrackChange: () => {},
onSubtitleTrackListChange: () => {},
onSubtitleTiming: () => {},
onMediaPathChange: () => {},
onMediaTitleChange: () => {},
@@ -92,6 +94,8 @@ test('mpv event bindings register all expected events', () => {
'subtitle-change',
'subtitle-ass-change',
'secondary-subtitle-change',
'subtitle-track-change',
'subtitle-track-list-change',
'subtitle-timing',
'media-path-change',
'media-title-change',

View File

@@ -3,6 +3,8 @@ type MpvBindingEventName =
| 'subtitle-change'
| 'subtitle-ass-change'
| 'secondary-subtitle-change'
| 'subtitle-track-change'
| 'subtitle-track-list-change'
| 'subtitle-timing'
| 'media-path-change'
| 'media-title-change'
@@ -69,6 +71,8 @@ export function createBindMpvClientEventHandlers(deps: {
onSubtitleChange: (payload: { text: string }) => void;
onSubtitleAssChange: (payload: { text: string }) => void;
onSecondarySubtitleChange: (payload: { text: string }) => void;
onSubtitleTrackChange: (payload: { sid: number | null }) => void;
onSubtitleTrackListChange: (payload: { trackList: unknown[] | null }) => void;
onSubtitleTiming: (payload: { text: string; start: number; end: number }) => void;
onMediaPathChange: (payload: { path: string | null }) => void;
onMediaTitleChange: (payload: { title: string | null }) => void;
@@ -83,6 +87,8 @@ export function createBindMpvClientEventHandlers(deps: {
mpvClient.on('subtitle-change', deps.onSubtitleChange);
mpvClient.on('subtitle-ass-change', deps.onSubtitleAssChange);
mpvClient.on('secondary-subtitle-change', deps.onSecondarySubtitleChange);
mpvClient.on('subtitle-track-change', deps.onSubtitleTrackChange);
mpvClient.on('subtitle-track-list-change', deps.onSubtitleTrackListChange);
mpvClient.on('subtitle-timing', deps.onSubtitleTiming);
mpvClient.on('media-path-change', deps.onMediaPathChange);
mpvClient.on('media-title-change', deps.onMediaTitleChange);

View File

@@ -34,6 +34,8 @@ test('main mpv event binder wires callbacks through to runtime deps', () => {
setCurrentSubAssText: (text) => calls.push(`set-ass:${text}`),
broadcastSubtitleAss: (text) => calls.push(`broadcast-ass:${text}`),
broadcastSecondarySubtitle: (text) => calls.push(`broadcast-secondary:${text}`),
onSubtitleTrackChange: () => calls.push('subtitle-track-change'),
onSubtitleTrackListChange: () => calls.push('subtitle-track-list-change'),
updateCurrentMediaPath: (path) => calls.push(`media-path:${path}`),
restoreMpvSubVisibility: () => calls.push('restore-mpv-sub'),
@@ -65,6 +67,8 @@ test('main mpv event binder wires callbacks through to runtime deps', () => {
});
handlers.get('subtitle-change')?.({ text: 'line' });
handlers.get('subtitle-track-change')?.({ sid: 3 });
handlers.get('subtitle-track-list-change')?.({ trackList: [] });
handlers.get('media-path-change')?.({ path: '' });
handlers.get('media-title-change')?.({ title: 'Episode 1' });
handlers.get('time-pos-change')?.({ time: 2.5 });
@@ -73,6 +77,8 @@ test('main mpv event binder wires callbacks through to runtime deps', () => {
assert.ok(calls.includes('set-sub:line'));
assert.ok(calls.includes('broadcast-sub:line'));
assert.ok(calls.includes('subtitle-change:line'));
assert.ok(calls.includes('subtitle-track-change'));
assert.ok(calls.includes('subtitle-track-list-change'));
assert.ok(calls.includes('media-title:Episode 1'));
assert.ok(calls.includes('restore-mpv-sub'));
assert.ok(calls.includes('reset-guess-state'));

View File

@@ -42,6 +42,8 @@ export function createBindMpvMainEventHandlersHandler(deps: {
setCurrentSubAssText: (text: string) => void;
broadcastSubtitleAss: (text: string) => void;
broadcastSecondarySubtitle: (text: string) => void;
onSubtitleTrackChange?: (sid: number | null) => void;
onSubtitleTrackListChange?: (trackList: unknown[] | null) => void;
updateCurrentMediaPath: (path: string) => void;
restoreMpvSubVisibility: () => void;
@@ -146,6 +148,8 @@ export function createBindMpvMainEventHandlersHandler(deps: {
onSubtitleChange: handleMpvSubtitleChange,
onSubtitleAssChange: handleMpvSubtitleAssChange,
onSecondarySubtitleChange: handleMpvSecondarySubtitleChange,
onSubtitleTrackChange: ({ sid }) => deps.onSubtitleTrackChange?.(sid),
onSubtitleTrackListChange: ({ trackList }) => deps.onSubtitleTrackListChange?.(trackList),
onSubtitleTiming: handleMpvSubtitleTiming,
onMediaPathChange: handleMpvMediaPathChange,
onMediaTitleChange: handleMpvMediaTitleChange,

View File

@@ -35,6 +35,8 @@ export function createBuildBindMpvMainEventHandlersMainDepsHandler(deps: {
logSubtitleTimingError: (message: string, error: unknown) => void;
broadcastToOverlayWindows: (channel: string, payload: unknown) => void;
onSubtitleChange: (text: string) => void;
onSubtitleTrackChange?: (sid: number | null) => void;
onSubtitleTrackListChange?: (trackList: unknown[] | null) => void;
updateCurrentMediaPath: (path: string) => void;
restoreMpvSubVisibility: () => void;
getCurrentAnilistMediaKey: () => string | null;
@@ -101,6 +103,12 @@ export function createBuildBindMpvMainEventHandlersMainDepsHandler(deps: {
broadcastSubtitle: (payload: { text: string; tokens: null }) =>
deps.broadcastToOverlayWindows('subtitle:set', payload),
onSubtitleChange: (text: string) => deps.onSubtitleChange(text),
onSubtitleTrackChange: deps.onSubtitleTrackChange
? (sid: number | null) => deps.onSubtitleTrackChange!(sid)
: undefined,
onSubtitleTrackListChange: deps.onSubtitleTrackListChange
? (trackList: unknown[] | null) => deps.onSubtitleTrackListChange!(trackList)
: undefined,
refreshDiscordPresence: () => deps.refreshDiscordPresence(),
setCurrentSubAssText: (text: string) => {
deps.appState.currentSubAssText = text;

View File

@@ -0,0 +1,41 @@
import assert from 'node:assert/strict';
import test from 'node:test';
import {
getActiveExternalSubtitleSource,
resolveSubtitleSourcePath,
} from './subtitle-prefetch-source';
test('getActiveExternalSubtitleSource returns the active external subtitle path', () => {
const source = getActiveExternalSubtitleSource(
[
{ type: 'sub', id: 1, external: false },
{ type: 'sub', id: 2, external: true, 'external-filename': ' https://host/subs.ass ' },
],
'2',
);
assert.equal(source, 'https://host/subs.ass');
});
test('getActiveExternalSubtitleSource returns null when the selected track is not external', () => {
const source = getActiveExternalSubtitleSource(
[{ type: 'sub', id: 2, external: false, 'external-filename': '/tmp/subs.ass' }],
2,
);
assert.equal(source, null);
});
test('resolveSubtitleSourcePath converts file URLs with spaces into filesystem paths', () => {
const fileUrl = process.platform === 'win32'
? 'file:///C:/Users/test/Sub%20Folder/subs.ass'
: 'file:///tmp/Sub%20Folder/subs.ass';
const resolved = resolveSubtitleSourcePath(fileUrl);
assert.ok(resolved.endsWith('/Sub Folder/subs.ass') || resolved.endsWith('\\Sub Folder\\subs.ass'));
});
test('resolveSubtitleSourcePath leaves non-file sources unchanged', () => {
assert.equal(resolveSubtitleSourcePath('/tmp/subs.ass'), '/tmp/subs.ass');
});

View File

@@ -0,0 +1,34 @@
import { fileURLToPath } from 'node:url';
export function getActiveExternalSubtitleSource(
trackListRaw: unknown,
sidRaw: unknown,
): string | null {
if (!Array.isArray(trackListRaw) || sidRaw == null) {
return null;
}
const sid =
typeof sidRaw === 'number' ? sidRaw : typeof sidRaw === 'string' ? Number(sidRaw) : null;
if (sid == null || !Number.isFinite(sid)) {
return null;
}
const activeTrack = trackListRaw.find((entry: unknown) => {
if (!entry || typeof entry !== 'object') {
return false;
}
const track = entry as Record<string, unknown>;
return track.type === 'sub' && track.id === sid && track.external === true;
}) as Record<string, unknown> | undefined;
const externalFilename =
typeof activeTrack?.['external-filename'] === 'string'
? activeTrack['external-filename'].trim()
: '';
return externalFilename || null;
}
export function resolveSubtitleSourcePath(source: string): string {
return source.startsWith('file://') ? fileURLToPath(new URL(source)) : source;
}