Skip to content
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

[bug]: Wrong implementation of the projects list item #5986

Open
9 tasks
oussamakhadraoui opened this issue Nov 11, 2024 · 1 comment
Open
9 tasks

[bug]: Wrong implementation of the projects list item #5986

oussamakhadraoui opened this issue Nov 11, 2024 · 1 comment
Assignees
Labels
🐛bug Something isn't working

Comments

@oussamakhadraoui
Copy link

oussamakhadraoui commented Nov 11, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

Sidebar Projects List Component Optimization

Current Behavior

The current sidebar projects list implementation (SidebarProjectsListItem) has several areas that could be optimized for better performance, maintainability, and user experience.

Issues and Suggested Improvements

1. Performance Optimizations

  • Multiple Re-renders: The component has several state variables (isMenuActive, isDragging, isProjectListOpen) that could trigger unnecessary re-renders. Consider consolidating these into a single state object.
  • Complex Effect Dependencies: The main useEffect for drag-and-drop has multiple dependencies that could be memoized.
// Current
useEffect(() => {
  // drag-and-drop logic
}, [projectRef?.current, dragHandleRef?.current, projectId, isLastChild, projectListType, handleOnProjectDrop]);

// Suggested
const dragDropDeps = useMemo(
  () => ({
    projectRef: projectRef?.current,
    dragHandle: dragHandleRef?.current,
    projectId,
    isLastChild,
    projectListType
  }),
  [projectRef?.current, dragHandleRef?.current, projectId, isLastChild, projectListType]
);

2. Code Organization

  • Component Size: The component is quite large (400+ lines) and handles multiple responsibilities. Consider splitting into smaller, focused components:
    // Suggested structure
    - SidebarProjectsListItem/
      ├── index.tsx
      ├── ProjectHeader.tsx
      ├── ProjectNavigation.tsx
      ├── ProjectActions.tsx
      └── hooks/
          ├── useProjectDragDrop.ts
          └── useProjectNavigation.ts

3. Type Safety Improvements

  • Props Interface: Current Props type could be more specific:
interface ProjectsListItemProps {
  projectId: string;
  handleCopyText: () => void;
  handleOnProjectDrop?: (
    sourceId: string,
    destinationId: string,
    shouldDropAtEnd: boolean
  ) => Promise<void>; // Make async explicit
  projectListType: "JOINED" | "FAVORITES";
  disableDrag: boolean;
  disableDrop: boolean;
  isLastChild: boolean;
}

4. Drag and Drop Logic

  • Error Handling: Add error boundaries for drag-and-drop operations
  • Loading States: Include visual feedback during drag operations
  • Accessibility: Add ARIA attributes for drag-and-drop interactions
const dragHandleProps = {
  role: "button",
  "aria-label": "Drag to reorder project",
  "aria-grabbed": isDragging,
  "aria-dropeffect": "move"
};

5. Mobile Experience

  • Current mobile detection relies on window width:
// Current
if (window.innerWidth < 768) {
  toggleSidebar();
}

// Suggested
const { isMobile, breakpoint } = useResponsive();
if (isMobile) {
  toggleSidebar();
}

6. State Management

  • Consider using a reducer for complex state interactions:
type ProjectState = {
  isMenuActive: boolean;
  isDragging: boolean;
  isOpen: boolean;
  instruction?: "DRAG_OVER" | "DRAG_BELOW";
};

type ProjectAction = 
  | { type: "TOGGLE_MENU" }
  | { type: "SET_DRAGGING"; payload: boolean }
  | { type: "SET_INSTRUCTION"; payload?: "DRAG_OVER" | "DRAG_BELOW" }
  | { type: "TOGGLE_OPEN" };

const projectReducer = (state: ProjectState, action: ProjectAction): ProjectState => {
  // reducer implementation
};

Technical Implementation

Here's a suggested approach for the refactor:

  1. Split Component:
// ProjectHeader.tsx
const ProjectHeader: FC<ProjectHeaderProps> = ({ project, isCollapsed }) => {
  const { logo, name } = project;
  return (
    <div className="flex items-center gap-2">
      <Logo logo={logo} size={16} />
      {!isCollapsed && <span className="truncate">{name}</span>}
    </div>
  );
};
  1. Custom Hooks:
// useProjectDragDrop.ts
const useProjectDragDrop = (props: DragDropProps) => {
  const { projectId, handleDrop, disabled } = props;
  
  return {
    dragHandleProps,
    isDragging,
    dropIndicator
  };
};

Testing Plan

  1. Unit tests for individual components
  2. Integration tests for drag-and-drop functionality
  3. Mobile responsiveness testing
  4. Performance benchmarking before/after refactor

Migration Strategy

  1. Create new component structure
  2. Add new components alongside existing ones
  3. Gradually migrate features
  4. Add comprehensive tests
  5. Remove old implementation

Questions

  • Should we consider using a virtual list for large project lists?
  • Do we need to support nested project hierarchies in the future?
  • Should we implement undo/redo for drag-and-drop operations?

Tasks

  • Split component into smaller pieces
  • Implement proper TypeScript interfaces
  • Add error boundaries
  • Improve mobile experience
  • Add comprehensive tests
  • Document new component structure
  • Add performance monitoring
  • Update drag-and-drop accessibility

/label enhancement, performance
/label documentation
/milestone v2.0.0

Steps to reproduce

Just follow the instruction

Environment

Production

Browser

Google Chrome

Variant

Cloud

Version

last version

@oussamakhadraoui oussamakhadraoui added the 🐛bug Something isn't working label Nov 11, 2024
@pushya22 pushya22 assigned SatishGandham and unassigned vihar and pushya22 Nov 11, 2024
@SatishGandham
Copy link
Collaborator

Hello @oussamakhadraoui,

Thank you for taking the time to report this issue. The suggested performance optimizations don’t seem applicable, as the component needs to re-render if any of these values change. Even if they were, the performance gains are unlikely to be significant, so this is not a current priority for us.

We'll keep the issue open for further review and will assess it in due course.

Let us know if you disagree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants