mirror of
https://github.com/n8n-io/n8n.git
synced 2026-10-17 05:29:44 +02:00
fix(editor): Keep the builder skill modal open on a taken name (no-changelog) (#41227)
This commit is contained in:
parent
1a6b6a65d4
commit
f553331100
|
|
@ -117,6 +117,7 @@ function renderModal({
|
|||
| 'onRemove'
|
||||
| 'outdated'
|
||||
| 'onUseLatestVersion'
|
||||
| 'getDefaultName'
|
||||
> = {}) {
|
||||
const renderComponent = createComponentRenderer(AgentSkillModal, {
|
||||
global: {
|
||||
|
|
@ -725,6 +726,23 @@ describe('AgentSkillModal', () => {
|
|||
expect(getByTestId('agent-skill-picker')).toBeVisible();
|
||||
});
|
||||
|
||||
it('asks for a default name where "available to" puts the new skill', async () => {
|
||||
listAttachableSkills.mockResolvedValue([]);
|
||||
const getDefaultName = vi.fn().mockResolvedValue('New skill 3');
|
||||
const { getByTestId } = renderModal({
|
||||
hubSkillsEnabled: true,
|
||||
onAttach: vi.fn(),
|
||||
getDefaultName,
|
||||
});
|
||||
|
||||
// "Create" from the "This agent" tab makes the skill available to this agent only.
|
||||
await fireEvent.click(getByTestId('agent-skill-picker-create-this-agent'));
|
||||
await fireEvent.click(getByTestId('agent-skill-add-manually'));
|
||||
|
||||
expect(getDefaultName).toHaveBeenCalledWith('agent');
|
||||
await waitFor(() => expect(getTitleInput(document.body)).toHaveValue('New skill 3'));
|
||||
});
|
||||
|
||||
it('opens a read-only preview from a row click and adds the skill from there', async () => {
|
||||
listAttachableSkills.mockResolvedValue([projectSkill, instanceSkill]);
|
||||
getHubSkill.mockResolvedValue(
|
||||
|
|
|
|||
|
|
@ -8,8 +8,9 @@ import { useUIStore } from '@/app/stores/ui.store';
|
|||
import { useSettingsStore } from '@n8n/stores/settings.store';
|
||||
import { AGENT_TOOLS_MODAL_KEY, AGENT_SKILL_MODAL_KEY } from '../constants';
|
||||
import { useAgentCapabilitiesActions } from '../composables/useAgentCapabilitiesActions';
|
||||
import { createAgentSkill, getHubSkill } from '../composables/useAgentApi';
|
||||
import type { SkillDetail, SkillListItem } from '@n8n/api-types';
|
||||
import { createAgentSkill, getHubSkill, getNewSkillName } from '../composables/useAgentApi';
|
||||
import { SKILL_NAME_TAKEN_MESSAGE, type SkillDetail, type SkillListItem } from '@n8n/api-types';
|
||||
import { ResponseError } from '@n8n/rest-api-client';
|
||||
import type {
|
||||
AgentJsonConfig,
|
||||
AgentJsonMcpServerConfig,
|
||||
|
|
@ -37,6 +38,7 @@ vi.mock('@n8n/composables/useToast', () => ({
|
|||
vi.mock('../composables/useAgentApi', () => ({
|
||||
createAgentSkill: vi.fn(),
|
||||
getHubSkill: vi.fn(),
|
||||
getNewSkillName: vi.fn(),
|
||||
}));
|
||||
|
||||
const { hasScopeSpy } = vi.hoisted(() => ({
|
||||
|
|
@ -177,11 +179,12 @@ type SkillModalData = {
|
|||
onUseLatestVersion?: (skillId: string) => void;
|
||||
getAttachedSkills?: () => Array<{ id: string; name: string }>;
|
||||
canCreateInstanceSkills?: boolean;
|
||||
getDefaultName?: (availableTo: string) => Promise<string>;
|
||||
onConfirm: (payload: {
|
||||
id?: string;
|
||||
skill: AgentSkill;
|
||||
availableTo?: 'project' | 'agent' | 'instance';
|
||||
}) => void;
|
||||
}) => unknown;
|
||||
onAttach?: (skill: SkillListItem) => Promise<void>;
|
||||
onRemove?: (skillId: string) => void;
|
||||
};
|
||||
|
|
@ -244,6 +247,12 @@ describe('useAgentCapabilitiesActions — localSkills host seam', () => {
|
|||
const persisted = new Promise<void>((resolve) => {
|
||||
releasePersist = resolve;
|
||||
});
|
||||
vi.mocked(createAgentSkill).mockResolvedValue({
|
||||
id: 'skill_new',
|
||||
skill: { ...triage, name: 'New Skill' },
|
||||
skillHash: 'hash-new',
|
||||
versionId: 'v2',
|
||||
});
|
||||
const { uiStore, actions } = setup({ ensureAgentPersisted: async () => await persisted });
|
||||
|
||||
actions.onOpenAddSkillModal();
|
||||
|
|
@ -730,6 +739,50 @@ describe('useAgentCapabilitiesActions — skills hub', () => {
|
|||
expect(hasScopeSpy).toHaveBeenCalledWith('skill:create');
|
||||
});
|
||||
|
||||
it.each([
|
||||
['project', { scope: 'project', projectId: 'project-1' }],
|
||||
['agent', { scope: 'agent', agentId: 'agent-1' }],
|
||||
['instance', { scope: 'instance' }],
|
||||
])(
|
||||
'asks the server for a default name where "available to" %s puts the skill',
|
||||
async (availableTo, target) => {
|
||||
vi.mocked(getNewSkillName).mockResolvedValue('New skill 3');
|
||||
const { modalData } = setupHub();
|
||||
|
||||
await expect(modalData.getDefaultName?.(availableTo)).resolves.toBe('New skill 3');
|
||||
expect(getNewSkillName).toHaveBeenCalledWith(expect.anything(), {
|
||||
...target,
|
||||
name: 'agents.builder.skills.defaultName',
|
||||
});
|
||||
},
|
||||
);
|
||||
|
||||
it('keeps the modal open with the error on the name when the name is taken', async () => {
|
||||
vi.mocked(createAgentSkill).mockRejectedValue(
|
||||
new ResponseError(SKILL_NAME_TAKEN_MESSAGE, { httpStatusCode: 409 }),
|
||||
);
|
||||
const { modalData } = setupHub();
|
||||
|
||||
const result = await modalData.onConfirm({ skill: sqlHelper, availableTo: 'project' });
|
||||
|
||||
expect(result).toEqual({
|
||||
saved: false,
|
||||
nameError: 'settings.context.skills.error.nameTaken',
|
||||
});
|
||||
expect(showErrorSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('keeps the modal open and reports any other failed create', async () => {
|
||||
const error = new Error('Network down');
|
||||
vi.mocked(createAgentSkill).mockRejectedValue(error);
|
||||
const { modalData } = setupHub();
|
||||
|
||||
const result = await modalData.onConfirm({ skill: sqlHelper, availableTo: 'project' });
|
||||
|
||||
expect(result).toEqual({ saved: false });
|
||||
expect(showErrorSpy).toHaveBeenCalledWith(error, 'agents.builder.skills.create.error');
|
||||
});
|
||||
|
||||
it('keeps the picker off and creates the skill on the agent when the skills hub is off', async () => {
|
||||
vi.mocked(createAgentSkill).mockResolvedValue({
|
||||
id: 'sql',
|
||||
|
|
@ -740,6 +793,8 @@ describe('useAgentCapabilitiesActions — skills hub', () => {
|
|||
const { modalData, actions, uiStore } = setupHub({ skillsHubEnabled: false });
|
||||
|
||||
expect(modalData.hubSkillsEnabled).toBe(false);
|
||||
// Without the hub, names are only unique per agent, so the modal numbers them itself.
|
||||
expect(modalData.getDefaultName).toBeUndefined();
|
||||
modalData.onConfirm({ skill: sqlHelper, availableTo: 'project' });
|
||||
await flushPromises();
|
||||
expect(createAgentSkill).toHaveBeenCalledWith(
|
||||
|
|
|
|||
|
|
@ -76,8 +76,9 @@ export type AgentSkillModalData = {
|
|||
scopeOptions?: AgentSkillModalScopeOption[];
|
||||
initialScope?: string;
|
||||
/**
|
||||
* Settings only: a default name that no skill in the scope has yet, as new workflows
|
||||
* get one. Called with the chosen scope value when the user starts a skill by hand.
|
||||
* A default name that no skill in the scope has yet, as new workflows get one. Called
|
||||
* when the user starts a skill by hand, with the Settings scope value or, in the agent
|
||||
* builder, the "available to" value.
|
||||
*/
|
||||
getDefaultName?: (scope: string) => Promise<string>;
|
||||
/** Locks the scope field and explains why in a tooltip. */
|
||||
|
|
@ -162,13 +163,13 @@ async function refreshDefaultName() {
|
|||
const current = skill.value.name;
|
||||
if (current !== getDefaultSkillName() && current !== serverDefaultName.value) return;
|
||||
const request = ++latestNameRequest;
|
||||
const requestedScope = scope.value;
|
||||
const requestedScope = nameScope.value;
|
||||
defaultNameLoading.value = true;
|
||||
try {
|
||||
const name = await props.data.getDefaultName(requestedScope);
|
||||
// Only the newest answer for the scope still selected counts, and only while the
|
||||
// user has not typed a name of their own.
|
||||
if (request !== latestNameRequest || scope.value !== requestedScope) return;
|
||||
if (request !== latestNameRequest || nameScope.value !== requestedScope) return;
|
||||
if (skill.value.name !== current) return;
|
||||
serverDefaultName.value = name;
|
||||
skill.value = { ...skill.value, name };
|
||||
|
|
@ -184,10 +185,6 @@ function onAddManually() {
|
|||
void refreshDefaultName();
|
||||
}
|
||||
|
||||
// A new scope can hold other names, so a default name follows the scope.
|
||||
watch(scope, () => {
|
||||
if (step.value === 'manual') void refreshDefaultName();
|
||||
});
|
||||
const showScope = computed(() => !!props.data.scopeOptions?.length && step.value !== 'preview');
|
||||
const canChangeScope = computed(
|
||||
() => showScope.value && !props.data.scopeLockedReason && !props.data.readOnly,
|
||||
|
|
@ -196,6 +193,15 @@ const selectedScopeIcon = computed(
|
|||
() => props.data.scopeOptions?.find((option) => option.value === scope.value)?.icon,
|
||||
);
|
||||
const availableTo = ref<AgentSkillAvailability>('project');
|
||||
/**
|
||||
* Where the new skill goes, for its default name: the scope field in Settings, or
|
||||
* "available to" in the agent builder. `getDefaultName` gets this value.
|
||||
*/
|
||||
const nameScope = computed<string>(() => (showScope.value ? scope.value : availableTo.value));
|
||||
// A new scope can hold other names, so a default name follows the scope.
|
||||
watch(nameScope, () => {
|
||||
if (step.value === 'manual') void refreshDefaultName();
|
||||
});
|
||||
|
||||
const librarySkills = ref<SkillListItem[]>([]);
|
||||
const libraryLoading = ref(false);
|
||||
|
|
|
|||
|
|
@ -37,6 +37,8 @@ import type {
|
|||
SkillDetail,
|
||||
SkillListItem,
|
||||
SkillListResponse,
|
||||
SkillNewNameQueryDto,
|
||||
SkillNewNameResponse,
|
||||
VectorStoreTestResult,
|
||||
} from '@n8n/api-types';
|
||||
import { getFullApiResponse, makeRestApiRequest, request } from '@n8n/rest-api-client';
|
||||
|
|
@ -755,6 +757,20 @@ export const getHubSkill = async (
|
|||
return await makeRestApiRequest<SkillDetail>(context, 'GET', `/skills/${skillId}`);
|
||||
};
|
||||
|
||||
/** A name like `name` that no skill in the scope has yet, such as "New skill 2". */
|
||||
export const getNewSkillName = async (
|
||||
context: IRestApiContext,
|
||||
query: SkillNewNameQueryDto,
|
||||
): Promise<string> => {
|
||||
const { name } = await makeRestApiRequest<SkillNewNameResponse>(
|
||||
context,
|
||||
'GET',
|
||||
'/skills/new-name',
|
||||
query,
|
||||
);
|
||||
return name;
|
||||
};
|
||||
|
||||
export const getAgentBackgroundJobs = async (
|
||||
context: IRestApiContext,
|
||||
projectId: string,
|
||||
|
|
|
|||
|
|
@ -10,11 +10,12 @@ import {
|
|||
type AgentSkillAvailability,
|
||||
type SkillDetail,
|
||||
type SkillListItem,
|
||||
type SkillNewNameQueryDto,
|
||||
} from '@n8n/api-types';
|
||||
import { useUIStore } from '@/app/stores/ui.store';
|
||||
import { useNodeTypesStore } from '@/app/stores/nodeTypes.store';
|
||||
import { AI_MCP_TOOL_NODE_TYPE } from '@/app/constants/nodeTypes';
|
||||
import { createAgentSkill, getHubSkill } from './useAgentApi';
|
||||
import { createAgentSkill, getHubSkill, getNewSkillName } from './useAgentApi';
|
||||
import { useAgentProjectBreadcrumb } from './useAgentProjectBreadcrumb';
|
||||
import { mcpServerToNode } from './useMcpServerAdapter';
|
||||
import {
|
||||
|
|
@ -22,8 +23,9 @@ import {
|
|||
AGENT_TOOL_CONFIG_MODAL_KEY,
|
||||
AGENT_SKILL_MODAL_KEY,
|
||||
} from '../constants';
|
||||
import { normalizeAgentSkillForSave } from '../utils/agentSkill';
|
||||
import { isSkillNameTakenError, normalizeAgentSkillForSave } from '../utils/agentSkill';
|
||||
import type { ToolOpenTarget, ToolPickerMode } from '../components/AgentCapabilitiesSection.types';
|
||||
import type { AgentSkillConfirmResult } from '../components/AgentSkillModal.vue';
|
||||
import type {
|
||||
AgentResource,
|
||||
AgentJsonConfig,
|
||||
|
|
@ -410,6 +412,17 @@ export function useAgentCapabilitiesActions(deps: UseAgentCapabilitiesActionsDep
|
|||
* Authoring-time mirror of the backend's `assertSkillNameIsUnique` for
|
||||
* local-skill hosts. The modal validates against `existingSkillNames` before closing
|
||||
*/
|
||||
/** The scope of the new-name lookup for an "available to" choice. */
|
||||
function newNameTarget(
|
||||
availableTo: string,
|
||||
targetProjectId: string,
|
||||
targetAgentId: string,
|
||||
): Pick<SkillNewNameQueryDto, 'scope' | 'projectId' | 'agentId'> {
|
||||
if (availableTo === 'instance') return { scope: 'instance' };
|
||||
if (availableTo === 'agent') return { scope: 'agent', agentId: targetAgentId };
|
||||
return { scope: 'project', projectId: targetProjectId };
|
||||
}
|
||||
|
||||
function hasDuplicateSkillName(name: string, excludeId?: string): boolean {
|
||||
const key = skillNameKey(name);
|
||||
return appliedSkillNames(excludeId).some((existing) => skillNameKey(existing) === key);
|
||||
|
|
@ -484,6 +497,17 @@ export function useAgentCapabilitiesActions(deps: UseAgentCapabilitiesActionsDep
|
|||
appliedSkills.value.map(({ id, skill }) => ({ id, name: skill.name })),
|
||||
projectLabel: projectName.value ?? undefined,
|
||||
canCreateInstanceSkills: rbacStore.hasScope('skill:create'),
|
||||
// The hub refuses a name that the skill's scope already has, so the default
|
||||
// name comes from the server, for the "available to" choice.
|
||||
...(hubSkillsEnabled.value
|
||||
? {
|
||||
getDefaultName: async (availableTo: string) =>
|
||||
await getNewSkillName(rootStore.restApiContext, {
|
||||
...newNameTarget(availableTo, targetProjectId, targetAgentId),
|
||||
name: locale.baseText('agents.builder.skills.defaultName'),
|
||||
}),
|
||||
}
|
||||
: {}),
|
||||
onRemove: (skillId: string) => {
|
||||
if (!isCurrentTarget()) return;
|
||||
onRemoveSkill(skillId);
|
||||
|
|
@ -493,59 +517,63 @@ export function useAgentCapabilitiesActions(deps: UseAgentCapabilitiesActionsDep
|
|||
isCurrentTarget,
|
||||
async () => await getHubSkill(rootStore.restApiContext, librarySkill.id),
|
||||
),
|
||||
onConfirm: ({
|
||||
// The modal stays open until the skill is created, and stays open when the create
|
||||
// fails, so the user keeps what they typed. A taken name shows on the name field.
|
||||
onConfirm: async ({
|
||||
skill,
|
||||
availableTo,
|
||||
}: {
|
||||
id?: string;
|
||||
skill: AgentSkill;
|
||||
availableTo?: AgentSkillAvailability;
|
||||
}) => {
|
||||
}): Promise<AgentSkillConfirmResult> => {
|
||||
if (localSkills) {
|
||||
if (!isCurrentTarget()) return;
|
||||
if (!isCurrentTarget()) return undefined;
|
||||
const sanitizedSkill = normalizeAgentSkillForSave(skill);
|
||||
if (hasDuplicateSkillName(sanitizedSkill.name)) {
|
||||
showDuplicateSkillNameError(sanitizedSkill.name);
|
||||
return;
|
||||
return undefined;
|
||||
}
|
||||
|
||||
// The host mints the skill id and writes body + ref together.
|
||||
localSkills.createSkill(sanitizedSkill);
|
||||
return;
|
||||
return undefined;
|
||||
}
|
||||
|
||||
const sanitizedSkill = normalizeAgentSkillForSave(skill);
|
||||
try {
|
||||
await beforeAgentMutation?.();
|
||||
} catch {
|
||||
// The host owns the autosave error message. Do not also report
|
||||
// this as a skill-creation failure.
|
||||
return { saved: false };
|
||||
}
|
||||
if (!isCurrentTarget()) return undefined;
|
||||
let result: Awaited<ReturnType<typeof createAgentSkill>>;
|
||||
try {
|
||||
await ensureAgentPersisted?.();
|
||||
if (!isCurrentTarget()) return undefined;
|
||||
result = await createAgentSkill(
|
||||
rootStore.restApiContext,
|
||||
targetProjectId,
|
||||
targetAgentId,
|
||||
sanitizedSkill,
|
||||
hubSkillsEnabled.value ? availableTo : undefined,
|
||||
);
|
||||
} catch (error) {
|
||||
if (isSkillNameTakenError(error)) {
|
||||
return {
|
||||
saved: false,
|
||||
nameError: locale.baseText('settings.context.skills.error.nameTaken'),
|
||||
};
|
||||
}
|
||||
showError(error, locale.baseText('agents.builder.skills.create.error'));
|
||||
return { saved: false };
|
||||
}
|
||||
|
||||
// The skill exists now, so the modal can close while the agent catches up.
|
||||
void (async () => {
|
||||
const sanitizedSkill = normalizeAgentSkillForSave(skill);
|
||||
let created: AgentSkill;
|
||||
let skillHash: string;
|
||||
let versionId: string | null;
|
||||
let skillId: string;
|
||||
try {
|
||||
await beforeAgentMutation?.();
|
||||
} catch {
|
||||
// The host owns the autosave error message. Do not also report
|
||||
// this as a skill-creation failure.
|
||||
return;
|
||||
}
|
||||
if (!isCurrentTarget()) return;
|
||||
try {
|
||||
await ensureAgentPersisted?.();
|
||||
if (!isCurrentTarget()) return;
|
||||
const result = await createAgentSkill(
|
||||
rootStore.restApiContext,
|
||||
targetProjectId,
|
||||
targetAgentId,
|
||||
sanitizedSkill,
|
||||
hubSkillsEnabled.value ? availableTo : undefined,
|
||||
);
|
||||
skillId = result.id;
|
||||
created = result.skill;
|
||||
skillHash = result.skillHash;
|
||||
versionId = result.versionId;
|
||||
} catch (error) {
|
||||
showError(error, locale.baseText('agents.builder.skills.create.error'));
|
||||
return;
|
||||
}
|
||||
const { id: skillId, skill: created, skillHash, versionId } = result;
|
||||
if (!isCurrentTarget() || agent.value?.id !== targetAgentId) return;
|
||||
agent.value = {
|
||||
...agent.value,
|
||||
|
|
@ -577,6 +605,7 @@ export function useAgentCapabilitiesActions(deps: UseAgentCapabilitiesActionsDep
|
|||
};
|
||||
}
|
||||
})();
|
||||
return undefined;
|
||||
},
|
||||
},
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,5 +1,17 @@
|
|||
import { SKILL_NAME_TAKEN_MESSAGE } from '@n8n/api-types';
|
||||
import { ResponseError } from '@n8n/rest-api-client';
|
||||
|
||||
import type { AgentSkill } from '../types';
|
||||
|
||||
/** Whether a save failed because the skill's scope already has a name like it. */
|
||||
export function isSkillNameTakenError(error: unknown): boolean {
|
||||
return (
|
||||
error instanceof ResponseError &&
|
||||
error.httpStatusCode === 409 &&
|
||||
error.message === SKILL_NAME_TAKEN_MESSAGE
|
||||
);
|
||||
}
|
||||
|
||||
/** Skills drop `allowedTools`: the editor does not show it and the runtime does not get it. */
|
||||
export function normalizeAgentSkillForSave(skill: AgentSkill): AgentSkill {
|
||||
const { allowedTools: _allowedTools, ...rest } = skill;
|
||||
|
|
|
|||
|
|
@ -2,14 +2,8 @@
|
|||
import { computed, onMounted, ref } from 'vue';
|
||||
import { useRoute, useRouter } from 'vue-router';
|
||||
import { useDebounceFn } from '@vueuse/core';
|
||||
import {
|
||||
SKILL_NAME_TAKEN_MESSAGE,
|
||||
type AgentSkill,
|
||||
type SkillListItem,
|
||||
type SkillScope,
|
||||
} from '@n8n/api-types';
|
||||
import type { AgentSkill, SkillListItem, SkillScope } from '@n8n/api-types';
|
||||
import { useI18n } from '@n8n/i18n';
|
||||
import { ResponseError } from '@n8n/rest-api-client';
|
||||
import { useToast } from '@n8n/composables/useToast';
|
||||
import { getDebounceTime } from '@n8n/composables/useDebounce';
|
||||
import {
|
||||
|
|
@ -31,6 +25,7 @@ import { DEBOUNCE_TIME, MODAL_CONFIRM, VIEWS } from '@/app/constants';
|
|||
import { useUIStore } from '@/app/stores/ui.store';
|
||||
import { useProjectsStore } from '@/features/collaboration/projects/projects.store';
|
||||
import { AGENT_SKILL_MODAL_KEY } from '@/features/agents/constants';
|
||||
import { isSkillNameTakenError } from '@/features/agents/utils/agentSkill';
|
||||
import type {
|
||||
AgentSkillConfirmFailure,
|
||||
AgentSkillModalData,
|
||||
|
|
@ -221,11 +216,7 @@ function confirmIfHidden(skillId: string, key: 'created' | 'saved') {
|
|||
* shows on the name field, any other error in a toast.
|
||||
*/
|
||||
function saveFailed(error: unknown): AgentSkillConfirmFailure {
|
||||
if (
|
||||
error instanceof ResponseError &&
|
||||
error.httpStatusCode === 409 &&
|
||||
error.message === SKILL_NAME_TAKEN_MESSAGE
|
||||
) {
|
||||
if (isSkillNameTakenError(error)) {
|
||||
return { saved: false, nameError: i18n.baseText('settings.context.skills.error.nameTaken') };
|
||||
}
|
||||
showError(error, i18n.baseText('settings.context.skills.error.save'));
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user