-
Notifications
You must be signed in to change notification settings - Fork 349
Programming exercises: Prevent online code editor from overwriting other files during build/refresh
#11418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Programming exercises: Prevent online code editor from overwriting other files during build/refresh
#11418
Conversation
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
…udent-submissions
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
WalkthroughText-change events from Monaco editors now emit objects Changes
Sequence Diagram(s)sequenceDiagram
participant ME as MonacoEditorComponent
participant CEM as CodeEditorMonacoComponent
participant MEM as MarkdownEditorMonacoComponent
participant PECB as ProgrammingExerciseCustomBuildPlanComponent
participant PEAB as ProgrammingExerciseCustomAeolusBuildPlanComponent
Note over ME: User edits in Monaco
ME->>ME: extractFilePathFromModel(model)
ME-->>CEM: textChanged { text, fileName }
ME-->>MEM: textChanged { text, fileName }
rect rgba(200,230,255,0.3)
Note over CEM: File-specific update
CEM->>CEM: Update fileSession[fileName].content = text
CEM-->>PECB: onFileContentChange { text, fileName }
CEM-->>PEAB: onFileContentChange { text, fileName }
end
rect rgba(200,255,200,0.3)
Note over MEM: Forward text only
MEM->>MEM: Read event.text
MEM-->>PECB: text output = text
end
PECB->>PECB: codeChanged(input: string | {text,fileName}) -> resolve text
PEAB->>PEAB: codeChanged(event: {text,fileName}) uses event.text
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-aeolus-build-plan.component.ts (1)
175-178: Align codeChanged signature and state update with sibling componentFor consistency and backward-compatibility with other call sites, accept both string and object payloads and keep this.code in sync.
Apply:
- codeChanged(event: { text: string; fileName: string }): void { - if (this.active instanceof ScriptAction) { - (this.active as ScriptAction).script = event.text; - } - } + codeChanged(codeOrEvent: string | { text: string; fileName: string }): void { + const code = typeof codeOrEvent === 'string' ? codeOrEvent : codeOrEvent.text; + this.code = code; + if (this.active instanceof ScriptAction) { + (this.active as ScriptAction).script = code; + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-aeolus-build-plan.component.spec.ts(2 hunks)src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-aeolus-build-plan.component.ts(1 hunks)src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-build-plan.component.ts(1 hunks)src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.spec.ts(1 hunks)src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts(1 hunks)src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.spec.ts(1 hunks)src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/monaco-editor.component.spec.ts(2 hunks)src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts(2 hunks)src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/**/*.ts
⚙️ CodeRabbit configuration file
Files:
src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-aeolus-build-plan.component.tssrc/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.spec.tssrc/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.tssrc/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.spec.tssrc/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-build-plan.component.tssrc/main/webapp/app/shared/monaco-editor/monaco-editor.component.spec.tssrc/main/webapp/app/shared/monaco-editor/monaco-editor.component.tssrc/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-aeolus-build-plan.component.spec.tssrc/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts
src/test/javascript/spec/**/*.ts
⚙️ CodeRabbit configuration file
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}}
Files:
src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts
🧠 Learnings (9)
📚 Learning: 2024-10-25T07:52:32.513Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9580
File: src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts:475-491
Timestamp: 2024-10-25T07:52:32.513Z
Learning: In `src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts`, `useCommunicationForFileUpload` is a Signal, so it should be called as a function when used.
Applied to files:
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.spec.tssrc/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.tssrc/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.spec.tssrc/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts
📚 Learning: 2024-10-13T12:03:02.430Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9463
File: src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts:50-55
Timestamp: 2024-10-13T12:03:02.430Z
Learning: In `src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts`, when a function is called multiple times in a test, use `toHaveBeenCalledTimes` and `toHaveBeenNthCalledWith` assertions instead of `toHaveBeenCalledExactlyOnceWith`.
Applied to files:
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.spec.tssrc/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.spec.tssrc/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.tssrc/main/webapp/app/shared/monaco-editor/monaco-editor.component.spec.ts
📚 Learning: 2024-10-08T15:35:42.972Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9431
File: angular.json:116-116
Timestamp: 2024-10-08T15:35:42.972Z
Learning: In `angular.json`, the path `./node_modules/monaco-editor/min/vs` is used to provide CSS styles to the client and can remain unchanged when updating `monaco-editor`.
Applied to files:
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.spec.tssrc/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts
📚 Learning: 2024-10-07T17:50:03.674Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9431
File: src/main/webapp/app/core/config/monaco.config.ts:7-7
Timestamp: 2024-10-07T17:50:03.674Z
Learning: In the file 'src/main/webapp/app/core/config/monaco.config.ts', which is an mjs file, TypeScript type annotations should not be added.
Applied to files:
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.spec.ts
📚 Learning: 2024-10-20T21:59:11.630Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9505
File: src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html:9-9
Timestamp: 2024-10-20T21:59:11.630Z
Learning: In Angular templates within the Artemis project (e.g., `src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html`), properties like `selectedFile()`, `readOnlyManualFeedback()`, `highlightDifferences()`, and `course()` are signals. It is appropriate to call these signals directly in the template.
Applied to files:
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts
📚 Learning: 2024-10-02T06:53:22.192Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9407
File: src/test/javascript/spec/component/programming-exercise/programming-exercise-custom-aeolus-build-plan.component.spec.ts:172-175
Timestamp: 2024-10-02T06:53:22.192Z
Learning: When testing components that use `MonacoEditorComponent`, it's acceptable not to test `MonacoEditorComponent` functionality within those tests, as dedicated tests already exist for it.
Applied to files:
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.spec.tssrc/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts
📚 Learning: 2024-10-20T22:00:52.335Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9505
File: src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts:179-181
Timestamp: 2024-10-20T22:00:52.335Z
Learning: In `src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts`, `ResizeObserver` is mocked within the `beforeEach` block.
Applied to files:
src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts
📚 Learning: 2024-10-10T11:42:23.069Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9443
File: src/test/javascript/spec/component/hestia/git-diff-report/git-diff-modal.component.spec.ts:55-60
Timestamp: 2024-10-10T11:42:23.069Z
Learning: In `git-diff-report-modal.component.spec.ts`, using `fakeAsync` and `tick` does not work for handling asynchronous operations in the tests; alternative methods are needed.
Applied to files:
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.spec.ts
📚 Learning: 2025-09-01T10:20:40.706Z
Learnt from: Michael-Breu-UIbk
PR: ls1intum/Artemis#10989
File: src/main/webapp/app/programming/manage/detail/programming-exercise-detail.component.with-sharing.spec.ts:132-149
Timestamp: 2025-09-01T10:20:40.706Z
Learning: In the Artemis codebase, Angular component test files for ProgrammingExerciseDetailComponent follow a pattern where the component is imported but not explicitly declared in TestBed.configureTestingModule(), yet TestBed.createComponent() still works successfully. This pattern is consistently used across test files like programming-exercise-detail.component.spec.ts and programming-exercise-detail.component.with-sharing.spec.ts.
Applied to files:
src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-aeolus-build-plan.component.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (10)
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts (1)
448-451: Event-shaped onTextChanged is correctly handledUsing event.text to update and emit markdown is consistent with the new payload shape.
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.spec.ts (1)
82-85: Spec aligned with new event payloadTest correctly calls onTextChanged with { text, fileName } and asserts the emitted text.
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.spec.ts (1)
142-147: Spec updated for per-file text changeonFileTextChanged invocation now passes { text, fileName }; assertions remain valid.
src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts (1)
323-332: Integration test uses the new event shapeContainer interaction reflects { text, fileName } and state assertions remain correct.
src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-aeolus-build-plan.component.spec.ts (2)
134-139: Test adjusted to object payloadcodeChanged is invoked with { text, fileName } and ScriptAction.script is asserted accordingly.
167-176: Second test similarly alignedInvocation and expectations match the new API.
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.spec.ts (2)
58-59: Expectation updated to object emissionTest now expects { text, fileName } from textChanged; correct.
71-72: Debounced emission assertion updatedSingle debounced emit with the new payload shape looks good.
src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-build-plan.component.ts (1)
131-136: Flexible handler and state updates look goodAccepting string | object and updating code, editor, and buildScript is consistent and robust.
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (1)
59-59: API shape change looks good.Switching to
{ text, fileName }fortextChangedaligns with the PR’s objective.
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts
Outdated
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts (2)
216-237: Minor: reduce nesting with early returns and tighten reads.Simplifies control flow and avoids reading scrollTop unless we actually update.
- onFileTextChanged(event: { text: string; fileName: string }): void { - const { text, fileName } = event; - // Apply text change to the specific file it belongs to, not the currently selected file - if (fileName && this.fileSession()[fileName]) { - const previousText = this.fileSession()[fileName].code; - const previousScrollTop = this.fileSession()[fileName].scrollTop; - - if (previousText !== text) { - this.fileSession.set({ - ...this.fileSession(), - [fileName]: { - code: text, - loadingError: false, - scrollTop: previousScrollTop, - cursor: fileName === this.selectedFile() ? this.editor().getPosition() : this.fileSession()[fileName].cursor, - }, - }); - - this.onFileContentChange.emit({ file: fileName, fileContent: text }); - } - } - } + onFileTextChanged(event: { text: string; fileName: string }): void { + const { text, fileName } = event; + if (!fileName || !this.fileSession()[fileName]) { + return; + } + const previousText = this.fileSession()[fileName].code; + if (previousText === text) { + return; + } + const previousScrollTop = this.fileSession()[fileName].scrollTop; + this.fileSession.set({ + ...this.fileSession(), + [fileName]: { + code: text, + loadingError: false, + scrollTop: previousScrollTop, + cursor: fileName === this.selectedFile() ? this.editor().getPosition() : this.fileSession()[fileName].cursor, + }, + }); + this.onFileContentChange.emit({ file: fileName, fileContent: text }); + }
216-216: Optional: introduce a shared event type for reuse.Define and reuse a type (improves readability and code reuse).
Example near other types:
type FileTextChangeEvent = { text: string; fileName: string };Then change signature:
onFileTextChanged(event: FileTextChangeEvent): void {As per coding guidelines (code_reuse:true).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/**/*.ts
⚙️ CodeRabbit configuration file
Files:
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.tssrc/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts
🧠 Learnings (4)
📚 Learning: 2024-10-25T07:52:32.513Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9580
File: src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts:475-491
Timestamp: 2024-10-25T07:52:32.513Z
Learning: In `src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts`, `useCommunicationForFileUpload` is a Signal, so it should be called as a function when used.
Applied to files:
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.tssrc/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts
📚 Learning: 2024-10-20T21:59:11.630Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9505
File: src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html:9-9
Timestamp: 2024-10-20T21:59:11.630Z
Learning: In Angular templates within the Artemis project (e.g., `src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html`), properties like `selectedFile()`, `readOnlyManualFeedback()`, `highlightDifferences()`, and `course()` are signals. It is appropriate to call these signals directly in the template.
Applied to files:
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts
📚 Learning: 2024-10-08T15:35:42.972Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9431
File: angular.json:116-116
Timestamp: 2024-10-08T15:35:42.972Z
Learning: In `angular.json`, the path `./node_modules/monaco-editor/min/vs` is used to provide CSS styles to the client and can remain unchanged when updating `monaco-editor`.
Applied to files:
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts
📚 Learning: 2024-10-20T22:04:46.906Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9505
File: src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html:36-36
Timestamp: 2024-10-20T22:04:46.906Z
Learning: In Angular templates, methods that return Signals (e.g., `feedbackSuggestionsForSelectedFile()` in `code-editor-monaco.component.ts`) can be used directly without performance issues because Signals are optimized for efficient change detection.
Applied to files:
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: client-tests
- GitHub Check: client-tests-selected
- GitHub Check: server-style
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyse
🔇 Additional comments (4)
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts (1)
216-237: Good fix: apply edits to originating file and preserve non-selected cursors.
- Using event.fileName to target updates fixes the race-condition overwrite.
- Conditional cursor update avoids clobbering cursors for non-selected files.
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (3)
201-213: File path extraction from in‑memory URI is correctCorrectly parses
/model/<editorId>/<full/file/path>and returns the full path; sensible fallback included.
185-197: Global debounce is shared across models; updates get dropped when switching filesUsing one
textChangedEmitTimeoutcancels pending emissions from file A when typing in file B, losing A’s last edits. Maintain a per-model debounce keyed bymodel.urito preserve each model’s trailing change.Apply within this hunk:
- if (!delay) { - this.textChanged.emit({ text: newValue, fileName: fullFilePath }); - } else { - if (this.textChangedEmitTimeout) { - clearTimeout(this.textChangedEmitTimeout); - this.textChangedEmitTimeout = undefined; - } - this.textChangedEmitTimeout = setTimeout(() => { - this.textChanged.emit({ text: newValue, fileName: fullFilePath }); - }, delay); - } + if (!delay) { + this.textChanged.emit({ text: newValue, fileName: fullFilePath }); + return; + } + const modelKey = model?.uri?.toString() ?? ''; + const existing = this.textChangedEmitTimeouts.get(modelKey); + if (existing) { + clearTimeout(existing); + } + const payload = { text: newValue, fileName: fullFilePath }; + const timeoutId = setTimeout(() => { + this.textChanged.emit(payload); + this.textChangedEmitTimeouts.delete(modelKey); + }, delay); + this.textChangedEmitTimeouts.set(modelKey, timeoutId);Add outside this hunk:
- A per-model map:
private textChangedEmitTimeouts = new Map<string, ReturnType<typeof setTimeout>>();
- Optional cleanup (memory_leak_prevention): clear pending timeouts in
ngOnDestroy():for (const t of this.textChangedEmitTimeouts.values()) clearTimeout(t); this.textChangedEmitTimeouts.clear();As per coding guidelines (memory_leak_prevention:true).
59-59: Fix build-plan-editor onTextChanged to handle new payload shape
- In build-plan-editor.component.ts, change onTextChanged(event: any) to onTextChanged(event: { text: string; fileName: string }) and assign event.text instead of casting the entire event to string.
- Update its spec (build-plan-editor.component.spec.ts) to call comp.onTextChanged({ text: 'new text', fileName: '…' }) and assert that buildPlan.buildPlan equals event.text.
⛔ Skipped due to learnings
Learnt from: pzdr7 PR: ls1intum/Artemis#9580 File: src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts:475-491 Timestamp: 2024-10-25T07:52:32.513Z Learning: In `src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts`, `useCommunicationForFileUpload` is a Signal, so it should be called as a function when used.Learnt from: pzdr7 PR: ls1intum/Artemis#9431 File: src/main/webapp/app/core/config/monaco.config.ts:7-7 Timestamp: 2024-10-07T17:50:03.674Z Learning: In the file 'src/main/webapp/app/core/config/monaco.config.ts', which is an mjs file, TypeScript type annotations should not be added.Learnt from: pzdr7 PR: ls1intum/Artemis#9463 File: src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts:50-55 Timestamp: 2024-10-13T12:03:02.430Z Learning: In `src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts`, when a function is called multiple times in a test, use `toHaveBeenCalledTimes` and `toHaveBeenNthCalledWith` assertions instead of `toHaveBeenCalledExactlyOnceWith`.
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
ekayandan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS1. Used TS6 for reproducing the bug.
I was able to reproduce it almost each time on faulty version. However, could not reproduce on this branch.
tobias-lippert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code lgtm
| return ''; | ||
| } | ||
| // Path format: /model/<editorId>/<full/file/path> | ||
| const parts = path.split('/').filter(Boolean); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment that the boolean filter filters empty strings
Programming exercises: Prevent Monaco model switch from overwriting other files during build/refreshProgramming exercises: Prevent online code editor from overwriting other files during build/refresh
Checklist
General
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
This PR fixes a critical race condition in the Monaco Editor that causes file content corruption during rapid file switching scenarios. The issue was reported in issue #11353 and occurs when:
The root cause was that delayed text emissions were being applied to the currently selected file instead of the file they originated from, leading to data loss and file corruption.
Short notice
I was able to reproduce the bug more consistently than described in the original issue. Here’s what I found:
Description
Core Changes:
textChangedevent from emitting a simplestringto emitting{ text: string; fileName: string }to include explicit file contextComponent Updates:
Steps for Testing
Prerequisites:
Race Condition Testing:
Rapid File Switching Testing:
Edge Case Testing:
Exam Mode Testing
Prerequisites:
Testing steps are identical
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Refactor