[PE-80] fix: escape markdown content for images #6096
Conversation
WalkthroughThe pull request introduces significant modifications to 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
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: 1
🧹 Outside diff range and nitpick comments (3)
packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (3)
Line range hint
13-39: Consider adding accessibility attributesThe image component could benefit from accessibility improvements. Consider adding support for
alttext androleattributes in the configuration.addAttributes() { return { ...this.parent?.(), + alt: { + default: null, + }, + role: { + default: 'img', + }, width: { default: "35%", },
Line range hint
61-65: Add error handling for image source retrievalThe
getImageSourcecommand should include error handling to gracefully handle failed image retrievals.addCommands() { return { - getImageSource: (path: string) => async () => await getAssetSrc(path), + getImageSource: (path: string) => async () => { + try { + return await getAssetSrc(path); + } catch (error) { + console.error('Failed to load image:', error); + return null; + } + }, }; },
Line range hint
1-71: Verify integration with markdown export systemThe removal of markdown serialization in this component might affect the broader markdown export system. Please ensure:
- The change aligns with the overall markdown processing pipeline
- Other components that might depend on image markdown serialization are updated
- The export functionality still works as expected with this change
Consider documenting the new markdown handling approach in the component's documentation or README to help other developers understand the intended behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/editor/src/core/extensions/custom-image/custom-image.ts(2 hunks)packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts(2 hunks)
🔇 Additional comments (2)
packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (1)
55-59: Verify the empty serializer's impact on markdown escaping
While removing the serialization logic might prevent incorrect markdown generation, an empty serializer might not properly handle all image-related markdown scenarios. Please clarify:
- How are images represented when copying content?
- What happens when exporting content with images?
- How are existing markdown images handled?
Let's check for any related test cases or usage patterns:
✅ Verification successful
Empty serializer is the correct approach for read-only image handling
The empty serialize() method in the markdown storage is intentional and correct for the following reasons:
- The read-only extension (
CustomReadOnlyImageExtension) is specifically designed to display images without editing capabilities (noteselectable: false, draggable: false). - The parent extension configuration shows that markdown transformation is handled at the editor level with
Markdown.configure({ html: true, transformCopiedText: true }). - The empty serializer prevents any markdown generation for images in read-only mode, which is the desired behavior as these images should not be editable or exportable.
- The editable version (
CustomImageExtension) has the same empty serializer, maintaining consistency in how images are handled during markdown operations.
The implementation aligns with the PR's goal of fixing markdown content escaping for images by explicitly preventing markdown serialization in both read-only and editable contexts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test cases and usage patterns
echo "Searching for test files related to image handling..."
fd -e test.ts -e spec.ts | xargs rg -l "imageComponent|CustomReadOnlyImageExtension"
echo "Searching for markdown export/copy implementations..."
rg -g '!*.test.ts' -g '!*.spec.ts' "markdown.*export|markdown.*copy|markdown.*serialize"
Length of output: 1171
Script:
#!/bin/bash
# Let's check the implementation details of the custom image extensions and related markdown handling
# Check the custom image implementation
echo "=== Custom Image Implementation ==="
rg -A 10 "markdown.*serialize" packages/editor/src/core/extensions/custom-image/custom-image.ts
# Check if there are any other markdown serializers in the codebase
echo -e "\n=== Other Markdown Serializers ==="
rg -A 5 "markdown.*serialize"
# Check how images are handled in exports
echo -e "\n=== Image Export Handling ==="
rg -A 10 "export.*image|copy.*image"
# Check the parent extension configuration
echo -e "\n=== Parent Extension Configuration ==="
cat packages/editor/src/core/extensions/read-only-extensions.tsx
Length of output: 10189
Script:
#!/bin/bash
# Let's examine the markdown configuration and image handling in both custom and read-only image extensions
echo "=== Markdown Configuration in Extensions ==="
ast-grep --pattern 'Markdown.configure({
$$$
})'
echo -e "\n=== Custom Image Extension Implementation ==="
cat packages/editor/src/core/extensions/custom-image/custom-image.ts
echo -e "\n=== Read-only Image Extension Implementation ==="
cat packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts
echo -e "\n=== Base Image Extension Implementation ==="
cat packages/editor/src/core/extensions/image/image-extension-without-props.ts
Length of output: 8710
packages/editor/src/core/extensions/custom-image/custom-image.ts (1)
127-130: Verify the impact of removing markdown serialization
While removing the serialization logic aligns with the goal of escaping markdown content for images, we should verify that this change doesn't break existing functionality.
Let's check for any dependencies on markdown serialization:
✅ Verification successful
Empty markdown serialization is intentional and safe
The empty markdown serialization in custom-image.ts is correct and aligns with the codebase's image handling strategy. Here's why:
- The markdown export functionality is handled separately in
editor.helper.tsthroughreplaceCustomComponentsFromMarkdownContent(), which properly converts image components to markdown format. - Both read-only and editable image extensions have the same empty serialization, indicating this is the intended design.
- The export functionality in
export-page-modal.tsxuses the editor's built-ingetMarkDown()method combined with the helper functions, bypassing the need for individual extension serialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for code that might depend on image markdown serialization
rg -l "markdown" | xargs rg -l "serialize|export|copy"
# Search for tests related to image serialization
rg -l "test|spec" | xargs rg -l "image.*markdown|markdown.*image"
Length of output: 1019
Script:
#!/bin/bash
# Check the implementation of read-only-custom-image.ts to understand the relationship
cat packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts
# Check export-page-modal.tsx to understand if it depends on markdown serialization
cat web/core/components/pages/modals/export-page-modal.tsx
# Check markdown-to-component.tsx to understand markdown processing
cat web/core/components/ui/markdown-to-component.tsx
Length of output: 12633
Script:
#!/bin/bash
# Check the editor helper to understand markdown content handling
cat web/helpers/editor.helper.ts
# Check if there are any tests for image export functionality
rg -l "test" packages/editor/src/core/extensions/custom-image/ | xargs cat
Length of output: 10075
| // escape markdown for images | ||
| markdown: { | ||
| serialize(state: MarkdownSerializerState, node: Node) { | ||
| const attrs = node.attrs as ImageAttributes; | ||
| const imageSource = state.esc(this?.editor?.commands?.getImageSource?.(attrs.src) || attrs.src); | ||
| const imageWidth = state.esc(attrs.width?.toString()); | ||
| state.write(`<img src="${state.esc(imageSource)}" width="${imageWidth}" />`); | ||
| state.closeBlock(node); | ||
| }, | ||
| serialize() {}, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests to validate the fix
To ensure the fix works as intended and prevent regressions, consider adding tests that verify:
- Images are correctly handled when copying content
- Images are properly preserved when exporting to markdown
- The editor correctly displays images after import/export operations
Here's a suggested test structure:
describe('CustomImageExtension markdown handling', () => {
it('should preserve images when copying content', () => {
// Test implementation
});
it('should correctly export images to markdown', () => {
// Test implementation
});
it('should properly display images after import/export', () => {
// Test implementation
});
});
|
Why are we not trying to translate it to markdown? Is it because of presigned urls? |
Bug fix:
Escape image components when copying/exporting editor content as markdown.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor