feat(mobile-expo): VideoHeroRenderer section component#321
feat(mobile-expo): VideoHeroRenderer section component#321
Conversation
Replace the VideoHero stub with a real renderer that displays a hero banner with video thumbnail background, heading, subheading, and CTA button. Handles missing optional fields gracefully. Resolves #306 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughAdds a new VideoHeroRenderer (replacing the stub), extends VideoHeroSection with streamingUrl, integrates expo-video (dependency, plugin, Jest mock), updates exports and tests, and adds unit tests for the new renderer. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Renderer as VideoHeroRenderer
participant PlayerHook as useVideoPlayer
participant Video as VideoView
participant Linking as Linking.openURL
User->>Renderer: tap Play
Renderer->>PlayerHook: toggle play/pause
PlayerHook->>Video: play (HLS) / pause
Video-->>Renderer: playback state update
User->>Renderer: tap CTA
Renderer->>Linking: openURL(ctaLink)
Linking-->>User: external link opened
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/VideoHeroRenderer.tsx`:
- Around line 20-24: handleCtaPress currently treats any non-null ctaLink as
valid and calls Linking.openURL while ignoring failures; change handleCtaPress
to first trim and validate ctaLink (ensure it's non-empty after trim and matches
a valid URL scheme), then use Linking.canOpenURL(ctaLink) and only call
Linking.openURL if canOpenURL resolves true, wrapping the open call in a
try/catch to handle rejected promises (log or surface an error) and avoid
creating a visible CTA when ctaLink is empty/whitespace; reference
handleCtaPress, ctaLink, ctaLabel, and Linking.openURL/Linking.canOpenURL when
locating where to implement these checks.
- Around line 27-81: The JSX suppression comments in VideoHeroRenderer.tsx hide
prop type errors; import JSX from React and annotate the component and local JSX
values instead of using `@ts-expect-error`: add import { JSX } from "react",
change the VideoHeroRenderer function signature to return JSX.Element (and type
the local content variable as JSX.Element | null if present), then remove the
inline `@ts-expect-error` comments on View, Text, Pressable, and ImageBackground
so React Native prop types are checked properly (references: VideoHeroRenderer,
content, thumbnailUrl, and the JSX elements
View/Text/Pressable/ImageBackground).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b93c46a-b0e7-4fc6-8adf-cf148c310a4b
📒 Files selected for processing (5)
mobile/expo/src/components/sections/SectionDispatcher.tsxmobile/expo/src/components/sections/VideoHeroRenderer.test.tsxmobile/expo/src/components/sections/VideoHeroRenderer.tsxmobile/expo/src/components/sections/VideoHeroStub.tsxmobile/expo/src/components/sections/index.ts
💤 Files with no reviewable changes (1)
- mobile/expo/src/components/sections/VideoHeroStub.tsx
…roRenderer Trim ctaLabel and ctaLink before rendering to prevent showing a CTA button with empty or whitespace-only values from CMS data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review feedback addressed (e12a03b)Handled:
Declined:
|
Review: Inline video playback requiredThe current implementation uses What needs to change:
Reference:
The heading, subheading, CTA, and fallback behavior all look good — just needs the inline video player added. |
Replace static ImageBackground with expo-video VideoView for inline HLS streaming. Add streamingUrl field to VideoHeroSection model. Includes play/pause overlay controls, fallback to thumbnail when streamingUrl is null, and global jest.setup.js for expo-video mocks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review feedback addressed (ab09c82)Handled:
Changes:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mobile/expo/src/lib/sectionModels.ts (1)
80-89:⚠️ Potential issue | 🔴 CriticalMap
streamingUrlinsectionMapperor the new hero player never renders.This interface now requires
streamingUrl, butmobile/expo/src/lib/sectionMapper.ts:1-20still buildsVideoHeroSectionwithout it. In production that leavesstreamingUrlundefined, soVideoHeroRendereralways takes the non-video branch and the inline HLS path added in this PR never activates.🐛 Proposed fix
function mapVideoHero( raw: RawSection & { __typename: "ComponentSectionsVideoHero" }, ): VideoHeroSection { return { kind: "videoHero", id: raw.id, sectionKey: raw.sectionKey ?? null, heading: raw.videoHeroHeading ?? null, subheading: raw.subheading ?? null, + streamingUrl: raw.streamingUrl ?? null, ctaLink: raw.ctaLink ?? null, ctaLabel: raw.ctaLabel ?? null, video: mapVideoModel(raw.heroVideo), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/expo/src/lib/sectionModels.ts` around lines 80 - 89, The VideoHeroSection interface now requires streamingUrl but sectionMapper still constructs VideoHeroSection without it, causing VideoHeroRenderer to take the non-video branch; update the sectionMapper function that builds VideoHeroSection to populate the streamingUrl field (use the source object's streaming URL or null fallback) so VideoHeroSection.streamingUrl is defined when appropriate and the inline HLS path in VideoHeroRenderer can activate; ensure you reference the same property name (streamingUrl) and preserve other fields (video, ctaLink, ctaLabel, heading, subheading, sectionKey, id).
🧹 Nitpick comments (1)
mobile/expo/jest.setup.js (1)
1-4: Run theuseVideoPlayersetup callback in the mock.
VideoHeroRendererconfigures the player in the hook callback (p.loop = false), but this mock never invokes that callback. Tests will stay green even if the initialization path regresses.♻️ Proposed fix
jest.mock("expo-video", () => ({ - useVideoPlayer: () => ({ play: jest.fn(), pause: jest.fn() }), + useVideoPlayer: (_source, setup) => { + const player = { play: jest.fn(), pause: jest.fn(), loop: false } + setup?.(player) + return player + }, VideoView: "VideoView", }))🤖 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 does not invoke the setup callback passed to useVideoPlayer, so VideoHeroRenderer's initialization (e.g., setting p.loop = false) never runs in tests; update the jest.mock for "expo-video" so useVideoPlayer accepts and calls the provided setup callback with a player object (containing play, pause, loop, etc.) before returning control, ensuring the hook's initializer in VideoHeroRenderer is exercised during tests.
🤖 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/VideoHeroRenderer.tsx`:
- Around line 48-73: When streamingUrl is present we currently render VideoView
immediately so the poster never shows; update VideoHeroRenderer to display
thumbnailUrl as a visible poster before playback by rendering a thumbnail Image
(or ImageBackground) when !isPlaying and thumbnailUrl exists, and hide it once
playback starts (isPlaying true). Keep existing VideoView, Pressable,
handlePlayPress/handlePausePress and play/pause overlay, but conditionally show
the thumbnail (above the VideoView or by toggling VideoView opacity/visibility)
so users see the poster while the stream buffers; reference streamingUrl,
thumbnailUrl, isPlaying, VideoView, Pressable, handlePlayPress,
handlePausePress, styles.playPauseOverlay, styles.playButton, and
styles.playIcon when making the change.
- Around line 21-23: The component currently passes an empty string to
useVideoPlayer when streamingUrl is missing; change the initial source argument
to null (i.e., call useVideoPlayer(streamingUrl ?? null, ...)) so the player
doesn't treat "" as an invalid URI, and when a stream appears call
player.replaceAsync or player.replace to load the media; update references
around useVideoPlayer, player, and streamingUrl to implement this change.
---
Outside diff comments:
In `@mobile/expo/src/lib/sectionModels.ts`:
- Around line 80-89: The VideoHeroSection interface now requires streamingUrl
but sectionMapper still constructs VideoHeroSection without it, causing
VideoHeroRenderer to take the non-video branch; update the sectionMapper
function that builds VideoHeroSection to populate the streamingUrl field (use
the source object's streaming URL or null fallback) so
VideoHeroSection.streamingUrl is defined when appropriate and the inline HLS
path in VideoHeroRenderer can activate; ensure you reference the same property
name (streamingUrl) and preserve other fields (video, ctaLink, ctaLabel,
heading, subheading, sectionKey, id).
---
Nitpick comments:
In `@mobile/expo/jest.setup.js`:
- Around line 1-4: The mock for expo-video does not invoke the setup callback
passed to useVideoPlayer, so VideoHeroRenderer's initialization (e.g., setting
p.loop = false) never runs in tests; update the jest.mock for "expo-video" so
useVideoPlayer accepts and calls the provided setup callback with a player
object (containing play, pause, loop, etc.) before returning control, ensuring
the hook's initializer in VideoHeroRenderer is exercised during tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00133f7b-631d-426b-bfee-0fbf7080c974
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
mobile/expo/app.jsonmobile/expo/jest.setup.jsmobile/expo/package.jsonmobile/expo/src/components/sections/SectionDispatcher.test.tsxmobile/expo/src/components/sections/VideoHeroRenderer.test.tsxmobile/expo/src/components/sections/VideoHeroRenderer.tsxmobile/expo/src/lib/sectionModels.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mobile/expo/src/components/sections/VideoHeroRenderer.test.tsx
| const player = useVideoPlayer(streamingUrl ?? "", (p) => { | ||
| p.loop = false | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the official expo-video useVideoPlayerAPI, should the source benull rather than an empty string when no media should be loaded yet?
💡 Result:
Yes—per the official expo-video docs, if you don’t want any media loaded yet, initialize the player with a null source (then later load media by replacing the source via replaceAsync / replace). Passing an empty string would be treated like a (invalid) URI rather than “no source.” [1]
Source:
[1] Expo Documentation – expo-video (“Preloading videos” section, mentions creating a VideoPlayer with a null source)
Use null, not "", for the no-stream player source.
When streamingUrl is absent, the component passes an empty string to useVideoPlayer instead of null. According to the official expo-video documentation, an empty string is treated as an (invalid) URI. Instead, pass null to the player when no media should be loaded yet, then use replaceAsync or replace to load media later.
Proposed fix
- const player = useVideoPlayer(streamingUrl ?? "", (p) => {
+ const player = useVideoPlayer(streamingUrl, (p) => {
p.loop = false
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const player = useVideoPlayer(streamingUrl ?? "", (p) => { | |
| p.loop = false | |
| }) | |
| const player = useVideoPlayer(streamingUrl, (p) => { | |
| p.loop = false | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/expo/src/components/sections/VideoHeroRenderer.tsx` around lines 21 -
23, The component currently passes an empty string to useVideoPlayer when
streamingUrl is missing; change the initial source argument to null (i.e., call
useVideoPlayer(streamingUrl ?? null, ...)) so the player doesn't treat "" as an
invalid URI, and when a stream appears call player.replaceAsync or
player.replace to load the media; update references around useVideoPlayer,
player, and streamingUrl to implement this change.
| {streamingUrl ? ( | ||
| <> | ||
| {/* @ts-expect-error React 19 vs RN component types */} | ||
| <VideoView | ||
| player={player} | ||
| style={StyleSheet.absoluteFill} | ||
| nativeControls={false} | ||
| contentFit="cover" | ||
| /> | ||
| {/* Play/pause overlay */} | ||
| {/* @ts-expect-error React 19 vs RN component types */} | ||
| <Pressable | ||
| style={styles.playPauseOverlay} | ||
| onPress={isPlaying ? handlePausePress : handlePlayPress} | ||
| accessibilityRole="button" | ||
| accessibilityLabel={isPlaying ? "Pause video" : "Play video"} | ||
| > | ||
| {!isPlaying && ( | ||
| // @ts-expect-error React 19 vs RN component types | ||
| <View style={styles.playButton}> | ||
| {/* @ts-expect-error RN Text vs React 19 ReactNode */} | ||
| <Text style={styles.playIcon}>▶</Text> | ||
| </View> | ||
| )} | ||
| </Pressable> | ||
| </> |
There was a problem hiding this comment.
The thumbnail never appears before playback starts.
When streamingUrl exists, this branch renders VideoView immediately and never uses thumbnailUrl. That misses the poster/placeholder requirement from issue #306 and can leave users staring at a blank hero while the stream buffers.
🐛 Proposed fix
- const [isPlaying, setIsPlaying] = useState(false)
+ const [isPlaying, setIsPlaying] = useState(false)
+ const [hasStarted, setHasStarted] = useState(false)
@@
const handlePlayPress = () => {
if (player) {
player.play()
+ setHasStarted(true)
setIsPlaying(true)
}
}
@@
{streamingUrl ? (
<>
{/* `@ts-expect-error` React 19 vs RN component types */}
<VideoView
player={player}
style={StyleSheet.absoluteFill}
nativeControls={false}
contentFit="cover"
/>
+ {!hasStarted && thumbnailUrl ? (
+ // `@ts-expect-error` React 19 vs RN component types
+ <Image
+ source={{ uri: thumbnailUrl }}
+ style={StyleSheet.absoluteFill}
+ resizeMode="cover"
+ accessibilityLabel={
+ video.image?.alternativeText ?? `${video.title} thumbnail`
+ }
+ />
+ ) : null}
{/* Play/pause overlay */}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/expo/src/components/sections/VideoHeroRenderer.tsx` around lines 48 -
73, When streamingUrl is present we currently render VideoView immediately so
the poster never shows; update VideoHeroRenderer to display thumbnailUrl as a
visible poster before playback by rendering a thumbnail Image (or
ImageBackground) when !isPlaying and thumbnailUrl exists, and hide it once
playback starts (isPlaying true). Keep existing VideoView, Pressable,
handlePlayPress/handlePausePress and play/pause overlay, but conditionally show
the thumbnail (above the VideoView or by toggling VideoView opacity/visibility)
so users see the poster while the stream buffers; reference streamingUrl,
thumbnailUrl, isPlaying, VideoView, Pressable, handlePlayPress,
handlePausePress, styles.playPauseOverlay, styles.playButton, and
styles.playIcon when making the change.
Test Report — PR #321 (VideoHeroRenderer)CI StatusAll checks passing. Local Checks
Fixes Applied
iOS Screenshot (live data from Strapi)VideoHeroRenderer renders correctly with live API data:
Notes
🤖 Generated with Claude Code |
… issues Add ComponentSectionsVideoHero inline fragment to GET_WATCH_EXPERIENCE query to resolve runtime crash (Cannot read property 'documentId' of undefined). Also add missing streamingUrl mapping, jest globals for eslint, and remove unused @ts-expect-error directives. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
mobile/expo/src/components/sections/VideoHeroRenderer.tsx (2)
47-68:⚠️ Potential issue | 🟠 MajorPoster/thumbnail not shown before playback starts.
When
streamingUrlexists,VideoViewrenders immediately without displaying the thumbnail. Per issue#306, the video entity's image should be used as a "poster/thumbnail before playback." Currently,thumbnailUrlis computed but never used in this branch, leaving users with a potentially blank hero while the stream buffers.Proposed fix
+ const [hasStarted, setHasStarted] = useState(false) + const handlePlayPress = () => { if (player) { player.play() + setHasStarted(true) setIsPlaying(true) } } @@ {streamingUrl ? ( <> - <VideoView - player={player} - style={StyleSheet.absoluteFill} - nativeControls={false} - contentFit="cover" - /> + {hasStarted ? ( + <VideoView + player={player} + style={StyleSheet.absoluteFill} + nativeControls={false} + contentFit="cover" + /> + ) : thumbnailUrl ? ( + <Image + source={{ uri: thumbnailUrl }} + style={StyleSheet.absoluteFill} + resizeMode="cover" + accessibilityLabel={ + video.image?.alternativeText ?? `${video.title} thumbnail` + } + /> + ) : ( + <View style={[StyleSheet.absoluteFill, styles.fallbackBackground]} /> + )} {/* Play/pause overlay */}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/expo/src/components/sections/VideoHeroRenderer.tsx` around lines 47 - 68, The VideoView renders immediately when streamingUrl exists and never shows the computed thumbnailUrl; update VideoHeroRenderer to display the thumbnail before playback by rendering the thumbnail Image beneath (or as a background to) the VideoView when streamingUrl is truthy and isPlaying is false (use thumbnailUrl as the Image source and StyleSheet.absoluteFill so it fills the hero). Ensure the Image is hidden or removed once playback starts (tied to isPlaying or video-ready state) and keep the existing Play/pause overlay and handlers (handlePlayPress, handlePausePress) intact so tapping plays the video and hides the poster.
21-23:⚠️ Potential issue | 🟠 MajorUse
null, not"", for the no-stream player source.The
expo-videouseVideoPlayerhook acceptsnullas a valid source type for when no media is loaded yet. An empty string would be treated as an invalid URI. Initialize withnulland usereplaceAsync()to load the source when available.Proposed fix
- const player = useVideoPlayer(streamingUrl ?? "", (p) => { + const player = useVideoPlayer(streamingUrl ?? null, (p) => { p.loop = false })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/expo/src/components/sections/VideoHeroRenderer.tsx` around lines 21 - 23, Change the useVideoPlayer initialization to pass null instead of an empty string so the hook treats it as no source: call useVideoPlayer(null, (p) => { p.loop = false }) and when streamingUrl becomes available, load it via player.replaceAsync or equivalent (reference symbols: useVideoPlayer, player.replaceAsync, streamingUrl) to set the media source dynamically.
🧹 Nitpick comments (1)
mobile/expo/src/components/sections/VideoHeroRenderer.tsx (1)
31-43: SynchronizeisPlayingstate with player status events.The
isPlayingstate is managed manually viahandlePlayPress/handlePausePressbut doesn't sync with actual player state. If the video ends, buffers, or encounters an error,isPlayingwill be stale. Useexpo'suseEventhook to listen to the player'sstatusChangeevent (orplayingChangeif available):import { useEvent } from 'expo'; const { status } = useEvent(player, 'statusChange', { status: player.status, }); // Update isPlaying based on status.isPlaying or player.playing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/expo/src/components/sections/VideoHeroRenderer.tsx` around lines 31 - 43, The isPlaying state set only in handlePlayPress/handlePausePress can become stale; wire it to the player's real status by subscribing to player events (useEvent) and updating setIsPlaying from the event payload (e.g., status.isPlaying or status.playing). Add a useEvent subscription for player with the 'statusChange' (or 'playingChange') event, read the status boolean and call setIsPlaying accordingly, and ensure you still keep handlePlayPress/handlePausePress for user-initiated play/pause but remove any conflicting manual state updates so state always reflects player status.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mobile/expo/src/components/sections/VideoHeroRenderer.tsx`:
- Around line 47-68: The VideoView renders immediately when streamingUrl exists
and never shows the computed thumbnailUrl; update VideoHeroRenderer to display
the thumbnail before playback by rendering the thumbnail Image beneath (or as a
background to) the VideoView when streamingUrl is truthy and isPlaying is false
(use thumbnailUrl as the Image source and StyleSheet.absoluteFill so it fills
the hero). Ensure the Image is hidden or removed once playback starts (tied to
isPlaying or video-ready state) and keep the existing Play/pause overlay and
handlers (handlePlayPress, handlePausePress) intact so tapping plays the video
and hides the poster.
- Around line 21-23: Change the useVideoPlayer initialization to pass null
instead of an empty string so the hook treats it as no source: call
useVideoPlayer(null, (p) => { p.loop = false }) and when streamingUrl becomes
available, load it via player.replaceAsync or equivalent (reference symbols:
useVideoPlayer, player.replaceAsync, streamingUrl) to set the media source
dynamically.
---
Nitpick comments:
In `@mobile/expo/src/components/sections/VideoHeroRenderer.tsx`:
- Around line 31-43: The isPlaying state set only in
handlePlayPress/handlePausePress can become stale; wire it to the player's real
status by subscribing to player events (useEvent) and updating setIsPlaying from
the event payload (e.g., status.isPlaying or status.playing). Add a useEvent
subscription for player with the 'statusChange' (or 'playingChange') event, read
the status boolean and call setIsPlaying accordingly, and ensure you still keep
handlePlayPress/handlePausePress for user-initiated play/pause but remove any
conflicting manual state updates so state always reflects player status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f845cd2e-aa60-42d9-9bf8-23e2bb8f5b51
📒 Files selected for processing (5)
mobile/expo/eslint.config.jsmobile/expo/src/components/sections/SectionDispatcher.tsxmobile/expo/src/components/sections/VideoHeroRenderer.tsxmobile/expo/src/lib/sectionMapper.tspackages/graphql/src/watchExperience.ts
Summary
VideoHerostub renderer with a real implementation that displays a full-width hero bannerImageBackgroundfor video thumbnail with dark overlay for text legibilityPressable+Linking.openURL)ctaLabel/ctaLinkabsent, fallback background when no thumbnail)Contracts Changed
No
Regeneration Required
No
Validation
pnpm --filter @forge/expo lint— passes with 0 warningspnpm --filter @forge/expo test -- --testPathPattern=VideoHeroRenderer— 4/4 tests passpnpm --filter @forge/expo test -- --testPathPattern=SectionDispatcher— 11/11 tests pass (no regressions)Resolves #306
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores