[HOLD for payment 2024-04-09] [HOLD for payment 2024-03-13] [$500] Video - Play&Pause buttons don't function in preview mode if expanded the video before

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.43-0
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation:

Action Performed:

Prerequisites: The user should be logged in and a video exist in the chat

1. Go to prerequisite chat
2. Play&Pause the video
3. Tap on the expand button on the video preview
4. Play&Pause the video
5. Exit from the attachment view via clicking X button top right
6. Try to Play&Pause the video in preview mode

Expected Result:

Play&Pause buttons function correctly

Actual Result:

Play&Pause buttons don't function in preview mode and console error occurs

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6385395_1708412002465.2024-02-20_09-46-30.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013093a3e364e94fed
• Upwork Job ID: 1759909332125061120
• Last Price Increase: 2024-02-20

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013093a3e364e94fed

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ishpaul777 (External)

[melvin-bot]

Triggered auto assignment to @miljakljajic (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @lakchote (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[izarutskaya]

We think that this bug might be related to #vip-vsb
CC @quinthar

[lakchote]

This looks related to #30829

cc @francoisl @Skalakid @akinwale

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The play/pause button doesn't work after expanding and closing the video modal.

What is the root cause of that problem?

From the video, we can see there is a console error. The error indicates that the video player is not ready yet and it's because the video player element/ref doesn't exist anymore.

We have a useEffect here that is responsible for setting the video player ref.

App/src/components/VideoPlayer/BaseVideoPlayer.js

Lines 139 to 143 in 7430c5f

	useEffect(() => {
	if (shouldUseSharedVideoElement || url !== currentlyPlayingURL) {
	return;
	}
	shareVideoPlayerElements(videoPlayerRef.current, videoPlayerElementParentRef.current, videoPlayerElementRef.current);

Let's say we play a video A from the video preview. The video player ref is first set to the video preview A. Then, we press the expand button to see the video in the attachment modal. Currently, the video player ref is updated to the video player in the attachment modal (this is wrong). After we close the attachment modal, the video player doesn't exist anymore, so it always throws an error.

However, the expected result is, when we expand the video to view it in the attachment modal, the ref shouldn't be updated because we early return if shouldUseSharedVideoElement is true.

App/src/components/VideoPlayer/BaseVideoPlayer.js

Lines 139 to 143 in 7430c5f

	useEffect(() => {
	if (shouldUseSharedVideoElement || url !== currentlyPlayingURL) {
	return;
	}
	shareVideoPlayerElements(videoPlayerRef.current, videoPlayerElementParentRef.current, videoPlayerElementRef.current);

And shouldUseSharedVideoElement is true if it's used in the carousel.

App/src/components/Attachments/AttachmentView/index.js

Line 208 in 7430c5f

shouldUseSharedVideoElement={isUsedInCarousel}

shouldUseSharedVideoElement basically shares the video player between components. So, the video player state between video preview and video in the carousel.

However, isUsedInCarousel is always false because we never pass it.

What changes do you think we should make in order to solve the problem?

Pass isUsedInCarousel from CarouselItem.

App/src/components/Attachments/AttachmentCarousel/CarouselItem.js

Lines 102 to 111 in 7430c5f

	<AttachmentView
	source={item.source}
	file={item.file}
	isAuthTokenRequired={item.isAuthTokenRequired}
	onPress={onPress}
	transactionID={item.transactionID}
	isHovered={isModalHovered}
	isFocused={isFocused}
	optionalVideoDuration={item.duration}
	/>

[ishpaul777]

@bernhardoj's proposal looks good and solution tests well!

🎀 👀 🎀 C+ reviewed

Screen.Recording.2024-02-20.at.8.35.43.PM.mov

[melvin-bot]

Current assignee @lakchote is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[DylanDylann]

The solution for this issue is the same this one and @tienifr proposed this issue before. I think we should handle this issue in #36634

@lakchote @ishpaul777 We can put this issue on hold for #36634

[ishpaul777]

this is a deploy blocker, if @tienifr is available it would be best to handle this issue urgently as the fix is straightforward

[ishpaul777]

By the way, i have this fix included 3 days ago in my PR cb32fe7 as it was required there if we are holding then we can hold for that

[melvin-bot]

Payment Summary

Upwork Job

• ROLE: @ishpaul777 paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@miljakljajic)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1759909332125061120/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[Skalakid]

@ishpaul777 Yes, I'm investigating the issue right now

[Skalakid]

@ishpaul777 @aimane-chnaif I created a follow-up draft PR with the fix for Play&Pause button that enables video sharing feature once again. On Monday I will add last changes and test them on all platforms, so I can give it to review.

The main problem that will be fixed in my PR is the fact that when entering full-screen mode, the whole app rerenders due to useWindowDimensions() hook that checks windowWidth and windowHeight. Because of that many bugs appear. In my PR I've added dimension locking, so every time user enters fullscreen mode app dimensions won't change.
Thanks to that, we will be able to use the shared video feature and also handle screen rotation on all native devices

[ishpaul777]

Not overdue, Waiting for the PR to be open for review

[Skalakid]

@ishpaul777 I opened the PR for review

[ishpaul777]

Thanks for the ping @Skalakid I totally missed that

[miljakljajic]

We aren't actually waiting for payment on this one, correct? Ping me when payment is due.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.58-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix video sharing #38407

If no regressions arise, payment will be issued on 2024-04-09. 🎊

For reference, here are some details about the assignees on this issue:

• @ishpaul777 requires payment (Needs manual offer from BZ)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ishpaul777] The PR that introduced the bug has been identified. Link to the PR:
• [@ishpaul777] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@ishpaul777] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@ishpaul777] Determine if we should create a regression test for this bug.
• [@ishpaul777] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@miljakljajic] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[ishpaul777]

[@ishpaul777] The PR that introduced the bug has been identified. Link to the PR: #30829
[@ishpaul777] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: #30829 (comment)
[@ishpaul777] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: not required
[@ishpaul777] Determine if we should create a regression test for this bug. - yes
[@ishpaul777] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression test Proposal

Prerequisites: The user should be logged in and a video exist in the chat

• Go to prerequisite chat
• Play&Pause the video, verify it works as expected
• Tap on the expand button on the video preview
• Play&Pause the video, verify it works as expected
• Exit from the attachment view via clicking X button top right
• Try to Play&Pause the video in preview mode, verify it works as expected
• Open Video in full screen mode, Try to Play&Pause the video in preview mode, verify it works as expected

Do we agree 👍 or 👎 ?

[miljakljajic]

Is payment for this issue docked by 50% for the regression?

[ishpaul777]

i dont think the regression was from the initial PR directly, the RC was much complex than initially expected, also the only a small part was reverted from the first PR so i feel there shouldn't a penalty in this case, @lakchote can you please help with evaluation here as assigned engineer

[melvin-bot]

Payment Summary

Upwork Job

• ROLE: @ishpaul777 paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@miljakljajic)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1759909332125061120/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[lakchote]

i dont think the regression was from the initial PR directly, the RC was much complex than initially expected, also the only a small part was reverted from the first PR so i feel there shouldn't a penalty in this case, @lakchote can you please help with evaluation here as assigned engineer

Even though the RC was complex to fix, this comes directly from the initial PR related to the video player and files concerned (BaseVideoPlayer for instance). This case should have been identified and fixed from the beginning, even from just a functional standpoint.

That's why, since it's a regression, I think 50% payment seems fair to me.

[ishpaul777]

OkY no problem 🙂 Thanks for the evaluation @lakchote

[miljakljajic]

Paid 250 via upwork to @ishpaul777 - thank you all