Communication: Allow users to paste images from the clipboard
#9637
Conversation
github-actions
bot
added
tests
client
pzdr7
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
pzdr7
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
pzdr7
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
There actually weren't any tests for this service, so I added some
| export class MockClipboardItem { | ||
| types: string[]; | ||
|
|
||
| getType(_type: string): Promise<Blob> { |
There was a problem hiding this comment.
I prefix this with _ here to avoid a warning for an unused variable while still preserving the signature
pzdr7
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe changes in this pull request enhance the functionality of 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: 5
🧹 Outside diff range and nitpick comments (11)
src/test/javascript/spec/component/shared/http/file-uploader.service.spec.ts (3)
11-19: Add afterEach() to verify no outstanding HTTP requests.To ensure all HTTP requests are properly handled in each test, add an afterEach block:
afterEach(() => { httpMock.verify(); });This will help catch any unexpected or unhandled HTTP requests between tests.
21-43: Enhance test specificity for HTTP requests.While the tests verify the basic functionality, they could be more specific:
-const request = httpMock.expectOne({ method: 'POST', url: '/api/markdown-file-upload' }); +const request = httpMock.expectOne(req => { + return req.method === 'POST' + && req.url === '/api/markdown-file-upload' + && req.body instanceof FormData + && req.body.has('file'); +});This ensures not just the URL and method, but also validates the request payload structure.
45-53: Verify specific error messages in rejection tests.The tests should verify the exact error messages to ensure proper error handling:
-await expect(service.uploadMarkdownFileInCurrentMetisConversation(file, undefined, 2)).rejects.toThrow(Error); +await expect(service.uploadMarkdownFileInCurrentMetisConversation(file, undefined, 2)) + .rejects.toThrow('Course ID must be specified for communication file upload');src/main/webapp/app/shared/monaco-editor/model/actions/adapter/text-editor.interface.ts (1)
110-115: LGTM! Well-structured interface addition.The new
addPasteListenermethod is well-designed and documented:
- Clear JSDoc documentation with parameter and return value descriptions
- Follows the established pattern of returning a Disposable for cleanup
- Supports both synchronous and asynchronous callbacks via Promise
- Consistent with Monaco editor's event handling patterns
Consider adding an interface for the paste event data to support future extensions (e.g., handling different mime types, metadata) without breaking changes:
interface PasteEventData { text: string; // Future fields can be added here }Then update the method signature:
addPasteListener(callback: (data: PasteEventData) => void | Promise<void>): Disposable;src/main/webapp/app/shared/monaco-editor/model/actions/adapter/monaco-text-editor.adapter.ts (1)
197-201: Document memory leak prevention.The method returns a
Disposablewhich needs to be properly handled to prevent memory leaks.Add JSDoc comment to document proper usage:
+/** + * Adds a listener for paste events in the editor. + * @param callback Function to be called when content is pasted + * @returns A disposable that should be disposed when the listener is no longer needed + * @example + * ```typescript + * const disposable = editor.addPasteListener(async (text) => { + * // Handle pasted text + * }); + * // When done listening + * disposable.dispose(); + * ``` + */ addPasteListener(callback: (insertedText: string) => void | Promise<void>): Disposable {src/test/javascript/spec/component/markdown-editor/markdown-editor-monaco.component.spec.ts (2)
155-168: Improve test robustness and specificityThe test verifies the upload callback setup, but could be more robust:
Consider these improvements:
it('should set the upload callback on the attachment actions', () => { const attachmentAction = new AttachmentAction(); const setUploadCallbackSpy = jest.spyOn(attachmentAction, 'setUploadCallback'); const embedFilesStub = jest.spyOn(comp, 'embedFiles').mockImplementation(); comp.defaultActions = [attachmentAction]; comp.enableFileUpload = true; fixture.detectChanges(); - expect(setUploadCallbackSpy).toHaveBeenCalledOnce(); + expect(setUploadCallbackSpy).toHaveBeenCalledExactlyOnceWith(expect.any(Function)); // Check if the correct function is passed to the action. const argument = setUploadCallbackSpy.mock.calls[0][0]; - expect(argument).toBeDefined(); + expect(argument).toBeInstanceOf(Function); argument!([]); - expect(embedFilesStub).toHaveBeenCalledExactlyOnceWith([]); + expect(embedFilesStub).toHaveBeenCalledExactlyOnceWith([], undefined); });
211-218: Enhance test coverage for action click handlingWhile the test verifies basic action execution, it could be more comprehensive:
Consider these improvements:
-it('should execute the action when clicked', () => { +it.each([ + [new UrlAction(), {}], + [new AttachmentAction(), { files: [] }], + [new ColorAction(), { color: 'red' }] +])('should execute %p when clicked', (action, expectedArgs) => { - const action = new UrlAction(); const executeInCurrentEditorStub = jest.spyOn(action, 'executeInCurrentEditor').mockImplementation(); comp.defaultActions = [action]; fixture.detectChanges(); - comp.handleActionClick(new MouseEvent('click'), action); - expect(executeInCurrentEditorStub).toHaveBeenCalledOnce(); + const mockEvent = new MouseEvent('click', { + bubbles: true, + cancelable: true, + button: 0 + }); + comp.handleActionClick(mockEvent, action); + expect(executeInCurrentEditorStub).toHaveBeenCalledExactlyOnceWith(expectedArgs); });src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-action.integration.spec.ts (1)
88-113: Consider improving test clarity with constants.The test effectively verifies clipboard file processing, but could be improved by:
- Using a meaningful constant instead of empty string for the paste event
- Making the default filename more explicit with a named constant
+ const PASTE_EVENT_DATA = 'paste_event_data'; + const DEFAULT_IMAGE_FILENAME = 'image.png'; - await pasteListener(''); + await pasteListener(PASTE_EVENT_DATA); - expect(uploadCallback).toHaveBeenCalledExactlyOnceWith([new File([imageBlob], 'image.png', { type: 'image/png' })]); + expect(uploadCallback).toHaveBeenCalledExactlyOnceWith([new File([imageBlob], DEFAULT_IMAGE_FILENAME, { type: 'image/png' })]);src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts (2)
493-495: Consider adding user feedback when file upload is disabled.While the early return is a good practice, users might be confused when their paste action doesn't work. Consider adding a notification to inform users when file upload is disabled.
embedFiles(files: File[], inputElement?: HTMLInputElement): void { if (!this.enableFileUpload) { + this.alertService.addAlert({ + type: AlertType.WARNING, + message: 'File upload is currently disabled.', + }); return; }
Line range hint
493-524: Consider implementing upload cancellation for memory leak prevention.The file upload implementation could benefit from proper cleanup of ongoing upload requests when the component is destroyed or when new uploads are initiated. This would prevent potential memory leaks and improve resource management.
Consider these improvements:
- Track ongoing uploads using a Subject/Subscription pattern
- Cancel previous uploads when new ones start
- Clean up pending uploads in ngOnDestroy
Example implementation:
private readonly destroy$ = new Subject<void>(); private currentUploads$ = new Subject<void>(); ngOnDestroy(): void { this.destroy$.next(); this.destroy$.complete(); this.currentUploads$.next(); this.currentUploads$.complete(); } embedFiles(files: File[], inputElement?: HTMLInputElement): void { // Cancel previous uploads this.currentUploads$.next(); // ... existing checks ... files.forEach((file) => { // Wrap the upload in a cancelable observable from(this.uploadFile(file)).pipe( takeUntil(this.currentUploads$), takeUntil(this.destroy$) ).subscribe({ next: (response) => this.processFileUploadResponse(response, file), error: (error) => { this.alertService.addAlert({ type: AlertType.DANGER, message: error.message, disableTranslation: true, }); }, complete: () => this.resetInputElement(inputElement) }); }); }src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts (1)
55-56: Check for non-empty file list before invokinguploadCallbackCalling
uploadCallbackwith an empty array may not be necessary and could lead to unintended behavior. Consider checking if any files were retrieved before invoking the callback.Apply this diff to add the conditional check:
- this.uploadCallback(files); + if (files.length > 0) { + this.uploadCallback(files); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts(2 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/adapter/monaco-text-editor.adapter.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/adapter/text-editor.interface.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts(2 hunks)src/test/javascript/spec/component/markdown-editor/markdown-editor-monaco.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/http/file-uploader.service.spec.ts(1 hunks)src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts(1 hunks)src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-action.integration.spec.ts(2 hunks)src/test/javascript/spec/helpers/mocks/service/mock-clipboard-item.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/adapter/monaco-text-editor.adapter.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/adapter/text-editor.interface.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts (1)
src/test/javascript/spec/component/markdown-editor/markdown-editor-monaco.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/http/file-uploader.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}}
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}}
src/test/javascript/spec/helpers/mocks/service/mock-clipboard-item.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}}
📓 Learnings (1)
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-action.integration.spec.ts (2)
Learnt from: pzdr7
PR: ls1intum/Artemis#9407
File: src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.ts:46-46
Timestamp: 2024-10-08T15:35:48.767Z
Learning: In integration tests, it's acceptable to import the actual `MonacoEditorComponent` instead of mocking it.
Learnt from: pzdr7
PR: ls1intum/Artemis#9407
File: src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.ts:46-46
Timestamp: 2024-10-02T06:50:08.640Z
Learning: In integration tests, it's acceptable to import the actual `MonacoEditorComponent` instead of mocking it.
🔇 Additional comments (12)
src/test/javascript/spec/component/shared/http/file-uploader.service.spec.ts (1)
7-73: Well-structured test suite with comprehensive coverage.
The test suite effectively covers the main functionality and error cases of the FileUploaderService. It follows good testing practices including:
- Proper use of TestBed and HttpTestingController
- Comprehensive error case coverage
- Clean and consistent test structure
src/main/webapp/app/shared/monaco-editor/model/actions/adapter/monaco-text-editor.adapter.ts (1)
197-201: Verify integration with clipboard image handling.
The current implementation only handles text content. Let's verify how this integrates with the clipboard image handling functionality mentioned in the PR objectives.
✅ Verification successful
Let me gather more information about the clipboard handling implementation and the test file found.
Based on the search results, I can now provide a final response as we have found the complete implementation details showing how clipboard image handling is integrated.
The clipboard image handling is properly implemented through the AttachmentAction class
The current implementation in monaco-text-editor.adapter.ts correctly provides text content to the paste listener, which is then used by AttachmentAction to handle both text and image paste scenarios:
- When text is pasted, it's handled directly through the adapter
- When the pasted content is empty,
AttachmentActionchecks the clipboard for image content usingnavigator.clipboard.read() - Images are detected by checking MIME types starting with 'image/' and are processed accordingly
The integration is verified through test coverage in monaco-editor-action.integration.spec.ts which confirms both clipboard text and image handling scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how clipboard image handling is implemented
# Expected: Find code that uses this paste listener for handling clipboard images
# Look for clipboard image handling code
echo "Searching for clipboard image handling code..."
rg -l "clipboard.*image|image.*clipboard" --type ts
# Look for usage of addPasteListener
echo "Searching for usage of addPasteListener..."
ast-grep --pattern 'addPasteListener($$$)'
Length of output: 517
Script:
#!/bin/bash
# Check the test file content for clipboard image handling
echo "Checking test file content..."
rg -A 10 "clipboard.*image|image.*clipboard" src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-action.integration.spec.ts
# Look for paste-related code in the editor implementation
echo "Checking editor implementation for paste handling..."
rg -A 10 "onPaste|paste" src/main/webapp/app/shared/monaco-editor/
# Look for image-related code in the editor
echo "Checking for image handling in editor..."
rg -A 10 "image.*upload|upload.*image|handleImage" src/main/webapp/app/shared/monaco-editor/
Length of output: 6591
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (2)
73-73: LGTM: Mock implementation follows established patterns
The mock implementation of addPasteListener is correctly added to the mockEditor object using Jest's function mocking.
73-73:
Add test coverage for clipboard paste functionality
Given that this PR adds clipboard image paste functionality, we need additional test cases to ensure the feature works correctly. Please add tests for:
- Successful image paste events
- Regular text paste events
- Error handling during paste
- Integration with file upload functionality
Example test structure:
it('should handle image paste events correctly', () => {
const pasteCallback = jest.fn();
mockEditor.addPasteListener.mock.calls[0][0]({
clipboardData: {
items: [{ type: 'image/png', getAsFile: () => new File([], 'test.png') }]
}
});
expect(/* verify expected behavior */);
});
it('should handle non-image paste events correctly', () => {
const pasteCallback = jest.fn();
mockEditor.addPasteListener.mock.calls[0][0]({
clipboardData: {
items: [{ type: 'text/plain' }]
}
});
expect(/* verify expected behavior */);
});
it('should handle paste errors gracefully', () => {
const pasteCallback = jest.fn();
mockEditor.addPasteListener.mock.calls[0][0]({
clipboardData: {
items: [{ type: 'image/png', getAsFile: () => { throw new Error('Test error'); } }]
}
});
expect(/* verify error handling */);
});Let's verify the current test coverage:
src/test/javascript/spec/component/markdown-editor/markdown-editor-monaco.component.spec.ts (1)
197-209: LGTM! Well-structured negative test case
The test effectively verifies that file embedding is blocked when uploads are disabled. Good use of spy assertions and clear setup.
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-action.integration.spec.ts (2)
24-47: Well-structured test setup with proper mocking!
Good practices observed:
- Proper use of async/await in beforeEach
- Clean clipboard mock setup with proper isolation
- Appropriate test cleanup in afterEach
75-86: LGTM! Well-structured negative test case.
The test effectively verifies that the clipboard is not accessed when no upload callback is specified, using appropriate spies and assertions.
src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts (1)
332-333: LGTM: Proper initialization of file upload callback.
The code correctly sets up the file upload callback only when file uploads are enabled, maintaining proper context with bind(this).
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts (4)
5-6: New imports are necessary and appropriate
The added imports for Disposable and TranslateService are required for the new functionality and are correctly included.
20-22: Private properties added with correct naming conventions
The new private properties disposablePasteListener and uploadCallback follow camelCase naming conventions and adhere to the project's coding guidelines.
27-33: setUploadCallback method is well-implemented
The setUploadCallback method allows setting an optional callback for handling pasted files. The method is properly documented and follows best practices.
59-63: Proper resource cleanup in dispose method
The dispose method correctly disposes of disposablePasteListener and resets uploadCallback to undefined, adhering to memory leak prevention guidelines.
src/test/javascript/spec/component/shared/http/file-uploader.service.spec.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/adapter/monaco-text-editor.adapter.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts
Show resolved
Hide resolved
Checklist
General
Client
Motivation and Context
It's currently not possible to (like on GitHub) paste images into the Markdown editor using the keyboard shortcut CTRL+V/Cmd+V.
Description
Added a paste listener that checks for images (and only images) in the clipboard and embeds them in the editor. Also, added some basic tests to improve the coverage.
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
Screenshots
Screencast.from.29.10.2024.14_41_19.webm
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests