Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdjusts withdrawal navigation/state and bank details rendering; adds UK Faster Payments fields and a default account holder constant; updates transaction receipt UI to show UK sort code/account number and fallback account holder name. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
src/components/AddMoney/components/AddMoneyBankDetails.tsx (1)
260-266: Minor typo and consider extracting the fallback constant.The fix correctly addresses the missing account holder name for
faster_payments. Two minor observations:
- Typo in comment: "retrun" should be "return"
- The hardcoded fallback
'Bridge Building Sp. Z.o.o.'could be extracted to a constant in a shared config file for maintainability (e.g., if the company name changes).🔧 Suggested improvement
- {/* note: fallback to bridge as account holder name, to cover faster_payments onramp requests as bridge currently doesnt retrun a account holder name in api response */} + {/* note: fallback to bridge as account holder name, to cover faster_payments onramp requests as bridge currently doesn't return an account holder name in api response */} <PaymentInfoRow label={'Account Holder Name'} value={onrampData?.depositInstructions?.accountHolderName || 'Bridge Building Sp. Z.o.o.'}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/AddMoney/components/AddMoneyBankDetails.tsx` around lines 260 - 266, Fix the comment typo and extract the hardcoded fallback into a shared constant: correct "retrun" to "return" in the comment above PaymentInfoRow, and replace the inline string 'Bridge Building Sp. Z.o.o.' used in PaymentInfoRow value (onrampData?.depositInstructions?.accountHolderName || 'Bridge Building Sp. Z.o.o.') with a named constant (e.g., DEFAULT_ACCOUNT_HOLDER or BRIDGE_COMPANY_NAME) exported from a shared config/consts module so it can be maintained centrally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/AddMoney/components/AddMoneyBankDetails.tsx`:
- Around line 260-266: Fix the comment typo and extract the hardcoded fallback
into a shared constant: correct "retrun" to "return" in the comment above
PaymentInfoRow, and replace the inline string 'Bridge Building Sp. Z.o.o.' used
in PaymentInfoRow value (onrampData?.depositInstructions?.accountHolderName ||
'Bridge Building Sp. Z.o.o.') with a named constant (e.g.,
DEFAULT_ACCOUNT_HOLDER or BRIDGE_COMPANY_NAME) exported from a shared
config/consts module so it can be maintained centrally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee280512-55fe-4be2-8357-6bc2c78337ac
📒 Files selected for processing (3)
src/app/(mobile-ui)/withdraw/[country]/bank/page.tsxsrc/app/(mobile-ui)/withdraw/page.tsxsrc/components/AddMoney/components/AddMoneyBankDetails.tsx
💤 Files with no reviewable changes (1)
- src/app/(mobile-ui)/withdraw/[country]/bank/page.tsx
|
@coderabbitai review in depth. Evaluate:
Also, write a very concise testing guide for QA |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me gather the full file contents for a thorough review. 🏁 Script executed: Length of output: 59599
🔍 Risk of Introducing New Bugs / Breaking Existing Functionality
|
| Area | Observation |
|---|---|
BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME |
✅ Correctly uses a named constant instead of an inline string |
methodQueryParam construction |
handleAmountContinue — could be extracted to a variable once, but not introduced by this PR |
| Context destructuring | ✅ setSelectedBankAccount was missing from destructuring in withdraw/page.tsx — correctly added |
| Routing guard comment | ✅ Inline comment clearly explains the crypto-first ordering rationale |
allowCopy on Account Holder Name |
✅ Clean — always true since there's always a value |
🧪 QA Testing Guide
Withdraw Flow — Navigation Bug (TASK-18699)
Test A: Back navigation from bank confirm screen preserves state
- Go to
/withdraw→ select a bank account method → enter an amount → hit Continue - On the bank confirm screen (
/withdraw/[country]/bank), tap Back - ✅ Should land back on the amount screen with the amount still populated (not redirect to
/withdraw)
Test B: Back navigation from amount screen resets all state
- Go to
/withdraw→ select a bank method → reach the amount screen - Tap Back
- ✅ Should return to method selection screen with no amount, no method, no bank account pre-selected
Test C: Crypto method routing not hijacked by stale bank account
- Complete or partially complete a bank withdrawal (so
selectedBankAccountis set in context) - Start a new withdraw, select Crypto method → enter an amount → hit Continue
- ✅ Should route to
/withdraw/crypto, NOT to/withdraw/[country]/bank
Test D: Success state navigation
- Complete a bank withdrawal to the SUCCESS view
- Tap the X / back icon
- ✅ Should navigate to
/homecleanly without redirect loops
Deposit / GBP Onramp — Account Holder Name (TASK-18693)
Test E: Account Holder Name shown for GBP (faster_payments)
- Go to
/add-money/gb/bank(GBP onramp) - Enter an amount → reach the bank details screen
- ✅ "Account Holder Name" row should be visible with value "Bridge Building Sp. Z.o.o."
- ✅ Copy button should be functional on that row
Test F: Account Holder Name shown for other countries
- Repeat Test E for US, EUR (e.g. DE), MX
- ✅ Account Holder Name row should appear for all — for countries where the API returns a name, that name is used; otherwise fallback to "Bridge Building Sp. Z.o.o."
Test G: "Share Details" text for GBP (known pre-existing gap)
- On GBP bank details, tap Share Details
⚠️ Note: Account Holder Name will not appear in the shared text (pre-existing behavior, not a regression from this PR)
🧠 Learnings used
Learnt from: Hugo0
Repo: peanutprotocol/peanut-ui PR: 852
File: src/hooks/useTransactionHistory.ts:177-239
Timestamp: 2025-05-14T21:16:06.369Z
Learning: When reviewing code, pay attention to and resolve CodeRabbit comments as they can contain useful refactoring suggestions and insights.
Learnt from: CR
Repo: peanutprotocol/peanut-ui PR: 0
File: .cursorrules:0-0
Timestamp: 2026-02-26T21:54:25.278Z
Learning: Applies to docs/PR.md : Maintain a concise docs/PR.md during PR work with: 1) Summary of changes, 2) Risks (what might break), 3) QA guidelines (what to test)
Learnt from: Hugo0
Repo: peanutprotocol/peanut-ui PR: 594
File: src/components/Request/Pay/Views/Initial.view.tsx:188-213
Timestamp: 2024-12-31T15:08:15.640Z
Learning: Always resolve coderabbit comments when reviewing code, since they can contain useful suggestions.
Learnt from: kushagrasarathe
Repo: peanutprotocol/peanut-ui PR: 0
File: :0-0
Timestamp: 2026-03-04T18:32:51.654Z
Learning: In peanutprotocol/peanut-ui, Bridge-specific add-money and withdraw flows (`src/app/(mobile-ui)/add-money/[country]/bank/page.tsx`, `src/components/AddWithdraw/AddWithdrawCountriesList.tsx`, `src/components/Claim/Link/views/BankFlowManager.view.tsx`) have been migrated (PR `#1713`) to use the unified `isUserKycApproved` from `useKycStatus()` hook instead of per-component `liveKycStatus` + WebSocket state. The migration is complete; no more direct `bridgeKycStatus === 'approved'` checks in those files.
Learnt from: kushagrasarathe
Repo: peanutprotocol/peanut-ui PR: 1693
File: src/app/(mobile-ui)/add-money/[country]/bank/page.tsx:206-207
Timestamp: 2026-03-04T18:35:17.673Z
Learning: In Bridge-specific add-money and withdraw flows (e.g., src/app/(mobile-ui)/add-money/[country]/bank/page.tsx), liveKycStatus from WebSocket tracks real-time bridge KYC status changes, and bridgeKycStatus is the persisted status. These flows use direct bridgeKycStatus === 'approved' checks instead of the unified isUserKycVerified helper. Migrating to the unified verifier requires careful testing of WebSocket-based real-time status tracking and is deferred to follow-up work.
Learnt from: Hugo0
Repo: peanutprotocol/peanut-ui PR: 0
File: :0-0
Timestamp: 2026-01-02T13:14:32.635Z
Learning: In the peanut-ui codebase, the add-money flows (bank and Manteca) use nuqs library for URL-based state management (step, amount, currency via useQueryStates), while RequestFulfillmentFlowContext still uses enum-based context state. This creates intentional partial migration where URL state is used for user-facing flows that benefit from deep linking and bookmarking.
Learnt from: Hugo0
Repo: peanutprotocol/peanut-ui PR: 1654
File: src/components/Card/CardGeoScreen.tsx:41-45
Timestamp: 2026-02-02T06:03:48.192Z
Learning: In peanutprotocol/peanut-ui, KYC 2.0 uses the unified entry route '/profile/identity-verification'. Components (e.g., src/components/Card/CardGeoScreen.tsx) should avoid region-specific paths like '/profile/identity-verification/europe/bridge' and navigate to the unified route instead.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/AddMoney/components/AddMoneyBankDetails.tsx`:
- Around line 261-267: The UK faster-payments holder name is only handled in the
JSX PaymentInfoRow but not appended in generateBankDetails(), causing Share
Details to miss it; derive a single resolvedHolderName (e.g., from
onrampData?.depositInstructions?.accountHolderName ||
BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME using the existing
BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME constant) and reuse that variable in both the
PaymentInfoRow value and in the isUk branch of generateBankDetails() so the
shared text includes an "Account Holder Name" line; update references in the
component to use resolvedHolderName and ensure generateBankDetails() appends the
same resolvedHolderName string.
In `@src/components/TransactionDetails/TransactionDetailsReceipt.tsx`:
- Around line 931-940: The current rendering infers Faster Payments from
presence of sort_code/account_number and falls back to
BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME for any missing account_holder_name; instead,
change the logic in TransactionDetailsReceipt.tsx to branch on
extraDataForDrawer.depositInstructions.payment_rail === 'faster_payments' when
deciding labels and copy targets for UK fields (use PaymentInfoRow for "Account
Holder Name" but only use depositInstructions.account_holder_name fallback to
BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME inside that faster_payments branch); do not
infer rail from sort_code/account_number, and for missing UK fields inside the
faster_payments branch render "N/A" rather than letting the component fall
through to US account/routing rows; apply the same fix to the other similar
block referenced around the other PaymentInfoRow usage (the duplicated section
noted in the review).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb561233-c7da-4dd7-ae18-b954d014a274
📒 Files selected for processing (4)
src/components/AddMoney/components/AddMoneyBankDetails.tsxsrc/components/TransactionDetails/TransactionDetailsReceipt.tsxsrc/components/TransactionDetails/transactionTransformer.tssrc/constants/payment.consts.ts
| {/* note: fallback to bridge as account holder name, to cover faster_payments onramp requests as bridge currently doesnt retrun a account holder name in api response */} | ||
| <PaymentInfoRow | ||
| label={'Account Holder Name'} | ||
| value={onrampData?.depositInstructions?.accountHolderName || BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME} | ||
| allowCopy | ||
| hideBottomBorder | ||
| /> |
There was a problem hiding this comment.
Share Details still misses the holder name for UK transfers.
This fixes the visible card, but generateBankDetails() still never appends Account Holder Name in the isUk branch. Users copying from Share Details will still get incomplete Faster Payments instructions, and the fallback resolution is now split across two paths. Derive one resolved holder-name value and reuse it in both the PaymentInfoRow and the shared text.
Proposed direction
+ const resolvedAccountHolderName =
+ onrampData?.depositInstructions?.accountHolderName ||
+ (isUk && onrampData?.depositInstructions ? BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME : 'Loading...')
...
- <PaymentInfoRow
- label={'Account Holder Name'}
- value={onrampData?.depositInstructions?.accountHolderName || BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME}
- allowCopy
- hideBottomBorder
- />
+ <PaymentInfoRow
+ label={'Account Holder Name'}
+ value={resolvedAccountHolderName}
+ allowCopy={resolvedAccountHolderName !== 'Loading...'}
+ hideBottomBorder
+ /> // uk faster payments
if (isUk) {
bankDetails += `
+Account Holder Name: ${resolvedAccountHolderName}
Sort Code: ${onrampData?.depositInstructions?.sortCode || 'Loading...'}
Account Number: ${onrampData?.depositInstructions?.accountNumber || 'Loading...'}`
}As per coding guidelines, "Follow DRY (Don't Repeat Yourself) - reuse existing code and abstract shared functionality; use shared consts from src/constants"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/AddMoney/components/AddMoneyBankDetails.tsx` around lines 261
- 267, The UK faster-payments holder name is only handled in the JSX
PaymentInfoRow but not appended in generateBankDetails(), causing Share Details
to miss it; derive a single resolvedHolderName (e.g., from
onrampData?.depositInstructions?.accountHolderName ||
BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME using the existing
BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME constant) and reuse that variable in both the
PaymentInfoRow value and in the isUk branch of generateBankDetails() so the
shared text includes an "Account Holder Name" line; update references in the
component to use resolvedHolderName and ensure generateBankDetails() appends the
same resolvedHolderName string.
| {/* note: fallback to bridge as account holder name, to cover faster_payments onramp requests as bridge currently doesnt return an account holder name in api response */} | ||
| <PaymentInfoRow | ||
| label="Account Holder Name" | ||
| value={ | ||
| transaction.extraDataForDrawer.depositInstructions.account_holder_name || | ||
| BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME | ||
| } | ||
| allowCopy | ||
| hideBottomBorder={false} | ||
| /> |
There was a problem hiding this comment.
Select the receipt format from payment_rail, not from missing fields.
depositInstructions.payment_rail is already available, but this change infers Faster Payments from sort_code/account_number presence and applies the holder-name fallback to any missing account_holder_name. A partial Faster Payments payload would now fall through to the US account/routing rows and show the wrong labels/copy targets. Branch on payment_rail === 'faster_payments' instead, then render missing UK fields as N/A inside that branch.
Safer branching
+ const isFasterPayments =
+ transaction.extraDataForDrawer.depositInstructions.payment_rail ===
+ 'faster_payments'
...
- <PaymentInfoRow
- label="Account Holder Name"
- value={
- transaction.extraDataForDrawer.depositInstructions.account_holder_name ||
- BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME
- }
- allowCopy
- hideBottomBorder={false}
- />
+ <PaymentInfoRow
+ label="Account Holder Name"
+ value={
+ transaction.extraDataForDrawer.depositInstructions.account_holder_name ||
+ (isFasterPayments
+ ? BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME
+ : 'N/A')
+ }
+ allowCopy={
+ !!transaction.extraDataForDrawer.depositInstructions.account_holder_name ||
+ isFasterPayments
+ }
+ hideBottomBorder={false}
+ />
...
- ) : transaction.extraDataForDrawer.depositInstructions.sort_code &&
- transaction.extraDataForDrawer.depositInstructions.account_number ? (
+ ) : isFasterPayments ? (
<>
<PaymentInfoRow
label="Sort Code"
- value={transaction.extraDataForDrawer.depositInstructions.sort_code}
- allowCopy
+ value={
+ transaction.extraDataForDrawer.depositInstructions.sort_code ||
+ 'N/A'
+ }
+ allowCopy={
+ !!transaction.extraDataForDrawer.depositInstructions.sort_code
+ }
hideBottomBorder
/>
<PaymentInfoRow
label="Account Number"
value={
- transaction.extraDataForDrawer.depositInstructions.account_number
+ transaction.extraDataForDrawer.depositInstructions.account_number ||
+ 'N/A'
}
- allowCopy
+ allowCopy={
+ !!transaction.extraDataForDrawer.depositInstructions.account_number
+ }
hideBottomBorder
/>
</>Also applies to: 1017-1035
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/TransactionDetails/TransactionDetailsReceipt.tsx` around lines
931 - 940, The current rendering infers Faster Payments from presence of
sort_code/account_number and falls back to BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME
for any missing account_holder_name; instead, change the logic in
TransactionDetailsReceipt.tsx to branch on
extraDataForDrawer.depositInstructions.payment_rail === 'faster_payments' when
deciding labels and copy targets for UK fields (use PaymentInfoRow for "Account
Holder Name" but only use depositInstructions.account_holder_name fallback to
BRIDGE_DEFAULT_ACCOUNT_HOLDER_NAME inside that faster_payments branch); do not
infer rail from sort_code/account_number, and for missing UK fields inside the
faster_payments branch render "N/A" rather than letting the component fall
through to US account/routing rows; apply the same fix to the other similar
block referenced around the other PaymentInfoRow usage (the duplicated section
noted in the review).
2 participants
faster_payments