Communication: Fix improper deletion of combined emojis in Monaco editor
#10242
Conversation
asliayk
added
tests
client
asliayk
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThis pull request introduces a new dependency, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant EmojiAction
User->>Editor: Open emoji picker
Editor->>EmojiAction: Request emoji insertion at cursor
Note right of EmojiAction: Calculate new cursor using emoji length
EmojiAction-->>Editor: Insert emoji and update cursor
sequenceDiagram
participant User
participant Editor
participant MonacoComponent
participant GraphemeSplitter
User->>Editor: Press Backspace key
Editor->>MonacoComponent: Invoke custom backspace command
MonacoComponent->>GraphemeSplitter: Split current line into grapheme clusters
GraphemeSplitter-->>MonacoComponent: Return grapheme clusters
MonacoComponent-->>Editor: Delete appropriate grapheme and update cursor
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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: 0
🧹 Nitpick comments (3)
src/main/webapp/app/shared/monaco-editor/model/actions/emoji.action.ts (1)
97-97: LGTM! Improved cursor positioning for emojis.The change correctly uses the emoji's actual length instead of a fixed value, which is more accurate for combined emojis like flags (e.g., 🇩🇪).
Consider adding a comment explaining why we use
emoji.lengthfor cursor positioning, as it might not be immediately obvious that some emojis can be longer than others:+ // Use emoji.length to handle combined emojis (e.g., flags) which can be longer than 2 characters const newPosition = new TextEditorPosition(position.getLineNumber(), position.getColumn() + emoji.length);src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.component.spec.ts (1)
349-376: Enhance test readability with descriptive assertions.The test effectively verifies sequential deletion of combined emojis, but could be more readable with descriptive assertion messages.
Consider adding descriptive messages to assertions:
- expect(comp.getText()).toEqual(emoji1); + expect(comp.getText()).toEqual(emoji1, 'Should retain first emoji after deleting second emoji'); - expect(comp.getText()).toEqual(''); + expect(comp.getText()).toEqual('', 'Should have empty text after deleting all emojis');src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (1)
160-196: Consider breaking down the complex backspace command logic.While the implementation is correct, the code could be more maintainable if split into smaller, focused functions.
Consider refactoring into smaller functions:
+ private handleSelectionDelete(): void { + this._editor.trigger('keyboard', 'deleteLeft', null); + } + + private getTextBeforeCursor(model: monaco.editor.ITextModel, lineNumber: number, column: number): string { + const lineContent = model.getLineContent(lineNumber); + return lineContent.substring(0, column - 1); + } + + private deleteLastGrapheme(textBeforeCursor: string, textAfterCursor: string, lineNumber: number, lineContent: string): void { + const splitter = new GraphemeSplitter(); + const graphemes = splitter.splitGraphemes(textBeforeCursor); + if (graphemes.length === 0) return; + + graphemes.pop(); + const newTextBeforeCursor = graphemes.join(''); + const newLineContent = newTextBeforeCursor + textAfterCursor; + + this.updateEditorContent(lineNumber, lineContent, newLineContent); + } + + private updateEditorContent(lineNumber: number, oldContent: string, newContent: string): void { + this._editor.getModel()?.pushEditOperations( + [], + [{ + range: new monaco.Range(lineNumber, 1, lineNumber, oldContent.length + 1), + text: newContent, + }], + () => null + ); + } this.customBackspaceCommandId = this._editor.addCommand(monaco.KeyCode.Backspace, () => { const model = this._editor.getModel(); const selection = this._editor.getSelection(); if (!model || !selection) return; if (!selection.isEmpty()) { - this._editor.trigger('keyboard', 'deleteLeft', null); + this.handleSelectionDelete(); return; } const lineNumber = selection.startLineNumber; const column = selection.startColumn; - const lineContent = model.getLineContent(lineNumber); - const textBeforeCursor = lineContent.substring(0, column - 1); - const splitter = new GraphemeSplitter(); - const graphemes = splitter.splitGraphemes(textBeforeCursor); - - if (graphemes.length === 0) return; - - graphemes.pop(); - const newTextBeforeCursor = graphemes.join(''); const textAfterCursor = lineContent.substring(column - 1); + const textBeforeCursor = this.getTextBeforeCursor(model, lineNumber, column); + const lineContent = model.getLineContent(lineNumber); - const newLineContent = newTextBeforeCursor + textAfterCursor; - model.pushEditOperations( - [], - [{ - range: new monaco.Range(lineNumber, 1, lineNumber, lineContent.length + 1), - text: newLineContent, - }], - () => null - ); + this.deleteLastGrapheme(textBeforeCursor, textAfterCursor, lineNumber, lineContent); }) || undefined;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/emoji.action.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts(5 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.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/shared/monaco-editor/model/actions/emoji.action.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/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}}
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (8)
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.component.spec.ts (3)
331-347: LGTM! Well-structured test for combined emoji deletion.The test effectively verifies that backspace correctly deletes an entire combined emoji (🇩🇪) without leaving partial characters.
378-393: LGTM! Comprehensive test for emoji deletion in mixed text.The test effectively verifies that only the emoji is deleted while preserving surrounding text, which is a common real-world scenario.
395-408: LGTM! Precise cursor position verification after emoji deletion.The test effectively verifies that the cursor is placed at the correct position (column 7) after deleting an emoji.
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (2)
13-13: LGTM! Added GraphemeSplitter for proper emoji handling.The GraphemeSplitter library is essential for correctly handling grapheme clusters, especially for combined emojis.
488-490: LGTM! Clean getter implementation for backspace command ID.The getter properly encapsulates access to the custom backspace command ID with correct typing.
src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (2)
609-619: LGTM! Thorough test for emoji insertion.The test effectively verifies emoji insertion, cursor positioning, and editor focus.
621-642: LGTM! Comprehensive test for emoji picker workflow.The test effectively verifies the complete emoji selection flow, from picker interaction to insertion and cleanup.
package.json (1)
56-56: Grapheme-splitter Dependency AddedThe new dependency
"grapheme-splitter": "^1.0.4"has been introduced to support improved deletion of combined emojis by enabling grapheme-aware text manipulation. This addition directly addresses the PR objective of ensuring that complete emoji clusters (such as 🇩🇪) are correctly removed without leaving residual characters.A couple of points to consider:
- Version Verification: Please verify that version ^1.0.4 fully meets the grapheme-splitting requirements and that there are no known breaking changes or security issues with this version.
- Integration Testing: Ensure that the integration tests covering the Monaco editor's backspace functionality adequately validate this enhancement.
ItsaaaMeMario
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
This issue is not related to this PR—you can see the yellow rectangle on other test servers as well. This PR only includes the deletion of emojis, along with a fix for the issue where emojis were only partially deleted during the deletion process. Here is what I face on TS5 (not deployed in this PR)
|
There was a problem hiding this comment.
Tested on TS3, works as expected.
PS: I noticed this while testing #10248. Maybe since it's similar to this PR you might be interested.
zagemello
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
alekspetrov9e
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
|
github-actions
bot
added
the
deployment-error
SindiBuklaji
added
deploy:artemis-test2
and removed
deployment-error
Checklist
General
Client
Motivation and Context
Currently, deleting a combined emoji (e.g., 🇩🇪) leaves behind a partial character instead of fully removing it. (Closes #9944)
Description
This issue has been fixed using grapheme-splitter. The backspace command in Monaco editor now detects and removes entire grapheme clusters, ensuring proper deletion of multi-character emojis.
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