Feat/angular roadmap #424
Feat/angular roadmap #424
Conversation
## Walkthrough
This 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
| File(s) / Path(s) | Change Summary |
|----------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `libs/blog/roadmap/feature-roadmap/*`<br>`libs/blog/roadmap/ui-roadmap/*`<br>`libs/blog/roadmap/ui-roadmap-node/*` | Introduced new Angular libraries for the roadmap feature and UI components. Added components for roadmap visualization, roadmap layers, SVG controls, node types (primary, secondary, cluster, angular-love), and connection directives. Included SCSS styles, test setup, Jest configs, TypeScript configs, ESLint configs, project configs, and README files. |
| `libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/*`<br>`libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/*`<br>`libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-svg-controls/*`<br>`libs/blog/roadmap/ui-roadmap/src/lib/components/vertical-connector-arrow/*` | Added Angular components and directives for rendering connected nodes, roadmap layers, SVG controls, and vertical connector arrows. Included templates, styles, and logic for dynamic rendering and interaction. |
| `libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connection-point.directive.ts`<br>`libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/node-connections-group.directive.ts` | Added Angular directives to manage node connection points and groups, computing dynamic SVG paths to visually connect roadmap nodes horizontally with arcs and vertical lines. |
| `libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/left-slice.pipe.ts`<br>`libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/right-slice.pipe.ts` | Added Angular pipes to slice arrays into left and right halves for layout purposes in roadmap layers. |
| `libs/blog/roadmap/ui-roadmap/src/lib/types/roadmap-node.ts` | Added TypeScript interfaces and types for roadmap nodes, including base, standard, cluster nodes, and a union type. |
| `libs/blog/roadmap/ui-roadmap/src/index.ts`<br>`libs/blog/roadmap/ui-roadmap-node/src/index.ts` | Added index files to re-export components, types, and modules for simplified imports. |
| `libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts`<br>`feature-roadmap.component.html`<br>`feature-roadmap.component.scss` | Added the main Angular component for the roadmap feature, including logic for loading and transforming roadmap data, SVG pan/zoom integration, and rendering layers. Included template and fade-in animation styles. |
| `libs/blog/roadmap/feature-roadmap/src/index.ts` | Re-exported the main feature component for external use. |
| `libs/blog/roadmap/feature-roadmap/src/lib/build-roadmap-layers-from-dto.ts` | Added a module to transform roadmap node DTOs into structured roadmap layers, including logic to order nodes by previousNodeId and distinguish node types (primary, secondary, cluster). |
| `apps/blog/src/assets/roadmap-tiles.json` | Added a comprehensive JSON file defining the hierarchical structure and metadata for the Angular learning roadmap. |
| `apps/blog/src/assets/i18n/en.json`<br>`apps/blog/src/assets/i18n/pl.json` | Added new localization keys for roadmap page controls in English and Polish. |
| `libs/blog/shell/feature-shell-web/src/lib/blog-shell.routes.ts` | Added a new route for the roadmap feature, loading the shell component asynchronously. |
| `libs/blog/shell/feature-shell-web/src/lib/roadmap-shell.component.ts` | Added a shell component for the roadmap page, integrating header, roadmap feature, and footer, with localization and ad banner logic. |
| `libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts` | Updated the root shell component to set layout CSS classes on the host element. |
| `tsconfig.base.json` | Added TypeScript path aliases for the new roadmap feature and UI libraries. |
| `nx.json` | Updated Angular component generator defaults to set `changeDetection` to `OnPush`. |
| `package.json` | Added the `panzoom` dependency for interactive SVG roadmap features. |
| `libs/blog-contracts/roadmap/*` | Added a new contracts library for roadmap node DTOs, including interfaces, ESLint and Jest configurations, project setup, test setup, and README. |
## Sequence Diagram(s)
```mermaid
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 eventsPossibly related PRs
Suggested reviewers
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 |
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 |
|
PR is detected, will deploy to dev environment |
|
Deployed to dev environment |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (1)
68-73: Add error handling for HTTP requests.The HTTP request lacks error handling, which could lead to poor user experience if the roadmap data fails to load.
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 data + }, }).value.asReadonly();
🧹 Nitpick comments (2)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (2)
83-83: Remove debug console.log statement.Debug console.log statements should be removed before production deployment.
- console.log('afterRenderEffect');
97-125: Clean up debug code and commented lines.The method contains debug statements and commented code that should be removed for production.
- console.log('window Height', window.innerHeight); - console.log(centerY); - console.log(multiplier); - // if (event === 'decrement') svgPanZoom.zoomOut();Also consider implementing the decrement functionality or removing the commented line entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
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/src/lib/components/roadmap-svg-controls/roadmap-svg-controls.component.ts(1 hunks)libs/blog/shell/feature-shell-web/src/lib/roadmap-shell.component.ts(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- 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 comments (5)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-svg-controls/roadmap-svg-controls.component.ts (4)
1-11: Well-structured imports and type definitions.Clean imports and excellent use of TypeScript for type safety. The
EventTypeunion andControlinterface provide clear contracts for the component's behavior.
13-33: Excellent component configuration with modern Angular features.Great use of:
- Standalone component architecture
- Modern
@fortemplate syntax- Accessibility with
aria-labeland transloco- OnPush change detection for performance
- Responsive design with Tailwind classes
34-35: Modern signal-based output implementation.Excellent use of the new
output<EventType>()function, following Angular's latest signal-based patterns.
37-58: Well-organized controls configuration.The controls array is thoughtfully structured with:
- Clear mapping between icons and functionality
- Consistent naming conventions
- Logical order for user experience
- Proper separation between view reset and zoom reset actions
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (1)
1-36: Well-structured imports and configuration.Clean imports and sensible panzoom configuration with appropriate zoom limits and smooth scrolling enabled for better user experience.
| private initPanZoom() { | ||
| const roadmapWrapper = this.roadmapWrapper.nativeElement; | ||
| this.panZoomInstance = panzoom(roadmapWrapper, this._panZoomInitialConfig); | ||
| console.log(this.nodesDto()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug statement and add error handling.
Remove the debug console.log and consider adding error handling for panzoom initialization as suggested in previous reviews.
private initPanZoom() {
+ try {
const roadmapWrapper = this.roadmapWrapper.nativeElement;
this.panZoomInstance = panzoom(roadmapWrapper, this._panZoomInitialConfig);
- console.log(this.nodesDto());
+ } catch (error) {
+ console.error('Failed to initialize pan-zoom:', error);
+ }
}📝 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 initPanZoom() { | |
| const roadmapWrapper = this.roadmapWrapper.nativeElement; | |
| this.panZoomInstance = panzoom(roadmapWrapper, this._panZoomInitialConfig); | |
| console.log(this.nodesDto()); | |
| } | |
| private initPanZoom() { | |
| try { | |
| const roadmapWrapper = this.roadmapWrapper.nativeElement; | |
| this.panZoomInstance = panzoom(roadmapWrapper, this._panZoomInitialConfig); | |
| } catch (error) { | |
| console.error('Failed to initialize pan-zoom:', error); | |
| } | |
| } |
🤖 Prompt for AI Agents
In libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts around
lines 145 to 149, remove the debug console.log statement that outputs
this.nodesDto(). Add error handling around the panzoom initialization by
wrapping the panzoom call in a try-catch block and handle any errors
appropriately, such as logging an error message or fallback behavior.
|
PR is detected, will deploy to dev environment |
|
Deployed to dev environment |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (2)
153-156:panzoominitialisation lacks error handling (repeat).Previous review already suggested wrapping this in
try / catchto avoid hard crashes when the library fails to load. Still missing.
45-49:⚠️ Potential issue
styleUrlshould bestyleUrls.Same compilation issue as flagged elsewhere.
- styleUrl: './feature-roadmap.component.scss', + styleUrls: ['./feature-roadmap.component.scss'],libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.ts (1)
17-27: Missingstandalone: trueand wrongstyleUrlkey.A component providing an
importsarray must be markedstandalone: true, andstyleUrlmust bestyleUrls.-@Component({ +@Component({ + standalone: true, ... - styleUrl: 'connected-nodes.component.scss', + styleUrls: ['connected-nodes.component.scss'],libs/blog/roadmap/feature-roadmap/src/lib/build-roadmap-layers-from-dto.ts (1)
25-28: 🛠️ Refactor suggestionO(n²) spread in reducer – replace with mutable assignment.
The current pattern recreates a fresh object per iteration.
- const allNodeDtosMap = roadmapNodesDto.reduce( - (acc, node) => ({ ...acc, [node.id]: node }), - {} as { [nodeId: string]: RoadmapNodeDTO }, - ); + const allNodeDtosMap: { [nodeId: string]: RoadmapNodeDTO } = {}; + for (const node of roadmapNodesDto) { + allNodeDtosMap[node.id] = node; + }🧰 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 ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🧹 Nitpick comments (13)
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts (1)
31-33: Event/output naming could be clearer
getBottomsheetreads like a query rather than an event. Consistent Angular naming conventions use past-tense (bottomSheetRequested) or imperative (openBottomSheet). Consider renaming for readability across the codebase.libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html (1)
18-34: Repeated signal evaluations – cache the value once per render
layer().childNodesis invoked 4 × in this template. Signals are cheap but the pipes and slice logic are not. Capture the value once with the@letsyntax to avoid redundant work and improve readability:@let childNodes = layer().childNodes ?? []; @if (childNodes.length) { <div class="grid grid-cols-2"> <al-connected-nodes [connectedNodes]="childNodes | leftSlice" ... /> <al-connected-nodes [connectedNodes]="childNodes | rightSlice" ... /> </div> }🧰 Tools
🪛 HTMLHint (1.5.0)
[error] 23-23: The attribute name of [ [connectedNodes] ] must be in lowercase.
(attr-lowercase)
[error] 24-24: The attribute name of [ [nodesDrawingDirection] ] must be in lowercase.
(attr-lowercase)
[error] 25-25: The attribute name of [ (getBottomsheet) ] must be in lowercase.
(attr-lowercase)
[error] 30-30: The attribute name of [ [connectedNodes] ] must be in lowercase.
(attr-lowercase)
[error] 31-31: The attribute name of [ [nodesDrawingDirection] ] must be in lowercase.
(attr-lowercase)
[error] 32-32: The attribute name of [ (getBottomsheet) ] must be in lowercase.
(attr-lowercase)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html (1)
2-3: Outstanding TODO – surface empty/error statesThe roadmap area renders nothing when data is missing or errored. Add an
ngSwitchor similar to present a spinner or friendly error message so the user isn’t left with a blank screen.libs/blog/roadmap/data-access/src/lib/state/roadmap-store.ts (1)
47-50: Duplicate close logic – usefinalizefor DRY error handling
bottomsheetService.close()is called both before the request and again on error. Usingfinalizekeeps the flow symmetrical and future-proof:switchMap(id => roadmapService.getNodeDetails(id).pipe( tapResponse({ next: … }), - finalize(() => bottomsheetService.close()) + finalize(() => bottomsheetService.close()) ) )libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-cluster/roadmap-cluster.component.ts (1)
20-34: Avoid repeatedly dereferencing theclustersignal
cluster()is invoked four times within this block. Even though signal reads are cheap, caching the value improves readability and avoids accidental multiple recomputations once a getter becomes heavier.- <div class="m-[2px] flex flex-col gap-[10px] p-3"> - @for (clusterNode of cluster().clusteredNodes; track clusterNode.id) { + @let cl = cluster(); + <div class="m-[2px] flex flex-col gap-[10px] p-3"> + @for (clusterNode of cl.clusteredNodes; track clusterNode.id) {libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html (1)
28-29: Placeholder renders as visible text
//card-become-an-author//will be rendered to the DOM. Replace it with an HTML comment or a dedicated component.- //card-become-an-author// + <!-- card-become-an-author -->libs/blog/roadmap/data-access/src/lib/infractructure/roadmap.service.ts (1)
56-58: Remove unused injections or use them
_httpand_apiBaseUrlare injected but never referenced, triggeringnoUnusedLocals/noUnusedParametersin strict TS configs. Either remove them or implement the real HTTP call.- private readonly _http = inject(HttpClient); - private readonly _apiBaseUrl = inject(ConfigService).get('apiBaseUrl'); + // private readonly _http = inject(HttpClient); + // private readonly _apiBaseUrl = inject(ConfigService).get('apiBaseUrl');libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts (1)
12-21: Cachenode()in a local template variableSimilar to the cluster component,
node()is called three times here. Store it once for clarity.- <div + @let n = node(); + <div class="roadmap-hover-border-gradient relative w-fit text-nowrap rounded-lg bg-[#FDF5FD] text-[#FDF5FD]" - (click)="getBottomsheet.emit(node().id)" + (click)="getBottomsheet.emit(n.id)" > <div class="relative z-10 m-[4px] rounded-lg bg-[--primary-color] px-6 py-4 text-[24px]" > - {{ node().title }} + {{ n.title }} </div> </div>libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.ts (1)
41-44: Remove strayconsole.logdebugging.Leaving console statements in production code clutters the console and may leak information.
- console.log('closed');libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet.service.ts (1)
34-35: Drop debug logging from service.
console.loginside a shared service pollutes consumers’ logs and hurts performance.- console.log('bottomSheetService data:', nodeDetails);libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (3)
88-93: Left-over debugconsole.log.- console.log(this.nodesDto());Remove or guard behind an environment flag.
158-161: Second strayconsole.log.- console.log('feature,getBottomSheet', event);
153-156: Memory leak:panZoomInstancenever disposed.
ImplementOnDestroyand callthis.panZoomInstance.dispose().+import { OnDestroy } from '@angular/core'; ... -export class FeatureRoadmapComponent { +export class FeatureRoadmapComponent implements OnDestroy { ... + ngOnDestroy(): void { + this.panZoomInstance?.dispose(); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
libs/blog/roadmap/data-access/.eslintrc.json(1 hunks)libs/blog/roadmap/data-access/README.md(1 hunks)libs/blog/roadmap/data-access/jest.config.ts(1 hunks)libs/blog/roadmap/data-access/project.json(1 hunks)libs/blog/roadmap/data-access/src/index.ts(1 hunks)libs/blog/roadmap/data-access/src/lib/infractructure/index.ts(1 hunks)libs/blog/roadmap/data-access/src/lib/infractructure/roadmap.service.ts(1 hunks)libs/blog/roadmap/data-access/src/lib/state/index.ts(1 hunks)libs/blog/roadmap/data-access/src/lib/state/roadmap-store.ts(1 hunks)libs/blog/roadmap/data-access/src/test-setup.ts(1 hunks)libs/blog/roadmap/data-access/tsconfig.json(1 hunks)libs/blog/roadmap/data-access/tsconfig.lib.json(1 hunks)libs/blog/roadmap/data-access/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.html(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/src/index.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/roadmap-bottomsheet/roadmap-bottomsheet.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.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/utils/.eslintrc.json(1 hunks)libs/blog/roadmap/utils/README.md(1 hunks)libs/blog/roadmap/utils/jest.config.ts(1 hunks)libs/blog/roadmap/utils/project.json(1 hunks)libs/blog/roadmap/utils/src/index.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/angular-love-node.type.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/base-node.type.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/content-slug.type.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/creator.type.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/index.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/node-details.type.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/regular-node.type.ts(1 hunks)libs/blog/roadmap/utils/src/lib/services/index.ts(1 hunks)libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet.service.ts(1 hunks)libs/blog/roadmap/utils/src/test-setup.ts(1 hunks)libs/blog/roadmap/utils/tsconfig.json(1 hunks)libs/blog/roadmap/utils/tsconfig.lib.json(1 hunks)libs/blog/roadmap/utils/tsconfig.spec.json(1 hunks)tsconfig.base.json(3 hunks)
✅ Files skipped from review due to trivial changes (30)
- libs/blog/roadmap/data-access/src/lib/state/index.ts
- libs/blog/roadmap/data-access/src/test-setup.ts
- libs/blog/roadmap/utils/src/lib/models/angular-love-node.type.ts
- libs/blog/roadmap/utils/src/lib/models/node-details.type.ts
- libs/blog/roadmap/data-access/src/index.ts
- libs/blog/roadmap/utils/README.md
- libs/blog/roadmap/utils/src/lib/services/index.ts
- libs/blog/roadmap/data-access/src/lib/infractructure/index.ts
- libs/blog/roadmap/utils/src/lib/models/regular-node.type.ts
- libs/blog/roadmap/data-access/tsconfig.json
- libs/blog/roadmap/data-access/tsconfig.lib.json
- libs/blog/roadmap/utils/src/index.ts
- libs/blog/roadmap/utils/src/test-setup.ts
- libs/blog/roadmap/utils/jest.config.ts
- libs/blog/roadmap/data-access/jest.config.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.scss
- libs/blog/roadmap/utils/src/lib/models/content-slug.type.ts
- libs/blog/roadmap/utils/src/lib/models/base-node.type.ts
- libs/blog/roadmap/utils/tsconfig.spec.json
- libs/blog/roadmap/utils/.eslintrc.json
- libs/blog/roadmap/utils/src/lib/models/creator.type.ts
- libs/blog/roadmap/utils/tsconfig.lib.json
- libs/blog/roadmap/utils/project.json
- libs/blog/roadmap/data-access/tsconfig.spec.json
- libs/blog/roadmap/utils/tsconfig.json
- libs/blog/roadmap/data-access/.eslintrc.json
- libs/blog/roadmap/utils/src/lib/models/index.ts
- libs/blog/roadmap/data-access/project.json
- libs/blog/roadmap/data-access/README.md
- tsconfig.base.json
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.ts
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-secondary-node/roadmap-secondary-node.component.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
libs/blog/roadmap/data-access/src/lib/infractructure/roadmap.service.ts (4)
libs/blog/roadmap/utils/src/lib/models/angular-love-node.type.ts (1)
AngularLoveNode(4-7)libs/blog/roadmap/utils/src/lib/models/regular-node.type.ts (1)
RegularNode(4-7)libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet.service.ts (1)
Injectable(13-41)libs/blog/roadmap/utils/src/lib/models/node-details.type.ts (1)
NodeDetails(4-4)
libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet.service.ts (4)
libs/blog/roadmap/utils/src/lib/models/node-details.type.ts (1)
NodeDetails(4-4)libs/blog/roadmap/utils/src/lib/models/angular-love-node.type.ts (1)
AngularLoveNode(4-7)libs/blog/roadmap/utils/src/lib/models/regular-node.type.ts (1)
RegularNode(4-7)libs/blog/roadmap/data-access/src/lib/infractructure/roadmap.service.ts (1)
Injectable(54-66)
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(21-24)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)
🪛 HTMLHint (1.5.0)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html
[error] 1-1: The attribute name of [ #roadmapWrapper ] must be in lowercase.
(attr-lowercase)
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
[error] 11-11: The attribute name of [ [showLayerConnector] ] must be in lowercase.
(attr-lowercase)
[error] 12-12: The attribute name of [ (getBottomSheet) ] must be in lowercase.
(attr-lowercase)
[error] 19-19: The attribute name of [ (resizeRoadmap) ] must be in lowercase.
(attr-lowercase)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
[error] 4-4: The attribute name of [ alNodeConnectionsGroup ] must be in lowercase.
(attr-lowercase)
[error] 5-5: The attribute name of [ #connectionsGroup ] must be in lowercase.
(attr-lowercase)
[error] 8-8: The attribute name of [ [connectorHeight] ] must be in lowercase.
(attr-lowercase)
[error] 32-32: The attribute name of [ alNodeConnectionPoint ] must be in lowercase.
(attr-lowercase)
[error] 34-34: The attribute name of [ (getBottomsheet) ] must be in lowercase.
(attr-lowercase)
[error] 38-38: The attribute name of [ alNodeConnectionPoint ] must be in lowercase.
(attr-lowercase)
[error] 40-40: The attribute name of [ (getBottomsheet) ] must be in lowercase.
(attr-lowercase)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
[error] 5-5: The attribute name of [ (getBottomsheet) ] must be in lowercase.
(attr-lowercase)
[error] 10-10: The attribute name of [ (getBottomsheet) ] must be in lowercase.
(attr-lowercase)
[error] 23-23: The attribute name of [ [connectedNodes] ] must be in lowercase.
(attr-lowercase)
[error] 24-24: The attribute name of [ [nodesDrawingDirection] ] must be in lowercase.
(attr-lowercase)
[error] 25-25: The attribute name of [ (getBottomsheet) ] must be in lowercase.
(attr-lowercase)
[error] 30-30: The attribute name of [ [connectedNodes] ] must be in lowercase.
(attr-lowercase)
[error] 31-31: The attribute name of [ [nodesDrawingDirection] ] must be in lowercase.
(attr-lowercase)
[error] 32-32: The attribute name of [ (getBottomsheet) ] must be in lowercase.
(attr-lowercase)
🪛 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 (3)
libs/blog/roadmap/ui-roadmap/src/index.ts (1)
1-3: Re-export file looks goodStraightforward barrel file – no issues spotted.
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-cluster/roadmap-cluster.component.ts (1)
40-41: VerifystyleUrlvsstyleUrls
styleUrlis only recognised in Angular v17+ (stand-alone components). If your workspace still targets ≤ v16 anywhere, this will break compilation. Please confirm the global target or switch back tostyleUrls: ['roadmap-cluster.component.scss'].libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts (1)
24-25: ConfirmstyleUrlsupportSame concern as in
RoadmapClusterComponent: ensure the project tree is on Angular 17+, or switch to the pluralstyleUrls.
| </div> | ||
| </div> | ||
| `, | ||
| styleUrl: 'roadmap-angular-love-node.component.scss', |
There was a problem hiding this comment.
styleUrl is not a valid Angular metadata key – use the plural styleUrls array
Angular’s @Component metadata accepts styleUrls (array) or inline styles, there is no single-string styleUrl. Using an unsupported key will silently ignore your stylesheet and break styling in prod builds.
- styleUrl: 'roadmap-angular-love-node.component.scss',
+ styleUrls: ['roadmap-angular-love-node.component.scss'],🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts
at line 24, replace the incorrect single string property 'styleUrl' with the
correct plural 'styleUrls' and provide the stylesheet path as an array
containing that string. This change ensures Angular properly loads the component
styles.
| [layer]="layer" | ||
| [showLayerConnector]="!last" | ||
| (getBottomSheet)="getBottomSheet($event)" | ||
| /> |
There was a problem hiding this comment.
Event name casing mismatch may break binding
(getBottomSheet) (capital “S”) differs from the lower-case getBottomsheet emitted by child components. Angular treats event names case-sensitively; this will never fire.
- (getBottomSheet)="getBottomSheet($event)"
+ (getBottomsheet)="getBottomSheet($event)"📝 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.
| [layer]="layer" | |
| [showLayerConnector]="!last" | |
| (getBottomSheet)="getBottomSheet($event)" | |
| /> | |
| [layer]="layer" | |
| [showLayerConnector]="!last" | |
| (getBottomsheet)="getBottomSheet($event)" | |
| /> |
🧰 Tools
🪛 HTMLHint (1.5.0)
[error] 11-11: The attribute name of [ [showLayerConnector] ] must be in lowercase.
(attr-lowercase)
[error] 12-12: The attribute name of [ (getBottomSheet) ] must be in lowercase.
(attr-lowercase)
🤖 Prompt for AI Agents
In libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html
around lines 10 to 13, the event binding uses (getBottomSheet) with a capital
"S", but the child component emits the event as getBottomsheet with a lowercase
"s". To fix this, change the event binding to (getBottomsheet) to exactly match
the emitted event name, ensuring the event handler will be triggered correctly.
| <div class="mx-16 flex gap-8"> | ||
| @for (node of connectedNodes(); track node.id) { | ||
| @if (node.nodeType === 'secondary') { | ||
| <al-roadmap-secondary-node | ||
| alNodeConnectionPoint | ||
| [node]="node" | ||
| (getBottomsheet)="getBottomsheet.emit($event)" | ||
| /> | ||
| } @else if (node.nodeType === 'cluster') { | ||
| <al-roadmap-cluster | ||
| alNodeConnectionPoint | ||
| [cluster]="node" | ||
| (getBottomsheet)="getBottomsheet.emit($event)" | ||
| /> | ||
| } | ||
| } | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Directional spacing regression
The dynamic left / right margins previously handled with ms-* / me-* are now hard-coded to mx-16, so the layout no longer adapts when nodesDrawingDirection() changes. Re-introduce conditional spacing (either [ngClass] or two bound attributes) to keep the node strip aligned.
- <div class="mx-16 flex gap-8">
+ <div
+ class="flex gap-8"
+ [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="mx-16 flex gap-8"> | |
| @for (node of connectedNodes(); track node.id) { | |
| @if (node.nodeType === 'secondary') { | |
| <al-roadmap-secondary-node | |
| alNodeConnectionPoint | |
| [node]="node" | |
| (getBottomsheet)="getBottomsheet.emit($event)" | |
| /> | |
| } @else if (node.nodeType === 'cluster') { | |
| <al-roadmap-cluster | |
| alNodeConnectionPoint | |
| [cluster]="node" | |
| (getBottomsheet)="getBottomsheet.emit($event)" | |
| /> | |
| } | |
| } | |
| </div> | |
| <div | |
| class="flex gap-8" | |
| [ngClass]="{ | |
| 'ms-[50px]': nodesDrawingDirection() === 'to-right', | |
| 'me-[50px]': nodesDrawingDirection() === 'to-left' | |
| }" | |
| > | |
| @for (node of connectedNodes(); track node.id) { | |
| @if (node.nodeType === 'secondary') { | |
| <al-roadmap-secondary-node | |
| alNodeConnectionPoint | |
| [node]="node" | |
| (getBottomsheet)="getBottomsheet.emit($event)" | |
| /> | |
| } @else if (node.nodeType === 'cluster') { | |
| <al-roadmap-cluster | |
| alNodeConnectionPoint | |
| [cluster]="node" | |
| (getBottomsheet)="getBottomsheet.emit($event)" | |
| /> | |
| } | |
| } | |
| </div> |
🧰 Tools
🪛 HTMLHint (1.5.0)
[error] 32-32: The attribute name of [ alNodeConnectionPoint ] must be in lowercase.
(attr-lowercase)
[error] 34-34: The attribute name of [ (getBottomsheet) ] must be in lowercase.
(attr-lowercase)
[error] 38-38: The attribute name of [ alNodeConnectionPoint ] must be in lowercase.
(attr-lowercase)
[error] 40-40: The attribute name of [ (getBottomsheet) ] must be in lowercase.
(attr-lowercase)
🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html
between lines 28 and 44, the horizontal margins are hard-coded with mx-16, which
breaks directional spacing when nodesDrawingDirection() changes. Replace the
fixed mx-16 class with conditional margin classes using either [ngClass] or
separate bound attributes to apply ms-16 or me-16 dynamically based on the
current nodesDrawingDirection() value, restoring adaptive left/right spacing.
| <a [href]="creator.slug" target="_blank">{{ creator.name }}</a> | ||
| </li> | ||
| } | ||
| </ul> |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to external links
Opening links with target="_blank" without rel="noopener noreferrer" exposes the app to tab-nabbing. Add the attribute to every external anchor.
-<a [href]="creator.slug" target="_blank">{{ creator.name }}</a>
+<a
+ [href]="creator.slug"
+ target="_blank"
+ rel="noopener noreferrer"
+>
+ {{ creator.name }}
+</a>Repeat for the article and movie links below.
Also applies to: 35-36, 46-47
🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html
at lines 20-23, 35-36, and 46-47, the anchor tags with target="_blank" are
missing rel="noopener noreferrer". To fix this, add rel="noopener noreferrer" to
all external anchor tags that open in a new tab to prevent tab-nabbing security
risks.
| templateUrl: './roadmap-bottomsheet.component.html', | ||
| styleUrl: 'roadmap-bottomsheet.component.scss', | ||
| }) |
There was a problem hiding this comment.
Incorrect styleUrl key – must be styleUrls.
Angular only recognises styleUrls: string[]. Using styleUrl will break compilation for this standalone component.
- styleUrl: 'roadmap-bottomsheet.component.scss',
+ styleUrls: ['roadmap-bottomsheet.component.scss'],🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.ts
around lines 13 to 15, the component metadata uses the incorrect key `styleUrl`
instead of `styleUrls`. Change `styleUrl` to `styleUrls` and ensure its value is
an array of strings, e.g., ['roadmap-bottomsheet.component.scss'], to fix the
Angular compilation error.
| protected readonly isContentInRegularNodeDetails = computed(() => { | ||
| const regularNodeDetails = this.regularNodeDetails(); | ||
| if (regularNodeDetails?.movies.length && regularNodeDetails.articles.length) | ||
| return true; | ||
| else return false; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Logical flaw in isContentInRegularNodeDetails check.
You currently require BOTH movies and articles to have length.
If the intention is “show when at least one has content”, change to an OR.
-if (regularNodeDetails?.movies.length && regularNodeDetails.articles.length)
+if (
+ (regularNodeDetails?.movies.length ?? 0) > 0 ||
+ (regularNodeDetails?.articles.length ?? 0) > 0
+)🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.ts
around lines 28 to 33, the computed property isContentInRegularNodeDetails
incorrectly requires both movies and articles arrays to have content. To fix
this, change the condition to return true if either movies or articles has
length, using a logical OR instead of AND.
DamianBrzezinskiHoA
force-pushed
the
feat/angular-roadmap
branch
from
3a60fd7 to
303c221
Compare
|
PR is detected, will deploy to dev environment |
|
Deploy failed, please check the logs in jenkins for more details. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html (1)
31-34:⚠️ Potential issueAdd
rel="noopener noreferrer"to all links opened withtarget="_blank"This prevents tab-nabbing and satisfies security best-practices already pointed out in a previous review.
-<a [href]="'author/' + creator.slug" target="_blank"> +<a [href]="'author/' + creator.slug" target="_blank" rel="noopener noreferrer"> … -<a [href]="article.url" target="_blank"> +<a [href]="article.url" target="_blank" rel="noopener noreferrer"> … -<a [href]="movie.url" target="_blank"> +<a [href]="movie.url" target="_blank" rel="noopener noreferrer">Also applies to: 52-55, 65-68
libs/blog/roadmap/feature-roadmap/src/lib/build-roadmap-layers-from-dto.ts (1)
25-28:⚠️ Potential issueQuadratic spread on accumulator not addressed
The
reducestill clones an ever-growing object on every iteration – O(n²) time & memory for large datasets. The refactor suggested earlier (mutating the accumulator or usingMap) should be applied.🧰 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 ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html (1)
10-13:⚠️ Potential issueEvent name mismatch – will never fire
The child component emits
getBottomsheet(lower-case “s”), yet the template listens forgetBottomSheet. Angular event names are case-sensitive, so the handler is never invoked.- (getBottomSheet)="getBottomSheet($event)" + (getBottomsheet)="getBottomSheet($event)"libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html (1)
28-28: Restore adaptive margins with conditional classes
The hard-codedmx-16breaks dynamic spacing based onisLeftDrawingDirection. Reintroduce conditional margins using[ngClass]or class bindings.Example diff:
- <div class="mx-16 flex gap-8"> + <div + class="flex gap-8" + [ngClass]="{ + 'ms-16': !isLeftDrawingDirection, + 'me-16': isLeftDrawingDirection + }" + >
🧹 Nitpick comments (4)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html (1)
14-16: Consider moving the layer-connector into its own@ifbranch for clarityThe connector is rendered unconditionally once
showLayerConnector()returnstrue. Because the surrounding template already branches on the parent node type, grouping the connector markup together with the parent node markup (or extracting it into a small presentational component) would improve readability and make future styling changes less error-prone.libs/blog/roadmap/feature-roadmap/src/lib/build-roadmap-layers-from-dto.ts (1)
21-23: Empty input drops the “introduction” layerWhen
roadmapNodesDtoisundefinedthe function now returns an empty array. This omits the mandatoryroadmapTitleLayerand forces callers to handle the special case. Consider always returning the intro layer so UI code can stay simple.-if (!roadmapNodesDto) { - return []; -} +if (!roadmapNodesDto) { + return [roadmapTitleLayer]; +}libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html (2)
31-41: Use kebab-case for directive selectors
Angular templates normalize attribute names to lowercase-hyphenated syntax. UpdatealNodeConnectionPointtoal-node-connection-point.Example diff:
- <al-roadmap-secondary-node - alNodeConnectionPoint + <al-roadmap-secondary-node + al-node-connection-point [node]="node" (getBottomsheet)="getBottomsheet.emit($event)" - /> + /> @@ - <al-roadmap-cluster - alNodeConnectionPoint + <al-roadmap-cluster + al-node-connection-point [cluster]="node" (getBottomsheet)="getBottomsheet.emit($event)" - /> + />
18-25: Simplify SVG element to<path>
Using<svg:path>is redundant. You can use the<path>tag directly for better readability.Example diff:
- <svg:path + <path [attr.d]="connectionsGroup.groupConnectionPath()" [attr.stroke-width]="connectorStrokeWidth" fill="none" stroke="#FDF5FD" stroke-linecap="round" stroke-linejoin="round" - /> + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
libs/blog/roadmap/data-access/.eslintrc.json(1 hunks)libs/blog/roadmap/data-access/README.md(1 hunks)libs/blog/roadmap/data-access/jest.config.ts(1 hunks)libs/blog/roadmap/data-access/project.json(1 hunks)libs/blog/roadmap/data-access/src/index.ts(1 hunks)libs/blog/roadmap/data-access/src/lib/infractructure/index.ts(1 hunks)libs/blog/roadmap/data-access/src/lib/infractructure/roadmap.service.ts(1 hunks)libs/blog/roadmap/data-access/src/lib/state/index.ts(1 hunks)libs/blog/roadmap/data-access/src/lib/state/roadmap-store.ts(1 hunks)libs/blog/roadmap/data-access/src/test-setup.ts(1 hunks)libs/blog/roadmap/data-access/tsconfig.json(1 hunks)libs/blog/roadmap/data-access/tsconfig.lib.json(1 hunks)libs/blog/roadmap/data-access/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.html(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/src/index.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/roadmap-bottomsheet/roadmap-bottomsheet.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.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/utils/.eslintrc.json(1 hunks)libs/blog/roadmap/utils/README.md(1 hunks)libs/blog/roadmap/utils/jest.config.ts(1 hunks)libs/blog/roadmap/utils/project.json(1 hunks)libs/blog/roadmap/utils/src/index.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/angular-love-node.type.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/base-node.type.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/content-slug.type.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/creator.type.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/index.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/node-details.type.ts(1 hunks)libs/blog/roadmap/utils/src/lib/models/regular-node.type.ts(1 hunks)libs/blog/roadmap/utils/src/lib/services/index.ts(1 hunks)libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet.service.ts(1 hunks)libs/blog/roadmap/utils/src/test-setup.ts(1 hunks)libs/blog/roadmap/utils/tsconfig.json(1 hunks)libs/blog/roadmap/utils/tsconfig.lib.json(1 hunks)libs/blog/roadmap/utils/tsconfig.spec.json(1 hunks)tsconfig.base.json(3 hunks)
✅ Files skipped from review due to trivial changes (5)
- libs/blog/roadmap/data-access/src/lib/infractructure/index.ts
- libs/blog/roadmap/utils/src/lib/services/index.ts
- libs/blog/roadmap/utils/tsconfig.spec.json
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.scss
- tsconfig.base.json
🚧 Files skipped from review as they are similar to previous changes (37)
- libs/blog/roadmap/data-access/README.md
- libs/blog/roadmap/utils/README.md
- libs/blog/roadmap/utils/src/test-setup.ts
- libs/blog/roadmap/utils/src/lib/models/angular-love-node.type.ts
- libs/blog/roadmap/utils/src/lib/models/creator.type.ts
- libs/blog/roadmap/data-access/src/lib/state/index.ts
- libs/blog/roadmap/ui-roadmap/src/index.ts
- libs/blog/roadmap/utils/src/lib/models/regular-node.type.ts
- libs/blog/roadmap/data-access/src/test-setup.ts
- libs/blog/roadmap/data-access/tsconfig.spec.json
- libs/blog/roadmap/utils/src/lib/models/node-details.type.ts
- libs/blog/roadmap/utils/project.json
- libs/blog/roadmap/data-access/jest.config.ts
- libs/blog/roadmap/data-access/tsconfig.lib.json
- libs/blog/roadmap/utils/tsconfig.lib.json
- libs/blog/roadmap/utils/jest.config.ts
- libs/blog/roadmap/utils/.eslintrc.json
- libs/blog/roadmap/utils/tsconfig.json
- libs/blog/roadmap/data-access/project.json
- libs/blog/roadmap/data-access/tsconfig.json
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-secondary-node/roadmap-secondary-node.component.ts
- libs/blog/roadmap/utils/src/lib/models/index.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.ts
- libs/blog/roadmap/utils/src/lib/models/base-node.type.ts
- libs/blog/roadmap/data-access/src/index.ts
- libs/blog/roadmap/data-access/src/lib/state/roadmap-store.ts
- libs/blog/roadmap/utils/src/lib/models/content-slug.type.ts
- libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet.service.ts
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts
- libs/blog/roadmap/data-access/.eslintrc.json
- libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
- libs/blog/roadmap/utils/src/index.ts
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-cluster/roadmap-cluster.component.ts
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts
- libs/blog/roadmap/data-access/src/lib/infractructure/roadmap.service.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
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(21-24)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)
🪛 HTMLHint (1.5.0)
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (1)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html (1)
1-2: Verify template macro support
The usage of@let,@for, and@ifmacros requires Angular’s template macro feature to be enabled and properly configured. Ensure yourtsconfig.jsonand compiler settings allow these macros and that the necessary imports (e.g., from@angular/compiler-clior custom macro directives) are in place.🧰 Tools
🪛 HTMLHint (1.5.0)
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
|
PR is detected, will deploy to dev environment |
|
Deployed to dev environment |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html (2)
47-57:⚠️ Potential issueAdd
rel="noopener noreferrer"to external article linksOpening external URLs with
target="_blank"withoutrelis a tab-nabbing risk.
Apply this diff to secure the link:- <a [href]="article.url" target="_blank"> + <a [href]="article.url" target="_blank" rel="noopener noreferrer">
63-71:⚠️ Potential issueAdd
rel="noopener noreferrer"to external movie linksSame tab-nabbing vulnerability applies here.
- <a [href]="movie.url" target="_blank"> + <a [href]="movie.url" target="_blank" rel="noopener noreferrer">
🧹 Nitpick comments (4)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html (4)
5-8: Simplify redundant visibility classesSince the outer
@ifguard only renders this block whennodeDetailsis truthy,nodeDetails === undefinedcan never occur inside. You can remove thehiddenclass logic and always apply a single.visibleclass to reduce complexity:- [ngClass]="{ - visible: nodeDetails !== undefined, - hidden: nodeDetails === undefined, - }" + class="bottomsheet visible"
29-35: Use a unique trackBy and internal routing helperAvoid using the index as a track identifier—prefer a stable key like
creator.slug. Also, for internal navigation links, use Angular’srouterLinkwith an absolute path:- @for (creator of angularLoveNodeDetails.creators; track $index) { + @for (creator of angularLoveNodeDetails.creators; track creator.slug) { ... - <a [href]="'author/' + creator.slug" target="_blank"> + <a [routerLink]="['/author', creator.slug]" target="_blank" rel="noopener noreferrer">
41-43: Implement fallback content for empty nodesThe placeholder comment
<!-- card-become-an-author -->indicates missing UI for nodes without content. Please flesh out or remove this TODO to avoid a broken blank state.
2-12: Enhance accessibility with ARIA rolesMark the overlay and sheet with appropriate ARIA attributes for screen readers and focus management. For example:
- <div class="overlay" (pointerup)="handleOverlayClick()"> + <div class="overlay" role="dialog" aria-modal="true" aria-labelledby="roadmap-title" (pointerup)="handleOverlayClick()"> ... - <h2>{{ nodeDetails.title }}</h2> + <h2 id="roadmap-title">{{ nodeDetails.title }}</h2>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/blog/roadmap/data-access/src/lib/state/roadmap-store.ts(1 hunks)libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.ts(1 hunks)libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/blog/roadmap/data-access/src/lib/state/roadmap-store.ts
- libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
- libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet.service.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.ts
🧰 Additional context used
🪛 HTMLHint (1.5.0)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
|
PR is detected, will deploy to dev environment |
|
Deployed to dev environment |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html (1)
28-36: Directional spacing regression – re-introduce dynamic marginHard-coded
mx-16removes the left/right mirroring that was present earlier and makes the strip look offset whennodesDrawingDirection()flips.- <div class="mx-16 flex gap-8"> + <div + class="flex gap-8" + [ngClass]="{ + 'ms-16': nodesDrawingDirection() === 'to-right', + 'me-16': nodesDrawingDirection() === 'to-left' + }" + >This restores adaptive alignment and prevents visual jitter when drawing direction changes.
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html (1)
30-33: Addrel="noopener noreferrer"to external linksOpening links with
target="_blank"without this attribute exposes the app to tab-nabbing attacks.-<a … target="_blank"> +<a … target="_blank" rel="noopener noreferrer">Apply to every anchor that opens a new tab.
Also applies to: 51-54, 64-67
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts (1)
27-28:⚠️ Potential issueInvalid Angular metadata key – use
styleUrlsAngular ignores
styleUrl, so the SCSS is never loaded.- styleUrl: 'roadmap-angular-love-node.component.scss', + styleUrls: ['roadmap-angular-love-node.component.scss'],
🧹 Nitpick comments (5)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html (1)
13-27: Avoid repeated function calls, use safe navigation, and fix self-closing tags
- Cache
layer()in a local variable via<ng-container *ngIf="layer() as currentLayer">to prevent multiple calls.- Use safe navigation:
currentLayer.childNodes?.length > 0instead of(layer().childNodes || []).length.- Close
<al-connected-nodes>tags explicitly.Example refactor:
- @if ((layer().childNodes || []).length) { - <div class="grid grid-cols-2"> - <al-connected-nodes [connectedNodes]="layer().childNodes | leftSlice" [nodesDrawingDirection]="'to-left'" /> - <al-connected-nodes [connectedNodes]="layer().childNodes | rightSlice" [nodesDrawingDirection]="'to-right'" /> - </div> - } + <ng-container *ngIf="layer() as currentLayer"> + <ng-container *ngIf="currentLayer.childNodes?.length > 0"> + <div class="grid grid-cols-2"> + <al-connected-nodes + [connectedNodes]="currentLayer.childNodes | leftSlice" + [nodesDrawingDirection]="'to-left'"> + </al-connected-nodes> + <al-connected-nodes + [connectedNodes]="currentLayer.childNodes | rightSlice" + [nodesDrawingDirection]="'to-right'"> + </al-connected-nodes> + </div> + </ng-container> + </ng-container>libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-description/roadmap-bottomsheet-description.component.html (2)
1-3: Consider making the header text translatable
Descriptionis hard-coded, which prevents localisation. Angular provides thei18nattribute for templates.- <h4 class="font-semibold">Description</h4> + <h4 i18n="@@roadmap.bottomSheet.descriptionTitle" class="font-semibold"> + Description + </h4>🧰 Tools
🪛 HTMLHint (1.5.0)
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
5-6: Minor accessibility enhancement
<p class="text-justify …">is fine, but long justified text can reduce readability for dyslexic users. Consider left-aligning or providing a max-width on large screens.libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts (1)
1-7:outputimport is unused
outputis imported but never referenced. Remove it to keep the import list clean and avoid ESLint “unused” warnings.libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-secondary-node/roadmap-secondary-node.component.ts (1)
1-7: UnusedoutputimportSame as the previous component – delete to silence unused-import lint errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
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/src/lib/components/connected-nodes/connected-nodes.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-description/roadmap-bottomsheet-description.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-description/roadmap-bottomsheet-description.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html(1 hunks)libs/blog/roadmap/utils/src/lib/services/index.ts(1 hunks)libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet-menager.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.scss
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.scss
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-cluster/roadmap-cluster.component.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.ts
- libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts
- libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-primary-node/roadmap-primary-node.component.ts
🧰 Additional context used
🪛 HTMLHint (1.5.0)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-description/roadmap-bottomsheet-description.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (1)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html (1)
46-49:track article.titlerisks non-unique keysTwo articles can share the same title, resulting in unstable DOM reuse. Prefer URL (unique) or the loop index.
-@for (article of regularNodeDetails.articles; track article.title) +@for (article of regularNodeDetails.articles; track article.url)
| @if (showLayerConnector()) { | ||
| <al-vertical-connector-arrow class="absolute" /> | ||
| } |
There was a problem hiding this comment.
Use supported conditional directives and close component tags properly
Replace the @if sugar with a standard *ngIf="showLayerConnector()" and convert the self-closing:
- @if (showLayerConnector()) {
- <al-vertical-connector-arrow class="absolute" />
- }
+ <ng-container *ngIf="showLayerConnector()">
+ <al-vertical-connector-arrow class="absolute"></al-vertical-connector-arrow>
+ </ng-container>📝 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.
| @if (showLayerConnector()) { | |
| <al-vertical-connector-arrow class="absolute" /> | |
| } | |
| <ng-container *ngIf="showLayerConnector()"> | |
| <al-vertical-connector-arrow class="absolute"></al-vertical-connector-arrow> | |
| </ng-container> |
🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html
around lines 8 to 10, replace the unsupported `@if` directive with the Angular
standard `*ngIf="showLayerConnector()"` on the <al-vertical-connector-arrow>
element. Also, change the self-closing tag to a properly closed tag by adding a
closing tag or using the Angular syntax for void elements.
| @if (layer().parentNode.nodeType === 'primary') { | ||
| <al-roadmap-primary-node [node]="layer().parentNode" /> | ||
| } @else if (layer().parentNode.nodeType === 'angular-love') { | ||
| <al-roadmap-angular-love-node [node]="layer().parentNode" /> | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure compatible conditional directives and fix self-closing tags
Angular’s HTML parser doesn’t support self-closing tags for custom components, and the @if sugar may not be recognized depending on your Angular version. Convert:
- @if (layer().parentNode.nodeType === 'primary') {
- <al-roadmap-primary-node [node]="layer().parentNode" />
- } @else if (layer().parentNode.nodeType === 'angular-love') {
- <al-roadmap-angular-love-node [node]="layer().parentNode" />
- }to standard syntax, for example:
- <ng-container *ngIf="layer().parentNode.nodeType === 'primary'">
- <al-roadmap-primary-node [node]="layer().parentNode"></al-roadmap-primary-node>
- </ng-container>
- <ng-container *ngIf="layer().parentNode.nodeType === 'angular-love'">
- <al-roadmap-angular-love-node [node]="layer().parentNode"></al-roadmap-angular-love-node>
- </ng-container>To determine whether the @if sugar is supported in this project, let’s check the Angular core version:
🏁 Script executed:
#!/usr/bin/env bash
# Extract the @angular/core version from package.json
grep -E '"@angular/core"' package.json | sed -E 's/.*: *"([^"]+)".*/\1/'Length of output: 81
Use standard *ngIf directives and explicit closing tags
Angular 19.2.5 does not support the @if template sugar for structural directives, and custom components must have a separate closing tag rather than a self-closing one. Please update:
• File: libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html (lines 2–6)
- @if (layer().parentNode.nodeType === 'primary') {
- <al-roadmap-primary-node [node]="layer().parentNode" />
- } @else if (layer().parentNode.nodeType === 'angular-love') {
- <al-roadmap-angular-love-node [node]="layer().parentNode" />
- }
+ <ng-container *ngIf="layer().parentNode.nodeType === 'primary'">
+ <al-roadmap-primary-node [node]="layer().parentNode"></al-roadmap-primary-node>
+ </ng-container>
+ <ng-container *ngIf="layer().parentNode.nodeType === 'angular-love'">
+ <al-roadmap-angular-love-node [node]="layer().parentNode"></al-roadmap-angular-love-node>
+ </ng-container>🧰 Tools
🪛 HTMLHint (1.5.0)
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html
around lines 2 to 6, replace the unsupported @if conditional syntax with
standard Angular *ngIf structural directives, and change self-closing custom
component tags to explicit opening and closing tags to comply with Angular
19.2.5 requirements.
| [ngClass]="{ | ||
| 'bg-[--secondary-color]:': nodeType() === 'regular', | ||
| 'bg-gradient-to-r from-[--secondary-color] to-[--gradient-color]': | ||
| nodeType() === 'angular-love', | ||
| }" |
There was a problem hiding this comment.
Trailing colon renders an invalid Tailwind class
'bg-[--secondary-color]:' keeps the colon, producing an invalid CSS class at runtime.
- 'bg-[--secondary-color]:': nodeType() === 'regular',
+ 'bg-[--secondary-color]': nodeType() === 'regular',📝 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.
| [ngClass]="{ | |
| 'bg-[--secondary-color]:': nodeType() === 'regular', | |
| 'bg-gradient-to-r from-[--secondary-color] to-[--gradient-color]': | |
| nodeType() === 'angular-love', | |
| }" | |
| [ngClass]="{ | |
| 'bg-[--secondary-color]': nodeType() === 'regular', | |
| 'bg-gradient-to-r from-[--secondary-color] to-[--gradient-color]': | |
| nodeType() === 'angular-love', | |
| }" |
🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.html
between lines 3 and 7, remove the trailing colon from the Tailwind class
'bg-[--secondary-color]:'. This colon causes an invalid CSS class at runtime.
Update the class to 'bg-[--secondary-color]' without the colon to fix the issue.
| export * from './roadmap-bottomsheet.service'; | ||
| export * from './roadmap-bottomsheet-menager.service'; |
There was a problem hiding this comment.
Typo in exported filename will break builds
menager → manager.
-export * from './roadmap-bottomsheet-menager.service';
+export * from './roadmap-bottomsheet-manager.service';Verify the actual filename on disk and rename consistently.
📝 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.
| export * from './roadmap-bottomsheet.service'; | |
| export * from './roadmap-bottomsheet-menager.service'; | |
| export * from './roadmap-bottomsheet.service'; | |
| export * from './roadmap-bottomsheet-manager.service'; |
🤖 Prompt for AI Agents
In libs/blog/roadmap/utils/src/lib/services/index.ts at lines 1 to 2, there is a
typo in the exported filename 'roadmap-bottomsheet-menager.service' which should
be 'roadmap-bottomsheet-manager.service'. Verify the correct filename on disk
and update the export statement to use the correct spelling. Also, rename the
actual file if necessary to ensure consistency and prevent build errors.
| import { Component, input } from '@angular/core'; | ||
|
|
||
| @Component({ | ||
| selector: 'al-roadmap-bottomsheet-description', | ||
| templateUrl: 'roadmap-bottomsheet-description.component.html', | ||
| host: { | ||
| class: 'px-6 py-6', | ||
| }, | ||
| }) | ||
| export class RoadmapBottomsheetDescriptionComponent { | ||
| description = input.required<string>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mark component as standalone and use OnPush
imports aren’t specified, so this looks like a standalone component. Declaring standalone: true and changeDetection: ChangeDetectionStrategy.OnPush enables optimal tree-shaking and performance.
-import { Component, input } from '@angular/core';
+import { Component, input, ChangeDetectionStrategy } from '@angular/core';
@Component({
selector: 'al-roadmap-bottomsheet-description',
templateUrl: 'roadmap-bottomsheet-description.component.html',
host: {
class: 'px-6 py-6',
},
+ standalone: true,
+ changeDetection: ChangeDetectionStrategy.OnPush,
})📝 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.
| import { Component, input } from '@angular/core'; | |
| @Component({ | |
| selector: 'al-roadmap-bottomsheet-description', | |
| templateUrl: 'roadmap-bottomsheet-description.component.html', | |
| host: { | |
| class: 'px-6 py-6', | |
| }, | |
| }) | |
| export class RoadmapBottomsheetDescriptionComponent { | |
| description = input.required<string>(); | |
| import { Component, input, ChangeDetectionStrategy } from '@angular/core'; | |
| @Component({ | |
| selector: 'al-roadmap-bottomsheet-description', | |
| templateUrl: 'roadmap-bottomsheet-description.component.html', | |
| host: { | |
| class: 'px-6 py-6', | |
| }, | |
| standalone: true, | |
| changeDetection: ChangeDetectionStrategy.OnPush, | |
| }) | |
| export class RoadmapBottomsheetDescriptionComponent { | |
| description = input.required<string>(); | |
| } |
🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-description/roadmap-bottomsheet-description.component.ts
around lines 1 to 11, the component is missing the standalone flag and OnPush
change detection strategy. Add `standalone: true` to the @Component decorator
and import `ChangeDetectionStrategy` from '@angular/core'. Then set
`changeDetection: ChangeDetectionStrategy.OnPush` in the decorator to improve
performance and enable better tree-shaking.
| import { NgClass, TitleCasePipe } from '@angular/common'; | ||
| import { Component, input } from '@angular/core'; | ||
|
|
||
| import { NodeDetails } from '@angular-love/roadmap-utils'; | ||
|
|
||
| @Component({ | ||
| selector: 'al-roadmap-bottomsheet-header', | ||
| imports: [TitleCasePipe, NgClass], | ||
| templateUrl: './roadmap-bottomsheet-header.component.html', | ||
| styleUrl: 'roadmap-bottomsheet-header.component.scss', | ||
| }) |
There was a problem hiding this comment.
Missing standalone: true, wrong styleUrl, and no OnPush
The presence of the imports array means this must be a standalone component. Additionally, Angular expects styleUrls (plural). Lack of OnPush will default to default CD strategy, less efficient for mostly-static UI.
-import { NgClass, TitleCasePipe } from '@angular/common';
-import { Component, input } from '@angular/core';
+import { NgClass, TitleCasePipe } from '@angular/common';
+import { Component, input, ChangeDetectionStrategy } from '@angular/core';
@Component({
selector: 'al-roadmap-bottomsheet-header',
+ standalone: true,
imports: [TitleCasePipe, NgClass],
templateUrl: './roadmap-bottomsheet-header.component.html',
- styleUrl: 'roadmap-bottomsheet-header.component.scss',
+ styleUrls: ['roadmap-bottomsheet-header.component.scss'],
+ changeDetection: ChangeDetectionStrategy.OnPush,
})🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.ts
lines 1 to 11, add the property standalone: true to the @Component decorator
since the imports array is present, change styleUrl to styleUrls (plural) and
provide an array for the styles, and set the changeDetection property to
ChangeDetectionStrategy.OnPush to improve performance for this mostly-static UI
component.
| @@ -0,0 +1,14 @@ | |||
| import { Injectable } from '@angular/core'; | |||
There was a problem hiding this comment.
Filename typo breaks public API paths
The file is called roadmap-bottomsheet-menager.service.ts (note the missing “a”).
Consumers will need to import .../roadmap-bottomsheet-menager.service which is easy to miss and contradicts the correctly-spelled class name.
-…/roadmap-bottomsheet-menager.service.ts
+…/roadmap-bottomsheet-manager.service.tsRename the file (and update barrel re-exports) before this typo leaks into released packages.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet-menager.service.ts
at line 1, the filename contains a typo "menager" instead of "manager," causing
incorrect import paths. Rename the file to
roadmap-bottomsheet-manager.service.ts to match the correct spelling and update
all related barrel file exports and import statements accordingly to prevent
breaking the public API.
| private readonly nodeId = new Subject<string>(); | ||
| public bottomSheetOpen$ = this.nodeId.asObservable(); | ||
|
|
||
| openBottomSheet(id: string): void { | ||
| this.nodeId.next(id); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider BehaviorSubject to avoid first-subscriber race
openBottomSheet() pushes an id through a cold Subject.
Late subscribers (e.g. components initialised after the first node click) will never receive the last value.
-private readonly nodeId = new Subject<string>();
+private readonly nodeId = new BehaviorSubject<string | null>(null);A BehaviorSubject (or ReplaySubject(1)) guarantees every consumer gets the most-recent emission without forcing the caller to emit twice.
📝 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 nodeId = new Subject<string>(); | |
| public bottomSheetOpen$ = this.nodeId.asObservable(); | |
| openBottomSheet(id: string): void { | |
| this.nodeId.next(id); | |
| } | |
| private readonly nodeId = new BehaviorSubject<string | null>(null); | |
| public bottomSheetOpen$ = this.nodeId.asObservable(); | |
| openBottomSheet(id: string): void { | |
| this.nodeId.next(id); | |
| } |
🤖 Prompt for AI Agents
In
libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet-menager.service.ts
between lines 8 and 13, replace the Subject<string> used for nodeId with a
BehaviorSubject<string> initialized with a default value or null. This change
ensures that any late subscribers to bottomSheetOpen$ will immediately receive
the most recent emitted id without missing it. Update the declaration and
initialization accordingly and keep the openBottomSheet method unchanged to emit
new values.
| styleUrl: 'roadmap-secondary-node.component.scss', | ||
| changeDetection: ChangeDetectionStrategy.OnPush, |
There was a problem hiding this comment.
styleUrl must be plural
The stylesheet will not be bundled; switch to styleUrls.
- styleUrl: 'roadmap-secondary-node.component.scss',
+ styleUrls: ['roadmap-secondary-node.component.scss'],📝 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.
| styleUrl: 'roadmap-secondary-node.component.scss', | |
| changeDetection: ChangeDetectionStrategy.OnPush, | |
| styleUrls: ['roadmap-secondary-node.component.scss'], | |
| changeDetection: ChangeDetectionStrategy.OnPush, |
🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap-node/src/lib/components/roadmap-secondary-node/roadmap-secondary-node.component.ts
at lines 27 to 28, change the property name from 'styleUrl' to 'styleUrls' to
ensure the stylesheet is correctly bundled. Update the key to 'styleUrls' and
keep the value as an array containing the stylesheet path.
2b72a62 to
04ee5f5
Compare
|
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 (6)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.html (1)
4-4: Remove trailing colon from Tailwind class
The colon in the class key produces an invalid CSS class at runtime.
Apply this diff to fix it:- 'bg-[--secondary-color]:': nodeType() === 'regular', + 'bg-[--secondary-color]': nodeType() === 'regular',libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html (1)
28-36: 🛠️ Refactor suggestionFixed margin breaks L-to-R / R-to-L alignment – reinstate conditional spacing
Earlier feedback suggested swapping the hard-coded
mx-16for direction-aware margins.
The class is still hard-coded, so the strip will be mis-aligned whennodesDrawingDirection()flips.- <div class="mx-16 flex gap-8"> + <div + class="flex gap-8" + [ngClass]="{ + 'ms-[50px]': nodesDrawingDirection() === 'to-right', + 'me-[50px]': nodesDrawingDirection() === 'to-left' + }" + >libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html (2)
2-10:⚠️ Potential issue
@if/ self-closing tags still present – switch to standard*ngIfand explicit closing tagsThis exact issue was raised in earlier reviews but is still unresolved.
Angular (≤ 17) rejects self-closing custom elements and may not recognise@ifwithout the control-flow flag.-@if (layer().parentNode.nodeType === 'primary') { - <al-roadmap-primary-node [node]="layer().parentNode" /> -} @else if (layer().parentNode.nodeType === 'angular-love') { - <al-roadmap-angular-love-node [node]="layer().parentNode" /> -} +<ng-container *ngIf="layer().parentNode.nodeType === 'primary'"> + <al-roadmap-primary-node [node]="layer().parentNode"></al-roadmap-primary-node> +</ng-container> +<ng-container *ngIf="layer().parentNode.nodeType === 'angular-love'"> + <al-roadmap-angular-love-node [node]="layer().parentNode"></al-roadmap-angular-love-node> +</ng-container>🧰 Tools
🪛 HTMLHint (1.5.0)
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
8-10:⚠️ Potential issueSame problem for the layer connector arrow
-@if (showLayerConnector()) { - <al-vertical-connector-arrow class="absolute" /> -} +<al-vertical-connector-arrow + *ngIf="showLayerConnector()" + class="absolute" +></al-vertical-connector-arrow>libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html (1)
30-33:⚠️ Potential issueAdd
rel="noopener noreferrer"to external links opened withtarget="_blank"The anchors still miss the
relattribute, leaving the app open to tab-nabbing.-<a [href]="'author/' + creator.slug" target="_blank"> +<a + [href]="'author/' + creator.slug" + target="_blank" + rel="noopener noreferrer" +>Apply the same change to article and movie links.
Also applies to: 51-54, 64-66
libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (1)
52-53:⚠️ Potential issue
styleUrl→styleUrls(array) – Angular will ignore styles otherwiseThe singular
styleUrlproperty is invalid. Use the plural array form.- styleUrl: './feature-roadmap.component.scss', + styleUrls: ['./feature-roadmap.component.scss'],
🧹 Nitpick comments (3)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-description/roadmap-bottomsheet-description.component.html (1)
1-5: Enhance accessibility of the description sectionWrap the block in a semantic
<section>with anaria-labelledbyattribute and add anidto the heading for better screen-reader support:- <div class="px-6 py-6"> + <section aria-labelledby="description-heading" class="px-6 py-6">- <h4 class="font-semibold">Description</h4> + <h4 id="description-heading" class="font-semibold">Description</h4>🧰 Tools
🪛 HTMLHint (1.5.0)
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html (1)
18-25: Prefer plain<path>over namespacedsvg:pathinside the<svg>Angular adds the correct SVG namespace automatically when an element is placed inside an
<svg>container.
Using thesvg:prefix is unnecessary noise and may confuse some IDE linters / formatters.- <svg:path + <path [attr.d]="connectionsGroup.groupConnectionPath()" [attr.stroke-width]="connectorStrokeWidth" fill="none" stroke="#FDF5FD" stroke-linecap="round" stroke-linejoin="round" />libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts (1)
83-88: Provide error handling for the HTTP resourceA failed fetch of
roadmap-tiles.jsoncurrently crashes the signal.
Add anerrorHandlerto return a fallback array or surface the error gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
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/src/lib/components/connected-nodes/connected-nodes.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-description/roadmap-bottomsheet-description.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-description/roadmap-bottomsheet-description.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.scss(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.ts(1 hunks)libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html(1 hunks)libs/blog/roadmap/utils/src/lib/services/index.ts(1 hunks)libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet-menager.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.scss
- libs/blog/roadmap/utils/src/lib/services/index.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-description/roadmap-bottomsheet-description.component.ts
- libs/blog/roadmap/utils/src/lib/services/roadmap-bottomsheet-menager.service.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.scss
- 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/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-node/src/lib/components/roadmap-angular-love-node/roadmap-angular-love-node.component.ts
- libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.ts
🧰 Additional context used
🪛 HTMLHint (1.5.0)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-description/roadmap-bottomsheet-description.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-header/roadmap-bottomsheet-header.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet.component.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-layer/roadmap-layer.component.html
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (1)
libs/blog/roadmap/ui-roadmap/src/lib/components/connected-nodes/connected-nodes.component.html (1)
1-1: Verify control-flow syntax is enabledThe template uses the v17+
@let/@forsyntax.
Double-check thatcontrolFlow: trueis set intsconfig.angularCompilerOptionsand that the project is on Angular ≥ 17; otherwise the build will fail.🧰 Tools
🪛 HTMLHint (1.5.0)
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
| <div class="h-px flex-grow bg-white"></div> | ||
| </div> | ||
| <div class="pt-4"> | ||
| <p class="text-justify leading-8">{{ description() }}</p> |
There was a problem hiding this comment.
Fix improper function binding in template
Calling description() in the template will cause a runtime error since description is declared as an @Input() description: string, not a function. Change the interpolation to bind the property directly:
- <p class="text-justify leading-8">{{ description() }}</p>
+ <p class="text-justify leading-8">{{ description }}</p>📝 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.
| <p class="text-justify leading-8">{{ description() }}</p> | |
| <p class="text-justify leading-8">{{ description }}</p> |
🤖 Prompt for AI Agents
In
libs/blog/roadmap/ui-roadmap/src/lib/components/roadmap-bottomsheet/roadmap-bottomsheet-description/roadmap-bottomsheet-description.component.html
at line 7, the template incorrectly calls description() as a function, but
description is an @Input() string property. Fix this by removing the parentheses
and binding the description property directly using {{ description }} instead of
{{ description() }}.
| private initPanZoom() { | ||
| const roadmapWrapper = this.roadmapWrapper.nativeElement; | ||
| this.panZoomInstance = panzoom(roadmapWrapper, this._panZoomInitialConfig); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Destroy the PanZoom instance on component teardown
panzoom adds global listeners; forgetting to dispose leaks memory on route changes.
+ ngOnDestroy(): void {
+ this.panZoomInstance?.dispose();
+ }📝 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 initPanZoom() { | |
| const roadmapWrapper = this.roadmapWrapper.nativeElement; | |
| this.panZoomInstance = panzoom(roadmapWrapper, this._panZoomInitialConfig); | |
| } | |
| private initPanZoom() { | |
| const roadmapWrapper = this.roadmapWrapper.nativeElement; | |
| this.panZoomInstance = panzoom(roadmapWrapper, this._panZoomInitialConfig); | |
| } | |
| ngOnDestroy(): void { | |
| this.panZoomInstance?.dispose(); | |
| } |
🤖 Prompt for AI Agents
In libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts around
lines 169 to 173, the panzoom instance created in initPanZoom is not destroyed
on component teardown, causing memory leaks due to global listeners. Implement
the ngOnDestroy lifecycle hook and call the dispose method on the
panZoomInstance to properly clean up event listeners and prevent memory leaks
when the component is destroyed.
| if (event === 'decrement') { | ||
| const currentScale = currentTransform.scale; | ||
| const multiplier = | ||
| Math.round(currentScale * 2) / 2 - this._scaleMultiplier; | ||
|
|
||
| this.panZoomInstance.smoothZoom(centerX, centerY, multiplier); | ||
| } | ||
|
|
||
| if (event === 'increment') { | ||
| const currentScale = currentTransform.scale; | ||
| const multiplier = | ||
| Math.round(currentScale * 2) / 2 + this._scaleMultiplier; | ||
|
|
||
| this.panZoomInstance.smoothZoom(centerX, centerY, multiplier); | ||
| } |
There was a problem hiding this comment.
Zoom calculation passes an absolute scale to smoothZoom instead of a multiplier
panzoom.smoothZoom expects a relative factor (e.g. 0.8 or 1.2), not the new absolute zoom level.
The current formula causes zoom-out to stall at 1× and zoom-in to jump unpredictably.
-const multiplier =
- Math.round(currentScale * 2) / 2 + this._scaleMultiplier;
+const multiplier = 1 + this._scaleMultiplier; // e.g. 1.5 for +0.5 stepDo the analogous fix for the decrement branch (1 - this._scaleMultiplier).
📝 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.
| if (event === 'decrement') { | |
| const currentScale = currentTransform.scale; | |
| const multiplier = | |
| Math.round(currentScale * 2) / 2 - this._scaleMultiplier; | |
| this.panZoomInstance.smoothZoom(centerX, centerY, multiplier); | |
| } | |
| if (event === 'increment') { | |
| const currentScale = currentTransform.scale; | |
| const multiplier = | |
| Math.round(currentScale * 2) / 2 + this._scaleMultiplier; | |
| this.panZoomInstance.smoothZoom(centerX, centerY, multiplier); | |
| } | |
| if (event === 'decrement') { | |
| const currentScale = currentTransform.scale; | |
| - const multiplier = | |
| - Math.round(currentScale * 2) / 2 - this._scaleMultiplier; | |
| + const multiplier = 1 - this._scaleMultiplier; | |
| this.panZoomInstance.smoothZoom(centerX, centerY, multiplier); | |
| } | |
| if (event === 'increment') { | |
| const currentScale = currentTransform.scale; | |
| - const multiplier = | |
| - Math.round(currentScale * 2) / 2 + this._scaleMultiplier; | |
| + const multiplier = 1 + this._scaleMultiplier; // e.g. 1.5 for a +0.5 step | |
| this.panZoomInstance.smoothZoom(centerX, centerY, multiplier); | |
| } |
🤖 Prompt for AI Agents
In libs/blog/roadmap/feature-roadmap/src/lib/feature-roadmap.component.ts lines
126 to 140, the zoom calculation incorrectly passes an absolute scale value to
panZoomInstance.smoothZoom instead of a relative multiplier. To fix this, change
the multiplier calculation to compute a relative zoom factor by multiplying 1 by
(1 ± this._scaleMultiplier) for increment and decrement respectively, rather
than using the rounded absolute scale. This means for increment use 1 +
this._scaleMultiplier and for decrement use 1 - this._scaleMultiplier as the
multiplier argument to smoothZoom.
Summary by CodeRabbit