Up/nes 1194 youtube videos fail to play on android when opened via#8759
Up/nes 1194 youtube videos fail to play on android when opened via#8759
Conversation
Video.js with videojs-youtube plugin fails to play YouTube videos in Instagram's Android in-app browser due to YouTube IFrame API restrictions. Detect Instagram Android WebView and render a plain <iframe> embed instead.
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
|
|
WalkthroughAdds Instagram Android in-app browser detection and conditional YouTube iframe fallbacks in two video components; fixes two Prisma where-clause keys in the user creation/update flow; exposes the new device utility via the shared deviceUtils index. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Instagram Android WebView
participant Component as Video / BackgroundVideo
participant Utils as deviceUtils.isInstagramAndroidWebView
participant Renderer as iframe or video element
Browser->>Component: load page with video
Component->>Utils: call isInstagramAndroidWebView()
Utils-->>Component: returns true/false
alt returns true and source is YouTube & videoId present
Component->>Renderer: render YouTube iframe fallback
else returns false or not YouTube
Component->>Renderer: initialize video.js / render video element
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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 |
|
View your CI Pipeline Execution ↗ for commit 46a4fd5
☁️ Nx Cloud last updated this comment at |
github-actions
bot
requested a deployment
to
Preview - journeys-admin
|
Found 1 test failure on Blacksmith runners: Failure
|
|
The latest updates on your projects.
|
github-actions
bot
temporarily deployed
to
Preview - journeys-admin
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
libs/shared/ui/src/libs/deviceUtils/deviceUtils.ts (1)
63-72: Optional: Add the convention// TODOcomment.Every other UA-sniffing function in this file carries the
// TODO: should only resort to user agent sniffing as a last resportannotation. For consistency, the same note could be added above this function.📝 Proposed addition
+// TODO: should only resort to user agent sniffing as a last resport export function isInstagramAndroidWebView(): boolean {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/shared/ui/src/libs/deviceUtils/deviceUtils.ts` around lines 63 - 72, Add the same TODO annotation used by other UA-sniffing helpers above the isInstagramAndroidWebView function: insert a comment like "// TODO: should only resort to user agent sniffing as a last resort" immediately above the export function isInstagramAndroidWebView() declaration so this function matches the file's convention and signals UA sniffing is a last-resort approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsx`:
- Around line 155-160: The iframe embed in BackgroundVideo uses loop=1 but omits
the required playlist parameter, so the video won't actually loop; update the
iframe src construction in BackgroundVideo (the template that builds the YouTube
URL for the iframe) to append &playlist=${videoId} whenever loop=1 (or always
append playlist=${videoId} when videoId is present) so the embed URL becomes
...?start=...&end=...&autoplay=1&mute=1&loop=1&playlist=${videoId}&playsinline=1&controls=0;
ensure videoId is used safely (only add playlist if videoId exists) and keep
pointerEvents/style/title unchanged.
- Around line 40-44: Replace the current useState + useEffect that sets
inAppBrowser with a lazy initializer so inAppBrowser is correctly known on first
render (useState(() => isInstagramAndroidWebView())). Then update the player
initialization effect (the effect that calls videojs with videoRef.current and
attaches 'seeked' => setLoading(false)) to early-return when inAppBrowser is
true so the player is not created for the iframe path; finally, in the iframe
branch (where StyledVideo is not rendered) call setLoading(false) on mount
(e.g., a small effect tied to the iframe branch) so loading is resolved
immediately; also ensure any created player is disposed in the player cleanup to
avoid orphaned players if state changes.
In `@libs/journeys/ui/src/components/Video/Video.tsx`:
- Around line 78-82: InitAndPlay currently always initializes the video.js
player before the async isInstagramAndroidWebView check completes, causing an
orphaned player when StyledVideo/videoRef is removed; update the component so
InitAndPlay only initializes when the inAppBrowser flag is false (i.e., render
InitAndPlay conditionally: {!inAppBrowser && <InitAndPlay ...>}), or implement
the same playerRef guard pattern used in BackgroundVideo.tsx (check
videoRef.current before creating/attaching the player in InitAndPlay, store the
instance in playerRef, and ensure cleanup/dispose and removal of VideoEvents
listeners when videoRef becomes null) so VideoEvents (which attaches listeners
to player) never binds to an orphaned player.
---
Nitpick comments:
In `@libs/shared/ui/src/libs/deviceUtils/deviceUtils.ts`:
- Around line 63-72: Add the same TODO annotation used by other UA-sniffing
helpers above the isInstagramAndroidWebView function: insert a comment like "//
TODO: should only resort to user agent sniffing as a last resort" immediately
above the export function isInstagramAndroidWebView() declaration so this
function matches the file's convention and signals UA sniffing is a last-resort
approach.
| const [inAppBrowser, setInAppBrowser] = useState(false) | ||
|
|
||
| useEffect(() => { | ||
| setInAppBrowser(isInstagramAndroidWebView()) | ||
| }, []) |
There was a problem hiding this comment.
Race condition: video.js player is initialized before the iframe path takes over, leaving it orphaned and setLoading unresolved.
All useEffect hooks with [] deps run sequentially after the first commit. On that first render inAppBrowser is still false, so StyledVideo is in the DOM and videoRef.current is valid. The execution order is:
- Effect at lines 42–44 →
setInAppBrowser(true)(schedules a state update) - Effect at lines 47–68 →
videojs(videoRef.current, …)runs ✅ — player IS created - Effect at lines 71–124 → event handlers attached (
'seeked'→setLoading(false)) - State flush →
inAppBrowser = true→ re-render →StyledVideoremoved
Consequences:
- The orphaned player is never disposed, causing a memory leak and unnecessary network requests to YouTube (which fail in Instagram's WebView anyway).
setLoading(false)is wired to the player's'seeked'event. Because YouTube never loads in Instagram WebView,seekednever fires, sosetLoadingstaystrueand the parent's loading spinner can be stuck indefinitely.
The inAppBrowser iframe path should also call setLoading(false) on mount, and the player init effect should be skipped when in-app-browser is active.
🛠️ Suggested fix
Replace the useState(false) + useEffect pair with a lazy initializer so the value is known at first render (avoiding the player-init race), and add setLoading(false) in the iframe branch:
- const [inAppBrowser, setInAppBrowser] = useState(false)
-
- useEffect(() => {
- setInAppBrowser(isInstagramAndroidWebView())
- }, [])
+ const [inAppBrowser] = useState<boolean>(
+ () => typeof navigator !== 'undefined' && isInstagramAndroidWebView()
+ )
// Initiate Video
useEffect(() => {
- if (videoRef.current != null) {
+ if (!inAppBrowser && videoRef.current != null) {
playerRef.current = videojs(videoRef.current, {
...
})
}
- }, [])
+ }, [inAppBrowser])And in the iframe JSX branch, resolve the loading state immediately:
if (inAppBrowser && isYouTube && videoId != null) {
+ setLoading(false) // or use a one-time useEffect in a sub-component
return (
<Box ...>
<iframe ... />
</Box>
)
}
⚠️ Note: the lazy initializer computesfalseon the server (SSR) and the actual value on the client — if the component is SSR'd, React may log a hydration mismatch. Given this is a mobile in-app browser path, that trade-off is usually acceptable; alternatively keep theuseEffectapproach but add an explicitdispose()cleanup on the player and the iframe-branchuseEffectforsetLoading(false).
Also applies to: 47-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsx`
around lines 40 - 44, Replace the current useState + useEffect that sets
inAppBrowser with a lazy initializer so inAppBrowser is correctly known on first
render (useState(() => isInstagramAndroidWebView())). Then update the player
initialization effect (the effect that calls videojs with videoRef.current and
attaches 'seeked' => setLoading(false)) to early-return when inAppBrowser is
true so the player is not created for the iframe path; finally, in the iframe
branch (where StyledVideo is not rendered) call setLoading(false) on mount
(e.g., a small effect tied to the iframe branch) so loading is resolved
immediately; also ensure any created player is disposed in the player cleanup to
avoid orphaned players if state changes.
| <iframe | ||
| src={`https://www.youtube.com/embed/${videoId}?start=${startAt ?? 0}&end=${endAt ?? 0}&autoplay=1&mute=1&loop=1&playsinline=1&controls=0`} | ||
| allow="accelerometer; autoplay; encrypted-media" | ||
| style={{ width: '100%', height: '100%', border: 'none', pointerEvents: 'none' }} | ||
| title="Background video" | ||
| /> |
There was a problem hiding this comment.
loop=1 without &playlist=${videoId} — the video will not loop.
The YouTube embed loop parameter only works in conjunction with the playlist parameter. To loop a single video, set loop=1 and playlist=VIDEO_ID in the embed URL. Without playlist, the video plays once and stops — the background will be a frozen last frame.
🛠️ Proposed fix
- src={`https://www.youtube.com/embed/${videoId}?start=${startAt ?? 0}&end=${endAt ?? 0}&autoplay=1&mute=1&loop=1&playsinline=1&controls=0`}
+ src={`https://www.youtube.com/embed/${videoId}?start=${startAt ?? 0}&end=${endAt ?? 0}&autoplay=1&mute=1&loop=1&playlist=${videoId}&playsinline=1&controls=0`}📝 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.
| <iframe | |
| src={`https://www.youtube.com/embed/${videoId}?start=${startAt ?? 0}&end=${endAt ?? 0}&autoplay=1&mute=1&loop=1&playsinline=1&controls=0`} | |
| allow="accelerometer; autoplay; encrypted-media" | |
| style={{ width: '100%', height: '100%', border: 'none', pointerEvents: 'none' }} | |
| title="Background video" | |
| /> | |
| <iframe | |
| src={`https://www.youtube.com/embed/${videoId}?start=${startAt ?? 0}&end=${endAt ?? 0}&autoplay=1&mute=1&loop=1&playlist=${videoId}&playsinline=1&controls=0`} | |
| allow="accelerometer; autoplay; encrypted-media" | |
| style={{ width: '100%', height: '100%', border: 'none', pointerEvents: 'none' }} | |
| title="Background video" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsx`
around lines 155 - 160, The iframe embed in BackgroundVideo uses loop=1 but
omits the required playlist parameter, so the video won't actually loop; update
the iframe src construction in BackgroundVideo (the template that builds the
YouTube URL for the iframe) to append &playlist=${videoId} whenever loop=1 (or
always append playlist=${videoId} when videoId is present) so the embed URL
becomes
...?start=...&end=...&autoplay=1&mute=1&loop=1&playlist=${videoId}&playsinline=1&controls=0;
ensure videoId is used safely (only add playlist if videoId exists) and keep
pointerEvents/style/title unchanged.
| const [inAppBrowser, setInAppBrowser] = useState(false) | ||
|
|
||
| useEffect(() => { | ||
| setInAppBrowser(isInstagramAndroidWebView()) | ||
| }, []) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate InitAndPlay component
fd "InitAndPlay" --type f -e ts -e tsxRepository: JesusFilm/core
Length of output: 194
🏁 Script executed:
#!/bin/bash
# Read InitAndPlay implementation
cat -n libs/journeys/ui/src/components/Video/InitAndPlay/InitAndPlay.tsxRepository: JesusFilm/core
Length of output: 10452
🏁 Script executed:
#!/bin/bash
# Check Video.tsx for any cleanup logic and VideoEvents implementation
cat -n libs/journeys/ui/src/components/Video/Video.tsx | head -250Repository: JesusFilm/core
Length of output: 9153
🏁 Script executed:
#!/bin/bash
# Continue reading Video.tsx from line 250 onwards
tail -n +250 libs/journeys/ui/src/components/Video/Video.tsxRepository: JesusFilm/core
Length of output: 4820
🏁 Script executed:
#!/bin/bash
# Check if there's any cleanup logic when videoRef changes or when player is disposed
grep -n "dispose\|cleanup\|unmount" libs/journeys/ui/src/components/Video/Video.tsxRepository: JesusFilm/core
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Find and inspect VideoEvents implementation
fd "VideoEvents" --type f -e ts -e tsx | head -5Repository: JesusFilm/core
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Read VideoEvents implementation
cat -n libs/journeys/ui/src/components/VideoEvents/VideoEvents.tsxRepository: JesusFilm/core
Length of output: 27171
🏁 Script executed:
#!/bin/bash
# Check if player is disposed/cleaned up when inAppBrowser changes
grep -rn "dispose\|destroy" libs/journeys/ui/src/components/Video/ --include="*.tsx" --include="*.ts"Repository: JesusFilm/core
Length of output: 248
🏁 Script executed:
#!/bin/bash
# Check if player is conditionally rendered or cleaned up anywhere
grep -n "setPlayer\|player =" libs/journeys/ui/src/components/Video/Video.tsxRepository: JesusFilm/core
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Check BackgroundVideo.tsx to see if same pattern exists there for comparison
fd "BackgroundVideo" --type f -e tsxRepository: JesusFilm/core
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Read BackgroundVideo.tsx to compare the pattern
cat -n libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsx | head -150Repository: JesusFilm/core
Length of output: 5226
🏁 Script executed:
#!/bin/bash
# Continue reading BackgroundVideo.tsx to see the full conditional rendering
tail -n +145 libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsxRepository: JesusFilm/core
Length of output: 2710
🏁 Script executed:
#!/bin/bash
# Check if there's any cleanup when player or inAppBrowser changes in Video.tsx
grep -A5 -B5 "useEffect.*inAppBrowser\|useEffect.*player\|setPlayer.*null" libs/journeys/ui/src/components/Video/Video.tsxRepository: JesusFilm/core
Length of output: 40
Conditional InitAndPlay and player cleanup needed—same race condition as BackgroundVideo.tsx.
InitAndPlay is always rendered (line 187) and initializes the video.js player on first render, before inAppBrowser resolves from its async check. When inAppBrowser becomes true:
StyledVideois removed from the DOM;videoRef.currentbecomesnullInitAndPlayremains mounted with a player object that has no backing DOM elementVideoEvents(lines 211–227) still renders ifactiveStep && player != null, attaching listeners to the orphaned player- Any player events fire analytics mutations and GTM events for a video that isn't actually displayed
Fix: Make InitAndPlay conditional on !inAppBrowser, or apply the player-initialization safeguard used in BackgroundVideo.tsx (use playerRef with a guard to prevent initialization if the condition changes mid-render).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/journeys/ui/src/components/Video/Video.tsx` around lines 78 - 82,
InitAndPlay currently always initializes the video.js player before the async
isInstagramAndroidWebView check completes, causing an orphaned player when
StyledVideo/videoRef is removed; update the component so InitAndPlay only
initializes when the inAppBrowser flag is false (i.e., render InitAndPlay
conditionally: {!inAppBrowser && <InitAndPlay ...>}), or implement the same
playerRef guard pattern used in BackgroundVideo.tsx (check videoRef.current
before creating/attaching the player in InitAndPlay, store the instance in
playerRef, and ensure cleanup/dispose and removal of VideoEvents listeners when
videoRef becomes null) so VideoEvents (which attaches listeners to player) never
binds to an orphaned player.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsx`:
- Line 156: The iframe src construction in BackgroundVideo is adding end=0 when
endAt is undefined which stops playback; update the src string building logic in
the BackgroundVideo component (where
src={`https://www.youtube.com/embed/${videoId}?start=${startAt ?? 0}&end=${endAt
?? 0}&...` } is created) to only append the &end=... query parameter when endAt
is not null/undefined (e.g., conditionally add `&end=${endAt}` only if endAt !=
null), leaving the end parameter out otherwise; apply the same
conditional-append fix to the videojs-youtube source tag construction that
currently uses endAt similarly.
---
Duplicate comments:
In
`@libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsx`:
- Around line 155-160: The iframe src in the BackgroundVideo component currently
sets loop=1 but omits the required playlist parameter, so YouTube will not
actually loop; update the src template (where videoId is used in
BackgroundVideo) to append &playlist=${videoId} alongside the existing loop=1
(preserving start/end/autoplay/mute/etc.) so the embed will correctly loop the
same video.
- Around line 40-44: The inAppBrowser detection must run before first render and
prevent orphaned video.js initialization: change useState(false) to a lazy
initializer using isInstagramAndroidWebView() for inAppBrowser, guard the
videojs init effect so it only runs when inAppBrowser is false and
videoRef.current exists, ensure the effect disposes the player on cleanup, and
in the iframe rendering branch call setLoading(false) immediately so loading
never remains unresolved; refer to inAppBrowser, isInstagramAndroidWebView(),
StyledVideo/videoRef, the videojs initialization effect, and setLoading when
making these changes.
| data-testid="CardContainedBackgroundVideo" | ||
| > | ||
| <iframe | ||
| src={`https://www.youtube.com/embed/${videoId}?start=${startAt ?? 0}&end=${endAt ?? 0}&autoplay=1&mute=1&loop=1&playsinline=1&controls=0`} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
YouTube IFrame embed API end parameter behavior documentation
💡 Result:
The YouTube IFrame embed end player parameter (passed in the embed URL query string or in playerVars) is documented as:
- Meaning: “the time, measured in seconds from the start of the video, when the player should stop playing the video.”
- Type: positive integer.
- Important behavior: the stop time is measured from the beginning of the video, not relative to the
startparameter (and not relative to thestartSecondsused in API load/queue calls). [1]
Related (API-call) behavior that’s often confused with end:
- In the IFrame Player API methods like
loadVideoById({ ..., startSeconds, endSeconds }),endSecondssets the stop time for that playback call, but callingseekTo()cancels theendSecondslimit (it “will no longer be in effect”). [2]
Sources:
[1] YouTube “Embedded Players and Player Parameters” (end parameter) https://developers.google.com/youtube/player_parameters
[2] YouTube “IFrame Player API Reference” (endSeconds in loadVideoById) https://developers.google.com/youtube/iframe_api_reference
🏁 Script executed:
# Let's examine the actual code file and its context
fd BackgroundVideo.tsx --type fRepository: JesusFilm/core
Length of output: 145
🏁 Script executed:
# Read the BackgroundVideo.tsx file to see the context
cat -n libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsx | sed -n '140,170p'Repository: JesusFilm/core
Length of output: 1057
🏁 Script executed:
# Look at the beginning of the file to find where startAt and endAt are defined
head -n 145 libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsx | tail -n 100Repository: JesusFilm/core
Length of output: 2739
🏁 Script executed:
# Search for startAt and endAt declarations in the file
rg -n "startAt|endAt" libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsxRepository: JesusFilm/core
Length of output: 671
🏁 Script executed:
# Search for videojs usage mentioned in the review comment
rg -n "videojs" libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsxRepository: JesusFilm/core
Length of output: 175
🏁 Script executed:
# Let me see what's at line 219 (another endAt issue)
sed -n '210,230p' libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsx | cat -nRepository: JesusFilm/core
Length of output: 957
🏁 Script executed:
# Get the full component signature to understand startAt and endAt types
head -n 50 libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsxRepository: JesusFilm/core
Length of output: 1558
Fix YouTube iframe end parameter defaulting to 0 which stops playback.
When endAt is null or undefined, the current code defaults to end=0 in the iframe URL. According to YouTube's IFrame embed API documentation, the end parameter specifies "the time, measured in seconds from the start of the video, when the player should stop playing the video." Setting end=0 causes the player to stop at the very beginning, preventing playback entirely.
The fix should conditionally include the end parameter only when endAt is defined:
Proposed fix for line 156
- src={`https://www.youtube.com/embed/${videoId}?start=${startAt ?? 0}&end=${endAt ?? 0}&autoplay=1&mute=1&loop=1&playsinline=1&controls=0`}
+ src={`https://www.youtube.com/embed/${videoId}?start=${startAt ?? 0}${endAt != null ? `&end=${endAt}` : ''}&autoplay=1&mute=1&loop=1&playsinline=1&controls=0`}Note: The same issue exists at line 219 in the videojs-youtube source tag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@libs/journeys/ui/src/components/Card/ContainedCover/BackgroundVideo/BackgroundVideo.tsx`
at line 156, The iframe src construction in BackgroundVideo is adding end=0 when
endAt is undefined which stops playback; update the src string building logic in
the BackgroundVideo component (where
src={`https://www.youtube.com/embed/${videoId}?start=${startAt ?? 0}&end=${endAt
?? 0}&...` } is created) to only append the &end=... query parameter when endAt
is not null/undefined (e.g., conditionally add `&end=${endAt}` only if endAt !=
null), leaving the end parameter out otherwise; apply the same
conditional-append fix to the videojs-youtube source tag construction that
currently uses endAt similarly.
There was a problem hiding this comment.
This change doesn't seem to be related to the existing issue. Moreover, since this is a back-end change, please merge into production as a separate PR first, before merging in the remaining front-end changes.
There was a problem hiding this comment.
Please add unit tests for the changes made to this component.
There was a problem hiding this comment.
Please add unit tests for the changes made to this component.
| {videoId != null && | ||
| inAppBrowser && | ||
| source === VideoBlockSource.youTube ? ( | ||
| <Box | ||
| data-testid="in-app-browser-youtube-fallback" | ||
| sx={{ | ||
| width: '100%', | ||
| height: '100%', | ||
| position: 'relative' | ||
| }} | ||
| > | ||
| <iframe | ||
| src={`https://www.youtube.com/embed/${videoId}?start=${effectiveStartAt}&end=${effectiveEndAt}&autoplay=0&playsinline=1`} | ||
| allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; fullscreen" | ||
| allowFullScreen | ||
| style={{ width: '100%', height: '100%', border: 'none' }} | ||
| title={title ?? 'YouTube video'} | ||
| /> | ||
| </Box> | ||
| ) : videoId != null ? ( |
There was a problem hiding this comment.
Not sure if possible cleanly, but could we remove the repeated videoId != null ? ternary operator condition? (In accordance with DRY principle)
Ur-imazing
left a comment
Ur-imazing
left a comment
There was a problem hiding this comment.
Please see my comments + code rabbit comments.
|
This pull request has been marked stale due to 28 days without activity. It will be closed in 14 days unless updated. |
csiyang
left a comment
csiyang
left a comment
There was a problem hiding this comment.
Also don't for get to address the existing comments
| } | ||
| } | ||
|
|
||
| if (inAppBrowser && isYouTube && videoId != null) { |
There was a problem hiding this comment.
Lets rename for clarity, maybe something like this.
| if (inAppBrowser && isYouTube && videoId != null) { | |
| if ( useIframePlayer && isYouTube && videoId != null) { |
Also make the same change for Video.tsx
| position="absolute" | ||
| data-testid="CardContainedBackgroundVideo" | ||
| > | ||
| <StyledVideo |
There was a problem hiding this comment.
We can actually move your new player in here, and use a ternary to render either the styled video, or your new iframe player. Just makes the code tidier, and helps keeps styling on the parent box consistent
| {videoId != null && | ||
| inAppBrowser && | ||
| source === VideoBlockSource.youTube ? ( | ||
| <Box |
There was a problem hiding this comment.
I noticed the iframe player has different styling on the box, was this intentional?
4 participants
This fix follows @Ur-imazing’s suggestion.
It resolves the issue with Instagram WebView on Android without affecting other scenarios.
My previous push needs to be merged to prevent conflicts. #8749
Summary by CodeRabbit