Development: Upgrade markdown library to markdown-it
#9354
Conversation
github-actions
bot
added
the
client
pzdr7
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request involves a significant transition from the Showdown library to the Markdown-it library for markdown processing across various components in the project. Key changes include the removal of Showdown dependencies and the introduction of Markdown-it plugins in both configuration files and component implementations. The modifications also encompass updates to class structures, method signatures, and type definitions to align with the new library's architecture. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional context usedPath-based instructions (1)
Additional comments not posted (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range comments (3)
angular.json (1)
Line range hint
1-287: Consider additional configuration changes for markdown-it integration.While the current changes correctly add
markdown-it-classto the allowed dependencies, there might be additional configuration changes needed for a complete migration from showdown to markdown-it.Please consider the following:
- Review the
scriptsandstylessections to ensure any showdown-related entries are removed and markdown-it entries are added if necessary.- Check if any
fileReplacementsor environment-specific configurations need updating to reflect the new markdown processing library.- Verify that the
budgetssection accommodates any size changes resulting from the library switch.- Ensure that any custom webpack configurations (if used) are updated to handle markdown-it and its plugins correctly.
These steps will help ensure a smooth and complete transition to the new markdown processing library throughout the Angular application.
src/main/webapp/app/exercises/quiz/shared/short-answer-question-util.service.ts (1)
Line range hint
1-458: Consider refactoring for improved maintainabilityWhile the immediate change is beneficial, there are opportunities for broader improvements in this file:
Some methods, like
addIndentationToTextParts, are quite long and complex. Consider breaking them down into smaller, more focused methods to improve readability and maintainability.There are several TODO comments throughout the code. It would be beneficial to address these or create issues to track them if they represent larger tasks.
The
ShortAnswerQuestionUtilclass is quite large. Consider if some of its functionality could be split into separate, more focused service classes.Ensure all methods have appropriate JSDoc comments for better documentation.
These suggestions aim to improve the overall code quality and maintainability of the file.
src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts (1)
Issues Found: Remaining 'showdown' References in Codebase
The migration from
showdowntomarkdown-itis not fully complete. The following files still contain references toshowdown, which may lead to inconsistencies in markdown rendering:
src/main/webapp/app/shared/util/markdown.conversion.util.ts
- Contains comments referencing showdown behavior.
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.html
- Includes a link to the showdown demo:
<a class="col-auto" href="http://demo.showdownjs.com">Please remove or update these lingering
showdownreferences to ensure a complete and consistent transition tomarkdown-it.Analysis chain
Line range hint
1-438: Verify markdown rendering functionalityWhile the changes to accommodate the new markdown-it library appear correct, it's crucial to thoroughly test the markdown rendering functionality across various scenarios. This includes:
- Rendering of basic markdown elements (headers, lists, code blocks, etc.)
- Proper functioning of custom extensions (e.g., task parsing, UML diagrams)
- Interaction with other components that consume rendered markdown
Please ensure that all existing markdown features work as expected with the new library implementation.
To assist in verifying the markdown rendering, you can run the following script to check for any potential issues or inconsistencies in the markdown processing:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential issues in markdown rendering # Test: Look for any remaining references to 'showdown' in the codebase echo "Checking for remaining 'showdown' references:" rg 'showdown' --type typescript --type html # Test: Verify that all markdown-it plugins are properly imported and used echo "Verifying markdown-it plugin usage:" rg 'markdown-it-' --type typescript # Test: Check for any TODO comments related to markdown processing echo "Checking for TODO comments related to markdown:" rg 'TODO.*markdown' --type typescript --type htmlLength of output: 529
Script:
#!/bin/bash # Description: Check for potential issues in markdown rendering # Test: Look for any remaining references to 'showdown' in the codebase echo "Checking for remaining 'showdown' references:" rg 'showdown' --type ts --type html # Test: Verify that all markdown-it plugins are properly imported and used echo "Verifying markdown-it plugin usage:" rg 'markdown-it-' --type ts # Test: Check for any TODO comments related to markdown processing echo "Checking for TODO comments related to markdown:" rg 'TODO.*markdown' --type ts --type htmlLength of output: 1527
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (16)
- angular.json (1 hunks)
- package.json (4 hunks)
- src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts (3 hunks)
- src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-task.extension.ts (3 hunks)
- src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts (2 hunks)
- src/main/webapp/app/exercises/quiz/shared/short-answer-question-util.service.ts (1 hunks)
- src/main/webapp/app/index.d.ts (1 hunks)
- src/main/webapp/app/shared/markdown-editor/extensions/ArtemisTextReplacementExtension.ts (1 hunks)
- src/main/webapp/app/shared/markdown-editor/extensions/artemis-showdown-extension-wrapper.ts (0 hunks)
- src/main/webapp/app/shared/markdown.service.ts (2 hunks)
- src/main/webapp/app/shared/pipes/html-for-markdown.pipe.ts (2 hunks)
- src/main/webapp/app/shared/util/markdown.conversion.util.ts (3 hunks)
- src/test/javascript/spec/component/lecture-unit/text-unit/text-unit.component.spec.ts (1 hunks)
- src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts (1 hunks)
- src/test/javascript/spec/component/short-answer-quiz/short-answer-question-util.service.spec.ts (1 hunks)
- src/test/javascript/spec/helpers/sample/problemStatement.json (1 hunks)
Files not reviewed due to no reviewable changes (1)
- src/main/webapp/app/shared/markdown-editor/extensions/artemis-showdown-extension-wrapper.ts
Additional context used
Path-based instructions (12)
src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts (1)
src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-task.extension.ts (1)
src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts (1)
src/main/webapp/app/exercises/quiz/shared/short-answer-question-util.service.ts (1)
src/main/webapp/app/index.d.ts (1)
src/main/webapp/app/shared/markdown-editor/extensions/ArtemisTextReplacementExtension.ts (1)
src/main/webapp/app/shared/markdown.service.ts (1)
src/main/webapp/app/shared/pipes/html-for-markdown.pipe.ts (1)
src/main/webapp/app/shared/util/markdown.conversion.util.ts (1)
src/test/javascript/spec/component/lecture-unit/text-unit/text-unit.component.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: 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}}src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: 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}}src/test/javascript/spec/component/short-answer-quiz/short-answer-question-util.service.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: 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}}
Biome
src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts
[error] 97-97: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
Additional comments not posted (28)
src/main/webapp/app/index.d.ts (1)
1-4: Verify the impact of removing showdown-related declarations.The replacement of showdown-related declarations with the new markdown-it-class declaration aligns well with the PR objective and addresses the security vulnerability mentioned in issue #9333. However, this change may have broader implications across the codebase.
Please run the following script to ensure that all references to the removed showdown modules have been updated or removed:
This script will help identify any places where the old modules are still referenced and confirm that the new markdown-it-class module is being used correctly.
Verification successful
Verification Successful: Showdown modules correctly removed and markdown-it-class integrated.
All references to 'showdown-katex' and 'showdown-highlight' have been successfully removed, and 'markdown-it-class' is being utilized as intended in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to removed showdown modules echo "Checking for references to showdown-katex:" rg --type typescript --type javascript "showdown-katex" echo "Checking for references to showdown-highlight:" rg --type typescript --type javascript "showdown-highlight" echo "Verifying markdown-it-class usage:" rg --type typescript --type javascript "markdown-it-class"Length of output: 556
Script:
#!/bin/bash # Description: Check for any remaining references to removed showdown modules echo "Checking for references to showdown-katex:" rg --glob "*.ts" --glob "*.js" "showdown-katex" echo "Checking for references to showdown-highlight:" rg --glob "*.ts" --glob "*.js" "showdown-highlight" echo "Verifying markdown-it-class usage:" rg --glob "*.ts" --glob "*.js" "markdown-it-class"Length of output: 604
src/main/webapp/app/shared/markdown-editor/extensions/ArtemisTextReplacementExtension.ts (5)
1-2: LGTM: Imports are correctly typed and follow best practices.The imports are properly using the 'type' keyword for type-only imports, which is good for optimizing bundle size. The naming conventions follow TypeScript standards.
4-7: LGTM: Class declaration and documentation are well-structured.The abstract class
ArtemisTextReplacementExtensionis properly declared and exported. The class name follows the PascalCase convention as per the coding guidelines. The JSDoc comment effectively describes the purpose of the class.
8-19: LGTM:getExtension()method is well-implemented.The
getExtension()method is correctly implemented as a MarkdownIt plugin. It effectively overrides therendermethod while preserving the original functionality. The use of arrow functions and the spread operator aligns with the coding guidelines. The method name follows the camelCase convention as required.
21-21: LGTM:replaceText()abstract method is properly declared.The
replaceText()method is correctly declared as abstract, following the camelCase naming convention. Its signature is clear and concise, with appropriate parameter and return types.
1-22: Great implementation of the Markdown-It extension for text replacement.This new
ArtemisTextReplacementExtensionclass aligns well with the PR objectives of upgrading the markdown library. It provides a flexible and reusable way to implement text replacement in raw markdown before processing. The implementation follows TypeScript and Angular coding guidelines, making it a valuable addition to the project.Some key points:
- The class is designed to be extended, allowing for various text replacement strategies.
- It correctly integrates with Markdown-It by providing a plugin that overrides the
rendermethod.- The original rendering functionality is preserved while adding the new text replacement step.
This implementation contributes to the overall goal of replacing the showdown library with markdown-it, addressing the security concerns mentioned in the linked issue #9333.
src/main/webapp/app/shared/pipes/html-for-markdown.pipe.ts (4)
15-15: LGTM: Method signature comment updated correctly.The JSDoc comment has been updated to reflect the change from
ShowdownExtension[]toPluginSimple[], which is consistent with the upgrade to markdown-it. The comment follows the project's coding guidelines for documentation.
Line range hint
1-28: Overall assessment: Changes successfully implement markdown-it upgrade.The modifications in this file effectively transition from the showdown library to markdown-it, aligning with the PR objectives. The changes are consistent throughout the file, including imports, JSDoc comments, and method signatures. The implementation adheres to the project's coding guidelines and maintains the existing functionality while upgrading the markdown processing library.
4-4: LGTM: Import statement for markdown-it added correctly.The import of
PluginSimplefrom 'markdown-it' is in line with the PR objective of upgrading to the markdown-it library. This change is correctly implemented and follows the project's coding guidelines.Let's verify the usage of
PluginSimplein the file:#!/bin/bash # Description: Verify the usage of PluginSimple in the file. # Test: Search for PluginSimple usage. Expect: At least one occurrence. rg --type typescript 'PluginSimple' src/main/webapp/app/shared/pipes/html-for-markdown.pipe.ts
22-22: LGTM: Method signature updated correctly. Verify impact on codebase.The
transformmethod signature has been correctly updated to usePluginSimple[]instead ofShowdownExtension[], which is consistent with the upgrade to markdown-it. This change follows the project's coding guidelines and aligns with the PR objectives.To ensure this change doesn't introduce any issues, let's verify its impact on the rest of the codebase:
Please review the results of these checks to ensure that all related code has been updated accordingly.
src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-task.extension.ts (2)
3-3: LGTM: Import statement added correctly.The new import for
ArtemisTextReplacementExtensionis correctly added and follows the Angular style guide for imports.
20-20: LGTM: Class extension updated correctly. Verify impact on usage.The class definition has been updated to extend
ArtemisTextReplacementExtension, which aligns with the new import. The class name follows PascalCase as required.Please verify that this change doesn't break any existing functionality. Run the following script to check for any references to the old implementation:
Verification successful
Verified: No remaining references to
ArtemisShowdownExtensionWrapperfound. ThegetExtension()method is properly defined and utilized with the new class extension.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to ArtemisShowdownExtensionWrapper rg --type typescript "ArtemisShowdownExtensionWrapper" # Search for uses of getExtension method which was removed rg --type typescript "getExtension\\(\\)"Length of output: 166
Script:
#!/bin/bash # Search for references to ArtemisShowdownExtensionWrapper in .ts and .tsx files rg "ArtemisShowdownExtensionWrapper" -g "*.ts" -g "*.tsx" # Search for uses of getExtension method which was removed in .ts and .tsx files rg "getExtension\(\)" -g "*.ts" -g "*.tsx"Length of output: 485
src/main/webapp/app/shared/markdown.service.ts (2)
28-28: Verify CSS class functionality after removing addCSSClass utility.The
htmlForMarkdownfunction call has been correctly updated to align with the new markdown-it library usage. The removal of theaddCSSClassutility simplifies the code.Please verify if the CSS class functionality previously provided by
addCSSClassis still needed. If it is, ensure that it's handled appropriately in the new implementation. Here's a script to help verify the usage of CSS classes in markdown conversion:#!/bin/bash # Description: Check for CSS class usage in markdown conversion # Test 1: Search for addCSSClass usage in the codebase echo "Checking for any remaining addCSSClass usage:" rg --type typescript "addCSSClass" src/main/webapp # Test 2: Search for CSS class addition in markdown conversion util echo "Checking for CSS class addition in markdown conversion util:" rg --type typescript "class.*=.*" src/main/webapp/app/shared/util/markdown.conversion.util.ts # Test 3: Search for markdown-it plugins that might handle CSS classes echo "Checking for markdown-it plugins that might handle CSS classes:" rg --type typescript "markdown-it-.*class" src/main/webappIf the CSS class functionality is still needed but not handled by the new implementation, consider adding a markdown-it plugin or custom renderer to handle it.
3-4: LGTM: Import statements updated correctly.The import statements have been updated to reflect the transition from showdown to markdown-it. The
htmlForMarkdownimport path change and the addition ofPluginSimpletype from markdown-it are appropriate.To ensure consistency across the codebase, let's verify the usage of these imports:
Verification successful
Verified: Import statements updated correctly.
All
showdownimports have been removed, and the new imports forhtmlForMarkdownandPluginSimplefrommarkdown-itare properly integrated across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new imports across the codebase # Test 1: Check for any remaining showdown imports echo "Checking for any remaining showdown imports:" rg --type typescript "from 'showdown'" src/main/webapp # Test 2: Verify the usage of htmlForMarkdown from the new path echo "Verifying usage of htmlForMarkdown from the new path:" rg --type typescript "import.*htmlForMarkdown.*from 'app/shared/util/markdown.conversion.util'" src/main/webapp # Test 3: Check for proper usage of PluginSimple type echo "Checking for proper usage of PluginSimple type:" rg --type typescript "import.*PluginSimple.*from 'markdown-it'" src/main/webappLength of output: 681
Script:
#!/bin/bash # Description: Verify the usage of new imports across the codebase using file extensions # Test 1: Check for any remaining showdown imports in .ts and .tsx files echo "Checking for any remaining showdown imports in .ts and .tsx files:" rg "from 'showdown'" --glob '*.ts' --glob '*.tsx' src/main/webapp # Test 2: Verify the usage of htmlForMarkdown from the new path in .ts and .tsx files echo "Verifying usage of htmlForMarkdown from the new path in .ts and .tsx files:" rg "import.*htmlForMarkdown.*from 'app/shared/util/markdown.conversion.util'" --glob '*.ts' --glob '*.tsx' src/main/webapp # Test 3: Check for proper usage of PluginSimple type in .ts and .tsx files echo "Checking for proper usage of PluginSimple type in .ts and .tsx files:" rg "import.*PluginSimple.*from 'markdown-it'" --glob '*.ts' --glob '*.tsx' src/main/webappLength of output: 2857
src/test/javascript/spec/component/lecture-unit/text-unit/text-unit.component.spec.ts (1)
30-30: LGTM, but verify markdown output and id attribute removal.The change from
<h3>to<h1>aligns with the PR objective of upgrading the markdown library. However, please ensure the following:
- Verify that the new markdown-it library actually converts
# Sample Markdownto<h1>without an id attribute. If it does generate an id, update the test expectation accordingly.- Check if the removal of the
idattribute is intentional and doesn't affect any functionality or accessibility features in the actual component.To confirm the markdown-it output, you can run the following script:
This will show the actual HTML output from markdown-it for the given markdown input.
package.json (6)
44-44: LGTM: Addition of @vscode/markdown-it-katexThe addition of
@vscode/markdown-it-katex(version 1.1.0) aligns with the transition from showdown to markdown-it for markdown processing. This plugin replaces the functionality previously provided by showdown-katex.Note: As mentioned in the PR objectives, there might be a future transition to a different library for KaTeX support. Keep this in mind for future updates.
61-63: LGTM: Addition of markdown-it and related pluginsThe addition of
markdown-it(v14.1.0) and its plugins (markdown-it-classv1.0.0,markdown-it-highlightjsv4.2.0) directly addresses the PR objective of replacing showdown with markdown-it. These up-to-date versions provide the core functionality for markdown processing and maintain features like syntax highlighting.This change aligns perfectly with the goal of upgrading the markdown library as stated in the PR objectives.
78-78: LGTM: Addition of turndown libraryThe addition of
turndown(v7.2.0) is noted. This library allows for conversion of HTML to Markdown, which could be useful in maintaining compatibility or adding new functionality.Could you please clarify the specific use case for turndown in the project? This addition wasn't explicitly mentioned in the PR objectives, so understanding its role would be helpful for future maintenance.
132-132: LGTM: Addition of type definitions for markdown-it and turndownThe addition of type definitions for
markdown-it(v14.1.2) andturndown(v5.0.5) is a good practice. These definitions will help maintain type safety and improve developer experience when working with these libraries in a TypeScript environment.Also applies to: 137-137
Line range hint
1-1: LGTM: Removal of showdown-related dependenciesWhile not visible in the provided diff, the removal of showdown-related dependencies (
showdown,showdown-highlight,showdown-katex, and@types/showdown) is a crucial part of this PR. This change directly addresses the security vulnerability mentioned in issue #9333 and aligns with the main objective of replacing showdown with markdown-it.This removal is a significant step in improving the security of the project by eliminating the vulnerable showdown library.
Line range hint
1-193: Overall assessment: Successful transition to markdown-itThe changes in this
package.jsonfile successfully implement the transition from showdown to markdown-it for markdown processing. Key points:
- All necessary markdown-it related dependencies and plugins have been added.
- Corresponding type definitions have been included for improved TypeScript support.
- The addition of turndown provides HTML to Markdown conversion capabilities.
- Showdown-related dependencies have been removed, addressing the security vulnerability from issue showdown-2.1.0.tgz: 1 vulnerabilities (highest severity is: 5.3) #9333.
These changes align well with the PR objectives and should result in a more secure and maintainable markdown processing system for the project.
src/test/javascript/spec/helpers/sample/problemStatement.json (1)
Line range hint
1-15: Overall assessment: Improvements align with PR objectivesThe changes in this file successfully enhance the visual representation of test statuses, which aligns well with the PR objectives of upgrading the markdown library and improving the UI. The addition of SVG icons provides clear and scalable visual indicators.
While the implementation is solid, consider the suggested accessibility improvements to make the UI more inclusive for users relying on screen readers.
src/main/webapp/app/exercises/quiz/shared/short-answer-question-util.service.ts (1)
376-376: LGTM: Improved code readabilityThe addition of a blank line before the
firstWordIndexcalculation improves the code's readability by visually separating the variable declarations from the subsequent logic. This change aligns with good coding practices for maintaining clean and easily readable code.src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts (2)
19-19: LGTM: Import change aligns with library upgradeThe import of
PluginSimplefrom 'markdown-it' is consistent with the PR objective of upgrading from Showdown to markdown-it library. This change appropriately reflects the transition in the markdown processing library being used.
83-83: LGTM: Property type updated for markdown-it compatibilityThe
markdownExtensionsproperty type has been correctly updated fromShowdownExtension[]toPluginSimple[]. This change ensures type safety and compatibility with the new markdown-it library's plugin system.src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts (3)
3-4: Appropriate imports for extended functionalityThe added imports for
ArtemisTextReplacementExtensionandescapeStringForUseInRegexare necessary for extending the base class and utilizing utility functions.
16-16: Class correctly extends ArtemisTextReplacementExtensionChanging the class to extend
ArtemisTextReplacementExtensionaligns with the updated architecture using markdown-it and promotes better code reuse and maintainability.
27-29: Proper initialization of the superclassThe call to
super();in the constructor ensures that the superclass is properly initialized, which is essential when extending a base class.
...ses/programming/shared/instructions-render/extensions/programming-exercise-task.extension.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/short-answer-quiz/short-answer-question-util.service.spec.ts
Show resolved
Hide resolved
...rogramming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts
Show resolved
Hide resolved
...rogramming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts
Show resolved
Hide resolved
Strohgelaender
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (2)
- src/main/webapp/app/shared/markdown.service.ts (2 hunks)
- src/main/webapp/app/shared/util/markdown.conversion.util.ts (3 hunks)
Additional context used
Path-based instructions (2)
src/main/webapp/app/shared/markdown.service.ts (1)
src/main/webapp/app/shared/util/markdown.conversion.util.ts (1)
Additional comments not posted (3)
src/main/webapp/app/shared/markdown.service.ts (2)
3-4: LGTM: Import changes align with library transition.The import changes correctly reflect the transition from showdown to markdown-it. The utility import promotes code reuse, and the type import ensures proper typing, enhancing maintainability.
21-21: LGTM: Method signature and implementation updated correctly.The changes in the
safeHtmlForMarkdownmethod align well with the transition to markdown-it:
- The parameter type change from
showdown.ShowdownExtension[]toPluginSimple[]is correct.- The removal of the
addCSSClassparameter in thehtmlForMarkdowncall suggests a change in CSS class handling.These modifications are consistent with the PR objectives and adhere to Angular style guidelines.
To ensure the changes are applied consistently across the codebase, please run the following verification script:
This script will help identify any inconsistencies in type usage or function calls related to the markdown library transition.
Also applies to: 28-28
Verification successful
Verified: Method signature and implementation updated correctly with the removal of
showdown.ShowdownExtension[]types and theaddCSSClassparameter inhtmlForMarkdownfunction calls.All changes align with the PR objectives and adhere to Angular style guidelines.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of PluginSimple[] and htmlForMarkdown function calls # Test 1: Check for any remaining showdown.ShowdownExtension[] types echo "Checking for remaining showdown.ShowdownExtension[] types:" rg --type typescript "showdown\.ShowdownExtension\[\]" # Test 2: Verify htmlForMarkdown function calls echo "Verifying htmlForMarkdown function calls:" rg --type typescript "htmlForMarkdown\("Length of output: 389
Script:
#!/bin/bash # Description: Verify consistent usage of PluginSimple[] and htmlForMarkdown function calls # Test 1: Check for any remaining showdown.ShowdownExtension[] types in TypeScript files echo "Checking for remaining showdown.ShowdownExtension[] types:" rg "showdown\.ShowdownExtension\[\]" --glob "*.ts" --glob "*.tsx" # Test 2: Verify htmlForMarkdown function calls in TypeScript files echo "Verifying htmlForMarkdown function calls:" rg "htmlForMarkdown\(" --glob "*.ts" --glob "*.tsx"Length of output: 4563
Script:
#!/bin/bash # Description: Verify removal of addCSSClass parameter in htmlForMarkdown function calls echo "Checking for any htmlForMarkdown function calls with addCSSClass parameter:" rg "htmlForMarkdown\([^)]*addCSSClass[^)]*\)" --glob "*.ts" --glob "*.tsx"Length of output: 237
src/main/webapp/app/shared/util/markdown.conversion.util.ts (1)
58-68: Verify plugin application order for correct renderingThe order in which plugins are applied in markdown-it can affect the rendering output. Currently, user-provided extensions are applied before the default plugins. Please ensure that this order yields the intended results. If necessary, consider applying default plugins before user-defined ones.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (1)
- src/main/webapp/app/shared/util/markdown.conversion.util.ts (3 hunks)
Additional context used
Path-based instructions (1)
src/main/webapp/app/shared/util/markdown.conversion.util.ts (1)
Additional comments not posted (2)
src/main/webapp/app/shared/util/markdown.conversion.util.ts (2)
26-27: Verify the correctness of backslash replacements in LaTeX contentThe code replaces
\\\\beginwith\\beginand\\\\endwith\\endin the token content. Please verify that this replacement is necessary and correctly handles LaTeX expressions. Incorrect handling of backslashes may lead to rendering issues in LaTeX formulas.
55-59: Ensure that enabling 'html: true' in markdown-it options is secureSetting
html: truein the markdown-it configuration allows raw HTML in the markdown content to be parsed and rendered. While the output is sanitized using DOMPurify, please verify that this behavior is intended and does not introduce any security vulnerabilities.
There was a problem hiding this comment.
Actionable comments posted: 12
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (5)
- src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts (3 hunks)
- src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-task.extension.ts (3 hunks)
- src/main/webapp/app/shared/markdown-editor/extensions/ArtemisTextReplacementPlugin.ts (1 hunks)
- src/main/webapp/app/shared/util/markdown.conversion.util.ts (3 hunks)
- src/test/javascript/spec/service/markdown.service.spec.ts (1 hunks)
Additional context used
Path-based instructions (5)
src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts (1)
src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-task.extension.ts (1)
src/main/webapp/app/shared/markdown-editor/extensions/ArtemisTextReplacementPlugin.ts (1)
src/main/webapp/app/shared/util/markdown.conversion.util.ts (1)
src/test/javascript/spec/service/markdown.service.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: 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}}
Biome
src/main/webapp/app/shared/util/markdown.conversion.util.ts
[error] 26-26: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 29-29: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
Additional comments not posted (8)
src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-task.extension.ts (3)
3-3: LGTM: Import statement added correctly.The new import for
ArtemisTextReplacementPluginis correctly added and aligns with the class extension change.
20-20: LGTM: Class definition updated correctly.The
ProgrammingExerciseTaskExtensionWrapperclass now extendsArtemisTextReplacementPlugin, which aligns with the PR objectives to upgrade the markdown library. This change is consistent with the new import statement and follows the coding guidelines.
34-42: LGTM: New replaceText method implemented correctly. Consider adding JSDoc.The new
replaceTextmethod is implemented correctly and aligns with the PR objectives. It effectively replaces the functionality of the removedgetExtensionmethod.As suggested in a previous review, consider adding a JSDoc comment for the
replaceTextmethod to improve code documentation. Here's a suggested addition:/** * Replaces task-related text in the input string with an escaped version. * @param text The input text to process * @returns The processed text with escaped task-related content */ replaceText(text: string): string { // ... existing implementation ... }src/test/javascript/spec/service/markdown.service.spec.ts (1)
119-123: LGTM! Comprehensive check for block formula rendering.This test case effectively verifies that block formulas without surrounding text are not converted to inline formulas. It checks for both the
class="katex-block"anddisplay="block"attributes, which are crucial for proper block formula rendering.src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts (4)
3-4: Imports added for new base class and utilitiesThe imports for
ArtemisTextReplacementPluginandescapeStringForUseInRegexare appropriately included to support the new class inheritance and utility functions.
16-16: Class refactored to extendArtemisTextReplacementPluginUpdating the class to extend
ArtemisTextReplacementPluginaligns it with the new architecture and leverages inherited functionality.
27-29: Constructor updated withsuper()callThe inclusion of
super()in the constructor ensures proper initialization of the base class.
102-113: Ensure optional properties are defined before usage
this.testCasesandthis.latestResultare optional and might beundefined. Before using them inconvertTestListToIdsandtestStatusForTask, ensure they are defined to prevent potential runtime errors.Run the following script to verify method handling of
undefinedvalues:
src/main/webapp/app/shared/markdown-editor/extensions/ArtemisTextReplacementPlugin.ts
Outdated
Show resolved
Hide resolved
...rogramming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts
Outdated
Show resolved
Hide resolved
...rogramming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts
Outdated
Show resolved
Hide resolved
…extReplacementPlugin.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nto chore/markdown-it-replacement
Strohgelaender
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (4)
- src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts (3 hunks)
- src/main/webapp/app/shared/markdown-editor/extensions/ArtemisTextReplacementPlugin.ts (1 hunks)
- src/main/webapp/app/shared/util/markdown.conversion.util.ts (3 hunks)
- src/test/javascript/spec/service/markdown.service.spec.ts (1 hunks)
Additional context used
Path-based instructions (4)
src/main/webapp/app/exercises/programming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts (1)
src/main/webapp/app/shared/markdown-editor/extensions/ArtemisTextReplacementPlugin.ts (1)
src/main/webapp/app/shared/util/markdown.conversion.util.ts (1)
src/test/javascript/spec/service/markdown.service.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: 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}}
Biome
src/main/webapp/app/shared/util/markdown.conversion.util.ts
[error] 26-26: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 29-29: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
Additional comments not posted (11)
src/test/javascript/spec/service/markdown.service.spec.ts (5)
113-117: LGTM! Consider adding a more specific expectation.The test case effectively checks for the conversion of block formulas to inline formulas using
it.each. This approach is efficient and follows Jest conventions.As suggested in a previous review, consider adding a more specific expectation to ensure the entire formula is correctly rendered. For example:
expect(result).toContain('<span class="katex"><span class="katex-mathml"><math xmlns="http://www.w3.org/1998/Math/MathML">');This would provide a stronger assertion that the formula is being properly processed by the KaTeX renderer.
119-123: LGTM! Consider adding a negative assertion for consistency.The test case correctly verifies that block formulas without surrounding text are not converted to inline formulas. The expectations are specific and align with the expected behavior.
For consistency with other test cases, consider adding a negative assertion:
expect(result).not.toContain('<span class="katex">');This would explicitly verify that the inline formula rendering is not present.
125-129: LGTM! Consider adding a negative assertion.This test case effectively checks the conversion of double-backslash LaTeX begin and end tags. It verifies the presence of both
<span class="katex">andclass="katex-html", which are crucial for proper LaTeX rendering.As suggested in a previous review, consider adding a negative assertion to ensure that the original LaTeX tags are not present in the rendered output:
expect(result).not.toContain('\\\\begin{equation}'); expect(result).not.toContain('\\\\end{equation}');This would provide additional confidence that the LaTeX tags are being properly processed and not left as plain text in the output.
131-137: LGTM! Consider adding an assertion for inline rendering.This test case effectively verifies the handling of multiple formulas in the same text. The use of regex to count occurrences is a clever approach, and the assertions for the absence of block formulas are good.
To further improve the test based on a previous suggestion, consider adding an assertion to explicitly check for inline rendering:
expect(result).toContain('<span class="katex-inline">');This would provide additional confidence that the formulas are being rendered inline as expected when surrounded by text.
112-138: Great addition of test cases forformulaCompatibilityPlugin.The new test suite for
formulaCompatibilityPluginprovides comprehensive coverage for various formula rendering scenarios. The tests are well-structured, follow Jest conventions, and align with the provided coding guidelines.To further improve coverage, consider adding the following test cases as suggested in a previous review:
- Test with an empty formula:
$$$$- Test with invalid LaTeX syntax
- Test with nested formulas
- Test with very long formulas to ensure they don't break the layout
These additional test cases would help ensure the robustness of the
formulaCompatibilityPluginimplementation.src/main/webapp/app/shared/markdown-editor/extensions/ArtemisTextReplacementPlugin.ts (1)
12-12: Add error handling forreplaceTextto prevent runtime exceptionsIf
replaceTextthrows an error, it could cause the rendering process to fail. Consider adding error handling to prevent unhandled exceptions and ensure robustness.src/main/webapp/app/shared/util/markdown.conversion.util.ts (5)
1-8: Updated imports align with new markdown-it implementationThe import statements have been successfully updated to incorporate
markdown-itand its plugins, enhancing the markdown parsing capabilities and addressing security concerns outlined in the PR objectives.
17-18: Inline formula regex is appropriateThe
inlineFormulaRegexis designed to identify inline LaTeX formulas using$$delimiters when they are surrounded by other characters. The regex appears to correctly handle the intended cases for inline formulas.
20-36: Effective implementation of 'FormulaCompatibilityPlugin'The
FormulaCompatibilityPluginclass extendsArtemisTextReplacementPluginand overrides thereplaceTextmethod to ensure compatibility with existing LaTeX formulas. The logic for processing each line and replacing delimiters and backslashes is sound and enhances backward compatibility.Tools
Biome
[error] 26-26: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 29-29: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
70-75: Correct addition of markdown-it pluginsThe default extensions for code highlighting and LaTeX rendering are properly included using
markdownItHighlightjs,formulaCompatibilityPlugin,markdownItKatex, andmarkdownItClass. The configuration options, such asenableMathInlineInHtml: true, are correctly set to support inline math within HTML.
92-92: Sanitization with DOMPurify maintains securityThe sanitization process using
DOMPurify.sanitizeis correctly configured to include additional tags and attributes when provided. This ensures that the rendered HTML is safe while allowing necessary elements for functionality.
src/main/webapp/app/shared/markdown-editor/extensions/ArtemisTextReplacementPlugin.ts
Show resolved
Hide resolved
...rogramming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts
Show resolved
Hide resolved
...rogramming/shared/instructions-render/extensions/programming-exercise-plant-uml.extension.ts
Show resolved
Hide resolved
Checklist
General
Client
Motivation and Context
The currently used showdown library for markdown parsing is unmaintained, see #9333.
This PR replaces it with the more modern library
markdown-it.Closes #9333.
Description
These dependencies were added:
markdown-it: Replacement forshowdown(Markdown -> HTML)turndown: Replacement forshowdown(HTML -> Markdown)markdown-it-katex: Replacement forshowdown-katex.markdown-it-highlightjs: Replacement forshowdown-highlightmarkdown-it-classReplaces the custom extension to improve table styling.I migrated the Task and PlantUML extensions to MarkdownIt Plugins and added a custom one that ensues backwards compatibility for inline formulas.
This fixes issue #768, but formulas with escaped underscores now don't work. I did not find a away to ensure compatibility here, feel free to suggest something.
Steps for Testing
Please test the different places where Markdown is rendered, especially:
I created a small exercise on TS2 that includes these special cases: https://artemis-test2.artemis.cit.tum.de/course-management/10/programming-exercises/283
Deploy this and another PR and check that the Problem Statement renders correctly.
Also test quiz exercises, especially short answer quizzes, here the markdown parsing has some additional complexity.
Exam Mode Testing
Please test that problem statements are correctly rendered when taking an exam. Please test both the online code editor and the exercise view (when only offline editors are activated). Also test quiz exercises of the different types.
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
New Features
markdown-itlibrary for enhanced markdown processing.Bug Fixes
Documentation
Refactor
markdown-itequivalents.Tests