[PAID] [$500] Chat - When opening the mp4 video that was just sent, console error appears

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

---

Found when validating PR : #37007

Version Number: v1.4.43-14
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:

1. Open the app
2. Login with any account
3. Start any new chat
4. Click on plus icon inside the compose box
5. Click on add attachment
6. Click on choose from gallery
7. Select an mp4 video
8. When the preview opens up play the video and manually scrub through the video using the progress bar
9. Send a video
10. Open the video after sending it

Expected Result:

No any error in console

Actual Result:

Console error appears

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

Bug6389704_1708701973864._____________2024-02-23___15.53.25-1.mp4

Bug6389704_1708701973856!Console_23.02.txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~015cdee1805b9b7baa
• Upwork Job ID: 1761051577601671168
• Last Price Increase: 2024-02-23
• Automatic offers:
• alitoshmatov | Reviewer | 0
• jeremy-croff | Contributor | 0

Issue Owner

Current Issue Owner: @strepanier03

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~015cdee1805b9b7baa

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @strepanier03 (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 @pecanoro (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

[ikevin127]

The error is coming from expo-av Video component function here:

  _performOperationAndHandleStatusAsync = async (
    operation: (tag: number) => Promise<AVPlaybackStatus>
  ): Promise<AVPlaybackStatus> => {
    const video = this._nativeRef.current;
    if (!video) {
      throw new Error(`Cannot complete operation because the Video component has not yet loaded`);
    }

    const handle = findNodeHandle(this._nativeRef.current)!;
    const status: AVPlaybackStatus = await operation(handle);
    this._handleNewStatus(status);
    return status;
  };

[pecanoro]

Ok, since this is just a console error and everything seems to be working just fine, I am going to remove the deploy blocker label.

[jeremy-croff]

Proposal

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

We get a console error when we play a video in the attachment preview. And then, play a video in the report.

What is the root cause of that problem?

The root cause of this bug is that the PlaybackContext.js is trying to pause the previous video from the modal because currentlyPlayingURL is changing. However, that video context has been destroyed, failing downstream in expo-av like @ikevin127 linked. Our PlaybackContext.js still has the stale video reference in currentVideoPlayerRef.current despite it not being loaded.

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

We currently do a good job cleaning up refs when we navigate between reports because there's a clean-up hook resetVideoPlayerData. This runs on [currentReportID] . This, however, requires the video to exist and doesn't run between attachments and reports, for example. The attachment flow is also unique because the blob was only in memory, and when we close the modal, we can no longer clean it up with resetVideoPlayerData.

So, how do we ensure that when the attachment video gets destroyed, its references are cleaned up?

A reliable lifecycle is:

1. In AttachmentsView, we can make sure to set currentVideoPlayerRef.current = null; when it unmounts;
2. Consequently, all the downstream methods in PlaybackContext won't execute, like pausing, on their lifecycles methods because they early return on currentVideoPlayerRef.current = null
3. In AttachmentsView, we can also leverage [isVideo] variable for executing this logic

This fixes the bug in OP. However, it will fix 2 more bugs that should be considered in all proposals:

1. when playing an existing video in the report and then uploading an attachment and scrubbing it, it throws a console error because of stale reference in trying to pause the blob( without requiring the play the new video)
2. When uploading a video, scrubbing it, and then hovering over the previous videos, it throws console errors because of volumeContext trying to reference the blob

What alternative solutions did you explore? (Optional)

1. explored managing and resetting the currentlyPlayingURL variable; however, it only seems like a partial solution.
2. I explored cleaning up the refs on PlaybackContext.js unmount; however, it is not a reliable lifecycle and breaks other flows

[jeremy-croff]

Tested my logic:

2024-02-25_14-21-18.mp4

[melvin-bot]

📣 @alitoshmatov 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @jeremy-croff 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

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

• fix(37142): unmount video ref when attachment closes #37386

If no regressions arise, payment will be issued on 2024-03-13. 🎊

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

• @alitoshmatov requires payment automatic offer (Reviewer)
• @jeremy-croff requires payment automatic offer (Contributor)

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

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

[strepanier03]

@jeremy-croff - I've paid you via Upwork and closed the contract.

@alitoshmatov - I'll keep an eye open for the checklist so post that as soon as you can and I'll finish payment.

[alitoshmatov]

• The PR that introduced the bug has been identified. Link to the PR: Related to this PR: Video player #30829
• 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: https://github.com/Expensify/App/pull/30829/files#r1527477209
• 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: N/a
• Determine if we should create a regression test for this bug.
  No need, since it is a console error issue, and existing regression testing would catch it again.

[alitoshmatov]

@strepanier03 Completed the checklist

[melvin-bot]

@pecanoro, @strepanier03, @alitoshmatov, @jeremy-croff Eep! 4 days overdue now. Issues have feelings too...

[strepanier03]

Paid @alitoshmatov and closed the contract. Thanks, everyone!