Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS - Chat - User icon flickers when sending a message #4210

Closed
1 of 5 tasks
kavimuru opened this issue Jul 24, 2021 · 58 comments
Closed
1 of 5 tasks

iOS - Chat - User icon flickers when sending a message #4210

kavimuru opened this issue Jul 24, 2021 · 58 comments
Assignees
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Jul 24, 2021

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
  • iOS
  • Android
  • Desktop App
  • Mobile Web

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

Bug5164754_Flashes.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

@kavimuru kavimuru added the DeployBlockerCash This issue or pull request should block deployment label Jul 24, 2021
@OSBotify
Copy link
Contributor

👋 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
Copy link

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

@Dal-Papa Dal-Papa added Daily KSv2 External Added to denote the issue can be worked on by a contributor and removed Hourly KSv2 labels Jul 26, 2021
@MelvinBot
Copy link

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

@mallenexpensify
Copy link
Contributor

@kavimuru is this still a deploy blocker?

@isagoico
Copy link

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.

@isagoico isagoico removed the DeployBlockerCash This issue or pull request should block deployment label Jul 26, 2021
@mallenexpensify
Copy link
Contributor

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 26, 2021
@MelvinBot
Copy link

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

@aliabbasmalik8
Copy link
Contributor

aliabbasmalik8 commented Jul 26, 2021

PROPOSAL

It can be fixed by adding keyExtractor in FlatList like this way keyExtractor={item => item.index}

<FlatList
// eslint-disable-next-line react/jsx-props-no-spreading
{...this.props}
ref={this.props.innerRef}
inverted
renderItem={this.renderItem}
// Native platforms do not need to measure items and work fine without this.
// Web requires that items be measured or else crazy things happen when scrolling.
getItemLayout={this.props.shouldMeasureItems ? this.getItemLayout : undefined}
bounces={false}
// We keep this property very low so that chat switching remains fast
maxToRenderPerBatch={1}
windowSize={15}
removeClippedSubviews={this.props.shouldRemoveClippedSubviews}
/>

OR can update keyExtractor here

keyExtractor={item => `${item.action.sequenceNumber}${item.action.clientID}`}

Thanks

@mananjadhav
Copy link
Collaborator

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
Copy link
Contributor

@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
Copy link
Contributor

aliabbasmalik8 commented Jul 26, 2021

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

keyExtractor={item => `${item.action.sequenceNumber}${item.action.clientID}`}

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
Copy link
Collaborator

mananjadhav commented Jul 27, 2021

@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.

User.Image.flicker.issue.mov

@arpitdeveloper
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor

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

@MelvinBot MelvinBot removed the Overdue label Aug 9, 2021
@thienlnam thienlnam added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 9, 2021
@mallenexpensify
Copy link
Contributor

@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
Copy link
Contributor

@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)

@thienlnam thienlnam added the Reviewing Has a PR in review label Aug 11, 2021
@MelvinBot MelvinBot removed the Overdue label Aug 11, 2021
@MelvinBot
Copy link

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

@thienlnam
Copy link
Contributor

Reviews still in process - delay with internal focus right now

@MelvinBot
Copy link

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

@mallenexpensify
Copy link
Contributor

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

@mallenexpensify
Copy link
Contributor

Reassigning, @thienlnam does someone need to be compensated?

@thienlnam
Copy link
Contributor

@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

@MelvinBot
Copy link

@mallenexpensify, @thienlnam, @AlfredoAlc Huh... This is 4 days overdue. Who can take care of this?

@mallenexpensify
Copy link
Contributor

I created a new job in Upwork, @AlfredoAlc , you you please apply there and confirm here when you have? If your name shows differently than Alfredo A, please share it. Once done I'll pay $250
https://www.upwork.com/jobs/~0139053ca72f539195

@AlfredoAlc
Copy link
Contributor

@mallenexpensify I already applied on Upwork, my name on upwork shows Alfredo Alcantara

@mallenexpensify
Copy link
Contributor

Hird @AlfredoAlc in Upwork!

@mallenexpensify mallenexpensify added Weekly KSv2 and removed Daily KSv2 labels Sep 8, 2021
@thienlnam
Copy link
Contributor

@kavimuru could you please give this a re-test? You'll have to try with newly uploaded profile images instead of existing ones

@kavimuru
Copy link
Author

kavimuru commented Sep 8, 2021

@thienlnam I uploaded a new profile image, and not able to reproduce this issue in 1.0.94-0

Not.reproducible.MP4

@thienlnam
Copy link
Contributor

Awesome! @mallenexpensify task is considered completed, feel free to close the issue after paying it out.

@mallenexpensify
Copy link
Contributor

Paid @AlfredoAlc in Upwork! Thanks for the help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests