feat: earn redesign #8831
feat: earn redesign #8831
Conversation
WalkthroughAdds Earn v2 backend endpoints, new Earn UI (pages, tabs, tables, charts), many hooks and helpers, routing updates to use Earn tab routes, portfolio/event refresh events, charting utilities (LightweightChart), and various staking/manage-page components and types. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as App UI
participant Nav as Navigation
participant EarnTab as Earn Tab
participant API as Backend API
participant Store as Jotai Store
User->>UI: Open Earn / Tap protocol
UI->>Nav: switchTab(ETabRoutes.Earn, { screen: ETabEarnRoutes.EarnHome })
Nav->>EarnTab: mount EarnHome
EarnTab->>API: fetch banners/faqs/accounts (useBannerInfo, useFAQListInfo, useEarnAccounts)
API-->>Store: update banners/recommendedTokens
Store-->>EarnTab: notify changes
User->>EarnTab: Open Protocols
EarnTab->>API: getProtocolList
API-->>EarnTab: protocol list
EarnTab->>UI: render TableList
User->>UI: Select protocol row
UI->>Nav: switchTab(ETabRoutes.Earn, { screen: ETabEarnRoutes.EarnProtocolDetails, params })
Nav->>EarnTab: show EarnProtocolDetails
EarnTab->>API: getProtocolDetailsV2, getApyHistory
API-->>EarnTab: detail + apy history
EarnTab->>UI: render details + ApyChart
User->>UI: Stake / Withdraw
UI->>Nav: open ManagePosition (modal/route)
Nav->>API: getManagePage
UI->>API: submit tx
API-->>Store: emit RefreshEarnPortfolioItem / RefreshEarnPortfolio
Store-->>EarnTab: portfolio hooks refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus review on:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
…e and performance
…bContent for improved layout and currency formatting
… handling, and improved UI components
…for improved handling
… EarnProtocolDetailsPage
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/kit/src/views/Earn/components/Overview.tsx (1)
126-191: Guard the map calls and length access - this was flagged before.The past review flagged unguarded
.map()calls, but the issue remains. Ifdistributedorundistributedis null/undefined, the bare.map()throws. TheisEmptychecks only hide the section headers, not the maps themselves.Additionally, line 133 accesses
rebateData.distributed.lengthwithout optional chaining, which will also throw.Apply this diff:
- {rebateData?.distributed.map((item, index) => { + {(rebateData?.distributed ?? []).map((item, index) => { const needDivider = - index === rebateData.distributed.length - 1 && + index === (rebateData?.distributed ?? []).length - 1 && !isEmpty(rebateData?.undistributed); return itemRender({ key: index, needDivider, children: ( <> <XStack ai="center" gap="$2.5"> <Token size="sm" borderRadius="$2" tokenImageUri={item.token.logoURI} /> <EarnText size="$bodyMdMedium" color="$text" text={item.title} /> </XStack> <EarnActionIcon actionIcon={item.button} onHistory={handleHistoryClick} /> </> ), }); })} {isEmpty(rebateData?.undistributed) ? null : ( <SizableText mx="$5" size="$bodyMdMedium" color="$textSubdued"> {intl.formatMessage({ id: ETranslations.referral_undistributed })} </SizableText> )} - {rebateData?.undistributed.map((item, index) => { + {(rebateData?.undistributed ?? []).map((item, index) => {packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx (2)
321-334: Fix countdown visibility window.The alert only shows after the end time has passed. The comparison should be flipped so the alert displays while now is between startTime and endTime.
{detailInfo?.countDownAlert?.startTime && detailInfo?.countDownAlert?.endTime && now > detailInfo.countDownAlert.startTime && - detailInfo.countDownAlert.endTime < now ? ( + detailInfo.countDownAlert.endTime > now ? (
517-523: Remove incorrect await on void function.Line 519 awaits
onCreateAddress()which returns void, not a Promise. This causes a linter error.const handleCreateAddress = useCallback(async () => { if (onCreateAddress) { - await onCreateAddress(); + onCreateAddress(); } await refreshManageAccount(); await refreshManageData(); }, [onCreateAddress, refreshManageAccount, refreshManageData]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/kit/src/views/Earn/components/Overview.tsx(1 hunks)packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx(1 hunks)
🧰 Additional context used
🪛 ESLint
packages/kit/src/views/Earn/components/Overview.tsx
[error] 163-163: Replace ·id:·ETranslations.referral_undistributed with ⏎··················id:·ETranslations.referral_undistributed,⏎···············
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint (20.x)
- GitHub Check: unittest (20.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx (1)
755-777: No issues found. Hooks handle empty accountId gracefully.Both downstream hooks defend against empty accountId:
useManagePageincludes a guard (if (!accountId) { return undefined; }) that catches empty strings and prevents API callsuseProtocolDetailDatapasses accountId to services and returns undefined when data loads empty, following graceful degradationThe empty string fallback in the code under review flows safely through both hooks without causing silent failures.
| <XStack flexWrap="wrap" m="$-5" p="$2"> | ||
| {intro.items.map((cell) => ( | ||
| <GridItem | ||
| key={cell.title.text} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a more unique key.
Using cell.title.text as a key assumes titles are unique. If they're not, React will throw warnings or behave incorrectly.
Use an index or a unique identifier if available:
- {intro.items.map((cell) => (
+ {intro.items.map((cell, idx) => (
<GridItem
- key={cell.title.text}
+ key={`${cell.title.text}-${idx}`}🤖 Prompt for AI Agents
In packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx around line
147 the list item uses key={cell.title.text}, which can collide if titles
repeat; change the key to a truly unique value (preferably a stable id on the
cell object, e.g., cell.id or cell.key) and if no unique id exists fall back to
using the index from the map (e.g., (cell, i) =>
key={`${cell.title.text}-${i}`}) to ensure uniqueness and stable rendering.
| {risk.items?.map((item) => ( | ||
| <> | ||
| <XStack ai="center" gap="$3" key={item.title.text}> | ||
| <YStack flex={1} gap="$2"> | ||
| <XStack ai="center" gap="$2"> | ||
| <XStack | ||
| ai="center" | ||
| jc="center" | ||
| w="$6" | ||
| h="$6" | ||
| borderRadius="$1" | ||
| > | ||
| <EarnIcon | ||
| icon={item.icon} | ||
| size="$6" | ||
| color="$iconCaution" | ||
| /> | ||
| </XStack> | ||
| <EarnText text={item.title} size="$bodyMdMedium" /> | ||
| </XStack> | ||
| <EarnText | ||
| text={item.description} | ||
| size="$bodyMd" | ||
| color={item.description.color || '$textSubdued'} | ||
| /> | ||
| </YStack> | ||
| <EarnActionIcon | ||
| title={item.title.text} | ||
| actionIcon={item.actionButton} | ||
| /> | ||
| </XStack> | ||
|
|
||
| {item.list?.length ? ( | ||
| <YStack gap="$1"> | ||
| {item.list.map((i, indexOfList) => ( | ||
| <XStack key={indexOfList} gap="$1"> | ||
| <EarnIcon icon={i.icon} size="$4" color="$iconCaution" /> | ||
| <EarnText | ||
| text={i.title} | ||
| size="$bodySm" | ||
| color="$textCaution" | ||
| /> | ||
| </XStack> | ||
| ))} | ||
| </YStack> | ||
| ) : null} | ||
| </> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move key prop to Fragment.
When mapping returns a Fragment, the key must be on the Fragment itself, not on its children. This prevents React reconciliation issues.
Apply this fix:
{risk.items?.map((item) => (
- <>
+ <Fragment key={item.title.text}>
- <XStack ai="center" gap="$3" key={item.title.text}>
+ <XStack ai="center" gap="$3">
<YStack flex={1} gap="$2">
...
- </>
+ </Fragment>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx around lines
217 to 264, the map returns a Fragment but the key is currently set on the inner
XStack which can break React reconciliation; move the key to the Fragment (use a
keyed fragment or an explicit <Fragment key={...}>), remove the key from the
child XStack, and keep using a stable unique value (e.g., item.title.text) as
the key so each mapped Fragment has the key at the top level.
| const { accountId, networkId, indexedAccountId, symbol, provider, vault } = | ||
| resolvedParams; | ||
|
|
||
| const { | ||
| earnAccount, | ||
| detailInfo, | ||
| tokenInfo, | ||
| protocolInfo, | ||
| isLoading, | ||
| refreshData, | ||
| refreshAccount, | ||
| } = useProtocolDetailData({ | ||
| accountId, | ||
| networkId, | ||
| indexedAccountId: indexedAccount?.id, | ||
| symbol, | ||
| provider, | ||
| vault, | ||
| }); |
There was a problem hiding this comment.
Fix inconsistent indexedAccountId usage.
Line 788 destructures indexedAccountId from resolvedParams, but line 802 passes indexedAccount?.id from the activeAccount hook instead. This creates inconsistency—share-link navigation might intend a different account context than what's actually used.
Use the resolved param consistently:
- const { accountId, networkId, indexedAccountId, symbol, provider, vault } =
+ const { accountId, networkId, indexedAccountId: resolvedIndexedAccountId, symbol, provider, vault } =
resolvedParams;
const {
earnAccount,
detailInfo,
tokenInfo,
protocolInfo,
isLoading,
refreshData,
refreshAccount,
} = useProtocolDetailData({
accountId,
networkId,
- indexedAccountId: indexedAccount?.id,
+ indexedAccountId: resolvedIndexedAccountId,
symbol,
provider,
vault,
});📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { accountId, networkId, indexedAccountId, symbol, provider, vault } = | |
| resolvedParams; | |
| const { | |
| earnAccount, | |
| detailInfo, | |
| tokenInfo, | |
| protocolInfo, | |
| isLoading, | |
| refreshData, | |
| refreshAccount, | |
| } = useProtocolDetailData({ | |
| accountId, | |
| networkId, | |
| indexedAccountId: indexedAccount?.id, | |
| symbol, | |
| provider, | |
| vault, | |
| }); | |
| const { accountId, networkId, indexedAccountId: resolvedIndexedAccountId, symbol, provider, vault } = | |
| resolvedParams; | |
| const { | |
| earnAccount, | |
| detailInfo, | |
| tokenInfo, | |
| protocolInfo, | |
| isLoading, | |
| refreshData, | |
| refreshAccount, | |
| } = useProtocolDetailData({ | |
| accountId, | |
| networkId, | |
| indexedAccountId: resolvedIndexedAccountId, | |
| symbol, | |
| provider, | |
| vault, | |
| }); |
🤖 Prompt for AI Agents
In packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx around lines
788 to 806, the code destructures indexedAccountId from resolvedParams but then
calls useProtocolDetailData with indexedAccount?.id (from activeAccount)
creating an inconsistency; replace the indexedAccount?.id argument with the
already-destructured indexedAccountId so the resolved route param is used
consistently when calling useProtocolDetailData, and remove any fallback to
activeAccount here (or explicitly pass a fallback only if intended) to ensure
share-link navigation uses the resolved account context.
…ndering in components
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/kit/src/views/Earn/EarnHome.tsx (4)
74-108: Consider extracting hardcoded key construction.The template literal keys (
availableAssets-${EAvailableAssetsTypeEnum.X}) are repeated and fragile. If the enum changes, these keys break silently.Extract to a helper function or constants for maintainability.
Example:
const getAvailableAssetsKey = (type: EAvailableAssetsTypeEnum) => `availableAssets-${type}`; const keys = [ getAvailableAssetsKey(EAvailableAssetsTypeEnum.All), getAvailableAssetsKey(EAvailableAssetsTypeEnum.StableCoins), getAvailableAssetsKey(EAvailableAssetsTypeEnum.NativeTokens), ];
110-168: Add error handling for async operations.The
getEarnAccountcall (line 123) lacks try-catch. Malformed URLs could also break the string parsing (lines 114-120).Wrap in try-catch to prevent unhandled rejections and crashes.
const onBannerPress = useCallback( async ({ hrefType, href }: IDiscoveryBanner) => { + try { if (account || indexedAccount) { if (href.includes('/defi/staking')) { const [path, query] = href.split('?'); const paths = path.split('/'); const provider = paths.pop(); const symbol = paths.pop(); const params = new URLSearchParams(query); const networkId = params.get('networkId'); const vault = params.get('vault'); if (provider && symbol && networkId) { const earnAccount = await backgroundApiProxy.serviceStaking.getEarnAccount({ indexedAccountId: indexedAccount?.id, accountId: account?.id ?? '', networkId, }); // ... rest of logic } return; } if (hrefType === 'external') { openUrlExternal(href); } else { openUrlInApp(href); } } else { await accountSelectorActions.current.showAccountSelector({ navigation, activeWallet: undefined, num: 0, sceneName: EAccountSelectorSceneName.home, }); } + } catch (error) { + console.error('Failed to handle banner press:', error); + // Consider showing user-facing error message + } }, [account, accountSelectorActions, indexedAccount, navigation], );
201-208: Clarify the height calculation.Line 206 comment says "without arbitrary adjustments" but then subtracts 20. Why?
Add a comment explaining the -20 offset or remove it if unnecessary.
224-324: Platform rendering divergence is complex.Native (lines 224-284) and non-native (lines 286-324) paths have significantly different structures. The native version uses absolute positioning and nested YStacks, while non-native uses EarnPageContainer.
This works but makes maintenance harder. Consider extracting shared layout logic if possible.
♻️ Duplicate comments (1)
packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts (1)
410-414: Let zeroed-out positions drop from the portfolio.We add every key to
fetchedKeysbefore the fetch resolves, butfetchInvestmentDetailreturnsnullfor zero balances (line 295-298). Those keys stay infetchedKeys, so the final sweep (lines 471-475) keeps stale entries and users still see positions after cashing out. Only mark a key as fetched when we actually have data.Apply this diff:
- const fetchPromises = pairsWithType.map(({ params, isAirdrop }) => { - const key = createInvestmentKey(params); - fetchedKeys.add(key); - return fetchInvestmentDetail(params, isAirdrop, requestId); - }); + const fetchPromises = pairsWithType.map(({ params, isAirdrop }) => { + return fetchInvestmentDetail(params, isAirdrop, requestId); + });And add the key tracking inside
processResult:const processResult = ( result: Awaited<ReturnType<typeof fetchInvestmentDetail>>, ) => { if (!result || isRequestStale(requestId)) return; const { key, investment } = result; + fetchedKeys.add(key);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/kit/src/views/Discovery/pages/Browser/Browser.native.tsx(1 hunks)packages/kit/src/views/Earn/EarnHome.tsx(8 hunks)packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts(1 hunks)packages/kit/src/views/Home/components/WalletActions/WalletActionEarn.tsx(3 hunks)packages/kit/src/views/Market/components/tradeHook.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint (20.x)
- GitHub Check: unittest (20.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
packages/kit/src/views/Home/components/WalletActions/WalletActionEarn.tsx (3)
12-12: LGTM!The routing constants import aligns with the new tab-based navigation pattern.
116-123: Code is correct. Route exists and parameters match.The
EarnProtocolsroute is properly defined with the correct parameter types. Your code passessymbolandfilterNetworkId, which match the expected interface exactly. No changes needed.
100-114: Route verified—no issues found.The
ETabEarnRoutes.EarnProtocolDetailsroute is properly defined and configured to accept all passed parameters:networkId,accountId,indexedAccountId,symbol,provider, andvault. The route component handles these params viaITabEarnParamList. The navigation pattern here matches usage across the codebase.packages/kit/src/views/Market/components/tradeHook.tsx (2)
9-13: LGTM! Import changes align with navigation refactor.The imports correctly reflect the shift from modal-based to tab-based Earn navigation.
258-265: Param structure is correct—no issues found.The params passed (
symbolandfilterNetworkId) match the route definition exactly.filterNetworkIdis optional, so passingnetworkIdis fine. The tab navigation is a deliberate UX change, not a mistake.packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts (10)
27-34: LGTM!Key generation logic is clear and handles optional vault correctly.
35-53: LGTM!Utility functions are pure and handle BigNumber arithmetic correctly.
71-95: LGTM!Asset enrichment and investment merging logic are correct. BigNumber arithmetic ensures precision when summing fiat values.
97-114: LGTM!Protocol aggregation logic correctly merges investments by protocol code and sorts by value.
117-167: LGTM!Investment state management correctly filters, sorts, and tracks investments. Airdrops are kept even with zero value as intended.
169-210: LGTM!Account state tracking and request ID management correctly prevent race conditions when accounts change.
234-239: LGTM!Effect correctly clears stale portfolio data when account changes.
241-326: LGTM!Investment detail fetching correctly handles airdrops vs. normal investments, filters zero-balance positions, and guards against stale requests.
384-480: LGTM!RAF throttling for incremental updates and final cleanup logic are well-designed. The code correctly handles partial vs. full refresh scenarios.
495-524: LGTM!Hook integration and public API are clean. Auto-revalidation and manual refresh with filtering options provide good UX.
| <Stack | ||
| flex={1} | ||
| display={ | ||
| selectedHeaderTab === ETranslations.global_earn ? undefined : 'none' | ||
| } | ||
| > | ||
| <EarnHomeWithProvider | ||
| showHeader={false} | ||
| showContent={selectedHeaderTab === ETranslations.global_earn} | ||
| /> | ||
| </Stack> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify redundant conditional and consider performance optimization.
Two issues:
-
Redundant conditional logic: The Stack's
displayprop andshowContentprop both checkselectedHeaderTab === ETranslations.global_earn. When the Stack hasdisplay: 'none', theshowContentcheck is redundant unless it controls internal data fetching or computation withinEarnHomeWithProvider. -
Missing Freeze component: The browser content section (lines 342-379) uses
Freezecomponents to optimize performance when content isn't visible. Consider wrappingEarnHomeWithProviderwithFreezefor consistency and to prevent unnecessary work when the tab isn't selected.
Consider this pattern for consistency:
<Stack
flex={1}
display={
selectedHeaderTab === ETranslations.global_earn ? undefined : 'none'
}
>
+ <Freeze freeze={selectedHeaderTab !== ETranslations.global_earn}>
<EarnHomeWithProvider
showHeader={false}
- showContent={selectedHeaderTab === ETranslations.global_earn}
/>
+ </Freeze>
</Stack>Or if showContent serves a distinct purpose, document why both conditionals are needed.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Stack | |
| flex={1} | |
| display={ | |
| selectedHeaderTab === ETranslations.global_earn ? undefined : 'none' | |
| } | |
| > | |
| <EarnHomeWithProvider | |
| showHeader={false} | |
| showContent={selectedHeaderTab === ETranslations.global_earn} | |
| /> | |
| </Stack> | |
| <Stack | |
| flex={1} | |
| display={ | |
| selectedHeaderTab === ETranslations.global_earn ? undefined : 'none' | |
| } | |
| > | |
| <Freeze freeze={selectedHeaderTab !== ETranslations.global_earn}> | |
| <EarnHomeWithProvider | |
| showHeader={false} | |
| /> | |
| </Freeze> | |
| </Stack> |
| import { Image, SizableText, Stack, useMedia } from '@onekeyhq/components'; | ||
| import type { IDiscoveryBanner } from '@onekeyhq/shared/types/discovery'; | ||
|
|
||
| const BANNER_TITLE_OFFSET = { |
There was a problem hiding this comment.
这类没用到的变量都可以清理下，还有下面的 media
|
|
||
| return ( | ||
| <YStack> | ||
| {investments.map((item, index) => { |
| token, | ||
| ...rest | ||
| }: { token?: IRecommendAsset } & IYStackProps) { | ||
| const accountInfo = useActiveAccount({ num: 0 }); |
There was a problem hiding this comment.
事件和账户信息都从父组件传入，减少子组件的事件绑定数量，和 hooks 变动导致的重渲染
| import backgroundApiProxy from '@onekeyhq/kit/src/background/instance/backgroundApiProxy'; | ||
| import { usePromiseResult } from '@onekeyhq/kit/src/hooks/usePromiseResult'; | ||
|
|
||
| export const useEarnAccount = ({ |
originalix
left a comment
originalix
left a comment
There was a problem hiding this comment.
移动端进入 2 级页面，得像 Market 一样隐藏下 TabBar？跟 pm 确认下
ok tabbar 这期间应该改版过，我问下我这边咋适配 |
… for network ID retrieval
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
packages/kit/src/views/Earn/hooks/useEarnAccounts.ts (2)
31-34: Remove redundant account existence check.The inner guard duplicates the outer check at lines 22-24. Since
fetchAndUpdateOverviewis only called when an account exists, this check is unnecessary.Apply this diff:
const fetchAndUpdateOverview = async () => { - if (!isAccountExists && !indexedAccount?.id) { - return; - } - // Fetch account assets (contains accounts array)
61-67: Document the 350ms delay rationale.The 350ms wait when cached data exists lacks explanation. Clarify why this throttle is needed.
Add a comment or extract as a named constant:
+ // Throttle refetch when cached data exists to reduce server load const earnAccountData = actions.current.getEarnAccount(totalFiatMapKey); if (earnAccountData) { await timerUtils.wait(350);Or:
+ const CACHED_DATA_REFETCH_DELAY_MS = 350; + export const useEarnAccounts = () => { // ... if (earnAccountData) { - await timerUtils.wait(350); + await timerUtils.wait(CACHED_DATA_REFETCH_DELAY_MS);packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts (2)
53-67: Remove unused function.
filterByOptionsis never called. The filtering logic is duplicated inline at lines 367-377. Remove this dead code.Apply this diff:
-const filterByOptions = < - T extends { provider: string; networkId: string; symbol: string }, ->( - list: Array<T>, - options?: IRefreshOptions, -): T[] => { - if (!options) return list; - - return list.filter((item) => { - if (options.provider && item.provider !== options.provider) return false; - if (options.networkId && item.networkId !== options.networkId) return false; - if (options.symbol && item.symbol !== options.symbol) return false; - return true; - }); -}; -
408-412: Fix fetchedKeys tracking to let zeroed positions drop.Keys are added to
fetchedKeysbefore the fetch completes (line 410), butfetchInvestmentDetailreturnsnullfor zero balances. Those keys remain infetchedKeys, so the final sweep keeps stale entries—users still see value after cashing out. Only mark a key as fetched when valid data arrives.Move
fetchedKeys.add(key)intoprocessResultafter confirming valid data:const fetchPromises = pairsWithType.map(({ params, isAirdrop }) => { const key = createInvestmentKey(params); - fetchedKeys.add(key); return fetchInvestmentDetail(params, isAirdrop, requestId); }); // Process results incrementally const processResult = ( result: Awaited<ReturnType<typeof fetchInvestmentDetail>>, ) => { if (!result || isRequestStale(requestId)) return; const { key, investment } = result; + fetchedKeys.add(key);packages/kit/src/views/Earn/components/Recommended.tsx (2)
76-94: Guard against empty protocols array.Lines 82 and 88 access
token.protocols[0]?.networkIdwithout verifying the array has elements. Add a length check.Apply this diff:
const onPress = useCallback(async () => { if (token) { + if (!token.protocols || token.protocols.length === 0) { + return; + } const earnAccount = await backgroundApiProxy.serviceStaking.getEarnAccount({
101-122: Consider semantic button element for accessibility.Line 102 uses
role="button"on a YStack. While functional, a semantic Button component would provide better built-in accessibility.If the UI framework's Button supports the same styling, replace YStack with Button and remove the explicit
roleattribute.packages/kit/src/views/Earn/components/Overview.tsx (1)
130-133: Guard rebate lists before accessing properties.The
.map()and.lengthcalls will throw ifdistributedorundistributedare null/undefined. Use fallback arrays.Apply this diff:
- {rebateData?.distributed.map((item, index) => { + {(rebateData?.distributed ?? []).map((item, index) => { const needDivider = - index === rebateData.distributed.length - 1 && + index === (rebateData?.distributed ?? []).length - 1 && !isEmpty(rebateData?.undistributed); @@ - {rebateData?.undistributed.map((item, index) => { + {(rebateData?.undistributed ?? []).map((item, index) => {Also applies to: 167-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
packages/kit-bg/src/services/ServiceStaking.ts(9 hunks)packages/kit/src/states/jotai/contexts/earn/actions.ts(6 hunks)packages/kit/src/states/jotai/contexts/earn/atoms.ts(2 hunks)packages/kit/src/views/Earn/components/Overview.tsx(1 hunks)packages/kit/src/views/Earn/components/Recommended.tsx(1 hunks)packages/kit/src/views/Earn/hooks/useBannerInfo.ts(1 hunks)packages/kit/src/views/Earn/hooks/useEarnAccounts.ts(1 hunks)packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts(1 hunks)packages/shared/types/staking.ts(10 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
packages/kit/src/views/Earn/components/Recommended.tsx
[error] 102-102: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint (20.x)
- GitHub Check: unittest (20.x)
🔇 Additional comments (7)
packages/kit/src/states/jotai/contexts/earn/atoms.ts (2)
39-39: Good type safety improvement.Replacing
anywith a union type makes the setter more type-safe.
24-25: LGTM! Consistent initialization.The new fields follow the same pattern as existing fields with proper empty array fallbacks during initialization.
Also applies to: 46-47
packages/kit/src/states/jotai/contexts/earn/actions.ts (3)
48-48: Good cleanup.Removing the unnecessary wrapper function simplifies the code.
180-202: LGTM! Methods follow existing patterns.The new getter/setter methods for
recommendedTokensandbannersare consistent with existing methods and properly persist viasyncToDb.
234-237: Hook exposure is complete.All new methods are properly exposed in the
useEarnActionshook.Also applies to: 252-255
packages/kit/src/views/Earn/components/Overview.tsx (2)
258-265: Rebate fetch pattern looks good.The conditional fetch based on
evmAccountavailability is appropriate.
310-316: LGTM! Refresh logic coordinates multiple updates.The refresh properly clears cache, triggers state updates, and emits the portfolio refresh event.
| async fetchInvestmentDetailV2(params: { | ||
| publicKey?: string | undefined; | ||
| vault?: string | undefined; | ||
| accountAddress: string; | ||
| networkId: string; | ||
| provider: string; | ||
| symbol: string; | ||
| }) { | ||
| const client = await this.getClient(EServiceEndpointEnum.Earn); | ||
|
|
||
| const response = await client.get<{ data: IEarnInvestmentItemV2 }>( | ||
| `/earn/v2/investment/detail`, | ||
| { params }, | ||
| ); | ||
|
|
||
| return response.data.data; | ||
| } | ||
|
|
||
| @backgroundMethod() | ||
| async fetchAirdropInvestmentDetail(params: { | ||
| publicKey?: string | undefined; | ||
| vault?: string | undefined; | ||
| accountAddress: string; | ||
| networkId: string; | ||
| provider: string; | ||
| symbol: string; | ||
| }) { | ||
| const client = await this.getClient(EServiceEndpointEnum.Earn); | ||
|
|
||
| const response = await client.get<{ data: IEarnAirdropInvestmentItemV2 }>( | ||
| `/earn/v1/investment/airdrop-detail`, | ||
| { params }, | ||
| ); | ||
|
|
||
| return response.data.data; | ||
| } | ||
|
|
There was a problem hiding this comment.
Lowercase provider before calling the v2 detail APIs.
Both fetchInvestmentDetailV2 and fetchAirdropInvestmentDetail forward the provider string without normalization. A mixed-case provider (for example "Babylon") slips through here, the backend lookup fails, and the returned V2 payload is empty. Please mirror the normalization already used in getManagePage / getApyHistory and force the provider to lowercase before issuing these requests.
Apply this diff:
@backgroundMethod()
async fetchInvestmentDetailV2(params: {
publicKey?: string | undefined;
vault?: string | undefined;
accountAddress: string;
networkId: string;
provider: string;
symbol: string;
}) {
const client = await this.getClient(EServiceEndpointEnum.Earn);
- const response = await client.get<{ data: IEarnInvestmentItemV2 }>(
- `/earn/v2/investment/detail`,
- { params },
- );
+ const requestParams = {
+ ...params,
+ provider: params.provider.toLowerCase(),
+ };
+
+ const response = await client.get<{ data: IEarnInvestmentItemV2 }>(
+ `/earn/v2/investment/detail`,
+ { params: requestParams },
+ );
return response.data.data;
}
@backgroundMethod()
async fetchAirdropInvestmentDetail(params: {
publicKey?: string | undefined;
vault?: string | undefined;
accountAddress: string;
networkId: string;
provider: string;
symbol: string;
}) {
const client = await this.getClient(EServiceEndpointEnum.Earn);
- const response = await client.get<{ data: IEarnAirdropInvestmentItemV2 }>(
- `/earn/v1/investment/airdrop-detail`,
- { params },
- );
+ const response = await client.get<{ data: IEarnAirdropInvestmentItemV2 }>(
+ `/earn/v1/investment/airdrop-detail`,
+ {
+ params: {
+ ...params,
+ provider: params.provider.toLowerCase(),
+ },
+ },
+ );
return response.data.data;
}🤖 Prompt for AI Agents
In packages/kit-bg/src/services/ServiceStaking.ts around lines 1097 to 1133,
both fetchInvestmentDetailV2 and fetchAirdropInvestmentDetail pass the provider
string as-is; normalize it to lowercase before calling the backend to avoid
lookup failures for mixed-case provider names. Update each method to copy/modify
the params (or set params.provider) to params.provider =
params.provider?.toLowerCase() (or construct a new paramsLower = { ...params,
provider: params.provider?.toLowerCase() }) and then pass that normalized params
object to client.get.
| const refetchBanners = useCallback(async () => { | ||
| const bannerResult = | ||
| await backgroundApiProxy.serviceStaking.fetchEarnHomePageBannerList(); | ||
| const transformedBanners = | ||
| bannerResult?.map((i) => ({ | ||
| ...i, | ||
| imgUrl: i.src, | ||
| title: i.title || '', | ||
| titleTextProps: { | ||
| size: '$headingMd', | ||
| }, | ||
| })) || []; | ||
|
|
||
| actions.current.updateBanners(transformedBanners); | ||
| }, [actions]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider error handling for banner fetch.
Line 14 calls fetchEarnHomePageBannerList without a try-catch. If the fetch fails, bannerResult is undefined and transformedBanners becomes an empty array (line 23). This silent failure might confuse users during network issues.
Add error handling or logging:
const refetchBanners = useCallback(async () => {
+ try {
const bannerResult =
await backgroundApiProxy.serviceStaking.fetchEarnHomePageBannerList();
const transformedBanners =
bannerResult?.map((i) => ({
...i,
imgUrl: i.src,
title: i.title || '',
titleTextProps: {
size: '$headingMd',
},
})) || [];
actions.current.updateBanners(transformedBanners);
+ } catch (error) {
+ console.error('Failed to fetch banners:', error);
+ actions.current.updateBanners([]);
+ }
}, [actions]);📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const refetchBanners = useCallback(async () => { | |
| const bannerResult = | |
| await backgroundApiProxy.serviceStaking.fetchEarnHomePageBannerList(); | |
| const transformedBanners = | |
| bannerResult?.map((i) => ({ | |
| ...i, | |
| imgUrl: i.src, | |
| title: i.title || '', | |
| titleTextProps: { | |
| size: '$headingMd', | |
| }, | |
| })) || []; | |
| actions.current.updateBanners(transformedBanners); | |
| }, [actions]); | |
| const refetchBanners = useCallback(async () => { | |
| try { | |
| const bannerResult = | |
| await backgroundApiProxy.serviceStaking.fetchEarnHomePageBannerList(); | |
| const transformedBanners = | |
| bannerResult?.map((i) => ({ | |
| ...i, | |
| imgUrl: i.src, | |
| title: i.title || '', | |
| titleTextProps: { | |
| size: '$headingMd', | |
| }, | |
| })) || []; | |
| actions.current.updateBanners(transformedBanners); | |
| } catch (error) { | |
| console.error('Failed to fetch banners:', error); | |
| actions.current.updateBanners([]); | |
| } | |
| }, [actions]); |
🤖 Prompt for AI Agents
In packages/kit/src/views/Earn/hooks/useBannerInfo.ts around lines 12 to 26, the
call to fetchEarnHomePageBannerList lacks error handling so network or API
failures are swallowed and only an empty banner list is used; wrap the await
call in a try-catch, log or report the caught error (using backgroundApiProxy
logger or process logger) and optionally set a fallback state or keep previous
banners when the fetch fails, then only transform and update banners on
successful response inside the try block.
…including airdrops
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts (2)
53-67: Remove unused function (already flagged).This function is still dead code. The filtering logic is duplicated inline at lines 362-372.
406-410: Stale investments persist after cashing out.You add each key to
fetchedKeysbefore the fetch completes. WhenfetchInvestmentDetailreturnsnull(zero balance), the key stays infetchedKeysbut never entersbatchMap. At lines 467-471, you only delete keys NOT infetchedKeys, so the old investment remains infinalMap. Users still see zeroed positions.Move
fetchedKeys.add(key)intoprocessResultafter confirming the result is valid.Apply this diff:
const fetchPromises = pairsWithType.map(({ params, isAirdrop }) => { - const key = createInvestmentKey(params); - fetchedKeys.add(key); return fetchInvestmentDetail(params, isAirdrop, requestId); }); // Process results incrementally const processResult = ( result: Awaited<ReturnType<typeof fetchInvestmentDetail>>, ) => { if (!result || isRequestStale(requestId)) return; const { key, investment } = result; + fetchedKeys.add(key); // Merge with existing data for the same key
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unittest (20.x)
- GitHub Check: lint (20.x)
- GitHub Check: Analyze (javascript-typescript)
| const firstAsset = inv.assets[0] || inv.airdropAssets[0]; | ||
| return [ | ||
| createInvestmentKey({ | ||
| provider: inv.protocol.providerDetail.code, | ||
| symbol: firstAsset?.token.info.symbol || '', |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider defensive check for empty assets.
If both assets and airdropAssets are empty, firstAsset is undefined and the key gets an empty symbol. While optional chaining protects against crashes, an empty symbol could cause key collisions. Investments with positive fiat value should always have assets, but a defensive check or assertion would make this assumption explicit.
🤖 Prompt for AI Agents
In packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts around lines 136-140,
the code picks firstAsset from inv.assets or inv.airdropAssets and then uses
firstAsset?.token.info.symbol which can be empty and cause key collisions; add a
defensive check: if neither assets nor airdropAssets has an entry, either
assert/throw (for investments that should always have assets) or use a stable
fallback (e.g. inv.id or a combination of provider+inv.id) as the symbol when
building the key, and log a warning so collisions are avoided and the assumption
is explicit.
…erformanceSection to display performance details
…structures, improve layout responsiveness
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/kit/src/views/Market/components/Chart/ChartViewAdapter.tsx (1)
23-40: Fix the contradictory eslint-disable pattern.Your comment disables the rule but still provides a dependency array—these conflict. The current setup also creates a real problem: if the parent passes a different
heightor updates theonHovercallback, the chart won't react to those changes.Choose one approach:
- Complete the deps array: Remove the disable comment and add
onHoverandheightto dependencies so the chart recreates when they change.- Justify the exclusion: If intentional (e.g., lazy callback binding), remove the deps array entirely and replace the disable comment with a clear explanation of why these props are intentionally excluded.
Current code leaves it ambiguous whether this is deliberate or an oversight.
packages/kit/src/views/Staking/components/UniversalWithdraw/index.tsx (1)
235-276: Ensure transaction confirmation refetches when disabled state changes
fetchTransactionConfirmationnow short‑circuits whenisDisabledis true, which is good. But theuseEffectthat invokesdebouncedFetchTransactionConfirmationonly depends onamountValueand the debounced function. IfisDisabledflips fromtruetofalsewhile the amount stays the same, you never refetch a confirmation for the current amount.If
isDisabledcan change during the component’s lifetime (e.g., after some async gating check), consider adding it as a dependency so you refetch when enabling:- useEffect(() => { - void debouncedFetchTransactionConfirmation(amountValue); - }, [amountValue, debouncedFetchTransactionConfirmation]); + useEffect(() => { + void debouncedFetchTransactionConfirmation(amountValue); + }, [amountValue, isDisabled, debouncedFetchTransactionConfirmation]);This keeps
transactionConfirmationin sync with the actual enabled/disabled state.packages/kit/src/views/Staking/components/UniversalStake/index.tsx (2)
906-960: Add stable keys foraccordionContentitems
accordionContentreturns an array of React elements built viaitems.push(...), but none of the pushed elements have akeyprop. When more than one item renders, React will log “each child in a list should have a unique key”.You can fix this by adding explicit keys when pushing:
if (transactionConfirmation?.receive) { - items.push( - <CalculationListItem> + items.push( + <CalculationListItem key="receive"> ... </CalculationListItem>, ); } if (estimateFeeResp) { - items.push( - <EstimateNetworkFee + items.push( + <EstimateNetworkFee + key="estimate-fee" isVisible estimateFeeResp={estimateFeeResp} onPress={() => { ... }} />, ); } if ( providerName?.toLowerCase() === EEarnProviderEnum.Babylon.toLowerCase() && estimateFeeUTXO ) { - items.push( - <BtcFeeRateInput + items.push( + <BtcFeeRateInput + key="btc-fee-rate" estimateFeeUTXO={estimateFeeUTXO} onFeeRateChange={onFeeRateChange} />, ); }This keeps reconciliation predictable and removes noisy warnings.
544-575: FoldisDisabledintoisDisableand gate mobile controlsVerification confirms the gaps. The
isDisablememo (line 548) lacksisDisabledin both its logic and dependencies. This means the confirm button can remain enabled even when the product is disabled. On mobile,PercentageStageOnKeyboard(line 1313) has noisDisabledcheck, so users can still adjust amounts via the keyboard when the product is disabled.Apply both fixes:
- Add
isDisabledtoisDisableconditions and dependencies (lines 548–570)- Gate the mobile footer with
&& !isDisabled(line 1310)This prevents the staking flow from accepting user input when the product is disabled.
♻️ Duplicate comments (14)
packages/kit/src/views/Earn/components/PortfolioTabContent.tsx (1)
698-712: Move the key to the fragment wrapper.The
keyprop should be on the wrapping fragment, not onPortfolioItem. React needs the key on the direct child of the map. Replace the shorthand fragment withReact.Fragment.Apply this diff:
{investments.map((item, index) => { const showDivider = index < investments.length - 1; const key = `${item.protocol.providerDetail.code}_${ item.protocol.vaultName || '' }_${item.network.networkId}`; return ( - <> - <PortfolioItem key={key} portfolioItem={item} /> + <React.Fragment key={key}> + <PortfolioItem portfolioItem={item} /> {showDivider ? <Divider my="$4" /> : null} - </> + </React.Fragment> ); })}packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts (2)
118-131: Consider defensive check for empty assets in key generation.Line 120 assumes investments with positive fiat value always have at least one asset. While this should be true, an explicit check or assertion would make the invariant clear and prevent potential key collisions from empty symbols.
Example:
validInvestments.map((inv) => { const firstAsset = inv.assets[0] || inv.airdropAssets[0]; if (!firstAsset) { console.warn('Investment has positive fiat value but no assets', inv); return null; // or throw } return [ createInvestmentKey({ provider: inv.protocol.providerDetail.code, symbol: firstAsset.token.info.symbol, vault: inv.protocol.vault, networkId: inv.network.networkId, }), inv, ]; }).filter(Boolean)
390-394: Fix key tracking to allow zeroed positions to drop.Lines 390-394 add each key to
fetchedKeysbefore the fetch completes. IffetchInvestmentDetailreturnsnull(e.g., for zero balances), the key remains infetchedKeys, so the final sweep at lines 450-456 won't remove it. Users will see stale data after cashing out.Only add a key to
fetchedKeysafter a successful fetch returns data.Apply this diff:
// Create fetch promises const fetchPromises = pairsWithType.map(({ params, isAirdrop }) => { - const key = createInvestmentKey(params); - fetchedKeys.add(key); - return fetchInvestmentDetail(params, isAirdrop, requestId); + return fetchInvestmentDetail(params, isAirdrop, requestId); }); // Process results incrementally const processResult = ( result: Awaited<ReturnType<typeof fetchInvestmentDetail>>, ) => { if (!result || isRequestStale(requestId)) return; const { key, investment } = result; + fetchedKeys.add(key); // Merge with existing data for the same keypackages/kit/src/views/Earn/pages/EarnProtocolDetails/components/ApyChart.tsx (1)
78-102: Fix hover handling to support zero values.The truthy checks on
timeandpriceexclude legitimate zero values. Users lose hover feedback when APY is 0%. Check forundefinedinstead.const handleHover = useCallback( ({ time, price, x, y, }: { time?: number; price?: number; x?: number; y?: number; }) => { - if (time && price && x !== undefined && y !== undefined) { + if (time !== undefined && price !== undefined && x !== undefined && y !== undefined) { setHoverData({ time, apy: price, x, y, }); } else { setHoverData(null); } }, [], );packages/kit/src/views/Earn/pages/EarnProtocols/index.tsx (3)
81-108: Log fetch errors for debugging.The catch block silently swallows errors. Add logging so failures are recorded.
} catch (error) { + defaultLogger.staking.page.error('Failed to fetch protocol data:', error); setProtocolData([]); } finally {
114-157: Log navigation errors for debugging.The catch block silently swallows navigation errors. Add logging to help debug failed attempts.
}); } catch (error) { + defaultLogger.staking.page.error('Failed to navigate to protocol details:', error); // Handle error silently }
230-242: Enable APR sorting before shipping.The yield column lacks
sortablemetadata and a comparator. The default sort at line 298 won't work. Add the flag and comparator.{ key: 'yield', label: intl.formatMessage({ id: ETranslations.global_apr }), flex: 1, + sortable: true, + comparator: (a, b) => { + const aprA = parseFloat( + a?.provider?.aprWithoutFee ?? + a?.aprInfo?.value ?? + a?.aprInfo?.apr ?? + '0', + ); + const aprB = parseFloat( + b?.provider?.aprWithoutFee ?? + b?.aprInfo?.value ?? + b?.aprInfo?.apr ?? + '0', + ); + return aprA - aprB; + }, render: (item) => ( <AprTextpackages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx (5)
369-383: Fix countdown visibility window.The alert only renders after the end time passes. Flip the comparison so it shows while time is between start and end.
{detailInfo?.countDownAlert?.startTime && detailInfo?.countDownAlert?.endTime && now > detailInfo.countDownAlert.startTime && - detailInfo.countDownAlert.endTime < now ? ( + detailInfo.countDownAlert.endTime > now ? ( <YStack pb="$1">
565-571: Remove incorrect await.Line 567 awaits
onCreateAddress()but it returns void, not a Promise. Remove the await to fix the linter error.const handleCreateAddress = useCallback(async () => { if (onCreateAddress) { - await onCreateAddress(); + onCreateAddress(); } await refreshManageAccount(); await refreshManageData(); }, [onCreateAddress, refreshManageAccount, refreshManageData]);
145-171: Use a more unique key.Using
cell.title.textassumes titles are unique. If they repeat, React will misbehave. Use an index or unique ID.- {intro.items.map((cell) => ( + {intro.items.map((cell, idx) => ( <GridItem - key={cell.title.text} + key={`${cell.title.text}-${idx}`}
267-314: Move key to Fragment.When mapping returns a Fragment, put the key on the Fragment itself, not its children. This prevents reconciliation issues.
{risk.items?.map((item) => ( - <> - <XStack ai="center" gap="$3" key={item.title.text}> + <Fragment key={item.title.text}> + <XStack ai="center" gap="$3"> ... - </> + </Fragment>
825-843: Fix inconsistent indexedAccountId usage.Line 825 destructures
indexedAccountIdfrom resolved params, but line 839 passesindexedAccount?.idfrom the hook. This creates inconsistency—share-link navigation might use wrong account context.const { accountId, networkId, indexedAccountId, symbol, provider, vault } = resolvedParams; const { earnAccount, detailInfo, tokenInfo, protocolInfo, isLoading, refreshData, refreshAccount, } = useProtocolDetailData({ accountId, networkId, - indexedAccountId: indexedAccount?.id, + indexedAccountId, symbol, provider, vault, });packages/kit/src/views/Staking/components/UniversalWithdraw/index.tsx (1)
498-583: Fix crash when rewards are missing
transactionConfirmation?.rewards.map((reward) => { ... })still calls.maponundefinedwhentransactionConfirmationis unset or when it has norewards, which will crash the component (especially on first render while data loads).Guard
rewardsbefore mapping, for example:- {transactionConfirmation?.rewards.map((reward) => { + {transactionConfirmation?.rewards?.map((reward) => {This matches the earlier review note and makes the rewards section safe in all loading/error states.
packages/kit/src/views/Staking/components/UniversalStake/index.tsx (1)
1168-1212: Guard.rewards.mapto avoid runtime crash whenrewardsis undefined
transactionConfirmation?.rewards.map((reward) => { ... })still assumesrewardsis always a defined array.If
transactionConfirmationexists butrewardsisundefined(which can happen while the confirmation payload is still shaping up), this will throwCannot read properties of undefined (reading 'map')and crash the component.You already use optional chaining on
transactionConfirmation, so the minimal fix is to also guardrewards:- {transactionConfirmation?.rewards.map((reward) => { + {transactionConfirmation?.rewards?.map((reward) => {You could also choose a more explicit guard (
if (!transactionConfirmation?.rewards?.length) return null;) if you want to avoid rendering the whole rewards block until the list is present.This is the same issue called out in an earlier review; it still needs a fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
packages/kit/src/components/LightweightChart/LightweightChart.native.tsx(1 hunks)packages/kit/src/components/LightweightChart/LightweightChart.tsx(1 hunks)packages/kit/src/components/LightweightChart/hooks/useChartConfig.ts(1 hunks)packages/kit/src/components/LightweightChart/index.ts(1 hunks)packages/kit/src/components/LightweightChart/types/index.ts(1 hunks)packages/kit/src/components/LightweightChart/utils/chartOptions.ts(1 hunks)packages/kit/src/components/LightweightChart/utils/constants.ts(1 hunks)packages/kit/src/components/LightweightChart/utils/htmlTemplate.ts(1 hunks)packages/kit/src/views/Earn/components/EarnPageContainer.tsx(1 hunks)packages/kit/src/views/Earn/components/PortfolioTabContent.tsx(1 hunks)packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts(1 hunks)packages/kit/src/views/Earn/pages/EarnProtocolDetails/components/ApyChart.tsx(1 hunks)packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx(1 hunks)packages/kit/src/views/Earn/pages/EarnProtocols/index.tsx(1 hunks)packages/kit/src/views/Market/components/Chart/ChartViewAdapter.tsx(3 hunks)packages/kit/src/views/Market/components/Chart/chartUtils.ts(2 hunks)packages/kit/src/views/Staking/components/StakingFormWrapper/index.tsx(1 hunks)packages/kit/src/views/Staking/components/UniversalStake/index.tsx(4 hunks)packages/kit/src/views/Staking/components/UniversalWithdraw/index.tsx(6 hunks)packages/kit/src/views/Staking/pages/ManagePosition/components/HeaderRight.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (22)
packages/kit/src/views/Staking/components/StakingFormWrapper/index.tsx (1)
8-8: LGTM! Spacing increase improves layout.The increased gap enhances visual hierarchy and breathing room in the staking form. Clean change aligned with the redesign goals.
packages/kit/src/views/Market/components/Chart/chartUtils.ts (2)
79-91: Time scale is now visible—confirm this is intentional.You've enabled the time scale with custom MM/DD formatting. This changes the chart's appearance. Verify this matches the design spec.
86-91: Date formatter looks clean.The MM/DD formatter is straightforward and handles padding correctly.
packages/kit/src/views/Earn/components/PortfolioTabContent.tsx (4)
45-115: LGTM: WrappedActionButton handles action dispatch cleanly.The component correctly determines the symbol for config lookup, handles airdrop vs normal claims, and passes all necessary data to
handleAction. Type guards for optional properties are appropriate.
117-147: LGTM: DepositField renders deposit info correctly.Clean presentational component with appropriate conditional rendering for
vaultName.
178-199: LGTM: ActionField correctly keys the mapped elements.
584-627: LGTM: Skeleton components provide appropriate loading state.packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts (4)
24-96: LGTM: Utility functions are well-structured and pure.The helper functions for creating keys, filtering by fiat value, sorting, calculating totals, and aggregating by protocol are clear and correct. Using BigNumber for currency calculations is appropriate.
149-190: LGTM: useAccountState correctly manages account change detection and request staleness.The ref-based tracking of account changes and request IDs prevents stale data from updating the UI.
308-473: LGTM: RAF batching and incremental updates are well-implemented.The request animation frame throttling, staleness checks, and partial vs. full refresh logic are solid. The only issue is the
fetchedKeystracking (noted separately).
475-503: LGTM: Hook integration and memoization are correct.The
usePromiseResultintegration,refreshwrapper, andaggregatedInvestmentsmemo are properly structured. The eslint disable forinvestmentMapRefis justified since refs don't trigger re-renders.packages/kit/src/components/LightweightChart/LightweightChart.native.tsx (2)
36-59: LGTM: Message handling is robust.The try-catch block properly handles JSON parsing errors, and the type guard for 'ready' and 'hover' messages is clear.
81-95: Good security posture for WebView.Lines 91-94 properly restrict WebView capabilities:
allowFileAccess={false},allowUniversalAccessFromFileURLs={false}, andmixedContentMode="never"minimize security risks.packages/kit/src/components/LightweightChart/index.ts (1)
1-3: LGTM: Clean barrel exports.packages/kit/src/components/LightweightChart/utils/constants.ts (2)
6-10: LGTM: Default colors defined.
1-4: Version is correctly aligned. No action needed.The constant
3.8.0matches the^3.8.0range in both package.json files. Since CDN URLs require exact versions, using a hardcoded constant is the right approach. Importing directly from package.json wouldn't work—package.json stores the range^3.8.0, not the exact version.packages/kit/src/components/LightweightChart/LightweightChart.tsx (1)
86-96: LGTM: ResizeObserver properly handles chart resizing.packages/kit/src/components/LightweightChart/hooks/useChartConfig.ts (1)
18-47: LGTM: Hook correctly memoizes config.The dependency array properly includes all inputs that affect the config, and default values are sensibly chosen from constants. Theme fallbacks ensure the chart always has valid colors.
packages/kit/src/views/Earn/components/EarnPageContainer.tsx (1)
48-66: LGTM: Proper memoization of callbacks and computed values.
handleBackandcustomHeaderLeftare correctly memoized with appropriate dependencies.packages/kit/src/components/LightweightChart/utils/htmlTemplate.ts (1)
132-145: LGTM!Clean HTML template generation with proper structure and documentation. The function correctly embeds config and wires up the chart library.
packages/kit/src/views/Earn/pages/EarnProtocolDetails/components/ApyChart.tsx (1)
117-157: LGTM!Data fetching and transformation logic is solid. Correctly handles empty results, calculates high/low values, and converts timestamps for the chart.
packages/kit/src/views/Staking/components/UniversalStake/index.tsx (1)
230-256: Guarding transaction confirmation requests when disabled looks goodShort‑circuiting
fetchTransactionConfirmationwhenisDisabledis true avoids pointless API calls and state updates while the product is disabled. IncludingisDisabledin the hook dependency list also keeps the debounced flow consistent with the new flag. No issues here from my side.
| useEffect(() => { | ||
| if (webViewReady && webViewRef.current) { | ||
| const updateScript = ` | ||
| (function() { | ||
| const newConfig = ${JSON.stringify(chartConfig)}; | ||
| if (window.series) { | ||
| window.series.setData(newConfig.data); | ||
| window.chart.timeScale().fitContent(); | ||
| } | ||
| })(); | ||
| true; | ||
| `; | ||
| webViewRef.current.injectJavaScript(updateScript); | ||
| } | ||
| }, [chartConfig, webViewReady]); |
There was a problem hiding this comment.
🧩 Analysis chain
Check serialized config size and injection performance.
Line 66 serializes the entire chartConfig with JSON.stringify on every data update. For large datasets, this could impact performance. Additionally, no error handling exists if injectJavaScript fails.
Consider:
- Measuring the typical size of
chartConfig.dataarrays - Adding error handling for injection failures
- Potentially debouncing updates if data changes rapidly
🏁 Script executed:
#!/bin/bash
# Check typical usage patterns of LightweightChart to assess data size
rg -A 5 -B 2 '<LightweightChart' --type=tsx --type=tsLength of output: 27
---
🏁 Script executed:
```shell
#!/bin/bash
# Find LightweightChart usage with corrected syntax
rg -A 5 -B 2 '<LightweightChart' -g '*.ts' -g '*.tsx'
Length of output: 812
🏁 Script executed:
#!/bin/bash
# Look for error handling patterns around injectJavaScript
rg -n 'injectJavaScript' -C 4 -g '*.ts' -g '*.tsx'Length of output: 7246
🏁 Script executed:
#!/bin/bash
# Check if debounce or throttle is used anywhere in the component file
rg -n 'debounce|throttle|memo|useMemo|useCallback' packages/kit/src/components/LightweightChart/LightweightChart.native.tsxLength of output: 150
🏁 Script executed:
#!/bin/bash
# Examine the full LightweightChart.native.tsx file around the useEffect
cat packages/kit/src/components/LightweightChart/LightweightChart.native.tsx | head -100Length of output: 2758
Add error handling for injectJavaScript call.
Line 74 lacks a try-catch wrapper. Other injectJavaScript calls in the codebase (explorerUtils.ts, Browser.native.tsx, discovery/actions.ts) wrap this in try-catch because the call can fail—especially on mobile orientation changes—and crash the app. Apply the same pattern here.
For JSON serialization and update frequency: the component already uses useMemo for the HTML content. Monitor actual usage patterns in production before optimizing further, but error handling is mandatory now.
🤖 Prompt for AI Agents
In packages/kit/src/components/LightweightChart/LightweightChart.native.tsx
around lines 62 to 76, the webViewRef.current.injectJavaScript call is not
wrapped in error handling and can throw (e.g. on orientation changes); wrap the
injectJavaScript invocation in a try-catch, catch any thrown error, and handle
it consistently with other parts of the codebase (log the error via the existing
logger or console.error with context) so the app does not crash; ensure the try
block only contains the injectJavaScript call and keep the existing logic
otherwise unchanged.
| useEffect(() => { | ||
| if (!chartContainerRef.current) return; | ||
|
|
||
| const chart = createChart(chartContainerRef.current, { | ||
| ...createChartOptions(chartConfig.theme), | ||
| width: chartContainerRef.current.clientWidth, | ||
| height, | ||
| }); | ||
|
|
||
| const series = chart.addAreaSeries( | ||
| createAreaSeriesOptions(chartConfig.theme), | ||
| ); | ||
| series.setData(chartConfig.data as any); | ||
| chart.timeScale().fitContent(); | ||
|
|
||
| chartRef.current = chart; | ||
| seriesRef.current = series; | ||
|
|
||
| // Subscribe to crosshair move events | ||
| if (onHover) { | ||
| chart.subscribeCrosshairMove((param) => { | ||
| if ( | ||
| param.time && | ||
| param.seriesPrices && | ||
| param.seriesPrices.size > 0 && | ||
| param.point | ||
| ) { | ||
| const price = param.seriesPrices.get(series); | ||
| if (price !== undefined) { | ||
| onHover({ | ||
| time: | ||
| typeof param.time === 'number' | ||
| ? param.time | ||
| : Number(param.time), | ||
| price: typeof price === 'number' ? price : Number(price), | ||
| x: param.point.x, | ||
| y: param.point.y, | ||
| }); | ||
| } | ||
| } else { | ||
| onHover({ | ||
| time: undefined, | ||
| price: undefined, | ||
| x: undefined, | ||
| y: undefined, | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| // Handle resize | ||
| const resizeObserver = new ResizeObserver((entries) => { | ||
| if ( | ||
| entries.length === 0 || | ||
| entries[0].target !== chartContainerRef.current | ||
| ) | ||
| return; | ||
| const { width: newWidth } = entries[0].contentRect; | ||
| chart.applyOptions({ width: newWidth }); | ||
| }); | ||
|
|
||
| resizeObserver.observe(chartContainerRef.current); | ||
|
|
||
| return () => { | ||
| resizeObserver.disconnect(); | ||
| chart.remove(); | ||
| }; | ||
| }, [chartConfig, height, onHover]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Large useEffect could be split for clarity.
This useEffect handles chart creation, event subscription, and resize observation all in one. The dependency array on line 102 includes chartConfig, height, and onHover, which means the entire chart is recreated when any of these change.
Consider splitting into:
- Chart creation/destruction (depends on
height) - Data updates (depends on
chartConfig) - Hover handler updates (depends on
onHover)
This would avoid recreating the chart on every data change.
🤖 Prompt for AI Agents
In packages/kit/src/components/LightweightChart/LightweightChart.tsx around
lines 35 to 102, the single large useEffect is creating/removing the chart,
subscribing to crosshair events, and observing resizes so any change to
chartConfig or onHover recreates the whole chart; refactor into three smaller
effects: 1) a chart creation/destruction effect that depends only on height
(creates chart, adds series, sets initial options, starts ResizeObserver, and
cleans up chart and observer), 2) a data update effect that depends on
chartRef.current/seriesRef.current and chartConfig.data (calls series.setData
and chart.timeScale().fitContent without recreating the chart), and 3) a hover
subscription effect that depends only on onHover and the current series
(subscribeCrosshairMove on mount or when onHover changes and unsubscribe on
cleanup, sending undefined coordinates when no hit); ensure you reference
chartRef.current/seriesRef.current safely and tighten dependency arrays so data
or hover updates do not recreate the chart.
| const series = chart.addAreaSeries( | ||
| createAreaSeriesOptions(chartConfig.theme), | ||
| ); | ||
| series.setData(chartConfig.data as any); | ||
| chart.timeScale().fitContent(); |
There was a problem hiding this comment.
🧩 Analysis chain
Type assertion suggests API mismatch.
Line 47 uses as any when calling series.setData(chartConfig.data as any). This indicates a type incompatibility between ILightweightChartData[] and what the library expects.
Verify the data format matches lightweight-charts v3.8.0 requirements:
🌐 Web query:
What is the expected data format for setData in lightweight-charts 3.8.0 area series?
💡 Result:
For an area series in lightweight-charts v3.8.0, setData expects an array of AreaData items: objects with a time (Time) and a numeric value, optionally including per-point colors (lineColor, topColor, bottomColor). [1][2]
References:
- AreaData interface (time, value, lineColor, topColor, bottomColor). [1]
- Type definitions showing AreaData / SingleValueData shape. [2]
🏁 Script executed:
#!/bin/bash
# Search for ILightweightChartData definition and chartConfig usage in the repo
echo "=== Searching for ILightweightChartData type definition ==="
rg -n "ILightweightChartData" --type=ts --type=tsx -B3 -A5
echo ""
echo "=== Searching for chartConfig.data reference ==="
rg -n "chartConfig\\.data" --type=ts --type=tsx -B2 -A2
echo ""
echo "=== Checking LightweightChart.tsx for interface definitions ==="
fd "LightweightChart\\.tsx" --type=f -x cat {}Length of output: 2966
🏁 Script executed:
#!/bin/bash
# Find and display types definition file
echo "=== Finding types file ==="
fd "types\\.ts" "packages/kit/src/components/LightweightChart/" --type=f -x cat {}
echo ""
echo "=== Finding useChartConfig hook ==="
fd "useChartConfig" "packages/kit/src/components/LightweightChart/" --type=f -x cat {}
echo ""
echo "=== Searching for AreaData or lightweight-charts type imports ==="
rg "AreaData|from.*lightweight-charts" "packages/kit/src/components/LightweightChart/" -n -A2Length of output: 2158
🏁 Script executed:
#!/bin/bash
# Find ILightweightChartConfig definition
echo "=== Searching for ILightweightChartConfig ==="
rg "interface ILightweightChartConfig|type ILightweightChartConfig" "packages/kit/src/components/LightweightChart/" -A10
echo ""
echo "=== Looking for types directory ==="
fd "types" "packages/kit/src/components/LightweightChart/" --type d
echo ""
echo "=== Check all TypeScript files in LightweightChart ==="
fd "\\.ts$" "packages/kit/src/components/LightweightChart/" --type fLength of output: 1497
🏁 Script executed:
#!/bin/bash
# Display complete types/index.ts file
echo "=== Complete types/index.ts ==="
cat "packages/kit/src/components/LightweightChart/types/index.ts"Length of output: 858
Remove the unnecessary type assertion.
The as any on line 47 is unwarranted. ILightweightChartData defines objects with time: number and value: number, which perfectly matches what lightweight-charts v3.8.0 AreaData expects: objects with a time (accepts numbers) and numeric value. The data transformation in useChartConfig already produces the correct shape. Remove as any to restore type safety.
series.setData(chartConfig.data);
🤖 Prompt for AI Agents
In packages/kit/src/components/LightweightChart/LightweightChart.tsx around
lines 44 to 48, remove the unnecessary unsafe type assertion on series.setData:
the chartConfig.data already matches the AreaData shape (time:number,
value:number) so drop "as any" and call series.setData(chartConfig.data) to
restore type safety; ensure imports/types remain compatible with
lightweight-charts v3.8.0 and adjust any surrounding code or types if the
compiler complains.
| export interface IChartMessage { | ||
| type: 'ready' | 'hover'; | ||
| time?: string; | ||
| price?: string; | ||
| x?: number; | ||
| y?: number; | ||
| } |
There was a problem hiding this comment.
Type mismatch: IChartMessage uses strings but onHover expects numbers.
Lines 38-39 define time and price as optional strings in IChartMessage, but lines 29-30 in ILightweightChartProps.onHover expect optional numbers. This inconsistency requires runtime conversion and could introduce bugs.
Consider aligning the types. If the message format requires strings for serialization, update the interface to reflect that clearly:
export interface IChartMessage {
type: 'ready' | 'hover';
- time?: string;
- price?: string;
+ time?: number;
+ price?: number;
x?: number;
y?: number;
}Or add JSDoc comments explaining the conversion requirement if strings are mandatory for the WebView bridge.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface IChartMessage { | |
| type: 'ready' | 'hover'; | |
| time?: string; | |
| price?: string; | |
| x?: number; | |
| y?: number; | |
| } | |
| export interface IChartMessage { | |
| type: 'ready' | 'hover'; | |
| time?: number; | |
| price?: number; | |
| x?: number; | |
| y?: number; | |
| } |
🤖 Prompt for AI Agents
In packages/kit/src/components/LightweightChart/types/index.ts around lines 36
to 42, IChartMessage declares time and price as optional strings while
ILightweightChartProps.onHover expects numbers; align these types to avoid
runtime conversion bugs: either change IChartMessage.time and
IChartMessage.price to optional numbers to match onHover (preferred), or if the
WebView bridge requires string serialization, change the onHover parameter types
to optional strings and add JSDoc explaining the serialization/parse contract
and where conversion happens; ensure only one canonical type is used across the
message definition and handler.
| crosshair: { | ||
| mode: 1, // CrosshairMode.Normal | ||
| vertLine: { | ||
| color: theme.lineColor, | ||
| width: 1, | ||
| style: 3, | ||
| labelVisible: false, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace magic number with enum.
Line 21 uses mode: 1 with a comment // CrosshairMode.Normal. Import and use the actual enum instead.
+import { CrosshairMode } from 'lightweight-charts';
+
export function createChartOptions(
theme: ILightweightChartTheme,
): DeepPartial<ChartOptions> {
return {
// ...
crosshair: {
- mode: 1, // CrosshairMode.Normal
+ mode: CrosshairMode.Normal,📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| crosshair: { | |
| mode: 1, // CrosshairMode.Normal | |
| vertLine: { | |
| color: theme.lineColor, | |
| width: 1, | |
| style: 3, | |
| labelVisible: false, | |
| import { CrosshairMode } from 'lightweight-charts'; | |
| export function createChartOptions( | |
| theme: ILightweightChartTheme, | |
| ): DeepPartial<ChartOptions> { | |
| return { | |
| // ... | |
| crosshair: { | |
| mode: CrosshairMode.Normal, | |
| vertLine: { | |
| color: theme.lineColor, | |
| width: 1, | |
| style: 3, | |
| labelVisible: false, |
🤖 Prompt for AI Agents
In packages/kit/src/components/LightweightChart/utils/chartOptions.ts around
lines 20 to 26, the crosshair.mode is set to the magic number `1` with a comment
indicating `CrosshairMode.Normal`; replace the literal with the actual enum
value by importing CrosshairMode from the lightweight-charts package (or the
project’s chart enums) and set mode: CrosshairMode.Normal instead of 1; add the
import at the top of the file and update the crosshair object to use the enum
constant.
| const handleManagePress = useCallback( | ||
| async (asset: IEarnPortfolioInvestment['assets'][number]) => { | ||
| const symbol = asset.token.info.symbol; | ||
| if (symbol === 'USDe') { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider externalizing the USDe symbol check.
The hardcoded check for symbol === 'USDe' creates a special navigation path. If this list grows, consider moving symbol-based routing logic to a config map or helper function for maintainability.
🤖 Prompt for AI Agents
In packages/kit/src/views/Earn/components/PortfolioTabContent.tsx around line
406, there's a hardcoded symbol check `if (symbol === 'USDe')` creating
special-case navigation; extract this logic into a centralized mapping or helper
(e.g., a symbolToRoute map or getRouteForSymbol(symbol) function) and replace
the inline comparison with a lookup call that returns the appropriate
route/action for the symbol (falling back to default behavior when not found).
Update imports and tests as needed and keep behavior identical for 'USDe' while
making it easy to add more symbol rules later.
| <YStack> | ||
| {investments.map((item, index) => { | ||
| const showDivider = index < investments.length - 1; | ||
| const key = `${item.protocol.providerDetail.code}_${ | ||
| item.protocol.vaultName || '' | ||
| }_${item.network.networkId}`; | ||
|
|
||
| return ( | ||
| <> | ||
| <PortfolioItem key={key} portfolioItem={item} /> | ||
| {showDivider ? <Divider my="$4" /> : null} | ||
| </> | ||
| ); | ||
| })} | ||
| </YStack> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider virtualization for large portfolios.
If users accumulate many protocol positions, this non-virtualized list could impact scroll performance. Monitor and consider adding virtualization if portfolios commonly exceed 50 items.
🤖 Prompt for AI Agents
In packages/kit/src/views/Earn/components/PortfolioTabContent.tsx around lines
698 to 712, the current map renders all PortfolioItem components inline which
can hurt scroll performance for large portfolios; replace this non-virtualized
mapping with a virtualization solution (e.g., react-virtual, react-window,
react-virtuoso or a platform-appropriate virtual list) so only visible items are
rendered. Implement virtualization with a stable itemKey using the existing key
composition (protocol.providerDetail.code + vaultName + networkId), preserve
Divider rendering between items (render divider inside the row renderer for all
but the last visible item), supply an estimated item height and container
ref/scrollParent, and conditionally enable virtualization only when
investments.length exceeds a threshold (e.g., 50) to avoid unnecessary
complexity for small lists.
| priceLineVisible: false, | ||
| lastValueVisible: false, | ||
| crosshairMarkerVisible: true, | ||
| crosshairMarkerRadius: 4, | ||
| crosshairMarkerBorderColor: lineColor, | ||
| crosshairMarkerBackgroundColor: '#FFFFFF', |
There was a problem hiding this comment.
Hardcoded white breaks dark theme.
Line 145 uses '#FFFFFF' for the crosshair marker background. This won't work in dark mode. Pass a theme-aware color (like you did with textColor) or derive it from the lineColor parameter.
Apply this diff:
+ backgroundTextColor?: string,
) {
...
const newSeries = chart.addAreaSeries({
lineColor,
topColor,
bottomColor,
lineWidth: 2,
priceLineVisible: false,
lastValueVisible: false,
crosshairMarkerVisible: true,
crosshairMarkerRadius: 4,
crosshairMarkerBorderColor: lineColor,
- crosshairMarkerBackgroundColor: '#FFFFFF',
+ crosshairMarkerBackgroundColor: backgroundTextColor ?? '#FFFFFF',
});Then pass the appropriate theme color from ChartViewAdapter.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/kit/src/views/Market/components/Chart/chartUtils.ts around lines 140
to 145 the crosshairMarkerBackgroundColor is hardcoded to '#FFFFFF' which breaks
dark theme; replace that hardcoded value with a theme-aware color (either accept
a new backgroundColor parameter, derive a contrasting background from the
existing lineColor, or use the same textColor logic used elsewhere) and update
callers to pass the theme-aware color; then update ChartViewAdapter to pass the
appropriate color from the active theme (e.g., a light background in light theme
and a dark background in dark theme) so the marker remains visible in both
themes.
| historyAction?: IEarnHistoryActionIcon; | ||
| onHistory?: ((params?: { filterType?: string }) => void) | undefined; | ||
| }) => { | ||
| if (historyAction && !historyAction?.disabled) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove redundant optional chaining.
After the &&, historyAction is guaranteed truthy. The ?. on the right side does nothing.
Apply this diff:
- if (historyAction && !historyAction?.disabled) {
+ if (historyAction && !historyAction.disabled) {📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (historyAction && !historyAction?.disabled) { | |
| if (historyAction && !historyAction.disabled) { |
🤖 Prompt for AI Agents
In
packages/kit/src/views/Staking/pages/ManagePosition/components/HeaderRight.tsx
around line 15, the conditional uses redundant optional chaining: "if
(historyAction && !historyAction?.disabled)". Since the left side ensures
historyAction is truthy, remove the "?." and change the check to "if
(historyAction && !historyAction.disabled)"; update any surrounding
code/comments if needed and run lint/tests.
…ation and improve routing parameters
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kit/src/views/Earn/EarnHome.tsx (1)
130-188: Handle malformed staking deeplinks more gracefully.In
onBannerPress, whenhrefcontains/defi/stakingbut required params (likenetworkId) are missing, the function hits the staking branch and then returns without any fallback. From a user view, tapping such a banner does nothing.Consider falling back to
openUrlInApp/openUrlExternal(or logging/monitoring) when deeplink parsing fails, instead of a silent no‑op.
♻️ Duplicate comments (9)
packages/shared/src/routes/staking.ts (1)
58-65: Unresolved: ManagePosition still missing accountId.This is the same critical issue flagged in the previous review. The ManagePosition component requires
accountId(used at lines 132, 151, 245, 254 in index.tsx), but the route definition omits it. The component falls back touseActiveAccount, which is inconsistent withProtocolDetailsV2that passesaccountIdexplicitly.Either add
accountIdandindexedAccountIdto the params, or document why they're intentionally omitted (likeProtocolDetailsV2Sharedoes on line 56).packages/kit/src/views/Earn/pages/EarnProtocolDetails/components/ApyChart.tsx (1)
87-111: Fix hover handling when APY or timestamp equals zero.The truthy checks on line 99 (
if (time && price && x !== undefined && y !== undefined)) will reject legitimate zero values. A 0% APY or timestamp 0 won't update hover state.Guard against
undefined/nullinstead:const handleHover = useCallback( ({ time, price, x, y, }: { time?: number; price?: number; x?: number; y?: number; }) => { - if (time && price && x !== undefined && y !== undefined) { + if ( + time !== undefined && + time !== null && + price !== undefined && + price !== null && + x !== undefined && + y !== undefined + ) { setHoverData({ time, apy: price, x, y, }); } else { setHoverData(null); } }, [], );packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx (4)
369-383: Fix countdown visibility window.Line 372 checks
endTime < now, which only renders the alert AFTER the countdown ends. Flip it to show the alert WHILE counting down (between startTime and endTime).{detailInfo?.countDownAlert?.startTime && detailInfo?.countDownAlert?.endTime && now > detailInfo.countDownAlert.startTime && - detailInfo.countDownAlert.endTime < now ? ( + detailInfo.countDownAlert.endTime > now ? ( <YStack pb="$1">
145-171: Consider using a more unique key.Line 147 uses
key={cell.title.text}, which assumes titles are unique. If they're not, React will warn.Use index or composite key:
- {intro.items.map((cell) => ( + {intro.items.map((cell, idx) => ( <GridItem - key={cell.title.text} + key={`${cell.title.text}-${idx}`}
267-314: Move key to Fragment.When mapping returns a Fragment, the key must be on the Fragment itself, not its children. This prevents React reconciliation issues.
{risk.items?.map((item) => ( - <> - <XStack ai="center" gap="$3" key={item.title.text}> + <Fragment key={item.title.text}> + <XStack ai="center" gap="$3"> <YStack flex={1} gap="$2"> ... </YStack> </XStack> ... - </> + </Fragment> ))}
825-843: Fix inconsistent indexedAccountId usage.Line 825 destructures
indexedAccountIdfromresolvedParams, but line 839 passesindexedAccount?.idfrom the active account hook instead. This creates inconsistency—share-link navigation might use a different account than intended.Use the resolved param consistently:
const { accountId, networkId, indexedAccountId, symbol, provider, vault } = resolvedParams; const { earnAccount, detailInfo, tokenInfo, protocolInfo, isLoading, refreshData, refreshAccount, } = useProtocolDetailData({ accountId, networkId, - indexedAccountId: indexedAccount?.id, + indexedAccountId, symbol, provider, vault, });packages/kit/src/views/Staking/pages/ManagePosition/index.tsx (1)
89-117: Fix the incomplete type annotation (past review issue).The type annotation still omits
accountIdandindexedAccountId, but they're returned on lines 108-110. This causes a TypeScript error. Add both fields to the type or remove the explicit generic.Also,
isFromShareLinkis hardcoded tofalse(line 115). If it's never true, remove it from the type and return object.Apply this diff:
- const resolvedParams = useMemo<{ - networkId: string; - symbol: ISupportedSymbol; - provider: string; - vault: string | undefined; - isFromShareLink: boolean; - }>(() => { + const resolvedParams = useMemo<{ + accountId: string; + indexedAccountId: string | undefined; + networkId: string; + symbol: ISupportedSymbol; + provider: string; + vault: string | undefined; + }>(() => { const routeParams = route.params as any; const { accountId: routeAccountId, indexedAccountId: routeIndexedAccountId, networkId, symbol, provider, vault, } = routeParams; return { accountId: routeAccountId || activeAccount.account?.id || '', indexedAccountId: routeIndexedAccountId || activeAccount.indexedAccount?.id, networkId, symbol, provider, vault, - isFromShareLink: false, }; }, [route.params, activeAccount]);packages/kit/src/views/Earn/EarnHome.tsx (1)
225-283: Spacer<Stack h={tabPageHeight} />is still a layout workaround.The native+md branch still relies on an empty Stack plus an absolutely positioned header to reserve vertical space. This is brittle and hard to reason about.
Consider refactoring so the header participates in normal layout (padding/margin or a wrapping container) instead of using an empty spacer.
packages/kit/src/views/Discovery/pages/Browser/Browser.native.tsx (1)
380-391: Earn section visibility still duplicates conditions and lacks Freeze.You still check
selectedHeaderTab === ETranslations.global_earntwice (StackdisplayandshowContent) and renderEarnHomeWithProvidereven when the Earn tab is hidden. Consider:
- Relying on a single visibility condition, and
- Wrapping
EarnHomeWithProviderinFreezeso Earn logic pauses when the tab is not active.This matches the earlier feedback on this block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
packages/kit/src/views/Discovery/pages/Browser/Browser.native.tsx(2 hunks)packages/kit/src/views/Earn/EarnHome.tsx(8 hunks)packages/kit/src/views/Earn/components/EarnMainTabs.tsx(1 hunks)packages/kit/src/views/Earn/pages/EarnProtocolDetails/components/ApyChart.tsx(1 hunks)packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx(1 hunks)packages/kit/src/views/Staking/pages/ManagePosition/index.tsx(1 hunks)packages/shared/src/routes/staking.ts(3 hunks)packages/shared/src/routes/tabDiscovery.ts(1 hunks)packages/shared/src/routes/tabEarn.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
packages/shared/src/routes/tabDiscovery.ts (1)
10-10: LGTM!Clean type extension for Earn tab navigation. The optional
earnTabparam aligns well with the existingdefaultTabpattern.packages/shared/src/routes/tabEarn.ts (2)
19-26: LGTM!The
EarnProtocolDetailsparams cover all necessary context for protocol detail pages, including optional indexed account support for account-specific views.
27-32: Good separation of share link parameters.Using friendly network names (like 'ethereum') instead of IDs makes share URLs more readable. The comment clarifies the intent well.
packages/kit/src/views/Earn/pages/EarnProtocolDetails/components/ApyChart.tsx (2)
126-166: LGTM!APY history fetching and data transformation is well-structured. The timestamp conversion (ms to seconds) and sorting ensure chart compatibility. High/low calculations provide useful context.
265-293: Popover positioned correctly relative to hover point.The popover uses
hoverData.xandhoverData.ywith transform to center horizontally. TheMath.max(10, hoverData.y - 70)prevents edge clipping. Good implementation.packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx (3)
87-104: LGTM!The ManagersSection cleanly renders manager info with proper separators. The conditional rendering and index-based separator logic work correctly.
756-823: Well-structured route param resolution.The
resolvedParamsmemo handles both share links (with friendly network names) and normal navigation. Error handling withOneKeyLocalErrorfor invalid network/symbol/provider is appropriate. Fallback toactiveAccountfor share links ensures graceful handling.
936-1016: Smart conditional footer rendering.The footer logic differentiates between USDe (Receive/Trade) and other assets (Deposit/Withdraw), with proper button disabling based on loading/account state. Mobile-only rendering via
gtMdcheck is appropriate.packages/kit/src/views/Staking/pages/ManagePosition/index.tsx (1)
301-317: ConfirmsceneNameshould be 'home'.Line 305 sets
sceneNametoEAccountSelectorSceneName.home. Is 'home' the correct scene for a manage position page, or should it be a staking-specific scene?packages/kit/src/views/Discovery/pages/Browser/Browser.native.tsx (1)
133-141: Route params wiring for Earn looks correct.Destructuring
{ defaultTab, earnTab } = route?.params || {}is safe and keeps the header tab and Earn inner tab concerns separate. No changes needed here.packages/kit/src/views/Earn/EarnHome.tsx (3)
288-327: Desktop/web Earn container and tabs wiring look good.The non‑native branch neatly composes
EarnPageContainer, overview + banners, andEarnMainTabswith a standardRefreshControl. Props passed toEarnMainTabs(defaultTab,onTabChange, loading flags, FAQ data) align with its interface.No changes needed here.
330-355: Provider composition for Earn state looks correct.Wrapping
BasicEarnHomewithAccountSelectorProviderMirrorandEarnProviderMirror(usingEJotaiContextStoreNames.earn) keeps Earn state scoped and avoids leaking store details to callers. PassingoverrideDefaultTab={defaultTab}cleanly bridges external routing into the inner tabs.Looks solid.
358-395: Earn→Discovery navigation bridge is clean and preserves tab selection.
useNavigateToNativeEarnPagereads the Earn route’stabparam and, on native+md, redirects into Discovery withdefaultTab: global_earnandearnTab: tabParam. The mainEarnHomecomponent then still renders a normal Earn page on other platforms.This keeps deep links working while unifying the iPad layout under Discovery. Nicely done.
| const tabNames = useMemo( | ||
| () => ({ | ||
| assets: intl.formatMessage({ | ||
| id: ETranslations.earn_available_assets, | ||
| }), | ||
| portfolio: intl.formatMessage({ | ||
| id: ETranslations.earn_portfolio, | ||
| }), | ||
| faqs: intl.formatMessage({ id: ETranslations.global_faqs }), | ||
| }), | ||
| [intl], | ||
| ); | ||
|
|
||
| const initialTabName = useMemo(() => { | ||
| if (defaultTab === 'portfolio') return tabNames.portfolio; | ||
| if (defaultTab === 'faqs') return tabNames.faqs; | ||
| return tabNames.assets; | ||
| }, [defaultTab, tabNames]); | ||
|
|
||
| // Switch tab when defaultTab changes (from route navigation) | ||
| useEffect(() => { | ||
| if (defaultTab && tabsRef.current) { | ||
| const targetTabName = initialTabName; | ||
| const currentTabName = tabsRef.current.getFocusedTab(); | ||
| if (currentTabName !== targetTabName) { | ||
| tabsRef.current.jumpToTab(targetTabName); | ||
| } | ||
| } | ||
| }, [defaultTab, initialTabName]); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid using translated labels as tab identifiers.
Tab names are the translated labels, and handleTabPress infers 'assets' | 'portfolio' | 'faqs' by comparing those strings. This is fragile if copy changes or two labels collide.
Prefer stable IDs for Tabs.Tab.name (e.g. 'assets' | 'portfolio' | 'faqs') and pass the localized label separately (via a label prop or custom tabBar renderer). That keeps routing logic robust against text changes.
🤖 Prompt for AI Agents
In packages/kit/src/views/Earn/components/EarnMainTabs.tsx around lines 36 to
65, the code currently uses translated labels as tab identifiers which is
fragile; change to stable tab IDs ('assets' | 'portfolio' | 'faqs') for
Tabs.Tab.name and keep translations only as a separate label property.
Concretely: replace tabNames so it maps id->label (e.g. { assets:
intl.formatMessage(...), ... }), derive initialTabId from defaultTab (return
'assets'|'portfolio'|'faqs' not translated text), update the effect to compare
tabsRef.current.getFocusedTab() to the target tab id and call jumpToTab with the
id, and update handleTabPress/any other comparisons to use the stable id values
rather than translated strings. Ensure Tabs render uses the id as name and the
localized string as label/children.
| return ( | ||
| <Tabs.Container | ||
| ref={tabsRef} | ||
| renderTabBar={(tabBarProps) => { | ||
| const handleTabPress = (name: string) => { | ||
| tabBarProps.onTabPress?.(name); | ||
| if (onTabChange) { | ||
| if (name === tabNames.portfolio) { | ||
| onTabChange('portfolio'); | ||
| } else if (name === tabNames.faqs) { | ||
| onTabChange('faqs'); | ||
| } else { | ||
| onTabChange('assets'); | ||
| } | ||
| } | ||
| }; | ||
| return <Tabs.TabBar {...tabBarProps} onTabPress={handleTabPress} />; | ||
| }} | ||
| initialTabName={initialTabName} | ||
| {...containerProps} | ||
| > | ||
| <Tabs.Tab name={tabNames.assets}> | ||
| {isMobile ? ( | ||
| <Tabs.ScrollView refreshControl={refreshControl}> | ||
| <YStack pt="$6" gap="$8"> | ||
| <ProtocolsTabContent /> | ||
| </YStack> | ||
| </Tabs.ScrollView> | ||
| ) : ( | ||
| <YStack> | ||
| <YStack pt="$6" gap="$8"> | ||
| <ProtocolsTabContent /> | ||
| </YStack> | ||
| </YStack> | ||
| )} | ||
| </Tabs.Tab> | ||
| <Tabs.Tab name={tabNames.portfolio}> | ||
| {isMobile ? ( | ||
| <Tabs.ScrollView refreshControl={refreshControl}> | ||
| <YStack pt="$6" gap="$8"> | ||
| <PortfolioTabContent /> | ||
| </YStack> | ||
| </Tabs.ScrollView> | ||
| ) : ( | ||
| <YStack> | ||
| <YStack pt="$6" gap="$8"> | ||
| <PortfolioTabContent /> | ||
| </YStack> | ||
| </YStack> | ||
| )} | ||
| </Tabs.Tab> | ||
| <Tabs.Tab name={tabNames.faqs}> | ||
| {isMobile ? ( | ||
| <Tabs.ScrollView refreshControl={refreshControl}> | ||
| <YStack px="$5" pt="$6" gap="$8"> | ||
| <FAQContent faqList={faqList} isLoading={isFaqLoading} /> | ||
| </YStack> | ||
| </Tabs.ScrollView> | ||
| ) : ( | ||
| <YStack> | ||
| <YStack px="$5" pt="$6" gap="$8"> | ||
| <FAQContent faqList={faqList} isLoading={isFaqLoading} /> | ||
| </YStack> | ||
| </YStack> | ||
| )} | ||
| </Tabs.Tab> | ||
| </Tabs.Container> | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tighten typing of containerProps for better safety.
containerProps?: any hides the actual shape of props passed into Tabs.Container. If this surface is reused, consider introducing a dedicated type (e.g. ITabsContainerProps or ComponentProps<typeof Tabs.Container>) instead of any, so future changes are type‑checked.
🤖 Prompt for AI Agents
In packages/kit/src/views/Earn/components/EarnMainTabs.tsx around lines 74 to
141, containerProps is currently typed as any which hides the actual shape
passed into Tabs.Container; replace the any with a precise type (for example
React.ComponentProps<typeof Tabs.Container> or a dedicated interface like
ITabsContainerProps) in the component props definition, update any imports
(import React or use the existing React import) as needed, and use that typed
containerProps where spread into Tabs.Container so TypeScript will validate
props passed through.
| const banners = useMemo( | ||
| () => ( | ||
| <Stack px="$5"> | ||
| <BannerV2 data={earnBanners} onBannerPress={onBannerPress} /> | ||
| </Stack> | ||
| ), | ||
| [earnBanners, onBannerPress], | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
banners memo is always truthy; condition checks the wrong thing.
banners is a JSX element created in useMemo, so {banners ? (...) : null} will always render the wrapper, even when earnBanners is empty/undefined.
If you only want to show the banner area when there is data, check the data instead, for example:
const hasBanners = !!earnBanners?.length;
...
{hasBanners ? <YStack ...>{banners}</YStack> : null}This keeps the layout cleaner when there are no banners.
🤖 Prompt for AI Agents
In packages/kit/src/views/Earn/EarnHome.tsx around lines 190 to 197, the
memoized variable `banners` is a JSX element so checking `{banners ? ... :
null}` always passes; instead derive a boolean like `hasBanners =
!!earnBanners?.length` and use that for conditional rendering. Update the
conditional to render the banner wrapper only when `hasBanners` is true (and
optionally only build the memoized JSX when there is data), keeping the current
dependencies `[earnBanners, onBannerPress]`.
| // Initialize selectedTabIndex based on defaultTab | ||
| const [selectedTabIndex, setSelectedTabIndex] = useState(() => { | ||
| if (defaultTab === 'withdraw') return 1; | ||
| return 0; | ||
| }); | ||
|
|
||
| // Update selectedTabIndex when defaultTab changes from route | ||
| useEffect(() => { | ||
| if (defaultTab === 'withdraw') { | ||
| setSelectedTabIndex(1); | ||
| } else if (defaultTab === 'deposit') { | ||
| setSelectedTabIndex(0); | ||
| } | ||
| }, [defaultTab]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify tab state management.
You're managing tab state in three places: selectedTabIndex (local), defaultTab (route params), and focusedTab (animation). This creates sync complexity. The useEffect also lacks a default case if defaultTab is neither 'withdraw' nor 'deposit'.
Consider deriving selectedTabIndex directly from route params instead of mirroring it in local state.
🤖 Prompt for AI Agents
In packages/kit/src/views/Staking/pages/ManagePosition/index.tsx around lines
176 to 189, remove the local useState and useEffect that mirror route param
defaultTab and instead derive selectedTabIndex directly from defaultTab (e.g.
const selectedTabIndex = defaultTab === 'withdraw' ? 1 : 0), ensuring you handle
any other/undefined defaultTab by treating it as the deposit (0) or explicit
fallback; keep focusedTab (animation state) separate and only update it where
animations require, and remove the effect that sets selectedTabIndex to avoid
duplicated state and synchronization issues.
| {selectedTabIndex === 0 ? ( | ||
| <StakeSection | ||
| accountId={earnAccount?.account?.id || ''} | ||
| networkId={networkId} | ||
| tokenInfo={tokenInfo} | ||
| protocolInfo={protocolInfo} | ||
| isDisabled={depositDisabled} | ||
| /> | ||
| ) : null} | ||
| {selectedTabIndex === 1 ? ( | ||
| <WithdrawSection | ||
| accountId={earnAccount?.account?.id || ''} | ||
| networkId={networkId} | ||
| tokenInfo={tokenInfo} | ||
| protocolInfo={protocolInfo} | ||
| isDisabled={withdrawDisabled} | ||
| /> | ||
| ) : null} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify empty string is valid for accountId.
Lines 275 and 284 fall back to an empty string when earnAccount?.account?.id is missing. If StakeSection and WithdrawSection expect a valid account ID or undefined, passing '' could cause subtle bugs.
Check if an empty string is acceptable, or pass undefined instead.
🏁 Script executed:
#!/bin/bash
# Check how StakeSection and WithdrawSection handle accountId prop
ast-grep --pattern 'function StakeSection({ $$$, accountId, $$$ }) {
$$$
}'
ast-grep --pattern 'function WithdrawSection({ $$$, accountId, $$$ }) {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find StakeSection and WithdrawSection component files
fd -type f \( -name "*StakeSection*" -o -name "*WithdrawSection*" \) | head -20Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for StakeSection component definition
rg -n "StakeSection" --type=ts --type=tsx -A 5 | head -50Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for WithdrawSection component definition
rg -n "WithdrawSection" --type=ts --type=tsx -A 5 | head -50Length of output: 27
🏁 Script executed:
#!/bin/bash
# Find component files in staking views directory
fd "Stake|Withdraw" packages/kit/src/views/Staking/ | grep -E "\.(ts|tsx)$" | head -20Length of output: 303
🏁 Script executed:
#!/bin/bash
# Search for StakeSection component definition (arrow function or export)
rg -n "StakeSection\s*[:=]|export.*StakeSection" -A 10 packages/kit/src/views/Staking/ | head -60Length of output: 1121
🏁 Script executed:
#!/bin/bash
# Search for WithdrawSection component definition (arrow function or export)
rg -n "WithdrawSection\s*[:=]|export.*WithdrawSection" -A 10 packages/kit/src/views/Staking/ | head -60Length of output: 1157
🏁 Script executed:
#!/bin/bash
# Look for prop interfaces/types for these components
rg -n "interface.*Stake|interface.*Withdraw|type.*Stake|type.*Withdraw" packages/kit/src/views/Staking/ -A 3 | head -40Length of output: 3476
🏁 Script executed:
#!/bin/bash
# Check how accountId is used in StakeSection component
rg -n "accountId" packages/kit/src/views/Staking/pages/ManagePosition/components/StakeSection.tsx -C 3Length of output: 3178
🏁 Script executed:
#!/bin/bash
# Check how accountId is used in WithdrawSection component
rg -n "accountId" packages/kit/src/views/Staking/pages/ManagePosition/components/WithdrawSection.tsx -C 3Length of output: 1220
🏁 Script executed:
#!/bin/bash
# Check parent component for guards on earnAccount before rendering StakeSection/WithdrawSection
rg -n "earnAccount" packages/kit/src/views/Staking/pages/ManagePosition/index.tsx -B 5 -A 1 | grep -E "(earnAccount|selectedTabIndex|StakeSection|WithdrawSection)" | head -40Length of output: 446
🏁 Script executed:
#!/bin/bash
# Check if there's a guard preventing empty accountId scenarios
rg -n "earnAccount\?" packages/kit/src/views/Staking/pages/ManagePosition/index.tsx -C 2 | head -60Length of output: 953
🏁 Script executed:
#!/bin/bash
# Look at the full context around lines 273-290 to see if earnAccount is validated
sed -n '260,295p' packages/kit/src/views/Staking/pages/ManagePosition/index.tsxLength of output: 1196
Pass undefined instead of empty string for missing accountId.
Lines 275 and 284 use earnAccount?.account?.id || '', falling back to empty string. Both StakeSection and WithdrawSection pass accountId directly to API calls like backgroundApiProxy.serviceAccount.getAccount({ accountId }) and useUniversalWithdraw({ accountId }). An empty string satisfies the TypeScript type requirement but causes runtime failures when the API receives it.
Change the fallback to undefined:
accountId={earnAccount?.account?.id}
This lets child components handle the missing ID gracefully or prevents rendering the sections entirely if they can't function without a valid account ID.
🤖 Prompt for AI Agents
In packages/kit/src/views/Staking/pages/ManagePosition/index.tsx around lines
273 to 290, the code currently falls back to an empty string for missing account
IDs (earnAccount?.account?.id || ''), which passes an invalid accountId to child
components and downstream APIs; change the props to pass undefined when the ID
is missing by using earnAccount?.account?.id (remove the || '' fallback) for
both StakeSection and WithdrawSection so children can handle absent IDs or
prevent rendering when an ID is required.
| symbol?: string; | ||
| provider?: string; |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify the redundancy between nested and direct fields.
IDetailPageInfoParams already has protocolInfo and tokenInfo (which contain symbol and provider). Adding symbol and provider as direct optional fields creates redundancy. Which source should consumers prefer? Are both needed?
Verify the usage pattern:
🏁 Script executed:
#!/bin/bash
# Check how routes using IDetailPageInfoParams access symbol/provider
rg -nP --type=ts 'protocolInfo\.symbol|tokenInfo\.symbol|params\.symbol' -g '**/Staking/**' -C2
# Check if the direct fields are used alongside nested objects
rg -nP --type=ts 'IDetailPageInfoParams' -g '**/Staking/**' -A10Length of output: 1712
🏁 Script executed:
#!/bin/bash
# Get full context for EarnActionIcon assignment
rg -nP --type=ts 'protocolInfo\.symbol' packages/kit/src/views/Staking/components/ProtocolDetails/EarnActionIcon.tsx -B10 -A3
# Get full context for InvestmentDetails assignment
rg -nP --type=ts 'symbol: tokenInfo\.symbol' packages/kit/src/views/Staking/pages/InvestmentDetails/index.tsx -B10 -A3
# Search for actual usage of params.symbol or symbol field from route params
rg -nP --type=ts 'params\.symbol|route\.params\.symbol' -g '**/Staking/**' -B2 -A2Length of output: 1224
🏁 Script executed:
# Wait for resultsLength of output: 59
🏁 Script executed:
#!/bin/bash
# Find where route params are consumed - look for useRoute or route.params patterns
rg -nP --type=ts '\buseRoute\b|\broute\.params\b' -g '**/Staking/**' -B2 -A5
# Check what component/page handles EarnProtocolDetails route
rg -nP --type=ts 'EarnProtocolDetails|ETabEarnRoutes\.EarnProtocolDetails' -g '**/Staking/**' -B3 -A3Length of output: 12611
🏁 Script executed:
#!/bin/bash
# Get lines 58-65 from staking.ts to see ManagePosition route definition
sed -n '58,65p' packages/shared/src/routes/staking.tsLength of output: 202
Remove the redundancy concern—the added fields serve a purpose.
The symbol and provider fields aren't redundant. Multiple routes (ProtocolDetails, ProtocolDetailsV2, PortfolioDetails, ManagePosition) access them directly from route params instead of nested objects. This design is intentional.
However, the ManagePosition route definition has a type safety problem. It's missing accountId and indexedAccountId, which the component actually needs (lines 99-100). The component works around this by casting params to any, defeating type checking.
[EModalStakingRoutes.ManagePosition]: {
networkId: string;
symbol: string;
provider: string;
details?: IStakeProtocolDetails;
vault?: string;
tab?: 'deposit' | 'withdraw';
};
Add the missing fields to the type definition or update the component to not access them.
🤖 Prompt for AI Agents
In packages/shared/src/routes/staking.ts around lines 33-34 and the
ManagePosition route definition near lines ~99-100, the ManagePosition route
type is missing accountId and indexedAccountId which the component actually
reads (causing an any cast). Update the ManagePosition route params type to
include accountId: string and indexedAccountId: string (or the correct
optionality if they can be undefined), so the component can access them with
full type safety; alternatively, if those fields truly should not be passed via
route, update the component to obtain them from the correct source instead of
route params.
| [ETabEarnRoutes.EarnProtocols]: { | ||
| symbol: string; | ||
| filterNetworkId?: string; | ||
| logoURI?: string; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider stronger typing for symbol parameter.
The symbol param is currently string, but the codebase uses ISupportedSymbol elsewhere (see line 560 in index.tsx). Consider using that type here for compile-time safety.
Apply this diff if ISupportedSymbol is available at this level:
+import type { ISupportedSymbol } from '@onekeyhq/shared/types/earn';
+
export type ITabEarnParamList = {
...
[ETabEarnRoutes.EarnProtocols]: {
- symbol: string;
+ symbol: ISupportedSymbol;
filterNetworkId?: string;
logoURI?: string;
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/shared/src/routes/tabEarn.ts around lines 14 to 18, the symbol
property is typed as string; change it to use the project's ISupportedSymbol
type for stronger compile-time safety and update any affected call sites. Import
ISupportedSymbol from its declaring module (the path where ISupportedSymbol is
defined in this repo) at the top of the file, replace "symbol: string" with
"symbol: ISupportedSymbol", and ensure the file compiles and tests pass.
…g states and withdraw action handling
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (11)
packages/kit/src/views/Staking/pages/ManagePosition/index.tsx (4)
92-120: Fix the type annotation to include returned fields.Past review flagged this: you return
accountIdandindexedAccountId(lines 111-113), but the type annotation (lines 92-98) omits them. TypeScript will reject this. Add both fields to the type or remove the explicit generic.Apply this diff:
const resolvedParams = useMemo<{ + accountId: string; + indexedAccountId: string | undefined; networkId: string; symbol: ISupportedSymbol; provider: string; vault: string | undefined; isFromShareLink: boolean; }>(() => {
179-192: Simplify tab state management.You mirror
defaultTab(route param) intoselectedTabIndex(local state) viauseEffect. This duplicates state. DeriveselectedTabIndexdirectly fromdefaultTab:const selectedTabIndex = defaultTab === 'withdraw' ? 1 : 0;Keep
focusedTab(animation state) separate.
247-249: Remove theas anycast when callingsetParams.Line 249 bypasses type safety. Ensure
IModalStakingParamList[EModalStakingRoutes.ManagePosition]includes atabfield. Then remove the cast.
307-307: Passundefinedinstead of empty string for missing accountId.Lines 307 and 316 use
earnAccount?.account?.id || ''. An empty string satisfies TypeScript but breaks at runtime when child components call APIs with it. Past review confirmed bothStakeSectionandWithdrawSectionpassaccountIdto background APIs that fail on''.Change both lines:
- accountId={earnAccount?.account?.id || ''} + accountId={earnAccount?.account?.id}Child components must handle
undefinedor prevent rendering whenaccountIdis absent.Also applies to: 316-316
packages/kit/src/views/Staking/components/UniversalWithdraw/index.tsx (2)
541-541: Fix crash when rewards is undefined.
transactionConfirmation?.rewards.map(...)will crash ifrewardsis undefined or null. Optional chaining ontransactionConfirmationisn't enough.Apply this diff:
- {transactionConfirmation?.rewards.map((reward) => { + {transactionConfirmation?.rewards?.map((reward) => {
694-709: Percentage keyboard ignores disabled state on mobile.
PercentageStageOnKeyboardat line 706 remains interactive even whenamountInputDisabledis true. Users can tap percentage buttons, triggeringonSelectPercentageStageand changing the amount despite the disabled state. This breaks the disabled form surface on mobile.Consider passing a disabled prop or conditionally rendering the keyboard:
<PercentageStageOnKeyboard onSelectPercentageStage={onSelectPercentageStage} + disabled={amountInputDisabled} />Or conditionally render it:
- <PercentageStageOnKeyboard - onSelectPercentageStage={onSelectPercentageStage} - /> + {!amountInputDisabled ? ( + <PercentageStageOnKeyboard + onSelectPercentageStage={onSelectPercentageStage} + /> + ) : null}packages/kit/src/views/Earn/pages/EarnProtocolDetails/components/ApyChart.tsx (1)
87-111: Fix hover handling for zero values.The truthy checks at line 99 fail when
timeorpriceequals zero. Legitimate zero-yield data points lose hover feedback.Replace truthy checks with explicit null/undefined checks:
const handleHover = useCallback( ({ time, price, x, y, }: { time?: number; price?: number; x?: number; y?: number; }) => { - if (time && price && x !== undefined && y !== undefined) { + if ( + time !== undefined && + time !== null && + price !== undefined && + price !== null && + x !== undefined && + y !== undefined + ) { setHoverData({ time, apy: price, x, y, }); } else { setHoverData(null); } }, [], );packages/kit/src/views/Earn/components/PortfolioTabContent.tsx (1)
149-176: Remove duplicate keys from child components.Lines 159 and 165 incorrectly apply
key={index}toEarnTextchildren. Only the parentXStack(the direct map child) needs the key.Apply this diff:
{asset.assetsStatus?.map((status, index) => ( <XStack key={index} ai="center"> <EarnText - key={index} mr="$2" size="$bodyLgMedium" text={status.title} /> <EarnText - key={index} mr="$2" size="$bodyLgMedium" color="$textSubdued" text={status.description} /> <EarnTooltip tooltip={status.tooltip} /> </XStack> ))}packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx (2)
370-383: Fix countdown visibility window.Line 373 checks
endTime < now, which only renders the alert after the end time passes. Flip the comparison so it displays while the current time sits betweenstartTimeandendTime.Apply this diff:
{detailInfo?.countDownAlert?.startTime && detailInfo?.countDownAlert?.endTime && now > detailInfo.countDownAlert.startTime && - detailInfo.countDownAlert.endTime < now ? ( + detailInfo.countDownAlert.endTime > now ? ( <YStack pb="$1">
826-844: Use resolved indexedAccountId consistently.Line 826 destructures
indexedAccountIdfromresolvedParams, but line 840 passesindexedAccount?.idfrom theactiveAccounthook instead. This inconsistency means share-link navigation might intend a different account than what's actually used.Apply this diff:
const { accountId, networkId, indexedAccountId, symbol, provider, vault } = resolvedParams; const { earnAccount, detailInfo, tokenInfo, protocolInfo, isLoading, refreshData, refreshAccount, } = useProtocolDetailData({ accountId, networkId, - indexedAccountId: indexedAccount?.id, + indexedAccountId, symbol, provider, vault, });packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts (1)
394-398: Move fetchedKeys.add() inside processResult.Keys are added to
fetchedKeysbefore the fetch resolves. When a position goes to zero,fetchInvestmentDetailreturnsnull, but the key stays infetchedKeys. The final sweep at lines 454-460 won't remove the stale entry, so users still see the position after cashing out.Only mark a key as fetched when we have data:
- const fetchPromises = pairsWithType.map(({ params, isAirdrop }) => { - const key = createInvestmentKey(params); - fetchedKeys.add(key); - return fetchInvestmentDetail(params, isAirdrop, requestId); - }); + const fetchPromises = pairsWithType.map(({ params, isAirdrop }) => { + return fetchInvestmentDetail(params, isAirdrop, requestId); + });And in
processResult:const processResult = ( result: Awaited<ReturnType<typeof fetchInvestmentDetail>>, ) => { if (!result || isRequestStale(requestId)) return; const { key, investment } = result; + fetchedKeys.add(key); // Merge with existing data for the same key
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (19)
packages/shared/src/locale/json/bn.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/de.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/en_US.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/es.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/fr_FR.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/hi_IN.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/id.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/it_IT.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/ja_JP.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/ko_KR.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/pt.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/pt_BR.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/ru.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/th_TH.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/uk_UA.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/vi.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/zh_CN.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/zh_HK.jsonis excluded by!packages/shared/src/locale/json/*.jsonpackages/shared/src/locale/json/zh_TW.jsonis excluded by!packages/shared/src/locale/json/*.json
📒 Files selected for processing (11)
packages/kit/src/views/Earn/components/PortfolioTabContent.tsx(1 hunks)packages/kit/src/views/Earn/hooks/useEarnPortfolio.ts(1 hunks)packages/kit/src/views/Earn/pages/EarnProtocolDetails/components/ApyChart.tsx(1 hunks)packages/kit/src/views/Earn/pages/EarnProtocolDetails/hooks/useProtocolDetailBreadcrumb.ts(1 hunks)packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx(1 hunks)packages/kit/src/views/Staking/components/UniversalWithdraw/index.tsx(10 hunks)packages/kit/src/views/Staking/pages/ManagePosition/hooks/useManagePage.ts(1 hunks)packages/kit/src/views/Staking/pages/ManagePosition/hooks/useProtocolDetails.ts(1 hunks)packages/kit/src/views/Staking/pages/ManagePosition/index.tsx(1 hunks)packages/shared/src/locale/enum/translations.ts(14 hunks)packages/shared/types/staking.ts(11 hunks)
🧰 Additional context used
🪛 ESLint
packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx
[error] 30-36: Import "ETabEarnRoutes" is only used as types.
(@typescript-eslint/consistent-type-imports)
packages/kit/src/views/Staking/components/UniversalWithdraw/index.tsx
[error] 635-637: Replace ⏎··························?·'180deg'⏎························· with ·?·'180deg'
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (8)
packages/shared/src/locale/enum/translations.ts (1)
1-2: Note: File is auto-generated.Changes to this file won't persist. Address naming issues in the source locale files before running
yarn fetch:locale.packages/kit/src/views/Staking/pages/ManagePosition/hooks/useProtocolDetails.ts (1)
60-87: Clean BigNumber handling with proper NaN guard.The balance parsing correctly falls back to '0' when the value is invalid or missing. Dependencies are comprehensive.
packages/kit/src/views/Staking/pages/ManagePosition/hooks/useManagePage.ts (1)
90-114: Good BigNumber handling with NaN guard.Balance parsing (lines 95-96) correctly falls back to '0' for invalid values. Dependencies are comprehensive.
packages/kit/src/views/Staking/components/UniversalWithdraw/index.tsx (3)
237-239: Short-circuit API call when disabled—good.Prevents unnecessary API calls when the component is disabled.
370-372: Clean derivation for input disabled state.Combines
isDisabledandinitialAmountchecks clearly.
670-692: Desktop footer renders inline—clean split.The desktop footer rendering inline with a full-width button is clear and straightforward.
packages/shared/types/staking.ts (1)
697-699: The data field was already optional—no breaking change here.The review comment claims
datachanged from required to optional, but it was already optional (data?: { text: IEarnText; }) in the previous commit. This PR doesn't modify the field. All existing consumers already use optional chaining, so there's no breaking change to address.Likely an incorrect or invalid review comment.
packages/kit/src/views/Earn/pages/EarnProtocolDetails/hooks/useProtocolDetailBreadcrumb.ts (1)
1-101: LGTM!Clean hook implementation with proper error handling and conditional breadcrumb logic. The guard clauses and fallbacks ensure robust behavior.
| {investments.map((item, index) => { | ||
| const showDivider = index < investments.length - 1; | ||
| const key = `${item.protocol.providerDetail.code}_${ | ||
| item.protocol.vaultName || '' | ||
| }_${item.network.networkId}`; | ||
|
|
||
| return ( | ||
| <> | ||
| <PortfolioItem key={key} portfolioItem={item} /> | ||
| {showDivider ? <Divider my="$4" /> : null} | ||
| </> | ||
| ); | ||
| })} | ||
| </YStack> |
There was a problem hiding this comment.
Move key to Fragment.
The map returns a Fragment with PortfolioItem and conditional Divider, but the key is on PortfolioItem instead of the Fragment. This can break React reconciliation.
Apply this diff:
{investments.map((item, index) => {
const showDivider = index < investments.length - 1;
const key = `${item.protocol.providerDetail.code}_${
item.protocol.vaultName || ''
}_${item.network.networkId}`;
return (
- <>
- <PortfolioItem key={key} portfolioItem={item} />
+ <Fragment key={key}>
+ <PortfolioItem portfolioItem={item} />
{showDivider ? <Divider my="$4" /> : null}
- </>
+ </Fragment>
);
})}Add Fragment to imports:
-import { memo, useCallback, useEffect, useMemo } from 'react';
+import { Fragment, memo, useCallback, useEffect, useMemo } from 'react';📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {investments.map((item, index) => { | |
| const showDivider = index < investments.length - 1; | |
| const key = `${item.protocol.providerDetail.code}_${ | |
| item.protocol.vaultName || '' | |
| }_${item.network.networkId}`; | |
| return ( | |
| <> | |
| <PortfolioItem key={key} portfolioItem={item} /> | |
| {showDivider ? <Divider my="$4" /> : null} | |
| </> | |
| ); | |
| })} | |
| </YStack> | |
| {investments.map((item, index) => { | |
| const showDivider = index < investments.length - 1; | |
| const key = `${item.protocol.providerDetail.code}_${ | |
| item.protocol.vaultName || '' | |
| }_${item.network.networkId}`; | |
| return ( | |
| <Fragment key={key}> | |
| <PortfolioItem portfolioItem={item} /> | |
| {showDivider ? <Divider my="$4" /> : null} | |
| </Fragment> | |
| ); | |
| })} | |
| </YStack> |
| {investments.map((item, index) => { | |
| const showDivider = index < investments.length - 1; | |
| const key = `${item.protocol.providerDetail.code}_${ | |
| item.protocol.vaultName || '' | |
| }_${item.network.networkId}`; | |
| return ( | |
| <> | |
| <PortfolioItem key={key} portfolioItem={item} /> | |
| {showDivider ? <Divider my="$4" /> : null} | |
| </> | |
| ); | |
| })} | |
| </YStack> | |
| import { Fragment, memo, useCallback, useEffect, useMemo } from 'react'; |
🤖 Prompt for AI Agents
In packages/kit/src/views/Earn/components/PortfolioTabContent.tsx around lines
700 to 713, the key is applied to the PortfolioItem instead of the surrounding
Fragment which breaks reconciliation; import Fragment from 'react' (or use
React.Fragment), move the computed key onto the Fragment (e.g. <Fragment
key={key}> ... </Fragment>), and remove the key prop from PortfolioItem so the
list item key is applied to the Fragment that wraps PortfolioItem and the
conditional Divider.
| import { | ||
| EModalReceiveRoutes, | ||
| EModalRoutes, | ||
| EModalStakingRoutes, | ||
| ETabEarnRoutes, | ||
| ETabRoutes, | ||
| } from '@onekeyhq/shared/src/routes'; |
There was a problem hiding this comment.
Use type-only import.
ETabEarnRoutes is only used as a type. Use a type-only import to clarify intent and potentially improve tree-shaking.
Apply this diff:
import { ETranslations } from '@onekeyhq/shared/src/locale';
-import type { ITabEarnParamList } from '@onekeyhq/shared/src/routes';
+import type {
+ ITabEarnParamList,
+ ETabEarnRoutes,
+} from '@onekeyhq/shared/src/routes';
import {
EModalReceiveRoutes,
EModalRoutes,
EModalStakingRoutes,
- ETabEarnRoutes,
ETabRoutes,
} from '@onekeyhq/shared/src/routes';Based on learnings
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 30-36: Import "ETabEarnRoutes" is only used as types.
(@typescript-eslint/consistent-type-imports)
🤖 Prompt for AI Agents
packages/kit/src/views/Earn/pages/EarnProtocolDetails/index.tsx around lines 30
to 36, ETabEarnRoutes is imported but only used as a type; change the import to
a type-only import (e.g., use "import type { ETabEarnRoutes } from '...'" or
split it out as an import type line) so the intent is explicit and to aid
tree-shaking, keeping other value imports unchanged.
| open && !isAccordionTriggerDisabled | ||
| ? '180deg' | ||
| : '0deg' |
There was a problem hiding this comment.
Fix prettier formatting.
ESLint reports a formatting issue with the ternary operator.
Apply this diff:
rotate={
- open && !isAccordionTriggerDisabled
- ? '180deg'
- : '0deg'
+ open && !isAccordionTriggerDisabled ? '180deg' : '0deg'
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| open && !isAccordionTriggerDisabled | |
| ? '180deg' | |
| : '0deg' | |
| rotate={ | |
| open && !isAccordionTriggerDisabled ? '180deg' : '0deg' | |
| } |
🧰 Tools
🪛 ESLint
[error] 635-637: Replace ⏎··························?·'180deg'⏎························· with ·?·'180deg'
(prettier/prettier)
🤖 Prompt for AI Agents
In packages/kit/src/views/Staking/components/UniversalWithdraw/index.tsx around
lines 635 to 637, the ternary expression for the rotation value is misformatted
by Prettier; rewrite the expression to meet Prettier rules (either keep the
whole ternary on one line or break lines consistently with surrounding
JSX/props), e.g. place the condition, question-mark, consequent and colon on the
same line or wrap the ternary in parentheses so Prettier can format it without
introducing awkward line breaks; update the file accordingly to satisfy
ESLint/Prettier.
| const protocolInfo: IProtocolInfo | undefined = useMemo(() => { | ||
| if (!managePageData) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // Find the matching protocol from protocol list | ||
| const matchingProtocol = protocolList?.find( | ||
| (item: IStakeProtocolListItem) => | ||
| item.provider.name.toLowerCase() === provider.toLowerCase() && | ||
| item.network.networkId === networkId && | ||
| (!vault || item.provider.vault === vault), | ||
| ); | ||
|
|
||
| // Get withdraw action from managePageData | ||
| const withdrawAction = managePageData.withdraw as | ||
| | IEarnWithdrawActionIcon | ||
| | undefined; | ||
|
|
||
| return { | ||
| symbol, | ||
| provider, | ||
| vault, | ||
| networkId, | ||
| earnAccount, | ||
| activeBalance: managePageData.withdraw?.data?.balance, | ||
| stakeTag: buildLocalTxStatusSyncId({ | ||
| providerName: provider, | ||
| tokenSymbol: symbol, | ||
| }), | ||
| providerDetail: { | ||
| name: matchingProtocol?.provider.name || provider, | ||
| logoURI: matchingProtocol?.provider.logoURI || '', | ||
| }, | ||
| // withdraw | ||
| withdrawAction, | ||
| overflowBalance: managePageData.nums?.overflow, | ||
| maxUnstakeAmount: managePageData.nums?.maxUnstakeAmount, | ||
| minUnstakeAmount: managePageData.nums?.minUnstakeAmount, | ||
| // staking | ||
| minTransactionFee: managePageData.nums?.minTransactionFee, | ||
| remainingCap: managePageData.nums?.remainingCap, | ||
| // claim | ||
| claimable: managePageData.nums?.claimable, | ||
| // approve | ||
| approve: managePageData.approve | ||
| ? { | ||
| allowance: managePageData.approve.allowance, | ||
| approveType: managePageData.approve.approveType as any, | ||
| approveTarget: managePageData.approve.approveTarget, | ||
| } | ||
| : undefined, | ||
| } as IProtocolInfo; | ||
| }, [ | ||
| managePageData, | ||
| protocolList, | ||
| symbol, | ||
| provider, | ||
| vault, | ||
| networkId, | ||
| earnAccount, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the as any cast for approveType.
Line 163 uses as any to force the type. If managePageData.approve.approveType doesn't match IProtocolInfo['approve']['approveType'], fix the type definition or add proper type mapping. Don't bypass the type system.
| vault: string | undefined; | ||
| isFromShareLink: boolean; | ||
| }>(() => { | ||
| const routeParams = route.params as any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the as any cast on route params.
as any disables type checking. If routeParams needs all these fields, define the correct type for EModalStakingRoutes.ManagePosition in your route param list.
🤖 Prompt for AI Agents
packages/kit/src/views/Staking/pages/ManagePosition/index.tsx around line 99:
the code currently uses "const routeParams = route.params as any", which
disables type checking; remove the "as any" cast and instead provide the correct
typed params for the ManagePosition route by adding or updating the route param
type for EModalStakingRoutes.ManagePosition in your route param list (declare
the shape of params there) and then type route.params appropriately (use the
route prop generic or a RouteProp mapped to that route key) so routeParams is
strongly typed and all fields are type-checked.
| id_refer_a_friend_desc = 'id.refer_a_friend_desc', | ||
| identical_name_asset_alert = 'identical_name_asset_alert', | ||
| import_backup_password_desc = 'import_backup_password_desc', | ||
| import_hardware_phrases_warning = 'import_hardware_phrases_warning', |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Check plural usage: "phrases" vs "phrase".
import_hardware_phrases_warning uses plural "phrases". Standard terminology is "recovery phrase" (singular). Verify if plural is intentional.
| export interface IEarnWithdrawActionData { | ||
| type: 'withdraw' | 'withdrawOrder'; | ||
| disabled: boolean; | ||
| text: IEarnText; | ||
| data?: { | ||
| balance?: string; | ||
| token?: { | ||
| info: IEarnToken; | ||
| price: string; | ||
| }; | ||
| text?: IEarnText; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using enum for action type.
Line 716 uses string literal union 'withdraw' | 'withdrawOrder', but EStakingActionType already defines these values. Using the enum improves type safety and maintainability.
Apply this change:
-export interface IEarnWithdrawActionData {
- type: 'withdraw' | 'withdrawOrder';
+export interface IEarnWithdrawActionData {
+ type: EStakingActionType.Withdraw | EStakingActionType.WithdrawOrder;
disabled: boolean;
text: IEarnText;📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface IEarnWithdrawActionData { | |
| type: 'withdraw' | 'withdrawOrder'; | |
| disabled: boolean; | |
| text: IEarnText; | |
| data?: { | |
| balance?: string; | |
| token?: { | |
| info: IEarnToken; | |
| price: string; | |
| }; | |
| text?: IEarnText; | |
| }; | |
| } | |
| export interface IEarnWithdrawActionData { | |
| type: EStakingActionType.Withdraw | EStakingActionType.WithdrawOrder; | |
| disabled: boolean; | |
| text: IEarnText; | |
| data?: { | |
| balance?: string; | |
| token?: { | |
| info: IEarnToken; | |
| price: string; | |
| }; | |
| text?: IEarnText; | |
| }; | |
| } |
🤖 Prompt for AI Agents
In packages/shared/types/staking.ts around lines 715 to 727, the
IEarnWithdrawActionData.type property uses the string literal union 'withdraw' |
'withdrawOrder' but EStakingActionType already defines these values; change the
property to use EStakingActionType instead (e.g. type: EStakingActionType),
import EStakingActionType at the top of the file if needed, and update any
nearby code or callers to reference the enum members
(EStakingActionType.withdraw / EStakingActionType.withdrawOrder) as required to
satisfy types.
| approve?: { | ||
| allowance: string; | ||
| approveType: string; | ||
| approveTarget: string; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type inconsistency: approveType should use enum.
Line 737 uses approveType: string but EApproveType enum exists (line 71-74). Use the enum for type safety.
Apply this change:
approve?: {
allowance: string;
- approveType: string;
+ approveType: EApproveType;
approveTarget: string;
};📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| approve?: { | |
| allowance: string; | |
| approveType: string; | |
| approveTarget: string; | |
| }; | |
| approve?: { | |
| allowance: string; | |
| approveType: EApproveType; | |
| approveTarget: string; | |
| }; |
🤖 Prompt for AI Agents
In packages/shared/types/staking.ts around lines 735 to 739, the approve object
declares approveType as a string but the EApproveType enum is defined earlier
(lines 71–74); change the approveType property type from string to EApproveType
and update any local usages to pass EApproveType members (or cast appropriately)
so the type aligns with the existing enum.
| buttons: { | ||
| type: string; | ||
| text: { | ||
| text: string; | ||
| }; | ||
| disabled: boolean; | ||
| }[]; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Loose typing: buttons[].type should be more specific.
Line 1210 uses generic string for button type, but specific button types exist (EStakingActionType, EClaimType). This loses type safety.
Consider creating a union of valid button type strings or referencing existing enums:
buttons: {
- type: string;
+ type: EStakingActionType | EClaimType | 'history';
text: {
text: string;
};
disabled: boolean;
}[];📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| buttons: { | |
| type: string; | |
| text: { | |
| text: string; | |
| }; | |
| disabled: boolean; | |
| }[]; | |
| buttons: { | |
| type: EStakingActionType | EClaimType | 'history'; | |
| text: { | |
| text: string; | |
| }; | |
| disabled: boolean; | |
| }[]; |
🤖 Prompt for AI Agents
In packages/shared/types/staking.ts around lines 1209 to 1215, the
buttons[].type property is declared as a loose string; replace it with a precise
union referencing the existing enums (e.g. EStakingActionType | EClaimType) or
an explicit union of valid string literals so the compiler enforces allowed
button types; update the import/exports to include those enums if needed and
adjust any call sites to satisfy the new type (or add type assertions where a
literal is guaranteed).
| export interface IApyHistoryResponse { | ||
| code: number; | ||
| message: string; | ||
| data: IApyHistoryItem[]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mixes API response structure with domain types.
IApyHistoryResponse includes code and message fields typical of API responses. Domain types should be separate from transport wrappers. Consider unwrapping at the service layer and keeping just IApyHistoryItem[].
Alternative structure:
// Domain type
export type IApyHistory = IApyHistoryItem[];
// Or if response wrapper is needed, make it generic:
export interface IApiResponse<T> {
code: number;
message: string;
data: T;
}
// Then use: IApiResponse<IApyHistoryItem[]>🤖 Prompt for AI Agents
In packages/shared/types/staking.ts around lines 1433 to 1437, the
IApyHistoryResponse type mixes transport-layer fields (code, message) with
domain data; extract the domain type and keep transport wrappers separate by
replacing IApyHistoryResponse with a domain-only type (e.g., export type
IApyHistory = IApyHistoryItem[]) and, if a response wrapper is needed, add a
generic IApiResponse<T> (export interface IApiResponse<T> { code: number;
message: string; data: T; }) and update callers to unwrap
IApiResponse<IApyHistory> at the service layer so domain layers only consume
IApyHistory.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/kit/src/views/Earn/EarnHome.tsx (2)
203-210: Tidy up tab header height logic and spacer usageThe tab header height handling is a bit confusing:
- The comment says “Use the actual measured height without arbitrary adjustments” but you still subtract
20.- An empty
<Stack h={tabPageHeight} />is used only as a spacer for the absolutely positioned header.To make this clearer and less brittle:
- Extract the
20into a named constant (e.g.TAB_HEADER_OFFSET) or drop it if not needed.- Either:
- Move the spacer behavior into the header/container styles (padding/margin/top offset), or
- Replace the empty
Stackwith a small named component (e.g.<TabHeaderSpacer height={tabPageHeight} />) so intent is explicit.This will make the layout easier to reason about and maintain.
Also applies to: 226-285
191-198: Use banner data to drive rendering, not the JSX element
bannersis always a JSX element fromuseMemo, so checks like{banners ? (...) : null}will always render the wrapper, even whenearnBannersis empty/undefined. That was flagged before and still applies.Consider tracking a
hasBannersflag and only building/rendering when you actually have data:- const banners = useMemo( - () => ( - <Stack px="$5"> - <BannerV2 data={earnBanners} onBannerPress={onBannerPress} /> - </Stack> - ), - [earnBanners, onBannerPress], - ); + const hasBanners = Array.isArray(earnBanners) && earnBanners.length > 0; + + const banners = useMemo( + () => + !hasBanners ? null : ( + <Stack px="$5"> + <BannerV2 data={earnBanners} onBannerPress={onBannerPress} /> + </Stack> + ), + [earnBanners, hasBanners, onBannerPress], + );Then use
hasBannersfor the wrappers:- {banners ? ( + {hasBanners ? ( @@ - {banners ? ( + {hasBanners ? (This keeps the layout clean when no banners exist.
Also applies to: 253-263, 306-318
packages/kit/src/views/Earn/components/Overview.tsx (1)
125-192: Guard rebate lists before mapping to avoid runtime crashesYou still call
.maponrebateData?.distributedandrebateData?.undistributed. If either list isnull/undefined, this will throw and break the popover. Also,needDividershould use the same guarded arrays.Consider normalizing the arrays once after the null check and using those everywhere:
if (!rebateData) { return null; } + + const distributedItems = rebateData.distributed ?? []; + const undistributedItems = rebateData.undistributed ?? []; return ( @@ - {isEmpty(rebateData?.distributed) ? null : ( + {isEmpty(distributedItems) ? null : ( @@ - {rebateData?.distributed.map((item, index) => { + {distributedItems.map((item, index) => { const needDivider = - index === rebateData.distributed.length - 1 && - !isEmpty(rebateData?.undistributed); + index === distributedItems.length - 1 && + !isEmpty(undistributedItems); @@ - {isEmpty(rebateData?.undistributed) ? null : ( + {isEmpty(undistributedItems) ? null : ( @@ - {rebateData?.undistributed.map((item, index) => { + {undistributedItems.map((item, index) => {This keeps rendering safe even when the API omits one of the lists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/kit/src/views/Earn/EarnHome.tsx(8 hunks)packages/kit/src/views/Earn/components/Overview.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/kit/src/views/Earn/components/Overview.tsx (1)
214-316: Overview data flow and refresh wiring look consistentAccount/earn state lookup, rebate fetch, and refresh logic (including portfolio refresh via the event bus) are wired cleanly and respect loading flags. The conditional
shouldShowReferralBonusprevents rendering the popover until data is ready.No change needed here.
Also applies to: 318-429
packages/kit/src/views/Earn/EarnHome.tsx (1)
333-359: Provider wiring and native navigation redirection look coherent
EarnHomeWithProvidernow cleanly threadsdefaultTabviaoverrideDefaultTab, and uses the earn Jotai store explicitly. The native redirect hook (useNavigateToNativeEarnPage) also passes the current tab param through to Discovery, which keeps tab state consistent across surfaces.No changes needed here.
Also applies to: 361-399
| const itemRender = useCallback( | ||
| ({ | ||
| children, | ||
| key, | ||
| needDivider, | ||
| }: { | ||
| children: React.ReactNode; | ||
| key: string | number; | ||
| needDivider?: boolean; | ||
| }) => { | ||
| return ( | ||
| <> | ||
| <ListItem | ||
| my="$2" | ||
| key={key} | ||
| ai="center" | ||
| jc="space-between" | ||
| borderWidth="$0" | ||
| > | ||
| {children} | ||
| </ListItem> | ||
| {needDivider ? <Divider mx="$5" my="$2.5" /> : null} | ||
| </> | ||
| ); | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Move the list key to the top‑level element returned from map
itemRender returns a fragment (<>...</>) but applies key to the inner ListItem. React expects the key on the top‑level element returned by .map, so this will trigger key warnings.
Wrap the row + divider in a single container and put key there, e.g.:
- const itemRender = useCallback(
- ({
- children,
- key,
- needDivider,
- }: {
- children: React.ReactNode;
- key: string | number;
- needDivider?: boolean;
- }) => {
- return (
- <>
- <ListItem
- my="$2"
- key={key}
- ai="center"
- jc="space-between"
- borderWidth="$0"
- >
- {children}
- </ListItem>
- {needDivider ? <Divider mx="$5" my="$2.5" /> : null}
- </>
- );
- },
- [],
- );
+ const itemRender = useCallback(
+ ({
+ children,
+ key,
+ needDivider,
+ }: {
+ children: React.ReactNode;
+ key: string | number;
+ needDivider?: boolean;
+ }) => (
+ <YStack key={key}>
+ <ListItem
+ my="$2"
+ ai="center"
+ jc="space-between"
+ borderWidth="$0"
+ >
+ {children}
+ </ListItem>
+ {needDivider ? <Divider mx="$5" my="$2.5" /> : null}
+ </YStack>
+ ),
+ [],
+ );This removes the React key warning and keeps the layout the same.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const itemRender = useCallback( | |
| ({ | |
| children, | |
| key, | |
| needDivider, | |
| }: { | |
| children: React.ReactNode; | |
| key: string | number; | |
| needDivider?: boolean; | |
| }) => { | |
| return ( | |
| <> | |
| <ListItem | |
| my="$2" | |
| key={key} | |
| ai="center" | |
| jc="space-between" | |
| borderWidth="$0" | |
| > | |
| {children} | |
| </ListItem> | |
| {needDivider ? <Divider mx="$5" my="$2.5" /> : null} | |
| </> | |
| ); | |
| }, | |
| [], | |
| ); | |
| const itemRender = useCallback( | |
| ({ | |
| children, | |
| key, | |
| needDivider, | |
| }: { | |
| children: React.ReactNode; | |
| key: string | number; | |
| needDivider?: boolean; | |
| }) => ( | |
| <YStack key={key}> | |
| <ListItem | |
| my="$2" | |
| ai="center" | |
| jc="space-between" | |
| borderWidth="$0" | |
| > | |
| {children} | |
| </ListItem> | |
| {needDivider ? <Divider mx="$5" my="$2.5" /> : null} | |
| </YStack> | |
| ), | |
| [], | |
| ); |
🤖 Prompt for AI Agents
In packages/kit/src/views/Earn/components/Overview.tsx around lines 62–88, the
returned fragment currently applies the key to the inner ListItem which triggers
React key warnings because the top-level element returned from map must carry
the key; replace the fragment with a single container element (e.g., a
Box/Stack/View or a keyed React.Fragment long form) that wraps the ListItem and
the conditional Divider, move the key prop from ListItem to that container, and
leave the Divider rendering logic unchanged so layout is preserved.
Summary by CodeRabbit
New Features
Improvements