Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@Hugo0 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughWebSocket-injected history entries are now passed through the async utility Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
|
@coderabbitai Review carefully. Do these tackle actual bugs? Is there any new unexpected behavior introduced or new risks to be considered? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14258 🏁 Script executed: Length of output: 38497 @Hugo0, I've completed a thorough review of this PR. Here's my analysis: ✅ Actual Bugs FixedBug #1: Inconsistent Amount Formatting Between WebSocket and API DataFiles: Problem: WebSocket entries were not being processed through
Fix: All WebSocket entries now pass through Impact: This was causing display bugs where amounts appeared incorrectly or links were missing for real-time transaction updates. Bug #2: False "Insufficient Balance" Errors During Payment FlowFile: Problem: After optimistic balance updates on CONFIRM/STATUS views, the balance check effect would still run and compare the input amount against the reduced balance, showing false errors. Fix: Skip balance validation when Impact: Users would see incorrect balance errors mid-payment, degrading UX.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/app/(mobile-ui)/history/page.tsx (2)
71-84: Trim first page to limit after prepend to prevent unbounded growth.Prepending can exceed the intended page size (20). Slice to limit after insertion.
Apply this patch:
- entries: [completedEntry, ...page.entries], + entries: [completedEntry, ...page.entries].slice(0, 20),Optionally, derive 20 from the same limit used in the query key to avoid drift.
49-49: Reduce console noise for prod.Downgrade to debug or wrap with env guard to avoid chatty logs in production.
Also applies to: 67-67
src/components/Payment/PaymentForm/index.tsx (2)
188-191: Clear stale errors before skipping balance check on CONFIRM/STATUS.Returning before dispatch(paymentActions.setError(null)) can leave old errors visible. Clear error first, then return.
Apply:
- // Skip balance check if on CONFIRM or STATUS view (balance has been optimistically updated) - if (currentView === 'CONFIRM' || currentView === 'STATUS') { - return - } - - dispatch(paymentActions.setError(null)) + // Skip balance check if on CONFIRM or STATUS view (balance has been optimistically updated) + if (currentView === 'CONFIRM' || currentView === 'STATUS') { + dispatch(paymentActions.setError(null)) + return + } + + dispatch(paymentActions.setError(null))Also applies to: 193-193
72-80: Now let me check the actual payment store implementation and howcurrentViewis typed:Now I need to locate the payment reducer and custom hook to verify the actual typing:
Based on my verification, I have found that the review comment is accurate and points to a valid optional refactoring suggestion. The
TPaymentViewtype already exists in the codebase and string literal unions have no bundle bloat compared to enums. The standard approach in Redux Toolkit is to declare a type for state and create an initial state value that uses that type.The review comment correctly identifies that using the existing
TPaymentViewtype union forcurrentViewwould improve type safety and enable better IDE autocompletion, preventing potential typos when callingsetView().
Consider using the
TPaymentViewtype for view state transitions instead of string literals.The
TPaymentViewtype already exists insrc/redux/types/payment.types.tsand covers all view values used throughout the codebase ('INITIAL' | 'CONFIRM' | 'STATUS' | 'PUBLIC_PROFILE'). Using it consistently when initializing payment state and in the custom hook will provide better type safety and IDE autocomplete support.src/components/Home/HomeHistory.tsx (1)
92-108: Avoid sequential awaits; batch completeHistoryEntry with Promise.all and handle per-entry errors.Sequential processing can stall rendering under backlogs. Process in parallel, skip failures, then merge.
Apply:
- // Process WebSocket entries through completeHistoryEntry to format amounts correctly - for (const wsEntry of sortedWsEntries) { - const completedEntry = await completeHistoryEntry(wsEntry) - const existingIndex = entries.findIndex((entry) => entry.uuid === completedEntry.uuid) - - if (existingIndex !== -1) { - // update existing entry with latest websocket data - entries[existingIndex] = completedEntry - } else { - // add new entry if it doesn't exist - entries.push(completedEntry) - } - } + // Normalize WS entries in parallel, skipping failures + const normalized = await Promise.all( + sortedWsEntries.map(async (wsEntry) => { + try { + return await completeHistoryEntry(wsEntry) + } catch (e) { + Sentry.captureException(e) + return null + } + }) + ) + for (const completedEntry of normalized) { + if (!completedEntry) continue + const idx = entries.findIndex((entry) => entry.uuid === completedEntry.uuid) + if (idx !== -1) entries[idx] = completedEntry + else entries.push(completedEntry) + }Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/(mobile-ui)/history/page.tsx(3 hunks)src/app/(mobile-ui)/qr-pay/page.tsx(4 hunks)src/components/Home/HomeHistory.tsx(2 hunks)src/components/Payment/PaymentForm/index.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/components/Home/HomeHistory.tsxsrc/app/(mobile-ui)/qr-pay/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.tsxsrc/components/Home/HomeHistory.tsx
🧬 Code graph analysis (4)
src/app/(mobile-ui)/history/page.tsx (3)
src/utils/history.utils.ts (1)
completeHistoryEntry(200-291)src/hooks/useTransactionHistory.ts (1)
HistoryResponse(17-21)src/constants/query.consts.ts (1)
TRANSACTIONS(2-2)
src/components/Home/HomeHistory.tsx (2)
src/hooks/useBridgeKycFlow.ts (1)
KycHistoryEntry(17-23)src/utils/history.utils.ts (1)
completeHistoryEntry(200-291)
src/app/(mobile-ui)/qr-pay/page.tsx (1)
src/utils/history.utils.ts (2)
HistoryEntry(89-131)completeHistoryEntry(200-291)
src/components/Payment/PaymentForm/index.tsx (1)
src/redux/hooks.ts (1)
usePaymentStore(12-12)
🔇 Additional comments (1)
src/app/(mobile-ui)/history/page.tsx (1)
56-58: Cache key parity is correct—no issues found.The
useTransactionHistoryhook called withmode: 'infinite'andlimit: 20(lines 40-43) produces the query key[TRANSACTIONS, 'infinite', { limit: 20 }], which matches exactly the cache key insetQueryData(line 57). The 'infinite' mode intentionally excludes username flags (only the 'public' mode includes username); this aligns with the hook's design for querying the current user's own transaction history.
|
@coderabbitai Review the entire PR again with risk analysis. |
🧩 Analysis chain🏁 Script executed: Length of output: 37401 @Hugo0, I've completed a comprehensive re-review with deep risk analysis. Here's my complete assessment: 🔴 CRITICAL ISSUESIssue #1: Race Condition in HomeHistory.tsxSeverity: CRITICAL // Line 85: Function is called but NOT awaited
processEntries()Problem: The async Attack Vector: User receives multiple WebSocket entries → first call starts processing → dependencies change → second call starts → first call finishes and sets stale state. Fix Required: useEffect(() => {
if (!isLoading && historyData?.entries) {
let cancelled = false
const processEntries = async () => {
// ... processing logic
if (!cancelled) {
setCombinedEntries(entries.slice(0, isPublic ? 20 : 5))
}
}
processEntries()
return () => { cancelled = true }
}
}, [historyData, wsHistoryEntries, isPublic, user, isLoading, isSameUser])Issue #2: Unhandled API Failures in completeHistoryEntrySeverity: HIGH The // From completeHistoryEntry - NO try-catch here
const price = await getCurrencyPrice(entry.currency.code)
usdAmount = (Number(entry.currency.amount) / price.buy).toString()Impact: If the currency price API fails (rate limit, network error, invalid currency code), the entire function throws. While callers in Recommendation: Add defensive error handling inside case EHistoryEntryType.BRIDGE_ONRAMP: {
tokenSymbol = entry.tokenSymbol
usdAmount = entry.amount.toString()
if (entry.currency?.code) {
entry.currency.code = entry.currency.code.toUpperCase()
}
if (usdAmount === entry.currency?.amount && entry.currency?.code && entry.currency?.code !== 'USD') {
try {
const price = await getCurrencyPrice(entry.currency.code)
usdAmount = (Number(entry.currency.amount) / price.buy).toString()
} catch (error) {
console.error('[completeHistoryEntry] Failed to fetch currency price:', error)
// Fallback: use original amount as USD approximation
usdAmount = entry.amount.toString()
}
}
break
}
|
| Risk | Severity | Likelihood | Impact | Mitigation Required |
|---|---|---|---|---|
| Race condition in HomeHistory | CRITICAL | High | UI shows stale/wrong data | YES - Add cancellation token |
| getCurrencyPrice failure | HIGH | Medium | Transaction display breaks | YES - Add error handling |
| Fallback data shows wei amounts | HIGH | Medium | Users see wrong amounts | YES - Format fallback amounts |
| SimpleFi guards hide bugs | MEDIUM | Low | Masks state management issues | MAYBE - Add telemetry |
| Sequential processing slow | MEDIUM | High | Sluggish UI with multiple txs | OPTIONAL - Parallel processing |
| Long retry delays | MEDIUM | High | User waits 15s for errors | OPTIONAL - Add user feedback |
| Async callback support | LOW | Low | Callbacks might not complete | VERIFY - Check useWebSocket impl |
🎯 RECOMMENDATIONS
Must Fix Before Production:
- ✅ Fix HomeHistory race condition (CRITICAL)
- ✅ Add error handling to getCurrencyPrice calls (HIGH)
- ✅ Fix fallback amount formatting for DEPOSIT entries (HIGH)
Should Fix:
⚠️ Add telemetry to SimpleFi approval guards⚠️ Consider parallel processing in HomeHistory⚠️ Add user feedback during long Claim retries
Verify:
- 🔍 Check that
useWebSocketawaits async callbacks - 🔍 Load test with multiple simultaneous bridge transactions
Would you like me to generate the complete fixed code for the critical issues (1-3)?
✅ Actions performed
Full review triggered.
1 participant
Getting ready for prod. Various bug fixes attempted here.