fix: Folder Sequencing When we try to move any folder at the bottom o…#8208
fix: Folder Sequencing When we try to move any folder at the bottom o…#8208ravindra-bruno wants to merge 4 commits into
Conversation
…f the folders list
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughCentralizes drop placement/legality into utilities (above/inside/below), threads dropType into reordering, updates CollectionItem and collection-row hover styling to use the new contract, and adds Jest tests covering placement, pathname calculation, legality, and reordering. ChangesEnhanced Collection Drag-and-Drop
Sequence DiagramsequenceDiagram
participant User as User Pointer
participant Item as CollectionItem Component
participant Monitor as ReactDnD_Monitor
participant Determine as determineCollectionItemDrop
participant Can as canCollectionItemBeDropped
participant Action as handleCollectionItemDrop
participant Reorder as getReorderedItemsInTargetDirectory
participant Store as ReduxStore
User->>Item: hover / drag over row
Item->>Monitor: read clientOffset & hoverBoundingRect
Monitor->>Determine: clientOffset, hoverBoundingRect, item
Determine->>Item: dropType (above/inside/below)
Item->>Can: draggedItem,targetItem,dropType,collection context
Can->>Item: allow/reject
Item->>Action: dispatch drop with dropType
Action->>Reorder: items,targetItemUid,draggedItemUid,dropType
Reorder->>Store: update sequences
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-app/src/utils/collections/index.js`:
- Around line 1549-1554: The current descendant check uses
targetItemPathname?.startsWith(draggedItemPathname) which incorrectly treats
siblings like "/users-archive" as descendants of "/users"; change this to a
path-segment-aware check: ensure you only treat target as a descendant when
targetItemPathname === draggedItemPathname or targetItemPathname starts with
draggedItemPathname + "/" (taking care of leading/trailing slashes), and replace
the existing startsWith check in the block that calls
calculateDraggedItemNewPathname (using draggedItemPathname and
targetItemPathname) with that logic; then apply the exact same change inside
handleCollectionItemDrop() so validation and execution use the same
separator-aware ancestry test.
In `@packages/bruno-app/src/utils/tests/collections/drag-and-drop.spec.js`:
- Around line 154-201: Add a test to cover the sibling-prefix regression for
canCollectionItemBeDropped: create two folder items whose pathnames share a
prefix but are not ancestor/descendant (e.g., parent pathname
'/collections/my-collection/users' and sibling pathname
'/collections/my-collection/users-archive'), call canCollectionItemBeDropped
with draggedItem having sourceCollectionUid equal to collectionUid and
targetItem the sibling, and assert the result is true; place this alongside the
existing descendant and same-collection tests to ensure the function
(canCollectionItemBeDropped) checks full path-segment ancestry rather than
simple string prefix matching.
- Around line 8-10: The test uses hardcoded POSIX strings for
collectionPathname, folderPathname, and requestPathname which breaks on Windows;
replace these literals with the shared path helper (e.g., path.join or the
repo's path utility) to build the input path fixtures and the expected results
so they are OS-agnostic; update the declarations that create collectionPathname,
folderPathname, requestPathname and any expectations/assertions that compare
against them (also apply the same change to the other fixtures/expectations in
the 107–137 block) so all paths use the helper instead of hardcoded '/'
separators.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60702ceb-b8ed-47ea-919f-b9619438ebdf
📒 Files selected for processing (4)
packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/utils/collections/index.jspackages/bruno-app/src/utils/tests/collections/drag-and-drop.spec.js
…void sibling-prefix false positives
…dge zones (BRU-1112) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…f the folders list
Description
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Tests