feat(mobile-expo): VideoRenderer section component#324
Conversation
Replace the Video stub with a real renderer that displays a video thumbnail with play button overlay, title, and subtitle. Tapping opens the streaming URL via Linking. Resolves #309 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughThis pull request introduces a functional VideoRenderer component for inline video playback in mobile/expo. It replaces a stub with a real React Native implementation using expo-video, adds unit tests, configures Jest mocks, and updates Expo plugin and package dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Review: Inline video playback required — do not use Linking.openURLThe current implementation uses What needs to change:
Example with expo-video:import { useVideoPlayer, VideoView } from 'expo-video'
const player = useVideoPlayer(streamingUrl, (p) => {
p.loop = false
})
<VideoView
player={player}
style={{ width: '100%', aspectRatio: 16 / 9 }}
allowsFullscreen
allowsPictureInPicture
/>Reference:
The thumbnail fallback, title/subtitle rendering, and accessibility labels all look good — the main change is replacing |
Replace Linking.openURL with expo-video VideoView for inline HLS streaming. Shows poster thumbnail with play button, tapping starts inline playback with native controls. Includes fullscreen and picture-in-picture support. Added global jest.setup.js for expo-video mocks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review feedback addressed (3cc093d)Handled:
Changes:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
mobile/expo/jest.setup.js (1)
1-4: MockVideoViewas a component instead of a string.The current mock returns
"VideoView"as a string. While this works for the current tests (which only callcreateElementwithout rendering), it will break if future tests actually render the component and trigger theisPlaying=truebranch—React cannot render a plain string as a component.♻️ Proposed fix
jest.mock("expo-video", () => ({ useVideoPlayer: () => ({ play: jest.fn(), pause: jest.fn() }), - VideoView: "VideoView", + VideoView: ({ children, ...props }) => children ?? null, }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/expo/jest.setup.js` around lines 1 - 4, The mock for expo-video returns VideoView as a string which cannot be rendered; update the jest.mock to export VideoView as a React component (e.g., a functional component or forwardRef) instead of a string and ensure it accepts props used by code under test (such as isPlaying and any children) so rendering and prop-branch logic will work; keep useVideoPlayer mocked as before and ensure the test setup imports React if required so the mocked component can return a valid React element.mobile/expo/src/components/sections/VideoRenderer.test.tsx (1)
28-67: Consider expanding test coverage beyond "doesn't throw" checks.The current tests only verify that
createElementdoesn't throw, which confirms the component can be instantiated but doesn't validate:
- Actual rendering output (thumbnail, play button, title/subtitle)
- User interactions (play/pause behavior)
- Accessibility attributes are correctly applied
This is a reasonable starting point, but consider adding render-based tests using
@testing-library/react-nativefor more robust coverage:♻️ Example expanded test
import { render, fireEvent } from "@testing-library/react-native" it("renders title and subtitle when provided", () => { const { getByText } = render(<VideoRenderer section={baseSection} />) expect(getByText("Jesus' Victory Over Sin and Death")).toBeTruthy() expect(getByText("Watch the full story")).toBeTruthy() }) it("renders play button with accessible label", () => { const { getByLabelText } = render(<VideoRenderer section={baseSection} />) expect(getByLabelText("Play Jesus' Victory Over Sin and Death")).toBeTruthy() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/expo/src/components/sections/VideoRenderer.test.tsx` around lines 28 - 67, Replace the current "doesn't throw" instantiation tests in VideoRenderer.test.tsx with render-based assertions using `@testing-library/react-native`: import render and fireEvent, render <VideoRenderer section={baseSection} /> and assert visible text for title/subtitle (use getByText), assert thumbnail and play button presence and accessible label (use getByLabelText or getByA11yLabel), and add an interaction test using fireEvent.press to simulate play/pause and verify UI/state changes; reference the VideoRenderer component and the baseSection/minimal section objects to create the different test cases (full, minimal, fallback thumbnail, no-thumb).mobile/expo/src/components/sections/VideoRenderer.tsx (1)
17-35: SyncisPlayingstate with actual player status using Expo'suseEventhook.The
isPlayingstate is managed independently from the actual player state. If a user pauses via native controls (enabled on line 48),isPlayingremainstrueand the poster won't reappear. Similarly, when the video ends naturally, the state won't reset.Use Expo's
useEventhook to automatically synchronize state with player events:♻️ Recommended approach with useEvent
import { useEvent } from 'expo' // ... const { isPlaying } = useEvent(player, 'playingChange', { isPlaying: false })This hook manages the event subscription and cleanup automatically, keeping your UI in sync with the actual player state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/expo/src/components/sections/VideoRenderer.tsx` around lines 17 - 35, The local isPlaying state is out of sync with the real player when users use native controls or when playback ends; replace the manual state management (setIsPlaying in handlePlayPress/handlePausePress) by subscribing to the player's playing state via Expo's useEvent so UI follows the player's actual 'playingChange' events (use the existing player from useVideoPlayer and remove or stop relying on manual setIsPlaying). Ensure you import useEvent from 'expo', derive isPlaying from the event subscription (keep the variable name isPlaying), and remove any redundant state updates to avoid conflicting sources of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mobile/expo/src/components/sections/VideoRenderer.tsx`:
- Around line 53-59: The pause overlay Pressable (style pauseOverlay) is
covering the entire video (absoluteFillObject) and intercepting nativeControls
touches; update the Pressable so it no longer blocks underlying native controls
by either removing the overlay or changing its touch behavior—recommended
change: remove absoluteFillObject from styles.pauseOverlay and add
pointerEvents="box-none" to the Pressable (keep onPress handlePausePress) so
taps on nativeControls are passed through; alternatively, if you prefer removing
manual overlay entirely, delete the Pressable that references pauseOverlay and
handlePausePress.
---
Nitpick comments:
In `@mobile/expo/jest.setup.js`:
- Around line 1-4: The mock for expo-video returns VideoView as a string which
cannot be rendered; update the jest.mock to export VideoView as a React
component (e.g., a functional component or forwardRef) instead of a string and
ensure it accepts props used by code under test (such as isPlaying and any
children) so rendering and prop-branch logic will work; keep useVideoPlayer
mocked as before and ensure the test setup imports React if required so the
mocked component can return a valid React element.
In `@mobile/expo/src/components/sections/VideoRenderer.test.tsx`:
- Around line 28-67: Replace the current "doesn't throw" instantiation tests in
VideoRenderer.test.tsx with render-based assertions using
`@testing-library/react-native`: import render and fireEvent, render
<VideoRenderer section={baseSection} /> and assert visible text for
title/subtitle (use getByText), assert thumbnail and play button presence and
accessible label (use getByLabelText or getByA11yLabel), and add an interaction
test using fireEvent.press to simulate play/pause and verify UI/state changes;
reference the VideoRenderer component and the baseSection/minimal section
objects to create the different test cases (full, minimal, fallback thumbnail,
no-thumb).
In `@mobile/expo/src/components/sections/VideoRenderer.tsx`:
- Around line 17-35: The local isPlaying state is out of sync with the real
player when users use native controls or when playback ends; replace the manual
state management (setIsPlaying in handlePlayPress/handlePausePress) by
subscribing to the player's playing state via Expo's useEvent so UI follows the
player's actual 'playingChange' events (use the existing player from
useVideoPlayer and remove or stop relying on manual setIsPlaying). Ensure you
import useEvent from 'expo', derive isPlaying from the event subscription (keep
the variable name isPlaying), and remove any redundant state updates to avoid
conflicting sources of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4434c145-d81b-43db-a2b0-991f6f4a33b9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
mobile/expo/app.jsonmobile/expo/jest.setup.jsmobile/expo/package.jsonmobile/expo/src/components/sections/SectionDispatcher.tsxmobile/expo/src/components/sections/VideoRenderer.test.tsxmobile/expo/src/components/sections/VideoRenderer.tsxmobile/expo/src/components/sections/VideoStub.tsxmobile/expo/src/components/sections/index.ts
💤 Files with no reviewable changes (1)
- mobile/expo/src/components/sections/VideoStub.tsx
# Conflicts: # mobile/expo/jest.setup.js # mobile/expo/src/components/sections/SectionDispatcher.tsx # mobile/expo/src/components/sections/index.ts
…deoRenderer Replace manual play/pause toggle with poster-over-VideoView pattern. VideoView with nativeControls is always rendered; poster overlay hides once playback starts, letting native controls handle pause/seek/fullscreen. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review feedback addressed (fa17416)Handled:
Test results
|
Summary
mediaorvideo.image) with 16:9 aspect ratiostreamingUrlviaLinking.openURLContracts Changed
No
Regeneration Required
No
Validation
pnpm --filter @forge/expo lint— passes with 0 warningspnpm --filter @forge/expo test -- --testPathPattern=VideoRenderer— 4/4 tests passpnpm --filter @forge/expo test -- --testPathPattern=SectionDispatcher— 11/11 tests pass (no regressions)Resolves #309
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests