[$500] Web - Unlimited API requests when image endpoint is giving 404

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open any report and attach an image.
2. Go to Chrome Dev Tools Network tab and disable cache & offline.
3. Open another report and reopen the report in which I attached the image.
4. Check if unlimited API requests are displayed in the console tab and image preview on the report enter an infinite re-render loop.

Expected Result:

The image component should not enter an infinite re-render loop.

Actual Result:

The image component should enter an infinite re-render loop.

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.65.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
Notes/Photos/Videos: Any additional supporting documentation

bug-report.mp4

Recording.4304.mp4

Expensify/Expensify Issue URL:
Issue reported by: @napster125
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694029320303739

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c7adabaca13f7f62
• Upwork Job ID: 1699856040881467392
• Last Price Increase: 2023-09-28

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[Pluto0104]

Proposal

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

Unlimited API requests when image endpoint is giving 404.

What is the root cause of that problem?

The root cause is that we don't have error handling.
If the image loads successfully, isLoadedRef.current will be true, so we shouldn't set isLoading to true. In this case, there won't be an error.
However, if isLoadedRef.current is null (indicating the image did not load), this condition relies on whether isLoading is true or false, leading to an infinite rerender.

App/src/components/ImageWithSizeCalculation.js

Lines 74 to 91 in b119f9a

	<Image
	style={[styles.w100, styles.h100]}
	source={{uri: props.url}}
	isAuthTokenRequired={props.isAuthTokenRequired}
	resizeMode={Image.resizeMode.cover}
	onLoadStart={() => {
	if (isLoadedRef.current || isLoading) {
	return;
	}
	setIsLoading(true);
	}}
	onLoadEnd={() => {
	setIsLoading(false);
	setIsImageCached(true);
	}}
	onError={onError}
	onLoad={imageLoadedSuccessfully}
	/>

When the image starts to load (onLoadStart), the logic in place checks if isLoading is false. If so, it sets isLoading to true. but during the process of the image fully loading (onLoadEnd), isLoading is then correctly set back to false.
Therefore this makes an infinite rerender. Each time the component re-renders, it triggers onLoadStart again. Since isLoading is now true, it toggles back to false, perpetuating the loop.

We set isLoadedRef.current to true in the onLoad function, but this function is not called when an error occurs. Instead, the onError function is triggered. Hence, isLoadedRef.current remains unset to true.

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

To resolve this problem, I suggest modifying the condition in onLoadEnd prop as follows:

if (!isLoadedRef.current) {
  return;
}
setIsLoading(false);
setIsImageCached(true);

This solution might prevent isLoading from being set to false when isLoadedRef.current is null, indicating that the image loading has failed.

What alternative solutions did you explore? (Optional)

I investigated various options, such as utilizing useMemo, useCallback, and employing React.memo for the Image component's props. However, most components using the Image component didn't require these functions and still functioned correctly. Therefore, I believe the suggested solution is more consistent than these alternatives.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01c7adabaca13f7f62

[melvin-bot]

Triggered auto assignment to @greg-schroeder (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[dukenv0307]

Proposal

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

The image component should enter an infinite re-render loop.

What is the root cause of that problem?

When the image starts loading, it will set isLoading to true here

When the loading ends, it will set isLoading to false here

When the isLoading changes, the ImageWithSizeCalculation component will rerender. Let's look at this line. Every time the ImageWithSizeCalculation rerenders, it will pass a new source object to the Image (although the URL didn't change at all). This will make the Image load again, and set isLoading to true, continuing the loop.

The reason why it doesn't happen when the Image is loaded successfully because when that happens, in onLoadStart, we'll not set the isLoading to true any more due to this condition. So in that case the loop only continues for a few hundred milliseconds (until the image is loaded successfully).

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

We can simply use useMemo to make sure the source of the Image only changes if the url actually changes, not every time the ImageWithSizeCalculation rerenders.

const source = useMemo(() => ({uri: props.url}), [props.url]);

And use that source here.

What alternative solutions did you explore? (Optional)

This unnecessary loading of the Image probably happens for other places where we use the Image as well, although it doesn't cause any noticeable bug yet. We can consider applying the same above fix to all those places as well.

[tienifr]

Proposal

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

Web - Unlimited API requests when image endpoint is giving 404

What is the root cause of that problem?

In

App/src/components/ImageWithSizeCalculation.js

Lines 79 to 88 in 7c9fc84

	onLoadStart={() => {
	if (isLoadedRef.current || isLoading) {
	return;
	}
	setIsLoading(true);
	}}
	onLoadEnd={() => {
	setIsLoading(false);
	setIsImageCached(true);
	}}

we set isLoading: true when onLoadStart, and false when onLoadEnd.

• isLoading is set to true when RNImage starts to fetch data
• The API is failed -> isLoading is set to false in onLoadEnd
• When isLoading change -> ImageWithSizeCalculation component will be re-render -> the imageLoadedSuccessfully and source will be created to the new one that makes the API call continuously (isLoadedRef.current is still false)

App/src/components/Image/index.js

Lines 33 to 42 in 7c9fc84

	useEffect(() => {
	// If an onLoad callback was specified then manually call it and pass
	// the natural image dimensions to match the native API
	if (onLoad == null) {
	return;
	}
	RNImage.getSize(source.uri, (width, height) => {
	onLoad({nativeEvent: {width, height}});
	});
	}, [onLoad, source]);

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

We should wrap source and imageLoadedSuccessfully in useMemo and useCallback, so they won't be re-created when isLoading change to false

In

App/src/components/Image/index.js

Line 33 in 7c9fc84

useEffect(() => {

We should use network.isOffline to allow re-load image when the network is on

[kaushiktd]

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

Unlimited API requests when image endpoint is giving 404

What is the root cause of that problem?

the issue, is that there might have been a situation where onLoadStart was being triggered multiple times, leading to the re-render loop.

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

Setting isLoading to false in this case prevents the component from entering the re-render loop.
the file link is below
https://github.com/Expensify/App/blob/main/src/components/ImageWithSizeCalculation.js#L74-L91

const {isOffline} = useNetwork();

   onLoadStart={() => {
                    if (isLoadedRef.current || isOffline) {
                        setIsLoading(false);
                    } else {
                        setIsLoading(true);
                    }
                }}

add this in line number 80. Setting isLoading to false in this case prevents the component from entering the re-render loop.

Video: https://drive.google.com/file/d/1Oia-tg6VXTaktknQ3vEV687jK189iJE9/view?usp=sharing

[slafortune]

Not sure why you got assigned as well @greg-schroeder, since I'm going on sabbatical I'll just remove myself.

[jNembhard]

Proposal

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

Unlimited API requests when image endpoint is giving a 404.

What is the root cause of that problem?

In the ImageWithSizeCalculation component, isImageCached is initialized with true and isLoading is initialized with false. Later on in the component logic, the states are set and checked to determine whether or not an image is cached resulting in shorter load times.

the useEffect for isLoading and isImageCached states may not behave as expected. The implementation checks if isLoading is false and isLoadedRef.current is true before considering if the image is cached. However, image loading events can be asynchronous, and this approach may not guarantee accurate results.

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

Here:

  const [isImageCached, setIsImageCached] = useState(true);
  const [isLoading, setIsLoading] = useState(false);

We can change the initial states of isImageCached to false and isLoading to true.

in the onError function, we can setIsLoading and setIsImageCached to false.

in the imageLoadedSuccessfully function, we can setIsLoading to false and setImageCached to true.

In the useEffect hook we want to check whether or not the image is cached after a loading delay of 200 milliseconds. If the image is not loaded (isLoading is false) we setIsImageCached to false.

useRef is used for persisting states across re-renders, but there may not be a need to make use of useRef because we can effectively manage the loading and caching states with useState that dictate the rendering behavior of this component.

In the Image component, we could check to see if the image is not cached and is currently loading. When image loads completely, we can set the caching value to true and the isLoading value to false.

[melvin-bot]

📣 @jNembhard! 📣
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. 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.
2. 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.
3. 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>

[ntdiary]

@Pluto0104 , eh, do we only need to change this one place? It seems not working.

App/src/components/ImageWithSizeCalculation.js

Line 80 in b119f9a

if (isLoadedRef.current || isLoading) {

[jNembhard]

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

[melvin-bot]

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

[Pluto0104]

I apologize for the confusion. @ntdiary, I made an error in my proposal. I've now revised it. Could you please take a look here?

[ntdiary]

Hi everyone, I'll give an update on the review status for all the proposals. However, please don't rush to update these proposals yet, because I think there's a small detail we need to check with the design team first,
i.e., when offline, should we display a default image, or just keep it blank like it it now?

[ntdiary]

@Pluto0104 , It looks like your updated version will keep showing loading indefinitely, and the image cannot reload even when the device is back online.

Pluto0104.mp4

[ntdiary]

@dukenv0307 , your proposal also does not reload the image when the device is online.

dukenv0307.mp4

@tienifr, your [proposal] seems to add wrapping imageLoadedSuccessfully with useCallback compared to @dukenv0307's, I'm not too sure if we really need it, and it also has the same reloading issue.

[greg-schroeder]

Reviews continue on linked PR

[melvin-bot]

This issue has not been updated in over 15 days. @dannymcclain, @ntdiary, @cristipaval, @greg-schroeder, @kaushiktd eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[greg-schroeder]

Work continues on linked PR - seems like it's been going a while, is there anything you need from Expensify team to help push this over the finish line?

[ntdiary]

Work continues on linked PR - seems like it's been going a while, is there anything you need from Expensify team to help push this over the finish line?

@greg-schroeder, so far, in my personal opinion, the fallback icon will only be displayed when there is a failure to load the generic receipt image. And I noticed that our code introduced the "EReceipt" concept. If the below document providing a more detailed explanation of it, could you please grant me access to it? I would like to understand this concept for further clarification.
Possible related issue and document:
#28826
https://docs.google.com/document/d/1iELOXPOAcnuIPZJ3XbO1S8_3qHaM31Ptf8dR4LRZMyE/edit#bookmark=kix.ccj1ab3m8lq

[greg-schroeder]

Hmm. I'm not sure if we can share access to that as it's an internal design doc, I am asking.

[greg-schroeder]

Discussing in Slack

[greg-schroeder]

• @kaushiktd I'm sending you an offer for $250 (half the bounty of this issue) for the work you've done on the linked PR. Even though the issue was fixed elsewhere, we appreciate your effort here.
• Let’s split off the second part of this issue (improving the display of receipt fallback image) into a new issue and start fresh, as we don’t feel the current implementation is ideal
• We can probably just close the linked PR and this issue

[greg-schroeder]

Offer sent to @kaushiktd

[greg-schroeder]

I'll pay that out as soon as you accept. Closing this accordingly!

[kaushiktd]

• I'm sending you an offer for $250 (half the bounty of this issue) for the work you've done on the linked PR. Even though the issue was fixed elsewhere, we appreciate your effort here.

That is a bummer! But alright.
Also if you wish I can work on this issue for second part as I have worked so far on first part and second is relevant to this.

And I accepted the offer as well.

[kaushiktd]

One more request, since I have not worked on Upwork for quite a long time, can you please give me nice rating with review? This will mean a lot to me.

[greg-schroeder]

Yes of course - I left a 5 star review. I'll get it on my list to create the new issue for the other item. @ntdiary indicated we weren't in a great place for that yet so we want to split it out to get a more unified approach going

[kaushiktd]

So can I submit solution of second part here OR will you create a new issue?