Feat/angular roadmap #424
Feat/angular roadmap #424
Conversation
WalkthroughThis set of changes introduces a comprehensive Angular "roadmap" feature for a blog application. It adds new feature and UI libraries, components, directives, pipes, configuration files, localization, and assets to visualize a hierarchical Angular learning roadmap. The update also configures testing, linting, and workspace settings, and integrates the roadmap into the application's routing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router
participant RoadmapShellComponent
participant FeatureRoadmapComponent
participant HttpClient
participant SVGPanZoom
User->>Router: Navigates to '/roadmap'
Router->>RoadmapShellComponent: Instantiates
RoadmapShellComponent->>FeatureRoadmapComponent: Displays feature roadmap
FeatureRoadmapComponent->>HttpClient: Loads roadmap-tiles.json
HttpClient-->>FeatureRoadmapComponent: Returns roadmap data
FeatureRoadmapComponent->>FeatureRoadmapComponent: Transforms data into layers
FeatureRoadmapComponent->>SVGPanZoom: Initializes SVG pan/zoom
User->>FeatureRoadmapComponent: Interacts with roadmap controls (zoom, reset)
FeatureRoadmapComponent->>SVGPanZoom: Handles zoom/reset events
Poem
|
|
PR is detected, will deploy to dev environment |
|
Deployed to dev environment |
![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
🧹 Nitpick comments (15)
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-angular-love-node.component.ts (1)
9-12: Consider extracting CSS variables to a shared fileThe CSS variables defined inline are duplicated across multiple node components (primary, secondary, and this one) as seen in the relevant code snippets.
Consider extracting these variables to a shared styles file to maintain consistency and make theme changes easier. This would reduce duplication across your node components.
- style=" - --primary-color: #B3004A; --secondary-color: #66002B; --gradient-color: #481CAB; --on-hover-border-1: #923CFF; --on-hover-border-2: #FF006A" + class="roadmap-node-theme"Then define these variables in a shared SCSS file that all node components can import.
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.spec.ts (1)
1-22: Basic test structure looks good, but could benefit from more comprehensive test coverage.The test file correctly sets up the TestBed configuration and verifies component creation. However, for a component that manages a complex roadmap structure with SVG visuals, consider adding more test cases to verify:
- Proper rendering of different node types
- Correct layer calculations
- Edge cases like empty layers or single nodes
// Example of additional tests you might want to add: it('should display layers correctly when data is provided', () => { // Setup test data component.roadmapData = mockRoadmapData; fixture.detectChanges(); // Assert expected DOM elements const layerElements = fixture.nativeElement.querySelectorAll('.layer'); expect(layerElements.length).toBe(mockRoadmapData.length); }); it('should handle empty roadmap data gracefully', () => { // Test with empty data component.roadmapData = []; fixture.detectChanges(); // Assert appropriate handling expect(fixture.nativeElement.querySelector('.no-data-message')).toBeTruthy(); });libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-primary-node.component.ts (1)
5-21: Component template could benefit from improved accessibility and maintainability.The component is well-structured, but there are potential improvements:
- The hardcoded colors in the style attribute should be moved to a theme file for better maintainability
- Consider adding ARIA attributes for improved accessibility
- Color contrast should be verified for WCAG compliance
@Component({ selector: 'al-ui-roadmap-primary-node', template: ` <div class="roadmap-hover-border-gradient relative w-fit text-nowrap rounded-lg bg-[#FDF5FD] text-[#FDF5FD]" - style=" - --primary-color: #B3004A; --secondary-color: #66002B; --gradient-color: #481CAB; --on-hover-border-1: #923CFF; --on-hover-border-2: #FF006A" + [ngClass]="'theme-primary'" + role="listitem" + aria-label="Primary roadmap node: {{node().title}}" > <div class="relative z-10 m-[4px] rounded-lg bg-[--primary-color] px-6 py-4" > <div class="text-[24px]">{{ node().title }}</div> </div> </div> `, styleUrls: ['./roadmap-hover-border-gradient.scss'], })Then define theme classes in your SCSS file:
// In roadmap-hover-border-gradient.scss .theme-primary { --primary-color: #B3004A; --secondary-color: #66002B; --gradient-color: #481CAB; --on-hover-border-1: #923CFF; --on-hover-border-2: #FF006A; }libs/blog/roadmap/feature-roadmap/src/lib/slice.pipes.ts (3)
3-11: Good implementation but consider making the pipe standalone.The LeftSlicePipe implementation is clean and functional. For modern Angular applications (v14+), it's recommended to make pipes standalone.
@Pipe({ name: 'leftSlice', + standalone: true, }) export class LeftSlicePipe implements PipeTransform { transform<T>(value: T[]): T[] { const halfLength = Math.ceil(value.length / 2); return value.slice(0, halfLength); } }
13-21: Good implementation but consider making the pipe standalone.The RightSlicePipe implementation is clean and functional. For modern Angular applications (v14+), it's recommended to make pipes standalone.
@Pipe({ name: 'rightSlice', + standalone: true, }) export class RightSlicePipe implements PipeTransform { transform<T>(value: T[]): T[] { const halfLength = Math.ceil(value.length / 2); return value.slice(halfLength); } }
7-10: Consider handling edge cases in pipe implementations.While the pipe implementations work for valid arrays, they don't handle edge cases like null or undefined values.
// For LeftSlicePipe transform<T>(value: T[]): T[] { + if (!value || !Array.isArray(value)) { + return []; + } const halfLength = Math.ceil(value.length / 2); return value.slice(0, halfLength); } // For RightSlicePipe transform<T>(value: T[]): T[] { + if (!value || !Array.isArray(value)) { + return []; + } const halfLength = Math.ceil(value.length / 2); return value.slice(halfLength); }Also applies to: 17-20
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html (1)
65-66: Type casting with $any() indicates a potential type mismatch.The use of
$any()for type casting suggests that the node type doesn't match the expected cluster type. This should be resolved with proper typing.// In the component class, add a type guard: isCluster(node: RoadmapNode): node is RoadmapCluster { return node.nodeType === 'cluster'; } // Then in the template: <al-ui-roadmap-cluster - [cluster]="$any(node)" + [cluster]="node" *ngIf="isCluster(node)" ></al-ui-roadmap-cluster>Also applies to: 77-78
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-secondary-node.component.ts (1)
5-21: Consider accessibility and theming improvementsThe component implementation is clean, but consider:
- The hardcoded color values like
#B3004A,#66002B, etc. might be better placed in a central theme file for easier maintenance- Verify that the text colors provide sufficient contrast with the background for accessibility (WCAG compliance)
You could refactor the inline styles to use application-wide CSS variables:
- style=" - --primary-color: #B3004A; --secondary-color: #66002B; --gradient-color: #481CAB; --on-hover-border-1: #923CFF; --on-hover-border-2: #FF006A" + style=" + --primary-color: var(--theme-primary-color, #B3004A); --secondary-color: var(--theme-secondary-color, #66002B); --gradient-color: var(--theme-gradient-color, #481CAB); --on-hover-border-1: var(--theme-hover-border-1, #923CFF); --on-hover-border-2: var(--theme-hover-border-2, #FF006A)"libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-cluster.component.ts (2)
5-26: Well-structured template with modern Angular syntaxGreat use of modern Angular control flow with the
@forloop and proper tracking by node ID. The component structure is clean and follows Angular best practices.As with the other component, consider:
- Extracting color values to a theme file
- Verifying color contrast for accessibility
27-32: Consider centralizing styling variablesThe host binding approach is good, but the inline color variables could be moved to a central theme:
- style: - '--primary-color: #B3004A; --secondary-color: #66002B; --gradient-color: #481CAB; --on-hover-border-1: #923CFF; --on-hover-border-2: #FF006A', + style: + '--primary-color: var(--theme-primary-color, #B3004A); --secondary-color: var(--theme-secondary-color, #66002B); --gradient-color: var(--theme-gradient-color, #481CAB); --on-hover-border-1: var(--theme-hover-border-1, #923CFF); --on-hover-border-2: var(--theme-hover-border-2, #FF006A)',libs/blog/roadmap/feature-roadmap/src/lib/ui/roadmap-hover-border-gradient.scss (1)
1-32: Well-implemented hover gradient effect with animationThe gradient border effect implementation with the ::before pseudo-element and rotation animation is clean. The exclusion of .cluster-node elements from the hover effect is handled appropriately.
Consider if the 2000px dimensions for the pseudo-element are necessary - while it works, a smaller size might be more performance-friendly, especially on mobile devices.
- width: 2000px; - height: 2000px; - top: calc(50% - 1000px); - left: calc(50% - 1000px); + width: 1000px; + height: 1000px; + top: calc(50% - 500px); + left: calc(50% - 500px);libs/blog/shell/feature-shell-web/src/lib/roadmap-shell.component.ts (3)
20-30: Simplified template without top banner or router outletThe template only includes header, roadmap feature, and footer components, which is simpler than the RootShellComponent shown in the relevant snippets. However, the component still initializes and processes ad banner data that isn't used in the template.
If ad banners aren't intended to be shown in this view, consider removing the related code to simplify the component.
59-60: Hardcoded adBannerVisible valueThe adBannerVisible computed value is hardcoded to always return false, which means the effect to set viewport offset will always set it to [0, 80].
Consider simplifying this since the value never changes:
- // todo: temporary solution to keep in mind how banner influence the layout - protected readonly adBannerVisible = computed(() => false); + // Banner is not displayed in roadmap view + protected readonly adBannerVisible = false;
1-89: Component structure duplicates RootShellComponentThis component shares significant code with RootShellComponent from the provided context snippets, suggesting potential duplication.
Consider extracting common functionality to a base class or shared service to avoid duplication between shell components. Alternatively, evaluate if this component could extend RootShellComponent and override only what's different.
libs/blog/roadmap/feature-roadmap/src/lib/temp.component.ts (1)
46-49: Multiple calls togeneratePathcould accumulate paths inpathData.
Each call appends a new path topathData. If the intended behavior is to render multiple paths, this is fine. Otherwise, consider clearing or resetting the array before subsequent calls to avoid unintentional duplication.ngOnInit(): void { - this.generatePath(); - this.generatePath(120); - this.generatePath(300); + this.pathData = []; + [0, 120, 300].forEach((len) => this.generatePath(len)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
libs/blog/roadmap/feature-roadmap/.eslintrc.json(1 hunks)libs/blog/roadmap/feature-roadmap/README.md(1 hunks)libs/blog/roadmap/feature-roadmap/jest.config.ts(1 hunks)libs/blog/roadmap/feature-roadmap/project.json(1 hunks)libs/blog/roadmap/feature-roadmap/src/index.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.spec.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/slice.pipes.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/temp.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/ui/roadmap-hover-border-gradient.scss(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-angular-love-node.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-cluster.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-primary-node.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-secondary-node.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/test-setup.ts(1 hunks)libs/blog/roadmap/feature-roadmap/tsconfig.json(1 hunks)libs/blog/roadmap/feature-roadmap/tsconfig.lib.json(1 hunks)libs/blog/roadmap/feature-roadmap/tsconfig.spec.json(1 hunks)libs/blog/shell/feature-shell-web/src/lib/blog-shell.routes.ts(1 hunks)libs/blog/shell/feature-shell-web/src/lib/roadmap-shell.component.ts(1 hunks)libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts(1 hunks)tsconfig.base.json(1 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
libs/blog/shell/feature-shell-web/src/lib/roadmap-shell.component.ts (2)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (1)
Component(39-214)libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts (1)
Component(22-109)
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-primary-node.component.ts (3)
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-angular-love-node.component.ts (1)
Component(5-24)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-secondary-node.component.ts (1)
Component(5-24)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (2)
Component(39-214)RoadmapNode(23-27)
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-angular-love-node.component.ts (3)
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-primary-node.component.ts (1)
Component(5-24)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-secondary-node.component.ts (1)
Component(5-24)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (2)
Component(39-214)RoadmapNode(23-27)
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-secondary-node.component.ts (3)
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-angular-love-node.component.ts (1)
Component(5-24)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-primary-node.component.ts (1)
Component(5-24)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (2)
Component(39-214)RoadmapNode(23-27)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (4)
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-angular-love-node.component.ts (1)
Component(5-24)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-primary-node.component.ts (1)
Component(5-24)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-cluster.component.ts (1)
Component(5-37)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-secondary-node.component.ts (1)
Component(5-24)
🪛 Biome (1.9.4)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
[error] 106-106: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 161-161: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 180-180: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (23)
libs/blog/roadmap/feature-roadmap/src/index.ts (1)
1-1: Clear Public API ExportThe export statement correctly re-exports all public members from the FeatureRoadmapComponent, making it easier to import from a single entry point. This meets the goal of simplifying the public API for the roadmap feature.
libs/blog/roadmap/feature-roadmap/README.md (1)
1-8: Informative README FileThe README provides a succinct description of the library and clear instructions on running the unit tests (i.e., using
nx test feature-roadmap). Including the Nx reference is helpful for developers who might be new to this setup.libs/blog/roadmap/feature-roadmap/src/test-setup.ts (1)
1-6: Robust Test Environment ConfigurationThe use of
setupZoneTestEnvwith strict options (errorOnUnknownElementsanderrorOnUnknownPropertiesset to true) establishes a solid testing foundation. This configuration will help catch unintended template errors and is aligned with best practices for Angular testing.libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts (1)
57-59: Effective Host Styling AdditionThe new
hostproperty that applies the classes'flex flex-col min-h-screen'is a clean solution to enforce the desired layout on the component’s host element. This change is well-targeted and integrates seamlessly with the component’s styling strategy.tsconfig.base.json (1)
191-193: Accurate Module Path MappingThe newly added path alias
"@angular-love/feature-roadmap"correctly points tolibs/blog/roadmap/feature-roadmap/src/index.ts. This change facilitates easier imports and enhances TypeScript module resolution for the roadmap feature.libs/blog/roadmap/feature-roadmap/tsconfig.lib.json (1)
1-17: Configuration looks appropriate for an Angular libraryThe TypeScript configuration extends the base config and sets up standard library options:
- Output directory correctly points to the dist folder
- Declaration files and source maps are enabled for better debugging
- Test files are properly excluded
- Only TypeScript files from src are included
This follows standard practices for Angular libraries built with Nx.
libs/blog/roadmap/feature-roadmap/project.json (1)
1-20: Project configuration follows Nx standardsThe configuration properly defines:
- Project name, schema, and source root paths
- Angular component prefix "al" for consistent component naming
- Library project type
- Appropriate tags for monorepo organization
- Test and lint targets with correct executors
This setup aligns with Nx workspace best practices and will integrate well with the existing architecture.
libs/blog/shell/feature-shell-web/src/lib/blog-shell.routes.ts (1)
24-30: Route implementation follows Angular best practicesThe new roadmap route:
- Is correctly added to the commonRoutes array
- Uses lazy loading with async/await pattern
- Matches the module structure pattern used by other routes
This implementation maintains consistency with the existing routing architecture.
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-angular-love-node.component.ts (3)
1-4: Imports look correctAngular core imports and the RoadmapNode interface are properly imported.
5-21: Component definition follows Angular standardsThe template implementation:
- Uses Angular's standalone component pattern
- Follows the project's naming conventions with the 'al-' prefix
- Includes appropriate styling with CSS variables for theming
- Uses consistent styling approach with the related node components
The gradient styling and CSS variables approach allows for flexible theming.
22-24: Input property correctly implementedThe component:
- Uses Angular's modern input API
- Properly marks the node input as required
- Correctly types the input with the RoadmapNode interface
This ensures type safety and clear component API design.
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-primary-node.component.ts (1)
22-24: Component class implementation looks good.The implementation of the component class with a required input is clean and follows Angular's best practices for modern input syntax.
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html (1)
1-88: Overall structure is complex but well-organized.The SVG-based roadmap visualization is complex but follows a logical structure. Consider adding comments to explain the purpose of different sections for improved maintainability.
libs/blog/roadmap/feature-roadmap/.eslintrc.json (1)
1-36: ESLint configuration looks goodThe configuration properly extends the root ESLint config and sets up appropriate rules for Angular components and directives. The prefix "al" is used consistently across the feature components, which is a good practice for maintaining a clear namespace.
libs/blog/roadmap/feature-roadmap/jest.config.ts (1)
1-21: Jest configuration is properly set upThe configuration correctly sets up Jest for Angular testing with appropriate transformations, serializers, and coverage reporting. The setup aligns with best practices for Angular libraries.
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-secondary-node.component.ts (2)
1-4: Imports look goodThe component correctly imports the necessary dependencies and the RoadmapNode type from the feature component.
22-24: Input usage looks goodThe component correctly uses Angular's input() API with required type checking, which is a modern approach.
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-cluster.component.ts (2)
1-4: Imports look goodThe component correctly imports the necessary dependencies and the RoadmapCluster type.
33-37: Input usage looks goodThe component correctly uses Angular's input() API with required type checking for the cluster property.
libs/blog/roadmap/feature-roadmap/tsconfig.spec.json (1)
1-16: Configuration looks good for Jest testing in AngularThe TypeScript configuration properly extends the base config and includes all necessary settings for Jest-based testing, including correct module settings, target ECMAScript version, and type definitions.
libs/blog/roadmap/feature-roadmap/tsconfig.json (1)
1-28: Good TypeScript configuration with strict settingsThe TypeScript configuration appropriately enables strict type checking and follows Angular best practices with the compiler options. The references to both lib and spec configurations are correctly set up.
libs/blog/roadmap/feature-roadmap/src/lib/temp.component.ts (2)
52-88: Looks good!
The SVG path generation logic is solid, and dynamically adjusting the SVG's dimensions to fit the path is a nice touch.
23-33:❓ Verification inconclusive
Potential structural directive concern with
@for.
This@for (path of pathData; track $index)syntax is non-standard in Angular unless you're intentionally using experimental features or a custom structural directive. Ensure it's functioning as expected in your application.Run this command to confirm whether a custom structural directive for
@foris defined:
🏁 Script executed:
#!/bin/bash fd -e ts --exec rg "@Directive"Length of output: 226
Attention: Verify Custom
@forDirective ImplementationThe template syntax
@for (path of pathData; track $index)remains non-standard in Angular. Although our initial scan returned multiple occurrences of@Directive({in the codebase, we found no clear evidence of a custom structural directive specifically implemented for@for.
- Please confirm that a custom directive supporting the
@forsyntax is intentionally defined (e.g., with a selector like[for]) and documented.- If this syntax was not intended, consider reverting to the standard Angular
*ngForsyntax to prevent potential runtime issues.
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html
Outdated
Show resolved
Hide resolved
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html
Outdated
Show resolved
Hide resolved
| @Component({ | ||
| selector: 'al-root-shell', | ||
| template: ` | ||
| <al-header | ||
| class="block w-full" | ||
| [language]="language()" | ||
| (languageChange)="onLanguageChange($event)" | ||
| > | ||
| <al-search /> | ||
| </al-header> | ||
| <al-feature-roadmap class="flex-1 basis-0" /> | ||
| <al-footer /> | ||
| `, | ||
| imports: [ | ||
| HeaderComponent, | ||
| FooterComponent, | ||
| SearchComponent, | ||
| FeatureRoadmapComponent, | ||
| ], | ||
| host: { | ||
| class: 'flex flex-col min-h-screen', | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Selector name doesn't match component name
The selector al-root-shell doesn't match the component class name RoadmapShellComponent. This could lead to confusion since typically selectors should reflect the component name.
Change the selector to match the component name:
@Component({
- selector: 'al-root-shell',
+ selector: 'al-roadmap-shell',
template: `📝 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.
| @Component({ | |
| selector: 'al-root-shell', | |
| template: ` | |
| <al-header | |
| class="block w-full" | |
| [language]="language()" | |
| (languageChange)="onLanguageChange($event)" | |
| > | |
| <al-search /> | |
| </al-header> | |
| <al-feature-roadmap class="flex-1 basis-0" /> | |
| <al-footer /> | |
| `, | |
| imports: [ | |
| HeaderComponent, | |
| FooterComponent, | |
| SearchComponent, | |
| FeatureRoadmapComponent, | |
| ], | |
| host: { | |
| class: 'flex flex-col min-h-screen', | |
| }, | |
| }) | |
| @Component({ | |
| selector: 'al-roadmap-shell', | |
| template: ` | |
| <al-header | |
| class="block w-full" | |
| [language]="language()" | |
| (languageChange)="onLanguageChange($event)" | |
| > | |
| <al-search /> | |
| </al-header> | |
| <al-feature-roadmap class="flex-1 basis-0" /> | |
| <al-footer /> | |
| `, | |
| imports: [ | |
| HeaderComponent, | |
| FooterComponent, | |
| SearchComponent, | |
| FeatureRoadmapComponent, | |
| ], | |
| host: { | |
| class: 'flex flex-col min-h-screen', | |
| }, | |
| }) |
| constructor(viewport: ViewportScroller) { | ||
| // todo: temporary solution to keep in mind how banner influence the layout | ||
| effect(() => { | ||
| this.adBannerVisible() | ||
| ? viewport.setOffset([0, 160]) | ||
| : viewport.setOffset([0, 80]); | ||
| }); | ||
| this.sliderStore.getData(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unnecessary data fetching for unused banners
The component calls this.sliderStore.getData() but doesn't display the data in the template.
If the ad banner data isn't needed in this view, remove the data fetch:
constructor(viewport: ViewportScroller) {
// todo: temporary solution to keep in mind how banner influence the layout
effect(() => {
this.adBannerVisible()
? viewport.setOffset([0, 160])
: viewport.setOffset([0, 80]);
});
- this.sliderStore.getData();
}📝 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.
| constructor(viewport: ViewportScroller) { | |
| // todo: temporary solution to keep in mind how banner influence the layout | |
| effect(() => { | |
| this.adBannerVisible() | |
| ? viewport.setOffset([0, 160]) | |
| : viewport.setOffset([0, 80]); | |
| }); | |
| this.sliderStore.getData(); | |
| } | |
| constructor(viewport: ViewportScroller) { | |
| // todo: temporary solution to keep in mind how banner influence the layout | |
| effect(() => { | |
| this.adBannerVisible() | |
| ? viewport.setOffset([0, 160]) | |
| : viewport.setOffset([0, 80]); | |
| }); | |
| } |
| protected readonly sliderStore = inject(AdBannerStore); | ||
| protected readonly slides = computed<AdImageBanner[] | undefined>(() => | ||
| this.sliderStore.slider()?.slides.map((slide) => ({ | ||
| url: slide.url, | ||
| alt: slide.alt, | ||
| action: { | ||
| type: 'url', | ||
| url: slide.navigateTo, | ||
| }, | ||
| })), | ||
| ); | ||
| protected readonly msPerSlide = computed( | ||
| () => this.sliderStore.slider()?.slideDisplayTimeMs, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused ad banner functionality
The component processes ad banner data (slides and msPerSlide) but doesn't use it in the template.
Since the template doesn't display an ad banner carousel, remove the unused code:
- protected readonly slides = computed<AdImageBanner[] | undefined>(() =>
- this.sliderStore.slider()?.slides.map((slide) => ({
- url: slide.url,
- alt: slide.alt,
- action: {
- type: 'url',
- url: slide.navigateTo,
- },
- })),
- );
- protected readonly msPerSlide = computed(
- () => this.sliderStore.slider()?.slideDisplayTimeMs,
- );📝 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.
| protected readonly sliderStore = inject(AdBannerStore); | |
| protected readonly slides = computed<AdImageBanner[] | undefined>(() => | |
| this.sliderStore.slider()?.slides.map((slide) => ({ | |
| url: slide.url, | |
| alt: slide.alt, | |
| action: { | |
| type: 'url', | |
| url: slide.navigateTo, | |
| }, | |
| })), | |
| ); | |
| protected readonly msPerSlide = computed( | |
| () => this.sliderStore.slider()?.slideDisplayTimeMs, | |
| ); | |
| protected readonly sliderStore = inject(AdBannerStore); |
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
Outdated
Show resolved
Hide resolved
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
Outdated
Show resolved
Hide resolved
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
Outdated
Show resolved
Hide resolved
|
PR is detected, will deploy to dev environment |
|
Deployed to dev environment |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html (1)
29-38:⚠️ Potential issueSVG marker ID could cause conflicts with multiple component instances.
The hardcoded
id="arrowhead"could cause conflicts if multiple instances of this component exist on the same page.Generate unique IDs for SVG markers by incorporating the layer's parent node ID:
- id="arrowhead" + [attr.id]="'arrowhead-' + layer.parentNode.id"And update the reference:
- marker-end="url(#arrowhead)" + [attr.marker-end]="'url(#arrowhead-' + layer.parentNode.id + ')'"libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (4)
52-52:⚠️ Potential issueIncorrect Angular configuration with
styleUrl.Angular expects
styleUrls(plural) as an array instead of the singularstyleUrlproperty.- styleUrl: './feature-roadmap.component.scss', + styleUrls: ['./feature-roadmap.component.scss'],
103-103: 🛠️ Refactor suggestionAvoid spread syntax in reducers for better performance.
Using spread syntax in reducers causes O(n²) time complexity which can impact performance with larger datasets.
- (acc, node) => ({ ...acc, [node.id]: node }), + (acc, node) => { + acc[node.id] = node; + return acc; + },🧰 Tools
🪛 Biome (1.9.4)
[error] 103-103: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
157-161: 🛠️ Refactor suggestionOptimize the cluster creation reducer.
Using spread syntax in reducers causes O(n²) time complexity which can impact performance with larger datasets.
- (acc, primaryNodeId) => ({ - ...acc, - [nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode']: primaryNodeId, - }), + (acc, primaryNodeId) => { + acc[nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode'] = primaryNodeId; + return acc; + },🧰 Tools
🪛 Biome (1.9.4)
[error] 158-158: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
176-180: 🛠️ Refactor suggestionMinimize object creation overhead in layer mapping.
Using spread syntax in reducers causes O(n²) time complexity which can impact performance with larger datasets.
- (acc, primaryNodeId) => ({ - ...acc, - [nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode']: primaryNodeId, - }), + (acc, primaryNodeId) => { + acc[nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode'] = primaryNodeId; + return acc; + },🧰 Tools
🪛 Biome (1.9.4)
[error] 177-177: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🧹 Nitpick comments (3)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (3)
56-99: Consider externalizing roadmap data.Hard-coding the roadmap data directly in the component makes it difficult to maintain, update, or translate.
Consider moving this data to a separate service or configuration file:
// roadmap-data.service.ts @Injectable({ providedIn: 'root' }) export class RoadmapDataService { getRoadmapNodes(): RoadmapNodeDTO[] { return [ { id: '2', title: 'Components', }, // ... other nodes ]; } } // Then in your component: constructor(private roadmapDataService: RoadmapDataService) { this.nodesDto.set(this.roadmapDataService.getRoadmapNodes()); }
115-118: Avoid array spread for better performance.The spread operator creates a new array on each iteration, which is inefficient for large datasets.
- clusterMap[parentClusterNodeDto.id] = [ - ...(clusterMap[parentClusterNodeDto.id] ?? []), - nodeDto.id, - ]; + if (!clusterMap[parentClusterNodeDto.id]) { + clusterMap[parentClusterNodeDto.id] = []; + } + clusterMap[parentClusterNodeDto.id].push(nodeDto.id);
130-133: Avoid array spread for better performance.The spread operator creates a new array on each iteration, which is inefficient for large datasets.
- layerMap[nodeDto.parentNodeId] = [ - ...(layerMap[nodeDto.parentNodeId] ?? []), - nodeDto.id, - ]; + if (!layerMap[nodeDto.parentNodeId]) { + layerMap[nodeDto.parentNodeId] = []; + } + layerMap[nodeDto.parentNodeId].push(nodeDto.id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.scss(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/secondary-arrow.pipe.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.scss
🧰 Additional context used
🧬 Code Definitions (1)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (4)
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-angular-love-node.component.ts (1)
Component(5-24)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-secondary-node.component.ts (1)
Component(5-24)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-primary-node.component.ts (1)
Component(5-24)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-cluster.component.ts (1)
Component(5-37)
🪛 Biome (1.9.4)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
[error] 103-103: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 158-158: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 177-177: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (1)
libs/blog/roadmap/feature-roadmap/src/lib/secondary-arrow.pipe.ts (1)
19-26: LGTM! Implementation is clear and well-documented.The pipe effectively handles different cases for arrow generation with clear documentation. The arc calculation is properly implemented to create accurate curved corners.
| xmlns:svg="http://www.w3.org/1999/html" | ||
| > | ||
| <svg:foreignObject | ||
| style="transform: translateX(calc(50% - {{ layoutEl.clientWidth / 2 }}px))" |
There was a problem hiding this comment.
Template reference variable used before definition could cause rendering issues.
The translateX calculation references layoutEl.clientWidth before the #layoutEl element is fully rendered, which may cause incorrect initial positioning or flickering.
Consider using Angular's AfterViewInit lifecycle hook with ViewChild to calculate the translation after the view is fully rendered:
// In the component class
@ViewChild('layoutEl') layoutEl: ElementRef;
private translationOffset = signal(0);
ngAfterViewInit() {
// Calculate after view is initialized
this.translationOffset.set(this.layoutEl.nativeElement.clientWidth / 2);
}Then update your template:
- style="transform: translateX(calc(50% - {{ layoutEl.clientWidth / 2 }}px))"
+ style="transform: translateX(calc(50% - {{ translationOffset() }}px))"| @let shift = | ||
| (allChildNodesEl.clientWidth / 2 - leftChildNodesEl.clientWidth || | ||
| 0) - 32; |
There was a problem hiding this comment.
Layout calculation uses template references before initialization.
The calculation for shift uses allChildNodesEl and leftChildNodesEl template references potentially before they're fully initialized, which could lead to incorrect positioning.
Consider using a reactive approach with ViewChildren and AfterViewInit:
@ViewChildren('allChildNodesEl') allChildNodesElements: QueryList<ElementRef>;
@ViewChildren('leftChildNodesEl') leftChildNodesElements: QueryList<ElementRef>;
private nodeShifts = signal<{[key: string]: number}>({});
ngAfterViewInit() {
// Calculate shifts after view is initialized
this.calculateChildNodeShifts();
}
private calculateChildNodeShifts() {
// Calculate shifts for each layer's child nodes
// Store in nodeShifts signal for reactive updates
}Then in your template:
- @let shift =
- (allChildNodesEl.clientWidth / 2 - leftChildNodesEl.clientWidth || 0) - 32;
+ @let shift = nodeShifts()[layer.parentNode.id] || 0;| if (nodeDto.parentNodeId) { | ||
| if (nodeDtoMap[nodeDto.parentNodeId].parentNodeId) { | ||
| const parentClusterNodeDto = nodeDtoMap[nodeDto.parentNodeId]; | ||
|
|
||
| clusterMap[parentClusterNodeDto.id] = [ | ||
| ...(clusterMap[parentClusterNodeDto.id] ?? []), | ||
| nodeDto.id, | ||
| ]; | ||
|
|
||
| if (nodeMap[nodeDto.parentNodeId]) { | ||
| nodeMap[parentClusterNodeDto.id].nodeType = 'cluster'; | ||
| } else { | ||
| nodeMap[parentClusterNodeDto.id] = { | ||
| id: parentClusterNodeDto.id, | ||
| nodeType: 'cluster', | ||
| title: parentClusterNodeDto.title, | ||
| }; | ||
| } | ||
| } else { | ||
| layerMap[nodeDto.parentNodeId] = [ | ||
| ...(layerMap[nodeDto.parentNodeId] ?? []), | ||
| nodeDto.id, | ||
| ]; | ||
| } | ||
| if (!nodeMap[nodeDto.id]) { | ||
| nodeMap[nodeDto.id] = { | ||
| id: nodeDto.id, | ||
| nodeType: 'secondary', | ||
| title: nodeDto.title, | ||
| }; | ||
| } | ||
| } else { | ||
| nodeMap[nodeDto.id] = { | ||
| id: nodeDto.id, | ||
| nodeType: 'primary', | ||
| title: nodeDto.title, | ||
| }; | ||
| if (!layerMap[nodeDto.id]) { | ||
| layerMap[nodeDto.id] = []; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // setup clusters | ||
| Object.entries(clusterMap).forEach(([clusterNodeId, childrenNodeIds]) => { | ||
| const previousClusterNodeIdToNodeIdMap = childrenNodeIds.reduce( | ||
| (acc, primaryNodeId) => ({ | ||
| ...acc, | ||
| [nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode']: | ||
| primaryNodeId, | ||
| }), | ||
| {} as { [previousNodeId: string | 'initialNode']: string }, | ||
| ); | ||
|
|
||
| const clusterNode = nodeMap[clusterNodeId] as RoadmapCluster; | ||
| clusterNode.clusteredNodes = []; | ||
| let nextNodeId = previousClusterNodeIdToNodeIdMap['initialNode']; | ||
| while (nextNodeId) { | ||
| clusterNode.clusteredNodes.push(nodeMap[nextNodeId]); | ||
| nextNodeId = previousClusterNodeIdToNodeIdMap[nextNodeId]; | ||
| } | ||
| }); | ||
|
|
||
| // setup layers | ||
| const previousLayerNodeIdToNodeIdMap = Object.keys(layerMap).reduce( | ||
| (acc, primaryNodeId) => ({ | ||
| ...acc, | ||
| [nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode']: | ||
| primaryNodeId, | ||
| }), | ||
| {} as { [previousNodeId: string | 'initialNode']: string }, | ||
| ); | ||
|
|
||
| const layers: { | ||
| parentNode: RoadmapNode; | ||
| childNodes: RoadmapNode[]; | ||
| }[] = []; | ||
| let nextParentNodeId = previousLayerNodeIdToNodeIdMap['initialNode']; | ||
| while (nextParentNodeId) { | ||
| layers.push({ | ||
| parentNode: nodeMap[nextParentNodeId], | ||
| childNodes: layerMap[nextParentNodeId].map( | ||
| (childrenNodeId) => nodeMap[childrenNodeId], | ||
| ), | ||
| }); | ||
| nextParentNodeId = previousLayerNodeIdToNodeIdMap[nextParentNodeId]; | ||
| } | ||
|
|
||
| return [ | ||
| { | ||
| parentNode: { | ||
| id: '1', | ||
| title: 'Angular.Love Roadmap Introduction', | ||
| nodeType: 'angular-love', | ||
| }, | ||
| childNodes: [], | ||
| }, | ||
| ...layers, | ||
| ]; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor complex roadmap calculation into smaller functions.
The roadmap calculation logic is very complex and contained in a single computed function, making it difficult to understand and maintain.
Break down the logic into smaller, focused functions:
protected readonly roadmapLayers = computed<RoadmapLayer[]>(() => {
const nodeDtoMap = this.createNodeDtoMap();
const { layerMap, clusterMap, nodeMap } = this.createMaps(nodeDtoMap);
this.setupClusters(clusterMap, nodeMap, nodeDtoMap);
const layers = this.setupLayers(layerMap, nodeMap, nodeDtoMap);
return [this.createRootLayer(), ...layers];
});
private createNodeDtoMap(): { [nodeId: string]: RoadmapNodeDTO } {
// Extract node map creation logic
}
private createMaps(nodeDtoMap: { [nodeId: string]: RoadmapNodeDTO }): {
layerMap: { [parentNodeId: string]: string[] },
clusterMap: { [clusterNodeId: string]: string[] },
nodeMap: { [nodeId: string]: RoadmapNode }
} {
// Extract maps creation logic
}
// Additional methods for other parts of the logic🧰 Tools
🪛 Biome (1.9.4)
[error] 103-103: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 158-158: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 177-177: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (4)
78-78: Use the correct Angular property for component styles.In Angular, the correct property is
styleUrls(plural), notstyleUrl. UsingstyleUrlcan lead to build or runtime issues.- styleUrl: './feature-roadmap.component.scss', + styleUrls: ['./feature-roadmap.component.scss'],
169-169: Avoid spread syntax in a.reduce()accumulator for better performance.Constructing a new object on each iteration using spread (
...acc) is potentially O(n^2). Use a mutable approach (direct assignment) to improve performance.- (acc, node) => ({ ...acc, [node.id]: node }), + (acc, node) => { + acc[node.id] = node; + return acc; + },🧰 Tools
🪛 Biome (1.9.4)
[error] 169-169: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
224-224: Avoid spread syntax in a.reduce()accumulator for better performance.Same rationale: every iteration triggers a new object creation, leading to suboptimal time complexity.
- (acc, primaryNodeId) => ({ - ...acc, - [nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode']: primaryNodeId, - }), + (acc, primaryNodeId) => { + acc[nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode'] = primaryNodeId; + return acc; + },🧰 Tools
🪛 Biome (1.9.4)
[error] 224-224: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
243-243: Avoid spread syntax in a.reduce()accumulator for better performance.Repeated usage of spread on an accumulator can degrade performance at scale. Switching to direct assignment is more efficient.
- (acc, primaryNodeId) => ({ - ...acc, - [nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode']: primaryNodeId, - }), + (acc, primaryNodeId) => { + acc[nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode'] = primaryNodeId; + return acc; + },🧰 Tools
🪛 Biome (1.9.4)
[error] 243-243: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🧹 Nitpick comments (2)
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-svg-control.component.ts (2)
14-14: Remove thethis.notation in the template for readability.In Angular templates, referencing signal calls with
this.is unconventional. You can safely removethis.for brevity since it’s implicit in the template context.- <fast-svg [name]="this.iconName()" [size]="this.size()" /> + <fast-svg [name]="iconName()" [size]="size()" />
19-21: Rename the "event" property to avoid confusion.Using
eventas a property name can be confusing since it’s commonly associated with the DOMeventobject. Consider naming it something more descriptive, likeresizeAction, to clarify its purpose.- readonly event = input.required<EventType>(); + readonly resizeAction = input.required<EventType>(); - (click)="resizeRoadmap.emit(this.event())" + (click)="resizeRoadmap.emit(this.resizeAction())"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
apps/blog/src/assets/icons/circle-center.svgis excluded by!**/*.svgapps/blog/src/assets/icons/zoom-in.svgis excluded by!**/*.svgapps/blog/src/assets/icons/zoom-out.svgis excluded by!**/*.svgapps/blog/src/assets/icons/zoom-reset.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-svg-control.component.ts(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html
🧰 Additional context used
🧬 Code Graph Analysis (2)
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-svg-control.component.ts (1)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (2)
Component(64-290)EventType(25-25)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (4)
libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-cluster.component.ts (1)
Component(5-37)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-svg-control.component.ts (1)
Component(6-24)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-primary-node.component.ts (1)
Component(5-24)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-secondary-node.component.ts (1)
Component(5-24)
🪛 Biome (1.9.4)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
[error] 169-169: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 224-224: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 243-243: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (1)
package.json (1)
66-66:✅ Verification successful
Confirm and verify the security status of the newly added package.
The addition of
"svg-pan-zoom": "^3.6.2"appears beneficial for the SVG pan-and-zoom functionality. However, let's make sure there are no known security advisories or deprecated versions.Please run the following script to check known vulnerabilities or advisories for this NPM package:
🏁 Script executed:
#!/bin/bash # Description: Query GitHub for security vulnerabilities related to the "svg-pan-zoom" package in the npm ecosystem gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "svg-pan-zoom") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' 'Length of output: 462
Action: Verify Vulnerability Query for "svg-pan-zoom" Package
The initial security check for
"svg-pan-zoom": "^3.6.2"resulted in an empty vulnerabilities list but also produced a quoting error (unexpected EOF while looking for matching '\''), which prevents us from confidently confirming the package’s security status. Please re-run the query with corrected quoting or verify manually to ensure no vulnerabilities or deprecations exist. For instance, you can try this revised script:
- File: package.json, Line 66:
"svg-pan-zoom": "^3.6.2"- Next Steps:
• Re-run the above script (or an equivalent manual check) to verify that no vulnerabilities are reported for this package.
• Confirm that the query now returns a valid response (ideally with an empty nodes array) without any syntax errors.Once you’ve confirmed the security status manually, please update the review accordingly.
🏁 Script executed:
#!/bin/bash # Revised script to query GitHub for known security vulnerabilities for the "svg-pan-zoom" package using a corrected quoting approach. gh api graphql -f query="$(cat <<'EOF' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "svg-pan-zoom") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } } EOF )"Length of output: 473
Action: Confirm Package "svg-pan-zoom" Security Status
The GitHub GraphQL API query executed with the revised script returned an empty vulnerabilities list, indicating that no known security issues were found for
"svg-pan-zoom": "^3.6.2". Please ensure that this verification covers your security standards, but based on the query, the package appears secure for use.
- File: package.json, Line 66:
"svg-pan-zoom": "^3.6.2"
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html (3)
10-12: Template reference variable used before definition could cause rendering issues.The
translateXcalculation referenceslayoutEl.clientWidthbefore the#layoutElelement is fully rendered, which may cause incorrect initial positioning or flickering.Consider using Angular's AfterViewInit lifecycle hook with ViewChild to calculate the translation after the view is fully rendered:
// In the component class @ViewChild('layoutEl') layoutEl: ElementRef; private translationOffset = signal(0); ngAfterViewInit() { // Calculate after view is initialized this.translationOffset.set(this.layoutEl.nativeElement.clientWidth / 2); }Then update your template:
- style="transform: translateX(calc(50% - {{ - layoutEl.clientWidth / 2 - }}px))" + style="transform: translateX(calc(50% - {{ translationOffset() }}px))"
30-39:⚠️ Potential issueSVG marker ID could cause conflicts with multiple component instances.
The current implementation uses a hardcoded ID for the SVG arrowhead marker, which could cause conflicts if multiple instances of the component are rendered on the same page.
<marker - id="arrowhead" + [attr.id]="'arrowhead-' + layer.parentNode.id" markerWidth="6" markerHeight="8" refX="0" refY="4" orient="auto" > <polygon points="0 0, 6 4, 0 8" fill="#FDF5FD" /> </marker>And update the reference:
<line [attr.y2]="layerHeightWithGap" x1="50" y1="0" x2="50" stroke="white" stroke-width="4" - marker-end="url(#arrowhead)" + [attr.marker-end]="'url(#arrowhead-' + layer.parentNode.id + ')'" />
64-66:⚠️ Potential issueLayout calculation uses template references before initialization.
The calculation for
shiftusesallChildNodesElandleftChildNodesEltemplate references potentially before they're fully initialized, which could lead to incorrect positioning.Consider using a reactive approach with ViewChildren and AfterViewInit:
@ViewChildren('allChildNodesEl') allChildNodesElements: QueryList<ElementRef>; @ViewChildren('leftChildNodesEl') leftChildNodesElements: QueryList<ElementRef>; private nodeShifts = signal<{[key: string]: number}>({}); ngAfterViewInit() { // Calculate shifts after view is initialized this.calculateChildNodeShifts(); } private calculateChildNodeShifts() { // Calculate shifts for each layer's child nodes // Store in nodeShifts signal for reactive updates }Then in your template:
- @let shift = - (allChildNodesEl.clientWidth / 2 - leftChildNodesEl.clientWidth || 0) - 32; + @let shift = nodeShifts()[layer.parentNode.id] || 0;libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (5)
129-242: Refactor complex roadmap calculation into smaller functions.The roadmap calculation logic is very complex and contained in a single computed function, making it difficult to understand and maintain.
Break down the logic into smaller, focused functions:
protected readonly roadmapLayers = computed<RoadmapLayer[]>(() => { const nodesDto = this.nodesDto(); if (!nodesDto) return []; const nodeDtoMap = this.createNodeDtoMap(nodesDto); const { layerMap, clusterMap, nodeMap } = this.createMaps(nodesDto, nodeDtoMap); this.setupClusters(clusterMap, nodeMap, nodeDtoMap); const layers = this.setupLayers(layerMap, nodeMap, nodeDtoMap); return [this.createRootLayer(), ...layers]; }); private createNodeDtoMap(nodesDto: RoadmapNodeDTO[]): { [nodeId: string]: RoadmapNodeDTO } { // Extract node map creation logic return nodesDto.reduce((acc, node) => { acc[node.id] = node; return acc; }, {} as { [nodeId: string]: RoadmapNodeDTO }); } private createMaps(nodesDto: RoadmapNodeDTO[], nodeDtoMap: { [nodeId: string]: RoadmapNodeDTO }): { layerMap: { [parentNodeId: string]: string[] }, clusterMap: { [clusterNodeId: string]: string[] }, nodeMap: { [nodeId: string]: RoadmapNode } } { // Extract maps creation logic // ... } // Additional methods for other parts of the logic🧰 Tools
🪛 Biome (1.9.4)
[error] 133-133: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 188-188: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 207-207: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
77-77:⚠️ Potential issueLikely Angular configuration issue with
styleUrl.Angular expects a
styleUrlsarray instead ofstyleUrl. This could cause runtime or compilation issues.- styleUrl: './feature-roadmap.component.scss', + styleUrls: ['./feature-roadmap.component.scss'],
132-135: 🛠️ Refactor suggestionPerformance issue with spread operator in reducer.
Using the spread syntax inside
.reduce(...)can degrade performance at scale with O(n²) time complexity.- const nodeDtoMap = nodesDto.reduce( - (acc, node) => ({ ...acc, [node.id]: node }), - {} as { [nodeId: string]: RoadmapNodeDTO }, - ); + const nodeDtoMap = nodesDto.reduce( + (acc, node) => { + acc[node.id] = node; + return acc; + }, + {} as { [nodeId: string]: RoadmapNodeDTO }, + );🧰 Tools
🪛 Biome (1.9.4)
[error] 133-133: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
186-193: 🛠️ Refactor suggestionPerformance issue with spread operator in reducer.
Using the spread syntax inside
.reduce(...)can degrade performance at scale with O(n²) time complexity.- const previousClusterNodeIdToNodeIdMap = childrenNodeIds.reduce( - (acc, primaryNodeId) => ({ - ...acc, - [nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode']: - primaryNodeId, - }), - {} as { [previousNodeId: string | 'initialNode']: string }, - ); + const previousClusterNodeIdToNodeIdMap = childrenNodeIds.reduce( + (acc, primaryNodeId) => { + acc[nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode'] = primaryNodeId; + return acc; + }, + {} as { [previousNodeId: string | 'initialNode']: string }, + );🧰 Tools
🪛 Biome (1.9.4)
[error] 188-188: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
205-212: 🛠️ Refactor suggestionPerformance issue with spread operator in reducer.
Using the spread syntax inside
.reduce(...)can degrade performance at scale with O(n²) time complexity.- const previousLayerNodeIdToNodeIdMap = Object.keys(layerMap).reduce( - (acc, primaryNodeId) => ({ - ...acc, - [nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode']: - primaryNodeId, - }), - {} as { [previousNodeId: string | 'initialNode']: string }, - ); + const previousLayerNodeIdToNodeIdMap = Object.keys(layerMap).reduce( + (acc, primaryNodeId) => { + acc[nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode'] = primaryNodeId; + return acc; + }, + {} as { [previousNodeId: string | 'initialNode']: string }, + );🧰 Tools
🪛 Biome (1.9.4)
[error] 207-207: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🧹 Nitpick comments (3)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (3)
145-148: Performance issue with array spread operator.Using the spread operator to append elements to an array can be inefficient, especially in a loop or with large datasets.
- clusterMap[parentClusterNodeDto.id] = [ - ...(clusterMap[parentClusterNodeDto.id] ?? []), - nodeDto.id, - ]; + if (!clusterMap[parentClusterNodeDto.id]) { + clusterMap[parentClusterNodeDto.id] = []; + } + clusterMap[parentClusterNodeDto.id].push(nodeDto.id);
160-163: Performance issue with array spread operator.Using the spread operator to append elements to an array can be inefficient, especially in a loop or with large datasets.
- layerMap[nodeDto.parentNodeId] = [ - ...(layerMap[nodeDto.parentNodeId] ?? []), - nodeDto.id, - ]; + if (!layerMap[nodeDto.parentNodeId]) { + layerMap[nodeDto.parentNodeId] = []; + } + layerMap[nodeDto.parentNodeId].push(nodeDto.id);
244-255: Improve error handling for SVG Pan Zoom initialization.The current SVG Pan Zoom initialization lacks error handling, which could lead to silent failures if the library fails to load or initialize properly.
private async initSvgPanZoom() { + try { const svgPanZoomModule = await import('svg-pan-zoom'); const svgPanZoom: SvgPanZoom.Instance = (svgPanZoomModule as any)['default'] || svgPanZoomModule; const svgRoadmap = this._svgRoadmap(); if (svgRoadmap) { this._svgPanZoom = svgPanZoom(svgRoadmap.nativeElement, { ...svgPanZoomInitialConfig, }); } + } catch (error) { + console.error('Failed to initialize SVG Pan Zoom:', error); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/blog/src/assets/roadmap-tiles.json(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/blog/src/assets/roadmap-tiles.json
🧰 Additional context used
🪛 Biome (1.9.4)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
[error] 133-133: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 188-188: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 207-207: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html
Outdated
Show resolved
Hide resolved
* feat(client): appended svg-pan-zoom lib * feat(client): appended svg pan initialisation and customised buttons
dokolyski
force-pushed
the
feat/angular-roadmap
branch
from
5e64047 to
268958c
Compare
|
PR is detected, will deploy to dev environment |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (5)
77-77:⚠️ Potential issueFix Angular property naming for styles.
Angular expects a
styleUrlsarray instead ofstyleUrl. This could cause runtime or compilation issues.- styleUrl: './feature-roadmap.component.scss', + styleUrls: ['./feature-roadmap.component.scss'],
132-135: 🛠️ Refactor suggestionOptimize node mapping for better performance.
Using spread syntax inside reducers leads to O(n²) time complexity. Direct property assignment is more efficient.
- const nodeDtoMap = nodesDto.reduce( - (acc, node) => ({ ...acc, [node.id]: node }), - {} as { [nodeId: string]: RoadmapNodeDTO }, - ); + const nodeDtoMap = nodesDto.reduce( + (acc, node) => { + acc[node.id] = node; + return acc; + }, + {} as { [nodeId: string]: RoadmapNodeDTO }, + );🧰 Tools
🪛 Biome (1.9.4)
[error] 133-133: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
186-193: 🛠️ Refactor suggestionOptimize cluster mapping reducer.
Using spread syntax inside reducers leads to O(n²) time complexity. Direct property assignment is more efficient.
- const previousClusterNodeIdToNodeIdMap = childrenNodeIds.reduce( - (acc, primaryNodeId) => ({ - ...acc, - [nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode']: - primaryNodeId, - }), - {} as { [previousNodeId: string | 'initialNode']: string }, - ); + const previousClusterNodeIdToNodeIdMap = childrenNodeIds.reduce( + (acc, primaryNodeId) => { + acc[nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode'] = primaryNodeId; + return acc; + }, + {} as { [previousNodeId: string | 'initialNode']: string }, + );🧰 Tools
🪛 Biome (1.9.4)
[error] 188-188: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
205-212: 🛠️ Refactor suggestionOptimize layer mapping reducer.
Using spread syntax inside reducers leads to O(n²) time complexity. Direct property assignment is more efficient.
- const previousLayerNodeIdToNodeIdMap = Object.keys(layerMap).reduce( - (acc, primaryNodeId) => ({ - ...acc, - [nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode']: - primaryNodeId, - }), - {} as { [previousNodeId: string | 'initialNode']: string }, - ); + const previousLayerNodeIdToNodeIdMap = Object.keys(layerMap).reduce( + (acc, primaryNodeId) => { + acc[nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode'] = primaryNodeId; + return acc; + }, + {} as { [previousNodeId: string | 'initialNode']: string }, + );🧰 Tools
🪛 Biome (1.9.4)
[error] 207-207: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
129-242: 🛠️ Refactor suggestionRefactor complex roadmap calculation into smaller functions.
The roadmap calculation logic is very complex and contained in a single computed function, making it difficult to understand and maintain.
Break down the logic into smaller, focused functions:
protected readonly roadmapLayers = computed<RoadmapLayer[]>(() => { const nodesDto = this.nodesDto(); if (!nodesDto) return []; const nodeDtoMap = this.createNodeDtoMap(nodesDto); const { layerMap, clusterMap, nodeMap } = this.createMaps(nodesDto, nodeDtoMap); this.setupClusters(clusterMap, nodeMap, nodeDtoMap); const layers = this.setupLayers(layerMap, nodeMap, nodeDtoMap); return [this.createRootLayer(), ...layers]; }); private createNodeDtoMap(nodesDto: RoadmapNodeDTO[]): { [nodeId: string]: RoadmapNodeDTO } { return nodesDto.reduce((acc, node) => { acc[node.id] = node; return acc; }, {} as { [nodeId: string]: RoadmapNodeDTO }); } private createMaps(nodesDto: RoadmapNodeDTO[], nodeDtoMap: { [nodeId: string]: RoadmapNodeDTO }): { layerMap: { [parentNodeId: string]: string[] }, clusterMap: { [clusterNodeId: string]: string[] }, nodeMap: { [nodeId: string]: RoadmapNode } } { // Extract maps creation logic } // Additional methods for other parts of the logic🧰 Tools
🪛 Biome (1.9.4)
[error] 133-133: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 188-188: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 207-207: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
apps/blog/src/assets/icons/circle-center.svgis excluded by!**/*.svgapps/blog/src/assets/icons/zoom-in.svgis excluded by!**/*.svgapps/blog/src/assets/icons/zoom-out.svgis excluded by!**/*.svgapps/blog/src/assets/icons/zoom-reset.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
apps/blog/src/assets/roadmap-tiles.json(1 hunks)libs/blog/roadmap/feature-roadmap/.eslintrc.json(1 hunks)libs/blog/roadmap/feature-roadmap/README.md(1 hunks)libs/blog/roadmap/feature-roadmap/jest.config.ts(1 hunks)libs/blog/roadmap/feature-roadmap/project.json(1 hunks)libs/blog/roadmap/feature-roadmap/src/index.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.scss(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.spec.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/secondary-arrow.pipe.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/slice.pipes.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/ui/roadmap-hover-border-gradient.scss(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-angular-love-node.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-cluster.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-primary-node.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-secondary-node.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-svg-control.component.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/test-setup.ts(1 hunks)libs/blog/roadmap/feature-roadmap/tsconfig.json(1 hunks)libs/blog/roadmap/feature-roadmap/tsconfig.lib.json(1 hunks)libs/blog/roadmap/feature-roadmap/tsconfig.spec.json(1 hunks)libs/blog/shell/feature-shell-web/src/lib/blog-shell.routes.ts(1 hunks)libs/blog/shell/feature-shell-web/src/lib/roadmap-shell.component.ts(1 hunks)libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts(1 hunks)package.json(1 hunks)tsconfig.base.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (25)
- libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts
- tsconfig.base.json
- libs/blog/roadmap/feature-roadmap/tsconfig.lib.json
- libs/blog/roadmap/feature-roadmap/README.md
- libs/blog/roadmap/feature-roadmap/project.json
- libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.spec.ts
- libs/blog/roadmap/feature-roadmap/src/index.ts
- libs/blog/roadmap/feature-roadmap/src/test-setup.ts
- libs/blog/roadmap/feature-roadmap/src/lib/ui/roadmap-hover-border-gradient.scss
- libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-angular-love-node.component.ts
- libs/blog/shell/feature-shell-web/src/lib/blog-shell.routes.ts
- libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-svg-control.component.ts
- libs/blog/roadmap/feature-roadmap/src/lib/slice.pipes.ts
- libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.scss
- libs/blog/roadmap/feature-roadmap/jest.config.ts
- libs/blog/roadmap/feature-roadmap/.eslintrc.json
- libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-cluster.component.ts
- libs/blog/roadmap/feature-roadmap/tsconfig.json
- libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-secondary-node.component.ts
- libs/blog/shell/feature-shell-web/src/lib/roadmap-shell.component.ts
- libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html
- libs/blog/roadmap/feature-roadmap/src/lib/secondary-arrow.pipe.ts
- apps/blog/src/assets/roadmap-tiles.json
- libs/blog/roadmap/feature-roadmap/tsconfig.spec.json
- libs/blog/roadmap/feature-roadmap/src/lib/ui/ui-roadmap-primary-node.component.ts
🧰 Additional context used
🪛 Biome (1.9.4)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
[error] 133-133: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 188-188: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 207-207: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
| layerMap[nodeDto.parentNodeId] = [ | ||
| ...(layerMap[nodeDto.parentNodeId] ?? []), | ||
| nodeDto.id, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize array handling in layer map.
Similar to the cluster map, use push instead of spread operator for better performance.
- layerMap[nodeDto.parentNodeId] = [
- ...(layerMap[nodeDto.parentNodeId] ?? []),
- nodeDto.id,
- ];
+ if (!layerMap[nodeDto.parentNodeId]) {
+ layerMap[nodeDto.parentNodeId] = [];
+ }
+ layerMap[nodeDto.parentNodeId].push(nodeDto.id);📝 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.
| layerMap[nodeDto.parentNodeId] = [ | |
| ...(layerMap[nodeDto.parentNodeId] ?? []), | |
| nodeDto.id, | |
| ]; | |
| if (!layerMap[nodeDto.parentNodeId]) { | |
| layerMap[nodeDto.parentNodeId] = []; | |
| } | |
| layerMap[nodeDto.parentNodeId].push(nodeDto.id); |
| clusterMap[parentClusterNodeDto.id] = [ | ||
| ...(clusterMap[parentClusterNodeDto.id] ?? []), | ||
| nodeDto.id, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve array handling in cluster map.
Using spread operator for array concatenation creates unnecessary copies. Use push for better performance.
- clusterMap[parentClusterNodeDto.id] = [
- ...(clusterMap[parentClusterNodeDto.id] ?? []),
- nodeDto.id,
- ];
+ if (!clusterMap[parentClusterNodeDto.id]) {
+ clusterMap[parentClusterNodeDto.id] = [];
+ }
+ clusterMap[parentClusterNodeDto.id].push(nodeDto.id);
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
Outdated
Show resolved
Hide resolved
| private readonly nodesDto = rxResource({ | ||
| loader: () => | ||
| this._http.get<RoadmapNodeDTO[]>('assets/roadmap-tiles.json', { | ||
| responseType: 'json', | ||
| }), | ||
| }).value.asReadonly(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for HTTP requests.
The current implementation doesn't handle potential errors in HTTP requests. Add error handling to provide feedback to users.
private readonly nodesDto = rxResource({
loader: () =>
this._http.get<RoadmapNodeDTO[]>('assets/roadmap-tiles.json', {
responseType: 'json',
}),
+ errorHandler: (error) => {
+ console.error('Failed to load roadmap data:', error);
+ return []; // Provide fallback or empty data
+ },
}).value.asReadonly();📝 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.
| private readonly nodesDto = rxResource({ | |
| loader: () => | |
| this._http.get<RoadmapNodeDTO[]>('assets/roadmap-tiles.json', { | |
| responseType: 'json', | |
| }), | |
| }).value.asReadonly(); | |
| private readonly nodesDto = rxResource({ | |
| loader: () => | |
| this._http.get<RoadmapNodeDTO[]>('assets/roadmap-tiles.json', { | |
| responseType: 'json', | |
| }), | |
| errorHandler: (error) => { | |
| console.error('Failed to load roadmap data:', error); | |
| return []; // Provide fallback or empty data | |
| }, | |
| }).value.asReadonly(); |
| private async initSvgPanZoom() { | ||
| const svgPanZoomModule = await import('svg-pan-zoom'); | ||
| const svgPanZoom: SvgPanZoom.Instance = | ||
| (svgPanZoomModule as any)['default'] || svgPanZoomModule; | ||
|
|
||
| const svgRoadmap = this._svgRoadmap(); | ||
| if (svgRoadmap) { | ||
| this._svgPanZoom = svgPanZoom(svgRoadmap.nativeElement, { | ||
| ...svgPanZoomInitialConfig, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for SVG Pan-Zoom initialization.
The dynamic import and initialization of SVG Pan-Zoom lacks error handling, which could lead to unexpected behavior if the library fails to load or initialize.
private async initSvgPanZoom() {
- const svgPanZoomModule = await import('svg-pan-zoom');
- const svgPanZoom: SvgPanZoom.Instance =
- (svgPanZoomModule as any)['default'] || svgPanZoomModule;
-
- const svgRoadmap = this._svgRoadmap();
- if (svgRoadmap) {
- this._svgPanZoom = svgPanZoom(svgRoadmap.nativeElement, {
- ...svgPanZoomInitialConfig,
- });
- }
+ try {
+ const svgPanZoomModule = await import('svg-pan-zoom');
+ const svgPanZoom: SvgPanZoom.Instance =
+ 'default' in svgPanZoomModule ? svgPanZoomModule.default : svgPanZoomModule;
+
+ const svgRoadmap = this._svgRoadmap();
+ if (svgRoadmap) {
+ this._svgPanZoom = svgPanZoom(svgRoadmap.nativeElement, {
+ ...svgPanZoomInitialConfig,
+ });
+ }
+ } catch (error) {
+ console.error('Failed to initialize SVG Pan-Zoom:', error);
+ // Consider providing fallback behavior or notifying the user
+ }
}
|
Deployed to dev environment |
Extract additional UI components, introduce UI libs, update roadmap layering logic - use css-based connectors (instead of svg-based) and simplify styling.
|
PR is detected, will deploy to dev environment |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (8)
91-94: 🛠️ Refactor suggestionOptimize reducer to avoid O(n²) complexity.
Using the spread operator in reducers leads to O(n²) time complexity as it creates a new object on each iteration.
- const nodeDtoMap = roadmapNodesDto.reduce( - (acc, node) => ({ ...acc, [node.id]: node }), - {} as { [nodeId: string]: RoadmapNodeDTO }, - ); + const nodeDtoMap = roadmapNodesDto.reduce( + (acc, node) => { + acc[node.id] = node; + return acc; + }, + {} as { [nodeId: string]: RoadmapNodeDTO }, + );🧰 Tools
🪛 Biome (1.9.4)
[error] 92-92: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
104-107: 🛠️ Refactor suggestionOptimize array handling in cluster map.
Using the spread operator for array concatenation creates unnecessary copies and can lead to performance issues.
- clusterMap[parentClusterNodeDto.id] = [ - ...(clusterMap[parentClusterNodeDto.id] ?? []), - nodeDto.id, - ]; + if (!clusterMap[parentClusterNodeDto.id]) { + clusterMap[parentClusterNodeDto.id] = []; + } + clusterMap[parentClusterNodeDto.id].push(nodeDto.id);
119-122: 🛠️ Refactor suggestionOptimize array handling in layer map.
Similar to the cluster map, use push instead of spread operator for better performance.
- layerMap[nodeDto.parentNodeId] = [ - ...(layerMap[nodeDto.parentNodeId] ?? []), - nodeDto.id, - ]; + if (!layerMap[nodeDto.parentNodeId]) { + layerMap[nodeDto.parentNodeId] = []; + } + layerMap[nodeDto.parentNodeId].push(nodeDto.id);
145-152: 🛠️ Refactor suggestionOptimize the cluster creation reducer.
Using the spread syntax inside
.reduce(...)can degrade performance at scale.- const previousClusterNodeIdToNodeIdMap = childrenNodeIds.reduce( - (acc, primaryNodeId) => ({ - ...acc, - [nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode']: - primaryNodeId, - }), - {} as { [previousNodeId: string | 'initialNode']: string }, - ); + const previousClusterNodeIdToNodeIdMap = childrenNodeIds.reduce( + (acc, primaryNodeId) => { + acc[nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode'] = primaryNodeId; + return acc; + }, + {} as { [previousNodeId: string | 'initialNode']: string }, + );🧰 Tools
🪛 Biome (1.9.4)
[error] 147-147: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
166-173: 🛠️ Refactor suggestionMinimize object creation overhead in layer mapping.
Same as previous reducers, using spread in reducers causes O(n²) complexity.
- const previousLayerNodeIdToNodeIdMap = Object.keys(layerMap).reduce( - (acc, primaryNodeId) => ({ - ...acc, - [nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode']: - primaryNodeId, - }), - {} as { [previousNodeId: string | 'initialNode']: string }, - ); + const previousLayerNodeIdToNodeIdMap = Object.keys(layerMap).reduce( + (acc, primaryNodeId) => { + acc[nodeDtoMap[primaryNodeId].previousNodeId || 'initialNode'] = primaryNodeId; + return acc; + }, + {} as { [previousNodeId: string | 'initialNode']: string }, + );🧰 Tools
🪛 Biome (1.9.4)
[error] 168-168: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
59-64: 🛠️ Refactor suggestionAdd error handling for HTTP requests.
The current implementation doesn't handle potential errors in HTTP requests. This could lead to unexpected behavior if the request fails.
private readonly nodesDto = rxResource({ loader: () => this._http.get<RoadmapNodeDTO[]>('assets/roadmap-tiles.json', { responseType: 'json', }), + errorHandler: (error) => { + console.error('Failed to load roadmap data:', error); + return []; // Provide fallback or empty data + }, }).value.asReadonly();
200-210: 🛠️ Refactor suggestionAdd error handling for SVG Pan-Zoom initialization.
The dynamic import and initialization of SVG Pan-Zoom lacks error handling, which could lead to unexpected behavior if the library fails to load.
private async initSvgPanZoom() { - const svgPanZoomModule = await import('svg-pan-zoom'); - const svgPanZoom: SvgPanZoom.Instance = - 'default' in svgPanZoomModule - ? (svgPanZoomModule.default as SvgPanZoom.Instance) - : svgPanZoomModule; - - this._svgPanZoom = svgPanZoom(this._svgRoadmap().nativeElement, { - ...svgPanZoomInitialConfig, - }); + try { + const svgPanZoomModule = await import('svg-pan-zoom'); + const svgPanZoom: SvgPanZoom.Instance = + 'default' in svgPanZoomModule + ? (svgPanZoomModule.default as SvgPanZoom.Instance) + : svgPanZoomModule; + + this._svgPanZoom = svgPanZoom(this._svgRoadmap().nativeElement, { + ...svgPanZoomInitialConfig, + }); + } catch (error) { + console.error('Failed to initialize SVG Pan-Zoom:', error); + // Consider providing fallback behavior or notifying the user + } }
83-198: 🛠️ Refactor suggestionComplex roadmap calculation logic needs refactoring.
The
buildRoadmapLayersmethod is quite complex and contains multiple responsibilities. As noted in the TODO comment, this should be extracted into smaller, more focused functions.Consider refactoring into separate methods such as:
- // TODO - maybe extract to util function and rewrite this to be more readable - private buildRoadmapLayers( - roadmapNodesDto: RoadmapNodeDTO[] | undefined, - ): RoadmapLayer[] { - // ... complex implementation - } + private buildRoadmapLayers( + roadmapNodesDto: RoadmapNodeDTO[] | undefined, + ): RoadmapLayer[] { + if (!roadmapNodesDto) { + return []; + } + + const nodeDtoMap = this.createNodeDtoMap(roadmapNodesDto); + const { layerMap, clusterMap, nodeMap } = this.createMaps(roadmapNodesDto, nodeDtoMap); + this.setupClusters(clusterMap, nodeMap, nodeDtoMap); + const layers = this.setupLayers(layerMap, nodeMap, nodeDtoMap); + + return [this.createRootLayer(), ...layers]; + } + + private createNodeDtoMap(nodes: RoadmapNodeDTO[]): { [nodeId: string]: RoadmapNodeDTO } { + const map: { [nodeId: string]: RoadmapNodeDTO } = {}; + for (const node of nodes) { + map[node.id] = node; + } + return map; + } + + // Additional helper methods for maps creation, cluster setup, layer setup, etc.🧰 Tools
🪛 Biome (1.9.4)
[error] 92-92: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 147-147: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 168-168: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🧹 Nitpick comments (4)
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-cluster/roadmap-cluster.component.ts (1)
1-37: Well-structured component with OnPush change detection.The component is well-implemented with a clear responsibility for rendering a cluster of roadmap nodes. The use of OnPush change detection is appropriate for performance optimization.
Consider extracting hard-coded color values.
The template contains hard-coded color values like
#FDF5FDand#100F15that would be better maintained as CSS variables.- class="relative z-10 m-[-2px] rounded-lg border-2 border-[#FDF5FD] bg-[--secondary-color] px-6 py-4" + class="relative z-10 m-[-2px] rounded-lg border-2 border-[--border-color] bg-[--secondary-color] px-6 py-4"Consider adding semantic HTML elements and ARIA attributes.
For improved accessibility, consider using semantic HTML elements and adding appropriate ARIA attributes to clarify the component's role in the roadmap visualization.
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts (2)
12-12: Remove extra space in class attribute.There's an extra space between "rounded-lg" and "bg-gradient-to-r" in the class attribute.
- class="relative z-10 m-[4px] rounded-lg bg-gradient-to-r from-[--secondary-color] to-[--gradient-color] px-6 py-4" + class="relative z-10 m-[4px] rounded-lg bg-gradient-to-r from-[--secondary-color] to-[--gradient-color] px-6 py-4"
1-23: Maintain consistency in styling approach across node components.Unlike the
RoadmapPrimaryNodeComponent, this component doesn't use host bindings for styling. Consider adopting a consistent styling approach across all node components.Extract hard-coded color values to CSS variables.
For better maintainability and theming support, extract hard-coded color values like
#FDF5FDto CSS variables.libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-secondary-node/roadmap-secondary-node.component.ts (1)
1-23: Ensure consistent styling across node components.This component uses a margin of 2px, while the primary and angular-love nodes use 4px. Similarly, the text size here is 20px compared to 24px in other components. Consider standardizing these values or documenting the intentional differences.
Extract hard-coded color values to CSS variables.
For better maintainability and theming support, extract hard-coded color values like
#FDF5FDto CSS variables.Consider a shared base class for roadmap nodes.
The roadmap node components share similar templates and functionality. Consider creating a base component class to reduce code duplication across the different node types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (49)
libs/blog/roadmap/feature-roadmap/README.md(1 hunks)libs/blog/roadmap/feature-roadmap/jest.config.ts(1 hunks)libs/blog/roadmap/feature-roadmap/project.json(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.scss(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/.eslintrc.json(1 hunks)libs/blog/roadmap/ui-roadmap-node/README.md(1 hunks)libs/blog/roadmap/ui-roadmap-node/jest.config.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/project.json(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/index.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-cluster/roadmap-cluster.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-cluster/roadmap-cluster.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-secondary-node/roadmap-secondary-node.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-secondary-node/roadmap-secondary-node.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/style/_roadmap-hover-border-gradient.scss(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/types/roadmap-node.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/test-setup.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/tsconfig.json(1 hunks)libs/blog/roadmap/ui-roadmap-node/tsconfig.lib.json(1 hunks)libs/blog/roadmap/ui-roadmap-node/tsconfig.spec.json(1 hunks)libs/blog/roadmap/ui-roadmap/.eslintrc.json(1 hunks)libs/blog/roadmap/ui-roadmap/README.md(1 hunks)libs/blog/roadmap/ui-roadmap/jest.config.ts(1 hunks)libs/blog/roadmap/ui-roadmap/project.json(1 hunks)libs/blog/roadmap/ui-roadmap/src/index.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/connected-node/connected-node.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/connected-node/connected-node.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/node-connector-line/node-connector-line.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/node-connector-line/node-connector-line.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/left-slice.pipe.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/right-slice.pipe.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-svg-controls/roadmap-svg-controls.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/vertical-connector-arrow/vertical-connector-arrow.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/vertical-connector-arrow/vertical-connector-arrow.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/test-setup.ts(1 hunks)libs/blog/roadmap/ui-roadmap/tsconfig.json(1 hunks)libs/blog/roadmap/ui-roadmap/tsconfig.lib.json(1 hunks)libs/blog/roadmap/ui-roadmap/tsconfig.spec.json(1 hunks)libs/blog/shell/feature-shell-web/src/lib/roadmap-shell.component.ts(1 hunks)nx.json(1 hunks)tsconfig.base.json(1 hunks)
✅ Files skipped from review due to trivial changes (31)
- libs/blog/roadmap/ui-roadmap/src/lib/components/vertical-connector-arrow/vertical-connector-arrow.component.ts
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.scss
- libs/blog/roadmap/ui-roadmap/src/index.ts
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-secondary-node/roadmap-secondary-node.component.scss
- libs/blog/roadmap/ui-roadmap/README.md
- libs/blog/roadmap/ui-roadmap-node/README.md
- libs/blog/roadmap/ui-roadmap-node/src/test-setup.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.scss
- libs/blog/roadmap/ui-roadmap/tsconfig.spec.json
- libs/blog/roadmap/ui-roadmap-node/tsconfig.json
- libs/blog/roadmap/ui-roadmap/src/lib/components/connected-node/connected-node.component.html
- libs/blog/roadmap/ui-roadmap/src/test-setup.ts
- libs/blog/roadmap/ui-roadmap/tsconfig.lib.json
- libs/blog/roadmap/ui-roadmap/src/lib/components/vertical-connector-arrow/vertical-connector-arrow.component.scss
- libs/blog/roadmap/ui-roadmap-node/tsconfig.lib.json
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.scss
- libs/blog/roadmap/ui-roadmap-node/.eslintrc.json
- nx.json
- libs/blog/roadmap/ui-roadmap-node/jest.config.ts
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-cluster/roadmap-cluster.component.scss
- libs/blog/roadmap/ui-roadmap-node/tsconfig.spec.json
- libs/blog/roadmap/ui-roadmap/project.json
- libs/blog/roadmap/ui-roadmap/tsconfig.json
- libs/blog/roadmap/ui-roadmap-node/project.json
- libs/blog/roadmap/ui-roadmap/src/lib/components/node-connector-line/node-connector-line.component.scss
- libs/blog/roadmap/ui-roadmap/.eslintrc.json
- libs/blog/roadmap/ui-roadmap-node/src/index.ts
- libs/blog/roadmap/ui-roadmap-node/src/lib/style/_roadmap-hover-border-gradient.scss
- libs/blog/roadmap/ui-roadmap-node/src/lib/types/roadmap-node.ts
- libs/blog/roadmap/ui-roadmap/jest.config.ts
- tsconfig.base.json
🚧 Files skipped from review as they are similar to previous changes (6)
- libs/blog/roadmap/feature-roadmap/jest.config.ts
- libs/blog/roadmap/feature-roadmap/project.json
- libs/blog/roadmap/feature-roadmap/README.md
- libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.scss
- libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html
- libs/blog/shell/feature-shell-web/src/lib/roadmap-shell.component.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/right-slice.pipe.ts (1)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/left-slice.pipe.ts (1)
Pipe(3-11)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/left-slice.pipe.ts (1)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/right-slice.pipe.ts (1)
Pipe(3-11)
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts (5)
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts (1)
Component(5-23)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-secondary-node/roadmap-secondary-node.component.ts (1)
Component(5-23)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.ts (1)
Component(24-48)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (1)
Component(42-211)libs/blog/roadmap/ui-roadmap-node/src/lib/types/roadmap-node.ts (1)
RoadmapNode(15-15)
🪛 Biome (1.9.4)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
[error] 92-92: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 147-147: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 168-168: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (7)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/right-slice.pipe.ts (1)
1-11: Clean and well-implemented pipe!This pipe correctly extracts the right half of an array by calculating the midpoint and returning the elements from that point to the end. The use of generics ensures type safety, and the implementation follows Angular's best practices.
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/left-slice.pipe.ts (1)
1-11: Clean implementation complementing the right-slice pipe!This pipe correctly extracts the left half of an array, using the same midpoint calculation as the
RightSlicePipe. The consistent use ofMath.ceil()ensures that for odd-length arrays, the left half gets one more element than the right half, which maintains consistent behavior when these pipes are used together.libs/blog/roadmap/ui-roadmap/src/lib/components/node-connector-line/node-connector-line.component.ts (1)
1-23: Excellent use of modern Angular patterns!This component uses several modern Angular best practices:
- Empty template with host bindings for CSS class application
- OnPush change detection for better performance
- The new input() API with default values
- Clear type definition with a dedicated type alias
The component serves as a visual connector with different styles based on its position, efficiently implemented without unnecessary DOM elements.
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-node/connected-node.component.ts (1)
1-34: Well-designed component with good use of Angular signals!This component effectively uses:
- OnPush change detection for performance
- Modern input() API with default values
- The computed() signal for deriving the connector type
- Clear, readable conditionals
The component intelligently determines the appropriate connector type based on the node's position in the sequence, creating a clean visual hierarchy for the roadmap.
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.ts (1)
1-48: Well-structured component using modern Angular features.This component is well-organized, using Angular's modern signals-based input API, OnPush change detection, and standalone component architecture. The clear interface definition and component organization follow good practices.
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html (1)
1-51: Good use of modern Angular template syntax and layout structure.The template effectively uses Angular's new control flow syntax (
@if,@for) and properly utilizes tracking for performance optimization. The layout is structured well with clear separation between parent node, layer connector, and child nodes.libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-svg-controls/roadmap-svg-controls.component.ts (1)
1-55: Well-implemented controls component with type safety.The component is focused and well-organized with clear typing for events and controls. Good use of modern Angular features including output signals and the
@forloop syntax.
...ap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts
Outdated
Show resolved
Hide resolved
|
Deployed to dev environment |
…centralized logic
|
PR is detected, will deploy to dev environment |
|
Deployed to dev environment |
|
PR is detected, will deploy to dev environment |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts (1)
8-10: Extract hardcoded color values to CSS variables.For better maintainability and theming support, consider extracting the hardcoded color value
#FDF5FDto a CSS variable in your SCSS file.- class="roadmap-hover-border-gradient relative w-fit text-nowrap rounded-lg bg-[#FDF5FD] text-[#FDF5FD]" + class="roadmap-hover-border-gradient relative w-fit text-nowrap rounded-lg bg-[--background-color] text-[--background-color]"Then define the variable in your SCSS file:
:host { --background-color: #FDF5FD; }
🧹 Nitpick comments (5)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connection-point.directive.ts (2)
20-25: Consider making the parent‐directive injection optional
inject(NodeConnectionsGroupDirective)will throw aNullInjectorErrorif somebody usesalNodeConnectionPointoutside ofalNodeConnectionsGroup. Making the injection optional (or using@Host+ a clear runtime error) will fail fast in development instead of crashing the whole app at runtime.- private readonly _nodesConnectionsGroupDirective = inject( - NodeConnectionsGroupDirective, - ); + private readonly _nodesConnectionsGroupDirective = inject( + NodeConnectionsGroupDirective, + { optional: true } + )!;Then guard the two method calls with a null-check.
34-44: Potential timing issue – unsubscribe / cancelafterNextRender
afterNextRender()cannot be cancelled, so if a node is destroyed before the callback fires (e.g. during fast list updates) the directive will still touch a destroyed element. Consider guarding the callback withdestroyRef.onDestroy()or useafterNextRender(this.destroyRef, …)once you migrate to Angular 18.No immediate bug, but worth tracking.
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html (1)
15-22: SVG width of1may clip the pathThe SVG element has
width="1"while the generated path can extend hundreds of pixels left or right. Althoughoverflow="visible"alleviates most issues, some browsers still apply hit-testing/clipping based on the viewport box. Settingwidth="0"(auto-sizing) or a large enough width avoids corner-cases.libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.ts (1)
28-30: Duplicate magic numbers – promote to a shared constant
connectorHeight = 40andconnectorStrokeWidth = 3are hard-coded here and inNodeConnectionsGroupDirective. Extract them into a sharedconstants.tsfile to keep the values in sync.libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connections-group.directive.ts (1)
77-88: Arc direction calculation can flip for negative offsets
radiusOffset = offsetX > 0 ? -radius : radiusassumes positiveoffsetXcorresponds to “to-right”, but that relation reverses whendirection()is'to-left'. This occasionally produces a kink in the path for mixed positive/negative offsets.
Consider basing the offset ondirectionModifierinstead of the raw sign ofoffsetX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (12)
apps/blog/src/assets/i18n/en.json(1 hunks)apps/blog/src/assets/i18n/pl.json(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connection-point.directive.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connections-group.directive.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-svg-controls/roadmap-svg-controls.component.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/blog/src/assets/i18n/en.json
- apps/blog/src/assets/i18n/pl.json
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.scss
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-svg-controls/roadmap-svg-controls.component.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.ts (4)
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts (1)
Component(5-23)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts (1)
Component(5-23)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.ts (1)
Component(21-41)libs/blog/roadmap/ui-roadmap-node/src/lib/types/roadmap-node.ts (1)
RoadmapNode(15-15)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connection-point.directive.ts (1)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connections-group.directive.ts (1)
Directive(17-92)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connections-group.directive.ts (1)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connection-point.directive.ts (2)
Directive(16-52)NodeConnectionPoint(12-14)
🔇 Additional comments (5)
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts (4)
1-3: LGTM! Good use of modern Angular features.The component uses the new signal-based input API from Angular with proper imports.
11-15: LGTM! Good nested structure.The inner div with z-index and proper spacing looks good, and correctly uses the CSS variable for primary color.
18-20: LGTM! Good use of OnPush change detection.Using OnPush change detection strategy is a best practice for performance optimization.
21-23: LGTM! Well-structured component API.Good use of the new signal-based input API with the required decorator to ensure the component receives the necessary data.
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connections-group.directive.ts (1)
60-63: Early return yields empty path string; consumers should handle it
groupConnectionPath()returns''when no points exist. Ensure the template guards against drawing an empty path; otherwise SVG renders a 0-length path that may still consume pointer events.
| <div | ||
| class="flex gap-[32px]" | ||
| [class]="{ | ||
| 'ms-[50px]': nodesDrawingDirection() === 'to-right', | ||
| 'me-[50px]': nodesDrawingDirection() === 'to-left', | ||
| }" | ||
| > |
There was a problem hiding this comment.
Use ngClass instead of [class] when binding an object
[class]="{ … }" converts the object to the literal string "[object Object]", so the dynamic margins will never be applied.
- class="flex gap-[32px]"
- [class]="{
+ class="flex gap-[32px]"
+ [ngClass]="{
'ms-[50px]': nodesDrawingDirection() === 'to-right',
'me-[50px]': nodesDrawingDirection() === 'to-left',
}"📝 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="flex gap-[32px]" | |
| [class]="{ | |
| 'ms-[50px]': nodesDrawingDirection() === 'to-right', | |
| 'me-[50px]': nodesDrawingDirection() === 'to-left', | |
| }" | |
| > | |
| <div | |
| class="flex gap-[32px]" | |
| [ngClass]="{ | |
| 'ms-[50px]': nodesDrawingDirection() === 'to-right', | |
| 'me-[50px]': nodesDrawingDirection() === 'to-left', | |
| }" | |
| > |
🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html
around lines 25 to 31, the code uses [class] binding with an object, which
results in the object being converted to a string "[object Object]" and prevents
dynamic margin classes from applying. Replace the [class] binding with [ngClass]
to correctly apply the conditional classes based on nodesDrawingDirection().
| @Component({ | ||
| selector: 'al-connected-nodes', | ||
| templateUrl: 'connected-nodes.component.html', | ||
| imports: [ | ||
| NodeConnectionPointDirective, | ||
| NodeConnectionsGroupDirective, | ||
| RoadmapClusterComponent, | ||
| RoadmapSecondaryNodeComponent, | ||
| ], | ||
| changeDetection: ChangeDetectionStrategy.OnPush, | ||
| }) |
There was a problem hiding this comment.
Missing standalone: true results in Angular compilation error
The component supplies an imports array, which is only allowed for standalone components. Without the standalone: true flag the build will fail with “Component metadata expected a module id…” error.
@Component({
+ standalone: true,
selector: 'al-connected-nodes',
templateUrl: 'connected-nodes.component.html',📝 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.
| @Component({ | |
| selector: 'al-connected-nodes', | |
| templateUrl: 'connected-nodes.component.html', | |
| imports: [ | |
| NodeConnectionPointDirective, | |
| NodeConnectionsGroupDirective, | |
| RoadmapClusterComponent, | |
| RoadmapSecondaryNodeComponent, | |
| ], | |
| changeDetection: ChangeDetectionStrategy.OnPush, | |
| }) | |
| @Component({ | |
| standalone: true, | |
| selector: 'al-connected-nodes', | |
| templateUrl: 'connected-nodes.component.html', | |
| imports: [ | |
| NodeConnectionPointDirective, | |
| NodeConnectionsGroupDirective, | |
| RoadmapClusterComponent, | |
| RoadmapSecondaryNodeComponent, | |
| ], | |
| changeDetection: ChangeDetectionStrategy.OnPush, | |
| }) |
🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.ts
between lines 12 and 22, the component defines an imports array but is missing
the standalone: true flag. To fix the Angular compilation error, add standalone:
true to the @Component decorator metadata so Angular treats this as a standalone
component.
| private readonly connectionPoints: Signal<NodeConnectionPoint[]> = computed( | ||
| () => | ||
| this._connectionPointDirectives() | ||
| .map((cp) => cp.nodeConnectionPoint()) | ||
| .filter((cp) => cp !== undefined), | ||
| ); |
There was a problem hiding this comment.
Type narrowing lost – filter does not remove undefined
Array.filter without a user-defined type predicate keeps the union type, so connectionPoints() is inferred as (NodeConnectionPoint | undefined)[]. This breaks strict‐null builds and IntelliSense later on.
- .map((cp) => cp.nodeConnectionPoint())
- .filter((cp) => cp !== undefined),
+ .map((cp) => cp.nodeConnectionPoint())
+ .filter((cp): cp is NodeConnectionPoint => cp !== undefined),📝 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.
| private readonly connectionPoints: Signal<NodeConnectionPoint[]> = computed( | |
| () => | |
| this._connectionPointDirectives() | |
| .map((cp) => cp.nodeConnectionPoint()) | |
| .filter((cp) => cp !== undefined), | |
| ); | |
| private readonly connectionPoints: Signal<NodeConnectionPoint[]> = computed( | |
| () => | |
| this._connectionPointDirectives() | |
| .map((cp) => cp.nodeConnectionPoint()) | |
| .filter((cp): cp is NodeConnectionPoint => cp !== undefined), | |
| ); |
🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connections-group.directive.ts
around lines 30 to 35, the filter used to remove undefined values does not
narrow the type properly, causing connectionPoints() to be inferred as
(NodeConnectionPoint | undefined)[]. Fix this by providing a user-defined type
predicate in the filter callback to explicitly assert that the filtered values
are NodeConnectionPoint, ensuring correct type narrowing and compatibility with
strict-null checks.
|
Deployed to dev environment |
Additionally: * update horizontal connections drawing issues and simplify logic
|
PR is detected, will deploy to dev environment |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (3)
40-41:styleUrlshould bestyleUrls(array) – Angular will ignore the current propertyAngular component metadata expects the plural
styleUrlsarray. Using the singular form will silently drop your stylesheet in AoT builds and break styling.- styleUrl: './feature-roadmap.component.scss', + styleUrls: ['./feature-roadmap.component.scss'],
67-72: Missing error-handling for roadmap HTTP fetchThe
rxResourceloader lacks anerrorHandler, so a network / CORS failure will propagate an unhandled error and keep the signal in “loading” forever. The exact same concern was raised previously and still applies.Consider:
loader: () => this._http.get<RoadmapNodeDTO[]>('assets/roadmap-tiles.json', { responseType: 'json', }), +errorHandler: (err) => { + console.error('[Roadmap] Failed to load tiles:', err); + return []; // or a domain-specific fallback +},
133-144: Wrap dynamicsvg-pan-zoomimport in try/catch to avoid white-screen on load failureA network error or SSR hydration race will currently throw and break the component. The same improvement was suggested before but isn’t present yet.
-private async initSvgPanZoom() { - const svgPanZoomModule = await import('svg-pan-zoom'); +private async initSvgPanZoom() { + try { + const svgPanZoomModule = await import('svg-pan-zoom'); const svgPanZoom: SvgPanZoom.Instance = 'default' in svgPanZoomModule ? (svgPanZoomModule.default as SvgPanZoom.Instance) : svgPanZoomModule; const svgPanZoomInstance = svgPanZoom(this._svgRoadmap().nativeElement, { ...svgPanZoomInitialConfig, }); this.svgPanZoom.set(svgPanZoomInstance); + } catch (err) { + console.error('[Roadmap] Failed to bootstrap svg-pan-zoom:', err); + } }libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.ts (1)
12-23:⚠️ Potential issueMissing
standalone: trueresults in Angular compilation errorThe component supplies an
importsarray, which is only allowed for standalone components. Without thestandalone: trueflag the build will fail with "Component metadata expected a module id…" error.@Component({ + standalone: true, selector: 'al-connected-nodes', templateUrl: 'connected-nodes.component.html', styleUrl: 'connected-nodes.component.scss', imports: [ NodeConnectionPointDirective, NodeConnectionsGroupDirective, RoadmapClusterComponent, RoadmapSecondaryNodeComponent, ], changeDetection: ChangeDetectionStrategy.OnPush, })libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connections-group.directive.ts (1)
25-29:⚠️ Potential issueType narrowing lost – filter does not remove
undefined
Array.filterwithout a user-defined type predicate keeps the union type, soconnectionPoints()is inferred as(NodeConnectionPoint | undefined)[]. This breaks strict‐null builds and IntelliSense later on.- this._connectionPointDirectives() - .map((cp) => cp.nodeConnectionPoint()) - .filter((cp) => cp !== undefined), + this._connectionPointDirectives() + .map((cp) => cp.nodeConnectionPoint()) + .filter((cp): cp is NodeConnectionPoint => cp !== undefined),
🧹 Nitpick comments (4)
libs/blog/roadmap/feature-roadmap/src/lib/build-roadmap-layers-from-dto.ts (1)
120-141: Guard against cyclicpreviousNodeIdchains to prevent infinite loops
getOrderedNodeIdsListwill spin forever if a cycle sneaks into the data (e.g., A → B → A). Add a visited-set safeguard:const orderedNodeIds: string[] = []; - while (nextNodeId) { + const visited = new Set<string>(); + while (nextNodeId) { + if (visited.has(nextNodeId)) { + console.warn('[Roadmap] Detected circular linked list starting at', nextNodeId); + break; + } + visited.add(nextNodeId); orderedNodeIds.push(nextNodeId); nextNodeId = chainedNodeIdsMap[nextNodeId]; }This prevents accidental infinite loops on malformed data.
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connection-point.directive.ts (3)
25-36: Consider handling position updates for dynamic layoutsThe directive calculates its position once after the initial render using
afterNextRender. This works for static layouts, but won't update if the element's position changes dynamically (e.g., during resize events or when parent containers change).Consider adding a resize observer or event listeners if dynamic repositioning is needed:
constructor() { afterNextRender(() => { + this.updateConnectionPoint(); + + // Optional: Add resize observer for dynamic updates + const resizeObserver = new ResizeObserver(() => { + this.updateConnectionPoint(); + }); + resizeObserver.observe(this._elementRef.nativeElement.offsetParent || document.body); + }); +} + +private updateConnectionPoint(): void { const elementOffsetLeft = this._elementRef.nativeElement.offsetLeft; const elementOffsetWidth = this._elementRef.nativeElement.offsetWidth; const roadmapCenterPoint = (this._elementRef.nativeElement.offsetParent?.clientWidth || 0) / 2; // Offset is calculated relative to the center of the roadmap const offsetX = elementOffsetLeft + elementOffsetWidth / 2 - roadmapCenterPoint; this.nodeConnectionPoint.set({ offsetX: Math.abs(offsetX) }); }); }
35-35: Consider preserving directionality informationUsing
Math.abs(offsetX)discards the sign of the offset, which could be useful for determining if the node is to the left or right of center. This might limit flexibility if direction matters in the future.- this.nodeConnectionPoint.set({ offsetX: Math.abs(offsetX) }); + this.nodeConnectionPoint.set({ offsetX });
1-38: Add OnDestroy implementation for cleanupWhile there are no explicit subscriptions to clean up, it's a best practice to implement OnDestroy for directives that might need cleanup in the future (e.g., if you add observers or event listeners).
import { afterNextRender, Directive, ElementRef, inject, + OnDestroy, signal, } from '@angular/core'; export interface NodeConnectionPoint { offsetX: number; } @Directive({ selector: '[alNodeConnectionPoint]', }) -export class NodeConnectionPointDirective { +export class NodeConnectionPointDirective implements OnDestroy { private readonly _elementRef = inject<ElementRef<HTMLElement>>( ElementRef<HTMLElement>, ); readonly nodeConnectionPoint = signal<NodeConnectionPoint | undefined>( undefined, ); + + // For potential future observers or event listeners + ngOnDestroy(): void { + // Cleanup resources if needed + } constructor() { // ... existing code } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (23)
libs/blog-contracts/roadmap/.eslintrc.json(1 hunks)libs/blog-contracts/roadmap/README.md(1 hunks)libs/blog-contracts/roadmap/jest.config.ts(1 hunks)libs/blog-contracts/roadmap/project.json(1 hunks)libs/blog-contracts/roadmap/src/index.ts(1 hunks)libs/blog-contracts/roadmap/src/lib/roadmap-node.ts(1 hunks)libs/blog-contracts/roadmap/src/test-setup.ts(1 hunks)libs/blog-contracts/roadmap/tsconfig.json(1 hunks)libs/blog-contracts/roadmap/tsconfig.lib.json(1 hunks)libs/blog-contracts/roadmap/tsconfig.spec.json(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/build-roadmap-layers-from-dto.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-cluster/roadmap-cluster.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-secondary-node/roadmap-secondary-node.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap-node/src/lib/types/roadmap-node.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connection-point.directive.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connections-group.directive.ts(1 hunks)tsconfig.base.json(2 hunks)
✅ Files skipped from review due to trivial changes (11)
- libs/blog-contracts/roadmap/tsconfig.lib.json
- libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.scss
- libs/blog-contracts/roadmap/README.md
- libs/blog-contracts/roadmap/src/index.ts
- libs/blog-contracts/roadmap/src/test-setup.ts
- libs/blog-contracts/roadmap/jest.config.ts
- libs/blog-contracts/roadmap/src/lib/roadmap-node.ts
- libs/blog-contracts/roadmap/tsconfig.json
- libs/blog-contracts/roadmap/tsconfig.spec.json
- libs/blog-contracts/roadmap/.eslintrc.json
- libs/blog-contracts/roadmap/project.json
🚧 Files skipped from review as they are similar to previous changes (7)
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-secondary-node/roadmap-secondary-node.component.ts
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-cluster/roadmap-cluster.component.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts
- libs/blog/roadmap/ui-roadmap-node/src/lib/types/roadmap-node.ts
- tsconfig.base.json
🧰 Additional context used
🧬 Code Graph Analysis (4)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connection-point.directive.ts (1)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connections-group.directive.ts (1)
Directive(10-65)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connections-group.directive.ts (1)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connection-point.directive.ts (2)
Directive(13-38)NodeConnectionPoint(9-11)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.ts (6)
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts (1)
Component(5-26)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-cluster/roadmap-cluster.component.ts (1)
Component(5-39)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-secondary-node/roadmap-secondary-node.component.ts (1)
Component(5-26)libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts (1)
Component(5-26)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.ts (1)
Component(21-41)libs/blog/roadmap/ui-roadmap-node/src/lib/types/roadmap-node.ts (1)
RoadmapNode(15-15)
libs/blog/roadmap/feature-roadmap/src/lib/build-roadmap-layers-from-dto.ts (3)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.ts (1)
RoadmapLayer(16-19)libs/blog-contracts/roadmap/src/lib/roadmap-node.ts (1)
RoadmapNodeDTO(1-6)libs/blog/roadmap/ui-roadmap-node/src/lib/types/roadmap-node.ts (3)
RoadmapNode(15-15)RoadmapClusterNode(10-13)RoadmapStandardNode(6-8)
🪛 Biome (1.9.4)
libs/blog/roadmap/feature-roadmap/src/lib/build-roadmap-layers-from-dto.ts
[error] 26-26: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (8)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (1)
118-130:focusSelectedNodemixes DOM pixels with SVG world units – verify panning accuracy
getBoundingClientRect()returns CSS-pixel coordinates, whereassvgPanZoom.pan()expects SVG viewport units (after current zoom). Unless the SVG has a 1:1 CSS-to-SVG scaling, the node may be panned to an unexpected offset.Please confirm behaviour on various zoom levels; if misaligned, convert screen coords to SVG coordinates via
svgPanZoom.getRelativePoint()orsvgPanZoom.getSizes().libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connection-point.directive.ts (2)
17-19: Good use of dependency injectionThe directive properly uses Angular's
injectfunction to get theElementRef, following modern Angular practices.
21-23: Well-implemented reactive patternUsing a signal for the connection point data allows for reactive updates and integration with the computed values in the connections group directive.
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.ts (2)
25-27: Good use of reactive inputsUsing Angular's new
input()API creates more readable, declarative inputs and provides a clear default value.
29-30: Protected values match directive defaultsThe
connectorHeightvalue of 40 matches the default in theNodeConnectionsGroupDirective, ensuring consistency. Consider using a shared constant if these values need to be synchronized across components.libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connections-group.directive.ts (3)
10-13: Good directive organization with exportAsUsing
exportAsallows template reference variables to access the directive instance, which is useful for accessing computed properties likegroupConnectionPath.
19-23: Effective content children query with descendants optionThe directive properly uses
contentChildrento query allNodeConnectionPointDirectiveinstances, including those in nested components (viadescendants: true).
35-64: SVG path generation logic is well structuredThe path generation logic is well-organized and handles the connection points effectively:
- Sorts points by their x-offset
- Draws horizontal lines between points
- Adds an arc at the last point
- Adds vertical lines to connect to the actual nodes
| const allNodeDtosMap = roadmapNodesDto.reduce( | ||
| (acc, node) => ({ ...acc, [node.id]: node }), | ||
| {} as { [nodeId: string]: RoadmapNodeDTO }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid O(n²) object spreading inside reduce
Each iteration creates a brand-new object, giving quadratic time & memory for large datasets. Mutate the accumulator instead:
-const allNodeDtosMap = roadmapNodesDto.reduce(
- (acc, node) => ({ ...acc, [node.id]: node }),
- {} as { [nodeId: string]: RoadmapNodeDTO },
-);
+const allNodeDtosMap: { [id: string]: RoadmapNodeDTO } = {};
+for (const dto of roadmapNodesDto) {
+ allNodeDtosMap[dto.id] = dto;
+}This satisfies Biome’s noAccumulatingSpread rule and scales linearly.
📝 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.
| const allNodeDtosMap = roadmapNodesDto.reduce( | |
| (acc, node) => ({ ...acc, [node.id]: node }), | |
| {} as { [nodeId: string]: RoadmapNodeDTO }, | |
| ); | |
| const allNodeDtosMap: { [id: string]: RoadmapNodeDTO } = {}; | |
| for (const dto of roadmapNodesDto) { | |
| allNodeDtosMap[dto.id] = dto; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 26-26: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🤖 Prompt for AI Agents
In libs/blog/roadmap/feature-roadmap/src/lib/build-roadmap-layers-from-dto.ts
around lines 25 to 28, the reduce function uses object spreading inside the
accumulator which causes O(n²) time and memory complexity. To fix this, avoid
spreading the accumulator on each iteration and instead directly mutate the
accumulator object by assigning the new key-value pair. This will improve
performance to linear time and satisfy the noAccumulatingSpread rule.
|
Deployed to dev environment |
|
PR is detected, will deploy to dev environment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/blog/src/assets/roadmap-tiles.json (1)
1-813: Add a JSON Schema and TypeScript typings for validation
This 800+ line JSON defines the core roadmap structure. To catch missing fields, type mismatches, or property-ordering issues, introduce a JSON Schema (e.g.roadmap-tiles.schema.json) and generate corresponding TypeScript interfaces:export interface RoadmapNode { id: string; title: string; articleSlugs: string[]; parentNodeId?: string | null; previousNodeId?: string | null; }Validating against a schema in CI or at compile time will greatly improve maintainability and guard against shape changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/blog/src/assets/roadmap-tiles.json(1 hunks)
🔇 Additional comments (1)
apps/blog/src/assets/roadmap-tiles.json (1)
667-686:✅ Verification successful
Sanitize
idvalues containing special characters
The nodes with"id": "deployment-&-ci/cd"and"id": "bundling-&-optimization"include&and/. If these IDs map directly to HTMLidattributes, CSS selectors, or router fragment IDs, they can break valid selectors or URL parsing.Please verify downstream usage handles these characters safely (e.g., via encoding or sanitization), or consider normalizing IDs (e.g., replace
&withand, remove/). You can search for problematic IDs with:
🏁 Script executed:
rg -Hn '"id": ".*[&/].*"' -A2 apps/blog/src/assets/roadmap-tiles.jsonLength of output: 1281
Let’s verify how these IDs are consumed—specifically whether they’re bound to HTML
idattributes or used in routing/fragments:
🏁 Script executed:
#!/bin/bash # 1. Find where the JSON is imported or fetched rg -Hn "roadmap-tiles.json" -g "*.ts" -g "*.js" -g "*.html" # 2. Search for usages of the `id` property in code and templates rg -Hn "tile\.id" -g "*.ts" -g "*.html" # 3. Look for direct Angular bindings to HTML `id` attributes rg -Hn "\[id\]" -g "*.html"Length of output: 1387
No need to sanitize
idvalues inroadmap-tiles.json
I searched the roadmap feature for any bindings oftile.idto HTML attributes or router fragments and found none. Theseidfields are only used internally (not rendered as DOM IDs or URL hashes), so the&and/characters pose no risk in the current implementation.
| "id": "styling", | ||
| "title": "Styling", | ||
| "articleSlugs": [ | ||
| "angular-material-theming-application-with-material-3", | ||
| "theming-angular-app-its-libraries", | ||
| "angular-styles-masterclass-2", | ||
| "lets-implement-a-theme-switch-like-the-angular-material-site", | ||
| "switch-themes-like-a-fox-based-on-ambient-light-in-your-angular-apps", | ||
| "techniques-to-style-component-host-element-in-angular" | ||
| ], | ||
| "parentNodeId": "components" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure explicit previousNodeId for first‐child nodes
The "styling" node defines a parentNodeId: "components" but omits previousNodeId, whereas most branch starters explicitly set "previousNodeId": null. This inconsistency can lead to undefined vs. null checks in your rendering logic.
Add "previousNodeId": null to any node that begins a new sequence under its parent for consistency and to avoid runtime edge cases.
🤖 Prompt for AI Agents
In apps/blog/src/assets/roadmap-tiles.json between lines 12 and 23, the
"styling" node specifies a parentNodeId but does not include a previousNodeId,
unlike other branch starter nodes that explicitly set previousNodeId to null.
Add "previousNodeId": null to the "styling" node to ensure consistency and
prevent potential runtime issues with undefined vs. null checks in your
rendering logic.
|
Deployed to dev environment |
Summary by CodeRabbit