From 39b2ccad8e99d9f9224d0ef730165d7e25ca65e7 Mon Sep 17 00:00:00 2001 From: sudacode Date: Wed, 25 Mar 2026 21:10:43 -0700 Subject: [PATCH] fix: address latest coderabbit review --- ...ss-PR-35-latest-CodeRabbit-review-round.md | 68 +++++++++++++++++++ src/cli/args.test.ts | 5 ++ src/main.ts | 15 ++-- .../runtime/cli-command-prechecks.test.ts | 12 +++- src/main/runtime/youtube-playback-launch.ts | 4 +- 5 files changed, 96 insertions(+), 8 deletions(-) create mode 100644 backlog/tasks/task-234 - Address-PR-35-latest-CodeRabbit-review-round.md diff --git a/backlog/tasks/task-234 - Address-PR-35-latest-CodeRabbit-review-round.md b/backlog/tasks/task-234 - Address-PR-35-latest-CodeRabbit-review-round.md new file mode 100644 index 0000000..5751f9f --- /dev/null +++ b/backlog/tasks/task-234 - Address-PR-35-latest-CodeRabbit-review-round.md @@ -0,0 +1,68 @@ +--- +id: TASK-234 +title: 'Address PR #35 latest CodeRabbit review round' +status: Done +assignee: + - codex +created_date: '2026-03-26 03:59' +updated_date: '2026-03-26 04:01' +labels: + - review-comments + - coderabbit +dependencies: [] +references: + - /Users/sudacode/projects/japanese/SubMiner/src/main.ts + - /Users/sudacode/projects/japanese/SubMiner/src/cli/args.test.ts + - >- + /Users/sudacode/projects/japanese/SubMiner/src/main/runtime/cli-command-prechecks.test.ts + - >- + /Users/sudacode/projects/japanese/SubMiner/src/main/runtime/youtube-playback-launch.ts +priority: medium +--- + +## Description + + +Assess and implement the latest actionable CodeRabbit feedback on PR #35 for the Windows YouTube playback flow. Scope includes fixing the overlapping youtubePlay cleanup race in main runtime state and any low-risk follow-up test/clarity comments from the same review round. + + +## Acceptance Criteria + +- [x] #1 Overlapping youtubePlay requests no longer let an older flow clear active quit-on-disconnect/app-owned-flow state for a newer flow. +- [x] #2 Latest low-risk CodeRabbit test and clarity follow-ups for this PR round are addressed or intentionally rejected based on code verification. +- [x] #3 Relevant tests covering the touched areas pass locally. + + +## Implementation Plan + + +1. Update runYoutubePlaybackFlowMain in src/main.ts to use a per-request generation guard around shared YouTube flow state so overlapping requests cannot clear the active timer, armed flag, or app-owned-flow marker for a newer request. +2. Address verified low-risk latest-round follow-ups: add direct startup-prereq assertions in src/cli/args.test.ts, extend side-effect assertions in src/main/runtime/cli-command-prechecks.test.ts, and rename the youtube-playback-launch polling variable for clarity. +3. Run targeted Bun tests for the touched areas and record results in the task notes/final summary. + + +## Implementation Notes + + +Implemented per-request youtubePlaybackFlowGeneration guard in src/main.ts so superseded youtubePlay flows cannot clear the active arm timer, armed flag, or app-owned-flow state for a newer request. + +Added explicit startup-prereq assertions in src/cli/args.test.ts and stronger warmup/log side-effect assertions in src/main/runtime/cli-command-prechecks.test.ts for the latest CodeRabbit follow-ups. + +Renamed youtube-playback-launch polling variable from pathChanged to pathDiffersFromInitial for accuracy without behavior change. + +Verification: bun test src/cli/args.test.ts; bun test src/main/runtime/cli-command-prechecks.test.ts; bun test src/main/runtime/youtube-playback-launch.test.ts; bun run typecheck. + + +## Final Summary + + +Addressed the latest PR #35 CodeRabbit round by making YouTube playback flow cleanup generation-safe in src/main.ts. Overlapping youtubePlay requests now isolate timer/armed/app-owned-flow cleanup to the currently active request so an older flow cannot clear state for its replacement. + +Also folded in the latest low-risk follow-ups: args tests now assert that youtube playback requires overlay startup prerequisites, cli-command precheck tests now assert warmup/log side effects for the youtube transition, and youtube-playback-launch.ts uses a clearer variable name for the initial-path comparison. + +Verification: +- bun test src/cli/args.test.ts +- bun test src/main/runtime/cli-command-prechecks.test.ts +- bun test src/main/runtime/youtube-playback-launch.test.ts +- bun run typecheck + diff --git a/src/cli/args.test.ts b/src/cli/args.test.ts index a766480..4efb631 100644 --- a/src/cli/args.test.ts +++ b/src/cli/args.test.ts @@ -1,6 +1,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { + commandNeedsOverlayStartupPrereqs, commandNeedsOverlayRuntime, hasExplicitCommand, isHeadlessInitialCommand, @@ -75,6 +76,7 @@ test('youtube playback does not use generic overlay-runtime bootstrap classifica const args = parseArgs(['--youtube-play', 'https://youtube.com/watch?v=abc']); assert.equal(commandNeedsOverlayRuntime(args), false); + assert.equal(commandNeedsOverlayStartupPrereqs(args), true); }); test('parseArgs handles jellyfin item listing controls', () => { @@ -148,6 +150,9 @@ test('hasExplicitCommand and shouldStartApp preserve command intent', () => { assert.equal(shouldStartApp(help), false); assert.equal(shouldRunSettingsOnlyStartup(help), false); + const youtubePlay = parseArgs(['--youtube-play', 'https://youtube.com/watch?v=abc']); + assert.equal(commandNeedsOverlayStartupPrereqs(youtubePlay), true); + const anilistStatus = parseArgs(['--anilist-status']); assert.equal(anilistStatus.anilistStatus, true); assert.equal(hasExplicitCommand(anilistStatus), true); diff --git a/src/main.ts b/src/main.ts index 6ceb813..655f28a 100644 --- a/src/main.ts +++ b/src/main.ts @@ -503,6 +503,7 @@ let anilistCachedAccessToken: string | null = null; let jellyfinPlayQuitOnDisconnectArmed = false; let youtubePlayQuitOnDisconnectArmed = false; let youtubePlayQuitOnDisconnectArmTimer: ReturnType | null = null; +let youtubePlaybackFlowGeneration = 0; const JELLYFIN_LANG_PREF = 'ja,jp,jpn,japanese,en,eng,english,enUS,en-US'; const JELLYFIN_TICKS_PER_SECOND = 10_000_000; const JELLYFIN_REMOTE_PROGRESS_INTERVAL_MS = 3000; @@ -986,6 +987,7 @@ async function runYoutubePlaybackFlowMain(request: { mode: NonNullable; source: CliCommandSource; }): Promise { + const flowGeneration = ++youtubePlaybackFlowGeneration; youtubePrimarySubtitleNotificationRuntime.setAppOwnedFlowInFlight(true); let flowCompleted = false; try { @@ -1047,6 +1049,9 @@ async function runYoutubePlaybackFlowMain(request: { } if (request.source === 'initial') { youtubePlayQuitOnDisconnectArmTimer = setTimeout(() => { + if (youtubePlaybackFlowGeneration !== flowGeneration) { + return; + } youtubePlayQuitOnDisconnectArmed = true; youtubePlayQuitOnDisconnectArmTimer = null; }, 3000); @@ -1062,11 +1067,13 @@ async function runYoutubePlaybackFlowMain(request: { flowCompleted = true; logger.info(`YouTube playback flow completed from ${request.source}.`); } finally { - if (!flowCompleted) { - clearYoutubePlayQuitOnDisconnectArmTimer(); - youtubePlayQuitOnDisconnectArmed = false; + if (youtubePlaybackFlowGeneration === flowGeneration) { + if (!flowCompleted) { + clearYoutubePlayQuitOnDisconnectArmTimer(); + youtubePlayQuitOnDisconnectArmed = false; + } + youtubePrimarySubtitleNotificationRuntime.setAppOwnedFlowInFlight(false); } - youtubePrimarySubtitleNotificationRuntime.setAppOwnedFlowInFlight(false); } } diff --git a/src/main/runtime/cli-command-prechecks.test.ts b/src/main/runtime/cli-command-prechecks.test.ts index c985533..3b50430 100644 --- a/src/main/runtime/cli-command-prechecks.test.ts +++ b/src/main/runtime/cli-command-prechecks.test.ts @@ -68,6 +68,8 @@ test('texthooker precheck no-ops for texthooker command', () => { test('texthooker precheck transitions for youtube playback startup prereqs', () => { let mode = true; let prereqs = 0; + let warmups = 0; + let logs = 0; const handlePrecheck = createHandleTexthookerOnlyModeTransitionHandler({ isTexthookerOnlyMode: () => mode, setTexthookerOnlyMode: (enabled) => { @@ -77,12 +79,18 @@ test('texthooker precheck transitions for youtube playback startup prereqs', () ensureOverlayStartupPrereqs: () => { prereqs += 1; }, - startBackgroundWarmups: () => {}, - logInfo: () => {}, + startBackgroundWarmups: () => { + warmups += 1; + }, + logInfo: () => { + logs += 1; + }, }); handlePrecheck({ youtubePlay: 'https://youtube.com/watch?v=abc', texthooker: false } as never); assert.equal(mode, false); assert.equal(prereqs, 1); + assert.equal(warmups, 1); + assert.equal(logs, 1); }); diff --git a/src/main/runtime/youtube-playback-launch.ts b/src/main/runtime/youtube-playback-launch.ts index 085ae68..0de1e16 100644 --- a/src/main/runtime/youtube-playback-launch.ts +++ b/src/main/runtime/youtube-playback-launch.ts @@ -144,13 +144,13 @@ export function createPrepareYoutubePlaybackInMpvHandler(deps: YoutubePlaybackLa // Continue polling until media tracks are actually available. } } - const pathChanged = currentPath !== previousPath; + const pathDiffersFromInitial = currentPath !== previousPath; const matchesChangedTarget = currentPath === targetUrl || (isYoutubeMediaPath(currentPath) && isYoutubeMediaPath(targetUrl) && pathMatchesYoutubeTarget(currentPath, targetUrl)); - if (pathChanged && matchesChangedTarget) { + if (pathDiffersFromInitial && matchesChangedTarget) { if (deps.requestProperty) { try { const trackList = await deps.requestProperty('track-list');