Video player

[Skalakid]

Details

Fixed Issues

$ https://github.com/Expensify/App/issues/20471 PROPOSAL: https://github.com/Expensify/App/issues/20471

Tests

• [x] Verify that no errors appear in the JS console

Offline tests

QA Steps

• [x] Verify that no errors appear in the JS console

PR Author Checklist

• [x] I linked the correct issue in the ### Fixed Issues section above
• [x] I wrote clear testing steps that cover the changes made in this PR
  • [x] I added steps for local testing in the Tests section
  • [x] I added steps for the expected offline behavior in the Offline steps section
  • [x] I added steps for Staging and/or Production testing in the QA steps section
  • [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • [x] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• [x] I included screenshots or videos for tests on all platforms
• [x] I ran the tests on all platforms & verified they passed on:
  • [x] Android: Native
  • [x] Android: mWeb Chrome
  • [x] iOS: Native
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop
• [x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [x] I followed proper code patterns (see Reviewing the code)
  • [x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • [x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • [x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • [x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • [x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • [x] I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • [x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I followed the guidelines as stated in the Review Guidelines
• [x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• [x] I verified that if a function's arguments changed that all usages have also been updated correctly
• [x] If a new component is created I verified that:
  • [x] A similar component doesn't exist in the codebase
  • [x] All props are defined accurately and each prop has a /** comment above it */
  • [x] The file is named correctly
  • [x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [x] The only data being stored in the state is data necessary for rendering and nothing else
  • [x] If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
    • [x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • [x] All JSX used for rendering exists in the render method
  • [x] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• [x] If any new file was added I verified that:
  • [x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))
• [x] If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• [x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• [x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• [x] If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
• [x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• [x] If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• [x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

android native.webm

Android: mWeb Chrome

android web.webm

iOS: Native

https://github.com/Expensify/App/assets/39538890/1a183542-05b4-4ee4-bc00-40119eb8061f

iOS: mWeb Safari

https://github.com/Expensify/App/assets/39538890/9134498d-1fc9-4bd1-b92e-209156e79b78

MacOS: Chrome / Safari

https://github.com/Expensify/App/assets/39538890/5c7813bb-1a79-48a5-85d8-eb49a7378b0f

MacOS: Desktop

https://github.com/Expensify/App/assets/39538890/9863d37b-aa53-4aa2-93c8-953ed71b85a0

[melvin-bot[bot]]

@akinwale @francoisl One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[github-actions[bot]]

:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube:

Android :robot:	iOS :apple:
❌ FAILED ❌	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/30829/index.html
The QR code can't be generated, because the android build failed
Desktop :computer:	Web :spider_web:
❌ FAILED ❌	❌ FAILED ❌
The QR code can't be generated, because the Desktop build failed	The QR code can't be generated, because the web build failed

---

:eyes: View the workflow run that generated this build :eyes:

[dannymcclain]

@francoisl I do not have strong feelings—if we have established patterns elsewhere for this type of thing let's use them!

[akinwale]

I get the following error when I try to upload a video attachment to test with. The error doesn't happen on the main branch, however. Any ideas?

Uncaught TypeError: expensify_common_lib_str__WEBPACK_IMPORTED_MODULE_3__.default.isVideo is not a function
    at AttachmentView (index.js:93:20)
    at renderWithHooks (react-dom.development.js:16175:18)
    at mountIndeterminateComponent (react-dom.development.js:20908:13)
    at beginWork (react-dom.development.js:22411:16)
    at HTMLUnknownElement.callCallback (react-dom.development.js:4161:14)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:4210:16)
    at invokeGuardedCallback (react-dom.development.js:4274:31)
    at beginWork$1 (react-dom.development.js:27400:7)
    at performUnitOfWork (react-dom.development.js:26508:12)
    at workLoopSync (react-dom.development.js:26417:5)

[francoisl]

Getting the same expensify_common_lib_str__WEBPACK_IMPORTED_MODULE_3__.default.isVideo is not a function error with a crash when I try to expand an existing video that's playing.

https://github.com/Expensify/App/assets/2229301/fed781bb-f95c-4cca-9515-be847cf6fc68

[kowczarz]

Hey, sorry for being offline I had a sick leave from Wednesday to Friday and I wasn't able to get to the computer. I've fixedt these all things you've mentioned.

[akinwale]

@kowczarz Did you get a chance to look into this? https://github.com/Expensify/App/pull/30829#issuecomment-1919352731

[kowczarz]

@akinwale yes, it should be fixed in https://github.com/Expensify/App/pull/30829/commits/0fb774da9121831725bbe3ddfb69de09e9703769

[kowczarz]

BTW we are aware of that the PR breaks no-new-js-files, but we decided to skip it and solve it as a followup

[github-actions[bot]]

:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube:

Android :robot:	iOS :apple:
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/30829/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/30829/index.html
Desktop :computer:	Web :spider_web:
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/30829/NewExpensify.dmg	https://30829.pr-testing.expensify.com

---

:eyes: View the workflow run that generated this build :eyes:

[francoisl]

It's getting pretty good on web and desktop!

I tested Android on my physical device with the ad-hoc build, 3 things:

• the app crashes if you try to change the position of the video with the progress bar
• when you select a video to upload, the preview screen doesn't show the video player
• (minor) when the video finishes and you play the ▶️ button again, it doesn't restart playing the video from the start; which it does on web/desktop.

I'm still unable to run the Android build on my dev environment so I couldn't get logs of the crash unfortunately.

https://github.com/Expensify/App/assets/2229301/ea0f2629-2bc4-49b1-9152-f8d5d3a1d0fd

Going to test iOS next.

[francoisl]

Also getting the crash on iOS, this time with the cause of the error since I can run it locally. The stacktrace starts from the call to progressBarInteraction(event); in the Gesture.Pan().onBegin() event handler, if that helps.

ERROR  ReanimatedError: [Reanimated] Tried to synchronously call a non-worklet function on the UI thread.
See `https://docs.swmansion.com/react-native-reanimated/docs/guides/troubleshooting#tried-to-synchronously-call-a-non-worklet-function-on-the-ui-thread` for more details., js engine: reanimated

https://github.com/Expensify/App/assets/2229301/7b8fc134-79fc-41d9-a07f-922e388cfe92

Also, the thumbnails don't seem to load on iOS.

[kowczarz]

@akinwale bump

[kowczarz]

@francoisl fixed ✅

[github-actions[bot]]

:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube:

Android :robot:	iOS :apple:
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/30829/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/30829/index.html
Desktop :computer:	Web :spider_web:
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/30829/NewExpensify.dmg	https://30829.pr-testing.expensify.com

---

:eyes: View the workflow run that generated this build :eyes:

[akinwale]

@akinwale bump

Thanks for the bump. I'll take another look tomorrow.

[akinwale]

@kowczarz Looking great! Some notes and changes.

• On mobile platforms (native and mweb), we can hide the volume control and set the player volume to max (not enitrely sure how expo-av handles this or if it's all automatic). The expectation here is that users can adjust the volume using their device's hardware volume buttons. This is observable on a number of popular mobile video players such as YouTube, Netflix, Prime Video, Twitch and others as well.
• On iOS native and mWeb, when I switch to fullscreen and then exit fullscreen mode while playing, the video pauses.
• It makes sense to have a loading indicator when the video is loading which happens upon displaying the attachment view. As can be seen in the video I included here, after clicking on the video, the duration sits on 00:00/00:00 for a noticeable while before the video starts playing.

https://github.com/Expensify/App/assets/4160319/f5555a86-8f02-4b47-b8b6-5fa5585f5fa0

Probably not in scope for this PR, but nice-to-haves

• Inline playback in the chat report on mobile.
• Auto-switch to fullscreen when the device is rotated to the landscape orientation.
• Picture-in-picture (PIP) mode.

[kowczarz]

On mobile platforms (native and mweb), we can hide the volume control and set the player volume to max (not enitrely sure how expo-av handles this or if it's all automatic). The expectation here is that users can adjust the volume using their device's hardware volume buttons. This is observable on a number of popular mobile video players such as YouTube, Netflix, Prime Video, Twitch and others as well.

On mobile platforms it has only mute/unmute toggle feature, but the volume level is controlled by the system.

[kowczarz]

Inline playback in the chat report on mobile.

It was discussed here and there was decision not to do it.

Auto-switch to fullscreen when the device is rotated to the landscape orientation.

Good point, we can consider implementing it in the Video Player V2.

Picture-in-picture (PIP) mode

This is planned in Video Player V2.

[kowczarz]

@akinwale I've addressed all your notes, feel free to recheck it.

[akinwale]

I'm seeing this warning when I try to play a video on native. Everything else looks good.

[kowczarz]

@francoisl Oh, I missed the issue with not loading attachment preview on android. That's pretty weird, because it should work in the way it's implemented right now. I will contact with expo maintainers about it.

[kowczarz]

While preparing a repro of the issue with not loading attachment preview on android I spotted that it's a bug caused by react-native-image-picker, with expo-image-picker everything works fine. It seems RN-image-picker saves the video in a place of the memory the expo-av can not read and it's the root the problem. I will try to figure out a quick fix tomorrow, but I suppose the expo-image-picker is the way to go.

[kowczarz]

@akinwale the prop types warning is fixed

[github-actions[bot]]

:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube:

Android :robot:	iOS :apple:
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/30829/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/30829/index.html
Desktop :computer:	Web :spider_web:
❌ FAILED ❌	https://30829.pr-testing.expensify.com
The QR code can't be generated, because the Desktop build failed

---

:eyes: View the workflow run that generated this build :eyes:

[github-actions[bot]]

:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube:

Android :robot:	iOS :apple:
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/30829/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/30829/index.html
Desktop :computer:	Web :spider_web:
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/30829/NewExpensify.dmg	https://30829.pr-testing.expensify.com

---

:eyes: View the workflow run that generated this build :eyes:

[kowczarz]

I will be OOO for the following week, but @Skalakid will take care of this task again.

[akinwale]

@Skalakid When I switch to fullscreen mode and then exit fullscreen on iOS Safari mWeb while playing, the video pauses. This was fixed for iOS native which I have just tested, but it's still happening on mWeb.

https://github.com/Expensify/App/assets/4160319/dff0f1b5-b8c5-44af-8721-5f112d7133ee

[akinwale]

@Skalakid I am also seeing a weird issue on Android Chrome. The first time I switch to fullscreen, it only shows a black screen and then automatically exits fullscreen mode and restarts the video. Subsequently, fullscreen switching works fine.

https://github.com/Expensify/App/assets/4160319/deabecf2-39a2-4f13-8001-54052cad800d

[github-actions[bot]]

:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube:

Android :robot:	iOS :apple:
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/30829/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/30829/index.html
Desktop :computer:	Web :spider_web:
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/30829/NewExpensify.dmg	https://30829.pr-testing.expensify.com

---

:eyes: View the workflow run that generated this build :eyes: