feat: support youtube iframe for web #1
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a web implementation of the YouTube player component, add supporting types for the YouTube IFrame API, and refactor the player wrapper for consistent styling. The Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant YoutubePlayerWrapper
participant YoutubePlayer (.tsx/.web.tsx)
participant YouTube IFrame API
App->>YoutubePlayerWrapper: Render with width, height, style
YoutubePlayerWrapper->>YoutubePlayer: Render as child
YoutubePlayer->>YouTube IFrame API: Initialize player (web) or native logic (native)
App->>YoutubePlayer: Pass event handlers (memoized with useCallback)
YouTube IFrame API-->>YoutubePlayer: Player events (ready, stateChange, error, etc.)
YoutubePlayer-->>App: Invoke corresponding callbacks
App->>YoutubePlayer: Use imperative controls via ref
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (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 (
|
saseungmin
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/hooks/useCreateLocalPlayerHtml.ts (1)
24-27:⚠️ Potential issueUndefined
originbecomes literal'undefined'in iframe URL.When
originis falsy,escapeHtml(origin)returns the string'undefined', triggering the ternarysafeOrigin ? …branch and injecting an invalidoriginparam.-const safeOrigin = escapeHtml(origin); +const safeOrigin = origin ? escapeHtml(origin) : '';Then keep the existing ternary.
🧹 Nitpick comments (5)
biome.json (1)
9-12: Broader include-glob may slow linting & surface noise.
"src/**/*"matches every file (images, JSON, build artefacts, etc.).
Consider narrowing the glob (e.g.src/**/*.{js,jsx,ts,tsx}) or adding an"exclude"for asset folders to avoid needless lint/format passes and spurious warnings.src/YoutubePlayerWrapper.tsx (1)
10-12: Prop-driven size can be unintentionally overridden bystyleprop.
styleis placed after{ width, height }in the array, so callers can silently overwrite the explicitwidth/heightyou expose. If the wrapper’s contract is “width/heightwin”, invert the order:-return <View style={[styles.container, { width, height }, style]}>{children}</View>; +return <View style={[styles.container, style, { width, height }]}>{children}</View>;src/types/iframe.d.ts (1)
162-164: Constructor overload misses HTMLElement signature.YouTube’s IFrame API allows passing an
HTMLElement. Add an overload to prevent false-errors when callers already have the element reference.-Player: { new (elementId: string, options: Options): YouTubePlayer }; +Player: { + new (elementId: string, options: Options): YouTubePlayer; + new (element: HTMLElement, options: Options): YouTubePlayer; +};src/YoutubePlayer.tsx (2)
107-108: Consider using a generic type instead ofany.The
biome-ignorecomment suppresses theanytype warning, but a generic type would provide better type safety.-// biome-ignore lint/suspicious/noExplicitAny: <explanation> -(command: string, args: (string | number | boolean | undefined)[] = [], needsResult = false): Promise<any> => { +<T = unknown>(command: string, args: (string | number | boolean | undefined)[] = [], needsResult = false): Promise<T | null> => {
193-193: Translate comment to English for consistency.The Korean comment should be in English to maintain consistency across the codebase.
-// 모든 pending commands 정리 +// Clear all pending commands
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
biome.json(2 hunks)example/src/App.tsx(4 hunks)src/YoutubePlayer.tsx(3 hunks)src/YoutubePlayer.web.tsx(1 hunks)src/YoutubePlayerWrapper.tsx(1 hunks)src/hooks/useCreateLocalPlayerHtml.ts(2 hunks)src/types/iframe.d.ts(1 hunks)src/types/youtube.ts(0 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/types/youtube.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
example/src/App.tsx (1)
src/types/youtube.ts (2)
ProgressData(56-61)YouTubeError(41-44)
src/YoutubePlayer.web.tsx (3)
src/types/youtube.ts (3)
PlayerControls(63-98)YoutubePlayerProps(16-30)ERROR_CODES(46-52)src/types/iframe.d.ts (1)
YouTubePlayer(64-160)src/index.tsx (1)
PlayerState(3-3)
🔇 Additional comments (8)
tsconfig.json (1)
9-9: DOM libs addition is appropriate – no further action.Adding
"DOM"and"DOM.Iterable"tolibis necessary for the new web-specific code that references browser globals. Nothing else in the file is affected – good change.biome.json (1)
23-26: Rule set change looks safe but may flood CI with warnings.Turning on
suspicious.noExplicitAnyas warn is fine if your CI treats warnings non-fatally. Verify pipelines won’t become noisy.src/hooks/useCreateLocalPlayerHtml.ts (1)
94-104:muteplayerVar is unofficial – confirm necessity.
playerVarsdoesn’t documentmute; muting is normally done afteronReady. If relying on unofficial query params, ensure they remain supported or move muting logic toonReady.src/YoutubePlayer.tsx (1)
46-104: LGTM! Well-structured message handling.The message handler properly handles all event types with appropriate error handling and logging.
example/src/App.tsx (1)
23-91: Good optimization with useCallback hooks!The memoization of event handlers prevents unnecessary re-renders of the YoutubePlayer component. The empty dependency arrays are correct since these callbacks only use stable references (refs, state setters, and global functions).
src/YoutubePlayer.web.tsx (3)
49-77: Well-implemented progress tracking!The progress tracking properly handles errors and cleans up intervals. The 1-second interval provides a good balance between accuracy and performance.
239-309: Excellent null handling in async methods!All async control methods properly handle cases where the player might not be initialized, returning sensible defaults using nullish coalescing.
116-178: 🛠️ Refactor suggestionWrap player creation logic in useCallback.
Direct assignment to
createPlayerRef.currentoutside of hooks could cause issues in React's concurrent mode.Replace the direct assignment with:
-createPlayerRef.current = () => { +const createPlayerFunction = useCallback(() => { if (!containerRef.current || !window.YT?.Player || !videoId) { return; } // ... rest of the function -}; +}, [videoId, autoplay, controls, loop, startTime, endTime, playsinline, rel, onReady, onStateChange, onError, onPlaybackQualityChange, onPlaybackRateChange, onAutoplayBlocked, startProgressTracking, stopProgressTracking]); + +createPlayerRef.current = createPlayerFunction;Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/types/iframe.d.ts (1)
1-2: Remove the unusedYouTubeErrorimport.
YouTubeErroris imported but never referenced in this declaration file, which will triggertsc --noUnusedLocalsor ESLint errors.-import type { ERROR_CODES, PlaybackQuality, PlayerState, YouTubeError } from './youtube'; +import type { ERROR_CODES, PlaybackQuality, PlayerState } from './youtube';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/YoutubePlayer.web.tsx(1 hunks)src/YoutubePlayerWrapper.tsx(1 hunks)src/types/iframe.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/YoutubePlayerWrapper.tsx
- src/YoutubePlayer.web.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/types/iframe.d.ts (2)
150-150: Return a namedPlayerSizeinstead of an inline object.Re-introduce the
PlayerSizeinterface (width / height) for reuse and clearer IDE hints.+export interface PlayerSize { + width: number; + height: number; +} ... - setSize(width: number, height: number): Promise<{ width: number; height: number }>; + setSize(width: number, height: number): Promise<PlayerSize>;
30-32:⚠️ Potential issueMissing
2value forplayerVars.controls.The IFrame API allows
controls: 0 | 1 | 2; omitting2causes false-positive type errors for the “chromeless” UI.- controls?: 0 | 1 | undefined; + controls?: 0 | 1 | 2 | undefined;
🧹 Nitpick comments (1)
src/types/iframe.d.ts (1)
1-1: Remove unusedYouTubeErrorimport or leverage it inonError.
YouTubeErroris imported but never referenced, which will triggerno-unused-vars/unused-importslint rules.
Either (a) drop the identifier, or (b) switch theonErrorcallback to the richer type:-import type { ERROR_CODES, PlaybackQuality, PlayerState, YouTubeError } from './youtube'; +import type { ERROR_CODES, PlaybackQuality, PlayerState } from './youtube';or
- onError?: (event: { data: keyof typeof ERROR_CODES }) => void; + onError?: (event: { data: YouTubeError }) => void;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/types/iframe.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/types/iframe.d.ts (2)
src/index.tsx (1)
PlayerState(3-3)src/types/youtube.ts (2)
PlaybackQuality(54-54)ERROR_CODES(46-52)
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
modestbrandingoption from player settings to align with updated requirements.Chores
anytypes.