Skip to content

fix(player): advance ASS window after successful load - #216

Merged
drondeseries merged 3 commits into
mainfrom
happy-gecko
Oct 4, 2026
Merged

drondeseries merged 3 commits into
mainfrom
happy-gecko

Conversation

@randrini

@randrini randrini commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Related issue: N/A
Validation tasks: none

During virtual-library playback the player re-requested the same ASS subtitle window on every timeupdate after the first window loaded, flashing the loading indicator. The window bounds stayed pinned to the initial [0,600] range instead of advancing to the successfully loaded window.

Approach

Make the ASS window bounds mutable and commit them (plus whole-track state) after each successful window load, so continued playback inside the loaded window issues no further fetches and the next request fires exactly at the new boundary. Add boundary and post-seek regression coverage.

Note: virtual font-source resolution and relay upstream logging already on main (base dddb534); this diff delivers only the ASS window fix + tests.

Validation

  • vitest useASSSubtitles.test.tsx boundary test: load [570,1170] -> 600/650/700 assert no refetch (fetch stays 2, instances 2) -> 1120 asserts exactly one next window position=1100
  • vitest seek test: replacement [2980,3580] ready -> 3010/3050 assert calls stay 3, constructors stay 2
  • CI 8/8 green (Go contract, Go lint, changed-lines, router-recovery, Go test, Web, Web-test 1/2 + 2/2)

Risks

ASS change only moves the refresh boundary to the loaded window. No backend, contract, or retry-policy change in this diff. Web font loader treats only 409 as pending (subtitleFonts.ts:204-221); 500/502 yield a cacheable empty bundle with fallback fonts (existing behavior, untouched here).

Checklist

  • I read and can explain the complete diff.
  • This pull request addresses one concern.

AI Disclosure

  • Harness: OpenCode
  • Tool(s): OpenCode (explorer, fixer subagents)
  • Model(s): opencode-go/muse-spark-1.3-contributor
  • Involvement: AI-assisted
  • Adversarial review: n/a - small bounded fix verified by focused tests and lint; full review left to PR reviewers

@drondeseries

Copy link
Copy Markdown
Collaborator

Production-readiness review: Request changes (Grade 84/100)

Correct, narrowly scoped fix — but missing targeted regression proof and the description overclaims. No merge yet.

Finding 1 — the ASS fix is correct; no async redesign needed

web/src/player/hooks/useASSSubtitles.ts:585-596 commits bounds only after renderer readiness/fill synchronization, guarded by cancellation, abort state, and instance identity. attempt() serializes via busy; seek supersession aborts + queues. Updating windowStart/windowEnd stops repeated refreshes against the original [0,600] bounds; usingWholeTrack makes whole-track state explicit. No request-generation abstraction warranted.

Finding 2 — the precise regression is untested (required)

web/src/player/hooks/useASSSubtitles.test.tsx:749-768 checks the first boundary refresh at t=590 but stops before dispatching another timeupdate — it passes with the original bug. The seek test (1009-1051) covers cancellation/replacement, not continued playback after replacement. No tests change in this PR.

Fix: extend the boundary test — await the refreshed renderer's successful load, dispatch several timeupdates inside the new window and assert no additional fetch/renderer construction, then advance near its new end and assert exactly one next-window request. Extend seek coverage to assert no immediate re-fetch after the replacement becomes ready (optional: delayed-readiness supersede proving an aborted attempt can't commit stale bounds).

Finding 3 — PR description overclaims; font-retry claim is false for web (required)

Actual diff is useASSSubtitles.ts +8/-4 plus one stray blank-line deletion in stream.go. Virtual-font resolution, 502 mapping, and relay logging are inherited from main, not delivered here (stream.go:2266-2273 maps errVirtualFontResolve→502; resolution in shared core :2293-2302). And web/src/player/utils/subtitleFonts.ts:204-221 treats only 409 as pending — both 500 and 502 produce a cacheable empty bundle with pending: false. So there is no web 500-only retry regression, but "clients already treat non-2xx as pending" must be struck (Apple/Android unverified; transient 502 → fallback fonts is existing behavior).

Fix: retitle/rewrite around advancing the ASS window; describe backend as inherited context; correct the font-pending claim; drop the blank-line deletion.

CI

Web / Go lint / contract pass; Go test, Web-tests, changed-lint, router-recovery pending — green gate on the final head required.

Acceptance: boundary + seek regression tests, corrected description, full CI green. No backend changes needed.

@randrini randrini self-assigned this Oct 4, 2026
@randrini randrini added the bug Something isn't working label Oct 4, 2026
@drondeseries

Copy link
Copy Markdown
Collaborator

Production-readiness re-review: Request changes (Grade 94/100, was 84/100)

Reviewed d8e0aaa0 at the exact SHA. Code and tests approved — body correction only, then merge. No further implementation changes required.

Verified fixed

  • Regression tests genuinely cover the fix (useASSSubtitles.test.tsx:770-809, 1083-1103): load [570,1170] → 600/650/700 assert fetches + renderer instances stay at 2 → 1120 asserts exactly one new fetch with position=1100&duration=600. Seek: replacement [2980,3580] ready → 3010/3050 stable at 3 calls / 2 constructors. These would fail on the old immutable bounds by control-flow inspection (useASSSubtitles.ts:701-728). Bounds commit post-readiness (584-594) matches what the tests await.
  • Stray blank line resolved — stream.go is absent from the live main...d8e0aaa0 diff entirely.
  • CI 8/8 green.

Required: correct the PR body (blocking)

The live merge diff vs base dddb534f contains only useASSSubtitles.ts + its test. Virtual-source resolution and relay logging are already on main, not delivered here — the three-change Approach and "resolve failures now answer 502" wording are stale. And the Risks claim "clients already treat non-2xx as pending" is false for the web client: web/src/player/utils/subtitleFonts.ts:204-221 returns pending: true only for 409; 500/502 yield an empty bundle with pending: false, cached definitively (227-238). Not a code regression — do not expand this patch into font retry policy — but the claim must be removed or corrected.

Fix: scope Problem/Approach to repeated ASS-window refreshes + committing the loaded window bounds; describe the new boundary and post-seek coverage; drop font/relay risk claims or label them already-on-main; record the green CI.

After that body edit: merge. @oracle code approval stands; no bypass needed beyond the edit.

@drondeseries drondeseries changed the title fix(playback): resolve virtual font source, log relay upstream, advance ASS window fix(player): advance ASS window after successful load Oct 4, 2026
@drondeseries
drondeseries merged commit 3ac649f into main Oct 4, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants