Development: Migrate client code for emoji components and conversation services
#10021
Conversation
asliayk
added
client
WalkthroughThe pull request introduces a series of modernization changes across several TypeScript files in the Metis module, primarily focusing on dependency injection and component architecture. The modifications involve transitioning from traditional constructor-based dependency injection to Angular's new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
src/main/webapp/app/shared/metis/conversations/group-chat.service.ts (1)
Line range hint
18-21: Consider enhancing error handling for HTTP requests.The HTTP requests could benefit from proper error handling using catchError operator.
Consider adding error handling:
create(courseId: number, loginsOfChatPartners: string[]): Observable<HttpResponse<GroupChatDTO>> { return this.http .post<OneToOneChatDTO>(`${this.resourceUrl}${courseId}/group-chats`, loginsOfChatPartners, { observe: 'response' }) - .pipe(map(this.conversationService.convertDateFromServer)); + .pipe( + map(this.conversationService.convertDateFromServer), + catchError(error => { + console.error('Error creating group chat:', error); + throw error; + }) + ); }src/main/webapp/app/shared/metis/conversations/channel.service.ts (2)
Line range hint
17-76: Consider organizing methods by responsibility.The service has grown quite large with many methods. Consider organizing them into logical groups using private methods or even splitting into smaller services.
Consider grouping methods by responsibility:
// Channel Management private channelManagementMethods = { create: (courseId: number, channelDTO: ChannelDTO) => {...}, update: (courseId: number, channelId: number, channelDTO: ChannelDTO) => {...}, delete: (courseId: number, channelId: number) => {...}, }; // Channel Status private channelStatusMethods = { archive: (courseId: number, channelId: number) => {...}, unarchive: (courseId: number, channelId: number) => {...}, }; // User Management private userManagementMethods = { registerUsers: (courseId: number, channelId: number, options: ChannelRegistrationOptions) => {...}, deregisterUsers: (courseId: number, channelId: number, logins?: string[]) => {...}, };
Line range hint
77-96: Consider using a type for registration options.The
registerUsersToChannelmethod has multiple optional parameters. Consider using a type to make it more maintainable.Consider this improvement:
interface ChannelRegistrationOptions { addAllStudents?: boolean; addAllTutors?: boolean; addAllInstructors?: boolean; logins?: string[]; } registerUsersToChannel( courseId: number, channelId: number, options: ChannelRegistrationOptions = {} ): Observable<HttpResponse<void>> { const userLogins = options.logins ?? [this.accountService.userIdentity?.login]; let params = new HttpParams(); if (options.addAllStudents) { params = params.set('addAllStudents', 'true'); } // ... rest of the implementation }src/main/webapp/app/shared/metis/conversations/conversation.service.ts (1)
1-1: LGTM! Consider grouping related imports.The migration to
inject()function follows Angular's latest best practices and maintains the correct access modifiers. The implementation is clean and type-safe.Consider grouping the Angular core imports together for better organization:
-import { Injectable, inject } from '@angular/core'; +import { inject, Injectable } from '@angular/core';Also applies to: 37-39
src/main/webapp/app/shared/metis/emoji/emoji-picker.component.html (1)
9-9: Consider adding aria-label for accessibility.While translations are properly implemented, consider adding an aria-label to improve accessibility for screen readers.
[i18n]="{ search: 'artemisApp.metis.searchEmoji' | artemisTranslate, categories: { recent: 'artemisApp.metis.courseEmojiSelectionCategory' | artemisTranslate } }" + [aria-label]="'artemisApp.metis.emojiPicker' | artemisTranslate"src/main/webapp/app/shared/metis/emoji/emoji.component.ts (1)
19-19: Consider cleanup for computed signal subscription.While the computed signal is correctly implemented, consider cleanup to prevent potential memory leaks in certain scenarios.
export class EmojiComponent { private themeService = inject(ThemeService); + private destroyed = false; utils = EmojiUtils; emoji = input<string>(''); - dark = computed(() => this.themeService.currentTheme() === Theme.DARK); + dark = computed(() => !this.destroyed && this.themeService.currentTheme() === Theme.DARK); + ngOnDestroy() { + this.destroyed = true; + } }src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts (1)
17-20: Consider using a specific type instead of 'any' for emojiSelectWhile the migration to signal-based input/output is good, the
emojiSelectoutput uses typeany. Consider defining a specific interface for the emoji selection event.- emojiSelect = output<any>(); + emojiSelect = output<{ emoji: EmojiData }>();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
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/emoji/emoji-picker.component.html(1 hunks)src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts(1 hunks)src/main/webapp/app/shared/metis/emoji/emoji.component.html(1 hunks)src/main/webapp/app/shared/metis/emoji/emoji.component.ts(1 hunks)src/main/webapp/app/shared/metis/metis.module.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
src/main/webapp/app/shared/metis/emoji/emoji.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/shared/metis/metis.module.ts (1)
src/main/webapp/app/shared/metis/conversations/one-to-one-chat.service.ts (1)
src/main/webapp/app/shared/metis/emoji/emoji.component.ts (1)
src/main/webapp/app/shared/metis/conversations/conversation.service.ts (1)
src/main/webapp/app/shared/metis/conversations/channel.service.ts (1)
src/main/webapp/app/shared/metis/conversations/group-chat.service.ts (1)
src/main/webapp/app/shared/metis/emoji/emoji-picker.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts (1)
🔇 Additional comments (14)
src/main/webapp/app/shared/metis/conversations/one-to-one-chat.service.ts (1)
1-1: LGTM! Modern dependency injection implementation.
The migration to using inject() function aligns with Angular's modern dependency injection pattern. The implementation is clean and maintains the service's functionality while reducing boilerplate code.
Also applies to: 12-13
src/main/webapp/app/shared/metis/conversations/group-chat.service.ts (1)
6-6: LGTM! Clean dependency injection implementation.
The migration to inject() function is well implemented for all three dependencies.
Also applies to: 13-15
src/main/webapp/app/shared/metis/conversations/channel.service.ts (1)
1-1: LGTM! Consistent dependency injection implementation.
The migration to inject() function follows the same pattern as other services and is well implemented.
Also applies to: 13-15
src/main/webapp/app/shared/metis/conversations/conversation.service.ts (2)
Line range hint 41-182: Verify error handling for HTTP calls.
The service methods make HTTP calls but don't handle potential errors. Consider adding error handling to improve robustness.
Let's check if error handling exists in components using this service:
Consider adding error handling using catchError operator:
return this.http.get<ConversationDTO[]>(...).pipe(
map(this.convertDateArrayFromServer),
catchError(error => {
console.error('Error fetching conversations:', error);
return throwError(() => error);
})
);Line range hint 41-182: Check for potential memory leaks in components using this service.
The service returns Observables but doesn't handle unsubscription. Ensure components properly handle subscription cleanup.
Let's check component implementations:
Consider these approaches to prevent memory leaks:
- Use
asyncpipe in templates where possible - Implement
ngOnDestroyto unsubscribe from subscriptions - Use
takeUntiloperator with a destroy$ subject
✅ Verification successful
Memory leak concerns in conversation service usage are properly handled
The codebase shows proper subscription management through multiple approaches:
-
Components using
takeUntilwith a destroy subject:conversation-members.component.tsconversation-messages.component.tsconversation-header.component.tscourse-conversations.component.tsconversation-options.component.ts
-
Services implementing
ngOnDestroy:metis.service.tsmetis-conversation.service.ts- Both properly unsubscribe from websocket topics and clean up subscriptions
-
Components implementing proper cleanup in
ngOnDestroy:- All components using the conversation service properly unsubscribe from their subscriptions
- Websocket subscriptions are properly cleaned up
- Multiple subscriptions are tracked and cleaned up individually
The review comment's concern about memory leaks is unfounded as the codebase demonstrates consistent and thorough subscription management practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for proper subscription handling in components
ast-grep --pattern 'conversation.service.$$$).subscribe($$$)'
Length of output: 187
Script:
#!/bin/bash
# Let's try a different approach to find component usages of the conversation service
# First, find files that import the conversation service
rg -l "import.*ConversationService" --type ts
# Then, let's look for subscription patterns in components
rg "subscribe\(" --type ts -A 2
# Also search for ngOnDestroy implementations
ast-grep --pattern 'ngOnDestroy() {
$$$
}'
Length of output: 69698
Script:
#!/bin/bash
# Let's search for conversation service usage in components and check their cleanup
rg -l "import.*ConversationService" --type ts | xargs rg "ngOnDestroy|takeUntil|unsubscribe" -B 2 -A 2
Length of output: 57744
src/main/webapp/app/shared/metis/emoji/emoji.component.html (2)
2-2: LGTM! Correct usage of new Angular control flow syntax.
The @if/@else syntax follows the new Angular guidelines, replacing *ngIf.
Also applies to: 4-4
3-3: Verify emoji() signal implementation.
The emoji property is now accessed as a function call, which suggests it's using Angular's new input signal syntax. This is the correct approach, but ensure that all parent components are passing the emoji value correctly.
Also applies to: 5-5
✅ Verification successful
Let me verify the implementation of the emoji signal and its usage by checking the component files.
Let me check the actual implementation of these components to verify the emoji property usage.
Signal implementation and usage are correctly implemented
The emoji component correctly uses Angular's new input signal syntax with emoji = input<string>(''), and the parent components are passing the emoji value correctly using property binding syntax [emoji]="...". The implementation aligns with Angular's latest practices for input handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that use jhi-emoji to verify correct property binding
rg -t html '<jhi-emoji.*\[emoji\]'
Length of output: 817
Script:
#!/bin/bash
# Check the emoji component implementation
ast-grep --pattern 'export class EmojiComponent {
$$$
emoji$$$
$$$
}'
# Check the components using jhi-emoji to verify their emoji property implementations
rg -l "class.*PostReactionsBarComponent|class.*AnswerPostReactionsBarComponent" --type ts
Length of output: 395
Script:
#!/bin/bash
# Check the emoji component implementation
cat src/main/webapp/app/shared/metis/emoji/emoji.component.ts
# Check how emoji is used in the parent components
rg -A 5 "emoji.*=" src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts
rg -A 5 "emoji.*=" src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts
Length of output: 1241
src/main/webapp/app/shared/metis/emoji/emoji-picker.component.html (1)
2-3: LGTM! Proper implementation of signal-based properties.
All properties correctly use function calls to access signal values.
Also applies to: 8-11
src/main/webapp/app/shared/metis/emoji/emoji.component.ts (2)
10-11: LGTM! Proper standalone component configuration.
The component is correctly configured as standalone with the necessary imports.
17-17: LGTM! Correct implementation of input signal.
The emoji input is properly implemented using the new signal-based input syntax with an appropriate default value.
src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts (2)
1-1: LGTM: Import organization follows Angular style guide
The imports are well-organized, following the Angular style guide with third-party imports separated from application imports.
Also applies to: 5-6
11-12: LGTM: Proper standalone component configuration
The component is correctly configured as standalone with necessary imports. This aligns with modern Angular practices for better modularity and tree-shaking.
src/main/webapp/app/shared/metis/metis.module.ts (2)
68-69: LGTM: Proper standalone component integration
The EmojiComponent and EmojiPickerComponent are correctly added to the imports array as they are now standalone components. This maintains backward compatibility while benefiting from the standalone architecture.
68-69: Verify component usage across the application
Let's ensure all usages of these components have been updated to work with the standalone versions.
✅ Verification successful
Let me check the actual usage of these components to verify if they're properly updated for standalone versions.
Let me check one final thing to verify if these components are properly imported in the components that use them.
Let me fix the regex and try one last time to verify the component configurations.
Components are properly configured as standalone
The verification shows that both components are correctly configured as standalone components and are being used in templates. The components:
- Are marked with
standalone: true - Have their required imports configured in their decorators
- Are properly used in templates with
<jhi-emoji>and<jhi-emoji-picker>selectors - No issues found with their integration in the application
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining module imports of these components
rg -l "import.*EmojiComponent.*from|import.*EmojiPickerComponent.*from" --type ts
# Search for component usage in templates
rg -l "jhi-emoji|jhi-emoji-picker" --type html
Length of output: 952
Script:
#!/bin/bash
# Check the component declarations in the test files
rg "import.*EmojiComponent|import.*EmojiPickerComponent" -A 5 src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts src/test/javascript/spec/component/emoji/emoji-picker.component.spec.ts
# Check how these components are used in the HTML templates
rg "<jhi-emoji|<jhi-emoji-picker" -A 5 src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html src/main/webapp/app/shared/metis/post/post.component.html src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.html src/main/webapp/app/shared/metis/answer-post/answer-post.component.html
# Check the component imports in the corresponding TS files
rg "import.*EmojiComponent|import.*EmojiPickerComponent" src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts src/main/webapp/app/shared/metis/post/post.component.ts src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts
Length of output: 10921
Script:
#!/bin/bash
# Check the imports in the component files that use these components
rg "^import.*{.*EmojiComponent.*}|^import.*{.*EmojiPickerComponent.*}" src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts src/main/webapp/app/shared/metis/post/post.component.ts src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts
# Also check if these components are marked as standalone
rg "@Component.*standalone" -B 5 src/main/webapp/app/shared/metis/emoji/emoji.component.ts src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts
Length of output: 740
Script:
#!/bin/bash
# Check the imports in the component files that use these components
rg "import.*\{.*EmojiComponent.*\}|import.*\{.*EmojiPickerComponent.*\}" src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts src/main/webapp/app/shared/metis/post/post.component.ts src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts
# Check the component decorators
rg "@Component" -A 5 src/main/webapp/app/shared/metis/emoji/emoji.component.ts src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts
Length of output: 1660
asliayk
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
|
github-actions
bot
added
the
deployment-error
HawKhiem
added
deploy:artemis-test6
and removed
deployment-error
HawKhiem
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
sachmii
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
JerroyTan
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Motivation and Context
For the client migration, the following files need to be updated:
Description
EmojiComponentandEmojiPickerComponenthave been updated to use signals and converted into standalone components.ChannelService,ConversationService,GroupChatService, andOneToOneChatServicehave been updated to use inject.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
Code Review
Manual Tests
Test Coverage
Summary by CodeRabbit
New Features
EmojiPickerComponentandEmojiComponent.Bug Fixes
Documentation
Chores