feat/PRO-3182/portfolio-tile-refactor#310
Conversation
WalkthroughThis update introduces a comprehensive wallet portfolio dashboard for the PillarX app, featuring new React components, Redux state management, API services, utility functions, and extensive test coverage. The changes implement a modular architecture for displaying wallet balances, token details, historical graphs, and user controls, while enhancing styling and number formatting consistency across the UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WalletPortfolioTile
participant ReduxStore
participant APIService
User->>WalletPortfolioTile: Open Dashboard
WalletPortfolioTile->>ReduxStore: Select wallet address and filters
WalletPortfolioTile->>APIService: Fetch portfolio, history, PnL data
APIService-->>WalletPortfolioTile: Return data
WalletPortfolioTile->>ReduxStore: Update state with data
WalletPortfolioTile->>WalletPortfolioBalance: Render with balance and change
WalletPortfolioTile->>PrimeTokensBalance: Render with prime token balance
WalletPortfolioTile->>WalletPortfolioGraph: Render graph with period/data type
WalletPortfolioTile->>TopTokens: Render top non-prime tokens
User->>WalletPortfolioButtons: Click "Receive" or "Connect"
WalletPortfolioButtons->>ReduxStore: Dispatch modal or connect actions
User->>WalletPortfolioBalance: Click refresh
WalletPortfolioBalance->>ReduxStore: Dispatch refresh action
WalletPortfolioTile->>APIService: Refetch all data
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Deploying x with
|
| Latest commit: |
59a617a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9ce64e6c.x-e62.pages.dev |
| Branch Preview URL: | https://feat-pro-3182-portfolio-tile.x-e62.pages.dev |
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3182/portfolio-tile-refactor)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (2)
src/apps/pillarx-app/components/WalletConnectDropdown/WalletConnectDropdown.tsx (1)
235-235:⚠️ Potential issueFix onClick handler for disconnectAllSessions button.
The disconnectAllSessions button is using an incorrect onClick handler that doesn't invoke the function.
<button type="button" className="text-[11px] px-2 py-1.5 bg-container_grey border border-container_grey rounded-md" - onClick={() => disconnectAllSessions} + onClick={() => disconnectAllSessions()} >src/services/pillarXApiWalletPortfolio.ts (1)
35-37: 🛠️ Refactor suggestionFragile
chainId.split(':')[1]parsing
contract.chainIdmust contain a colon, otherwisesplit(':')[1]isundefined,Number(undefined)yieldsNaN, andchainIdToChainNameTokensDatamay misbehave.Add a safety check or fallback:
-Number(contract.chainId.split(':')[1]) +Number(contract.chainId.split(':')[1] ?? contract.chainId)(or throw a descriptive error when the format is unexpected).
🧹 Nitpick comments (24)
src/utils/number.tsx (1)
42-69: Consider additional validations for the new utility functionThe
limitDigitsNumberfunction handles various number formatting scenarios well, but it could benefit from additional validations:
- Explicit handling for negative numbers
- Input validation to ensure the input is a valid number
export const limitDigitsNumber = (num: number): number => { // Handle zero or undefined number - if (num === 0 || !num) return 0; + if (num === 0 || !num || isNaN(num)) return 0; + + // Use absolute value for calculations, preserve sign for final result + const isNegative = num < 0; + const absNum = Math.abs(num); // Convert number to string with a large number of decimals to make sure it covers all decimals - const numStr = num.toFixed(20); + const numStr = absNum.toFixed(20); const [integerPart, fractionalPart] = numStr.split('.'); // If integer part is greater than 0 it will show between 2 and 4 decimals - if (parseInt(integerPart) > 0) { - if (parseInt(integerPart) >= 1000) { - return Number(num.toFixed(2)); + if (parseIntLodash(integerPart) > 0) { + if (parseIntLodash(integerPart) >= 1000) { + return Number((isNegative ? -absNum : absNum).toFixed(2)); } - return Number(num.toFixed(4)); + return Number((isNegative ? -absNum : absNum).toFixed(4)); } // If integer part is equal to 0 it will find the position of the first non-zero digit const firstNonZeroIndex = fractionalPart.search(/[1-9]/); // If we do not find 0, return 0 if (firstNonZeroIndex === -1) return 0; // Show up to firstNonZeroIndex + 2-4 significant digits const significantDigits = 4; // Show first non-zero digit plus 3 more (4 significant) const decimalPlaces = firstNonZeroIndex + significantDigits; // Ensure we have at least those digits in the fractional part - return Number(num.toFixed(decimalPlaces)); + return Number((isNegative ? -absNum : absNum).toFixed(decimalPlaces)); };src/apps/pillarx-app/components/TopTokens/TopTokens.tsx (3)
28-30: Fix typo in selector nameThere appears to be a typo in the selector name: "isWalletPorfolioLoading" (missing 't').
- const isWalletPorfolioLoading = useAppSelector( - (state) => state.walletPortfolio.isWalletPorfolioLoading as boolean + const isWalletPortfolioLoading = useAppSelector( + (state) => state.walletPortfolio.isWalletPortfolioLoading as boolean );Also update all references to this variable throughout the component.
160-192: Simplify conditional rendering logicThe conditional rendering for loading, error, and empty states is quite complex and nested. Consider extracting these into separate components or helper functions to improve readability.
You could create helper components like
ErrorStateandEmptyStateto simplify the main component's render logic:// Example simplified approach const ErrorState = ({ isWalletPortfolioErroring, isTopTokenUnrealizedPnLErroring }) => ( <div className="flex flex-col items-center justify-center gap-2"> {isWalletPortfolioErroring && ( <Body className="italic text-percentage_red font-normal"> Failed to load wallet portfolio. </Body> )} {isTopTokenUnrealizedPnLErroring && ( <Body className="italic text-percentage_red font-normal"> Failed to load unrealized PnL. </Body> )} <Body className="italic text-percentage_red font-normal"> Please check your internet connection and reload the page. </Body> </div> ); // Then in the main render: {isError ? ( <ErrorState isWalletPortfolioErroring={isWalletPortfolioErroring} isTopTokenUnrealizedPnLErroring={isTopTokenUnrealizedPnLErroring} /> ) : isLoading ? ( null // or a loading spinner ) : isTopTokensEmpty ? ( <EmptyState /> ) : null}
76-76: Use more reliable unique key for React fragmentsUsing a combination of symbol and index as a key might not be unique if there are multiple tokens with the same symbol on different chains.
-<React.Fragment key={`${token.asset.symbol}-${index}`}> +<React.Fragment key={`${token.contract.address}-${token.contract.chainId}`}>src/apps/pillarx-app/utils/portfolio.ts (1)
16-38: Add equivalent function for PnL resolutionThe file defines two enums (
PeriodFilterBalanceandPeriodFilterPnl) but only includes a resolution mapping function for balance. Consider adding a corresponding function for PnL periods.export const getGraphResolutionPnl = ( filter: PeriodFilterPnl ): string => { switch (filter) { case PeriodFilterPnl.DAY: // every 5 min return '5min'; case PeriodFilterPnl.WEEK: // every hour return '1h'; case PeriodFilterPnl.MONTH: // every 6h return '6h'; case PeriodFilterPnl.YEAR: // every day return '1d'; default: return '1h'; } };src/apps/pillarx-app/components/PrimeTokensBalance/PrimeTokensBalance.tsx (2)
51-52: Consider extracting the repeated condition logic to improve readability.The complex condition
((primeAssetsBalance && primeAssetsBalance === 0) || !primeAssetsBalance)is repeated multiple times in the component. Extract it to a variable for better readability and maintainability.- <img - src={PrimeTokensIcon} - alt="prime-tokens-icon" - className={`w-4 h-4 ${((primeAssetsBalance && primeAssetsBalance === 0) || !primeAssetsBalance) && 'opacity-50'}`} - /> + const isZeroOrUndefined = ((primeAssetsBalance && primeAssetsBalance === 0) || !primeAssetsBalance); + + <img + src={PrimeTokensIcon} + alt="prime-tokens-icon" + className={`w-4 h-4 ${isZeroOrUndefined && 'opacity-50'}`} + />
62-82: Enhance tooltip accessibility for keyboard navigation.The tooltip only responds to mouse events, which excludes keyboard users. Consider adding keyboard accessibility such as focus events and ARIA attributes.
<div className="relative flex items-center w-auto" onMouseEnter={() => setIsHovered(true)} onMouseLeave={() => setIsHovered(false)} + onFocus={() => setIsHovered(true)} + onBlur={() => setIsHovered(false)} + tabIndex={0} + role="button" + aria-label="Information about Prime Tokens" > <img src={PrimeTokensQuestionIcon} alt="prime-tokens-question-icon" className="w-3 h-3 cursor-pointer" /> {isHovered && ( - <div className="absolute bottom-full mb-2 left-1/2 -translate-x-[85%] desktop:-translate-x-1/2 z-10"> + <div + className="absolute bottom-full mb-2 left-1/2 -translate-x-[85%] desktop:-translate-x-1/2 z-10" + role="tooltip" + aria-live="polite" + > <div className="bg-lighter_container_grey text-white text-[10px] italic font-normal px-2.5 py-2 rounded shadow-lg border border-white/[.05] max-w-[270px] w-[calc(100vw-2rem)] break-words"> Prime Tokens are used for trading and paying gas fees across all chains. You'll use them when buying assets and receive them when selling. </div> </div> )} </div>src/services/pillarXApiWalletHistory.ts (1)
33-38: Consider consolidating chain ID mapping logic.There are two separate chain ID mappings in this file. One for the URL query params (lines 33-36) and another for the request body (lines 45-47). Consider consolidating this to avoid potential inconsistencies.
query: ({ wallet, period, from, to }) => { - const chainIds = isTestnet - ? [11155111] - : CompatibleChains.map((chain) => chain.chainId); - const chainIdsQuery = chainIds.map((id) => `chainIds=${id}`).join('&'); + const chainIds = isTestnet ? [11155111] : CompatibleChains.map((chain) => chain.chainId); + const chainIdsString = chainIds.join(','); + const chainIdsQuery = chainIds.map((id) => `chainIds=${id}`).join('&'); return { url: `?${chainIdsQuery}&testnets=${String(isTestnet)}`, method: 'POST', body: { path: 'wallet/history', params: { wallet, - blockchains: CompatibleChains.map((chain) => chain.chainId).join( - ',' - ), + blockchains: chainIdsString, period,src/apps/pillarx-app/hooks/useDataFetchingState.ts (1)
25-38: Add error type checking and refined error handling.The current error handling is binary (either error or no error). Consider refining error handling with appropriate error types and providing more context when dispatching error states.
// Update data and error states useEffect(() => { const isError = !isLoading && !isFetching && !!error; if (data && isSuccess) { dispatch(setData(data)); if (setIsErroring) { dispatch(setIsErroring(false)); } } else if (isError) { dispatch(setData(undefined)); if (setIsErroring) { - dispatch(setIsErroring(true)); + // Optionally pass error details to a more sophisticated error state handler + dispatch(setIsErroring(true)); + // Log for debugging purposes + console.error('Data fetching error:', error); } } }, [src/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraph.tsx (2)
111-134: Reduce duplication between mobile and desktop button implementations.The mobile button implementation repeats logic that's similar to the desktop version. Consider extracting a common button component to reduce duplication.
+// Add this component either in this file (above the main component) or in a separate file +const ToggleButton = ({ + isActive, + onClick, + text +}: { + isActive: boolean; + onClick: () => void; + text: string +}) => ( + <button + type="button" + className={`items-center justify-center w-full rounded-md p-[3px] ${ + isActive ? 'bg-container_grey' : 'bg-[#121116]' + }`} + onClick={onClick} + > + <BodySmall + className={`font-normal ${isActive ? 'text-white' : 'text-white/[.5]'}`} + > + {text} + </BodySmall> + </button> +); // Then in the main component's return: <div className="desktop:hidden flex w-full p-[2px] bg-[#121116] rounded-lg mb-3"> <ToggleButton isActive={selectedBalanceOrPnl === 'balance'} onClick={() => dispatch(setSelectedBalanceOrPnl('balance'))} text="Balance" /> <ToggleButton isActive={selectedBalanceOrPnl === 'pnl'} onClick={() => dispatch(setSelectedBalanceOrPnl('pnl'))} text="PnL" /> </div>
51-89: Consider using an object map for switch cases to simplify logic.The switch statement could be replaced with an object lookup to make the code more maintainable, especially if more time periods are added later.
const handleClickTimePeriod = ( filter: PeriodFilterBalance | PeriodFilterPnl ) => { if (selectedBalanceOrPnl === 'balance') { dispatch(setPeriodFilter(filter as PeriodFilterBalance)); const now = new Date(); - let from; - switch (filter) { - case PeriodFilterBalance.HOUR: - from = sub(now, { hours: 1 }); - break; - case PeriodFilterBalance.DAY: - from = sub(now, { days: 1 }); - break; - case PeriodFilterBalance.WEEK: - from = sub(now, { weeks: 1 }); - break; - case PeriodFilterBalance.MONTH: - from = sub(now, { months: 1 }); - break; - case PeriodFilterBalance.HALF_YEAR: - from = sub(now, { months: 6 }); - break; - default: - from = sub(now, { days: 1 }); - break; - } + const periodMap = { + [PeriodFilterBalance.HOUR]: { hours: 1 }, + [PeriodFilterBalance.DAY]: { days: 1 }, + [PeriodFilterBalance.WEEK]: { weeks: 1 }, + [PeriodFilterBalance.MONTH]: { months: 1 }, + [PeriodFilterBalance.HALF_YEAR]: { months: 6 }, + }; + + const timeOffset = periodMap[filter as PeriodFilterBalance] || { days: 1 }; + const from = sub(now, timeOffset); + dispatch( setPriceGraphPeriod({ from: convertDateToUnixTimestamp(from), to: undefined, }) ); }src/apps/pillarx-app/components/ReceiveModal.tsx (5)
1-2: Consider removing the ESLint disable comment.The ESLint rule
jsx-a11y/no-noninteractive-element-interactionsis disabled at the top of the file. While this allows the use of non-interactive elements with click handlers, it's better to address the underlying accessibility issues directly.Consider using appropriate interactive elements like
<button>instead of disabling the rule, or add proper ARIA attributes if you must use non-interactive elements.
34-41: Simplify useEffect cleanup.The useEffect cleanup logic includes an unnecessary return of
undefinedwhen the condition is not met.useEffect(() => { if (copied) { const timer = setTimeout(() => setCopied(false), 3000); return () => clearTimeout(timer); } - - return undefined; }, [copied]);
94-95: Prevent potential layout shift with onClick on img element.Using
onClickon the image without proper keyboard accessibility can cause layout shifts and accessibility issues.Wrap the image in a button element for better accessibility:
-<img - src={CopyIcon} - alt="copy-evm-address" - className="w-3 h-3.5" - onClick={(e) => e.stopPropagation()} -/> +<button + type="button" + className="p-0 bg-transparent border-0" + onClick={(e) => { + e.stopPropagation(); + }} + aria-label="Copy EVM address" +> + <img + src={CopyIcon} + alt="copy-evm-address" + className="w-3 h-3.5" + /> +</button>
67-99: Add visual feedback for address copy functionality.The component shows a checkmark after copying the address but doesn't provide a screen reader announcement.
Consider adding an ARIA live region to announce successful copy operations:
+const [copyMessage, setCopyMessage] = useState(''); useEffect(() => { if (copied) { + setCopyMessage('Address copied to clipboard'); const timer = setTimeout(() => { setCopied(false); + setCopyMessage(''); }, 3000); return () => clearTimeout(timer); } }, [copied]); // Add this near the end of the component +<div className="sr-only" aria-live="polite"> + {copyMessage} +</div>
101-115: Improve grid layout for better responsiveness.The current grid layout is fixed at 2 columns which might not be optimal for all screen sizes.
Consider using a more responsive grid approach:
-<div className="grid grid-cols-2 gap-4"> +<div className="grid grid-cols-1 sm:grid-cols-2 gap-4">This ensures a single column on very small screens and two columns on larger screens.
src/apps/pillarx-app/components/WalletPortfolioBalance/WalletPortfolioBalance.tsx (5)
33-44: Consolidate isLoading states for cleaner code.Multiple loading states are individually queried from the Redux store and then combined. This could be simplified for better maintainability.
Consider creating a selector in the reducer to combine these loading states:
// In WalletPortfolioSlice.ts +export const selectIsAnyWalletPortfolioDataLoading = (state) => + state.walletPortfolio.isWalletPorfolioLoading || + state.walletPortfolio.isWalletPorfolioWithPnlLoading || + state.walletPortfolio.isWalletHistoryGraphLoading || + state.walletPortfolio.isTopTokenUnrealizedPnLLoading; // In this component +const isAnyDataFetching = useAppSelector(selectIsAnyWalletPortfolioDataLoading); -const isWalletPorfolioLoading = useAppSelector( - (state) => state.walletPortfolio.isWalletPorfolioLoading as boolean -); -const isWalletPorfolioWithPnlLoading = useAppSelector( - (state) => state.walletPortfolio.isWalletPorfolioWithPnlLoading as boolean -); -const isWalletHistoryGraphLoading = useAppSelector( - (state) => state.walletPortfolio.isWalletHistoryGraphLoading as boolean -); -const isTopTokenUnrealizedPnLLoading = useAppSelector( - (state) => state.walletPortfolio.isTopTokenUnrealizedPnLLoading as boolean -); - -const isAnyDataFetching = - isWalletPorfolioLoading || - isWalletPorfolioWithPnlLoading || - isWalletHistoryGraphLoading || - isTopTokenUnrealizedPnLLoading;
66-78: Improve null/zero handling in getUsdChangeText.The function has multiple conditions for handling null/undefined/zero values which could be simplified.
const getUsdChangeText = useCallback(() => { - if ( - !balanceChange || - !balanceChange.usdValue || - balanceChange.usdValue === 0 - ) + if (!balanceChange?.usdValue) return '$0.00'; const absValue = Math.abs(balanceChange.usdValue); return balanceChange.usdValue > 0 ? `+$${limitDigitsNumber(absValue)}` : `-$${limitDigitsNumber(absValue)}`; }, [balanceChange]);
96-102: Extract conditional class logic into helper function.The component has multiple instances of complex ternary expressions for conditional classes.
Consider creating helper functions for common conditional class patterns:
const getBalanceTextColor = (balance) => { return balance > 0 ? 'text-white' : 'text-white text-opacity-50'; }; // Usage <p className={`text-[30px] font-semibold ${getBalanceTextColor(walletPortfolio.total_wallet_balance)}`}>
104-113: Simplify complex conditional class logic.The conditional classes for the balance change text are complex and hard to read.
-<Body - className={`text-base font-normal ${balanceChange.usdValue > 0 && 'text-market_row_green'} ${balanceChange.usdValue < 0 && 'text-percentage_red'} ${getUsdChangeText() === '$0.00' && 'text-white text-opacity-50'} ${balanceChange.usdValue === 0 && 'text-white text-opacity-50 bg-white/[.1]'}`} -> +<Body + className={`text-base font-normal ${ + balanceChange.usdValue > 0 + ? 'text-market_row_green' + : balanceChange.usdValue < 0 + ? 'text-percentage_red' + : 'text-white text-opacity-50' + } ${balanceChange.usdValue === 0 ? 'bg-white/[.1]' : ''}`} +>
152-160: Add aria-disabled attribute to disabled refresh button.The refresh button is visually disabled when data is fetching, but lacks proper accessibility attributes.
<div className={`flex w-fit h-fit items-center justify-center rounded-[10px] border-x-2 border-t-2 border-b-4 border-[#121116] ${isAnyDataFetching ? 'cursor-not-allowed opacity-50' : 'cursor-pointer'}`} + aria-disabled={isAnyDataFetching} + role="button" + tabIndex={isAnyDataFetching ? -1 : 0} onClick={() => { if (!isAnyDataFetching) { dispatch(setIsRefreshAll(true)); } }} >src/apps/pillarx-app/components/WalletPortfolioTile/WalletPortfolioTile.tsx (2)
185-204:useEffectdependency suppression may hide stale-data bugs
// eslint-disable-next-line react-hooks/exhaustive-depssilences the rule that protects against missing dependencies.
In this effect you rely ondispatchandselectedBalanceOrPnl, yet omit them from the dep-array. If either changes whileisRefreshAllis false the effect won’t re-run, potentially leaving the UI out of sync (e.g., a user switches to PnL view and pulls to refresh).Either:
- Add the missing deps and make the callbacks stable with
useCallback, or- Justify in a comment why skipping them is safe (rare).
Sticking with the linter is usually the safer route.
136-139: Possibly wrong endpoint used for Top-Token unrealised PnL
useGetWalletHistoryQueryis invoked a second time to fetchtopTokenUnrealizedPnL.
Unless the history endpoint really returns that data, this is either (a) a copy-paste error or (b) a confusing naming mismatch that will surprise maintainers. Consider creating / using a dedicateduseGetTopTokenUnrealizedPnLQueryhook (or renaming the existing one) for clarity.src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx (1)
317-327: Timestamp unit inconsistency may distort the X-axisBalance history labels (
walletHistoryGraph.balance_history.map(x => x[0])) are used verbatim, while PnL labels are converted* 1000(seconds → ms). Ensure both datasets supply milliseconds; otherwise Chart.js mixes units and the time scale becomes skewed.Consider normalising all incoming timestamps to milliseconds in the RTK-Query transform layer for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
src/apps/pillarx-app/images/prime-tokens-icon.pngis excluded by!**/*.pngsrc/apps/pillarx-app/images/prime-tokens-question-icon.pngis excluded by!**/*.pngsrc/apps/pillarx-app/images/refresh-button.pngis excluded by!**/*.pngsrc/apps/pillarx-app/images/wallet-portfolio-icon.pngis excluded by!**/*.png
📒 Files selected for processing (29)
src/apps/pillarx-app/components/PrimeTokensBalance/PrimeTokensBalance.tsx(1 hunks)src/apps/pillarx-app/components/ReceiveModal.tsx(1 hunks)src/apps/pillarx-app/components/TileContainer/TileContainer.tsx(1 hunks)src/apps/pillarx-app/components/TokenMarketDataRow/RightColumnTokenMarketDataRow.tsx(2 hunks)src/apps/pillarx-app/components/TokenMarketDataRow/TokenLogoMarketDataRow.tsx(2 hunks)src/apps/pillarx-app/components/TokenMarketDataRow/TokenMarketDataRow.tsx(1 hunks)src/apps/pillarx-app/components/TopTokens/TopTokens.tsx(1 hunks)src/apps/pillarx-app/components/WalletConnectDropdown/WalletConnectDropdown.tsx(2 hunks)src/apps/pillarx-app/components/WalletPortfolioBalance/WalletPortfolioBalance.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraph.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraphButton.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioTile/WalletPortfolioTile.tsx(1 hunks)src/apps/pillarx-app/hooks/useDataFetchingState.ts(1 hunks)src/apps/pillarx-app/hooks/useReducerHooks.tsx(1 hunks)src/apps/pillarx-app/index.tsx(2 hunks)src/apps/pillarx-app/reducer/WalletPortfolioSlice.ts(1 hunks)src/apps/pillarx-app/tailwind.pillarx.config.js(1 hunks)src/apps/pillarx-app/utils/constants.ts(1 hunks)src/apps/pillarx-app/utils/portfolio.ts(1 hunks)src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx(1 hunks)src/services/pillarXApiWalletHistory.ts(1 hunks)src/services/pillarXApiWalletPortfolio.ts(4 hunks)src/services/tokensData.ts(1 hunks)src/store.ts(2 hunks)src/theme/index.ts(1 hunks)src/types/api.ts(2 hunks)src/utils/number.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
src/apps/pillarx-app/hooks/useReducerHooks.tsx (1)
src/store.ts (2)
AppDispatch(98-98)RootState(96-96)
src/services/pillarXApiWalletHistory.ts (3)
src/utils/blockchain.ts (2)
isTestnet(29-35)CompatibleChains(260-289)src/types/api.ts (1)
WalletHistoryMobulaResponse(748-750)src/store.ts (1)
addMiddleware(49-59)
src/store.ts (1)
src/apps/pillarx-app/reducer/WalletPortfolioSlice.ts (1)
walletPortfolioSlice(134-153)
src/apps/pillarx-app/components/TokenMarketDataRow/RightColumnTokenMarketDataRow.tsx (1)
src/utils/number.tsx (1)
limitDigitsNumber(42-69)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (2)
src/apps/pillarx-app/hooks/useReducerHooks.tsx (1)
useAppDispatch(5-5)src/apps/pillarx-app/reducer/WalletPortfolioSlice.ts (1)
setIsReceiveModalOpen(128-130)
src/apps/pillarx-app/utils/constants.ts (1)
src/types/api.ts (1)
PrimeAssetType(752-752)
src/apps/pillarx-app/components/PrimeTokensBalance/PrimeTokensBalance.tsx (5)
src/apps/pillarx-app/hooks/useReducerHooks.tsx (1)
useAppSelector(6-6)src/types/api.ts (1)
PortfolioData(725-734)src/services/pillarXApiWalletPortfolio.ts (1)
getPrimeAssetsWithBalances(48-73)src/apps/pillarx-app/utils/constants.ts (1)
PRIME_ASSETS_MOBULA(5-14)src/utils/number.tsx (1)
limitDigitsNumber(42-69)
src/apps/pillarx-app/hooks/useDataFetchingState.ts (2)
src/apps/simpleswap/contexts/InformationContext.jsx (1)
error(37-37)src/apps/pillarx-app/hooks/useReducerHooks.tsx (1)
useAppDispatch(5-5)
src/services/pillarXApiWalletPortfolio.ts (2)
src/types/api.ts (4)
PortfolioData(725-734)PrimeAssetType(752-752)AssetMobula(669-677)ContractsBalanceMobula(650-656)src/services/tokensData.ts (1)
PortfolioToken(23-26)
src/apps/pillarx-app/reducer/WalletPortfolioSlice.ts (2)
src/types/api.ts (3)
PortfolioData(725-734)TokenPriceGraphPeriod(283-286)WalletHistory(742-746)src/utils/common.ts (1)
convertDateToUnixTimestamp(40-41)
🪛 Biome (1.9.4)
src/utils/number.tsx
[error] 1-1: Do not shadow the global "parseInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Cloudflare Pages
🔇 Additional comments (32)
src/theme/index.ts (1)
92-92: Background color update aligned with design refreshThe change from
#1F1D23to#121116makes the app background darker, which aligns with the broader UI styling refresh mentioned in the PR objectives.src/apps/pillarx-app/components/TileContainer/TileContainer.tsx (1)
11-11: Increased border radius for consistent stylingChanging from
rounded-2xltorounded-3xlincreases corner rounding, maintaining visual consistency with other UI refinements in the wallet portfolio implementation.src/apps/pillarx-app/components/TokenMarketDataRow/RightColumnTokenMarketDataRow.tsx (2)
6-8: Added number formatting utility importAdded import for the
limitDigitsNumberutility which will improve numeric value formatting.
27-27: Enhanced price formatting with appropriate decimal precisionApplying
limitDigitsNumberto the price value ensures consistent and readable numeric formatting based on the number's magnitude. This improves user experience by showing appropriate decimal precision (2-4 digits for numbers >= 1, and more significant digits for small fractional values).src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx (1)
109-110:Details
❓ Verification inconclusive
Added isPnl parameter to wallet portfolio query
The addition of
isPnl: falsealigns with the updated API query signature that now controls inclusion of profit and loss data. This ensures consistency with the broader wallet portfolio feature implementation.Verify that this parameter addition doesn't affect existing functionality by running the following script:
🏁 Script executed:
#!/bin/bash # Check if there are any other calls to useGetWalletPortfolioQuery that might need updating rg --type-add 'tsx:*.{tsx,ts}' --type tsx 'useGetWalletPortfolioQuery\(' -A 4 -B 2Length of output: 2275
Ensure
isPnlis applied consistently across all queriesPlease confirm that every
useGetWalletPortfolioQuerycall explicitly includes theisPnlflag:• In
src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsxyou’ve added{ wallet: accountAddress || '', isPnl: false }, { skip: !accountAddress }– this is correct for non-P&L data.
• In
src/apps/pillarx-app/components/WalletPortfolioTile/WalletPortfolioTile.tsxyou’ve similarly updated the “no-P&L” branch:{ wallet: accountAddress || '', isPnl: false }, { skip: !accountAddress }• However, there’s a call using a prebuilt args object:
useGetWalletPortfolioQuery( walletPortfolioWithPnlArgs, { skip: !shouldFetchPnl } );– please verify that
walletPortfolioWithPnlArgsincludesisPnl: trueso P&L data is fetched when expected.src/services/tokensData.ts (1)
23-26: New PortfolioToken type looks good!The extension of
Tokento includeprice_change_24handcross_chain_balanceproperties is well-structured and appropriately typed. Making these fields optional maintains backward compatibility.src/apps/pillarx-app/components/TokenMarketDataRow/TokenMarketDataRow.tsx (1)
27-27: Good use of themed border colorReplacing the hardcoded hex color value with the
border-lighter_container_greyclass improves consistency with your design system and makes future theme changes easier.src/store.ts (2)
14-14: New walletPortfolioSlice import looks goodThe import statement follows the project's established pattern and makes the new slice available for integration.
89-89: Proper integration of walletPortfolioSliceAdding the wallet portfolio reducer to the store correctly integrates the new feature with the application's state management system.
src/apps/pillarx-app/tailwind.pillarx.config.js (2)
11-11: Breakpoint adjustment for desktopThe desktop breakpoint change from 1024px to 1025px is minimal but worth noting. This may help avoid edge cases where devices at exactly 1024px width might receive conflicting styles.
19-19: Color palette refinementThe darkening of
container_greyand addition of new colorsdark_blueandlighter_container_greyprovides more design flexibility. These changes support the updated styling seen in components like TokenMarketDataRow.Also applies to: 26-27
src/apps/pillarx-app/utils/constants.ts (1)
1-14: Comprehensive prime assets constant defined correctlyThe
PRIME_ASSETS_MOBULAconstant is correctly defined with the appropriate type structure imported from the API types. This will provide a consistent reference point for prime asset filtering across the wallet portfolio feature.Consider adding a JSDoc comment to explain the purpose and usage of this constant for improved maintainability.
src/apps/pillarx-app/hooks/useReducerHooks.tsx (1)
1-6: Well-implemented typed Redux hooksThe implementation of typed Redux hooks follows best practices for type safety in React-Redux applications. This pattern ensures that all components using these hooks will benefit from TypeScript's type checking for state and actions.
src/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraphButton.tsx (1)
4-8: Well-structured props type definitionThe props interface is clearly defined with appropriate types for all properties.
src/apps/pillarx-app/components/TokenMarketDataRow/TokenLogoMarketDataRow.tsx (1)
10-11: Great enhancement for flexible logo sizing!The addition of
sizeandchainLogoSizeprops with sensible defaults makes this component more versatile and reusable across different UI contexts. The implementation cleanly integrates with the existing component structure.Also applies to: 18-19, 25-25, 49-51
src/apps/pillarx-app/utils/portfolio.ts (1)
1-14: Well-structured enums for period filtersThe enums are properly defined with clear, descriptive values. The separation between balance and PnL period filters allows for flexibility in UI implementations.
src/apps/pillarx-app/components/PrimeTokensBalance/PrimeTokensBalance.tsx (1)
23-44: Good use of memoization for the balance calculation!The
useMemois appropriately used to calculate the prime assets balance only when the wallet portfolio changes, preventing unnecessary recalculations on re-renders.src/services/pillarXApiWalletHistory.ts (2)
12-22: Good implementation of retry logic for API resilience.The configuration with 5 retries will help ensure the API calls are resilient against temporary network issues.
49-50:Details
❓ Verification inconclusive
Verify the timestamp conversion is as expected by the API.
The conversion from seconds to milliseconds is applied to the timestamps. Ensure this matches the API's expected format, as some APIs use seconds while others use milliseconds.
🏁 Script executed:
#!/bin/bash # Check if the API documentation or other API calls use milliseconds for timestamps rg -A 5 -B 5 "\* 1000" --glob "!{*.json,*.log,*.md}"Length of output: 7413
Confirm timestamp conversion units
The snippet below converts
from/tofrom seconds to milliseconds by multiplying by 1000. Ensure the PillarX API actually expects timestamps in milliseconds—if it expects seconds, this will lead to 1000× inflated values.File:
src/services/pillarXApiWalletHistory.ts
Lines: 49–50- from: from, - to: to ? to : undefined, + from: from * 1000, + to: to ? to * 1000 : undefined,• If the API expects seconds, remove the
* 1000conversion.
• If it expects milliseconds (as insrc/apps/token-atlas/api/token.ts), this is correct.
• Please double-check against the latest PillarX API documentation or example payloads and adjust accordingly.src/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraph.tsx (2)
34-48: Good pattern for conditional time filter selection.The component correctly manages different time filters based on whether balance or PnL is selected, making the UI context-aware.
91-94:Details
❌ Incorrect review comment
Consider handling component cleanup for timers or subscriptions.
The useEffect sets a default time period on mount, but if this component creates any subscriptions or timers, you should add a cleanup function to prevent memory leaks.
Check if this component needs cleanup for any resources it initializes:
🏁 Script executed:
#!/bin/bash # Check for other useEffect hooks with cleanup functions in similar components rg -A 5 "useEffect\(\(\) => \{[\s\S]*return \(\) =>" --glob "*.tsx" src/apps/pillarx-app/components/Length of output: 102
I want to verify that this component truly doesn’t establish any timers, subscriptions, or similar resources before concluding that no cleanup is needed.
🏁 Script executed:
#!/bin/bash # Locate the useEffect in question echo "=== useEffect occurrences ===" rg -C3 "useEffect" src/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraph.tsx # Check for common timer/subscription patterns echo "=== timer/subscription patterns ===" rg -nE "setTimeout|setInterval|subscribe|new\s+WebSocket|EventSource" src/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraph.tsx || trueLength of output: 806
No cleanup required in WalletPortfolioGraph.tsx
We inspected theuseEffectinsrc/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraph.tsxand confirmed it only callshandleClickTimePeriodon mount. There are no timers (setTimeout,setInterval), subscriptions (subscribe), WebSockets, or other resources instantiated that would require a cleanup function. You can safely ignore the generic cleanup suggestion.Likely an incorrect or invalid review comment.
src/apps/pillarx-app/index.tsx (3)
16-16: Updated import structure aligns with component changes.The homeFeed API import has been updated to match the new component architecture. This looks good as it removes the wallet info query dependency.
27-27: Good modularization with WalletPortfolioTile.Adding the WalletPortfolioTile component creates better separation of concerns by encapsulating wallet portfolio functionality.
215-215:Details
❓ Verification inconclusive
Verify all previous functionality is preserved in WalletPortfolioTile.
The
PortfolioOverviewcomponent has been replaced withWalletPortfolioTile. Ensure that all functionality from the previous implementation is preserved in the new component.Run the following script to check for any lost props or functionality:
🏁 Script executed:
#!/bin/bash # Search for the previous PortfolioOverview component usage rg "PortfolioOverview" --type ts --type tsx -A 5 -B 5Length of output: 81
🏁 Script executed:
#!/bin/bash set -e echo "=== References to PortfolioOverview ===" rg "PortfolioOverview" -A5 -B5 || echo "No occurrences of PortfolioOverview found" echo echo "=== Definition of PortfolioOverview ===" rg "export .*PortfolioOverview" -A5 -B5 || rg "function PortfolioOverview" -A5 -B5 || rg "class PortfolioOverview" -A5 -B5 || echo "No definition of PortfolioOverview found" echo echo "=== Definition of WalletPortfolioTile ===" rg "export .*WalletPortfolioTile" -A5 -B5 || rg "function WalletPortfolioTile" -A5 -B5 || rg "class WalletPortfolioTile" -A5 -B5 || echo "No definition of WalletPortfolioTile found"Length of output: 15072
🏁 Script executed:
#!/bin/bash set -e echo "=== PortfolioOverview.tsx (first 200 lines) ===" sed -n '1,200p' src/apps/pillarx-app/components/PortfolioOverview/PortfolioOverview.tsx || true echo echo "=== WalletPortfolioTile.tsx (first 200 lines) ===" sed -n '1,200p' src/apps/pillarx-app/components/WalletPortfolioTile/WalletPortfolioTile.tsx || trueLength of output: 11384
🏁 Script executed:
#!/bin/bash set -e echo "=== return statements in WalletPortfolioTile.tsx ===" rg -n "return" src/apps/pillarx-app/components/WalletPortfolioTile/WalletPortfolioTile.tsx echo echo "=== WalletPortfolioTile.tsx around return (50 lines before and after) ===" rg -n "return" -C50 src/apps/pillarx-app/components/WalletPortfolioTile/WalletPortfolioTile.tsx || trueLength of output: 3891
Verify preserved PortfolioOverview functionality in WalletPortfolioTile
Ensure that the new
WalletPortfolioTilecomponent fully covers every feature previously implemented inPortfolioOverview. In particular:• Loading state
– Is there a skeleton or placeholder shown while data is loading? (Previously viaTileContainer+SkeletonLoader)
• Wallet address and connect dropdown
–<WalletAddressOverview>and<WalletConnectDropdown>behavior matches the old layout
• Total balance & PnL
– Display oftotal_wallet_balanceformatted to two decimals and percentage change (TokensPercentage)
• Tags for tokens & chains
– “X tokens” tag and “across N chain(s)” tag with the correct icons
• Tokens logos list
– Horizontal list of blockchain logos (TokensHorizontalList) when assets exist
• Translations
– All user-facing text still wrapped int\`` calls
• Tests & snapshots
– Equivalent coverage and snapshots exist for the new componentPlease review and confirm that no functionality, UI state, or tests from the removed
PortfolioOverviewhave been omitted or regressed.src/apps/pillarx-app/components/WalletConnectDropdown/WalletConnectDropdown.tsx (6)
7-7: Good use of React Icons.Replacing the static SVG with a dynamic React Icon component improves maintainability and allows for the rotation animation on toggle.
337-338: Updated class structure improves styling consistency.The revised width and height classes create a more consistent component size.
341-342: Improved button styling with consistent border pattern.The updated border styling using border-x-2, border-t-2, and border-b-4 creates a consistent 3D button effect that matches the design system.
348-350: Updated logo size and typography for better visual hierarchy.Reducing the logo size from 24px to 16px and using a smaller typography component creates a more balanced visual hierarchy.
358-362: Improved dropdown toggle indicator with animated rotation.The static arrow SVG has been replaced with a React icon that rotates on toggle, providing better visual feedback to users.
<RiArrowDownSLine color="white" size={20} className={`transform ${isDropdownOpen ? 'rotate-180' : ''}`} />
366-367: Consistent dropdown styling with main button.The dropdown content now uses the same border styling as the button, creating a more cohesive design.
src/types/api.ts (1)
697-704: Good addition of strongly-typedPnLEntry& history bucketsDefining
PnLEntryas a tuple with explicit realised / unrealised keys greatly improves type-safety over the previous[string, number]approach. 👍src/services/pillarXApiWalletPortfolio.ts (1)
156-170:blockchainsbody param diverges fromchainIdsused in the query stringWhen
isTestnetistrueyou restrict the query string to11155111, yet the request body still submits allCompatibleChains(main-net IDs included).
Depending on the backend’s validation this can return empty data or unexpected balances.Consider re-using the same
chainIdsarray:- blockchains: CompatibleChains.map((chain) => chain.chainId).join(','), + blockchains: chainIds.join(','),Please confirm the backend expectation before merging.
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
Show resolved
Hide resolved
src/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraphButton.tsx
Outdated
Show resolved
Hide resolved
src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx
Outdated
Show resolved
Hide resolved
src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx
Show resolved
Hide resolved
src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx
Outdated
Show resolved
Hide resolved
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3182/portfolio-tile-refactor)
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (30)
src/apps/pillarx-app/components/TokensWithMarketDataTile/test/TokensWithMarketDataTile.test.tsx (1)
179-179: Minor typo in test description.There's a small typo in the test description: "rowsd" should be "rows".
- it('renders the right number of rowsd', () => { + it('renders the right number of rows', () => {src/apps/pillarx-app/components/WalletPortfolioBalance/test/WalletPortfolioBalance.test.tsx (6)
1-1: Consider avoiding eslint-disable for no-explicit-anyWhile using
anytype might be convenient in tests, it's generally better to use proper TypeScript interfaces or types that match the expected structure of your data. This improves type safety and documentation.-/* eslint-disable @typescript-eslint/no-explicit-any */ +// Import proper types insteadConsider creating interfaces for the wallet portfolio and PnL data structures, which can be shared between components and tests.
49-51: Replaceanytypes with more specific interfacesUsing
anytype reduces TypeScript's ability to provide type checking and autocompletion.- walletPortfolio?: any; - topTokenUnrealizedPnL?: any; + walletPortfolio?: { + total_wallet_balance: number; + // Add other properties as needed + }; + topTokenUnrealizedPnL?: { + balance_history: [number, number][]; + // Add other properties as needed + };
68-71: Reconsider the snapshot test approachSnapshot tests are prone to frequent changes and don't clearly communicate expected behavior. They're useful for UI components that change infrequently, but can become maintenance overhead.
Consider adding specific assertions that test the component's structure instead, or if keeping the snapshot test, add a comment explaining what specific aspects of the rendering are important to preserve.
150-153: Add a test ID to the refresh button for more reliable testingFinding elements by their alt text and then navigating to parent elements creates fragile tests that can break with HTML structure changes.
// In the component file: -<button className={isLoading ? 'opacity-50' : ''} onClick={handleRefresh} disabled={isLoading}> +<button + data-testid="refresh-button" + className={isLoading ? 'opacity-50' : ''} + onClick={handleRefresh} + disabled={isLoading} +> // Then in the test file: -const button = screen.getByAltText('refresh-button').parentElement; +const button = screen.getByTestId('refresh-button');
152-153: Avoid non-null assertion operatorUsing the non-null assertion operator (
!) bypasses TypeScript's type checking. It's better to use assertions or conditional checks.-fireEvent.click(button!); +// Either ensure button is not null with an assertion +expect(button).not.toBeNull(); +fireEvent.click(button as HTMLElement); + +// Or use a conditional +if (button) { + fireEvent.click(button); +}
73-166: Consider adding tests for additional edge casesThe test suite is quite thorough, but a few edge cases could be added:
- When
balance_historyis empty or has only one entry- When
topTokenUnrealizedPnLexists butbalance_historyis undefined- Accessibility testing (keyboard navigation, screen reader compatibility)
You could add tests like:
it('handles empty balance history gracefully', () => { renderWithSelector({ walletPortfolio: { total_wallet_balance: 1000 }, topTokenUnrealizedPnL: { balance_history: [] } }); // Assert on expected behavior }); it('handles single entry in balance history gracefully', () => { renderWithSelector({ walletPortfolio: { total_wallet_balance: 1000 }, topTokenUnrealizedPnL: { balance_history: [[1, 500]] } }); // Assert on expected behavior });src/apps/pillarx-app/components/TopTokens/test/TopTokens.test.tsx (3)
58-61: Consider adding mock data to the snapshot test.The snapshot test is currently rendering the component without providing any mock data. This could lead to inconsistent snapshots depending on the default state of the component or make the snapshot hard to interpret.
it('renders correctly and matches snapshot', () => { - const tree = render(<TopTokens />); + useAppSelectorMock.mockImplementation((selector) => + selector({ + walletPortfolio: { + walletPortfolio: mockWalletPortfolio, + isWalletPortfolioLoading: false, + isWalletPortfolioErroring: false, + isTopTokenUnrealizedPnLLoading: false, + isTopTokenUnrealizedPnLErroring: false, + }, + }) + ); + + (portfolioService.getTopNonPrimeAssetsAcrossChains as jest.Mock) + .mockReturnValue(mockWalletPortfolio.assets); + + const tree = render(<TopTokens />); expect(tree).toMatchSnapshot(); });
63-78: Consider checking for loading indicators.The test verifies that token data is not shown while loading, but doesn't explicitly check for any loading indicators that should be displayed to users during this state.
If your component displays loading indicators (like spinners or "Loading..." text), consider adding an assertion to verify they appear:
render(<TopTokens />); expect(screen.queryByText(/Unrealized PnL/i)).not.toBeInTheDocument(); +expect(screen.getByTestId('loading-indicator')).toBeInTheDocument();This assumes you have a test ID added to your loading indicator. If not, you might need to add one to the component first.
149-157: Clarify negative PnL display and use test IDs for more precise testing.The comment on line 156 indicates that negative values are shown as absolute values (without the minus sign). This could potentially confuse users if not properly indicated with color or icons in the UI.
Additionally, finding elements by text might be brittle, especially if the same text appears multiple times in the DOM as noted in your comment on line 149.
Consider using test IDs for more precise testing:
// Token info (without some of them because repeated in the DOM but hidden from user) -expect(screen.getByText('USDC')).toBeInTheDocument(); -expect(screen.getByText('USD Coin')).toBeInTheDocument(); -expect(screen.getByText('10.00%')).toBeInTheDocument(); +expect(screen.getByTestId('token-symbol-USDC')).toBeInTheDocument(); +expect(screen.getByTestId('token-name-USDC')).toHaveTextContent('USD Coin'); +expect(screen.getByTestId('token-pnl-USDC')).toHaveTextContent('10.00%'); +// Also verify positive indicator +expect(screen.getByTestId('token-pnl-indicator-USDC')).toHaveClass('positive'); -expect(screen.getByText('ETH')).toBeInTheDocument(); -expect(screen.getByText('Ethereum')).toBeInTheDocument(); -expect(screen.getByText('9.50%')).toBeInTheDocument(); // negative shown as absolute +expect(screen.getByTestId('token-symbol-ETH')).toBeInTheDocument(); +expect(screen.getByTestId('token-name-ETH')).toHaveTextContent('Ethereum'); +expect(screen.getByTestId('token-pnl-ETH')).toHaveTextContent('9.50%'); +// Also verify negative indicator +expect(screen.getByTestId('token-pnl-indicator-ETH')).toHaveClass('negative');This assumes adding appropriate test IDs to the component, which would make tests more robust and clearer.
src/apps/pillarx-app/components/PrimeTokensBalance/test/PrimeTokensBalance.test.tsx (3)
40-47: Consider enhancing the snapshot testWhile snapshot testing is useful for detecting unintended changes, it doesn't provide much value on its own. Consider adding specific assertions to verify key component structure elements.
it('renders correctly and matches snapshot', () => { const tree = render( <Provider store={store}> <PrimeTokensBalance /> </Provider> ); expect(tree).toMatchSnapshot(); + // Add specific structural assertions + expect(screen.getByText(/Prime Tokens Balance:/i)).toBeInTheDocument(); + expect(screen.getByAltText('prime-tokens-icon')).toBeInTheDocument(); });
1-132: Consider adding accessibility testingThe component includes interactive elements like tooltips, but there are no tests for accessibility attributes or keyboard interactions. Consider adding tests for accessibility to ensure the component is usable by all users.
+ it('has proper accessibility attributes for tooltip', () => { + useAppSelectorMock.mockReturnValue(undefined); + + render(<PrimeTokensBalance />); + + const helpIcon = screen.getByAltText('prime-tokens-question-icon'); + expect(helpIcon).toHaveAttribute('role', 'button'); + expect(helpIcon).toHaveAttribute('tabIndex', '0'); + + // Test keyboard interaction + fireEvent.keyDown(helpIcon, { key: 'Enter' }); + expect( + screen.getByText( + /Prime Tokens are used for trading and paying gas fees across all chains/i + ) + ).toBeInTheDocument(); + });
23-35: Consider using Jest's modern mock syntaxThe current mocking approach uses
jest.spyOnwith explicit casting. For cleaner and more type-safe mocks, consider using Jest's newer mock syntax with automatic type inference.- const mockGetPrimeAssetsWithBalances = jest.spyOn( - portfolioService, - 'getPrimeAssetsWithBalances' - ) as jest.Mock; + const mockGetPrimeAssetsWithBalances = jest.fn().mockName('getPrimeAssetsWithBalances'); + jest.spyOn(portfolioService, 'getPrimeAssetsWithBalances').mockImplementation(mockGetPrimeAssetsWithBalances);src/apps/pillarx-app/components/ReceiveModal/test/ReceiveModal.test.tsx (1)
56-67: Verify presence of copy functionality in testsThe test verifies that the modal renders with the correct address, but doesn't test the copy-to-clipboard functionality which is a key feature of the modal.
Consider adding a test for the copy-to-clipboard feature that verifies:
- The copy icon is visible initially
- The check icon appears after clicking the copy button
- The copy icon reappears after the timeout
src/apps/pillarx-app/components/ReceiveModal/ReceiveModal.tsx (5)
1-1: Consider adding explanations for disabled linting rulesWhen disabling ESLint rules, it's helpful to provide a comment explaining why the rule is being disabled.
-/* eslint-disable jsx-a11y/no-noninteractive-element-interactions */ +/* eslint-disable jsx-a11y/no-noninteractive-element-interactions */ +// Disabled because we're using an img tag with onClick for the copy button
25-31: Consider using a more descriptive variable nameThe variable
accountAddresscould be renamed towalletAddressto be more consistent with the EVM address context.
57-62: Consider extracting the ESC button to a reusable componentThe ESC button styling appears to be a common UI element that could be reused across the application.
Consider extracting this into a reusable
EscButtoncomponent, especially if it's used in other modals.
104-118: Add accessibility labels for supported chainsThe chain items lack proper ARIA labels for better screen reader support.
{CompatibleChains.map((chain, index) => ( - <div key={index} className="flex items-center gap-2"> + <div + key={index} + className="flex items-center gap-2" + role="listitem" + aria-label={`${chain.chainName} blockchain`} + > <TokenLogoMarketDataRow tokenLogo={getLogoForChainId(chain.chainId)} size="w-9 h-9" chainLogoSize="w-[14px] h-[14px]" /> <Body className="text-white/[.5] font-normal"> {chain.chainName} </Body> </div> ))}Also, wrap the mapping in a parent div with proper ARIA role:
-<div className="grid grid-cols-2 gap-4"> +<div className="grid grid-cols-2 gap-4" role="list" aria-label="Supported blockchain networks">
48-121: Consider adding keyboard navigation for the modalThe modal can be improved with keyboard navigation and focus trapping for better accessibility.
Consider using a library like
focus-trap-reactto trap focus within the modal when it's open, and add keyboard event handling for the ESC key to close the modal.Here's an example implementation:
// Add to imports +import { useEffect, useRef, useState } from 'react'; +import FocusTrap from 'focus-trap-react'; // Inside the component, add useEffect for keyboard handling +const modalRef = useRef(null); + +useEffect(() => { + const handleKeyDown = (event: KeyboardEvent) => { + if (event.key === 'Escape') { + handleOnCloseReceiveModal(); + } + }; + + window.addEventListener('keydown', handleKeyDown); + return () => { + window.removeEventListener('keydown', handleKeyDown); + }; +}, []); + +// Focus the modal when it opens +useEffect(() => { + if (isReceiveModalOpen && modalRef.current) { + (modalRef.current as HTMLElement).focus(); + } +}, [isReceiveModalOpen]); // In the JSX, wrap with FocusTrap -return ( +return ( + <FocusTrap active={isReceiveModalOpen}> <div id="wallet-portfolio-tile-receive-modal" className="fixed inset-0 bg-[#12111680]/[.5] backdrop-blur-sm z-30" > - <div className="fixed inset-0 flex items-center justify-center z-40 mobile:items-start tablet:items-start mt-20"> + <div + ref={modalRef} + className="fixed inset-0 flex items-center justify-center z-40 mobile:items-start tablet:items-start mt-20" + tabIndex={-1} + >src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx (5)
322-322: Update the chart dataset label to match actual dataThe dataset label "Token price" is misleading since the chart is displaying either wallet balance or PnL data, not token price.
- label: 'Token price', + label: isBalanceGraph ? 'Balance' : 'PnL',
362-363: Remove or update the chart title to match component purposeThe chart title is set to "Token price history" but the component is displaying wallet balance or PnL history.
- display: true, - text: 'Token price history', + display: false,
469-469: Use full width for chart container for better displayUsing 99% width seems like a workaround. Consider using 100% width if possible, or add a comment explaining why 99% is needed.
- className="flex w-[99%] h-full rounded-b-xl mobile:mb-0" + className="flex w-full h-full rounded-b-xl mobile:mb-0"
340-420: Extract chart options to a useMemo or extract constant valuesThe chart options object is recreated on every render. Consider using useMemo for dynamic parts or extracting static configurations outside the component.
The chart options are complex and re-created on every render. Consider using useMemo to optimize performance:
+ const graphOptions = useMemo(() => ({ onHover: (event, chartElements) => { if (chartElements.length > 0 && (hasBalanceData || hasPnlData)) { const { datasetIndex, index } = chartElements[0]; const dataset = data.datasets[datasetIndex]; try { const dataPoint = dataset.data[index]; const value = typeof dataPoint === 'object' && dataPoint !== null ? (dataPoint as {y: number}).y : dataPoint as number; setHoverValue(value); } catch (error) { console.error('Error parsing hover value:', error); setHoverValue(undefined); } } else { setHoverValue(undefined); } }, // Rest of the options... - }); + }), [graphXScale, hasBalanceData, hasPnlData, getGraphStepSize]); + + // Then use 'graphOptions' instead of 'options' when rendering the Line component
457-465: Improve accessibility by using semantic HTML elements for the value displayThe current implementation uses generic spans for displaying value information. Consider using more semantic elements.
- <Body> - <span className="font-normal text-white/[.5] pl-3.5"> - Total Value:{' '} - </span> - <span - className={`font-normal ${hoverValue ? 'text-white' : 'text-white/[.5]'}`} - > - {hoverValue ? `$${limitDigitsNumber(hoverValue)}` : '$0.00'} - </span> - </Body> + <Body> + <label className="font-normal text-white/[.5] pl-3.5"> + Total Value:{' '} + <output + className={`font-normal ${hoverValue ? 'text-white' : 'text-white/[.5]'}`} + > + {hoverValue ? `$${limitDigitsNumber(hoverValue)}` : '$0.00'} + </output> + </label> + </Body>src/apps/pillarx-app/components/WalletPortfolioGraph/tests/WalletPortfolioGraph.test.tsx (2)
77-80: Add content verification to the snapshot testThe snapshot test only verifies rendering without errors but doesn't check for specific content. Consider adding assertions for key elements.
it('renders correctly and matches snapshot', () => { const tree = render(<WalletPortfolioGraph />); + expect(screen.getByTestId('balance-graph')).toBeInTheDocument(); + expect(screen.getByTestId('btn-Balance')).toBeInTheDocument(); + expect(screen.getByTestId('btn-PnL')).toBeInTheDocument(); expect(tree).toMatchSnapshot(); });
82-93: Improve test specificity for default period filterThe test assertions are too broad. Consider removing the second assertion which only checks for an object containing a type property.
it('dispatches default DAY filter on mount', () => { renderWithState('balance'); expect(mockDispatch).toHaveBeenCalledWith( setPeriodFilter(PeriodFilterBalance.DAY) ); - expect(mockDispatch).toHaveBeenCalledWith( - expect.objectContaining({ - type: 'walletPortfolio/setPriceGraphPeriod', - }) - ); });src/apps/pillarx-app/components/WalletPortfolioTile/test/WalletPortfolioTile.test.tsx (4)
1-1: Consider avoidingeslint-disablefor explicit anyWhile disabling the eslint rule may be acceptable in test files, it would be better to define proper types where possible instead of using
any.
5-7: Fix typo in comment"servuces" should be "services"
-// servuces +// services
82-101: Differentiate mock implementations for pnl and non-pnl casesThe current mock implementations for
useGetWalletPortfolioQueryare identical for both the pnl and non-pnl cases. Consider differentiating the mock data to better test how the component handles different responses.if (args.isPnl) { return { - data: { result: { data: {} } }, + data: { result: { data: { pnl: 123.45, pnlPercentage: 5.67 } } }, isLoading: false, isFetching: false, isSuccess: true, error: null, refetch: jest.fn(), }; } return { - data: { result: { data: {} } }, + data: { result: { data: { balance: 1000.00, tokens: [] } } }, isLoading: false, isFetching: false, isSuccess: true, error: null, refetch: jest.fn(), };
136-182: Add explanation for expected refetch call countThe test expects
refetchMockto be called exactly 4 times, but it's not immediately clear why. Add a comment explaining which API calls are being refetched to make the test more maintainable.render(<WalletPortfolioTile />); + // Should refetch: + // 1. Portfolio balance + // 2. Portfolio PnL + // 3. Wallet history + // 4. Top tokens unrealized PnL expect(refetchMock).toHaveBeenCalledTimes(4);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (18)
src/apps/pillarx-app/components/EditorialTile/test/__snapshots__/EditorialTile.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/GenericBannerTile/test/__snapshots__/GenericBannerTile.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/HighlightedMediaGridTile/test/__snapshots__/HighlightedMediaGridTile.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/MediaGridCollection/tests/__snapshots__/DisplayCollectionImage.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/PortfolioOverview/test/__snapshots__/PortfolioOverview.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/PrimeTokensBalance/test/__snapshots__/PrimeTokensBalance.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/ReceiveModal/test/__snapshots__/ReceiveModal.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TileContainer/test/__snapshots__/TileContainer.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TokenMarketDataRow/tests/__snapshots__/LeftColumnTokenMarketDataRow.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TokenMarketDataRow/tests/__snapshots__/RightColumnTokenMarketDataRow.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TokensHorizontalTile/test/__snapshots__/TokensHorizontalTile.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TokensVerticalTile/test/__snapshots__/TokensVerticalTile.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TokensWithMarketDataTile/test/__snapshots__/TokensWithMarketDataTile.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TopTokens/test/__snapshots__/TopTokens.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/WalletConnectDropdown/test/__snapshots__/WalletConnectDropdown.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/WalletPortfolioBalance/test/__snapshots__/WalletPortfolioBalance.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/WalletPortfolioGraph/tests/__snapshots__/WalletPortfolioGraph.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/WalletPortfolioTile/test/__snapshots__/WalletPortfolioTile.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (19)
src/apps/pillarx-app/components/PrimeTokensBalance/test/PrimeTokensBalance.test.tsx(1 hunks)src/apps/pillarx-app/components/ReceiveModal/ReceiveModal.tsx(1 hunks)src/apps/pillarx-app/components/ReceiveModal/test/ReceiveModal.test.tsx(1 hunks)src/apps/pillarx-app/components/TileContainer/test/TileContainer.test.tsx(2 hunks)src/apps/pillarx-app/components/TokenMarketDataRow/tests/RightColumnTokenMarketDataRow.test.tsx(1 hunks)src/apps/pillarx-app/components/TokensWithMarketDataTile/test/TokensWithMarketDataTile.test.tsx(1 hunks)src/apps/pillarx-app/components/TopTokens/TopTokens.tsx(1 hunks)src/apps/pillarx-app/components/TopTokens/test/TopTokens.test.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioBalance/WalletPortfolioBalance.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioBalance/test/WalletPortfolioBalance.test.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraphButton.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioGraph/tests/WalletPortfolioGraph.test.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioTile/WalletPortfolioTile.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioTile/test/WalletPortfolioTile.test.tsx(1 hunks)src/apps/pillarx-app/reducer/WalletPortfolioSlice.ts(1 hunks)src/services/pillarXApiWalletPortfolio.ts(4 hunks)src/utils/number.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/apps/pillarx-app/components/TileContainer/test/TileContainer.test.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- src/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraphButton.tsx
- src/utils/number.tsx
- src/apps/pillarx-app/components/WalletPortfolioBalance/WalletPortfolioBalance.tsx
- src/apps/pillarx-app/components/TopTokens/TopTokens.tsx
- src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
- src/services/pillarXApiWalletPortfolio.ts
- src/apps/pillarx-app/reducer/WalletPortfolioSlice.ts
- src/apps/pillarx-app/components/WalletPortfolioTile/WalletPortfolioTile.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/apps/pillarx-app/components/WalletPortfolioBalance/test/WalletPortfolioBalance.test.tsx (1)
src/apps/pillarx-app/reducer/WalletPortfolioSlice.ts (1)
setIsRefreshAll(125-127)
src/apps/pillarx-app/components/PrimeTokensBalance/test/PrimeTokensBalance.test.tsx (1)
src/types/api.ts (1)
PortfolioData(725-734)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages
🔇 Additional comments (19)
src/apps/pillarx-app/components/TokenMarketDataRow/tests/RightColumnTokenMarketDataRow.test.tsx (1)
44-44: Test updated to match new formatting behavior.The test expectation has been correctly updated to match the rounding behavior implemented by the new
limitDigitsNumberhelper function, converting the raw value0.042188to the displayed value$0.04219. The added comment is helpful for explaining the change.src/apps/pillarx-app/components/TokensWithMarketDataTile/test/TokensWithMarketDataTile.test.tsx (2)
138-138: Test updated to match new formatting behavior.The test expectation has been correctly updated to match the rounding behavior implemented by the
limitDigitsNumberhelper function, converting the raw value0.042188to the displayed value$0.04219. The explanatory comment is helpful.
145-145: Test updated to match new formatting behavior.The test expectation has been correctly updated to match the rounding behavior implemented by the
limitDigitsNumberhelper function, converting the raw value1.062188to the displayed value$1.0622. The explanatory comment is helpful.src/apps/pillarx-app/components/WalletPortfolioBalance/test/WalletPortfolioBalance.test.tsx (1)
30-166: Well-structured test suite with good coverageThe test suite is very well organized with good coverage of different portfolio balance scenarios. I particularly like:
- The use of the renderWithSelector helper function to simplify test setup
- Clear separation of different test cases
- Testing of both UI rendering and interaction logic
- Proper mocking of dependencies
src/apps/pillarx-app/components/TopTokens/test/TopTokens.test.tsx (3)
12-13: LGTM! Good practices on mocking dependencies.The test file properly mocks the necessary dependencies, including the reducer hooks and portfolio service, which is essential for unit testing React components with external dependencies.
54-56: LGTM! Good test isolation practices.Using
jest.clearAllMocks()inbeforeEachensures that each test runs with fresh mocks, preventing potential side effects between tests.
17-52: LGTM! Well-structured test data.The mock wallet portfolio data is well-structured and includes a good variety of test cases (positive and negative PnL values) that will help ensure the component handles different scenarios correctly.
src/apps/pillarx-app/components/PrimeTokensBalance/test/PrimeTokensBalance.test.tsx (3)
20-21: Good use of mocking for external dependenciesProperly mocking the external dependencies ensures isolated testing of the component behavior without relying on the actual implementations of these services.
36-38: Well-implemented cleanup between testsUsing
jest.clearAllMocks()in thebeforeEachhook ensures that each test starts with a clean slate, preventing test pollution and making tests more reliable.
108-131: Well-structured tooltip interaction testThis test effectively verifies the tooltip's show/hide behavior on mouse events, which is an important UI interaction pattern to validate.
src/apps/pillarx-app/components/ReceiveModal/test/ReceiveModal.test.tsx (6)
1-27: Well-structured test setup with appropriate mocksThe test file properly mocks external dependencies (
transaction-kit,useReducerHooks) and blockchain utilities, which is good practice for unit testing.
28-44: Good test organization with clear test casesThe test setup with mocking Redux hooks and clearing mocks before each test is a good practice to ensure test isolation.
46-54: Thorough conditional rendering testThis test correctly verifies that the modal doesn't render when
isReceiveModalOpenis false.
69-80: Good edge case handlingProperly tests the fallback state when no wallet address is available.
82-103: Thorough interaction testingGood test coverage for the modal closing functionality, verifying both the action dispatch and the payload value.
105-116: Complete feature coverageThe test verifies that all supported chains are rendered correctly.
src/apps/pillarx-app/components/ReceiveModal/ReceiveModal.tsx (1)
37-44: Good use of cleanup in useEffectThe useEffect correctly cleans up the timeout to prevent memory leaks.
src/apps/pillarx-app/components/WalletPortfolioTile/test/WalletPortfolioTile.test.tsx (2)
122-125: Verify if duplicate components are expectedThe test expects two instances of
WalletPortfolioButtonsandWalletPortfolioGraph. Please verify if this is the intended behavior or if it might indicate a potential issue with component duplication.
1-184: Overall good test structure with comprehensive coverageThe test suite is well-organized with clear sections for imports, mocks, and tests. It effectively tests the most important functionality including basic rendering, conditional rendering, and data refreshing behaviors. Good job on mocking dependencies and isolating the component under test!
src/apps/pillarx-app/components/PrimeTokensBalance/test/PrimeTokensBalance.test.tsx
Show resolved
Hide resolved
src/apps/pillarx-app/components/PrimeTokensBalance/test/PrimeTokensBalance.test.tsx
Show resolved
Hide resolved
src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx
Show resolved
Hide resolved
src/apps/pillarx-app/components/WalletPortfolioGraph/tests/WalletPortfolioGraph.test.tsx
Show resolved
Hide resolved
src/apps/pillarx-app/components/WalletPortfolioTile/test/WalletPortfolioTile.test.tsx
Show resolved
Hide resolved
src/apps/pillarx-app/components/WalletPortfolioTile/test/WalletPortfolioTile.test.tsx
Show resolved
Hide resolved
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores