fix: address PR #31 latest review follow-ups

This commit is contained in:
2026-03-23 20:59:05 -07:00
parent a2716c5ce4
commit cdb128272d
9 changed files with 170 additions and 26 deletions

View File

@@ -5,7 +5,9 @@ area: launcher
- Kept startup playback gating tied only to primary subtitle load and tokenization readiness; secondary failures no longer block resume.
- Added a default `Ctrl+Alt+C` keybinding that opens the YouTube subtitle picker manually during active YouTube playback.
- Routed primary YouTube subtitle auto-load failures through the configured notification output path so failed startup still resumes cleanly.
- Suppressed false primary-subtitle failure notifications while app-owned YouTube subtitle probing and downloads are still in flight.
- Kept `youtubeSubgen.whisper*` settings as legacy compatibility configuration only; normal startup now resolves tracks through YouTube source download/selection.
- Fixed startup-launched YouTube playback so primary subtitle overlay updates continue after auto-load completes.
- Fixed auto-loaded YouTube primary subtitles so parsed cues appear in the subtitle sidebar without needing a manual picker retry.
- Kept the YouTube picker controls disabled when no subtitle tracks are available, even after a failed continue attempt.
- Prefer existing authoritative YouTube subtitle tracks when available, fall back to downloaded tracks only for missing sides, and keep native mpv secondary subtitle rendering hidden so the overlay remains the sole secondary display.

View File

@@ -85,6 +85,12 @@ function createContext(): LauncherCommandContext {
test('youtube playback launches overlay with app-owned youtube flow args', async () => {
const calls: string[] = [];
const context = createContext();
context.pluginRuntimeConfig = {
...context.pluginRuntimeConfig,
autoStart: false,
autoStartVisibleOverlay: false,
autoStartPauseUntilReady: false,
};
let receivedStartMpvOptions: Record<string, unknown> | null = null;
await runPlaybackCommandWithDeps(context, {

View File

@@ -246,6 +246,24 @@ test('handleCliCommand defaults youtube mode to download when omitted', () => {
]);
});
test('handleCliCommand reports youtube playback flow failures to logs and OSD', async () => {
const { deps, calls, osd } = createDeps({
runYoutubePlaybackFlow: async () => {
throw new Error('yt failed');
},
});
handleCliCommand(
makeArgs({ youtubePlay: 'https://youtube.com/watch?v=abc', youtubeMode: 'download' }),
'initial',
deps,
);
await new Promise((resolve) => setImmediate(resolve));
assert.ok(calls.some((value) => value.startsWith('error:runYoutubePlaybackFlow failed:')));
assert.ok(osd.includes('YouTube playback failed: yt failed'));
});
test('handleCliCommand reconnects MPV for second-instance --start when overlay runtime is already initialized', () => {
const { deps, calls } = createDeps({
isOverlayRuntimeInitialized: () => true,

View File

@@ -404,11 +404,18 @@ export function handleCliCommand(
deps.openJellyfinSetup();
deps.log('Opened Jellyfin setup flow.');
} else if (args.youtubePlay) {
void deps.runYoutubePlaybackFlow({
url: args.youtubePlay,
const youtubeUrl = args.youtubePlay;
runAsyncWithOsd(
() =>
deps.runYoutubePlaybackFlow({
url: youtubeUrl,
mode: args.youtubeMode ?? 'download',
source,
});
}),
deps,
'runYoutubePlaybackFlow',
'YouTube playback failed',
);
} else if (args.dictionary) {
const shouldStopAfterRun = source === 'initial' && !deps.hasMainWindow();
deps.log('Generating character dictionary for current anime...');

View File

@@ -946,6 +946,7 @@ async function runYoutubePlaybackFlowMain(request: {
mode: NonNullable<CliArgs['youtubeMode']>;
source: CliCommandSource;
}): Promise<void> {
youtubePrimarySubtitleNotificationRuntime.setAppOwnedFlowInFlight(true);
if (process.platform === 'win32' && !appState.mpvClient?.connected) {
const launchResult = launchWindowsMpv(
[request.url],
@@ -969,11 +970,15 @@ async function runYoutubePlaybackFlowMain(request: {
appState.mpvClient?.connect();
}
try {
await youtubeFlowRuntime.runYoutubePlaybackFlow({
url: request.url,
mode: request.mode,
});
logger.info(`YouTube playback flow completed from ${request.source}.`);
} finally {
youtubePrimarySubtitleNotificationRuntime.setAppOwnedFlowInFlight(false);
}
}
let firstRunSetupMessage: string | null = null;
@@ -3965,21 +3970,6 @@ function destroyTray(): void {
function initializeOverlayRuntime(): void {
initializeOverlayRuntimeHandler();
initializeOverlayAnkiIntegrationCore({
getResolvedConfig: () => getResolvedConfig(),
getSubtitleTimingTracker: () => appState.subtitleTimingTracker,
getMpvClient: () => appState.mpvClient,
getRuntimeOptionsManager: () => appState.runtimeOptionsManager,
getAnkiIntegration: () => appState.ankiIntegration,
setAnkiIntegration: (integration) => {
appState.ankiIntegration = integration as AnkiIntegration | null;
},
showDesktopNotification,
createFieldGroupingCallback: () => createFieldGroupingCallback(),
getKnownWordCacheStatePath: () => path.join(USER_DATA_PATH, 'known-words-cache.json'),
shouldStartAnkiIntegration: () =>
!(appState.initialArgs && isHeadlessInitialCommand(appState.initialArgs)),
});
appState.ankiIntegration?.setRecordCardsMinedCallback(recordTrackedCardsMined);
syncOverlayMpvSubtitleSuppression();
}

View File

@@ -147,3 +147,29 @@ test('notifier ignores empty and null media paths and waits for track list befor
timers.runAll();
assert.deepEqual(notifications, []);
});
test('notifier suppresses timer while app-owned youtube flow is still settling', () => {
const notifications: string[] = [];
const timers = createTimerHarness();
const runtime = createYoutubePrimarySubtitleNotificationRuntime({
getPrimarySubtitleLanguages: () => ['ja'],
notifyFailure: (message) => {
notifications.push(message);
},
schedule: (fn) => timers.schedule(fn),
clearSchedule: (timer) => timers.clear(timer),
});
runtime.setAppOwnedFlowInFlight(true);
runtime.handleMediaPathChange('https://www.youtube.com/watch?v=abc');
assert.equal(timers.size(), 0);
runtime.setAppOwnedFlowInFlight(false);
assert.equal(timers.size(), 1);
timers.runAll();
assert.deepEqual(notifications, [
'Primary subtitle failed to download or load. Try again from the subtitle modal.',
]);
});

View File

@@ -102,6 +102,7 @@ export function createYoutubePrimarySubtitleNotificationRuntime(deps: {
let currentTrackList: unknown[] | null = null;
let pendingTimer: YoutubePrimarySubtitleNotificationTimer | null = null;
let lastReportedMediaPath: string | null = null;
let appOwnedFlowInFlight = false;
const clearPendingTimer = (): void => {
deps.clearSchedule(pendingTimer);
@@ -129,6 +130,9 @@ export function createYoutubePrimarySubtitleNotificationRuntime(deps: {
const schedulePendingCheck = (): void => {
clearPendingTimer();
if (appOwnedFlowInFlight) {
return;
}
const mediaPath = currentMediaPath?.trim() || '';
if (!mediaPath || !isYoutubeMediaPath(mediaPath)) {
return;
@@ -164,5 +168,13 @@ export function createYoutubePrimarySubtitleNotificationRuntime(deps: {
clearPendingTimer();
}
},
setAppOwnedFlowInFlight: (inFlight: boolean): void => {
appOwnedFlowInFlight = inFlight;
if (inFlight) {
clearPendingTimer();
return;
}
schedulePendingCheck();
},
};
}

View File

@@ -469,6 +469,87 @@ test('youtube track picker ignores duplicate resolve submissions while request i
}
});
test('youtube track picker keeps no-track controls disabled after a rejected continue request', async () => {
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 () => ({ ok: false, message: 'still no tracks' }),
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: [],
defaultPrimaryTrackId: null,
defaultSecondaryTrackId: null,
hasTracks: false,
});
modal.wireDomEvents();
const listeners = dom.youtubePickerContinueButton.listeners.get('click') ?? [];
await Promise.all(listeners.map((listener) => listener()));
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.equal(dom.youtubePickerStatus.textContent, 'still no tracks');
} 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;

View File

@@ -159,7 +159,9 @@ export function createYoutubeTrackPickerModal(
setStatus(error instanceof Error ? error.message : String(error), true);
} finally {
resolveSelectionInFlight = false;
setResolveControlsDisabled(false);
const shouldKeepDisabled =
ctx.state.youtubePickerModalOpen && !(ctx.state.youtubePickerPayload?.hasTracks ?? false);
setResolveControlsDisabled(shouldKeepDisabled);
}
}