fix: display same Avatar from WalletButton in dashboard page #939
Conversation
|
@wheval is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
components/UI/avatar.tsx (3)
35-35: Good addition of accessible alt text!The dynamic alt text improves accessibility. However, consider making it more descriptive for screen readers.
- alt={`${profile?.name?.length !== 0 ? profile?.name : "User"}'s avatar`} + alt={`Profile picture for ${profile?.name?.length !== 0 ? profile?.name : "User"}`}
Line range hint
19-24: Consider adding error handling for profile data fetchingThe profile data fetching in useEffect should include error handling to gracefully handle API failures.
useEffect(() => { if (!starknetIdNavigator) return; - starknetIdNavigator.getProfileData(address).then((profile) => { - setProfile(profile); - }); + starknetIdNavigator.getProfileData(address) + .then((profile) => { + setProfile(profile); + }) + .catch((error) => { + console.error('Failed to fetch profile:', error); + setProfile(null); + }); }, [starknetIdNavigator, address]);
Line range hint
8-13: Strengthen type safety for the StarkProfile interfaceThe component relies on optional chaining for profile properties. Consider defining stricter types.
type AvatarProps = { address: string; width?: string; + onError?: (error: Error) => void; }; +interface ProfileData extends StarkProfile { + name: string | null; + profilePicture: string | null; +}components/UI/profileCard/profileCard.tsx (3)
51-51: Consider removing unusedaccountStatusvariable.The
accountStatusis destructured fromuseAccountbut never used in the component.- const { address, status: accountStatus } = useAccount(); + const { address } = useAccount();
116-117: Consider type improvements for the width prop.The Avatar implementation successfully achieves the PR objective of consistent avatar display. However, consider passing the width as a number instead of a string for better type safety.
- <Avatar width="120" address={formattedAddress} /> + <Avatar width={120} address={formattedAddress} />
Line range hint
62-83: Consider optimizing the retry logic.The current implementation has the following concerns:
- The while(true) loop with MAX_RETRIES=1000 could lead to excessive retries
- No exponential backoff strategy for retries
- No break condition in the catch block despite reaching max retries
Consider implementing exponential backoff and proper loop termination:
const fetchTotalBalance = async () => { let attempts = 0; - while (true) { + while (attempts < MAX_RETRIES) { try { const balance = await calculateTotalBalance(formattedAddress, "USD", {signal}); setTotalBalance(balance); - return; // Exit if successful + break; } catch (err) { attempts++; console.error(`Attempt ${attempts} - Error fetching total balance:`, err); - if (attempts >= MAX_RETRIES) { + if (attempts === MAX_RETRIES) { console.error("Failed to fetch total balance after multiple attempts."); + break; } else { - await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY)); + await new Promise((resolve) => + setTimeout(resolve, RETRY_DELAY * Math.pow(2, attempts - 1)) + ); } } } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
components/UI/avatar.tsx(1 hunks)components/UI/profileCard/profileCard.tsx(4 hunks)
🔇 Additional comments (3)
components/UI/avatar.tsx (1)
Line range hint 15-41: Verify profile picture loading behavior
Since this component is central to fixing the avatar display issue (#928), we should verify its usage across the application.
✅ Verification successful
Avatar component implementation is consistently used across the application
The verification shows that:
- The Avatar component is properly imported and used across 7 different components/pages
- All instances use the component with the correct props (address and optional width)
- There are no direct img tag implementations for profile pictures that bypass the Avatar component
- The component is used in key UI areas including:
- Leaderboard (RankCard, RankingsTable, page)
- Navigation (walletButton)
- Profile displays (profileCard)
- Quest-related UI (questTag)
The profile picture loading behavior is consistently implemented through this centralized Avatar component, which aligns with the fix for issue #928.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instances where Avatar component is used
# to ensure consistent implementation across the application
echo "Searching for Avatar component usage..."
rg -l "import.*Avatar.*from.*components/UI/avatar" --type tsx
echo "\nChecking for direct img tag usage that might need to be replaced with Avatar..."
rg -l "img.*src=.*profile.*picture" --type tsx
Length of output: 430
Script:
#!/bin/bash
# Let's try without file type restriction since tsx might not be recognized
# First, find all potential Avatar component imports
echo "Searching for Avatar component usage..."
rg -l "import.*Avatar.*from.*['\"].*avatar['\"]"
echo "\nChecking for direct img tag usage with profile pictures..."
rg -l "img.*src.*profile.*picture"
# Let's also check for the actual component usage
echo "\nChecking for Avatar component usage in JSX..."
rg "<Avatar"
Length of output: 1255
components/UI/profileCard/profileCard.tsx (2)
10-10: LGTM! Import changes align with the PR objectives.
The switch from useStarkProfile to useAccount and the addition of the Avatar component import support the goal of consistent avatar display across the application.
Also applies to: 26-26
Line range hint 1-249: Implementation successfully meets PR objectives.
The changes effectively resolve issue #928 by:
- Using the same Avatar component across the application
- Properly handling the profile picture display on the dashboard
- Maintaining consistent UI elements with appropriate fallbacks
The code is well-structured and maintains good error handling and loading states.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -47,7 +48,7 @@ const ProfileCard: FunctionComponent<ProfileCardProps> = ({ | |||
| const formattedAddress = ( | |||
| identity.owner.startsWith("0x") ? identity.owner : `0x${identity.owner}` | |||
| ) as Address; | |||
| const { data: profileData } = useStarkProfile({ address: formattedAddress }); | |||
| const { address, status: accountStatus } = useAccount(); | |||
There was a problem hiding this comment.
You don't use address or accountStatus here, do you ?
| @@ -112,8 +113,8 @@ const ProfileCard: FunctionComponent<ProfileCardProps> = ({ | |||
| <div className={styles.dashboard_profile_card}> | |||
| <div className={styles.left}> | |||
| <div className={styles.profile_picture_div}> | |||
| {profileData?.profilePicture ? ( | |||
| <img src={profileData.profilePicture} className="rounded-full" /> | |||
| { formattedAddress && formattedAddress?.length !== 0 ? ( // show the avatar of the address in the URL | |||
There was a problem hiding this comment.
simplify
{formattedAddress?.length ? (
...
There was a problem hiding this comment.
how did i not see that, on it..
|
@fricoben please review, thanks! |
Added alt text for the avatar image and displayed the avatar on the dashboard page.
Please add the labels corresponding to the type of changes your PR introduces:
Resolves: #928
Other information
Here's how it looks.
Summary by CodeRabbit
New Features
Avatarcomponent with analtattribute for user profile images.ProfileCardcomponent to simplify data flow by directly using the user's address for rendering profile pictures.Bug Fixes
ProfileCard, ensuring default icons are displayed when necessary.