Add image cropping for logos and banners using react-image-crop #1796
Conversation
Co-authored-by: marcftone <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughA new image cropping feature has been implemented for logo and banner uploads in branding-related pages. The cropping UI uses the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Branding Page)
participant ReactCrop
participant Canvas
participant State
User->>UI (Branding Page): Uploads image (logo/banner)
UI (Branding Page)->>State: Set imageToCrop, showCropper = true
UI (Branding Page)->>ReactCrop: Render cropping modal with image
User->>ReactCrop: Adjusts crop area, confirms crop
ReactCrop->>Canvas: Render cropped area to canvas
Canvas->>UI (Branding Page): Generate cropped image data URL
UI (Branding Page)->>State: Update logo/banner with cropped image
UI (Branding Page)->>UI (Branding Page): Hide cropping modal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🧹 Nitpick comments (2)
pages/branding.tsx (1)
717-789: Well-implemented cropper modalThe modal UI is well-structured with proper z-indexing, responsive design, and clear user actions. The preview functionality provides good visual feedback.
Consider adding keyboard support for better UX:
{showCropper && imageToCrop && ( - <div className="fixed inset-0 z-50 bg-black/80 flex items-center justify-center p-4"> + <div + className="fixed inset-0 z-50 bg-black/80 flex items-center justify-center p-4" + onKeyDown={(e) => { + if (e.key === 'Escape') { + handleCropCancel(); + } + }} + tabIndex={-1} + >pages/datarooms/[id]/branding/index.tsx (1)
942-1090: Well-structured dual cropper modalsThe separate modals for logo and banner cropping are well-implemented with appropriate aspect ratios and preview dimensions. The UI maintains consistency while accommodating the different requirements.
Similar to the branding.tsx suggestion, consider adding ESC key support for both modals to improve keyboard accessibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)pages/branding.tsx(5 hunks)pages/datarooms/[id]/branding/index.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
package.json (1)
119-119: LGTM!The addition of
react-image-cropdependency is appropriate for implementing the image cropping feature. Version^11.0.10allows for patch and minor updates which is reasonable for a UI library.pages/branding.tsx (1)
143-151: Good UX improvement with input resetSetting the image to crop and showing the cropper modal instead of directly setting the logo is the correct approach. The input value reset at line 150 is a nice touch that allows users to re-select the same file.
pages/datarooms/[id]/branding/index.tsx (1)
253-257: Appropriate banner aspect ratio configurationThe 6:1 aspect ratio for banners (matching the 1920x320 dimension requirement mentioned in the label) is correctly implemented.
| const handleCropComplete = useCallback(() => { | ||
| if (!previewCanvasRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| previewCanvasRef.current.toBlob((blob) => { | ||
| if (!blob) { | ||
| toast.error("Failed to process image"); | ||
| return; | ||
| } | ||
|
|
||
| const reader = new FileReader(); | ||
| reader.onload = () => { | ||
| const dataUrl = reader.result as string; | ||
| setLogo(dataUrl); | ||
| // create a blob url for preview | ||
| const blob = convertDataUrlToFile({ dataUrl }); | ||
| const blobUrl = URL.createObjectURL(blob); | ||
| setBlobUrl(blobUrl); | ||
| setShowCropper(false); | ||
| setImageToCrop(""); | ||
| setCrop(undefined); | ||
| setCompletedCrop(undefined); | ||
| }; | ||
| reader.readAsDataURL(blob); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
Potential memory leak with blob URLs
When creating a new blob URL at line 178, the previous blobUrl should be revoked to prevent memory leaks.
Add URL revocation before setting the new blob URL:
const reader = new FileReader();
reader.onload = () => {
const dataUrl = reader.result as string;
setLogo(dataUrl);
// create a blob url for preview
const blob = convertDataUrlToFile({ dataUrl });
+ // Revoke previous blob URL to prevent memory leak
+ if (blobUrl) {
+ URL.revokeObjectURL(blobUrl);
+ }
const blobUrl = URL.createObjectURL(blob);
setBlobUrl(blobUrl);
setShowCropper(false);
setImageToCrop("");
setCrop(undefined);
setCompletedCrop(undefined);
};Also consider adding cleanup in a useEffect:
useEffect(() => {
return () => {
if (blobUrl) {
URL.revokeObjectURL(blobUrl);
}
};
}, [blobUrl]);🤖 Prompt for AI Agents
In pages/branding.tsx around lines 160 to 186, the code creates a new blob URL
without revoking the previous one, which can cause memory leaks. To fix this,
before setting a new blob URL with setBlobUrl, check if there is an existing
blobUrl and call URL.revokeObjectURL on it to release the old URL. Additionally,
add a useEffect hook that cleans up by revoking the current blobUrl when the
component unmounts or when blobUrl changes, ensuring no blob URLs remain
allocated unnecessarily.
| const handleLogoCropComplete = useCallback(() => { | ||
| if (!logoPreviewCanvasRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| logoPreviewCanvasRef.current.toBlob((blob) => { | ||
| if (!blob) { | ||
| toast.error("Failed to process image"); | ||
| return; | ||
| } | ||
|
|
||
| const reader = new FileReader(); | ||
| reader.onload = () => { | ||
| const dataUrl = reader.result as string; | ||
| setLogo(dataUrl); | ||
| // create a blob url for preview | ||
| const blob = convertDataUrlToFile({ dataUrl }); | ||
| const blobUrl = URL.createObjectURL(blob); | ||
| setBlobUrl(blobUrl); | ||
| setShowLogoCropper(false); | ||
| setLogoImageToCrop(""); | ||
| setLogoCrop(undefined); | ||
| setLogoCompletedCrop(undefined); | ||
| }; | ||
| reader.readAsDataURL(blob); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
Memory leak with logo blob URL
Same issue as in branding.tsx - blob URLs should be revoked to prevent memory leaks.
const reader = new FileReader();
reader.onload = () => {
const dataUrl = reader.result as string;
setLogo(dataUrl);
// create a blob url for preview
const blob = convertDataUrlToFile({ dataUrl });
+ // Revoke previous blob URL to prevent memory leak
+ if (blobUrl) {
+ URL.revokeObjectURL(blobUrl);
+ }
const blobUrl = URL.createObjectURL(blob);
setBlobUrl(blobUrl);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pages/datarooms/[id]/branding/index.tsx around lines 218 to 244, the blob URL
created with URL.createObjectURL is not revoked, causing a memory leak. To fix
this, store the previous blob URL in a ref or state and call URL.revokeObjectURL
on it before setting a new blob URL. Also, ensure to revoke the blob URL when
the component unmounts to fully prevent memory leaks.
| const handleBannerCropComplete = useCallback(() => { | ||
| if (!bannerPreviewCanvasRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| bannerPreviewCanvasRef.current.toBlob((blob) => { | ||
| if (!blob) { | ||
| toast.error("Failed to process image"); | ||
| return; | ||
| } | ||
|
|
||
| const reader = new FileReader(); | ||
| reader.onload = () => { | ||
| const dataUrl = reader.result as string; | ||
| setBanner(dataUrl); | ||
| // create a blob url for preview | ||
| const blob = convertDataUrlToFile({ dataUrl }); | ||
| const bannerBlobUrl = URL.createObjectURL(blob); | ||
| setBannerBlobUrl(bannerBlobUrl); | ||
| setShowBannerCropper(false); | ||
| setBannerImageToCrop(""); | ||
| setBannerCrop(undefined); | ||
| setBannerCompletedCrop(undefined); | ||
| }; | ||
| reader.readAsDataURL(blob); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
Memory leak with banner blob URL
Banner blob URL should also be revoked to prevent memory leaks.
const reader = new FileReader();
reader.onload = () => {
const dataUrl = reader.result as string;
setBanner(dataUrl);
// create a blob url for preview
const blob = convertDataUrlToFile({ dataUrl });
+ // Revoke previous blob URL to prevent memory leak
+ if (bannerBlobUrl) {
+ URL.revokeObjectURL(bannerBlobUrl);
+ }
const bannerBlobUrl = URL.createObjectURL(blob);
setBannerBlobUrl(bannerBlobUrl);Consider adding cleanup in useEffect for both URLs:
useEffect(() => {
return () => {
if (blobUrl) URL.revokeObjectURL(blobUrl);
if (bannerBlobUrl) URL.revokeObjectURL(bannerBlobUrl);
};
}, [blobUrl, bannerBlobUrl]);🤖 Prompt for AI Agents
In pages/datarooms/[id]/branding/index.tsx around lines 259 to 285, the
bannerBlobUrl created with URL.createObjectURL is not revoked, causing a memory
leak. To fix this, add a useEffect hook that cleans up by calling
URL.revokeObjectURL on bannerBlobUrl (and any other blob URLs like blobUrl) when
the component unmounts or when these URLs change. This will properly release the
memory associated with the blob URLs.
2 participants
Summary by CodeRabbit
New Features
Chores