Communication: Migrate metis service to standalone components
#9907
Conversation
github-actions
bot
added
the
client
cremertim
changed the title
Communication: Migrate metis service to standalone components
WalkthroughThe pull request introduces significant modifications across multiple components and services within the Angular application, primarily focusing on refactoring the dependency injection mechanism to utilize Angular's Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
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 (68)
src/main/webapp/app/shared/metis/posting-content/enlarge-slide-image/enlarge-slide-image.component.ts (1)
8-11: Consider enhancing the standalone component configuration.While marking the component as standalone is good, consider adding these configurations for better maintainability:
- Add
importsarray for Material Dialog dependencies- Consider specifying
ChangeDetectionStrategy.OnPushfor better performance@Component({ templateUrl: './enlarge-slide-image.component.html', standalone: true, + imports: [MatDialogModule], + changeDetection: ChangeDetectionStrategy.OnPush, })src/main/webapp/app/shared/metis/emoji/emoji.component.ts (1)
4-4: Consider using a more descriptive alias name.The numeric suffix in
EmojiComponent_1is not very descriptive. Consider using a more meaningful alias that describes its purpose, such asNgxEmojiComponentorEmojiMartComponent.-import { EmojiComponent as EmojiComponent_1 } from '@ctrl/ngx-emoji-mart/ngx-emoji'; +import { EmojiComponent as NgxEmojiComponent } from '@ctrl/ngx-emoji-mart/ngx-emoji';src/main/webapp/app/shared/metis/posting-button/posting-button.component.ts (1)
10-11: Consider adding ChangeDetectionStrategy.OnPushThe standalone configuration is correctly implemented. However, since this is a presentational component with input properties, it could benefit from OnPush change detection strategy for better performance.
@Component({ selector: 'button[jhi-posting-button]', templateUrl: './posting-button.component.html', standalone: true, imports: [FaIconComponent], + changeDetection: ChangeDetectionStrategy.OnPush, })src/main/webapp/app/shared/metis/conversations/one-to-one-chat.service.ts (1)
Line range hint
15-19: Enhance error handling and input validation.While the implementation is functional, consider these improvements for better robustness:
- Add input validation for courseId and loginOfChatPartner
- Implement error handling using catchError operator
- Consider using URL encoding for loginOfChatPartner
Here's a suggested implementation:
create(courseId: number, loginOfChatPartner: string): Observable<HttpResponse<OneToOneChatDTO>> { + if (!courseId || !loginOfChatPartner) { + throw new Error('courseId and loginOfChatPartner are required'); + } return this.http - .post<OneToOneChatDTO>(`${this.resourceUrl}${courseId}/one-to-one-chats`, [loginOfChatPartner], { observe: 'response' }) - .pipe(map(this.conversationService.convertDateFromServer)); + .post<OneToOneChatDTO>( + `${this.resourceUrl}${courseId}/one-to-one-chats`, + [encodeURIComponent(loginOfChatPartner)], + { observe: 'response' } + ) + .pipe( + map(this.conversationService.convertDateFromServer), + catchError(error => { + console.error('Error creating one-to-one chat:', error); + throw error; + }) + ); }src/main/webapp/app/shared/metis/posting-thread/posting-thread.component.ts (1)
Line range hint
14-26: Consider implementing OnDestroy for cleanupWhile the component looks good overall, consider implementing memory leak prevention:
- Implement OnDestroy interface to clean up the EventEmitter
- Consider unsubscribing from any subscriptions if they exist in the full implementation
Example implementation:
export class PostingThreadComponent implements OnDestroy { // ... existing code ... ngOnDestroy(): void { this.openThread.complete(); } }src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts (1)
Line range hint
20-20: Improve type safety and prevent memory leaks.
- Replace
anywith a proper type for the EventEmitter and method parameter:-@Output() emojiSelect: EventEmitter<any> = new EventEmitter(); +@Output() emojiSelect: EventEmitter<EmojiData> = new EventEmitter(); -onEmojiSelect(event: any) { +onEmojiSelect(event: EmojiData) {
- Consider implementing OnDestroy to clean up the EventEmitter:
export class EmojiPickerComponent implements OnDestroy { // ... existing code ... ngOnDestroy() { this.emojiSelect.complete(); } }Also applies to: 27-29
src/main/webapp/app/shared/metis/posting-create-edit-modal/posting-create-edit-modal.directive.ts (4)
10-12: Consider adding access modifiers and readonlyWhile the migration to the
injectfunction is correct, consider the following improvements for better encapsulation and immutability:- protected metisService = inject(MetisService); - protected modalService = inject(NgbModal); - protected formBuilder = inject(FormBuilder); + protected readonly metisService = inject(MetisService); + protected readonly modalService = inject(NgbModal); + protected readonly formBuilder = inject(FormBuilder);
8-9: Consider architectural improvementsA few suggestions to improve the code structure:
- Consider renaming the class to
AbstractPostingCreateEditModalDirectiveto better indicate its abstract nature- The generic type parameter
T extends Postingcould be more descriptive, e.g.,TPosting extends Posting
15-15: Add static flag to ViewChild decoratorFor better performance, consider adding the static flag to the ViewChild decorator:
- @ViewChild('postingEditor') postingEditor: TemplateRef<any>; + @ViewChild('postingEditor', { static: true }) postingEditor: TemplateRef<any>;This allows Angular to resolve the template reference during component initialization rather than after change detection.
16-16: Consider adding access modifier to modalTitleFor better encapsulation:
- modalTitle: string; + protected modalTitle: string;src/main/webapp/app/shared/metis/reaction.service.ts (3)
12-14: Consider enhancing property declarations for better type safetyWhile the migration to
inject()is good, there are opportunities for improvement:Consider applying these enhancements:
- protected http = inject(HttpClient); - public resourceUrl = 'api/courses/'; + protected readonly http = inject(HttpClient); + public readonly resourceUrl = 'api/courses/' as const;This makes the properties immutable and provides better type inference for the URL string.
Line range hint
21-45: Consider enhancing error handling and type safetyThe service methods could benefit from improved error handling and type safety:
Consider applying these improvements:
create(courseId: number, reaction: Reaction): Observable<EntityResponseType> { + const url = `${this.resourceUrl}${courseId}/postings/reactions`; return this.http - .post<Reaction>(`${this.resourceUrl}${courseId}/postings/reactions`, reaction, { observe: 'response' }) - .pipe(map(this.convertPostingResponseDateFromServer)); + .post<Reaction>(url, reaction, { observe: 'response' }) + .pipe( + map(this.convertPostingResponseDateFromServer), + catchError((error: HttpErrorResponse) => { + console.error('Error creating reaction:', error); + return throwError(() => error); + }) + ); }Add these imports:
import { catchError, throwError } from 'rxjs'; import { HttpErrorResponse } from '@angular/common/http';Similar error handling should be added to the
deletemethod as well.
Line range hint
1-45: Add proper subscription management to prevent memory leaksTo align with the memory leak prevention guideline:
Consider implementing one of these patterns:
- Use the async pipe in templates when consuming these observables
- Implement proper subscription cleanup in components:
// In the consuming component: private destroy$ = new Subject<void>(); ngOnInit() { this.reactionService.create(courseId, reaction).pipe( takeUntil(this.destroy$) ).subscribe(/* ... */); } ngOnDestroy() { this.destroy$.next(); this.destroy$.complete(); }src/main/webapp/app/shared/metis/course-discussion.directive.ts (2)
15-15: LGTM! Consider implementing OnDestroy interface.The migration to the
inject()function is well implemented. However, since the directive manages subscriptions, it should explicitly implement theOnDestroyinterface to ensure proper cleanup.Consider applying this change:
-export abstract class CourseDiscussionDirective { +export abstract class CourseDiscussionDirective implements OnDestroy { protected metisService = inject(MetisService); // ... other properties ... - onDestroy(): void { + ngOnDestroy(): void { this.paramSubscription?.unsubscribe(); this.postsSubscription?.unsubscribe(); }
Line range hint
37-64: Consider implementing a subscription management pattern.While the subscription cleanup is handled in
onDestroy, consider implementing a more robust subscription management pattern to prevent potential memory leaks.Consider applying this pattern:
private destroy$ = new Subject<void>(); // Use in subscriptions this.metisService.getFilteredPosts(this.currentPostContextFilter) .pipe(takeUntil(this.destroy$)) .subscribe(...); // In ngOnDestroy ngOnDestroy(): void { this.destroy$.next(); this.destroy$.complete(); }This approach:
- Automatically handles all subscriptions
- Reduces the risk of memory leaks
- Eliminates the need to manually track individual subscriptions
src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.ts (1)
Line range hint
22-53: Good preservation of component functionality during standalone migrationThe migration to standalone architecture maintains all essential functionality:
- Proper inheritance from
PostingHeaderDirective- Lifecycle hooks implementation
- Memory cleanup in
ngOnDestroy- User authority evaluation
Consider documenting this migration pattern for other components in the Artemis project.
src/main/webapp/app/shared/metis/conversations/group-chat.service.ts (2)
11-15: Consider improving resource URL declarationThe migration to inject() pattern is well implemented. However, consider these improvements:
- Make
resourceUrlprivate since it's only used within the service- Extract the URL path to a constant for better maintainability
+ private static readonly API_PATH = 'api/courses/'; private http = inject(HttpClient); private conversationService = inject(ConversationService); private accountService = inject(AccountService); - public resourceUrl = 'api/courses/'; + private resourceUrl = GroupChatService.API_PATH;
Line range hint
31-34: Improve null handling in removeUsersFromGroupChatThe current implementation could be more robust in handling the optional login parameter.
removeUsersFromGroupChat(courseId: number, groupChatId: number, logins?: string[]): Observable<HttpResponse<void>> { - // if no explicit login is give we assume self deregistration - const userLogins = logins ? logins : [this.accountService.userIdentity?.login]; + // If no explicit logins are provided, remove the current user from the group chat + const currentUserLogin = this.accountService.userIdentity?.login; + if (!currentUserLogin && !logins) { + throw new Error('No user login available for deregistration'); + } + const userLogins = logins ?? [currentUserLogin!]; return this.http.post<void>(`${this.resourceUrl}${courseId}/group-chats/${groupChatId}/deregister`, userLogins, { observe: 'response' }); }src/main/webapp/app/shared/metis/answer-post.service.ts (1)
14-14: Consider making the resourceUrl readonly and environment-based.To prevent accidental modifications and improve maintainability, consider:
- Making the URL readonly
- Moving the base URL to environment configuration
- public resourceUrl = 'api/courses/'; + public readonly resourceUrl = environment.apiUrl + '/courses/';src/main/webapp/app/shared/metis/posting-create-edit.directive.ts (1)
15-17: Consider adding readonly modifier to injected services.While the migration to
inject()is good, consider these improvements:
- Add
readonlyto prevent accidental reassignment- Group related services together (UI-related vs data-related)
- protected metisService = inject(MetisService); - protected modalService = inject(NgbModal); - protected formBuilder = inject(FormBuilder); + protected readonly metisService = inject(MetisService); + // UI-related services + protected readonly modalService = inject(NgbModal); + protected readonly formBuilder = inject(FormBuilder);src/main/webapp/app/shared/link-preview/components/link-preview/link-preview.component.ts (4)
Line range hint
31-33: Consider using the new inject() function for dependency injectionReplace constructor-based DI with the modern inject() pattern for better tree-shaking and alignment with current Angular practices.
-constructor(private metisService: MetisService) {} +private metisService = inject(MetisService);
Line range hint
35-37: Add null safety checks for posting propertyThe
ngOnInitmethod uses non-null assertion (!), butpostingis marked as optional. Consider adding proper null checks.ngOnInit() { - this.isAuthorOfOriginalPost = this.metisService.metisUserIsAuthorOfPosting(this.posting!); + this.isAuthorOfOriginalPost = this.posting ? this.metisService.metisUserIsAuthorOfPosting(this.posting) : false; }
Line range hint
76-84: Add proper error handling for service callsThe service calls lack error handling and have empty success handlers. This could lead to silent failures and missed state updates.
if (this.isReply) { this.metisService.updateAnswerPost(this.posting).subscribe({ - next: () => {}, + next: () => { + // Consider updating UI state or showing success message + }, + error: (error) => { + console.error('Failed to update answer post:', error); + // Consider showing error message to user + } }); } else { this.metisService.updatePost(this.posting).subscribe({ - next: () => {}, + next: () => { + // Consider updating UI state or showing success message + }, + error: (error) => { + console.error('Failed to update post:', error); + // Consider showing error message to user + } }); }
Line range hint
18-84: Implement proper subscription managementTo prevent memory leaks, consider implementing proper subscription cleanup using either the async pipe or manual unsubscription in ngOnDestroy.
export class LinkPreviewComponent implements OnInit { + private destroy$ = new Subject<void>(); + // ... existing code ... + + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + } + removeLinkPreview(linkPreview: LinkPreview) { // ... existing code ... if (this.isReply) { this.metisService.updateAnswerPost(this.posting).pipe( + takeUntil(this.destroy$) ).subscribe({ // ... handlers ... }); } else { this.metisService.updatePost(this.posting).pipe( + takeUntil(this.destroy$) ).subscribe({ // ... handlers ... }); } } }src/main/webapp/app/shared/pipes/reacting-users-on-posting.pipe.ts (3)
Line range hint
23-30: Critical: Potential memory leak in Observable subscriptionThe current implementation creates new subscriptions without cleanup on each transform call, which can lead to memory leaks. This is particularly problematic because:
- Pipes can be called frequently during change detection
- Each call creates a new subscription to
onLangChange- These subscriptions are never cleaned up
Consider this safer implementation:
+ private destroy$ = new Subject<void>(); + transform(reactingUsers: string[]): Observable<string> { - return new Observable((observer: any) => { - observer.next(this.updateReactingUsersString(reactingUsers)); - this.translateService.onLangChange.subscribe(() => { - observer.next(this.updateReactingUsersString(reactingUsers)); - }); - }); + return merge( + of(this.updateReactingUsersString(reactingUsers)), + this.translateService.onLangChange.pipe( + map(() => this.updateReactingUsersString(reactingUsers)) + ) + ).pipe( + takeUntil(this.destroy$) + ); } + + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }Don't forget to import:
import { Subject, merge, of } from 'rxjs'; import { takeUntil, map } from 'rxjs/operators';
Line range hint
36-61: Improve string manipulation and i18n handlingThe current string manipulation logic could be made more maintainable and performant.
Consider these improvements:
+ // Define translation keys as constants + const TRANSLATION_KEYS = { + YOU: 'artemisApp.metis.you', + REACTED_TOOLTIP: 'artemisApp.metis.reactedTooltip', + REACTED_TOOLTIP_TRIMMED: 'artemisApp.metis.reactedTooltipTrimmed', + AND: 'artemisApp.metis.and' + } as const; + private updateReactingUsersString(reactingUsers: string[]): string { if (reactingUsers.includes(PLACEHOLDER_USER_REACTED)) { if (reactingUsers.length === 1) { - return this.translateService.instant('artemisApp.metis.you'); + return this.translateService.instant(TRANSLATION_KEYS.YOU); } - reactingUsers = reactingUsers.filter((user) => user !== PLACEHOLDER_USER_REACTED); - reactingUsers = [this.translateService.instant('artemisApp.metis.you')].concat(reactingUsers); + const filteredUsers = reactingUsers.filter((user) => user !== PLACEHOLDER_USER_REACTED); + return this.formatUserList([ + this.translateService.instant(TRANSLATION_KEYS.YOU), + ...filteredUsers + ]); } + return this.formatUserList(reactingUsers); + } + + private formatUserList(users: string[]): string { const numberOfReactingUsers = users.length; if (numberOfReactingUsers > USER_COUNT_LIMIT) { - reactingUsers = reactingUsers.slice(0, USER_COUNT_LIMIT); - return reactingUsers.join(', ') + this.translateService.instant('artemisApp.metis.reactedTooltipTrimmed', { number: numberOfReactingUsers - USER_COUNT_LIMIT }); + const visibleUsers = users.slice(0, USER_COUNT_LIMIT).join(', '); + return `${visibleUsers}${this.translateService.instant(TRANSLATION_KEYS.REACTED_TOOLTIP_TRIMMED, { + number: numberOfReactingUsers - USER_COUNT_LIMIT + })}`; } - let listOfReactingUsers = reactingUsers.join(', ') + this.translateService.instant('artemisApp.metis.reactedTooltip'); - const lastCommaIndex = listOfReactingUsers.lastIndexOf(','); - if (lastCommaIndex > -1) { - const beforeLastComma = listOfReactingUsers.substring(0, lastCommaIndex); - const afterLastComma = listOfReactingUsers.substring(lastCommaIndex + 2, listOfReactingUsers.length); - listOfReactingUsers = beforeLastComma + this.translateService.instant('artemisApp.metis.and') + afterLastComma; - } - return listOfReactingUsers; + if (users.length === 1) { + return `${users[0]}${this.translateService.instant(TRANSLATION_KEYS.REACTED_TOOLTIP)}`; + } + const lastUser = users[users.length - 1]; + const otherUsers = users.slice(0, -1).join(', '); + return `${otherUsers}${this.translateService.instant(TRANSLATION_KEYS.AND)}${lastUser}${ + this.translateService.instant(TRANSLATION_KEYS.REACTED_TOOLTIP) + }`; }
Line range hint
23-24: Improve type safetyThe code uses
anytype and lacks proper type guards for array operations.Consider these type safety improvements:
- transform(reactingUsers: string[]): Observable<string> { - return new Observable((observer: any) => { + transform(reactingUsers: readonly string[]): Observable<string> { + if (!Array.isArray(reactingUsers)) { + throw new Error('Input must be an array of strings'); + } + return new Observable<string>((observer: Observer<string>) => {Also add type guard for the array content:
private isStringArray(arr: unknown[]): arr is string[] { return arr.every(item => typeof item === 'string'); }src/main/webapp/app/shared/link-preview/components/link-preview-container/link-preview-container.component.ts (4)
Line range hint
1-14: LGTM! Consider organizing imports.The migration to a standalone component and the use of modern Angular features (signals, inject) is well implemented. The component is properly configured with OnPush change detection and necessary imports.
Consider organizing imports into these groups (separated by newline):
- Angular core
- Application services
- Models
- Components
import { ChangeDetectionStrategy, Component, OnChanges, OnInit, inject, input, signal } from '@angular/core'; + import { LinkPreview, LinkPreviewService } from 'app/shared/link-preview/services/link-preview.service'; import { Link, LinkifyService } from 'app/shared/link-preview/services/linkify.service'; + import { User } from 'app/core/user/user.model'; import { Posting } from 'app/entities/metis/posting.model'; + import { LinkPreviewComponent } from '../link-preview/link-preview.component';
Line range hint
33-44: Consider optimizing lifecycle hooks to prevent duplicate API calls.The current implementation calls
findPreviews()in bothngOnInitandngOnChanges, which could lead to duplicate API calls when the component initializes. Consider callingreloadLinkPreviews()only inngOnInitif the initial data is available.ngOnInit() { - this.findPreviews(); + if (this.data()) { + this.reloadLinkPreviews(); + } } ngOnChanges() { - this.reloadLinkPreviews(); + if (this.data()) { + this.reloadLinkPreviews(); + } }
Line range hint
46-46: Address the TODO comment regarding configurable limit.The hardcoded limit of 5 link previews should be moved to a configuration setting, potentially at the course level as noted in the TODO.
Would you like me to help create a GitHub issue to track this enhancement or provide an implementation for configurable link preview limits?
Also applies to: 47-48
Line range hint
49-77: Prevent potential memory leaks and improve error handling.The subscription to
linkPreviewService.fetchLink()is not being cleaned up, which could lead to memory leaks. Additionally, error handling could be improved.Consider these improvements:
private findPreviews() { const links: Link[] = this.linkifyService.find(this.data() ?? ''); + // Store subscriptions for cleanup + const subscriptions: Subscription[] = []; links .filter((link) => !link.isLinkPreviewRemoved) .slice(0, 5) .forEach((link) => { - this.linkPreviewService.fetchLink(link.href).subscribe({ + const subscription = this.linkPreviewService.fetchLink(link.href).subscribe({ next: (linkPreview) => { linkPreview.shouldPreviewBeShown = !!(linkPreview.url && linkPreview.title && linkPreview.description && linkPreview.image); // ... existing code ... }, + error: (error) => { + console.error('Failed to fetch link preview:', error); + this.hasError.set(true); + this.loaded.set(true); + this.showLoadingsProgress.set(false); + } }); + subscriptions.push(subscription); }); + + // Clean up subscriptions when component is destroyed + this.destroy$.next(); + this.destroy$.complete(); }Also add this to your component:
private readonly destroy$ = new Subject<void>(); ngOnDestroy() { this.destroy$.next(); this.destroy$.complete(); }src/main/webapp/app/shared/metis/message/message-inline-input/message-inline-input.component.ts (1)
Line range hint
31-85: Consider enhancing error handling and subscription management.While the implementation is functional, consider these improvements:
- Add specific error handling instead of just resetting isLoading
- Consider using takeUntil pattern for subscription management to prevent memory leaks
Example implementation:
+ private destroy$ = new Subject<void>(); + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + } createPosting(): void { this.posting.content = this.formGroup.get('content')?.value; this.metisService.createPost(this.posting) + .pipe(takeUntil(this.destroy$)) .subscribe({ next: (post: Post) => { this.isLoading = false; this.onCreate.emit(post); }, error: (error) => { this.isLoading = false; + // Handle specific error cases + console.error('Failed to create post:', error); + // Consider adding user feedback }, }); }src/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.ts (2)
Line range hint
39-53: Consider simplifying and type-safeguarding ngOnChanges.The current implementation has potential type-safety issues and could be simplified.
Consider this safer implementation:
- ngOnChanges(changes: SimpleChanges | void) { - if (this.formGroup && changes) { - for (const propName in changes) { - if (changes.hasOwnProperty(propName) && propName === 'posting') { - if (changes['posting'].previousValue?.post?.id === changes['posting'].currentValue?.post?.id) { - this.posting.content = this.formGroup.get('content')?.value; - } - } - } - } + ngOnChanges(changes: SimpleChanges | void): void { + if (this.formGroup && changes?.['posting']) { + const postingChange = changes['posting']; + if (postingChange.previousValue?.post?.id === postingChange.currentValue?.post?.id) { + this.posting.content = this.formGroup.get('content')?.value ?? ''; + } + } super.ngOnChanges(); }
Line range hint
71-93: Enhance error handling in posting operations.The error handling in createPosting and updatePosting could be improved with more specific error handling and DRY implementation.
Consider extracting the common error handling:
+ private handlePostingError(error: any): void { + console.error('Error during posting operation:', error); + this.isLoading = false; + } + createPosting(): void { this.posting.content = this.formGroup.get('content')?.value; this.metisService.createAnswerPost(this.posting).subscribe({ next: (answerPost: AnswerPost) => { this.resetFormGroup(''); this.isLoading = false; this.onCreate.emit(answerPost); }, - error: () => { - this.isLoading = false; - }, + error: this.handlePostingError.bind(this), }); } updatePosting(): void { this.posting.content = this.formGroup.get('content')?.value; this.metisService.updateAnswerPost(this.posting).subscribe({ next: () => { this.isLoading = false; }, - error: () => { - this.isLoading = false; - }, + error: this.handlePostingError.bind(this), }); }src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts (4)
1-1: LGTM! Consider organizing imports for better maintainability.The transition to a standalone component is well implemented with all necessary dependencies properly imported and declared.
Consider organizing imports into these groups with a blank line between each:
- Angular core imports
- Angular form imports
- Third-party library imports (ng-bootstrap)
- Application-specific imports
import { Component, EventEmitter, Input, Output, ViewContainerRef, ViewEncapsulation, inject } from '@angular/core'; + import { FormBuilder, FormsModule, ReactiveFormsModule, Validators } from '@angular/forms'; + import { NgbModal } from '@ng-bootstrap/ng-bootstrap'; + import { PostingCreateEditModalDirective } from 'app/shared/metis/posting-create-edit-modal/posting-create-edit-modal.directive'; import { AnswerPost } from 'app/entities/metis/answer-post.model'; // ... rest of the importsAlso applies to: 6-6, 9-11, 18-19
Line range hint
63-77: Prevent memory leaks in subscription handling.The
createPostingmethod creates a subscription but doesn't handle cleanup. This could lead to memory leaks, especially if the component is destroyed while the request is pending.Consider using either:
- The async pipe in the template
- Storing and cleaning up the subscription
+ private destroy$ = new Subject<void>(); createPosting(): void { this.posting.content = this.formGroup.get('content')?.value; - this.metisService.createAnswerPost(this.posting).subscribe({ + this.metisService.createAnswerPost(this.posting) + .pipe(takeUntil(this.destroy$)) + .subscribe({ next: (answerPost: AnswerPost) => { this.resetFormGroup(); this.isLoading = false; this.onCreate.emit(answerPost); this.createEditAnswerPostContainerRef?.clear(); }, error: () => { this.isLoading = false; + // Add user feedback + // this.toastr.error('Failed to create post'); }, }); } + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }
Line range hint
48-54: Consider enhancing form validation.The form validation could be more robust by adding custom validators for specific content requirements.
Consider adding:
- A custom validator for minimum content length
- A validator for special characters if needed
- A validator for formatting requirements
resetFormGroup(): void { this.posting = this.posting || { content: '' }; this.formGroup = this.formBuilder.group({ content: [ this.posting.content, [ Validators.required, Validators.maxLength(this.maxContentLength), PostContentValidationPattern, + Validators.minLength(10), // Ensure meaningful content + // Add custom validators as needed ] ], }); }
Line range hint
78-92: Enhance error handling in updatePosting method.Similar to
createPosting, theupdatePostingmethod needs subscription cleanup and better error handling.Apply similar changes as suggested for
createPosting:
- Add subscription cleanup
- Improve error feedback
- Consider adding loading state management
updatePosting(): void { this.posting.content = this.formGroup.get('content')?.value; - this.metisService.updateAnswerPost(this.posting).subscribe({ + this.metisService.updateAnswerPost(this.posting) + .pipe(takeUntil(this.destroy$)) + .subscribe({ next: (updatedPost: AnswerPost) => { this.postingUpdated.emit(updatedPost); this.isLoading = false; this.isInputOpen = false; this.createEditAnswerPostContainerRef?.clear(); }, error: () => { this.isLoading = false; + // Add user feedback + // this.toastr.error('Failed to update post'); }, }); }src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-tag-selector/post-tag-selector.component.ts (3)
1-1: LGTM! Standalone component configuration looks good.The component has been properly configured as standalone with all necessary dependencies. The migration from module-based to standalone follows Angular's recommended patterns.
Consider organizing the imports array by grouping related Material components together for better maintainability. For example:
imports: [ // Material Form Controls MatFormField, ReactiveFormsModule, FormsModule, // Material Chips MatChipGrid, MatChipRow, MatChipRemove, MatChipInput, // Material Autocomplete MatAutocompleteTrigger, MatAutocomplete, MatOption, // Other FaIconComponent, AsyncPipe, ArtemisSharedCommonModule, ]Also applies to: 5-13, 19-34
Line range hint
91-102: Consider extracting the tag limit constant and improving user feedback.The hardcoded tag limit of 3 could be extracted as a constant for better maintainability. Additionally, consider providing user feedback when the tag limit is reached.
Here's a suggested improvement:
+ private readonly MAX_TAGS = 3; + @Output() onTagLimitReached = new EventEmitter<void>(); onItemAdd(event: MatChipInputEvent) { const tagString = (event.value || '').trim(); - if (tagString && !this.tags?.includes(tagString) && this.tags?.length < 3) { + if (tagString && !this.tags?.includes(tagString) && this.tags?.length < this.MAX_TAGS) { if (!this.tags) { this.tags = []; } this.tags.push(tagString); this.postTagsChange.emit(this.tags); + } else if (this.tags?.length >= this.MAX_TAGS) { + this.onTagLimitReached.emit(); }
Line range hint
64-71: Consider optimizing the filter function.The current filter implementation creates a new array on each keystroke. Consider memoizing the filtered results for better performance with large tag lists.
Here's a suggested improvement:
+ private lastFilterValue: string; + private lastFilterResult: string[]; private _filter(value: string): string[] { + if (value === this.lastFilterValue) { + return this.lastFilterResult; + } const filterValue = value.toLowerCase(); - return this.tags.filter((tag) => tag.toLowerCase().includes(filterValue)); + this.lastFilterValue = value; + this.lastFilterResult = this.tags.filter((tag) => tag.toLowerCase().includes(filterValue)); + return this.lastFilterResult; }src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts (1)
22-36: Consider grouping imports by category for better maintainability.While the imports are correctly configured, consider organizing them into logical groups:
- Angular core modules (NgClass, AsyncPipe, KeyValuePipe)
- Angular Material/CDK modules (CdkOverlayOrigin, CdkConnectedOverlay)
- Third-party modules (NgbTooltip, FaIconComponent)
- Application-specific modules (remaining imports)
imports: [ + // Angular core modules + NgClass, + AsyncPipe, + KeyValuePipe, + + // Angular CDK modules + CdkOverlayOrigin, + CdkConnectedOverlay, + + // Third-party modules NgbTooltip, - EmojiComponent, - CdkOverlayOrigin, FaIconComponent, - CdkConnectedOverlay, + + // Application modules + EmojiComponent, EmojiPickerComponent, - NgClass, ArtemisConfirmIconModule, - AsyncPipe, - KeyValuePipe, ArtemisSharedCommonModule, ReactingUsersOnPostingPipe, ],src/main/webapp/app/shared/metis/conversations/channel.service.ts (1)
Line range hint
17-93: Consider architectural improvements for better maintainabilityThe HTTP methods implementation is solid, but consider these architectural improvements:
- Extract API routes into constants or a configuration object for better maintainability
- Implement a consistent error handling strategy across all methods
- Consider adding retry logic for failed requests
Example implementation for route constants:
private static readonly ROUTES = { channels: (courseId: number) => `/api/courses/${courseId}/channels`, exerciseChannel: (courseId: number, exerciseId: number) => `/api/courses/${courseId}/exercises/${exerciseId}/channel`, // ... other routes } as const;src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (2)
Line range hint
1-26: Consider organizing imports by category for better maintainability.While the imports are correct, consider organizing them into distinct categories:
- Angular core imports
- Third-party library imports (FontAwesome)
- Application-specific imports
import { Component, EventEmitter, Input, OnInit, Output, inject } from '@angular/core'; +import { RouterLink } from '@angular/router'; +import { MatDialog } from '@angular/material/dialog'; + +import { IconProp } from '@fortawesome/fontawesome-svg-core'; +import { FaIconComponent } from '@fortawesome/angular-fontawesome'; import { faAt, faBan, faChalkboardUser, // ... other icons } from '@fortawesome/free-solid-svg-icons'; -import { IconProp } from '@fortawesome/fontawesome-svg-core'; + +import { AccountService } from 'app/core/auth/account.service'; +import { FileService } from 'app/shared/http/file.service'; import { PostingContentPart, ReferenceType } from '../../metis.util'; -import { FileService } from 'app/shared/http/file.service'; import { EnlargeSlideImageComponent } from 'app/shared/metis/posting-content/enlarge-slide-image/enlarge-slide-image.component'; -import { MatDialog } from '@angular/material/dialog'; -import { AccountService } from 'app/core/auth/account.service'; -import { RouterLink } from '@angular/router'; -import { FaIconComponent } from '@fortawesome/angular-fontawesome'; import { TranslateDirective } from '../../../language/translate.directive'; import { HtmlForPostingMarkdownPipe } from 'app/shared/pipes/html-for-posting-markdown.pipe';
Line range hint
44-45: Consider resetting state on input changes.The
hasClickedUserReferenceflag might prevent subsequent clicks if the component is reused with different content.export class PostingContentPartComponent implements OnInit { @Input() postingContentPart: PostingContentPart; + + ngOnChanges(changes: SimpleChanges): void { + if (changes['postingContentPart']) { + this.hasClickedUserReference = false; + } + }src/main/webapp/app/shared/metis/post.service.ts (3)
18-18: Consider making resourceUrl readonly and moving it above httpFor better code organization and immutability:
- Consider making the URL readonly since it's a constant
- Consider moving it above the http property (constants before instance properties)
- public resourceUrl = 'api/courses/'; + public readonly resourceUrl = 'api/courses/';
Line range hint
25-31: Add error handling to HTTP operationsThe HTTP operations (create, update, delete) should include error handling using catchError operator from RxJS to properly handle and propagate errors to the UI layer.
Example implementation:
import { catchError } from 'rxjs/operators'; // Apply to each HTTP operation create(courseId: number, post: Post): Observable<EntityResponseType> { const copy = this.convertPostingDateFromClient(post); return this.http .post<Post>( `${this.resourceUrl}${courseId}${PostService.getResourceEndpoint(undefined, post)}`, copy, { observe: 'response' } ) .pipe( map(this.convertPostingResponseDateFromServer), catchError(error => { console.error('Error creating post:', error); throw error; }) ); }Also applies to: 77-83, 92-95
Line range hint
25-31: Consider encoding URL parameters for safetyThe URL construction in methods using template literals could benefit from proper encoding of parameters to prevent potential injection vulnerabilities.
private static getResourceEndpoint(postContextFilter?: PostContextFilter, post?: Post): string { const endpoint = post?.conversation || postContextFilter?.conversationId || postContextFilter?.courseWideChannelIds ? '/messages' : '/posts'; return encodeURIComponent(endpoint); } // Usage in methods create(courseId: number, post: Post): Observable<EntityResponseType> { const copy = this.convertPostingDateFromClient(post); const endpoint = PostService.getResourceEndpoint(undefined, post); const url = `${this.resourceUrl}${encodeURIComponent(courseId)}${endpoint}`; return this.http.post<Post>(url, copy, { observe: 'response' }) .pipe(map(this.convertPostingResponseDateFromServer)); }Also applies to: 77-83, 92-95, 134-144
src/main/webapp/app/shared/metis/conversations/conversation.service.ts (3)
1-1: LGTM! Consider adding type annotations for better type safety.The migration to the new
injectfunction for dependency injection is well implemented. However, consider adding explicit type annotations to the injected services for better type safety and code clarity.- protected http = inject(HttpClient); - protected translationService = inject(TranslateService); - protected accountService = inject(AccountService); + protected readonly http: HttpClient = inject(HttpClient); + protected readonly translationService: TranslateService = inject(TranslateService); + protected readonly accountService: AccountService = inject(AccountService);Also applies to: 35-39
Line range hint
164-179: Fix typo in method namecreatSearchPagingParamsThe method name has a typo and should be
createSearchPagingParams.- private creatSearchPagingParams = ( + private createSearchPagingParams = (
Line range hint
91-96: Consider implementing memory leak prevention for HTTP observablesHTTP observables should be properly managed to prevent memory leaks. Consider using the
takeUntiloperator with a destruction subject for observables that might have a longer lifecycle.Example implementation:
@Injectable({ providedIn: 'root' }) export class ConversationService implements OnDestroy { private readonly destroy$ = new Subject<void>(); searchMembersOfConversation(...): Observable<HttpResponse<ConversationUserDTO[]>> { const params = this.createSearchPagingParams(sortingParameters, page, size, loginOrName, filter); return this.http.get<ConversationUserDTO[]>( `${this.resourceUrl}${courseId}/conversations/${conversationId}/members/search`, { observe: 'response', params } ).pipe( takeUntil(this.destroy$) ); } ngOnDestroy(): void { this.destroy$.next(); this.destroy$.complete(); } }src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.directive.ts (1)
Line range hint
62-63: Consider consistent event emitter patternsThe code currently mixes two different patterns for event emitters:
- Traditional
@Output()withEventEmitter- New
output()functionConsider migrating all event emitters to use the same pattern for better maintainability and consistency.
Example of consistent usage with the new pattern:
-@Output() isModalOpen = new EventEmitter<void>(); +isModalOpen = output<void>(); -@Output() openPostingCreateEditModal = new EventEmitter<void>(); +openPostingCreateEditModal = output<void>(); -@Output() reactionsUpdated = new EventEmitter<Reaction[]>(); +reactionsUpdated = output<Reaction[]>();src/main/webapp/app/overview/discussion-section/discussion-section.component.ts (1)
62-66: Consider grouping injected services.The migration to inject() function is implemented correctly. Consider grouping the injected services by their responsibility (e.g., routing-related, form-related, etc.) with a blank line between groups for better organization.
protected metisService = inject(MetisService); private channelService = inject(ChannelService); + private activatedRoute = inject(ActivatedRoute); private router = inject(Router); + private formBuilder = inject(FormBuilder);src/main/webapp/app/shared/metis/post/post.component.ts (4)
15-16: LGTM! Clean migration to standalone component.The migration to a standalone component follows Angular best practices with proper import declarations. All necessary dependencies are correctly included in the imports array.
Consider grouping related imports together (e.g., Angular core, Material/CDK, feature components) for better organization.
Also applies to: 21-21, 28-28, 36-44, 57-76
Line range hint
171-183: Prevent potential memory leaks in scroll handling.The scroll manipulation methods directly manipulate DOM styles and could lead to memory leaks. Consider using Angular's lifecycle hooks for cleanup.
+ private scrollContainer: HTMLElement | null = null; + + ngOnInit() { + this.scrollContainer = this.document.querySelector('.posting-infinite-scroll-container'); + // ... existing code + } + disableBodyScroll() { - const mainContainer = this.document.querySelector('.posting-infinite-scroll-container'); - if (mainContainer) { - this.renderer.setStyle(mainContainer, 'overflow', 'hidden'); + if (this.scrollContainer) { + this.renderer.setStyle(this.scrollContainer, 'overflow', 'hidden'); } } enableBodyScroll() { - const mainContainer = this.document.querySelector('.posting-infinite-scroll-container'); - if (mainContainer) { - this.renderer.setStyle(mainContainer, 'overflow-y', 'auto'); + if (this.scrollContainer) { + this.renderer.setStyle(this.scrollContainer, 'overflow-y', 'auto'); } } + ngOnDestroy() { + this.enableBodyScroll(); + this.scrollContainer = null; + }
Line range hint
185-189: Consider using RxJS for click outside handling.The current implementation using
@HostListenercould be improved using RxJS for better cleanup and testing.- @HostListener('document:click', ['$event']) - onClickOutside() { - this.showDropdown = false; - this.enableBodyScroll(); - } + private destroy$ = new Subject<void>(); + + ngOnInit() { + fromEvent(document, 'click') + .pipe( + takeUntil(this.destroy$), + filter(() => this.showDropdown) + ) + .subscribe(() => { + this.showDropdown = false; + this.enableBodyScroll(); + }); + } + + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + }
Line range hint
249-251: Optimize change detection handling.Manual change detection in
ngAfterContentCheckedcould impact performance. Consider usingmarkForCheck()instead ofdetectChanges()when possible.ngAfterContentChecked() { - this.changeDetector.detectChanges(); + this.changeDetector.markForCheck(); }src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (3)
Line range hint
26-43: Standardize input/output declarations for consistencyThe component mixes old-style @Input/@output decorators with new input()/output() syntax. Consider standardizing to the new syntax for better maintainability.
Apply this pattern to all inputs:
-@Input() content?: string; +content = input<string | undefined>();Consider using DestroyRef for subscription management
The manual subscription cleanup could be replaced with Angular's DestroyRef for more robust memory management.
+private destroyRef = inject(DestroyRef); -private postsSubscription: Subscription; ngOnInit(): void { if (!this.isSubscribeToMetis()) { const patternMatches: PatternMatch[] = this.getPatternMatches(); this.computePostingContentParts(patternMatches); } else { - this.computeContentPartsOfPosts(); + this.metisService.posts + .pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe((posts: Post[]) => { + this.currentlyLoadedPosts = posts; + const patternMatches: PatternMatch[] = this.getPatternMatches(); + this.computePostingContentParts(patternMatches); + }); } } -ngOnDestroy(): void { - this.postsSubscription?.unsubscribe(); -}
Line range hint
89-196: Refactor computePostingContentParts for better maintainabilityThe method is complex with multiple responsibilities. Consider breaking it down into smaller, focused methods:
- Extract reference type handling into separate methods:
private handlePostReference(patternMatch: PatternMatch, referencedId: string): Partial<PostingContentPart> { const referencedPost = this.currentlyLoadedPosts.find((post: Post) => post.id! === +referencedId); const referenceStr = this.content!.substring(patternMatch.startIndex, patternMatch.endIndex); return { referenceStr, linkToReference: this.metisService.getLinkForPost(), queryParams: referencedPost ? this.metisService.getQueryParamsForPost(referencedPost) : { searchText: referenceStr } as Params }; }
- Use a strategy pattern for different reference types:
private referenceHandlers = new Map<ReferenceType, (match: PatternMatch) => Partial<PostingContentPart>>([ [ReferenceType.POST, this.handlePostReference.bind(this)], [ReferenceType.LECTURE, this.handleLectureReference.bind(this)], // ... other handlers ]);Consider caching pattern regex
The complex regex pattern in getPatternMatches could be cached as a static property to avoid recompilation.
-const pattern = /(?<POST>#\d+)|...)/g; +private static readonly REFERENCE_PATTERN = /(?<POST>#\d+)|...)/g;
Line range hint
197-234: Optimize pattern matching performanceThe pattern matching implementation could be optimized for better performance with large content:
- Consider using a more efficient approach for multiple pattern matching:
private getPatternMatches(): PatternMatch[] { if (!this.content) return []; return Array.from(this.content.matchAll(PostingContentComponent.REFERENCE_PATTERN)) .map(match => ({ startIndex: match.index!, endIndex: match.index! + match[0].length, referenceType: this.determineReferenceType(match.groups!) })); } private determineReferenceType(groups: { [key: string]: string }): ReferenceType { return Object.entries(groups) .find(([_, value]) => value !== undefined)?.[0] as ReferenceType; }src/main/webapp/app/shared/metis/metis-conversation.service.ts (2)
Line range hint
1-28: Consider providing the service in root if it's widely usedThe service is currently decorated with
@Injectable()without being provided in root. If this service is used across multiple feature modules, consider providing it in root to ensure a single instance is shared.-@Injectable() +@Injectable({ + providedIn: 'root' +})
Line range hint
65-74: Consider using an initialization method for better organizationWhile the constructor implementation is functional, consider moving the initialization logic to a separate method for better organization and testability. This would also allow for async initialization if needed in the future.
constructor() { + this.initialize(); + } + + private initialize(): void { this.accountService.identity().then((user: User) => { this.userId = user.id!; }); this.activeConversationSubscription = this.notificationService.newOrUpdatedMessage.subscribe((postDTO: MetisPostDTO) => { if (postDTO.action === MetisPostAction.CREATE && postDTO.post.author?.id !== this.userId) { this.handleNewMessage(postDTO.post.conversation?.id, postDTO.post.conversation?.lastMessageDate); } }); }src/main/webapp/app/shared/metis/metis.service.ts (4)
64-66: Consider moving initialization logic out of constructor.The constructor contains business logic that could be moved to lifecycle hooks or initialization methods for better separation of concerns.
-constructor() { - const notificationService = inject(NotificationService); - - this.accountService.identity().then((user: User) => { - this.user = user!; - }); - - this.courseWideTopicSubscription = notificationService.newOrUpdatedMessage.subscribe(this.handleNewOrUpdatedMessage); -} +constructor() { + this.initializeService(); +} + +private initializeService(): void { + const notificationService = inject(NotificationService); + + this.accountService.identity().then((user: User) => { + this.user = user!; + }); + + this.courseWideTopicSubscription = notificationService.newOrUpdatedMessage.subscribe(this.handleNewOrUpdatedMessage); +}
Line range hint
509-514: Potential memory leak in subscription handling.The
savePost,removeSavedPost, and similar methods create subscriptions without cleanup. Consider usingtakeUntilor storing these subscriptions for proper cleanup.+private destroy$ = new Subject<void>(); public savePost(post: Posting) { this.setIsSavedAndStatusOfPost(post, true, post.savedPostStatus as SavedPostStatus); - this.savedPostService.savePost(post).subscribe({ + this.savedPostService.savePost(post).pipe( + takeUntil(this.destroy$) + ).subscribe({ next: () => {}, }); this.posts$.next(this.cachedPosts); } ngOnDestroy(): void { if (this.subscriptionChannel) { this.jhiWebsocketService.unsubscribe(this.subscriptionChannel); } this.courseWideTopicSubscription.unsubscribe(); + this.destroy$.next(); + this.destroy$.complete(); }Also applies to: 606-608
Line range hint
472-486: Add error handling for WebSocket connections.The WebSocket subscription creation lacks error handling for connection failures or disconnections. Consider adding error handlers and reconnection logic.
private createWebsocketSubscription(channel: string): void { if (this.subscriptionChannel === channel) { return; } if (this.subscriptionChannel) { this.jhiWebsocketService.unsubscribe(this.subscriptionChannel); this.subscriptionChannel = undefined; } this.subscriptionChannel = channel; - this.jhiWebsocketService.subscribe(this.subscriptionChannel); - this.jhiWebsocketService.receive(this.subscriptionChannel).subscribe(this.handleNewOrUpdatedMessage); + try { + this.jhiWebsocketService.subscribe(this.subscriptionChannel); + this.jhiWebsocketService.receive(this.subscriptionChannel) + .pipe( + takeUntil(this.destroy$), + retryWhen(errors => errors.pipe( + delay(1000), + take(3) + )) + ) + .subscribe({ + next: this.handleNewOrUpdatedMessage, + error: (error) => console.error('WebSocket error:', error) + }); + } catch (error) { + console.error('Failed to create WebSocket subscription:', error); + } }
Line range hint
536-557: Optimize array operations for better performance.The
handleNewOrUpdatedMessagemethod performs multiple array operations that could be optimized for better performance.-const indexToUpdate = this.cachedPosts.findIndex((post) => post.id === postDTO.post.id); -if (indexToUpdate > -1) { - postDTO.post.authorRole = this.cachedPosts[indexToUpdate].authorRole; - postDTO.post.answers?.forEach((answer: AnswerPost) => { - const cachedAnswer = this.cachedPosts[indexToUpdate].answers?.find((a) => a.id === answer.id); - if (cachedAnswer) { - answer.authorRole = cachedAnswer.authorRole; - } - }); - this.cachedPosts[indexToUpdate] = postDTO.post; -} +// Create a Map for O(1) lookup +const answersMap = new Map( + this.cachedPosts[indexToUpdate]?.answers?.map(a => [a.id, a]) ?? [] +); + +if (indexToUpdate > -1) { + postDTO.post.authorRole = this.cachedPosts[indexToUpdate].authorRole; + postDTO.post.answers?.forEach((answer: AnswerPost) => { + const cachedAnswer = answersMap.get(answer.id); + if (cachedAnswer) { + answer.authorRole = cachedAnswer.authorRole; + } + }); + this.cachedPosts[indexToUpdate] = postDTO.post; +}src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (1)
62-64: Prefer Constructor Injection OverinjectFunction in ComponentsWhile the
injectfunction is available, it's standard practice in Angular components to use constructor injection for dependencies. This enhances readability and aligns with common Angular coding conventions.Consider refactoring as follows:
export class AnswerPostComponent extends PostingDirective<AnswerPost> implements OnInit, OnChanges { - changeDetector = inject(ChangeDetectorRef); - renderer = inject(Renderer2); - private document = inject<Document>(DOCUMENT); + constructor( + private changeDetector: ChangeDetectorRef, + private renderer: Renderer2, + @Inject(DOCUMENT) private document: Document + ) { + super(); + }src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (1)
24-25: Verify module imports for optimization and consider lazy loadingIncluding
ArtemisSharedCommonModulemay introduce unnecessary code into the bundle. Ensure that only the required modules are imported to optimize performance. Consider lazy loading components where appropriate to adhere to the coding guideline oflazy_loading:true.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (40)
src/main/webapp/app/overview/discussion-section/discussion-section.component.ts(2 hunks)src/main/webapp/app/shared/link-preview/components/link-preview-container/link-preview-container.component.ts(1 hunks)src/main/webapp/app/shared/link-preview/components/link-preview/link-preview.component.ts(1 hunks)src/main/webapp/app/shared/metis/answer-post.service.ts(2 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts(3 hunks)src/main/webapp/app/shared/metis/conversations/channel.service.ts(2 hunks)src/main/webapp/app/shared/metis/conversations/conversation.service.ts(2 hunks)src/main/webapp/app/shared/metis/conversations/group-chat.service.ts(1 hunks)src/main/webapp/app/shared/metis/conversations/one-to-one-chat.service.ts(2 hunks)src/main/webapp/app/shared/metis/course-discussion.directive.ts(2 hunks)src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts(1 hunks)src/main/webapp/app/shared/metis/emoji/emoji.component.ts(1 hunks)src/main/webapp/app/shared/metis/message/message-inline-input/message-inline-input.component.ts(1 hunks)src/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.ts(1 hunks)src/main/webapp/app/shared/metis/metis-conversation.service.ts(3 hunks)src/main/webapp/app/shared/metis/metis.module.ts(0 hunks)src/main/webapp/app/shared/metis/metis.service.ts(2 hunks)src/main/webapp/app/shared/metis/post.service.ts(2 hunks)src/main/webapp/app/shared/metis/post/post.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-button/posting-button.component.ts(1 hunks)src/main/webapp/app/shared/metis/posting-content/enlarge-slide-image/enlarge-slide-image.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts(2 hunks)src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts(2 hunks)src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts(1 hunks)src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.html(2 hunks)src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-tag-selector/post-tag-selector.component.ts(1 hunks)src/main/webapp/app/shared/metis/posting-create-edit-modal/posting-create-edit-modal.directive.ts(2 hunks)src/main/webapp/app/shared/metis/posting-create-edit.directive.ts(2 hunks)src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts(3 hunks)src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.ts(1 hunks)src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.ts(1 hunks)src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts(3 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.directive.ts(2 hunks)src/main/webapp/app/shared/metis/posting-thread/posting-thread.component.ts(1 hunks)src/main/webapp/app/shared/metis/reaction.service.ts(2 hunks)src/main/webapp/app/shared/pipes/html-for-posting-markdown.pipe.ts(1 hunks)src/main/webapp/app/shared/pipes/reacting-users-on-posting.pipe.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/webapp/app/shared/metis/metis.module.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.html
🧰 Additional context used
📓 Path-based instructions (38)
src/main/webapp/app/overview/discussion-section/discussion-section.component.ts (1)
src/main/webapp/app/shared/link-preview/components/link-preview-container/link-preview-container.component.ts (1)
src/main/webapp/app/shared/link-preview/components/link-preview/link-preview.component.ts (1)
src/main/webapp/app/shared/metis/answer-post.service.ts (1)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (1)
src/main/webapp/app/shared/metis/conversations/channel.service.ts (1)
src/main/webapp/app/shared/metis/conversations/conversation.service.ts (1)
src/main/webapp/app/shared/metis/conversations/group-chat.service.ts (1)
src/main/webapp/app/shared/metis/conversations/one-to-one-chat.service.ts (1)
src/main/webapp/app/shared/metis/course-discussion.directive.ts (1)
src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts (1)
src/main/webapp/app/shared/metis/emoji/emoji.component.ts (1)
src/main/webapp/app/shared/metis/message/message-inline-input/message-inline-input.component.ts (1)
src/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.ts (1)
src/main/webapp/app/shared/metis/metis-conversation.service.ts (1)
src/main/webapp/app/shared/metis/metis.service.ts (1)
src/main/webapp/app/shared/metis/post.service.ts (1)
src/main/webapp/app/shared/metis/post/post.component.ts (1)
src/main/webapp/app/shared/metis/posting-button/posting-button.component.ts (1)
src/main/webapp/app/shared/metis/posting-content/enlarge-slide-image/enlarge-slide-image.component.ts (1)
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (1)
src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (1)
src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts (1)
src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts (1)
src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-tag-selector/post-tag-selector.component.ts (1)
src/main/webapp/app/shared/metis/posting-create-edit-modal/posting-create-edit-modal.directive.ts (1)
src/main/webapp/app/shared/metis/posting-create-edit.directive.ts (1)
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (1)
src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.ts (1)
src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.ts (1)
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (1)
src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts (1)
src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts (1)
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.directive.ts (1)
src/main/webapp/app/shared/metis/posting-thread/posting-thread.component.ts (1)
src/main/webapp/app/shared/metis/reaction.service.ts (1)
src/main/webapp/app/shared/pipes/html-for-posting-markdown.pipe.ts (1)
src/main/webapp/app/shared/pipes/reacting-users-on-posting.pipe.ts (1)
🔇 Additional comments (72)
src/main/webapp/app/shared/metis/posting-content/enlarge-slide-image/enlarge-slide-image.component.ts (2)
Line range hint 1-6: LGTM! Clean imports and well-defined interface.
The imports are minimal and specific, and the interface follows proper naming conventions.
Line range hint 1-14: Verify dialog component usage across the codebase.
Let's ensure this component is properly integrated with the dialog system.
✅ Verification successful
Dialog component integration is properly implemented
The verification shows that the component is correctly integrated with Angular Material's dialog system:
- The component is properly opened in
posting-content-part.components.tsusingdialog.open()with correct data passing - The
DialogDatainterface is consistently used across the codebase - The template correctly uses the injected data to display the enlarged image
- The dialog configuration includes appropriate sizing constraints (
maxWidth: '95vw')
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where this dialog is opened
echo "Checking dialog opening patterns..."
rg -l "EnlargeSlideImageComponent" | xargs rg "dialog.open.*EnlargeSlideImageComponent" -A 2
# Verify the data interface usage
echo "Verifying DialogData interface usage..."
rg "slideToReference.*:" --type typescript
Length of output: 782
Script:
#!/bin/bash
# Search for dialog opening patterns with better context
echo "Checking dialog opening patterns..."
rg "EnlargeSlideImageComponent" -B 2 -A 3
# Check the interface usage with proper file type
echo "Verifying DialogData interface usage..."
rg "slideToReference.*:" -t ts
# Verify the component's template
echo "Checking component template..."
cat src/main/webapp/app/shared/metis/posting-content/enlarge-slide-image/enlarge-slide-image.component.html
Length of output: 5979
src/main/webapp/app/shared/metis/emoji/emoji.component.ts (1)
10-11: LGTM! Well-structured standalone component.
The component is correctly configured as standalone with proper imports, following Angular's recommended patterns for modular architecture.
src/main/webapp/app/shared/metis/posting-button/posting-button.component.ts (2)
4-4: LGTM: Import statement is correctly placed and necessary
The FaIconComponent import is properly added to support the standalone component configuration.
10-11: Verify template usage of FaIconComponent
Let's ensure the template properly uses the FaIconComponent with the defined icons.
✅ Verification successful
FaIconComponent is properly used in the template
The template correctly uses the fa-icon component with appropriate bindings:
- Conditional icon rendering with
buttonIconinput - Loading state with
faCircleNotchicon and spin animation - Proper usage of template reference variables (#icon, #loadingIcon)
- Correct structural and attribute directives ([hidden], @if)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check template usage of fa-icon component and icon bindings
rg -l "posting-button.component.html" | xargs cat 2>/dev/null | grep -A 5 "<fa-icon"
Length of output: 72
Script:
#!/bin/bash
# First, let's find the template file using fd
fd "posting-button.component.html"
# Then let's check the content of the template file
fd "posting-button.component.html" -x cat {}
Length of output: 431
src/main/webapp/app/shared/metis/conversations/one-to-one-chat.service.ts (2)
1-1: LGTM! Import statement follows Angular style guide.
The addition of the inject import follows Angular's recommended practices for organizing imports, with framework imports preceding feature imports.
10-11: Consider adding memory leak prevention for observables.
While the migration to inject function is good and aligns with modern Angular practices, ensure that any subscriptions to observables from this service are properly managed and cleaned up in the components using it.
Let's verify if the components using this service properly handle subscription cleanup:
src/main/webapp/app/shared/metis/posting-thread/posting-thread.component.ts (2)
4-4: LGTM: Import statement follows Angular style guide
The PostComponent import is correctly structured and follows proper path conventions.
11-12: Verify template usage with standalone component
The standalone configuration looks good, but we should verify the template usage.
✅ Verification successful
Template usage with standalone component is correctly configured
The template correctly uses the jhi-post selector with all necessary bindings (readOnlyMode, lastReadDate, posting, isConsecutive, showAnswers), and the component's imports array includes the corresponding PostComponent. No additional dependencies are required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify template usage and dependencies
# Check template for PostComponent usage and any missing imports
# Check template file for usage patterns
rg -A 5 '<jhi-post' "./src/main/webapp/app/shared/metis/posting-thread/posting-thread.component.html"
# Check for any other potential dependencies in template
ast-grep --pattern 'selector: "jhi-post"'
Length of output: 469
src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts (2)
5-6: LGTM! Import statements are well-organized.
The new imports are properly structured and necessary for the standalone component configuration.
11-12: Verify ArtemisSharedCommonModule usage in template.
The component is correctly configured as standalone. However, let's verify that ArtemisSharedCommonModule is actually needed in the template.
✅ Verification successful
ArtemisSharedCommonModule import is required for artemisTranslate pipe usage
The template uses the artemisTranslate pipe in multiple places (e.g., 'artemisApp.metis.searchEmoji' | artemisTranslate), and the code inspection shows that ArtemisTranslatePipe is declared and exported by ArtemisSharedCommonModule. Therefore, this module import is necessary for the component to function correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any directives/components from ArtemisSharedCommonModule are used in the template
# Get the contents of the template file
cat src/main/webapp/app/shared/metis/emoji/emoji-picker.component.html
# List exports from ArtemisSharedCommonModule to cross-reference
ast-grep --pattern 'export class ArtemisSharedCommonModule {
$$$
}'
Length of output: 823
Script:
#!/bin/bash
# Let's check the ArtemisSharedCommonModule definition and exports
rg -l "ArtemisSharedCommonModule" --type ts
# Then check for artemisTranslate pipe which seems to be used in the template
rg -l "artemisTranslate" --type ts
Length of output: 10547
Script:
#!/bin/bash
# Let's check the shared-common.module.ts to see what it exports
cat src/main/webapp/app/shared/shared-common.module.ts
# Also check if artemisTranslate pipe is defined in this module
rg "artemisTranslate" src/main/webapp/app/shared/shared-common.module.ts -A 5 -B 5
Length of output: 2117
src/main/webapp/app/shared/metis/posting-create-edit-modal/posting-create-edit-modal.directive.ts (2)
1-1: LGTM: Import statement follows Angular style guide
The imports are well-organized and the inject function is correctly imported from @angular/core.
Line range hint 1-35: Verify inheritance chain and usage pattern
Let's ensure this refactoring is consistent across the inheritance chain and related components.
✅ Verification successful
Inheritance chain and dependency injection pattern are consistent
The verification shows that:
- Both child components (
PostCreateEditModalComponentandAnswerPostCreateEditModalComponent) properly extendPostingCreateEditModalDirective - All components consistently use the new
inject()pattern for dependency injection - The base
PostingCreateEditDirectiveand all its derivatives follow the same pattern - No constructor-based dependency injection remains in the component hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for components extending this directive and verify consistent inject() usage
# Find components extending this directive
ast-grep --pattern 'class $_ extends PostingCreateEditModalDirective'
# Check for any remaining constructor-based DI in related files
rg -l "constructor.*MetisService" "src/main/webapp/app/shared/metis"
Length of output: 264
Script:
#!/bin/bash
# Let's try a different approach to find components extending this directive
# First, let's find all TypeScript files in the metis directory
fd "\.ts$" "src/main/webapp/app/shared/metis" --exec grep -l "PostingCreateEditModalDirective" {}
# Also check for any remaining constructor-based dependency injection pattern
fd "\.ts$" "src/main/webapp/app/shared/metis" --exec grep -l "constructor.*(" {}
# Let's also check the base directive implementation
fd "posting-create-edit.directive.ts$" "src/main/webapp/app/shared/metis" --exec cat {}
Length of output: 3208
Script:
#!/bin/bash
# Let's check the implementation of the child components to verify the inheritance pattern
cat "src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts"
cat "src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts"
Length of output: 13468
src/main/webapp/app/shared/pipes/html-for-posting-markdown.pipe.ts (2)
Line range hint 12-29: Excellent security practices and documentation!
The pipe demonstrates good practices:
- Comprehensive JSDoc documentation with clear parameter descriptions
- Proper HTML sanitization through the markdown service
- Type-safe implementation using SafeHtml
- Flexible configuration for allowed HTML tags and attributes
5-8: LGTM! Verify standalone pipe imports.
The migration to standalone pipe is correct and aligns with Angular's recommended architecture.
Let's verify that this pipe is properly imported where it's used:
✅ Verification successful
Standalone pipe migration is correctly implemented and imported
The verification shows that the standalone pipe migration is properly handled:
- The pipe is directly imported and used in
posting-content-part.componentvia itsimportsarray - All other imports are in test files which is expected
- The import in
metis.module.tscan be safely removed since the pipe is now imported directly where it's used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all components that use this pipe to ensure they import it correctly
# after the standalone migration
# Search for usage of the pipe in templates
rg -l "htmlForPostingMarkdown" --type html
# Search for imports of the pipe in component files
rg "import.*HtmlForPostingMarkdownPipe" --type ts
Length of output: 1099
src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.ts (3)
6-11: LGTM! Import statements follow Angular guidelines
The new imports are properly organized and necessary for the standalone component configuration.
Line range hint 20-37: Clean implementation of standalone component migration
The component successfully maintains its existing functionality while adopting the standalone architecture. The inheritance from PostingHeaderDirective and lifecycle implementations remain properly handled.
17-18: Verify component dependencies and consider optimization opportunities
While the standalone configuration is correct, consider the following optimizations:
- Verify that all imported modules are used in the template
- Consider granular imports from ArtemisSharedCommonModule instead of importing the entire module
✅ Verification successful
Let me gather more information about the ArtemisSharedCommonModule exports.
All imported dependencies are actively used, module import is justified
The verification shows that:
- All standalone imports are actively used in the template:
ProfilePictureComponent(jhi-profile-picture)NgClass([ngClass])FaIconComponent(fa-icon)NgbTooltip(ngbTooltip)TranslateDirective([jhiTranslate])
ArtemisSharedCommonModuleis necessary as it provides required pipes:artemisTranslatepipeartemisDatepipe
The current imports configuration is optimal and there's no need for granular imports since the used pipes are part of the shared module's core functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify template dependencies and module usage
# Check template file for actual usage of imported dependencies
echo "Checking template usage..."
rg -A 2 "ngClass|fa-icon|ngbTooltip|translate|jhi-profile-picture" "./src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html"
# Check what's exported from ArtemisSharedCommonModule
echo "Checking shared module exports..."
rg "exports:" "./src/main/webapp/app/shared/shared-common.module.ts"
Length of output: 1436
Script:
#!/bin/bash
# Get the full content of shared-common.module.ts to see its exports
echo "Checking ArtemisSharedCommonModule content..."
cat "./src/main/webapp/app/shared/shared-common.module.ts"
# Check if artemisDate and artemisTranslate are used in the template
echo "Checking for pipe usage..."
rg "artemisDate|artemisTranslate" "./src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html"
Length of output: 2757
src/main/webapp/app/shared/metis/reaction.service.ts (1)
Line range hint 1-11: LGTM! Modern dependency injection pattern adopted
The change to use the new inject function aligns with Angular's recommended patterns for dependency injection.
src/main/webapp/app/shared/metis/course-discussion.directive.ts (1)
1-1: LGTM! Import changes align with modern Angular practices.
The addition of the inject import from '@angular/core' is correct and follows Angular's recommended approach for dependency injection.
src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.ts (3)
8-13: LGTM: Import statements are well-organized
The new imports are correctly structured and necessary for the standalone component migration.
Line range hint 1-53: Verify coding guidelines compliance
Let's ensure the code follows all specified guidelines for TypeScript files.
✅ Verification successful
Let me generate additional verification scripts to check the specific file in question, as the previous results were too broad.
Based on the verification results from both script runs, I can now generate the final response:
Code follows TypeScript guidelines
The code in post-header.component.ts adheres to the TypeScript coding guidelines:
- Uses PascalCase for class names (PostHeaderComponent)
- Uses camelCase for properties and methods
- Uses single quotes consistently for string literals
- Follows Angular component naming conventions
- Properly extends and implements interfaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify coding guidelines compliance
# Check for proper casing in types and enums
echo "Checking PascalCase for types:"
ast-grep --pattern 'class $TYPE {$$$}' | grep -v '^[A-Z][a-zA-Z]*[a-zA-Z0-9]*$'
# Check for proper casing in methods and properties
echo "\nChecking camelCase for methods and properties:"
ast-grep --pattern 'class {$$$public $PROP$$$}' | grep -v '^[a-z][a-zA-Z]*[a-zA-Z0-9]*$'
# Check for single quotes in strings
echo "\nChecking for double quotes usage:"
rg '"[^"]*"' --type ts
# Check for traditional function declarations
echo "\nChecking for traditional function declarations:"
rg "function [a-zA-Z]+" --type ts
Length of output: 111678
Script:
#!/bin/bash
# Check specific file for coding guidelines
# Check for proper casing in types and methods
echo "Checking casing in post-header.component.ts:"
ast-grep --pattern 'class $TYPE {$$$}' src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.ts
# Check for string quotes usage
echo "\nChecking string quotes:"
rg "['\"].*['\"]" src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.ts
# Check for method declarations
echo "\nChecking method declarations:"
ast-grep --pattern 'class {$$$method $NAME($$$) {$$$}}' src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.ts
Length of output: 1730
19-20: Verify template usage of imported dependencies
The standalone configuration looks correct. However, let's verify that all imported components and directives are actually used in the template.
✅ Verification successful
All imported components and directives are used in the template
Based on the template content verification:
ProfilePictureComponent: Used with<jhi-profile-picture>tagNgClass: Used with[ngClass]bindingFaIconComponent: Used with<fa-icon>tagNgbTooltip: Used with[ngbTooltip]bindingTranslateDirective: Used indirectly through the| artemisTranslatepipeArtemisSharedCommonModule: Contains common dependencies used in the template
The standalone configuration is correct and all imported dependencies are actively used in the template.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check template usage of imported components and directives
# Get the template content
echo "Checking template usage in post-header.component.html:"
rg -l "post-header.component.html" | xargs cat
# Check for specific component/directive usage
echo "\nChecking for ProfilePictureComponent usage:"
rg "jhi-profile-picture" -A 2
echo "\nChecking for NgClass usage:"
rg "\\[ngClass\\]" -A 2
echo "\nChecking for FaIconComponent usage:"
rg "fa-icon" -A 2
echo "\nChecking for NgbTooltip usage:"
rg "\\[ngbTooltip\\]" -A 2
echo "\nChecking for TranslateDirective usage:"
rg "\\[translate\\]" -A 2
Length of output: 146128
Script:
#!/bin/bash
# Let's check the specific template file for usage of imported components/directives
echo "Checking post-header.component.html content:"
rg -l "post-header.component.html" | xargs cat
echo "\nChecking for specific usages in post-header.component.html:"
rg -g "post-header.component.html" "jhi-profile-picture|\\[ngClass\\]|fa-icon|\\[ngbTooltip\\]|\\[translate\\]"
Length of output: 5081
src/main/webapp/app/shared/metis/conversations/group-chat.service.ts (2)
6-6: LGTM! Import changes align with new DI pattern
The addition of inject import is correct and follows Angular's new dependency injection pattern.
Line range hint 1-42: Overall implementation follows Angular guidelines
The service implementation adheres to Angular's style guide and best practices:
- Proper use of the new inject() pattern
- Consistent naming conventions
- Type safety throughout
- Proper HTTP response handling
- Follows single responsibility principle
src/main/webapp/app/shared/metis/answer-post.service.ts (2)
1-1: LGTM: Import statement correctly includes inject.
The import statement properly includes the inject function from @angular/core, which is required for the new dependency injection pattern.
12-12: LGTM: Properly implemented new inject pattern.
The change from constructor-based DI to the inject function follows Angular's modern best practices. The protected access modifier is appropriate for inheritance.
Let's verify that this pattern is consistently applied across related services:
✅ Verification successful
LGTM: The inject pattern is consistently applied across related services
From the search results, we can see that all TypeScript services extending PostingService (post.service.ts, answer-post.service.ts) are already using the new inject pattern with protected http = inject(HttpClient). This confirms that the pattern is consistently applied across the related services in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other services that might need similar updates
ast-grep --pattern 'class $SERVICE_NAME extends PostingService {
constructor($$$) {
$$$
}
}'
Length of output: 226
Script:
#!/bin/bash
# Let's try a different approach to find services extending PostingService
rg "extends PostingService" -l
# Then let's check the current state of dependency injection in these files
rg "constructor.*HttpClient|protected http = inject\(HttpClient\)" -A 2
Length of output: 21704
src/main/webapp/app/shared/metis/posting-create-edit.directive.ts (2)
1-1: LGTM! Import changes align with Angular guidelines.
The addition of the inject import is properly organized with related Angular core imports.
Line range hint 23-24: Verify cleanup of modalRef and EventEmitters.
To prevent memory leaks, ensure that:
- The
modalRefis properly closed in component destruction - EventEmitter subscriptions are cleaned up
✅ Verification successful
Let me gather more information about the cleanup patterns in these components.
Modal and subscription cleanup is properly implemented across the codebase
The verification shows that modal and subscription cleanup is well-handled throughout the codebase:
- Child components like
post-header.component.tsandpost-reactions-bar.component.tsproperly close their modals inngOnDestroy() - All components with subscriptions (e.g.,
posting-content.components.ts,post-tag-selector.component.ts) properly unsubscribe in their cleanup - The base services (
metis.service.ts,metis-conversation.service.ts) have comprehensive cleanup of subscriptions and websocket connections
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cleanup patterns in related components
ast-grep --pattern 'ngOnDestroy() {
$$$
this.modalRef?.close();
$$$
}'
# Check for potential memory leaks in child components
rg -A 5 "extends PostingCreateEditDirective"
Length of output: 2963
Script:
#!/bin/bash
# Check for ngOnDestroy implementations in the base class and child components
rg -A 10 "ngOnDestroy\(\)" src/main/webapp/app/shared/metis/
# Check for subscription cleanup patterns
rg -A 3 "subscription|unsubscribe" src/main/webapp/app/shared/metis/
Length of output: 14517
src/main/webapp/app/shared/link-preview/components/link-preview/link-preview.component.ts (1)
7-9: LGTM! Proper standalone component configuration
The component has been correctly configured as standalone with appropriate module imports.
Also applies to: 15-16
src/main/webapp/app/shared/pipes/reacting-users-on-posting.pipe.ts (1)
13-13: LGTM: Standalone pipe migration
The migration to a standalone pipe aligns with Angular's modern architecture practices.
src/main/webapp/app/shared/link-preview/components/link-preview-container/link-preview-container.component.ts (2)
Line range hint 16-31: Well-structured state management using signals!
The component demonstrates excellent use of modern Angular patterns:
- Clean dependency injection using
inject() - Type-safe input properties using the new
input()signal syntax - Clear state management using signals for reactive updates
Line range hint 79-81: LGTM! Proper track by function implementation.
The trackLinks function correctly uses a unique identifier (URL) for tracking, which helps Angular's change detection optimize rendering performance.
src/main/webapp/app/shared/metis/message/message-inline-input/message-inline-input.component.ts (4)
1-13: LGTM! Imports are well-organized and complete.
The imports follow the Angular style guide and include all necessary dependencies for the standalone component.
24-29: LGTM! Modern dependency injection pattern implemented correctly.
The migration to using the inject() function improves code clarity and follows current Angular best practices.
15-22: Consider reviewing the ViewEncapsulation strategy.
While the standalone configuration is correct, using ViewEncapsulation.None might lead to style leaks. Consider using Emulated or ShadowDom unless global style exposure is specifically required.
Line range hint 1-85: Verify localization of user-facing strings.
Ensure all user-facing strings (including error messages) are properly localized using the translation system.
✅ Verification successful
All user-facing strings are properly localized
The template file shows that all user-facing strings are correctly using translation directives:
- Warning message sections use
jhiTranslatedirective:artemisApp.messageWarning.headerTextartemisApp.messageWarning.mainParagraphartemisApp.messageWarning.lastParagraph
- Buttons use
artemisTranslatepipe:artemisApp.metis.cancelartemisApp.conversationsLayout.sendMessage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for hardcoded strings in the template file
rg -l "\'[^']+\'" "src/main/webapp/app/shared/metis/message/message-inline-input/message-inline-input.component.html"
# Check if translation keys exist
rg -l "translate" "src/main/webapp/app/shared/metis/message/message-inline-input/message-inline-input.component.html"
Length of output: 318
Script:
#!/bin/bash
# Check for hardcoded strings in the template file
rg -l "['\"][^'\"]+['\"]" "src/main/webapp/app/shared/metis/message/message-inline-input/message-inline-input.component.html"
# Check template content to analyze translation usage
cat "src/main/webapp/app/shared/metis/message/message-inline-input/message-inline-input.component.html"
# Check for translation keys in i18n files that might be used by this component
rg -g "*.json" "message-inline-input" "src/main/webapp/i18n/"
Length of output: 2170
src/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.ts (3)
23-27: LGTM! Clean implementation of modern DI pattern.
The migration to the new inject() pattern is well implemented, maintaining proper encapsulation with protected services.
1-21: LGTM! Verify imported module usage.
The conversion to a standalone component and the import structure look good and follow Angular best practices.
Let's verify that all imported modules are being used in the template:
✅ Verification successful
Based on the shell script results, I can now generate the final response as all imported modules are being actively used in the template:
All imported modules are properly utilized
The verification confirms that all imported modules are actively used in the template:
FormsModule/ReactiveFormsModule: Used via[formGroup]andformControlNamePostingMarkdownEditorComponent: Used via<jhi-posting-markdown-editor>TranslateDirective: Used viajhiTranslatedirectivesPostingButtonComponent: Used via<jhi-posting-button>ArtemisSharedCommonModule: Contains common dependencies used in the template
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all imported modules are being used in the template
# Get the template content
rg -l "message-reply-inline-input.component.html" | xargs cat
# Verify specific directive/component usage
rg -A 2 "formGroup|ngModel|jhiTranslate|jhi-posting-markdown-editor|jhi-posting-button" "src/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.html"
Length of output: 5872
Line range hint 1-108: Verify translations for user-facing strings.
The PR objectives require translations in English and German. Let's ensure all user-facing strings are properly translated.
✅ Verification successful
All user-facing strings are properly translated
The template uses proper translation directives (jhiTranslate) for all text content:
- Warning header:
artemisApp.messageWarning.headerText - Main warning paragraph:
artemisApp.messageWarning.mainParagraph - Last warning paragraph:
artemisApp.messageWarning.lastParagraph - Cancel button:
artemisApp.metis.cancel - Send button:
artemisApp.conversationsLayout.sendMessage
All these translation keys are present in both English and German translation files with appropriate translations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for untranslated strings in template and translation files
# Check template for jhiTranslate usage
rg "'>.*?<'" "src/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.html"
# Check translation files for corresponding keys
rg -l "i18n" | grep -E "en|de.json" | xargs rg "message-reply-inline-input"
Length of output: 204
Script:
#!/bin/bash
# Let's first check if the template file exists and see its content
fd "message-reply-inline-input.component.html" --type f
# If found, let's examine its content for any hardcoded text
if [ -f "src/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.html" ]; then
cat "src/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.html"
fi
# Let's also check for translation files in the i18n directory
fd "i18n" --type d
# Check for any translation keys containing "metis" or "message" in translation files
if [ -d "src/main/webapp/i18n" ]; then
rg "metis|message" "src/main/webapp/i18n/en/*.json" "src/main/webapp/i18n/de/*.json" || true
fi
Length of output: 2655
Script:
#!/bin/bash
# Let's check the actual translation files in the i18n directories
fd -e json . "src/main/webapp/i18n"
# If we find translation files, let's check their content for the specific keys used in the template
if [ -d "src/main/webapp/i18n" ]; then
# Looking for the specific translation keys used in the template
rg -l "artemisApp.messageWarning.headerText|artemisApp.messageWarning.mainParagraph|artemisApp.messageWarning.lastParagraph|artemisApp.metis.cancel|artemisApp.conversationsLayout.sendMessage" src/main/webapp/i18n/
# If we find any files, let's see the actual translations
echo "=== Translation content ==="
rg "artemisApp.messageWarning|artemisApp.metis.cancel|artemisApp.conversationsLayout.sendMessage" src/main/webapp/i18n/
fi
Length of output: 14086
Script:
#!/bin/bash
# Let's check the specific files that might contain our translation keys
cat src/main/webapp/i18n/en/metis.json src/main/webapp/i18n/de/metis.json || true
cat src/main/webapp/i18n/en/conversation.json src/main/webapp/i18n/de/conversation.json || true
cat src/main/webapp/i18n/en/global.json src/main/webapp/i18n/de/global.json || true
# Also check if there's a dedicated message warning translations file
fd "messageWarning.json" --type f
Length of output: 81036
src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts (2)
26-28: LGTM! Modern dependency injection implementation.
The migration to the inject() function follows Angular's modern best practices and reduces boilerplate code while maintaining proper encapsulation with the protected modifier.
Line range hint 93-103: Verify localization of all user-facing strings.
The modal titles use translation keys, but let's verify that all user-facing strings are properly localized.
✅ Verification successful
Based on the results from our verification scripts, I can now provide a final response:
All user-facing strings are properly localized in this component
- The template file shows all text content is using translation pipes (
artemisTranslate) - The component uses translation keys for modal titles (
artemisApp.metis.editPostingandartemisApp.metis.createModalTitleAnswer) - No hardcoded user-facing strings were found in either the template or component files
- Button labels use translation keys (
artemisApp.metis.cancelandartemisApp.metis.savePosting)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for non-localized strings in the component and related files
# Look for hardcoded strings in the component and template files
echo "Checking for potential non-localized strings..."
rg -t ts -t html "['\"]((?!artemisApp\.|assets/|app/)[^'\"]*)['\"]\s*$" src/main/webapp/app/shared/metis/posting-create-edit-modal/
# Check if translation keys exist in translation files
echo "Verifying translation keys..."
rg "artemisApp\.metis\.(editPosting|createModalTitleAnswer)" src/main/webapp/i18n/
Length of output: 656
Script:
#!/bin/bash
# Let's try a simpler approach to check for non-localized strings and translation keys
# Check the component file for string literals
echo "Checking component file for potential non-localized strings..."
rg -t ts "'[^']*'" src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts
# Check the template file for text content
echo "Checking template file for text content..."
rg -t html "<[^>]*>[^<]*[a-zA-Z]+[^<]*</[^>]*>" src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.html
# Check for translation keys in the component
echo "Checking for translation keys in component..."
rg "artemisApp\.metis\." src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts
# Check translation files for these keys
echo "Checking translation files..."
fd -t f . src/main/webapp/i18n/ -x grep -l "artemisApp.metis.editPosting\|artemisApp.metis.createModalTitleAnswer" {}
Length of output: 2634
Script:
#!/bin/bash
# Let's check the template file and translation files more thoroughly
# Check the template file content directly
echo "Checking template file content..."
cat src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.html
# Check if the translation keys exist in the i18n files
echo "Checking English translations..."
rg "artemisApp\.metis\.(editPosting|createModalTitleAnswer)" src/main/webapp/i18n/en/
echo "Checking German translations..."
rg "artemisApp\.metis\.(editPosting|createModalTitleAnswer)" src/main/webapp/i18n/de/
Length of output: 1622
src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-tag-selector/post-tag-selector.component.ts (1)
37-38: LGTM! Proper usage of the new inject() pattern.
The migration from constructor-based DI to the new inject() pattern is well implemented and follows Angular's modern best practices.
src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts (3)
1-1: LGTM! Well-organized imports following Angular guidelines.
The imports are properly organized and include all necessary dependencies for the standalone component configuration.
Also applies to: 8-15
58-58: LGTM! Modern dependency injection approach.
The use of inject() function for dependency injection aligns with modern Angular practices and reduces constructor boilerplate while maintaining proper encapsulation.
Line range hint 1-58: Verify test coverage for the standalone component.
As per coding guidelines, meaningful tests are required. Let's verify the existence and coverage of tests for this component.
✅ Verification successful
Test coverage is comprehensive and meets coding guidelines
The component has thorough test coverage including:
- Reaction creation and deletion functionality
- Edit/delete permission checks for different user roles
- Event emission for edit modal
- Answer visibility logic
- Resolution toggle functionality
- Component initialization and state management
The test file demonstrates:
- Proper test setup with mocked dependencies
- Clear test descriptions
- Coverage of component's core functionality
- Edge case handling
- User permission scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the corresponding test file
fd -e spec.ts "answer-post-reactions-bar.component.spec.ts"
# Search for test coverage of critical methods
rg -A 5 "describe.*AnswerPostReactionsBarComponent" -g "*.spec.ts"
Length of output: 1391
Script:
#!/bin/bash
# Let's examine the test file content to verify test coverage
cat "src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts"
Length of output: 11601
src/main/webapp/app/shared/metis/conversations/channel.service.ts (3)
Line range hint 1-10: LGTM! Clean transition to modern DI pattern
The addition of the inject import aligns with Angular's recommended approach for dependency injection.
11-13: LGTM! Clean dependency injection implementation
The transition to using inject() follows Angular's modern patterns and improves code clarity by removing constructor boilerplate.
15-15: Consider making resourceUrl private and verify subscription handling
Two suggestions for improvement:
- Since
resourceUrlis only used internally within the service, consider making it private for better encapsulation. - Ensure all HTTP subscriptions in components using this service are properly managed to prevent memory leaks.
Let's verify the subscription handling in components using this service:
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (1)
32-33: LGTM! Good migration to standalone component.
The component is properly configured as standalone with all necessary dependencies imported.
src/main/webapp/app/shared/metis/post.service.ts (2)
1-1: LGTM: Clean import addition for new DI pattern
The addition of the inject import aligns with Angular's modern dependency injection pattern.
16-16: LGTM: Proper implementation of inject pattern
The HTTP client injection is correctly implemented using the new pattern.
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.directive.ts (2)
1-1: LGTM: Import changes align with Angular's new DI pattern
The addition of inject and output imports from '@angular/core' properly supports the transition to the new dependency injection pattern.
47-48: Verify derived class compatibility with new injection pattern
The transition from constructor injection to property injection looks good, but we should verify that all derived classes are compatible with this change.
✅ Verification successful
Let me search for more specific information about derived classes and their usage patterns.
Let me check the derived components specifically for any constructor usage of metisService.
Change to property injection is safe and compatible
The derived components (PostReactionsBarComponent and AnswerPostReactionsBarComponent) are already using property injection for MetisService and don't rely on constructor injection. Both components properly extend PostingsReactionsBarDirective and are compatible with the base class's injection pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all classes that extend PostingsReactionsBarDirective to verify compatibility
# Search for classes extending PostingsReactionsBarDirective
ast-grep --pattern 'class $_ extends PostingsReactionsBarDirective'
# Search for potential constructor references to metisService in derived classes
rg -l "constructor.*metisService" src/main/webapp/app/
Length of output: 438
Script:
#!/bin/bash
# Search for files that import PostingsReactionsBarDirective
rg -l "PostingsReactionsBarDirective" src/main/webapp/app/
# Get the content of these files to understand the usage
rg -A 5 "PostingsReactionsBarDirective" src/main/webapp/app/
Length of output: 5403
Script:
#!/bin/bash
# Get full content of the derived components to check their constructors
cat src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts
cat src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts
Length of output: 14854
src/main/webapp/app/shared/metis/post/post.component.ts (1)
79-85: Verify dependency injection completeness across component usage.
The migration to inject() function is well implemented. However, let's verify that all dependencies are properly used throughout the component.
✅ Verification successful
All injected dependencies are properly utilized in the component
The verification shows that all injected dependencies are actively used within the component:
metisService: Used for page type, context, course info, and post-related operationschangeDetector: Used for change detection in dropdown and componentoneToOneChatService: Used for chat creationmetisConversationService: Used for conversation managementrouter: Used for navigationrenderer: Used for DOM manipulation of overflow stylesdocument: Implicitly used with renderer (as it's a required dependency for DOM operations)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of injected services to ensure they're properly utilized
rg "metisService\." src/main/webapp/app/shared/metis/post/post.component.ts
rg "changeDetector\." src/main/webapp/app/shared/metis/post/post.component.ts
rg "oneToOneChatService\." src/main/webapp/app/shared/metis/post/post.component.ts
rg "metisConversationService\." src/main/webapp/app/shared/metis/post/post.component.ts
rg "router\." src/main/webapp/app/shared/metis/post/post.component.ts
rg "renderer\." src/main/webapp/app/shared/metis/post/post.component.ts
Length of output: 1890
src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (1)
Line range hint 1-15: Consider reviewing ArtemisSharedModule imports
Since the component is now standalone, consider reviewing the ArtemisSharedModule import to ensure only necessary dependencies are included. This can help reduce the bundle size.
Also applies to: 22-23
src/main/webapp/app/shared/metis/metis-conversation.service.ts (1)
29-37: LGTM! Clean migration to inject()
The migration from constructor injection to the new inject() function is well implemented. The access modifiers and organization are maintained correctly.
src/main/webapp/app/shared/metis/metis.service.ts (1)
39-46: LGTM! Clean migration to modern dependency injection.
The migration to Angular's inject() function follows best practices for dependency injection in standalone components.
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (2)
45-59: Verify Standalone Component Imports
Ensure that all dependencies required by this component are correctly included in the imports array. Double-check for any missing or unnecessary imports to prevent runtime errors.
14-14:
Use @Input() Decorator Instead of input Function
The input function is not a standard Angular approach for defining input properties. Instead, use the @Input() decorator to maintain consistency with Angular's best practices.
Apply this change:
- import {
- ChangeDetectionStrategy,
- ChangeDetectorRef,
- Component,
- EventEmitter,
- HostListener,
- Input,
- OnChanges,
- OnInit,
- Output,
- Renderer2,
- ViewChild,
- ViewContainerRef,
- inject,
- input,
- } from '@angular/core';
+ import {
+ ChangeDetectionStrategy,
+ ChangeDetectorRef,
+ Component,
+ EventEmitter,
+ HostListener,
+ Input,
+ OnChanges,
+ OnInit,
+ Output,
+ Renderer2,
+ ViewChild,
+ ViewContainerRef,
+ inject,
+ } from '@angular/core';And update the property definition:
- isConsecutive = input<boolean>(false);
+ @Input()
+ isConsecutive = false;Likely invalid or redundant comment.
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (3)
14-14: Importing 'inject' is appropriate for dependency injection in standalone components
The addition of inject to the imports aligns with Angular's best practices for standalone components.
36-37: Conversion to a standalone component is correctly implemented
The addition of standalone: true and the imports array correctly converts the component to a standalone component, enhancing modularity.
40-41: Use of 'inject' function for dependency injection is appropriate
The transition from constructor-based injection to using inject enhances code conciseness and leverages Angular's updated dependency injection mechanism.
src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts (6)
1-1: Approved: Importing 'inject' from '@angular/core'
The addition of inject in the import statement is appropriate for using Angular's dependency injection with the inject function.
3-13: Approved: Importing required NgBootstrap components
The imported NgBootstrap components are necessary for the accordion functionality in the modal. This enhances the user interface by incorporating accordion features.
16-16: Approved: Importing Form modules and Validators
Including FormsModule, ReactiveFormsModule, and Validators is essential for form handling and validation within the component.
25-25: Approved: Importing 'TranslateDirective' for localization
The TranslateDirective import ensures that the component supports localization, aligning with internationalization best practices.
33-38: Approved: Importing shared modules and components
The imports of shared modules and components like ArtemisSharedComponentModule, PostTagSelectorComponent, PostingMarkdownEditorComponent, and PostingButtonComponent promote code reuse and maintainability.
43-61: Approved: Declaring the component as standalone and updating imports
Setting standalone: true and defining the imports array correctly transforms the component into a standalone component. This change enhances modularity and simplifies dependency management.
src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts (1)
1-1:
Incorrect import of 'output' from '@angular/core'
The output function does not exist in '@angular/core'. To define an Output property, use the @Output decorator from '@angular/core'.
Apply the following diff to fix the import statement:
- import { Component, EventEmitter, Input, OnChanges, OnDestroy, OnInit, Output, ViewChild, inject, output } from '@angular/core';
+ import { Component, EventEmitter, Input, OnChanges, OnDestroy, OnInit, Output, ViewChild, inject } from '@angular/core';Additionally, verify all instances where output is used in the codebase to ensure they are correctly replaced with the @Output decorator.
Run the following script to locate usages of output:
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (2)
61-62: Component successfully converted to a standalone component
The component has been correctly transformed into a standalone component with the appropriate standalone: true property and the necessary imports.
65-74: Verify compatibility with Angular version
The use of the inject and input functions, as well as standalone components, requires Angular 14 or higher (with some features introduced in Angular 16). Please ensure that the project is using a compatible Angular version to support these features.
Run the following script to verify the Angular version used in the project:
✅ Verification successful
Angular version is compatible with the code features
The project uses Angular 18.2.12, which fully supports the inject function, input decorators, and standalone components. These features were introduced in Angular 14 and are stable in Angular 18.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Retrieve the Angular version used in the project.
# Search for '@angular/core' version in package.json
rg '"@angular/core":' $(fd package.json)
Length of output: 586
...webapp/app/shared/metis/posting-content/enlarge-slide-image/enlarge-slide-image.component.ts
Show resolved
Hide resolved
...app/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts
Show resolved
Hide resolved
src/main/webapp/app/overview/discussion-section/discussion-section.component.ts
Show resolved
Hide resolved
src/main/webapp/app/overview/discussion-section/discussion-section.component.ts
Show resolved
Hide resolved
...d/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
src/test/javascript/spec/component/shared/metis/message-inline-input-component/message-inline-input.component.spec.ts (1)
Line range hint
1-120: Consider adding more test coverage.While the current test suite covers the basic CRUD operations and error handling well, consider adding tests for:
- Form validation scenarios (empty content, max length)
- Content formatting edge cases
- Boundary conditions for loading states
Example test case to add:
it('should validate empty content', fakeAsync(() => { component.posting = metisPostToCreateUser1; component.ngOnChanges(); component.formGroup.setValue({ content: '' }); component.confirm(); expect(metisServiceCreateStub).not.toHaveBeenCalled(); expect(component.formGroup.get('content')?.errors).toContainEntries([ ['required', true] ]); }));src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (3)
Line range hint
321-365: Consider extracting simulateListAction helper to improve test maintainability.The simulateListAction helper function is well-implemented but could be moved to a separate test utilities file to promote reusability and maintain single responsibility principle.
Line range hint
366-458: Consider adding error scenario test cases.While the happy path test cases are comprehensive, consider adding test cases for error scenarios such as:
- Invalid list formats
- Empty selections
- Mixed list types in selection
Line range hint
81-117: Consider extracting mock setup to reduce duplication.The mock setup for PointerEvent and mockEmojiSelect could be moved to a beforeAll block or a separate setup function to reduce code duplication.
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts (1)
Missing edge case tests needed for ReactingUsersOnPostingPipe
Based on the implementation, the pipe doesn't handle empty arrays or arrays with null/undefined values. Adding these test cases is important because:
- Empty arrays could occur when no users have reacted
- Null/undefined values in the array could occur due to API responses or data inconsistencies
- The implementation directly uses array methods without defensive checks
Specific locations needing tests:
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts:
- Add test for empty array input
- Add test for array containing null/undefined values
- Add test for null/undefined input
🔗 Analysis chain
Line range hint
1-120: Verify edge cases in test coverage.While the existing test cases are comprehensive, consider adding tests for these scenarios:
- Empty array input
- Array with null/undefined user names
- Maximum number of users (boundary testing)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar test files to compare test coverage patterns rg -l "transform.*subscribe.*tick" "src/test/javascript/spec/**/*pipe.spec.ts" | while read -r file; do echo "=== $file ===" rg "it\(.*null.*undefined" "$file" || echo "No null/undefined tests found" rg "it\(.*empty" "$file" || echo "No empty input tests found" doneLength of output: 177
Script:
#!/bin/bash # Let's try a different approach to find the test files and analyze their content fd "pipe.spec.ts" "src/test/javascript/spec" | while read -r file; do echo "=== $file ===" rg "it\(.*null.*undefined" "$file" || echo "No null/undefined tests found" rg "it\(.*empty" "$file" || echo "No empty input tests found" done # Also let's check the implementation file to understand the transform method ast-grep --pattern 'transform($$$) { $$$ }'Length of output: 5244
Script:
#!/bin/bash # Let's examine the implementation of ReactingUsersOnPostingPipe to understand how it handles edge cases rg -A 10 "class ReactingUsersOnPostingPipe" # Also check if there are any existing null/undefined checks in the implementation rg "(!|undefined|null)" "src/app/shared/pipes/reacting-users-on-posting.pipe.ts"Length of output: 1603
Script:
#!/bin/bash # Let's check the implementation file with the correct path rg -A 15 "class ReactingUsersOnPostingPipe" "src/main/webapp/app/shared/pipes/reacting-users-on-posting.pipe.ts" # Also check how updateReactingUsersString handles the array rg -A 20 "updateReactingUsersString" "src/main/webapp/app/shared/pipes/reacting-users-on-posting.pipe.ts" # Check if there are any array length checks rg "length|\.length" "src/main/webapp/app/shared/pipes/reacting-users-on-posting.pipe.ts"Length of output: 3278
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-tag-selector.component.spec.ts (1)
Line range hint
46-46: Critical: Implement missing test cases for tag managementThe TODO comment indicates missing test coverage for core functionality:
- Adding categories
- Removing categories
This gap in test coverage could lead to undetected issues in tag management functionality.
Would you like me to help implement these test cases? Here's a suggested structure:
it('should add a tag when selected', fakeAsync(() => { const newTag = metisTags[0]; component.addTag(newTag); tick(); expect(component.postTags).toContainEqual(newTag); })); it('should remove a tag when requested', fakeAsync(() => { const tagToRemove = metisTags[0]; component.postTags = [tagToRemove]; component.removeTag(tagToRemove); tick(); expect(component.postTags).not.toContainEqual(tagToRemove); }));src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.spec.ts (1)
Line range hint
35-50: Consider adding tests for form validation and error handlingWhile the current test coverage is good, consider adding tests for:
- Form validation scenarios (invalid title, empty content)
- Error handling during post creation/update
- Edge cases in context selection
Example test cases:
it('should validate required fields', () => { component.formGroup.setValue({ title: '', content: '', context: {} }); expect(component.formGroup.valid).toBeFalse(); expect(component.formGroup.get('title')?.errors?.['required']).toBeTrue(); expect(component.formGroup.get('content')?.errors?.['required']).toBeTrue(); }); it('should handle errors during post creation', fakeAsync(() => { metisServiceCreateStub.mockRejectedValue(new Error('Network error')); component.formGroup.setValue({ title: 'Test', content: 'Content', context: {} }); component.confirm(); tick(); expect(component.isLoading).toBeFalse(); expect(component.error).toBeTruthy(); }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
src/test/javascript/spec/component/link-preview/link-preview.component.spec.ts(2 hunks)src/test/javascript/spec/component/overview/discussion-section/discussion-section.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/message-inline-input-component/message-inline-input.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/message-reply-inline-input/message-reply-inline-input.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts(3 hunks)src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-tag-selector.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts(4 hunks)src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts(2 hunks)src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-tag-selector.component.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}}
src/test/javascript/spec/component/shared/metis/message-reply-inline-input/message-reply-inline-input.component.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}}
src/test/javascript/spec/component/shared/metis/message-inline-input-component/message-inline-input.component.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}}
src/test/javascript/spec/component/overview/discussion-section/discussion-section.component.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}}
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.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}}
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.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}}
src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.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}}
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.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}}
src/test/javascript/spec/component/link-preview/link-preview.component.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}}
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.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 (19)
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts (2)
33-35: LGTM: New imports align with standalone component migration
The added imports for ReactiveFormsModule, ArtemisSharedCommonModule, and ArtemisConfirmIconModule are appropriate for the component's testing requirements and align with the PR's objective of migrating to standalone components.
52-59: LGTM: TestBed configuration follows best practices
The TestBed configuration:
- Properly mocks all modules using NgMocks
- Maintains test isolation
- Follows the coding guideline to avoid full module imports
Also applies to: 61-61
src/test/javascript/spec/component/shared/metis/message-reply-inline-input/message-reply-inline-input.component.spec.ts (3)
14-14: LGTM!
Clean import statement for the shared module.
25-25: LGTM! Good use of MockModule
Excellent practice using MockModule from ng-mocks to mock the ArtemisSharedCommonModule. This follows the best practice of avoiding full module imports in tests and helps isolate the component under test.
Line range hint 16-115: Test implementation follows best practices
The test suite demonstrates excellent testing practices:
- Proper use of jest expectations and spies
- Comprehensive coverage of success and error scenarios
- Correct async testing with fakeAsync/tick
- Good isolation of the component under test with proper mocking
src/test/javascript/spec/component/shared/metis/message-inline-input-component/message-inline-input.component.spec.ts (1)
14-14: LGTM! Proper module mocking setup.
The ArtemisSharedCommonModule is correctly imported and mocked using MockModule, following best practices for Angular component testing.
Also applies to: 25-25
src/test/javascript/spec/component/overview/discussion-section/discussion-section.component.spec.ts (2)
49-53: LGTM! Imports are well-organized.
The new imports for shared modules are correctly added and follow the module organization pattern.
75-86: LGTM! Test configuration is properly set up.
The TestBed configuration correctly uses MockModule for all imported modules, which is the recommended approach for isolating component tests.
src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts (3)
3-3: LGTM! Import statement is correctly updated.
The import statement properly includes MockModule along with other ng-mocks utilities.
17-18: LGTM! Shared module imports are properly added.
The imports for ArtemisSharedCommonModule and ArtemisSharedModule are correctly defined.
27-27: LGTM! TestBed configuration is properly updated.
The imports array correctly uses MockModule for the shared modules, following testing best practices.
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (2)
4-4: LGTM! Import changes follow best practices.
The changes improve the test isolation by using MockModule and maintain consistent import paths.
Also applies to: 26-27, 37-39
119-119: LGTM! TestBed configuration properly mocks dependencies.
Using MockModule for ArtemisMarkdownEditorModule improves test isolation.
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts (1)
15-15: LGTM! Empty declarations array aligns with standalone pipe architecture.
The removal of HtmlForPostingMarkdownPipe from declarations is correct since the pipe under test is now standalone.
src/test/javascript/spec/component/link-preview/link-preview.component.spec.ts (3)
6-6: LGTM: Import statements are properly structured.
The new imports for MockModule and the required Artemis modules are correctly added and follow the established pattern.
Also applies to: 14-15
Line range hint 43-176: LGTM: Test cases follow best practices.
The test suite demonstrates good practices:
- Proper use of jest matchers (toBeTrue, toHaveBeenCalled)
- Comprehensive coverage of component behavior
- Clear and specific expectations
24-30: 🛠️ Refactor suggestion
Refactor TestBed configuration for better test isolation.
The current configuration has several issues:
- Mock modules should be in the
importsarray, notdeclarations - Mixing individual component mocks with module mocks may lead to unexpected behavior
Apply this diff to fix the configuration:
TestBed.configureTestingModule({
declarations: [
LinkPreviewComponent,
MockPipe(ArtemisTranslatePipe),
- MockComponent(ConfirmIconComponent),
- MockModule(ArtemisConfirmIconModule),
- MockModule(ArtemisSharedCommonModule),
+ MockComponent(ConfirmIconComponent)
],
+ imports: [
+ MockModule(ArtemisConfirmIconModule),
+ MockModule(ArtemisSharedCommonModule)
+ ],
providers: [Let's verify if these modules are actually required:
✅ Verification successful
The shell script results confirm that both ArtemisConfirmIconModule and ArtemisSharedCommonModule are actually imported and used in the component's imports array. Let me verify the template usage with one more check:
TestBed configuration needs to be fixed, but modules are correctly required
The review comment was partially correct. The TestBed configuration does need to be fixed by moving mock modules to the imports array, but both modules are actually required by the component:
ArtemisConfirmIconModuleis needed as the template uses<jhi-confirm-icon>componentArtemisSharedCommonModuleis needed as the component usesNgClassand translation pipes
Apply this fix to the TestBed configuration:
TestBed.configureTestingModule({
declarations: [
LinkPreviewComponent,
MockPipe(ArtemisTranslatePipe),
- MockComponent(ConfirmIconComponent),
- MockModule(ArtemisConfirmIconModule),
- MockModule(ArtemisSharedCommonModule),
+ MockComponent(ConfirmIconComponent)
],
+ imports: [
+ MockModule(ArtemisConfirmIconModule),
+ MockModule(ArtemisSharedCommonModule)
+ ],🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the component actually depends on these modules
# Look for imports or usage of components from these modules in the component file
# Search for imports from these modules in the component
rg -A 5 "import.*ArtemisSharedCommon|import.*ArtemisConfirmIcon" "src/main/webapp/app/shared/link-preview/components/link-preview/"
# Search for any usage of components from these modules
ast-grep --pattern 'class LinkPreviewComponent {
$$$
}'
Length of output: 2177
Script:
#!/bin/bash
# Check template usage of components from these modules
cat src/main/webapp/app/shared/link-preview/components/link-preview/link-preview.component.html
Length of output: 1779
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-tag-selector.component.spec.ts (1)
12-12: LGTM: Module import aligns with standalone component migration
The addition of ArtemisSharedCommonModule to the test setup is consistent with the PR's objective of migrating to standalone components.
Also applies to: 22-22
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.spec.ts (1)
21-21: LGTM: Module setup aligns with standalone component migration
The addition of ArtemisSharedComponentModule to the test setup is appropriate for the migration to standalone components.
Also applies to: 35-35
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts (2)
Line range hint
31-41: Enhance test assertions using recommended patterns.The test assertions can be improved to follow the coding guidelines for jest expectations.
Consider updating the assertions to use the recommended patterns:
- expect(transformedStringWithReactingUsers).toBe(metisUser1.name + 'artemisApp.metis.reactedTooltip'); - expect(updateReactingUsersStringSpy).toHaveBeenCalledOnce(); + expect(transformedStringWithReactingUsers).toEqual(metisUser1.name + 'artemisApp.metis.reactedTooltip'); + expect(updateReactingUsersStringSpy).toHaveBeenCalledExactlyOnceWith();Similar updates can be applied to other test cases for consistency with the coding guidelines.
Line range hint
109-122: Verify language change test implementation.The language change test is good, but could be more specific in its assertions.
Consider enhancing the test to verify the actual translated string:
translateService.use('de'); tick(); - expect(updateReactingUsersStringSpy).toHaveBeenCalledTimes(2); + expect(updateReactingUsersStringSpy).toHaveBeenCalledExactlyOnceWith(); + expect(transformedStringWithReactingUsers).toEqual(expect.any(String)); + expect(transformedStringWithReactingUsers).not.toEqual('');src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (2)
Line range hint
146-447: Excellent test coverage and implementation!The test suite demonstrates several strengths:
- Comprehensive coverage of component functionality
- Well-structured test cases following AAA pattern
- Thorough mocking of dependencies
- Clear test descriptions
- Good edge case handling
However, consider adding tests for:
- Error scenarios in markdown processing
- Memory leak prevention in emoji picker
Line range hint
311-332: Consider enhancing type safety in test helper functions.The
simulateKeydownEventhelper function could benefit from explicit TypeScript types:-const simulateKeydownEvent = (editor: TextEditor, key: string, modifiers: { shiftKey?: boolean; metaKey?: boolean } = {}) => { +interface KeyEventModifiers { + shiftKey?: boolean; + metaKey?: boolean; +} + +const simulateKeydownEvent = ( + editor: TextEditor, + key: string, + modifiers: KeyEventModifiers = {} +): { event: KeyboardEvent; preventDefaultSpy: jest.SpyInstance; stopPropagationSpy: jest.SpyInstance } => {src/test/javascript/spec/component/shared/metis/message-reply-inline-input/message-reply-inline-input.component.spec.ts (1)
Line range hint
63-64: Improve expectation specificityConsider using
toHaveBeenCalledExactlyOnceWithinstead oftoHaveBeenCalledOnceto make the test more specific about the expected payload.-expect(onCreateSpy).toHaveBeenCalledOnce(); +expect(onCreateSpy).toHaveBeenCalledExactlyOnceWith({ ...component.posting, content: newContent });src/test/javascript/spec/component/shared/metis/message-inline-input-component/message-inline-input.component.spec.ts (2)
Line range hint
63-64: Enhance test specificity with exact matcher assertionsConsider using more specific matchers to verify the exact payload being emitted:
-expect(onCreateSpy).toHaveBeenCalledOnce(); +expect(onCreateSpy).toHaveBeenCalledExactlyOnceWith({ ...component.posting, content: newContent }); -expect(onEditSpy).toHaveBeenCalledOnce(); +expect(onEditSpy).toHaveBeenCalledExactlyOnceWith(false);Also applies to: 98-99
Line range hint
71-90: Consider enhancing error scenario testsThe error handling tests could be improved by:
- Verifying that error messages are properly handled/displayed
- Testing different types of errors (network, validation, etc.)
- Ensuring proper cleanup of component state
Would you like me to provide an example implementation of these enhancements?
Also applies to: 108-127
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-tag-selector.component.spec.ts (1)
Line range hint
1-48: Add missing test cases for core functionalityThe test suite is missing critical test cases:
- Adding a category
- Removing a category
- Tag selection validation
- Error handling scenarios
Consider adding these test cases to ensure comprehensive coverage:
it('should add a category when selected', () => { const newTag = metisTags[0]; component.addTag(newTag); expect(component.postTags).toContain(newTag); }); it('should remove a category when clicked', () => { const tagToRemove = metisTags[0]; component.postTags = [tagToRemove]; component.removeTag(tagToRemove); expect(component.postTags).not.toContain(tagToRemove); }); it('should validate tag selection', () => { component.postTags = metisTags; const newTag = metisTags[0]; component.addTag(newTag); expect(component.postTags).not.toContain(newTag); });src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.spec.ts (1)
Line range hint
89-94: Consider debouncing similar posts check for better performanceThe similar posts check in the title input handler could benefit from debouncing to reduce unnecessary API calls.
Consider implementing debounce:
-expect(component.similarPosts).toEqual([]); +// Add test for debounced similar posts check +it('should debounce similar posts check', fakeAsync(() => { + const similarPostsSpy = jest.spyOn(metisService, 'getSimilarPosts'); + component.formGroup.get('title')?.setValue('New Title'); + tick(300); // Before debounce time + expect(similarPostsSpy).not.toHaveBeenCalled(); + tick(500); // After debounce time + expect(similarPostsSpy).toHaveBeenCalledOnce(); +}));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
src/test/javascript/spec/component/link-preview/link-preview.component.spec.ts(2 hunks)src/test/javascript/spec/component/overview/discussion-section/discussion-section.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/message-inline-input-component/message-inline-input.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/message-reply-inline-input/message-reply-inline-input.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts(3 hunks)src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-tag-selector.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts(4 hunks)src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts(2 hunks)src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-tag-selector.component.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}}
src/test/javascript/spec/component/shared/metis/message-inline-input-component/message-inline-input.component.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}}
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.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}}
src/test/javascript/spec/component/overview/discussion-section/discussion-section.component.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}}
src/test/javascript/spec/component/shared/metis/message-reply-inline-input/message-reply-inline-input.component.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}}
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.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}}
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.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}}
src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.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}}
src/test/javascript/spec/component/link-preview/link-preview.component.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}}
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.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 (15)
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts (1)
15-15: LGTM! Empty declarations array aligns with standalone pipe architecture.
The removal of declarations is correct as the pipe is now standalone and doesn't require module-level declarations.
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (2)
4-4: LGTM! Import statements are well-organized.
The imports are logically grouped and follow a consistent pattern. The new imports for list-related actions and markdown editor module enhance the testing capabilities.
Also applies to: 26-27, 37-39
119-119: LGTM! Test module configuration follows best practices.
Using MockModule(ArtemisMarkdownEditorModule) instead of the actual module is the correct approach for unit testing, as it helps isolate the component under test and prevents unnecessary initialization of dependencies.
src/test/javascript/spec/component/shared/metis/message-reply-inline-input/message-reply-inline-input.component.spec.ts (1)
14-14: LGTM: Proper module configuration for standalone component testing
The addition of ArtemisSharedCommonModule and its proper mocking using MockModule aligns with the PR's objective of migrating to standalone components. The test configuration maintains proper isolation while providing necessary dependencies.
Also applies to: 25-25
src/test/javascript/spec/component/shared/metis/message-inline-input-component/message-inline-input.component.spec.ts (1)
14-14: LGTM: Consistent module configuration across components
The addition of ArtemisSharedCommonModule maintains consistency with other component tests and properly supports standalone component testing.
Also applies to: 25-25
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-tag-selector.component.spec.ts (2)
12-12: LGTM: Module import aligns with standalone component migration
The addition of ArtemisSharedCommonModule is consistent with the PR's objective of migrating to standalone components.
22-22: LGTM: Test configuration properly mocks all dependencies
The TestBed configuration correctly includes all required modules with proper mocking.
src/test/javascript/spec/component/link-preview/link-preview.component.spec.ts (2)
6-6: LGTM: Module imports are properly organized
The imports are correctly structured with proper separation between external and internal modules.
Also applies to: 14-15
24-30: LGTM: Test configuration follows best practices
The test module configuration:
- Properly mocks all required components and modules
- Correctly includes shared modules needed for standalone components
- Maintains clear separation of concerns
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.spec.ts (1)
21-21: LGTM: Module configuration aligns with standalone component architecture
The addition of ArtemisSharedComponentModule and its integration into the test configuration is correct.
Also applies to: 35-35
src/test/javascript/spec/component/overview/discussion-section/discussion-section.component.spec.ts (2)
75-86: LGTM: Test configuration follows best practices.
The test setup properly mocks all required modules using MockModule, which is the recommended approach for Angular testing. The configuration includes all necessary shared modules and follows Angular testing best practices.
82-82: Consider moving DiscussionSectionComponent to declarations array.
The component under test is currently in the imports array, which suggests it's a standalone component. If this is intentional as part of the migration to standalone components, this is fine. However, if this wasn't intended, it should be moved to the declarations array.
✅ Verification successful
Based on the output, I can see that the DiscussionSectionComponent is indeed a standalone component, as evidenced by this snippet from the search results:
src/main/webapp/app/overview/discussion-section/discussion-section.component.ts:26: imports: [FontAwesomeModule, ArtemisSharedModule, ArtemisPlagiarismCasesSharedModule, InfiniteScrollModule],
src/main/webapp/app/overview/discussion-section/discussion-section.component.ts:27: standalone: true,The component is correctly placed in the imports array since it's a standalone component.
The component is explicitly marked with standalone: true in its decorator, which means it should be imported rather than declared. This is part of the Angular standalone components architecture, and the current setup is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if DiscussionSectionComponent is a standalone component
# Search for standalone: true in the component definition
ast-grep --pattern 'Component({
$$$
standalone: true,
$$$
})'
Length of output: 66132
src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts (1)
27-27: LGTM: Proper module mocking configuration.
The test setup correctly uses MockModule for shared modules, which is the recommended approach for unit testing Angular components.
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts (2)
33-35: LGTM: Imports align with standalone component requirements
The new imports are correctly added and follow Angular's import organization conventions.
Line range hint 1-252: LGTM: Comprehensive test coverage with proper assertions
The test suite follows best practices with:
- Proper use of jest expectations and spy assertions
- Comprehensive coverage of component functionality
- Well-structured test organization
- Clear test descriptions
...postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts (2)
Line range hint
54-73: Improve test assertions using jest best practicesWhile the test assertions are functional, they can be improved to follow jest best practices:
-expect(markdownRenderedTexts).toHaveLength(1); +expect(markdownRenderedTexts).toHaveLength(1); -expect(referenceLink).toBeNull(); +expect(referenceLink).toBeNull(); -expect(markdownRenderedTexts![0].innerHTML).toBe('<p class="inline-paragraph">' + postingContent + '</p>'); +expect(markdownRenderedTexts![0].innerHTML).toEqual('<p class="inline-paragraph">' + postingContent + '</p>');Consider adding tests for:
- Standalone component initialization
- Error handling scenarios
Line range hint
127-166: Enhance test maintainability and readabilityConsider the following improvements:
- Extract test data into constants or test fixtures
- Make assertions more specific for reference types
-['File Upload Exercise', '/courses/1/exercises/30', ReferenceType.FILE_UPLOAD, 'fa fa-file-arrow-up'], +const TEST_REFERENCES = [ + { + name: 'File Upload Exercise', + path: '/courses/1/exercises/30', + type: ReferenceType.FILE_UPLOAD, + icon: 'fa fa-file-arrow-up' + }, + // ... other references +]; -it.each([ +it.each(TEST_REFERENCES)('should contain a reference to %s with icon', - ['File Upload Exercise', '/courses/1/exercises/30', ReferenceType.FILE_UPLOAD, 'fa fa-file-arrow-up'], + ({name, path, type, icon}) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/test/javascript/spec/component/link-preview/link-preview-container.component.spec.ts(1 hunks)src/test/javascript/spec/component/shared/metis/message-inline-input-component/message-inline-input.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/message-reply-inline-input/message-reply-inline-input.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts(1 hunks)src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/javascript/spec/component/shared/metis/message-inline-input-component/message-inline-input.component.spec.ts
- src/test/javascript/spec/component/shared/metis/message-reply-inline-input/message-reply-inline-input.component.spec.ts
- src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/test/javascript/spec/component/link-preview/link-preview-container.component.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}}
src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.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 (4)
src/test/javascript/spec/component/link-preview/link-preview-container.component.spec.ts (2)
Line range hint 1-93: LGTM! Test implementation follows best practices
The test suite demonstrates excellent practices:
- Proper use of jest matchers (toBeFalse, toBeTrue)
- Correct spy verification with toHaveBeenCalledOnce
- Well-structured test cases with clear arrange-act-assert patterns
- Proper mocking of services
19-19: Verify standalone component configuration
The empty declarations array suggests that LinkPreviewContainerComponent might have been converted to a standalone component. Let's verify this configuration.
✅ Verification successful
Empty declarations array is correct for standalone component
The empty declarations array in the test file is correct. The component is configured as a standalone component with standalone: true in its decorator, which means it doesn't need to be declared in any module or test declarations array.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if LinkPreviewContainerComponent is configured as standalone
# Check if the component is declared as standalone
ast-grep --pattern 'class LinkPreviewContainerComponent {
$$$
standalone: true,
$$$
}'
# Alternative: Check component decorator
ast-grep --pattern '@Component({
$$$
standalone: true,
$$$
})'
Length of output: 66108
Script:
#!/bin/bash
# Let's search for LinkPreviewContainerComponent's definition and configuration
# Search for the component file
fd "link-preview-container.component.ts" --type f
# Search for the component's configuration
rg -l "class LinkPreviewContainerComponent" --type ts
# Alternative: Search for the component's decorator
rg "@Component.*LinkPreviewContainerComponent" --type ts
Length of output: 384
Script:
#!/bin/bash
# Let's check the content of the component file to verify its configuration
cat "src/main/webapp/app/shared/link-preview/components/link-preview-container/link-preview-container.component.ts"
Length of output: 3593
src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts (2)
34-34: Verify standalone component test configuration
The test configuration has been updated to remove HtmlForPostingMarkdownPipe from declarations, which is correct for standalone components. However, ensure that the pipe is properly imported in the component itself.
✅ Verification successful
Test configuration is correctly updated for standalone component
The test configuration is correct. The component has been properly migrated to standalone with:
HtmlForPostingMarkdownPipeis imported in the component's imports array- The pipe itself is also standalone
- Only necessary mock directives remain in the test declarations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the pipe import in the component
ast-grep --pattern 'import { HtmlForPostingMarkdownPipe } from $_' 'src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts'
Length of output: 173
Script:
#!/bin/bash
# First, let's verify if the component file exists (there might be a typo in the path)
fd "posting-content-part.component.ts" --type f
# Then, let's check the content of the component file to see the imports and standalone configuration
fd "posting-content-part.component.ts" --type f --exec cat {}
# Also check for any markdown pipe files in the codebase
fd "html-for-posting-markdown.pipe.ts" --type f
Length of output: 226
Script:
#!/bin/bash
# Let's check the correct path for the component and its content
fd "posting-content-part" --type f
# Let's also check the content of the markdown pipe
cat src/main/webapp/app/shared/pipes/html-for-posting-markdown.pipe.ts
Length of output: 1760
Script:
#!/bin/bash
# Let's check the content of the component file to verify its standalone configuration and imports
cat src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts
Length of output: 5434
Line range hint 1-15: Verify standalone component migration completeness
The test configuration has been updated for standalone components, but please verify:
- All required imports are present in the component
- No regression in test coverage
✅ Verification successful
Migration to standalone component is properly configured
The component has been correctly migrated to standalone with all necessary imports and dependencies:
- Component is properly configured with
standalone: true - Required imports are present:
RouterLink,FaIconComponent,TranslateDirective,HtmlForPostingMarkdownPipe - Test configuration includes all necessary dependencies through imports and providers
- No regression in test coverage as all test cases are maintained
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify component imports and test coverage
# Check standalone component configuration
ast-grep --pattern 'import { $_ } from $_' 'src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts'
# Check test coverage (if jest coverage reports are available)
find . -name "coverage-final.json" -exec cat {} \; | jq '.["src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts"].statements.pct'
Length of output: 335
Script:
#!/bin/bash
# Let's first check if the component file exists and verify its path
fd "posting-content-part.component" --type f
# If found with a different path, let's check its content for standalone configuration
# and imports using ripgrep for better context
rg -l "standalone:\s*true" --type ts
rg "@Component" -A 5 --type ts
Length of output: 66817
Script:
#!/bin/bash
# Let's check the specific component file for standalone configuration and imports
rg -A 10 "@Component" src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts
# Also check test file for any missing imports or dependencies
rg -A 10 "describe|beforeEach" src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts
Length of output: 2914
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (20)
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.spec.ts (1)
Line range hint
23-191: Test structure follows best practices but could benefit from additional verifications.The test suite demonstrates good practices:
- Proper setup/teardown
- Clear test descriptions
- Correct async handling with fakeAsync/tick
- Specific assertions
Consider adding these verifications:
- Error scenarios (e.g., service failures)
- Form validation cases
- Edge cases for context selection
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts (4)
Line range hint
1-13: Add missing import for HtmlForPostingMarkdownPipeThe
HtmlForPostingMarkdownPipeis used in the TestBed configuration but is not imported.Add this import at the top of the file:
+ import { HtmlForPostingMarkdownPipe } from 'app/shared/pipes/html-for-posting-markdown.pipe';🧰 Tools
🪛 eslint
[error] 14-15: Delete
⏎(prettier/prettier)
🪛 GitHub Check: client-tests-selected
[failure] 17-17:
Cannot find name 'HtmlForPostingMarkdownPipe'.🪛 GitHub Check: client-style
[failure] 14-14:
Delete⏎🪛 GitHub Check: client-tests
[failure] 17-17:
Cannot find name 'HtmlForPostingMarkdownPipe'.
14-14: Remove extra blank lineRemove the extra blank line to comply with the project's formatting guidelines.
🧰 Tools
🪛 eslint
[error] 14-15: Delete
⏎(prettier/prettier)
🪛 GitHub Check: client-style
[failure] 14-14:
Delete⏎
Line range hint
27-139: Consider improving test readability and maintainabilityWhile the test cases are comprehensive and well-structured, consider these improvements:
- Use template literals instead of string concatenation for better readability
- Extract the trimming limit (10) as a constant
Example refactor for the expectations:
- expect(transformedStringWithReactingUsers).toBe(metisUser1.name + 'artemisApp.metis.reactedTooltip'); + expect(transformedStringWithReactingUsers).toBe(`${metisUser1.name}artemisApp.metis.reactedTooltip`);Add a constant at the top of the file:
const MAX_DISPLAYED_USERS = 10;🧰 Tools
🪛 eslint
[error] 14-15: Delete
⏎(prettier/prettier)
🪛 GitHub Check: client-tests-selected
[failure] 17-17:
Cannot find name 'HtmlForPostingMarkdownPipe'.🪛 GitHub Check: client-style
[failure] 14-14:
Delete⏎🪛 GitHub Check: client-tests
[failure] 17-17:
Cannot find name 'HtmlForPostingMarkdownPipe'.
Line range hint
27-139: Add tests for edge casesConsider adding test cases for the following scenarios:
- Empty array input
- Null/undefined input
- Array containing null/undefined values
Example test case:
it('should handle empty array input', fakeAsync(() => { reactingUsersPipe.transform([]).subscribe((transformedReactingUsers: string) => { transformedStringWithReactingUsers = transformedReactingUsers; }); tick(); expect(transformedStringWithReactingUsers).toBe(''); expect(updateReactingUsersStringSpy).toHaveBeenCalledOnce(); }));🧰 Tools
🪛 eslint
[error] 14-15: Delete
⏎(prettier/prettier)
🪛 GitHub Check: client-tests-selected
[failure] 17-17:
Cannot find name 'HtmlForPostingMarkdownPipe'.🪛 GitHub Check: client-style
[failure] 14-14:
Delete⏎🪛 GitHub Check: client-tests
[failure] 17-17:
Cannot find name 'HtmlForPostingMarkdownPipe'.src/main/webapp/app/shared/metis/metis.service.ts (3)
39-46: LGTM! Modern dependency injection pattern applied.The refactoring to use Angular's
injectfunction aligns with modern best practices and improves code readability.Consider declaring the injected services as
readonlyto prevent accidental reassignment:- protected postService = inject(PostService); + protected readonly postService = inject(PostService);
Line range hint
516-528: Improve WebSocket subscription cleanup.The WebSocket subscription management could be enhanced to prevent potential memory leaks.
Consider these improvements:
private createWebsocketSubscription(channel: string): void { + const subscription = this.jhiWebsocketService.receive(this.subscriptionChannel).subscribe(this.handleNewOrUpdatedMessage); + this.subscriptions.push(subscription); // if channel subscription does not change, do nothing if (this.subscriptionChannel === channel) { return; } // unsubscribe from existing channel subscription if (this.subscriptionChannel) { this.jhiWebsocketService.unsubscribe(this.subscriptionChannel); + this.clearSubscriptions(); this.subscriptionChannel = undefined; }Add this to manage subscriptions:
private subscriptions: Subscription[] = []; private clearSubscriptions(): void { this.subscriptions.forEach(sub => sub.unsubscribe()); this.subscriptions = []; } ngOnDestroy(): void { this.clearSubscriptions(); if (this.subscriptionChannel) { this.jhiWebsocketService.unsubscribe(this.subscriptionChannel); } this.courseWideTopicSubscription.unsubscribe(); }
Line range hint
48-50: Implement cache size management.The unbounded cache of posts could lead to memory issues in large courses.
Consider implementing:
- A maximum cache size limit
- An LRU (Least Recently Used) eviction policy
- Periodic cache cleanup for old entries
Example implementation:
private readonly MAX_CACHE_SIZE = 1000; private readonly CACHE_CLEANUP_INTERVAL = 5 * 60 * 1000; // 5 minutes private initializeCacheCleanup(): void { setInterval(() => { if (this.cachedPosts.length > this.MAX_CACHE_SIZE) { this.cachedPosts = this.cachedPosts.slice(-this.MAX_CACHE_SIZE); this.posts$.next(this.cachedPosts); } }, this.CACHE_CLEANUP_INTERVAL); }src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts (2)
3-13: Consider grouping related importsConsider grouping the NgbAccordion-related imports into a single import statement for better maintainability.
-import { - NgbAccordionBody, - NgbAccordionButton, - NgbAccordionCollapse, - NgbAccordionDirective, - NgbAccordionHeader, - NgbAccordionItem, - NgbAccordionToggle, - NgbCollapse, - NgbModal, -} from '@ng-bootstrap/ng-bootstrap'; +import { + NgbAccordionBody, NgbAccordionButton, NgbAccordionCollapse, + NgbAccordionDirective, NgbAccordionHeader, NgbAccordionItem, + NgbAccordionToggle, NgbCollapse, NgbModal +} from '@ng-bootstrap/ng-bootstrap';
Line range hint
134-139: Prevent potential memory leak in form subscriptionThe subscription to form control changes should be cleaned up to prevent memory leaks.
+private contextSubscription?: Subscription; +ngOnDestroy(): void { + this.contextSubscription?.unsubscribe(); +} resetFormGroup(): void { this.pageType = this.metisService.getPageType(); this.tags = this.posting?.tags ?? []; this.similarPosts = []; this.posting.title = this.posting.title ?? ''; this.resetCurrentContextSelectorOption(); this.formGroup = this.formBuilder.group(this.postValidator()); - this.formGroup.controls['context'].valueChanges.subscribe((context: ContextSelectorOption) => { + this.contextSubscription = this.formGroup.controls['context'].valueChanges.subscribe((context: ContextSelectorOption) => { this.currentContextSelectorOption = context; this.similarPosts = []; }); }src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts (1)
Line range hint
64-65: Standardize expectation patternsWhile the expectations are generally good, consider standardizing them according to the coding guidelines:
- Replace
toBeNull()withtoBeNull()ornot.toBeNull()consistently- Use
toHaveBeenCalledExactlyOnceWith()instead of separatetoHaveBeenCalledOnce()andtoHaveBeenCalledWith()Example refactor:
-expect(navigateByUrlSpy).toHaveBeenCalledOnce(); -expect(openAttachmentSpy).toHaveBeenCalledWith(attachmentURL); +expect(navigateByUrlSpy).toHaveBeenCalledExactlyOnceWith(); +expect(openAttachmentSpy).toHaveBeenCalledExactlyOnceWith(attachmentURL);Also applies to: 82-83, 146-147, 187-188, 235-236
🧰 Tools
🪛 eslint
[error] 52-53: Delete
⏎(prettier/prettier)
🪛 GitHub Check: client-style
[failure] 52-52:
Delete⏎src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (1)
Missing test coverage for StrikethroughAction in markdown editor component
The StrikethroughAction is actively used in the component's defaultActions array but lacks test coverage in the spec file. This should be addressed to maintain consistent test coverage across all editor actions.
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts: Add test cases for StrikethroughAction similar to other action tests.🔗 Analysis chain
Line range hint
1-577: Consider adding tests for StrikethroughAction.The test file has comprehensive coverage for various editor actions (BulletedListAction, OrderedListAction, etc.) but lacks tests for StrikethroughAction despite it being imported.
Consider adding test cases for StrikethroughAction if it's used in the component, or remove the import if it's not needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if StrikethroughAction is used elsewhere in the codebase ast-grep --pattern 'new StrikethroughAction($$$)'Length of output: 584
Script:
#!/bin/bash # Check if StrikethroughAction is used in the component under test ast-grep --pattern 'class PostingMarkdownEditorComponent { $$$ defaultActions = [ $$$ ] $$$ }'Length of output: 232
Script:
#!/bin/bash # Let's try a simpler pattern to find StrikethroughAction usage in the component rg -A 5 -B 5 "StrikethroughAction" "src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts"Length of output: 1296
🧰 Tools
🪛 eslint
[error] 151-152: Delete
⏎(prettier/prettier)
🪛 GitHub Check: client-style
[failure] 151-151:
Delete⏎src/main/webapp/app/shared/metis/posting-create-edit-modal/posting-create-edit-modal.directive.ts (1)
7-8: Consider further standalone component preparationSince this PR aims to migrate to standalone components, consider these architectural improvements:
- Add
standalone: trueto child components that extend this directive- Consider splitting the modal functionality into a composable service
- Document the migration path for components using this directive
Example standalone component structure for child classes:
@Component({ standalone: true, imports: [CommonModule, ReactiveFormsModule], // ... other metadata }) export class YourModalComponent extends PostingCreateEditModalDirective<Posting> { // implementation }src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (3)
Line range hint
26-43: Consider migrating all inputs/outputs to the new signal-based syntaxFor consistency, consider migrating all
@Input()and@Output()decorators to the new signal-basedinput()andoutput()functions. This would align with Angular's latest best practices and provide better change detection.Example migration:
- @Input() content?: string; - @Input() previewMode?: boolean; - @Input() author?: User; - @Input() isEdited = false; - @Input() posting?: Posting; - @Input() isReply?: boolean; - @Output() userReferenceClicked = new EventEmitter<string>(); - @Output() channelReferenceClicked = new EventEmitter<number>(); + content = input<string | undefined>(undefined); + previewMode = input<boolean | undefined>(undefined); + author = input<User | undefined>(undefined); + isEdited = input(false); + posting = input<Posting | undefined>(undefined); + isReply = input<boolean | undefined>(undefined); + userReferenceClicked = output<string>(); + channelReferenceClicked = output<number>();
Line range hint
89-264: Consider refactoring reference handling logic into a separate serviceThe
computePostingContentPartsmethod is quite complex and handles multiple types of references. Consider extracting this logic into a dedicated service to improve maintainability and testability.Suggested approach:
- Create a new
PostingReferenceService- Extract the reference type handling logic into separate methods
- Use dependency injection to utilize the service
Example structure:
@Injectable({ providedIn: 'root' }) export class PostingReferenceService { handlePostReference(content: string, match: PatternMatch): Partial<PostingContentPart> { // Extract post reference handling logic } handleExerciseReference(content: string, match: PatternMatch): Partial<PostingContentPart> { // Extract exercise reference handling logic } // Additional methods for other reference types }
Line range hint
265-308: Consider using a more maintainable regex patternThe current regex pattern is complex and hard to maintain. Consider breaking it down into smaller, named patterns and combining them.
Example:
private static readonly PATTERNS = { POST: /#\d+/, PROGRAMMING: /\[programming].*?\[\/programming]/, // ... other patterns } as const; private static readonly COMBINED_PATTERN = new RegExp( Object.entries(PATTERNS) .map(([key, pattern]) => `(?<${key}>${pattern.source})`) .join('|'), 'g' );src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (4)
46-60: Consider organizing imports array by categoryWhile the imports are correct, consider organizing them by category for better maintainability:
imports: [ // Angular built-in NgClass, NgIf, NgStyle, // CDK modules CdkOverlayOrigin, CdkConnectedOverlay, // Third-party FaIconComponent, // Application-specific TranslateDirective, AnswerPostHeaderComponent, PostingContentComponent, AnswerPostReactionsBarComponent, AnswerPostCreateEditModalComponent, EmojiPickerComponent, ],
Line range hint
127-146: Consider improving error handling in click handlerThe try-catch block in
onRightClickswallows the error after logging it and defaults totrue, which might lead to unexpected behavior.try { isPointerCursor = window.getComputedStyle(targetElement).cursor === 'pointer'; } catch (error) { - console.error('Failed to compute style:', error); - isPointerCursor = true; + console.error('Failed to compute style for element:', { error, element: targetElement }); + isPointerCursor = false; // safer default }
Line range hint
116-124: Potential memory leak in static referenceThe static
activeDropdownPostreference could lead to memory leaks if not properly cleaned up. While there is cleanup inngOnDestroy, consider using a service with proper lifecycle management instead.Consider moving the dropdown management to a service:
@Injectable({ providedIn: 'root' }) export class DropdownService { private activeDropdown = signal<AnswerPostComponent | null>(null); setActiveDropdown(component: AnswerPostComponent | null) { this.activeDropdown.set(component); } cleanup() { const current = this.activeDropdown(); if (current) { current.showDropdown = false; current.enableBodyScroll(); current.changeDetector.detectChanges(); this.setActiveDropdown(null); } } }Also applies to: 166-171
Line range hint
95-102: Improve type safety in DOM manipulationThe
querySelectorcalls could return null, which isn't handled.private disableBodyScroll() { const mainContainer = this.document.querySelector('.thread-answer-post'); - if (mainContainer) { + if (!mainContainer) { + console.warn('Thread answer post container not found'); + return; + } this.renderer.setStyle(mainContainer, 'overflow', 'hidden'); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (15)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts(3 hunks)src/main/webapp/app/shared/metis/metis.module.ts(1 hunks)src/main/webapp/app/shared/metis/metis.service.ts(2 hunks)src/main/webapp/app/shared/metis/post/post.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts(2 hunks)src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts(2 hunks)src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-create-edit-modal/posting-create-edit-modal.directive.ts(1 hunks)src/main/webapp/app/shared/metis/posting-create-edit.directive.ts(2 hunks)src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts(2 hunks)src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts(1 hunks)src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts(3 hunks)src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts(5 hunks)src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts
- src/main/webapp/app/shared/metis/posting-create-edit.directive.ts
- src/main/webapp/app/shared/metis/metis.module.ts
- src/main/webapp/app/shared/metis/post/post.component.ts
- src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts
🧰 Additional context used
📓 Path-based instructions (10)
src/main/webapp/app/shared/metis/posting-create-edit-modal/posting-create-edit-modal.directive.ts (1)
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (1)
src/main/webapp/app/shared/metis/metis.service.ts (1)
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.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}}
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (1)
src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (1)
src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts (1)
src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.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}}
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.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}}
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.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}}
🪛 GitHub Check: client-style
src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts
[failure] 52-52:
Delete ⏎
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts
[failure] 43-43:
'StrikethroughAction' is defined but never used
[failure] 151-151:
Delete ⏎
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts
[failure] 14-14:
Delete ⏎
🪛 eslint
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts
[error] 43-43: 'StrikethroughAction' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 151-152: Delete ⏎
(prettier/prettier)
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts
[error] 14-15: Delete ⏎
(prettier/prettier)
🔇 Additional comments (18)
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.spec.ts (1)
Line range hint 184-189: LGTM! Modal service test follows best practices.
The test correctly:
- Uses jest spying
- Implements proper mock return values
- Uses the recommended
toHaveBeenCalledOnce()assertion
src/main/webapp/app/shared/metis/metis.service.ts (1)
65-68:
Add proper null checks in the constructor.
The user identity assignment still uses a non-null assertion which could lead to runtime errors.
Apply this fix:
- this.accountService.identity().then((user: User) => {
- this.user = user!;
- });
+ this.accountService.identity().then((user: User | null) => {
+ if (!user) {
+ console.error('User identity not found');
+ return;
+ }
+ this.user = user;
+ });src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (4)
47-47: Fix the import path for PostingContentComponent
The import path appears to be incorrect with .components extension.
62-63: LGTM: Proper standalone component configuration
The component is correctly configured as standalone with the necessary imports.
66-72: LGTM: Clean dependency injection implementation
The migration to inject() function is well implemented, maintaining proper scoping and following Angular's modern dependency injection pattern.
Line range hint 85-85: LGTM: Modern input property implementation
The migration to signal-based input() function follows Angular's latest best practices.
src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts (3)
43-61: LGTM: Proper standalone component setup
The component is correctly configured as standalone with all necessary imports properly declared.
64-67: LGTM: Clean dependency injection implementation
The migration to inject() function is well implemented with proper access modifiers.
Line range hint 192-198: LGTM: Robust form validation implementation
The form validation is well implemented with proper use of constants and pattern validation.
src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts (1)
Line range hint 54-270: Well-structured test implementation
The test suite demonstrates excellent practices:
- Clear test organization using describe blocks
- Comprehensive coverage of different scenarios
- Effective use of parameterized tests with jest.each
- Proper isolation of test cases
🧰 Tools
🪛 eslint
[error] 52-53: Delete ⏎
(prettier/prettier)
🪛 GitHub Check: client-style
[failure] 52-52:
Delete ⏎
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (3)
130-138: LGTM: TestBed configuration is well structured.
The configuration properly imports the required modules and provides mock services:
- MockModule(ArtemisMarkdownEditorModule) for markdown editor functionality
- MockLocalStorageService for storage operations
- MockTranslateService for translation handling
142-143: LGTM: Proper overlay service configuration.
The overlay providers are correctly overridden with mock implementations, which is essential for testing components that use CDK overlays (like the emoji picker).
Line range hint 332-338: LGTM: Comprehensive emoji picker overlay testing.
The test properly verifies the cleanup of overlay resources when the backdrop is clicked.
src/main/webapp/app/shared/metis/posting-create-edit-modal/posting-create-edit-modal.directive.ts (2)
1-1: LGTM: Import changes align with Angular style guide
The addition of inject to the existing Angular core imports is clean and properly organized.
9-10: Verify modal functionality after NgbModal removal
While the transition to property-based injection using inject() is good, the removal of NgbModal service needs verification:
- How is the modal functionality now handled?
- Are there any potential breaking changes in child classes?
src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (1)
Line range hint 1-15: Consider reviewing ArtemisSharedModule imports
Since the component is now standalone and explicitly imports its dependencies, consider reviewing the ArtemisSharedModule to remove any unused imports that might have been needed only for this component.
Also applies to: 22-23
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (2)
15-16: LGTM: Import statements are well-organized
The new imports support the standalone component migration and follow Angular's style guide for import ordering.
Also applies to: 25-25, 27-33
62-66: LGTM: Clean dependency injection implementation
The migration to inject() function and input() for property initialization follows Angular's latest best practices and improves code readability.
...s/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.spec.ts
Show resolved
Hide resolved
...avascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts
Show resolved
Hide resolved
...avascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts
Outdated
Show resolved
Hide resolved
...c/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts (1)
Line range hint
38-70: Add tests for translation functionalityThe addition of
ArtemisTranslatePipesuggests that the component handles translations, but there are no specific tests verifying this functionality. Consider adding test cases that:
- Verify that text content is properly translated
- Check translation key resolution
- Test fallback behavior for missing translations
Example test to add:
it('should properly translate reaction tooltips', () => { component.ngOnInit(); fixture.detectChanges(); // Verify that tooltip text is translated expect(component.pinTooltip).toBe('artemisApp.metis.pinPostTooltip'); const translatedText = fixture.debugElement .query(By.css('.pin')) .nativeElement .getAttribute('title'); expect(translatedText).not.toBe('artemisApp.metis.pinPostTooltip'); expect(translatedText).toBe('Translated Pin Post Tooltip'); });src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (1)
128-141: Consider usingMockProviderfor consistency.While the current setup works, consider using
MockProviderfor LocalStorageService and TranslateService to maintain consistency with other service mocks in the test configuration.-{ provide: LocalStorageService, useClass: MockLocalStorageService }, -{ provide: TranslateService, useClass: MockTranslateService }, +MockProvider(LocalStorageService, { useClass: MockLocalStorageService }), +MockProvider(TranslateService, { useClass: MockTranslateService }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
src/main/webapp/app/shared/metis/metis.module.ts(1 hunks)src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts(5 hunks)src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts(2 hunks)src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/webapp/app/shared/metis/metis.module.ts
- src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts
- src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts
- src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.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}}
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.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 (3)
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (3)
4-4: LGTM! Import organization aligns with Angular best practices.
The imports are well-organized, separating Angular core, testing utilities, and mock services. The addition of LocalStorageService and TranslateService with their corresponding mocks follows good testing practices.
Also applies to: 26-26, 28-28, 32-32, 41-47
329-329: LGTM! Test case change maintains test coverage.
The addition of fixture detection ensures proper component initialization before test execution.
147-154: Verify mock service initialization.
The test setup includes new service injections but doesn't verify their initialization. Consider adding assertions to ensure these services are properly initialized.
.../shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts
Show resolved
Hide resolved
| @Output() | ||
| valueChange = new EventEmitter<void>(); |
There was a problem hiding this comment.
Do you think you can change the output too?, to use the new output<EventEmitter<void>>()
There was a problem hiding this comment.
We discussed this in the communication team and will redistribute it into seperate smaller PR's, changing every output/input/viewchild etc. will lead to a lot of changes in the testfiles
PaRangger
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on TS4. Changes LGTM as a first step towards making the communication component ready for the angular 19 migration. 👍
Small things I noticed, that can be done as soon as we change the components themselves to also use signals:
- I saw an occurence where something is renamed by the script: EmojiComponent as EmojiComponent_1, which we do not need to rename (emoji.component.ts)
- There are some relative imports, which should be aliased (e.g. link-preview-container.component.ts, ...)
- The standalone components now import the modules, which should be limited to the components they actually use
I tested the following:
✅ Navigating the communication module
✅ Creating and formatting posts, referencing lectures and creating attachments
✅ Searching for posts
✅ Creating and formatting answer posts
✅ Deletion of answer posts and posts
✅ Removing users from group chats and leaving group chats
✅ Renaming a channel
✅ Saving messages for later and marking them as completed
✅ (Un-)Favoriting and (Un-)Hiding channels
✅ Tested on Chrome on macOS and Safari on iOS
Something I noticed: The changes also fix #9754 😄
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on TS1. Everything works fine
I tested:
✅ Reacting to posts and post answers
✅ Mute and unmute channels
✅ Tested formating posts, uploading pictures, referencing lectures and exercises
✅ Unread messages notifications
✅ Archive und unarchive channels
✅ Searching posts and channels
…s-service-to-standalone' into chore/communication/migrate-metis-service-to-standalone
cremertim
dismissed stale reviews from HawKhiem, PaRangger, and coderabbitai[bot]
via
e953525
sawys777
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Motivation and Context
Description
This PR adresses the MetisModule of Artemis. It migrates every component to a standalone component.
Steps for Testing
Prerequisites:
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
Summary by CodeRabbit
New Features
LinkPreviewContainerComponentandLinkPreviewComponentnow utilize new imports for improved functionality.PostingContentComponent,PostingThreadComponent, and other components through the addition of imports.Bug Fixes
Documentation
Refactor
injectfunction.Style