[$2000] Chat - Deleted image URL crashes the app on mac and windows (works fine on mobile)

[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 the app and login with user A (device 1)
2. Open any group chat
3. Login with any other user that is in the above group in another device (device 2)
4. Send any image attachment from device 2 in group chat
5. From device 1, open the attachment in preview, copy the URL and send it in the group
6. From device 2, delete the attachment sent in step 4
7. From device 1, click on the URL and observe that app crashes instead of showing 'Hmm its not here' page like it does on android chrome

Expected Result:

App should display 'Hmm its not here' page when we open link of delete image like it does on android chrome

Actual Result:

App crashes when we open link of delete image

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: 1.3.36-4
Reproducible in staging?: y
Reproducible in production?: y
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

app.crash.deleted.image.url.mp4

Recording.3425.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688488679783719

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0113703c85388c5be7
• Upwork Job ID: 1678516819849125888
• Last Price Increase: 2023-08-02

[melvin-bot]

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

[ahmedGaber93]

Proposal

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

Chat - Deleted image URL crashes the app on mac and windows

What is the root cause of that problem?

when attachment not found we throw js error instead of handling the error with UI.

App/src/components/AttachmentCarousel/index.js

Lines 187 to 190 in f9a8529

	const page = _.findIndex(attachments, (a) => a.source === this.props.source);
	if (page === -1) {
	throw new Error('Attachment not found');
	}

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

1. remove the error throw condition.
2. add condition before onNavigate to prevent error.

// this.props.onNavigate(attachments[page]);

if(page !== -1){
    this.props.onNavigate(attachments[page]);
}

3. create new prop to control icon download visibility, and hide the icon when file not found.

// in AttachmentCarousel
this.props.hideDownloadButton(page === -1); 

// in AttachmentModal
const [shouldShowDownloadButton, setShouldShowDownloadButton] = React.useState(true);

const hideDownloadButton = useCallback((shouldHide) => {
    setShouldShowDownloadButton(!shouldHide);
}, []);

shouldShowDownloadButton={props.allowDownload && shouldShowDownloadButton}

4. show Not found screen UI if file not found

//wrap content with
<FullPageNotFoundView
     shouldShow={this.state.page === -1}
     ...
/>
     ...    
</FullPageNotFoundView>     

FullPageNotFoundView has header with back arrow, I think we need also add prop to it to control this header visibility, or we can use BlockingView directly in AttachmentCarousel in render condition.

<BlockingView
    icon={Illustrations.ToddBehindCloud}
    iconWidth={variables.modalTopIconWidth}
    iconHeight={variables.modalTopIconHeight}
    title={props.translate('notFound.notHere')}
    subtitle={props.translate('notFound.pageNotFound')}
/>                 

What alternative solutions did you explore? (Optional)

instead we can fix it in AttachmentModal file here by adding condition

shouldShowDownloadButton={props.allowDownload && !fileNotFound}

!fileNotFound ? <AttachmentCarousel/> : <BlockingView/>

and used the same way here to check if file found or not.

[samh-nl]

Proposal

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

A deleted image crashes the app on Mac and Windows when viewed.

What is the root cause of that problem?

If the image is not found when setting the initial state of the AttachmentCarousel component, an error is thrown. This leads to the crash screen being rendered.

App/src/components/AttachmentCarousel/index.js

Lines 187 to 202 in f9a8529

	const page = _.findIndex(attachments, (a) => a.source === this.props.source);
	if (page === -1) {
	throw new Error('Attachment not found');
	}
	// Update the parent modal's state with the source and name from the mapped attachments
	this.props.onNavigate(attachments[page]);
	return {
	page,
	attachments,
	shouldShowArrow: this.canUseTouchScreen,
	containerWidth: 0,
	isZoomed: false,
	activeSource: null,
	};

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

Not found images should be handled gracefully.

First, we must remove the thrown error and instead we normally return the state. This is a slightly more elegant approach than the previously proposed change as this results in no duplicate code:

const page = _.findIndex(attachments, (a) => a.source === this.props.source);

if (page > -1) {
    // Update the parent modal's state with the source and name from the mapped attachments
    this.props.onNavigate(attachments[page]);
}

return {
    page,
    attachments,
    shouldShowArrow: this.canUseTouchScreen,
    containerWidth: 0,
    isZoomed: false,
    activeSource: null,
};

Secondly, the FullPageNotFoundView component must be used in rendering this component, where the shouldShow prop should depend on this.state.page being equal to -1.

We should define a translations key for a more specific error message than Oops, this page cannot be found, such as Oops, this **attachment** cannot be found. In addition, we can add a new prop to FullPageNotFoundView called shouldShowBackButton and set this to false.

This should hide the following component, as it is redundant here:

App/src/components/BlockingViews/FullPageNotFoundView.js

Line 57 in f9a8529

<HeaderWithBackButton onBackButtonPress={props.onBackButtonPress} />

Lastly, the download button should be removed in the case of an error. To achieve this, we can create and pass an onAttachmentError prop to AttachmentCarousel (in AttachmentModal). This function should callback true is page equals -1 and set a state value hasAttachmentError=true in AttachmentModal.

This should then be used to conditionally hide the download button:

shouldShowDownloadButton={props.allowDownload && !state.hasAttachmentError}

App/src/components/AttachmentModal.js

Lines 290 to 307 in e753f4d

	<HeaderWithBackButton
	title={props.headerTitle || props.translate('common.attachment')}
	shouldShowBorderBottom
	shouldShowDownloadButton={props.allowDownload}
	onDownloadButtonPress={() => downloadAttachment(source)}
	shouldShowCloseButton={!props.isSmallScreenWidth}
	shouldShowBackButton={props.isSmallScreenWidth}
	onBackButtonPress={closeModal}
	onCloseButtonPress={closeModal}
	/>
	<View style={styles.imageModalImageCenterContainer}>
	{!_.isEmpty(props.report) ? (
	<AttachmentCarousel
	report={props.report}
	onNavigate={onNavigate}
	source={props.source}
	onToggleKeyboard={updateConfirmButtonVisibility}
	/>

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

@twisterdotcom Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0113703c85388c5be7

[melvin-bot]

Current assignee @twisterdotcom is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[twisterdotcom]

@mananjadhav - Looks like we have a few proposals here that might work.

[mananjadhav]

Checking those today.

[twisterdotcom]

Bump @mananjadhav. I'll reassign to another on Wednesday to give you some time. If you're a bit busy, don't worry, somebody else will surely pick it up!

[mananjadhav]

@twisterdotcom Thanks for the bump. I reviewed the proposals, and both have the correct RCA. I just need to verify the pros and cons of both. I'll have that confirmed by tomorrow.

[melvin-bot]

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

[mananjadhav]

Thanks for the patience here. Apologies folks, it takes time when we have similar proposals with edited history. I think @ahmedGaber93's proposal covered most expects from the beginning and hence we should be using their proposal.

🎀 👀 🎀 C+ reviewed.

[melvin-bot]

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

[marcochavezf]

Thanks for the review @mananjadhav, assigning @ahmedGaber93 🚀

[ahmedGaber93]

@mananjadhav

I am not aware maximum number of attachments in the chat. Are you aware @ahmedGaber93 ?

I am also not aware of that, but we already use _.findIndex through attachments here.

App/src/components/AttachmentCarousel/createInitialState.js

Line 52 in 024d210

const page = _.findIndex(attachments, (a) => a.source === props.source);

and we can simplify it to

// replace this !_.find(initialState.attachments, (a) => a.source === props.source
// with initialState.page === -1
// it calculate in createInitialState no need to calculate it again
if(_.find(attachments, (a) => a.source === props.source) && initialState.page === -1)) {


// it found in oldState   
// _.find(attachments, (a) => a.source === props.source) 

// it not found in newState   
// initialState.page === -1

[mananjadhav]

Okay this makes sense, but better to also add initialState.page === -1 on the LHS of the condition.

@marcochavezf I think the updated proposal looks good here.

@marcochavezf @twisterdotcom -
Also while we're at it, I'd like to highlight we are trying to cover following scopes:

1. Ensure that the deleted image won't crash (we worked, finished the PR)
2. Add the code to get the valid deep link to open the attachment
3. After the refactor, we're now working out a fresh out solution to get the original issue be worked upon.

I think this counts for increase in the bounty.

[twisterdotcom]

I am happy to increase to 2000 for the increased work. Can we retain @ahmedGaber93 as the assignee for this?

[melvin-bot]

Upwork job price has been updated to $2000

[mananjadhav]

Thanks @twisterdotcom. Yes we can retain @ahmedGaber93.

[mananjadhav]

I was away for the weekend, and I'll be reviewing this tomorrow.

[mananjadhav]

We're again blocked by conflicts causing additional changes. @ahmedGaber93 is working through it.

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @ahmedGaber93 got assigned: 2023-07-18 15:24:24 Z
• when the PR got merged: 2023-08-17 18:55:04 UTC
• days elapsed: 22

On to the next one 🚀

[twisterdotcom]

Apologies but I am finally going on Parental Leave. I am reassigning to another member of the team.

[melvin-bot]

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[mananjadhav]

This was deployed on production 2 days back but the title wasn't updated.

[mananjadhav]

@mallenexpensify Can you post the payout summary? This is in production for 7 days, but the title wasn't updated.

[mananjadhav]

The Attachment not found was intentionally added, and we didn't consider this a bug until we had this issue. Moreover based on multiple iterations, we moved through dismissModal, etc. but all of them were expected behaviors. Hence there is no regression PR.

But I do think we should add regression test as it involves App crash. The test steps from the PR are good for the regression steps here. wdyt @marcochavezf @mallenexpensify ?

@mallenexpensify quick bump on the previous comment for the payout summary.

[mallenexpensify]

@dhanashree-sawant can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01e88042a91e290cf6

For issue payment, I see days elapsed: 22 which would mean a 50% penalty. Is there any reason this shouldn't be applied @mananjadhav and @marcochavezf ?

TestRail GH created https://github.com/Expensify/Expensify/issues/313988

@twisterdotcom I'm off this week, can you please keep 👀 on this then I'll snag it back on Monday? Thx

[mananjadhav]

@mallenexpensify the scope of the issue was changed multiple times. Hence we increased the bounty to 2000. I don't think there would be any penalty here, and also no timeline bonus.

[mallenexpensify]

Thanks @mananjadhav

Issue reporter: @dhanashree-sawant paid $250 via Upwork
Contributor: @ahmedGaber93 paid $2000 via Upwork
Contributor+: @mananjadhav is due $2000 via NewDot.

Closing.. I think we got everything.. right?

[mananjadhav]

@mallenexpensify I think this needs to be opened until @JmillsExpensify approves it on NewDot.

[mallenexpensify]

It shouldn't need to, but I'll reopen, assign to Mills and let him close again. Per the internal StackOverflow post

If you are assigned an issue that they are the C+ on, you can close the issue as soon as the Contributor and bug reporter and regression steps have been completed.

[mananjadhav]

Okay in previous issues I've seen issues kept open. Waiting for @JmillsExpensify to process this one though.

[JmillsExpensify]

$2,000 payment approved via NewDot based on BZ summary.