General: Migrate test run module to signals, inject and standalone
#9914
Conversation
github-actions
bot
added
the
client
coolchock
changed the title
Chore: Migrate test run module to signals, inject and standaloneGeneral: Migrate test run module to signals, inject and standalone
coolchock
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🧹 Outside diff range and nitpick comments (4)
src/main/webapp/app/exam/manage/test-runs/create-test-run-modal.component.ts (1)
19-24: Consider implementing OnDestroy for proper cleanupWhile the component looks good, it's handling exam data and form controls which should be properly cleaned up.
Consider implementing OnDestroy:
-export class CreateTestRunModalComponent implements OnInit { +export class CreateTestRunModalComponent implements OnInit, OnDestroy { + private destroy$ = new Subject<void>(); + + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }src/main/webapp/app/exam/manage/test-runs/test-run-management.component.ts (3)
Line range hint
38-39: Implement proper cleanup for dialogErrorSource SubjectThe dialogErrorSource Subject needs proper cleanup to prevent memory leaks.
Add cleanup in ngOnDestroy:
+ private destroy$ = new Subject<void>(); private dialogErrorSource = new Subject<string>(); dialogError$ = this.dialogErrorSource.asObservable(); + ngOnDestroy(): void { + this.dialogErrorSource.complete(); + this.destroy$.next(); + this.destroy$.complete(); + }
Line range hint
51-67: Use destroy$ Subject to manage subscription cleanupThe HTTP subscriptions in ngOnInit should be properly managed to prevent memory leaks.
Implement proper subscription management:
ngOnInit(): void { - this.examManagementService.find(/*...*/).subscribe({ + this.examManagementService.find(/*...*/) + .pipe(takeUntil(this.destroy$)) + .subscribe({ next: (response: HttpResponse<Exam>) => { this.exam = response.body!; this.isExamStarted = this.exam.started!; this.course = this.exam.course!; this.course.isAtLeastInstructor = this.accountService.isAtLeastInstructorInCourse(this.course); - this.examManagementService.findAllTestRunsForExam(/*...*/).subscribe({ + this.examManagementService.findAllTestRunsForExam(/*...*/) + .pipe(takeUntil(this.destroy$)) + .subscribe({ next: (res: HttpResponse<StudentExam[]>) => { this.testRuns = res.body!; },
Line range hint
89-93: Consider using async pipe for dialogError$Since you're using an Observable for dialog errors, consider leveraging the async pipe in the template for automatic subscription management.
Update the template to use async pipe:
<div *ngIf="dialogError$ | async as error" class="alert alert-danger"> {{ error }} </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/exam/manage/test-runs/create-test-run-modal.component.ts(2 hunks)src/main/webapp/app/exam/manage/test-runs/test-run-management.component.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/exam/manage/test-runs/create-test-run-modal.component.ts (1)
src/main/webapp/app/exam/manage/test-runs/test-run-management.component.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/exam/manage/test-runs/create-test-run-modal.component.ts (1)
1-1: LGTM: Clean migration to inject() pattern
The migration from constructor injection to the new inject() pattern is well implemented. This change:
- Reduces boilerplate code
- Follows modern Angular practices
- Maintains proper encapsulation with private members
Also applies to: 17-18
src/main/webapp/app/exam/manage/test-runs/test-run-management.component.ts (1)
23-28: LGTM: Clean service injection implementation
The migration to inject() for all services is well implemented and follows a consistent pattern.
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
SimonEntholzer
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
src/main/webapp/app/exam/manage/test-runs/create-test-run-modal.component.ts
Outdated
Show resolved
Hide resolved
| course: Course; | ||
| exam: Exam; | ||
| isLoading: boolean; |
There was a problem hiding this comment.
can you use signals for the variables here?
src/main/webapp/app/exam/manage/test-runs/test-run-management.component.ts
Show resolved
Hide resolved
sawys777
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
kevinfischer4
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
coolchock
dismissed stale reviews from kevinfischer4, sawys777, HawKhiem, and coderabbitai[bot]
via
1b785d9
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/main/webapp/app/exam/manage/test-runs/test-run-management.component.ts (1)
Line range hint
54-71: Improve subscription management in ngOnInitThe nested subscriptions could lead to memory leaks. Consider using RxJS operators to flatten the observable chain.
ngOnInit(): void { - this.examManagementService.find(Number(this.route.snapshot.paramMap.get('courseId')), Number(this.route.snapshot.paramMap.get('examId')), false, true).subscribe({ - next: (response: HttpResponse<Exam>) => { - this.exam = response.body!; - this.isExamStarted = this.exam.started!; - this.course = this.exam.course!; - this.course.isAtLeastInstructor = this.accountService.isAtLeastInstructorInCourse(this.course); - this.examManagementService.findAllTestRunsForExam(this.course.id!, this.exam.id!).subscribe({ - next: (res: HttpResponse<StudentExam[]>) => { - this.testRuns = res.body!; - }, - error: (error: HttpErrorResponse) => onError(this.alertService, error), - }); - }, - error: (error: HttpErrorResponse) => onError(this.alertService, error), - }); + this.examManagementService + .find(Number(this.route.snapshot.paramMap.get('courseId')), Number(this.route.snapshot.paramMap.get('examId')), false, true) + .pipe( + tap((response: HttpResponse<Exam>) => { + this.exam.set(response.body!); + this.isExamStarted.set(response.body!.started!); + this.course.set(response.body!.course!); + this.course().isAtLeastInstructor = this.accountService.isAtLeastInstructorInCourse(this.course()!); + }), + switchMap((response: HttpResponse<Exam>) => + this.examManagementService.findAllTestRunsForExam(response.body!.course!.id!, response.body!.id!) + ), + catchError((error) => { + onError(this.alertService, error); + return EMPTY; + }) + ) + .subscribe({ + next: (res: HttpResponse<StudentExam[]>) => { + this.testRuns.set(res.body!); + }, + error: (error: HttpErrorResponse) => onError(this.alertService, error), + });
🧹 Nitpick comments (2)
src/main/webapp/app/exam/manage/test-runs/test-run-ribbon.component.ts (1)
Line range hint
1-16: LGTM! Clean standalone component migrationThe migration to a standalone component is well-implemented with proper translation support. The component follows Angular style guidelines and effectively reuses shared styles.
Consider adding a component description using JSDoc to improve documentation:
/** * Displays a ribbon indicating that the current context is a test run. * Used in exam management to visually distinguish test runs from actual exams. */ @Component({src/main/webapp/app/exam/manage/test-runs/test-run-management.component.ts (1)
Line range hint
82-102: Improve error handling in modal interactionThe error handling in the modal interaction could be more consistent with the rest of the application.
openCreateTestRunModal() { const modalRef: NgbModalRef = this.modalService.open(CreateTestRunModalComponent as Component, { size: 'lg', backdrop: 'static' }); modalRef.componentInstance.exam = this.exam; modalRef.result .then((testRunConfiguration: StudentExam) => { - this.examManagementService.createTestRun(this.course.id!, this.exam.id!, testRunConfiguration).subscribe({ - next: (response: HttpResponse<StudentExam>) => { - if (response.body != undefined) { - this.testRuns.push(response.body!); - } - }, - error: (error: HttpErrorResponse) => { - onError(this.alertService, error); - }, - }); + this.examManagementService + .createTestRun(this.course().id!, this.exam().id!, testRunConfiguration) + .pipe( + catchError((error) => { + onError(this.alertService, error); + return EMPTY; + }) + ) + .subscribe((response: HttpResponse<StudentExam>) => { + if (response.body) { + this.testRuns.update(runs => [...runs, response.body!]); + } + }); }) .catch(() => {}); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/webapp/app/exam/manage/exam-management.module.ts(0 hunks)src/main/webapp/app/exam/manage/test-runs/create-test-run-modal.component.ts(1 hunks)src/main/webapp/app/exam/manage/test-runs/test-run-management.component.ts(3 hunks)src/main/webapp/app/exam/manage/test-runs/test-run-ribbon.component.ts(2 hunks)src/main/webapp/app/exam/participate/exam-participation.module.ts(2 hunks)src/main/webapp/app/exam/participate/summary/exam-result-summary.module.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/webapp/app/exam/manage/exam-management.module.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/exam/manage/test-runs/create-test-run-modal.component.ts
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/exam/participate/exam-participation.module.ts (1)
src/main/webapp/app/exam/participate/summary/exam-result-summary.module.ts (1)
src/main/webapp/app/exam/manage/test-runs/test-run-ribbon.component.ts (1)
src/main/webapp/app/exam/manage/test-runs/test-run-management.component.ts (1)
🔇 Additional comments (4)
src/main/webapp/app/exam/participate/exam-participation.module.ts (1)
35-36: LGTM! Proper integration of standalone component
The TestRunRibbonComponent is correctly imported and added to the module's imports array, following Angular's standalone component integration pattern.
Let's verify that the component is actually used in this module's context:
Also applies to: 69-69
✅ Verification successful
Component integration verified successfully
The TestRunRibbonComponent is properly imported and actively used in the exam-participation.component.html template with the correct selector jhi-test-run-ribbon. The component is conditionally rendered when testRunId is present, which aligns with its intended usage pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TestRunRibbonComponent selector usage
rg -l "jhi-test-run-ribbon" "src/main/webapp/app/exam/participate/"
Length of output: 136
Script:
#!/bin/bash
# Check the actual usage of the selector in the HTML file
rg "jhi-test-run-ribbon" "src/main/webapp/app/exam/participate/exam-participation.component.html" -C 2
Length of output: 182
src/main/webapp/app/exam/manage/test-runs/test-run-management.component.ts (3)
Line range hint 1-26: LGTM! Good transition to standalone component
The component has been properly configured as standalone with appropriate imports and dependencies.
28-34: LGTM! Clean dependency injection implementation
Good use of the new inject() function pattern for dependency injection.
Line range hint 35-49: Consider using signals for reactive state management
Properties that represent component state could benefit from using Angular signals for better reactivity and performance. This aligns with the previous review comment and modern Angular practices.
Consider refactoring the state properties to use signals:
- course: Course;
- exam: Exam;
- isLoading: boolean;
- isExamStarted: boolean;
- testRuns: StudentExam[] = [];
- instructor?: User;
+ course = signal<Course | undefined>(undefined);
+ exam = signal<Exam | undefined>(undefined);
+ isLoading = signal<boolean>(false);
+ isExamStarted = signal<boolean>(false);
+ testRuns = signal<StudentExam[]>([]);
+ instructor = signal<User | undefined>(undefined);This would provide better reactivity and change detection optimization.
| ExampleSolutionComponent, | ||
| CollapsibleCardComponent, | ||
| ], | ||
| exports: [ExamResultSummaryComponent, ExamGeneralInformationComponent, TestRunRibbonComponent], | ||
| exports: [ExamResultSummaryComponent, ExamGeneralInformationComponent], |
There was a problem hiding this comment.
💡 Codebase verification
Component removal requires updates in exam-participation module
The removal of TestRunRibbonComponent from exam-result-summary.module.ts is incomplete. The component is still being used in:
exam-participation.component.html: Uses the component selector<jhi-test-run-ribbon>exam-participation.module.ts: Still imports the component from its old location
Since TestRunRibbonComponent is now standalone, these files need to be updated to:
- Import the standalone component directly in exam-participation.module.ts
- Update the template usage if the selector has changed with the standalone migration
🔗 Analysis chain
LGTM! Clean removal of component from module
The removal of TestRunRibbonComponent from declarations and exports is correct as part of the standalone migration.
Let's verify there are no remaining references to this component in the summary module's context:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to TestRunRibbonComponent in summary context
rg -l "TestRunRibbonComponent|jhi-test-run-ribbon" "src/main/webapp/app/exam/participate/summary/"
# Search for potential imports that might need updating
rg -l "from.*test-run-ribbon" "src/main/webapp/app/exam/participate/summary/"
Length of output: 176
Script:
#!/bin/bash
# Let's broaden the search to check for any references in the entire webapp directory
rg -l "TestRunRibbonComponent|jhi-test-run-ribbon" "src/main/webapp/"
# Also check for any template usage with the component selector
rg "<jhi-test-run-ribbon" "src/main/webapp/"
# Check for any remaining imports
rg "import.*TestRunRibbonComponent" "src/main/webapp/"
Length of output: 655
Checklist
General
Client
Description
This pull request migrates test runs module to signals, inject and standalone.
Steps for Testing
Prerequisites:
Create a Test RunTest Run Managementpage workTestserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Summary by CodeRabbit
New Features
injectfunction.Bug Fixes
Refactor
Chores