[HOLD #23143] Dev: Web - App crashes when deleting an image while the image is opened

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open a tab where user A is signed in
2. Send an image to user B
3. Open a tab where user B is signed in
4. Open the sent image
5. Switch to the tab where user A is signed in
6. Delete the sent image
7. Observe the behavior on the tab where user B is signed

Expected Result:

Image preview carousel should close

Actual Result:

App crashes

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: Dev 1.3.47-2
Reproducible in staging?: no
Reproducible in production?: no
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

Screencast.from.2023-07-29.13-46-11.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Natnael-Guchima
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690627861170639

View all open jobs on GitHub

[melvin-bot]

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

[ygshbht]

Proposal

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

The problem is that when an image attachment is deleted by User A while it's opened in the view of User B, the app crashes. This is not the expected behavior. The image preview carousel should close when the image gets deleted.

What is the root cause of that problem?

The root cause of the problem is that when an attachment is deleted, it becomes a null value. Any subsequent attempts to access its properties such as source or URL results in an error, causing the app to crash.

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

A solution is to check if the attachment exists before trying to display it or accessing its properties. This can be achieved by, for example, adding optional chaining (the question mark syntax) to the attachment variable when accessing its properties. Similarly, for onCarouselAttachmentChange, we can add a condition to verify if attachment exists before navigation. The modifications will look like the following:

setSource(attachment?.source);
setFile(attachment?.file);
setIsAuthTokenRequired(attachment?.isAuthTokenRequired);
onCarouselAttachmentChange(attachment);

onCarouselAttachmentChange={(attachment) => {
    if (attachment) {
        const route = ROUTES.getReportAttachmentRoute(reportID, attachment.source);
        Navigation.navigate(route);
    }
}}

This will ensure that even if the attachment is null, accessing its properties won't cause the application to crash and the modal will close gracefully.

We can also manually call Navigation.dismissModal to close the modal if the attachment is no longer present or show an error that this attachment has been deleted in the modal iteself

Edit 1: Please see comment

2023-07-31.14-11-06.mp4

[tienifr]

Proposal

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

Dev: Web - App crashes when deleting an image while the image is opened

What is the root cause of that problem?

In

App/src/components/AttachmentCarousel/index.js

Lines 51 to 56 in 024d210

	useEffect(() => {
	const initialState = createInitialState(props);
	setPage(initialState.page);
	setAttachments(initialState.attachments);
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [props.reportActions]);

We're executing createInitialState function whenever the reportActions get changed. So when users delete the image, we call createInitialState again, but at that time the image is gone so we can't find the page => page = -1

=> attachments[page] is undefined

App/src/components/AttachmentCarousel/createInitialState.js

Lines 52 to 58 in 024d210

	const page = _.findIndex(attachments, (a) => a.source === props.source);
	if (page === -1) {
	Navigation.dismissModal();
	}
	// Update the parent modal's state with the source and name from the mapped attachments
	props.onNavigate(attachments[page]);

At this line, we're trying to access data from undefined => App crashes

App/src/components/AttachmentModal.js

Line 117 in 024d210

setSource(attachment.source);

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

We shouldn't call call onNavigate if page is -1

Change

App/src/components/AttachmentCarousel/createInitialState.js

Lines 53 to 55 in 024d210

	if (page === -1) {
	Navigation.dismissModal();
	}

to

if (page === -1) {
        Navigation.dismissModal();
    }
    return {
            page,
            attachments,
        };

[ygshbht]

Proposal

Edit

Even though I've already suggested that we should dismiss the modal if the attachment is not present, I would like to further argue that the functionality of dismissModal and onNavigate is not best placed inside the createInitialState function.

App/src/components/AttachmentCarousel/createInitialState.js

Lines 14 to 64 in 8cdb506

	function createInitialState(props) {
	const actions = [ReportActionsUtils.getParentReportAction(props.report), ...ReportActionsUtils.getSortedReportActions(_.values(props.reportActions))];
	const attachments = [];
	const htmlParser = new HtmlParser({
	onopentag: (name, attribs) => {
	if (name !== 'img' || !attribs.src) {
	return;
	}
	const expensifySource = attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE];
	// By iterating actions in chronological order and prepending each attachment
	// we ensure correct order of attachments even across actions with multiple attachments.
	attachments.unshift({
	source: tryResolveUrlFromApiRoot(expensifySource || attribs.src),
	isAuthTokenRequired: Boolean(expensifySource),
	file: {name: attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE]},
	});
	},
	});
	_.forEach(actions, (action, key) => {
	if (!ReportActionsUtils.shouldReportActionBeVisible(action, key)) {
	return;
	}
	htmlParser.write(_.get(action, ['message', 0, 'html']));
	});
	htmlParser.end();
	// Inverting the list for touchscreen devices that can swipe or have an animation when scrolling
	// promotes the natural feeling of swiping left/right to go to the next/previous image
	// We don't want to invert the list for desktop/web because this interferes with mouse
	// wheel or trackpad scrolling (in cases like document preview where you can scroll vertically)
	if (DeviceCapabilities.canUseTouchScreen()) {
	attachments.reverse();
	}
	const page = _.findIndex(attachments, (a) => a.source === props.source);
	if (page === -1) {
	Navigation.dismissModal();
	}
	// Update the parent modal's state with the source and name from the mapped attachments
	props.onNavigate(attachments[page]);
	return {
	page,
	attachments,
	};
	}

A more suitable location would be within the AttachmentCarousel that listens for changes in reportActions like so:

App/src/components/AttachmentCarousel/index.js

Lines 51 to 56 in 8cdb506

	useEffect(() => {
	const initialState = createInitialState(props);
	setPage(initialState.page);
	setAttachments(initialState.attachments);
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [props.reportActions]);

We can modify this to:

useEffect(() => {
    const initialState = createInitialState(props);

    if (initialState.page === -1){
       return Navigation.dismissModal();
    }

    props.onNavigate(attachments[page]);

    setPage(initialState.page);
    setAttachments(initialState.attachments);
    // eslint-disable-next-line react-hooks/exhaustive-deps
}, [props.reportActions]);

This approach brings several benefits:

1. Consistency: Keeping side effects such as navigation or modal display changes out of utility functions like createInitialState ensures they remain pure and only depend on their input.

2. Clarity and Isolation: Having these side effects in the component's useEffect makes the logic easier to follow. All the state changes and side effects are happening in one place, which also simplifies debugging.

Original proposal

[ahmedGaber93]

This issue is a regression from #21334.
And will fix by this PR #23143

https://github.com/Expensify/App/pull/23143/files#diff-9c73db7c8d0624bb6c0911271f174d67a8f4b02f7d01c5922584555e10f1b59a

[puneetlath]

Thanks @ahmedGaber93. I'm going to go ahead and close this issue out then. Appreciate the help in triaging.

[Natnael-Guchima]

@puneetlath this issue is a regression of #21334, and is only found on staging and not reproducible on production. The fixing PR @ahmedGaber93 shared is two weeks old pr which is created before this bug was introduced, I am not sure how that pr can fix this issue unless it is amended to do so. I think it is a good idea to keep this issue open and see if a resolution for this issue comes from the offending PR as this comment suggests #22261 (comment)