WEB-826: Improve null-safety in ErrorHandlerInterceptor#3340
WEB-826: Improve null-safety in ErrorHandlerInterceptor#3340tkshsbcue wants to merge 2 commits intoopenMF:devfrom
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Error Handler Refactor src/app/core/http/error-handler.interceptor.ts |
Use const error = response?.error and optional chaining (error?.errors?.[0]). Derive errorMessage via error?.defaultUserMessage → error?.developerMessage → response?.message → 'Unknown error'. Adjust logging to use `response?.message |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
- IOhacker
- alberto-art3ch
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately summarizes the main change: improving null-safety in the ErrorHandlerInterceptor through optional chaining, which is exactly what the changeset implements. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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 help to get the list of available commands and usage tips.
|
Run npx prettier |
Safely access response.error with optional chaining; honor root error.defaultUserMessage before developerMessage. Handles network errors and non-JSON responses without crashing. Made-with: Cursor
458bf45 to
2cf5cf2
Compare
|
@IOhacker fixed! |
| } | ||
| const error = response?.error; | ||
| let errorMessage = | ||
| error?.defaultUserMessage || error?.developerMessage || response?.message || 'Unknown error'; |
There was a problem hiding this comment.
'Unknown error'
is hard-coded. If the application supports translations (which seems likely given other alert messages), it might be better to route this through the translation service or a shared constant to keep error messaging consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/core/http/error-handler.interceptor.ts`:
- Around line 48-51: The current user-facing fallback chain incorrectly includes
the Angular transport string (response?.message) via the errorMessage
initialization; remove response?.message from the user-facing chain and only use
backend fields (error?.defaultUserMessage, error?.developerMessage and any
error.errors[0] values) to build errorMessage so the later status-specific
branches (400/403/404 handling) can still override when the backend didn't
return structured errors; keep the original transport string (response?.message
or the HttpErrorResponse.message) for logging only (e.g., logMessage or when
calling logger.error) while ensuring variables referenced are response, error,
errorMessage and error.errors[0] so you can locate the change in
error-handler.interceptor.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9467a213-d6d1-4516-b807-a192b1bcb2e9
📒 Files selected for processing (1)
src/app/core/http/error-handler.interceptor.ts
| errorMessage = response.error.errors[0].defaultUserMessage || response.error.errors[0].developerMessage; | ||
| } | ||
| const error = response?.error; | ||
| let errorMessage = |
There was a problem hiding this comment.
In the previous implementation, only the defaultUserMessage from response.error.errors[0] was used.
In the new implementation, error?.defaultUserMessage is used first.
That changes the order of which message will be used to display to the user.
If this change was intentional, it might be worth mentioning this in the PR description so reviewers understand the reasoning.
Remove response?.message from user-facing chain; keep transport string for logging only (logMessage). Made-with: Cursor
| if (response.error.errors[0]) { | ||
| errorMessage = response.error.errors[0].defaultUserMessage || response.error.errors[0].developerMessage; | ||
| } | ||
| const error = response?.error; |
There was a problem hiding this comment.
response is typed as HttpErrorResponse and should never be null or undefined, so optional chaining here may not be necessary.
This could be simplified to const error = response.error;.
|
@tkshsbcue Could you please squash the commits into a single commit |
Description:
During network failures, CORS issues, or non-JSON server responses, response.error can be null or undefined. The interceptor directly accesses properties like developerMessage and errors, which can cause runtime errors.
Fix:
Use optional chaining and safe fallbacks when accessing response.error fields so the interceptor handles all error shapes gracefully.
ticket -> https://mifosforge.jira.com/browse/WEB-826?atlOrigin=eyJpIjoiMmRlZWE0NzNjNDkyNDQ0ZmJlMmRjMWYzMGE3NmJlZDkiLCJwIjoiaiJ9
Summary by CodeRabbit