Update profileCard.tsx #942
Conversation
Updated the Specific Profile Card with skeleton Loading component
|
@GradleD is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
app/[addressOrDomain]/page.tsxOops! Something went wrong! :( ESLint: 9.14.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 📜 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: 2
🧹 Outside diff range and nitpick comments (2)
components/UI/profileCard/profileCard.tsx (2)
Line range hint
140-172: Fix syntax and structure issuesThere are several syntax issues in the conditional rendering structure:
- Missing closing fragment tag
- Inconsistent indentation
- Complex nested conditional rendering
Apply these fixes:
- {totalBalance === null ? ( - <> - // ...skeleton components - </> - ) : ( - <> - - <Typography + {isLoading ? ( + <SkeletonUI /> + ) : ( + <> + <Typography type={TEXT_TYPE.BODY_SMALL} color="secondary" className={styles.accountCreationDate} > {sinceDate ? `${sinceDate}` : ""} </Typography> {/* Rest of the content */} - </> - )} + </> + )}Consider extracting the skeleton UI into a separate component
SkeletonUIto improve code organization and reusability.🧰 Tools
🪛 Biome
[error] 140-140: JSX fragment has no corresponding closing tag.
Opening fragment
Closing tag
(parse)
[error] 129-129: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
Line range hint
65-84: Improve error handling and cleanup in data fetchingThe current implementation has potential issues:
- Infinite retry loop could lead to excessive API calls
- No cleanup of AbortController on component unmount
- Missing loading state management during retries
Consider implementing these improvements:
+ const [isRetrying, setIsRetrying] = useState(false); useEffect(() => { + const controller = new AbortController(); const fetchTotalBalance = async () => { let attempts = 0; + setIsRetrying(true); while (attempts < MAX_RETRIES) { try { - const balance = await calculateTotalBalance(formattedAddress, "USD", { signal }); + const balance = await calculateTotalBalance( + formattedAddress, + "USD", + { signal: controller.signal } + ); setTotalBalance(balance); + setIsRetrying(false); return; } catch (err) { + if (err.name === 'AbortError') return; attempts++; console.error(`Attempt ${attempts} - Error fetching total balance:`, err); - if (attempts >= MAX_RETRIES) { - console.error("Failed to fetch total balance after multiple attempts."); - } else { - await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY)); - } + if (attempts < MAX_RETRIES) { + await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY)); + } } } + setIsRetrying(false); + console.error("Failed to fetch total balance after maximum attempts"); }; fetchTotalBalance(); + return () => controller.abort(); }, [formattedAddress]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/UI/profileCard/profileCard.tsx(4 hunks)
🧰 Additional context used
🪛 Biome
components/UI/profileCard/profileCard.tsx
[error] 140-140: JSX fragment has no corresponding closing tag.
Opening fragment
Closing tag
(parse)
[error] 129-129: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
[error] 172-172: Unexpected token. Did you mean {'}'} or }?
(parse)
| @@ -36,6 +37,7 @@ type ProfileCardProps = { | |||
| identity: Identity; | |||
| leaderboardData: LeaderboardToppersData; | |||
| isOwner: boolean; | |||
| isLoading:boolean; | |||
There was a problem hiding this comment.
Add missing isLoading prop to component parameters
The isLoading prop is defined in ProfileCardProps but not included in the component's parameter destructuring. This makes the prop inaccessible within the component.
Apply this fix:
const ProfileCard: FunctionComponent<ProfileCardProps> = ({
rankingData,
identity,
leaderboardData,
isOwner,
+ isLoading,
}) => {Also applies to: 44-48
There was a problem hiding this comment.
You add is Loading to the component props but you don't add it where the component is used + you unforce it.
Please test the stuff you send before a PR
There was a problem hiding this comment.
@fricoben, thank you for pointing this out. You're absolutely correct; the isLoading prop was added to the ProfileCard component but wasn't included where the component is used. To fix this, please ensure that isLoading is passed as a prop in all instances where ProfileCard is instantiated.
For example:
<ProfileCard
rankingData={rankingData}
identity={identity}
leaderboardData={leaderboardData}
isOwner={isOwner}
isLoading={isLoading}
/>Additionally, it's important to thoroughly test your changes before submitting a PR to avoid such oversights.
| {totalBalance === null ? ( // Check if balance is still loading | ||
| <> | ||
| //Updated the Specific Profile Card Component with skeleton Loading component | ||
| <Skeleton variant="text" width="80%" height={20} /> | ||
| <Skeleton variant="text" width="60%" height={30} className="mt-2" /> | ||
| <div className={styles.address_div}> | ||
| <div className="flex items-center gap-2"> | ||
| <Skeleton variant="rectangular" width={100} height={30} /> | ||
| <EyeIcon /> | ||
| </div> | ||
| </div> | ||
| </> | ||
| ) : ( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve loading state implementation
Several issues to address in the loading state implementation:
- Using
totalBalance === nullas a loading indicator is implicit. Consider using the explicitisLoadingprop instead. - The comment is incorrectly formatted and should be wrapped in braces.
- The skeleton UI could be more semantic with aria labels for accessibility.
Apply these improvements:
- {totalBalance === null ? ( // Check if balance is still loading
+ {isLoading ? (
<>
- //Updated the Specific Profile Card Component with skeleton Loading component
+ {/* Profile information loading state */}
<Skeleton
+ aria-label="Loading profile name"
variant="text"
width="80%"
height={20}
/>
<Skeleton
+ aria-label="Loading balance"
variant="text"
width="60%"
height={30}
className="mt-2"
/>
<div className={styles.address_div}>
<div className="flex items-center gap-2">
<Skeleton
+ aria-label="Loading address"
variant="rectangular"
width={100}
height={30}
/>
<EyeIcon />
</div>
</div>
</>Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 129-129: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| <EyeIcon /> | ||
| </div> | ||
| </div> | ||
| </> |
There was a problem hiding this comment.
Create a specific component skeleton in the /skeleton folder
| @@ -36,6 +37,7 @@ type ProfileCardProps = { | |||
| identity: Identity; | |||
| leaderboardData: LeaderboardToppersData; | |||
| isOwner: boolean; | |||
| isLoading:boolean; | |||
There was a problem hiding this comment.
You add is Loading to the component props but you don't add it where the component is used + you unforce it.
Please test the stuff you send before a PR
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/[addressOrDomain]/page.tsx (4)
86-87: Consider initializing isLoading to true.The
isLoadingstate is initialized tofalse, but since data fetching starts immediately, it would be more accurate to initialize it totrueto prevent a flash of unloaded content.- const [isLoading, setIsLoading] = useState(false); + const [isLoading, setIsLoading] = useState(true);
752-774: Improve readability of the loading condition.The loading condition could be more readable by extracting it into a descriptive variable.
- {(!portfolioAssets || !portfolioProtocols || loadingProtocols) ? ( + const isPortfolioLoading = !portfolioAssets || !portfolioProtocols || loadingProtocols; + {isPortfolioLoading ? (
Line range hint
544-589: Add missing dependencies to useCallback.The
fetchPortfolioDatacallback depends onshowNotificationbut it's not included in the dependency array. This could lead to stale closure issues.}, [fetchPortfolioProtocols, fetchPortfolioAssets]); + }, [fetchPortfolioProtocols, fetchPortfolioAssets, showNotification]);
Line range hint
586-589: Add loading state cleanup on component unmount.The loading state should be reset when the component unmounts to prevent state updates on an unmounted component.
useEffect(() => { const abortController = new AbortController(); if (!identity) return; fetchQuestData(identity.owner); fetchPageData(identity.owner); fetchPortfolioData(identity.owner, abortController); - return () => abortController.abort(); + return () => { + abortController.abort(); + setLoadingProtocols(false); + setIsLoading(false); + }; }, [identity]);
📜 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)
app/[addressOrDomain]/page.tsx(10 hunks)components/UI/profileCard/profileCard.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/UI/profileCard/profileCard.tsx
There was a problem hiding this comment.
- Lots of code improvements
- It's still not working, I don't see any skeleton on my profile card it has exactly the same problem than before the issue was created.
Test before sending a commit on github cause we're both loosing time here, I'll review one more time. If it's not working on my next review I'll need to give this issue to someone else :/
|
|
||
|
|
||
|
|
||
|
|
| @@ -103,6 +105,8 @@ export default function Page({ params }: AddressOrDomainProps) { | |||
| [] | |||
| ); | |||
|
|
|||
|
|
|||
|
|
|||
There was a problem hiding this comment.
useless space again please use prettier vs code extension to format everything
| @@ -83,6 +83,8 @@ export default function Page({ params }: AddressOrDomainProps) { | |||
| const [identity, setIdentity] = useState<Identity>(); | |||
| const [notFound, setNotFound] = useState(false); | |||
| const [isOwner, setIsOwner] = useState(false); | |||
| const [isLoading, setIsLoading] = useState(true); | |||
|
|
|||
There was a problem hiding this comment.
useless space + unprecise naming, what is loading here ? I would go for isProfileCardLoading
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
useleszs space again use prettier !
|
@fricoben if he can't handle this, i would love to help fix this. Thanks. |
|
Hey @fricoben I would love and help to work on this if he is not able to finish the task |
|
@fricoben Kindly assign someone else |
|
Will assign to someone else |
Please add the labels corresponding to the type of changes your PR introduces:
Summary by CodeRabbit
isLoading, to indicate the loading state of the ProfileCard.These changes aim to provide users with a smoother experience while waiting for data to load.