Skip to content

[WEB-2774]fix:reordering favorites and favorite folders #6119

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.

Sign up for GitHub

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

Merged
pushya22 merged 18 commits into from
Dec 2, 2024
Merged

[WEB-2774]fix:reordering favorites and favorite folders #6119

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
0f641ec
fixed re order for favorites
vamsikrishnamathala Nov 20, 2024
12743b5
fixed lint errors
vamsikrishnamathala Nov 20, 2024
0a6cb1b
Merge branch 'preview' of github.com:makeplane/plane into chore-favor…
vamsikrishnamathala Nov 20, 2024
23b30d0
added reorder
vamsikrishnamathala Nov 22, 2024
0bec7a1
Merge branch 'preview' of github.com:makeplane/plane into chore-favor…
vamsikrishnamathala Nov 22, 2024
a612ec7
fixed reorder inside folder
vamsikrishnamathala Nov 22, 2024
b2a58a3
fixed lint issues
vamsikrishnamathala Nov 22, 2024
1b124c6
memoized reorder
vamsikrishnamathala Nov 22, 2024
d1cc0e0
removed unnecessary comments
vamsikrishnamathala Nov 25, 2024
9535a4a
seprated duplicate logic to a common file
vamsikrishnamathala Nov 26, 2024
797a756
removed code comments
vamsikrishnamathala Nov 26, 2024
9b494bf
Merge branch 'preview' of github.com:makeplane/plane into chore-favor…
vamsikrishnamathala Nov 28, 2024
997ea5b
fixed favorite remove while reorder inside folder
vamsikrishnamathala Nov 28, 2024
073cc06
fixed folder remove while reorder inside folder
vamsikrishnamathala Nov 29, 2024
65248c7
fixed-reorder issue
vamsikrishnamathala Nov 29, 2024
6501cdd
added last child to drop handled
vamsikrishnamathala Nov 29, 2024
e1e7865
fixed orderby function
vamsikrishnamathala Nov 29, 2024
9541f5b
removed unncessasary comments
vamsikrishnamathala Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion web/core/components/workspace/sidebar/favorites/favorite-folder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ type Props = {
favorite: IFavorite;
handleRemoveFromFavorites: (favorite: IFavorite) => void;
handleRemoveFromFavoritesFolder: (favoriteId: string) => void;
handleReorder: (favoriteId: string, sequence: number) => void;
handleDrop: (self: DropTargetRecord,source: ElementDragPayload, location: DragLocationHistory) => void;
};

Expand Down
19 changes: 8 additions & 11 deletions web/core/components/workspace/sidebar/favorites/favorites-menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import { usePlatformOS } from "@/hooks/use-platform-os";
// plane web components
import { FavoriteFolder } from "./favorite-folder";
import { FavoriteRoot } from "./favorite-items";
import { getDestinationStateSequence, getInstructionFromPayload, TargetData } from "./favorites.helpers";
import { getInstructionFromPayload, TargetData } from "./favorites.helpers";
import { NewFavoriteFolder } from "./new-fav-folder";

export const SidebarFavoritesMenu = observer(() => {
Expand Down Expand Up @@ -94,22 +94,22 @@ export const SidebarFavoritesMenu = observer(() => {
handleMoveToFolder(sourceData.id, parentId);
}
//handle remove from folder if dropped outside of the folder
if (parentId && sourceData.isChild) {
if (parentId && parentId !== sourceData.parentId && sourceData.isChild) {
handleRemoveFromFavoritesFolder(sourceData.id);
}

// handle reordering at root level
if (droppedFavId) {
if (instruction != "make-child") {
const destinationSequence = getDestinationStateSequence(groupedFavorites, droppedFavId, instruction);
handleReorder(sourceData.id, destinationSequence || 0);
// const destinationSequence = getgetDestinationSequence(droppedFavId, instruction);
handleReorder(sourceData.id, droppedFavId, instruction);
}
}
} else {
//handling reordering for favorites
if (droppedFavId) {
const destinationSequence = getDestinationStateSequence(groupedFavorites, droppedFavId, instruction);
handleReorder(sourceData.id, destinationSequence || 0);
// const destinationSequence = getgetDestinationSequence(droppedFavId, instruction);
handleReorder(sourceData.id, droppedFavId, instruction);
}

// handle removal from folder if dropped outside a folder
Expand Down Expand Up @@ -147,10 +147,8 @@ export const SidebarFavoritesMenu = observer(() => {
};

const handleReorder = useCallback(
(favoriteId: string, sequence: number) => {
reOrderFavorite(workspaceSlug.toString(), favoriteId, {
sequence: sequence,
}).catch(() => {
(favoriteId: string, droppedFavId: string, edge: string | undefined) => {
reOrderFavorite(workspaceSlug.toString(), favoriteId, droppedFavId, edge).catch(() => {
setToast({
type: TOAST_TYPE.ERROR,
title: "Error!",
Expand Down Expand Up @@ -271,7 +269,6 @@ export const SidebarFavoritesMenu = observer(() => {
isLastChild={index === length - 1}
handleRemoveFromFavorites={handleRemoveFromFavorites}
handleRemoveFromFavoritesFolder={handleRemoveFromFavoritesFolder}
handleReorder={handleReorder}
handleDrop={handleDrop}
/>
) : (
Expand Down
41 changes: 0 additions & 41 deletions web/core/components/workspace/sidebar/favorites/favorites.helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,47 +9,6 @@ export type TargetData = {
isChild: boolean;
}

export const getDestinationStateSequence = (
favoriteMap: Record<string, IFavorite>,
destinationId: string,
edge: string | undefined
) => {
const defaultSequence = 65535;
if (!edge) return defaultSequence;


const favoriteIds = orderBy(Object.values(favoriteMap), "sequence", "desc")
.filter((fav: IFavorite) => !fav.parent)
.map((fav: IFavorite) => fav.id);
const destinationStateIndex = favoriteIds.findIndex((id) => id === destinationId);
const destinationStateSequence = favoriteMap[destinationId]?.sequence || undefined;

if (!destinationStateSequence) return defaultSequence;


let resultSequence = defaultSequence;
if (edge === "reorder-above") {
const prevStateSequence = favoriteMap[favoriteIds[destinationStateIndex - 1]]?.sequence || undefined;

if (prevStateSequence === undefined) {
resultSequence = destinationStateSequence + defaultSequence;
}else {
resultSequence = (destinationStateSequence + prevStateSequence) / 2
}
} else if (edge === "reorder-below") {
const nextStateSequence = favoriteMap[favoriteIds[destinationStateIndex + 1]]?.sequence || undefined;

if (nextStateSequence === undefined) {
resultSequence = destinationStateSequence - defaultSequence;
} else {
resultSequence = (destinationStateSequence + nextStateSequence) / 2;
}
}
resultSequence = Math.round(resultSequence)

return resultSequence;
};

/**
* extracts the Payload and translates the instruction for the current dropTarget based on drag and drop payload
* @param dropTarget dropTarget for which the instruction is required
Expand Down
41 changes: 34 additions & 7 deletions web/core/store/favorite.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { uniqBy } from "lodash";
import { orderBy, result, uniqBy } from "lodash";
import set from "lodash/set";
import { action, observable, makeObservable, runInAction, computed } from "mobx";
import { v4 as uuidv4 } from "uuid";
Expand Down Expand Up @@ -28,7 +28,12 @@ export interface IFavoriteStore {
getGroupedFavorites: (workspaceSlug: string, favoriteId: string) => Promise<IFavorite[]>;
moveFavoriteToFolder: (workspaceSlug: string, favoriteId: string, data: Partial<IFavorite>) => Promise<void>;
removeFavoriteEntity: (workspaceSlug: string, entityId: string) => Promise<void>;
reOrderFavorite: (workspaceSlug: string, favoriteId: string, data: Partial<IFavorite>) => Promise<void>;
reOrderFavorite: (
workspaceSlug: string,
favoriteId: string,
destinationId: string,
edge: string | undefined
) => Promise<void>;
removeFromFavoriteFolder: (workspaceSlug: string, favoriteId: string) => Promise<void>;
removeFavoriteFromStore: (entity_identifier: string) => void;
}
Expand Down Expand Up @@ -190,14 +195,37 @@ export class FavoriteStore implements IFavoriteStore {
}
};

reOrderFavorite = async (workspaceSlug: string, favoriteId: string, data: Partial<IFavorite>) => {
reOrderFavorite = async (
workspaceSlug: string,
favoriteId: string,
destinationId: string,
edge: string | undefined
) => {
const initialSequence = this.favoriteMap[favoriteId].sequence;
try {
let resultSequence = 10000;
if (edge) {
const sortedIds = orderBy(this.favoriteMap, "sequence", "desc").map((fav: IFavorite) => fav.id);
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect usage of orderBy on an object.

At line 208, orderBy is being called on this.favoriteMap, which is an object. The orderBy function expects an array as its first argument. To sort the favorites correctly, convert the object to an array using Object.values.

Apply this diff to fix the issue:

-const sortedIds = orderBy(this.favoriteMap, "sequence", "desc").map((fav: IFavorite) => fav.id);
+const sortedIds = orderBy(Object.values(this.favoriteMap), "sequence", "desc").map((fav: IFavorite) => fav.id);
📝 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 sortedIds = orderBy(this.favoriteMap, "sequence", "desc").map((fav: IFavorite) => fav.id);
const sortedIds = orderBy(Object.values(this.favoriteMap), "sequence", "desc").map((fav: IFavorite) => fav.id);

Copy link
Member Author

@vamsikrishnamathala vamsikrishnamathala Nov 29, 2024

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 29, 2024

Choose a reason for hiding this comment

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

@mathalav55, thank you for fixing the issue!

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

const destinationSequence = this.favoriteMap[destinationId]?.sequence || undefined;
if (destinationSequence) {
const destinationIndex = sortedIds.findIndex((id) => id === destinationId);
if (edge === "reorder-above") {
const prevSequence = this.favoriteMap[sortedIds[destinationIndex - 1]]?.sequence || undefined;
if (prevSequence) {
resultSequence = (destinationSequence + prevSequence) / 2;
} else {
resultSequence = destinationSequence + resultSequence;
}
} else {
resultSequence = destinationSequence - resultSequence;
}
}
}
Comment on lines +206 to +223
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 29, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider sequence normalization and bounds checking

The current sequence calculation has several potential issues:

  1. When reordering above (line 217-218), adding a fixed value (10000) could lead to very large numbers over time
  2. When reordering below (line 220), subtracting could lead to negative sequences
  3. No handling for when sequences get too close to each other

Consider implementing sequence normalization:

  reOrderFavorite = async (workspaceSlug: string, favoriteId: string, destinationId: string, edge: string | undefined) => {
    const initialSequence = this.favoriteMap[favoriteId].sequence;
    try {
-     let resultSequence = 10000;
+     const SEQUENCE_STEP = 1000;
+     const MAX_SEQUENCE = 1000000;
+     
+     // Normalize sequences if they get too large
+     const normalizeSequences = () => {
+       const favorites = Object.values(this.favoriteMap);
+       if (favorites.some(f => f.sequence > MAX_SEQUENCE)) {
+         const sorted = orderBy(favorites, "sequence", "desc");
+         sorted.forEach((fav, idx) => {
+           fav.sequence = (sorted.length - idx) * SEQUENCE_STEP;
+         });
+       }
+     };
      
      let resultSequence;
      if (edge) {
        const sortedIds = orderBy(Object.values(this.favoriteMap), "sequence", "desc").map((fav: IFavorite) => fav.id);
        const destinationSequence = this.favoriteMap[destinationId]?.sequence || undefined;
        if (destinationSequence) {
          const destinationIndex = sortedIds.findIndex((id) => id === destinationId);
          if (edge === "reorder-above") {
            const prevSequence = this.favoriteMap[sortedIds[destinationIndex - 1]]?.sequence || destinationSequence + SEQUENCE_STEP;
            resultSequence = (destinationSequence + prevSequence) / 2;
          } else {
            const nextSequence = this.favoriteMap[sortedIds[destinationIndex + 1]]?.sequence || Math.max(0, destinationSequence - SEQUENCE_STEP);
            resultSequence = (destinationSequence + nextSequence) / 2;
          }
        }
      }
+     normalizeSequences();
📝 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
let resultSequence = 10000;
if (edge) {
const sortedIds = orderBy(Object.values(this.favoriteMap), "sequence", "desc").map((fav: IFavorite) => fav.id);
const destinationSequence = this.favoriteMap[destinationId]?.sequence || undefined;
if (destinationSequence) {
const destinationIndex = sortedIds.findIndex((id) => id === destinationId);
if (edge === "reorder-above") {
const prevSequence = this.favoriteMap[sortedIds[destinationIndex - 1]]?.sequence || undefined;
if (prevSequence) {
resultSequence = (destinationSequence + prevSequence) / 2;
} else {
resultSequence = destinationSequence + resultSequence;
}
} else {
resultSequence = destinationSequence - resultSequence;
}
}
}
const SEQUENCE_STEP = 1000;
const MAX_SEQUENCE = 1000000;
// Normalize sequences if they get too large
const normalizeSequences = () => {
const favorites = Object.values(this.favoriteMap);
if (favorites.some(f => f.sequence > MAX_SEQUENCE)) {
const sorted = orderBy(favorites, "sequence", "desc");
sorted.forEach((fav, idx) => {
fav.sequence = (sorted.length - idx) * SEQUENCE_STEP;
});
}
};
let resultSequence;
if (edge) {
const sortedIds = orderBy(Object.values(this.favoriteMap), "sequence", "desc").map((fav: IFavorite) => fav.id);
const destinationSequence = this.favoriteMap[destinationId]?.sequence || undefined;
if (destinationSequence) {
const destinationIndex = sortedIds.findIndex((id) => id === destinationId);
if (edge === "reorder-above") {
const prevSequence = this.favoriteMap[sortedIds[destinationIndex - 1]]?.sequence || destinationSequence + SEQUENCE_STEP;
resultSequence = (destinationSequence + prevSequence) / 2;
} else {
const nextSequence = this.favoriteMap[sortedIds[destinationIndex + 1]]?.sequence || Math.max(0, destinationSequence - SEQUENCE_STEP);
resultSequence = (destinationSequence + nextSequence) / 2;
}
}
}
normalizeSequences();

runInAction(() => {
set(this.favoriteMap, [favoriteId, "sequence"], data.sequence);
set(this.favoriteMap, [favoriteId, "sequence"], resultSequence);
});

await this.favoriteService.updateFavorite(workspaceSlug, favoriteId, data);
await this.favoriteService.updateFavorite(workspaceSlug, favoriteId, { sequence: resultSequence });
} catch (error) {
console.error("Failed to move favorite folder");
runInAction(() => {
Expand All @@ -214,8 +242,7 @@ export class FavoriteStore implements IFavoriteStore {
//remove parent
set(this.favoriteMap, [favoriteId, "parent"], null);
});
await this.favoriteService.updateFavorite(workspaceSlug, favoriteId, { parent: null});

await this.favoriteService.updateFavorite(workspaceSlug, favoriteId, { parent: null });
} catch (error) {
console.error("Failed to move favorite");
runInAction(() => {
Expand Down