feat!: introduce hooks-based API for v2.0 #64
Conversation
BREAKING CHANGE: Complete API redesign with React hooks - Replace YoutubePlayer component with YoutubeView + useYouTubePlayer hook - Add useYouTubeEvent hook for reactive event handling - Remove ref-based imperative API in favor of declarative approach - Simplify component props and reduce coupling between components - Follow expo-audio/expo-video patterns for better DX Migration required from v1: - YoutubePlayer → YoutubeView + useYouTubePlayer() - Event props → useYouTubeEvent() hooks - playerRef.current.method() → player.method() Fixes: Memory leaks, complex state management, tight coupling Improves: Developer experience, maintainability, performance
🦋 Changeset detectedLatest commit: cdf7bdd The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 introduces a major redesign of the YouTube player integration for React Native and web. The imperative, ref-based Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant useYouTubePlayer
participant YoutubePlayer
participant YoutubeView
participant PlayerController (Webview or Web)
participant WebView/DOM
App->>useYouTubePlayer: Call with source/config
useYouTubePlayer->>YoutubePlayer: Create instance (videoId, options)
useYouTubePlayer-->>App: Return YoutubePlayer instance
App->>YoutubeView: Render with player instance
YoutubeView->>PlayerController: Attach player/controller to WebView or DOM
YoutubeView->>WebView/DOM: Render player UI
App->>useYouTubeEvent: Subscribe to player events
YoutubePlayer->>App: Emit events (ready, stateChange, etc.) via hook
App->>YoutubePlayer: Call control methods (play, pause, seek, etc.)
YoutubePlayer->>PlayerController: Forward commands
PlayerController->>WebView/DOM: Execute player actions
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 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-example with
|
| Latest commit: |
cdf7bdd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d2f62828.react-native-youtube-bridge-example.pages.dev |
| Branch Preview URL: | https://feat-v2-hooks-api.react-native-youtube-bridge-example.pages.dev |
Deploying react-native-youtube-bridge with
|
| Latest commit: |
cdf7bdd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://08f6beea.react-native-youtube-bridge.pages.dev |
| Branch Preview URL: | https://feat-v2-hooks-api.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: 13
🔭 Outside diff range comments (1)
packages/core/src/WebYoutubePlayerController.ts (1)
58-58: Fix typo: double window reference.There's a typo with double
windowreference.- window.window.onYouTubeIframeAPIReady = () => { + window.onYouTubeIframeAPIReady = () => {
🧹 Nitpick comments (11)
packages/react-native-youtube-bridge/src/YoutubeView.web.tsx (1)
25-76: Optimize useEffect dependencies to prevent unnecessary re-renders.The
playerobject in the dependency array might cause unnecessary effect re-runs. Consider depending on stable values instead.+ const videoId = player?.getVideoId(); + useEffect(() => { if (!isInitialized || !containerRef.current || !player) { return; } - const videoId = player.getVideoId(); - // ... rest of the effect - }, [isInitialized, player]); + }, [isInitialized, videoId, player]);packages/react/src/hooks/useYouTubeEvent.ts (3)
76-80: Simplify the dependency array spreading.The spread operator on
depsis unnecessarily complex. React's useEffect already handles undefined deps arrays properly.- useEffect(() => { - if (isCallback) { - callbackRef.current = callbackOrThrottleOrDefaultValue; - } - }, [callbackOrThrottleOrDefaultValue, isCallback, ...(deps ?? [])]); + useEffect(() => { + if (isCallback) { + callbackRef.current = callbackOrThrottleOrDefaultValue; + } + }, deps ? [callbackOrThrottleOrDefaultValue, isCallback, ...deps] : [callbackOrThrottleOrDefaultValue, isCallback]);
88-89: Add explanation for the biome-ignore directive.The biome-ignore comment lacks an explanation. Please provide a clear reason why exhaustive dependencies are being ignored here.
- // biome-ignore lint/correctness/useExhaustiveDependencies: <explanation> + // biome-ignore lint/correctness/useExhaustiveDependencies: defaultValue is intentionally excluded to prevent unnecessary re-subscriptions
105-108: Consider removing state reset in cleanup.Resetting state to
defaultValueduring cleanup might cause unnecessary re-renders and could be confusing since the component is unmounting or dependencies are changing.return () => { - setData(defaultValue ?? null); unsubscribe(); };packages/web/src/YoutubePlayer.tsx (1)
29-30: Improve number validation and use consistent defaults.The current validation returns 0 for invalid numbers but undefined for endTime. Consider using a consistent approach and extracting to a utility function.
- const startTimeNumber = Number.isNaN(Number(startTime)) ? 0 : Number(startTime); - const endTimeNumber = Number.isNaN(Number(endTime)) ? undefined : Number(endTime); + const parseTimeParam = (time: string | null, defaultValue?: number): number | undefined => { + if (!time) return defaultValue; + const parsed = Number(time); + return Number.isNaN(parsed) ? defaultValue : parsed; + }; + + const startTimeNumber = parseTimeParam(startTime, 0); + const endTimeNumber = parseTimeParam(endTime);packages/react-native-youtube-bridge/src/YoutubeView.tsx (2)
126-131: Remove redundant cleanup effect.This cleanup effect only sets
playerRef.currentto null, which is redundant since the ref will be garbage collected when the component unmounts.- useEffect(() => { - return () => { - if (playerRef.current) { - playerRef.current = null; - } - }; - }, []);
144-150: Review platform-specific WebView props.Good use of platform-specific props. However, consider if
webviewDebuggingEnabled={__DEV__}should be configurable via props for debugging production issues.Consider exposing
webviewDebuggingEnabledas an optional prop to allow debugging in specific production scenarios when needed.packages/core/src/YoutubePlayer.ts (2)
11-11: Remove commented out code.Please remove the commented out code to keep the codebase clean. If this functionality might be needed later, consider documenting it in a TODO comment or issue instead.
- // private eventStates = new Map<YoutubeEventType, YoutubePlayerEvents[YoutubeEventType]>(); private controller: WebviewYoutubePlayerController | WebYoutubePlayerController | null = null; ... - // getSnapshot<T extends YoutubeEventType>(event: T) { - // return this.eventStates.get(event); - // }Also applies to: 80-82
147-152: Review cleanup order in destroy method.The current cleanup order might cause issues. Consider clearing listeners after destroying the controller to prevent any race conditions with ongoing events.
destroy() { - this.controller?.destroy(); - this.progressInterval = null; this.listeners.clear(); + this.controller?.destroy(); this.controller = null; + this.progressInterval = null; }packages/core/src/WebviewYoutubePlayerController.ts (2)
134-138: Make command timeout configurableThe 5-second timeout is hardcoded. Consider making this configurable to accommodate different network conditions or player states.
+ private static readonly DEFAULT_COMMAND_TIMEOUT = 5000; + private commandTimeout: number; + + constructor(webViewRef: React.RefObject<WebView | null>, options?: { commandTimeout?: number }) { + this.webViewRef = webViewRef; + this.commandTimeout = options?.commandTimeout ?? WebviewYoutubePlayerController.DEFAULT_COMMAND_TIMEOUT; + } // In executeCommand: - const timeout = setTimeout(() => { - this.pendingCommands.delete(messageId); - console.warn('Command timeout:', command, messageId); - resolve(null); - }, 5000); + const timeout = setTimeout(() => { + this.pendingCommands.delete(messageId); + console.warn('Command timeout:', command, messageId); + resolve(null); + }, this.commandTimeout);
164-166: Consider removing or restructuring the empty updateCallbacks methodHaving an empty method with a "no-op only for web" comment suggests a design issue. If this method is only relevant for web implementations, consider:
- Making this class abstract with updateCallbacks as an abstract method
- Using an interface that makes this method optional
- Removing it entirely if it's not needed for the WebView implementation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.changeset/config.json(1 hunks).changeset/funny-moons-hear.md(1 hunks)example/CHANGELOG.md(0 hunks)example/src/App.tsx(7 hunks)packages/core/src/WebYoutubePlayerController.ts(5 hunks)packages/core/src/WebviewYoutubePlayerController.ts(1 hunks)packages/core/src/YoutubePlayer.ts(1 hunks)packages/core/src/YoutubePlayerStore.ts(0 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/types/index.ts(2 hunks)packages/core/src/types/webview.ts(1 hunks)packages/react-native-youtube-bridge/src/YoutubePlayer.tsx(0 hunks)packages/react-native-youtube-bridge/src/YoutubePlayer.web.tsx(0 hunks)packages/react-native-youtube-bridge/src/YoutubeView.tsx(1 hunks)packages/react-native-youtube-bridge/src/YoutubeView.web.tsx(1 hunks)packages/react-native-youtube-bridge/src/YoutubeViewWrapper.tsx(2 hunks)packages/react-native-youtube-bridge/src/hooks/youtubeIframeScripts.ts(3 hunks)packages/react-native-youtube-bridge/src/index.tsx(2 hunks)packages/react-native-youtube-bridge/src/types/youtube.ts(2 hunks)packages/react-native-youtube-bridge/src/utils/youtube.ts(1 hunks)packages/react/src/hooks/index.ts(1 hunks)packages/react/src/hooks/useYouTubeEvent.ts(1 hunks)packages/react/src/hooks/useYoutubePlayer.ts(1 hunks)packages/react/src/index.ts(1 hunks)packages/web/src/YoutubePlayer.tsx(5 hunks)
💤 Files with no reviewable changes (4)
- example/CHANGELOG.md
- packages/react-native-youtube-bridge/src/YoutubePlayer.web.tsx
- packages/react-native-youtube-bridge/src/YoutubePlayer.tsx
- packages/core/src/YoutubePlayerStore.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/core/src/types/index.ts (2)
packages/core/src/index.ts (2)
PlayerState(4-4)EventCallback(13-13)packages/react-native-youtube-bridge/src/index.tsx (1)
PlayerState(5-5)
packages/core/src/types/webview.ts (1)
packages/core/src/types/index.ts (1)
ProgressData(8-16)
packages/react-native-youtube-bridge/src/types/youtube.ts (1)
packages/react-native-youtube-bridge/src/index.tsx (2)
YoutubeViewProps(19-19)YoutubePlayer(13-13)
packages/core/src/YoutubePlayer.ts (2)
packages/core/src/index.ts (6)
INTERNAL_SET_CONTROLLER_INSTANCE(20-20)INTERNAL_UPDATE_PROGRESS_INTERVAL(21-21)INTERNAL_SET_PROGRESS_INTERVAL(22-22)EventCallback(13-13)YoutubePlayerVars(7-7)YoutubePlayerEvents(12-12)packages/core/src/types/index.ts (4)
YoutubeEventType(205-205)EventCallback(207-207)YoutubePlayerVars(100-146)YoutubePlayerEvents(195-203)
packages/core/src/WebviewYoutubePlayerController.ts (1)
packages/core/src/types/index.ts (1)
PlayerEvents(57-94)
packages/web/src/YoutubePlayer.tsx (2)
packages/web/src/hooks/useWebView.ts (1)
useWebView(12-70)packages/react/src/index.ts (1)
useYouTubeVideoId(1-1)
🔇 Additional comments (22)
.changeset/config.json (1)
10-10: LGTM! Sensible configuration update.Adding "example" to the ignore array is appropriate since example app changes shouldn't trigger version bumps for the actual packages.
packages/react/src/hooks/index.ts (1)
4-4: LGTM! Consistent export pattern.The new
useYouTubeEventexport follows the same pattern as other hooks in this module and supports the new hooks-based API design.packages/react/src/index.ts (1)
1-1: LGTM! Proper API exposure.Adding
useYouTubeEventto the exports correctly exposes the new hook as part of the public API for the hooks-based redesign.packages/react-native-youtube-bridge/src/utils/youtube.ts (1)
10-10: LGTM! Good defensive programming.Adding the
!videoIdguard prevents the function from generating malformed URLs when the videoId is falsy, improving the robustness of the URL generation logic.packages/core/src/types/index.ts (2)
197-197: LGTM! Improved naming consistency.Changing
statechangetostateChangefollows camelCase conventions and improves consistency with the rest of the codebase.
207-207: LGTM! Well-defined generic type.The new
EventCallback<Data = any>type is cleanly defined and will support the new event subscription mechanism in the hooks-based API.packages/core/src/types/webview.ts (1)
18-21: LGTM! Improved type structure.The refactoring from extending
ProgressDatadirectly to encapsulating it within aprogressproperty improves the message structure organization and aligns with the updated iframe scripts that now nest progress data.packages/react-native-youtube-bridge/src/YoutubeViewWrapper.tsx (3)
4-9: LGTM! Consistent naming with the new API.The renaming from
YoutubePlayerWrapperPropstoYoutubeViewWrapperPropsaligns with the overall migration fromYoutubePlayertoYoutubeViewcomponents.
11-15: LGTM! Proper component renaming.The function renaming from
YoutubePlayerWrappertoYoutubeViewWrappermaintains the same functionality while aligning with the new component naming conventions.
24-24: LGTM! Consistent export naming.The export update completes the renaming from
YoutubePlayerWrappertoYoutubeViewWrapper..changeset/funny-moons-hear.md (1)
1-26: LGTM! Comprehensive changeset documentation.The changeset properly documents the major breaking changes and provides clear migration guidance from v1 to v2. The content accurately reflects:
- The shift from
YoutubePlayercomponent toYoutubeView+useYouTubePlayerhook- Introduction of
useYouTubeEventfor reactive event handling- Migration steps for users upgrading from v1
- Benefits of the new architecture
packages/react-native-youtube-bridge/src/hooks/youtubeIframeScripts.ts (3)
25-31: LGTM! Improved progress data structure.The restructuring to nest progress properties within a
progressobject aligns with the updatedProgressMessageDatainterface and improves message organization.
60-66: LGTM! Consistent progress data structure.The same progress data nesting applied in
sendProgressmaintains consistency with thestartProgressTrackingfunction and the type definitions.
79-97: LGTM! Explicit playerInfo construction.The explicit construction of the
playerInfoobject provides better control over which properties are exposed and aligns with type safety principles. This is an improvement over forwarding the entireevent.target.playerInfoobject.packages/core/src/index.ts (3)
12-14: LGTM! New event-related type exports.The addition of
YoutubePlayerEventsandEventCallbacktypes supports the new hooks-based event handling system introduced in the v2 API.
16-17: LGTM! Controller-based architecture exports.The separate exports for
WebYoutubePlayerControllerandWebviewYoutubePlayerControllerproperly reflect the new modular architecture that separates web and React Native WebView implementations.
18-23: LGTM! Unified player interface with controlled access.The export of the
YoutubePlayerclass along with internal symbols provides a unified interface while maintaining controlled access to internal functionality. This supports the new hooks-based API design.packages/react-native-youtube-bridge/src/types/youtube.ts (1)
1-1: Type definitions align well with the new hooks-based architecture.The addition of
YoutubePlayerimport and the newYoutubeViewPropstype properly support the architectural shift to a hooks-based API. The separation of player management (via hooks) from the view component (via props) is a clean design pattern.Also applies to: 70-82
packages/react-native-youtube-bridge/src/index.tsx (1)
1-20: Export changes properly implement the new hooks-based API.The module exports have been cleanly updated to reflect the architectural changes, providing all necessary components, hooks, and types for the new API.
packages/react-native-youtube-bridge/src/YoutubeView.tsx (1)
54-61: Add null checks for pending commands.The
getPendingCommands()method might return undefined, but the code assumes it returns a Map.if (data.type === 'commandResult') { const pendingCommands = playerRef.current?.getPendingCommands(); - - const resolver = pendingCommands?.get(data.id); - - if (resolver) { - resolver(data.result); - pendingCommands?.delete(data.id); - } + if (pendingCommands && data.id) { + const resolver = pendingCommands.get(data.id); + if (resolver) { + resolver(data.result); + pendingCommands.delete(data.id); + } + } return; }Likely an incorrect or invalid review comment.
example/src/App.tsx (1)
1-313: Excellent migration to the new hooks-based API!The example effectively demonstrates the new API patterns:
- Clean separation of player instance creation with
useYouTubePlayer- Declarative event handling with
useYouTubeEvent- Simplified player control without refs
This serves as a great reference for users migrating from v1.
packages/core/src/WebviewYoutubePlayerController.ts (1)
28-119: Well-structured player control methodsThe player control methods are well-organized and follow a consistent pattern. The comprehensive API coverage for YouTube player functionality is excellent.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
packages/react-native-youtube-bridge/src/modules/YoutubePlayer.ts (2)
18-18: Remove commented out code.The commented lines for
eventStatesandgetSnapshotmethod appear to be leftover from development and should be removed to maintain code cleanliness.Also applies to: 87-89
91-153: Consider adding null safety for controller methods.All the playback control methods return the result of optional chaining on the controller (e.g.,
this.controller?.play()), which returnsundefinedwhen controller is null. Consider either:
- Throwing an error when controller is not initialized
- Documenting that these methods may return undefined
- Adding a guard method to check controller availability
Example implementation with error handling:
play() { - return this.controller?.play(); + if (!this.controller) { + throw new Error('Player not initialized'); + } + return this.controller.play(); }packages/react-native-youtube-bridge/src/hooks/useYouTubePlayer.ts (2)
28-28: Add explanation for biome-ignore directive.The comment
// biome-ignore lint/correctness/useExhaustiveDependencies: only oncedoesn't clearly explain why the dependencies are incomplete.- // biome-ignore lint/correctness/useExhaustiveDependencies: only once + // biome-ignore lint/correctness/useExhaustiveDependencies: config is intentionally stringified to detect deep changes
40-41: Simplify fast refresh detection logic.The
isFastRefreshlogic seems complex and might be prone to race conditions. Consider using a more robust approach or documenting why this pattern is necessary.Also applies to: 46-46, 49-50
packages/react-native-youtube-bridge/src/hooks/useYouTubeEvent.ts (2)
86-86: Add explanation for biome-ignore directive.Missing explanation for why exhaustive dependencies are being ignored.
- // biome-ignore lint/correctness/useExhaustiveDependencies: <explanation> + // biome-ignore lint/correctness/useExhaustiveDependencies: defaultValue is intentionally excluded to prevent infinite loops
14-44: Consider simplifying the function overloads.The multiple overloads make the API complex. Consider splitting into separate hooks for clarity:
useYouTubeEventfor callback-based subscriptionsuseYouTubeEventStatefor state-based subscriptionsuseYouTubeProgressfor progress events with throttlingpackages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts (1)
136-136: Replace console.warn with configurable logging.Using
console.warndirectly in production code isn't ideal. Consider using a logging abstraction.- console.warn('Command timeout:', command, messageId); + if (__DEV__) { + console.warn('Command timeout:', command, messageId); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/core/src/index.ts(1 hunks)packages/core/src/types/index.ts(1 hunks)packages/react-native-youtube-bridge/src/YoutubeView.tsx(1 hunks)packages/react-native-youtube-bridge/src/YoutubeView.web.tsx(1 hunks)packages/react-native-youtube-bridge/src/hooks/useCreateLocalPlayerHtml.ts(1 hunks)packages/react-native-youtube-bridge/src/hooks/useYouTubeEvent.ts(1 hunks)packages/react-native-youtube-bridge/src/hooks/useYouTubePlayer.ts(1 hunks)packages/react-native-youtube-bridge/src/index.tsx(2 hunks)packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts(1 hunks)packages/react-native-youtube-bridge/src/modules/YoutubePlayer.ts(1 hunks)packages/react-native-youtube-bridge/src/types/youtube.ts(2 hunks)packages/react/src/hooks/index.ts(0 hunks)packages/react/src/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react/src/hooks/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react-native-youtube-bridge/src/hooks/useCreateLocalPlayerHtml.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/core/src/index.ts
- packages/react-native-youtube-bridge/src/YoutubeView.web.tsx
- packages/react-native-youtube-bridge/src/types/youtube.ts
- packages/react-native-youtube-bridge/src/index.tsx
- packages/react-native-youtube-bridge/src/YoutubeView.tsx
- packages/core/src/types/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/react-native-youtube-bridge/src/hooks/useYouTubePlayer.ts (3)
packages/core/src/types/index.ts (3)
YouTubeSource(6-6)YoutubePlayerVars(100-146)YouTubeError(18-25)packages/react-native-youtube-bridge/src/index.tsx (2)
YoutubePlayerVars(6-6)YouTubeError(9-9)packages/react/src/index.ts (1)
useYouTubeVideoId(1-1)
packages/react-native-youtube-bridge/src/hooks/useYouTubeEvent.ts (3)
packages/core/src/index.ts (2)
YoutubePlayerEvents(12-12)EventCallback(13-13)packages/core/src/types/index.ts (2)
YoutubePlayerEvents(195-203)EventCallback(205-205)packages/react-native-youtube-bridge/src/modules/YoutubePlayer.ts (2)
INTERNAL_SET_PROGRESS_INTERVAL(12-12)INTERNAL_SET_PROGRESS_INTERVAL(55-60)
packages/react-native-youtube-bridge/src/modules/YoutubePlayer.ts (2)
packages/core/src/types/index.ts (3)
YoutubePlayerEvents(195-203)EventCallback(205-205)YoutubePlayerVars(100-146)packages/react-native-youtube-bridge/src/index.tsx (1)
YoutubePlayerVars(6-6)
🔇 Additional comments (4)
packages/react/src/index.ts (1)
1-1: LGTM! Export change aligns with the architectural redesign.The removal of
useYouTubePlayerfrom this package is consistent with the v2.0 API redesign where YouTube player functionality is consolidated into the react-native-youtube-bridge package.packages/react-native-youtube-bridge/src/modules/YoutubePlayer.ts (1)
49-53: Good use of symbols for internal methods.Using symbols for internal methods (
INTERNAL_SET_CONTROLLER_INSTANCE) prevents external access and provides better encapsulation.packages/react-native-youtube-bridge/src/hooks/useYouTubeEvent.ts (1)
78-78: Fix dependency array spreading issue.Spreading
depsarray inside the dependency array creates a nested structure that won't work as intended for dependency comparison.- }, [callbackOrThrottleOrDefaultValue, isCallback, ...(deps ?? [])]); + }, [callbackOrThrottleOrDefaultValue, isCallback, ...(deps ?? [])].flat());However, this pattern is unusual. Consider restructuring to avoid this complexity.
Likely an incorrect or invalid review comment.
packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts (1)
152-156: JSON.stringify Escapes Data, and Commands Are Whitelisted
The injected script embeds a JSON object literal—JSON.stringifyescapes quotes, control characters, and backslashes—so there’s no way for untrusted input to break out of the literal. On top of that, in the iframe context we only callwindow.playerCommands[command]if it’s a known function, so arbitrary code cannot be executed.No further validation is required here. If you plan to expose new commands in the future, you can maintain an explicit whitelist of allowed command names.
packages/react-native-youtube-bridge/src/hooks/useYouTubePlayer.ts
Outdated
Show resolved
Hide resolved
packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts
Outdated
Show resolved
Hide resolved
packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts (2)
14-22: Singleton pattern issues remain unaddressed.The singleton pattern with mutable
webViewRefstill causes the same issues identified in previous reviews. Multiple YouTube players will conflict as each overwrites the shared reference.
168-173: Race condition in destroy method persists.The previous review comment about race conditions in the destroy method remains valid. Setting the singleton instance to null without synchronization could cause issues.
🧹 Nitpick comments (5)
packages/react-native-youtube-bridge/src/hooks/useYouTubeEvent.ts (2)
59-66: Complex parameter parsing logic is correct but could be simplified.The logic correctly handles different parameter types, but the nested ternary operators make it hard to follow. Consider extracting this into helper functions for better readability.
- const isProgress = eventType === 'progress'; - const isCallback = typeof callbackOrThrottleOrDefaultValue === 'function'; - const throttleMs = isProgress - ? typeof callbackOrThrottleOrDefaultValue === 'number' - ? callbackOrThrottleOrDefaultValue - : DEFAULT_PROGRESS_INTERVAL - : undefined; - const defaultValue = isCallback || isProgress ? null : (callbackOrThrottleOrDefaultValue ?? null); + const isProgress = eventType === 'progress'; + const isCallback = typeof callbackOrThrottleOrDefaultValue === 'function'; + + const getThrottleMs = (): number | undefined => { + if (!isProgress) return undefined; + return typeof callbackOrThrottleOrDefaultValue === 'number' + ? callbackOrThrottleOrDefaultValue + : DEFAULT_PROGRESS_INTERVAL; + }; + + const getDefaultValue = () => { + if (isCallback || isProgress) return null; + return callbackOrThrottleOrDefaultValue ?? null; + }; + + const throttleMs = getThrottleMs(); + const defaultValue = getDefaultValue();
74-78: Dependency array handling is correct but consider edge case.The spread of
deps ?? []is handled properly, but the effect could run unnecessarily ifcallbackOrThrottleOrDefaultValuechanges for non-callback usage.useEffect(() => { if (isCallback) { callbackRef.current = callbackOrThrottleOrDefaultValue; } - }, [callbackOrThrottleOrDefaultValue, isCallback, ...(deps ?? [])]); + }, [isCallback ? callbackOrThrottleOrDefaultValue : null, isCallback, ...(deps ?? [])]);packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts (3)
134-138: Command timeout handling could be more informative.The timeout warning only includes command name and message ID. Adding the arguments would help with debugging complex command failures.
const timeout = setTimeout(() => { this.pendingCommands.delete(messageId); - console.warn('Command timeout:', command, messageId); + console.warn(`Command timeout: ${command}(${JSON.stringify(args)}) [id: ${messageId}]`); resolve(null); }, 5000);
152-156: JavaScript injection is secure but consider error handling.The injected script properly checks for
window.__execCommandexistence and usesJSON.stringifyfor safe serialization. However, consider handling potential injection failures.- this.webViewRef.current.injectJavaScript(injectScript); + try { + this.webViewRef.current.injectJavaScript(injectScript); + } catch (error) { + console.error(`Failed to inject command '${command}':`, error); + if (needsResult && messageId) { + this.pendingCommands.delete(messageId); + resolve(null); + } + }
164-166: Consider documenting the web compatibility pattern.The no-op
updateCallbacksmethod with the comment "only for web" suggests interface compatibility. This pattern could be confusing without proper documentation.+ /** + * Updates player event callbacks. No-op in WebView implementation. + * This method exists for interface compatibility with web implementation. + * @param _newCallbacks - Event callbacks (ignored in WebView) + */ updateCallbacks(_newCallbacks: Partial<PlayerEvents>): void { // no-op only for web }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/react-native-youtube-bridge/src/YoutubeView.web.tsx(1 hunks)packages/react-native-youtube-bridge/src/hooks/useYouTubeEvent.ts(1 hunks)packages/react-native-youtube-bridge/src/hooks/useYouTubePlayer.ts(1 hunks)packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts(1 hunks)packages/react-native-youtube-bridge/src/modules/YoutubePlayer.ts(1 hunks)packages/web/src/YoutubePlayer.tsx(5 hunks)packages/web/src/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/react-native-youtube-bridge/src/modules/YoutubePlayer.ts
- packages/react-native-youtube-bridge/src/YoutubeView.web.tsx
- packages/web/src/YoutubePlayer.tsx
- packages/react-native-youtube-bridge/src/hooks/useYouTubePlayer.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/react-native-youtube-bridge/src/hooks/useYouTubeEvent.ts (3)
packages/core/src/index.ts (2)
YoutubePlayerEvents(12-12)EventCallback(13-13)packages/core/src/types/index.ts (2)
YoutubePlayerEvents(195-203)EventCallback(205-205)packages/react-native-youtube-bridge/src/modules/YoutubePlayer.ts (2)
INTERNAL_SET_PROGRESS_INTERVAL(12-12)INTERNAL_SET_PROGRESS_INTERVAL(55-60)
🔇 Additional comments (3)
packages/web/src/utils.ts (1)
1-8: LGTM! Clean utility function with appropriate error handling.The implementation correctly handles null/falsy inputs and NaN values. The flexible return type allowing
undefinedwhen no default is provided gives callers good control over fallback behavior.Minor consideration: The function will accept and return
Infinityor-Infinityfor inputs like"Infinity"sinceNumber.isNaN(Infinity)returnsfalse. Depending on how this is used with YouTube player time parameters, you may want to add additional validation if infinite values aren't acceptable.packages/react-native-youtube-bridge/src/hooks/useYouTubeEvent.ts (1)
14-44: Well-designed function overloads for different usage patterns.The overloads provide clear separation between:
- State-based subscription (excluding progress events)
- Callback-based subscription (all events)
- Progress-specific subscription with throttling
The type constraints and return types are correctly defined for each pattern.
packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts (1)
120-162: Command execution logic is well-implemented with proper timeout handling.The
executeCommandmethod correctly:
- Handles synchronous commands (no result needed)
- Manages asynchronous commands with unique message IDs
- Implements 5-second timeout with cleanup
- Properly handles WebView availability checks
The promise-based approach with timeout management prevents memory leaks from hanging promises.
packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts
Show resolved
Hide resolved
- Replace getInstance() with createInstance() factory method - Remove static instance property and related cleanup - Each YoutubePlayer now gets its own controller instance - Fixes WebView reference conflicts when multiple players exist
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts (1)
118-122: Improve error handling for missing WebView reference.The method silently resolves with
nullwhen WebView is unavailable, which could mask real issues. Consider logging or throwing an error to help with debugging.return new Promise((resolve) => { if (!this.webViewRef.current) { + console.warn(`Cannot execute command '${command}': WebView not available`); resolve(null); return; }
🧹 Nitpick comments (2)
packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts (2)
37-39: Add input validation for volume parameter.The YouTube API expects volume values between 0-100. Consider adding validation to prevent invalid values from being sent to the WebView.
async setVolume(volume: number): Promise<void> { + if (volume < 0 || volume > 100) { + throw new Error('Volume must be between 0 and 100'); + } await this.executeCommand('setVolume', [volume]); }
145-149: Add error handling for JavaScript injection.The
injectJavaScriptcall could potentially fail, but there's no error handling. Consider wrapping it in a try-catch block to handle injection failures gracefully.- this.webViewRef.current.injectJavaScript(injectScript); + try { + this.webViewRef.current.injectJavaScript(injectScript); + } catch (error) { + console.error('Failed to inject JavaScript:', error); + if (needsResult && messageId) { + this.pendingCommands.delete(messageId); + } + resolve(null); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/src/WebYoutubePlayerController.ts(6 hunks)packages/react-native-youtube-bridge/src/YoutubeView.tsx(1 hunks)packages/react-native-youtube-bridge/src/YoutubeView.web.tsx(1 hunks)packages/react-native-youtube-bridge/src/hooks/useYouTubeEvent.ts(1 hunks)packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts(1 hunks)packages/web/src/YoutubePlayer.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/web/src/YoutubePlayer.tsx
- packages/react-native-youtube-bridge/src/YoutubeView.web.tsx
- packages/core/src/WebYoutubePlayerController.ts
- packages/react-native-youtube-bridge/src/YoutubeView.tsx
- packages/react-native-youtube-bridge/src/hooks/useYouTubeEvent.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts (1)
packages/core/src/types/index.ts (1)
PlayerEvents(57-94)
🔇 Additional comments (3)
packages/react-native-youtube-bridge/src/modules/WebviewYoutubePlayerController.ts (3)
4-15: LGTM! Clean class structure with proper encapsulation.The class properly encapsulates its state with private fields and provides a clear factory method. The pending commands Map is well-typed for handling asynchronous command results.
157-164: LGTM! Proper interface compatibility pattern.The no-op
updateCallbacksmethod is a clean way to maintain interface compatibility between web and React Native implementations while clearly documenting its purpose.
166-169: LGTM! Proper resource cleanup.The
destroymethod correctly clears pending commands and calls cleanup. Unlike previous implementations, this doesn't suffer from singleton race conditions since it's a regular class instance.
BREAKING CHANGE: Complete API redesign with React hooks
YoutubePlayercomponent withYoutubeView+useYouTubePlayerhookuseYouTubeEventhook for reactive event handlingMigration required from v1:
YoutubePlayer→YoutubeView+useYouTubePlayer()useYouTubeEvent()hooksplayerRef.current.method()→player.method()Fixes: Memory leaks, complex state management, tight coupling
Improves: Developer experience, maintainability, performance
Summary by CodeRabbit
New Features
YoutubeViewcomponent and new hooks (useYouTubePlayer,useYouTubeEvent) for more declarative and flexible YouTube player integration.Bug Fixes
Refactor
YoutubePlayercomponent withYoutubeViewand removed imperative ref-based APIs.Chores