Development: Migrate client code for posting content
#10197
Conversation
asliayk
added
tests
client
asliayk
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request introduces modifications to several TypeScript and HTML files within the Metis service and posting components. The changes primarily focus on updating the way input properties are handled in Angular components, using the new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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: 3
🔭 Outside diff range comments (2)
src/main/webapp/app/shared/metis/posting-header/posting-header.component.ts (2)
Line range hint
134-157: Fix inconsistent user role handling in else block.This code segment appears to be the source of the bug mentioned in PR #9987 where a "Deleted" label incorrectly appears in the user role section. The else block inconsistently sets:
userAuthority = 'student'butuserRoleBadgeanduserAuthorityTooltipto 'deleted' valuesThis mismatch could cause the UI to show incorrect labels.
- this.userAuthority = 'student'; + this.userAuthority = 'deleted'; this.userRoleBadge = 'artemisApp.metis.userRoles.deleted'; this.userAuthorityTooltip = 'artemisApp.metis.userAuthorityTooltips.deleted';
Line range hint
71-82: Consider unsubscribing from authentication state subscription.The subscription to
getAuthenticationState()inngOnInitshould be properly managed to prevent memory leaks.+ private destroy$ = new Subject<void>(); ngOnInit(): void { this.accountService .getAuthenticationState() .pipe( + takeUntil(this.destroy$), tap((user: User) => { this.currentUser = user; this.setUserProperties(); }), ) .subscribe(); this.postingIsOfToday = dayjs().isSame(this.posting()?.creationDate, 'day'); this.todayFlag = this.getTodayFlag(); } + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }Don't forget to add the imports:
import { Subject } from 'rxjs'; import { takeUntil } from 'rxjs/operators';
🧹 Nitpick comments (2)
src/main/webapp/app/shared/metis/posting-header/posting-header.component.ts (1)
Line range hint
65-69: Consider using strict equality for enum comparison.When comparing with enum values, it's recommended to use strict equality (
===) to ensure type safety.- return this.isPost(posting) && posting.displayPriority == DisplayPriority.PINNED; + return this.isPost(posting) && posting.displayPriority === DisplayPriority.PINNED;src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (1)
Line range hint
106-203: Optimize repeated content() calls.Multiple calls to content() within the same method scope impact performance and readability. Consider storing the result in a local variable.
computePostingContentParts(patternMatches: PatternMatch[]): void { this.postingContentParts.set([]); if (patternMatches && patternMatches.length > 0) { + const content = this.content(); + if (!content) { + return; + } patternMatches.forEach((patternMatch: PatternMatch, index: number) => { - if (this.content() === undefined) { - return; - } - const referencedId = this.content()!.substring(patternMatch.startIndex + 1, patternMatch.endIndex); + const referencedId = content.substring(patternMatch.startIndex + 1, patternMatch.endIndex); // ... rest of the method using content instead of this.content() }); } }🧰 Tools
🪛 Biome (1.9.4)
[error] 186-186: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/webapp/app/shared/metis/metis.service.ts(1 hunks)src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.component.html(3 hunks)src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts(3 hunks)src/main/webapp/app/shared/metis/posting-content/posting-content.component.html(3 hunks)src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts(8 hunks)src/main/webapp/app/shared/metis/posting-header/posting-header.component.ts(2 hunks)src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts(5 hunks)src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts(10 hunks)src/test/javascript/spec/component/shared/metis/postings-header/posting-header.component.spec.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/test/javascript/spec/component/shared/metis/postings-header/posting-header.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (8)
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/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.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/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/main/webapp/app/shared/metis/metis.service.ts (1)
src/main/webapp/app/shared/metis/posting-header/posting-header.component.ts (1)
src/main/webapp/app/shared/metis/posting-content/posting-content.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/posting-content/posting-content-part/posting-content-part.components.ts (1)
src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (1)
🪛 Biome (1.9.4)
src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts
[error] 186-186: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (14)
src/main/webapp/app/shared/metis/posting-header/posting-header.component.ts (2)
1-1: LGTM! Good migration to signal-based APIs.The import statement has been correctly updated to include the new signal-based APIs (
computed,input,output).
Line range hint
30-39: Well-structured migration to signal-based inputs!The component has been properly migrated to use Angular's new input/output syntax. Properties are well-typed and follow naming conventions.
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (2)
39-41: LGTM! Clean migration to the new input/output API.The migration from decorators to the new input/output functions maintains type safety and follows Angular's latest best practices.
83-84: Well-handled null safety with optional chaining.The changes correctly adapt to the new input() function syntax while improving null safety through optional chaining operators.
Also applies to: 88-89
src/main/webapp/app/shared/metis/posting-content/posting-content.components.ts (1)
27-36: LGTM! Well-structured input/output declarations.The migration maintains proper typing and includes appropriate default values where needed.
src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts (1)
Line range hint
60-366: LGTM! Well-structured test migration.The tests have been properly migrated to use
runInInjectionContextfor setting inputs, maintaining comprehensive coverage and proper verification of component behavior.src/main/webapp/app/shared/metis/metis.service.ts (1)
260-260: LGTM! Good fix for authorRole preservation.The change correctly preserves the authorRole during post updates, fixing the issue where the "Deleted" label incorrectly appeared in the user role section after editing.
src/test/javascript/spec/component/shared/metis/posting-content/posting-content.component.spec.ts (2)
51-60: LGTM! Pattern matching test cases are well-structured.The test cases for pattern matching are comprehensive and follow best practices:
- Proper use of
runInInjectionContextfor setting inputs- Clear test cases for different scenarios
- Correct assertions using Jest's expect API
236-248: LGTM! Content parts computation test case is thorough.The test case for computing posting content parts:
- Properly tests the empty pattern matches scenario
- Correctly asserts the expected structure of
PostingContentPart- Uses type casting appropriately to satisfy TypeScript
src/main/webapp/app/shared/metis/posting-content/posting-content.component.html (2)
Line range hint
11-26: LGTM! Preview mode content display follows Angular's new syntax.The template correctly:
- Uses
@ifand@forcontrol flow syntax- Updates property access to method calls (e.g.,
previewMode(),isEdited())- Maintains proper component structure
Line range hint
34-46: LGTM! Non-preview mode content display is well-structured.The template:
- Correctly uses method calls for inputs
- Properly binds data to child components
- Maintains consistent structure with preview mode
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.component.html (3)
1-7: LGTM! Content display with proper null safety.The template correctly:
- Uses optional chaining with
postingContentPart()?- Safely handles content display
- Properly processes markdown content
8-12: LGTM! Link reference handling is robust.The template:
- Properly handles link references with optional chaining
- Correctly binds router parameters
- Maintains consistent icon display
Line range hint
54-75: LGTM! Image reference handling is comprehensive.The template:
- Properly handles image references with optional chaining
- Includes error handling for missing images
- Maintains consistent styling and layout
| if (this.postingContentPart()?.contentBeforeReference) { | ||
| this.processedContentBeforeReference = this.escapeNumberedList(this.postingContentPart()?.contentBeforeReference || ''); | ||
| this.processedContentBeforeReference = this.escapeUnorderedList(this.processedContentBeforeReference); | ||
| } | ||
|
|
||
| if (this.postingContentPart.contentAfterReference) { | ||
| this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart.contentAfterReference); | ||
| if (this.postingContentPart()?.contentAfterReference) { | ||
| this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart()?.contentAfterReference || ''); | ||
| this.processedContentAfterReference = this.escapeUnorderedList(this.processedContentAfterReference); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for undefined content.
The method should handle cases where content is undefined more gracefully to prevent potential runtime errors.
processContent() {
+ const content = this.postingContentPart();
+ if (!content) {
+ this.processedContentBeforeReference = '';
+ this.processedContentAfterReference = '';
+ return;
+ }
- if (this.postingContentPart()?.contentBeforeReference) {
+ if (content.contentBeforeReference) {
- this.processedContentBeforeReference = this.escapeNumberedList(this.postingContentPart()?.contentBeforeReference || '');
+ this.processedContentBeforeReference = this.escapeNumberedList(content.contentBeforeReference);
this.processedContentBeforeReference = this.escapeUnorderedList(this.processedContentBeforeReference);
}
- if (this.postingContentPart()?.contentAfterReference) {
+ if (content.contentAfterReference) {
- this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart()?.contentAfterReference || '');
+ this.processedContentAfterReference = this.escapeNumberedList(content.contentAfterReference);
this.processedContentAfterReference = this.escapeUnorderedList(this.processedContentAfterReference);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this.postingContentPart()?.contentBeforeReference) { | |
| this.processedContentBeforeReference = this.escapeNumberedList(this.postingContentPart()?.contentBeforeReference || ''); | |
| this.processedContentBeforeReference = this.escapeUnorderedList(this.processedContentBeforeReference); | |
| } | |
| if (this.postingContentPart.contentAfterReference) { | |
| this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart.contentAfterReference); | |
| if (this.postingContentPart()?.contentAfterReference) { | |
| this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart()?.contentAfterReference || ''); | |
| this.processedContentAfterReference = this.escapeUnorderedList(this.processedContentAfterReference); | |
| } | |
| const content = this.postingContentPart(); | |
| if (!content) { | |
| this.processedContentBeforeReference = ''; | |
| this.processedContentAfterReference = ''; | |
| return; | |
| } | |
| if (content.contentBeforeReference) { | |
| this.processedContentBeforeReference = this.escapeNumberedList(content.contentBeforeReference); | |
| this.processedContentBeforeReference = this.escapeUnorderedList(this.processedContentBeforeReference); | |
| } | |
| if (content.contentAfterReference) { | |
| this.processedContentAfterReference = this.escapeNumberedList(content.contentAfterReference); | |
| this.processedContentAfterReference = this.escapeUnorderedList(this.processedContentAfterReference); | |
| } |
| referenceStr = this.content()!.substring(this.content()!.indexOf(']', patternMatch.startIndex)! + 1, this.content()!.indexOf('(', patternMatch.startIndex)!); | ||
| const channelId = parseInt( | ||
| this.content()!.substring(this.content()!.indexOf('(', patternMatch.startIndex)! + 1, this.content()!.indexOf(')', patternMatch.startIndex)), | ||
| ); | ||
| queryParams = { | ||
| channelId: isNaN(channelId) ? undefined : channelId, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace unsafe isNaN with Number.isNaN.
The global isNaN function performs type coercion which can lead to unexpected results. Use Number.isNaN for safer number validation.
- isNaN(channelId) ? undefined : channelId,
+ Number.isNaN(channelId) ? undefined : channelId,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| channelId: isNaN(channelId) ? undefined : channelId, | |
| channelId: Number.isNaN(channelId) ? undefined : channelId, |
🧰 Tools
🪛 Biome (1.9.4)
[error] 186-186: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| } | ||
| return patternMatches; | ||
| } | ||
|
|
||
| contentPartTrack(index: number) { | ||
| return this.posting?.id + '_' + index; | ||
| return this.posting()?.id + '_' + index; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve track function stability.
The track function should always return a stable identifier. The current implementation might return undefined, which could cause Angular change detection issues.
- return this.posting()?.id + '_' + index;
+ const postId = this.posting()?.id ?? 'unknown';
+ return `${postId}_${index}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return this.posting()?.id + '_' + index; | |
| const postId = this.posting()?.id ?? 'unknown'; | |
| return `${postId}_${index}`; |
Checklist
General
Client
Motivation and Context
Communication: "Deleted" label incorrectly appears in message header after editing a message #9987)Description
PostingContentComponentandPostingContentPartComponenthave been updated to use signals.PostingHeaderComponenthave been removed.authorRoleis properly assigned.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
Client
Summary by CodeRabbit
Refactor
@Input()and@Output()decorators withinput()andoutput()functionsImprovements
Testing