Add avatar to the onboarding flow #1113
Conversation
|
@sahilpatel2712 is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
sahilpatel2712
temporarily deployed
to
production
GitHub Actions
Inactive
WalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
app/(app)/alpha/additional-details/page.tsx (2)
41-41: LGTM: Image field added to detailsWithNullsRemoved objectThe addition of the
imagefield to thedetailsWithNullsRemovedobject is correct and consistent with the handling of other fields. This ensures that the user's image is passed to theContentcomponent, which is crucial for displaying the avatar in the onboarding flow.For consistency with TypeScript best practices, consider using nullish coalescing (
??) instead of logical OR (||) for the default value. This would handle cases where an empty string is a valid value:image: details?.image ?? "",This suggestion is optional and doesn't affect the functionality of the current implementation.
Line range hint
1-44: Overall implementation looks good, consider additional enhancementsThe changes successfully implement the retrieval and passing of the user's image to the Content component, aligning well with the PR objective of adding an avatar to the onboarding flow. The implementation is consistent with the existing code structure and error handling approach.
To further enhance the feature, consider the following suggestions:
- Ensure that the Content component is updated to display the avatar using the newly passed image field.
- If not already implemented elsewhere, consider adding functionality for image upload during the onboarding process.
- Add error handling for cases where the image retrieval fails or the image is in an unsupported format.
app/(app)/alpha/additional-details/_client.tsx (4)
152-157: Simplify theonErrorhandler in mutationThe
onErrorhandler always receives anerrorobject, so the checkif (error)is unnecessary. Simplify the code by removing this conditional.Here's the simplified code:
onError(error) { - if (error) return toast.error(error.message); - return toast.error( - "Something went wrong uploading the photo, please retry.", - ); + toast.error(error.message); },This makes the error handling clearer and more concise.
210-210: Remove unnecessary template literal inAvatarcomponentSince
profilePhoto.urlis already a string, you can pass it directly to thesrcprop without using a template literal.Apply this diff to simplify the code:
? undefined - : `${profilePhoto.url}` + : profilePhoto.urlThis avoids unnecessary string interpolation.
Line range hint
265-267: Adjust progress calculation to reflect the correct number of slidesThe
SignupProgressBarcomponent calculates progress based on(100 / 4) * currentSlide, but there are only 3 slides in the onboarding process. This causes the progress to never reach 100%. Adjust the calculation to use the correct number of slides.Here's how you can fix it:
function SignupProgressBar({ currentSlide }: { currentSlide: number }) { - const progressPercentage = (100 / 4) * currentSlide; + const progressPercentage = (100 / 3) * currentSlide; return ( <div className="flex h-2">This change ensures the progress bar accurately reflects the user's progress.
Line range hint
296-316: Fix logic inversion in form validation for SlideThreeIn the
onFormSubmitfunction, thetriggermethod returnstruewhen the validation passes, but the variableisErrorsuggests the opposite. This inversion can lead to unintended behavior during form submission.Here's how you can correct the logic:
const onFormSubmit = async (data: TypeSlideThreeSchema) => { - let isError = await trigger(["professionalOrStudent"]); + let isValid = await trigger(["professionalOrStudent"]); const professionalOrStudent = getValues("professionalOrStudent"); - if (isError && professionalOrStudent === "Working professional") { - isError = await trigger(["workplace", "jobTitle"]); + if (isValid && professionalOrStudent === "Working professional") { + isValid = await trigger(["workplace", "jobTitle"]); } - if (isError && professionalOrStudent === "Current student") { - isError = await trigger(["levelOfStudy", "course"]); + if (isValid && professionalOrStudent === "Current student") { + isValid = await trigger(["levelOfStudy", "course"]); } - if (isError) { + if (isValid) { try { const isSuccess = await slideThreeSubmitAction(data); if (isSuccess) { toast.success("Saved"); router.push(`/`, { scroll: false }); } else { toast.error("Error, saving was unsuccessful."); } } catch (error) { toast.error("An unexpected error occurred."); } } };This ensures that the form submission only proceeds when validation passes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/(app)/alpha/additional-details/_client.tsx (6 hunks)
- app/(app)/alpha/additional-details/page.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
app/(app)/alpha/additional-details/page.tsx (1)
25-25: LGTM: Image field added to database queryThe addition of the
image: truefield to thecolumnsobject in the database query is correct and aligns well with the PR objective of adding an avatar to the onboarding flow. This change enables the retrieval of the user's image from the database, which is essential for displaying the avatar in the onboarding process.
| const { mutate: getUploadUrl } = api.profile.getUploadUrl.useMutation(); | ||
| const { mutate: updateUserPhotoUrl } = | ||
| api.profile.updateProfilePhotoUrl.useMutation(); |
There was a problem hiding this comment.
Use mutateAsync instead of mutate when awaiting mutation results
To properly await the results of mutations and handle potential errors, use mutateAsync instead of mutate. This ensures that you can await the promise returned by the mutation functions.
Apply this diff to fix the issue:
- const { mutate: getUploadUrl } = api.profile.getUploadUrl.useMutation();
- const { mutate: updateUserPhotoUrl } =
- api.profile.updateProfilePhotoUrl.useMutation();
+ const { mutateAsync: getUploadUrl } = api.profile.getUploadUrl.useMutation();
+ const { mutateAsync: updateUserPhotoUrl } =
+ api.profile.updateProfilePhotoUrl.useMutation();This change allows you to use await getUploadUrl(...) and await updateUserPhotoUrl(...) as intended.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { mutate: getUploadUrl } = api.profile.getUploadUrl.useMutation(); | |
| const { mutate: updateUserPhotoUrl } = | |
| api.profile.updateProfilePhotoUrl.useMutation(); | |
| const { mutateAsync: getUploadUrl } = api.profile.getUploadUrl.useMutation(); | |
| const { mutateAsync: updateUserPhotoUrl } = | |
| api.profile.updateProfilePhotoUrl.useMutation(); |
| if (e.target.files && e.target.files.length > 0) { | ||
| const file = e.target.files[0]; | ||
| const { size, type } = file; |
There was a problem hiding this comment.
Add file size validation to enforce maximum upload size
Currently, there is no validation on the file size. Since the UI indicates a maximum file size of 1MB, consider adding a check to ensure that the uploaded file does not exceed this limit.
Here's how you can add the validation:
const imageChange = async (e: React.ChangeEvent<HTMLInputElement>) => {
if (e.target.files && e.target.files.length > 0) {
const file = e.target.files[0];
const { size, type } = file;
+ // Check file size (1MB = 1,048,576 bytes)
+ if (size > 1048576) {
+ toast.error("File size exceeds 1MB. Please upload a smaller image.");
+ return;
+ }
await getUploadUrl(
{ size, type },
{
// ...This ensures users are notified if the selected file exceeds the allowed size.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (e.target.files && e.target.files.length > 0) { | |
| const file = e.target.files[0]; | |
| const { size, type } = file; | |
| if (e.target.files && e.target.files.length > 0) { | |
| const file = e.target.files[0]; | |
| const { size, type } = file; | |
| // Check file size (1MB = 1,048,576 bytes) | |
| if (size > 1048576) { | |
| toast.error("File size exceeds 1MB. Please upload a smaller image."); | |
| return; | |
| } |
Add file type validation to ensure accepted image formats
While the accept attribute on the <Input> element filters the file selection dialog, users might bypass this restriction. Add a check to validate the file's MIME type before uploading.
Here's how you can add the validation:
const imageChange = async (e: React.ChangeEvent<HTMLInputElement>) => {
if (e.target.files && e.target.files.length > 0) {
const file = e.target.files[0];
const { size, type } = file;
+ // Validate file type
+ const acceptedTypes = ["image/png", "image/gif", "image/jpeg"];
+ if (!acceptedTypes.includes(type)) {
+ toast.error("Invalid file type. Please upload a JPG, GIF, or PNG image.");
+ return;
+ }
// Check file size (1MB = 1,048,576 bytes)
if (size > 1048576) {
toast.error("File size exceeds 1MB. Please upload a smaller image.");
return;
}
await getUploadUrl(
{ size, type },
{
// ...This ensures only valid image formats are processed.
Committable suggestion was skipped due to low confidence.
| const uploadToUrl = async (signedUrl: string, file: File) => { | ||
| setProfilePhoto({ status: "loading", url: "" }); | ||
|
|
||
| if (!file) { | ||
| setProfilePhoto({ status: "error", url: "" }); | ||
| toast.error("Invalid file upload."); | ||
| return; | ||
| } | ||
|
|
||
| const response = await uploadFile(signedUrl, file); | ||
| const { fileLocation } = response; | ||
| await updateUserPhotoUrl({ | ||
| url: fileLocation, | ||
| }); | ||
|
|
||
| return fileLocation; | ||
| }; |
There was a problem hiding this comment.
Handle errors from uploadFile and updateUserPhotoUrl functions
Currently, the uploadToUrl function does not handle errors that might occur during the file upload or while updating the user's photo URL. To improve robustness, consider adding error handling to these operations.
Here's how you can enhance error handling:
const uploadToUrl = async (signedUrl: string, file: File) => {
setProfilePhoto({ status: "loading", url: "" });
if (!file) {
setProfilePhoto({ status: "error", url: "" });
toast.error("Invalid file upload.");
return;
}
+ try {
const response = await uploadFile(signedUrl, file);
const { fileLocation } = response;
await updateUserPhotoUrl({
url: fileLocation,
});
return fileLocation;
+ } catch (error) {
+ setProfilePhoto({ status: "error", url: "" });
+ toast.error("Failed to upload profile photo. Please try again.");
+ return null;
+ }
};This ensures that any errors during the upload process are properly handled and communicated to the user.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const uploadToUrl = async (signedUrl: string, file: File) => { | |
| setProfilePhoto({ status: "loading", url: "" }); | |
| if (!file) { | |
| setProfilePhoto({ status: "error", url: "" }); | |
| toast.error("Invalid file upload."); | |
| return; | |
| } | |
| const response = await uploadFile(signedUrl, file); | |
| const { fileLocation } = response; | |
| await updateUserPhotoUrl({ | |
| url: fileLocation, | |
| }); | |
| return fileLocation; | |
| }; | |
| const uploadToUrl = async (signedUrl: string, file: File) => { | |
| setProfilePhoto({ status: "loading", url: "" }); | |
| if (!file) { | |
| setProfilePhoto({ status: "error", url: "" }); | |
| toast.error("Invalid file upload."); | |
| return; | |
| } | |
| try { | |
| const response = await uploadFile(signedUrl, file); | |
| const { fileLocation } = response; | |
| await updateUserPhotoUrl({ | |
| url: fileLocation, | |
| }); | |
| return fileLocation; | |
| } catch (error) { | |
| setProfilePhoto({ status: "error", url: "" }); | |
| toast.error("Failed to upload profile photo. Please try again."); | |
| return null; | |
| } | |
| }; |
|
Hey @sahilpatel2712 Also, the alignment of the elements you added are not visually balanced. Again, I realise this code comes directly from the existing settings page, however in this case the styles should be customised for this form. Thanks, |
sahilpatel2712
had a problem deploying
to
production
GitHub Actions
Error
sahilpatel2712
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
app/(app)/alpha/additional-details/_client.tsx (2)
207-245: Improve alignment of avatar upload sectionTo enhance the visual balance of the avatar upload section, consider adjusting the alignment of the "Change avatar" button and the instruction text. Here's a suggested improvement:
<div className="mx-4 my-4"> <Field className="flex-grow"> <Label>Profile Picture</Label> - <div className="mt-3 flex items-center justify-between gap-4 px-3"> + <div className="mt-3 flex items-center gap-4 px-3"> <Avatar square src={ profilePhoto.status === "error" || profilePhoto.status === "loading" ? undefined : `${profilePhoto.url}` } alt="Profile photo upload section" className="h-16 w-16 overflow-hidden rounded-full" /> - <div className="pt-[30px]"> + <div className="flex flex-col items-start justify-center"> <Button color="dark/white" type="button" - className="h-[30px] rounded-md text-xs" + className="mb-1 h-[30px] rounded-md text-xs" onClick={() => fileInputRef.current?.click()} > Change avatar </Button> <Input type="file" id="file-input" name="user-photo" accept="image/png, image/gif, image/jpeg" onChange={imageChange} className="hidden" ref={fileInputRef} /> - <Text className="mt-1 text-xs text-gray-500"> + <Text className="text-xs text-gray-500"> JPG, GIF or PNG. 10MB max. </Text> </div> </div> </Field> </div>These changes will align the "Change avatar" button and instruction text with the avatar, creating a more cohesive look.
Inconsistent File Size Limits Detected
- The UI text indicates a 1MB maximum file size.
- Other parts of the codebase enforce 10MB and 20MB limits.
Affected Files:
server/api/router/profile.ts: "Maximum file size 10mb"components/editor/editor/plugins/upload-images.tsx: "File size too big (max 20MB)."🔗 Analysis chain
Line range hint
3-45: Update file size limit in UI textThe UI text mentions a 10MB max file size, but the previous review comments suggest a 1MB limit. Ensure consistency between the implemented limit and the UI text.
- JPG, GIF or PNG. 10MB max. + JPG, GIF or PNG. 1MB max.Also applies to: 58-63
Let's correct the shell script to use the appropriate file type identifiers and re-run the search for file size limits.
Let's adjust the shell script to specify file extensions directly and re-run the search for file size limits.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other occurrences of file size limits rg -i "file.?size|size.?limit" --type typescript --type javascriptLength of output: 103
Script:
#!/bin/bash # Check for other occurrences of file size limits with correct file types rg -i "file.?size|size.?limit" --type ts --type tsx --type js --type jsxLength of output: 102
Script:
#!/bin/bash # Check for other occurrences of file size limits using file extensions rg -i "file.?size|size.?limit" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx"Length of output: 559
| <Avatar | ||
| square | ||
| src={ | ||
| profilePhoto.status === "error" || | ||
| profilePhoto.status === "loading" | ||
| ? undefined | ||
| : `${profilePhoto.url}` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading indicator for avatar upload
To improve user experience, consider adding a loading indicator when the avatar is being uploaded. This can be done by utilizing the profilePhoto.status state:
<Avatar
square
src={
profilePhoto.status === "error" ||
profilePhoto.status === "loading"
? undefined
: `${profilePhoto.url}`
}
alt="Profile photo upload section"
- className="h-16 w-16 overflow-hidden rounded-full"
+ className={`h-16 w-16 overflow-hidden rounded-full ${
+ profilePhoto.status === "loading" ? "animate-pulse bg-gray-200" : ""
+ }`}
/>
+{profilePhoto.status === "loading" && (
+ <div className="absolute inset-0 flex items-center justify-center">
+ <span className="loading loading-spinner loading-md"></span>
+ </div>
+)}This will show a pulsing animation and a spinner when the avatar is being uploaded, providing visual feedback to the user.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Avatar | |
| square | |
| src={ | |
| profilePhoto.status === "error" || | |
| profilePhoto.status === "loading" | |
| ? undefined | |
| : `${profilePhoto.url}` | |
| } | |
| <Avatar | |
| square | |
| src={ | |
| profilePhoto.status === "error" || | |
| profilePhoto.status === "loading" | |
| ? undefined | |
| : `${profilePhoto.url}` | |
| } | |
| alt="Profile photo upload section" | |
| className={`h-16 w-16 overflow-hidden rounded-full ${ | |
| profilePhoto.status === "loading" ? "animate-pulse bg-gray-200" : "" | |
| }`} | |
| /> | |
| {profilePhoto.status === "loading" && ( | |
| <div className="absolute inset-0 flex items-center justify-center"> | |
| <span className="loading loading-spinner loading-md"></span> | |
| </div> | |
| )} |
| const uploadToUrl = async (signedUrl: string, file: File) => { | ||
| setProfilePhoto({ status: "loading", url: "" }); | ||
|
|
||
| if (!file) { | ||
| setProfilePhoto({ status: "error", url: "" }); | ||
| toast.error("Invalid file upload."); | ||
| return; | ||
| } | ||
| try { | ||
| const response = await uploadFile(signedUrl, file); | ||
| const { fileLocation } = response; | ||
| await updateUserPhotoUrl({ | ||
| url: fileLocation, | ||
| }); | ||
|
|
||
| return fileLocation; | ||
| } catch (error) { | ||
| setProfilePhoto({ status: "error", url: "" }); | ||
| toast.error("Failed to upload profile photo. Please try again."); | ||
| return null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in uploadToUrl function
Consider adding more specific error handling to provide better feedback to the user. For example:
try {
const response = await uploadFile(signedUrl, file);
const { fileLocation } = response;
await updateUserPhotoUrl({
url: fileLocation,
});
return fileLocation;
} catch (error) {
setProfilePhoto({ status: "error", url: "" });
- toast.error("Failed to upload profile photo. Please try again.");
+ if (error instanceof Error) {
+ toast.error(`Failed to upload profile photo: ${error.message}`);
+ } else {
+ toast.error("Failed to upload profile photo. Please try again.");
+ }
return null;
}This will provide more specific error messages when possible.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const uploadToUrl = async (signedUrl: string, file: File) => { | |
| setProfilePhoto({ status: "loading", url: "" }); | |
| if (!file) { | |
| setProfilePhoto({ status: "error", url: "" }); | |
| toast.error("Invalid file upload."); | |
| return; | |
| } | |
| try { | |
| const response = await uploadFile(signedUrl, file); | |
| const { fileLocation } = response; | |
| await updateUserPhotoUrl({ | |
| url: fileLocation, | |
| }); | |
| return fileLocation; | |
| } catch (error) { | |
| setProfilePhoto({ status: "error", url: "" }); | |
| toast.error("Failed to upload profile photo. Please try again."); | |
| return null; | |
| } | |
| }; | |
| const uploadToUrl = async (signedUrl: string, file: File) => { | |
| setProfilePhoto({ status: "loading", url: "" }); | |
| if (!file) { | |
| setProfilePhoto({ status: "error", url: "" }); | |
| toast.error("Invalid file upload."); | |
| return; | |
| } | |
| try { | |
| const response = await uploadFile(signedUrl, file); | |
| const { fileLocation } = response; | |
| await updateUserPhotoUrl({ | |
| url: fileLocation, | |
| }); | |
| return fileLocation; | |
| } catch (error) { | |
| setProfilePhoto({ status: "error", url: "" }); | |
| if (error instanceof Error) { | |
| toast.error(`Failed to upload profile photo: ${error.message}`); | |
| } else { | |
| toast.error("Failed to upload profile photo. Please try again."); | |
| } | |
| return null; | |
| } | |
| }; |
| const imageChange = async (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| if (e.target.files && e.target.files.length > 0) { | ||
| const file = e.target.files[0]; | ||
| const { size, type } = file; | ||
|
|
||
| await getUploadUrl( | ||
| { size, type }, | ||
| { | ||
| onError(error) { | ||
| if (error) return toast.error(error.message); | ||
| return toast.error( | ||
| "Something went wrong uploading the photo, please retry.", | ||
| ); | ||
| }, | ||
| async onSuccess(signedUrl) { | ||
| const url = await uploadToUrl(signedUrl, file); | ||
| if (!url) { | ||
| return toast.error( | ||
| "Something went wrong uploading the photo, please retry.", | ||
| ); | ||
| } | ||
| setProfilePhoto({ status: "success", url }); | ||
| toast.success( | ||
| "Profile photo successfully updated. This may take a few minutes to update around the site.", | ||
| ); | ||
| }, | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Implement file type and size validation
As suggested in previous comments, add file type and size validation before uploading:
const imageChange = async (e: React.ChangeEvent<HTMLInputElement>) => {
if (e.target.files && e.target.files.length > 0) {
const file = e.target.files[0];
const { size, type } = file;
+ // Validate file type
+ const acceptedTypes = ["image/png", "image/gif", "image/jpeg"];
+ if (!acceptedTypes.includes(type)) {
+ toast.error("Invalid file type. Please upload a JPG, GIF, or PNG image.");
+ return;
+ }
+
+ // Validate file size (1MB = 1,048,576 bytes)
+ if (size > 1048576) {
+ toast.error("File size exceeds 1MB. Please upload a smaller image.");
+ return;
+ }
await getUploadUrl(
{ size, type },
{
// ... rest of the functionThis ensures that only valid file types and sizes are processed, improving user experience and reducing server load.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const imageChange = async (e: React.ChangeEvent<HTMLInputElement>) => { | |
| if (e.target.files && e.target.files.length > 0) { | |
| const file = e.target.files[0]; | |
| const { size, type } = file; | |
| await getUploadUrl( | |
| { size, type }, | |
| { | |
| onError(error) { | |
| if (error) return toast.error(error.message); | |
| return toast.error( | |
| "Something went wrong uploading the photo, please retry.", | |
| ); | |
| }, | |
| async onSuccess(signedUrl) { | |
| const url = await uploadToUrl(signedUrl, file); | |
| if (!url) { | |
| return toast.error( | |
| "Something went wrong uploading the photo, please retry.", | |
| ); | |
| } | |
| setProfilePhoto({ status: "success", url }); | |
| toast.success( | |
| "Profile photo successfully updated. This may take a few minutes to update around the site.", | |
| ); | |
| }, | |
| }, | |
| ); | |
| } | |
| const imageChange = async (e: React.ChangeEvent<HTMLInputElement>) => { | |
| if (e.target.files && e.target.files.length > 0) { | |
| const file = e.target.files[0]; | |
| const { size, type } = file; | |
| // Validate file type | |
| const acceptedTypes = ["image/png", "image/gif", "image/jpeg"]; | |
| if (!acceptedTypes.includes(type)) { | |
| toast.error("Invalid file type. Please upload a JPG, GIF, or PNG image."); | |
| return; | |
| } | |
| // Validate file size (1MB = 1,048,576 bytes) | |
| if (size > 1048576) { | |
| toast.error("File size exceeds 1MB. Please upload a smaller image."); | |
| return; | |
| } | |
| await getUploadUrl( | |
| { size, type }, | |
| { | |
| onError(error) { | |
| if (error) return toast.error(error.message); | |
| return toast.error( | |
| "Something went wrong uploading the photo, please retry.", | |
| ); | |
| }, | |
| async onSuccess(signedUrl) { | |
| const url = await uploadToUrl(signedUrl, file); | |
| if (!url) { | |
| return toast.error( | |
| "Something went wrong uploading the photo, please retry.", | |
| ); | |
| } | |
| setProfilePhoto({ status: "success", url }); | |
| toast.success( | |
| "Profile photo successfully updated. This may take a few minutes to update around the site.", | |
| ); | |
| }, | |
| }, | |
| ); | |
| } |
|
Hi @John-Paul-Larkin
|
|
Uh oh! @sahilpatel2712, the image you shared is missing helpful alt text. Check #1113 (comment). Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs. |
3 similar comments
|
Uh oh! @sahilpatel2712, the image you shared is missing helpful alt text. Check #1113 (comment). Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs. |
|
Uh oh! @sahilpatel2712, the image you shared is missing helpful alt text. Check #1113 (comment). Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs. |
|
Uh oh! @sahilpatel2712, the image you shared is missing helpful alt text. Check #1113 (comment). Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs. |
John-Paul-Larkin
left a comment
John-Paul-Larkin
left a comment
There was a problem hiding this comment.
Hey @sahilpatel2712
Thank you for implementing the recent changes.
At this point, the uploadToUrl functionality is being used both on the settings page and the additional-details page. However, the code has started to diverge due to the error handling modifications you introduced.
Given this, it would make sense to refactor the logic into a shared utility function. Could you please extract this logic into a utility function and place it in the utils folder so it can be reused in both places?
Thanks!
sahilpatel2712
temporarily deployed
to
production
GitHub Actions
Inactive
sahilpatel2712
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
app/(app)/alpha/additional-details/_client.tsx (1)
112-120: LGTM: File upload setup in SlideOne functionThe changes in the
SlideOnefunction are well-implemented:
- The
fileInputRefis correctly used for the file input.- The
profilePhotostate is appropriately managed with useState.- The tRPC mutations for getting the upload URL and updating the user's photo URL are correctly implemented.
Consider adding error handling for the
updateUserPhotoUrlmutation, similar to how you've handled errors forgetUploadUrl. This would provide a more consistent error handling approach throughout the component.utils/fileUpload.ts (3)
12-15: Consider removing the'loading'status fromUploadResultThe
statustype includes'loading', but theuploadToUrlfunction only returns'success'or'error'. Removing'loading'from thestatustype can simplify the code and avoid confusion.
35-38: Log errors in the catch block for better debuggingCapturing and logging the error can aid in diagnosing issues during the upload process. This information can be invaluable during development and troubleshooting.
For example:
} catch (error) { console.error("Error uploading profile photo:", error); toast.error("Failed to upload profile photo. Please try again."); return { status: "error", fileLocation: null }; }
7-9: Simplify the return type ofupdateUserPhotoUrlif return values are unusedIf the returned values
'role','id', and'name'fromupdateUserPhotoUrlare not utilized in this context, consider changing the return type toPromise<void>for clarity and simplicity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- app/(app)/alpha/additional-details/_client.tsx (6 hunks)
- app/(app)/settings/_client.tsx (3 hunks)
- utils/fileUpload.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
app/(app)/alpha/additional-details/_client.tsx (2)
3-3: LGTM: New imports and type definitions for profile picture featureThe new imports and type definitions are well-structured and appropriate for implementing the profile picture feature. The
UserDetailstype has been correctly updated to include theimageproperty, and the newProfilePhototype is well-defined for managing the upload state.Also applies to: 42-45, 58-63
Line range hint
1-729: Overall assessment: Good implementation with room for minor improvementsThe implementation of the profile picture upload feature is well-structured and functional. The changes include appropriate imports, type definitions, and UI components. The
SlideOnefunction has been effectively updated to handle file uploads, and the newimageChangefunction manages the upload process.However, there are a few areas that could be improved:
- Add file type and size validation before upload.
- Enhance error handling with more specific error messages.
- Improve UI feedback during the upload process (loading states, button disabling).
- Ensure consistency between the stated and implemented file size limits.
Addressing these points will result in a more robust and user-friendly implementation. Great work overall!
app/(app)/settings/_client.tsx (4)
27-27: Import statement foruploadToUrladded correctlyThe addition of the import statement for
uploadToUrlfrom@/utils/fileUploadis appropriate and necessary for handling file uploads.
112-112: Initializing profile photo upload stateSetting the profile photo status to
"loading"and clearing the URL before starting the upload process is appropriate. This provides immediate feedback to the user that the upload is in progress.
125-129: Verify parameters passed touploadToUrlEnsure that the
uploadToUrlfunction accepts the parameters{ signedUrl, file, updateUserPhotoUrl }and thatupdateUserPhotoUrlis used correctly within this function. SinceupdateUserPhotoUrlis now usingmutateAsync, confirm that it is awaited appropriately insideuploadToUrl.
131-134: Handling upload success and error states appropriatelyThe logic for updating the profile photo state based on the result of the upload is correctly implemented. It sets the state to
"success"and updates the URL when the upload is successful, and sets it to"error"otherwise.
| if (!file) { | ||
| toast.error("Invalid file upload."); | ||
| return { status: "error", fileLocation: null }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for file type and size before uploading
Currently, the code only checks if a file is provided. To enhance security and user experience, consider validating the file's type and size before uploading. This can prevent unsupported file formats and oversized files from causing issues.
For example:
if (!file) {
toast.error("No file selected for upload.");
return { status: "error", fileLocation: null };
}
const allowedTypes = ["image/jpeg", "image/png"];
const maxSize = 5 * 1024 * 1024; // 5 MB
if (!allowedTypes.includes(file.type)) {
toast.error("Invalid file type. Please upload a JPEG or PNG image.");
return { status: "error", fileLocation: null };
}
if (file.size > maxSize) {
toast.error("File size exceeds the 5MB limit. Please upload a smaller file.");
return { status: "error", fileLocation: null };
}
sahilpatel2712
temporarily deployed
to
production
GitHub Actions
Inactive
sahilpatel2712
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
app/(app)/settings/_client.tsx (1)
112-134: LGTM: Improved error handling and state management inimageChangeThe
imageChangefunction has been significantly improved:
- The
profilePhotostate is now set to "loading" before starting the upload process.- Error handling has been enhanced with more specific error messages.
- The
uploadToUrlfunction is called with the correct parameters.- The success case properly sets the profile photo URL based on the upload response.
These changes improve the user experience and error reporting.
Consider adding a more specific error message for the case when
statusis not "success":if (status === "success" && fileLocation) { setProfilePhoto({ status: "success", url: fileLocation }); } else { - setProfilePhoto({ status: "error", url: "" }); + setProfilePhoto({ status: "error", url: "" }); + toast.error("Failed to update profile photo. Please try again."); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- app/(app)/alpha/additional-details/_client.tsx (6 hunks)
- app/(app)/settings/_client.tsx (4 hunks)
- utils/fileUpload.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- utils/fileUpload.ts
🧰 Additional context used
🔇 Additional comments (5)
app/(app)/settings/_client.tsx (2)
27-27: LGTM: Import statement updated correctlyThe import statement has been updated to use
uploadToUrlfrom@/utils/fileUpload, which aligns with the changes in theimageChangefunction.
219-219: Verify server-side file size limitThe UI text has been updated to indicate a maximum file size of 10MB, which is a user-friendly improvement. However, we need to ensure that the server-side validation also supports this new limit.
Let's verify the server-side file size limit:
Please confirm that the server-side validation has been updated to allow files up to 10MB.
app/(app)/alpha/additional-details/_client.tsx (3)
3-3: New imports added for profile picture functionalityThe new imports for Avatar, Text, api, and uploadToUrl are correctly added to support the new profile picture upload feature.
Also applies to: 42-45
58-59: New types added for profile picture supportThe
UserDetailstype has been correctly updated to include theimageproperty, and a newProfilePhototype has been added to manage the profile photo upload state.Also applies to: 61-63
Line range hint
1-729: Overall assessment of profile picture upload implementationThe implementation of the profile picture upload feature in the
SlideOnecomponent is generally well-done and addresses the main objective of the PR. The changes include:
- Adding necessary imports and type definitions
- Implementing state management for the profile photo
- Adding mutations for URL generation and photo upload
- Creating an
imageChangefunction to handle the upload process- Updating the UI to include the avatar and upload functionality
However, there are some areas for improvement:
- File type and size validation should be added before initiating the upload
- Error handling can be enhanced to provide more specific feedback
- UI can be improved to better handle loading states and provide more accurate information
Despite these minor issues, the implementation is functional and achieves the desired outcome. I recommend addressing the suggested improvements to enhance the robustness and user experience of the feature.
app/(app)/settings/_client.tsx
Outdated
| }); | ||
|
|
||
| const { mutate, isError, isSuccess } = api.profile.edit.useMutation(); | ||
| const { mutate: getUploadUrl } = api.profile.getUploadUrl.useMutation(); |
There was a problem hiding this comment.
Update remaining mutation hooks to use mutateAsync
For consistency with the updateUserPhotoUrl mutation and to improve error handling, please update the getUploadUrl and updateEmail mutation hooks to use mutateAsync:
-const { mutate: getUploadUrl } = api.profile.getUploadUrl.useMutation();
+const { mutateAsync: getUploadUrl } = api.profile.getUploadUrl.useMutation();
// ...
-const { mutate: updateEmail } = api.profile.updateEmail.useMutation();
+const { mutateAsync: updateEmail } = api.profile.updateEmail.useMutation();Additionally, refactor the imageChange and handleNewEmailUpdate functions to use try/catch with async/await when calling these mutations. For example:
const imageChange = async (e: React.ChangeEvent<HTMLInputElement>) => {
// ...
try {
const signedUrl = await getUploadUrl({ size, type });
const { status, fileLocation } = await uploadToUrl({
signedUrl,
file,
updateUserPhotoUrl,
});
// ... handle success
} catch (error) {
// ... handle error
}
};
const handleNewEmailUpdate = async () => {
// ... existing checks
try {
await updateEmail({ newEmail });
// ... handle success
} catch (error) {
// ... handle error
}
};This change will improve error handling and make the code more consistent throughout the component.
Also applies to: 82-82
| const { mutateAsync: updateUserPhotoUrl } = | ||
| api.profile.updateProfilePhotoUrl.useMutation(); | ||
| const { mutate: updateEmail } = api.profile.updateEmail.useMutation(); |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Update updateEmail to use mutateAsync
The updateEmail mutation hook is still using mutate. For consistent asynchronous handling, it should be updated to use mutateAsync similar to updateUserPhotoUrl.
- File:
app/(app)/settings/_client.tsx- Line: 81
🔗 Analysis chain
LGTM: Mutation hook updated to use mutateAsync
The updateUserPhotoUrl mutation hook has been correctly updated to use mutateAsync. This change allows for better handling of asynchronous operations.
Let's verify if similar changes have been applied to other mutation hooks in the file:
Let's attempt to verify the usage of mutateAsync in other mutation hooks by running an updated shell script:
Let's attempt to verify the usage of mutateAsync in other mutation hooks by running an updated shell script:
Let's attempt to verify the usage of mutateAsync in other mutation hooks by running an updated shell script:
Let's attempt to verify if there are other instances of mutateAsync in the mutation hooks by running an updated shell script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other mutation hooks have been updated to use mutateAsync
rg --type typescript 'const \{ mutate(Async)?: \w+ \} = api\.\w+\.\w+\.useMutation\(\);' app/\(app\)/settings/_client.tsx
Length of output: 158
Script:
#!/bin/bash
# Check if other mutation hooks have been updated to use mutateAsync
rg --type tsx 'const \{ mutateAsync: \w+ \} = api\.\w+\.\w+\.useMutation\(\);' app/(app)/settings/_client.tsx
Length of output: 246
Script:
#!/bin/bash
# Check if other mutation hooks have been updated to use mutateAsync
rg --type tsx 'const \{ mutateAsync: \w+ \} = api\.\w+\.\w+\.useMutation\(\);' 'app/\(app\)/settings/_client.tsx'
Length of output: 143
Script:
#!/bin/bash
# Check if other mutation hooks have been updated to use mutateAsync
rg -g "*.tsx" 'const \{ mutateAsync: \w+ \} = api\.\w+\.\w+\.useMutation\(\);' 'app/(app)/settings/_client.tsx'
Length of output: 113
Script:
#!/bin/bash
# Search for any usage of mutateAsync in mutation hooks within the specified file
rg -F 'mutateAsync' 'app/(app)/settings/_client.tsx'
Length of output: 98
| const imageChange = async (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| if (e.target.files && e.target.files.length > 0) { | ||
| const file = e.target.files[0]; | ||
| const { size, type } = file; | ||
|
|
||
| setProfilePhoto({ status: "loading", url: "" }); | ||
|
|
||
| await getUploadUrl( | ||
| { size, type }, | ||
| { | ||
| onError(error) { | ||
| toast.error( | ||
| error.message || | ||
| "Something went wrong uploading the photo, please retry.", | ||
| ); | ||
| setProfilePhoto({ status: "error", url: "" }); | ||
| }, | ||
| async onSuccess(signedUrl) { | ||
| const { status, fileLocation } = await uploadToUrl({ | ||
| signedUrl, | ||
| file, | ||
| updateUserPhotoUrl, | ||
| }); | ||
|
|
||
| if (status === "success" && fileLocation) { | ||
| setProfilePhoto({ status: "success", url: fileLocation }); | ||
| } else { | ||
| setProfilePhoto({ status: "error", url: "" }); | ||
| } | ||
| }, | ||
| }, | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Implementation of imageChange function
The imageChange function handles file selection and initiates the upload process. However, there are some improvements that can be made:
- File type and size validation are missing. Add these checks before initiating the upload:
const maxSize = 10 * 1024 * 1024; // 10MB
const allowedTypes = ['image/jpeg', 'image/png', 'image/gif'];
if (file.size > maxSize) {
toast.error("File size exceeds 10MB limit.");
return;
}
if (!allowedTypes.includes(file.type)) {
toast.error("Invalid file type. Please upload a JPG, GIF, or PNG image.");
return;
}- The error handling in the
onErrorcallback can be improved:
onError(error) {
toast.error(
error.message || "Failed to get upload URL. Please try again."
);
setProfilePhoto({ status: "error", url: "" });
},- Add a success notification after successful upload:
if (status === "success" && fileLocation) {
setProfilePhoto({ status: "success", url: fileLocation });
toast.success("Profile picture updated successfully!");
} else {
setProfilePhoto({ status: "error", url: "" });
}These changes will improve the user experience and make the function more robust.
| <div className="mx-4 my-4"> | ||
| <Field className="flex-grow"> | ||
| <Label>Profile Picture</Label> | ||
| <div className="mt-3 flex items-center justify-between gap-4 px-3"> | ||
| <Avatar | ||
| square | ||
| src={ | ||
| profilePhoto.status === "error" || | ||
| profilePhoto.status === "loading" | ||
| ? undefined | ||
| : `${profilePhoto.url}` | ||
| } | ||
| alt="Profile photo upload section" | ||
| className="h-16 w-16 overflow-hidden rounded-full" | ||
| /> | ||
| <div className="pt-[30px]"> | ||
| <Button | ||
| color="dark/white" | ||
| type="button" | ||
| className="h-[30px] rounded-md text-xs" | ||
| onClick={() => fileInputRef.current?.click()} | ||
| > | ||
| Change avatar | ||
| </Button> | ||
| <Input | ||
| type="file" | ||
| id="file-input" | ||
| name="user-photo" | ||
| accept="image/png, image/gif, image/jpeg" | ||
| onChange={imageChange} | ||
| className="hidden" | ||
| ref={fileInputRef} | ||
| /> | ||
| <Text className="mt-1 text-xs text-gray-500"> | ||
| JPG, GIF or PNG. 10MB max. | ||
| </Text> | ||
| </div> | ||
| </div> | ||
| </Field> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
UI implementation for profile picture upload
The UI implementation for profile picture upload is good, but could be improved:
- Add a loading state to the Avatar component:
<Avatar
square
src={profilePhoto.status === "loading" ? undefined : profilePhoto.url}
alt="Profile photo upload section"
className={`h-16 w-16 overflow-hidden rounded-full ${
profilePhoto.status === "loading" ? "animate-pulse" : ""
}`}
/>
{profilePhoto.status === "loading" && (
<div className="absolute inset-0 flex items-center justify-center">
<span className="loading loading-spinner loading-md"></span>
</div>
)}- Disable the "Change avatar" button during upload:
<Button
color="dark/white"
type="button"
className="h-[30px] rounded-md text-xs"
onClick={() => fileInputRef.current?.click()}
disabled={profilePhoto.status === "loading"}
>
Change avatar
</Button>- Update the file size limit in the instructions to match the actual limit:
<Text className="mt-1 text-xs text-gray-500">
JPG, GIF or PNG. 10MB max.
</Text>These changes will improve the user experience and provide more accurate information.
| const fileInputRef = useRef<HTMLInputElement>(null); | ||
| const [profilePhoto, setProfilePhoto] = useState<ProfilePhoto>({ | ||
| status: "idle", | ||
| url: details.image, | ||
| }); | ||
| const { username, name, location } = details; | ||
|
|
||
| const { mutate: getUploadUrl } = api.profile.getUploadUrl.useMutation(); | ||
| const { mutateAsync: updateUserPhotoUrl } = | ||
| api.profile.updateProfilePhotoUrl.useMutation(); |
There was a problem hiding this comment.
New state and mutations added for profile picture upload
The fileInputRef, profilePhoto state, and mutations for getUploadUrl and updateUserPhotoUrl have been correctly implemented to support the profile picture upload feature.
However, there's a potential issue with the getUploadUrl mutation:
The getUploadUrl mutation is using mutate instead of mutateAsync. This could lead to issues when awaiting the mutation result. Consider changing it to:
- const { mutate: getUploadUrl } = api.profile.getUploadUrl.useMutation();
+ const { mutateAsync: getUploadUrl } = api.profile.getUploadUrl.useMutation();This change will allow you to properly await the mutation result in the imageChange function.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fileInputRef = useRef<HTMLInputElement>(null); | |
| const [profilePhoto, setProfilePhoto] = useState<ProfilePhoto>({ | |
| status: "idle", | |
| url: details.image, | |
| }); | |
| const { username, name, location } = details; | |
| const { mutate: getUploadUrl } = api.profile.getUploadUrl.useMutation(); | |
| const { mutateAsync: updateUserPhotoUrl } = | |
| api.profile.updateProfilePhotoUrl.useMutation(); | |
| const fileInputRef = useRef<HTMLInputElement>(null); | |
| const [profilePhoto, setProfilePhoto] = useState<ProfilePhoto>({ | |
| status: "idle", | |
| url: details.image, | |
| }); | |
| const { username, name, location } = details; | |
| const { mutateAsync: getUploadUrl } = api.profile.getUploadUrl.useMutation(); | |
| const { mutateAsync: updateUserPhotoUrl } = | |
| api.profile.updateProfilePhotoUrl.useMutation(); |
|
Hi @sahilpatel2712 |
sahilpatel2712
temporarily deployed
to
production
GitHub Actions
Inactive
that mean I also should move imageChange function logic from both components into util function? |
Yes, the logic which is shared should be extracted from both and combined into a utility. |
sahilpatel2712
temporarily deployed
to
production
GitHub Actions
Inactive
sahilpatel2712
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
utils/fileUpload.ts (1)
27-47: LGTM: Main logic is sound with good error handlingThe main function logic is well-structured and includes proper error handling for various scenarios. It's great to see that the
fileLocationcheck suggested in the previous review has been implemented.One minor suggestion for improvement:
Consider adding more specific error messages for different failure scenarios. For example:
if (!signedUrl) { toast.error("Failed to obtain upload URL. Please try again."); return { status: "error", fileLocation: "" }; } // ... (after file upload) if (!fileLocation) { toast.error("File uploaded successfully, but location retrieval failed."); return { status: "error", fileLocation: "" }; }This will provide more informative feedback to users and potentially help with debugging.
app/(app)/settings/_client.tsx (3)
27-27: LGTM: Import statement updated for image upload functionalityThe change from
uploadFiletoimageUploadToUrlsuggests a refactoring of the file upload functionality. This is likely an improvement in the abstraction or implementation of image uploads.Consider updating any relevant documentation to reflect this change in the image upload utility function.
107-123: LGTM: Improved image upload handlingThe
handleImageChangefunction has been significantly improved:
- It now uses the
imageUploadToUrlfunction for a more streamlined upload process.- Error handling has been enhanced with more specific error messages.
- The use of async/await improves readability and error handling.
Consider adding more specific error handling:
} catch (error) { - toast.error("Failed to upload profile photo. Please try again."); + toast.error(`Failed to upload profile photo: ${error.message || "Unknown error"}. Please try again.`); setProfilePhoto({ status: "error", url: "" }); }This will provide more detailed error information to the user when available.
Line range hint
126-159: RefactorhandleNewEmailUpdateto usemutateAsyncThe
handleNewEmailUpdatefunction should be updated to usemutateAsyncand try/catch for consistency with the improvements made to other parts of the code.Please refactor the function as follows:
const handleNewEmailUpdate = async () => { if (cooldown > 0) { return; } if (!isValidEmail(newEmail)) { setEmailError("Please enter a valid email address"); return; } setLoading(true); try { await updateEmail({ newEmail }); setLoading(false); toast.success("Verification link sent to your email."); setSendForVerification(true); setCooldown(120); // Set a 2 minute cooldown setEmailError(""); // Clear any existing error } catch (error) { setLoading(false); toast.error(error.message || "Something went wrong sending the verification link."); } };This refactoring aligns with the
mutateAsyncpattern used elsewhere in the component and improves error handling.app/(app)/alpha/additional-details/_client.tsx (4)
58-59: New type definitions for user details and profile photoThe
UserDetailstype has been updated to include animageproperty, and a newProfilePhototype has been added. These changes are appropriate for managing the new profile picture functionality.However, consider using more specific types for the
statusproperty inProfilePhoto:type ProfilePhoto = { status: "success" | "error" | "loading" | "idle"; url: string; };This will provide better type safety and autocompletion.
Also applies to: 61-63
112-120: New state and mutations for profile picture uploadThe implementation of
fileInputRef,profilePhotostate, and TRPC mutations looks good. However, there's a potential issue with error handling:
- Consider adding error handling for the TRPC mutations:
const { mutateAsync: getUploadUrl, error: getUploadUrlError } = api.profile.getUploadUrl.useMutation(); const { mutateAsync: updateUserPhotoUrl, error: updateUserPhotoUrlError } = api.profile.updateProfilePhotoUrl.useMutation(); // Later in the component: useEffect(() => { if (getUploadUrlError) { toast.error("Failed to get upload URL. Please try again."); } if (updateUserPhotoUrlError) { toast.error("Failed to update profile photo. Please try again."); } }, [getUploadUrlError, updateUserPhotoUrlError]);This will provide more specific error messages to the user.
130-148: Implementation of handleImageChange functionThe
handleImageChangefunction looks good overall, but there are a few improvements we can make:
- Add file type and size validation before uploading:
const handleImageChange = async (e: React.ChangeEvent<HTMLInputElement>) => { if (e.target.files && e.target.files.length > 0) { const file = e.target.files[0]; // Validate file type const allowedTypes = ['image/jpeg', 'image/png', 'image/gif']; if (!allowedTypes.includes(file.type)) { toast.error("Invalid file type. Please upload a JPG, GIF, or PNG image."); return; } // Validate file size (10MB max) const maxSize = 10 * 1024 * 1024; // 10MB in bytes if (file.size > maxSize) { toast.error("File size exceeds 10MB limit."); return; } try { setProfilePhoto({ status: "loading", url: "" }); const { status, fileLocation } = await imageUploadToUrl({ file, updateUserPhotoUrl, getUploadUrl, }); setProfilePhoto({ status: status, url: fileLocation }); } catch (error) { toast.error("Failed to upload profile photo. Please try again."); setProfilePhoto({ status: "error", url: "" }); } } else { toast.error("No file selected. Please try again."); } };
- Consider adding a success notification:
if (status === "success" && fileLocation) { toast.success("Profile picture updated successfully!"); }These changes will improve the user experience and add necessary validations.
175-213: UI implementation for profile picture uploadThe UI implementation looks good, but there are a few improvements we can make:
- Add a loading state to the Avatar component:
<Avatar square src={profilePhoto.status === "loading" ? undefined : profilePhoto.url} alt="Profile photo upload section" className={`h-16 w-16 overflow-hidden rounded-full ${ profilePhoto.status === "loading" ? "animate-pulse" : "" }`} /> {profilePhoto.status === "loading" && ( <div className="absolute inset-0 flex items-center justify-center"> <span className="loading loading-spinner loading-md"></span> </div> )}
- Disable the "Change avatar" button during upload:
<Button color="dark/white" type="button" className="h-[30px] rounded-md text-xs" onClick={() => fileInputRef.current?.click()} disabled={profilePhoto.status === "loading"} > Change avatar </Button>
- Update the file size limit in the instructions to match the actual limit:
<Text className="mt-1 text-xs text-gray-500"> JPG, GIF or PNG. 10MB max. </Text>These changes will improve the user experience and provide more accurate information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- app/(app)/alpha/additional-details/_client.tsx (6 hunks)
- app/(app)/settings/_client.tsx (4 hunks)
- utils/fileUpload.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
utils/fileUpload.ts (2)
1-15: LGTM: Imports and type definitions are well-structuredThe imports and type definitions are clear, concise, and appropriate for the file's purpose. The
UploadToUrlPropsandUploadResulttypes provide a good structure for the function parameters and return value.
1-56: Overall: Well-implemented image upload functionality with room for minor enhancementsThe
imageUploadToUrlfunction is well-structured and implements the required functionality for uploading user profile images. It includes proper type definitions, error handling, and addresses most concerns from previous reviews.Key strengths:
- Clear type definitions for function props and result.
- Proper error handling for various scenarios.
- User-friendly notifications using toast messages.
Suggested improvements:
- Enhance initial file validation to include type and size checks.
- Provide more specific error messages for different failure scenarios.
- Add error logging for improved debugging capabilities.
These enhancements will further improve the robustness and maintainability of the code. Great job overall!
app/(app)/settings/_client.tsx (2)
Line range hint
1-424: Overall improvements with minor inconsistenciesThe changes in this PR significantly enhance the
Settingscomponent, particularly in terms of image upload handling and error management. The use ofmutateAsyncfor most mutation hooks improves asynchronous operation handling.Key improvements:
- Refactored image upload functionality with better error handling.
- Updated mutation hooks to use
mutateAsyncforgetUploadUrlandupdateUserPhotoUrl.- Increased file size limit to 10MB as per PR objectives.
Areas for further improvement:
- Update the
updateEmailmutation hook to usemutateAsync.- Refactor the
handleNewEmailUpdatefunction to use try/catch with async/await.- Ensure backend support for the increased file size limit.
Once these minor issues are addressed, the code will be more consistent and robust throughout the component.
204-204: Verify backend support for increased file size limitThe UI text has been updated to reflect an increased maximum file size of 10MB, which aligns with the PR objectives.
Please ensure that the backend (server-side code and any cloud storage services) can handle this increased file size limit. Run the following script to check for any hardcoded file size limits in the backend code:
If any results are found, make sure they are consistent with the new 10MB limit.
✅ Verification successful
Confirm all backend file size limits are updated to 10MB
The backend codebase contains a maximum file size limit of 10MB in
server/api/router/profile.ts, which aligns with the UI update. To ensure consistency across the entire backend, please run the following script to verify there are no other conflicting file size limits:
Backend file size limits successfully updated to 10MB
All backend file size limits have been verified and set to 10MB in
server/api/router/profile.ts. No other conflicting file size limits were found in the backend codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for file size limits in backend code rg -i '(file|upload|size|limit).*(\d+\s*(mb|kb|bytes))' --type ts --type js server/ api/Length of output: 209
Script:
# Search for all file size limits in the backend code rg -i '(file|upload|size|limit).*(\d+\s*(mb|kb|bytes))' --type ts --type js server/Length of output: 159
app/(app)/alpha/additional-details/_client.tsx (2)
3-3: New imports added for profile picture functionalityThe new imports seem appropriate for the added functionality:
useRefanduseStatefrom React for managing the file input and profile photo state.AvatarandTextcomponents for UI elements.apifor TRPC integration.imageUploadToUrlutility function for handling file uploads.These additions align well with the new profile picture upload feature.
Also applies to: 42-45
Line range hint
1-724: Overall assessment of profile picture upload implementationThe implementation of the profile picture upload feature in the
SlideOnecomponent is well-integrated and functional. The code structure is clear, and the use of TRPC for API calls is appropriate. However, there are a few areas for improvement:
- Enhanced error handling for TRPC mutations.
- File type and size validation before upload.
- Improved UI feedback during the upload process.
- Minor updates to the UI for better user experience.
Implementing the suggested changes will result in a more robust and user-friendly profile picture upload feature. Great job on the implementation, and these small tweaks will make it even better!
| export const imageUploadToUrl = async ({ | ||
| file, | ||
| updateUserPhotoUrl, | ||
| getUploadUrl, | ||
| }: UploadToUrlProps): Promise<UploadResult> => { | ||
| if (!file) { | ||
| toast.error("Invalid file upload."); | ||
| return { status: "error", fileLocation: "" }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance initial file validation
While the initial file check is a good start, consider implementing more comprehensive validation as suggested in the previous review:
- Make the error message more specific: "No file selected for upload" instead of "Invalid file upload."
- Add file type and size validation before proceeding with the upload.
Example implementation:
if (!file) {
toast.error("No file selected for upload.");
return { status: "error", fileLocation: "" };
}
const allowedTypes = ["image/jpeg", "image/png"];
const maxSize = 5 * 1024 * 1024; // 5 MB
if (!allowedTypes.includes(file.type)) {
toast.error("Invalid file type. Please upload a JPEG or PNG image.");
return { status: "error", fileLocation: "" };
}
if (file.size > maxSize) {
toast.error("File size exceeds the 5MB limit. Please upload a smaller file.");
return { status: "error", fileLocation: "" };
}This will improve error handling and prevent potential issues with unsupported file types or oversized files.
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| toast.error(error.message); | ||
| } else { | ||
| toast.error("Failed to upload profile photo. Please try again."); | ||
| } | ||
| return { status: "error", fileLocation: "" }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error logging for debugging
The error handling in the catch block is good, differentiating between Error instances and unknown errors. To further improve debugging capabilities, consider adding error logging:
} catch (error) {
console.error('Image upload failed:', error);
if (error instanceof Error) {
toast.error(error.message);
} else {
toast.error("Failed to upload profile photo. Please try again.");
}
return { status: "error", fileLocation: "" };
}This will help with identifying and resolving issues in production environments while maintaining the user-friendly error messages.
| const { mutateAsync: getUploadUrl } = api.profile.getUploadUrl.useMutation(); | ||
| const { mutateAsync: updateUserPhotoUrl } = | ||
| api.profile.updateProfilePhotoUrl.useMutation(); | ||
| const { mutate: updateEmail } = api.profile.updateEmail.useMutation(); |
There was a problem hiding this comment.
Update remaining mutation hook to use mutateAsync
Great job updating the getUploadUrl and updateUserPhotoUrl hooks to use mutateAsync. This change improves error handling and consistency in asynchronous operations.
However, the updateEmail hook on line 81 still uses mutate. For consistency, please update it as well:
-const { mutate: updateEmail } = api.profile.updateEmail.useMutation();
+const { mutateAsync: updateEmail } = api.profile.updateEmail.useMutation();Don't forget to refactor the handleNewEmailUpdate function to use try/catch with async/await when calling this mutation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { mutateAsync: getUploadUrl } = api.profile.getUploadUrl.useMutation(); | |
| const { mutateAsync: updateUserPhotoUrl } = | |
| api.profile.updateProfilePhotoUrl.useMutation(); | |
| const { mutate: updateEmail } = api.profile.updateEmail.useMutation(); | |
| const { mutateAsync: getUploadUrl } = api.profile.getUploadUrl.useMutation(); | |
| const { mutateAsync: updateUserPhotoUrl } = | |
| api.profile.updateProfilePhotoUrl.useMutation(); | |
| const { mutateAsync: updateEmail } = api.profile.updateEmail.useMutation(); |
|
There is a ton of code duplication, and the design doesn't look as it does on the settings page, so I won't merge this in just yet. It will create more code than needed to maintain.
Feel free to reopen if you want to tackle it without the copy/pasting. |
3 participants
✨ Codu Pull Request 💻
Fixes #1008
Pull Request details
Any Breaking changes
'None'
Associated Screenshots