fix(web): initialize caption tracks after load#1885
Merged
richiemcilroy merged 1 commit intoJun 18, 2026
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+61
to
+63
| const bindTrackElementLoads = (): void => { | ||
| for (const trackElement of video.querySelectorAll("track")) { | ||
| if (trackLoadHandlers.has(trackElement)) continue; |
Contributor
There was a problem hiding this comment.
handleTrackUpdate fires after a <track> element is removed from the DOM (via MutationObserver or removetrack), syncTracks correctly cleans up the TextTrack from cueTracks, but trackLoadHandlers is never pruned. The detached HTMLTrackElement and its handler closure are held in memory until the returned cleanup function runs. Pruning stale entries in bindTrackElementLoads keeps the Map tidy across the binding's lifetime.
Suggested change
| const bindTrackElementLoads = (): void => { | |
| for (const trackElement of video.querySelectorAll("track")) { | |
| if (trackLoadHandlers.has(trackElement)) continue; | |
| const bindTrackElementLoads = (): void => { | |
| const liveElements = new Set(video.querySelectorAll("track")); | |
| for (const [el, handler] of trackLoadHandlers) { | |
| if (!liveElements.has(el)) { | |
| el.removeEventListener("load", handler); | |
| trackLoadHandlers.delete(el); | |
| } | |
| } | |
| for (const trackElement of liveElements) { | |
| if (trackLoadHandlers.has(trackElement)) continue; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/s/[videoId]/_components/caption-tracks.ts
Line: 61-63
Comment:
When `handleTrackUpdate` fires after a `<track>` element is removed from the DOM (via `MutationObserver` or `removetrack`), `syncTracks` correctly cleans up the `TextTrack` from `cueTracks`, but `trackLoadHandlers` is never pruned. The detached `HTMLTrackElement` and its handler closure are held in memory until the returned cleanup function runs. Pruning stale entries in `bindTrackElementLoads` keeps the Map tidy across the binding's lifetime.
```suggestion
const bindTrackElementLoads = (): void => {
const liveElements = new Set(video.querySelectorAll("track"));
for (const [el, handler] of trackLoadHandlers) {
if (!liveElements.has(el)) {
el.removeEventListener("load", handler);
trackLoadHandlers.delete(el);
}
}
for (const trackElement of liveElements) {
if (trackLoadHandlers.has(trackElement)) continue;
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+86
to
+111
| describe("bindCaptionTrackCueText", () => { | ||
| it("initializes caption cues when a track element loads after metadata", () => { | ||
| const textTracks = new FakeTextTrackList(); | ||
| const trackElement = new FakeTrackElement(); | ||
| const video = createVideo(textTracks, [trackElement]); | ||
| const captionTrack = new FakeTextTrack( | ||
| "captions", | ||
| createCueList("Loaded caption"), | ||
| ); | ||
| const cueTexts: string[] = []; | ||
|
|
||
| const cleanup = bindCaptionTrackCueText(video, (text) => { | ||
| cueTexts.push(text); | ||
| }); | ||
|
|
||
| video.dispatch("loadedmetadata"); | ||
| textTracks.addSilently(captionTrack); | ||
| trackElement.readyState = 2; | ||
| trackElement.dispatch("load"); | ||
| captionTrack.dispatch("cuechange"); | ||
|
|
||
| expect(captionTrack.mode).toBe("hidden"); | ||
| expect(cueTexts.at(-1)).toBe("Loaded caption"); | ||
|
|
||
| cleanup(); | ||
| }); |
Contributor
There was a problem hiding this comment.
The suite only covers the async VTT-load path (track element loads after loadedmetadata). The synchronous path — where <track> elements are already in the DOM with readyState === 2 before bindCaptionTrackCueText is called — is exercised by the immediate handleTrackUpdate() call at the end of setup, but is not tested. A regression here (e.g., the sync syncTracks call being dropped) would go undetected.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/__tests__/unit/caption-tracks.test.ts
Line: 86-111
Comment:
**Missing synchronous-init test case**
The suite only covers the async VTT-load path (track element loads after `loadedmetadata`). The synchronous path — where `<track>` elements are already in the DOM with `readyState === 2` before `bindCaptionTrackCueText` is called — is exercised by the immediate `handleTrackUpdate()` call at the end of setup, but is not tested. A regression here (e.g., the sync `syncTracks` call being dropped) would go undetected.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Member
|
hey @greptileai please re-review |
Member
|
Thanks for the contribution! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
<track>load eventsFixes #1773.
Tests
Note: a broad
pnpm --dir apps/web exec tsc --noEmit --pretty false --incremental falserun was attempted, but it stayed silent for several minutes and was stopped to avoid tying up the sweep environment.Greptile Summary
This PR refactors caption/subtitle track binding into a new shared
bindCaptionTrackCueTexthelper and fixes a bug where captions failed to initialize when<track>elements loaded their VTT file asynchronously afterloadedmetadata.caption-tracks.ts– New helper that uses a MutationObserver, per-<track>-elementloadlisteners, andtextTrackslist events to cover both the synchronous and async VTT-load paths; cleanup function returned to callers handles all teardown.CapVideoPlayer/HLSVideoPlayer– Inline track-management code (~35 and ~65 lines respectively) replaced with a singlebindCaptionTrackCueTextcall; cleanup wired correctly in bothuseEffectreturn functions.caption-tracks.test.ts– Focused unit test for the async load path using lightweight fakes forHTMLVideoElement,TextTrackList, andHTMLTrackElement.Confidence Score: 5/5
Safe to merge — the refactor is a net simplification and the async-VTT bug fix is well-scoped with no regressions on the single-track path.
The change consolidates duplicated track-management logic into a single, well-tested helper. Cleanup is correctly wired in both players, and the async load path that was the root of the original bug is now explicitly covered by a unit test. The only observations are minor efficiency nits (extra redundant syncTracks call when elements are already loaded, and all-tracks cuechange subscriptions that read only the first track), neither of which affects correctness in the common single-track case.
caption-tracks.ts has two style-level observations worth a quick read-through; the player files are straightforward call-site changes.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(web): initialize caption tracks afte..." | Re-trigger Greptile