[HOLD for payment 2024-01-17] [$500] Avatar - Default Avatar photo is not fully rendered

[kbecciv]

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

---

Version Number: 1.4.17-1
Reproducible in staging?: y
Reproducible in production?: n
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Navigate to staging.new.expensify.com
2. Create group chat ( one of the members should have default avatar image)
3. Tap on Header
4. Tap on member with default avatar

Expected Result:

Avatar should be fully rendered

Actual Result:

Default Avatar photo is not fully rendered.
It shows low quality image

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6326359_1703617087227.az_recorder_20231226_144911.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01cf187b06b1d900a9
• Upwork Job ID: 1739769092481089536
• Last Price Increase: 2023-12-26
• Automatic offers:
• dukenv0307 | Contributor | 28072165

[github-actions]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[robertjchen]

Hm, can't seem to reproduce this, removing DeployBlockerCash label

[mountiny]

I do not think this is a blocker, this is not blocking user from any action.

@robertjchen This might require release build of the native app and its related to the expo-image PR so I am tagging @WojtekBoman @WoLewicki in case you could help handle this

[situchan]

This is reproducible only on native (android/iOS). Happens on dev build as well

[dukenv0307]

@mountiny I can raise a PR asap once assigned

Proposal

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

Default Avatar photo is not fully rendered.
It shows low quality image

What is the root cause of that problem?

The formula here is wrong. The intention was to show a clear image, let's say the native width is 80, the device pixel ratio is 3, in this case the width of the image dimension we should show is 80 * 3 = 240 because 3 pixels of the image will fit it 1 pixel in that device because the pixel ratio is 3.

However we currently divide by the pixel ratio, leading to the image (or svg) width and height become only 26, causing the blur.

This only happens on native because only native uses the Lightbox component and have that logic.

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

It should be * PixelRatio.get() in these lines (just need to change / to *)

Update: We need to add this logic to the fallback image as well, as mentioned in here

What alternative solutions did you explore? (Optional)

We can modify this to

if (typeof source === 'number' && !transactionID) {

so that the default avatar for workspace and user will be displayed in an Icon (as it was before the expo-image PR was merged), but the pixelation issue above with the PixelRatio.get will still need to be fixed for images.

[situchan]

@dukenv0307's solution makes sense.
However, the PR which introduced that formula was deployed to production already, while this was deploy blocker.
@dukenv0307 do you think expo image PR is offending one?

[mananjadhav]

@dukenv0307's solution seems like would work.

@situchan Sorry I didn't understand your comment.

🎀 👀 🎀 C+ reviewed.

[melvin-bot]

Current assignee @robertjchen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[situchan]

@mananjadhav this is deploy blocker which means doesn't happen on production.
So still curious to know what the offending PR is. #31308 is definitely not.
(You may need this when fill BZ checklist)

[shubham1206agra]

@mananjadhav @situchan is right. This is stemming from another place. Let me put together a proposal so that you can review

[shubham1206agra]

Proposal

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

Avatar - Default Avatar photo is not fully rendered

What is the root cause of that problem?

This is stemming from #30905, as we changed the ImageRenderer Engine from FastImage to Expo Image, and so the properties https://github.com/Expensify/App/pull/30905/files?short_path=b5e8f8f#r1436919622

Plus, we have another problem. If we use default workspace avatar, the background color will be missing in this case. This is happening due to we are using AttachmentViewImage component to render this instead of Icon component.

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

I am not sure that we need PixelRatio.get(). Cause I am able to use the image correctly without this.
Asked here: https://github.com/Expensify/App/pull/31308/files#r1436925363

There are different solutions depending on the ideal behaviour

1. If we want to double tap zoom / pinch behaviour on native devices (which is doubtful given this is not present on web/mWeb), then we need to correct the width and height by removing PixelRatio.get() as it is not required, and pass fill color for workspace icons to the AttachmentViewImage, and then to Image component also.

2. If we do not want to double tap zoom / pinch behaviour on native devices (which is likely behaviour), then we can pass a prop through AttachmentModal to know the image is avatar and render that through Icon component instead of AttachmentViewImage for the default avatars.

Test branch - https://github.com/shubham1206agra/App/tree/test-avatar-d

What alternative solutions did you explore? (Optional)

[dukenv0307]

I am not sure that we need PixelRatio.get()

@shubham1206agra the Pixel ratio multiplication is so that the image has correct pixelation and looks good, if we remove it, the image will look blurry, since the device pixel ratio is high. The issue here is just that the formula we're using is incorrect. I asked @chrispader to confirm here

If we want to double tap zoom / pinch behaviour on native devices (which is doubtful given this is not present on web/mWeb)

IMO we want this, because the attachment modal in the chat report also has zoom/pinch behavior.

Plus, we have another problem. If we use default workspace avatar, the background color will be missing in this case.

@shubham1206agra I think this is a completely different issue, with a different root cause, can you report it somewhere in Slack to have another GH open? Thank you!

cc @mananjadhav @robertjchen

[aswin-s]

Proposal

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

Default avatar looks pixelated on native devices

What is the root cause of that problem?

We are using expo-image to render SVG images now. The logic for computing image dimension in Lightbox component considered only remote images. For sharper rendering of images on high dpi devices, it divided the native image dimension by device pixel density.

App/src/components/Lightbox.js

Lines 185 to 189 in d6f359f

	onLoad={(e) => {
	const width = (e.nativeEvent?.width || 0) / PixelRatio.get();
	const height = (e.nativeEvent?.height || 0) / PixelRatio.get();
	setImageDimensions({...imageDimensions, lightboxSize: {width, height}});
	}}

However for SVG images this logic is not required as SVGs are vectors and could easily be resized to required width. Right now default avatars have a native width and height of 80px and dividing that by device pixel ratio further reduce the size of rendered image. That's the reason default avatars look blurred.

Also right now ImageView is throwing an error for local SVG images as its prop types expect only string types.

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

Instead of reducing the SVG size further by dividing its native size by device pixel ratio, it should be resized either to the DEFAULT_IMAGE_SIZE of 200px or the containerWidth so that SVG gets resized as per device width.

<Image
    source={{uri: source}}
    style={imageDimensions?.lightboxSize || {width: DEFAULT_IMAGE_SIZE, height: DEFAULT_IMAGE_SIZE}}
    isAuthTokenRequired={isAuthTokenRequired}
    onError={onError}
    onLoadEnd={() => setImageLoaded(true)}
    onLoad={(e) => {
        // For remote images use dpi adjusted size, for SVG resize it to fit container
        const width = typeof source === 'string' ? (e?.nativeEvent?.width || 0) / PixelRatio.get() : Math.max(e?.nativeEvent?.width || 0, DEFAULT_IMAGE_SIZE);
        const height = typeof source === 'string' ? (e?.nativeEvent?.height || 0) / PixelRatio.get() : Math.max(e?.nativeEvent?.height || 0, DEFAULT_IMAGE_SIZE);
        setImageDimensions({...imageDimensions, lightboxSize: {width, height}});
    }}
/>

Also local assets uri is going to be of type number. So to fix the prop type error, we need to modify the prop types of ImageView as below.

App/src/components/ImageView/propTypes.js

Line 12 in d6f359f

url: PropTypes.string.isRequired,

const imageViewPropTypes = {
     ..
    /** URL to full-sized image */
    url: PropTypes.oneOfType([PropTypes.string.isRequired, PropTypes.number.isRequired]),

What alternative solutions did you explore? (Optional)

None (edited)

[dukenv0307]

Plus, we have another problem. If we use default workspace avatar, the background color will be missing in this case.

@shubham1206agra looks like the separate issue you mentioned has its own GH created here, maybe you can move your solution there for centralized discussion.

[melvin-bot]

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

[robertjchen]

ok, let's move forward with @dukenv0307 's proposal 👍

[dukenv0307]

@mananjadhav The PR is ready for review.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.23-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix default avatar render with low quality #33745

If no regressions arise, payment will be issued on 2024-01-17. 🎊

For reference, here are some details about the assignees on this issue:

• @mananjadhav requires payment through NewDot Manual Requests
• @dukenv0307 requires payment automatic offer (Contributor)

[melvin-bot]

Issue is ready for payment but no BZ is assigned. @kevinksullivan you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[robertjchen]

Awaiting completion of BZ checklist

[mananjadhav]

The offending PR was already identified here and the comment was added during investigation. I don't think we need a regression test for this.

This is ready for payout @kevinksullivan Can you please post the payout summary?

[kevinksullivan]

Payment summary:

• C+: @mananjadhav $500
• Contributor: @dukenv0307 $500

[kevinksullivan]

Paid @dukenv0307 , closing this one out and @mananjadhav can request in newdot.

[JmillsExpensify]

$500 approved for @mananjadhav based on summary above.