feat(editor): Change default node names depending on node operation and resource #15954
Conversation
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
n8n-assistant
bot
added
core
CharlieKolb
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic found 3 issues across 4 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
packages/frontend/editor-ui/src/components/Node/NodeCreator/composables/useActions.ts
Outdated
Show resolved
Hide resolved
packages/frontend/editor-ui/src/components/Node/NodeCreator/composables/useActions.ts
Outdated
Show resolved
Hide resolved
|
|
||
| export const useActions = () => { | ||
| const nodeCreatorStore = useNodeCreatorStore(); | ||
| const nodeTypesStore = useNodeTypesStore(); | ||
| const i18n = useI18n(); | ||
| const canvasOperations = useCanvasOperations({ router: useRouter() }); |
There was a problem hiding this comment.
Creating router instance inside a composable may lead to multiple router instances. The router should be injected from the parent component.
| const canvasOperations = useCanvasOperations({ router: useRouter() }); | |
| const canvasOperations = useCanvasOperations({ router }); |
CharlieKolb
requested review from
a team,
nikhilkuria and
schrothbn
and removed request for
a team and
RicardoE105
|
@schrothbn I moved the |
packages/core/src/execution-engine/partial-execution-utils/is-tool.ts
Outdated
Show resolved
Hide resolved
| parameters: INodeParameters, | ||
| ): boolean { | ||
| // Check if node is a vector store in retrieve-as-tool mode | ||
| if (nodeTypeDescription.name.includes('vectorStore')) { |
There was a problem hiding this comment.
[Question] is the implementation here is different from what we had at is-tool.ts ?
There was a problem hiding this comment.
Good eye! node.type is equivalent to nodeTypeDescription.name, and this makes for a more convenient API since we already need the description anyway. But definitely want @schrothbn eye on this as well!
|
|
|
|
|
|
|
|
|
|
|
|
|
|
n8n
|
||||||||||||||||||||||||||||||||||
| Project |
n8n
|
| Branch Review |
ADO-3437
|
| Run status |
|
| Run duration | 04m 17s |
| Commit |
|
| Committer | Charlie Kolb |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
40
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
e2e/5-ndv.cy.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| NDV > should show validation errors only after blur or re-opening of NDV of nodes without operation and resource |
Screenshots
Video
|
|
Summary
Replace the current default names for nodes with dynamic ones based on operations and/or resources if available.
For tools also append the node name since they will be provided to the AI Agent.
Also moved an
isToolimplementation to NodeHelpers since it seems like the best one I've seen so far.Not really happy with the implementation in
useActionsbut I can't think of better alternatives :/Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/ADO-3437/feature-auto-name-nodes
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)