fix: should be clearer for US users how to enter their bank account #514
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to improve the clarity of error messages and validation logic related to US bank account numbers across several components. Specifically, the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![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
🧹 Outside diff range and nitpick comments (6)
src/components/Cashout/Components/RecipientInfo.comp.tsx (2)
24-25: Improve text formatting for better readability.The updated text successfully clarifies US bank account requirements, which directly addresses the user complaints. However, consider improving readability with visual separation.
Consider this formatting improvement:
- You can claim directly to your IBAN OR US bank account. For US bank accounts, enter your - bank routing number and account number together. + You can claim directly to: + • IBAN, or + • US bank account (enter 9-digit routing number followed by account number)
Line range hint
17-17: Consider addressing the TODO comment in a future PR.The comment indicates a need to standardize Tailwind classes across components. This could be addressed by:
- Creating reusable style components
- Extracting common classes into Tailwind components
- Using CSS modules or styled-components
Would you like me to create a GitHub issue to track this technical debt?
src/components/Cashout/Components/Initial.view.tsx (2)
327-327: Enhance error message clarity for US bank accounts.While the updated error message is an improvement, it could be more specific about the requirements.
Consider this more detailed message:
-'Invalid bank account. For US bank accounts, enter your bank routing number followed by your account number.', +'Invalid bank account. For US bank accounts, enter your 9-digit routing number immediately followed by your account number (no spaces or special characters).',
Line range hint
327-336: Consider separating US and IBAN account validation logic.The current implementation uses a single error message and validation flow for both US bank accounts and IBANs. This could lead to confusion as they have different formats and requirements.
Consider:
- Adding a country selector or account type toggle
- Implementing separate validation logic and error messages for each type
- Using specific input masks based on the selected type
Example architectural approach:
type AccountType = 'US' | 'IBAN'; interface ValidationResult { isValid: boolean; error?: string; } const validateUSAccount = (value: string): ValidationResult => { const routingRegex = /^\d{9}/; if (!routingRegex.test(value)) { return { isValid: false, error: 'US accounts must start with a 9-digit routing number.' }; } // Additional US-specific validation... return { isValid: true }; }; const validateIBAN = (value: string): ValidationResult => { // IBAN-specific validation... return { isValid: true }; };src/components/Global/LinkAccountComponent/index.tsx (2)
Line range hint
103-106: Enhance error messages for US bank accounts.Current error messages don't guide users on the correct format. Consider providing more specific validation feedback:
-setIbanFormError('accountNumber', { message: 'Invalid account number' }) +setIbanFormError('accountNumber', { + message: getAccountDetailsValue('type') === 'us' + ? 'Please enter your 9-digit routing number followed by your account number' + : 'Invalid account number' +})
Line range hint
249-257: Improve input field clarity for US users.The current input field doesn't clearly communicate the expected format. Consider these improvements:
<input {...registerIban('accountNumber', { required: 'This field is required', })} className={`custom-input ${ibanErrors.accountNumber ? 'border border-red' : ''}`} - placeholder={'Bank account'} + placeholder={'For US: 9-digit routing number + account number'} autoComplete="on" name="bankAccount" /> +<div className="text-sm text-gray-600 mt-1"> + {/* Add helper text */} + For US accounts: Enter your 9-digit routing number immediately followed by your account number +</div>Alternatively, consider splitting into separate fields for better user experience:
{getAccountDetailsValue('type') === 'us' && ( <div className="flex flex-col gap-2"> <input {...registerIban('routingNumber', { required: 'Routing number is required', pattern: { value: /^[0-9]{9}$/, message: 'Routing number must be 9 digits' } })} placeholder="9-digit routing number" /> <input {...registerIban('accountNumber', { required: 'Account number is required', pattern: { value: /^[0-9]{10,12}$/, message: 'Account number must be 10-12 digits' } })} placeholder="Account number" /> </div> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/components/Cashout/Components/Initial.view.tsx(1 hunks)src/components/Cashout/Components/RecipientInfo.comp.tsx(1 hunks)src/components/Global/GeneralRecipientInput/index.tsx(1 hunks)src/components/Global/LinkAccountComponent/index.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Cashout/Components/Initial.view.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#484
File: src/components/Cashout/Components/Initial.view.tsx:273-274
Timestamp: 2024-10-25T11:33:46.776Z
Learning: In the `InitialCashoutView` component (`src/components/Cashout/Components/Initial.view.tsx`), linked bank accounts should not generate error states, and the `ValidatedInput` component will clear any error messages if needed. Therefore, it's unnecessary to manually clear the error state when selecting or clearing linked bank accounts.
🔇 Additional comments (3)
src/components/Cashout/Components/RecipientInfo.comp.tsx (1)
33-34: LGTM! Improved link text for better accessibility.
The change from generic "here" to descriptive "Supported regions" follows accessibility best practices by clearly indicating the link's destination.
src/components/Global/GeneralRecipientInput/index.tsx (1)
37-42: Verify the impact on existing users
The regex change might affect existing users or stored bank account numbers.
Let's check for any existing bank account numbers that might be affected:
✅ Verification successful
Bank account validation change is safe to proceed
The validation is handled through a centralized API endpoint /api/peanut/iban/validate-bank-account-number, not through local regex patterns. The regex /^[0-9]{6,26}$/ is only used for initial format validation before making the API call, while the actual account validation is done server-side. This means:
- No stored account numbers are affected as the validation is done at input time
- The API endpoint remains the same, ensuring consistent validation rules
- The regex change only affects the client-side format check, providing a wider range (6-26 digits) which is more permissive than before
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for US bank account validation patterns across the codebase
echo "Searching for existing bank account validation patterns..."
rg -l "validateBankAccount|isValidBankAccount"
# Search for any stored account number patterns
echo "Searching for potential stored account numbers..."
rg -l "account.*number.*pattern|bankAccount.*pattern"
Length of output: 525
Script:
#!/bin/bash
# Let's examine the actual validation patterns and their usage
echo "Checking bank account validation in utils:"
rg -A 5 "validateBankAccount" src/utils/cashout.utils.ts
echo -e "\nChecking usage in components:"
rg -A 5 "validateBankAccount" src/components/Cashout/Components/Initial.view.tsx src/components/Global/LinkAccountComponent/index.tsx
echo -e "\nLooking for bank account number regex patterns:"
rg "^\s*const\s+.*pattern.*=.*\d.*account" || rg "[0-9]{6,}.*account|account.*[0-9]{6,}"
Length of output: 2850
src/components/Global/LinkAccountComponent/index.tsx (1)
102-102:
Enhance US bank account validation pattern.
The current regex /^[0-9]{6,26}$/ is too permissive and might accept invalid combinations. Consider implementing stricter validation for US accounts:
- Routing number should be exactly 9 digits
- Account number typically ranges from 10-12 digits
-} else if (/^[0-9]{6,26}$/.test(sanitizedAccountNumber)) {
+} else if (/^[0-9]{9}[0-9]{10,12}$/.test(sanitizedAccountNumber)) {Likely invalid or redundant comment.
| } else if (/^[0-9]{6,26}$/.test(recipient)) { | ||
| // routing number: 9 digits | ||
| // account number: 8-12 digits (can go up to 17) | ||
| type = 'us' | ||
| isValid = await utils.validateBankAccount(recipient) | ||
| if (!isValid) errorMessage.current = 'Invalid US account number' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding routing number validation
To further improve validation accuracy and user experience, consider adding specific routing number validation.
Add a helper function to validate the routing number format:
const isValidRoutingNumber = (routing: string): boolean => {
if (!/^\d{9}$/.test(routing)) return false;
// Implement checksum validation
// Each routing number contains a checksum as its last digit
const digits = routing.split('').map(Number);
const checksum = (
7 * (digits[0] + digits[3] + digits[6]) +
3 * (digits[1] + digits[4] + digits[7]) +
9 * (digits[2] + digits[5])
) % 10;
return checksum === digits[8];
};Then update the validation:
} else if (/^[0-9]{9}[0-9]{8,12}$/.test(recipient)) {
type = 'us'
+ const routingNumber = recipient.slice(0, 9);
+ if (!isValidRoutingNumber(routingNumber)) {
+ errorMessage.current = 'Invalid routing number. Please check and try again.';
+ return false;
+ }
isValid = await utils.validateBankAccount(recipient)Strengthen the US bank account validation logic
The current validation has several potential issues:
- The regex
/^[0-9]{6,26}$/is too permissive:- Allows numbers shorter than required (routing number is 9 digits, account number is 8-12 digits)
- Accepts numbers longer than typical US bank account formats
- No specific validation of routing number format
- No clear separation between routing and account numbers
Consider implementing this more robust validation:
-} else if (/^[0-9]{6,26}$/.test(recipient)) {
- // routing number: 9 digits
- // account number: 8-12 digits (can go up to 17)
+} else if (/^[0-9]{9}[0-9]{8,12}$/.test(recipient)) {
+ // Validate US bank account format:
+ // First 9 digits: routing number
+ // Following 8-12 digits: account number
type = 'us'
isValid = await utils.validateBankAccount(recipient)
- if (!isValid) errorMessage.current = 'Invalid US account number'
+ if (!isValid) errorMessage.current = 'Please enter your 9-digit routing number followed by your 8-12 digit account number'This change:
- Enforces the exact format required for US bank accounts
- Makes it clear that routing number must come first
- Provides a more helpful error message
📝 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.
| } else if (/^[0-9]{6,26}$/.test(recipient)) { | |
| // routing number: 9 digits | |
| // account number: 8-12 digits (can go up to 17) | |
| type = 'us' | |
| isValid = await utils.validateBankAccount(recipient) | |
| if (!isValid) errorMessage.current = 'Invalid US account number' | |
| } else if (/^[0-9]{9}[0-9]{8,12}$/.test(recipient)) { | |
| // Validate US bank account format: | |
| // First 9 digits: routing number | |
| // Following 8-12 digits: account number | |
| type = 'us' | |
| isValid = await utils.validateBankAccount(recipient) | |
| if (!isValid) errorMessage.current = 'Please enter your 9-digit routing number followed by your 8-12 digit account number' |
| // this code is also repeated in GeneralRecipientInput.tsx | ||
| } else if (/^[0-9]{6,26}$/.test(sanitizedAccountNumber)) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract shared validation logic into a common utility.
The comment indicates code duplication between components. Consider extracting the bank account validation logic into a shared utility function to improve maintainability.
// Create a new file: src/utils/bankValidation.ts
+export const validateBankAccountFormat = (accountNumber: string): { isValid: boolean; type: 'iban' | 'us' | null } => {
+ const sanitizedAccount = accountNumber.replaceAll(/\s/g, '').toLowerCase()
+ if (isIBAN(sanitizedAccount)) {
+ return { isValid: true, type: 'iban' }
+ }
+ if (/^[0-9]{6,26}$/.test(sanitizedAccount)) {
+ return { isValid: true, type: 'us' }
+ }
+ return { isValid: false, type: null }
+}
// In LinkAccountComponent and GeneralRecipientInput:
-// this code is also repeated in GeneralRecipientInput.tsx
-} else if (/^[0-9]{6,26}$/.test(sanitizedAccountNumber)) {
+const validation = validateBankAccountFormat(accountNumber)
+if (validation.isValid) {
+ setAccountDetailsValue('type', validation.type)Committable suggestion skipped: line range outside the PR's diff.
| showError: true, | ||
| errorMessage: | ||
| 'Invalid bank account. Please make sure your account is supported', | ||
| 'Invalid bank account. For US bank accounts, enter your bank routing number followed by your account number.', |
There was a problem hiding this comment.
thought: maybe we should add an example on the error and info messages. Something like:
Input: 1112223330001234567
where:
- Routing Number (9 digits): example
111222333 - Account Number (8-17 digits): example
0001234567
wdyt?
| } else if (/^[0-9]{6,17}$/.test(recipient)) { | ||
| } else if (/^[0-9]{6,26}$/.test(recipient)) { | ||
| // routing number: 9 digits | ||
| // account number: 8-12 digits (can go up to 17) |
There was a problem hiding this comment.
thought: These checks may prove not to be enough down the line (as more geographies are added, maybe there is overlap in format detection). For now, I'm good with this.
US bank accounts have 9 digit routing number and then account number. This isn;t clear for users currently and we had 5 complaints