fix(editor): Hide fromAI button in old workflow tool #13552
Conversation
n8n-assistant
bot
added
n8n team
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
| @@ -44,7 +45,8 @@ function sanitizeFromAiParameterName(s: string) { | |||
| return s; | |||
| } | |||
|
|
|||
| const NODE_DENYLIST = ['toolCode', 'toolHttpRequest']; | |||
| // nodeName | [nodeName, highestUnsupportedVersion] | |||
| const NODE_DENYLIST = ['toolCode', 'toolHttpRequest', ['toolWorkflow', 1.2]] as const; | |||
There was a problem hiding this comment.
really would have loved to see this at the node description level instead of the UI. Sorry if I am repeating a conversation we already have.
What's blocking us from refactoring this to be more generic like that?
There was a problem hiding this comment.
I don't think there's significant upside here, it's not like we're gonna keep growing this list in the future. This solution works and doesn't introduce more permanent configuration flags on our node descriptions.
For handling the workflow tool for example we'd need realistically want to support the I guess the versioned node saves us here. Begs the question if this applies in general though@version displayOptions syntax for a "proper" implementation.
Otherwise we'd end up with maxVersionWithFromAIUnsupported or similar?
In contrast we might well end up with a flag on node properties if we happen upon use cases where expressions make sense but fromAI is disallowed for whatever reason, since in that case we can expect that we'll keep adding more in the future.
There was a problem hiding this comment.
It seems like we are already adding more nodes and might continue.
Like you suggested, we could solve this in a generic way. Maybe similar to display options.
hideFromAIButton: { lte: 1.5 } might work.
This could be a lightweight implementation, it does not need to support all possible variations of this.
There was a problem hiding this comment.
There's a very finite amount of tools that don't support fromAI, and we don't intend to produce more. This seems like a lot of effort for little practical upside 😅
There was a problem hiding this comment.
I feel it would be cleaner and more generic and better in touch with how n8n is supposed to be. So I guess it's more code hygiene for me here.
It's your call though.
|
|
n8n
|
||||||||||||||||||||||||||||
| Project |
n8n
|
| Branch Review |
ADO-3282
|
| Run status |
|
| Run duration | 05m 52s |
| Commit |
|
| Committer | Charlie Kolb |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
3
|
Flaky
|
0
|
Pending
|
4
|
Skipped
|
0
|
Passing
|
437
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
e2e/1-workflows.cy.ts • 3 failed tests
Summary
We show the button here even though it won't work.
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/ADO-3282/bug-fromai-button-displays-on-old-workflow-tool
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)