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

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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:

Action Performed:

1. Send a image attachment in any report, wait until attachment is uploaded
2. 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?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Screen.Recording.2024-06-26.at.10.17.30.PM.mov

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @mallenexpensify

[melvin-bot]

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]

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

[mallenexpensify]

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

[bernhardoj]

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

App/src/components/Attachments/AttachmentCarousel/index.tsx

Lines 89 to 92 in d489ba4

	// Update the parent modal's state with the source and name from the mapped attachments
	if (targetAttachments[initialPage] !== undefined && onNavigate) {
	onNavigate(targetAttachments[initialPage]);
	}

onNavigate will call this function to update the route.

App/src/pages/home/report/ReportAttachments.tsx

Lines 25 to 31 in d489ba4

	const onCarouselAttachmentChange = useCallback(
	(attachment: Attachment) => {
	const routeToNavigate = ROUTES.ATTACHMENTS.getRoute(reportID, type, String(attachment.source), Number(accountID));
	Navigation.navigate(routeToNavigate);
	},
	[reportID, accountID, type],
	);

But the modal component has a cleanup effect that will calls onModalHide.

App/src/components/Modal/BaseModal.tsx

Lines 113 to 123 in d489ba4

	useEffect(
	() => () => {
	// Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
	if (!isVisibleRef.current) {
	return;
	}
	hideModal(true);
	},
	// eslint-disable-next-line react-hooks/exhaustive-deps
	[],
	);

And onModalHide will close the attachment carousel page in this case.

App/src/pages/home/report/ReportAttachments.tsx

Lines 41 to 45 in d489ba4

	onModalHide={() => {
	Navigation.dismissModal();
	// This enables Composer refocus when the attachments modal is closed by the browser navigation
	ComposerFocusManager.setReadyToFocus();
	}}

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.

App/patches/react-native-web+0.19.12+003+image-header-support.patch

Lines 78 to 99 in d489ba4

	+ React.useEffect(() => {
	+ if (!hasSourceDiff(nextSource, request.current.source)) {
	+ return;
	+ }
	+
	+ // When source changes we want to clean up any old/running requests
	+ request.current.cancel();
	+ if (onLoadStart) {
	+ onLoadStart();
	+ }
	+
	+ // Store a ref for the current load request so we know what's the last loaded source,
	+ // and so we can cancel it if a different source is passed through props
	+ request.current = ImageLoader.loadWithHeaders(nextSource);
	+ request.current.promise.then(uri => setBlobUri(uri)).catch(() => raiseOnErrorEvent(request.current.source.uri, {
	+ onError,
	+ onLoadEnd
	+ }));
	+ }, [nextSource, onLoadStart, onError, onLoadEnd]);
	+
	+ // Cancel any request on unmount
	+ React.useEffect(() => request.current.cancel, []);

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.

App/src/pages/home/report/ReportAttachments.tsx

Lines 41 to 45 in d489ba4

	onModalHide={() => {
	Navigation.dismissModal();
	// This enables Composer refocus when the attachments modal is closed by the browser navigation
	ComposerFocusManager.setReadyToFocus();
	}}

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,

App/patches/react-native-web+0.19.12+003+image-header-support.patch

Lines 22 to 24 in d489ba4

	+function hasSourceDiff(a, b) {
	+ return a.uri !== b.uri || JSON.stringify(a.headers) !== JSON.stringify(b.headers);
	+}

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.

App/patches/react-native-web+0.19.12+003+image-header-support.patch

Line 79 in d489ba4

+ if (!hasSourceDiff(nextSource, request.current.source)) {

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]

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

[melvin-bot]

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

[melvin-bot]

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

[mallenexpensify]

@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]

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

[melvin-bot]

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

[mallenexpensify]

@alitoshmatov , 👀 above plz

[alitoshmatov]

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]

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]

Issue not reproducible during KI retests. (First week)

[melvin-bot]

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

[Kicu]

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]

@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]

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

[bernhardoj]

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

[bernhardoj]

PR is ready

cc: @ikevin127

[ikevin127]

⚠️ 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]

cc @mallenexpensify

[mallenexpensify]

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]

@ikevin127 paid, post above updated to reflect payment.

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: Enable React concurrent mode #42592.
• [@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: Enable React concurrent mode #42592 (comment).
• [@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.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@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

1. Send an image attachment to a chat.
2. While the attachment is still loading (uploading), click on the loading attachment preview to open it.
3. On the attachment page, if the image is still loading (uploading) -> verify that the loading indicator is displayed and the app doesn't crash.
4. 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]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj

[mallenexpensify]

Test Case GH

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