Iris: Add Artemis Intelligence rewrite action for FAQs
#10157
Conversation
github-actions
bot
added
tests
server
FelixTJDietrich
changed the title
IRIS: Add rephrasing pipeline to ArtemisIris: Add rephrasing pipeline to Artemis for FAQs
FelixTJDietrich
changed the title
Iris: Add rephrasing pipeline to Artemis for FAQsIris: Add AI rephrasing for FAQs
FelixTJDietrich
changed the title
Iris: Add AI rephrasing for FAQsIris: Add AI rephrasing action for FAQs
WalkthroughThis pull request introduces a comprehensive implementation of a text rewriting feature within the Artemis platform, specifically for the Iris intelligence system. The changes span both backend and frontend components, adding support for rewriting markdown text in different variants (such as FAQ and problem statements) through a new AI-powered pipeline. The implementation includes REST endpoints, services, data transfer objects, and frontend components that enable users to rewrite text using Artemis Intelligence, with robust error handling and real-time status updates. 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)
⏰ Context from checks skipped due to timeout of 90000ms (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
|
…-pipeline' into feature/communication/rephrasing-pipeline
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (9)
src/main/webapp/app/shared/monaco-editor/rephrase.service.ts (1)
19-21: Use constructor injection for dependency injection.In Angular services, it's recommended to use constructor injection for dependency injection to enhance readability and testability. Using the
injectfunction is typically reserved for scenarios where constructor injection isn't feasible.Apply this diff to refactor the code:
-export class RephraseService { - // ... - private http = inject(HttpClient); - private jhiWebsocketService = inject(JhiWebsocketService); - private alertService = inject(AlertService); +export class RephraseService { + constructor( + private http: HttpClient, + private jhiWebsocketService: JhiWebsocketService, + private alertService: AlertService, + ) {} // ... }src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/PyrisRephrasingDTO.java (1)
5-13: Improve Javadoc comments formatting.Ensure that Javadoc comments are properly formatted for clarity and consistency.
Apply this diff to improve the Javadoc:
/** - * DTO for the Iris rephrasing feature. - * A rephrasing is just a text and determines the variant of the rephrasing. - * - * @param text The rephrased text - * @param variant The variant of the rephrasing - * - * - */ + * DTO representing a rephrased text and its variant for the Iris rephrasing feature. + * + * @param text the rephrased text + * @param variant the variant of the rephrasing + */src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/PyrisRephrasingPipelineExecutionDTO.java (1)
7-12: Correct the pipeline name in Javadoc comments.The Javadoc mentions "Iris competency extraction pipeline," which may be incorrect in the context of rephrasing.
Apply this diff to correct the comment:
/** - * DTO to execute the Iris competency extraction pipeline on Pyris + * DTO to execute the Iris rephrasing pipeline on Pyris * * @param execution The pipeline execution details * @param toBeRephrased The text to be rephrased */src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/RephrasingJob.java (1)
15-15: Consider using wrapper types for IDs instead of primitives.Using primitive
longfor IDs could lead to potential issues with null values and serialization. Consider usingLongwrapper type instead.-public record RephrasingJob(String jobId, long courseId, long userId) implements PyrisJob { +public record RephrasingJob(String jobId, Long courseId, Long userId) implements PyrisJob {src/test/javascript/spec/service/rephrasing.service.spec.ts (2)
45-47: UseURLSearchParamsfor query parameter construction.The current URL construction using string concatenation is prone to encoding issues. Consider using
URLSearchParamsfor safer parameter handling.- const req = httpMock.expectOne({ - url: `api/courses/${courseId}/rephrase-text?toBeRephrased=${toBeRephrased}&variant=${rephrasingVariant}`, - method: 'POST', + const params = new URLSearchParams({ + toBeRephrased, + variant: rephrasingVariant.toString() + }); + const req = httpMock.expectOne({ + url: `api/courses/${courseId}/rephrase-text?${params.toString()}`, + method: 'POST',
63-64: Enhance error message assertion.The error message assertion is too generic. Consider asserting the complete error message including the status code and text.
- expect(err).toBe('HTTP Request Error:'); + expect(err).toBe('HTTP Request Error: 400 Bad Request');src/main/webapp/app/iris/iris-chat.service.ts (1)
22-22: LGTM! Consider addressing the TODO comment.The new
REPHRASEmode follows the existing naming pattern. While adding this mode, it might be a good time to address the TODO comment about renamingEXERCISEtoPROGRAMMING_EXERCISEfor better clarity.src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts (1)
316-316: Consider using a signal for loading state.Since Angular 16+, signals provide a more efficient way to handle reactive state changes, especially with OnPush change detection.
-isLoading: boolean = false; +isLoading = signal<boolean>(false);src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html (1)
46-50: Improve loading state UX.The loading spinner's placement within the file upload footer makes it less visible and could be confusing. Consider:
- Moving the loading indicator to a more prominent position
- Adding an overlay to prevent user interaction during rephrasing
Consider implementing a centered overlay with the loading spinner:
+ <div *ngIf="isLoading" class="position-absolute w-100 h-100 d-flex justify-content-center align-items-center bg-white bg-opacity-75" style="top: 0; left: 0; z-index: 1000;"> + <fa-icon [icon]="faSpinner" animation="spin" size="2x" /> + </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/RephrasingResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/IrisRephrasingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/PyrisRephrasingDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/PyrisRephrasingPipelineExecutionDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/PyrisRephrasingStatusUpdateDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/RephrasingVariant.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/RephrasingJob.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/web/open/PublicPyrisStatusUpdateResource.java(2 hunks)src/main/webapp/app/faq/faq-update.component.html(1 hunks)src/main/webapp/app/faq/faq-update.component.ts(5 hunks)src/main/webapp/app/iris/iris-chat.service.ts(1 hunks)src/main/webapp/app/iris/iris-websocket.service.ts(2 hunks)src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html(1 hunks)src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts(6 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/adapter/monaco-text-editor.adapter.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/adapter/text-editor.interface.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/rephrase.action.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts(2 hunks)src/main/webapp/app/shared/monaco-editor/model/rephrasing-variant.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/rephrase.service.ts(1 hunks)src/main/webapp/i18n/de/markdownEditor.json(2 hunks)src/main/webapp/i18n/en/markdownEditor.json(2 hunks)src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java(2 hunks)src/test/javascript/spec/service/rephrasing.service.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/RephrasingVariant.java
- src/main/webapp/app/shared/monaco-editor/model/rephrasing-variant.ts
🧰 Additional context used
📓 Path-based instructions (22)
src/main/webapp/app/faq/faq-update.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/monaco-editor/model/actions/adapter/text-editor.interface.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/adapter/monaco-text-editor.adapter.ts (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/PyrisRephrasingDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.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/iris/iris-chat.service.ts (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/PyrisRephrasingStatusUpdateDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts (1)
src/main/webapp/i18n/de/markdownEditor.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/test/javascript/spec/service/rephrasing.service.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/java/de/tum/cit/aet/artemis/iris/web/open/PublicPyrisStatusUpdateResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/RephrasingJob.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/shared/monaco-editor/model/actions/rephrase.action.ts (1)
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/main/webapp/app/iris/iris-websocket.service.ts (1)
src/main/webapp/app/shared/monaco-editor/rephrase.service.ts (1)
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts (1)
src/main/webapp/app/faq/faq-update.component.ts (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/PyrisRephrasingPipelineExecutionDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisRephrasingService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/RephrasingResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (13)
src/main/webapp/app/shared/monaco-editor/rephrase.service.ts (2)
52-52: Localize success message properly.Ensure that the success message key
'artemisApp.markdownEditor.rephrasing.success'exists in the localization files to provide proper feedback to users.Run the following script to verify the localization key:
✅ Verification successful
Localization key exists and is properly configured.
The key
artemisApp.markdownEditor.rephrasing.successis present in both English and German translation files with appropriate translations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the localization key exists. # Test: Search for the localization key in the translation files. grep -r 'artemisApp\.markdownEditor\.rephrasing\.success' src/main/webapp/i18n/Length of output: 81
Script:
#!/bin/bash # Check directory structure and list translation files echo "=== Translation Files ===" fd . src/main/webapp/i18n/ -t f -x ls -l {} # Search for similar keys (markdown editor related) echo -e "\n=== Markdown Editor Related Keys ===" rg "markdownEditor" src/main/webapp/i18n/ -A 1 -B 1 # Search for rephrasing related keys echo -e "\n=== Rephrasing Related Keys ===" rg "rephras" src/main/webapp/i18n/Length of output: 30369
46-54: Add error handling for unexpected responses from WebSocket.If the WebSocket receives a message without a
resultproperty, the code doesn't handle it, which could lead to the loading indicator not being dismissed.Consider adding an
elseblock to handle unexpected messages or errors:if (update.result) { observer.next(update.result); observer.complete(); this.isLoadingSubject.next(false); this.jhiWebsocketService.unsubscribe(websocketTopic); this.alertService.success('artemisApp.markdownEditor.rephrasing.success'); +} else { + observer.error('Unexpected WebSocket response.'); + this.isLoadingSubject.next(false); + this.jhiWebsocketService.unsubscribe(websocketTopic); +}src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/PyrisRephrasingDTO.java (1)
15-15: Adhere to DTO naming conventions and Java best practices.According to the coding guidelines, DTOs should use Java Records and have minimal data with single responsibility.
The
PyrisRephrasingDTOcorrectly uses a Java Record and follows single responsibility principles.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/PyrisRephrasingPipelineExecutionDTO.java (1)
14-14: Ensure minimal data in DTOs as per guidelines.The
PyrisRephrasingPipelineExecutionDTOcontains only necessary data, adhering to the guideline of minimal data in DTOs.src/main/webapp/app/shared/monaco-editor/model/actions/adapter/text-editor.interface.ts (1)
56-61: LGTM!The new
getFullText()method is well-documented and follows the interface's existing patterns for text retrieval methods.src/main/java/de/tum/cit/aet/artemis/iris/service/IrisRephrasingService.java (1)
80-90: 🛠️ Refactor suggestionAdd transaction management and improve null handling.
The
handleStatusUpdatemethod should use transaction management and improve null handling for tokens.+ @Transactional public RephrasingJob handleStatusUpdate(RephrasingJob job, PyrisRephrasingStatusUpdateDTO statusUpdate) { + if (job == null || statusUpdate == null) { + throw new IllegalArgumentException("Job and status update must not be null"); + } Course course = courseRepository.findByIdForUpdateElseThrow(job.courseId()); - if (statusUpdate.tokens() != null && !statusUpdate.tokens().isEmpty()) { + var tokens = statusUpdate.tokens(); + if (tokens != null && !tokens.isEmpty()) { llmTokenUsageService.saveLLMTokenUsage(statusUpdate.tokens(), LLMServiceType.IRIS, builder -> builder.withCourse(course.getId()).withUser(job.userId())); } var user = userRepository.findById(job.userId()).orElseThrow(); websocketService.send(user.getLogin(), websocketTopic(job.courseId()), statusUpdate); return job; }Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java (1)
47-47: LGTM! Constructor injection and field declaration follow best practices.The changes correctly implement constructor injection for the new
IrisRephrasingServicedependency.Also applies to: 53-53, 59-59
src/main/webapp/app/faq/faq-update.component.ts (1)
57-60: LGTM! Service injection follows Angular best practices.The changes correctly use Angular's dependency injection with the
injectfunction.src/main/java/de/tum/cit/aet/artemis/iris/web/open/PublicPyrisStatusUpdateResource.java (1)
154-177: LGTM! The endpoint implementation follows established patterns.The implementation correctly:
- Uses proper authentication via
@EnforceNothing- Validates job ID match
- Handles errors consistently with other endpoints
- Returns appropriate response
src/main/webapp/app/shared/monaco-editor/model/actions/adapter/monaco-text-editor.adapter.ts (1)
236-241: LGTM! The implementation is clean and efficient.The
getFullTextmethod correctly uses existing functionality to retrieve the full text content.src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (1)
166-178: LGTM! The mock implementation follows established patterns.The new mock method is well-implemented and consistent with other mock methods in the class. It properly handles the DTO and response status.
src/main/webapp/i18n/en/markdownEditor.json (1)
7-7: LGTM! Clear and informative translations.The translations are well-implemented with clear, user-friendly messages. The success message helpfully informs users about the Ctrl+Z shortcut to undo changes.
Also applies to: 17-19
src/main/webapp/app/faq/faq-update.component.html (1)
24-24: LGTM! Proper integration of rephrasing actions.The addition of
[metaActions]properly integrates the rephrasing functionality into the markdown editor component.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/RephrasingJob.java
Outdated
Show resolved
Hide resolved
...de/tum/cit/aet/artemis/iris/service/pyris/dto/rephrasing/PyrisRephrasingStatusUpdateDTO.java
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/rephrase.action.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisRephrasingService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisRephrasingService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/RephrasingResource.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java
Show resolved
Hide resolved
...m/cit/aet/artemis/iris/service/pyris/dto/rephrasing/PyrisRephrasingPipelineExecutionDTO.java
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/rephrase.action.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/main/webapp/app/faq/faq-update.component.ts (1)
80-85:⚠️ Potential issueFix potential memory leak in subscription handling.
The component subscribes to profile information but doesn't unsubscribe when destroyed.
- Implement OnDestroy interface:
-export class FaqUpdateComponent implements OnInit { +export class FaqUpdateComponent implements OnInit, OnDestroy {
- Add ngOnDestroy to clean up the subscription:
+ ngOnDestroy() { + if (this.profileInfoSubscription) { + this.profileInfoSubscription.unsubscribe(); + } + }src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html (1)
46-50:⚠️ Potential issueRemove unnecessary link from loading spinner.
The loading spinner shouldn't be wrapped in an
<a>tag linking to markdown documentation. This could confuse users and doesn't serve any purpose.Apply this diff:
- @if (isLoading) { - <a class="col-auto" href="https://markdown-it.github.io/"> - <fa-icon [icon]="faSpinner" animation="spin" /> - </a> - } + @if (isLoading) { + <div class="col-auto"> + <fa-icon [icon]="faSpinner" animation="spin" /> + </div> + }
🧹 Nitpick comments (16)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisReWritingService.java (3)
24-26: Update class-level JavaDoc to reflect the rewriting functionalityThe class-level JavaDoc comment describes handling the "Competency generation subsystem" of Iris, but this class
IrisReWritingServicemanages the rewriting feature. Please update the comment to accurately reflect the purpose of this service.
53-59: Correct method JavaDoc forexecuteRewritingPipelineThe JavaDoc for
executeRewritingPipelinerefers to executing the competency extraction pipeline and mentions "course description" astoBeRewritten, which seems inconsistent with the method's purpose. Please update the documentation to accurately describe the execution of the rewriting pipeline and adjust parameter descriptions accordingly.
73-79: Adjust method JavaDoc forhandleStatusUpdateThe JavaDoc for
handleStatusUpdatementions "a new competency extraction result" and "new competency recommendations", which does not align with the method's functionality of handling rewriting status updates. Please revise the documentation to accurately reflect that it processes status updates for the rewriting job and update parameter descriptions accordingly.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingDTO.java (1)
5-13: Improve class-level JavaDoc for clarityThe statement "A rewriting is just a text and determines the variant of the rewriting." is a bit unclear. Consider rephrasing it to explain that this DTO encapsulates the text to be rewritten along with the specified rewriting variant.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingPipelineExecutionDTO.java (1)
7-12: Update JavaDoc to reflect the rewriting pipeline executionThe JavaDoc mentions executing the "Iris competency extraction pipeline," but this DTO is used for executing the rewriting pipeline on Pyris. Please adjust the documentation to accurately describe the purpose of this DTO and update parameter descriptions accordingly.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/RewritingJob.java (1)
8-8: Fix typo in documentationThe JavaDoc comment contains a grammatical error: "that rewritten" should be "that rewrites".
-* A pyris job that rewritten a text. +* A pyris job that rewrites a text.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingStatusUpdateDTO.java (1)
20-21: Consider adding validation annotationsThe DTO fields lack validation annotations. Consider adding
@NotNullfor required fields and@Sizeconstraints for collections to ensure data integrity.@JsonInclude(JsonInclude.Include.NON_EMPTY) +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Size; -public record PyrisRewritingStatusUpdateDTO(List<PyrisStageDTO> stages, String result, List<LLMRequest> tokens) { +public record PyrisRewritingStatusUpdateDTO( + @NotNull @Size(min = 1) List<PyrisStageDTO> stages, + String result, + @NotNull List<LLMRequest> tokens +) {src/main/webapp/app/shared/monaco-editor/model/actions/rewriteAction.ts (2)
8-9: Fix documentation grammarThe class documentation contains a grammatical error.
-* Action to the rewriting action the editor. +* Action to rewrite content in the editor.
24-27: Fix method documentationThe method documentation contains typos and unclear description.
-* Toggles the rephrrewritingasing of the markdown content the editor. -* @param editor The editor in which to rewrite the markdown. -* @param variant The variant of the rewritingVariant. +* Initiates the rewriting of markdown content in the editor. +* @param editor The editor instance containing the markdown content to rewrite. +* @param variant The type of rewriting to perform.src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/RewritingResource.java (1)
31-32: Remove unused fieldThe
applicationNamefield is declared but never used.- @Value("${jhipster.clientApp.name}") - private String applicationName;src/main/webapp/app/shared/monaco-editor/rewriting.service.ts (1)
41-43: Remove redundant loading state update.The loading state is already set to true at line 31. This second update is unnecessary.
.subscribe({ next: () => { - this.isLoadingSubject.next(true);src/main/webapp/app/iris/iris-websocket.service.ts (1)
72-110: Reduce code duplication with session and topic handling.The topic subscription logic duplicates much of the session subscription logic. Consider extracting common patterns into shared methods.
private subscribeToChannel<T>( channelId: T, channelMap: Map<T, SubscribedChannel>, getChannelPath: (id: T) => string ): Observable<any> { if (!channelId) { throw new Error('Channel identifier is required'); } if (!channelMap.has(channelId)) { const channel = getChannelPath(channelId); const subject = new Subject<any>(); const wsSubscription = this.jhiWebsocketService .subscribe(channel) .receive(channel) .subscribe({ next: (response: any) => { subject.next(response); }, error: (error: any) => { subject.error(error); this.unsubscribeFromChannel(channelId, channelMap, channel); } }); channelMap.set(channelId, { wsSubscription, subject }); } return channelMap.get(channelId)!.subject.asObservable(); }src/main/java/de/tum/cit/aet/artemis/iris/web/open/PublicPyrisStatusUpdateResource.java (1)
154-177: Fix unclosed paragraph tag in JavaDoc.The
<p>tag in the JavaDoc comment is not closed.Apply this diff to fix the JavaDoc:
/** * POST public/pyris/pipelines/rewriting/runs/:runId/status : Send the rewritten text in a status update - * <p> + * <p></p> * Uses custom token based authentication.Otherwise, the implementation looks good and follows the established pattern for status update endpoints.
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts (2)
299-304: Fix documentation issues.The JSDoc comment has the following issues:
- Incorrectly mentions "toggling" which doesn't match the implementation
- Missing parameter description for
courseId- * Toggles the rewriting of the given text. If no text is provided, the editor's will return undefined. + * Rewrites the text content using the specified variant and service. If no text is present, no action is taken. * @param editor The editor to toggle the text rewriting for. * @param variant The variant to use for rewriting the text. * @param rewritingService The service to use for rewriting the text. + * @param courseId The ID of the course context for rewriting.
305-317: Add loading state management.The method should manage loading state to provide feedback during the rewriting process.
rewriteMarkdown(editor: TextEditor, variant: RewritingVariant, rewritingService: RewritingService, courseId: number): void { const text = editor.getFullText(); if (text) { + this.isRewriting = true; + this.notifyLoadingState(true); rewritingService.rewritteMarkdown(text, variant, courseId).subscribe({ next: (message) => { this.replaceTextAtRange(editor, new TextEditorRange(new TextEditorPosition(1, 1), this.getEndPosition(editor)), message); + this.isRewriting = false; + this.notifyLoadingState(false); }, error: (error) => { console.error('Error during rewriting:', error); + this.isRewriting = false; + this.notifyLoadingState(false); }, }); } }src/main/webapp/i18n/en/markdownEditor.json (1)
7-7: Consider platform-agnostic keyboard shortcut text.The success message mentions "Ctrl+Z" which is specific to Windows/Linux. Mac users use "Cmd+Z" instead.
Consider using a more generic phrase like "Use undo to restore the original text" or detect the platform to show the appropriate shortcut.
Also applies to: 17-19
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/RewritingResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/IrisReWritingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingPipelineExecutionDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingStatusUpdateDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/RewritingVariant.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/RewritingJob.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/web/open/PublicPyrisStatusUpdateResource.java(2 hunks)src/main/webapp/app/faq/faq-update.component.html(1 hunks)src/main/webapp/app/faq/faq-update.component.ts(5 hunks)src/main/webapp/app/icons/icons.ts(1 hunks)src/main/webapp/app/iris/iris-chat.service.ts(1 hunks)src/main/webapp/app/iris/iris-websocket.service.ts(2 hunks)src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html(1 hunks)src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts(6 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/adapter/monaco-text-editor.adapter.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/adapter/text-editor.interface.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/rewriteAction.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts(2 hunks)src/main/webapp/app/shared/monaco-editor/model/rewriting-variant.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/rewriting.service.ts(1 hunks)src/main/webapp/i18n/de/markdownEditor.json(2 hunks)src/main/webapp/i18n/en/markdownEditor.json(2 hunks)src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java(2 hunks)src/test/javascript/spec/service/rewriting.service.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/main/webapp/app/shared/monaco-editor/model/rewriting-variant.ts
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/RewritingVariant.java
🧰 Additional context used
📓 Path-based instructions (23)
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.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/monaco-editor/model/actions/adapter/text-editor.interface.ts (1)
src/main/webapp/app/faq/faq-update.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/monaco-editor/model/actions/adapter/monaco-text-editor.adapter.ts (1)
src/main/webapp/app/iris/iris-chat.service.ts (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingStatusUpdateDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/main/webapp/i18n/de/markdownEditor.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/app/shared/monaco-editor/model/actions/rewriteAction.ts (1)
src/main/webapp/app/icons/icons.ts (1)
src/test/javascript/spec/service/rewriting.service.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/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingPipelineExecutionDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/RewritingJob.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/faq/faq-update.component.ts (1)
src/main/webapp/app/shared/monaco-editor/rewriting.service.ts (1)
src/main/java/de/tum/cit/aet/artemis/iris/web/open/PublicPyrisStatusUpdateResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisReWritingService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/iris/iris-websocket.service.ts (1)
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/RewritingResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
🔇 Additional comments (19)
src/main/webapp/app/iris/iris-websocket.service.ts (2)
15-15: Add topic cleanup in ngOnDestroy.The
ngOnDestroymethod only cleans up session subscriptions but not topic subscriptions, which could lead to memory leaks.Also applies to: 19-24
78-96: Add error handling for subscription failures.The
subscribeToTopicmethod should handle subscription failures to prevent silent errors and potential memory leaks.src/main/webapp/app/shared/monaco-editor/model/actions/adapter/text-editor.interface.ts (1)
56-61: LGTM! Well-documented interface method.The new method is properly documented and maintains consistency with existing methods in normalizing line endings.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java (2)
114-120: Fix incorrect JavaDoc.The JavaDoc comment incorrectly references
IrisCompetencyGenerationServiceinstead ofIrisReWritingService.Apply this diff to fix the JavaDoc:
/** - * Handles the status update of a competency extraction job and forwards it to - * {@link IrisCompetencyGenerationService#handleStatusUpdate(CompetencyExtractionJob, PyrisCompetencyStatusUpdateDTO)} + * Handles the status update of a rewriting job and forwards it to + * {@link IrisReWritingService#handleStatusUpdate(RewritingJob, PyrisRewritingStatusUpdateDTO)} * * @param job the job that is updated * @param statusUpdate the status update */
121-124: LGTM!The implementation follows the established pattern for handling status updates, maintaining consistency with other job types.
src/main/webapp/app/faq/faq-update.component.ts (1)
43-43: Consider using a signal for profile management.The
irisEnabledflag could be managed more efficiently using Angular's signal feature.Consider using
toSignalto interface withprofileService.getProfileInfo():irisEnabled = toSignal(this.profileService.getProfileInfo().pipe( map(info => info.activeProfiles.includes(PROFILE_IRIS)) )); metaActions = computed(() => this.irisEnabled() ? [new RewriteAction(this.rewriteService, RewritingVariant.FAQ, this.courseId), new FullscreenAction()] : [new FullscreenAction()] );src/main/webapp/app/icons/icons.ts (1)
59-69: LGTM!The new icon definition follows the established pattern and maintains consistency with other icons in the pack.
src/main/webapp/app/shared/monaco-editor/model/actions/adapter/monaco-text-editor.adapter.ts (1)
236-241: LGTM! Clean implementation reusing existing functionality.The implementation efficiently retrieves the full text content by creating a range spanning the entire document and utilizing the existing
getTextAtRangemethod.src/main/webapp/app/iris/iris-chat.service.ts (1)
22-22: LGTM! Enum value follows existing patterns.The new
REWRITEenum value follows the established naming convention and string value pattern of theChatServiceModeenum.src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (1)
166-178: LGTM! Implementation follows established patterns.The mock method follows the same pattern as other mock methods in the class, properly setting up expectations and handling responses for the rewriting pipeline endpoint.
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts (1)
312-314: Enhance error handling and user feedback.The current implementation only logs errors to the console. Consider using the alert service for user feedback.
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html (1)
46-46: LGTM! Proper usage of new Angular syntax.The code correctly uses the new
@ifdirective as required by the coding guidelines.src/main/webapp/app/faq/faq-update.component.html (1)
24-24: LGTM! MetaActions integration looks correct.The addition of
[metaActions]binding aligns with the PR objective to add AI rephrasing capabilities to the FAQ editor.src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts (4)
10-10: LGTM!The imports and class signature changes are correctly implemented to support the new rewriting functionality.
Also applies to: 74-74, 139-139, 145-145
316-316: LGTM!The loading state property is correctly typed and initialized.
318-323: Remove debug console.log and handle potential memory leak.The loading state subscription has the following issues:
- Contains a debug console.log statement
- Subscription is not cleaned up on component destruction
- May need ChangeDetectorRef due to OnPush strategy
301-301: LGTM!The spinner icon is correctly added for the loading state.
src/main/webapp/i18n/de/markdownEditor.json (2)
18-18: Use informal language (du-form) in German translations.According to the coding guidelines, German translations should use informal language (dutzen) instead of formal language (sietzen).
18-18: Consider platform-agnostic keyboard shortcut text in German translation.The success message mentions "Strg+Z" which is specific to Windows/Linux. Mac users use "Cmd+Z" instead.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/RewritingJob.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/RewritingResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/webapp/app/faq/faq-update.component.ts (1)
45-45:⚠️ Potential issueImplement OnDestroy to prevent memory leaks.
The component has subscriptions but doesn't implement OnDestroy for cleanup.
-export class FaqUpdateComponent implements OnInit { +export class FaqUpdateComponent implements OnInit, OnDestroy { + ngOnDestroy() { + if (this.profileInfoSubscription) { + this.profileInfoSubscription.unsubscribe(); + } + }Also applies to: 62-62
🧹 Nitpick comments (4)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisReWritingService.java (4)
24-26: Enhance class-level JavaDoc documentation.The current documentation is minimal. Consider adding more details about:
- The purpose and responsibilities of the service
- The relationship with Pyris system
- The types of rewriting operations supported
- The expected interaction flow
31-51: Consider splitting the service to better adhere to Single Responsibility Principle.The service has 6 dependencies which might indicate it's handling too many responsibilities. Consider splitting into:
IrisReWritingExecutionService: Handle pipeline execution (pyrisPipelineService, pyrisJobService)IrisReWritingNotificationService: Handle websocket communication (websocketService)IrisReWritingUsageService: Handle token usage and user/course data (llmTokenUsageService, courseRepository, userRepository)
53-59: Complete the method documentation.The JavaDoc is missing:
@param variantdocumentation@throwsdocumentation for potential exceptions- Description of the websocket communication flow
92-94: Minor improvements for websocketTopic method.Consider these enhancements:
- Use String.format for better readability
- Add courseId validation
private static String websocketTopic(long courseId) { + if (courseId <= 0) { + throw new IllegalArgumentException("Course ID must be positive"); + } - return "rewriting/" + courseId; + return String.format("rewriting/%d", courseId); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisReWritingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.java(1 hunks)src/main/webapp/app/faq/faq-update.component.ts(5 hunks)src/main/webapp/app/iris/iris-websocket.service.ts(1 hunks)src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html(1 hunks)src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts(6 hunks)src/main/webapp/i18n/de/markdownEditor.json(2 hunks)src/main/webapp/i18n/en/markdownEditor.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyExtractionPipelineExecutionDTO.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html
- src/main/webapp/app/iris/iris-websocket.service.ts
- src/main/webapp/i18n/en/markdownEditor.json
- src/main/webapp/i18n/de/markdownEditor.json
- src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/faq/faq-update.component.ts (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisReWritingService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (2)
src/main/webapp/app/faq/faq-update.component.ts (2)
1-1: LGTM! Clean import organization.The new imports are well-organized and follow the Angular style guide.
Also applies to: 20-28
82-86: 🛠️ Refactor suggestionTrack the profile info subscription.
While the signal setup is good, the subscription from
toSignalshould be tracked to prevent memory leaks.- const activeProfilesSignal = toSignal(this.profileService.getProfileInfo(), { initialValue: null }); + this.profileInfoSubscription = this.profileService.getProfileInfo().subscribe(); + const activeProfilesSignal = toSignal(this.profileService.getProfileInfo(), { initialValue: null });Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisReWritingService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisReWritingService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/webapp/app/faq/faq-update.component.ts (1)
45-46:⚠️ Potential issueFix potential memory leak in subscription handling.
The component subscribes to profile information but doesn't properly clean up the subscription.
Add
ngOnDestroyand implement theOnDestroyinterface:-export class FaqUpdateComponent implements OnInit { +export class FaqUpdateComponent implements OnInit, OnDestroy { + ngOnDestroy() { + if (this.profileInfoSubscription) { + this.profileInfoSubscription.unsubscribe(); + } + }Also applies to: 63-65
🧹 Nitpick comments (1)
src/main/webapp/app/faq/faq-update.component.ts (1)
Line range hint
52-62: Follow coding guidelines for private members.According to the coding guidelines, private members should not use the
privateprefix.- private alertService = inject(AlertService); - private faqService = inject(FaqService); - private activatedRoute = inject(ActivatedRoute); - private navigationUtilService = inject(ArtemisNavigationUtilService); - private router = inject(Router); - private translateService = inject(TranslateService); - private accountService = inject(AccountService); - private rewriteService = inject(RewritingService); - private profileService = inject(ProfileService); + alertService = inject(AlertService); + faqService = inject(FaqService); + activatedRoute = inject(ActivatedRoute); + navigationUtilService = inject(ArtemisNavigationUtilService); + router = inject(Router); + translateService = inject(TranslateService); + accountService = inject(AccountService); + rewriteService = inject(RewritingService); + profileService = inject(ProfileService);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/webapp/app/faq/faq-update.component.html(1 hunks)src/main/webapp/app/faq/faq-update.component.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/faq/faq-update.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/faq/faq-update.component.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (2)
src/main/webapp/app/faq/faq-update.component.ts (2)
1-1: LGTM! Imports are well-organized.The new imports are properly organized and follow Angular style guidelines.
Also applies to: 4-4, 20-28
67-69: Consider using IrisEnabledComponent for course-level IRIS check.The current implementation only checks the global IRIS profile. Consider using the dedicated
IrisEnabledComponentto ensure IRIS features are only shown when enabled for the specific course.Additionally, consider refactoring the signal initialization:
- constructor() { - this.activeProfilesSignal = toSignal(this.profileService.getProfileInfo(), { initialValue: null }); - } + private readonly activeProfilesSignal = toSignal(this.profileService.getProfileInfo(), { initialValue: null });Also applies to: 88-89
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/webapp/app/shared/monaco-editor/rewriting.service.ts (1)
46-54:⚠️ Potential issueAdd type safety and timeout handling for WebSocket updates.
The current implementation lacks type safety for WebSocket responses and doesn't handle timeouts.
🧹 Nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisRewritingService.java (3)
64-64: Usevariant.name()Instead ofvariant.toString()for Enum NamesWhen working with enums,
name()is preferred overtoString()to retrieve the name of the enum constant. This ensures consistency and avoids unexpected results iftoString()is overridden.Apply this diff to make the change:
- variant.toString(), + variant.name(),
68-68: Avoid PassingnullValues to ConstructorsPassing
nullvalues can lead toNullPointerExceptionif not handled properly. Consider using overloaded constructors or builder patterns to handle optional fields, or ensure that the constructor can safely handlenullinputs.
86-86: Handle Potential Exceptions When Retrieving UserRetrieving the user with
findById(job.userId()).orElseThrow()may throw an exception if the user is not found. Consider handling this exception to provide a meaningful error message or to implement alternative logic.src/main/webapp/app/faq/faq-update.component.ts (1)
51-51: Add type annotations for better type safetyConsider adding explicit type annotations to the computed property for better maintainability and type safety.
- metaActions = computed(() => ( + metaActions = computed<TextEditorAction[]>(() => ( this.irisEnabled() ? [new RewriteAction(RewritingVariant.FAQ, this.courseId), new FullscreenAction()] : [] ));src/main/webapp/app/shared/monaco-editor/rewriting.service.ts (1)
14-21: Consider making the resource URL configurable.The hardcoded API URL could be moved to a configuration file or environment variable for better maintainability.
- public resourceUrl = 'api/courses'; + public resourceUrl = environment.apiUrl + '/courses';src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts (1)
300-305: Improve method documentation.The current documentation is unclear. The parameter description for
editormentions "toggle" which is incorrect, and the return value is not documented./** - * Rewrites the given text using Artemis Intelligence. If no text is provided, the editor's will return undefined. - * @param editor The editor to toggle the text rewriting for. + * Rewrites the text content using Artemis Intelligence. + * @param editor The editor containing the text to be rewritten. * @param variant The variant to use for rewriting the text. * @param courseId The ID of the course to use for rewriting the text (for tracking purposes). + * @returns void This method updates the editor content directly. */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/RewritingResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/IrisRewritingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingPipelineExecutionDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/RewritingJob.java(1 hunks)src/main/webapp/app/faq/faq-update.component.ts(3 hunks)src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/rewriteAction.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts(2 hunks)src/main/webapp/app/shared/monaco-editor/rewriting.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/rewriting/PyrisRewritingPipelineExecutionDTO.java
- src/main/webapp/app/shared/monaco-editor/model/actions/rewriteAction.ts
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/RewritingJob.java
- src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/RewritingResource.java
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/faq/faq-update.component.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisRewritingService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/shared/monaco-editor/rewriting.service.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
- GitHub Check: Build .war artifact
- GitHub Check: Mend Security Check
🔇 Additional comments (8)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisRewritingService.java (2)
44-51: Constructor Injection is Properly ImplementedThe service correctly uses constructor injection for dependency management, adhering to best practices.
81-81: Review the Necessity of Database Lock withfindByIdForUpdateElseThrowUsing
findByIdForUpdateElseThrowacquires a database lock on theCourseentity. Verify if this lock is necessary for the operation to prevent potential performance impacts due to database contention.Would you like assistance in determining whether the database lock is essential for this context?
src/main/webapp/app/faq/faq-update.component.ts (4)
1-1: Great use of modern Angular patterns!The code follows Angular best practices by:
- Using the new inject() pattern for dependency injection
- Adopting signals and computed values
- Organizing imports logically
Also applies to: 19-24
33-40: Clean dependency injection implementation!Consistent use of the inject() pattern for all dependencies makes the code more maintainable and testable.
50-51: Consider implementing course-level IRIS checkThe current implementation only checks the global IRIS profile. As suggested in previous reviews, consider using
IrisEnabledComponentto ensure IRIS features are only shown when enabled for the specific course.- irisEnabled = toSignal(this.profileService.getProfileInfo().pipe( - map((profileInfo) => profileInfo.activeProfiles.includes(PROFILE_IRIS)) - ), { initialValue: false }); + irisEnabled = toSignal( + combineLatest([ + this.profileService.getProfileInfo(), + this.irisSettingsService.getCourseSettings(this.courseId) + ]).pipe( + map(([profileInfo, courseSettings]) => + profileInfo.activeProfiles.includes(PROFILE_IRIS) && courseSettings.enabled + ) + ), + { initialValue: false } + );
50-50: Consider adding cleanup for profile subscriptionWhile using
toSignalis good, it's important to clean up subscriptions when the component is destroyed.Implement
OnDestroyand clean up the subscription:-export class FaqUpdateComponent implements OnInit { +export class FaqUpdateComponent implements OnInit, OnDestroy { + private readonly destroy = new DestroyRef(); + irisEnabled = toSignal( - this.profileService.getProfileInfo().pipe( + this.profileService.getProfileInfo().pipe( + takeUntilDestroyed(this.destroy), map((profileInfo) => profileInfo.activeProfiles.includes(PROFILE_IRIS)) ), { initialValue: false } ); + + ngOnDestroy(): void { + // Angular's DI handles cleanup of DestroyRef + }src/main/webapp/app/shared/monaco-editor/rewriting.service.ts (1)
1-13: LGTM! Clean imports and well-documented class.The imports are organized logically, and the class documentation clearly explains its purpose.
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts (1)
315-317: Enhance error handling.The current implementation only logs errors to the console. Consider providing user feedback through the alert service.
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts
Outdated
Show resolved
Hide resolved
sachmii
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
github-actions
bot
added
deployment-error
cremertim
removed
the
deployment-error
github-actions
bot
added
deployment-error
sawys777
added
deploy:artemis-test5
and removed
deployment-error
sawys777
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
Cathy0123456789
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Note
This PR depends on the counterpart in IRIS. For testing, deploy Pyris#191 via
deploy:pyris-testif it is not already deployed (lock:pyris-test). You can use TS1, TS3, TS5 for testing since it has Iris enabled.Checklist
General
Server
Client
Motivation and Context
With new AI tools, there is no need for any grammer mistakes any longer. Consequently, we want to support automatik grammer checking
Description
I added a pipeline to IRIS to rephrase the content of an markdown editor via EditorAction
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
Create._.Test.Course.Tim.Cremer.-.Google.Chrome.2025-01-17.15-43-54.mp4
Summary by CodeRabbit
Release Notes: Artemis Intelligence Text Rewriting Feature
New Features
Improvements
Technical Enhancements
User Experience