SubMiner/backlog/tasks/task-200 - Address-latest-PR-19-CodeRabbit-follow-ups.md at 5feed360caa29aed4e61cefc78f206d52774ff55 - SubMiner

mirror of https://github.com/ksyasuda/SubMiner.git synced 2026-03-24 00:11:27 -07:00

Files

sudacode 5feed360ca feat: add app-owned YouTube subtitle flow with absPlayer-style parsing (#31 )

* fix: harden preload argv parsing for popup windows

* fix: align youtube playback with shared overlay startup

* fix: unwrap mpv youtube streams for anki media mining

* docs: update docs for youtube subtitle and mining flow

* refactor: unify cli and runtime wiring for startup and youtube flow

* feat: update subtitle sidebar overlay behavior

* chore: add shared log-file source for diagnostics

* fix(ci): add changelog fragment for immersion changes

* fix: address CodeRabbit review feedback

* fix: persist canonical title from youtube metadata

* style: format stats library tab

* fix: address latest review feedback

* style: format stats library files

* test: stub launcher youtube deps in CI

* test: isolate launcher youtube flow deps

* test: stub launcher youtube deps in failing case

* test: force x11 backend in launcher ci harness

* test: address latest review feedback

* fix(launcher): preserve user YouTube ytdl raw options

* docs(backlog): update task tracking notes

* fix(immersion): special-case youtube media paths in runtime and tracking

* feat(stats): improve YouTube media metadata and picker key handling

* fix(ci): format stats media library hook

* fix: address latest CodeRabbit review items

* docs: update youtube release notes and docs

* feat: auto-load youtube subtitles before manual picker

* fix: restore app-owned youtube subtitle flow

* docs: update youtube playback docs and config copy

* refactor: remove legacy youtube launcher mode plumbing

* fix: refine youtube subtitle startup binding

* docs: clarify youtube subtitle startup behavior

* fix: address PR #31 latest review follow-ups

* fix: address PR #31 follow-up review comments

* test: harden youtube picker test harness

* udpate backlog

* fix: add timeout to youtube metadata probe

* docs: refresh youtube and stats docs

* update backlog

* update backlog

* chore: release v0.9.0

2026-03-24 00:01:24 -07:00

6.9 KiB

Raw Blame History

id, title, status, assignee, created_date, updated_date, labels, milestone, dependencies, references, priority, ordinal

title

status

assignee

created_date

updated_date

labels

milestone

dependencies

references

priority

ordinal

TASK-200

Address latest PR #19 CodeRabbit follow-ups

Done

@codex

2026-03-19 07:18

2026-03-23 03:22

pr-review

anki-integration

launcher

m-1

launcher/mpv.test.ts

src/anki-integration.ts

src/anki-integration/card-creation.ts

src/anki-integration/runtime.ts

src/anki-integration/known-word-cache.ts

medium

133500

Description

Validate the latest 2026-03-19 CodeRabbit review round on PR #19, implement only the confirmed fixes, and verify the touched launcher and Anki integration paths.

Acceptance Criteria

#1 Each latest-round PR #19 CodeRabbit inline comment is validated against the current branch and classified as actionable or not warranted
#2 Confirmed correctness issues in launcher and Anki integration code are fixed with focused regression coverage where practical
#3 Targeted verification runs for the touched areas and the task notes record what changed versus what was rejected

Implementation Plan

Validate the five inline comments from the 2026-03-19 CodeRabbit PR #19 review against current launcher and Anki integration code.
Add or extend focused tests for any confirmed launcher env-sandbox, notification-state, AVIF lead-in propagation, or known-word-cache lifecycle/scope regressions.
Apply the smallest safe fixes in launcher/mpv.test.ts, src/anki-integration.ts, src/anki-integration/card-creation.ts, src/anki-integration/runtime.ts, and src/anki-integration/known-word-cache.ts as needed.
Run targeted unit tests plus the SubMiner verification helper on the touched files, then record which comments were accepted or rejected in task notes.

Implementation Notes

Validated the five latest inline comments from CodeRabbit review 3973222927 on PR #19.

Accepted fixes:

Hardened the three findAppBinary launcher tests against host leakage by sandboxing SUBMINER_APPIMAGE_PATH / SUBMINER_BINARY_PATH and stubbing executable checks so /opt and PATH resolution are deterministic.
showNotification() now marks OSD/both updates as failed when errorSuffix is present instead of always rendering a success marker.
applyRuntimeConfigPatch() now avoids starting or stopping known-word cache lifecycle work while the runtime is stopped, while still clearing cached state when highlighting is disabled.
Extracted shared known-word cache lifecycle helpers and switched the persisted cache identity to the same lifecycle config used by runtime restart detection, so changes to fields.word, per-deck field mappings, or refresh interval invalidate stale cache state correctly.

Rejected fix:

The createSentenceCard() AVIF lead-in comment was technically incomplete for this branch. There is no current caller that computes an animatedLeadInSeconds input for sentence-card creation, and the existing lead-in resolver depends on note media fields that do not exist before the new card's media is generated.

Regression coverage added:

src/anki-integration.test.ts partial-failure OSD result marker.
src/anki-integration/runtime.test.ts stopped-runtime known-word lifecycle guards.
src/anki-integration/known-word-cache.test.ts cache invalidation when fields.word or per-deck field mappings change.

Verification:

bun test src/anki-integration/runtime.test.ts
bun test src/anki-integration/known-word-cache.test.ts
bun test src/anki-integration.test.ts --test-name-pattern 'marks partial update notifications as failures in OSD mode'
bun test launcher/mpv.test.ts --test-name-pattern 'findAppBinary resolves ~/.local/bin/SubMiner.AppImage when it exists|findAppBinary resolves /opt/SubMiner/SubMiner.AppImage when ~/.local/bin candidate does not exist|findAppBinary finds subminer on PATH when AppImage candidates do not exist'
bun test src/anki-integration.test.ts src/anki-integration/runtime.test.ts src/anki-integration/known-word-cache.test.ts launcher/mpv.test.ts
bash .agents/skills/subminer-change-verification/scripts/classify_subminer_diff.sh launcher/mpv.test.ts src/anki-integration.ts src/anki-integration/runtime.ts src/anki-integration/known-word-cache.ts src/anki-integration/runtime.test.ts src/anki-integration/known-word-cache.test.ts src/anki-integration.test.ts
bash .agents/skills/subminer-change-verification/scripts/verify_subminer_change.sh --lane launcher-plugin --lane core launcher/mpv.test.ts src/anki-integration.ts src/anki-integration/runtime.ts src/anki-integration/known-word-cache.ts src/anki-integration/runtime.test.ts src/anki-integration/known-word-cache.test.ts src/anki-integration.test.ts

Verifier result:

launcher-plugin lane passed (test:launcher:smoke:src, test:plugin:src).
core/typecheck passed.
core/test-fast failed for an unrelated existing environment issue in scripts/update-aur-package.test.ts: scripts/update-aur-package.sh: line 71: mapfile: command not found under the local macOS Bash environment.
Verifier artifacts: .tmp/skill-verification/subminer-verify-20260319-002617-UgpKUy

Classification: actionable and fixed -> launcher/mpv.test.ts env leakage hardening, src/anki-integration.ts partial-failure OSD marker, src/anki-integration/runtime.ts started-guard for known-word lifecycle calls, src/anki-integration/known-word-cache.ts cache identity alignment with runtime lifecycle config.

Classification: not warranted as written -> src/anki-integration/card-creation.ts lead-in threading comment. No current createSentenceCard() caller computes or owns an animatedLeadInSeconds value, and the existing lead-in helper derives from preexisting note media fields, so blindly adding an optional parameter would not fix a real branch behavior bug.

Final Summary

Fixed four confirmed PR #19 latest-round CodeRabbit issues locally: deterministic launcher findAppBinary tests, correct partial-failure OSD result markers, started-state guards around known-word cache lifecycle restarts, and shared known-word cache identity logic so field-mapping changes invalidate stale cache state. Added focused regression coverage for each confirmed behavior.

One comment was intentionally not applied: the createSentenceCard() AVIF lead-in suggestion does not match the current branch architecture because no caller computes that value today and the existing resolver requires preexisting note media fields. Verification is green for all touched targeted tests plus the launcher-plugin/core typecheck lanes; the only remaining red is an unrelated existing test:fast failure in scripts/update-aur-package.test.ts caused by mapfile being unavailable in the local Bash environment.

6.9 KiB Raw Blame History