fix(editor): Auto-ungroup invalid groups on workflow save (backport to release-candidate/2.30.x) (#34514)

Co-authored-by: Milorad FIlipović <milorad@n8n.io>

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",

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: <ul style="list-style-position: inside"><li>Group 1</li></ul>',
+			}),
+		);
+		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('<li>Group 2</li><li>Group 3</li>'),
+			}),
+		);
+		// 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: '<img src=x onerror=alert(1)>', nodeIds: ['node-a', 'node-b'] },
+			],
+		});
+
+		const { removeInvalidNodeGroups } = useInvalidNodeGroupCleanup();
+		removeInvalidNodeGroups(store);
+
+		expect(showMessageSpy).toHaveBeenCalledWith(
+			expect.objectContaining({
+				message: expect.stringContaining('<li>&lt;img src=x onerror=alert(1)&gt;</li>'),
+			}),
+		);
+	});
+
+	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();
+	});
+});

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 = `<ul style="list-style-position: inside">${removedGroups
+			.map((group) => `<li>${escapeHtml(group.name)}</li>`)
+			.join('')}</ul>`;
+
+		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,
+	};
+}

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('<li>Group 1</li>'),
+				}),
+			);
+		});
+
+		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', () => {

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;