-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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(editor): Restrict when a ChatTrigger Node is added automatically #11523
feat(editor): Restrict when a ChatTrigger Node is added automatically #11523
Conversation
…ange-auto-add-of-chattrigger
…ange-auto-add-of-chattrigger
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
…ps://github.com/n8n-io/n8n into ADO-2728/feature-change-auto-add-of-chattrigger
…ange-auto-add-of-chattrigger
…ange-auto-add-of-chattrigger
85ef434
to
4538520
Compare
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.
Looks good. Added one comment
const { getByNameAndVersion } = getNodeTypes(); | ||
|
||
// We want to add a trigger if there are no triggers other than Manual Triggers | ||
const shouldAddChatTrigger = allNodes.every((node) => { |
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.
I am a bit worried about the performance of every
for large workflows. Can we use some
or something similar that would stop as soon as it finds the trigger that is not the manual trigger?
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.
I had the same concern, which is why I moved isCompatibleNode
up before the chatTrigger
logic. 👍 This way this will only be run on adding compatible nodes.
every
will also stop as soon as it hits the first false, so as soon as we encounter a trigger other than MANUAL_TRIGGER
. (Though I appreciate larger workflows will scale non-triggers more than triggers usually)
getByNameAndVersion
ends up calling a computed property, so we should reuse the one call to the backend to get node types anyway if I understand the code correctly.
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.
Should I add a comment in the code acknowledging 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.
Sure, thanks for clarification
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.
Looks good, thanks for adding tests! 🙌🏻
✅ All Cypress E2E specs passed |
Got released with |
Summary
Tighten the restrictions on when we add a ChatTrigger node for AI Agents or similar.
Screen.Recording.2024-11-08.at.09.31.26.mov
Related Linear tickets, Github issues, and Community forum posts
Part 1 of https://linear.app/n8n/issue/ADO-2728/feature-change-auto-add-of-chattrigger
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)