Fix - Attachment page keeps on loading infinitely. #39290

FitseTLT · 2024-03-29T19:52:55Z

Details

Fixed Issues

Tests

Via Fab add image attachment in any chat.
After the image loads
Switch off connection on your device (note that offline mode under Troubleshoot section doesn't disallow the loading of images so you have to disconnect your device)
clear image data cache on your browser

switch to another chat and switch back to the previous chat via LHN or search (but don't use the browser back navigator)
Verify that the image preview shows offline cloud icon instead of infinite loading indicator.
press on the preview, and check that the offline indicator and also a message that the user is offline is displayed.
Again repeat these steps for video attachment but for video, you don't even need to clear cache data.
Now create request with receipt.
Wait until the image loads from the BE (one sign to know that is, it blinks when the image is loaded from server)
Now clear image data cache on your browser.
switch to another chat and switch back to the request chat via LHN or search (but don't use the browser back navigator)
Verify that the receipt preview shows offline cloud icon instead of infinite loading indicator in all report preview, money request preview and also money request detail view.

Alternatively for platforms where it is hard to clear the cache, easier way to test would be: send attachments from one user to another and on the receiving side without opening the chat (as the image will load if you open it) when you see the attachment message is received (of course by seeing the boldness) disconnect your device's connection and open the chat 👍

Verify that no errors appear in the JS console

Offline tests

same here

QA Steps

same here

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

nat.mp4

Android: mWeb Chrome

2024-04-11.15-50-48.mp4

iOS: Native

2024-04-15.14-50-46.mp4

iOS: mWeb Safari

ios.web.mp4

MacOS: Chrome / Safari

web.mp4

MacOS: Desktop

desk.mp4

FitseTLT · 2024-03-29T20:00:43Z

@shawnborton I want You to confirm some graphics staff before I proceed.

Do you think we should indicate the offline indicator for attachement previews too (in the chat)??
If Yes, we need to tweek some css settings like font-sizes for the title and subtitle, icon size Could you provide me ??

shawnborton · 2024-04-01T05:25:19Z

Do you think we should indicate the offline indicator for attachement previews too (in the chat)??

cc @Expensify/design (particularly @dannymcclain ) but I think we already have a pattern for this, right? We just show a little broken cloud image, not the full text empty state like you are showing. Can you test this as it already exists today and report back?

dannymcclain · 2024-04-01T13:53:10Z

@shawnborton agree 100%. Here's the mock we used when establishing that pattern—I just can't remember if we decided to go with the Offline version or if we decided to just use the Didn't Load version for everything (offline, failed loading, etc.) But hopefully it's already implemented and we won't need to mess with anything.

shawnborton · 2024-04-01T13:54:48Z

Dope, I knew we had somethin' - thanks for sharing!

FitseTLT · 2024-04-04T20:55:26Z

@shawnborton Here is how it looks for preview and attachement view for web and also mobile. Need confirmation from U before finalizing.

shawnborton · 2024-04-05T06:53:39Z

Looks good to me, and just to confirm, the only behavior you changed was in the actual attachment modal view itself right?

shawnborton · 2024-04-05T06:54:21Z

Just going back to this question here:

Do you think we should indicate the offline indicator for attachement previews too (in the chat)??

Do we actually need to implement anything new here, or do we already have the patterns that Danny shared implemented?

FitseTLT · 2024-04-05T12:26:13Z

Looks good to me, and just to confirm, the only behavior you changed was in the actual attachment modal view itself right?

yep.

Do we actually need to implement anything new here, or do we already have the patterns that Danny shared implemented?

What danny refered is the one U guys implemented for preview of loading failed receipt so technically I had to do similar thing for attachement preview but graphically it is the same as U saw it in the snapshot it's just OfflineCloud Icon centered in the preview box and I think U are ok with what u saw in the snapshots.

dannymcclain · 2024-04-05T13:15:29Z

Just a quick note—in this image/scenario:

The icon color should be borders / product-400 (instead of icons / product-700):

FitseTLT · 2024-04-05T13:29:47Z

Ok Thx. And also when I make the PR ready I will notify U guys to take a look at the final snapshots 👍

FitseTLT · 2024-04-05T19:51:06Z

@shawnborton @dannymcclain I have updated the icon color and also bg as indicated in your snapshots and now it looks like this, do you approve???

2024-04-05.22-48-34.mp4

shawnborton · 2024-04-08T06:20:26Z

I think that looks right to me, but let's see what @dannymcclain thinks

dannymcclain · 2024-04-08T13:52:37Z

Yeah I think that looks good 👍

mollfpr · 2024-04-30T09:06:35Z

Mostly looks good and the issue is fixed. Just a small improvement we can make when uploading the attachment offline.

Screen.Recording.2024-04-30.at.16.00.35.mp4

As you can see in the video, the offline caption is showing for a moment before it shows the image. What currently happens in the production is it's showing the loading indicator. The bevahior in the production looks fine, but showing the offline caption is weird to me.

So probably we can either not show the offline caption or show the loading indicator.

We might need a designer's opinion on the expected behavior @shawnborton

shawnborton · 2024-04-30T12:27:22Z

Yeah, good catch on that - I agree the quick flash is weird, so perhaps we just show the loading indicator there for that quick moment or nothing at all.

trjExpensify · 2024-05-03T10:41:50Z

👋 Just wondering if this PR fixes this issue: #16694 CC: @Beamanator

FitseTLT · 2024-05-07T13:00:30Z

@mollfpr last bug is fixed. You can proceed.

mollfpr · 2024-05-07T13:01:47Z

Thanks @FitseTLT! I will review and test shortly.

mollfpr · 2024-05-11T14:33:43Z

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 either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- 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 UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design 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

39290.Android.mov

Android: mWeb Chrome

39290.mWeb-Chrome.mp4

iOS: Native

39290.iOS.mov

iOS: mWeb Safari

39290.mWeb-Safari.mov

MacOS: Chrome / Safari

39290.Web.mp4

MacOS: Desktop

39290.Desktop.mp4

trjExpensify · 2024-05-13T10:37:23Z

@mollfpr @FitseTLT can you answer this, please? #39290 (comment)

mollfpr · 2024-05-13T10:59:15Z

@trjExpensify Sorry I miss the comment, I'll check it now.

marcochavezf

LGTM, thanks guys!

mollfpr · 2024-05-14T14:08:31Z

@marcochavezf Can we merge this?

marcochavezf · 2024-05-14T16:54:48Z

Yesterday, the PR reviewer check was stuck, but now it's passing. Merging

OSBotify · 2024-05-14T17:02:23Z

✋ 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-05-15T10:12:21Z

🚀 Deployed to staging by https://github.com/marcochavezf in version: 1.4.74-0 🚀

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

OSBotify · 2024-05-22T23:57:21Z

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.4.74-6 🚀

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

allgandalf · 2024-07-10T13:11:57Z

src/components/ImageView/index.tsx

@@ -210,7 +213,8 @@ function ImageView({isAuthTokenRequired = false, url, fileName, onError}: ImageV
                    onLoad={imageLoad}
                    onError={onError}
                />
-                {(isLoading || zoomScale === 0) && <FullscreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />}
+                {((isLoading && !isOffline) || (!isLoading && zoomScale === 0)) && <FullscreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />}
+                {isLoading && <AttachmentOfflineIndicator />}


We should have also checked if the file is a localfile in case of offline upload, this caused #42657

implemented offline indicator for video

91daa59

FitseTLT added 4 commits April 3, 2024 18:33

Merge branch 'main' into fix-attachement-offline-indicator

58cea0a

Merge branch 'main' into fix-attachement-offline-indicator

108740d

Created attachment offline indicator component

7b007e7

Merge branch 'main' into fix-attachement-offline-indicator

2a25277

FitseTLT added 7 commits April 5, 2024 20:24

Merge branch 'main' into fix-attachement-offline-indicator

211c18b

did code cleanup

80a7113

fix lint and type

e6153f2

revert unnecessary change

cbf52fe

removed unnecessary view

b3f14d6

implemented for image view

17cb608

implemented for thumbnail preview

29c89d7

minor fix

da53445

lschurr mentioned this pull request Apr 5, 2024

[$500] Chat - Attachment page keeps on loading infinitely. #35502

Closed

6 tasks

FitseTLT added 3 commits April 8, 2024 19:40

Merge branch 'main' into fix-attachement-offline-indicator

a2c8cbc

implemented offline indicator for native

2a69717

minor fix

bf1786e

Merge branch 'main' into fix-attachement-offline-indicator

0c01e8f

FitseTLT added 4 commits May 2, 2024 13:55

Merge branch 'main' into fix-attachement-offline-indicator

243dd47

added delay for offline indicator

23ed2e3

revert unnecessary change

4de0a96

Merge branch 'main' into fix-attachement-offline-indicator

99218b3

trjExpensify mentioned this pull request May 3, 2024

[$250] Android - Chat - In offline mode, protected PDF file shows "Failed to load PDF file" messages. #16694

Closed

6 tasks

FitseTLT added 2 commits May 7, 2024 15:14

Merge branch 'main' into fix-attachement-offline-indicator

3e31bf8

add offline indicator for lightbox

1dd5ee4

mollfpr approved these changes May 11, 2024

View reviewed changes

melvin-bot bot requested a review from marcochavezf May 11, 2024 14:33

marcochavezf approved these changes May 13, 2024

View reviewed changes

marcochavezf merged commit 0b9cc89 into Expensify:main May 14, 2024
16 checks passed

mallenexpensify mentioned this pull request May 15, 2024

[PAY] Image infinite loading when offline #13826

Closed

2 tasks

github-actions bot mentioned this pull request May 15, 2024

Deploy Checklist: New Expensify 2024-05-15 #42185

Closed

lanitochka17 mentioned this pull request May 16, 2024

[Ready for payment][$250] Chat - Offline indicator and message isn't visible for video attachment and preview #42316

Closed

6 tasks

allgandalf reviewed Jul 10, 2024

View reviewed changes

allgandalf mentioned this pull request Jul 10, 2024

[HOLD for payment 2024-06-28] [$250] mWeb - Chat - Uploading an image while offline briefly shows an offline message in the preview #42657

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix - Attachment page keeps on loading infinitely. #39290

Fix - Attachment page keeps on loading infinitely. #39290

FitseTLT commented Mar 29, 2024 •

edited

Loading

FitseTLT commented Mar 29, 2024

shawnborton commented Apr 1, 2024

dannymcclain commented Apr 1, 2024

shawnborton commented Apr 1, 2024

FitseTLT commented Apr 4, 2024

shawnborton commented Apr 5, 2024

shawnborton commented Apr 5, 2024

FitseTLT commented Apr 5, 2024

dannymcclain commented Apr 5, 2024

FitseTLT commented Apr 5, 2024

FitseTLT commented Apr 5, 2024

shawnborton commented Apr 8, 2024

dannymcclain commented Apr 8, 2024

mollfpr commented Apr 30, 2024

shawnborton commented Apr 30, 2024

trjExpensify commented May 3, 2024

FitseTLT commented May 7, 2024

mollfpr commented May 7, 2024

mollfpr commented May 11, 2024

trjExpensify commented May 13, 2024

mollfpr commented May 13, 2024

marcochavezf left a comment

mollfpr commented May 14, 2024

marcochavezf commented May 14, 2024

OSBotify commented May 14, 2024

OSBotify commented May 15, 2024

OSBotify commented May 22, 2024

allgandalf Jul 10, 2024

Fix - Attachment page keeps on loading infinitely. #39290

Fix - Attachment page keeps on loading infinitely. #39290

Conversation

FitseTLT commented Mar 29, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

FitseTLT commented Mar 29, 2024

shawnborton commented Apr 1, 2024

dannymcclain commented Apr 1, 2024

shawnborton commented Apr 1, 2024

FitseTLT commented Apr 4, 2024

shawnborton commented Apr 5, 2024

shawnborton commented Apr 5, 2024

FitseTLT commented Apr 5, 2024

dannymcclain commented Apr 5, 2024

FitseTLT commented Apr 5, 2024

FitseTLT commented Apr 5, 2024

shawnborton commented Apr 8, 2024

dannymcclain commented Apr 8, 2024

mollfpr commented Apr 30, 2024

shawnborton commented Apr 30, 2024

trjExpensify commented May 3, 2024

FitseTLT commented May 7, 2024

mollfpr commented May 7, 2024

mollfpr commented May 11, 2024

Reviewer Checklist

Screenshots/Videos

trjExpensify commented May 13, 2024

mollfpr commented May 13, 2024

marcochavezf left a comment

Choose a reason for hiding this comment

mollfpr commented May 14, 2024

marcochavezf commented May 14, 2024

OSBotify commented May 14, 2024

OSBotify commented May 15, 2024

OSBotify commented May 22, 2024

allgandalf Jul 10, 2024

Choose a reason for hiding this comment

FitseTLT commented Mar 29, 2024 •

edited

Loading