Repository navigation
Added component - #112
Added component#112
Conversation
WalkthroughAdds an admin CRUD UI for salaries historical-data templates, wires routing and service, refactors historical charts to template-based data, removes grade-based chart components, converts profession inputs to array-based ng-selects, and applies null-safety fixes in company views. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant AdminUI as Admin UI (HistoricalDataTemplatesComponent)
participant EditForm as HistoricalDataTemplateEditForm
participant TemplatesSvc as SalariesHistoricalDataTemplatesService
participant Api as ApiService / HTTP
participant Backend as Backend API
AdminUI->>TemplatesSvc: search(page)
TemplatesSvc->>Api: GET /api/salaries-historical-data-templates?page=...
Api->>Backend: HTTP request
Backend-->>Api: 200 paginated templates
Api-->>TemplatesSvc: response
TemplatesSvc-->>AdminUI: PaginatedList<Template>
AdminUI->>EditForm: open(editItem?)
EditForm-->>AdminUI: form values
AdminUI->>TemplatesSvc: create(body) / update(id, body)
TemplatesSvc->>Api: POST /api/salaries-historical-data-templates (or /{id})
Api->>Backend: HTTP request
Backend-->>Api: 201/200 created/updated
Api-->>TemplatesSvc: template dto
TemplatesSvc-->>AdminUI: created/updated dto
AdminUI->>AdminUI: reload current page (search)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (21)
src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart.component.scss (1)
1-10: Good refactor to class-based selector.The change from ID selectors to
.canvas-containeris a best practice improvement. However, consider these optional refinements:
Redundant min-height: The
canvasinherits sizing from its container, so settingmin-height: 600pxon both the container (line 5) and the nested canvas (line 8) is redundant. Consider removing it from the canvas.Responsive design: The fixed
600pxmin-height may not work well on smaller screens. Consider using a responsive unit or media queries for mobile devices.CSS variable for theming: Extract the magic number
600pxto a CSS variable (e.g.,--chart-min-height) for easier maintenance and theming consistency.🔎 Optional refactor
.canvas-container { position: relative; width: 100%; height: 100%; min-height: 600px; canvas { - min-height: 600px; + width: 100%; + height: 100%; } }Or with CSS variables:
// In your theme or global styles :root { --chart-min-height: 600px; } // In component .canvas-container { position: relative; width: 100%; height: 100%; min-height: var(--chart-min-height); canvas { width: 100%; height: 100%; } }src/app/modules/admin/components/companies/companies-admin-page/companies-admin-page.component.html (1)
67-67: Consider adding a trackBy function for the ngFor loop.Without a trackBy function, Angular re-renders all list items when the
companiesarray changes, which can impact performance with large datasets in admin tables.🔎 Example trackBy implementation
In the component TypeScript file, add:
trackByCompanyId(index: number, item: any): number { return item.id; }Then update the template:
-<tr *ngFor="let item of companies; let i = index" class="small"> +<tr *ngFor="let item of companies; let i = index; trackBy: trackByCompanyId" class="small">src/app/modules/admin/components/companies/company-admin-page/company-admin-page.component.html (1)
159-159: Consider adding a trackBy function for the reviews ngFor loop.Without a trackBy function, Angular re-renders all review items when the
company.reviewsarray changes, which can degrade performance when the reviews list is updated.🔎 Example trackBy implementation
In the component TypeScript file, add:
trackByReviewId(index: number, item: any): number { return item.id; }Then update the template:
-<tr *ngFor="let item of company.reviews; let i = index"> +<tr *ngFor="let item of company.reviews; let i = index; trackBy: trackByReviewId">src/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.scss (1)
7-16: Consider alternatives to deprecated ::ng-deep.The
::ng-deeppseudo-element is deprecated in Angular. While it's commonly used for styling third-party components like ng-select, consider these alternatives:
- Use
:host ::ng-deep(current approach) with a comment noting the necessity- Apply
ViewEncapsulation.Noneif the entire component needs global styles- Use global styles in
styles.scssfor application-wide dropdown behaviorAlso, forcing
overflow: visibleon modal containers can cause layout issues if modal content exceeds viewport bounds. Test with long profession lists to ensure proper scrolling behavior.As per coding guidelines, proper use of
:hostand::ng-deepshould be reviewed with alternatives suggested.src/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.html (2)
32-40: Simplify complex inline pluralization logic.The inline expression for Russian pluralization is complex and hard to maintain:
item.professionIds.length === 1 ? 'ь' : (item.professionIds.length < 5 ? 'и' : 'ей')Consider extracting this to a component method or pipe for better readability and reusability.
🔎 Proposed refactor
Add to component:
getProfessionsCountLabel(count: number): string { if (count === 1) return `${count} специальность`; if (count < 5) return `${count} специальности`; return `${count} специальностей`; }Then in template:
-{{ item.professionIds.length }} специальност{{ item.professionIds.length === 1 ? 'ь' : (item.professionIds.length < 5 ? 'и' : 'ей') }} +{{ getProfessionsCountLabel(item.professionIds.length) }}As per coding guidelines, avoid complex expressions in templates.
172-186: Enhance modal accessibility.The professions modal is missing accessibility attributes. Consider adding:
role="dialog"andaria-modal="true"on the dialog containeraria-labelledbyreferencing the header- Focus management to trap focus within the modal when open
As per coding guidelines, templates should include proper ARIA attributes and accessibility features.
src/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.html (2)
30-30: Extract ID truncation logic to a pipe.The inline ID truncation logic
...{{ item.id.split('-').pop() }}is repeated and hard to maintain. Consider creating a reusable pipe:🔎 Proposed implementation
// truncate-uuid.pipe.ts @Pipe({ name: 'truncateUuid', standalone: true }) export class TruncateUuidPipe implements PipeTransform { transform(uuid: string): string { return '...' + uuid.split('-').pop(); } }Then in template:
-<span class="text-muted small" [title]="item.id">...{{ item.id.split('-').pop() }}</span> +<span class="text-muted small" [title]="item.id">{{ item.id | truncateUuid }}</span>
34-42: Consider extracting shared profession modal to a reusable component.The profession display modal pattern (lines 34-42 for the link, 149-163 for the modal) is duplicated in
job-posting-message-subscriptions.component.html. This violates DRY principles.Consider creating a shared component like
<app-professions-modal>that accepts professions as input and emits close events.This would improve maintainability and ensure consistent behavior across admin components.
Also applies to: 149-163
src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-charts-page.component.ts (2)
16-16: Make canvasId readonly for immutability.The
canvasIdis generated once and never reassigned, but it's not marked asreadonly. Consider:-readonly canvasId = "canvas_" + Math.random().toString(36); +private readonly canvasId = "canvas_" + Math.random().toString(36);Note: The field should be
publicif referenced in the template. Verify template usage.
33-36: Simplify redundant null checks.Line 34 checks
chartEl != nullbut line 35 uses optional chainingchartEl?.parentElement, which already handles null. Also,chartEl?.parentElement.style.heightshould use optional chaining consistently:🔎 Proposed refactor
-const chartEl = document.getElementById(this.canvasId); -if (chartEl != null && chartEl.parentElement != null) { - chartEl.style.height = chartEl?.parentElement.style.height ?? "100%"; -} +const chartEl = document.getElementById(this.canvasId); +if (chartEl?.parentElement) { + chartEl.style.height = chartEl.parentElement.style.height || "100%"; +}src/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.ts (1)
162-173: Optimize getProfessionNames for large datasets.The method calls
this.professions.find()inside amap(), resulting in O(n*m) complexity. For large profession arrays, consider creating a lookup map:🔎 Proposed optimization
private professionsMap: Map<number, LabelEntityDto> = new Map(); loadProfessions(): void { // ... existing code ... .subscribe((x) => { this.professions = x; this.professionsMap = new Map(x.map(p => [p.id, p])); this.professionsAsOptions = x.map((p) => ({ value: p.id.toString(), item: p.id, label: p.title, })); }); } getProfessionNames(professionIds: number[] | null): string { if (!professionIds || professionIds.length === 0) { return "-"; } return professionIds .map((id) => this.professionsMap.get(id)?.title ?? id.toString()) .join(", "); }src/app/modules/salaries/components/historical-charts-page/historical-charts-page.component.ts (1)
50-55: Consider adding error handling for chart loading.The
salariesChart()observable subscription has no error handler. If the service call fails, users won't receive feedback. Consider adding error handling:🔎 Proposed enhancement
this.service .salariesChart() .pipe(untilDestroyed(this)) .subscribe({ - (x) => { + next: (x) => { this.data = x; - } + }, + error: (err) => { + console.error('Failed to load historical charts:', err); + // Optionally show user-facing error message + } });src/app/modules/admin/components/telegram/salaries-stat-subscriptions/stat-data-cache-records.component.scss (2)
15-24: Replace deprecated::ng-deepwith modern alternatives.The
::ng-deeppseudo-class is deprecated in Angular. Consider these alternatives:
- Use
ViewEncapsulation.Noneon the component if global styling is needed- Use host context selectors:
:host ::ng-deep→:host-context(.modal) .modal-body- Apply styles directly in the parent component that owns the modal
- Use CSS custom properties (CSS variables) for theming
As per coding guidelines, avoid
::ng-deepin favor of supported alternatives.🔎 Alternative approach using ViewEncapsulation
In the component TypeScript file:
+import { Component, ViewEncapsulation } from '@angular/core'; + @Component({ selector: '...', templateUrl: '...', styleUrls: ['...'], + encapsulation: ViewEncapsulation.None, standalone: false, })Then remove
::ng-deepfrom the SCSS:-:host ::ng-deep { - .modal-body { +.modal-body { overflow: visible; - } +} - .modal-content { +.modal-content { overflow: visible; - } }
27-34: Settingoverflow: visiblemay break responsive table scrolling.Changing
.table-responsiveoverflow tovisibledefeats its purpose of providing horizontal scroll for wide tables on small screens. Consider:
- Using
overflow-x: autowithoverflow-y: visibleif the browser supports it- Positioning dropdowns with JavaScript/Popper.js to avoid clipping
- Using Bootstrap's dropdown with
data-bs-boundary="viewport"attributesrc/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.scss (2)
3-12: Replace deprecated::ng-deepwith modern alternatives.The
::ng-deeppseudo-class is deprecated in Angular. Consider usingViewEncapsulation.Noneon the component or host context selectors as alternatives. See the suggested approach in the stat-data-cache-records.component.scss review comments.As per coding guidelines, avoid
::ng-deepin favor of supported alternatives.
14-24: Comment is misleading—code usesoverflow: visible, notoverflow: clip.The comment on lines 15-16 mentions "Using overflow: clip on x-axis preserves horizontal scroll behavior," but the actual implementation uses
overflow: visible, which removes all scrolling. Either:
- Update the comment to match the implementation, or
- Implement the described behavior with
overflow-x: auto; overflow-y: visible(if browser support allows)Additionally, setting
overflow: visibleon.table-responsivedefeats its purpose of providing horizontal scroll on small screens.src/app/modules/admin/components/telegram/salaries-stat-subscriptions/stat-data-cache-records.component.html (1)
270-276: Consider adding trackBy function for the professions list.The
*ngForloop would benefit from atrackByfunction to optimize rendering when the profession list changes.As per coding guidelines, trackBy functions should be used for ngFor loops.
🔎 Add trackBy function
In the component TypeScript file:
trackByProfessionId(index: number, profession: { id: number; title: string }): number { return profession.id; }Then update the template:
-<li *ngFor="let profession of selectedProfessionsForModal" +<li *ngFor="let profession of selectedProfessionsForModal; trackBy: trackByProfessionId"src/app/modules/salaries/components/historical-charts-page/historical-charts-page.component.html (2)
11-11: *Add trackBy function to ngFor for better performance.The *ngFor loop should include a trackBy function to help Angular efficiently track and update DOM elements when the templates array changes. This prevents unnecessary re-rendering of unchanged items.
🔎 Suggested implementation
In the component TypeScript file, add a trackBy function:
trackByTemplateId(index: number, template: HistoricalDataByTemplate): string { return template.templateId; }Then update the template:
- <div class="card card-body mobile-px-5 mobile-py-5 mb-4" *ngFor="let template of data.templates"> + <div class="card card-body mobile-px-5 mobile-py-5 mb-4" *ngFor="let template of data.templates; trackBy: trackByTemplateId">
4-8: Consider adding i18n markers for user-facing text.The template contains Russian text that should be marked for internationalization to support potential future localization efforts.
Also applies to: 10-14, 22-25
src/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.ts (2)
53-69: Consider extracting duplicated profession management logic.The
loadProfessions,getProfessionNames,openProfessionsModal, andcloseProfessionsModalmethods are identical to those instat-data-cache-records.component.ts. Consider extracting this logic into:
- A shared base class
- A reusable service or helper class
- A mixin or decorator pattern
This would improve maintainability and reduce the risk of inconsistencies when changes are needed.
Also applies to: 152-177
18-22: Consider using OnPush change detection strategy.The component doesn't specify a change detection strategy. Since the component relies on observable-based data loading and manual state updates, using
ChangeDetectionStrategy.OnPushcould improve performance.🔎 Suggested implementation
+import { Component, OnDestroy, OnInit, ChangeDetectionStrategy } from "@angular/core"; @Component({ templateUrl: "./historical-data-templates.component.html", styleUrls: ["./historical-data-templates.component.scss"], + changeDetection: ChangeDetectionStrategy.OnPush, standalone: false, })Note: If you implement this, you may need to inject
ChangeDetectorRefand manually trigger detection after async operations if you're not using the async pipe everywhere.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/app/components/admin-navbar/admin-navbar.component.tssrc/app/modules/admin/admin-routing.module.tssrc/app/modules/admin/admin.module.tssrc/app/modules/admin/components/companies/companies-admin-page/companies-admin-page.component.htmlsrc/app/modules/admin/components/companies/company-admin-page/company-admin-page.component.htmlsrc/app/modules/admin/components/salaries/historical-data-templates/historical-data-template-edit-form.tssrc/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.htmlsrc/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.scsssrc/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.tssrc/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.htmlsrc/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.scsssrc/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.tssrc/app/modules/admin/components/telegram/job-posting-message-subscriptions/subscription-edit-form.tssrc/app/modules/admin/components/telegram/salaries-stat-subscriptions/stat-data-cache-records.component.htmlsrc/app/modules/admin/components/telegram/salaries-stat-subscriptions/stat-data-cache-records.component.scsssrc/app/modules/admin/components/telegram/salaries-stat-subscriptions/stat-data-cache-records.component.tssrc/app/modules/admin/components/telegram/salaries-stat-subscriptions/subscription-edit-form.tssrc/app/modules/salaries/components/historical-charts-page/historical-charts-page.component.htmlsrc/app/modules/salaries/components/historical-charts-page/historical-charts-page.component.tssrc/app/modules/salaries/components/historical-charts-page/historical-salaries-by-grade-chart/historical-salaries-by-grade-chart-object.tssrc/app/modules/salaries/components/historical-charts-page/historical-salaries-by-grade-chart/historical-salaries-by-grade-chart.component.htmlsrc/app/modules/salaries/components/historical-charts-page/historical-salaries-by-grade-chart/historical-salaries-by-grade-chart.component.scsssrc/app/modules/salaries/components/historical-charts-page/historical-salaries-by-grade-chart/historical-salaries-by-grade-chart.component.spec.tssrc/app/modules/salaries/components/historical-charts-page/historical-salaries-by-grade-chart/historical-salaries-by-grade-chart.component.tssrc/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-charts-page.component.tssrc/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart-object.tssrc/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart.component.htmlsrc/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart.component.scsssrc/app/modules/salaries/salaries.module.tssrc/app/services/historical-charts.service.tssrc/app/services/salaries-historical-data-templates.service.ts
💤 Files with no reviewable changes (6)
- src/app/modules/salaries/components/historical-charts-page/historical-salaries-by-grade-chart/historical-salaries-by-grade-chart.component.html
- src/app/modules/salaries/components/historical-charts-page/historical-salaries-by-grade-chart/historical-salaries-by-grade-chart.component.scss
- src/app/modules/salaries/salaries.module.ts
- src/app/modules/salaries/components/historical-charts-page/historical-salaries-by-grade-chart/historical-salaries-by-grade-chart.component.ts
- src/app/modules/salaries/components/historical-charts-page/historical-salaries-by-grade-chart/historical-salaries-by-grade-chart.component.spec.ts
- src/app/modules/salaries/components/historical-charts-page/historical-salaries-by-grade-chart/historical-salaries-by-grade-chart-object.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: Review TypeScript code for:
- Proper typing and avoiding 'any' type
- Consistent use of interfaces and type aliases
- Null safety with strict null checks
- Proper error handling
- Code organization and single responsibility
Files:
src/app/components/admin-navbar/admin-navbar.component.tssrc/app/modules/admin/admin-routing.module.tssrc/app/modules/admin/admin.module.tssrc/app/modules/admin/components/telegram/salaries-stat-subscriptions/stat-data-cache-records.component.tssrc/app/modules/admin/components/telegram/job-posting-message-subscriptions/subscription-edit-form.tssrc/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart-object.tssrc/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-charts-page.component.tssrc/app/modules/admin/components/salaries/historical-data-templates/historical-data-template-edit-form.tssrc/app/services/salaries-historical-data-templates.service.tssrc/app/services/historical-charts.service.tssrc/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.tssrc/app/modules/salaries/components/historical-charts-page/historical-charts-page.component.tssrc/app/modules/admin/components/telegram/salaries-stat-subscriptions/subscription-edit-form.tssrc/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.ts
**/*.component.ts
⚙️ CodeRabbit configuration file
**/*.component.ts: Review Angular components for:
- Proper use of lifecycle hooks (ngOnInit, ngOnDestroy, etc.)
- OnPush change detection strategy where applicable
- Proper subscription management (unsubscribe, takeUntilDestroyed, async pipe)
- Input/Output decorator usage and proper typing
- Avoiding logic in templates
- Component size and potential need for splitting
Files:
src/app/components/admin-navbar/admin-navbar.component.tssrc/app/modules/admin/components/telegram/salaries-stat-subscriptions/stat-data-cache-records.component.tssrc/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-charts-page.component.tssrc/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.tssrc/app/modules/salaries/components/historical-charts-page/historical-charts-page.component.tssrc/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.ts
**/*.component.html
⚙️ CodeRabbit configuration file
**/*.component.html: Review Angular templates for:
Files:
src/app/modules/admin/components/companies/companies-admin-page/companies-admin-page.component.htmlsrc/app/modules/admin/components/companies/company-admin-page/company-admin-page.component.htmlsrc/app/modules/admin/components/telegram/salaries-stat-subscriptions/stat-data-cache-records.component.htmlsrc/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.htmlsrc/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart.component.htmlsrc/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.htmlsrc/app/modules/salaries/components/historical-charts-page/historical-charts-page.component.html
**/*.component.scss
⚙️ CodeRabbit configuration file
**/*.component.scss: Review component styles for:
- Proper use of :host and ::ng-deep (deprecated, suggest alternatives)
- CSS variable usage for theming
- Responsive design patterns
- BEM or consistent naming convention
Files:
src/app/modules/admin/components/telegram/salaries-stat-subscriptions/stat-data-cache-records.component.scsssrc/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.scsssrc/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.scsssrc/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart.component.scss
**/*.service.ts
⚙️ CodeRabbit configuration file
**/*.service.ts: Review Angular services for:
- Proper dependency injection with providedIn
- RxJS best practices (operators, error handling, memory leaks)
- HTTP error handling and retry strategies
- Caching strategies where appropriate
- Single responsibility principle
Files:
src/app/services/salaries-historical-data-templates.service.tssrc/app/services/historical-charts.service.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: maximgorbatyuk
Repo: Techinterview-space/frontend PR: 74
File: src/app/modules/companies/components/add-review-page/add-review-page.component.spec.ts:31-33
Timestamp: 2025-04-30T04:43:39.004Z
Learning: maximgorbatyuk is prioritizing MVP functionality for the companies feature and may address expanded test coverage (form validation, submission, error handling, rating selection) in a later phase.
🧬 Code graph analysis (6)
src/app/modules/admin/components/telegram/salaries-stat-subscriptions/stat-data-cache-records.component.ts (2)
src/app/services/label-entity.model.ts (1)
LabelEntityDto(9-9)src/app/models/extended/application-user-extended.ts (1)
id(9-11)
src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart-object.ts (2)
src/app/services/historical-charts.service.ts (1)
HistoricalDataByTemplate(26-31)src/app/modules/salaries/components/random-rgb-color.ts (1)
RandomRgbColor(3-15)
src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-charts-page.component.ts (2)
src/app/services/historical-charts.service.ts (1)
HistoricalDataByTemplate(26-31)src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart-object.ts (1)
HistoricalSalariesChartObject(15-122)
src/app/services/salaries-historical-data-templates.service.ts (3)
src/app/models/page-params.ts (2)
PageParams(8-8)defaultPageParams(10-13)src/app/models/paginated-list.ts (1)
PaginatedList(17-19)src/app/shared/value-objects/convert-object-to-http.ts (1)
ConvertObjectToHttpParams(4-35)
src/app/services/historical-charts.service.ts (2)
src/app/shared/value-objects/convert-object-to-http.ts (1)
ConvertObjectToHttpParams(4-35)src/app/services/historical-charts.models.ts (1)
HistoricalSurveyChartResponse(1-10)
src/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.ts (1)
src/app/services/label-entity.model.ts (1)
LabelEntityDto(9-9)
🔇 Additional comments (22)
src/app/modules/admin/components/companies/companies-admin-page/companies-admin-page.component.html (1)
77-77: Null safety improvement looks good.The optional chaining prevents potential null reference errors when
reviewsis undefined or null, providing a safe fallback toreviewsCount.src/app/modules/admin/components/companies/company-admin-page/company-admin-page.component.html (1)
90-90: Null safety improvements look good.Both changes appropriately use optional chaining to prevent null reference errors:
- Line 90 safely accesses the length with a fallback to
reviewsCount- Line 146 provides null safety while maintaining the same functional behavior (empty arrays and null/undefined both resolve to falsy)
Also applies to: 146-146
src/app/modules/admin/components/telegram/job-posting-message-subscriptions/subscription-edit-form.ts (2)
19-19: Consider adding validation for professionIds.The
professionIdscontrol has no validators. While this makes the field optional, consider whether business logic requires:
- At least one profession selected (Validators.required or custom minLength validator)
- Maximum number of professions (e.g., Validators.maxLength for array)
If professions are truly optional, the current implementation is fine, but verify this aligns with requirements.
38-44: LGTM! Clean type-safe implementation.The refactor from comma-separated strings to
Array<number>improves type safety and eliminates error-prone string parsing. The fallback to empty array[](line 43) properly handles null/undefined cases.src/app/modules/admin/components/telegram/salaries-stat-subscriptions/subscription-edit-form.ts (1)
19-19: LGTM! Consistent implementation with job-posting subscriptions.This form mirrors the refactor in
job-posting-message-subscriptions/subscription-edit-form.ts, converting professionIds from string toArray<number>. The implementation is consistent and type-safe.Same consideration as the other form: verify whether professionIds should be optional or require validation (min/max selections).
Also applies to: 45-52
src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-charts-page.component.ts (1)
11-11: LGTM! Proper lifecycle hook usage.The switch from
OnInittoAfterViewInitis correct for chart initialization that depends on DOM availability. The addition ofngOnDestroyensures proper cleanup of the Chart.js instance, preventing memory leaks.As per coding guidelines, proper use of lifecycle hooks is essential.
Also applies to: 18-24
src/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.ts (2)
59-75: LGTM! Well-implemented caching pattern.The
loadProfessionsmethod properly:
- Caches data to avoid redundant API calls (lines 60-62)
- Uses
untilDestroyedfor subscription cleanup- Maps professions to select options for ng-select
152-155: LGTM! Improved post-creation refresh.Changing from
this.ngOnInit()tothis.loadData(this.currentPage)is more targeted and efficient, avoiding unnecessary re-initialization of professions data.src/app/modules/salaries/components/historical-charts-page/historical-charts-page.component.ts (1)
21-28: Verify intentional removal of query parameter filtering.The component has been significantly simplified by removing:
ActivatedRoutedependency and query param subscriptionSalariesServiceand global filters functionality- Parameterized loading with filters
This aligns with the PR's shift to template-based historical data. However, verify that:
- Users don't need URL-based filtering for historical charts anymore
- The new template-based approach covers all previous filter use cases
- This doesn't break existing bookmarked URLs with query parameters
The simplification improves maintainability, but ensure it's aligned with product requirements.
Also applies to: 32-56
src/app/components/admin-navbar/admin-navbar.component.ts (1)
74-78: LGTM! Navigation link properly integrated.The new navigation link follows the existing pattern and correctly points to the newly added route for historical data templates management.
src/app/modules/admin/admin-routing.module.ts (1)
26-26: LGTM! Route properly configured.The new route follows the existing routing conventions and correctly maps to the newly introduced
HistoricalDataTemplatesComponent.Also applies to: 77-80
src/app/modules/admin/admin.module.ts (1)
7-7: LGTM! Module properly configured.The new component and service are correctly declared and provided in the AdminModule, and NgSelectModule is properly imported to support the profession selection UI across admin components.
Also applies to: 40-41, 50-50, 81-81, 89-89
src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart.component.html (1)
1-3: LGTM! Good use of optional chaining for null safety.The dynamic header with
template?.nameproperly handles the null case and follows the template-based refactoring described in the AI summary.src/app/modules/admin/components/telegram/salaries-stat-subscriptions/stat-data-cache-records.component.html (2)
36-46: LGTM! Good UX for displaying profession counts.The profession count link with proper Russian pluralization and modal trigger provides a clean interface for viewing associated professions. The use of
$event.preventDefault()is appropriate.
183-198: LGTM! Proper ng-select integration for multi-select.The ng-select component is properly configured with:
- Multiple selection enabled
- Clear selection option
- Tab selection support
- Proper form control binding
This aligns with the PR's goal of improving profession selection UI.
src/app/modules/admin/components/telegram/salaries-stat-subscriptions/stat-data-cache-records.component.ts (3)
57-73: LGTM! Well-implemented profession loading with caching.The profession loading logic is well-structured with:
- Proper caching guard to prevent redundant API calls
- Correct subscription management using
untilDestroyed- Safe transformation of professions into select-box format
215-226: LGTM! Proper null safety and fallback handling.The
getProfessionNamesmethod handles edge cases well with proper null checks and provides a sensible fallback to display the ID when a profession is not found.
154-154: Good refactor to reload data instead of reinitializing the component.Changing from
this.ngOnInit()tothis.loadData(this.currentPage)is a better approach that preserves the current page state without resetting the entire component.Also applies to: 166-166
src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart-object.ts (2)
18-19: LGTM! Safe data extraction with nullish coalescing.The constructor properly extracts dataPoints from the template with a sensible default to an empty array, ensuring the code handles null/undefined gracefully.
48-78: LGTM! Well-implemented grade-based dataset logic.The implementation correctly:
- Checks for the presence of grade data before iterating
- Skips the "Undefined" grade
- Validates that data exists before adding datasets (line 65)
- Uses proper TypeScript typing with keyof pattern
src/app/services/salaries-historical-data-templates.service.ts (1)
51-59: Verify that POST is the intended HTTP method for updates.The
updatemethod usesPOSTinstead of the more conventionalPUTorPATCH. While this may be intentional based on your API design, it's worth confirming this is correct.src/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.ts (1)
45-51: LGTM! Proper initialization and data loading pattern.The component correctly:
- Initializes state to null during
ngOnInit- Loads professions with caching
- Manages pagination state
- Uses
untilDestroyedfor proper subscription cleanupAlso applies to: 71-86
| professionIds: new FormControl(item?.professionIds ?? [], [ | ||
| Validators.required, | ||
| ]), |
There was a problem hiding this comment.
Validators.required doesn't validate array length.
The Validators.required validator only checks for null/undefined, so an empty array [] would pass validation. For array fields that must contain at least one item, use Validators.minLength(1) or a custom validator.
🔎 Fix array validation
professionIds: new FormControl(item?.professionIds ?? [], [
Validators.required,
+ Validators.minLength(1),
]),Or create a custom validator:
function requiredArray(control: AbstractControl): ValidationErrors | null {
const value = control.value;
return Array.isArray(value) && value.length > 0 ? null : { requiredArray: true };
} professionIds: new FormControl(item?.professionIds ?? [], [
- Validators.required,
+ requiredArray,
]),📝 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.
| professionIds: new FormControl(item?.professionIds ?? [], [ | |
| Validators.required, | |
| ]), | |
| professionIds: new FormControl(item?.professionIds ?? [], [ | |
| Validators.required, | |
| Validators.minLength(1), | |
| ]), |
🤖 Prompt for AI Agents
In
src/app/modules/admin/components/salaries/historical-data-templates/historical-data-template-edit-form.ts
around lines 16 to 18, the FormControl for professionIds uses
Validators.required which does not fail for an empty array; change the validator
to enforce non-empty arrays by replacing Validators.required with
Validators.minLength(1) or attach a custom validator that returns an error when
the control value is not an array with length > 0 (e.g., requiredArray), and
update any related error keys/messages in the template or form handling to
reflect the new validator.
| createRequestOrNull(): CreateSalariesHistoricalDataRecordTemplateRequest | null { | ||
| if (!this.valid) { | ||
| this.markAllAsTouched(); | ||
| return null; | ||
| } | ||
|
|
||
| const professionIds = this.value.professionIds as Array<number>; | ||
|
|
||
| return { | ||
| name: this.value.name, | ||
| professionIds: professionIds ?? [], | ||
| }; | ||
| } |
There was a problem hiding this comment.
Type safety concern and redundant null coalescing.
Line 36 uses a type assertion as Array<number> without checking if the value is actually an array. If the form control value is null or undefined, this could cause runtime issues.
Line 40's null coalescing professionIds ?? [] is redundant since professionIds is already assigned on line 36.
🔎 Improve type safety
createRequestOrNull(): CreateSalariesHistoricalDataRecordTemplateRequest | null {
if (!this.valid) {
this.markAllAsTouched();
return null;
}
- const professionIds = this.value.professionIds as Array<number>;
+ const professionIds = (this.value.professionIds as Array<number> | null) ?? [];
return {
name: this.value.name,
- professionIds: professionIds ?? [],
+ professionIds: professionIds,
};
}📝 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.
| createRequestOrNull(): CreateSalariesHistoricalDataRecordTemplateRequest | null { | |
| if (!this.valid) { | |
| this.markAllAsTouched(); | |
| return null; | |
| } | |
| const professionIds = this.value.professionIds as Array<number>; | |
| return { | |
| name: this.value.name, | |
| professionIds: professionIds ?? [], | |
| }; | |
| } | |
| createRequestOrNull(): CreateSalariesHistoricalDataRecordTemplateRequest | null { | |
| if (!this.valid) { | |
| this.markAllAsTouched(); | |
| return null; | |
| } | |
| const professionIds = (this.value.professionIds as Array<number> | null) ?? []; | |
| return { | |
| name: this.value.name, | |
| professionIds: professionIds, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In
src/app/modules/admin/components/salaries/historical-data-templates/historical-data-template-edit-form.ts
around lines 30 to 42, the code unsafely asserts this.value.professionIds as
Array<number> and then redundantly uses professionIds ?? []; instead, safely
read the control value (e.g. const val = this.value.professionIds), validate its
type (Array.isArray(val)), and assign professionIds = Array.isArray(val) ? val
as Array<number> : []; then return the request using that safe professionIds
value (remove the redundant null-coalescing). Ensure you still call
markAllAsTouched() when invalid and keep the same return shape.
| <tr class="small" *ngFor="let item of items; let i = index"> | ||
| <td> | ||
| <span class="text-muted small" [title]="item.id">...{{ item.id.split('-').pop() }}</span> | ||
| </td> | ||
| <td>{{ item.name }}</td> | ||
| <td> | ||
| <a | ||
| *ngIf="item.professionIds && item.professionIds.length > 0" | ||
| href="#" | ||
| class="text-primary" | ||
| (click)="openProfessionsModal(item.professionIds); $event.preventDefault()" | ||
| > | ||
| {{ item.professionIds.length }} специальност{{ item.professionIds.length === 1 ? 'ь' : (item.professionIds.length < 5 ? 'и' : 'ей') }} | ||
| </a> | ||
| <span *ngIf="!item.professionIds || item.professionIds.length === 0">-</span> | ||
| </td> | ||
| <td>{{ item.createdAt | date: "yyyy-MM-dd HH:mm" }}</td> | ||
| <td>{{ item.updatedAt | date: "yyyy-MM-dd HH:mm" }}</td> | ||
| <td> | ||
| <div class="btn-group" role="group"> | ||
| <button | ||
| type="button" | ||
| class="btn btn-outline-dark dropdown-toggle btn-sm" | ||
| data-bs-toggle="dropdown" | ||
| aria-expanded="false" | ||
| > | ||
| Действия | ||
| </button> | ||
| <ul class="dropdown-menu"> | ||
| <li> | ||
| <a class="dropdown-item" (click)="openEditDlg(item)" | ||
| >Редактировать</a | ||
| > | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="dropdown-item text-danger" | ||
| href="#" | ||
| (click)="delete(item); $event.preventDefault()" | ||
| >Удалить</a | ||
| > | ||
| </li> | ||
| </ul> | ||
| </div> | ||
| </td> | ||
| </tr> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add trackBy functions for performance.
Multiple *ngFor loops lack trackBy functions:
- Line 28: items loop
- Line 157: selectedProfessionsForModal loop
This can cause unnecessary re-renders, especially after edit/delete operations.
🔎 Proposed fix
Add to component:
trackById(index: number, item: { id: string }): string {
return item.id;
}
trackByProfessionId(index: number, profession: LabelEntityDto): number {
return profession.id;
}Update template:
-<tr class="small" *ngFor="let item of items; let i = index">
+<tr class="small" *ngFor="let item of items; trackBy: trackById">
-*ngFor="let profession of selectedProfessionsForModal"
+*ngFor="let profession of selectedProfessionsForModal; trackBy: trackByProfessionId"As per coding guidelines, trackBy functions are required for ngFor loops.
Also applies to: 157-161
🤖 Prompt for AI Agents
In
src/app/modules/admin/components/salaries/historical-data-templates/historical-data-templates.component.html
around lines 28 and 157-161, the *ngFor loops are missing trackBy functions
which causes unnecessary DOM re-renders after edits/deletes; add two methods to
the component class — e.g. trackById(index: number, item: { id: string }) {
return item.id; } and trackByProfessionId(index: number, profession: { id:
number }) { return profession.id; } — then update the template *ngFor directives
to use trackBy: *ngFor="let item of items; let i = index; trackBy: trackById"
and *ngFor="let profession of selectedProfessionsForModal; trackBy:
trackByProfessionId".
| @@ -29,9 +29,15 @@ | |||
| <tr class="small" *ngFor="let item of items; let i = index"> | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add trackBy functions for ngFor loops.
Both *ngFor loops (lines 29 and 180) are missing trackBy functions, which can cause performance issues during re-renders, especially when data is refreshed after CRUD operations.
🔎 Proposed fix
Add to component:
trackByItemId(index: number, item: JobPostingMessageSubscription): string {
return item.id;
}
trackByProfessionId(index: number, profession: LabelEntityDto): number {
return profession.id;
}Update template:
-<tr class="small" *ngFor="let item of items; let i = index">
+<tr class="small" *ngFor="let item of items; trackBy: trackByItemId">
-*ngFor="let profession of selectedProfessionsForModal"
+*ngFor="let profession of selectedProfessionsForModal; trackBy: trackByProfessionId"As per coding guidelines, trackBy functions are required for ngFor loops.
Also applies to: 178-183
🤖 Prompt for AI Agents
In
src/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.html
around lines 29 and 178-183, the two *ngFor loops lack trackBy functions which
harms performance; add two methods to the corresponding component class (e.g.,
trackByItemId(index: number, item: JobPostingMessageSubscription): string
returning item.id and trackByProfessionId(index: number, profession:
LabelEntityDto): number returning profession.id) and then update each *ngFor in
the template to use trackBy (e.g., *ngFor="let item of items; let i = index;
trackBy: trackByItemId" and similarly for the profession loop) to ensure stable
keys during re-renders.
| // Ensure action dropdown is visible outside table container | ||
| .table-responsive { | ||
| overflow: visible; | ||
|
|
||
| .dropdown-menu { | ||
| position: absolute; | ||
| z-index: 1050; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the actual file to see the full context
cat -n "src/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.scss" 2>/dev/null || echo "File not found, let me search for it"Repository: Techinterview-space/frontend
Length of output: 674
🏁 Script executed:
# Search for the component file by name
fd "job-posting-message-subscriptions.component.scss" --type fRepository: Techinterview-space/frontend
Length of output: 196
🏁 Script executed:
# Find similar overflow: visible patterns in SCSS files
rg -n "overflow:\s*visible" --type=scss -B 2 -A 2Repository: Techinterview-space/frontend
Length of output: 100
🏁 Script executed:
# Search for table-responsive usage in SCSS files
rg -n "\.table-responsive" --type=scss -A 3 -B 1Repository: Techinterview-space/frontend
Length of output: 100
🏁 Script executed:
# Search for overflow: visible patterns in CSS/SCSS files (broader search)
rg -n "overflow:\s*visible" -t css -B 2 -A 2Repository: Techinterview-space/frontend
Length of output: 5692
🏁 Script executed:
# Also search in all files
rg -n "table-responsive" -B 2 -A 3 | head -50Repository: Techinterview-space/frontend
Length of output: 5410
🏁 Script executed:
# Check if there's any HTML template that shows how this component is used
fd "job-posting-message-subscriptions.component.html" --type fRepository: Techinterview-space/frontend
Length of output: 196
🏁 Script executed:
# Read the HTML template to understand the structure
cat -n "src/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.html" | head -50Repository: Techinterview-space/frontend
Length of output: 2406
🏁 Script executed:
# Read more of the HTML to see the dropdown structure
cat -n "src/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.html" | sed -n '46,80p'Repository: Techinterview-space/frontend
Length of output: 1634
Address ::ng-deep deprecation and dropdown overflow positioning.
The code uses :host ::ng-deep (lines 8-16) which is deprecated. Additionally, overflow: visible on .table-responsive (lines 19-20) defeats Bootstrap's horizontal scrolling on mobile devices.
Recommended alternatives:
- Replace
:host ::ng-deepwith a container component or encapsulation strategy - For dropdown positioning, consider using a positioning context wrapper or adjusting the modal/table structure instead of removing overflow containment
Verify that horizontal scrolling is preserved on narrow viewports and dropdowns don't cause page-wide overflow.
| this.professionsAsOptions = x.map((p) => ({ | ||
| value: p.id.toString(), | ||
| item: p.id, | ||
| label: p.title, | ||
| })); |
There was a problem hiding this comment.
Fix SelectItem type inconsistency.
The mapping creates SelectItem<number> but assigns value as a string (line 70):
value: p.id.toString(), // string
item: p.id, // numberThis type mismatch can cause issues. Since bindValue="item" in the template, the value property may be redundant. Verify the SelectItem interface definition.
🔎 Proposed fix
this.professionsAsOptions = x.map((p) => ({
- value: p.id.toString(),
+ value: p.id.toString(), // or remove if not used
item: p.id,
label: p.title,
}));Or simplify if value isn't needed:
this.professionsAsOptions = x.map((p) => ({
item: p.id,
label: p.title,
}));🤖 Prompt for AI Agents
In
src/app/modules/admin/components/telegram/job-posting-message-subscriptions/job-posting-message-subscriptions.component.ts
around lines 69 to 73, the SelectItem mapping creates a type mismatch by setting
value to a string (p.id.toString()) while item is a number; since the template
uses bindValue="item" the value field is redundant — either remove the value
property entirely and map only item: p.id and label: p.title, or make value a
number (value: p.id) to match SelectItem<number>; also verify the SelectItem<T>
interface definition to ensure the chosen shape matches its generic type.
| <div class="canvas-container"> | ||
| <canvas [id]="canvasId">{{ chart }}</canvas> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the pattern of canvas with interpolation in the codebase
rg -nP --type=html '<canvas[^>]*>.*\{\{.*\}\}.*</canvas>' -C2Repository: Techinterview-space/frontend
Length of output: 13624
🏁 Script executed:
#!/bin/bash
# Also check the specific component file to understand the context
cat -n src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart.component.htmlRepository: Techinterview-space/frontend
Length of output: 302
🏁 Script executed:
#!/bin/bash
# Check the TypeScript component to understand what the 'chart' variable is
cat -n src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart.component.tsRepository: Techinterview-space/frontend
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Find the TypeScript file for this component
fd "historical-salaries-chart" -t fRepository: Techinterview-space/frontend
Length of output: 444
🏁 Script executed:
#!/bin/bash
# Check the directory structure
ls -la src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/Repository: Techinterview-space/frontend
Length of output: 623
🏁 Script executed:
#!/bin/bash
# Search for any Chart.js or canvas setup patterns to understand the intent
rg -n "canvasId\|chart.*Chart\|Chart\.js" src/app/modules/salaries/components/historical-charts-page/ -A 2 -B 2Repository: Techinterview-space/frontend
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check the parent component to understand the structure
cat -n src/app/modules/salaries/components/historical-charts-page/historical-charts-page.component.tsRepository: Techinterview-space/frontend
Length of output: 2199
🏁 Script executed:
#!/bin/bash
# Check what the object file contains
cat -n src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart-object.tsRepository: Techinterview-space/frontend
Length of output: 4521
🏁 Script executed:
#!/bin/bash
# Search for how this component is used (maybe it's standalone or declared differently)
rg -n "historical-salaries-chart" src/app/modules/salaries/ --type=ts -A 2 -B 2Repository: Techinterview-space/frontend
Length of output: 2623
🏁 Script executed:
#!/bin/bash
# Get the full TypeScript component file
cat -n src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-charts-page.component.tsRepository: Techinterview-space/frontend
Length of output: 1525
🏁 Script executed:
#!/bin/bash
# Check the parent HTML to see how this component is used
cat -n src/app/modules/salaries/components/historical-charts-page/historical-charts-page.component.htmlRepository: Techinterview-space/frontend
Length of output: 1278
Remove the canvas text interpolation {{ chart }}—it serves no functional purpose.
The chart variable is a HistoricalSalariesChartObject instance (which extends Chart.js's Chart class). Chart.js renders via direct canvas 2D API manipulation, so the Angular interpolation {{ chart }} either displays nothing or renders as [object Object] as fallback text. This pattern appears systematically across 18+ chart components in the codebase and should be cleaned up to reduce confusion.
🤖 Prompt for AI Agents
In
src/app/modules/salaries/components/historical-charts-page/historical-salaries-chart/historical-salaries-chart.component.html
around lines 5 to 7, remove the Angular interpolation {{ chart }} inside the
<canvas> element because Chart.js draws directly to the canvas and the
interpolation either shows nothing or prints [object Object]; simply leave the
canvas with its id binding (e.g., <canvas [id]="canvasId"></canvas>) so the
canvas is empty for Chart.js rendering and avoid similar interpolations across
other chart components.
| salariesChart( | ||
| params: SalariesChartFilterData, | ||
| params?: SalariesChartFilterData, | ||
| ): Observable<GetSalariesHistoricalChartResponse> { | ||
| return this.api.get<GetSalariesHistoricalChartResponse>( | ||
| this.apiUrl + "salaries?" + new ConvertObjectToHttpParams(params).get(), | ||
| ); | ||
| const queryString = params | ||
| ? new ConvertObjectToHttpParams(params).get() | ||
| : ""; | ||
| const url = queryString | ||
| ? this.apiUrl + "salaries?" + queryString | ||
| : this.apiUrl + "salaries"; | ||
| return this.api.get<GetSalariesHistoricalChartResponse>(url); | ||
| } |
There was a problem hiding this comment.
Fix query string construction.
The ConvertObjectToHttpParams(...).get() method returns an HttpParams object, not a string. You need to call .toString() on the result to convert it to a query string, or pass the HttpParams object directly to the API call.
🔎 Proposed fix
salariesChart(
params?: SalariesChartFilterData,
): Observable<GetSalariesHistoricalChartResponse> {
- const queryString = params
- ? new ConvertObjectToHttpParams(params).get()
- : "";
- const url = queryString
- ? this.apiUrl + "salaries?" + queryString
- : this.apiUrl + "salaries";
- return this.api.get<GetSalariesHistoricalChartResponse>(url);
+ if (params) {
+ const httpParams = new ConvertObjectToHttpParams(params).get();
+ return this.api.get<GetSalariesHistoricalChartResponse>(
+ this.apiUrl + "salaries",
+ { params: httpParams }
+ );
+ }
+ return this.api.get<GetSalariesHistoricalChartResponse>(
+ this.apiUrl + "salaries"
+ );
}🤖 Prompt for AI Agents
In src/app/services/historical-charts.service.ts around lines 52 to 62, the code
treats ConvertObjectToHttpParams(...).get() as a string but it returns an
HttpParams; either call .toString() on that result when building the URL query
string, or better: keep the HttpParams and pass it to this.api.get(...) via the
request options (e.g.,
this.api.get<GetSalariesHistoricalChartResponse>(this.apiUrl + 'salaries', {
params: httpParams })). Update the query construction accordingly and remove
manual string concatenation when using the options-based approach.
| surveyChart( | ||
| params: SalariesChartFilterData, | ||
| ): Observable<HistoricalSurveyChartResponse> { | ||
| return this.api.get<HistoricalSurveyChartResponse>( | ||
| this.apiUrl + "survey?" + new ConvertObjectToHttpParams(params).get(), | ||
| ); | ||
| const queryString = params | ||
| ? new ConvertObjectToHttpParams(params).get() | ||
| : ""; | ||
| const url = queryString | ||
| ? this.apiUrl + "survey?" + queryString | ||
| : this.apiUrl + "survey"; | ||
| return this.api.get<HistoricalSurveyChartResponse>(url); | ||
| } |
There was a problem hiding this comment.
Fix query string construction and make params optional.
Similar to salariesChart, the query string construction has the same issue. Additionally, consider making params optional for consistency with salariesChart.
🔎 Proposed fix
surveyChart(
- params: SalariesChartFilterData,
+ params?: SalariesChartFilterData,
): Observable<HistoricalSurveyChartResponse> {
- const queryString = params
- ? new ConvertObjectToHttpParams(params).get()
- : "";
- const url = queryString
- ? this.apiUrl + "survey?" + queryString
- : this.apiUrl + "survey";
- return this.api.get<HistoricalSurveyChartResponse>(url);
+ if (params) {
+ const httpParams = new ConvertObjectToHttpParams(params).get();
+ return this.api.get<HistoricalSurveyChartResponse>(
+ this.apiUrl + "survey",
+ { params: httpParams }
+ );
+ }
+ return this.api.get<HistoricalSurveyChartResponse>(
+ this.apiUrl + "survey"
+ );
}🤖 Prompt for AI Agents
In src/app/services/historical-charts.service.ts around lines 64 to 74, the
surveyChart method currently requires params and builds the query string
unconditionally which can produce an empty or malformed query; change the
signature to make params optional, only construct ConvertObjectToHttpParams when
params is provided (and use its get() result), and build the URL such that when
queryString is empty you call this.apiUrl + "survey" otherwise this.apiUrl +
"survey?" + queryString; return the same
this.api.get<HistoricalSurveyChartResponse>(url). Ensure the behavior mirrors
salariesChart for consistency.
| search( | ||
| pageParams: PageParams = defaultPageParams, | ||
| ): Observable<PaginatedList<SalariesHistoricalDataRecordTemplateDto>> { | ||
| return this.api.get<PaginatedList<SalariesHistoricalDataRecordTemplateDto>>( | ||
| this.apiUrl + "?" + new ConvertObjectToHttpParams(pageParams).get(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix query string construction.
The ConvertObjectToHttpParams(...).get() method returns an HttpParams object, not a string. You should pass the HttpParams object directly to the API call instead of concatenating it as a string.
🔎 Proposed fix
search(
pageParams: PageParams = defaultPageParams,
): Observable<PaginatedList<SalariesHistoricalDataRecordTemplateDto>> {
+ const httpParams = new ConvertObjectToHttpParams(pageParams).get();
return this.api.get<PaginatedList<SalariesHistoricalDataRecordTemplateDto>>(
- this.apiUrl + "?" + new ConvertObjectToHttpParams(pageParams).get(),
+ this.apiUrl,
+ { params: httpParams }
);
}📝 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.
| search( | |
| pageParams: PageParams = defaultPageParams, | |
| ): Observable<PaginatedList<SalariesHistoricalDataRecordTemplateDto>> { | |
| return this.api.get<PaginatedList<SalariesHistoricalDataRecordTemplateDto>>( | |
| this.apiUrl + "?" + new ConvertObjectToHttpParams(pageParams).get(), | |
| ); | |
| } | |
| search( | |
| pageParams: PageParams = defaultPageParams, | |
| ): Observable<PaginatedList<SalariesHistoricalDataRecordTemplateDto>> { | |
| const httpParams = new ConvertObjectToHttpParams(pageParams).get(); | |
| return this.api.get<PaginatedList<SalariesHistoricalDataRecordTemplateDto>>( | |
| this.apiUrl, | |
| { params: httpParams } | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/app/services/salaries-historical-data-templates.service.ts around lines
34 to 40, the code concatenates the result of
ConvertObjectToHttpParams(...).get() to the URL even though get() returns an
HttpParams object; change the call to pass the params object to this.api.get as
options instead of string concatenation (e.g. call
this.api.get<...>(this.apiUrl, { params: new
ConvertObjectToHttpParams(pageParams).get() })), removing the "?" +
concatenation so the HttpParams is sent correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/app.component.spec.ts (1)
18-20: Remove redundantAuthServiceprovider.The
AuthServiceis provided explicitly on line 18 withMockAuthService, butmostUsedServices(line 20) also includes the same provider. While the explicit provider takes precedence, this duplication is redundant and could confuse maintainers.🔎 Proposed fix
providers: [ - { provide: AuthService, useClass: MockAuthService }, ...testUtilStubs, ...mostUsedServices, SpinnerService, ],
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/app.component.spec.tssrc/app/app.component.ts
💤 Files with no reviewable changes (1)
- src/app/app.component.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: Review TypeScript code for:
- Proper typing and avoiding 'any' type
- Consistent use of interfaces and type aliases
- Null safety with strict null checks
- Proper error handling
- Code organization and single responsibility
Files:
src/app/app.component.spec.ts
**/*.spec.ts
⚙️ CodeRabbit configuration file
**/*.spec.ts: Review test files for:
- Adequate test coverage
- Proper use of TestBed and component fixtures
- Mock services and HTTP testing
- Async testing with fakeAsync/waitForAsync
- Isolated unit tests vs integration tests
Files:
src/app/app.component.spec.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: maximgorbatyuk
Repo: Techinterview-space/frontend PR: 74
File: src/app/modules/companies/components/add-review-page/add-review-page.component.spec.ts:31-33
Timestamp: 2025-04-30T04:43:39.004Z
Learning: maximgorbatyuk is prioritizing MVP functionality for the companies feature and may address expanded test coverage (form validation, submission, error handling, rating selection) in a later phase.
🧬 Code graph analysis (1)
src/app/app.component.spec.ts (2)
src/app/shared/test-utils/mock-auth.service.ts (1)
MockAuthService(8-57)src/app/shared/test-utils/index.ts (2)
testUtilStubs(36-41)mostUsedServices(43-53)
🔇 Additional comments (4)
src/app/app.component.spec.ts (4)
2-2: LGTM: Correct migration towaitForAsync.The migration from the deprecated
asynctowaitForAsyncfollows Angular testing best practices and improves code clarity for asynchronous test initialization.Also applies to: 15-15, 27-27
6-6: Verify whetherSpinnerServiceshould be mocked.
SpinnerServiceis added as a real implementation rather than a mock. For unit test isolation, services are typically mocked unless they're simple and have no side effects or external dependencies. Please confirm whether this service should remain as-is or be replaced with a mock.As per coding guidelines, test files should use mock services for proper isolation.
Also applies to: 21-21
5-5: LGTM: Test utilities and SharedModule addition.The addition of
SharedModuleand consolidation of test utilities (testUtilStubs) improves test setup and maintainability. This aligns with the broader test modernization effort across the codebase.Also applies to: 19-19, 24-24
20-20: LGTM: Consolidated service providers.Using
mostUsedServicesreduces boilerplate and improves maintainability by consolidating commonly used test service providers.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.