fix: resolve Common-UI import errors and login duplication#72
fix: resolve Common-UI import errors and login duplication#7223bsm038-Mahi wants to merge 1 commit intoPSMRI:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces new feedback and tracking modules to the Common-UI library, adds a TypeScript path alias for Common-UI imports, and enhances the HTTP interceptor and login component to detect authentication requests and improve error handling during login flows. Changes
Sequence DiagramsequenceDiagram
actor User
participant LoginComponent
participant FormValidation
participant HttpInterceptor
participant AuthAPI
participant ErrorHandler
User->>LoginComponent: Click submit
LoginComponent->>LoginComponent: Check isSubmitting & form.valid
alt isSubmitting or form invalid
LoginComponent->>User: Disable submit button
else Valid & not submitting
LoginComponent->>LoginComponent: Set isSubmitting = true
LoginComponent->>FormValidation: Validate captcha
LoginComponent->>HttpInterceptor: POST to /user/userAuthenticate
HttpInterceptor->>HttpInterceptor: Detect login request (URL match)
HttpInterceptor->>AuthAPI: Forward request
AuthAPI->>HttpInterceptor: Return error
HttpInterceptor->>LoginComponent: throwError(error.error) [short-circuit]
LoginComponent->>ErrorHandler: handleLoginError(err)
ErrorHandler->>User: Display error dialog
LoginComponent->>LoginComponent: finalize: isSubmitting = false
LoginComponent->>User: Re-enable submit button
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
📝 Coding Plan
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 Tip CodeRabbit can use your project's `biome` configuration to improve the quality of JS/TS/CSS/JSON code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
|
|
Hi maintainers 👋 I have fixed the issue #124 by resolving Common-UI import errors and Angular dependency injection issues. The project now builds successfully without errors. Please review and let me know if any changes are required. Thank you! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/Common-UI/src/registrar/services/session-storage.service.ts`:
- Line 1: The project currently re-exports SessionStorageService from the app,
creating an app->library dependency; replace this temporary shim by adding a
concrete implementation of SessionStorageService inside the Common-UI library,
remove the re-export, and update the library's public exports to export the new
internal implementation instead of '../../../../../src/app/...'. Locate
references to SessionStorageService (the exported symbol) and create a matching
service file in the library (implement same API used by consumers), update index
barrel exports to export that local file, and run/build to ensure all imports
across the codebase now resolve to the Common-UI implementation.
In `@projects/Common-UI/src/tracking/amrit-tracking.service.ts`:
- Around line 15-19: The console.debug in amrit-tracking.service.ts is emitting
user-identifying data; remove this.userId from the logged payload in the
Tracking interaction call (replace the object with { fieldName, pageName }), or
alternatively wrap the console.debug call in a non-production gate (e.g., if
(!environment.production) console.debug(...)) so that userId is never logged to
the client console; update the line where console.debug('Tracking interaction',
{ userId: this.userId, fieldName, pageName }) is used accordingly.
In `@projects/Common-UI/src/tracking/tracking.module.ts`:
- Around line 6-10: The providers array in TrackingModule.forRoot() redundantly
re-declares AmritTrackingService which is already providedIn: 'root'; remove
AmritTrackingService from the returned providers array in
TrackingModule.forRoot() so the return object only sets ngModule: TrackingModule
(i.e., eliminate AmritTrackingService from the providers list in the forRoot()
implementation).
In `@src/app/app-modules/services/http-inteceptor/http-interceptor.service.ts`:
- Around line 100-102: The current branch for login requests unwraps the payload
with return throwError(error.error), losing HttpErrorResponse metadata; change
this to rethrow the full HttpErrorResponse so downstream can inspect
status/headers while still short-circuiting global handlers—i.e., where
isLoginRequest is true return the full error (use throwError(() => error) or
throwError(error) depending on your RxJS version) instead of error.error,
keeping the early-return behavior in the interceptor method that contains
isLoginRequest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99fafff5-03e3-4d70-88e9-665d4b52c19e
📒 Files selected for processing (9)
projects/Common-UI/src/feedback/feedback.component.tsprojects/Common-UI/src/feedback/feedback.module.tsprojects/Common-UI/src/registrar/services/session-storage.service.tsprojects/Common-UI/src/tracking/amrit-tracking.service.tsprojects/Common-UI/src/tracking/index.tsprojects/Common-UI/src/tracking/tracking.module.tssrc/app/app-modules/services/http-inteceptor/http-interceptor.service.tssrc/app/app-modules/user-login/login/login.component.tstsconfig.json
| @@ -0,0 +1 @@ | |||
| export { SessionStorageService } from '../../../../../src/app/app-modules/services/core/session-storage.service'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the source file exists and check for SessionStorageService usage
# Check if the source SessionStorageService exists
fd -t f "session-storage.service.ts" src/app/app-modules/services/core/
# Find all imports of SessionStorageService to assess migration impact
rg -n "from.*session-storage\.service" --type ts -C 2
# Check if there's already a SessionStorageService implementation in Common-UI
fd -t f "session-storage.service.ts" projects/Common-UI/Repository: PSMRI/ECD-UI
Length of output: 50368
Common-UI library temporarily depends on app implementation; this should be migrated per PR objectives.
The re-export works correctly and is consistently used throughout the application (all components import via 'Common-UI/src/registrar/services/session-storage.service'). However, this creates a problematic dependency where the library re-exports from the main application rather than providing its own implementation.
While this appears intentional as a temporary shim (per PR context), the proper solution is to move the actual SessionStorageService implementation into Common-UI itself. This aligns with the PR objective to "replace by the actual Common-UI library if available" and eliminates the library's dependency on application code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/Common-UI/src/registrar/services/session-storage.service.ts` at line
1, The project currently re-exports SessionStorageService from the app, creating
an app->library dependency; replace this temporary shim by adding a concrete
implementation of SessionStorageService inside the Common-UI library, remove the
re-export, and update the library's public exports to export the new internal
implementation instead of '../../../../../src/app/...'. Locate references to
SessionStorageService (the exported symbol) and create a matching service file
in the library (implement same API used by consumers), update index barrel
exports to export that local file, and run/build to ensure all imports across
the codebase now resolve to the Common-UI implementation.
| console.debug('Tracking interaction', { | ||
| userId: this.userId, | ||
| fieldName, | ||
| pageName, | ||
| }); |
There was a problem hiding this comment.
Avoid logging userId in client-side debug output.
Line 16 emits user-identifying data to the browser console, which can leak sensitive identifiers in shared devices/support captures. Remove userId from log payloads (or fully gate these logs to non-production).
🔧 Suggested minimal change
trackFieldInteraction(fieldName: string, pageName: string): void {
if (typeof console !== 'undefined' && console.debug) {
console.debug('Tracking interaction', {
- userId: this.userId,
fieldName,
pageName,
});
}
}📝 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.
| console.debug('Tracking interaction', { | |
| userId: this.userId, | |
| fieldName, | |
| pageName, | |
| }); | |
| console.debug('Tracking interaction', { | |
| fieldName, | |
| pageName, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/Common-UI/src/tracking/amrit-tracking.service.ts` around lines 15 -
19, The console.debug in amrit-tracking.service.ts is emitting user-identifying
data; remove this.userId from the logged payload in the Tracking interaction
call (replace the object with { fieldName, pageName }), or alternatively wrap
the console.debug call in a non-production gate (e.g., if
(!environment.production) console.debug(...)) so that userId is never logged to
the client console; update the line where console.debug('Tracking interaction',
{ userId: this.userId, fieldName, pageName }) is used accordingly.
| static forRoot(): ModuleWithProviders<TrackingModule> { | ||
| return { | ||
| ngModule: TrackingModule, | ||
| providers: [AmritTrackingService], | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current provisioning style and usage points
rg -n --type=ts -C2 "class AmritTrackingService|providedIn:\\s*'root'|providedIn:\\s*\"root\""
rg -n --type=ts -C2 "providers:\\s*\\[\\s*AmritTrackingService\\s*\\]"
rg -n --type=ts -C2 "TrackingModule\\.forRoot\\("Repository: PSMRI/ECD-UI
Length of output: 9267
Remove redundant AmritTrackingService from module providers.
AmritTrackingService is already provided at the root level via @Injectable({ providedIn: 'root' }). Listing it again in the module's forRoot() providers is redundant and violates Angular's tree-shaking guidelines. Remove the provider declaration.
♻️ Suggested refactor
export class TrackingModule {
static forRoot(): ModuleWithProviders<TrackingModule> {
return {
ngModule: TrackingModule,
- providers: [AmritTrackingService],
};
}
}📝 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.
| static forRoot(): ModuleWithProviders<TrackingModule> { | |
| return { | |
| ngModule: TrackingModule, | |
| providers: [AmritTrackingService], | |
| }; | |
| static forRoot(): ModuleWithProviders<TrackingModule> { | |
| return { | |
| ngModule: TrackingModule, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/Common-UI/src/tracking/tracking.module.ts` around lines 6 - 10, The
providers array in TrackingModule.forRoot() redundantly re-declares
AmritTrackingService which is already providedIn: 'root'; remove
AmritTrackingService from the returned providers array in
TrackingModule.forRoot() so the return object only sets ngModule: TrackingModule
(i.e., eliminate AmritTrackingService from the providers list in the forRoot()
implementation).
| if (isLoginRequest) { | ||
| return throwError(error.error); | ||
| } |
There was a problem hiding this comment.
Keep login errors as full HTTP errors instead of unwrapping payloads.
Line 101 rethrows only error.error, which drops HttpErrorResponse metadata (status, url, headers) and forces downstream shape-guessing. Keep the full error object here while still bypassing global redirect/session-clear logic for login requests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/app-modules/services/http-inteceptor/http-interceptor.service.ts`
around lines 100 - 102, The current branch for login requests unwraps the
payload with return throwError(error.error), losing HttpErrorResponse metadata;
change this to rethrow the full HttpErrorResponse so downstream can inspect
status/headers while still short-circuiting global handlers—i.e., where
isLoginRequest is true return the full error (use throwError(() => error) or
throwError(error) depending on your RxJS version) instead of error.error,
keeping the early-return behavior in the interceptor method that contains
isLoginRequest.
1 participant
Fixes #124
🛠️ Summary
This PR resolves build and runtime issues caused by missing Common-UI module dependencies.
✅ Changes Made
Common-UImodules🧪 Testing
📌 Notes
Summary by CodeRabbit
New Features
Bug Fixes
Chores