From 131e9c8679f96254873d401fbb7e9e57c5e976f2 Mon Sep 17 00:00:00 2001 From: "n8n-cat-bot[bot]" <283985454+n8n-cat-bot[bot]@users.noreply.github.com> Date: Thu, 15 Oct 2026 14:41:40 +0000 Subject: [PATCH] fix: Keep free-floating sticky notes clear of workflow nodes (#41195) Co-authored-by: n8n-cat-bot[bot] --- .../handlers/parse-validate-handler.ts | 10 +- .../parse-validate-handler-layout.test.ts | 78 +++++++++++++ .../test/parse-validate-handler.test.ts | 2 + .../src/workflow-builder/constants.ts | 3 - .../src/workflow-builder/layout-utils.ts | 23 +++- .../plugins/serializers/json-serializer.ts | 2 +- .../workflow-builder/sticky-placement.test.ts | 110 +++++++++++++++++- 7 files changed, 217 insertions(+), 11 deletions(-) create mode 100644 packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/test/parse-validate-handler-layout.test.ts diff --git a/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/parse-validate-handler.ts b/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/parse-validate-handler.ts index 8a3bf5466a4..ab27cd59db9 100644 --- a/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/parse-validate-handler.ts +++ b/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/parse-validate-handler.ts @@ -11,6 +11,7 @@ import { connectRequiredSubnodeInputs, describeAddedSubnodeConnection, parseWorkflowCodeToBuilder, + resolveNodePorts, validateWorkflow, workflow, } from '@n8n/workflow-sdk'; @@ -308,7 +309,14 @@ export class ParseValidateHandler { ); // Convert to JSON with Dagre layout matching the FE's tidy-up - const workflowJson: WorkflowJSON = builder.toJSON({ tidyUp: true, existingGroupIdsByName }); + const nodePorts = this.nodeTypesProvider + ? await resolveNodePorts(json, this.nodeTypesProvider) + : undefined; + const workflowJson: WorkflowJSON = builder.toJSON({ + tidyUp: true, + existingGroupIdsByName, + nodePorts, + }); // A fresh serialization, so the links added above have to be re-added. for (const link of this.connectRequiredInputs(workflowJson)) { allWarnings.push(describeAddedSubnodeConnection(link)); diff --git a/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/test/parse-validate-handler-layout.test.ts b/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/test/parse-validate-handler-layout.test.ts new file mode 100644 index 00000000000..663d3f634e4 --- /dev/null +++ b/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/test/parse-validate-handler-layout.test.ts @@ -0,0 +1,78 @@ +import type { INodeType, INodeTypeDescription, INodeTypes } from 'n8n-workflow'; +import { mock } from 'vitest-mock-extended'; + +import { ParseValidateHandler } from '../parse-validate-handler'; + +describe('ParseValidateHandler layout', () => { + it('keeps a free-floating sticky clear of a Switch with unwired outputs', async () => { + const nodeType = ( + name: string, + description: Partial = {}, + ): INodeType => ({ + description: { + name, + displayName: name, + description: '', + version: 1, + defaults: { name }, + group: [], + inputs: ['main'], + outputs: ['main'], + properties: [], + ...description, + }, + }); + const nodeTypes: Record = { + 'n8n-nodes-base.manualTrigger': nodeType('n8n-nodes-base.manualTrigger', { + inputs: [], + group: ['trigger'], + }), + 'n8n-nodes-base.switch': nodeType('n8n-nodes-base.switch', { + version: 3.2, + outputs: '={{ Array($parameter.numberOutputs).fill("main") }}', + properties: [{ displayName: 'Outputs', name: 'numberOutputs', type: 'number', default: 4 }], + }), + 'n8n-nodes-base.noOp': nodeType('n8n-nodes-base.noOp'), + 'n8n-nodes-base.stickyNote': nodeType('n8n-nodes-base.stickyNote', { + inputs: [], + outputs: [], + }), + }; + const nodeTypesProvider = mock({ + getByNameAndVersion: (name) => nodeTypes[name], + }); + const handler = new ParseValidateHandler({ nodeTypesProvider, generatePinData: false }); + + const { workflow } = await handler.parseAndValidate(` + const start = trigger({ + type: 'n8n-nodes-base.manualTrigger', version: 1, config: { name: 'Start' } + }); + const router = node({ + type: 'n8n-nodes-base.switch', version: 3.2, + config: { name: 'Switch', parameters: { mode: 'expression', numberOutputs: 8 } } + }); + const done = node({ + type: 'n8n-nodes-base.noOp', version: 1, config: { name: 'Done' } + }); + workflow('wf', 'Test') + .add(start.to(router.onCase(0, done))) + .add(sticky('## Workflow notes\\nRun this workflow manually.', { name: 'Notes' })); + `); + + expect(workflow.connections.Switch.main).toEqual([[{ node: 'Done', type: 'main', index: 0 }]]); + const note = workflow.nodes.find(({ name }) => name === 'Notes'); + expect(note).toBeDefined(); + if (!note) throw new Error('Notes not found'); + const [noteX, noteY] = note.position; + const overlappingNodes = workflow.nodes + .filter(({ name }) => name !== 'Notes') + .filter(({ name, position: [x, y] }) => { + // Eight main outputs make the Switch 288 pixels tall on the canvas. + const height = name === 'Switch' ? 288 : 96; + return x < noteX + 240 && noteX < x + 96 && y < noteY + 160 && noteY < y + height; + }) + .map(({ name }) => name); + + expect(overlappingNodes).toEqual([]); + }); +}); diff --git a/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/test/parse-validate-handler.test.ts b/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/test/parse-validate-handler.test.ts index ad1a6f6caf7..80f4b852c70 100644 --- a/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/test/parse-validate-handler.test.ts +++ b/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/test/parse-validate-handler.test.ts @@ -18,6 +18,7 @@ import { ParseValidateHandler } from '../parse-validate-handler'; // Mock the workflow-sdk module vi.mock('@n8n/workflow-sdk', () => ({ parseWorkflowCodeToBuilder: vi.fn(), + resolveNodePorts: vi.fn(), validateWorkflow: vi.fn(), workflow: { fromJSON: vi.fn() }, stripImportStatements: vi.fn((code: string) => code), @@ -388,6 +389,7 @@ describe('ParseValidateHandler', () => { expect(mockBuilder.toJSON).toHaveBeenCalledWith({ tidyUp: true, + nodePorts: undefined, existingGroupIdsByName: new Map([ ['Ingestion', 'group-random-1'], ['Processing', 'group-random-2'], diff --git a/packages/@n8n/workflow-sdk/src/workflow-builder/constants.ts b/packages/@n8n/workflow-sdk/src/workflow-builder/constants.ts index a2f65a5efc1..ae90aa6ad05 100644 --- a/packages/@n8n/workflow-sdk/src/workflow-builder/constants.ts +++ b/packages/@n8n/workflow-sdk/src/workflow-builder/constants.ts @@ -38,9 +38,6 @@ export const STICKY_PADDING = GRID_SIZE * 2; // 32 /** Headroom above wrapped nodes so the sticky's text does not sit on top of them. */ export const STICKY_HEADER_HEIGHT = GRID_SIZE * 4; // 64 -/** Bound on the sticky de-overlap walk so a pathological graph can't spin. */ -export const MAX_STICKY_SEPARATION_STEPS = 50; - // BFS layout constants (used by calculateNodePositions for basic positioning) export const NODE_SPACING_X = 200; export const DEFAULT_Y = 300; diff --git a/packages/@n8n/workflow-sdk/src/workflow-builder/layout-utils.ts b/packages/@n8n/workflow-sdk/src/workflow-builder/layout-utils.ts index 65639c8d2c6..d06a74c9446 100644 --- a/packages/@n8n/workflow-sdk/src/workflow-builder/layout-utils.ts +++ b/packages/@n8n/workflow-sdk/src/workflow-builder/layout-utils.ts @@ -38,7 +38,6 @@ import { DEFAULT_STICKY_SIZE, STICKY_PADDING, STICKY_HEADER_HEIGHT, - MAX_STICKY_SEPARATION_STEPS, } from './constants'; import { collapseNodeGroups, @@ -917,7 +916,8 @@ function wrappingBoxFor(anchorBoxes: BoundingBox[]): BoundingBox | undefined { /** Push a box down until it clears every box already placed. */ function separateFrom(placed: BoundingBox[], box: BoundingBox): BoundingBox { let separated = box; - for (let step = 0; step < MAX_STICKY_SEPARATION_STEPS; step++) { + // Each move clears one box and only increases y, so each box needs at most one step. + for (let step = 0; step < placed.length; step++) { const collision = placed.find((placedBox) => boxesOverlap(placedBox, separated)); if (!collision) break; separated = { @@ -942,6 +942,7 @@ function separateFrom(placed: BoundingBox[], box: BoundingBox): BoundingBox { export function resolveStickyGeometry( nodes: ReadonlyMap, positions: ReadonlyMap, + knownPorts?: ReadonlyMap, ): Map { const geometryByName = new Map(); @@ -962,9 +963,15 @@ export function resolveStickyGeometry( const boxOfNode = (name: string): BoundingBox | undefined => { const graphNode = nodes.get(name); if (!graphNode) return undefined; - const position = graphNode.instance.config?.position ?? positions.get(name); - if (!position) return undefined; - const { width, height } = getNodeDimensions(name, aiParentNames, aiConfigNames, nodes); + const position = graphNode.instance.config?.position ?? + positions.get(name) ?? [START_X, DEFAULT_Y]; + const { width, height } = getNodeDimensions( + name, + aiParentNames, + aiConfigNames, + nodes, + knownPorts, + ); return { x: position[0], y: position[1], width, height }; }; @@ -1015,6 +1022,12 @@ export function resolveStickyGeometry( }; const placed = resolved.filter((sticky) => sticky.pinned).map(({ box }) => box); + for (const [name, graphNode] of nodes) { + if (graphNode.instance.type === STICKY_NODE_TYPE) continue; + const box = boxOfNode(name); + if (box) placed.push(box); + } + for (const { name, box, sizedByAnchors, pinned } of resolved) { if (pinned) { record(name, box, sizedByAnchors); diff --git a/packages/@n8n/workflow-sdk/src/workflow-builder/plugins/serializers/json-serializer.ts b/packages/@n8n/workflow-sdk/src/workflow-builder/plugins/serializers/json-serializer.ts index f37512b459a..239690c9442 100644 --- a/packages/@n8n/workflow-sdk/src/workflow-builder/plugins/serializers/json-serializer.ts +++ b/packages/@n8n/workflow-sdk/src/workflow-builder/plugins/serializers/json-serializer.ts @@ -248,7 +248,7 @@ export const jsonSerializer: SerializerPlugin = { : calculateNodePositions(ctx.nodes); // Sticky notes are placed last: their box depends on where their anchors landed - const stickyGeometry = resolveStickyGeometry(ctx.nodes, nodePositions); + const stickyGeometry = resolveStickyGeometry(ctx.nodes, nodePositions, ctx.nodePorts); // Convert nodes and connections for (const [mapKey, graphNode] of ctx.nodes) { diff --git a/packages/@n8n/workflow-sdk/src/workflow-builder/sticky-placement.test.ts b/packages/@n8n/workflow-sdk/src/workflow-builder/sticky-placement.test.ts index a167b487a81..2ed0f6b985d 100644 --- a/packages/@n8n/workflow-sdk/src/workflow-builder/sticky-placement.test.ts +++ b/packages/@n8n/workflow-sdk/src/workflow-builder/sticky-placement.test.ts @@ -5,7 +5,7 @@ * at toJSON({ tidyUp: true }). These tests pin the placement contract for that case. */ -import type { NodeJSON, WorkflowJSON } from '../types/base'; +import type { NodeJSON, NodePorts, WorkflowJSON } from '../types/base'; import { workflow } from '../workflow-builder'; import { DEFAULT_NODE_SIZE, DEFAULT_STICKY_SIZE, STICKY_NODE_TYPE } from './constants'; import { node, sticky, trigger } from './node-builders/node-builder'; @@ -129,6 +129,114 @@ describe('sticky note placement with tidyUp', () => { } }); + it('keeps a sticky without anchors or an explicit position clear of workflow nodes', () => { + const { start, fetch, compute } = buildChain(); + const builder = workflow('wf', 'Test') + .add(start.to(fetch).to(compute)) + .add(sticky('## Workflow notes', { name: 'Workflow notes' })); + + // CONTEXT-238: MCP regenerates node IDs before it serializes the workflow with tidyUp. + builder.regenerateNodeIds(); + const json = builder.toJSON({ tidyUp: true }); + + const box = stickyBox(json, 'Workflow notes'); + const overlappingNodes = [start, fetch, compute] + .filter(({ name }) => overlaps(box, nodeBox(json, name))) + .map(({ name }) => name); + + expect(overlappingNodes).toEqual([]); + }); + + it('keeps free-floating notes clear of each other and a long column of nodes', () => { + const builder = workflow('wf', 'Test'); + const obstacles = Array.from({ length: 51 }, (_, index) => + node({ + type: 'n8n-nodes-base.code', + version: 2, + config: { name: `Step ${index}`, position: [100, 300 + index * 192] }, + }), + ); + for (const obstacle of obstacles) { + builder.add(obstacle); + } + const json = builder + .add(sticky('## One', { name: 'One' })) + .add(sticky('## Two', { name: 'Two' })) + .toJSON({ tidyUp: true }); + + const boxes = ['One', 'Two'].map((name) => stickyBox(json, name)); + expect(overlaps(boxes[0], boxes[1])).toBe(false); + for (const box of boxes) { + for (const { name } of obstacles) { + expect(overlaps(box, nodeBox(json, name))).toBe(false); + } + } + }); + + it.each([false, true])('keeps a sticky clear of a rootless cycle with tidyUp=%s', (tidyUp) => { + const { fetch, compute } = buildChain(); + const json = workflow('wf', 'Test') + .add(fetch.to(compute)) + .connect(compute, 0, fetch, 0) + .add(sticky('## Workflow notes', { name: 'Workflow notes' })) + .toJSON({ tidyUp }); + + const box = stickyBox(json, 'Workflow notes'); + for (const { name } of [fetch, compute]) { + expect(overlaps(box, nodeBox(json, name))).toBe(false); + } + }); + + it.each([ + { + dimension: 'height', + mainOutputs: 8, + nonMainInputs: 0, + configurable: false, + x: 100, + width: 96, + height: 288, + }, + { + dimension: 'width', + mainOutputs: 1, + nonMainInputs: 8, + configurable: true, + x: 0, + width: 416, + height: 96, + }, + ])('uses the declared node $dimension to keep a sticky clear', (testCase) => { + const router = node({ + type: 'n8n-nodes-base.switch', + version: 3.2, + config: { name: 'Router', position: [testCase.x, 300] }, + }); + const nodePorts = new Map([ + [ + router.name, + { + mainInputs: 1, + mainOutputs: testCase.mainOutputs, + nonMainInputs: testCase.nonMainInputs, + configuration: false, + configurable: testCase.configurable, + }, + ], + ]); + const json = workflow('wf', 'Test') + .add(router) + .add(sticky('## Workflow notes', { name: 'Workflow notes' })) + .toJSON({ tidyUp: true, nodePorts }); + + const routerBox = { + ...nodeBox(json, router.name), + width: testCase.width, + height: testCase.height, + }; + expect(overlaps(stickyBox(json, 'Workflow notes'), routerBox)).toBe(false); + }); + it('keeps an explicit position and size the caller passed', () => { const { start, fetch } = buildChain(); const note = sticky('## Pinned', [start, fetch], {