mirror of
https://github.com/n8n-io/n8n.git
synced 2026-10-17 05:29:44 +02:00
fix: Keep free-floating sticky notes clear of workflow nodes (#41195)
Co-authored-by: n8n-cat-bot[bot] <n8n-cat-bot[bot]@users.noreply.github.com>
This commit is contained in:
parent
236220b248
commit
131e9c8679
|
|
@ -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));
|
||||
|
|
|
|||
|
|
@ -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<INodeTypeDescription> = {},
|
||||
): INodeType => ({
|
||||
description: {
|
||||
name,
|
||||
displayName: name,
|
||||
description: '',
|
||||
version: 1,
|
||||
defaults: { name },
|
||||
group: [],
|
||||
inputs: ['main'],
|
||||
outputs: ['main'],
|
||||
properties: [],
|
||||
...description,
|
||||
},
|
||||
});
|
||||
const nodeTypes: Record<string, INodeType> = {
|
||||
'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<INodeTypes>({
|
||||
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([]);
|
||||
});
|
||||
});
|
||||
|
|
@ -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'],
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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<string, GraphNode>,
|
||||
positions: ReadonlyMap<string, [number, number]>,
|
||||
knownPorts?: ReadonlyMap<string, NodePorts>,
|
||||
): Map<string, StickyGeometry> {
|
||||
const geometryByName = new Map<string, StickyGeometry>();
|
||||
|
||||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -248,7 +248,7 @@ export const jsonSerializer: SerializerPlugin<WorkflowJSON> = {
|
|||
: 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) {
|
||||
|
|
|
|||
|
|
@ -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<string, NodePorts>([
|
||||
[
|
||||
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], {
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user