Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,17 @@ export const CustomImageBlock: React.FC<CustomImageBlockProps> = (props) => {
const containerRect = useRef<DOMRect | null>(null);
const imageRef = useRef<HTMLImageElement>(null);

const updateAttributesSafely = useCallback(
(attributes: Partial<ImageAttributes>, errorMessage: string) => {
try {
updateAttributes(attributes);
} catch (error) {
console.error(`${errorMessage}:`, error);
}
},
[updateAttributes]
);

const handleImageLoad = useCallback(() => {
const img = imageRef.current;
if (!img) return;
Expand Down Expand Up @@ -105,17 +116,25 @@ export const CustomImageBlock: React.FC<CustomImageBlockProps> = (props) => {
};

setSize(initialComputedSize);
updateAttributes(initialComputedSize);
updateAttributesSafely(
initialComputedSize,
"Failed to update attributes while initializing an image for the first time:"
);
} else {
// as the aspect ratio in not stored for old images, we need to update the attrs
setSize((prevSize) => {
const newSize = { ...prevSize, aspectRatio };
updateAttributes(newSize);
return newSize;
});
if (!aspectRatio) {
setSize((prevSize) => {
const newSize = { ...prevSize, aspectRatio };
updateAttributesSafely(
newSize,
"Failed to update attributes while initializing images with width but no aspect ratio:"
);
return newSize;
});
}
}
setInitialResizeComplete(true);
}, [width, updateAttributes, editorContainer]);
}, [width, updateAttributes, editorContainer, aspectRatio]);

// for real time resizing
useLayoutEffect(() => {
Expand All @@ -142,7 +161,7 @@ export const CustomImageBlock: React.FC<CustomImageBlockProps> = (props) => {

const handleResizeEnd = useCallback(() => {
setIsResizing(false);
updateAttributes(size);
updateAttributesSafely(size, "Failed to update attributes at the end of resizing:");
}, [size, updateAttributes]);

const handleResizeStart = useCallback((e: React.MouseEvent | React.TouchEvent) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ import { ImageIcon } from "lucide-react";
// helpers
import { cn } from "@/helpers/common";
// hooks
import { useUploader, useDropZone } from "@/hooks/use-file-upload";
// plugins
import { isFileValid } from "@/plugins/image";
import { useUploader, useDropZone, uploadFirstImageAndInsertRemaining } from "@/hooks/use-file-upload";
// extensions
import { getImageComponentImageFileMap, ImageAttributes } from "@/extensions/custom-image";

Expand Down Expand Up @@ -74,17 +72,18 @@ export const CustomImageUploader = (props: {
);
// hooks
const { uploading: isImageBeingUploaded, uploadFile } = useUploader({ onUpload, editor, loadImageFromFileSystem });
const { draggedInside, onDrop, onDragEnter, onDragLeave } = useDropZone({ uploader: uploadFile });
const { draggedInside, onDrop, onDragEnter, onDragLeave } = useDropZone({
uploader: uploadFile,
editor,
pos: getPos(),
});

// the meta data of the image component
const meta = useMemo(
() => imageComponentImageFileMap?.get(imageEntityId),
[imageComponentImageFileMap, imageEntityId]
);

// if the image component is dropped, we check if it has an existing file
const existingFile = useMemo(() => (meta && meta.event === "drop" ? meta.file : undefined), [meta]);

// after the image component is mounted we start the upload process based on
// it's uploaded
useEffect(() => {
Expand All @@ -100,27 +99,20 @@ export const CustomImageUploader = (props: {
}
}, [meta, uploadFile, imageComponentImageFileMap]);

// check if the image is dropped and set the local image as the existing file
useEffect(() => {
if (existingFile) {
uploadFile(existingFile);
}
}, [existingFile, uploadFile]);

const onFileChange = useCallback(
(e: ChangeEvent<HTMLInputElement>) => {
const file = e.target.files?.[0];
if (file) {
if (isFileValid(file)) {
uploadFile(file);
}
async (e: ChangeEvent<HTMLInputElement>) => {
e.preventDefault();
const fileList = e.target.files;
if (!fileList) {
return;
}
await uploadFirstImageAndInsertRemaining(editor, fileList, getPos(), uploadFile);
},
[uploadFile]
[uploadFile, editor, getPos]
Comment on lines +103 to +111
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

LGTM with a minor suggestion: Improved file change handling.

The changes in the onFileChange callback effectively address multiple file uploads and include a null check for fileList, which resolves the previous concern about potential runtime errors. The use of uploadFirstImageAndInsertRemaining aligns with the PR objectives.

However, there's still a minor issue in the dependency array:

The uploadFile dependency is listed twice in the dependency array. Please remove the duplicate to avoid potential confusion:

-    [uploadFile, editor, getPos, uploadFile]
+    [uploadFile, editor, getPos]

Committable suggestion was skipped due to low confidence.

);

const getDisplayMessage = useCallback(() => {
const isUploading = isImageBeingUploaded || existingFile;
const isUploading = isImageBeingUploaded;
if (failedToLoadImage) {
return "Error loading image";
}
Expand All @@ -134,13 +126,14 @@ export const CustomImageUploader = (props: {
}

return "Add an image";
}, [draggedInside, failedToLoadImage, existingFile, isImageBeingUploaded]);
}, [draggedInside, failedToLoadImage, isImageBeingUploaded]);

return (
<div
className={cn(
"image-upload-component flex items-center justify-start gap-2 py-3 px-2 rounded-lg text-custom-text-300 hover:text-custom-text-200 bg-custom-background-90 hover:bg-custom-background-80 border border-dashed border-custom-border-300 cursor-pointer transition-all duration-200 ease-in-out",
"image-upload-component flex items-center justify-start gap-2 py-3 px-2 rounded-lg text-custom-text-300 hover:text-custom-text-200 bg-custom-background-90 hover:bg-custom-background-80 border border-dashed border-custom-border-300 transition-all duration-200 ease-in-out cursor-default",
{
"hover:text-custom-text-200 cursor-pointer": editor.isEditable,
"bg-custom-background-80 text-custom-text-200": draggedInside,
"text-custom-primary-200 bg-custom-primary-100/10 hover:bg-custom-primary-100/10 hover:text-custom-primary-200 border-custom-primary-200/10":
selected,
Expand All @@ -153,7 +146,7 @@ export const CustomImageUploader = (props: {
onDragLeave={onDragLeave}
contentEditable={false}
onClick={() => {
if (!failedToLoadImage) {
if (!failedToLoadImage && editor.isEditable) {
fileInputRef.current?.click();
}
}}
Expand All @@ -167,6 +160,7 @@ export const CustomImageUploader = (props: {
type="file"
accept=".jpg,.jpeg,.png,.webp"
onChange={onFileChange}
multiple
/>
</div>
);
Expand Down
13 changes: 3 additions & 10 deletions packages/editor/src/core/extensions/drop.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export const DropHandlerExtension = () =>

if (imageFiles.length > 0) {
const pos = view.state.selection.from;
insertImages({ editor, files: imageFiles, initialPos: pos, event: "drop" });
insertImagesSafely({ editor, files: imageFiles, initialPos: pos, event: "drop" });
}
return true;
}
Expand All @@ -41,7 +41,7 @@ export const DropHandlerExtension = () =>

if (coordinates) {
const pos = coordinates.pos;
insertImages({ editor, files: imageFiles, initialPos: pos, event: "drop" });
insertImagesSafely({ editor, files: imageFiles, initialPos: pos, event: "drop" });
}
return true;
}
Expand All @@ -54,7 +54,7 @@ export const DropHandlerExtension = () =>
},
});

const insertImages = async ({
export const insertImagesSafely = async ({
editor,
files,
initialPos,
Expand All @@ -72,13 +72,6 @@ const insertImages = async ({
const docSize = editor.state.doc.content.size;
pos = Math.min(pos, docSize);

// Check if the position has a non-empty node
const nodeAtPos = editor.state.doc.nodeAt(pos);
if (nodeAtPos && nodeAtPos.content.size > 0) {
// Move to the end of the current node
pos += nodeAtPos.nodeSize;
}

try {
// Insert the image at the current position
editor.commands.insertImageComponent({ file, pos, event });
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/core/hooks/use-editor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ export const useEditor = (props: CustomEditorProps) => {
forwardedRef,
() => ({
clearEditor: (emitUpdate = false) => {
editorRef.current?.commands.clearContent(emitUpdate);
editorRef.current?.chain().setMeta("skipImageDeletion", true).clearContent(emitUpdate).run();
},
setEditorValue: (content: string) => {
editorRef.current?.commands.setContent(content, false, { preserveWhitespace: "full" });
Expand Down
80 changes: 51 additions & 29 deletions packages/editor/src/core/hooks/use-file-upload.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { DragEvent, useCallback, useEffect, useState } from "react";
import { Editor } from "@tiptap/core";
import { isFileValid } from "@/plugins/image";
import { insertImagesSafely } from "@/extensions/drop";

export const useUploader = ({
onUpload,
Expand Down Expand Up @@ -63,7 +64,15 @@ export const useUploader = ({
return { uploading, uploadFile };
};

export const useDropZone = ({ uploader }: { uploader: (file: File) => void }) => {
export const useDropZone = ({
uploader,
editor,
pos,
}: {
uploader: (file: File) => Promise<void>;
editor: Editor;
pos: number;
}) => {
const [isDragging, setIsDragging] = useState<boolean>(false);
const [draggedInside, setDraggedInside] = useState<boolean>(false);

Expand All @@ -86,40 +95,16 @@ export const useDropZone = ({ uploader }: { uploader: (file: File) => void }) =>
}, []);

const onDrop = useCallback(
(e: DragEvent<HTMLDivElement>) => {
async (e: DragEvent<HTMLDivElement>) => {
e.preventDefault();
setDraggedInside(false);
if (e.dataTransfer.files.length === 0) {
return;
}

const fileList = e.dataTransfer.files;

const files: File[] = [];

for (let i = 0; i < fileList.length; i += 1) {
const item = fileList.item(i);
if (item) {
files.push(item);
}
}

if (files.some((file) => file.type.indexOf("image") === -1)) {
return;
}

e.preventDefault();

const filteredFiles = files.filter((f) => f.type.indexOf("image") !== -1);

const file = filteredFiles.length > 0 ? filteredFiles[0] : undefined;

if (file) {
uploader(file);
} else {
console.error("No file found");
}
await uploadFirstImageAndInsertRemaining(editor, fileList, pos, uploader);
},
[uploader]
[uploader, editor, pos]
);

const onDragEnter = () => {
Expand All @@ -143,3 +128,40 @@ function trimFileName(fileName: string, maxLength = 100) {

return fileName;
}

// Upload the first image and insert the remaining images for uploading multiple image
// post insertion of image-component
export async function uploadFirstImageAndInsertRemaining(
editor: Editor,
fileList: FileList,
pos: number,
uploaderFn: (file: File) => Promise<void>
) {
const filteredFiles: File[] = [];
for (let i = 0; i < fileList.length; i += 1) {
const item = fileList.item(i);
if (item && item.type.indexOf("image") !== -1 && isFileValid(item)) {
filteredFiles.push(item);
}
}
if (filteredFiles.length !== fileList.length) {
console.warn("Some files were not images and have been ignored.");
}
if (filteredFiles.length === 0) {
console.error("No image files found to upload");
return;
}

// Upload the first image
const firstFile = filteredFiles[0];
uploaderFn(firstFile);

// Insert the remaining images
const remainingFiles = filteredFiles.slice(1);

if (remainingFiles.length > 0) {
const docSize = editor.state.doc.content.size;
const posOfNextImageToBeInserted = Math.min(pos + 1, docSize);
insertImagesSafely({ editor, files: remainingFiles, initialPos: posOfNextImageToBeInserted, event: "drop" });
}
}
Comment on lines +132 to +167
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Good implementation, but consider awaiting the first image upload.

The uploadFirstImageAndInsertRemaining function is well-structured and handles multiple image uploads effectively. However, there's a potential issue:

  • The upload of the first image (line 157) is not awaited, which could lead to race conditions with the insertion of remaining images.

Consider modifying line 157 to await the upload:

- uploaderFn(firstFile);
+ await uploaderFn(firstFile);

This ensures that the first image is fully uploaded before proceeding with the insertion of remaining images.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Upload the first image and insert the remaining images for uploading multiple image
// post insertion of image-component
export async function uploadFirstImageAndInsertRemaining(
editor: Editor,
fileList: FileList,
pos: number,
uploaderFn: (file: File) => Promise<void>
) {
const filteredFiles: File[] = [];
for (let i = 0; i < fileList.length; i += 1) {
const item = fileList.item(i);
if (item && item.type.indexOf("image") !== -1 && isFileValid(item)) {
filteredFiles.push(item);
}
}
if (filteredFiles.length !== fileList.length) {
console.warn("Some files were not images and have been ignored.");
}
if (filteredFiles.length === 0) {
console.error("No image files found to upload");
return;
}
// Upload the first image
const firstFile = filteredFiles[0];
uploaderFn(firstFile);
// Insert the remaining images
const remainingFiles = filteredFiles.slice(1);
if (remainingFiles.length > 0) {
const docSize = editor.state.doc.content.size;
const posOfNextImageToBeInserted = Math.min(pos + 1, docSize);
insertImagesSafely({ editor, files: remainingFiles, initialPos: posOfNextImageToBeInserted, event: "drop" });
}
}
// Upload the first image and insert the remaining images for uploading multiple image
// post insertion of image-component
export async function uploadFirstImageAndInsertRemaining(
editor: Editor,
fileList: FileList,
pos: number,
uploaderFn: (file: File) => Promise<void>
) {
const filteredFiles: File[] = [];
for (let i = 0; i < fileList.length; i += 1) {
const item = fileList.item(i);
if (item && item.type.indexOf("image") !== -1 && isFileValid(item)) {
filteredFiles.push(item);
}
}
if (filteredFiles.length !== fileList.length) {
console.warn("Some files were not images and have been ignored.");
}
if (filteredFiles.length === 0) {
console.error("No image files found to upload");
return;
}
// Upload the first image
const firstFile = filteredFiles[0];
await uploaderFn(firstFile);
// Insert the remaining images
const remainingFiles = filteredFiles.slice(1);
if (remainingFiles.length > 0) {
const docSize = editor.state.doc.content.size;
const posOfNextImageToBeInserted = Math.min(pos + 1, docSize);
insertImagesSafely({ editor, files: remainingFiles, initialPos: posOfNextImageToBeInserted, event: "drop" });
}
}

4 changes: 2 additions & 2 deletions packages/editor/src/core/hooks/use-read-only-editor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ export const useReadOnlyEditor = (props: CustomReadOnlyEditorProps) => {
const editorRef: MutableRefObject<Editor | null> = useRef(null);

useImperativeHandle(forwardedRef, () => ({
clearEditor: () => {
editorRef.current?.commands.clearContent();
clearEditor: (emitUpdate = false) => {
editorRef.current?.chain().setMeta("skipImageDeletion", true).clearContent(emitUpdate).run();
},
setEditorValue: (content: string) => {
editorRef.current?.commands.setContent(content, false, { preserveWhitespace: "full" });
Expand Down
2 changes: 2 additions & 0 deletions packages/editor/src/core/plugins/image/delete-image.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ export const TrackImageDeletionPlugin = (editor: Editor, deleteImage: DeleteImag
});

transactions.forEach((transaction) => {
// if the transaction has meta of skipImageDeletion get to true, then return (like while clearing the editor content programatically)
if (transaction.getMeta("skipImageDeletion")) return;
// transaction could be a selection
if (!transaction.docChanged) return;

Expand Down