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

[WEB-2774] Chore: re-ordering functionality for entities in favorites. #6078

Open
wants to merge 9 commits into
base: preview
Choose a base branch
from
87 changes: 48 additions & 39 deletions web/core/components/workspace/sidebar/favorites/favorite-folder.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,17 @@
import { useEffect, useRef, useState } from "react";
import { combine } from "@atlaskit/pragmatic-drag-and-drop/combine";
import { draggable, dropTargetForElements } from "@atlaskit/pragmatic-drag-and-drop/element/adapter";
import { pointerOutsideOfPreview } from "@atlaskit/pragmatic-drag-and-drop/element/pointer-outside-of-preview";
import { setCustomNativeDragPreview } from "@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview";

import { attachClosestEdge, extractClosestEdge } from "@atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge";
import orderBy from "lodash/orderBy";
import uniqBy from "lodash/uniqBy";
import { useParams } from "next/navigation";
import { createRoot } from "react-dom/client";
import { PenSquare, Star, MoreHorizontal, ChevronRight, GripVertical } from "lucide-react";
import { Disclosure, Transition } from "@headlessui/react";

// plane helpers
import { useOutsideClickDetector } from "@plane/helpers";
// ui
Expand All @@ -30,15 +35,16 @@ type Props = {
favorite: IFavorite;
handleRemoveFromFavorites: (favorite: IFavorite) => void;
handleRemoveFromFavoritesFolder: (favoriteId: string) => void;
handleReorder: (favoriteId: string, sequence: number) => void;
};

export const FavoriteFolder: React.FC<Props> = (props) => {
const { favorite, handleRemoveFromFavorites, handleRemoveFromFavoritesFolder } = props;
const { favorite, handleRemoveFromFavorites, handleRemoveFromFavoritesFolder, handleReorder } = props;
// store hooks
const { sidebarCollapsed: isSidebarCollapsed } = useAppTheme();

const { isMobile } = usePlatformOS();
const { moveFavorite, getGroupedFavorites, groupedFavorites, moveFavoriteFolder } = useFavorite();
const { getGroupedFavorites, groupedFavorites, moveFavoriteToFolder } = useFavorite();
const { workspaceSlug } = useParams();
// states
const [isMenuActive, setIsMenuActive] = useState(false);
Expand All @@ -51,10 +57,10 @@ export const FavoriteFolder: React.FC<Props> = (props) => {
const actionSectionRef = useRef<HTMLDivElement | null>(null);
const elementRef = useRef<HTMLDivElement | null>(null);

!favorite.children && getGroupedFavorites(workspaceSlug.toString(), favorite.id);
if(!favorite.children) getGroupedFavorites(workspaceSlug.toString(), favorite.id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for grouped favorites fetch

The conditional fetch should include error handling to gracefully handle failures.

-  if(!favorite.children) getGroupedFavorites(workspaceSlug.toString(), favorite.id);
+  if(!favorite.children) {
+    getGroupedFavorites(workspaceSlug.toString(), favorite.id)
+      .catch((error) => {
+        setToast({
+          type: TOAST_TYPE.ERROR,
+          title: "Error!",
+          message: "Failed to fetch favorites.",
+        });
+      });
+  }

Committable suggestion skipped: line range outside the PR's diff.


const handleOnDrop = (source: string, destination: string) => {
moveFavorite(workspaceSlug.toString(), source, {
const handleMoveToFolder = (source: string, destination: string) => {
moveFavoriteToFolder(workspaceSlug.toString(), source, {
parent: destination,
})
.then(() => {
Expand All @@ -73,24 +79,6 @@ export const FavoriteFolder: React.FC<Props> = (props) => {
});
};

const handleOnDropFolder = (payload: Partial<IFavorite>) => {
moveFavoriteFolder(workspaceSlug.toString(), favorite.id, payload)
.then(() => {
setToast({
type: TOAST_TYPE.SUCCESS,
title: "Success!",
message: "Folder moved successfully.",
});
})
.catch(() => {
setToast({
type: TOAST_TYPE.ERROR,
title: "Error!",
message: "Failed to move folder.",
});
});
};

useEffect(() => {
const element = elementRef.current;

Expand All @@ -101,22 +89,36 @@ export const FavoriteFolder: React.FC<Props> = (props) => {
draggable({
element,
getInitialData: () => initialData,
onDragStart: () => setIsDragging(true),
// onDragStart: () => setIsDragging(true),
mathalav55 marked this conversation as resolved.
Show resolved Hide resolved
onGenerateDragPreview: ({ nativeSetDragImage }) =>{
setCustomNativeDragPreview({
getOffset: pointerOutsideOfPreview({ x: "0px", y: "0px" }),
render: ({ container }) => {
const root = createRoot(container);
root.render(
<div className="rounded flex gap-1 bg-custom-background-100 text-sm p-1 pr-2">
<div className="size-5 grid place-items-center flex-shrink-0">
<FavoriteFolderIcon />
</div>
<p className="truncate text-sm font-medium text-custom-sidebar-text-200">{favorite.name}</p>
</div>
);
return () => root.unmount();
},
nativeSetDragImage,
});
},
onDrop: (data) => {
setIsDraggedOver(false);
if (!data.location.current.dropTargets[0]) return;
const destinationData = data.location.current.dropTargets[0].data;

if (favorite.id && destinationData) {
const edge = extractClosestEdge(destinationData) || undefined;
const payload = {
id: favorite.id,
sequence: Math.round(
getDestinationStateSequence(groupedFavorites, destinationData.id as string, edge) || 0
),
};

handleOnDropFolder(payload);
const sequence = Math.round(
getDestinationStateSequence(groupedFavorites, destinationData.id as string, edge) || 0
);
handleReorder(favorite.id,sequence);
}
}, // canDrag: () => isDraggable,
}),
Expand All @@ -128,10 +130,14 @@ export const FavoriteFolder: React.FC<Props> = (props) => {
element,
allowedEdges: ["top", "bottom"],
}),
onDragEnter: (args) => {
setIsDragging(true);
setIsDraggedOver(true);
args.source.data.is_folder && setClosestEdge(extractClosestEdge(args.self.data));
onDragEnter: ({source,self}) => {
const sourceId = source?.data?.id as string;
const destinationId = self?.data?.id as string | undefined;
if (groupedFavorites[sourceId].parent !== destinationId) {
setIsDraggedOver(true);
setIsDragging(true);
};
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent potential runtime errors due to undefined properties

Accessing groupedFavorites[sourceId] without ensuring it exists may cause runtime errors if sourceId is not a valid key. Please add a check to ensure groupedFavorites[sourceId] is defined before accessing its properties.

Apply this diff to add the necessary check:

  const sourceId = source?.data?.id as string;
  const destinationId = self?.data?.id as string | undefined;
- if (groupedFavorites[sourceId].parent !== destinationId) {
+ if (groupedFavorites[sourceId] && groupedFavorites[sourceId].parent !== destinationId) {
      setIsDraggedOver(true);
      setIsDragging(true);
  };
📝 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.

Suggested change
const sourceId = source?.data?.id as string;
const destinationId = self?.data?.id as string | undefined;
if (groupedFavorites[sourceId].parent !== destinationId) {
setIsDraggedOver(true);
setIsDragging(true);
};
const sourceId = source?.data?.id as string;
const destinationId = self?.data?.id as string | undefined;
if (groupedFavorites[sourceId] && groupedFavorites[sourceId].parent !== destinationId) {
setIsDraggedOver(true);
setIsDragging(true);
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rahulramesha, I reviewed the code again.

In the line:

if (groupedFavorites[sourceId].parent !== destinationId) {

there's a potential risk that groupedFavorites[sourceId] might be undefined if sourceId is not present in groupedFavorites. Accessing groupedFavorites[sourceId].parent without ensuring groupedFavorites[sourceId] exists could lead to a runtime error.

However, if sourceId is guaranteed to be a key in groupedFavorites at this point in the code, then the additional check may not be necessary. Please let me know if that's the case, and we can proceed without the additional check.

Let me know if you have any questions or need further assistance.

if(source.data.is_folder) setClosestEdge(extractClosestEdge(self.data));
},
onDragLeave: () => {
setIsDragging(false);
Expand All @@ -146,16 +152,18 @@ export const FavoriteFolder: React.FC<Props> = (props) => {
setIsDraggedOver(false);
const sourceId = source?.data?.id as string | undefined;
const destinationId = self?.data?.id as string | undefined;

if (source.data.is_folder) return;
if (sourceId === destinationId) return;
if (!sourceId || !destinationId) return;
if (groupedFavorites[sourceId].parent === destinationId) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null check to prevent runtime errors

Similarly, ensure that groupedFavorites[sourceId] is defined before accessing .parent to prevent potential errors.

Apply this diff to add the necessary check:

  if (!sourceId || !destinationId) return;
- if (groupedFavorites[sourceId].parent === destinationId) return;
+ if (groupedFavorites[sourceId] && groupedFavorites[sourceId].parent === destinationId) return;
📝 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.

Suggested change
if (!sourceId || !destinationId) return;
if (groupedFavorites[sourceId].parent === destinationId) return;
if (!sourceId || !destinationId) return;
if (groupedFavorites[sourceId] && groupedFavorites[sourceId].parent === destinationId) return;

handleOnDrop(sourceId, destinationId);

handleMoveToFolder(sourceId, destinationId);
},
})
);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [elementRef.current, isDragging, favorite.id, handleOnDrop]);
}, [elementRef.current, isDragging, favorite.id, handleMoveToFolder]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid including mutable refs in the useEffect dependency array

Including elementRef.current in the dependency array of useEffect is unnecessary and can lead to unexpected behaviors. Mutable refs like elementRef.current do not trigger re-renders when they change, so it's safe to omit them from the dependency array.

Apply this diff to adjust the dependency array:

- }, [elementRef.current, isDragging, favorite.id, handleMoveToFolder]);
+ }, [isDragging, favorite.id, handleMoveToFolder]);
📝 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.

Suggested change
}, [elementRef.current, isDragging, favorite.id, handleMoveToFolder]);
}, [isDragging, favorite.id, handleMoveToFolder]);


useOutsideClickDetector(actionSectionRef, () => setIsMenuActive(false));

Expand Down Expand Up @@ -316,14 +324,15 @@ export const FavoriteFolder: React.FC<Props> = (props) => {
"px-2": !isSidebarCollapsed,
})}
>
{uniqBy(favorite.children, "id").map((child) => (
{orderBy(uniqBy(favorite.children, "id"),'sequence','desc').map((child) => (
<FavoriteRoot
key={child.id}
workspaceSlug={workspaceSlug.toString()}
favorite={child}
handleRemoveFromFavorites={handleRemoveFromFavorites}
handleRemoveFromFavoritesFolder={handleRemoveFromFavoritesFolder}
favoriteMap={groupedFavorites}
handleReorder={handleReorder}
/>
))}
</Disclosure.Panel>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,17 @@
import React, { FC, useEffect, useRef, useState } from "react";
import { combine } from "@atlaskit/pragmatic-drag-and-drop/combine";
import { draggable, dropTargetForElements } from "@atlaskit/pragmatic-drag-and-drop/element/adapter";
import { pointerOutsideOfPreview } from "@atlaskit/pragmatic-drag-and-drop/element/pointer-outside-of-preview";
import { setCustomNativeDragPreview } from "@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview";
import { attachClosestEdge, extractClosestEdge } from "@atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge";
import { observer } from "mobx-react";
// plane helpers
import { createRoot } from "react-dom/client";
import { useOutsideClickDetector } from "@plane/helpers";
// ui
import { IFavorite } from "@plane/types";
// components
import { DropIndicator } from "@plane/ui";
import {
FavoriteItemDragHandle,
FavoriteItemQuickAction,
Expand All @@ -19,66 +24,126 @@ import {
import { useAppTheme } from "@/hooks/store";
import { useFavoriteItemDetails } from "@/hooks/use-favorite-item-details";

//constants
import { getDestinationStateSequence } from "../favorites.helpers";

type Props = {
workspaceSlug: string;
favorite: IFavorite;
favoriteMap: Record<string, IFavorite>;
handleRemoveFromFavorites: (favorite: IFavorite) => void;
handleRemoveFromFavoritesFolder: (favoriteId: string) => void;
handleReorder: (favoriteId: string, sequence: number) => void;
};

export const FavoriteRoot: FC<Props> = observer((props) => {
// props
const { workspaceSlug, favorite, favoriteMap, handleRemoveFromFavorites, handleRemoveFromFavoritesFolder } = props;
const {
workspaceSlug,
favorite,
favoriteMap,
handleRemoveFromFavorites,
handleRemoveFromFavoritesFolder,
handleReorder,
} = props;
// store hooks
const { sidebarCollapsed } = useAppTheme();

const { itemLink, itemIcon, itemTitle } = useFavoriteItemDetails(workspaceSlug, favorite);
//state
const [isDragging, setIsDragging] = useState(false);
const [isMenuActive, setIsMenuActive] = useState(false);
const [closestEdge, setClosestEdge] = useState<string | null>(null);
const [isDraggedOver, setIsDraggedOver] = useState(false);

//ref
const elementRef = useRef<HTMLDivElement>(null);
const actionSectionRef = useRef<HTMLDivElement | null>(null);

const handleQuickAction = (value: boolean) => setIsMenuActive(value);

const { itemLink, itemIcon, itemTitle } = useFavoriteItemDetails(workspaceSlug, favorite);

// drag and drop
useEffect(() => {
const element = elementRef.current;

if (!element) return;
const initialData = { id: favorite.id, type: favorite.parent ? 'CHILD' : 'NON_PARENT' };

return combine(
draggable({
element,
dragHandle: elementRef.current,
canDrag: () => true,
getInitialData: () => ({ id: favorite.id, type: "CHILD" }),
getInitialData: () => initialData,
onDragStart: () => {
setIsDragging(true);
},
onDrop: () => {
onDrop: (data) => {
setIsDraggedOver(false);
setIsDragging(false);
if (!data.location.current.dropTargets[0]) return;
const destinationData = data.location.current.dropTargets[0].data;

if (favorite.id && destinationData) {
const edge = extractClosestEdge(destinationData) || undefined;
const sequence = Math.round(
getDestinationStateSequence(favoriteMap, destinationData.id as string, edge) || 0
);
handleReorder(favorite.id, sequence);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for sequence calculation.

The sequence calculation and reordering logic lacks error handling. Consider adding validation and error handling to prevent potential issues.

 if (favorite.id && destinationData) {
+  try {
     const edge = extractClosestEdge(destinationData) || undefined;
     const sequence = Math.round(
       getDestinationStateSequence(favoriteMap, destinationData.id as string, edge) || 0
     );
     handleReorder(favorite.id, sequence);
+  } catch (error) {
+    console.error('Failed to reorder favorite:', error);
+    // Consider showing a user-friendly error notification
+  }
 }
📝 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.

Suggested change
if (favorite.id && destinationData) {
const edge = extractClosestEdge(destinationData) || undefined;
const sequence = Math.round(
getDestinationStateSequence(favoriteMap, destinationData.id as string, edge) || 0
);
handleReorder(favorite.id, sequence);
}
if (favorite.id && destinationData) {
try {
const edge = extractClosestEdge(destinationData) || undefined;
const sequence = Math.round(
getDestinationStateSequence(favoriteMap, destinationData.id as string, edge) || 0
);
handleReorder(favorite.id, sequence);
} catch (error) {
console.error('Failed to reorder favorite:', error);
// Consider showing a user-friendly error notification
}
}

},
onGenerateDragPreview: ({ nativeSetDragImage }) => {
setCustomNativeDragPreview({
getOffset: pointerOutsideOfPreview({ x: "0px", y: "0px" }),
render: ({ container }) => {
const root = createRoot(container);
root.render(
<div className="rounded bg-custom-background-100 text-sm p-1 pr-2">
<FavoriteItemTitle
href={itemLink}
icon={itemIcon}
title={itemTitle}
isSidebarCollapsed={!!sidebarCollapsed}
/>
</div>
);
return () => root.unmount();
},
nativeSetDragImage,
});
Comment on lines +86 to +104
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add cleanup for drag preview root.

The current implementation of the drag preview could potentially cause memory leaks if the component unmounts during a drag operation. Consider wrapping the root creation in a try-catch block and ensuring cleanup.

 onGenerateDragPreview: ({ nativeSetDragImage }) => {
+  let root: Root | null = null;
   setCustomNativeDragPreview({
     getOffset: pointerOutsideOfPreview({ x: "0px", y: "0px" }),
     render: ({ container }) => {
-      const root = createRoot(container);
+      try {
+        root = createRoot(container);
+        root.render(
-      root.render(
         <div className="rounded bg-custom-background-100 text-sm p-1 pr-2">
           <FavoriteItemTitle
             href={itemLink}
             icon={itemIcon}
             title={itemTitle}
             isSidebarCollapsed={!!sidebarCollapsed}
           />
         </div>
       );
-      return () => root.unmount();
+      return () => {
+        root?.unmount();
+        root = null;
+      };
+      } catch (error) {
+        console.error('Failed to create drag preview:', error);
+        return () => {};
+      }
     },
     nativeSetDragImage,
   });
 },
📝 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.

Suggested change
onGenerateDragPreview: ({ nativeSetDragImage }) => {
setCustomNativeDragPreview({
getOffset: pointerOutsideOfPreview({ x: "0px", y: "0px" }),
render: ({ container }) => {
const root = createRoot(container);
root.render(
<div className="rounded bg-custom-background-100 text-sm p-1 pr-2">
<FavoriteItemTitle
href={itemLink}
icon={itemIcon}
title={itemTitle}
isSidebarCollapsed={!!sidebarCollapsed}
/>
</div>
);
return () => root.unmount();
},
nativeSetDragImage,
});
onGenerateDragPreview: ({ nativeSetDragImage }) => {
let root: Root | null = null;
setCustomNativeDragPreview({
getOffset: pointerOutsideOfPreview({ x: "0px", y: "0px" }),
render: ({ container }) => {
try {
root = createRoot(container);
root.render(
<div className="rounded bg-custom-background-100 text-sm p-1 pr-2">
<FavoriteItemTitle
href={itemLink}
icon={itemIcon}
title={itemTitle}
isSidebarCollapsed={!!sidebarCollapsed}
/>
</div>
);
return () => {
root?.unmount();
root = null;
};
} catch (error) {
console.error('Failed to create drag preview:', error);
return () => {};
}
},
nativeSetDragImage,
});

},
}),
dropTargetForElements({
element,
onDragStart: () => {
setIsDragging(true);
},
onDragEnter: () => {
getData: ({ input, element }) =>
attachClosestEdge(initialData, {
input,
element,
allowedEdges: ["top", "bottom"],
}),
onDragEnter: (args) => {
setIsDragging(true);
setIsDraggedOver(true);
setClosestEdge(extractClosestEdge(args.self.data));
},
onDragLeave: () => {
setIsDragging(false);
setIsDraggedOver(false);
setClosestEdge(null);
},
onDrop: ({ source }) => {
onDrop: ({ self, source }) => {
setIsDragging(false);
const sourceId = source?.data?.id as string | undefined;
if (!sourceId || !favoriteMap[sourceId].parent) return;
handleRemoveFromFavoritesFolder(sourceId);
setIsDraggedOver(false);
const sourceId = source.data?.id as string | undefined;
const destinationType = self.data?.type as string | undefined;

if(!sourceId) return;

if(destinationType === 'NON_PARENT'){
handleRemoveFromFavoritesFolder(sourceId)
}
},
})
);
Expand All @@ -90,6 +155,7 @@ export const FavoriteRoot: FC<Props> = observer((props) => {
return (
<>
<FavoriteItemWrapper elementRef={elementRef} isMenuActive={isMenuActive} sidebarCollapsed={sidebarCollapsed}>
<DropIndicator isVisible={isDraggedOver && closestEdge === "top"} classNames="absolute top-0" />
{!sidebarCollapsed && <FavoriteItemDragHandle isDragging={isDragging} sort_order={favorite.sort_order} />}
<FavoriteItemTitle href={itemLink} icon={itemIcon} title={itemTitle} isSidebarCollapsed={!!sidebarCollapsed} />
{!sidebarCollapsed && (
Expand All @@ -101,6 +167,7 @@ export const FavoriteRoot: FC<Props> = observer((props) => {
handleRemoveFromFavorites={handleRemoveFromFavorites}
/>
)}
<DropIndicator isVisible={isDraggedOver && closestEdge === "bottom"} classNames="absolute bottom-0" />
</FavoriteItemWrapper>
</>
);
Expand Down
Loading
Loading