feat: Add comprehensive PnL tracking with Relay API integration#466
feat: Add comprehensive PnL tracking with Relay API integration#466
Conversation
WalkthroughAdds end-to-end PnL: types and utilities for trade reconstruction, a polling hook (useTokenPnL) that fetches wallet transactions and relay data, RTK Query slices and async helpers, new PnL UI components and skeletons, wiring into Search/Buy/Sell, and test/mocks updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (Buy / Sell / Search)
participant Hook as useTokenPnL
participant RTK as RTK Query (walletTxs)
participant Relay as relayApi
participant Utils as PnL Utils
participant Display as PnLStats / TokenPnLCell
UI->>Hook: request PnL (token, wallet, chain, balance, price)
Hook->>RTK: query wallet transactions
RTK-->>Hook: transactions (MobulaTransactionRow[])
Hook->>Utils: group & reconstruct trades
loop per txHash
Utils->>Relay: fetch relay request by hash
Relay-->>Utils: RelayRequest | null
end
Utils->>Utils: calculatePnLFromRelay / calculatePnL
Utils-->>Hook: ReconstructedTrade[] / PnLMetrics
Hook-->>UI: { metrics, isLoading, refetch }
UI->>Display: render PnLStats / TokenPnLCell (metrics or skeleton)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60-75 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
src/store.ts (2)
35-47: addReducer path resolution is reasonable, but consider tightening typesUsing
reducerPath || namewith a runtimeconsole.errorguard works, but it still allows misconfigured slices to slip through until runtime. You could typeaddReducerto accept only known slice/API shapes (e.g.Slice | Apiunion) so that eithernameorreducerPathis guaranteed at compile time, avoiding the need for a runtime error path.
73-79:initplaceholder reducer keeps configureStore happy but hurts RootState typingSeeding the store with
{ init: (state = {}) => state }is a pragmatic way to avoid an empty reducer map, but it also meansRootStateis inferred with aninitkey and no knowledge of dynamically added slices. If you rely onRootStatefor typed selectors, consider instead:
- Seeding
middlewareReducers.initbeforeconfigureStore, and- Passing
middlewareReducersdirectly intoconfigureStoreso that the inferred state better matches the dynamic shape.src/services/relayApi.ts (1)
79-155: Unify base URL usage and consider future pagination support
RELAY_API_URLis used forfetchRelayRequestsandfetchRelayRequestByHash, butfetchRelayRequestsByUserhardcodes the URL string. For consistency and easier maintenance, it’s worth switching that call to\${RELAY_API_URL}/requests/v2`and, if you later need full history, plumbingnextCursorfromRelayRequestsResponse` into a simple pagination loop.Since this relies on the current Relay API shape, please verify that
/requestsand/requests/v2still exposerequestsand (for v1)nextCursoras expected, and that there isn’t a more idiomatic “by user” endpoint you should be targeting.src/hooks/useTokenPnL.ts (1)
167-177: Auto-refresh interval depends onpropsobject identity and may restart on every renderThe auto-refresh effect lists
[props]as its sole dependency. If callers invoke the hook with an inline object (e.g.useTokenPnL({ token, transactionsData, walletAddress, chainId })),propswill be a new reference on each render, so this effect will clear and recreate the interval every render, effectively resetting the 30s timer.To make the interval more stable, depend on primitives instead, for example:
- useEffect(() => { - if (!props) return; + useEffect(() => { + if (!props) return; const interval = setInterval(() => { setResult(prev => ({ ...prev, isLoading: true })); setRefreshTrigger(prev => prev + 1); }, 30000); return () => clearInterval(interval); - }, [props]); + }, [!!props]);or use
walletAddress/props.token.contractas dependencies if you only want to reset the timer when the target token/wallet truly changes.Please verify how
TokenPnLCellconstructs thepropsobject when calling this hook so you can choose the tightest, stable dependency set.src/apps/pulse/components/Search/PortfolioTokenList.tsx (1)
102-147: Separate filtering from sorting to avoid duplicated work and hidden deps
getFilteredPortfolioTokensboth filters and sorts usingsortConfig, butportfolioTokensis created viauseMemothat does not listsortConfigas a dependency, and you then sort again insortedTokens.To simplify and make the behavior clearer:
- Keep
getFilteredPortfolioTokenspurely for filtering (includinghideSmallBalancesand search).- Move all sorting logic into the
sortedTokensuseMemo(which already depends onsortConfig).
This avoids double-sorting and removes an implicit dependency onsortConfigfrom a memoized function that doesn’t declare it.src/utils/pnl.ts (1)
19-45: Normalizing timestamp units across trade reconstruction paths
reconstructTradesassignstimestamp = tx.timestampdirectly, whereasgetRelayValidatedTradesusestimestamp / 1000andcalculatePnLFromRelayusesnew Date(req.createdAt).getTime() / 1000. While PnL math doesn’t depend on absolute units today (timestamps are only used for sorting), this inconsistency can become problematic if consumers later display dates or merge trades from multiple sources.Consider normalizing all trade timestamps to seconds since epoch (or milliseconds, but consistently) across all helpers so downstream consumers can rely on a single time unit.
Please double‑check Mobula’s
timestampunit (seconds vs milliseconds) to pick the correct normalization factor when you standardize these helpers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
index.html(3 hunks)src/apps/pulse/components/PnLStats/PnLStats.tsx(1 hunks)src/apps/pulse/components/Search/PortfolioTokenList.tsx(8 hunks)src/apps/pulse/components/Search/TokenPnLCell.tsx(1 hunks)src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx(3 hunks)src/hooks/useTokenPnL.ts(1 hunks)src/services/pillarXApiWalletTransactions.ts(1 hunks)src/services/relayApi.ts(1 hunks)src/store.ts(4 hunks)src/types/api.ts(2 hunks)src/utils/pnl.test.ts(1 hunks)src/utils/pnl.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
index.htmlsrc/services/pillarXApiWalletTransactions.tssrc/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/store.ts
📚 Learning: 2025-10-15T10:31:06.760Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 432
File: src/containers/Main.tsx:135-141
Timestamp: 2025-10-15T10:31:06.760Z
Learning: In src/containers/Main.tsx, the React Native webview messaging setup intentionally only activates when the `devicePlatform` URL parameter is present ('ios' or 'android'). On reloads or direct navigation without URL params, the site should behave normally and not attempt to re-establish RN messaging. This is by design to ensure the site functions properly in both RN webview and standard browser contexts.
Applied to files:
index.html
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Search/TokenPnLCell.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Search/TokenPnLCell.tsxsrc/apps/pulse/components/PnLStats/PnLStats.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/Search/TokenPnLCell.tsxsrc/hooks/useTokenPnL.tssrc/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx
📚 Learning: 2025-04-23T15:04:20.826Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 290
File: src/apps/pillarx-app/components/TileTitle/TitleTitle.tsx:6-10
Timestamp: 2025-04-23T15:04:20.826Z
Learning: In this repository, TileTitleProps and TileTitle are different types that serve different purposes. TileTitleProps is used for the TileTitle component and has optional fields (title?, leftDecorator?, rightDecorator?), while TileTitle in api.ts has a required text field. The TileTitleProps interface aligns with the TokensMarketData.title type in api.ts which also has optional fields.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/types/api.ts
📚 Learning: 2025-04-23T15:04:20.826Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 290
File: src/apps/pillarx-app/components/TileTitle/TitleTitle.tsx:6-10
Timestamp: 2025-04-23T15:04:20.826Z
Learning: In this repository, TileTitleProps and TileTitle are different types that serve different purposes. TileTitleProps is used for the TileTitle component and has optional fields (title?, leftDecorator?, rightDecorator?), while TileTitle in api.ts has a required title field. The TileTitleProps structure aligns with how it's used in the TokensMarketData type in api.ts.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/types/api.ts
🧬 Code graph analysis (8)
src/services/pillarXApiWalletTransactions.ts (2)
src/utils/blockchain.ts (1)
isTestnet(33-39)src/types/api.ts (1)
WalletTransactionsMobulaResponse(792-801)
src/utils/pnl.test.ts (2)
src/types/api.ts (1)
MobulaTransactionRow(768-790)src/utils/pnl.ts (2)
reconstructTrades(12-106)calculatePnL(108-184)
src/apps/pulse/components/Search/TokenPnLCell.tsx (4)
src/services/tokensData.ts (1)
Token(19-29)src/types/api.ts (1)
WalletTransactionsMobulaResponse(792-801)src/hooks/useTokenPnL.ts (1)
useTokenPnL(26-180)src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)
src/hooks/useTokenPnL.ts (4)
src/services/tokensData.ts (1)
Token(19-29)src/types/api.ts (2)
WalletTransactionsMobulaResponse(792-801)PnLMetrics(815-826)src/utils/pnl.ts (2)
calculatePnL(108-184)calculatePnLFromRelay(362-499)src/services/relayApi.ts (2)
RelayRequest(3-77)fetchRelayRequestByHash(107-130)
src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
src/types/api.ts (1)
PnLMetrics(815-826)
src/apps/pulse/components/Search/PortfolioTokenList.tsx (3)
src/types/api.ts (1)
PortfolioData(739-748)src/services/tokensData.ts (2)
Token(19-29)chainNameToChainIdTokensData(234-255)src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)
src/utils/pnl.ts (2)
src/types/api.ts (3)
MobulaTransactionRow(768-790)ReconstructedTrade(803-813)PnLMetrics(815-826)src/services/relayApi.ts (2)
RelayRequest(3-77)fetchRelayRequestByHash(107-130)
src/store.ts (1)
src/services/pillarXApiWalletTransactions.ts (2)
pillarXApiWalletTransactions(21-48)pillarXApiWalletTransactions(50-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (7)
index.html (1)
4-61: Confirm env placeholder replacement and TradingView script pathThe GA/Plausible tags and Hypelab script all depend on
%VITE_GOOGLE_ANALYTICS_MEASUREMENT_ID%/%VITE_PLAUSIBLE_DOMAIN%being replaced at build time, and on/charting_library/charting_library/charting_library.jsbeing available at that path in all deploy environments. Please confirm these are wired into your HTML build pipeline (or static hosting config) so you don’t ship literal%VITE_*%strings or a broken TradingView import.src/types/api.ts (1)
184-188: PnL-related API types look consistent; verify against Mobula schema
MobulaTransactionRow,WalletTransactionsMobulaResponse,ReconstructedTrade, andPnLMetricsline up with how they’re consumed inuseTokenPnLandsrc/utils/pnl.ts(fields liketx_hash,asset.contract(s),amount_usd, and the PnL metric names are all present and typed asnumber). TheProjection.dataunion reformat is also fine.Please double‑check these shapes against the latest Mobula wallet-transactions API response (especially
data.transactions[...]vsresult.data[...], and required vs optionalasset.contract/contracts) so you don’t get runtime shape mismatches.Also applies to: 768-826
src/store.ts (1)
94-99: RTK Query wallet-transactions API is wired correctly into dynamic middlewareRegistering
pillarXApiWalletTransactionsviaaddMiddlewarealigns with how the other API slices are integrated (reducer + middleware viacreateApi), so the new slice will participate in caching/polling as expected.Please double‑check against the current RTK Query docs that
pillarXApiWalletTransactions’sreducerPathand middleware are configured as intended in your version of@reduxjs/toolkit.src/services/pillarXApiWalletTransactions.ts (1)
1-50: Wallet-transactions RTK Query slice looks consistent with the Mobula response typeThe
fetchBaseQueryWithRetrysetup,getWalletTransactionsquery body, andtransformResponseunwrapping intoWalletTransactionsMobulaResponseare coherent, and the slice should compose cleanly with the dynamicaddMiddlewarewiring insrc/store.ts.Please confirm against the Mobula/HiFi data API docs that the
path: 'wallet/transactions'payload and{ result: WalletTransactionsMobulaResponse }envelope match the live response shape for both testnet and mainnet URLs.src/hooks/useTokenPnL.ts (2)
93-101: I'll help you verify the hash field inconsistency issue. Let me search the codebase to confirm whethertx.hashandtx.tx_hashare indeed being used inconsistently.
<function_calls>
#!/bin/bashFirst, let's find and examine the pnl.ts file to see how getRelayValidatedTrades uses tx fields
fd -t f 'pnl.ts' --exec cat -n {} ;
</function_calls>Now let me search for the specific usage of
tx_hashandtx.hashin the codebase:
<function_calls>
#!/bin/bashSearch for tx.tx_hash usage
echo "=== Searching for tx.tx_hash usage ==="
rg 'tx.tx_hash' -n -B2 -A2echo ""
echo "=== Searching for tx.hash usage in context ==="
rg 'tx.hash' -n -B2 -A2 --type=ts
</function_calls>Let me also check the type definitions to understand what fields are available on transaction objects:
<function_calls>
#!/bin/bashSearch for type definitions related to transactions
echo "=== Looking for Mobula transaction type definitions ==="
rg -n 'interface.[Tt]ransaction|type.[Tt]ransaction' --type=ts | head -20echo ""
echo "=== Checking useTokenPnL.ts for context ==="
cat -n src/hooks/useTokenPnL.ts | sed -n '85,110p'
</function_calls><function_calls>
#!/bin/bashLet's check the getRelayValidatedTrades function specifically
echo "=== Searching for getRelayValidatedTrades function ==="
rg -n 'getRelayValidatedTrades' -A 15 --type=ts
</function_calls><function_calls>
#!/bin/bashLet's also check if there's any documentation or comments about Relay API
echo "=== Checking for Relay API references ==="
rg -n 'Relay|requests/v2' -B2 -A2 --type=ts | head -40
</function_calls><function_calls>
#!/bin/bashCheck if there are any type imports or definitions for transaction data from Mobula
echo "=== Looking for Mobula type imports ==="
rg -n 'from.mobula|import.[Mm]obula' -A3 --type=tsecho ""
echo "=== Searching for transactionsData structure ==="
rg -n 'transactionsData.data.transactions' -B5 -A5 --type=ts
</function_calls>
108-123: I'll verify the concerns raised in this review comment about sequential Relay API calls. Let me start by examining the implementation and checking for relevant documentation.
<function_calls>
find . -name "useTokenPnL.ts" -type f
</function_calls>
<function_calls>
wc -l src/hooks/useTokenPnL.ts
</function_calls>
<function_calls>
sed -n '90,140p' src/hooks/useTokenPnL.ts
</function_calls>
<function_calls>
rg -n "fetchRelayRequestByHash" -A 3 -B 3
</function_calls>
<function_calls>
rg -n "RelayRequest" --type ts -C 2 | head -50
</function_calls>Let me also check for Relay API documentation and rate limit information:
<function_calls>
Relay API rate limits documentation
</function_calls>
<function_calls>
Relay protocol fetchRelayRequestByHash rate limit concurrency
</function_calls>Now let me check if there are existing caching mechanisms in the codebase:
<function_calls>
rg -n "cache|Cache|memoiz" --type ts src/hooks/ | head -30
</function_calls>
<function_calls>
rg -n "Promise.all" --type ts | head -20
</function_calls>Let me examine the broader context of the useTokenPnL.ts file:
<function_calls>
sed -n '1,50p' src/hooks/useTokenPnL.ts
</function_calls>
<function_calls>
rg -n "import.*fetchRelayRequestByHash" src/hooks/useTokenPnL.ts -A 5
</function_calls>Let me check where fetchRelayRequestByHash is defined:
<function_calls>
rg -rn "export.*fetchRelayRequestByHash|function fetchRelayRequestByHash" --type ts
</function_calls><function_calls>
find . -name "*.ts" -type f -exec grep -l "fetchRelayRequestByHash" {} ;
</function_calls>src/utils/pnl.ts (1)
191-353: I'll help you verify this review comment. Let me examine the relevant code to confirm the issues identified.
<function_calls>
cat -n src/utils/pnl.ts | sed -n '191,353p'
</function_calls>Let me search for both functions and their USDC_ADDRESSES definitions:
<function_calls>
rg -n "USDC_ADDRESSES" src/utils/pnl.ts -A 10 -B 2
</function_calls><function_calls>
rg -n "calculatePnLFromRelay" src/utils/pnl.ts -A 100 | head -150
</function_calls><function_calls>
rg -n "inTxs|outTxs" src/utils/pnl.ts -B 2 -A 2
</function_calls>Now let me check the full structure of both functions to understand the context better:
<function_calls>
wc -l src/utils/pnl.ts
</function_calls><function_calls>
cat -n src/utils/pnl.ts | sed -n '1,100p'
</function_calls><function_calls>
rg -n "export const (getRelayValidatedTrades|calculatePnLFromRelay)" src/utils/pnl.ts
</function_calls><function_calls>
cat -n src/utils/pnl.ts | sed -n '191,370p'
</function_calls><function_calls>
grep -n "calculatePnLFromRelay" src/utils/pnl.ts | head -5
</function_calls><function_calls>
cat src/utils/pnl.ts | tail -300 | head -150
</function_calls>Let me get the exact line numbers for calculatePnLFromRelay:
<function_calls>
ast-grep --pattern 'export const calculatePnLFromRelay'
</function_calls><function_calls>
python3 << 'EOF'
with open('src/utils/pnl.ts', 'r') as f:
lines = f.readlines()
for i, line in enumerate(lines, 1):
if 'calculatePnLFromRelay' in line or 'getRelayValidatedTrades' in line:
print(f"{i}: {line.rstrip()}")
EOF
</function_calls><function_calls>
tail -400 src/utils/pnl.ts | head -200
</function_calls><function_calls>
python3 << 'EOF'
import rewith open('src/utils/pnl.ts', 'r') as f:
content = f.read()Find all USDC_ADDRESSES definitions
usdc_matches = list(re.finditer(r'USDC_ADDRESSES\s*=\s*[(.*?)]', content, re.DOTALL))
print(f"Found {len(usdc_matches)} USDC_ADDRESSES definitions\n")for i, match in enumerate(usdc_matches):
start = content.rfind('\n', 0, match.start()) + 1
end = content.find('\n', match.end())
line_num = content[:match.start()].count('\n') + 1
context_start = max(0, match.start() - 200)
context_end = min(len(content), match.end() + 100)print(f"Match {i+1} at line ~{line_num}:") print(content[context_start:context_end]) print("\n" + "="*80 + "\n")Find inTxs and outTxs usage
print("\ninTxs and outTxs usage:")
for match in re.finditer(r'(inTxs|outTxs)', content):
line_num = content[:match.start()].count('\n') + 1
start = content.rfind('\n', 0, match.start()) + 1
end = content.find('\n', match.start())
print(f"Line {line_num}: {content[start:end]}")
EOF
</function_calls>Now let me check the structure more carefully:
<function_calls>
sed -n '191,280p' src/utils/pnl.ts
</function_calls><function_calls>
sed -n '280,353p' src/utils/pnl.ts
</function_calls>Let me check if there's a calculatePnLFromRelay function and its USDC_ADDRESSES:
<function_calls>
sed -n '353,$p' src/utils/pnl.ts | head -300
</function_calls><function_calls>
grep -n "const calculatePnLFromRelay|export const calculatePnLFromRelay|function calculatePnLFromRelay" src/utils/pnl.ts
</function_calls>Let me verify the file content and check if calculatePnLFromRelay exists:
<function_calls>
rg "calculatePnLFromRelay" src/
</function_calls><function_calls>
grep -r "USDC_ADDRESSES.*Arbitrum|USDC_ADDRESSES.*Optimism" src/
</function_calls>Let me search more broadly:
<function_calls>
rg "0x.*// .*USDC" src/utils/pnl.ts -B 1 -A 1
</function_calls><function_calls>
cat src/utils/pnl.ts | grep -A 30 "getRelayValidatedTrades" | head -40
</function_calls>Let me take a different approach - check the entire file structure:
<function_calls>
python3 << 'EOF'
with open('src/utils/pnl.ts', 'r') as f:
lines = f.readlines()Print summary of functions and key elements
in_function = None
for i, line in enumerate(lines[190:354], start=191):
if 'export const' in line or 'const ' in line or 'USDC_ADDRESSES' in line or 'inTxs' in line or 'outTxs' in line:
print(f"{i}: {line.rstrip()}")
EOF
</function_calls><function_calls>
sed -n '220,250p' src/utils/pnl.ts
</function_calls><function_calls>
sed -n '250,280p' src/utils/pnl.ts
</function_calls>Now let me confirm the review's claim about calculatePnLFromRelay by searching the entire codebase:
<function_calls>
find src -name ".ts" -o -name ".tsx" | xargs grep -l "calculatePnLFromRelay"
</function_calls><function_calls>
find src -name ".ts" -o -name ".tsx" | xargs grep -l "outTxs"
</function_calls>Let me check if the review is referencing code that might not exist in this PR:
<function_calls>
rg "relayReq.data" src/utils/pnl.ts -B 2 -A 2
</function_calls><function_calls>
sed -n '191,353p' src/utils/pnl.ts | grep -n "inTxs|outTxs|USDC_ADDRESSES"
</function_calls>Let me get the full content of the reviewed function:
<function_calls>
sed -n '191,353p' src/utils/pnl.ts > /tmp/reviewed_section.ts && wc -l /tmp/reviewed_section.ts
</function_calls><function_calls>
cat /tmp/reviewed_section.ts | tail -100
</function_calls>Now let me verify the key claims in the review by examining what's actually at those line numbers:
<function_calls>
python3 << 'EOF'
with open('src/utils/pnl.ts', 'r') as f:
lines = f.readlines()Check lines 191-353
section = lines[190:353]
Check for USDC_ADDRESSES
for i, line in enumerate(section, start=191):
if 'USDC_ADDRESSES' in line:
print(f"Line {i}: {line.rstrip()}")
# Print next 10 lines
for j in range(1, 11):
if 190 + i - 191 + j < len(lines):
print(f"Line {i+j}: {lines[190 + i - 191 + j].rstrip()}")
breakprint("\n" + "="*80 + "\n")
Check for inTxs
for i, line in enumerate(section, start=191):
if 'inTxs' in line:
print(f"Line {i}: {line.rstrip()}")print("\n" + "="*80 + "\n")
Check for outTxs
for i, line in enumerate(section, start=191):
if 'outTxs' in line:
print(f"Line {i}: {line.rstrip()}")
EOF
</function_calls><function_calls>
sed -n '235,245p' src/utils/pnl.ts
</function_calls>Let me get a clearer picture:
<function_calls>
python3 << 'EOF'
with open('src/utils/pnl.ts', 'r') as f:
content = f.read()
lines = content.split('\n')Find line 235 and nearby for USDC_ADDRESSES
for i in range(230, 250):
print(f"{i+1}: {lines[i]}")
EOF
</function_calls>Based on my exploration, let me now search for the specific patterns mentioned in the review:
<function_calls>
grep -n "0x833589fcd6edb6e08f4c7c32d4f71b54bda02913|0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48" src/utils/pnl.ts
</function_calls><function_calls>
sed -n '230,280p' src/utils/pnl.ts
</function_calls><function_calls>
sed -n '290,320p' src/utils/pnl.ts
</function_calls>Now let me check if there are any other functions dealing with Relay and USDC:
<function_calls>
rg -n "export const.*Relay|const.*Relay" src/utils/pnl.ts
</function_calls><function_calls>
python3 << 'EOF'
import rewith open('src/utils/pnl.ts', 'r') as f:
lines = f.readlines()Find all function definitions
functions = []
for i, line in enumerate(lines):
if re.match(r'\s*(export\s+)?const\s+\w+\s*=\s*(async\s*)?(|function\s+\w+', line):
functions.append((i+1, line.strip()))print("Functions in pnl.ts:")
for line_num, func_def in functions:
print(f"Line {line_num}: {func_def[:80]}")
EOF
</function_calls>Now let me check if calculatePnLFromRelay exists anywhere in the codebase or if it's mentioned only in the review:
<function_calls>
git log --oneline -20
</function_calls><function_calls>
git diff HEAD^ src/utils/pnl.ts | grep -A 5 -B 5 "calculatePnLFromRelay|outTxs" | head -40
</function_calls>Let me check what branch we're on and see if calculatePnLFromRelay might be added:
<function_calls>
git status
</function_calls><function_calls>
git log --all --pretty=format:"%h %s" | head -20
</function_calls>Let me verify whether the review comment references code that doesn't actually exist:
<function_calls>
python3 << 'EOF'Read the entire file and check key claims from the review
with open('src/utils/pnl.ts', 'r') as f:
content = f.read()Check 1: Does calculatePnLFromRelay exist?
has_calculatePnLFromRelay = 'calculatePnLFromRelay' in content
print(f"calculatePnLFromRelay exists: {has_calculatePnLFromRelay}")Check 2: Count USDC_ADDRESSES definitions
usdc_count = content.count('USDC_ADDRESSES')
print(f"USDC_ADDRESSES appears {usdc_count} times")Check 3: Does getRelayValidatedTrades check outTxs?
getRelayValidatedTrades_idx = content.find('getRelayValidatedTrades')
if getRelayValidatedTrades_idx >= 0:
# Extract the function (estimate end by looking for next export const)
func_start = getRelayValidatedTrades_idx
next_export = content.find('export const', func_start + 1)
if next_export < 0:
next_export = len(content)
func_content = content[func_start:next_export]has_outTxs_in_func = 'outTxs' in func_content has_inTxs_in_func = 'inTxs' in func_content print(f"In getRelayValidatedTrades:") print(f" - has inTxs: {has_inTxs_in_func}") print(f" - has outTxs: {has_outTxs_in_func}")EOF
</function_calls>Perfect! Now I have the key finding. Let me create one more verification to see the actual USDC_ADDRESSES list:
<function_calls>
python3 << 'EOF'
import rewith open('src/utils/pnl.ts', 'r') as f:
content = f.read()Extract all USDC_ADDRESSES lists
pattern = r"USDC_ADDRESSES\s*=\s*[(.*?)]"
matches = list(re.finditer(pattern, content, re.DOTALL))print(f"Found {len(matches)} USDC_ADDRESSES definitions:\n")
for i, match in enumerate(matches, 1):
line_num = content[:match.start()].count('\n') + 1
usdc_list = match.group(1)
# Count addresses in list
addr_count = usdc_list.count('0x')
print(f"Match {i} (line ~{line_num}): {addr_count} addresses")
print(usdc_list.strip()[:200] + "...\n")
EOF
</function_calls>Now let me verify the specific addresses mentioned in the review:
<function_calls>
grep -n "// Base USDC|// Ethereum USDC|// Polygon USDC|// BSC USDC|// Arbitrum|// Optimism" src/utils/pnl.ts
</function_calls>Perfect! Now I have enough information. Let me do a final check on the complete section to understand what's there:
<function_calls>
sed -n '191,353p' src/utils/pnl.ts | grep -n "USDC|inTxs|outTxs" | head -20
</function_calls>Based on my verification, I can now draw conclusions:
Now let me search for any related code that might handle outTxs or other USDC addresses:
<function_calls>
rg "outTxs" src/ --type ts --type tsx
</function_calls><function_calls>
rg "Arbitrum.*USDC|Optimism.*USDC" src/ --type ts --type tsx
</function_calls>Let me check the Relay types to understand the schema:
<function_calls>
find src -name ".ts" -o -name ".tsx" | xargs grep -l "RelayRequest|inTxs|outTxs" | head -5
</function_calls><function_calls>
rg "interface RelayRequest|type RelayRequest" src/ -A 20
</function_calls><function_calls>
rg "inTxs|outTxs" src/ -B 3 -A 3 | head -50
</function_calls>Let me check if there are type definitions or imports that show the Relay schema:
<function_calls>
head -50 src/utils/pnl.ts
</function_calls><function_calls>
rg "type.=.{|interface" src/utils/pnl.ts | head -20
</function_calls>Let me check related types in the types file:
<function_calls>
find src -name "*.ts" | xargs grep -l "RelayRequest" | head -3
</function_calls><function_calls>
rg "export.*RelayRequest|type RelayRequest" src/ -A 30 | head -80
</function_calls>Based on my comprehensive verification, I can now provide my findings:
Verify review comment references non-existent
calculatePnLFromRelayfunctionThe review references
calculatePnLFromRelayand its handling of bothinTxsandoutTxs, but this function does not exist in the codebase.While the concern about
getRelayValidatedTradeschecking onlyrelayReq.data?.inTxs(and notoutTxs) is technically valid, the comparison to a hypotheticalcalculatePnLFromRelaywith 6 supported chains cannot be verified. The claim about Arbitrum/Optimism USDC addresses being in a different list is unsubstantiated.The identified issues remain valid as concerns:
USDC_ADDRESSESingetRelayValidatedTradescovers 4 chains only (Base, Ethereum, Polygon, BSC)- The function checks only
inTxsand ignoresoutTxsHowever, without evidence that the broader
outTxspattern or additional chains are actually needed for correctness, the review's framing is speculative. The recommendations should be rewritten to focus on actual code patterns in the repository.
src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx
Outdated
Show resolved
Hide resolved
src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx
Outdated
Show resolved
Hide resolved
- Implement PnL calculation using Relay API for accurate buy/sell tracking - Add PnLStats component with 4-column layout - Add TokenPnLCell component for portfolio list PnL display - Create useTokenPnL hook for fetching and calculating PnL metrics - Add auto-refresh (30s) and manual refresh support for PnL data - Integrate PnL display in Buy and Sell components - Add skeleton loading animations for PnL components - Update My Holdings layout with improved column spacing and sort indicators - Add Relay API service for fetching transaction data - Add wallet transactions API integration - Implement PnL utilities with test coverage Refactored PnL utilities to comply with linting rules: - Replaced for-of loops with Promise.all and array methods - Eliminated continue statements using filter/map - Added explicit return types and fixed hook dependencies
Deploying pillarx-debug with
|
| Latest commit: |
f2ce30b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7609f21b.pillarx-debug.pages.dev |
| Branch Preview URL: | https://feature-pnl.pillarx-debug.pages.dev |
github-actions
bot
requested a deployment
to
Preview (feature-pnl)
Deploying x with
|
| Latest commit: |
f2ce30b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://37cd9dcc.x-e62.pages.dev |
| Branch Preview URL: | https://feature-pnl.x-e62.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (4)
inspect_aave_contracts_full.js (1)
1-21: Duplicate code: part of AAVE inspection script consolidation.This is the third script with nearly identical logic to
inspect_aave_portfolio.jsandinspect_full_aave.js. Please see the consolidation suggestion in the review comment forinspect_full_aave.js.src/apps/pulse/components/Search/PortfolioTokenList.tsx (2)
77-80: Debug console.log already flaggedThis debug logging was previously flagged and should be removed before merging.
195-199: PnL sort no-op already flaggedThe PnL sort returning 0 for both values was previously flagged.
src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
150-156: Negative PnL sign still missing in USD formatting (previously flagged).The past review correctly identified that negative USD values render without the
-sign (e.g.,$0.80 (-0.5%)instead of-$0.80 (-0.5%)). This occurs becauseMath.abs(usdValue)removes the sign, and thesignvariable is empty string for negative values rather than-.const formatPnLWithPercent = (usdValue: number, pctValue: number) => { - const sign = usdValue >= 0 ? '+' : ''; - const usdFormatted = `${sign}${formatUSD(Math.abs(usdValue))}`; - const pctFormatted = `${sign}${pctValue.toFixed(2)}%`; + const usdSign = usdValue > 0 ? '+' : usdValue < 0 ? '-' : ''; + const pctSign = pctValue > 0 ? '+' : pctValue < 0 ? '-' : ''; + const usdFormatted = `${usdSign}${formatUSD(Math.abs(usdValue))}`; + const pctFormatted = `${pctSign}${Math.abs(pctValue).toFixed(2)}%`; return `${usdFormatted} (${pctFormatted})`; };
🟡 Minor comments (11)
inspect_relay_full.js-1-9 (1)
1-9: Add error handling and consider consolidating Relay fetch scripts.This script lacks error handling and duplicates logic found in other Relay-related scripts.
Apply this diff to add basic error handling:
const fetch = require('node-fetch'); (async () => { const hash = '0xad9cc422081297ddd3955b60a7573da4c6fc3086b4475b38af48ed25155c7ff6'; console.log(`Fetching Relay data for ${hash}...`); + try { const response = await fetch(`https://api.relay.link/requests/v2?hash=${hash}`); + if (!response.ok) { + throw new Error(`HTTP ${response.status}: ${response.statusText}`); + } const data = await response.json(); console.log(JSON.stringify(data, null, 2)); + } catch (error) { + console.error('Failed to fetch Relay data:', error.message); + process.exit(1); + } })();Additionally, consider consolidating with
test_relay_by_user.jsanddebug_steth_tx.jsinto a single parameterized utility script.debug_steth_tx.ts-3-3 (1)
3-3: Remove unused import.
calculatePnLFromRelayis imported but never used in this script.import { fetchRelayRequestByHash } from './src/services/relayApi'; -import { calculatePnLFromRelay } from './src/utils/pnl';debug_aave.ts-77-78 (1)
77-78: Type the error parameter in catch blockAccessing
e.messageon an implicitlyunknowntype will fail in strict TypeScript:- } catch (e) { - console.log(`\n❌ Error querying Relay: ${e.message}`); + } catch (e) { + const message = e instanceof Error ? e.message : String(e); + console.log(`\n❌ Error querying Relay: ${message}`); }verify_transactions.ts-49-49 (1)
49-49: Missing.catch()handler on script invocation.Other scripts use
.catch(console.error)to handle promise rejections. This script should follow the same pattern for consistency and to ensure errors are visible.-verifyTransactions(); +verifyTransactions().catch(console.error);test_portfolio_pnl.ts-74-77 (1)
74-77: Potential runtime error from undefined array access.
asset.asset.contracts[0]will throw ifcontractsis undefined or empty. Add a safety check.- const contract = asset.asset.contracts[0]; + const contract = asset.asset.contracts?.[0]; const price = asset.price; const usdValue = price * asset.token_balance;verify_aave_pnl.js-38-47 (1)
38-47: Missing null check before accessingaaveAssetproperties.If AAVE is not found in the portfolio,
aaveAssetwill beundefinedand line 39 will throwCannot read properties of undefined.const aaveAsset = assets.find(a => a.asset.symbol === 'AAVE'); + if (!aaveAsset) { + console.error('AAVE asset not found in portfolio!'); + return; + } const aaveToken = aaveAsset.contracts_balances .filter(cb => cb.balance > 0)test_relay_matching.ts-72-81 (1)
72-81: USDC transaction filter appears ineffective.The
usdcTxsfilter on line 78 filters transactions wheret.asset.symbol === 'USDC', buttxsis already grouped by symbol from line 72. Ifsymbolis 'USDC', all transactions match; otherwise, none do. This logic seems unintentional.Did you mean to check for USDC involvement in the transaction pairs rather than filtering by the grouped symbol?
test_all_assets_pnl.js-163-167 (1)
163-167: Potential undefined access forasset.asset.contracts[0].If
asset.asset.contractsis an empty array,contracts[0]will beundefined, and the fallback toasset.asset.contractmay also be undefined, causing issues downstream.const symbol = asset.asset.symbol; - const contract = asset.asset.contracts[0] || asset.asset.contract; + const contract = asset.asset.contracts?.[0] || asset.asset.contract; + if (!contract) { + console.log(`Skipping ${symbol}: no contract address`); + continue; + }src/utils/pnl.ts-88-98 (1)
88-98:ReconstructedTradeusesfeesUSDbut the type definition usesfees.The type
ReconstructedTradeinsrc/types/api.ts(lines 802-812) definesfees?: number, but this code setsfeesUSD. This will cause a type error or the field will be ignored.trades.push({ side, txHash, timestamp, amountToken: absTokenChange, amountQuoteUSDC: absUsdcChange, execPriceUSD: absUsdcChange / absTokenChange, - feesUSD: feeUsd, // Not implemented fully yet as per complexity + fees: feeUsd, // Not implemented fully yet as per complexity tokenAddress: tokenAddress || '', tokenSymbol, });The same issue exists on lines 352 and 500 (using
feesUSDinstead offees).test_complete_pnl_flow.js-1-3 (1)
1-3: Hardcoded wallet address should be externalized or removed before merge.The wallet address on line 2 appears to be a real address. For a test script that may be committed, consider using environment variables or command-line arguments.
const fetch = require('node-fetch'); -const WALLET = '0x70e8741c1758Ba32176B188286B8086956627B1c'; +const WALLET = process.env.TEST_WALLET || process.argv[2]; + +if (!WALLET) { + console.error('Usage: node test_complete_pnl_flow.js <wallet_address>'); + console.error('Or set TEST_WALLET environment variable'); + process.exit(1); +}src/store.ts-35-47 (1)
35-47: Based on my verification, I can now provide the rewritten review comment:Tighten
addReducertyping to require at least one reducer identifierThe current type signature allows callers to pass an object with only
reducer(missing bothreducerPathandname), which defers validation to runtime. All five call sites in the codebase pass Redux Toolkit slices that include thenameproperty, so adopting a discriminated union would be compatible with existing usage while preventing compile-time errors.Use a discriminated union to enforce that at least one identifier is present:
-export const addReducer = (newReducer: { - reducerPath?: string; - name?: string; - reducer: Reducer; -}) => { +type DynamicReducerConfig<R extends Reducer = Reducer> = + | { reducerPath: string; reducer: R; name?: never } + | { name: string; reducer: R; reducerPath?: never }; + +export const addReducer = (newReducer: DynamicReducerConfig) => { const path = newReducer.reducerPath || newReducer.name; if (!path) { console.error('Reducer path or name is missing', newReducer); return; } middlewareReducers[path] = newReducer.reducer; store.replaceReducer(combineReducers(middlewareReducers)); };This enforces at compile time that exactly one of
reducerPathornameis provided, preventing the runtime error condition.
🧹 Nitpick comments (48)
test_relay_by_user.js (1)
1-27: Well-structured test script with proper error handling.This script demonstrates good practices with comprehensive error handling and response structure inspection. However, the hardcoded wallet address should be parameterized.
Consider making the wallet address configurable:
const fetch = require('node-fetch'); -const WALLET = '0x70e8741c1758Ba32176B188286B8086956627B1c'; +const WALLET = process.argv[2] || process.env.WALLET_ADDRESS || '0x70e8741c1758Ba32176B188286B8086956627B1c'; (async () => { console.log('Testing Relay API fetch by user...'); + console.log('Wallet:', WALLET); try { // ... rest of codedebug_steth_tx.ts (1)
22-32: Incomplete logic: clarify the purpose or complete the implementation.This section contains incomplete/commented logic about testing PnL calculations. If this script is intended for debugging, consider either:
- Completing the PnL calculation test with actual token details
- Removing the incomplete section
- Moving this to a TODO or issue if the work is deferred
The comments suggest uncertainty about chain/token details. Consider adding a clear purpose statement at the top of the file explaining what this debug script validates.
debug_steth_tx.js (1)
1-41: Consider consolidating with TypeScript version or documenting the rationale.This JavaScript version duplicates
debug_steth_tx.tsand re-implementsfetchRelayRequestByHashlocally. While the comments (lines 6-7) explain this avoids TypeScript/ESM import issues, consider:
- If both versions are needed: Document in a README why both JS and TS versions exist
- If only one is needed: Remove the duplicate and use a simple build step for the TS version
- If this is temporary: Add a TODO comment indicating this should be consolidated
Additionally, the return structure at line 16 (
{ data: data }) doesn't match the actual Relay API response structure. According tosrc/services/relayApi.ts, the API returns{ requests: [...] }, andfetchRelayRequestByHashreturns a singleRelayRequest | null.const data = await response.json(); - return { data: data }; // Mimic the structure expected + // Relay API v2 returns { requests: [...] } + const requests = data.requests || []; + return requests.length > 0 ? requests[0] : null; } catch (error) {test_rtk_response.js (1)
1-59: Test script should not be committed to production codebaseThis debug script contains a hardcoded wallet address and lacks error handling. Consider:
- Moving to a
scripts/ortests/directory if needed for debugging- Using environment variables for wallet addresses instead of hardcoding
- Adding error handling for network failures
+const WALLET = process.env.TEST_WALLET || '0x...'; + (async () => { + try { console.log('=== TESTING RTK QUERY RESPONSE STRUCTURE ===\n'); // ... rest of the code + } catch (error) { + console.error('Error testing RTK response:', error.message); + process.exit(1); + } })();If this is only for local debugging, consider adding it to
.gitignoreinstead of committing.src/apps/pulse/components/Search/PortfolioTokenList.tsx (1)
486-489: Remove commented-out codeThis commented-out code block appears to be leftover from development. Remove it to keep the codebase clean:
${/* // Skip PnL for small balances */} - {/* if (valueUSD < 0.01) { */} - {/* return { ...token, pnl: null }; */} - {/* } */} {token.pricetest_usdc_detection.js (3)
6-11: CentralizeUSDC_ADDRESSESconstantThis constant is duplicated across multiple test files (
test_complete_pnl_flow.js,test_optimized_pnl.js, and here). Extract it to a shared test utilities file or constants module to maintain a single source of truth.// e.g., in test/constants.js or src/constants/tokens.ts export const USDC_ADDRESSES = [ '0x833589fcd6edb6e08f4c7c32d4f71b54bda02913', // Base USDC '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48', // Ethereum USDC // ... ];
4-5: Avoid hardcoding wallet addresses and transaction hashesHardcoded credentials can be a privacy concern. Use environment variables or a test configuration file:
-const WALLET = '0x70e8741c1758Ba32176B188286B8086956627B1c'; -const TX_HASH = '0xad9cc422081297ddd3955b60a7573da4c6fc3086b4475b38af48ed25155c7ff6'; +const WALLET = process.env.TEST_WALLET; +const TX_HASH = process.env.TEST_TX_HASH; + +if (!WALLET || !TX_HASH) { + console.error('Set TEST_WALLET and TEST_TX_HASH environment variables'); + process.exit(1); +}
13-40: Add error handling for network requestsThe script lacks error handling for fetch failures, which could cause unhandled promise rejections:
(async () => { + try { console.log('Testing USDC detection in Relay stateChanges\n'); // ... existing code + } catch (error) { + console.error('Error:', error.message); + process.exit(1); + } })();debug_aave.ts (2)
1-2: Use proper TypeScript import fornode-fetchUsing
@ts-ignoresuppresses type checking. Since this is a TypeScript file, use a proper import:-// @ts-ignore -const fetch = require('node-fetch'); +import fetch from 'node-fetch';Ensure
@types/node-fetchis installed as a dev dependency if not already present.
4-6: Consider using environment variables for configurationSimilar to other test scripts, hardcoded URLs and wallet addresses could be moved to environment variables for flexibility:
-const WALLET = '0x70e8741c1758Ba32176B188286B8086956627B1c'; -const API_URL = 'https://hifidata-7eu4izffpa-uc.a.run.app'; -const RELAY_URL = 'https://api.relay.link'; +const WALLET = process.env.TEST_WALLET || ''; +const API_URL = process.env.API_URL || 'https://hifidata-7eu4izffpa-uc.a.run.app'; +const RELAY_URL = process.env.RELAY_URL || 'https://api.relay.link';inspect_aave_contracts.js (2)
1-35: Consolidate debug scripts into a unified toolThis script follows the same pattern as
test_rtk_response.js,test_usdc_detection.js, anddebug_aave.ts. Consider:
- Creating a unified CLI tool with subcommands for different inspections
- Centralizing wallet address configuration
- Adding proper error handling
- Moving to a dedicated
scripts/directory// Example: scripts/debug-cli.js const commands = { 'aave-contracts': inspectAaveContracts, 'usdc-detection': testUsdcDetection, 'rtk-response': testRtkResponse, }; const command = process.argv[2]; if (!commands[command]) { console.log('Usage: node debug-cli.js <command>'); process.exit(1); } commands[command]().catch(console.error);This reduces duplication and provides a cleaner development experience.
4-35: Add error handling for async operationsWrap the IIFE in try-catch to handle potential failures gracefully:
(async () => { + try { console.log('Fetching transactions for wallet:', WALLET); // ... existing code + } catch (error) { + console.error('Failed to inspect AAVE contracts:', error.message); + process.exit(1); + } })();find_hashes.ts (3)
1-6: Debug scripts should not reside insrc/or production directories.These diagnostic scripts (find_hashes.ts, inspect_relay.ts, etc.) appear to be ad-hoc debugging tools with hardcoded wallet addresses and API URLs. Consider:
- Moving to a dedicated
scripts/ortools/directory.- Using environment variables for
WALLET_ADDRESSandRELAY_API_URL.- Adding these to
.gitignoreif they contain personal test data, or sanitizing the wallet address.-const WALLET_ADDRESS = '0x70e8741c1758Ba32176B188286B8086956627B1c'; -const RELAY_API_URL = 'https://api.relay.link'; +const WALLET_ADDRESS = process.env.TEST_WALLET_ADDRESS || ''; +const RELAY_API_URL = process.env.RELAY_API_URL || 'https://api.relay.link';
7-12: Add error handling for fetch and response validation.The fetch call lacks error handling for network failures and does not validate the response status before parsing JSON.
async function findTransactionHashes() { - const relayResponse = await fetch(`${RELAY_API_URL}/requests?user=${WALLET_ADDRESS}&status=filled`); - const relayData = await relayResponse.json(); + const relayResponse = await fetch(`${RELAY_API_URL}/requests?user=${WALLET_ADDRESS}&status=filled`); + if (!relayResponse.ok) { + throw new Error(`Relay API request failed: ${relayResponse.status}`); + } + const relayData = await relayResponse.json(); const relayRequests = relayData?.requests || [];
21-37: Consider adding explicit types for the recursive function.The
findHashesRecursivefunction usesanytype. For better maintainability, consider using a more descriptive type orunknownwith type guards.inspect_relay.ts (2)
1-6: Duplicated setup pattern across debug scripts.This file shares the same hardcoded wallet address and API URL pattern as
find_hashes.tsand other debug scripts. Consider creating a shared configuration module for these scripts to reduce duplication and make testing different wallets easier.
10-12: Missing response validation before JSON parsing.Similar to
find_hashes.ts, this lacks error handling for failed requests.const relayResponse = await fetch(`${RELAY_API_URL}/requests?user=${WALLET_ADDRESS}&status=filled`); + if (!relayResponse.ok) { + throw new Error(`Relay API request failed: ${relayResponse.status}`); + } const relayData = await relayResponse.json();inspect_aave_hashes.js (2)
7-24: Missing error handling for API request.The fetch call does not check
resp.okbefore parsing. If the API returns an error status,resp.json()may fail or return unexpected data.const resp = await fetch('https://hifidata-7eu4izffpa-uc.a.run.app?testnets=false', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ path: 'wallet/transactions', params: { wallet: WALLET, limit: 500, page: 1, filterSpam: true, }, }), }); + if (!resp.ok) { + throw new Error(`API request failed: ${resp.status}`); + } + const data = await resp.json();
31-35: Consider usingMapfor grouping consistency.Other scripts in this PR use
Mapfor grouping (e.g.,test_final_matching.ts). Using a consistent approach across scripts improves maintainability.test_final_matching.ts (2)
12-28: Add error handling for API requests.Both the Mobula and Relay API calls lack response validation. Network failures or API errors will cause silent failures or confusing error messages.
const mobResp = await fetch(`${API_URL}?testnets=false`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ path: 'wallet/transactions', params: { wallet: WALLET, limit: 500, page: 1, filterSpam: true }, }), }); + if (!mobResp.ok) { + throw new Error(`Mobula API request failed: ${mobResp.status}`); + } const mobData = await mobResp.json(); const txs = mobData?.result?.data?.transactions || []; console.log(`✓ Fetched ${txs.length} Mobula transactions\n`); // 2. Fetch Relay requests const relayResp = await fetch(`${RELAY_URL}/requests?user=${WALLET}&status=filled`); + if (!relayResp.ok) { + throw new Error(`Relay API request failed: ${relayResp.status}`); + } const relayData = await relayResp.json();
61-65: O(n²) complexity for USDC involvement check.The nested
txs.some()inside the outer loop creates quadratic complexity. For large transaction sets, consider pre-building a hash-to-USDC lookup map.+ // Pre-build USDC lookup by hash + const usdcByHash = new Set( + txs.filter(tx => tx.asset?.symbol === 'USDC').map(tx => tx.tx_hash) + ); + tokenMatches.forEach((data, symbol) => { // ... const hasUSDC = data.txs.some(t => { - const txHash = t.tx_hash; - return txs.some(tx2 => tx2.tx_hash === txHash && tx2.asset.symbol === 'USDC'); + return usdcByHash.has(t.tx_hash); });test_pnl_calculation.ts (3)
9-32: Duplicates logic fromsrc/utils/pnl.ts.This local
calculatePnLfunction duplicates the core PnL calculation logic already implemented insrc/utils/pnl.ts. The production version is more comprehensive (includesavgBuyPrice,avgSellPrice,realisedPnLPct, etc.). Consider importing and reusing the existing utility.+import { calculatePnL } from './utils/pnl'; + -// Simple PnL calculation -function calculatePnL(trades, currentPrice) { - // ... local implementation -}Based on the relevant code snippet from
src/utils/pnl.ts, the production function returns a richerPnLMetricsobject that this test could validate against.
73-110: N+1 query pattern and empty catch block.Two concerns:
N+1 queries: Each unique hash triggers a separate Relay API call. For wallets with many transactions, this could be slow and risk rate limiting. Consider batching or using the user-based endpoint with local filtering.
Silent error swallowing: The empty catch block (lines 107-109) hides all errors, making debugging difficult.
} catch (e) { - // 404 or error = not in Relay + // Log non-404 errors for debugging + if (e.response?.status !== 404) { + console.warn(`Relay lookup failed for ${hash}:`, e.message); + } }
38-48: Missing response validation for Mobula API call.Consistent with other files, add response status check before parsing JSON.
const mobResp = await fetch(`${API_URL}?testnets=false`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ path: 'wallet/transactions', params: { wallet: WALLET, limit: 500, page: 1, filterSpam: true }, }), }); + if (!mobResp.ok) { + throw new Error(`Mobula API request failed: ${mobResp.status}`); + } const mobData = await mobResp.json();validate_pnl.js (4)
4-6: Hardcoded test credentials should be documented or parameterized.Consider accepting the wallet address as a command-line argument or environment variable for flexibility when testing different wallets. At minimum, add a comment indicating this is a test-only value.
+// Test wallet address - change as needed for validation const WALLET = '0x70e8741c1758Ba32176B188286B8086956627B1c';
9-33: DuplicatedcalculatePnLfunction across test scripts.This function is duplicated in
validate_pnl.js,test_portfolio_pnl.ts, andtest_optimized_pnl.js. The production version exists insrc/utils/pnl.ts. For test scripts, consider importing from the production module to ensure consistency and reduce maintenance burden.-// Exact PnL calculation from the codebase -function calculatePnL(trades, currentPrice) { - let totalTokens = 0; - let totalCostUSDC = 0; - // ... function body -} +const { calculatePnL } = require('./src/utils/pnl');
43-43: Silent error swallowing hides debugging information.While tolerant error handling is appropriate for this validation script, completely empty catch blocks make debugging difficult. Consider logging at debug level.
- } catch (e) { } + } catch (e) { + // Silently skip - hash may not exist in Relay + }
51-59: Missing error handling for API response failures.If the API returns an error status or malformed JSON, the script will throw an unhandled exception. Consider adding response status checks.
const txResp = await fetch(`${API_URL}?testnets=false`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ path: 'wallet/transactions', params: { wallet: WALLET, limit: 500, page: 1, filterSpam: true }, }), }); + if (!txResp.ok) { + throw new Error(`Transaction API failed: ${txResp.status}`); + } const txData = await txResp.json();test_portfolio_pnl.ts (1)
1-2: TypeScript file lacks type annotations.This is a
.tsfile but uses@ts-ignoreand no type annotations. Either convert to plain JavaScript or add proper types to leverage TypeScript's safety benefits.test_optimized_pnl.js (2)
8-13: USDC addresses duplicated across multiple test files.This array is defined identically in
test_all_assets_pnl.js,verify_hybrid_pnl.js,test_complete_pnl_flow.js, andtest_usdc_detection.js. Consider extracting to a shared constants file.
86-121: Sequential API calls without rate limiting may hit Relay API limits.Querying Relay for each hash sequentially without any delay could result in rate limiting for wallets with many transactions. Consider adding a small delay or using batch requests if supported.
for (const hash of uniqueHashes) { try { + // Small delay to avoid rate limiting + await new Promise(resolve => setTimeout(resolve, 50)); const relayResp = await fetch(`${RELAY_URL}/requests/v2?hash=${hash}`);test_relay_pnl.ts (1)
40-47: Excessive use ofanytype throughout the file.The file is TypeScript but relies heavily on
anytypes, which provides no type safety. Since related types likeMobulaTransactionRowandRelayRequestexist in the codebase (per the AI summary), consider importing and using them.Also applies to: 75-77
test_relay_matching.ts (2)
4-6: Hardcoded wallet address and API URLs in committed test script.Consider moving the wallet address and API URLs to environment variables or a configuration file. Hardcoded wallet addresses in source control can be a privacy concern, even in test scripts.
-const WALLET_ADDRESS = '0x70e8741c1758Ba32176B188286B8086956627B1c'; -const API_URL = 'https://hifidata-7eu4izffpa-uc.a.run.app'; -const RELAY_API_URL = 'https://api.relay.link'; +const WALLET_ADDRESS = process.env.TEST_WALLET_ADDRESS || ''; +const API_URL = process.env.HIFI_API_URL || 'https://hifidata-7eu4izffpa-uc.a.run.app'; +const RELAY_API_URL = process.env.RELAY_API_URL || 'https://api.relay.link';
12-28: Add error handling for API fetch calls.Both fetch calls lack error handling. If either API is unavailable or returns an error, the script will crash with an unhelpful error message. Consider wrapping in try/catch or checking response status.
// 1. Fetch Mobula transactions - const mobulaResponse = await fetch(`${API_URL}?testnets=false`, { + const mobulaResponse = await fetch(`${API_URL}?testnets=false`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ path: 'wallet/transactions', params: { wallet: WALLET_ADDRESS, limit: 500, page: 1, filterSpam: true }, }), }); + if (!mobulaResponse.ok) { + console.error(`Mobula API error: ${mobulaResponse.status}`); + return; + }src/services/relayApi.ts (1)
130-131: Add blank line between functions for consistency.There's a missing blank line between
fetchRelayRequestByHashandfetchRelayRequestsByUser, unlike the spacing between other functions.return null; } }; + export const fetchRelayRequestsByUser = async (verify_hybrid_pnl.js (1)
91-96: USDC_ADDRESSES constant duplicated across multiple files.This same array appears in
test_complete_pnl_flow.js,test_all_assets_pnl.js, and here. Consider extracting to a shared constants file to avoid maintenance burden and potential inconsistencies.test_all_assets_pnl.js (2)
15-39: LocalcalculatePnLimplementation differs from production version.This local implementation omits realised PnL tracking that exists in
src/utils/pnl.ts(lines 103-179). While acceptable for a test script, consider importing the production function to ensure test behavior matches actual app behavior.+// Import production calculatePnL for consistent behavior +// const { calculatePnL } = require('./src/utils/pnl'); + function calculatePnL(trades, currentPrice) {
113-115: Silent catch block swallows errors without logging.The empty catch block makes it impossible to debug issues with individual Relay API calls. Consider at least logging errors or tracking failure counts.
} catch (e) { - // Skip on error + console.debug(`Relay fetch failed for ${txHash}:`, e.message); }src/hooks/useTokenPnL.ts (5)
29-52: Storingrefetchin state creates unnecessary re-renders and circular updates.The pattern of storing
refetchinside the result state and then updating it via a separateuseEffectis convoluted. Therefetchfunction reference changes on every render becausesetResultin line 45 creates a new closure, causing the effect on line 49-52 to run repeatedly.Consider a simpler approach that avoids storing
refetchin state:- const [result, setResult] = useState<TokenPnLResult>({ - pnl: null, - isLoading: false, - refetch: () => {}, - debug: { - mobulaTxCount: 0, - relayRequestCount: 0, - relayResponseCount: 0, - status: 'Idle', - }, - }); + const [pnlState, setPnlState] = useState<{ + pnl: PnLMetrics | null; + isLoading: boolean; + debug: TokenPnLResult['debug']; + }>({ + pnl: null, + isLoading: false, + debug: { + mobulaTxCount: 0, + relayRequestCount: 0, + relayResponseCount: 0, + status: 'Idle', + }, + }); const [refreshTrigger, setRefreshTrigger] = useState(0); const refetch = useCallback(() => { - setResult((prev) => ({ ...prev, isLoading: true })); + setPnlState((prev) => ({ ...prev, isLoading: true })); setRefreshTrigger((prev) => prev + 1); }, []); - useEffect(() => { - // Update refetch function in result - setResult((prev) => ({ ...prev, refetch })); - }, [refetch]); + + // Return combined result at end of hook + return { ...pnlState, refetch };
222-226: Cleanup function returnsundefinedunnecessarily.The cleanup function in the useEffect should simply set
isMounted = falsewithout returning anything. React cleanup functions don't need an explicit return value.return () => { isMounted = false; - return undefined; }; - }, [props, refreshTrigger]); + }, [props, refreshTrigger]);
148-158: Sequential API calls inPromise.allmay overwhelm the Relay API.While you limit to 20 hashes, firing all requests simultaneously could trigger rate limiting. Consider adding a small delay between batches or implementing proper retry logic with exponential backoff.
- const relayResults = await Promise.all( - hashesToFetch.map(async (hash) => { - try { - const req = await fetchRelayRequestByHash(hash); - return req; - } catch (e) { - console.error(`Failed to fetch Relay for ${hash}:`, e); - return null; - } - }) - ); + // Process in batches of 5 to avoid rate limiting + const BATCH_SIZE = 5; + const relayResults: (RelayRequest | null)[] = []; + + for (let i = 0; i < hashesToFetch.length; i += BATCH_SIZE) { + const batch = hashesToFetch.slice(i, i + BATCH_SIZE); + const batchResults = await Promise.all( + batch.map(async (hash) => { + try { + return await fetchRelayRequestByHash(hash); + } catch (e) { + console.error(`Failed to fetch Relay for ${hash}:`, e); + return null; + } + }) + ); + relayResults.push(...batchResults); + if (!isMounted) return undefined; + }
160-164: RedundantisMountedchecks after awaitingPromise.all.Lines 160 and 164 both check
isMountedand return early. The second check on line 164 is redundant since no async operation occurs between them.if (!isMounted) return undefined; relayRequests.push(...relayResults.filter((req) => req !== null)); - if (!isMounted) return undefined; - if (isMounted)
228-238: Auto-refresh interval may cause unnecessary API calls when tab is not visible.The 30-second interval runs regardless of whether the user is viewing the page, potentially wasting API quota.
Consider pausing the interval when the document is hidden:
useEffect(() => { if (!props) return undefined; + let interval: NodeJS.Timeout; + + const startInterval = () => { + interval = setInterval(() => { + setResult((prev) => ({ ...prev, isLoading: true })); + setRefreshTrigger((prev) => prev + 1); + }, 30000); + }; + + const handleVisibilityChange = () => { + if (document.hidden) { + clearInterval(interval); + } else { + startInterval(); + } + }; + + startInterval(); + document.addEventListener('visibilitychange', handleVisibilityChange); - const interval = setInterval(() => { - setResult((prev) => ({ ...prev, isLoading: true })); - setRefreshTrigger((prev) => prev + 1); - }, 30000); // 30 seconds - return () => clearInterval(interval); + return () => { + clearInterval(interval); + document.removeEventListener('visibilitychange', handleVisibilityChange); + }; }, [props]);test_complete_pnl_flow.js (3)
19-136: Duplicated implementation ofcalculatePnLFromRelay.This function duplicates the logic from
src/utils/pnl.ts(lines 366-507). Code duplication increases maintenance burden and risks divergence.For a test script, consider importing from the source:
// If using ES modules or transpilation: // import { calculatePnLFromRelay } from './src/utils/pnl'; // Or use a shared test utilities approachIf this script must remain standalone (e.g., for quick debugging without build), add a comment explaining why duplication is intentional.
278-285: Race condition when pushing to shared array in parallel.Multiple async callbacks are pushing to
relayRequestsconcurrently insidePromise.all. While JavaScript's single-threaded nature prevents data corruption, the order of insertions is non-deterministic. This may not matter for this script, but be aware if order is important.await Promise.all( batch.map(async (hash) => { const req = await fetchRelayRequestByHash(hash); - if (req) { - relayRequests.push(req); - } + return req; }) - ); + ).then(results => { + relayRequests.push(...results.filter(Boolean)); + });
171-224: No error handling for main async flow.If any API call fails (e.g., network error on portfolio fetch), the script will throw an unhandled promise rejection. Consider wrapping the main flow in try-catch.
(async () => { + try { console.log('=== COMPLETE PNL FLOW TEST ===\n'); // ... rest of the code ... console.log('\n✓ Test Complete!'); + } catch (error) { + console.error('\n✗ Test failed:', error.message); + process.exit(1); + } })();src/utils/pnl.ts (2)
44-47: Native token transactions are silently skipped without tracking gas fees.The comment suggests gas fees should be tracked, but the current implementation discards all native token transactions. This could affect PnL accuracy for trades where significant gas was spent.
Consider at least logging or accumulating gas costs for future implementation:
if (tx.type === 'native') { - // Gas fee usually - return; + // TODO: Track gas fees for more accurate PnL + // feeUsd += tx.amount_usd || 0; + return; }Also applies to: 49-56
182-360:getRelayValidatedTradesmakes unbounded parallel API calls.Unlike
useTokenPnLwhich limits to 20 hashes, this function fetches all unique transaction hashes in parallel (line 227-233). For wallets with many transactions, this could trigger rate limiting or cause performance issues.Consider adding a limit or batching:
const txHashes = Object.keys(groupedByTxHash); + const MAX_RELAY_REQUESTS = 50; + const limitedHashes = txHashes.slice(0, MAX_RELAY_REQUESTS); // Fetch all relay requests in parallel - const relayRequestPromises = txHashes.map(async (txHash) => { + const relayRequestPromises = limitedHashes.map(async (txHash) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apps/pulse/components/Search/tests/__snapshots__/PortfolioTokenList.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (56)
debug_aave.ts(1 hunks)debug_steth_tx.js(1 hunks)debug_steth_tx.ts(1 hunks)find_hashes.ts(1 hunks)inspect_aave_contracts.js(1 hunks)inspect_aave_contracts_full.js(1 hunks)inspect_aave_hashes.js(1 hunks)inspect_aave_portfolio.js(1 hunks)inspect_full_aave.js(1 hunks)inspect_relay.ts(1 hunks)inspect_relay_full.js(1 hunks)src/apps/key-wallet/components/AssetRow.tsx(0 hunks)src/apps/key-wallet/components/AssetsList.tsx(6 hunks)src/apps/key-wallet/components/SearchAssets.tsx(1 hunks)src/apps/key-wallet/components/SendAssetModal.tsx(4 hunks)src/apps/key-wallet/components/TransactionStatus.tsx(0 hunks)src/apps/key-wallet/components/WalletAddress.tsx(1 hunks)src/apps/key-wallet/components/__tests__/AssetRow.test.tsx(2 hunks)src/apps/key-wallet/components/__tests__/SearchAssets.test.tsx(7 hunks)src/apps/key-wallet/components/__tests__/SendAssetModal.test.tsx(3 hunks)src/apps/key-wallet/components/__tests__/TransactionStatus.test.tsx(11 hunks)src/apps/key-wallet/components/__tests__/WalletAddress.test.tsx(6 hunks)src/apps/key-wallet/index.tsx(6 hunks)src/apps/key-wallet/manifest.json(0 hunks)src/apps/key-wallet/styles/tailwindKeyWallet.css(1 hunks)src/apps/key-wallet/tailwind.keywallet.config.js(1 hunks)src/apps/key-wallet/types/index.ts(0 hunks)src/apps/key-wallet/utils/__tests__/blockchain.test.ts(9 hunks)src/apps/key-wallet/utils/blockchain.ts(4 hunks)src/apps/key-wallet/utils/portfolio.ts(0 hunks)src/apps/pulse/components/PnLStats/PnLStats.tsx(1 hunks)src/apps/pulse/components/Search/PortfolioTokenList.tsx(5 hunks)src/apps/pulse/components/Search/TokenPnLCell.tsx(1 hunks)src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx(2 hunks)src/hooks/useTokenPnL.ts(1 hunks)src/services/pillarXApiWalletTransactions.ts(1 hunks)src/services/relayApi.ts(1 hunks)src/store.ts(4 hunks)src/types/api.ts(1 hunks)src/utils/pnl.test.ts(1 hunks)src/utils/pnl.ts(1 hunks)test_all_assets_pnl.js(1 hunks)test_complete_pnl_flow.js(1 hunks)test_final_matching.ts(1 hunks)test_optimized_pnl.js(1 hunks)test_pnl_calculation.ts(1 hunks)test_portfolio_pnl.ts(1 hunks)test_relay_by_user.js(1 hunks)test_relay_matching.ts(1 hunks)test_relay_pnl.ts(1 hunks)test_rtk_response.js(1 hunks)test_usdc_detection.js(1 hunks)validate_pnl.js(1 hunks)verify_aave_pnl.js(1 hunks)verify_hybrid_pnl.js(1 hunks)verify_transactions.ts(1 hunks)
💤 Files with no reviewable changes (5)
- src/apps/key-wallet/components/AssetRow.tsx
- src/apps/key-wallet/utils/portfolio.ts
- src/apps/key-wallet/types/index.ts
- src/apps/key-wallet/manifest.json
- src/apps/key-wallet/components/TransactionStatus.tsx
✅ Files skipped from review due to trivial changes (11)
- src/apps/key-wallet/components/tests/AssetRow.test.tsx
- src/apps/key-wallet/components/tests/SearchAssets.test.tsx
- src/apps/key-wallet/tailwind.keywallet.config.js
- src/apps/key-wallet/components/WalletAddress.tsx
- src/apps/key-wallet/utils/tests/blockchain.test.ts
- src/apps/key-wallet/components/tests/WalletAddress.test.tsx
- src/apps/key-wallet/components/tests/SendAssetModal.test.tsx
- src/apps/key-wallet/components/SearchAssets.tsx
- src/apps/key-wallet/index.tsx
- src/apps/key-wallet/components/AssetsList.tsx
- src/apps/key-wallet/styles/tailwindKeyWallet.css
🚧 Files skipped from review as they are similar to previous changes (5)
- src/services/pillarXApiWalletTransactions.ts
- src/apps/pulse/components/Search/TokenPnLCell.tsx
- src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx
- src/types/api.ts
- src/utils/pnl.test.ts
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/store.tssrc/apps/key-wallet/components/SendAssetModal.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/PnLStats/PnLStats.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/PnLStats/PnLStats.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/key-wallet/components/SendAssetModal.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/hooks/useTokenPnL.ts
📚 Learning: 2025-04-23T15:04:20.826Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 290
File: src/apps/pillarx-app/components/TileTitle/TitleTitle.tsx:6-10
Timestamp: 2025-04-23T15:04:20.826Z
Learning: In this repository, TileTitleProps and TileTitle are different types that serve different purposes. TileTitleProps is used for the TileTitle component and has optional fields (title?, leftDecorator?, rightDecorator?), while TileTitle in api.ts has a required text field. The TileTitleProps interface aligns with the TokensMarketData.title type in api.ts which also has optional fields.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-04-23T15:04:20.826Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 290
File: src/apps/pillarx-app/components/TileTitle/TitleTitle.tsx:6-10
Timestamp: 2025-04-23T15:04:20.826Z
Learning: In this repository, TileTitleProps and TileTitle are different types that serve different purposes. TileTitleProps is used for the TileTitle component and has optional fields (title?, leftDecorator?, rightDecorator?), while TileTitle in api.ts has a required title field. The TileTitleProps structure aligns with how it's used in the TokensMarketData type in api.ts.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsx
🧬 Code graph analysis (26)
test_usdc_detection.js (2)
test_complete_pnl_flow.js (1)
USDC_ADDRESSES(23-30)test_optimized_pnl.js (1)
USDC_ADDRESSES(8-13)
debug_steth_tx.ts (2)
verify_hybrid_pnl.js (1)
req(78-78)src/services/relayApi.ts (1)
fetchRelayRequestByHash(107-130)
inspect_aave_contracts_full.js (1)
inspect_aave_portfolio.js (6)
fetch(1-1)WALLET(2-2)resp(5-12)data(13-13)assets(14-14)aave(16-16)
src/store.ts (1)
src/services/pillarXApiWalletTransactions.ts (2)
pillarXApiWalletTransactions(21-49)pillarXApiWalletTransactions(51-51)
src/apps/key-wallet/components/__tests__/TransactionStatus.test.tsx (1)
src/apps/key-wallet/types/index.ts (1)
TransactionStatus(21-26)
find_hashes.ts (1)
test_relay_by_user.js (1)
fetch(1-1)
test_final_matching.ts (4)
inspect_aave_contracts.js (3)
fetch(1-1)txs(22-22)data(21-21)test_usdc_detection.js (4)
fetch(2-2)relayResp(16-16)relayData(17-17)hasUSDC(22-22)validate_pnl.js (3)
fetch(2-2)data(39-39)hasUSDC(92-92)verify_hybrid_pnl.js (3)
fetch(1-1)req(78-78)data(8-8)
inspect_full_aave.js (2)
inspect_aave_contracts_full.js (6)
fetch(1-1)WALLET(2-2)resp(5-12)data(13-13)assets(14-14)aave(16-16)inspect_aave_portfolio.js (6)
fetch(1-1)WALLET(2-2)resp(5-12)data(13-13)assets(14-14)aave(16-16)
inspect_aave_hashes.js (8)
inspect_aave_contracts.js (6)
fetch(1-1)WALLET(2-2)resp(7-19)data(21-21)txs(22-22)aaveTxs(24-24)inspect_full_aave.js (4)
fetch(1-1)WALLET(2-2)resp(5-12)data(13-13)inspect_relay_full.js (2)
fetch(1-1)data(7-7)test_all_assets_pnl.js (3)
fetch(2-2)WALLET(4-4)group(90-90)test_complete_pnl_flow.js (4)
fetch(1-1)WALLET(2-2)data(8-8)i(274-274)test_relay_by_user.js (4)
fetch(1-1)WALLET(2-2)resp(7-7)data(10-10)validate_pnl.js (4)
fetch(2-2)WALLET(4-4)data(39-39)aaveTxs(78-78)verify_aave_pnl.js (3)
fetch(1-1)WALLET(2-2)data(9-9)
test_relay_matching.ts (3)
test_all_assets_pnl.js (2)
fetch(2-2)API_URL(5-5)test_relay_by_user.js (1)
fetch(1-1)test_usdc_detection.js (1)
fetch(2-2)
src/apps/pulse/components/PnLStats/PnLStats.tsx (5)
src/types/api.ts (1)
PnLMetrics(815-826)test_all_assets_pnl.js (3)
usdValue(167-167)unrealisedPnLUSDC(35-35)unrealisedPnLPct(36-36)test_optimized_pnl.js (3)
usdValue(134-134)unrealisedPnLUSDC(35-35)unrealisedPnLPct(36-36)test_complete_pnl_flow.js (2)
unrealisedPnLUSDC(157-157)unrealisedPnLPct(158-158)validate_pnl.js (2)
unrealisedPnLUSDC(29-29)unrealisedPnLPct(30-30)
inspect_relay.ts (16)
debug_steth_tx.js (1)
fetch(2-2)inspect_aave_contracts.js (1)
fetch(1-1)inspect_aave_contracts_full.js (1)
fetch(1-1)inspect_aave_hashes.js (1)
fetch(1-1)inspect_aave_portfolio.js (1)
fetch(1-1)inspect_full_aave.js (1)
fetch(1-1)inspect_relay_full.js (1)
fetch(1-1)test_all_assets_pnl.js (1)
fetch(2-2)test_complete_pnl_flow.js (2)
fetch(1-1)i(274-274)test_optimized_pnl.js (1)
fetch(2-2)test_relay_by_user.js (1)
fetch(1-1)test_rtk_response.js (1)
fetch(1-1)test_usdc_detection.js (1)
fetch(2-2)validate_pnl.js (1)
fetch(2-2)verify_aave_pnl.js (1)
fetch(1-1)verify_hybrid_pnl.js (1)
fetch(1-1)
src/services/relayApi.ts (7)
test_complete_pnl_flow.js (2)
userAddress(70-70)response(6-6)verify_aave_pnl.js (2)
userAddress(102-102)response(7-7)verify_hybrid_pnl.js (2)
userAddress(144-144)response(6-6)debug_steth_tx.js (1)
response(10-10)inspect_relay_full.js (1)
response(6-6)test_rtk_response.js (1)
response(8-15)validate_pnl.js (2)
response(37-37)requests(40-40)
inspect_aave_portfolio.js (2)
inspect_aave_contracts_full.js (6)
fetch(1-1)WALLET(2-2)resp(5-12)data(13-13)assets(14-14)aave(16-16)inspect_full_aave.js (6)
fetch(1-1)WALLET(2-2)resp(5-12)data(13-13)assets(14-14)aave(16-16)
inspect_relay_full.js (3)
test_complete_pnl_flow.js (3)
fetch(1-1)response(6-6)data(8-8)test_relay_by_user.js (2)
fetch(1-1)data(10-10)verify_aave_pnl.js (3)
fetch(1-1)response(7-7)data(9-9)
src/apps/key-wallet/components/SendAssetModal.tsx (1)
src/types/walletProvider.ts (1)
WalletProviderLike(15-15)
test_all_assets_pnl.js (3)
test_complete_pnl_flow.js (7)
USDC_ADDRESSES(23-30)trades(20-20)trades(295-295)unrealisedPnLUSDC(157-157)unrealisedPnLPct(158-158)side(35-35)pnl(304-304)verify_hybrid_pnl.js (3)
USDC_ADDRESSES(91-96)trades(89-89)side(101-101)src/utils/pnl.ts (1)
calculatePnL(104-180)
src/apps/key-wallet/utils/blockchain.ts (2)
src/types/walletProvider.ts (1)
Eip1193LikeProvider(8-13)src/utils/blockchain.ts (1)
getBlockScan(225-246)
test_pnl_calculation.ts (2)
test_relay_by_user.js (1)
WALLET(2-2)src/utils/pnl.ts (1)
calculatePnL(104-180)
test_complete_pnl_flow.js (2)
src/services/relayApi.ts (1)
fetchRelayRequestByHash(107-130)src/utils/pnl.ts (1)
calculatePnLFromRelay(366-508)
test_optimized_pnl.js (4)
test_all_assets_pnl.js (9)
USDC_ADDRESSES(8-13)trades(42-42)unrealisedPnLUSDC(35-35)unrealisedPnLPct(36-36)assets(134-134)addr(76-76)price(165-165)balance(166-166)pnl(177-177)test_complete_pnl_flow.js (7)
USDC_ADDRESSES(23-30)trades(20-20)trades(295-295)unrealisedPnLUSDC(157-157)unrealisedPnLPct(158-158)assets(186-186)pnl(304-304)test_usdc_detection.js (1)
USDC_ADDRESSES(6-11)verify_hybrid_pnl.js (3)
USDC_ADDRESSES(91-96)trades(89-89)assets(32-32)
verify_transactions.ts (2)
test_relay_by_user.js (1)
fetch(1-1)test_usdc_detection.js (1)
fetch(2-2)
src/utils/pnl.ts (3)
src/types/api.ts (3)
MobulaTransactionRow(768-790)ReconstructedTrade(803-813)PnLMetrics(815-826)src/services/relayApi.ts (2)
RelayRequest(3-77)fetchRelayRequestByHash(107-130)test_usdc_detection.js (2)
USDC_ADDRESSES(6-11)tokenAddr(28-28)
verify_aave_pnl.js (1)
src/services/relayApi.ts (1)
fetchRelayRequestByHash(107-130)
src/hooks/useTokenPnL.ts (4)
src/services/tokensData.ts (1)
Token(19-29)src/types/api.ts (2)
WalletTransactionsMobulaResponse(792-801)PnLMetrics(815-826)src/utils/pnl.ts (2)
calculatePnL(104-180)calculatePnLFromRelay(366-508)src/services/relayApi.ts (2)
RelayRequest(3-77)fetchRelayRequestByHash(107-130)
debug_steth_tx.js (1)
src/services/relayApi.ts (1)
fetchRelayRequestByHash(107-130)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
Destructured props and updated useEffect dependency array to use specific stable values (token.contract, walletAddress, etc.) instead of the unstable props object. This prevents infinite re-rendering loops caused by TokenPnLCell creating a new props object on every render. Also fixed a TypeScript error by using token.contract instead of token.address.
github-actions
bot
requested a deployment
to
Preview (feature-pnl)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/hooks/useTokenPnL.ts (1)
241-251: Dependency array still causes excessive re-renders.While this approach improves on using the entire
propsobject, line 247 still includesprops.transactionsData, which is a full object reference. This will trigger the effect whenevertransactionsDatareference changes, even if its content is identical—potentially causing excessive API calls.Destructure at the effect's start and use stable identifiers:
}, [ - // Use specific dependencies to avoid infinite loops from unstable props object - props ? props.token.contract : null, - props ? props.token.symbol : null, - props ? props.token.balance : null, - props ? props.token.price : null, - props ? props.transactionsData : null, - props ? props.walletAddress : null, - props ? props.chainId : null, - refreshTrigger + props?.token.contract, + props?.token.symbol, + props?.token.balance, + props?.token.price, + // Only depend on transaction count, not the entire object + props?.transactionsData?.data?.transactions?.length, + props?.walletAddress, + props?.chainId, + refreshTrigger, ]);Alternatively, require callers to memoize
transactionsDatawithuseMemoif the full object is needed.
🧹 Nitpick comments (2)
src/hooks/useTokenPnL.ts (2)
74-81: Remove commented debug code.This commented-out console.log block should be removed to keep the codebase clean.
Apply this diff:
- // Debug log (can remove later) - /* - console.log(`[useTokenPnL] Hook called for ${token?.symbol}`, { - hasTransactionsData: !!transactionsData, - txCount: transactionsData?.data?.transactions?.length, - walletAddress, - chainId - }); - */ -
68-68: Consider simplifying return statements.Explicit
return undefinedcan be simplified to justreturn;for cleaner code. This pattern appears at lines 68, 93, 105, 231, and 239.Also applies to: 93-93, 105-105, 231-231, 239-239
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/pulse/components/Search/TokenPnLCell.tsx(1 hunks)src/hooks/useTokenPnL.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/pulse/components/Search/TokenPnLCell.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/hooks/useTokenPnL.ts
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/hooks/useTokenPnL.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (2)
src/hooks/useTokenPnL.ts (2)
1-27: LGTM! Clean type definitions.The imports and interface definitions are well-structured and provide clear contracts for the hook's inputs and outputs.
29-52: LGTM! Solid refetch mechanism.The use of
useCallbackfor therefetchfunction and the separaterefreshTriggerstate provides a clean manual refresh capability.
- Move 'Hide <$0.5' toggle from PortfolioTokenList to Search.tsx header - Toggle appears next to 'My Holdings' text in Sell mode - Filters tokens with balance < $0.5 when enabled - Fix test failures by adding complete RTK Query API mocks - Add TestWrapper to Sell.test.tsx, HomeScreen.test.tsx, AppWrapper.test.tsx - Add mocks for useTokenPnL, pillarXApiWalletTransactions, useRelayBuy, useRemoteConfig - Update test snapshots for all modified components Test Results: - 946/951 tests passing (99.5% pass rate) - Fixed 68 test failures across 4 test files - UI manually verified and working correctly
github-actions
bot
temporarily deployed
to
Preview (feature/pulse-search-ui-mobile-refinements)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/apps/pulse/components/Search/PortfolioTokenList.tsx (3)
5-9: Clean up unused PnL-related imports
PnLMetrics,WalletTransactionsMobulaResponse,fetchRelayRequestByHash,RelayRequest, and thepnlutility imports are not used in this component. Removing them will avoid lint noise and keep the bundle leaner.-import { - PortfolioData, - PnLMetrics, - WalletTransactionsMobulaResponse, -} from '../../../../types/api'; +import { PortfolioData } from '../../../../types/api'; @@ -import { - fetchRelayRequestByHash, - RelayRequest, -} from '../../../../services/relayApi'; - -// utils -import { - reconstructTrades, - calculatePnL as calculatePnLMetrics, - calculatePnLFromRelay, -} from '../../../../utils/pnl'; +// (relayApi and pnl utilities can be re-imported here when needed)Also applies to: 33-43
76-81: Remove debugconsole.logfrom production componentThe
console.loginPortfolioTokenListwill fire on every render and spam logs in production. It should either be removed or gated behind an explicit debug flag/env check.- console.log('[PortfolioTokenList] Component rendering!', { - propsKeys: Object.keys(props), - hasPortfolioData: !!props.walletPortfolioData, - });
92-103: Consolidate sorting and clarify PnL sort behaviorRight now:
getFilteredPortfolioTokenssortstokensusingsortConfig, andsortedTokensimmediately re-sortsportfolioTokensusingsortConfigagain, while- The
'pnl'branch insortedTokenssetsvalA/valBto0, so PnL sorting is effectively a no-op even though the header and indicator are clickable.This double‑sorting adds complexity with no benefit, and the PnL column currently suggests a working sort that doesn’t actually reorder rows.
Consider:
- Removing the sort from
getFilteredPortfolioTokensso it only filters, and- Keeping all sorting logic in
sortedTokens(or disabling the PnL header click/indicator until real PnL sorting is wired).const getFilteredPortfolioTokens = () => { @@ - // Apply sorting - tokens = tokens.sort((a: Token, b: Token) => { - let valA = 0; - let valB = 0; - - switch (sortConfig.key) { - case 'price': - return a.symbol.localeCompare(b.symbol); - case 'balance': - valA = (a.price || 0) * (a.balance || 0); - valB = (b.price || 0) * (b.balance || 0); - break; - case 'pnl': - valA = (a.price || 0) * (a.balance || 0); - valB = (b.price || 0) * (b.balance || 0); - break; - } - return sortConfig.direction === 'asc' ? valA - valB : valB - valA; - }); + // Leave sorting to `sortedTokens` so filtering is isolated here.And if you don’t plan to support PnL sort yet, you could also temporarily disable the header click:
- <div - className="flex items-center justify-end flex-[1.8] cursor-pointer gap-1.5" - onClick={() => handleSort('pnl')} + <div + className="flex items-center justify-end flex-[1.8] gap-1.5" >Also applies to: 117-158, 174-203, 305-311
🧹 Nitpick comments (2)
src/apps/pulse/components/Search/tests/PortfolioTokenList.test.tsx (1)
16-35: Mocks and newhideSmallBalancesprop usage look soundThe added mocks for
useTransactionKitanduseGetWalletTransactionsQueryplus the newhideSmallBalancesprop indefaultPropsintegrate cleanly with the updatedPortfolioTokenListAPI. Explicitly disabling the filter in the “null price” test (hideSmallBalances={false}) is a good way to cover that edge case.If you want fuller coverage later, you could add a small test that verifies a token just under the
$0.5threshold disappears whenhideSmallBalancesistrue, but this isn’t blocking.Also applies to: 98-105, 321-362
src/apps/pulse/components/Search/Search.tsx (1)
451-460: PasswalletPortfolioFetchingthrough toPortfolioTokenList.isFetching
PortfolioTokenListnow supports anisFetchingprop and rendersSkeletonTokenRows plus opacity changes when it’s true, butSearchdoesn’t currently passwalletPortfolioFetchingdown, so that refresh skeleton path will never activate.Consider wiring it like this so auto-refresh and manual refresh show the intended skeleton state:
{searchType === SearchType.MyHoldings && ( <div className="flex flex-col"> <PortfolioTokenList walletPortfolioData={walletPortfolioData} handleTokenSelect={handleTokenSelect} isLoading={walletPortfolioLoading} isError={walletPortfolioError} searchText={searchText} - hideSmallBalances={hideSmallBalances} + hideSmallBalances={hideSmallBalances} + isFetching={walletPortfolioFetching} /> </div> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
src/apps/pulse/components/App/tests/__snapshots__/AppWrapper.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/App/tests/__snapshots__/HomeScreen.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Buy/tests/__snapshots__/Buy.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Search/tests/__snapshots__/PortfolioTokenList.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Sell/tests/__snapshots__/Sell.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
src/apps/pulse/components/App/tests/AppWrapper.test.tsx(1 hunks)src/apps/pulse/components/App/tests/HomeScreen.test.tsx(1 hunks)src/apps/pulse/components/Buy/tests/Buy.test.tsx(2 hunks)src/apps/pulse/components/Search/PortfolioTokenList.tsx(5 hunks)src/apps/pulse/components/Search/Search.tsx(4 hunks)src/apps/pulse/components/Search/tests/PortfolioTokenList.test.tsx(3 hunks)src/apps/pulse/components/Sell/tests/Sell.test.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/components/Sell/tests/Sell.test.tsxsrc/apps/pulse/components/Buy/tests/Buy.test.tsxsrc/apps/pulse/components/App/tests/AppWrapper.test.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/apps/pulse/components/Search/tests/PortfolioTokenList.test.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Search/Search.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/apps/pulse/components/Search/tests/PortfolioTokenList.test.tsx
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/Search/Search.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/Search/Search.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/apps/pulse/components/Search/tests/PortfolioTokenList.test.tsx
📚 Learning: 2025-04-23T15:04:20.826Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 290
File: src/apps/pillarx-app/components/TileTitle/TitleTitle.tsx:6-10
Timestamp: 2025-04-23T15:04:20.826Z
Learning: In this repository, TileTitleProps and TileTitle are different types that serve different purposes. TileTitleProps is used for the TileTitle component and has optional fields (title?, leftDecorator?, rightDecorator?), while TileTitle in api.ts has a required text field. The TileTitleProps interface aligns with the TokensMarketData.title type in api.ts which also has optional fields.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsx
🧬 Code graph analysis (2)
src/apps/pulse/components/Sell/tests/Sell.test.tsx (1)
src/test-utils/testUtils.tsx (1)
TestWrapper(116-134)
src/apps/pulse/components/Search/PortfolioTokenList.tsx (5)
src/types/api.ts (1)
PortfolioData(739-748)src/services/tokensData.ts (2)
Token(19-29)chainNameToChainIdTokensData(234-255)src/services/pillarXApiWalletPortfolio.ts (1)
convertPortfolioAPIResponseToToken(27-61)src/utils/blockchain.ts (1)
getLogoForChainId(165-199)src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (6)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (2)
27-107: Mock wiring for PnL/Relay/remote-config looks consistentThe added mocks for
useTokenPnL, wallet transactions, Relay hooks, search tokens, anduseRemoteConfigmirror patterns used elsewhere and should keep Buy tests isolated from network/state. Just keep an eye on these mock shapes if the real hooks’ signatures change so tests don’t silently diverge from production behavior.If you later change any of these hooks’ return signatures, update the mocks here in lockstep to avoid subtle test breakages.
474-481: More robust “missing portfolio data” assertionSwitching to
getAllByText('$0.00')with a count check is a good way to tolerate multiple$0.00renderings while still asserting the empty-wallet state.src/apps/pulse/components/Sell/tests/Sell.test.tsx (1)
16-50: Sell tests correctly wired to new PnL and wallet-transactions plumbingWrapping
<Sell />inTestWrapperand adding mocks foruseTokenPnL,useGetWalletTransactionsQuery, anduseRemoteConfigkeeps these tests aligned with the new infrastructure without pulling in real network state. The existing interaction and state tests remain valid with these additions.Also applies to: 129-135
src/apps/pulse/components/App/tests/HomeScreen.test.tsx (1)
32-73: HomeScreen tests now isolated from PnL/Relay and wallet-transactions side effectsThe new mocks for
useTokenPnL,useGetWalletTransactionsQuery, Relay hooks, anduseRemoteConfiggive you deterministic HomeScreen renders while the real PnL/Relay logic evolves. This is a solid approach; just ensure these mocked return shapes stay in sync with the underlying hooks.If you later extend
useTokenPnLor the wallet-transactions hook (e.g., extra flags or fields), mirror those in these mocks so assertions still cover realistic states.src/apps/pulse/components/App/tests/AppWrapper.test.tsx (1)
36-77: AppWrapper tests aligned with new PnL and Relay wiringMocking
useTokenPnL,useGetWalletTransactionsQuery, Relay hooks, anduseRemoteConfighere mirrors the approach in other Pulse tests and cleanly decouples routing/UI assertions from data-layer changes. This should make future PnL/Relay tweaks less brittle for these tests.src/apps/pulse/components/Search/Search.tsx (1)
85-88: Hide-small-balances state and toggle are wired correctly for MyHoldingsThe
hideSmallBalancesstate, sell-only “Hide <$0.5” toggle, and propagation of that flag intoPortfolioTokenListforSearchType.MyHoldingsare coherent and should give a clear UX for trimming tiny positions while selling.Also applies to: 373-388, 451-460
- Move PnL Stats in Buy.tsx to appear after buy button (matching Sell layout) - Remove debug console.log from Sell.tsx - Update @mui/joy to 5.0.0-beta.48 to resolve @mui/base deprecation - Add eslint override to force v8.57.0 across all dependencies - Fix Search.test.tsx Redux context errors by removing Provider and using proper mocks - Update test snapshots for PortfolioTokenList
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/apps/pulse/components/Sell/Sell.tsx (1)
103-122: Type casting bypasses type safety; consider defining proper types.The
as anycasts suppress TypeScript's type checking. While this works, it reduces maintainability and can hide type mismatches.Additionally, there's an inconsistency:
Sell.tsxrequiresportfolioTokento be present (line 104), whileBuy.tsx(lines 177-192) does not useportfolioTokenand thus passes potentially undefinedbalanceandpricevalues. This could lead to incorrect PnL calculations in the Buy component.Consider defining a proper interface for the
useTokenPnLprops:- const { pnl, isLoading: isPnLLoading } = useTokenPnL( - token && accountAddress && portfolioToken - ? ({ - token: { - ...token, - contract: token.address || '', - id: token.symbol, - blockchain: 'ethereum', - balance: portfolioToken.balance || 0, - price: portfolioToken.price || 0, - decimals: token.decimals || 18, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any, - transactionsData, - walletAddress: accountAddress, - chainId: token.chainId, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - : null - ); + const { pnl, isLoading: isPnLLoading } = useTokenPnL( + token && accountAddress && portfolioToken + ? { + token: { + contract: token.address || '', + symbol: token.symbol, + decimals: token.decimals || 18, + balance: portfolioToken.balance || 0, + price: portfolioToken.price || 0, + }, + transactionsData, + walletAddress: accountAddress, + chainId: token.chainId, + } + : null + );This requires updating the
UseTokenPnLPropsinterface inuseTokenPnL.tsto accept this structure.src/apps/pulse/components/Buy/Buy.tsx (1)
810-813: Consider a type-safe union forexpressIntentResponseprop.The
as anycast is used to passbuyOfferwhen using Relay Buy. While this works, a cleaner approach would be to update theBuyButtonprop type to accept a union type:expressIntentResponse: ExpressIntentResponse | BuyOffer | nullThis would maintain type safety while supporting both flows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/apps/pulse/components/Search/tests/__snapshots__/PortfolioTokenList.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
package.json(2 hunks)src/apps/pulse/components/Buy/Buy.tsx(7 hunks)src/apps/pulse/components/Search/tests/Search.test.tsx(15 hunks)src/apps/pulse/components/Sell/Sell.tsx(10 hunks)src/apps/pulse/components/Sell/tests/Sell.test.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/pulse/components/Sell/tests/Sell.test.tsx
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Sell/Sell.tsx
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Sell/Sell.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Sell/Sell.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Search/tests/Search.test.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Search/tests/Search.test.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsx
🧬 Code graph analysis (3)
src/apps/pulse/components/Buy/Buy.tsx (2)
src/apps/pulse/utils/blockchain.ts (1)
isNativeToken(17-18)src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
PnLStats(114-205)
src/apps/pulse/components/Sell/Sell.tsx (4)
src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/hooks/useTokenPnL.ts (1)
useTokenPnL(29-266)src/apps/pulse/utils/blockchain.ts (1)
isNativeToken(17-18)src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
PnLStats(114-205)
src/apps/pulse/components/Search/tests/Search.test.tsx (2)
src/theme/index.ts (1)
defaultTheme(83-163)src/apps/pulse/components/Search/Search.tsx (1)
Search(64-475)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (12)
src/apps/pulse/components/Search/tests/Search.test.tsx (3)
134-140: Good refactor for test consistency.The
renderWithProvidershelper is a clean pattern that reduces boilerplate and ensures all tests have consistent theming and routing context.
148-159: LGTM: Snapshot test now includes theme context.Adding
ThemeProviderto the snapshot test ensures that theme-dependent styling is captured in the snapshot, making it more comprehensive.
162-344: Consistent test scaffolding improvements.All test cases have been consistently updated to use
renderWithProviders, ensuring uniform theming and routing context across the test suite. This improves maintainability and consistency.src/apps/pulse/components/Sell/Sell.tsx (3)
92-100: LGTM!The
portfolioTokenlookup correctly matches by both contract address (case-insensitive) and chain ID, with proper null guards.
84-89: LGTM!The transactions query correctly skips when
accountAddressis unavailable, preventing unnecessary API calls.
617-628: LGTM!The conditional rendering correctly shows the skeleton during loading and only displays PnL data when there's actual trading history. The short-circuit evaluation properly guards against null
pnlaccess.src/apps/pulse/components/Buy/Buy.tsx (3)
169-174: LGTM!The transactions query pattern is consistent with
Sell.tsxand correctly handles the skip condition.
831-842: PnL rendering logic is correct but will rarely display due to upstream issue.The conditional rendering pattern is correct and consistent with
Sell.tsx. However, due to the missingbalanceandpriceproperties flagged above (lines 176-192), thepnlobject will typically be null (hook returns early with "Small Balance" status), so this section will only flash during loading and then disappear.Once the upstream issue is fixed, this section will work as intended.
779-782: Minor formatting adjustments look fine.The className template literal formatting is consistent with the codebase style.
package.json (3)
168-168: viem override is consistent with dependency version.The viem override at line 168 correctly pins version 2.37.1, which matches the dependency at line 87. No action needed.
121-121: <function_calls>
#!/bin/bashRead package.json lines around 121 and 169 to verify eslint version
echo "=== package.json lines 120-125 ==="
sed -n '120,125p' package.jsonecho ""
echo "=== package.json lines 168-172 ==="
sed -n '168,172p' package.jsonecho ""
echo "=== Looking for eslint config files ==="
find . -maxdepth 2 -type f ( -name '.eslintrc*' -o -name 'eslint.config.*' ) 2>/dev/null | head -20echo ""
echo "=== Checking if eslint config is in package.json ==="
grep -A 20 '"eslintConfig"' package.json | head -30
</function_calls>#!/bin/bash # Examine a few key new PnL files to understand their content echo "=== Examining src/utils/pnl.ts ===" wc -l src/utils/pnl.ts echo "" echo "=== Examining src/hooks/useTokenPnL.ts ===" wc -l src/hooks/useTokenPnL.ts echo "" echo "=== Examining src/services/relayApi.ts ===" wc -l src/services/relayApi.ts echo "" echo "=== Checking for obvious syntax issues in pnl.ts ===" head -50 src/utils/pnl.ts </parameter> </invoke> </function_calls> --- `30-30`: **No testing required for @mui/joy version bump—new PnL components don't use it.** The new PnL components (PnLStats, TokenPnLCell) are built with Tailwind CSS, not @mui/joy, so the version bump from 5.0.0-beta.22 to 5.0.0-beta.48 does not affect them. Additionally, the changelog between these beta versions contains only bug fixes and documentation updates with no breaking changes. The existing @mui/joy usage elsewhere in the codebase (AssetSelect, fear-and-greed app) remains compatible with beta.48. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/hooks/useTokenPnL.ts (1)
269-279: Auto-refresh interval still depends on unstablepropsobject.The interval effect depends on the entire
propsobject reference, which changes on every parent render. This causes the interval to be cleared and recreated unnecessarily.Apply the fix suggested in the previous review:
useEffect(() => { if (!props) return undefined; const interval = setInterval(() => { setResult((prev) => ({ ...prev, isLoading: true })); setRefreshTrigger((prev) => prev + 1); }, 30000); // 30 seconds return () => clearInterval(interval); - }, [props]); + }, [!!props]);src/apps/pulse/components/Search/tests/Search.test.tsx (1)
7-8: Remove unused imports.
ProviderandconfigureStoreare imported but never used in this test file.-import { Provider } from 'react-redux'; -import { configureStore } from '@reduxjs/toolkit'; import { ThemeProvider } from 'styled-components';src/apps/pulse/components/Search/TokenPnLCell.tsx (1)
51-63: Negative PnL values don't display explicit minus sign.For negative
unrealisedPnLUSDC, the cell renders$5.00(in red) without a minus sign, relying solely on color to indicate loss. This is inconsistent with typical financial displays and could be confusing.As noted in a previous review, compute signs explicitly:
+ const usdSign = + pnl.unrealisedPnLUSDC > 0 ? '+' : pnl.unrealisedPnLUSDC < 0 ? '-' : ''; + const pctSign = + pnl.unrealisedPnLPct > 0 ? '+' : pnl.unrealisedPnLPct < 0 ? '-' : ''; + return ( <div className="flex flex-col items-end"> <p className={`text-[13px] font-normal tracking-[-0.26px] text-right ${ pnl.unrealisedPnLUSDC >= 0 ? 'text-[#4ADE80]' : 'text-[#EF4444]' }`} > - {pnl.unrealisedPnLUSDC >= 0 ? '+' : ''}$ + {usdSign}$ {formatExponentialSmallNumber( limitDigitsNumber(Math.abs(pnl.unrealisedPnLUSDC)) )} </p> <p className={`text-xs font-normal tracking-[-0.24px] text-right ${ pnl.unrealisedPnLPct >= 0 ? 'text-[#4ADE80]' : 'text-[#EF4444]' }`} > - {pnl.unrealisedPnLPct >= 0 ? '+' : ''} - {pnl.unrealisedPnLPct.toFixed(2)}% + {pctSign} + {Math.abs(pnl.unrealisedPnLPct).toFixed(2)}% </p> </div> );src/apps/pulse/components/Search/PortfolioTokenList.tsx (1)
130-193: Consolidate sorting and clarifypnlsort behaviorRight now sorting is applied twice and
pnlsorting is misleading:
getFilteredPortfolioTokenssortstokens(lines 145-178) based onsortConfig, thensortedTokensre-sortsportfolioTokensagain (lines 198-227), with the'pnl'case settingvalA/valBto0, so that comparator is effectively a no‑op.- For
sortConfig.key === 'pnl', the final ordering ends up coming from the first sort (by balance USD), while the UI header and code comments suggest either PnL-based sorting or disabled sorting.This is both redundant work and confusing behavior for users.
I’d recommend:
- Remove sorting from
getFilteredPortfolioTokensso it only handles filtering/search, and- Keep all sorting centralized in
sortedTokens(or vice versa), and- Either:
- Implement a real PnL sort once metrics are available at this level, or
- Temporarily disable sorting for the PnL column (no click handler / indicator) or make it explicitly sort by balance USD as a documented proxy.
Example of removing the first sort block:
- // Apply sorting - tokens = tokens.sort((a: Token, b: Token) => { - let valA = 0; - let valB = 0; - - switch (sortConfig.key) { - case 'price': - return a.symbol.localeCompare(b.symbol); - case 'balance': - valA = (a.price || 0) * (a.balance || 0); - valB = (b.price || 0) * (b.balance || 0); - break; - case 'pnl': - valA = (a.price || 0) * (a.balance || 0); - valB = (b.price || 0) * (b.balance || 0); - break; - default: - valA = (a.price || 0) * (a.balance || 0); - valB = (b.price || 0) * (b.balance || 0); - break; - } - return sortConfig.direction === 'asc' ? valA - valB : valB - valA; - });and then adjust
sortedTokens/ the PnL header to reflect the actual intended behavior.Also applies to: 198-227, 329-390
🧹 Nitpick comments (7)
src/hooks/useTokenPnL.ts (3)
42-47: Remove console.log statements before merging.Debug logging on every render will pollute the console in production. Consider using a debug flag or removing these logs entirely.
- // eslint-disable-next-line no-console - console.log('[useTokenPnL] Hook render', { - tokenSymbol: props?.token?.symbol, - isLoading: result.isLoading, - pnl: result.pnl, - });
190-196: RedundantisMountedcheck after early return.Line 190 returns early if
!isMounted, so the check on line 192 is always true and unnecessary.if (!isMounted) return; - if (isMounted) - setResult((prev) => ({ + setResult((prev) => ({ ...prev, debug: { ...prev.debug, relayResponseCount: relayRequests.length }, - })); + }));
104-108: Additional console.log statements in async flow.These debug statements should also be removed for production code.
- // eslint-disable-next-line no-console - console.log('[useTokenPnL] calculatePnL start', { - hasTransactions: !!transactions, - walletAddress: !!walletAddress, - });- // eslint-disable-next-line no-console - console.log('[useTokenPnL] Calculation complete', pnlMetrics);Also applies to: 236-237
src/apps/pulse/components/Search/tests/Search.test.tsx (1)
24-26: Unused import:pillarXApiWalletTransactions.The named import
pillarXApiWalletTransactionson line 25 is imported but not used in the test file. It's only defined in the mock.-// services -import { pillarXApiWalletTransactions } from '../../../../../services/pillarXApiWalletTransactions';src/apps/pulse/components/Search/TokenPnLCell.tsx (1)
29-43: Consider deduplicating identical loading and no-data states.Both
isLoadingand!pnlconditions render the same JSX. This could be simplified.- if (isLoading) { - return ( - <div className="flex flex-col items-end"> - <p className="text-xs text-white/30">-</p> - </div> - ); - } - - if (!pnl) { + if (isLoading || !pnl) { return ( <div className="flex flex-col items-end"> <p className="text-xs text-white/30">-</p> </div> ); }src/apps/pulse/components/Search/PortfolioTokenList.tsx (2)
229-231: Remove stale commented-out PnL summary logicThese comments refer to a removed “Total Portfolio PnL” summary box and even quote a prior user conversation. They don’t add value for future maintainers and can be safely deleted to keep the component clean.
- // Calculate Total Portfolio PnL (Removed as per new design, but keeping logic if needed later? No, remove to clean up) - // The user explicitly said "nope its not here", implying the summary box shouldn't be there.
471-479: Clean up stray PnL-related comments in the price cellThe inline comments about “Skip PnL for small balances” and
valueUSDare unrelated to the token price display and look like leftovers from earlier iterations. They can confuse future readers.Consider removing this block entirely and leaving only the price formatting:
- ${/* // Skip PnL for small balances */} - {/* if (valueUSD < 0.01) { */} - {/* return { ...token, pnl: null }; */} - {/* } */} - {token.price + $ + {token.price ? formatExponentialSmallNumber( limitDigitsNumber(token.price) ) : '0.00'}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/apps/pulse/components/Buy/tests/__snapshots__/Buy.test.tsx.snapis excluded by!**/*.snapsrc/apps/the-exchange/components/CardsSwap/test/__snapshots__/CardSwap.test.tsx.snapis excluded by!**/*.snapsrc/apps/token-atlas/components/TokenGraphColumn/test/__snapshots__/TokenGraphColumn.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
src/apps/pulse/components/Buy/Buy.tsx(8 hunks)src/apps/pulse/components/Buy/tests/Buy.test.tsx(2 hunks)src/apps/pulse/components/PnLStats/PnLStats.tsx(1 hunks)src/apps/pulse/components/Search/PortfolioTokenList.tsx(5 hunks)src/apps/pulse/components/Search/Search.tsx(3 hunks)src/apps/pulse/components/Search/TokenPnLCell.tsx(1 hunks)src/apps/pulse/components/Search/tests/Search.test.tsx(15 hunks)src/apps/pulse/components/Sell/Sell.tsx(10 hunks)src/apps/pulse/components/Sell/tests/Sell.test.tsx(3 hunks)src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx(0 hunks)src/hooks/useTokenPnL.ts(1 hunks)src/services/pillarXApiWalletPortfolio.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/apps/pulse/components/Sell/tests/Sell.test.tsx
- src/apps/pulse/components/PnLStats/PnLStats.tsx
- src/apps/pulse/components/Buy/Buy.tsx
- src/apps/pulse/components/Search/Search.tsx
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/apps/pulse/components/Search/tests/Search.test.tsxsrc/hooks/useTokenPnL.tssrc/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Search/TokenPnLCell.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Search/TokenPnLCell.tsx
📚 Learning: 2025-04-23T15:04:20.826Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 290
File: src/apps/pillarx-app/components/TileTitle/TitleTitle.tsx:6-10
Timestamp: 2025-04-23T15:04:20.826Z
Learning: In this repository, TileTitleProps and TileTitle are different types that serve different purposes. TileTitleProps is used for the TileTitle component and has optional fields (title?, leftDecorator?, rightDecorator?), while TileTitle in api.ts has a required text field. The TileTitleProps interface aligns with the TokensMarketData.title type in api.ts which also has optional fields.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-04-23T15:04:20.826Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 290
File: src/apps/pillarx-app/components/TileTitle/TitleTitle.tsx:6-10
Timestamp: 2025-04-23T15:04:20.826Z
Learning: In this repository, TileTitleProps and TileTitle are different types that serve different purposes. TileTitleProps is used for the TileTitle component and has optional fields (title?, leftDecorator?, rightDecorator?), while TileTitle in api.ts has a required title field. The TileTitleProps structure aligns with how it's used in the TokensMarketData type in api.ts.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/apps/pulse/components/Search/TokenPnLCell.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/apps/pulse/components/Search/tests/Search.test.tsxsrc/apps/pulse/components/Buy/tests/Buy.test.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/hooks/useTokenPnL.ts
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsx
🧬 Code graph analysis (5)
src/apps/pulse/components/Search/PortfolioTokenList.tsx (5)
src/types/api.ts (1)
PortfolioData(739-748)src/services/tokensData.ts (2)
Token(19-29)chainNameToChainIdTokensData(234-255)src/services/pillarXApiWalletPortfolio.ts (1)
convertPortfolioAPIResponseToToken(27-61)src/utils/blockchain.ts (1)
getLogoForChainId(165-199)src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)
src/apps/pulse/components/Search/tests/Search.test.tsx (2)
src/theme/index.ts (1)
defaultTheme(83-163)src/apps/pulse/components/Search/Search.tsx (1)
Search(64-484)
src/hooks/useTokenPnL.ts (4)
src/services/tokensData.ts (1)
Token(19-29)src/types/api.ts (2)
WalletTransactionsMobulaResponse(792-801)PnLMetrics(815-826)src/utils/pnl.ts (2)
calculatePnL(104-180)calculatePnLFromRelay(366-508)src/services/relayApi.ts (2)
RelayRequest(3-77)fetchRelayRequestByHash(107-130)
src/apps/pulse/components/Sell/Sell.tsx (4)
src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/hooks/useTokenPnL.ts (1)
useTokenPnL(29-282)src/apps/pulse/utils/blockchain.ts (1)
isNativeToken(17-18)src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
PnLStats(113-200)
src/apps/pulse/components/Search/TokenPnLCell.tsx (4)
src/services/tokensData.ts (1)
Token(19-29)src/types/api.ts (1)
WalletTransactionsMobulaResponse(792-801)src/hooks/useTokenPnL.ts (1)
useTokenPnL(29-282)src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (10)
src/services/pillarXApiWalletPortfolio.ts (1)
193-193: Cache configuration looks good.The 5-minute cache duration is reasonable for portfolio data and follows RTK Query best practices. Since
keepUnusedDataForonly applies after components unmount, it won't interfere with the 30-second auto-refresh mentioned in the PR objectives. When users return to the page, they'll see cached data briefly while fresh data loads in the background.Please confirm that a 5-minute cache window aligns with your product requirements for PnL data freshness, especially for scenarios where users navigate away and return to portfolio views.
src/apps/pulse/components/Buy/tests/Buy.test.tsx (4)
27-33: Good mock setup for the new useTokenPnL hook.The mock provides sensible defaults with
pnl: null,isLoading: false, and a mockrefetchfunction. This aligns well with the hook's return type.
58-69: Mock for wallet transactions API is well-structured.The mock includes both the RTK Query hook and the API slice scaffolding (reducerPath, reducer, middleware), which is necessary to avoid Redux configuration errors during tests.
95-108: Relay Buy and remote config mocks added correctly.These mocks support the new Relay Buy integration tests added at lines 926-1026.
481-481: Less brittle assertion for missing wallet data.Using
getAllByText('$0.00').length > 0instead of an exact count is more resilient to UI changes while still verifying the expected behavior.src/apps/pulse/components/Search/tests/Search.test.tsx (2)
135-141: Well-structured test helper with theme context.The
renderWithProvidershelper properly wraps components withThemeProviderandMemoryRouter, ensuring consistent test context across all tests.
32-62: Comprehensive mocks for new PnL and transaction dependencies.The mocks for
useTokenPnL,pillarXApiWalletTransactions, anduseTransactionKitare well-structured and provide appropriate default values for testing.src/apps/pulse/components/Sell/Sell.tsx (3)
92-100: Good use of useMemo for portfolio token lookup.The memoization correctly depends on
tokenandportfolioTokens, preventing unnecessary recalculations when other state changes.
625-635: PnL stats display logic is well-implemented.The conditional rendering correctly shows the PnLStats component only when there's meaningful data (
totalBoughtUSDC > 0 || totalSoldUSDC > 0) or during loading, preventing empty UI states.
125-129: PnL refetch integration looks correct.The effect properly triggers
refetchPnLwhenisRefreshingchanges, allowing parent components to coordinate refresh cycles.
- Buy.tsx: Use portfolioToken for PnL calculation to match Sell.tsx logic - PnLStats.tsx: Fix negative value formatting for USD and percentage - CardSwap.test.tsx: Update snapshot to reflect removed button - Buy.test.tsx & Sell.test.tsx: Update snapshots for PnL formatting changes
- Remove extra space in refetch definition - Remove unused transactionsData variable - Suppress exhaustive-deps warning for intentional omission
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/hooks/useTokenPnL.ts (3)
270-279: Auto-refresh interval restarts unnecessarily on props reference changes.The effect depends on the entire
propsobject (line 279), causing the interval to be cleared and recreated whenever the props reference changes—even if the content is identical. This could lead to unpredictable refresh timing and missed intervals.Depend only on whether props exists:
useEffect(() => { if (!props) return undefined; const interval = setInterval(() => { setResult((prev) => ({ ...prev, isLoading: true })); setRefreshTrigger((prev) => prev + 1); }, 30000); // 30 seconds return () => clearInterval(interval); - }, [props]); + }, [!!props]);Alternatively, extract the specific values needed from props and depend on those, or omit the dependency entirely if you want the interval to run once for the hook's lifetime after props becomes truthy.
49-57: Critical: Missing destructure oftransactionsDatafrom props.Line 55 attempts to access
props?.transactionsData?.data?.transactions, buttransactionsDatais never destructured fromprops. This will cause the hook to always evaluate transactions as undefined, preventing PnL calculation.Add the missing destructure:
const token = props?.token; const tokenContract = props?.token?.contract; const tokenSymbol = props?.token?.symbol; const tokenDecimals = props?.token?.decimals; const tokenBalance = props?.token?.balance; const tokenPrice = props?.token?.price; + const transactionsData = props?.transactionsData; const transactions = props?.transactionsData?.data?.transactions; const walletAddress = props?.walletAddress; const chainId = props?.chainId;
208-214: Replace non-null assertion with proper validation.The non-null assertion
chainId!at line 212 assumes chainId is always defined, but this is not guaranteed by the preceding validation checks. If props are null or chainId is undefined, this could cause runtime errors.Add explicit chainId validation:
// Calculate PnL from Relay data + if (!chainId) { + if (isMounted) + setResult((prev) => ({ + ...prev, + isLoading: false, + debug: { ...prev.debug, status: 'No ChainId' }, + })); + return; + } + const trades = calculatePnLFromRelay(relayRequests, { address: tokenContract || '', symbol: tokenSymbol || '', decimals: tokenDecimals || 18, - chainId: chainId!, + chainId, price: tokenPrice, });
🧹 Nitpick comments (3)
src/hooks/useTokenPnL.ts (1)
42-47: Remove or guard console.log statements in production code.Console logging on every render can impact performance and potentially leak sensitive debugging information in production builds.
Consider wrapping these in a development-only check or removing them:
- // eslint-disable-next-line no-console - console.log('[useTokenPnL] Hook render', { - tokenSymbol: props?.token?.symbol, - isLoading: result.isLoading, - pnl: result.pnl, - }); + if (process.env.NODE_ENV === 'development') { + // eslint-disable-next-line no-console + console.log('[useTokenPnL] Hook render', { + tokenSymbol: props?.token?.symbol, + isLoading: result.isLoading, + pnl: result.pnl, + }); + }Apply the same pattern to the console.log at line 236.
src/apps/pulse/components/Buy/Buy.tsx (2)
187-207: Type casting toanybypasses type safety for PnL hook.The token object is cast to
any(lines 200, 205) to satisfy useTokenPnL's type requirements, which indicates a type mismatch betweenSelectedTokenand the expectedTokentype. This bypasses TypeScript's type checking and could hide potential runtime errors.Consider either:
- Creating a proper type adapter function that maps
SelectedTokento the expectedTokentype, or- Updating the
Tokentype in useTokenPnL to accept the properties being passedExample adapter approach:
const adaptTokenForPnL = ( selectedToken: SelectedToken, portfolioToken: PortfolioToken ): Token => ({ ...selectedToken, contract: selectedToken.address || '', id: selectedToken.symbol, blockchain: 'ethereum', balance: portfolioToken.balance || 0, price: portfolioToken.price || 0, decimals: selectedToken.decimals || 18, });Then use it:
const { pnl, isLoading: isPnLLoading, refetch: refetchPnL } = useTokenPnL( token && accountAddress && portfolioToken ? { token: adaptTokenForPnL(token, portfolioToken), transactionsData, walletAddress: accountAddress, chainId: token.chainId, } : null );
830-833: Type casting buyOffer toanybypasses type safety.The
buyOfferis cast toanyto pass it asexpressIntentResponseto BuyButton, indicating thatBuyOfferandExpressIntentResponseare incompatible types. This bypasses type checking and could lead to runtime errors if BuyButton accesses properties that don't exist on BuyOffer.Consider updating BuyButton to accept a union type:
// In BuyButton component interface BuyButtonProps { // ... other props expressIntentResponse: ExpressIntentResponse | BuyOffer | null; // ... other props }Then remove the type casting:
expressIntentResponse={ - // eslint-disable-next-line @typescript-eslint/no-explicit-any - USE_RELAY_BUY ? (buyOffer as any) : expressIntentResponse + USE_RELAY_BUY ? buyOffer : expressIntentResponse }This preserves type safety while handling both Relay and Intent SDK flows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/apps/pulse/components/Buy/Buy.tsx(9 hunks)src/apps/pulse/components/PnLStats/PnLStats.tsx(1 hunks)src/hooks/useTokenPnL.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/pulse/components/PnLStats/PnLStats.tsx
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsx
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/hooks/useTokenPnL.ts
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/hooks/useTokenPnL.ts
📚 Learning: 2025-05-28T14:30:02.702Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 320
File: src/components/BottomMenuModal/HistoryModal/TransactionInfo.tsx:177-182
Timestamp: 2025-05-28T14:30:02.702Z
Learning: In the transaction history tracking feature, chain ID validation is performed earlier in the flow before the values reach the TransactionInfo component, so additional validation at the display level is not needed.
Applied to files:
src/hooks/useTokenPnL.ts
🧬 Code graph analysis (1)
src/apps/pulse/components/Buy/Buy.tsx (3)
src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/apps/pulse/utils/blockchain.ts (1)
isNativeToken(17-18)src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
PnLStats(113-202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
- Buy.tsx: Fix runtime error by destructuring transactionsData - PnLStats.tsx: Fix nested ternary lint error - Search.test.tsx: Remove unused imports - useTokenPnL.ts: Fix exhaustive-deps warning by adding token dependency - Buy.tsx: Fix formatting issues using prettier
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/utils/pnl.ts (3)
36-71: Normalize stablecoin symbol casing to avoid missing USDC tradesThe reconstruction logic is solid (grouping by
tx_hash, netting token/USDC flows, skipping multi‑token/fee rows), but the stablecoin checks are case‑sensitive:const { symbol } = tx.asset; if (symbol === 'USDC' || symbol === 'USDT' || symbol === 'DAI') { ... if (symbol === 'USDC') {If Mobula ever returns
usdc/Usdc, those trades will be ignored for PnL.Consider normalizing once and using the normalized value only for comparisons, while preserving the original symbol for display:
- const { symbol } = tx.asset; + const rawSymbol = tx.asset.symbol; + const symbolUpper = rawSymbol?.toUpperCase?.() ?? rawSymbol; - if (symbol === 'USDC' || symbol === 'USDT' || symbol === 'DAI') { + if (symbolUpper === 'USDC' || symbolUpper === 'USDT' || symbolUpper === 'DAI') { // ... - if (symbol === 'USDC') { + if (symbolUpper === 'USDC') { if (isInbound) usdcChange += amount; if (isOutbound) usdcChange -= amount; } - } else if (tokenSymbol && tokenSymbol !== symbol) { + } else if (tokenSymbol && tokenSymbol !== rawSymbol) { tokenSymbol = 'INVALID'; } else { - tokenSymbol = symbol; + tokenSymbol = rawSymbol;
116-133: Handle oversells and prevent negative inventory incalculatePnLThe SELL branch assumes you never sell more than your current inventory:
if (trade.side === 'BUY') { totalTokens += trade.amountToken; totalCostUSDC += trade.amountQuoteUSDC; } else { // SELL if (totalTokens === 0) return; const wac = totalCostUSDC / totalTokens; const costBasis = trade.amountToken * wac; totalTokens -= trade.amountToken; totalCostUSDC -= costBasis; ... } ... if (totalTokens < 0) totalTokens = 0; if (totalCostUSDC < 0) totalCostUSDC = 0;With incomplete history or bad data, a SELL can exceed
totalTokens, drivingtotalTokensandtotalCostUSDCnegative, and realised PnL/cost‑basis become inconsistent; the end‑loop clamp hides the negative state but doesn’t fix the earlier math.At minimum, you should:
- Treat non‑positive inventory as a hard guard, and
- Clamp the sell amount to available inventory so it never goes negative.
For example:
} else { // SELL - if (totalTokens === 0) return; // Selling without inventory (shouldn't happen in perfect history) - - const wac = totalCostUSDC / totalTokens; - const costBasis = trade.amountToken * wac; - - totalTokens -= trade.amountToken; - totalCostUSDC -= costBasis; - - totalCostBasisSold += costBasis; - realisedPnLUSDC += trade.amountQuoteUSDC - costBasis; + if (totalTokens <= 0) return; // Guard against zero/negative inventory + + const wac = totalCostUSDC / totalTokens; + const sellAmount = Math.min(trade.amountToken, totalTokens); + const costBasis = sellAmount * wac; + + totalTokens -= sellAmount; + totalCostUSDC -= costBasis; + + totalCostBasisSold += costBasis; + + // Scale quote amount if we clamped sellAmount + const quoteProportion = + trade.amountToken > 0 ? sellAmount / trade.amountToken : 1; + realisedPnLUSDC += trade.amountQuoteUSDC * quoteProportion - costBasis;Also applies to: 135-138
196-222: Unify USDC address list and includeoutTxswhen scanning Relay state changesTwo related issues here:
- Duplicated and inconsistent USDC address lists
getRelayValidatedTradesdefinesUSDC_ADDRESSESwith 4 chains (Base, Ethereum, Polygon, BSC) on lines 252‑258.calculatePnLFromRelaydefines anotherUSDC_ADDRESSESwith 6 chains (adds Arbitrum and Optimism) on lines 387‑395.This can cause
getRelayValidatedTradesto miss USDC flows on Arbitrum/Optimism and is error‑prone.
- USDC state changes only read from
inTxs, ignoringoutTxsIn
getRelayValidatedTrades, you only inspectrelayReq.data.inTxs:if (relayReq.data?.inTxs) { relayReq.data.inTxs.forEach((inTx) => { ... }); }For many SELL flows, the USDC leg will be recorded in
outTxs, so those trades will appear without a USDC amount and fall back totoken_priceor be dropped, diverging from the more completecalculatePnLFromRelaypath (which correctly combinesinTxsandoutTxs).You can address both by introducing a shared, normalized constant and using
allTxsin both places. For example:+// Known USDC addresses (lowercased) across supported chains +const USDC_ADDRESSES = [ + '0x833589fcd6edb6e08f4c7c32d4f71b54bda02913', // Base USDC + '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48', // Ethereum USDC + '0x3c499c54b84a76ad7e9c93437bfc5ac33e2ddae9', // Polygon USDC + '0x8ac76a51cc950d9822d68b83fe1ad97b32cd580d', // BSC USDC + '0xaf88d065e77c8cc2239327c5edb3a432268e5831', // Arbitrum USDC + '0x0b2c639c533813f4aa9d7837caf992837bd5787f', // Optimism USDC +].map((addr) => addr.toLowerCase());Then in
getRelayValidatedTrades:- // For USDC amount, extract it from Relay stateChanges - usdcAmount = 0; - if (relayReq.data?.inTxs) { - relayReq.data.inTxs.forEach((inTx) => { - if (inTx.stateChanges) { - inTx.stateChanges.forEach((stateChange) => { + // For USDC amount, extract it from all Relay stateChanges + usdcAmount = 0; + const allTxs = [ + ...(relayReq.data?.inTxs || []), + ...(relayReq.data?.outTxs || []), + ]; + + allTxs.forEach((tx) => { + if (tx.stateChanges) { + tx.stateChanges.forEach((stateChange) => { const tokenAddr = stateChange.change?.data?.tokenAddress?.toLowerCase(); const changeAddress = stateChange.address?.toLowerCase(); @@ - USDC_ADDRESSES.some( - (addr) => addr.toLowerCase() === tokenAddr - ) && + USDC_ADDRESSES.includes(tokenAddr) && changeAddress === userAddress ) { ... - }); - } - }); - } + }); + } + } + });And in
calculatePnLFromRelay, reuse the sameUSDC_ADDRESSES:- // Known USDC addresses for matching quote currency - const USDC_ADDRESSES = [ ... ]; + // Reuse shared USDC_ADDRESSES defined at top of fileAlso applies to: 252-262, 304-341, 387-395, 450-463
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)src/hooks/useTokenPnL.ts(1 hunks)src/utils/pnl.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/useTokenPnL.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/utils/pnl.ts
🧬 Code graph analysis (1)
src/utils/pnl.ts (2)
src/types/api.ts (3)
MobulaTransactionRow(768-790)ReconstructedTrade(803-813)PnLMetrics(815-826)src/services/relayApi.ts (2)
RelayRequest(3-99)fetchRelayRequestByHash(130-154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (2)
src/utils/pnl.ts (1)
375-549: Relay‑first PnL reconstruction logic looks robustThe overall structure of
calculatePnLFromRelayis solid:
- Prefer metadata (
currencyIn/currencyOut) when available.- Fall back to combined
inTxs/outTxsstate changes with proper decimal normalization.- Apply sensible sanity checks on
amountUSDCandexecPrice, with conservative fallbacks totoken.priceor dropping suspicious trades.- Return sorted
ReconstructedTrade[]consistent with the rest of the module.Once the shared
USDC_ADDRESSESconstant is factored out (per previous comment), this function reads cleanly and defensively.package.json (1)
30-30: Test for UI regressions in AssetSelect and Fear-and-Greed components@mui/joy's codebase uses modern patterns (sx prop, CssVarsProvider) compatible with the beta versions in play. However, beta-to-beta jumps can still introduce styling or behavioral changes. Verify visually that the Tabs/TabList in AssetSelect and Box/Grid/Typography layouts in Fear-and-Greed render consistently.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/hooks/useTokenPnL.ts (1)
219-225: Non-null assertion onchainIdis risky.The
chainId!assertion on line 223 assumeschainIdis always defined, but props can be null. While there's an early return check for!token || !walletAddress,chainIdcould still be 0 or undefined in edge cases.Use a safe fallback instead:
const trades = calculatePnLFromRelay(relayRequests, { address: tokenContract || '', symbol: tokenSymbol || '', decimals: tokenDecimals || 18, - chainId: chainId!, + chainId: chainId ?? 0, price: tokenPrice, });src/apps/pulse/components/Search/PortfolioTokenList.tsx (1)
90-123: Duplicate sorting logic persists - remove fromgetFilteredPortfolioTokens.Sorting is performed both here (lines 91-123) and again in
sortedTokensuseMemo (lines 147-171). The extensive comments on lines 97-103 and 109-113 indicate uncertainty about this design. SincesortedTokensalready handles all sorting cases, this block is redundant and adds cognitive overhead.Remove the sorting logic from
getFilteredPortfolioTokensand letsortedTokenshandle it exclusively:// Filter small balances if toggle is enabled if (hideSmallBalances) { tokens = tokens.filter((token) => { const balanceUSD = (token.price || 0) * (token.balance || 0); return balanceUSD >= 0.5; }); } - // Apply sorting - tokens = tokens.sort((a: Token, b: Token) => { - let valA = 0; - let valB = 0; - - switch (sortConfig.key) { - case 'price': - // Sort by Token Name/Price? Usually alphabetical or by price. - // Let's assume alphabetical by symbol for "Token" column sorting, or price? - // The header says "Token/Price". Let's sort by Symbol for now as it's more common for "Token". - // Or maybe USD Value? - // Let's stick to the previous default: Balance USD. - // But if user clicks "Token/Price", maybe alphabetical? - return a.symbol.localeCompare(b.symbol); - case 'balance': - valA = (a.price || 0) * (a.balance || 0); - valB = (b.price || 0) * (b.balance || 0); - break; - case 'pnl': - // We need PnL data for this. We'll handle PnL sorting in the useMemo below or move PnL calc up. - // Since PnL calc depends on transactions, and we are inside getFilteredPortfolioTokens which is called before PnL calc... - // We should probably move sorting AFTER PnL calc. - // For now, let's return tokens sorted by balance as default, and handle re-sorting in the rendered list or move logic. - // To avoid complex refactoring, I'll keep basic filtering here and do final sort on `tokensWithPnL`. - valA = (a.price || 0) * (a.balance || 0); - valB = (b.price || 0) * (b.balance || 0); - break; - default: - valA = (a.price || 0) * (a.balance || 0); - valB = (b.price || 0) * (b.balance || 0); - break; - } - return sortConfig.direction === 'asc' ? valA - valB : valB - valA; - }); - // Apply search filter if searchText is providedAlso update the dependency array to remove
sortConfig:- }, [walletPortfolioData, hideSmallBalances, sortConfig, searchText]); + }, [walletPortfolioData, hideSmallBalances, searchText]);src/apps/pulse/components/Search/TokenPnLCell.tsx (1)
70-91: Negative PnL values lack explicit minus sign.For negative
unrealisedPnLUSDC, the cell renders the absolute value without a minus sign (e.g.,$5.00instead of-$5.00), relying solely on red text to indicate a loss. This is inconsistent and could be confusing, especially for accessibility.Apply explicit signs for clarity:
+ const usdSign = + pnl.unrealisedPnLUSDC > 0 ? '+' : pnl.unrealisedPnLUSDC < 0 ? '-' : ''; + const pctSign = + pnl.unrealisedPnLPct > 0 ? '+' : pnl.unrealisedPnLPct < 0 ? '-' : ''; + return ( <div className="flex flex-col items-end"> <p className={`text-[13px] font-normal tracking-[-0.26px] text-right ${ pnl.unrealisedPnLUSDC >= 0 ? 'text-[#4ADE80]' : 'text-[#EF4444]' }`} > - {pnl.unrealisedPnLUSDC >= 0 ? '+' : ''}$ + {usdSign}$ {formatExponentialSmallNumber( limitDigitsNumber(Math.abs(pnl.unrealisedPnLUSDC)) )} </p> <p className={`text-xs font-normal tracking-[-0.24px] text-right ${ pnl.unrealisedPnLPct >= 0 ? 'text-[#4ADE80]' : 'text-[#EF4444]' }`} > - {pnl.unrealisedPnLPct >= 0 ? '+' : ''} - {pnl.unrealisedPnLPct.toFixed(2)}% + {pctSign} + {Math.abs(pnl.unrealisedPnLPct).toFixed(2)}% </p> </div> );src/utils/pnl.ts (3)
48-48: Timestamp unit inconsistency across functions.As noted in a past review,
reconstructTradesusestx.timestampas-is (Line 48, likely milliseconds from Mobula), whilegetRelayValidatedTradesdivides by 1000 (Line 360). This inconsistency can cause sorting/comparison issues when trades from different sources are combined.Consider standardizing on one unit (seconds or milliseconds) across all
ReconstructedTradeproducers.Also applies to: 105-105, 360-360
131-143: Guard for negative inventory not fully addressed.The check
if (totalTokens === 0) return;only handles exact zero. If transaction history is incomplete and a SELL occurs whentotalTokensis already negative from a previous iteration, division at Line 135 proceeds with a negative denominator, producing incorrect WAC.} else { // SELL - if (totalTokens === 0) return; // Selling without inventory (shouldn't happen in perfect history) + if (totalTokens <= 0) return; // Selling without inventory (shouldn't happen in perfect history)
311-344: Only checkinginTxsfor USDC state changes;outTxsmay contain USDC for SELLs.This code only iterates
relayReq.data.inTxs, but for SELL transactions the USDC received may appear inoutTxs. This could cause valid trades to be skipped or haveusdcAmount = 0. Note thatcalculatePnLFromRelay(Lines 440-443) correctly processes both.- if (relayReq.data?.inTxs) { - relayReq.data.inTxs.forEach((inTx) => { + const allTxs = [ + ...(relayReq.data?.inTxs || []), + ...(relayReq.data?.outTxs || []), + ]; + + allTxs.forEach((tx) => { + if (tx.stateChanges) { + tx.stateChanges.forEach((stateChange) => {
🧹 Nitpick comments (11)
src/apps/pulse/components/Buy/BuyButton.tsx (1)
113-144: Consider using a cleaner type guard for response shape detection.The current approach uses
'bids' in (expressIntentResponse || {})to detectExpressIntentResponse, which works but is somewhat fragile and requires a type assertion on line 134. A helper function or explicit type guard would make the intent clearer and improve type safety.Consider refactoring to use a type guard function:
+function isExpressIntentResponse( + response: ExpressIntentResponse | BuyOffer | null | { error: string } +): response is ExpressIntentResponse { + return response !== null && typeof response === 'object' && 'bids' in response; +} + const isDisabled = () => { if (isInstalling || isFetching) { return true; } if (notEnoughLiquidity) { return true; } if (!areModulesInstalled && payingTokens.length > 0) { return false; } // Check if it's an error object if ((expressIntentResponse as { error?: string })?.error) { return true; } // Check if it's ExpressIntentResponse with no bids - if ('bids' in (expressIntentResponse || {})) { + if (isExpressIntentResponse(expressIntentResponse)) { return ( isLoading || !token || !(parseFloat(usdAmount) > 0) || !expressIntentResponse || - (expressIntentResponse as ExpressIntentResponse)?.bids?.length === 0 + expressIntentResponse.bids?.length === 0 ); } // For BuyOffer or other types, just check basic conditions return ( isLoading || !token || !(parseFloat(usdAmount) > 0) || !expressIntentResponse ); };src/apps/pulse/components/Sell/Sell.tsx (1)
264-264: Remove unnecessary comment.The comment "Calculate token balance (must be after getTokenBalance function)" doesn't add value since
getTokenBalanceis defined earlier in the same file (lines 206-240). The variable assignment is straightforward.- // Calculate token balance (must be after getTokenBalance function) const tokenBalance = getTokenBalance();src/apps/pulse/components/Buy/Buy.tsx (1)
427-428: Remove duplicate comment.The comment "// Refresh PnL data" appears without associated code and duplicates the comment at line 380. This appears to be a leftover from refactoring.
- // Refresh PnL data - try {src/apps/pulse/components/Search/PortfolioTokenList.tsx (3)
160-164: PnL sort key is effectively a no-op.The
'pnl'case setsvalAandvalBto 0, so clicking the "Unrealized PnL/%" header only toggles the sort indicator without changing row order. The comment acknowledges PnL is calculated in child components.Consider either:
- Disabling the click handler and hiding the sort indicator for the PnL column until sorting is implemented
- Implementing PnL-based sorting by lifting PnL calculation to this component
416-419: Remove commented-out code.These commented lines appear to be leftover debug/development code and should be removed for cleaner production code.
<p className="text-xs font-normal tracking-[-0.24px] text-white opacity-50 text-left" data-testid="pulse-portfolio-token-price" > - ${/* // Skip PnL for small balances */} - {/* if (valueUSD < 0.01) { */} - {/* return { ...token, pnl: null }; */} - {/* } */} + $ {token.price
338-347: Skeleton rows shown alongside actual tokens during refresh.When
isFetchingis true, skeleton rows are rendered (lines 339-347) AND actual tokens are shown withopacity-30(line 359). This creates a confusing UI where both placeholders and dimmed content appear simultaneously.Consider showing only one or the other:
{/* Show skeleton during refresh */} - {isFetching && !isLoading && ( + {isFetching && !isLoading && sortedTokens.length === 0 && ( <> <SkeletonTokenRow /> ... </> )}Or remove the skeleton entirely during refresh since the opacity reduction already indicates loading state.
src/hooks/useTokenPnL.ts (1)
65-68: Redundant effect to update refetch in result.The
refetchfunction is already stable (wrapped inuseCallbackwith empty deps), so updating it inresulton every mount is unnecessary. The initial state already sets an empty function, and the main calculation effect could directly use the callback.Consider removing this effect and initializing
refetchdirectly in the initial state or updating it once during initialization.src/services/relayApi.ts (2)
3-99: Large interface definition - consider consolidating with types/api.ts.The
RelayRequestinterface spans ~100 lines and is defined here rather than in the centraltypes/api.tsfile where other API types reside. For consistency and maintainability, consider moving this totypes/api.ts.
108-114: RTK Query'sretry()wrapper already includes exponential backoff by default.The configuration (2 retries, 5-second timeout) is reasonable for user-facing PnL calculations with 30s auto-refresh. However, consider adding a
retryConditionto bail out early on non-transient errors (4xx client errors, validation failures) rather than exhausting all retries on permanent failures.Example improvement:
const fetchBaseQueryWithRetry = retry( fetchBaseQuery({ baseUrl: RELAY_API_URL, timeout: 5000, }), { maxRetries: 2, retryCondition: (error) => error.status > 499 || error.status === 408, } );src/apps/pulse/components/PnLStats/PnLStats.tsx (2)
42-44: Remove duplicated comment.The same comment appears three times consecutively.
// Format PnL with sign and percentage: -$0.80 (-0.5%) or +$1.20 (+2.3%) - // Format PnL with sign and percentage: -$0.80 (-0.5%) or +$1.20 (+2.3%) - // Format PnL with sign and percentage: -$0.80 (-0.5%) or +$1.20 (+2.3%) const formatPnLWithPercent = (usdValue: number, pctValue: number) => {
34-40: Minor inconsistency informatTokenoutput.For
val === 0, this returns'0.00', but for other values (e.g.,1.50), trailing zeros are stripped to'1.5'. If consistency is desired, consider returning'0'for zero or keeping two decimals for all values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/apps/pulse/components/Buy/Buy.tsx(7 hunks)src/apps/pulse/components/Buy/BuyButton.tsx(3 hunks)src/apps/pulse/components/PnLStats/PnLStats.tsx(1 hunks)src/apps/pulse/components/PnLStats/PnLStatsSkeleton.tsx(1 hunks)src/apps/pulse/components/PnLStats/StatGroup.tsx(1 hunks)src/apps/pulse/components/Search/PortfolioTokenList.tsx(5 hunks)src/apps/pulse/components/Search/SkeletonTokenRow.tsx(1 hunks)src/apps/pulse/components/Search/SortIndicator.tsx(1 hunks)src/apps/pulse/components/Search/TokenPnLCell.tsx(1 hunks)src/apps/pulse/components/Sell/Sell.tsx(7 hunks)src/apps/pulse/constants/tokens.ts(1 hunks)src/hooks/useTokenPnL.ts(1 hunks)src/services/pillarXApiWalletPortfolio.ts(1 hunks)src/services/relayApi.ts(1 hunks)src/services/relayApiAsync.ts(1 hunks)src/store.ts(2 hunks)src/utils/__tests__/pnl.test.ts(1 hunks)src/utils/pnl.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/services/pillarXApiWalletPortfolio.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/store.ts
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/constants/tokens.tssrc/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/PnLStats/PnLStats.tsxsrc/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/apps/pulse/components/Buy/BuyButton.tsxsrc/apps/pulse/components/Search/TokenPnLCell.tsx
📚 Learning: 2025-05-23T14:44:33.911Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 315
File: src/apps/the-exchange/utils/wrappedTokens.ts:6-20
Timestamp: 2025-05-23T14:44:33.911Z
Learning: XDAI (Gnosis Chain) is intentionally excluded from the WRAPPED_NATIVE_TOKEN_ADDRESSES mapping in the exchange app's wrappedTokens utility.
Applied to files:
src/apps/pulse/constants/tokens.ts
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Buy/Buy.tsxsrc/utils/pnl.ts
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/PnLStats/PnLStats.tsxsrc/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/apps/pulse/components/Search/TokenPnLCell.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsxsrc/hooks/useTokenPnL.tssrc/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/apps/pulse/components/Search/TokenPnLCell.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/hooks/useTokenPnL.tssrc/apps/pulse/components/Buy/Buy.tsx
📚 Learning: 2025-05-28T14:30:02.702Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 320
File: src/components/BottomMenuModal/HistoryModal/TransactionInfo.tsx:177-182
Timestamp: 2025-05-28T14:30:02.702Z
Learning: In the transaction history tracking feature, chain ID validation is performed earlier in the flow before the values reach the TransactionInfo component, so additional validation at the display level is not needed.
Applied to files:
src/hooks/useTokenPnL.ts
📚 Learning: 2025-04-23T15:04:20.826Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 290
File: src/apps/pillarx-app/components/TileTitle/TitleTitle.tsx:6-10
Timestamp: 2025-04-23T15:04:20.826Z
Learning: In this repository, TileTitleProps and TileTitle are different types that serve different purposes. TileTitleProps is used for the TileTitle component and has optional fields (title?, leftDecorator?, rightDecorator?), while TileTitle in api.ts has a required text field. The TileTitleProps interface aligns with the TokensMarketData.title type in api.ts which also has optional fields.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-04-23T15:04:20.826Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 290
File: src/apps/pillarx-app/components/TileTitle/TitleTitle.tsx:6-10
Timestamp: 2025-04-23T15:04:20.826Z
Learning: In this repository, TileTitleProps and TileTitle are different types that serve different purposes. TileTitleProps is used for the TileTitle component and has optional fields (title?, leftDecorator?, rightDecorator?), while TileTitle in api.ts has a required title field. The TileTitleProps structure aligns with how it's used in the TokensMarketData type in api.ts.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsx
🧬 Code graph analysis (9)
src/apps/pulse/components/Sell/Sell.tsx (3)
src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/hooks/useTokenPnL.ts (1)
useTokenPnL(35-310)src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
PnLStats(10-112)
src/hooks/useTokenPnL.ts (4)
src/types/api.ts (2)
WalletTransactionsMobulaResponse(792-801)PnLMetrics(815-826)src/utils/pnl.ts (2)
calculatePnL(118-200)calculatePnLFromRelay(378-545)src/services/relayApi.ts (1)
RelayRequest(3-99)src/services/relayApiAsync.ts (1)
fetchRelayRequestByHash(32-60)
src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
src/types/api.ts (1)
PnLMetrics(815-826)
src/apps/pulse/components/Buy/Buy.tsx (3)
src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/hooks/useTokenPnL.ts (1)
useTokenPnL(35-310)src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
PnLStats(10-112)
src/services/relayApiAsync.ts (2)
src/services/relayApi.ts (3)
RelayRequest(3-99)relayApi(116-163)relayApi(165-169)src/store.ts (1)
store(67-79)
src/apps/pulse/components/Search/PortfolioTokenList.tsx (4)
src/services/tokensData.ts (2)
Token(19-29)chainNameToChainIdTokensData(234-255)src/services/pillarXApiWalletPortfolio.ts (1)
convertPortfolioAPIResponseToToken(27-61)src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)src/apps/pulse/components/Search/TokenPnLCell.tsx (1)
TokenPnLCell(18-92)
src/apps/pulse/components/Buy/BuyButton.tsx (1)
src/apps/pulse/hooks/useRelayBuy.ts (1)
BuyOffer(30-36)
src/apps/pulse/components/Search/TokenPnLCell.tsx (4)
src/services/tokensData.ts (1)
PortfolioToken(31-34)src/types/api.ts (1)
WalletTransactionsMobulaResponse(792-801)src/hooks/useTokenPnL.ts (1)
useTokenPnL(35-310)src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)
src/utils/pnl.ts (4)
src/apps/pulse/constants/tokens.ts (1)
allStableCurrencies(3-11)src/types/api.ts (3)
MobulaTransactionRow(768-790)ReconstructedTrade(803-813)PnLMetrics(815-826)src/services/relayApi.ts (1)
RelayRequest(3-99)src/services/relayApiAsync.ts (1)
fetchRelayRequestByHash(32-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (18)
src/apps/pulse/constants/tokens.ts (1)
3-15: ExportingallStableCurrencieslooks fine; confirm intended usage vsSTABLE_CURRENCIES.Making
allStableCurrenciespublic is a clean way to expose the raw mapping for consumers like PnL, while keepingSTABLE_CURRENCIESas the feature‑gated view. Just make sure new call sites that care about the Gnosis toggle continue to useSTABLE_CURRENCIES, and only useallStableCurrencieswhere bypassingisGnosisEnabledis explicitly desired (e.g., purely analytical/off‑UI logic).src/utils/__tests__/pnl.test.ts (1)
1-211: LGTM! Comprehensive test coverage for PnL logic.The test suite covers:
- Trade reconstruction from buy/sell transactions
- PnL calculation with realised and unrealised metrics
- Edge case handling (SELL-only transactions)
The mock data structure now includes all required fields (hash, method_label, contract) addressing the previous review feedback.
src/apps/pulse/components/Sell/Sell.tsx (1)
82-122: LGTM! PnL integration properly typed without unsafe casts.The PnL data flow is well-structured:
- Fetches wallet transactions with proper skip logic
- Finds matching portfolio token using useMemo
- Constructs properly typed token object for useTokenPnL
- Handles null cases gracefully
This addresses the previous concerns about type assertions.
src/apps/pulse/components/Buy/Buy.tsx (1)
170-208: LGTM! PnL integration properly implemented.The PnL data flow correctly:
- Captures wallet transactions query results (addresses previous review feedback)
- Finds matching portfolio token with proper memoization
- Constructs properly typed token object without unsafe type assertions
- Integrates PnL refresh into the buy flow
The previous concerns about uncaptured query results and type assertions have been resolved.
src/services/relayApiAsync.ts (3)
1-30: LGTM - Well-structured async helpers for non-React contexts.The separation to avoid circular dependencies with
store.tsis a good architectural decision. The error handling is consistent, returning fallback values ([]) instead of throwing, which aligns with the original axios implementation as noted in the comments.
32-60: Good handling of 404 as expected case.The special handling of 404 status for
fetchRelayRequestByHashis appropriate since a transaction not being in Relay is an expected scenario, not an error condition. The type assertion on line 42 is necessary but acceptable given RTK Query's error shape.
62-79: LGTM - Consistent pattern with other helpers.The error handling follows the same pattern as the other functions in this file.
src/apps/pulse/components/Search/TokenPnLCell.tsx (2)
18-45: LGTM - Clean conditional hook pattern.The conditional passing of
nulltouseTokenPnLwhentokenorwalletAddressis missing is a clean pattern that avoids hook violations while skipping unnecessary calculations.
47-51: The refetch dependency is properly managed and won't fire repeatedly.
isRefreshingis correctly toggled back to false in thefinallyblock of the asynchandleRefresh()operation (HomeScreen.tsx:369), so the effect won't trigger repeatedly. Therefetchfunction fromuseTokenPnLis memoized withuseCallbackand has no dependencies, ensuring it remains stable across renders. No ref tracking or transition detection is needed here.src/hooks/useTokenPnL.ts (2)
120-132: Minimum loading duration for UX is a nice touch.The 100ms minimum loading duration ensures skeleton animations are visible and prevents UI flicker. This is a thoughtful UX consideration.
281-294: Good dependency array management.The eslint-disable comment is justified here - the specific primitive dependencies are correctly listed to avoid infinite loops from unstable object references. The comment on line 284 clearly explains why
tokenis excluded.src/services/relayApi.ts (2)
146-160: Good defensive handling of heterogeneous response shapes.The
transformResponsecorrectly handles both array responses and object-with-requests responses from the API. This defensive coding prevents runtime errors from API inconsistencies.
116-140: LGTM - Clean RTK Query endpoint definitions.The endpoints are well-structured with appropriate query parameters and transform responses. The use of
status = 'filled'as default forgetRelayRequestsaligns with PnL calculation needs (only completed transactions matter).src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
1-112: Component structure looks good.The component properly handles loading state, destructures metrics with sensible defaults, and the
formatPnLWithPercentimplementation correctly handles sign display for both positive and negative values. The StatGroup abstraction keeps the layout clean.src/utils/pnl.ts (4)
15-19: Good consolidation of USDC addresses.Using
allStableCurrenciesas the single source of truth addresses the previous duplication concern.
437-466: Good handling of bothinTxsandoutTxsin the fallback strategy.The state-change fallback correctly combines both transaction arrays, addressing the USDC extraction concern for this code path.
503-528: Good sanity checks for data integrity.The validation thresholds (USDC < $1T, exec price < $1M, positive price) provide sensible guardrails against malformed Relay data corrupting PnL calculations.
328-331: Potential precision loss withparseFloaton large integer strings.
balanceDiffvalues from Relay can be very large integers (wei).parseFloatloses precision for integers > 2^53. After division by decimals the impact is typically minimal, but for very large trades this could introduce small errors.Based on learnings, the team has accepted JavaScript's floating-point limitations as a valid trade-off.
Also applies to: 453-453
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/apps/pulse/components/Search/Search.tsx (1)
671-681: Add keyboard accessibility to the toggle.The toggle div lacks keyboard navigation support. Add
role="checkbox",tabIndex={0},aria-checked, andonKeyDownto make it accessible to keyboard users.Apply this diff to improve accessibility:
- <div - className="flex items-center gap-1.5 cursor-pointer opacity-50 hover:opacity-100 transition-opacity" - onClick={() => setHideSmallBalances(!hideSmallBalances)} - > + <div + className="flex items-center gap-1.5 cursor-pointer opacity-50 hover:opacity-100 transition-opacity" + onClick={() => setHideSmallBalances(!hideSmallBalances)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + setHideSmallBalances(!hideSmallBalances); + } + }} + role="checkbox" + aria-checked={hideSmallBalances} + tabIndex={0} + aria-label="Hide balances under $0.5" + >src/apps/pulse/components/Buy/BuyButton.tsx (1)
144-178: SimplifyisDisabledunion handling and make the bids guard clearerThe new error/bids handling works, but there are two opportunities to tighten it up:
- Error is checked twice: once via optional chaining and again in the fallback branch via
'error' in expressIntentResponse. You can rely on the first check and drop the second for clarity.'bids' in (expressIntentResponse || {})is a bit opaque; a directexpressIntentResponse && 'bids' in expressIntentResponseguard is easier to read and keeps the'in'operator clearly on a non-null object, avoiding the need for the extra!expressIntentResponsecheck inside that branch.A refactor that preserves behavior could look like:
- // Check if it's an error object - if ((expressIntentResponse as { error?: string })?.error) { - return true; - } - // Check if it's ExpressIntentResponse with no bids - if ('bids' in (expressIntentResponse || {})) { + // Check if it's an error object + const errorMessage = ( + expressIntentResponse as { error?: string } | null | undefined + )?.error; + if (errorMessage) { + return true; + } + + // ExpressIntentResponse branch – guard on truthy object before `'in'` + if (expressIntentResponse && 'bids' in expressIntentResponse) { return ( isLoading || !token || !(parseFloat(usdAmount) > 0) || - !expressIntentResponse || (expressIntentResponse as ExpressIntentResponse)?.bids?.length === 0 ); } - // For BuyOffer or other types, just check basic conditions + // For BuyOffer or other types, just check basic conditions return ( isLoading || !token || !(parseFloat(usdAmount) > 0) || - !expressIntentResponse || - ('error' in expressIntentResponse && !!expressIntentResponse.error) || - ('bids' in expressIntentResponse && - (expressIntentResponse as ExpressIntentResponse)?.bids?.length === 0) + !expressIntentResponse );This keeps the special ExpressIntentResponse-with-bids behavior while reducing duplication and making the control flow over the union shapes easier to follow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/pulse/components/App/tests/AppWrapper.test.tsx(1 hunks)src/apps/pulse/components/Buy/Buy.tsx(7 hunks)src/apps/pulse/components/Buy/BuyButton.tsx(2 hunks)src/apps/pulse/components/Search/Search.tsx(5 hunks)src/apps/pulse/components/Search/tests/Search.test.tsx(16 hunks)src/apps/pulse/components/Sell/Sell.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/apps/pulse/components/App/tests/AppWrapper.test.tsx
- src/apps/pulse/components/Search/tests/Search.test.tsx
- src/apps/pulse/components/Buy/Buy.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Search/Search.tsxsrc/apps/pulse/components/Sell/Sell.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/Search/Search.tsxsrc/apps/pulse/components/Sell/Sell.tsx
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/Search/Search.tsxsrc/apps/pulse/components/Sell/Sell.tsx
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: Cloudflare Pages: pillarx-debug
- GitHub Check: build
🔇 Additional comments (9)
src/apps/pulse/components/Search/Search.tsx (3)
17-17: LGTM!The new imports are correctly used:
useIsMobileon line 521 andfilterMarketsByLiquidityon line 172. Import reordering is cosmetic.Also applies to: 26-26, 53-54
107-107: LGTM!The
hideSmallBalancesstate with a default value oftrueprovides a sensible default UX—hiding small balances initially and allowing users to toggle visibility.
856-856: LGTM!The
hideSmallBalancesprop is correctly passed toPortfolioTokenList, enabling the filtering logic downstream.src/apps/pulse/components/Sell/Sell.tsx (5)
7-7: LGTM!The new imports for PnL functionality are appropriate and all are utilized in the component.
Also applies to: 34-34, 38-39, 43-43
82-89: LGTM!Transaction fetching is properly implemented with the
skipparameter preventing unnecessary queries when the wallet address is unavailable.
91-100: LGTM!The portfolio token lookup is properly memoized with correct dependencies. The matching logic correctly compares both contract address and chain ID to handle multi-chain scenarios.
124-129: LGTM!The refetch effect correctly responds to the parent component's refresh trigger with appropriate dependencies and safe optional chaining.
631-642: LGTM!The conditional rendering logic for PnLStats is well-designed. It appropriately shows the component when loading or when there's actual transaction history (bought or sold > 0), which avoids displaying empty PnL data for tokens without transaction history.
src/apps/pulse/components/Buy/BuyButton.tsx (1)
15-43: Chain name import and CTA usage look goodImporting
getChainNameand using it in the "Enable Trading on ..." CTA is consistent and safely guarded by thepayingTokencheck; no issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/apps/pulse/components/Buy/Buy.tsx (1)
433-434: Remove orphaned comment.The comment "// Refresh PnL data" on line 433 appears to be leftover from editing. The actual
refetchPnL()call is already handled earlier at lines 386-389.- // Refresh PnL data - try {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/pulse/components/App/HomeScreen.tsx(3 hunks)src/apps/pulse/components/Buy/Buy.tsx(9 hunks)src/apps/pulse/components/Search/Search.tsx(5 hunks)src/apps/pulse/components/Search/TokenPnLCell.tsx(1 hunks)src/apps/pulse/components/Search/tests/Search.test.tsx(15 hunks)src/apps/pulse/components/Sell/Sell.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/pulse/components/Search/TokenPnLCell.tsx
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/App/HomeScreen.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/App/HomeScreen.tsxsrc/apps/pulse/components/Search/Search.tsx
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Buy/Buy.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Search/tests/Search.test.tsxsrc/apps/pulse/components/App/HomeScreen.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Search/tests/Search.test.tsxsrc/apps/pulse/components/App/HomeScreen.tsxsrc/apps/pulse/components/Search/Search.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsx
🧬 Code graph analysis (3)
src/apps/pulse/components/Sell/Sell.tsx (3)
src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/hooks/useTokenPnL.ts (1)
useTokenPnL(35-310)src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
PnLStats(10-112)
src/apps/pulse/components/Buy/Buy.tsx (3)
src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/hooks/useTokenPnL.ts (1)
useTokenPnL(35-310)src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
PnLStats(10-112)
src/apps/pulse/components/Search/tests/Search.test.tsx (2)
src/theme/index.ts (1)
defaultTheme(83-163)src/apps/pulse/components/Search/Search.tsx (1)
Search(82-876)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (12)
src/apps/pulse/components/Sell/Sell.tsx (3)
82-100: PnL data wiring looks correct.The transactions fetching with
skipoption and portfolio token matching viauseMemoare properly implemented. The chain ID comparison usinggetChainIdensures accurate matching between portfolio tokens and the selected token.
103-129: PnL hook integration and refresh logic are well-structured.The
useTokenPnLhook is invoked with properly typed props (noas anycasts), and the refresh effect correctly triggersrefetchPnLwhen the parent'sisRefreshingchanges. The conditional null pattern for missing prerequisites is appropriate.
631-642: PnL display conditions are appropriate.The conditional rendering logic correctly shows PnLStats only when:
- A token is selected, AND
- Either loading is in progress OR there's actual PnL data (buys or sells)
This prevents showing an empty PnL component when there's no trading history.
src/apps/pulse/components/Buy/Buy.tsx (2)
174-212: PnL integration follows consistent patterns with Sell.tsx.The transactions fetching, portfolio token lookup, and
useTokenPnLhook usage mirror the implementation in Sell.tsx. The properly typed token object construction (withoutas anycasts) addresses the concerns from previous reviews.
862-873: PnLStats rendering logic is consistent with Sell.tsx.The conditional display logic and loading state combination (
isPnLLoading || isTransactionsLoading || isRefreshing) maintain consistency across both Buy and Sell components.src/apps/pulse/components/App/HomeScreen.tsx (1)
1143-1143: Correctly forwards refresh state to Buy component.The
isRefreshing={isRefreshingHome}prop enables the Buy component to synchronize its PnL loading state with the parent's refresh cycle, maintaining UI consistency.src/apps/pulse/components/Search/Search.tsx (3)
107-108: State initialization for hideSmallBalances is appropriate.Defaulting to
truealigns with user expectations to hide dust/small balances in the portfolio view by default.
662-682: Toggle UI implementation for hiding small balances.The toggle provides a clear visual indicator (filled/empty circle) and reasonable labeling. The click handler correctly inverts the state.
One minor UX consideration: the click target could be expanded to include the text label for easier interaction, but the current implementation is functional.
856-857: Props correctly passed to PortfolioTokenList.Both
hideSmallBalancesandisFetching(mapped fromwalletPortfolioFetching) are forwarded, enabling the child component to filter tokens and show loading skeletons appropriately.src/apps/pulse/components/Search/tests/Search.test.tsx (3)
27-57: Mocks are properly structured for the new PnL dependencies.The mocks for
useTokenPnL,useGetWalletTransactionsQuery, anduseTransactionKitreturn appropriate default values that match the expected hook return types. ThepillarXApiWalletTransactionsmock includes the RTK Query service structure (reducerPath,reducer,middleware).
130-136: Good test utility pattern with renderWithProviders.The helper consolidates
ThemeProviderandMemoryRouterwrapping, reducing boilerplate across tests and ensuring consistent provider setup.
325-333: Test correctly targets ESC button in sell mode.The updated test properly uses
isBuy={false}since the ESC close button (withdata-testid="pulse-search-esc-button") only renders in non-buy mode. This aligns with the component's conditional rendering logic.
| }, | ||
| })); | ||
|
|
||
| const relayResults = await Promise.all( |
There was a problem hiding this comment.
Don't think this is an issue since its user calling it and we are reducing the amount of calls by filtering. If the user gets rate limited, it would be great problem to address since they are trading so much :)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/utils/pnl.ts (2)
118-200: Handle over‑sells/negative inventory more explicitly incalculatePnLThe WAC logic is correct for “perfect” histories, but if the trade history is truncated or a user sells more than your reconstructed inventory,
totalTokenscan go negative. You clamptotalTokens/totalCostUSDCback to0at the end, but realised PnL andtotalCostBasisSoldwill already have included cost basis for non‑existent tokens, which can materially skew PnL.To make this more robust against incomplete history:
- Guard against
totalTokens <= 0before computing WAC.- Clamp the sell size to available inventory so balances never go negative.
- Scale the USDC proceeds proportionally when clamping, so realised PnL remains consistent with the partial fill.
For example:
} else { // SELL - if (totalTokens === 0) return; // Selling without inventory (shouldn't happen in perfect history) - - const wac = totalCostUSDC / totalTokens; - const costBasis = trade.amountToken * wac; - - totalTokens -= trade.amountToken; - totalCostUSDC -= costBasis; - - totalCostBasisSold += costBasis; - realisedPnLUSDC += trade.amountQuoteUSDC - costBasis; + if (totalTokens <= 0) { + // No inventory left – skip trades that cannot be reconciled with history + return; + } + + const wac = totalCostUSDC / totalTokens; + + // Clamp sell size to available inventory to avoid negative balances + const sellAmount = Math.min(trade.amountToken, totalTokens); + const proportion = sellAmount / trade.amountToken; + + const costBasis = sellAmount * wac; + + totalTokens -= sellAmount; + totalCostUSDC -= costBasis; + + totalCostBasisSold += costBasis; + realisedPnLUSDC += trade.amountQuoteUSDC * proportion - costBasis; }This keeps balances non‑negative while still deriving a sensible realised PnL from partial history.
258-371: Include RelayoutTxswhen extracting USDC state changes ingetRelayValidatedTradesHere you only scan
relayReq.data.inTxsfor USDCstateChanges. For SELLs, the USDC leg can appear inoutTxsinstead, which means:
hasUSDCInRelaystaysfalseand the trade is dropped entirely, even though it’s a valid Pulse trade.- You never reach the price fallback because you return early when
!hasUSDCInRelay.To align with
calculatePnLFromRelay(which already combinesinTxsandoutTxs) and avoid missing trades, you should process both:- // For USDC amount, extract it from Relay stateChanges - usdcAmount = 0; - if (relayReq.data?.inTxs) { - relayReq.data.inTxs.forEach((inTx) => { - if (inTx.stateChanges) { - inTx.stateChanges.forEach((stateChange) => { + // For USDC amount, extract it from Relay stateChanges (in + out) + usdcAmount = 0; + const allTxs = [ + ...(relayReq.data?.inTxs || []), + ...(relayReq.data?.outTxs || []), + ]; + + allTxs.forEach((tx) => { + if (tx.stateChanges) { + tx.stateChanges.forEach((stateChange) => { const tokenAddr = stateChange.change?.data?.tokenAddress?.toLowerCase(); const changeAddress = stateChange.address?.toLowerCase(); @@ - } - }); - } - }); - } + } + }); + } + } + });Without this, you’ll systematically under‑report PnL for SELLs whose USDC flow is only visible in
outTxs.
🧹 Nitpick comments (2)
src/utils/pnl.ts (2)
15-19: Clarify USDC_ADDRESSES coupling toallStableCurrenciesfor future safety
USDC_ADDRESSESis built by blindly mappingallStableCurrencies. Today that list contains only USDC addresses, but if other stables (USDT/DAI, etc.) are ever added there, this module will start treating them as USDC (including hard‑coding1e6decimals in several places), which will silently skew PnL.I’d recommend either:
- Introducing a dedicated USDC‑only export in
tokens.ts(e.g.usdcCurrencies/usdcAddresses) and using that here, or- Renaming
allStableCurrenciesto something USDC‑specific if it is intended to be USDC‑only long‑term, so the intent is explicit.
21-116: Minor: useasset.contractas a fallback fortokenAddress
reconstructTradeslooks solid overall (grouping bytx_hash, USDC by address, and direction inference are all consistent), buttokenAddressis set totx.asset.contracts?.[0] || ''. That can leave you with an empty token address even though Mobula exposesasset.contract.Using the same fallback you already use for
txContractkeeps things consistent and avoids empty addresses:- } else { - tokenSymbol = symbol; - tokenAddress = tx.asset.contracts?.[0] || ''; + } else { + tokenSymbol = symbol; + tokenAddress = + (tx.asset.contracts && tx.asset.contracts[0]) || tx.asset.contract; if (isInbound) tokenChange += amount; if (isOutbound) tokenChange -= amount; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/pnl.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/utils/pnl.ts
🧬 Code graph analysis (1)
src/utils/pnl.ts (4)
src/apps/pulse/constants/tokens.ts (1)
allStableCurrencies(3-11)src/types/api.ts (3)
MobulaTransactionRow(768-790)ReconstructedTrade(803-813)PnLMetrics(815-826)src/services/relayApi.ts (1)
RelayRequest(3-99)src/services/relayApiAsync.ts (1)
fetchRelayRequestByHash(32-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cloudflare Pages: pillarx-debug
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/utils/pnl.ts (2)
118-200: Oversell scenario still skews PnL; consider clamping sell size to available inventoryThe
totalTokens <= 0guard (Line 133) prevents division by zero/negative inwac, but if a SELL trade’samountTokenexceedstotalTokens, you still:
- Compute
wacusing the full current inventory.- Subtract
trade.amountToken(Line 138), drivingtotalTokensnegative.- Clamp
totalTokens/totalCostUSDCback to zero later (Lines 146-148), leavingrealisedPnLUSDCandtotalCostBasisSoldoverstated for incomplete histories.For more robust behaviour when history is incomplete or data is slightly off, clamping the sell amount to available inventory (and optionally logging) avoids negative inventory and keeps realised PnL consistent:
} else { // SELL if (totalTokens <= 0) return; // Selling without inventory (handles zero and negative cases) - const wac = totalCostUSDC / totalTokens; - const costBasis = trade.amountToken * wac; - - totalTokens -= trade.amountToken; + const wac = totalCostUSDC / totalTokens; + const sellAmount = Math.min(trade.amountToken, totalTokens); + const costBasis = sellAmount * wac; + + if (sellAmount < trade.amountToken) { + // Optional: log or track that history is incomplete / oversell occurred + // console.warn('[calculatePnL] Sell amount exceeds inventory; clamping to available balance'); + } + + totalTokens -= sellAmount; totalCostUSDC -= costBasis; totalCostBasisSold += costBasis; - realisedPnLUSDC += trade.amountQuoteUSDC - costBasis; + realisedPnLUSDC += trade.amountQuoteUSDC * (sellAmount / trade.amountToken) - costBasis;This keeps state internally consistent even when the upstream trade stream is imperfect.
207-372: Relay USDC extraction still ignoresoutTxs; align withcalculatePnLFromRelayThis function currently only inspects
relayReq.data.inTxswhen extracting USDC state changes (Lines 311-343). For SELL flows, the USDC leg is often recorded inoutTxs, so you can miss USDC deltas and incorrectly drop valid trades whenhasUSDCInRelaystaysfalse.You already handle this correctly in
calculatePnLFromRelayby combininginTxsandoutTxs. Mirroring that pattern here would make the behaviour consistent and fix missing USDC for some trades:- // For USDC amount, extract it from Relay stateChanges - usdcAmount = 0; - if (relayReq.data?.inTxs) { - relayReq.data.inTxs.forEach((inTx) => { - if (inTx.stateChanges) { - inTx.stateChanges.forEach((stateChange) => { + // For USDC amount, extract it from Relay stateChanges + usdcAmount = 0; + const allTxs = [ + ...(relayReq.data?.inTxs || []), + ...(relayReq.data?.outTxs || []), + ]; + + allTxs.forEach((tx) => { + if (tx.stateChanges) { + tx.stateChanges.forEach((stateChange) => { const tokenAddr = stateChange.change?.data?.tokenAddress?.toLowerCase(); const changeAddress = stateChange.address?.toLowerCase(); @@ - }); - } - }); - } + }); + } + });Given
calculatePnLFromRelayalready uses the combined-array approach, you might also consider extracting a small helper for “USDC delta from Relay stateChanges” to keep the logic DRY between the two paths.
🧹 Nitpick comments (1)
src/utils/pnl.ts (1)
378-551: Solid Relay-based reconstruction; consider renaming and small DRY/refinementThe overall structure here looks good: prefer metadata, fall back to state changes, normalise timestamps to seconds, use price fallback, and guard against absurd values. A few small points:
Function name vs return type (clarity)
calculatePnLFromRelayreturnsReconstructedTrade[], notPnLMetrics. That’s slightly misleading alongsidecalculatePnL. If API stability allows it, consider renaming to something likereconstructTradesFromRelayorbuildRelayTradesto better reflect its role.Share state-change parsing with
getRelayValidatedTrades
The state-change path here (Lines 438-492) and the USDC extraction logic ingetRelayValidatedTradesare conceptually similar: both walk Relay state changes, aggregate token and USDC deltas, and normalise timestamps. Pulling the common part into a shared helper (e.g.extractRelayTokenAndUsdcDeltas(req, tokenContract)) would reduce duplication and help keep future fixes in sync.Big-number precision (optional)
UsingparseFloatand10 ** token.decimalsis pragmatic but will lose precision for very large on-chain integers (especiallydecimals >= 18). If you expect high-volume or very precise PnL, migrating to a bigint/BN library for Relay amounts would improve numerical stability, though it’s not blocking for typical retail-sized flows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/pnl.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/utils/pnl.ts
🧬 Code graph analysis (1)
src/utils/pnl.ts (4)
src/apps/pulse/constants/tokens.ts (1)
allStableCurrencies(3-11)src/types/api.ts (3)
MobulaTransactionRow(768-790)ReconstructedTrade(803-813)PnLMetrics(815-826)src/services/relayApi.ts (1)
RelayRequest(3-99)src/services/relayApiAsync.ts (1)
fetchRelayRequestByHash(32-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (1)
src/utils/pnl.ts (1)
15-19: Centralising USDC addresses viaallStableCurrencieslooks goodDeriving
USDC_ADDRESSESfromallStableCurrenciesremoves duplication and keeps the address set consistent across consumers; the lowercasing here also simplifies comparisons later. No issues from my side.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/apps/pulse/components/Search/PortfolioTokenList.tsx (1)
83-132: Consolidate sorting logic and avoid a no-op PnL sort header.Currently tokens are sorted twice (inside
getFilteredPortfolioTokensand again insortedTokens), and the'pnl'branch sets both comparison values to0, so clicking “Unrealized PnL/%” only flips the indicator without changing row order. It would be cleaner to keepgetFilteredPortfolioTokensfor filtering only and letsortedTokensbe the single source of sorting, and either implement a real PnL-based sort once metrics are available or disable the PnL header click until then to avoid a misleading UX.Also applies to: 152-181, 283-289, 330-343
🧹 Nitpick comments (3)
src/apps/pulse/components/Buy/Buy.tsx (1)
395-401: Remove leftover PnL refresh comment to avoid confusion.There’s a “Refresh PnL data” comment in
refreshBuyIntentwithout corresponding logic now that the actual refresh call is earlier in the function; consider deleting it to keep the intent clear.Also applies to: 444-445
src/services/relayApiAsync.ts (1)
9-30: Consider disabling subscriptions for one-shot Relay helpers.
fetchRelayRequestsandfetchRelayRequestsByUserdon’t passsubscribe: falseor callunsubscribe(), so each call leaves a live subscription even though you only need the resolved data; aligning these withfetchRelayRequestByHashby usingsubscribe: falsewould avoid unnecessary subscribers while still leveraging RTK Query’s caching.Also applies to: 64-80
src/apps/pulse/components/Sell/Sell.tsx (1)
95-103: Optional: extract sharedportfolioTokenlookup used in Buy/Sell.The contract+chainId matching logic to find
portfolioTokenis duplicated here and in Buy; consider a small shared helper to reduce drift if that matching rule ever changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/pulse/components/Buy/Buy.tsx(9 hunks)src/apps/pulse/components/Search/PortfolioTokenList.tsx(5 hunks)src/apps/pulse/components/Search/TokenPnLCell.tsx(1 hunks)src/apps/pulse/components/Sell/Sell.tsx(7 hunks)src/hooks/useTokenPnL.ts(1 hunks)src/services/relayApiAsync.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Search/TokenPnLCell.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Sell/Sell.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Search/TokenPnLCell.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Search/TokenPnLCell.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/hooks/useTokenPnL.ts
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsxsrc/hooks/useTokenPnL.ts
📚 Learning: 2025-04-23T15:04:20.826Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 290
File: src/apps/pillarx-app/components/TileTitle/TitleTitle.tsx:6-10
Timestamp: 2025-04-23T15:04:20.826Z
Learning: In this repository, TileTitleProps and TileTitle are different types that serve different purposes. TileTitleProps is used for the TileTitle component and has optional fields (title?, leftDecorator?, rightDecorator?), while TileTitle in api.ts has a required text field. The TileTitleProps interface aligns with the TokensMarketData.title type in api.ts which also has optional fields.
Applied to files:
src/apps/pulse/components/Search/TokenPnLCell.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-04-23T15:04:20.826Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 290
File: src/apps/pillarx-app/components/TileTitle/TitleTitle.tsx:6-10
Timestamp: 2025-04-23T15:04:20.826Z
Learning: In this repository, TileTitleProps and TileTitle are different types that serve different purposes. TileTitleProps is used for the TileTitle component and has optional fields (title?, leftDecorator?, rightDecorator?), while TileTitle in api.ts has a required title field. The TileTitleProps structure aligns with how it's used in the TokensMarketData type in api.ts.
Applied to files:
src/apps/pulse/components/Search/TokenPnLCell.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-05-28T14:30:02.702Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 320
File: src/components/BottomMenuModal/HistoryModal/TransactionInfo.tsx:177-182
Timestamp: 2025-05-28T14:30:02.702Z
Learning: In the transaction history tracking feature, chain ID validation is performed earlier in the flow before the values reach the TransactionInfo component, so additional validation at the display level is not needed.
Applied to files:
src/hooks/useTokenPnL.ts
🧬 Code graph analysis (5)
src/services/relayApiAsync.ts (2)
src/services/relayApi.ts (3)
RelayRequest(3-99)relayApi(116-163)relayApi(165-169)src/store.ts (1)
store(67-79)
src/apps/pulse/components/Buy/Buy.tsx (3)
src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/hooks/useTokenPnL.ts (1)
useTokenPnL(35-339)src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
PnLStats(10-112)
src/apps/pulse/components/Sell/Sell.tsx (3)
src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/hooks/useTokenPnL.ts (1)
useTokenPnL(35-339)src/apps/pulse/components/PnLStats/PnLStats.tsx (1)
PnLStats(10-112)
src/apps/pulse/components/Search/TokenPnLCell.tsx (4)
src/services/tokensData.ts (1)
PortfolioToken(31-34)src/types/api.ts (1)
WalletTransactionsMobulaResponse(792-801)src/hooks/useTokenPnL.ts (1)
useTokenPnL(35-339)src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)
src/hooks/useTokenPnL.ts (4)
src/types/api.ts (2)
WalletTransactionsMobulaResponse(792-801)PnLMetrics(815-826)src/utils/pnl.ts (2)
calculatePnL(118-200)calculatePnLFromRelay(378-551)src/services/relayApi.ts (1)
RelayRequest(3-99)src/services/relayApiAsync.ts (1)
fetchRelayRequestByHash(32-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (5)
src/apps/pulse/components/Buy/Buy.tsx (1)
174-215: PnL integration and refresh flow in Buy are wired correctly.Transactions query, portfolio token lookup,
useTokenPnLprops, and the PnLStats visibility/loading conditions all line up with the hook’s contract andisRefreshingbehavior; I don’t see functional issues here.Also applies to: 217-223, 873-883
src/apps/pulse/components/Sell/Sell.tsx (1)
82-103: Sell-side PnL wiring and refresh behavior look sound.Wallet transactions, portfolio token resolution,
useTokenPnLprops, theisRefreshing→refetchTransactionseffect, and the PnLStats visibility/loading logic are consistent with the Buy flow and the hook’s expectations.Also applies to: 105-121, 123-129, 631-640
src/apps/pulse/components/Search/PortfolioTokenList.tsx (1)
53-73: Wallet transaction refresh and per-row TokenPnLCell usage look correct.Using
useTransactionKit+useGetWalletTransactionsQuerywithskip, refetching onisFetching, showing SkeletonTokenRow placeholders, and passingtransactionsData,walletAddress,chainId, andisRefreshing={isFetching}intoTokenPnLCellform a consistent, predictable PnL data flow for each portfolio row.Also applies to: 347-356, 358-478
src/apps/pulse/components/Search/TokenPnLCell.tsx (1)
9-80: TokenPnLCell’s useTokenPnL integration and states are well-structured.Props passed into
useTokenPnLmatch the hook’s contract, and the loading (isRefreshing || isLoading), empty (!pnl), and populated render paths give a clear, consistent PnL cell without obvious edge-case issues.src/hooks/useTokenPnL.ts (1)
35-339: useTokenPnL’s calculation, refresh, and safety guards look solid.The hook correctly validates inputs, filters relevant Mobula transactions, limits Relay calls, reconstructs trades, adjusts metrics for actual wallet balance, and uses
isMounted, refined dependencies, and a 30s interval +refetchtrigger to manage loading and refresh without obvious race conditions or leaks.
- Changed trade detection to check state changes first before metadata - Fixes issue where LINK sells were hidden in state changes while metadata showed USDC bridge - Ensures all token movements are captured in complex multi-step transactions - Verified fix detects previously missed LINK sell (0.039147 LINK for $0.01)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utils/pnl.ts (1)
21-116: Address unresolved issues: missing token address fallback and repeated lowercasing.Two previously flagged issues remain unaddressed:
Missing fallback to
tx.asset.contractfor token address (Line 78): The USDC detection logic (Lines 63-64) correctly falls back totx.asset.contract, buttokenAddressassignment only usestx.asset.contracts?.[0]. For tokens that only populateasset.contract, this results in an empty string in the reconstructed trade.Repeated
walletAddress.toLowerCase()calls (Lines 49-50): ComputingwalletAddress.toLowerCase()on every iteration is inefficient and reduces readability.Apply this diff to address both issues:
export const reconstructTrades = ( transactions: MobulaTransactionRow[], walletAddress: string ): ReconstructedTrade[] => { const trades: ReconstructedTrade[] = []; const groupedByTxHash: { [txHash: string]: MobulaTransactionRow[] } = {}; + const walletAddressLc = walletAddress.toLowerCase(); // Group by txHash transactions.forEach((tx) => { if (!groupedByTxHash[tx.tx_hash]) { groupedByTxHash[tx.tx_hash] = []; } groupedByTxHash[tx.tx_hash].push(tx); }); // Process each group Object.keys(groupedByTxHash).forEach((txHash) => { const group = groupedByTxHash[txHash]; let usdcChange = 0; let tokenChange = 0; let tokenSymbol = ''; let tokenAddress = ''; const feeUsd = 0; let timestamp = 0; // Identify assets and calculate net changes group.forEach((tx) => { timestamp = tx.timestamp; // Assume all rows have same timestamp or close enough - const isInbound = tx.to.toLowerCase() === walletAddress.toLowerCase(); - const isOutbound = tx.from.toLowerCase() === walletAddress.toLowerCase(); + const isInbound = tx.to.toLowerCase() === walletAddressLc; + const isOutbound = tx.from.toLowerCase() === walletAddressLc; if (!isInbound && !isOutbound) return; // Not related to wallet directly? (Maybe fee payer?) const { amount } = tx; const { symbol } = tx.asset; if (tx.type === 'native') { // Gas fee usually return; } // Check if this is a USDC transaction by matching address const txContract = (tx.asset.contracts && tx.asset.contracts[0]) || tx.asset.contract; const isUSDC = txContract && USDC_ADDRESSES.includes(txContract.toLowerCase()); if (isUSDC) { if (isInbound) usdcChange += amount; if (isOutbound) usdcChange -= amount; } else if (tokenSymbol && tokenSymbol !== symbol) { // Base Token // If we already found a DIFFERENT base token in this tx, it's a multi-token trade (unsupported). // Mark as invalid/ignored tokenSymbol = 'INVALID'; } else { tokenSymbol = symbol; - tokenAddress = tx.asset.contracts?.[0] || ''; + tokenAddress = tx.asset.contracts?.[0] || tx.asset.contract || ''; if (isInbound) tokenChange += amount; if (isOutbound) tokenChange -= amount; } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/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!**/*.snap
📒 Files selected for processing (1)
src/utils/pnl.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/utils/pnl.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (3)
src/utils/pnl.ts (3)
1-20: LGTM! Clean imports and USDC address extraction.The imports are well-organized, and extracting
USDC_ADDRESSESfromallStableCurrenciesprovides a single source of truth, resolving the previous duplicate addresses issue.
118-200: LGTM! Solid WAC-based PnL calculation with proper edge case handling.The function correctly implements weighted average cost (WAC) accounting, handles the zero/negative inventory guard (Line 133), prevents negative dust (Lines 147-148), and returns
nullwhen there are no buy transactions to avoid misleading +$0 displays.
378-548: LGTM! Comprehensive Relay-based PnL calculation with robust error handling.This function demonstrates excellent practices:
- Correctly processes both
inTxsandoutTxs(Line 406)- Proper timestamp normalization handling milliseconds vs seconds (Lines 411-415)
- Case-insensitive USDC symbol checks (Lines 468, 480)
- Comprehensive sanity checks preventing absurd values (Lines 506-531)
- Clear fallback logic with warning logs
The reducer pattern for accumulating state changes is clean and efficient.
vignesha22
left a comment
vignesha22
left a comment
There was a problem hiding this comment.
LGTM a lot of formatting from prettier on most of the files
3 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.