added send link retry. Backend PR as well#1339
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
WalkthroughAdds TanStack Query-driven wallet balance and send flows (optimistic updates and rollback), WebSocket-driven real-time history insertion with duplicate detection, refactors claim link processing to a linkUrl/query flow with error handling, introduces related tests and docs, updates UI guards, and re-exports a sendLink helper. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Claim/Claim.tsx (1)
229-242: Don’t mark link NOT_FOUND on token-price failures; isolate errors.The outer try/catch treats any failure (including price fetch noise) as NOT_FOUND. This regresses UX by hiding a valid link when price fetch fails. Wrap price fetching in its own try/catch and default to 0/skip state.
- let price = 0 - if (isStableCoin(tokenDetails.symbol)) { - price = 1 - } else { - const tokenPriceDetails = await fetchTokenPrice( - sendLink.tokenAddress.toLowerCase(), - sendLink.chainId - ) - if (tokenPriceDetails) { - price = tokenPriceDetails.price - } - } - if (0 < price) setTokenPrice(price) + try { + let price = 0 + if (isStableCoin(tokenDetails.symbol)) { + price = 1 + } else { + const tokenPriceDetails = await fetchTokenPrice( + sendLink.tokenAddress.toLowerCase(), + sendLink.chainId + ) + if (tokenPriceDetails) price = tokenPriceDetails.price + } + if (price > 0) setTokenPrice(price) + } catch (e) { + console.warn('[Claim] Token price fetch failed, continuing without price', e) + // keep link usable; do not change linkState here + }Based on learnings.
Also applies to: 253-256
src/hooks/wallet/useWallet.ts (1)
90-98: Remove non-null assertion onaddressto expose undefined in public API.The
addressfromuseZeroDev()is typed asAddress | undefined(evident from casts on lines 31, 41), but the non-null assertion on line 91 masks this, making consumers believeaddressis always defined. TypeScript won't require guards, but runtime crashes can occur. Align the return type with reality:return { - address: address!, + address, balance, isConnected: isKernelClientReady, sendTransactions, sendMoney, fetchBalance, isFetchingBalance, }This forces consumers to check
isConnected && addressbefore use, as seen in Create.request.link.view.tsx (lines 83, 88, 371).
🧹 Nitpick comments (20)
src/hooks/wallet/useBalance.ts (1)
24-31: Return type clarity (bigint) and optional formatting.Viem returns bigint for ERC‑20 balances. Explicitly type the query data to enforce consistency and avoid accidental number coercion later. Optionally expose a formatted string via select.
-export const useBalance = (address: Address | undefined) => { - return useQuery({ +export const useBalance = (address: Address | undefined) => { + return useQuery<bigint>({ queryKey: ['balance', address], queryFn: async () => { … const balance = await peanutPublicClient.readContract({ address: PEANUT_WALLET_TOKEN, abi: erc20Abi, functionName: 'balanceOf', args: [address], }) - return balance + return balance },Optionally:
select: (b) => b.toString(), // or formatUnits(b, 6)src/app/(mobile-ui)/history/page.tsx (3)
51-53: Extract and reuse the limit constant to avoid key drift.Hardcoding
20in both the query and the cache key risks mismatches if the limit changes.- [TRANSACTIONS, 'infinite', { limit: 20 }], + [TRANSACTIONS, 'infinite', { limit }],Define
const limit = 20near the hook invocation and reuse it foruseTransactionHistoryand the cache key.
77-79: Narrow balance invalidation if possible.Invalidating
['balance']refreshes all balance queries. If the app can derive the active address here, invalidate['balance', address]to reduce redundant traffic.- queryClient.invalidateQueries({ queryKey: ['balance'] }) + // if you can get activeAddress: + // queryClient.invalidateQueries({ queryKey: ['balance', activeAddress] })
47-64: Gate console logs behind env or debug flag.Unconditional logs can spam consoles in production.
- console.log('[History] New transaction received via WebSocket:', newEntry) + if (process.env.NODE_ENV !== 'production') { + console.log('[History] New transaction received via WebSocket:', newEntry) + } … - console.log('[History] Duplicate transaction ignored:', newEntry.uuid) + if (process.env.NODE_ENV !== 'production') { + console.log('[History] Duplicate transaction ignored:', newEntry.uuid) + }src/app/(mobile-ui)/history/__tests__/websocket-duplicate-detection.test.tsx (3)
163-213: Add a test for “duplicate exists on page > 0”.Current suite doesn’t cover a duplicate present on later pages; the production code can currently prepend a second copy. Add a case asserting no insertion when UUID exists on page 2+.
I can draft this test case if helpful.
28-34: Clean up QueryClient between tests.Prevent potential memory/leak warnings by clearing the client after each test.
beforeEach(() => { queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }) }) +afterEach(() => { + queryClient.clear() +})
36-63: Minor: reuse constants/types to reduce drift.Import
TRANSACTIONSandHistoryResponsefrom source modules instead of redefining them locally; extractconst LIMIT = 20for reuse across expectations..cursorrules (1)
5-9: Rephrase absolute prohibitions for accuracy.“Never open SVG / never run jq / sleep hibernates pc” reads misleading. Prefer actionable, environment‑specific guidance (e.g., “Known to crash our CI runner — avoid in CI; use X instead”).
docs/CHANGELOG.md (1)
33-36: Scope alignment and traceability.These “Fixed” items map to this PR; consider appending PR number(s) and date for traceability, and ensure unrelated features under “Added/Changed” reflect actual merged work.
Would you like me to normalize entries to Keep‑a‑Changelog style with PR links (e.g., #1339) and release date?
src/components/Claim/Claim.tsx (5)
135-135: Guard access to events[0] when deriving cancelledDate.
claimLinkData.events[0]can throw if events is empty/undefined. Use optional chaining and length check.- cancelledDate: status === 'cancelled' ? new Date(claimLinkData.events[0].timestamp) : undefined, + cancelledDate: + status === 'cancelled' && claimLinkData.events?.length + ? new Date(claimLinkData.events[0].timestamp) + : undefined,
295-300: Missing dependency: effect should track searchParams.The redirect effect reads
searchParams.get('step')but only depends onuser. IncludesearchParamsso it reacts to URL changes.- }, [user]) + }, [user, searchParams])
277-283: Unnecessary refetch on auth changes; set linkUrl once.Re-setting
linkUrlon everyuserchange can cause redundant query churn. Initialize on mount only (or depend on actual URL changes).- }, [user]) + }, [])
78-91: Query cache knobs: consider less aggressive gcTime/staleTime.
staleTime: 0andgcTime: 0will thrash the network on any re-render and hide DevTools history. If links are one-time, you can still allow brief cache residency to avoid duplicate requests during processing.Try
staleTime: 5_000andgcTime: 60_000, ornetworkMode: 'always'if you must bypass offline. No behavior change for single-use links but smoother UX.
362-363: onClose setter is likely a no-op.
setLinkUrl(window.location.href)sets the same value, so React will bail and nothing happens. If you intend a refresh, invalidate the query instead; if you intend only to close, remove this side effect.Example:
- onClose={() => setLinkUrl(window.location.href)} + // onClose={() => queryClient.invalidateQueries({ queryKey: ['sendLink', linkUrl] })} + onClose={undefined}src/hooks/wallet/useWallet.ts (2)
22-31: Address validation gate is permissive before user loads; confirm intent.
isValidAddressbecomes true whenuserAddressis not yet available, allowing balance fetches that may later be suppressed. If you want strict matching, delay the query until user is known.Example: introduce a
const isUserLoaded = user !== undefinedand gateenabled: isUserLoaded && isValidAddress.
33-38: State sync to Redux looks fine; avoid double writes on unchanged values.You may skip dispatch if the bigint equals the stored value to avoid noisy updates.
- if (balanceFromQuery !== undefined) { - dispatch(walletActions.setBalance(balanceFromQuery)) - } + if (balanceFromQuery !== undefined && balanceFromQuery !== BigInt(reduxBalance ?? -1)) { + dispatch(walletActions.setBalance(balanceFromQuery)) + }src/hooks/wallet/useSendMoney.ts (3)
39-60: Type the mutation generics to makecontextand return types explicit.Improves safety and DX for handlers (
onError,onSuccess).- return useMutation({ + return useMutation< + { userOpHash: Hash; amount: bigint; receipt: TransactionReceipt | null }, // TData + Error, // TError + SendMoneyParams, // TVariables + { previousBalance?: bigint } // TContext + >({Also applies to: 61-86, 87-109
61-86: Optimistic update guard is good; also consider canceling history queries.To reduce flicker in consumers reading transactions immediately, you can cancel
[TRANSACTIONS]queries duringonMutate.await queryClient.cancelQueries({ queryKey: ['balance', address] }) +await queryClient.cancelQueries({ queryKey: [TRANSACTIONS] })
98-107: Rollback path is correct; add debug context to logs.Minimal, but including
variables.toAddressandamountInUsdhelps field triage.- console.error('[useSendMoney] Transaction failed, rolled back balance:', error) + console.error('[useSendMoney] Transaction failed, rolled back balance:', { error, to: variables?.toAddress, amount: variables?.amountInUsd })src/hooks/wallet/__tests__/useSendMoney.test.tsx (1)
159-179: Add afterEach cleanup hook to restore mocks and prevent test isolation issues.The test file uses
jest.spyOn()at line 159 without explicit cleanup. While thebeforeEachhook creates a freshQueryClientper test (providing natural scope isolation), explicit mock cleanup is still a Jest best practice to prevent state leakage of module-level mocks and ensure complete test isolation:afterEach(() => { jest.restoreAllMocks() })Add this after the
beforeEachblock (around line 36) within thedescribe('useSendMoney')block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.cursorrules(1 hunks)docs/CHANGELOG.md(2 hunks)docs/TANSTACK_QUERY_OPPORTUNITIES.md(1 hunks)src/app/(mobile-ui)/history/__tests__/websocket-duplicate-detection.test.tsx(1 hunks)src/app/(mobile-ui)/history/page.tsx(2 hunks)src/components/Claim/Claim.tsx(8 hunks)src/hooks/wallet/__tests__/useSendMoney.test.tsx(1 hunks)src/hooks/wallet/useBalance.ts(1 hunks)src/hooks/wallet/useSendMoney.ts(1 hunks)src/hooks/wallet/useWallet.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-15T14:47:26.891Z
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#857
File: src/hooks/useWebSocket.ts:77-82
Timestamp: 2025-05-15T14:47:26.891Z
Learning: The useWebSocket hook in src/hooks/useWebSocket.ts is designed to provide raw history entries, while the components using it (such as HomeHistory.tsx) are responsible for implementing deduplication logic based on UUID to prevent duplicate entries when combining WebSocket data with other data sources.
Applied to files:
src/app/(mobile-ui)/history/page.tsxsrc/app/(mobile-ui)/history/__tests__/websocket-duplicate-detection.test.tsx
📚 Learning: 2025-04-11T11:33:53.245Z
Learnt from: kushagrasarathe
PR: peanutprotocol/peanut-ui#798
File: src/components/Home/HomeHistory.tsx:138-192
Timestamp: 2025-04-11T11:33:53.245Z
Learning: In the HomeHistory component, infinite scrolling is intentionally not implemented despite the presence of useInfiniteQuery and IntersectionObserver code. The component is designed to only display the first 5 entries with a "View all transactions" link for viewing the complete history.
Applied to files:
src/app/(mobile-ui)/history/page.tsx
📚 Learning: 2024-11-18T21:36:11.486Z
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#535
File: src/components/Claim/Claim.tsx:142-146
Timestamp: 2024-11-18T21:36:11.486Z
Learning: In `src/components/Claim/Claim.tsx`, external calls like token price fetching and cross-chain details retrieval are already encapsulated within existing `try...catch` blocks, so additional error handling may be unnecessary.
Applied to files:
src/components/Claim/Claim.tsx
🧬 Code graph analysis (7)
src/hooks/wallet/useWallet.ts (5)
src/redux/hooks.ts (2)
useAppDispatch(5-5)useWalletStore(10-10)src/hooks/useZeroDev.ts (1)
useZeroDev(36-187)src/context/authContext.tsx (1)
useAuth(191-197)src/hooks/wallet/useBalance.ts (1)
useBalance(16-41)src/redux/slices/wallet-slice.ts (1)
walletActions(24-24)
src/app/(mobile-ui)/history/page.tsx (4)
src/redux/hooks.ts (1)
useUserStore(13-13)src/hooks/useWebSocket.ts (1)
useWebSocket(19-186)src/hooks/useTransactionHistory.ts (1)
HistoryResponse(17-21)src/constants/query.consts.ts (1)
TRANSACTIONS(2-2)
src/hooks/wallet/useSendMoney.ts (2)
src/constants/zerodev.consts.ts (3)
PEANUT_WALLET_TOKEN_DECIMALS(19-19)PEANUT_WALLET_TOKEN(20-20)PEANUT_WALLET_CHAIN(18-18)src/constants/query.consts.ts (1)
TRANSACTIONS(2-2)
src/hooks/wallet/useBalance.ts (1)
src/constants/zerodev.consts.ts (2)
peanutPublicClient(68-68)PEANUT_WALLET_TOKEN(20-20)
src/app/(mobile-ui)/history/__tests__/websocket-duplicate-detection.test.tsx (2)
src/hooks/useTransactionHistory.ts (1)
HistoryResponse(17-21)src/constants/query.consts.ts (1)
TRANSACTIONS(2-2)
src/components/Claim/Claim.tsx (1)
src/services/sendLinks.ts (1)
sendLinksApi(37-217)
src/hooks/wallet/__tests__/useSendMoney.test.tsx (2)
src/constants/zerodev.consts.ts (1)
PEANUT_WALLET_TOKEN_DECIMALS(19-19)src/hooks/wallet/useSendMoney.ts (1)
useSendMoney(36-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (4)
src/app/(mobile-ui)/history/__tests__/websocket-duplicate-detection.test.tsx (1)
1-7: Solid coverage focus.Great targeted tests for prepend, duplicates, empty page, and multipage behavior.
.cursorrules (1)
13-21: Nice consolidation of quality guidance.Clear, concise, and aligned with the repo’s practices.
src/app/(mobile-ui)/history/page.tsx (1)
43-81: Query key consistency verified—no issues found.The infinite query key used in the WebSocket cache update exactly matches the key generated by
useTransactionHistory:
useTransactionHistoryis called withmode: 'infinite', limit: 20(line 38–40)- This generates the queryKey
[TRANSACTIONS, 'infinite', { limit: 20 }]- The WebSocket callback uses the same key structure
[TRANSACTIONS, 'infinite', { limit: 20 }]Both reference the same
TRANSACTIONSconstant, use the same string literal'infinite', and the same limit value. The keys are consistent across the hook setup and cache mutations.docs/TANSTACK_QUERY_OPPORTUNITIES.md (1)
1-651: Solid, actionable roadmap; aligns with current PR direction.Clear priorities and query-key conventions. No blocking issues.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/app/(mobile-ui)/history/page.tsx (1)
51-76: Deduplicate across all pages, not just the current page.The duplicate check
page.entries.some((entry) => entry.uuid === newEntry.uuid)only scans the current page being mapped. If the same UUID exists on a later page, you will surface a duplicate after prepending to page 0.Apply this diff to check across all pages:
queryClient.setQueryData<InfiniteData<HistoryResponse>>( [TRANSACTIONS, 'infinite', { limit: 20 }], (oldData) => { if (!oldData) return oldData + + // Check if entry exists anywhere in the cache + const existsAnywhere = oldData.pages.some(p => + p.entries.some(e => e.uuid === newEntry.uuid) + ) + if (existsAnywhere) { + console.log('[History] Duplicate transaction ignored:', newEntry.uuid) + return oldData + } - // Add new entry to the first page (with duplicate check) + // Add new entry to the first page return { ...oldData, pages: oldData.pages.map((page, index) => { if (index === 0) { - // Check if entry already exists to prevent duplicates - const isDuplicate = page.entries.some((entry) => entry.uuid === newEntry.uuid) - if (isDuplicate) { - console.log('[History] Duplicate transaction ignored:', newEntry.uuid) - return page - } return { ...page, entries: [newEntry, ...page.entries], } } return page }), } } )This avoids cross-page duplication while prepending to the head page. Based on learnings.
src/components/Claim/Claim.tsx (1)
196-197: Fragile password parsing; add robust extraction and guard.
url.hash.split('=')[1]breaks if the hash is absent, uses a different key, or contains additional params, and may produceundefinedthat flows intogenerateKeysFromString. Parse with URLSearchParams or reusegetParamsFromLink(if exported) and guard before deriving keys.Apply one of the following:
Option A — reuse shared parser (preferred, if
getParamsFromLinkis exported):+import { getParamsFromLink } from '@/services/sendLinks' ... - const url = new URL(linkUrl) - const password = url.hash.split('=')[1] + const { password } = getParamsFromLink(linkUrl) + if (!password) { + setLinkState(_consts.claimLinkStateType.WRONG_PASSWORD) + return + } ... const keyPair = peanut.generateKeysFromString(password)Option B — local robust parsing:
- const url = new URL(linkUrl) - const password = url.hash.split('=')[1] + const u = new URL(linkUrl) + const fromHash = new URLSearchParams(u.hash.startsWith('#') ? u.hash.slice(1) : u.hash).get('p') + const fromSearch = new URLSearchParams(u.search).get('p') + const password = fromHash ?? fromSearch ?? '' + if (!password) { + setLinkState(_consts.claimLinkStateType.WRONG_PASSWORD) + return + }Based on learnings.
🧹 Nitpick comments (1)
src/hooks/wallet/useWallet.ts (1)
22-31: Validate address comparison logic carefully.The
isValidAddresscheck returnstruewhen eitheraddressoruserAddressis undefined, which means the balance query could be enabled even when addresses are missing. While the useBalance hook hasenabled: !!addressas a secondary guard, the validation logic is counterintuitive.Consider this clearer approach:
- const isValidAddress = !address || !userAddress || userAddress.toLowerCase() === address.toLowerCase() + const isValidAddress = !!address && !!userAddress && userAddress.toLowerCase() === address.toLowerCase()This ensures
isValidAddressistrueonly when both addresses exist and match, making the intent explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/(mobile-ui)/history/page.tsx(2 hunks)src/components/Claim/Claim.tsx(9 hunks)src/hooks/wallet/useWallet.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-11-18T21:36:11.486Z
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#535
File: src/components/Claim/Claim.tsx:142-146
Timestamp: 2024-11-18T21:36:11.486Z
Learning: In `src/components/Claim/Claim.tsx`, external calls like token price fetching and cross-chain details retrieval are already encapsulated within existing `try...catch` blocks, so additional error handling may be unnecessary.
Applied to files:
src/components/Claim/Claim.tsx
📚 Learning: 2024-10-08T20:13:42.967Z
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Pay.tsx:103-111
Timestamp: 2024-10-08T20:13:42.967Z
Learning: When the token price cannot be fetched in `src/components/Request/Pay/Pay.tsx` within the `PayRequestLink` component, set `tokenPriceData.price` to 0 to ensure the UI remains functional. Since Squid uses their own price engine for x-chain fulfillment transactions, this approach will not affect the transaction computation.
Applied to files:
src/components/Claim/Claim.tsx
📚 Learning: 2025-05-15T14:47:26.891Z
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#857
File: src/hooks/useWebSocket.ts:77-82
Timestamp: 2025-05-15T14:47:26.891Z
Learning: The useWebSocket hook in src/hooks/useWebSocket.ts is designed to provide raw history entries, while the components using it (such as HomeHistory.tsx) are responsible for implementing deduplication logic based on UUID to prevent duplicate entries when combining WebSocket data with other data sources.
Applied to files:
src/app/(mobile-ui)/history/page.tsx
📚 Learning: 2025-04-11T11:33:53.245Z
Learnt from: kushagrasarathe
PR: peanutprotocol/peanut-ui#798
File: src/components/Home/HomeHistory.tsx:138-192
Timestamp: 2025-04-11T11:33:53.245Z
Learning: In the HomeHistory component, infinite scrolling is intentionally not implemented despite the presence of useInfiniteQuery and IntersectionObserver code. The component is designed to only display the first 5 entries with a "View all transactions" link for viewing the complete history.
Applied to files:
src/app/(mobile-ui)/history/page.tsx
🧬 Code graph analysis (3)
src/components/Claim/Claim.tsx (3)
src/services/sendLinks.ts (1)
sendLinksApi(37-217)src/utils/general.utils.ts (1)
isStableCoin(1197-1199)src/app/actions/tokens.ts (1)
fetchTokenPrice(106-149)
src/app/(mobile-ui)/history/page.tsx (4)
src/redux/hooks.ts (1)
useUserStore(13-13)src/hooks/useWebSocket.ts (1)
useWebSocket(19-186)src/hooks/useTransactionHistory.ts (1)
HistoryResponse(17-21)src/constants/query.consts.ts (1)
TRANSACTIONS(2-2)
src/hooks/wallet/useWallet.ts (5)
src/redux/hooks.ts (2)
useAppDispatch(5-5)useWalletStore(10-10)src/hooks/useZeroDev.ts (1)
useZeroDev(36-187)src/context/authContext.tsx (1)
useAuth(191-197)src/hooks/wallet/useBalance.ts (1)
useBalance(16-41)src/redux/slices/wallet-slice.ts (1)
walletActions(24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (9)
src/hooks/wallet/useWallet.ts (3)
34-38: LGTM: Clean Redux sync for backward compatibility.The effect properly syncs TanStack Query balance data to Redux, maintaining compatibility with existing code that reads from the Redux store.
43-51: LGTM: Mutation-based send with proper result structure.The sendMoney function correctly delegates to the mutation and returns the full result, maintaining backward compatibility with the expected return shape.
82-88: No type safety issues found—code is correct as-is.The Redux state types
balanceasstring | undefinedand always sets it via.toString(). In the hook, the code safely checksreduxBalance !== undefinedbefore callingBigInt(), ensuring that only string values (neverundefinedor other types) are passed to the conversion. This is type-safe and works correctly.src/app/(mobile-ui)/history/page.tsx (2)
78-84: LGTM: Balance invalidation correctly scoped to user's wallet.The balance query is invalidated with the correct scope (user's wallet address), ensuring the balance refreshes after receiving a new transaction via WebSocket.
17-21: LGTM: Clean imports and QueryClient setup.The imports for TanStack Query, WebSocket, and query keys are properly structured, and the queryClient is correctly initialized for cache mutations.
Also applies to: 29-29
src/components/Claim/Claim.tsx (4)
78-91: LGTM: Well-configured query for one-time link fetching.The retry configuration (3 attempts with linear backoff) addresses RPC sync race conditions mentioned in the PR objectives, and the zero cache times are appropriate for one-time-use links.
229-247: LGTM: Price fetch properly isolated from claim flow.The try/catch isolates price fetch failures, allowing the link to remain claimable even without price display. This aligns with learnings about price fetch not blocking core functionality. Based on learnings.
268-275: LGTM: Clean error handling for query failures.The separate effect properly handles sendLink fetch errors, setting appropriate state and reporting to Sentry.
190-266: Link processing flow is well-structured.The effect properly coordinates link processing steps: checking link status for CLAIMED/CANCELLED states, and conditionally performing user-related checks after user fetch completes. The async flow is clear aside from the password extraction issue flagged separately.
|
@coderabbitai review in depth |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 546 🏁 Script executed: Length of output: 72182 🏁 Script executed: Length of output: 201 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 882 🏁 Script executed: Length of output: 710 🏁 Script executed: Length of output: 816 🏁 Script executed: Length of output: 2805 🏁 Script executed: Length of output: 1044 🏁 Script executed: Length of output: 2971 🏁 Script executed: Length of output: 44 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 674 🏁 Script executed: Length of output: 3552 🏁 Script executed: Length of output: 1935 🏁 Script executed: Length of output: 1558 🏁 Script executed: Length of output: 0 @Hugo0, I've completed an in-depth review of this PR. This is a high-quality implementation with solid architecture, but I've identified several areas that need attention before merging. 🎯 Overall AssessmentStrengths:
Risk Level: Medium — Core functionality is solid, but several edge cases and production readiness issues need addressing. 🔴 Critical Issues1. Race Condition in Claim Component User ValidationLocation: The // Current code (line 190)
useEffect(() => {
if (!sendLink || !linkUrl) return
const processLink = async () => {
// ... lots of processing ...
// User check happens LAST (line 253)
if (!user && !isFetchingUser) {
setLinkState(_consts.claimLinkStateType.READY)
}
}
}, [sendLink, linkUrl, user, isFetchingUser]) // Dependencies include user!Problem: If Recommendation: useEffect(() => {
if (!sendLink || !linkUrl) return
if (isFetchingUser) return // Wait for user data
const processLink = async () => {
// ... processing with stable user value ...
}
}, [sendLink, linkUrl, isFetchingUser]) // Remove user from deps
// Separate effect for user-dependent state updates
useEffect(() => {
if (!claimLinkData || isFetchingUser) return
// Update link state based on user
if (!user) {
setLinkState(_consts.claimLinkStateType.READY)
} else {
// Other user-dependent checks
}
}, [user, isFetchingUser, claimLinkData])2. Query Key Inconsistency Between Tests and ImplementationLocation: // Test file uses string literal
const TRANSACTIONS = 'transactions'
// Implementation uses imported constant
import { TRANSACTIONS } from '@/constants/query.consts'If the constant value ever changes, tests will pass but production will break. Fix: // In test file
import { TRANSACTIONS } from '@/constants/query.consts'
// Remove: const TRANSACTIONS = 'transactions'🟡 High Priority Issues3. Console Pollution (11 New Console Statements)Production-ready code should not have console.log statements. Found: // src/app/(mobile-ui)/history/page.tsx
console.log('[History] New transaction received via WebSocket:', newEntry)
console.log('[History] Duplicate transaction ignored:', newEntry.uuid)
// src/hooks/wallet/useSendMoney.ts
console.log('[useSendMoney] Transaction successful, refreshing balance and history')
console.warn('[useSendMoney] Insufficient balance for optimistic update')
console.error('[useSendMoney] Transaction failed, rolled back balance:', error)
// src/components/Claim/Claim.tsx
console.warn('[Claim] Token price fetch failed, continuing without price:', priceError)
console.error('Error processing link:', error)
console.error('Failed to load link:', sendLinkError)Recommendations:
// utils/logger.ts
export const logger = {
info: (...args: any[]) => {
if (process.env.NODE_ENV === 'development') {
console.log(...args)
}
},
warn: console.warn,
error: console.error,
}
// Usage
logger.info('[History] New transaction received:', newEntry)4. Missing Error UI for WebSocket Connection FailuresLocation: The history page uses WebSocket but doesn't show any UI feedback when the connection fails. Users might not see real-time updates and won't know why. Recommendation: const { status: wsStatus } = useWebSocket({
username: user?.user.username ?? undefined,
onHistoryEntry: (newEntry) => { /* ... */ }
})
// Add banner or toast
{wsStatus === 'error' && (
<div className="websocket-error-banner">
Real-time updates unavailable. Pull down to refresh.
</div>
)}5. Balance Query Runs on Every Render When Address is UndefinedLocation: queryFn: async () => {
if (!address) {
return 0n // Returns immediately but still runs
}
// ...
}The Better approach: export const useBalance = (address: Address | undefined) => {
return useQuery({
queryKey: ['balance', address],
queryFn: async () => {
// TypeScript now knows address is defined
const balance = await peanutPublicClient.readContract({
address: PEANUT_WALLET_TOKEN,
abi: erc20Abi,
functionName: 'balanceOf',
args: [address!], // Safe because enabled guards this
})
return balance
},
enabled: !!address,
// ... rest
})
}🟢 Medium Priority Issues6. Potential Memory Leak: InfiniteData Cache GrowthLocation: The WebSocket handler keeps prepending new entries to the first page indefinitely. Over time, the first page could grow very large. Current behavior: return {
...oldData,
pages: oldData.pages.map((page, index) => {
if (index === 0) {
return {
...page,
entries: [newEntry, ...page.entries], // Grows forever
}
}
return page
}),
}Recommendation: const MAX_FIRST_PAGE_SIZE = 50 // Or match your limit (20)
if (index === 0) {
const newEntries = [newEntry, ...page.entries]
return {
...page,
entries: newEntries.slice(0, MAX_FIRST_PAGE_SIZE),
}
}7. Missing Address Validation in Direct RequestLocation: const toAddress = authUser?.user.userId ? address : recipient.address
if (!toAddress) {
throw new Error('No recipient address available')
}Good validation, but the error is thrown synchronously inside a try/catch. It should be handled more gracefully: if (!toAddress) {
setErrorState({
showError: true,
errorMessage: 'Recipient address is not available. Please try again.'
})
setLoadingState('Idle')
return
}8. Duplicate Detection Only Checks First PageLocation: // Check if entry exists on ANY page to prevent duplicates
const existsAnywhere = oldData.pages.some((p) =>
p.entries.some((e) => e.uuid === newEntry.uuid)
)✅ Good! But the comment says "ANY page" while the earlier version only checked the first page. This is correct now, but worth noting that checking all pages on every WebSocket message could be slow with many pages. Potential optimization (if performance becomes an issue): // Check first page + last 2 pages (most recent activity)
const pagesToCheck = [
oldData.pages[0],
...oldData.pages.slice(-2)
].filter(Boolean)
const existsAnywhere = pagesToCheck.some((p) =>
p.entries.some((e) => e.uuid === newEntry.uuid)
)🔵 Low Priority Issues9. Inconsistent Retry Strategy DocumentationLocation: The CHANGELOG says:
But the code uses linear backoff (1s, 2s, 3s): retryDelay: (attemptIndex) => (attemptIndex + 1) * 1000Fix: Update changelog to match: - 3 attempts with linear backoff (1s, 2s, 3s delays)10. Test Coverage Gap: Concurrent WebSocket EventsLocation: Tests cover sequential duplicate detection but not concurrent events (two transactions arriving at the same time). Add test case: it('should handle concurrent duplicate events correctly', async () => {
// Simulate race condition: two identical events arrive before React can update
const duplicateEntry = { uuid: 'tx-1', /* ... */ }
// Fire both updates synchronously
handleNewHistoryEntry(duplicateEntry)
handleNewHistoryEntry(duplicateEntry)
const data = queryClient.getQueryData(/* ... */)
expect(data?.pages[0].entries).toHaveLength(1) // Should only have one
})11. Balance Refetch Validation is RedundantLocation: const fetchBalance = useCallback(async () => {
if (!address) {
console.warn('Cannot fetch balance, address is undefined.')
return
}
if (!isValidAddress) {
console.warn('Skipping fetch balance, address is not the same as the user address.')
return
}
await refetchBalance()
}, [address, isValidAddress, refetchBalance])Since Simplification: const fetchBalance = useCallback(async () => {
await refetchBalance()
}, [refetchBalance])🧪 Test Quality AssessmentExcellent test coverage overall! Both test files are comprehensive: ✅ useSendMoney.test.tsx (234 lines)
✅ websocket-duplicate-detection.test.tsx (293 lines)
Missing:
📋 Actionable RecommendationsMust Fix Before Merge:
Should Fix Soon:
Nice to Have:
🎨 Code Quality NotesExcellent documentation in TANSTACK_QUERY_OPPORTUNITIES.md — this is a model for future refactors! Minor style inconsistency:
Type safety is good overall, but consider: // In useBalance, the return type could be explicit
export const useBalance = (address: Address | undefined): UseQueryResult<bigint> => {
// ...
}🚀 Performance ImpactExpected improvements:
Potential concerns:
✅ Security Check
Final VerdictThis is well-architected code with solid foundations. The TanStack Query integration is done correctly, and the test coverage is impressive. However, production readiness requires addressing the console.log cleanup, race condition in Claim.tsx, and WebSocket error UI. Recommendation: Request changes for critical issues (#1-4), then approve after fixes. Would you like me to create GitHub issues for the medium/low priority items to track them separately? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Request/direct-request/views/Initial.direct.request.view.tsx (1)
93-97: Good defensive validation with clear intent.Extracting
toAddressto a local variable and validating it before the API call improves readability and provides explicit error handling. This complements the existingisButtonDisabledcheck (line 79) with defense in depth.Optionally, consider making the error message more specific:
- if (!toAddress) { - throw new Error('No recipient address available') - } + if (!toAddress) { + const context = authUser?.user.userId ? 'wallet address' : 'recipient address' + throw new Error(`No ${context} available`) + }src/hooks/wallet/useBalance.ts (1)
19-23: Consider returningnullinstead of0nfor semantic clarity.Good fix returning
0ninstead of throwing to avoid error state on manual refetch. However,0ncould be confused with an actual zero balance.Consider returning
nullorundefinedto make the "no address" case semantically distinct:queryFn: async () => { if (!address) { - // Return 0 instead of throwing to avoid error state on manual refetch - return 0n + // Return null when no address to distinguish from zero balance + return null }Then update the return type to
bigint | nulland ensure consumers handle null appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/(mobile-ui)/add-money/crypto/page.tsx(1 hunks)src/app/(mobile-ui)/history/page.tsx(2 hunks)src/components/Claim/Claim.tsx(10 hunks)src/components/Claim/Link/Initial.view.tsx(1 hunks)src/components/Request/direct-request/views/Initial.direct.request.view.tsx(1 hunks)src/hooks/wallet/useBalance.ts(1 hunks)src/services/sendLinks.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-04-30T21:31:27.790Z
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#827
File: src/components/Claim/Link/Initial.view.tsx:120-126
Timestamp: 2025-04-30T21:31:27.790Z
Learning: The `sendLinksApi.claim` function in the Peanut Protocol UI accepts both username and wallet address as the first parameter.
Applied to files:
src/components/Claim/Link/Initial.view.tsx
📚 Learning: 2025-05-15T14:47:26.891Z
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#857
File: src/hooks/useWebSocket.ts:77-82
Timestamp: 2025-05-15T14:47:26.891Z
Learning: The useWebSocket hook in src/hooks/useWebSocket.ts is designed to provide raw history entries, while the components using it (such as HomeHistory.tsx) are responsible for implementing deduplication logic based on UUID to prevent duplicate entries when combining WebSocket data with other data sources.
Applied to files:
src/app/(mobile-ui)/history/page.tsx
📚 Learning: 2025-04-11T11:33:53.245Z
Learnt from: kushagrasarathe
PR: peanutprotocol/peanut-ui#798
File: src/components/Home/HomeHistory.tsx:138-192
Timestamp: 2025-04-11T11:33:53.245Z
Learning: In the HomeHistory component, infinite scrolling is intentionally not implemented despite the presence of useInfiniteQuery and IntersectionObserver code. The component is designed to only display the first 5 entries with a "View all transactions" link for viewing the complete history.
Applied to files:
src/app/(mobile-ui)/history/page.tsx
📚 Learning: 2024-11-18T21:36:11.486Z
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#535
File: src/components/Claim/Claim.tsx:142-146
Timestamp: 2024-11-18T21:36:11.486Z
Learning: In `src/components/Claim/Claim.tsx`, external calls like token price fetching and cross-chain details retrieval are already encapsulated within existing `try...catch` blocks, so additional error handling may be unnecessary.
Applied to files:
src/components/Claim/Claim.tsx
📚 Learning: 2024-10-08T20:13:42.967Z
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Pay.tsx:103-111
Timestamp: 2024-10-08T20:13:42.967Z
Learning: When the token price cannot be fetched in `src/components/Request/Pay/Pay.tsx` within the `PayRequestLink` component, set `tokenPriceData.price` to 0 to ensure the UI remains functional. Since Squid uses their own price engine for x-chain fulfillment transactions, this approach will not affect the transaction computation.
Applied to files:
src/components/Claim/Claim.tsx
🧬 Code graph analysis (6)
src/app/(mobile-ui)/add-money/crypto/page.tsx (1)
src/components/AddMoney/views/CryptoDepositQR.view.tsx (1)
CryptoDepositQR(20-77)
src/components/Claim/Link/Initial.view.tsx (1)
src/services/sendLinks.ts (1)
sendLinksApi(38-218)
src/components/Request/direct-request/views/Initial.direct.request.view.tsx (1)
src/services/users.ts (1)
usersApi(40-101)
src/app/(mobile-ui)/history/page.tsx (4)
src/redux/hooks.ts (1)
useUserStore(13-13)src/hooks/useWebSocket.ts (1)
useWebSocket(19-186)src/hooks/useTransactionHistory.ts (1)
HistoryResponse(17-21)src/constants/query.consts.ts (1)
TRANSACTIONS(2-2)
src/hooks/wallet/useBalance.ts (1)
src/constants/zerodev.consts.ts (2)
peanutPublicClient(68-68)PEANUT_WALLET_TOKEN(20-20)
src/components/Claim/Claim.tsx (3)
src/services/sendLinks.ts (2)
sendLinksApi(38-218)getParamsFromLink(10-10)src/utils/general.utils.ts (1)
isStableCoin(1197-1199)src/app/actions/tokens.ts (1)
fetchTokenPrice(106-149)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (8)
src/components/Request/direct-request/views/Initial.direct.request.view.tsx (1)
102-102: LGTM!Correctly uses the validated
toAddressvariable.src/services/sendLinks.ts (1)
10-10: LGTM! Centralized SDK export.Re-exporting
getParamsFromLinkfrom the shared service layer is a good practice that enables consistent usage across components.src/app/(mobile-ui)/add-money/crypto/page.tsx (1)
123-129: LGTM! Good defensive guard.The
finalDepositAddressguard prevents rendering with undefined/null addresses and provides a reasonable fallback by navigating to the root.src/app/(mobile-ui)/history/page.tsx (1)
44-88: LGTM! WebSocket integration with proper duplicate detection and scoped invalidation.The implementation correctly addresses previous concerns:
- Line 57:
existsAnywherenow checks across all pages, preventing cross-page duplicates- Lines 81-86: Balance invalidation is properly scoped to the user's wallet address with the full query key
['balance', walletAddress]The real-time update flow is well-structured with appropriate logging and cache mutations.
src/hooks/wallet/useBalance.ts (1)
34-41: LGTM! Sensible query configuration.The caching, refetch, and retry settings are appropriate for balance queries:
- 30-second auto-refresh aligns with PR objectives
- Refetch on focus/reconnect keeps data fresh
- Exponential backoff with retry cap is good practice
src/components/Claim/Claim.tsx (3)
78-91: LGTM! TanStack Query with appropriate one-time-use semantics.The query configuration is well-suited for claim links:
- Retry with linear backoff addresses RPC sync issues mentioned in the PR
staleTime: 0andgcTime: 0enforce one-time-use semantics per linkenabled: !!linkUrlprevents unnecessary fetches
191-202: LGTM! Robust password extraction with proper guard.The code now uses the shared
getParamsFromLink(linkUrl)parser and validates the password before use, addressing the previous concern about fragileurl.hash.split('=')[1]parsing.
234-252: Excellent resilience pattern for price fetch.Isolating the price fetch in a try-catch allows the link to remain claimable even if price display fails. This is a good user experience improvement.
Based on learnings.
| // Ensure we have a valid recipient (username or address) | ||
| const recipient = user?.user.username ?? address | ||
| if (!recipient) { | ||
| throw new Error('No recipient address available') | ||
| } | ||
|
|
||
| if (autoClaim) { | ||
| await sendLinksApi.autoClaimLink(user?.user.username ?? address, claimLinkData.link) | ||
| await sendLinksApi.autoClaimLink(recipient, claimLinkData.link) | ||
| } else { | ||
| await sendLinksApi.claim(user?.user.username ?? address, claimLinkData.link) | ||
| await sendLinksApi.claim(recipient, claimLinkData.link) |
There was a problem hiding this comment.
Variable shadowing: rename the local recipient to avoid confusion.
The local recipient variable (line 212) shadows the component's recipient prop (line 59), which has a different type ({ name?, address } vs string). This creates confusion since the function uses both recipient.address from the component state (lines 167, 232, 240) and the local string recipient (lines 218, 220).
Apply this diff to rename the local variable:
- // Ensure we have a valid recipient (username or address)
- const recipient = user?.user.username ?? address
- if (!recipient) {
+ // Ensure we have a valid recipient identifier (username or address)
+ const recipientIdentifier = user?.user.username ?? address
+ if (!recipientIdentifier) {
throw new Error('No recipient address available')
}
if (autoClaim) {
- await sendLinksApi.autoClaimLink(recipient, claimLinkData.link)
+ await sendLinksApi.autoClaimLink(recipientIdentifier, claimLinkData.link)
} else {
- await sendLinksApi.claim(recipient, claimLinkData.link)
+ await sendLinksApi.claim(recipientIdentifier, claimLinkData.link)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Ensure we have a valid recipient (username or address) | |
| const recipient = user?.user.username ?? address | |
| if (!recipient) { | |
| throw new Error('No recipient address available') | |
| } | |
| if (autoClaim) { | |
| await sendLinksApi.autoClaimLink(user?.user.username ?? address, claimLinkData.link) | |
| await sendLinksApi.autoClaimLink(recipient, claimLinkData.link) | |
| } else { | |
| await sendLinksApi.claim(user?.user.username ?? address, claimLinkData.link) | |
| await sendLinksApi.claim(recipient, claimLinkData.link) | |
| // Ensure we have a valid recipient identifier (username or address) | |
| const recipientIdentifier = user?.user.username ?? address | |
| if (!recipientIdentifier) { | |
| throw new Error('No recipient address available') | |
| } | |
| if (autoClaim) { | |
| await sendLinksApi.autoClaimLink(recipientIdentifier, claimLinkData.link) | |
| } else { | |
| await sendLinksApi.claim(recipientIdentifier, claimLinkData.link) |
🤖 Prompt for AI Agents
In src/components/Claim/Link/Initial.view.tsx around lines 211 to 220, the local
variable recipient (a string) shadows the component prop recipient (an object
with name? and address) causing confusion and mixed usages; rename the local
variable to recipientAddress (or similar) and update its declaration and all
local usages within this block (the existence check and the calls to
sendLinksApi.autoClaimLink / sendLinksApi.claim) to use recipientAddress,
leaving the component prop recipient untouched for other code that accesses
recipient.address.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/app/(mobile-ui)/history/__tests__/websocket-duplicate-detection.test.tsx (5)
12-24: Import domain types instead of redefining them.Redefining HistoryEntry/HistoryResponse here risks drift from production types (e.g., cursor?: string on HistoryResponse). Import the exported types from src/hooks/useTransactionHistory.ts to keep tests aligned.
Apply:
-import { QueryClient } from '@tanstack/react-query' -import type { InfiniteData } from '@tanstack/react-query' +import { QueryClient } from '@tanstack/react-query' +import type { InfiniteData } from '@tanstack/react-query' import { TRANSACTIONS } from '@/constants/query.consts' - -// Mock transaction entry type -type HistoryEntry = { - uuid: string - type: string - status: string - timestamp: string - amount: string -} - -type HistoryResponse = { - entries: HistoryEntry[] - hasMore: boolean -} +import type { HistoryEntry, HistoryResponse } from '@/hooks/useTransactionHistory'If HistoryEntry isn’t exported yet, consider exporting it alongside HistoryResponse. Based on learnings.
37-62: De-duplicate the insertion logic by extracting a shared helper.handleNewHistoryEntry reimplements production behavior. Extract a pure utility (e.g., insertNonDuplicateFirstPage) and import it in both page code and tests to prevent divergence.
Example new helper (outside this file):
// src/features/history/insertNonDuplicateFirstPage.ts import type { InfiniteData } from '@tanstack/react-query' import type { HistoryEntry, HistoryResponse } from '@/hooks/useTransactionHistory' export function insertNonDuplicateFirstPage( data: InfiniteData<HistoryResponse> | undefined, newEntry: HistoryEntry ): InfiniteData<HistoryResponse> | undefined { if (!data) return data return { ...data, pages: data.pages.map((page, i) => { if (i !== 0) return page if (page.entries.some(e => e.uuid === newEntry.uuid)) return page return { ...page, entries: [newEntry, ...page.entries] } }), } }Then in this test:
-const handleNewHistoryEntry = (newEntry: HistoryEntry, limit: number = 20) => { - queryClient.setQueryData<InfiniteData<HistoryResponse>>([TRANSACTIONS, 'infinite', { limit }], (oldData) => { - if (!oldData) return oldData - // ... - }) -} +import { insertNonDuplicateFirstPage } from '@/features/history/insertNonDuplicateFirstPage' +const handleNewHistoryEntry = (newEntry: HistoryEntry, limit = 20) => { + queryClient.setQueryData<InfiniteData<HistoryResponse>>( + [TRANSACTIONS, 'infinite', { limit }], + (oldData) => insertNonDuplicateFirstPage(oldData, newEntry) + ) +}Based on learnings.
162-211: Add coverage: duplicate present on a non-first page should be ignored.Current tests only check duplicates on the first page. Add a case where the duplicate exists on page 2+ and ensure it isn’t re-inserted on page 1.
New test:
it('should NOT add when duplicate exists on a later page', () => { const initialData: InfiniteData<HistoryResponse> = { pages: [ { entries: [], hasMore: true }, { entries: [{ uuid: 'tx-2', type: 'RECEIVE', status: 'COMPLETED', timestamp: '2025-01-02', amount: '20' }], hasMore: false }, ], pageParams: [undefined, 'cursor-1'], } queryClient.setQueryData([TRANSACTIONS, 'infinite', { limit: 20 }], initialData) const duplicate = { uuid: 'tx-2', type: 'RECEIVE', status: 'COMPLETED', timestamp: '2025-01-03', amount: '20' } as HistoryEntry handleNewHistoryEntry(duplicate) const updated = queryClient.getQueryData<InfiniteData<HistoryResponse>>([TRANSACTIONS, 'infinite', { limit: 20 }]) expect(updated?.pages[0].entries).toHaveLength(0) expect(updated?.pages[1].entries).toHaveLength(1) })This aligns with the dedup-at-consumer pattern. Based on learnings.
29-35: Optional: silence React Query logs in tests.If tests emit React Query network/visibility warnings, silence them per-suite to keep output clean.
Example:
import { setLogger } from '@tanstack/react-query' beforeEach(() => { setLogger({ log: () => {}, warn: () => {}, error: () => {} }) })
39-41: I need to verify the actual query key usage in the production code before concluding:Now I need to examine the actual production hook and test file to verify the key pattern:Based on the web search results and the original review comment, I can now provide a comprehensive analysis. Manually declaring query keys is error-prone and makes changes harder in the future, and that's why a query key factory per feature is recommended as a simple object with entries and functions that produce query keys. The review comment's suggestion aligns with this established best practice.
However, the critical issue is that no key factory exists yet in the codebase (as confirmed by the shell script search). This means:
- The suggestion to use
transactionsInfiniteKey(limit)references a function that doesn't exist- Both production and test code likely hardcode the same pattern independently
- The risk of drift exists, but implementing the fix requires first creating the key factory
Since the suggestion is a valid refactoring based on React Query best practices but requires creating infrastructure that doesn't yet exist, this falls into the category of an optional refactoring recommendation—it improves maintainability but isn't a critical bug.
Centralize transaction history query key via a shared key factory.
The test hardcodes
[TRANSACTIONS, 'infinite', { limit }]and similar key patterns across multiple test locations. Manually declaring query keys is error-prone and makes changes harder in the future; a query key factory per feature is recommended.Create a key factory in
src/hooks/useTransactionHistory.ts(or a dedicatedkeysmodule) and export it for use in both production queries and tests:+export const transactionsKeys = { + infinite: (limit: number) => [TRANSACTIONS, 'infinite', { limit }], + // other transaction key variants as needed +}Then update test calls from
[TRANSACTIONS, 'infinite', { limit }]totransactionsKeys.infinite(limit). This prevents key drift between test setup and production hook definitions.Applies to: lines 39-41, 86-88, 100-107, 131-133, 147-151, 201-206, 225-227, 238-243, 262-264, 275-279
src/components/Claim/Claim.tsx (2)
192-266: Good refactor: link processing now waits for user data.The
isFetchingUserguard at line 194 properly addresses the race condition concern from previous reviews by ensuring user validation completes before heavy link processing. The isolated price fetch (lines 236-254) correctly prevents price errors from blocking valid links.Minor: Price condition may skip legitimate zero values.
Line 250's
if (0 < price)will skip settingtokenPriceif the price is exactly 0. If this is intentional to avoid displaying $0.00, consider documenting it.
389-389: Verify: onClose re-triggers link query.Setting
linkUrlto the current URL on drawer close will re-fetch the link from the server. If this is intentional (e.g., to detect if the link was cancelled while the drawer was open), consider adding a comment to clarify. Otherwise, this creates unnecessary network calls.If refreshing state after drawer close is needed, consider a more explicit refetch:
onClose={() => { // Refetch to detect link state changes (e.g., cancellation) queryClient.invalidateQueries({ queryKey: ['sendLink', linkUrl] }) }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/(mobile-ui)/history/__tests__/websocket-duplicate-detection.test.tsx(1 hunks)src/components/Claim/Claim.tsx(9 hunks)src/hooks/wallet/useBalance.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/wallet/useBalance.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-15T14:47:26.891Z
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#857
File: src/hooks/useWebSocket.ts:77-82
Timestamp: 2025-05-15T14:47:26.891Z
Learning: The useWebSocket hook in src/hooks/useWebSocket.ts is designed to provide raw history entries, while the components using it (such as HomeHistory.tsx) are responsible for implementing deduplication logic based on UUID to prevent duplicate entries when combining WebSocket data with other data sources.
Applied to files:
src/app/(mobile-ui)/history/__tests__/websocket-duplicate-detection.test.tsx
📚 Learning: 2024-11-18T21:36:11.486Z
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#535
File: src/components/Claim/Claim.tsx:142-146
Timestamp: 2024-11-18T21:36:11.486Z
Learning: In `src/components/Claim/Claim.tsx`, external calls like token price fetching and cross-chain details retrieval are already encapsulated within existing `try...catch` blocks, so additional error handling may be unnecessary.
Applied to files:
src/components/Claim/Claim.tsx
🧬 Code graph analysis (2)
src/app/(mobile-ui)/history/__tests__/websocket-duplicate-detection.test.tsx (2)
src/hooks/useTransactionHistory.ts (1)
HistoryResponse(17-21)src/constants/query.consts.ts (1)
TRANSACTIONS(2-2)
src/components/Claim/Claim.tsx (3)
src/services/sendLinks.ts (3)
sendLinksApi(38-218)getParamsFromLink(10-10)ESendLinkStatus(8-8)src/utils/general.utils.ts (1)
isStableCoin(1197-1199)src/app/actions/tokens.ts (1)
fetchTokenPrice(106-149)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (5)
src/app/(mobile-ui)/history/__tests__/websocket-duplicate-detection.test.tsx (1)
64-108: Verification complete: Repository uses Jest.The codebase exclusively uses Jest (confirmed by widespread
jest.fn()andjest.spyOn()patterns throughout setup files and test files). The test file correctly usesjest.spyOn()at line 142, so no replacement withvi.spyOn()is needed. Test scenarios, assertions, and React Query v5 InfiniteData alignment are solid.src/components/Claim/Claim.tsx (4)
2-2: LGTM: SDK utilities and TanStack Query imports.The imports correctly bring in
generateKeysFromString,getParamsFromLink(addressing the previous password parsing concern), anduseQueryfor the query-driven flow.Also applies to: 17-17, 20-20
78-91: LGTM: Well-configured TanStack Query with retry logic.The retry configuration (3 attempts with 1s/2s/3s linear backoff) aligns with the PR objectives. The
staleTime: 0andgcTime: 0correctly enforce one-time-use semantics for each unique link URL.
268-287: LGTM: Clean separation of user-dependent state logic.This effect properly isolates user role determination after link processing completes. The precedence logic (claimed/cancelled state takes priority) is correct, and the guards ensure data is ready before state updates.
289-296: LGTM: Focused error handling.The error effect cleanly handles query failures and reports to Sentry appropriately.
src/app/(mobile-ui)/history/__tests__/websocket-duplicate-detection.test.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Claim/Claim.tsx (1)
190-266: Remove unnecessaryisFetchingUserdependency to avoid delaying link processing.Line 194 waits for
isFetchingUserto becomefalse, but this effect doesn't useuserdata—the comment at line 191 confirms it processes "WITHOUT user-dependent logic." IncludingisFetchingUserin the dependency array (line 266) also causes the effect to re-run when user fetching completes, potentially duplicating work. The user-dependent state logic has been correctly moved to a separate effect (lines 270-287).Apply this diff to remove the unnecessary guard and dependency:
useEffect(() => { if (!sendLink || !linkUrl) return - if (isFetchingUser) return // Wait for user data to be ready before processing const processLink = async () => { try { @@ -263,7 +262,7 @@ } processLink() - }, [sendLink, linkUrl, isFetchingUser, setSelectedChainID, setSelectedTokenAddress]) + }, [sendLink, linkUrl, setSelectedChainID, setSelectedTokenAddress])
🧹 Nitpick comments (1)
src/components/Claim/Claim.tsx (1)
268-287: Good separation of user-dependent logic, but watch for brief UI flicker.Extracting user-dependent state updates into a separate effect is a solid design. However, once you remove the
isFetchingUserguard from the main processing effect (per the previous comment), there's a small window whereclaimLinkDatamay be set (line 212) andlinkStatedefaults toCLAIM(line 257) before this effect runs to update it toCLAIM_SENDER. Users might see a brief flicker of the wrong screen.If flicker becomes noticeable, consider deferring
setLinkStatein the main effect until this user-dependent effect has determined the final state, or introduce a transient loading state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Claim/Claim.tsx(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-18T21:36:11.486Z
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#535
File: src/components/Claim/Claim.tsx:142-146
Timestamp: 2024-11-18T21:36:11.486Z
Learning: In `src/components/Claim/Claim.tsx`, external calls like token price fetching and cross-chain details retrieval are already encapsulated within existing `try...catch` blocks, so additional error handling may be unnecessary.
Applied to files:
src/components/Claim/Claim.tsx
🧬 Code graph analysis (1)
src/components/Claim/Claim.tsx (3)
src/services/sendLinks.ts (2)
sendLinksApi(38-218)getParamsFromLink(10-10)src/utils/general.utils.ts (1)
isStableCoin(1197-1199)src/app/actions/tokens.ts (1)
fetchTokenPrice(106-149)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (4)
src/components/Claim/Claim.tsx (4)
78-91: TanStack Query setup is sound for one-time link semantics.The retry logic (3 attempts with linear backoff) appropriately handles RPC indexing delays mentioned in the PR objectives. Setting
staleTime: 0andgcTime: 0enforces one-time use per link, preventing stale data from being served.
236-254: Excellent defensive pattern—price fetch failures won't block claims.Isolating the token price fetch in a dedicated
try/catchensures that API failures or stablecoin detection issues don't prevent users from claiming their funds. The fallback behavior (continuing without price display) is appropriate and user-friendly.
389-389: Clarify intent ofsetLinkUrlinonClosehandler.Setting
linkUrltowindow.location.hrefon close appears to be the same value already set during initialization (lines 304-309). React's state identity check should prevent a re-render/re-fetch, but the intent is unclear.If you need to refresh the link state after closing the drawer, use
queryClient.invalidateQueries(['sendLink', linkUrl])or the query'srefetch()method. If the handler is simply meant to close the drawer without refresh, consider removing thesetLinkUrlcall.Please clarify the intended behavior when the transaction receipt drawer closes.
304-309: Previous review concern addressed—effect now runs only on mount.The dependency array change from
[user]to[]prevents wasteful re-executions when user authentication state changes, as recommended in the earlier review. Sincewindow.location.hrefdoesn't change on user login/logout, running this effect once on mount is correct.
| if (isConnected && !peanutWalletAddress) { | ||
| // Ensure we have a valid deposit address | ||
| const finalDepositAddress = depositAddress ?? peanutWalletAddress | ||
| if (!finalDepositAddress) { |
2 participants
tackles this taskk: https://www.notion.so/peanutprotocol/ec22805a317a4affb146de7504137fb6?v=de61aac467d345c1876afed30d0743b4&p=279838117579806ca032fb7a2322ee5c&pm=s
also check Backend PR, larger changes there
The FE PR:
Send links retry loading details multiple times. Also fetches in parallel directly from RPC (under the assumption that the CLIENT rpc is the one that also should've indexed the send link tx since it processed teh creation)
Also refactors balance and activity to make them feel faster: Users now get auto-refreshing balance (every 30s), instant transaction updates via WebSocket, and optimistic UI feedback when sending money.