[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 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:

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.