Development: Migrate text module to use inject
#9772
Conversation
github-actions
bot
added
the
client
FelixTJDietrich
added
code quality
refactoring
text
FelixTJDietrich
added
fileupload
WalkthroughThis pull request implements a significant refactor across multiple components and services in the Angular application, transitioning from constructor-based dependency injection to the use of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/main/webapp/app/exercises/text/assess/textblock-feedback-editor/textblock-feedback-editor.component.tsOops! Something went wrong! :( ESLint: 9.14.0 TypeError: Error while loading rule '@typescript-eslint/no-unused-expressions': Cannot read properties of undefined (reading 'allowShortCircuit') 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: 7
🧹 Outside diff range and nitpick comments (47)
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-paging.service.ts (1)
10-13: Consider using property initialization for a more concise implementation.While the current implementation is correct, it can be made more concise by using property initialization with
inject.Here's a more concise implementation:
- constructor() { - const http = inject(HttpClient); - super(http, TextExercisePagingService.RESOURCE_URL); - } + constructor() { + super(inject(HttpClient), TextExercisePagingService.RESOURCE_URL); + }This change:
- Reduces the number of lines
- Eliminates the need for a temporary variable
- Maintains the same functionality
src/main/webapp/app/exercises/text/participate/text-editor.service.ts (3)
Line range hint
1-10: LGTM! Consider grouping imports by category.The import statements and service declaration follow Angular best practices. The addition of the
injectimport alongsideInjectableis correct for the new DI pattern.Consider organizing imports into these groups with a blank line between each:
- Angular framework
- Third-party libraries
- Application imports
import { Injectable, inject } from '@angular/core'; import { HttpClient } from '@angular/common/http'; import { Observable, tap } from 'rxjs'; + import { francAll } from 'franc-min'; + import { Language } from 'app/entities/course.model'; import { StudentParticipation } from 'app/entities/participation/student-participation.model'; import { ExerciseService } from 'app/exercises/shared/exercise/exercise.service';
Line range hint
13-17: Add JSDoc documentation for the get method.For consistency with the rest of the codebase and better maintainability, consider adding documentation for the get method.
+ /** + * Retrieves student participation data for a given participation ID + * @param {number} participationId - The ID of the participation to fetch + * @returns {Observable<StudentParticipation>} Observable of the student participation data + */ get(participationId: number): Observable<StudentParticipation> {
Line range hint
24-33: Enhance type safety in language detection.The current implementation could be improved to handle edge cases and provide better type safety.
Consider these improvements:
- Add type for the franc-all library return type
- Handle empty text input
- Add a default return value
- predictLanguage(text: string): Language | undefined { + predictLanguage(text: string): Language | undefined { + if (!text?.trim()) { + return undefined; + } + const languageProbabilities = francAll(text); + // Ensure we have results + if (!languageProbabilities?.length) { + return undefined; + } switch (languageProbabilities[0][0]) { case 'eng': return Language.ENGLISH; case 'deu': return Language.GERMAN; + default: + return undefined; } }src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-row-buttons.component.ts (1)
15-16: LGTM: Clean service injection implementationThe migration to
inject()is implemented correctly. Consider adding readonly modifier to prevent accidental reassignment:- private eventManager = inject(EventManager); - private textExerciseService = inject(TextExerciseService); + private readonly eventManager = inject(EventManager); + private readonly textExerciseService = inject(TextExerciseService);src/main/webapp/app/exercises/text/shared/manual-text-selection/manual-text-selection.component.ts (2)
24-25: Consider using readonly and ngOnInit for better practicesWhile the inject implementation is correct, consider these improvements:
- Mark injected services as readonly to prevent accidental reassignment
- Move initialization to ngOnInit lifecycle hook for better predictability
- protected route = inject(ActivatedRoute); - private textAssessmentAnalytics = inject(TextAssessmentAnalytics); + protected readonly route = inject(ActivatedRoute); + private readonly textAssessmentAnalytics = inject(TextAssessmentAnalytics);
Line range hint
24-41: Implement OnDestroy for proper cleanupThe component uses services that may create subscriptions (ActivatedRoute via textAssessmentAnalytics). To prevent memory leaks, implement proper cleanup:
-export class ManualTextSelectionComponent { +export class ManualTextSelectionComponent implements OnInit, OnDestroy { + private readonly destroy$ = new Subject<void>(); + constructor() {} + + ngOnInit(): void { + this.textAssessmentAnalytics.setComponentRoute(this.route); + } + + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }Then ensure any subscriptions in textAssessmentAnalytics are properly cleaned up using this destroy$ subject.
src/main/webapp/app/exercises/text/assess/textblock-assessment-card/textblock-assessment-card.component.ts (3)
1-1: LGTM! Consider grouping related imports.The migration to using the
injectfunction is well implemented. The service injections are properly scoped and follow naming conventions.Consider grouping related imports together for better organization:
-import { Component, EventEmitter, Input, Output, ViewChild, inject } from '@angular/core'; +import { Component, EventEmitter, Input, Output, ViewChild } from '@angular/core'; +import { inject } from '@angular/core';Also applies to: 20-22
Line range hint
44-57: Prevent potential memory leaks from setTimeoutThe
selectmethod usessetTimeoutwithout cleanup. This could lead to memory leaks if the component is destroyed while the timeout is pending.+private timeoutRef: number | undefined; + select(autofocus = true): void { if (this.readOnly) { return; } if (this.textBlockRef && !this.textBlockRef.selectable) { return; } this.didSelect.emit(this.textBlockRef); this.textBlockRef.initFeedback(); if (autofocus) { - setTimeout(() => this.feedbackEditor.focus()); + this.timeoutRef = window.setTimeout(() => this.feedbackEditor.focus()); if (!this.selected && this.textBlockRef.feedback?.type === FeedbackType.MANUAL) { this.textAssessmentAnalytics.sendAssessmentEvent(TextAssessmentEventType.ADD_FEEDBACK_AUTOMATICALLY_SELECTED_BLOCK, FeedbackType.MANUAL, TextBlockType.AUTOMATIC); } } } + +ngOnDestroy(): void { + if (this.timeoutRef) { + window.clearTimeout(this.timeoutRef); + } +}
Line range hint
44-57: Enhance type safety for better maintainabilityConsider adding stricter typing for the
autofocusparameter and the setTimeout callback.-select(autofocus = true): void { +select(autofocus: boolean = true): void { // ... if (autofocus) { - this.timeoutRef = window.setTimeout(() => this.feedbackEditor.focus()); + this.timeoutRef = window.setTimeout((): void => { + if (this.feedbackEditor) { + this.feedbackEditor.focus(); + } + }); } }src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.component.ts (3)
23-30: Consider grouping service injections by access modifier.While the service injections are correctly implemented using the new
inject()function, consider organizing them by access modifier for improved readability:// Protected services protected exerciseService = inject(ExerciseService); protected textExerciseService = inject(TextExerciseService); // Private services private router = inject(Router); private courseExerciseService = inject(CourseExerciseService); private modalService = inject(NgbModal); private alertService = inject(AlertService); private sortService = inject(SortService); private accountService = inject(AccountService);
Line range hint
44-61: Consider implementing proper subscription management.The
loadExercisesmethod creates a subscription that should be properly managed to prevent memory leaks.+ private destroy$ = new Subject<void>(); protected loadExercises(): void { this.courseExerciseService.findAllTextExercisesForCourse(this.courseId) + .pipe(takeUntil(this.destroy$)) .subscribe({ // ... existing implementation }); } + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }
Line range hint
91-98: Add error handling to modal result.The
openImportModalmethod should handle potential modal dismissal or rejection.openImportModal() { const modalRef = this.modalService.open(ExerciseImportWrapperComponent, { size: 'lg', backdrop: 'static' }); modalRef.componentInstance.exerciseType = ExerciseType.TEXT; - modalRef.result.then((result: TextExercise) => { - this.router.navigate(['course-management', this.courseId, 'text-exercises', result.id, 'import']); - }); + modalRef.result.then( + (result: TextExercise) => { + this.router.navigate(['course-management', this.courseId, 'text-exercises', result.id, 'import']); + }, + (reason) => { + // Handle modal dismissal + console.debug('Modal dismissed', reason); + } + ); }src/main/webapp/app/exercises/text/assess/analytics/text-assesment-analytics.service.ts (1)
Line range hint
31-36: Add error handling to the ProfileService subscription.The subscription to
profileService.getProfileInfo()lacks error handling, which could lead to silent failures.Consider adding error handling:
constructor() { this.profileService.getProfileInfo().subscribe({ next: (profileInfo) => { this.analyticsEnabled = profileInfo.textAssessmentAnalyticsEnabled || false; }, error: (error) => { console.error('Failed to fetch profile info:', error); this.analyticsEnabled = false; // fallback to safe default } }); }src/main/webapp/app/exercises/text/participate/text-submission.service.ts (1)
15-16: Consider grouping injected dependencies with a descriptive commentThe migration to using the
inject()function is correct and follows Angular's modern patterns. Consider adding a descriptive comment to group the injected dependencies for better code organization:+ // Injected dependencies private http = inject(HttpClient); private submissionService = inject(SubmissionService); +src/main/webapp/app/exercises/modeling/manage/modeling-exercise.component.ts (2)
20-25: Consider grouping service declarations by access levelThe migration to
inject()is well implemented. However, consider organizing the service declarations by access level for better readability:// Protected services protected exerciseService = inject(ExerciseService); protected modelingExerciseService = inject(ModelingExerciseService); // Private services private courseExerciseService = inject(CourseExerciseService); private alertService = inject(AlertService); private accountService = inject(AccountService); private sortService = inject(SortService);
27-27: Consider using a more specific type annotationWhile the default initialization is good, consider making the type annotation more specific to prevent potential runtime errors:
-@Input() modelingExercises: ModelingExercise[] = []; +@Input() modelingExercises: readonly ModelingExercise[] = [];This change would:
- Prevent accidental modifications to the input array
- Better communicate the intent that this is an input-only property
src/main/webapp/app/exercises/text/assess/text-submission-assessment.route.ts (1)
Line range hint
41-54: Consider breaking down the resolve method for better maintainabilityThe resolve method handles multiple scenarios with nested conditions. Consider extracting the logic into separate private methods for better readability and maintainability.
Here's a suggested refactor:
private resolveByResultId(submissionId: number, resultId: number) { return this.textAssessmentService .getFeedbackDataForExerciseSubmission(submissionId, undefined, resultId) .pipe(catchError(() => of(undefined))); } private resolveBySubmissionId(submissionId: number, correctionRound: number) { return this.textAssessmentService .getFeedbackDataForExerciseSubmission(submissionId, correctionRound) .pipe(catchError(() => of(undefined))); } resolve(route: ActivatedRouteSnapshot) { const submissionId = Number(route.paramMap.get('submissionId')); const correctionRound = Number(route.queryParamMap.get('correction-round')); const resultId = Number(route.paramMap.get('resultId')); if (resultId) { return this.resolveByResultId(submissionId, resultId); } if (submissionId) { return this.resolveBySubmissionId(submissionId, correctionRound); } return of(undefined); }src/main/webapp/app/exercises/file-upload/manage/file-upload-exercise.component.ts (3)
Line range hint
89-99: Fix missing eventManager injectionThe
eventManageris used in thedeleteFileUploadExercisemethod but it wasn't properly injected using the newinject()pattern. This could lead to runtime errors.Add the following injection:
+ private eventManager = inject(EventManager);
Line range hint
89-99: Improve error handling in deleteFileUploadExerciseThe error handling in
deleteFileUploadExerciseuses adialogErrorSourcewhich seems to be a Subject for error management, but consider using the injectedalertServicefor consistency with other error handling in the component.Consider refactoring to:
deleteFileUploadExercise(fileUploadExerciseId: number) { this.fileUploadExerciseService.delete(fileUploadExerciseId).subscribe({ next: () => { this.eventManager.broadcast({ name: 'fileUploadExerciseListModification', content: 'Deleted an fileUploadExercise', }); - this.dialogErrorSource.next(''); }, - error: (error: HttpErrorResponse) => this.dialogErrorSource.next(error.message), + error: (error: HttpErrorResponse) => onError(this.alertService, error), }); }
17-20: Consider implementing OnDestroy interfaceSince this component manages subscriptions and extends from
ExerciseComponent, it should implement theOnDestroyinterface to ensure proper cleanup of resources.Consider adding:
- export class FileUploadExerciseComponent extends ExerciseComponent { + export class FileUploadExerciseComponent extends ExerciseComponent implements OnDestroy { + ngOnDestroy() { + // Cleanup subscriptions + super.ngOnDestroy(); + }src/main/webapp/app/exercises/shared/exercise/exercise.component.ts (1)
Line range hint
37-38: Prevent potential memory leak in dialogErrorSourceThe
dialogErrorSourceSubject should be completed inngOnDestroyto prevent memory leaks. Currently, only unsubscribe is called which is not sufficient for a Subject.Apply this change to properly clean up the Subject:
ngOnDestroy() { this.eventManager.destroy(this.eventSubscriber); - this.dialogErrorSource.unsubscribe(); + this.dialogErrorSource.complete(); }src/main/webapp/app/exercises/text/participate/text-result/text-result.component.ts (5)
1-1: LGTM! Consider grouping related imports.The migration to using the
injectfunction is well implemented. The services are correctly injected as private class properties, which aligns with modern Angular practices and simplifies dependency management.Consider grouping related imports together for better organization:
import { Component, Input, inject } from '@angular/core'; +import { TranslateService } from '@ngx-translate/core'; +import { LocaleConversionService } from 'app/shared/service/locale-conversion.service'; import { Feedback, buildFeedbackTextForReview, checkSubsequentFeedbackInAssessment } from 'app/entities/feedback.model'; import { TextSubmission } from 'app/entities/text/text-submission.model'; import { Result } from 'app/entities/result.model'; import { TextResultBlock } from './text-result-block'; -import { TranslateService } from '@ngx-translate/core'; import { TextBlock } from 'app/entities/text/text-block.model'; import { faExclamationTriangle } from '@fortawesome/free-solid-svg-icons'; -import { LocaleConversionService } from 'app/shared/service/locale-conversion.service'; import { Course } from 'app/entities/course.model';Also applies to: 18-19
Line range hint
24-24: Consider moving the SHA1 regex to a constants file.The SHA1 regex pattern could be moved to a shared constants file for reusability and maintainability.
- private readonly SHA1_REGEX = /^[a-f0-9]{40}$/i;Create a new file
app/shared/constants/validation.constants.ts:export const VALIDATION_PATTERNS = { SHA1: /^[a-f0-9]{40}$/i, };
Line range hint
44-82: Add documentation for the complex text block conversion logic.The
convertTextToResultBlocksmethod implements complex logic for handling different types of feedback and converting them to result blocks. Consider adding JSDoc comments to explain the algorithm and its purpose.Add documentation like this:
/** * Converts feedback into text result blocks by processing both reference-based and block-based feedback. * The method performs the following steps: * 1. Separates feedbacks into reference-based and block-based arrays * 2. Converts each feedback type to corresponding result blocks * 3. Merges and sorts the blocks by start index * 4. Fills gaps between blocks with plain text blocks * @param feedbacks - Array of feedback items to process */ private convertTextToResultBlocks(feedbacks: Feedback[] = []): void {
Line range hint
119-127: Simplify credit calculation and prevent potential floating-point issues.The credit calculation could be simplified and made more robust against floating-point precision issues.
- public repeatForEachCredit(textResultBlock: TextResultBlock): number[] { - if (!textResultBlock.feedback || textResultBlock.feedback.credits === 0) { - return []; - } - - const value = Math.ceil(Math.abs(textResultBlock.feedback.credits || 0)); - return new Array(value).fill(1); - } + public repeatForEachCredit(textResultBlock: TextResultBlock): number[] { + const credits = textResultBlock.feedback?.credits ?? 0; + return credits === 0 ? [] : Array(Math.ceil(Math.abs(credits))).fill(1); + }
Line range hint
129-135: Prevent potential memory leaks in translation handling.The
creditsTranslationForTextResultBlockmethod uses theTranslateServicedirectly. Consider implementingOnDestroyto clean up any potential translation subscriptions.+ private destroy$ = new Subject<void>(); + public creditsTranslationForTextResultBlock(textResultBlock: TextResultBlock): string { - const singular = Math.abs(textResultBlock.feedback!.credits || 0) === 1; - return this.translateService.instant(`artemisApp.assessment.detail.points.${singular ? 'one' : 'many'}`, { - points: this.localeConversionService.toLocaleString(textResultBlock.feedback?.credits || 0, this.course?.accuracyOfScores), - }); + const credits = textResultBlock.feedback?.credits ?? 0; + const singular = Math.abs(credits) === 1; + const points = this.localeConversionService.toLocaleString(credits, this.course?.accuracyOfScores); + + return this.translateService + .get(`artemisApp.assessment.detail.points.${singular ? 'one' : 'many'}`, { points }) + .pipe(takeUntil(this.destroy$)) + .subscribe(); } + + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }src/main/webapp/app/exercises/quiz/manage/quiz-exercise.component.ts (2)
19-23: Consider making injected services readonlyWhile the migration to
inject()is correct, consider making the services readonly to prevent accidental reassignment:- protected exerciseService = inject(ExerciseService); - protected quizExerciseService = inject(QuizExerciseService); - private accountService = inject(AccountService); - private alertService = inject(AlertService); - private sortService = inject(SortService); + protected readonly exerciseService = inject(ExerciseService); + protected readonly quizExerciseService = inject(QuizExerciseService); + private readonly accountService = inject(AccountService); + private readonly alertService = inject(AlertService); + private readonly sortService = inject(SortService);
Line range hint
41-54: Prevent potential memory leaks in HTTP subscriptionsThe HTTP subscriptions in
loadExercises()andloadOne()methods should be properly managed to prevent memory leaks. Consider using either the async pipe in the template or implementing proper cleanup in ngOnDestroy.Here's how to fix this using takeUntilDestroyed():
+import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; export class QuizExerciseComponent extends ExerciseComponent { protected loadExercises(): void { this.quizExerciseService.findForCourse(this.courseId).pipe( + takeUntilDestroyed() ).subscribe({ next: (res: HttpResponse<QuizExercise[]>) => { // ... existing code }, error: (res: HttpErrorResponse) => this.onError(res), }); } loadOne(quizExerciseId: number) { this.quizExerciseService.find(quizExerciseId).pipe( + takeUntilDestroyed() ).subscribe((res: HttpResponse<QuizExercise>) => { this.handleNewQuizExercise(res.body!); }); } }Also applies to: 106-110
src/main/webapp/app/exercises/text/assess/textblock-feedback-editor/textblock-feedback-editor.component.ts (1)
63-64: Consider moving route setup to ngOnInitSetting up the component route in the constructor might be too early in the component lifecycle. If the route setup depends on any route parameters or requires the component to be fully initialized, it would be safer to move this to
ngOnInit.constructor() { - this.textAssessmentAnalytics.setComponentRoute(this.route); } + +ngOnInit(): void { + this.textAssessmentAnalytics.setComponentRoute(this.route); }src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-detail.component.ts (2)
32-36: LGTM: Clean implementation of dependency injectionThe migration to using the
injectfunction is well implemented. The services are properly scoped as private and follow naming conventions.Consider marking these properties as
readonlysince they're only initialized once:- private route = inject(ActivatedRoute); + private readonly route = inject(ActivatedRoute); - private eventManager = inject(EventManager); + private readonly eventManager = inject(EventManager); - private artemisMarkdownService = inject(ArtemisMarkdownService); + private readonly artemisMarkdownService = inject(ArtemisMarkdownService); - private textExerciseService = inject(TextExerciseService); + private readonly textExerciseService = inject(TextExerciseService); - private statisticsService = inject(StatisticsService); + private readonly statisticsService = inject(StatisticsService);
79-81: Consider implementing markdown caching for performance optimizationWhile the markdown formatting implementation is correct, consider implementing caching for the formatted markdown content to optimize performance, especially if the content is large or frequently accessed. This would help prevent unnecessary re-rendering and improve memory usage.
Example approach:
private markdownCache = new Map<string, SafeHtml>(); private getFormattedMarkdown(content: string | undefined): SafeHtml | null { if (!content) return null; if (this.markdownCache.has(content)) { return this.markdownCache.get(content)!; } const formatted = this.artemisMarkdownService.safeHtmlForMarkdown(content); this.markdownCache.set(content, formatted); return formatted; }src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.route.ts (1)
Line range hint
63-65: Consider using translation keys for all page titlesWhile some routes use translation keys (e.g., 'artemisApp.textExercise.home.title'), others like 'exercise-statistics.title' might need to be updated to follow the same pattern for consistency in localization.
Example change:
data: { authorities: [Authority.TA, Authority.EDITOR, Authority.INSTRUCTOR, Authority.ADMIN], - pageTitle: 'exercise-statistics.title', + pageTitle: 'artemisApp.exerciseStatistics.title', },Also applies to: 119-121
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.service.ts (1)
19-22: LGTM: Clean migration to inject() with a minor suggestion.The migration from constructor-based DI to
inject()is well implemented. Consider adding a blank line after the injected services for better visual separation of concerns:private http = inject(HttpClient); private exerciseService = inject(ExerciseService); + private resourceUrl = 'api/text-exercises';src/main/webapp/app/exercises/text/shared/text-select.directive.ts (2)
35-36: Consider adding explicit type annotationsThe dependency injection using
inject()is implemented correctly. However, consider adding explicit type annotations for better code clarity:- private elementRef = inject(ElementRef); - private zone = inject(NgZone); + private elementRef: ElementRef = inject(ElementRef); + private zone: NgZone = inject(NgZone);
Line range hint
28-196: Excellent implementation with performance considerationsThe directive implementation is robust and well-thought-out:
- Proper use of
NgZone.runOutsideAngular()for performance optimization- Comprehensive event cleanup in
ngOnDestroy- Well-documented code with clear explanations
- Strong type safety with proper interfaces
- Careful handling of edge cases and null checks
The use of
runOutsideAngular()for event binding andrunGuarded()for state changes is a great pattern for performance optimization in Angular directives. This approach minimizes change detection cycles while ensuring proper state management.src/main/webapp/app/exercises/text/manage/tutor-effort/tutor-effort-statistics.component.ts (3)
1-1: LGTM! Consider grouping related imports.The migration to using the
injectfunction follows Angular's modern dependency injection pattern correctly. The service injections are properly initialized as private class properties with appropriate naming conventions.Consider grouping related imports together for better readability:
import { Component, OnInit, inject } from '@angular/core'; -import { TutorEffort } from 'app/entities/tutor-effort.model'; -import { TextExerciseService } from 'app/exercises/text/manage/text-exercise/text-exercise.service'; import { ActivatedRoute, Router } from '@angular/router'; +import { TranslateService } from '@ngx-translate/core'; +import { TutorEffort } from 'app/entities/tutor-effort.model'; +import { TextExerciseService } from 'app/exercises/text/manage/text-exercise/text-exercise.service'; import { TextAssessmentService } from 'app/exercises/text/assess/text-assessment.service';Also applies to: 24-28
Line range hint
52-56: Prevent potential memory leak from translation subscription.While the constructor refactoring is good, the translation subscription should be properly cleaned up to prevent memory leaks.
Consider implementing the
OnDestroyinterface and unsubscribing:-export class TutorEffortStatisticsComponent extends PlagiarismAndTutorEffortDirective implements OnInit { +export class TutorEffortStatisticsComponent extends PlagiarismAndTutorEffortDirective implements OnInit, OnDestroy { + private destroy$ = new Subject<void>(); + constructor() { super(); - this.translateService.onLangChange.subscribe(() => { + this.translateService.onLangChange.pipe( + takeUntil(this.destroy$) + ).subscribe(() => { this.translateLabels(); }); } + + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }Don't forget to add these imports:
import { Subject } from 'rxjs'; import { takeUntil } from 'rxjs/operators'; import { OnDestroy } from '@angular/core';
Line range hint
1-199: Consider architectural improvements for better maintainability.The component handles both data processing and visualization, which could be separated for better maintainability and testability.
Consider these improvements:
- Extract data processing logic (e.g.,
distributeEffortToSets,computeEffortMedian, etc.) into a dedicated service:@Injectable({ providedIn: 'root' }) export class TutorEffortStatisticsService { distributeEffortToSets(tutorEfforts: TutorEffort[], bucketSize: number): number[] { // Move logic here } computeEffortMedian(tutorEfforts: TutorEffort[]): number { // Move logic here } }
- Add comprehensive unit tests for the complex calculations:
describe('TutorEffortStatisticsService', () => { describe('distributeEffortToSets', () => { it('should correctly distribute efforts into buckets', () => { // Test cases }); }); });
- Add input validation for the statistics data:
private validateTutorEfforts(tutorEfforts: TutorEffort[]): void { if (!Array.isArray(tutorEfforts)) { throw new Error('Invalid tutor efforts data'); } // Add more validation as needed }Would you like me to help create these improvements in a separate PR?
src/main/webapp/app/exercises/programming/manage/programming-exercise.component.ts (1)
Line range hint
1-254: Consider breaking down component responsibilitiesThis component handles multiple concerns:
- Exercise management
- Repository operations
- Consistency checks
- Modal management
- Sorting
- Filtering
Consider splitting these into smaller, focused components or services to improve maintainability and testability.
Suggestions:
- Extract repository operations into a dedicated service
- Create a separate modal service for handling different modal types
- Consider using a state management solution (e.g., NgRx) to handle the complex state interactions
src/main/webapp/app/exercises/text/assess/text-assessment.service.ts (3)
Line range hint
40-65: Consider enhancing HTTP error handlingWhile the implementation is solid, consider adding centralized error handling for HTTP operations. This would improve error reporting and recovery, especially important during assessment submissions.
Example implementation:
private handleError(operation = 'operation') { return (error: HttpErrorResponse): Observable<never> => { console.error(`${operation} failed:`, error); // Optionally integrate with a notification service return throwError(() => error); }; } // Usage in methods: public save(...): Observable<EntityResponseType> { const body = TextAssessmentService.prepareFeedbacksAndTextblocksForRequest(feedbacks, textBlocks, assessmentNote); return this.http .put<Result>(`${this.RESOURCE_URL}/participations/${participationId}/results/${resultId}/text-assessment`, body, { observe: 'response' }) .pipe( map((res: EntityResponseType) => this.convertResultEntityResponseTypeFromServer(res)), catchError(this.handleError('save assessment')) ); }
Line range hint
134-170: Consider adding retry logic for network resilienceFor critical operations like submission and assessment updates, consider adding retry logic to handle temporary network issues.
Example implementation:
private static readonly RETRY_CONFIG = { maxRetries: 3, backoff: 1000, excludeStatusCodes: [400, 401, 403, 404] }; // Usage in methods: public getFeedbackDataForExerciseSubmission(...): Observable<StudentParticipation> { return this.http .get<StudentParticipation>(`${this.RESOURCE_URL}/text-submissions/${submissionId}/for-assessment`, { observe: 'response', params }) .pipe( retryWhen(errors => errors.pipe( concatMap((error, index) => { if (index >= TextAssessmentService.RETRY_CONFIG.maxRetries || TextAssessmentService.RETRY_CONFIG.excludeStatusCodes.includes(error.status)) { return throwError(() => error); } return timer(TextAssessmentService.RETRY_CONFIG.backoff); }) ) ), // ... rest of the pipe ); }
Line range hint
196-234: Consider simplifying response mappingThe response mapping logic could be simplified using TypeScript utility types and modern JavaScript features.
Example implementation:
private convertResultEntityResponseTypeFromServer(res: EntityResponseType): EntityResponseType { const result = { ...res.body!, completionDate: convertDateFromServer(res.body!.completionDate), submission: res.body!.submission && { ...res.body!.submission, submissionDate: convertDateFromServer(res.body!.submission.submissionDate) }, participation: res.body!.participation && { ...res.body!.participation, initializationDate: convertDateFromServer(res.body!.participation.initializationDate), individualDueDate: convertDateFromServer(res.body!.participation.individualDueDate) } }; if (result.participation?.exercise) { this.accountService.setAccessRightsForExercise(result.participation.exercise); } return res.clone({ body: result }); }src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-update.component.ts (1)
40-50: Consider organizing services by domainConsider grouping related services together and adding readonly modifiers for better code organization and immutability:
- private activatedRoute = inject(ActivatedRoute); - private alertService = inject(AlertService); - private textExerciseService = inject(TextExerciseService); - private modalService = inject(NgbModal); - private popupService = inject(ExerciseUpdateWarningService); - private exerciseService = inject(ExerciseService); - private exerciseGroupService = inject(ExerciseGroupService); - private courseService = inject(CourseManagementService); - private eventManager = inject(EventManager); - private navigationUtilService = inject(ArtemisNavigationUtilService); - private athenaService = inject(AthenaService); + // Core Angular services + private readonly activatedRoute = inject(ActivatedRoute); + private readonly modalService = inject(NgbModal); + + // Exercise related services + private readonly textExerciseService = inject(TextExerciseService); + private readonly exerciseService = inject(ExerciseService); + private readonly exerciseGroupService = inject(ExerciseGroupService); + private readonly courseService = inject(CourseManagementService); + + // Utility services + private readonly alertService = inject(AlertService); + private readonly popupService = inject(ExerciseUpdateWarningService); + private readonly eventManager = inject(EventManager); + private readonly navigationUtilService = inject(ArtemisNavigationUtilService); + private readonly athenaService = inject(AthenaService);src/main/webapp/app/exercises/text/manage/example-text-submission/example-text-submission.component.ts (1)
Line range hint
1-41: LGTM! Consider grouping related service injections.The migration to the
inject()function is well implemented. All necessary services are properly injected as private class properties.Consider grouping related service injections for better readability:
- private route = inject(ActivatedRoute); - private router = inject(Router); - private exampleSubmissionService = inject(ExampleSubmissionService); - private tutorParticipationService = inject(TutorParticipationService); - private guidedTourService = inject(GuidedTourService); - private navigationUtilService = inject(ArtemisNavigationUtilService); - private exerciseService = inject(ExerciseService); + // Routing related + private route = inject(ActivatedRoute); + private router = inject(Router); + private navigationUtilService = inject(ArtemisNavigationUtilService); + + // Exercise related + private exerciseService = inject(ExerciseService); + private exampleSubmissionService = inject(ExampleSubmissionService); + private tutorParticipationService = inject(TutorParticipationService); + + // UI related + private guidedTourService = inject(GuidedTourService);src/main/webapp/app/exercises/text/participate/text-editor.component.ts (1)
Line range hint
183-270: Consider breaking down updateParticipation methodThe
updateParticipationmethod is handling multiple responsibilities including:
- Participation data management
- Exercise state management
- Submission handling
- Result processing
Consider breaking this into smaller, focused methods for better maintainability.
Example refactor:
- private updateParticipation(participation: StudentParticipation, submissionId: number | undefined = undefined) { + private updateParticipation(participation: StudentParticipation, submissionId?: number) { + if (!participation) return; + this.updateParticipationData(participation); + this.updateExerciseState(); + this.updateSubmissionAndResults(submissionId); +} + +private updateParticipationData(participation: StudentParticipation): void { this.participation = participation; this.textExercise = this.participation.exercise as TextExercise; this.examMode = !!this.textExercise.exerciseGroup; this.textExercise.studentParticipations = [this.participation]; +} + +private updateExerciseState(): void { this.checkIfSubmitAlwaysEnabled(); this.isAfterAssessmentDueDate = !!this.textExercise.course && (!this.textExercise.assessmentDueDate || dayjs().isAfter(this.textExercise.assessmentDueDate)); this.isAfterPublishDate = !!this.textExercise.exerciseGroup?.exam?.publishResultsDate && dayjs().isAfter(this.textExercise.exerciseGroup.exam.publishResultsDate); this.course = getCourseFromExercise(this.textExercise); +} + +private updateSubmissionAndResults(submissionId?: number): void { // ... rest of the submission and result handling logicsrc/main/webapp/app/exercises/text/assess/text-submission-assessment.component.ts (1)
Line range hint
1-576: Consider breaking down this large component.While the implementation is functionally correct, this component has grown quite large with multiple responsibilities:
- Assessment management
- Navigation
- Complaint handling
- Example submission handling
- Feedback validation
- Text block management
Consider splitting these responsibilities into separate services or sub-components to improve maintainability and testability.
Some suggestions:
- Extract text block management logic into a separate service
- Create a dedicated complaint handling service
- Move feedback validation logic to a shared service
- Consider using the facade pattern to manage the complexity of these interactions
Would you like me to help create a detailed plan for this refactoring?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (28)
src/main/webapp/app/exercises/file-upload/manage/file-upload-exercise.component.ts(1 hunks)src/main/webapp/app/exercises/modeling/manage/modeling-exercise.component.ts(2 hunks)src/main/webapp/app/exercises/programming/manage/programming-exercise.component.ts(2 hunks)src/main/webapp/app/exercises/quiz/manage/quiz-exercise.component.ts(2 hunks)src/main/webapp/app/exercises/shared/exercise/exercise.component.ts(3 hunks)src/main/webapp/app/exercises/text/assess/analytics/text-assesment-analytics.service.ts(3 hunks)src/main/webapp/app/exercises/text/assess/text-assessment-area/text-assessment-area.component.ts(2 hunks)src/main/webapp/app/exercises/text/assess/text-assessment-base.component.ts(2 hunks)src/main/webapp/app/exercises/text/assess/text-assessment.service.ts(2 hunks)src/main/webapp/app/exercises/text/assess/text-submission-assessment.component.ts(4 hunks)src/main/webapp/app/exercises/text/assess/text-submission-assessment.route.ts(3 hunks)src/main/webapp/app/exercises/text/assess/textblock-assessment-card/textblock-assessment-card.component.ts(3 hunks)src/main/webapp/app/exercises/text/assess/textblock-feedback-editor/textblock-feedback-editor.component.ts(3 hunks)src/main/webapp/app/exercises/text/manage/example-text-submission/example-text-submission.component.ts(3 hunks)src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-detail.component.ts(3 hunks)src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-paging.service.ts(1 hunks)src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-row-buttons.component.ts(2 hunks)src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-update.component.ts(2 hunks)src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.component.ts(2 hunks)src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.route.ts(2 hunks)src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.service.ts(2 hunks)src/main/webapp/app/exercises/text/manage/tutor-effort/tutor-effort-statistics.component.ts(3 hunks)src/main/webapp/app/exercises/text/participate/text-editor.component.ts(3 hunks)src/main/webapp/app/exercises/text/participate/text-editor.service.ts(2 hunks)src/main/webapp/app/exercises/text/participate/text-result/text-result.component.ts(2 hunks)src/main/webapp/app/exercises/text/participate/text-submission.service.ts(2 hunks)src/main/webapp/app/exercises/text/shared/manual-text-selection/manual-text-selection.component.ts(3 hunks)src/main/webapp/app/exercises/text/shared/text-select.directive.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (28)
src/main/webapp/app/exercises/file-upload/manage/file-upload-exercise.component.ts (1)
src/main/webapp/app/exercises/modeling/manage/modeling-exercise.component.ts (1)
src/main/webapp/app/exercises/programming/manage/programming-exercise.component.ts (1)
src/main/webapp/app/exercises/quiz/manage/quiz-exercise.component.ts (1)
src/main/webapp/app/exercises/shared/exercise/exercise.component.ts (1)
src/main/webapp/app/exercises/text/assess/analytics/text-assesment-analytics.service.ts (1)
src/main/webapp/app/exercises/text/assess/text-assessment-area/text-assessment-area.component.ts (1)
src/main/webapp/app/exercises/text/assess/text-assessment-base.component.ts (1)
src/main/webapp/app/exercises/text/assess/text-assessment.service.ts (1)
src/main/webapp/app/exercises/text/assess/text-submission-assessment.component.ts (1)
src/main/webapp/app/exercises/text/assess/text-submission-assessment.route.ts (1)
src/main/webapp/app/exercises/text/assess/textblock-assessment-card/textblock-assessment-card.component.ts (1)
src/main/webapp/app/exercises/text/assess/textblock-feedback-editor/textblock-feedback-editor.component.ts (1)
src/main/webapp/app/exercises/text/manage/example-text-submission/example-text-submission.component.ts (1)
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-detail.component.ts (1)
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-paging.service.ts (1)
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-row-buttons.component.ts (1)
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-update.component.ts (1)
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.component.ts (1)
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.route.ts (1)
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.service.ts (1)
src/main/webapp/app/exercises/text/manage/tutor-effort/tutor-effort-statistics.component.ts (1)
src/main/webapp/app/exercises/text/participate/text-editor.component.ts (1)
src/main/webapp/app/exercises/text/participate/text-editor.service.ts (1)
src/main/webapp/app/exercises/text/participate/text-result/text-result.component.ts (1)
src/main/webapp/app/exercises/text/participate/text-submission.service.ts (1)
src/main/webapp/app/exercises/text/shared/manual-text-selection/manual-text-selection.component.ts (1)
src/main/webapp/app/exercises/text/shared/text-select.directive.ts (1)
🪛 Biome
src/main/webapp/app/exercises/text/assess/text-submission-assessment.component.ts
[error] 111-111: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (50)
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-paging.service.ts (1)
2-2: LGTM: Import statement correctly includes the inject function.
The import statement follows Angular's style guide and includes the necessary inject function for the new DI pattern.
src/main/webapp/app/exercises/text/participate/text-editor.service.ts (2)
11-11: LGTM! Correct implementation of dependency injection.
The migration to the new inject() pattern is properly implemented, following Angular's recommended approach for dependency injection.
Line range hint 1-33: Verify service usage and subscription handling.
Let's verify that components using this service properly handle subscriptions to prevent memory leaks.
✅ Verification successful
Let me analyze the results and generate the final response:
No memory leak concerns found in TextEditorService usage
The service is properly designed as a stateless Angular service that uses Angular's dependency injection. The get() method returns an Observable that completes after emitting a single value through the HTTP request, which means:
- The HTTP request automatically completes and cleans up
- The
tapoperator used is just for side effects and doesn't affect subscription handling - No long-lived subscriptions are created by the service itself
Components using this service should handle their own subscription cleanup, but that's a general Angular best practice not specific to this service's implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using TextEditorService
echo "Components using TextEditorService:"
ast-grep --pattern 'class $_ {
$$$
private textEditorService = inject(TextEditorService)
$$$
}'
# Check for proper subscription handling
echo "\nSubscription handling in components:"
rg -l "textEditorService.get\(" | xargs rg "takeUntil|first\(\)|take\(1\)"
Length of output: 85535
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-row-buttons.component.ts (2)
1-1: LGTM: Clean import addition
The inject import is correctly added to the existing Angular core imports.
15-16: Verify consistent migration pattern across the codebase
Let's ensure this migration pattern is consistently applied across other components.
✅ Verification successful
Based on the results, I can generate the final response as we have clear evidence about the DI patterns in the codebase.
Migration to inject() pattern is consistently implemented ✅
The codebase shows a consistent adoption of the inject() pattern for both EventManager and TextExerciseService. No constructor-based injection was found for these services, and all instances use the new pattern across different components:
homecomponentstext-exerciserelated componentstutor-effortcomponents
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent DI patterns in the codebase
# Look for remaining constructor-based DI for these services
echo "Checking for constructor-based injection patterns..."
ast-grep --pattern 'constructor($$$) {
$$$
private $service: EventManager
$$$
}'
# Look for the new inject() pattern to verify consistency
echo "Checking for inject() pattern adoption..."
rg "private \w+ = inject\((EventManager|TextExerciseService)\)" "src/main/webapp"
Length of output: 1764
src/main/webapp/app/exercises/text/shared/manual-text-selection/manual-text-selection.component.ts (1)
1-1: LGTM: Import changes look good
The addition of the inject import from '@angular/core' is correct and follows Angular's style guide.
src/main/webapp/app/exercises/text/assess/text-assessment-area/text-assessment-area.component.ts (2)
1-1: LGTM! Import changes align with Angular style guide.
The addition of inject to the existing Angular core imports maintains clean organization and follows the style guide.
19-20: Verify memory management with injected service.
The migration to inject() looks good and follows modern Angular practices. However, let's ensure proper cleanup of any potential subscriptions from the StringCountService.
Consider moving the service injection after the @input declarations.
While the current placement works, moving it after the decorators would improve readability by grouping all class properties together.
- private stringCountService = inject(StringCountService);
-
// inputs
@Input() submission: TextSubmission;
@Input() textBlockRefs: TextBlockRef[];
@Input() readOnly: boolean;
@Input() highlightDifferences: boolean;
@Input() criteria?: GradingCriterion[];
@Input() allowManualBlockSelection = true;
+
+ private stringCountService = inject(StringCountService);src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.component.ts (3)
1-16: LGTM! Imports are well-organized and follow Angular guidelines.
The imports are properly structured and include all necessary dependencies for the component's functionality.
Line range hint 18-22: LGTM! Component structure follows Angular best practices.
The component declaration follows Angular style guidelines with proper naming conventions and inheritance structure.
32-33: LGTM! Properties are properly initialized.
The array initializations follow Angular best practices for property initialization.
src/main/webapp/app/exercises/text/assess/analytics/text-assesment-analytics.service.ts (2)
Line range hint 1-15: LGTM! Import changes and class declaration follow Angular guidelines.
The addition of the inject import and the service declaration follow Angular's style guide and best practices.
Line range hint 1-93: Verify the migration impact across dependent components.
While the migration to inject() is implemented correctly, we should verify that all components consuming this service are compatible with the changes.
✅ Verification successful
Let me check the actual usage in the components to verify if there are any constructor injections or potential breaking changes.
No migration impact concerns - all components are already using inject()
The verification shows that all components consuming TextAssessmentAnalytics are already using the modern inject() pattern:
manual-text-selection.component.tstextblock-feedback-editor.component.tstextblock-assessment-card.component.ts
No breaking changes or migration work is needed as the components are already aligned with the service implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that might be affected by the TextAssessmentAnalytics service changes
# Find components that inject TextAssessmentAnalytics
echo "Components using TextAssessmentAnalytics:"
rg -l "TextAssessmentAnalytics" src/main/webapp/app/exercises/text/
# Check for potential constructor injections that need migration
echo "\nPotential constructor injections that might need updates:"
ast-grep --pattern 'constructor($params) { $$$; TextAssessmentAnalytics $$$; }'
Length of output: 772
Script:
#!/bin/bash
# Check the actual implementation in the components
echo "=== manual-text-selection.component.ts ==="
rg -A 5 "TextAssessmentAnalytics" src/main/webapp/app/exercises/text/shared/manual-text-selection/manual-text-selection.component.ts
echo "\n=== textblock-feedback-editor.component.ts ==="
rg -A 5 "TextAssessmentAnalytics" src/main/webapp/app/exercises/text/assess/textblock-feedback-editor/textblock-feedback-editor.component.ts
echo "\n=== textblock-assessment-card.component.ts ==="
rg -A 5 "TextAssessmentAnalytics" src/main/webapp/app/exercises/text/assess/textblock-assessment-card/textblock-assessment-card.component.ts
Length of output: 2612
src/main/webapp/app/exercises/text/participate/text-submission.service.ts (1)
1-1: LGTM! Clean import of the inject function
The addition of the inject import alongside Injectable follows Angular's modern dependency injection pattern.
src/main/webapp/app/exercises/modeling/manage/modeling-exercise.component.ts (2)
1-1: LGTM: Import changes are correct
The inject import from @angular/core is properly added to support the new dependency injection pattern.
20-27: Verify service cleanup and component lifecycle
Please ensure that any service subscriptions are properly cleaned up to prevent memory leaks. Consider implementing ngOnDestroy if not already present in the parent class.
✅ Verification successful
Let me verify the implementation of OnDestroy in the parent class to ensure proper cleanup.
Let me check the implementation of ngOnDestroy in the parent class to verify cleanup.
Let me fix the regex and try again.
Based on the verification results, I can now provide the final response:
Service cleanup is properly handled
The component extends ExerciseComponent which already implements OnDestroy and handles cleanup of subscriptions. The parent class's ngOnDestroy() implementation properly cleans up:
- The event subscriber via
eventManager.destroy() - The dialog error source subscription
No additional cleanup is needed as the injected services are handled by Angular's dependency injection lifecycle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for subscription cleanup patterns in the component hierarchy
ast-grep --pattern 'class $_ implements OnDestroy {
$$$
ngOnDestroy() {
$$$
}
$$$
}'
# Check for subscription cleanup patterns in the parent class
rg -l "ExerciseComponent.*implements.*OnDestroy" --type ts
Length of output: 97487
Script:
#!/bin/bash
# Check the parent class implementation
rg -A 10 "export.*class.*ExerciseComponent.*implements.*OnDestroy" --type ts
# Check the modeling exercise component implementation
rg -A 10 "export.*class.*ModelingExerciseComponent" --type ts
Length of output: 4275
Script:
#!/bin/bash
# Check the ngOnDestroy implementation in the parent class
rg -A 10 "ngOnDestroy\(\).*{" src/main/webapp/app/exercises/shared/exercise/exercise.component.ts
Length of output: 199
Script:
#!/bin/bash
# Check the ngOnDestroy implementation in the parent class
rg -A 10 "ngOnDestroy\(\)" src/main/webapp/app/exercises/shared/exercise/exercise.component.ts
# Alternative search to find the implementation
rg -A 10 "implements.*OnDestroy" src/main/webapp/app/exercises/shared/exercise/exercise.component.ts
Length of output: 930
src/main/webapp/app/exercises/text/assess/text-submission-assessment.route.ts (4)
1-1: LGTM: Import changes align with dependency injection migration
The addition of the inject import is appropriate for the migration from constructor-based dependency injection.
15-15: LGTM: Clean migration to modern DI pattern
The migration to using inject() follows Angular's modern dependency injection pattern while maintaining the existing functionality. The implementation remains clean and follows single responsibility principle.
36-36: LGTM: Consistent migration to modern DI pattern
The migration to inject() is clean and consistent with the application-wide pattern.
Line range hint 1-54: Verify resolver usage across the application
Let's ensure that these resolvers are consistently used across the application and that the migration hasn't introduced any issues in components that depend on them.
✅ Verification successful
Migration to inject() is properly implemented and used consistently
Based on the comprehensive analysis of the codebase:
-
The resolvers are properly configured in the route definitions with three distinct paths:
/submissions/new/assessmentusingNewStudentParticipationResolver/submissions/:submissionId/assessmentusingStudentParticipationResolver/submissions/:submissionId/assessments/:resultIdusingStudentParticipationResolver
-
The
TextSubmissionAssessmentComponentitself also uses the inject pattern consistently:private activatedRoute = inject(ActivatedRoute); private router = inject(Router);
-
The test suite properly validates both resolvers' functionality and their integration with the services.
The migration to inject() has been implemented consistently across the related components and is properly tested. No issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to these resolvers in route configurations and components
echo "Checking resolver usage in route configurations..."
rg -l "NewStudentParticipationResolver|StudentParticipationResolver" --type ts
echo "Checking for any remaining constructor-based DI in text assessment related files..."
rg "constructor\((private|public|protected)?\s*(readonly)?\s*(textSubmissionService|textAssessmentService)" --type ts
Length of output: 631
Script:
#!/bin/bash
# Let's check how these resolvers are used in the routes configuration
echo "Checking route configurations for resolver usage..."
rg -A 10 -B 2 "path.*text.*assessment" --type ts
# Let's also check the test file to understand the testing context
echo "Checking test file content..."
cat "src/test/javascript/spec/service/text-assessment.service.spec.ts"
# Let's verify if there are any other similar resolvers using inject pattern
echo "Checking for other resolvers using inject pattern..."
ast-grep --pattern 'export class $_ implements Resolve<$_> {
private $_ = inject($_);
}'
Length of output: 12360
Script:
#!/bin/bash
# Let's check the route configuration file specifically
echo "Checking text-submission-assessment.route.ts for route configuration..."
cat "src/main/webapp/app/exercises/text/assess/text-submission-assessment.route.ts"
# Let's also check for any components that might be using these resolvers
echo "Checking for components using these resolvers..."
rg -A 5 "TextSubmissionAssessmentComponent" --type ts
Length of output: 18935
src/main/webapp/app/exercises/file-upload/manage/file-upload-exercise.component.ts (3)
1-1: LGTM: Clean import of inject
The addition of inject to the existing '@angular/core' import is clean and follows Angular's style guidelines.
21-26: LGTM: Clean migration to inject() pattern
The service injections have been properly migrated to use the new inject() function, with appropriate access modifiers and clean initialization.
21-26: Verify subscription cleanup
Since multiple services are injected that might create subscriptions (e.g., exerciseService, accountService), ensure that all subscriptions are properly cleaned up to prevent memory leaks.
src/main/webapp/app/exercises/shared/exercise/exercise.component.ts (3)
1-1: LGTM: Import changes align with Angular's dependency injection guidelines.
The addition of the inject import from '@angular/core' is correct and follows Angular's recommended approach for modern dependency injection.
18-21: LGTM: Clean dependency injection implementation using inject()
The migration to use the inject() function follows Angular's modern dependency injection pattern and improves code readability by removing constructor boilerplate.
31-32: LGTM: Clean property initialization
The initialization of predicate and reverse properties at declaration is clean and follows TypeScript best practices.
src/main/webapp/app/exercises/quiz/manage/quiz-exercise.component.ts (2)
Line range hint 1-18: LGTM! Component declaration follows Angular guidelines
The import changes and component structure align with Angular best practices and the PR's objective to migrate to dependency injection using inject.
Line range hint 1-124: Verify removed dependencies and localization
Let's ensure there are no lingering references to removed services and check for proper localization.
✅ Verification successful
All dependencies properly handled and localization is in place
The verification shows:
- All service injections are properly handled using the new inject() syntax
- Template uses proper localization with jhiTranslate directives
- RouterLink usage is valid and expected for navigation
- Error handling uses the standard X-artemisApp-error header which is properly translated
- No lingering references to removed services (NgbModal, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to removed services and verify localization
# Check for references to removed services (NgbModal, Router, etc.)
echo "Checking for references to removed services..."
rg -l "NgbModal|Router|CourseManagementService|TranslateService|EventManager" src/main/webapp/app/exercises/quiz/manage/quiz-exercise.component.ts
# Check for non-localized strings in template
echo "Checking for non-localized strings in template..."
rg -l "\"[^\"]+\"" src/main/webapp/app/exercises/quiz/manage/quiz-exercise.component.html
# Check if translations exist for error messages
echo "Checking for translations..."
rg -l "X-artemisApp-error" src/main/webapp/i18n/
Length of output: 636
Script:
#!/bin/bash
# Let's check the actual content of the template file and error handling
echo "Checking template content for non-localized strings..."
rg -A 2 '"[^"]+"' src/main/webapp/app/exercises/quiz/manage/quiz-exercise.component.html
echo "Checking error handling implementation..."
rg -A 2 "X-artemisApp-error" src/main/webapp/app/exercises/quiz/manage/quiz-exercise.component.ts
# Check if there are any remaining service injections or usages we missed
echo "Checking for any service references..."
ast-grep --pattern 'inject($_)' src/main/webapp/app/exercises/quiz/manage/quiz-exercise.component.ts
# Check template for any remaining modal or router references
echo "Checking template for navigation/modal references..."
rg -i "(router|modal|navigate)" src/main/webapp/app/exercises/quiz/manage/quiz-exercise.component.html
Length of output: 7643
src/main/webapp/app/exercises/text/assess/textblock-feedback-editor/textblock-feedback-editor.component.ts (1)
1-1: Clean implementation of the inject() pattern!
The migration from constructor injection to the new inject() pattern follows Angular's recommended practices. The services are properly typed and use appropriate access modifiers.
Also applies to: 19-22
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-detail.component.ts (1)
1-1: LGTM: Import changes align with dependency injection migration
The addition of the inject import from '@angular/core' is appropriate for the migration to the new dependency injection pattern.
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.route.ts (3)
6-6: LGTM: Import statement follows Angular conventions
The addition of the inject import aligns with the migration to modern dependency injection.
22-24: LGTM: Clean implementation of modern dependency injection
The migration from constructor-based DI to inject() follows Angular's recommended patterns and reduces boilerplate while maintaining type safety. The services are properly scoped as private class properties.
22-24: Verify resolver usage across components
Let's ensure there are no circular dependencies or compatibility issues with components using this resolver.
✅ Verification successful
Migration to inject() is safe and properly implemented
The verification shows:
- The resolver is correctly implemented as an injectable service
- Services are properly injected and used within the resolver's methods
- The resolver is used consistently in both text exercise and exam routes
- No circular dependencies were found
- The migration maintains the expected dependency flow and component compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using TextExerciseResolver
echo "Components using TextExerciseResolver:"
rg -l "TextExerciseResolver" "src/main/webapp"
# Check for potential circular dependencies
echo -e "\nPotential circular dependencies:"
rg -l "import.*TextExerciseResolver" "src/main/webapp/app/exercises/text/manage/text-exercise"
Length of output: 443
Script:
#!/bin/bash
# Check the implementation of TextExerciseResolver and its dependencies
echo "TextExerciseResolver implementation:"
ast-grep --pattern 'class TextExerciseResolver {
$$$
}'
# Check how these services are used in the resolver
echo -e "\nService usage in resolver:"
rg -B2 -A2 "textExerciseService|courseService|exerciseGroupService" "src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.route.ts"
# Check exam route usage
echo -e "\nExam route implementation:"
rg -B2 -A2 "TextExerciseResolver" "src/main/webapp/app/exam/manage/exam-management.route.ts"
Length of output: 3304
src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.service.ts (2)
1-1: LGTM: Import changes are correct.
The addition of inject to the Angular core imports is properly implemented.
19-20: Verify consistent usage of inject() across the codebase.
Let's ensure this DI pattern is consistently applied across related components and services.
✅ Verification successful
Consistent usage of inject() pattern confirmed across the codebase
The codebase shows consistent usage of the inject() pattern for dependency injection across related components and services. The search results reveal extensive adoption of this pattern in:
- Text exercise components and services
- Other exercise-related components (programming, modeling, file-upload)
- Assessment components
- Shared services and utilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent DI patterns in related exercise services and components
# Look for constructor-based DI in exercise-related services
echo "Checking for constructor-based DI in exercise services..."
ast-grep --pattern 'class $name implements ExerciseServicable {
constructor($$$) {
$$$
}
}'
# Look for inconsistent DI patterns in text exercise components
echo "Checking for constructor-based DI in text exercise components..."
ast-grep --pattern 'class $name {
constructor(private $service: TextExerciseService, $$$) {
$$$
}
}'
# Look for existing inject() usage pattern
echo "Checking current inject() usage pattern..."
rg "private \w+ = inject\(" "src/main/webapp/app/exercises/"
Length of output: 15864
src/main/webapp/app/exercises/text/assess/text-assessment-base.component.ts (3)
1-1: LGTM: Import changes align with dependency injection migration
The addition of the inject import from '@angular/core' is correct and follows Angular's dependency injection best practices.
20-23: LGTM: Service injection implementation is correct
The migration to use the inject function follows Angular's recommended patterns:
- Services are properly scoped as protected
- Injection pattern is consistent across all services
- Service initialization is clean and maintainable
20-23: ```shell
#!/bin/bash
Let's try one more time with correct path handling
Find all TypeScript files that reference TextAssessmentBaseComponent
echo "Files referencing TextAssessmentBaseComponent:"
rg -t typescript -l "TextAssessmentBaseComponent"
Show the content of these files focusing on class declarations and inheritance
echo -e "\nClass declarations and inheritance patterns:"
rg -t typescript "class.*extends.*TextAssessmentBaseComponent" -B 2 -A 5
Look for constructor patterns in the same context
echo -e "\nConstructor patterns in related files:"
rg -t typescript "constructor.*(" -B 2 -A 5 $(rg -t typescript -l "TextAssessmentBaseComponent")
</details>
<details>
<summary>src/main/webapp/app/exercises/text/shared/text-select.directive.ts (1)</summary>
`1-1`: **LGTM: Import statement correctly updated**
The import statement has been properly updated to include the `inject` function from '@angular/core'.
</details>
<details>
<summary>src/main/webapp/app/exercises/programming/manage/programming-exercise.component.ts (3)</summary>
`1-1`: **LGTM: Clean import addition**
The `inject` import is correctly added alongside other Angular core imports.
---
`45-52`: **LGTM: Clean migration to inject() pattern**
The services are correctly migrated to use the new inject() pattern with appropriate access modifiers and typing.
---
`54-54`: **LGTM: Safe input initialization**
Initializing the `@Input()` property with an empty array prevents potential undefined errors and follows Angular best practices.
</details>
<details>
<summary>src/main/webapp/app/exercises/text/assess/text-assessment.service.ts (1)</summary>
`1-1`: **LGTM! Clean migration to modern Angular DI**
The migration from constructor-based DI to the `inject()` function follows Angular's modern patterns, reducing boilerplate while maintaining type safety through inference. The declaration order (injections followed by constants) is clean and logical.
Also applies to: 25-28
</details>
<details>
<summary>src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise-update.component.ts (2)</summary>
`1-1`: **LGTM: Clean import addition**
The `inject` import is correctly added to the existing Angular core imports, following the style guide recommendations.
---
`40-50`: **LGTM: Clean migration to inject pattern**
The migration to the `inject` function follows Angular's recommended approach for dependency injection. Each service is properly typed and marked as private.
</details>
<details>
<summary>src/main/webapp/app/exercises/text/manage/example-text-submission/example-text-submission.component.ts (1)</summary>
`70-74`: **Verify parent class compatibility with inject() pattern.**
The constructor has been correctly simplified. However, we should verify that the parent class `TextAssessmentBaseComponent` has also been migrated to use the `inject()` function, as it might affect the behavior of this component.
<details>
<summary>✅ Verification successful</summary>
**Parent class has been migrated to use inject() pattern correctly**
The search results show that the parent class `TextAssessmentBaseComponent` has been properly migrated to use the `inject()` pattern, as evidenced by the following injected dependencies in `text-assessment-base.component.ts`:
```typescript
protected alertService = inject(AlertService);
protected accountService = inject(AccountService);
protected assessmentsService = inject(TextAssessmentService);
protected structuredGradingCriterionService = inject(StructuredGradingCriterionService);
The constructor changes in the child component are safe since the parent class has been properly updated to use dependency injection via the inject() function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the parent class has been migrated to use inject()
# Search for the parent class implementation
ast-grep --pattern 'class TextAssessmentBaseComponent {
$$$
constructor($$$) {
$$$
}
$$$
}'
# Search for any remaining constructor-based DI in the parent class
rg -A 5 "constructor\([^)]+\)" "src/main/webapp/app/exercises/text/assess/text-assessment-base.component.ts"
Length of output: 338
Script:
#!/bin/bash
# Let's try a different approach to find the parent class and its implementation
# First, let's check if the file exists
fd "text-assessment-base.component.ts"
# Then let's look at its content focusing on the class definition and constructor
rg -l "TextAssessmentBaseComponent" --type ts
# If found, let's examine the file content
rg "class TextAssessmentBaseComponent" -A 20 --type ts
# Also search for inject usage in the file
rg "inject\(" --type ts
Length of output: 663
Script:
#!/bin/bash
# Let's try without the typescript file type flag since it's not recognized
# Find the base component file
fd "text-assessment-base.component.ts"
# Look for class definition and constructor
rg "class TextAssessmentBaseComponent" -A 20
# Look for files that extend this component
rg "extends TextAssessmentBaseComponent"
# Look for inject usage
rg "inject\("
Length of output: 278695
src/main/webapp/app/exercises/text/participate/text-editor.component.ts (3)
1-1: LGTM: Import changes follow Angular guidelines
The addition of the inject import from @angular/core is correctly placed and aligns with Angular's style guide.
44-52: LGTM: Service injection follows best practices
The refactoring to use inject() is well-implemented with consistent patterns and proper encapsulation.
76-76: LGTM: Property initialization is clean and explicit
Direct initialization of isSaving to false is cleaner than constructor initialization and provides clear initial state.
src/main/webapp/app/exercises/text/assess/text-submission-assessment.component.ts (1)
1-1: LGTM! Import changes align with dependency injection migration.
The addition of inject import from '@angular/core' is appropriate for the migration to the new dependency injection pattern.
...in/webapp/app/exercises/text/shared/manual-text-selection/manual-text-selection.component.ts
Show resolved
Hide resolved
...p/app/exercises/text/assess/textblock-assessment-card/textblock-assessment-card.component.ts
Show resolved
Hide resolved
src/main/webapp/app/exercises/text/assess/analytics/text-assesment-analytics.service.ts
Show resolved
Hide resolved
src/main/webapp/app/exercises/programming/manage/programming-exercise.component.ts
Show resolved
Hide resolved
src/main/webapp/app/exercises/text/assess/text-submission-assessment.component.ts
Show resolved
Hide resolved
src/main/webapp/app/exercises/text/assess/text-submission-assessment.component.ts
Show resolved
Hide resolved
vinceclifford
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
github-actions
bot
added
tests
and removed
quiz
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/test/javascript/spec/service/text-editor.service.spec.ts (1)
Line range hint
18-63: Enhance test assertions for better specificityAccording to our coding guidelines, we should use more specific assertion methods:
- Replace
toBewithtoEqualfor enum comparisons- Use
toBeUndefined()instead of implicit undefined comparisonExample improvement:
-expect(textEditorService.predictLanguage(testString)).toBe(Language.GERMAN); +expect(textEditorService.predictLanguage(testString)).toEqual(Language.GERMAN); -expect(textEditorService.predictLanguage(testString)).toBeUndefined(); +expect(textEditorService.predictLanguage(testString)).toBeUndefined();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/service/text-editor.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/service/text-editor.service.spec.ts (1)
Pattern 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}}
🔇 Additional comments (2)
src/test/javascript/spec/service/text-editor.service.spec.ts (2)
3-6: LGTM! Proper use of Angular's testing utilities
The imports correctly use the new standalone-style providers with provideHttpClient and provideHttpClientTesting, which is the recommended approach for Angular testing.
1-1: Verify TextEditorService implementation
Since this PR is part of the migration to use inject, we should verify that the TextEditorService implementation has been updated accordingly.
✅ Verification successful
TextEditorService is correctly using inject pattern
The service implementation has been properly migrated to use the inject pattern with private http = inject(HttpClient). The test import is consistent with the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TextEditorService implementation
ast-grep --pattern 'class TextEditorService {
$$$
}'
# Verify if inject is used
rg "inject\(HttpClient\)" "src/main/webapp/app/exercises/text/participate/text-editor.service.ts"
Length of output: 3134
| TestBed.configureTestingModule({ | ||
| imports: [], | ||
| providers: [provideHttpClient(), provideHttpClientTesting()], | ||
| }); | ||
| textEditorService = TestBed.inject(TextEditorService); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the test setup
While the migration to standalone providers is good, there are a few improvements to consider:
- The empty imports array can be removed
- HTTP request expectations should be set up using HttpTestingController
Consider applying these changes:
TestBed.configureTestingModule({
- imports: [],
providers: [provideHttpClient(), provideHttpClientTesting()],
});
textEditorService = TestBed.inject(TextEditorService);
+const httpTestingController = TestBed.inject(HttpTestingController);
+
+afterEach(() => {
+ httpTestingController.verify(); // Ensure no outstanding requests
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TestBed.configureTestingModule({ | |
| imports: [], | |
| providers: [provideHttpClient(), provideHttpClientTesting()], | |
| }); | |
| textEditorService = TestBed.inject(TextEditorService); | |
| TestBed.configureTestingModule({ | |
| providers: [provideHttpClient(), provideHttpClientTesting()], | |
| }); | |
| textEditorService = TestBed.inject(TextEditorService); | |
| const httpTestingController = TestBed.inject(HttpTestingController); | |
| afterEach(() => { | |
| httpTestingController.verify(); // Ensure no outstanding requests | |
| }); |
|
@cremertim I'm migrating step by step to keep it reviewable. First I will start with |
FelixTJDietrich
requested review from
eceeeren,
sachmii,
vinceclifford and
HawKhiem
|
No need to retest, only the test code changed to fix the failing tests. |
FelixTJDietrich
dismissed stale reviews from sachmii, eceeeren, and cremertim
via
5faf971
|
Had to fix some TypeScript property access errors in 5faf971 Somehow it did not error when I ran the tests locally and I assumed the tests would pass :/ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/exercises/text/assess/textblock-assessment-card/textblock-assessment-card.component.ts (1)
Line range hint
47-63: Prevent potential memory leaks in select() methodThe
setTimeoutcall in theselect()method should be cleaned up when the component is destroyed to prevent memory leaks.Consider implementing OnDestroy and clearing the timeout:
export class TextblockAssessmentCardComponent implements OnDestroy { private timeoutRef?: number; select(autofocus = true): void { // ... existing code ... if (autofocus) { this.timeoutRef = setTimeout(() => this.feedbackEditor.focus()); // ... rest of the code ... } } ngOnDestroy(): void { if (this.timeoutRef) { clearTimeout(this.timeoutRef); } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/exercises/quiz/manage/quiz-exercise.component.ts(2 hunks)src/main/webapp/app/exercises/text/assess/textblock-assessment-card/textblock-assessment-card.component.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/exercises/quiz/manage/quiz-exercise.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/exercises/text/assess/textblock-assessment-card/textblock-assessment-card.component.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/exercises/text/assess/textblock-assessment-card/textblock-assessment-card.component.ts (2)
1-1: LGTM! Clean migration to inject()
The migration to using inject() is well implemented, with appropriate access modifiers and following Angular's naming conventions.
Also applies to: 20-22
36-37: Skip comment: Route initialization issue already identified
A previous review comment already correctly identified that the route-dependent initialization should be moved to ngOnInit.
|
I missed some more TS issues in the tests, should be good to go now, sorry for that |
Checklist
General
Client
Motivation and Context
We want to modernize our Angular code, part of this modernization is switching to the
injectmethods which has some minor advantages.Description
injectfor the whole text moduleSteps for Testing
Prerequisites:
You have to check if all features of text exercises still work as expected.
Testserver 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
Test Coverage
Unchanged.
Summary by CodeRabbit
New Features
injectfunction for dependency injection across multiple components and services, enhancing code readability and maintainability.modelingExercises,programmingExercises, andisSaving, directly in their declarations.Bug Fixes
Refactor
injectfunction for direct service instantiation.Documentation