mirror of
https://github.com/Alfresco/alfresco-ng2-components.git
synced 2026-09-09 18:03:21 +00:00
[ACS-11173] Initial set of rules for Copilot review (#11683)
* [ACS-11173] Initial set of rules for Copilot review * [ACS-11173] CR fix
This commit is contained in:
@@ -0,0 +1,78 @@
|
||||
# General Code Review Instructions
|
||||
|
||||
## Review Priorities
|
||||
When performing a code review, prioritize issues in the following order:
|
||||
|
||||
🔴 CRITICAL (Block merge)
|
||||
* Security: Vulnerabilities, exposed secrets, authentication/authorization issues, injection, XSS
|
||||
* Memory/resource leaks
|
||||
* Crashes or undefined behavior
|
||||
* Correctness: Logic errors, data corruption risks, race conditions, missing error handling
|
||||
* Data Loss: Risk of data loss or corruption
|
||||
|
||||
🟡 IMPORTANT (Requires discussion)
|
||||
* Code Quality: Severe violations of SOLID principles, excessive duplication
|
||||
* Test Coverage: Missing tests for critical paths or new functionality
|
||||
* Performance: Obvious performance bottlenecks (N+1 queries, memory leaks)
|
||||
* Architecture: Significant deviations from established patterns
|
||||
|
||||
🟢 SUGGESTION (Non-blocking improvements)
|
||||
* Readability: Poor naming, complex logic that could be simplified
|
||||
* Optimization: Performance improvements without functional impact
|
||||
* Best Practices: Minor deviations from conventions
|
||||
* Documentation: Missing or incomplete comments/documentation
|
||||
|
||||
## General Review Principles
|
||||
When performing a code review, follow these principles:
|
||||
|
||||
* Be specific: Reference exact lines, files, and provide concrete examples
|
||||
* Be pragmatic: Not every suggestion needs immediate implementation
|
||||
* Provide context: Explain WHY something is an issue and the potential impact
|
||||
* Suggest solutions: Show corrected code when applicable, not just what's wrong
|
||||
* Be constructive: Focus on improving the code, not criticizing the author
|
||||
* Recognize good practices: Acknowledge well-written code and smart solutions
|
||||
* Group related comments: Avoid multiple comments about the same topic
|
||||
* Ask clarifying questions when code intent is unclear
|
||||
|
||||
## Code Quality Standards
|
||||
When performing a code review, check for:
|
||||
|
||||
### Clean Code
|
||||
* Descriptive and meaningful names for variables, functions, and classes
|
||||
* Single Responsibility Principle: each function/class does one thing well
|
||||
* DRY (Don't Repeat Yourself): no code duplication
|
||||
* Functions should be small and focused (ideally < 20-30 lines)
|
||||
* Avoid deeply nested code (max 3-4 levels), unit tests should be an exception from this rule
|
||||
* Avoid magic numbers and strings (use constants)
|
||||
* Code should be self-documenting; comments only when necessary
|
||||
* Code follows consistent style and conventions
|
||||
* No commented-out code or TODO without tickets
|
||||
|
||||
## Error Handling
|
||||
* Proper error handling at appropriate levels
|
||||
* Meaningful error messages
|
||||
* No silent failures or ignored exceptions
|
||||
* Fail fast: validate inputs early
|
||||
* Use appropriate error types/exceptions
|
||||
|
||||
## Testing Standards
|
||||
When performing a code review, verify test quality:
|
||||
|
||||
* Coverage: Critical paths and new functionality must have tests
|
||||
* Test Names: Descriptive names that explain what is being tested
|
||||
* Independence: Tests should not depend on each other or external state
|
||||
* Assertions: Use specific assertions, avoid generics
|
||||
* Edge Cases: Test boundary conditions, null values, empty inputs
|
||||
* Mock Appropriately: Mock external dependencies, not domain logic
|
||||
* New code has appropriate test coverage
|
||||
* No tests that always pass or are commented out
|
||||
|
||||
## Architecture and Design
|
||||
When performing a code review, verify architectural principles:
|
||||
|
||||
* Separation of Concerns: Clear boundaries between layers/modules
|
||||
* Dependency Direction: High-level modules don't depend on low-level details
|
||||
* Interface Segregation: Prefer small, focused interfaces
|
||||
* Loose Coupling: Components should be independently testable
|
||||
* High Cohesion: Related functionality grouped together
|
||||
* Consistent Patterns: Follow established patterns in the codebase
|
||||
@@ -0,0 +1,23 @@
|
||||
---
|
||||
applyTo: "**/*.html"
|
||||
---
|
||||
|
||||
# HTML Development Standards
|
||||
|
||||
* Simple Templates: Keep templates as simple as possible, avoiding complex logic directly in the template. Delegate complex logic to the component's TypeScript code.
|
||||
* Native Control Flow: Use the new built-in control flow syntax (`@if`, `@for`, `@switch`) instead of the older structural directives (`*ngIf`, `*ngFor`, `*ngSwitch`).
|
||||
* NgOptimizedImage: Use `NgOptimizedImage` for all static images to automatically optimize image loading and performance.
|
||||
* Async Pipe: Use the `async` pipe to handle observables in templates. This automatically subscribes and unsubscribes, preventing memory leaks.
|
||||
* Prefer pipes over functions in templates for performance reasons, as pipes are only re-evaluated when their inputs change.
|
||||
|
||||
## Accessibility Standards
|
||||
|
||||
* Add `alt` text to all images
|
||||
* Label form inputs with `<mat-label>` or `aria-label`
|
||||
* Ensure interactive elements have accessible names
|
||||
* Add `role`, `aria-labelledby`, and `aria-describedby` when semantic HTML isn't sufficient
|
||||
* All interactive elements must be keyboard accessible
|
||||
* Ensure 4.5:1 contrast ratio for normal text, 3:1 for large text
|
||||
* Use `aria-live="polite"` for status updates
|
||||
* Watch out for misused/non-semantic elements (e.g., <div> instead of <section>)
|
||||
* Avoid broken heading hierarchy (e.g., h1 → h3 without h2)
|
||||
@@ -0,0 +1,19 @@
|
||||
---
|
||||
applyTo: "**/*.scss"
|
||||
---
|
||||
|
||||
# SCSS Development Standards
|
||||
|
||||
* Avoid using `!important` to override styles unless absolutely necessary; instead, increase specificity or refactor the code structure.
|
||||
* Avoid using Angular Material internal class; prefer using Angular Material Design 3 theming and tokens.
|
||||
* Use variables for colors, fonts, and other design tokens to maintain consistency across the project.
|
||||
* Use mixins for reusable styles and to avoid code duplication.
|
||||
* Ensure that styles are responsive and work well across different screen sizes and devices.
|
||||
* Use CSS Grid and Flexbox for layout to create flexible and responsive designs.
|
||||
* Avoid using overly specific selectors (e.g., #header .nav ul li a)
|
||||
* Be DRY (avoid repeated styles for similar elements)
|
||||
* Check for inconsistent naming (e.g., mixing BEM and arbitrary classes)
|
||||
* Make sure selected colors have right contrast (satisfy WCAG AA)
|
||||
* All interactive elements should have focus state
|
||||
* Avoid disabled outline without alternative focus indicators
|
||||
|
||||
@@ -0,0 +1,96 @@
|
||||
---
|
||||
applyTo: "**/*.ts"
|
||||
---
|
||||
|
||||
# TypeScript Development Standards
|
||||
|
||||
## Type Safety
|
||||
|
||||
* Strict Type Checking: Always enable and adhere to strict type checking. This helps catch errors early and improves code quality.
|
||||
* Prefer Type Inference: Allow TypeScript to infer types when they are obvious from the context. This reduces verbosity while maintaining type safety.
|
||||
* Avoid `any`: Do not use the `any` type unless absolutely necessary as it bypasses type checking. Prefer `unknown` when a type is uncertain and you need to handle it safely.
|
||||
* Use strict null checks (no `null` or `undefined` without explicit handling)
|
||||
* Use type guards and union types for robust type checking
|
||||
* Check for missing return types in function signatures
|
||||
* Avoid implicit `any` (untyped function parameters)
|
||||
|
||||
## Naming Conventions
|
||||
|
||||
* Use PascalCase for types, interfaces, and classes
|
||||
* Use camelCase for variables, functions, and methods
|
||||
* Use UPPER_CASE for constants
|
||||
|
||||
## Modern TypeScript Patterns
|
||||
|
||||
* Use optional chaining (`?.`) and nullish coalescing (`??`)
|
||||
* Prefer `const` over `let`; never use `var`
|
||||
* Use arrow functions for callbacks and short functions
|
||||
* Avoid enums - they generate additional code at compile time, which increases the size of the final file. This can have a negative impact on the loading speed and performance of the app. Prefer union types or literal types instead.
|
||||
* Avoid unhandled promise rejections (missing .catch() or try/catch)
|
||||
* Use proper async/await pattern
|
||||
* Avoid inefficient array operations (e.g., nested .map())
|
||||
* Use destructuring for object/array access
|
||||
* Prefer arrow functions
|
||||
|
||||
## Angular Best Practices
|
||||
|
||||
* Standalone Components: Always use standalone components, directives, and pipes. Avoid using `NgModules` for new features or refactoring existing ones.
|
||||
* Implicit Standalone: When creating standalone components, you do not need to explicitly set `standalone: true` inside the `@Component`, `@Directive` and `@Pipe` decorators, as it is implied by default.
|
||||
* Lazy Loading: Implement lazy loading for feature routes to improve initial load times of your application.
|
||||
* Use Angular Material or other modern UI libraries for consistent styling and UI components.
|
||||
* Implement proper error handling with RxJS operators (e.g., catchError)
|
||||
* Verify if newly added functionalities can utilize Angular Signals for fine-grained reactivity, reducing change detection overhead.
|
||||
* Utilize AOT (Ahead-of-Time) compilation and tree-shaking for efficient, smaller bundle sizes.
|
||||
* Prefer class binding over `ngClass` and `ngStyle` for better performance.
|
||||
* Use protected on class members that are only used by a component's template, as it allows for better encapsulation while still being accessible to the template.
|
||||
* Use readonly for properties that shouldn't change.
|
||||
* Use `takeUntilDestroyed` & `destroyRef`: The `takeUntilDestroyed` and `destroyRef` have been introduced with Angular 16 and help to reduce boilerplate code related to unsubscribing on the `OnDestroy` hook.
|
||||
* Organize the order of properties and methods in Angular components for readability and maintainability. Recommended order is:
|
||||
1. **Injected services** Whether they are public or private, it's clear they are dependencies of the class.
|
||||
2. **Inputs**: Properties that receive data from outside.
|
||||
3. **Outputs**: Events that the component can trigger.
|
||||
4. **ViewChild/ContentChild**: References to HTML elements.
|
||||
5. **Public static properties**: Constants and static members that are accessible to everyone.
|
||||
6. **Readonly properties**: Immutable public properties.
|
||||
7. **Public properties**: Data and functions available to everyone.
|
||||
8. **Private static properties**: Constants and static members that are only accessible within the class.
|
||||
9. **Private readonly properties**: Immutable private properties.
|
||||
10. **Private properties**: Data and functions used only inside the component.
|
||||
11. **Setters and Getters**: Methods for accessing and modifying properties.
|
||||
12. **Constructor**: Used to initialize the component.
|
||||
13. **Lifecycle Hooks**: Methods that run at specific times in the component’s lifecycle.
|
||||
14. **Public methods**: Functions available to everyone.
|
||||
15. **Private methods**: Functions used only inside the component.
|
||||
|
||||
## Components
|
||||
|
||||
* Single Responsibility: Keep components small, focused, and responsible for a single piece of functionality.
|
||||
* Reactive Forms: Prefer Reactive forms over Template-driven forms for complex forms, validation, and dynamic controls due to their explicit, immutable, and synchronous nature.
|
||||
* Use Typed Forms: Typed Forms in Angular are a new feature introduced in Angular 14 that provide stronger type checking for reactive forms. They allow developers to define the structure and types of form controls, making it easier to catch errors at compile-time rather than runtime.
|
||||
|
||||
## Services
|
||||
|
||||
* Single Responsibility: Design services around a single, well-defined responsibility.
|
||||
* `providedIn: 'root'`: Use the `providedIn: 'root'` option when declaring injectable services to ensure they are singletons and tree-shakable.
|
||||
* `inject()` Function: Prefer the `inject()` function over constructor injection when injecting dependencies, especially within `provide` functions, `computed` properties, or outside of constructor context.
|
||||
|
||||
## Unit testing
|
||||
|
||||
* Write unit tests for components, services, and pipes using Jasmine and Karma.
|
||||
* Test cases should be reasonably groupped based on tested functionality/behaviour using describe blocks.
|
||||
* Use plain English test names based on the should <expectedBehavior> when <stateUnderTest> pattern as a guideline.
|
||||
* Use Angular's TestBed for component testing with mocked dependencies
|
||||
* Avoid Direct Calls to Component Lifecycle Hooks: Instead of directly invoking lifecycle hooks like `ngOnInit()`, use Angular's testing utilities to trigger them naturally. For example, use `fixture.detectChanges()` to trigger change detection, which will automatically call `ngOnInit()` and other lifecycle hooks in the correct order.
|
||||
* Use fixture.componentRef.setInput() Instead of Direct Input Assignment: When testing components with inputs, use `fixture.componentRef.setInput()` to set input values. This method ensures that Angular's change detection is properly triggered, allowing the component to react to input changes as it would in a real application.
|
||||
* Use the Provide Mock Store for testing components that rely on NgRx state management. This allows you to mock the store and control the state during tests without needing to set up a full NgRx environment.
|
||||
* Mock HTTP requests using provideHttpClientTesting
|
||||
* Import only the minimal required modules
|
||||
* Avoid NO_ERRORS_SCHEMA and CUSTOM_ELEMENTS_SCHEMA in tests to ensure proper error detection
|
||||
* Do not verify mocked methods
|
||||
* Avoid mocking component methods unless necessary; prefer testing actual behavior
|
||||
* Avoid testing private methods directly; test them through public methods instead
|
||||
* Avoid testing methods or behaviours of children components; use shallow testing or mock child components instead
|
||||
* Use the overrideProviders API to replace components, directives, pipes, or services declared deep within the module hierarchy
|
||||
* Avoid async/await in synchronous unit tests
|
||||
* Prefer data-automation-id over CSS class when possible
|
||||
* Do not use toBeDefined() to check if an element is visible.
|
||||
Reference in New Issue
Block a user