feat: add avatar crop modal with zoom and pan support#39084
Open
himanshu6093 wants to merge 9 commits intoRocketChat:developfrom
Open
feat: add avatar crop modal with zoom and pan support#39084himanshu6093 wants to merge 9 commits intoRocketChat:developfrom
himanshu6093 wants to merge 9 commits intoRocketChat:developfrom
Conversation
Contributor
|
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: d34fbc1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ 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). (1)
WalkthroughAdds a new AvatarCropModal component (circular 300×300 crop viewport with pan/zoom and export) and integrates it into UserAvatarEditor to defer uploads until Apply; updates useUpdateAvatar to convert cropped data-URI avatars to File+FormData and call saveAvatarAction. Includes a changelog entry. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UserAvatarEditor
participant AvatarCropModal
participant useUpdateAvatar
participant Backend
User->>UserAvatarEditor: Upload image
UserAvatarEditor->>UserAvatarEditor: set imageToCrop
UserAvatarEditor->>AvatarCropModal: render(imageSrc)
User->>AvatarCropModal: pan/zoom interactions
User->>AvatarCropModal: click Apply
AvatarCropModal->>AvatarCropModal: export 300x300 PNG dataURI
AvatarCropModal->>UserAvatarEditor: onApply(dataURI)
UserAvatarEditor->>useUpdateAvatar: trigger update with dataURI
useUpdateAvatar->>useUpdateAvatar: dataUriToFile -> FormData
useUpdateAvatar->>Backend: saveAvatarAction(FormData)
Backend-->>useUpdateAvatar: success
useUpdateAvatar-->>UserAvatarEditor: updated
UserAvatarEditor-->>User: display new avatar
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx`:
- Around line 56-80: The effect is recreating a new Image() and rebinding
img.onload on every zoom/offset change which causes jank; instead, create and
cache the HTMLImageElement once when imageSrc changes (e.g. imageRef or
loadedImage in an effect that depends only on imageSrc), set its onload there,
and in the effect that depends on zoom and offset use the cached imageRef to
redraw to the canvas (no new Image creation). Ensure you handle cleanup of
onload and check imageRef.current exists before drawing (and optionally use
requestAnimationFrame to schedule the canvas redraw in the zoom/offset effect).
- Around line 34-49: Replace mouse-only handlers with pointer event handlers to
add touch support: rename onMouseDown -> onPointerDown, onMouseMove ->
onPointerMove, onMouseUp -> onPointerUp and onMouseLeave -> onPointerCancel (or
onPointerUp) and update all usages (e.g., JSX props) accordingly; in
onPointerDown call e.currentTarget.setPointerCapture(e.pointerId) after setting
setIsDragging(true) and initializing dragStart.current, then in onPointerMove
use the same logic that uses dragStart.current and setOffset, and in
onPointerUp/onPointerCancel call setIsDragging(false) and release capture if
needed. Ensure the handler signatures accept React.PointerEvent<HTMLDivElement>
instead of MouseEvent and keep references to setIsDragging, dragStart, and
setOffset unchanged.
In `@apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx`:
- Around line 35-49: The file input reset is only called on the success path
inside setUploadedPreview, so when readFileAsDataURL or isValidImageFormat
throws the input isn't cleared; update setUploadedPreview to always call
resetUpload (e.g., via a finally block or by invoking it in both success and
catch branches) after attempting to read/validate the file, keeping the existing
error handling that dispatches the toast in the catch; reference the
setUploadedPreview function and resetUpload, and ensure readFileAsDataURL and
isValidImageFormat calls remain unchanged.
In `@apps/meteor/client/hooks/useUpdateAvatar.ts`:
- Around line 15-19: The data-URI path can throw from atob() causing an uncaught
crash; update dataUriToFile(dataUri: string) to validate the input more strictly
(ensure header and base64 part exist, header matches /^data:([^;]+);base64$/ and
base64Data is non-empty) and have it throw a clear Error on invalid input, and
wrap the call site in useUpdateAvatar's data-URI branch (the block that
currently calls dataUriToFile) in a try-catch that dispatches the same error
toast used in the service-object path (so user sees a message) and prevents the
unhandled exception—identify the caller by the hook useUpdateAvatar and the
helper dataUriToFile when making these changes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsxapps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsxapps/meteor/client/hooks/useUpdateAvatar.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsxapps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsxapps/meteor/client/hooks/useUpdateAvatar.ts
🧠 Learnings (2)
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/hooks/useUpdateAvatar.ts
🧬 Code graph analysis (1)
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx (2)
packages/ui-contexts/src/index.ts (1)
useToastMessageDispatch(78-78)packages/core-typings/src/IUser.ts (1)
AvatarObject(302-302)
apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx">
<violation number="1" location="apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx:43">
P2: `resetUpload()` is only called on the success path. If an exception is thrown (e.g., by `readFileAsDataURL` or `isValidImageFormat`), the file input is never reset, preventing the user from re-selecting the same file. Move `resetUpload()` to a `finally` block. Also, `resetUpload` is missing from the `useCallback` dependency array, which violates the React hooks exhaustive-deps rule and may cause the callback to reference a stale closure.</violation>
</file>
<file name="apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx">
<violation number="1" location="apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx:34">
P2: Using mouse events (`onMouseDown`, `onMouseMove`, `onMouseUp`, `onMouseLeave`) prevents the drag/pan functionality from working on touch devices. Replace with pointer events (`onPointerDown`, `onPointerMove`, `onPointerUp`, `onPointerCancel`) which handle both mouse and touch inputs. Add `e.currentTarget.setPointerCapture(e.pointerId)` in the down handler to ensure reliable drag tracking.</violation>
<violation number="2" location="apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx:63">
P2: Creating a new `Image()` on every zoom/offset change causes unnecessary image reloads during panning. Since the `useEffect` depends on `[imageSrc, zoom, offset]`, each drag movement triggers a new `Image()` instantiation and `onload` binding, which can cause visible jank. Load the image once when `imageSrc` changes (storing it in a ref), and use a separate effect for the canvas drawing that reads from the ref.</violation>
<violation number="3" location="apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx:66">
P2: The canvas draw effect has no cleanup/guard, so a slow previous Image load can still run its onload after imageSrc changes and overwrite the preview with stale content.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx (1)
128-132: Remove unnecessary double type assertion.The
as unknown as AvatarObjectcast is redundant. Since{ avatarUrl: croppedImage }has type{ avatarUrl: string }, which matchesAvatarUrlObj(a member of theAvatarObjectunion), it's directly assignable without casting—as shown in line 56 with the same pattern usingavatarFromUrl.setAvatarObj({ avatarUrl: croppedImage });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx` around lines 128 - 132, In the onApply handler for UserAvatarEditor, remove the unnecessary double type assertion when setting the avatar object: instead of casting the object literal to unknown then AvatarObject, assign a plain object with avatarUrl set to croppedImage to setAvatarObj; update the onApply callback that currently calls setImageToCrop, setNewAvatarSource and setAvatarObj(...) so setAvatarObj receives a correctly-typed { avatarUrl: croppedImage } without any "as unknown as AvatarObject" cast.
🤖 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/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx`:
- Around line 53-84: The draw effect can run before imageRef.current is set
because img.onload is async; fix by triggering a redraw when the image finishes
loading—e.g., in the first useEffect where you create img and set
imageRef.current, also set a small piece of state (like imageLoaded or
imageVersion) or call a redraw trigger so the second effect runs again; add that
state (imageLoaded/imageVersion) to the dependency array of the drawing
useEffect that reads canvasRef and imageRef, or invoke the draw logic directly
from img.onload to ensure ctx.drawImage runs after imageRef is populated (update
references: imageRef, canvasRef, the two useEffect hooks, setZoom/setOffset as
needed).
---
Nitpick comments:
In `@apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx`:
- Around line 128-132: In the onApply handler for UserAvatarEditor, remove the
unnecessary double type assertion when setting the avatar object: instead of
casting the object literal to unknown then AvatarObject, assign a plain object
with avatarUrl set to croppedImage to setAvatarObj; update the onApply callback
that currently calls setImageToCrop, setNewAvatarSource and setAvatarObj(...) so
setAvatarObj receives a correctly-typed { avatarUrl: croppedImage } without any
"as unknown as AvatarObject" cast.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/add-avatar-crop-modal.mdapps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsxapps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsxapps/meteor/client/hooks/useUpdateAvatar.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/hooks/useUpdateAvatar.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsxapps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx
🧠 Learnings (3)
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx
🧬 Code graph analysis (1)
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx (2)
packages/ui-contexts/src/index.ts (1)
useToastMessageDispatch(78-78)packages/core-typings/src/IUser.ts (1)
AvatarObject(302-302)
🔇 Additional comments (4)
apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx (1)
103-157: LGTM!The modal structure is well-organized with proper Fuselage components. The circular crop preview with pointer events for touch/mouse support and the zoom slider provide a good UX.
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx (2)
124-134: LGTM on the crop modal integration.The conditional rendering of
AvatarCropModaland the flow of clearingimageToCropon both cancel and apply paths is correct.
35-52: No changes needed. The dependency array is correct.
resetUploadis created usinguseEffectEvent()from '@rocket.chat/fuselage-hooks', which returns a stable callback reference that maintains the same identity across renders. Per React hook rules, stable dependencies don't need to be included in dependency arrays. The code follows best practices and requires no fixes.Likely an incorrect or invalid review comment.
.changeset/add-avatar-crop-modal.md (1)
1-5: LGTM!The changeset correctly documents the new feature with an appropriate minor version bump.
apps/meteor/client/components/avatar/UserAvatarEditor/AvatarCropModal.tsx
Outdated
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
This PR adds a crop modal to the user avatar upload flow. When a user selects an image for their profile picture, a modal now appears allowing them to:
Zoom the image using a slider (50%–300%)
Pan the image by clicking and dragging
Preview the final result through a circular crop window
The cropped image is then uploaded as a file via the existing users.setAvatar endpoint.
Technical changes
New file:
AvatarCropModal.tsxModified:
UserAvatarEditor.tsxuseSingleFileInputreset to allow re-selecting the same fileModified:
useUpdateAvatar.tsdataUriToFile()helper to convert base64 data URIs touseSingleFileInput.tsBefore:
https://github.com/user-attachments/assets/b9535d12-95cd-467b-8a15-60500ca27a74
After:
https://github.com/user-attachments/assets/2307640d-0d37-4399-8ee7-0bbc8d1949ad
Issue(s)
Closes #39055
Steps to test or reproduce
Further comments
I built a custom crop modal using HTML5 Canvas instead of adding a third-party cropping library, to avoid increasing the bundle size. It uses existing Fuselage components for the UI, so it fits naturally with the rest of the app.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements