fix: improve error handling in avatar upload components#37908
fix: improve error handling in avatar upload components#37908adityasuhane-06 wants to merge 2 commits into
Conversation
|
WalkthroughRoomAvatarEditor and UserAvatarEditor now use the shared readFileAsDataURL helper and surface file-read and format-validation failures via explicit error toasts ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx (1)
33-49: Good improvement in error handling, but consider order of operations.The distinction between
Avatar_format_invalidandFileUpload_Errorcorrectly separates format validation from file read failures.However,
setAvatarObj(avatarObj)at line 35 is called before format validation completes. IfisValidImageFormatreturnsfalse, the parent component still holds the invalid file reference while the user sees an error toast—this could lead to confusing state if the user later submits the form.Consider moving
setAvatarObj(avatarObj)inside theifblock (after line 39) so the avatar object is only set when format validation passes.🔎 Suggested fix
const setUploadedPreview = useCallback( async (file: File, avatarObj: AvatarObject) => { - setAvatarObj(avatarObj); try { const dataURL = await readFileAsDataURL(file); if (await isValidImageFormat(dataURL)) { + setAvatarObj(avatarObj); setNewAvatarSource(dataURL); } else { dispatchToastMessage({ type: 'error', message: t('Avatar_format_invalid') }); } } catch (error) { dispatchToastMessage({ type: 'error', message: t('FileUpload_Error') }); } }, [setAvatarObj, t, dispatchToastMessage], );
🧹 Nitpick comments (1)
apps/meteor/client/components/avatar/RoomAvatarEditor.tsx (1)
13-13: Consider relocating shared utility.
readFileAsDataURLis now used by bothUserAvatarEditorandRoomAvatarEditor, but resides inside theUserAvatarEditorfolder. For discoverability and to reflect its shared nature, consider moving it to a common location such as../../lib/utils/alongsideisValidImageFormat.
📜 Review details
Configuration used: Organization 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 selected for processing (2)
apps/meteor/client/components/avatar/RoomAvatarEditor.tsx(2 hunks)apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx(1 hunks)
🧰 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/RoomAvatarEditor.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: aleksandernsilva
Repo: RocketChat/Rocket.Chat PR: 36974
File: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx:124-129
Timestamp: 2025-09-18T17:32:33.969Z
Learning: The ARIA mismatch issue in MessageForm's template field (where FieldError id used templateId instead of messageFormId) was addressed in PR #36972 through refactoring the template field into a separate TemplateField component, which uses consistent templateFieldId for both aria-describedby and FieldError id.
🔇 Additional comments (1)
apps/meteor/client/components/avatar/RoomAvatarEditor.tsx (1)
28-40: Clean refactor with proper error handling.The implementation correctly:
- Uses the shared
readFileAsDataURLutility for consistency- Validates format before calling
onChangeAvatar, ensuring only valid images update state- Distinguishes between file read errors and format validation errors with appropriate toast messages
- Refactored RoomAvatarEditor to use readFileAsDataURL utility for consistency - Added explicit error handling for both file read and format validation failures - Distinguished between FileUpload_Error (read failures) and Avatar_format_invalid (format issues) - Improved code maintainability by using shared utility function Fixes RocketChat#37895
adityasuhane-06
force-pushed
the
fix/avatar-upload-error-handling-improved
branch
from
af06cc8 to
4952000
Compare
- Move setAvatarObj call inside validation check - Ensures avatar object is only updated for valid images - Addresses CodeRabbit review suggestion
1 participant
Description
This PR improves error handling in avatar upload components by refactoring to use a shared utility function and providing more specific error messages to users.
Changes
readFileAsDataURLutility instead of inline FileReader logicError Scenarios Handled
Benefits
How to Test
User Profile - Invalid Format:
.txtfile or non-image fileRoom Info - File Read Error:
Valid Upload:
Related Issues
Fixes #37895
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.