Communication: Fix an issue with list formatting in Markdown
#9925
Conversation
github-actions
bot
added
tests
client
WalkthroughThe pull request introduces several modifications across multiple components related to content processing in a markdown editor. Key changes include the addition of a method to escape unordered list items, updates to existing methods for handling numbered lists, and enhancements to the markdown editor's event handling for user input. Additionally, the representation of bullet points is altered, and new public methods are introduced in the Monaco editor component to improve interaction capabilities. The changes are accompanied by new test cases to ensure proper functionality across the updated features. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@asliayk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 5
🧹 Outside diff range and nitpick comments (19)
src/main/webapp/app/shared/monaco-editor/model/actions/ordered-list.action.ts (2)
15-16: Optimize and align with style guidelinesThe implementation can be improved for better maintainability and style compliance:
- const space = ' '; - return `${lineNumber}.${space}`; + private static readonly SPACE = ' '; + return lineNumber + '.' + OrderedListAction.SPACE;Changes:
- Move space constant to class level as it's static
- Use 4-space indentation per guidelines
- Use single quotes instead of template literals per guidelines
- Use string concatenation for better readability
Line range hint
19-19: Initialize the required PREFIX propertyThe
PREFIXproperty is declared but not initialized. This could lead to undefined behavior if the base classListActionattempts to use this property.- protected readonly PREFIX: string; + protected readonly PREFIX = '1. ';src/main/webapp/app/shared/monaco-editor/model/actions/bulleted-list.action.ts (1)
Line range hint
7-9: Consider enhancing the class documentation.While the current documentation describes the basic purpose, it could be more detailed to reflect the automatic list continuation behavior.
/** * Action used to add or modify a bullet-point list in the text editor. + * The list items use the markdown-style hyphen prefix ('- ') and support + * automatic continuation when pressing Enter at the end of a list item. */src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.component.spec.ts (2)
293-300: Enhance assertion specificity in the model content changes testThe test correctly verifies the listener registration, but could be more specific in its assertion.
Consider updating the assertion to verify the exact parameters passed to the listener:
- expect(listenerStub).toHaveBeenCalled(); + expect(listenerStub).toHaveBeenCalledExactlyOnceWith(expect.any(Object));
302-308: Consider adding edge case coverage for model retrievalWhile the test covers the basic functionality, consider adding edge cases.
Add test cases for:
- Model retrieval before setting text
- Model retrieval after model disposal
- Model retrieval with empty text
Example:
it('should handle edge cases in model retrieval', () => { fixture.detectChanges(); // Before setting text expect(comp.getModel()).not.toBeNull(); expect(comp.getModel()?.getValue()).toBe(''); // After disposal const model = comp.getModel(); model?.dispose(); expect(comp.getModel()).toBeNull(); });src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (2)
125-127: Add return type and documentationThe method implementation is correct but could be improved with explicit typing and documentation.
- public getModel() { + /** + * Returns the current text model of the editor. + * @returns The editor's text model or null if no model is set. + */ + public getModel(): monaco.editor.ITextModel | null { return this._editor.getModel(); }
129-132: Add parameter validation and documentationThe method handles null models well but could be improved with input validation and documentation.
- public getLineContent(lineNumber: number): string { + /** + * Returns the content of a specific line in the editor. + * @param lineNumber The 1-based line number to get content from + * @returns The content of the specified line or empty string if line doesn't exist + * @throws {Error} If lineNumber is less than 1 + */ + public getLineContent(lineNumber: number): string { + if (lineNumber < 1) { + throw new Error('Line number must be greater than 0'); + } const model = this._editor.getModel(); return model ? model.getLineContent(lineNumber) : ''; }src/main/webapp/app/shared/monaco-editor/model/actions/list.action.ts (3)
96-100: Consider trimming the line before prefix checkWhile the prefix handling logic is correct, consider trimming whitespace before checking the prefix to handle cases where the line might have leading spaces.
-if (currentLineText.startsWith(newPrefix)) { +if (currentLineText.trim().startsWith(newPrefix.trim())) {
Line range hint
116-130: Refactor prefix type determinationThe current implementation uses magic strings and direct comparisons which could be fragile. Consider introducing constants and a more robust type checking mechanism.
+private static readonly BULLET_PREFIX = '- '; +private static readonly NUMBERED_PREFIX_REGEX = /^\s*\d+\.\s+/; -if (this.getPrefix(1) != '- ') { +if (!this.getPrefix(1).startsWith(ListAction.BULLET_PREFIX)) { - const numberedListRegex = /^\s*\d+\.\s+/; - allLinesHaveCurrentPrefix = lines.every((line) => numberedListRegex.test(line)); + allLinesHaveCurrentPrefix = lines.every((line) => + ListAction.NUMBERED_PREFIX_REGEX.test(line));
Line range hint
52-90: Consider adding event cleanup mechanismThe current implementation adds event listeners but doesn't provide a mechanism to remove them when the editor is destroyed. This could potentially lead to memory leaks.
Consider adding a cleanup method:
protected cleanup(editor: TextEditor): void { const domNode = editor.getDomNode(); if (domNode) { domNode.removeEventListener('keydown', this.handleKeyDown); } ListAction.editorsWithListener.delete(editor); }src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (2)
40-40: Consider consolidating mock setup.The mock setup for the Monaco editor is spread across multiple locations. Consider consolidating all Monaco-related mock setup into a single location, preferably in the
beforeEachblock, to improve maintainability.beforeEach(() => { // ... existing setup ... mockMarkdownEditorComponent = fixture.debugElement.query(By.directive(MarkdownEditorMonacoComponent)).componentInstance; - mockMarkdownEditorComponent.monacoEditor.getPosition = jest.fn(); - mockMarkdownEditorComponent.monacoEditor.getModel = jest.fn(); + // Consolidate Monaco editor mock setup + mockMarkdownEditorComponent.monacoEditor = { + ...mockMarkdownEditorComponent.monacoEditor, + getPosition: jest.fn(), + getModel: jest.fn(), + };Also applies to: 128-128, 145-146
444-444: Extract repeated test data into constants.There are multiple occurrences of the same test data strings across different test cases. Consider extracting these into constants to improve maintainability and reduce duplication.
+const TEST_DATA = { + PLAIN_TEXT: 'First line\nSecond line\nThird line', + BULLETED_LIST: '- First line\n- Second line\n- Third line', + ORDERED_LIST: '1. First line\n2. Second line\n3. Third line', +}; it('should add bulleted list prefixes correctly', () => { const bulletedListAction = component.defaultActions.find((action: any) => action instanceof BulletedListAction) as BulletedListAction; - const selectedText = `First line\nSecond line\nThird line`; - const expectedText = `- First line\n- Second line\n- Third line`; + const selectedText = TEST_DATA.PLAIN_TEXT; + const expectedText = TEST_DATA.BULLETED_LIST;Also applies to: 451-451, 460-460, 476-477, 489-489, 500-500, 509-510, 524-524, 530-530, 544-544, 547-547
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (2)
92-93: Consider supporting additional unordered list markersThe current implementation only handles hyphen-prefixed lists (
-). Consider extending support for other common markdown list markers like asterisk (*) and plus (+).escapeUnorderedList(content: string): string { - return content.replace(/^(- )/gm, '\\$1'); + return content.replace(/^([*+-] )/gm, '\\$1'); }
79-93: Consider extracting markdown processing logicThe component currently handles both UI responsibilities and markdown processing logic. Consider extracting the list processing methods into a separate markdown service for better separation of concerns and reusability.
This would:
- Make the markdown processing logic more reusable across components
- Simplify testing of the processing logic
- Keep the component focused on its primary UI responsibilities
src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts (4)
275-291: Consider improving test readability and maintainability.While the test is comprehensive, consider these improvements:
- Make the test name more descriptive, e.g., "should correctly escape both numbered and unordered lists in content before and after references"
- Extract test data into variables with meaningful names for better readability
- it('should process content before and after reference with escaped numbered and unordered lists', () => { + it('should correctly escape both numbered and unordered lists in content before and after references', () => { + const numberedListContent = '1. This is a numbered list\n2. Another item'; + const unorderedListContent = '- This is an unordered list'; + const contentBefore = `${numberedListContent}\n${unorderedListContent}`; - const contentBefore = '1. This is a numbered list\n2. Another item\n- This is an unordered list'; - const contentAfter = '1. Numbered again\n- Unordered again'; + const contentAfter = '1. Numbered again\n- Unordered again';
293-297: Consider adding edge cases for numbered list escaping.The test covers the basic case well, but consider adding test cases for:
- Single-digit vs multi-digit numbers (e.g., "10. ", "100. ")
- Different spacing after numbers
- Invalid number formats
it('should escape numbered lists correctly', () => { - const content = '1. First item\n2. Second item\n3. Third item'; - const escapedContent = component.escapeNumberedList(content); - expect(escapedContent).toBe('1\\. First item\n2\\. Second item\n3\\. Third item'); + const testCases = [ + { + input: '1. First item\n2. Second item\n3. Third item', + expected: '1\\. First item\n2\\. Second item\n3\\. Third item' + }, + { + input: '10. Tenth item\n100. Hundredth item', + expected: '10\\. Tenth item\n100\\. Hundredth item' + }, + { + input: '1.First item\n2. Second item', + expected: '1\\. First item\n2\\. Second item' + } + ]; + + testCases.forEach(({ input, expected }) => { + expect(component.escapeNumberedList(input)).toBe(expected); + }); });
299-303: Consider testing different unordered list markers.The test covers the basic case with "-" markers, but consider adding test cases for:
- Different list markers (* and +)
- Mixed markers in the same content
- Different spacing after markers
it('should escape unordered lists correctly', () => { - const content = '- First item\n- Second item\n- Third item'; - const escapedContent = component.escapeUnorderedList(content); - expect(escapedContent).toBe('\\- First item\n\\- Second item\n\\- Third item'); + const testCases = [ + { + input: '- First item\n- Second item', + expected: '\\- First item\n\\- Second item' + }, + { + input: '* First item\n+ Second item', + expected: '\\* First item\n\\+ Second item' + }, + { + input: '-First item\n- Second item', + expected: '\\-First item\n\\- Second item' + } + ]; + + testCases.forEach(({ input, expected }) => { + expect(component.escapeUnorderedList(input)).toBe(expected); + }); });
313-317: Consider verifying order independence of escaping operations.The test verifies one order of operations (unordered then numbered), but it would be valuable to verify that the order of escaping operations doesn't affect the result.
it('should handle mixed numbered and unordered lists in content', () => { const content = '1. Numbered item\n- Unordered item\n2. Another numbered item\n- Another unordered item'; - const escapedContent = component.escapeNumberedList(component.escapeUnorderedList(content)); - expect(escapedContent).toBe('1\\. Numbered item\n\\- Unordered item\n2\\. Another numbered item\n\\- Another unordered item'); + const expected = '1\\. Numbered item\n\\- Unordered item\n2\\. Another numbered item\n\\- Another unordered item'; + + // Verify order doesn't matter + const unorderedThenNumbered = component.escapeNumberedList(component.escapeUnorderedList(content)); + const numberedThenUnordered = component.escapeUnorderedList(component.escapeNumberedList(content)); + + expect(unorderedThenNumbered).toBe(expected); + expect(numberedThenUnordered).toBe(expected); });src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (1)
153-158: Handle potential null values when finding actionsIn the
handleKeyDownmethod, the code uses the non-null assertion operator!afterfind, assuming that the action will always be found inthis.defaultActions. If the action is not found, this could lead to a runtime error. Consider adding a null check to ensure the code is robust.Apply this change:
- this.markdownEditor.handleActionClick(new MouseEvent('click'), this.defaultActions.find((action) => action instanceof BulletedListAction)!); + const bulletedListAction = this.defaultActions.find((action) => action instanceof BulletedListAction); + if (bulletedListAction) { + this.markdownEditor.handleActionClick(new MouseEvent('click'), bulletedListAction); + }Similarly, update for the
OrderedListAction:- this.markdownEditor.handleActionClick(new MouseEvent('click'), this.defaultActions.find((action) => action instanceof OrderedListAction)!); + const orderedListAction = this.defaultActions.find((action) => action instanceof OrderedListAction); + if (orderedListAction) { + this.markdownEditor.handleActionClick(new MouseEvent('click'), orderedListAction); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts(1 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(7 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/ordered-list.action.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/monaco-editor.component.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/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts(13 hunks)src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
src/main/webapp/app/shared/monaco-editor/model/actions/ordered-list.action.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/bulleted-list.action.ts (1)
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/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (1)
src/test/javascript/spec/component/shared/monaco-editor/monaco-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/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (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}}
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/list.action.ts (1)
🔇 Additional comments (11)
src/main/webapp/app/shared/monaco-editor/model/actions/bulleted-list.action.ts (2)
4-4: LGTM! The change to hyphen-based bullet points aligns with standard markdown syntax.
The switch from bullet character ('• ') to hyphen ('- ') makes the editor more compatible with standard markdown and matches common editor behaviors (like Slack). This change effectively supports the PR's goal of automatic list continuation.
Line range hint 4-24: Verify integration with related components.
The change to hyphen-based bullets needs to work seamlessly with the new escapeUnorderedList method in PostingContentPartComponent and the list prefix handling in PostingMarkdownEditorComponent.
✅ Verification successful
Based on the shell script results, I can now generate the final response as the verification is complete. The results show consistent integration across components:
Integration verified - hyphen-based bullets are consistently implemented
The codebase shows proper integration with:
PostingContentPartComponentcorrectly escapes unordered lists with hyphens viaescapeUnorderedListmethodPostingMarkdownEditorComponentproperly handles hyphen prefixes in its event handling- The
BULLET_PREFIXconstant with value'- 'is consistently used across list-related actions - Test coverage confirms the proper escaping and handling of hyphen-based bullets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistent usage of the new hyphen prefix across related components
# Check for list prefix handling in the editor components
echo "Checking for list prefix handling implementations..."
rg -A 5 "escapeUnorderedList|handleListPrefix" --type ts
# Verify the usage of hyphen prefix across the codebase
echo "Checking for bullet point prefix usage..."
rg -A 2 "BULLET_PREFIX|'- '|\"- \"" --type ts
Length of output: 9927
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (2)
121-123: LGTM! Clean implementation of content change listener
The method properly exposes Monaco's content change event while maintaining proper cleanup through IDisposable return type.
121-132: Consider adding unit tests for new methods
While the implementation looks solid, it would be beneficial to add unit tests for these new methods to ensure they handle edge cases correctly (e.g., invalid line numbers, null models, etc.).
src/main/webapp/app/shared/monaco-editor/model/actions/list.action.ts (2)
40-40: LGTM: Consistent regex patterns for standard markdown list prefixes
The simplified regex patterns correctly match standard markdown list prefixes (-, *, +) and maintain consistency across methods.
Also applies to: 147-147
193-193: LGTM: Consistent prefix detection in backspace handling
The regex pattern correctly matches both numbered and bullet list prefixes, maintaining consistency with other patterns in the file.
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (1)
Line range hint 1-570: Overall test implementation looks good!
The test suite provides comprehensive coverage of the list functionality and key event handling. The implementation follows testing best practices and includes proper mocking and setup.
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (2)
79-79: LGTM: Consistent processing of list formatting
The changes correctly apply unordered list escaping after numbered list processing for both content segments, maintaining consistency.
Also applies to: 84-84
89-90: Verify the impact of double spacing after numbered list markers
The change adds an extra space after escaped numbered list markers. Please verify this doesn't affect the rendered output or user experience negatively.
✅ Verification successful
The previous search was too specific. Let's broaden it to find relevant test files and usages of the method to understand its testing coverage and impact.
Double spacing after numbered list markers is expected and verified
The test cases in posting-content-part.component.spec.ts explicitly verify that the double spacing after escaped numbered list markers is the expected behavior:
- Test case
should escape numbered lists correctlyverifies the exact output format with two spaces:'1\\. First item' - Test case
should handle mixed numbered and unordered listsconfirms the same spacing pattern - The behavior is consistently tested across multiple scenarios including mixed content
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests that verify list rendering
rg -l "escapeNumberedList.*test" --type ts
Length of output: 44
Script:
#!/bin/bash
# Search for test files related to posting content
fd "posting.*test" --type f --extension ts
# Search for any usage of escapeNumberedList
rg "escapeNumberedList" --type ts -A 2 -B 2
# Search for any test cases involving numbered lists
rg "numbered.*list" --type ts -i
Length of output: 8733
src/test/javascript/spec/component/shared/metis/posting-content/posting-content-part.component.spec.ts (2)
274-274: LGTM! Well-organized test suite structure.
The test suite follows Jest best practices with clear organization and descriptive naming.
305-311: LGTM! Good edge case coverage.
The test effectively verifies that non-list content remains unchanged when passed through both escaping methods.
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.component.spec.ts
Show resolved
Hide resolved
...c/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts(12 hunks)src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-action.integration.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
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 (1)
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-action.integration.spec.ts (1)
141-141: LGTM! Standardizing list prefix spacing.
The change from two spaces to one space after the ordered list prefix improves consistency with common markdown editors.
Let's verify that this spacing convention is consistently applied across the codebase:
✅ Verification successful
The search results show that the list prefix spacing is consistent across the codebase. The matches found in artemis-date.pipe.spec.ts are actually for date formatting in German locale (where dates are formatted like "14. Apr. 2020"), not for list prefixes. The only actual list-related assertions are in monaco-editor-action.integration.spec.ts, and they both use single space after the prefix ('- ' and '1. ').
List prefix spacing is consistently implemented
The change from two spaces to one space aligns with the existing implementation, as verified by the codebase search. All list-related assertions use single space after both ordered ('1. ') and unordered ('- ') list prefixes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent list prefix spacing across the codebase
# Expected: All list prefixes should be followed by a single space
# Search for ordered list patterns with multiple spaces
rg "^\d+\.\s{2,}" --type ts
# Search for unordered list patterns with multiple spaces
rg "^[-*+]\s{2,}" --type ts
# Search for list-related test assertions to ensure consistent spacing expectations
rg "expect\(.+\).toBe\(['\"](\d+\.|[-*+])\s+" --type ts
Length of output: 899
...test/javascript/spec/component/shared/monaco-editor/monaco-editor-action.integration.spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.component.spec.ts (3)
293-300: Enhance listener test assertionsWhile the test verifies that the listener is called, it should also verify the content of the change event to ensure the listener receives the correct data.
it('should register a listener for model content changes', () => { const listenerStub = jest.fn(); fixture.detectChanges(); const disposable = comp.onDidChangeModelContent(listenerStub); comp.setText(singleLineText); - expect(listenerStub).toHaveBeenCalled(); + expect(listenerStub).toHaveBeenCalledOnce(); + const event = listenerStub.mock.calls[0][0]; + expect(event.changes).toBeDefined(); + expect(event.changes.length).toBeGreaterThan(0); disposable.dispose(); });
302-308: Add test case for empty modelThe test should also verify the behavior when no text has been set in the editor.
it('should retrieve the editor model', () => { fixture.detectChanges(); + // Initial empty state + const emptyModel = comp.getModel(); + expect(emptyModel).not.toBeNull(); + expect(emptyModel?.getValue()).toBe(''); + + // After setting text comp.setText(singleLineText); const model = comp.getModel(); expect(model).not.toBeNull(); expect(model?.getValue()).toBe(singleLineText); });
310-315: Add boundary test cases for line content retrievalThe test should also verify the behavior for first and last lines of the text to ensure proper handling of boundary cases.
it('should get the content of a specific line', () => { fixture.detectChanges(); comp.setText(multiLineText); + // First line + expect(comp.getLineContent(1)).toBe('public class Main {'); + // Middle line const lineContent = comp.getLineContent(2); expect(lineContent).toBe('static void main() {'); + // Last line + expect(comp.getLineContent(5)).toBe('}'); });src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (1)
126-160: Consider extracting list handling logic into a separate serviceWhile the implementation achieves the auto-list prefixing objective, the current approach mixes concerns within the component. Consider:
- Creating a dedicated
ListHandlingServiceto encapsulate list-related logic- Using RxJS for handling editor events
- Implementing proper cleanup using takeUntil pattern
This would improve:
- Testability: List handling logic can be tested in isolation
- Maintainability: Easier to add new list types or modify existing behavior
- Reusability: List handling could be used in other editor components
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts(12 hunks)src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.component.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (1)
src/test/javascript/spec/component/shared/monaco-editor/monaco-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}}
🔇 Additional comments (3)
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.component.spec.ts (1)
317-329: LGTM! Comprehensive error case coverage
The test thoroughly covers invalid line numbers and empty line scenarios as suggested in the previous review. Good job implementing the feedback!
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (2)
18-18: LGTM: Monaco editor import added correctly
The import follows Angular style guidelines and is necessary for the new functionality.
129-149:
Memory leak prevention needed
The event listener needs to be properly disposed when the component is destroyed.
Apply this change:
+ private modelContentChangeListener?: monaco.IDisposable;
ngAfterViewInit(): void {
this.markdownEditor.enableTextFieldMode();
const editor = this.markdownEditor.monacoEditor;
if (editor) {
- editor.onDidChangeModelContent((event: monaco.editor.IModelContentChangedEvent) => {
+ this.modelContentChangeListener = editor.onDidChangeModelContent((event: monaco.editor.IModelContentChangedEvent) => {
// ... existing code ...
});
}
}
+ ngOnDestroy(): void {
+ this.modelContentChangeListener?.dispose();
+ }
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts
Show resolved
Hide resolved
asliayk
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
asliayk
added
bugfix
component:Communication
ready for review
and removed
lock:artemis-test2
labels
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
|
github-actions
bot
added
the
deployment-error
Haoyuli2002
added
deploy:artemis-test3
and removed
deployment-error
|
github-actions
bot
added
the
deployment-error
zagemello
added
deploy:artemis-test2
and removed
deployment-error
zagemello
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
Communication: Add auto-list prefixing without selecting list optionsCommunication: Fix an issue with list formatting in Markdown
Checklist
General
Client
Motivation and Context
(Closes
Communication:Invalid Markdown for Lists in Artemis #9881)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
Client
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests