Add skeleton for portfolio value sum #947
Conversation
|
@Yunusabdul38 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 the modification of the Changes
Assessment against linked issues
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
components/UI/profileCard/profileCard.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: 0
🧹 Outside diff range and nitpick comments (2)
components/UI/profileCard/profileCard.tsx (2)
Line range hint
169-194: Consider consolidating loading states.There appears to be two different loading implementations:
- The new
Loadingcomponent wrapper- The existing MUI
Skeletoncomponent inside the TypographyThis might lead to redundant loading states. Consider:
- Using only the new
Loadingcomponent for consistency- Removing the MUI Skeleton implementation
- {totalBalance !== null ? ( - hidePortfolio ? ( - '******' - ) : ( - `$${totalBalance?.toFixed(2)}` - ) - ) : ( - <Skeleton - variant='text' - width={60} - height={30} - /> - )} + {hidePortfolio ? '******' : `$${totalBalance?.toFixed(2)}`}
Line range hint
78-106: Improve error handling and cleanup in balance fetching.Several improvements could enhance the robustness of the balance fetching logic:
- The while(true) loop with MAX_RETRIES could lead to infinite retries
- The abort controller is defined but not properly utilized for cleanup
- Error states are not surfaced to the UI
Consider implementing these improvements:
useEffect(() => { const fetchTotalBalance = async () => { - let attempts = 0; - while (true) { + for (let attempts = 0; attempts < MAX_RETRIES; attempts++) { try { const balance = await calculateTotalBalance(formattedAddress, 'USD', { signal, }); setTotalBalance(balance); - return; + break; } catch (err) { - 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 - 1) { + setTotalBalance(0); // or some error state + console.error('Failed to fetch total balance after multiple attempts.'); + return; + } + await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY)); } } }; if (address) { fetchTotalBalance(); } + + return () => { + controller.abort(); + }; }, [formattedAddress, address]);
📜 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 (1)
components/UI/profileCard/profileCard.tsx(3 hunks)
🔇 Additional comments (2)
components/UI/profileCard/profileCard.tsx (2)
32-32: LGTM: Import statement aligns with PR objectives.
The import of the Loading component is correctly placed and follows the project's component organization structure.
Line range hint 169-194: Verify loading behavior matches design.
While the skeleton loader implementation meets the basic requirements from issue #936, please ensure that:
- The loading behavior matches the before/after images provided in the issue
- The loading state properly handles the privacy icon visibility
Consider implementing a comprehensive loading state that includes:
- Skeleton for the amount
- Placeholder for the privacy icon
- Consistent loading indicators across all dynamic content in the card
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Nope, we want a skeleton like so https://mui.com/material-ui/react-skeleton/
Check the /skeletons and the rest of the codebase to see, if you change the skeleton type then we're good
There was a problem hiding this comment.
Nope, we want a skeleton not a spinner (like so https://mui.com/material-ui/react-skeleton/)
Check the /skeletons and the rest of the codebase to see, if you change the skeleton type then we're good
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
A skeleton loader for the portfolio value sum to address the loading delay issue was added
Resolves: #936
Other information
i make use of the loading component in the project file, the loading component has two type of loading component
1- A spinner and
2- A skeleton
i try using the spinner but i noticed it makes the ui to look more buggy but it has a better indication for a loading feature but due to the ui issue i end up using the skeleton type (loading) which looks much more better
if you want more modification to this please let me know
Summary by CodeRabbit
New Features
Bug Fixes