fix: address latest coderabbit review

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
+
+<!-- SECTION:DESCRIPTION:BEGIN -->
+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.
+<!-- SECTION:DESCRIPTION:END -->
+
+## Acceptance Criteria
+<!-- AC:BEGIN -->
+- [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.
+<!-- AC:END -->
+
+## Implementation Plan
+
+<!-- SECTION:PLAN:BEGIN -->
+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.
+<!-- SECTION:PLAN:END -->
+
+## Implementation Notes
+
+<!-- SECTION:NOTES:BEGIN -->
+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.
+<!-- SECTION:NOTES:END -->
+
+## Final Summary
+
+<!-- SECTION:FINAL_SUMMARY:BEGIN -->
+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
+<!-- SECTION:FINAL_SUMMARY:END -->

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);

src/main.ts

@@ -503,6 +503,7 @@ let anilistCachedAccessToken: string | null = null;
 let jellyfinPlayQuitOnDisconnectArmed = false;
 let youtubePlayQuitOnDisconnectArmed = false;
 let youtubePlayQuitOnDisconnectArmTimer: ReturnType<typeof setTimeout> | 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<CliArgs['youtubeMode']>;
   source: CliCommandSource;
 }): Promise<void> {
+  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,6 +1067,7 @@ async function runYoutubePlaybackFlowMain(request: {
     flowCompleted = true;
     logger.info(`YouTube playback flow completed from ${request.source}.`);
   } finally {
+    if (youtubePlaybackFlowGeneration === flowGeneration) {
       if (!flowCompleted) {
         clearYoutubePlayQuitOnDisconnectArmTimer();
         youtubePlayQuitOnDisconnectArmed = false;
@@ -1069,6 +1075,7 @@ async function runYoutubePlaybackFlowMain(request: {
       youtubePrimarySubtitleNotificationRuntime.setAppOwnedFlowInFlight(false);
     }
   }
+}
 
 let firstRunSetupMessage: string | null = null;
 const resolveWindowsMpvShortcutRuntimePaths = () =>

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);
 });

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');