chore: Added sentry call for 500 API errors #3413
Conversation
WalkthroughThis pull request enhances error handling across multiple components by integrating Sentry for logging API errors. The Changes
Suggested Reviewers
Possibly Related PRs
Poem
✨ Finishing Touches
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: 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, { |
| @@ -153,6 +154,7 @@ export class HttpConfigInterceptor implements HttpInterceptor { | |||
| return next.handle(request).pipe( | |||
| catchError((error) => { | |||
| if (error instanceof HttpErrorResponse) { | |||
| this.handleSentryError(error, request); | |||
There was a problem hiding this comment.
| this.handleSentryError(error, request); | |
| if (error.status >= 500) { | |
| this.sendErrorToSentry(error, request); | |
| } |
There was a problem hiding this comment.
this condition is considered by sendErrorToSentry method itself.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/app/core/interceptors/httpInterceptor.ts(3 hunks)src/app/fyle/my-view-report/my-view-report.page.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/app/core/interceptors/httpInterceptor.ts (1)
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.534Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (3)
src/app/core/interceptors/httpInterceptor.ts (2)
25-25: Vanakkam, the Sentry import is well done!
This is a great move to capture errors for monitoring, thalaiva!
183-204:⚠️ Potential issueSuperb approach to log 500 errors—just ensure we mask any secret info.
You're capturing the key details, but remember to hide or mask sensitive data inresponseDataor anywhere else. This keeps your app safe, anna!private trackErrorInSentry(error: HttpErrorResponse, request: HttpRequest<string>): void { if (error.status >= 500) { const errorObject = new Error(\`API \${error.status} Error: \${error.message || 'Server error'}\`); + // Example of sanitizing response data + const sanitizedErrorData = this.sanitizeErrorData(error.error); + Object.assign(errorObject, { status: error.status, statusText: error.statusText, name: \`API Error \${error.status} : \${error.url?.split('?')[0]}\`, }); Sentry.captureException(errorObject, { tags: { errorType: \`API_\${error.status}\`, apiEndpoint: error.url, }, extra: { requestUrl: request.url, - responseData: error.error, + responseData: sanitizedErrorData, }, }); } }Likely invalid or redundant comment.
src/app/fyle/my-view-report/my-view-report.page.ts (1)
457-464: 🛠️ Refactor suggestionAyya, we need to handle errors for report submission too!
Right now, we only see a success flow in.subscribe(). If there's an error, the user won't be informed, da. Consider adding an error callback to keep them in the loop:.subscribe( () => { this.router.navigate(['/', 'enterprise', 'my_reports']); const message = 'Report submitted successfully.'; this.matSnackBar.openFromComponent(ToastMessageComponent, { ...this.snackbarProperties.setSnackbarProperties('success', { message }), panelClass: ['msb-success-with-camera-icon'], }); this.trackingService.showToastMessage({ ToastContent: message }); + }, + (err) => { + const errorMessage = 'Unable to submit report. Please try again.'; + this.matSnackBar.openFromComponent(ToastMessageComponent, { + ...this.snackbarProperties.setSnackbarProperties('failure', { message: errorMessage }), + panelClass: ['msb-failure'], + }); + this.trackingService.showToastMessage({ ToastContent: errorMessage }); } );Likely invalid or redundant comment.
| @@ -153,6 +154,7 @@ export class HttpConfigInterceptor implements HttpInterceptor { | |||
| return next.handle(request).pipe( | |||
| catchError((error) => { | |||
| if (error instanceof HttpErrorResponse) { | |||
| this.trackErrorInSentry(error, request); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Aiyyo, please sanitize sensitive data before calling trackErrorInSentry.
My friend, always ensure we don't pass confidential user details or tokens directly to Sentry. Consider sanitizing relevant headers, body, or error responses here.
|
Clickup
https://app.clickup.com/t/86cxkwzq3
Summary by CodeRabbit
New Features
Bug Fixes
Chores