[No QA] Avatar component decomposition#95050
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
| import useAvatarLoadError from './useAvatarLoadError'; | ||
|
|
||
| type UseUserAvatarSourceParams = { | ||
| source?: AvatarSource; |
There was a problem hiding this comment.
Some comments explaining source and avatarID would be helpful. It is ever valid to have both, or would a discriminated union be more precise?
If we could make some of these fields non-optional it might ripple some simplifications through the hook.
| import useAvatarLoadError from './useAvatarLoadError'; | ||
|
|
||
| type UseWorkspaceAvatarSourceParams = { | ||
| source?: AvatarSource; |
There was a problem hiding this comment.
Does it really make sense for all of these to be optional?
|
Thanks for making those changes @jmusial, but I remain skeptical that both |
I see your point that maybe this may be split down further. Unfortunately I need to run for today, I will pick it up from here next week. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77f2fd7407
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b53c297188
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| size, | ||
| fill, | ||
| testID, | ||
| avatarID = CONST.DEFAULT_NUMBER_ID, |
There was a problem hiding this comment.
When the compatibility Avatar is used for a workspace without avatarID (the prop is still optional), this default converts the missing value to 0; WorkspaceAvatar then hashes '0' for the default workspace icon colors. Before the refactor, the workspace path used avatarID?.toString() ?? '', so omitted IDs hashed the empty string. This silently changes the background/fill for legal calls such as a workspace avatar with only type and name, especially on fallback after an image error; pass undefined through for workspaces or keep the old empty-string fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is deliberate and I think acceptable.
|
@roryabraham I updated the PR so now for I explored some options with splitting out fallback logic out of the hooks but it overcomplicated the code IMO. let me know what you think, thx. And FYI I'll be OOO for Thu and Fri. |
|
@abdulrahuman5196 kind bump |
| /** Renders a user or workspace avatar depending on the `type` prop. | ||
| * This wrapper exists for backward compatibility. If possible use WorkspaceAvatar and UserAvatar directly. | ||
| */ | ||
| function Avatar({ |
There was a problem hiding this comment.
I presume we'll have a follow-up PR to update the callsites to use the correct level-two components directly? i.e: consuming components will use UserAvatar and WorkspaceAvatar directly, and we'll get rid of this wrapper/barrel entirely, right?
|
checking now |
abdulrahuman5196
left a comment
There was a problem hiding this comment.
@jmusial I had only minor comments in the code. Seems most comments are already addressed.
Other than that,
Why is the PR marked as No QA since this has obviously changed the code path of the Avatar component? And without the migration PRs if this moves to staging it needs QA runthrough. If we have different QA plan please mention it in PR.
And the Author's checklist only have screen recordings of mac, we should check in all platforms.
| const letterAvatar = parseLetterAvatarURL(resolvedSource); | ||
| if (letterAvatar) { | ||
| return ( | ||
| <AvatarContainer |
There was a problem hiding this comment.
Nit: the parent
<AvatarContainer
containerStyles={containerStyles}
testID={testID}
>
code is duplicated couple of times. If possible we should think of avoiding it.
There was a problem hiding this comment.
In theory we could, but with each Avatar primitive having slightly different logic I think this is more readable (avoiding conditionals in the code or pushing it into primitives)
| import React from 'react'; | ||
| import {View} from 'react-native'; | ||
|
|
||
| type UserLetterAvatarProps = AvatarPrimitivesCommonProps & { |
There was a problem hiding this comment.
The component name is AvatarLetter but why do we name its props UserLetterAvatarProps? which is little odd.
@abdulrahuman5196 No QA because it's a refactor not changing any user facing behaviour, which is also reflected in test steps. Just checking if avatars display the same as on prod. Same reason for just recording web. If you feel like more reboust test steps & recordings are needed let me know I can add them |
I think adding QA would be good to check on avatars, regardless the code is good. I will start to check on different platforms and approve in couple of hours if no issues are found. |
Explanation of Change
This PR decomposes the monolithic
Avatarcomponent into focusedUserAvatar,WorkspaceAvatar, constructed fromimage,icon, andinitialsavatar primitives while preserving the existing public API.It also keeps
Avatarsource resolution and load-error handling in dedicated hooks, making the rendering paths easier to understand, test, and extend.Fixed Issues
$ #95597
PROPOSAL:
Tests
Pure refactor, should be unnoticable to user.
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
MacOS: Chrome / Safari
Screen.Recording.2026-07-09.at.13.41.52.mov