Personalized strategy ordering enhancement #1636
Conversation
otsalex
requested review from
zmanian,
poldsam,
51N84D,
philipjames44,
henrio123 and
josephterrigno
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes in this update encompass a comprehensive refinement of component functionality, state management, and data handling across various files to elevate user experience and optimize system performance. These updates include adjustments to hook usage, variable naming for clarity, and enhancements in data retrieval mechanisms throughout the codebase. Changes
Possibly related issues
Poem
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (2)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- src/components/Nav.tsx (2 hunks)
- src/components/_buttons/WithdrawButton.tsx (2 hunks)
- src/components/_cards/ApyPerfomanceCard.tsx (1 hunks)
- src/components/_cards/PortfolioCard/index.tsx (2 hunks)
- src/components/_layout/LayoutWithSidebar.tsx (1 hunks)
- src/components/_pages/PageHome.tsx (4 hunks)
- src/data/hooks/useAllStrategiesData.ts (1 hunks)
- src/data/hooks/useUserDataAllStrategies.ts (3 hunks)
Additional comments (11)
src/components/_layout/LayoutWithSidebar.tsx (1)
- 17-21: The introduction of local state management for
isConnectedusinguseStateanduseEffectis a good practice to handle potential asynchronous updates and prevent rendering issues in Next.js. This change ensures that the component's state is correctly updated in response to changes in the user's connection status.src/data/hooks/useUserDataAllStrategies.ts (1)
- 63-63: Simplifying the
isErrorassignment to directly usequery.isErrorenhances code readability and maintainability by directly leveraging the properties of theuseQueryhook from@tanstack/react-query. This change makes the error handling more straightforward and concise.src/data/hooks/useAllStrategiesData.ts (1)
- 43-46: Adding a cleanup function in the
useEffecthook to reseterrorandcellarDatastates upon component unmounting is an excellent practice for managing component lifecycle and preventing memory leaks. This ensures that the component cleans up after itself, avoiding potential issues with stale state or memory usage.src/components/_buttons/WithdrawButton.tsx (2)
- 23-23: Directly destructuring
isDeprecatedandbuttonPropsin the function parameters enhances the clarity and readability of the component's prop usage. This approach makes it easier to understand which props are being used and how, promoting better maintainability.- 62-64: Replacing
propswithbuttonPropsin the button component ensures that all passed props are correctly applied to theSecondaryButton. This change leverages the destructuredbuttonPropsfor more explicit and readable prop passing, aligning with best practices for component composition.src/components/Nav.tsx (1)
- 44-45: Adding
setScrolled(false)in the cleanup function of theuseEffecthook for the scroll event listener is a good practice for resetting component state upon unmounting. This ensures that the component's state is correctly managed and does not retain any stale values when the component is no longer in use.src/components/_cards/ApyPerfomanceCard.tsx (1)
- 115-121: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [58-58]
Modifying the rendering within the
CardHeadingcomponent to directly includetimelineandapyChartLabel(cellarConfig)without the enclosing<Text>component simplifies the rendering logic and improves the readability of the component. This approach makes the component's structure more straightforward and easier to understand.src/components/_pages/PageHome.tsx (2)
- 42-46: The addition of new imports such as
add,isBeforefromdate-fns,useUserDataAllStrategiesfromdata/hooks,useAccountfromwagmi, andBigNumberfrombignumber.jssupports the enhanced functionality introduced in this file. These imports are necessary for the updated sorting logic and handling user data, contributing to the overall objectives of the pull request.- 292-333: The revised sorting logic within the
useMemohook, which sorts strategy data based on user-held strategies, newness, rewards, and TVL, is a significant enhancement. This logic effectively prioritizes strategies according to the outlined criteria, improving the user experience by presenting strategies in a personalized and optimized manner. The use ofisConnected,userData, and various utility functions fromdate-fnsandBigNumber.jsare well-integrated into this logic.src/components/_cards/PortfolioCard/index.tsx (2)
- 63-63: Renaming the
isConnectedvariable toconnectedwithin theuseAccounthook is a thoughtful change to avoid naming conflicts, especially considering the introduction of local state management for theisConnectedstate. This ensures clarity in variable naming and improves code readability.- 75-79: Introducing local state management for the
isConnectedstate usinguseStateanduseEffectis a good practice for handling potential asynchronous updates and preventing rendering issues. This change ensures that the component's state is correctly updated in response to changes in the user's connection status, similar to the approach taken inLayoutWithSidebar.tsx.
otsalex
force-pushed
the
Personalized-Strategy-Ordering-Enhancement
branch
from
d0c4267
to
420918b
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- src/components/Nav.tsx (2 hunks)
- src/components/_buttons/WithdrawButton.tsx (2 hunks)
- src/components/_cards/ApyPerfomanceCard.tsx (1 hunks)
- src/components/_cards/PortfolioCard/index.tsx (2 hunks)
- src/components/_layout/LayoutWithSidebar.tsx (1 hunks)
- src/components/_pages/PageHome.tsx (4 hunks)
- src/data/hooks/useAllStrategiesData.ts (1 hunks)
- src/data/hooks/useUserDataAllStrategies.ts (3 hunks)
Files skipped from review as they are similar to previous changes (8)
- src/components/Nav.tsx
- src/components/_buttons/WithdrawButton.tsx
- src/components/_cards/ApyPerfomanceCard.tsx
- src/components/_cards/PortfolioCard/index.tsx
- src/components/_layout/LayoutWithSidebar.tsx
- src/components/_pages/PageHome.tsx
- src/data/hooks/useAllStrategiesData.ts
- src/data/hooks/useUserDataAllStrategies.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/components/_pages/PageHome.tsx (4 hunks)
- src/data/actions/common/getUserDataAllStrategies.ts (3 hunks)
- src/data/tokenConfig.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/components/_pages/PageHome.tsx
Additional Context Used
Additional comments not posted (5)
src/data/actions/common/getUserDataAllStrategies.ts (3)
8-11: The addition of imports forfetchBalance,getAddress,getAcceptedDepositAssetsByChain, andResolvedConfigis aligned with the PR's objective to enhance strategy ordering based on various factors including user holdings. Ensure these imported functions and types are used appropriately within the file.
81-96: The loop added to fetch balances for accepted deposit assets is a crucial part of enhancing the strategy ordering based on user holdings. However, there are a few points to consider for improvement:
- The use of
getAddressto normalize addresses is good practice. However, ensure thattoken!.addressis always defined to avoid potential runtime errors.- The check
if (!balance.value.isZero())before pushing todepositAssetBalancesis efficient for filtering out tokens with zero balance. This aligns with the goal of prioritizing strategies based on user holdings.- Consider adding error handling for the
fetchBalancecall to gracefully handle any failures, ensuring the application's stability.Consider adding error handling around the
fetchBalancecall to improve resilience and user experience in case of API failures or network issues.
140-140: The inclusion ofdepositAssetBalancesin the returned data structure supports the PR's objective by potentially influencing strategy ordering based on user holdings. This addition is well-integrated with the rest of the function's return structure.src/data/tokenConfig.ts (2)
565-565: The addition ofWETH,wstETH, andrETHtoacceptedARBDepositTokensand the introduction ofacceptedOPTDepositTokensare significant for supporting strategy ordering across different chains. This aligns with the PR's objective to enhance user experience by considering various factors, including chain-specific assets.
588-592: The creation ofacceptedDepositTokensByChainmapping tokens by chain for Ethereum, Arbitrum, and Optimism is a strategic enhancement. It facilitates the dynamic retrieval of accepted deposit assets based on the user's chain, supporting the PR's goal of personalized strategy ordering.
| export const getAcceptedDepositAssetsByChain = (chainId: string) => { | ||
| return acceptedDepositTokensByChain[chainId] | ||
| ? getTokenConfig(acceptedDepositTokensByChain[chainId], chainId) | ||
| : [] | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The getTokenConfig function's implementation appears to align with the requirements for supporting chain-specific configurations by mapping over a list of token symbols and returning configurations that match both the symbol and the chain. However, there seems to be no test coverage for this function, which is crucial for ensuring its reliability and correctness across different scenarios. It's recommended to add tests for getTokenConfig to verify its functionality and handle potential edge cases.
Analysis chain
The addition of the getAcceptedDepositAssetsByChain function is crucial for retrieving accepted deposit assets by chain. This function enhances the flexibility and scalability of the strategy ordering feature by allowing for chain-specific configurations. Ensure that the getTokenConfig function called within getAcceptedDepositAssetsByChain is correctly implemented and tested for various scenarios to prevent potential issues.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the implementation and test coverage of getTokenConfig function.
rg --type ts "function getTokenConfig"
Length of output: 133
Script:
#!/bin/bash
# Find the implementation details of getTokenConfig
rg --type ts -A 10 "function getTokenConfig" src/data/tokenConfig.ts
# Search for references to getTokenConfig in test files
rg --type ts "getTokenConfig" --glob '*test.ts' --glob '*spec.ts'
Length of output: 336
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/data/actions/common/getUserDataAllStrategies.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/data/actions/common/getUserDataAllStrategies.ts
Additional Context Used
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/components/_pages/PageHome.tsx (4 hunks)
- src/data/actions/common/getUserDataAllStrategies.ts (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/components/_pages/PageHome.tsx
- src/data/actions/common/getUserDataAllStrategies.ts
Additional Context Used
|
I think the code also needs to be aware of quantity of the accepted assets in users wallets. We should probably us the amount of the asset as an input and we probably need to couple that with coingecko prices. |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (14)
- src/components/_buttons/DepositAndWithdrawButton.tsx (2 hunks)
- src/components/_cards/PortfolioCard/index.tsx (3 hunks)
- src/components/_forms/BondForm/index.tsx (2 hunks)
- src/components/_forms/UnstakeForm.tsx (2 hunks)
- src/components/_forms/WithdrawForm.tsx (2 hunks)
- src/components/_forms/WithdrawQueueForm.tsx (2 hunks)
- src/components/_modals/UnstakeModal.tsx (2 hunks)
- src/components/_pages/PageHome.tsx (4 hunks)
- src/components/_sidebar/PortofolioItem.tsx (2 hunks)
- src/data/actions/common/getStrategyData.ts (5 hunks)
- src/data/actions/common/getUserDataAllStrategies.ts (2 hunks)
- src/data/hooks/useUserBalance.tsx (1 hunks)
- src/data/hooks/useUserBalances.ts (1 hunks)
- src/data/hooks/useUserStrategyData.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- src/data/hooks/useUserBalance.tsx
Files skipped from review as they are similar to previous changes (3)
- src/components/_cards/PortfolioCard/index.tsx
- src/components/_pages/PageHome.tsx
- src/data/actions/common/getUserDataAllStrategies.ts
Additional comments not posted (19)
src/components/_modals/UnstakeModal.tsx (1)
9-9: The update fromuseUserBalancestouseUserBalanceis correctly implemented.src/data/hooks/useUserStrategyData.ts (1)
8-8: The update fromuseUserBalancestouseUserBalanceis correctly implemented.src/components/_sidebar/PortofolioItem.tsx (1)
12-12: The update fromuseUserBalancestouseUserBalanceis correctly implemented.src/components/_forms/UnstakeForm.tsx (1)
25-25: The update fromuseUserBalancestouseUserBalanceis correctly implemented.src/components/_buttons/DepositAndWithdrawButton.tsx (1)
13-13: The update fromuseUserBalancestouseUserBalanceis correctly implemented.src/components/_forms/BondForm/index.tsx (1)
32-32: The update fromuseUserBalancestouseUserBalanceis correctly implemented.src/data/actions/common/getStrategyData.ts (3)
110-110: Consider removing or clarifying the purpose of commented-out code sections related to custom reward APY overrides for better code maintainability and clarity.
149-149: Similarly, clarify or remove the commented-out sections for extra rewards APY calculations to ensure the codebase remains clean and understandable.
96-96: AddingdepositTokensto the returned object fromgetStrategyDataaligns with the PR's objectives to enhance strategy ordering and personalization.src/components/_forms/WithdrawForm.tsx (5)
35-35: Ensure theuseUserBalancehook adequately meets the component's requirements for fetching and utilizing user balance data, considering the switch fromuseUserBalances.
90-90: Consider adding error handling for theuseUserBalancehook to manage scenarios where fetching the user's balance might fail.
90-90: Verify that all potential error scenarios, including network errors and smart contract call failures, are adequately handled during the withdrawal operation.
90-90: Ensure that the form validation logic comprehensively covers all necessary scenarios, including edge cases, for user inputs.
90-90: Review the component's UI for responsiveness and accessibility, ensuring it provides a consistent user experience across different devices and screen sizes.src/components/_forms/WithdrawQueueForm.tsx (5)
31-31: Ensure theuseUserBalancehook adequately meets the component's requirements for fetching and utilizing user balance data, considering the switch fromuseUserBalances.
143-143: Consider adding error handling for theuseUserBalancehook to manage scenarios where fetching the user's balance might fail.
143-143: Verify that all potential error scenarios, including network errors and smart contract call failures, are adequately handled during the withdrawal queue operation.
143-143: Ensure that the form validation logic comprehensively covers all necessary scenarios, including edge cases, for user inputs.
143-143: Review the component's UI for responsiveness and accessibility, ensuring it provides a consistent user experience across different devices and screen sizes.
Signed-off-by: henrio123 <henriots33@gmail.com>
| } catch (error) { | ||
| console.error("error", error); | ||
| } |
There was a problem hiding this comment.
Enhance error logging in fetchBalances for better debugging and user feedback.
- console.error("error", error);
+ console.error("Failed to fetch user balances:", error);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| } catch (error) { | |
| console.error("error", error); | |
| } | |
| } catch (error) { | |
| console.error("Failed to fetch user balances:", error); | |
| } |
| window.removeEventListener("scroll", () => {}); | ||
| setScrolled(false); |
There was a problem hiding this comment.
Incorrect removal of event listener in useEffect cleanup function.
- window.removeEventListener("scroll", () => {});
+ const handleScroll = () => {
+ if (window.scrollY >= 80) {
+ setScrolled(true);
+ } else {
+ setScrolled(false);
+ }
+ };
+ window.addEventListener("scroll", handleScroll);
+ return () => {
+ window.removeEventListener("scroll", handleScroll);
+ };This change ensures that the same function instance is used for both adding and removing the event listener, preventing potential memory leaks and ensuring proper cleanup.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| window.removeEventListener("scroll", () => {}); | |
| setScrolled(false); | |
| const handleScroll = () => { | |
| if (window.scrollY >= 80) { | |
| setScrolled(true); | |
| } else { | |
| setScrolled(false); | |
| } | |
| }; | |
| window.addEventListener("scroll", handleScroll); | |
| return () => { | |
| window.removeEventListener("scroll", handleScroll); | |
| }; |
Issue #1627
Description
Added ordering for strategies on vaults page based on 4 priorities:
Changes
Summary by CodeRabbit
WETH,wstETH,rETH) and created mappings for accepted deposit tokens by chain (Ethereum, Arbitrum, Optimism).WithdrawButtonfor improved conditional rendering based on props.PortfolioCardandLayoutWithSidebar.DepositAndWithdrawButton,ApyPerformanceCard, multiple forms) to use theuseUserBalancehook for a more accurate retrieval of user balance data.