[Image][Performance] Fast Image with Auth Token Header Caching (revert follow-up)

[thomas-coldwell]

Details

This PR is the follow up the the original fast image PR that was reverted here. The key improvements in this PR are:

• Better naming convention for the Image component
• Getting the auth token and headers are now fully encapsulated by the Image component itself (+ reactive with the withOnyx HoC rather than Onyx.connect())
• Resolves the revert issue [$1000] [HOLD for payment 2022-12-05] Android - Attachments - PDF file is disappeared in the chat after sent it #13002 where react-native-fast-image would not load if the style width / height were set to zero.

Fixed Issues

Original image caching issue - #10792
Android PDF thumbnail not displayed (triggered initial revert) - #13002

Tests

1. Checkout this branch & npm i
2. Run the app on both iOS and Android
3. Open a chat with some image attachments & PDF thumbnails
4. They will load in as normal the first time, but reload the app and then you will see the cached images loaded instantly in the same chat

• Verify that no errors appear in the JS console

Offline tests

1. Open a chat and wait for all chat attachment thumbnail images to finish loading
2. Go offline and reload the app and open the same chat - all chat attachment thumbnails should be displayed
3. Go back online and open the attachment view of an image attachment in the chat and let it load
4. Go offline, reload the app and open the same chat and then open the same attachment - this should very briefly show a loading spinner before then displaying the cached image

NOTE: The thumbnail images should be cached for any attachment type, however, when opening the attachment view for an attachment only image attachments will be cached - for example, PDFs will have their preview thumbnail image cached, but the PDF file itself is not cached and thus it is expected behaviour that you will see "Failed to load PDF" when performing steps 3&4 for PDF attachment.

QA Steps

• Open a report chat and add a new image attachment and send so the image appears in the chat (this should display a loading indicator the first time it is requested)
• Reload the app and you should see the cached image displayed (with no loading indicator)
• Click on an image in the chat to open the attachment modal view, close it and reopen it to see the cached image displayed without a loading state
• Verify that any attachments still download as normal without any errors (including image attachments and PDFs)
• Upload a PDF and the thumbnail should render correctly in chat

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 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 correct English and 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 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.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

Screen.Recording.2023-01-03.at.14.58.27.mov

iOS

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-01-03.at.14.28.23.mp4

Android

Screen.Recording.2023-01-03.at.14.45.23.mov

[thomas-coldwell]

I've just done a quick test with the fix that was mentioned here in slack. It does indeed load the images and the onLoad event is now fired, however as suggested by other comments in the OG GitHub issue, this applies no compression to the actual image on Android and thus large images cause a crash - I tested with a 10,000 x 5,000 px image (only 1.1mb on disk) and was able to reproduce this.

Test image: https://user-images.githubusercontent.com/31568400/205428950-cd7119fb-cc70-4e76-9518-8027ead43f6f.jpg

I will do some more digging and see if a better fix can be found as we definitely cannot apply this directly if it is going to cause crashes. Also maybe raises a question if we want to ever impose a max resolution for images and videos uploaded as chat attachments cc @Beamanator. Will keep you posted 🙌

[thomas-coldwell]

Looks like Glide (the image loading module used in Fast Image on Android) requires a valid, non-zero view size before it can begin image loading as this comment from one of the collaborators suggests:

• onResourceReady not called, issue with visibility bumptech/glide#618 (comment)
• https://github.com/bumptech/glide/blob/v4.7.1/library/src/main/java/com/bumptech/glide/request/target/ViewTarget.java#L516
• https://bumptech.github.io/glide/doc/targets.html#view-targets

With that in mind do we still want to class the onLoad not firing for zero width/height images on Android as a bug or just how it should work on Android? I know you strive to have the same functionality of components cross-platform

I think the options are from here:

• Rework Glide so it can somehow provide the width/height of the image source and fire the onLoad event regardless of view size
• Rework Fast Image to have either a onImageSizeAvailable prop at the component level or a static FastImage.getSizeWithHeaders() function that can somehow access the natural width and height of the image with Glide, likely through Targets.SIZE_ORIGINAL
• Ignore the fact it doesn't load if the view size is zero and then always set the expectation that the view should have a non-zero width/height (which in this specific case will be fixed by the backend change documented here by @Beamanator [Image][Performance] Fast Image with Auth Token Header Caching #12648 (comment))

Let me know your thoughts @Beamanator @kidroca 🙌

[Beamanator]

Hey @thomas-coldwell thanks so much for the in depth investigation!! I believe I'd prefer we go with options 1 or 2 ^ since they're fixing bugs instead of implementing workarounds... It looks like https://github.com/bumptech/glide/ is a pretty active repo so maybe we could get the fix in there, though that issue you linked was opened (and closed) a long time ago 😅 Maybe you can see if there's other related issues that are more recent / still open? And if not, maybe we can start a discussion there to see if it's something they'd consider "fixing"?

[thomas-coldwell]

Cheers @Beamanator, I agree it would be preferable to fix this at the source.

I'm OOO till tomorrow, but will see what can be done with Glide or if there is any more recent issues around this 🙌

[thomas-coldwell]

There doesn't seem to be any more recent issues / PRs on Glide for loading without a valid view (the source code does a check to make sure the the view's dimensions are all non-zero before loading starts). There appears to be a way to set a custom view target with glide and get it to report the natural dimensions for the image irrespective of the view it is being laid out into.

This will likely then become a PR into the @pieter-pot/react-native-fast-image fork we have - do we know who owns this fork and if not should we create a specific Expensify owned fork of it @Beamanator ?

[thomas-coldwell]

Actually, this seems to be the recommended way of getting the natural dimensions of the image with Glide and ensure that it is only making a single request or decoding this from the cached image bumptech/glide#781 (comment).

One question I have is do we want to always report the natural image dimensions in the onLoad event (which would be the case with this fix)? Glide currently resizes images so they have a smaller memory footprint and then reports the resized dimensions of the image like so (this is for a tiny 50x50 dp view):

 LOG  RN Image (onLoad) = {
  "height": 1800,
  "width": 1800,
}
 LOG  Fast Image (onLoad) = {
  "width": 132,
  "height": 132
}

I'm assuming we do want the natural dimensions so the API is identical with RN core image component? I'm conscious that the image displayed by glide will not actually have these dimensions but I don't think this will have any knock on effects as the width and height are just used to compute image aspect ratio and not directly for other view layouts.

[Beamanator]

@thomas-coldwell since there's no current issues / PRs on Glide about this issue, do you think it makes sense to open an issue there to discuss this situation? It's possible those maintainers would recommend we take this issue to the react-native-fast-image lib, but I'm inclined to want to start there since that's higher up the chain, what do you think?

Also I am not sure who we'd contact at @pieter-pot/react-native-fast-image to get a PR merged... I wonder if we would have to go through the main react-native-fast-image lib first and get that passed down. Though, as I'm comparing the two, there's very very very small differences between the fork we use & the main library - honestly it's hard to even see what the differences are, do you see any? I wonder if it's a better idea to see if we can use the main repo instead of the fork?

About the natural image dimensions vs resized dimensions, this is getting a bit far above my understanding, sorry 😅 So far I like your thinking but honestly I don't quite understand the benefits / drawbacks very well - if you think you're onto something I thinkkkk we should move forward with that and see what library maintainers think and move forward that way?

Also FYI I'm going to start a discussion in Slack (soon) to see if people think we should move forward with a little workaround while this conversation / implementation is happening in other repos, then once the fix is implemented & is usable by the New Expensify app, we can remove the workaround. Coming soon :D

[thomas-coldwell]

Hey @Beamanator 👋 I don't think it makes sense to open an issue with Glide, glide works as it should loading an image into a target - usually a drawable. This is how Glide is used in Fast Image.

The fix that I've found, and implemented in a separate sandbox app, is to setup an additional request for glide which decodes the image to get the size and then emits this as the onLoad event back to JS.

I agree there seems to be literally no difference between the fork we are currently using and the main react-native-fast-image repo so I'd opt that we make a fork of react-native-fast-image for Expensify and then we can hopefully PR these changes upstream (though the maintainer of Fast Image is very inactive so the changes might have to stay on our fork, but will try to get it merged upstream nonetheless). Is this possible to get a fork on Expensify or should I just fork it myself? 😃

The natural dimensions are that of the source image we load from the backend e.g. you might have a 1000x1000px image. With Glide and a lot of other image loading libraries they will resize this down appropriately for the target e.g. if its being loaded into a tiny 50x50dp view (like a profile picture) then it will resize it down to maybe 100x100px to preserve memory. I think it makes more sense to do it the same as the RN core Image API and report the natural dimensions so will stick with that 👍

[Beamanator]

The fix that I've found, and implemented in a separate sandbox app, is to setup an additional request for glide which decodes the image to get the size and then emits this as the onLoad event back to JS.

That sounnnds great! Do you think we should try to test it with our App before moving forward with that solution? Or was your test solid enough? :D

About the forking question - we reeeeally try not to make our own Forks whenever possible, so what we'll most likely do is try to get the conversation & PR going on the main react-native-fast-image repo, and if it goes too long w/ no response we will consider making the Fork. In the meantime I may implement the backend workaround just to get this unstuck

[thomas-coldwell]

Perfect, cheers @Beamanator !! I'll create a fork and apply my changes there, open up a PR to the OG repo and then I'll test using my PR branch in the Expensify app as well to make sure there are no obvious issues with this approach 🙌

[Beamanator]

That sounds fantastic, thanks so much @thomas-coldwell ! 👍

[thomas-coldwell]

I've fixed any merge conflicts there were (as main contained the revert changes for the original PR) so this should be good to test with the backend workaround if you wanted to @Beamanator :)

I'm now creating a PR for react-native-fast-image with the changes I made to return the natural dimensions from the onLoad event. There was one similar PR already opened there, but they decode the whole bitmap to get the size and use the .override(Target.SIZE_ORIGINAL) method for Glide which does not perform any compression and causes crashes on Android (I've verified this is the case in the Expensify app too with a 10,000px image) - their PR seemed to go stale due to these issues being pointed out. My PR is following the recommended way to achieve this as mentioned here bumptech/glide#781 (comment)

I will link this here when it's up 🙂

[Beamanator]

Thanks @thomas-coldwell ! I think I'll wait till some resolution on this convo (feel free to add your opinion) before testing w/ my workaround PR :D

[thomas-coldwell]

Opened up the PR on the react-native-fast-image repo DylanVann/react-native-fast-image#953 🙌

[Beamanator]

Reached out to maintainer to see if we can get the upstream PR moving - if no response this week we will have to think about forking

[thomas-coldwell]

Established with @puneetlath that a patch of the upstream PR will work best for now and works perfectly fine with the native code changes. The upstream PR will remain open and hopefully the maintainer will have some time to eventually look at it - if it does get merged upstream this patch can then be removed and upgraded to the latest version

[mountiny]

Great @thomas-coldwell and @kidroca I think we are now all happy and this can be merged as we had 5 approvals before.

Is there anything else @kidroca which you would like to address?

[Beamanator]

Thanks for the quick response & fix @thomas-coldwell 👍

I think we should do one more round of testing before we merge 🙏

[mananjadhav]

Yeah I agree. I am going to test this in a while @Beamanator. Give me like 30-45 mins.

[Beamanator]

Amazing, thanks so much @mananjadhav 👍

[mountiny]

Amazing @mananjadhav

[mananjadhav]

@thomas-coldwell , @Beamanator

Quick update, I just tested Desktop and Web. The caching seems to be working fine. I won't be able to do offline tests as I can't disconnect my laptop from Wifi right now. While I am testing other platforms, I found one issue.

I missed to check this earlier. When I download the images and PDF, the file name of the downloaded file is correct. But when I download a word document, the file name of the downloaded doc, doesn't match the original file name. This works fine with staging and production. Attaching the video.

web-file-download-name.mov

[mananjadhav]

@thomas-coldwell @Beamanator So I did all the testing for all the platforms and the caching works fine.

I've got two concerns/issues:

1. File Download name isn't working as expected so we'll have to fix that (which I posted in the previous comment).

2. If I am offline, and I open a new image, what's the expected output for the Attachment Preview? Android has just a loader, and Web has a blank screen. PDF shows Failed to load PDF file.. And I think that's how it is in the current prod/main version too.

android-offline-loader.mov

[Beamanator]

Thanks for the tests and notes @mananjadhav !

In my opinion:

1. File Download - I would be completely fine opening a follow-up issue to investigate this, I don't see how this PR could have broken that, especially if it's only for word documents
2. As for chat attachment images in the Attachment Preview - I think we'd expect to be able to see the images, right @thomas-coldwell ?
   • Note: For PDFs, we don't expect to see the PDF (see notes starting here)

[mananjadhav]

Okay for 1.

for 2, what I meant is opening the image for the first time in the offline mode. I am inclined to solve this also as a follow up Pr because the behavior is same as prod, but just wanted to highlight it here.

[thomas-coldwell]

Thanks @mananjadhav and @Beamanator

I've just checked out main and pulled the latest changes and can confirm the file download name issue exists outside this PR. As for 2, I'd say if this is showing the same behaviour while offline and opening a new, uncached image (e.g. you uploaded an image, go offline, then click to open it in the attachment view which tries to load the higher resolution, uncached image URL) as it is in prod/staging then that should be fine and the UX here could be improved in a follow up PR 👍

[Beamanator]

@mananjadhav Aah because the image hasn't been viewed yet so it probably hasn't been cached either? I'd at least expect a consistent thing to happen across apps - but yeah as @thomas-coldwell said if this is consistent with production, let's discuss outside this PR - probably in Slack to get some more eyes 👍

[mananjadhav]

Thanks for confirming @thomas-coldwell.

Agreed to keep both of them outside the scope of this PR. All good here then.

[Beamanator]

FYI it looks like the failing test is not related to this PR, and it was noted in Slack here: https://expensify.slack.com/archives/C01GTK53T8Q/p1673916481148719

So I'm going to merge 👍

[melvin-bot]

@Beamanator looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

[Beamanator]

Mentioned above that 1 test wasn't passing, but it's not related to this PR and is being investigated in Slack and has this issue for investigating: #14352

[github-actions]

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	674.317 ms → 684.039 ms (+9.722 ms, +1.4%)
App start regularAppStart	0.014 ms → 0.017 ms (+0.002 ms, +15.4%) 🟡
App start nativeLaunch	20.226 ms → 19.759 ms (-0.467 ms, -2.3%)
App start runJsBundle	188.031 ms → 187.031 ms (-1.000 ms, -0.5%)
Open Search Page TTI	612.976 ms → 609.587 ms (-3.389 ms, -0.6%)

Show details

Name	Duration
App start TTI	Baseline Mean: 674.317 ms Stdev: 24.236 ms (3.6%) Runs: 631.9363259999081 638.933752000099 643.41374800005 643.7281239998993 645.7523640000727 646.5710829999298 649.4097829998937 650.3707590000704 654.9559269999154 660.2531059999019 660.6519740000367 663.3258469998837 666.5857979999855 668.8872159998864 669.5266090000514 675.2744980000425 675.514351000078 680.314737000037 681.7773480000906 683.3502549999394 685.1995689999312 686.2015649999958 691.3849879999179 696.0857009999454 696.3966049998999 699.2526159998961 701.3830780000426 710.9573329999112 711.6242160000838 715.4863700000569 719.3265579999425 Current Mean: 684.039 ms Stdev: 30.188 ms (4.4%) Runs: 636.8109689999837 639.9906530000735 648.9527479999233 651.1539660000708 651.745462999912 658.4214780000038 662.04752500006 663.3393379999325 664.1186200000811 666.0464230000507 666.4262349999044 668.0293040000834 669.2267130000982 671.9246179999318 674.3322350000963 674.9428620000836 676.1934789998922 677.0932889999822 677.649459999986 679.1448560000863 689.1256939999294 692.7641650000587 695.813402999891 706.9918219998945 713.634371999884 715.848060999997 717.0645969999023 717.9894049998838 729.8511079999153 732.6237820000388 747.8646430000663 752.0939160000999
App start regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (3.6%) Runs: 0.013427000027149916 0.013631999958306551 0.013712000101804733 0.013752999948337674 0.013752999948337674 0.013752999948337674 0.013794000027701259 0.013875999953597784 0.013956999871879816 0.014078999869525433 0.014079000102356076 0.014200999867171049 0.014241999946534634 0.01432300009764731 0.01436399994418025 0.014404000015929341 0.014405000023543835 0.014445000095292926 0.014484999934211373 0.014567000092938542 0.01460699993185699 0.014689000090584159 0.014690000098198652 0.0147299999371171 0.014932999853044748 0.014973999932408333 0.014973999932408333 0.014974000165238976 0.015746999997645617 Current Mean: 0.017 ms Stdev: 0.001 ms (6.2%) Runs: 0.014771000016480684 0.015055000083521008 0.015300000086426735 0.015420999843627214 0.015503000002354383 0.015583999920636415 0.015625 0.015666000079363585 0.015706000151112676 0.01599099999293685 0.01603199983946979 0.01611399999819696 0.01619499991647899 0.016439000144600868 0.016478999983519316 0.0165200000628829 0.01656099990941584 0.016561000142246485 0.016641999827697873 0.016642000060528517 0.016764000058174133 0.016885999822989106 0.017089999979361892 0.017131000058725476 0.017170999897643924 0.017416000133380294 0.01745599997229874 0.017699999967589974 0.0185139998793602 0.018962000031024218 0.018962000031024218
App start nativeLaunch	Baseline Mean: 20.226 ms Stdev: 1.963 ms (9.7%) Runs: 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 21 21 21 21 21 22 22 22 22 23 24 24 25 Current Mean: 19.759 ms Stdev: 1.330 ms (6.7%) Runs: 18 18 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 21 21 21 21 22 23
App start runJsBundle	Baseline Mean: 188.031 ms Stdev: 19.609 ms (10.4%) Runs: 157 163 165 165 167 168 168 171 175 176 178 180 181 181 182 183 185 185 185 186 189 192 199 199 206 209 212 219 219 221 221 230 Current Mean: 187.031 ms Stdev: 16.040 ms (8.6%) Runs: 160 165 167 169 170 170 172 175 175 177 178 179 180 183 183 184 184 187 189 190 192 192 195 197 199 200 204 204 205 214 216 230
Open Search Page TTI	Baseline Mean: 612.976 ms Stdev: 30.627 ms (5.0%) Runs: 559.4135340000503 570.7141110000666 574.5441890000366 575.3415530000348 581.6207280000672 584.8881029998884 588.085001999978 588.8836670001037 590.9793300000019 593.3075769999996 594.2154540000483 596.0911049998831 597.3087570001371 599.243286000099 599.8012699999381 599.9442549999803 608.9034430000465 610.9958900001366 611.3704429999925 620.422567000147 623.5995689998381 624.983398999786 631.5948489999864 635.9384769999888 647.1783040000591 649.4792080000043 649.6335459998809 654.0551350000314 654.1037999999244 654.7186689998489 665.6060799998231 678.2722579999827 Current Mean: 609.587 ms Stdev: 10.825 ms (1.8%) Runs: 590.2568360001314 590.6814780000132 591.2717689999845 594.3564860001206 597.1129970001057 597.9006349998526 606.8589280000888 607.8230390001554 608.0974119999446 608.1432699998841 608.9827479999512 609.4709069998935 610.9358319998719 612.1387940000277 612.3134359999094 612.6619879999198 612.6708580001723 612.789347999962 612.8476559999399 616.3550209999084 617.8881840000395 618.5128580001183 619.3342699999921 620.3748370001558 620.5863849997986 638.8885910001118

[OSBotify]

🚀 Deployed to staging by @Beamanator in version: 1.2.56-0 🚀

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

[OSBotify]

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.56-0 🚀

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