Communication: Migrate conversation components to use signals
#10223
Conversation
…n/migrate-components-to-signals
PaRangger
added
client
github-actions
bot
added
tests
and removed
communication
github-actions
bot
added
deployment-error
PaRangger
added
deploy:artemis-test2
and removed
deployment-error
PaRangger
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
PaRangger
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThis pull request introduces a significant refactoring of Angular components across multiple files, focusing on transitioning from traditional decorator-based property declarations to a more functional approach using Changes
Suggested labels
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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: 4
🔭 Outside diff range comments (4)
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts (1)
Line range hint
164-172: Migrate remaining tests from runInInjectionContext to setInput.Some tests still use runInInjectionContext while others have been migrated to use setInput.
For consistency, migrate these tests to use setInput:
-runInInjectionContext(injector, () => { - post.answers = unsortedAnswerArray; - component.posting = input<Posting>(post); - metisServiceUserAuthorityStub.mockReturnValue(false); - component.ngOnInit(); - expect(component.isAtLeastTutorInCourse).toBeFalse(); - expect(component.createdAnswerPost.resolvesPost).toBeFalse(); -}); +post.answers = unsortedAnswerArray; +fixture.componentRef.setInput('posting', post); +metisServiceUserAuthorityStub.mockReturnValue(false); +component.ngOnInit(); +fixture.detectChanges(); +expect(component.isAtLeastTutorInCourse).toBeFalse(); +expect(component.createdAnswerPost.resolvesPost).toBeFalse();Also applies to: 244-250
🧰 Tools
🪛 GitHub Actions: Test
[error] Coverage threshold for branches (74.47%) not met: 74.46%
src/test/javascript/spec/component/overview/course-conversations/course-wide-search.component.spec.ts (1)
Line range hint
161-188: Add test cases to improve branch coverage.The pipeline indicates that branch coverage is slightly below the threshold (74.46% vs 74.47%). Consider adding test cases for edge conditions in the fetchNextPage method.
+ it('should not fetch posts when total number of posts is less than current posts', fakeAsync(() => { + const getFilteredPostSpy = jest.spyOn(metisService, 'getFilteredPosts'); + fixture.componentRef.setInput('courseWideSearchConfig', courseWideSearchConfig); + component.totalNumberOfPosts = 1; + component.posts = [examplePost]; + component.fetchNextPage(); + expect(getFilteredPostSpy).not.toHaveBeenCalled(); + }));🧰 Tools
🪛 GitHub Actions: Test
[error] Coverage threshold for branches (74.47%) not met: 74.46%
src/main/webapp/app/shared/metis/post/post.component.html (1)
Line range hint
1-216: **Migrate remaining ngIf and ngFor directives to new syntax.While most of the file uses the new
@ifsyntax, there are still instances of*ngIfand*ngFor. According to the coding guidelines,@ifand@forshould always be used over the old style.For example:
- [ngClass]="{ 'mx-0': !isThreadSidebar, 'pinned-message': isPinned(), 'is-saved': !isConsecutive() && posting.isSaved, 'non-consecutive': !isConsecutive() }" + [class]="{ 'mx-0': !isThreadSidebar, 'pinned-message': isPinned(), 'is-saved': !isConsecutive() && posting.isSaved, 'non-consecutive': !isConsecutive() }"src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (1)
Line range hint
1-424: Add missing test coverage for signals.The pipeline shows that branch coverage is slightly below the threshold (74.46% vs required 74.47%). Add tests for the new signal-based properties:
- Add tests for input signal initialization:
it('should initialize input signals with default values', () => { expect(component.lastReadDate()).toBeUndefined(); expect(component.readOnlyMode()).toBeFalse(); expect(component.previewMode()).toBeFalse(); expect(component.modalRef()).toBeUndefined(); });
- Add tests for model signal:
it('should handle showAnswers model signal', () => { expect(component.showAnswers()).toBeFalse(); component.showAnswers.set(true); expect(component.showAnswers()).toBeTrue(); });
- Add tests for viewChild signals:
it('should properly initialize viewChild signals', () => { fixture.detectChanges(); expect(component.postFooterComponent()).toBeDefined(); expect(component.reactionsBarComponent()).toBeDefined(); });🧰 Tools
🪛 GitHub Actions: Test
[error] Coverage threshold for branches (74.47%) not met: 74.46%
🧹 Nitpick comments (6)
src/test/javascript/spec/component/shared/metis/postings-thread/postings-thread.component.spec.ts (2)
25-27: Improve documentation of the ng-mocks workaround.The comment should explain why this workaround is necessary and how it works, not just reference an issue.
Consider adding a more detailed comment:
-// Workaround for mocked components with viewChild: https://github.com/help-me-mom/ng-mocks/issues/8634 +// When mocking components with @ViewChild that use signals, we need to initialize the signals +// to prevent "signal not found" errors during testing. +// See: https://github.com/help-me-mom/ng-mocks/issues/8634🧰 Tools
🪛 GitHub Actions: Test
[error] Coverage threshold for branches (74.47%) not met: 74.46%
27-27: Improve type safety of the signal initialization.The current cast to
PostReactionsBarComponentbypasses type checking.Consider creating a minimal mock implementation:
-MockInstance(PostComponent, 'reactionsBarComponent', signal({} as unknown as PostReactionsBarComponent)); +const mockReactionsBar = { + // Add minimal required properties +} satisfies Partial<PostReactionsBarComponent>; +MockInstance(PostComponent, 'reactionsBarComponent', signal(mockReactionsBar));🧰 Tools
🪛 GitHub Actions: Test
[error] Coverage threshold for branches (74.47%) not met: 74.46%
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts (1)
90-91: Use more specific jest matchers.According to the coding guidelines, use more specific matchers for better error messages:
-expect(component.groupedAnswerPosts.length).toBeGreaterThan(0); +expect(component.groupedAnswerPosts).not.toHaveLength(0); -expect(changeDetectorSpy).toHaveBeenCalled(); +expect(changeDetectorSpy).toHaveBeenCalledOnce(); -expect(createAnswerPostModalOpen).toHaveBeenCalledOnce(); +expect(createAnswerPostModalOpen).toHaveBeenCalledExactlyOnceWith(); -expect(group1.posts).toContainEqual(expect.objectContaining({ id: post1.id })); +expect(group1.posts).toContainEntries([[0, expect.objectContaining({ id: post1.id })]]);Also applies to: 103-106, 182-184, 197-199, 222-240
🧰 Tools
🪛 GitHub Actions: Test
[error] Coverage threshold for branches (74.47%) not met: 74.46%
src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.ts (1)
142-153: Consider adding error handling for undefined searchConfig.While the null check is present, consider adding error logging for debugging purposes when searchConfig is undefined.
const searchConfig = this.courseWideSearchConfig(); -if (!searchConfig) return; +if (!searchConfig) { + console.warn('Search config is undefined'); + return; +}src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (1)
598-598: Consider using optional chaining for safer element access.While the non-null assertion works, it could lead to runtime errors if the element doesn't exist. Consider using optional chaining for safer access:
-this.searchElement()!.nativeElement.focus(); +this.searchElement()?.nativeElement?.focus();src/main/webapp/app/shared/metis/post/post.component.ts (1)
235-235: Fix potential null reference issues.The optional chaining operator is used correctly with function calls, but consider adding null checks for better defensive programming.
- this.modalRef()?.dismiss(); + const modal = this.modalRef(); + if (modal) { + modal.dismiss(); + } - this.postFooterComponent()?.openCreateAnswerPostModal(); + const footer = this.postFooterComponent(); + if (footer) { + footer.openCreateAnswerPostModal(); + } - this.postFooterComponent()?.closeCreateAnswerPostModal(); + const footer = this.postFooterComponent(); + if (footer) { + footer.closeCreateAnswerPostModal(); + }Also applies to: 244-244, 251-251
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/main/webapp/app/overview/course-conversations/code-of-conduct/course-conversations-code-of-conduct.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/code-of-conduct/course-conversations-code-of-conduct.component.ts(2 hunks)src/main/webapp/app/overview/course-conversations/course-conversations.component.ts(4 hunks)src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.ts(8 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.html(4 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts(3 hunks)src/main/webapp/app/shared/metis/post/post.component.html(8 hunks)src/main/webapp/app/shared/metis/post/post.component.ts(5 hunks)src/test/javascript/spec/component/overview/course-conversations/course-conversations-code-of-conduct.component.spec.ts(1 hunks)src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts(3 hunks)src/test/javascript/spec/component/overview/course-conversations/course-wide-search.component.spec.ts(8 hunks)src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.spec.ts(0 hunks)src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts(3 hunks)src/test/javascript/spec/component/shared/metis/postings-thread/postings-thread.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (15)
src/main/webapp/app/overview/course-conversations/code-of-conduct/course-conversations-code-of-conduct.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/overview/course-conversations/course-conversations-code-of-conduct.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/post/post.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/overview/course-conversations/course-wide-search/course-wide-search.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/overview/course-conversations/code-of-conduct/course-conversations-code-of-conduct.component.ts (1)
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.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/course-conversations/course-wide-search.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/course-conversations/course-conversations.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/overview/course-conversations/course-conversations.component.ts (1)
src/test/javascript/spec/component/shared/metis/postings-thread/postings-thread.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/post/post.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/answer-post/answer-post.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/post/post.component.ts (1)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (1)
src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.ts (1)
🪛 GitHub Actions: Test
src/test/javascript/spec/component/overview/course-conversations/course-conversations-code-of-conduct.component.spec.ts
[error] Coverage threshold for branches (74.47%) not met: 74.46%
src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts
[error] Coverage threshold for branches (74.47%) not met: 74.46%
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts
[error] Coverage threshold for branches (74.47%) not met: 74.46%
src/test/javascript/spec/component/overview/course-conversations/course-wide-search.component.spec.ts
[error] Coverage threshold for branches (74.47%) not met: 74.46%
src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts
[error] Coverage threshold for branches (74.47%) not met: 74.46%
src/test/javascript/spec/component/shared/metis/postings-thread/postings-thread.component.spec.ts
[error] Coverage threshold for branches (74.47%) not met: 74.46%
🔇 Additional comments (30)
src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (2)
6-6: Good use of ng-mocks for component mocking!The addition of
MockInstancefrom ng-mocks and the setup of mock signals forcontentandmessagesproperties follows best practices for Angular component testing.Also applies to: 47-47, 86-88
🧰 Tools
🪛 GitHub Actions: Test
[error] Coverage threshold for branches (74.47%) not met: 74.46%
Line range hint
1-500: Increase branch coverage to meet the pipeline threshold.The pipeline indicates that branch coverage (74.46%) is slightly below the required threshold (74.47%). Consider adding test cases for the following scenarios to improve coverage:
- Error handling paths in dialog opening tests
- Edge cases in query parameter handling
- Boundary conditions in conversation selection
Let's analyze the untested branches:
🧰 Tools
🪛 GitHub Actions: Test
[error] Coverage threshold for branches (74.47%) not met: 74.46%
src/test/javascript/spec/component/overview/course-conversations/course-conversations-code-of-conduct.component.spec.ts (1)
Line range hint
32-35: Improve test coverage by adding more test cases.The current test suite only verifies component initialization. Consider adding test cases for:
- Component behavior when course input changes
- Error scenarios
- Component interactions
This could help address the coverage threshold failure (74.46% vs required 74.47%).
🧰 Tools
🪛 GitHub Actions: Test
[error] Coverage threshold for branches (74.47%) not met: 74.46%
src/main/webapp/app/overview/course-conversations/code-of-conduct/course-conversations-code-of-conduct.component.ts (2)
20-20: LGTM! Clean migration to input() signal.The migration from @input decorator to input() signal follows Angular's new input API guidelines.
Line range hint
25-34: Verify null handling in the HTTP call.The code uses optional chaining for the initial check but non-null assertions for the HTTP call. While this works, consider adding error handling for the HTTP call to improve robustness.
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (3)
67-70: LGTM! Well-structured signal migrations with proper initialization.The input properties are correctly migrated with appropriate default values.
72-74: LGTM! Clean output signal declarations.The output signals are properly declared using the output() function.
76-77: LGTM! Proper use of viewChild with type safety.The viewChild declarations use proper type annotations and the required flag where necessary.
src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.ts (1)
32-34: LGTM! Proper use of viewChildren with toObservable.Good use of toObservable for reactive handling of viewChildren changes.
src/test/javascript/spec/component/overview/course-conversations/course-wide-search.component.spec.ts (1)
96-96: LGTM! Proper test setup for input signals.Correctly using setInput for initializing the signal in tests.
🧰 Tools
🪛 GitHub Actions: Test
[error] Coverage threshold for branches (74.47%) not met: 74.46%
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (2)
174-175: LGTM! Good migration to signal-based viewChild.The transition from decorator-based
@ViewChildto signal-basedviewChildaligns with modern Angular practices and should improve performance through better change detection.
421-421: LGTM! Proper signal access with null safety.The optional chaining operator ensures safe access to the CourseWideSearchComponent instance when using the signal-based viewChild.
src/main/webapp/app/overview/course-conversations/code-of-conduct/course-conversations-code-of-conduct.component.html (1)
2-2: LGTM! Safe signal access with fallback.The optional chaining with empty string fallback ensures safe rendering of the code of conduct content when using signal-based input.
src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.html (2)
1-5: LGTM! Consistent signal usage in template bindings.The changes correctly implement signal-based property access with appropriate null safety checks in the template bindings.
9-15: LGTM! Proper signal usage in template logic.The changes correctly implement signal-based property access in both conditional statements and translation value bindings.
src/main/webapp/app/shared/metis/answer-post/answer-post.component.html (4)
23-25: LGTM! Proper signal binding to child component.The changes correctly implement signal-based property access for input bindings to the posting-header component.
48-50: LGTM! Consistent signal usage in reactions bar.The changes correctly implement signal-based property access for input bindings to the first instance of answer-post-reactions-bar.
69-71: LGTM! Consistent signal usage across component instances.The changes maintain consistent signal-based property access across multiple instances of the answer-post-reactions-bar component.
81-81: LGTM! Proper signal binding for modal component.The change correctly implements signal-based property access for the containerRef input binding to the answer-post-create-edit-modal component.
src/main/webapp/app/shared/metis/post/post.component.html (8)
21-22: LGTM! Properties correctly migrated to signal syntax.The properties
previewMode,readOnlyMode, andlastReadDatehave been properly converted to use function calls, aligning with Angular's signal-based reactivity.Also applies to: 28-28
45-45: LGTM! Condition correctly uses signal syntax.The condition properly uses
previewMode()function call in the template expression.
80-80: LGTM! Consistent usage of signal syntax.The
previewModeproperty is consistently used as a function call in both property binding and class binding contexts.Also applies to: 92-92
96-98: LGTM! Properties correctly migrated in reactions bar.The properties
lastReadDate,readOnlyMode, andpreviewModehave been properly converted to use function calls in the reactions bar component.
126-132: LGTM! Consistent signal usage in post reactions bar.The properties
previewMode,lastReadDate, andreadOnlyModeare consistently using function calls in the post reactions bar section.
152-154: LGTM! Footer properties correctly use signal syntax.All properties in the footer section (
readOnlyMode,modalRef,previewMode,showAnswers,lastReadDate) have been properly converted to use function calls.Also applies to: 156-156, 161-161
192-192: LGTM! Proper signal usage with optional chaining.The
reactionsBarComponentis correctly accessed as a function with optional chaining operator.
213-213: LGTM! Proper signal usage in emoji picker condition.The
readOnlyModeproperty is correctly accessed as a function in the emoji picker condition.src/main/webapp/app/shared/metis/post/post.component.ts (3)
12-14: LGTM! Migration to signals.The imports for
model,input,output, andviewChildalign with Angular's new signals API, improving reactivity and performance.
78-84: LGTM! Clean signal declarations.The migration from decorators to signals is well-implemented:
- Input properties are correctly initialized with default values
- The model signal for
showAnswersis appropriately used for two-way binding
86-89: Verify child component access patterns.The ViewChild declarations look correct, but we should ensure consistent access patterns throughout the codebase.
✅ Verification successful
ViewChild declarations follow the new standardized pattern
The code uses the new
viewChild<T>()function syntax which is consistently applied across the codebase, including similar usages in conversation components, form components, and exercise components. The required modifier is appropriately used for mandatory child components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other ViewChild usages to ensure consistent migration ast-grep --pattern 'ViewChild($$$)'Length of output: 41013
Script:
#!/bin/bash # Search for documentation about viewChild function and similar usage patterns rg -g '*.{ts,md}' -i "viewChild.*function|migrat.*viewChild" # Search for similar viewChild function usage ast-grep --pattern 'viewChild<$_>($$$)'Length of output: 2947
...test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts
Show resolved
Hide resolved
PaRangger
dismissed stale reviews from SindiBuklaji, coderabbitai[bot], zagemello, Cathy0123456789, and vinceclifford
via
c1161bf
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/webapp/app/overview/course-conversations/code-of-conduct/course-conversations-code-of-conduct.component.ts (1)
25-26: Simplify the null checks.The double null check with
if (this.course().id)followed bythis.course().id!can be simplified sinceinput.requiredguarantees thatcourseis defined.Apply this diff to simplify the null checks:
- if (this.course().id) { - this.conversationService.getResponsibleUsersForCodeOfConduct(this.course().id!).subscribe({ + const courseId = this.course().id; + if (courseId) { + this.conversationService.getResponsibleUsersForCodeOfConduct(courseId).subscribe({src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.ts (2)
103-104: Simplify content() null checks.Multiple null checks for
content()can be simplified by extracting the element reference.Apply this pattern to simplify the null checks:
setPosts(posts: Post[]): void { - if (this.content()) { - this.previousScrollDistanceFromTop = this.content()!.nativeElement.scrollHeight - this.content()!.nativeElement.scrollTop; + const container = this.content(); + if (container) { + this.previousScrollDistanceFromTop = container.nativeElement.scrollHeight - container.nativeElement.scrollTop; } this.posts = posts.slice().reverse(); }Also applies to: 110-113, 119-120, 129-130
142-153: Optimize configuration handling.The configuration access and property updates can be optimized by destructuring and using a more functional approach.
Apply this pattern to optimize the configuration handling:
private refreshMetisConversationPostContextFilter(): void { const searchConfig = this.courseWideSearchConfig(); if (!searchConfig) return; + const { searchTerm, filterToUnresolved, filterToOwn, filterToAnsweredOrReacted, sortingOrder } = searchConfig; this.currentPostContextFilter = { courseId: this.course?.id, - searchText: searchConfig.searchTerm ? searchConfig.searchTerm.trim() : undefined, + searchText: searchTerm?.trim(), postSortCriterion: PostSortCriterion.CREATION_DATE, - filterToUnresolved: searchConfig.filterToUnresolved, - filterToOwn: searchConfig.filterToOwn, - filterToAnsweredOrReacted: searchConfig.filterToAnsweredOrReacted, - sortingOrder: searchConfig.sortingOrder, + filterToUnresolved, + filterToOwn, + filterToAnsweredOrReacted, + sortingOrder, pagingEnabled: true, page: this.page - 1, pageSize: 50, }; }Also applies to: 197-202
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/webapp/app/overview/course-conversations/code-of-conduct/course-conversations-code-of-conduct.component.ts(2 hunks)src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.html
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/overview/course-conversations/code-of-conduct/course-conversations-code-of-conduct.component.ts (1)
src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
src/main/webapp/app/overview/course-conversations/code-of-conduct/course-conversations-code-of-conduct.component.ts (1)
20-20: LGTM! Good use of input.required.The use of
input.required<Course>()is a good practice as it enforces the requirement of a Course object and aligns with Angular's new functional approach.src/main/webapp/app/overview/course-conversations/course-wide-search/course-wide-search.component.ts (1)
30-50: LGTM! Excellent use of Angular's functional approach and immutability.The changes demonstrate good practices:
- Using
input.requiredfor required inputs- Using
viewChildrenandviewChildfor DOM references- Using
outputfor event emitters- Marking properties as
readonlyfor immutability
…n/migrate-components-to-signals
PaRangger
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (1)
100-102: Add return type annotation to the getter.Consider adding a return type annotation to the getter for better maintainability and type safety:
- get reactionsBar() { + get reactionsBar(): PostingReactionsBarComponent<AnswerPost> | undefined { return this.reactionsBarComponent(); }src/main/webapp/app/shared/metis/post/post.component.ts (4)
84-84: Consider documenting the model usage.The use of
model()forshowAnswersindicates two-way binding. Consider adding a comment explaining why this property needs two-way binding versus a regular input/output pair, as this pattern should be used sparingly.
81-83: Improve the modalRef comment.The current comment could be more descriptive. Consider:
- // if the post is previewed in the create/edit modal, - // we need to pass the ref in order to close it when navigating to the previewed post via post title + /** + * Reference to the modal when post is previewed in create/edit mode. + * Required to close the modal when navigating to the previewed post via post title. + */
Line range hint
234-245: Add error handling for modal and footer operations.Consider adding error handling for cases where the modal reference or footer component is undefined:
onNavigateToContext($event: MouseEvent) { if (!$event.metaKey) { - this.modalRef()?.dismiss(); + const modal = this.modalRef(); + if (!modal) { + console.warn('Modal reference is undefined'); + return; + } + modal.dismiss(); this.metisConversationService.setActiveConversation(this.contextInformation.queryParams!['conversationId']); } }
122-124: Add return type annotation to the getter.Consider adding a return type annotation to the getter for better maintainability and type safety:
- get reactionsBar() { + get reactionsBar(): PostingReactionsBarComponent<Post> { return this.reactionsBarComponent(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/webapp/app/overview/course-conversations/code-of-conduct/course-conversations-code-of-conduct.component.html(1 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.html(4 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts(3 hunks)src/main/webapp/app/shared/metis/post/post.component.html(8 hunks)src/main/webapp/app/shared/metis/post/post.component.ts(6 hunks)src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/webapp/app/overview/course-conversations/code-of-conduct/course-conversations-code-of-conduct.component.html
- src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts
- src/main/webapp/app/shared/metis/answer-post/answer-post.component.html
- src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts
- src/main/webapp/app/shared/metis/post/post.component.html
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/shared/metis/post/post.component.ts (1)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (1)
🔇 Additional comments (1)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (1)
67-70: Consider usinginput.required()for mandatory inputs.For inputs that are required for the component to function correctly, consider using
input.required()instead ofinput()with a default value. This makes the contract more explicit and helps catch missing inputs at runtime.
PaRangger
requested review from
SimonEntholzer,
zagemello,
sawys777,
SindiBuklaji,
HawKhiem,
vinceclifford and
Cathy0123456789
Checklist
General
Client
Motivation and Context
This PR is part of the effort to migrate the communication module to use signals instead of decorators. A list of those components can be found on confluence.
Description
I changed the
conversations-code-of-conduct.component.ts,course-conversations.component.ts,course-wide-search.component.ts,post.component.tsandanswer-post.component.tsto use signals instead of decorators.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
Release Notes
New Features
Technical Improvements
Testing Enhancements
Performance
These changes represent a significant refactoring of the application's component architecture, focusing on modernizing the codebase and improving type safety.