Skip to content

Conversation

@HawKhiem
Copy link
Contributor

@HawKhiem HawKhiem commented Sep 25, 2025

Checklist

General

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).
  • I tested all changes and their related features with all corresponding user types on a test server configured with LocalVC and Jenkins.

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:

  1. A user edits a file (e.g., BubbleSort.java) and submits changes
  2. While the build is running, they switch to another file (e.g., MergeSort.java) and make changes
  3. Switch back to the first file and wait till the build is finished
  4. Switch to the other file containing unsubmitted changes (MergeSort) then switch back to the first file (BubbleSort)
  5. Due to delayed text change emissions, the changes from the first file get applied to the currently selected file, causing content corruption

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:

  1. The problem almost always occurs when the files are not syntax-highlighted.
  2. It is unclear whether the bug also occurs if only one of the files is not syntax-highlighted.
  3. The position of the files in the file tree does not matter; I have reproduced the bug even with files in the middle of the tree.
  4. The file type also does not matter.
  5. The bug occurs when a user edits one file while it is not syntax-highlighted, submits the changes, switches to another file that is also not syntax-highlighted, edits it, and then switches back to the first file.
  6. When are files not syntax-highlighted? (No guarantee—based on personal observation):
  • After submitting changes and reloading while the build is still running — the files are not syntax-highlighted for a short time.
  • When the build has finished, reloading usually restores syntax highlighting.
  • Files are also briefly not syntax-highlighted when first opening them in the editor.

Description

Core Changes:

  1. New Event Signature: Modified the textChanged event from emitting a simple string to emitting { text: string; fileName: string } to include explicit file context
  2. File Path Resolution: Added [extractFilePathFromModel()] in monaco-editor.component.ts to properly extract full file paths from Monaco Editor model URIs
  3. Contex-based File Handling: Updated [onFileTextChanged()] in code-editor-monaco.component.ts to apply changes to the correct file based on the fileName property, not the currently selected file

Component Updates:

  • Monaco Editor Component: Modified to emit file context with all text changes
  • Programming Component: Modified to handle new event signature and apply changes to correct files
  • Custom Build Plan Components: Updated both Aeolus and regular build plan components
  • Markdown Editor Component: Adapted to extract text from new event object format

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 2 Students
  • 1 Programming Exercise with multiple files (e.g., BubbleSort.java, MergeSort.java)

Race Condition Testing:

  1. Log in as a student and navigate to the programming exercise
  2. Open the code editor and make changes to one file (e.g., BubbleSort.java)
  3. Click Submit and immediately switch to (e.g., MergeSort.java) while build is running
  4. Make changes to MergeSort.java and switch back to BubbleSort.java
  5. After build completes, verify that:
    • BubbleSort.java contains only its original changes
    • MergeSort.java contains only its changes
    • No file content corruption occurs

Rapid File Switching Testing:

  1. Open multiple files in the editor
  2. Rapidly switch between files while typing in each
  3. Verify that all changes are preserved and applied to correct files
  4. Submit changes and verify persistence through the workflow

Edge Case Testing:

  1. Test during build states when syntax highlighting may be disabled
  2. Test file switching during editor resets
  3. Verify changes persist through submission workflows
  4. Test with files that have similar names or content

Exam Mode Testing

Prerequisites:

  • 1 Instructor
  • 2 Students
  • 1 Programming Exercise with multiple files (e.g., BubbleSort.java, MergeSort.java)

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

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Exam Mode Test

  • Test 1
  • Test 2

Performance Tests

  • Test 1
  • Test 2

Test Coverage

Screenshots

Summary by CodeRabbit

  • New Features

    • Editors (code & markdown) now include file name with text changes, enabling precise per-file updates and background edits.
  • Bug Fixes

    • Fixed cases where edits were wrongly applied to the selected file instead of the intended file.
  • Refactor

    • Standardized change events across editors to emit a consistent, file-aware payload for improved integration and reliability.

@github-project-automation github-project-automation bot moved this to Work In Progress in Artemis Development Sep 25, 2025
@HawKhiem HawKhiem self-assigned this Sep 25, 2025
@github-actions github-actions bot added client Pull requests that update TypeScript code. (Added Automatically!) programming Pull requests that affect the corresponding module labels Sep 25, 2025
@helios-aet helios-aet bot temporarily deployed to artemis-test1.artemis.cit.tum.de September 25, 2025 13:14 Inactive
@github-actions
Copy link

End-to-End (E2E) Test Results Summary

TestsPassed ☑️Skipped ⚠️Failed ❌️Time ⏱
End-to-End (E2E) Test Report205 ran188 passed4 skipped13 failed1h 15m 51s 625ms
TestResultTime ⏱
End-to-End (E2E) Test Report
e2e/exam/ExamCreationDeletion.spec.ts
ts.Exam creation/deletion › Creates an exam❌ failure30s 902ms
ts.Exam creation/deletion › Edits an exam › Edits an existing exam❌ failure21s 935ms
e2e/exam/ExamDateVerification.spec.ts
ts.Exam date verification › Exam timing › Student can start after start Date❌ failure2m 4s 40ms
e2e/exam/test-exam/TestExamCreationDeletion.spec.ts
ts.Test Exam creation/deletion › Creates a test exam❌ failure23s 475ms
e2e/exercise/quiz-exercise/QuizExerciseManagement.spec.ts
ts.Quiz Exercise Management › Quiz Exercise Creation › Creates a Quiz with Drag and Drop❌ failure2m 3s 964ms
e2e/exercise/quiz-exercise/QuizExerciseParticipation.spec.ts
ts.Quiz Exercise Participation › DnD Quiz participation › Student can participate in DnD Quiz❌ failure2m 4s 618ms
e2e/exam/ExamParticipation.spec.ts
ts.Exam participation › Early Hand-in › Participates as a student in a registered exam❌ failure3m 411ms
e2e/exam/ExamAssessment.spec.ts
ts.Exam assessment › Programming exercise assessment › Assess a programming exercise submission (MANUAL)❌ failure
e2e/exam/ExamResults.spec.ts
ts.Exam Results › Check exam exercise results › Check exam results for programming exercise › Check exam programming exercise results❌ failure1m 53s 767ms
e2e/exercise/ExerciseImport.spec.ts
ts.Import exercises › Imports exercises › Imports programming exercise❌ failure1m 53s 159ms
e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts
ts.Programming exercise participation › Makes a partially successful Java submission › Makes a submission using code editor❌ failure1m 25s 506ms
ts.Programming exercise participation › Makes a successful Java submission › Makes a submission using code editor❌ failure1m 50s 295ms
e2e/exercise/programming/ProgrammingExerciseStaticCodeAnalysis.spec.ts
ts.Static code analysis tests › Configures SCA grading and makes a successful submission with SCA errors❌ failure2m 13s 696ms

@github-actions github-actions bot removed the programming Pull requests that affect the corresponding module label Sep 25, 2025
@helios-aet helios-aet bot temporarily deployed to artemis-test1.artemis.cit.tum.de September 25, 2025 20:31 Inactive
@github-actions
Copy link

End-to-End (E2E) Test Results Summary

TestsPassed ☑️Skipped ⚠️Failed ❌️Time ⏱
End-to-End (E2E) Test Report205 ran200 passed3 skipped2 failed1h 9m 36s 344ms
TestResultTime ⏱
End-to-End (E2E) Test Report
e2e/exercise/quiz-exercise/QuizExerciseManagement.spec.ts
ts.Quiz Exercise Management › Quiz Exercise Creation › Creates a Quiz with Drag and Drop❌ failure2m 3s 551ms
e2e/exercise/quiz-exercise/QuizExerciseParticipation.spec.ts
ts.Quiz Exercise Participation › DnD Quiz participation › Student can participate in DnD Quiz❌ failure2m 4s 321ms

@github-actions github-actions bot added the programming Pull requests that affect the corresponding module label Sep 26, 2025
@helios-aet helios-aet bot temporarily deployed to artemis-test1.artemis.cit.tum.de September 26, 2025 11:52 Inactive
@github-actions github-actions bot added the tests label Sep 26, 2025
@github-actions
Copy link

End-to-End (E2E) Test Results Summary

TestsPassed ✅SkippedFailedTime ⏱
End-to-End (E2E) Test Report1 ran1 passed0 skipped0 failed1s 759ms
TestResultTime ⏱
No test annotations available

@github-actions
Copy link

End-to-End (E2E) Test Results Summary

TestsPassed ✅SkippedFailedTime ⏱
End-to-End (E2E) Test Report1 ran1 passed0 skipped0 failed1s 542ms
TestResultTime ⏱
No test annotations available

@helios-aet helios-aet bot temporarily deployed to artemis-test1.artemis.cit.tum.de September 26, 2025 12:47 Inactive
@HawKhiem HawKhiem marked this pull request as ready for review September 26, 2025 12:52
@HawKhiem HawKhiem requested review from a team and krusche as code owners September 26, 2025 12:52
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 26, 2025

Walkthrough

Text-change events from Monaco editors now emit objects { text, fileName } instead of plain strings. Consumers were updated: core Monaco emitter provides fileName, the programming and markdown editor wrappers accept the new shape, build-plan components adapt their codeChanged signatures, and tests/integration specs were aligned.

Changes

Cohort / File(s) Summary
Monaco editor core emission
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts, src/main/webapp/app/shared/monaco-editor/monaco-editor.component.spec.ts
textChanged output now emits { text, fileName } instead of string. Added extractFilePathFromModel helper and updated emit logic and tests.
Programming code editor (Monaco wrapper)
src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts, src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.spec.ts
onFileTextChanged now accepts { text, fileName }, updates fileSession[fileName], and emits changes for that file. Tests updated to pass the event object.
Markdown editor (Monaco wrapper)
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts, src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.spec.ts
onTextChanged signature changed to accept { text, fileName }; implementation reads event.text and continues emitting the text. Tests updated accordingly.
Programming exercise custom build plans
src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-build-plan.component.ts, src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-aeolus-build-plan.component.ts, src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-aeolus-build-plan.component.spec.ts
codeChanged signatures updated: general component accepts `string
Integration test alignment
src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts
Updated to call onFileTextChanged with { text, fileName } to match the new API.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Title Check ✅ Passed The title succinctly describes the primary change by indicating that the online code editor in programming exercises will no longer overwrite other files during build or refresh, which directly corresponds to the core fix addressing the Monaco Editor race condition and file context preservation introduced in this PR.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bugfix/programming-exercises/overwrite-student-submissions

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.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 component

For 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4e923b9 and c4d2072.

📒 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

angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

Files:

  • src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-aeolus-build-plan.component.ts
  • src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.spec.ts
  • src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts
  • src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.spec.ts
  • src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-build-plan.component.ts
  • src/main/webapp/app/shared/monaco-editor/monaco-editor.component.spec.ts
  • src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts
  • src/main/webapp/app/programming/manage/update/update-components/custom-build-plans/programming-exercise-custom-aeolus-build-plan.component.spec.ts
  • src/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.ts
  • src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts
  • src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.spec.ts
  • src/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.ts
  • src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.spec.ts
  • src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts
  • src/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.ts
  • src/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.ts
  • src/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 handled

Using 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 payload

Test 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 change

onFileTextChanged 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 shape

Container 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 payload

codeChanged is invoked with { text, fileName } and ScriptAction.script is asserted accordingly.


167-176: Second test similarly aligned

Invocation 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 emission

Test now expects { text, fileName } from textChanged; correct.


71-72: Debounced emission assertion updated

Single 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 good

Accepting 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 } for textChanged aligns with the PR’s objective.

@github-project-automation github-project-automation bot moved this from Work In Progress to Ready For Review in Artemis Development Sep 26, 2025
@github-actions
Copy link

End-to-End (E2E) Test Results Summary

TestsPassed ✅SkippedFailedTime ⏱
End-to-End (E2E) Test Report1 ran1 passed0 skipped0 failed1s 548ms
TestResultTime ⏱
No test annotations available

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between c4d2072 and 025ad33.

📒 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

angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

Files:

  • src/main/webapp/app/programming/shared/code-editor/monaco/code-editor-monaco.component.ts
  • src/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.ts
  • src/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 correct

Correctly 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 files

Using one textChangedEmitTimeout cancels pending emissions from file A when typing in file B, losing A’s last edits. Maintain a per-model debounce keyed by model.uri to 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`.

@github-actions
Copy link

End-to-End (E2E) Test Results Summary

TestsPassed ✅SkippedFailedTime ⏱
End-to-End (E2E) Test Report1 ran1 passed0 skipped0 failed1s 569ms
TestResultTime ⏱
No test annotations available

@helios-aet helios-aet bot temporarily deployed to artemis-test1.artemis.cit.tum.de September 26, 2025 13:56 Inactive
@github-actions
Copy link

End-to-End (E2E) Test Results Summary

TestsPassed ☑️Skipped ⚠️Failed ❌️Time ⏱
End-to-End (E2E) Test Report205 ran199 passed3 skipped3 failed1h 13m 53s 436ms
TestResultTime ⏱
End-to-End (E2E) Test Report
e2e/exercise/quiz-exercise/QuizExerciseManagement.spec.ts
ts.Quiz Exercise Management › Quiz Exercise Creation › Creates a Quiz with Drag and Drop❌ failure2m 4s 193ms
e2e/exercise/quiz-exercise/QuizExerciseParticipation.spec.ts
ts.Quiz Exercise Participation › DnD Quiz participation › Student can participate in DnD Quiz❌ failure2m 4s 605ms
e2e/exercise/programming/ProgrammingExerciseStaticCodeAnalysis.spec.ts
ts.Static code analysis tests › Configures SCA grading and makes a successful submission with SCA errors❌ failure2m 27s 922ms

@wasnertobias wasnertobias self-requested a review September 26, 2025 19:05
Copy link
Contributor

@ekayandan ekayandan left a 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.

Copy link
Contributor

@tobias-lippert tobias-lippert left a 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);
Copy link
Contributor

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

@krusche krusche added this to the 8.4.1 milestone Sep 28, 2025
@krusche krusche changed the title Programming exercises: Prevent Monaco model switch from overwriting other files during build/refresh Programming exercises: Prevent online code editor from overwriting other files during build/refresh Sep 28, 2025
@krusche krusche merged commit 54d40a7 into develop Sep 28, 2025
30 of 36 checks passed
@krusche krusche deleted the bugfix/programming-exercises/overwrite-student-submissions branch September 28, 2025 21:04
@github-project-automation github-project-automation bot moved this from Ready For Review to Merged in Artemis Development Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix client Pull requests that update TypeScript code. (Added Automatically!) programming Pull requests that affect the corresponding module ready for review tests

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants