m-natarajan commented 3 months ago

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: Reproducible in staging?: Needs reproduction Reproducible in production?: Needs reproduction 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: @neil-marcellini Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1719847597306219

Please dm @neil-marcellini on Slack during PDT hours if you would like to test against my dev env.

Action Performed:

Open New Expensify running on a backend dev environment (ask an internal engineer to share theirs via ngrok)
View an expense with a receipt
Expected Result:

Once the receipt is loaded there won't be a loading spinner

Actual Result:

Once the receipt is loaded there won't be a loading spinner

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?
- [ ] Android: Native
- [ ] Android: mWeb Chrome
- [ ] iOS: Native
- [ ] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/38435837/50ac8aad-1cce-4fe4-9363-e2a900e99dc0

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~014e28bd547489ae41
Upwork Job ID: 1807839133623941808
Last Price Increase: 2024-07-25

melvin-bot[bot] commented 3 months ago

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

MelvinBot commented 3 months ago

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~014e28bd547489ae41

melvin-bot[bot] commented 3 months ago

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

Taiwrash commented 3 months ago

Proposal

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

The spinner keep rolling after the receipt as loaded and currently not showing the image again
What is the root cause of that problem?
Width value of the image is undefined and cannot be determined App/blob/main/src/components/Image/BaseImage.tsx#L7-L18
What changes do you think we should make in order to solve the problem?

Handle the state using the callback and memo correctly by updating the imageLoadedSuccessfully function with

const imageLoadedSuccessfully = useCallback(
(event: {nativeEvent: ImageLoadEventData}) => {
    if (!onLoad) {
        return;
    }

    // We override `onLoad`, so both web and native have the same signature
    const source = event.nativeEvent.source;
    if (source && typeof source === 'object') {
        const {width, height} = source;
        onLoad({nativeEvent: {width, height}});
    } else {
        // Handle the case where source is undefined or not an object
        console.warn('Image source is undefined or not an object');
        onLoad({nativeEvent: {width: 0, height: 0}}); 
    }
},
[onLoad],
);

What alternative solutions did you explore? (Optional)

ishpaul777 commented 3 months ago

This might be dupe of https://github.com/Expensify/App/issues/44566, I can also reproduce with stag/prod server, setting USE_REACT_STRICT_MODE=false in CONFIG.js should resolve the issue temporarily.

Taiwrash commented 3 months ago

@ishpaul777 yes, it looks the same but I think for this demo, the app is not crashing which is different from the previous suggestion

eh2077 commented 3 months ago

@Taiwrash Can you please elaborate on the root cause of this issue?

Taiwrash commented 3 months ago

YES!

The main cause of the error is the improper cleaning up of the data after fetching it from the backend. Although this can also be handled by setting the right dependencies for the useEffect where data fetching is happening. In summary, the root cause is the improper way of state update and wrong cleanup

neil-marcellini commented 3 months ago

@Taiwrash thanks for elaborating. Would you please update your original proposal with links to the current code causing the root problem and explain exactly what the issue is, along with your proposed solution? Please see our contributing guidelines and you can also look at proposals on other issue to get an idea of how they are written.

Thanks @ishpaul777 for recommending setting USE_STRICT_MODE = false. I'll try that.

neil-marcellini commented 3 months ago

This might be dupe of #44566,

Let's keep an eye on that one because it might have the same fix or root cause, but this feels a bit different so I'll keep it separate for now.

neil-marcellini commented 3 months ago

Setting USE_REACT_STRICT_MODE: false in Config.ts doesn't seem to solve the problem.

ishpaul777 commented 3 months ago

I just tried to reproduce again, i was not able to reproduce with staging server.

ishpaul777 commented 3 months ago

Sorry i take my words back its reproducable but USE_REACT_STRICT_MODE: false does solve it for me

https://github.com/Expensify/App/assets/104348397/a116ffed-94fc-4e32-8604-4014e3bf1efa

melvin-bot[bot] commented 3 months ago

@neil-marcellini, @kadiealexander, @eh2077 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

eh2077 commented 2 months ago

Still looking for proposals

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

kadiealexander commented 2 months ago

@neil-marcellini does this still happen for you?

neil-marcellini commented 2 months ago

Yes, and now the receipt images don't load at all. Let's double the price.

melvin-bot[bot] commented 2 months ago

Upwork job price has been updated to $500

Taiwrash commented 2 months ago

Yes, and now the receipt images don't load at all. Let's double the price.

Do you have a new error when the receipt is not showing now?

Kindly help with screenshot or video of the current state. I am sorry i have been out on this issue before now

Taiwrash commented 2 months ago

I figured out the error is due to the failure to receive width properties from from the event.nativeEvent in some cases of the responses

const {width, height} = event.nativeEvent.source;

Here is a quick hack I came up to fix the issue for now

const imageLoadedSuccessfully = useCallback(
    (event: {nativeEvent: ImageLoadEventData}) => {
        if (!onLoad) {
            return;
        }

        // We override `onLoad`, so both web and native have the same signature
        const source = event.nativeEvent.source;
        if (source && typeof source === 'object') {
            const {width, height} = source;
            onLoad({nativeEvent: {width, height}});
        } else {
            // Handle the case where source is undefined or not an object
            console.warn('Image source is undefined or not an object');
            onLoad({nativeEvent: {width: 0, height: 0}}); 
        }
    },
    [onLoad],
);

The hacks: check source availability and is an object before destructuring.

eh2077 commented 2 months ago

@Taiwrash Thanks for sharing your findings. Can you please update your proposal based on suggestion here https://github.com/Expensify/App/issues/44692#issuecomment-2204798363

Taiwrash commented 2 months ago

Proposal

Updated

Taiwrash commented 2 months ago

@neil-marcellini @eh2077 looking forward to your review and besides i haven't been added to the slack channel, all the links i am seeing are not working for me. they are requesting for a unique email address

eh2077 commented 2 months ago

@Taiwrash

Width value of the image is undefined and cannot be determined

Why does this keep images loading?

Taiwrash commented 2 months ago

@Taiwrash

Width value of the image is undefined and cannot be determined

Why does this keep images loading?

The code is trying to access the width property of an object that is undefined. This is happening in the image loading callback
The code is structure to use the loading spinner while waiting for images to load, the spinner's behaviour is tied to the successful completion of the image loading process which is not completing.
The error has prevented the normal flow of the code that would otherwise signal that the image has finished loading, in other words, code to update the state of the spinner is not executed

Those are my observations @eh2077

dominictb commented 2 months ago

Proposal

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

The receipt is infinitely loading in dev mode. In my machine it's the behavior is different from what I see in the evidence video: the image doesn't load at all. However, I believe all share the same root cause.

What is the root cause of that problem?

When enabling Strict Mode, React 18 simulates unmounting/mounting the component. Read more here. However, it seems like it doesn't re-create the ref during that simulation, and we'll have trouble with our patch for react-native-web's Image component.

During the unmount simulation, the image request will be cancelled here. However, since the ref is not re-created, in the next mount simulation this condition will hold true as the props.source is the same, hence, the image will never be loaded. We can easily check the Network tab and see that the image request gets cancelled.

This also explains if we disable the strict mode, as there's no mount/unmount simulation, the request is never cancelled and we got the image loaded successfully.

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

The idea is that if the request got cancelled during unmount/mount simulation, we should catch that and allows the request to be re-created during in the next mount. Details are below:

In here, introduce isCancelled function

isCancelled: () => abortController.signal.aborted

By default, in here, isCancelled() should always be true

+  var request = React.useRef({
+    cancel: () => {},
+    source: {
+      uri: '',
+      headers: {}
+    },
+    promise: Promise.resolve(''),
+.   isCancelled: () => true,
+  });

And in here, we should allow the request ref to be re-created (i.e: start a new image request) in case the current request is cancelled

if (!hasSourceDiff(nextSource, request.current.source) && !request.current.isCancelled()) {
   return;
}

What alternative solutions did you explore? (Optional)

N/A

eh2077 commented 2 months ago

@dominictb Thank you for sharing your investigation!

eh2077 commented 2 months ago

This might be dupe of #44566,

Let's keep an eye on that one because it might have the same fix or root cause, but this feels a bit different so I'll keep it separate for now.

@neil-marcellini I think we should put this on hold for https://github.com/Expensify/App/issues/44566 as they're very likely to share the same root cause.

Setting USE_REACT_STRICT_MODE: false in Config.ts doesn't seem to solve the problem.

Now it solved the problem if USE_REACT_STRICT_MODE: false. It also didn’t work in my previous testing.

neil-marcellini commented 2 months ago

This can be take off hold now. USE_REACT_STRICT_MODE: false does indeed fix the problem now, but it would be great to get this working with strict mode enabled too. @eh2077 @dominictb will your proposal work as is or do you need to make any updates now that the issue is off hold?

eh2077 commented 2 months ago

There're some recent updates on patches/react-native-web+0.19.12+003+image-header-support.patch from PR https://github.com/Expensify/App/pull/45559/files. I think we need to diagnose the root cause again.

@dominictb Would you like to take a look into this?

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

dominictb commented 2 months ago

@eh2077 I think we can close this issue. That patch itself fix this problem as well. Thanks!

neil-marcellini commented 2 months ago

Ah yeah, the problem is fixed after I double checked. Sweet!

I had to run this to get the package patch to apply properly.

npm cache clean --force
rm -rf node_modules
npm install

Expensify / App

[$500] [Dev] Infinite receipt loading spinner with backend dev environment #44692

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Here is a quick hack I came up to fix the issue for now

Proposal

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)