refactor!: merge image/video as file #249
Conversation
WalkthroughThis pull request makes widespread changes across documentation, demos, and core source files to adjust file, image, and video upload functionality. The sidebar menu in the docs has been updated by removing specific items. Several demo components and associated documentation files have been removed or refactored, and a new Vue component for file upload is introduced. In the editor’s source code, type safety and configuration options have been improved, including a refactored file uploader and modifications to CSS and theme styles. The test selectors have also been updated to reflect structural changes. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant E as Editor
participant FU as FileUploader
U->>E: Initiate file upload
E->>FU: upload(range, files)
loop For each file
FU->>FU: validateFile(file)
end
alt Validation passes
FU->>E: Return file URL(s)
E->>U: Display file(s) in editor
else Validation fails
FU->>E: Invoke fail callback
E->>U: Show error notification
end
Suggested reviewers
Poem
🪧 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: 4
🧹 Nitpick comments (8)
packages/docs/fluent-editor/demos/file-upload-handle.vue (1)
47-49: Debug button should be removed or renamed.The "awgawg" button appears to be for debugging purposes. Consider removing it or renaming it to something more descriptive like "Log Contents" if it's intended for demonstration purposes.
- <button @click="ww">awgawg</button> + <button @click="ww">Log Contents</button>packages/fluent-editor/src/modules/custom-image/image.ts (2)
17-18: Consider avoidingsuperin a static context for clarity.Using
superwithin a static method can be confusing. Consider using the parent class name, for example:-static create(value: ImageValue) { - const node = super.create(value) as HTMLElement +static create(value: ImageValue) { + const node = Embed.create(value) as HTMLElement🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
50-50: Be mindful of deprecated API usage.
document.execCommand('enableObjectResizing', ...)is deprecated in modern browsers. Consider researching alternatives for disabling image object resizing cross-browser.packages/docs/fluent-editor/docs/demo/file-upload.md (1)
10-20: Minor spelling check and clarity.The text references “hanler” instead of “handler.” Consider correcting the spelling. Updated excerpt:
-通过 hanler 回调函数模拟 local 图片上传 +通过 handler 回调函数模拟 local 图片上传packages/fluent-editor/src/modules/custom-uploader.ts (1)
31-41: Default option resolution.Well-structured fallback logic. Consider adding runtime checks or logs if an unusual set of options is passed, for added developer clarity.
packages/fluent-editor/src/modules/file/formats/file.ts (3)
19-21: Consider replacingthiswith class name in static methods.Using
thisin static methods can be confusing as it refers to the class itself, not an instance. Consider using the class name directly for clarity.- static create(value: FileValue) { - const node = super.create(value) as HTMLAnchorElement - //... - node.setAttribute('contenteditable', 'false') - //... - const src = this.sanitize(value.src) + static create(value: FileValue) { + const node = File.create(value) as HTMLAnchorElement + //... + node.setAttribute('contenteditable', 'false') + //... + const src = File.sanitize(value.src)Also applies to: 24-24, 27-27
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
40-42: Improved type safety in value method.Adding explicit parameter type improves clarity and type safety. However, using
thisin a static context should be replaced with the class name.static value(domNode: HTMLAnchorElement) { - return this.getFormats(domNode) + return File.getFormats(domNode) }🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
44-46: Enhanced type safety with explicit return type.Adding the explicit
Record<string, string>type for formats improves type safety. However, the same static method issue applies here.static getFormats(domNode: HTMLAnchorElement) { const formats: Record<string, string> = {} - const href = this.sanitize(domNode.href) + const href = File.sanitize(domNode.href) // ... }🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
packages/docs/fluent-editor/.vitepress/sidebar.ts(0 hunks)packages/docs/fluent-editor/demos/file-upload-handle.vue(1 hunks)packages/docs/fluent-editor/demos/file-upload-multi.vue(0 hunks)packages/docs/fluent-editor/demos/file-upload-to-server.vue(0 hunks)packages/docs/fluent-editor/demos/file-upload.spec.ts(1 hunks)packages/docs/fluent-editor/demos/file-upload.vue(1 hunks)packages/docs/fluent-editor/demos/image-upload-before-upload.vue(0 hunks)packages/docs/fluent-editor/demos/image-upload-multi.vue(0 hunks)packages/docs/fluent-editor/demos/image-upload-option.vue(0 hunks)packages/docs/fluent-editor/demos/image-upload-to-server.vue(0 hunks)packages/docs/fluent-editor/demos/image-upload.vue(0 hunks)packages/docs/fluent-editor/demos/video.vue(0 hunks)packages/docs/fluent-editor/docs/demo/file-upload.md(1 hunks)packages/docs/fluent-editor/docs/demo/image-upload.md(0 hunks)packages/docs/fluent-editor/docs/demo/video.md(0 hunks)packages/docs/package.json(1 hunks)packages/fluent-editor/src/assets/editor.scss(0 hunks)packages/fluent-editor/src/assets/style.scss(1 hunks)packages/fluent-editor/src/assets/toolbar.scss(0 hunks)packages/fluent-editor/src/assets/video.scss(1 hunks)packages/fluent-editor/src/config/index.ts(1 hunks)packages/fluent-editor/src/config/types/editor-config.interface.ts(0 hunks)packages/fluent-editor/src/config/types/editor-modules.interface.ts(2 hunks)packages/fluent-editor/src/core/fluent-editor.ts(1 hunks)packages/fluent-editor/src/fluent-editor.ts(2 hunks)packages/fluent-editor/src/formats/video.ts(1 hunks)packages/fluent-editor/src/modules/custom-clipboard.ts(2 hunks)packages/fluent-editor/src/modules/custom-image/BlotFormatter.ts(1 hunks)packages/fluent-editor/src/modules/custom-image/image.ts(5 hunks)packages/fluent-editor/src/modules/custom-uploader.ts(1 hunks)packages/fluent-editor/src/modules/file/formats/file.ts(3 hunks)packages/fluent-editor/src/modules/file/index.ts(1 hunks)packages/fluent-editor/src/themes/snow.ts(1 hunks)packages/fluent-editor/src/utils/is.ts(1 hunks)
💤 Files with no reviewable changes (14)
- packages/docs/fluent-editor/demos/file-upload-to-server.vue
- packages/docs/fluent-editor/demos/video.vue
- packages/docs/fluent-editor/demos/image-upload-before-upload.vue
- packages/docs/fluent-editor/demos/image-upload-option.vue
- packages/docs/fluent-editor/.vitepress/sidebar.ts
- packages/fluent-editor/src/assets/editor.scss
- packages/fluent-editor/src/assets/toolbar.scss
- packages/docs/fluent-editor/demos/image-upload.vue
- packages/docs/fluent-editor/demos/file-upload-multi.vue
- packages/docs/fluent-editor/demos/image-upload-multi.vue
- packages/fluent-editor/src/config/types/editor-config.interface.ts
- packages/docs/fluent-editor/docs/demo/video.md
- packages/docs/fluent-editor/demos/image-upload-to-server.vue
- packages/docs/fluent-editor/docs/demo/image-upload.md
🧰 Additional context used
🪛 GitHub Check: lint
packages/docs/fluent-editor/demos/file-upload-handle.vue
[failure] 42-42:
Unexpected console statement. Only these console methods are allowed: warn, error
[failure] 33-33:
Unexpected console statement. Only these console methods are allowed: warn, error
[failure] 29-29:
Unexpected console statement. Only these console methods are allowed: warn, error
packages/fluent-editor/src/modules/custom-uploader.ts
[failure] 63-63:
Unexpected console statement. Only these console methods are allowed: warn, error
🪛 ESLint
packages/docs/fluent-editor/demos/file-upload-handle.vue
[error] 29-29: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
[error] 33-33: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
[error] 34-34: Extra space before value for key 'image'.
(style/key-spacing)
[error] 34-34: A space is required before '}'.
(style/object-curly-spacing)
[error] 34-34: A space is required before '}'.
(object-curly-spacing)
[error] 41-41: Top-level functions should be declared with function keyword
(antfu/top-level-function)
[error] 42-42: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
packages/fluent-editor/src/modules/custom-uploader.ts
[error] 63-63: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
🪛 GitHub Actions: autofix.ci
packages/docs/fluent-editor/demos/file-upload-handle.vue
[error] 29-29: ESLint: Unexpected console statement. Only these console methods are allowed: warn, error (no-console)
🪛 Biome (1.9.4)
packages/fluent-editor/src/modules/custom-image/image.ts
[error] 18-18: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
packages/fluent-editor/src/modules/file/formats/file.ts
[error] 20-20: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 27-27: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (51)
packages/fluent-editor/src/assets/video.scss (1)
1-5: Good styling for responsive video elementsThe
.ql-videoclass is well-designed with responsive width (100%) and a reasonable max-height constraint (400px). The automatic horizontal centering ensures videos are properly aligned within the editor.packages/docs/package.json (1)
13-13: Enhances developer experience with visual test debuggingAdding the
test:uiscript provides a valuable UI-based way to run Playwright tests, which will be helpful for debugging the refactored file upload functionality.packages/fluent-editor/src/assets/style.scss (1)
1-1: Properly integrates video styles with main stylesheetThe import of video styles at the top level ensures video elements are consistently styled throughout the editor. This aligns well with the PR's objective to unify image and video handling.
packages/fluent-editor/src/config/types/editor-modules.interface.ts (2)
1-1: Good import for new FileUploaderOptions typeThe import of
FileUploaderOptionsfrom the custom-uploader module provides the necessary type information for the new uploader configuration.
42-42: Well-defined type for flexible uploader configurationThe new
uploaderproperty provides good flexibility by accepting either a boolean or a partialFileUploaderOptionsobject. This enhances configurability while maintaining type safety, which aligns perfectly with the PR's objective to unify image and video upload handling.packages/fluent-editor/src/utils/is.ts (1)
8-9: Good addition of type checking utilitiesThe new utility functions
isBooleanandisArrayare well-implemented and consistent with the existing type checking functions. The type predicate onisBooleanhelps with TypeScript's type narrowing, and usingArray.isArraydirectly is best practice.packages/fluent-editor/src/core/fluent-editor.ts (1)
3-3: LGTM - Appropriate type importThe import of the
FileUploadertype is appropriate for the changes being made to theFluentEditorclass.packages/docs/fluent-editor/demos/file-upload.vue (4)
2-2: Good Vue 3 composition API usageProperly importing and using
reffrom Vue's composition API for creating a reference to the editor element.Also applies to: 5-5
17-19: Improved editor instantiation with null checkThe changes properly handle SSR compatibility and include a null check before instantiating the editor, which is a good defensive programming practice.
30-30: Properly referenced DOM elementThe template correctly references the editor container using
ref="editorRef", which aligns with the TypeScript changes above.
12-12:❓ Verification inconclusive
Verify toolbar configuration compatibility
The toolbar now includes 'file', 'image', and 'video' buttons. Ensure that the
FluentEditorcomponent properly recognizes all these options after the refactoring.
🏁 Script executed:
#!/bin/bash # Check if the editor correctly registers the file, image, and video toolbar handlers rg -A 3 -B 3 "toolbar:.+(file|image|video)" --type ts packages/fluent-editor/src/Length of output: 83
Action Required: Manual Verification of Toolbar Registration
The automated search did not yield any evidence that the FluentEditor component registers the 'file', 'image', and 'video' toolbar options. Please verify manually that after refactoring, the FluentEditor component correctly recognizes and handles these toolbar options. In particular, check that:
- The toolbar configuration in the demo (line 12 in
packages/docs/fluent-editor/demos/file-upload.vue) is propagated to the component.- The registration logic within the FluentEditor source code (likely in files under
packages/fluent-editor/src/) correctly maps these toolbar options without any missing references.Once confirmed, please update the review comment accordingly.
packages/fluent-editor/src/formats/video.ts (2)
59-59: Good addition of blob protocol supportAdding 'blob' to the
PROTOCOL_WHITELISTallows for handling locally generated video URLs, which is essential for preview functionality when uploading videos.
60-60:❓ Verification inconclusive
Class name change requires style verification
Changing the class name from 'ql-video-item' to 'ql-video' is a breaking change that could affect styling. Ensure all corresponding CSS selectors have been updated.
🏁 Script executed:
#!/bin/bash # Check for any CSS or SCSS files still using the old class name rg "ql-video-item" --type css --type scss packages/ # Check if the new class name has proper styling rg "ql-video(\s|[^-])" --type css --type scss packages/Length of output: 167
#!/bin/bash # Re-run search for the old class name "ql-video-item" in CSS and SCSS files rg -g '*.css' -g '*.scss' "ql-video-item" packages/ # Re-run search for the new class name "ql-video" in CSS and SCSS files rg -g '*.css' -g '*.scss' "ql-video\\b" packages/Action: Verify CSS/SCSS Styling Consistency
The class name change in
packages/fluent-editor/src/formats/video.tsfrom"ql-video-item"to"ql-video"may break existing styles if all references to the old class haven’t been updated.
- Please re-run the above commands to confirm that no CSS or SCSS files still reference
"ql-video-item".- Verify that the new
"ql-video"class has proper styling defined.packages/fluent-editor/src/fluent-editor.ts (2)
11-11: Module import update aligned with refactoring goal.The import has been updated to use the new
FileUploaderclass from the custom-uploader module, which aligns with the PR objective to merge image/video uploading as a unified file upload capability.
63-63: Module registration update consistent with import change.The registration of the uploader module has been correctly updated to use the new
FileUploaderclass, maintaining consistency with the import change on line 11.packages/docs/fluent-editor/demos/file-upload.spec.ts (4)
10-10: Test URL updated to match new demo endpoint.The test URL has been correctly updated to point to the new unified file-upload demo instead of the previous image-upload demo, which is consistent with the refactoring approach.
15-15: Selector updated to match new component structure.The selector has been updated from
#editorto.ql-editor.first().getByRole('img')to correctly target image elements in the new component structure.
18-18: Image element click selector updated.The selector for clicking on image elements has been correctly updated to use the class-based selector instead of ID-based selector.
23-23: Image element reference updated for consistency.The image element reference has been updated to maintain consistency with the previous selector changes, ensuring that the test properly interacts with elements in the new structure.
packages/docs/fluent-editor/demos/file-upload-handle.vue (1)
25-37:❓ Verification inconclusive
Uploader configuration focuses on image files only.
While the PR goal is to unify image/video as file uploads, this demo is configured to only accept image files with
mimetypes: ['image/*']. This may be intentional for this specific demo but consider adding a more comprehensive example that demonstrates the full capability of the unified file uploader with both image and video handling.
🏁 Script executed:
#!/bin/bash # Check if there are other examples showing different file types handling fd -g "*file-upload*.vue" --type fileLength of output: 146
Clarification on Demo Scope: Image-Only vs. Unified Upload
In
packages/docs/fluent-editor/demos/file-upload-handle.vue, the uploader is currently configured to accept only images (mimetypes: ['image/*']). Considering the PR aims to support a unified image/video uploader, please confirm one of the following:
- If this demo is intentionally focused on image uploads, add a note or cross-reference to the alternate demo (
file-upload.vue) that may demonstrate both image and video handling.- Otherwise, update this demo to include video support so that it fully reflects the unified upload capability.
🧰 Tools
🪛 GitHub Check: lint
[failure] 33-33:
Unexpected console statement. Only these console methods are allowed: warn, error
[failure] 29-29:
Unexpected console statement. Only these console methods are allowed: warn, error🪛 ESLint
[error] 29-29: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
[error] 33-33: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
[error] 34-34: Extra space before value for key 'image'.
(style/key-spacing)
[error] 34-34: A space is required before '}'.
(style/object-curly-spacing)
[error] 34-34: A space is required before '}'.
(object-curly-spacing)
🪛 GitHub Actions: autofix.ci
[error] 29-29: ESLint: Unexpected console statement. Only these console methods are allowed: warn, error (no-console)
packages/fluent-editor/src/modules/custom-image/BlotFormatter.ts (2)
25-28:❓ Verification inconclusive
Registration method now only registers ImageBlot.
The
registermethod no longer registersimage-containerformat with Quill, which is consistent with the removal of theImageContainerBlotimport. Ensure that this doesn't break any existing functionality that might depend on the image-container format.
🏁 Script executed:
#!/bin/bash # Search for any code that might be using 'image-container' format rg -A 2 "image-container"Length of output: 25
Action Required: Manual Verification for Image-Container Dependency
The updated registration now only registers
ImageBlotandCustomImageSpec, and the code no longer includes the registration forimage-container, which aligns with the removal ofImageContainerBlot. Although an automated search usingrgdid not produce any references toimage-container, the low-quality output suggests that a manual verification is still advised to ensure no legacy functionality relies on the removed registration.
- Confirm in the application that no UI or module depends on the
image-containerformat.- Verify that any expected functionality is intact after the removal of
ImageContainerBlot.
6-6:❓ Verification inconclusive
ImageContainerBlot import removed to align with unified file handling.
The import has been updated to only import
ImageBlotand no longer importsImageContainerBlot. This change is consistent with the PR objective to simplify and unify file handling.Please verify that all references to
ImageContainerBlothave been properly removed or updated elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash # Search for any remaining references to ImageContainerBlot that might need updating rg -A 2 "ImageContainerBlot"Length of output: 28
Unified File Handling: Confirm Removal of
ImageContainerBlotReferencesThe import in
packages/fluent-editor/src/modules/custom-image/BlotFormatter.ts(Line 6) has been updated to import onlyImageBlot—in line with the PR’s goal to simplify and unify file handling. An automated search forImageContainerBlotproduced no results; however, please manually verify that no lingering references remain in the codebase.- import ImageBlot, { ImageContainerBlot } from './image'; + import ImageBlot from './image';packages/fluent-editor/src/config/index.ts (4)
7-8: Improved type safety and early validation.The function now properly types the parameters and includes an early return for empty accept arrays, which prevents unnecessary DOM operations.
12-12: Hidden file input improves UI.Setting the file input to be hidden by default is a good practice as these inputs should generally be triggered programmatically rather than directly interacted with by users.
15-16: Simplified MIME type handling and enabled multiple uploads by default.The code now handles wildcard MIME types more elegantly and enables multiple file uploads by default, aligning with the PR objective to allow multiple files to be uploaded simultaneously.
19-19: Unified upload handling.The upload logic has been simplified by removing conditional checks based on file type, creating a more consistent approach to file uploads.
packages/fluent-editor/src/themes/snow.ts (3)
45-46: Centralized file type handling.The file handler now correctly retrieves accepted file types from the centralized uploader options, rather than from separate config options, improving consistency.
49-53: Improved image type filtering logic.The image handler now properly filters accepted MIME types for images, with special handling for wildcard types. This approach is more maintainable and consistent.
55-61: Added video handler with consistent filtering approach.The new video handler follows the same pattern as the image handler, providing a consistent user experience across different media types.
packages/fluent-editor/src/modules/custom-clipboard.ts (2)
240-240: Enhanced type safety with proper parameter typing.Adding the
File[]type annotation for the files parameter improves code clarity and helps catch type-related errors early.
254-258:✅ Verification successful
Simplified upload logic but potential functionality loss.
The code now uses a simplified approach by removing conditional handling based on upload options, but this has eliminated the logic for handling placeholder or cross-origin images that previously existed.
Verify that this functionality is either no longer needed or is handled elsewhere in the codebase.
🏁 Script executed:
#!/bin/bash # Search for handling of placeholder or cross-origin images elsewhere in the codebase echo "Searching for cross-origin image handling:" rg "cross-origin|placeholder.*image" --type tsLength of output: 659
Simplified upload branch is acceptable; placeholder/cross-origin handling is retained elsewhere.
The updated logic now directly calls
this.quill.uploader.uploadafter obtaining the image selection. Our search confirms that placeholder (and likely cross-origin) image handling is still processed via functions likeflipFilesArrayandfiles2urlsin the same file. Provided these helper routines correctly manage such cases, the removal of the inline conditional handling in this branch appears intentional. Please verify that this design aligns with your overall strategy.packages/fluent-editor/src/modules/file/index.ts (3)
1-1: Simplified class structure and improved imports.The FileModule no longer extends from Quill's module class, making it a standalone class with its own implementation. This aligns with the PR objective to refactor the uploader module for a more unified approach.
Also applies to: 6-6
13-16: Enhanced constructor with proper typing.The constructor now properly types the quill parameter as FluentEditor and makes it public, improving code readability and type safety.
18-21: Improved event handling with proper type annotations.Adding type annotations for the event parameter and target element enhances code clarity and helps prevent potential runtime errors related to incorrect property access.
packages/fluent-editor/src/modules/custom-image/image.ts (5)
1-5: No issues found with typed import usage.
8-8: Clarity on the new union type.This union type improves flexibility by allowing a bare string or an object with a
srcproperty.
28-28: Attribute assignment looks good.Setting a string value for custom data attributes is correct. No issues found here.
101-101: Removal of the named export “CustomImageContainer.”You have removed the secondary export. If any external consumers rely on
CustomImageContainer, this is a breaking change. Ensure you coordinate or document the removal accordingly.
56-56:❓ Verification inconclusive
Double-check the
blob:protocol handling.Ensure that
'blob:'is correctly interpreted bysanitize. If needed, add the exact scheme'blob:'rather than'blob'. Please verify:
🏁 Script executed:
#!/bin/bash # Searching for references to sanitize usage to confirm the presence of 'blob:' or 'blob' rg -A 5 "sanitize\(url, \['http', 'https', 'blob'"Length of output: 595
Blob Protocol Verification Needed
The allowed protocols array in
packages/fluent-editor/src/modules/custom-image/image.tscurrently lists'blob'(without a colon):return sanitize(url, ['http', 'https', 'blob', 'data']) ? url : '//:0'Please verify that the
sanitizefunction interprets this correctly. If the function requires the exact scheme format (i.e.,'blob:'), then update the allowed protocols list accordingly.packages/docs/fluent-editor/docs/demo/file-upload.md (2)
5-5: Clearer explanation of default handling.Documenting that images and videos are processed differently from other file types clarifies the editor’s default upload behavior.
23-34: Thorough and helpful options documentation.The new configuration table is well-structured, providing clarity on file handling constraints (
mimetypes,maxSize) and callback behaviors (handler,success,fail). Good job.packages/fluent-editor/src/modules/custom-uploader.ts (6)
1-9: Imports and type definitions.No issues found. The typed imports and initialization for
UploaderandDeltafollow Quill’s recommended extension approach.
10-13: InterfaceUploaderOptionslogic.This interface is concise and appears consistent with Quill’s extension pattern.
14-20: Expanded file uploader options.The additional properties (
maxSize,success,fail) enhance flexibility. Good use of promise-returning handlers for asynchronous uploads.
21-29: Constructor usage and overriding Quill’s uploader.The approach of extending Quill’s
Uploaderto unify editor and custom logic is sound. Type casting withas anyis typical due to Quill’s bundling.
43-45: Effective file validation.Allowing wildcard matches with
replaceAll('*', '.*')is a neat approach. Ensure all supported browsers or Node versions allowString.replaceAll.
47-50: Selecting which files to upload.Filtering valid files before calling
handlerhelps avoid needless overhead. No issues found.packages/fluent-editor/src/modules/file/formats/file.ts (4)
1-1: Improved type safety with explicit import.The change from using
Quill.importstoQuill.importwith proper TypeScript type assertion improves type safety and makes the code more maintainable.Also applies to: 5-5
8-12: Good addition of explicit FileValue interface.Creating this explicit interface for file values improves type safety and makes the API contract clearer. This also helps with the PR's goal of unifying file, image, and video uploads by standardizing the expected input structure.
14-17: Modernized static property initialization.Using class field syntax for static properties is more modern and readable than assigning them after class declaration.
58-60: Improved type safety in sanitize method.Adding the explicit string parameter type is a good improvement for clarity and type safety.
🧰 Tools
🪛 Biome (1.9.4)
[error] 59-59: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
packages/docs/fluent-editor/demos/file-upload-handle.vue (1)
29-29: Consider commenting the demo file upload handler logic.The current implementation alternates between returning false and a URL based on the file index. While this is appropriate for a demo, adding a comment explaining this pattern would improve clarity.
- return files.map((_, i) => i % 2 === 0 ? false : 'https://developer.mozilla.org/static/media/chrome.5e791c51c323fbb93c31.svg') + // Demo: Alternate between failing uploads (false) and successful uploads (URL) based on index + return files.map((_, i) => i % 2 === 0 ? false : 'https://developer.mozilla.org/static/media/chrome.5e791c51c323fbb93c31.svg')packages/fluent-editor/src/modules/file/index.ts (1)
13-16: Enhanced constructor with TypeScript parameter property.Using parameter property syntax for the quill instance improves code conciseness while maintaining type safety. However, since line 14 reassigns the quill property after it's already defined in the constructor parameter, it's redundant.
constructor(public quill: FluentEditor) { - this.quill = quill quill.root.addEventListener('click', event => this.clickEvent(event), false) }packages/fluent-editor/src/modules/custom-image/image.ts (1)
17-18: Update TypeScript method signature but static context issueThe method signature has been improved with proper TypeScript typing, but using
superin a static context can be confusing.static create(value: ImageValue) { - const node = super.create(value) as HTMLElement + const node = Embed.create(value) as HTMLElement🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/fluent-editor/src/modules/custom-uploader.ts (4)
21-29: Good implementation of the FileUploader class constructorThe constructor properly initializes options and extends the base Uploader class, though the
as anytype assertion might be improved with a more specific type.- super(quill, options as any) + super(quill, options as Partial<UploaderOptions>)
43-45: File validation could be more robustThe validation method checks MIME types and file size, but it compresses the logic into a single line which might be harder to maintain or debug.
- validateFile(file: File) { - return this.options.mimetypes.some(type => (file.type || 'text/plain').match(type.replaceAll('*', '.*'))) && file.size < this.options.maxSize + validateFile(file: File) { + const isValidType = this.options.mimetypes.some(type => + (file.type || 'text/plain').match(type.replaceAll('*', '.*')) + ); + const isValidSize = file.size < this.options.maxSize; + return isValidType && isValidSize; + }
47-50: Streamlined file URL resolutionThe method efficiently filters valid files and delegates to the handler, though it could handle errors more explicitly.
async getFileUrls(files: File[], range: Range) { const uploads = files.filter(file => this.validateFile(file)) - return this.options.handler.call(this, range, uploads) + try { + return this.options.handler.call(this, range, uploads) + } catch (error) { + console.warn('Error getting file URLs:', error) + return uploads.map(() => false) + } }
52-95: Complex upload method with comprehensive handlingThe
uploadmethod comprehensively handles file validation, Delta updates, and callbacks, but its complexity could make it harder to maintain.Consider breaking this method into smaller, focused helper methods:
async upload(range: Range, files: FileList | File[]) { + const { validFiles, invalidFiles } = this.categorizeFiles(files); + const results = await this.processFiles(range, validFiles); + this.updateEditor(range, validFiles, results); + this.handleFailures(invalidFiles, range); + this.triggerCallbacks(files, results, range); + } + + private categorizeFiles(files: FileList | File[]) { const uploads = [] const fails = [] for (const file of Array.from(files)) { if (this.validateFile(file)) { uploads.push(file) } else { fails.push(file) } } + return { validFiles: uploads, invalidFiles: fails }; + } + + private async processFiles(range: Range, files: File[]) { + return this.options.handler.call(this, range, files); + } + + private updateEditor(range: Range, files: File[], results: (string | false)[]) { const updateDelta = results.reduce((delta, url, i) => { if (isString(url)) { const type = files[i].type if (type.startsWith('image/')) { delta.insert({ image: url }) } else if (type.startsWith('video/')) { delta.insert({ video: { src: url } }) } else { delta.insert({ file: { size: files[i].size, title: files[i].name, src: url } }) } } else { delta.insert('\n') } return delta }, new Delta().retain(range.index).delete(range.length)) this.quill.updateContents(updateDelta, Quill.sources.USER) this.quill.setSelection(range.index + results.length, Quill.sources.SILENT) + } + + private handleFailures(files: File[], range: Range) { for (const file of files) { this.options.fail.call(this, file, range) } + } + + private triggerCallbacks(allFiles: FileList | File[], results: (string | false)[], range: Range) { for (const [i, res] of results.entries()) { if (isString(res)) { this.options.success.call(this, allFiles[i], { index: range.index + i, length: 0 }) } else { this.options.fail.call(this, allFiles[i], { index: range.index + i, length: 0 }) } } }packages/fluent-editor/src/modules/file/formats/file.ts (4)
19-21: Type safety improvement but static context issueSimilar to the image.ts file, using
superin a static context can be confusing.static create(value: FileValue) { - const node = super.create(value) as HTMLAnchorElement + const node = Embed.create(value) as HTMLAnchorElement🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
27-27: Using this in static contextUsing
thisin a static context can be confusing. Consider using the class name instead.- const src = this.sanitize(value.src) + const src = File.sanitize(value.src)🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
40-42: Using this in static contextThe same issue with using
thisin a static context. Consider using the class name instead.static value(domNode: HTMLAnchorElement) { - return this.getFormats(domNode) + return File.getFormats(domNode) }🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
44-46: Improved type safety with Record typeThe use of
Record<string, string>provides better type safety for the formats object, but there's the same issue with usingthisin a static context.static getFormats(domNode: HTMLAnchorElement) { const formats: Record<string, string> = {} - const href = this.sanitize(domNode.href) + const href = File.sanitize(domNode.href)🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
packages/docs/fluent-editor/.vitepress/sidebar.ts(0 hunks)packages/docs/fluent-editor/demos/file-upload-handle.vue(1 hunks)packages/docs/fluent-editor/demos/file-upload-multi.vue(0 hunks)packages/docs/fluent-editor/demos/file-upload-to-server.vue(0 hunks)packages/docs/fluent-editor/demos/file-upload.spec.ts(1 hunks)packages/docs/fluent-editor/demos/file-upload.vue(1 hunks)packages/docs/fluent-editor/demos/image-upload-before-upload.vue(0 hunks)packages/docs/fluent-editor/demos/image-upload-multi.vue(0 hunks)packages/docs/fluent-editor/demos/image-upload-option.vue(0 hunks)packages/docs/fluent-editor/demos/image-upload-to-server.vue(0 hunks)packages/docs/fluent-editor/demos/image-upload.vue(0 hunks)packages/docs/fluent-editor/demos/video.vue(0 hunks)packages/docs/fluent-editor/docs/demo/file-upload.md(1 hunks)packages/docs/fluent-editor/docs/demo/image-upload.md(0 hunks)packages/docs/fluent-editor/docs/demo/video.md(0 hunks)packages/docs/package.json(1 hunks)packages/fluent-editor/src/assets/editor.scss(0 hunks)packages/fluent-editor/src/assets/style.scss(1 hunks)packages/fluent-editor/src/assets/toolbar.scss(0 hunks)packages/fluent-editor/src/assets/video.scss(1 hunks)packages/fluent-editor/src/config/index.ts(1 hunks)packages/fluent-editor/src/config/types/editor-config.interface.ts(0 hunks)packages/fluent-editor/src/config/types/editor-modules.interface.ts(2 hunks)packages/fluent-editor/src/core/fluent-editor.ts(1 hunks)packages/fluent-editor/src/fluent-editor.ts(2 hunks)packages/fluent-editor/src/formats/video.ts(1 hunks)packages/fluent-editor/src/modules/custom-clipboard.ts(2 hunks)packages/fluent-editor/src/modules/custom-image/BlotFormatter.ts(1 hunks)packages/fluent-editor/src/modules/custom-image/image.ts(5 hunks)packages/fluent-editor/src/modules/custom-uploader.ts(1 hunks)packages/fluent-editor/src/modules/file/formats/file.ts(3 hunks)packages/fluent-editor/src/modules/file/index.ts(1 hunks)packages/fluent-editor/src/themes/snow.ts(1 hunks)packages/fluent-editor/src/utils/is.ts(1 hunks)
💤 Files with no reviewable changes (14)
- packages/docs/fluent-editor/docs/demo/video.md
- packages/docs/fluent-editor/demos/file-upload-to-server.vue
- packages/fluent-editor/src/assets/editor.scss
- packages/docs/fluent-editor/demos/video.vue
- packages/docs/fluent-editor/demos/image-upload.vue
- packages/docs/fluent-editor/demos/image-upload-before-upload.vue
- packages/docs/fluent-editor/docs/demo/image-upload.md
- packages/docs/fluent-editor/demos/image-upload-multi.vue
- packages/docs/fluent-editor/.vitepress/sidebar.ts
- packages/fluent-editor/src/config/types/editor-config.interface.ts
- packages/docs/fluent-editor/demos/file-upload-multi.vue
- packages/fluent-editor/src/assets/toolbar.scss
- packages/docs/fluent-editor/demos/image-upload-option.vue
- packages/docs/fluent-editor/demos/image-upload-to-server.vue
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/fluent-editor/src/assets/video.scss
- packages/fluent-editor/src/assets/style.scss
- packages/fluent-editor/src/utils/is.ts
- packages/docs/fluent-editor/demos/file-upload.vue
- packages/docs/fluent-editor/demos/file-upload.spec.ts
- packages/fluent-editor/src/core/fluent-editor.ts
- packages/fluent-editor/src/config/types/editor-modules.interface.ts
- packages/fluent-editor/src/formats/video.ts
- packages/fluent-editor/src/fluent-editor.ts
- packages/fluent-editor/src/modules/custom-clipboard.ts
- packages/fluent-editor/src/modules/custom-image/BlotFormatter.ts
- packages/docs/package.json
- packages/fluent-editor/src/themes/snow.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/fluent-editor/src/modules/custom-image/image.ts
[error] 18-18: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
packages/fluent-editor/src/modules/file/formats/file.ts
[error] 20-20: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 27-27: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (23)
packages/fluent-editor/src/config/index.ts (3)
7-8: Strong type safety improvement with early validation.The function now includes proper type annotations for parameters, and the early return prevents unnecessary processing when the accept array is empty.
12-17: Good refactoring of file input configuration.The code now:
- Hides the file input by default
- Maps wildcard MIME types correctly
- Always enables multiple file selection
This implementation aligns well with the PR objective to merge image/video handling into a unified file uploader.
19-19: Simplified upload method call.The upload method call has been streamlined by removing conditional logic based on the type parameter, which supports the unified file handling approach.
packages/docs/fluent-editor/demos/file-upload-handle.vue (1)
1-39: Well-structured file upload demo implementation.The component correctly implements the new unified file upload approach with:
- Proper TypeScript type annotations
- Clear organization of the editor configuration
- A demonstration of the handler and fail callback functionality
The demo effectively shows how to configure the uploader to accept only specific file types and handle successful/failed uploads differently.
packages/fluent-editor/src/modules/file/index.ts (2)
1-6: Improved module imports and class structure.The class now correctly imports FluentEditor type and no longer extends from another class, which simplifies the implementation. This cleaner approach aligns with modern TypeScript practices.
18-21: Improved type safety in event handling.Adding type annotations for the event parameter and casting the target to HTMLElement improves type safety and helps prevent potential runtime errors.
packages/docs/fluent-editor/docs/demo/file-upload.md (4)
5-5: Clear description of default behavior.The updated description effectively explains how different file types are handled by default, which helps users understand the core functionality.
10-21: Improved section title and comprehensive example explanation.The section title now better reflects its content, focusing on server-side upload rather than multi-file upload. The detailed explanation of the example behavior helps users understand what to expect when trying the demo.
23-32: Excellent documentation of configuration options.The new options table provides clear, comprehensive documentation of all configuration properties available for the uploader. This is essential information for developers implementing the component.
33-34: Important notes about error handling behavior.These notes provide crucial information about how files are filtered and how error handling works in different scenarios. This helps prevent confusion when implementing custom upload handlers.
packages/fluent-editor/src/modules/custom-image/image.ts (6)
1-5: Type safety improvements with proper importsThe import has been updated to use
Quill.importinstead ofQuill.importswith proper type assertion, which enhances type safety throughout the component.
8-8: Good addition of a type definition for image valuesThe
ImageValuetype provides better type safety by clearly defining the expected structure for image values, either as a string or an object with asrcproperty.
28-28: Attribute value changed from boolean to stringThe attribute value for
devui-editorx-imagehas been changed from a boolean to a string. This is consistent with HTML attribute best practices, as HTML attributes are always stored as strings.
50-50: Command parameter updated to stringThe
document.execCommandnow correctly receives a string parameter instead of a boolean, which aligns with the expected parameter type for this browser API.
55-56: Added 'blob' protocol to whitelistAdding 'blob' to the list of allowed URL schemes allows working with Blob URLs, which is essential for handling file uploads from memory or temporary storage.
101-101:✅ Verification successful
Removed ImageContainerBlot export
The export statement has been simplified to only export the
CustomImageclass. Make sure this change doesn't break any code that was using theImageContainerBlot.
🏁 Script executed:
#!/bin/bash # Check if there are any imports of ImageContainerBlot in the codebase rg "ImageContainerBlot" --type tsLength of output: 33
Here's an updated review comment recommending an additional verification step to ensure that no files reference the removed export:
Action: Verify No Residual Usage of ImageContainerBlot
The export statement in
packages/fluent-editor/src/modules/custom-image/image.tshas been simplified to export onlyCustomImageas the default export. Please double-check that no parts of the repository are still referencingImageContainerBlot. To be thorough, run a repository-wide search (e.g., usingrg "ImageContainerBlot") to confirm there are no lingering usages. If the search returns no results, it should be safe to proceed with this change.
To help with this verification, you can execute the following shell script from the repository root:
Make sure to review the results carefully. If any usages are discovered, they’ll need to be updated accordingly.
🏁 Script executed:
#!/bin/bash # Thoroughly search the entire repository for any occurrences of "ImageContainerBlot" rg "ImageContainerBlot"Length of output: 145
Confirmed: No Residual Usage Found
After performing a repository-wide search for
"ImageContainerBlot", no references were found. This indicates that removing theImageContainerBlotexport inpackages/fluent-editor/src/modules/custom-image/image.tsis safe and does not break any external usage. You can proceed with this change.packages/fluent-editor/src/modules/custom-uploader.ts (3)
1-9: Improved imports with explicit type definitionsThe imports have been reorganized with explicit type annotations, which improves type safety and code clarity.
10-20: Well-structured interface definitionsThe interfaces provide clear type definitions for the uploader options, including typed callbacks that will improve developer experience when implementing custom upload handlers.
31-41: Well-implemented default optionsThe
resolveOptionsmethod provides sensible defaults while allowing customization. The default handler creates object URLs, which is a good starting point.packages/fluent-editor/src/modules/file/formats/file.ts (4)
1-5: Type safety improvements with proper importsSimilar to the image.ts file, the import has been updated with proper type assertion, which enhances type safety.
8-12: Good addition of FileValue interfaceThe FileValue interface clearly defines the required properties for file objects, improving type safety and developer experience.
14-17: Modern class property syntax for static propertiesUsing class property syntax for static properties makes the code more readable and maintainable.
58-60: Proper type annotation for URL parameterThe sanitize method now has a proper type annotation for the URL parameter, improving type safety.
🧰 Tools
🪛 Biome (1.9.4)
[error] 59-59: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/docs/fluent-editor/docs/demo/file-upload.md (2)
14-14: Typo & Clarification in Callback Description
There is a minor spelling typo: “hanler” should be corrected to “handler.” Additionally, consider clarifying that the callback intentionally alternates between accepting (displaying a chrome icon) and rejecting (triggering a fail scenario) uploads for even/odd images to avoid any ambiguity.
23-32: Comprehensive Options Section with Table
The newly added “Options” section and its table offer a clear overview of the configuration settings—such asmimetypes,maxSize,handler,success, andfail. The descriptions and default values are clearly presented. For improved clarity, consider adding a brief example or note for each option to illustrate its usage in practice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/docs/fluent-editor/demos/file-upload-handle.vue(1 hunks)packages/docs/fluent-editor/docs/demo/file-upload.md(1 hunks)packages/fluent-editor/src/modules/custom-clipboard.ts(2 hunks)packages/fluent-editor/src/themes/snow.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/fluent-editor/src/themes/snow.ts
- packages/fluent-editor/src/modules/custom-clipboard.ts
- packages/docs/fluent-editor/demos/file-upload-handle.vue
🔇 Additional comments (6)
packages/docs/fluent-editor/docs/demo/file-upload.md (6)
5-5: Clarify Default Upload Behavior
The sentence clearly states that by default video and image formats are processed, while other formats are uniformly displayed as files. This description is concise and accurate.
10-10: Effective Section Renaming
The new header “## 服务器端上传” effectively signals the shift in focus toward server-side uploads. This change aligns well with the unified upload approach.
12-12: Detail on MIME Type Configuration
The explanation regarding setting the mimetype to'image/*'and its effect on toolbar behavior is clear. However, double-check that this example remains consistent with the overall unified file upload strategy introduced in this PR.
16-16: Clear Explanation for Fail Callback Behavior
The description of how thefailcallback manually inserts an image (displayed as an edge browser icon) to denote a failed upload is straightforward and illustrative.
18-18: Consistent Example Outcome
The example sequence (edge, chrome, edge, chrome) clearly illustrates the alternating outcomes when four images are uploaded. Ensure that this pattern accurately reflects the intended demonstration of the callback logic.
33-34: Detailed Behavior for Filtered Uploads
The block quotes effectively explain that files filtered out bymimetypesormaxSizewill not be passed to thehandlercallback and that therangein thefailcallback reflects the initial upload position. This nuanced detail is valuable for users implementing custom upload logic.
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
由 uploader 模块的 options 代替 uploadOptions
配置不再单独处理图片，内部处理上传的图片与视频显示对应格式，默认支持多文件上传
应该可以解决 #179 #180 #227
Does this PR introduce a breaking change?
Other information
由于没有测试文件以及对应文档，不确定是否能够完全覆盖之前的功能，如有遗漏请说明，我会补齐
Summary by CodeRabbit