Programming exercises: Change the problem statement editor to Monaco
#8856
Conversation
…problem-statement-monaco
…problem-statement-monaco
WalkthroughThe changes introduce a range of new actions for the Monaco editor in Artemis, enhancing its functionality for text formatting, inserting elements, and managing editor modes. These actions include toggling text styles (italic, underline, heading), handling list types (ordered, unordered), inserting specific elements (URLs, tasks, test cases), and adjusting the editor's display mode (fullscreen). Each new action is encapsulated in its own class and builds on a shared base of domain action models. Changes
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional context usedPath-based instructions (1)
Biome
Additional comments not posted (10)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range comments (20)
src/test/javascript/spec/jest-test-setup.ts (1)
Line range hint
9-75: Mock Implementations and Global AssignmentsThe setup file for Jest tests includes various mocks and global assignments to ensure the tests run smoothly with the Monaco editor. These setups are necessary but ensure that they do not interfere with other tests or global settings.
However, consider isolating these mocks to specific test suites where they are needed to avoid potential side effects in unrelated tests.
src/main/webapp/app/exercises/programming/manage/code-editor/code-editor-instructor-and-editor-container.component.ts (1)
Line range hint
1-47: Unnecessary ConstructorThe constructor in
CodeEditorInstructorAndEditorContainerComponentdoes not perform any custom logic and merely calls the superclass constructor. This could be omitted for cleaner code.- constructor( - router: Router, - exerciseService: ProgrammingExerciseService, - courseExerciseService: CourseExerciseService, - domainService: DomainService, - programmingExerciseParticipationService: ProgrammingExerciseParticipationService, - location: Location, - participationService: ParticipationService, - route: ActivatedRoute, - alertService: AlertService, - ) { - super(router, exerciseService, courseExerciseService, domainService, programmingExerciseParticipationService, location, participationService, route, alertService); - }src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts (3)
Line range hint
16-16: Remove unnecessary type annotation.The type annotation on line 16 is inferred and thus redundant. Simplifying this can improve code readability.
- protected readonly AssessmentType = AssessmentType; + protected readonly AssessmentType;
Line range hint
55-55: Use strict equality checks.Using
==can lead to unexpected type coercion. It is safer to use===for comparisons to ensure type safety.- this.exercise.dueDate == undefined + this.exercise.dueDate === undefinedAlso applies to: 56-56
Line range hint
71-71: Specify a more explicit type thanany.Using
anydisables TypeScript's type checking. For better code safety and maintainability, specify a more explicit type.src/main/webapp/app/orion/management/code-editor-instructor-and-editor-orion-container.component.ts (2)
Line range hint
55-55: Avoid assignments within expressions.Assignments within expressions can lead to code that is hard to read and maintain. Consider separating this assignment to enhance clarity.
- this.orionConnectorService.state().subscribe((state) => (this.orionState = state)); + this.orionConnectorService.state().subscribe((state) => { + this.orionState = state; + });
Line range hint
58-58: Use more specific types instead ofany.The use of
anycan lead to potential bugs and less maintainable code by bypassing TypeScript's type system. Consider defining a more specific type.src/main/webapp/app/orion/management/code-editor-instructor-and-editor-orion-container.component.html (1)
Line range hint
1-86: Ensure usage of new Angular syntax and check for accessibility enhancements.The file uses the new
@ifdirective correctly. However, consider addingaria-liveattributes to dynamically updated regions to improve accessibility, especially where loading states and errors are displayed.Enhancing accessibility can improve the usability of the platform for all users, including those using assistive technologies.
src/main/webapp/app/exercises/programming/shared/code-editor/layout/code-editor-grid.scss (1)
Line range hint
22-299: Review and optimize grid and flexbox usage for editor layout.The CSS for the editor layout uses grid and flexbox effectively. However, consider using CSS variables for repeated values like margins and padding to enhance maintainability.
Using CSS variables can help in making the stylesheet easier to maintain and update. For example:
- .editor-sidebar-right { - width: 500px; - } + :root { + --sidebar-right-width: 500px; + } + .editor-sidebar-right { + width: var(--sidebar-right-width); + }src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-task.extension.ts (1)
[!TIP]
Codebase VerificationAddress type safety and improve null handling in TypeScript file.
The use of non-null assertions (
!) can lead to runtime errors if not handled properly. Consider using optional chaining or proper checks before accessing properties.
- Replace
this.exercise.id!withthis.exercise?.idto ensure type safety.- Ensure that
testCasesis properly initialized before use to prevent runtime errors.Locations to review:
src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-task.extension.tsAnalysis chain
Line range hint
25-143: Address type safety and improve null handling in TypeScript file.The use of non-null assertions (!) can lead to runtime errors if not handled properly. Consider using optional chaining or proper checks before accessing properties.
- this.exercise.id! + this.exercise?.idAlso, ensure that variables like
testCasesare initialized before use to prevent runtime errors.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify initialization of `testCases` before use. # Test: Search for the initialization of `testCases`. Expect: Initialization before use. rg --type typescript $'this.testCases ='Length of output: 76
Script:
#!/bin/bash # Description: Verify initialization of `testCases` before use. # Test: Search for the initialization of `testCases`. Expect: Initialization before use. rg 'this.testCases ='Length of output: 1802
src/main/webapp/app/exercises/programming/manage/code-editor/code-editor-instructor-and-editor-container.component.html (1)
Line range hint
1-136: Ensure proper use of Angular directives and improve error handling in HTML template.The use of
@ifand other directives is consistent with Angular best practices. However, consider adding more descriptive error messages and handling for edge cases in repository selection and participation management.Improving error handling and user feedback can greatly enhance the user experience. For example, adding timeouts or retries for loading states could prevent the UI from being stuck in an error state.
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.component.spec.ts (1)
Line range hint
96-96: Avoid Non-Null AssertionsNon-null assertions can lead to runtime errors if the assumed non-null values are actually null. It's better to handle these cases explicitly.
- const button: MonacoEditorGlyphMarginHoverButton = comp.glyphMarginHoverButton!; - const fullscreenElement = element ?? editor.getDomNode()!; - const modelDisposeSpy = jest.spyOn(model!, 'dispose'); + // Add checks to ensure values are not null before using them + const button = comp.glyphMarginHoverButton; + if (!button) throw new Error('Glyph margin hover button is not initialized.'); + const fullscreenElement = element ?? editor.getDomNode(); + if (!fullscreenElement) throw new Error('Fullscreen element is not available.'); + const modelDisposeSpy = model ? jest.spyOn(model, 'dispose') : undefined; + if (!modelDisposeSpy) throw new Error('Model is not initialized.');Also applies to: 190-190, 216-216
src/main/webapp/app/exercises/programming/manage/instructions-editor/programming-exercise-editable-instruction.component.ts (4)
Line range hint
141-141: Type specification needed instead of 'any'.Using 'any' type can lead to potential bugs and less maintainable code by bypassing TypeScript's static typing. It's advisable to specify a more explicit type.
- let someVariable: any; + let someVariable: SpecificType; // Replace 'SpecificType' with the actual expected type
Line range hint
145-145: Avoid using non-null assertion operators.Non-null assertions may lead to runtime errors if not handled carefully. It's safer to handle potential nulls gracefully using conditional checks or optional chaining.
- this.someNullableProperty!.someMethod(); + this.someNullableProperty?.someMethod();Also applies to: 200-200, 207-207, 212-212, 238-238
Line range hint
210-213: Redundant else clause detected.The 'else' clause here is unnecessary as the previous branches of conditionals use 'return' statements, which already exit the function scope when met. Removing it can simplify the code flow and improve readability.
- else { - // some code - }
Line range hint
261-261: Use template literals for string operations.Template literals provide a more readable and concise way to handle complex string operations compared to traditional string concatenation.
- const message = 'Error: ' + error.message; + const message = `Error: ${error.message}`;src/main/webapp/app/exercises/programming/shared/code-editor/container/code-editor-container.component.ts (2)
Line range hint
200-200: Avoid non-null assertion operator.Using non-null assertion operators can lead to runtime errors if the value is null or undefined. Consider checking for nullity before usage or using optional chaining.
- this.selectedFile = this.fileService.updateFileReference(this.selectedFile!, fileChange); + const updatedFile = this.fileService.updateFileReference(this.selectedFile, fileChange); + if (updatedFile) { + this.selectedFile = updatedFile; + }
Line range hint
258-258: Specify a more accurate type thanany.Using
anycan lead to errors that TypeScript's type system could otherwise prevent. Specify a more detailed type forerrorto enhance code robustness and maintainability.- onError(error: any) { + onError(error: string | ConnectionError) {Also applies to: 297-297
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (1)
Line range hint
308-308: Use template literals for string concatenation.Template literals provide a more readable and concise syntax compared to traditional string concatenation.
- this.glyphMarginHoverButton = new MonacoEditorGlyphMarginHoverButton(this._editor, 'glyph-margin-hover-button-' + this._editor.getId(), domNode, clickCallback); + this.glyphMarginHoverButton = new MonacoEditorGlyphMarginHoverButton(this._editor, `glyph-margin-hover-button-${this._editor.getId()}`, domNode, clickCallback);src/test/javascript/spec/component/programming-exercise/programming-exercise-editable-instruction.component.spec.ts (1)
Line range hint
175-175: Replace non-null assertion with optional chaining.Non-null assertions can lead to runtime errors if the value is null. Use optional chaining to safely access properties of potentially null objects.
- getLatestResultWithFeedbacksStub.mockReturnValue(subject); + getLatestResultWithFeedbacksStub?.mockReturnValue(subject);
src/main/webapp/app/shared/monaco-editor/model/actions/monaco-heading.action.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/monaco-heading.action.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/languages/monaco-custom-markdown.language.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/monaco-underline.action.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/monaco-unordered-list.action.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/monaco-formula.action.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/monaco-color.action.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/monaco-url.action.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/monaco-attachment.action.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts
Outdated
Show resolved
Hide resolved
egekurt123
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
az108
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
|
Tested on TS6 and everything works as expected but I found this small visual issue while navigating trough the repositories. |
|
@az108 Thanks for testing & reporting 😃. I was able to reproduce this on TS1 without this PR deployed. I'd say we can solve this in a follow-up as I didn't change this 👍 |
Checklist
General
Client
Changes affecting Programming Exercises
Motivation and Context
We want to replace the Ace code editor in Artemis. The largest and most-used component using Ace is the Markdown editor (used in several cases, e.g. programming exercises).
Description
Known issues:
Regarding the security warnings: This is code we would have used indirectly anyway (it's taken from the Monaco editor source code). DoS is unlikely to be an issue for the problem statements we have and HTML parsing is not supposed to be a big part of our custom markdown language.
Steps for Testing
Prerequisites:
Edit in editor
Edit (regular)
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
Client
Screenshots
Summary by CodeRabbit