Feature: added LF login #449
Conversation
Signed-off-by: Amol Sontakke <[email protected]>
Signed-off-by: Amol Sontakke <[email protected]>
Signed-off-by: Amol Sontakke <[email protected]>
WalkthroughThis update restructures authentication and dashboard navigation flows in the application. It removes the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppComponent
participant AuthDashboardComponent
participant StorageService
participant Router
participant AuthService
User->>AppComponent: Loads root path ('/')
AppComponent->>AuthDashboardComponent: Initializes component
AuthDashboardComponent->>StorageService: Retrieve PROJECT_ID, USER_ID, REDIRECT
AuthDashboardComponent->>Router: Navigate to /dashboard/project/:projectId/user/:userId?redirect=...
Router->>AuthService: (If needed) Check authentication
AuthService-->>Router: (If not authenticated) Trigger login
sequenceDiagram
participant User
participant ClaDashboardComponent
participant detectIncognito
participant AuthService
User->>ClaDashboardComponent: Loads dashboard
ClaDashboardComponent->>detectIncognito: Check for private browsing
detectIncognito-->>ClaDashboardComponent: Returns result
ClaDashboardComponent->>AuthService: Subscribe to loading/authenticated
AuthService-->>ClaDashboardComponent: Returns authentication status
ClaDashboardComponent->>AuthService: If not authenticated and not private, trigger login
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Amol Sontakke <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
src/app/modules/individual-contributor/container/individual-dashboard/individual-dashboard.component.html (1)
4-4: Same note as corporate dashboard – validate component interfaceDouble-check that
app-project-titlefetches its context from storage and has zero required inputs, otherwise this dashboard will fail to render silently.
🧹 Nitpick comments (4)
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.spec.ts (1)
22-24: Consider expanding test coverage.The current test only verifies component creation. For a new authentication component, consider adding tests for:
- Component initialization logic
- Navigation behavior
- Storage interaction
- Error handling scenarios
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.ts (1)
13-13: Fix spacing inconsistency in constructor parameters.- constructor(private router:Router, private storageService:StorageService) { } + constructor(private router: Router, private storageService: StorageService) { }src/app/shared/components/project-title/project-title.component.ts (1)
4-4: Consider using OnInit instead of AfterViewInit.Since this component doesn't manipulate view children or require DOM access,
OnInitwould be more appropriate thanAfterViewInit.-import { Component, EventEmitter, Output, AfterViewInit } from '@angular/core'; +import { Component, EventEmitter, Output, OnInit } from '@angular/core';-export class ProjectTitleComponent implements AfterViewInit { +export class ProjectTitleComponent implements OnInit {- ngAfterViewInit(): void { + ngOnInit(): void {Also applies to: 17-17, 31-31
src/app/modules/dashboard/container/cla-dashboard/cla-dashboard.component.ts (1)
71-71: Fix spacing in if statement.- if(this.hasPrivateWindow){ + if (this.hasPrivateWindow) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
package.json(1 hunks)src/app/app-routing.module.ts(1 hunks)src/app/app.component.ts(2 hunks)src/app/app.module.ts(1 hunks)src/app/modules/corporate-contributor/component/configure-cla-manager-modal/configure-cla-manager-modal.component.ts(0 hunks)src/app/modules/corporate-contributor/container/cla-request-authorization/cla-request-authorization.component.html(1 hunks)src/app/modules/corporate-contributor/container/corporate-dashboard/corporate-dashboard.component.html(0 hunks)src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.html(1 hunks)src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.spec.ts(1 hunks)src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.ts(1 hunks)src/app/modules/dashboard/container/cla-dashboard/cla-dashboard.component.html(5 hunks)src/app/modules/dashboard/container/cla-dashboard/cla-dashboard.component.ts(2 hunks)src/app/modules/dashboard/dashboard.module.ts(1 hunks)src/app/modules/individual-contributor/container/individual-dashboard/individual-dashboard.component.html(1 hunks)src/app/shared/components/alert/alert.component.scss(0 hunks)src/app/shared/components/auth/auth.component.html(0 hunks)src/app/shared/components/auth/auth.component.scss(0 hunks)src/app/shared/components/auth/auth.component.spec.ts(0 hunks)src/app/shared/components/auth/auth.component.ts(0 hunks)src/app/shared/components/page-not-found/page-not-found.component.html(1 hunks)src/app/shared/components/page-not-found/page-not-found.component.ts(2 hunks)src/app/shared/components/project-title/project-title.component.ts(6 hunks)src/app/shared/services/auth.service.ts(3 hunks)src/app/shared/shared.module.ts(0 hunks)
💤 Files with no reviewable changes (8)
- src/app/shared/components/alert/alert.component.scss
- src/app/shared/shared.module.ts
- src/app/shared/components/auth/auth.component.scss
- src/app/modules/corporate-contributor/component/configure-cla-manager-modal/configure-cla-manager-modal.component.ts
- src/app/shared/components/auth/auth.component.spec.ts
- src/app/shared/components/auth/auth.component.html
- src/app/modules/corporate-contributor/container/corporate-dashboard/corporate-dashboard.component.html
- src/app/shared/components/auth/auth.component.ts
🧰 Additional context used
🪛 HTMLHint (1.5.0)
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
src/app/modules/individual-contributor/container/individual-dashboard/individual-dashboard.component.html
[error] 4-4: Doctype must be declared before any non-comment content.
(doctype-first)
src/app/modules/corporate-contributor/container/cla-request-authorization/cla-request-authorization.component.html
[error] 4-4: Doctype must be declared before any non-comment content.
(doctype-first)
src/app/modules/dashboard/container/cla-dashboard/cla-dashboard.component.html
[error] 3-3: Doctype must be declared before any non-comment content.
(doctype-first)
🪛 Biome (1.9.4)
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.spec.ts
[error] 16-20: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🪛 GitHub Actions: License Header Check
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.spec.ts
[error] 1-1: Missing license header in source file.
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.ts
[error] 1-1: Missing license header in source file.
🪛 GitHub Actions: Snyk Scan NPM Dependencies
package.json
[error] 1-1: Snyk authentication error (SNYK-0005): Authentication credentials not recognized or user access not provisioned. Command 'snyk test --org=*** --file=package.json' failed with exit code 2.
🪛 GitHub Actions: Snyk Scan Edge NPM Dependencies
package.json
[error] 1-1: Snyk monitor failed: Could not find the specified file 'package.json'. Please check that it exists and try again. (SNYK-CLI-0000)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (16)
src/app/shared/components/page-not-found/page-not-found.component.ts (2)
5-6: LGTM!The imports are correctly added for the new storage-based redirect functionality.
17-19: LGTM!The constructor correctly injects the
StorageServicedependency.package.json (1)
55-66: Verify new dependency & monitor licensing/security before shippingThe only functional change here is the introduction of
detectincognitojs(^1.5.0) and the re-ordering of a couple of existing deps.
Before merging, please double-check:
- License compatibility (GPL vs MIT/Apache for the rest of the stack).
- Bundle impact – the lib ships transpiled ES5 + UMD; tree-shaking might not remove the whole thing.
- Any known supply-chain advisories (Snyk / OSV).
The current pipeline already fails on Snyk auth; until that is unblocked we have no automated gate on the above.
Consider running a one-offnpm audit --productionlocally or fixing the CI secret so Snyk resumes.src/app/app.module.ts (1)
35-35: LGTM – change keeps comma-style consistentThe trailing comma after
FormsModulematches the existing style in this array; no functional impact.src/app/modules/corporate-contributor/container/cla-request-authorization/cla-request-authorization.component.html (1)
4-4: Confirm<app-project-title>no longer expects inputsInputs
projectId/userIdwere removed across templates.
Please ensureProjectTitleComponent’s@Inputdecorators are actually gone; otherwise Angular will log warnings at runtime.src/app/app.component.ts (1)
6-7: LGTM! Clean dependency injection setup.The injection of
AuthServiceandActivatedRoutealigns well with the authentication refactoring. The imports and constructor changes look correct.Also applies to: 19-19
src/app/modules/dashboard/dashboard.module.ts (1)
11-11: LGTM! Proper module registration.The
AuthDashboardComponentis correctly imported and declared in the module following Angular conventions.Also applies to: 17-18
src/app/app-routing.module.ts (1)
12-12: LGTM! Routing refactoring looks correct.The change from redirecting to '/cla' to directly loading
AuthDashboardComponenton the root path aligns with the authentication flow refactoring. The import and route configuration are properly updated.Also applies to: 19-19
src/app/shared/services/auth.service.ts (3)
7-7: LGTM! Clean import removal.Removing unused
combineLatestimport improves code cleanliness.
128-128: Verify timeout reduction won't affect slower networks.The login timeout was reduced from 1500ms to 500ms, but the comment above (line 118-119) specifically mentions the timeout was increased to 1500ms for slower networks to avoid "Cannot read property 'querySelector' of null" errors.
This change could potentially reintroduce the original issue on slower networks. Please verify this reduction is safe or update the comment if the timeout is no longer needed.
147-150: LGTM! Consistent code formatting.The change to use block body syntax in the logout subscription callback improves code consistency.
src/app/modules/dashboard/container/cla-dashboard/cla-dashboard.component.html (3)
3-3: LGTM!The event handler name change from
onAPILoadtosetProjectis more descriptive and better reflects its purpose.
32-32: Good use of safe navigation operators.Adding the safe navigation operator (
?.) prevents potential runtime errors whenprojectis null or undefined.Also applies to: 41-41
55-55: Excellent refactoring to use computed properties.Using
hasCclaEnabledandhasIclaEnabledcomputed properties improves code readability and maintainability.Also applies to: 63-63
src/app/shared/components/project-title/project-title.component.ts (1)
53-53: ```shell
#!/bin/bashDisplay class definition and property declarations
rg -n "export class ClaDashboardComponent" -A 20 src/app/modules/dashboard/container/cla-dashboard/cla-dashboard.component.ts
</details> <details> <summary>src/app/modules/dashboard/container/cla-dashboard/cla-dashboard.component.ts (1)</summary> `95-97`: **LGTM!** The `setProject` method provides a clean way to update the project model from the template. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| <div class="col-auto not-found ">The page you are looking for was not found. Please <a [href]="redirectUrl" class="">click here</a> | ||
| to go back on main page or contact the administrator.</div> |
There was a problem hiding this comment.
Add validation and error handling for the redirect URL.
The template directly binds redirectUrl to href without validation, which could lead to issues if the URL is null, undefined, or contains malicious content.
Consider adding validation in the template:
- <div class="col-auto not-found ">The page you are looking for was not found. Please <a [href]="redirectUrl" class="">click here</a>
+ <div class="col-auto not-found ">The page you are looking for was not found. Please <a [href]="redirectUrl || '/'" class="">click here</a>
to go back on main page or contact the administrator.</div>Or better yet, add proper URL validation in the component and use conditional rendering:
- <div class="col-auto not-found ">The page you are looking for was not found. Please <a [href]="redirectUrl" class="">click here</a>
+ <div class="col-auto not-found ">The page you are looking for was not found. Please <a *ngIf="isValidUrl(redirectUrl)" [href]="redirectUrl" class="">click here</a><span *ngIf="!isValidUrl(redirectUrl)">contact the administrator</span>
to go back on main page or contact the administrator.</div>📝 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.
| <div class="col-auto not-found ">The page you are looking for was not found. Please <a [href]="redirectUrl" class="">click here</a> | |
| to go back on main page or contact the administrator.</div> | |
| <div class="col-auto not-found "> | |
| The page you are looking for was not found. Please | |
| <a [href]="redirectUrl || '/'" class="">click here</a> | |
| to go back on main page or contact the administrator. | |
| </div> |
🤖 Prompt for AI Agents
In src/app/shared/components/page-not-found/page-not-found.component.html around
lines 6 to 7, the redirectUrl is directly bound to the href attribute without
validation, which risks null, undefined, or unsafe URLs. To fix this, add
validation logic in the component to ensure redirectUrl is a valid, safe URL
before binding it. Then update the template to conditionally render the anchor
tag only if redirectUrl passes validation, otherwise display a fallback message
or disable the link.
| export class PageNotFoundComponent implements OnInit { | ||
| message: string; | ||
| actionType: string; | ||
| redirectUrl:string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and initialization.
The redirectUrl property should handle the possibility of null/undefined values from storage.
- redirectUrl:string;
+ redirectUrl: string | null = null;This ensures type safety and prevents potential runtime errors when the storage value is not available.
📝 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.
| redirectUrl:string; | |
| redirectUrl: string | null = null; |
🤖 Prompt for AI Agents
In src/app/shared/components/page-not-found/page-not-found.component.ts at line
15, the redirectUrl property is declared without a type or initialization, which
risks runtime errors if the storage value is null or undefined. Update the
declaration to explicitly type redirectUrl as string or null, and initialize it
safely by checking the storage value, ensuring it handles null or undefined
cases to improve type safety and prevent errors.
| ngOnInit(): void { | ||
| this.message = 'Wait... we are validating session'; | ||
| // If auth0 response is null then page not exist. | ||
| setTimeout(() => { | ||
| this.message = 'The page you are looking for was not found.'; | ||
| }, 2000); | ||
| this.redirectUrl = JSON.parse(this.storageService.getItem(AppSettings.REDIRECT)); | ||
| } |
There was a problem hiding this comment.
Add proper error handling and validation for JSON parsing.
The current implementation has several critical issues:
JSON.parse()can throw an error if the stored value is not valid JSON- No null/undefined check before parsing
- Potential security risk parsing arbitrary JSON from storage
ngOnInit(): void {
- this.redirectUrl = JSON.parse(this.storageService.getItem(AppSettings.REDIRECT));
+ try {
+ const storedValue = this.storageService.getItem(AppSettings.REDIRECT);
+ if (storedValue) {
+ this.redirectUrl = JSON.parse(storedValue);
+ // Validate URL format for security
+ if (!this.isValidUrl(this.redirectUrl)) {
+ this.redirectUrl = null;
+ }
+ }
+ } catch (error) {
+ console.error('Error parsing redirect URL from storage:', error);
+ this.redirectUrl = null;
+ }
}
+
+ private isValidUrl(url: string | null): boolean {
+ if (!url) return false;
+ try {
+ const urlObj = new URL(url);
+ return ['http:', 'https:'].includes(urlObj.protocol);
+ } catch {
+ return false;
+ }
+ }📝 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.
| ngOnInit(): void { | |
| this.message = 'Wait... we are validating session'; | |
| // If auth0 response is null then page not exist. | |
| setTimeout(() => { | |
| this.message = 'The page you are looking for was not found.'; | |
| }, 2000); | |
| this.redirectUrl = JSON.parse(this.storageService.getItem(AppSettings.REDIRECT)); | |
| } | |
| ngOnInit(): void { | |
| try { | |
| const storedValue = this.storageService.getItem(AppSettings.REDIRECT); | |
| if (storedValue) { | |
| this.redirectUrl = JSON.parse(storedValue); | |
| // Validate URL format for security | |
| if (!this.isValidUrl(this.redirectUrl)) { | |
| this.redirectUrl = null; | |
| } | |
| } | |
| } catch (error) { | |
| console.error('Error parsing redirect URL from storage:', error); | |
| this.redirectUrl = null; | |
| } | |
| } | |
| private isValidUrl(url: string | null): boolean { | |
| if (!url) return false; | |
| try { | |
| const urlObj = new URL(url); | |
| return ['http:', 'https:'].includes(urlObj.protocol); | |
| } catch { | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/app/shared/components/page-not-found/page-not-found.component.ts around
lines 21 to 23, the code directly parses JSON from storage without validation or
error handling, which can cause runtime errors or security issues. Modify the
code to first check if the stored value exists and is a non-empty string, then
wrap the JSON.parse call in a try-catch block to handle any parsing errors
gracefully. If parsing fails or the value is invalid, set redirectUrl to a safe
default or null to avoid runtime exceptions.
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.html
Outdated
Show resolved
Hide resolved
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.spec.ts
Show resolved
Hide resolved
| <div class="row"> | ||
| <div class="col-xs-12 col-sm-12 col-md-6 col-lg-6"> | ||
| <app-dashboard-contributor-card [type]="corporateContributor" [highlights]="corporateHightlights" | ||
| <app-dashboard-contributor-card [type]="corporateContributor" [highlights]="corporateHightLights" |
There was a problem hiding this comment.
Property name mismatch between template and component.
The template uses corporateHightLights and individualHightLights, but these should match the corrected spelling in the component properties.
- <app-dashboard-contributor-card [type]="corporateContributor" [highlights]="corporateHightLights"
+ <app-dashboard-contributor-card [type]="corporateContributor" [highlights]="corporateHighlights"
(btnEmitter)="onClickCorporateProceed()">- <app-dashboard-contributor-card [type]="individualContributor" [highlights]="individualHightLights"
+ <app-dashboard-contributor-card [type]="individualContributor" [highlights]="individualHighlights"
(btnEmitter)="onClickIndividualProceed()">Also applies to: 23-23
🤖 Prompt for AI Agents
In
src/app/modules/dashboard/container/cla-dashboard/cla-dashboard.component.html
at lines 18 and 23, the property names used in the template are misspelled as
corporateHightLights and individualHightLights. Update these property bindings
to match the correct spelling used in the component, which should be
corporateHighlights and individualHighlights respectively, ensuring consistency
between the template and component properties.
| const projectId = JSON.parse(this.storageService.getItem(AppSettings.PROJECT_ID)); | ||
| if (projectId) { |
There was a problem hiding this comment.
Add error handling for JSON.parse operations.
Multiple JSON.parse calls could throw errors if the stored values are not valid JSON.
For example, in the getProject method:
- const projectId = JSON.parse(this.storageService.getItem(AppSettings.PROJECT_ID));
+ let projectId;
+ try {
+ projectId = JSON.parse(this.storageService.getItem(AppSettings.PROJECT_ID) || 'null');
+ } catch (error) {
+ this.errorEmitter.emit(true);
+ this.alertService.error('Invalid project ID format');
+ return;
+ }Apply similar error handling to other JSON.parse calls in lines 69 and 32.
Also applies to: 51-51, 69-70, 74-74
🤖 Prompt for AI Agents
In src/app/shared/components/project-title/project-title.component.ts around
lines 32, 45-46, 51, 69-70, and 74, the JSON.parse calls lack error handling and
may throw exceptions if the stored values are invalid JSON. Wrap each JSON.parse
call in a try-catch block to safely handle parsing errors, returning a default
value or null if parsing fails, to prevent runtime exceptions and improve
robustness.
| corporateHightLights: string[]; | ||
| individualHightLights: string[]; |
There was a problem hiding this comment.
Fix the spelling of highlight properties.
The property names still use the misspelled "hightLights" instead of "highlights".
- corporateHightLights: string[];
- individualHightLights: string[];
+ corporateHighlights: string[];
+ individualHighlights: string[];- this.corporateHightLights = [
+ this.corporateHighlights = [- this.individualHightLights = [
+ this.individualHighlights = [Also applies to: 41-41, 48-48
🤖 Prompt for AI Agents
In src/app/modules/dashboard/container/cla-dashboard/cla-dashboard.component.ts
at lines 22-23, 41, and 48, the property names use the incorrect spelling
"hightLights". Rename these properties to "highlights" to correct the spelling.
Update all references to these properties accordingly to maintain consistency
and avoid errors.
| this.authService.loading$.subscribe((loading) => { | ||
| if (!loading) { | ||
| this.authService.isAuthenticated$.subscribe((authenticated) => { | ||
| if (!authenticated && !this.hasPrivateWindow) { | ||
| this.authService.login(); | ||
| } | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Manage subscriptions to prevent memory leaks.
The nested subscriptions are not unsubscribed, which can cause memory leaks. Consider using RxJS operators and implementing OnDestroy.
First, import the necessary items:
-import { Component, OnInit } from '@angular/core';
+import { Component, OnInit, OnDestroy } from '@angular/core';
+import { Subject } from 'rxjs';
+import { takeUntil, filter, switchMap } from 'rxjs/operators';Then update the class:
-export class ClaDashboardComponent implements OnInit {
+export class ClaDashboardComponent implements OnInit, OnDestroy {
+ private destroy$ = new Subject<void>();Refactor the subscription logic:
- this.authService.loading$.subscribe((loading) => {
- if (!loading) {
- this.authService.isAuthenticated$.subscribe((authenticated) => {
- if (!authenticated && !this.hasPrivateWindow) {
- this.authService.login();
- }
- });
- }
- });
+ this.authService.loading$.pipe(
+ filter(loading => !loading),
+ switchMap(() => this.authService.isAuthenticated$),
+ takeUntil(this.destroy$)
+ ).subscribe((authenticated) => {
+ if (!authenticated && !this.hasPrivateWindow) {
+ this.authService.login();
+ }
+ });Add the ngOnDestroy method:
ngOnDestroy(): void {
this.destroy$.next();
this.destroy$.complete();
}🤖 Prompt for AI Agents
In src/app/modules/dashboard/container/cla-dashboard/cla-dashboard.component.ts
around lines 76 to 84, the nested subscriptions to loading$ and isAuthenticated$
are not unsubscribed, risking memory leaks. Refactor by using RxJS operators
like switchMap and takeUntil with a destroy$ subject to manage unsubscription.
Import Subject and takeUntil, declare a private destroy$ subject in the class,
pipe the observables with takeUntil(this.destroy$), and implement ngOnDestroy to
call destroy$.next() and destroy$.complete() to clean up subscriptions properly.
| this.userId = this.route.snapshot.paramMap.get('userId'); | ||
| const redirect = this.route.snapshot.queryParamMap.get('redirect'); | ||
|
|
||
| this.project = JSON.parse(this.storageService.getItem(AppSettings.PROJECT)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for JSON.parse.
The JSON.parse call could throw if the stored value is not valid JSON.
- this.project = JSON.parse(this.storageService.getItem(AppSettings.PROJECT));
+ try {
+ const storedProject = this.storageService.getItem(AppSettings.PROJECT);
+ this.project = storedProject ? JSON.parse(storedProject) : new ProjectModel();
+ } catch (error) {
+ console.error('Error parsing stored project:', error);
+ this.project = new ProjectModel();
+ }📝 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.project = JSON.parse(this.storageService.getItem(AppSettings.PROJECT)); | |
| try { | |
| const storedProject = this.storageService.getItem(AppSettings.PROJECT); | |
| this.project = storedProject | |
| ? JSON.parse(storedProject) | |
| : new ProjectModel(); | |
| } catch (error) { | |
| console.error('Error parsing stored project:', error); | |
| this.project = new ProjectModel(); | |
| } |
🤖 Prompt for AI Agents
In src/app/modules/dashboard/container/cla-dashboard/cla-dashboard.component.ts
at line 62, the JSON.parse call on the stored project data can throw an error if
the data is not valid JSON. Wrap the JSON.parse call in a try-catch block to
handle potential parsing errors gracefully. In the catch block, handle the error
by either setting this.project to a default value or logging the error for
debugging purposes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.spec.ts (1)
12-23: DuplicatebeforeEachhooks still violate Biome rule.The lint error flagged earlier (
noDuplicateTestHooks) remains because lines 12-17 and 19-23 define two separatebeforeEachblocks. Merge them to a single hook:- beforeEach(async () => { - await TestBed.configureTestingModule({ - declarations: [ AuthDashboardComponent ] - }) - .compileComponents(); - }); - - beforeEach(() => { - fixture = TestBed.createComponent(AuthDashboardComponent); - component = fixture.componentInstance; - fixture.detectChanges(); - }); + beforeEach(async () => { + await TestBed.configureTestingModule({ + declarations: [AuthDashboardComponent], + }).compileComponents(); + + fixture = TestBed.createComponent(AuthDashboardComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + });
🧹 Nitpick comments (1)
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.html (1)
1-2: Template is now empty – consider adding a loader or router outletThe placeholder paragraph is gone (✅ thanks!), but the template is now completely empty apart from the license header.
If the TS logic performs an immediate redirect this is fine, otherwise end-users will see a blank screen.
Typical patterns are to either:+<cla-loader *ngIf="isLoading"></cla-loader> +<router-outlet *ngIf="!isLoading"></router-outlet>or leave an explanatory comment so future maintainers know the emptiness is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.html(1 hunks)src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.scss(1 hunks)src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.spec.ts(1 hunks)src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.scss
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.ts
🧰 Additional context used
🪛 HTMLHint (1.5.0)
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.html
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
🪛 Biome (1.9.4)
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.spec.ts
[error] 19-23: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (2)
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.spec.ts (1)
1-3: License header now present – nice catch.The required SPDX-MIT header has been added, resolving the pipeline error. 👍
src/app/modules/dashboard/container/auth-dashboard/auth-dashboard.component.html (1)
1-2: License header looks goodThe SPDX identifier and copyright notice are correctly added.
No further action required.
This pull request introduces several changes across multiple files, primarily focusing on refactoring and improving the dashboard components, updating dependencies, and removing unused code. The most significant updates include replacing the
AuthComponentwith a newAuthDashboardComponent, enhancing theClaDashboardComponentwith additional functionality, and simplifying project title bindings in various dashboard components.Dashboard Refactoring and Enhancements:
AuthComponentwithAuthDashboardComponentinsrc/app/app-routing.module.tsand implemented its logic to handle project and user redirection based on stored settings. ([[1]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-bdebfe9c9aa11d74e7d28dcfb0cc5c678070b2d01d56edad196cb18357f96487L12-R19),[[2]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-b2506bb873cf4bade0774059387e5a732f19a4bd51fce7ddce59b4020ab9f133R1-R23),[[3]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-ad216ff6ffcdc0297ec04c876db602a484b6ab4b94505c63ddc04e04d7ed5972R1-R25),[[4]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-99453c3bef5e14943b85476cdaf74f8ed95341083a4a139fc32a835dce95de3eR1))ClaDashboardComponentto detect private browsing modes, authenticate users, and dynamically bind project settings. IntroducedhasCclaEnabledandhasIclaEnabledcomputed properties for cleaner logic. ([[1]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-62df717bac294a04b900316d01bedb51d8e889c81247e341871ef86904233e86R11-R12),[[2]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-62df717bac294a04b900316d01bedb51d8e889c81247e341871ef86904233e86L20-R98),[[3]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-62df717bac294a04b900316d01bedb51d8e889c81247e341871ef86904233e86L85-L91))projectIdanduserIdinapp-project-titleacross multiple dashboard components (cla-request-authorization,corporate-dashboard,individual-dashboard, andcla-dashboard). ([[1]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-2228ddb8afafed19683b3f5ccc2ca9b8d48b66504cb4c1e1126a1aa38fd9112eL4-R4),[[2]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-1dd51e7420cb30f246e1f6e70945e2a973138da7a386559448051b97fa7717ceL4-L8),[[3]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-36e9ed202e855e6b9f0629754e7e5a96271f0c836298026842e7fc3ff21c62a7L4-R4),[[4]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-dfe617114d75550db10ba00918d10e62648c0b273012e1aa961b6d41387f6a06L3-L8))Dependency Updates:
package.jsonto include new dependencies (detectincognitojs) and specify the package manager ([email protected]). ([[1]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R57-R64),[[2]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519L142-R144))Code Cleanup:
AuthComponentfiles (auth.component.htmlandauth.component.scss). ([[1]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-290f3823c625f5db1a64cc26e02ff1bd641503b7322bbbba06fcd7fae0154ed3L1-L8),[[2]](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-03568fda6cfa2102882b9c96802697f438531eaf27ccad9099d74f4c7b187f87L1-L11))ConfigureClaManagerModalComponent. ([src/app/modules/corporate-contributor/component/configure-cla-manager-modal/configure-cla-manager-modal.component.tsL285](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-5300d32dfb5c1fc831d8d1deb3ef9a53af1678c76ad683caf9051e4dc0a97c6cL285))Minor Fixes:
corporateHightlights→corporateHightLights,individualHightlights→individualHightLights) inClaDashboardComponent. ([src/app/modules/dashboard/container/cla-dashboard/cla-dashboard.component.tsL20-R98](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-62df717bac294a04b900316d01bedb51d8e889c81247e341871ef86904233e86L20-R98))alert.component.scssto remove fixed positioning from.alert. ([src/app/shared/components/alert/alert.component.scssL6](https://github.com/linuxfoundation/easycla-contributor-console/pull/449/files#diff-d1e5619c43c0ef7d66bab46c7e119805d0bb69faf46c0ac843a894a0cf747f52L6))Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Chores
Tests