Bugfix: Fix markdown in modeling exercises example solutions #11653
Conversation
…nt.html, revert changes to modeling-explanation-editor.component.html
github-actions
bot
added
client
WalkthroughThe modeling editor component's explanation display has been refactored to render markdown content using the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.ts (1)
217-220: Remove the unusedonExplanationInputmethod.The method at lines 217-220 in
modeling-editor.component.tsis dead code. The template call is commented out (line 119 in the HTML file), and the method is only invoked in tests. Since the interactive editor was replaced with a read-only markdown display, this method serves no purpose in production code and should be deleted.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.html(1 hunks)src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/**/*.html
⚙️ CodeRabbit configuration file
@if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
Files:
src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.html
src/main/webapp/**/*.ts
⚙️ CodeRabbit configuration file
Files:
src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.ts
🧠 Learnings (7)
📚 Learning: 2024-10-08T15:35:52.595Z
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 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/modeling/shared/modeling-editor/modeling-editor.component.html
📚 Learning: 2025-04-22T10:19:41.546Z
Learnt from: tobias-lippert
Repo: ls1intum/Artemis PR: 10714
File: src/main/webapp/app/quiz/manage/create-buttons/quiz-exercise-create-buttons.component.ts:11-15
Timestamp: 2025-04-22T10:19:41.546Z
Learning: In Angular 19, standalone components are the default, so components with an `imports` array don't need to explicitly declare `standalone: true` property in the component decorator.
Applied to files:
src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.ts
📚 Learning: 2025-08-21T17:30:20.530Z
Learnt from: MoritzSpengler
Repo: ls1intum/Artemis PR: 11297
File: src/main/webapp/app/quiz/shared/questions/drag-and-drop-question/drag-and-drop-question.component.spec.ts:34-34
Timestamp: 2025-08-21T17:30:20.530Z
Learning: FitTextDirective in src/main/webapp/app/quiz/shared/fit-text/fit-text.directive.ts is a standalone directive marked with standalone: true, so it should be imported in TestBed imports array, not declarations array.
Applied to files:
src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.ts
📚 Learning: 2024-10-28T11:34:03.750Z
Learnt from: isabellagessl
Repo: ls1intum/Artemis PR: 9615
File: src/main/webapp/app/overview/exercise-details/course-exercise-details.module.ts:75-75
Timestamp: 2024-10-28T11:34:03.750Z
Learning: In Angular, when a non-standalone component declared in an NgModule needs to use a standalone component, the standalone component should be imported into the NgModule's `imports` array. This allows the non-standalone component to utilize the standalone component in its template.
Applied to files:
src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.ts
📚 Learning: 2025-09-01T10:20:40.706Z
Learnt from: Michael-Breu-UIbk
Repo: ls1intum/Artemis PR: 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/modeling/shared/modeling-editor/modeling-editor.component.ts
📚 Learning: 2024-10-20T21:59:11.630Z
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 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/modeling/shared/modeling-editor/modeling-editor.component.ts
📚 Learning: 2025-05-27T15:39:30.592Z
Learnt from: eylulnc
Repo: ls1intum/Artemis PR: 10910
File: src/main/webapp/app/core/user/settings/global-notifications-settings/global-notifications-settings.component.ts:101-101
Timestamp: 2025-05-27T15:39:30.592Z
Learning: In Angular components, constants defined at module level (even if exported) are not accessible in templates. To use constants in templates for conditional rendering or comparisons, they must be assigned to a protected or public property of the component class, like `protected readonly CONSTANT_NAME = CONSTANT_NAME;`.
Applied to files:
src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.ts
🧬 Code graph analysis (1)
src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.ts (1)
src/main/webapp/app/modeling/shared/modeling-explanation-editor/modeling-explanation-editor.component.ts (1)
Component(7-26)
⏰ 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). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Mend Security Check
🔇 Additional comments (1)
src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.html (1)
124-124: XSS protection confirmed through Angular's DomSanitizer and SafeHtml pattern.Verification confirms the
htmlForMarkdownpipe properly sanitizes content. The pipe delegates toArtemisMarkdownService.safeHtmlForMarkdown(), which injects Angular'sDomSanitizerand returns aSafeHtmlobject after processing the markdown through thehtmlForMarkdown()utility function. The service acceptsallowedHtmlTagsandallowedHtmlAttributesparameters for granular control, and thebypassSecurityTrustHtml()call marks the sanitized output as safe for Angular's template binding. This pattern correctly implements Angular's security model for dynamic HTML rendering.
| @if (withExplanation) { | ||
| <jhi-modeling-explanation-editor [explanation]="explanation" (explanationChange)="onExplanationInput($event)" [readOnly]="readOnly" /> | ||
| <!-- <jhi-modeling-explanation-editor [explanation]="explanation" (explanationChange)="onExplanationInput($event)" [readOnly]="readOnly" /> --> | ||
|
|
||
| <!-- workaround until further investigation into example solution rendering --> | ||
| <label for="explanationText" jhiTranslate="artemisApp.modelingSubmission.explanationText"></label> | ||
| <div class="p-3 border markdown-preview"> | ||
| <p id="explanationText" [innerHTML]="explanation | htmlForMarkdown"></p> | ||
| </div> | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Verify instructor/admin edit workflow is not broken.
The new implementation shows the same read-only markdown display for all users when withExplanation is true, without checking the readOnly flag. The original ModelingExplanationEditorComponent accepted [readOnly]="readOnly" to conditionally show an editable interface for instructors/admins versus a read-only view for students.
Unless instructors/admins have another interface for editing the explanation, or withExplanation is only true in student contexts, this change breaks the editing capability.
Consider wrapping the new display in a conditional check:
@if (withExplanation) {
- <!-- <jhi-modeling-explanation-editor [explanation]="explanation" (explanationChange)="onExplanationInput($event)" [readOnly]="readOnly" /> -->
-
- <!-- workaround until further investigation into example solution rendering -->
- <label for="explanationText" jhiTranslate="artemisApp.modelingSubmission.explanationText"></label>
- <div class="p-3 border markdown-preview">
- <p id="explanationText" [innerHTML]="explanation | htmlForMarkdown"></p>
- </div>
+ @if (readOnly) {
+ <!-- Student view: read-only markdown display -->
+ <label for="explanationText" jhiTranslate="artemisApp.modelingSubmission.explanationText"></label>
+ <div class="p-3 border markdown-preview">
+ <p id="explanationText" [innerHTML]="explanation | htmlForMarkdown"></p>
+ </div>
+ } @else {
+ <!-- Instructor/admin view: editable interface -->
+ <jhi-modeling-explanation-editor [explanation]="explanation" (explanationChange)="onExplanationInput($event)" [readOnly]="false" />
+ }
}Alternatively, verify that one of the following is true:
- Instructors/admins never reach this component with
withExplanation=true - There's another dedicated editing interface elsewhere
- The
readOnlyflag is always true when this block is shown
Run this script to understand when withExplanation is set:
🏁 Script executed:
#!/bin/bash
# Description: Find usages of the withExplanation input to understand when it's enabled
# Search for withExplanation property bindings
rg -n "withExplanation" --type=html --type=ts -C3Length of output: 9848
The web search returned generic results. I need to search the codebase directly for the onExplanationInput method. Let me run a targeted script:
Let me correct this - I need to run the script on the codebase:
🏁 Script executed:
#!/bin/bash
# Search for onExplanationInput method in modeling-editor component
rg -n "onExplanationInput" --type=ts --type=html -C5Length of output: 6659
🏁 Script executed:
#!/bin/bash
# Search for onExplanationInput method in modeling-editor component
rg -n "onExplanationInput" --type=ts --type=html -C5Length of output: 6659
Critical: Editing capability broken for instructors and students in multiple contexts.
Verification confirms the original concern. The readOnly flag is ignored in the new implementation, breaking explanation editing where it should be available:
- example-modeling-submission.component.html (line 148): Instructors editing example solutions with
[readOnly]="readOnly"— can now only view, not edit. - modeling-submission.component.html (line 104): Students submitting solutions with
[readOnly]="!isOwnerOfParticipation"— owners can now only view, not edit. - modeling-exam-submission.component.html (line 33): Exam submissions with
[readOnly]="readonly()"— editable exams now read-only.
The onExplanationInput method (modeling-editor.component.ts:217) still exists and has passing tests, but is unreachable since the component that calls it is commented out.
Restore the conditional logic to handle both read-only and editable modes:
@if (withExplanation) {
- <!-- <jhi-modeling-explanation-editor [explanation]="explanation" (explanationChange)="onExplanationInput($event)" [readOnly]="readOnly" /> -->
-
- <!-- workaround until further investigation into example solution rendering -->
- <label for="explanationText" jhiTranslate="artemisApp.modelingSubmission.explanationText"></label>
- <div class="p-3 border markdown-preview">
- <p id="explanationText" [innerHTML]="explanation | htmlForMarkdown"></p>
- </div>
+ @if (readOnly) {
+ <label for="explanationText" jhiTranslate="artemisApp.modelingSubmission.explanationText"></label>
+ <div class="p-3 border markdown-preview">
+ <p id="explanationText" [innerHTML]="explanation | htmlForMarkdown"></p>
+ </div>
+ } @else {
+ <jhi-modeling-explanation-editor [explanation]="explanation" (explanationChange)="onExplanationInput($event)" [readOnly]="false" />
+ }
}| <label for="explanationText" jhiTranslate="artemisApp.modelingSubmission.explanationText"></label> | ||
| <div class="p-3 border markdown-preview"> | ||
| <p id="explanationText" [innerHTML]="explanation | htmlForMarkdown"></p> |
There was a problem hiding this comment.
Incorrect semantic HTML: label pointing to non-form element.
The <label for="explanationText"> points to a <p> element, which is not a form control. The for attribute should only reference form elements like <input>, <textarea>, or <select>. This creates invalid HTML and doesn't provide the expected accessibility benefits.
Apply this diff to fix the semantic issue:
-<label for="explanationText" jhiTranslate="artemisApp.modelingSubmission.explanationText"></label>
+<p class="form-label mb-2" jhiTranslate="artemisApp.modelingSubmission.explanationText"></p>
<div class="p-3 border markdown-preview">
- <p id="explanationText" [innerHTML]="explanation | htmlForMarkdown"></p>
+ <div [innerHTML]="explanation | htmlForMarkdown"></div>
</div>Alternatively, if you want to maintain label semantics for styling purposes, remove the for attribute:
-<label for="explanationText" jhiTranslate="artemisApp.modelingSubmission.explanationText"></label>
+<label jhiTranslate="artemisApp.modelingSubmission.explanationText"></label>
<div class="p-3 border markdown-preview">
<p id="explanationText" [innerHTML]="explanation | htmlForMarkdown"></p>
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <label for="explanationText" jhiTranslate="artemisApp.modelingSubmission.explanationText"></label> | |
| <div class="p-3 border markdown-preview"> | |
| <p id="explanationText" [innerHTML]="explanation | htmlForMarkdown"></p> | |
| <p class="form-label mb-2" jhiTranslate="artemisApp.modelingSubmission.explanationText"></p> | |
| <div class="p-3 border markdown-preview"> | |
| <div [innerHTML]="explanation | htmlForMarkdown"></div> |
🤖 Prompt for AI Agents
In
src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.html
around lines 122 to 124, the <label for="explanationText"> targets a <p>
(non-form) element which is invalid; replace the label with an appropriate
non-form container (e.g., <h3> or <span class="label-text">) or remove the for
attribute so it’s purely presentational, and keep the existing id on the <p> for
reference if needed for styling or anchors; ensure the resulting element conveys
the same text and accessibility semantics (use a heading level if it’s a section
title).
| import { NgClass, NgStyle } from '@angular/common'; | ||
| import { ModelingExplanationEditorComponent } from '../modeling-explanation-editor/modeling-explanation-editor.component'; | ||
| import { captureException } from '@sentry/angular'; | ||
| import { FormsModule } from '@angular/forms'; |
There was a problem hiding this comment.
Unnecessary FormsModule import.
The FormsModule import appears unnecessary. The new template code (lines 118-126 in the HTML file) doesn't use any forms-related directives like ngModel, ngForm, or form controls. This import was likely needed by the now-commented-out ModelingExplanationEditorComponent, but the replacement code doesn't require it.
Apply this diff to remove the unnecessary import:
-import { FormsModule } from '@angular/forms';
import { HtmlForMarkdownPipe } from 'app/shared/pipes/html-for-markdown.pipe';- imports: [TranslateDirective, FaIconComponent, NgStyle, NgClass, FormsModule, HtmlForMarkdownPipe],
+ imports: [TranslateDirective, FaIconComponent, NgStyle, NgClass, HtmlForMarkdownPipe],Also applies to: 22-22
🤖 Prompt for AI Agents
In
src/main/webapp/app/modeling/shared/modeling-editor/modeling-editor.component.ts
around lines 14 and 22, remove the unused FormsModule reference: delete the
top-level import statement for FormsModule and remove it from the
component/module imports array (or any place it was added) since the template no
longer uses forms directives; ensure no other code depends on ngModel/ngForm
before removing.
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
1 participant
Checklist
General
Motivation and Context
This pull request temporarily fixes #9680. When creating modeling exercises, the example solution for students is not rendered as markdown. The solution I found can only be seen as a workaround, as the example solution rendering could be overhauled completely. Depending on the exercise type, different example solution stylings and components are used. Maybe one could refactor all example solution handling into a single component.
I fully understand that this is not considered good practise and I do not think that my solution fully fixes the issue. I understand if the code guidelines do not accept such a quick & dirty fix and I will look more into it.
The Problem: We need a fix asap as the semester has already begun and we very frequently use modeling exercises at KIT with example solutions as text.
Description
As described in Issue #9680, the problem seems to be
modeling/shared/modeling-explanation-editor/modeling-explanation-editor.component.html:A textarea is used to display the example solution. Since textareas will not render either html or markdown, the already existing htmlForMarkdown pipe should be used in conjuction with a different html element which can render innerHTML like p, div etc.
My solution consists of temporarily commenting out the modeling-explanation-editor and replacing the student view with a p tag with the
markdownForHTMLpipe. Thus, the markdown gets converted to html and is rendered correctly. Styling wise I tried to mimic the admin example solution view. This currently only affects the student overview page.Why not change the textarea to p or div? The textarea supports the ngModelChange event, whereas p or div elements do not. This functionalitiy is used in other components and as to not refactor all of those (and potentially break even more), I opted to leave it as is and just fix the student view.
Steps for Testing
I tested all markdown elements supported by Artemis, everything works fine.
Screenshots
Summary by CodeRabbit