fix(text-editor): don't interpret : as links when pasting #3562
Conversation
📝 WalkthroughWalkthroughThis update refactors and extends link handling across the text editor and markdown parser. It introduces a centralized link attribute utility, enhances link mark specification and paste handling in ProseMirror, replaces an external markdown plugin with a custom one, and updates schema integration and tests. Redundant code and dependencies are removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant LinkUtils
participant MarkdownPlugin
User->>Editor: Paste text (may contain links)
Editor->>LinkUtils: Parse text, detect links, generate attributes
LinkUtils-->>Editor: Return nodes with appropriate link marks
Editor->>Editor: Insert nodes (linked text and plain text) into document
User->>Editor: Insert or edit link
Editor->>LinkUtils: getLinkAttributes(url, title)
LinkUtils-->>Editor: Return link attributes (target, rel, etc.)
Editor->>Editor: Apply link mark to selection
User->>MarkdownPlugin: Render markdown with links
MarkdownPlugin->>LinkUtils: getLinkAttributes(href, title)
LinkUtils-->>MarkdownPlugin: Return attributes
MarkdownPlugin->>MarkdownPlugin: Add attributes to <a> elements
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
There was a problem hiding this comment.
Pull Request Overview
This PR aims to improve link handling in the text editor by refactoring and enhancing paste event processing. Key changes include:
- Replacing the basic URL validation with a regular expression that supports http, mailto, and tel schemes.
- Refactoring paste event handling to generate text and link nodes dynamically.
- Removing redundant URL validation and copy-paste command logic from the menu commands.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts | Updated paste event processing with new helper functions and regex for detecting URLs, along with improved link node insertion logic. |
| src/components/text-editor/prosemirror-adapter/menu/menu-commands.ts | Removed the obsolete isValidUrl function and the copyPasteLinkCommand, updating the command mapping accordingly. |
Comments suppressed due to low confidence (1)
src/components/text-editor/prosemirror-adapter/menu/menu-commands.ts:214
- The removal of the isValidUrl function and copyPasteLinkCommand may affect workflows related to paste handling. Ensure that the new paste handling in the link plugin comprehensively covers these cases and that any necessary updates to documentation are made.
export const isValidUrl = (text: string): boolean => {
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts
Outdated
Show resolved
Hide resolved
FredrikWallstrom
force-pushed
the
link-improvements
branch
from
6fe93a6 to
ba61c34
Compare
FredrikWallstrom
changed the title
FredrikWallstrom
force-pushed
the
link-improvements
branch
from
ba61c34 to
02708fe
Compare
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3562/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
190-198: UseSchemainstead ofanyfor stronger type-safetyThe helper functions still accept
schema: any; switching toschema: Schema(fromprosemirror-model) surfaces typing errors at compile-time and fulfils a previous review suggestion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/components/text-editor/prosemirror-adapter/menu/menu-commands.ts(0 hunks)src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/components/text-editor/prosemirror-adapter/menu/menu-commands.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts
| const linkMark = schema.marks.link.create({ | ||
| href: url, | ||
| title: url, | ||
| // Only set _blank for http/https links, not for mailto/tel | ||
| target: url.startsWith('http') && isExternalLink(url) ? '_blank' : null, | ||
| }); | ||
|
|
||
| return schema.text(url, [linkMark]); |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to external links opened in a new tab
Opening external URLs with target="_blank" without the rel attribute allows the new page to access window.opener, posing a phishing/vector risk.
Extend the attributes so the rendered <a> element gets the protective rel.
- target: url.startsWith('http') && isExternalLink(url) ? '_blank' : null,
+ target: url.startsWith('http') && isExternalLink(url) ? '_blank' : null,
+ rel:
+ url.startsWith('http') && isExternalLink(url)
+ ? 'noopener noreferrer'
+ : null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const linkMark = schema.marks.link.create({ | |
| href: url, | |
| title: url, | |
| // Only set _blank for http/https links, not for mailto/tel | |
| target: url.startsWith('http') && isExternalLink(url) ? '_blank' : null, | |
| }); | |
| return schema.text(url, [linkMark]); | |
| const linkMark = schema.marks.link.create({ | |
| href: url, | |
| title: url, | |
| // Only set _blank for http/https links, not for mailto/tel | |
| target: url.startsWith('http') && isExternalLink(url) ? '_blank' : null, | |
| rel: | |
| url.startsWith('http') && isExternalLink(url) | |
| ? 'noopener noreferrer' | |
| : null, | |
| }); | |
| return schema.text(url, [linkMark]); |
🤖 Prompt for AI Agents
In src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts around
lines 198 to 205, the link mark sets target="_blank" for external http/https
links but does not include the rel="noopener noreferrer" attribute. To fix this,
add the rel attribute with the value "noopener noreferrer" alongside
target="_blank" for external links to prevent security risks from window.opener
access.
There was a problem hiding this comment.
This has not been introduced in this PR. I will fix this in a follow up PR.
FredrikWallstrom
force-pushed
the
link-improvements
branch
3 times, most recently
from
6c25b3c to
5d52323
Compare
Paste events for links are processed in link-plugin
FredrikWallstrom
force-pushed
the
link-improvements
branch
from
9b2c5d8 to
09dcdcd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/components/text-editor/prosemirror-adapter/plugins/link/link-plugin.ts (1)
146-146: 🧹 Nitpick (assertive)Add type annotation for better code quality.
The
clickTimeoutvariable should be typed to improve code maintainability and prevent potential type-related issues.-let clickTimeout; +let clickTimeout: ReturnType<typeof setTimeout> | undefined;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.json(0 hunks)src/components/markdown/link-markdown-plugin.ts(1 hunks)src/components/markdown/markdown-parser.ts(3 hunks)src/components/text-editor/prosemirror-adapter/menu/menu-commands.ts(2 hunks)src/components/text-editor/prosemirror-adapter/plugins/link/link-mark-spec.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/link/link-plugin.ts(2 hunks)src/components/text-editor/prosemirror-adapter/plugins/link/utils.spec.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/link/utils.ts(1 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(2 hunks)src/design-guidelines/text-editor/menu-command.md(2 hunks)
💤 Files with no reviewable changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/markdown/markdown-parser.tssrc/components/markdown/link-markdown-plugin.tssrc/components/text-editor/prosemirror-adapter/plugins/link/utils.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/link/utils.tssrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsxsrc/components/text-editor/prosemirror-adapter/plugins/link/link-mark-spec.tssrc/components/text-editor/prosemirror-adapter/plugins/link/link-plugin.tssrc/components/text-editor/prosemirror-adapter/menu/menu-commands.ts
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from the `render` method, they will sometime...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
🧬 Code Graph Analysis (6)
src/components/markdown/markdown-parser.ts (1)
src/components/markdown/link-markdown-plugin.ts (1)
createLinksPlugin(12-35)
src/components/markdown/link-markdown-plugin.ts (1)
src/components/text-editor/prosemirror-adapter/plugins/link/utils.ts (1)
getLinkAttributes(3-24)
src/components/text-editor/prosemirror-adapter/plugins/link/utils.spec.ts (1)
src/components/text-editor/prosemirror-adapter/plugins/link/utils.ts (1)
getLinkAttributes(3-24)
src/components/text-editor/prosemirror-adapter/plugins/link/utils.ts (1)
src/components/text-editor/prosemirror-adapter/plugins/link/link-mark-spec.ts (1)
LinkMarkAttrs(3-9)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (1)
src/components/text-editor/prosemirror-adapter/plugins/link/link-mark-spec.ts (1)
linkMarkSpec(11-44)
src/components/text-editor/prosemirror-adapter/menu/menu-commands.ts (1)
src/components/text-editor/prosemirror-adapter/plugins/link/utils.ts (1)
getLinkAttributes(3-24)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Build
- GitHub Check: Test
🔇 Additional comments (28)
src/components/text-editor/prosemirror-adapter/plugins/link/utils.ts (2)
3-24: LGTM! Solid implementation with proper security attributes.The function correctly distinguishes between external and internal URLs and applies appropriate security attributes (
target="_blank",rel="noopener noreferrer",referrerpolicy="noreferrer") for external links while keeping internal links clean.
46-48: LGTM! Simple and effective helper function.The protocol-relative URL detection is straightforward and correct.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (2)
37-38: LGTM! Import paths correctly follow coding guidelines.The import paths use relative paths as required by the coding guidelines for files in the same module. The organization into a
plugins/link/directory structure improves code organization.
394-394: Excellent integration of the new link mark specification.Adding
linkMarkSpecto the schema marks properly integrates the enhanced link handling capabilities into the ProseMirror editor. This will enable the centralized link attribute logic across the editor.src/components/markdown/markdown-parser.ts (3)
15-15: LGTM! Proper use of relative import path.The import correctly uses a relative path as required by the coding guidelines for same-module imports.
44-44: Excellent replacement of external dependency with custom solution.Replacing
rehype-external-linkswith the customcreateLinksPlugin()provides better control over link attribute handling and ensures consistency with the ProseMirror editor's link processing logic.
110-110: Good addition of referrerpolicy to sanitization whitelist.Adding
referrerpolicyto the allowed attributes for<a>elements ensures that the security attributes set by the custom link plugin are preserved during HTML sanitization.src/components/markdown/link-markdown-plugin.ts (2)
6-11: LGTM! Clear and comprehensive documentation.The JSDoc documentation clearly explains the plugin's purpose and return type. Good documentation practices.
12-35: Excellent unified.js plugin implementation.The plugin implementation follows unified.js best practices:
- Proper plugin structure returning a transformer function
- Efficient AST traversal using
visit- Appropriate type checking (
node.tagName === 'a')- Null safety check for
hrefbefore processing- Consistent attribute application using the centralized
getLinkAttributesutilityThe reuse of
getLinkAttributesensures consistency between markdown processing and ProseMirror editor link handling.src/components/text-editor/prosemirror-adapter/plugins/link/utils.spec.ts (3)
1-1: Import path needs to be relativeThe import statement uses a relative path which is correct for this module. This follows the coding guideline for imports within the same module.
3-23: Well-implemented test setup and teardownThe test setup properly mocks
window.locationwith a realistic internal domain configuration and restores it after each test. This ensures test isolation and prevents side effects between tests.
25-132: Comprehensive test coverage with parameterized testsThe test suite provides excellent coverage of various URL scenarios including:
- Relative URLs, fragment URLs, protocol-relative URLs
- Internal vs external domain handling
- URLs with query parameters
- Malformed URLs
- Null title preservation
The assertions correctly verify that:
- Internal links have
target,rel, andreferrerpolicyset tonull- External links have security attributes set (
target="_blank",rel="noopener noreferrer",referrerpolicy="noreferrer")- URL and title values are preserved correctly
src/design-guidelines/text-editor/menu-command.md (3)
108-110: Good refactoring to centralize link mark creationMoving the
linkMarkcreation outside the conditional branches eliminates code duplication and leverages the centralizedgetLinkAttributesutility. This makes the code more maintainable and consistent with the broader link handling refactor.
113-113: Clear documentation with helpful commentsThe added comment clearly explains when this code path is executed (no text selected), making the documentation more accessible to developers.
118-118: Clear documentation with helpful commentsThe added comment clearly explains when this code path is executed (text is selected), improving code comprehension.
src/components/text-editor/prosemirror-adapter/menu/menu-commands.ts (2)
6-6: Import path should be relativeThe import uses a relative path
'../plugins/link/utils'which is correct according to the coding guidelines for imports within the same module (lime-elements).
86-88: Excellent refactoring to use centralized link attributesThe refactoring to use
getLinkAttributes(link.href, link.href)centralizes link attribute creation and ensures consistent handling of internal vs external links with appropriate security attributes. This eliminates the previous manual attribute setting and aligns with the broader link handling improvements.Note: Using
link.hrefas both the URL and title parameter seems intentional based on the function signature, where title can be null and defaults to the URL.src/components/text-editor/prosemirror-adapter/plugins/link/link-mark-spec.ts (5)
1-1: Import statement is correctThe import uses absolute imports from the ProseMirror library, which is appropriate for external dependencies.
3-9: Well-defined interface for link attributesThe
LinkMarkAttrsinterface comprehensively covers all necessary link attributes including security-related ones (rel,referrerpolicy). The nullable types for optional attributes are correctly specified.
11-19: Proper mark specification with sensible defaultsThe mark specification correctly defines all attributes with appropriate defaults. Setting
inclusive: falseis correct for link marks to prevent them from extending to adjacent text.
20-33: Comprehensive DOM parsing implementationThe
parseDOMconfiguration correctly extracts all link attributes from anchor elements. ThegetAttrsfunction properly handles missing attributes by defaultinghrefto an empty string and allowing other attributes to be null.
34-44: Excellent security-aware DOM serializationThe
toDOMmethod implements important security measures by automatically addingrel="noopener noreferrer"andreferrerpolicy="noreferrer"whentarget="_blank"is set. This prevents potential security vulnerabilities when opening external links in new tabs.The approach of spreading both
mark.attrsandsecurityAttrsensures that security attributes take precedence over any potentially unsafe attributes from the mark.src/components/text-editor/prosemirror-adapter/plugins/link/link-plugin.ts (6)
3-6: LGTM! Imports align with the new link handling functionality.The added imports for ProseMirror model types and the centralized
getLinkAttributesutility support the enhanced paste handling implementation.
175-189: Well-designed URL detection that addresses the PR objective.The URL regex pattern correctly matches HTTP(S), mailto, and tel links while avoiding false positives on isolated colons (like timestamps). Good practice resetting the regex
lastIndexbefore use.
193-227: Clean utility functions with proper regex handling.The node creation utilities are well-structured, and
findLinkMatchescorrectly manages the regex state by resettinglastIndex. The structured return data makes it easy to process matches.
232-264: Excellent implementation of text-to-nodes conversion.The function correctly handles all scenarios: text without links, links with surrounding text, multiple links, and proper text segmentation. The logic ensures no text is lost during the conversion.
271-337: Well-designed paste handling with good UX considerations.The implementation correctly distinguishes between single link and multi-node pastes. The single link logic thoughtfully preserves selected text when applying a link mark, providing intuitive behavior for users.
342-355: Clean refactoring of paste event handling.The simplified implementation effectively uses the new utility functions to handle URL detection and node creation, making the code more maintainable while addressing the PR's objective of proper link detection.
| function isExternalUrl(url: string): boolean { | ||
| try { | ||
| let urlObj: URL; | ||
|
|
||
| if (isProtocolRelativeUrl(url)) { | ||
| urlObj = new URL(window.location.protocol + url); | ||
| } else { | ||
| urlObj = new URL(url, window.location.origin); | ||
| } | ||
|
|
||
| return ( | ||
| urlObj.protocol.startsWith('http') && | ||
| urlObj.hostname !== window.location.hostname | ||
| ); | ||
| } catch { | ||
| // Malformed URLs → internal | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify edge case handling for malformed URLs and review protocol logic.
The URL parsing logic handles most cases well, but there are a few considerations:
-
Malformed URL handling: Defaulting to
internal(line 42) for malformed URLs is generally safe, but consider if this could mask legitimate external URLs that are slightly malformed. -
Protocol restriction: The check
urlObj.protocol.startsWith('http')(line 37) excludes other protocols likehttps://,ftp://,mailto:,tel:, etc. This seems intentional for security, but verify this aligns with requirements.
Let me verify how this handles various URL formats:
🏁 Script executed:
#!/bin/bash
# Description: Test URL parsing logic with various formats to verify behavior
# Test: Check for existing tests that cover edge cases
rg -A 10 -B 2 "isExternalUrl|getLinkAttributes" --type ts | head -50Length of output: 5064
Verify isExternalUrl protocol logic & add edge-case tests
Please confirm that defaulting malformed URLs to “internal” and only treating http* protocols as external aligns with product/security requirements. Also add unit tests for these cases:
- Absolute HTTP/HTTPS URLs (external vs same-host)
- Relative URLs (e.g.
/path/to/resource) - Protocol-relative URLs (e.g.
//cdn.example.com) - Malformed URLs (e.g.
ht tp://, empty strings) - Other schemes (e.g.
ftp://,mailto:,tel:,data:)
Ensure any intended external schemes are correctly detected or explicitly excluded.
🤖 Prompt for AI Agents
In src/components/text-editor/prosemirror-adapter/plugins/link/utils.ts around
lines 26 to 44, confirm with the product/security team that treating malformed
URLs as internal and restricting external URLs to those with protocols starting
with 'http' meets requirements. Then, add comprehensive unit tests covering
absolute HTTP/HTTPS URLs (both external and same-host), relative URLs,
protocol-relative URLs, malformed URLs (like invalid syntax and empty strings),
and other schemes such as ftp, mailto, tel, and data. Ensure the tests verify
that only intended external schemes are detected as external and others are
excluded or treated as internal accordingly.
| import { visit } from 'unist-util-visit'; | ||
| import { Node } from 'unist'; | ||
| import { Plugin, Transformer } from 'unified'; | ||
| import { getLinkAttributes } from '../text-editor/prosemirror-adapter/plugins/link/utils'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify import path compliance with coding guidelines.
The import uses a relative path ../text-editor/prosemirror-adapter/plugins/link/utils, which follows the coding guidelines for same-module imports. However, this creates a dependency from the markdown component to the text-editor component.
Let me verify if this cross-component dependency is acceptable in the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Check for similar cross-component imports in markdown files
# Expected: Find patterns of imports between different component directories
rg "from '\.\./[^/]+/" src/components/markdown/ || echo "No cross-component imports found"Length of output: 540
Extract getLinkAttributes into a shared utilities module
Importing getLinkAttributes directly from the text-editor component is valid but introduces unnecessary coupling between the markdown and text-editor modules. To preserve clear boundaries, please:
- Move the
getLinkAttributesfunction into a shared location (for example,src/shared/link-utils.tsorsrc/utils/link.ts). - Update this file’s import (and the text-editor plugin) to reference the new shared module path.
• File: src/components/markdown/link-markdown-plugin.ts (line 4)
This refactor ensures both markdown and text-editor plugins can consume the utility without cross-component dependencies.
🤖 Prompt for AI Agents
In src/components/markdown/link-markdown-plugin.ts at line 4, the import of
getLinkAttributes from the text-editor component creates an unwanted
cross-component dependency. To fix this, move the getLinkAttributes function to
a shared utilities module such as src/shared/link-utils.ts or src/utils/link.ts.
Then update the import in this file and in the text-editor plugin to reference
the new shared module path, ensuring both components use the utility without
coupling.
|
🎉 This PR is included in version 38.13.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Fixes https://github.com/Lundalogik/crm-client/issues/24
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores
How to Test
This PR improves the link handling in the text editor, particularly when pasting content with URLs. Here's how to test these enhancements:
Multiple links in one paste operation
Links with surrounding text
Test URLs with query parameters, hashes, and paths (e.g., "https://example.com/path?query=value#section")
Trailing text after links
Testing Email Links
Testing Phone links
Testing Colon Handling
External vs. Internal links
Command/Ctrl+Click behavior
Paste link over selection
Play around on your own with pasting/creating links, make sure everything works as you would expect.
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: