Skip to content
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

fix(Switch Node): Maintain output connections #11162

Conversation

michael-radency
Copy link
Contributor

@michael-radency michael-radency commented Oct 8, 2024

Summary

maintain connection on re-order
remove connection if rule for this connection was removed
if fallback set to extra preserve connected node and do not attach it to added rule output
if using expression for setting number of outputs trim connections if their number was reduced

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/NODE-1669/switch-node-adding-routing-rule-reconnects-fallback-route

fixes #11587

@michael-radency michael-radency added node/improvement New feature or request ui Enhancement in /editor-ui or /design-system n8n team Authored by the n8n team node/issue Issue with a node labels Oct 8, 2024
@michael-radency michael-radency changed the title fix(Switch Node): Maintain output connections when changing number of outputs fix(Switch Node): Maintain output connections Oct 11, 2024
Copy link
Contributor

@ShireenMissi ShireenMissi left a comment

Choose a reason for hiding this comment

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

I tested locally and on test instance, the only issue is when you add a new rule and create a new connection then reorder the new output doesn't maintain its connection
on test instance it works once I save the workflow
locally I had to save and refresh the page to get it to work

packages/editor-ui/src/components/NodeSettings.vue Outdated Show resolved Hide resolved
Copy link
Member

@elsmr elsmr left a comment

Choose a reason for hiding this comment

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

Found a bug when the workflow is not saved.

In general I think it's an OK solution for the Switch node, but it would be great if connections were saved by ID instead of by index to avoid all this complexity. But not clear to me how to do it without a breaking change.

I think you could have similar bugs in other nodes with dynamic outputs such as the Webhook node.

packages/editor-ui/src/stores/workflows.store.ts Outdated Show resolved Hide resolved
packages/editor-ui/src/components/NodeSettings.vue Outdated Show resolved Hide resolved
elsmr
elsmr previously approved these changes Nov 1, 2024
Copy link
Contributor

github-actions bot commented Nov 1, 2024

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

Copy link

cypress bot commented Nov 1, 2024

n8n    Run #7843

Run Properties:  status check passed Passed #7843  •  git commit 8fc77cc794: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 michael-radency 🗃️ e2e/*
Project n8n
Branch Review node-1669-switch-node-adding-routing-rule-reconnects-fallback-route
Run status status check passed Passed #7843
Run duration 04m 28s
Commit git commit 8fc77cc794: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 michael-radency 🗃️ e2e/*
Committer Michael Kret
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 2
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 470
View all changes introduced in this branch ↗︎

Copy link
Contributor

github-actions bot commented Nov 1, 2024

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

Copy link

codecov bot commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 61.81818% with 42 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/editor-ui/src/utils/nodeSettingsUtils.ts 65.65% 34 Missing ⚠️
packages/editor-ui/src/components/NodeSettings.vue 28.57% 5 Missing ⚠️
packages/editor-ui/src/stores/workflows.store.ts 25.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Contributor

✅ All Cypress E2E specs passed

@michael-radency michael-radency merged commit 9bd79fc into master Nov 13, 2024
35 checks passed
@michael-radency michael-radency deleted the node-1669-switch-node-adding-routing-rule-reconnects-fallback-route branch November 13, 2024 11:30
@github-actions github-actions bot mentioned this pull request Nov 13, 2024
@janober
Copy link
Member

janober commented Nov 13, 2024

Got released with [email protected]

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 node/improvement New feature or request node/issue Issue with a node Released ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When removing a switch rule the outputs changes
4 participants