Fix avatar upload stuck state and update header after changing avatar #9427
Conversation
WalkthroughThis pull request focuses on improving the file upload and avatar management process across multiple components. The changes primarily address error handling, state management, and user experience during file uploads. The modifications include updating the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/Users/UserAvatar.tsx (2)
Line range hint
51-70: Consider adding error boundary for upload failuresWhile the error handling is delegated to the uploadFile utility, it would be beneficial to add an error boundary to gracefully handle any unhandled exceptions during the upload process.
Consider wrapping the avatar upload functionality in an error boundary component:
class AvatarUploadErrorBoundary extends React.Component { componentDidCatch(error, errorInfo) { Notification.Error({ msg: "Failed to upload avatar. Please try again." }); console.error(error, errorInfo); } render() { return this.props.children; } }
Line range hint
73-84: Add loading state for delete operationThe delete operation sets isProcessing but doesn't reflect this in the UI. Consider showing a loading indicator during deletion.
Apply this diff:
const handleAvatarDelete = async (onError: () => void) => { const { res } = await request(routes.deleteProfilePicture, { pathParams: { username }, }); if (res?.ok) { Notification.Success({ msg: "Profile picture deleted" }); refetchUserData?.(); setEditAvatar(false); } else { onError(); } };src/components/Common/AvatarEditModal.tsx (1)
117-135: LGTM: Robust error handling with guaranteed state cleanupThe try/finally block ensures that loading states are properly reset even if an error occurs during upload.
However, consider adding a catch block to provide user feedback for unexpected errors:
try { if (!selectedFile) { closeModal(); return; } setIsProcessing(true); setIsCaptureImgBeingUploaded(true); await handleUpload(selectedFile, () => { setSelectedFile(undefined); setPreview(undefined); setPreviewImage(null); setIsCaptureImgBeingUploaded(false); setIsProcessing(false); }); + } catch (error) { + Notification.Error({ msg: "An unexpected error occurred while uploading the avatar." }); } finally { setIsCaptureImgBeingUploaded(false); setIsProcessing(false); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Utils/request/uploadFile.ts(1 hunks)src/components/Common/AvatarEditModal.tsx(1 hunks)src/components/Users/UserAvatar.tsx(4 hunks)
🔇 Additional comments (3)
src/Utils/request/uploadFile.ts (2)
6-14: LGTM: Function signature properly reflects async nature
The updated function signature correctly indicates that this is an asynchronous operation returning a Promise.
46-51: LGTM: Network error handling is complete
The error handling for network failures is comprehensive, including user notification, error callback, and promise rejection.
src/components/Users/UserAvatar.tsx (1)
22-28: LGTM: Props interface properly typed
The optional refetchUserData prop is correctly typed with TypeScript.
src/Utils/request/uploadFile.ts
Outdated
| xhr.onload = () => { | ||
| onLoad(xhr); | ||
| if (400 <= xhr.status && xhr.status <= 499) { | ||
| const error = JSON.parse(xhr.responseText); | ||
| if (typeof error === "object" && !Array.isArray(error)) { | ||
| Object.values(error).forEach((msg) => { | ||
| Notification.Error({ msg: msg || "Something went wrong!" }); | ||
| }); | ||
| } else { | ||
| Notification.Error({ msg: error || "Something went wrong!" }); | ||
| } | ||
| reject(new Error("Client error")); | ||
| } else { | ||
| Notification.Error({ msg: error || "Something went wrong!" }); | ||
| resolve(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Enhance error handling for non-JSON responses
The current implementation assumes that all error responses are JSON-parseable. This could throw an exception if the server returns a plain text error.
Apply this diff to safely handle non-JSON responses:
xhr.onload = () => {
onLoad(xhr);
if (400 <= xhr.status && xhr.status <= 499) {
- const error = JSON.parse(xhr.responseText);
+ let error;
+ try {
+ error = JSON.parse(xhr.responseText);
+ } catch {
+ error = xhr.responseText;
+ }
if (typeof error === "object" && !Array.isArray(error)) {
Object.values(error).forEach((msg) => {
Notification.Error({ msg: msg || "Something went wrong!" });Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Utils/request/uploadFile.ts (3)
15-21: Consider adding timeout and abort handlingWhile the XHR setup is correct, consider adding:
- Timeout handling to prevent indefinite waiting
- AbortController integration to allow cleanup
const uploadFile = async ( url: string, file: File | FormData, reqMethod: string, headers: object, onLoad: (xhr: XMLHttpRequest) => void, setUploadPercent: Dispatch<SetStateAction<number>> | null, - onError: () => void, + onError: () => void, + timeout: number = 30000, ): Promise<void> => { return new Promise((resolve, reject) => { const xhr = new XMLHttpRequest(); + xhr.timeout = timeout; xhr.open(reqMethod, url); Object.entries(headers).forEach(([key, value]) => { xhr.setRequestHeader(key, value); }); + + xhr.ontimeout = () => { + Notification.Error({ msg: "Upload timed out. Please try again." }); + onError(); + reject(new Error("Timeout")); + };
45-48: Consider cleanup for progress handlerWhile the progress handling works correctly, consider cleaning up the progress handler when the upload is complete or errors out to prevent potential memory leaks or updates after component unmount.
if (setUploadPercent != null) { xhr.upload.onprogress = (event: ProgressEvent) => { handleUploadPercentage(event, setUploadPercent); }; + + // Clean up progress handler + xhr.onloadend = () => { + xhr.upload.onprogress = null; + }; }
59-59: Consider adding file type validationTo prevent invalid uploads early, consider adding type validation before sending the file.
+ if (!(file instanceof FormData) && !(file instanceof File)) { + reject(new Error("Invalid file type")); + return; + } xhr.send(file);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Utils/request/uploadFile.ts(1 hunks)
🔇 Additional comments (4)
src/Utils/request/uploadFile.ts (4)
6-14: LGTM! Good improvement to async/await pattern
The function signature has been properly updated to return a Promise, which better aligns with modern JavaScript practices and makes error handling more straightforward for consumers.
23-43: Well-implemented error handling with proper fallbacks
The error handling implementation is robust:
- Safely handles both JSON and non-JSON error responses
- Provides user-friendly error messages
- Properly resolves/rejects the promise based on response status
51-56: Good error handling implementation
The error handler properly addresses the PR objective of fixing stuck states by:
- Showing clear user feedback about network issues
- Calling the error callback to reset UI state
- Rejecting the promise to allow proper error handling upstream
6-60: Verify integration with avatar components
The changes look good and align with the PR objectives. To ensure complete resolution of the stuck state issue, let's verify the integration with the avatar components.
✅ Verification successful
Avatar upload implementation is properly integrated
The verification shows that both UserAvatar and UserProfile components properly integrate with the uploadFile utility:
- Both components handle file uploads through FormData
- Error handling is managed through the onError callback
- Loading states are tracked in components
- The uploadFile utility's error notifications will be displayed as implemented
The changes in uploadFile.ts properly address the avatar upload stuck state by:
- Properly rejecting promises on errors
- Handling network failures
- Providing upload progress feedback when requested
- Showing appropriate error notifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for avatar-related components that use this utility
rg -l "uploadFile.*avatar" --type ts --type tsx
# Check error handling in avatar components
ast-grep --pattern 'try {
$$$
uploadFile($$$)
$$$
} catch {
$$$
}'
Length of output: 154
Script:
#!/bin/bash
# Search for files containing uploadFile usage
rg -l "uploadFile"
# Search for avatar-related files
rg -l "avatar" --type ts
# Check error handling patterns around uploadFile
rg -B2 -A2 "uploadFile"
Length of output: 6759
Script:
#!/bin/bash
# Check error handling in UserAvatar.tsx
rg -B5 -A5 "uploadFile" src/components/Users/UserAvatar.tsx
# Check error handling in UserProfile.tsx
rg -B5 -A5 "uploadFile" src/components/Users/UserProfile.tsx
# Check state management in these components
rg "useState.*upload|loading|progress" src/components/Users/UserAvatar.tsx src/components/Users/UserProfile.tsx
Length of output: 1908
|
LGTM |
|
@shauryag2002 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
Video of Solution:
fix_uploading_state_and_header.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
refetchUserDataparameter to theUserAvatarcomponent to refresh user data after avatar updates.Bug Fixes
AvatarEditModalto ensure UI reflects the correct status during upload attempts.Documentation