[Pay 7/30] [Dev] - [StrictMode] Attachments not loading and app crashes when click on loading preview from attachment

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?: need reproduction Reproducible in production?: need 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: @ishpaul777 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1719508144288849

Action Performed:

Send a image attachment in any report, wait until attachment is uploaded
Click on loading preview. Expected Result: Actual Result: Image infinitely loading in preview and app crashes on clicking preview.
Expected Result:

Image not infinitely loading in preview and app not crash on clicking preview.

Actual Result:

Image infinitely loading in preview and app crashes on clicking preview.

Workaround:

unknown

Platforms:

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

Screenshots/Videos

https://github.com/Expensify/App/assets/38435837/2a29ca3c-e6a0-45e7-b825-98d0b1000e11

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @mallenexpensify

melvin-bot[bot] commented 3 months ago

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

mallenexpensify commented 3 months ago

Can't remember the best practice for dev issues so checking on in #bug-zero https://expensify.slack.com/archives/C01SKUP7QR0/p1719540502698309

bernhardoj commented 3 months ago

Proposal

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

Attachment is infinitely loading and crash when opens in DEV.

What is the root cause of that problem?

This happens because of the new strict mode. For the crash, it happens becuase of maximum update stack error. As we know, strict mode will cause the component to mount and unmounted twice. What happen is, the modal is open and close repeatedly and infinitely. The attachment carousel has an effect that calls onNavigate when the valid attachment is found on the report https://github.com/Expensify/App/blob/d489ba44a1d4c951de7a405caa452f33b86ac70f/src/components/Attachments/AttachmentCarousel/index.tsx#L89-L92 onNavigate will call this function to update the route. https://github.com/Expensify/App/blob/d489ba44a1d4c951de7a405caa452f33b86ac70f/src/pages/home/report/ReportAttachments.tsx#L25-L31

But the modal component has a cleanup effect that will calls onModalHide. https://github.com/Expensify/App/blob/d489ba44a1d4c951de7a405caa452f33b86ac70f/src/components/Modal/BaseModal.tsx#L113-L123

And onModalHide will close the attachment carousel page in this case. https://github.com/Expensify/App/blob/d489ba44a1d4c951de7a405caa452f33b86ac70f/src/pages/home/report/ReportAttachments.tsx#L41-L45

For the infinite loading issue, it comes from the react-native-web image headers patch that we have. The image header has 2 useEffects.

The first one is to cancel any ongoing request and remade a new one when the source is updated, and the others to cancel any ongoing request when unmounted. https://github.com/Expensify/App/blob/d489ba44a1d4c951de7a405caa452f33b86ac70f/patches/react-native-web%2B0.19.12%2B003%2Bimage-header-support.patch#L78-L99

But because of the strict mode, the unmounted cleanup is called, which cancels the ongoing request. The 1st useEffect won't do the setup anymore on the 2nd call/run because the source hasn't been updated yet (the current source and the prop source is still the same), so the request is never remade.

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

To fix the crash, we should replace onModalHide with onModalClose. https://github.com/Expensify/App/blob/d489ba44a1d4c951de7a405caa452f33b86ac70f/src/pages/home/report/ReportAttachments.tsx#L41-L45

Why? I think in this case, doing a navigation isn't supposed to be done in a useEffect and rather should be done when the user takes the action to do so, like when a user buy an item from a button click, not from an effect.

onModalClose is called whenever the user takes the action to close the modal, either by pressing ESC, back button, backdrop/overlay press, swipe to bottom, shortcut to open other pages, etc. (we use onModalClose for other attachment page, WorkspaceAvatar, ProfileAvatar, ReportAvatar, & TransactionReceiptPage).

If we want to keep using onModalHide, then we can introduce a new ref in AttachmentModal that turns true whenever closeModal is called (called by user action), then we pass it to the onModalHide params. If it's true, then we do the navigation.

For the infinite loading issue, another reason the strict mode exist is to make sure we setup and cleanup an effect properly. In the image with headers case, it's a bit different. In a normal case, we would do something like this:

useEffect(() => {
    setUp();
    return cleanup;
}, [deps1, deps2, ...]);

However, the deps of image with headers includes onLoadStart, onError, onLoadEnd and looks like we don't want the request to be cancelled and remade when those changes. We can remove them from the effect deps, but the source prop is an object which is unstable, thus we made a comparison using hasSourceDiff, https://github.com/Expensify/App/blob/d489ba44a1d4c951de7a405caa452f33b86ac70f/patches/react-native-web%2B0.19.12%2B003%2Bimage-header-support.patch#L22-L24

and only cancel and remade the request when the source content (uri and headers) is really updated.

So, to fix this, I propose to clears the current request source when the component is unmounted.

// Cancel any request on unmount
React.useEffect(() => () => {
    request.current.cancel();
    // reset to the init value of request, we can move the initial value to a variable
    request.current = {
        cancel: () => {},
        source: {
            uri: '',
            headers: {}
        },
        promise: Promise.resolve('')
    }
}, []);

The above code clears the current request with the initial request value, but I think it's better if the initial request value is null to make it simple. Then, we just need to adjust the code here to add null safety to the current request source and default it to an empty object. https://github.com/Expensify/App/blob/d489ba44a1d4c951de7a405caa452f33b86ac70f/patches/react-native-web%2B0.19.12%2B003%2Bimage-header-support.patch#L79

Resetting it to null is similar to what the rn-web did with the image (without headers). initial value is null: https://github.com/necolas/react-native-web/blob/54c14d64dabd175e8055e1dc92e9196c821f9b7d/packages/react-native-web/src/exports/Image/index.js#L218 resets to null: https://github.com/necolas/react-native-web/blob/54c14d64dabd175e8055e1dc92e9196c821f9b7d/packages/react-native-web/src/exports/Image/index.js#L312-L320

If you see the 2nd link above, you will notice that the image (without headers) will cancel and remade the request whenever onLoadStart, onError, onLoadEnd is updated, if we want to do that with image with headers too, then we can update the current useEffect structure to be like in the snippet example I gave above.

But, I'm not sure how should we approach this rn-web patch fix. The upstream PR for this patch hasn't been closed yet and currently is merged to the rn-web 0.20 preview PR.

melvin-bot[bot] commented 3 months ago

@mallenexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 3 months ago

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

melvin-bot[bot] commented 3 months ago

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

mallenexpensify commented 3 months ago

@alitoshmatov can you attempt reproduction? If you're able to reproduce , can you review @bernhardoj 's proposal above? (also.. I'm assuming this can be External, I'm not that familiar with dev environment-only issues.

mallenexpensify commented 3 months ago

cc @Kicu cuz it involves [strict mode], in case you wanted to address.

melvin-bot[bot] commented 2 months ago

@mallenexpensify, @alitoshmatov Huh... This is 4 days overdue. Who can take care of this?

mallenexpensify commented 2 months ago

@alitoshmatov , 👀 above plz

alitoshmatov commented 2 months ago

Sorry missed this couple of times. I was low on bandwidth this week, so I would unassign myself and ask for another C+ on slack.

cc: @mallenexpensify

ikevin127 commented 2 months ago

I'll take this over from @alitoshmatov, based on Slack thread.

I'll try to reproduce ~~today~~ and if successful, will move on to reviewing existing proposals.

mvtglobally commented 2 months ago

Issue not reproducible during KI retests. (First week)

melvin-bot[bot] commented 2 months ago

@mallenexpensify 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!

ikevin127 commented 2 months ago

Callback to https://github.com/Expensify/App/issues/44566#issuecomment-2218513434, will try reproducing once assigned and we'll take it from there.

mallenexpensify commented 2 months ago

You're assigned now @ikevin127

ikevin127 commented 2 months ago

Issue is only reproducible in dev mode (locally) because of React's StrictMode. ♻️ Moving forward with the proposal review!

ikevin127 commented 2 months ago

@bernhardoj's proposal looks good to me. I verified the RCA as correct and the proposed solutions for both parts of the issue are fixing the bugs as requested by the expected result.

Note: The only aspect I was not able to verify entirely is the 2nd part of the patch update, which I was not able to apply myself since I was not provided the complete diff - but the references and examples provided make sense and I'm sure that, if applied correctly, they will fix the infinite loading issue.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @tgolen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

ikevin127 commented 2 months ago

♻️ Bump on ^ https://github.com/Expensify/App/issues/44566#issuecomment-2226553207.

@mallenexpensify You can safely remove the Needs Reproduction label since this issue can only be reproduced locally, so I'm assuming the fix will only be verifiable on local dev. The QA should still verify that no regressions arise once the solution gets to staging though.

mallenexpensify commented 2 months ago

@tgolen 👀 on the comment above about the proposal above plz

Kicu commented 2 months ago

Sorry I kinda forgot about this and I'm late to the party, but I think the RCA part is good work. Indeed useEffect cleanup switching some kind of flag or cleaning some shared state was the most common source of errors with StrictMode that I have noticed so far.

I don't want to speak for the Modals fix since Im not familiar with modals code in this project.

The fix regarding cancelling and resetting request sound legit 👍

ikevin127 commented 2 months ago

@bernhardoj You were (silently) assigned, I think you have the 🟢 to open the PR.

Looks like our automations didn't trigger maybe.

Yep, because melvin removed the Help Wanted label about 5 days ago when I took over the issue from Ali.

tgolen commented 2 months ago

Yes, indeed! Looks like our automations didn't trigger maybe. Oops! 🟢 to @bernhardoj

bernhardoj commented 2 months ago

Thanks for the bump! Working on the PR...

bernhardoj commented 2 months ago

PR is ready

cc: @ikevin127

ikevin127 commented 2 months ago

⚠️ Automation failed for this one -> should be on [HOLD for payment 2024-07-30] according to yesterday's (23rd) production deploy from #45559 (comment).

cc @mallenexpensify

ikevin127 commented 2 months ago

cc @mallenexpensify

mallenexpensify commented 2 months ago

Contributor: @bernhardoj owed $250 via Upwork Contributor+: @ikevin127 paid $250 via Upwork.

@ikevin127 can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~01650c8361cab58c60

@ikevin127 plz complete the BZ checklist below. thx

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] [@] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@] Determine if we should create a regression test for this bug.
[ ] [@] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@BZ] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

mallenexpensify commented 2 months ago

@ikevin127 paid, post above updated to reflect payment.

ikevin127 commented 2 months ago

[x] [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/42592.
[x] [@ikevin127] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/42592#issuecomment-2261673246.
[x] [@ikevin127] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: https://expensify.slack.com/archives/C02NK2DQWUX/p1721073264111289.
[x] [@ikevin127] Determine if we should create a regression test for this bug.
[x] [@ikevin127] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

Send an image attachment to a chat.
While the attachment is still loading (uploading), click on the loading attachment preview to open it.
On the attachment page, if the image is still loading (uploading) -> verify that the loading indicator is displayed and the app doesn't crash.
On the attachment page, once the image was uploaded, verify that the image is displayed and the app doesn't crash.

Do we agree 👍 or 👎.

bernhardoj commented 2 months ago

Requested in ND.

JmillsExpensify commented 2 months ago

$250 approved for @bernhardoj

mallenexpensify commented 2 months ago

Test Case GH

https://github.com/Expensify/Expensify/issues/418334

Expensify / App