Created home page navbar & setup project module and its route #1343
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.
Pull request overview
This PR restructures the home page and project viewing experience by introducing a dedicated home page navbar, migrating the project module to a nested v1 route structure, and optimizing API calls to prevent unnecessary duplicate requests. The changes establish a clearer separation between the home feed and individual project pages while improving performance through better state management.
Key Changes:
- Implemented a simplified home page navbar without authentication-dependent features
- Migrated project module from
/modules/projectto/modules/home/modules/project/v1with proper nested routing - Optimized
/meAPI calls by adding ref-based tracking to prevent duplicate fetches on component remounts
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
client/src/shared/hooks/use-auth.ts |
Added ref tracking and user state check to prevent duplicate /me API calls |
client/src/shared/components/organisms/navbar/index.tsx |
Simplified navbar to remove authentication features, theme toggle, and user menu |
client/src/shared/components/organisms/navbar/hooks/index.ts |
Refactored navbar hooks to manage search state and keyboard shortcuts |
client/src/shared/components/organisms/navbar/components/render-mobile-menu.tsx |
Simplified mobile menu to only show Write and Subscribe options |
client/src/shared/components/organisms/navbar/components/render-menu.tsx |
Removed entire user profile menu component |
client/src/shared/components/organisms/comments-wrapper/index.tsx |
Updated import path to reflect project module migration |
client/src/shared/components/organisms/comments-wrapper/hooks/index.ts |
Enhanced comment fetching with reset functionality and project change detection |
client/src/shared/components/organisms/comments-wrapper/components/comment-field.tsx |
Updated import path for SelectedProjectAtom |
client/src/shared/components/organisms/comments-wrapper/components/comment-card.tsx |
Updated import path for SelectedProjectAtom |
client/src/shared/components/molecules/searchbar/index.tsx |
Converted to forwardRef pattern with improved ref handling and keyboard event support |
client/src/shared/components/atoms/typography/index.tsx |
Added support for dangerouslySetInnerHTML without rendering text prop |
client/src/modules/settings/v1/hooks/index.ts |
Updated to use new settingsRoutes function name |
client/src/modules/settings/routes/index.tsx |
Refactored settings routes with new constant structure and lazy loading |
client/src/modules/settings/modules/index.tsx |
Added centralized lazy component exports for settings modules |
client/src/modules/search/index.tsx |
Updated hook name from useHome to useHomeV1 |
client/src/modules/project/index.tsx |
Deleted old project component (migrated to new location) |
client/src/modules/project/components/project-content.tsx |
Deleted old project content component (migrated to new location) |
client/src/modules/profile/index.tsx |
Updated hook name from useHome to useHomeV1 |
client/src/modules/profile/hooks/index.ts |
Updated hook name from useHome to useHomeV1 |
client/src/modules/notification/components/notificationCard.tsx |
Updated project links to use ROUTES_V1.HOME constant |
client/src/modules/manage-projects/components/publish-projects.tsx |
Updated project links to use ROUTES_V1.HOME constant |
client/src/modules/home/v1/states/index.ts |
Removed unused state atoms (HomePageStateAtom, HomePageTrendingProjectsAtom) |
client/src/modules/home/v1/index.tsx |
Added navbar, nested routing support, and improved project fetching logic |
client/src/modules/home/v1/hooks/index.ts |
Refactored to support nested routing with isHomePage check and proper memoization |
client/src/modules/home/v1/components/no-banner-project.tsx |
Migrated to use A2ZTypography and updated route constants |
client/src/modules/home/v1/components/banner-project-card.tsx |
Migrated to use A2ZTypography and updated route constants |
client/src/modules/home/routes/index.tsx |
Added new home routes configuration for nested project routing |
client/src/modules/home/modules/project/v1/states/index.ts |
Fixed import path for ProjectData type |
client/src/modules/home/modules/project/v1/index.tsx |
Migrated project component with improved external link handling and styling |
client/src/modules/home/modules/project/v1/hooks/use-project-interaction.ts |
Simplified like handling by removing authentication check |
client/src/modules/home/modules/project/v1/hooks/index.ts |
Added ref-based duplicate fetch prevention and reset functionality |
client/src/modules/home/modules/project/v1/components/project-interaction.tsx |
Added module-level refs to prevent duplicate like status API calls |
client/src/modules/home/modules/project/v1/components/project-content.tsx |
Migrated with A2ZTypography usage and improved theme-aware styling |
client/src/modules/home/modules/index.tsx |
Added centralized lazy component exports for home modules |
client/src/modules/app/routes/constants/routes.ts |
Restructured route constants into separate enums for better organization |
client/src/modules/app/routes/auth-routes/v1/index.tsx |
Updated home route to support wildcard nested routing |
Comments suppressed due to low confidence (2)
client/src/modules/home/modules/project/v1/components/project-interaction.tsx:61
- The
setLikedByUserdependency is missing from the useEffect dependency array. While the code usessetLikedByUser, it's not included in the dependencies on line 61. This violates React's exhaustive-deps rule.
client/src/modules/home/modules/project/v1/components/project-interaction.tsx:19 - The ref logic for preventing duplicate fetches should use a React ref (
useRef) instead of a module-level object. Module-level refs are shared across all component instances, which could cause issues if multiple instances of this component are mounted. Consider usinguseRefwithin the component and passing it through context or lifting the state management to a parent component.
| useEffect(() => { | ||
| document.addEventListener('keydown', triggerSearchByKeyboard, true); | ||
| return () => { | ||
| document.removeEventListener('keydown', triggerSearchByKeyboard, true); | ||
| }; | ||
| }, [isDesktop]); |
There was a problem hiding this comment.
The useEffect hook has a missing dependency. The triggerSearchByKeyboard function is referenced in the dependency array, but it's defined in the same file and recreated on every render. This could cause the effect to run more times than necessary. Consider wrapping triggerSearchByKeyboard in a useCallback hook or removing it from the dependencies if isDesktop is sufficient.
| const fetchLatestProjects = useCallback( | ||
| async (page = 1) => { | ||
| if (!isHomePage) return; | ||
| const response = await getAllProjects(page); | ||
| if (response.data) { | ||
| setProjects(response.data); | ||
| } | ||
| }, | ||
| [setProjects] | ||
| ); | ||
|
|
||
| const fetchTrendingProjects = async () => { | ||
| const fetchTrendingProjects = useCallback(async () => { | ||
| if (!isHomePage) return; | ||
| const response = await getTrendingProjects(); | ||
| if (response.data) { | ||
| setTrending(response.data); | ||
| setTrendingProjects(response.data); | ||
| } | ||
| }; | ||
| }, [setTrendingProjects]); |
There was a problem hiding this comment.
The isHomePage dependency is missing from the useCallback dependencies. The callback functions fetchLatestProjects, fetchTrendingProjects, and fetchProjectsByCategory all check isHomePage internally, so it should be included in their dependency arrays. Without it, these functions may use stale values of isHomePage.
| const fetchProjectsByCategory = useCallback( | ||
| async ({ | ||
| tag, | ||
| query, | ||
| user_id, | ||
| page, | ||
| limit, | ||
| page = 1, | ||
| limit = 10, | ||
| rmv_project_by_id, | ||
| }); | ||
| if (response.data) { | ||
| setProjects(response.data); | ||
| } | ||
| }; | ||
| }: { | ||
| tag?: string; | ||
| query?: string; | ||
| user_id?: string; | ||
| page?: number; | ||
| limit?: number; | ||
| rmv_project_by_id?: string; | ||
| }) => { | ||
| if (!isHomePage) return; | ||
| const response = await searchProjects({ | ||
| tag, | ||
| query, | ||
| user_id, | ||
| page, | ||
| limit, | ||
| rmv_project_by_id, | ||
| }); | ||
| if (response.data) { | ||
| setProjects(response.data); | ||
| } | ||
| }, | ||
| [setProjects] | ||
| ); |
There was a problem hiding this comment.
The isHomePage dependency is missing from the useCallback dependencies. This function checks isHomePage internally but doesn't include it in the dependency array, which could lead to stale closure issues.
| // Only fetch user if we don't already have user data and haven't fetched yet | ||
| if (!user && !hasFetchedUserRef.current) { | ||
| hasFetchedUserRef.current = true; | ||
| try { | ||
| const response = await getCurrentUser(); | ||
| if (response.status === 'success' && response.data) { | ||
| setUser(response.data); | ||
| } | ||
| } catch (error) { | ||
| // If fetching user fails, token might be invalid | ||
| // Don't clear token here - let the interceptor handle it | ||
| console.error('Failed to fetch current user:', error); | ||
| hasFetchedUserRef.current = false; // Allow retry on next mount if needed | ||
| } |
There was a problem hiding this comment.
The check hasFetchedUserRef.current in the condition will always allow the user fetch to happen on first mount since the ref starts as false. However, if the user atom already has data (e.g., from a previous session or hydration), this will still trigger an unnecessary API call. Consider also checking if user data already exists before making the API call: if (!user && !hasFetchedUserRef.current). This appears to be the intended behavior based on the comment, but it's worth verifying.
| export const homeRoutes = () => { | ||
| const routes: React.ReactNode[] = [ | ||
| <Route | ||
| key={ROUTES_HOME_V1.PROJECT} | ||
| path={ROUTES_HOME_V1.PROJECT} | ||
| element={ | ||
| <ProtectedRoute component={ProjectLazyComponentV1} hasAccess={true} /> | ||
| } | ||
| />, | ||
| ]; | ||
|
|
||
| return { routes }; | ||
| }; |
There was a problem hiding this comment.
The function is exported but lacks JSDoc documentation explaining its purpose, parameters, and return value. Consider adding documentation to describe what this function does, especially since it's a utility function that handles route configuration.
| const HeaderBlock = ({ level, text }: { level: number; text: string }) => { | ||
| const variant = level === 3 ? 'h5' : 'h4'; | ||
| const component = level === 3 ? 'h3' : 'h2'; | ||
| return ( | ||
| <A2ZTypography | ||
| variant={variant} | ||
| component={component} | ||
| text="" | ||
| props={{ | ||
| dangerouslySetInnerHTML: { __html: text }, | ||
| sx: { fontWeight: 600, mb: 2, mt: 3 }, | ||
| }} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Similar to the ParagraphBlock, using text="" with dangerouslySetInnerHTML is unnecessary. Omit the text prop when using dangerouslySetInnerHTML.
| {items.map((item, i) => ( | ||
| <li key={i}> | ||
| <A2ZTypography | ||
| variant="body1" | ||
| text="" | ||
| props={{ | ||
| dangerouslySetInnerHTML: { __html: item }, | ||
| }} | ||
| /> | ||
| </li> |
There was a problem hiding this comment.
Using text="" with dangerouslySetInnerHTML is redundant. Omit the text prop when using dangerouslySetInnerHTML.
| const handleClearClick = () => { | ||
| handleOnClearClick(); | ||
| // Focus the input after clearing | ||
| setTimeout(() => { | ||
| const refToUse = inputRef || ref || internalInputRef; | ||
| if ( | ||
| refToUse && | ||
| typeof refToUse === 'object' && | ||
| 'current' in refToUse && | ||
| refToUse.current | ||
| ) { | ||
| refToUse.current.focus(); | ||
| } |
There was a problem hiding this comment.
The ref handling logic is complex and could be simplified. The code attempts to handle three different ref sources (inputRef, ref, and internalInputRef) and checks if they're objects with a 'current' property. This is overly complex. Consider using a single ref approach by forwarding the ref properly through the component, or simplify the ref merging logic.
| const handleClearClick = () => { | |
| handleOnClearClick(); | |
| // Focus the input after clearing | |
| setTimeout(() => { | |
| const refToUse = inputRef || ref || internalInputRef; | |
| if ( | |
| refToUse && | |
| typeof refToUse === 'object' && | |
| 'current' in refToUse && | |
| refToUse.current | |
| ) { | |
| refToUse.current.focus(); | |
| } | |
| const focusInput = () => { | |
| const possibleRefs = [inputRef, ref, internalInputRef]; | |
| for (const candidate of possibleRefs) { | |
| if ( | |
| candidate && | |
| typeof candidate === 'object' && | |
| 'current' in candidate && | |
| candidate.current | |
| ) { | |
| candidate.current.focus(); | |
| return; | |
| } | |
| } | |
| }; | |
| const handleClearClick = () => { | |
| handleOnClearClick(); | |
| // Focus the input after clearing | |
| setTimeout(() => { | |
| focusInput(); |
| const ParagraphBlock = ({ text }: { text: string }) => { | ||
| return ( | ||
| <A2ZTypography | ||
| variant="body1" | ||
| text="" | ||
| props={{ | ||
| dangerouslySetInnerHTML: { __html: text }, | ||
| sx: { lineHeight: 1.8 }, | ||
| }} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
The empty text="" prop passed to A2ZTypography when using dangerouslySetInnerHTML is unnecessary and could be confusing. When using dangerouslySetInnerHTML, you should omit the text prop entirely since it won't be used. This makes the component's API clearer.
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 📃
/meapi calls