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

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Login with any account
2. Go to any chat room
3. Send image as an attachment
4. Send pdf as an attachment
5. Click pdf to show preview
6. Click left and right arrows multiple times to see image and pdf

Expected Result:

Previews should show correctly

Actual Result:

previews keep loading forever and sometimes image shows briefly and disappears (glitch)

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.86-1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

bug.mp4

az_recorder_20230316_120442.1.mp4

Expensify/Expensify Issue URL:
Issue reported by: @situchan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1678644450224539

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e756be997448cf18
• Upwork Job ID: 1637815865356738560
• Last Price Increase: 2023-05-08

[MelvinBot]

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[stephanieelliott]

It sounds like this is similar to #15922? However the endless spinner and image flickering seem to be unique to Android so will treat this as separate.

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01e756be997448cf18

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[tienifr]

Proposal

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

Image previews keep loading forever and sometimes image shows briefly and disappears when switching quickly between PDF and image.

What is the root cause of that problem?

It's the use of onLoadStart here. The onLoadStart fires after onLoad already fired, causing the isLoading state to stuck at true. This was already fixed by this PR, but it was reintroduced in this PR which adds the Image Carousel.

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

We need to remove this line

App/src/components/ImageView/index.native.js

Line 213 in 8304865

onLoadStart={this.imageLoadingStart}

, we don't need it since the isLoading already defaults to true in the component, and it's already reset to true here if the image url changes (the imageHeight and imageWidth are also reset properly there as well). So that line is redundant and causes this bug.

Please note this is the same approach that was used to fix the issue earlier in here.

What alternative solutions did you explore? (Optional)

NA

Result

Working well after the fix:

Screen.Recording.2023-03-20.at.22.15.32.mov

[hellohublot]

Proposal

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

Attachment preview keeps loading forever when cycle between image and pdf

What is the root cause of that problem?

App/src/components/ImageView/index.native.js

Lines 227 to 228 in b0b31d1

	onLoadStart={this.imageLoadingStart}
	onLoad={this.configureImageZoom}

onLoadStart of the above code will call setState({ isLoading: true }), onLoad -> configureImageZoom of the above code will call setState({ isLoading: true }), if onLoadStart is before onLoad is responded, which is the correct logic

But if onLoad is responded before onLoadStart, our process becomes onLoad -> setState({ isLoading: false }) -> onLoadStart -> setState({ isLoading: true }) , so isLoading will eventually become true

---

Expensify/react-native-fast-image@a9b2a50#diff-12891c84ba8a335922bc60d5ea52118fdf436b6d950179a3101989aae9fa2f90L133-R169

From the above code, we can see our react-native-fast-image fork, in order to obtain the cachePath, onLoadStart is changed from synchronous to asynchronous, so the onLoad will be responded before onLoadStart, this is the main reason for this issue

This issue expresses the same problem, but this is caused by our react-native-fast-image fork, not the react-native-fast-image upstream code

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

Here is why the author introduced cachePath: #13304 (comment)
and 5b9db07#diff-171c9c0c3fbef4818a05438d1976262ffd2ff7f3e8686adc757618735c241694R88

Here is why the author removed cachePath: #13304 (comment)
and 16d7738#diff-171c9c0c3fbef4818a05438d1976262ffd2ff7f3e8686adc757618735c241694L90

He explained that he didn’t completely delete cachePath because another issue #12893 only wanted to display loading when image is loaded from the network. When loading from memory and disk, loading don't need be displayed. However, cachePath can only get whether there have a disk cache, it cannot judge Is there have a memory cache, so this idea is wrong

We should remove cachePath, and we should change onLoadStart be called asynchronous back to synchronous same as the upstream code.

+    eventEmitter.receiveEvent(viewId,
+            FastImageViewManager.REACT_ON_LOAD_START_EVENT,
+            new WritableNativeMap());
-    requestManager
-           .asFile()
-           .onlyRetrieveFromCache(true)
-    	 		...
-            .submit();

_after.mp4

What alternative solutions did you explore? (Optional)

But to be honest, we encountered too many problems on react-native-fast-image, maybe we should use the Image that from react-native to replace it, react-native on android will automatically use Fresco as a cache, but iOS we need to add the following code in AppDelegate to let the iOS system do the automatically cache

[NSURLCache setSharedURLCache:[[NSURLCache alloc] initWithMemoryCapacity:100 * 1024 * 1024
                                                               diskCapacity:500 * 1024 * 1024
                                                                   diskPath:nil]];

[mollfpr]

@tienifr removing onLoadStart={this.imageLoadingStart} makes the loading spinner not show and makes the image zoomed after the image loaded.

Screen_Recording_20230322_233847_New.Expensify.mp4

[mollfpr]

@hellohublot Could you show the working result from your proposal?

[hellohublot]

Proposal

Updated

@mollfpr Hello. I have modified my solution and added the video link

[mollfpr]

@hellohublot Thanks for the update! I still need to get what solution you propose. Could you explain more about that?

[hellohublot]

Proposal

Updated

@mollfpr Hello, I revised my proposal again, if it still confuses you, Feel free to ask again, Thank you

[mollfpr]

@hellohublot Thanks for the update!

I'm still unsure about removing the requestManager while the code tries to get the image from the cache. Isn't this mean that we will not show the cached image?

[hellohublot]

@mollfpr

https://github.com/DylanVann/react-native-fast-image/blob/9ab80fcd570b7f56da66ab20e52c9a35934067c9/android/src/main/java/com/dylanvann/fastimage/FastImageViewWithUrl.java#L133-L150

No, what actually sets the picture is the above piece of code, it will get it from the cache first, and if it fails, it will get it from the network

App/patches/react-native-fast-image+8.6.3.patch

Lines 176 to 201 in 10d3d52

	+ requestManager
	+ .asFile()
	+ .load(glideUrl)
	+ .onlyRetrieveFromCache(true)
	+ .listener(new RequestListener<File>() {
	+ @Override
	+ public boolean onLoadFailed(@Nullable GlideException e, Object model, Target<File> target, boolean isFirstResource) {
	+ WritableNativeMap result = new WritableNativeMap();
	+ result.putNull("cachePath");
	+ eventEmitter.receiveEvent(viewId,
	+ FastImageViewManager.REACT_ON_LOAD_START_EVENT,
	+ result);
	+ return false;
	+ }
	+
	+ @Override
	+ public boolean onResourceReady(File resource, Object model, Target<File> target, DataSource dataSource, boolean isFirstResource) {
	+ WritableNativeMap result = new WritableNativeMap();
	+ result.putString("cachePath", resource.getAbsolutePath());
	+ eventEmitter.receiveEvent(viewId,
	+ FastImageViewManager.REACT_ON_LOAD_START_EVENT,
	+ result);
	+ return false;
	+ }
	+ })
	+ .submit();

Our react-native-fast-image.patch.requestManager().onlyRetrieveFromCache().listener(new Listener( onLoadStart() )) is just simply obtains the cachePath, and does not call the builder.into(ImageView) method to set the image. And I didn't find any code calling cachePath in our all repository, so this is a piece of redundant code, we'd better to revert it

[melvin-bot]

@danieldoglas, @hellohublot, @mollfpr, @stephanieelliott Whoops! This issue is 2 days overdue. Let's get this updated quick!

[mollfpr]

Not overdue

[danieldoglas]

Changing this to weekly while we wait for the other PR.

[stephanieelliott]

Still waiting on merge of #18997

[danieldoglas]

@stephanieelliott I'll be OOO for 2 weeks. I'll unassign myself from this issue in case there's progress, can you please add Engineering label again?

[stephanieelliott]

Still waiting on merge of #18997. Will wait to pull un engineer once there is progress

[stephanieelliott]

This is still on hold for the other PRs.

[stephanieelliott]

Still on hold for of #18997

[stephanieelliott]

Still on hold for #18997

[stephanieelliott]

Still held on the other issue.

[stephanieelliott]

Still blocked on #18997

[stephanieelliott]

Ok so it looks like the blocking PR was held on Expensify/react-native-fast-image#6, which was closed in favor of an upstream PR which. has still not been merged.

However, I just re-tested this and it seems to be resolved now -- perhaps this was fixed by the move to expo-image or #30905 which was recently merged? If so, I think we can pay this out based on the PR that was merged: Expensify/react-native-fast-image#5

---

I was only able to test via simulator, so would love for someone else to test this to make sure it's resolved. Tagging in @kavimuru to see if we can re-test the action steps on this issue to see if it still persists>

[stephanieelliott]

Tried a few other simulator devices and still unable to repro. Closing this as resolved but feel free to reopen if you can reproduce.

[situchan]

@stephanieelliott reporting bonus is pending #16046 (comment)

[stephanieelliott]

OH! Yes I still need to issue payment on this one don't I. Ok, considering that this issue resulted in Expensify/react-native-fast-image#5 being created, reviewed and merged, and the PR described in the proposal (#18997) was created and reviewed before being put on hold, I think we should pay out the full amount on this one.

That would make the payment summary for this issue:

• Reporter: @situchan $250 via Upwork (PAID)
• Contributor: @hellohublot $1,000 via Upwork (offer extended, please accept)
• Contributor+: @mollfpr $1,000 NewDot request

Upwork job is here: https://www.upwork.com/jobs/~01825fa7d9e9517278

[stephanieelliott]

All paid, thank you!

[JmillsExpensify]

$1,000 payment approved for @mollfpr based on summary above.