feat(backend, frontend): upload avatar #165
Conversation
WalkthroughThis PR integrates avatar upload functionality and media management across the system. In the backend, the UploadModule is added to chat and user modules, along with new GraphQL types, resolvers, and service methods for avatar uploads. Utility functions for media directories are introduced. On the frontend, a new AvatarUploader component, updates to GraphQL queries/mutations, and adjustments to settings and sidebar components enable users to manage their avatars. Additionally, a new media serving API route is added and minor cleanup and refactoring are performed in several components. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User (AvatarUploader)
participant G as GraphQL API
participant R as UserResolver
participant S as UserService
participant US as UploadService
participant DB as Database
U->>G: Trigger uploadAvatar mutation with file
G->>R: Forward request to resolver
R->>S: Call updateAvatar(userId, file)
S->>US: Validate and buffer file, then upload file
US-->>S: Return uploaded file URL
S->>DB: Update user record with avatarUrl
DB-->>S: Confirm update
S-->>R: Return AvatarUploadResponse { success, avatarUrl }
R-->>G: Return response
G-->>U: Deliver upload result for UI update
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
frontend/src/components/user-settings-bar.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/frontend/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. frontend/src/components/avatar-uploader.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/frontend/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
🪧 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 (
|
ZHallen122
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
frontend/src/components/edit-username-form.tsx (1)
82-86:⚠️ Potential issueInconsistency between local storage and auth context
The onSubmit function updates the username in localStorage and dispatches a storage event, but the component now uses the authentication context for user data. This might lead to inconsistencies between the local storage and the actual user state.
Consider updating the user through an API call and refreshing the user context:
function onSubmit(values: z.infer<typeof formSchema>) { - localStorage.setItem('ollama_user', values.username); - window.dispatchEvent(new Event('storage')); + // Call API to update username + updateUsernameMutation({ variables: { username: values.username } }) + .then(() => { + refreshUserInfo(); // Refresh user information in context + toast.success('Name updated successfully'); + }) + .catch((error) => { + toast.error('Failed to update name: ' + error.message); + }); toast.success('Name updated successfully'); }
🧹 Nitpick comments (10)
frontend/src/graphql/type.tsx (1)
367-367: Unusual property placementThe
avatarUrlproperty is positioned before the__typenamefield, which is unconventional as__typenametypically comes first in GraphQL object types. While this doesn't affect functionality, consider maintaining a consistent ordering of fields across all types.- avatarUrl: string; __typename: 'User'; + avatarUrl: string;backend/src/user/user.service.ts (1)
45-62: Consider cleaning up old avatar filesThe updateAvatar method is well-implemented with appropriate error handling and validation. However, it doesn't handle the case where a user already has an avatar. Consider cleaning up the old avatar file to avoid accumulating unused files in storage.
async updateAvatar(userId: string, file: Promise<FileUpload>): Promise<User> { // Get the user const user = await this.userRepository.findOneBy({ id: userId }); if (!user) { throw new Error('User not found'); } + // Check if user already has an avatar and delete old one if needed + if (user.avatarUrl) { + try { + await this.uploadService.deleteFile(user.avatarUrl); + } catch (error) { + console.warn(`Failed to delete old avatar: ${error.message}`); + // Continue with upload even if deletion fails + } + } // Validate and convert file to buffer const uploadedFile = await file; const { buffer, mimetype } = await validateAndBufferFile(uploadedFile); // Upload the validated buffer to storage const result = await this.uploadService.upload(buffer, mimetype, 'avatars'); // Update the user's avatar URL user.avatarUrl = result.url; return this.userRepository.save(user); }frontend/src/app/api/media/[...path]/route.ts (3)
42-44: Use async file operations instead of blocking synchronous calls.The current implementation uses
fs.readFileSyncwhich blocks the Node.js event loop and can impact performance, especially for larger files.Use non-blocking async file operations instead:
- // Read the file - const fileBuffer = fs.readFileSync(filePath); + // Read the file asynchronously + const fileBuffer = await fs.promises.readFile(filePath);
19-40: Move debugging logic to a separate function and consider environment-based logging.The current debug logging code is mixed with the main logic and always executes in production, which isn't ideal.
Extract debug logic to a separate function and consider adding environment checks:
// Check if the file exists if (!fs.existsSync(filePath)) { - // Log directory contents for debugging - try { - if (fs.existsSync(mediaDir)) { - const avatarsDir = path.join(mediaDir, 'avatars'); - if (fs.existsSync(avatarsDir)) { - console.log( - 'Avatars directory contents:', - fs.readdirSync(avatarsDir) - ); - } else { - console.log('Avatars directory does not exist'); - } - } else { - console.log('Media directory does not exist'); - } - } catch (err) { - console.error('Error reading directory:', err); - } + // Log directory contents only in development + if (process.env.NODE_ENV === 'development') { + logDirectoryContents(mediaDir); + } return new Response('File not found', { status: 404 }); } + // Separate function for directory logging + function logDirectoryContents(mediaDir) { + try { + if (fs.existsSync(mediaDir)) { + const avatarsDir = path.join(mediaDir, 'avatars'); + if (fs.existsSync(avatarsDir)) { + console.log( + 'Avatars directory contents:', + fs.readdirSync(avatarsDir) + ); + } else { + console.log('Avatars directory does not exist'); + } + } else { + console.log('Media directory does not exist'); + } + } catch (err) { + console.error('Error reading directory:', err); + } + }
57-63: Add content disposition header for security.Adding a Content-Disposition header can help prevent certain types of attacks (like MIME-type confusion) and provide a better download experience when needed.
Add Content-Disposition header for inline viewing:
// Return the file with appropriate headers return new Response(fileBuffer, { headers: { 'Content-Type': contentType, 'Cache-Control': 'public, max-age=31536000', + 'Content-Disposition': 'inline', }, });frontend/src/components/user-settings.tsx (2)
80-89: Consider removing the setTimeout from handleSettingsClick.Using setTimeout with a 0ms delay to dispatch an event after navigation is a bit of a workaround pattern. Consider more direct approaches.
You could try to dispatch the event directly, or if there's a specific reason for the delay (like waiting for the route change to complete), add a comment explaining why.
const handleSettingsClick = () => { // First navigate using Next.js router router.push('/chat?id=setting'); // Then dispatch the event - setTimeout(() => { - const event = new Event(EventEnum.SETTING); - window.dispatchEvent(event); - }, 0); + // Dispatch event after navigation + const event = new Event(EventEnum.SETTING); + window.dispatchEvent(event); + + // If you need to keep the setTimeout for specific reasons, add a comment explaining why: + // setTimeout(() => { + // const event = new Event(EventEnum.SETTING); + // window.dispatchEvent(event); + // }, 0); // Using setTimeout to ensure event fires after navigation completes };
101-106: Add error handling for avatar image loading.Missing error handling for the AvatarImage component could result in a broken UI if the image fails to load.
Add an onError handler to handle image loading failures:
{/* Use normalized avatar URL */} <AvatarImage src={normalizedAvatarUrl} alt="User" key={user?.avatarUrl} + onError={(e) => { + // Reset src to empty to show fallback + e.currentTarget.src = ''; + }} />frontend/src/components/avatar-uploader.tsx (3)
57-60: Extract file size limit to a constant.The 5MB file size limit is hardcoded. It would be better to extract it to a constant for maintainability.
Extract the file size limit to a constant:
+ // Constants + const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB + const ALLOWED_FILE_TYPES = ['image/jpeg', 'image/png', 'image/webp']; // ... // Check file size on client-side as well (5MB limit) - if (file.size > 5 * 1024 * 1024) { + if (file.size > MAX_FILE_SIZE) { toast.error('File size exceeds the maximum allowed limit (5MB)'); return; } // ... // Check file type on client-side const fileType = file.type; - if (!['image/jpeg', 'image/png', 'image/webp'].includes(fileType)) { + if (!ALLOWED_FILE_TYPES.includes(fileType)) { toast.error('Only JPEG, PNG, and WebP files are allowed'); return; }
48-50: Add cleanup for FileReader preview URL.The component creates a preview URL but doesn't clean it up when the component unmounts, which could potentially lead to memory leaks.
Add a useEffect to clean up the preview URL:
const [uploadAvatar, { loading }] = useMutation(UPLOAD_AVATAR); const fileInputRef = useRef<HTMLInputElement>(null); const [previewUrl, setPreviewUrl] = useState<string | null>(null); const { refreshUserInfo } = useAuthContext(); + // Clean up preview URL when component unmounts or when previewUrl changes + useEffect(() => { + return () => { + // Clean up any object URLs to avoid memory leaks + if (previewUrl?.startsWith('blob:')) { + URL.revokeObjectURL(previewUrl); + } + }; + }, [previewUrl]);Don't forget to add the import:
- import React, { useRef, useState } from 'react'; + import React, { useRef, useState, useEffect } from 'react';
76-95: Add better loading indicator during upload.The component shows "Uploading..." on the button, but there's no visual indication that the avatar is being uploaded.
Add a loading overlay on the avatar during upload:
return ( <div className="flex flex-col items-center gap-4"> <input type="file" ref={fileInputRef} onChange={handleFileChange} accept="image/*" className="hidden" /> - <Avatar className="w-24 h-24 cursor-pointer" onClick={triggerFileInput}> + <div className="relative"> + <Avatar className="w-24 h-24 cursor-pointer" onClick={triggerFileInput}> <AvatarImage src={displayUrl} alt="User Avatar" /> <AvatarFallback>{avatarFallback}</AvatarFallback> - </Avatar> + </Avatar> + {loading && ( + <div className="absolute inset-0 bg-black bg-opacity-50 rounded-full flex items-center justify-center"> + <div className="animate-spin h-6 w-6 border-2 border-white border-t-transparent rounded-full"></div> + </div> + )} + </div> <Button onClick={triggerFileInput} disabled={loading} size="sm" variant="outline" > {loading ? 'Uploading...' : 'Change Avatar'} </Button> </div> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/src/chat/chat.module.ts(1 hunks)backend/src/user/dto/upload-avatar.input.ts(1 hunks)backend/src/user/user.model.ts(1 hunks)backend/src/user/user.module.ts(1 hunks)backend/src/user/user.resolver.ts(3 hunks)backend/src/user/user.service.ts(2 hunks)codefox-common/src/common-path.ts(1 hunks)frontend/src/app/api/media/[...path]/route.ts(1 hunks)frontend/src/app/api/runProject/route.ts(2 hunks)frontend/src/components/avatar-uploader.tsx(1 hunks)frontend/src/components/edit-username-form.tsx(3 hunks)frontend/src/components/user-settings.tsx(4 hunks)frontend/src/graphql/request.ts(2 hunks)frontend/src/graphql/type.tsx(1 hunks)frontend/src/providers/AuthProvider.tsx(4 hunks)
🔇 Additional comments (19)
backend/src/user/user.module.ts (1)
10-10: Import and configuration for avatar upload featureGood addition of the
UploadModuleto handle file uploads for user avatars. This is consistent with the newavatarUrlfield in the User model.Also applies to: 18-18
backend/src/chat/chat.module.ts (1)
13-13:❓ Verification inconclusive
Consistent module integration
Adding the
UploadModuleto the ChatModule is consistent with the changes in the UserModule. This suggests a unified approach to file uploads across different parts of the application.Also applies to: 20-20
🏁 Script executed:
#!/bin/bash # Checking if chat-related upload functionality is implemented # Look for upload-related methods in chat services and resolvers echo "Checking chat resolvers and services for upload methods..." rg -A 10 -B 10 "upload|Upload" backend/src/chat/*resolver.ts backend/src/chat/*service.ts # Look for file/media fields in message-related models echo "Checking message models for file/media fields..." rg "@Field|@Column" -A 3 "file|image|media|attachment" backend/src/chat/message.model.tsLength of output: 1238
Action: Please Verify Chat Upload Integration Consistency
The inclusion of the
UploadModulein the ChatModule (lines 13 and 20) appears to follow the approach implemented in the UserModule for handling file uploads. However, our automated script did not reveal explicit upload-related methods in the chat resolvers or services, which might indicate that the functionality is handled differently (or deferred to shared components). Given the inconclusive automated output, please perform a manual review to ensure that:
- The chat module’s upload functionality is correctly integrated and activated (either directly or via shared services).
- There’s consistency between how file uploads are managed in ChatModule versus UserModule.
- No critical upload handlers or method references in chat-specific components are inadvertently missing.
backend/src/user/user.model.ts (1)
31-33: Properly implemented optional fieldThe
avatarUrlfield is correctly implemented as nullable in the database and optional in the TypeScript type. This is a good practice when adding new fields to existing entities as it won't break existing records.backend/src/user/dto/upload-avatar.input.ts (1)
1-8: LGTM! Well-structured input type for avatar uploads.The
UploadAvatarInputclass is correctly defined with the appropriate GraphQL decorators and uses theFileUploadtype fromgraphql-upload-minimalas expected. This follows NestJS best practices for handling file uploads in GraphQL.frontend/src/app/api/runProject/route.ts (2)
11-13: Good addition for cross-platform compatibility.Adding OS detection to enable Windows compatibility is a good improvement.
87-89: Excellent platform-specific command handling.The conditional commands handle Windows vs Unix/Linux file system operations appropriately, maintaining identical functionality across platforms.
frontend/src/graphql/request.ts (2)
104-104: Good addition of avatarUrl field to user info.Adding the avatarUrl field to the existing query is the correct approach to extend user information.
255-270: Well-implemented GraphQL operations for avatar functionality.The new mutation and query for avatar management follow the established patterns in the codebase and provide all necessary fields.
Some considerations:
- The UPLOAD_AVATAR mutation correctly uses the GraphQL Upload scalar
- The success field in the response allows for proper error handling
- The GET_USER_AVATAR query accepts a userId parameter to fetch specific user avatars
backend/src/user/user.resolver.ts (3)
32-39: Good implementation of AvatarUploadResponse typeThis class is well-structured with appropriate fields for success status and avatarUrl.
91-113: Well-implemented avatar upload mutation with proper error handlingThe implementation includes:
- Clear documentation
- Proper authentication via user token
- GraphQLUpload type for file handling
- Comprehensive error handling with detailed logging
This approach ensures errors are properly caught, logged, and propagated to the GraphQL error handler.
118-122: Consider access control for getUserAvatar queryWhile the implementation is correct, consider whether any user should be able to access any other user's avatar URL. If this is intended for public profiles, then the current implementation is fine. Otherwise, you might want to add authorization checks.
backend/src/user/user.service.ts (1)
14-14: Good dependency injection of UploadServiceThe UploadService is properly injected as a dependency, following NestJS best practices.
frontend/src/components/edit-username-form.tsx (3)
60-61: Good use of authentication contextUsing the authentication context is a better approach than directly accessing localStorage for user data.
68-73: Well implemented user data initializationThe useEffect hook is properly set up to initialize username and avatar URL from authenticated user data.
111-115: Well implemented AvatarUploader integrationThe AvatarUploader component is properly integrated with all necessary props: current avatar URL, fallback avatar, and a callback for handling avatar changes.
frontend/src/providers/AuthProvider.tsx (3)
26-26: Good interface extensionAdding the refreshUserInfo method to the AuthContextValue interface properly exposes this functionality to consumers of the context.
87-89: Well implemented refreshUserInfo methodThe refreshUserInfo method is:
- Properly memoized with useCallback
- Simple and focused (calls the existing fetchUserInfo method)
- Returns a boolean to indicate success/failure
This approach supports updating the user information after avatar changes and other user data updates.
196-196: Context value properly updatedThe refreshUserInfo method is correctly added to the context provider value, making it accessible to components that consume the AuthContext.
frontend/src/components/avatar-uploader.tsx (1)
11-34: Extract normalizeAvatarUrl to a shared utility.This function is duplicated in both
user-settings.tsxandavatar-uploader.tsx. Following the DRY principle, it should be extracted to a shared utility file.
frontend/src/graphql/type.tsx
Outdated
| @@ -364,6 +364,7 @@ export type UpdateProjectPhotoInput = { | |||
| }; | |||
|
|
|||
| export type User = { | |||
| avatarUrl: string; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type inconsistency with backend model
The avatarUrl property is defined as a required string in the frontend type definition, but it's defined as optional (avatarUrl?: string) in the backend model. This could lead to runtime type errors if the backend returns null for users without an avatar.
🏁 Script executed:
#!/bin/bash
# Verify if the avatarUrl property is handled correctly in the front-end code
# Check if there are any null checks for avatarUrl in the codebase
echo "Checking for null checks on avatarUrl..."
rg "avatarUrl(?:\s+)?\?\?" frontend/src/Length of output: 133
Action Required: Resolve Type Inconsistency for avatarUrl
The current type definition in frontend/src/graphql/type.tsx (line 367) declares avatarUrl as a required string, while the backend model defines it as optional. Our verification via a regex search for null-coalescing checks (using rg "avatarUrl(?:\s+)?\?\?") returned no results, indicating that there are no safeguards in place to handle a null value. This discrepancy can lead to runtime errors if the backend returns null for users without an avatar.
Recommendations:
- Update the Type Definition: Change the line from
to
avatarUrl: string;
to directly match the backend model.avatarUrl?: string;
- Alternatively, Add Null Checks: If the frontend logic must treat
avatarUrlas required, implement comprehensive null checks to provide fallback values whereveravatarUrlis used.
backend/src/user/user.model.ts
Outdated
| @Field() | ||
| @Column({ nullable: true }) | ||
| avatarUrl?: string; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type inconsistency with frontend model
The backend defines avatarUrl as optional (avatarUrl?: string), but the frontend GraphQL type in frontend/src/graphql/type.tsx defines it as required (avatarUrl: string). This mismatch could lead to runtime errors.
🏁 Script executed:
#!/bin/bash
# Checking for potential handling of null avatarUrls in frontend code
# Search for GraphQL queries that include avatarUrl
echo "Checking GraphQL queries that include avatarUrl..."
rg "avatarUrl" frontend/src/graphql/request.ts
# Check component usage of avatarUrl for null handling
echo "Checking components that might use avatarUrl..."
rg -A 5 -B 5 "avatarUrl" frontend/src/components/Length of output: 10806
Address GraphQL Type Mismatch for avatarUrl
- The backend model (
backend/src/user/user.model.ts) marksavatarUrlas optional (nullable). - The frontend, however, defines the corresponding GraphQL type in
frontend/src/graphql/type.tsxas required. - While the UI components (e.g., in
avatar-uploader.tsx,edit-username-form.tsx, anduser-settings.tsx) normalize and provide default values whenavatarUrlis not set, the type declaration still implies the value should always be present. - Recommendation: Update the frontend GraphQL type to allow
avatarUrlto benull/undefined(for example, usingavatarUrl?: stringoravatarUrl: string | null) or modify the backend to guarantee a non-null string. This alignment will prevent potential runtime errors and ensure consistency across the data flow.
| export const getMediaDir = (): string => | ||
| ensureDir(path.join(getRootDir(), 'media')); | ||
| export const getMediaPath = (modelName: string): string => | ||
| path.join(getModelsDir(), modelName); | ||
| export const getMediaAvatarsDir = (): string => | ||
| ensureDir(path.join(getMediaDir(), 'avatars')); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consistent implementation of media directory utilities.
The new media directory utility functions follow the established pattern in the codebase and use the existing ensureDir helper for creating directories when needed.
One minor issue:
🏁 Script executed:
#!/bin/bash
# Check if getMediaPath is using the correct base directory
# The function references getModelsDir instead of getMediaDir
echo "Possible inconsistency in getMediaPath implementation:"
grep -n "getMediaPath" codefox-common/src/common-path.tsLength of output: 237
Media Directory Path Inconsistency – Action Needed
The new media directory utility functions are consistent overall, but there’s a minor discrepancy: the getMediaPath function uses getModelsDir instead of the expected getMediaDir (as used in getMediaAvatarsDir). Please confirm whether this is intentional; if not, update the function to use getMediaDir for consistency with the media utilities pattern.
Suggested diff:
- export const getMediaPath = (modelName: string): string =>
- path.join(getModelsDir(), modelName);
+ export const getMediaPath = (modelName: string): string =>
+ path.join(getMediaDir(), modelName);📝 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.
| export const getMediaDir = (): string => | |
| ensureDir(path.join(getRootDir(), 'media')); | |
| export const getMediaPath = (modelName: string): string => | |
| path.join(getModelsDir(), modelName); | |
| export const getMediaAvatarsDir = (): string => | |
| ensureDir(path.join(getMediaDir(), 'avatars')); | |
| export const getMediaDir = (): string => | |
| ensureDir(path.join(getRootDir(), 'media')); | |
| export const getMediaPath = (modelName: string): string => | |
| path.join(getMediaDir(), modelName); | |
| export const getMediaAvatarsDir = (): string => | |
| ensureDir(path.join(getMediaDir(), 'avatars')); |
| // For App Router: app/api/media/[...path]/route.ts | ||
| import { NextRequest } from 'next/server'; | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import { getMediaDir } from 'codefox-common'; | ||
|
|
||
| export async function GET( | ||
| request: NextRequest, | ||
| { params }: { params: { path: string[] } } | ||
| ) { | ||
| try { | ||
| // Get the media directory path | ||
| const mediaDir = getMediaDir(); | ||
|
|
||
| // Construct the full path to the requested file | ||
| const filePath = path.join(mediaDir, ...params.path); | ||
|
|
||
| // Check if the file exists | ||
| if (!fs.existsSync(filePath)) { | ||
| // Log directory contents for debugging | ||
| try { | ||
| if (fs.existsSync(mediaDir)) { | ||
| const avatarsDir = path.join(mediaDir, 'avatars'); | ||
| if (fs.existsSync(avatarsDir)) { | ||
| console.log( | ||
| 'Avatars directory contents:', | ||
| fs.readdirSync(avatarsDir) | ||
| ); | ||
| } else { | ||
| console.log('Avatars directory does not exist'); | ||
| } | ||
| } else { | ||
| console.log('Media directory does not exist'); | ||
| } | ||
| } catch (err) { | ||
| console.error('Error reading directory:', err); | ||
| } | ||
|
|
||
| return new Response('File not found', { status: 404 }); | ||
| } | ||
|
|
||
| // Read the file | ||
| const fileBuffer = fs.readFileSync(filePath); | ||
|
|
||
| // Determine content type based on file extension | ||
| const ext = path.extname(filePath).toLowerCase(); | ||
| const contentTypeMap: Record<string, string> = { | ||
| '.jpg': 'image/jpeg', | ||
| '.jpeg': 'image/jpeg', | ||
| '.png': 'image/png', | ||
| '.webp': 'image/webp', | ||
| '.gif': 'image/gif', | ||
| }; | ||
|
|
||
| const contentType = contentTypeMap[ext] || 'application/octet-stream'; | ||
|
|
||
| // Return the file with appropriate headers | ||
| return new Response(fileBuffer, { | ||
| headers: { | ||
| 'Content-Type': contentType, | ||
| 'Cache-Control': 'public, max-age=31536000', | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.error('Error serving media file:', error); | ||
| return new Response(`Error serving file: ${error.message}`, { | ||
| status: 500, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add path sanitization to prevent directory traversal attacks.
The current implementation doesn't validate or sanitize the path parameters, which could potentially allow directory traversal attacks where malicious users attempt to access files outside the intended media directory.
Add path sanitization by verifying that the final resolved path is still within the media directory:
// Construct the full path to the requested file
const filePath = path.join(mediaDir, ...params.path);
+ // Check for directory traversal attempts
+ const normalizedPath = path.normalize(filePath);
+ if (!normalizedPath.startsWith(mediaDir)) {
+ console.error('Possible directory traversal attempt:', filePath);
+ return new Response('Access denied', { status: 403 });
+ }📝 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.
| // For App Router: app/api/media/[...path]/route.ts | |
| import { NextRequest } from 'next/server'; | |
| import fs from 'fs'; | |
| import path from 'path'; | |
| import { getMediaDir } from 'codefox-common'; | |
| export async function GET( | |
| request: NextRequest, | |
| { params }: { params: { path: string[] } } | |
| ) { | |
| try { | |
| // Get the media directory path | |
| const mediaDir = getMediaDir(); | |
| // Construct the full path to the requested file | |
| const filePath = path.join(mediaDir, ...params.path); | |
| // Check if the file exists | |
| if (!fs.existsSync(filePath)) { | |
| // Log directory contents for debugging | |
| try { | |
| if (fs.existsSync(mediaDir)) { | |
| const avatarsDir = path.join(mediaDir, 'avatars'); | |
| if (fs.existsSync(avatarsDir)) { | |
| console.log( | |
| 'Avatars directory contents:', | |
| fs.readdirSync(avatarsDir) | |
| ); | |
| } else { | |
| console.log('Avatars directory does not exist'); | |
| } | |
| } else { | |
| console.log('Media directory does not exist'); | |
| } | |
| } catch (err) { | |
| console.error('Error reading directory:', err); | |
| } | |
| return new Response('File not found', { status: 404 }); | |
| } | |
| // Read the file | |
| const fileBuffer = fs.readFileSync(filePath); | |
| // Determine content type based on file extension | |
| const ext = path.extname(filePath).toLowerCase(); | |
| const contentTypeMap: Record<string, string> = { | |
| '.jpg': 'image/jpeg', | |
| '.jpeg': 'image/jpeg', | |
| '.png': 'image/png', | |
| '.webp': 'image/webp', | |
| '.gif': 'image/gif', | |
| }; | |
| const contentType = contentTypeMap[ext] || 'application/octet-stream'; | |
| // Return the file with appropriate headers | |
| return new Response(fileBuffer, { | |
| headers: { | |
| 'Content-Type': contentType, | |
| 'Cache-Control': 'public, max-age=31536000', | |
| }, | |
| }); | |
| } catch (error) { | |
| console.error('Error serving media file:', error); | |
| return new Response(`Error serving file: ${error.message}`, { | |
| status: 500, | |
| }); | |
| } | |
| } | |
| // For App Router: app/api/media/[...path]/route.ts | |
| import { NextRequest } from 'next/server'; | |
| import fs from 'fs'; | |
| import path from 'path'; | |
| import { getMediaDir } from 'codefox-common'; | |
| export async function GET( | |
| request: NextRequest, | |
| { params }: { params: { path: string[] } } | |
| ) { | |
| try { | |
| // Get the media directory path | |
| const mediaDir = getMediaDir(); | |
| // Construct the full path to the requested file | |
| const filePath = path.join(mediaDir, ...params.path); | |
| // Check for directory traversal attempts | |
| const normalizedPath = path.normalize(filePath); | |
| if (!normalizedPath.startsWith(mediaDir)) { | |
| console.error('Possible directory traversal attempt:', filePath); | |
| return new Response('Access denied', { status: 403 }); | |
| } | |
| // Check if the file exists | |
| if (!fs.existsSync(filePath)) { | |
| // Log directory contents for debugging | |
| try { | |
| if (fs.existsSync(mediaDir)) { | |
| const avatarsDir = path.join(mediaDir, 'avatars'); | |
| if (fs.existsSync(avatarsDir)) { | |
| console.log( | |
| 'Avatars directory contents:', | |
| fs.readdirSync(avatarsDir) | |
| ); | |
| } else { | |
| console.log('Avatars directory does not exist'); | |
| } | |
| } else { | |
| console.log('Media directory does not exist'); | |
| } | |
| } catch (err) { | |
| console.error('Error reading directory:', err); | |
| } | |
| return new Response('File not found', { status: 404 }); | |
| } | |
| // Read the file | |
| const fileBuffer = fs.readFileSync(filePath); | |
| // Determine content type based on file extension | |
| const ext = path.extname(filePath).toLowerCase(); | |
| const contentTypeMap: Record<string, string> = { | |
| '.jpg': 'image/jpeg', | |
| '.jpeg': 'image/jpeg', | |
| '.png': 'image/png', | |
| '.webp': 'image/webp', | |
| '.gif': 'image/gif', | |
| }; | |
| const contentType = contentTypeMap[ext] || 'application/octet-stream'; | |
| // Return the file with appropriate headers | |
| return new Response(fileBuffer, { | |
| headers: { | |
| 'Content-Type': contentType, | |
| 'Cache-Control': 'public, max-age=31536000', | |
| }, | |
| }); | |
| } catch (error) { | |
| console.error('Error serving media file:', error); | |
| return new Response(`Error serving file: ${error.message}`, { | |
| status: 500, | |
| }); | |
| } | |
| } |
| // Avatar URL normalization helper | ||
| function normalizeAvatarUrl(avatarUrl: string | null | undefined): string { | ||
| if (!avatarUrl) return ''; | ||
|
|
||
| // Check if it's already an absolute URL (S3 case) | ||
| if (avatarUrl.startsWith('http://') || avatarUrl.startsWith('https://')) { | ||
| return avatarUrl; | ||
| } | ||
|
|
||
| // Check if it's a relative media path | ||
| if (avatarUrl.startsWith('media/')) { | ||
| // Convert to API route path | ||
| return `/api/${avatarUrl}`; | ||
| } | ||
|
|
||
| // Handle paths that might not have the media/ prefix | ||
| if (avatarUrl.includes('avatars/')) { | ||
| const parts = avatarUrl.split('avatars/'); | ||
| return `/api/media/avatars/${parts[parts.length - 1]}`; | ||
| } | ||
|
|
||
| // Return as is for other cases | ||
| return avatarUrl; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract normalizeAvatarUrl to a shared utility.
This function is duplicated in both user-settings.tsx and avatar-uploader.tsx. Following the DRY principle, it should be extracted to a shared utility file.
Create a new utility file (e.g., frontend/src/utils/avatar-helpers.ts) and move the function there:
// In frontend/src/utils/avatar-helpers.ts
export function normalizeAvatarUrl(avatarUrl: string | null | undefined): string {
if (!avatarUrl) return '';
// Check if it's already an absolute URL (S3 case)
if (avatarUrl.startsWith('http://') || avatarUrl.startsWith('https://')) {
return avatarUrl;
}
// Check if it's a relative media path
if (avatarUrl.startsWith('media/')) {
// Convert to API route path
return `/api/${avatarUrl}`;
}
// Handle paths that might not have the media/ prefix
if (avatarUrl.includes('avatars/')) {
const parts = avatarUrl.split('avatars/');
return `/api/media/avatars/${parts[parts.length - 1]}`;
}
// Return as is for other cases
return avatarUrl;
}Then import it in both files:
- // Avatar URL normalization helper
- function normalizeAvatarUrl(avatarUrl: string | null | undefined): string {
- if (!avatarUrl) return '';
-
- // Check if it's already an absolute URL (S3 case)
- if (avatarUrl.startsWith('http://') || avatarUrl.startsWith('https://')) {
- return avatarUrl;
- }
-
- // Check if it's a relative media path
- if (avatarUrl.startsWith('media/')) {
- // Convert to API route path
- return `/api/${avatarUrl}`;
- }
-
- // Handle paths that might not have the media/ prefix
- if (avatarUrl.includes('avatars/')) {
- const parts = avatarUrl.split('avatars/');
- return `/api/media/avatars/${parts[parts.length - 1]}`;
- }
-
- // Return as is for other cases
- return avatarUrl;
- }
+ import { normalizeAvatarUrl } from '../utils/avatar-helpers';| // Create a preview URL | ||
| const reader = new FileReader(); | ||
| reader.onload = () => { | ||
| setPreviewUrl(reader.result as string); | ||
| }; | ||
| reader.readAsDataURL(file); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for FileReader operations.
The FileReader operations lack error handling, which could lead to unhandled exceptions if file reading fails.
Add error handling for the FileReader:
// Create a preview URL
const reader = new FileReader();
reader.onload = () => {
setPreviewUrl(reader.result as string);
};
+ reader.onerror = () => {
+ toast.error('Failed to read file');
+ console.error('FileReader error:', reader.error);
+ };
reader.readAsDataURL(file);📝 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.
| // Create a preview URL | |
| const reader = new FileReader(); | |
| reader.onload = () => { | |
| setPreviewUrl(reader.result as string); | |
| }; | |
| reader.readAsDataURL(file); | |
| // Create a preview URL | |
| const reader = new FileReader(); | |
| reader.onload = () => { | |
| setPreviewUrl(reader.result as string); | |
| }; | |
| reader.onerror = () => { | |
| toast.error('Failed to read file'); | |
| console.error('FileReader error:', reader.error); | |
| }; | |
| reader.readAsDataURL(file); |
| if (error.graphQLErrors && error.graphQLErrors.length > 0) { | ||
| errorMessage = error.graphQLErrors[0].message; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check before accessing error.graphQLErrors.
Accessing properties without null checks could lead to runtime errors.
Add null check before accessing error.graphQLErrors:
let errorMessage = 'Failed to upload avatar';
- if (error.graphQLErrors && error.graphQLErrors.length > 0) {
+ if (error?.graphQLErrors?.length > 0) {
errorMessage = error.graphQLErrors[0].message;
}📝 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 (error.graphQLErrors && error.graphQLErrors.length > 0) { | |
| errorMessage = error.graphQLErrors[0].message; | |
| } | |
| let errorMessage = 'Failed to upload avatar'; | |
| if (error?.graphQLErrors?.length > 0) { | |
| errorMessage = error.graphQLErrors[0].message; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/src/graphql/type.tsx (1)
588-596: Ensure consistency inAvatarUploadResponseResolvers.
IfavatarUrlcan realistically be null, consider matching the resolver signature with aMaybe<String>return. Otherwise, returning a required string is fine if an actual URL is always guaranteed.frontend/src/app/api/media/[...path]/route.ts (4)
10-19: Good directory traversal check with path normalization.
This logic is vital for preventing unauthorized file access. One note: on certain Windows deployments, case or drive letters might produce edge cases. If you plan on cross-platform usage, keep an eye on these.
20-32: File extension allowlist is an excellent security measure.
If there's a need to serve other formats (like GIF or SVG), consider adding them to the allowlist. Otherwise, this effectively limits exposure.
41-44: File size limit check is reasonable.
For files above 10MB, or if you anticipate streaming large assets in the future, consider switching to streaming endpoints instead of pulling the entire file in memory.
46-55: Consider adding ETag or Last-Modified headers.
With a full year (max-age=31536000) cache, any updated file might not be fetched correctly without a versioned filename or additional headers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/user/user.model.ts(1 hunks)frontend/src/app/api/media/[...path]/route.ts(1 hunks)frontend/src/graphql/schema.gql(4 hunks)frontend/src/graphql/type.tsx(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/user/user.model.ts
🔇 Additional comments (19)
frontend/src/graphql/schema.gql (4)
5-9: Well-structured type definition for avatar uploads.The
AvatarUploadResponsetype is clearly defined with appropriate required fields for returning avatar upload results.
130-130: Clean implementation of the avatar upload mutation.The
uploadAvatarmutation correctly leverages the existingUploadscalar type and returns a well-defined response type.
187-187: Appropriate query definition for avatar retrieval.The
getUserAvatarquery is properly defined with a required userId parameter and returns a nullable String, which is suitable since not all users may have avatars.
239-239: User type properly extended with avatar support.The optional
avatarUrlfield is correctly added to the User type with an appropriate nullable type.frontend/src/graphql/type.tsx (11)
43-47: Consider makingavatarUrloptional for the new response type.
If there's any chance that uploading an avatar might fail or return no URL, having a requiredavatarUrlhere could pose risks ofnullreturns at runtime. Confirm whether the server always guarantees a valid URL.
175-175: Mutation field looks good.
The newuploadAvatarfield in theMutationtype aligns well with your goal. Please ensure thorough testing for scenarios where the user lacks authorization or the file upload fails.
247-249: Validate authorization checks on upload.
Ensure that only authenticated and authorized users can access theuploadAvatarmutation. This helps prevent unauthorized avatar uploads.
307-307: Query forgetUserAvataris fine.
ReturningMaybe<String>is consistent for scenarios where the avatar might not exist.
335-337: User ID required forgetUserAvatar.
Confirm that you have security measures in place so that users don’t retrieve confidential data about others.
510-510: NewAvatarUploadResponsein ResolversTypes.
No issues with adding this mapping.
550-550: Parent type mapping is correct.
No concerns here.
838-843: Confirm error handling inuploadAvatarresolver.
Verify that your resolver gracefully handles file upload failures, invalid file formats, or missing authentication.
960-965:getUserAvatarresolver definition looks correct.
The return type accommodates users without avatars. Properly handle thenullcase in the frontend if the user has no avatar.
1020-1024: OptionalavatarUrlinUsertype is appropriate.
This aligns with the possibility of users having no avatar. No issues here.
1053-1053: Resolver registration forAvatarUploadResponse.
No problems adding the new type to the resolvers map. Everything is consistent.frontend/src/app/api/media/[...path]/route.ts (4)
1-5: Imports look good.
No concerns with the imported modules and usage of the promises API forfs.
6-9: Validate non-empty param path array.
Consider checking ifparams.pathis empty or null to ensure you don't accidentally target the media directory root.
33-39: Robust file existence handling.
This try/catch pattern properly returns a 404 ifstatfails. If you want to prevent serving file system symlinks, you could verifyfileStat.isSymbolicLink()to reject them.
56-63: Error handling is effectively sanitized.
Logging the detailed error in development while returning a generic message in production is a good practice. For further security, you can omit or redact overly sensitive data fromconsole.erroreven in development if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/components/avatar-uploader.tsx (3)
96-107: Enhance error handling robustness.The implementation has improved error handling but could be more robust when accessing potentially undefined properties.
// Extract the error message if available let errorMessage = 'Failed to upload avatar'; - if (error.graphQLErrors && error.graphQLErrors.length > 0) { + if (error?.graphQLErrors?.length > 0) { errorMessage = error.graphQLErrors[0].message; }Also consider adding type information to the error parameter:
- } catch (error) { + } catch (error: any) { console.error('Error uploading avatar:', error);
117-147: UI implementation could be simplified to reduce redundant click handlers.The component has overlapping click handlers that perform the same action.
The button wrapping the entire component and the Avatar component both have onClick handlers calling
triggerFileInput. Consider simplifying:<button onClick={triggerFileInput} disabled={loading} className="flex justify-end" > <input type="file" ref={fileInputRef} onChange={handleFileChange} accept="image/*" className="hidden" /> <div className="relative group"> <Avatar className="w-24 h-24 cursor-pointer transition-opacity hover:opacity-80" - onClick={triggerFileInput} > <AvatarImage src={displayUrl} alt="User Avatar" /> <AvatarFallback>{avatarFallback}</AvatarFallback> </Avatar> <!-- rest of the component remains the same -->
117-147: Consider adding aria-label for improved accessibility.The button lacks an accessible label for screen readers.
<button onClick={triggerFileInput} disabled={loading} className="flex justify-end" + aria-label="Upload avatar" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/src/app/(main)/settings/page.tsx(1 hunks)frontend/src/components/avatar-uploader.tsx(1 hunks)frontend/src/components/chat/chat-panel.tsx(0 hunks)frontend/src/components/chat/index.tsx(3 hunks)frontend/src/components/detail-settings.tsx(2 hunks)frontend/src/components/file-sidebar.tsx(2 hunks)frontend/src/components/settings/settings.tsx(3 hunks)frontend/src/components/sidebar.tsx(2 hunks)frontend/src/components/user-settings-bar.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/chat/chat-panel.tsx
✅ Files skipped from review due to trivial changes (2)
- frontend/src/app/(main)/settings/page.tsx
- frontend/src/components/file-sidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/chat/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Install and Build Frontend
🔇 Additional comments (21)
frontend/src/components/detail-settings.tsx (2)
21-21: Component Import UpdatedThe import has been changed from
EditUsernameFormtoUserSetting, which reflects the renamed and enhanced component from the settings module.
36-36: Component Usage UpdatedThe component has been updated from
<EditUsernameForm />to<UserSetting />to match the renamed component, which now includes additional user setting capabilities like avatar management.frontend/src/components/sidebar.tsx (2)
7-7: Import Updated for Enhanced User SettingsThe import has been changed from
UserSettingstoUserSettingsBarto use the new component that includes avatar display capabilities.
234-234: User Settings Component ReplacedThe
UserSettingscomponent has been replaced withUserSettingsBarwhile maintaining the same prop interface, allowing for a seamless integration of avatar functionality in the sidebar.frontend/src/components/user-settings-bar.tsx (3)
24-47: Well-implemented URL normalization utilityThe
normalizeAvatarUrlfunction properly handles different avatar URL formats, including absolute URLs, relative media paths, and paths containing 'avatars/' segments.
53-153: Well-structured user settings bar componentThe
UserSettingsBarcomponent follows React best practices with:
- Properly memoized values and callbacks
- Clean separation of concerns
- Responsive design with
isSimpleprop support- Proper avatar fallback handling
- Normalized avatar URL implementation
The component efficiently integrates with the authentication context and provides a clean interface for navigating to settings and logging out.
155-155: Appropriate use of memoizationThe component is correctly wrapped in
memoto prevent unnecessary re-renders, optimizing performance.frontend/src/components/settings/settings.tsx (6)
16-22: Updated imports to support avatar functionalityThe import paths have been adjusted and new imports added for avatar management functionality, including the
AvatarUploadercomponent and authentication context.
58-58: Component renamed for better semantic meaningThe component has been appropriately renamed from
EditUsernameFormtoUserSettingto better reflect its expanded functionality, which now includes avatar management.
60-61: Enhanced state management with authentication contextThe component now properly uses
useAuthContextto access user data and adds state for managing avatar URLs.
68-73: Improved user data handlingThe component now correctly initializes both the username and avatar URL from the authenticated user data, replacing the previous localStorage approach.
94-97: Added handler for avatar changesThe new
handleAvatarChangefunction properly updates the avatar URL state when the user uploads a new avatar.
108-114: Integration of avatar uploader componentThe
AvatarUploadercomponent is well-integrated with appropriate props for the current avatar URL, fallback display, and change handler.frontend/src/components/avatar-uploader.tsx (8)
1-9: Appropriate imports for an avatar upload component.The imports cover all necessary dependencies for file handling, UI components, GraphQL mutations, and notifications.
11-34: Well-implemented URL normalization helper.The
normalizeAvatarUrlfunction handles various avatar URL formats comprehensively:
- Handles null/undefined values
- Preserves absolute URLs (http/https)
- Converts relative media paths to API route paths
- Handles paths with "avatars/" but missing the "media/" prefix
This ensures consistent avatar URL rendering across different storage scenarios.
36-40: Clear and well-defined component props interface.The interface
AvatarUploaderPropsprovides a clean contract for the component with appropriate types for each prop.
42-50: Proper usage of React hooks for component state.The component correctly uses:
useMutationfor GraphQL operationsuseReffor accessing the file input DOM elementuseStatefor managing preview URL stateuseAuthContextfor accessing user refresh functionality
52-67: Good client-side file validation.The implementation includes appropriate client-side validation for:
- File existence check
- Maximum file size restriction (5MB)
- Allowed file types (JPEG, PNG, WebP)
This prevents unnecessary server calls for invalid files and provides immediate user feedback.
69-74: Add error handling for FileReader operations.The FileReader operations lack error handling, which could lead to unhandled exceptions if file reading fails.
Add error handling for the FileReader:
// Create a preview URL const reader = new FileReader(); reader.onload = () => { setPreviewUrl(reader.result as string); }; + reader.onerror = () => { + toast.error('Failed to read file'); + console.error('FileReader error:', reader.error); + }; reader.readAsDataURL(file);
76-95: Well-structured avatar upload flow with user feedback.The implementation correctly:
- Uses Apollo client's file upload capabilities
- Sets necessary headers for file uploads
- Handles successful uploads with proper user notification
- Updates the parent component via callback
- Refreshes user information to ensure consistency
This provides a complete upload flow with appropriate user feedback.
110-116: Clean implementation of file input trigger and URL display logic.The implementation cleanly handles:
- Programmatic triggering of the file input
- Prioritizing preview URL over normalized current avatar URL
This creates a smooth user experience for avatar selection and preview.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/components/avatar-uploader.tsx (2)
119-149: Enhance accessibility of the avatar upload UI.The UI implementation has good visual feedback with hover effects and loading states, but could benefit from accessibility improvements.
Consider these accessibility enhancements:
<button onClick={triggerFileInput} disabled={loading} - className="flex justify-end" + className="flex justify-end" + aria-label="Upload avatar image" + title="Click to upload a new avatar image (Max: 5MB, Formats: JPEG, PNG, WebP)" >This provides users with clear instructions about file requirements before they attempt to upload.
120-147: Consider adding image optimization before upload.The current implementation uploads the original image without any optimization, which might result in unnecessarily large files.
Consider adding client-side image resizing before upload for better performance:
const optimizeImage = async (file: File, maxWidth = 512, maxHeight = 512): Promise<Blob> => { return new Promise((resolve, reject) => { const img = new Image(); img.onload = () => { const canvas = document.createElement('canvas'); let width = img.width; let height = img.height; // Calculate new dimensions while maintaining aspect ratio if (width > height) { if (width > maxWidth) { height = Math.round((height * maxWidth) / width); width = maxWidth; } } else { if (height > maxHeight) { width = Math.round((width * maxHeight) / height); height = maxHeight; } } canvas.width = width; canvas.height = height; const ctx = canvas.getContext('2d'); ctx?.drawImage(img, 0, 0, width, height); // Convert to blob canvas.toBlob( (blob) => { if (blob) resolve(blob); else reject(new Error('Canvas to Blob conversion failed')); }, file.type, 0.85 // Quality parameter for JPEG ); }; img.onerror = reject; img.src = URL.createObjectURL(file); }); }; // Then in your handleFileChange function: const optimizedFile = await optimizeImage(file); // Use optimizedFile instead of file for uploadThis would significantly reduce upload size and improve performance, especially for users with slower internet connections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/avatar-uploader.tsx(1 hunks)frontend/src/components/user-settings-bar.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/user-settings-bar.tsx
🔇 Additional comments (8)
frontend/src/components/avatar-uploader.tsx (8)
11-36: Well-structured URL normalization helper.The
normalizeAvatarUrlfunction effectively handles various avatar URL formats with appropriate comments explaining each case. The implementation covers absolute URLs, relative media paths, and paths without the media prefix.
38-42: Clean, well-defined interface.The props interface is clearly defined with appropriate types for all necessary properties required by the component.
59-62: Good client-side validation for file size.The 5MB file size limit is appropriate for avatar images and helps prevent unnecessarily large uploads. The user gets immediate feedback via toast notification.
65-69: Appropriate file type validation.Limiting uploads to JPEG, PNG, and WebP formats is a good practice for avatar images. These formats provide a good balance of quality and file size.
71-76: Add error handling for FileReader operations.The FileReader operations lack error handling, which could lead to unhandled exceptions if file reading fails.
78-98: Good implementation of the avatar upload process with proper feedback.The avatar upload implementation:
- Uses Apollo mutation with proper context for file uploads
- Provides success notification to the user
- Correctly updates the avatar URL and refreshes user information
- Has appropriate error handling
This ensures a smooth user experience during the upload process.
103-105: Add null check before accessing error.graphQLErrors.Accessing properties without null checks could lead to runtime errors.
1-151: Effective implementation of avatar upload functionality.The overall component is well-structured with proper error handling, validation, and user feedback. The component:
- Validates file size and type before upload
- Provides visual feedback during the upload process
- Handles various URL formats through the normalization helper
- Integrates with authentication context to refresh user information
The implementation follows React best practices with proper separation of concerns and componentization.
Summary by CodeRabbit
New Features
Refactor
Videos:
https://jam.dev/c/c8fc225a-d3e1-4ced-9deb-411fe67918ed