refactor(web): extract YouTubePlayerCore class for better architecture #22
Conversation
- Separate business logic from React lifecycle - Create framework-agnostic core with useYouTubePlayer hook
🦋 Changeset detectedLatest commit: 724aa40 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 |
WalkthroughThe changes introduce a modular refactor of the YouTube player logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant ParentComponent
participant YoutubePlayer (web)
participant useYouTubePlayer
participant YouTubePlayerCore
participant YouTubeIframeAPI
ParentComponent->>YoutubePlayer: Mounts with props
YoutubePlayer->>useYouTubePlayer: Calls hook with config
useYouTubePlayer->>YouTubePlayerCore: Instantiates core with config
YouTubePlayerCore->>YouTubeIframeAPI: Loads API and creates player
YouTubeIframeAPI-->>YouTubePlayerCore: Player ready/events
YouTubePlayerCore-->>useYouTubePlayer: Triggers event callbacks
useYouTubePlayer-->>YoutubePlayer: Provides refs and controls
YoutubePlayer-->>ParentComponent: Exposes controls via ref
ParentComponent->>YoutubePlayer: Calls imperative controls
YoutubePlayer->>useYouTubePlayer: Delegates control calls
useYouTubePlayer->>YouTubePlayerCore: Executes player methods
Possibly related PRs
Poem
✨ 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: |
724aa40
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://56a9eb9d.react-native-youtube-bridge.pages.dev |
| Branch Preview URL: | https://refactor-youtube-player-core.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: 2
🧹 Nitpick comments (1)
src/hooks/useYoutubePlayer.ts (1)
111-139: Improve memoization dependencies for player controls.The
playerControlsobject uses an empty dependency array, but it referencescoreRef.currentwhich could change. While refs don't trigger re-renders, the controls object should be stable once created.Consider adding proper dependencies or using a more explicit approach:
const playerControls = useMemo( (): PlayerControls => ({ play: () => coreRef.current?.play(), pause: () => coreRef.current?.pause(), stop: () => coreRef.current?.stop(), // ... rest of controls }), - [], + [isReady], // Re-create controls when player becomes ready );Alternatively, create the controls object once and rely on the ref pattern for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/orange-snakes-wonder.md(1 hunks)example/src/App.tsx(0 hunks)src/YoutubePlayer.tsx(0 hunks)src/YoutubePlayer.web.tsx(1 hunks)src/hooks/useYoutubePlayer.ts(1 hunks)src/index.tsx(1 hunks)src/modules/YouTubePlayerCore.tsx(1 hunks)src/types/youtube.ts(2 hunks)
💤 Files with no reviewable changes (2)
- src/YoutubePlayer.tsx
- example/src/App.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (15)
.changeset/orange-snakes-wonder.md (1)
1-8: LGTM! Changeset documentation is accurate.The changeset properly documents the patch-level refactoring with a clear description that aligns with the PR objectives.
src/index.tsx (1)
12-12: LGTM! Export addition supports the refactoring goals.Adding the
PlayerEventsexport makes the centralized event callback type available for external usage, supporting the modular architecture improvements.src/types/youtube.ts (3)
36-52: LGTM! Well-designed type centralization.The
PlayerEventstype effectively centralizes all event callback signatures, improving code organization and reusability across the refactored architecture.
76-77: LGTM! Clean composition with backward compatibility.Moving
playerVarsinside the props type and extendingPlayerEventsmaintains the same public API while improving type organization through composition rather than repetition.
79-83: LGTM! Configuration type supports the new architecture.The
YoutubePlayerConfigtype provides a clean interface for configuring the core player class, separating concerns between component props and core player configuration.src/YoutubePlayer.web.tsx (1)
1-28: LGTM! Excellent architectural improvement.The refactored component successfully achieves the separation of concerns by delegating all player logic to the
useYouTubePlayerhook while maintaining the same public API. The implementation is clean, focused, and much more maintainable.src/hooks/useYoutubePlayer.ts (3)
141-146: LGTM! Clean hook interface with proper encapsulation.The hook returns a well-structured interface with container ref, controls, and ready state, providing everything needed for the component to function properly.
116-117: Handle potential void return from seekTo method.The
seekTomethod might returnvoidfrom the core player, but the control interface expects it to be chainable or return a promise.seekTo: (seconds: number, allowSeekAhead?: boolean) => - coreRef.current?.seekTo(seconds, allowSeekAhead) ?? Promise.resolve(), + coreRef.current?.seekTo(seconds, allowSeekAhead) ?? undefined,Or ensure the core player's
seekTomethod consistently returns a promise if that's the intended interface.Likely an incorrect or invalid review comment.
80-88: Let’s pull in the fullcreatePlayerbody plus both hook locations (player init and load effect) to confirm the control flow:#!/bin/bash # 1) Extract createPlayer implementation sed -n '53,120p' src/modules/YouTubePlayerCore.tsx # 2) Show where we call createPlayer in the hook rg -n "coreRef\.current\?\.createPlayer" -C 20 src/hooks/useYoutubePlayer.ts # 3) Show the loadVideoById useEffect in the hook sed -n '75,100p' src/hooks/useYoutubePlayer.tssrc/modules/YouTubePlayerCore.tsx (6)
1-14: LGTM! Clean class structure with proper encapsulation.The imports are well-organized, and the class uses appropriate private fields with good default values. The constructor properly initializes the callback system.
53-149: Excellent player creation logic with comprehensive validation.The method properly validates the environment, container, and video ID before creating the player. The event handler setup is thorough and error handling uses consistent error codes. The cleanup of existing players prevents memory leaks.
151-181: Well-structured state change handling.The method appropriately manages progress tracking for different player states and ensures final progress updates are sent when playback stops.
183-226: Robust progress tracking implementation.The progress tracking logic properly handles async operations, includes appropriate guards, and has good error handling that prevents tracking from continuing if the player becomes unavailable.
228-327: Comprehensive and safe player control methods.Excellent use of optional chaining throughout for safe player access. The async getter methods provide sensible defaults, and the
seekTomethod's delayed progress update enhances user experience.
329-363: Excellent utility methods with comprehensive cleanup.The
destroy()method properly cleans up all resources including intervals, timeouts, and the player instance. ThesetProgressInterval()method correctly handles dynamic interval changes, and callback merging is implemented safely.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/modules/YouTubePlayerCore.tsx (1)
25-44: LGTM! Timeout protection implemented correctly.The timeout protection for API loading has been properly implemented as suggested in the previous review, with appropriate error logging and graceful handling of failure cases.
🧹 Nitpick comments (1)
src/modules/YouTubePlayerCore.tsx (1)
246-256: Consider awaiting the progress update after seeking.The
sendProgress()call in the timeout is not awaited, which could lead to unhandled promise rejections if it fails.this.seekTimeout = setTimeout(() => { - this.sendProgress(); + this.sendProgress().catch(error => + console.error('Error sending progress after seek:', error) + ); }, 200);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/hooks/useYoutubePlayer.ts(1 hunks)src/modules/YouTubePlayerCore.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/useYoutubePlayer.ts
🔇 Additional comments (3)
src/modules/YouTubePlayerCore.tsx (3)
190-208: Robust progress tracking implementation.The progress tracking logic properly handles edge cases with null checks, error handling, and automatic cleanup on failures. The interval management prevents duplicate intervals.
262-322: Excellent async method implementations with safe defaults.All async getter methods properly handle null/undefined cases with sensible default values and use optional chaining for safety. This prevents runtime errors when the player is not available.
354-371: Comprehensive cleanup in destroy method.The destroy method properly cleans up all resources including intervals, timeouts, and the player instance, with appropriate error handling to prevent cleanup failures from breaking the application.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/modules/YouTubePlayerCore.tsx (1)
60-77: Provide feedback on player creation success/failure.The method silently returns on failure conditions without indicating whether the player was successfully created, making it difficult for consumers to handle creation failures appropriately.
This issue was previously identified but remains unaddressed. Consider returning a boolean or throwing errors to indicate success/failure status.
🧹 Nitpick comments (2)
src/modules/YouTubePlayerCore.tsx (2)
16-24: Consider adding protection against race conditions in API loading.While the promise caching mechanism is good, there's a potential race condition if
loadAPI()is called multiple times simultaneously beforewindow._ytApiPromiseis set. The second call might start creating a new promise before the first one finishes setting the global promise.static async loadAPI(): Promise<void> { if (typeof window === 'undefined' || window.YT?.Player) { return Promise.resolve(); } - if (window._ytApiPromise) { - return window._ytApiPromise; - } - - window._ytApiPromise = new Promise<void>((resolve) => { + if (!window._ytApiPromise) { + window._ytApiPromise = new Promise<void>((resolve) => {
65-69: Add validation for container suitability.The method only checks if the container exists but doesn't validate whether it's appropriate for hosting a YouTube player (e.g., if it already contains other content that might interfere).
const container = document.getElementById(containerId); if (!container) { return; } +// Warn if container already has content that might interfere +if (container.children.length > 0) { + console.warn(`Container ${containerId} already contains ${container.children.length} child elements`); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/modules/YouTubePlayerCore.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
src/modules/YouTubePlayerCore.tsx (4)
186-204: Good defensive programming in progress tracking.The progress tracking implementation properly handles edge cases with null checks, error handling, and automatic cleanup on errors. The interval management prevents duplicate timers.
258-318: Excellent fallback handling in async getter methods.All async getter methods consistently provide sensible fallback values when the player is null or methods fail. This defensive approach prevents crashes and provides predictable behavior.
332-344: Robust progress interval management.The
setProgressIntervalmethod properly handles dynamic updates by stopping existing tracking, updating the interval, and restarting if needed. The logic correctly handles the case whereintervalMsis 0 to disable tracking.
350-366: Comprehensive cleanup in destroy method.The destroy method properly cleans up all resources including intervals, timeouts, and the player instance. Error handling for player destruction prevents crashes during cleanup.
Summary by CodeRabbit