From 207151dba3f8cbe0301da3cf4f83bf1d60aa8965 Mon Sep 17 00:00:00 2001 From: sudacode Date: Mon, 23 Mar 2026 01:01:53 -0700 Subject: [PATCH] fix: address latest CodeRabbit review items --- src/main/runtime/youtube-playback.test.ts | 1 + src/main/runtime/youtube-playback.ts | 11 +- .../modals/youtube-track-picker.test.ts | 120 ++++++++++++++++++ src/renderer/modals/youtube-track-picker.ts | 38 ++++-- .../library/MediaDetailView.test.tsx | 2 + 5 files changed, 159 insertions(+), 13 deletions(-) diff --git a/src/main/runtime/youtube-playback.test.ts b/src/main/runtime/youtube-playback.test.ts index 6c98cdd..e8b4fec 100644 --- a/src/main/runtime/youtube-playback.test.ts +++ b/src/main/runtime/youtube-playback.test.ts @@ -12,6 +12,7 @@ test('isYoutubeMediaPath detects youtube watch and short urls', () => { test('isYoutubeMediaPath ignores local files and non-youtube urls', () => { assert.equal(isYoutubeMediaPath('/tmp/video.mkv'), false); assert.equal(isYoutubeMediaPath('https://example.com/watch?v=abc123'), false); + assert.equal(isYoutubeMediaPath('https://notyoutube.com/watch?v=abc123'), false); assert.equal(isYoutubeMediaPath(' '), false); assert.equal(isYoutubeMediaPath(null), false); }); diff --git a/src/main/runtime/youtube-playback.ts b/src/main/runtime/youtube-playback.ts index 5a09a34..7e21ba5 100644 --- a/src/main/runtime/youtube-playback.ts +++ b/src/main/runtime/youtube-playback.ts @@ -6,6 +6,10 @@ function trimToNull(value: string | null | undefined): string | null { return trimmed.length > 0 ? trimmed : null; } +function matchesYoutubeHost(hostname: string, expectedHost: string): boolean { + return hostname === expectedHost || hostname.endsWith(`.${expectedHost}`); +} + export function isYoutubeMediaPath(mediaPath: string | null | undefined): boolean { const normalized = trimToNull(mediaPath); if (!normalized) { @@ -21,10 +25,9 @@ export function isYoutubeMediaPath(mediaPath: string | null | undefined): boolea const host = parsed.hostname.toLowerCase(); return ( - host === 'youtu.be' || - host.endsWith('.youtu.be') || - host.endsWith('youtube.com') || - host.endsWith('youtube-nocookie.com') + matchesYoutubeHost(host, 'youtu.be') || + matchesYoutubeHost(host, 'youtube.com') || + matchesYoutubeHost(host, 'youtube-nocookie.com') ); } diff --git a/src/renderer/modals/youtube-track-picker.test.ts b/src/renderer/modals/youtube-track-picker.test.ts index 1233060..90cb5d7 100644 --- a/src/renderer/modals/youtube-track-picker.test.ts +++ b/src/renderer/modals/youtube-track-picker.test.ts @@ -349,6 +349,126 @@ test('youtube track picker surfaces rejected resolve calls as modal status', asy } }); +test('youtube track picker ignores duplicate resolve submissions while request is in flight', async () => { + const resolveCalls: Array<{ + sessionId: string; + action: string; + primaryTrackId: string | null; + secondaryTrackId: string | null; + }> = []; + const originalWindow = globalThis.window; + const originalDocument = globalThis.document; + let releaseResolve: (() => void) | null = null; + + 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); + await new Promise((resolve) => { + releaseResolve = resolve; + }); + 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', + mode: 'download', + tracks: [ + { + id: 'auto:ja-orig', + language: 'ja', + sourceLanguage: 'ja-orig', + kind: 'auto', + label: 'Japanese (auto)', + }, + ], + defaultPrimaryTrackId: 'auto:ja-orig', + defaultSecondaryTrackId: null, + hasTracks: true, + }); + modal.wireDomEvents(); + + const listeners = dom.youtubePickerContinueButton.listeners.get('click') ?? []; + const first = listeners[0]?.(); + const second = listeners[0]?.(); + await Promise.resolve(); + + assert.equal(resolveCalls.length, 1); + assert.equal(dom.youtubePickerPrimarySelect.disabled, true); + assert.equal(dom.youtubePickerSecondarySelect.disabled, true); + assert.equal(dom.youtubePickerContinueButton.disabled, true); + assert.equal(dom.youtubePickerCloseButton.disabled, true); + + assert.ok(releaseResolve); + const release = releaseResolve as () => void; + release(); + await Promise.all([first, second]); + + assert.equal(dom.youtubePickerPrimarySelect.disabled, false); + assert.equal(dom.youtubePickerSecondarySelect.disabled, false); + assert.equal(dom.youtubePickerContinueButton.disabled, false); + assert.equal(dom.youtubePickerCloseButton.disabled, false); + } finally { + Object.defineProperty(globalThis, 'window', { configurable: true, value: originalWindow }); + Object.defineProperty(globalThis, 'document', { configurable: true, value: originalDocument }); + } +}); + test('youtube track picker only consumes handled keys', async () => { const originalWindow = globalThis.window; const originalDocument = globalThis.document; diff --git a/src/renderer/modals/youtube-track-picker.ts b/src/renderer/modals/youtube-track-picker.ts index ec5dd11..3700116 100644 --- a/src/renderer/modals/youtube-track-picker.ts +++ b/src/renderer/modals/youtube-track-picker.ts @@ -17,6 +17,8 @@ export function createYoutubeTrackPickerModal( syncSettingsModalSubtitleSuppression: () => void; }, ) { + let resolveSelectionInFlight = false; + function setStatus(message: string, isError = false): void { ctx.state.youtubePickerStatus = message; ctx.dom.youtubePickerStatus.textContent = message; @@ -34,7 +36,12 @@ export function createYoutubeTrackPickerModal( const payload = ctx.state.youtubePickerPayload; if (!payload || payload.tracks.length === 0) { const li = document.createElement('li'); - li.innerHTML = 'No subtitle tracks foundContinue without subtitles'; + const left = document.createElement('span'); + left.textContent = 'No subtitle tracks found'; + const right = document.createElement('span'); + right.className = 'youtube-picker-track-meta'; + right.textContent = 'Continue without subtitles'; + li.append(left, right); ctx.dom.youtubePickerTracks.appendChild(li); return; } @@ -51,6 +58,13 @@ export function createYoutubeTrackPickerModal( } } + function setResolveControlsDisabled(disabled: boolean): void { + ctx.dom.youtubePickerPrimarySelect.disabled = disabled; + ctx.dom.youtubePickerSecondarySelect.disabled = disabled; + ctx.dom.youtubePickerContinueButton.disabled = disabled; + ctx.dom.youtubePickerCloseButton.disabled = disabled; + } + function syncSecondaryOptions(): void { const payload = ctx.state.youtubePickerPayload; const primaryTrackId = ctx.dom.youtubePickerPrimarySelect.value || null; @@ -107,6 +121,9 @@ export function createYoutubeTrackPickerModal( } async function resolveSelection(action: 'use-selected' | 'continue-without-subtitles'): Promise { + if (resolveSelectionInFlight) { + return; + } const payload = ctx.state.youtubePickerPayload; if (!payload) return; if (action === 'use-selected' && payload.hasTracks && !ctx.dom.youtubePickerPrimarySelect.value) { @@ -114,9 +131,10 @@ export function createYoutubeTrackPickerModal( return; } - let response; + resolveSelectionInFlight = true; + setResolveControlsDisabled(true); try { - response = + const response = action === 'use-selected' ? await window.electronAPI.youtubePickerResolve({ sessionId: payload.sessionId, @@ -130,15 +148,17 @@ export function createYoutubeTrackPickerModal( primaryTrackId: null, secondaryTrackId: null, }); + if (!response.ok) { + setStatus(response.message, true); + return; + } + closeYoutubePickerModal(); } catch (error) { setStatus(error instanceof Error ? error.message : String(error), true); - return; + } finally { + resolveSelectionInFlight = false; + setResolveControlsDisabled(false); } - if (!response.ok) { - setStatus(response.message, true); - return; - } - closeYoutubePickerModal(); } function openYoutubePickerModal(payload: YoutubePickerOpenPayload): void { diff --git a/stats/src/components/library/MediaDetailView.test.tsx b/stats/src/components/library/MediaDetailView.test.tsx index 51d35fc..accce1e 100644 --- a/stats/src/components/library/MediaDetailView.test.tsx +++ b/stats/src/components/library/MediaDetailView.test.tsx @@ -5,6 +5,7 @@ import { getRelatedCollectionLabel } from './MediaDetailView'; test('getRelatedCollectionLabel returns View Channel for youtube-backed media', () => { assert.equal( getRelatedCollectionLabel({ + videoId: 1, animeId: 1, canonicalTitle: 'Video', totalSessions: 1, @@ -24,6 +25,7 @@ test('getRelatedCollectionLabel returns View Channel for youtube-backed media', test('getRelatedCollectionLabel returns View Anime for non-youtube media', () => { assert.equal( getRelatedCollectionLabel({ + videoId: 2, animeId: 1, canonicalTitle: 'Episode 5', totalSessions: 1,