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 workflow-insert with reroute nodes #2008

Merged
merged 9 commits into from
Dec 23, 2024
Merged

Conversation

christian-byrne
Copy link
Collaborator

@christian-byrne christian-byrne commented Dec 22, 2024

Fix #1999: Reroute nodes cannot be be added via workflow-insert because their onConnectionsChange is invoked before graph is fully configured. Fix by adding the same guard other widgets have:

onConnectionsChange(_, index, connected) {
if (app.configuringGraph) {
// Dont run while the graph is still setting up
return
}

The connections are still set up correctly after the graph is configured.

┆Issue is synchronized with this Notion page by Unito

@christian-byrne christian-byrne requested review from a team as code owners December 22, 2024 07:04
@christian-byrne christian-byrne added the New Browser Test Expectations New browser test screenshot should be set by github action label Dec 22, 2024
Copy link
Contributor

@webfiltered webfiltered left a comment

Choose a reason for hiding this comment

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

With tests, even! Thank you. 😄

Copy link
Member

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

Can we collapse the sidebar before taking the screenshot?

Also I would like a more minimal reproduction for the example workflow used for this test to be more isolated to other features.

@christian-byrne christian-byrne force-pushed the bugfix/reroute-node-load branch from de3a8b0 to b6463b5 Compare December 22, 2024 23:23
@christian-byrne christian-byrne added New Browser Test Expectations New browser test screenshot should be set by github action and removed New Browser Test Expectations New browser test screenshot should be set by github action labels Dec 22, 2024
@huchenlei huchenlei added New Browser Test Expectations New browser test screenshot should be set by github action and removed New Browser Test Expectations New browser test screenshot should be set by github action labels Dec 23, 2024
@huchenlei huchenlei merged commit feeb1d1 into main Dec 23, 2024
2 checks passed
@huchenlei huchenlei deleted the bugfix/reroute-node-load branch December 23, 2024 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Browser Test Expectations New browser test screenshot should be set by github action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: A workflow cannot be inserted if there are reroutes in it
3 participants