feat(core): migrate to Angular 19, fix audit vulnerabilities, and convert components to standalone, replace input output decorators with signal-based inputs and add trivy workflow #98
Conversation
…vert components to standalone GH-79
piyushsinghgaur1
force-pushed
the
GH-97
branch
from
2d6d01f to
3f47e3a
Compare
|
==== Karma Test Coverage ==== |
…and add trivy workflow replace input output decorators with signal-based inputs and add trivy workflow BREAKING CHANGE: Yes GH-97
piyushsinghgaur1
force-pushed
the
GH-97
branch
from
3f47e3a to
c8d4f6c
Compare
|
==== Karma Test Coverage ==== |
added sandbox and fix minor bugs GH-97
|
==== Karma Test Coverage ==== |
rohit-sourcefuse
left a comment
rohit-sourcefuse
left a comment
There was a problem hiding this comment.
🎯 APPROVE WITH CHANGES - Excellent Angular 19 Migration
This is outstanding migration work that demonstrates deep understanding of Angular 19 patterns and modern best practices. The architectural improvements are substantial and well-executed. However, there are several critical issues that need fixing before merge.
Critical Issues (Must Fix):
1. 🔴 Duplicate ngClass Binding
File: projects/workflows-creator/src/lib/builder/group/group.component.html Line: 47
Remove the duplicate [ngClass] binding (Angular will only use the last one):
<workflow-node
[ngClass]="{'last-node': i === group().children.length - 1}"
[node]="node"
[ngClass]="{'last-node': i === group().children.length - 1}" <!-- REMOVE THIS -->
[isLast]="i === group().children.length - 1"2. 🔴 Memory Leak: Unmanaged Overlay Subscriptions
Files: builder.component.ts:459, group.component.ts:385,520
The backdrop click subscriptions will cause memory leaks:
// Current problematic code:
this.overlayRef.backdropClick().subscribe(() => this.closeOverlay());
// Fix Option 1: Proper cleanup
private subscription?: Subscription;
ngOnDestroy() {
this.subscription?.unsubscribe();
}
// Fix Option 2: Auto-complete
this.overlayRef.backdropClick().pipe(take(1)).subscribe(() => this.closeOverlay());3. 🔴 Missing Trivy Configuration File
File: .github/workflows/trivy.yml Line: 29
The workflow references a non-existent config file:
trivy-config: "${{ github.workspace }}/trivy.yaml" # File doesn't existFix: Either add the trivy.yaml file or remove this line to use Trivy defaults.
4. 🔴 Unused Import Cleanup
File: group.component.spec.ts Line: 37
import {popper} from '@popperjs/core'; // Not used - remove thisImportant Issues (Should Fix):
5. Missing OnDestroy Implementation
Both BuilderComponent and GroupComponent should implement OnDestroy:
export class BuilderComponent<E> implements OnInit, OnDestroy {
ngOnDestroy(): void {
this.closeOverlay();
}
}6. Effect Error Handling
File: builder.component.ts Lines: 98-127
Consider adding error handling to the signal effect:
effect(() => {
try {
// existing logic
} catch (error) {
console.error('Effect error in BuilderComponent:', error);
// Handle gracefully - maybe reset state or show user error
}
});✅ Excellent Migration Highlights:
🎉 Perfect Signal Implementation:
- Proper use of
input(),input.required(), andoutput() - Clean migration from
@Input/@Outputdecorators - Correct signal reactivity patterns
🎉 Outstanding Standalone Components:
- Well-implemented with proper
importsarrays - Clean removal of NgModule dependencies
- Excellent
provideWorkflowCreator()provider function
🎉 Solid CDK Integration:
- Good migration from ngx-popperjs to Angular CDK overlays
- Proper overlay positioning and management
- Clean template updates
🎉 Modern Build System:
- Correct Angular 19 builder configuration (
@angular-devkit/build-angular:application) - Smart
bundle-element.tsupdates for hashed output files - Good CI/CD improvements with
--ignore-scripts
🎉 Security & Dependencies:
- Thoughtful dependency pinning with
overrides - Good Trivy security scanning addition (once config is fixed)
- Clean SCSS migration to
@usesyntax
Test Coverage Notes:
Consider adding tests for:
- Signal effect behavior and error handling
- Overlay subscription cleanup verification
- Component destruction scenarios
- Signal reactivity edge cases
Minor Cleanup:
- Delete the empty
workflow-builder.module.tsfile entirely
Overall Assessment:
This migration showcases excellent Angular expertise. The architectural improvements are significant:
- Clean separation with standalone components
- Modern reactive patterns with signals
- Better maintainability with CDK overlays
- Improved build pipeline
Once the critical issues are addressed (especially memory leaks and duplicate bindings), this will be a stellar migration that sets a great foundation for Angular 19.
Great work on tackling such a comprehensive framework upgrade! 🚀
📋 Quick Action ChecklistFor faster resolution, here's a prioritized checklist of the critical fixes: Priority 1 - Breaking Issues:
Priority 2 - Memory Management:
Priority 3 - Error Handling:
Optional Cleanup:
The Priority 1 items will cause immediate build/runtime issues, so tackle those first. The memory management fixes (Priority 2) are critical for production stability. Excellent work on this migration - these are just polish items on an otherwise outstanding Angular 19 upgrade! 🎯 |
There was a problem hiding this comment.
please remove the extra comments
…s, trivy, imports, and effect errors
|
==== Karma Test Coverage ==== |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Angular codebase by upgrading to Angular 19, refactoring components to use standalone architecture and signal-based inputs, replacing ngx-popperjs with Angular CDK overlays, and enhancing CI/CD workflows with security scanning and improved dependency management.
Changes:
- Upgraded Angular and related dependencies to v19 with standalone component architecture and signal-based inputs/outputs
- Replaced
ngx-popperjswith Angular CDK overlay system for improved maintainability - Added Trivy security scanning workflow and updated CI/CD workflows to use
--ignore-scriptsflag - Refactored bundling script to handle new Angular build output structure with hashed filenames
Reviewed changes
Copilot reviewed 69 out of 85 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| workflow-sandbox/* | New standalone Angular 19 sandbox application with modern configuration |
| web-component-sandbox/* | Updated web component sandbox with zone.js patches and updated styling |
| projects/workflows-element/package.json | Updated hash for rebuilt web component bundle |
| projects/workflows-creator/src/lib/builder/* | Converted components to standalone with signal-based inputs, replaced popper with CDK overlays |
| projects/workflows-creator/src/lib/provide-workflow-creator.ts | New provider function replacing WorkflowBuilderModule for Angular 19 |
| projects/workflows-creator/src/zone-flags.ts | Added zone.js configuration to fix hasOwnProperty errors |
| projects/workflows-creator/bundle-element.ts | Refactored to handle Angular 19 hashed build outputs |
| package.json, angular.json | Updated dependencies to Angular 19 and modernized build configuration |
| .github/workflows/* | Added Trivy scan and updated workflows with security improvements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new ValueInput(), | ||
| ); | ||
| expect(component.tooltipText).toBe('Select a column first'); | ||
| expect(component.tooltipText).toBe('selectColumnTooltip'); |
There was a problem hiding this comment.
The expected value changed from 'Select a column first' to 'selectColumnTooltip', which appears to be a localization key rather than the actual displayed text. This test should verify the localized output, not the key itself.
| const context = require.context('./', true, /\.spec\.ts$/); | ||
| // And load the modules. | ||
| context.keys().map(context); | ||
| const req: any = require as any; |
There was a problem hiding this comment.
Using double type assertion as any suppresses type checking. Consider defining a proper interface for the require context or using a more specific type guard to maintain type safety.
| // resul in changing its hash, so the element will also be published | ||
|
|
||
| const file = '../workflows-element/dist/workflows-element.js'; | ||
| const distBase = './projects/workflows-creator/dist/workflows-element'; |
There was a problem hiding this comment.
The base path is hardcoded. Consider extracting this and other paths to constants at the top of the file or accepting them as configuration to improve maintainability and reusability.
| break; | ||
| case window.IntervalInput.identifier: | ||
| const timescale = TIMESCALE.find( | ||
| // brekets |
There was a problem hiding this comment.
Corrected spelling of 'brekets' to 'brackets'.
| // brekets | |
| // brackets |
| padding: 5px 0; | ||
| @media screen and (max-width: 480px) { | ||
| margin-bottom: -25px; | ||
| margin-bottom: 8px; |
There was a problem hiding this comment.
The margin-bottom value changed from -25px to 8px for mobile screens. Ensure this change is intentional and doesn't break the mobile layout, as the negative margin may have been used for specific spacing adjustments.
| margin-bottom: 8px; | |
| margin-bottom: -25px; |
| <div class="workflow-group"> | ||
| <div class="workflow-group-content"> | ||
| <span *ngIf="!isFirst"> {{ group.name }} </span> | ||
| <span *ngIf="!isFirst() && group().children.length > 0"> |
There was a problem hiding this comment.
The condition now includes && group().children.length > 0, which prevents showing the group name when there are no children. Verify this is the intended behavior change and that it doesn't hide necessary labels in the UI.
| <span *ngIf="!isFirst() && group().children.length > 0"> | |
| <span *ngIf="!isFirst()"> |
This pull request introduces significant updates to modernize the Angular codebase, improve the build and packaging process, and enhance CI/CD workflows. The most notable changes include upgrading Angular and related dependencies to v19, refactoring the custom element bundling script for compatibility with new Angular output formats, updating build configurations, and improving test and workflow reliability.
Major dependency and build system upgrades:
@angular/*,@ng-select/ng-select,lerna,rxjs,zone.js, etc.) to version 19 in bothpackage.jsonandprojects/workflows-creator/package.json. This also includes devDependencies and TypeScript updates for compatibility. [1] F9c4fafeL9R10, [2] [3]angular.jsonto use the new Angular v19 builder (@angular-devkit/build-angular:application), revised build options for output compatibility, and updated schematic collections. [1] [2] [3]Element bundling and packaging improvements:
bundle-element.tsto support the new Angular build output structure (with hashed chunk files), improved robustness by dynamically detecting output files, and ensured correct copying of styles and README. The hash calculation and package.json update logic were also improved.elementBuildandpostbuildscripts in both root and package-levelpackage.jsonfiles to use the new bundling approach and ensure correct working directories. [1] [2]Continuous Integration and workflow enhancements:
trivy.yml) to run Trivy security scans on pull requests targeting themasterbranch.actions/cache@v4), and changednpm cisteps to use--ignore-scriptsfor improved security and reproducibility. [1] [2] [3]Code and test modernization:
ngx-popperjsusage from builder tests and templates, replacing popper-based overlays with Angular CDK overlays for better compatibility and maintainability. Updated test mocks and input handling accordingly. [1] [2] [3] [4] [5]@usesyntax instead of@import.Other improvements:
overridesinpackage.jsonto pin certain transitive dependencies for security and compatibility.Dependency and Build System Upgrades
package.jsonfiles. [1] F9c4fafeL9R10, [2] [3]angular.jsonbuild and schematic configuration to match Angular v19 requirements and output structure. [1] [2] [3]Element Bundling and Packaging
bundle-element.tsto dynamically detect and concatenate Angular build output files, supporting hashed filenames and copying assets robustly.CI/CD and Workflow Improvements
master.actions/cache@v4and runnpm ci --ignore-scriptsfor improved reliability. [1] [2] [3]Code and Test Modernization
ngx-popperjsoverlays with Angular CDK overlays in builder components and tests, and updated test mocks for new Angular versions. [1] [2] [3] [4] [5]@usesyntax.Security and Dependency Management
overridesinpackage.jsonto pin specific vulnerable or problematic transitive dependencies.