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

feat(editor): Add Info Note to NDV Output Panel if no existing Tools were used during Execution #11672

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

CharlieKolb
Copy link
Contributor

@CharlieKolb CharlieKolb commented Nov 11, 2024

Summary

Add an info notice to the output panel if the AI Node had tools available but used none.

With calculator tool:

image image

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/ADO-2753/feature-add-a-warning-when-no-tool-is-called

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Nov 11, 2024
@CharlieKolb CharlieKolb marked this pull request as ready for review November 11, 2024 08:34
@CharlieKolb CharlieKolb changed the title feat(editor): Add Info Panel to NDV Output Panel if no existing Tools were used during Execution feat(editor): Add Info Note to NDV Output Panel if no existing Tools were used during Execution Nov 11, 2024
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 26.08696% with 17 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/editor-ui/src/components/OutputPanel.vue 5.55% 17 Missing ⚠️

📢 Thoughts on this report? Let us know!

@@ -1360,6 +1360,9 @@ defineExpose({ enterEditMode });
<N8nText v-n8n-html="hint.message" size="small"></N8nText>
</N8nCallout>

<div v-if="hasNodeRun">
<slot name="panel-callout-info"></slot>
Copy link
Contributor

Choose a reason for hiding this comment

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

The slot here allows us to inject components for different panels, but the callout here could be reused for different features and panels. Because we might want to reuse this in the future, it would be best to add the Callout component here and the message as a prop.
Also this would get rid of the unnecessary empty div in the test/render.

<N8nCallout v-if="props.calloutMessageKey" "props.:key="calloutMessageKey" />

Comment on lines 469 to 471
padding-left: var(--spacing-s);
padding-right: var(--spacing-s);
padding-bottom: var(--spacing-xs);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit/optional - could use padding:

:key="props.calloutMessageKey"
:class="$style.callout"
theme="secondary"
data-test-id="no-tools-used-callout"
Copy link
Contributor

Choose a reason for hiding this comment

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

test-id needs to be more generic now.

mutdmour
mutdmour previously approved these changes Nov 14, 2024
Copy link

cypress bot commented Nov 14, 2024

n8n    Run #7870

Run Properties:  status check passed Passed #7870  •  git commit a6445ade5c: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 CharlieKolb 🗃️ e2e/*
Project n8n
Branch Review ADO-2753/feature-add-a-warning-when-no-tool-is-called
Run status status check passed Passed #7870
Run duration 04m 23s
Commit git commit a6445ade5c: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 CharlieKolb 🗃️ e2e/*
Committer Charlie Kolb
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 468
View all changes introduced in this branch ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

@CharlieKolb CharlieKolb marked this pull request as draft November 14, 2024 13:00
@CharlieKolb CharlieKolb marked this pull request as ready for review November 14, 2024 13:52
const allToolsWereUnusedNotice = computed(() => {
if (!node.value || runsCount.value === 0) return undefined;

const toolsAvailable = props.workflow.getParentNodes(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to get parents from props.runIndex here? Would this track between runs or could this use current parents (and thus tools) with an outdated runIndex if pinned or selecting between multiple executions of the node?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants