[$250] Onboarding- After finishing "onboarding" and switching to DM, an error appears in the console

[kbecciv]

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: v1.4.63-0
Reproducible in staging?: y
Reproducible in production?: n
Issue reported by: Applause - Internal Team
Issue found when executing PR: #40328

Action Performed:

Prerequisites:
There must be at least one conversation in the account. Enable the console.

Step:

1. Open https://staging.new.expensify.com/onboarding
2. Fill in the required data
3. Click the "Continue" button
4. Select "Track business spend for taxes"
5. Click the "Continue" button
6. Navigate to any existing conversation

Expected Result:

After completing "onboarding" and switching to DM, no errors should appear in the console

Actual Result:

After finishing "onboarding" and switching to DM, an error appears in the console

Workaround:

n/a

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

Bug6453504_1713439454477.Recording__55.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017d01cb735c28a1cd
• Upwork Job ID: 1780977474718523392
• Last Price Increase: 2024-05-02

Issue Owner

Current Issue Owner: @mananjadhav

[melvin-bot]

Triggered auto assignment to @tgolen (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ 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.

[kbecciv]

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

[melvin-bot]

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[tgolen]

This is not a blocker. Removing the label and sending this to external to investigate a fix.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017d01cb735c28a1cd

[melvin-bot]

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

[skyweb331]

Proposal

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

After playing video and changing the room, resetting the VideoPlayerData causes error.

What is the root cause of that problem?

According to the following codes, when the room changes, it will reset the VideoPlayerData by running the following code.

App/src/components/VideoPlayerContexts/PlaybackContext.tsx

Lines 85 to 90 in 26c9eba

	useEffect(() => {
	if (!currentReportID) {
	return;
	}
	resetVideoPlayerData();
	}, [currentReportID, resetVideoPlayerData]);

App/src/components/VideoPlayerContexts/PlaybackContext.tsx

Lines 75 to 83 in 26c9eba

	const resetVideoPlayerData = useCallback(() => {
	videoResumeTryNumber.current = 0;
	stopVideo();
	setCurrentlyPlayingURL(null);
	setSharedElement(null);
	setOriginalParent(null);
	currentVideoPlayerRef.current = null;
	unloadVideo();
	}, [stopVideo, unloadVideo]);

And stopVideo function is here

App/src/components/VideoPlayerContexts/PlaybackContext.tsx

Lines 24 to 26 in 26c9eba

	const stopVideo = useCallback(() => {
	currentVideoPlayerRef.current?.setStatusAsync?.({shouldPlay: false, positionMillis: 0});
	}, [currentVideoPlayerRef]);

Problem is stopVideo and unloadVideo are async functions, so currentVideoPlayerRef.current should not be set as null before releasing functions are finished.

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

After stopVideo and unloadVideo are all finished, currentVideoPlayerRef.current should be set as null

skyweb331@9df23dc

What alternative solutions did you explore? (Optional)

N/A

[bernhardoj]

Proposal

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

The console error shows after closing the onboarding video and switch to another report.

What is the root cause of that problem?

When the video player is mounted, currentVideoPlayerRef will be set to the onboarding video ref. When we close it and switch to another report, we are trying to stop the video (and other things).

App/src/components/VideoPlayerContexts/PlaybackContext.tsx

Lines 85 to 90 in 48f64d0

	useEffect(() => {
	if (!currentReportID) {
	return;
	}
	resetVideoPlayerData();
	}, [currentReportID, resetVideoPlayerData]);

App/src/components/VideoPlayerContexts/PlaybackContext.tsx

Lines 75 to 77 in 48f64d0

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

Because the currentVideoPlayerRef still hold the onboarding video player ref that is already destroyed, we got the console error. I previously fix this console error in my PR here, but looks like the code was removed

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

We need to clear the currentVideoPlayerRef when the video player is unmounted and shouldUseSharedVideoElement is false, basically the same solution I did before.

useEffect(() => {
    shouldUseSharedVideoElementRef.current = shouldUseSharedVideoElement;
}, [shouldUseSharedVideoElement]);

useEffect(
    () => () => {
        if (shouldUseSharedVideoElementRef.current) {
            return;
        }
        // If it's not a shared video player, clear the video player ref.
        currentVideoPlayerRef.current = null;
    },
    [currentVideoPlayerRef],
);

We can even call resetVideoPlayerData so it clears the currentlyPlayingURL so we can remove this code.

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Lines 206 to 218 in 48f64d0

	const isCurrentlyURLSetRef = useRef<boolean>();
	isCurrentlyURLSetRef.current = isCurrentlyURLSet;
	useEffect(
	() => () => {
	if (!isCurrentlyURLSetRef.current) {
	return;
	}
	setCurrentlyPlayingURL(null);
	},
	[setCurrentlyPlayingURL],
	);

[melvin-bot]

@tgolen, @mananjadhav, @greg-schroeder Eep! 4 days overdue now. Issues have feelings too...

[mananjadhav]

Will review this today.

[tgolen]

@mananjadhav Can you please take a look at this one soon?

[mananjadhav]

I looked through the proposals yesterday. I'll test them today and confirm if one of them is good to go.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[mananjadhav]

I was checking this and I am unable to reproduce this on staging as well as latest main.

console-error-dm.mov

[bernhardoj]

It's already fixed in #40132

[skyweb331]

This means, this bug is duplicated??? :-(

[melvin-bot]

@tgolen, @mananjadhav, @greg-schroeder Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@tgolen, @mananjadhav, @greg-schroeder Eep! 4 days overdue now. Issues have feelings too...

[mananjadhav]

@kbecciv Can you please confirm if this is still reproducible.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

@tgolen @mananjadhav @greg-schroeder this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[tgolen]

It sounds like this can be closed since it's already fixed. Please reopen if anyone is able to reproduce it.