Enhance UI HTTP Interceptor to handle standardized API error codes#258
Open
ManikaKutiyal wants to merge 1 commit intoPSMRI:mainfrom
Open
Enhance UI HTTP Interceptor to handle standardized API error codes#258ManikaKutiyal wants to merge 1 commit intoPSMRI:mainfrom
ManikaKutiyal wants to merge 1 commit intoPSMRI:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThe HTTP interceptor service was refactored to improve error handling with centralized error message resolution, status-specific error alerts (400, 500), and revised session cleanup logic that only clears sessionStorage on 401/403 responses. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/app/app-modules/core/services/http-interceptor.service.ts (1)
123-127: Consolidate 401/403 cleanup in one place.Line 124 duplicates session cleanup already done by
handleSessionExpiry(...). Keeping cleanup in both places increases branching and makes auth-expiry behavior harder to reason about.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/app-modules/core/services/http-interceptor.service.ts` around lines 123 - 127, The 401/403 branch in the HTTP interceptor duplicates session clearing already performed by handleSessionExpiry; remove the direct calls to sessionStorage.clear() and this.sessionstorage.clear() from the error.status === 401 || error.status === 403 block and instead invoke the existing handleSessionExpiry(...) helper (or delegate to the same path that calls it) so all session cleanup is centralized; update the error handling in the interceptor (the block that checks error.status) to call handleSessionExpiry with the same arguments used elsewhere and remove the duplicate clearing logic referencing this.sessionstorage and sessionStorage.
🤖 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/app-modules/core/services/http-interceptor.service.ts`:
- Around line 186-187: The session-extension request in
http-interceptor.service.ts currently swallows errors in the subscribe callbacks
((res: any) => {}, (err: any) => {}); update the error callback used when
sending the extension request (the anonymous second callback in the subscribe)
to handle failures: log the error (use the service logger or console.error),
surface a user-facing notification via the NotificationService or AuthService
(e.g., show “Session extension failed, please sign in again”), and ensure the
Observable completes or triggers a safe fallback (optionally trigger
retry/backoff or call AuthService.logout()/refresh flow). Locate the subscribe
call in the interceptor’s session-extension logic (the anonymous success/error
callbacks) and replace the empty err handler with these actions so extension
failures are not silently ignored.
- Around line 91-96: The interceptor currently assumes error.error is an object;
update the error handling in http-interceptor.service.ts (around the
errorMessage assignment) to detect when error.error is a string, attempt to
JSON.parse it into an object, and then read the standardized field
(errorMessage) from that parsed object; if parsing fails or the field is
missing, fall back to existing checks (error.message or generic message). Ensure
parsing is wrapped in a try/catch to avoid throwing, and reference the existing
errorMessage variable and the error.error source so you change the minimal code
path.
- Around line 111-119: Guard the language lookups in the
confirmationService.alert calls to avoid crashes when currentLanguageSet is
uninitialized and flip the fallback order so localized strings are used when
available: replace direct accesses to
this.currentLanguageSet.internaleServerError and
this.currentLanguageSet.somethingWentWrong with safe optional chaining
(this.currentLanguageSet?.internaleServerError,
this.currentLanguageSet?.somethingWentWrong) and change the second alert's
message expression from errorMessage ||
this.currentLanguageSet.somethingWentWrong to
(this.currentLanguageSet?.somethingWentWrong || errorMessage) so the localized
fallback is used if present; update the two places inside the
http-interceptor.service.ts confirmationService.alert block accordingly.
- Line 129: The code uses the deprecated internal RxJS import and non-factory
form of throwError; update the import to the public API (import { throwError }
from 'rxjs') and change the call in the error path (in HttpInterceptorService,
e.g., inside intercept/handleError where it currently returns
throwError(error.error || error)) to the factory form return throwError(() =>
error.error || error) so it uses the RxJS 7.8+ public factory API.
---
Nitpick comments:
In `@src/app/app-modules/core/services/http-interceptor.service.ts`:
- Around line 123-127: The 401/403 branch in the HTTP interceptor duplicates
session clearing already performed by handleSessionExpiry; remove the direct
calls to sessionStorage.clear() and this.sessionstorage.clear() from the
error.status === 401 || error.status === 403 block and instead invoke the
existing handleSessionExpiry(...) helper (or delegate to the same path that
calls it) so all session cleanup is centralized; update the error handling in
the interceptor (the block that checks error.status) to call handleSessionExpiry
with the same arguments used elsewhere and remove the duplicate clearing logic
referencing this.sessionstorage and sessionStorage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 136911b7-9074-435c-8fdc-f40c1e3ecde5
📒 Files selected for processing (1)
src/app/app-modules/core/services/http-interceptor.service.ts
Comment on lines
+91
to
+96
| let errorMessage = 'Something went wrong. Please try again later.'; | ||
| if (error.error && error.error.errorMessage) { | ||
| errorMessage = error.error.errorMessage; | ||
| } else if (error.message) { | ||
| errorMessage = error.message; | ||
| } |
Contributor
There was a problem hiding this comment.
Parse string error bodies before resolving errorMessage.
Line 92 assumes error.error is already an object. If backend sends OutputResponse as a JSON string, this path falls back to generic text and drops the standardized message.
💡 Proposed fix
- let errorMessage = 'Something went wrong. Please try again later.';
- if (error.error && error.error.errorMessage) {
- errorMessage = error.error.errorMessage;
- } else if (error.message) {
- errorMessage = error.message;
- }
+ let parsedError: any = error.error;
+ if (typeof parsedError === 'string') {
+ try {
+ parsedError = JSON.parse(parsedError);
+ } catch {
+ // keep original string payload
+ }
+ }
+
+ const errorMessage =
+ parsedError?.errorMessage ||
+ parsedError?.message ||
+ error.message ||
+ 'Something went wrong. Please try again later.';📝 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.
Suggested change
| let errorMessage = 'Something went wrong. Please try again later.'; | |
| if (error.error && error.error.errorMessage) { | |
| errorMessage = error.error.errorMessage; | |
| } else if (error.message) { | |
| errorMessage = error.message; | |
| } | |
| let parsedError: any = error.error; | |
| if (typeof parsedError === 'string') { | |
| try { | |
| parsedError = JSON.parse(parsedError); | |
| } catch { | |
| // keep original string payload | |
| } | |
| } | |
| const errorMessage = | |
| parsedError?.errorMessage || | |
| parsedError?.message || | |
| error.message || | |
| 'Something went wrong. Please try again later.'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/app-modules/core/services/http-interceptor.service.ts` around lines
91 - 96, The interceptor currently assumes error.error is an object; update the
error handling in http-interceptor.service.ts (around the errorMessage
assignment) to detect when error.error is a string, attempt to JSON.parse it
into an object, and then read the standardized field (errorMessage) from that
parsed object; if parsing fails or the field is missing, fall back to existing
checks (error.message or generic message). Ensure parsing is wrapped in a
try/catch to avoid throwing, and reference the existing errorMessage variable
and the error.error source so you change the minimal code path.
Comment on lines
+111
to
+119
| this.confirmationService.alert( | ||
| this.currentLanguageSet.internaleServerError || errorMessage, | ||
| 'error' | ||
| ); | ||
| } else { | ||
| this.handleSessionExpiry( | ||
| error.message || | ||
| this.currentLanguageSet.somethingWentWrong || | ||
| 'Something went wrong. Please try again later.', | ||
| this.confirmationService.alert( | ||
| errorMessage || | ||
| this.currentLanguageSet.somethingWentWrong, | ||
| 'error' |
Contributor
There was a problem hiding this comment.
Guard language lookups to prevent secondary interceptor crashes.
At Line 112 and Line 118, direct access to this.currentLanguageSet.* can throw when language has not been initialized yet. Also, errorMessage || ... at Line 117 never reaches the localized fallback because errorMessage is always non-empty.
💡 Proposed fix
- this.confirmationService.alert(
- this.currentLanguageSet.internaleServerError || errorMessage,
- 'error'
- );
+ this.confirmationService.alert(
+ this.currentLanguageSet?.internaleServerError || errorMessage,
+ 'error'
+ );
} else {
- this.confirmationService.alert(
- errorMessage ||
- this.currentLanguageSet.somethingWentWrong,
- 'error'
- );
+ this.confirmationService.alert(
+ errorMessage || this.currentLanguageSet?.somethingWentWrong || 'Something went wrong. Please try again later.',
+ 'error'
+ );
}📝 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.
Suggested change
| this.confirmationService.alert( | |
| this.currentLanguageSet.internaleServerError || errorMessage, | |
| 'error' | |
| ); | |
| } else { | |
| this.handleSessionExpiry( | |
| error.message || | |
| this.currentLanguageSet.somethingWentWrong || | |
| 'Something went wrong. Please try again later.', | |
| this.confirmationService.alert( | |
| errorMessage || | |
| this.currentLanguageSet.somethingWentWrong, | |
| 'error' | |
| this.confirmationService.alert( | |
| this.currentLanguageSet?.internaleServerError || errorMessage, | |
| 'error' | |
| ); | |
| } else { | |
| this.confirmationService.alert( | |
| errorMessage || this.currentLanguageSet?.somethingWentWrong || 'Something went wrong. Please try again later.', | |
| 'error' | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/app-modules/core/services/http-interceptor.service.ts` around lines
111 - 119, Guard the language lookups in the confirmationService.alert calls to
avoid crashes when currentLanguageSet is uninitialized and flip the fallback
order so localized strings are used when available: replace direct accesses to
this.currentLanguageSet.internaleServerError and
this.currentLanguageSet.somethingWentWrong with safe optional chaining
(this.currentLanguageSet?.internaleServerError,
this.currentLanguageSet?.somethingWentWrong) and change the second alert's
message expression from errorMessage ||
this.currentLanguageSet.somethingWentWrong to
(this.currentLanguageSet?.somethingWentWrong || errorMessage) so the localized
fallback is used if present; update the two places inside the
http-interceptor.service.ts confirmationService.alert block accordingly.
| this.sessionstorage.clear(); | ||
| } | ||
|
|
||
| return throwError(error.error || error); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
jq -r '.dependencies.rxjs // .devDependencies.rxjs // "rxjs not declared"' package.jsonRepository: PSMRI/HWC-UI
Length of output: 62
🏁 Script executed:
rg -n "throwError" src/app/app-modules/core/services/http-interceptor.service.ts -B5 -A5Repository: PSMRI/HWC-UI
Length of output: 1098
🏁 Script executed:
cat -n src/app/app-modules/core/services/http-interceptor.service.ts | head -150 | tail -30Repository: PSMRI/HWC-UI
Length of output: 1148
Update RxJS throwError import and usage to public API factory form.
Line 15 imports throwError from the internal RxJS path (rxjs/internal/observable/throwError), and line 129 uses the deprecated non-factory form. With RxJS 7.8.0, throwError must be imported from the public API (rxjs) and called with a factory function.
Proposed fix
-import { throwError } from 'rxjs/internal/observable/throwError';
+import { throwError } from 'rxjs';
...
- return throwError(error.error || error);
+ return throwError(() => (error.error || error));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/app-modules/core/services/http-interceptor.service.ts` at line 129,
The code uses the deprecated internal RxJS import and non-factory form of
throwError; update the import to the public API (import { throwError } from
'rxjs') and change the call in the error path (in HttpInterceptorService, e.g.,
inside intercept/handleError where it currently returns throwError(error.error
|| error)) to the factory form return throwError(() => error.error || error) so
it uses the RxJS 7.8+ public factory API.
Comment on lines
+186
to
+187
| (res: any) => { }, | ||
| (err: any) => { }, |
Contributor
There was a problem hiding this comment.
Avoid silent failure when session extension request fails.
Line 187 drops extension errors completely. If extend-session fails, the user gets no signal and may be logged out unexpectedly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/app-modules/core/services/http-interceptor.service.ts` around lines
186 - 187, The session-extension request in http-interceptor.service.ts
currently swallows errors in the subscribe callbacks ((res: any) => {}, (err:
any) => {}); update the error callback used when sending the extension request
(the anonymous second callback in the subscribe) to handle failures: log the
error (use the service logger or console.error), surface a user-facing
notification via the NotificationService or AuthService (e.g., show “Session
extension failed, please sign in again”), and ensure the Observable completes or
triggers a safe fallback (optionally trigger retry/backoff or call
AuthService.logout()/refresh flow). Locate the subscribe call in the
interceptor’s session-extension logic (the anonymous success/error callbacks)
and replace the empty err handler with these actions so extension failures are
not silently ignored.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR updates the frontend HTTP interceptor to align with the standardized error handling implemented in the HWC-API (Issue #115). It ensures that the UI correctly interprets RESTful status codes (4xx/5xx) and extracts user-friendly error messages from the backend's
OutputResponseformat.Changes
interceptlogic to catch error responses.OutputResponseJSON from the error body and display the standardized error message to the user.400 Bad Request,401 Unauthorized,403 Forbidden, and500 Internal Server Error.Testing
Related
This PR is a companion to the backend changes in the
HWC-APIrepository under the same branch name:fix/standardize-error-handling-115.Summary by CodeRabbit