Communication: Add additional input formatting options
#9657
Conversation
github-actions
bot
added
tests
client
WalkthroughThis pull request introduces several modifications to the text editing capabilities within the Angular application, particularly focusing on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 6
🧹 Outside diff range and nitpick comments (16)
src/main/webapp/app/shared/monaco-editor/model/actions/bulleted-list.action.ts (2)
4-4: Consider improving constant naming and accessibilityThe bullet character (•) might cause accessibility issues with screen readers. Consider using a more accessible ASCII character like '*' or '-'.
-const BULLET_PREFIX = '• '; +const BULLET_LIST_PREFIX = '* ';
6-9: Enhance JSDoc documentationWhile the documentation is clear, it would be helpful to include:
- @example showing usage
- @see reference to ListAction base class
- Description of how bullet points are handled
/** * Action used to add or modify a bullet-point list in the text editor. + * + * @example + * ```typescript + * const action = new BulletedListAction(); + * action.run(editor); // Adds/toggles bullet points for selected lines + * ``` + * + * @see ListAction The base class providing common list functionality */src/main/webapp/app/shared/monaco-editor/model/actions/strikethrough.action.ts (2)
7-11: Enhance class documentation with more details.While the basic documentation is present, it would be beneficial to add:
- @example showing usage
- @see reference to related markdown documentation
- Description of when this action is available/enabled
Here's a suggested enhancement:
/** - * Action to toggle strikethrough text in the editor. + * Action to toggle strikethrough text formatting in the editor using markdown syntax. + * + * @example + * // Selected text: "example" + * // After action: "~~example~~" + * + * @see https://www.markdownguide.org/extended-syntax/#strikethrough */
1-26: Well-designed action class following the Command pattern.The implementation follows good architectural principles:
- Encapsulation of formatting logic in a dedicated action class
- Clean integration with Monaco editor through the TextEditor interface
- Consistent with other formatting actions in the codebase
Consider adding an interface for all formatting actions to enforce consistent implementation across similar features (strikethrough, bold, italic, etc.).
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.component.html (1)
Line range hint
1-79: Consider splitting component responsibilitiesThe component currently handles multiple concerns including content processing, reference handling, image management, and event handling. Consider splitting these responsibilities into separate components or services for better maintainability and testing:
- Create a dedicated content processor service
- Extract image handling into a separate component
- Move reference handling logic to a dedicated service
This would align with the Single Responsibility Principle and make the codebase more maintainable as new formatting options are added.
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (3)
48-49: Add type annotations for processed content properties.For better type safety and code maintainability, explicitly declare the types of the processed content properties.
- processedContentBeforeReference: string; - processedContentAfterReference: string; + processedContentBeforeReference: string | undefined; + processedContentAfterReference: string | undefined;
57-59: Consider implementing OnDestroy for cleanup.To prevent memory leaks, implement OnDestroy to clean up any subscriptions and event listeners, particularly for
userReferenceClickedandchannelReferenceClickedEventEmitters.-export class PostingContentPartComponent implements OnInit { +export class PostingContentPartComponent implements OnInit, OnDestroy { + ngOnDestroy() { + // Clean up subscriptions and event listeners + }
84-86: Enhance numbered list processing for edge cases.The current regex pattern might miss some edge cases. Consider:
- Multiple digits at the start
- Different types of whitespace
- Empty lines between list items
escapeNumberedList(content: string): string { - return content.replace(/^(\s*\d+)\. /gm, '$1\\. '); + if (!content) return ''; + return content.replace(/^([ \t]*\d+)\. (?=\S)/gm, '$1\\. '); }src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (1)
Line range hint
91-115: Consider refactoring action initialization for better maintainability.As the number of available actions grows, consider implementing a factory pattern or using dependency injection for action initialization. This would improve maintainability and make it easier to add/remove actions in the future.
Example approach:
// Create an action factory service @Injectable() class TextEditorActionFactory { createActions(options: EditorOptions): TextEditorAction[] { return [ // Basic formatting this.createBasicFormattingActions(), // List formatting this.createListFormattingActions(), // Advanced formatting this.createAdvancedFormattingActions(), // Special actions this.createSpecialActions(options), ].flat(); } }src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (1)
113-118: Consider optimizing for performance.For better performance in real-time editing scenarios, consider:
- Using regex replacement instead of split/join operations
- Implementing lazy evaluation for large texts
- Adding input validation
Here's a more efficient implementation:
convertTextToEmoji(text: string): string { - const words = text.split(' '); - const convertedWords = words.map((word) => { - return word.startsWith(':') ? this.emojiConvertor.replace_emoticons(word) : word; - }); - - return convertedWords.join(' '); + if (!text?.trim()) { + return text; + } + // Use regex to find and replace only words starting with ':' + return text.replace(/(\s|^):[^\s]+/g, (match) => { + const converted = this.emojiConvertor.replace_emoticons(match); + // If no conversion occurred, return original match + return converted === match ? match : converted; + }); }src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (1)
335-376: Consider enhancing the helper function with types and documentation.The
simulateListActionhelper is well-implemented but could be more maintainable with:
- TypeScript type annotations
- JSDoc documentation
- Separated verification logic
+/** + * Simulates a list action on the mock editor and verifies the result + * @param action - The text editor action to simulate + * @param selectedText - The text currently selected in the editor + * @param expectedText - The expected text after the action is applied + * @param startLineNumber - The starting line number (default: 1) + */ -const simulateListAction = (action: TextEditorAction, selectedText: string, expectedText: string, startLineNumber: number = 1) => { +const simulateListAction = ( + action: TextEditorAction, + selectedText: string, + expectedText: string, + startLineNumber: number = 1 +): void => { // ... setup code ... - action.run(mockEditor); + action.run(mockEditor); + verifyActionResult(mockEditor, expectedText); +}; + +/** + * Verifies the result of a list action + * @param editor - The mock editor instance + * @param expectedText - The expected text after the action + */ +const verifyActionResult = (editor: jest.Mocked<TextEditor>, expectedText: string): void => { const replaceCalls = mockEditor.replaceTextAtRange.mock.calls; expect(replaceCalls).toHaveLength(1); const [range, text] = replaceCalls[0]; // ... verification code ... };src/main/webapp/app/shared/monaco-editor/model/actions/ordered-list.action.ts (3)
5-5: Ensure accuracy in the class documentationThe class documentation states "Action used to add or modify a numbered list in the text editor." Ensure that this description accurately reflects the functionality of the
OrderedListActionclass.
8-8: Ensure consistent naming between ID and class nameThe
IDis set to'numberedList.action', but the class is namedOrderedListAction. For consistency and clarity, consider aligning theIDwith the class name.Apply this diff to synchronize the
IDwith the class name:-static readonly ID = 'numberedList.action'; +static readonly ID = 'orderedList.action';
19-19: Remove unusedPREFIXpropertyThe
PREFIXproperty is declared but not assigned or utilized within the class. If it's unnecessary, consider removing it to keep the code clean.Apply this diff to remove the unused property:
-protected readonly PREFIX: string;src/main/webapp/app/shared/monaco-editor/model/actions/list.action.ts (2)
102-107: [Refactor Suggestion] Avoid hardcoding prefix values to enhance maintainabilityThe code is using a hardcoded value
'• 'to determine whether the list is numbered or bulleted, which can lead to maintenance issues and makes the code less flexible. Instead, consider introducing anisNumberedListmethod or property to encapsulate this logic. This change will make the code more readable and easier to extend in the future.
39-46: [Refactor Suggestion] Extract common regular expressions into constantsThe regular expressions used to detect list prefixes are repeated in multiple methods (
stripAnyListPrefix,hasPrefix, andhandleBackspace). To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider extracting these regular expressions into constants or utility functions. This will make any future changes easier and reduce the risk of inconsistencies.Also applies to: 132-135, 179-190
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
src/main/webapp/app/exercises/quiz/manage/short-answer-question/short-answer-question-edit.component.ts(2 hunks)src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.component.html(2 hunks)src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts(5 hunks)src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts(2 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/bulleted-list.action.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/list.action.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/ordered-list.action.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/strikethrough.action.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts(1 hunks)src/main/webapp/i18n/de/multipleChoiceQuestion.json(1 hunks)src/main/webapp/i18n/en/multipleChoiceQuestion.json(1 hunks)src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts(5 hunks)src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-action.integration.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
src/main/webapp/app/exercises/quiz/manage/short-answer-question/short-answer-question-edit.component.ts (1)
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts (1)
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (1)
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/bulleted-list.action.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/list.action.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/ordered-list.action.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/strikethrough.action.ts (1)
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (1)
src/main/webapp/i18n/de/multipleChoiceQuestion.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.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/monaco-editor/monaco-editor-action.integration.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}}
🔇 Additional comments (25)
src/main/webapp/app/shared/monaco-editor/model/actions/bulleted-list.action.ts (1)
14-16: Clarify the purpose of undefined parameter in super call
The constructor passes undefined as the last parameter to the base class. Please document why this parameter is undefined and what it represents.
src/main/webapp/app/shared/monaco-editor/model/actions/strikethrough.action.ts (3)
1-5: LGTM! Well-organized imports and properly defined constant.
The imports are specific and follow Angular guidelines. The strikethrough delimiter constant is appropriately defined and follows naming conventions.
13-15: Verify localization key and clarify undefined parameter.
The constructor follows the parent class structure, but there are two points to verify:
- Ensure the localization key exists in the translation files
- The purpose of the undefined parameter should be documented
17-25: LGTM! Clean implementation with good documentation.
The run method is well-implemented with proper focus management. Consider adding @returns void to complete the JSDoc.
✅ Verification successful
Based on the search results, I can now generate a final response. We found the base TextEditorAction class and its implementation of toggleDelimiterAroundSelection which is used by all text formatting actions including StrikethroughAction. The implementation shows proper handling of text selection, delimiter toggling, and cursor positioning.
Implementation verified and follows established patterns
The StrikethroughAction correctly inherits from TextEditorAction and uses the shared toggleDelimiterAroundSelection method which:
- Handles text selection state
- Toggles delimiters (adds/removes them)
- Maintains proper cursor position
- Focuses the editor after the operation
The suggestion to add @returns void to JSDoc is valid but optional since TypeScript already shows the return type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the behavior of toggleDelimiterAroundSelection in parent class
# Test: Look for the implementation of toggleDelimiterAroundSelection
ast-grep --pattern 'toggleDelimiterAroundSelection($$$) {
$$$
}'
Length of output: 68
Script:
#!/bin/bash
# Let's try to find the parent class and the method implementation using ripgrep
rg "toggleDelimiterAroundSelection" -A 10
Length of output: 6123
Script:
#!/bin/bash
# Let's examine the parent class TextEditorAction to understand the inheritance
rg "class.*TextEditorAction" -A 5
# And also check if there are any tests for the strikethrough action
rg "describe.*StrikethroughAction" -A 10
Length of output: 17174
src/main/webapp/i18n/en/multipleChoiceQuestion.json (1)
49-50: LGTM! The localization entries are well-structured.
The new strikethrough entry follows consistent formatting and naming conventions, properly integrating with existing editor options.
src/main/webapp/i18n/de/multipleChoiceQuestion.json (1)
49-50: LGTM! Translation and syntax are correct.
The German translation "Durchgestrichen" for strikethrough is accurate and consistent with the style of other formatting options. The JSON syntax is valid with the proper comma placement.
✅ Verification successful
Translation and JSON syntax are verified and correct
The verification confirms:
- JSON syntax is valid in the German translation file
- "Durchgestrichen" correctly translates the English "Strikethrough"
- The translation maintains consistency with other formatting options in the editor section
- The informal style matches the rest of the file (e.g., "Du" is used elsewhere)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify JSON syntax
jq '.' src/main/webapp/i18n/de/multipleChoiceQuestion.json
# Check if the same key exists in English file for consistency
jq -r '.artemisApp.multipleChoiceQuestion.editor.strikethrough' src/main/webapp/i18n/en/multipleChoiceQuestion.json
Length of output: 2715
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.component.html (3)
1-1: Excellent use of new Angular control flow syntax!
The template consistently uses the new @if syntax instead of the older *ngIf, which aligns perfectly with the coding guidelines.
Also applies to: 8-8, 14-14, 18-18, 22-22, 26-26, 29-29, 39-39, 47-47, 74-74
75-79: LGTM: Consistent implementation for after-reference content
The implementation mirrors the before-reference section, maintaining consistency in how content is processed and displayed.
2-6: LGTM: Content processing enhancement for line breaks
The addition of white-space: pre-wrap and use of processedContentBeforeReference aligns with the PR objective of preserving line breaks in messages. The htmlForPostingMarkdown pipe ensures proper rendering of the new formatting options.
Let's verify the content processing implementation:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Content processing implementation verified successfully
The implementation correctly handles line breaks and content processing through:
processedContentBeforeReferenceis properly processed usingescapeNumberedListmethod- The
white-space: pre-wrapstyle works in conjunction with the markdown processing - The
htmlForPostingMarkdownpipe correctly transforms the content with proper sanitization and markdown conversion - Allowed HTML tags include 'br' and other formatting elements needed for proper rendering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of content processing in the component
# Expected: Find the content processing logic that handles the new formatting
# Look for the component class implementation
ast-grep --pattern 'class PostingContentPartComponent {
$$$
processedContentBeforeReference$_
$$$
}'
# Look for content processing method
ast-grep --pattern 'processContent($_) {
$$$
}'
Length of output: 284
Script:
#!/bin/bash
# Let's try a different approach to find the component implementation
# First, find the component file
fd "posting-content-part.component.ts"
# Then use ripgrep to look for the content processing logic
rg "processedContentBeforeReference" -A 5
rg "class PostingContentPartComponent" -A 10
Length of output: 4707
Script:
#!/bin/bash
# Let's look for the escapeNumberedList method implementation
rg "escapeNumberedList" -A 10
# Also check for the htmlForPostingMarkdown pipe implementation
fd "html-for-posting-markdown.pipe.ts"
rg "transform.*htmlForPostingMarkdown" -A 5
Length of output: 2874
Script:
#!/bin/bash
# Let's examine the content of the markdown pipe implementation
cat src/main/webapp/app/shared/pipes/html-for-posting-markdown.pipe.ts
# Also check if there are any related tests for this functionality
fd "posting-content-part.component.spec.ts"
Length of output: 1536
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (1)
38-39: Verify security implications of HTML sanitization.
The allowed HTML tags list has been modified. Please ensure that the Angular sanitizer is properly handling these tags to prevent XSS attacks.
✅ Verification successful
Let me check the specific implementation related to the HTML sanitization in the posting content component and related files.
Let me check the implementation of the htmlForPostingMarkdown pipe since it's being used to process the content with the allowed tags.
Let me check the implementation of ArtemisMarkdownService to see how it handles the sanitization.
Let me check the final piece - the markdown conversion util implementation since it's using DOMPurify for sanitization.
HTML sanitization is properly implemented and secure
The code implements a robust security approach for HTML sanitization:
- Uses DOMPurify for sanitization with explicit allowlist of HTML tags and attributes
- Applies Angular's DomSanitizer as an additional security layer
- Follows proper sanitization flow:
- Markdown conversion → DOMPurify sanitization → Angular sanitizer
- Custom tags/attributes are explicitly controlled via allowlists
- No direct bypassing of security without sanitization
The modified HTML tags list is safe as it's properly enforced through DOMPurify's ALLOWED_TAGS configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DomSanitizer usage in the template and related files
rg -l "DomSanitizer|bypassSecurityTrustHtml" --type ts
Length of output: 696
Script:
#!/bin/bash
# Check the implementation of posting-content-part component and its template
rg -A 10 "allowedHtmlTags" src/main/webapp/app/shared/metis/posting-content/posting-content-part/
# Check the safe-html pipe implementation
cat src/main/webapp/app/shared/pipes/safe-html.pipe.ts
# Look for sanitization related code in the component
rg -A 5 "sanitize|sanitization|sanitizer" src/main/webapp/app/shared/metis/posting-content/posting-content-part/
Length of output: 4716
Script:
#!/bin/bash
# Check the implementation of htmlForPostingMarkdown pipe
rg -l "htmlForPostingMarkdown" --type ts
# Once found, let's see its implementation
cat src/main/webapp/app/shared/pipes/html-for-posting-markdown.pipe.ts
Length of output: 1497
Script:
#!/bin/bash
# Check the markdown service implementation
cat src/main/webapp/app/shared/markdown.service.ts
# Also check for any DOMPurify usage as it's a common sanitization library
rg -A 5 "DOMPurify" --type ts
Length of output: 6400
Script:
#!/bin/bash
# Check the markdown conversion util implementation
cat src/main/webapp/app/shared/util/markdown.conversion.util.ts
Length of output: 3743
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (3)
42-44: LGTM: New action imports follow Angular conventions.
The imports for the new formatting actions are properly organized and follow the Angular style guide naming conventions.
Line range hint 65-69: LGTM: Well-documented input property using modern Angular syntax.
The activeConversation input is properly typed, immutable, and well-documented. The usage of the new input() syntax aligns with modern Angular practices.
103-106: Verify cleanup of action instances.
The new formatting actions are logically placed within the text formatting group. However, since these actions are instantiated in ngOnInit, please ensure they are properly cleaned up to prevent memory leaks.
Let's verify if the actions implement cleanup methods:
✅ Verification successful
No cleanup concerns - proper disposal is implemented
The review comment's concern about cleanup is addressed by the existing implementation:
- All actions inherit from
TextEditorActionwhich implementsDisposableinterface with a properdispose()method that cleans up editor registrations EmojiActionspecifically has additional cleanup throughdestroyEmojiPicker()that handles its overlay component- The actions are instantiated in
ngOnInitand will be automatically disposed when the component is destroyed through Angular's lifecycle
The implementation follows proper cleanup patterns and no memory leaks are expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new actions implement destroy/cleanup methods
ast-grep --pattern 'class $_(Action) {
$$$
destroy() {
$$$
}
$$$
}'
Length of output: 81
Script:
#!/bin/bash
# Let's check the implementation of these specific action classes
rg -A 10 "class (Strikethrough|BulletedList|OrderedList)Action"
# Also check the base class or interface they might implement
rg -A 5 "interface.*Action|class.*Action.*\{"
Length of output: 339924
Script:
#!/bin/bash
# Let's check the base class TextEditorAction to see if it has cleanup/destroy methods
rg -A 10 "class TextEditorAction.*implements Disposable"
# Also check if EmojiAction has any special cleanup
rg -A 10 "class EmojiAction"
Length of output: 2621
Script:
#!/bin/bash
# Let's check if EmojiAction has any cleanup/destroy methods
rg -B 5 -A 10 "destroy\(\)|dispose\(\)" src/main/webapp/app/shared/monaco-editor/model/actions/emoji.action.ts
# Also check the full implementation of TextEditorAction's dispose method
rg -B 5 -A 10 "dispose\(\)" src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts
Length of output: 1153
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-action.integration.spec.ts (1)
95-95: Verify the double space requirement after ordered list marker.
The test now expects two spaces after the ordered list marker ('1. '). While this change aligns with some markdown specifications, we should verify if this is intentional and consistent with the application's markdown parser.
✅ Verification successful
Let me gather more context about the ordered list formatting implementation and tests.
Let me search for more context about the ordered list formatting in the codebase.
Let me search for the implementation of the ordered list formatting to understand if the double space is intentional.
Let me try one more search to find the ordered list action implementation.
Based on the implementation found in ordered-list.action.ts, I can now provide a final response:
The double space after ordered list marker is intentional and correct.
The implementation in OrderedListAction class explicitly handles spacing to ensure consistent alignment:
public getPrefix(lineNumber: number): string {
const space = lineNumber >= 10 ? ' ' : ' ';
return `${lineNumber}.${space}`;
}The code adds two spaces for single-digit numbers (1-9) and one space for double-digit numbers (10+) to maintain consistent alignment of list content. This is a common markdown formatting practice to ensure proper rendering across different markdown parsers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the double space after ordered list marker is consistently used across the codebase
# Look for other test files that might be testing ordered list formatting
rg -g '*.spec.ts' -A 2 "expect\(.*\.getText\(\)\)\.toBe\('1\."
Length of output: 412
Script:
#!/bin/bash
# Check the implementation of ordered list formatting
ast-grep --pattern 'function $FUNC($$$) {
$$$
$TEXT = $$.replace(/^\d+\./g, $$$)
$$$
}'
# Look for markdown-related configuration or constants
rg -g '*.ts' 'const.*LIST.*=.*1\.' -B2 -A2
# Check for any documentation about markdown formatting
fd README.md --exec cat {}
Length of output: 64619
Script:
#!/bin/bash
# Look for markdown formatting related files and configurations
rg -g '*.ts' -g '*.js' 'orderedList|ordered-list' -B2 -A2
# Check for any Monaco editor formatting related code
ast-grep --pattern 'function $FUNC($$$) {
$$$
text = $$.replace(/^\d+\./g, $$$)
$$$
}'
# Look for test files related to Monaco editor formatting
rg -g '*.spec.ts' 'monaco.*format' -B2 -A2
Length of output: 25290
Script:
#!/bin/bash
# Look for the ordered list action implementation
ast-grep --pattern 'class $CLASS extends $BASE {
$$$
static readonly ID = "ordered-list.action"
$$$
}'
# Search for list-related constants or configurations
rg -g '*.ts' 'LIST_.*=.*' -B2 -A2
# Look for markdown formatting related tests
rg -g '*.spec.ts' 'markdown.*format|format.*markdown' -B2 -A2
Length of output: 1426
Script:
#!/bin/bash
# Look for ordered list implementation
rg -g '*.ts' 'class OrderedListAction' -B5 -A20
# Look for list formatting related constants
rg -g '*.ts' 'const.*LIST.*=.*\d+\.' -B2 -A2
# Check for markdown formatting configuration
rg -g '*.ts' 'markdown.*config|format.*config' -B2 -A2
Length of output: 3437
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (1)
113-118: LGTM! Consider handling additional edge cases.
The implementation correctly addresses the requirement to only convert words starting with ':' to emojis. However, be aware of these edge cases:
- Multiple spaces between words might be collapsed
- Emojis within words (not space-separated) won't be converted
- Special whitespace characters (newlines, tabs) might not be preserved
Let's verify the edge cases:
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (3)
32-38: LGTM! Well-structured imports and comprehensive mock implementation.
The new imports are well-organized, and the mock editor implementation is thorough, including all necessary methods for testing text editor functionality.
Also applies to: 62-80
107-109: LGTM! Proper test isolation with WeakMap cleanup.
The cleanup of ListAction listeners using WeakMap in beforeEach ensures proper test isolation and prevents test pollution.
378-490: LGTM! Comprehensive test coverage for list formatting features.
The test suite thoroughly covers:
- Basic list formatting (bulleted and ordered)
- List type switching
- Prefix toggling
- Edge cases with inline lists
- Interaction between different list types
The tests are well-organized, follow the AAA pattern, and properly clean up between test cases.
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts (3)
29-30: LGTM! Import changes align with new formatting features.
The new imports for BulletedListAction and StrikethroughAction are properly organized and follow Angular style guidelines.
163-163: LGTM! New formatting actions are properly integrated.
The new actions are logically placed in the defaultActions array:
- StrikethroughAction is grouped with other text formatting actions
- BulletedListAction is appropriately placed next to OrderedListAction
Also applies to: 170-170
163-163: Verify integration of new formatting features.
Please ensure that:
- The preview mode correctly renders the new strikethrough and bulleted list formatting
- The visual mode supports these new formatting options
- The markdown parser handles these new formats correctly
Also applies to: 170-170
✅ Verification successful
The previous search failed due to incorrect paths. Let's find the actual file locations and then check their implementations for handling these markdown features.
Let's verify if the markdown service and content components properly handle the rendering of these markdown features.
We need to verify how the markdown service is configured and how the HTML pipe processes the markdown content to ensure proper rendering support for these features.
We need to check the actual markdown conversion implementation to verify if strikethrough and bullet lists are properly configured in the markdown-it setup.
Based on the comprehensive analysis of the codebase, I can now provide the final response:
Strikethrough and bullet list formatting are properly supported
The implementation shows that:
- The markdown-it library is properly configured with HTML support enabled
- The content is rendered through the
HtmlForPostingMarkdownPipewhich usesArtemisMarkdownService - The markdown preview is displayed in elements with the
markdown-previewclass - The content is properly sanitized while preserving HTML formatting
The new formatting actions are well-integrated with the existing markdown rendering pipeline and will work correctly in both preview and final display.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if preview/visual mode components handle new formatting
echo "Checking preview mode handling..."
rg -A 5 "strikethrough|bullet" "src/main/webapp/**/posting-content-part.component.*"
echo "Checking visual mode handling..."
rg -A 5 "strikethrough|bullet" "src/main/webapp/**/visual-editor.component.*"
echo "Checking markdown parser handling..."
rg -A 5 "strikethrough|bullet" "src/main/webapp/**/markdown.service.*"
Length of output: 729
Script:
#!/bin/bash
# Let's first find the correct paths for these files
echo "Finding relevant files..."
fd -e ts -e html "posting-content|visual-editor|markdown.service"
echo "Checking markdown rendering implementation..."
rg -A 5 "marked" --type ts
echo "Checking strikethrough configuration..."
rg -A 5 "strikethrough|bullet" --type ts
Length of output: 51273
Script:
#!/bin/bash
# Let's check the markdown service implementation for rendering support
echo "Checking markdown service implementation..."
rg -A 10 "marked\.setOptions|marked\.parse" src/main/webapp/app/shared/markdown.service.ts
# Check if there are any tests for markdown rendering
echo "Checking markdown rendering tests..."
rg -A 5 "strikethrough|bullet" src/test/javascript/spec/service/markdown.service.spec.ts
# Check if the posting content component handles markdown rendering
echo "Checking posting content component..."
rg -A 5 "markdown" src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.component.html
Length of output: 1455
Script:
#!/bin/bash
# Let's check the markdown service implementation directly
echo "Checking markdown service file content..."
cat src/main/webapp/app/shared/markdown.service.ts
# Check the HTML pipe that processes markdown
echo "Checking HTML pipe implementation..."
rg -A 10 "class HtmlForPostingMarkdownPipe" --type ts
Length of output: 4956
Script:
#!/bin/bash
# Let's check the markdown conversion utility implementation
echo "Checking markdown conversion utility..."
cat src/main/webapp/app/shared/util/markdown.conversion.util.ts
# Check if there are any markdown-it configuration files
echo "Checking for markdown-it configuration..."
fd -e ts "markdown-it|markdown.config"
Length of output: 3966
src/main/webapp/app/exercises/quiz/manage/short-answer-question/short-answer-question-edit.component.ts (1)
35-36: LGTM! New formatting actions imported.
The imports align with the PR objectives to add strikethrough and bulleted list formatting options.
src/main/webapp/app/shared/monaco-editor/model/actions/ordered-list.action.ts (3)
2-2: LGTM!
The import statement correctly imports ListAction to extend common list functionalities.
7-7: Confirm inheritance from ListAction
Changing the superclass to ListAction is appropriate if you're unifying list actions under a common parent. Ensure that all necessary functionalities are correctly inherited.
14-16: LGTM!
The getPrefix method correctly generates the numbered list prefix based on the line number.
| protected getPrefix(lineNumber: number): string { | ||
| void lineNumber; | ||
| return this.PREFIX; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve handling of unused lineNumber parameter
The lineNumber parameter is marked as unused with void. If the parameter isn't needed, consider:
- Removing it if not required by base class contract
- Implementing line number-based prefixing if it should affect the bullet style
If the parameter is truly unused, apply this change:
- protected getPrefix(lineNumber: number): string {
- void lineNumber;
- return this.PREFIX;
- }
+ protected getPrefix(): string {
+ return this.PREFIX;
+ }Committable suggestion skipped: line range outside the PR's diff.
| processContent() { | ||
| if (this.postingContentPart.contentBeforeReference) { | ||
| this.processedContentBeforeReference = this.escapeNumberedList(this.postingContentPart.contentBeforeReference); | ||
| } | ||
|
|
||
| if (this.postingContentPart.contentAfterReference) { | ||
| this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart.contentAfterReference); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling in processContent method.
The method should handle cases where postingContentPart is undefined or null to prevent runtime errors.
processContent() {
+ if (!this.postingContentPart) {
+ return;
+ }
+
if (this.postingContentPart.contentBeforeReference) {
this.processedContentBeforeReference = this.escapeNumberedList(this.postingContentPart.contentBeforeReference);
}
if (this.postingContentPart.contentAfterReference) {
this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart.contentAfterReference);
}
}📝 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.
| processContent() { | |
| if (this.postingContentPart.contentBeforeReference) { | |
| this.processedContentBeforeReference = this.escapeNumberedList(this.postingContentPart.contentBeforeReference); | |
| } | |
| if (this.postingContentPart.contentAfterReference) { | |
| this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart.contentAfterReference); | |
| } | |
| } | |
| processContent() { | |
| if (!this.postingContentPart) { | |
| return; | |
| } | |
| if (this.postingContentPart.contentBeforeReference) { | |
| this.processedContentBeforeReference = this.escapeNumberedList(this.postingContentPart.contentBeforeReference); | |
| } | |
| if (this.postingContentPart.contentAfterReference) { | |
| this.processedContentAfterReference = this.escapeNumberedList(this.postingContentPart.contentAfterReference); | |
| } | |
| } |
| action.executeInCurrentEditor(); | ||
| expect(comp.getText()).toBe('1. '); | ||
| }); | ||
|
|
||
| it('should toggle ordered list, skipping empty lines', () => { | ||
| const action = new OrderedListAction(); | ||
| comp.registerAction(action); | ||
| const lines = ['One', '', 'Two', 'Three']; | ||
| const numberedLines = lines.map((line, index) => (line ? `${index + 1}. ${line}` : '')); | ||
| comp.setText(lines.join('\n')); | ||
| comp.setSelection({ startLineNumber: 1, startColumn: 1, endLineNumber: lines.length, endColumn: lines[lines.length - 1].length + 1 }); | ||
| // Introduce list | ||
| action.executeInCurrentEditor(); | ||
| expect(comp.getText()).toBe(numberedLines.join('\n')); | ||
| // Remove list | ||
| action.executeInCurrentEditor(); | ||
| expect(comp.getText()).toBe(lines.join('\n')); | ||
| expect(comp.getText()).toBe('1. '); | ||
| }); |
There was a problem hiding this comment.
Add test coverage for ordered list toggling.
The test suite is missing coverage for toggling ordered lists. Similar to the unordered list test at lines 40-54, we should add a test case to verify that ordered lists can be properly toggled while skipping empty lines.
Here's a suggested test case to add:
it('should toggle ordered list, skipping empty lines', () => {
const action = new OrderedListAction();
comp.registerAction(action);
const lines = ['One', '', 'Two', 'Three'];
const orderedLines = lines.map((line, index) => (line ? `${index + 1}. ${line}` : ''));
comp.setText(lines.join('\n'));
comp.setSelection({
startLineNumber: 1,
startColumn: 1,
endLineNumber: lines.length,
endColumn: lines[lines.length - 1].length + 1
});
// Introduce list
action.executeInCurrentEditor();
expect(comp.getText()).toBe(orderedLines.join('\n'));
// Remove list
action.executeInCurrentEditor();
expect(comp.getText()).toBe(lines.join('\n'));
});| new StrikethroughAction(), | ||
| new CodeAction(), | ||
| new UrlAction(), | ||
| new UnorderedListAction(), | ||
| new BulletedListAction(), | ||
| new OrderedListAction(), |
There was a problem hiding this comment.
💡 Codebase verification
Remaining references to UnorderedListAction need to be updated
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-action.integration.spec.ts: Tests are still using UnorderedListActionsrc/main/webapp/app/shared/monaco-editor/model/actions/unordered-list.action.ts: The old action file still exists and needs to be removed
The codebase still contains references to the old UnorderedListAction in test files and the original action file. These need to be updated to use BulletedListAction for consistency.
🔗 Analysis chain
Verify the replacement of UnorderedListAction with BulletedListAction.
The changes look good, but let's verify that all existing usages of UnorderedListAction have been updated to use BulletedListAction to prevent any broken functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to UnorderedListAction
rg "UnorderedListAction" -A 5
# Search for existing usages of BulletedListAction to ensure proper integration
rg "BulletedListAction" -A 5
Length of output: 20864
| const numberedListRegex = /^\s*\d+\.\s+/; | ||
| const bulletListRegex = /^\s*[-*+•]\s+/; | ||
|
|
||
| if (numberedListRegex.test(line)) { | ||
| return line.replace(numberedListRegex, ''); | ||
| } else if (bulletListRegex.test(line)) { | ||
| return line.replace(bulletListRegex, ''); | ||
| } |
There was a problem hiding this comment.
[Critical Issue] Correct the regular expressions to match list prefixes accurately
The regular expressions used to detect bullet list prefixes may incorrectly interpret the hyphen - as a range indicator within the character class, leading to unexpected matches. To avoid this issue, the hyphen should either be escaped or placed at the beginning or end of the character class.
Apply the following diff to fix the regular expressions:
In `stripAnyListPrefix` method (lines 39-46):
- const bulletListRegex = /^\s*[-*+•]\s+/;
+ const bulletListRegex = /^\s*[*+•-]\s+/;
In `hasPrefix` method (lines 132-135):
- const bulletListRegex = /^\s*[•\-*+]\s+/;
+ const bulletListRegex = /^\s*[*+•-]\s+/;
In `handleBackspace` method (lines 179-190):
- const linePrefixMatch = lineContent.match(/^\s*(\d+\.\s+|[-*+•]\s+)/);
+ const linePrefixMatch = lineContent.match(/^\s*(\d+\.\s+|[*+•-]\s+)/);Also applies to: 132-135, 179-190
| run(editor: TextEditor) { | ||
| if (!ListAction.editorsWithListener.has(editor)) { | ||
| ListAction.editorsWithListener.set(editor, true); | ||
|
|
||
| editor.getDomNode()?.addEventListener('keydown', (event: KeyboardEvent) => { | ||
| if (event.key === 'Enter' && event.shiftKey) { | ||
| event.preventDefault(); | ||
| this.handleShiftEnter(editor); | ||
| } else if (event.key === 'Backspace') { | ||
| this.handleBackspace(editor, event); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
[Refactor Suggestion] Use editor's API to manage event listeners and prevent memory leaks
Adding event listeners directly to the editor's DOM node may lead to memory leaks if the editor is disposed and recreated, as the event listeners may persist. To prevent this and to ensure proper lifecycle management, consider using the editor's API methods such as addCommand or addAction to handle key events. This approach integrates with the editor's lifecycle and eliminates the need to track editors manually.
Feras797
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
|
thank you for your reviews :) I've updated the code with these changes:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (4)
107-112: Consider improving type safety and test isolation.The mock setup could be enhanced in two ways:
- Use a more type-safe approach for the event listener mock
- Move the WeakMap clearing to the beforeEach block for better test isolation
beforeEach(() => { + // Clear the WeakMap at the start of each test + (ListAction as any).editorsWithListener = new WeakMap<TextEditor, boolean>(); + mockEditor.getDomNode.mockReturnValue({ - addEventListener: jest.fn(), + addEventListener: jest.fn<void, [string, (event: KeyboardEvent) => void]>(), } as any); jest.clearAllMocks(); - (ListAction as any).editorsWithListener = new WeakMap<TextEditor, boolean>(); });
339-429: Add JSDoc documentation to helper functions.The helper functions are well-implemented but would benefit from documentation to improve maintainability.
+/** + * Simulates a keydown event on the editor and returns the event and spy objects. + * @param editor - The text editor instance + * @param key - The key to simulate + * @param modifiers - Optional keyboard modifiers (shift, meta) + * @returns Object containing the event and spy objects + */ const simulateKeydownEvent = (editor: TextEditor, key: string, modifiers: { shiftKey?: boolean; metaKey?: boolean } = {}) => { // ... existing implementation }; +/** + * Simulates a list action on the editor and verifies the expected text transformation. + * @param action - The text editor action to simulate + * @param selectedText - The initial selected text + * @param expectedText - The expected text after transformation + * @param startLineNumber - Optional starting line number (defaults to 1) + */ const simulateListAction = (action: TextEditorAction, selectedText: string, expectedText: string, startLineNumber: number = 1) => { // ... existing implementation };
431-543: Consider adding edge cases and improving test organization.While the test coverage is comprehensive for basic list formatting, consider:
- Adding tests for edge cases:
- Empty lines within lists
- Mixed content (lists with non-list items)
- Lists with different indentation levels
- Organizing tests using describe blocks for better readability
+describe('List Formatting', () => { + describe('Bulleted Lists', () => { it('should add bulleted list prefixes correctly', () => { // ... existing test }); it('should remove bulleted list prefixes correctly when toggled', () => { // ... existing test }); + + it('should handle empty lines within bulleted lists', () => { + const bulletedListAction = component.defaultActions.find((action: any) => action instanceof BulletedListAction) as BulletedListAction; + const selectedText = `First line\n\nThird line`; + const expectedText = `• First line\n\n• Third line`; + + simulateListAction(bulletedListAction, selectedText, expectedText); + }); }); + describe('Ordered Lists', () => { // ... existing ordered list tests }); + describe('List Type Switching', () => { // ... existing list switching tests }); });
351-367: Consider testing backspace behavior in list formatting.While the Enter key handling is well-tested, consider adding tests for backspace behavior at the start of list items, which is a common edge case in list formatting.
it('should handle backspace at start of list item correctly', () => { const bulletedListAction = component.defaultActions.find((action) => action instanceof BulletedListAction) as BulletedListAction; mockEditor.getPosition.mockReturnValue({ getLineNumber: () => 2, getColumn: () => 3, } as TextEditorPosition); mockEditor.getLineText.mockReturnValue('• Second line'); bulletedListAction.run(mockEditor); const { preventDefaultSpy } = simulateKeydownEvent(mockEditor, 'Backspace'); expect(preventDefaultSpy).toHaveBeenCalled(); expect(mockEditor.replaceTextAtRange).toHaveBeenCalledWith( expect.any(TextEditorRange), 'Second line' ); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/shared/monaco-editor/model/actions/list.action.ts(1 hunks)src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/shared/monaco-editor/model/actions/list.action.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.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}}
HawKhiem
added
deploy:artemis-test2
deploy:artemis-test1
and removed
deploy:artemis-test2
labels
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
sachmii
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
deleted the
feature/communication/add-more-input-formatting-options
branch
Checklist
General
Client
Motivation and Context
Description
Steps for Testing
Prerequisites:
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
Code Review
Manual Tests
Test Coverage
Screenshots
new formatting options on editor
strikethrough example
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests