Wallet Portfolio Tile - ui minor fixes after QA#313
Conversation
WalkthroughThis update introduces enhanced UI responsiveness, improved modal accessibility, and refined data handling for portfolio and token components. It includes new event-driven modal close behaviors, responsive layout adjustments across breakpoints, precise balance formatting, and expanded token support. Associated test suites are updated to reflect these changes and ensure proper functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ReceiveModal
participant ReduxStore
User->>ReceiveModal: Open modal
alt Press ESC key
User-->>ReceiveModal: Keydown (ESC)
ReceiveModal->>ReduxStore: setIsReceiveModalOpen(false)
else Click outside modal
User-->>ReceiveModal: Mousedown (outside)
ReceiveModal->>ReduxStore: setIsReceiveModalOpen(false)
end
Suggested reviewers
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: |
a078ad7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://05e6d8a9.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: 2
🧹 Nitpick comments (9)
src/apps/pillarx-app/components/ReceiveModal/ReceiveModal.tsx (2)
39-51: Great addition of ESC key handling for modal dismissal!The implementation properly adds and removes the event listener with the useEffect cleanup function. Consider adding the
handleOnCloseReceiveModalfunction to the dependency array instead of using the eslint-disable comment, to ensure the latest version of the function is always used.- // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [handleOnCloseReceiveModal]);
53-65: Good implementation of click-outside-to-close functionality.The click-outside pattern follows best practices with proper event cleanup. Similar to the ESC key handler, consider adding
handleOnCloseReceiveModalto the dependency array instead of disabling the eslint rule.- // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [handleOnCloseReceiveModal]);src/apps/pillarx-app/components/ReceiveModal/test/ReceiveModal.test.tsx (1)
138-157: Good test for click-outside dismissal.The test verifies that clicking outside the modal triggers the close action. For a more accurate test, consider simulating the click outside the modal element rather than on the document.
You could improve the test to better simulate clicking outside specifically:
- fireEvent.mouseDown(document); // simulates clicking outside modal + // Get the modal backdrop element and fire event on it, not on the modal itself + const modalBackdrop = document.querySelector('#wallet-portfolio-tile-receive-modal'); + if (modalBackdrop) { + fireEvent.mouseDown(modalBackdrop); + }This would more accurately test the real user behavior of clicking on the backdrop.
src/apps/pillarx-app/components/PrimeTokensBalance/test/PrimeTokensBalance.test.tsx (1)
86-93: Consider restoring the original mock after each case
jest.clearAllMocks()resets call history but notmockImplementation.
If another test (inside this describe or elsewhere) relies on the default selector implementation it may inadvertently receive this mocked state.afterEach(() => { jest.resetAllMocks(); // resets implementation & call counts });This is inexpensive insurance against test bleed-through.
src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx (2)
88-89:latestValuestate is helpful but make the initial value explicit
useState<number | undefined>()will start asundefined; declaring that explicitly clarifies intent:- const [latestValue, setLatestValue] = useState<number | undefined>(); + const [latestValue, setLatestValue] = useState<number | undefined>(undefined);Tiny clarity win, no functional change.
341-359: Array dependencies will retrigger effect on every render
walletHistoryGraph?.balance_historyandwalletPortfolioWithPnl?.pnl_historyare new array instances whenever Redux delivers fresh objects, causing the effect to fire even if their contents did not change.
If you only care about the latest point consider depending on their.length(or a memoised selector).- walletHistoryGraph?.balance_history, - walletPortfolioWithPnl?.pnl_history, + walletHistoryGraph?.balance_history?.length, + walletPortfolioWithPnl?.pnl_history ? Object.keys(walletPortfolioWithPnl.pnl_history).length : 0,Reduces unnecessary state churn and paints.
src/apps/pillarx-app/components/TopTokens/TopTokens.tsx (3)
104-106: DuplicatekeypropBoth the
<React.Fragment>and the inner<div>usekey={index}. The inner key is unnecessary and React will warn in StrictMode.- <div key={index} className="flex …"> + <div className="flex …">
148-155: Consider ARIA semantics for clickable rows
<div>withonClickis not keyboard-accessible. Converting to a<button>or addingrole="button"plustabIndex={0}and key handlers will improve accessibility.
160-188: Triangle icon colour calculation duplicates logicColour and rotation are recalculated twice (here and above). Extracting a tiny helper (e.g.,
getTrendProps(value)) would DRY up the code and keep JSX lean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
src/apps/pillarx-app/components/MediaGridCollection/tests/__snapshots__/DisplayCollectionImage.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TokenMarketDataRow/tests/__snapshots__/LeftColumnTokenMarketDataRow.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!**/*.snap
📒 Files selected for processing (10)
config-overrides.js(1 hunks)src/apps/pillarx-app/components/PrimeTokensBalance/PrimeTokensBalance.tsx(2 hunks)src/apps/pillarx-app/components/PrimeTokensBalance/test/PrimeTokensBalance.test.tsx(2 hunks)src/apps/pillarx-app/components/ReceiveModal/ReceiveModal.tsx(3 hunks)src/apps/pillarx-app/components/ReceiveModal/test/ReceiveModal.test.tsx(1 hunks)src/apps/pillarx-app/components/TopTokens/TopTokens.tsx(4 hunks)src/apps/pillarx-app/components/TopTokens/test/TopTokens.test.tsx(6 hunks)src/apps/pillarx-app/components/WalletPortfolioBalance/WalletPortfolioBalance.tsx(2 hunks)src/apps/pillarx-app/components/WalletPortfolioBalance/test/WalletPortfolioBalance.test.tsx(1 hunks)src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/apps/pillarx-app/components/PrimeTokensBalance/PrimeTokensBalance.tsx (1)
src/apps/pillarx-app/hooks/useReducerHooks.tsx (1)
useAppSelector(6-6)
src/apps/pillarx-app/components/ReceiveModal/ReceiveModal.tsx (2)
src/apps/pillarx-app/reducer/WalletPortfolioSlice.ts (1)
setIsReceiveModalOpen(128-130)src/apps/simpleswap/hooks/useComponentVisible.js (1)
handleClickOutside(7-14)
src/apps/pillarx-app/components/WalletPortfolioBalance/WalletPortfolioBalance.tsx (1)
src/utils/number.tsx (1)
limitDigitsNumber(42-69)
src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx (1)
src/utils/number.tsx (1)
limitDigitsNumber(42-69)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages
🔇 Additional comments (20)
src/apps/pillarx-app/components/ReceiveModal/ReceiveModal.tsx (2)
84-86: The modalRef is correctly applied to the modal container.This properly connects the ref to the DOM element for the click-outside detection.
92-92: Good simplification of onClick handler.Directly passing the handler function rather than wrapping it in an arrow function is more efficient.
src/apps/pillarx-app/components/ReceiveModal/test/ReceiveModal.test.tsx (1)
117-136: Great test for ESC key dismissal behavior.The test properly verifies that pressing the Escape key triggers the modal close action. It follows the test patterns established in the file with appropriate mocks and assertions.
src/apps/pillarx-app/components/WalletPortfolioBalance/test/WalletPortfolioBalance.test.tsx (1)
120-120: Updated test assertion to match new formattingThe test assertion now expects
-$500.00with two decimal places, which aligns with the updated formatting in the component where the USD change text now uses.toFixed(2). Good consistency between component implementation and test.src/apps/pillarx-app/components/TopTokens/test/TopTokens.test.tsx (6)
2-2: Added MemoryRouter import for router context in testsThe import of MemoryRouter is necessary to provide router context for the component during testing.
59-64: Wrapped component in MemoryRouter for router contextThe TopTokens component now requires router context, likely because it uses navigation hooks internally. This change ensures the component has the necessary context during testing.
81-85: Wrapped component in MemoryRouter for loading state testConsistent application of the router context in the loading state test case.
102-106: Wrapped component in MemoryRouter for error state testConsistent application of the router context in the error state test case.
139-143: Wrapped component in MemoryRouter for empty tokens testConsistent application of the router context in the empty tokens test case.
164-168: Wrapped component in MemoryRouter for token data testConsistent application of the router context in the token data rendering test case.
src/apps/pillarx-app/components/WalletPortfolioBalance/WalletPortfolioBalance.tsx (2)
76-77: Improved formatting consistency with two decimal placesThe change ensures that both positive and negative balance changes are displayed with consistent decimal precision (always two decimal places) by applying
.toFixed(2)after thelimitDigitsNumberfunction. This improves UI consistency and readability.
103-103: Updated flex item alignment from baseline to centerChanged the alignment of flex items from
items-baselinetoitems-centerfor better vertical alignment of the balance change and percentage elements.src/apps/pillarx-app/components/PrimeTokensBalance/PrimeTokensBalance.tsx (2)
31-33: Added loading state selectorAdded a selector to get the
isWalletPortfolioLoadingflag from the Redux store, enabling better loading state handling in the component.
50-54: Added skeleton loader during loading stateAdded conditional rendering to show a skeleton loader placeholder when data is loading, which prevents premature rendering of incomplete data and improves user experience during data fetching.
src/apps/pillarx-app/components/PrimeTokensBalance/test/PrimeTokensBalance.test.tsx (1)
52-59: Mock wrapper shape now mirrors reducer – good catchSwitching from a flat object to the nested shape
{ walletPortfolio: { … } }keeps the test aligned with the updated selector logic and avoids silentundefinedassertions.
Looks correct and keeps the test resilient to further state‐shape changes.src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx (1)
341-359: Verify direction of “latest” PnL valueFor balance you take the last point (
graphValues[graphValues.length-1]), but for PnL you take the first (graphValues[0]).
Unless the PnL dataset comes in reverse chronological order this will display the oldest value. Please confirm ordering; if both datasets are ascending in time, use the last element for both.- const latestGraphValue = isBalanceGraph - ? graphValues[graphValues.length - 1] - : graphValues[0]; + const latestGraphValue = graphValues[graphValues.length - 1];src/apps/pillarx-app/components/TopTokens/TopTokens.tsx (3)
26-34:useNavigateintegration looks goodInjecting routing directly into the component removes prop-drilling and keeps navigation concerns local.
53-69: Boolean flags read well – nice consolidationDeriving
isLoading,isError, andshowEmptyStatehere simplifies the JSX flow and prevents deep nested ternaries later on.
70-79: Loading skeleton ✅Early return keeps the DOM shallow and avoids multiple conditional branches.
config-overrides.js (1)
30-39:Details
❓ Verification inconclusive
Disabling custom chunk splitting may affect bundle sizes and loading performance.
Commenting out the
splitChunksconfiguration removes the custom chunking strategy that was previously limiting vendor chunks from node_modules to 50KB. This change will revert to webpack's default chunking behavior.Please verify that this change resolves the UI issues identified in QA without negatively impacting:
- Initial load performance
- Bundle sizes
- Caching effectiveness
🏁 Script executed:
#!/bin/bash # Check the impact on bundle sizes before and after this change # Look for build size information in recent builds echo "Searching for build size information in logs..." git log -n 10 --grep="build size" --pretty=format:"%h %s %b" # Check webpack config in package.json echo -e "\nChecking webpack configurations in package.json..." cat package.json | grep -A 10 -B 10 webpackLength of output: 976
Action Required: Verify Impact of Removing Custom Chunk Splitting
Removing the
splitChunksoverride inconfig-overrides.jsfalls back to webpack’s default chunking, which may increase vendor bundle sizes and affect load performance or caching. There are no recent “build size” entries in git logs and no webpack settings inpackage.json, so please verify manually:• Run a production build (e.g.
npm run build) before/after this change and compare bundle artifact sizes
• Measure initial load performance (Lighthouse, Web Vitals)
• Confirm effective caching (inspect fingerprinted filenames andcache-controlheaders)
src/apps/pillarx-app/components/WalletPortfolioGraph/BalancePnlGraph.tsx
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: 0
♻️ Duplicate comments (1)
src/apps/pillarx-app/components/TopTokens/TopTokens.tsx (1)
110-111: Navigation URL has a redundant&and should be encodedThe generated URL starts with
?&asset=…which is syntactically valid but untidy. Also, addresses may contain characters requiring encoding.- `/token-atlas?${`&asset=${token.contract.address}`}&blockchain=${parseInt(token.contract.chainId.split(':')[1], 10)}` + `/token-atlas?asset=${encodeURIComponent( + token.contract.address + )}&blockchain=${parseInt(token.contract.chainId.split(':')[1], 10)}`
🧹 Nitpick comments (2)
src/apps/pillarx-app/components/TopTokens/TopTokens.tsx (2)
93-93: Fix the missing space in classNameThere's a missing space in the className property which could cause styling issues.
- <div className="tablet:hidden mobile:hidden desktop:flextext-[13px] font-normal text-white/[.5] text-end"> + <div className="tablet:hidden mobile:hidden desktop:flex text-[13px] font-normal text-white/[.5] text-end">
105-107: Remove redundant key propThe div has a redundant key prop since the parent React.Fragment already has a key.
<React.Fragment key={`${token.asset.symbol}-${index}`}> <div - key={index} className="flex items-center flex-1 min-w-0 overflow-hidden cursor-pointer"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
src/apps/pillarx-app/components/MediaGridCollection/tests/__snapshots__/DisplayCollectionImage.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TokensWithMarketDataTile/test/__snapshots__/TokensWithMarketDataTile.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 (5)
src/apps/pillarx-app/components/TokensWithMarketDataTile/TokensWithMarketDataTile.tsx(2 hunks)src/apps/pillarx-app/components/TopTokens/TopTokens.tsx(4 hunks)src/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraph.tsx(2 hunks)src/apps/pillarx-app/components/WalletPortfolioTile/WalletPortfolioTile.tsx(1 hunks)src/apps/pillarx-app/utils/constants.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/apps/pillarx-app/utils/constants.ts
- src/apps/pillarx-app/components/WalletPortfolioGraph/WalletPortfolioGraph.tsx
- src/apps/pillarx-app/components/TokensWithMarketDataTile/TokensWithMarketDataTile.tsx
- src/apps/pillarx-app/components/WalletPortfolioTile/WalletPortfolioTile.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
src/apps/pillarx-app/components/TopTokens/TopTokens.tsx (3)
54-68: Well-structured loading and error state handlingThe refactored loading, error, and empty state logic is clear and provides a better user experience by correctly prioritizing the display of different states. This makes the component more robust and maintainable.
70-79: Good implementation of loading skeletonThe loading skeleton provides excellent visual feedback to users while data is being fetched. This is a great improvement to the user experience.
88-102: Well-structured responsive layoutThe grid structure with responsive column definitions for different screen sizes enhances the usability across devices. The conditional display of elements based on screen size is well implemented.
| // ESC key listener | ||
| useEffect(() => { | ||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === 'Escape') { |
There was a problem hiding this comment.
always a personal favourite
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3182/portfolio-tile-refactor)
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests