Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-06-28] [$250] mWeb - Chat - Uploading an image while offline briefly shows an offline message in the preview #42657

Closed
1 of 6 tasks
kbecciv opened this issue May 27, 2024 · 38 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented May 27, 2024

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.76.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: https://expensify.testrail.io/index.php?/tests/view/4575560
Issue reported by: Applause - Internal Team

Action Performed:

Precondition: Go offline

  1. Go to https://staging.new.expensify.com/home
  2. Tap on a report
  3. Tap plus icon - upload an attachment image

Expected Result:

The image preview should be displayed even when uploading an attachment while offline.

Actual Result:

Uploading an image while offline briefly shows an offline message in the preview

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

Bug6493126_1716829499362.Screenrecorder-2024-05-27-21-18-51-168.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017a93bd4c637b4a77
  • Upwork Job ID: 1798155121003208704
  • Last Price Increase: 2024-06-05
  • Automatic offers:
    • allgandalf | Reviewer | 102625969
    • Krishna2323 | Contributor | 102625971
Issue OwnerCurrent Issue Owner: @anmurali
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 27, 2024
Copy link

melvin-bot bot commented May 27, 2024

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

@kbecciv
Copy link
Author

kbecciv commented May 27, 2024

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

@kbecciv
Copy link
Author

kbecciv commented May 27, 2024

@anmurali 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.

@Krishna2323
Copy link
Contributor

Krishna2323 commented May 27, 2024

Proposal

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

mWeb - Chat - Uploading an image while offline briefly shows an offline message in the preview

What is the root cause of that problem?

We use AttachmentOfflineIndicator component for local files also and isLoading is initially true.

{isLoading && <AttachmentOfflineIndicator />}

{isLoading && <AttachmentOfflineIndicator />}

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

We can check if the attachment is a local file or not and only show AttachmentOfflineIndicator when the attachment is not a local file. We can also use isPreview prop of AttachmentOfflineIndicator.

const isLocalFile = url.startsWith('blob:') || url.startsWith('file:');

We might also want to do the same in other components which uses AttachmentOfflineIndicator.

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label May 29, 2024
Copy link

melvin-bot bot commented May 31, 2024

@anmurali Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Jun 3, 2024

@anmurali 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@anmurali anmurali added the External Added to denote the issue can be worked on by a contributor label Jun 5, 2024
@melvin-bot melvin-bot bot changed the title mWeb - Chat - Uploading an image while offline briefly shows an offline message in the preview [$250] mWeb - Chat - Uploading an image while offline briefly shows an offline message in the preview Jun 5, 2024
Copy link

melvin-bot bot commented Jun 5, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 5, 2024
Copy link

melvin-bot bot commented Jun 5, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Jun 5, 2024
@ashuvssut
Copy link
Contributor

ashuvssut commented Jun 5, 2024

Proposal

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

While offline, when we add an local file attachment, the <AttachmentOfflineIndicator /> shows up

What is the root cause of that problem?

<AttachmentOfflineIndicator /> cannot distinguish whether the uploaded file is a local file or not, and it displays in both cases.

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

This <AttachmentOfflineIndicator /> component should accept an uri prop. Based on the uri, it will judge if it has to show the offline indicator or not

if (!isOffline || onCacheDelay) {
return null;
}

+ const isLocalFile = uri.startsWith('blob:') || uri.startsWith('file:');
+ if (isLocalFile || !isOffline || onCacheDelay) {
- if (!isOffline || onCacheDelay) { 

We have 5 different components who use this <AttachmentOfflineIndicator /> component. I have verified that each of those components can readily pass this new uri prop to <AttachmentOfflineIndicator />

Alternatively,

We can use the isPreview={isLocalFile} prop of <AttachmentOfflineIndicator /> component to disable this offline indicator rendering when isLocalFile is true. But by this we would have to write isLocalFile conditional across all the 5 components.

What alternative solutions did you explore? (Optional)

And easier solution could be to just change the rendering conditionals in ImageView component

That is,

  1. first create the conditional using the url prop of ImageView component:-

     const isLocalFile = url.startsWith('file:') || url.startsWith('blob:');
     const showOfflineIndicator = isLoading && !isLocalFile;
  2. Apply the conditional

    -     {isLoading && <AttachmentOfflineIndicator />}
    +     {showOfflineIndicator && <AttachmentOfflineIndicator />}
       </View>

Using this 2nd solution will only fix this issue for ImageView component. This issue will still persist with other components such as LightBox, ImageWithSizeCalculation, etc who use <AttachmentOfflineIndicator /> component

@dominictb
Copy link
Contributor

dominictb commented Jun 5, 2024

Proposal

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

Uploading an image while offline briefly shows an offline message in the preview

What is the root cause of that problem?

These 2 lines are the UX of showing offline indicator/loading indicator for images:

{((isLoading && !isOffline) || (!isLoading && zoomScale === 0)) && <FullscreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />}
{isLoading && <AttachmentOfflineIndicator />}

We can see that:

  • If isLoading is true, and the user is not offline, we show FullscreenLoadingIndicator because the image will load soon, the user should see the loading indicator to know that the image will show shortly
  • If isLoading is true, and the user is offline, the FullscreenLoadingIndicator will not show, instead AttachmentOfflineIndicator will show

When the image is not loaded, the user should see either the FullscreenLoadingIndicator or AttachmentOfflineIndicator, never a blank screen.

In AttachmentOfflineIndicator, we have this logic to wait a bit for the image to load in case it's cached/a local file

const timeout = setTimeout(() => setOnCacheDelay(false), 200);

If the image doesn't finish loading within that timeframe, and isOffline is true, the AttachmentOfflineIndicator will show.

So in case it's a large image, or the device's resources are limited (in case of Android Chrome), 200ms is not enough for the cached/local image to show, then we'll see offline indicator for some time before the image shows.

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

First, add a method to check if a file URI is a local file which will use same logic as in

const isLocalFile = typeof receiptPath === 'number' || receiptPath?.startsWith('blob:') || receiptPath?.startsWith('file:') || receiptPath?.startsWith('/');

const isFileLocal = (uri: string) => {
    return typeof uri === 'number' || uri?.startsWith('blob:') || uri?.startsWith('file:') || uri?.startsWith('/');
}

It's important to check the typeof uri === 'number' here as in native, if the source is directly passed by require(..., the uri will be a number assigned by the OS. It's also used to check the receipt too

const isLocalFile = typeof receiptPath === 'number' || receiptPath?.startsWith('blob:') || receiptPath?.startsWith('file:') || receiptPath?.startsWith('/');

While the image is loading and it's a local file, we should:

  • Not show the AttachmentOfflineIndicator, by updating
    In here, add:
const isLocalFile = isFileLocal(url);

Then update this to

{isLoading && !isLocalFile && <AttachmentOfflineIndicator />}
  • Show the FullscreenLoadingIndicator until the loading completes, large local files still need some time to load, it would be bad UX if the user sees blank page during this time. Ideal UX is, if the image is to be shown, a loading indicator should be there while it's loading (ie. local images in offline mode, online images in online mode)

Update this to

{((isLoading && (!isOffline || isLocalFile)) || (!isLoading && zoomScale === 0)) && <FullscreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />}

We need to fix the same for other places that use AttachmentOfflineIndicator and FullscreenLoadingIndicator.

The logic can be moved into AttachmentOfflineIndicator if needed.

What alternative solutions did you explore? (Optional)

An improvement is during the time of cacheDelay here, we should probably still show the loading indicator to avoid blank screens.

@allgandalf
Copy link
Contributor

Proposals from @Krishna2323 (here) as well as @dominictb (here) fixes the bug (mentioned in the GH issue)

Chronologically:

But @dominictb also found out one more bug in the same flow that we do not show the loading indicator initially when the user uploads attachment when offline:

The extra bug reported by @dominictb is:

Video of online and offline bug
Online Offline
Screen.Recording.2024-06-06.at.1.04.12.AM.mov
Screen.Recording.2024-06-06.at.1.06.00.AM.mov

In online mode, we show the loading indicator, but in offline mode we do not show the loading indicator.

So we have two choices here:

  • Stick to the original reported bug over this GH issue and solve that, @Krishna2323's proposal was the first in this case so we can go with their solution and perhaps create another GH for the additional bug found.

  • As the additional bug was found in the same flow we fix both, in this case @dominictb's proposal was the one to find the additional bug so we can go with their proposal instead.

I have tried to be fair to both the contributors as the reported bug was something extra and not part of original GH issue, but i leave the final decision to the internal engineer for final assignment.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Jun 5, 2024

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

@allgandalf
Copy link
Contributor

allgandalf commented Jun 5, 2024

@arosiclair , an additional bug was found by a contributor, i have created a detailed summary of the issue here, please assign accordingly, thanks 🥂

@dominictb
Copy link
Contributor

dominictb commented Jun 6, 2024

Thanks for the kind feedback @allgandalf !

To further clarify, my proposal is the only one that does this for checking local file

It's important to check the typeof uri === 'number' here as in native, if the source is directly passed by require(..., the uri will be a number assigned by the OS. It's also used to check the receipt too

const isLocalFile = typeof receiptPath === 'number' || receiptPath?.startsWith('blob:') || receiptPath?.startsWith('file:') || receiptPath?.startsWith('/');

Also, regarding the FullscreenLoadingIndicator fix, I would not consider it an additional bug, when looking at

{((isLoading && !isOffline) || (!isLoading && zoomScale === 0)) && <FullscreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />}
{isLoading && <AttachmentOfflineIndicator />}

We can see that the condition for FullscreenLoadingIndicator and AttachmentOfflineIndicator always contradicts each other when the image is loading. That's because we want to avoid a case that the user will see blank screen while the image is not loaded yet, and when image is loading, user will see either the AttachmentOfflineIndicator so she knows she's offline, or FullscreenLoadingIndicator so she knows she can expect an image to be loaded soon.

If we change the condition of AttachmentOfflineIndicator but not FullscreenLoadingIndicator, it will be a regression because there'll be a period of blank screen where both AttachmentOfflineIndicator and FullscreenLoadingIndicator do not show, no offline indicator, no loading indicator, that is especially severe and noticeable if the image is large and the wait time is long (ie > 10mb).

I'd be curious to hear what @arosiclair thinks!

@Krishna2323
Copy link
Contributor

Even if we don't consider that an additional bug, it would have been easily caught in the PR phase. There is no additional effort in making it work since it just uses the same variable. In my opinion, that shouldn't be considered a point for selecting a proposal, as most of the time there are minor issues in the selected proposal that are caught and fixed during the PR. This one is also an easy fix and would have definitely been caught while recording the video.

cc: @arosiclair @allgandalf

@allgandalf
Copy link
Contributor

@dominictb @Krishna2323 i have put up a pretty detailed summary here, so let’s refrain from commenting further, so let’s wait for the internal engineers decision 🙏

@dominictb
Copy link
Contributor

dominictb commented Jun 6, 2024

This one is also an easy fix and would have definitely been caught while recording the video.

@Krishna2323 I would disagree, it's only rather noticeable when you try to upload a large video. The bug here itself is straight forward to fix so it's important to make sure we don't leave any regression/bugs behind. A "sure" is better than a "maybe it will be fixed in the PR, maybe not".

As far as I can see, proposals with regressions/bugs left behind shouldn't be considered "complete proposal" and selected.

It took me a good amount of time to figure it out so I don't agree it's easy either (it would've been easy for me too if I've seen it in someone else's proposal 😄 ).

Let's wait for @arosiclair to decide 🙏

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jun 6, 2024

it's only rather noticeable when you try to upload a large video.

Nope, it can be easily seen while uploading an image. You can see it here. Also, this is component for images, it won't be displayed for videos.

It took me a good amount of time to figure it out so I don't agree it's easy either (it would've been easy for me too if I've seen it in someone else's proposal 😄 ).

I don't think it would have taken more than a minute of few for me because,

  1. I already suggested to create a variable isLocalFile before your proposal.
  2. I suggested to replace {isLoading && <AttachmentOfflineIndicator />} with {isLoading && !isLocalFile && <AttachmentOfflineIndicator />}

And similarly, we need to also modify the isLoading check here. Thats doesn't seem a lot of work to me 😅

Rather, it would have been very easy for you to find a solution since I already had the fix for the original issue, and the same approach is also applied to the loader.

As far as I can see, proposals with regressions/bugs left behind shouldn't be considered "complete proposal" and selected.

You can't call it a regression, it would have been only found after applying the fix. it isn't visible in the issue screenshots section nor it is mentioned anywhere.

@dominictb
Copy link
Contributor

@arosiclair Sorry for the back-and-forth here, here's the C+ comment for your review!

@arosiclair
Copy link
Contributor

Thanks for the breakdown @allgandalf

But @dominictb also found out one more bug in the same flow that we do not show the loading indicator initially when the user uploads attachment when offline

This doesn't really seem like much of a bug to me just a very minor quirk. Let's ignore this. @Krishna2323's proposal looks pretty straight forward so let's go with that.

@Krishna2323
Copy link
Contributor

@allgandalf, PR ready for review.

@dominictb
Copy link
Contributor

dominictb commented Jun 12, 2024

This doesn't really seem like much of a bug to me just a very minor quirk. Let's ignore this. @Krishna2323's proposal looks pretty straight forward so let's go with that.

@arosiclair The reason for not selecting my proposal is that the loading bug in the flow I fixed is minor and will be ignored. However, it's being fixed in the PR using the changes I suggested.

According to @allgandalf

As the additional bug was found in the same flow we fix both, in this case @dominictb's #42657 (comment) was the one to find the additional bug so we can go with their proposal instead.

Could you help double check if the decision here is fair when we select one proposal but uses another proposal in the PR?

I wouldn't insist to assign me as the PR is in progress, but I think splitting bounty would be more fair if we're to fix the additional bug that is only addressed in my proposal

Thanks

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jun 21, 2024
@melvin-bot melvin-bot bot changed the title [$250] mWeb - Chat - Uploading an image while offline briefly shows an offline message in the preview [HOLD for payment 2024-06-28] [$250] mWeb - Chat - Uploading an image while offline briefly shows an offline message in the preview Jun 21, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 21, 2024
Copy link

melvin-bot bot commented Jun 21, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Jun 21, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.0-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-06-28. 🎊

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

Copy link

melvin-bot bot commented Jun 21, 2024

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:

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 28, 2024
@Krishna2323
Copy link
Contributor

@anmurali, bump for payments :)

@dominictb
Copy link
Contributor

dominictb commented Jun 29, 2024

@arosiclair What's your thought on this?

@mallenexpensify curious for your thought too on how you think we should handle this case.

Thanks

@melvin-bot melvin-bot bot added the Overdue label Jul 1, 2024
@arosiclair
Copy link
Contributor

@dominictb Krishna posted a better proposal earlier than yours. Your proposal included a fix for another bug which I deemed not important. This is why I hired Krishna. Whether or not the bug you reported ends up getting fixed here or anywhere is not important. You do not "own" that bug and we never posted a job to fix it.

I'd appreciate it if you would let this go. The repeated back in forth in these comments is not really doing you any favors and this energy would be better spent on another proposal instead. Thank you 🙏

Copy link

melvin-bot bot commented Jul 1, 2024

@arosiclair, @anmurali, @Krishna2323, @allgandalf Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Copy link

melvin-bot bot commented Jul 3, 2024

@arosiclair, @anmurali, @Krishna2323, @allgandalf Eep! 4 days overdue now. Issues have feelings too...

@allgandalf
Copy link
Contributor

Can you issue payments for this please @anmurali :)

Copy link

melvin-bot bot commented Jul 5, 2024

@arosiclair, @anmurali, @Krishna2323, @allgandalf 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@anmurali
Copy link

anmurali commented Jul 8, 2024

@Krishna2323 has been paid but @allgandalf still needs to complete the BZ checklist before payment is due.

@allgandalf
Copy link
Contributor

  • The PR that introduced the bug has been identified. Link to the PR: Fix - Attachment page keeps on loading infinitely. #39290

  • 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/39290/files#r1672243491

  • 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

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

Regression Test Proposal

  1. Go offline
  2. Open a report
  3. Tap plus icon - upload an attachment image

Verify offline message is not shown

Do we agree 👍 or 👎

@anmurali
Copy link

@allgandalf - paid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Archived in project
Development

No branches or pull requests

7 participants