-
Notifications
You must be signed in to change notification settings - Fork 14
Up/nes 1194 youtube videos fail to play on android when opened via #8759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add unit tests for the changes made to this component. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,9 +1,10 @@ | ||||||||||||||||||||||||||
| import Box from '@mui/material/Box' | ||||||||||||||||||||||||||
| import { styled } from '@mui/material/styles' | ||||||||||||||||||||||||||
| import { CSSProperties, ReactElement, useEffect, useRef } from 'react' | ||||||||||||||||||||||||||
| import { CSSProperties, ReactElement, useEffect, useRef, useState } from 'react' | ||||||||||||||||||||||||||
| import videojs from 'video.js' | ||||||||||||||||||||||||||
| import Player from 'video.js/dist/types/player' | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import { isInstagramAndroidWebView } from '@core/shared/ui/deviceUtils' | ||||||||||||||||||||||||||
| import { defaultBackgroundVideoJsOptions } from '@core/shared/ui/defaultVideoJsOptions' | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||
|
|
@@ -36,6 +37,11 @@ export function BackgroundVideo({ | |||||||||||||||||||||||||
| const videoRef = useRef<HTMLVideoElement>(null) | ||||||||||||||||||||||||||
| const playerRef = useRef<Player | null>(null) | ||||||||||||||||||||||||||
| const isYouTube = source === VideoBlockSource.youTube | ||||||||||||||||||||||||||
| const [inAppBrowser, setInAppBrowser] = useState(false) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||
| setInAppBrowser(isInstagramAndroidWebView()) | ||||||||||||||||||||||||||
| }, []) | ||||||||||||||||||||||||||
|
Comment on lines
+40
to
+44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Race condition: video.js player is initialized before the iframe path takes over, leaving it orphaned and All
Consequences:
The 🛠️ Suggested fixReplace the - 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>
)
}
Also applies to: 47-68 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Initiate Video | ||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||
|
|
@@ -136,6 +142,26 @@ export function BackgroundVideo({ | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (inAppBrowser && isYouTube && videoId != null) { | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets rename for clarity, maybe something like this.
Suggested change
Also make the same change for |
||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||
| <Box | ||||||||||||||||||||||||||
| height="100%" | ||||||||||||||||||||||||||
| width="100%" | ||||||||||||||||||||||||||
| minHeight="-webkit-fill-available" | ||||||||||||||||||||||||||
| overflow="hidden" | ||||||||||||||||||||||||||
| position="absolute" | ||||||||||||||||||||||||||
| 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`} | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: The YouTube IFrame embed
Related (API-call) behavior that’s often confused with
Sources: 🏁 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 When The fix should conditionally include the 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 |
||||||||||||||||||||||||||
| allow="accelerometer; autoplay; encrypted-media" | ||||||||||||||||||||||||||
| style={{ width: '100%', height: '100%', border: 'none', pointerEvents: 'none' }} | ||||||||||||||||||||||||||
| title="Background video" | ||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||
|
Comment on lines
+155
to
+160
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The YouTube embed 🛠️ 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| </Box> | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||
| <Box | ||||||||||||||||||||||||||
| height="100%" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add unit tests for the changes made to this component. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ import { | |
| } from 'react' | ||
| import { use100vh } from 'react-div-100vh' | ||
|
|
||
| import { isInstagramAndroidWebView } from '@core/shared/ui/deviceUtils' | ||
| import { NextImage } from '@core/shared/ui/NextImage' | ||
|
|
||
| import { | ||
|
|
@@ -74,6 +75,11 @@ export function Video({ | |
| const [player, setPlayer] = useState<VideoJsPlayer>() | ||
| const [showPoster, setShowPoster] = useState(true) | ||
| const [activeStep, setActiveStep] = useState(false) | ||
| const [inAppBrowser, setInAppBrowser] = useState(false) | ||
|
|
||
| useEffect(() => { | ||
| setInAppBrowser(isInstagramAndroidWebView()) | ||
| }, []) | ||
|
Comment on lines
+78
to
+82
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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
Fix: Make 🤖 Prompt for AI Agents |
||
|
|
||
| const { blockHistory } = useBlocks() | ||
| const { variant, journey } = useJourney() | ||
|
|
@@ -220,7 +226,24 @@ export function Video({ | |
| /> | ||
| )} | ||
|
|
||
| {videoId != null ? ( | ||
| {videoId != null && inAppBrowser && source === VideoBlockSource.youTube ? ( | ||
| <Box | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed the iframe player has different styling on the box, was this intentional? |
||
| 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 ? ( | ||
| <> | ||
| <Box | ||
| height={{ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| export { | ||
| hasTouchScreen, | ||
| isInstagramAndroidWebView, | ||
| isIPhone, | ||
| isMobile, | ||
| isIOS, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.