Closed kbecciv closed 3 months ago
Job added to Upwork: https://www.upwork.com/jobs/~019f92fc532944b2e5
Triggered auto assignment to @lschurr (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @situchan (External
)
@kbecciv Please check recent changes. I can't send attachments to reproduce this issue.
Hi there,
Clearly define the nature of "fallbackSource" in AttachmentView to address the current issue.
The problem arises from the undefined nature of "fallbackSource" in AttachmentView.
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.
Uploading images to memory and subsequently hashing them after a successful upload. This approach ensures that the "fallbackSource" is always defined.
FYI example:
📣 @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:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
@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
This results in an infinite loading state.
FYI: I am using "version": "1.4.34-1".
Contributor details Your Expensify account email: vitaliipivenwork@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/vitaliip17
✅ Contributor details stored successfully. Thank you for contributing to Expensify!
Could you review @situchan?
@Amoralchik thanks for the proposal. The root cause doesn't make sense to me. It's incorrect.
We're waiting proposals
@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.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Could you review this again @situchan?
@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
@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!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Still looking for proposals.
Same
@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!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@situchan - Should we raise the bounty here or start a conversation in Slack?
Thoughts on this one @situchan?
@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?
cc: @Expensify/design ^
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.
Curious if the other designers agree though!
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 this is not image caching problem. Please check my comment.
small thumbnail image might be cached but big image with original size isn't cached as user hasn't opened attachment modal yet.
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.
Honestly even if we just showed the Offline
icon the way we do for thumbnails, that would be better than an infinite spinner.
Agreed
@trjExpensify this is not image caching problem. Please check https://github.com/Expensify/App/issues/35502#issuecomment-1964484878.
Do you not agree this issue is the same as the issue created in Dec 2022, so it's not a "new" regression here?
small thumbnail image might be cached but big image with original size isn't cached as user hasn't opened attachment modal yet.
Oh, but in the video in the OP the user uploads a picture > goes offline > then tries to open it. If we had implemented better image caching, why wouldn't the original image be stored in the cache layer or whatever we decide to do?
A broader overview of series of improvements we're going to start chomping down on is here for ref.
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. Honestly even if we just showed the Offline icon the way we do for thumbnails, that would be better than an infinite spinner. I'm Agreed
@shawnborton @dannymcclain @dubielzyk-expensify ultimately I'm not passionate about going forward and doing this btw, I think it sounds reasonable. 👍
Oh, but in the video in the OP the user uploads a picture > goes offline > then tries to open it. If we had implemented better image caching, why wouldn't the original image be stored in the cache layer or whatever we decide to do?
If user already opened modal before going offline, then image cache problem like you said. But if user hasn't ever opened modal before going offline, big image (original size) is never fetched at all so cache doesn't make sense.
@lschurr @situchan this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:
Thanks!
Current assignee @situchan is eligible for the Internal assigner, not assigning anyone new.
Okay, well sounds like we're going to add an offline indicator either way. 👍
Where'd we end up on this one @situchan? What's the next step?
This is still awaiting proposals
Got it - Should this be External rather than Internal @situchan?
yes, External
Current assignee @situchan is eligible for the External assigner, not assigning anyone new.
When attachement is loading and user is offline we show loading indicator infinitely
We show loading indicator when the image is loading here https://github.com/Expensify/App/blob/a04481ffef2c60243c8c5680222a01a82e162916/src/components/ImageView/index.tsx#L246-L247 https://github.com/Expensify/App/blob/a04481ffef2c60243c8c5680222a01a82e162916/src/components/ImageView/index.tsx#L213-L214 So when the user is offline it will not finish loading until he gets back online so it will show loading indicator infinitely
We should only display it when the user is online
{isLoading && !isOffline && <FullscreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />}
And prepare a component that will be displayed when the attachement is loading but user offline. That component will depend on the UI the design team will give us but it should prompt the user is offline and the image will load as soon as they get back online or may be we can use offline blocking view such as this one https://github.com/Expensify/App/blob/a04481ffef2c60243c8c5680222a01a82e162916/src/components/BlockingViews/FullPageOfflineBlockingView.tsx#L15-L23
We might need to apply the same change for other component that display other types of attachements For video https://github.com/Expensify/App/blob/a04481ffef2c60243c8c5680222a01a82e162916/src/components/VideoPlayer/BaseVideoPlayer.js#L260-L261
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@situchan could you review the proposal?
yes, I will provide feedback on Monday
@situchan will update today
Could you please update @situchan?
I reviewed proposal. Will comment soon
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.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:
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?
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