-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: move requests between collections #5410
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.
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
feat: move requests between collections #5410
Conversation
|
Thank you, @jayakrishnancn! |
|
Hi @jayakrishnancn, Thanks a lot for working on this. It works well when it comes to moving requests/folders across the collections. I was testing the PR on my local and found a major issue within the PR, , for some reason none of the On top of this - I guess we will have to expand the playwright test-cases to accomodate this scenario I guess we can add similar Screen.Recording.2025-09-04.at.12.51.52.PM.mov |
|
Hi @sanish-bruno, However, I've included a fix for it in my latest commit. I'm reusing the "clone-folder," where the 'grpc-request' option was missing. Ideally, all requests should be cloned, in my opinion, and shouldn't be limited to just a few types. Since I'm not sure about the impact of removing the If you suggest removing it, I can do that through another bugfix PR. Before my change (existing main branch code): Cloning a gRPC request within a collection wasn't functioning properly in main branch code: before.movafter the chagnes: Screen.Recording.2025-09-04.at.1.48.11.PM.mov |
can you clarify on this? I have already added few cases in the test files
|
Scenarios where we have grpc, http, graphql requests within a folder and all of them are moved successfully |
|
|
||
| await dispatch(handleCollectionItemDrop({ targetItem: item, draggedItem, dropType, collectionUid })) | ||
| // Handle cross-collection drops differently | ||
| if (sourceCollectionUid !== collectionUid) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jayakrishnancn do we need the check here? we can just pass the 'sourceCollectoionUid' and handle it from handleCollectionItemDrop function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Removed the guard 'if' condition.
| if (isCollectionItem(itemType)) { | ||
| dispatch(handleCollectionItemDrop({ targetItem: collection, draggedItem, dropType: 'inside', collectionUid: collection.uid })) | ||
| // Handle cross-collection drops | ||
| if (draggedItem.sourceCollectionUid && draggedItem.sourceCollectionUid !== collection.uid) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the check here? we can just pass the 'sourceCollectoionUid' and handle it from handleCollectionItemDrop function if it is undefined, same as above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made adjustments.
| if (draggedItemUid === targetItemUid) return false; | ||
|
|
||
| // For cross-collection moves, we allow the drop | ||
| if (sourceCollectionUid !== collectionUid) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jayakrishnancn I guess we can add this logic inside calculateDraggedItemNewPathname function, were we will return a newPathname according to the collection pathname.
Don't have the clarity one when it will happen but what if sourceCollectionId is undefined, the function will return true leading into some error/failure along the line.
What are your thought on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, every item being dragged should have a unique ID. While being undefined is valid in isolation, sourceCollectionUid is set during drag initialization. The drag system ensures this property when dragging begins.
I guess we can add this logic inside calculateDraggedItemNewPathname function, were we will return a newPathname according to the collection pathname.
Personally, I’m not in favor of moving this logic to the calculateDraggedItemNewPathname method, as its purpose is to return the pathname, not to determine if it’s a cross-collection.
Additionally, calculateDraggedItemNewPathname is also utilized in handleReorderInSameLocation. I'm unsure about the impact of modifying it.
if you still believe moving it makes sense, we can make the changes
|
|
||
| // Handle cross-collection moves | ||
| if (sourceCollectionUid && sourceCollectionUid !== collectionUid) { | ||
| return dispatch(handleCrossCollectionItemMove({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jayakrishnancn Just out of curiosity, do we need a extra handleCrossCollectionItemMove function? is it possible to make little modifications to the existing logic to handle this?
For example:
we can generate targetItemDirectory (L 735) , draggedItemDirectory (L 737) from collectionUid , sourceCollectionUid respectively.
Have you tried that approach before?
I was thinking of hassle of maintaining two different functions that almost handle similar functionality. Let me know if you think it is better to keep them seperate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first, I thought of adding features like renaming through generateUniqueFilename() when duplicate request names exist in a collection/folders, since I usually work with collectionswhere this happens. However, I didn’t want to modify the current code.
After thinking it through, you’re right—reusing the method without renaming is better. In the case of duplicates, we can show an error toast. Later, if needed, we can introduce the renaming feature in a different PR if required
Yup make sense, thanks for fixing it! we will be keeping the |
2d01d3e to
1a84a72
Compare
|
Hi @sanish-bruno, I have updated the UX as well. While dropping, instead of showing a line, it now displays a complete border, similar to a folder drop. Additionally, in case of a duplicate request during dropping, it will now show the default error message. Screen.Recording.2025-09-06.at.11.59.47.AM.mov |
|
@jayakrishnancn currently gRPC is in beta, you can enable it from preferences -> beta -> enable grpc, we just hidden the entry points until it is out of beta https://docs.usebruno.com/send-requests/grpc/overview, but you can ignore the grpc-requests for now! we can write a playwright test case once it is out of beta |
|
@jayakrishnancn i rebased, squashed commits moved the test files to a new folder as per the main branch and raised a new PR #5525 Thanks for working on this! We will try to get it merged for upcoming release that will ideally happen in 2 weeks time. |
|
@jayakrishnancn We made some changes on top of your PR and have merged it. Thank you! |



Description
Move requests between collections.
Move requests/folders between collections using drag and drop. Currently it is only possible to move requests/folders within a collection.
while moving, if the target collection or folder has the same request/folder, then rename the new dropped folder/request and move it to the new target
resolves #3320
Screen.Recording.2025-08-23.at.8.12.26.PM.mov
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.