[$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 [email protected] 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 https://github.com/Expensify/App/pull/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
• [x] iOS: Native
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/e8e78082-db88-4536-8013-61950f4018a2

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-09
• Automatic offers:
• jrstanley | Contributor | 102669083

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

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

[melvin-bot[bot]]

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

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

https://github.com/Expensify/App/assets/9059945/25d0b6e2-6add-4494-a28c-21ca42d35843

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

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/VideoPlayer/BaseVideoPlayer.tsx#L132-L137

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.

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/VideoPlayer/BaseVideoPlayer.tsx#L364-L367

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

https://github.com/Expensify/App/commit/17cb6084bc61c3ac0a9e1831ad69a4e6e40f54d4#diff-e8a3241440502ed6c70db5702f3f432fd79c24f1bf2d16c4a0a44ced176f51a5L309-L317 - note the removal of the isOffline condition when displaying the AttachmentOfflineIndicator in this change

https://github.com/Expensify/App/commit/2f89b25b60996bd287817ca952a4c4b57373a9cd#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

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/VideoPlayer/BaseVideoPlayer.tsx#L137

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.

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/VideoPlayer/BaseVideoPlayer.tsx#L365

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.

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/VideoPlayer/BaseVideoPlayer.tsx#L364-L365

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 && .... https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/VideoPlayer/BaseVideoPlayer.tsx#L365

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

[iwiznia]

Whenever we can show the image, we should show it. The offline indicator should only be shown if we can't show the actual image.

[melvin-bot[bot]]

@iwiznia @eVoloshchak @kadiealexander 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!

[melvin-bot[bot]]

@iwiznia, @eVoloshchak, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

[jrstanley]

Proposal

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

I believe there are two or three distinct elements to the issue here.

This proposal focusses on the first element listed below. Please consider this proposal in combination with the proposal linked to in the second element listed below.

1. When offline a VideoPlayerThumbnail can appear blank. Instead an offline indicator should be displayed. This proposal focuses on this issue.

2. When offline a video attachment can appear blank or with a loading spinner. Instead an offline indicator should be displayed. Please see my proposal above for this element which has been C+ reviewed. I link to this proposal to avoid repetition.

3. There have been comments in the issue around expectations of thumbnails for images and videos being cached when offline. I am not attempting to address this here, and would suggest - if possible and necessary - we investigate this separately?

What is the root cause of that problem?

The VideoPlayerThumbnail component does not contain any logic to display an offline indicator if, for whatever reason, the thumbnail image cannot be displayed.

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

The existing ThumbnailImage component already contains logic to detect if the device is offline or if the image has otherwise failed to load. I recommend reusing this component. In VideoPlayerThumbnail we can replace the use of the Image component here with ThumbnailImage.

As highlighted by @CleverWolf1220, the play button in VideoPlayerThumbnail obscures the fallback icon in ThumbnailImage. I do not believe we should hide the play button, as it may be the case that the thumbnail image has failed to load but the video itself is playable. This is also consistent with images which, even when the device is offline or the thumbnail has failed to load, retain their click handler to allow the attachment to be viewed in full. In other words, in absence of a thumbnail we don't prevent the user from trying to access the 'full' content.

To address this visually, I would suggest displaying a larger fallback icon in ThumbnailImage e.g. fallbackIconSize={variables.iconSizeUltraLarge}, such that it is displayed larger than the play button. The default fallback icon for ThumbnailImage is Expensicons.Gallery; it would be nice if we had a generic Video icon (which I don't believe exists at the moment?).

What alternative solutions did you explore? (Optional)

I considered removing the Play icon altogether from VideoPlayerThumbnail when the device is offline (or the thumbnail otherwise couldn't be loaded). I avoided this as it is not in keeping with the behaviour of other attachments when their thumbnail does not load. It is not given that because the thumbnail cannot load that the content itself cannot load. The play icon is also the way in which videos are distinguished from images. However, if we were to follow this approach then we would need to look at using ImageWithSizeCalculation rather than ThumbnailImage, as the latter does not expose whether or not an image has failed to load.

I also considered avoiding the reuse of existing components and implementing all of the required logic into VideoPlayerThumbnail. Unless there are factors I am not aware of, this does not seem like the logical approach.

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

@iwiznia, @eVoloshchak, @kadiealexander Still overdue 6 days?! Let's take care of this!

[eVoloshchak]

There have been comments in the issue around expectations of thumbnails for images and videos being cached when offline. I am not attempting to address this here, and would suggest - if possible and necessary - we investigate this separately?

@jrstanley, this is already done (not sure about video thumbnails, but definitely regular images)

I do not believe we should hide the play button, as it may be the case that the thumbnail image has failed to load but the video itself is playable. This is also consistent with images which, even when the device is offline or the thumbnail has failed to load, retain their click handler to allow the attachment to be viewed in full. In other words, in absence of a thumbnail we don't prevent the user from trying to access the 'full' content.

The play icon is also the way in which videos are distinguished from images.

I agree with both points. We might need to consult with the design team on this, a standard player UI on top of the offline indicator might look weird. But we definitely need to retain a play button of some sort even when offline and video is not possible to play, to let users easily distinguish between videos and other attachments

@jrstanley's proposal looks good to me 🎀👀🎀 C+ reviewed!

[melvin-bot[bot]]

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

[mountiny]

I think I was assigned only because Ionatan has been ooo that day. So going to leave this to you.

@iwiznia the proposal was chosen by C+ so feel free to check it out

[melvin-bot[bot]]

📣 @jrstanley 🎉 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 📖

[iwiznia]

Proposal looks good.

Agree with this:

There have been comments in the issue around expectations of thumbnails for images and videos being cached when offline. I am not attempting to address this here, and would suggest - if possible and necessary - we investigate this separately?

[melvin-bot[bot]]

@jrstanley @iwiznia @eVoloshchak @kadiealexander this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

@jrstanley, @iwiznia, @eVoloshchak, @kadiealexander Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[jrstanley]

I am happy to take the issue on. I've just become eligible for payment via. Expensify and so I am waiting to find out the process, i.e. do I accept the job via. UpWork as normal?