fix(auth-guard): remove unmanaged currentLangugae$ subscription in canActivate #337
fix(auth-guard): remove unmanaged currentLangugae$ subscription in canActivate #337VarshiniGunti wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe ChangesAuthGuard Simplification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 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/app-modules/core/services/auth-guard.service.ts`:
- Around line 1-2: This file is missing the mandatory AMRIT GPL-3.0 license
header; add the exact required license header block as the very first lines of
auth-guard.service.ts (before any imports such as the existing "import {
Injectable }..." and "import { CanActivate, Router }...") so the source file
complies with the project's header rule for src/**/*.{ts,html,scss,css}; ensure
the header is verbatim and precedes the rest of the file content.
🪄 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: 795ca94b-2726-460c-a4d9-fd2aab12c362
📒 Files selected for processing (1)
src/app/app-modules/core/services/auth-guard.service.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/app-modules/core/services/auth-guard.service.ts (1)
35-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
tap-only guard permits route activation on any truthy HTTP response — usemapto enforce authenticationThe guard returns an observable that emits the raw HTTP response object from
validateSessionKey(), not aboolean. Angular's router expectscanActivateto returnboolean | UrlTree; when receiving other types, it coerces via!!. SincevalidateSessionKey()emits a response object (truthy even on failure, e.g.,{ statusCode: 401 }), Angular interprets this astrueand allows navigation. Meanwhile,tapsimultaneously triggersrouter.navigate(['/login']), creating a race condition where the protected route briefly activates before redirect occurs.Fix: Use
mapto explicitly returnboolean(orUrlTreefor cleaner redirect handling):Proposed fix
-import { tap } from 'rxjs'; +import { map } from 'rxjs'; canActivate(route: any, state: any) { return this.auth.validateSessionKey().pipe( - tap((res: any) => { - if (!(res && res.statusCode === 200 && res.data)) { - this.router.navigate(['/login']); - } - }) + map((res: any) => { + if (res && res.statusCode === 200 && res.data) { + return true; + } + return this.router.createUrlTree(['/login']); + }) ); }🤖 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` around lines 35 - 41, The guard currently uses tap on validateSessionKey() which emits the raw HTTP response and causes routing races; change the pipeline in the canActivate method to use map (imported from rxjs/operators) to convert the response into a boolean or UrlTree: return map((res: any) => res && res.statusCode === 200 && res.data ? true : this.router.createUrlTree(['/login'])); reference validateSessionKey() and the canActivate implementation in auth-guard.service.ts and remove the router.navigate side-effect inside tap so the router receives an explicit boolean/UrlTree result.
🧹 Nitpick comments (1)
src/app/app-modules/core/services/auth-guard.service.ts (1)
34-34: 💤 Low value
routeandstateparameters should use proper Angular typesUsing
anyloses the type contract required by theCanActivateinterface. Import and useActivatedRouteSnapshotandRouterStateSnapshot.♻️ Proposed refactor
-import { CanActivate, Router } from '@angular/router'; +import { ActivatedRouteSnapshot, CanActivate, Router, RouterStateSnapshot } from '@angular/router'; - canActivate(route: any, state: any) { + canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot) {🤖 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 34, The canActivate method currently uses untyped parameters (route: any, state: any); import ActivatedRouteSnapshot and RouterStateSnapshot from `@angular/router` and change the signature of canActivate to accept (route: ActivatedRouteSnapshot, state: RouterStateSnapshot) and return the appropriate CanActivate-compatible type (e.g. boolean | UrlTree | Observable<boolean | UrlTree> | Promise<boolean | UrlTree>) so the method conforms to the CanActivate contract; update any internal references to route/state as needed.
🤖 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.
Outside diff comments:
In `@src/app/app-modules/core/services/auth-guard.service.ts`:
- Around line 35-41: The guard currently uses tap on validateSessionKey() which
emits the raw HTTP response and causes routing races; change the pipeline in the
canActivate method to use map (imported from rxjs/operators) to convert the
response into a boolean or UrlTree: return map((res: any) => res &&
res.statusCode === 200 && res.data ? true :
this.router.createUrlTree(['/login'])); reference validateSessionKey() and the
canActivate implementation in auth-guard.service.ts and remove the
router.navigate side-effect inside tap so the router receives an explicit
boolean/UrlTree result.
---
Nitpick comments:
In `@src/app/app-modules/core/services/auth-guard.service.ts`:
- Line 34: The canActivate method currently uses untyped parameters (route: any,
state: any); import ActivatedRouteSnapshot and RouterStateSnapshot from
`@angular/router` and change the signature of canActivate to accept (route:
ActivatedRouteSnapshot, state: RouterStateSnapshot) and return the appropriate
CanActivate-compatible type (e.g. boolean | UrlTree | Observable<boolean |
UrlTree> | Promise<boolean | UrlTree>) so the method conforms to the CanActivate
contract; update any internal references to route/state as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0b189e5-cbf1-44ab-abfd-1f21e621543d
📒 Files selected for processing (1)
src/app/app-modules/core/services/auth-guard.service.ts
|
1 participant
📋 Description
JIRA ID: N/A (Closes issue PSMRI/AMRIT#138)
This PR fixes a memory leak in
AuthGuard.canActivate()caused by an unmanaged subscription tocurrentLangugae$on every protected-route activation.Problem
AuthGuardis a singleton.currentLangugae$is backed byBehaviorSubjectand does not complete.current_language_set) was not used in guard logic.Change
currentLangugae$subscription fromcanActivate().current_language_setHttpServiceServiceinjection/importActivatedRouteinjection/importMotivation
Prevent unnecessary subscription accumulation and memory growth during repeated navigation, while keeping guard behavior unchanged.
✅ Type of Change
ℹ️ Additional Information
Validation performed
/service,/servicePoint,/registrar,/nurse-doctor,/lab,/pharmacist,/datasync)./logincurrentLangugae$subscriptions.No UI changes were introduced.
Summary by CodeRabbit