feat: enable cropping of images before sending in chat #36737
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 4035891 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
|
Hey @Khizarshah01 ! I was looking at this some weeks ago! Perfect timing! |
|
Hey @jeanfbrito , thanks for the suggestion! 😄 |
Khizarshah01
force-pushed
the
feature/image-crop
branch
from
20033cf to
8e293de
Compare
|
@Khizarshah01 Nice contribution!
Let's do a separate PR for the avatars, it makes easier for the maintainers to review Also would you mind using our feature preview flag for this new feature? This will allow us to merge this as an "Experimental" feature. From reading your code, I think you can do that by creating a new I'll help you out along the way |
|
@Khizarshah01 sorry I forgot to send the message... 🤦 |
|
In Avatar we have a fixed size, so it would be easier to make it work directly, for the image uploads would be better if we could draw a box to select what we want to crop. |
Khizarshah01
force-pushed
the
feature/image-crop
branch
from
8e293de to
8541fd6
Compare
|
Thanks @MartinSchoeler , I have applied your feedback, created |
|
Thanks @jeanfbrito , for the suggestions 🙏 I completely agree
For this PR, should I continue implementing these changes here? |
Khizarshah01
force-pushed
the
feature/image-crop
branch
from
8541fd6 to
a56f045
Compare
rocketChatCropImage.mp4This PR has been updated with all the requested feedback, feature preview flag, separate component, and (UI) Fuselage usage. Ready for review whenever you have time. |
Khizarshah01
force-pushed
the
feature/image-crop
branch
from
a56f045 to
131d8af
Compare
WalkthroughAdds an image-cropping flow to file uploads: new CropFilePreview component, FileUploadModal updates to support cropping and pass a File to submit, upload flow updated to handle an optional cropped File through encryption and upload; feature flag, i18n keys, and react-easy-crop dependency added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant M as FileUploadModal
participant C as CropFilePreview
participant F as uploadFiles
participant E as E2EE
participant S as Server
U->>M: Select file
M-->>U: Show preview (+ Crop button if image/feature)
alt User chooses Crop
M->>C: Open cropping modal with File
C-->>M: Return cropped File
else No cropping
M-->>M: Use original File
end
M->>F: onSubmit(name, desc, file)
opt Encryption enabled
F->>E: Encrypt fileToUpload
E-->>F: Encrypted data
end
F->>S: Upload attachment (uses fileToUpload metadata)
S-->>U: Message with attachment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)apps/meteor/client/lib/chats/flows/uploadFiles.ts (3)
🔇 Additional comments (4)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/meteor/client/lib/chats/flows/uploadFiles.ts (4)
73-79: Don’t mutate File.name; create a new File when renaming.Directly redefining File.name is unreliable and can throw. Create a new File with the desired name.
Apply this diff:
- onSubmit: async (fileName: string, description?: string, croppedFile?: File): Promise<void> => { - const fileToUpload = croppedFile ?? file; - Object.defineProperty(fileToUpload, 'name', { - writable: true, - value: fileName, - }); + onSubmit: async (fileName: string, description?: string, croppedFile?: File): Promise<void> => { + const baseFile = croppedFile ?? file; + const fileToUpload = + baseFile.name === fileName + ? baseFile + : new File([baseFile], fileName, { + type: baseFile.type, + lastModified: baseFile.lastModified, + });
106-170: Use the cropped/renamed file for all metadata (type, size, dims, name) in E2EE content.This block still references the original file, producing wrong metadata and dimensions after cropping.
Apply this diff:
const getContent = async (_id: string, fileUrl: string): Promise<IE2EEMessage['content']> => { const attachments = []; const attachment: FileAttachmentProps = { - title: fileToUpload.name, + title: fileToUpload.name, type: 'file', description, title_link: fileUrl, title_link_download: true, encryption: { key: encryptedFile.key, iv: encryptedFile.iv, }, hashes: { sha256: encryptedFile.hash, }, }; - if (/^image\/.+/.test(file.type)) { - const dimensions = await getHeightAndWidthFromDataUrl(window.URL.createObjectURL(file)); + if (/^image\/.+/.test(fileToUpload.type)) { + const objectUrl = window.URL.createObjectURL(fileToUpload); + const dimensions = await getHeightAndWidthFromDataUrl(objectUrl); + window.URL.revokeObjectURL(objectUrl); attachments.push({ ...attachment, image_url: fileUrl, - image_type: file.type, - image_size: file.size, + image_type: fileToUpload.type, + image_size: fileToUpload.size, ...(dimensions && { image_dimensions: dimensions, }), }); - } else if (/^audio\/.+/.test(file.type)) { + } else if (/^audio\/.+/.test(fileToUpload.type)) { attachments.push({ ...attachment, audio_url: fileUrl, - audio_type: file.type, - audio_size: file.size, + audio_type: fileToUpload.type, + audio_size: fileToUpload.size, }); - } else if (/^video\/.+/.test(file.type)) { + } else if (/^video\/.+/.test(fileToUpload.type)) { attachments.push({ ...attachment, video_url: fileUrl, - video_type: file.type, - video_size: file.size, + video_type: fileToUpload.type, + video_size: fileToUpload.size, }); } else { attachments.push({ ...attachment, - size: file.size, - format: getFileExtension(file.name), + size: fileToUpload.size, + format: getFileExtension(fileToUpload.name), }); } const files = [ { _id, - name: file.name, - type: file.type, - size: file.size, + name: fileToUpload.name, + type: fileToUpload.type, + size: fileToUpload.size, // "format": "png" }, ] as IMessage['files']; return e2eRoom.encryptMessageContent({ attachments, files, file: files?.[0], }); };
172-183: Build fileContentData from the actual file being sent.Keep metadata consistent with the cropped/renamed file.
Apply this diff:
- const fileContentData = { - type: file.type, - typeGroup: file.type.split('/')[0], - name: fileName, + const fileContentData = { + type: fileToUpload.type, + typeGroup: fileToUpload.type.split('/')[0], + name: fileToUpload.name, encryption: { key: encryptedFile.key, iv: encryptedFile.iv, }, hashes: { sha256: encryptedFile.hash, }, };
121-123: Revoke temporary object URLs to avoid leaks.createObjectURL must be revoked after use.
Included in the previous diff: store the URL in a variable and call URL.revokeObjectURL(objectUrl) after awaiting dimensions.
apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx (1)
65-76: Validate against the cropped file’s size.Size check uses the original file; use currentFile so users don’t get false results after cropping.
Apply this diff:
- const submit = ({ name, description }: { name: string; description?: string }): void => { + const submit = ({ name, description }: { name: string; description?: string }): void => { // -1 maxFileSize means there is no limit - if (maxFileSize > -1 && (file.size || 0) > maxFileSize) { + if (maxFileSize > -1 && (currentFile.size || 0) > maxFileSize) { onClose(); return dispatchToastMessage({ type: 'error', message: t('File_exceeds_allowed_size_of_bytes', { size: fileSize(maxFileSize) }), }); } onSubmit(name, description, currentFile); };
🧹 Nitpick comments (4)
package.json (1)
86-88: Avoid duplicating app-only deps at the repo root.react-easy-crop is only used by @rocket.chat/meteor; keep it there to reduce surface area and lockfile churn.
Apply this diff to remove the root-level dependency:
"dependencies": { "@types/stream-buffers": "^3.0.7", - "node-gyp": "^10.2.0", - "react-easy-crop": "^5.5.0" + "node-gyp": "^10.2.0" }If you intend to pin the version across workspaces, prefer a root "resolutions" entry instead:
"resolutions": { + "react-easy-crop": "^5.5.0", "minimist": "1.2.6", ... }apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx (1)
50-55: Sync the file name input after cropping.After applying a crop, update the form field so users see the actual filename being uploaded.
Apply this diff:
- const { - register, - handleSubmit, - formState: { errors, isSubmitting }, - } = useForm({ mode: 'onBlur', defaultValues: { name: fileName, description: fileDescription } }); + const { + register, + handleSubmit, + formState: { errors, isSubmitting }, + setValue, + } = useForm({ mode: 'onBlur', defaultValues: { name: fileName, description: fileDescription } });- <CropFilePreview + <CropFilePreview file={currentFile} onFileChange={(newFile) => { setCurrentFile(newFile); + setValue('name', newFile.name, { shouldValidate: true }); setIsCropping(false); }} onCancel={() => setIsCropping(false)} />Also applies to: 116-123
apps/meteor/client/views/room/modals/FileUploadModal/CropFilePreview.tsx (2)
11-15: Avoid re‑declaring FilePreviewType; import it to prevent type drift.Duplicating the enum risks type mismatches with MediaPreview/FilePreview. Prefer importing the existing type.
If FilePreview.tsx exports FilePreviewType, switch to:
-import { isIE11 } from '../../../../lib/utils/isIE11'; - -export enum FilePreviewType { - IMAGE = 'image', - AUDIO = 'audio', - VIDEO = 'video', -} +import { isIE11 } from '../../../../lib/utils/isIE11'; +import { FilePreviewType } from './FilePreview';If it doesn’t, consider exporting it there and reusing here. Verify MediaPreview’s prop typing to ensure compatibility.
115-124: Dead UI branch after onFileChange.Parent immediately closes the crop modal; the local croppedImageURL preview won’t render. Safe to remove for simplicity.
You can drop croppedImageURL state and the corresponding early return to reduce complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.changeset/young-buses-grow.md(1 hunks)apps/meteor/client/lib/chats/flows/uploadFiles.ts(2 hunks)apps/meteor/client/views/room/modals/FileUploadModal/CropFilePreview.tsx(1 hunks)apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx(6 hunks)apps/meteor/package.json(1 hunks)package.json(1 hunks)packages/i18n/src/locales/en.i18n.json(2 hunks)packages/ui-client/src/hooks/useFeaturePreviewList.ts(2 hunks)
🔇 Additional comments (7)
packages/ui-client/src/hooks/useFeaturePreviewList.ts (2)
8-9: Add imageCropPreview and secondarySidebar to FeaturesAvailable: LGTM.Prevents union/type drift with defaultFeaturesPreview.
46-53: Feature preview entry verified — i18n keys and gating present
- i18n keys exist: packages/i18n/src/locales/en.i18n.json — Image_crop_preview, Image_crop_preview_description.
- Flag is consumed in UI: apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx uses useFeaturePreview('imageCropPreview'); entry is in packages/ui-client/src/hooks/useFeaturePreviewList.ts.
apps/meteor/package.json (1)
431-431: Approve: react-easy-crop added at app scope; peer deps compatible with React 18.
Usage confirmed in apps/meteor/client/views/room/modals/FileUploadModal/CropFilePreview.tsx; npm registry (v5.5.1) lists peerDependencies { react: '>=16.4.0', 'react-dom': '>=16.4.0' } — compatible with React 18.packages/i18n/src/locales/en.i18n.json (1)
1491-1493: LGTM on new crop UI stringsThe keys and copy look fine for the crop action and modal title.
apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx (1)
145-149: Nice: feature‑flagged Crop UI, image‑only.Good gating via useFeaturePreview and MIME check.
Please confirm the feature key is imageCropPreview and is included in defaultFeaturesPreview/useFeaturePreviewList.
apps/meteor/client/views/room/modals/FileUploadModal/CropFilePreview.tsx (2)
99-110: Object URL lifecycle handled—LGTM.Good creation and cleanup of object URLs for previews.
136-153: Cropper config: sensible defaults.Free aspect, wheel zoom, and bounded container are fine for v1.
Khizarshah01
force-pushed
the
feature/image-crop
branch
from
131d8af to
944f734
Compare
|
this PR is now ready for review |
4 participants
Proposed changes (including videos or screenshots)
This PR adds image cropping functionality to Rocket.Chat when uploading images in chat.
react-easy-crop.crop-imageRocketChat.mp4
Issue(s)
#27735
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Chores