Conversation
- Add preload='metadata' attribute to force browser to load subtitle tracks immediately - Include label property in subtitle track configuration for better accessibility - Export Subtitle type for external use This ensures subtitles are loaded eagerly rather than lazily, improving the user experience. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit addresses inconsistent subtitle loading issues by: 1. **Programmatic Track Initialization**: Added `initializeSubtitleTracks` callback that directly accesses the HTML5 video element and forces subtitle tracks into 'showing' mode 2. **Fixed CORS Configuration**: Changed crossOrigin from 'true' to 'anonymous' for proper CORS handling of .vtt files 3. **Multiple Initialization Points**: - Initialize on player ready - Fallback initialization when playback starts - Prevents duplicate initialization with ref tracking 4. **Video Change Handling**: Reset initialization state when video source changes 5. **Enhanced Default Track Handling**: Properly matches and enables default subtitle tracks based on language and isDefault flag This ensures subtitles load reliably across different browsers and scenarios, eliminating the need for manual reloads or toggling. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit completely rewrites the subtitle initialization logic to handle inconsistent subtitle display issues where tracks load but don't show. Key improvements: 1. **Event-Driven Initialization**: Listen to 'addtrack' events instead of relying solely on setTimeout, ensuring we catch tracks as they're added 2. **Multiple Retry Strategy**: Attempt activation immediately, then retry at 100ms, 300ms, 500ms, and 1000ms intervals 3. **Aggressive Mode Enforcement**: Added a watcher that runs every 1 second for 10 seconds, continuously forcing default tracks to 'showing' mode. This prevents ReactPlayer or browser from resetting the mode. 4. **Detailed Console Logging**: Added comprehensive logging to debug: - Track count and availability - Each track's language, mode, and kind - Activation attempts and success - Mode re-enforcement events 5. **Proper Cleanup**: Clear watchers on video change and component unmount The watcher is particularly important as it prevents the browser or ReactPlayer from silently changing the track mode back to 'disabled' or 'hidden'. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit adds extensive debugging to diagnose why subtitle tracks show as 'showing' mode but no subtitles appear on screen. New debugging features: 1. **VTT File Testing**: Added testVttFile() function that: - Fetches the VTT file directly via fetch API - Validates HTTP response status - Checks file size and content - Verifies WEBVTT header - Counts cue entries - Detects CORS errors explicitly 2. **Cue Inspection**: Track activation now logs: - Number of cues loaded in each track - Track element src attribute - Track readyState - Warnings when track is 'showing' but has no cues 3. **Event Listeners**: Added listeners for: - 'load' event on tracks - 'cuechange' event to detect when cues become active 4. **Configuration Logging**: Logs subtitle configuration on player ready: - All subtitle objects - All subtitle URLs This will help identify: - Empty or malformed VTT files - CORS issues preventing VTT file access - Timing issues with cue loading - Invalid VTT file format 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Root cause: TextTrack mode was set to 'showing' before the browser
parsed the VTT file, resulting in tracks with 0 cues.
Solutions implemented:
1. **Wait for Video Metadata**: Check video.readyState and wait for
'loadedmetadata' event before activating tracks. This ensures the
browser has had time to parse VTT files.
2. **Track Mode Cycling**: Force track through disabled → hidden → showing
mode transition to trigger VTT parsing by the browser.
3. **Fixed Cue Counting Regex**: Updated regex from requiring HH:MM:SS.mmm
format to support both HH:MM:SS.mmm and MM:SS.mmm formats.
Previous: /\d{2}:\d{2}:\d{2}\.\d{3}.../
New: /\d{1,2}:\d{2}[:.]\d{3}.../
4. **Enhanced Debugging**: Added:
- Cue count verification after mode changes
- Video readyState logging
- Warnings when no cues found despite valid VTT
The key insight: Setting track.mode='showing' doesn't trigger VTT parsing.
We need to wait for the video element to reach readyState >= 1 (metadata
loaded) before the browser will have parsed the VTT file into cues.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Added extensive debugging to diagnose why VTT files aren't parsing: 1. **Track Element ReadyState Logging**: - Log both TextTrack.readyState and HTMLTrackElement.readyState - Display human-readable state names (NONE/LOADING/LOADED/ERROR) 2. **Error Event Listener**: Listen for 'error' events on track elements to detect CORS or loading failures 3. **Load Event Listener**: Listen for 'load' events to know when browser successfully loads the VTT file 4. **Forced Track Reload**: If track has 0 cues, force reload by: - Setting trackElement.src = '' - Wait 100ms - Restore originalSrc - Check cues after 1 second This will help identify if: - Track element is in ERROR state (CORS/404/etc) - Track is stuck in LOADING state - Load event never fires (browser not attempting to load) - Force reload fixes the issue 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Cleaned up the subtitle initialization code after confirming it works consistently. This eliminates multiple redundant VTT file requests. Changes: 1. **Prevent Multiple Initializations**: Added guard to check subtitlesInitialized.current and skip if already initialized. This prevents the function from running multiple times. 2. **Disabled Debug VTT Fetching**: Commented out testVttFile() calls that were manually fetching VTT files via fetch API for debugging. The browser already loads VTT files for tracks - no need to fetch twice. 3. **Removed Forced Track Reload**: Removed the src reset logic that was causing additional VTT loads. The mode cycling approach works without needing to reload the source. 4. **Simplified Retry Logic**: Reduced from 4 retry attempts (100ms, 300ms, 500ms, 1000ms) to single 500ms fallback. Removed addtrack event listener. 5. **Removed Excessive Logging**: Cleaned up console.logs while keeping essential error tracking. 6. **Removed Unused Code**: Removed trackModeWatcher ref and associated interval code. Result: Single VTT request per subtitle file instead of 3-5 requests. Subtitles still load consistently with the mode cycling approach. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The previous cleanup was too aggressive and removed critical retry attempts needed for consistent loading across different timing scenarios. Changes: 1. **Smart Early Exit**: Check if track is already 'showing' with cues loaded. If yes, skip processing and mark as initialized. 2. **Verify Cues After Activation**: Only set subtitlesInitialized flag to true after verifying cues are actually loaded (200ms after mode change). This prevents false positives. 3. **Restore Multiple Retries**: Added back 3 retry attempts at 300ms, 600ms, and 1000ms. These catch timing edge cases. 4. **Allow Multiple Calls**: Removed overly aggressive guard that prevented any retry. Now checks if already working first, but allows retries if not yet successful. The issue was setting subtitlesInitialized=true before cues were loaded, then blocking all retries. Now we verify success before blocking retries. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added detailed console logging at every step to identify why subtitles stopped working after cleanup. Debug logs added: - Track count and initialization status - Track mode and cue count for each track - Mode cycling start/progress/completion - Cue verification results - Metadata loading status - Retry attempt triggers This will help identify: - If tracks are found - If mode cycling is happening - If cues are loading - If retries are triggering - Where the process is failing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Root cause identified: Mode cycling was attempted before the browser loaded/parsed the VTT file. The track started in 'showing' mode from ReactPlayer config, but with 0 cues because the VTT wasn't loaded yet. Solution: 1. **Check Track Element readyState**: Before mode cycling, verify trackElement.readyState === 2 (LOADED). If not loaded, skip cycling. 2. **Listen for Load Event**: Added event listener for track element's 'load' event. When VTT file loads, force track.mode = 'showing' and mark as initialized if cues are present. 3. **Backup Cuechange Listener**: Also listen for 'cuechange' event as a fallback to catch when cues become available. 4. **Conditional Mode Cycling**: Only perform the disabled→hidden→showing cycle if track is already loaded. Otherwise, just set to 'showing' and let event listeners handle activation when ready. The key insight: ReactPlayer sets track.mode='showing' immediately via the config default:true, but the browser hasn't loaded the VTT yet. We need to wait for the actual load event rather than forcing mode changes prematurely. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant UI as VideoPlayer (component)
participant Video as HTMLVideoElement
participant Tracks as TextTrackList / VTTCues
rect rgb(235,245,255)
UI->>Video: onReady()
note right of UI: initializeSubtitleTracks()
UI->>Video: inspect existing TextTracks
alt cues present & usable
Tracks-->>UI: cues available
UI->>Video: select/enable track
else need injection/load
UI->>Video: add <track> elements (addSubtitleTracksToVideo)
Video->>Video: load & parse VTT
Tracks-->>UI: cuechange / load result
alt cues loaded
UI->>UI: set subtitlesInitialized = true
else load failed
UI->>UI: retry (initializationAttempts < maxAttempts)
end
end
end
rect rgb(245,255,235)
Video->>UI: onPlay()
UI->>UI: ensure initializeSubtitleTracks if not initialized
note right of UI: reset init state when src/id changes -> re-run onReady/onPlay
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Removed all setTimeout calls with arbitrary delays (300ms, 600ms, 1000ms)
in favor of a reliable event-driven implementation.
## Problems with Previous Approach
1. **Magic Numbers**: Used arbitrary timeouts that might be too short/long
2. **Unreliable**: Depended on guessing when VTT files would load
3. **Complex**: Multiple retry attempts with nested setTimeout calls
4. **Wasteful**: Called activation logic multiple times unnecessarily
## New Pure Event-Driven Approach
### How It Works
```typescript
// Check current state
if (trackElement.readyState === 2 && track.cues?.length > 0) {
// Already loaded - activate immediately
track.mode = 'showing';
} else {
// Not loaded yet - wait for browser events
trackElement.addEventListener('load', activateTrack);
track.addEventListener('cuechange', activateTrack);
}
```
### Key Benefits
1. **No Magic Numbers**: Zero setTimeout calls
2. **Reliable**: Activates exactly when VTT is ready (event-driven)
3. **Simple**: Single activation point, clean logic flow
4. **Efficient**: Only processes once (guard at function start)
### How It Handles Different Scenarios
**Fast Connection (VTT loads immediately):**
- readyState === 2 on first check
- Activates immediately, no waiting
**Slow Connection (VTT loads later):**
- readyState !== 2 on first check
- Event listeners fire when ready
- Activates when browser completes loading
**Already Playing (late initialization):**
- Cues already available from browser
- Activates immediately
### Changes Made
- Removed all setTimeout/retry logic
- Simplified to single activation function
- Added readyState check for immediate activation
- Used load + cuechange events for delayed activation
- Removed unnecessary handlePlayWithSubtitles wrapper
- Added guard to prevent duplicate processing
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/watch/videos/video-player/index.tsx (1)
84-88: Null-out ref on unmount to avoid stale instanceThe ref callback early-returns on null, leaving a stale playerRef when the component unmounts.
- const setPlayerRef = useCallback((player: any) => { - if (!player) return; - playerRef.current = player; - }, []); + const setPlayerRef = useCallback((player: any | null) => { + playerRef.current = player; + }, []);
🧹 Nitpick comments (3)
packages/ui/src/watch/videos/video-player/index.tsx (3)
90-118: Remove or gate the unused VTT debug helper; drop from depstestVttFile is a no-op and only inflates the dependency list. Keep it behind a dev flag or remove, and exclude from initializeSubtitleTracks deps.
- const testVttFile = useCallback(async (url: string, lang: string) => { + // Optional: enable locally for debugging VTT issues + const testVttFile = useCallback(async (_url: string, _lang: string) => { /* ...debug-only code... */ }, []);And update the dependency list on initializeSubtitleTracks (see below).
119-269: Subtitle init: clean up timeouts, simplify listeners, and tighten loggingSolid approach (readyState gate, load/cuechange fallbacks, retries). Recommend small robustness/DX tweaks:
- Track and clear retry timeouts on video change/unmount.
- Remove redundant loadedmetadata removal (listener already { once: true }).
- Log error event object instead of non-standard trackElement.error.
- Avoid re-querying the same track element.
const initializeSubtitleTracks = useCallback(() => { if (!playerRef.current) return; @@ - const activateDefaultTrack = () => { + const activateDefaultTrack = () => { console.log('Activating tracks, count:', textTracks.length, 'initialized:', subtitlesInitialized.current); @@ - for (let i = 0; i < textTracks.length; i++) { + for (let i = 0; i < textTracks.length; i++) { const track = textTracks[i]; - console.log('Track status:', { + console.log('Track status:', { language: track.language, mode: track.mode, cueCount: track.cues?.length || 0 }); @@ - const trackElement = internalPlayer.querySelector(`track[srclang="${track.language}"]`) as HTMLTrackElement; + const trackElement = internalPlayer.querySelector(`track[srclang="${track.language}"]`) as HTMLTrackElement | null; if (trackElement) { console.log('Track element readyState:', trackElement.readyState); // Listen for track errors - trackElement.addEventListener('error', (e) => { - console.error('❌ Subtitle track error:', trackElement.error); - }, { once: true }); + trackElement.addEventListener('error', (e) => { + console.error('❌ Subtitle track error for', trackElement.src, e); + }, { once: true }); @@ - if (matchingSubtitle?.isDefault) { - // Only do mode cycling if track is already loaded - const trackElem = internalPlayer.querySelector(`track[srclang="${track.language}"]`) as HTMLTrackElement; - if (trackElem && trackElem.readyState === 2) { // LOADED + if (matchingSubtitle?.isDefault) { + // Only do mode cycling if track is already loaded + if (trackElement && trackElement.readyState === 2) { // LOADED console.log('→ Track is loaded, starting mode cycle for:', track.language); // Force track mode cycle to trigger VTT parsing track.mode = 'disabled'; @@ // Multiple retry attempts to ensure consistency - const retryDelays = [300, 600, 1000]; - retryDelays.forEach((delay) => { - setTimeout(() => { + const retryDelays = [300, 600, 1000] as const; + retryDelays.forEach((delay) => { + const id = window.setTimeout(() => { if (!subtitlesInitialized.current) { console.log(`Retry attempt after ${delay}ms`); activateDefaultTrack(); } - }, delay); + }, delay); + // store for cleanup + // @ts-ignore - window.setTimeout returns number in browser + timeoutsRef.current.push(id); }); - // Cleanup event listener - setTimeout(() => { - internalPlayer.removeEventListener('loadedmetadata', handleLoadedMetadata); - }, 2000); + // No explicit removal needed for loadedmetadata because we used { once: true }Add the timeout bucket and cleanup:
@@ - const subtitlesInitialized = useRef(false); + const subtitlesInitialized = useRef(false); + const timeoutsRef = useRef<number[]>([]); @@ - useEffect(() => { - subtitlesInitialized.current = false; - }, [video.id, video.source]); + useEffect(() => { + subtitlesInitialized.current = false; + return () => { + timeoutsRef.current.forEach(clearTimeout); + timeoutsRef.current = []; + }; + }, [video.id, video.source]);
546-548: CORS and preload notes for trackscrossOrigin: 'anonymous' is the right value (not 'true'). Ensure your VTT endpoints send Access-Control-Allow-Origin accordingly; otherwise tracks will fail silently. preload: 'metadata' affects the video element, not guaranteed to accelerate VTT fetch, but harmless here.
If subtitles are served from a different domain, verify CORS headers in staging with DevTools (track request should be CORS mode=anonymous).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui/src/watch/videos/types.ts(1 hunks)packages/ui/src/watch/videos/video-player/index.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{ts,tsx}: Always use ES modules
Always use async/await syntax if possible
Never use thefunctionkeyword; always use arrow functions (e.g., const method = async () =>)
All exports must be named exports and should be placed at the bottom of the file
Prefer defining an interface for method parameters to keep the method definition on one line
Files:
packages/ui/src/watch/videos/types.tspackages/ui/src/watch/videos/video-player/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: prod_deploy
🔇 Additional comments (3)
packages/ui/src/watch/videos/types.ts (1)
55-56: LGTM: public type re-exportAdding Subtitle to the named export block is correct and consistent with the module’s export pattern.
packages/ui/src/watch/videos/video-player/index.tsx (2)
478-484: Good fallback trigger on playWrapping handlePlay to retry subtitle init on playback start is a pragmatic safety net for slow networks.
Please sanity-check that initializeSubtitleTracks is cheap enough to call multiple times; it appears idempotent via subtitlesInitialized, but confirm no flicker on repeated play/pause.
553-555: Track labeling addition is correctProviding label improves browser track UI and doesn’t affect programmatic activation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ui/src/watch/videos/video-player/index.tsx (1)
91-117: Consider removing or extracting the commented debug function.The
testVttFilefunction is entirely commented out and unused. Consider either removing it to reduce clutter or extracting it to a separate debugging utility file if it's valuable for future debugging sessions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/watch/videos/video-player/index.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{ts,tsx}: Always use ES modules
Always use async/await syntax if possible
Never use thefunctionkeyword; always use arrow functions (e.g., const method = async () =>)
All exports must be named exports and should be placed at the bottom of the file
Prefer defining an interface for method parameters to keep the method definition on one line
Files:
packages/ui/src/watch/videos/video-player/index.tsx
🪛 GitHub Actions: Unit Test on PR
packages/ui/src/watch/videos/video-player/index.tsx
[error] 1-1: UI tests failed. Command 'pnpm run test' exited with code 1.
🔇 Additional comments (8)
packages/ui/src/watch/videos/video-player/index.tsx (8)
81-81: LGTM: Proper use of ref for tracking initialization state.The ref prevents duplicate initialization attempts when
onReadyfires multiple times or the component re-renders.
119-195: PR description mentions retry logic that doesn't exist in the implementation.The PR description states: "Implement multiple retry attempts at 300ms, 600ms, and 1000ms to handle varying timing scenarios." However, the actual implementation is purely event-driven without any timeout-based retries. While the event-driven approach is arguably better than arbitrary timeouts, this inconsistency between the PR description and implementation could confuse future maintainers.
Consider updating the PR description to accurately reflect the event-driven implementation, or clarify if timeout-based retries were intentionally replaced with the event-driven approach.
407-409: LGTM: Proper reset of subtitle state on video change.The effect correctly resets the initialization flag when the video ID or source changes, ensuring subtitles are reinitialized for each video.
459-461: LGTM: Appropriate timing for subtitle initialization.Calling
initializeSubtitleTracks()in theonReadycallback ensures the internal player is fully initialized before attempting to access subtitle tracks.
474-474: LGTM: Label attribute improves accessibility.Adding the
labelattribute to subtitle tracks provides better user-facing descriptions in the subtitle menu.
133-135: Verify querySelector works on internalPlayer.The code assumes
internalPlayerhas aquerySelectormethod, but ReactPlayer's internal player type varies by source (HTML5 video element, YouTube iframe, etc.). For HTML5 video, this should work, but ensure this doesn't cause runtime errors with other player types.
467-468: Verify necessity ofpreload: 'metadata'attribute.The comment states it "forces browser to load subtitle tracks immediately," but
preload: 'metadata'only loads video metadata, not necessarily the subtitle track VTT files. The event-driven approach ininitializeSubtitleTracksshould handle subtitle loading regardless of this attribute. Consider whether this attribute is actually necessary or if it causes unnecessary network requests when users don't play the video.Test subtitle loading with and without this attribute to confirm it's needed:
1-1: Address the failing UI tests before merging.The pipeline shows that unit tests are failing. The new subtitle initialization logic likely requires test updates or new tests to cover the event-driven subtitle activation flow. Please investigate and fix the test failures.
| if (matchingSubtitle.isDefault) { | ||
| // This is the default subtitle track we want to activate | ||
| const activateTrack = () => { | ||
| if (track.cues && track.cues.length > 0) { | ||
| track.mode = 'showing'; | ||
| subtitlesInitialized.current = true; | ||
| console.log('✓ Subtitles activated:', track.language, track.cues.length, 'cues'); | ||
| } | ||
| }; | ||
|
|
||
| // Check if already loaded | ||
| if (trackElement.readyState === 2 && track.cues && track.cues.length > 0) { | ||
| // Already loaded and has cues - activate immediately | ||
| activateTrack(); | ||
| } else { | ||
| // Not loaded yet - set up event listeners | ||
| track.mode = 'showing'; // Set to showing so browser will load it | ||
|
|
||
| // Listen for track load event | ||
| trackElement.addEventListener('load', activateTrack, { once: true }); | ||
|
|
||
| // Backup: listen for cuechange event | ||
| track.addEventListener( | ||
| 'cuechange', | ||
| () => { | ||
| if (!subtitlesInitialized.current) { | ||
| activateTrack(); | ||
| } | ||
| }, | ||
| { once: true } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Redundant track.mode assignment may cause browser inconsistencies.
The code sets track.mode = 'showing' twice:
- Line 171: Before the track loads (to force browser to load it)
- Line 159: In
activateTrack()after cues are loaded
This redundancy could cause browser-specific behavior. According to the HTML5 spec, setting mode to 'showing' before the track loads should work, but the double assignment is unnecessary. Consider removing the assignment at line 159 if line 171 already forces the load.
Apply this diff to eliminate redundancy:
if (matchingSubtitle.isDefault) {
// This is the default subtitle track we want to activate
const activateTrack = () => {
if (track.cues && track.cues.length > 0) {
- track.mode = 'showing';
subtitlesInitialized.current = true;
console.log('✓ Subtitles activated:', track.language, track.cues.length, 'cues');
}
};
// Check if already loaded
if (trackElement.readyState === 2 && track.cues && track.cues.length > 0) {
// Already loaded and has cues - activate immediately
+ track.mode = 'showing';
activateTrack();
} else {
// Not loaded yet - set up event listeners
track.mode = 'showing'; // Set to showing so browser will load it
// Listen for track load event
trackElement.addEventListener('load', activateTrack, { once: true });
// Backup: listen for cuechange event
track.addEventListener(
'cuechange',
() => {
if (!subtitlesInitialized.current) {
activateTrack();
}
},
{ once: true }
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (matchingSubtitle.isDefault) { | |
| // This is the default subtitle track we want to activate | |
| const activateTrack = () => { | |
| if (track.cues && track.cues.length > 0) { | |
| track.mode = 'showing'; | |
| subtitlesInitialized.current = true; | |
| console.log('✓ Subtitles activated:', track.language, track.cues.length, 'cues'); | |
| } | |
| }; | |
| // Check if already loaded | |
| if (trackElement.readyState === 2 && track.cues && track.cues.length > 0) { | |
| // Already loaded and has cues - activate immediately | |
| activateTrack(); | |
| } else { | |
| // Not loaded yet - set up event listeners | |
| track.mode = 'showing'; // Set to showing so browser will load it | |
| // Listen for track load event | |
| trackElement.addEventListener('load', activateTrack, { once: true }); | |
| // Backup: listen for cuechange event | |
| track.addEventListener( | |
| 'cuechange', | |
| () => { | |
| if (!subtitlesInitialized.current) { | |
| activateTrack(); | |
| } | |
| }, | |
| { once: true } | |
| ); | |
| } | |
| if (matchingSubtitle.isDefault) { | |
| // This is the default subtitle track we want to activate | |
| const activateTrack = () => { | |
| if (track.cues && track.cues.length > 0) { | |
| subtitlesInitialized.current = true; | |
| console.log('✓ Subtitles activated:', track.language, track.cues.length, 'cues'); | |
| } | |
| }; | |
| // Check if already loaded | |
| if (trackElement.readyState === 2 && track.cues && track.cues.length > 0) { | |
| // Already loaded and has cues - activate immediately | |
| track.mode = 'showing'; | |
| activateTrack(); | |
| } else { | |
| // Not loaded yet - set up event listeners | |
| track.mode = 'showing'; // Set to showing so browser will load it | |
| // Listen for track load event | |
| trackElement.addEventListener('load', activateTrack, { once: true }); | |
| // Backup: listen for cuechange event | |
| track.addEventListener( | |
| 'cuechange', | |
| () => { | |
| if (!subtitlesInitialized.current) { | |
| activateTrack(); | |
| } | |
| }, | |
| { once: true } | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/ui/src/watch/videos/video-player/index.tsx around lines 155 to 186,
remove the redundant assignment track.mode = 'showing' inside activateTrack (the
assignment at ~line 159) and keep the single assignment that forces the browser
to load the track (the one at ~line 171); ensure activateTrack still sets
subtitlesInitialized.current = true and logs activation so loaded tracks are
marked active without duplicating mode changes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/ui/src/watch/videos/video-player/index.tsx (3)
433-446: Clear pending init timers on video change.Outstanding timeouts can race with the new source and re-flip modes. Clear them when resetting state.
useEffect(() => { + // Cancel any scheduled retries + if (initTimeoutsRef.current?.length) { + initTimeoutsRef.current.forEach((id) => window.clearTimeout(id)); + initTimeoutsRef.current = []; + } subtitlesInitialized.current = false; initializationAttempts.current = 0; // Re-trigger initialization if player is ready if (playerRef.current) { // Small delay to ensure video has changed const timer = setTimeout(() => { initializeSubtitleTracks(); }, 100); return () => clearTimeout(timer); } }, [video.id, video.source, initializeSubtitleTracks]);
447-452: Cleanup: also clear subtitle init timeouts on unmount.Prevents late-firing retries after unmount.
useEffect(() => { return () => { + if (initTimeoutsRef.current?.length) { + initTimeoutsRef.current.forEach((id) => window.clearTimeout(id)); + initTimeoutsRef.current = []; + } cleanup(); }; }, [cleanup]);
509-511: CORS check for VTT delivery.With crossOrigin='anonymous', ensure VTT endpoints return appropriate CORS headers (e.g., Access-Control-Allow-Origin) or cues won’t load on some browsers/CDNs.
Would you confirm the VTT servers respond with the expected CORS headers in staging and prod?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/watch/videos/video-player/index.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{ts,tsx}: Always use ES modules
Always use async/await syntax if possible
Never use thefunctionkeyword; always use arrow functions (e.g., const method = async () =>)
All exports must be named exports and should be placed at the bottom of the file
Prefer defining an interface for method parameters to keep the method definition on one line
Files:
packages/ui/src/watch/videos/video-player/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: prod_deploy
🔇 Additional comments (1)
packages/ui/src/watch/videos/video-player/index.tsx (1)
489-496: LGTM: onPlay safeguard for light mode.Conditionally triggering initialization on play covers the thumbnail/light case without double-initializing.
| // Add subtitle tracks directly to the video element | ||
| const addSubtitleTracksToVideo = useCallback( | ||
| (videoElement: HTMLVideoElement) => { | ||
| if (!subtitles || subtitles.length === 0) return; | ||
|
|
||
| // Remove existing tracks to avoid duplicates | ||
| const existingTracks = videoElement.querySelectorAll('track'); | ||
| existingTracks.forEach((track) => { | ||
| track.remove(); | ||
| }); | ||
|
|
||
| // Add each subtitle as a track element | ||
| subtitles.forEach((subtitle) => { | ||
| const track = document.createElement('track'); | ||
| track.kind = 'subtitles'; | ||
| track.label = subtitle.label; | ||
| track.srclang = subtitle.lang; | ||
| track.src = subtitle.src; | ||
|
|
||
| if (subtitle.isDefault) { | ||
| track.default = true; | ||
| } | ||
|
|
||
| videoElement.appendChild(track); | ||
| }); | ||
|
|
||
| // Tracks added successfully | ||
| }, | ||
| [subtitles], | ||
| ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep references to created elements for event-driven activation.
Without storing HTMLTrackElement refs you can’t check readyState or attach 'load' listeners later. Store them in a ref map keyed by lang and clear it when rebuilding tracks.
@@
- const addSubtitleTracksToVideo = useCallback(
- (videoElement: HTMLVideoElement) => {
+ const subtitleTrackElsRef = useRef<Map<string, HTMLTrackElement>>(new Map());
+ const addSubtitleTracksToVideo = useCallback(
+ (videoElement: HTMLVideoElement) => {
if (!subtitles || subtitles.length === 0) return;
// Remove existing tracks to avoid duplicates
const existingTracks = videoElement.querySelectorAll('track');
existingTracks.forEach((track) => {
track.remove();
});
+ // Reset our refs map
+ subtitleTrackElsRef.current.clear();
+
// Add each subtitle as a track element
subtitles.forEach((subtitle) => {
const track = document.createElement('track');
track.kind = 'subtitles';
track.label = subtitle.label;
track.srclang = subtitle.lang;
track.src = subtitle.src;
if (subtitle.isDefault) {
track.default = true;
}
videoElement.appendChild(track);
+ subtitleTrackElsRef.current.set(subtitle.lang, track);
});
// Tracks added successfully
},
[subtitles],
);🤖 Prompt for AI Agents
In packages/ui/src/watch/videos/video-player/index.tsx around lines 121 to 151,
the current function creates <track> elements but doesn't keep references to
them; change it to store created HTMLTrackElement references in a useRef map
keyed by subtitle.lang (or another unique key) before appending to the video
element, clear the map (and remove existing entries) when rebuilding tracks, and
use those refs to attach 'load' listeners and check track.readyState later; also
ensure the ref map is cleaned up on component unmount or when subtitles change
to avoid stale refs.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (21.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #268 +/- ##
==========================================
- Coverage 80.66% 80.01% -0.66%
==========================================
Files 203 203
Lines 8737 8823 +86
Branches 1024 1026 +2
==========================================
+ Hits 7048 7060 +12
- Misses 1689 1763 +74
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/ui/src/watch/videos/video-player/index.tsx (3)
81-84: Add refs for track elements and pending timeouts to enable event-driven init and cleanup.You’ll need a Map<string, HTMLTrackElement> to find tracks later and a list to clear scheduled retries on unmount/video change.
Apply:
const playerRef = useRef<any | null>(null); const wrapperRef = useRef<HTMLDivElement | null>(null); const subtitlesInitialized = useRef(false); const initializationAttempts = useRef(0); const maxAttempts = 10; // Maximum retry attempts + const subtitleTrackElsRef = useRef<Map<string, HTMLTrackElement>>(new Map()); + const initTimeoutsRef = useRef<number[]>([]);
92-121: Store created elements; clear refs when rebuilding.Keep references when injecting tracks so you can check readyState, attach 'load' listeners, and avoid DOM queries later. Also reset the map when rebuilding.
const addSubtitleTracksToVideo = useCallback( (videoElement: HTMLVideoElement) => { if (!subtitles || subtitles.length === 0) return; // Remove existing tracks to avoid duplicates const existingTracks = videoElement.querySelectorAll('track'); existingTracks.forEach((track) => { track.remove(); }); + // Reset refs map + subtitleTrackElsRef.current.clear(); + // Add each subtitle as a track element subtitles.forEach((subtitle) => { const track = document.createElement('track'); track.kind = 'subtitles'; track.label = subtitle.label; track.srclang = subtitle.lang; track.src = subtitle.src; if (subtitle.isDefault) { track.default = true; } videoElement.appendChild(track); + subtitleTrackElsRef.current.set(subtitle.lang, track); }); // Tracks added successfully }, - [subtitles], + [subtitles], );
123-192: Race persists; implement readyState checks + 'load'/'cuechange' listeners with controlled backoff.Current logic still polls and sets mode immediately. Align with PR objective: only cycle mode after LOADED; otherwise set showing to trigger load, listen for 'load'/cue availability, and retry at 300/600/1000ms. Also track and clear timeouts.
- // Initialize subtitles by ensuring the default track is set to 'showing' + // Initialize subtitles: event-driven, with backoff and idempotency const initializeSubtitleTracks = useCallback(() => { if (!subtitles || subtitles.length === 0) return; if (!playerRef.current) return; if (subtitlesInitialized.current) return; - if (initializationAttempts.current >= maxAttempts) { - return; // Silently stop after max attempts - } + if (initializationAttempts.current >= maxAttempts) return; initializationAttempts.current += 1; try { const internalPlayer = playerRef.current.getInternalPlayer(); - if (!internalPlayer || !internalPlayer.textTracks) { - if (initializationAttempts.current < maxAttempts) { - setTimeout(() => initializeSubtitleTracks(), 200); - } - return; - } + if (!internalPlayer) return; // Add tracks directly to video element if it's a video tag if (internalPlayer.tagName === 'VIDEO') { addSubtitleTracksToVideo(internalPlayer); } - const textTracks = internalPlayer.textTracks; - - if (textTracks.length === 0) { - if (initializationAttempts.current < maxAttempts) { - setTimeout(() => initializeSubtitleTracks(), 200); - } - return; - } - - // Find and activate the default track - for (let i = 0; i < textTracks.length; i++) { - const track = textTracks[i]; - - const matchingSubtitle = subtitles?.find( - (sub) => sub.lang === track.language, - ); - - if (matchingSubtitle?.isDefault) { - // Set mode to showing to trigger loading and display - track.mode = 'showing'; - - // If cues are already loaded, mark as initialized - if (track.cues && track.cues.length > 0) { - subtitlesInitialized.current = true; - return; - } - } - } - - // Retry if not initialized yet - if ( - !subtitlesInitialized.current && - initializationAttempts.current < maxAttempts - ) { - setTimeout(() => initializeSubtitleTracks(), 300); - } + // Determine default subtitle + const defaultSub = subtitles.find((s) => s.isDefault); + if (!defaultSub) return; + + // Find the corresponding HTMLTrackElement + const videoEl: HTMLVideoElement | null = + internalPlayer.tagName === 'VIDEO' ? internalPlayer : null; + const trackEl = + subtitleTrackElsRef.current.get(defaultSub.lang) || + (videoEl + ? (videoEl.querySelector( + `track[srclang="${defaultSub.lang}"]`, + ) as HTMLTrackElement | null) + : null); + + const scheduleRetry = () => { + const delays = [300, 600, 1000]; + const idx = Math.min(initializationAttempts.current - 1, delays.length - 1); + const id = window.setTimeout(() => initializeSubtitleTracks(), delays[idx]); + initTimeoutsRef.current.push(id); + }; + + if (!trackEl) { + scheduleRetry(); + return; + } + + const textTrack = trackEl.track as TextTrack; + const cycleIfLoaded = () => { + try { + textTrack.mode = 'disabled'; + textTrack.mode = 'hidden'; + textTrack.mode = 'showing'; + } catch { + textTrack.mode = 'showing'; + } + }; + const markInitializedIfReady = () => { + if (!subtitlesInitialized.current && textTrack.cues && textTrack.cues.length > 0) { + subtitlesInitialized.current = true; + } + }; + + // If already LOADED (2), cycle and mark. + // readyState values on HTMLTrackElement: 0 NONE, 1 LOADING, 2 LOADED, 3 ERROR + if ((trackEl as any).readyState === 2) { + cycleIfLoaded(); + markInitializedIfReady(); + return; + } + + // Not loaded yet: set mode to 'showing' to trigger load, then listen. + textTrack.mode = 'showing'; + + trackEl.addEventListener('load', () => { + // Loaded: cues should exist; no need to set mode again. + markInitializedIfReady(); + }, { once: true }); + + textTrack.addEventListener('cuechange', () => { + markInitializedIfReady(); + }, { once: true }); + + if (!subtitlesInitialized.current) { + scheduleRetry(); + } } catch (error) { console.error('Error initializing subtitles:', error); - if (initializationAttempts.current < maxAttempts) { - setTimeout(() => initializeSubtitleTracks(), 300); - } + if (initializationAttempts.current < maxAttempts) { + const id = window.setTimeout(() => initializeSubtitleTracks(), 300); + initTimeoutsRef.current.push(id); + } } }, [subtitles, addSubtitleTracksToVideo]);
🧹 Nitpick comments (1)
packages/ui/src/watch/videos/video-player/index.test.tsx (1)
146-157: Tests updated correctly; consider adding an event-driven subtitle init test.Assertions align with config changes (playsInline + crossOrigin, tracks not in config). Add a focused test that stubs getInternalPlayer() to a VIDEO element, appends a track, dispatches a 'load' event or simulates cues to verify event-driven activation runs once and doesn’t rely on polling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui/src/watch/videos/video-player/index.test.tsx(1 hunks)packages/ui/src/watch/videos/video-player/index.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{ts,tsx}: Always use ES modules
Always use async/await syntax if possible
Never use thefunctionkeyword; always use arrow functions (e.g., const method = async () =>)
All exports must be named exports and should be placed at the bottom of the file
Prefer defining an interface for method parameters to keep the method definition on one line
Files:
packages/ui/src/watch/videos/video-player/index.test.tsxpackages/ui/src/watch/videos/video-player/index.tsx
{**/test/**/*.{ts,tsx},**/*.test.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files in test/ directories or alongside source files using *.test.ts/tsx naming
Files:
packages/ui/src/watch/videos/video-player/index.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: prod_deploy
🔇 Additional comments (4)
packages/ui/src/watch/videos/video-player/index.tsx (4)
300-359: Hotkey seek now reports via handleSeek — good.
460-467: Triggering subtitle init on play is correct with light thumbnails; guarded by idempotency.Ensure onReady + onPlay don’t double-init in edge cases; your subtitlesInitialized guard should make this safe.
474-475: onReady hook to initialize subtitles — LGTM.
480-482: playsInline + crossOrigin('anonymous') via ReactPlayer config — LGTM.Matches the updated tests and CORS best practice for VTT.
| // No need for fallback - event listeners handle everything | ||
|
|
There was a problem hiding this comment.
Comment is misleading.
It says “No need for fallback - event listeners handle everything,” but there are no listeners. Either implement listeners (see suggested diff) or update/remove the comment.
🤖 Prompt for AI Agents
In packages/ui/src/watch/videos/video-player/index.tsx around lines 402-403, the
inline comment "No need for fallback - event listeners handle everything" is
incorrect because no event listeners exist; either remove or update the comment,
or implement the missing listeners: attach appropriate event listeners (e.g.,
play, pause, ended, timeupdate) to the video element via the component's ref
inside a useEffect, call the existing handler functions on those events, and
ensure you remove listeners in the cleanup to avoid leaks; if you choose not to
add listeners, simply delete or rewrite the comment to reflect the current
implementation.
| // Reset subtitle initialization when video changes | ||
| useEffect(() => { | ||
| subtitlesInitialized.current = false; | ||
| initializationAttempts.current = 0; | ||
| // Re-trigger initialization if player is ready | ||
| if (playerRef.current) { | ||
| // Small delay to ensure video has changed | ||
| const timer = setTimeout(() => { | ||
| initializeSubtitleTracks(); | ||
| }, 100); | ||
| return () => clearTimeout(timer); | ||
| } | ||
| }, [initializeSubtitleTracks]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clear scheduled retries on video change to avoid orphaned timers.
Track timeout IDs and clear them when resetting init state.
useEffect(() => {
subtitlesInitialized.current = false;
initializationAttempts.current = 0;
+ // Cancel pending retries for the previous video
+ initTimeoutsRef.current.forEach((id) => clearTimeout(id));
+ initTimeoutsRef.current = [];
// Re-trigger initialization if player is ready
if (playerRef.current) {
// Small delay to ensure video has changed
const timer = setTimeout(() => {
initializeSubtitleTracks();
}, 100);
return () => clearTimeout(timer);
}
}, [initializeSubtitleTracks]);🤖 Prompt for AI Agents
packages/ui/src/watch/videos/video-player/index.tsx around lines 404 to 417: the
effect that resets subtitle initialization schedules a setTimeout but does not
track or clear its ID, which can leave orphaned timers when the video changes;
create a ref (e.g., subtitleInitTimerRef) to hold the timeout ID (or an array if
multiple timers are used), clear any existing timeout(s) from that ref before
setting a new timer, store the new timeout ID in the ref, and ensure the effect
cleanup clears the stored timeout(s) to avoid stray callbacks running after the
video has changed.
|
Close this shit, too low level implementation, no consistent working behavior, just introduce more bugs |
2 participants
Why It Was Inconsistent
Solution
Implemented event-driven activation that waits for VTT files to actually load before trying to activate them.
Key Changes
Check Track Element readyState: Before attempting mode cycling, verify
trackElement.readyState === 2(LOADED)Event-Driven Activation: If VTT not loaded yet, attach listeners:
loadevent on track element - fires when VTT file is loaded and parsedcuechangeevent as backup - fires when cues become activeConditional Mode Cycling: Only perform the
disabled → hidden → showingcycle if track is already loadedMultiple Retry Strategy: Added retry attempts at 300ms, 600ms, and 1000ms to catch different timing scenarios
Code Flow