feat: add portfolio price change #1641
Conversation
WalkthroughThe pull request introduces several modifications across multiple components and hooks within the project. Key changes include the addition of a Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
packages/extension-polkagate/src/util/api/getPrices.ts (4)
Line range hint
4-4: Consider removing@ts-nocheckdirective.The file uses TypeScript types but has type checking disabled. This could mask potential type-related issues. Consider enabling type checking and fixing any type errors to improve code reliability.
-//@ts-nocheck
19-20: Add JSDoc comment to document the constant's purpose and unit.The constant would benefit from documentation explaining that it represents hours and its significance in the price change calculation.
+/** Duration in hours for which to fetch price change data from CoinGecko API */ export const COIN_GECKO_PRICE_CHANGE_DURATION = 24;
26-26: Enhance URL construction safety and maintainability.The current URL construction could be improved in terms of safety and maintainability:
- Consider URL-encoding the parameters to prevent injection.
- Extract the base URL as a constant.
+const COINGECKO_API_BASE_URL = 'https://api.coingecko.com/api/v3'; + const prices = await getReq( - `https://api.coingecko.com/api/v3/simple/price?ids=${revisedPriceIds}&vs_currencies=${currencyCode}&include_${COIN_GECKO_PRICE_CHANGE_DURATION}hr_change=true`, + `${COINGECKO_API_BASE_URL}/simple/price?ids=${encodeURIComponent(revisedPriceIds)}&vs_currencies=${encodeURIComponent(currencyCode)}&include_${COIN_GECKO_PRICE_CHANGE_DURATION}hr_change=true`, {} );
Line range hint
22-35: Add error handling and response validation.The current implementation lacks proper error handling and response validation:
- API requests could fail and should be handled gracefully.
- The response format should be validated before use.
Consider implementing the following improvements:
interface CoinGeckoResponse { [key: string]: { [currency: string]: number; [`${string}_24h_change`]?: number; }; } export default async function getPrices(priceIds: string[], currencyCode = 'usd'): Promise<PricesType> { try { console.log(' getting prices for:', priceIds.sort()); const revisedPriceIds = priceIds.map((item) => (EXTRA_PRICE_IDS[item] || item)); const prices = await getReq<CoinGeckoResponse>(`...`); if (!prices || typeof prices !== 'object') { throw new Error('Invalid response from CoinGecko API'); } const outputObjectPrices: PricesType = {}; for (const [key, value] of Object.entries(prices)) { const priceValue = value[currencyCode]; const changeValue = value[`${currencyCode}_24h_change`]; if (typeof priceValue !== 'number' || (changeValue !== undefined && typeof changeValue !== 'number')) { console.warn(`Invalid price data for ${key}`); continue; } outputObjectPrices[key] = { change: changeValue ?? 0, value: priceValue }; } return { currencyCode, date: Date.now(), prices: outputObjectPrices }; } catch (error) { console.error('Failed to fetch prices:', error); throw error; } }packages/extension-polkagate/src/hooks/useYouHave.ts (1)
Line range hint
41-68: Consider performance optimization for large portfoliosThe current implementation recalculates prices and changes for all assets on every render. For large portfolios, consider:
- Memoizing individual asset calculations
- Using a reducer pattern to maintain running totals
- Implementing virtualization if this data is used in a list view
Example approach:
const memoizedAssetCalc = useCallback((asset) => ({ price: calcPrice(prices[asset.priceId]?.value, asset.totalBalance, asset.decimal), change: calcChange(/*...*/) }), [calcPrice, calcChange, prices]); const assetResults = useMemo(() => Object.values(balances).map(memoizedAssetCalc), [balances, memoizedAssetCalc] );packages/extension-polkagate/src/components/FormatPrice.tsx (1)
57-57: Consider grouping related props for better maintainabilityThe function has many parameters that could be logically grouped. Consider creating nested prop interfaces for related properties:
interface StyleProps { fontSize?: string; fontWeight?: number; lineHeight?: number; textAlign?: 'left' | 'right'; textColor?: string; height?: number; width?: string; } interface FormatProps { decimalPoint?: number; decimals?: number; commify?: boolean; sign?: string; } interface Props { amount?: BN | null; num?: number | string; price?: number | null; style?: StyleProps; format?: FormatProps; skeletonHeight?: number; mt?: string; }packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx (1)
203-222: LGTM: Well-implemented price change display.The price change display is well-implemented with:
- Clear visual distinction between positive and negative changes
- Proper formatting using the commify prop
- Helpful time frame context with duration display
- Appropriate color coding for user feedback
However, consider adding a tooltip to explain the meaning of the percentage change to improve user understanding.
Consider adding a tooltip:
+ import { Tooltip } from '@mui/material'; ... - <Typography sx={{ color: youHave.change > 0 ? 'success.main' : 'warning.main', fontSize: '20px', fontWeight: 500 }}> + <Tooltip title={t('Price change in the last {{duration}} hours', { duration: COIN_GECKO_PRICE_CHANGE_DURATION })}> + <Typography sx={{ color: youHave.change > 0 ? 'success.main' : 'warning.main', fontSize: '20px', fontWeight: 500 }}> {youHave.change > 0 ? '+' : '-'} { fixFloatingPoint(youHave?.change, 2, true)} {`(${COIN_GECKO_PRICE_CHANGE_DURATION}h)`} - </Typography> + </Typography> + </Tooltip>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/extension-polkagate/src/components/FormatPrice.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx(4 hunks)packages/extension-polkagate/src/hooks/useYouHave.ts(3 hunks)packages/extension-polkagate/src/util/api/getPrices.ts(1 hunks)
🔇 Additional comments (9)
packages/extension-polkagate/src/util/api/getPrices.ts (1)
19-26: Verify the impact of price change duration across the codebase.
Let's verify that all components using this price data handle the new duration correctly.
✅ Verification successful
Let's examine the actual usage of price change data in these files to verify the impact.
Let's check how the price change data is actually structured in the response by examining the interface or type definitions.
Let's try the search without the file type restriction:
Based on the gathered information, I can now provide a final response:
No impact from price change duration constant
The codebase consistently handles the 24-hour price change duration:
- The interface
PriceValueintypes.tshas a genericchangefield that's independent of the duration - Components like
WatchList.tsxandTotalBalancePieChart.tsxonly display the change value without assuming a specific duration - The duration is only used in two places:
- In the API request URL construction in
getPrices.ts - As a display label in
TotalBalancePieChart.tsx
- In the API request URL construction in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that might be affected by the price change duration
# Look for files using getPrices
echo "Files importing getPrices:"
rg -l "import.*getPrices.*from.*util/api/getPrices"
# Look for components using price change data
echo "\nComponents potentially using price change data:"
rg -l "24h_change|price.*change|change.*price"
Length of output: 506
Script:
#!/bin/bash
# Check how price change data is used in the identified files
echo "Content of useYouHave.ts:"
rg -A 5 -B 5 "24h_change|price.*change|change.*price" packages/extension-polkagate/src/hooks/useYouHave.ts
echo "\nContent of WatchList.tsx:"
rg -A 5 -B 5 "24h_change|price.*change|change.*price" packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/WatchList.tsx
echo "\nContent of Popup/index.tsx:"
rg -A 5 -B 5 "24h_change|price.*change|change.*price" packages/extension-ui/src/Popup/index.tsx
Length of output: 2277
Script:
#!/bin/bash
# Look for type definitions related to price changes
echo "Price-related type definitions:"
rg -A 10 "interface.*Price|type.*Price" --type typescript
echo "\nUsage of COIN_GECKO_PRICE_CHANGE_DURATION:"
rg -A 5 -B 5 "COIN_GECKO_PRICE_CHANGE_DURATION" --type typescript
Length of output: 367
Script:
#!/bin/bash
# Look for type definitions and usage without file type restriction
echo "Price-related type definitions:"
rg -A 10 "interface.*Price|type.*Price"
echo "\nUsage of COIN_GECKO_PRICE_CHANGE_DURATION:"
rg -A 5 -B 5 "COIN_GECKO_PRICE_CHANGE_DURATION"
# Also search for the actual response structure handling
echo "\nHandling of price change response:"
rg -A 5 "usd.*_change|[0-9]+hr_change"
Length of output: 22788
packages/extension-polkagate/src/hooks/useYouHave.ts (3)
13-15: LGTM: Interface update is well-structured
The addition of the change property to YouHaveType is clean and properly typed.
27-30: LGTM: Improved function naming
The rename from calPrice to calcPrice improves code clarity while maintaining correct functionality.
50-50: Verify handling of zero values and edge cases
While the overall logic is sound, we should verify the behavior when dealing with:
- Zero current prices
- Zero balances
- Missing price data
Let's verify the edge cases:
Also applies to: 57-60, 65-67
✅ Verification successful
Based on the search results, I can see the edge case handling in the codebase. Let me analyze the key findings:
- In
useYouHave.ts, zero prices are handled with nullish coalescing:
calcPrice(pricesInCurrencies.prices[asset.priceId]?.value ?? 0, asset.totalBalance, asset.decimal)- In
useTokenPrice.ts, zero/undefined prices are handled:
const maybePriceValue = priceId ? pricesInCurrencies.prices?.[priceId]?.value || 0 : 0;- In
FormatPrice.tsx, price calculations handle undefined/zero values:
if (amount && decimals && price !== undefined) {
return parseFloat(amountToHuman(amount, decimals)) * (price || 0);
}Let me verify the calcChange function implementation to ensure it properly handles division by zero:
Edge cases are properly handled in price calculations
The codebase has robust handling of edge cases:
- Zero prices default to 0 via nullish coalescing:
value ?? 0 - Missing price data handled via optional chaining:
prices?.[priceId]?.value - Division by zero prevented in
calcChange:- Returns 0 for -100% change
- Returns 0 when either price or change is falsy
- Safe division with denominator always > 0 due to
(1 + change/100)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests or handling of edge cases in price calculations
rg -A 5 "test.*Price|price.*0|price.*undefined" --type ts
Length of output: 24309
Script:
#!/bin/bash
# Search for calcChange function implementation
ast-grep --pattern 'function calcChange($_) { $$$ }'
rg -A 5 "calcChange.*=" --type ts
Length of output: 627
packages/extension-polkagate/src/components/FormatPrice.tsx (3)
11-11: LGTM: Import statement is properly organized
The addition of fixFloatingPoint import is well-placed alongside the related amountToHuman utility.
17-17: LGTM: Props interface is well-structured
The commify prop is appropriately marked as optional and follows boolean prop naming conventions.
94-94: Improve readability of the formatting logic
The ternary expression could be simplified for better readability.
- {sign || currency?.sign || ''}{ commify ? fixFloatingPoint(total as number, _decimalPoint, true) : nFormatter(total as number, _decimalPoint)}
+ {sign || currency?.sign || ''}
+ {formatValue(total as number, _decimalPoint)}Add a helper function above the return statement:
const formatValue = (value: number, decimals: number) => {
return commify
? fixFloatingPoint(value, decimals, true)
: nFormatter(value, decimals);
};Let's verify the behavior of both formatting functions with different number ranges:
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx (2)
19-19: LGTM: New imports and utility function changes.
The new imports and utility function modifications align well with the portfolio price change feature. The calPrice function correctly handles balance conversion using amountToHuman.
Also applies to: 22-22, 125-125
197-202: LGTM: Improved layout structure.
The layout changes improve the visual hierarchy and provide better alignment for the portfolio information. The adjustments to Grid alignments and Typography sizes create a more polished look.
| @@ -140,13 +141,13 @@ function TotalBalancePieChart ({ hideNumbers, setGroupedAssets }: Props): React. | |||
| Object.keys(balances).forEach((address) => { | |||
| Object.keys(balances?.[address]).forEach((genesisHash) => { | |||
| if (!TEST_NETS.includes(genesisHash)) { | |||
| //@ts-ignore | |||
| // @ts-ignore | |||
There was a problem hiding this comment.
Consider using a more stable alternative to Object.groupBy.
The Object.groupBy method is an experimental feature that might not be supported in all environments. Consider using a more stable alternative like reduce or lodash.groupBy.
Additionally, the @ts-ignore comments might be hiding potential type issues. Consider addressing the underlying type problems instead.
Here's a suggested implementation using reduce:
- // @ts-ignore
- const groupedAssets = Object.groupBy(allAccountsAssets, ({ genesisHash, token }: { genesisHash: string, token: string }) => `${token}_${genesisHash}`);
+ const groupedAssets = allAccountsAssets.reduce((acc, asset) => {
+ const key = `${asset.token}_${asset.genesisHash}`;
+ acc[key] = acc[key] || [];
+ acc[key].push(asset);
+ return acc;
+ }, {} as Record<string, typeof allAccountsAssets>);Also applies to: 150-150
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/extension-polkagate/src/popup/home/YouHave.tsx (1)
Line range hint
58-66: Consider showing portfolio price change percentage.Since this PR aims to add portfolio price change functionality and the
useYouHavehook now includes achangeproperty, consider enhancing the UI to display the price change percentage alongside the total value. This would provide users with valuable insights into their portfolio performance.Here's a suggested implementation:
<FormatPrice fontSize='38px' fontWeight={500} height={36} num={youHave?.portfolio } skeletonHeight={36} textColor= { isPriceOutdated(youHave) ? 'primary.light' : 'text.primary'} width='223px' /> +{youHave?.change !== undefined && !hideNumbers && + <Typography + sx={{ + color: youHave.change >= 0 ? 'success.main' : 'error.main', + fontSize: '14px', + ml: 1 + }} + > + {(youHave.change >= 0 ? '+' : '') + youHave.change.toFixed(2)}% + </Typography> +}packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/WatchList.tsx (3)
102-102: Simplify nested translationsWhile removing the generic type argument is correct, the nested translation pattern
t(showMore ? t('Less') : t('More'))could be simplified to avoid potential interpolation issues.Consider using a single translation key instead:
-{t(showMore ? t('Less') : t('More'))} +{t(showMore ? 'less' : 'more')}Make sure to add these translation keys to your translation files:
- 'less': "Less"
- 'more': "More"
Line range hint
26-63: Enhance accessibility and semantics of price changesThe price change indicators could be more accessible and semantic.
Consider these improvements:
-{change !== undefined && change > 0 - ? <UpIcon sx={{ color: 'success.main', fontSize: '40px' }} /> - : <DownIcon sx={{ color: 'warning.main', fontSize: '40px' }} /> -} -<Typography color={change !== undefined ? change > 0 ? 'success.main' : 'warning.main' : undefined} fontSize='16px' fontWeight={400} ml='-5px' width='40px'> - {`${(change ?? 0).toFixed(2)}%`} -</Typography> +<Grid + role="status" + aria-label={`Price ${change > 0 ? 'increased' : 'decreased'} by ${(change ?? 0).toFixed(2)}%`} + alignItems="center" + container +> + {change !== undefined && change > 0 + ? <UpIcon sx={{ color: 'success.main', fontSize: '40px' }} aria-hidden="true" /> + : <DownIcon sx={{ color: 'warning.main', fontSize: '40px' }} aria-hidden="true" /> + } + <Typography + component="span" + color={change !== undefined ? change > 0 ? 'success.main' : 'warning.main' : undefined} + fontSize='16px' + fontWeight={400} + ml='-5px' + width='40px' + > + {`${(change ?? 0).toFixed(2)}%`} + </Typography> +</Grid>
Line range hint
65-108: Improve component structure and maintainabilityThe component could benefit from some structural improvements.
- Extract magic numbers as constants:
const INITIAL_VISIBLE_ITEMS = 3;
- Make the show more/less toggle more semantic:
-<Grid alignItems='center' container item onClick={toggleAssets} sx={{ cursor: 'pointer', p: '5px', width: 'fit-content' }}> +<Grid + component="button" + role="button" + aria-expanded={showMore} + alignItems='center' + container + item + onClick={toggleAssets} + sx={{ + cursor: 'pointer', + p: '5px', + width: 'fit-content', + border: 'none', + background: 'none' + }} +>
- Add error boundary:
// Create ErrorBoundary component class WatchListErrorBoundary extends React.Component { // ... error boundary implementation } // Wrap the component export default React.memo((props: Props) => ( <WatchListErrorBoundary> <WatchList {...props} /> </WatchListErrorBoundary> ));packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx (2)
Line range hint
181-191: Consider optimizing asset calculations.The
totalBalancecalculation could be combined with theassetsToShowsorting to avoid iterating over the assets twice.Consider refactoring like this:
const assetsToShow = useMemo(() => { if (!accountAssets || !pricesInCurrencies) { return accountAssets; // null or undefined } else { - const sortedAssets = accountAssets.sort((a, b) => calculatePrice(b.totalBalance, b.decimal, pricesInCurrencies.prices?.[b.priceId]?.value ?? 0) - calculatePrice(a.totalBalance, a.decimal, pricesInCurrencies.prices?.[a.priceId]?.value ?? 0)); + let total = 0; + const sortedAssets = accountAssets + .map(asset => { + const price = calculatePrice(asset.totalBalance, asset.decimal, pricesInCurrencies.prices?.[asset.priceId]?.value ?? 0); + total += price; + return { ...asset, calculatedPrice: price }; + }) + .sort((a, b) => b.calculatedPrice - a.calculatedPrice); + + setTotalBalance(total); // Store total in state return sortedAssets.filter((_asset) => !getValue('total', _asset as unknown as BalancesInfo)?.isZero()); } - }, [accountAssets, calculatePrice, pricesInCurrencies]); + }, [accountAssets, calculatePrice, pricesInCurrencies, setTotalBalance]);
Line range hint
165-167: Enhance error handling for price calculations.The
calculatePricefunction should handle potential NaN or infinite values that could occur during price calculations.Consider adding validation:
const calculatePrice = useCallback((amount: BN, decimal: number, price: number) => { - return parseFloat(amountToHuman(amount, decimal)) * price; + const humanAmount = parseFloat(amountToHuman(amount, decimal)); + const calculated = humanAmount * price; + + if (Number.isNaN(calculated) || !Number.isFinite(calculated)) { + console.warn('Invalid price calculation:', { amount: amount.toString(), decimal, price }); + return 0; + } + + return calculated; }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx(5 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/WatchList.tsx(1 hunks)packages/extension-polkagate/src/popup/home/YouHave.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx
🔇 Additional comments (3)
packages/extension-polkagate/src/popup/home/YouHave.tsx (1)
59-59: LGTM: Font size adjustment for visual consistency.
The reduction in font size from 42px to 38px aligns with similar adjustments made in other components, maintaining UI consistency.
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx (2)
96-96: LGTM: Font size adjustment maintains visual consistency.
The font size reduction from 32px to 28px aligns with similar changes in other components, maintaining a consistent visual hierarchy across the application.
Line range hint 103-180: Verify implementation of portfolio price change feature.
While the component handles total balance calculation, I don't see implementation of the portfolio price change feature mentioned in the PR objectives. This might affect the completeness of the feature.
Let's verify if the price change implementation exists in related files:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/extension-polkagate/src/util/utils.ts (2)
43-51: Add input validation to prevent potential issues.The function works correctly but could benefit from input validation to handle edge cases more robustly.
Consider adding input validation:
function countLeadingZerosInFraction (numStr: string) { + if (typeof numStr !== 'string' || !numStr) { + return 0; + } + const match = numStr.match(/\.(0+)/); if (match) { return match[1].length; } return 0; }
53-54: Add JSDoc to document function parameters.The function has multiple parameters that would benefit from clear documentation.
Consider adding JSDoc:
+/** + * Formats a number with fixed decimal points + * @param _number - The number to format + * @param decimalDigit - Number of decimal places (default: FLOATING_POINT_DIGIT) + * @param commify - Whether to add thousand separators + * @param dynamicDecimal - Whether to adjust decimal places for very small numbers + * @returns Formatted number string + */ export function fixFloatingPoint (_number: number | string, decimalDigit = FLOATING_POINT_DIGIT, commify?: boolean, dynamicDecimal?: boolean): string {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx(5 hunks)packages/extension-polkagate/src/util/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx
🔇 Additional comments (2)
packages/extension-polkagate/src/util/utils.ts (2)
67-73: LGTM! Well-implemented dynamic decimal handling.
The implementation correctly handles very small numbers by dynamically adjusting decimal places while maintaining a reasonable maximum limit.
77-77: LGTM! Proper number formatting implementation.
The use of toLocaleString() is a good choice for reliable thousand separator formatting.
# [0.26.0](v0.25.0...v0.26.0) (2024-11-11) ### Features * add portfolio price change ([#1641](#1641)) ([8e4eacb](8e4eacb))
Summary by CodeRabbit
New Features
commifyprop in theFormatPricecomponent for enhanced price formatting.TotalBalancePieChartcomponent.Improvements
TotalBalancePieChartfor better visual clarity.useYouHavehook to include achangeproperty, providing insights into portfolio performance.Chores