Integrated code lifecycle: Improve build details view
#10462
Conversation
github-actions
bot
added
tests
server
WalkthroughThis update introduces a new DTO, BuildAgentDetailsDTO, to encapsulate build agent metrics and refactors the BuildAgentInformation record to include detailed build data. A new BuildAgentInformationService is added to centralize build agent state management within a Hazelcast distributed environment, with corresponding adjustments across backend endpoints and Angular components. The changes also streamline filtering functionality in the build queue, update localization keys, and revise several test suites while removing deprecated methods. Changes
Sequence Diagram(s)sequenceDiagram
participant SQS as SharedQueueProcessingService
participant BAIS as BuildAgentInformationService
participant HA as HazelcastInstance
participant DDA as DistributedDataAccessService
SQS->>BAIS: updateLocalBuildAgentInformation(isPaused)
BAIS->>BAIS: updateLocalBuildAgentInformationWithRecentJob(recentBuildJob, isPaused)
BAIS->>HA: Acquire lock & update build agent state
BAIS->>DDA: Retrieve current build agent metrics
DDA-->>BAIS: Return BuildAgentInformation
BAIS->>BAIS: Compute BuildAgentDetailsDTO
BAIS-->>SQS: Return updated BuildAgentInformation
sequenceDiagram
participant U as User
participant BQC as BuildQueueComponent
participant FBFC as FinishedBuildsFilterModalComponent
participant BQS as BuildQueueService
U->>BQC: Click to open filter modal
BQC->>FBFC: Open filter modal dialog
FBFC->>U: Display filter options
U->>FBFC: Set filter criteria & confirm
FBFC->>BQC: Return filter data
BQC->>BQS: Request finished build jobs with filters
BQS-->>BQC: Return filtered build jobs
BQC->>U: Update job list on UI
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 29 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (31)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 29 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (31)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
WalkthroughThis pull request introduces a new Java record, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SharedQueueProcessingService
participant BuildAgentInformationService
participant Hazelcast
Client->>SharedQueueProcessingService: Trigger update (e.g., job completion)
SharedQueueProcessingService->>BuildAgentInformationService: updateLocalBuildAgentInformation(isPaused)
BuildAgentInformationService->>Hazelcast: Lock and update build agent info map
Hazelcast-->>BuildAgentInformationService: Return updated data
BuildAgentInformationService-->>SharedQueueProcessingService: Return updated info
SharedQueueProcessingService-->>Client: Acknowledge update
sequenceDiagram
participant User
participant BuildQueueComponent
participant NgbModal
participant FinishedBuildsFilterModalComponent
User->>BuildQueueComponent: Click filter/build logs button
BuildQueueComponent->>NgbModal: open(FinishedBuildsFilterModalComponent)
NgbModal->>FinishedBuildsFilterModalComponent: Display modal for finished build jobs filtering
FinishedBuildsFilterModalComponent-->>User: User inputs filter criteria and confirms
FinishedBuildsFilterModalComponent-->>BuildQueueComponent: Return filter state
BuildQueueComponent->>BuildQueueComponent: Load finished build jobs with new filters
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (16)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (2)
246-258: Method needs renaming to reflect its new purposeThe method
addResultToBuildAgentsRecentBuildJobsno longer adds results to recent build jobs as the name suggests. Consider renaming it to something likeupdateBuildAgentInformationto better reflect its current purpose.-private void addResultToBuildAgentsRecentBuildJobs(BuildJobQueueItem buildJob, Result result) { +private void updateBuildAgentInformation(BuildJobQueueItem buildJob, Result result) {
204-204: Update method call to match the suggested name changeIf you rename the method as suggested above, update this method call as well.
- addResultToBuildAgentsRecentBuildJobs(buildJob, result); + updateBuildAgentInformation(buildJob, result);src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html (2)
20-32: Suggested accessibility improvement for radio buttons.While the implementation is correct, consider adding
aria-labelattributes to the radio buttons for better accessibility.<input class="form-check-input" (change)="this.toggleBuildStatusFilter(status)" [checked]="this.finishedBuildJobFilter.status === status" type="radio" + [attr.aria-label]="'artemisApp.buildQueue.filter.buildStatus.' + status | artemisTranslate" />
80-87: Consider consistent ID naming pattern for date-time pickers.The current implementation uses
field_startDateandfield_endDateas IDs. Consider using camelCase or kebab-case consistently throughout your application.-id="field_startDate" +id="fieldStartDate" -id="field_endDate" +id="fieldEndDate"Also applies to: 93-100
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts (2)
13-19: Consider using a stricter enum type forstatus.Currently,
statusis declared as a nullablestring. Consider using theBuildJobStatusFilterenum to reduce potential type mismatches and improve clarity, as it strongly indicates all valid status options.- status?: string = undefined; + status?: BuildJobStatusFilter = undefined;
217-224: Ensure negative durations are disallowed or handled.Currently, the only check is to ensure
lowerBound <= upperBound. To safeguard from invalid inputs (e.g., negative values), consider validating that both bounds are ≥ 0.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (4)
27-49: Consider adding i18n for labels."Status," "Start Date," "Latest Build Date," etc. appear as plain text. Wrapping them with a translation directive (e.g.,
jhiTranslate) could provide consistent localization support.
53-122: Extract magic number for clarity.The literal 240 seconds is used to flag long build durations. Representing it as a named constant or adding an explanatory comment increases readability and maintainability.
289-320: Enhance usability with a placeholder.Adding a placeholder text (e.g., "Search jobs…") could clarify the field’s purpose for users.
423-456: Refactor repeated routerLink logic if reused elsewhere.Several routerLink variants exist for template, solution, or user triggers. A small utility function or pipe could centralize this logic if used beyond this component.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (1)
295-295: Update local info on RejectedExecutionException.Consider logging success/failure of buildAgentInformationService calls for better traceability.
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (2)
49-57: Avoid duplicate entries in theimportsarray.
Lines 41/54 (DataTableComponent) and 40/55 (NgxDatatableModule) appear twice, which can be removed to adhere to the DRY principle.imports: [ NgxDatatableModule, DataTableComponent, ArtemisDurationFromSecondsPipe, ... - DataTableComponent, - NgxDatatableModule, ... ]
149-156: Properly initializing filter object and loading finished jobs.
Consider adding an error callback to handle potential failures ingetBuildAgentDetails().src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (3)
71-90: Locking and unlocking Hazelcast map ensures concurrency control.
Consider using a lock timeout to avoid potential indefinite lock scenarios under heavy load.
140-148: Integer-based average build duration.
This truncates fractional seconds. Consider if a higher granularity (e.g., double) is needed.
174-178: Retrieving processing jobs by filtering all map values.
For larger-scale usage, consider indexing by member address to avoid iterating all jobs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(10 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java(0 hunks)src/main/webapp/app/entities/programming/build-agent-information.model.ts(2 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html(2 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.scss(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(7 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html(2 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.html(2 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.scss(0 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.service.ts(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.scss(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(1 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts(7 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts(5 hunks)src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts(5 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts(6 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java
- src/main/webapp/app/localci/build-queue/build-queue.component.scss
🧰 Additional context used
📓 Path-based instructions (5)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.javasrc/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.htmlsrc/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.htmlsrc/main/webapp/app/localci/build-queue/build-queue.component.htmlsrc/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/localci/build-queue/build-queue.service.tssrc/main/webapp/app/entities/programming/build-agent-information.model.tssrc/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.tssrc/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.tssrc/main/webapp/app/localci/build-queue/build-queue.component.tssrc/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.tssrc/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (113)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.scss (1)
6-8: Good addition for consistent layoutAdding a minimum width to the stats column ensures that the UI will maintain a consistent layout even with varying content lengths, which improves readability.
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.scss (1)
1-11: Good use of CSS variables for consistent stylingUsing CSS variables like
--overview-blue-border-colorand--user-management-background-colorpromotes consistency across the application and makes theme updates easier to manage.The class naming follows a consistent pattern (
finished-build-jobs-filter-*) which makes the purpose of each style clear and maintains good organization.src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts (1)
23-23: Import cleanup improves code maintenanceRemoving unused imports like
ArtemisTranslatePipefollows Angular best practices and keeps the component's dependencies minimal and clear.src/main/webapp/app/localci/build-queue/build-queue.service.ts (1)
9-9: Import path correctly updated for relocated componentThe import path has been properly updated to reflect the new location of
FinishedBuildJobFilterin the separated filter modal component, which aligns with the PR objective of moving the modal to its own component.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (1)
74-74: Updated method call to align with refactored APIThe change from using
getBuildAgentInformationWithoutRecentBuildJobs()togetBuildAgentInformation()aligns with the broader refactoring in the PR where build agent information handling has been centralized. This ensures consistent information retrieval across the application.src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java (1)
86-86: Consistent method replacement for build agent information retrievalThis change is part of the broader refactoring to centralize build agent information handling. The updated method call maintains the same functionality while aligning with the new implementation pattern.
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
99-99: Updated BuildAgentInformation constructor usageThe test has been properly updated to match the new BuildAgentInformation structure. This change reflects the modifications to the BuildAgentInformation record where recentBuildJobs parameter has been removed and agent details are now handled differently.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java (1)
184-184: Consistent method update for health monitoringThe method call change from
getBuildAgentInformationWithoutRecentBuildJobs()togetBuildAgentInformation()ensures consistency with the other components. This maintains the same health monitoring functionality while aligning with the new agent information architecture.src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.java (2)
7-8: Good implementation of a DTO as a recordThe use of a Java record here is ideal for a Data Transfer Object. Records provide immutable data structures with built-in equals(), hashCode(), and toString() methods, making them perfect for transferring data.
10-11: Good practice for serialization compatibilityThe inclusion of a serialVersionUID field with the @serial annotation is a good practice for serializable classes, ensuring compatibility across different versions of the class.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html (2)
5-21: Good use of new Angular @if directiveYou've correctly implemented the new Angular syntax using @if instead of *ngIf, which aligns with the coding guidelines. This conditional rendering improves component reusability.
76-87: Consistent implementation of conditional renderingThe implementation of conditional rendering for the missing builds section follows the same pattern as other sections, providing consistent UI behavior.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
251-251: Good simplification of build agent information updateThe code has been simplified to only update the build agent information without modifying the recent build jobs list, aligning with the refactoring of build agent information management described in the PR objectives.
src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts (2)
9-9: Good import specificationThe import is correctly specified for the BuildAgentInformation model, which aligns with the project's structure after the refactoring.
22-27: Test setup appropriately simplifiedThe test setup has been simplified by removing the unnecessary mock data for build jobs, which aligns with the changes in how build agent information is structured. The tests still effectively verify the API interactions of the BuildAgentsService.
src/main/webapp/app/entities/programming/build-agent-information.model.ts (3)
4-4: Good import of dayjs module.The import of dayjs from its ESM path is correct and follows Angular best practices for tree-shakable imports.
19-19: Appropriate addition of BuildAgentDetails property.The addition of an optional
buildAgentDetailsproperty to theBuildAgentInformationclass correctly implements the new data structure requirement. This property follows the proper camelCase naming convention as specified in the coding guidelines.
22-31: Well-structured BuildAgentDetails class.The new
BuildAgentDetailsclass is well-organized with appropriate property types and follows the camelCase naming convention for properties as specified in the coding guidelines. Usingdayjs.Dayjsas the type for date fields is the correct approach for date handling in TypeScript.src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts (3)
16-16: Properly updated import path for FinishedBuildJobFilter.The import path has been correctly updated to reflect the new location of the
FinishedBuildJobFiltercomponent, which is now in a dedicated modal component.
18-18: Simplified import path for BuildLogEntry.The import path has been updated to use the absolute path format which is more maintainable and follows Angular best practices.
53-53: Proper test cleanup.Adding this line ensures that the mock for
getBuildJobStatisticsForCourseis cleared before each test, preventing any state from leaking between tests. This is a good testing practice.src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html (1)
1-138: Well-structured modal component using modern Angular syntax.The component follows a clean, organized structure with proper separation of concerns. Key observations:
- Uses modern
@ifand@fordirectives instead of older*ngIfand*ngFordirectives as specified in the coding guidelines.- Correctly implements i18n with
jhiTranslatedirectives.- Uses appropriate Bootstrap classes for layout and styling.
- Implements proper form controls with two-way binding and appropriate event handlers.
- Includes validation feedback for form inputs.
The structure is consistent with the PR's objective of moving the filter functionality to a dedicated modal component.
src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts (3)
16-16: Good change from constant to variable for mockActivatedRoute.Changing
mockActivatedRoutefrom a constant to a variable allows for more flexible test setup, enabling the assignment of different values between tests.
57-58: Consistent test setup for routing scenarios.The addition of the
urlproperty to themockActivatedRoutein these tests is a good practice as it more accurately simulates the behavior of the actual router in different scenarios.Also applies to: 68-69, 79-80
90-102: Good test for input-driven component behavior.This new test properly verifies that when statistics are provided via input property, the component correctly uses them instead of fetching from the service. The test also validates display property states, which is thorough.
The testing approach correctly uses:
fixture.componentRef.setInput()to set the input property- Assertions on service call counts to verify the services aren't called
- Verification of component state reflecting the input data
This follows Jest testing best practices as specified in the coding guidelines.
src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts (5)
17-18: Good addition of required imports for modal testingThe addition of these modal-related imports is necessary to properly test the modal interactions in the component.
113-113: LGTM - Properly declared modal service variable for testingAdding this variable declaration allows for proper dependency injection and mocking of the NgbModal service in tests.
122-122: Good practice using mock service for NgbModal testingUsing the MockNgbModalService instead of the real NgbModal service is a good practice as it avoids DOM manipulation during tests and provides controlled test behavior.
132-132: LGTM - Properly injected modal serviceCorrectly injected the modal service into the test suite using TestBed.inject().
254-265: Good test coverage for modal interactionsThe test properly verifies that the component's methods correctly interact with the modal service. This is important for ensuring that users can open the required modals from the UI.
The test follows best practices by:
- Mocking the NgbModalRef return value
- Using a spy to verify the modal service's open method is called
- Checking the number of calls to ensure proper behavior
src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts (6)
1-12: Good test setup with proper imports and dependenciesThe test properly imports all necessary dependencies and sets up the testing environment correctly for the new component.
13-65: Well-structured test setup with good mock dataThe test suite is well-structured with a comprehensive mock dataset that covers the scenarios needed for testing. The TestBed configuration properly imports the required modules and provides mock services.
67-74: Basic tests ensure component creation and core functionalityGood practice to verify that the component is created successfully and test core functionality like retrieving build agent addresses.
76-96: Comprehensive filter counting logic testingThis test thoroughly verifies the filter counting logic by testing various combinations of filter changes and ensuring the count is updated correctly.
98-122: Good testing of filter persistence and reset functionalityThis test verifies both the saving of filters and the reset functionality, ensuring the filter state is properly managed.
124-149: Thorough validation logic testingGood test coverage for the validation logic, testing both valid and invalid date and duration filter combinations. The test ensures the component correctly identifies valid and invalid filter states.
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java (2)
14-15: Good refactoring to encapsulate build agent detailsReplacing direct build jobs handling with a dedicated DTO improves separation of concerns and follows the single responsibility principle. The record now accepts
BuildAgentDetailsDTOwhich encapsulates build agent statistics.
25-28: Constructor properly updated to use the new field structureThe constructor correctly passes the buildAgentDetails from the parameter to maintain the record's state during copy operations.
src/main/webapp/app/localci/build-queue/build-queue.component.html (3)
438-440: Good refactoring of filter modal interactionThe change from direct modal template reference to a method call improves component organization by moving the filter modal to a separate component. The use of optional chaining (
?.) fornumberOfAppliedFiltersis also a good practice to prevent null reference errors.
443-443: Good use of nullish coalescing for safe default valueUsing
?? 0provides a safe default value whenfinishedBuildJobFilterornumberOfAppliedFiltersis null or undefined, preventing potential display issues.
676-704: Well-structured modal for build logsThe modal follows good UI practices with clear header, body, and footer sections. It properly handles the display of build logs and provides actions for closing the modal and downloading logs.
The template correctly:
- Uses modern Angular syntax (
@ifdirective)- Provides proper user feedback when no logs are available
- Includes appropriate buttons for user interaction
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts (3)
34-55: Looks good – remember to handle empty or null field scenarios.Using
appendinaddHttpParamsis straightforward. Just confirm that you won't pass empty or invalid values by mistake (e.g.,undefined) to the server.
184-202: Validate time zone handling for date filters.When converting dayjs dates to ISO strings, be cautious about implicit time zone assumptions. If needed, clarify whether to store times in UTC or in the user’s local time.
108-236: Add or confirm unit test coverage for this new component.Given the importance of these filter operations, ensure robust test scenarios for date validations, duration bounds, and status toggles. This helps maintain correctness as the code evolves.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (5)
1-1: New Angular 16 imports look correct.Using
inputfrom@angular/coreis valid in Angular 16. Ensure your project’s version supports this feature and that teammates are aware of this new pattern.
30-30: Signal-based input usage is a great approach.
buildJobStatisticsInput = input<BuildJobStatistics>();nicely enables a reactive approach for passing data. This is consistent with modern Angular best practices.
45-46: Good initiative displaying additional UI elements conditionally.Using
displaySpanSelectoranddisplayMissingBuildsflags promotes clearer conditional logic in the template. This is a clean way to handle context-dependent UI states.
69-80: Logic to determine statistics source is neatly handled.The conditional approach for checking
url[0].path === 'build-queue'is clear. Just ensure that any future path changes or routing updates also adjust this check accordingly.
84-84: Method renaming clarifies purpose.Renaming to
getBuildJobStatisticsForBuildQueuebetter communicates its specialized role. This aligns with the single-responsibility principle and improves maintainability.src/main/webapp/app/localci/build-queue/build-queue.component.ts (3)
1-1: Consolidation of imports is appropriate.Changes to import statements reflect the introduction of the new filter modal and streamlined RxJS usage. This makes the file more focused on core build-queue logic.
Also applies to: 7-7, 12-12, 15-15, 31-31, 32-32, 33-33
91-92: Good choice to store the user’s filters as a typed object.Defining
finishedBuildJobFilterwith a dedicated type clarifies which fields can be used to filter results. Keep it in sync as filtering requirements evolve.
398-407: Modal usage is cleanly integrated.Opening the new
FinishedBuildsFilterModalComponentand handling its promise via.then()is straightforward. Tying the result tothis.loadFinishedBuildJobs()ensures the list re-fetches with updated filters.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (5)
4-4: No concerns.This margin update from
mb-3tomb-4is a minor stylistic adjustment.
51-51: Usage of jhi-build-job-statistics component looks good.
321-421: Table structure is consistent and well-organized.
458-520: Columns are descriptive and straightforward.
523-537: Pagination handling is correct.Displays item count and integrates well with ngb-pagination.
src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (4)
4-4: Import statements reflect new filter modal design.Also applies to: 18-21
255-255: Injecting NgbModal for filter modal usage.No issues found.
269-269: Providing MockNgbModalService is consistent.
534-553: New filter modal test is adequate.Ensures the modal’s properties and calls are handled correctly.
src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts (12)
5-5: Importing FinishedBuildJob is appropriate.Expands test coverage to include finished job scenarios.
18-23: HTTP and modal imports correctly added.No issues identified with the new dependencies.
130-166: Mock finished build jobs.Data set appears suitable. Confirm coverage of edge cases like missing or invalid fields if relevant.
168-169: HttpResponse for finishedBuildJobs.
172-177: Mock buildQueueService references are consistent.
202-205: Added mocks for queue operations.Implementation aligns with tested methods.
324-335: Search term refresh test is robust.Waits for debounce and verifies the updated request call.
337-348: Validating build job duration.Test thoroughly checks date handling logic.
350-370: Filter modal values test is clear.The modalRef usage ensures correctness of the instance properties.
372-377: Open build logs in new tab.Simple and effective approach to verifying external link logic.
40-40:❓ Verification inconclusive
Mocking getFinishedBuildJobs.
Double-check that the returned mock aligns with your test scenarios to prevent mismatched data shapes.
🏁 Script executed:
#!/bin/bash # Searching references to getFinishedBuildJobs in test files rg "getFinishedBuildJobs" -A 5Length of output: 24266
Ensure the getFinishedBuildJobs mock returns a realistic data structure
Please verify that the mock's return value exactly mirrors the expected finished build jobs data for this component’s test case. Currently, it returns an empty object:
getFinishedBuildJobs: jest.fn().mockReturnValue(of({})),Consider the following:
- Confirm whether your component expects an array (or a more detailed structure) rather than an empty object.
- Compare against similar tests (e.g., in build-queue.component.spec.ts, which mocks the response using
mockFinishedJobsResponse) and, if necessary, update the return value accordingly. For example:getFinishedBuildJobs: jest.fn().mockReturnValue(of(mockFinishedJobsResponse)),Review and adjust the mock to ensure consistency with your test scenarios and the actual data shape expected by your component.
109-128:❓ Verification inconclusive
Ensure request structure matches backend expectations.
Confirm that the server expects fields like page, pageSize, sortedColumn, sortingOrder, and searchTerm exactly as defined.
🏁 Script executed:
#!/bin/bash # Checking usage of request object across the codebase to confirm field alignment rg "page.*pageSize.*sortedColumn.*sortingOrder.*searchTerm" -A 10Length of output: 67
Action Required: Verify Request Object Alignment with Backend
The current test file snippet (src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts, lines 109-128) defines a request object with fields
page,pageSize,sortedColumn,sortingOrder, andsearchTerm. However, our grep search did not yield conclusive evidence of their usage elsewhere in the codebase.Please take a moment to manually verify that:
- These fields match the backend’s expected structure and naming conventions.
- There are no discrepancies between how the request is constructed in tests and how it is consumed by backend services.
Once confirmed, no changes are required. If misalignments are found, please update the request fields accordingly.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (7)
76-76: Introducing BuildAgentInformationService field.Good approach to centralize agent data updates.
132-140: Constructor injection using BuildAgentInformationService.Replaces older SSH key service; clarifies responsibility for agent updates.
319-319: Consistent local agent info update after queue poll.
375-375: Using updateLocalBuildAgentInformationWithRecentJob on success.Ensures the build agent’s recent job data is accurately tracked.
424-424: Updating info on failed job.Nice coverage for all job completion states.
443-443: Pause triggers a local info update.Lock usage prevents race conditions while pausing or resuming.
514-514: Resume refreshes local agent info.Properly updates the paused state back to active.
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (16)
4-5: No issues with new RxJS imports.
Additional operators likedebounceTime,switchMap, andtapalign well with Angular's reactive patterns for search functionality.
19-26: New imports for build job statistics and filtering look good.
These imports correctly introduce relevant data structures (e.g.,BuildJob,FinishedBuildJob,BuildJobStatistics) and components.
66-66: Consistent use of Angular inject for ModalService.
Usingprivate readonly modalService = inject(NgbModal)follows recommended Angular DI patterns.
70-71: Initialization of build job properties looks fine.
SettingbuildJobStatisticsandrunningBuildJobsat class level is clean and maintains clarity of component state.
78-78: Initialization of finishedBuildJobs array.
An empty array ensures the UI won’t break if no jobs exist.
81-90: FontAwesome icons declarations are consistent with project style.
Associating each icon with a readonly property is clear and aligns with typical Angular usage.
92-103: Pagination and filtering fields are well-defined.
However, make sure to unsubscribe from related observables if needed.Please confirm if these new properties (especially
searchSubscription) are unsubscribed inngOnDestroyto avoid memory leaks.
160-169: Populating buildJobStatistics from buildAgent details is correct.
Ensure thatmissingBuildsis either intentionally kept at 0 or computed if needed in the future.
181-181: Redirect to build logs endpoint.
Be mindful of potential user input inresultId. If user-provided, check for sanitation or route protection.
234-245: Filter modal usage appears consistent with Angular best practices.
The code sets the modal’s inputs, then updates the filter and reloads.
247-261: Fetching finished jobs via service and handling response.
Implementation follows a typical Angular pattern for success/error handling.
263-273: onSuccess method properly updates total items and builds array.
InvokingsetFinishedBuildJobsDurationkeeps UI logic separate and clear.
275-288: Computation of job duration via Day.js is straightforward.
This is reliable and clear.
290-304: Refactoring job fetching into a dedicated method is valid.
No issues; returning an observable is standard for Angular data flow.
306-313: Search debounce logic.
Limiting fetches to whensearchTermhas ≥ 3 characters is a good approach for performance.
315-325: Pagination change logic is clear.
Re-triggering the load upon page change is standard.src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (17)
1-4: Package and import statements.
No issues with package naming or static imports.
11-12: Usage ofjakarta.annotation.PostConstruct.
Modern approach for lifecycle management; no concerns.
13-21: Imports for logging and Hazelcast.
All relevant dependencies appear correct for the core service logic.
29-31: Service class with build agent profile.
Marking it with@Profile(PROFILE_BUILDAGENT)and@Servicealigns with separation of concerns in Spring.
33-59: Constructor injection is consistent with best practices.
All required beans are injected. This fosters immutable dependency references.
61-65: Initialization of Hazelcast maps.
@PostConstructusage is valid to set upbuildAgentInformationandprocessingJobs.
67-69: Simple public method for agent info update.
Delegates to the more detailed method without duplicating code.
92-110: Aggregating updated local build agent info.
Method is concise, with a clear return of a newBuildAgentInformationinstance reflecting the latest state.
112-125: Creation ofBuildAgentDetailsDTOseparates concerns neatly.
Good approach: gather relevant details in one place for agent statistics.
127-130: Determining last build date logic.
Use ofrecentBuildJob’s timestamp if available, else fallback to existing agent data. This is valid.
132-134: Assigning start date tonowif missing.
Verify that forcibly settingZonedDateTime.now()is correct for your domain logic.
136-138: Computing current build duration.
Check for potential null fields in buildStartDate or buildCompletionDate for incomplete jobs.
150-152: Incrementing total builds.
Logic is straightforward, adding 1 if a recent job exists.
154-157: Counting successful builds.
No issues; it checks existing stats and current job’s status.
159-162: Counting failed builds.
Same pattern as successful builds. Correct usage ofBuildStatus.FAILED.
164-167: Counting cancelled builds.
Continues consistent counting logic.
169-172: Counting timed-out builds.
UsesBuildStatus.TIMEOUT. No concerns.
...ocalci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
Show resolved
Hide resolved
WalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant BJ as Build Job Queue Item
participant SQPS as SharedQueueProcessingService
participant BAIS as BuildAgentInformationService
participant HZ as Hazelcast IMap
BJ->>SQPS: Build job finished
SQPS->>BAIS: updateLocalBuildAgentInformationWithRecentJob(finishedJob, isPaused)
BAIS->>BAIS: Compute metrics (duration, counts, dates, details)
BAIS->>HZ: Update build agent information in distributed map
BAIS-->>SQPS: Acknowledge update
sequenceDiagram
participant User as End User
participant BQC as BuildQueueComponent
participant FBM as FinishedBuildsFilterModalComponent
User->>BQC: Clicks filter modal button
BQC->>FBM: openFilterModal()
FBM->>User: Display filtering options (statuses, dates, duration)
User->>FBM: Sets filter criteria and confirms
FBM->>BQC: Returns selected filter state
BQC->>BQC: Fetch finished build jobs based on filters
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html (2)
1-139: Well-structured filter modal component with proper Angular syntaxThe new filter modal component is well-organized with a clear separation between header, body, and footer sections. The use of modern Angular control flow directives (
@if,@for) follows the coding guidelines. The form controls are properly bound to the filter model, and validation feedback is appropriately implemented.One suggestion for improved accessibility: consider adding
aria-labelattributes to form inputs that don't have visible labels to ensure screen readers can properly identify their purpose.
115-127: Duration filtering with validationThe duration filtering section is implemented appropriately with numeric input fields and validation feedback. Consider adding minimum value validation (e.g.,
min="0") to prevent negative duration values, which wouldn't make sense in this context.-<input type="number" class="form-control" [(ngModel)]="finishedBuildJobFilter.buildDurationFilterLowerBound" (change)="filterDurationChanged()" /> +<input type="number" min="0" class="form-control" [(ngModel)]="finishedBuildJobFilter.buildDurationFilterLowerBound" (change)="filterDurationChanged()" /> -<input type="number" class="form-control" [(ngModel)]="finishedBuildJobFilter.buildDurationFilterUpperBound" (change)="filterDurationChanged()" /> +<input type="number" min="0" class="form-control" [(ngModel)]="finishedBuildJobFilter.buildDurationFilterUpperBound" (change)="filterDurationChanged()" />src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts (1)
254-265: Consider verifying the arguments passed toopen.
The test successfully ensures the modal service is called twice. For improved assurance, you could verify the specific component or data provided tomodalService.open()if relevant.src/main/webapp/app/localci/build-queue/build-queue.component.ts (1)
7-7: Import organization looks good but could be simplified.The imports are well-organized, with Angular-related imports first, followed by the application-specific ones. The RxJS operators are nicely grouped together.
Consider grouping related RxJS imports together for better readability:
-import { debounceTime, switchMap, take, tap } from 'rxjs/operators'; -import { UI_RELOAD_TIME } from 'app/shared/constants/exercise-exam-constants'; -import { Subject, Subscription } from 'rxjs'; +import { Subject, Subscription } from 'rxjs'; +import { debounceTime, switchMap, take, tap } from 'rxjs/operators'; +import { UI_RELOAD_TIME } from 'app/shared/constants/exercise-exam-constants';Also applies to: 12-12, 31-33
src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (1)
534-553: Comprehensive test for filter modal initialization.This test effectively verifies that the filter modal is correctly initialized with the expected values. The use of Jest's spy functionality to mock the modal service is appropriate.
Consider adding a test for when the modal result is rejected:
it('should handle modal dismiss', async () => { const modalRef = { componentInstance: { finishedBuildJobFilter: undefined, buildAgentFilterable: undefined, finishedBuildJobs: undefined, }, result: Promise.reject('cancel'), } as NgbModalRef; jest.spyOn(modalService, 'open').mockReturnValue(modalRef); const loadSpy = jest.spyOn(component, 'loadFinishedBuildJobs'); await component.openFilterModal(); expect(loadSpy).not.toHaveBeenCalled(); });src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (7)
4-5: Consider unsubscribing from newly introduced subscriptions.
debounceTime, switchMap, tapare useful RxJS operators, but note that there's a newsearchSubscriptionfurther below. ThengOnDestroylifecycle hook currently unsubscribes fromwebsocketSubscription, but not fromsearchSubscription, which may cause a memory leak.+ ngOnDestroy() { + this.websocketService.unsubscribe(this.channel); + this.websocketSubscription?.unsubscribe(); + this.searchSubscription?.unsubscribe(); // Ensure to unsubscribe + }
49-57: Double-check for redundant imports in the imports array.
DataTableComponent,NgxDatatableModuleappear twice here—lines 16 and 41, then again lines 54 and 55. Angular best practices discourage listing duplicates in theimportsarray.- DataTableComponent, - NgxDatatableModule, ... - DataTableComponent, - NgxDatatableModule, + DataTableComponent, + NgxDatatableModule, ...
70-71: Provide or verify tests for buildJobStatistics usage.
ThebuildJobStatisticsproperty is newly introduced. Consider verifying that it’s being tested to ensure the displayed statistics are accurate.Would you like help creating or extending unit tests for this property?
91-97: Clarify searchTerm usage for partial queries.
Currently, it’s set to trigger after 3+ characters. Confirm you want to ignore shorter searches. Otherwise, consider removing the length check or using a separate approach (e.g., an immediate search for partial matches).
234-245: Improve user experience upon closing filter modal.
Currently, the modal rejection path (catch(() => {})) is silent. Consider showing a message or restoring defaults if the modal is closed without applying filters.
250-261: Avoid repeated code in loadFinishedBuildJobs and searchSubscription.
loadFinishedBuildJobsand the subscription’s.pipe(...)logic both callfetchFinishedBuildJobs(). Factor out a shared method or rely on the same data path for consistency.
309-313: Consider immediate search for single-character or empty queries.
Enforcing a minimum length is a design choice, but some users might prefer searching on shorter terms or if they type slowly.src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (3)
74-89: Check Hazelcast locking performance in high-traffic scenarios.
Locking the entire buildAgentInformation IMap for each update can cause contention if many agents update concurrently. If you anticipate heavy concurrency, consider a more granular approach (e.g., a separate lock per agent or a different concurrency strategy).
112-125: Avoid setting startDate to ZonedDateTime.now() if data is missing.
Projecting a brand-new timestamp each time can skew metrics. Instead, consider preserving any known stable start date or leaving it null to signify unknown.
174-179: Optimize retrieving processing jobs for large clusters.
CallingprocessingJobs.values()can be costly if the map is large. If performance is a concern, consider an indexed approach or a direct IMap query keyed by buildAgent.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(10 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java(0 hunks)src/main/webapp/app/entities/programming/build-agent-information.model.ts(2 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html(2 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.scss(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(7 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html(2 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.html(2 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.scss(0 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.service.ts(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.scss(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(1 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts(7 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts(5 hunks)src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts(5 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts(6 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java
- src/main/webapp/app/localci/build-queue/build-queue.component.scss
🧰 Additional context used
📓 Path-based instructions (5)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.javasrc/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/localci/build-queue/build-queue.service.tssrc/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.tssrc/main/webapp/app/entities/programming/build-agent-information.model.tssrc/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.tssrc/main/webapp/app/localci/build-queue/build-queue.component.tssrc/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.tssrc/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.htmlsrc/main/webapp/app/localci/build-queue/build-queue.component.htmlsrc/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.htmlsrc/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (80)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.scss (1)
6-8: Good addition for consistent column sizing.The new
.stats-columnclass with a minimum width ensures consistent sizing for statistical data display, improving the UI layout and readability of build agent metrics.src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts (1)
23-23: Removal of unused dependency is good.The removal of
ArtemisTranslatePipefrom the imports array reduces unnecessary dependencies. This aligns with Angular's best practices to only import what's needed.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java (1)
184-184:❓ Verification inconclusive
API method change reflects architectural improvements.
The switch from
getBuildAgentInformationWithoutRecentBuildJobs()togetBuildAgentInformation()aligns with the new centralized build agent information handling architecture. This change ensures that health checks include complete build agent data without filtering.
🏁 Script executed:
#!/bin/bash # Check if getBuildAgentInformationWithoutRecentBuildJobs() was removed from SharedQueueManagementService echo "Verifying that the old method was properly removed:" rg "getBuildAgentInformationWithoutRecentBuildJobs" --type javaLength of output: 176
Ensure Deprecated Method Removal & Confirm Centralized Data Handling
The new implementation in
LocalCIService.javacorrectly switches togetBuildAgentInformation(), which aligns with our updated centralized build agent handling architecture. An automated search (usingrg "getBuildAgentInformationWithoutRecentBuildJobs") showed no occurrences of the old method, suggesting it has been removed. However, since the automated output was minimal, please manually verify that all references togetBuildAgentInformationWithoutRecentBuildJobs()are completely removed from the codebase.
- Confirm that all instances and references to the deprecated method have been purged.
- Verify that using
getBuildAgentInformation()indeed returns a complete set of build agent data without the previous filtering.src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java (1)
86-86: API method change maintains consistency.The update to use
getBuildAgentInformation()instead ofgetBuildAgentInformationWithoutRecentBuildJobs()maintains consistency with other service calls and aligns with the architectural changes in build agent information handling. This ensures that the admin API returns the same complete data as other endpoints.src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.scss (2)
1-7: LGTM! Clean styling for filter border and background.The class definitions follow Angular's style guide with clear naming that indicates functionality. The use of CSS variables is good for maintainability and theme support.
9-11: LGTM! Good width definition for form prepend element.The width is set to an appropriate value for the UI component, ensuring consistent form layout.
src/main/webapp/app/localci/build-queue/build-queue.service.ts (1)
9-9: LGTM! Correct import path update.The import path update correctly reflects the relocation of
FinishedBuildJobFilterto its own dedicated component, which follows good code organization principles by separating the filter logic into its own component.src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
99-99:✅ Verification successful
Verify constructor parameter alignment with BuildAgentInformation record.
The updated constructor call no longer includes
job2in the list of jobs, which aligns with the PR objective of refactoring build agent information. However, if theBuildAgentInformationrecord now expects aBuildAgentDetailsDTOparameter instead of a list of recent build jobs, this constructor call should be updated accordingly.
🏁 Script executed:
#!/bin/bash # Check the current BuildAgentInformation constructor signature rg -A 10 "public record BuildAgentInformation\(" --type javaLength of output: 1439
Constructor parameters verified – no further changes required.
The BuildAgentInformation record now expects the following parameters:
- BuildAgentDTO (buildAgent)
- int (maxNumberOfConcurrentBuildJobs)
- int (numberOfCurrentBuildJobs)
- List<BuildJobQueueItem> (runningBuildJobs)
- BuildAgentStatus (status)
- String (publicSshKey)
- BuildAgentDetailsDTO (buildAgentDetails)
The constructor call in
LocalCIResourceIntegrationTest.java:agent1 = new BuildAgentInformation(buildAgent, 2, 1, new ArrayList<>(List.of(job1)), BuildAgentInformation.BuildAgentStatus.IDLE, null, null);remains aligned with this signature. The removal of
job2from the list complies with the PR objective. Additionally, passingnullforbuildAgentDetailsis valid if no BuildAgentDetailsDTO is available. No further updates are needed based on the current codebase.src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.java (1)
1-12: LGTM! Well-structured DTO for build agent details.This new record properly encapsulates build agent metrics with appropriate field types. The implementation of Serializable with a serialVersionUID follows best practices for Java serialization. The use of Java records for DTOs aligns with the coding guidelines.
Some specific benefits of this approach:
- Immutable data structure prevents accidental modifications
- Clear separation of build agent statistics from other build agent information
- Comprehensive metrics collection supporting the PR's goal of improved build details views
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (1)
74-74: Method call updated to retrieve complete build agent informationThe change from
getBuildAgentInformationWithoutRecentBuildJobs()togetBuildAgentInformation()aligns with the PR's objective of improving how build job information is retrieved. This modification ensures that all build agent information, including details about recent build jobs, is now included in the websocket messages.src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html (2)
5-21: Good use of conditional rendering with @if directiveYou've correctly implemented the new Angular syntax with
@ifto conditionally render the span selector UI elements. This follows the coding guidelines to use the new control flow directives instead of the older*ngIfsyntax.
76-87: Properly implemented conditional display of missing builds statisticsThe conditional rendering of the missing builds statistics section using
@ifis correctly implemented and aligns with the modern Angular syntax guidelines. This change improves the component's flexibility by allowing it to adapt its UI based on the context in which it's used.src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts (1)
9-9: Added import for BuildAgentInformation modelThis import addition is appropriate and aligns with the changes to how build agent information is being handled throughout the application. This change ensures that test files correctly reference the updated model structure.
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html (2)
43-66: Properly implemented conditional rendering for build agent filteringThe conditional section for filtering by build agent address is correctly implemented using the
@ifdirective. The typeahead functionality is well integrated with appropriate event handling for focus, click, and change events.
75-106: Date range filtering with validationThe date range filtering section is well-implemented with proper error handling. The use of the custom date-time picker component with validation feedback provides a good user experience. The error messages are internationalized correctly.
src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts (2)
16-16: Import path correctly updated to reflect component reorganizationThe import path for
FinishedBuildJobFilterhas been updated to reference its new location in the dedicatedfinished-builds-filter-modalcomponent. This aligns with the PR objective of transitioning the filtering modal to a separate component.
18-18: Import path updated to use absolute pathThe import for
BuildLogEntryhas been updated from a relative path to an absolute path, following Angular best practices for imports. This makes the code more maintainable if files are moved.src/main/webapp/app/entities/programming/build-agent-information.model.ts (3)
4-4: Added required import for dayjsThe dayjs import is correctly added to support the date properties in the new BuildAgentDetails class.
19-20: Added buildAgentDetails property to BuildAgentInformationThe new optional property correctly implements the DTO extension mentioned in the PR objectives to incorporate new information about build agents.
22-32: New BuildAgentDetails class to encapsulate build agent metricsThis class properly encapsulates various metrics related to build agent performance and status, following the TypeScript naming conventions with PascalCase for the class name and camelCase for properties. The class enables better organization of build agent statistics as described in the PR objectives.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
246-257: Simplified build agent information update logicThe method has been significantly simplified to only update the BuildAgentInformation without modifying recentBuildJobs. This aligns with the PR objective of refactoring build agent information logic into a separate service. The responsibility for managing recent build jobs has likely been moved to the new BuildAgentInformationService.
src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts (4)
16-17: Changed mockActivatedRoute from const to letThis change allows the mock to be reassigned between tests, improving test isolation and flexibility.
53-54: Added mock reset for getBuildJobStatisticsForCourseProperly clearing the mock between tests prevents test interference and ensures accurate assertions about method call counts.
🧰 Tools
🪛 Biome (1.9.4)
[error] 51-54: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
57-58: Simulated route context in testsSetting mockActivatedRoute.url before calling ngOnInit ensures the component has the correct route context during initialization, which is important for testing route-dependent behavior.
Also applies to: 68-69, 79-81
90-102: Added test for input-based statisticsThis new test properly verifies that the component can accept statistics directly as input without calling service methods. It ensures the component correctly updates its internal state based on input properties, which supports the improved component composition mentioned in the PR objectives.
src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts (4)
17-18: Looks good importing the mock classes for modal tests.
This approach of importingMockNgbModalServiceandNgbModalRefis standard to test modal functionalities.
113-113: No issue with definingmodalServicehere.
Declaringlet modalService: NgbModal;clearly separates the door for modal testing.
122-122: MockingNgbModal: Correct usage.
Providing theMockNgbModalServiceunderNgbModalis a clean way to isolate modal behavior in unit tests.
132-132: InjectingNgbModalviaTestBedis acceptable.
This ensures direct test control over modal interactions.src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts (3)
1-69: New test suite and setup appear comprehensive.
YourbeforeEachblocks and usage ofMockProvider(NgbActiveModal)follow best practices. The test ensures the component is created and basic methods run without error.
71-122: Impressive coverage for filters logic.
The tests thoroughly verify the applied filters, local storage logic, and toggling functionality. This ensures robust coverage of key user interactions.
124-149: Validation tests are effective.
Good job testing the date and duration bounds. Explicit negative test scenarios (where values are reversed) demonstrate correctness.src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts (2)
13-80: Filter class design is clear and maintainable.
Using a map to track the active filters is a neat approach, and theaddHttpParamsmethod systematically appends each filter to request params. Consider whetherset()might better suit some appended params if uniqueness is desired, but this is not a strict requirement.
82-236: Component logic is well-structured and user-friendly.
- The typeahead mechanism is straightforward, and the approach to verifying valid date & duration ranges is well implemented.
- The explicit addition and removal of filter keys keeps the code modular and testable.
- Confirming or dismissing the modal is handled in a standard, expected manner.
Overall, this design aligns with Angular best practices.
src/main/webapp/app/localci/build-queue/build-queue.component.ts (2)
88-91: Well-structured reactive search implementation.The implementation of the search subject and loading state is clean and follows reactive programming patterns.
397-408: Modal implementation follows good Angular practices.The
openFilterModalmethod follows best practices for working with NgbModal. Nice job using componentInstance to pass data to the modal and handling the result with Promise chaining.One suggestion to improve error handling:
- .catch(() => {}); + .catch((error) => { + // Only log if it's not a dismissal (user clicked outside or pressed Escape) + if (error !== 'dismissed') { + console.error('Error in filter modal:', error); + } + });src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (4)
1-1: Good adoption of Angular's new input() signal-based API.Using Angular's modern input API improves type safety and reactivity. The updated imports reflect the modern Angular practices.
Also applies to: 30-30
45-46: Nice addition of display control flags.Adding explicit boolean flags to control UI elements makes the component more flexible and reusable across different contexts.
50-54: Excellent use of Angular's effect() for reactive updates.Using the
effect()function is an elegant solution for reactively responding to changes in thecurrentSpanproperty. It eliminates the need for manual subscription management and improves code maintainability.
84-84: Good method renaming for clarity.Renaming the method to
getBuildJobStatisticsForBuildQueueprovides better clarity about its purpose and context of use.src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java (2)
15-15: Good data structure refactoring to use BuildAgentDetailsDTO.Replacing the
recentBuildJobsparameter with the structuredBuildAgentDetailsDTOimproves encapsulation and type safety. This aligns with the PR objective of collecting more information about build agents.
25-28: Constructor implementation correctly maintains the record's immutability.The updated constructor correctly passes all fields, including the new
buildAgentDetailsfield, while maintaining the immutability of the record.src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (1)
18-20: Good test setup for modal functionality.The test setup correctly includes the necessary imports and providers for testing the modal functionality. Using MockNgbModalService is a good approach.
Also applies to: 255-255, 269-269, 276-276
src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts (12)
5-5: Updated import for BuildJob and FinishedBuildJob.The import statement has been extended to include
FinishedBuildJobalong withBuildJob. This supports the new tests for finished build jobs functionality.
18-23: Added necessary imports for new functionality.New imports for HTTP, sorting, modal services, and build queue functionality - all essential for implementing and testing the finished build jobs feature.
40-40: Mock for getFinishedBuildJobs added.The mock function for accessing finished build jobs has been added to the mock service, which is needed for the new functionality being tested.
109-128: Well-structured test request and filter constants.The request and filterOptionsEmpty constants provide consistent test data for verifying filtering functionality, with appropriate default values for pagination, sorting, and empty filters.
130-168: Comprehensive mock data for finished build jobs.The mock data includes appropriate test cases for different build job scenarios with various statuses, times, and properties needed for thorough testing.
173-177: Mock BuildQueueService correctly defined.Service mock includes all required methods for testing the component's interaction with finished build jobs.
187-188: Proper service registration in test module.BuildQueueService and NgbModal are correctly provided in the testing module setup.
198-198: Test setup handles modal service and mock responses.The test properly injects the NgbModal service and configures mock responses for the BuildQueueService methods.
Also applies to: 202-205
323-335: Test for search term triggering refresh.The test properly verifies that changing the search term correctly triggers loading finished jobs with an appropriate debounce time.
337-348: Test for build job duration calculation.The test verifies that build durations are correctly calculated and formatted for finished build jobs based on start and completion dates.
350-370: Test for filter modal value initialization.This test ensures the filter modal receives the correct initial values when opened. It comprehensively validates all relevant filter properties.
372-376: Test for build log viewing functionality.The test verifies that the correct URL is opened when viewing build logs for a specific job ID.
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (7)
27-49: Improved build agent details display.The new section provides comprehensive information about the build agent's status, times, and performance metrics, enhancing the user experience by making important information immediately visible.
51-51: Added dedicated build job statistics component.Using a dedicated component for statistics improves code organization by following the single responsibility principle and makes the statistics section reusable across different views.
53-286: Enhanced running build jobs table implementation.The refactored running build jobs table provides comprehensive information with improved visualization, sorting capabilities, and detailed build information. The implementation correctly uses the Angular
@ifdirective instead of*ngIfas per coding guidelines.
289-307: Well-designed filtering controls for finished build jobs.The filtering controls include search input, help tooltips, refresh button, and loading indicator - providing a complete and user-friendly interface for managing finished build jobs.
308-319: Improved filter button with visual feedback.The filter button changes color based on the number of applied filters, providing visual feedback to the user about the current filtering state.
322-520: Comprehensive finished build jobs table with sorting and indicators.The table implementation includes:
- Status indicators with appropriate colors
- Sortable columns
- Build log access
- Links to related entities (participations, repositories)
- Formatted timestamps and durations
This provides a complete overview of build job history with all necessary actions and information.
522-539: Added pagination for finished build jobs.Pagination improves performance and usability when dealing with large numbers of finished build jobs, with proper item count display and page navigation controls.
src/main/webapp/app/localci/build-queue/build-queue.component.html (3)
438-439: Updated filter modal opening with null safety.The changes replace direct modal reference with a method call and add a null-safe check on the filter options, preventing potential runtime errors.
443-443: Added fallback for missing filter count.Using the nullish coalescing operator (
??) provides a default value of 0 when the filter count is undefined, ensuring the UI always displays a valid number.
676-704: Added modal for displaying build logs.The build logs modal provides a clean interface for viewing logs with proper formatting, error handling when logs are unavailable, and a download option for further analysis.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (8)
76-76: Added BuildAgentInformationService dependency.The service dependency has been changed from BuildAgentSshKeyService to BuildAgentInformationService, centralizing build agent information management.
131-138: Updated constructor with new service dependency.Constructor parameters have been updated to use BuildAgentInformationService instead of BuildAgentSshKeyService, and the dependency is properly assigned to the instance variable.
231-232: Refactored build agent information update.Replaced direct manipulation of build agent information with a call to the specialized service, improving separation of concerns.
248-249: Consistent service use for build agent updates.Using the centralized service for all build agent information updates ensures consistent behavior and implementation.
295-295: Centralized error handling for build agent updates.Exception handling now uses the dedicated service to update build agent information, maintaining consistency in error scenarios.
319-319: Consistent build agent information update when adding jobs.Using the dedicated service when adding jobs to processing ensures the build agent information stays in sync with the actual job state.
374-374: Updated finished job handling with dedicated service.Build agent statistics are now updated through the dedicated service when jobs finish or fail, ensuring consistent tracking of job metrics.
Also applies to: 424-424
443-443: Consistent service usage for pause/resume operations.The pause and resume operations now use the dedicated service to update build agent information, maintaining a single source of truth for agent state.
Also applies to: 514-514
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (5)
278-288: Ensure dayjs usage is consistent.
dayjsis used to compute durations for each job. Confirm your data always includes validbuildStartDate/buildCompletionDate. An undefined date might cause runtime errors.
78-78:✅ Verification successful
Consider naming consistency for finishedBuildJobs.
The property name is descriptive; verify no conflict with similarly named arrays or methods.You can run this shell snippet to confirm there are no overshadowed variables:
🏁 Script executed:
#!/bin/bash rg -A 3 -B 3 'finishedBuildJobs'Length of output: 31650
Naming consistency verified for
finishedBuildJobs.
After reviewing the codebase, the property is used consistently across components (e.g., inbuild-agent-details.component.tsandbuild-queue.component.ts) and in related tests and i18n translation files. There is no conflict or overshadowing with similarly named arrays or methods.
66-67:❓ Verification inconclusive
Ensure error handling or user feedback for modalService injection.
When injectingmodalService, confirm that all usage paths, including error paths, provide clear user feedback or logs.
🏁 Script executed:
#!/bin/bash # Check for openFilterModal usage to assure user-friendly error handling ast-grep --pattern $'openFilterModal() { $$$ }' --jsonLength of output: 58
Action Required: Verify comprehensive error handling for modal invocations
The injection ofmodalServiceusinginject(NgbModal)appears to be set up correctly; however, our verification (via the absence of anyopenFilterModal()implementation) did not reveal dedicated error handling or explicit user feedback when modal operations fail. Please ensure that all modal invocations (e.g., viamodalService.open(...)) are either wrapped in try/catch blocks or are otherwise instrumented to log errors and provide clear user feedback in error scenarios.
- Review all modal usage sites: Confirm that any failure during modal invocations is handled gracefully (with appropriate notifications or logs).
- Add explicit error handling if missing: Consider refactoring calls to include detailed error messages for the user when modals fail to load or open.
111-126:❓ Verification inconclusive
Prevent potential race condition in concurrent search triggers.
switchMap already cancels ongoing requests, which is good, but ensure no extraneous results slip in if quick expansions or collapses occur. Testing concurrent triggers can confirm no overlap.
🏁 Script executed:
#!/bin/bash # Simulate concurrent calls to fetchFinishedBuildJobs # This is just a placeholder demonstration for how you'd test concurrency echo "Reviews concurrency test recommended within e2e or integration tests."Length of output: 148
Action: Verify concurrent search behavior via integration tests
The implementation of
switchMapcorrectly cancels prior requests, which mitigates common race conditions. However, please ensure that rapid UI interactions (such as quick expansions or collapses triggering the search) do not inadvertently allow stale results to be displayed. It is recommended to verify this behavior with targeted e2e or integration tests to confidently rule out any edge-case overlaps.
269-273:❓ Verification inconclusive
Validate presence of 'X-Total-Count' header.
TheonSuccessmethod relies onX-Total-Countfor pagination. Include handling if the header is missing, or confirm that the server always sets it.
🏁 Script executed:
#!/bin/bash # Check if server code references X-Total-Count in all relevant build job endpoints rg 'X-Total-Count'Length of output: 3041
Ensure Robust Pagination Header Handling
The current implementation in the
onSuccessmethod converts the value of the'X-Total-Count'header to a number without checking for its presence. Although many parts of the codebase follow a similar pattern, at least one component (inmetis.service.ts) provides a fallback (?? '0'). Please either add a null check/fallback (e.g., defaulting to 0) for the header in this method or confirm—with appropriate documentation—that the backend always sets this header. This clarification is important to prevent potential runtime errors if the header is unexpectedly absent.
- Location:
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(Lines 269–273)- Suggestion: Update the assignment from:
to something like:this.totalItems = Number(headers.get('X-Total-Count'));const totalCountHeader = headers.get('X-Total-Count'); this.totalItems = totalCountHeader !== null ? Number(totalCountHeader) : 0;src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (2)
127-172: Verify null safety of buildStartDate and buildCompletionDate.
Duration.between()usage can throw exceptions if these fields are null. Currently, the code expects them to be non-null. Confirm the job data is guaranteed to have those timestamps in all states (especially TIMEOUT or CANCELLED).
39-39:✅ Verification successful
Validate usage of BuildAgentSshKeyService.
Ensure that thebuildAgentSSHKeyServiceis fully utilized. It appears to only retrieve a public key. Confirm if any further usage is necessary or if other operations are missing.
🏁 Script executed:
#!/bin/bash rg 'buildAgentSSHKeyService'Length of output: 1154
Confirmed BuildAgentSshKeyService Usage
- The service is injected into
BuildAgentInformationServiceand used to retrieve the public SSH key viagetPublicKeyAsString().- Its usage in the integration test (
BuildAgentSshAuthenticationIntegrationTest) confirms that the expected public key is correctly obtained and compared.- Based on the current design and usage, no additional operations appear to be necessary.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts
Show resolved
Hide resolved
...ocalci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
Show resolved
Hide resolved
WalkthroughThis pull request introduces a new Java record, Changes
Sequence Diagram(s)sequenceDiagram
participant S as SharedQueueProcessingService
participant B as BuildAgentInformationService
participant H as Hazelcast IMap
S->>B: updateLocalBuildAgentInformation(isPaused)
B->>H: Lock IMap for local build agent data
B->>H: Update build agent metrics (calculations, status update)
H-->>B: Return updated data
B->>H: Release lock
B-->>S: Return updated build agent information
sequenceDiagram
participant U as User
participant BQ as BuildQueueComponent
participant FM as FinishedBuildsFilterModalComponent
U->>BQ: Click to open filter modal
BQ->>FM: openFilterModal() is invoked
FM-->>BQ: Return selected filter criteria
BQ->>Service: Fetch finished build jobs based on filters
Service-->>BQ: Return filtered job list
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
251-251: Consider extracting method parameter from inline constructorFor improved readability, consider extracting the new BuildAgentInformation instance creation to a separate line before putting it in the map:
- buildAgentInformation.put(buildJob.buildAgent().memberAddress(), new BuildAgentInformation(buildAgent)); + BuildAgentInformation updatedAgent = new BuildAgentInformation(buildAgent); + buildAgentInformation.put(buildJob.buildAgent().memberAddress(), updatedAgent);src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html (1)
53-64: Enhance keyboard accessibility for typeahead componentThe typeahead component would benefit from additional accessibility attributes:
<input type="text" class="form-control" [placeholder]="'artemisApp.buildQueue.filter.buildAgentAddress' | artemisTranslate" [(ngModel)]="finishedBuildJobFilter.buildAgentAddress" [ngbTypeahead]="typeaheadSearch" (focus)="focus$.next($any($event).target.value)" (click)="clickEvents($event, addressTypeahead)" (change)="filterBuildAgentAddressChanged()" #addressTypeahead="ngbTypeahead" + aria-label="Filter by build agent address" + autocomplete="off" />src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (1)
54-265: Rendering running build jobs in a data table.
Overall, the table configuration and dynamic row rendering look correct. Consider safely checking forjobTimingInfoto prevent runtime errors:- 'text-danger': row.jobTimingInfo.buildDuration > 240, + 'text-danger': row.jobTimingInfo?.buildDuration > 240,src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts (2)
128-133: Consider adding error handling for typeahead eventsWhile the current implementation works, it could benefit from error handling for the typeahead click events to prevent potential runtime errors if the value is undefined.
clickEvents($event: Event, typeaheadInstance: NgbTypeahead) { if (typeaheadInstance.isPopupOpen()) { - this.click$.next(($event.target as HTMLInputElement).value); + const target = $event.target as HTMLInputElement; + if (target && target.value !== undefined) { + this.click$.next(target.value); + } } }
138-140: Efficient implementation of unique build agent addressesUsing Set to extract unique addresses is an elegant solution. Consider memoizing this value if it's called frequently to avoid recalculating on every access.
+private _buildAgentAddresses: string[] | null = null; /** * Get all build agents' addresses from the finished build jobs. */ get buildAgentAddresses(): string[] { + if (this._buildAgentAddresses) { + return this._buildAgentAddresses; + } - return Array.from(new Set(this.finishedBuildJobs.map((buildJob) => buildJob.buildAgentAddress ?? '').filter((address) => address !== ''))); + this._buildAgentAddresses = Array.from(new Set(this.finishedBuildJobs.map((buildJob) => buildJob.buildAgentAddress ?? '').filter((address) => address !== ''))); + return this._buildAgentAddresses; }src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (1)
56-58: Consider removing redundant ngOnInit callThe
ngOnInitmethod callsgetBuildJobStatistics, which is also called by the effect setup in the constructor. This creates a duplicate call on initialization.ngOnInit() { - this.getBuildJobStatistics(this.currentSpan); }src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (1)
174-178: Evaluate performance impact of loading large maps.
Mapping the entireprocessingJobsmap into a list for filtering might become expensive for large-scale deployments. Consider alternative approaches, such as storing a secondary map keyed by agent address.src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (1)
572-572: Confirm log downloads with repeated calls.
The assertiontoHaveBeenCalledTimes(1)thentoHaveBeenCalledTimes(2)is valid. Ensure the final logic after multiple downloads is also tested (e.g., clearing states).src/main/webapp/app/localci/build-queue/build-queue.component.html (1)
676-704: New log modal integration looks solid.
This modal provides a clear, user-friendly interface for viewing and downloading build logs. Consider adding a shorter text snippet or excerpt for extremely large logs, as very large logs might impact rendering performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(10 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java(0 hunks)src/main/webapp/app/entities/programming/build-agent-information.model.ts(2 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html(2 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.scss(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(7 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html(2 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.html(2 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.scss(0 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.service.ts(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.scss(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(1 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts(7 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts(5 hunks)src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts(5 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts(6 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/webapp/app/localci/build-queue/build-queue.component.scss
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java
🧰 Additional context used
📓 Path-based instructions (5)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.javasrc/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.htmlsrc/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.htmlsrc/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.htmlsrc/main/webapp/app/localci/build-queue/build-queue.component.html
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.tssrc/main/webapp/app/localci/build-queue/build-queue.service.tssrc/main/webapp/app/entities/programming/build-agent-information.model.tssrc/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.tssrc/main/webapp/app/localci/build-queue/build-queue.component.tssrc/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.tssrc/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (118)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.scss (1)
6-8: Addition of stats-column class enhances UI layout stabilityThe new
stats-columnclass with a minimum width of 250px ensures that columns displaying statistics maintain a consistent width, improving readability and preventing layout shifts.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java (1)
184-184: Updated to use the new build agent information methodThe change from
getBuildAgentInformationWithoutRecentBuildJobs()togetBuildAgentInformation()aligns with the PR's objective of enhancing build agent information retrieval. This ensures the health check returns complete build agent data, including the newly added metrics.src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.scss (1)
1-11: Good separation of concerns for filter modal stylingThese styles have been properly moved from the build-queue component to a dedicated file for the filter modal component, following Angular's style guide recommendation for component encapsulation. The use of CSS variables for colors promotes consistency throughout the application.
src/main/webapp/app/localci/build-queue/build-queue.service.ts (1)
9-9: Updated import path for FinishedBuildJobFilterThe import statement has been correctly updated to reflect the new location of the
FinishedBuildJobFilterclass, which has been moved to its own dedicated component. This change supports better code organization and modular architecture.src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.java (2)
7-12: Well-structured DTO for build agent metricsThis new record effectively encapsulates build agent details into a clean, immutable data structure. The implementation of Serializable is appropriate for data transfer across the network.
10-11: Good practice for serialization compatibilityThe
@Serialannotation and explicit serialVersionUID declaration follow best practices for serializable classes, ensuring proper versioning for deserialization.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
246-257: Method simplification aligns with DTO restructuringThe refactored method now creates a new BuildAgentInformation instance without modifying recent build jobs, which aligns with the architectural changes introduced with BuildAgentDetailsDTO. The proper locking mechanism ensures thread safety when updating the distributed map.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (2)
74-74: Method call updated to include complete build agent informationThe change from
getBuildAgentInformationWithoutRecentBuildJobs()togetBuildAgentInformation()aligns with the refactoring strategy to simplify the API while providing more comprehensive build agent data.
126-134:❓ Verification inconclusive
Verify that filtering still works with new BuildAgentDetailsDTO
The
removeUnnecessaryInformationFromBuildAgentInformationmethod creates new BuildAgentInformation objects but doesn't explicitly handle the new buildAgentDetails field. Ensure this is intentional and doesn't result in sending unnecessarily large objects over the websocket.
🏁 Script executed:
#!/bin/bash # Check the structure of BuildAgentInformation objects being created ast-grep --pattern $'new BuildAgentInformation\\([^)]*\\)'Length of output: 57
Action: Verify BuildAgentDetailsDTO Filtering
In
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(lines 126–134), the method creates newBuildAgentInformationobjects as follows:new BuildAgentInformation(agent.buildAgent(), agent.maxNumberOfConcurrentBuildJobs(), agent.numberOfCurrentBuildJobs(), runningJobs, agent.status(), null, null)It appears that the new field—associated with
BuildAgentDetailsDTO—is being set tonull. Please verify manually that this omission is intentional and that any filtering logic forBuildAgentDetailsDTOis either handled elsewhere or is meant to exclude large details from the websocket payload.src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts (1)
23-23: Removed unused importGood cleanup by removing the ArtemisTranslatePipe from component imports since it's not being used.
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java (1)
86-86: API now returns complete build agent informationThe method call has been updated to use
getBuildAgentInformation()instead ofgetBuildAgentInformationWithoutRecentBuildJobs(), aligning with the refactoring of build agent information handling in this PR. This change ensures consistent data structure across all endpoints.src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts (1)
9-9: Simplified import reflects model changesThe import has been streamlined to just include the BuildAgentInformation model, removing no longer needed types. This aligns with the backend changes to restructure how build agent information is handled.
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html (1)
1-138: Well-structured filter modal with proper validationThe new filter modal component provides a clean, organized UI for filtering finished build jobs. The implementation includes:
- Proper validation feedback for date ranges and duration bounds
- Responsive layout with appropriate grouping of related controls
- Angular best practices (@for loops, @if conditions)
- Internationalization support via translation keys
This component successfully centralizes filtering logic that was previously scattered across the application.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html (2)
5-21: Excellent use of modern Angular syntax!The change from
*ngIfto@iffor conditional rendering of the span selector follows the recommended Angular syntax guidelines.
76-87: LGTM: Proper conditional rendering for missing builds section.The implementation correctly uses the modern
@ifdirective instead of the legacy*ngIf, which aligns with the coding guidelines.src/main/webapp/app/entities/programming/build-agent-information.model.ts (3)
4-4: Appropriate import for date handling.Adding the dayjs import is necessary for the new date properties in the BuildAgentDetails class.
19-20: Good model structure enhancement.The addition of the buildAgentDetails property and removal of the runningBuildJobs property (as mentioned in the AI summary) aligns with the PR objective of centralizing build agent information management.
22-32: Well-structured new class for build agent details.The new BuildAgentDetails class follows the TypeScript coding guidelines with proper:
- PascalCase for type name
- camelCase for properties
- Appropriate type definitions including dayjs.Dayjs for date properties
This enhancement supports the PR objective of collecting more information about build agents.
src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts (2)
16-16: Updated import path for refactored component.The import path has been correctly updated to reference the FinishedBuildJobFilter from its new location in the finished-builds-filter-modal component, supporting the PR's goal of moving the modal to a separate component.
18-18: Improved import using absolute path.Changing from a relative path to an absolute path for BuildLogEntry import improves code maintainability and readability, which is a good practice.
src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts (5)
1-12: Well-structured test imports and setup.The imports follow best practices by:
- Using MockProvider from ng-mocks for dependency mocking
- Properly importing the necessary Angular testing utilities
- Including the OwlNativeDateTimeModule for datetime functionality
This follows the test guidelines for proper dependency management.
13-69: Complete test setup and basic component validation.The test setup properly:
- Creates the test environment with MockTranslateService
- Initializes the component with a new filter instance
- Includes a basic creation test
This follows the Jest testing guidelines of providing meaningful tests with proper setup.
71-96: Good tests for core filtering functionality.These tests effectively verify:
- The component's ability to extract unique build agent addresses
- Filter count tracking functionality when adding/removing different filter types
The assertions use the specific expectation methods (toBe, toEqual) as recommended in the testing guidelines.
98-122: Thorough testing of filter state management.The tests properly verify:
- Filter application and counting functionality
- State reset behavior
These tests ensure the component's state management works correctly, which is essential for the filter modal functionality.
124-149: Comprehensive validation testing.The test cases effectively verify:
- Date range validation
- Duration filter validation
- Both valid and invalid scenarios
The assertions correctly use toBeTruthy() and toBeFalsy() per the Jest testing guidelines.
src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts (5)
17-18: Importing mock modal service.
These imports properly mock the modal functionalities in tests, ensuring isolation from real UI dependencies.
113-113: Declaring a modal service variable.
DeclaringmodalServicehere allows the test suite to manage modal instances without polluting the component’s constructor.
122-122: Providing MockNgbModalService.
Replacing the actualNgbModalwith the mock service is a best practice for fully isolated unit tests.
132-132: Injecting NgbModal in test.
Retrieving the modal service viaTestBed.injectensures proper test isolation and type safety.
253-265: Testing modal behavior.
This new test correctly verifies that the modal opens for each respective method call, improving coverage of user interactions.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (6)
4-4: Adding top-level container with spacing.
The updated Bootstrap classes enhance layout and readability. No issues found.
27-49: Introducing build agent status details.
Presenting “Status,” “Start Date,” “Latest Build Date,” “Average Build Duration,” and “Git Revision” is clear and follows a straightforward layout.
51-51: Embedding build job statistics component.
Passing in the input as[buildJobStatisticsInput]="buildJobStatistics"aligns with Angular’s one-way data flow.
289-307: Adding filter bar and loading indicator for recent builds.
The search input, button trigger, and isLoading feedback form a coherent approach for user-driven data fetching.
316-317: Utilizing nullish coalescing for filter count.
Using?? 0prevents undefined references when the filter is not yet initialized.
322-537: Building the finished jobs table.
The multi-column layout for finished jobs, status icons, and repository links is well-structured and consistent with the running jobs table.src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts (6)
16-16: Switching to a mutable route object.
Usingletinstead ofconstallows dynamic reuse ofmockActivatedRouteacross multiple tests.
53-53: Clearing the course-based stats mock.
Resetting the mock before each test iteration prevents leftover calls from polluting subsequent tests.
57-57: Setting route to 'build-queue' for test.
This properly simulates the scenario where the component reads the URL and fetches queue statistics.
68-68: Reusing route path 'build-queue'.
Again, ensures the correct path context for the test scenario.
79-79: Simulating 'build-queue' route for course stats test.
Same approach, verifying consistent route-based logic.
90-102: Verifying input-driven stats behavior.
This new test ensures that when a different route is used ('build-agent'), the component relies on the passed-in input stats rather than fetching.src/main/webapp/app/localci/build-queue/build-queue.component.ts (10)
1-1: Using the AngularinjectAPI.
Employinginjecthelps reduce boilerplate and keeps the component concise, adhering to Angular’s recommended approach in recent versions.
7-7: Importing RxJS operators for search flow.
This aligns with debouncing user input and handling asynchronous streams for build job retrieval.
12-12: Including HTTP response types.
Explicit imports forHttpErrorResponse,HttpHeaders,HttpResponseimprove clarity around request handling.
15-15: Adding bootstrap modal & pagination.
Provides quick access to NG Bootstrap’s modal and pagination features.
32-32: Referencing UI reload constants.
UI_RELOAD_TIMEis used for controlling search input debounce, optimizing user experience.
33-34: Importing new filter-related components.
Subject,Subscription,FinishedBuildJobFilter, andFinishedBuildsFilterModalComponentintegrate reactive streams with the new filter modal approach.
88-88: Initializing search subject.
ThisSubject<void>underpins the debounced search queries for finished jobs.
89-89: Tracking loading state.
isLoadingensures the UI can display a loading indicator or disable actions during async operations.
91-91: Initializing new finished job filter.
InstantiatingFinishedBuildJobFiltersets default criteria for future filtering tasks.
397-407: Opening filter modal and updating filters.
Utilizes the newFinishedBuildsFilterModalComponentto gather user preferences and reloads data upon modal closure.src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java (3)
14-15: Added buildAgentDetails parameter to BuildAgentInformation recordThe addition of the BuildAgentDetailsDTO parameter aligns with the refactoring effort to better organize build agent metrics. This change follows the java records pattern for DTOs as required by coding guidelines.
10-11: Verify communication of breaking changesAs noted in the comment, this DTO is used in shared code between core and build agent nodes. Since you're changing the structure by replacing recentBuildJobs with buildAgentDetails, ensure this breaking change is documented in release notes as recommended.
25-27:❓ Verification inconclusive
Updated constructor to include buildAgentDetails
The constructor has been correctly updated to initialize the new buildAgentDetails field from the provided agentInformation, replacing the previous reference to recentBuildJobs.
🏁 Script executed:
#!/bin/bash # Check if there are any places in the codebase still trying to access recentBuildJobs rg "\.recentBuildJobs\(\)" --type javaLength of output: 40
Constructor Update Verified – Please Confirm Manually
The updated constructor insrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.javanow correctly initializes the newbuildAgentDetailsfield from the passedagentInformation, replacing the old reference torecentBuildJobs. Our search for usages ofrecentBuildJobs()returned no results, indicating that the transition appears complete. However, because the grep output was empty and might be subject to interpretation, please manually verify that no other parts of the codebase (or tests) rely onrecentBuildJobs.
- Confirm that there are no residual references or dependencies on
recentBuildJobsin related modules.- Ensure that related tests run as expected following this update.
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts (11)
13-80: Well-structured filter class with clear separation of concernsThe
FinishedBuildJobFilterclass is well-designed with:
- Clear property definitions
- Proper HTTP parameter handling
- Helper methods for filter management
- Good validation state tracking
The implementation follows Angular coding guidelines with proper camelCase for methods and properties.
82-91: Comprehensive BuildJobStatusFilter enumThe enum definition follows PascalCase naming convention as required by coding guidelines and includes all relevant build statuses.
93-100: Translatable filter keys using enumGood practice using enum for internationalization keys, which enables consistent and maintainable translation references throughout the application.
102-108: Component declaration with proper importsThe component declaration follows Angular best practices with:
- Descriptive selector
- Appropriate imports
- Standalone component configuration
- Proper template and style references
109-117: Clean dependency injection using inject functionUsing the inject function for dependency injection follows modern Angular practices. The ViewChild decorator is properly applied to reference the typeahead component.
146-155: Well-implemented typeahead search with RxJS operationsThe typeahead implementation correctly uses RxJS operators to debounce input, handle distinct values, and process multiple event sources. This follows Angular best practices for reactive programming.
160-168: Consistent toggle implementation for status filtersThe toggle implementation correctly handles both setting and clearing of status filters. This approach maintains the consistency of the filter state.
173-179: Clean filter handling for build agent addressThe method correctly updates the filter map based on the presence of a build agent address.
184-202: Thorough date validation logicThe date filter handling includes proper validation of input dates and their relationship to each other. This ensures data integrity and prevents invalid filter combinations.
207-224: Robust duration filter validationSimilar to date validation, the duration filter implementation includes proper validation of lower and upper bounds to prevent invalid filter combinations.
229-235: Simple and effective modal handlingThe modal interaction methods are concise and follow best practices for Angular component design.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (6)
1-1: Updated imports to use Angular's signal-based reactivityImporting
effectandinputfrom Angular core enables the component to use Angular's reactive primitives, which aligns with modern Angular best practices.
30-31: Added input signal for build job statisticsUsing the new input() function from Angular follows the latest best practices for component inputs.
45-47: Added control flags for conditional UI renderingThese boolean flags enable conditional rendering of UI elements, improving the component's flexibility across different contexts.
50-54: Implemented reactive effect for span changesUsing Angular's
effect()function properly creates a reactive side effect that automatically updates statistics when the span changes.
69-79: Improved context-aware statistics loadingThe updated method now intelligently determines the source of statistics based on the route path, enhancing component reusability across different views.
81-84: Updated method documentation for clarityThe documentation correctly specifies that this method is specifically for the build queue view, improving code maintainability.
src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts (11)
5-23: Updated imports to support new functionalityThe imports have been properly extended to include new types and services needed for the enhanced functionality, following the test guidelines.
40-40: Added mock method for retrieving finished build jobsThe mock service has been correctly extended to include the new method for finished build jobs support.
109-129: Well-structured test data for pagination and filteringThe test data includes proper request parameters and filter options, enabling thorough testing of the new pagination and filtering functionality.
130-169: Comprehensive mock data for finished build jobsThe mock data is well-structured and includes all necessary fields for testing the component's handling of finished build jobs.
173-177: Complete mock service for build queue operationsThe mock service properly implements all methods needed to test the component's interactions with the build queue service.
187-188: Added required service providers for testingThe test setup now properly includes the BuildQueueService and NgbModal services needed for the component's functionality.
198-206: Proper setup of mock services before testsThe test initializes mocks with appropriate return values, enabling effective testing of component behavior.
323-336: Well-implemented test for search functionalityThe test properly verifies the search term handling, including waiting for the debounce timeout to ensure accurate testing of asynchronous behavior.
337-348: Thorough test for build duration calculationThe test verifies that build durations are correctly calculated and formatted for display.
350-370: Comprehensive modal configuration testingThe test properly verifies that the modal is opened with the correct configuration and data.
372-377: Proper testing of build log viewing functionalityThe test correctly verifies that the window.open method is called with the appropriate URL when viewing build logs.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (1)
71-90:❓ Verification inconclusive
Consider verifying concurrency lock release under error conditions.
Although thetry/finallyblock is commendable for ensuring the lock is released, consider adding a test or verification script to confirm that concurrent exceptions or lock timeouts don’t leave the agent in a partially updated or locked state.
🏁 Script executed:
#!/bin/bash # Description: Attempt to simulate error conditions during lock acquisition. # Then verify that the map remains accessible and unlock is properly performed. # This script greps for concurrency or lock usage in other relevant classes that interact with buildAgentInformation. # We'll scan the codebase for any references to lock/unlock to ensure there's no deadlock scenario. rg "buildAgentInformation\.lock|buildAgentInformation\.unlock|lock\s*\(" -A 5 -B 5Length of output: 70364
Action: Enhance Concurrency Testing for Lock Release
The current implementation correctly uses a try/finally block to ensure that the lock is released. However, the grep results indicate that while lock and unlock calls are consistently applied in the codebase, there isn’t clear evidence of dedicated tests simulating failure or timeout scenarios in this service. To fully verify that concurrent exceptions or lock acquisition errors do not leave the agent in a partially updated or locked state, please consider the following:
- Add a test case that simulates an exception occurring right after acquiring the lock (or during the put operation) to ensure the finally block consistently releases the lock.
- Optionally, check that the unlock is only called if the lock was successfully acquired, to prevent any unintended behavior if lock acquisition fails.
src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (1)
534-553: Great test coverage for the filter modal.
This new test precisely verifies that the modal instance is opened and supplied with the correct initial data. You may also want to test the modal’s dismissal or confirmation behaviors for completeness.src/main/webapp/app/localci/build-queue/build-queue.component.html (1)
438-443: Good use of nullish coalescing and safe checks for filter counts.
Switching from a direct property access to?.and?? 0effectively prevents null-pointer issues and gracefully handles zero filters.src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (11)
76-76: Use ofBuildAgentInformationServicefield is consistent.
No issues spotted. The field is immutable, respecting best practices for dependency injection.
131-132: Constructor injection is aligned with recommended DI patterns.
Replacing the old service withbuildAgentInformationServiceclarifies responsibilities.
137-137: AssigningbuildAgentInformationServiceis correct.
The assignment completes the refactoring to the new service-based approach.
231-231: Periodic update for build agent info is properly refactored.
Now callsbuildAgentInformationService.updateLocalBuildAgentInformation. This keeps data current in the cluster.
248-248: Consistent call toupdateLocalBuildAgentInformation.
Ensures the cluster sees accurate paused state.
295-295: Exception handling block updates build agent info.
Keeps node status updated when a build job is rejected.
319-319: Post-queue addition call.
Good practice to notify the cluster whenever a new job moves to local processing.
374-374: Records recently finished job in cluster.
The new methodupdateLocalBuildAgentInformationWithRecentJobcentralizes the logic.
425-425: Handles failed build updates.
Ensures the finished job state is logged in the cluster service.
444-444: Pause state update is comprehensive.
InvokingupdateLocalBuildAgentInformationcaptures paused status.
514-514: Resume state update is comprehensive.
Notifies the cluster that this build agent has resumed.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (21)
4-5: New RxJS imports for search functionality.
UsingSubject,debounceTime,switchMapis a solid approach for a reactive search.
19-20: Additional local components and model imports.
ImportingBuildJobStatisticsComponentand typed models improves clarity.
22-25: Useful shared directives and pagination components.
These imports align with standard Angular best practices for modular design.
26-33: Imports for filtering, modals, constants, and dayjs.
These external utilities enhance readability and maintain concise logic.
66-66: Using theinject(NgbModal)approach.
Clean injection style, consistent with Angular’s composable DI.
70-71: Counters and running jobs tracking.
Storing stats inbuildJobStatisticsandrunningBuildJobskeeps the component well-structured.
78-78: MaintainingfinishedBuildJobsin-state.
Facilitates direct table rendering.
81-85: Declaring FontAwesome icons.
Clear usage for UI elements, adhering to consistent naming.
88-89: Sorting and synchronization icons.
Completes icon set for user interactions.
91-97: Reactive search and filter properties.
EncapsulatingsearchSubscription,searchTerm, andfinishedBuildJobFilterfosters maintainability.
99-103: Pagination parameters for finished builds.
Implementingpredicateandascendingis consistent with the data table pattern.
111-126: Debounced search subscription.
Excellent use ofdebounceTimeandswitchMapto avoid excessive requests, improving performance.
149-149: Ensuring everything is loaded initially.
Method invocation is straightforward: it fetches agent details and sets up filter defaults.
154-156: Initializing build job filter and initial load.
Ensures data is populated in the component for the agent.
234-245: Modal-based filter UI.
Replaces local storage filters, properly separated into a dedicated modal component.
247-261:loadFinishedBuildJobs()method
Central entry point for retrieving data, further improves separation of concerns.
263-273:onSuccesscallback updates state.
Correctly updatestotalItemsand sets durations for each finished build job.
278-288: Set job durations with dayjs.
Clear and readable logic for calculating total build time.
293-304:fetchFinishedBuildJobsrequest.
Uses pagination and filtering to keep the back-end calls succinct.
309-313: Fire search events on minimal input.
Only triggering searches when the user input is 3+ chars or empty is a good optimization.
320-325: Page change handler.
Properly reloads data when pagination changes.
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java
Show resolved
Hide resolved
WalkthroughThis pull request introduces a new DTO ( Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Build Agent
participant BAS as BuildAgentInformationService
participant Hazel as HazelcastInstance
participant Info as BuildAgentInformation
Agent->>BAS: Request update (with/without recent job)
BAS->>Hazel: Lock buildAgentInformation map
Hazel-->>BAS: Lock acquired
BAS->>Info: Retrieve and update agent metrics
BAS->>Hazel: Release lock
BAS-->>Agent: Return updated BuildAgentInformation
sequenceDiagram
participant User as User
participant Comp as BuildAgentDetailsComponent
participant Serv as BuildQueueService
participant API as Backend API
User->>Comp: Enter search query
Comp->>Comp: Debounce search observable
Comp->>Serv: fetchFinishedBuildJobs(pagination, sorting)
Serv->>API: HTTP request for finished build jobs
API-->>Serv: Return jobs data
Serv-->>Comp: Provide finished build jobs
Comp->>Comp: Update buildJobStatistics and display results
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (1)
126-134: 💡 Verification agent🧩 Analysis chain
Verify the removeUnnecessaryInformationFromBuildAgentInformation method.
The
removeUnnecessaryInformationFromBuildAgentInformationmethod's implementation may need updating to handle the newbuildAgentDetailsfield that replacesrecentBuildJobs, as indicated in the PR summary.
🏁 Script executed:
#!/bin/bash # Check the BuildAgentInformation class to confirm the field changes echo "BuildAgentInformation class structure:" rg -p "record BuildAgentInformation" -A 15 src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java # Check the constructor arguments in the current code echo "Current constructor usage in removeUnnecessaryInformationFromBuildAgentInformation:" cat -n src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java | grep -A 6 "filteredBuildAgentSummary.add"Length of output: 1800
Action Required: Update handling for the new buildAgentDetails field
It appears that the BuildAgentInformation record has been updated to include a new field—buildAgentDetails—while the existing implementation of removeUnnecessaryInformationFromBuildAgentInformation continues to pass null for that parameter. To align with the PR summary (which indicates replacing recentBuildJobs with buildAgentDetails), please review and update the method as follows:
Review Constructor Usage:
The current constructor call in LocalCIQueueWebsocketService (lines 130–131) passes null for both publicSshKey and buildAgentDetails. This needs revision for buildAgentDetails to reflect its intended value or a processed version (e.g., using a dedicated cleaning method similar to removeUnnecessaryInformation for runningBuildJobs).Implement Appropriate Handling:
Determine if the new buildAgentDetails field requires processing (removing unnecessary information) similar to runningBuildJobs. If so, add the appropriate logic, and update the constructor call accordingly.Please make the necessary updates to ensure the method properly accommodates the new field.
🧹 Nitpick comments (15)
src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts (1)
254-265: Consider verifying modal argumentsThe new test for opening modals correctly verifies that the
openmethod is called. As a minor improvement, you might also consider verifying that the correct component or template is passed toopen, if relevant to the test logic.src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (1)
50-54: Avoid duplicating initialization logic.
Callingthis.getBuildJobStatistics(this.currentSpan);in both the constructor’seffectandngOnInit()can result in multiple/duplicate service calls. Verify that the repeated call is intentional.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (2)
4-25: Leverage translation keys for all labels.
Lines 29-48 show “Status”, “Start Date”, etc. as raw strings. For consistency with your i18n approach (seen in other places likejhiTranslateusage), consider applying translations for these labels.Also applies to: 27-49
289-541: UX consistency for finished vs. running jobs.
Finished and running job sections appear to have overlapping columns and behaviors. Matching presentation patterns (e.g., consistent column widths, column order) can enhance user familiarity and usability. Consider harmonizing them or consolidating logic.src/main/webapp/app/localci/build-queue/build-queue.component.ts (1)
1-33: Consolidate imports for readability.
Multiple new imports (e.g.,NgbModal,HttpHeaders,Subscription) could be grouped or imported selectively to streamline the top section.src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts (2)
13-80: Good implementation of the filter class, but consider optimizing memory usageThe
FinishedBuildJobFilterclass is well-structured with clear methods for tracking and applying filters. The constructor approach with optional build agent address is good for reusability.However, the filter map approach could be optimized:
- numberOfAppliedFilters = 0; - appliedFilters = new Map<string, boolean>(); + appliedFilters = new Map<string, boolean>(); + + get numberOfAppliedFilters(): number { + return this.appliedFilters.size; + }Then update the related methods:
- addFilterToFilterMap(filterKey: string) { - if (!this.appliedFilters.get(filterKey)) { - this.appliedFilters.set(filterKey, true); - this.numberOfAppliedFilters++; - } - } + addFilterToFilterMap(filterKey: string) { + this.appliedFilters.set(filterKey, true); + } - removeFilterFromFilterMap(filterKey: string) { - if (this.appliedFilters.get(filterKey)) { - this.appliedFilters.delete(filterKey); - this.numberOfAppliedFilters--; - } - } + removeFilterFromFilterMap(filterKey: string) { + this.appliedFilters.delete(filterKey); + }This would simplify the logic and eliminate potential state inconsistencies between the map size and the counter.
138-140: Improve error handling when no build jobs are availableThe
buildAgentAddressesgetter assumesfinishedBuildJobsis populated. You should add defensive code to handle empty arrays or undefined values.get buildAgentAddresses(): string[] { + if (!this.finishedBuildJobs?.length) { + return []; + } return Array.from(new Set(this.finishedBuildJobs.map((buildJob) => buildJob.buildAgentAddress ?? '').filter((address) => address !== ''))); }src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts (3)
4-23: Follow Jest testing conventions for improved clarityThe imports have been appropriately updated to support the new functionality. However, to better follow Jest testing best practices:
- import { BuildJob, FinishedBuildJob } from 'app/entities/programming/build-job.model'; + // Prefer explicit imports for clarity in tests + import { BuildJob } from 'app/entities/programming/build-job.model'; + import { FinishedBuildJob } from 'app/entities/programming/build-job.model';
323-336: Use more specific assertion for testing debounced searchThe test for search term changes is good but could use more specific assertions to verify proper debounce behavior.
it('should trigger refresh on search term change', async () => { mockBuildAgentsService.getBuildAgentDetails.mockReturnValue(of(mockBuildAgent)); component.ngOnInit(); component.searchTerm = 'search'; + // Spy on the actual method that makes the API call + const loadSpy = jest.spyOn(component, 'loadFinishedBuildJobs'); component.triggerLoadFinishedJobs(); const requestWithSearchTerm = { ...request }; requestWithSearchTerm.searchTerm = 'search'; // Wait for the debounce time to pass await new Promise((resolve) => setTimeout(resolve, 110)); + // Verify debounce worked - method should be called exactly once + expect(loadSpy).toHaveBeenCalledTimes(1); expect(mockBuildQueueService.getFinishedBuildJobs).toHaveBeenNthCalledWith(2, requestWithSearchTerm, filterOptionsEmpty); });
350-370: Add edge case testing for filter modal valuesThe test for initializing filter modal values is good, but should include tests for edge cases such as when
buildAgentis undefined or doesn't have a member address.it('should correctly set filterModal values', () => { const modalRef = { componentInstance: { finishedBuildJobFilter: undefined, buildAgentAddress: undefined, finishedBuildJobs: undefined, }, result: Promise.resolve('close'), } as NgbModalRef; const openSpy = jest.spyOn(modalService, 'open').mockReturnValue(modalRef); component.finishedBuildJobs = mockFinishedJobs; component.buildAgent = mockBuildAgent; component.finishedBuildJobFilter = new FinishedBuildJobFilter(mockBuildAgent.buildAgent?.memberAddress!); component.openFilterModal(); expect(openSpy).toHaveBeenCalledTimes(1); expect(modalRef.componentInstance.finishedBuildJobFilter).toEqual(filterOptionsEmpty); expect(modalRef.componentInstance.buildAgentAddress).toEqual(mockBuildAgent.buildAgent?.memberAddress); expect(modalRef.componentInstance.finishedBuildJobs).toEqual(component.finishedBuildJobs); }); +it('should handle missing buildAgent when setting filterModal values', () => { + const modalRef = { + componentInstance: { + finishedBuildJobFilter: undefined, + buildAgentAddress: undefined, + finishedBuildJobs: undefined, + }, + result: Promise.resolve('close'), + } as NgbModalRef; + const openSpy = jest.spyOn(modalService, 'open').mockReturnValue(modalRef); + component.finishedBuildJobs = mockFinishedJobs; + component.buildAgent = { ...mockBuildAgent, buildAgent: undefined }; + component.finishedBuildJobFilter = new FinishedBuildJobFilter(); + + component.openFilterModal(); + + expect(openSpy).toHaveBeenCalledTimes(1); + expect(modalRef.componentInstance.buildAgentAddress).toBeUndefined(); +});src/main/webapp/app/localci/build-queue/build-queue.component.html (1)
676-704: Improve accessibility of the build logs modalThe build logs modal is well-structured but could benefit from improved accessibility attributes.
<!-- Modal --> -<ng-template #buildLogsModal let-modal> +<ng-template #buildLogsModal let-modal> <div class="modal-header"> - <h5 class="modal-title"> + <h5 class="modal-title" id="build-logs-modal-title"> <span jhiTranslate="artemisApp.buildQueue.logs.title"></span> <span> {{ this.displayedBuildJobId }}</span> </h5> - <button type="button" class="btn-close" aria-label="Close" (click)="modal.dismiss()"></button> + <button type="button" class="btn-close" aria-label="Close" (click)="modal.dismiss()" aria-labelledby="build-logs-modal-title"></button> </div> - <div class="modal-body"> + <div class="modal-body" aria-live="polite"> <table class="table table-borderless"> <tbody> @if (rawBuildLogsString) { <pre>{{ rawBuildLogsString }}</pre> } @else { <span jhiTranslate="artemisApp.buildQueue.logs.noLogs"></span> } </tbody> </table> </div> <div class="modal-footer"> - <button type="button" class="btn btn-secondary" (click)="modal.close()"> + <button type="button" class="btn btn-secondary me-2" (click)="modal.close()"> <span jhiTranslate="artemisApp.buildQueue.filter.close"></span> </button> <button class="btn btn-primary" (click)="downloadBuildLogs()"> + <fa-icon [icon]="faDownload" class="me-1"></fa-icon> <span jhiTranslate="artemisApp.buildQueue.logs.download"></span> </button> </div> </ng-template>src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (1)
534-553: Good test for filter modal initialization but can be more comprehensiveThe test correctly verifies that the modal is initialized with the proper values, but could be enhanced to verify the behavior when the modal is closed with a result.
it('should correctly set filterModal values', () => { const modalRef = { componentInstance: { finishedBuildJobFilter: undefined, buildAgentAddress: undefined, finishedBuildJobs: undefined, }, result: Promise.resolve('close'), } as NgbModalRef; const openSpy = jest.spyOn(modalService, 'open').mockReturnValue(modalRef); component.finishedBuildJobs = mockFinishedJobs; component.finishedBuildJobFilter = new FinishedBuildJobFilter(); component.openFilterModal(); expect(openSpy).toHaveBeenCalledTimes(1); expect(modalRef.componentInstance.finishedBuildJobFilter).toEqual(filterOptionsEmpty); expect(modalRef.componentInstance.finishedBuildJobs).toEqual(component.finishedBuildJobs); expect(modalRef.componentInstance.buildAgentFilterable).toEqual(true); + + // Test modal result handling + const updatedFilter = new FinishedBuildJobFilter(); + updatedFilter.status = 'SUCCESSFUL'; + modalRef.result = Promise.resolve(updatedFilter); + + return modalRef.result.then((result) => { + expect(component.finishedBuildJobFilter).toEqual(updatedFilter); + expect(component.loadFinishedBuildJobs).toHaveBeenCalled(); + }); });src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (2)
230-231: Duplicate check for missing build agent info.
This logic appears in multiple methods (also at lines [248-249], [295], [315], [319], [444], [514]). Consider refactoring into a helper method to avoid duplication and centralize this check:+private void updateBuildAgentInfoIfAbsent() { + if (!buildAgentInformation.containsKey(hazelcastInstance.getCluster().getLocalMember().getAddress().toString())) { + buildAgentInformationService.updateLocalBuildAgentInformation(isPaused.get()); + } +}
374-375: Ensure robust error handling for recent job info update.
While this is correct, consider try-catch or logging in case the update fails.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (1)
181-181: Opening build log in new tab.
Consider using AngularRouteronly if you want an in-app route; otherwise, this is fine.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(10 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java(0 hunks)src/main/webapp/app/entities/programming/build-agent-information.model.ts(2 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html(2 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.scss(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(7 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html(2 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.html(2 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.scss(0 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.service.ts(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.scss(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(1 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts(7 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts(5 hunks)src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts(5 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts(6 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java
- src/main/webapp/app/localci/build-queue/build-queue.component.scss
🧰 Additional context used
📓 Path-based instructions (5)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.javasrc/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.tssrc/main/webapp/app/entities/programming/build-agent-information.model.tssrc/main/webapp/app/localci/build-queue/build-queue.service.tssrc/main/webapp/app/localci/build-queue/build-queue.component.tssrc/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.tssrc/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.tssrc/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.htmlsrc/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.htmlsrc/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.htmlsrc/main/webapp/app/localci/build-queue/build-queue.component.html
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (70)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.scss (1)
6-8: Good addition of the stats-column class.The new
stats-columnclass with a minimum width ensures consistent display of statistical information, preventing narrow columns that could compromise readability of build agent metrics.src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts (1)
23-23:❓ Verification inconclusive
Verify the removal of ArtemisTranslatePipe.
The
ArtemisTranslatePipehas been removed from the component imports, but verify that:
- The pipe isn't used in the component's template
- Translation functionality is still properly handled by the remaining
TranslateDirective
🏁 Script executed:
#!/bin/bash # Check if ArtemisTranslatePipe is used in the component's template cat src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.html | grep -E "translateNumber|translateText|translate:"Length of output: 180
Action Required: Confirm complete removal of ArtemisTranslatePipe and proper translation handling
- The initial grep from the component’s HTML did not return any matches for ArtemisTranslatePipe-related bindings (e.g.,
translateNumber,translateText,translate:). However, since the output was inconclusive, please manually verify the following:
- Ensure that ArtemisTranslatePipe is not referenced in any part of the component (both the HTML template and the TypeScript file).
- Confirm that translation functionality is fully handled by the remaining TranslateDirective.
Once you have performed a manual check or additional scripts if needed, please update or confirm that no further changes are necessary.
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.scss (1)
1-11: LGTM: Consistent styling for the filter modal.The new styling classes provide consistent visual presentation for the finished builds filter modal:
- Border styling to visually separate sections
- Background color for visual hierarchy
- Width constraint for form labels ensures consistent alignment
These styles align with Angular guidelines for responsive layout and maintaining visual consistency.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (1)
74-74:❓ Verification inconclusive
Update method to use complete build agent information.
The method now retrieves complete build agent information instead of filtering out recent build jobs. This change aligns with the refactoring of build agent information handling described in the PR objectives.
🏁 Script executed:
#!/bin/bash # Verify that getBuildAgentInformationWithoutRecentBuildJobs() has been removed from SharedQueueManagementService and that all references have been updated echo "Checking if old method still exists in SharedQueueManagementService:" rg -p "getBuildAgentInformationWithoutRecentBuildJobs" src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java echo "Checking for any remaining references to the old method:" rg -p "getBuildAgentInformationWithoutRecentBuildJobs" --type javaLength of output: 490
Action Required: Verify Usage of Complete Build Agent Information
- The code now calls
sharedQueueManagementService.getBuildAgentInformation(), and references to the deprecatedgetBuildAgentInformationWithoutRecentBuildJobs()method are no longer found in the codebase.- In
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(line 74), the call remains:var buildAgentSummary = removeUnnecessaryInformationFromBuildAgentInformation(sharedQueueManagementService.getBuildAgentInformation());- Please manually confirm that the helper method (
removeUnnecessaryInformationFromBuildAgentInformation) has been updated (or is no longer filtering out recent build jobs) to align with the PR objective of using complete build agent information.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java (1)
183-184: Updated build agent information retrieval methodThe change from using a filtered build agent information method to retrieving complete build agent information aligns with the centralization of build agent update logic described in the PR objectives. This provides more comprehensive health information.
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java (1)
85-86: Consistent method call update for build agent informationThe endpoint now retrieves complete build agent information instead of filtering out recent build jobs, which is consistent with the changes in
LocalCIService. This update supports the refactoring described in the PR objectives.src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.java (1)
1-12: Well-structured DTO for build agent metricsThe new
BuildAgentDetailsDTOrecord provides a clean, immutable representation of build agent metrics. It follows Java best practices by:
- Implementing
Serializablefor data transfer- Including a proper
serialVersionUIDwith the@Serialannotation- Using appropriate data types for each field
The record contains comprehensive build metrics that support the PR objective of collecting more detailed information about build agents.
src/main/webapp/app/localci/build-queue/build-queue.service.ts (1)
9-9: Updated import path for FinishedBuildJobFilterThe import has been updated to reflect the new component location for
FinishedBuildJobFilter, supporting the PR objective of moving the filtering modal to a separate component. This change improves code organization and maintainability.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
246-257: Simplification of build agent information updateThe method has been simplified to just create a new BuildAgentInformation instance without modifying any recent build jobs list. This change aligns with the refactoring of BuildAgentInformation to use a separate BuildAgentDetailsDTO for metrics instead of maintaining the job list directly.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html (2)
5-21: Correctly implemented conditional rendering using @if directiveGood implementation of conditional rendering for the span selector UI elements using the new Angular
@ifdirective, which properly follows the coding guidelines that recommend using@ifover the older*ngIfsyntax.
76-87: Correctly implemented conditional rendering for missing builds statisticsThe missing builds statistics section now uses the
@ifdirective for conditional display, following the Angular coding guidelines. This improves the component's flexibility by showing this section only when relevant.src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
99-99: Test updated to match refactored BuildAgentInformation structureThe test setup has been properly updated to align with the changes in the BuildAgentInformation class structure, where the list of jobs now only includes job1. This ensures that the test correctly reflects the current implementation.
src/main/webapp/app/entities/programming/build-agent-information.model.ts (3)
4-4: Added dayjs import for date handlingThe import of dayjs is appropriate for handling the date properties in the new BuildAgentDetails class.
19-20: Updated BuildAgentInformation with buildAgentDetails propertyThe BuildAgentInformation class now includes a reference to the BuildAgentDetails object instead of managing running build jobs directly, which aligns with the server-side changes to improve encapsulation of build agent metrics.
22-32: Well-structured BuildAgentDetails class with appropriate propertiesThe new BuildAgentDetails class properly encapsulates all the metrics related to build agents, following the TypeScript coding guidelines with PascalCase for the class name and camelCase for properties. The class includes comprehensive metrics such as build counts by status, durations, and timestamps.
src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts (2)
16-16: Import path updated for FinishedBuildJobFilterThe import path for
FinishedBuildJobFilterhas been updated to reflect its new location in the dedicated filter modal component, aligning with the PR objective of moving the filtering functionality to a separate component.
18-18: Import simplified for BuildLogEntryThe import for
BuildLogEntryhas been updated to use a direct path rather than a relative path, making it clearer where this model is located.src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts (3)
16-16: Variable declaration changed from const to letChanging
mockActivatedRoutefrom a constant to a variable allows it to be modified across different test cases, which is necessary for the updated tests.
57-57: URL path set for activated route in testsSetting the
urlproperty in themockActivatedRouteto simulate navigation to the 'build-queue' path helps test the component's behavior in a specific routing context, ensuring the component reacts correctly to different URL paths.Also applies to: 68-68, 79-79
90-102: New test case for input-based statisticsGood addition of a test case that verifies the component uses statistics provided through the input property instead of calling service methods. The test correctly checks that service methods aren't called and that display flags are properly set.
The test follows Angular testing best practices by:
- Setting the necessary input via
setInput- Using precise expectations with
toHaveBeenCalledTimes(0)andtoEqual()- Verifying both the data and UI configuration properties
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html (6)
1-7: Well-structured modal header with proper close functionalityThe modal header follows UI best practices with a clear title and properly implemented close button.
20-32: Modern Angular syntax used for iterationExcellent use of the new
@forsyntax instead of the deprecated*ngForas recommended in the coding guidelines. The radio button implementation for build status filtering is clean and well-organized.
43-66: Conditional build agent filter with modern syntaxGood use of the new
@ifdirective instead of*ngIfas recommended in the coding guidelines. The typeahead functionality for the build agent address filter enhances usability.
76-104: Well-implemented date range filter with validationThe date range filter implementation includes proper validation feedback when dates are invalid and uses the date-time picker component correctly. The use of
@iffor conditional error message display follows modern Angular practices.
117-124: Duration filter with validationThe build duration filter is well-implemented with appropriate validation for the lower and upper bounds. The error message is conditionally displayed using the modern
@ifsyntax as per coding guidelines.
130-137: Clear action buttons with proper translationsThe modal footer contains clearly labeled action buttons with appropriate translations and button styling according to their actions.
src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts (1)
9-9: Import simplified for BuildAgentInformationThe import for
BuildAgentInformationhas been updated, reflecting the refactoring of build agent logic mentioned in the PR objectives.src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts (1)
1-151: Well-structured test coverageAll tests thoroughly validate the filter logic, date boundaries, and local storage functionality. The usage of mocked data and dayjs is appropriate, ensuring multiple scenarios are covered. This test suite provides impressive coverage and clear, maintainable test cases.
src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts (1)
17-18: Good use of MockNgbModalServiceInjecting the mocked modal service aligns well with Angular test best practices. Initializing
modalServiceand providing it under test ensures consistent, isolated tests without depending on the real modal implementation.Also applies to: 113-113, 122-122, 133-133
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java (1)
15-27: Record structure and copy constructor look goodThe updated record fields and constructor follow a clear, minimal, single-responsibility approach in line with DTO best practices. The copy constructor properly replicates existing data. No issues found.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (1)
174-178: Potential performance caveat with large job setsRetrieving all values from the
processingJobsmap and then streaming to filter bymemberAddressmay become costly if the map grows large. Given the note about potential issues with streams on IMaps, confirm that the data volume is manageable or consider a more direct approach to reduce overhead.Please review how many build jobs can accumulate in
processingJobs. If there’s a risk of large-scale data, consider a specialized query or partitioning strategy in Hazelcast.src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (3)
30-30: Validate property initialization.
Double-check the generic type is correctly recognized by your version of Angular. If you keep this pattern, confirm that the property is properly assigned before used.
45-46: Check initial UI states.
Ensure thatdisplaySpanSelector = trueanddisplayMissingBuilds = trueprovide the desired default behavior, especially if the user’s initial route doesn't require these flags.
82-84: Good clarity in renaming.
Renaming the method togetBuildJobStatisticsForBuildQueueimproves readability and clarifies its context.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (1)
51-52: Proactively check input usage.
Ensure that the[buildJobStatisticsInput]binding receives correct data in the parent component. If the data is never updated, consider a static property or a simpler approach.src/main/webapp/app/localci/build-queue/build-queue.component.ts (2)
88-92: Ensure minimal re-renders from additional states.
Addingsearch,isLoading,finishedBuildJobFilter, etc. introduces new states. Confirm none cause unnecessary re-renders or overshadow Angular’s usual lifecycle approach, especially with websockets in play.
397-407: Modal approach is a cleaner design.
Opening a dedicated filter modal is more intuitive than inline filtering. Good step toward better maintainability.src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts (1)
109-128: LGTM! Request parameters and filter options are well-definedThe constant definitions provide clear test data structures that match the expected API interfaces.
src/main/webapp/app/localci/build-queue/build-queue.component.html (1)
438-444: Good use of optional chaining for robust template renderingThe changes to use optional chaining with
finishedBuildJobFilter?.numberOfAppliedFiltersand the null-coalescing operator in the translation values provide protection against undefined errors and improve template resilience.src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (2)
18-20: Import organization follows Angular best practicesThe updated imports are well-organized and focused on the new functionality.
572-573: Good test assertion update for Jest best practicesUpdating to
toHaveBeenCalledTimes(1)follows Jest best practices more closely thantoHaveBeenCalledOnce().src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (9)
76-76: New service field injection looks fine.
No issues found with this field’s naming and usage.
131-132: Use of constructor injection is consistent with best practices.
InjectingBuildAgentInformationServicethrough the constructor follows the single-responsibility and DI guidelines.Also applies to: 137-137
248-249: Same duplication of missing build agent info check.
295-295: Same duplication of missing build agent info check.
315-315: Same duplication of missing build agent info check.
319-319: Same duplication of missing build agent info check.
424-425: Same call to update with recent job info.
Likely the same considerations apply as in lines [374-375].
444-444: Same duplication of missing build agent info check.
514-514: Same duplication of missing build agent info check.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (20)
4-5: New RxJS and FontAwesome imports appear correct.
These imports support search debouncing and icon usage in line with Angular style guidelines.
19-31: Additional imports for build job statistics, filtering, pagination, etc.
No star imports are used; each import is explicit. This aligns with best practices.
66-66: Injecting modal service is appropriate.
This properly injectsNgbModalwithout using a global reference.
70-71: Typed property initialization for build job data.
DeclaringbuildJobStatisticsandrunningBuildJobswith explicit types is good for clarity.
78-78: Finished build jobs tracked in a typed array.
Likewise, specifyingFinishedBuildJob[]is clear and consistent.
81-85: FontAwesome icon references.
These constants are appropriately declared, matching Angular style guidelines.Also applies to: 88-90
91-91: No functional change in this comment line.
92-98: Properties for search logic, filter, and loading states.
Introducing a debounced search approach withSubjectis a solid pattern.
99-103: Pagination and sorting properties introduced.
These fields align with typical pagination logic in Angular.
112-126: Debounced search subscription with error handling.
Good implementation pattern usingdebounceTime,tap, andswitchMap.
148-148: Documenting the load method.
Helpful doc comment; no issues.
150-156: Filter creation and finished build jobs loading.
ConstructingFinishedBuildJobFilterbefore loading jobs is logical.
160-168: Populating local statistics from buildAgent details.
Direct assignment is simple and clear.
234-245: Modal-based approach for filter settings.
Implementation looks correct and user-friendly.
247-261: loadFinishedBuildJobs method with immediate subscription.
Explicit error handling is present; this is good.
263-273: onSuccess callback for finished build jobs.
Updates pagination info and data set accordingly.
275-289: setFinishedBuildJobsDuration method.
Time calculations appear correct and usedayjs.diffappropriately.
290-304: fetchFinishedBuildJobs method.
The method properly delegates tobuildQueueServicewith filter and pagination.
306-313: triggerLoadFinishedJobs with Subject-based search.
The threshold check (length >= 3) is a good pattern.
315-325: Handling page changes for pagination.
Reloading the finished jobs upon page change is consistent with typical pagination flows.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java
Outdated
Show resolved
Hide resolved
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts
Show resolved
Hide resolved
...alci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html
Outdated
Show resolved
Hide resolved
...p/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts
Show resolved
Hide resolved
...p/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts
Show resolved
Hide resolved
WalkthroughThe PR introduces a new Java record ( Changes
Sequence Diagram(s)sequenceDiagram
participant SQP as SharedQueueProcessingService
participant BAIS as BuildAgentInformationService
participant HZ as Hazelcast Instance
participant BA as BuildAgentInformation (record)
SQP->>BAIS: updateLocalBuildAgentInformation(isPaused)
BAIS->>HZ: Lock & retrieve buildAgentInformation map
BAIS->>BA: Construct new BuildAgentInformation (using BuildAgentDetailsDTO)
BAIS->>HZ: Update buildAgentInformation map with new record
BAIS-->>SQP: Return updated information
sequenceDiagram
participant User as User
participant BAD as BuildAgentDetailsComponent
participant API as Backend API (Finished Build Jobs)
User->>BAD: Enters search term for finished jobs
BAD->>BAD: Debounce and trigger loadFinishedBuildJobs()
BAD->>API: Request finished build jobs with filter criteria
API-->>BAD: Return finished build jobs data
BAD->>BAD: Update finishedBuildJobs and refresh UI
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (18)
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html (1)
20-32: Consider extracting radio button items to a reusable componentThe repetitive radio button structure could be extracted into a reusable component if this pattern appears elsewhere in the application.
src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts (4)
16-16: Consider specifying a more precise type.
Usinganycan risk type-safety. Suggest using a partial mock ofActivatedRouteor an explicit type with required properties.
57-57: Use a helper for repeated route mocking.
This line duplicates the route setup used elsewhere. Consider extracting a utility function to reduce repetition.
68-68: Avoid repeated route assignments.
Similar code appears in lines 57 and 79. Moving this setup logic to a shared helper orbeforeEachblock might help.
79-79: Consolidate duplicate testing code.
This route assignment is repeated across multiple tests. A single shared approach would increase maintainability.src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts (1)
76-96: Detailed filter count verification.
This thoroughly checks the logic for toggling duration, date, and status filters. Consider adding boundary-edge scenarios as well.src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts (1)
13-80: Filter class implementation.
- Tracking applied filters in a map is efficient.
- Suggest testing edge cases like negative or extremely large duration bounds.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (3)
30-30: Align input property name with Angular conventions.
Typically, Angular uses@Input() somePropertyinstead of the separateinput<T>()function. If you need to keep the signal-based approach, consider renaming to something likebuildJobStatisticsSignalor simplybuildJobStatisticsfor uniformity.
45-46: Consider storing flags in a unified object.
displaySpanSelectoranddisplayMissingBuildscan both be descriptive boolean flags, but consider grouping them into a state object or using a single display-mode property if they are closely related. This can improve readability and maintainability.
69-79: Check for safe indexing when accessing URL segments.
url[0]may be undefined if the route is not defined or has fewer segments. Ensure safe checks are in place.if (url.length > 0 && url[0].path === 'build-queue') { this.getBuildJobStatisticsForBuildQueue(span); } else { this.displayMissingBuilds = false; this.displaySpanSelector = false; this.updateDisplayedBuildJobStatistics(this.buildJobStatisticsInput()!); }src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (3)
67-69: Consolidate update methods.
updateLocalBuildAgentInformation(boolean isPaused)is a wrapper that callsupdateLocalBuildAgentInformationWithRecentJob(null, isPaused). If there's minimal difference between them, consider combining them to simplify the code.
112-125: Update method docstrings to clarify partial context.
Methods likegetBuildAgentDetailsrely onagentpossibly beingnull, which indicates you are merging data from an older record. Document how the final result is formed whenagentorrecentBuildJobare missing, for future maintainers.
127-178: Expand test coverage for aggregations & historical data.
You’re computing aggregated fields (e.g., averageBuildDuration, successful/fail counts) by incrementally adding new results. Ensure you have tests covering edge cases like zero total builds, multiple consecutive updates, and correct average computations.Do you need help writing additional tests to confirm these calculations over multiple updates?
src/main/webapp/app/localci/build-queue/build-queue.component.ts (1)
397-407: Modal error handling
Using.then()and a.catch(() => {})on the modal result is fine, but any errors are silently ignored. Consider logging or alerting these errors for better debugging.modalRef.result .then((result: FinishedBuildJobFilter) => { this.finishedBuildJobFilter = result; this.loadFinishedBuildJobs(); }) - .catch(() => {}); + .catch((error) => { + console.warn('Filter modal closed with error:', error); + });src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts (2)
40-40: Mocking finished build jobs
ThemockBuildAgentsService.getFinishedBuildJobsstub returning an empty object by default may limit test coverage. Consider returning more realistic data in a default scenario to expand coverage depth.-getFinishedBuildJobs: jest.fn().mockReturnValue(of({})), +getFinishedBuildJobs: jest.fn().mockReturnValue(of(mockFinishedJobsResponse)),
203-205: Stub returns
Returning empty objects or default success from stubs is fine as a baseline, but consider negative test scenarios for more robust coverage.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (2)
27-49: Localize the newly added labels.These labels (e.g., "Status:", "Start Date", "Latest Build Date", etc.) appear to be unlocalized. To maintain consistency with your use of
jhiTranslateelsewhere, it’s good practice to localize them for multi-language support.Example diff for localizing “Status:”:
- <span class="bold">Status:</span> + <span class="bold" jhiTranslate="artemisApp.buildAgents.statusLabel"></span>
81-85: Consider extracting the magic number into a constant.You’re checking
row.jobTimingInfo.buildDuration > 240directly. Using 240 as a named constant (e.g.,MAX_ALLOWED_DURATION) clarifies intent.- 'text-danger': row.jobTimingInfo.buildDuration > 240, + 'text-danger': row.jobTimingInfo.buildDuration > MAX_ALLOWED_DURATION,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(10 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java(0 hunks)src/main/webapp/app/entities/programming/build-agent-information.model.ts(2 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html(2 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.scss(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(7 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html(2 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.html(2 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.scss(0 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.service.ts(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.scss(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(1 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts(7 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts(5 hunks)src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts(5 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts(6 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java
- src/main/webapp/app/localci/build-queue/build-queue.component.scss
🧰 Additional context used
📓 Path-based instructions (5)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.javasrc/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.tssrc/main/webapp/app/localci/build-queue/build-queue.service.tssrc/main/webapp/app/entities/programming/build-agent-information.model.tssrc/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.tssrc/main/webapp/app/localci/build-queue/build-queue.component.tssrc/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.tssrc/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.htmlsrc/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.htmlsrc/main/webapp/app/localci/build-queue/build-queue.component.htmlsrc/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.tssrc/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (98)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.scss (1)
6-8: Good addition for consistent layout.Adding the
.stats-columnclass with a minimum width ensures that statistics data remains readable and consistently laid out, preventing layout shifts when data changes.src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.scss (1)
1-11: Clean organization of styles in the new component.This new SCSS file properly encapsulates the styles specific to the filter modal component. The use of CSS variables for colors aligns with best practices for maintaining a consistent theme.
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts (1)
23-23:❓ Verification inconclusive
Verify template doesn't use ArtemisTranslatePipe.
The
ArtemisTranslatePipehas been removed from the component imports. Ensure that the corresponding template doesn't use this pipe, or you'll encounter runtime errors.
🏁 Script executed:
#!/bin/bash # Check if the template uses the ArtemisTranslatePipe grep -n "translateArtemis" app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.htmlLength of output: 271
Action: Verify Inline Template for ArtemisTranslatePipe Usage
It looks like the external template file (
create-exercise-unit.component.html) was not found—this suggests the component may be using an inline template. Please manually verify that the inline template defined in the component (in the filesrc/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts) does not referenceArtemisTranslatePipe(or any similar identifiers such astranslateArtemis), as that could cause runtime errors.
- Confirm whether the component utilizes an inline template (look for a
template:property) in the TypeScript file.- Ensure no usage of
ArtemisTranslatePipeexists within that inline template.src/main/webapp/app/localci/build-queue/build-queue.service.ts (1)
9-9: Good refactoring of import path.The updated import path correctly reflects the new location of
FinishedBuildJobFilter, which has been moved to its own dedicated component. This change aligns with the overall refactoring to improve code organization.src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.java (2)
7-8: Good use of Java record for the DTO implementation.The
BuildAgentDetailsDTOrecord is well-structured and follows the coding guidelines for DTOs (using Java records). It appropriately implementsSerializablefor transferring data across the network and includes all necessary fields for build agent metrics.
10-11: Serial version UID is correctly implemented.Including the
@Serialannotation and serialVersionUID follows best practices for serializable classes.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java (1)
184-184: Updated method call reflects refactored service.The change from
getBuildAgentInformationWithoutRecentBuildJobs()togetBuildAgentInformation()aligns with the PR's objective of refactoring build agent information handling. Thehealth()method now returns complete build agent information rather than filtered information.src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
99-99: Test updated to match refactored BuildAgentInformation constructor.The constructor call has been updated to pass
nullinstead of a list containingjob2, reflecting the changes to theBuildAgentInformationrecord structure wherebuildAgentDetailshas replacedrecentBuildJobs.src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html (2)
5-21: Proper use of Angular's modern conditional rendering.The span selector section is now correctly rendered conditionally using
@ifinstead of the older*ngIfdirective as specified in the coding guidelines. This improves flexibility by showing these controls only when needed.
76-87: Conditionally displaying missing builds section follows modern Angular practices.Using
@ifto conditionally render the missing builds statistics section aligns with the coding guidelines and allows for more flexible UI based on the component's state.src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java (1)
86-86: Method call updated to align with service refactoringThe change from
getBuildAgentInformationWithoutRecentBuildJobs()togetBuildAgentInformation()aligns with the broader refactoring of build agent information handling in the PR.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (1)
74-74: Method call updated consistently with service refactoringThe change from
getBuildAgentInformationWithoutRecentBuildJobs()togetBuildAgentInformation()maintains consistency with the same change inAdminBuildJobQueueResourceand aligns with the broader refactoring of build agent information handling.src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html (2)
1-138: Well-structured component following Angular best practicesThis new modal component does an excellent job centralizing the finished builds filtering logic. The component's structure is clean and follows the modern Angular syntax guidelines with
@ifand@fordirectives. The component includes appropriate validation feedback for date ranges and duration filters.
43-66: Good use of conditional rendering for build agent filtersThe conditional rendering of build agent filtering options using
@if (buildAgentFilterable)shows appropriate consideration for different use cases.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (2)
246-257: Simplified build agent information update logicThe method has been simplified to align with the new structure of
BuildAgentInformation, which now usesBuildAgentDetailsDTOinstead of tracking recent build jobs directly. This refactoring improves code maintainability by centralizing build agent metrics management.
251-251:❓ Verification inconclusive
Verify BuildAgentInformation constructor behavior
The simplified constructor call
new BuildAgentInformation(buildAgent)assumes that this copy constructor properly handles all fields including the newbuildAgentDetails. Ensure this constructor preserves all necessary information.
🏁 Script executed:
#!/bin/bash # Find BuildAgentInformation constructors ast-grep --pattern "class BuildAgentInformation $$$" ast-grep --pattern "BuildAgentInformation($$$)"Length of output: 358
Below is a new shell script that narrows the search scope to the main source files. This should help us confirm whether the BuildAgentInformation class—and in particular its constructor(s) that are supposed to handle the new buildAgentDetails field—are defined as expected. Please run the following script and share its output:
Once we have these details, we can verify that the copy constructor used in
new BuildAgentInformation(buildAgent)
properly copies over all fields (including the newbuildAgentDetails).
🏁 Script executed:
#!/bin/bash # Limit search to the main source code to avoid non-UTF8 test files echo "Searching for BuildAgentInformation class definition in src/main/java..." rg "class BuildAgentInformation" src/main/java echo "" echo "Searching for BuildAgentInformation constructors in src/main/java..." rg "public BuildAgentInformation\(" src/main/javaLength of output: 823
Action Required: Verify Copy Constructor Field Propagation
The automated search confirms that the only
BuildAgentInformationconstructor in the main code is a copy constructor located at:
- File:
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java- Constructor Signature:
public BuildAgentInformation(BuildAgentInformation agentInformation)Since the call in
LocalCIResultProcessingService.java(line 251) uses:new BuildAgentInformation(buildAgent)and
buildAgentis of typeBuildAgentInformation, please ensure that the copy constructor properly propagates all relevant fields, particularly the newbuildAgentDetailsfield.src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts (2)
16-16: Import path updated to match new component location.The import for
FinishedBuildJobFilterhas been correctly updated to reference the new location in the finished-builds-filter-modal component. This aligns with the PR objective of moving the filtering modal into a separate component.
18-18: Import path updated to use absolute path.The import for
BuildLogEntryhas been changed from a relative path to an absolute path. This improves maintainability by removing nested relative paths.src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts (2)
9-9: Import for BuildAgentInformation simplified.The
BuildAgentInformationimport has been adjusted, aligning with the broader refactoring of build agent information in this PR.
14-27: Test setup simplified with reduced mock data.The test setup has been properly simplified to match the updated
BuildAgentInformationmodel that no longer containsrunningBuildJobs. The removal of complex mock objects (repositories, timing info, configs) aligns with the refactoring objectives of this PR.src/main/webapp/app/entities/programming/build-agent-information.model.ts (3)
4-4: Added dayjs import for date handling.Appropriate import added to support the date fields in the new BuildAgentDetails class.
19-19: Added buildAgentDetails property to BuildAgentInformation.The
buildAgentDetailsproperty correctly uses the new BuildAgentDetails type and is marked as optional with the?operator, maintaining backward compatibility.
22-32: Added BuildAgentDetails class with comprehensive metrics.The new class captures essential build agent metrics like average duration, successful/failed builds count, and timing information. This implementation aligns with the PR objective to collect and display more detailed information about build agents.
The class follows the Angular style guidelines with:
- PascalCase for the class name
- camelCase for all properties
- Appropriate typing including dayjs.Dayjs for date fields
src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts (5)
17-18: Added modal service imports for testing modal functionality.Appropriate imports for NgbModal and the mock modal service, which are needed to test the new modal functionality in the component.
113-113: Added modalService variable for test access.The modalService variable allows the tests to interact with and verify the modal functionality.
122-122: Added NgbModal provider with mock implementation.The mock NgbModal provider properly follows the test guidelines by mocking external dependencies.
132-132: Initialized modalService in test setup.The modalService is correctly initialized in the beforeEach block, following good testing practices.
254-265: Added test for modal dialog functionality.The test properly verifies that the component's modal display methods correctly interact with the NgbModal service. It checks that the open method is called the expected number of times, following the test expectation guidelines.
The use of a spy on the open method is an effective way to verify the interaction without testing the implementation details of the modal itself.
src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts (4)
53-53: No issues found.
Clearing thegetBuildJobStatisticsForCoursemock helps keep the tests isolated and reliable.
82-82: Validate mocked return value.
ReturningmockBuildJobStatisticsfor the course aligns with this test scenario. Looks good.
89-89: Skipping empty line.
No substantive change here.
90-102: Test coverage for input behavior is correct.
Ensures that the component relies on the provided input without making service calls. Solid approach to prevent unnecessary API requests.src/main/webapp/app/localci/build-queue/build-queue.component.html (4)
439-439: Dynamic styling is appropriate.
Highlighting the filter button differently when filters are active improves user awareness. No issues found.
443-443: Good translation usage.
Passing the number of filters as a translation parameter is a neat approach to keep the UI localized and dynamic.
438-438:✅ Verification successful
Confirm the called method exists.
Replacing the old filter approach withthis.openFilterModal()is acceptable. Verify the method is indeed implemented.
🏁 Script executed:
#!/bin/bash rg -A 10 "openFilterModal"Length of output: 18661
Verified: The openFilterModal method exists as implemented.
- The grep output confirms that
openFilterModal()is defined insrc/main/webapp/app/localci/build-queue/build-queue.component.ts.- Tests in related spec files (e.g., in
build-queue.component.spec.ts) further verify that the method is being correctly invoked and behaves as expected.No further changes are required.
676-705:✅ Verification successful
New modal for viewing build logs.
- The
<pre>block for raw logs is straightforward, but consider sanitizing if logs can contain HTML.- The modal structure (header, body, footer) is well-formed, with clear actions for close and download.
🏁 Script executed:
#!/bin/bash rg --multiline "rawBuildLogsString"Length of output: 1038
Build Logs Modal Verified – Safe Handling of Log Content
- The modal structure is well-formed with clear header, body, and footer actions for closing and downloading.
- The raw logs are displayed using Angular interpolation (
{{ rawBuildLogsString }}), which escapes HTML by default. However, if future changes use bindings that render HTML (such as[innerHTML]), then sanitization should be implemented.src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts (6)
1-12: Well-organized imports.
All required testing modules and mock dependencies are properly set up.
13-65: Solid describe block and test configuration.
- The
mockFinishedJobsarray provides realistic data for filtering tests.- Correct usage of Angular test utilities.
67-69: Basic instantiation test.
Verifies the component can be created without errors. Essential coverage.
71-74: Accurate test of build agent address extraction.
Ensures multiple addresses are aggregated correctly. Good detail.
98-122: Validates local storage practices.
Ensures applied filters are correctly tracked and saved. Straightforward approach.
124-149: Comprehensive validation testing.
Covers date and duration constraint checks, confirming invalid ranges are flagged.src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts (5)
1-12: Imports look complete.
Day.js and RxJS are well-integrated for date/time and typeahead features.
82-91: Well-defined build status enum.
Using uppercase constants for status values could further align with server expectations.
93-101: Filter key enum for clarity.
Mapping filter keys to descriptive constants improves maintainability.
102-107: Correct component metadata usage.
Standalone component pattern is properly followed. No concerns noted.
108-236: Clear, modular filter modal logic.
- The approach to typeahead and separate toggles for date/duration ensures each filter is handled distinctly.
- Good practice returning filters upon modal closure.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (1)
1-1: Confirm Angular version compatibility forinputfunction.
While the newinput<T>API is part of Angular’s emerging signal-based features, make sure your application is running a version of Angular that supports this API. Otherwise, consider using the conventional@Input()decorator.Would you like me to generate a script to check the Angular version in your package.json, or do you already have it covered?
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java (2)
15-15: Validate data structure changes in distributed caches.
AddingBuildAgentDetailsDTOshifts your Hazelcast records. Make sure you handle any migration or backlog clearing if older nodes are still using the previous record layout.
25-27: Confirm consistency of the copy constructor.
The copy constructor referencesagentInformation.buildAgentDetails(), but the original context mentions removingrecentBuildJobs. Verify that no code paths still expect or rely onrecentBuildJobsin parallel with this new approach.src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (4)
1-31: Ensure the new service runs only under the correct profile.
Using@Profile(PROFILE_BUILDAGENT)is appropriate, but confirm your deployment environment is loading this profile in the desired build-agent context.
33-59: Inject dependencies using constructor injection only.
You’ve done it correctly by injectingHazelcastInstance,BuildAgentConfiguration,BuildAgentSshKeyService, andGitPropertiesin the constructor. This aligns with best practices for immutability and testability.
61-65: Verify Hazelcast map naming and usage.
ThebuildAgentInformationandprocessingJobsmaps are created with fixed names ("buildAgentInformation"and"processingJobs") ininit(). Double-check that your existing code expects these exact names and that the keys/values are properly migrated if these maps existed under different names before.
92-110: Leverage existingBuildAgentInformationreferences carefully.
When constructing the newBuildAgentInformation, you rely on a previously retrievedagentobject from the map. Mark sure no race conditions occur ifagentbecame stale between retrieval and re-insertion.src/main/webapp/app/localci/build-queue/build-queue.component.ts (7)
1-1: Use of Angular inject()
Good adoption of Angular'sinject()function. This keeps the component constructor succinct and aligns with Angular's recommended approach for dependency injection in newer versions.
7-7: RxJS import clarity
These RxJS operators (debounceTime,switchMap,take,tap) are well-chosen for this workflow.
12-12: HTTP headers & error imports
ImportingHttpErrorResponse,HttpHeaders, andHttpResponseis consistent with the usage in your HTTP request/response handling.
15-15: NgbModal usage
InjectingNgbModalis correct. Be mindful to handle modal closing and error states properly.
31-33: New imports for constants and filter
ImportingUI_RELOAD_TIME,Subject,Subscription, andFinishedBuildJobFilterbroadens functionality. Ensure all references are used appropriately throughout the file.
88-89: Search subject initialization
Declaringsearch = new Subject<void>();andisLoading = false;is clear. This improves readability and properly signals loading status.
91-93: Initializing the default filter
HavingfinishedBuildJobFilter = new FinishedBuildJobFilter();ensures the component always starts with a valid, default filter instance. Also, referencingfaFilteris consistent with other icons.src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts (13)
5-5: Expanded model imports
Importing bothBuildJobandFinishedBuildJobshows you're testing new functionalities that rely on completed build job data.
19-24: Additional providers and test imports
IncludingprovideHttpClient, advanced HTTP mocks,MockNgbModalService, and theFinishedBuildJobFilterimport ensures comprehensive testing coverage for new features.
109-115: Paging & sorting request
Definingrequestwith pagination, sorting column, etc., is a neat approach for driving uniform tests across multiple scenarios.
116-128: Empty filter settings
filterOptionsEmptyis helpful for verifying boundary conditions when no filters are applied.
130-166: Detailed mock finished jobs
These finished job entries thoroughly test the newly introduced fields (e.g.,triggeredByPushTo,commitHash). Good job ensuring coverage of multiple properties.
168-168: HTTP response mock
mockFinishedJobsResponsefosters consistency by simulating the HTTP layer’s return payload for the job list.
169-169: Ensuring typed response
Wrapping the array in anHttpResponseobject is best practice for aligning with Angular's HTTP data structures.
172-172: NgbModal injection
ReferencingmodalServiceensures testers can verify modal interactions, providing more reliable coverage of user flows.
173-177: Better mocking coverage
ThemockBuildQueueServicedefinitions for new endpoints and cancellation methods help verify the integration with the build queue in the context of build-agent details.
323-335: Verifying search term refresh
Debounce-based refresh logic is tested, ensuring that minimal performance overhead is introduced. Good approach for asynchronous verification.
337-348: Build duration calculation
Verifying thatbuildDurationis correctly updated in the test is great. Confirms that dayjs integration works as intended.
350-371: Filter modal usage
Thoroughly testing modal assignment offinishedBuildJobFilterhelps confirm that the filter UI updates. Good coverage of corner cases like default filter states.
372-378: Opening build logs
Ensuringwindow.openis called with the correct path covers the newly introduced log viewing functionality.src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (5)
4-4: Approach to tested component
Directly importingBuildQueueComponentfor test coverage is straightforward. Keep an eye on potential cyclical references across test modules.
18-20: Modal test enhancements
ImportingFinishedBuildJobFilter,NgbModal, andMockNgbModalServicesupports advanced test scenarios around the new filtering modal.
256-256: Modal injection
Declaringlet modalService: NgbModal;ensures consistent usage inside tests, enabling robust validation of modal flows.
534-553: Filter modal data test
Confirms thatopenFilterModalsets the correct fields on the modal’s component instance. This effectively validates the new approach to filter management.
572-572: Adjust Jest call count
Ensuring.toHaveBeenCalledTimes(1)is an improvement over older patterns like.toHaveBeenCalledOnce(). This clarifies the exact call count.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (7)
4-26: No specific concerns identified.
51-51: This addition looks great!
53-80: Implementation appears solid.
86-264: New columns and UI display logic look good.
266-281: No issues found.
289-322: Pagination and filtering UI appear correct.
323-539: Overall table layout for finished build jobs is well-structured.src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (1)
76-76: Refactoring to use BuildAgentInformationService looks good.The delegation of update calls to
buildAgentInformationService.updateLocalBuildAgentInformation(...)andupdateLocalBuildAgentInformationWithRecentJob(...)is consistent with the single-responsibility principle. No concerns identified.Also applies to: 131-132, 137-137, 231-231, 248-248, 295-295, 319-319, 374-374, 424-424, 443-443, 514-514
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (11)
4-5: New & updated imports look appropriate.Also applies to: 19-33
66-66: Field initializations and icon declarations look fine.Also applies to: 70-71, 78-78, 81-104
99-104: Pagination settings are consistent with project constants.
153-156: Loading logic and statistics aggregation are correct.Also applies to: 161-169
234-245: Modal filtering logic is clear.
250-261: Good approach to separately load finished build jobs.
269-273: Response handling is straightforward and correct.
275-288: Time calculation using dayjs is neatly implemented.
290-304: Fetching logic aligns well with your new filter approach.
306-313: Debounced search triggers appear well-handled.
315-325: Pagination callback is neatly integrated with loadFinishedBuildJobs.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java
Outdated
Show resolved
Hide resolved
...ocalci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (1)
63-73: 🛠️ Refactor suggestionMissing null check when accessing buildJobStatisticsInput.
The non-null assertion (
!) on line 70 could cause runtime errors ifbuildJobStatisticsInput()returns null or undefined.- this.updateDisplayedBuildJobStatistics(this.buildJobStatisticsInput()!); + const statistics = this.buildJobStatisticsInput(); + if (statistics) { + this.updateDisplayedBuildJobStatistics(statistics); + } else { + // Handle the case when no input statistics are provided + this.updateDisplayedBuildJobStatistics(new BuildJobStatistics()); + }Also, the route-based condition may be fragile. Checking
url[0].path === 'build-queue'might fail if route structure changes. Consider using route configuration data or route parameters for a more robust solution.src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts (1)
114-115: 🛠️ Refactor suggestionUnsubscribe from local Subjects to prevent memory leaks.
Like previous review comments, consider implementing
OnDestroyand unsubscribing fromfocus$andclick$, since these Subjects remain active beyond the component’s lifecycle.+ import { OnDestroy } from '@angular/core'; ... + export class FinishedBuildsFilterModalComponent implements OnInit, OnDestroy { ... + ngOnDestroy(): void { + this.focus$.complete(); + this.click$.complete(); + }
🧹 Nitpick comments (11)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
246-257: Method name should reflect its current functionality.The method
addResultToBuildAgentsRecentBuildJobsno longer adds anything to recent build jobs as indicated by its name. It simply creates a newBuildAgentInformationfrom an existing one, which aligns with the refactoring of build agent information management.Rename the method to better reflect its current functionality:
-private void addResultToBuildAgentsRecentBuildJobs(BuildJobQueueItem buildJob, Result result) { +private void updateBuildAgentInformation(BuildJobQueueItem buildJob, Result result) {Also ensure that any method calls are updated accordingly.
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts (2)
13-80: Review the defaulting logic for filters.While the
FinishedBuildJobFilterclass correctly default-initializes fields and handles filter additions/removals, keep in mind:
- Appending
undefinedfields as query params can clutter logs or produce inconsistent query strings.- Consider ensuring consistent usage of the
BuildJobStatusFilterenum to reduce the potential for typos.Otherwise, the class is well-structured for readability and modular usage.
102-108: Minor consistency check: use conventional Angular metadata fields.Typically Angular uses
styleUrls(plural) instead ofstyleUrl. While this might still work in some setups, for consistency, consider:- styleUrl: './finished-builds-filter-modal.component.scss', + styleUrls: ['./finished-builds-filter-modal.component.scss'],src/main/webapp/app/localci/build-queue/build-queue.component.html (2)
438-439: Omit explicit 'this' in template calls.Using
this.openFilterModal()in the template is unusual in Angular. Consider removingthisfor better readability:- (click)="this.openFilterModal()" + (click)="openFilterModal()"
676-704: Modal structure is consistent with ng-bootstrap patterns.The newly added modal for build logs is straightforward and adheres to standard
ng-templateusage. Ensure you handle edge cases where large logs might degrade performance in<pre>. If logs can be huge, consider streaming or pagination to mitigate performance issues.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (1)
29-48: Consider localizing these text labels.You could wrap “Status”, “Start Date”, “Latest Build Date”, “Average Build Duration”, and “Git Revision” with
jhiTranslatefor consistent i18n support throughout the application.src/main/webapp/app/localci/build-queue/build-queue.component.ts (1)
397-407: Add error handling for modal rejections.The
.catch(() => {})block swallows any rejection from the modal. Consider logging or alerting to avoid silently ignoring potential errors.- .catch(() => {}); + .catch((error) => { + console.error('Filter modal dismissed', error); + });src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts (1)
323-335: Use a single constant for debounce durations.Hardcoding the timeout (110ms) in the test might lead to flakiness if
UI_RELOAD_TIMEchanges. You can reference the same constant or add a small margin to ensure tests remain stable.src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (1)
231-231: Frequent calls toupdateLocalBuildAgentInformation—watch for potential overhead.
Multiple additions ofbuildAgentInformationService.updateLocalBuildAgentInformation(isPaused.get())ensure agent status is up-to-date. This is correct for consistency, but from a performance perspective, be mindful of the overhead if these calls significantly accumulate in high-traffic scenarios.Consider consolidating redundant status updates if you can safely do so without losing correctness.
Also applies to: 248-248, 295-295, 319-319, 443-443, 514-514
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (1)
310-314: Conditionally trigger search with minimal length but handle short inputs gracefully.
CallingtriggerLoadFinishedJobs()only after 3 characters is a good optimization. However, your code does not address strings shorter than 3 once typed, meaning partial searches might be missed. Consider if partial matching is useful.src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (1)
146-154: Avoid integer truncation in average build duration.
Usinglongmay lose fractional seconds when computing the new average build time. If timing precision is important, consider storing durations as floating-point or storing milliseconds to retain partial second granularity.- return (agent.buildAgentDetails().averageBuildDuration() * agent.buildAgentDetails().totalBuilds() + currentBuildDuration) / (agent.buildAgentDetails().totalBuilds() + 1); + long newTotal = agent.buildAgentDetails().totalBuilds() + 1; + double runningSum = agent.buildAgentDetails().averageBuildDuration() * agent.buildAgentDetails().totalBuilds() + currentBuildDuration; + return (long) Math.round(runningSum / newTotal);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(10 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java(0 hunks)src/main/webapp/app/entities/programming/build-agent-information.model.ts(2 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html(2 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.scss(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(7 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html(2 hunks)src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.html(2 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.scss(0 hunks)src/main/webapp/app/localci/build-queue/build-queue.component.ts(4 hunks)src/main/webapp/app/localci/build-queue/build-queue.service.ts(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.scss(1 hunks)src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java(1 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts(7 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts(5 hunks)src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts(5 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts(6 hunks)src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts(1 hunks)src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java
- src/main/webapp/app/localci/build-queue/build-queue.component.scss
🧰 Additional context used
📓 Path-based instructions (5)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.javasrc/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.tssrc/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.tssrc/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.tssrc/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/entities/programming/build-agent-information.model.tssrc/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.tssrc/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.tssrc/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.tssrc/main/webapp/app/localci/build-queue/build-queue.service.tssrc/main/webapp/app/localci/build-queue/build-queue.component.tssrc/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.htmlsrc/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.htmlsrc/main/webapp/app/localci/build-queue/build-queue.component.htmlsrc/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (64)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.scss (1)
6-8: LGTM: The new stats-column class is appropriately defined.The addition of the
.stats-columnclass with a minimum width of 250px will ensure that the columns displaying build agent statistics maintain a consistent width, improving readability of the details view.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java (1)
184-184: Updated health method to use refactored service method.The change aligns with the broader refactoring where
getBuildAgentInformationWithoutRecentBuildJobs()has been removed in favor of the more comprehensivegetBuildAgentInformation()method. This change maintains the contract of the health method while using the consolidated service method.src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.scss (1)
1-11: LGTM: Appropriately extracted styles for new component.This style extraction follows good component design practices by moving styles specific to the finished builds filter modal from the build-queue component to its own dedicated component stylesheet. The use of CSS variables promotes consistency across the application.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java (2)
74-74: Updated to use refactored service method.The change correctly calls
getBuildAgentInformation()instead of the removedgetBuildAgentInformationWithoutRecentBuildJobs()method, aligning with the broader refactoring in the PR.
126-134:✅ Verification successful
Verify compatibility with updated BuildAgentInformation record structure.
The method creates a new BuildAgentInformation object, but based on the model changes mentioned in the PR (especially regarding runningBuildJobs and the new buildAgentDetails field), there might be a mismatch between this constructor call and the updated record structure.
🏁 Script executed:
#!/bin/bash # Check the current structure of BuildAgentInformation record to ensure compatibility # Look for BuildAgentInformation record definition rg -A 10 "record BuildAgentInformation" --type java # Check usages of runningBuildJobs in other parts of the codebase rg "runningBuildJobs" --type java # Check if buildAgentDetails is used in the constructor rg "new BuildAgentInformation\\(.*buildAgentDetails" --type javaLength of output: 3451
Confirmed: The BuildAgentInformation constructor is compatible with its updated record structure.
After verifying the record definition and usage, the constructor call in
LocalCIQueueWebsocketService.javacorrectly passes seven arguments corresponding to the updated fields (includingbuildAgentDetails). The assignment ofnullfor bothpublicSshKeyandbuildAgentDetailsis consistent with the record’s structure, and no incompatibility is detected.src/main/webapp/app/localci/build-queue/build-queue.service.ts (1)
9-9: Import path updated to reflect component restructuringThe import statement has been updated to reference the
FinishedBuildJobFilterclass from the new modal component instead of directly from the build-queue component. This change aligns with the PR objective of converting the modal for filtering finished build jobs into a separate component.src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java (1)
86-86: Method call updated to use the new build agent information serviceThe method call has been updated from
getBuildAgentInformationWithoutRecentBuildJobs()togetBuildAgentInformation(), reflecting the refactoring of build agent information logic into a separate service as mentioned in the PR objectives.src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIResourceIntegrationTest.java (1)
99-99: Update test initialization to fully utilize the BuildAgentDetailsDTOThe agent1 initialization now passes null for the BuildAgentDetailsDTO parameter, which doesn't properly test the new functionality introduced in this PR. Consider creating a proper BuildAgentDetailsDTO instance to ensure comprehensive test coverage of the new data structure.
- agent1 = new BuildAgentInformation(buildAgent, 2, 1, new ArrayList<>(List.of(job1)), BuildAgentInformation.BuildAgentStatus.IDLE, null, null); + var buildAgentDetails = new BuildAgentDetailsDTO(100L, 5L, 2L, 1L, 0L, 8L, ZonedDateTime.now(), ZonedDateTime.now().minusDays(1), "main-12345"); + agent1 = new BuildAgentInformation(buildAgent, 2, 1, new ArrayList<>(List.of(job1)), BuildAgentInformation.BuildAgentStatus.IDLE, buildAgentDetails, null);src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentDetailsDTO.java (1)
1-12: Excellent use of Java record for the new DTOThis new DTO properly encapsulates build agent metrics using a Java record, which aligns with the Java coding guidelines for DTOs. The implementation of Serializable with a serialVersionUID is appropriate for a DTO that will be used in a distributed environment.
The metrics captured (build durations, success/failure counts, timestamps) directly support the PR objective of extending DTOs to include additional information about build agents.
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts (1)
23-23: LGTM! Removed unused import.The removal of
ArtemisTranslatePipefrom the imports array is appropriate as it's no longer needed in this component.src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.html (2)
5-21: Good use of @if for conditional rendering.Wrapping the span selector UI in an
@ifdirective improves the component's flexibility. This follows the Angular guidelines for using the new control flow syntax over the older*ngIfapproach.
76-87: Good use of @if for conditionally displaying missing builds statistics.The conditional rendering of the missing builds statistics using the new
@ifdirective is well-implemented and aligns with the PR's goal of enhancing how build information is displayed.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java (1)
251-251: Simplification of build agent information update.The simplification of the build agent information update aligns with the PR's goal of refactoring logic into a separate service. This change follows the single responsibility principle.
src/main/webapp/app/entities/programming/build-agent-information.model.ts (3)
4-4: Appropriate import for date handling.Adding the dayjs import is appropriate for the new date properties in the BuildAgentDetails class.
19-19: Good enhancement to BuildAgentInformation model.Adding the buildAgentDetails property enhances the BuildAgentInformation model to include the detailed metrics, which aligns well with the PR's objective of collecting more information about build agents.
22-32: Well-structured BuildAgentDetails class.The BuildAgentDetails class is well-structured with appropriate property types and naming. It provides comprehensive metrics that will be useful for the enhanced build details view.
src/test/javascript/spec/component/localci/build-queue/build-queue.service.spec.ts (2)
16-16: Updated import path for FinishedBuildJobFilter.The import for
FinishedBuildJobFilterhas been updated to reflect its new location in the dedicated filter modal component, which aligns with the separation of concerns in the PR.
18-18: Switched to absolute import path.Import path for
BuildLogEntryhas been changed from relative to absolute, which is consistent with the project's import style.src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.html (4)
20-32: Good use of Angular's new@forsyntax.The implementation correctly uses Angular's new
@forsyntax instead of the deprecated*ngFor, as specified in the coding guidelines.
43-66: Appropriate conditional rendering with@if.The component correctly uses Angular's new
@ifdirective instead of the older*ngIfsyntax, as specified in the coding guidelines. The conditional display of the build agent filter is a good UX decision.
101-103: Good validation feedback implementation.The component provides clear error messages with appropriate validation state handling for the date fields.
122-124: Consistent validation feedback for duration filters.The validation approach is consistent with the date validation, providing clear user feedback for invalid input states.
src/test/javascript/spec/component/localci/build-agents/build-agent-summary.component.spec.ts (2)
17-18: Good test setup with mock modal service.The changes properly integrate the NgbModal service with appropriate mocking, following the test guidelines for mocking irrelevant dependencies.
Also applies to: 113-113, 122-122, 132-132
254-265: Well-structured test for modal interactions.This new test properly verifies that modals are correctly opened when the respective methods are called. The test follows best practices by:
- Mocking the modal reference
- Spying on the open method
- Using specific assertions with
toHaveBeenCalledTimesThis ensures that the UI interaction methods are working as expected.
src/main/webapp/app/localci/build-queue/build-job-statistics/build-job-statistics.component.ts (3)
30-30: Signal-based input used instead of traditional @input decorator.Using
input<BuildJobStatistics>()is valid in newer Angular versions, but differs from the traditional approach. Consider if this is consistent with your team's standards.
45-46: Good addition of display control flags.Adding these boolean properties allows for more flexible usage of the component in different contexts. This enables the component to be reused with different visual configurations.
78-78: Improved method naming for clarity.Renaming the method to
getBuildJobStatisticsForBuildQueuemakes its purpose clearer and better reflects its specific functionality.src/test/javascript/spec/component/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.spec.ts (9)
1-12: Good import organization with appropriate mocksThe imports are well organized and include necessary testing utilities. The use of
MockProviderandMockTranslateServicefollows the coding guidelines for service testing.
13-16: Component setup follows Angular testing best practicesThe test suite structure with appropriate variable declarations follows Angular testing conventions.
17-53: Well-defined mock data for comprehensive test coverageThe mock data provides a good variety of test cases, including different build agent addresses and dates for testing filtering functionality.
55-65: TestBed configuration correctly uses standalone componentsThe test module is properly configured with necessary imports and providers. Using
OwlNativeDateTimeModuleand standalone component imports follows Angular best practices.
67-69: Basic component creation test is presentStandard component creation test is implemented correctly.
71-74: Test for buildAgentAddresses getter uses specific assertionGood use of
toEqualfor array comparison rather than a generic matcher, which aligns with the expectation specificity guideline.
76-96: Filter counting logic is thoroughly testedThe test comprehensively verifies that filters are correctly counted when applied and removed.
98-122: Local storage filter persistence test is well implementedTest thoroughly checks that filter state is correctly tracked in the
appliedFiltersset.
124-149: Validation logic is properly tested with multiple scenariosTest covers both valid and invalid scenarios for date and duration filters, using appropriate boolean assertions.
src/test/javascript/spec/component/localci/build-queue/build-job-statistics/build-job-statistics.component.spec.ts (6)
16-16: Using let for mockActivatedRoute enables dynamic reassignmentGood change from const to let for mockActivatedRoute to enable dynamic route simulation in different test cases.
53-53: Mock service method now correctly cleared between testsAdding mockBuildQueueService.getBuildJobStatisticsForCourse.mockClear() ensures proper test isolation by resetting mock function call counts between tests.
57-57: Added route URL simulation for accurate component testingSetting mockActivatedRoute.url properly simulates the component being accessed from the 'build-queue' route.
68-68: Consistent URL path setup across test casesUsing the same approach for URL path simulation ensures consistent test behavior.
79-79: Route URL simulation maintained for course-specific testEnsuring the same route context is applied to the course-specific test case for consistency.
90-102: Added test for input-based statistics displayThis new test covers an important use case: when statistics are provided directly as component input, they should be displayed without making service calls, and display flags should be set accordingly.
src/test/javascript/spec/component/localci/build-agents/build-agents.service.spec.ts (1)
9-9: Import simplified to reflect model changesImport statement has been simplified to only import BuildAgentInformation, which aligns with the model changes where runningBuildJobs are now encapsulated within BuildAgentInformation.
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.java (3)
14-15: BuildAgentDetailsDTO added to improve encapsulationThe record now includes BuildAgentDetailsDTO instead of recentBuildJobs, improving encapsulation of build agent metrics and separating concerns.
25-28: Constructor updated to align with new parameter structureThe constructor has been updated to use the new BuildAgentDetailsDTO parameter instead of recentBuildJobs, consistent with the record definition change.
1-14:❓ Verification inconclusive
Ensure serialization compatibility with distributed data structures
The BuildAgentInformation record is used in shared code between core and build agent nodes. Verify that these structural changes won't cause issues with existing serialized data in Hazelcast.
Also applies to: 16-34
🏁 Script executed:
#!/bin/bash # Find usages of BuildAgentInformation in Hazelcast-related code to ensure serialization compatibility echo "Checking BuildAgentInformation usage in Hazelcast context:" rg -l --type java "Hazelcast|IMap|MapStore|MapLoader" | xargs rg -l "BuildAgentInformation" echo -e "\nChecking if the BuildAgentDetailsDTO is properly serializable:" rg -l --type java "Serializable" | xargs rg -l "BuildAgentDetailsDTO"Length of output: 1334
Action: Verify Distributed Serialization Compatibility
The changes to
BuildAgentInformation(and the relatedBuildAgentDetailsDTO, which applies to lines 16–34) affect shared data used across core and build-agent nodes. The shell script confirms thatBuildAgentInformationis indeed referenced in multiple Hazelcast contexts (and similarly,BuildAgentDetailsDTOis actively used), so any structural modifications risk breaking backward compatibility with existing serialized data. Before merging these changes, please ensure that:
- Any updates to
BuildAgentInformation(located insrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/BuildAgentInformation.javaat lines 1–14) are backward compatible with cached/distributed entries in Hazelcast.- Corresponding changes in
BuildAgentDetailsDTO(at lines 16–34 in its file) are also managed appropriately (e.g., via proper versioning, migration, or cache clearing).src/main/webapp/app/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts (2)
82-101: Enums appear clean and consistent.The enum naming for
BuildJobStatusFilterandFinishedBuildJobFilterKeyfollows a readable pattern. The usage of string assignments for each member is clear and matches Angular/TypeScript coding conventions.
109-235: Overall component logic is clear and well-documented.Functions for toggling filters and validating bounds (e.g., date intervals, duration intervals) look correct. The approach for combining filters into a
Mapis sensible. Keep an eye on side effects for large data sets, as your repeated merges withHttpParamscould grow if done in loops elsewhere, but in its current usage, it seems appropriate.src/main/webapp/app/localci/build-queue/build-queue.component.html (1)
443-443: Dynamic translation usage looks good.The
[translateValues]="{ num: finishedBuildJobFilter?.numberOfAppliedFilters ?? 0 }"usage is a neat way of localizing dynamic data. This is consistent with your i18n approach.src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts (4)
4-20: Imports refactoring is coherent.Migrating the
FinishedBuildJobFilterimport to the new modal file and introducingNgbModalreferences is aligned with your new filtering logic. The approach seems correct given the architectural shift.
534-553: Nice coverage for the new filter modal logic.Your test thoroughly checks that the modal gets the correct initial state (
finishedBuildJobFilter,finishedBuildJobs,buildAgentFilterable). You might also test user interaction flows, but this at least confirms initial binding is correct.
572-572: Good practice: .toHaveBeenCalledTimes(1) aligns with Jest patterns.Replacing
.toHaveBeenCalledOnce()with.toHaveBeenCalledTimes(1)ensures consistency with standard Jest usage, preventing confusion.
255-269:✅ Verification successful
Verify usage of the injected
NgbModal.Ensuring the mock replaces the actual service is correct. Confirm you don’t accidentally re-instantiate the real
NgbModalin other providers. Otherwise, test isolation might break.
🏁 Script executed:
#!/bin/bash # Check for any direct reference to NgbModal in test folder aside from your declared mock usage rg "new NgbModal\(" src/test/ rg "NgbModal[^,]+" src/test/Length of output: 69035
Verified: Proper Mocking of NgbModal in BuildQueueComponent Tests
The TestBed configuration in src/test/javascript/spec/component/localci/build-queue/build-queue.component.spec.ts correctly replaces NgbModal with the MockNgbModalService via the provider:
- The configuration uses
{ provide: NgbModal, useClass: MockNgbModalService }.- The injection via
TestBed.inject(NgbModal)confirms that the mock is used.No instances of re-instantiating the real NgbModal were detected in this file. Please ensure that similar consistency is maintained across tests to safeguard test isolation.
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (2)
53-285: Refactor suggestion repeated from prior commit.This data table for running build jobs appears similar to other tables. Reuse or factor it into a shared component to reduce duplication and keep logic centralized.
323-520: Refactor suggestion repeated from prior commit.This table layout for finished build jobs also shares similarities with existing tables. Consider extracting common table functionality into a shared or table-builder style component to streamline updates.
src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts (1)
1-379: Overall test additions look good.The new tests for finished build jobs, filter values, and build log display appear thorough and help ensure coverage against regressions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 208-210: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java (3)
76-76: Use of new service field for centralized agent tracking.
The introduction ofBuildAgentInformationServiceis consistent with maintaining single responsibility for build agent information. Ensure this field remains private and final to follow best practices for DI and immutability.
130-137: Constructor injection refactor looks good, minor caution on parameter growth.
AddingBuildAgentInformationServiceto the constructor aligns with constructor injection guidelines. However, watch out for parameter bloat as classes grow in complexity. Consider encapsulating configuration objects if more dependencies arise.
374-374:❓ Verification inconclusive
Use of
updateLocalBuildAgentInformationWithRecentJobfor completed builds is sound, but verify concurrency.
InvokingupdateLocalBuildAgentInformationWithRecentJob(finishedJob, isPaused.get())helps track the final status of the recently processed job. Ensure no race conditions occur if multiple threads finish jobs concurrently, potentially overwriting each other’s updates inBuildAgentInformationService.Also applies to: 424-424
🏁 Script executed:
#!/bin/bash # Check usage of updateLocalBuildAgentInformationWithRecentJob for concurrency control rg -A 10 'updateLocalBuildAgentInformationWithRecentJob'Length of output: 5658
Double-check concurrency handling in agent updates
UsingupdateLocalBuildAgentInformationWithRecentJob(finishedJob, isPaused.get())(observed at lines 374 and 424) is a sound approach to capture the final job state. The implementation inBuildAgentInformationServiceshows that a lock is obtained viabuildAgentInformation.lock(memberAddress), which is intended to serialize concurrent updates by different threads. However, please ensure that:
- The acquired lock is indeed released properly (e.g., via a finally block or equivalent mechanism) so that subsequent job completions are not blocked.
- The locking strategy is robust against race conditions when multiple threads complete jobs concurrently.
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (3)
4-5: New RxJS and FontAwesome imports are consistent with Angular style.
The additional imports fordebounceTime,switchMap, and FontAwesome icons align well with typical Angular usage and enhance readability.
111-126: Unsubscribe from thesearchSubscriptionto prevent memory leaks.
The subscription tosearchis created here but not unsubscribed inngOnDestroy, risking memory leaks if the component is destroyed while a search is in progress.
321-325: Validate page number boundaries.
If users manually set the page to a negative or zero value in the query parameters, this code proceeds regardless. For a more robust UX, bound pageNumber to a valid range.src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (2)
80-95: Ensure robust error handling and locking.
This code logs exceptions but may silently swallow critical states if updates fail. Consider rethrowing or handling them more explicitly to ensure upstream components can respond. Also confirm the lock is always released, which can be risky if the node crashes.
142-144: Handle possible null completion date to avoid potentialNullPointerException.
IfrecentBuildJob.jobTimingInfo().buildCompletionDate()is null,Duration.between()might throw an exception. Safeguard this check to handle in-progress or missing completion timestamps.
...p/localci/build-queue/finished-builds-filter-modal/finished-builds-filter-modal.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (1)
116-131:⚠️ Potential issueUnsubscribe from searchSubscription to prevent memory leaks.
The searchSubscription is initialized but not properly unsubscribed in ngOnDestroy, which could lead to memory leaks.
ngOnDestroy() { this.websocketService.unsubscribe(this.channel); this.websocketSubscription?.unsubscribe(); this.agentDetailsSubscription?.unsubscribe(); this.runningJobsSubscription?.unsubscribe(); + this.searchSubscription?.unsubscribe(); clearInterval(this.buildDurationInterval); this.paramSub?.unsubscribe(); }
🧹 Nitpick comments (4)
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java (1)
73-79: Implementation of agent filtering is correct but could be more efficient.The implementation correctly adds an optional parameter and filtering logic for running build jobs by agent name. The null and empty string checks are appropriate defensive programming.
However, consider using a stream filter instead of removeIf for better readability:
- List<BuildJobQueueItem> runningBuildJobs = localCIBuildJobQueueService.getProcessingJobs(); - if (agentName != null && !agentName.isEmpty()) { - runningBuildJobs.removeIf(buildJobQueueItem -> !buildJobQueueItem.buildAgent().name().equals(agentName)); - } + List<BuildJobQueueItem> runningBuildJobs = localCIBuildJobQueueService.getProcessingJobs(); + if (agentName != null && !agentName.isEmpty()) { + runningBuildJobs = runningBuildJobs.stream() + .filter(buildJobQueueItem -> buildJobQueueItem.buildAgent().name().equals(agentName)) + .toList(); + }src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (1)
362-363: Duplicate conditional styling could be extracted into a helper class.The same condition for text styling based on build job status appears in multiple places. Consider creating a CSS class or utility function to avoid repetition.
// Create a class in your component's SCSS file // build-agent-details.component.scss + .build-status-text { + &.successful { color: var(--bs-success); } + &.cancelled { color: var(--bs-warning); } + &.failed, &.error { color: var(--bs-danger); } + } // Then use it in the template -<span - [ngClass]="{ - 'text-success': finishedBuildJob.status === 'SUCCESSFUL', - 'text-warning': finishedBuildJob.status === 'CANCELLED', - 'text-danger': finishedBuildJob.status === 'FAILED' || finishedBuildJob.status === 'ERROR', - }" ->{{ finishedBuildJob.name }}</span> +<span class="build-status-text" + [ngClass]="{ + 'successful': finishedBuildJob.status === 'SUCCESSFUL', + 'cancelled': finishedBuildJob.status === 'CANCELLED', + 'failed': finishedBuildJob.status === 'FAILED' || finishedBuildJob.status === 'ERROR', + }" ->{{ finishedBuildJob.name }}</span>Also applies to: 383-391
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (2)
331-334: Add user feedback for minimum search term length.When a user enters a search term shorter than 3 characters, nothing happens without any feedback. Consider adding a message or disabling the search button.
triggerLoadFinishedJobs() { if (!this.searchTerm || this.searchTerm.length >= 3) { this.search.next(); + } else if (this.searchTerm.length > 0 && this.searchTerm.length < 3) { + this.alertService.addAlert({ + type: AlertType.INFO, + message: 'artemisApp.buildQueue.filter.search.minLength', + translationParams: { minLength: 3 }, + }); } }
356-360: Use optional chaining to prevent potential null reference.Change to an optional chain when accessing
buildJob.jobTimingInfo.buildStartDateto prevent potential null reference exceptions if jobTimingInfo is undefined.updateBuildJobDuration(buildJobs: BuildJob[]): BuildJob[] { // iterate over all build jobs and calculate the duration return buildJobs.map((buildJob) => { - if (buildJob.jobTimingInfo && buildJob.jobTimingInfo.buildStartDate) { + if (buildJob.jobTimingInfo?.buildStartDate) { const start = dayjs(buildJob.jobTimingInfo.buildStartDate); const now = dayjs(); buildJob.jobTimingInfo.buildDuration = now.diff(start, 'seconds'); } // This is necessary to update the view when the build job duration is updated return { ...buildJob }; }); }🧰 Tools
🪛 Biome (1.9.4)
[error] 356-356: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java(2 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html(2 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(8 hunks)src/main/webapp/app/localci/build-queue/build-queue.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/localci/build-queue/build-queue.service.ts
🧰 Additional context used
📓 Path-based instructions (3)
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
🪛 Biome (1.9.4)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
[error] 356-356: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (8)
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java (2)
67-70: Documentation correctly updated to reflect new parameter.The Javadoc has been properly updated to document the new optional parameter for filtering running build jobs by agent name.
90-90:❓ Verification inconclusive
Verify if removed "WithoutRecentBuildJobs" suffix changes expected behavior.
The method call has been changed from
getBuildAgentInformationWithoutRecentBuildJobs()togetBuildAgentInformation(). This change in method call suggests a change in behavior.
🏁 Script executed:
#!/bin/bash # Search for the method signature in SharedQueueManagementService to understand behavior change cat src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java | grep -A 10 "getBuildAgentInformation" # Also check if there are any references to the old method that might break rg "getBuildAgentInformationWithoutRecentBuildJobs" --type javaLength of output: 2116
ACTION REQUIRED: Verify Intentional Removal of Filtering Logic for Recent Build Jobs
The call on line 90 in
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.javawas updated from usinggetBuildAgentInformationWithoutRecentBuildJobs()togetBuildAgentInformation(). Shell script searches confirmed that the old method is no longer referenced in the codebase and thatgetBuildAgentInformation()simply returns all build agent information without filtering out recent build jobs.
- In
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java,getBuildAgentInformation()returns a complete list from the underlying map.- There are no remaining references to or definitions of a method with the "WithoutRecentBuildJobs" suffix, indicating that the change is comprehensive.
Please verify manually whether the absence of filtering for recent build jobs is intentional and meets the desired functionality.
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (5)
27-49: Well-structured agent details section with clear information presentation.The new layout with dedicated sections for each agent detail (status, dates, metrics) improves readability and organization of information.
51-51: Good use of component extraction for build job statistics.Using the dedicated
jhi-build-job-statisticscomponent promotes code reuse and separation of concerns.
53-274: Consider extracting running jobs table into a reusable component.This large block for listing running jobs is quite similar to other job-table structures in the codebase. Consider factoring out into a shared component to reduce duplication and centralize logic, especially for columns like ID, repository, commit hash, etc.
276-308: Good implementation of search and filter functionality.The search and filter implementation with clear visual feedback (button color change when filters are applied) enhances usability.
509-526: Good implementation of pagination controls.The pagination implementation with item count and page navigation controls follows best practices and ensures good user experience when dealing with large datasets.
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (1)
341-346: Handle invalid or out-of-range paging inputs.If users manually set the URL page to a negative or zero value, the code calls
loadFinishedBuildJobsanyway. Consider adding validation.onPageChange(pageNumber: number) { - if (pageNumber) { + if (pageNumber && pageNumber > 0 && pageNumber <= Math.ceil(this.totalItems / this.itemsPerPage)) { this.page = pageNumber; this.loadFinishedBuildJobs(); } }
...alci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html
Show resolved
Hide resolved
...ocalci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts (4)
37-37: Consider returning a more representative mock value.This mock function currently returns an empty object. If feasible, return an array of finished build jobs (or relevant mock data) so the test can verify component behavior more thoroughly.
- getFinishedBuildJobs: jest.fn().mockReturnValue(of({})), + getFinishedBuildJobs: jest.fn().mockReturnValue(of([])),
114-125: Consider testing partial or complex filters.While
filterOptionsEmptyis a good baseline, it might be beneficial to add a dedicated test for partial or combined filter criteria to ensure filter logic is thoroughly validated.
317-329: Use fakeAsync to avoid real timers if possible.Relying on
setTimeoutand manual delays can lead to flaky tests. Consider using Angular testing utilities likefakeAsyncandtickto handle debounced or async logic more reliably.
331-342: Add edge-case coverage.This test verifies correct duration assignment when dates are present. You might also include negative or missing date scenarios to ensure robust validation.
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (3)
235-246: Consider catching modal dismissal errors or logs.Currently,
.catch(() => {})silently ignores modal close/dismiss events. If there's a meaningful scenario to handle upon modal dismissal, consider logging or graceful fallback.
251-262: Consider storing subscription for manual cleanup.
loadFinishedBuildJobs()calls.subscribedirectly. If you expect the component to be destroyed while requests are mid-flight, storing the subscription and unsubscribing inngOnDestroyis a safer approach.
279-289: Build job duration calculation is valid.You convert the difference from milliseconds to seconds with three decimal points. If you require rounding or truncation, ensure consistent formatting (e.g.,
toFixedmay round).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(5 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(10 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts(0 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(3 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts(7 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts(5 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(5 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(4 hunks)src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
- src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
🧰 Additional context used
📓 Path-based instructions (2)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts
🪛 Biome (1.9.4)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
[error] 360-360: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (18)
src/test/javascript/spec/component/localci/build-agents/build-agent-details.component.spec.ts (7)
5-5: Imports look fine.The newly introduced imports for
FinishedBuildJob,HttpResponse, andSortingOrderappear correct and align well with the test enhancements for finished build jobs.Also applies to: 18-19
106-112: Test request object initialization looks good.All fields (page, pageSize, sortedColumn, sortingOrder, and searchTerm) are suitably defined and will help in verifying pagination and sorting logic.
127-163: Mock finished jobs data is consistent.The mock data for finished build jobs covers relevant fields. The structure seems appropriate for testing and should facilitate robust assertions.
165-166: Use of HttpResponse is appropriate.This response wrapper correctly simulates server-returned data. No issues found.
169-173: Mock build queue service is properly defined.The added methods align with the test requirements and make the component’s interactions testable.
183-183: Injecting the mock build queue service.This ensures consistent usage across tests for finished build jobs. Implementation is correct.
197-199: Mock return values ensure controlled test environment.Returning observables of empty objects or mock data helps isolate and verify component logic.
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (11)
4-5: New RxJS imports and icons appear correct.The
Subject,Subscription, and associated operators are correctly referenced to implement debounced search logic, and newly added FontAwesome icons match usage patterns.
20-33: All newly imported symbols for job models, directives, pagination, etc. are appropriate.These imports align with the new functionalities around finished build jobs, filtering, and pagination. No issues found.
66-66: Injecting the modal service is correct.This allows opening the filter modal from within the component as needed.
78-78: Initializing finished build jobs array.The dedicated array for storing finished jobs is appropriate, matching the new logic introduced for job retrieval and display.
88-89: Added icons for sort and sync.No concerns; properly used for displaying relevant actions in the UI.
91-103: Unsubscribe from the search subscription to avoid memory leaks.Although multiple subscriptions are unsubscribed in
ngOnDestroy, you should also unsubscribe fromsearchSubscription. This was flagged in a previous review as well.ngOnDestroy() { ... + this.searchSubscription?.unsubscribe(); }
109-112: Chaining initialization on param subscription.Invoking
loadInitialDetails, setting up websockets, and loading finished build jobs together follows a logical flow. No issues found.
270-274: Success handler is straightforward.Updating
totalItemsand finished build jobs, then computing durations, is well-defined. No concerns found.
294-305: Fetching finished jobs through the service.This centralized method fosters clarity. No issues found.
310-314: Debounce strategy for loading jobs is good.Triggering searches only if the term is empty or sufficiently long helps performance. Nice approach.
317-326: Bound page numbers to avoid invalid values.As previously suggested, you can guard against negative or zero input in
onPageChange. This issue was raised in an earlier review and still applies.onPageChange(pageNumber: number) { + if (pageNumber <= 0) { + pageNumber = 1; + } this.page = pageNumber; this.loadFinishedBuildJobs(); }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (1)
318-327: Filter Modal Button – Remove RedundantthisReference.
In the filter modal button markup (lines 318–327), the click handler is bound as(click)="this.openFilterModal()". In Angular templates, it’s conventional to reference component methods directly without thethis.prefix.A quick fix diff:
- (click)="this.openFilterModal()" + (click)="openFilterModal()"Consider updating this for consistency and clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html(2 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html(2 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html(1 hunks)src/main/webapp/i18n/de/buildAgents.json(1 hunks)src/main/webapp/i18n/en/buildAgents.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html
- src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html
🧰 Additional context used
📓 Path-based instructions (2)
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html
`src/main/webapp/i18n/de/**/*.json`: German language transla...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/buildAgents.json
🔇 Additional comments (9)
src/main/webapp/i18n/de/buildAgents.json (1)
13-16: New Build Agent Metrics Localization Keys AddedDie neuen Schlüssel
"startDate","lastBuildDate","averageBuildDuration"und"gitRevision"mit den Übersetzungen "Startdatum", "Letztes Build Datum", "Durchschnittliche Build-Dauer (in Sekunden)" und "Git Revision" wurden erfolgreich eingefügt. Diese Ergänzungen unterstützen die erweiterte Anzeige von Build-Agent-Metadaten und entsprechen der informellen Ansprache gemäß den Richtlinien.src/main/webapp/i18n/en/buildAgents.json (1)
13-16: Consistent Addition of Build Agent Metrics Localization KeysThe new keys
"startDate","lastBuildDate","averageBuildDuration", and"gitRevision"have been added with clear and concise values. This update aligns perfectly with the new data structure provided by the updatedBuildAgentDetailsDTOand supports the enhanced display of build agent details.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html (7)
4-4: Flex container in header looks good.
The use of Bootstrap’s flex classes on line 4 appropriately sets up alignment and spacing for the build agent’s title area.
27-57: Enhanced Build Agent Details Section.
This block introduces the new build agent details layout with additional metrics such as latest build date, average build duration, git revision, and start date. The integration of the new<jhi-build-job-statistics>component (line 28) and the clear segmentation using flex containers provide a more detailed and organized view.A couple of suggestions:
• Consider using a formatting pipe or unit indicator for the average build duration if it’s a raw numeric value (as done with dates via theartemisDatepipe).
• Verify that the data bound viabuildAgent.buildAgentDetailsis always present to avoid potential null/undefined issues.
59-308: Robust Running Build Jobs Data Table.
This extensive block (lines 59–308) shows a well-structured data table for running build jobs using the new Angular structural directives (@ifand@for). The table is detailed—with sortable columns and conditional styling (e.g. flagging build durations over 240 seconds).Points to consider:
• There is a significant amount of markup repeated here. Since the running build jobs table and the finished build jobs table share many similarities, it might be beneficial to extract a shared job-table component in the future to adhere to DRY principles.
• Also, consider adding ARIA attributes or additional accessibility enhancements where practical.
312-317: Recent Finished Build Jobs Header.
The header for the recent build jobs section (lines 312–317) is clearly defined with an appropriate heading and a divider. This visually separates the finished jobs section from the running jobs.
329-330: Structural Divider.
The divider on lines 329–330 is correctly implemented to separate sections. No changes are necessary here.
331-530: Comprehensive Finished Build Jobs Data Table.
This block (lines 331–530) implements the finished build jobs table with pagination, sorting, and conditional rendering using the new Angular@ifand@fordirectives. Notable points include:• The commit history links now correctly pass
finishedBuildJob.commitHashas per previous feedback, ensuring accurate routing parameters.
• The overall structure (including use of<ngb-pagination>and<jhi-item-count>) reinforces good practices by providing clear navigational aids.As with the running jobs table, there is considerable duplication. Consider refactoring common table logic into a shared component if similar patterns appear across different tables.
531-533: Closing Container Tags.
The closing tags (lines 531–533) properly wrap up the component’s markup. No issues noted.
...alci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.html
Show resolved
Hide resolved
...ocalci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
Outdated
Show resolved
Hide resolved
…-build-details-view # Conflicts: # src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java # src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java # src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java # src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java # src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java # src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (1)
108-169: Consider extracting build statistics computation
Several methods here compute different build stats (avg duration, success/failure counts, etc.). For better maintainability and single responsibility, consider factoring them out into a dedicated helper/service or aggregator class.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (1)
353-369: Use optional chaining for clarity
To further reduce null checks, consider directly using optional chaining, e.g.,buildJob.jobTimingInfo?.buildStartDate.🧰 Tools
🪛 Biome (1.9.4)
[error] 361-361: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(10 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/DistributedDataAccessService.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java(1 hunks)src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts(7 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/DistributedDataAccessService.java
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIResultProcessingService.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIQueueWebsocketService.java
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIService.java
- src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminBuildJobQueueResource.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
🧰 Additional context used
📓 Path-based instructions (2)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
🪛 Biome (1.9.4)
src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts
[error] 361-361: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: server-tests
- GitHub Check: client-tests
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (28)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentInformationService.java (3)
70-85: Consider rethrowing or handling exceptions more robustly
This block locks the Hazelcast map and updates build agent information, but exceptions are merely logged and then swallowed. In some cases, higher-level components may need to be informed of locking or update failures.
132-134: Verify that the completion date is not null
IfrecentBuildJob.jobTimingInfo().buildCompletionDate()is null for a job in progress,Duration.between()may yield unexpected behavior. Consider safeguarding against a null or in-progress completion date.
88-107: Looks good!
Your approach to constructing a newBuildAgentInformationusing local states, recentJob, and distributed data is clear and follows constructor-based dependency injection.src/main/webapp/app/localci/build-agents/build-agent-details/build-agent-details/build-agent-details.component.ts (25)
4-5: Import changes look good
No issues spotted with newly added RxJS and FontAwesome imports.
19-20: Importing BuildJobStatisticsComponent
Good addition for encapsulating build job statistics functionality.
25-27: Pagination and modal imports
Clean integration of pagination constants andFinishedBuildsFilterModalComponent.
66-66: Modal service injection
InjectingNgbModalis inline with Angular best practices.
70-71: Initialization of new statistics and build jobs
DeclaringbuildJobStatisticsandrunningBuildJobsclarifies the default state.
73-77: Subscription fields
Declaring separate subscription fields improves unsubscribing and helps avoid memory leaks.
80-81: New WebSocket channels
Defining constants for agent and running jobs channels is more readable than inline strings.
83-83: finishedBuildJobs variable
Straightforward initialization.
85-90: Icon references
Icon usage is consistent with FontAwesome best practices.
93-94: faSort and faSync
No concerns about these icon additions.
96-104: Filter and search fields
Properly structured approach for storing search-related values and loading states.
110-116: Periodic duration updates
Running an interval for job duration refresh is acceptable as long as performance remains stable for the environment.
119-124: Unsubscribe from search subscription
While this implements a search subscription with debounce, it is not unsubscribed inngOnDestroy, risking a memory leak. Please unsubscribe it.
143-147: Multiple subscriptions unsubscribed, but missing search
agentDetailsWebsocketSubscription,runningJobsWebsocketSubscription, and others are unsubscribed, butthis.searchSubscriptionis never unsubscribed, which can cause leaks.
155-168: WebSocket subscription method
Establishing separate channels and subscriptions is well-structured and unsubscribed properly, aside from the noted search subscription.
175-182: Consolidated loading
Fetching running jobs and agent details in one flow leads to a coherent initialization process.
185-195: updateBuildAgent
PopulatingbuildJobStatisticsfrombuildAgent.buildAgentDetailsis straightforward and consistent.
197-197: Refined signatures
Using astringforbuildJobIdcan reduce potential confusion with numeric IDs.
260-271: Filter modal integration
Opening the modal and returning the updated filter result is a clean approach to filtering.
273-287: loadFinishedBuildJobs
Clear separation of fetching logic. Error handling delegates to a shared utility (onError).
289-299: onSuccess
Properly extracting headers and updating the local array.
301-315: setFinishedBuildJobsDuration
Well-implemented approach usingdayjsfor computing differences.
316-330: fetchFinishedBuildJobs
Nicely implemented fetch process with query params.
332-339: triggerLoadFinishedJobs
This is a good approach for on-demand searching, which avoids excessive requests.
341-351: onPageChange
Straightforward pagination logic.
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
We want to collect more information about build agents.
We also want to remove the recentBuildJobs from the distributed map and fetch them from the db.
Description
On the server side, we extended the dtos the include the new information. We also moved most of the build agent information logic to a separate service (as SharedQueueProcessingService is getting too big)
On the client side:
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Style & Localization
Refactor