[ISSUE-169] fix: us link account form #487
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe pull request introduces modifications to error handling in 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 (
|
| console.error('Error creating external account', e) | ||
| } | ||
| throw new Error(`HTTP error! status: ${response.status}`) | ||
| return new NextResponse('', { |
There was a problem hiding this comment.
question (non-blocking): is an empty data field here enough to handle all errors to function it returns it too? wdyt?
There was a problem hiding this comment.
as an additional note that's visible in the TODOs in these diffs: we need a std way to handle these errors and propagate that error state in the whole UI. I'm currently creating a prototype of what a solution could look like in peanut-wallet.
tl;dr: don't waste too much time on this
There was a problem hiding this comment.
the empty data field is not enough and I love that you are doing something about the error state, it is needed
There was a problem hiding this comment.
cool!
this is not the scope to fix that and I have a Task and TODOs around that.
![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 (3)
src/components/Global/LinkAccountComponent/index.tsx (3)
Line range hint
89-102: Consider enhancing account number validationThe current validation logic for US account numbers uses a magic number in the regex pattern (
[0-9]{6,17}). Consider:
- Extracting this pattern to a constant with documentation
- Adding more comprehensive validation for US account numbers
- Adding unit tests for both IBAN and US account validation paths
+ // US bank account numbers are typically between 6-17 digits + const US_ACCOUNT_NUMBER_PATTERN = /^[0-9]{6,17}$/; + if (isIBAN(sanitizedAccountNumber)) { setAccountDetailsValue('type', 'iban') - } else if (/^[0-9]{6,17}$/.test(sanitizedAccountNumber)) { + } else if (US_ACCOUNT_NUMBER_PATTERN.test(sanitizedAccountNumber)) { setAccountDetailsValue('type', 'us') } else {
Line range hint
134-176: Enhance error messages for better user experienceWhile the error handling logic is solid, consider making error messages more user-friendly and actionable:
- setErrorState({ showError: true, errorMessage: 'This account has already been linked' }) + setErrorState({ + showError: true, + errorMessage: 'This account is already linked to your profile. Please use a different account or contact support if you believe this is an error.' + }) if (!response.success) { if (response.data && response.data.code == 'duplicate_external_account') { - const errorMessage = 'An external account with the same information has already been added for this customer' + const errorMessage = 'This account appears to be already registered. Please verify the account details or contact support for assistance.' throw new Error(errorMessage) } - throw new Error('Creating Bridge account failed') + throw new Error('Unable to link your account at this time. Please try again later or contact support if the issue persists.') }
Line range hint
279-372: Refactor repeated validation conditionsThe form contains multiple instances of the same validation condition for US accounts. Consider extracting this logic to improve maintainability:
+ const isUsAccount = () => getAccountDetailsValue('type') === 'us'; + const getRequiredFieldValidation = () => ({ + required: isUsAccount() ? 'This field is required' : false + }); + <input {...registerAccountDetails('street', { - required: getAccountDetailsValue('type') === 'us' ? 'This field is required' : false, + ...getRequiredFieldValidation() })} className={`custom-input ${accountDetailsErrors.street ? 'border border-red' : ''}`} placeholder="Your street and number" />This pattern can be applied to all address fields to reduce code duplication and make future updates easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/app/api/bridge/external-account/create-external-account/route.ts (1 hunks)
- src/components/Global/LinkAccountComponent/index.tsx (2 hunks)
- src/utils/cashout.utils.ts (1 hunks)
🔇 Additional comments (4)
src/app/api/bridge/external-account/create-external-account/route.ts (1)
69-99: Verify US account form error handling.Let's ensure these error handling changes properly support the US account form requirements.
✅ Verification successful
Error handling implementation is consistent and well-structured
The error handling changes in the route align well with existing patterns across the codebase:
- The
duplicate_external_accounterror is consistently handled in both frontend (LinkAccountComponent) and backend (cashout.utils.ts)- Frontend properly displays user-friendly error messages through the
errorStatemechanism- Error responses maintain the same structure across components with proper status codes and content types
The TODO comment about standardizing error responses is valid but doesn't impact current functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for US account form error handling in the frontend rg -l "us.*account.*error" --type ts --type tsx # Look for related test files fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx accountLength of output: 136
Script:
#!/bin/bash # Search for US account form error handling with correct ripgrep syntax rg -t ts "us.*account.*error" # Look for error handling patterns in account-related files rg -t ts "duplicate_external_account|createExternalAccount.*error" # Find account-related test files using fd without tsx extension fd -e ts account # Search for error handling patterns in the frontend components ast-grep --pattern 'catch (e) { $$$$ }'Length of output: 779
Script:
#!/bin/bash # Check the implementation of error handling in LinkAccountComponent rg -A 10 "duplicate_external_account" src/components/Global/LinkAccountComponent/index.tsx # Check the implementation in cashout utils rg -A 10 "duplicate_external_account" src/utils/cashout.utils.ts # Look for any other error handling patterns in these files rg "error|Error" src/components/Global/LinkAccountComponent/index.tsx src/utils/cashout.utils.tsLength of output: 12206
src/utils/cashout.utils.ts (2)
191-215:⚠️ Potential issueImprove error handling consistency and type safety.
While the error handling improvements are good, there are a few areas that need attention:
- The error type for duplicate accounts is currently undefined in
interfaces.IResponse.- Error handling is inconsistent - duplicate accounts return structured response while other errors throw.
- The generic "Unexpected error" message doesn't provide useful context.
Consider these improvements:
try { const data = await response.json() if (data.code && data.code == 'duplicate_external_account') { - // if bridge account already exists for this iban - // but somehow not stored in our DB (this should never happen in - // prod, but can happen if same email used in prod & staging) - // - // returns: not interfaces.IBridgeAccount type, but - // a currently undefined error type on the data field of interfaces.IResponse - // of format: - // { - // id: '4c537540-80bf-41dd-a528-d79a4', - // code: 'duplicate_external_account', - // message: 'An external account with the same information has already been added for this customer' - // } - // - // TODO: HTTP responses need to be standardized client wide return { success: false, data, } as interfaces.IResponse } + // Handle other error cases consistently + return { + success: false, + data: { + code: data.code || 'unknown_error', + message: data.message || 'Failed to create external account', + }, + } as interfaces.IResponse } catch (error) { console.error('Error creating external account', response) - throw new Error('Unexpected error') + return { + success: false, + data: { + code: 'parse_error', + message: 'Failed to parse error response', + }, + } as interfaces.IResponse }Let's verify the error type definition:
#!/bin/bash # Search for IResponse interface definition rg -A 10 "interface IResponse"Consider creating a standardized error handling utility that:
- Defines consistent error types and responses
- Provides type-safe error handling functions
- Implements uniform error logging and reporting
218-218: 🛠️ Refactor suggestionAdd runtime type validation for the response data.
The success case assumes the response matches
IBridgeAccounttype without validation.Consider adding runtime type validation:
-const data = await response.json() +const data = await response.json() +// Add type validation helper +if (!isValidBridgeAccount(data)) { + return { + success: false, + data: { + code: 'invalid_response', + message: 'Invalid account data received', + }, + } as interfaces.IResponse +}Let's check the IBridgeAccount interface definition:
src/components/Global/LinkAccountComponent/index.tsx (1)
57-57: LGTM: Improved form error handlingThe addition of
clearErrorsand its usage with the country field improves the form state management by properly clearing field-specific errors instead of using undefined values.Also applies to: 372-372
| }, | ||
| }) | ||
| } | ||
| const data = await response.json() |
There was a problem hiding this comment.
Add error handling for successful response parsing.
The JSON parsing of the successful response should also be wrapped in a try-catch block to prevent unhandled exceptions.
-const data = await response.json()
+let data;
+try {
+ data = await response.json()
+} catch (e) {
+ console.error('Error parsing successful response:', e)
+ return new NextResponse('Internal Server Error', { status: 500 })
+}📝 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.
| const data = await response.json() | |
| let data; | |
| try { | |
| data = await response.json() | |
| } catch (e) { | |
| console.error('Error parsing successful response:', e) | |
| return new NextResponse('Internal Server Error', { status: 500 }) | |
| } |
| try { | ||
| const data = await response.json() | ||
| if (data.code && data.code == 'duplicate_external_account') { | ||
| // in case the bridge account already exists | ||
| // | ||
| // sending back error responses is not currently common across app | ||
| // in how we deliver errors from backend -> frontend | ||
| // sends back an object like: | ||
| // { | ||
| // id: '4c537540-80bf-41dd-a528-dbe39a4', | ||
| // code: 'duplicate_external_account', | ||
| // message: 'An external account with the same information has already been added for this customer' | ||
| // } | ||
| // | ||
| // TODO: standardize error responses across backend | ||
| return new NextResponse(JSON.stringify(data), { | ||
| status: response.status, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }) | ||
| } | ||
| } catch (e) { | ||
| console.error('Error creating external account', e) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling to provide more informative responses.
While the try-catch block for JSON parsing is good, the error handling could be more informative:
- The catch block's error logging could include the response status and URL
- Consider adding type safety for the error response structure
try {
const data = await response.json()
+ // Add type safety for error response
+ type BridgeError = {
+ id: string;
+ code: string;
+ message: string;
+ }
+ const errorData = data as BridgeError;
- if (data.code && data.code == 'duplicate_external_account') {
+ if (errorData.code && errorData.code === 'duplicate_external_account') {
return new NextResponse(JSON.stringify(data), {
status: response.status,
headers: {
'Content-Type': 'application/json',
},
})
}
} catch (e) {
- console.error('Error creating external account', e)
+ console.error(`Error creating external account (status: ${response.status})`, e)
}Committable suggestion was skipped due to low confidence.
| return new NextResponse('', { | ||
| status: response.status, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Consider preserving error details in non-duplicate error cases.
The current implementation returns an empty response for non-duplicate errors, which might make debugging harder for clients.
-return new NextResponse('', {
+return new NextResponse(JSON.stringify({
+ code: 'external_account_error',
+ message: `Failed to create external account (status: ${response.status})`
+}), {
status: response.status,
headers: {
'Content-Type': 'application/json',
},
})📝 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.
| return new NextResponse('', { | |
| status: response.status, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }) | |
| return new NextResponse(JSON.stringify({ | |
| code: 'external_account_error', | |
| message: `Failed to create external account (status: ${response.status})` | |
| }), { | |
| status: response.status, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }) |
US account details form was impossible to advance due to wrong error handling and clearing, this is now fixed. Also modified a little the handling of errors for create-external-account, because on error the API doesn't return a json so it was always failing when trying to parse it