fix: where onProgress is not called when seekTo is invoked #15
Conversation
- add TSDoc documentation - add defensive logic for cases without videoId - fix issue where seekTo doesn't work properly when paused without interval
🦋 Changeset detectedLatest commit: 2904ac5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update improves the YouTube player integration by ensuring the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant YoutubePlayer
participant Iframe
participant youtubeIframeScripts
App->>YoutubePlayer: Calls seekTo(newTime)
YoutubePlayer->>Iframe: Sends seekTo command
Iframe->>youtubeIframeScripts: Executes seekTo
youtubeIframeScripts->>Iframe: Seeks to new time
Note over youtubeIframeScripts: Waits 200ms
youtubeIframeScripts->>Iframe: Calls sendProgress
Iframe->>YoutubePlayer: Sends progress update
YoutubePlayer->>App: Invokes onProgress callback
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying react-native-youtube-bridge with
|
| Latest commit: |
2904ac5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1b7afef3.react-native-youtube-bridge.pages.dev |
| Branch Preview URL: | https://fix-call-seek-to-progress.react-native-youtube-bridge.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/utils/validate.ts (1)
1-4: Acceptsundefinedbut notnull– consider widening the type for completeness
videoIdcan now be omitted, which is great. However, callers that might passnull(e.g. values coming straight from JSON) will still be rejected at the TS level even though the runtime implementation gracefully coalescesnullto an empty string.-export const validateVideoId = (videoId?: string): boolean => { +export const validateVideoId = (videoId?: string | null): boolean => {Purely a nicety; feel free to skip if you are certain
nullwill never propagate..changeset/gentle-houses-attack.md (1)
5-8: Minor wording / punctuation polish for the release note-fix: where onProgress is not called when seekTo is invoked +fix: onProgress not emitted when seekTo is invoked - - add defensive logic for cases without videoId + - add defensive logic for missing `videoId` - - fix issue where seekTo doesn't work properly when paused without interval + - fix seekTo when video is paused and no interval is activeTotally optional, but reads a bit smoother.
src/hooks/useCreateLocalPlayerHtml.ts (1)
137-147: Magic200 msdelay – expose as a constant for easier tuningA hard-coded timeout is fine today, but if experimentation shows 200 ms is too short/long you’ll need to reopen this template string. Consider declaring a file-level constant so the value can be tweaked in one spot:
- setTimeout(() => { - sendProgress(); - }, 200); + const SEEK_PROGRESS_DELAY = 200; // ms + setTimeout(sendProgress, SEEK_PROGRESS_DELAY);Very minor, yet improves maintainability.
src/hooks/youtubeIframeScripts.ts (1)
47-68:sendProgressduplicates the serialization logic – DRY up with a shared helperBoth
startProgressTracking’s interval body andsendProgressbuild an identical payload. Extract the common code to a singlepostProgress()helper and call it from both places:+ function postProgress() { + const currentTime = player.getCurrentTime(); + const duration = player.getDuration(); + const percentage = duration > 0 ? (currentTime / duration) * 100 : 0; + const loadedFraction = player.getVideoLoadedFraction(); + + window.ReactNativeWebView.postMessage(JSON.stringify({ + type: 'progress', + currentTime, + duration, + percentage, + loadedFraction, + })); + } + function sendProgress() { if (player && player.getCurrentTime) { try { - const currentTime = player.getCurrentTime(); - const duration = player.getDuration(); - const percentage = duration > 0 ? (currentTime / duration) * 100 : 0; - const loadedFraction = player.getVideoLoadedFraction(); - - window.ReactNativeWebView.postMessage(JSON.stringify({ - type: 'progress', - currentTime, - duration, - percentage, - loadedFraction, - })); + postProgress(); } catch (error) { console.error('Final progress error:', error); } } }Reduces duplication and keeps any future payload tweaks consistent.
src/YoutubePlayer.web.tsx (1)
194-221: StreamlineonStateChangewith aswitchto avoid repetitive branchesThe current cascade of
ifstatements repeats common actions (stopProgressTracking,sendProgress,startProgressTracking), making it harder to see what each state actually does.- if (state === PlayerState.ENDED) { - stopProgressTracking(); - sendProgress(); - return; - } - ... - if (state === PlayerState.CUED) { - stopProgressTracking(); - sendProgress(); - return; - } - - stopProgressTracking(); + switch (state) { + case PlayerState.ENDED: + case PlayerState.PAUSED: + case PlayerState.CUED: + stopProgressTracking(); + sendProgress(); + break; + case PlayerState.PLAYING: + case PlayerState.BUFFERING: + startProgressTracking(); + break; + default: + stopProgressTracking(); + }This reduces cognitive load and prevents future divergence when additional states are introduced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/gentle-houses-attack.md(1 hunks)src/YoutubePlayer.web.tsx(6 hunks)src/hooks/useCreateLocalPlayerHtml.ts(2 hunks)src/hooks/youtubeIframeScripts.ts(4 hunks)src/types/youtube.ts(2 hunks)src/utils/validate.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/hooks/useCreateLocalPlayerHtml.ts (1)
src/hooks/youtubeIframeScripts.ts (1)
youtubeIframeScripts(275-286)
src/YoutubePlayer.web.tsx (2)
src/utils/validate.ts (1)
validateVideoId(1-5)src/index.tsx (1)
PlayerState(3-3)
🪛 LanguageTool
.changeset/gentle-houses-attack.md
[style] ~8-~8: Consider using a different verb for a more formal wording.
Context: ...nsive logic for cases without videoId - fix issue where seekTo doesn't work properl...
(FIX_RESOLVE)
[uncategorized] ~8-~8: You might be missing the article “an” here.
Context: ...esn't work properly when paused without interval
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
src/hooks/useCreateLocalPlayerHtml.ts (1)
75-77: 👍 InjectingsendProgressearly keeps it hoisted and callable
Placing the helper just afterreceiveMessagemeans it is defined before any consumer (e.g.seekTo) executes – good ordering.src/hooks/youtubeIframeScripts.ts (1)
100-129: Early returns clean the flow – but the finalstopProgressTracking()is now unreachableBecause every handled state path now
returns, the unconditionalstopProgressTracking();at line 128 can never run. Either remove it or keep one “catch-all” branch instead of multiple returns to avoid dead code.src/types/youtube.ts (1)
60-71: Docs perfectly mirror the new behaviour – thanks
The clarifications aroundonProgressandseekToremove previous ambiguity. No further action needed.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Refactor