Dynamic Wallet-State Binding by adding hook dependency#55
Conversation
|
Warning Review limit reached
More reviews will be available in 55 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe transactions hook now reads walletAddress from useWallet, resets local state when the wallet changes, uses a per-wallet localStorage cache with expiry (restoring blockNumber strings to BigInt), avoids cross-wallet race updates during fetch, and only clears the active wallet’s cache. ChangesHook & Wallet-aware Caching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dashboard/hooks/use-transactions.ts (2)
22-42:⚠️ Potential issue | 🟠 MajorCache is not isolated per wallet—transactions may leak between accounts.
Adding
walletAddressto the dependency array correctly triggers a re-run when the wallet changes, but the cache key (CACHE_KEY) is static. This means:
- When user switches wallets, the effect loads cached transactions that may belong to a different wallet.
- If the cache is empty/expired, previously loaded transactions remain in state until a new fetch completes.
Consider making the cache wallet-specific and clearing state on wallet change:
🔧 Proposed fix
+ // Clear state when wallet changes useEffect(() => { + // Reset state for new wallet context + setTransactions([]); + setHasFetched(false); + setError(null); + + if (!walletAddress) return; + + const cacheKey = `${CACHE_KEY}_${walletAddress}`; - const cached = localStorage.getItem(CACHE_KEY); + const cached = localStorage.getItem(cacheKey); if (cached) { try { const { transactions: cachedTransactions, timestamp }: CachedData = JSON.parse(cached); const isExpired = Date.now() - timestamp > CACHE_EXPIRY; if (!isExpired && cachedTransactions.length > 0) { // Convert string blockNumber back to BigInt const restoredTransactions = cachedTransactions.map(event => ({ ...event, blockNumber: BigInt(event.blockNumber) })); setTransactions(restoredTransactions); setHasFetched(true); } } catch (err) { console.warn('Failed to parse cached transactions:', err); } } }, [walletAddress]);You'll also need to update
fetchTransactionsandclearCacheto use the wallet-specific cache key (or pass it via closure/ref).
48-50:⚠️ Potential issue | 🟠 MajorEmpty
merchantAddresscauses unfiltered data fetch—should usewalletAddressinsteadThe
merchantAddressis hardcoded to an empty string, which causes the filter condition on line 103 of transaction-service.ts (args: merchantAddress ? { receiver: merchantAddress }) to fail. This means allBoughtStableCoinsevents across all networks are fetched instead of filtering for a specific merchant, even though the comment suggests filtering intent. SincewalletAddressis available from theuseWallet()hook, it should likely be passed here to retrieve only relevant transactions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@dashboard/hooks/use-transactions.ts`:
- Around line 21-31: The cache key is inconsistent: useEffect reads
localStorage.getItem(cacheKey) where cacheKey = `${CACHE_KEY}_${walletAddress}`
but other reads/writes/clears use plain CACHE_KEY; update all localStorage
interactions (reads, writes, and removes) to use the same per-wallet key pattern
`${CACHE_KEY}_${walletAddress}` and ensure you skip any caching operations when
walletAddress is falsy (e.g., in useEffect and in the functions that
persist/clear transactions). Locate occurrences by the symbols CACHE_KEY,
cacheKey, walletAddress, localStorage.getItem/setItem/removeItem and make them
consistent across useEffect and the transaction fetch/persist/clear logic.
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)
dashboard/hooks/use-transactions.ts (1)
52-75:⚠️ Potential issue | 🟡 MinorPrevent stale async fetches from overwriting a new wallet's state.
If the wallet switches whilefetchTransactionsis in flight, the resolved data can overwrite the freshly reset state for the new wallet. TheuseEffecton line 22 resets state whenwalletAddresschanges, but any pendingfetchTransactionscall (initiated before the switch) will execute its state updates afterward, causing cross-wallet data contamination in the cache (line 72-73). Consider tracking the active wallet/request and ignoring stale results.🛠️ Suggested guard against stale results
-import { useState, useEffect } from 'react'; +import { useState, useEffect, useRef } from 'react'; export function useTransactions() { const { walletAddress } = useWallet(); + const latestWalletRef = useRef<string | null>(walletAddress); const [transactions, setTransactions] = useState<TransactionEvent[]>([]); @@ useEffect(() => { + latestWalletRef.current = walletAddress; // Reset state for new wallet context setTransactions([]); @@ - const fetchTransactions = async () => { + const fetchTransactions = async () => { + const requestWallet = latestWalletRef.current; try { setLoading(true); setError(null); // Filter for specific merchant address: const merchantAddress = ''; const events = await transactionService.fetchStableCoinPurchases(merchantAddress); + if (latestWalletRef.current !== requestWallet) return; setTransactions(events); setHasFetched(true); @@ - if (walletAddress) { - const cacheKey = `${CACHE_KEY}_${walletAddress}`; + if (requestWallet) { + const cacheKey = `${CACHE_KEY}_${requestWallet}`; localStorage.setItem(cacheKey, JSON.stringify(cacheData)); }
There was a problem hiding this comment.
Pull request overview
Updates the useTransactions hook to react to wallet changes and isolate cached transaction state per connected wallet, addressing issue #40 (transactions not refreshing when switching wallets).
Changes:
- Adds
walletAddress-drivenuseEffectto reset local state and load a per-wallet localStorage cache on wallet changes. - Introduces per-wallet cache keys (
stablepay_transactions_<walletAddress>) and guards against stale async updates via a ref.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
dashboard/hooks/use-transactions.ts (3)
54-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard stale
catchandfinallypaths too.The wallet check on Line 62 only protects the success path. If wallet A's request rejects after the user switches to wallet B, Lines 81-84 will still surface A's error and clear B's loading state. Use the same staleness gate for
catch/finally, or switch to a request id so only the newest invocation can commit any state.Proposed fix
} catch (err) { + if (latestWalletRef.current !== requestWallet) return; console.error('Error fetching transactions:', err); setError(err instanceof Error ? err.message : 'Failed to fetch transactions'); } finally { - setLoading(false); + if (latestWalletRef.current === requestWallet) { + setLoading(false); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboard/hooks/use-transactions.ts` around lines 54 - 84, The fetchTransactions function currently only checks staleness on the success path using requestWallet vs latestWalletRef.current; extend that same staleness guard to the catch and finally blocks (or implement a monotonically increasing requestId checked at start/end) so that if latestWalletRef.current !== requestWallet the catch should not call setError and the finally should not call setLoading or other state setters; update references to setError, setLoading, setTransactions, and setHasFetched so they only run when the invocation is still fresh (use the same requestWallet or requestId guard).
72-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep cache persistence out of the fetch failure path.
localStorage.setItemcan throw quota or storage-access errors. Right now that happens inside the maintry, so a cache-write failure on Lines 76-78 falls into the fetchcatchand reports a failed fetch even aftersetTransactions(events)already succeeded. Cache writes should be best-effort.Proposed fix
const cacheData: CachedData = { transactions: serializableEvents as any, timestamp: Date.now() }; if (requestWallet) { - const cacheKey = `${CACHE_KEY}_${requestWallet}`; - localStorage.setItem(cacheKey, JSON.stringify(cacheData)); + try { + const cacheKey = `${CACHE_KEY}_${requestWallet}`; + localStorage.setItem(cacheKey, JSON.stringify(cacheData)); + } catch (storageError) { + console.warn('Failed to cache transactions:', storageError); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboard/hooks/use-transactions.ts` around lines 72 - 79, The cache write to localStorage (creating cacheKey with CACHE_KEY and requestWallet and calling localStorage.setItem with cacheData) runs inside the main fetch try block and can throw, causing successful setTransactions(events) to be treated as a fetch failure; make cache persistence best-effort by moving the JSON.stringify/localStorage.setItem call out of the fetch try/catch or wrapping only the setItem call in its own try/catch that swallows/storage-quotas errors (log them optionally) so setTransactions(events) success does not trigger the fetch failure path.
39-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat a fresh empty cache as a real cache hit.
Line 39 only restores cached state when
cachedTransactions.length > 0. That means a wallet with zero transactions will refetch every time you switch back to it withinCACHE_EXPIRY, which defeats the per-wallet cache for that case. SethasFetchedfor any non-expired cache entry, even when the cached array is empty.Proposed fix
- if (!isExpired && cachedTransactions.length > 0) { + if (!isExpired) { // Convert string blockNumber back to BigInt const restoredTransactions = cachedTransactions.map(event => ({ ...event, blockNumber: BigInt(event.blockNumber) })); setTransactions(restoredTransactions); setHasFetched(true); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboard/hooks/use-transactions.ts` around lines 39 - 47, The code in use-transactions.ts only treats non-empty cachedTransactions as a cache hit; remove the length check so any non-expired cache entry (even an empty array) is considered a hit. In the block guarded by !isExpired, map cachedTransactions (which may be empty) to restore blockNumber to BigInt as you already do (restoredTransactions) and call setTransactions(restoredTransactions) and setHasFetched(true) for all non-expired cached entries; keep the existing BigInt conversion logic and references to cachedTransactions, restoredTransactions, setTransactions, setHasFetched, and isExpired.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@dashboard/hooks/use-transactions.ts`:
- Around line 54-84: The fetchTransactions function currently only checks
staleness on the success path using requestWallet vs latestWalletRef.current;
extend that same staleness guard to the catch and finally blocks (or implement a
monotonically increasing requestId checked at start/end) so that if
latestWalletRef.current !== requestWallet the catch should not call setError and
the finally should not call setLoading or other state setters; update references
to setError, setLoading, setTransactions, and setHasFetched so they only run
when the invocation is still fresh (use the same requestWallet or requestId
guard).
- Around line 72-79: The cache write to localStorage (creating cacheKey with
CACHE_KEY and requestWallet and calling localStorage.setItem with cacheData)
runs inside the main fetch try block and can throw, causing successful
setTransactions(events) to be treated as a fetch failure; make cache persistence
best-effort by moving the JSON.stringify/localStorage.setItem call out of the
fetch try/catch or wrapping only the setItem call in its own try/catch that
swallows/storage-quotas errors (log them optionally) so setTransactions(events)
success does not trigger the fetch failure path.
- Around line 39-47: The code in use-transactions.ts only treats non-empty
cachedTransactions as a cache hit; remove the length check so any non-expired
cache entry (even an empty array) is considered a hit. In the block guarded by
!isExpired, map cachedTransactions (which may be empty) to restore blockNumber
to BigInt as you already do (restoredTransactions) and call
setTransactions(restoredTransactions) and setHasFetched(true) for all
non-expired cached entries; keep the existing BigInt conversion logic and
references to cachedTransactions, restoredTransactions, setTransactions,
setHasFetched, and isExpired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7831fe0-8b32-4f84-8c9c-f1bb2e2019b4
📒 Files selected for processing (1)
dashboard/hooks/use-transactions.ts
DeveloperAmrit
left a comment
DeveloperAmrit
left a comment
There was a problem hiding this comment.
Thanks for the changes, approved from my side.
Changes made:
Added
walletAddressto the dependency array of theuseEffecthook to ensure it re-runs when the wallet address changes.@DengreSarthak Please check it out , Closes #40
Summary by CodeRabbit
New Features
Bug Fixes / Reliability