fix: Improve error handling and stability in CinemaPlayer; add tests for bandwidth estimator and VTT parsing (3)

[umeraamir09]

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

[umeraamir09]

Review — fix: error handling and stability in CinemaPlayer (3)

Reviewed commit 20f55ab (6 files, +206/-25). I read the full surrounding context (CinemaPlayer stream-setup effect, codec-probe, bandwidth-estimator, useAdaptiveBitrate, vtt.ts) and ran npm run test:lib — 29/29 tests pass, including the 3 new ABR tests and the new VTT test.

Verdict: no blocking issues. Three low-severity notes (inline comments).

What I verified as sound

• 3.11 (loadedmetadata listener teardown) — correct. The listener is only registered on the two synchronous attach paths (direct, Safari-native HLS); the async hls.js path never registers it and is guarded by cancelled after the dynamic import, so the cleanup's removeEventListener covers every path that can leave a listener behind, and the once-flag makes teardown-vs-fire mutually safe.
• 3.4 (wall-clock countdown) — correct. Background-tab throttling can no longer freeze the countdown; Math.ceil keeps the displayed second consistent; the "same object reference when unchanged" trick correctly avoids re-render/effect churn at 4 ticks/sec. The one caveat is in the inline comment (stale-deadline invariant).
• 3.9 (single-track audio probe) — correct and a real win: CODEC_TAG maps dts to the dts tag, so files with a DTS track + supported default AAC previously failed the combined probe and were forced to transcode, even in direct-play-eligible condition. Per-track probing fixes that, and selecting a non-default track still forces HLS via wantsTranscode, so the unsupported-selection fallback is preserved.
• 3.7 (ABR margin/stability/hysteresis) — internally consistent. The 1.5x upgrade margin (vs 1.35x) plus the 0.85 safety factor creates an actual dead band between the downgrade threshold (below current bitrate / 0.85) and upgrade threshold (at least target x 1.5), so oscillation is prevented even outside the hysteresis window. The hysteresis window legitimately exempts the buffer-emergency path: that path requires strictly declining buffer readings over 3 checks, and a rebuild's buffer-0 dip produces equal readings, so it can't false-trigger during the post-upgrade rebuild it is meant to protect. Test fakes (FakeClockEstimator) are sound because suggest() reads the clock only for cooldown windows.
• codec-probe SSR guard move — behavior-preserving; the reordered code before it touches no DOM.

Minor notes (details in inline comments)

1. src/components/player/CinemaPlayer.tsx:1086 — countdown deadline is anchored on a bare zero-check rather than on the prompt that created it; latent stale-deadline trap if the prompt lifecycle ever gains another source or a variable secondsLeft. Works today.
2. src/components/player/CinemaPlayer.tsx:142 — the 3.5 comment claims NaN passed the old typeof check, but NaN > 0 is already false; the real (narrow) hardening is the Infinity guard, which previously reached hls.js startPosition. Harmless either way; comment could be more accurate.
3. src/lib/__tests__/vtt-security.test.ts:108 — the new VTT test covers pre-existing behavior (parseTimestamp already handled MM:SS.mmm via split-on-colon arithmetic); no parser change landed in this PR. Good regression coverage, though the commit message implies a fix.

[umeraamir09]

Low — countdown deadline anchor is keyed on a bare zero-check, not the prompt itself (robustness trap).

The anchor is taken only when countdownEndRef.current === 0 (once per countdown). That works today because nextPrompt is only ever created fresh in onEnded with the constant NEXT_EPISODE_COUNTDOWN (10s), and the ref is zeroed before onNextEpisode() runs. But nothing binds the deadline to the prompt that created it: if any future caller ever sets nextPrompt with a different secondsLeft (or a second prompt arrives while a deadline is still pending — e.g. a re-fired ended on rebuild), the effect silently keeps the stale deadline and the countdown fires early/late.

Suggestion: carry the deadline with the prompt itself (e.g. { secondsLeft, deadline } state) or anchor on a prompt-epoch ref, so a new prompt always rebinds the deadline. No action needed if the prompt lifecycle stays as constrained as it is today — flagging only because this is exactly the class of "wait, this only works because of an invariant elsewhere" code that breaks silently later.

[umeraamir09]

Nitpick — the comment's rationale is off; the actual hardening is the Infinity guard.

NaN > 0 is already false, so the old bare typeof !== "number" check already excluded NaN — the Number.isFinite addition doesn't change NaN handling. Its real (narrow but genuine) value is excluding Infinity: Infinity > 0 passes, so a malformed startAtSec="Infinity" (e.g. from query-string parsing) previously reached startPosition in the hls.js config (line ~614), where tryRestore's Math.min clamp never runs. Suggest rewording the comment to say it guards against non-finite values (NaN/Infinity) rather than claiming NaN passed the old check.

[umeraamir09]

Note — this test covers pre-existing behavior; no parser change is in this PR.

parseTimestamp in src/lib/vtt.ts already handled short timestamps (MM:SS.mmm) and single-digit minutes — the implementation is plain split(":") + seconds*60 + parseFloat, so "0:30.000" → 30 and "00:01:05.000" → 65 without any change. I verified npm run test:lib passes all 3 new ABR tests + this one (29/29). Fine to keep as regression coverage, but the commit message ("add tests for VTT parsing") may suggest a parser fix landed here when it didn't — if the short-timestamp bug was fixed in an earlier commit in this stack, no action needed.