Conversation
|
WalkthroughThe changes primarily involve modifications to several TypeScript files within the project. The Changes
Possibly related PRs
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: 5
🧹 Outside diff range and nitpick comments (2)
packages/core/src/parse-html/parse-elem-html.ts (1)
44-44: Approve functionality with style suggestionsThe text node handling logic is correct and properly addresses the inline element parsing issue. However, consider improving readability with better formatting:
- if ($childElem[0].nodeType === NodeType.TEXT_NODE) { return { text: $childElem[0].textContent || '' } } + if ($childElem[0].nodeType === NodeType.TEXT_NODE) { + return { text: $childElem[0].textContent || '' } + }packages/core/src/create/create-editor.ts (1)
8-31: Consider organizing imports by category.The imports could be better organized into clear categories for improved maintainability:
- External dependencies (slate, slate-history)
- Core configurations and interfaces
- Editor plugins
- UI components
- Utilities
import { createEditor, Descendant } from 'slate' import { withHistory } from 'slate-history' + import { genEditorConfig } from '../config/index' import { IEditorConfig } from '../config/interface' import { IDomEditor } from '../editor/interface' + import { withConfig } from '../editor/plugins/with-config' import { withContent } from '../editor/plugins/with-content' import { withDOM } from '../editor/plugins/with-dom' import { withEmitter } from '../editor/plugins/with-emitter' import { withEventData } from '../editor/plugins/with-event-data' import { withMaxLength } from '../editor/plugins/with-max-length' import { withSelection } from '../editor/plugins/with-selection' + import HoverBar from '../menus/bar/HoverBar' import TextArea from '../text-area/TextArea' + import type { DOMElement } from '../utils/dom' import { promiseResolveThen } from '../utils/util'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/core/src/create/create-editor.ts (5 hunks)
- packages/core/src/parse-html/parse-elem-html.ts (2 hunks)
- packages/list-module/tests/plugin.test.ts (0 hunks)
- packages/video-module/tests/plugin.test.ts (0 hunks)
💤 Files with no reviewable changes (2)
- packages/list-module/tests/plugin.test.ts
- packages/video-module/tests/plugin.test.ts
🧰 Additional context used
🔇 Additional comments (3)
packages/core/src/parse-html/parse-elem-html.ts (2)
11-11: LGTM: Clean import additionThe NodeType import is appropriately grouped with related DOM utilities and is used correctly for type checking.
42-47: Verify handling of deeply nested inline elementsWhile the changes look good, let's verify the handling of deeply nested inline elements to ensure the fix is comprehensive.
✅ Verification successful
Let me gather more specific information about the nested span handling by examining the test files and implementation.
This will help us verify:
- How nested spans are tested
- The actual implementation of span parsing
- Text node handling test coverage
Let me analyze the results to provide a comprehensive verification.
Let me gather one final piece of information to verify the text node handling implementation.
This will help us verify the actual implementation of text node handling and how it integrates with nested elements.
Based on the gathered evidence, I can now provide a final response:
Text node handling in nested spans is properly implemented
The implementation correctly handles text nodes and nested inline elements by:
- Properly detecting text nodes using
nodeType === NodeType.TEXT_NODE- Extracting text content safely with fallback (
textContent || '')- Recursively processing nested elements through
parseTextElemHtmlThe test coverage shows comprehensive testing of:
- Nested span scenarios with various text styles
- Text node content preservation
- Style inheritance in nested structures
The changes in the PR maintain the proper handling of deeply nested inline elements while adding robust text node processing. The recursive nature of
parseTextElemHtmlensures that all levels of nesting are properly handled, and the explicit text node check prevents any loss of content.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases involving nested span elements rg -l 'span.*span' --type ts # Look for existing nested span handling in the codebase ast-grep --pattern 'span[data-w-e-type]'Length of output: 2088
Script:
#!/bin/bash # Check test files for nested span scenarios rg -A 5 "span.*span" packages/core/__tests__ packages/basic-modules/__tests__ --type ts # Look for span parsing implementation ast-grep --pattern 'parseTextElemHtml($_, $_)' # Check for text node handling in tests rg -A 5 "TEXT_NODE" packages/core/__tests__ --type tsLength of output: 11753
Script:
#!/bin/bash # Check the implementation of text node handling in parse-elem-html.ts rg -A 10 "TEXT_NODE" packages/core/src/parse-html/parse-elem-html.ts # Look for the parseTextElemHtml implementation rg -A 15 "function parseTextElemHtml" packages/core/src/parse-html/parse-elem-html.tsLength of output: 484
packages/core/src/create/create-editor.ts (1)
112-112: 🛠️ Refactor suggestionImprove type safety in hoverbar initialization.
Consider using undefined instead of null for better TypeScript idioms and make the hoverbar keys check more explicit.
- let hoverbar: HoverBar | null + let hoverbar: HoverBar | undefined + + const hasHoverbarKeys = (keys: Record<string, unknown>): boolean => { + return Object.keys(keys).length > 0 + } + - if (Object.keys(hoverbarKeys).length > 0) { + if (hasHoverbarKeys(hoverbarKeys)) {Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/core/src/create/helper.ts (1)
41-41: Consider applying trailing commas consistentlyWhile adding trailing commas is good for git diffs, it should be applied consistently across all function signatures in the file.
Consider updating other function signatures (like
isRepeatedCreate) to maintain consistency:function isRepeatedCreate( editor: IDomEditor, attrKey: string, - selector: string | DOMElement + selector: string | DOMElement, ): boolean {Also applies to: 51-51
packages/core/src/text-area/TextArea.ts (1)
Line range hint
54-110: Consider adding error handling to the promise-based initialization.While the initialization logic is solid, the
promiseResolveThencallback could benefit from error handling to gracefully handle initialization failures.Consider wrapping the initialization in a try-catch:
promiseResolveThen(() => { + try { const editor = this.editorInstance const window = DomEditor.getWindow(editor) // ... rest of initialization + } catch (error) { + console.error('TextArea initialization failed:', error) + // Optionally emit an initialization error event + editor?.emit(EditorEvents.ERROR, error) + } })packages/core/src/menus/bar/Toolbar.ts (2)
Line range hint
44-67: Consider enhancing the error message for better debugging.The error handling is good, but we could make the error message more helpful.
Consider this enhancement:
-throw new Error(`Cannot find toolbar DOM by selector '${boxSelector}'`) +throw new Error(`Cannot find toolbar DOM by selector '${boxSelector}'. Please ensure the element exists before initialization.`)
Line range hint
190-236: Consider using custom error types for better error handling.While the error handling is comprehensive, using custom error types would improve error catching and handling.
Consider creating and using custom error types:
class ToolbarError extends Error { constructor(message: string) { super(message); this.name = 'ToolbarError'; } } // Usage: throw new ToolbarError(`Not found menu item factory by key '${key}'`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/core/src/create/create-editor.ts (5 hunks)
- packages/core/src/create/helper.ts (6 hunks)
- packages/core/src/menus/bar/Toolbar.ts (10 hunks)
- packages/core/src/text-area/TextArea.ts (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/create/create-editor.ts
🧰 Additional context used
🔇 Additional comments (12)
packages/core/src/create/helper.ts (4)
11-11: LGTM: Good practice using EditorEvents enumThe addition of
EditorEventsimport improves type safety by replacing string literals for event types.
29-29: LGTM: Consistent use of EditorEvents enumThe change from string literal to
EditorEvents.DESTROYEDimproves type safety and maintainability.
77-77: LGTM: Improved code clarity with bracesAdding braces to the if statement improves code clarity and prevents potential errors.
88-89:⚠️ Potential issueSecurity: Consider sanitizing HTML input
The code directly injects HTML into a div using jQuery, which could be vulnerable to XSS attacks if the input is not trusted.
Consider using a HTML sanitization library before injecting the HTML:
+ import DOMPurify from 'dompurify'; // ... - const $content = $(`<div>${html}</div>`) + const sanitizedHtml = DOMPurify.sanitize(html); + const $content = $(`<div>${sanitizedHtml}</div>`)Likely invalid or redundant comment.
packages/core/src/text-area/TextArea.ts (4)
7-50: LGTM: Well-structured imports and property declarations.The new imports and state management properties enhance the clarity and maintainability of the code. The TypeScript types are properly defined, and the initialization is handled correctly.
Line range hint
163-173: Verify the setTimeout usage in focus/blur handlers.The asynchronous focus/blur event handling could potentially lead to race conditions if multiple state changes occur in quick succession.
Consider adding a flag to track pending callbacks:
+ private pendingFocusBlurCallback: NodeJS.Timeout | null = null; editor.on(EditorEvents.CHANGE, () => { if (this.latestEditorSelection == null && editor.selection != null) { + if (this.pendingFocusBlurCallback) clearTimeout(this.pendingFocusBlurCallback); - setTimeout(() => onFocus && onFocus(editor)) + this.pendingFocusBlurCallback = setTimeout(() => { + onFocus && onFocus(editor); + this.pendingFocusBlurCallback = null; + }) } else if (this.latestEditorSelection != null && editor.selection == null) { // Similar changes for blur... } })
180-189: LGTM: Well-implemented utility methods.The maxLength info and progress bar implementations are robust and include proper null checks and cleanup.
Also applies to: 196-208
🧰 Tools
🪛 Biome
[error] 181-183: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
212-224: Address the performance TODO comment.The TODO raises a valid concern about potential performance bottlenecks with large content updates.
Let's analyze the performance impact:
Would you like help implementing performance benchmarks or exploring optimization strategies for the view updates?
packages/core/src/menus/bar/Toolbar.ts (4)
7-21: LGTM! Import organization is clean and logical.The new imports support the implementation changes and are well organized.
28-36: LGTM! Property declarations are well-structured.Properties are properly typed and initialized with appropriate visibility modifiers.
Line range hint
84-89: LGTM! Proper cleanup implementation.The toolbar cleanup before re-initialization prevents potential DOM-related issues.
129-139: LGTM! Robust divider placement logic.The implementation properly handles all edge cases for divider placement.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/core/src/create/create-editor.ts (5 hunks)
- packages/core/src/create/helper.ts (7 hunks)
🔇 Additional comments (6)
packages/core/src/create/helper.ts (5)
7-7: LGTM! Import changes improve type safetyThe addition of
EditorEventsimport and its usage instead of string literals enhances type safety and maintainability.Also applies to: 11-11
41-41: LGTM! Consistent parameter formattingThe addition of trailing commas follows TypeScript best practices and improves git diff readability.
Also applies to: 51-51
77-77: LGTM! Improved code block clarityThe addition of braces improves code clarity and prevents potential bugs from future modifications.
Let's check for consistency in other similar if statements:
#!/bin/bash # Search for single-line if statements without braces rg "if \(.*\) (?!({|return|throw|continue|break))" --type ts
104-118: 🛠️ Refactor suggestionConsider adding validation and error handling
The new
initializeContentfunction could benefit from additional robustness:
- Add validation for HTML content structure
- Add type checking for malformed JSON content
- Consider logging when falling back to default content
Let's check how content initialization is handled elsewhere:
#!/bin/bash # Search for similar content initialization patterns ast-grep --pattern 'function $_(editor: $_, options: { html?: string, content?: $_[] })'Consider adding these safety checks:
export function initializeContent(editor: IDomEditor, options: { html?: string, content?: Descendant[] }) { // 传入 html ，转换为 JSON content if (options.html != null) { + try { + const doc = new DOMParser().parseFromString(options.html, 'text/html'); + if (doc.body.querySelector('parsererror')) { + console.warn('Invalid HTML provided, falling back to default content'); + return genDefaultContent(); + } return htmlToContent(editor, options.html) + } catch (e) { + console.error('Error parsing HTML:', e); + return genDefaultContent(); + } } // 传入 JSON content if (options.content?.length) { + try { + // Validate content structure + if (!options.content.every(node => 'type' in node)) { + console.warn('Invalid content structure, falling back to default content'); + return genDefaultContent(); + } return options.content + } catch (e) { + console.error('Error validating content:', e); + return genDefaultContent(); + } } + console.info('No content provided, using default content'); // 默认内容 return genDefaultContent() }
29-31: LGTM! Event constant usage improves maintainabilityUsing
EditorEvents.DESTROYEDinstead of string literal improves type safety and maintainability.Let's check if there are any remaining string literals for events that should be replaced:
✅ Verification successful
Let me try a different search pattern to verify the event usage in the codebase.
This should help us:
- Find all event listener registrations
- Specifically identify string literal event names
- See how EditorEvents enum is used across the codebase
Based on the search results, I can now provide a final response:
All editor event usages are consistently using EditorEvents enum
The verification shows that all editor-specific events in the codebase are properly using the
EditorEventsenum (e.g.,DESTROYED,CHANGE,SCROLL, etc.). The string literals found in the search results are for other event types like:
- DOM events (e.g., 'click', 'mousedown')
- Third-party library events (e.g., Uppy's 'upload-success', YJS's 'stack-item-added')
- Custom events for specific features
No inconsistent usage of editor events was found that needs to be converted to use
EditorEvents.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for string literals that might be event names rg -i "\.on\(['\"].*['\"]" --type tsLength of output: 90
Script:
#!/bin/bash # Search for .on() method calls with different patterns rg "\.on\(" --type ts -A 1 # Also search specifically for string event names rg "\.on\(\s*['\"]" --type ts # Look for EditorEvents enum usage ast-grep --pattern 'EditorEvents.$_'Length of output: 18305
packages/core/src/create/create-editor.ts (1)
66-68: Clarify how changes address image rendering issues.The PR aims to fix issues with inline elements within images, but it's unclear how the editor creation changes address this. Please explain:
- How does the new plugin application order affect image rendering?
- What was the root cause of the inline element issues?
- Are there test cases that verify the fix?
Let's check for related image handling code:
Also applies to: 87-88
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.changeset/dry-zebras-tan.md (1)
7-7: Please add an English description of the changes.The current description is in Chinese. To improve accessibility for all contributors, please add an English translation that explains:
- What was the issue (image rendering with nested inline elements)
- What changes were made to fix it
- Any important implementation details
packages/core/src/create/create-editor.ts (1)
Line range hint
125-143: Use EditorEvents constant for event emission.While event handling has been updated to use
EditorEventsconstants, there's an inconsistency in event emission:- promiseResolveThen(() => editor.emit('created')) + promiseResolveThen(() => editor.emit(EditorEvents.CREATED))packages/core/src/config/interface.ts (1)
27-44: LGTM: Well-documented editor eventsThe EditorEvents constant is well-structured and thoroughly documented. The use of 'as const' ensures type safety for the event names.
Consider adding @SInCE tags to the JSDoc to track when these events were introduced:
/** * EditorEvents 包含所有编辑器的生命周期事件。 * + * @since 1.0.0 * @property {string} CREATED - 编辑器创建后触发，用于初始化操作。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- .changeset/dry-zebras-tan.md (1 hunks)
- packages/core/src/config/interface.ts (3 hunks)
- packages/core/src/create/create-editor.ts (4 hunks)
- packages/core/src/create/helper.ts (7 hunks)
- packages/core/src/menus/bar/HoverBar.ts (2 hunks)
- packages/core/src/menus/bar/Toolbar.ts (9 hunks)
- packages/core/src/text-area/TextArea.ts (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/create/helper.ts
- packages/core/src/menus/bar/Toolbar.ts
- packages/core/src/text-area/TextArea.ts
🔇 Additional comments (9)
.changeset/dry-zebras-tan.md (1)
1-4: LGTM on version bumps.The patch version increments are appropriate since these changes fix a bug without introducing breaking changes, following semantic versioning principles.
packages/core/src/create/create-editor.ts (3)
6-39: Well-structured imports and constants!Good improvements in code organization:
- Using typed events through
EditorEvents- Properly structured i18n messages
- Clear separation of configuration concerns
46-68: Clean implementation of plugin composition!The refactoring improves code readability by:
- Separating editor creation from plugin application
- Using functional composition for plugin chain
- Maintaining clear type safety with
ICreateOption
87-92: Enhance documentation for disabled content normalization.While the comment explains the reason for disabling normalization, it would be helpful to:
- Provide specific examples of formats that would break with normalization
- Document any potential side effects on the editor's behavior
- Add test cases demonstrating the need for this change
packages/core/src/config/interface.ts (3)
8-10: LGTM: Clean import structureThe new imports from 'slate' are well-organized and necessary for the type definitions.
46-46: LGTM: Type-safe event type definitionThe EditorEventType is properly defined using TypeScript's type system features.
247-255: Consider adding runtime validation for mutually exclusive propertiesThe types look good, but
contentandhtmlproperties inICreateOptionappear to be mutually exclusive. Consider adding runtime validation to ensure only one is provided.Let's check if there's any validation in the codebase:
Consider adding a type guard or validation function:
export function validateCreateOption(options: ICreateOption): void { if (options.content && options.html) { throw new Error('Cannot provide both content and html properties'); } }packages/core/src/menus/bar/HoverBar.ts (2)
12-12: LGTM! Good practice using typed event constants.The addition of EditorEvents import enhances type safety by replacing string literals with typed constants.
84-84: LGTM! Consistent use of typed event constants.The changes improve type safety and maintainability by using EditorEvents constants instead of string literals.
Let's verify the event constant definitions match the usage:
Also applies to: 89-89, 92-93
✅ Verification successful
Based on the search results, I can now generate the final response since I have found both the event constant definitions and their usage across the codebase.
Event constant definitions match the usage perfectly
The changes are verified to be correct. The EditorEvents interface in
packages/core/src/config/interface.tsdefines all the constants used:
CHANGE: 'change'SCROLL: 'scroll'FULLSCREEN: 'fullscreen'UNFULLSCREEN: 'unFullScreen'These constants are consistently used across multiple files including TextArea.ts, create-editor.ts, and Toolbar.ts, showing a codebase-wide adoption of these typed constants.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify EditorEvents interface contains the used event constants # and that they match the string values being replaced # Search for EditorEvents interface definition echo "Searching for EditorEvents interface definition..." rg -p "interface EditorEvents" -A 10 # Search for any other usage of these events to ensure consistency echo "Checking other event usages..." rg "CHANGE|SCROLL|FULLSCREEN|UNFULLSCREEN" --type tsLength of output: 2923
* style: refactor by new eslint * style: refactor code style * style: abbreviated logic * style: extract the init content function * Create dry-zebras-tan.md * docs: add en notes * style: extract editor event and create option type
Changes Overview
Implementation Approach
Testing Done
Verification Steps
Additional Notes
Checklist
Related Issues
Summary by CodeRabbit
New Features
<span>elements for improved content parsing.TextAreaandToolbarclasses.Bug Fixes
Tests
withVideoplugin.