Expo-av to expo-video migration#66793
Conversation
|
|
…-fork into Guccio163/expo-video
src/components/VideoPlayer/useHandleNativeVideoControls/index.ts
Outdated
Show resolved
Hide resolved
…-fork into Guccio163/expo-video
…-fork into Guccio163/expo-video
…-fork into Guccio163/expo-video
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
After reloading the page on a full video screen 2025-11-06.14.12.35.mov |
|
@Guccio163 What si the latest here? |
|
I'm investigating this bug, it seems like it's not a problem of not showing the loading indicator, but not playing the video when it's readyToPlay. The issue is really hard to test, because it's a race condition one, so to notice it I have to reload 10-20 times and it takes some time, but I'm contacting @JakubKorytko who wrote that code - we have to step cautiously not to break something else in the process. I believe that this is the last issue, because @ZhenjaHorbach didn't report anything else. |
|
@mountiny good news is that it is not a regression since it is reproducible on staging (still with expo-av of course), so we can potentially demote it to a follow-up and move on with the merge, WDYT? Below a full recording of trying to reproduce, issue at the end on 20th refresh so it's time-consuming, but reachable. I could fix it with @JakubKorytko after I'm done with the merge and potential reverts 💔 stagingRepro.mov |
…-fork into Guccio163/expo-video
mountiny
left a comment
There was a problem hiding this comment.
Thanks! Lets be on a look out for any sort of deploy blockers 🙌
| })); | ||
|
|
||
| const videoPlayerRef = useRef<VideoWithOnFullScreenUpdate | null>(null); | ||
| /* eslint-disable no-param-reassign */ |
There was a problem hiding this comment.
Could you please always add explanation of why the rule has to be disabled and there is no reasonable way to do it otherwise?
There was a problem hiding this comment.
It is a default way of initialising player in expo-video, should I write a comment right now?
There was a problem hiding this comment.
You can add it in a follow up.
|
We have done extensive testing on this PR, QA has gone through full regression testing and @ZhenjaHorbach @Guccio163 confirmed all the found issues been resolved. Hence I will go ahead and merge, there might be some new edge cases that popped up so revert is possible, but let's keep this on staging for a full regression testing and maybe we will be able to fix them with clinical precision |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.59-0 🚀
|
|
As the revert's incoming, we've decided with @mountiny to handle these reported issues at once and then merge them with a ready PR of the new branch. @blazejkustra also noticed that we didn't bump the podfile.lock, so it'll be just handled here then. If my comment is confirmed and the Busy week incoming, I'll keep you noticed as I'll be handling these issues 🛠️ |
|
The draft PR for the changes following the revert: #75270 |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.2.59-5 🚀
|

Explanation of Change
This PR introduces expo-video replacing expo-av Video usages.
Fixed Issues
$ #64846
PROPOSAL:
Tests
Video checklist introduced here below:
Checklist
/onboarding/welcome-videourl)Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
android.mWeb.mov
iOS: Native
ios.mov
iOS: mWeb Safari
iOS.mWeb.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov