-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Personalized strategy ordering enhancement #1636
Personalized strategy ordering enhancement #1636
Conversation
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
isConnected
usinguseState
anduseEffect
is 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
isError
assignment to directly usequery.isError
enhances code readability and maintainability by directly leveraging the properties of theuseQuery
hook 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
useEffect
hook to reseterror
andcellarData
states 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
isDeprecated
andbuttonProps
in 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
props
withbuttonProps
in the button component ensures that all passed props are correctly applied to theSecondaryButton
. This change leverages the destructuredbuttonProps
for 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 theuseEffect
hook 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
CardHeading
component to directly includetimeline
andapyChartLabel(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
,isBefore
fromdate-fns
,useUserDataAllStrategies
fromdata/hooks
,useAccount
fromwagmi
, andBigNumber
frombignumber.js
supports 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
useMemo
hook, 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-fns
andBigNumber.js
are well-integrated into this logic.src/components/_cards/PortfolioCard/index.tsx (2)
- 63-63: Renaming the
isConnected
variable toconnected
within theuseAccount
hook is a thoughtful change to avoid naming conflicts, especially considering the introduction of local state management for theisConnected
state. This ensures clarity in variable naming and improves code readability.- 75-79: Introducing local state management for the
isConnected
state usinguseState
anduseEffect
is 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
.
d0c4267
to
420918b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
, andResolvedConfig
is 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
getAddress
to normalize addresses is good practice. However, ensure thattoken!.address
is always defined to avoid potential runtime errors.- The check
if (!balance.value.isZero())
before pushing todepositAssetBalances
is 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
fetchBalance
call to gracefully handle any failures, ensuring the application's stability.Consider adding error handling around the
fetchBalance
call to improve resilience and user experience in case of API failures or network issues.
140-140
: The inclusion ofdepositAssetBalances
in 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
, andrETH
toacceptedARBDepositTokens
and the introduction ofacceptedOPTDepositTokens
are 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 ofacceptedDepositTokensByChain
mapping 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fromuseUserBalances
touseUserBalance
is correctly implemented.src/data/hooks/useUserStrategyData.ts (1)
8-8
: The update fromuseUserBalances
touseUserBalance
is correctly implemented.src/components/_sidebar/PortofolioItem.tsx (1)
12-12
: The update fromuseUserBalances
touseUserBalance
is correctly implemented.src/components/_forms/UnstakeForm.tsx (1)
25-25
: The update fromuseUserBalances
touseUserBalance
is correctly implemented.src/components/_buttons/DepositAndWithdrawButton.tsx (1)
13-13
: The update fromuseUserBalances
touseUserBalance
is correctly implemented.src/components/_forms/BondForm/index.tsx (1)
32-32
: The update fromuseUserBalances
touseUserBalance
is 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
: AddingdepositTokens
to the returned object fromgetStrategyData
aligns with the PR's objectives to enhance strategy ordering and personalization.src/components/_forms/WithdrawForm.tsx (5)
35-35
: Ensure theuseUserBalance
hook adequately meets the component's requirements for fetching and utilizing user balance data, considering the switch fromuseUserBalances
.
90-90
: Consider adding error handling for theuseUserBalance
hook 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 theuseUserBalance
hook adequately meets the component's requirements for fetching and utilizing user balance data, considering the switch fromuseUserBalances
.
143-143
: Consider adding error handling for theuseUserBalance
hook 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 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
} catch (error) { | ||
console.error("error", error); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
window.removeEventListener("scroll", () => {}); | ||
setScrolled(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).WithdrawButton
for improved conditional rendering based on props.PortfolioCard
andLayoutWithSidebar
.DepositAndWithdrawButton
,ApyPerformanceCard
, multiple forms) to use theuseUserBalance
hook for a more accurate retrieval of user balance data.