[Ready for payment][$250] Chat - Offline indicator and message isn't visible for video attachment and preview

[lanitochka17]

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.74.1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Issue found when executing PR #39290

Action Performed:

1. Log in with any account
2. Navigate to a 1:1 chat
3. Upload a video attachment
4. Turn off network connection
5. Navigate to another chat
6. Open the chat with the video attachment
7. Open the video

Expected Result:

Offline indicator and message should be visible

Actual Result:

Offline indicator and message isn't visible for video attachment and preview

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

Bug6482731_1715869066812.Jlyn5276.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013921e50ea628bfac
• Upwork Job ID: 1792390967573037056
• Last Price Increase: 2024-07-16
• Automatic offers:
• jrstanley | Contributor | 102669083

[melvin-bot]

Triggered auto assignment to @kadiealexander (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.

[lanitochka17]

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

[lanitochka17]

@kadiealexander 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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013921e50ea628bfac

[melvin-bot]

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

[melvin-bot]

📣 @CleverWolf1220! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[jrstanley]

I can replicate this on iOS mWeb Safari in production, but not in staging (i.e. I don't see the issue where #39290 is merged). I have not been able to check if this is something specific to iOS Native.

[eVoloshchak]

Can reproduce the issue on iOS Native

@CleverWolf1220, your proposal results in the following behavior:

Screen.Recording.2024-05-21.at.19.05.43.mov

We shouldn't hide thumbnails when offline, they're cached and currently visible when offline, let's keep it that way

[jrstanley]

Proposal

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

When a device is offline and a video attachment is viewed, the attachment area can appear either blank or with a loading spinner. The expectation is that an offline message should be displayed.

What is the root cause of that problem?

1. If the video status.isLoaded is false, then the isLoading state is set to false. They way in which isLoading is used appears to be closer to meaning has loading completed (isLoading = false), rather than is loading ongoing (isLoading = true).

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Lines 132 to 137 in e8ae3c5

	const handlePlaybackStatusUpdate = useCallback(
	(status: AVPlaybackStatus) => {
	if (!status.isLoaded) {
	preventPausingWhenExitingFullscreen(false);
	setIsPlaying(false);
	setIsLoading(false); // when video is ready to display duration is not NaN

2. The AttachmentOfflineIndicator will only display if isLoading state is true (see 1 above) and makes no reference to isOffline (see background below). When I am seeing the FullScreenLoadingIndicator whilst being offline, this is due to the isLoading state being false.

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Lines 364 to 367 in e8ae3c5

	{((isLoading && !isOffline) || isBuffering) && <FullScreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />}
	{isLoading && !isBuffering && <AttachmentOfflineIndicator isPreview={isPreview} />}
	{controlsStatus !== CONST.VIDEO_PLAYER.CONTROLS_STATUS.HIDE && !isLoading && (isPopoverVisible || isHovered || canUseTouchScreen) && (
	<VideoPlayerControls

Background:

PR #39290 introduced an AttachmentOfflineIndicator component. When this was implemented in BaseVideoPlayer the logic for displaying an offline indicator appears to have changed, please see (as examples):

17cb608#diff-e8a3241440502ed6c70db5702f3f432fd79c24f1bf2d16c4a0a44ced176f51a5L309-L317 - note the removal of the isOffline condition when displaying the AttachmentOfflineIndicator in this change

2f89b25#diff-e8a3241440502ed6c70db5702f3f432fd79c24f1bf2d16c4a0a44ced176f51a5 - note setIsLoading(false) is always called in this change when !status.isLoaded

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

1. Set this line to setIsLoading(true) - i.e. loading is ongoing/not completed/being attempted

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Line 137 in e8ae3c5

setIsLoading(false); // when video is ready to display duration is not NaN

2. Set the below line to {isLoading && (isOffline || !isBuffering) && <AttachmentOfflineIndicator isPreview={isPreview} />}. This is necessary because it is not always the case that isBuffering is true when the device isOffline.

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Line 365 in e8ae3c5

{isLoading && !isBuffering && <AttachmentOfflineIndicator isPreview={isPreview} />}

What alternative solutions did you explore? (Optional)

I explored changing only the conditions that result in AttachmentOfflineIndicator from being rendered, and avoid changing the state of setIsLoading. However, this seemed brittle and conflicting and resulted in more complex conditions. Looking at the history of changes (see background), it seems to me that the current setIsLoading(false) is a behaviour change and I assume the consequence is unintended.

[CleverWolf1220]

Proposal

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

Chat - Offline indicator and message isn't visible for video attachment and preview

What is the root cause of that problem?

Offline indicator is not visible in both video attachment and preview and since they have difference root causes we have to solve two problems here.

1. Attachment
   Video attachment is rendered in VideoPlayerThumbnail.
   But in VideoPlayerThumbnail we don't have logic to display offline indicator.

2. Preview
   Offline indicator is rendered in below part.

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Lines 364 to 365 in e8ae3c5

	{((isLoading && !isOffline) || isBuffering) && <FullScreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />}
	{isLoading && !isBuffering && <AttachmentOfflineIndicator isPreview={isPreview} />}

isLoading is set as false here when video load is failed.
So offline indicator will not be displayed when it fails to load video.

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

1. Attachment
   We have to add logic to display offline indicator in VideoPlayerThumbnail.
   First, render thumb nail using ThumbnailImage instead of current render like this part.
   Then, we have to hide play button when it is offline as offline icon overlaps with play button.

2. Preview
   To solve this problem, we have two ways.

First, we can set isLoading as true here when video load is failed.

Second, first solution will work correctly and uses state value that is already exist, but it's not meaningful enough.
So we can add state value isLoaded to store state whether video load fails or not.
Then we can update below line something like !isLoaded && !isBuffering && ....

App/src/components/VideoPlayer/BaseVideoPlayer.tsx

Line 365 in e8ae3c5

{isLoading && !isBuffering && <AttachmentOfflineIndicator isPreview={isPreview} />}

What alternative solutions did you explore? (Optional)

[eVoloshchak]

@jrstanley's proposal looks good to me!
We have to make sure to test this extra carefully, there are some interesting edge cases when using manual "force offline" option, please make sure to include this in the test cases

🎀👀🎀 C+ reviewed!

[melvin-bot]

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

[CleverWolf1220]

@eVoloshchak
Could you please let me know your thought about my proposal?

Offline indicator and message isn't visible for video attachment and preview

As it is in issue description, offline indicator is not visible for both video attachment and preview, and I think we have to solve those two problems.

[iwiznia]

@eVoloshchak can you check the comment above from @CleverWolf1220 ?

[melvin-bot]

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

[eVoloshchak]

As it is in issue description, offline indicator is not visible for both video attachment and preview, and I think we have to solve those two problems.

Good catch @CleverWolf1220
I think we have to align on the requirements first

Currently, if you're offline, the image preview is still shown in chat

But the video in the issue description demonstrates a different behavior, which indicates there were recent changes.

@iwiznia, @kadiealexander, what is the expected behavior in this case?
Should we show the image/video preview when offline or should we show AttachmentOfflineIndicator?

[kadiealexander]

Not overdue, @CleverWolf1220 is there anything remaining that is blocking you from working on the PR?

[CleverWolf1220]

Everything is clear now. I will raise PR soon.

[melvin-bot]

This issue has not been updated in over 15 days. @iwiznia, @eVoloshchak, @kadiealexander, @CleverWolf1220 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[eVoloshchak]

@kadiealexander, not sure why the automation has failed (the issue is correctly linked in PR description), but this was deployed to production 3 weeks ago, hence it's ready for payment

This is not a bug, but rather a feature request, so there is no checklist.
I think we can add a small regression test, this is very easy to test and is related to the flow users interact with regularly

Regression Test Proposal

1. Log in with any account
2. Navigate to a 1:1 chat
3. Upload a video attachment
4. Turn off the network connection
5. Navigate to another chat
6. Go back to the chat with the video attachment
7. Open the video
8. Offline indicator and message should be displayed

Do we agree 👍 or 👎

[eVoloshchak]

@kadiealexander, gentle bump on the above

[kadiealexander]

https://github.com/Expensify/Expensify/issues/429586

[kadiealexander]

Payouts due:

• Contributor: $250 @CleverWolf1220 via Upwork (please apply!)
• Contributor+: $250 @eVoloshchak via Manual Requests

Upwork job is here.

[CleverWolf1220]

Contributor details
Your Expensify account email: mathewlin1220@outlook.com
Upwork Profile Link: https://www.upwork.com/freelancers/~0121a7511f59b34d83

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[CleverWolf1220]

Hi, @kadiealexander
I've added contributor details.

[JmillsExpensify]

$250 approved for @eVoloshchak

[kadiealexander]

@CleverWolf1220 thank you! That will help us automatically send a contract for future jobs. For this one, can you please manually apply in Upwork?

[CleverWolf1220]

@kadiealexander
I've applied in Upwork.

[kadiealexander]

Contract sent!

[CleverWolf1220]

I've accepted offer!