fix: address PR #31 follow-up review comments

This commit is contained in:
2026-03-23 21:14:38 -07:00
parent cdb128272d
commit 5f6f93cd19
5 changed files with 179 additions and 35 deletions

View File

@@ -947,6 +947,7 @@ async function runYoutubePlaybackFlowMain(request: {
source: CliCommandSource; source: CliCommandSource;
}): Promise<void> { }): Promise<void> {
youtubePrimarySubtitleNotificationRuntime.setAppOwnedFlowInFlight(true); youtubePrimarySubtitleNotificationRuntime.setAppOwnedFlowInFlight(true);
try {
if (process.platform === 'win32' && !appState.mpvClient?.connected) { if (process.platform === 'win32' && !appState.mpvClient?.connected) {
const launchResult = launchWindowsMpv( const launchResult = launchWindowsMpv(
[request.url], [request.url],
@@ -969,8 +970,6 @@ async function runYoutubePlaybackFlowMain(request: {
if (!appState.mpvClient?.connected) { if (!appState.mpvClient?.connected) {
appState.mpvClient?.connect(); appState.mpvClient?.connect();
} }
try {
await youtubeFlowRuntime.runYoutubePlaybackFlow({ await youtubeFlowRuntime.runYoutubePlaybackFlow({
url: request.url, url: request.url,
mode: request.mode, mode: request.mode,
@@ -1263,6 +1262,10 @@ function reportYoutubeSubtitleFailure(message: string): void {
} }
async function openYoutubeTrackPickerFromPlayback(): Promise<void> { async function openYoutubeTrackPickerFromPlayback(): Promise<void> {
if (youtubeFlowRuntime.hasActiveSession()) {
showMpvOsd('YouTube subtitle flow already in progress.');
return;
}
const currentMediaPath = const currentMediaPath =
appState.currentMediaPath?.trim() || appState.mpvClient?.currentVideoPath?.trim() || ''; appState.currentMediaPath?.trim() || appState.mpvClient?.currentVideoPath?.trim() || '';
if (!isYoutubePlaybackActiveNow() || !currentMediaPath) { if (!isYoutubePlaybackActiveNow() || !currentMediaPath) {

View File

@@ -82,6 +82,28 @@ test('notifier suppresses failure when preferred primary subtitle is selected',
assert.deepEqual(notifications, []); assert.deepEqual(notifications, []);
}); });
test('notifier suppresses failure when selected track is marked active before sid arrives', () => {
const notifications: string[] = [];
const timers = createTimerHarness();
const runtime = createYoutubePrimarySubtitleNotificationRuntime({
getPrimarySubtitleLanguages: () => ['ja', 'jpn'],
notifyFailure: (message) => {
notifications.push(message);
},
schedule: (fn) => timers.schedule(fn),
clearSchedule: (timer) => timers.clear(timer),
});
runtime.handleMediaPathChange('https://www.youtube.com/watch?v=abc');
runtime.handleSubtitleTrackChange(null);
runtime.handleSubtitleTrackListChange([
{ type: 'sub', id: 5, lang: 'ja', title: 'Japanese', external: false, selected: true },
]);
timers.runAll();
assert.deepEqual(notifications, []);
});
test('notifier suppresses failure when any external subtitle track is selected', () => { test('notifier suppresses failure when any external subtitle track is selected', () => {
const notifications: string[] = []; const notifications: string[] = [];
const timers = createTimerHarness(); const timers = createTimerHarness();

View File

@@ -8,6 +8,7 @@ type SubtitleTrackEntry = {
type: string; type: string;
lang: string; lang: string;
external: boolean; external: boolean;
selected: boolean;
}; };
function parseTrackId(value: unknown): number | null { function parseTrackId(value: unknown): number | null {
@@ -31,6 +32,7 @@ function normalizeTrack(entry: unknown): SubtitleTrackEntry | null {
type: String(track.type || '').trim(), type: String(track.type || '').trim(),
lang: String(track.lang || '').trim(), lang: String(track.lang || '').trim(),
external: track.external === true, external: track.external === true,
selected: track.selected === true,
}; };
} }
@@ -74,12 +76,15 @@ function hasSelectedPrimarySubtitle(
trackList: unknown[] | null, trackList: unknown[] | null,
preferredLanguages: Set<string>, preferredLanguages: Set<string>,
): boolean { ): boolean {
if (sid === null || !Array.isArray(trackList)) { if (!Array.isArray(trackList)) {
return false; return false;
} }
const tracks = trackList.map(normalizeTrack);
const activeTrack = const activeTrack =
trackList.map(normalizeTrack).find((track) => track?.type === 'sub' && track.id === sid) ?? null; (sid === null ? null : tracks.find((track) => track?.type === 'sub' && track.id === sid) ?? null) ??
tracks.find((track) => track?.type === 'sub' && track.selected) ??
null;
if (!activeTrack) { if (!activeTrack) {
return false; return false;
} }

View File

@@ -35,6 +35,9 @@ function createFakeElement() {
append(...children: any[]) { append(...children: any[]) {
this.children.push(...children); this.children.push(...children);
}, },
replaceChildren(...children: any[]) {
this.children = [...children];
},
addEventListener(type: string, listener: (event?: any) => void) { addEventListener(type: string, listener: (event?: any) => void) {
const existing = this.listeners.get(type) ?? []; const existing = this.listeners.get(type) ?? [];
existing.push(listener); existing.push(listener);
@@ -763,3 +766,109 @@ test('youtube track picker ignores immediate Enter after open before allowing ke
Object.defineProperty(globalThis, 'document', { configurable: true, value: originalDocument }); Object.defineProperty(globalThis, 'document', { configurable: true, value: originalDocument });
} }
}); });
test('youtube track picker uses track list as the source of truth for available selections', async () => {
const resolveCalls: Array<{
sessionId: string;
action: string;
primaryTrackId: string | null;
secondaryTrackId: string | null;
}> = [];
const originalWindow = globalThis.window;
const originalDocument = globalThis.document;
Object.defineProperty(globalThis, 'document', {
configurable: true,
value: {
createElement: () => createFakeElement(),
},
});
Object.defineProperty(globalThis, 'window', {
configurable: true,
value: {
dispatchEvent: () => true,
focus: () => {},
electronAPI: {
notifyOverlayModalOpened: () => {},
notifyOverlayModalClosed: () => {},
youtubePickerResolve: async (payload: {
sessionId: string;
action: string;
primaryTrackId: string | null;
secondaryTrackId: string | null;
}) => {
resolveCalls.push(payload);
return { ok: true, message: '' };
},
setIgnoreMouseEvents: () => {},
},
},
});
try {
const state = createRendererState();
const dom = {
overlay: {
classList: createClassList(),
focus: () => {},
},
youtubePickerModal: createFakeElement(),
youtubePickerTitle: createFakeElement(),
youtubePickerPrimarySelect: createFakeElement(),
youtubePickerSecondarySelect: createFakeElement(),
youtubePickerTracks: createFakeElement(),
youtubePickerStatus: createFakeElement(),
youtubePickerContinueButton: createFakeElement(),
youtubePickerCloseButton: createFakeElement(),
};
const modal = createYoutubeTrackPickerModal(
{
state,
dom,
platform: {
shouldToggleMouseIgnore: false,
},
} as never,
{
modalStateReader: { isAnyModalOpen: () => true },
restorePointerInteractionState: () => {},
syncSettingsModalSubtitleSuppression: () => {},
},
);
modal.openYoutubePickerModal({
sessionId: 'yt-1',
url: 'https://example.com',
tracks: [
{
id: 'manual:ja',
language: 'ja',
sourceLanguage: 'ja',
kind: 'manual',
label: 'Japanese',
},
],
defaultPrimaryTrackId: 'manual:ja',
defaultSecondaryTrackId: null,
hasTracks: false,
});
modal.wireDomEvents();
const listeners = dom.youtubePickerContinueButton.listeners.get('click') ?? [];
await Promise.all(listeners.map((listener) => listener()));
assert.deepEqual(resolveCalls, [
{
sessionId: 'yt-1',
action: 'use-selected',
primaryTrackId: 'manual:ja',
secondaryTrackId: null,
},
]);
} finally {
Object.defineProperty(globalThis, 'window', { configurable: true, value: originalWindow });
Object.defineProperty(globalThis, 'document', { configurable: true, value: originalDocument });
}
});

View File

@@ -9,6 +9,10 @@ function createOption(value: string, label: string): HTMLOptionElement {
return option; return option;
} }
function payloadHasTracks(payload: YoutubePickerOpenPayload | null): boolean {
return (payload?.tracks.length ?? 0) > 0;
}
export function createYoutubeTrackPickerModal( export function createYoutubeTrackPickerModal(
ctx: RendererContext, ctx: RendererContext,
options: { options: {
@@ -34,9 +38,9 @@ export function createYoutubeTrackPickerModal(
} }
function renderTrackList(): void { function renderTrackList(): void {
ctx.dom.youtubePickerTracks.innerHTML = ''; ctx.dom.youtubePickerTracks.replaceChildren();
const payload = ctx.state.youtubePickerPayload; const payload = ctx.state.youtubePickerPayload;
if (!payload || payload.tracks.length === 0) { if (!payload || !payloadHasTracks(payload)) {
const li = document.createElement('li'); const li = document.createElement('li');
const left = document.createElement('span'); const left = document.createElement('span');
left.textContent = 'No subtitle tracks found'; left.textContent = 'No subtitle tracks found';
@@ -70,7 +74,7 @@ export function createYoutubeTrackPickerModal(
function syncSecondaryOptions(): void { function syncSecondaryOptions(): void {
const payload = ctx.state.youtubePickerPayload; const payload = ctx.state.youtubePickerPayload;
const primaryTrackId = ctx.dom.youtubePickerPrimarySelect.value || null; const primaryTrackId = ctx.dom.youtubePickerPrimarySelect.value || null;
ctx.dom.youtubePickerSecondarySelect.innerHTML = ''; ctx.dom.youtubePickerSecondarySelect.replaceChildren();
ctx.dom.youtubePickerSecondarySelect.appendChild(createOption('', 'None')); ctx.dom.youtubePickerSecondarySelect.appendChild(createOption('', 'None'));
if (!payload) return; if (!payload) return;
@@ -97,10 +101,10 @@ export function createYoutubeTrackPickerModal(
function applyPayload(payload: YoutubePickerOpenPayload): void { function applyPayload(payload: YoutubePickerOpenPayload): void {
ctx.state.youtubePickerPayload = payload; ctx.state.youtubePickerPayload = payload;
ctx.dom.youtubePickerTitle.textContent = `Select YouTube subtitles for ${payload.url}`; ctx.dom.youtubePickerTitle.textContent = `Select YouTube subtitles for ${payload.url}`;
ctx.dom.youtubePickerPrimarySelect.innerHTML = ''; ctx.dom.youtubePickerPrimarySelect.replaceChildren();
ctx.dom.youtubePickerSecondarySelect.innerHTML = ''; ctx.dom.youtubePickerSecondarySelect.replaceChildren();
if (payload.tracks.length === 0) { if (!payloadHasTracks(payload)) {
ctx.dom.youtubePickerPrimarySelect.appendChild(createOption('', 'No tracks available')); ctx.dom.youtubePickerPrimarySelect.appendChild(createOption('', 'No tracks available'));
ctx.dom.youtubePickerPrimarySelect.disabled = true; ctx.dom.youtubePickerPrimarySelect.disabled = true;
ctx.dom.youtubePickerSecondarySelect.disabled = true; ctx.dom.youtubePickerSecondarySelect.disabled = true;
@@ -128,7 +132,8 @@ export function createYoutubeTrackPickerModal(
} }
const payload = ctx.state.youtubePickerPayload; const payload = ctx.state.youtubePickerPayload;
if (!payload) return; if (!payload) return;
if (action === 'use-selected' && payload.hasTracks && !ctx.dom.youtubePickerPrimarySelect.value) { const hasTracks = payloadHasTracks(payload);
if (action === 'use-selected' && hasTracks && !ctx.dom.youtubePickerPrimarySelect.value) {
setStatus('Primary subtitle selection is required.', true); setStatus('Primary subtitle selection is required.', true);
return; return;
} }
@@ -160,7 +165,7 @@ export function createYoutubeTrackPickerModal(
} finally { } finally {
resolveSelectionInFlight = false; resolveSelectionInFlight = false;
const shouldKeepDisabled = const shouldKeepDisabled =
ctx.state.youtubePickerModalOpen && !(ctx.state.youtubePickerPayload?.hasTracks ?? false); ctx.state.youtubePickerModalOpen && !payloadHasTracks(ctx.state.youtubePickerPayload);
setResolveControlsDisabled(shouldKeepDisabled); setResolveControlsDisabled(shouldKeepDisabled);
} }
} }
@@ -233,7 +238,7 @@ export function createYoutubeTrackPickerModal(
return true; return true;
} }
void resolveSelection( void resolveSelection(
ctx.state.youtubePickerPayload?.hasTracks ? 'use-selected' : 'continue-without-subtitles', payloadHasTracks(ctx.state.youtubePickerPayload) ? 'use-selected' : 'continue-without-subtitles',
); );
return true; return true;
} }
@@ -264,7 +269,7 @@ export function createYoutubeTrackPickerModal(
ctx.dom.youtubePickerContinueButton.addEventListener('click', () => { ctx.dom.youtubePickerContinueButton.addEventListener('click', () => {
void resolveSelection( void resolveSelection(
ctx.state.youtubePickerPayload?.hasTracks ? 'use-selected' : 'continue-without-subtitles', payloadHasTracks(ctx.state.youtubePickerPayload) ? 'use-selected' : 'continue-without-subtitles',
); );
}); });