[WEB-2774] Chore: re-ordering functionality for entities in favorites. #6078
Conversation
mathalav55
requested review from
SatishGandham,
sriramveeraghanta,
gurusainath and
rahulramesha
|
|
WalkthroughThe changes in this pull request enhance the drag-and-drop functionality across several components related to favorites management. Key modifications include the introduction of a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 7
🧹 Outside diff range and nitpick comments (8)
web/core/components/workspace/sidebar/favorites/favorites-menu.tsx (1)
Line range hint
1-238: Consider adding documentation for reordering behaviorThe component would benefit from:
- Documentation explaining the reordering behavior and sequence calculation logic
- JSDoc comments for the handleReorder method describing its parameters and behavior
- Comments explaining the interaction between drag-and-drop and reordering functionality
This would improve maintainability and make it easier for other developers to understand the implementation.
web/core/store/favorite.store.ts (2)
Line range hint
171-192: Remove unnecessary return statement in error handlingThe error handling logic contains an unnecessary return statement that can be removed for better code clarity.
runInAction(() => { if (!data.parent) return; - // revert the parent set(this.favoriteMap, [favoriteId, "parent"], oldParent); });
Line range hint
193-209: Move throw statement outside runInActionThe throw statement inside runInAction could interfere with MobX state tracking. Consider moving it outside:
} catch (error) { console.error("Failed to move favorite folder"); runInAction(() => { set(this.favoriteMap, [favoriteId, "sequence"], initialSequence); - throw error; }); + throw error; }web/core/components/workspace/sidebar/favorites/favorite-items/root.tsx (3)
6-8: Reorder imports to follow proper import orderingThe imports at lines 7 and 8 should be placed before the import at line 6 to comply with the project's import ordering conventions indicated by the linter.
Apply this diff to reorder the imports:
-import { attachClosestEdge, extractClosestEdge } from "@atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge"; +import { setCustomNativeDragPreview } from "@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview"; +import { pointerOutsideOfPreview } from "@atlaskit/pragmatic-drag-and-drop/element/pointer-outside-of-preview"; +import { attachClosestEdge, extractClosestEdge } from "@atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge";🧰 Tools
🪛 GitHub Check: lint-web
[failure] 7-7:
@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-previewimport should occur before import of@atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge
[failure] 8-8:
@atlaskit/pragmatic-drag-and-drop/element/pointer-outside-of-previewimport should occur before import of@atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge
25-26: Adjust import order to meet coding standardsThe import from
"react-dom/client"at line 25 should come before the import from"@plane/helpers", and the import from"@plane/ui"at line 26 should be placed before imports from"@/components"to adhere to the project's import ordering rules.Apply this diff to reorder the imports:
-import { useOutsideClickDetector } from "@plane/helpers"; +import { createRoot } from "react-dom/client"; +import { useOutsideClickDetector } from "@plane/helpers"; +import { DropIndicator } from "@plane/ui"; import { observer } from "mobx-react"; // plane helpers -import { createRoot } from "react-dom/client"; // components -import { DropIndicator } from "@plane/ui"; import { FavoriteItemDragHandle, FavoriteItemQuickAction,🧰 Tools
🪛 GitHub Check: lint-web
[failure] 25-25:
react-dom/clientimport should occur before import of@plane/helpers
[failure] 26-26:
@plane/uiimport should occur before import of@/components/workspace/sidebar/favorites
142-142: Remove trailing spacesThere's a trailing space at line 142. Removing it will keep the code clean and conform to coding standards.
Apply this diff:
- const sourceType = source.data?.type as string | undefined; - + const sourceType = source.data?.type as string | undefined;🧰 Tools
🪛 GitHub Check: lint-web
[failure] 142-142:
Trailing spaces not allowedweb/core/components/workspace/sidebar/favorites/favorite-folder.tsx (2)
92-92: Remove commented-out codeThe line
// onDragStart: () => setIsDragging(true),is commented out. If it's no longer needed, consider removing it to keep the codebase clean.
155-155: Remove trailing whitespaceLine 155 contains trailing spaces, which should be removed to comply with coding style guidelines.
Apply this diff to remove trailing spaces:
- +🧰 Tools
🪛 GitHub Check: lint-web
[failure] 155-155:
Trailing spaces not allowed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx(7 hunks)web/core/components/workspace/sidebar/favorites/favorite-items/root.tsx(4 hunks)web/core/components/workspace/sidebar/favorites/favorites-menu.tsx(4 hunks)web/core/store/favorite.store.ts(4 hunks)
🧰 Additional context used
🪛 GitHub Check: lint-web
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx
[failure] 7-7:
@atlaskit/pragmatic-drag-and-drop/element/pointer-outside-of-preview import should occur before import of @atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview
[failure] 14-14:
react-dom/client import should occur before import of lucide-react
[failure] 31-31:
lodash import should occur before import of lodash/uniqBy
[failure] 155-155:
Trailing spaces not allowed
web/core/components/workspace/sidebar/favorites/favorite-items/root.tsx
[failure] 7-7:
@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview import should occur before import of @atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge
[failure] 8-8:
@atlaskit/pragmatic-drag-and-drop/element/pointer-outside-of-preview import should occur before import of @atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge
[failure] 25-25:
react-dom/client import should occur before import of @plane/helpers
[failure] 26-26:
@plane/ui import should occur before import of @/components/workspace/sidebar/favorites
[failure] 142-142:
Trailing spaces not allowed
🔇 Additional comments (5)
web/core/components/workspace/sidebar/favorites/favorites-menu.tsx (2)
36-36: LGTM: Hook usage is correct
The addition of reOrderFavorite to the useFavorite hook destructuring is consistent with the reordering functionality requirements.
217-217: LGTM: Props passing is implemented correctly
The handleReorder prop is consistently passed to both FavoriteFolder and FavoriteRoot components, enabling reordering functionality throughout the favorites hierarchy.
Also applies to: 226-226
web/core/store/favorite.store.ts (3)
29-31: LGTM! Method names now better reflect their functionality
The renaming of methods from moveFavorite to moveFavoriteToFolder and moveFavoriteFolder to reOrderFavorite improves code clarity by making the intended functionality more explicit.
67-69: LGTM! MobX action decorators properly updated
The action decorators have been correctly updated to match the renamed methods, maintaining proper MobX state management.
Line range hint 171-209: Consider adding unit tests for reordering edge cases
The reordering functionality should be thoroughly tested, particularly for:
- Concurrent reordering operations
- Edge cases with sequence numbers
- Error recovery scenarios
- State rollback accuracy
Would you like help creating comprehensive unit tests for these scenarios?
web/core/components/workspace/sidebar/favorites/favorites-menu.tsx
Outdated
Show resolved
Hide resolved
| }, | ||
| }) | ||
| ); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [elementRef.current, isDragging, favorite.id, handleOnDrop]); | ||
| }, [elementRef.current, isDragging, favorite.id, handleMoveToFolder]); |
There was a problem hiding this comment.
🛠️ 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.
| }, [elementRef.current, isDragging, favorite.id, handleMoveToFolder]); | |
| }, [isDragging, favorite.id, handleMoveToFolder]); |
| const sourceId = source?.data?.id as string; | ||
| const destinationId = self?.data?.id as string | undefined; | ||
| if (groupedFavorites[sourceId].parent !== destinationId) { | ||
| setIsDraggedOver(true); | ||
| setIsDragging(true); | ||
| }; |
There was a problem hiding this comment.
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.
| 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); | |
| }; |
There was a problem hiding this comment.
@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.
| import { setCustomNativeDragPreview } from "@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview"; | ||
| import { pointerOutsideOfPreview } from "@atlaskit/pragmatic-drag-and-drop/element/pointer-outside-of-preview"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct the import order according to coding standards
The static analysis tool indicates that pointerOutsideOfPreview import should occur before setCustomNativeDragPreview. Please reorder the imports to comply with project conventions.
Apply this diff to fix the import order:
- import { setCustomNativeDragPreview } from "@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview";
- import { pointerOutsideOfPreview } from "@atlaskit/pragmatic-drag-and-drop/element/pointer-outside-of-preview";
+ 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";📝 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.
| import { setCustomNativeDragPreview } from "@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview"; | |
| import { pointerOutsideOfPreview } from "@atlaskit/pragmatic-drag-and-drop/element/pointer-outside-of-preview"; | |
| 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"; |
🧰 Tools
🪛 GitHub Check: lint-web
[failure] 7-7:
@atlaskit/pragmatic-drag-and-drop/element/pointer-outside-of-preview import should occur before import of @atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview
| if (!sourceId || !destinationId) return; | ||
| if (groupedFavorites[sourceId].parent === destinationId) return; |
There was a problem hiding this comment.
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.
| if (!sourceId || !destinationId) return; | |
| if (groupedFavorites[sourceId].parent === destinationId) return; | |
| if (!sourceId || !destinationId) return; | |
| if (groupedFavorites[sourceId] && groupedFavorites[sourceId].parent === destinationId) return; |
| @@ -24,21 +28,23 @@ | |||
| import { FavoriteRoot } from "./favorite-items"; | |||
| import { getDestinationStateSequence } from "./favorites.helpers"; | |||
| import { NewFavoriteFolder } from "./new-fav-folder"; | |||
| import { orderBy } from "lodash"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reorder imports to comply with coding standards
The static analysis tool suggests that the import of lodash should occur before the import of lodash/uniqBy. Please reorder the imports to align with project conventions.
Apply this diff to fix the import order:
- import uniqBy from "lodash/uniqBy";
import { useParams } from "next/navigation";
import { PenSquare, Star, MoreHorizontal, ChevronRight, GripVertical } from "lucide-react";
import { Disclosure, Transition } from "@headlessui/react";
+ import { orderBy } from "lodash";
+ import uniqBy from "lodash/uniqBy";Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: lint-web
[failure] 31-31:
lodash import should occur before import of lodash/uniqBy
|
|
||
| import { attachClosestEdge, extractClosestEdge } from "@atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge"; | ||
| import uniqBy from "lodash/uniqBy"; | ||
| import { useParams } from "next/navigation"; | ||
| import { PenSquare, Star, MoreHorizontal, ChevronRight, GripVertical } from "lucide-react"; | ||
| import { Disclosure, Transition } from "@headlessui/react"; | ||
| import { createRoot } from "react-dom/client"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust the import order to follow project conventions
According to the static analysis tool, the import of react-dom/client should occur before the import of lucide-react. Please adjust the import statements accordingly.
Apply this diff to fix the import order:
- import { PenSquare, Star, MoreHorizontal, ChevronRight, GripVertical } from "lucide-react";
import { Disclosure, Transition } from "@headlessui/react";
+ import { createRoot } from "react-dom/client";
+ import { PenSquare, Star, MoreHorizontal, ChevronRight, GripVertical } from "lucide-react";Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: lint-web
[failure] 14-14:
react-dom/client import should occur before import of lucide-react
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
web/core/components/workspace/sidebar/favorites/favorite-items/root.tsx (2)
136-146: Simplify drop handler logic.The drop handler contains nested conditionals that could be simplified for better readability and maintainability.
-onDrop: ({ self, source }) => { - setIsDragging(false); - 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) - } -}, +onDrop: ({ self, source }) => { + setIsDragging(false); + setIsDraggedOver(false); + + const sourceId = source.data?.id; + const isNonParentDestination = self.data?.type === 'NON_PARENT'; + + if (sourceId && isNonParentDestination) { + handleRemoveFromFavoritesFolder(sourceId); + } +},
158-158: Consider consolidating DropIndicator implementation.The duplicate DropIndicator components could be consolidated into a single reusable component or mapping to improve maintainability.
-<DropIndicator isVisible={isDraggedOver && closestEdge === "top"} classNames="absolute top-0" /> // ... other content ... -<DropIndicator isVisible={isDraggedOver && closestEdge === "bottom"} classNames="absolute bottom-0" /> +{['top', 'bottom'].map((edge) => ( + <DropIndicator + key={edge} + isVisible={isDraggedOver && closestEdge === edge} + classNames={`absolute ${edge}-0`} + /> +))}Also applies to: 170-170
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx (3)
93-109: Consider memoizing the drag preview render functionThe drag preview render function is recreated on every render. Consider memoizing it for better performance.
+ const renderDragPreview = React.useCallback(({ 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(); + }, [favorite.name]); 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(); - }, + render: renderDragPreview, nativeSetDragImage, }); },
118-121: Extract sequence calculation logicConsider extracting the sequence calculation logic into a separate helper function for better maintainability and reusability.
+ const calculateNewSequence = ( + groupedFavorites: Record<string, any>, + destinationId: string, + edge?: string + ): number => { + return Math.round( + getDestinationStateSequence(groupedFavorites, destinationId, edge) || 0 + ); + }; - const sequence = Math.round( - getDestinationStateSequence(groupedFavorites, destinationData.id as string, edge) || 0 - ); + const sequence = calculateNewSequence( + groupedFavorites, + destinationData.id as string, + edge + );
327-327: Add type safety to orderBy parametersConsider adding type safety to the
orderByparameters to prevent potential runtime errors.- orderBy(uniqBy(favorite.children, "id"),'sequence','desc') + orderBy( + uniqBy(favorite.children, "id"), + ['sequence'] as const, + ['desc'] as const + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx(7 hunks)web/core/components/workspace/sidebar/favorites/favorite-items/root.tsx(4 hunks)web/core/components/workspace/sidebar/favorites/favorites-menu.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/workspace/sidebar/favorites/favorites-menu.tsx
🔇 Additional comments (4)
web/core/components/workspace/sidebar/favorites/favorite-items/root.tsx (2)
6-8: LGTM: Imports and type definitions are well-structured.
The new imports and type definitions appropriately support the drag-and-drop functionality enhancements.
Also applies to: 11-11, 16-16, 27-29, 36-36
41-48: LGTM: Props destructuring is clean and consistent.
The props destructuring properly includes all required properties and maintains consistency with the type definition.
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx (2)
38-38: LGTM: Props type definition is well-structured
The new handleReorder prop is properly typed with clear parameter definitions.
42-42: LGTM: Clean props destructuring and hook usage
Props destructuring and hook usage follow React best practices.
Also applies to: 47-47
| 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); | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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, | ||
| }); |
There was a problem hiding this comment.
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.
| 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, | |
| }); |
| @@ -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); | |||
There was a problem hiding this comment.
🛠️ 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.
| @@ -83,6 +83,26 @@ export const SidebarFavoritesMenu = observer(() => { | |||
| }); | |||
| }); | |||
| }; | |||
|
|
|||
| const handleReorder = (favoriteId: string, sequence: number) => { | |||
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx
Outdated
Show resolved
Hide resolved
| const sourceId = source?.data?.id as string; | ||
| const destinationId = self?.data?.id as string | undefined; | ||
| if (groupedFavorites[sourceId].parent !== destinationId) { | ||
| setIsDraggedOver(true); | ||
| setIsDragging(true); | ||
| }; |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
web/core/components/workspace/sidebar/favorites/favorites.helpers.ts (3)
49-50: Remove unnecessaryconsole.logstatementThe
console.log({resultSequence});statement appears to be a leftover from debugging and should be removed to clean up the code.Apply this diff to remove the statement:
- console.log({resultSequence});
85-86: Correct typo in commentThere's a typo in the comment. "A group cannon be a child..." should be "A group cannot be a child...".
Apply this diff to correct the typo:
- // if source that is being dragged is a group. A group cannon be a child of any other favorite, + // If the source that is being dragged is a group, a group cannot be a child of any other favorite,
104-105: Fix typo in commentThe comment contains a typo: "dropped unto itself" should be "dropped onto itself".
Apply this diff to fix the typo:
- // a favorite cannot be dropped unto itself + // a favorite cannot be dropped onto itselfweb/core/components/workspace/sidebar/favorites/favorite-items/root.tsx (1)
73-162: Consider performance optimizationsThe drag-and-drop implementation could benefit from the following optimizations:
- Memoize complex calculations
- Optimize effect dependencies
- Use callbacks for event handlers
Consider applying these optimizations:
+ const memoizedInitialData = useMemo( + () => ({ id: favorite.id, isGroup: false, isChild: !!parentId, parentId }), + [favorite.id, parentId] + ); + const handleDrop = useCallback(({ source, location }) => { + // ... existing drop logic + }, [favoriteMap, handleReorder, handleRemoveFromFavoritesFolder]); useEffect(() => { const element = elementRef.current; if (!element) return; - const initialData = { id: favorite.id, isGroup: false, isChild: !!parentId, parentId }; return combine( draggable({ element, dragHandle: elementRef.current, - getInitialData: () => initialData, + getInitialData: () => memoizedInitialData, // ... rest of the code }), dropTargetForElements({ // ... other props - onDrop: ({ source, location }) => { ... }, + onDrop: handleDrop, }) ); - // eslint-disable-next-line react-hooks/exhaustive-deps + // explicit dependencies + }, [elementRef.current, isDragging, memoizedInitialData, handleDrop]);web/core/components/workspace/sidebar/projects-list-item.tsx (2)
Line range hint
39-48: Consider enhancing type safety for project-related propsThe
projectListTypeprop could benefit from using a TypeScript enum or union type constant instead of string literals for better type safety and maintainability.+type ProjectListType = "JOINED" | "FAVORITES"; + type Props = { projectId: string; handleCopyText: () => void; handleOnProjectDrop?: ( sourceId: string | undefined, destinationId: string | undefined, shouldDropAtEnd: boolean ) => void; - projectListType: "JOINED" | "FAVORITES"; + projectListType: ProjectListType; disableDrag?: boolean; disableDrop?: boolean; isLastChild: boolean; };
Line range hint
77-81: Optimize state management to reduce re-rendersConsider combining related state variables into a single state object to reduce the number of state updates and potential re-renders.
- const [leaveProjectModalOpen, setLeaveProjectModal] = useState(false); - const [publishModalOpen, setPublishModal] = useState(false); - const [isMenuActive, setIsMenuActive] = useState(false); - const [isDragging, setIsDragging] = useState(false); - const [isProjectListOpen, setIsProjectListOpen] = useState(false); + const [uiState, setUiState] = useState({ + leaveProjectModalOpen: false, + publishModalOpen: false, + isMenuActive: false, + isDragging: false, + isProjectListOpen: false + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx(5 hunks)web/core/components/workspace/sidebar/favorites/favorite-items/root.tsx(4 hunks)web/core/components/workspace/sidebar/favorites/favorites-menu.tsx(7 hunks)web/core/components/workspace/sidebar/favorites/favorites.helpers.ts(3 hunks)web/core/components/workspace/sidebar/projects-list-item.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/core/components/workspace/sidebar/favorites/favorite-folder.tsx
- web/core/components/workspace/sidebar/favorites/favorites-menu.tsx
🔇 Additional comments (5)
web/core/components/workspace/sidebar/favorites/favorites.helpers.ts (1)
51-51: Double-check the necessity of rounding resultSequence
Confirm whether rounding resultSequence is essential. Depending on how sequence values are used elsewhere, rounding might introduce unintended ordering issues.
Would you like to verify if floating-point sequence values cause any problems in the application?
web/core/components/workspace/sidebar/favorites/favorite-items/root.tsx (2)
6-9: LGTM: Clean imports and well-defined types
The new imports and type definitions are well-organized and properly typed for the drag-and-drop functionality.
Also applies to: 15-17, 32-33, 39-39
170-170: LGTM: Clean implementation of drop indicators
The drop indicators are well-positioned and provide clear visual feedback during drag operations. The conditional rendering based on instruction state is implemented correctly.
Also applies to: 184-184
web/core/components/workspace/sidebar/projects-list-item.tsx (2)
Line range hint 1-607: Implementation successfully achieves re-ordering functionality
The changes effectively implement the project re-ordering functionality with proper drag-and-drop handling, state management, and UI feedback. The code is well-structured and maintains good separation of concerns.
301-307: Verify ref assignment and cleanup
The ref prop has been moved from the Disclosure component to the outer div. While this change supports the drag-and-drop functionality, ensure that the ref is properly cleaned up when the component unmounts.
✅ Verification successful
Let me search for the ref cleanup patterns and drag-drop implementation more broadly.
Let me search for the drag-drop implementation and ref usage in the specific file.
Based on the code analysis, I can now provide a final response about the ref cleanup concern:
Ref cleanup is properly handled through drag-drop cleanup
The projectRef is used within the drag-drop setup which is handled by the combine function from Atlaskit's pragmatic-drag-and-drop library. The cleanup is automatically managed through:
- The ref is properly scoped within component lifecycle
- The drag-drop cleanup is handled by the library's internal cleanup mechanism
- The ref is only used for DOM element reference and event delegation, not for storing mutable values that need explicit cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other instances of projectRef cleanup in the codebase
rg -A 5 "projectRef.current = null"
Length of output: 37
Script:
#!/bin/bash
# Search for useEffect cleanup patterns in the file
rg -A 10 "useEffect.*return" "web/core/components/workspace/sidebar/projects-list-item.tsx"
# Search for drag-drop related code to understand the ref usage
ast-grep --pattern 'useEffect(() => {
$$$
return () => {
$$$
}
}, $$$)'
Length of output: 63369
Script:
#!/bin/bash
# Search for drag-drop related code and ref usage in the file
rg -A 10 "projectRef|useEffect.*drag" "web/core/components/workspace/sidebar/projects-list-item.tsx"
# Search for the ref declaration
ast-grep --pattern 'const $ref = useRef'
Length of output: 2056
| const sourceData = source?.data; | ||
|
|
There was a problem hiding this comment.
Ensure consistent typing of sourceData
In the getCanDrop function, sourceData is not explicitly typed as TargetData. To ensure type safety and avoid runtime errors, cast sourceData to TargetData.
Apply this diff to cast sourceData:
- const sourceData = source?.data;
+ const sourceData = source?.data as TargetData;This ensures that properties like id and isGroup are properly recognized by TypeScript.
📝 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 sourceData = source?.data; | |
| const sourceData = source?.data as TargetData; | |
| onDrop: ({ source, location }) => { | ||
| setInstruction(undefined); | ||
| const dropTargets = location?.current?.dropTargets ?? [] | ||
| if(!dropTargets || dropTargets.length <= 0) return; | ||
|
|
||
| const dropTarget = dropTargets.length > 1 ? dropTargets.find(target=>target?.data?.isChild) : dropTargets[0]; | ||
|
|
||
| const dropTargetData = dropTarget?.data as TargetData; | ||
|
|
||
| if(!dropTarget || !dropTargetData) return; | ||
|
|
||
| const instruction = getInstructionFromPayload(dropTarget, source, location); | ||
| const parentId = instruction === 'make-child' ? dropTargetData.id : dropTargetData.parentId; | ||
| const droppedFavId = instruction !== "make-child" ? dropTargetData.id : undefined; | ||
| const sourceData = source.data as TargetData; | ||
|
|
||
| if(droppedFavId && sourceData.id){ | ||
| const destinationSequence = getDestinationStateSequence(favoriteMap,droppedFavId,instruction) | ||
| handleReorder(sourceData.id,destinationSequence || 0) | ||
| } | ||
|
|
||
| if(!parentId && sourceData.isChild){ | ||
| handleRemoveFromFavoritesFolder(sourceData.id) | ||
| } |
There was a problem hiding this comment.
Add error boundaries and validation for drop operations
The drop handling logic is complex and should be protected with proper error handling and validation:
- Validate sequence calculation results
- Add error boundaries for the drag-and-drop operations
- Handle edge cases when dropTarget data is malformed
Apply this diff to improve error handling:
onDrop: ({ source, location }) => {
setInstruction(undefined);
+ try {
const dropTargets = location?.current?.dropTargets ?? []
- if(!dropTargets || dropTargets.length <= 0) return;
+ if(!dropTargets || dropTargets.length <= 0) {
+ console.warn('No valid drop targets found');
+ return;
+ }
const dropTarget = dropTargets.length > 1 ? dropTargets.find(target=>target?.data?.isChild) : dropTargets[0];
const dropTargetData = dropTarget?.data as TargetData;
- if(!dropTarget || !dropTargetData) return;
+ if(!dropTarget || !dropTargetData) {
+ console.warn('Invalid drop target data');
+ return;
+ }
const instruction = getInstructionFromPayload(dropTarget, source, location);
const parentId = instruction === 'make-child' ? dropTargetData.id : dropTargetData.parentId;
const droppedFavId = instruction !== "make-child" ? dropTargetData.id : undefined;
const sourceData = source.data as TargetData;
if(droppedFavId && sourceData.id){
const destinationSequence = getDestinationStateSequence(favoriteMap,droppedFavId,instruction)
+ if (typeof destinationSequence !== 'number') {
+ console.error('Invalid sequence calculation');
+ return;
+ }
handleReorder(sourceData.id,destinationSequence || 0)
}
if(!parentId && sourceData.isChild){
handleRemoveFromFavoritesFolder(sourceData.id)
}
+ } catch (error) {
+ console.error('Error during drop operation:', 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.
| onDrop: ({ source, location }) => { | |
| setInstruction(undefined); | |
| const dropTargets = location?.current?.dropTargets ?? [] | |
| if(!dropTargets || dropTargets.length <= 0) return; | |
| const dropTarget = dropTargets.length > 1 ? dropTargets.find(target=>target?.data?.isChild) : dropTargets[0]; | |
| const dropTargetData = dropTarget?.data as TargetData; | |
| if(!dropTarget || !dropTargetData) return; | |
| const instruction = getInstructionFromPayload(dropTarget, source, location); | |
| const parentId = instruction === 'make-child' ? dropTargetData.id : dropTargetData.parentId; | |
| const droppedFavId = instruction !== "make-child" ? dropTargetData.id : undefined; | |
| const sourceData = source.data as TargetData; | |
| if(droppedFavId && sourceData.id){ | |
| const destinationSequence = getDestinationStateSequence(favoriteMap,droppedFavId,instruction) | |
| handleReorder(sourceData.id,destinationSequence || 0) | |
| } | |
| if(!parentId && sourceData.isChild){ | |
| handleRemoveFromFavoritesFolder(sourceData.id) | |
| } | |
| onDrop: ({ source, location }) => { | |
| setInstruction(undefined); | |
| try { | |
| const dropTargets = location?.current?.dropTargets ?? [] | |
| if(!dropTargets || dropTargets.length <= 0) { | |
| console.warn('No valid drop targets found'); | |
| return; | |
| } | |
| const dropTarget = dropTargets.length > 1 ? dropTargets.find(target=>target?.data?.isChild) : dropTargets[0]; | |
| const dropTargetData = dropTarget?.data as TargetData; | |
| if(!dropTarget || !dropTargetData) { | |
| console.warn('Invalid drop target data'); | |
| return; | |
| } | |
| const instruction = getInstructionFromPayload(dropTarget, source, location); | |
| const parentId = instruction === 'make-child' ? dropTargetData.id : dropTargetData.parentId; | |
| const droppedFavId = instruction !== "make-child" ? dropTargetData.id : undefined; | |
| const sourceData = source.data as TargetData; | |
| if(droppedFavId && sourceData.id){ | |
| const destinationSequence = getDestinationStateSequence(favoriteMap,droppedFavId,instruction) | |
| if (typeof destinationSequence !== 'number') { | |
| console.error('Invalid sequence calculation'); | |
| return; | |
| } | |
| handleReorder(sourceData.id,destinationSequence || 0) | |
| } | |
| if(!parentId && sourceData.isChild){ | |
| handleRemoveFromFavoritesFolder(sourceData.id) | |
| } | |
| } catch (error) { | |
| console.error('Error during drop operation:', error); | |
| // Consider showing a user-friendly error notification | |
| } |
| <Disclosure key={`${project.id}_${URLProjectId}`} defaultOpen={isProjectListOpen} as="div"> | ||
| <div | ||
| id={`sidebar-${projectId}-${projectListType}`} | ||
| className={cn("relative", { | ||
| "bg-custom-sidebar-background-80 opacity-60": isDragging, | ||
| })} | ||
| ref={projectRef} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for drag-and-drop interactions
The drag-and-drop functionality should include ARIA attributes to improve accessibility. Consider adding appropriate aria-labels and roles.
<Disclosure key={`${project.id}_${URLProjectId}`} defaultOpen={isProjectListOpen} as="div">
<div
id={`sidebar-${projectId}-${projectListType}`}
className={cn("relative", {
"bg-custom-sidebar-background-80 opacity-60": isDragging,
})}
+ role="listitem"
+ aria-label={`Project ${project.name}`}
+ aria-grabbed={isDragging}
ref={projectRef}
>📝 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.
| <Disclosure key={`${project.id}_${URLProjectId}`} defaultOpen={isProjectListOpen} as="div"> | |
| <div | |
| id={`sidebar-${projectId}-${projectListType}`} | |
| className={cn("relative", { | |
| "bg-custom-sidebar-background-80 opacity-60": isDragging, | |
| })} | |
| ref={projectRef} | |
| <Disclosure key={`${project.id}_${URLProjectId}`} defaultOpen={isProjectListOpen} as="div"> | |
| <div | |
| id={`sidebar-${projectId}-${projectListType}`} | |
| className={cn("relative", { | |
| "bg-custom-sidebar-background-80 opacity-60": isDragging, | |
| })} | |
| role="listitem" | |
| aria-label={`Project ${project.name}`} | |
| aria-grabbed={isDragging} | |
| ref={projectRef} |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx (2)
92-109: Consider extracting drag preview into a separate componentThe drag preview rendering logic could be moved to a separate component to improve maintainability and reusability.
+const FavoriteDragPreview: React.FC<{ name: string }> = ({ name }) => ( + <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">{name}</p> + </div> +); 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> - ); + root.render(<FavoriteDragPreview name={favorite.name} />); return () => root.unmount(); }, nativeSetDragImage, }); },
172-186: Remove commented-out codeThe commented-out code should be removed as it's no longer needed and can cause confusion.
- // if(parentId) - // handleMoveToFolder(sourceData.id,parentId); - // else - // handleRemoveFromFavoritesFolder(sourceData.id) - - // setIsDragging(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; - - // handleMoveToFolder(sourceId, destinationId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx(5 hunks)web/core/components/workspace/sidebar/favorites/favorites-menu.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/workspace/sidebar/favorites/favorites-menu.tsx
🔇 Additional comments (2)
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx (2)
32-38: LGTM! Props interface updated correctly
The addition of handleReorder prop and its proper destructuring aligns well with the reordering functionality requirements.
Also applies to: 41-41
211-211: LGTM! Rendering logic properly implements reordering functionality
The changes correctly implement:
- Instruction-based styling for drag and drop visual feedback
- Proper ordering of children using
orderBy - Correct prop passing for reordering functionality
Also applies to: 354-354, 359-360, 364-364
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx
Outdated
Show resolved
Hide resolved
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const dropTargetData = dropTarget?.data as TargetData; | ||
|
|
||
| if(!dropTarget || !dropTargetData) return; |
There was a problem hiding this comment.
This logic looks complex and similar to the favorite folder, try to separate it out to a common folder
web/core/components/workspace/sidebar/favorites/favorites-menu.tsx
Outdated
Show resolved
Hide resolved
web/core/components/workspace/sidebar/favorites/favorites-menu.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx (2)
133-162: Consider breaking down the complex drop handlerThe drop handler contains multiple responsibilities and nested conditions. Consider extracting the logic into smaller, focused functions for better maintainability.
Example refactor:
+const handleDropReorder = (sourceData: TargetData, droppedFavId: string, instruction: InstructionType) => { + if(instruction === 'make-child') return; + const destinationSequence = getDestinationStateSequence(groupedFavorites, droppedFavId, instruction); + handleReorder(sourceData.id, destinationSequence || 0); +}; +const handleDropParentChange = (sourceData: TargetData, parentId: string) => { + if(parentId !== sourceData.parentId) { + handleMoveToFolder(sourceData.id, parentId); + } +}; onDrop: ({ source, location }) => { setInstruction(undefined); const dropTargets = location?.current?.dropTargets ?? []; if(!dropTargets || dropTargets.length <= 0) return; const dropTarget = dropTargets.length > 1 ? dropTargets.find(target => target?.data?.isChild) : dropTargets[0]; const dropTargetData = dropTarget?.data as TargetData; if(!dropTarget || !dropTargetData) return; const instruction = getInstructionFromPayload(dropTarget, source, location); const parentId = instruction === 'make-child' ? dropTargetData.id : dropTargetData.parentId; const droppedFavId = instruction !== "make-child" ? dropTargetData.id : undefined; const sourceData = source.data as TargetData; if(!sourceData.id) return; + if(parentId) { + handleDropParentChange(sourceData, parentId); + } else if(sourceData.isChild) { + handleRemoveFromFavoritesFolder(sourceData.id); + } + if(droppedFavId) { + handleDropReorder(sourceData, droppedFavId, instruction); + } - if(parentId){ - if(parentId !== sourceData.parentId){ - handleMoveToFolder(sourceData.id,parentId) - } - } else { - if(sourceData.isChild){ - handleRemoveFromFavoritesFolder(sourceData.id) - } - } - if(droppedFavId){ - if(instruction === 'make-child') return; - const destinationSequence = getDestinationStateSequence(groupedFavorites,droppedFavId,instruction) - handleReorder(sourceData.id,destinationSequence || 0) - } },
330-340: Consider memoizing sorted childrenThe
orderByoperation on children array could be memoized to prevent unnecessary re-sorting on each render.Apply this diff:
+const sortedChildren = useMemo( + () => orderBy(favorite.children, 'sequence', 'desc'), + [favorite.children] +); -{orderBy(favorite.children,'sequence','desc').map((child,index) => ( +{sortedChildren.map((child, index) => (Don't forget to add the useMemo import:
-import { useEffect, useRef, useState } from "react"; +import { useEffect, useRef, useState, useMemo } from "react";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx(5 hunks)web/core/components/workspace/sidebar/favorites/favorites-menu.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/workspace/sidebar/favorites/favorites-menu.tsx
🔇 Additional comments (2)
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx (2)
6-8: LGTM: Import statements and Props type updates
The new imports enhance drag-and-drop functionality, and the Props type has been correctly extended with the handleReorder method.
Also applies to: 37-37
85-102: LGTM: Well-implemented custom drag preview
The drag preview implementation using createRoot provides a clean and consistent visual experience.
| const { workspaceSlug } = useParams(); | ||
| // states | ||
| const [isMenuActive, setIsMenuActive] = useState(false); | ||
| const [isDragging, setIsDragging] = useState(false); | ||
| const [folderToRename, setFolderToRename] = useState<string | boolean | null>(null); | ||
| const [isDraggedOver, setIsDraggedOver] = useState(false); | ||
| const [closestEdge, setClosestEdge] = useState<string | null>(null); | ||
| const [instruction, setInstruction] = useState<InstructionType | undefined>(undefined); |
There was a problem hiding this comment.
Add error handling to getGroupedFavorites call
The fetch operation should include error handling to manage potential failures gracefully.
Apply this diff:
-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.",
+ });
+ });
+}Also applies to: 58-58
Summary:
Added functionality to re-order entities under Favorites section.
Project highlight fix. Only highlighting the last project dropped.
Reference:
[WEB-2774]
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor