[$2000] Previously opened chat profile pic appears for second when switch between chats

[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.Switch switch between chats and check the profile pic (both chats should have profile picture added)

Expected Result:

Should not show previously opened chat profile pic

Actual Result:

Previously opened chat profile pic flashes for a second

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.84-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:

Screen.Recording.2023-03-10.at.4.42.06.PM.mov

Recording.1706.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b588d14cef8db1ab
• Upwork Job ID: 1636438676948840448
• Last Price Increase: 2023-03-23

[MelvinBot]

Triggered auto assignment to @bfitzexpensify (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

[MelvinBot]

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

[MelvinBot]

Current assignee @bfitzexpensify is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

📣 @shubhom-juneja-outlier! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[shivenj]

Proposal

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

Avatar flicker when switch between chats

What is the root cause of that problem?

Previous Avatar instance is still mounted and we try to mount a new one.

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

Adding a key prop to the View would force unmount the previous one and mount a new one.

Screen.Recording.2023-03-17.at.5.36.45.AM.mov

What alternative solutions did you explore? (Optional)

None
Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[MelvinBot]

📣 @shivenj! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[shivenj]

Contributor details
Your Expensify account email: shiven.juneja+expensify@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01e01dc3316f98f2d7

[MelvinBot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[bernhardoj]

Not able to reproduce

[shivenj]

Not able to reproduce

Which browser and OS you are checking?

[tienifr]

Proposal

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

Previously opened chat profile pic flashes for a second in the chat header when changing chats with avatar images.

What is the root cause of that problem?

Let's look at how we're rendering the Image element:

1. We initially set the imageSource to undefined here

   App/src/components/Image/index.js

   Line 17 in 772a947

   imageSource: undefined,

2. The image renders with that undefined source
3. Then when componentDidMount or componentDidUpdate runs, we configureImageSource, which set the imageSource to a correct value here

   App/src/components/Image/index.js

   Line 50 in 772a947

   this.setState({imageSource});

4. Whenever the source props change (when we switch chats with different avatar for example), the RNImage element still used the old imageSource before the change, and only receives the correct imageSource 200ms later due to the debounce

The configureImageSource is run in debounce, which means the source is incorrect for 200ms and only becomes correct after that, causing the flicker.

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

We should move the logic to get the imageSource to an util function and run it directly in constructor when we set the state, the logic is completely synchronous and quite straight forward so there's no reason we should debounce it.

1. We add

getImageSource() {
        const source = this.props.source;
        let imageSource = source;
        if (this.props.isAuthTokenRequired) {
            // There is currently a `react-native-web` bug preventing the authToken being passed
            // in the headers of the image request so the authToken is added as a query param.
            // On native the authToken IS passed in the image request headers
            const authToken = lodashGet(this.props, 'session.encryptedAuthToken', null);
            imageSource = {uri: `${source.uri}?encryptedAuthToken=${encodeURIComponent(authToken)}`};
        }
        return imageSource;
    }

to the Image component here https://github.com/Expensify/App/blob/main/src/components/Image/index.js.

2. And remove that logic in configureImageSource, so the method now is only:

configureImageSource() {
        this.props.onLoadStart();
        const imageSource = this.state.imageSource;

        // If an onLoad callback was specified then manually call it and pass
        // the natural image dimensions to match the native API
        if (this.props.onLoad == null) {
            return;
        }

        RNImage.getSize(imageSource.uri, (width, height) => {
            this.props.onLoad({nativeEvent: {width, height}});
        });
    }

3. And in

   App/src/components/Image/index.js

   Line 17 in 772a947

   imageSource: undefined,

   
   set imageSource: this.getImageSource();

4. In componentDidUpdate, add

this.setState({
    imageSource: this.getImageSource(),
});

before this line

App/src/components/Image/index.js

Line 30 in 772a947

this.debouncedConfigureImageSource.cancel();

to update the imageSource if the source props changes.

Please note the rest of the asynchronous logic like onLoadStart, RNImage.getSize still remains in configureImageSource and be debounced properly. Only the synchronous logic to get the imageSource is extracted out.

Another note is that for native we're already using the above method (set imageSource immediately rather than debouncing) in here. The problem is only for web.

What alternative solutions did you explore? (Optional)

We can also get rid of the state and use getImageSource whenever we need access to the imageSource

Result

Working well after the fix

Screen.Recording.2023-03-17.at.21.27.03.mov

[moezh]

Not able to reproduce on DEV (MacOS / Chrome), only reproducible on STG (MacOS / Chrome)

[bfitzexpensify]

Not overdue

[MelvinBot]

📣 @tienifr You have been assigned to this job by @deetergp!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[tienifr]

@deetergp @thesahindia while working on the PR, I noticed that the flickering has been fixed by another PR here with a quite similar solutions. This was merged recently and after @thesahindia approved the proposal.

Tested again on staging and works well, so I think we don't have to do any code change here.

[deetergp]

I can confirm that it is no longer doing it on staging, thanks for catching it and pointing it out @tienifr.

@bfitzexpensify I think we can close this one out as no longer needed.

[MelvinBot]

@deetergp @bfitzexpensify @thesahindia @tienifr this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[tienifr]

@deetergp Hi, just checking if I'm eligible for compensation here since I was already assigned and started on the PR.

The proposal was valid at the time, also the merged solution that affects this is quite similar as well.

There was another case here where we closed the issue after assignment and PR process started, and compensation was still applied.

[deetergp]

Great question, @tienifr, let me ask and get back to you.

[deetergp]

Hey @tienifr, you did offer a proposal and did some work towards fixing it. The general consensus is half ($1000) of the issue price ($2000) seems fair. Does that seem fair?

[tienifr]

@deetergp yes that sounds good to me. Thanks!

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[deetergp]

@bfitzexpensify let's pay @tienifr $1k for their effort so far and then close this one out.

[thesahindia]

Hey @deetergp, I think I am also eligible for the compensation according to the C+ process doc. It says that the C+ will also get compensated (the decided amount) if they approved a proposal. Here's the related thread.

[bfitzexpensify]

Thanks for that link @thesahindia, I think that seems fair. Can you please apply to the job on Upwork?

Same for you @tienifr, please apply on Upwork and I can get this paid out.

[tienifr]

@bfitzexpensify applied, thanks!

[thesahindia]

I have applied as well!

Note: The job price is $2000 but only $1000 needs to be paid

[MelvinBot]

@deetergp @bfitzexpensify @thesahindia @tienifr this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[bfitzexpensify]

Offers sent!

[thesahindia]

Accepted, thanks!

[bfitzexpensify]

OK, payments finalised and contracts ended. Closing this one out.