mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-04-01 06:12:07 -07:00
sudacode
3502cdc607
- Move ~35 completed tasks from backlog/tasks/ to backlog/completed/ - Add ordinal fields to remaining in-progress task files - Mark task-255 (playlist browser modal) as Done with final notes
65 lines
4.0 KiB
Markdown
65 lines
4.0 KiB
Markdown
---
|
||
id: TASK-228
|
||
title: 'Assess and address PR #31 subsequent CodeRabbit review round'
|
||
status: Done
|
||
assignee:
|
||
- codex
|
||
created_date: '2026-03-24 04:10'
|
||
updated_date: '2026-03-24 06:41'
|
||
labels:
|
||
- pr-review
|
||
- coderabbit
|
||
dependencies: []
|
||
references:
|
||
- >-
|
||
PR #31 feat: add app-owned YouTube subtitle flow with absPlayer-style
|
||
parsing
|
||
- 'commit cdb12827 fix: address PR #31 latest review follow-ups'
|
||
priority: medium
|
||
ordinal: 146500
|
||
---
|
||
|
||
## Description
|
||
|
||
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||
Inspect the subsequent CodeRabbit review round on PR #31 after commit cdb12827, verify each newly reported issue against the current branch, implement the valid fixes with regression coverage where appropriate, and prepare/update PR thread replies.
|
||
<!-- SECTION:DESCRIPTION:END -->
|
||
|
||
## Acceptance Criteria
|
||
<!-- AC:BEGIN -->
|
||
- [x] #1 New CodeRabbit comments after cdb12827 are triaged into valid fixes vs declined suggestions with rationale.
|
||
- [x] #2 Confirmed issues are fixed with regression coverage where appropriate.
|
||
- [x] #3 Relevant verification passes for the touched areas.
|
||
- [x] #4 PR threads are updated for the addressed comments.
|
||
<!-- AC:END -->
|
||
|
||
## Implementation Plan
|
||
|
||
<!-- SECTION:PLAN:BEGIN -->
|
||
1. Verify the new CodeRabbit comments after cdb12827 and separate valid bugs from refactor-only suggestions.
|
||
2. Add failing regression coverage for the valid runtime issues: `track.selected` fallback in the YouTube primary-subtitle notifier and consistent no-track handling in the picker.
|
||
3. Inspect existing test seams for the `main.ts` flow-entry guards; if lightweight coverage exists, add it before patching. Otherwise apply the minimal `main.ts` fixes and rely on typecheck plus targeted regression tests around the affected runtime helpers.
|
||
4. Implement the confirmed fixes: picker re-entry guard, broader `inFlight` cleanup, `track.selected` fallback, and a single canonical `hasTracks` check.
|
||
5. Run targeted tests/typecheck and update the new PR threads with landed fix refs.
|
||
<!-- SECTION:PLAN:END -->
|
||
|
||
## Implementation Notes
|
||
|
||
<!-- SECTION:NOTES:BEGIN -->
|
||
Triaged the post-cdb12827 CodeRabbit round. Implemented the 4 concrete follow-ups: manual picker re-entry guard, broader `setAppOwnedFlowInFlight(...)` cleanup, `track.selected` fallback in the YouTube primary-subtitle notifier, and a single canonical `payloadHasTracks(...)` helper in the picker. Also took the adjacent `replaceChildren()` cleanup while touching the same picker paths.
|
||
|
||
Verification passed: `bun test src/main/runtime/youtube-primary-subtitle-notification.test.ts src/renderer/modals/youtube-track-picker.test.ts launcher/commands/playback-command.test.ts src/core/services/cli-command.test.ts` and `bun run typecheck`.
|
||
|
||
Updated the new CodeRabbit inline threads with landed fix refs and left a top-level PR comment noting the large refactor suggestions are intentionally out of scope for this bugfix round.
|
||
<!-- SECTION:NOTES:END -->
|
||
|
||
## Final Summary
|
||
|
||
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
|
||
Assessed the subsequent CodeRabbit review round on PR #31 after cdb12827 and applied the valid follow-ups in commit 5f6f93cd. Added a guard in `openYoutubeTrackPickerFromPlayback()` so the manual picker cannot re-enter while another YouTube flow session is active, widened the app-owned in-flight suppression to cover synchronous Windows mpv bootstrap and connect failures, taught the primary-subtitle notifier to honor `track.selected` before `sid` arrives, and unified the picker’s subtitle-availability logic behind `payloadHasTracks(...)` while swapping node clearing to `replaceChildren()`.
|
||
|
||
Verification passed with `bun test src/main/runtime/youtube-primary-subtitle-notification.test.ts src/renderer/modals/youtube-track-picker.test.ts launcher/commands/playback-command.test.ts src/core/services/cli-command.test.ts` and `bun run typecheck`.
|
||
|
||
Updated the latest inline CodeRabbit threads plus a top-level PR comment summarizing the round and explicitly deferred the large refactor suggestions as non-blocking maintainability nits.
|
||
<!-- SECTION:FINAL_SUMMARY:END -->
|