Development: Remove the unused Ace code editor component
#8832
Conversation
github-actions
bot
added
tests
client
WalkthroughThe changes involve refactoring the codebase to replace instances of the Ace code editor with the Monaco code editor. This includes updating import statements, modifying HTML components, and adjusting TypeScript logic to remove dependencies on Ace and integrate Monaco instead. This streamlines the editor usage across various components, ensuring consistency and improving maintainability by standardizing on a single code editor framework. Changes
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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: 3
Outside diff range comments (40)
src/main/webapp/app/entities/build-log.model.ts (2)
Line range hint
12-12: Replace theanytype with a more specific type to enhance type safety and code quality.- time: any; + time: string; // Assuming ISO 8601 format, adjust as necessary
Line range hint
78-80: Address linting issues regarding non-null assertions and global functions.- feedback.getReferenceLine(suggestion)! + feedback.getReferenceLine(suggestion) ?? 0 - parseInt(row, 10) + Number.parseInt(row, 10) - parseInt(column, 10) + Number.parseInt(column, 10)Also applies to: 89-90
src/main/webapp/app/shared/markdown-editor/ace-editor/ace-editor.component.ts (3)
Line range hint
67-67: ReplaceInfinitywithNumber.Infinityfor consistency with modern JavaScript standards.- this._editor.$blockScrolling = Infinity; + this._editor.$blockScrolling = Number.Infinity;
Line range hint
200-200: Use strict equality===to avoid unexpected type coercion.- if (text == undefined) { + if (text === undefined) {
Line range hint
201-201: Avoid reassigning function parameters as it can lead to confusing bugs. Use a local variable instead.- setText(text: string) { + setText(inputText: string) { + let text = inputText;src/main/webapp/app/exercises/programming/shared/code-editor/build-output/code-editor-build-output.component.ts (5)
Line range hint
35-35: Typeanyshould be avoided as it bypasses TypeScript's type checking. Consider specifying a more precise type for better type safety and code maintainability.- function theWindow(): any { + function theWindow(): Window {
Line range hint
95-95: Avoid using assignments within expressions as they can lead to code that is difficult to read and maintain.- tap((result) => (this.result = result)), + tap((result) => { this.result = result; }),Apply this change to all similar occurrences in the file.
Also applies to: 144-144, 161-161
Line range hint
97-97: Usage of the non-null assertion operator (!) bypasses TypeScript's nullability checks. It's safer to handle potential nulls explicitly or use optional chaining (?.) if the context allows.- this.result!.completionDate + this.result?.completionDateApply this change to all similar occurrences where non-null assertions are used.
Also applies to: 121-121, 123-123, 125-125, 157-157, 161-161, 181-181
Line range hint
207-209: Theelseclause is unnecessary here as the previous branches return early. Simplifying the control flow improves readability.- } else { - if (!this.resultSubscription && this.participation) { - this.setupResultWebsocket(); - } - } + if (!this.resultSubscription && this.participation) { + this.setupResultWebsocket(); + }
Line range hint
217-217: The usage ofanyshould be avoided. Specifying a more precise type enhances type safety and code clarity.- function theWindow(): any { + function theWindow(): Window {src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (2)
Line range hint
62-62: Type annotations for properties initialized with a boolean or string are inferred by TypeScript and thus redundant.- private _readOnly: boolean = false; + private _readOnly = false;Apply this change to all similar occurrences.
Also applies to: 226-226
Line range hint
262-262: Prefer template literals over string concatenation for better readability and maintainability.- 'glyph-margin-hover-button-' + this._editor.getId() + `glyph-margin-hover-button-${this._editor.getId()}`src/main/webapp/app/shared/orion/orion-connector.service.ts (3)
Line range hint
16-16: The use ofanyshould be avoided. Consider providing a more specific type to enhance type safety.- function theWindow(): any { + function theWindow(): Window {
Line range hint
214-214: Avoid using non-null assertions. Consider using optional chaining to safely access properties.- this.activeAssessmentComponent!.updateFeedback(submissionId, feedbackAsArray); + this.activeAssessmentComponent?.updateFeedback(submissionId, feedbackAsArray);
Line range hint
150-150: Avoid using the spread operator in reducers as it can lead to performance issues due to itsO(n^2)complexity. Use other methods like.pushor direct assignment.- [fileName]: [...(buildLogErrors[fileName] || []), { ...rest, ts: timestamp }], + if (!buildLogErrors[fileName]) { + buildLogErrors[fileName] = []; + } + buildLogErrors[fileName].push({ ...rest, ts: timestamp });src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.component.spec.ts (1)
Line range hint
96-96: Avoid using non-null assertions. Consider using optional chaining to safely access properties.- comp.glyphMarginHoverButton!.moveAndUpdate(1); + comp.glyphMarginHoverButton?.moveAndUpdate(1);Apply this change to all similar occurrences where non-null assertions are used.
Also applies to: 190-190, 216-216
src/main/webapp/app/exercises/programming/shared/code-editor/container/code-editor-container.component.ts (2)
Line range hint
198-198: Remove the non-null assertion to adhere to best practices and avoid potential runtime errors.- this.monacoEditor!.onFileChange(fileChange); + this.monacoEditor.onFileChange(fileChange);
Line range hint
254-254: Type oferrorshould be specified instead of usingany. Consider defining a type that includes possible error values.interface ErrorDetails { translationKey: string; connectionIssue?: string; } onError(error: ErrorDetails) { // Implementation remains the same }src/test/javascript/spec/integration/code-editor/code-editor-student.integration.spec.ts (1)
Line range hint
191-191: The type oferrorshould be specified instead of usingany. Consider defining a type that includes possible error values.interface ErrorDetails { message: string; code?: number; } onError(error: ErrorDetails) { // Implementation remains the same }src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.ts (9)
Line range hint
169-169: Use template literals for string concatenation to enhance readability and maintain consistency with modern JavaScript practices.- this.onError.emit('loadingFailed' + error.message); + this.onError.emit(`loadingFailed${error.message}`);
Line range hint
306-306: Replace string concatenation with template literals to align with modern JavaScript standards.- this.editor.addLineWidget(line + 1, 'feedback-new-' + line, feedbackNode); + this.editor.addLineWidget(line + 1, `feedback-new-${line}`, feedbackNode);
Line range hint
323-323: Use template literals for more efficient and readable string concatenation.- this.editor.addLineWidget(line + 1, 'feedback-' + feedback.id, feedbackNode); + this.editor.addLineWidget(line + 1, `feedback-${feedback.id}`, feedbackNode);
Line range hint
339-339: Adopt template literals to improve the readability of string operations.- throw new Error('No line found for feedback ' + feedback.id); + throw new Error(`No line found for feedback ${feedback.id}`);
Line range hint
344-344: Convert string concatenation to template literals for cleaner code.- this.editor.addLineWidget(line + 1, 'feedback-' + feedback.id, feedbackNode); + this.editor.addLineWidget(line + 1, `feedback-${feedback.id}`, feedbackNode);
Line range hint
393-393: Template literals are preferred over concatenation for constructing strings.- throw new Error('No feedback node found at line ' + line); + throw new Error(`No feedback node found at line ${line}`);
Line range hint
405-405: Opt for template literals to simplify string construction in JavaScript.- const hash = a.fileName + a.row + a.column + a.text; + const hash = `${a.fileName}${a.row}${a.column}${a.text}`;
Line range hint
413-413: Use strict equality comparison (===) instead of loose equality (==) to avoid type coercion and potential bugs.- if (sessionAnnotations[hash] == undefined || sessionAnnotations[hash].timestamp < a.timestamp) { + if (sessionAnnotations[hash] === undefined || sessionAnnotations[hash].timestamp < a.timestamp) {
Line range hint
415-417: Theelseclause is unnecessary here because theifcondition already covers all possibilities.- } else { - return sessionAnnotations[hash]; - }src/test/javascript/spec/component/code-editor/code-editor-monaco.component.spec.ts (4)
Line range hint
86-86: Non-null assertion usage should be avoided for safer code practices.- expect(element!.hidden).toBeTrue(); + expect(element?.hidden).toBeTrue();
Line range hint
105-105: Replace non-null assertion with optional chaining for safer code.- expect(element!.hidden).toBeTrue(); + expect(element?.hidden).toBeTrue();
Line range hint
116-116: Use optional chaining instead of non-null assertion.- expect(element!.hidden).toBeFalse(); + expect(element?.hidden).toBeFalse();
Line range hint
122-126: Avoid using assignments within expressions to reduce confusion and potential bugs.- comp.ngOnChanges({}); + const changes = {}; + comp.ngOnChanges(changes);src/test/javascript/spec/integration/code-editor/code-editor-instructor.integration.spec.ts (4)
Line range hint
86-86: Replace non-null assertion with optional chaining for safer code.- expect(container.codeEditorContainer.grid).toBeDefined(); + expect(container.codeEditorContainer?.grid).toBeDefined();
Line range hint
105-105: Use optional chaining instead of non-null assertion.- expect(container.codeEditorContainer.fileBrowser).toBeDefined(); + expect(container.codeEditorContainer?.fileBrowser).toBeDefined();
Line range hint
116-116: Non-null assertion usage should be avoided for safer code practices.- expect(container.codeEditorContainer.actions).toBeDefined(); + expect(container.codeEditorContainer?.actions).toBeDefined();
Line range hint
122-126: Avoid using assignments within expressions to reduce confusion and potential bugs.- expect(container.codeEditorContainer.buildOutput).toBeDefined(); + const buildOutput = container.codeEditorContainer?.buildOutput; + expect(buildOutput).toBeDefined();src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts (2)
Line range hint
206-206: Replace the use ofanyto ensure type safety. Consider specifying a more explicit type or using generics if applicable.- const someVariable: any = ...; + const someVariable: SpecificType = ...; // Replace SpecificType with the actual type
Line range hint
256-256: Replace non-null assertions with optional chaining to avoid potential runtime errors.- someObject!.someProperty + someObject?.somePropertyAlso applies to: 332-332, 442-442, 454-454, 498-498, 510-510
src/main/webapp/app/exercises/programming/assess/code-editor-tutor-assessment-container.component.ts (2)
Line range hint
105-105: Specify explicit types instead of usingany.Consider replacing
anywith specific types to enhance type safety and code maintainability. TypeScript's type system can help catch many potential runtime errors at compile time.Also applies to: 106-106, 110-110
Line range hint
140-140: Avoid assignments within expressions to enhance clarity.- const examId = params['examId']; + const examId = Number(params['examId']); - if (examId) { + if (examId !== undefined) {This change clarifies the intent of the code by separating the assignment from the condition, making the code easier to read and understand.
Also applies to: 141-141, 182-182
.../webapp/app/exercises/programming/assess/code-editor-tutor-assessment-container.component.ts
Show resolved
Hide resolved
.../webapp/app/exercises/programming/assess/code-editor-tutor-assessment-container.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/markdown-editor/ace-editor/ace-editor.component.ts
Show resolved
Hide resolved
Checklist
General
Client
Motivation and Context
The CodeEditorAceComponent was only used in the code editor container if a flag
useMonacoEditorwasfalse. However, the flag wastruefor all instances of the code editor container, rendering it unused.Description
Steps for Testing
Code review
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
Test Coverage
No non-trivial changes
Screenshots
No UI changes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests