[WEB-2774]fix:reordering favorites and favorite folders #6119
Conversation
WalkthroughThe changes in this pull request involve modifications to several components related to the favorites feature within the application. Key updates include the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
vamsikrishnamathala
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
web/core/components/workspace/sidebar/favorites/favorites-menu.tsx (3)
104-105: Remove commented-out codeThe commented-out lines referencing
getgetDestinationSequenceshould be removed as they're no longer needed with the new instruction-based approach.- // const destinationSequence = getgetDestinationSequence(droppedFavId, instruction); handleReorder(sourceData.id, droppedFavId, instruction);Also applies to: 111-112
150-151: Add documentation for the edge parameterThe handleReorder function has been successfully updated to use the new instruction-based approach. However, the
edgeparameter's possible values and their implications should be documented.Add JSDoc comments to clarify the parameter usage:
const handleReorder = useCallback( + /** + * Reorders a favorite item relative to another item + * @param favoriteId - ID of the favorite to be reordered + * @param droppedFavId - ID of the favorite item it was dropped on + * @param edge - Position relative to the dropped item ('top' | 'bottom' | undefined) + */ (favoriteId: string, droppedFavId: string, edge: string | undefined) => {
Line range hint
150-158: Consider adding tests for the new reordering logicThe shift to instruction-based reordering is a significant change that would benefit from comprehensive test coverage to ensure reliability across different scenarios (e.g., reordering within folders, between folders, at root level).
Would you like help creating test cases for the various reordering scenarios?
web/core/store/favorite.store.ts (3)
1-1: Remove unused import 'result' from 'lodash'.The
resultfunction imported from lodash is not used in this file. Removing unused imports can help improve code clarity and reduce bundle size.Apply this diff to remove the unused import:
-import { orderBy, result, uniqBy } from "lodash"; +import { orderBy, uniqBy } from "lodash";
215-215: EnsureresultSequenceis an integer after averaging.When calculating
resultSequenceby averagingdestinationSequenceandprevSequence, the result may be a floating-point number. If thesequenceis expected to be an integer, consider rounding or flooring the result to maintain consistency.Apply this diff to ensure
resultSequenceis an integer:-resultSequence = (destinationSequence + prevSequence) / 2; +resultSequence = Math.floor((destinationSequence + prevSequence) / 2);
Line range hint
230-233: Avoid throwing errors insiderunInActionblocks.Throwing an error inside a
runInActionblock may lead to unintended side effects or suppress the error. It's better to perform state updates within the action and throw the error outside.Apply this diff to move the
throw errorstatement outside ofrunInAction:runInAction(() => { set(this.favoriteMap, [favoriteId, "sequence"], initialSequence); - throw error; }); +}); +throw error;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
web/core/components/workspace/sidebar/favorites/favorite-folder.tsx(0 hunks)web/core/components/workspace/sidebar/favorites/favorites-menu.tsx(3 hunks)web/core/components/workspace/sidebar/favorites/favorites.helpers.ts(0 hunks)web/core/store/favorite.store.ts(4 hunks)
💤 Files with no reviewable changes (2)
- web/core/components/workspace/sidebar/favorites/favorite-folder.tsx
- web/core/components/workspace/sidebar/favorites/favorites.helpers.ts
🔇 Additional comments (3)
web/core/components/workspace/sidebar/favorites/favorites-menu.tsx (2)
97-98: LGTM: Improved folder removal logic
The condition for removing favorites from folders is now more precise, properly checking both the parent ID difference and the child status.
31-31: Verify the architectural change in reordering logic
The shift from sequence-based to instruction-based reordering represents a significant architectural change. This approach might be more maintainable but needs verification across different scenarios.
✅ Verification successful
Architectural change in reordering logic is part of a broader refactoring
The shift from sequence-based to instruction-based reordering is a deliberate architectural change that's been consistently implemented across the codebase. The old destinationSequence references only remain in favorite.store.ts for the actual sequence calculations, while the UI components have been fully migrated to use the new instruction-based approach with getInstructionFromPayload and TargetData. This pattern is also consistently used in the labels component, showing it's a standardized approach across similar drag-and-drop functionalities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old sequence-based approach
rg "getDestinationStateSequence|destinationSequence" --type typescript
Length of output: 107
Script:
#!/bin/bash
# Search for any remaining references to the old sequence-based approach, using ripgrep without file type filter
rg "getDestinationStateSequence|destinationSequence"
# Also search for the new instruction-based approach to understand the scope of changes
rg "getInstructionFromPayload|TargetData"
Length of output: 6284
web/core/store/favorite.store.ts (1)
206-223: Verify the logic for calculating resultSequence.
The computation of resultSequence may have potential issues:
- When
edgeis undefined,resultSequenceremains at the initial value of10000. Is this the intended default behavior? - In the
reorder-abovecase, ifdestinationIndex - 1is less than0, accessingsortedIds[destinationIndex - 1]will result inundefined, which may cause unexpected behavior. - In the
elseblock, subtractingresultSequence(which is10000) fromdestinationSequencemight result in a negative sequence number.
Consider reviewing and testing the sequence calculations to ensure they handle all edge cases correctly.
web/core/store/favorite.store.ts
Outdated
| try { | ||
| let resultSequence = 10000; | ||
| if (edge) { | ||
| const sortedIds = orderBy(this.favoriteMap, "sequence", "desc").map((fav: IFavorite) => fav.id); |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
@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!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/core/store/favorite.store.ts (1)
Line range hint
256-282: Enhance type safety and reduce code duplicationThe switch statement has repetitive patterns and could benefit from type safety improvements.
Consider these improvements:
+ type EntityType = 'view' | 'module' | 'page' | 'cycle' | 'project'; + - removeFavoriteEntityFromStore = (entity_identifier: string, entity_type: string) => { + removeFavoriteEntityFromStore = (entity_identifier: string, entity_type: EntityType) => { + const entityMaps = { + view: this.viewStore.viewMap, + module: this.moduleStore.moduleMap, + page: this.pageStore.data, + cycle: this.cycleStore.cycleMap, + project: this.projectStore.projectMap + }; + + const entityMap = entityMaps[entity_type]; + if (entityMap?.[entity_identifier]) { + entityMap[entity_identifier].is_favorite = false; + } - switch (entity_type) { - case "view": - return ( - this.viewStore.viewMap[entity_identifier] && (this.viewStore.viewMap[entity_identifier].is_favorite = false) - ); - case "module": - return ( - this.moduleStore.moduleMap[entity_identifier] && - (this.moduleStore.moduleMap[entity_identifier].is_favorite = false) - ); - // ... other cases - default: - return; - } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/core/store/favorite.store.ts(4 hunks)
🔇 Additional comments (2)
web/core/store/favorite.store.ts (2)
31-36: LGTM: Interface changes for reordering functionality
The updated interface for reOrderFavorite with the new parameters destinationId and edge is well-defined and properly typed.
208-208: Fix incorrect usage of orderBy on favoriteMap
This issue was previously identified and still needs to be addressed.
| 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; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider sequence normalization and bounds checking
The current sequence calculation has several potential issues:
- When reordering above (line 217-218), adding a fixed value (10000) could lead to very large numbers over time
- When reordering below (line 220), subtracting could lead to negative sequences
- 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.
| 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(); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/core/components/workspace/sidebar/favorites/favorites-menu.tsx (1)
Line range hint
148-156: Enhance error handling specificityWhile the reordering logic is solid, the error message could be more specific to help users understand what went wrong.
Consider this improvement:
reOrderFavorite(workspaceSlug.toString(), favoriteId, droppedFavId, edge).catch(() => { setToast({ type: TOAST_TYPE.ERROR, title: "Error!", - message: "Failed reorder favorite", + message: `Failed to reorder favorite${edge ? ` to ${edge} of target` : ''}. Please try again.`, }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/core/components/workspace/sidebar/favorites/favorites-menu.tsx(3 hunks)
🔇 Additional comments (3)
web/core/components/workspace/sidebar/favorites/favorites-menu.tsx (3)
31-31: LGTM: Import changes align with the new reordering strategy
The removal of getDestinationStateSequence and addition of getInstructionFromPayload and TargetData imports reflect the shift from sequence-based to instruction-based reordering.
110-110: LGTM: Reordering logic updated correctly
The reordering call has been updated to use the new instruction-based approach, which better handles the edge cases in the drag-and-drop reordering.
97-105: Verify folder removal edge cases
The condition for removing favorites from folders has been improved, but we should verify that this handles all edge cases correctly, particularly when:
- Moving between nested folders
- Moving from a nested folder to root
- Moving from root to a nested folder
✅ Verification successful
Based on the search results, I can now generate a final response as I have enough context about the folder handling logic:
The folder removal logic correctly handles all edge cases
The implementation properly handles all folder movement scenarios:
- Moving between folders: Checks
parentId !== sourceData.parentId && sourceData.isChild - Moving to root: Additional check
if (!parentId && sourceData.isChild) - Moving to another folder: Uses
handleMoveToFolderwhenparentId && parentId !== sourceData.parentId
The code maintains proper state by:
- Tracking child status via
isChildflag set ininitialData - Preserving parent-child relationships through explicit
parentIdchecks - Using separate handlers for folder operations (
handleMoveToFolder,handleRemoveFromFavoritesFolder)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for nested folder handling in the codebase
ast-grep --pattern 'parent: $parent'
Length of output: 38
Script:
#!/bin/bash
# Search for folder-related operations and parent handling
rg -A 5 "parentId|isChild" web/core/components/workspace/sidebar/favorites/
# Search for handleRemoveFromFavoritesFolder implementation
ast-grep --pattern 'handleRemoveFromFavoritesFolder($$$)'
# Search for handleReorder implementation
ast-grep --pattern 'handleReorder($$$)'
# Search for folder-related types or interfaces
rg -A 5 "interface.*Favorite|type.*Favorite" web/core/components/workspace/sidebar/favorites/
Length of output: 9793
Problem Statement
The re-ordering sequence for favorites is not generating as expected.
Summary
Resolved the issue with the re-ordering sequence generation.
Reference
WEB-2774
Summary by CodeRabbit
New Features
Bug Fixes
Chores