Conversation
… TransactionDetails
WalkthroughReplaces global balance usage with per-recipient-network balance fetching and state in TransferForm; TransactionDetails now uses each transaction's own network for explorer links and receipt data. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TransferForm
participant Utils as fetchBalanceForNetwork
participant PublicClient
participant RPC
rect rgb(235,244,255)
Note over TransferForm,Utils: Per-recipient-network balance flow
User->>TransferForm: open transfer / select recipient
TransferForm->>TransferForm: derive transferNetwork from recipientNetwork
TransferForm->>Utils: fetchBalanceForNetwork(transferNetwork, walletAddress)
Utils->>PublicClient: create publicClient (BSC special-case)
PublicClient->>RPC: query balances for walletAddress
RPC-->>PublicClient: raw balance data
PublicClient-->>Utils: formatted balances
Utils-->>TransferForm: transferNetworkBalance / error
TransferForm->>TransferForm: update UI (balance, Max, transfer call)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/TransferForm.tsx (1)
246-273: Clarify balance display vs validation when balance data is missing or still loadingBoth the dedicated balance bar and the amount header use:
isBalanceLoading || transferNetworkBalance === nullto decide when to show a skeleton, andtokenBalance = Number(transferNetworkBalance?.balances?.[token]) || 0as the source of truth for display and as themaxvalidation rule.This leads to a couple of confusing cases:
- When the balance fetch hasn’t run yet or has failed,
transferNetworkBalanceisnull, so the UI shows a skeleton, but the effectivemaxis0. Any positive amount will fail validation with a “Max. amount is 0” style error, without the user ever seeing an explicit “0” or an error message.- The “Max” buttons call
handleBalanceMaxClick, which usestokenBalance. If the balance is still loading or missing, that will write0into the amount, which feels odd given the UI is still showing a skeleton.Consider:
- Using only
isBalanceLoadingfor the skeleton condition, and treating a non-loading, null/emptytransferNetworkBalanceas a real “0” balance (or an explicit “Balance unavailable” state), and/or- Guarding the “Max” button so it’s disabled or hidden while
isBalanceLoadingis true.This will make the UX clearer when balances are not yet available, without affecting the core per-network behavior.
Also applies to: 470-482, 495-517
🧹 Nitpick comments (2)
app/utils.ts (1)
529-555: Network-scoped balance helper looks good; consider tightening typing & small ergonomicsThis helper correctly encapsulates per-network balance fetching and special-cases BSC RPC, which aligns with the PR goal.
Two small improvements you may want to make:
- Type safety: instead of
network: { chain: any }, use the existingNetworktype from../../types(or an explicit interface) sochainis guaranteed and consumers stay consistent.- Transport construction: calling
http(undefined)is harmless but a bit awkward. You could branch to avoid passingundefinedexplicitly:export async function fetchBalanceForNetwork( - network: { chain: any }, + network: Network, walletAddress: string, ): Promise<{ total: number; balances: Record<string, number> }> { const { createPublicClient, http } = await import("viem"); const { bsc } = await import("viem/chains"); - const publicClient = createPublicClient({ - chain: network.chain, - transport: http( - network.chain.id === bsc.id - ? "https://bsc-dataseed.bnbchain.org/" - : undefined, - ), - }); + const isBsc = network.chain.id === bsc.id; + const publicClient = createPublicClient({ + chain: network.chain, + transport: isBsc + ? http("https://bsc-dataseed.bnbchain.org/") + : http(), + });These are non-blocking and mainly about maintainability/readability.
app/components/TransferForm.tsx (1)
12-13: Per-recipienttransferNetworkwiring and token/balance sourcing look consistentThe refactor to:
- derive
transferNetworkfrom the chosenrecipientNetwork(withselectedNetworkas a sensible fallback),- pull
fetchedTokensfromallTokens[transferNetwork.chain.name], and- pass
selectedNetwork: transferNetworkplussupportedTokens: fetchedTokensintouseSmartWalletTransfer,ensures both the token list and the actual transfer call are aligned with the recipient’s network rather than the global selection. Computing
tokenBalancefromtransferNetworkBalance.balances[token]is also consistent with howfetchWalletBalancestructures its result.Only very minor polish you might consider later:
- Consolidate the two
../utilsimports into a single import block to avoid duplicating module imports.- If
Networkis a shared type, consider reusing it fortransferNetworkand throughout to keep everything strongly typed.These are non-blocking and the core behavior change looks correct.
Also applies to: 47-48, 56-62, 80-89, 104-118
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/TransferForm.tsx(9 hunks)app/components/transaction/TransactionDetails.tsx(2 hunks)app/utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jeremy0x
Repo: paycrest/noblocks PR: 177
File: app/utils.ts:179-196
Timestamp: 2025-07-23T07:30:23.720Z
Learning: The `normalizeNetworkName` function in app/utils.ts has been updated to be dynamic and scalable, converting any network identifier to sentence case with hyphens replaced by spaces and proper handling of acronyms like "BNB".
📚 Learning: 2025-07-23T07:30:23.720Z
Learnt from: jeremy0x
Repo: paycrest/noblocks PR: 177
File: app/utils.ts:179-196
Timestamp: 2025-07-23T07:30:23.720Z
Learning: The `normalizeNetworkName` function in app/utils.ts has been updated to be dynamic and scalable, converting any network identifier to sentence case with hyphens replaced by spaces and proper handling of acronyms like "BNB".
Applied to files:
app/utils.tsapp/components/TransferForm.tsxapp/components/transaction/TransactionDetails.tsx
📚 Learning: 2025-10-10T16:44:32.125Z
Learnt from: Dprof-in-tech
Repo: paycrest/noblocks PR: 244
File: app/components/CopyAddressWarningModal.tsx:48-52
Timestamp: 2025-10-10T16:44:32.125Z
Learning: In the CopyAddressWarningModal component (app/components/CopyAddressWarningModal.tsx), selectedNetwork from useNetwork() is always defined and does not require null safety checks when accessing its properties like selectedNetwork.chain.name.
Applied to files:
app/utils.tsapp/components/TransferForm.tsxapp/components/transaction/TransactionDetails.tsx
📚 Learning: 2025-11-06T07:37:39.036Z
Learnt from: Dprof-in-tech
Repo: paycrest/noblocks PR: 231
File: app/components/recipient/RecipientDetailsForm.tsx:539-552
Timestamp: 2025-11-06T07:37:39.036Z
Learning: In RecipientDetailsForm (app/components/recipient/RecipientDetailsForm.tsx), when isRecipientNameEditable is true (verification failed/returned "Ok"), the recipient safety alert should display when: isRecipientNameEditable && recipientName && !errors.recipientName && !recipientNameError. The !isFetchingRecipientName check is redundant because recipientName is cleared at fetch start and only populated after fetching completes or when the user manually enters it.
Applied to files:
app/components/TransferForm.tsx
🧬 Code graph analysis (1)
app/components/transaction/TransactionDetails.tsx (1)
app/utils.ts (1)
getExplorerLink(133-156)
🔇 Additional comments (1)
app/components/transaction/TransactionDetails.tsx (1)
53-57: Usingtransaction.networkfor explorer and receipts is correct; verify network name normalizationSwitching both the explorer URL and the PDF receipt payload to use
transaction.networkinstead of the globalselectedNetworkfixes the original “wrong network” bug and aligns all views with the actual transaction context.One thing to double‑check: this now relies on
transaction.networkmatching the display names expected bygetExplorerLinkandgetNetworkFromName(e.g.,"Base","Arbitrum One","BNB Smart Chain"). If the backend sometimes stores identifiers like"base"or"arbitrum-one", consider normalizing once at ingestion (e.g., vianormalizeNetworkName) so icons and explorer links don’t silently disappear for those records.Also applies to: 62-67
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/TransferForm.tsx (1)
247-255: Fix timing of balance refresh after successful transfer.The global balance is refreshed via
refreshBalance(), which updatessmartWalletBalancefrom the BalanceContext. SincetransferNetworkBalanceis local state and the form closes immediately after transfer (viaonClose()), it does not need explicit refresh—the balance will be re-fetched when the form reopens.However, there's a timing issue at line 250-251:
refreshBalance()is called afterhandleFormClose(), which means it executes on an already-closed/unmounting component. Either:
- Swap the order: call
refreshBalance()beforehandleFormClose(), or- Leverage the existing
onSuccesscallback to handle balance refresh in the parent, avoiding redundant calls.
🧹 Nitpick comments (1)
app/components/TransferForm.tsx (1)
180-180: Optional: Optimize effect dependency to reduce unnecessary re-fetches.The effect depends on
user?.linkedAccounts, which will trigger a re-fetch whenever any linked account changes (e.g., adding/removing an email, wallet, etc.), not just when the smart wallet address changes.Consider extracting the smart wallet address and depending on that specific value to avoid unnecessary balance fetches:
+ // Extract smart wallet address for more granular dependency tracking + const smartWalletAddress = user?.linkedAccounts.find( + (account) => account.type === "smart_wallet" + )?.address; + // Fetch balance for the selected transfer network useEffect(() => { const fetchBalance = async () => { const smartWalletAccount = user?.linkedAccounts.find( (account) => account.type === "smart_wallet", ); // No smart wallet account - set empty balance state (not loading, not error) if (!smartWalletAccount?.address) { ... } ... }; fetchBalance(); - }, [transferNetwork, user?.linkedAccounts]); + }, [transferNetwork, smartWalletAddress]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/TransferForm.tsx(9 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-10T16:44:32.125Z
Learnt from: Dprof-in-tech
Repo: paycrest/noblocks PR: 244
File: app/components/CopyAddressWarningModal.tsx:48-52
Timestamp: 2025-10-10T16:44:32.125Z
Learning: In the CopyAddressWarningModal component (app/components/CopyAddressWarningModal.tsx), selectedNetwork from useNetwork() is always defined and does not require null safety checks when accessing its properties like selectedNetwork.chain.name.
Applied to files:
app/components/TransferForm.tsx
📚 Learning: 2025-07-23T07:30:23.720Z
Learnt from: jeremy0x
Repo: paycrest/noblocks PR: 177
File: app/utils.ts:179-196
Timestamp: 2025-07-23T07:30:23.720Z
Learning: The `normalizeNetworkName` function in app/utils.ts has been updated to be dynamic and scalable, converting any network identifier to sentence case with hyphens replaced by spaces and proper handling of acronyms like "BNB".
Applied to files:
app/components/TransferForm.tsx
📚 Learning: 2025-11-06T07:37:39.036Z
Learnt from: Dprof-in-tech
Repo: paycrest/noblocks PR: 231
File: app/components/recipient/RecipientDetailsForm.tsx:539-552
Timestamp: 2025-11-06T07:37:39.036Z
Learning: In RecipientDetailsForm (app/components/recipient/RecipientDetailsForm.tsx), when isRecipientNameEditable is true (verification failed/returned "Ok"), the recipient safety alert should display when: isRecipientNameEditable && recipientName && !errors.recipientName && !recipientNameError. The !isFetchingRecipientName check is redundant because recipientName is cleared at fetch start and only populated after fetching completes or when the user manually enters it.
Applied to files:
app/components/TransferForm.tsx
🧬 Code graph analysis (1)
app/components/TransferForm.tsx (3)
app/mocks.ts (1)
networks(43-83)app/types.ts (1)
Token(209-215)app/utils.ts (1)
fetchBalanceForNetwork(537-554)
🔇 Additional comments (5)
app/components/TransferForm.tsx (5)
82-82: LGTM: transferNetwork derivation is correct.The derivation correctly finds the network object matching the selected
recipientNetworkand falls back toselectedNetworkwhen not found (e.g., during initial render before line 123's effect runs). Sincenetworksis a module-level constant fromapp/mocks.ts, thefind()returns stable object references.
142-180: LGTM: Balance fetch effect addresses past review concerns effectively.The effect now properly handles all three states:
- No smart wallet (lines 149-155): Sets zero balance and stops loading immediately
- Success (lines 162-167): Stores fetched balance
- Error (lines 168-173): Sets zero balance, stores error message, and shows toast
The
finallyblock (lines 174-176) ensures loading state is always cleared. This resolves the issues raised in the previous review about perpetual skeleton states and missing error feedback.
84-89: LGTM: Token fetching and balance calculation use the correct network.Lines 84 and 89 correctly use
transferNetwork(derived from the recipient's network) instead of the globalselectedNetwork. The optional chaining and zero-default fortokenBalancesafely handle cases where the token is not present in the fetched balance.
105-119: LGTM: Transfer hook correctly uses recipient's network.Line 115 correctly passes
transferNetwork(the recipient's network) as theselectedNetworkparameter touseSmartWalletTransfer. This is the core change that ensures transfers execute on the recipient's chosen network rather than the global context, directly addressing the PR objective.
12-12: No duplication found—review comment is incorrect.Verification confirms only one definition of
fetchBalanceForNetworkexists at line 537 in app/utils.ts. The AI-generated summary's claim of duplication is unfounded. No action needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/components/TransferForm.tsx (1)
142-180: Effect handles balance fetching well; two minor optimizations suggested.The balance fetch effect correctly:
- Handles the no-smart-wallet case by setting an empty balance state (lines 149-155)
- Sets loading and error states appropriately
- Shows both toast and inline error on failure
- Uses the recipient's network for fetching
Two optional refinements:
- Line 157 guard could set explicit states: The early return when
!transferNetworkdoesn't setisBalanceLoadingor clear states. While line 82's fallback ensurestransferNetworkis always defined, adding explicit state updates here would make the code more defensive:if (!transferNetwork) { + setTransferNetworkBalance({ total: 0, balances: {} }); + setIsBalanceLoading(false); + setBalanceError(null); return; }
- Line 180 dependency optimization: Depending on
user?.linkedAccounts(an array) may cause unnecessary re-fetches if the array reference changes frequently. Consider extracting the smart wallet address:+ const smartWalletAddress = user?.linkedAccounts.find( + (account) => account.type === "smart_wallet" + )?.address; // ... inside effect: use smartWalletAddress directly ... - }, [transferNetwork, user?.linkedAccounts]); + }, [transferNetwork, smartWalletAddress]);Neither issue is critical, but both would improve robustness and efficiency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/TransferForm.tsx(11 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-10T16:44:32.125Z
Learnt from: Dprof-in-tech
Repo: paycrest/noblocks PR: 244
File: app/components/CopyAddressWarningModal.tsx:48-52
Timestamp: 2025-10-10T16:44:32.125Z
Learning: In the CopyAddressWarningModal component (app/components/CopyAddressWarningModal.tsx), selectedNetwork from useNetwork() is always defined and does not require null safety checks when accessing its properties like selectedNetwork.chain.name.
Applied to files:
app/components/TransferForm.tsx
📚 Learning: 2025-07-23T07:30:23.720Z
Learnt from: jeremy0x
Repo: paycrest/noblocks PR: 177
File: app/utils.ts:179-196
Timestamp: 2025-07-23T07:30:23.720Z
Learning: The `normalizeNetworkName` function in app/utils.ts has been updated to be dynamic and scalable, converting any network identifier to sentence case with hyphens replaced by spaces and proper handling of acronyms like "BNB".
Applied to files:
app/components/TransferForm.tsx
📚 Learning: 2025-11-06T07:37:39.036Z
Learnt from: Dprof-in-tech
Repo: paycrest/noblocks PR: 231
File: app/components/recipient/RecipientDetailsForm.tsx:539-552
Timestamp: 2025-11-06T07:37:39.036Z
Learning: In RecipientDetailsForm (app/components/recipient/RecipientDetailsForm.tsx), when isRecipientNameEditable is true (verification failed/returned "Ok"), the recipient safety alert should display when: isRecipientNameEditable && recipientName && !errors.recipientName && !recipientNameError. The !isFetchingRecipientName check is redundant because recipientName is cleared at fetch start and only populated after fetching completes or when the user manually enters it.
Applied to files:
app/components/TransferForm.tsx
📚 Learning: 2025-11-06T07:37:39.036Z
Learnt from: Dprof-in-tech
Repo: paycrest/noblocks PR: 231
File: app/components/recipient/RecipientDetailsForm.tsx:539-552
Timestamp: 2025-11-06T07:37:39.036Z
Learning: In RecipientDetailsForm (app/components/recipient/RecipientDetailsForm.tsx), when isRecipientNameEditable is false (verification succeeded), the recipient safety alert should display when: !isRecipientNameEditable && recipientName && !recipientNameError. The !errors.recipientName check is unnecessary because in non-editable mode the recipient name is displayed as read-only text (not an input field), so form validation errors cannot occur.
Applied to files:
app/components/TransferForm.tsx
🧬 Code graph analysis (1)
app/components/TransferForm.tsx (5)
app/context/index.ts (1)
useBalance(3-3)app/mocks.ts (1)
networks(43-83)app/types.ts (1)
Token(209-215)app/utils.ts (1)
fetchBalanceForNetwork(537-554)app/components/index.ts (2)
AnimatedComponent(15-15)slideInOut(19-19)
🔇 Additional comments (7)
app/components/TransferForm.tsx (7)
12-12: LGTM! State and imports correctly support network-specific balance fetching.The new state variables (
transferNetworkBalance,isBalanceLoading,balanceError) and imports (fetchBalanceForNetwork,refreshBalance) are well-structured and properly typed to support the per-recipient-network balance feature.Also applies to: 47-47, 56-62
81-82: LGTM! Fallback toselectedNetworkensures robustness.The lookup-with-fallback pattern ensures
transferNetworkis always defined. The fallback is safe because the recipient network dropdown (lines 93-103) uses the same filterednetworksarray, preventing selection of invalid networks.
84-89: LGTM! Token list and balance correctly scoped to recipient network.The code properly:
- Retrieves tokens for the recipient's network (
transferNetwork.chain.name)- Extracts the selected token's balance from the network-specific balance object
- Handles null/undefined cases with optional chaining and a safe default of 0
115-115: LGTM! Critical change enables cross-network transfers.Passing
transferNetwork(the recipient's network) instead of the globalselectedNetworkcorrectly implements the cross-network transfer feature described in the PR objectives.
260-287: LGTM! Balance UI correctly uses network-specific states.Both balance display sections (lines 260-287 and 475-506) consistently:
- Show skeletons during loading (
isBalanceLoading || transferNetworkBalance === null)- Display the correct network-specific token balance
- Enable/disable the "Max" button appropriately
- Use the same
tokenBalancevalue derived fromtransferNetworkBalanceAlso applies to: 475-506
562-569: LGTM! Inline error display improves user feedback.The balance error is now displayed inline below the amount field (in addition to the toast notification), providing persistent feedback when balance fetching fails. This addresses the previous review comment and matches the styling of other form errors.
246-256: LGTM! Success flow correctly refreshes balance.Calling
refreshBalance()before closing ensures the global wallet balance is updated after the transfer completes, maintaining consistency between the form's network-specific balance and the main wallet view.
chibie
deleted the
fix-bug-fix-for-wrong-network-selection-in-transfer-form
branch
3 participants
Description
This pull request updates the transfer form and transaction details components to correctly handle balances and network selection based on the recipient's network, rather than relying on the globally selected network. The changes ensure that token balances and explorer links are accurate for the context of each transaction, and introduce a utility function to fetch balances for a specific network.
Transfer Form improvements:
transferNetworkBalanceand loading state to manage and display balances for the selected transfer network. [1] [2] [3] [4]Transaction Details improvements:
Utility function addition:
fetchBalanceForNetworktoapp/utils.tsto fetch wallet balances for a specific network, supporting the new balance fetching logic in the transfer form.Imports update:
fetchBalanceForNetworkutility inapp/components/TransferForm.tsxto enable network-specific balance fetching.References
This fixes a bug Francis discovered while trying to make transfers.
closes #292
Testing
There was no UI change. Conclusive testing would be done on staging.
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.