[PE-69] fix: image restoration fixed for new images in private bucket #5839
Conversation
…stop updates on load
WalkthroughThe changes in this pull request introduce significant enhancements to image handling within the editor. Key modifications include the addition of a 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: 6
🧹 Outside diff range and nitpick comments (6)
packages/editor/src/core/extensions/image/image-extension-without-props.tsx (1)
14-16: Approved: Addition ofaspectRatioattribute, but documentation needed.The addition of the
aspectRatioattribute is a good improvement and aligns with the PR objectives. It follows the existing code structure and provides flexibility for image handling.However, to enhance code maintainability and clarity:
- Please add a comment explaining how
aspectRatiowill be used in conjunction withwidthandheight.- Consider documenting any specific logic or constraints related to these attributes (e.g., if
aspectRatiotakes precedence overheightwhenwidthis set).packages/editor/src/core/extensions/image/read-only-image.tsx (1)
21-23: LGTM! Consider adding a type annotation for consistency.The addition of the
aspectRatioattribute aligns well with the PR objectives and follows the existing pattern for image attributes. This change will allow for better control over image display, particularly for older images as mentioned in the PR description.For consistency with the
widthandheightattributes, consider adding a type annotation to theaspectRatioattribute. This would improve type safety and make the code more self-documenting. Here's a suggested modification:aspectRatio: { - default: null, + default: null as number | null, },packages/editor/src/core/plugins/image/restore-image.ts (1)
28-30: Approve with minor suggestions for improvementThe added condition aligns well with the PR objective of fixing image restoration for new images. It effectively filters out images that are likely from a private bucket, allowing them to be handled differently.
However, I have a couple of suggestions for improvement:
Consider including 'https' explicitly in the condition for clarity:
if (!node.attrs.src.startsWith("http") && !node.attrs.src.startsWith("https")) return;The comment could be slightly more precise. Consider updating it to:
// If the src is just an id (private bucket), we don't need to handle restoration here. // This will be handled when the image fails to load.These minor changes would enhance the clarity and robustness of the code.
packages/editor/src/core/extensions/image/extension.tsx (2)
39-42: Improved image source validation with unnecessary loggingThe addition of a check to ensure the image source starts with "http" is a good improvement for input validation. However, there's an unnecessary console.log statement that should be removed.
Consider removing the console.log statement on line 39:
- console.log("node", node.attrs.src.startsWith("http"));
73-75: New aspectRatio attribute addedThe addition of the
aspectRatioattribute with a default value of null is a good improvement. This can be useful for maintaining image proportions during resizing or layout adjustments.Consider adding a brief comment explaining the purpose and usage of the
aspectRatioattribute, for example:aspectRatio: { default: null, // The aspect ratio of the image (width/height). Set to null for automatic calculation. },packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (1)
Image resizing for mobile devices not addressed.
The
CustomImageUploadercomponent shows improvements in handling aspect ratios through theaspectRatioproperty. However, the PR objectives mention fixing image resizing for mobile devices and updating aspect ratios for older images, which are not reflected in the current changes.
- Missing Image Resizing for Mobile Devices: No implementations found that handle responsive image resizing specific to mobile viewports.
Please address the image resizing functionality to ensure images display correctly across different device sizes.
🔗 Analysis chain
Line range hint
1-180: Overall improvements in type safety and code organization, but some PR objectives not addressed.The changes in this file significantly improve type safety and code organization for the
CustomImageUploadercomponent. The new type definitions and prop structure enhance maintainability and readability.However, I noticed that the PR objectives mention fixing image resizing for mobile devices and updating aspect ratios for older images. These specific changes are not evident in this file.
To ensure we're not missing any relevant changes, let's check for related files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for files that might contain image resizing or aspect ratio logic rg -t tsx -t ts "image.*resize|aspect.*ratio" --ignore-caseLength of output: 89
Script:
#!/bin/bash # Search for files that might contain image resizing or aspect ratio logic rg -g "*.ts" -g "*.tsx" "image.*resize|aspect.*ratio" --ignore-caseLength of output: 5752
Script:
#!/bin/bash # Search for files that might contain image resizing logic with varied terminology rg -g "*.ts" -g "*.tsx" "(image.*resize|resize.*image|responsive.*image|image.*dimensions)" --ignore-caseLength of output: 1237
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- packages/editor/src/core/extensions/custom-image/components/image-block.tsx (11 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-node.tsx (3 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (2 hunks)
- packages/editor/src/core/extensions/custom-image/custom-image.ts (4 hunks)
- packages/editor/src/core/extensions/image/extension.tsx (3 hunks)
- packages/editor/src/core/extensions/image/image-extension-without-props.tsx (1 hunks)
- packages/editor/src/core/extensions/image/read-only-image.tsx (1 hunks)
- packages/editor/src/core/plugins/image/restore-image.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (18)
packages/editor/src/core/extensions/image/read-only-image.tsx (1)
Line range hint
1-38: Verify completeness of image handling improvementsWhile the addition of the
aspectRatioattribute is a good step towards improving image handling, it's important to ensure that all objectives mentioned in the PR description are addressed:
- Image restoration for new images
- Image resizing correction for mobile devices
- Aspect ratio updates for older images
To ensure these objectives are fully met, please run the following checks:
Additionally, consider the following questions:
- How is the new
aspectRatioattribute being used in theCustomImageNodecomponent?- Are there any other components or utilities that need to be updated to fully implement the image handling improvements mentioned in the PR description?
- Have appropriate tests been added or updated to cover these changes?
Consider documenting the rationale behind adding the
aspectRatioattribute and how it relates to the overall image handling strategy in the project. This could be valuable for future maintenance and onboarding of new developers.packages/editor/src/core/extensions/image/extension.tsx (3)
15-16: Improved function parameter namingThe renaming of
deletetodeleteImageFnandrestoretorestoreImageFnenhances code readability and clarity. These new names are more descriptive and better convey the purpose of these functions.
30-31: Consistent use of renamed functions in pluginsThe
TrackImageDeletionPluginandTrackImageRestorationPluginnow correctly use the renameddeleteImageFnandrestoreImageFnfunctions. This change maintains consistency with the earlier parameter renaming.
48-48: Consistent use of renamed restore functionThe
restoreImagefunction call has been correctly updated to userestoreImageFn, maintaining consistency with the earlier parameter renaming.packages/editor/src/core/extensions/custom-image/components/image-node.tsx (5)
2-2: LGTM: Import statement updated correctly.The addition of
NodeViewPropsto the import statement from@tiptap/reactis consistent with the type changes in the file and necessary for the updated type definitions.
6-14: Excellent: New type definition enhances type safety and clarity.The introduction of
CustomImageComponentPropstype is a great improvement:
- It consolidates all necessary properties for the component in one place.
- The use of
NodeViewProps["node"]for thenodeproperty, combined with{ attrs: ImageAttributes }, provides more specific and type-safe definitions.- This change will make the component more maintainable and less prone to type-related errors.
16-16: Great job: Improved type definition for CustomImageNodeViewProps.The updated
CustomImageNodeViewPropstype definition is a significant improvement:
- It extends both
NodeViewPropsandCustomImageComponentProps, providing a more comprehensive type for theCustomImageNodecomponent.- This change aligns perfectly with the goal of consolidating properties and enhancing type safety.
- It will make the component more robust and easier to maintain by ensuring all necessary props are included.
20-20: Well done: Improved variable access and effect dependencies.The changes in the
CustomImageNodecomponent are beneficial:
- Destructuring
srcasremoteImageSrcfromnode.attrs(line 20) simplifies access to the image source throughout the component.- Updating the
useEffectdependency array to includeremoteImageSrc(line 48) ensures proper reactivity when the image source changes.These modifications enhance code readability and maintain correct component behavior.
Also applies to: 48-48
59-59: Good change, but needs clarification: Updated image source retrieval.The modification to use
editor?.commands?.getImageSource?.(remoteImageSrc)for thesrcprop is a good improvement:
- It leverages the editor's command system, which might provide additional processing or validation of the image source.
- This approach is more flexible and aligns better with the editor's architecture.
However, there are two points that need attention:
- The TypeScript error suppression comment (
@ts-expect-error) suggests a type mismatch. It would be beneficial to address this type issue properly rather than suppressing it.- The multiple optional chaining operators (
?.) might make the code more fragile. Consider adding a fallback value or handling the case where the command doesn't exist.To ensure the
getImageSourcecommand exists and is used correctly, please run the following verification script:Could you please provide more context on why the TypeScript error suppression is necessary here? Is there a way to resolve this type mismatch without using
@ts-expect-error?packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (4)
8-8: LGTM: Import statement updated appropriately.The new import for
CustomImageComponentPropsandgetImageComponentImageFileMapaligns with the type changes in the component. This promotes better type safety and code organization.
10-15: LGTM: New type definition improves component interface.The new
CustomImageUploaderPropstype effectively consolidates all required props for theCustomImageUploadercomponent. This change enhances type safety and makes the component's interface more explicit. The included properties cover all necessary functionality for image uploading and handling.
17-17: LGTM: Component declaration updated to use new prop type.The
CustomImageUploadercomponent now correctly uses theCustomImageUploaderPropstype for its props. This change ensures type safety and consistency with the newly defined prop interface.
32-32: LGTM: SimplifiedimageEntityIdextraction.The
imageEntityIdis now directly destructured fromnode.attrs, which simplifies the code and improves readability. This change aligns well with the component's props structure.packages/editor/src/core/extensions/custom-image/custom-image.ts (5)
44-45: Improved naming to avoid reserved keywordsRenaming
deletetodeleteImageFnandrestoretorestoreImageFnavoids potential conflicts with reserved keywords in JavaScript/TypeScript. This enhances code readability and prevents unexpected behavior.
89-94: Add keyboard shortcuts for better user navigationThe
addKeyboardShortcuts()method introduces shortcuts forArrowDownandArrowUpto insert empty paragraphs at node boundaries. Ensure that these shortcuts enhance the user experience and do not interfere with existing editor functionalities.
107-109: Validate image source before restorationThe check
if (!node.attrs.src.startsWith("http")) { return; }ensures only images with sources starting with "http" are processed. SincestartsWith("http")covers both "http://" and "https://", this check is appropriate. However, consider if there are other valid URI schemes (e.g., data URIs) that should be included in the restoration process.If other schemes should be included, you might need to adjust the condition accordingly.
186-188: EnsurerestoreImagecommand aligns with expected usageThe
restoreImagecommand is implemented to asynchronously callrestoreImageFn(src). Ensure that this command is properly utilized within the editor and that any necessary error handling is in place to handle potential issues during image restoration.
96-102: Integrate image tracking plugins properlyThe
addProseMirrorPlugins()method now includesTrackImageDeletionPluginandTrackImageRestorationPlugin, which handle image deletion and restoration. Verify that these plugins are correctly implemented and that they interact seamlessly withdeleteImageFnandrestoreImageFn.To confirm the proper integration of the plugins, you can check for their correct instantiation and usage in the codebase:
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
Outdated
Show resolved
Hide resolved
| @@ -181,11 +184,15 @@ | |||
| window.addEventListener("mousemove", handleResize); | |||
| window.addEventListener("mouseup", handleResizeEnd); | |||
| window.addEventListener("mouseleave", handleResizeEnd); | |||
| window.addEventListener("touchmove", handleResize, { passive: false }); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Re-evaluate the necessity of { passive: false } in touch event listener
The handleResize function does not call e.preventDefault(). If you are not preventing the default touch behavior, you can remove { passive: false } to improve scrolling performance.
Apply this diff:
- window.addEventListener("touchmove", handleResize, { passive: false });
+ window.addEventListener("touchmove", handleResize);📝 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.
| window.addEventListener("touchmove", handleResize, { passive: false }); | |
| window.addEventListener("touchmove", handleResize); |
| @@ -124,9 +125,10 @@ | |||
| ); | |||
| } else { | |||
| // as the aspect ratio in not stored for old images, we need to update the attrs | |||
| if (!aspectRatio) { | |||
| // or if aspectRatioCalculated from the image's width and height doesn't match stored aspectRatio then also we'll update the attrs | |||
| if (!nodeAspectRatio || nodeAspectRatio !== aspectRatioCalculated) { | |||
There was a problem hiding this comment.
Use tolerance when comparing floating-point numbers
Directly comparing floating-point numbers can lead to precision issues. Use an epsilon value to compare nodeAspectRatio and aspectRatioCalculated to account for minor differences.
Apply this diff:
+ const EPSILON = 0.00001;
if (
!nodeAspectRatio ||
- nodeAspectRatio !== aspectRatioCalculated
+ Math.abs(nodeAspectRatio - aspectRatioCalculated) > EPSILON
) {
// existing code
}Committable suggestion was skipped due to low confidence.
| @@ -104,17 +105,17 @@ | |||
| } | |||
|
|
|||
| setEditorContainer(closestEditorContainer); | |||
| const aspectRatio = img.naturalWidth / img.naturalHeight; | |||
| const aspectRatioCalculated = img.naturalWidth / img.naturalHeight; | |||
There was a problem hiding this comment.
Prevent division by zero when calculating aspectRatioCalculated
When calculating aspectRatioCalculated, ensure that img.naturalHeight is not zero to avoid a potential division by zero error.
Apply this diff to add the check:
+ if (img.naturalHeight === 0) {
+ console.error("Image natural height is zero, cannot calculate aspect ratio.");
+ return;
+ }
const aspectRatioCalculated = img.naturalWidth / img.naturalHeight;📝 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.
| const aspectRatioCalculated = img.naturalWidth / img.naturalHeight; | |
| if (img.naturalHeight === 0) { | |
| console.error("Image natural height is zero, cannot calculate aspect ratio."); | |
| return; | |
| } | |
| const aspectRatioCalculated = img.naturalWidth / img.naturalHeight; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/editor/src/core/extensions/image/extension.tsx (2)
39-42: Improved image source validation and consistent function usageThe addition of a check to ensure the image source starts with "http" is a good validation step. The use of
restoreImageFnis consistent with the earlier renaming.However, there's an unnecessary console.log statement that should be removed:
- console.log("node", node.attrs.src?.startsWith("http"));Also applies to: 48-48
73-75: New aspectRatio attribute addedThe addition of the
aspectRatioattribute with a default value of null is a good enhancement. This can be useful for maintaining image proportions during resizing or display.Consider adding a brief comment explaining the purpose and expected usage of this new attribute for better code documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/editor/src/core/extensions/custom-image/custom-image.ts (4 hunks)
- packages/editor/src/core/extensions/image/extension.tsx (3 hunks)
- packages/editor/src/core/plugins/image/restore-image.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/editor/src/core/extensions/custom-image/custom-image.ts
- packages/editor/src/core/plugins/image/restore-image.ts
🧰 Additional context used
🔇 Additional comments (3)
packages/editor/src/core/extensions/image/extension.tsx (3)
15-16: Improved function parameter namingThe renaming of
deletetodeleteImageFnandrestoretorestoreImageFnenhances code readability and clarity. This change makes the purpose of these functions more explicit and follows better naming conventions.
30-31: Consistent use of renamed functions in pluginsThe
deleteImageFnandrestoreImageFnare now correctly used in theTrackImageDeletionPluginandTrackImageRestorationPluginrespectively. This change maintains consistency with the earlier parameter renaming.
Line range hint
1-91: Overall improvements align with PR objectivesThe changes in this file contribute significantly to the PR objectives of fixing image restoration and improving image handling. Key improvements include:
- Enhanced function naming for clarity
- Improved image source validation
- Addition of an aspectRatio attribute for better image display control
These modifications collectively enhance the image management capabilities of the editor, aligning well with the stated PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/editor/src/core/extensions/custom-image/components/image-block.tsx (7 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-node.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/extensions/custom-image/components/image-node.tsx
🧰 Additional context used
🔇 Additional comments (5)
packages/editor/src/core/extensions/custom-image/components/image-block.tsx (5)
75-75: Approve the new state variable with a note on namingThe introduction of
onFirstLoadErroris a good addition for tracking initial load errors. However, as previously suggested, consider renaming it tohasFirstLoadErrororisFirstLoadErrorfor clarity, as the current name might be misinterpreted as an event handler.
141-141: Approve the updated dependency array inhandleImageLoadAdding
size.aspectRatioto the dependency array ofhandleImageLoadis a good improvement. This ensures that the function will react to changes in the aspect ratio, maintaining consistency between the component's state and its visual representation.
149-151: Approve the updates touseLayoutEffectThe addition of
nodeAspectRatioto both the state update and the dependency array inuseLayoutEffectis a good improvement. This ensures that the component's state is properly synchronized with the node's aspect ratio, enhancing the overall consistency of the image rendering.
164-164: Approve the updated dependency array inhandleResizeAdding
size.aspectRatioto the dependency array ofhandleResizeis a good improvement. This ensures that the resize function always uses the most current aspect ratio, maintaining the correct proportions during image resizing.
Line range hint
1-313: Overall assessment: Approved with minor suggestionsThe changes in this file significantly improve the image handling capabilities of the
CustomImageBlockcomponent, aligning well with the PR objectives. The enhancements in error handling, particularly for image restoration, and the improved reactivity to aspect ratio changes are noteworthy improvements.Key points:
- The new
onFirstLoadErrorstate variable enhances error tracking (consider renaming for clarity).- Updates to dependency arrays in
handleImageLoadandhandleResizeimprove reactivity.- The
useLayoutEffecthook now properly synchronizes withnodeAspectRatio.- The enhanced error handling in the
onErrorhandler is a significant improvement, though consider implementing a retry limit to prevent potential infinite loops.These changes should result in more robust image handling, particularly for new images, as stated in the PR objectives. The suggestions provided in the individual comments, if implemented, would further refine the component's functionality and reliability.
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/editor/src/core/extensions/custom-image/components/image-block.tsx (7 hunks)
- packages/editor/src/core/extensions/image/extension.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/extensions/image/extension.tsx
🧰 Additional context used
🔇 Additional comments (6)
packages/editor/src/core/extensions/custom-image/components/image-block.tsx (6)
75-75: Existing comment is still validThe existing comment suggesting to rename
onFirstLoadErrorfor clarity is still relevant and valid. Consider implementing this suggestion to improve code readability.
149-151: LGTM: Improved state synchronizationThe addition of
nodeAspectRatioto both theuseLayoutEffectdependency array and thesizestate update is a good improvement. This ensures that the component's local state stays in sync with the node attributes, particularly when the aspect ratio changes.
164-164: LGTM: Correct dependency in handleResizeAdding
size.aspectRatioto the dependency array ofhandleResizeis a good practice. This ensures that the function is recreated when the aspect ratio changes, maintaining the correctness of the resize functionality.
187-188: Existing comment is still validThe existing comment suggesting to re-evaluate the necessity of
{ passive: false }in the touch event listener is still relevant. Consider implementing this suggestion to potentially improve scrolling performance ifpreventDefault()is not being called in thehandleResizefunction.Also applies to: 194-195
212-212: LGTM: Improved error state handlingIncluding
onFirstLoadErrorin theshowImageLoadercondition is a good improvement. This ensures that the image loader is displayed when there's an error during the initial load, providing better visual feedback to the user.
308-308: LGTM: Improved touch device supportAdding the
onTouchStartevent handler to the resize handle is a good improvement. This enhances the usability of the image resizing feature on touch devices, making the component more versatile across different platforms.
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
Outdated
Show resolved
Hide resolved
Palanikannan1437
force-pushed
the
fix/image-restore
branch
from
c64f051 to
beaaefd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/editor/src/core/extensions/extensions.tsx (1)
143-146: LGTM! Consider enhancing readability.The addition of
editor.isActive("imageComponent")to theshouldHidePlaceholdercondition is logical and consistent with the existing pattern. This change aligns well with the PR objectives, particularly in improving image handling.To enhance readability, consider extracting the conditions into an array and using the
some()method. Here's a suggested refactor:const activeComponents = ['table', 'codeBlock', 'image', 'imageComponent']; const shouldHidePlaceholder = activeComponents.some(component => editor.isActive(component));This approach makes it easier to add or remove components in the future without changing the logic structure.
packages/editor/src/core/extensions/custom-image/custom-image.ts (1)
107-114: Improved image source validation and restorationThe addition of a validation check for image sources starting with "http" enhances the robustness of the image handling process. The use of
restoreImageFnis consistent with earlier renaming.However, consider improving the error handling:
Instead of just logging the error, consider implementing a more robust error handling mechanism that informs the user or allows for retry attempts. For example:
imageSources.forEach(async (src) => { try { await restoreImageFn(src); } catch (error) { console.error("Error restoring image: ", error); // Inform the user about the failed restoration this.editor.commands.showNotification(`Failed to restore image: ${src}`, 'error'); // Or add the failed src to a list for potential retry } });packages/editor/src/core/extensions/custom-image/components/image-block.tsx (1)
188-189: LGTM: Added touch event support for resizingThe addition of touch event listeners (
touchmoveandtouchend) improves the component's functionality on touch devices, allowing for image resizing on mobile platforms. This is a valuable enhancement to the user experience.Note: The past review comment suggesting the use of
{ passive: true }for thetouchmoveevent listener has not been addressed. Consider adding this option to improve scrolling performance:- window.addEventListener("touchmove", handleResize); + window.addEventListener("touchmove", handleResize, { passive: true });Also applies to: 195-196
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/editor/src/core/extensions/custom-image/components/image-block.tsx (7 hunks)
- packages/editor/src/core/extensions/custom-image/custom-image.ts (4 hunks)
- packages/editor/src/core/extensions/extensions.tsx (1 hunks)
- packages/editor/src/core/extensions/image/extension.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/extensions/image/extension.tsx
🧰 Additional context used
🔇 Additional comments (10)
packages/editor/src/core/extensions/custom-image/custom-image.ts (5)
25-25: LGTM: NewrestoreImagecommand addedThe addition of the
restoreImagecommand to theCommandsinterface is consistent with the PR objective of fixing image restoration. The command signature follows the existing pattern and is well-integrated into the interface.
44-45: Improved naming convention for delete and restore propsThe renaming of
deletetodeleteImageFnandrestoretorestoreImageFnenhances code clarity and maintains consistency with the naming conventions used throughout the file. This change improves readability and makes the purpose of these props more explicit.
89-94: Enhanced user experience with keyboard shortcutsThe addition of keyboard shortcuts for ArrowDown and ArrowUp improves navigation around image blocks. The use of the
insertEmptyParagraphAtNodeBoundarieshelper function ensures consistent behavior across different node types. This enhancement aligns well with improving the overall usability of the editor.
96-101: Improved image tracking with ProseMirror pluginsThe addition of
TrackImageDeletionPluginandTrackImageRestorationPluginenhances the image handling capabilities of the editor. These plugins, utilizing the renameddeleteImageFnandrestoreImageFn, provide a robust mechanism for tracking image deletion and restoration events. This change aligns well with the PR objectives and improves the overall functionality of the image extension.
185-187: LGTM:restoreImagecommand implementationThe implementation of the
restoreImagecommand is consistent with the earlier interface definition and aligns well with the PR objectives. It correctly utilizes therestoreImageFnprop, maintaining consistency throughout the extension.packages/editor/src/core/extensions/custom-image/components/image-block.tsx (5)
Line range hint
18-18: LGTM: Addition of aspectRatio to ImageAttributesThe addition of the
aspectRatioproperty to theImageAttributestype is a good improvement. It allows for better control and consistency in image rendering, especially when resizing.
75-76: LGTM: New state variables for error handlingThe addition of
hasErroredOnFirstLoadandhasTriedRestoringImageOncestate variables improves error handling during image loading. These variables provide clear indicators of the loading state and whether an image restoration attempt has been made.Note: This change effectively addresses the past review comment about naming clarity for error-related variables.
150-152: LGTM: Aspect ratio update in useLayoutEffectThe addition of
aspectRatio: nodeAspectRatioin the useLayoutEffect hook ensures that the component's state stays in sync with the node's attributes. This change is consistent with the earlier addition of the aspectRatio property and improves the overall consistency of the image handling logic.
241-260: LGTM: Improved error handling for image loadingThe new error handling logic for image loading is a significant improvement. It properly checks for the existence of the
restoreImagecommand and uses the new state variables to manage error states and restoration attempts. This implementation effectively addresses the concern raised in a past review comment about potential infinite loops by using thehasTriedRestoringImageOnceflag.Key improvements:
- Checks for the existence of
restoreImagecommand before attempting to use it.- Uses
hasTriedRestoringImageOnceto prevent multiple restoration attempts.- Properly manages error states with
setHasErroredOnFirstLoadandsetFailedToLoadImage.- Includes error logging for better debugging.
These changes enhance the robustness and reliability of the image loading process.
165-165: LGTM with a suggestion: Simplified handleResize dependencyThe simplification of the handleResize dependency array to only include
size.aspectRatiois good for performance. However, please verify that this doesn't omit any necessary dependencies that could affect the behavior of the handleResize function.To ensure all necessary dependencies are included, let's analyze the handleResize function:
✅ Verification successful
Verified handleResize dependencies
After reviewing the
handleResizefunction, the dependency array correctly includes onlysize.aspectRatio. No additional dependencies are necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Extract the handleResize function and its dependencies sed -n '/const handleResize/,/^ }/p' packages/editor/src/core/extensions/custom-image/components/image-block.tsxLength of output: 1099
Palanikannan1437
changed the title
Palanikannan1437
changed the title
Description
Summary by CodeRabbit
New Features
aspectRatioattribute.Bug Fixes
Refactor