fix: temp commit, not fully done. Various cashout fixes and general improvements #545
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe pull request introduces multiple enhancements across various components related to bank account handling. Key additions include utility functions for sanitizing and formatting bank account information, improved validation logic for recipient inputs, and modifications to user interface components to enhance clarity and usability. Notable changes involve 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (13)
src/utils/format.utils.ts (1)
1-14: Consider implementing a comprehensive bank account handling strategyGiven that this code handles sensitive financial data, consider these architectural improvements:
- Create proper TypeScript interfaces for different bank account types
- Implement a centralized validation module
- Add proper error handling and logging (without exposing sensitive data)
- Consider using a dedicated financial data handling library
Example structure:
interface BankAccount { type: 'US' | 'IBAN'; value: string; routingNumber?: string; // for US accounts } interface ValidationResult { isValid: boolean; errors: string[]; }src/interfaces/index.ts (1)
1-38: Reconsider merging core type definitions in a temporary commitThe PR title suggests this is a temporary commit, but this file contains critical type definitions that will be used throughout the application. Consider:
- Splitting this into a separate, complete PR
- Removing the "temp commit" status
- Adding comprehensive documentation
- Getting team consensus on the type structure
These interfaces form a crucial part of the type system and should be well-thought-out before merging.
src/components/Global/GeneralRecipientInput/index.tsx (2)
107-114: Consider memoizing the format functionSince
formatDisplayValueis used during rendering and depends only on the recipient type, consider memoizing it withuseCallbackto prevent unnecessary re-renders.- const formatDisplayValue = (value: string) => { + const formatDisplayValue = useCallback((value: string) => { if (recipientType.current === 'iban') { return formatIBANDisplay(value) } else if (recipientType.current === 'us') { return formatUSAccountDisplay(value) } return value - } + }, [])
Line range hint
1-132: Address temporary status of changesThe PR title indicates this is a "temp commit, not fully done". Consider:
- Completing the US bank account validation
- Adding comprehensive test coverage
- Documenting the expected format for bank accounts
- Adding proper error messages for all validation cases
Please update the PR title and description once the implementation is complete.
Would you like help with any of these improvements?
src/components/Global/ValidatedInput/index.tsx (2)
107-107: Consider optimizing value formatting.While the formatting logic is correct, it could benefit from memoization to prevent unnecessary re-renders.
Consider using useMemo:
+const displayValue = useMemo( + () => (formatDisplayValue ? formatDisplayValue(value) : value), + [formatDisplayValue, value] +) <input type="text" - value={formatDisplayValue ? formatDisplayValue(value) : value} + value={displayValue} // ... other props />
Line range hint
1-158: Consider splitting responsibilities for better maintainability.The ValidatedInput component is handling multiple concerns:
- Input validation
- Value sanitization
- Value formatting
- UI rendering
Consider splitting this into smaller, more focused components following the Single Responsibility Principle.
Suggested architecture:
- Create a separate BankAccountInput component that uses ValidatedInput internally
- Move bank-specific logic (sanitization, formatting) to the new component
- Keep ValidatedInput as a generic validated input component
src/components/Cashout/Components/Initial.view.tsx (2)
231-236: Consider enhancing bank account formatting based on account typeWhile the current formatting improves readability, consider differentiating the formatting based on account type (US vs IBAN) for better user experience.
const formatBankAccountDisplay = (value: string) => { + // For US accounts (routing number + account number) + if (value.length === 17) { // 9 digits routing + 8 digits account + const routing = value.slice(0, 9) + const account = value.slice(9) + return `${routing.replace(/(.{3})/g, '$1 ').trim()} | ${account.replace(/(.{4})/g, '$1 ').trim()}` + } + // For IBAN return value .toUpperCase() .replace(/(.{4})/g, '$1 ') .trim()
368-376: Enhance error message clarity for bank account validationWhile the error message is improved, it could be more specific about the expected format for US bank accounts.
- 'Invalid bank account. For US bank accounts, enter your routing number and account number separately.', + 'Invalid bank account. For US bank accounts, enter your 9-digit routing number followed by your account number (typically 8-17 digits). For IBAN, enter your full IBAN number.',Also applies to: 387-387
src/components/Claim/Link/Initial.view.tsx (1)
461-465: Consider enhancing recipient type handlingThe current implementation sets the recipient type to 'address' when the address is empty, which might not handle all edge cases effectively.
Consider this more robust implementation:
-if (!update.recipient.address) { - setRecipientType('address') -} else { - setRecipientType(update.type) -} +const newType = !update.recipient.address ? 'address' : + update.type || determineRecipientType(update.recipient.address); +setRecipientType(newType);This would:
- Maintain the current logic
- Add a fallback to determine type from address format
- Make the code more maintainable
src/components/Offramp/Confirm.view.tsx (1)
Line range hint
1-24: Consider splitting into smaller, focused componentsThe
OfframpConfirmViewcomponent has grown quite large and handles multiple responsibilities. This makes it harder to maintain and test.Consider breaking it down into smaller, focused components:
KYCFlow- Handle KYC verification stepsBankAccountConfirmation- Display and handle bank account detailsTransactionConfirmation- Handle transaction submission and error statesThis would:
- Improve code organization and maintainability
- Make testing easier
- Allow for better reuse of common functionality
src/components/Global/LinkAccountComponent/index.tsx (3)
163-163: Remove unnecessary console.log statementThe
console.logstatement may have been used for debugging purposes and should be removed to clean up the code.Apply this diff to remove the
console.log:- console.log('Submitting form data:', formData)
167-167: Remove unnecessary console.log statementThe
console.logat this line is likely a leftover from debugging. It's best to remove it to maintain clean production code.Apply this diff to remove the
console.log:- console.log('Missing routing number')
202-202: Remove unnecessary console.log statementThe
console.logstatement here should be removed to avoid cluttering the console in production.Apply this diff to remove the
console.log:- console.log('Form validation failed')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
src/components/Cashout/Components/Initial.view.tsx(5 hunks)src/components/Claim/Link/Initial.view.tsx(1 hunks)src/components/Global/GeneralRecipientInput/index.tsx(3 hunks)src/components/Global/LinkAccountComponent/index.tsx(10 hunks)src/components/Global/USBankAccountInput/index.tsx(1 hunks)src/components/Global/ValidatedInput/index.tsx(5 hunks)src/components/LandingPage/BuildOnUs/index.tsx(1 hunks)src/components/Offramp/Confirm.view.tsx(3 hunks)src/interfaces/index.ts(1 hunks)src/utils/format.utils.ts(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-11-12T09:39:20.720Z
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 (13)
src/utils/format.utils.ts (2)
12-14:
Enhance US bank account formatting and security
The current implementation is too basic for US bank account formatting needs.
Consider these security and formatting improvements:
+/**
+ * Formats a US bank account number for display
+ * @param value - The raw account number
+ * @returns Formatted and partially masked account number
+ */
export const formatUSAccountDisplay = (value: string) => {
- return value.toUpperCase()
+ if (!value) return '';
+ const cleaned = value.replace(/\D/g, '');
+ // Mask all but last 4 digits for security
+ return cleaned.slice(-4).padStart(cleaned.length, '*');
}Additionally:
- Consider separating routing number formatting into a different function
- Add validation for valid US bank account length and format
- Consider adding proper ACH formatting guidelines
Let's check for existing bank account validation patterns:
5-10: 🛠️ Refactor suggestion
Add IBAN validation and documentation
The IBAN formatting looks correct but lacks validation and proper documentation.
Consider these improvements:
+/**
+ * Formats an IBAN for display according to the standard format
+ * @param value - The raw IBAN string
+ * @returns Formatted IBAN with spaces every 4 characters
+ * @example
+ * formatIBANDisplay('DE89370400440532013000') // returns 'DE89 3704 0044 0532 0130 00'
+ */
export const formatIBANDisplay = (value: string) => {
+ if (!value) return '';
+ // Remove existing spaces first to ensure consistent formatting
+ const cleaned = value.replace(/\s/g, '');
return value
.toUpperCase()
.replace(/(.{4})/g, '$1 ')
.trim()
}Consider adding IBAN structure validation using a library like ibantools to ensure valid IBANs.
Let's check if the project already uses any IBAN validation libraries:
src/interfaces/index.ts (1)
21-32: 🛠️ Refactor suggestion
Replace any[] types with proper interfaces and document fields
The interface uses any[] types which reduces type safety. Also, there seems to be redundancy in points tracking.
Consider the following improvements:
+interface Transaction {
+ id: string
+ amount: number
+ timestamp: Date
+ type: string
+ // Add other relevant fields
+}
+interface ReferralPointsTransaction extends Transaction {
+ referralId: string
+ // Add referral-specific fields
+}
export interface IUserProfile {
+ /** Current point balance */
points: number
- transactions: any[]
+ transactions: Transaction[]
- referralsPointsTxs: any[]
+ referralsPointsTxs: ReferralPointsTransaction[]
+ /** Total number of successful referral connections */
totalReferralConnections: number
+ /** Number of users referred */
referredUsers: number
+ /** Current streak count */
streak: number
accounts: Account[]
- contacts: any[]
+ contacts: User[]
- totalPoints: number // Possibly redundant with points?
+ /** Lifetime total points earned (including spent points) */
+ totalPoints: number
user: User
}Also, please clarify the difference between points and totalPoints. Is one the current balance and the other the lifetime total?
src/components/LandingPage/BuildOnUs/index.tsx (1)
17-17: Verify the new Notion URL is accessible and correct
The integration form URL has been significantly changed. Please ensure:
- The new URL is accessible and points to the correct integration form
- No information or functionality is lost with the removal of the
pvsparameter
src/components/Global/GeneralRecipientInput/index.tsx (1)
9-9: Verify the implementation of bank account formatting utilities
The code now uses utility functions for sanitization and formatting, which is a good practice. However, we should verify their implementation to ensure they handle all edge cases correctly.
Also applies to: 44-45
✅ Verification successful
Bank account formatting utilities are properly implemented
The utility functions are implemented correctly in src/utils/format.utils.ts:
sanitizeBankAccount: Removes spaces, hyphens, dots, and underscores, converting to lowercaseformatIBANDisplay: Formats IBAN with spaces every 4 characters and converts to uppercaseformatUSAccountDisplay: Converts US account numbers to uppercase
These implementations follow standard formatting practices for bank accounts and provide consistent handling of the input values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of the imported utility functions
ast-grep --pattern 'export function sanitizeBankAccount($_) {
$$$
}'
ast-grep --pattern 'export function formatIBANDisplay($_) {
$$$
}'
ast-grep --pattern 'export function formatUSAccountDisplay($_) {
$$$
}'
Length of output: 219
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find these functions
rg "export (const|function) (sanitizeBankAccount|formatIBANDisplay|formatUSAccountDisplay)" -A 10
# Also search for the file that might contain these functions
fd "format.utils.ts"
Length of output: 866
src/components/Global/ValidatedInput/index.tsx (3)
4-4: LGTM! Clean type definition and import.
The new import and type definition are well-structured and properly typed.
Also applies to: 17-17
36-36: LGTM! Proper props destructuring.
The formatDisplayValue prop is correctly destructured alongside other props.
82-88: 🛠️ Refactor suggestion
Consider improving error handling and state management.
While the sanitization logic is good, there are a few concerns:
- Missing error handling for sanitizeBankAccount
- The isChanging flag might be misleading if sanitization removes all characters
- This appears to be temporary code based on the PR title
Let's verify the sanitizeBankAccount implementation:
Consider this safer implementation:
const handleChange = (e: ChangeEvent<HTMLInputElement>) => {
const rawValue = e.target.value
- const sanitizedValue = sanitizeBankAccount(rawValue)
+ let sanitizedValue
+ try {
+ sanitizedValue = sanitizeBankAccount(rawValue)
+ } catch (error) {
+ console.error('Error sanitizing bank account:', error)
+ sanitizedValue = rawValue
+ }
onUpdate({
value: sanitizedValue,
isValid: false,
- isChanging: !!sanitizedValue,
+ isChanging: sanitizedValue !== value,
})
}src/components/Cashout/Components/Initial.view.tsx (3)
23-23: LGTM: Clean import of bank account sanitization utility
The addition of the sanitizeBankAccount utility aligns well with the component's bank account handling requirements.
224-225: LGTM: Proper sanitization of bank account identifiers
The sanitization of account identifiers before state updates ensures consistent handling of bank account numbers.
318-330: LGTM: Improved bank account display and interaction
The UI changes enhance the visual presentation and maintain consistent account handling with proper sanitization.
Also applies to: 339-340
src/components/Claim/Link/Initial.view.tsx (1)
461-465: Verify recipient type handling across the application
The changes to recipient type handling could affect other parts of the application.
Let's verify the usage of recipient types:
✅ Verification successful
Let me try different commands to gather the necessary information.
Recipient type handling is consistent across the application
The changes to recipient type handling in Initial.view.tsx are consistent with the application's type system and usage patterns. The search results show:
- The
RecipientTypeis properly defined ininterfaces.tsas'address' | 'ens' | 'iban' | 'us' - The
setRecipientTypeusage inInitial.view.tsxcorrectly sets the type based on the recipient's address presence - The recipient type is consistently used across components for conditional rendering and flow control
- The type handling aligns with the application's payment rail logic (SEPA for IBAN, ACH for US)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for recipient type usage
echo "Searching for recipient type references..."
rg "setRecipientType|recipientType.*=|type.*=.*('address'|'iban'|'us')" -t tsx -t ts
# Search for GeneralRecipientInput usage
echo "Searching for GeneralRecipientInput usage..."
ast-grep --pattern 'GeneralRecipientInput $$$'
# Search for related test files
echo "Searching for related test files..."
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts
Length of output: 507
Script:
#!/bin/bash
# Search for recipient type usage with correct file extensions
echo "Searching for recipient type references..."
rg "setRecipientType|recipientType.*=|type.*=.*('address'|'iban'|'us')" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts
# Search for recipient type enum or type definition
echo "Searching for recipient type definitions..."
rg "type.*Recipient.*=|enum.*Recipient|interface.*Recipient" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts
# Search specifically in the Claim directory
echo "Searching in Claim directory..."
rg "setRecipientType|recipientType" src/components/Claim/ --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts
Length of output: 9956
src/components/Offramp/Confirm.view.tsx (1)
657-662: LGTM! Improved bank account display formatting
The changes enhance the presentation of bank account information using proper flex layout and consistent formatting.
| export const sanitizeBankAccount = (value: string) => { | ||
| return value.replace(/[\s\-\._]/g, '').toLowerCase() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and consider preserving case
The sanitization function needs additional safety checks and might need to preserve case for certain bank account formats.
Consider these improvements:
export const sanitizeBankAccount = (value: string) => {
+ if (!value) return '';
+ // Some bank accounts are case-sensitive
- return value.replace(/[\s\-\._]/g, '').toLowerCase()
+ return value.replace(/[\s\-\._]/g, '')
}Also, consider adding JSDoc comments to document the function's purpose and expected input format.
📝 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.
| export const sanitizeBankAccount = (value: string) => { | |
| return value.replace(/[\s\-\._]/g, '').toLowerCase() | |
| } | |
| export const sanitizeBankAccount = (value: string) => { | |
| if (!value) return ''; | |
| // Some bank accounts are case-sensitive | |
| return value.replace(/[\s\-\._]/g, '') | |
| } |
| export const sanitizeBankAccount = (value: string) => { | ||
| return value.replace(/[\s\-\._]/g, '').toLowerCase() | ||
| } | ||
|
|
||
| export const formatIBANDisplay = (value: string) => { | ||
| return value | ||
| .toUpperCase() | ||
| .replace(/(.{4})/g, '$1 ') | ||
| .trim() | ||
| } | ||
|
|
||
| export const formatUSAccountDisplay = (value: string) => { | ||
| return value.toUpperCase() | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for bank account formatting utilities
The codebase lacks dedicated test coverage for the newly added bank account formatting utilities. No test files were found for:
sanitizeBankAccountformatIBANDisplayformatUSAccountDisplay
Recommend adding test cases to verify:
- IBAN formatting with various input patterns
- US account number formatting
- Bank account sanitization with different special characters
🔗 Analysis chain
Verify test coverage for bank account formatting
Given that this PR is marked as "temp commit, not fully done", ensure proper test coverage is added before merging to production.
Let's check for existing tests:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for related test files
fd -e test.ts -e spec.ts format.utils
# Check for existing test patterns
rg -l "describe.*format.*bank|test.*iban|test.*account" --type ts
Length of output: 187
Script:
#!/bin/bash
# Let's check the content of the files that matched our pattern
# to understand the test coverage context better
echo "=== Content of files with potential test patterns ==="
rg "describe.*format.*bank|test.*iban|test.*account" -A 5 --type ts
# Let's also check if there are any test files we might have missed
echo -e "\n=== Looking for any test files containing 'sanitize' or 'format' ==="
fd -e test.ts -e spec.ts | xargs rg "sanitize|format" -l
# Check for any existing test directory structure
echo -e "\n=== Test directory structure ==="
fd -t d "test|spec|__tests__"
Length of output: 3740
| export interface IResponse { | ||
| success: boolean | ||
| data: any | ||
| message?: string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make IResponse generic and follow naming conventions
The interface uses any type which reduces type safety, and the name doesn't follow TypeScript conventions.
-export interface IResponse {
+export interface Response<T = unknown> {
success: boolean
- data: any
+ data: T
message?: string
}This allows for type-safe usage like:
const userResponse: Response<User> = ...
const profileResponse: Response<IUserProfile> = ...| export interface Account { | ||
| account_id: string | ||
| account_identifier: string | ||
| account_type: string | ||
| bridge_account_id?: string | ||
| account_details?: string // JSON string containing address details | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace string JSON with typed interface for account_details
Using a string type for JSON data reduces type safety and makes it harder to work with the address details.
Consider creating a proper interface for address details:
+interface AddressDetails {
+ street?: string
+ city?: string
+ state?: string
+ postalCode?: string
+ country?: string
+}
export interface Account {
account_id: string
account_identifier: string
account_type: string
bridge_account_id?: string
- account_details?: string // JSON string containing address details
+ account_details?: AddressDetails
}📝 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.
| export interface Account { | |
| account_id: string | |
| account_identifier: string | |
| account_type: string | |
| bridge_account_id?: string | |
| account_details?: string // JSON string containing address details | |
| } | |
| interface AddressDetails { | |
| street?: string | |
| city?: string | |
| state?: string | |
| postalCode?: string | |
| country?: string | |
| } | |
| export interface Account { | |
| account_id: string | |
| account_identifier: string | |
| account_type: string | |
| bridge_account_id?: string | |
| account_details?: AddressDetails | |
| } |
| export interface User { | ||
| userId: string | ||
| email?: string | ||
| username?: string | ||
| full_name?: string | ||
| bridge_customer_id?: string | ||
| kycStatus?: 'verified' | 'pending' | 'rejected' | undefined | ||
| profile_picture?: string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type definitions in User interface
Several improvements can be made to enhance type safety and maintainability:
export interface User {
+ /** Unique identifier for the user */
userId: string
+ /** User's email address */
email?: string
+ /** User's chosen username */
username?: string
+ /** User's full name */
full_name?: string
+ /** Bridge platform customer ID */
bridge_customer_id?: string
- kycStatus?: 'verified' | 'pending' | 'rejected' | undefined
+ kycStatus?: 'verified' | 'pending' | 'rejected'
+ /** URL to user's profile picture */
profile_picture?: string
}- Removed redundant
undefinedfrom union type as it's already optional - Added JSDoc comments for better documentation
- Consider standardizing naming convention (either camelCase or snake_case)
📝 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.
| export interface User { | |
| userId: string | |
| email?: string | |
| username?: string | |
| full_name?: string | |
| bridge_customer_id?: string | |
| kycStatus?: 'verified' | 'pending' | 'rejected' | undefined | |
| profile_picture?: string | |
| } | |
| export interface User { | |
| /** Unique identifier for the user */ | |
| userId: string | |
| /** User's email address */ | |
| email?: string | |
| /** User's chosen username */ | |
| username?: string | |
| /** User's full name */ | |
| full_name?: string | |
| /** Bridge platform customer ID */ | |
| bridge_customer_id?: string | |
| kycStatus?: 'verified' | 'pending' | 'rejected' | |
| /** URL to user's profile picture */ | |
| profile_picture?: string | |
| } |
| <div className="flex flex-col gap-2"> | ||
| <input | ||
| disabled | ||
| value={formatIBANDisplay(getAccountDetailsValue('accountNumber'))} |
There was a problem hiding this comment.
Display US account numbers without IBAN formatting
Using formatIBANDisplay on US account numbers may result in incorrect formatting. Display US account numbers as-is to ensure accuracy.
Apply this diff to correct the display:
- value={formatIBANDisplay(getAccountDetailsValue('accountNumber'))}
+ value={getAccountDetailsValue('accountNumber')}Committable suggestion skipped: line range outside the PR's diff.
| const formatDisplayValue = (value: string) => { | ||
| return formatIBANDisplay(value) | ||
| } |
There was a problem hiding this comment.
Ensure 'formatDisplayValue' handles account types correctly
Currently, formatDisplayValue uses formatIBANDisplay, which may not format US account numbers properly. Modify the function to conditionally format the account number based on the account type.
Apply this diff to update formatDisplayValue:
const formatDisplayValue = (value: string) => {
- return formatIBANDisplay(value)
+ const accountType = getAccountDetailsValue('type')
+ if (accountType === 'iban') {
+ return formatIBANDisplay(value)
+ }
+ return value // For US account numbers, no special formatting
}Committable suggestion skipped: line range outside the PR's diff.
| setValueAs: (value: string) => value?.toLowerCase(), // Store as lowercase | ||
| })} | ||
| className={`custom-input ${ibanErrors.accountNumber ? 'border border-red' : ''}`} | ||
| placeholder={'Bank account'} | ||
| autoComplete="on" | ||
| name="bank-account" | ||
| value={formatDisplayValue(getIbanFormValue('accountNumber') || '')} // Display formatted | ||
| onChange={(e) => { | ||
| // Remove spaces and store lowercase | ||
| const rawValue = e.target.value.replace(/\s/g, '') | ||
| registerIban('accountNumber').onChange({ | ||
| target: { | ||
| value: rawValue.toLowerCase(), | ||
| name: 'accountNumber', | ||
| }, |
There was a problem hiding this comment.
Avoid converting account numbers to lowercase
Converting account numbers to lowercase may lead to incorrect data, especially for numeric US account numbers. Remove the .toLowerCase() conversion to preserve the original account number.
Apply this diff to fix the issue:
{...registerIban('accountNumber', {
required: 'This field is required',
- setValueAs: (value: string) => value?.toLowerCase(), // Store as lowercase
+ setValueAs: (value: string) => value?.replace(/\s/g, ''), // Remove spaces
})}And update the onChange handler:
onChange={(e) => {
// Remove spaces
const rawValue = e.target.value.replace(/\s/g, '')
registerIban('accountNumber').onChange({
target: {
- value: rawValue.toLowerCase(),
+ value: rawValue,
name: 'accountNumber',
},
})
}}📝 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.
| setValueAs: (value: string) => value?.toLowerCase(), // Store as lowercase | |
| })} | |
| className={`custom-input ${ibanErrors.accountNumber ? 'border border-red' : ''}`} | |
| placeholder={'Bank account'} | |
| autoComplete="on" | |
| name="bank-account" | |
| value={formatDisplayValue(getIbanFormValue('accountNumber') || '')} // Display formatted | |
| onChange={(e) => { | |
| // Remove spaces and store lowercase | |
| const rawValue = e.target.value.replace(/\s/g, '') | |
| registerIban('accountNumber').onChange({ | |
| target: { | |
| value: rawValue.toLowerCase(), | |
| name: 'accountNumber', | |
| }, | |
| setValueAs: (value: string) => value?.replace(/\s/g, ''), // Remove spaces | |
| })} | |
| className={`custom-input ${ibanErrors.accountNumber ? 'border border-red' : ''}`} | |
| placeholder={'Bank account'} | |
| autoComplete="on" | |
| name="bank-account" | |
| value={formatDisplayValue(getIbanFormValue('accountNumber') || '')} // Display formatted | |
| onChange={(e) => { | |
| // Remove spaces | |
| const rawValue = e.target.value.replace(/\s/g, '') | |
| registerIban('accountNumber').onChange({ | |
| target: { | |
| value: rawValue, | |
| name: 'accountNumber', | |
| }, |
| useEffect(() => { | ||
| if (getAccountDetailsValue('type') === 'us') { | ||
| setAccountDetailsValue('country', 'USA') | ||
| } | ||
| }, [getAccountDetailsValue('type')]) |
There was a problem hiding this comment.
Fix dependency array in useEffect to properly watch 'type' changes
The dependency array in the useEffect hook uses getAccountDetailsValue('type'), which may not trigger the effect when the 'type' field changes. Instead, use the watch function from React Hook Form to monitor changes to the 'type' field.
Apply this diff to fix the issue:
+ const accountType = watch('type')
useEffect(() => {
- if (getAccountDetailsValue('type') === 'us') {
+ if (accountType === 'us') {
setAccountDetailsValue('country', 'USA')
}
- }, [getAccountDetailsValue('type')])
+ }, [accountType])📝 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.
| useEffect(() => { | |
| if (getAccountDetailsValue('type') === 'us') { | |
| setAccountDetailsValue('country', 'USA') | |
| } | |
| }, [getAccountDetailsValue('type')]) | |
| const accountType = watch('type') | |
| useEffect(() => { | |
| if (accountType === 'us') { | |
| setAccountDetailsValue('country', 'USA') | |
| } | |
| }, [accountType]) |
| const validateUSAccount = (routingNumber: string, accountNumber: string) => { | ||
| if (!/^[0-9]{9}$/.test(routingNumber)) { | ||
| return 'Invalid routing number - must be 9 digits' | ||
| } | ||
| if (!/^[0-9]{1,17}$/.test(accountNumber)) { | ||
| return 'Invalid account number' | ||
| } | ||
| return null | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Integrate 'validateUSAccount' function into the validation flow
The validateUSAccount function is defined but not utilized. To ensure proper validation of US bank account details, integrate this function into your form submission or validation logic where US account numbers and routing numbers are handled.
Consider calling validateUSAccount during the US account handling logic to validate user input:
} else if (/^[0-9]{1,17}$/.test(sanitizedAccountNumber)) {
setAccountDetailsValue('type', 'us')
setAccountDetailsValue('accountNumber', sanitizedAccountNumber)
setAccountDetailsValue('routingNumber', '')
+
+ const usAccountError = validateUSAccount(getAccountDetailsValue('routingNumber'), sanitizedAccountNumber)
+ if (usAccountError) {
+ setIbanFormError('accountNumber', { message: usAccountError })
+ return
+ }Committable suggestion skipped: line range outside the PR's diff.
No description provided.