feat: eslint check when create PR #15
Conversation
WalkthroughA new GitHub Actions workflow file named Changes
Poem
Warning Rate limit exceeded@thongdanghoang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (19)
sep490-frontend/src/app/modules/shared/services/modal-provider.ts (2)
Line range hint
19-25: Consider adding dialog cleanup and error handling.While the implementation works, consider these improvements:
- Close any existing dialog before opening a new one to prevent memory leaks
- Add error handling for dialog operations
Consider this implementation:
showConfirm(params: Confirmation): Observable<boolean> { + if (this.ref) { + this.ref.close(); + } + try { this.ref = this.dialogService.open(ConfirmDialogComponent, { header: params.header, modal: true, data: params }); return this.ref.onClose; + } catch (error) { + console.error('Error opening confirm dialog:', error); + throw error; + } }
Line range hint
27-33: TODO comments need implementation.The unimplemented error and dirty check dialogs are important for proper error handling and form state management. These features should be implemented to ensure robust application behavior.
Would you like me to help create GitHub issues to track these implementations? I can provide suggested implementations for:
- Error dialog with proper error message handling
- Dirty check confirmation for unsaved form changes
.github/workflows/pull-request.yml (3)
1-1: Consider a more descriptive workflow nameThe current name "PR Review" is quite generic. Consider renaming it to better reflect its specific purpose, such as "Build and Lint Check" or "PR Quality Gates".
-name: PR Review +name: Build and Lint Check
16-19: Add fail-fast strategy to matrix configurationConsider adding
fail-fast: trueto the matrix strategy to stop all matrix jobs if any job fails, saving CI resources.strategy: + fail-fast: true matrix: module: [ 'sep490-common', 'sep490-idp' ]
41-43: Specify exact Node.js versionUsing a floating version like '20.x' could lead to inconsistent builds. Consider pinning to a specific version.
uses: actions/setup-node@v4 with: - node-version: '20.x' + node-version: '20.11.0'sep490-frontend/src/app/components/header/header.component.ts (2)
Line range hint
31-42: Consider adding type safety for menu item routesThe explicit return type on
ngOnInitis good. For additional type safety, consider creating a type for the route constants.type AppRoute = typeof AppRoutingConstants[keyof typeof AppRoutingConstants]; // Then use it in MenuItem type: protected items: (MenuItem & { route: AppRoute })[] | undefined;
Line range hint
59-64: Consider simplifying theme toggle logicWhile the current implementation is correct, it could be more concise.
protected toggleLightDark(): void { - if (this.themeService.isDarkMode()) { - this.themeService.selectTheme(Theme.AURA_LIGHT_CYAN); - return; - } - this.themeService.selectTheme(Theme.AURA_DARK_CYAN); + const newTheme = this.themeService.isDarkMode() + ? Theme.AURA_LIGHT_CYAN + : Theme.AURA_DARK_CYAN; + this.themeService.selectTheme(newTheme); }sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts (2)
67-67: Consider adding type safety to the option methodThe
optionmethod currently returnsany, which bypasses TypeScript's type checking. Consider creating a union type of all possible option values.Example improvement:
type ConfirmationOption = string | boolean | EventEmitter<any> | undefined; option(name: string): ConfirmationOption { // ... rest of the implementation }
59-63: Extract translation keys as constantsThe hardcoded translation keys should be extracted into constants to prevent typos and enable easier maintenance.
Consider creating a constants file or enum:
export enum TranslationKeys { ACCEPT = 'TranslationKeys.ACCEPT', REJECT = 'TranslationKeys.REJECT' }Then update the usage:
- return this.option('acceptLabel') ?? 'TranslationKeys.ACCEPT'; + return this.option('acceptLabel') ?? TranslationKeys.ACCEPT; - return this.option('rejectLabel') ?? 'TranslationKeys.REJECT'; + return this.option('rejectLabel') ?? TranslationKeys.REJECT;sep490-frontend/src/app/modules/core/services/application.service.ts (2)
10-12: Consider using type aliases or enriching interfacesThe empty interfaces
UserDataandUserInfoDataare currently equivalent to their supertypeJwtPayload. Consider either:
- Using type aliases for better semantics:
type UserData = JwtPayload; type UserInfoData = JwtPayload;
- Or enriching interfaces with expected payload properties:
interface UserData extends JwtPayload { // Add expected user data properties email?: string; roles?: string[]; }🧰 Tools
🪛 GitHub Check: Check Lint
[failure] 10-10:
An interface declaring no members is equivalent to its supertype
[failure] 12-12:
An interface declaring no members is equivalent to its supertype
57-60: Consider enhancing error handlingWhile the basic error handling is in place, consider:
- Implementing a centralized error handling service instead of direct console.error calls
- Adding user feedback mechanism for authentication failures
- Adding error tracking/monitoring for production environments
Example implementation:
@Injectable() export class ErrorHandlingService { handleAuthError(error: any, context: string) { // Log to monitoring service // Show user-friendly notification console.error(`Auth error in ${context}:`, error); } }Also applies to: 65-68
sep490-frontend/src/app/modules/enterprise/enterprise.component.ts (3)
Line range hint
39-43: Consider implementing marker managementThe current implementation adds markers without any mechanism to remove them. This could lead to memory leaks and performance issues if many markers are added.
Consider implementing a marker management system:
+ private markers: L.Marker[] = []; ngAfterViewInit(): void { this.initMap(); this.markerService.makeCapitalMarkers(this.map); - this.map.on('click', e => { - const marker = L.marker([e.latlng.lat, e.latlng.lng]); - marker.addTo(this.map); - }); + this.map.on('click', e => { + const marker = L.marker([e.latlng.lat, e.latlng.lng]); + this.markers.push(marker); + marker.addTo(this.map); + });Also add a cleanup method:
public clearMarkers(): void { this.markers.forEach(marker => marker.remove()); this.markers = []; }
75-81: Consider utilizing the feature parameter or remove if unusedThe
featureparameter in the style function is currently unused. If feature-based styling is not planned, consider removing it.- style: feature => ({ + style: () => ({Alternatively, if feature-based styling is planned, add a TODO comment explaining the future use case.
🧰 Tools
🪛 GitHub Check: Check Lint
[failure] 75-75:
'feature' is defined but never used
Line range hint
93-115: Extract duplicate styles into constantsThe highlight and reset styles are duplicated. Consider extracting them into constants for better maintainability.
private readonly DEFAULT_STYLE = { weight: 3, opacity: 0.5, color: '#008f68', fillOpacity: 0.8, fillColor: '#6DB65B' }; private readonly HIGHLIGHT_STYLE = { weight: 10, opacity: 1.0, color: '#DFA612', fillOpacity: 1.0, fillColor: '#FAE042' }; private highlightFeature(e: L.LeafletMouseEvent): void { const layer = e.target; layer.setStyle(this.HIGHLIGHT_STYLE); } private resetFeature(e: L.LeafletMouseEvent): void { const layer = e.target; layer.setStyle(this.DEFAULT_STYLE); }sep490-frontend/angular.json (1)
151-158: LGTM! Consider adding explicit file exclusions.The lint configuration is correctly set up using the official
@angular-eslint/builder:lintbuilder with appropriate patterns for TypeScript and HTML files.Consider adding explicit file exclusions to the lint configuration to improve performance:
"lint": { "builder": "@angular-eslint/builder:lint", "options": { "lintFilePatterns": [ "src/**/*.ts", "src/**/*.html" - ] + ], + "exclude": [ + "src/**/*.spec.ts", + "src/environments/*.ts" + ] } }sep490-frontend/.eslintrc.json (4)
8-12: Consider removingcreateDefaultProgramfor better performanceThe
createDefaultProgram: truesetting can significantly impact linting performance, especially in larger projects. This setting is typically only needed when you're having issues with TypeScript program creation."parserOptions": { "project": [ "tsconfig.json" ], - "createDefaultProgram": true }
71-73: Consider increasing the max-lines-per-function limitThe 35-line limit per function is quite restrictive for Angular components, especially those with complex lifecycle methods or component logic. Consider increasing this limit to around 50-75 lines to avoid unnecessary function splitting while still maintaining reasonable function sizes.
"max-lines-per-function": ["error", { - "max": 35 + "max": 50 }],
129-134: Consider additional decorator-specific rulesWhile disabling
max-lines-per-functionfor decorators is good, consider adding other common decorator-specific rules:
- Allow higher complexity for decorators
- Disable certain naming conventions that might not apply to decorators
"files": ["*.decorator.ts"], "rules": { - "max-lines-per-function": "off" + "max-lines-per-function": "off", + "complexity": ["error", 20], + "@typescript-eslint/naming-convention": "off" }
144-151: Consider using consistent tab width across all filesThe configuration uses different tab widths for HTML (4 spaces) and TypeScript (2 spaces) files. This inconsistency might lead to formatting confusion. Consider using the same tab width across all file types for better maintainability.
"prettier/prettier": [ "error", { "parser": "angular", "endOfLine": "auto", - "tabWidth": 4, + "tabWidth": 2, "useTabs": false } ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
sep490-frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.github/workflows/pull-request.yml(1 hunks)sep490-frontend/.eslintignore(1 hunks)sep490-frontend/.eslintrc.json(1 hunks)sep490-frontend/angular.json(1 hunks)sep490-frontend/package.json(2 hunks)sep490-frontend/src/app/app-routing.constant.ts(1 hunks)sep490-frontend/src/app/app-routing.module.ts(2 hunks)sep490-frontend/src/app/app.component.html(1 hunks)sep490-frontend/src/app/app.component.ts(2 hunks)sep490-frontend/src/app/app.module.ts(3 hunks)sep490-frontend/src/app/application.constant.ts(1 hunks)sep490-frontend/src/app/components/footer/footer.component.ts(1 hunks)sep490-frontend/src/app/components/forbidden/forbidden.component.ts(1 hunks)sep490-frontend/src/app/components/header/header.component.html(1 hunks)sep490-frontend/src/app/components/header/header.component.ts(5 hunks)sep490-frontend/src/app/components/home/home.component.ts(1 hunks)sep490-frontend/src/app/components/not-found/not-found.component.ts(1 hunks)sep490-frontend/src/app/components/unauthorized/unauthorized.component.ts(1 hunks)sep490-frontend/src/app/modules/core/core.module.ts(1 hunks)sep490-frontend/src/app/modules/core/module-import-guard.ts(1 hunks)sep490-frontend/src/app/modules/core/services/application.service.ts(3 hunks)sep490-frontend/src/app/modules/core/services/theme.service.ts(3 hunks)sep490-frontend/src/app/modules/core/subscription-aware.component.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/enterprise-routing.module.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.component.html(1 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.component.ts(3 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.module.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/services/marker.service.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/services/popup.service.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/services/region.service.ts(1 hunks)sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.html(1 hunks)sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts(3 hunks)sep490-frontend/src/app/modules/shared/components/form/abstract-form-component.ts(8 hunks)sep490-frontend/src/app/modules/shared/services/modal-provider.ts(2 hunks)sep490-frontend/src/app/modules/shared/shared.module.ts(2 hunks)
✅ Files skipped from review due to trivial changes (21)
- sep490-frontend/.eslintignore
- sep490-frontend/src/app/app-routing.constant.ts
- sep490-frontend/src/app/app-routing.module.ts
- sep490-frontend/src/app/app.component.html
- sep490-frontend/src/app/application.constant.ts
- sep490-frontend/src/app/components/footer/footer.component.ts
- sep490-frontend/src/app/components/forbidden/forbidden.component.ts
- sep490-frontend/src/app/components/header/header.component.html
- sep490-frontend/src/app/components/home/home.component.ts
- sep490-frontend/src/app/components/not-found/not-found.component.ts
- sep490-frontend/src/app/components/unauthorized/unauthorized.component.ts
- sep490-frontend/src/app/modules/core/core.module.ts
- sep490-frontend/src/app/modules/core/module-import-guard.ts
- sep490-frontend/src/app/modules/core/services/theme.service.ts
- sep490-frontend/src/app/modules/core/subscription-aware.component.ts
- sep490-frontend/src/app/modules/enterprise/enterprise-routing.module.ts
- sep490-frontend/src/app/modules/enterprise/enterprise.component.html
- sep490-frontend/src/app/modules/enterprise/enterprise.module.ts
- sep490-frontend/src/app/modules/enterprise/services/marker.service.ts
- sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.html
- sep490-frontend/src/app/modules/shared/shared.module.ts
🧰 Additional context used
🪛 yamllint (1.29.0-1)
.github/workflows/pull-request.yml
[error] 54-54: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Check: Check Lint
sep490-frontend/src/app/modules/core/services/application.service.ts
[failure] 10-10:
An interface declaring no members is equivalent to its supertype
[failure] 12-12:
An interface declaring no members is equivalent to its supertype
sep490-frontend/src/app/modules/enterprise/enterprise.component.ts
[failure] 75-75:
'feature' is defined but never used
sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts
[failure] 29-29:
Prefer to declare @Output as readonly since they are not supposed to be reassigned
[failure] 29-29:
Output bindings, including aliases, should not be named "on", nor prefixed with it (https://angular.dev/style-guide#style-05-16)
[failure] 68-68:
Do not access Object.prototype method 'hasOwnProperty' from target object
sep490-frontend/src/app/modules/shared/components/form/abstract-form-component.ts
[failure] 168-168:
'_error' is defined but never used
🪛 Biome (1.9.4)
sep490-frontend/src/app/modules/enterprise/services/popup.service.ts
[error] 6-6: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts
[error] 68-68: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (24)
sep490-frontend/src/app/modules/enterprise/services/region.service.ts (4)
4-4: LGTM! Import formatting aligns with ESLint standards
The single quotes usage is consistent with the PR's ESLint configuration objectives.
11-11: LGTM! Good use of readonly modifier
The readonly modifier on the HttpClient injection improves immutability guarantees.
13-18: LGTM! Improved method signature formatting
The multi-line formatting of complex types enhances readability while maintaining type safety.
8-9: Consider internationalization implications
The hardcoded path suggests US-specific geographic data. Consider making this configurable to support multiple regions/countries in the future.
Let's check if this file is used in other locations:
sep490-frontend/src/app/modules/shared/services/modal-provider.ts (2)
1-1: LGTM! Good practice implementing OnDestroy.
The implementation of OnDestroy interface ensures proper cleanup of resources, which is crucial for preventing memory leaks in Angular applications.
Also applies to: 9-12
Line range hint 36-41: LGTM! Proper cleanup implementation.
The ngOnDestroy implementation correctly handles dialog cleanup before calling the parent class's cleanup method. This ensures proper resource management and prevents memory leaks.
sep490-frontend/src/app/app.component.ts (1)
12-12: LGTM! Clean constructor implementation.
The constructor follows Angular's dependency injection best practices with proper readonly modifier.
.github/workflows/pull-request.yml (1)
1-54: Verify workflow dependencies
Let's verify that all required directories and configurations exist.
✅ Verification successful
All workflow dependencies are present and correctly configured
All required directories and configuration files for the workflow are present in the repository:
- Both matrix modules
sep490-commonandsep490-idpexist - Frontend directory
sep490-frontendexists with required lint configurations:.eslintrc.jsonfor ESLint rulespackage.jsonfor npm dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all required directories and configurations exist
# Check modules from matrix
for module in sep490-common sep490-idp; do
if [ ! -d "$module" ]; then
echo "❌ Missing module directory: $module"
else
echo "✅ Found module: $module"
fi
done
# Check frontend setup
if [ -d "sep490-frontend" ]; then
echo "✅ Found frontend directory"
# Check for required frontend configurations
for file in "sep490-frontend/.eslintrc.json" "sep490-frontend/package.json"; do
if [ -f "$file" ]; then
echo "✅ Found $file"
else
echo "❌ Missing $file"
fi
done
else
echo "❌ Missing frontend directory"
fi
Length of output: 776
🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 54-54: no new line character at the end of file
(new-line-at-end-of-file)
sep490-frontend/package.json (2)
9-10: LGTM! Proper integration of lint script
The lint script is correctly added using Angular CLI's lint command, which is the recommended approach for Angular projects.
41-45:
Critical: Fix ESLint version and remove redundant package
There are several version-related issues that need to be addressed:
- ESLint version
9.15.0is invalid as ESLint's latest version is in the 8.x range - The
typescript-eslintpackage is redundant as you already have@typescript-eslint/eslint-plugin
Please apply these corrections:
- "eslint": "^9.15.0",
+ "eslint": "^8.56.0",
- "typescript-eslint": "8.15.0",Let's verify the latest stable versions:
sep490-frontend/src/app/components/header/header.component.ts (3)
5-5: LGTM! Consistent quote style
The change to single quotes aligns with TypeScript/Angular style conventions.
15-18: LGTM! Improved class declaration readability
The multi-line format for class inheritance improves code readability and follows Angular style conventions.
15-18: Verify subscription handling
The component extends SubscriptionAwareComponent but no explicit subscription management is visible. Please verify that any subscriptions (e.g., from this.authenticated observable) are properly managed.
Also applies to: 31-31
✅ Verification successful
Let me gather more information about the subscription usage in the HeaderComponent.
No subscription management issues found
The component correctly extends SubscriptionAwareComponent which provides automatic cleanup of subscriptions through its ngOnDestroy implementation. The authenticated property is declared as an Observable<boolean> and is only assigned from applicationService.isAuthenticated() without any direct subscriptions in the component. This is a proper usage pattern as the observable is likely consumed in the template using the async pipe, which handles subscription management automatically.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for subscription handling in the component and base class
echo "Checking SubscriptionAwareComponent implementation:"
rg -A 10 "class SubscriptionAwareComponent"
echo "\nChecking for subscription usage in HeaderComponent:"
ast-grep --pattern 'this.authenticated.subscribe($_)'
Length of output: 1532
Script:
#!/bin/bash
# Search for any subscription usage in HeaderComponent
echo "Checking HeaderComponent implementation for subscriptions:"
rg -A 5 "subscribe\(" "sep490-frontend/src/app/components/header/header.component.ts"
echo "\nChecking for registerSubscription usage:"
rg "registerSubscription\(" "sep490-frontend/src/app/components/header/header.component.ts"
echo "\nChecking authenticated property usage:"
rg "authenticated" "sep490-frontend/src/app/components/header/header.component.ts"
Length of output: 696
sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts (1)
32-36: LGTM! Well-structured dependency injection
The constructor follows Angular's best practices with proper access modifiers and readonly declarations.
sep490-frontend/src/app/modules/core/services/application.service.ts (3)
Line range hint 15-26: LGTM! Clean implementation of OnDestroy
The class properly implements OnDestroy and initializes BehaviorSubjects in the constructor. This follows Angular best practices for lifecycle management and reactive patterns.
🧰 Tools
🪛 GitHub Check: Check Lint
[failure] 10-10:
An interface declaring no members is equivalent to its supertype
[failure] 12-12:
An interface declaring no members is equivalent to its supertype
Line range hint 28-32: LGTM! Proper cleanup in ngOnDestroy
The ngOnDestroy implementation correctly completes all subjects and calls super.ngOnDestroy(), ensuring proper cleanup of resources.
34-37: LGTM! Well-structured authentication flow
The authentication methods are well-implemented with:
- Proper subscription management using SubscriptionAwareComponent
- Clean reactive patterns with proper error handling
- Thorough cleanup in postLogout
Also applies to: 40-85
sep490-frontend/src/app/app.module.ts (3)
11-11: LGTM! Import organization looks good.
71-77: LGTM! Provider configuration is well-structured.
The setup follows Angular best practices for configuring APP_INITIALIZER and HTTP interceptors.
Also applies to: 79-79, 81-81
62-65: Verify authentication configuration completeness.
- The 'openid' scope alone might be too restrictive. Consider if additional scopes (like 'profile', 'email') are needed for your application's functionality.
- The empty
secureRoutesarray suggests no routes are being protected by the auth interceptor.
✅ Verification successful
Authentication configuration is appropriately set up
The current configuration is correct for this application:
- The 'openid' scope is sufficient as the application uses the
AutoLoginPartialRoutesGuardfor authentication, which only requires basic OpenID authentication. - The empty
secureRoutesarray is correct because:- Route protection is properly implemented using
canActivate: [AutoLoginPartialRoutesGuard]on sensitive routes (DEV_PATH and ENTERPRISE_PATH) - The application has proper unauthorized/forbidden route handling
- The auth interceptor's secure routes array is not needed since authentication is handled at the route level
- Route protection is properly implemented using
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential secure routes in the application
echo "Searching for route definitions that might need protection..."
rg -t ts "canActivate|Route\[" ./sep490-frontend/src
Length of output: 377
Script:
#!/bin/bash
# Let's check the route definitions and guard implementation
echo "Checking route definitions:"
cat ./sep490-frontend/src/app/app-routing.module.ts
echo -e "\nChecking for auth-related imports and configurations:"
rg -t ts "AutoLoginPartialRoutesGuard|OAuthService|AuthConfig" ./sep490-frontend/src
echo -e "\nChecking for additional auth-related code:"
rg -t ts "auth|protected|secure" ./sep490-frontend/src
Length of output: 8138
sep490-frontend/src/app/modules/enterprise/enterprise.component.ts (1)
34-34: LGTM!
The constructor formatting follows modern TypeScript/Angular conventions.
sep490-frontend/angular.json (1)
163-166: LGTM! Correct CLI configuration for Angular ESLint.
The CLI configuration correctly includes the @angular-eslint/schematics collection, which ensures that newly generated Angular components and services will be ESLint-compatible.
sep490-frontend/.eslintrc.json (2)
13-19: Well-structured extension configuration!
The configuration properly extends all necessary recommended rulesets and correctly places Prettier last to avoid rule conflicts.
119-128: Appropriate test file configuration!
The overrides for test files are well-considered, allowing necessary testing patterns while maintaining code quality standards.
...frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts
Outdated
Show resolved
Hide resolved
...frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts
Outdated
Show resolved
Hide resolved
sep490-frontend/src/app/modules/enterprise/enterprise.component.ts
Outdated
Show resolved
Hide resolved
feat: eslint check when create PR feat: eslint and prettier feat: eslint to angular
thongdanghoang
force-pushed
the
feature/14-ci-pipelines
branch
from
69f4552 to
e95260e
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
sep490-frontend/src/app/modules/enterprise/services/popup.service.ts (2)
Line range hint
8-17: Improve type safety and prevent XSS vulnerabilities
- Replace
anywith a proper interface for better type safety- Consider using Angular's DomSanitizer to prevent XSS attacks when rendering HTML
Here's how to improve the code:
interface CapitalData { name: string; state: string; population: number; } @Injectable() export class PopupService { constructor(private sanitizer: DomSanitizer) {} makeCapitalPopup(data: CapitalData): SafeHtml { if (!data?.name || !data?.state || !data?.population) { throw new Error('Invalid capital data provided'); } const html = ` <div>Capital: ${data.name}</div> <div>State: ${data.state}</div> <div>Population: ${data.population.toLocaleString()}</div> `; return this.sanitizer.bypassSecurityTrustHtml(html); } }This refactor:
- Adds type safety with a proper interface
- Prevents XSS attacks using DomSanitizer
- Makes the code more maintainable and self-documenting
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
Raw HTML injection vulnerability found in popup.service.ts
The verification reveals a potential XSS vulnerability in
popup.service.ts. ThemakeCapitalPopupmethod returns raw HTML without sanitization, while other parts of the codebase properly use DomSanitizer (as seen in confirm-dialog.component.ts). Specific issues:
- The method returns template literals containing HTML without sanitization
- Despite declaring SafeHtml return type, no DomSanitizer is used
- No other instances of unsafe HTML injection were found in the codebase
🔗 Analysis chain
Line range hint
8-17: Verify other instances of raw HTML injectionLet's check if there are other instances in the codebase where raw HTML is being returned without proper sanitization.
Let me search for more specific patterns to ensure we haven't missed any potential XSS vulnerabilities.
Let me check one final pattern to ensure we haven't missed any DomSanitizer usage or HTML manipulation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential raw HTML injection patterns echo "Searching for raw HTML string templates..." rg "return \`.*\`" --type ts echo "Searching for SafeHtml return types without DomSanitizer..." rg "SafeHtml.*\{" --type ts -A 5 | grep -v "sanitizer"Length of output: 937
Script:
#!/bin/bash # Search for template literals containing HTML tags echo "Searching for template literals with HTML..." rg "<.*>\$\{.*\}" --type ts # Search for methods returning HTML strings echo "Searching for HTML string returns..." rg "return.*<.*>.*" --type ts # Search for SafeHtml usages echo "Searching for SafeHtml imports and usages..." rg "SafeHtml" --type tsLength of output: 597
Script:
#!/bin/bash # Search for DomSanitizer usage echo "Searching for DomSanitizer..." rg "DomSanitizer" --type ts -A 5 # Search for innerHTML assignments echo "Searching for innerHTML assignments..." rg "innerHTML.*=" --type ts # Search for bypassSecurityTrustHtml usage echo "Searching for bypassSecurityTrustHtml..." rg "bypassSecurityTrustHtml" --type tsLength of output: 1829
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
.github/workflows/pull-request.yml (1)
3-10: Consider adding 'ready_for_review' event triggerThe current PR triggers are good, but consider adding the 'ready_for_review' event to ensure checks run when draft PRs are marked as ready.
types: - opened - synchronize - reopened - edited + - ready_for_reviewsep490-frontend/src/app/modules/core/services/application.service.ts (2)
15-44: Consider documenting the purpose of unused interfacesThe interfaces
UserInfoPhoneScope,UserInfoAddressScope, andUserInfoProfileScopeare marked as unused. If these are intended for future OIDC scope expansion, consider adding documentation to explain their purpose and implementation timeline.
86-90: Add return type to the complete callbackFor consistency with TypeScript best practices, explicitly type the complete callback.
- complete: (): void => { + complete: function(): void { this.postLogout(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
sep490-frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
.github/workflows/pull-request.yml(1 hunks)sep490-frontend/.eslintignore(1 hunks)sep490-frontend/.eslintrc.json(1 hunks)sep490-frontend/angular.json(1 hunks)sep490-frontend/package.json(2 hunks)sep490-frontend/src/app/app-routing.constant.ts(1 hunks)sep490-frontend/src/app/app-routing.module.ts(2 hunks)sep490-frontend/src/app/app.component.html(1 hunks)sep490-frontend/src/app/app.component.ts(2 hunks)sep490-frontend/src/app/app.module.ts(3 hunks)sep490-frontend/src/app/application.constant.ts(1 hunks)sep490-frontend/src/app/components/footer/footer.component.ts(1 hunks)sep490-frontend/src/app/components/forbidden/forbidden.component.ts(1 hunks)sep490-frontend/src/app/components/header/header.component.html(1 hunks)sep490-frontend/src/app/components/header/header.component.ts(5 hunks)sep490-frontend/src/app/components/home/home.component.ts(1 hunks)sep490-frontend/src/app/components/not-found/not-found.component.ts(1 hunks)sep490-frontend/src/app/components/unauthorized/unauthorized.component.ts(1 hunks)sep490-frontend/src/app/modules/core/core.module.ts(1 hunks)sep490-frontend/src/app/modules/core/module-import-guard.ts(1 hunks)sep490-frontend/src/app/modules/core/services/application.service.ts(1 hunks)sep490-frontend/src/app/modules/core/services/theme.service.ts(3 hunks)sep490-frontend/src/app/modules/core/subscription-aware.component.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/enterprise-routing.module.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.component.html(1 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.component.ts(3 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.module.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/services/marker.service.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/services/popup.service.ts(1 hunks)sep490-frontend/src/app/modules/enterprise/services/region.service.ts(1 hunks)sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.html(1 hunks)sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts(3 hunks)sep490-frontend/src/app/modules/shared/components/form/abstract-form-component.ts(9 hunks)sep490-frontend/src/app/modules/shared/services/modal-provider.ts(2 hunks)sep490-frontend/src/app/modules/shared/shared.module.ts(2 hunks)sep490-idp/src/main/java/sep490/idp/SecurityConfig.java(5 hunks)sep490-idp/src/test/java/sep490/idp/IdentityProviderTests.java(0 hunks)
💤 Files with no reviewable changes (1)
- sep490-idp/src/test/java/sep490/idp/IdentityProviderTests.java
🚧 Files skipped from review as they are similar to previous changes (31)
- sep490-frontend/.eslintignore
- sep490-frontend/.eslintrc.json
- sep490-frontend/angular.json
- sep490-frontend/package.json
- sep490-frontend/src/app/app-routing.constant.ts
- sep490-frontend/src/app/app-routing.module.ts
- sep490-frontend/src/app/app.component.html
- sep490-frontend/src/app/app.component.ts
- sep490-frontend/src/app/app.module.ts
- sep490-frontend/src/app/application.constant.ts
- sep490-frontend/src/app/components/footer/footer.component.ts
- sep490-frontend/src/app/components/forbidden/forbidden.component.ts
- sep490-frontend/src/app/components/header/header.component.html
- sep490-frontend/src/app/components/header/header.component.ts
- sep490-frontend/src/app/components/home/home.component.ts
- sep490-frontend/src/app/components/not-found/not-found.component.ts
- sep490-frontend/src/app/components/unauthorized/unauthorized.component.ts
- sep490-frontend/src/app/modules/core/core.module.ts
- sep490-frontend/src/app/modules/core/module-import-guard.ts
- sep490-frontend/src/app/modules/core/services/theme.service.ts
- sep490-frontend/src/app/modules/core/subscription-aware.component.ts
- sep490-frontend/src/app/modules/enterprise/enterprise-routing.module.ts
- sep490-frontend/src/app/modules/enterprise/enterprise.component.html
- sep490-frontend/src/app/modules/enterprise/enterprise.component.ts
- sep490-frontend/src/app/modules/enterprise/enterprise.module.ts
- sep490-frontend/src/app/modules/enterprise/services/marker.service.ts
- sep490-frontend/src/app/modules/enterprise/services/region.service.ts
- sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.html
- sep490-frontend/src/app/modules/shared/components/form/abstract-form-component.ts
- sep490-frontend/src/app/modules/shared/services/modal-provider.ts
- sep490-frontend/src/app/modules/shared/shared.module.ts
🧰 Additional context used
🪛 yamllint (1.29.0-1)
.github/workflows/pull-request.yml
[error] 54-54: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Biome (1.9.4)
sep490-frontend/src/app/modules/enterprise/services/popup.service.ts
[error] 6-6: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (9)
.github/workflows/pull-request.yml (2)
13-33: Enhance build job security and reliability
The previous review comment about enhancing the build job's security and reliability is still valid. Please implement the suggested changes for:
- Gradle wrapper validation
- Timeout limits
- Build scan capability
35-54: Enhance frontend job reliability
The previous review comment about enhancing the frontend job's reliability is still valid. Please implement the suggested timeout limits and fix the missing newline at EOF.
🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 54-54: no new line character at the end of file
(new-line-at-end-of-file)
sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts (3)
32-36: LGTM! Good use of readonly modifiers
The readonly modifiers on injected dependencies prevent accidental reassignment and follow best practices.
49-49: LGTM! Good type annotation
Adding the void return type improves type safety and consistency.
74-75: LGTM! Consistent method signature
The void return type and event emission are properly implemented.
sep490-frontend/src/app/modules/core/services/application.service.ts (3)
10-45: Well-structured interfaces following OIDC standards
The separation of user information into distinct scope interfaces follows OpenID Connect standards and provides good type safety. The extension of UserInfoData from UserInfoEmailScope and JwtPayload is a clean approach.
48-51: Good use of subscription management
Extending SubscriptionAwareComponent and implementing OnDestroy ensures proper cleanup of subscriptions, preventing memory leaks.
Line range hint 93-98: Well-implemented cleanup in postLogout
The postLogout method properly cleans up both session and local storage, and resets the user info state.
sep490-idp/src/main/java/sep490/idp/SecurityConfig.java (1)
74-85: LGTM: Updated Security Filter Chain Configuration
The security filter chain has been updated to include OpenID Connect (OIDC) configurations and exception handling. The implementation aligns with best practices and enhances the security of the application.
...frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts
Outdated
Show resolved
Hide resolved
...frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/pull-request.yml (1)
3-10: Consider adding branch protection rulesSince this workflow targets the
mainbranch, consider enabling branch protection rules to:
- Require status checks to pass before merging
- Require PR reviews before merging
- Prevent force pushes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/pull-request.yml(1 hunks)sep490-frontend/src/app/app.component.ts(2 hunks)sep490-frontend/src/app/modules/enterprise/enterprise.component.ts(3 hunks)sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sep490-frontend/src/app/app.component.ts
- sep490-frontend/src/app/modules/enterprise/enterprise.component.ts
🧰 Additional context used
🪛 yamllint (1.29.0-1)
.github/workflows/pull-request.yml
[error] 62-62: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (7)
.github/workflows/pull-request.yml (2)
13-39: Well-structured build job configuration!
The build job includes all essential elements:
- Matrix strategy for multiple modules
- Gradle wrapper validation
- Proper caching
- Build scanning
- Reasonable timeouts
57-59: Verify ESLint setup in frontend project
Let's ensure the frontend project has the necessary ESLint configuration and dependencies.
✅ Verification successful
Let me generate another verification script to check for the ESLint configuration file with the hidden flag and examine the Angular-specific ESLint setup.
ESLint is properly configured in the frontend project
The frontend project has a comprehensive ESLint setup with:
- A well-defined
.eslintrc.jsonconfiguration file - Required ESLint dependencies in package.json (
eslint,@typescript-eslint/eslint-plugin,angular-eslint) - Configured lint script (
ng lint) - Extensive rule sets for TypeScript, Angular, and HTML files
- Proper integration with Prettier for code formatting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ESLint setup in frontend project
# Check for ESLint config
echo "Checking for ESLint config files..."
fd -t f "^\.eslintrc\.(js|json|yaml|yml)$" sep490-frontend/
# Check for ESLint in package.json
echo "Checking ESLint dependencies and scripts..."
if [ -f "sep490-frontend/package.json" ]; then
jq '.devDependencies | keys | select(.[]=="eslint")' sep490-frontend/package.json
jq '.scripts.lint' sep490-frontend/package.json
fi
Length of output: 1109
Script:
#!/bin/bash
# Check for hidden ESLint config files and Angular ESLint configuration
echo "Checking for ESLint config files (including hidden)..."
fd -H -t f "^\.eslintrc\.(js|json|yaml|yml)$" sep490-frontend/
# Check Angular ESLint configuration in angular.json
echo "Checking Angular ESLint configuration..."
if [ -f "sep490-frontend/angular.json" ]; then
jq '.projects.*.architect.lint' sep490-frontend/angular.json
fi
# Display ESLint config content if exists
echo "ESLint config content (if exists):"
find sep490-frontend -maxdepth 1 -type f -name ".eslintrc*" -exec cat {} \;
Length of output: 5799
sep490-frontend/src/app/modules/shared/components/dialog/confirm-dialog/confirm-dialog.component.ts (5)
29-30: LGTM! Output decorator follows Angular best practices
The changes correctly implement the previous review suggestions by making the output readonly and following proper naming conventions.
32-36: LGTM! Constructor follows Angular dependency injection best practices
The constructor parameters are properly decorated with access modifiers and readonly where appropriate, improving code safety and maintainability.
45-45: LGTM! Method signatures and implementations are consistent
The changes improve type safety and method naming clarity:
- Added explicit void return type to reject()
- Renamed hide to hideByConfirmEventType for better clarity
- Consistent implementation between accept() and reject() methods
Also applies to: 49-54
67-68: LGTM! Property check uses safe Object.hasOwn method
The implementation correctly uses Object.hasOwn for safe property checking, following modern JavaScript best practices.
74-75: LGTM! Well-encapsulated private method
The new private method properly encapsulates the hide event emission and state reset logic with clear naming and typing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/pull-request.yml (3)
3-15: Enhance workflow configurationConsider these improvements to the workflow configuration:
- Add pull-request write permission for PR status checks
- Add concurrency control to cancel outdated runs
permissions: contents: read statuses: write + pull-requests: write + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
45-47: Improve job name clarityThe current job name "Check Build" is not descriptive enough. Consider renaming it to better reflect its purpose.
build-frontend: - name: Check Build + name: Build Frontend Application
64-66: Add missing newline at end of fileAdd a newline at the end of the file to comply with YAML best practices.
working-directory: sep490-frontend +🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 66-66: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
.github/workflows/pull-request.yml (1)
Learnt from: thongdanghoang
PR: thongdanghoang/sep490#15
File: .github/workflows/pull-request.yml:41-62
Timestamp: 2024-11-24T11:42:06.877Z
Learning: In the `sep490-frontend` project, the `build` script in `package.json` includes `ng lint`, so linting is already performed during the build step. There's no need to add a separate lint step in the GitHub Actions workflow.
🪛 yamllint (1.29.0-1)
.github/workflows/pull-request.yml
[error] 66-66: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
16-44: Build job configuration looks solid!
The build job includes all essential elements:
- Matrix strategy for multiple modules
- Proper Java setup
- Gradle caching
- Wrapper validation
- Build timeout
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pull-request.yml (2)
1-15: Consider a more descriptive workflow nameThe current name "PR Review" could be more specific about what the workflow actually does. Consider renaming it to better reflect its purpose, such as "PR Build and Lint Check".
-name: PR Review +name: PR Build and Lint Check
45-66: Add environment variable for working directoryConsider defining the working directory as an environment variable at the job level to avoid repetition and make maintenance easier.
build-frontend: name: Build Frontend runs-on: ubuntu-latest + env: + FRONTEND_DIR: sep490-frontend steps: - uses: actions/checkout@v4 - name: Use Node.js uses: actions/setup-node@v4 with: node-version: '20.x' - name: Cache npm uses: actions/cache@v3 with: path: ~/.npm key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }} restore-keys: | ${{ runner.os }}-node- - run: npm ci timeout-minutes: 5 - working-directory: sep490-frontend + working-directory: ${{ env.FRONTEND_DIR }} - run: npm run build timeout-minutes: 5 - working-directory: sep490-frontend + working-directory: ${{ env.FRONTEND_DIR }} +🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 66-66: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.29.0-1)
.github/workflows/pull-request.yml
[error] 66-66: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
16-44: Well-structured Java build configuration!
The build job implementation includes all the essential elements:
- Matrix strategy for multiple modules
- Proper Gradle caching
- Security measures with wrapper validation
- Build scanning for debugging
- Appropriate timeouts
* bugfix: fix lint feat: eslint check when create PR feat: eslint and prettier feat: eslint to angular * feat: check build on FE instead of lint * bugfix: fix PR * bugfix: add update status permissions * chores: rename build name --------- Co-authored-by: Thống ĐẶNG HOÀNG <thongdh3401@gmail.com>
* bugfix: fix lint feat: eslint check when create PR feat: eslint and prettier feat: eslint to angular * feat: check build on FE instead of lint * bugfix: fix PR * bugfix: add update status permissions * chores: rename build name --------- Co-authored-by: Thống ĐẶNG HOÀNG <thongdh3401@gmail.com>
* setup base mobile: add demo dart/light mode, i10n, OIDC * feat: eslint check when create PR (#15) * bugfix: fix lint feat: eslint check when create PR feat: eslint and prettier feat: eslint to angular * feat: check build on FE instead of lint * bugfix: fix PR * bugfix: add update status permissions * chores: rename build name --------- Co-authored-by: Thống ĐẶNG HOÀNG <thongdh3401@gmail.com> * Feature: introduce release pipe automatically push image to docker hub * feat: release pipeline --------- Co-authored-by: Thống ĐẶNG HOÀNG <thongdh3401@gmail.com> * setup base flutter with dependency inject * setup base full * first * first * feat: eslint check when create PR (#15) * bugfix: fix lint feat: eslint check when create PR feat: eslint and prettier feat: eslint to angular * feat: check build on FE instead of lint * bugfix: fix PR * bugfix: add update status permissions * chores: rename build name --------- Co-authored-by: Thống ĐẶNG HOÀNG <thongdh3401@gmail.com> * Feature: introduce release pipe automatically push image to docker hub * feat: release pipeline --------- Co-authored-by: Thống ĐẶNG HOÀNG <thongdh3401@gmail.com> * docs: move reports to Google drive * docs: UC-AUTHENTICATION * docs: Ready for UI/UX design * docs: use company name and hotline instead * docs: add more logout and oidc client user stories * bugfix: fix PR --------- Co-authored-by: Thống ĐẶNG HOÀNG <work@thongdanghoang.id.vn> Co-authored-by: Thống ĐẶNG HOÀNG <thongdh3401@gmail.com> Co-authored-by: dano <thong.danghoang@elca.vn>
feat: fix lint
feat: eslint and prettier
feat: eslint to angular
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
package.jsonto include new linting scripts and dependencies.Refactor
ModalProviderclass.Style
Chores