[HOLD for payment 2024-04-15] [$500] iOS - Video - User needs to tap on play button twice for the video to restart

[lanitochka17]

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.44-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4345661
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

Pre-requisite: the user must be logged in

1. Go to any chat
2. Send a video
3. Tap on the video preview
4. Tap on the full screen button
5. Wait for the video to end
6. Tap on the play button

Expected Result:

The video should be restarted after tapping on the play button one time

Actual Result:

The user needs to tap on the play button twice for the video to restart

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

Add any screenshot/video evidence

Bug6393487_1708996827803.Rpreplay_Final1708988256.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b0749f5a1ec15cd0
• Upwork Job ID: 1762300460726202368
• Last Price Increase: 2024-02-27

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01b0749f5a1ec15cd0

[melvin-bot]

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

[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]

Auto-assign attempt failed, all eligible assignees are OOO.

[lanitochka17]

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

[youssef-lr]

cc @francoisl

[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.

[eucool]

Proposal

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

User needs to tap on play button twice for the video to restart

What is the root cause of that problem?

The condition !isCurrentlyURLSet being true when it's not expected:

App/src/components/VideoPlayer/BaseVideoPlayer.js

Lines 65 to 76 in 174a032

	const togglePlayCurrentVideo = useCallback(() => {
	videoResumeTryNumber.current = 0;
	if (!isCurrentlyURLSet) {
	updateCurrentlyPlayingURL(url);
	} else if (isPlaying) {
	pauseVideo();
	} else {
	playVideo();
	}
	}, [isCurrentlyURLSet, isPlaying, pauseVideo, playVideo, updateCurrentlyPlayingURL, url]);

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

Always update the updateCurrentlyPlayingURL

const togglePlayCurrentVideo = useCallback(() => {
    videoResumeTryNumber.current = 0;

    // Always update the currently playing URL
    updateCurrentlyPlayingURL(url);

    // If the video is playing, pause it
    if (isPlaying) {
        pauseVideo();
    } else {
        // If the video is not playing, play it from the beginning
        playVideo();
    }
}, [isPlaying, pauseVideo, playVideo, updateCurrentlyPlayingURL, url]);

What alternative solutions did you explore? (Optional)

N/A

[mananjadhav]

Looking at PRs today, will check this tomorrow.

[ShridharGoel]

Proposal

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

User needs to tap on play button twice for the video to restart on iOS / Video does not replay in full screen mode.

What is the root cause of that problem?

isPlaying parameter in shouldReplayVideo is being passed as e.isPlaying || false and then the method checks for !isPlaying && e.isPlaying which ends up being !e.isPlaying && e.isPlaying which will always be false.

App/src/components/VideoPlayer/BaseVideoPlayer.js

Lines 99 to 119 in d4cacf7

	const handlePlaybackStatusUpdate = useCallback(
	(e) => {
	const isVideoPlaying = e.isPlaying || false;
	const currentDuration = e.durationMillis || videoDuration * 1000;
	const currentPositon = e.positionMillis || 0;
	if (shouldReplayVideo(e, isVideoPlaying, currentDuration, currentPositon)) {
	videoPlayerRef.current.setStatusAsync({positionMillis: 0, shouldPlay: true});
	}
	preventPausingWhenExitingFullscreen(isVideoPlaying);
	setIsPlaying(isVideoPlaying);
	setIsLoading(!e.isLoaded || Number.isNaN(e.durationMillis)); // when video is ready to display duration is not NaN
	setIsBuffering(e.isBuffering || false);
	setDuration(currentDuration);
	setPosition(currentPositon);
	onPlaybackStatusUpdate(e);
	},
	[onPlaybackStatusUpdate, preventPausingWhenExitingFullscreen, videoDuration],
	);

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

- if (shouldReplayVideo(e, isVideoPlaying, currentDuration, currentPositon)) {
+ if (shouldReplayVideo(e, isPlaying, currentDuration, currentPositon)) { 
      videoPlayerRef.current.setStatusAsync({positionMillis: 0, shouldPlay: true});
  }

isPlaying should be passed to shouldReplayVideo instead of isVideoPlaying.

Screen.Recording.2024-03-11.at.3.23.45.PM.mov

[mananjadhav]

yeah I think e.isPlaying check is unnecessary. Good to use @ShridharGoel 's proposal.

But before we formally accept, I want to check with @tienifr @jjcoffee It was added in this PR. Do you think it would be break anything if we remove e.isPlaying check.

🎀 👀 🎀 C+ reviewed.

[melvin-bot]

Triggered auto assignment to @cead22, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[mananjadhav]

@tienifr @jjcoffee Quick bump on the previous comment? I've also DMed @jjcoffee to take a look.

[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.60-13 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix video replay issue in iOS #38419

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

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

• @mananjadhav requires payment through NewDot Manual Requests
• @ShridharGoel 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:

• [@mananjadhav] The PR that introduced the bug has been identified. Link to the PR:
• [@mananjadhav] 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:
• [@mananjadhav] 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:
• [@mananjadhav] Determine if we should create a regression test for this bug.
• [@mananjadhav] 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.
• [] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mananjadhav]

I've posted a comment on the offending PR.

I am not sure if there exists any test for video replay. If it doesn't then we don't need to add a new one. @cead22 Can you please assign a BZ member for the next steps?

This is ready for payout.

[cead22]

I am not sure if there exists any test for video replay. If it doesn't then we don't need to add a new one

Can you ask if we have one in expensify-open-source? If we don't, that's a good reason to add one imo

[melvin-bot]

Issue is ready for payment but no BZ is assigned. @MitchExpensify you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @mananjadhav owed $500 via NewDot
• ROLE: @ShridharGoel paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@MitchExpensify)

• 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/1762300460726202368/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[MitchExpensify]

Invited you for Upwork payment here https://www.upwork.com/jobs/~0184c7f8e7c24fbbc1 @ShridharGoel

[JmillsExpensify]

$500 approved for @mananjadhav.

[cead22]

On hold for payment, not overdue

[ShridharGoel]

@MitchExpensify I've accepted the offer, thanks.

[melvin-bot]

@cead22, @mananjadhav, @ShridharGoel, @MitchExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MitchExpensify]

Paid and contract ended