[PE-298] Fix: Copy markdown to clipboard #6675
Conversation
WalkthroughThis pull request introduces a new clipboard extension for the Tiptap editor called Changes
Suggested labels
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Definitions (5)packages/editor/src/core/extensions/read-only-extensions.tsx (1)
packages/editor/src/core/extensions/mentions/extension.tsx (1)
space/core/components/editor/rich-text-read-only-editor.tsx (2)
space/core/components/editor/lite-text-read-only-editor.tsx (2)
packages/editor/src/core/extensions/mentions/extension-config.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (27)
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
![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 (5)
packages/editor/src/core/extensions/clipboard.ts (5)
6-41: The MarkdownClipboard extension implementation looks goodThe extension correctly determines when to use Markdown serialization based on content complexity and falls back to plain text when appropriate.
Consider adding a brief comment explaining the criteria used to determine when content should be copied as Markdown (lines 22-26) to make the logic more maintainable.
const copyAsMarkdown = + // Copy as Markdown when content spans multiple lines or contains complex nested structures isMultiline || slice.content.content.some( (node) => node.content.content.length > 1 );
28-33: Consider error handling when plain text serialization is usedThe current implementation assumes the node can be correctly serialized to plain text, but there might be edge cases.
if (!copyAsMarkdown) { - return slice.content.content.map((node) => - toPlainText(node, this.editor.schema) - ).join('') + try { + return slice.content.content.map((node) => + toPlainText(node, this.editor.schema) + ).join('') + } catch (error) { + console.warn('Error serializing to plain text, falling back to default', error); + return ''; + } }
34-35: Add error handling for Markdown serializationThe code assumes
this.editor.storage.markdown.serializeralways exists and works properly. Adding error handling would make the code more robust.-return this.editor.storage.markdown.serializer.serialize(slice.content); +try { + return this.editor.storage.markdown.serializer.serialize(slice.content); +} catch (error) { + console.warn('Error serializing to markdown, falling back to plain text', error); + // Fall back to plain text + return slice.content.textBetween(0, slice.content.size, "\n"); +}
47-53: Utility function looks good, but consider scopeThe
getTextSerializersfunction is well-implemented but exported without a clear need for external usage. Consider keeping it as a private function unless it's used elsewhere.-export function getTextSerializers(schema: Schema) { +function getTextSerializers(schema: Schema) {
55-96: Well-structured textBetween implementationThe function handles the complexity of extracting text with appropriate block separators. A couple of minor improvements could be made:
- Consider making the blockSeparator configurable through a parameter with a default value
- The function is exported as default while others aren't, which is inconsistent
-export type PlainTextSerializer = (node: ProsemirrorNode) => string; -export default function textBetween( +type PlainTextSerializer = (node: ProsemirrorNode) => string; +function textBetween( doc: ProsemirrorNode, from: number, to: number, - plainTextSerializers: Record<string, PlainTextSerializer | undefined> + plainTextSerializers: Record<string, PlainTextSerializer | undefined>, + blockSeparator: string = "\n" ): string { let text = ""; let first = true; - const blockSeparator = "\n";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/editor/package.json(1 hunks)packages/editor/src/core/extensions/clipboard.ts(1 hunks)packages/editor/src/core/extensions/extensions.tsx(2 hunks)
🔇 Additional comments (3)
packages/editor/package.json (1)
66-66: Addition of prosemirror-model dependency looks goodThis dependency is correctly added to support the new clipboard functionality for Markdown serialization.
packages/editor/src/core/extensions/extensions.tsx (2)
39-39: Correct import of the new MarkdownClipboard extensionThe import is properly placed with other related imports.
145-145: Good placement of the MarkdownClipboard extensionThe extension is strategically positioned after the Markdown extension, which makes sense for the clipboard functionality to work properly with Markdown serialization.
|
Hey @Palanikannan1437 Can you review this PR? |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/editor/src/core/extensions/clipboard.ts (4)
22-27: Improve readability of the copy condition.The condition for determining when to copy as Markdown is complex and could benefit from better naming or documentation.
- const copyAsMarkdown = - isMultiline || - slice.content.content.some( - (node) => node.content.content.length > 1 - ); + // Copy as Markdown if content is multiline or contains complex nodes with nested structure + const hasComplexNodes = slice.content.content.some( + (node) => node.content.content.length > 1 + ); + const copyAsMarkdown = isMultiline || hasComplexNodes;
80-90: Improve readability of block separator logic.The complex condition for deciding when to add block separators could be extracted into a named helper function for clarity.
+ const shouldAddBlockSeparator = (node: ProsemirrorNode, text: string): boolean => { + return node.isBlock && ((node.isLeaf && text) || node.isTextblock) && !!blockSeparator; + }; - if ( - node.isBlock && - ((node.isLeaf && nodeText) || node.isTextblock) && - blockSeparator - ) { + if (shouldAddBlockSeparator(node, nodeText)) { if (first) { first = false; } else { text += blockSeparator; } }
63-64: Improve variable name clarity.The variable
firstcould be more descriptive to better convey its purpose.- let first = true; + let isFirstBlock = true; // And later in the code: - if (first) { - first = false; + if (isFirstBlock) { + isFirstBlock = false;
2-3: Inconsistent import styles detected.There's inconsistency in the import quotes (double vs. single). Maintain consistency with the project's style guide.
import { Extension } from "@tiptap/core"; -import { Schema, Node as ProsemirrorNode } from "@tiptap/pm/model"; -import { Plugin, PluginKey } from '@tiptap/pm/state'; +import { Schema, Node as ProsemirrorNode } from "@tiptap/pm/model"; +import { Plugin, PluginKey } from "@tiptap/pm/state";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/src/core/extensions/clipboard.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
packages/editor/src/core/extensions/clipboard.ts (2)
1-97: Overall implementation looks sound.The implementation of the Markdown clipboard functionality is well-structured and provides a good enhancement to the editor's copy/paste capabilities. The extension correctly determines when to serialize content as Markdown versus plain text, with appropriate helper functions to support this functionality.
66-93:❓ Verification inconclusive
Consider performance optimization for large documents.
The
nodesBetweenmethod traverses potentially many nodes, which could be a performance concern for very large documents. Consider adding early return conditions or pagination for extremely large selections.
🏁 Script executed:
#!/bin/bash # Check if there are any large document handling mechanisms already in place rg "nodesBetween" --type ts -A 5 -B 5Length of output: 4306
Performance Optimization Suggestion: Review Node Traversal for Large Documents
The current implementation in
packages/editor/src/core/extensions/clipboard.ts(lines 66–93) usesdoc.nodesBetweento iterate over all nodes in the selected range. While this approach works well for typical document sizes, it could become a performance concern with very large documents. Similar patterns elsewhere in the codebase (for example, inpackages/editor/src/core/helpers/insert-empty-paragraph-at-node-boundary.ts, where an early exit is used) suggest that additional safeguards might be beneficial.Please evaluate whether introducing an early termination condition or paginating the iteration for extensive selections could help improve performance. Testing with larger document sizes might clarify if this optimization is necessary in the context of your application.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
packages/editor/src/core/extensions/clipboard.ts (4)
1-1: 🛠️ Refactor suggestionRemove unused import.
The
serializeimport from "node:v8" is not used anywhere in this file and should be removed.-import { serialize } from "node:v8";
9-14: 🛠️ Refactor suggestionAddress unused clipboard configuration options.
The extension defines
transformPastedTextandtransformCopiedTextoptions but never actually uses them in the implementation. Either implement the functionality to use these options or remove them to avoid confusion.addOptions() { return { transformPastedText: false, transformCopiedText: true, }; },The clipboardTextSerializer should check these options before transforming content:
clipboardTextSerializer: (slice) => { + const { transformCopiedText } = this.options; + if (!transformCopiedText) { + return null; // Let the default clipboard handler take over + } const markdownSerializer = this.editor.storage.markdown.serializer;
22-22:⚠️ Potential issueAdd error handling for missing Markdown serializer.
The code directly accesses
this.editor.storage.markdown.serializerwithout checking if it exists. This could cause runtime errors if the Markdown extension is not loaded or initialized properly.- const markdownSerializer = this.editor.storage.markdown.serializer; + if (!this.editor.storage.markdown?.serializer) { + console.warn('Markdown serializer not found, falling back to default clipboard behavior'); + return null; // Let the default clipboard handler take over + } + const markdownSerializer = this.editor.storage.markdown.serializer;
65-67:⚠️ Potential issueAdd null check and default return value.
The final part of the clipboardTextSerializer lacks proper null checks and a default return value.
- const targetNode = traverseToParentOfLeaf(slice.content.firstChild, slice.content, slice.openStart); + const targetNode = traverseToParentOfLeaf(slice.content.firstChild || null, slice.content, slice.openStart); return markdownSerializer.serialize(targetNode); + // Default fallback - this code is unreachable but added for completeness + return null;
🧹 Nitpick comments (1)
packages/editor/src/core/extensions/clipboard.ts (1)
1-72: Implement feature to handle pasted text transformation.The extension has an option
transformPastedTextbut doesn't implement any functionality for handling pasted text. This should be implemented to make the option useful.props: { clipboardTextSerializer: (slice) => { // Current implementation handling copied text }, + handlePaste: (view, event, slice) => { + const { transformPastedText } = this.options; + if (!transformPastedText) { + return false; // Let the default paste handler take over + } + + // Handle markdown text pasted into the editor + // This is just a skeleton - implement according to your requirements + if (event.clipboardData && event.clipboardData.getData('text/plain')) { + const text = event.clipboardData.getData('text/plain'); + // Process the text if needed + // Insert the processed content + return true; // Signal that the paste was handled + } + + return false; // Let the default paste handler take over + }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/src/core/extensions/clipboard.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
There was a problem hiding this comment.
transformCopiedText: true,
you have to make this false right @VipinDevelops
…ipboard into core extensions
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/editor/src/core/extensions/clipboard.ts (1)
14-14: 🛠️ Refactor suggestionAdd error handling for missing Markdown serializer.
The code directly accesses
this.editor.storage.markdown.serializerwithout checking if it exists. This could cause runtime errors if the Markdown extension is not loaded or initialized properly.- const markdownSerializer = this.editor.storage.markdown.serializer; + const markdownSerializer = this.editor.storage.markdown?.serializer; + if (!markdownSerializer) { + console.warn('Markdown serializer not found, falling back to plain text'); + return slice.content.textBetween(0, slice.content.size, '\n'); + }
🧹 Nitpick comments (1)
packages/editor/src/core/extensions/clipboard.ts (1)
72-72: Add null check for firstChild access.The code assumes
slice.content.firstChildexists when passing it totraverseToParentOfLeaf. This could lead to runtime errors if the slice has no content.- const targetNode = traverseToParentOfLeaf(slice.content.firstChild, slice.content, slice.openStart); + const targetNode = traverseToParentOfLeaf(slice.content.firstChild || null, slice.content, slice.openStart);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/editor/src/core/extensions/clipboard.ts(1 hunks)packages/editor/src/core/extensions/extensions.tsx(2 hunks)packages/editor/src/core/extensions/index.ts(1 hunks)packages/editor/src/core/extensions/read-only-extensions.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/extensions/extensions.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (11)
packages/editor/src/core/extensions/index.ts (1)
26-26: Added export for the new clipboard extension.This change exports the clipboard module, making the
MarkdownClipboardextension accessible to other parts of the codebase. This is a necessary addition to support the fix for copying markdown to clipboard.packages/editor/src/core/extensions/read-only-extensions.tsx (3)
27-27: Added new MarkdownClipboard extension import.Importing the MarkdownClipboard extension from the extensions directory to use it in the read-only editor configuration.
125-125: Changed Markdown configuration to disable built-in text transformation.Setting
transformCopiedText: falsedisables the built-in Markdown text transformation when copying, allowing the new MarkdownClipboard extension to handle this functionality instead.
127-127: Added MarkdownClipboard extension to the read-only editor.Added the MarkdownClipboard extension to the list of extensions for the read-only editor, enabling proper markdown serialization when copying content.
packages/editor/src/core/extensions/clipboard.ts (7)
1-4: Clean import statements for the new clipboard extension.The imports are correctly specified, importing only the necessary components from Tiptap and ProseMirror.
5-7: New MarkdownClipboard extension.The extension is properly created with a descriptive name "markdownClipboard".
8-17: Implemented clipboardTextSerializer for markdown support.The extension adds a ProseMirror plugin with a custom clipboardTextSerializer implementation to handle markdown serialization when copying content.
22-31: Table content processing with proper null checks.The table content processing function handles serialization of table cells to markdown format. The implementation includes proper null checks using optional chaining, which is good for preventing runtime errors.
33-41: Table row handling with safer property access.The table row handling now uses proper null checks with optional chaining and default values, which is an improvement over previous implementations. This prevents potential null reference errors when dealing with table structures.
43-67: Improved node traversal function.The
traverseToParentOfLeaffunction has been rewritten to use a while loop instead of recursion, which is more efficient and less prone to stack overflow errors. The implementation includes proper null checks and safely navigates the node structure.
69-74: Add default fallback return value.The function doesn't have a default return value for cases that might fall through all the conditional checks. It's important to add a default return to ensure consistent behavior.
if (slice.content.childCount > 1) { return markdownSerializer.serialize(slice.content); } else { const targetNode = traverseToParentOfLeaf(slice.content.firstChild, slice.content, slice.openStart); return markdownSerializer.serialize(targetNode); } + // Default fallback - this line should never be reached due to the if/else structure above + // but adding it for code safety + return markdownSerializer.serialize(slice.content);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/editor/src/core/extensions/clipboard.ts (1)
69-83: Consider optional chaining in line 75.
Replacing explicit checks with optional chaining can simplify the while condition.- while (currentNode && currentNode.content && currentNode.childCount === 1 && currentNode.firstChild) { + while (currentNode?.content && currentNode?.childCount === 1 && currentNode?.firstChild) {🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/src/core/extensions/clipboard.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/editor/src/core/extensions/clipboard.ts
[error] 75-75: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
packages/editor/src/core/extensions/clipboard.ts (7)
1-4: Imports look good.
No issues identified.
5-7: Extension naming is clear.
Straightforward naming, no concerns.
8-12: Plugin creation is clean.
Well-defined plugin key and setup.
13-21: Add error handling for missing Markdown serializer.
If the Markdown extension is not loaded, this could lead to runtime errors.
22-31: Table content processing logic looks correct.
The optional chaining ensures safe cell content access.
33-41: Table row handling looks well-structured.
Using early returns for single vs. multiple row/cell cases is clear.
43-67: Traverse function is logically sound.
The loop correctly returns the node when certain conditions are met.
VipinDevelops
changed the title
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
|
Is there any revision release planned for the next days including this fix? |
Markdown
Screen.Recording.2025-03-03.at.5.46.33.PM.mov
Triple click
Screen.Recording.2025-02-27.at.12.17.08.PM.mov
Summary by CodeRabbit
MarkdownClipboardextension to manage clipboard operations within the Tiptap editor framework.