Development: Migrate translate/markdown pipes and loading indicator components to standalone
#9880
Conversation
PaRangger
added
client
github-actions
bot
added
tests
and removed
core
PaRangger
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request introduces several modifications primarily focused on improving the structure and functionality of Angular components and pipes. Key changes include transitioning components to standalone status, updating method signatures, and modifying how loading states are managed. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 and nitpick comments (10)
src/main/webapp/app/shared/pipes/artemis-translate.pipe.ts (1)
15-16: Enhance the documentation about ChangeDetectorRef injectionWhile the code changes are correct, the comment could be more descriptive about why ChangeDetectorRef can't be injected and what the current workaround achieves.
Consider expanding the comment:
- // We cannot inject() ChangeDetectorRef here + // Note: We cannot use inject(ChangeDetectorRef) here as it's not available + // in the context where TranslatePipe is instantiated. Instead, we receive it + // through constructor injection.src/main/webapp/app/shared/pipes/html-for-markdown.pipe.ts (1)
Line range hint
13-21: Consider enhancing JSDoc with version informationWhile the documentation is comprehensive, consider adding a
@sincetag to indicate when this API was last modified. This helps track API changes across versions.Example enhancement:
/** * Converts markdown into html, sanitizes it and then declares it as safe to bypass further security. + * @since 5.0.0 - Migrated to standalone pipe * @param {string} markdown the original markdown text * @param {PluginSimple[]} extensions to use for markdown parsing * @param {string[]} allowedHtmlTags to allow during sanitization * @param {string[]} allowedHtmlAttributes to allow during sanitization * @returns {string} the resulting html as a SafeHtml object that can be inserted into the angular template */src/main/webapp/app/shared/pipes/html-for-posting-markdown.pipe.ts (1)
9-11: Consider leveraging type inference for injected serviceThe dependency injection implementation is clean and follows best practices. For better type safety, consider letting TypeScript infer the type:
-private readonly markdownService = inject(ArtemisMarkdownService); +private readonly markdownService = inject(ArtemisMarkdownService) as const;src/main/webapp/app/shared/shared-common.module.ts (1)
18-18: Consider restructuring imports for better organizationThe imports array mixes a module (
ArtemisSharedLibsModule) with standalone components (TranslateDirective,ArtemisTranslatePipe). Consider grouping them separately for better maintainability.- imports: [ArtemisSharedLibsModule, TranslateDirective, ArtemisTranslatePipe], + imports: [ + // Modules + ArtemisSharedLibsModule, + // Standalone Components + TranslateDirective, + ArtemisTranslatePipe, + ],src/main/webapp/app/shared/shared.module.ts (1)
Line range hint
34-83: Consider migrating additional components to standalone.Given the project's initiative to migrate to standalone components, consider evaluating other components in the declarations array for potential standalone migration, particularly:
- CircularProgressBarComponent
- SecuredImageComponent
- DeleteDialogComponent
- ResizeableContainerComponent
These components appear to be good candidates for standalone migration based on their names suggesting focused, single-responsibility components.
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts (1)
Line range hint
15-24: Consider using more specific spy assertionsWhile the test setup is good, consider using more specific spy assertions as per our guidelines:
- expect(updateReactingUsersStringSpy).toHaveBeenCalledOnce(); + expect(updateReactingUsersStringSpy).toHaveBeenCalledExactlyOnceWith(); - expect(updateReactingUsersStringSpy).toHaveBeenCalledTimes(2); + expect(updateReactingUsersStringSpy).toHaveBeenCalledTimes(2); + expect(updateReactingUsersStringSpy).toHaveBeenNthCalledWith(1, expect.any(Array)); + expect(updateReactingUsersStringSpy).toHaveBeenNthCalledWith(2, expect.any(Array));src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts (2)
34-39: Consider mocking FaIconComponent for better test isolationWhile the comment explains why FaIconComponent isn't mocked, consider using a lightweight mock that still allows icon type verification. This would improve test isolation and performance.
imports: [ ArtemisTestModule, MatDialogModule, MatMenuModule, HtmlForPostingMarkdownPipe, + MockComponent(FaIconComponent), ], -// FaIconComponent, // we want to test the type of rendered icons, therefore we cannot mock the component
60-62: Enhance spy assertions for better specificityAccording to coding guidelines, consider using more specific spy assertions:
- Replace
toHaveBeenCalledOnce()withtoHaveBeenCalledExactlyOnceWith()- Replace
not.toHaveBeenCalled()withnot.toHaveBeenCalled()Example usage in test cases:
expect(navigateByUrlSpy).toHaveBeenCalledExactlyOnceWith('/expected/url'); expect(openAttachmentSpy).toHaveBeenCalledExactlyOnceWith(expectedAttachmentURL);src/test/javascript/spec/component/assessment-shared/assessment-header.component.spec.ts (2)
52-53: LGTM with suggestions for further improvements.The TestBed configuration correctly uses MockPipe for ArtemisTranslatePipe, which aligns with the migration to standalone components. However, consider these improvements:
- Mock other dependencies that aren't directly related to the test scenarios
- Consider using
MockComponentforAssessmentWarningComponentandAlertOverlayComponentExample refactor:
imports: [ArtemisTestModule, NgbTooltipMocksModule, NgbAlertsMocksModule, RouterModule.forRoot([]), MockPipe(ArtemisTranslatePipe)], -declarations: [AssessmentHeaderComponent, AssessmentWarningComponent, AlertOverlayComponent, MockTranslateValuesDirective], +declarations: [ + AssessmentHeaderComponent, + MockComponent(AssessmentWarningComponent), + MockComponent(AlertOverlayComponent), + MockTranslateValuesDirective +],
Line range hint
86-86: Enhance test assertions with more specific matchers.According to the coding guidelines for test specificity, replace generic boolean assertions with more specific jest matchers:
- Component creation test:
-expect(component).toBeTruthy(); +expect(component).toBeDefined();
- Element query tests:
-expect(warningComponent).toBeTruthy(); +expect(warningComponent).not.toBeNull(); -expect(container).toBeTruthy(); +expect(container).not.toBeNull(); -expect(assessmentLocked).toBeFalsy(); +expect(assessmentLocked).toBeNull();Also applies to: 95-97, 117-119, 134-136, 144-146, 156-158, 182-184, 200-202
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
src/main/webapp/app/shared/loading-indicator-container/loading-indicator-container.component.html(1 hunks)src/main/webapp/app/shared/loading-indicator-container/loading-indicator-container.component.ts(1 hunks)src/main/webapp/app/shared/markdown.module.ts(1 hunks)src/main/webapp/app/shared/metis/metis.module.ts(1 hunks)src/main/webapp/app/shared/pipes/artemis-translate.pipe.ts(1 hunks)src/main/webapp/app/shared/pipes/html-for-markdown.pipe.ts(1 hunks)src/main/webapp/app/shared/pipes/html-for-posting-markdown.pipe.ts(1 hunks)src/main/webapp/app/shared/shared-common.module.ts(1 hunks)src/main/webapp/app/shared/shared.module.ts(1 hunks)src/test/javascript/spec/component/assessment-shared/assessment-header.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/pipe/reacting-users-on-posting.pipe.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
src/main/webapp/app/shared/loading-indicator-container/loading-indicator-container.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/loading-indicator-container/loading-indicator-container.component.ts (1)
src/main/webapp/app/shared/markdown.module.ts (1)
src/main/webapp/app/shared/metis/metis.module.ts (1)
src/main/webapp/app/shared/pipes/artemis-translate.pipe.ts (1)
src/main/webapp/app/shared/pipes/html-for-markdown.pipe.ts (1)
src/main/webapp/app/shared/pipes/html-for-posting-markdown.pipe.ts (1)
src/main/webapp/app/shared/shared-common.module.ts (1)
src/main/webapp/app/shared/shared.module.ts (1)
src/test/javascript/spec/component/assessment-shared/assessment-header.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}}
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}}
🔇 Additional comments (19)
src/main/webapp/app/shared/loading-indicator-container/loading-indicator-container.component.html (2)
1-1: Great use of the new Angular control flow syntax!
The template correctly implements the new @if syntax instead of the older *ngIf, which aligns with our coding guidelines.
Also applies to: 7-7
Line range hint 1-9: Verify loading state function implementation
The template now calls isLoading() as a function instead of using it as a property. This change suggests a shift in the component's implementation.
Let's verify the implementation:
✅ Verification successful
The loading state implementation has been correctly updated to use Angular signals
The verification confirms that:
- The
LoadingIndicatorContainerComponentcorrectly implementsisLoadingas a signal usinginput<boolean>(false) - The template's usage of
isLoading()aligns with the signal-based implementation - Other components using
@Input() isLoadingare unrelated to this component and don't need modifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the loading state implementation in the component file
# Expected: Should find the isLoading function implementation using input signal
# Search for the isLoading implementation in the component
ast-grep --pattern 'class LoadingIndicatorContainerComponent {
$$$
isLoading = input<boolean>($$$)
$$$
}'
# Also check for any remaining uses of the old @Input decorator for isLoading
rg "@Input\(\)\s+isLoading"
Length of output: 2075
src/main/webapp/app/shared/loading-indicator-container/loading-indicator-container.component.ts (2)
1-2: LGTM! Proper migration to Angular signals.
The imports correctly reflect the migration from @Input decorator to the new signals-based input function, and the TranslateDirective import follows Angular's style guide.
8-9: Verify template dependencies for standalone component.
The standalone configuration and TranslateDirective import look correct. However, let's verify if the template requires any additional dependencies.
✅ Verification successful
Component dependencies are correctly configured
The template uses:
@if/@elsecontrol flow (built into Angular 17+, no import needed)jhiTranslatedirective (covered by TranslateDirective import)ng-content(built-in, no import needed)
The current imports configuration is complete and correct for this standalone component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check template for any missing dependencies that need to be imported
# Check the template file for Angular directives/components usage
cat src/main/webapp/app/shared/loading-indicator-container/loading-indicator-container.component.html | grep -E '*ngIf|*ngFor|[(]|[[]' || echo "No additional Angular directives found"
Length of output: 236
Script:
#!/bin/bash
# Let's check the full template content to understand all dependencies
cat src/main/webapp/app/shared/loading-indicator-container/loading-indicator-container.component.html
# Also check the component file for any signal or function declarations
cat src/main/webapp/app/shared/loading-indicator-container/loading-indicator-container.component.ts
Length of output: 951
src/main/webapp/app/shared/pipes/artemis-translate.pipe.ts (1)
7-7: LGTM! Verify module imports for standalone pipe
The migration to a standalone pipe aligns with Angular's recommended architecture.
Let's verify the module imports have been updated correctly:
✅ Verification successful
Based on the search results, I can now generate the final response:
Standalone pipe migration is correctly implemented
The pipe is correctly migrated to standalone mode and all its usages in test files have been updated to use MockPipe(ArtemisTranslatePipe). The pipe is properly imported in shared modules (shared-common.module.ts and shared-pipes.module.ts) where it's needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining module declarations of ArtemisTranslatePipe
rg "ArtemisTranslatePipe.*declarations|declarations.*ArtemisTranslatePipe" -g "*.ts"
# Search for new standalone imports
rg "import.*ArtemisTranslatePipe" -g "*.ts"
Length of output: 100172
src/main/webapp/app/shared/pipes/html-for-markdown.pipe.ts (3)
1-9: LGTM! Clean migration to standalone pipe
The changes follow Angular's style guide and best practices for standalone components. The import organization and type imports are well structured.
11-11: LGTM! Clean migration to inject() pattern
The migration from constructor injection to the inject() function is well implemented. The readonly modifier adds extra safety by preventing accidental reassignment.
8-8: Verify standalone pipe integration
Since this pipe is now standalone, we should verify that all its imports have been updated accordingly.
✅ Verification successful
Let me verify the module integration further by checking the content of the markdown.module.ts file.
Standalone pipe integration is correctly implemented
The pipe's integration is properly handled:
- It's correctly imported and used in the
importsarray ofArtemisMarkdownModule(not in declarations) - No other module declarations were found
- The exports array correctly includes the standalone pipe
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues with the standalone migration
# Check for any remaining declarations of this pipe in NgModule
echo "Checking for remaining module declarations..."
rg "declarations.*HtmlForMarkdownPipe" src/main/webapp
# Check for imports to ensure they're using standalone import
echo "Checking import patterns..."
rg "import.*HtmlForMarkdownPipe" src/main/webapp
Length of output: 509
Script:
#!/bin/bash
# Check the full content of markdown.module.ts to verify the module setup
echo "Checking markdown.module.ts content..."
cat src/main/webapp/app/shared/markdown.module.ts
Length of output: 622
src/main/webapp/app/shared/pipes/html-for-posting-markdown.pipe.ts (2)
1-8: LGTM! Clean implementation of standalone pipe
The migration to a standalone pipe and the use of the new inject import aligns well with Angular's latest best practices and the PR objectives.
Line range hint 12-27: Verify security implications of HTML sanitization
The implementation looks solid with proper typing and documentation. Since this pipe handles HTML sanitization, let's verify its usage across the codebase to ensure secure implementation.
✅ Verification successful
Let me analyze the security implications based on the results. I can see the complete flow of HTML sanitization:
- The pipe delegates to
markdownService.safeHtmlForPostingMarkdown - The service uses
htmlForMarkdownutility with DOMPurify sanitization - The only place defining custom attributes is
posting-content-part.components.tswhich only allows 'href' attribute - The sanitization is properly enforced through DOMPurify and Angular's
SafeHtmltype
Let me verify one more aspect to ensure there are no bypasses of this sanitization:
HTML sanitization implementation is secure
The security analysis reveals a robust sanitization chain:
- All markdown-to-HTML conversions go through DOMPurify sanitization before being marked as SafeHtml
- Direct
bypassSecurityTrustHtmlusage is properly gated behind sanitization - The only custom attribute allowlist is in
posting-content-part.components.tswhich safely allows only 'href' - The pipe correctly delegates to the service which enforces consistent sanitization through DOMPurify
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potentially unsafe usages of the pipe where sanitization might be bypassed
rg -A 3 "htmlForPostingMarkdown.*allowedHtmlTags|allowedHtmlAttributes" --type ts
# Check if there are any direct calls to markdownService.safeHtmlForPostingMarkdown
# that might bypass the pipe's sanitization
ast-grep --pattern 'markdownService.safeHtmlForPostingMarkdown($$$)'
Length of output: 6567
Script:
#!/bin/bash
# Check for any direct usage of bypassSecurityTrustHtml that might bypass the sanitization
rg -A 3 "bypassSecurityTrustHtml" --type ts
# Check for any DOMPurify usage to ensure consistent configuration
rg -A 3 "DOMPurify.sanitize" --type ts
Length of output: 3880
src/main/webapp/app/shared/shared-common.module.ts (2)
18-18: Verify standalone component migration
The migration of ArtemisTranslatePipe to imports indicates it's now a standalone component. Let's verify the implementation is complete.
Line range hint 18-45: Review consistency of standalone migrations
Since ArtemisTranslatePipe has been migrated to standalone, consider evaluating other components in this module for similar migration opportunities. This would align with the PR's objective of improving the application's architecture.
src/main/webapp/app/shared/shared.module.ts (1)
32-32: LGTM! Correct migration to standalone component.
The LoadingIndicatorContainerComponent has been properly moved from declarations to imports, which is the correct approach for standalone components while maintaining its availability through exports.
src/test/javascript/spec/pipe/reacting-users-on-posting.pipe.spec.ts (2)
7-7: LGTM! Good use of NgMocks
The addition of MockPipe from ng-mocks aligns with our testing guidelines and provides better test isolation.
Line range hint 15-24: LGTM! Well-structured async test setup
The test setup follows best practices:
- Proper async/await usage
- Correct use of MockPipe
- Appropriate dependency injection
src/main/webapp/app/shared/metis/metis.module.ts (1)
67-67: LGTM! Change aligns with standalone component migration.
The addition of HtmlForPostingMarkdownPipe to the imports array correctly reflects its new standalone status, improving modularity as intended in the PR objectives.
src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts (2)
Line range hint 66-249: Test implementation looks good!
The test suite is well-structured with:
- Comprehensive coverage of different scenarios
- Good use of parameterized tests
- Proper test isolation
- Clear test descriptions
35-35: Verify the necessity of full ArtemisTestModule import
According to coding guidelines, we should avoid full module imports. Consider importing only the necessary components and services from ArtemisTestModule.
src/test/javascript/spec/component/assessment-shared/assessment-header.component.spec.ts (1)
10-10: LGTM! Import changes align with testing guidelines.
The addition of MockPipe from ng-mocks follows the coding guidelines for test files and supports the migration to standalone components.
src/main/webapp/app/shared/loading-indicator-container/loading-indicator-container.component.ts
Show resolved
Hide resolved
HawKhiem
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
| // We cannot inject() ChangeDetectorRef here | ||
| constructor(translateService: TranslateService, changeDetectorRef: ChangeDetectorRef) { |
There was a problem hiding this comment.
You could improve the comment here, by explaining why we cannot do that here (or maybe even find a work around to make it work?)
There was a problem hiding this comment.
Thanks for the feedback! 😄 I tried to extend the comment in this commit. Unfortunately, I already tried some ways to get around it, but I could not find a good solution. 😅 But I am open for suggestions, if anyone could think of something.
PaRangger
dismissed stale reviews from HawKhiem and coderabbitai[bot]
via
2925c0d
Checklist
General
Client
Motivation and Context
This is an effort to migrate some core components to the new angular apis. The process was covered in the all hands meeting and can be seen here. This PR addresses some core components which are used frequently in the communication module:
Description
I replaced
@Input,@Output,@ViewChildswith their corresponding signal equivalent and made sure to call them as functions in their templates. I replaced constructors with inject() functions. I converted them tostandalone: truecomponents. Lastly, I tried to fix the failing tests.Steps for Testing
Prerequisites:
These pipes and components are used frequently throughout artemis. Therefore just observe if any problems occur on the entire platform. In these steps I will just guide you to a single occurence of the corresponding pipe/component.
Loading Indicator
HtmlMarkdownPipe/PostingMarkdownPipe
Translate Pipe
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
Screenshots
Loading Indicator
HtmlMarkdownPipe/PostingMarkdownPipe
Translate Pipe
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests