-
Notifications
You must be signed in to change notification settings - Fork 350
Communication: Add possibility to add hyperlink to selected text
#11627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Communication: Add possibility to add hyperlink to selected text
#11627
Conversation
WalkthroughAdds a protected helper to TextEditorAction that conditionally wraps selected text via a provided wrapper (sanitizing the selection) or inserts a default placeholder when no selection exists. attachment and url actions now use this helper when invoked without arguments; argument-driven insertion behavior is unchanged. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Editor
participant TextEditorAction
participant SpecificAction
User->>SpecificAction: run() (no args)
SpecificAction->>TextEditorAction: wrapSelectionOrInsertDefault(editor, wrapperFn, defaultText)
alt selection exists
TextEditorAction->>Editor: getSelectedText()
Editor-->>TextEditorAction: selectedText
TextEditorAction->>TextEditorAction: sanitize selectedText
TextEditorAction->>Editor: replace selection with wrapperFn(sanitizedText)
Editor-->>TextEditorAction: selection replaced
else no selection
TextEditorAction->>Editor: replaceTextAtCurrentSelection(defaultText)
Editor-->>TextEditorAction: default inserted
end
SpecificAction->>Editor: editor.focus()
Editor-->>User: focused
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)src/main/webapp/**/*.ts⚙️ CodeRabbit configuration file Files:
🧠 Learnings (2)📚 Learning: 2024-10-25T07:52:32.513ZApplied to files:
📚 Learning: 2024-07-10T08:47:07.884ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/webapp/app/shared/monaco-editor/model/actions/url.action.ts (1)
42-46: Trim inconsistency (duplicate of issue in attachment.action.ts).The same trim inconsistency exists here as in
attachment.action.tslines 84-88. If the suggested refactoring is implemented, this issue will be resolved.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/url.action.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/**/*.ts
⚙️ CodeRabbit configuration file
Files:
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.tssrc/main/webapp/app/shared/monaco-editor/model/actions/url.action.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 8832
File: src/main/webapp/app/exercises/programming/assess/code-editor-tutor-assessment-container.component.ts:344-345
Timestamp: 2024-06-20T21:25:36.138Z
Learning: The Monaco editor uses 1-based line numbering, and this should be considered when handling line numbers in code related to this editor.
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 8832
File: src/main/webapp/app/exercises/programming/assess/code-editor-tutor-assessment-container.component.ts:344-345
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The Monaco editor uses 1-based line numbering, and this should be considered when handling line numbers in code related to this editor.
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 9463
File: src/main/webapp/app/shared/monaco-editor/model/themes/monaco-editor-theme.model.ts:51-51
Timestamp: 2024-10-13T12:01:14.787Z
Learning: In the Monaco editor TypeScript code, prefer simple string concatenation over nested template strings when nesting adds unnecessary complexity.
📚 Learning: 2024-10-25T07:52:32.513Z
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 9580
File: src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts:475-491
Timestamp: 2024-10-25T07:52:32.513Z
Learning: In `src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts`, `useCommunicationForFileUpload` is a Signal, so it should be called as a function when used.
Applied to files:
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: client-style
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
🔇 Additional comments (1)
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts (1)
83-88: Helper methods are properly defined in the parent class—no issues found.Both
getSelectedText()(line 172) andreplaceTextAtRange()(line 193) are correctly defined as public methods in the parent classTextEditorActionwithintext-editor-action.model.ts. The method calls in the code snippet are valid.
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts
Outdated
Show resolved
Hide resolved
| if (!args?.text || !args?.url) { | ||
| this.replaceTextAtCurrentSelection(editor, UrlAction.DEFAULT_INSERT_TEXT); | ||
| // Check if there's selected text | ||
| const selectedText = this.getSelectedText(editor); | ||
| if (selectedText && selectedText.trim()) { | ||
| // Wrap selected text with link syntax | ||
| const selection = editor.getSelection(); | ||
| if (selection) { | ||
| this.replaceTextAtRange(editor, selection, `[${sanitizeStringForMarkdownEditor(selectedText)}](https://)`); | ||
| } | ||
| } else { | ||
| // No selection, insert default text | ||
| this.replaceTextAtCurrentSelection(editor, UrlAction.DEFAULT_INSERT_TEXT); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated wrapping logic to parent class.
The text-wrapping logic in lines 39-51 is nearly identical to lines 81-93 in attachment.action.ts. The only difference is the markdown syntax format ([text](url) vs ).
Consider extracting this shared logic to the parent TextEditorAction class with a method like:
protected wrapSelectionOrInsertDefault(
editor: TextEditor,
wrapFormat: (text: string) => string,
defaultText: string
): void {
const selectedText = this.getSelectedText(editor)?.trim();
if (selectedText) {
const selection = editor.getSelection();
if (selection) {
this.replaceTextAtRange(editor, selection, wrapFormat(sanitizeStringForMarkdownEditor(selectedText)));
}
} else {
this.replaceTextAtCurrentSelection(editor, defaultText);
}
}Then both classes can call:
// In UrlAction:
this.wrapSelectionOrInsertDefault(editor, (text) => `[${text}](https://)`, UrlAction.DEFAULT_INSERT_TEXT);
// In AttachmentAction:
this.wrapSelectionOrInsertDefault(editor, (text) => ``, AttachmentAction.DEFAULT_INSERT_TEXT);🤖 Prompt for AI Agents
src/main/webapp/app/shared/monaco-editor/model/actions/url.action.ts lines
39-51: the selection-wrapping logic here duplicates logic in
attachment.action.ts (lines ~81-93) differing only by the wrapper syntax; move
that shared behavior into the parent TextEditorAction by adding a protected
helper (e.g., wrapSelectionOrInsertDefault) that accepts the editor, a function
to produce the wrapped string from sanitized text, and the default insert text,
and then replace the duplicated blocks in UrlAction and AttachmentAction with
calls to that helper using the appropriate wrapper-producing function for
`[text](https://)` and `` respectively.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts (1)
184-184: Tighten thewrapperFnparameter type.The
wrapperFnparameter is typed as(sanitizedText: string | undefined) => string, but at line 190 it's only ever invoked with astring(the result ofsanitizeStringForMarkdownEditorat line 189, called within theif (selectedText && selection)block whereselectedTextis guaranteed to be truthy). The| undefinedis unnecessary and may cause callers to add unneeded null checks.Apply this diff to correct the type signature:
- protected wrapSelectionOrInsertDefault(editor: TextEditor, wrapperFn: (sanitizedText: string | undefined) => string, defaultText: string): void { + protected wrapSelectionOrInsertDefault(editor: TextEditor, wrapperFn: (sanitizedText: string) => string, defaultText: string): void {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts(2 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/url.action.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/**/*.ts
⚙️ CodeRabbit configuration file
Files:
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.tssrc/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.tssrc/main/webapp/app/shared/monaco-editor/model/actions/url.action.ts
🧠 Learnings (8)
📚 Learning: 2024-10-25T07:52:32.513Z
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 9580
File: src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts:475-491
Timestamp: 2024-10-25T07:52:32.513Z
Learning: In `src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts`, `useCommunicationForFileUpload` is a Signal, so it should be called as a function when used.
Applied to files:
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.tssrc/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts
📚 Learning: 2024-07-10T08:47:07.884Z
Learnt from: florian-glombik
Repo: ls1intum/Artemis PR: 8858
File: src/main/webapp/app/shared/range-slider/range-slider.component.ts:16-16
Timestamp: 2024-07-10T08:47:07.884Z
Learning: For the PR ls1intum/Artemis#8858, avoid suggesting the removal of trivially inferred type annotations for the `step` property in `range-slider.component.ts`.
Applied to files:
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts
📚 Learning: 2024-10-13T12:01:14.787Z
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 9463
File: src/main/webapp/app/shared/monaco-editor/model/themes/monaco-editor-theme.model.ts:51-51
Timestamp: 2024-10-13T12:01:14.787Z
Learning: In the Monaco editor TypeScript code, prefer simple string concatenation over nested template strings when nesting adds unnecessary complexity.
Applied to files:
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.tssrc/main/webapp/app/shared/monaco-editor/model/actions/url.action.ts
📚 Learning: 2024-10-08T15:35:42.972Z
Learnt from: BaumiCoder
Repo: ls1intum/Artemis PR: 8552
File: src/main/webapp/app/exercises/shared/exercise-scores/exercise-scores.component.html:0-0
Timestamp: 2024-10-08T15:35:42.972Z
Learning: When rendering icons conditionally based on the presence of text, ensure that the text is not empty or whitespace to avoid displaying icons without popups.
Applied to files:
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts
📚 Learning: 2024-07-10T08:46:02.595Z
Learnt from: florian-glombik
Repo: ls1intum/Artemis PR: 8858
File: src/main/webapp/app/shared/range-slider/range-slider.component.ts:60-63
Timestamp: 2024-07-10T08:46:02.595Z
Learning: For the PR ls1intum/Artemis#8858, avoid suggesting changing the type for the `event` parameter in the `onSelectedMinValueChanged` and `onSelectedMaxValueChanged` methods in `range-slider.component.ts`.
Applied to files:
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts
📚 Learning: 2024-07-10T08:46:07.854Z
Learnt from: florian-glombik
Repo: ls1intum/Artemis PR: 8858
File: src/main/webapp/app/shared/range-slider/range-slider.component.ts:55-58
Timestamp: 2024-07-10T08:46:07.854Z
Learning: For the PR ls1intum/Artemis#8858, avoid suggesting changing the type of the `event` parameter in the `onSelectedMinValueChanged` method in `range-slider.component.ts`.
Applied to files:
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts
📚 Learning: 2024-10-20T21:59:11.630Z
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 9505
File: src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html:9-9
Timestamp: 2024-10-20T21:59:11.630Z
Learning: In Angular templates within the Artemis project (e.g., `src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html`), properties like `selectedFile()`, `readOnlyManualFeedback()`, `highlightDifferences()`, and `course()` are signals. It is appropriate to call these signals directly in the template.
Applied to files:
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts
📚 Learning: 2024-10-20T22:04:46.906Z
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 9505
File: src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html:36-36
Timestamp: 2024-10-20T22:04:46.906Z
Learning: In Angular templates, methods that return Signals (e.g., `feedbackSuggestionsForSelectedFile()` in `code-editor-monaco.component.ts`) can be used directly without performance issues because Signals are optimized for efficient change detection.
Applied to files:
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (3)
src/main/webapp/app/shared/monaco-editor/model/actions/attachment.action.ts (1)
81-82: LGTM! Refactoring addresses previous trim concern.The refactoring to use
wrapSelectionOrInsertDefaultresolves the earlier concern about trimming inconsistency. The helper now properly trims the selected text (line 185 in text-editor-action.model.ts) and uses the trimmed/sanitized result, avoiding markdown likewith surrounding whitespace.src/main/webapp/app/shared/monaco-editor/model/actions/url.action.ts (1)
39-40: LGTM! Duplication successfully eliminated.The extraction of the wrapping logic to the parent class's
wrapSelectionOrInsertDefaultmethod successfully addresses the previous concern about code duplication betweenUrlActionandAttachmentAction. Both now share the same core logic while providing their specific markdown syntax via the wrapper function.src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts (1)
177-195: LGTM! Helper method correctly handles all selection cases.The implementation correctly handles three scenarios:
- Non-empty selection: Trims, sanitizes, and wraps with the provided syntax
- Whitespace-only selection: Inserts default text (avoids empty links like
[](https://))- No selection (cursor only): Inserts default text at cursor position
The logic aligns well with the PR objectives and provides a clean, reusable solution for both
UrlActionandAttachmentAction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/**/*.ts
⚙️ CodeRabbit configuration file
Files:
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts
🧠 Learnings (3)
📚 Learning: 2024-10-25T07:52:32.513Z
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 9580
File: src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts:475-491
Timestamp: 2024-10-25T07:52:32.513Z
Learning: In `src/main/webapp/app/shared/markdown-editor/monaco/markdown-editor-monaco.component.ts`, `useCommunicationForFileUpload` is a Signal, so it should be called as a function when used.
Applied to files:
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts
📚 Learning: 2024-10-20T21:59:11.630Z
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 9505
File: src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html:9-9
Timestamp: 2024-10-20T21:59:11.630Z
Learning: In Angular templates within the Artemis project (e.g., `src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html`), properties like `selectedFile()`, `readOnlyManualFeedback()`, `highlightDifferences()`, and `course()` are signals. It is appropriate to call these signals directly in the template.
Applied to files:
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts
📚 Learning: 2024-10-20T22:04:46.906Z
Learnt from: pzdr7
Repo: ls1intum/Artemis PR: 9505
File: src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html:36-36
Timestamp: 2024-10-20T22:04:46.906Z
Learning: In Angular templates, methods that return Signals (e.g., `feedbackSuggestionsForSelectedFile()` in `code-editor-monaco.component.ts`) can be used directly without performance issues because Signals are optimized for efficient change detection.
Applied to files:
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: client-tests-selected
- GitHub Check: server-tests
- GitHub Check: client-style
🔇 Additional comments (1)
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts (1)
14-14: LGTM!The import is correctly added and used in the new helper method for sanitizing selected text.
src/main/webapp/app/shared/monaco-editor/model/actions/text-editor-action.model.ts
Outdated
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
Gavlitskii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested the hyperlink issue on a Test Server 2. Everything works as expected.
I followed the steps:
- I put example text and clicked the URL button. The text was appropriately formatted and converted into a link.
- It works with Bold or Italic as well.
- Even without any text the link is created as expected.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
Checklist
General
Client
Motivation and Context
Closes issue #9952
Currently, when users want to add a hyperlink to selected text in the markdown editor, they need to:
[text here]This workflow is cumbersome and doesn't match the behavior of modern markdown editors. Many applications (like Notion, Discord, Slack, etc.) allow users to simply select text and click the hyperlink button to wrap it with link syntax.
Description
This PR improves the user experience of adding hyperlinks in the markdown editor by automatically wrapping selected text with the link syntax when the hyperlink button is clicked.
Steps for Testing
Prerequisites:
Test 1: Wrap selected text with hyperlink
[Click here](https://)https://[Click here](https://example.com)Test 2: Insert default text when no selection
[🔗 Link](https://)is insertedTest 3: Wrap text with special characters
[**Bold** and *italic* text](https://)Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Screenshots
Before: User must unselect text, click button, then manually type
[text](url)old.mp4
After: User selects text, clicks button, and only needs to add the URL
test.mp4
Summary by CodeRabbit
New Features
Documentation