[$1000] Attachment - No feedback or error message when choosing invalid image file to be sent on chat

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open the Expensify app on the web browser
2. Go to any chat conversation
3. Click on the "+" icon(actions button) of the chat
4. Click on Add attachment
5. Select an invalid or corrupted image file from the file explorer(you can create invalid image file by changing any .txt or .pdf file to .jpg simply or you can find sample invalid image attached in thread)
6. Observe the infinite loading

Expected Result:

The app should display a feedback or error message indicating that the file is not valid or just upload and send as a file without trying to display preview(if the image file is invalid)

Actual Result:

The app does not show any feedback or error message and tries to preview the image file with infinite loading

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.19.2

Reproducible in staging?: yes

Reproducible in production?: yes

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

no.feedback.mp4

Recording.2865.mp4

Expensify/Expensify Issue URL:

Issue reported by: @KALLL

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1685001824882209

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01764cde9077e5564b
• Upwork Job ID: 1664297284350144512
• Last Price Increase: 2023-07-14

[melvin-bot]

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

[allroundexperts]

Proposal

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

For corrupt files, we're showing an infinite loader in the Attachment View.

What is the root cause of that problem?

The root cause of this issue is that we're not validating the file types correctly (based on the magic headers, more about this approach in the alternate section). This causes corrupt files to go through to the AttachmentView component. Further more, in the ImageView component, we're also not using the onError callback like we're using on the PDFView component.

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

The simplest solution (without magic header check) is to add a onError handler to the ImageView component here. I'm not sure how the UI should look like exactly once error is encountered but we can keep the error in state and if its true, we can render a message here.

What alternative solutions did you explore? (Optional)

The real solution for this sort of issue IMO is to check the file type based on magic headers of the file instead of relying on the file type extracted from the name. The code for this was added in this PR some time ago but the team decided not to add frontend validation of files until another issue came up.

For reference, here's how Slack deals with a jpg extension file that is actually a text file:
https://github.com/Expensify/App/assets/30054992/bffbd3ed-6d9e-4d85-a06c-001ad31050f4

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01764cde9077e5564b

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[sonialiap]

👍 adding External label

[s-alves10]

Proposal

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

When attach an invalid image file, the AttachPreview shows an infinite loading without showing any error

What is the root cause of that problem?

We're using the Image component defined here in our ImageView component.

The root cause of this issue is that we don't use onError callback of the Image component in ImageView

App/src/components/ImageView/index.js

Lines 244 to 254 in d661708

	<Image
	source={{uri: this.props.url}}
	isAuthTokenRequired={this.props.isAuthTokenRequired}
	// Hide image until finished loading to prevent showing preview with wrong dimensions.
	style={this.state.isLoading ? undefined : [styles.w100, styles.h100]}
	// When Image dimensions are lower than the container boundary(zoomscale <= 1), use `contain` to render the image with natural dimensions.
	// Both `center` and `contain` keeps the image centered on both x and y axis.
	resizeMode={this.state.zoomScale > 1 ? Image.resizeMode.center : Image.resizeMode.contain}
	onLoadStart={this.imageLoadingStart}
	onLoad={this.imageLoad}
	/>

App/src/components/ImageView/index.js

Lines 283 to 290 in d661708

	<Image
	source={{uri: this.props.url}}
	isAuthTokenRequired={this.props.isAuthTokenRequired}
	style={[styles.h100, styles.w100]}
	resizeMode={Image.resizeMode.contain}
	onLoadStart={this.imageLoadingStart}
	onLoad={this.imageLoad}
	/>

In addition, we didn't explicitly add onError props to its propTypes here

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

We have to add onError props to the Image component explicitly and use it in our ImageView component. In onError callback, we need to do something like

    this.setState({isLoading: false, loadingError: 'Failed to load image'});

and show error message instead of loading indicator.

Result

mac_chrome_19728.mp4

What alternative solutions did you explore? (Optional)

[marcaaron]

@aimane-chnaif can you take a look at the proposals so far?

[aimane-chnaif]

The root cause is correct in both proposals.
Showing user feedback on error callback is simple solution but will not fix issue completely. After sending attachment, nothing shows in chat, so we might want to fix that as well here.
I tend to agree with @allroundexperts's alternative solution as a permanent solution. So add logic of detecting real file type, not just from file name and extension. This will be complex solution and may require backend update as well.
@marcaaron what do you suggest?

[s-alves10]

@aimane-chnaif
We're using Image component of react-native and I think we should use error handling of the component. This is good for consistency as well.

What if a file is marked valid in our engine but can't be shown in Image view? Or inverse case?
We can show error message for Image can't render: Invalid or not supported image format

[aimane-chnaif]

What if a file is marked valid in our engine but can't be shown in Image view

@s-alves10 I can't think of that case but we can show placeholder icon. That will be consistent with preview after sent to chat.
I checked other social platforms (slack, skype, whatsapp), but don't see any error message. They treat this either as normal file, or as image but showing placeholder or "No preview available".

Btw your proposal is already a dupe and won't be accepted unless you have better solution.

[melvin-bot]

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

[marcaaron]

@aimane-chnaif can you summarize what you think we should do for this one? When you believe we're ready to move forward with something I will share my thoughts on that proposal, thanks!

[KALLL]

Proposal

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

The bug is that when the user attaches an image file that is invalid or corrupted, the app tries to display a preview of the image and takes forever. The expected behavior is that the app should display a feedback or error message indicating that the file is not valid, or just upload and send the file as a file without trying to display a preview.

What is the root cause of that problem?

The root cause of the problem is we are not handling onError well on the

App/src/components/ImageView/index.js

Lines 244 to 254 in d661708

	<Image
	source={{uri: this.props.url}}
	isAuthTokenRequired={this.props.isAuthTokenRequired}
	// Hide image until finished loading to prevent showing preview with wrong dimensions.
	style={this.state.isLoading ? undefined : [styles.w100, styles.h100]}
	// When Image dimensions are lower than the container boundary(zoomscale <= 1), use `contain` to render the image with natural dimensions.
	// Both `center` and `contain` keeps the image centered on both x and y axis.
	resizeMode={this.state.zoomScale > 1 ? Image.resizeMode.center : Image.resizeMode.contain}
	onLoadStart={this.imageLoadingStart}
	onLoad={this.imageLoad}
	/>

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

• Import the icon.png file

import icon from '../images/placeholder-image.png';

• Define a state variable that will store the real file

this.state = {
  realFile: null
}

• Define a function that will handle the file selection

handleFileSelect = (event) => {
  // Get the selected file from the event
  const file = event.target.files[0];
  // Set the realFile state to the selected file
  this.setState({realFile: file});
  // Set the image source to the file URL
  this.setState({imageSource: {uri: URL.createObjectURL(file)}});
}

• Define a function that will handle the image error

handleImageError = () => {
  // Set the image source to placeholder-image.png
  this.setState({imageSource: icon});
}

• Modify the function that will handle the file upload to assign realFile

handleFileUpload = () => {
  // Get the real file from the state
  const file = this.state.realFile;
  // Check if the file is not null
  if (file) {
    // Upload the file using your preferred method
}    
  }

<input type="file" onChange={this.handleFileSelect} />

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[melvin-bot]

This issue has not been updated in over 15 days. @MariaHCD, @sonialiap, @aimane-chnaif eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[sonialiap]

@youssef-lr I am not sure how to check, has this PR been opened to contributors? #23103 (comment)

[melvin-bot]

@MariaHCD, @sonialiap, @aimane-chnaif Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@MariaHCD, @sonialiap, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@MariaHCD, @sonialiap, @aimane-chnaif 12 days overdue now... This issue's end is nigh!

[aimane-chnaif]

PR is in review. @youssef-lr is OOO until the end of Nov

[melvin-bot]

@MariaHCD, @sonialiap, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@MariaHCD, @sonialiap, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@MariaHCD, @sonialiap, @aimane-chnaif 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[aimane-chnaif]

Hopefully @youssef-lr will finish PR soon

[melvin-bot]

@MariaHCD, @sonialiap, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@MariaHCD, @sonialiap, @aimane-chnaif Eep! 4 days overdue now. Issues have feelings too...

[youssef-lr]

Sorry, I'm working on higher priority issues. But I should pick this up again either this week or the next one.

[melvin-bot]

This issue has not been updated in over 15 days. @youssef-lr, @sonialiap, @aimane-chnaif eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[melvin-bot]

@youssef-lr, @sonialiap, @aimane-chnaif, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.