-
Notifications
You must be signed in to change notification settings - Fork 354
Create & Setup Settings page #1342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thank you for submitting your pull request! We'll review it as soon as possible. For further communication, join our discord server https://discord.gg/tSqtvHUJzE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR creates a comprehensive settings page with a sidebar navigation system and migrates the existing profile editing functionality from a standalone /edit-profile route to /settings/profile. The implementation includes a responsive layout that adapts between mobile and desktop views, a reusable searchbar component for filtering settings, and improvements to the authentication flow by fetching user data on app initialization.
Key Changes:
- Created new settings page infrastructure with sidebar navigation and nested routing
- Added
/api/user/meendpoint to fetch current authenticated user data - Migrated profile editing from standalone page to settings subsection
- Enhanced authentication hook to fetch user data on initialization
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/routes/api/user.routes.js | Added route registration for new /api/user/me endpoint |
| server/src/controllers/user/get-current-user.js | New controller to fetch current authenticated user profile |
| client/src/shared/hooks/use-auth.ts | Modified to fetch user data on initialization instead of just tokens |
| client/src/shared/hooks/use-device.ts | Added laptop breakpoint and isMobileOrTablet helper |
| client/src/shared/components/molecules/searchbar/index.tsx | New reusable searchbar component with mobile drawer support |
| client/src/modules/settings/v1/index.tsx | Main settings page with responsive sidebar and content layout |
| client/src/modules/settings/v1/hooks/index.ts | Custom hook managing settings state and routing logic |
| client/src/modules/settings/v1/hooks/get-settings.tsx | Settings configuration with routes and lazy-loaded components |
| client/src/modules/settings/v1/components/*.tsx | Settings UI components (tab, header, no-results) |
| client/src/modules/settings/modules/profile/v1/index.tsx | Migrated profile settings component |
| client/src/modules/settings/modules/profile/v1/hooks/index.ts | Profile settings logic with form validation and API calls |
| client/src/modules/settings/modules/profile/v1/components/*.tsx | Profile-specific UI components (image upload, social links) |
| client/src/modules/app/routes/auth-routes/protected-route/*.tsx | New reusable protected route wrapper with access control |
| client/src/modules/edit-profile/* | Removed old edit-profile module files |
| client/src/main.tsx | Added Jotai Provider at root level |
| client/src/App.tsx | Removed nested Jotai Provider (moved to main.tsx) |
| client/src/infra/rest/apis/user/index.ts | Added getCurrentUser API function |
Comments suppressed due to low confidence (1)
client/src/shared/hooks/use-auth.ts:49
- The clearToken function is not wrapped in useCallback but is used as a dependency in the logout callback on line 55. This could cause unnecessary re-renders and breaks the rules of hooks dependencies. Either wrap clearToken in useCallback or inline its logic directly in the logout function.
const clearToken = (): void => {
removeFromLocal(TOKEN_CONFIG.ACCESS_TOKEN_NAME);
setToken(null);
};
| ) : ( | ||
| filteredSettings.map((setting, index) => ( | ||
| <SettingsTab | ||
| key={index} |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using array index as the React key prop is an anti-pattern, especially when the list order can change or items can be filtered (as they are with the search feature). This can cause rendering bugs and performance issues. Use the setting.id field instead, which is unique and stable.
| key={index} | |
| key={setting.id} |
| throw error; | ||
| } | ||
| }, | ||
| [user] |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependency array for this useCallback hook is missing the setUser setter. While Jotai setters are stable and won't cause issues, for consistency and to follow React Hooks best practices, the dependency should be included.
| [user] | |
| [user, setUser] |
|
|
||
| userRoutes.get('/search', searchUser); | ||
| userRoutes.get('/profile', getProfile); | ||
| userRoutes.get('/me', authenticateUser, getCurrentUser); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new GET /api/user/me endpoint is not protected by a rate limiter. According to the coding guidelines, all routes should apply rate limiters (authLimiter for auth routes, generalLimiter for others). This endpoint should use generalLimiter middleware since it's a general user data fetch endpoint.
| !isMobileOrTablet && ( | ||
| <Route | ||
| key="*" | ||
| path="*" | ||
| element={ | ||
| <Navigate | ||
| to={`${ROUTES_V1.SETTINGS}${ROUTES_V1.SETTINGS_PROFILE}`} | ||
| replace | ||
| /> | ||
| } | ||
| /> | ||
| ), | ||
| ]; |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The routes array contains a conditional element (!isMobileOrTablet && <Route...>) on line 37 which could be false/undefined. While React handles this, it's cleaner to use .filter() to exclude falsy values from the array, or use conditional logic outside the array definition to avoid potential type issues.
| const { name, description, icon, path, locked, isNew, newText, feature } = | ||
| setting; | ||
| const absolutePath = `${ROUTES_V1.SETTINGS}${path}`; | ||
| const isTabActive = window.location.pathname.includes(absolutePath); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using window.location.pathname directly in a React component is not ideal because it doesn't trigger re-renders when the location changes, potentially causing stale state. Use the useLocation hook from react-router-dom instead, which is already imported and used elsewhere in the component.
| if (!user_id) { | ||
| return sendResponse(res, 401, 'User ID not found in token'); | ||
| } | ||
|
|
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for user_id on line 13 is redundant because the authenticateUser middleware already verifies the token and populates req.user. If the token is invalid or missing user_id, the middleware would have rejected the request before reaching this point. This check can be safely removed.
| if (!user_id) { | |
| return sendResponse(res, 401, 'User ID not found in token'); | |
| } |
| }; | ||
|
|
||
| const handleDrawerClose = () => { | ||
| setIsDrawerOpen(false); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the drawer closes, the search term and results are not cleared. This means when users reopen the drawer, they'll see stale search results. Consider calling handleOnClearClick in the handleDrawerClose function to reset the search state when the drawer is closed.
| setIsDrawerOpen(false); | |
| setIsDrawerOpen(false); | |
| handleOnClearClick(); |
| export const SOCIAL_LINKS_CONFIG = [ | ||
| { | ||
| key: 'youtube', | ||
| label: 'YouTube', | ||
| placeholder: 'https://youtube.com/@username', | ||
| }, | ||
| { | ||
| key: 'facebook', | ||
| label: 'Facebook', | ||
| placeholder: 'https://facebook.com/username', | ||
| }, | ||
| { key: 'x', label: 'Twitter/X', placeholder: 'https://x.com/username' }, | ||
| { | ||
| key: 'github', | ||
| label: 'GitHub', | ||
| placeholder: 'https://github.com/username', | ||
| }, | ||
| { | ||
| key: 'instagram', | ||
| label: 'Instagram', | ||
| placeholder: 'https://instagram.com/username', | ||
| }, | ||
| { key: 'website', label: 'Website', placeholder: 'https://yourwebsite.com' }, | ||
| ] as const; |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LinkedIn social link field is missing from the SOCIAL_LINKS_CONFIG array but is present in the USER_SOCIAL_LINKS type definition. This means users won't be able to update their LinkedIn profile link through the settings UI, even though the backend and type system support it.
| settings: settings, | ||
| routes: routes, |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The redundant assignment 'settings: settings' and 'routes: routes' can be simplified to just 'settings' and 'routes' using ES6 object property shorthand notation.
| settings: settings, | |
| routes: routes, | |
| settings, | |
| routes, |
| console.error('Error updating profile image:', error); | ||
| throw error; | ||
| } | ||
| }, | ||
| [user, setUser] | ||
| ); | ||
|
|
||
| const handleImageUpload = useCallback(async () => { | ||
| if (!selectedImageFile) return; | ||
|
|
||
| setUploading(true); | ||
| try { | ||
| await updateProfileImage(selectedImageFile); | ||
| setSelectedImageFile(null); | ||
| } finally { | ||
| setUploading(false); | ||
| } | ||
| }, [selectedImageFile, updateProfileImage]); | ||
|
|
||
| const updateUserProfile = useCallback( | ||
| async (profileData: UpdateProfilePayload) => { | ||
| if (!user) { | ||
| throw new Error('User not found'); | ||
| } | ||
|
|
||
| try { | ||
| const response = await updateProfile(profileData); | ||
| if (response.data && user) { | ||
| const updatedUser: USER_DB_STATE = { | ||
| ...user, | ||
| personal_info: { | ||
| ...user.personal_info, | ||
| username: response.data.username, | ||
| bio: profileData.bio, | ||
| }, | ||
| social_links: { | ||
| ...user.social_links, | ||
| ...profileData.social_links, | ||
| }, | ||
| }; | ||
| setUser(updatedUser); | ||
| } | ||
| return response; | ||
| } catch (error) { | ||
| console.error('Error updating profile:', error); | ||
| throw error; |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using console.error in production code is not recommended. Consider removing these console.error calls or replacing them with proper error handling since errors are already being re-thrown.
Pull Requests Review Criteria
Caution
PRs that fail to meet these review standards will be automatically flagged and may be rejected by maintainers.
mainDescribe the add-ons or changes you've made 📃
/api/user/meapi to persist users dataScreenshots