fix: Add fallback icons for thumbnails #36214

paultsimura · 2024-02-09T09:55:05Z

Details

Prevent the infinite loading for broken thumbnail images and show a fallback icon instead.

Fixed Issues

$ #34601
PROPOSAL: #34601 (comment)

Tests

Same as QA

Offline tests

Same as QA

QA Steps

Go to any chat
Send a message
Open react dev tools > components
Select the report action item message component for that chat
Manually edit the action prop: message.html, and set it to this string <img src=\"a%20href=\" alt=\"a href=\" />
Verify the image shows the "image" fallback icon when online, and "cloud" when offline.

For mobile browsers:

Send multiple attachments from user B to user A
As user A, go offline & disable cache
Open the conversation with user B
Verify the images show the "cloud" fallback icon when offline.

PR Author Checklist

Screenshots/Videos

Android: Native

Untitled-compressed.mp4

Android: mWeb Chrome

chrome-compressed.mp4

iOS: Native

Screen.Recording.2024-02-13.at.16.06.27-compressed.mp4

iOS: mWeb Safari

safari-compressed.mp4

MacOS: Chrome / Safari

Screen.Recording.2024-02-12.at.11.58.51-compressed.mp4

MacOS: Desktop

Screen.Recording.2024-02-14.at.00.51.36-compressed.mp4

paultsimura · 2024-02-09T09:56:04Z

Leaving this here for reference:

paultsimura · 2024-02-09T10:22:21Z

Hey @dannymcclain, I'd like to clarify a couple of things here:

Do we want to keep the default thumbnails of the fixed size, or to adjust them to the image size? Even though the attachment might not be loaded, when uploading an attachment we set html meta attributes with the image size.

For testing purposes, I show fallback when offline here, so you can see this approach keeps the thumbnails in fixed size while switching from a fallback to the real image:

Screen.Recording.2024-02-09.at.11.00.11.mov

However, this might look quite ugly – every thumbnail is of a different size.
What would you say?

paultsimura · 2024-02-09T10:26:14Z

Also, in your mockup I see you don't use any border – it will make the thumbnail blend in with the report action row on hover:

However, they do look better without the border 🙂

WDYT @dannymcclain ?

dannymcclain · 2024-02-09T15:00:29Z

@paultsimura oh yeah good catch on the border—I think we should definitely include the border.

As for this comment about the different sized thumbnails—I think we should do what you're doing and keep the fallback the same size as the thumbnail. I'm curious for the @Expensify/design team's thoughts but that makes the most sense to me, even if we end up with a bunch of different sized thumbnails. It feels more accurate and will likely create a smoother transition when going from offline > online.

shawnborton · 2024-02-09T17:18:31Z

Yup, makes sense to me - I agree with Danny.

dubielzyk-expensify · 2024-02-12T00:10:54Z

Agreed. If it helps with reducing the jumping between offline and online, I'm all for it

# Conflicts: # src/components/ReportActionItem/ReportActionItemImage.tsx # src/components/ReportActionItem/ReportActionItemImages.tsx

paultsimura · 2024-02-12T11:30:14Z

Hey @parasharrajat, this one is indeed tricky to test – I've collected some reproduction steps from the related bugs, but all of them are web-only, where you can edit the image source manually or disable caches. I cannot find a way to manually force an image to fail in the current system anymore (since the Distance receipt was fixed).

e.g. #36061

cc: @quinthar as you've reported the issue I mentioned above – is there a way to force Concierge Compose to send a broken image? Or should we just use the steps you provided (manually modifying the source) to test the flow, and that's it?

melvin-bot · 2024-02-12T13:00:29Z

@parasharrajat Please 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]

paultsimura · 2024-02-13T23:53:51Z

@parasharrajat all yours ✔️

allroundexperts · 2024-02-14T19:53:39Z

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: mWeb Chrome
- iOS: Native
- iOS: mWeb 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(theme.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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label so the design team can review the changes.
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

Android: Native

Android: mWeb Chrome

Screen.Recording.2024-02-15.at.2.10.02.AM.mov

iOS: Native

Screen.Recording.2024-02-15.at.2.06.56.AM.mov

iOS: mWeb Safari

MacOS: Chrome / Safari

Screen.Recording.2024-02-15.at.1.11.59.AM.mov

MacOS: Desktop

Screen.Recording.2024-02-15.at.1.20.25.AM.mov

neil-marcellini · 2024-02-14T19:53:40Z

This is related to a fire/urgent issue, so I asked for a new C+ to test ASAP in Slack here

neil-marcellini

Looks great, pending C+ testing.

neil-marcellini · 2024-02-14T20:01:10Z

src/components/ImageWithSizeCalculation.tsx

    const onError = () => {
        Log.hmmm('Unable to fetch image to calculate size', {url});
+        onLoadFailure?.();


NAB: Should we provide default behavior to display a fallback image in this component?

allroundexperts · 2024-02-14T20:35:42Z

On iOS Safari, the cloud icon never appears despite disabling cache.

Screen.Recording.2024-02-15.at.1.33.13.AM.mov

paultsimura · 2024-02-14T20:46:11Z

@allroundexperts for some reason, the "force offline" doesn't trigger this behavior on mWeb Safari for me either. However, if you turn off the wi-fi on your Mac, this will work. You could see the cloud icons on my safari recordings

allroundexperts · 2024-02-14T20:54:23Z

Weird enough, still doesn't work.

Screen.Recording.2024-02-15.at.1.52.44.AM.mov

aldo-expensify · 2024-02-14T21:00:05Z

Weird enough, still doesn't work.

I think it is an issue with mac / ios simulator, it is not picking up that you are offline even if you disabled the wifi in the mac. If it was detecting offline, you would have seen the message "You appear to be offline" in the bottom, and that is not seen in your video.

allroundexperts · 2024-02-14T21:02:56Z

Okay. Will continue on other devices for the meantime.

paultsimura · 2024-02-14T21:09:35Z

@allroundexperts here's a fresh recording, maybe try the same sequence as I do: disable cache -> refresh page -> kill the wi-fi -> open the chat:

Screen.Recording.2024-02-14.at.22.07.27-compressed.mp4

allroundexperts · 2024-02-14T21:27:29Z

Still the same @paultsimura. I guess we can trust you on this one?

Screen.Recording.2024-02-15.at.2.19.52.AM.mov

paultsimura · 2024-02-14T21:30:07Z

Still the same @paultsimura. I guess we can trust you on this one?

This is super weird🤔 I guess the decision is up to you and the assigned Engineer

aldo-expensify · 2024-02-14T21:31:19Z

I'll give it a try with testing IOS, @allroundexperts you may continue with the rest 🙏

aldo-expensify · 2024-02-14T21:38:11Z

An easy way to test this in dev is by replacing the lines:

--    // const {html = '', text} = fragment;
++    const html = '<img src="a%20href=" alt="a href=" />';
++    const text = 'attachment';

here

App/src/pages/home/report/comment/TextCommentFragment.tsx

Line 43 in 2254c66

const {html = '', text} = fragment;

allroundexperts · 2024-02-14T21:39:03Z

Is BetaSignIn command working for you all?

aldo-expensify · 2024-02-14T21:43:46Z

Is BetaSignIn command working for you all?

We are having site stability issues, so you probably will have problems talking to staging/production right now.

allroundexperts · 2024-02-14T21:44:38Z

Gotcha. Thanks.

aldo-expensify · 2024-02-14T21:49:03Z

IOS Web seems to be working

IOS Native seems to be working

allroundexperts · 2024-02-14T21:52:01Z

@aldo-expensify I've tested iOS native already and it works.

aldo-expensify · 2024-02-14T21:53:03Z

@allroundexperts thanks, I ended up being able to test web and native in ios and both worked for me. Please finish the checklist so we can get this merged :)

fix: Add fallback icons for thumbnails (cherry picked from commit 5a14fe1)

OSBotify · 2024-02-14T22:01:44Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-02-14T22:33:36Z

🚀 Cherry-picked to staging by https://github.com/thienlnam in version: 1.4.41-9 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2024-02-15T05:25:28Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.41-12 🚀

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

Add fallback icons for thumbnails

303ee7d

paultsimura added 4 commits February 12, 2024 09:19

Merge branch 'main' into fix/34601-thumbnails-fallback

39613d7

# Conflicts: # src/components/ReportActionItem/ReportActionItemImage.tsx # src/components/ReportActionItem/ReportActionItemImages.tsx

Add fallback icons for thumbnails

3dc9429

Add fallback icons for thumbnails

acede78

Add comments

2d37f7e

paultsimura marked this pull request as ready for review February 12, 2024 13:00

paultsimura requested a review from a team as a code owner February 12, 2024 13:00

melvin-bot bot removed the request for review from a team February 12, 2024 13:00

melvin-bot bot requested a review from parasharrajat February 12, 2024 13:00

Merge branch 'main' into fix/34601-thumbnails-fallback

b10d058

neil-marcellini requested review from allroundexperts and removed request for parasharrajat February 14, 2024 19:54

neil-marcellini approved these changes Feb 14, 2024

View reviewed changes

aldo-expensify merged commit 5a14fe1 into Expensify:main Feb 14, 2024
15 of 16 checks passed

OSBotify pushed a commit that referenced this pull request Feb 14, 2024

Merge pull request #36214 from paultsimura/fix/34601-thumbnails-fallback

d87ed87

fix: Add fallback icons for thumbnails (cherry picked from commit 5a14fe1)

github-actions bot mentioned this pull request Feb 14, 2024

Deploy Checklist: New Expensify 2024-02-13 #36429

Closed

paultsimura mentioned this pull request Feb 15, 2024

[HOLD for payment 2024-02-26][$500] [MEDIUM] Erroring thumbnail images try to reload infinitely #34601

Closed

6 tasks

tienifr mentioned this pull request Mar 11, 2024

Fix: show loading indicator for uncached attachments #35451

Merged

56 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add fallback icons for thumbnails #36214

fix: Add fallback icons for thumbnails #36214

paultsimura commented Feb 9, 2024 •

edited

Loading

paultsimura commented Feb 9, 2024

paultsimura commented Feb 9, 2024

paultsimura commented Feb 9, 2024

dannymcclain commented Feb 9, 2024

shawnborton commented Feb 9, 2024

dubielzyk-expensify commented Feb 12, 2024

paultsimura commented Feb 12, 2024

melvin-bot bot commented Feb 12, 2024

paultsimura commented Feb 13, 2024

allroundexperts commented Feb 14, 2024 •

edited

Loading

neil-marcellini commented Feb 14, 2024

neil-marcellini left a comment

neil-marcellini Feb 14, 2024

allroundexperts commented Feb 14, 2024

paultsimura commented Feb 14, 2024

allroundexperts commented Feb 14, 2024

aldo-expensify commented Feb 14, 2024

allroundexperts commented Feb 14, 2024

paultsimura commented Feb 14, 2024

allroundexperts commented Feb 14, 2024

paultsimura commented Feb 14, 2024

aldo-expensify commented Feb 14, 2024

aldo-expensify commented Feb 14, 2024

allroundexperts commented Feb 14, 2024

aldo-expensify commented Feb 14, 2024

allroundexperts commented Feb 14, 2024

aldo-expensify commented Feb 14, 2024 •

edited

Loading

allroundexperts commented Feb 14, 2024

aldo-expensify commented Feb 14, 2024

OSBotify commented Feb 14, 2024

OSBotify commented Feb 14, 2024

OSBotify commented Feb 15, 2024

fix: Add fallback icons for thumbnails #36214

fix: Add fallback icons for thumbnails #36214

Conversation

paultsimura commented Feb 9, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

paultsimura commented Feb 9, 2024

paultsimura commented Feb 9, 2024

paultsimura commented Feb 9, 2024

dannymcclain commented Feb 9, 2024

shawnborton commented Feb 9, 2024

dubielzyk-expensify commented Feb 12, 2024

paultsimura commented Feb 12, 2024

melvin-bot bot commented Feb 12, 2024

paultsimura commented Feb 13, 2024

allroundexperts commented Feb 14, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

neil-marcellini commented Feb 14, 2024

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini Feb 14, 2024

Choose a reason for hiding this comment

allroundexperts commented Feb 14, 2024

paultsimura commented Feb 14, 2024

allroundexperts commented Feb 14, 2024

aldo-expensify commented Feb 14, 2024

allroundexperts commented Feb 14, 2024

paultsimura commented Feb 14, 2024

allroundexperts commented Feb 14, 2024

paultsimura commented Feb 14, 2024

aldo-expensify commented Feb 14, 2024

aldo-expensify commented Feb 14, 2024

allroundexperts commented Feb 14, 2024

aldo-expensify commented Feb 14, 2024

allroundexperts commented Feb 14, 2024

aldo-expensify commented Feb 14, 2024 • edited Loading

allroundexperts commented Feb 14, 2024

aldo-expensify commented Feb 14, 2024

OSBotify commented Feb 14, 2024

OSBotify commented Feb 14, 2024

OSBotify commented Feb 15, 2024

paultsimura commented Feb 9, 2024 •

edited

Loading

allroundexperts commented Feb 14, 2024 •

edited

Loading

aldo-expensify commented Feb 14, 2024 •

edited

Loading