App: [HOLD for payment 2024-02-26][$500] [MEDIUM] Erroring thumbnail images try to reload infinitely
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.25-4 Reproducible in staging?: Yes Reproducible in production?: It’s worse on production, the app fails to load completely once I sign in. If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): lizzi@infinitered.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @lindboe Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1705427206739379?thread_ts=1705360862.915459&cid=C01GTK53T8Q
Action Performed:
- Have a request with a receipt image that produces a 404 when requested. (I am not sure how to reproduce this part outside of development, where you can give a thumbnail component an intentionally wrong URL.)
- Open the app in a web browser and open the devtools. Observe that the requests for the image that can’t be loaded are continuously made.
Expected Result:
After the request returns a 404, the requests should stop being made.
Actual Result:
Requests for the image are infinitely repeated.
Workaround:
If this issue is encountered in production in its current state, the app can’t be used at all, although I believe that particular issue may have been fixed by @cead22.
If this issue is encountered in staging, I don’t think this prevents usage.
In development, it causes a connection/file descriptor leak as webpack generates more and more connections it never clears. This eventually leads to the developer’s computer encountering errors and not working properly, including affecting internet access. (This must also be caused by a second issue with the app not clearing closed connections correctly).
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
In development:
https://github.com/Expensify/App/assets/5252268/701598d7-ee5c-45d6-9da8-4b60274683ad
Production screenshot:
Root cause and considerations for proposals:
Image, which handles the load request, is memoized based on whether the identity of thesourceprop stays the sameImageWithSizeCalculationis reconstructingsourceeach render because it’s creating the object in the prop. Pretty easy to fix with auseMemoinstead:
const source = useMemo(() => {
return {uri: url};
}, [url]);
This does NOT occur for the Image/index.native.js implementation.
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~010386784e89b6acc5
- Upwork Job ID: 1749005349524606976
- Last Price Increase: 2024-02-04
- Automatic offers:
- paultsimura | Contributor | 28145121
About this issue
- Original URL
- State: closed
- Created 5 months ago
- Comments: 58 (44 by maintainers)
Ok that sounds good to me!
@paultsimura someone more technical can probably guide you to those better than I can, but they are located in the
assets/imagesdirectory:Offline:
assets/images/offline-cloud.svgDocument:assets/images/document.svgReceipt:assets/images/receipt.svgImage: I could not find this one for reason, if anyone knows where it may be, let us know! Attaching the SVG file in case it’s not already in there for some reason: Image.svg.zipDang, I really like that! Nice and clean, and super simple.
@shawnborton @dubielzyk-expensify
I think I’m going to propose a potentially spicy take 🌶️
offlineicon for all placeholders that didn’t load because you’re offline.typeicons for all placeholders that didn’t load for whatever other reason.This keeps things super simple, no custom icon creations needed, and personally I feel like this still accomplishes the goal of letting users know “Something is supposed to be here, but it isn’t.” (We could honestly even just use the “type” icons for all situations—offline or not. I don’t think these need to be super duper literal. We just need to communicate to users that “There is a thing here, but it didn’t load at this time.”)
Super keen for y’all’s thoughts! 🌶️ 🌭 😈
May be we need Design team here to decide that.
Checklist
Deployed to prod: https://github.com/Expensify/App/pull/36214#issuecomment-1945393766
Hmm good question. If we want to maintain the ability for these icons to change colors as the theme changes, it makes sense to use the existing Expensicons svg-based icon system we have in product. So we just need to add styles to increase the size and color of them, but I think we should try that route first before providing new svg background images for these.
Seems like we’re all down for this treatment. Let me know if you all need any assets for this (file type, dimensions, etc.) and I can get them exported/uploaded for you.
I will note that those images are all using icons from our standard set of icons and using our standard product color variables, so if we can create them just using SVGs + code, I feel like that would be best since we’ll get automatic theme switching, more flexibility, and won’t have to deal with static image files, BUT I’ll defer to you more technical folks to let me know if that will/won’t work.
Proposal looks correct. I will do some testing on it and let you all know.
@jliexpensify I have described the easiest way to reproduce the issue at the moment:
https://github.com/Expensify/App/issues/34601#issuecomment-1902764996
@jliexpensify Sorry, I’m not a developer (I used to be a test contributor when the program was still open to testers). However, I’ve experienced that trying to reproduce issues can sometimes be impossible due to different conditions, so using some support tools can be very helpful. If you want to know more about how to use these tools to mimic scenarios like this one, I’m happy to help.
Once we can reproduce it, either in a normal way or by using the tool, I’m sure that we can fix it and verify the solution