fix: Propagate error when bridge account already exists for IBAN #423
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes introduce enhancements to the error handling and type definitions for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LinkAccountComponent
participant CashoutUtils
participant ExternalAPI
User->>LinkAccountComponent: Submit account linking
LinkAccountComponent->>CashoutUtils: createExternalAccount()
CashoutUtils->>ExternalAPI: POST request to create account
ExternalAPI-->>CashoutUtils: Response (success / error)
alt Duplicate Account
CashoutUtils-->>LinkAccountComponent: Return IResponse(success: false, data: errorDetails)
else Success
CashoutUtils-->>LinkAccountComponent: Return IResponse(success: true, data: accountDetails)
end
LinkAccountComponent-->>User: Display result
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: 7
🧹 Outside diff range and nitpick comments (3)
src/interfaces/interfaces.ts (1)
5-8: Approve the addition of IResponse interface with a suggestion for improvement.The new
IResponseinterface is a good addition for standardizing response structures across the application. It aligns well with the PR objective of improving error handling.However, to enhance type safety, consider using a generic type parameter for the
dataproperty instead ofany. This would allow for more specific typing when using the interface.Here's a suggested improvement:
export interface IResponse<T = unknown> { success: boolean; data?: T; }This change allows you to specify the type of
datawhen using the interface, e.g.,IResponse<UserData>, while still maintaining flexibility with the defaultunknowntype.src/utils/cashout.utils.ts (2)
216-219: Remove unnecessary type assertionsThe use of
as interfaces.IResponsemay be unnecessary if TypeScript can infer the return type based on the object structure. Removing redundant type assertions can simplify the code.Suggested change:
- } as interfaces.IResponse; + }
193-212: Standardize HTTP response handlingThe TODO comment indicates a need to standardize HTTP responses across the client. Implementing a unified approach to error handling and response formatting will improve maintainability and ease of debugging.
Consider creating a utility function to handle HTTP responses and errors uniformly throughout the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/app/api/bridge/external-account/create-external-account/route.ts (1 hunks)
- src/components/Global/LinkAccountComponent/index.tsx (3 hunks)
- src/interfaces/interfaces.ts (1 hunks)
- src/utils/cashout.utils.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
src/interfaces/interfaces.ts (2)
Line range hint
1-1: Overall, the file structure and content are well-organized and maintained.The addition of the
IResponseinterface enhances the existing set of interfaces without disrupting the current structure. The file continues to provide a comprehensive set of type definitions for various aspects of the application, promoting type safety and code clarity.As the codebase evolves, consider:
- Grouping related interfaces more tightly (e.g., all bridge-related interfaces together).
- Adding JSDoc comments to complex interfaces for better documentation.
- Regularly reviewing and refactoring interfaces to ensure they remain relevant and efficient.
5-8: Verify the usage of IResponse across the codebase.The addition of the
IResponseinterface provides an opportunity to standardize response handling across the application. To ensure consistency and maximize the benefits of this new interface:
- Identify all API calls and functions that return response objects.
- Gradually update these to use the new
IResponseinterface.- Ensure error handling in components and services is updated to work with the new structure.
To help with this process, you can run the following script to find potential places where
IResponsecould be used:This will help identify functions and variables that might benefit from using the new
IResponseinterface.✅ Verification successful
IResponse is not currently used in the codebase.
No existing functions or variables are utilizing the
IResponseinterface at this time. Consider integratingIResponsein relevant parts of the codebase to standardize response handling moving forward.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for function return types and variable declarations that might benefit from using IResponse rg --type typescript -e 'function.*:.*{' -e 'const.*=.*{.*success.*:.*boolean'Length of output: 224
Script:
#!/bin/bash # Search for function return types and variable declarations that might benefit from using IResponse rg --type typescript -e 'function.*:.*\{' -e 'const.*=.*\{.*success.*:.*boolean'Length of output: 117
Script:
#!/bin/bash # Search for function return types and variable declarations that might benefit from using IResponse rg --glob '*.ts' -e 'function.*:.*\{' -e 'const.*=.*\{.*success.*:.*boolean'Length of output: 7120
Script:
#!/bin/bash # Search for usages of IResponse in type annotations, interface implementations, and variable declarations rg --glob '*.ts' -e ':\s*IResponse\b' -e 'implements\s+IResponse\b' -e '=\s*IResponse\b'Length of output: 90
src/components/Global/LinkAccountComponent/index.tsx (2)
14-14: Importing necessary interfaces – looks good!The import of
IBridgeAccountandIResponsefrom@/interfacesis appropriate and enhances type safety.
176-182: Correctly typed response and function call – looks good!The call to
utils.createExternalAccountis properly typed withIResponse, and the parameters passed are appropriate. This improves type safety and clarity in your code.
| 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', | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Handling of 'duplicate_external_account' Errors Detected
The error handling for duplicate_external_account is implemented in src/app/api/bridge/external-account/create-external-account/route.ts but is not consistently handled in other parts of the codebase, such as:
src/utils/cashout.utils.tssrc/components/Global/LinkAccountComponent/index.tsx
This inconsistency may lead to unpredictable behaviors and complicate error management.
Recommendations:
-
Standardize Error Handling:
- Ensure that
duplicate_external_accounterrors are handled uniformly across all modules.
- Ensure that
-
Refactor Existing Implementations:
- Update
src/utils/cashout.utils.tsandsrc/components/Global/LinkAccountComponent/index.tsxto follow the same error handling pattern as inroute.ts.
- Update
-
Track Standardization Efforts:
- Create a separate issue to address the standardization of error responses across the backend.
🔗 Analysis chain
Approve changes with suggestions for improvement
The new error handling for duplicate external accounts is a good addition. It properly handles the specific case and maintains consistency with the API response.
However, there are a few points to consider:
-
The comment block (lines 74-83) is quite verbose. Consider refactoring it to be more concise while still conveying the necessary information.
-
The TODO comment about standardizing error responses across the backend is important. I recommend creating a separate issue to track this task for a more comprehensive solution.
Here's a suggestion for a more concise comment:
// Handle case where bridge account already exists
// TODO: Standardize error responses across backend
// Current response structure:
// {
// id: string,
// code: 'duplicate_external_account',
// message: string
// }Would you like me to create a GitHub issue for standardizing error responses across the backend?
Let's verify if this new error handling is consistently used across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent handling of 'duplicate_external_account' error
# Test 1: Search for other occurrences of 'duplicate_external_account'
echo "Searching for 'duplicate_external_account' occurrences:"
rg --type typescript "duplicate_external_account"
# Test 2: Search for similar error handling patterns
echo "\nSearching for similar error handling patterns:"
ast-grep --lang typescript --pattern $'if ($DATA.code && $DATA.code == $_) {
return new NextResponse(JSON.stringify($DATA), {
status: $_.status,
headers: {
"Content-Type": "application/json",
},
})
}'
Length of output: 542
Script:
#!/bin/bash
# Description: Check for consistent handling of 'duplicate_external_account' error without specifying file type
# Test 1: Search for other occurrences of 'duplicate_external_account'
echo "Searching for 'duplicate_external_account' occurrences across all files:"
rg "duplicate_external_account"
# Test 2: Search for similar error handling patterns without specifying language
echo "\nSearching for similar error handling patterns across all files:"
ast-grep --lang typescript --pattern $'if ($DATA.code && $DATA.code == $_) {
return new NextResponse(JSON.stringify($DATA), {
status: $_.status,
headers: {
"Content-Type": "application/json",
},
})
}'
Length of output: 1160
Script:
#!/bin/bash
# Description: Verify consistent handling of 'duplicate_external_account' in identified files
# Define the files to check based on previous search results
FILES=(
"src/utils/cashout.utils.ts"
"src/components/Global/LinkAccountComponent/index.tsx"
"src/app/api/bridge/external-account/create-external-account/route.ts"
)
# Loop through each file and extract the error handling block for 'duplicate_external_account'
for FILE in "${FILES[@]}"; do
echo "Extracting error handling for 'duplicate_external_account' in $FILE:"
ast-grep --lang typescript --pattern $'if ($DATA.code && $DATA.code == "duplicate_external_account") { $$$ }' "$FILE"
done
Length of output: 1451
| const data = await response.json() | ||
|
|
||
| if (!response.ok) { | ||
| if (data.code && data.code == 'duplicate_external_account') { |
There was a problem hiding this comment.
Use strict equality for string comparison
For type safety and to prevent unexpected type coercion, use === instead of == when comparing strings in TypeScript.
Suggested fix:
+ if (data.code && data.code === 'duplicate_external_account') {📝 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.
| if (data.code && data.code == 'duplicate_external_account') { | |
| if (data.code && data.code === 'duplicate_external_account') { |
There was a problem hiding this comment.
nit: this is a nitpick and wont change anth here, but generally good advice
There was a problem hiding this comment.
@Hugo0, understood. Thank you for your feedback.
(_/_)/(｡•́︿•̀｡)
| 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 | ||
| } |
There was a problem hiding this comment.
Consistent error handling and return types
The function createExternalAccount is expected to return a Promise<interfaces.IResponse>. However, when an error occurs, the function throws an exception instead of returning an IResponse object with success: false. This inconsistency can lead to unexpected behavior for callers expecting a consistent response structure.
Suggested fix:
Modify both the error handling within the if (!response.ok) block and the catch block to return a standardized error response. For example:
+ let errorData = await response.json();
if (errorData.code && errorData.code === 'duplicate_external_account') {
// Return structured error response
return {
success: false,
data: errorData,
} as interfaces.IResponse;
}
- throw new Error('Failed to create external account')
+ return {
+ success: false,
+ data: { message: 'Failed to create external account', details: errorData },
+ } as interfaces.IResponse;
}
...
} catch (error) {
console.error('Error:', error);
- throw new Error(`Failed to create external account. Error: ${error}`);
+ return {
+ success: false,
+ data: { message: `Failed to create external account. Error: ${error}` },
+ } as interfaces.IResponse;
}This ensures that the function consistently returns an IResponse object regardless of success or failure.
Committable suggestion was skipped due to low confidence.
Ensure data is defined before accessing properties
If response.json() fails or returns undefined, accessing data.code could result in a runtime error. Ensure that data is properly defined and handle cases where parsing the JSON might fail.
Suggested fix:
+ let errorData;
+ try {
+ errorData = await response.json();
+ } catch (parseError) {
+ console.error('Error parsing JSON:', parseError);
+ errorData = { message: 'Unknown error occurred' };
+ }
+ if (errorData.code && errorData.code === 'duplicate_external_account') {
// Return structured error response
return {
success: false,
- data
+ data: errorData,
} as interfaces.IResponse;
}Committable suggestion was skipped due to low confidence.
| const data = await response.json() | ||
|
|
There was a problem hiding this comment.
Check response.ok before parsing JSON
Calling await response.json() before verifying response.ok can lead to runtime errors if the response body is empty or not valid JSON. It's safer to check response.ok before attempting to parse the response body.
Suggested fix:
- const data = await response.json()
+ let data;
+ if (!response.ok) {
+ // Handle error without parsing JSON
+ } else {
+ data = await response.json();
+ }Alternatively, parse the JSON within the conditional blocks to ensure data is only accessed when the response is OK.
Committable suggestion was skipped due to low confidence.
| if (!response.success) { | ||
| if (response.data && response.data.code == 'duplicate_external_account' ) { | ||
| // bridge account already exists for this IBAN | ||
| const errorMessage = 'An external account with the same information has already been added for this customer' | ||
| throw new Error(errorMessage) | ||
|
|
||
| } | ||
| throw new Error('Creating Bridge account failed') | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use strict equality for error code comparison
In line 185, consider using strict equality (===) when comparing response.data.code to 'duplicate_external_account' to avoid unexpected type coercion and adhere to best practices.
Apply this diff to make the change:
- if (response.data && response.data.code == 'duplicate_external_account' ) {
+ if (response.data && response.data.code === 'duplicate_external_account' ) {📝 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.
| if (!response.success) { | |
| if (response.data && response.data.code == 'duplicate_external_account' ) { | |
| // bridge account already exists for this IBAN | |
| const errorMessage = 'An external account with the same information has already been added for this customer' | |
| throw new Error(errorMessage) | |
| } | |
| throw new Error('Creating Bridge account failed') | |
| } | |
| if (!response.success) { | |
| if (response.data && response.data.code === 'duplicate_external_account' ) { | |
| // bridge account already exists for this IBAN | |
| const errorMessage = 'An external account with the same information has already been added for this customer' | |
| throw new Error(errorMessage) | |
| } | |
| throw new Error('Creating Bridge account failed') | |
| } |
| } catch (error) { | ||
| console.error('Error during the submission process:', error) | ||
| setErrorState({ showError: true, errorMessage: 'An error occurred. Please try again later' }) | ||
| setErrorState({ showError: true, errorMessage: String(error)}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error messaging by using error.message
When setting the error message in setErrorState, consider using error.message instead of String(error) to provide a cleaner and more user-friendly error message.
Apply this diff to make the change:
- setErrorState({ showError: true, errorMessage: String(error)})
+ setErrorState({ showError: true, errorMessage: error.message})Committable suggestion was skipped due to low confidence.
| // message: 'An external account with the same information has already been added for this customer' | ||
| // } | ||
| // | ||
| // TODO: standardize error responses across backend |
| const data = await response.json() | ||
|
|
||
| if (!response.ok) { | ||
| if (data.code && data.code == 'duplicate_external_account') { |
There was a problem hiding this comment.
nit: this is a nitpick and wont change anth here, but generally good advice
Also adds example of Respone interface type