chore: Added sentry call for 500 API errors #3413
Conversation
WalkthroughSuperstar style, I'll break it down! 🌟 This pull request is all about leveling up error handling across multiple components. We're integrating Sentry error tracking with surgical precision, transforming how we capture and log errors in HTTP interceptors, report submission, and camera preview components. It's like adding a superhero shield to our application's error management - stylish and powerful! Changes
Suggested Reviewers
Suggested Labels
Poem
Possibly Related PRs
🪧 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/app/core/interceptors/httpInterceptor.ts(3 hunks)src/app/fyle/my-view-report/my-view-report.page.ts(2 hunks)src/app/shared/components/capture-receipt/camera-preview/camera-preview.component.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (1)
src/app/shared/components/capture-receipt/camera-preview/camera-preview.component.ts (1)
121-137: Fantastic! Error handling is sharp like a bladeYour enhancements to the
stopCameramethod gracefully handle errors. Well done ensuring exceptions are captured with Sentry.
| private handleSentryError(error: HttpErrorResponse, request: HttpRequest<string>): void { | ||
| if (error.status >= 500) { | ||
| const errorObject = new Error(`API ${error.status} Error: ${error.message || 'Server error'}`); | ||
|
|
||
| Object.assign(errorObject, { | ||
| status: error.status, | ||
| statusText: error.statusText, | ||
| }); | ||
|
|
||
| Sentry.captureException(errorObject, { | ||
| tags: { | ||
| errorType: 'API_500', | ||
| apiEndpoint: error.url, | ||
| }, | ||
| extra: { | ||
| requestUrl: request.url, | ||
| requestMethod: request.method, | ||
| requestHeaders: request.headers, | ||
| requestBody: request.body, | ||
| responseStatus: error.status, | ||
| responseStatusText: error.statusText, | ||
| responseData: error.error, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Safety is paramount! Sanitize data before sending to Sentry
When capturing exceptions, ensure that sensitive information isn't sent to Sentry. Let's sanitize request.headers, request.body, and error.error to protect user data.
Consider modifying your code like this:
+ // Sanitize sensitive data
+ const sanitizedHeaders = this.sanitizeHeaders(request.headers);
+ const sanitizedBody = this.sanitizeBody(request.body);
+
Sentry.captureException(errorObject, {
tags: {
errorType: 'API_500',
apiEndpoint: error.url,
},
extra: {
requestUrl: request.url,
requestMethod: request.method,
- requestHeaders: request.headers,
- requestBody: request.body,
+ requestHeaders: sanitizedHeaders,
+ requestBody: sanitizedBody,
responseStatus: error.status,
responseStatusText: error.statusText,
responseData: error.error,
},
});You'll need to implement the sanitizeHeaders and sanitizeBody methods to remove or mask sensitive data before logging.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
I guess this is important. Any API call may throw 500. We should not send headers and body without sanitizing it.
There was a problem hiding this comment.
@Chethan-Fyle Yo boss! You're absolutely right! Like a vigilant bodyguard protecting VIPs, we must protect sensitive data. I'll make sure to remember this for future reviews.
Would you like me to create a GitHub issue to track this security enhancement? I can help draft one with the necessary details while keeping sensitive implementation details private.
✏️ Learnings added
Learnt from: Chethan-Fyle
PR: fylein/fyle-mobile-app#3413
File: src/app/core/interceptors/httpInterceptor.ts:183-206
Timestamp: 2025-01-09T06:30:16.378Z
Learning: In Angular HTTP interceptors, when logging errors to Sentry (especially for 500 errors), always sanitize sensitive information from request headers, request body, and error responses before sending them to avoid potential data exposure. This applies to all API error logging scenarios.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Ok, i will update this
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| error: (error: HttpErrorResponse) => { | ||
| const errorObject = new Error(`Report Submission Failed: ${error.message || 'Unknown error'}`); | ||
| if (error.status) { | ||
| Object.assign(errorObject, { | ||
| status: error.status, | ||
| statusText: error.statusText, | ||
| }); | ||
| } | ||
|
|
||
| Sentry.addBreadcrumb({ | ||
| category: 'reports', | ||
| message: 'Failed to submit report', | ||
| level: 'error', | ||
| data: { | ||
| reportId: this.reportId, | ||
| errorStatus: error.status, | ||
| errorMessage: error.message, | ||
| }, | ||
| }); | ||
|
|
||
| Sentry.captureException(errorObject, { | ||
| tags: { | ||
| errorType: 'REPORT_SUBMISSION_FAILED', | ||
| reportId: this.reportId, | ||
| }, | ||
| extra: { | ||
| reportId: this.reportId, | ||
| errorResponse: error, | ||
| context: { | ||
| component: 'ReportSubmissionComponent', | ||
| action: 'submitReport', | ||
| }, |
There was a problem hiding this comment.
Heads up! Inform the user when submission fails
If the report submission fails, the user should be notified. Let's add a message to keep them informed.
Consider updating your error handler:
Sentry.captureException(errorObject, {
tags: {
errorType: 'REPORT_SUBMISSION_FAILED',
reportId: this.reportId,
},
extra: {
reportId: this.reportId,
errorResponse: error,
context: {
component: 'ReportSubmissionComponent',
action: 'submitReport',
},
},
});
+
+ // Notify the user about the error
+ const message = 'Report submission failed. Please try again.';
+ this.matSnackBar.openFromComponent(ToastMessageComponent, {
+ ...this.snackbarProperties.setSnackbarProperties('failure', { message }),
+ panelClass: ['msb-failure'],
+ });
+
+ // Track the error event
+ this.trackingService.showToastMessage({ ToastContent: message });Committable suggestion skipped: line range outside the PR's diff.
|
| error: (error: HttpErrorResponse) => { | ||
| const errorObject = new Error(`Report Submission Failed: ${error.message || 'Unknown error'}`); |
There was a problem hiding this comment.
why are we capturing sentry exceptions directly here? why is this not being captured globally?
There was a problem hiding this comment.
No we want some additional data for these cases for figure out why this is happening, not able to get info context from global exception
| this.cameraState = CameraState.STOPPED; | ||
| }, | ||
| error: (error) => { | ||
| Sentry.captureException(error, { |
Clickup
https://app.clickup.com/t/86cxkwzq3
Summary by CodeRabbit
New Features
Bug Fixes
Chores