[TASK-14554]feat: multi-step bank account form#1286
[TASK-14554]feat: multi-step bank account form#1286Zishan-7 wants to merge 3 commits intopeanut-wallet-devfrom
Conversation
Zishan-7
commented
Oct 6, 2025
Zishan-7
commented
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplaces DynamicBankAccountForm with a new MultiStepBankAccountForm in consumers and adds a MultiStepBankAccountForm module (components, steps, types, stepConfig, and barrel) implementing a three-step, account-type-aware bank details flow with validation and debounced IBAN/BIC handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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: 8
🧹 Nitpick comments (4)
src/components/MultiStepBankAccountForm/steps/AddressStep.tsx (2)
32-37: Consider country-specific validation for state field.Not all countries have states or administrative divisions. For some countries, this field might not be applicable. Consider making this field conditionally required based on the country, or providing guidance on what users should enter if their country doesn't have states.
40-46: Consider postal code format validation.Postal codes have different formats across countries (e.g., US: 5 digits, UK: alphanumeric, Canada: alphanumeric with space). The current required-only validation doesn't enforce format rules, which could lead to invalid data being submitted.
Consider adding country-specific validation patterns:
<FormInput name="postalCode" placeholder="Your Postal Code" - rules={{ required: 'Postal code is required' }} + rules={{ + required: 'Postal code is required', + pattern: { + value: getPostalCodePattern(country), + message: 'Invalid postal code format' + } + }} control={control} errors={errors} touchedFields={touchedFields} />src/components/AddWithdraw/AddWithdrawCountriesList.tsx (1)
241-241: Consider using undefined instead of null for error prop.TypeScript and React typically prefer
undefinedovernullfor optional values. The prop type should be checked to confirm what's expected.- error={null} + error={undefined}src/components/MultiStepBankAccountForm/StepIndicator.tsx (1)
10-26: Add accessibility features for screen readers.The step indicators lack semantic meaning and ARIA labels, making them inaccessible to screen reader users. Consider adding role="progressbar" or aria-label attributes.
Apply this diff to improve accessibility:
const StepIndicator: React.FC<StepIndicatorProps> = ({ currentStep, stepConfig }) => { return ( - <div className="flex items-center justify-center space-x-2"> + <div + className="flex items-center justify-center space-x-2" + role="progressbar" + aria-label={`Step ${currentStep} of ${stepConfig.totalSteps}`} + aria-valuenow={currentStep} + aria-valuemin={1} + aria-valuemax={stepConfig.totalSteps} + > {Array.from({ length: stepConfig.totalSteps }, (_, index) => { const stepNumber = index + 1 const isCurrent = currentStep === stepNumber return ( <div key={stepNumber} className={twMerge('size-2 rounded-full bg-grey-2', isCurrent && 'bg-primary-1')} + aria-label={`Step ${stepNumber}${isCurrent ? ' (current)' : ''}`} ></div> ) })} </div> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/components/AddWithdraw/AddWithdrawCountriesList.tsx(2 hunks)src/components/Claim/Link/views/BankFlowManager.view.tsx(2 hunks)src/components/MultiStepBankAccountForm/FormInput.tsx(1 hunks)src/components/MultiStepBankAccountForm/MultiStepBankAccountForm.tsx(1 hunks)src/components/MultiStepBankAccountForm/StepIndicator.tsx(1 hunks)src/components/MultiStepBankAccountForm/index.ts(1 hunks)src/components/MultiStepBankAccountForm/stepConfig.ts(1 hunks)src/components/MultiStepBankAccountForm/steps/AccountDetailsStep.tsx(1 hunks)src/components/MultiStepBankAccountForm/steps/AddressStep.tsx(1 hunks)src/components/MultiStepBankAccountForm/steps/PersonalInfoStep.tsx(1 hunks)src/components/MultiStepBankAccountForm/steps/index.ts(1 hunks)src/components/MultiStepBankAccountForm/types.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-22T15:38:48.586Z
Learnt from: kushagrasarathe
PR: peanutprotocol/peanut-ui#869
File: src/app/(mobile-ui)/withdraw/page.tsx:82-88
Timestamp: 2025-05-22T15:38:48.586Z
Learning: The country-specific withdrawal route exists at src/app/(mobile-ui)/withdraw/[...country]/page.tsx and renders the AddWithdrawCountriesList component with flow="withdraw".
Applied to files:
src/components/AddWithdraw/AddWithdrawCountriesList.tsx
📚 Learning: 2025-08-14T14:42:54.411Z
Learnt from: Zishan-7
PR: peanutprotocol/peanut-ui#1094
File: src/utils/withdraw.utils.ts:181-191
Timestamp: 2025-08-14T14:42:54.411Z
Learning: The countryCodeMap in src/components/AddMoney/consts/index.ts uses uppercase 3-letter country codes as keys (like 'AUT', 'BEL', 'CZE') that map to 2-letter country codes, requiring input normalization to uppercase for proper lookups.
Applied to files:
src/components/MultiStepBankAccountForm/stepConfig.ts
🧬 Code graph analysis (10)
src/components/MultiStepBankAccountForm/steps/AccountDetailsStep.tsx (4)
src/components/MultiStepBankAccountForm/types.ts (2)
IBankAccountDetails(1-16)AccountType(18-18)src/utils/withdraw.utils.ts (3)
validateMXCLabeAccount(82-134)getCountryFromIban(8-27)validateUSBankAccount(34-74)src/utils/bridge-accounts.utils.ts (3)
validateIban(36-38)validateBic(73-89)isValidRoutingNumber(91-118)src/app/actions/ibanToBic.ts (1)
getBicFromIban(10-17)
src/components/MultiStepBankAccountForm/steps/PersonalInfoStep.tsx (2)
src/components/MultiStepBankAccountForm/MultiStepBankAccountForm.tsx (1)
IBankAccountDetails(312-312)src/components/MultiStepBankAccountForm/types.ts (1)
IBankAccountDetails(1-16)
src/components/MultiStepBankAccountForm/steps/AddressStep.tsx (1)
src/components/MultiStepBankAccountForm/types.ts (1)
IBankAccountDetails(1-16)
src/components/AddWithdraw/AddWithdrawCountriesList.tsx (1)
src/components/MultiStepBankAccountForm/MultiStepBankAccountForm.tsx (1)
MultiStepBankAccountForm(36-307)
src/components/MultiStepBankAccountForm/StepIndicator.tsx (1)
src/components/MultiStepBankAccountForm/types.ts (1)
StepConfig(20-24)
src/components/MultiStepBankAccountForm/FormInput.tsx (1)
src/components/MultiStepBankAccountForm/types.ts (1)
IBankAccountDetails(1-16)
src/components/MultiStepBankAccountForm/types.ts (1)
src/components/MultiStepBankAccountForm/MultiStepBankAccountForm.tsx (2)
IBankAccountDetails(312-312)AccountType(312-312)
src/components/MultiStepBankAccountForm/stepConfig.ts (2)
src/components/AddMoney/consts/index.ts (1)
BRIDGE_ALPHA3_TO_ALPHA2(2487-2529)src/components/MultiStepBankAccountForm/types.ts (3)
AccountType(18-18)StepConfig(20-24)IBankAccountDetails(1-16)
src/components/MultiStepBankAccountForm/MultiStepBankAccountForm.tsx (12)
src/app/actions/types/users.types.ts (1)
AddBankAccountPayload(36-56)src/components/MultiStepBankAccountForm/types.ts (1)
IBankAccountDetails(1-16)src/components/Global/PeanutActionDetailsCard/index.tsx (2)
PeanutActionDetailsCardProps(29-52)PeanutActionDetailsCard(54-253)src/components/MultiStepBankAccountForm/stepConfig.ts (2)
getAccountType(8-13)getStepConfig(15-105)src/context/authContext.tsx (1)
useAuth(183-189)src/redux/hooks.ts (2)
useAppDispatch(5-5)useAppSelector(6-6)src/context/WithdrawFlowContext.tsx (1)
useWithdrawFlow(157-163)src/hooks/useSavedAccounts.tsx (1)
useSavedAccounts(12-25)src/hooks/useDebounce.ts (1)
useDebounce(9-23)src/redux/slices/bank-form-slice.ts (1)
bankFormActions(36-36)src/components/AddMoney/consts/index.ts (1)
ALL_COUNTRIES_ALPHA3_TO_ALPHA2(2537-2540)src/constants/zerodev.consts.ts (1)
PEANUT_WALLET_TOKEN_SYMBOL(21-21)
src/components/Claim/Link/views/BankFlowManager.view.tsx (1)
src/components/MultiStepBankAccountForm/MultiStepBankAccountForm.tsx (1)
MultiStepBankAccountForm(36-307)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (4)
src/components/MultiStepBankAccountForm/FormInput.tsx (1)
41-50: LGTM! Good async handling and trimming logic.The blur handler correctly:
- Trims string values with a type guard
- Calls the field's onBlur
- Awaits the custom onBlur callback if provided
This allows for debounced async validation patterns (like BIC validation) while maintaining proper form state.
src/components/MultiStepBankAccountForm/index.ts (1)
1-6: LGTM! Clean barrel export pattern.The barrel export properly consolidates the public API surface, making imports simpler for consumers.
src/components/MultiStepBankAccountForm/steps/index.ts (1)
1-3: LGTM! Clean step exports.Properly consolidates step components for easier importing.
src/components/AddWithdraw/AddWithdrawCountriesList.tsx (1)
236-242: initialData and error props are correctly used
PassinginitialData={{}}prevents a runtime error when spreading anderror={null}matches thestring | nullprop type. No changes required.
src/components/MultiStepBankAccountForm/steps/AccountDetailsStep.tsx
Outdated
Show resolved
Hide resolved
src/components/MultiStepBankAccountForm/steps/PersonalInfoStep.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@Zishan-7 left comments, requesting changes, mostly nits, some points:
- lots of new code, but good abstraction
- can use comments in components and logic for quick understanding
- lets not use
any - BIG concern on testing effort given time constraint
- resolve cr/bugbot comments
| const titles = [ | ||
| 'Who will receive the funds?', | ||
| 'Where should we send the money?', | ||
| 'Where this bank is located?', |
There was a problem hiding this comment.
blocking: its not the bank address, its the users address that we need to take, copy needs to be updated everywhere/anywhere relevant
| onBlur?: (field: any) => Promise<void> | void | ||
| } | ||
|
|
||
| const FormInput: React.FC<FormInputProps> = ({ |
There was a problem hiding this comment.
praise: i like this component
nit: lets add comments and not use any
| touchedFields: Partial<Record<keyof IBankAccountDetails, boolean>> | ||
| } | ||
|
|
||
| const AddressStep: React.FC<AddressStepProps> = ({ control, errors, touchedFields }) => { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/components/MultiStepBankAccountForm/stepConfig.ts (7)
17-22: [Duplicate] Fix inconsistent country code format and unsafe fallback.This issue was flagged in previous reviews: the function mixes alpha-2 ('MX') and alpha-3 ('USA') codes inconsistently, and the fallback to 'IBAN' on line 21 is unsafe as it will incorrectly classify unknown countries.
As mentioned by kushagrasarathe, this is a valid concern that needs addressing. Consider normalizing input to accept both alpha-2 and alpha-3 forms, and either use explicit IBAN country validation (via a Set of known IBAN countries) or throw an error for unsupported countries instead of defaulting to 'IBAN'.
77-77: Fix misleading copy: this is the user's address, not the bank's address.Same issue as line 32 - this should ask for the user's address, not the bank's location.
Apply this diff:
- 'Where this bank is located?', + 'What is your address?',
32-32: Fix misleading copy: this is the user's address, not the bank's address.As kushagrasarathe noted, the copy needs updating. This step asks for the user's residential address, not the bank's location.
Apply this diff:
- 'Where this bank is located?', + 'What is your address?',The same issue exists on line 77 for the MX flow.
17-22: Address the unsafe IBAN fallback flagged in previous review.The unsafe fallback to
'IBAN'for unknown countries remains unresolved from the previous review, where it was flagged as a critical issue. This will incorrectly classify unsupported countries as IBAN-compatible.As recommended in the previous review, either:
- Validate against an explicit IBAN country set, or
- Throw an error for unsupported countries
This ensures callers handle unsupported countries explicitly rather than silently misclassifying them.
17-22: Fix unsafe default to IBAN for unknown countries.The function defaults all unknown countries to 'IBAN', which is incorrect. Countries like Japan, Brazil, India, and others don't use IBAN. This will cause downstream issues when building the payload.
As previously suggested, use an explicit IBAN country set:
+const IBAN_COUNTRIES = new Set([ + 'AUT', 'BEL', 'BGR', 'HRV', 'CYP', 'CZE', 'DNK', 'EST', 'FIN', 'FRA', + 'DEU', 'GRC', 'HUN', 'ISL', 'IRL', 'ITA', 'LVA', 'LIE', 'LTU', 'LUX', + 'MLT', 'NLD', 'NOR', 'POL', 'PRT', 'ROU', 'SVK', 'SVN', 'ESP', 'SWE', + 'CHE', 'GBR' +]) + export const getAccountType = (country: string): AccountType => { const upperCountry = country.toUpperCase() if (upperCountry === 'USA') return 'US' if (upperCountry === 'MX' || upperCountry === 'MEX') return 'MX' - return 'IBAN' + if (IBAN_COUNTRIES.has(upperCountry)) return 'IBAN' + throw new Error(`Unsupported country code: ${country}`) }
28-34: Update misleading step title.The title "Where this bank is located?" is incorrect—this step collects the user's address, not the bank's address.
Apply this diff:
const titles = [ 'Who will receive the funds?', 'Where should we send the money?', - 'Where this bank is located?', + 'What is your address?', ]
73-79: Update misleading step title (MX flow).Same issue as the US flow—this collects the user's address, not the bank's location.
Apply this diff:
const titles = [ 'Who will receive the funds?', 'Where should we send the money?', - 'Where this bank is located?', + 'What is your address?', ]
🧹 Nitpick comments (12)
src/components/MultiStepBankAccountForm/stepConfig.ts (2)
4-96: Add documentation comments for better maintainability.As requested in previous review, the file needs comments to understand the use and flow. Consider adding JSDoc comments for:
- The purpose of each validator function (lines 5-15)
- The
getAccountTypefunction's behavior and expected inputs- The
getStepConfigfunction and the StepConfig pattern- Each account type configuration explaining when it's used
Example:
/** * Validates that required personal information fields are present and error-free. * @param data - The form data to validate * @param errors - React Hook Form errors object * @returns true if firstName and lastName are valid */ const validatePersonalInfo = (data: IBankAccountDetails, errors: FieldErrors<IBankAccountDetails>): boolean => {
24-96: Add documentation comments for clarity.Previous feedback requested comments to understand the use and flow of this configuration system. Consider adding JSDoc comments explaining:
- The purpose of
getStepConfig- How the step numbering works (1-indexed)
- What each account type's flow entails
Additionally, the step titles for US and MX are identical (lines 29-33, 74-78). Consider extracting to a shared constant:
const THREE_STEP_TITLES = [ 'Who will receive the funds?', 'Where should we send the money?', 'Where this bank is located?', ] as constThen use it in both configs:
US: { totalSteps: 3, getStepTitle: (step: number) => { - const titles = [ - 'Who will receive the funds?', - 'Where should we send the money?', - 'Where this bank is located?', - ] - return titles[step - 1] || '' + return THREE_STEP_TITLES[step - 1] || '' },src/components/MultiStepBankAccountForm/FormInput.tsx (3)
7-16: Excellent type safety improvements!The
anytypes have been properly replaced with react-hook-form types (RegisterOptionsandControllerRenderProps). This provides full type safety and autocomplete support.As kushagrasarathe noted, this component looks good. Consider adding JSDoc comments to explain:
- The auto-trim behavior on blur
- When to use the optional
onBlurcallback- The purpose of
touchedFieldsin error display logic
7-16: Good fix of type safety issues.The interface now uses proper react-hook-form types (
RegisterOptions,ControllerRenderProps) instead ofany, successfully addressing the previous review concern.Based on learnings: The codebase follows a strict policy of avoiding
anytypes.Optionally, consider adding JSDoc comments to document the props, especially
onBlurwhich has a specific use case (debounced validation for IBAN/BIC fields).
18-60: Consider adding component documentation.Previous feedback requested comments for this component. Consider adding a JSDoc comment explaining:
- The component's purpose (controlled input with react-hook-form)
- The trim-on-blur behavior (line 43-45)
- The optional async
onBlurcallback use case (debounced BIC/IBAN validation)Example:
/** * A controlled form input component using react-hook-form's Controller. * Automatically trims whitespace on blur and supports async validation callbacks. * Displays validation errors only after the field has been touched. */ const FormInput: React.FC<FormInputProps> = ({ ... }) => {src/components/MultiStepBankAccountForm/MultiStepBankAccountForm.tsx (5)
207-208: Type error asunknowninstead of inferring from catch.Caught errors should be typed as
unknownand narrowed with type guards rather than assumingError.Apply this diff:
- } catch (error) { + } catch (error: unknown) { setSubmissionError(error instanceof Error ? error.message : 'An unknown error occurred')
236-270: Consider extracting step rendering logic.As kushagrasarathe suggested in previous review, a helper function using a switch statement would improve readability and maintainability.
However, the current inline approach is clear enough. If you add more account types in the future (e.g., PIX, Mercado Pago), extracting to a dedicated
renderStepfunction would make the component more maintainable:const renderStep = ( step: number, accountType: AccountType, props: StepRenderProps ): JSX.Element | null => { switch (step) { case 1: return <PersonalInfoStep {...props.personalInfo} /> case 2: return <AccountDetailsStep {...props.accountDetails} /> case 3: return <AddressStep {...props.address} /> default: return null } }
236-270: Consider consolidating step rendering with step configuration.Previous feedback suggested improving the step rendering approach. While the current switch statement is clear, consider adding step component mappings to the
StepConfiginstepConfig.tsfor better scalability:// In stepConfig.ts export interface StepConfig { totalSteps: number getStepTitle: (step: number) => string getStepComponent: (step: number) => React.ComponentType<StepProps> isStepValid: (step: number, data: IBankAccountDetails, errors: FieldErrors<IBankAccountDetails>) => boolean }This would make adding new account types (e.g., Mercado Pago, PIX) more straightforward, as mentioned in the previous feedback.
66-68: Name splitting has limitations for multi-word first names.The current implementation treats the first word as
firstNameand everything else aslastName. This won't handle cases like "Mary Anne Smith" correctly (would split to firstName: "Mary", lastName: "Anne Smith").Consider documenting this limitation or using a more sophisticated name parser if handling international names with multiple given names is important. For most cases, this simple approach is acceptable.
159-181: Consider using sanitizeBankAccount for consistency.Line 161 manually removes spaces with
.replace(/\s/g, ''), butsanitizeBankAccountis already available and used elsewhere (line 136). Using it consistently would reduce duplication and ensure the same normalization is applied everywhere.const payload: Partial<AddBankAccountPayload> = { accountType: bridgeAccountType, - accountNumber: accountNumber.replace(/\s/g, ''), + accountNumber: sanitizeBankAccount(accountNumber), countryCode: isUs ? 'USA' : country.toUpperCase(),src/components/MultiStepBankAccountForm/types.ts (2)
3-18: Consider documenting field usage by account type.The
IBankAccountDetailstype includes all possible fields for US, IBAN, and MX account types, but only subsets are required for each type. Consider adding JSDoc comments to clarify which fields are used by which account type.Example:
export type IBankAccountDetails = { name?: string firstName: string lastName: string email: string /** Required for all account types (IBAN uses this for IBAN number, US/MX for account number) */ accountNumber: string /** Required for IBAN accounts only */ bic: string /** Required for US accounts only */ routingNumber: string /** Required for MX accounts only */ clabe: string // ... rest of fields }This would improve maintainability and reduce confusion about which fields are actually validated for each flow.
3-18: Consider making account-type-specific fields optional.Currently, all fields (except
name) are required strings. However,bic,routingNumber,clabe, andibanare only used for specific account types. Making them optional would better represent the domain model.export type IBankAccountDetails = { name?: string firstName: string lastName: string email: string accountNumber: string - bic: string - routingNumber: string - clabe: string + bic?: string + routingNumber?: string + clabe?: string street: string city: string state: string postalCode: string - iban: string + iban?: string country: string }This would require updating validation checks to handle
undefined, but would more accurately reflect which fields are required for each account type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/MultiStepBankAccountForm/FormInput.tsx(1 hunks)src/components/MultiStepBankAccountForm/MultiStepBankAccountForm.tsx(1 hunks)src/components/MultiStepBankAccountForm/stepConfig.ts(1 hunks)src/components/MultiStepBankAccountForm/steps/AccountDetailsStep.tsx(1 hunks)src/components/MultiStepBankAccountForm/steps/PersonalInfoStep.tsx(1 hunks)src/components/MultiStepBankAccountForm/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/MultiStepBankAccountForm/steps/PersonalInfoStep.tsx
- src/components/MultiStepBankAccountForm/steps/AccountDetailsStep.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-14T14:42:54.411Z
Learnt from: Zishan-7
PR: peanutprotocol/peanut-ui#1094
File: src/utils/withdraw.utils.ts:181-191
Timestamp: 2025-08-14T14:42:54.411Z
Learning: The countryCodeMap in src/components/AddMoney/consts/index.ts uses uppercase 3-letter country codes as keys (like 'AUT', 'BEL', 'CZE') that map to 2-letter country codes, requiring input normalization to uppercase for proper lookups.
Applied to files:
src/components/MultiStepBankAccountForm/stepConfig.ts
📚 Learning: 2025-10-07T10:19:20.059Z
Learnt from: kushagrasarathe
PR: peanutprotocol/peanut-ui#1286
File: src/components/MultiStepBankAccountForm/FormInput.tsx:10-10
Timestamp: 2025-10-07T10:19:20.059Z
Learning: The codebase follows a strict policy of avoiding `any` types. Always use proper TypeScript types, particularly from libraries like react-hook-form (RegisterOptions, FieldErrors, UseFormSetValue, etc.) rather than using `any`.
Applied to files:
src/components/MultiStepBankAccountForm/FormInput.tsx
🧬 Code graph analysis (3)
src/components/MultiStepBankAccountForm/stepConfig.ts (1)
src/components/MultiStepBankAccountForm/types.ts (3)
IBankAccountDetails(3-18)AccountType(20-20)StepConfig(22-26)
src/components/MultiStepBankAccountForm/MultiStepBankAccountForm.tsx (11)
src/app/actions/types/users.types.ts (1)
AddBankAccountPayload(36-56)src/components/MultiStepBankAccountForm/types.ts (1)
IBankAccountDetails(3-18)src/components/Global/PeanutActionDetailsCard/index.tsx (2)
PeanutActionDetailsCardProps(29-52)PeanutActionDetailsCard(54-253)src/components/MultiStepBankAccountForm/stepConfig.ts (2)
getAccountType(17-22)getStepConfig(24-96)src/context/authContext.tsx (1)
useAuth(183-189)src/redux/hooks.ts (2)
useAppDispatch(5-5)useAppSelector(6-6)src/context/WithdrawFlowContext.tsx (1)
useWithdrawFlow(157-163)src/hooks/useSavedAccounts.tsx (1)
useSavedAccounts(12-25)src/hooks/useDebounce.ts (1)
useDebounce(9-23)src/utils/format.utils.ts (1)
sanitizeBankAccount(1-4)src/redux/slices/bank-form-slice.ts (1)
bankFormActions(36-36)
src/components/MultiStepBankAccountForm/FormInput.tsx (1)
src/components/MultiStepBankAccountForm/types.ts (1)
IBankAccountDetails(3-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (19)
src/components/MultiStepBankAccountForm/FormInput.tsx (3)
41-49: LGTM: Thoughtful whitespace trimming.The automatic trimming of string values on blur (lines 43-44) prevents common user input errors while preserving the user's ability to type freely.
7-16: Excellent type safety improvements!The interface now uses proper react-hook-form types throughout:
rulesis correctly typed asRegisterOptions<IBankAccountDetails, keyof IBankAccountDetails>onBlurusesControllerRenderProps<IBankAccountDetails, keyof IBankAccountDetails>This addresses the previous review comments and aligns with the codebase policy of avoiding
anytypes.Based on learnings.
18-60: LGTM! Clean and well-structured component.The implementation is solid:
- Proper use of react-hook-form's
Controller- Automatic whitespace trimming on blur for data sanitization
- Conditional error display based on both error presence and field touch state
- Clean integration with
BaseInputandErrorAlertsrc/components/MultiStepBankAccountForm/types.ts (2)
3-18: Verify that all fields should be required.All fields in
IBankAccountDetailsare marked as required (non-optional), but the form uses them conditionally based on account type:
- US accounts need
accountNumberandroutingNumber- IBAN accounts need
accountNumberandbic- MX accounts need
clabeThis means most fields will have empty strings rather than being truly optional. While this works, it's less type-safe than using optional fields with proper guards.
Consider whether fields that are conditionally required should be marked optional:
export type IBankAccountDetails = { name?: string firstName: string lastName: string email: string accountNumber?: string // Only required for US/IBAN bic?: string // Only required for IBAN routingNumber?: string // Only required for US clabe?: string // Only required for MX street?: string // Only required for US/MX city?: string // Only required for US/MX state?: string // Only required for US/MX postalCode?: string // Only required for US/MX iban?: string country: string }This would require adding runtime type guards but would make the type system more accurate.
20-26: LGTM! Clean type definitions.The types are well-structured:
AccountTypeclearly defines the three supported account typesStepConfiginterface properly types the validation callback withFieldErrors<IBankAccountDetails>instead ofanyBased on learnings.
src/components/MultiStepBankAccountForm/MultiStepBankAccountForm.tsx (11)
136-139: Good fix: Identifiers now normalized before comparison.The use of
sanitizeBankAccounton both the input identifier (line 136) and saved account identifier (line 138) properly addresses the previous issue where IBANs with spaces/case differences weren't matching.
183-192: Verify iban field assignment logic.The logic for assigning the
ibanfield in the result object (line 185) is complex:iban: isIban ? data.accountNumber || iban || '' : '',This suggests that for IBAN accounts:
- The form field
accountNumbermight contain the IBAN- Or there's a separate
ibanvalue fromgetValues('iban')- Otherwise it defaults to empty string
Please confirm:
- Should
data.accountNumbercontain the IBAN for IBAN account types?- When would
ibanfromgetValues('iban')be used vsdata.accountNumber?- Is there an
ibaninput field in the form, or is this dead code?Looking at line 157:
const iban = data.iban || getValues('iban')butibanis never used except in line 185. This might be unnecessary complexity.
162-162: Confirm Bridge API country code format
Couldn't locate any existing Bridge API calls to verify whethercountryCodeshould be 2-letter or 3-letter. Lines 162 and 174 set it to'USA'orcountry.toUpperCase(). Confirm with Bridge documentation or working examples that this matches the API requirements.
136-139: Good fix of identifier normalization.The duplicate account check now properly normalizes both the input identifier and saved account identifiers using
sanitizeBankAccount, successfully addressing the concern from the previous review.
159-177: Incorrect concern about country code format
countryis always provided viagetCountryCodeForWithdraw, which guarantees a valid uppercase alpha-3 code, so usingcountry.toUpperCase()forcountryCodeandaddress.countryis correct.Likely an incorrect or invalid review comment.
27-51: LGTM! Well-structured component API.The props interface is comprehensive and the use of
forwardRefto exposehandleSubmitis a good pattern for external control of the form submission.
80-105: LGTM! Comprehensive form initialization.The default values properly cascade from multiple sources (user data, initialData, persisted Redux state), with persisted data taking precedence. The validation modes (
onBlurandonSubmit) provide good UX by avoiding premature validation while typing.
107-120: LGTM! Clean debouncing pattern for BIC validation.The 500ms debounce prevents excessive API calls while the user is typing, and the
useEffectproperly validates only for IBAN accounts. Good separation of concerns.
136-145: LGTM! Proper normalization for account deduplication.The use of
sanitizeBankAccounton both the input identifier and saved account identifiers ensures consistent normalization (removes spaces, hyphens, dots, underscores, and lowercases). This correctly addresses the previous review comment about IBAN mismatch due to formatting differences.
236-270: LGTM! Clear step rendering logic.The switch statement provides a straightforward way to render the appropriate step component. While a helper function could extract this, the current implementation is clear and maintainable.
272-319: LGTM! Well-structured JSX with proper error handling.The render includes:
- Clear step progression with
StepIndicator- Conditional button label ("Next" vs "Review" on final step)
- Proper use of
twMergefor conditional styling- Appropriate error display prioritizing submission errors over prop errors
- Correct button state management (disabled when invalid or submitting)
src/components/MultiStepBankAccountForm/stepConfig.ts (3)
4-15: Good refactor of validation logic.The extraction of
validatePersonalInfoandvalidateAddressinto shared validators successfully addresses the DRY concern from the previous review. The implementation is clean and properly typed.
4-15: LGTM! Clean extraction of common validators.The validation functions correctly check for required fields and absence of errors. Good DRY implementation that's reused across multiple account type configurations.
36-92: LGTM! Well-structured step configurations.The per-account-type configurations correctly specify:
- Total steps for each flow (US: 3, IBAN: 2, MX: 3)
- Appropriate validation logic reusing common validators
- Type-specific field checks (routingNumber for US, bic for IBAN, clabe for MX)
3 participants
