fix(ui): handle auth/session errors using HTTP semantics - #339
fix(ui): handle auth/session errors using HTTP semantics#339VarshiniGunti wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesAuthentication Error Handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/app-modules/core/services/auth-guard.service.ts`:
- Line 42: Update the AuthGuard session-validation flow around catchError and
LoginComponent.ngOnInit so failed or expired session credentials do not remain
marked as authenticated and trigger repeated validation errors. Handle
validation errors in the login initialization path or clear
sessionStorage.isAuthenticated only for confirmed invalid/expired sessions,
while preserving the flag for transient server failures.
In `@src/app/app-modules/login/login.component.ts`:
- Around line 206-212: Update the login error handler’s message selection around
confirmationService.alert to prioritize err?.error?.errorMessage, then map
err.status for HTTP 400, 401, 403, 404, 409, and 500 to the corresponding
product messages, preserving err?.message as appropriate. Use the existing
generic fallback only when no API or status-specific message applies, including
unmapped and network errors.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cccc5798-0926-4cd9-b50d-4fdf15f8bea6
📒 Files selected for processing (2)
src/app/app-modules/core/services/auth-guard.service.tssrc/app/app-modules/login/login.component.ts
| : this.router.createUrlTree(['/login']) | ||
| ) | ||
| ), | ||
| catchError(() => of(this.router.createUrlTree(['/login']))) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)src/app/(app-modules/core/services/auth-guard.service.ts|login/login.component.ts|.*login.*)\.ts$|(^|/)src/app/.*guard.*\.ts$' || true
echo "== auth guard outline and relevant contents =="
if [ -f src/app/app-modules/core/services/auth-guard.service.ts ]; then
ast-grep outline src/app/app-modules/core/services/auth-guard.service.ts --view expanded || true
wc -l src/app/app-modules/core/services/auth-guard.service.ts
cat -n src/app/app-modules/core/services/auth-guard.service.ts
fi
echo "== login component references =="
rg -n "validateSessionKey|isAuthenticated|authenticationToken|Subscription|Error|CatchError|catchError|ngOnInit" src/app -g '*.ts' || true
echo "== service validation details =="
rg -n "validateSessionKey|isAuthenticated|authenticationToken|isAuthenticated" src/app/src app Common-UI -g '*.ts' 2>/dev/null || trueRepository: PSMRI/MMU-UI
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== login component =="
wc -l src/app/app-modules/login/login.component.ts
sed -n '1,180p' src/app/app-modules/login/login.component.ts
echo "== login spec relevant lines =="
sed -n '1,180p' src/app/app-modules/login/login.component.spec.ts
echo "== focused auth/session references =="
rg -n "validateSessionKey|isAuthenticated|authenticationToken|catchError|Subscription|error" src/app/app-modules/login -g '*.ts' || true
rg -n "validateSessionKey" src/app -g '*.ts' || true
echo "== auth service files =="
git ls-files src/app | rg '(^|/)auth\.service\.ts$|auth-session|session|Auth' || true
for f in $(git ls-files src/app | rg 'auth\.service\.ts$|auth-session|session|Auth'); do
echo "--- $f"
wc -l "$f"
rg -n "validateSessionKey|isAuthenticated|authenticationToken" "$f" -C 4 || true
doneRepository: PSMRI/MMU-UI
Length of output: 10916
Handle session validation errors on login.
AuthGuard redirects to /login on validation failures while sessionStorage.isAuthenticated remains set. LoginComponent.ngOnInit() then subscribes to validateSessionKey() with no error handler, so the invalid session repeats the failing request and propagates an unhandled RxJS error. Clear the stale isAuthenticated state for failed/expired sessions, or handle the validation error in LoginComponent; do not clear credentials for transient server errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/app-modules/core/services/auth-guard.service.ts` at line 42, Update
the AuthGuard session-validation flow around catchError and
LoginComponent.ngOnInit so failed or expired session credentials do not remain
marked as authenticated and trigger repeated validation errors. Handle
validation errors in the login initialization path or clear
sessionStorage.isAuthenticated only for confirmed invalid/expired sessions,
while preserving the flag for transient server failures.
| const message = [err?.error?.errorMessage, err?.message].find( | ||
| msg => !!msg | ||
| ); | ||
| this.confirmationService.alert( | ||
| message || 'Login request failed. Please try again.', | ||
| 'error' | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files around login component:"
git ls-files | grep -E '(^|/)login\.component\.ts$|package\.json$|src/app/app-modules/login' | head -100
echo
echo "Login component excerpt:"
if [ -f src/app/app-modules/login/login.component.ts ]; then
nl -ba src/app/app-modules/login/login.component.ts | sed -n '1,280p'
else
echo "login.component.ts not found"
fi
echo
echo "Search for status-specific login message handling:"
rg -n "400|401|403|404|409|500|HttpErrorResponse|errorMessage|confirmationService\.alert|Login request failed" src/app/app-modules/login src || trueRepository: PSMRI/MMU-UI
Length of output: 459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Login component excerpt (wc + sed):"
wc -l src/app/app-modules/login/login.component.ts
sed -n '1,280p' src/app/app-modules/login/login.component.ts
echo
echo "Search for status-specific login handling:"
rg -n "400|401|403|404|409|500|HttpErrorResponse|errorMessage|confirmationService\.alert|Login request failed|status" src/app/app-modules/login src || true
echo
echo "Package script hints:"
python3 -c "
import json
from pathlib import Path
p=Path('package.json')
if p.exists():
d=json.loads(p.read_text())
for k,v in d.get('scripts',{}).items():
if 'lint' in k or 'test' in k or 'build' in k:
print(f'{k}: {v}')
"Repository: PSMRI/MMU-UI
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/app/app-modules/login/login.component.ts')
text = p.read_text()
print(f'file_exists={p.exists()} lines={text.count(chr(10))+1}')
for start,end in [(1,80),(180,230)]:
print(f'\n--- {start}-{end} ---')
for i,line in enumerate(text.splitlines(),1):
if start <= i <= end:
print(f'{i:4}: {line}')
print('\n--- targeted occurrences in login component ---')
for needle in ['confirmationService.alert', 'Login request failed', 'errorMessage', 'HttpErrorResponse', 'status', '400', '401', '403', '404', '409', '500']:
matches = []
for i,line in enumerate(text.splitlines(),1):
if needle in line:
matches.append(i)
print(f'{needle}: {matches[:40]}' if len(matches)<=40 else f'{needle}: {matches[:40]} ... count={len(matches)}')
PY
echo
echo "Login spec status/message snippets:"
python3 - <<'PY'
from pathlib import Path
p = Path('src/app/app-modules/login/login.component.spec.ts')
text = p.read_text()
for needle in ['Login request failed', 'errorMessage', 'HttpErrorResponse', 'status', '400', '401', '403', '404', '409', '500', 'confirmationService.alert']:
matches = [i for i,line in enumerate(text.splitlines(),1) if needle in line]
print(f'{needle}: first {matches[:10]} count={len(matches)} total_lines={text.count(chr(10))+1}')
PYRepository: PSMRI/MMU-UI
Length of output: 6101
Map HttpErrorResponse.status in the login error handler.
The error block at src/app/app-modules/login/login.component.ts:206-212 only reads err?.error?.errorMessage and err?.message. This means HTTP 400/401/403/404/409/500 login failures do not use status-specific product messages unless the API provides them in err.error.errorMessage. Keep any API-provided message first, then select messages by err.status, and use the generic fallback only for unmapped or network errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/app-modules/login/login.component.ts` around lines 206 - 212, Update
the login error handler’s message selection around confirmationService.alert to
prioritize err?.error?.errorMessage, then map err.status for HTTP 400, 401, 403,
404, 409, and 500 to the corresponding product messages, preserving err?.message
as appropriate. Use the existing generic fallback only when no API or
status-specific message applies, including unmapped and network errors.
2 participants
📋 Description
JIRA ID: N/A (GitHub issue #114)
This PR updates MMU-UI to correctly consume REST-style HTTP error responses (4xx/5xx) instead of relying on side-effect navigation or opaque error objects.
What changed
AuthGuard.canActivate()to return explicit router-compatible values:truefor valid session responseUrlTree('/login')for invalid/expired sessionrouter.navigate()flow withmap(...)return logic.catchError(...)fallback to route to login viaUrlTreeon HTTP errors.HttpErrorResponse.400,401,403,404,409,500errorMessagewhen available.Motivation
Issue #114 requires UI to handle real HTTP status codes once backend endpoints stop returning 200 for all outcomes.
This PR makes authentication/session and login flows robust against 4xx/5xx semantics and removes route activation race behavior.
✅ Type of Change
ℹ️ Additional Information
Validation performed
canActivatenow returnsboolean | UrlTreeand no longer depends onrouter.navigate()side effects.HttpErrorResponseand displays status-aware messages./loginconsistently through guard return values.Linked scope
Summary by CodeRabbit