iOS - Chat - User icon flickers when sending a message

[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. Launch the app and Log in
2. On a conversation, click on the + button in the compose box
3. Send a message

Expected Result:

When sending the message the user icon shouldn't flicker

Actual Result:

When sending the message the user icon flickers

Workaround:

Unknown

Platform:

Where is this issue occurring?

• [ ] Web
• [x] iOS
• [ ] Android
• [ ] Desktop App
• [ ] Mobile Web

Version Number: 1.0.80-0 Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/126857477-ee593723-c222-449c-bc5a-011c4f5bc7af.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

[OSBotify]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[MelvinBot]

Triggered auto assignment to @Dal-Papa (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[MelvinBot]

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

[mallenexpensify]

@kavimuru is this still a deploy blocker?

[isagoico]

Mmm I'm not able to reproduce consistently between several chats. I would say it's a 3/5 times. I also re checked this on production and found the same behaviour. Will remove the deploy locker label.

[mallenexpensify]

https://www.upwork.com/jobs/~01ccf19eee15ac9c94

[MelvinBot]

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

[aliabbasmalik8]

PROPOSAL

It can be fixed by adding keyExtractor in FlatList like this way keyExtractor={item => item.index} https://github.com/Expensify/App/blob/3d351694080b0c0d448e9d140d122de585eddd27/src/components/InvertedFlatList/BaseInvertedFlatList.js#L142-L158

OR can update keyExtractor here https://github.com/Expensify/App/blob/001f285d935b0edf77b26c7097205339cce89858/src/pages/home/report/ReportActionsView.js#L410

Thanks

[mananjadhav]

I can see that keyExtractor is already added to ReportActionsView. What is happening is the image is getting downloaded again when you're posting the message.

Proposal:

Can be resolved by replacing the default Image with RNFastImage

<View pointerEvents="none" style={this.props.containerStyles}>
                {this.props.isDefaultChatRoom
                    ? <RoomAvatar avatarStyle={imageStyle} isArchived={this.props.isArchivedRoom} />
                    : <Image source={{uri: this.props.source}} style={imageStyle} />}
            </View>

[thienlnam]

@aliabbasmalik8 Thanks for the proposal, could you explain why that is the problem?

@mananjadhav Are we seeing another network request for the image when the message is sent?

[aliabbasmalik8]

ISSUE: there is an issue on keyExtractor so the icons flickers on state update.

https://github.com/Expensify/App/blob/001f285d935b0edf77b26c7097205339cce89858/src/pages/home/report/ReportActionsView.js#L410

we can fix by updating keyExtractor like this way keyExtractor={item => item.index.toString()}

Thanks

After Updating keyExtractor https://user-images.githubusercontent.com/31027036/127396962-11bd526a-408f-46ea-9327-0ad3d5354e7f.mov

Issue Reproducing (Without updating keyExtractor) https://user-images.githubusercontent.com/31027036/127397015-9efa502e-365e-44c3-865f-76004154df00.mov

cache-control also helpful for image performance perspective for ios

[mananjadhav]

@thienlnam Yes. Attaching the video for the same. Also, we're already passing keyExtractor. It won't solve the image caching issue.

I know for sure that the RN Fast Image can resolve this for us.

https://user-images.githubusercontent.com/3069065/127173966-63ae7eb3-546f-41aa-b564-83c3bae88e1e.mov

[arpitdeveloper]

In src/components/InvertedFlatList/BaseInvertedFlatList.js we add keyExtractor={item => item.index} in flatlist or In src/pages/home/report/ReportActionsView.js replace keyExtractor with keyExtractor={item => item.index} in InvertedFlatList

[thienlnam]

@mananjadhav You have the Disable cache enabled so that isn't really a true test as to whether or not there is another image request. I tested it myself and the avatar images get cached on web where they have the correct image headers set.

@aliabbasmalik8 Going to give you the 🟢 for the proposal, we'll see if that fixes it

[thienlnam]

@arpitdeveloper Unfortunately another contributor got to that conclusion first, but thank you for your interest!

[parasharrajat]

@thienlnam we already have keyExtractor for ChatList. It's the first thing anybody will do with the most expensive list on the App so we have not missed this. Just bringing you more context here.

https://github.com/Expensify/App/blob/e043ac21f3706baed9b12fb9c7bd2b5ecbb55540/src/pages/home/report/ReportActionsView.js#L410

[thienlnam]

@parasharrajat Thanks for the catch - I should have checked that first. Going to unassign @aliabbasmalik8.

I still am not sure that image caching is the issue here since we are already caching those avatar thumbnails

[parasharrajat]

I still am not sure that image caching is the issue here since we are already caching those avatar thumbnails

I also think the same. I'll try to test this, if possible.

[AlfredoAlc]

PROPOSAL

I have been checking the code, and the problem it's in the keyExtractor in ReportActionsView. The key for each item is a combination of action.sequenceNumber and action.clientID.

https://github.com/Expensify/App/blob/b964c89bb2f010299cccd1cfa6263a00348eaf5f/src/pages/home/report/ReportActionsView.js#L407-L410

The issue stars when merged onto Onix, the sequenceNumber is a temporal optimisticReportActionID, quoting the comments of why this is used.. "We do this because it's not safe to assume that this will use the very next sequenceNumber".

https://github.com/Expensify/App/blob/de5b024fca24bc18fad99e583961035a87a8b950/src/libs/actions/Report.js#L1044-L1075

But once the ReportAction is created in the server, the sequenceNumber is modified with the actual number, and then updated in Onix.

https://github.com/Expensify/App/blob/de5b024fca24bc18fad99e583961035a87a8b950/src/libs/actions/Report.js#L1077-L1098

The change of that sequenceNumber causes to change the key of the item, and that causes to re render that last message sent, hence the flickering in the image.

A solution to avoid re rendering items could be to only use clientID for the key, and to prevent any issues in case the clientID its the same for two different items, it could be used the combination of clientID and index.

[rdjuric]

@AlfredoAlc Just curious, on the video in the issue the Avatar also flickers when we receive a message, and the sequence number of a received messaged is not optimistic (we receive the final sequenceNumber from the API).

So since the sequenceNumber doesn't change, the key for a received message shouldn't change either but the Avatar still flickers, do you know why?

[AlfredoAlc]

@rdjuric I did noticed that sometimes the Avatar flickers when the message is received, that could be the way the component Image from React Native handles cache, and like @mananjadhav said, RNFastImage should fix that issue.

The solution I proposed would fix the double (or in some cases triple) flickering of the avatar when sending a new message. Implementing the use of RNFastImage in combination of changing the key in keyExtractor , will fix both kind of flickering.

[AlfredoAlc]

Although, RNFastimage should be implemented only for moblie devices, and still using Image from React Native for web and desktop, since there is no problem with those, this could be a good fix.

[mananjadhav]

@AlfredoAlc I feel the keyExtractor should stay as is. As mentioned by Rafael, you'll see the avatar flickers in all cases.

@thienlnam @parasharrajat @rdjuric I gave a quick try adding FastImage to the chat list and you'll see the flicker is gone. Attached are the supporting screencasts.

Without FastImage (flicker/image load evident)

https://user-images.githubusercontent.com/3069065/127388923-2bdcdced-c2bf-448a-817a-925d89fd9afc.mp4

With FastImage (no flicker/delay)

https://user-images.githubusercontent.com/3069065/127389003-9fc5b8db-f2bb-4dda-ab36-57ba9c7d8b1b.mp4

[parasharrajat]

@mananjadhav truly saying that Without FastImage (flicker/image load evident) does not have the flicker we are looking to solve with this issue. I understand that with FastImage, image loading is fast but that is something different. Please check the main issue video for reference.

If image loading or keyExtractor was the culprit then we must have faced the same issue on Android as well not only IOS. Make sense?

[mananjadhav]

Okay got it. Just checked the video again. More than the delay, the erratic double flicker is what we're trying to resolve, right?

While I couldn't reproduce this, but I'll see if we can find a root cause.

[AlfredoAlc]

In the Image component of React Native, there is a property named cache, this property helps handling cache on iOS devices, by setting it to force-cache prevents the image to flicker.

NEW PROPOSAL Add this property to the Avatar component

[thienlnam]

Thank you all for keeping this conversation going!

@AlfredoAlc The docs for Cache Control doesn't really explain what default is but I don't think we want to force-cache as it ignores the expiration. I'm not sure if default is automatically enabled by default but we can start to see if that will work first

[AlfredoAlc]

@thienlnam Yes, the default property is automatically enabled by default. See ImageCacheEnum

Probably iOS handles cache storage by certain conditions, and this is the case where it doesn't store the image into cache.

One way to solve this is by implementing into native code SDWebImage library, which one of the main advantages is that handles cache storage and automatic expiration. Which falls back to implement RNFastImage, since is a react native wrapper around SDWebImage.

[thienlnam]

this is the case where it doesn't store the image into cache

Let's try to figure out why this is the case first, before trying to build a workaround for it

[GithubAshishHere]

• Can you try to give 100% height instead of flex:1 of the main container? Maybe it helps 
- Also can try to debug across frames and native components. 
- iOS has NSCache in which SDImages have the cache images. 
 
NEW PROPOSAL

[mananjadhav]

@parasharrajat @thienlnam I would still support my approach of using RNFastImage. You'll see others have also pointed out the same issue.

Some more issues were reported for Image Caching in iOS only for React-Native

https://github.com/facebook/react-native/issues/31584 https://github.com/facebook/react-native/issues/28557 https://github.com/facebook/react-native/issues/17137

Alternative with cache support is SDWebImage, which is what RNFastImage uses internally.

[MelvinBot]

@mallenexpensify, @thienlnam Whoops! This issue is 2 days overdue. Let's get this updated quick!

[thienlnam]

@GithubAshishHere

I'm having trouble following your proposal here -


- Also can try to debug across frames and native components. 
- iOS has NSCache in which SDImages have the cache images. 


These points don't seem to have any actionable changes that you would complete in your pull request

100% height instead of flex:1 of the main container

Can you explain how these changes might solve the issue?

[thienlnam]

@mananjadhav I'm not ruling out the use of RNFastImage in this project to solve the bug, but I would like to figure out why images doesn't get stored in the cache properly on IOS before building a solution around it. Is there a chance that we're using the incorrect image headers for the cache or using the component incorrectly? It would be good to understand why using RNFastImage solves this problem

[AlfredoAlc]

@thienlnam I saw that the default image has set a max-age value on the headers, while the image set by the user doesn't.. sometimes the default image doesn't flicker, maybe setting the value max-age to the header of a user selected image could tell iOS to cache the file.

[mananjadhav]

@thienlnam Afaik (couldn't find supporting documents), the default Image component of react-native uses UIImageView which does cache for smaller images but not images > 150-200kb. I am checking if I can for sure send you a way of verifying that this is a cache issue.

Whereas RNFastImage is a wrapper around SDWebImage which is primarily an image helper with cache support.

[thienlnam]

@AlfredoAlc That's an interesting idea thanks, I'll see if I can test that theory tomorrow

@mananjadhav Thanks, that would be great. Though the profile images (atleast on my contact profile pictures) range between 4-8 kb in size so I'm not sure if that is the case. Additionally, I'm aware that there is caching when opening an image preview on iOS and those images should be much larger than 150-200 kb

[MelvinBot]

@mallenexpensify, @thienlnam it looks like no one is assigned to work on this job. Please double the price or add a comment stating why the job isn't being doubled.

[thienlnam]

@AlfredoAlc Did some local testing and it seems like this actually does not happen for the default images at all (during my testing at least). So I think you're on to something about the image headers of the default images. It seems to maybe be the cache-control header that is the big difference?

default thumbnail is on the left, profile thumbnail on the right Response headers https://www.diffchecker.com/n9WAYkpL

I also noticed that you can replicate this behavior in Web by checking the Disable cache in the Network tab and the profile picture should flicker after sending a message

[AlfredoAlc]

@thienlnam Also, check the Expires header, I noticed that it has a past date.. from 2020, and while I was testing it I saw the same date. That would be more likely to be the problem of why it doesn't storage the image into cache.

[mallenexpensify]

@thienlnam Can you provide an update on this issue? Is it being worked on? should someone be hired? thx

[thienlnam]

Working on making the changes internally, @AlfredoAlc was the one to point out the issue so we should hire him for the job and pay it out

[mallenexpensify]

@thienlnam I think we should still wait til 7 days after merge before issuing payment (our current process) in case there are any issues with @AlfredoAlc's recommendation

[thienlnam]

@mallenexpensify That's fine 👍

There are actually two steps to this,

1. Prevent this header from getting added to newly uploaded images (PRs up for it right now)
2. Remove the header from current images in the S3 bucket

Since I've tested the solution and it works, we can wait 7 days until the current PRs get merged but it will take a bit more internal investigation to address the second part (but solution is the same)

[MelvinBot]

@mallenexpensify, @thienlnam Whoops! This issue is 2 days overdue. Let's get this updated quick!

[thienlnam]

Reviews still in process - delay with internal focus right now

[MelvinBot]

@mallenexpensify, @thienlnam Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[mallenexpensify]

Internal job, I removed the Upwork post, unassign me meow..

[mallenexpensify]

Reassigning, @thienlnam does someone need to be compensated?

[thienlnam]

@mallenexpensify Yup, I just merged the pending PRs so once those get deployed and can verify it doesn't happen with new profile images, we can pay @AlfredoAlc