Earn protocol WIP #17 #551
Conversation
# Conflicts: # apps/earn-protocol/app/earn/[network]/position/[vaultId]/page.tsx
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe changes in this pull request involve restructuring the handling of vault management and rebalancing activity data across multiple components in the application. Key modifications include the removal of several components and data structures related to vault management, the introduction of new components such as Changes
Possibly related PRs
Suggested reviewers
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: 24
🧹 Outside diff range and nitpick comments (13)
packages/app-earn-ui/src/components/molecules/HeadingWithCards/HeadingWithCards.module.scss.d.ts (1)
1-16: Could use some documentation love, just sayin'Hey, while the code is solid, throwing in some JSDoc comments would make it even better for the next dev who has to deal with this.
Here's a suggestion to make it more documentation-friendly:
+/** + * Style definitions for the HeadingWithCards component + * @property card - Styles for individual card containers + * @property cardDescription - Styles for card description text + * @property cardsWrapper - Styles for the cards container + * @property cardTitle - Styles for card titles + * @property description - Styles for main description + * @property heading - Styles for main heading + * @property headingIcons - Styles for icons in heading + * @property wrapper - Styles for the main component wrapper + */ export type Styles = { card: string cardDescription: string cardsWrapper: string cardTitle: string description: string heading: string headingIcons: string wrapper: string } +/** Union type of all available class names for type-safe access */ export type ClassNames = keyof Styles declare const styles: Styles export default stylesapps/earn-protocol/app/earn/rebalance-activity/page.tsx (1)
4-8: Bruh, where's your TypeScript at? 🤔The component lacks proper TypeScript types. Let's make TypeScript happy!
-const RebalanceActivityPage = () => { +const RebalanceActivityPage: React.FC = () => {apps/earn-protocol/features/rebalance-activity/table/dummyData.ts (2)
17-18: Those provider links though... 💀All provider links are set to '/'. Either you're really into root directories, or someone got lazy with the dummy data. Also, might want to consider adding more providers than just Summer.fi if this is supposed to be representative test data.
Consider adding realistic provider links and varying the providers:
- link: '/', - label: 'Summer.fi', + link: 'https://summer.fi/tx/0x123...', + label: 'Summer.fi',Also applies to: 34-35, 51-52
11-13: Those amounts looking sus! 👀"123123" for every amount? Really? That's about as random as a rigged dice!
Spice it up with more realistic and varied amounts:
- value: '123123', + value: '1000000', // 1M USDCAlso applies to: 28-30, 45-47
packages/app-earn-ui/src/components/organisms/Table/Table.module.scss (1)
67-69: Bruh... 'unset' padding with no replacement? That's kinda sus! 🚩Like, I get what you're trying to do here, but completely removing padding without a replacement value might make your content look like it's having a claustrophobic episode. Maybe consider setting a minimal padding instead?
Try this instead:
> td { - padding: unset; + padding: 4px 0; /* Maintain some breathing room */ }packages/app-earn-ui/src/components/molecules/TableCarousel/TableCarousel.tsx (1)
13-22: Bruh, your types could use some love! 💝The interface is solid but could be more robust with readonly modifiers and some documentation. Let me sprinkle some TypeScript magic on this:
+/** + * Props for the TableCarousel component + * @property carouselData - Array of carousel items containing title, description, and link + */ interface TableCarouselProps { - carouselData: { - title: string - description: string - link: { - label: string - href: string - } - }[] + readonly carouselData: readonly { + readonly title: string + readonly description: string + readonly link: { + readonly label: string + readonly href: string + } + }[] }apps/earn-protocol/components/organisms/RebalancingActivity/RebalancingActivity.tsx (1)
Line range hint
38-43: Yo dawg, I heard you like typos! 🤦♂️There's a missing comma after "Strategy" and "It" shouldn't be capitalized in the middle of a sentence. Here's the fix:
- Rebalancing crucial in attaining the best possible yield for a Strategy, It is responsible + Rebalancing is crucial in attaining the best possible yield for a Strategy, it is responsibleLike, how do you expect the code to rebalance properly when the documentation can't even balance its own grammar? 😅
apps/earn-protocol/features/rebalance-activity/components/RebalanceActivityView/RebalanceActivityView.tsx (1)
42-44: Chill interface, but could use some docs! 📚The interface is clean, but adding JSDoc would make it even better for other devs.
Add documentation like this:
+/** + * Props for the RebalanceActivityView component + * @property {RebalancingActivityRawData[]} rawData - Array of raw rebalancing activity data to be displayed + */ interface RebalanceActivityViewProps { rawData: RebalancingActivityRawData[] }packages/app-earn-ui/src/components/organisms/Table/Table.tsx (1)
86-108: Bruh, what's with all these inline styles? 🤦♂️These styles are repeated and should be moved to your CSS module. Keep it DRY (Don't Repeat Yourself), not WET (Write Everything Twice)!
Move these styles to your
Table.module.scss:+ // Table.module.scss + .headerCell { + display: flex; + align-items: center; + gap: var(--spacing-space-x-small); + width: fit-content; + } + + .sortable { + cursor: pointer; + } - <div - style={{ - display: 'flex', - alignItems: 'center', - gap: 'var(--spacing-space-x-small)', - width: 'fit-content', - cursor: column.sortable ? 'pointer' : 'default', - }} + <div + className={`${styles.headerCell} ${column.sortable ? styles.sortable : ''}`}packages/app-types/types/src/icons/index.ts (1)
238-239: Yo, these new social icons look pretty sweet! 🎸The additions follow the existing naming conventions and are placed in a logical section. Nice and clean!
Just a heads up - since these are social media icons, you might want to consider adding other common platforms too. No pressure though, we can always add more later if needed.
apps/earn-protocol/app/earn/[network]/position/[vaultId]/page.tsx (2)
Line range hint
171-173: Yo, this looks pretty clean! Just a few suggestions to make it even better.The implementation is solid, but consider adding:
- Loading states for better UX
- Error boundaries to catch rendering failures
- Type safety for the dummy data
Here's how you could add loading states:
const EarnVaultOpenManagePage = async ({ params }: EarnVaultOpenManagePageProps) => { const networkId = subgraphNetworkToId(params.network) try { const [selectedVault, { vaults }] = await Promise.all([ getVaultDetails({ vaultAddress: params.vaultId, chainId: networkId, }), getVaultsList(), ]) if (!selectedVault) { return <Text>No vault found with the id {params.vaultId} on the network {params.network}</Text> } return <VaultGridPreview {...props} /> } catch (error) { return <Text>Failed to load vault details: {error.message}</Text> } }
Yo, we've got a dummy data situation that needs some attention 🚨
Look, I hate to be that person, but we're using dummy data in production code and that's about as smart as bringing a rubber duck to a sword fight. The
rebalancingActivityRawDatais being used in both:
apps/earn-protocol/app/earn/rebalance-activity/page.tsxapps/earn-protocol/app/earn/[network]/position/[vaultId]/page.tsxAnd there's zero, zilch, nada implementation of actual data fetching. Like, where's the API integration? The real data? The whole thing's running on mock data like it's still in development, and that's not cool in production, my friend.
You might want to:
- Implement actual data fetching logic for rebalancing activity
- Remove the dummy data imports
- Set up proper API integration for both the main rebalancing page and the vault position view
🔗 Analysis chain
Line range hint
171-252: Verify the rebalancing activity data integration.Let's make sure this dummy data isn't causing issues elsewhere.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components using the dummy data echo "Checking for other components using dummy data..." rg -l "rebalance-activity/table/dummyData" # Look for actual data fetching implementations echo "\nChecking for potential real implementations..." rg -l "rebalance.*activity.*api"Length of output: 415
Script:
#!/bin/bash # Let's check what's actually in those files to see how the dummy data is used echo "Checking rebalance activity page implementation..." cat apps/earn-protocol/app/earn/rebalance-activity/page.tsx echo "\nLet's also look for any real data fetching logic..." rg -A 5 "rebalancingActivity" --type tsLength of output: 9586
packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarousel.tsx (1)
75-75: Replace inline styles with CSS classesInline styles are used at line 75, which may not be consistent with the project's styling conventions using CSS modules. It is better to move these styles into the CSS module for consistency and maintainability.
Apply this diff to move the styles into the CSS module:
- <div style={{ display: 'flex', flexDirection: 'column' }}> + <div className={classNames.emblaContentWrapper}>And in your
SlideCarousel.module.scss, add:.emblaContentWrapper { display: flex; flex-direction: column; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/app-icons/src/icons/social_link.svgis excluded by!**/*.svgpackages/app-icons/src/icons/social_x.svgis excluded by!**/*.svg
📒 Files selected for processing (25)
- apps/earn-protocol/app/earn/[network]/position/[vaultId]/page.tsx (2 hunks)
- apps/earn-protocol/app/earn/rebalance-activity/page.tsx (1 hunks)
- apps/earn-protocol/components/organisms/RebalancingActivity/RebalancingActivity.tsx (2 hunks)
- apps/earn-protocol/features/rebalance-activity/components/RebalanceActivityView/RebalanceActivityView.module.scss (1 hunks)
- apps/earn-protocol/features/rebalance-activity/components/RebalanceActivityView/RebalanceActivityView.tsx (1 hunks)
- apps/earn-protocol/features/rebalance-activity/table/columns.ts (1 hunks)
- apps/earn-protocol/features/rebalance-activity/table/dummyData.ts (1 hunks)
- apps/earn-protocol/features/rebalance-activity/table/mapper.tsx (2 hunks)
- apps/earn-protocol/features/rebalance-activity/table/types.ts (1 hunks)
- packages/app-earn-ui/src/components/molecules/HeadingWithCards/HeadingWithCards.module.scss (1 hunks)
- packages/app-earn-ui/src/components/molecules/HeadingWithCards/HeadingWithCards.module.scss.d.ts (1 hunks)
- packages/app-earn-ui/src/components/molecules/HeadingWithCards/HeadingWithCards.tsx (1 hunks)
- packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarousel.module.scss (3 hunks)
- packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarousel.module.scss.d.ts (1 hunks)
- packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarousel.tsx (1 hunks)
- packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarouselButtons.tsx (2 hunks)
- packages/app-earn-ui/src/components/molecules/TableCarousel/TableCarousel.module.scss (1 hunks)
- packages/app-earn-ui/src/components/molecules/TableCarousel/TableCarousel.module.scss.d.ts (1 hunks)
- packages/app-earn-ui/src/components/molecules/TableCarousel/TableCarousel.tsx (1 hunks)
- packages/app-earn-ui/src/components/organisms/Table/Table.module.scss (1 hunks)
- packages/app-earn-ui/src/components/organisms/Table/Table.module.scss.d.ts (1 hunks)
- packages/app-earn-ui/src/components/organisms/Table/Table.tsx (3 hunks)
- packages/app-earn-ui/src/index.ts (2 hunks)
- packages/app-icons/src/index.ts (1 hunks)
- packages/app-types/types/src/icons/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (3)
- apps/earn-protocol/features/rebalance-activity/components/RebalanceActivityView/RebalanceActivityView.module.scss
- packages/app-earn-ui/src/components/molecules/HeadingWithCards/HeadingWithCards.module.scss
- packages/app-earn-ui/src/components/molecules/TableCarousel/TableCarousel.module.scss
🔇 Additional comments (29)
packages/app-earn-ui/src/components/molecules/TableCarousel/TableCarousel.module.scss.d.ts (1)
1-9: Yo dawg, this TypeScript declaration is looking clean! 🚀Nice and straightforward type definitions for the CSS module. Everything's properly typed and follows best practices. Keep that good stuff coming!
packages/app-earn-ui/src/components/organisms/Table/Table.module.scss.d.ts (1)
2-2: Yo dawg, this change is totally rad! 🏄♂️The new
customRowtype declaration is clean and properly integrated with the existing type structure. Keep riding that TypeScript wave!packages/app-earn-ui/src/components/molecules/HeadingWithCards/HeadingWithCards.module.scss.d.ts (1)
1-10: Yo, these types are looking clean! 🎯Nice job on the type definitions, my friend. Everything's properly typed and follows the CSS modules pattern like a boss.
apps/earn-protocol/features/rebalance-activity/table/types.ts (1)
1-1: Clean import statement, my dude! 🤙Nice job using the type-only import. Keeping it tight and efficient!
apps/earn-protocol/app/earn/rebalance-activity/page.tsx (2)
1-2: Yo dawg, those imports be looking clean! 🎵Nice job keeping the imports organized and explicit.
1-8: Verify the component's integration with the routerLet's check if this page component is properly integrated with Next.js routing structure.
✅ Verification successful
Yo, this page is living its best life in the Next.js routing system 🏄♂️
The page is properly placed in the Next.js app directory structure at
apps/earn-protocol/app/earn/rebalance-activity/page.tsx, and it's actually being referenced in the PortfolioPageView as a tab with an ID and label. All the required components and data files exist in the features directory. Pretty chill setup, my dude!
- Component exists at:
apps/earn-protocol/features/rebalance-activity/components/RebalanceActivityView/RebalanceActivityView.tsx- Referenced in:
apps/earn-protocol/components/layout/PortfolioPageView/PortfolioPageView.tsx- Has proper dummy data at:
apps/earn-protocol/features/rebalance-activity/table/dummyData.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the page component's location and routing setup # Check if the file is in the correct location according to Next.js conventions fd --type f "page.tsx" apps/earn-protocol/app # Check for any route configurations or navigation to this page rg -l "rebalance-activity" --type ts --type tsxLength of output: 628
Script:
#!/bin/bash # Let's fix the file type issue and search more thoroughly rg -l "rebalance-activity" --type typescript --type tsx # Check for any navigation links or routes referencing this page rg -A 3 "rebalance-activity" --type typescript --type tsx # Check for any layout files that might wrap this page fd --type f "layout.tsx" apps/earn-protocol/app/earn/rebalance-activity # Check the parent layout fd --type f "layout.tsx" apps/earn-protocol/app/earnLength of output: 308
Script:
#!/bin/bash # Let's try with just plain file search, no type specifications rg -l "rebalance-activity" # Look for any navigation components or links rg "href.*rebalance-activity|route.*rebalance-activity" # Check if the component and its dependencies exist fd "RebalanceActivityView.tsx" fd "rebalancingActivityRawData"Length of output: 772
Script:
#!/bin/bash # Let's check how this page is being referenced in those files rg -A 3 "rebalance-activity" apps/earn-protocol/components/layout/PortfolioPageView/PortfolioPageView.tsx rg -A 3 "rebalance-activity" apps/earn-protocol/app/earn/[network]/position/[vaultId]/page.tsx # And check the layout structure cat apps/earn-protocol/app/earn/layout.tsxLength of output: 624
packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarousel.module.scss.d.ts (1)
2-15: Yo dawg, these type definitions are looking fresh! 🎨Nice work on keeping the style declarations clean and organized. The new types for dots navigation and slide count variations are totally rad. Everything's properly typed and follows the TypeScript way of life. Keep that vibe going!
apps/earn-protocol/features/rebalance-activity/table/columns.ts (1)
17-21: Yo, this Strategy column addition looks sweet! 🎸Nice and clean implementation, keeping it consistent with the other columns. Everything's in perfect harmony here, my friend.
apps/earn-protocol/features/rebalance-activity/table/dummyData.ts (2)
1-1: Yo, clean import statement!Nice and straightforward type import. We're off to a good start! 🚀
3-55: Yo, about these token pairs...The data structure looks solid, but there's no validation for token pairs. What's stopping someone from adding nonsensical pairs like DOGE-SHIB? (Actually, don't answer that 😅)
Let's check what token pairs are actually supported in the codebase:
Consider adding:
- A type or constant for supported token pairs
- Runtime validation for the dummy data
- Documentation about valid token combinations
packages/app-earn-ui/src/components/organisms/Table/Table.module.scss (2)
64-70: Yo dawg, these styles are pretty chill! 🏄♂️The
.customRowimplementation looks clean and straightforward. Nice job keeping it simple!
64-70: 🛠️ Refactor suggestionHold up! What about mobile views? 🤔
Bruh, I noticed you didn't add any mobile-specific styles for
.customRow. While it might work fine now, it could get funky on smaller screens since the table goes through some radical transformations in the mobile view (check out those styles after line 73).Consider adding mobile-specific handling:
.customRow { height: fit-content; > td { padding: unset; } } + +@media (max-width: 768px) { + .table { + .customRow { + > td { + padding: 8px; /* Restore padding for mobile to match other rows */ + } + } + } +}packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarousel.module.scss (3)
1-7: Yo, these base classes are looking clean! 🎯Nice use of CSS custom properties to keep things DRY. That's how we roll!
44-44: Heads up on that alignment change! 👀Switching from
centertoflex-startmight mess with your visual feng shui. Mind double-checking how this looks across different slide counts?
62-70: Sweet button styling, my dude! 🏄♂️Clean and simple button controls with proper spacing. Totally rad!
packages/app-earn-ui/src/components/molecules/TableCarousel/TableCarousel.tsx (1)
1-11: Yo, these imports are looking clean! 🎯Nice job keeping the imports organized and using absolute paths. That's how we roll!
packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarouselButtons.tsx (4)
12-12: Yo, tracking snap position! Nice addition!Adding
currentSnapstate to track carousel position is pretty rad. Clean implementation, my dude! 🏄♂️Also applies to: 20-20
25-25: Keeping that snap state fresh! ✨Updating
currentSnapafter scroll actions - that's some good state management right there!Also applies to: 31-31
61-61: Clean prop handling, my dude! 🎯Nice destructuring and prop passing. The
iconVariantis properly utilized in the Icon component.Also applies to: 68-68
55-58: 🛠️ Refactor suggestionHold up... what's with this type definition? 🤔
My brother in Christ, you're defining
iconVariantas'xs' | 'xxs'but what if we need other sizes in the future? This seems a bit restrictive.Let's see if these are the only sizes used across the codebase:
Consider using a union type with all available icon sizes or creating a dedicated type:
type IconSize = 'xs' | 'xxs' | /* other sizes */; type PropType = ComponentPropsWithRef<'button'> & { direction: 'left' | 'right' iconVariant: IconSize }apps/earn-protocol/features/rebalance-activity/table/mapper.tsx (2)
5-5: Yo, this import change is totally rad! 🏄♂️Moving types closer to their feature is a solid organizational move. Keep that codebase clean, my friend!
40-40: Let's make sure this strategy thing isn't just a fever dream 🕵️♂️We should verify that this field is actually defined in the type and consistently provided in the data.
✅ Verification successful
Yo dawg, your strategy field is totally legit! 🎯
The field is properly defined in the type definition and consistently used across the codebase:
- Defined as
strategy: stringinRebalancingActivityRawDatainterface- Used in the mapper component exactly as intended
- Present in dummy data with actual values
- Listed in the columns configuration
Everything's as chill as a penguin's toenails here. Keep rockin' that strategy field! 🤘
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if strategy is defined in the type definition echo "Checking type definition..." rg -A 5 "type RebalancingActivityRawData" # Check for actual usage of strategy field in the codebase echo "Checking strategy field usage..." rg "strategy.*:" --type typescriptLength of output: 2382
Script:
#!/bin/bash # Let's find that elusive type definition, my friend echo "Looking for the types file..." fd "types.ts" apps/earn-protocol/features/rebalance-activity/table/ echo "\nChecking the contents of the types file..." cat $(fd "types.ts" apps/earn-protocol/features/rebalance-activity/table/) echo "\nLet's see where this strategy field is actually used..." rg "strategy" apps/earn-protocol/features/rebalance-activity/Length of output: 1572
apps/earn-protocol/components/organisms/RebalancingActivity/RebalancingActivity.tsx (1)
5-7: Yo, these import changes are pretty sweet! 🎸Nice job organizing the imports into a feature-based structure. That's some clean architecture right there!
apps/earn-protocol/features/rebalance-activity/components/RebalanceActivityView/RebalanceActivityView.tsx (2)
1-10: Yo, these imports are looking clean! 🧹Nice organization of dependencies, keeping related imports grouped together. The 'use client' directive is properly placed at the top.
46-48: Nice memoization, bro! 💪Good use of useMemo to prevent unnecessary recalculations of the mapped data.
packages/app-earn-ui/src/index.ts (2)
61-62: Sweet new components, bro! 🎉Welcome to the family,
HeadingWithCardsandTableCarousel! Looking fresh in the molecules section.Just making sure these newbies have their docs in order:
49-52: Yo, heads up on that breaking change! 🚨The export syntax change for SlideCarousel could break stuff for folks using the old import. Better warn the homies about this in the PR description.
Let's see who might get burned by this:
✅ Verification successful
The results will help us understand if this is actually a breaking change by comparing the old and new export syntax, and seeing how it's currently being used in the portfolio component.
This will help us understand if SlideCarouselButtonPosition was already available but just not re-exported, or if it's completely new.
Yo, false alarm! We're all good here! 🎉
The SlideCarouselButtonPosition enum was already exported from the component file, and the change in index.ts is just making it available through the barrel file. The only consumer we found (PortfolioVaultsCarousel) is using the simple import
SlideCarousel, so this change is totally backward compatible and just adds more goodies for anyone who needs the enum.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all imports of SlideCarousel to check impact rg "import.*SlideCarousel" --type tsLength of output: 506
Script:
#!/bin/bash # Let's check the history of this file to see the old export syntax git log -p packages/app-earn-ui/src/index.ts | grep -A 5 "SlideCarousel" # Also check how it's being used in the portfolio component cat apps/earn-protocol/features/portfolio/components/PortfolioVaultsCarousel/PortfolioVaultsCarousel.tsxLength of output: 1159
Script:
#!/bin/bash # Let's check the actual SlideCarousel component to see if SlideCarouselButtonPosition was exported before cat packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarousel.tsxLength of output: 3862
packages/app-earn-ui/src/components/organisms/Table/Table.tsx (2)
25-30: Yo, these interface changes are clean! 🎸Nice job keeping it type-safe with the generic K. The optional props make it backward compatible too.
154-158: Sweet custom row implementation! 🏄♂️Clean and simple with proper colspan handling. Totally tubular!
packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarousel.module.scss
Show resolved
Hide resolved
packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarousel.module.scss
Show resolved
Hide resolved
apps/earn-protocol/app/earn/[network]/position/[vaultId]/page.tsx
Outdated
Show resolved
Hide resolved
packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarousel.tsx
Show resolved
Hide resolved
packages/app-earn-ui/src/components/molecules/SlideCarousel/SlideCarousel.tsx
Show resolved
Hide resolved
Simulation element on open vault page
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (10)
apps/earn-protocol/app/earn/[network]/position/[vaultId]/page.tsx (1)
28-34: Bruh, that error message is about as friendly as a hangry cat 😾Let's make it more user-friendly and add some guidance.
- <Text> - No vault found with the id {params.vaultId} on the network {params.network} - </Text> + <Text> + Oops! We couldn't find this vault. Double-check the vault ID or network, + or head back to the <a href="/earn">vault list</a>. + </Text>packages/app-earn-ui/src/components/atoms/Expander/Expander.tsx (1)
Line range hint
28-32: Bruh, hardcoding colors? That's so 2010! 🎨Let's not be that developer who sprinkles magic color values throughout the codebase like confetti.
Consider moving this to your design system or theme:
-color="rgba(119, 117, 118, 1)" +color="neutral.600" // or whatever your design system usesapps/earn-protocol/features/portfolio/components/PortfolioAssetsList/PortfolioAssetsList.module.scss (1)
14-14: Bruh... what's with that lazy comment? 🙄That "to be adjusted" comment is like leaving your laundry half-done. Either adjust it now or provide context about when/why it needs adjustment.
Consider either:
- Removing the comment if the value is final
- Adding a proper TODO with context about what conditions require adjustment
packages/app-earn-ui/src/components/atoms/AnimateHeight/AnimateHeight.tsx (1)
36-41: Performance has entered the chat! 🏎️Those scale animations could use some optimization love. Let's give the browser a heads up about what's coming.
Add this to your CSS module:
.animateScaleOn, .animateScaleOff { will-change: transform; transform: translateZ(0); }apps/earn-protocol/components/layout/VaultOpenView/VaultSimulationGraph.tsx (1)
11-17: Bruh, what's with that "ish" type? 🤨The props interface is solid, but that
SDKVaultishTypename is giving me anxiety. Also, consider extracting the props interface for better reusability.Here's a cleaner way to do it:
+interface VaultSimulationGraphProps { + amount?: BigNumber + vault: SDKVaultishType // TODO: Consider renaming this type to something more precise +} + -export const VaultSimulationGraph = ({ - amount, - vault, -}: { - amount?: BigNumber - vault: SDKVaultishType -}) => { +export const VaultSimulationGraph = ({ amount, vault }: VaultSimulationGraphProps) => {apps/earn-protocol/components/organisms/Form/Form.tsx (1)
Line range hint
43-45: Bruh, why are we hoarding unused state setters?You've got this
_unusedobject just chilling here withsetAction. If you're not using it, yeet it out! This is like keeping empty pizza boxes because they "might be useful someday" 🍕Here's how to fix this mess:
const [action, setAction] = useState(Action.DEPOSIT) const [amountValue, setAmountValue] = useState<string>() - const _unused = { - setAction, - }apps/earn-protocol/features/portfolio/components/PortfolioAssetsList/PortfolioAssetsList.tsx (1)
Line range hint
114-122: Bruh, did you just put an onClick on a div? That's so 2010! 🙄Let's make this more semantic and accessible. Your keyboard users are crying in the corner!
Transform this into a proper button:
-<div onClick={() => setIsSeeAll((prev) => !prev)} className={classNames.linkWrapper}> +<button + type="button" + onClick={() => setIsSeeAll((prev) => !prev)} + className={classNames.linkWrapper} + aria-expanded={isSeeAll} +> <Text as="p" variant="p3semi"> {isSeeAll ? 'Hide' : 'See'} all assets </Text> <Icon iconName={isSeeAll ? 'chevron_up' : 'chevron_down'} variant="xs" color="rgba(255, 73, 164, 1)" /> -</div> +</button>apps/earn-protocol/components/organisms/Charts/MockedLineChart.tsx (1)
Line range hint
29-91: Bruh, this mocked data is giving me anxiety! 🤯Look, I get it, we all need test data, but hardcoding dates like 'Jun 2024' through 'Dec 2024' is gonna make this component age like milk. Also, those magic numbers in your random ranges? Not cool, man.
Let's make this more maintainable:
+const MONTH_COUNT = 7; +const BASE_DATE = new Date(); + +const getMonthName = (date: Date) => + date.toLocaleString('default', { month: 'short' }) + ' ' + + date.getFullYear(); + +const generateTimePoints = () => + Array.from({ length: MONTH_COUNT }, (_, i) => { + const date = new Date(BASE_DATE); + date.setMonth(date.getMonth() + i); + return getMonthName(date); + }); const data = memoize( (_timeframe) => { + const timePoints = generateTimePoints(); + const YIELD_RANGES = { + base: { min: 0, max: 8 }, + strategy: { min: 6, max: 15 } + }; + return timePoints.map(monthName => ({ - name: 'Jun 2024', // hardcoded dates + name: monthName, 'Median Defi Yield': random(YIELD_RANGES.base.min, YIELD_RANGES.base.max, true), // ... rest of the yields with appropriate ranges 'Summer USDS Strategy': random(YIELD_RANGES.strategy.min, YIELD_RANGES.strategy.max, true), })) }, (timeframe) => timeframe, )packages/app-earn-ui/src/components/layout/VaultGridPreview/VaultGridPreview.tsx (1)
Bruh, these hardcoded values are like leaving pizza in the oven while you "BRB" 🤦♂️
The codebase shows we have actual yield calculations and APY data being fetched from DeFi Llama (
yields.llama.fi), but instead we're using magic numbers like they're going out of style.Either:
- Use the DeFi Llama integration that's already in the codebase (
getCachableYieldService) to get real median yields- Move these placeholder values to a config file (there's already config with APY values like
allTimeMedianApy: '0.0643')- Remove the fake subvalues entirely until the real data integration is ready
Hardcoding yield values in a DeFi app is like putting a "trust me bro" sticker on your financial calculator. 😤
🔗 Analysis chain
Line range hint
108-141: Bruh, what's with these hardcoded values? 🤨subValue="+2.1% Median DeFi Yield" // and subValue="+1.7% Median DeFi Yield" // and subValue={`231,232,321.01 ${vault.inputToken.symbol}`}You're just gonna leave these hardcoded values here with a TODO comment? That's like leaving your dirty socks on the floor and putting a "will clean later" sticky note on them.
Let's see if these values are defined somewhere else:
Either:
- Move these to constants/config
- Calculate them dynamically
- Remove them if they're just placeholders
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any potential constant definitions or calculations for these values rg -i "median defi yield|231,232,321.01" --type typescriptLength of output: 95
Script:
#!/bin/bash # Let's try a broader search without file type restriction rg -i "median defi yield|231,232,321.01" # Also search for any constant definitions or config files that might contain these values fd -e ts -e tsx -e json "config|constants" --exec grep -i "median\|231,232" # Let's also check for any APY/yield related calculations rg -i "median.*yield|defi.*yield"Length of output: 155795
Script:
#!/bin/bash # Let's check if these values are being calculated somewhere in the codebase rg -i "calculateMedianYield|getMedianYield|median.*yield" --type ts --type tsx # Let's also check for any yield-related calculations or services fd -e ts -e tsx "yield|apy" --exec grep -i "calculate\|get\|fetch" # And check for any config files that might define these values fd -e ts -e tsx -e json "config" --exec grep -i "yield\|apy"Length of output: 3179
apps/earn-protocol/components/layout/VaultOpenView/VaultOpenViewComponent.tsx (1)
89-89: Replace hardcoded balance with actual user balanceNot everyone has a balance of
123123. Unless you're aiming for a very generous demo, fetching the real user balance might be more practical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
- apps/earn-protocol/app/earn/[network]/position/[vaultId]/page.tsx (2 hunks)
- apps/earn-protocol/components/layout/VaultOpenView/VaultOpenHeaderBlock.tsx (1 hunks)
- apps/earn-protocol/components/layout/VaultOpenView/VaultOpenView.module.scss (1 hunks)
- apps/earn-protocol/components/layout/VaultOpenView/VaultOpenView.tsx (1 hunks)
- apps/earn-protocol/components/layout/VaultOpenView/VaultOpenViewComponent.tsx (1 hunks)
- apps/earn-protocol/components/layout/VaultOpenView/VaultSimulationGraph.tsx (1 hunks)
- apps/earn-protocol/components/layout/VaultOpenView/mocks.ts (1 hunks)
- apps/earn-protocol/components/organisms/Charts/MockedLineChart.tsx (3 hunks)
- apps/earn-protocol/components/organisms/Form/Form.tsx (4 hunks)
- apps/earn-protocol/features/portfolio/components/PortfolioAssetsList/PortfolioAssetsList.module.scss (2 hunks)
- apps/earn-protocol/features/portfolio/components/PortfolioAssetsList/PortfolioAssetsList.tsx (2 hunks)
- apps/earn-protocol/features/rebalance-activity/table/types.ts (1 hunks)
- packages/app-earn-ui/package.json (1 hunks)
- packages/app-earn-ui/src/components/atoms/AnimateHeight/AnimateHeight.module.scss (1 hunks)
- packages/app-earn-ui/src/components/atoms/AnimateHeight/AnimateHeight.module.scss.d.ts (1 hunks)
- packages/app-earn-ui/src/components/atoms/AnimateHeight/AnimateHeight.tsx (1 hunks)
- packages/app-earn-ui/src/components/atoms/Expander/Expander.tsx (2 hunks)
- packages/app-earn-ui/src/components/layout/VaultGridPreview/VaultGridPreview.tsx (4 hunks)
- packages/app-earn-ui/src/index.ts (3 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/earn-protocol/components/layout/VaultOpenView/VaultOpenView.module.scss
- apps/earn-protocol/components/layout/VaultOpenView/VaultOpenView.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/earn-protocol/features/rebalance-activity/table/types.ts
- packages/app-earn-ui/src/index.ts
🔇 Additional comments (25)
packages/app-earn-ui/src/components/atoms/AnimateHeight/AnimateHeight.module.scss.d.ts (1)
1-11: Yo, these types are looking clean! 🎨Nice work on the TypeScript declarations for the CSS modules. The types are well-structured and provide good type safety for the animation classes. Keep that vibe going!
packages/app-earn-ui/src/components/atoms/AnimateHeight/AnimateHeight.module.scss (2)
11-14: Smooth moves, my friend! 🎵Nice implementation of the scale-in animation. The timing and easing look totally rad!
16-19: Keeping it consistent, I dig it! 🤙The reverse animation is perfectly balanced, as all things should be.
apps/earn-protocol/app/earn/[network]/position/[vaultId]/page.tsx (4)
1-6: Yo, these imports are clean as a whistle! 🎵Nice job keeping it minimal and focused. The separation between UI components and data fetching is on point!
20-26: Concurrent fetching FTW! 🏃♂️💨Love that Promise.all action! Fetching vault details and list in parallel is chef's kiss.
Line range hint
8-15: Yo, what's up with that 60-second revalidation? 🤔The revalidation time of 60 seconds seems a bit arbitrary. Let's make sure this aligns with:
- The rate of vault data updates
- User expectations for data freshness
- Backend API rate limits
✅ Verification successful
Yo dawg, that 60-second revalidation is actually pretty standard around here! 🎯
Looking at the codebase, the 60-second revalidation time is consistently used across multiple pages in the earn-protocol app:
- Landing page
- Main earn page
- Network vaults page
- Vault details page
- Position management pages
It's all good in the hood - this consistency shows it's a deliberate choice for the app's data freshness strategy. The API routes that need real-time data are set to 0 revalidation, while content pages maintain this 60-second cache which is a solid balance between data freshness and server load.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other revalidation times in vault-related pages for consistency rg "export const revalidate = \d+" --type tsLength of output: 10758
Script:
#!/bin/bash # Let's try a different approach to find revalidation times rg -g "*.tsx" -g "*.ts" "revalidate.*=.*\d+" -A 2 -B 2Length of output: 4901
Script:
#!/bin/bash # Also search for any vault update intervals or refresh rates rg -g "*.tsx" -g "*.ts" "(interval|refresh|update).*=.*\d+" -A 2 -B 2Length of output: 1014
Script:
#!/bin/bash # Look for any polling or refetch configurations rg -g "*.tsx" -g "*.ts" "(polling|refetch).*\d+.*second" -A 2 -B 2Length of output: 68
36-39: Hold up, did we just yeet all that functionality into VaultOpenView? 🤔The code's looking clean, but let's make sure VaultOpenView hasn't become a kitchen sink component.
✅ Verification successful
Nah fam, we're good - this refactor is actually pretty clean! 🎯
The VaultOpenViewComponent is well-organized and properly modularized:
- It's got a clean separation of concerns with distinct components for header, simulation, and activities
- Uses shared UI components from
@summerfi/app-earn-ui- Handles deposit/withdraw actions with proper input validation
- Includes all the key features: historical yield, vault exposure, rebalancing activity, and user activity
The code that was "yeeted" wasn't really yeeted - it was thoughtfully reorganized into a proper component structure. Props for keeping it tight! 🤙
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the size and complexity of VaultOpenView ast-grep --pattern 'export const VaultOpenView = $_' -A 50Length of output: 10758
Script:
#!/bin/bash # Let's check out the actual VaultOpenViewComponent implementation rg -A 10 "export (const|function) VaultOpenViewComponent" --type typescript # Also check for VaultOpenViewWrapper since it's dynamically imported rg -A 10 "export (const|function) VaultOpenViewWrapper" --type typescript # And let's see what got moved from the original page fd -e tsx -e ts "VaultOpen" --exec rg -l "VaultOpen"Length of output: 503
Script:
#!/bin/bash # Let's see what's inside these files, yo cat apps/earn-protocol/components/layout/VaultOpenView/VaultOpenViewComponent.tsx cat apps/earn-protocol/components/layout/VaultOpenView/VaultOpenHeaderBlock.tsx # And check for any imports to see what other components it's using rg "import.*from" apps/earn-protocol/components/layout/VaultOpenView/*.tsxLength of output: 10881
packages/app-earn-ui/src/components/atoms/Expander/Expander.tsx (2)
Line range hint
1-12: Yo, these imports are clean as a whistle! 🎵Nice job keeping it simple with just the essentials. The props interface is straight to the point.
Line range hint
14-39: Totally digging these changes, my dude! 🏄♂️The switch to AnimateHeight component is chef's kiss. Way cleaner than the old manual height calculation shenanigans. State management is keeping it real simple too.
apps/earn-protocol/features/portfolio/components/PortfolioAssetsList/PortfolioAssetsList.module.scss (2)
11-11: Yo dawg, those scrollbar styles are looking fresh! 🔥Nice touch with the custom scrollbar styling! The thin grey scrollbar keeps things clean while maintaining usability.
Also applies to: 14-16
61-64: Spacing police here! 👮♂️The selector spacing change from
> pto>pis purely cosmetic and doesn't affect functionality. Keeping it chill, no big deal.apps/earn-protocol/components/layout/VaultOpenView/VaultOpenHeaderBlock.tsx (1)
1-4: Yo, these imports are clean AF! 🎯Nice job keeping it organized and importing only what's needed.
apps/earn-protocol/components/layout/VaultOpenView/VaultSimulationGraph.tsx (1)
1-10: Yo, these imports are clean AF! 🎯Nice job keeping it tight with those imports. Extra props for that type-only import of BigNumber - that's some clean code right there!
apps/earn-protocol/components/layout/VaultOpenView/mocks.ts (2)
1-2: Cool beans! These imports are clean as a whistle.Using type imports keeps the runtime bundle lean. Nice touch! 🎸
74-95: Smooth sailing with these links! 🏄♂️Clean structure, consistent naming convention, and good organization. Keep that vibe going!
packages/app-earn-ui/package.json (2)
69-69: Hold up, we're adding another animation library? 🤔Let's check if this new
react-animate-heightpackage isn't going to make our bundle size explode.#!/bin/bash # Check bundle size impact echo "Checking bundle size of react-animate-height..." curl -s https://bundlephobia.com/api/size?package=react-animate-height@3.2.3 | jq '{ size: .size, gzip: .gzip, dependencyCount: .dependencyCount }' # Check for alternative animation solutions already in use echo -e "\nChecking for existing animation libraries..." rg -l "import.*['\"].*animate"
67-67: Yo, what's up with this serverless-shared dependency doing the boomerang thing? 🪃I see we're bringing back
@summerfi/serverless-sharedafter it was removed. Let's make sure this isn't going to bite us later.apps/earn-protocol/components/organisms/Form/Form.tsx (1)
13-13: Yo dawg, what's with that sketchy type name?
SDKVaultishType? Really? The '-ish' suffix makes it sound like we're not quite sure what this type is supposed to be. That's about as confidence-inspiring as a chocolate teapot. 🫖Let's see what we're dealing with here:
apps/earn-protocol/features/portfolio/components/PortfolioAssetsList/PortfolioAssetsList.tsx (1)
4-4: Yo, nice import addition!The AnimateHeight component is a solid choice for handling expandable content. Keeping it smooth! 🎵
apps/earn-protocol/components/organisms/Charts/MockedLineChart.tsx (2)
92-96: Props interface looking clean, my dude! 🏄♂️Nice job with the default value and optional prop typing. That's how we roll!
105-105: Sweet prop usage, bro!The cardVariant prop is being used correctly. Everything's flowing smooth here.
packages/app-earn-ui/src/components/layout/VaultGridPreview/VaultGridPreview.tsx (4)
3-3: Yo, these imports are looking clean! 🎉Nice job keeping the imports organized and bringing in that sweet AnimateHeight component.
Also applies to: 9-9
24-28: Props interface update looking fresh! 👌The transition from leftContent/rightContent to more semantic names (detailsContent/sidebarContent) makes total sense. Adding displayGraph and simulationGraph props gives better control over the visualization.
96-98: Graph animation implementation is smooth as butter! 🧈Nice use of AnimateHeight for that sweet, sweet animation effect. The conditional rendering based on displayGraphStaggered is chef's kiss.
143-143: Content rendering looking clean! ✨The switch to semantic block names (detailsContent/sidebarContent) makes the code way more readable than the generic left/right content.
Also applies to: 146-146
packages/app-earn-ui/src/components/atoms/AnimateHeight/AnimateHeight.module.scss
Show resolved
Hide resolved
...n-protocol/features/portfolio/components/PortfolioAssetsList/PortfolioAssetsList.module.scss
Show resolved
Hide resolved
packages/app-earn-ui/src/components/atoms/AnimateHeight/AnimateHeight.tsx
Show resolved
Hide resolved
packages/app-earn-ui/src/components/atoms/AnimateHeight/AnimateHeight.tsx
Show resolved
Hide resolved
apps/earn-protocol/features/portfolio/components/PortfolioAssetsList/PortfolioAssetsList.tsx
Show resolved
Hide resolved
packages/app-earn-ui/src/components/layout/VaultGridPreview/VaultGridPreview.tsx
Show resolved
Hide resolved
apps/earn-protocol/components/layout/VaultOpenView/VaultOpenViewComponent.tsx
Show resolved
Hide resolved
apps/earn-protocol/components/layout/VaultOpenView/VaultOpenViewComponent.tsx
Show resolved
Hide resolved
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
16 of N pull requests.
This pull request refactors the
EarnVaultOpenManagePageand introduces new components to streamline the code and improve maintainability. The most important changes include the removal of hardcoded data, the creation of reusable components, and the dynamic import of theVaultOpenViewcomponent.Refactoring and Code Simplification:
apps/earn-protocol/app/earn/[network]/position/[vaultId]/page.tsx: Removed hardcoded data and replaced theVaultGridPreviewcomponent with the newVaultOpenViewcomponent. (apps/earn-protocol/app/earn/[network]/position/[vaultId]/page.tsxL1-R6, apps/earn-protocol/app/earn/[network]/position/[vaultId]/page.tsxL37-R36)apps/earn-protocol/components/layout/VaultOpenView/VaultOpenView.tsx: Added dynamic import forVaultOpenViewComponentto improve performance.apps/earn-protocol/components/layout/VaultOpenView/VaultOpenViewComponent.tsx: CreatedVaultOpenViewComponentto encapsulate the vault details and form logic, replacing the hardcoded values with props.New Components:
apps/earn-protocol/components/layout/VaultOpenView/VaultOpenHeaderBlock.tsx: AddedVaultOpenHeaderBlockto handle the header section of the vault details.apps/earn-protocol/components/layout/VaultOpenView/VaultSimulationGraph.tsx: AddedVaultSimulationGraphto display the simulated earnings graph.Data Management:
apps/earn-protocol/components/layout/VaultOpenView/mocks.ts: Moved mock data for vault exposure, user activity, and details links to a separate file for better data management.