Feature/image cropper - #338
Conversation
|
@MazenSghaier is attempting to deploy a commit to the FindMalek's projects Team on Vercel. A member of the Team first needs to authorize it. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new ProductPhotosSection component for managing product images, integrated with a new ImageCropperDialog utility that uses react-easy-crop. Key feedback includes addressing a potential memory leak by revoking object URLs when files are removed, improving robustness by providing a fallback for the product image field value, avoiding the use of array indices as React keys, and refactoring the file creation logic in the cropper to be more efficient.
| React.useEffect(() => { | ||
| const urlMap = previewUrlsRef.current; | ||
| return () => { | ||
| urlMap.forEach((url) => URL.revokeObjectURL(url)); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
There's a potential memory leak here. The useEffect hook has an empty dependency array [], so the cleanup function only runs when the component unmounts. However, previewUrlsRef can accumulate object URLs created by URL.createObjectURL. If the value prop (the array of files) changes from an external source (e.g., a form reset), this component will re-render, but the old URLs for files that are no longer in value will not be revoked until unmount, leaking memory.
To fix this, you should add value to the useEffect dependency array and implement logic to revoke URLs for any files that have been removed from the value prop during a re-render.
React.useEffect(() => {
const urlMap = previewUrlsRef.current;
const currentFiles = new Set(value);
// Revoke URLs for files that have been removed
for (const [file, url] of urlMap.entries()) {
if (!currentFiles.has(file)) {
URL.revokeObjectURL(url);
urlMap.delete(file);
}
}
return () => {
// On unmount, revoke all remaining URLs
urlMap.forEach((url) => URL.revokeObjectURL(url));
};
}, [value]);
| {(field) => ( | ||
| <ProductPhotosSection | ||
| label={t("form.photos")} | ||
| value={field.state.value as File[]} |
There was a problem hiding this comment.
The value prop is being cast to File[] using as. If field.state.value is null or undefined for any reason, it will be passed down to ProductPhotosSection, which will cause a runtime error when it tries to call .map() on a non-array value.
To make this component more robust, it's safer to provide a fallback to an empty array using the nullish coalescing operator (??).
| value={field.state.value as File[]} | |
| value={(field.state.value as File[] | undefined) ?? []} |
| {value.map((file, index) => ( | ||
| <PhotoThumbnail | ||
| key={`${file.name}-${index}`} | ||
| src={getPreviewUrl(file)} | ||
| alt={file.name} | ||
| onRemove={() => handleRemove(index)} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Using the array index as a key is an anti-pattern in React. It can lead to incorrect component state and performance issues if the list is ever re-ordered or items are removed from the middle. It's always better to use a stable, unique ID for each item.
In this case, the object URL from getPreviewUrl(file) is a perfect candidate for a stable key. This also provides an opportunity to avoid calling getPreviewUrl twice per item in the loop.
{value.map((file, index) => {
const previewUrl = getPreviewUrl(file);
return (
<PhotoThumbnail
key={previewUrl}
src={previewUrl}
alt={file.name}
onRemove={() => handleRemove(index)}
/>
);
})}
| const file = await getCroppedImg(imageSrc, croppedAreaPixels); | ||
| const baseName = fileName.replace(/\.[^.]+$/, ""); | ||
| const croppedFile = new File([file], `${baseName}-cropped.png`, { | ||
| type: "image/png", | ||
| }); | ||
| onComplete(croppedFile); |
There was a problem hiding this comment.
The current implementation creates a File object in getCroppedImg with a hardcoded name, and then immediately wraps it in another File object here in handleConfirm just to rename it. This is slightly inefficient and can be made clearer.
A better approach would be to pass the desired final filename to getCroppedImg and let it create the File with the correct name directly.
This would require updating getCroppedImg signature to async function getCroppedImg(imageSrc: string, pixelCrop: PixelCrop, fileName: string): Promise<File> and using that fileName when creating the blob.
const baseName = fileName.replace(/\.[^.]+$/, "");
const newFileName = baseName + "-cropped.png";
const croppedFile = await getCroppedImg(imageSrc, croppedAreaPixels, newFileName);
onComplete(croppedFile);
📝 WalkthroughWalkthroughProduct image handling was refactored: product-form now uses a new controlled ProductPhotosSection component, and a new ImageCropperDialog was added (using react-easy-crop) to support client-side cropping and optional file optimization. UI package deps were updated. Changes
Sequence DiagramsequenceDiagram
actor User
participant ProductForm
participant ProductPhotosSection
participant ImageCropperDialog
participant CanvasAPI as Canvas API
User->>ProductPhotosSection: Click "Add photo"
ProductPhotosSection->>ProductPhotosSection: Open hidden file input
User->>ProductPhotosSection: Select image file(s)
ProductPhotosSection->>ImageCropperDialog: Open with image data URL & filename
ImageCropperDialog->>User: Display crop UI (zoom/aspect)
User->>ImageCropperDialog: Adjust & confirm crop
ImageCropperDialog->>CanvasAPI: Draw crop region -> toBlob
CanvasAPI-->>ImageCropperDialog: Return cropped image Blob
ImageCropperDialog->>ProductPhotosSection: onComplete(croppedFile)
ProductPhotosSection->>ProductPhotosSection: optimizeFiles? -> append to value
ProductPhotosSection->>ProductForm: onChange(updated File[] value)
ProductForm->>ProductForm: Update form state (value.imageFiles)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/dashboard/src/components/app/products/product-photos-section.tsx (1)
24-29: Use a default export for this dashboard component.This file lives under
apps/dashboard/src/components, soProductPhotosSectionshould follow the repo’s default-export convention for client components. That keeps the feature module consistent and only requires a default import update inproduct-form.tsx.As per coding guidelines, "React components should export a default function component with the 'use client' directive for client-side rendering".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/app/products/product-photos-section.tsx` around lines 24 - 29, This component currently uses a named export; change ProductPhotosSection to be the default-exported client component by adding the "use client" directive at the top (if missing) and exporting the component as the default export (e.g., export default ProductPhotosSection) while keeping ProductPhotosSectionProps and the function signature intact so product-form.tsx can import it as the default import.packages/ui/src/components/image-cropper.tsx (1)
84-88: Rename the file to matchImageCropperDialog.The component is declared as
ImageCropperDialog, but the file isimage-cropper.tsx. Renaming it toimage-cropper-dialog.tsxkeeps the new UI component aligned with the repo’s TSX naming rule.As per coding guidelines, "React/Next.js component file name should match component name (lowercase)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/image-cropper.tsx` around lines 84 - 88, The file name doesn't match the exported React component ImageCropperDialog; rename the file from image-cropper.tsx to image-cropper-dialog.tsx and update any imports referencing this module to use the new filename so the component name ImageCropperDialog and file name follow the repo's TSX naming rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/src/components/app/products/product-photos-section.tsx`:
- Around line 30-49: getPreviewUrl currently calls createObjectURL during render
and the only cleanup runs on unmount, causing leaked blob URLs when the parent
replaces the value array; change to generate and reconcile URLs inside a
React.useEffect keyed on the value prop: in the effect iterate value (the
current File[] prop/state), for each File not present in previewUrlsRef.current
create an object URL and set it in the map, and for each entry in
previewUrlsRef.current whose File is not in value revoke the URL and delete the
map entry; then make getPreviewUrl only return the existing URL from
previewUrlsRef.current without creating new ones. Ensure the existing unmount
effect still revokes any remaining URLs.
In `@packages/ui/src/components/image-cropper.tsx`:
- Around line 76-82: The hardcoded English strings in ImageCropper (e.g.,
ASPECT_OPTIONS and the dialog/button labels between the blocks around lines
145–177 and 181–216) must be moved into a configurable copy object passed via
props (e.g., a prop named copy or translations) with sensible English defaults;
update ASPECT_OPTIONS to build labels from props.copy.aspect.* (or fallbacks)
instead of string literals, and replace all inline dialog/button/header text in
the render functions (the JSX blocks noted) to reference props.copy.* keys so
the host app can supply translations while preserving default copy when none is
provided.
- Around line 115-135: The dialog close path must be gated so onComplete(null)
isn't called while a crop is processing: update the onOpenChange handler to only
call handleCancel when the dialog is closing and isProcessing is false (e.g.,
onOpenChange={(isOpen) => !isOpen && !isProcessing && handleCancel()}); also
ensure handleCancel and handleConfirm remain the single callers of onComplete
and that handleConfirm sets isProcessing appropriately so the guard works as
intended (references: handleConfirm, handleCancel, onOpenChange, isProcessing,
onComplete).
---
Nitpick comments:
In `@apps/dashboard/src/components/app/products/product-photos-section.tsx`:
- Around line 24-29: This component currently uses a named export; change
ProductPhotosSection to be the default-exported client component by adding the
"use client" directive at the top (if missing) and exporting the component as
the default export (e.g., export default ProductPhotosSection) while keeping
ProductPhotosSectionProps and the function signature intact so product-form.tsx
can import it as the default import.
In `@packages/ui/src/components/image-cropper.tsx`:
- Around line 84-88: The file name doesn't match the exported React component
ImageCropperDialog; rename the file from image-cropper.tsx to
image-cropper-dialog.tsx and update any imports referencing this module to use
the new filename so the component name ImageCropperDialog and file name follow
the repo's TSX naming rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9a70a84-0486-4910-9015-6da6c2ebcd91
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/dashboard/src/components/app/products/product-form.tsxapps/dashboard/src/components/app/products/product-photos-section.tsxpackages/ui/package.jsonpackages/ui/src/components/image-cropper.tsx
| const previewUrlsRef = React.useRef<Map<File, string>>(new Map()); | ||
|
|
||
| const [pendingImageSrc, setPendingImageSrc] = React.useState<string | null>(null); | ||
| const [pendingFileName, setPendingFileName] = React.useState<string>("image.png"); | ||
|
|
||
| const fileInputRef = React.useRef<HTMLInputElement>(null); | ||
|
|
||
| React.useEffect(() => { | ||
| const urlMap = previewUrlsRef.current; | ||
| return () => { | ||
| urlMap.forEach((url) => URL.revokeObjectURL(url)); | ||
| }; | ||
| }, []); | ||
|
|
||
| function getPreviewUrl(file: File): string { | ||
| if (!previewUrlsRef.current.has(file)) { | ||
| previewUrlsRef.current.set(file, createObjectURL(file)); | ||
| } | ||
| return previewUrlsRef.current.get(file)!; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "product-photos-section.tsx" -type fRepository: FindMalek/dukkani
Length of output: 132
🏁 Script executed:
cat -n apps/dashboard/src/components/app/products/product-photos-section.tsx | head -150Repository: FindMalek/dukkani
Length of output: 5232
Move blob URL creation out of render and reconcile the map when value changes.
getPreviewUrl() allocates URL.createObjectURL() during render. The current cleanup effect (line 37) only runs on unmount, leaving URLs unreleased when the parent replaces the value prop entirely. This creates a memory leak of blob objects. Add an effect keyed by value to revoke URLs for files that are no longer in the current value array, ensuring orphaned blob URLs are cleaned up immediately rather than persisting until unmount.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/src/components/app/products/product-photos-section.tsx` around
lines 30 - 49, getPreviewUrl currently calls createObjectURL during render and
the only cleanup runs on unmount, causing leaked blob URLs when the parent
replaces the value array; change to generate and reconcile URLs inside a
React.useEffect keyed on the value prop: in the effect iterate value (the
current File[] prop/state), for each File not present in previewUrlsRef.current
create an object URL and set it in the map, and for each entry in
previewUrlsRef.current whose File is not in value revoke the URL and delete the
map entry; then make getPreviewUrl only return the existing URL from
previewUrlsRef.current without creating new ones. Ensure the existing unmount
effect still revokes any remaining URLs.
| const ASPECT_OPTIONS = [ | ||
| { label: "Free", value: "free" }, | ||
| { label: "1:1 (Square)", value: "1" }, | ||
| { label: "4:3", value: String(4 / 3) }, | ||
| { label: "3:4 (Portrait)", value: String(3 / 4) }, | ||
| { label: "16:9", value: String(16 / 9) }, | ||
| ] as const; |
There was a problem hiding this comment.
Externalize the cropper copy.
The aspect labels and dialog text are all hardcoded English. In the dashboard flow this will show mixed-language UI even though the surrounding form is translated, so these strings should come from props or a small copy object supplied by the app layer.
Also applies to: 145-177, 181-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/image-cropper.tsx` around lines 76 - 82, The
hardcoded English strings in ImageCropper (e.g., ASPECT_OPTIONS and the
dialog/button labels between the blocks around lines 145–177 and 181–216) must
be moved into a configurable copy object passed via props (e.g., a prop named
copy or translations) with sensible English defaults; update ASPECT_OPTIONS to
build labels from props.copy.aspect.* (or fallbacks) instead of string literals,
and replace all inline dialog/button/header text in the render functions (the
JSX blocks noted) to reference props.copy.* keys so the host app can supply
translations while preserving default copy when none is provided.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/ui/src/components/image-cropper.tsx (2)
84-88: Align component filename with exported component name.The file is
image-cropper.tsxbut exportsImageCropperDialog; rename file to match component name in lowercase (e.g.,image-cropper-dialog.tsx) for consistency. As per coding guidelines, "React/Next.js component file name should match component name (lowercase)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/image-cropper.tsx` around lines 84 - 88, The exported component is named ImageCropperDialog but the file is image-cropper.tsx; rename the file to match the component name in lowercase (e.g., image-cropper-dialog.tsx) and update any imports referencing image-cropper.tsx to import from the new filename so the React/Next.js component filename matches the exported component (check usages of ImageCropperDialog in the codebase and update their import paths).
22-27: Replace localPixelCropwithAreatype fromreact-easy-crop.The
PixelCropinterface duplicatesreact-easy-crop's exportedAreatype exactly. Importing and usingAreainstead eliminates maintenance drift and aligns type definitions with the library's contract. Additionally, this enables proper typing for the firstonCropCompletecallback parameter (currentlyunknown).Update the import and replace all usages at lines 22–27, 94, and 108–111.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/image-cropper.tsx` around lines 22 - 27, Remove the local PixelCrop interface and import the Area type from react-easy-crop, then replace all usages of PixelCrop with Area (including the type for the first parameter of onCropComplete) so the component uses the library's exported type; update the import statement to include Area and change any function signatures or variables referencing PixelCrop (e.g., the onCropComplete handler and any crop/state types) to Area to ensure proper typing and eliminate duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/image-cropper.tsx`:
- Around line 115-117: Add a re-entry guard at the top of handleConfirm to
return early if isProcessing is true, e.g. if (isProcessing) return; (then
proceed to check imageSrc and croppedAreaPixels and call setIsProcessing(true)
before awaiting work), and ensure the UI Apply button is disabled while
isProcessing is true (e.g. disabled={isProcessing || /* existing disable
conditions */}) so rapid clicks cannot trigger duplicate completion; apply the
same re-entry guard pattern to the other confirm handler referenced (lines
208-213) that also uses setIsProcessing/croppedAreaPixels.
---
Nitpick comments:
In `@packages/ui/src/components/image-cropper.tsx`:
- Around line 84-88: The exported component is named ImageCropperDialog but the
file is image-cropper.tsx; rename the file to match the component name in
lowercase (e.g., image-cropper-dialog.tsx) and update any imports referencing
image-cropper.tsx to import from the new filename so the React/Next.js component
filename matches the exported component (check usages of ImageCropperDialog in
the codebase and update their import paths).
- Around line 22-27: Remove the local PixelCrop interface and import the Area
type from react-easy-crop, then replace all usages of PixelCrop with Area
(including the type for the first parameter of onCropComplete) so the component
uses the library's exported type; update the import statement to include Area
and change any function signatures or variables referencing PixelCrop (e.g., the
onCropComplete handler and any crop/state types) to Area to ensure proper typing
and eliminate duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0930f813-3888-45ec-844f-b0f7eb5d3039
📒 Files selected for processing (3)
apps/dashboard/src/components/app/products/product-form.tsxapps/dashboard/src/components/app/products/product-photos-section.tsxpackages/ui/src/components/image-cropper.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard/src/components/app/products/product-form.tsx
- apps/dashboard/src/components/app/products/product-photos-section.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/components/image-cropper.tsx (1)
41-44: Provide a descriptive error on image load failure.When
onerrorfires, rejecting with the raw event yields an opaque"[object Event]"in logs. Wrapping it in anErrormakes debugging easier.🔧 Suggested improvement
await new Promise<void>((resolve, reject) => { image.onload = () => resolve(); - image.onerror = reject; + image.onerror = () => reject(new Error(`Failed to load image: ${imageSrc.slice(0, 50)}...`)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/image-cropper.tsx` around lines 41 - 44, The Promise that waits for image.onload currently rejects with the raw event (image.onerror = reject), producing an opaque "[object Event]"; change the onerror handler in the Promise that wraps image.onload/onerror so it rejects with a descriptive Error (e.g., include the image.src and event message) instead of the raw event; update the Promise around image (the image.onload/image.onerror handlers) to create and pass an Error instance on failure for clearer logs and debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/components/image-cropper.tsx`:
- Around line 41-44: The Promise that waits for image.onload currently rejects
with the raw event (image.onerror = reject), producing an opaque "[object
Event]"; change the onerror handler in the Promise that wraps
image.onload/onerror so it rejects with a descriptive Error (e.g., include the
image.src and event message) instead of the raw event; update the Promise around
image (the image.onload/image.onerror handlers) to create and pass an Error
instance on failure for clearer logs and debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b66ab16-6750-4e92-9f20-adeca7e972a9
📒 Files selected for processing (1)
packages/ui/src/components/image-cropper.tsx
|
@greptileai please review this! |
2 participants
🧠 Overview
Adds an image cropping step to the product photo upload flow. Previously, images were added directly to the preview grid after selection. Now, selecting a file opens a crop dialog powered by
react-easy-cropwhere the user can adjust zoom and aspect ratio before confirming. The cropped output is a properFileobject that flows through the existingcompressImagesForUploadpipeline unchanged.New files:
image-cropper-dialog.tsx— self-contained crop dialog (zoom slider, aspect ratio presets: Free / 1:1 / 4:3 / 3:4 / 16:9)product-photos-section.tsx— replaces the rawImagesInputfield; intercepts file selection, opens the cropper, then commits the result to the form✅ Pre-merge Checklist
Code Quality
🧪 Testing Checklist
Core Flow (Preview Environment)
Storefront Behavior
Storage Validation
Seeder / Demo Accounts
🔍 Additional Notes
crossOriginis only set on remote URLs — applying it todata:URLs caused a silent load failure that left the cropper blankheight: 380pxinstead offlex-1; RadixDialogContentdoes not propagate a constrained height down the flex chain, which causedreact-easy-cropto render with zero heightSummary by CodeRabbit