-
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): Improve how we show default Agent prompt and Memory session parameters #11491
Conversation
n8n Run #7825
Run Properties:
|
Project |
n8n
|
Branch Review |
ai-372-no-prompt-specified-text_disabled
|
Run status |
Passed #7825
|
Run duration | 04m 19s |
Commit |
d49767b093: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 OlegIvaniv 🗃️ e2e/*
|
Committer | Oleg Ivaniv |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
1
|
Pending |
2
|
Skipped |
0
|
Passing |
466
|
View all changes introduced in this branch ↗︎ |
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.
The code looks good and the feature looks even better!
I could see some users not immediately getting that/why the field is disabled, having a tooltip on hover that points calls out the controlling field or a tag next to the parameter name might be a good follow up item.
Having a visible hierarchy with indentation would also help.
Please do get another opinion as well since I'm new and mostly unfamiliar with existing best practices, as my comments will prove :D
@@ -315,7 +315,7 @@ export class ChainLlm implements INodeType { | |||
}, | |||
}, | |||
{ | |||
displayName: 'Prompt', | |||
displayName: 'Prompt Source', |
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 this take from packages/@n8n/nodes-langchain/utils/descriptions.ts
? That one still has Prompt
rather than Prompt Source
, as an aside. (Prompt Source is much better 👍 )
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.
Good point, I updated all of them to use promptTypeOptions
from the shared utils file
Appreciate the review, @CharlieKolb! I updated the PR to use shared |
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
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.
Thanks for the example workflow, that's great :D
PR looks good to go from my side 👍
✅ All Cypress E2E specs passed |
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.
LGTM! 🎉
Nice work, I like how you kept disabledOptions
simple by reusing the displayOptions
format. Maybe some of that code should be renamed in the future.
show
and hide
don't make much sense in disabledOptions. It's becoming a more general "Condition" object instead of something specific for hiding parameters.
✅ All Cypress E2E specs passed |
Thanks! |
Got released with |
…sion parameters (#11491)
Summary
Currently, our AI nodes default to using expressions like
{{ $json.chatInput }}
and{{ $json.sessionId }}
for certain fields. However, this implementation causes confusion to the new users.This PR improves the UX by:
promptType
isauto
)disabledOptions
property that follows the same logic asdisplayOptions
but disables fields instead of hiding themChanges
disabledOptions
interface parallel todisplayOptions
for node propertiesCleanShot.2024-10-31.at.16.12.03.mp4
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)