-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[HOLD for Payment 2024-05-15][$500] mWeb - Chat - Infinite loading on video preview before sending video #39078
Comments
Triggered auto assignment to @alexpensify ( |
@alexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
We think that this bug might be related to vip-vsp |
Job added to Upwork: https://www.upwork.com/jobs/~015c647e39b68ccf23 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @paultsimura ( |
Waiting for proposals here |
No update, we need proposals here. |
I have spent some time looking into this but can't reproduce this in dev server, this is making it harder to debug. iOS: 17.2 Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-04-01.at.18.42.43.mp4Edit: When I do manage to reproduce, only in staging + my actual physical device, I get the following error |
@cleverjam thanks for the investigation – indeed, it's only reproducible on physical devices. Just a side note: you can test your local dev env from a physical device as well, not only staging/production. Or do you only want to work with a simulator? |
@paultsimura I have not been able to access local dev server from my physical device, it seems the dev server binds to 127.0.0.1 only? I may have missed the instructions to make this work (I am new here), but I followed this README, would you mind sharing instructions to access local dev from physical device? |
ProposalPlease re-state the problem that we are trying to solve in this issueThe video preview keeps loading infinitely on iOS: mWeb. What is the root cause of that problem?Note This can only be reproduced on real iOS devices, and it happens on all browser (Safari, Chrome, etc) since all iOS browsers use the WebKit engine. The root cause comes from this code line: App/src/components/VideoPlayer/BaseVideoPlayer.js Lines 64 to 65 in 35b4d1b
The problem is that we're appending Appending This is most likely a WebKit browser engine limitation which does not allow us to skip the first milisecond of the video programatically via What changes do you think we should make in order to solve the problem?Based on #39078 (comment) findings related strictly to iOS: mWeb (WebKit-based browser engine) and CME decision from #39078 (comment) -> the updated solution is:
Once this is done, we won't get the blob error and the infinite loading spinner anymore. Given the known browser-limitation, the downside with this is that while it would fix the infinite loading issue / console log error, it will still leave us with a transparent preview of the video until the user will interact with it (see iOS: mWeb video below). All other platforms work as expected: no errors and the preview showing correctly using the existing VideosiOS: mWeb (before / after)
|
@cleverjam Try ngrok, once you instelled it and have the Expensify web app running locally, run the following command in a terminal: This will create a tunnel to your web app running locally (given it's running on port 8082) and ngrok will give you a link of form: Once you do that you can debug / monitor your iOS device console logs from Safari (desktop) if the devices are connected via USB. |
thanks for sharing @ikevin127 - very helpful! |
This comment was marked as outdated.
This comment was marked as outdated.
@paultsimura when you get a chance, can you review the recent proposals? Thanks! |
@ikevin127 thanks for your proposal. Unfortunately, the platform-specific solutions are highly discouraged. Also, could you please share a testing branch of your current solution? 🙇 |
Asked the author of the original PR as well: #36832 (comment) |
@amyevans @alexpensify @ikevin127 @ishpaul777 this issue is now 4 weeks old, please consider:
Thanks! |
@ishpaul777 Indeed, after extensive testing I can confirm too that none of my proposed solutions fixes the issue, specifically only on physical iOS: mWeb devices where all the browsers use WebKit engine by design. This must be due to the browser limitation / privacy concerns - where a video cannot be played / seeked without actual user interaction (button click or press), hence not allowing us to extract a frame to use for preview using the above suggested methods. I have a lead on how to possibly tackle this issue, I just have to test it first then I'll write-up a formal proposal, again. |
Yeah...it's not possible (IMO) to have this issue fixed on iOS: mWeb on real physical devices (all WebKit based browsers) from the FE side. Why do I say this ? The lead mentioned above was that I added an external The issue is that we cannot pass local files to the video player dirrectly for security reasons, unless we're transforming them in a URL blob via Given this, we most likely won't want to send local files to BE simply to generate a preview so this leaves us with the following possible solution that will do the following only on mWeb WebKit based browsers:
This still won't show a video preview since it's literally impossible (IMO) since on mWeb WebKit based browsers we're not allowed to set This is how it will look on mWeb WebKit based browsers: iOS: mWeb SafariRPReplay_Final1714183179.MP4Please let me know if this is an acceptable solution in your opinion for this specific edge case in order to at least unblock the user. If so, I'd update my proposal to remove the |
@amyevans What's your suggestion, should we wait for ideal solution that fixes the thumbnail issue or just move without showing preview to prevent infinite loading for now (its def. not ideal but unblocks the user) |
Let's just move forward with not showing a preview in mWeb. It's not great, but it's better than an infinite loader, and given this is a relatively niche flow I'm okay with accepting the limitation |
Cool! lets move this forward then, @ikevin127 Can you please update your proposal |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Updated proposal
Note With the updated solution -> the behaviour will now align on both iOS: mWeb physical devices and Simulator as in both will have transparent preview even though the cc @ishpaul777 |
Thanks for updating the proposal! Let us know once the PR is updated as well |
PR #39755 was updated according to proposal and is ready for review! 🎉 Note Sorry, I thought I'm supposed to wait for @ishpaul777 's approval of the updated proposal before moving on to updating the PR. cc @amyevans |
not overdue, PR is up for final review |
Thank you for the update @ishpaul777! |
cc @alexpensify |
not overdue, waiting for pay date |
Ok, Upwork is up to date for tomorrow. I'm adding the note to remember why there are two C+ here: #39078 (comment) Now, automation should kick in with the summaries tomorrow. |
Payouts due: 2024-05-15
Upwork job is here. We are treating this one as a |
Closing - everyone has been paid via Upwork and I closed the job there. |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.57-2
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4456414
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause - Internal Team
Action Performed:
Pre-requisite: the user must be logged in
Expected Result:
A video preview should be displayed before sending the video
Actual Result:
The video preview page keeps loading infinitely
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6428802_1711551855450.Gvyy1795_1_.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: