App icon indicating copy to clipboard operation
App copied to clipboard

[$500] Chat - Attachment page keeps on loading infinitely.

Open kbecciv opened this issue 1 year ago • 86 comments
trafficstars

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.34.0 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Tap on a report
  3. Tap plus icon near compose
  4. Tap Add attachment
  5. Take a picture using camera and send it
  6. Turn off mobile data
  7. Open the attachment

Expected Result:

Attachment page must show the image and must not load infinitely.

Actual Result:

Attachment page keeps on loading infinitely.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • [x] Android: Native
  • [x] Android: mWeb Chrome
  • [ ] iOS: Native
  • [ ] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/6fe79ab0-0e8f-42ee-8d14-a2811d39de75

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019f92fc532944b2e5
  • Upwork Job ID: 1752760607717834752
  • Last Price Increase: 2024-03-14
  • Automatic offers:
    • situchan | Reviewer | 0
    • FitseTLT | Contributor | 0

kbecciv avatar Jan 31 '24 18:01 kbecciv

Job added to Upwork: https://www.upwork.com/jobs/~019f92fc532944b2e5

melvin-bot[bot] avatar Jan 31 '24 18:01 melvin-bot[bot]

Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] avatar Jan 31 '24 18:01 melvin-bot[bot]

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

melvin-bot[bot] avatar Jan 31 '24 18:01 melvin-bot[bot]

@kbecciv Please check recent changes. I can't send attachments to reproduce this issue.

Yokomon avatar Jan 31 '24 20:01 Yokomon

Proposal

Hi there,

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

Clearly define the nature of "fallbackSource" in AttachmentView to address the current issue.

What is the root cause of that problem?

The problem arises from the undefined nature of "fallbackSource" in AttachmentView.

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

Implement a mechanism to display informative error messages to users when the "fallbackSource" is undefined. This enhancement aims to provide clarity in the event of an error, thus improving the overall user experience.

What alternative solutions did you explore? (Optional)

Uploading images to memory and subsequently hashing them after a successful upload. This approach ensures that the "fallbackSource" is always defined.


FYI example:

  1. Discord displays an Error
  2. Telegram display hires quality images from storage

Amoralchik avatar Feb 01 '24 01:02 Amoralchik

📣 @Amoralchik! 📣 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. Screen Shot 2022-11-16 at 4 42 54 PM Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

melvin-bot[bot] avatar Feb 01 '24 01:02 melvin-bot[bot]

@kbecciv Please check recent changes. I can't send attachments to reproduce this issue.

I believe you can reproduce the issue on any device by following these steps

  1. Upload any image
  2. Immediately disconnect your internet connection after success
  3. open the image

This results in an infinite loading state.

FYI: I am using "version": "1.4.34-1".

Amoralchik avatar Feb 01 '24 01:02 Amoralchik

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/vitaliip17

Amoralchik avatar Feb 01 '24 16:02 Amoralchik

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

melvin-bot[bot] avatar Feb 01 '24 16:02 melvin-bot[bot]

Could you review @situchan?

lschurr avatar Feb 05 '24 16:02 lschurr

@Amoralchik thanks for the proposal. The root cause doesn't make sense to me. It's incorrect.

situchan avatar Feb 05 '24 16:02 situchan

We're waiting proposals

situchan avatar Feb 05 '24 16:02 situchan

@situchan Let me try once again.

I propose adding a setBothSourceError function and incorporating it into the onError handler of AttachmentViewImage. This function will set bothSourceError to true if both source and fallbackSource return errors. https://github.com/Expensify/App/blob/554276f05e0165c6705e019729c050a462176d6f/src/components/Attachments/AttachmentView/index.js#L171-L190

       onError={() => {
            setImageError(true);
            if (imageError) setBothSourceError(true);
        }}

After this, we can add a check for allSourceError in the if (!bothSourceError || isImage || (file && Str.isImage(file.name))) condition, and then include:

    else if (bothSourceError) return (
        <Text style={[styles.textStrong, styles.flexShrink1, styles.breakAll, styles.flexWrap, styles.mw100]}>
            Unexpected error, please try again later
        </Text>
    );

This way, if both source and fallbackSource are absent in ONYX or return errors, we can display an error message instead of getting stuck in an infinite loading loop.

Amoralchik avatar Feb 05 '24 22:02 Amoralchik

📣 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] avatar Feb 07 '24 16:02 melvin-bot[bot]

Could you review this again @situchan?

lschurr avatar Feb 07 '24 20:02 lschurr

@Amoralchik sorry but your latest comment still doesn't explain the root cause correctly. For proposals to be accepted, both root cause and solution should be correctly provided.

Still looking for proposals

situchan avatar Feb 12 '24 15:02 situchan

@lschurr @situchan 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] avatar Feb 14 '24 15:02 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] avatar Feb 14 '24 16:02 melvin-bot[bot]

Still looking for proposals.

lschurr avatar Feb 14 '24 16:02 lschurr

Same

situchan avatar Feb 19 '24 15:02 situchan

@lschurr @situchan this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] avatar Feb 21 '24 15:02 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] avatar Feb 21 '24 16:02 melvin-bot[bot]

@situchan - Should we raise the bounty here or start a conversation in Slack?

lschurr avatar Feb 21 '24 16:02 lschurr

Thoughts on this one @situchan?

lschurr avatar Feb 26 '24 15:02 lschurr

@lschurr what do you think about the expected behavior?

Chat image is thumbnail, Attachment modal image is full size so it's expected not to show when offline. (no way to get full size image from server when offline)

If the current behavior (infinite loading) is bug, what should we show?

situchan avatar Feb 26 '24 15:02 situchan

cc: @Expensify/design ^

situchan avatar Feb 27 '24 13:02 situchan

Yeah, I would think that if you go offline, the attachment modal would not show an infinite spinner but rather some kind of small "you're offline" icon + message or something.

shawnborton avatar Feb 27 '24 13:02 shawnborton

Curious if the other designers agree though!

shawnborton avatar Feb 27 '24 13:02 shawnborton

I agree the infinite loading spinner isn't great, but I think this is a dupe and been around for a long time.

The real solution for this is better support for image caching, isn't it? So while you're offline, you'd see a local cached version of the attachment. That's being worked on elsewhere, starting with here. @situchan let me know if I'm missing something though!

trjExpensify avatar Feb 27 '24 13:02 trjExpensify

@trjExpensify this is not image caching problem. Please check my comment.

situchan avatar Feb 27 '24 14:02 situchan