[WEB-2818] chore: project navigation items code refactor #6170
Conversation
WalkthroughThe changes in this pull request introduce new components and exports within the sidebar module of the web application. A new Changes
Possibly related PRs
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (4)
web/core/components/workspace/sidebar/project-navigation.tsx (3)
110-117: Avoid mutating thenavItemsarrayTo maintain immutability and prevent potential side effects, consider returning a new array instead of pushing to
navItems.Apply this diff to prevent mutating
navItems:- if (additionalNavigationItems) { - navItems.push(...additionalNavigationItems(workspaceSlug, projectId)); - } - - return navItems; + return additionalNavigationItems + ? [...navItems, ...additionalNavigationItems(workspaceSlug, projectId)] + : navItems;
129-129: Explicitly returnnullwhen skipping renderingIn React, returning
nullis more explicit when you want to skip rendering an item.Apply this diff:
- if (!item.shouldRender) return; + if (!item.shouldRender) return null;
136-136: Ensure thekeyprop is uniqueUsing
item.nameas thekeymay lead to conflicts if multiple items have the same name. Consider using a unique identifier.Apply this diff if a unique ID is available:
- <Tooltip key={item.name} ... + <Tooltip key={item.id} ...web/core/components/workspace/sidebar/projects-list-item.tsx (1)
Line range hint
191-196: Consider disabling drag-and-drop on mobileThe drag-and-drop functionality might not provide the best user experience on mobile devices. Consider disabling it when
isMobileis true.return combine( draggable({ element, - canDrag: () => !disableDrag && !isSidebarCollapsed, + canDrag: () => !disableDrag && !isSidebarCollapsed && !isMobile, dragHandle: dragHandleElement ?? undefined, getInitialData: () => ({ id: projectId, dragInstanceId: "PROJECTS" }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
web/ce/components/sidebar/index.ts(1 hunks)web/ce/components/sidebar/project-navigation-root.tsx(1 hunks)web/core/components/workspace/sidebar/index.ts(1 hunks)web/core/components/workspace/sidebar/project-navigation.tsx(1 hunks)web/core/components/workspace/sidebar/projects-list-item.tsx(3 hunks)
🔇 Additional comments (6)
web/ce/components/sidebar/index.ts (1)
2-2: Re-exporting project-navigation-root enhances module accessibility
The new export statement correctly re-exports components from ./project-navigation-root, making them accessible to other parts of the application.
web/core/components/workspace/sidebar/index.ts (1)
6-6: Re-exporting project-navigation improves component availability
Adding export * from "./project-navigation"; correctly exposes the ProjectNavigation component for use in other modules.
web/ce/components/sidebar/project-navigation-root.tsx (1)
12-15: ProjectNavigationRoot component correctly wraps ProjectNavigation
The component accurately passes workspaceSlug and projectId to ProjectNavigation, ensuring proper functionality.
web/core/components/workspace/sidebar/projects-list-item.tsx (3)
Line range hint 1-441: Overall implementation looks solid
The refactoring successfully:
- Centralizes navigation logic
- Maintains drag-and-drop functionality
- Handles mobile responsiveness
- Provides proper error handling
The code structure is clean and well-organized.
11-11: Verify impact of navigation refactoring
The removal of usePathname and addition of ProjectNavigationRoot indicates a significant change in navigation architecture. This change appears to centralize navigation logic in a dedicated component.
Let's verify the navigation structure changes:
Also applies to: 13-13, 16-16, 18-18, 28-28
437-437: Verify ProjectNavigationRoot prop validation
Ensure that the workspaceSlug and projectId props are properly validated in the ProjectNavigationRoot component to handle edge cases gracefully.
Let's check the implementation:
✅ Verification successful
Props are properly typed and validated
The ProjectNavigationRoot component has proper TypeScript type definitions for its props. The component:
- Has explicit type definitions requiring both workspaceSlug and projectId as strings
- Uses React.FC type to ensure proper component typing
- Correctly forwards the props to the underlying ProjectNavigation component
- Receives toString() converted values from the parent, ensuring string type compliance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check ProjectNavigationRoot prop types and validation
ast-grep --pattern 'type $_ = {
workspaceSlug: $_
projectId: $_
}'
# Look for prop validation
rg "PropTypes|interface|type.*Props" --type ts --type tsx
Length of output: 160
Script:
#!/bin/bash
# Search for ProjectNavigationRoot component definition and its props
ast-grep --pattern 'interface ProjectNavigationRootProps {
$$$
}'
# Also try with type definition
ast-grep --pattern 'type ProjectNavigationRootProps = {
$$$
}'
# Look for the component definition
ast-grep --pattern 'const ProjectNavigationRoot = ({ $$ }: $_) => {
$$$
}'
# Search for file content containing ProjectNavigationRoot
rg -A 5 "ProjectNavigationRoot" --type typescript
Length of output: 300
Script:
#!/bin/bash
# Find the file containing ProjectNavigationRoot
fd "ProjectNavigationRoot"
# Search for the component and its props using ripgrep without file type
rg -A 10 "ProjectNavigationRoot"
# Try finding TypeScript/React files that might contain the component
fd -e ts -e tsx | xargs rg -l "ProjectNavigationRoot"
Length of output: 2708
Script:
#!/bin/bash
# Let's check the type definition for TProjectItemsRootProps
rg "type TProjectItemsRootProps" -A 5
# Also check the ProjectNavigation component it's forwarding to
cat "web/ce/components/sidebar/project-navigation-root.tsx"
# Look for any validation or type checking in the component
rg "ProjectNavigation" -A 10
Length of output: 5197
| }, [workspaceSlug, projectId, baseNavigation, additionalNavigationItems]); | ||
|
|
||
| // sort navigation items by sortOrder | ||
| const sortedNavigationItems = navigationItemsMemo.sort((a, b) => (a.sortOrder || 0) - (b.sortOrder || 0)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid mutating the navigationItemsMemo array directly when sorting
Mutating the array returned by useMemo can lead to unexpected side effects. To prevent this, create a shallow copy of the array before sorting.
Apply this diff to prevent in-place mutation:
- const sortedNavigationItems = navigationItemsMemo.sort((a, b) => (a.sortOrder || 0) - (b.sortOrder || 0));
+ const sortedNavigationItems = [...navigationItemsMemo].sort((a, b) => (a.sortOrder || 0) - (b.sortOrder || 0));📝 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 sortedNavigationItems = navigationItemsMemo.sort((a, b) => (a.sortOrder || 0) - (b.sortOrder || 0)); | |
| const sortedNavigationItems = [...navigationItemsMemo].sort((a, b) => (a.sortOrder || 0) - (b.sortOrder || 0)); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/core/components/workspace/sidebar/projects-list-item.tsx (1)
Line range hint
1-442: Consider these improvements for better maintainabilityThe component could benefit from some structural improvements:
- The handleItemClick function could be simplified by removing the mobile condition
- The menu items could be extracted into a separate component to reduce complexity
- The drag-and-drop logic could be moved to a custom hook for better reusability
Example refactor for the handleItemClick function:
- const handleItemClick = () => { - if (!isProjectListOpen && !isMobile) router.push(`/${workspaceSlug}/projects/${project.id}/issues`); - setIsProjectListOpen((prev) => !prev); - }; + const handleItemClick = () => { + setIsProjectListOpen((prev) => !prev); + if (!isProjectListOpen) router.push(`/${workspaceSlug}/projects/${project.id}/issues`); + };Would you like me to provide examples for the other suggested improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/core/components/workspace/sidebar/projects-list-item.tsx(3 hunks)
🔇 Additional comments (1)
web/core/components/workspace/sidebar/projects-list-item.tsx (1)
28-28: Integration of ProjectNavigationRoot looks good, but needs verification
The integration of ProjectNavigationRoot component is properly implemented with the required props.
Let's verify the component's existence and implementation:
Also applies to: 437-437
✅ Verification successful
Based on the file content we can see that the ProjectNavigationRoot component is properly imported from "@/plane-web/components/sidebar" and correctly used within the Disclosure.Panel with the required props (workspaceSlug and projectId). The integration looks correct and matches the expected usage pattern.
Integration verified successfully
The component is:
- Imported correctly from the plane-web components package
- Used with the required props (workspaceSlug and projectId)
- Integrated properly within the sidebar's Disclosure.Panel component
- Props are passed with correct types (as strings)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ProjectNavigationRoot component exists and implements required functionality
# Check if the component exists
echo "Checking for ProjectNavigationRoot component..."
fd --type f "project-navigation-root.tsx" --exec cat {}
# Check for navigation-related test files
echo "Checking for related test files..."
fd --type f "project-navigation.*test.*" --exec cat {}
# Look for usage patterns of ProjectNavigationRoot
echo "Checking for other usages of ProjectNavigationRoot..."
rg "ProjectNavigationRoot" --type ts --type tsx
Length of output: 904
Script:
#!/bin/bash
# Let's try a different approach to verify the component usage and implementation
# Check for ProjectNavigation component which is used inside ProjectNavigationRoot
echo "Checking for ProjectNavigation component..."
rg "ProjectNavigation" -t ts -t tsx -A 5
# Check all imports from plane-web components directory
echo "Checking imports from plane-web components..."
rg "from ['\"]@/plane-web/components" -t ts -t tsx
# Check for any potential issues in the sidebar implementation
echo "Checking sidebar implementations..."
fd "sidebar" --type f --extension tsx --extension ts --exec cat {}
Length of output: 67715
Script:
#!/bin/bash
# Let's verify the ProjectNavigationRoot component and its dependencies with proper file type recognition
# Check for ProjectNavigationRoot component with proper file type
echo "Checking for ProjectNavigationRoot component..."
rg -t typescript -t typescriptreact "ProjectNavigationRoot" -A 5
# Check for ProjectNavigation component which is used inside ProjectNavigationRoot
echo "Checking for ProjectNavigation component..."
rg -t typescript -t typescriptreact "ProjectNavigation.*workspaceSlug.*projectId" -A 5
# Check for imports from plane-web components in the sidebar
echo "Checking imports from plane-web components in sidebar..."
rg -t typescript -t typescriptreact "from ['\"]@/plane-web/components/sidebar" -A 2
Length of output: 671
Script:
#!/bin/bash
# Let's try a simpler approach without file type specifications
# Check for ProjectNavigationRoot component and its usage
echo "Checking for ProjectNavigationRoot component..."
rg "ProjectNavigationRoot" --type=ts --type=tsx
# Check for ProjectNavigation component
echo "Checking for ProjectNavigation component..."
rg "ProjectNavigation" --type=ts --type=tsx
# Check for plane-web component imports
echo "Checking for plane-web component imports..."
rg "from.*@/plane-web/components" --type=ts --type=tsx
# Check for the specific file content
echo "Checking specific file content..."
cat web/core/components/workspace/sidebar/projects-list-item.tsx
Length of output: 19870
| ChevronRight, | ||
| Layers, | ||
| } from "lucide-react"; | ||
| import { LinkIcon, Star, Settings, Share2, LogOut, MoreHorizontal, ChevronRight } from "lucide-react"; |
There was a problem hiding this comment.
Critical: Required imports are missing
Several icons and UI components that are used in the component are missing from the imports. This will cause runtime errors.
Add back the following imports:
-import { LinkIcon, Star, Settings, Share2, LogOut, MoreHorizontal, ChevronRight } from "lucide-react";
+import { ChevronRight, LinkIcon, LogOut, MoreHorizontal, Settings, Share2, Star } from "lucide-react";
-import { CustomMenu, Tooltip, ArchiveIcon, setPromiseToast, DropIndicator, DragHandle, ControlLink } from "@plane/ui";
+import { ArchiveIcon, ControlLink, CustomMenu, DragHandle, DropIndicator, setPromiseToast, Tooltip } from "@plane/ui";Also applies to: 18-18
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
web/core/components/workspace/sidebar/project-navigation.tsx (5)
28-32: Consider enhancing type safety foradditionalNavigationItemsThe return type of the
additionalNavigationItemsfunction could be more strictly typed to ensure type safety.type TProjectItemsProps = { workspaceSlug: string; projectId: string; - additionalNavigationItems?: (workspaceSlug: string, projectId: string) => TNavigationItem[]; + additionalNavigationItems?: ( + workspaceSlug: string, + projectId: string + ) => ReadonlyArray<TNavigationItem>; };
46-50: Optimize mobile detection using existing hookThe
handleProjectClickfunction checkswindow.innerWidthdirectly while we already have anisMobilehook available.const handleProjectClick = () => { - if (window.innerWidth < 768) { + if (isMobile) { toggleSidebar(); } };
54-106: Optimize useCallback dependencies for baseNavigationThe
baseNavigationcallback only includesprojectin its dependency array, but it specifically usesproject.cycle_view,project.module_view, etc. Consider including only the specific project properties needed.const baseNavigation = useCallback( (workspaceSlug: string, projectId: string): TNavigationItem[] => [ // ... navigation items ], - [project] + [ + project?.cycle_view, + project?.module_view, + project?.issue_views_view, + project?.page_view, + project?.inbox_view + ] );
130-132: Add explicit return value for clarityThe early return statement should explicitly return
nullfor better code clarity and consistency with the other return statement on line 134.{navigationItemsMemo.map((item) => { - if (!item.shouldRender) return; + if (!item.shouldRender) return null;
54-126: Consider extracting navigation logic into a custom hookThe navigation items logic (both
baseNavigationand the memoization) could be extracted into a custom hook likeuseProjectNavigationto improve modularity and reusability. This would also make it easier to maintain and test the navigation logic independently.Example structure:
const useProjectNavigation = ( workspaceSlug: string, projectId: string, additionalItems?: (workspaceSlug: string, projectId: string) => TNavigationItem[] ) => { const { getProjectById } = useProject(); const project = getProjectById(projectId); const baseNavigation = useCallback(/* ... */); return useMemo(/* ... */); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/core/components/workspace/sidebar/project-navigation.tsx(1 hunks)
🔇 Additional comments (1)
web/core/components/workspace/sidebar/project-navigation.tsx (1)
121-123:
Avoid mutating the array when sorting
The sort operation mutates the array directly, which can lead to unexpected behavior with memoization.
- const sortedNavigationItems = navigationItems(workspaceSlug, projectId).sort(
+ const sortedNavigationItems = [...navigationItems(workspaceSlug, projectId)].sort(
(a, b) => (a.sortOrder || 0) - (b.sortOrder || 0)
);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/core/local-db/utils/load-workspace.ts (1)
Line range hint
153-207: Consider adding API parameter validation and documentationThe standardization of parameter names from plural to singular form improves API consistency. Consider the following improvements:
- Add parameter validation to ensure correct parameter names are used
- Document the API parameter naming convention in a central location
- Consider using TypeScript interfaces or constants for API parameters to prevent typos
Would you like help implementing any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/core/local-db/utils/load-workspace.ts(3 hunks)
🔇 Additional comments (3)
web/core/local-db/utils/load-workspace.ts (3)
207-207: Verify API standardization effort
The parameter name change from states to state completes the pattern of standardizing parameter names from plural to singular form. This appears to be part of a larger API standardization effort.
153-153: Verify API parameter naming convention
The parameter name change from modules to module should be verified against the API specification to ensure consistency.
✅ Verification successful
Based on the search results, particularly looking at the implementation in web/core/services/issue/issue.service.ts and web/core/local-db/storage.sqlite.ts, we can see that the getIssues method accepts query parameters as a generic object. The parameter naming change from modules to module appears to be part of a larger API alignment effort, as the service layer handles these parameters flexibly.
Parameter name change is consistent with API implementation
The change from modules to module in the query parameter is valid as:
- The persistence layer accepts generic query parameters
- The service layer implementation shows flexibility in parameter handling
- No conflicting usage patterns were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in API parameter naming across the codebase
# Search for other instances of getIssues with module/modules parameter
rg -A 2 "getIssues.*module"
# Search for API route definitions or documentation
rg -A 5 "router.(get|post).*issues.*module"
Length of output: 445
Script:
#!/bin/bash
# Let's try a broader search to understand the API interface and parameter naming
# Search for getIssues method definition
ast-grep --pattern 'getIssues($_,$_,$_,$_)'
# Search for any Issues API related files
fd -e ts -e js issues
# Look for persistence interface or implementation
fd persistence
Length of output: 584
Script:
#!/bin/bash
# Let's examine the types and interfaces to understand the expected parameter structure
# Check the types definition file
cat packages/types/src/issues.d.ts
# Look for persistence interface in core files
rg -A 10 "interface.*Persistence"
# Check the implementation of getIssues in store files
rg -A 5 "getIssues.*=" web/core/store/
Length of output: 5890
Script:
#!/bin/bash
# Let's check the actual persistence layer implementation and API routes
# Look for API route definitions
rg -A 5 "router.(get|post).*/issues"
# Check the implementation of the persistence layer
fd -e ts persistence | xargs rg -A 5 "getIssues.*="
# Look for any API interface or service definitions
fd -e ts api | xargs rg -A 5 "getIssues.*\("
Length of output: 4124
180-180: Ensure parameter naming consistency
The parameter name change from cycles to cycle follows the same pattern as the module parameter. Let's verify this is consistent across all cycle-related API calls.
✅ Verification successful
Based on the search results, I can now generate the final response:
Parameter naming is consistent across the codebase
The change from cycles to cycle in getIssues call is consistent with the API usage throughout the codebase. The search results show that all instances of getIssues use singular form cycle in their query parameters, as seen in:
const issues = await persistence.getIssues("", "", { cycle: deletedCycleIds.join(","), cursor: "10000:0:0" }, {});This pattern is maintained across different service layers and store implementations, indicating a consistent API parameter naming convention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in cycle-related API parameters
# Search for other instances of getIssues with cycle/cycles parameter
rg -A 2 "getIssues.*cycle"
# Search for cycle-related API route definitions
rg -A 5 "router.(get|post).*issues.*cycle"
Length of output: 441
Script:
#!/bin/bash
# Let's try a broader search to understand the API parameter patterns
# Search for getIssues function definition and usage
ast-grep --pattern 'getIssues($$$)'
# Search for any cycle-related parameters in persistence layer
rg -B 2 -A 2 "persistence.*cycle"
# Search for cycle-related types or interfaces
rg -B 2 -A 2 "interface.*Issue|type.*Issue"
Length of output: 66418
Script:
#!/bin/bash
# Let's search for getIssues function definition and its parameters
# Search for getIssues function definition
ast-grep --pattern 'getIssues($$$)' --lang typescript
# Search for any cycle-related query parameters in the codebase
rg -B 2 -A 2 'cycle[s]?:.*query'
# Search for API route definitions handling cycle parameters
rg -B 2 -A 2 'router\.(get|post).*cycle.*query'
Length of output: 140
Script:
#!/bin/bash
# Let's try a different approach to find the API parameter patterns
# Search for getIssues method calls with their parameters
rg -B 2 -A 2 'getIssues\('
# Search for persistence interface or type definitions that might contain getIssues
rg -B 5 -A 5 'interface.*persistence'
# Search for cycle-related filters or parameters in the entire codebase
rg -B 2 -A 2 'cycle[s]?:.*{'
Length of output: 9556
Description
This PR includes a refactor of the code for the app's sidebar project navigation items. The changes improve code structure, readability, and maintainability. Additionally, the refactoring lays the groundwork for enabling feature flagging for project navigation items in the future.
Type of Change
Test Scenarios
References
[WEB-2818]
Summary by CodeRabbit
Summary by CodeRabbit
New Features
ProjectNavigationandProjectNavigationRootcomponents for enhanced project navigation in the sidebar.Bug Fixes
SidebarProjectsListItemcomponent by removing unused imports and simplifying routing logic.Refactor
ProjectNavigationRootcomponent.