Fix: Attachment Carousel not displaying images from Concierge

[kidroca]

Details

This Pull Request introduces a series of changes to improve the handling and downloading of attachments in our application. The main changes focus on the AttachmentCarousel and AttachmentModal components.

Here are the main changes:

1. Expanded compatibility of AttachmentCarousel with images originating from various sources, not only Expensify-related ones. This was done by updating the component's attachment parsing and state creation logic. This change fixes the issue where some attachments (for instance, Concierge ones) were not appearing in the carousel.

2. Updated error handling in the AttachmentCarousel component. If a user tries to open an image that cannot be mapped to available content, the application now shows an error page instead of a different image. This change enhances user experience by clearly indicating when there's a problem opening an image.

3. Refactored the AttachmentModal to manage the state of the source and isAuthTokenRequired attributes of the attachment, ensuring that they're in sync with the attachment displayed in the carousel.

4. Modified the AttachmentModal to decide whether to append auth-related parameters to an attachment source during download, based on the isAuthTokenRequired flag.

5. Removed the responsibility of appending auth parameters from the AttachmentCarousel component. This change makes the component more reusable and focused on its core functionality.

Overall, these changes should provide a more reliable and consistent attachment downloading experience, and avoid scattering logic across components.

Fixed Issues

$ #18150
PROPOSAL: #18150 (comment)

Tests

Test 1: Verify Existing Functionality is Unaltered

Objective: To ensure that the implemented changes haven't affected the existing functionality.

1. Navigate to a chat with multiple attachments.
2. Open different types of attachments (e.g., images, documents), and verify that they are correctly displayed in the carousel.
3. Download a variety of attachments and check that the downloading functionality remains intact.

Expected Outcomes:

• Each attachment should open and display correctly when selected from the chat.
• Navigation between attachments in the carousel should be smooth, with attachments appearing in the correct order.
• All downloads should proceed as before, with no new issues introduced.

Note: This PR does not address existing known issues such as the carousel occasionally displaying the wrong image (as detailed in #18150 (comment)).

Test 2: Concierge Attachments

Objective: To verify that attachments originating from Concierge are correctly identified and displayed.

1. Navigate to a chat with Concierge.

2. Request sample attachments for testing. Ask for multiple images in a single message, another message with a single image, and a third message with multiple attachments.

   Sample message from Concierge feature an attachments

   Lorem ipsum<br />
   <br />
   More lorem<br />
   <br />
   This lorem is only more lorem<br />
   <br />
   Lorem ipsum attachment sit<br />
   <br />
   <img src="https://s3-us-west-1.amazonaws.com/concierge-responses-expensify-com/uploads%2sample.png" 
      style="width:300px;" class="fr-fic fr-dib" alt="uploads%2sample-sample.png" /><br />
   <img src="https://s3-us-west-1.amazonaws.com/concierge-responses-expensify-com/uploads%2sample2.png" 
      style="width:300px;" class="fr-fic fr-dib" alt="uploads%2sample-sample2.png" />

Expected Outcomes:

• All Concierge attachments should open and display correctly.
• When navigating between attachments, they should appear in the correct order in the Attachment Carousel.
• All attachments should be downloadable. Note that due to CORS, locally these might open in a new window.

Test 3: Error Handling When Carousel Fails to Locate Selected Attachment Source

Objective: To verify that when the Carousel fails to initialize correctly, it handles the error in a predictable manner, displaying an error page instead of an incorrect image.

This test involves programmatically forcing incorrect data into the attachment carousel.

1. Open ImageRenderer and set an arbitrary string as the modal source (see

   App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js

   Line 57 in 2a786a7

   source={source}

   ).
2. Navigate to a chat and open an attachment.

Expected Outcomes:

• If the carousel is unable to present the attachment, the "Uh-oh, something went wrong!" error boundary page should be displayed.

Offline tests

The offline functionality should be unaffected by the changes introduced in this PR.

1. Attachments that were opened recently while being online should still load when offline.
2. Attachments that haven't been opened while online, or those that were opened more than 2 hours ago, are expected to fail to load when offline.

QA Steps

These steps are applicable to all platforms and both the staging and the production environments

Test 1: Verify Existing Functionality is Unaltered

Objective: To ensure that the implemented changes haven't affected the existing functionality.

1. Navigate to a chat with multiple attachments.
2. Open different types of attachments (e.g., images, documents), and verify that they are correctly displayed in the carousel.
3. Download a variety of attachments and check that the downloading functionality remains intact.

Expected Outcomes:

• Each attachment should open and display correctly when selected from the chat.
• Navigation between attachments in the carousel should be smooth, with attachments appearing in the correct order.
• All downloads should proceed as before, with no new issues introduced.

Note: This PR does not address existing known issues such as the carousel occasionally displaying the wrong image (as detailed in #18150 (comment)).

Test 2: Concierge Attachments

Objective: To verify that attachments originating from Concierge are correctly identified and displayed.

1. Navigate to a chat with Concierge.
2. Request sample attachments for testing. Ask for multiple images in a single message, another message with a single image, and a third message with multiple attachments.

Expected Outcomes:

• All Concierge attachments should open and display correctly.
• When navigating between attachments, they should appear in the correct order in the Attachment Carousel.
• All attachments should be downloadable. Note that due to CORS, locally these might open in a new window.

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))
• If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-05-28.at.15.56.25.mov

Mobile Web - Chrome

Studio_Project_V1.mp4

Mobile Web - Safari

Studio_Project_V1.mp4

Desktop

Screen.Recording.2023-05-28.at.16.22.19.mov

iOS

Android

[kidroca]

I'll add Android samples on Monday

In the meantime this PR is ready for review ✅

[melvin-bot]

@hayata-suenaga @eVoloshchak One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[eVoloshchak]

Reviewer Checklist

• I have verified the author checklist is complete (all boxes are checked off).
• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the steps for local testing are in the Tests section
  • I verified the steps for Staging and/or Production testing are in the QA steps section
  • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I checked that screenshots or videos are included for tests on all platforms
• I included screenshots or videos for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-05-31.at.16.08.19.mov

Mobile Web - Chrome

screen-20230531-163237.mp4

Mobile Web - Safari

Screen.Recording.2023-05-31.at.16.22.21.mov

Desktop

Screen.Recording.2023-05-31.at.16.17.54.mov

iOS

Screen.Recording.2023-05-31.at.16.20.52.mov

Android

screen-20230531-162958.mp4

[eVoloshchak]

Found an issue on Android. I couldn't get Concierge to send me a bunch of images, so just hardcoded your sample message as the last message from Concierge

Sample message

Lorem ipsum<br />
<br />
More lorem<br />
<br />
This lorem is only more lorem<br />
<br />
Lorem ipsum attachment sit<br />
<br />
<img src="https://2.img-dpreview.com/files/p/E~C1000x0S4000x4000T1200x1200~articles/3925134721/0266554465.jpeg" 
  style="width:300px;" class="fr-fic fr-dib" alt="uploads%2sample-sample.png" /><br />
<img src="https://media.istockphoto.com/id/1096052566/vector/stamprsimp2red.jpg?s=612x612&w=0&k=20&c=KVu0nVz7ZLbZsRsx81VBZcuXZ1MlEmLk9IQabO2GkYo=" 
  style="width:300px;" class="fr-fic fr-dib" alt="uploads%2sample-sample2.png" />

This tests well on all of the platforms except Android.

1. Open an image in the Concierge conversation
2. Navigate to the next image
3. Notice the infinite loading spinner

It starts/stops working each time you close/open the carousel

screen-20230530-160658.mp4

[kidroca]

Found an issue on Android.

Thank you for the context on this issue. I've noticed a similar problem with infinite loading on Android, but I thought it was some problem in the local environment and the emulator not having full internet access

If this bug doesn't occur on mWeb Android, then it could likely be related to the Fast Image library that's being utilized internally for image loading.

I've found a potential correlation with this existing issue:

• [HOLD Expensify/react-native-fast-image#6] [$1000] Attachment preview keeps loading forever when cycle between image and pdf #16046

Although in your case, the problem doesn't involve cycling between image and PDF formats.

---

Considering the fact that I haven't made any modifications to the loading code, and the attachment does occasionally display correctly, we can infer that the issue hasn't arisen as a result of changes made by me. Here are my reasons:

• If the image URL mapping was incorrect on my end, it would have resulted in a malfunction across all platforms.
• Any other potential causes might be related to the loading logic, which I haven't tampered with.

[eVoloshchak]

I agree this isn't caused by this PR, we've encountered similar problems with Android in #17454

I think the issue we're experiencing might be related to #17892

[eVoloshchak]

Looks good and tests well
cc: @hayata-suenaga

[hayata-suenaga]

LGTM!

[OSBotify]

🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.3.21-1 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[trjExpensify]

QA'ing this one:

Test 1: Verify Existing Functionality is Unaltered
✅ Chrome Web - https://recordit.co/BjY8ejAY6L
✅ Desktop - https://recordit.co/WrGQ5tGfI9
✅ mWeb Safari - https://recordit.co/tqqpPKKOiz
NAB: PDF's are a little slow to load, but unrelated to these changes I think
✅ iOS native - https://github.com/Expensify/App/assets/16232057/b0eaa59e-5fd2-4b77-b2f1-b1cadb5069eb
✅ Android - https://recordit.co/n5VrTgWnQh

---

Test 2: Concierge Attachments
✅ Chrome Web - https://recordit.co/7oDJoTTZyg
✅ Desktop - https://recordit.co/neCuFUvGKW
❌ mWeb Safari - can't seem to download the attachments. https://github.com/Expensify/App/assets/16232057/79dd7048-d035-40db-91cb-8e48b5cddcae
✅ iOS native - https://github.com/Expensify/App/assets/16232057/08472b57-dc80-49d0-9f97-8bab6b420b05
❌ Android - error when attempting to download image files. https://recordit.co/2MIKsGejSZ

Blocker, or expected?
mp4 video sent by Concierge displays as "Your browser does not support HTML5 video." on the receivers side on all platforms in the NewDot Concierge chat:

[AndrewGable]

@trjExpensify @kidroca - Can you please look at this in a follow up? We decided internally it's not a blocker, but we should make this work.

[OSBotify]

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.21-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[kidroca]

Hi @AndrewGable,

It appears from your message that you would like me to make a follow-up Pull Request, however, I need some clarification regarding what "this" pertains to in your message.

From the issues Tom discovered, none of them seem to be regressions from this PR. I hope you confirmed that the message, "Your browser does not support HTML5 video," was present even before merging any of my changes to production. I've also done a brief review and concluded that we're looking at three separate issues:

1. Video thumbnails not appearing in Concierge chat - No changes have been made in the logic that handles the rendering of thumbnails.
2. Safari not being able to download attachments - During testing, I'd left a note highlighting that, due to CORS, downloads might open in a new window locally. However, on reviewing, it appears that the App and/or S3 bucket from which Concierge shares attachments might not be handling CORS downloads properly.
3. Android error when downloading images - This could possibly be an oversight on my part. Concierge attachments do not have an attribute for the filename, so I included logic to deduce it from the URL. It seems I didn't anticipate the last part of the URL containing characters that can't be used in filenames.

Out of these, only the third can be directly linked as follow-up work on my end. I suggest we create separate tickets for each issue to manage them effectively. I'll promptly verify the Android issue as soon as I can.

While I'm happy to assist with resolving the remaining issues, I should mention that I'm not a full-time employee on this project. Thus, if it's essential that I handle these personally, it may take longer than usual to resolve them all.

Your understanding and guidance would be highly appreciated.

[trjExpensify]

Hey man, hope you're good!

Android error when downloading images - This could possibly be an oversight on my part. Concierge attachments do not have an attribute for the filename, so I included logic to deduce it from the URL. It seems I didn't anticipate the last part of the URL containing characters that can't be used in filenames.

If you could focus on confirming this one, as it seems like a potential regression on Android native from this PR.

Video thumbnails not appearing in Concierge chat - No changes have been made in the logic that handles the rendering of thumbnails.

Just to confirm, I don't think this is a problem with thumbnails. Rather if you receive a mp4 file from Concierge, it just shows "Your browser does not support HTML5 video." in written text. Still might not be related to this PR for sure, but did want to confirm that it's not a thumbnail issue I don't think.

[hayata-suenaga]

@kidroca thank you so much for your detailed explanation

@trjExpensify Could you report the first two issues discovered in the #expensify-open-source channel so that GH issues are created for them and triaged there? I believe other contributors can handle them

[hayata-suenaga]

@trjExpensify ah I didn't see your previous message

[trjExpensify]

Yeah, all good. I was in the process of creating the mWeb safari and mp4 issues here:

#19965
#19964

[kidroca]

Android error when downloading images - This could possibly be an oversight on my part. Concierge attachments do not have an attribute for the filename, so I included logic to deduce it from the URL. It seems I didn't anticipate the last part of the URL containing characters that can't be used in filenames.

If you could focus on confirming this one, as it seems like a potential regression on Android native from this PR.

I confirm this, I can post a fix about it tomorrow

Video thumbnails not appearing in Concierge chat - No changes have been made in the logic that handles the rendering of thumbnails.

Just to confirm, I don't think this is a problem with thumbnails. Rather if you receive a mp4 file from Concierge, it just shows "Your browser does not support HTML5 video." in written text. Still might not be related to this PR for sure, but did want to confirm that it's not a thumbnail issue I don't think.

I haven't changed logic receiving attachments from Concierge either.

My theory is that there's a <video> tag inside that message, instead of an anchor tag to a downloadable file
I don't think our HTML parser handles <video> tags, and it might render something weird as "Your browser does not support HTML5 video." in place of the <video> tag

[trjExpensify]

I confirm this, I can post a fix about it tomorrow

Awesome, thanks!

I haven't changed logic receiving attachments from Concierge either.

understood!

My theory is that there's a

👍 Thanks for jumping into that other issue to look at repro'ing it.