Skip to content

feat(editor): Combine Move and Change owner modals #15756

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

Merged

Conversation

Cadiac
Copy link
Contributor

@Cadiac Cadiac commented May 27, 2025

Summary

This PR combines the existing "Move to Folder" and "Change owner" modals into one generic "Move" modal. This new modal replaces existing MoveToFolderModal and partially replaces ProjectMoveResourceModal, which still used for credentials after this.

The new modal allows moving workflows or folders to other folders. It also supports transferring the folder / workflow ownership to another project (or user), and allows sharing credentials used at the transferred workflows if user wants to do so.

This is also the first time transferring folders to other projects is supported on the UI at all.

move-modal.mp4

The delete modal also uses the new folder dropdown, listing possible folders as the transfer target.

delete-modal.mp4

In order to accomplish this I had to do some amount of refactoring, and I also discovered some type issues / bugs (like we weren't fetching paginated available folders before on the old picker) which I fixed while working on this. The changeset is pretty big, but a large portion of that are tests.

"Move" option isn't available on "Overview" or "Shared with you" pages, for now in order for the modal to work it has to be a page with project ID on its route params.

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/ADO-3161/fe-combine-change-owner-and-move-to-folder-modals
https://linear.app/n8n/issue/ADO-3287/feature-non-flat-list-in-sharingmoving-folders-dialog

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

Cadiac added 21 commits May 27, 2025 16:48
@n8n-assistant n8n-assistant bot added the n8n team Authored by the n8n team label May 27, 2025
@Cadiac Cadiac marked this pull request as ready for review May 27, 2025 19:16
Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

cubic found 3 issues across 22 files. Review them in cubic.dev

React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.


// The dropdown focuses after a small delay (once modal's slide in animation is done).
// On the component we listen for an event, but here the wait should be very predictable.
cy.wait(500);
Copy link

Choose a reason for hiding this comment

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

Using a fixed wait time (500ms) can lead to flaky tests. Consider using Cypress's built-in retry and timeout mechanisms instead of arbitrary waits.

Suggested change
cy.wait(500);
getMoveFolderModal().find('input').should('be.visible').should('be.enabled');

Copy link
Contributor Author

@Cadiac Cadiac May 27, 2025

Choose a reason for hiding this comment

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

This case is tricky, and while I'm not a fan of the fixed timeout here it is way longer than the brief slide in animation that the modal does before opening the dropdown and focusing on it. If the idea is to test that the autofocus works some kind of wait has to happen here - the first couple of letteres typed were ignored before I added this.

Perhaps waiting for a focused field to appear could be the solution...

The reason the dropdown opens & focuses only once the modal stops is that otherwise the popper dropdown gets misplaced if opened mid animation.

@Cadiac Cadiac requested review from a team and CharlieKolb May 27, 2025 19:28
Copy link
Contributor

@CharlieKolb CharlieKolb left a comment

Choose a reason for hiding this comment

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

Not fully done yet, will want to test the credentials stuff in particular I imagine. But looks really good!

let folderText = '';
let workflowText = '';
if (subFolderCount.value > 0) {
folderText = i18n.baseText('folders.move.modal.folder.count', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like doing "| {count} folder | {count} folders" works to show an empty string at 0, though I won't argue that's strictly better, just fyi since I got curious :D

CharlieKolb
CharlieKolb previously approved these changes May 28, 2025
@shortstacked
Copy link
Contributor

Workflow Test Results 📊 ⚠️ 4 Warnings (0 Failed), 👍 79 Successful out of 83 total workflows.

View full workflow run

Tested Ref: e7d15f13377a4c7c5a3a9a13319cf2df25b5086a by @CharlieKolb

⚠️ Warnings (4)

Workflow ID Workflow Name Reason
237 BasicLLMChain:AzureChat Workflow contains new data that previously did not exist.
35 Slack:User:getPresence info:UserProfile:get update... Workflow contains new data that previously did not exist.
53 ConvertKit:CustomField:create getAll update delete... Workflow contains new data that previously did not exist.
257 Agent:auto-fix:anthropic Workflow contains new data that previously did not exist.

Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@Cadiac Cadiac requested a review from CharlieKolb May 28, 2025 14:53
CharlieKolb
CharlieKolb previously approved these changes May 28, 2025
Copy link
Contributor

@CharlieKolb CharlieKolb left a comment

Choose a reason for hiding this comment

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

Tested the credentials setup, looks amazing!

@shortstacked
Copy link
Contributor

Workflow Test Results 📊 ⚠️ 4 Warnings (0 Failed), 👍 79 Successful out of 83 total workflows.

View full workflow run

Tested Ref: f43cd7e1fed0ef87d6e71e01b7ce8e4a00f1db4c by @CharlieKolb

⚠️ Warnings (4)

Workflow ID Workflow Name Reason
237 BasicLLMChain:AzureChat Workflow contains new data that previously did not exist.
35 Slack:User:getPresence info:UserProfile:get update... Workflow contains new data that previously did not exist.
53 ConvertKit:CustomField:create getAll update delete... Workflow contains new data that previously did not exist.
257 Agent:auto-fix:anthropic Workflow contains new data that previously did not exist.

Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@shortstacked
Copy link
Contributor

Workflow Test Results 📊 ⚠️ 4 Warnings (0 Failed), 👍 79 Successful out of 83 total workflows.

View full workflow run

Tested Ref: aa5499c49529776eae376cce5409e08d009c302d by @RicardoE105

⚠️ Warnings (4)

Workflow ID Workflow Name Reason
237 BasicLLMChain:AzureChat Workflow contains new data that previously did not exist.
35 Slack:User:getPresence info:UserProfile:get update... Workflow contains new data that previously did not exist.
53 ConvertKit:CustomField:create getAll update delete... Workflow contains new data that previously did not exist.
257 Agent:auto-fix:anthropic Workflow contains new data that previously did not exist.

Copy link
Contributor

✅ All Cypress E2E specs passed

@Cadiac Cadiac merged commit e860dd6 into master May 28, 2025
35 checks passed
@Cadiac Cadiac deleted the ado-3161-fe-combine-change-owner-and-move-to-folder-modals branch May 28, 2025 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
n8n team Authored by the n8n team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants