Skip to content

Conversation

ekayandan
Copy link
Contributor

@ekayandan ekayandan commented Oct 9, 2025

Checklist

General

Motivation and Context

When diffs are too big between files (eg. csv files), Monaco Editor crashes browsers due to out-of-memory, as described in #11469. Limiting diff computation time and maximum file size should solve this issue.

Description

Added necessary configurations to limit computation time and file size.

Steps for Testing

  1. Log in to Artemis
  2. Navigate to any Programming Exercise Management Page, click on 'Edit in Editor', then 'Open Commit History'.
image 4. Click on a commit hash to see the diff editor. 5. Make sure view functions as expected (screenshot is from develop branch, for reference) image

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

Code Review

  • Code Review 1

Manual Tests

  • Test 1

Summary by CodeRabbit

  • New Features
    • Diff viewer now handles larger files and allows longer processing, improving reliability for big comparisons.
    • Increased computation time limit to 3 seconds to better accommodate complex diffs.
    • Enhancements apply to the diff editor only; the standard editor remains unchanged.

@ekayandan ekayandan requested review from a team and krusche as code owners October 9, 2025 16:40
@github-project-automation github-project-automation bot moved this to Work In Progress in Artemis Development Oct 9, 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 Oct 9, 2025
Copy link
Contributor

coderabbitai bot commented Oct 9, 2025

Walkthrough

Adds Monaco DiffEditor initialization options in two places, setting maxFileSize to 20 and maxComputationTime to 3000. Changes are limited to diff editor creation/configuration and do not modify exports or standalone editor behavior.

Changes

Cohort / File(s) Change Summary
Monaco Diff Editor configuration
src/main/webapp/app/programming/shared/utils/diff.utils.ts, src/main/webapp/app/shared/monaco-editor/service/monaco-editor.service.ts
Added DiffEditor options: maxFileSize: 20, maxComputationTime: 3000 during initialization. No public API changes; only affects diff editor setup.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title concisely indicates that within the programming exercises context, the pull request configures file size limits for the Monaco Diff Editor. It directly reflects the main change of adding maxFileSize and maxComputationTime constraints and avoids vague or noisy wording. This specificity ensures a teammate scanning history understands the primary change.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bugfix/limit-file-size-for-monaco

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: 1

🧹 Nitpick comments (1)
src/main/webapp/app/shared/monaco-editor/service/monaco-editor.service.ts (1)

82-83: Extract duplicated limits into shared constants and document their behavior.

  • Move 20 (MB) and 3000 (ms) from
    src/.../service/monaco-editor.service.ts (lines 82–83) and
    src/.../diff.utils.ts (lines 147–148) into a shared constants file.
  • In that file, clarify units and behavior on exceedance (timeout cancels diff; oversized files skip full diff).
  • Import and use those constants in both places.
📜 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 6c16c15 and 698f45d.

📒 Files selected for processing (2)
  • src/main/webapp/app/programming/shared/utils/diff.utils.ts (1 hunks)
  • src/main/webapp/app/shared/monaco-editor/service/monaco-editor.service.ts (1 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/shared/monaco-editor/service/monaco-editor.service.ts
  • src/main/webapp/app/programming/shared/utils/diff.utils.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: ekayandan
PR: ls1intum/Artemis#10885
File: src/main/webapp/app/programming/shared/utils/diff.utils.ts:143-143
Timestamp: 2025-05-25T13:26:42.641Z
Learning: In Monaco Editor diff computation, creating a diff editor with a detached DOM element (not appended to document.body) is an intentional performance optimization to prevent rendering while still allowing diff calculation functionality to work properly.
📚 Learning: 2024-10-08T15:35:52.595Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9407
File: src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts:66-66
Timestamp: 2024-10-08T15:35:52.595Z
Learning: In the `MonacoDiffEditorComponent` (`src/main/webapp/app/shared/monaco-editor/monaco-diff-editor.component.ts`), the diff editor is always set to read-only (`readOnly: true`), and there are currently no use cases that require editing in the diff view.

Applied to files:

  • src/main/webapp/app/shared/monaco-editor/service/monaco-editor.service.ts
  • src/main/webapp/app/programming/shared/utils/diff.utils.ts
📚 Learning: 2025-05-25T13:26:42.641Z
Learnt from: ekayandan
PR: ls1intum/Artemis#10885
File: src/main/webapp/app/programming/shared/utils/diff.utils.ts:143-143
Timestamp: 2025-05-25T13:26:42.641Z
Learning: In Monaco Editor diff computation, creating a diff editor with a detached DOM element (not appended to document.body) is an intentional performance optimization to prevent rendering while still allowing diff calculation functionality to work properly.

Applied to files:

  • src/main/webapp/app/programming/shared/utils/diff.utils.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: Codacy Static Code Analysis
  • 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: client-style
  • GitHub Check: client-tests-selected
  • GitHub Check: client-tests
  • GitHub Check: server-style
  • GitHub Check: server-tests
  • GitHub Check: Analyse

@github-project-automation github-project-automation bot moved this from Work In Progress to Ready For Review in Artemis Development Oct 9, 2025
const diffEditor = monaco.editor.createDiffEditor(document.createElement('div'), {
readOnly: true,
automaticLayout: false,
maxFileSize: 20,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What unit is this size? Lines? kiB? Some comment would be helpful.
I guess the computation time below are milliseconds?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add onto this: the input.constants.ts file provides a lot of constants, like e.g. MAX_FILE_SIZE, which you can use instead of a hardcoded value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link

github-actions bot commented Oct 9, 2025

End-to-End (E2E) Test Results Summary

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

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

Projects

Status: Ready For Review

Development

Successfully merging this pull request may close these issues.

4 participants