feat: expose player methods and events in ReactNative - #1
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a React Native player component with an imperative API for playback control and state queries, bridging native Android functionality to JavaScript. The native view and its manager expose new methods and events for play, pause, seek, playback speed, and state queries, while the JavaScript layer handles event-driven promise resolution and command dispatching. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as TPStreamsPlayer (JS)
participant NativeView as TPStreamsRNPlayerView (Android)
participant Manager as TPStreamsRNPlayerViewManager (Android)
JS->>NativeView: play() / pause() / seekTo() / setPlaybackSpeed()
JS->>NativeView: getCurrentPosition() / getDuration() / isPlaying() / getPlaybackSpeed()
NativeView-->>JS: Emits event (onCurrentPosition, onDuration, etc.) with value
JS->>JS: Resolves corresponding Promise via event handler
Poem
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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/TPStreamsPlayer.tsx (2)
41-50: Remove redundant null check for cleaner code.The event handler performs a redundant null check after retrieving the handler from the promise map.
const onCurrentPosition = useCallback((event: any) => { const key = `position-${instanceId.current}`; - if (promiseMap.current[key]) { - const handler = promiseMap.current[key]; - if (handler) { - handler.resolve(event.nativeEvent.position); - delete promiseMap.current[key]; - } - } + const handler = promiseMap.current[key]; + if (handler) { + handler.resolve(event.nativeEvent.position); + delete promiseMap.current[key]; + } }, []);This pattern should be applied to all similar event handlers (lines 52-83).
8-8: Consider potential overflow of global instance counter.While unlikely in practice, the global
nextInstanceIdcounter could theoretically overflow in long-running applications with many component instances.Consider using a more robust ID generation:
-let nextInstanceId = 0; +import { nanoid } from 'nanoid'; // or use crypto.randomUUID() if available -const instanceId = useRef<number>(nextInstanceId++); +const instanceId = useRef<string>(nanoid());This would also require updating the key generation in event handlers to use string concatenation instead of template literals with numbers.
android/src/main/java/com/tpstreams/TPStreamsRNPlayerViewManager.kt (1)
61-63: Potential precision loss in seekTo conversion.Converting
DoubletoLongforpositionMscould lose precision, especially for fractional millisecond values.Consider using
toLong()with proper rounding:override fun seekTo(view: TPStreamsRNPlayerView, positionMs: Double) { - view.seekTo(positionMs.toLong()) + view.seekTo(positionMs.roundToLong()) }Alternatively, change the interface to accept
Longdirectly if fractional precision isn't needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
android/src/main/java/com/tpstreams/TPStreamsRNPlayerView.kt(2 hunks)android/src/main/java/com/tpstreams/TPStreamsRNPlayerViewManager.kt(3 hunks)package.json(1 hunks)src/TPStreamsPlayer.tsx(1 hunks)src/TPStreamsPlayerViewNativeComponent.ts(1 hunks)src/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/TPStreamsPlayer.tsx (2)
src/index.tsx (1)
TPStreamsPlayerRef(6-6)src/TPStreamsPlayerViewNativeComponent.ts (2)
NativeProps(8-17)Commands(40-51)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-android
- GitHub Check: build-ios
🔇 Additional comments (12)
package.json (1)
3-3: LGTM! Appropriate version bump for new feature.The version increment from 0.1.0 to 0.2.0 correctly follows semantic versioning for the addition of new player methods functionality.
src/index.tsx (1)
5-6: LGTM! Clean export additions.The new exports for
TPStreamsPlayerandTPStreamsPlayerReffollow the existing patterns and properly expose the new player component functionality.src/TPStreamsPlayer.tsx (1)
181-191: LGTM! Clean component composition and rendering.The component properly merges event handlers with incoming props and renders the native component with the correct ref. The pattern follows React best practices.
src/TPStreamsPlayerViewNativeComponent.ts (4)
3-6: LGTM! Proper imports for React Native codegen.The additional imports for
Double,Float,DirectEventHandler, andcodegenNativeCommandsare correctly added to support the new event handlers and commands functionality.
8-17: LGTM! Well-typed event handler props.The event handler props are properly typed with
DirectEventHandlerand include the correct payload types (Double,Float,boolean) that align with the native implementation expectations.
19-38: LGTM! Comprehensive command interface definition.The
TPStreamsPlayerViewCommandsinterface properly defines all player control methods with correct parameter types and follows React Native patterns for native command interfaces.
40-51: LGTM! Proper command registration with codegen.The
Commandsexport correctly usescodegenNativeCommandswith all supported commands listed, enabling the bridge between JavaScript and native implementations.android/src/main/java/com/tpstreams/TPStreamsRNPlayerView.kt (2)
5-9: LGTM! Proper imports and context storage for event emission.The additional imports and ReactContext storage are correctly implemented for the event emission functionality.
Also applies to: 17-17
49-63: LGTM! Clean player control delegation.The player control methods follow a proper null-safe delegation pattern to the underlying player instance.
android/src/main/java/com/tpstreams/TPStreamsRNPlayerViewManager.kt (3)
11-11: LGTM! Proper event registration for React Native.The custom direct event types are correctly registered using MapBuilder, enabling proper event communication between native and JavaScript layers.
Also applies to: 30-37
53-67: LGTM! Clean delegation pattern for control methods.The player control methods properly delegate to the view instance with appropriate type handling.
69-83:Details
✅ Verification successful
Consider the event-driven architecture implications.
The getter methods in the manager don't return values but trigger event emission through the view. This is unusual for getter methods but appears intentional for the event-driven architecture.
Verify that this event-driven approach aligns with the JavaScript implementation and that the calling code doesn't expect synchronous return values from these methods.
🏁 Script executed:
#!/bin/bash # Description: Check how these getter methods are used in the JavaScript layer # Expected: JavaScript code should use event listeners rather than return values rg -A 10 -B 5 "getCurrentPosition|getDuration|isPlaying|getPlaybackSpeed" --type ts --type tsx --type js --type jsxLength of output: 145
🏁 Script executed:
#!/bin/bash # Re-run search across JS/TS files without relying on rg’s built-in types rg -g '*.js' -g '*.jsx' -g '*.ts' -g '*.tsx' -e "getCurrentPosition" -e "getDuration" -e "isPlaying" -e "getPlaybackSpeed" -A 5 -B 5Length of output: 8632
Event-driven getters align with JS promise-based API
The native
getCurrentPosition,getDuration,isPlaying, andgetPlaybackSpeedmethods fire events that the JS layer listens for (viaonCurrentPosition,onDuration,onIsPlaying,onPlaybackSpeed) and resolves as Promises. The React wrapper inTPStreamsPlayer.tsxcorrectly invokes these commands without expecting synchronous returns. No changes needed.
Summary by CodeRabbit
New Features
Chores