From bbd15de01f09484c237bc7bae784f1fc0c4f660d Mon Sep 17 00:00:00 2001 From: "n8n-assistant[bot]" <100856346+n8n-assistant[bot]@users.noreply.github.com> Date: Sun, 19 Jul 2026 14:33:14 +0000 Subject: [PATCH] fix(editor): Auto-ungroup invalid groups on workflow save (backport to release-candidate/2.30.x) (#34514) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Milorad FIlipović --- .../frontend/@n8n/i18n/src/locales/en.json | 2 + .../useInvalidNodeGroupCleanup.test.ts | 206 ++++++++++++++++++ .../composables/useInvalidNodeGroupCleanup.ts | 106 +++++++++ .../app/composables/useWorkflowSaving.test.ts | 109 ++++++++- .../src/app/composables/useWorkflowSaving.ts | 26 ++- 5 files changed, 441 insertions(+), 8 deletions(-) create mode 100644 packages/frontend/editor-ui/src/app/composables/useInvalidNodeGroupCleanup.test.ts create mode 100644 packages/frontend/editor-ui/src/app/composables/useInvalidNodeGroupCleanup.ts diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index c996c746011..660090a1282 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -4080,6 +4080,8 @@ "canvas.nodeGroup.connectionChangeBlocked.noContinuousPathFromRootToLeaf": "'{group}' must stay connected from first node to last node. You can make this change after ungrouping it.", "canvas.nodeGroup.autoExtended.title": "'{group}' extended", "canvas.nodeGroup.autoExtended.message": "Added '{node}' to keep '{group}' valid with the new connection.", + "canvas.nodeGroup.removedOnSave.title": "Groups removed", + "canvas.nodeGroup.removedOnSave.message": "The following groups are incompatible with your n8n version and have been removed: {groups}", "canvas.nodeGroup.titlePlaceholder": "Group name", "canvas.nodeGroup.expand": "Expand", "canvas.nodeGroup.collapse": "Collapse", diff --git a/packages/frontend/editor-ui/src/app/composables/useInvalidNodeGroupCleanup.test.ts b/packages/frontend/editor-ui/src/app/composables/useInvalidNodeGroupCleanup.test.ts new file mode 100644 index 00000000000..29b2bfa7d7d --- /dev/null +++ b/packages/frontend/editor-ui/src/app/composables/useInvalidNodeGroupCleanup.test.ts @@ -0,0 +1,206 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { createPinia, setActivePinia } from 'pinia'; +import type { IConnections, INode, IWorkflowGroup } from 'n8n-workflow'; +import { NodeConnectionTypes } from 'n8n-workflow'; + +import { useInvalidNodeGroupCleanup } from './useInvalidNodeGroupCleanup'; +import { + createWorkflowDocumentId, + useWorkflowDocumentStore, +} from '@/app/stores/workflowDocument.store'; +import { STICKY_NODE_TYPE } from '@/app/constants'; +import { createTestNode, createTestWorkflow } from '@/__tests__/mocks'; + +const trackSpy = vi.hoisted(() => vi.fn()); +const showMessageSpy = vi.hoisted(() => vi.fn()); + +vi.mock('@/app/composables/useTelemetry', () => ({ + useTelemetry: vi.fn(() => ({ track: trackSpy })), +})); + +vi.mock('@/app/composables/useToast', () => ({ + useToast: () => ({ showMessage: showMessageSpy }), +})); + +const WORKFLOW_ID = 'test-workflow'; + +function createConnection(from: string, to: string): IConnections { + return { + [from]: { + [NodeConnectionTypes.Main]: [[{ node: to, type: NodeConnectionTypes.Main, index: 0 }]], + }, + }; +} + +function setupDocumentStore({ + nodes, + connections = {}, + nodeGroups = [], +}: { + nodes: INode[]; + connections?: IConnections; + nodeGroups?: IWorkflowGroup[]; +}) { + const store = useWorkflowDocumentStore(createWorkflowDocumentId(WORKFLOW_ID)); + store.hydrate(createTestWorkflow({ id: WORKFLOW_ID, nodes, connections, nodeGroups })); + return store; +} + +describe('useInvalidNodeGroupCleanup', () => { + beforeEach(() => { + setActivePinia(createPinia()); + vi.clearAllMocks(); + }); + + it('keeps valid groups and shows no toast', () => { + const store = setupDocumentStore({ + nodes: [ + createTestNode({ id: 'node-a', name: 'Node A' }), + createTestNode({ id: 'node-b', name: 'Node B' }), + ], + connections: createConnection('Node A', 'Node B'), + nodeGroups: [{ id: 'group-1', name: 'Group 1', nodeIds: ['node-a', 'node-b'] }], + }); + + const { removeInvalidNodeGroups } = useInvalidNodeGroupCleanup(); + const removed = removeInvalidNodeGroups(store); + + expect(removed).toEqual([]); + expect(store.allGroups).toHaveLength(1); + expect(showMessageSpy).not.toHaveBeenCalled(); + expect(trackSpy).not.toHaveBeenCalled(); + }); + + it('removes a group whose members do not form a connected subgraph (sticky note member)', () => { + const store = setupDocumentStore({ + nodes: [ + createTestNode({ id: 'node-a', name: 'Node A' }), + createTestNode({ id: 'node-b', name: 'Node B' }), + createTestNode({ id: 'sticky', name: 'Sticky', type: STICKY_NODE_TYPE }), + ], + connections: createConnection('Node A', 'Node B'), + nodeGroups: [{ id: 'group-1', name: 'Group 1', nodeIds: ['node-a', 'node-b', 'sticky'] }], + }); + + const { removeInvalidNodeGroups } = useInvalidNodeGroupCleanup(); + const removed = removeInvalidNodeGroups(store); + + expect(removed.map((group) => group.id)).toEqual(['group-1']); + expect(store.allGroups).toHaveLength(0); + expect(showMessageSpy).toHaveBeenCalledTimes(1); + expect(showMessageSpy).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'warning', + title: 'Groups removed', + message: + 'The following groups are incompatible with your n8n version and have been removed: ', + }), + ); + expect(trackSpy).toHaveBeenCalledWith( + 'User ungrouped nodes', + expect.objectContaining({ + workflow_id: WORKFLOW_ID, + group_id: 'group-1', + group_title: 'Group 1', + node_ids: ['node-a', 'node-b', 'sticky'], + node_count: 3, + source: 'invalid-on-save', + }), + ); + expect(trackSpy).toHaveBeenCalledWith('Auto-ungrouped invalid node groups', { + groups_affected: 1, + }); + }); + + it('removes a group referencing a node that does not exist', () => { + const store = setupDocumentStore({ + nodes: [ + createTestNode({ id: 'node-a', name: 'Node A' }), + createTestNode({ id: 'node-b', name: 'Node B' }), + ], + connections: createConnection('Node A', 'Node B'), + nodeGroups: [ + { id: 'group-1', name: 'Group 1', nodeIds: ['node-a', 'node-b', 'does-not-exist'] }, + ], + }); + + const { removeInvalidNodeGroups } = useInvalidNodeGroupCleanup(); + const removed = removeInvalidNodeGroups(store); + + expect(removed.map((group) => group.id)).toEqual(['group-1']); + expect(store.allGroups).toHaveLength(0); + }); + + it('removes only invalid groups and lists them in a plural toast', () => { + const store = setupDocumentStore({ + nodes: [ + createTestNode({ id: 'node-a', name: 'Node A' }), + createTestNode({ id: 'node-b', name: 'Node B' }), + createTestNode({ id: 'node-c', name: 'Node C' }), + createTestNode({ id: 'node-d', name: 'Node D' }), + createTestNode({ id: 'node-e', name: 'Node E' }), + ], + connections: createConnection('Node A', 'Node B'), + nodeGroups: [ + { id: 'group-1', name: 'Group 1', nodeIds: ['node-a', 'node-b'] }, + // Members are not connected to each other + { id: 'group-2', name: 'Group 2', nodeIds: ['node-c', 'node-d'] }, + // References a missing node + { id: 'group-3', name: 'Group 3', nodeIds: ['node-e', 'does-not-exist'] }, + ], + }); + + const { removeInvalidNodeGroups } = useInvalidNodeGroupCleanup(); + const removed = removeInvalidNodeGroups(store); + + expect(removed.map((group) => group.id)).toEqual(['group-2', 'group-3']); + expect(store.allGroups.map((group) => group.id)).toEqual(['group-1']); + expect(showMessageSpy).toHaveBeenCalledTimes(1); + expect(showMessageSpy).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'warning', + title: 'Groups removed', + message: expect.stringContaining('
  • Group 2
  • Group 3
    • '), + }), + ); + // One 'User ungrouped nodes' per removed group, plus one aggregate event + expect(trackSpy).toHaveBeenCalledTimes(3); + expect(trackSpy).toHaveBeenCalledWith('Auto-ungrouped invalid node groups', { + groups_affected: 2, + }); + }); + + it('escapes HTML in group names listed in the toast', () => { + const store = setupDocumentStore({ + nodes: [ + createTestNode({ id: 'node-a', name: 'Node A' }), + createTestNode({ id: 'node-b', name: 'Node B' }), + ], + nodeGroups: [ + { id: 'group-1', name: '', nodeIds: ['node-a', 'node-b'] }, + ], + }); + + const { removeInvalidNodeGroups } = useInvalidNodeGroupCleanup(); + removeInvalidNodeGroups(store); + + expect(showMessageSpy).toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.stringContaining('
  • <img src=x onerror=alert(1)>
    • '), + }), + ); + }); + + it('does nothing when the workflow has no groups', () => { + const store = setupDocumentStore({ + nodes: [createTestNode({ id: 'node-a', name: 'Node A' })], + }); + + const { removeInvalidNodeGroups } = useInvalidNodeGroupCleanup(); + const removed = removeInvalidNodeGroups(store); + + expect(removed).toEqual([]); + expect(showMessageSpy).not.toHaveBeenCalled(); + expect(trackSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/frontend/editor-ui/src/app/composables/useInvalidNodeGroupCleanup.ts b/packages/frontend/editor-ui/src/app/composables/useInvalidNodeGroupCleanup.ts new file mode 100644 index 00000000000..5ecd6d9b035 --- /dev/null +++ b/packages/frontend/editor-ui/src/app/composables/useInvalidNodeGroupCleanup.ts @@ -0,0 +1,106 @@ +import type { IWorkflowGroup } from 'n8n-workflow'; +import { validateNodeSelectionForGrouping } from 'n8n-workflow'; +import { escapeHtml } from 'xss'; + +import { useI18n } from '@n8n/i18n'; +import { useRootStore } from '@n8n/stores/useRootStore'; + +import type { INodeUi } from '@/Interface'; +import { useNodeTypesStore } from '@/app/stores/nodeTypes.store'; +import type { WorkflowDocumentStore } from '@/app/stores/workflowDocument.store'; +import { useTelemetry } from './useTelemetry'; +import { useToast } from './useToast'; + +/** + * Removes node groups that this instance's backend would reject on save + * (`validateWorkflowNodeGroups`), using the same shared validator. + * + * Newer n8n versions may allow group shapes this version rejects (e.g. new + * groupable node types or looser connectivity rules). When such a workflow is + * saved here, the backend rejects every save attempt and autosave gets stuck + * in a retry loop of error toasts. Ungrouping the offending groups up front + * lets the save go through — nodes and connections are left untouched. + */ +export function useInvalidNodeGroupCleanup() { + const nodeTypesStore = useNodeTypesStore(); + const rootStore = useRootStore(); + const toast = useToast(); + const telemetry = useTelemetry(); + const i18n = useI18n(); + + function isGroupValid( + store: WorkflowDocumentStore, + group: IWorkflowGroup, + allGroups: IWorkflowGroup[], + ): boolean { + const nodes: INodeUi[] = []; + for (const nodeId of group.nodeIds) { + const node = store.getNodeById(nodeId); + // The backend rejects groups referencing nodes that don't exist + if (!node) return false; + nodes.push(node); + } + + return validateNodeSelectionForGrouping({ + nodes, + connectionsBySourceNode: store.connectionsBySourceNode, + getNodeType: (node) => nodeTypesStore.getNodeType(node.type, node.typeVersion), + existingNodeGroups: allGroups.filter((other) => other.id !== group.id), + }).valid; + } + + function showGroupsRemovedToast(removedGroups: IWorkflowGroup[]) { + // Toast messages render as sanitized HTML, so the group names can be + // shown as a bullet list. Escape them since they are user input. + // The global CSS reset zeroes list padding, which clips `outside` + // markers — position them inside so the bullets stay visible. + const groups = ``; + + toast.showMessage({ + title: i18n.baseText('canvas.nodeGroup.removedOnSave.title'), + message: i18n.baseText('canvas.nodeGroup.removedOnSave.message', { + interpolate: { groups }, + }), + type: 'warning', + }); + } + + /** + * Deletes all invalid node groups from the given workflow document and + * notifies the user with a single warning toast. Returns the removed groups. + */ + function removeInvalidNodeGroups(store: WorkflowDocumentStore): IWorkflowGroup[] { + const groups = store.allGroups; + if (groups.length === 0) return []; + + const invalidGroups = groups.filter((group) => !isGroupValid(store, group, groups)); + if (invalidGroups.length === 0) return []; + + for (const group of invalidGroups) { + store.deleteGroup(group.id); + telemetry.track('User ungrouped nodes', { + workflow_id: store.workflowId, + group_id: group.id, + node_ids: group.nodeIds, + node_count: group.nodeIds.length, + group_title: group.name, + source: 'invalid-on-save', + push_ref: rootStore.pushRef, + }); + } + + telemetry.track('Auto-ungrouped invalid node groups', { + groups_affected: invalidGroups.length, + }); + + showGroupsRemovedToast(invalidGroups); + + return invalidGroups; + } + + return { + removeInvalidNodeGroups, + }; +} diff --git a/packages/frontend/editor-ui/src/app/composables/useWorkflowSaving.test.ts b/packages/frontend/editor-ui/src/app/composables/useWorkflowSaving.test.ts index bbb570c3f36..039246f9b25 100644 --- a/packages/frontend/editor-ui/src/app/composables/useWorkflowSaving.test.ts +++ b/packages/frontend/editor-ui/src/app/composables/useWorkflowSaving.test.ts @@ -1,5 +1,11 @@ import { useUIStore } from '@/app/stores/ui.store'; -import { AutoSaveState, MODAL_CANCEL, MODAL_CONFIRM, VIEWS } from '@/app/constants'; +import { + AutoSaveState, + MODAL_CANCEL, + MODAL_CONFIRM, + STICKY_NODE_TYPE, + VIEWS, +} from '@/app/constants'; import { useWorkflowSaving } from './useWorkflowSaving'; import router from '@/app/router'; import { createTestingPinia } from '@pinia/testing'; @@ -13,7 +19,7 @@ import { useSettingsStore } from '@/app/stores/settings.store'; import type { WorkflowDataUpdate } from '@n8n/rest-api-client/api/workflows'; import { mockedStore } from '@/__tests__/utils'; import { createTestNode, createTestWorkflow, mockNodeTypeDescription } from '@/__tests__/mocks'; -import { CHAT_TRIGGER_NODE_TYPE } from 'n8n-workflow'; +import { CHAT_TRIGGER_NODE_TYPE, NodeConnectionTypes } from 'n8n-workflow'; import { useNodeTypesStore } from '@/app/stores/nodeTypes.store'; import { createWorkflowDocumentId, @@ -45,6 +51,18 @@ vi.mock('@/app/composables/useMessage', () => { }; }); +const showMessageSpy = vi.hoisted(() => vi.fn()); + +vi.mock('@/app/composables/useToast', () => ({ + useToast: () => ({ + showMessage: showMessageSpy, + showToast: vi.fn(() => ({ close: vi.fn() })), + showError: vi.fn(), + clearAllStickyNotifications: vi.fn(), + showNotificationForViews: vi.fn(), + }), +})); + vi.mock('@n8n/permissions', () => ({ getResourcePermissions: () => ({ workflow: { update: true }, @@ -652,6 +670,93 @@ describe('useWorkflowSaving', () => { false, ); }); + + it('should remove invalid node groups before saving and warn the user', async () => { + const workflow = createTestWorkflow({ + id: 'w6', + nodes: [ + createTestNode({ id: 'node-a', name: 'Node A' }), + createTestNode({ id: 'node-b', name: 'Node B' }), + createTestNode({ id: 'sticky', name: 'Sticky', type: STICKY_NODE_TYPE }), + ], + connections: { + 'Node A': { + [NodeConnectionTypes.Main]: [ + [{ node: 'Node B', type: NodeConnectionTypes.Main, index: 0 }], + ], + }, + }, + // The sticky member is not connected to the rest of the group, which + // this version's (<2.32) grouping rules reject + nodeGroups: [{ id: 'group-1', name: 'Group 1', nodeIds: ['node-a', 'node-b', 'sticky'] }], + }); + + vi.spyOn(workflowsListStore, 'fetchWorkflow').mockResolvedValue(workflow); + vi.spyOn(workflowsStore, 'updateWorkflow').mockResolvedValue(workflow); + + workflowsStore.setWorkflowId(workflow.id); + const documentStore = useWorkflowDocumentStore(createWorkflowDocumentId(workflow.id)); + documentStore.hydrate(workflow); + workflowsListStore.workflowsById = { [workflow.id]: workflow }; + + const { saveCurrentWorkflow } = useWorkflowSaving({ router }); + const saved = await saveCurrentWorkflow({ id: 'w6' }); + + expect(saved).toBe(true); + expect(workflowsStore.updateWorkflow).toHaveBeenCalledWith( + 'w6', + expect.objectContaining({ nodeGroups: [] }), + false, + ); + expect(documentStore.allGroups).toHaveLength(0); + expect(showMessageSpy).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'warning', + title: 'Groups removed', + message: expect.stringContaining('
  • Group 1
    • '), + }), + ); + }); + + it('should keep valid node groups in the save payload', async () => { + const workflow = createTestWorkflow({ + id: 'w7', + nodes: [ + createTestNode({ id: 'node-a', name: 'Node A' }), + createTestNode({ id: 'node-b', name: 'Node B' }), + ], + connections: { + 'Node A': { + [NodeConnectionTypes.Main]: [ + [{ node: 'Node B', type: NodeConnectionTypes.Main, index: 0 }], + ], + }, + }, + nodeGroups: [{ id: 'group-1', name: 'Group 1', nodeIds: ['node-a', 'node-b'] }], + }); + + vi.spyOn(workflowsListStore, 'fetchWorkflow').mockResolvedValue(workflow); + vi.spyOn(workflowsStore, 'updateWorkflow').mockResolvedValue(workflow); + + workflowsStore.setWorkflowId(workflow.id); + const documentStore = useWorkflowDocumentStore(createWorkflowDocumentId(workflow.id)); + documentStore.hydrate(workflow); + workflowsListStore.workflowsById = { [workflow.id]: workflow }; + + const { saveCurrentWorkflow } = useWorkflowSaving({ router }); + const saved = await saveCurrentWorkflow({ id: 'w7' }); + + expect(saved).toBe(true); + expect(workflowsStore.updateWorkflow).toHaveBeenCalledWith( + 'w7', + expect.objectContaining({ + nodeGroups: [{ id: 'group-1', name: 'Group 1', nodeIds: ['node-a', 'node-b'] }], + }), + false, + ); + expect(documentStore.allGroups).toHaveLength(1); + expect(showMessageSpy).not.toHaveBeenCalled(); + }); }); describe('autoSaveWorkflow', () => { diff --git a/packages/frontend/editor-ui/src/app/composables/useWorkflowSaving.ts b/packages/frontend/editor-ui/src/app/composables/useWorkflowSaving.ts index 614996d199c..a81eef5bed8 100644 --- a/packages/frontend/editor-ui/src/app/composables/useWorkflowSaving.ts +++ b/packages/frontend/editor-ui/src/app/composables/useWorkflowSaving.ts @@ -40,6 +40,7 @@ import { useWorkflowId } from '@/app/composables/useWorkflowId'; import { useWorkflowSaveStore } from '@/app/stores/workflowSave.store'; import { useBackendConnectionStore } from '@/app/stores/backendConnection.store'; import { useSettingsStore } from '@/app/stores/settings.store'; +import { useInvalidNodeGroupCleanup } from '@/app/composables/useInvalidNodeGroupCleanup'; export function useWorkflowSaving({ router, @@ -67,6 +68,7 @@ export function useWorkflowSaving({ const backendConnectionStore = useBackendConnectionStore(); const settingsStore = useSettingsStore(); const workflowId = useWorkflowId(); + const { removeInvalidNodeGroups } = useInvalidNodeGroupCleanup(); async function promptSaveUnsavedWorkflowChanges( next: NavigationGuardNext, @@ -201,12 +203,19 @@ export function useWorkflowSaving({ } uiStore.addActiveAction('workflowSaving'); - // Capture dirty state count before save to detect changes made during save - const dirtyCountBeforeSave = uiStore.dirtyStateSetCount; - const workflowDocumentStore = useWorkflowDocumentStore( createWorkflowDocumentId(currentWorkflow), ); + + // Ungroup node groups this version can't save (e.g. groups created on a + // newer n8n version) so the request isn't rejected on every (auto)save. + // Runs before the dirty-count capture so the removal doesn't keep the + // state dirty after a successful save. + removeInvalidNodeGroups(workflowDocumentStore); + + // Capture dirty state count before save to detect changes made during save + const dirtyCountBeforeSave = uiStore.dirtyStateSetCount; + const workflowDataRequest: WorkflowDataUpdate = workflowDocumentStore.serialize(); // This can happen if the user has another workflow in the browser history and navigates // via the browser back button, encountering our warning dialog with the new route already set @@ -385,12 +394,17 @@ export function useWorkflowSaving({ try { uiStore.addActiveAction('workflowSaving'); - // Capture dirty state count before save to detect changes made during save - const dirtyCountBeforeSave = uiStore.dirtyStateSetCount; - const currentDocumentStore = useWorkflowDocumentStore( createWorkflowDocumentId(workflowId.value), ); + + if (!data) { + removeInvalidNodeGroups(currentDocumentStore); + } + + // Capture dirty state count before save to detect changes made during save + const dirtyCountBeforeSave = uiStore.dirtyStateSetCount; + const workflowDataRequest: WorkflowDataCreate = data || currentDocumentStore.serialize(); const changedNodes = {} as IDataObject;