fix: resolve duplicate error messages on login failure#263
fix: resolve duplicate error messages on login failure#26323bsm038-Mahi wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughUpdates dependency versions, adds two ABdm facility API methods, centralizes and normalizes login error handling, makes session-storage access null-safe, refactors HTTP interceptor error propagation, removes a local environment property, and adds TypeScript path aliases for Common-UI. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
|
23bsm038-Mahi
changed the title
|
Hi @maintainers, Could you please review and approve the workflows? Thanks! |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/app-modules/core/services/http-interceptor.service.ts (1)
64-65:⚠️ Potential issue | 🔴 CriticalCritical bug: URL pattern match will never succeed.
req.url.toLowerCase()converts the URL to lowercase, but then checks for'user/userAuthenticate'which contains uppercase letters. This condition will never be true.🐛 Proposed fix
const isLoginRequest = - req.url && req.url.toLowerCase().includes('user/userAuthenticate'); + req.url && req.url.toLowerCase().includes('user/userauthenticate');Or use a case-insensitive approach:
const isLoginRequest = - req.url && req.url.toLowerCase().includes('user/userAuthenticate'); + req.url && req.url.includes('user/userAuthenticate');🤖 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 64 - 65, The isLoginRequest check in HttpInterceptorService uses req.url.toLowerCase() but compares against a mixed-case literal 'user/userAuthenticate', so the includes() will never match; update the comparison in the isLoginRequest assignment (referenced variable isLoginRequest and req.url) to use a lowercase literal (e.g., includes('user/userauthenticate')) or perform a case-insensitive match (e.g., use a RegExp with the i flag) so the URL check works reliably.tsconfig.json (1)
19-35:⚠️ Potential issue | 🟡 MinorDuplicate
noImplicitReturnscompiler option.
noImplicitReturns: falseappears at both line 19 and line 34. The duplicate should be removed.🧹 Proposed fix
"sourceMap": true, "declaration": false, "downlevelIteration": true, "experimentalDecorators": true, "moduleResolution": "node", "importHelpers": true, "target": "es5", "module": "ES2022", "useDefineForClassFields": false, "lib": [ "ES2022", "dom" - ], - "noImplicitReturns": false, - + ] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tsconfig.json` around lines 19 - 35, The tsconfig has a duplicate compiler option key "noImplicitReturns" (appearing in both the "noImplicitReturns": false entries); remove the redundant entry so the option appears only once, keeping the desired value (false) and preserving valid JSON formatting around the remaining properties (ensure commas/brackets stay correct) — locate the duplicate "noImplicitReturns" keys in the shown tsconfig block and delete the extra occurrence.
🧹 Nitpick comments (3)
src/app/app-modules/core/services/http-interceptor.service.ts (1)
156-158: Dead code:isLoginRequestmethod is never used.This private method duplicates the inline check at lines 64-65 but is never called. Per static analysis hint, if kept, it should use
.includes()instead of.indexOf().♻️ Options
Option 1: Remove the dead code
- private isLoginRequest(url: string): boolean { - return url.indexOf('user/userAuthenticate') >= 0; - } -Option 2: Use this method and fix both issues
- private isLoginRequest(url: string): boolean { - return url.indexOf('user/userAuthenticate') >= 0; - } + private isLoginRequest(url: string): boolean { + return url.includes('user/userAuthenticate'); + }Then replace line 64-65 with a call to this method.
🤖 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 156 - 158, Remove the dead private method isLoginRequest or, preferably, reuse and fix it: replace the inline login check (the existing direct 'user/userAuthenticate' indexOf usage) with a call to isLoginRequest(url), and update isLoginRequest to use url.includes('user/userAuthenticate') instead of indexOf; ensure the method remains private and returns a boolean so its signature matches existing usage.src/app/user-login/login/login.component.ts (1)
210-265: Excessive nesting depth (>4 levels) flagged by static analysis.The deeply nested subscribe callbacks make this code hard to maintain. Consider refactoring using RxJS operators like
switchMapandcatchErrorto flatten the structure.Also, lines 226-228 and 235-238 duplicate the
sessionstorage.setItem('loginDataResponse', ...)call.♻️ Suggested approach using RxJS operators
// Example refactor approach (pseudocode) this.authService.userLogoutPreviousSession(this.loginForm.controls.userName.value) .pipe( filter((res: any) => { if (res.statusCode !== 200) { this.showLoginError(res); return false; } return true; }), switchMap(() => this.authService.login( this.loginForm.controls.userName.value, encryptPassword, true, this.enableCaptcha ? this.captchaToken : undefined, )), catchError((error) => { this.showLoginError(error); return EMPTY; }) ) .subscribe((userLoggedIn: any) => { // Handle success case });This flattens the nesting and consolidates error handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/user-login/login/login.component.ts` around lines 210 - 265, Refactor the deep nested subscribes by replacing the userLogoutPreviousSession(...).subscribe(...).subscribe(...) pattern with an RxJS pipe: call authService.userLogoutPreviousSession(...) and pipe through filter or mergeMap/switchMap to authService.login(...), use tap to handle side-effects (trackingService.setUserId, sessionstorage.setItem, authService.sessionExpiredHandled, getServicesAuthdetails) and catchError to centralize error handling that calls showLoginError and resetCaptcha; remove the duplicated sessionstorage.setItem('loginDataResponse', ...) so it is set only once inside the success tap before calling getServicesAuthdetails, and keep the confirmationService.alert call in the branch where previlegeObj[0] is missing.src/app/user-login/service/service.component.ts (1)
185-188: Defensive null-safety is reasonable, but pattern differs from other files.The
?? '{}'fallback works correctly, butdoctor-worklist.component.ts(line 121) uses?? 'null'. Consider aligning the patterns for consistency across the codebase.Based on learnings, the loginData is expected to be available at this point, so this is purely defensive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/user-login/service/service.component.ts` around lines 185 - 188, Align this file with the pattern used elsewhere: change the fallback from '{}' to 'null' when reading loginData (the variable loginData in service.component.ts) and update subsequent access to be safe for a null parse result by using optional chaining on parsedLoginData (i.e. use parsedLoginData?.designation?.designationName instead of parsedLoginData.designation?.designationName) so JSON.parse('null') won't cause a property access error.
🤖 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 112-114: The set-password component's error handler assumes
error.errorMessage exists but the interceptor (HttpInterceptorService) can emit
either error.error or the raw HttpErrorResponse, so add a defensive extractor
(e.g., a private getErrorMessage(error: any): string in SetPasswordComponent)
that checks error?.errorMessage, then error?.error?.errorMessage, and falls back
to a default message; replace direct access to error.errorMessage in the
component's subscription/error handler with a call to this helper to safely
derive the displayed message.
In `@src/app/user-login/login/login.component.ts`:
- Around line 226-238: Remove the duplicate sessionStorage write for
'loginDataResponse' by deleting the redundant call to
this.sessionstorage.setItem('loginDataResponse',
JSON.stringify(userLoggedIn.data)); so that only one storage write remains (keep
the single call that logically fits alongside
trackingService.setUserId(userLoggedIn.data.userID) and
this.authService.sessionExpiredHandled = false). Ensure the remaining call uses
JSON.stringify(userLoggedIn.data) and keep the surrounding statements unchanged.
---
Outside diff comments:
In `@src/app/app-modules/core/services/http-interceptor.service.ts`:
- Around line 64-65: The isLoginRequest check in HttpInterceptorService uses
req.url.toLowerCase() but compares against a mixed-case literal
'user/userAuthenticate', so the includes() will never match; update the
comparison in the isLoginRequest assignment (referenced variable isLoginRequest
and req.url) to use a lowercase literal (e.g.,
includes('user/userauthenticate')) or perform a case-insensitive match (e.g.,
use a RegExp with the i flag) so the URL check works reliably.
In `@tsconfig.json`:
- Around line 19-35: The tsconfig has a duplicate compiler option key
"noImplicitReturns" (appearing in both the "noImplicitReturns": false entries);
remove the redundant entry so the option appears only once, keeping the desired
value (false) and preserving valid JSON formatting around the remaining
properties (ensure commas/brackets stay correct) — locate the duplicate
"noImplicitReturns" keys in the shown tsconfig block and delete the extra
occurrence.
---
Nitpick comments:
In `@src/app/app-modules/core/services/http-interceptor.service.ts`:
- Around line 156-158: Remove the dead private method isLoginRequest or,
preferably, reuse and fix it: replace the inline login check (the existing
direct 'user/userAuthenticate' indexOf usage) with a call to
isLoginRequest(url), and update isLoginRequest to use
url.includes('user/userAuthenticate') instead of indexOf; ensure the method
remains private and returns a boolean so its signature matches existing usage.
In `@src/app/user-login/login/login.component.ts`:
- Around line 210-265: Refactor the deep nested subscribes by replacing the
userLogoutPreviousSession(...).subscribe(...).subscribe(...) pattern with an
RxJS pipe: call authService.userLogoutPreviousSession(...) and pipe through
filter or mergeMap/switchMap to authService.login(...), use tap to handle
side-effects (trackingService.setUserId, sessionstorage.setItem,
authService.sessionExpiredHandled, getServicesAuthdetails) and catchError to
centralize error handling that calls showLoginError and resetCaptcha; remove the
duplicated sessionstorage.setItem('loginDataResponse', ...) so it is set only
once inside the success tap before calling getServicesAuthdetails, and keep the
confirmationService.alert call in the branch where previlegeObj[0] is missing.
In `@src/app/user-login/service/service.component.ts`:
- Around line 185-188: Align this file with the pattern used elsewhere: change
the fallback from '{}' to 'null' when reading loginData (the variable loginData
in service.component.ts) and update subsequent access to be safe for a null
parse result by using optional chaining on parsedLoginData (i.e. use
parsedLoginData?.designation?.designationName instead of
parsedLoginData.designation?.designationName) so JSON.parse('null') won't cause
a property access error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1176228-d121-4387-9448-626772255f40
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonsrc/app/app-modules/core/services/http-interceptor.service.tssrc/app/app-modules/nurse-doctor/doctor-worklist/doctor-worklist.component.tssrc/app/app-modules/registrar/shared/services/registrar.service.tssrc/app/user-login/login/login.component.tssrc/app/user-login/service/service.component.tssrc/environments/environment.local.tstsconfig.json
💤 Files with no reviewable changes (1)
- src/environments/environment.local.ts
| if (isLoginRequest) { | ||
| return throwError(() => error.error || error); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find error handlers that access errorMessage directly on the error object
rg -n --type=ts -C2 '\.subscribe\s*\(' | rg -A5 'error.*errorMessage'Repository: PSMRI/HWC-UI
Length of output: 38
🏁 Script executed:
# First, let's check if set-password.component.ts exists and examine lines 138-141
fd -t f 'set-password.component.ts'Repository: PSMRI/HWC-UI
Length of output: 175
🏁 Script executed:
# Search for errorMessage references across the codebase
rg -n 'errorMessage' --type=ts -B2 -A2Repository: PSMRI/HWC-UI
Length of output: 50368
🏁 Script executed:
# Look at the http-interceptor.service.ts file to understand the error handling context
fd -t f 'http-interceptor.service.ts'Repository: PSMRI/HWC-UI
Length of output: 117
🏁 Script executed:
# Read set-password.component.ts, focusing on lines 138-141
wc -l src/app/user-login/set-password/set-password.component.tsRepository: PSMRI/HWC-UI
Length of output: 117
🏁 Script executed:
# Read http-interceptor.service.ts around lines 112-114
wc -l src/app/app-modules/core/services/http-interceptor.service.tsRepository: PSMRI/HWC-UI
Length of output: 121
🏁 Script executed:
# Check set-password.component.ts around lines 138-141
sed -n '130,150p' src/app/user-login/set-password/set-password.component.tsRepository: PSMRI/HWC-UI
Length of output: 742
🏁 Script executed:
# Check http-interceptor.service.ts around lines 112-114 for context
sed -n '100,125p' src/app/app-modules/core/services/http-interceptor.service.tsRepository: PSMRI/HWC-UI
Length of output: 822
🏁 Script executed:
# Look at broader context in http-interceptor.service.ts to see how errors are handled
sed -n '1,50p' src/app/app-modules/core/services/http-interceptor.service.tsRepository: PSMRI/HWC-UI
Length of output: 1764
🏁 Script executed:
# Check the full error handling and what the API actually returns
sed -n '100,150p' src/app/app-modules/core/services/http-interceptor.service.tsRepository: PSMRI/HWC-UI
Length of output: 1476
🏁 Script executed:
# Let's see the full context of set-password.component.ts to understand the flow
cat -n src/app/user-login/set-password/set-password.component.tsRepository: PSMRI/HWC-UI
Length of output: 6105
🏁 Script executed:
# Search for any response/error object structure definitions or types
rg -n 'interface.*Error|interface.*Response' --type=ts | head -20Repository: PSMRI/HWC-UI
Length of output: 38
🏁 Script executed:
# Check if there's any error transformation or mapping happening elsewhere
rg -n 'error\.error.*errorMessage|errorMessage.*error\.error' --type=tsRepository: PSMRI/HWC-UI
Length of output: 504
🏁 Script executed:
# Look at actual error scenarios in the interceptor to see if error.error structure is defined anywhere
rg -n 'error\.error' src/app/app-modules/core/services/http-interceptor.service.ts -B2 -A2Repository: PSMRI/HWC-UI
Length of output: 588
🏁 Script executed:
# Let's look at the onSuccess/onError method more carefully to understand the error handling pattern
sed -n '160,200p' src/app/app-modules/core/services/http-interceptor.service.tsRepository: PSMRI/HWC-UI
Length of output: 1342
🏁 Script executed:
# Check the login.component.ts error handling to see how it deals with the error shape
sed -n '290,315p' src/app/user-login/login/login.component.tsRepository: PSMRI/HWC-UI
Length of output: 719
🏁 Script executed:
# Check how other components handle errors from setNewPassword or similar auth service calls
rg -n 'setNewPassword|authService' src/app/user-login/set-password/set-password.component.ts -B2 -A2Repository: PSMRI/HWC-UI
Length of output: 1049
🏁 Script executed:
# Look for any error response body structure in auth service
fd -t f 'auth.service.ts' | head -5Repository: PSMRI/HWC-UI
Length of output: 105
🏁 Script executed:
# Check if there are any HTTP error interceptor tests or docs that show expected error structure
fd -t f -i '*spec.ts' | xargs grep -l 'HttpInterceptor\|error\.error' 2>/dev/null | head -3Repository: PSMRI/HWC-UI
Length of output: 343
🏁 Script executed:
# Let's examine the auth.service.ts to see how setNewPassword is implemented
cat -n src/app/app-modules/core/services/auth.service.ts | grep -A 15 'setNewPassword'Repository: PSMRI/HWC-UI
Length of output: 654
🏁 Script executed:
# Check what the backend typically returns for errors by looking at successful responses structure
rg -n 'statusCode.*200\|errorMessage' src/app/app-modules/nurse-doctor/shared/mocks/mock-data.ts | head -10Repository: PSMRI/HWC-UI
Length of output: 38
🏁 Script executed:
# Let's verify what setNewPassword actually returns by checking if there are any test fixtures or API documentation
rg -n 'setNewPassword' --type=ts -B5 -A10 | head -40Repository: PSMRI/HWC-UI
Length of output: 3728
🏁 Script executed:
# Check how the error interceptor is being used in login component to see if there's a pattern difference
sed -n '280,320p' src/app/user-login/login/login.component.tsRepository: PSMRI/HWC-UI
Length of output: 1106
🏁 Script executed:
# Check if setNewPassword endpoint is marked as a login request in the interceptor
rg -n 'isLoginRequest\|setForgetPassword\|setNewPassword' src/app/app-modules/core/services/http-interceptor.service.ts -B3 -A3Repository: PSMRI/HWC-UI
Length of output: 38
🏁 Script executed:
# Look at what endpoints are considered login requests
sed -n '50,100p' src/app/app-modules/core/services/http-interceptor.service.tsRepository: PSMRI/HWC-UI
Length of output: 1664
🏁 Script executed:
# Let's check the full error handling in http-interceptor to see all error paths
sed -n '105,145p' src/app/app-modules/core/services/http-interceptor.service.tsRepository: PSMRI/HWC-UI
Length of output: 1242
🏁 Script executed:
# Let me verify if setNewPassword is marked as a login request
rg -n 'user/userAuthenticate\|setForgetPassword' src/app/app-modules/core/services/http-interceptor.service.tsRepository: PSMRI/HWC-UI
Length of output: 38
🏁 Script executed:
# Check the full intercept logic to understand both error paths
sed -n '59,75p' src/app/app-modules/core/services/http-interceptor.service.tsRepository: PSMRI/HWC-UI
Length of output: 553
Add defensive error extraction in set-password.component's error handler.
The error handler at line 140 directly accesses error.errorMessage, but the interceptor returns error.error || error without guaranteeing errorMessage exists. If error.error is falsy, the raw HttpErrorResponse is thrown (which lacks errorMessage). Use defensive extraction like login.component.ts (lines 295-315) to safely handle missing or nested error messages:
Example pattern:
private getErrorMessage(error: any): string {
if (error?.errorMessage && typeof error.errorMessage === 'string') {
return error.errorMessage;
}
if (error?.error?.errorMessage && typeof error.error.errorMessage === 'string') {
return error.error.errorMessage;
}
return 'Password update failed. Please try again.';
}🤖 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
112 - 114, The set-password component's error handler assumes error.errorMessage
exists but the interceptor (HttpInterceptorService) can emit either error.error
or the raw HttpErrorResponse, so add a defensive extractor (e.g., a private
getErrorMessage(error: any): string in SetPasswordComponent) that checks
error?.errorMessage, then error?.error?.errorMessage, and falls back to a
default message; replace direct access to error.errorMessage in the component's
subscription/error handler with a call to this helper to safely derive the
displayed message.
| this.sessionstorage.setItem( | ||
| 'loginDataResponse', | ||
| JSON.stringify(userLoggedIn.data), | ||
| ); | ||
| this.trackingService.setUserId( | ||
| userLoggedIn.data.userID, | ||
| ); | ||
| this.authService.sessionExpiredHandled = | ||
| false; | ||
| this.sessionstorage.setItem( | ||
| 'loginDataResponse', | ||
| JSON.stringify(userLoggedIn.data), | ||
| ); |
There was a problem hiding this comment.
Duplicate session storage write.
loginDataResponse is stored twice (lines 226-228 and 235-238). Remove one of the duplicates.
🧹 Proposed fix
if (userLoggedIn?.data?.previlegeObj[0]) {
- this.sessionstorage.setItem(
- 'loginDataResponse',
- JSON.stringify(userLoggedIn.data),
- );
this.trackingService.setUserId(
userLoggedIn.data.userID,
);
this.authService.sessionExpiredHandled =
false;
this.sessionstorage.setItem(
'loginDataResponse',
JSON.stringify(userLoggedIn.data),
);📝 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.
| this.sessionstorage.setItem( | |
| 'loginDataResponse', | |
| JSON.stringify(userLoggedIn.data), | |
| ); | |
| this.trackingService.setUserId( | |
| userLoggedIn.data.userID, | |
| ); | |
| this.authService.sessionExpiredHandled = | |
| false; | |
| this.sessionstorage.setItem( | |
| 'loginDataResponse', | |
| JSON.stringify(userLoggedIn.data), | |
| ); | |
| this.trackingService.setUserId( | |
| userLoggedIn.data.userID, | |
| ); | |
| this.authService.sessionExpiredHandled = | |
| false; | |
| this.sessionstorage.setItem( | |
| 'loginDataResponse', | |
| JSON.stringify(userLoggedIn.data), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/user-login/login/login.component.ts` around lines 226 - 238, Remove
the duplicate sessionStorage write for 'loginDataResponse' by deleting the
redundant call to this.sessionstorage.setItem('loginDataResponse',
JSON.stringify(userLoggedIn.data)); so that only one storage write remains (keep
the single call that logically fits alongside
trackingService.setUserId(userLoggedIn.data.userID) and
this.authService.sessionExpiredHandled = false). Ensure the remaining call uses
JSON.stringify(userLoggedIn.data) and keep the surrounding statements unchanged.
1 participant
🛠️ Summary
Fixes issue where multiple error messages were shown on login failure and improves error handling UX.
✅ Changes
🧪 Testing
📌 Impact
Summary by CodeRabbit
New Features
Bug Fixes
Chores