Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
17 changes: 6 additions & 11 deletions web/core/components/workspace/sidebar/favorites/favorites-menu.tsx
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,20 @@ 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);
handleReorder(sourceData.id, droppedFavId, instruction);
}
}
} else {
//handling reordering for favorites
if (droppedFavId) {
const destinationSequence = getDestinationStateSequence(groupedFavorites, droppedFavId, instruction);
handleReorder(sourceData.id, destinationSequence || 0);
handleReorder(sourceData.id, droppedFavId, instruction);
}

// handle removal from folder if dropped outside a folder
Expand Down Expand Up @@ -147,10 +145,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 +267,6 @@ export const SidebarFavoritesMenu = observer(() => {
isLastChild={index === length - 1}
handleRemoveFromFavorites={handleRemoveFromFavorites}
handleRemoveFromFavoritesFolder={handleRemoveFromFavoritesFolder}
handleReorder={handleReorder}
handleDrop={handleDrop}
/>
) : (
Expand Down
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
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(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;
}
}
}
Comment on lines +206 to +223
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

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
Loading