ronantakizawa/codereview-bench huggingface dataset:
Repository: n8n-io/n8n
File: packages/frontend/editor-ui/src/features/workflows/canvas/components/elements/handles/render-types/parts/CanvasHandlePlus.vue (Vue)

Code before:
		lineSize?: number;
		type?: 'success' | 'secondary' | 'default';
	}>(),
	{
		position: 'right',
		handleClasses: undefined,
		plusSize: 24,
		lineSize: 46,
		type: 'default',
	},
);

const { viewport } = useCanvas();
const { calculateEdgeLightness } = useZoomAdjustedValues(viewport);

const lineLightness = calculateEdgeLightness();

const emit = defineEmits<{
	'click:plus': [event: MouseEvent];
}>();

const style = useCssModule();

const classes = computed(() => [
	style.container,
	style[props.position],
	style[props.type],
	props.handleClasses,
]);

const viewBox = computed(() => {
	switch (props.position) {
		case 'bottom':
		case 'top':
			return {
				width: props.plusSize,
				height: props.lineSize + props.plusSize,
			};
		default:
			return {
				width: props.lineSize + props.plusSize,
				height: props.plusSize,
			};
	}
});

const svgStyles = computed(() => ({
	width: `${viewBox.value.width}px`,
	height: `${viewBox.value.height}px`,
	'--canvas-handle-plus-line--color--lightness--light': lineLightness.value.light,
	'--canvas-handle-plus-line--color--lightness--dark': lineLightness.value.dark,

Review comment (suggestion):
It looks like the `canvas-handle-plus-wrapper` class was added for this fix in the past: https://github.com/n8n-io/n8n/pull/16765

I think it's worth verifying that the new implementation also still has the "plus buttons" working when behind a text description.

Will test this after lunch.

Code after:
		lineSize?: number;
		type?: 'success' | 'secondary' | 'default';
	}>(),
	{
		position: 'right',
		handleClasses: undefined,
		plusSize: 24,
		lineSize: 46,
		type: 'default',
	},
);

const { viewport } = useCanvas();
const { calculateEdgeLightness } = useZoomAdjustedValues(viewport);

const lineLightness = calculateEdgeLightness();

const emit = defineEmits<{
	'click:plus': [event: MouseEvent];
}>();

const style = useCssModule();

const classes = computed(() => [
	'canvas-handle-plus',
	style.container,
	style[props.position],
	props.handleClasses,
]);

const viewBox = computed(() => {
	switch (props.position) {
		case 'bottom':
		case 'top':
			return {
				width: props.plusSize,
				height: props.lineSize + props.plusSize,
			};
		default:
			return {
				width: props.lineSize + props.plusSize,
				height: props.plusSize,
			};
	}
});

const svgStyles = computed(() => ({
	width: `${viewBox.value.width}px`,
	height: `${viewBox.value.height}px`,
	'--canvas-handle-plus-line--color--lightness--light': lineLightness.value.light,
	'--canvas-handle-plus-line--color--lightness--dark': lineLightness.value.dark,
Repository: n8n-io/n8n
File: packages/nodes-base/nodes/Form/v1/FormTriggerV1.node.ts (TypeScript)

Code before:
			name: 'options',
			type: 'collection',
			placeholder: 'Add option',
			default: {},
			displayOptions: {
				hide: {
					responseMode: ['responseNode'],
				},
			},
			options: [
				{
					displayName: 'Form Submitted Text',
					name: 'formSubmittedText',
					description: 'The text displayed to users after they filled the form',
					type: 'string',
					default: 'Your response has been recorded',
				},
				{
					displayName: 'IP(s) Allowlist',
					name: 'ipWhitelist',
					type: 'string',
					placeholder: 'e.g. 127.0.0.1, 192.168.1.0/24',
					default: '',
					description:
						'Comma-separated list of allowed IP addresses or CIDR ranges. Leave empty to allow all IPs.',
				},
			],
		},
	],
};

export class FormTriggerV1 implements INodeType {
	description: INodeTypeDescription;

	constructor(baseDescription: INodeTypeBaseDescription) {
		this.description = {
			...baseDescription,
			...descriptionV1,
		};
	}

	async webhook(this: IWebhookFunctions) {
		return await formWebhook(this);
	}
}


Review comment (refactor):
Since both `FormTriggerV1` and `FormTriggerV2` define the same IP allowlist option, consider extracting it to `common.descriptions.ts`.

Code after:
			displayName: 'Options',
			name: 'options',
			type: 'collection',
			placeholder: 'Add option',
			default: {},
			displayOptions: {
				hide: {
					responseMode: ['responseNode'],
				},
			},
			options: [
				ipAllowlist,
				{
					displayName: 'Form Submitted Text',
					name: 'formSubmittedText',
					description: 'The text displayed to users after they filled the form',
					type: 'string',
					default: 'Your response has been recorded',
				},
			],
		},
	],
};

export class FormTriggerV1 implements INodeType {
	description: INodeTypeDescription;

	constructor(baseDescription: INodeTypeBaseDescription) {
		this.description = {
			...baseDescription,
			...descriptionV1,
		};
	}

	async webhook(this: IWebhookFunctions) {
		return await formWebhook(this);
	}
}
Repository: n8n-io/n8n
File: packages/nodes-base/nodes/Form/v2/FormTriggerV2.node.ts (TypeScript)

Code before:
					type: 'string',
					default: 'Submit',
				},
				{
					...webhookPath,
					required: false,
					displayOptions: { show: { '@version': [{ _cnd: { gte: 2.2 } }] } },
				},
				{
					...respondWithOptions,
					displayOptions: {
						hide: {
							'/responseMode': ['responseNode'],
						},
					},
				},
				{
					displayName: 'Ignore Bots',
					name: 'ignoreBots',
					type: 'boolean',
					default: false,
					description: 'Whether to ignore requests from bots like link previewers and web crawlers',
				},
				{
					displayName: 'IP(s) Allowlist',
					name: 'ipWhitelist',
					type: 'string',
					placeholder: 'e.g. 127.0.0.1, 192.168.1.0/24',
					default: '',
					description:
						'Comma-separated list of allowed IP addresses or CIDR ranges. Leave empty to allow all IPs.',
				},
				{
					...useWorkflowTimezone,
					default: false,
					description: "Whether to use the workflow timezone in 'submittedAt' field or UTC",
					displayOptions: {
						show: {
							'@version': [2],
						},
					},
				},
				{
					...useWorkflowTimezone,
					default: true,
					description: "Whether to use the workflow timezone in 'submittedAt' field or UTC",
					displayOptions: {
						show: {
							'@version': [{ _cnd: { gt: 2 } }],
						},
					},

Review comment (refactor):
I just want to clarify something. I see we use 2 approaches for naming: `Whitelist` and `Allowlist` and then corresponding helper functions and test files are also affected with this mix.

Not a big deal overall, just wanted to double check if it's worth unifying?

Code after:
					description: 'The label of the submit button in the form',
					name: 'buttonLabel',
					type: 'string',
					default: 'Submit',
				},
				{
					...webhookPath,
					required: false,
					displayOptions: { show: { '@version': [{ _cnd: { gte: 2.2 } }] } },
				},
				{
					...respondWithOptions,
					displayOptions: {
						hide: {
							'/responseMode': ['responseNode'],
						},
					},
				},
				{
					displayName: 'Ignore Bots',
					name: 'ignoreBots',
					type: 'boolean',
					default: false,
					description: 'Whether to ignore requests from bots like link previewers and web crawlers',
				},
				{
					...useWorkflowTimezone,
					default: false,
					description: "Whether to use the workflow timezone in 'submittedAt' field or UTC",
					displayOptions: {
						show: {
							'@version': [2],
						},
					},
				},
				{
					...useWorkflowTimezone,
					default: true,
					description: "Whether to use the workflow timezone in 'submittedAt' field or UTC",
					displayOptions: {
						show: {
							'@version': [{ _cnd: { gt: 2 } }],
						},
					},
				},
				{
					displayName: 'Custom Form Styling',
					name: 'customCss',
					type: 'string',
					typeOptions: {
						rows: 10,
Repository: n8n-io/n8n
File: packages/frontend/@n8n/design-system/src/components/N8nBreadcrumbs/Breadcrumbs.vue (Vue)

Code before:
};

watch(
	(): PathItem[] | Promise<PathItem[]> => props.hiddenItems,
	(_newValue: PathItem[] | Promise<PathItem[]>) => {
		void getHiddenItems();
	},
);

const onHiddenMenuVisibleChange = async (visible: boolean) => {
	if (visible) {
		emit('tooltipOpened');
		await getHiddenItems();
	} else {
		emit('tooltipClosed');
	}
};

const emitItemSelected = (id: string) => {
	const item = [...props.items, ...loadedHiddenItems.value].find((i) => i.id === id);
	if (!item) {
		return;
	}
	emit('itemSelected', item);
};

const emitItemHover = (id: string) => {
	const item = [...props.items, ...loadedHiddenItems.value].find((i) => i.id === id);
	if (!item) {
		return;
	}
	emit('itemHover', item);
};

const onHiddenItemMouseUp = (item: UserAction<UserType>) => {
	const pathItem = [...props.items, ...loadedHiddenItems.value].find((i) => i.id === item.value);
	if (!pathItem || !props.dragActive) {
		return;
	}
	emit('itemDrop', pathItem);
};

const onItemMouseUp = (item: PathItem) => {
	if (!props.dragActive) {
		return;
	}
	emit('itemDrop', item);
};

const handleTooltipShow = async () => {
	emit('tooltipOpened');

Review comment (refactor):
nit: we have this `isMiddleMouseButton` utility function in `packages/frontend/editor-ui/src/app/utils/eventUtils.ts` that checks for the middle mouse click `event.button === 1`

Code after:
};

watch(
	(): PathItem[] | Promise<PathItem[]> => props.hiddenItems,
	(_newValue: PathItem[] | Promise<PathItem[]>) => {
		void getHiddenItems();
	},
);

const onHiddenMenuVisibleChange = async (visible: boolean) => {
	if (visible) {
		emit('tooltipOpened');
		await getHiddenItems();
	} else {
		emit('tooltipClosed');
	}
};

const emitItemSelected = (id: string, event?: MouseEvent) => {
	const item = [...props.items, ...loadedHiddenItems.value].find((i) => i.id === id);
	if (!item) {
		return;
	}

	// Allow default browser behavior for modifier keys (ctrl/cmd/shift + click) or middle mouse button
	if (event && (event.ctrlKey || event.metaKey || event.shiftKey || event.button === 1)) {
		return;
	}

	// Prevent default navigation and emit event for custom handling
	if (event && item.href) {
		event.preventDefault();
	}

	emit('itemSelected', item);
};

const emitItemHover = (id: string) => {
	const item = [...props.items, ...loadedHiddenItems.value].find((i) => i.id === id);
	if (!item) {
		return;
	}
	emit('itemHover', item);
};

const onHiddenItemMouseUp = (item: UserAction<UserType>) => {
	const pathItem = [...props.items, ...loadedHiddenItems.value].find((i) => i.id === item.value);
	if (!pathItem || !props.dragActive) {
		return;
	}
	emit('itemDrop', pathItem);
Repository: n8n-io/n8n
File: packages/nodes-base/nodes/Form/utils/utils.ts (TypeScript)

Code before:
	formDescription,
	formFields,
	responseMode,
	mode,
	formSubmittedText,
	redirectUrl,
	appendAttribution,
	buttonLabel,
	customCss,
	authToken,
}: {
	context: IWebhookFunctions;
	res: Response;
	formTitle: string;
	formDescription: string;
	formFields: FormFieldsParameter;
	responseMode: string;
	mode: 'test' | 'production';
	formSubmittedText?: string;
	redirectUrl?: string;
	appendAttribution?: boolean;
	buttonLabel?: string;
	customCss?: string;
	authToken?: string;
}) {
	formDescription = (formDescription || '').replace(/\\n/g, '\n');
	const instanceId = context.getInstanceId();

	const useResponseData = responseMode === 'responseNode';

	let query: IDataObject = {};

	if (context.getNode().type === FORM_TRIGGER_NODE_TYPE) {
		query = context.getRequestObject().query as IDataObject;
	} else if (context.getNode().type === FORM_NODE_TYPE) {
		const parentNodes = context.getParentNodes(context.getNode().name);
		const trigger = parentNodes.find(
			(node) => node.type === FORM_TRIGGER_NODE_TYPE,
		) as NodeTypeAndVersion;
		try {
			const triggerQueryParameters = context.evaluateExpression(
				`{{ $('${trigger?.name}').first().json.formQueryParameters }}`,
			) as IDataObject;

			if (triggerQueryParameters) {
				query = triggerQueryParameters;
			}
		} catch (error) {}
	}

	formFields = prepareFormFields(context, formFields);

Review comment (security):
`.replace(/<br>/g, '\n')` is not necessary, since `formDescription` goes through `sanitizeHtml` before getting to this line and that "normalizes" all `<br>` to `<br />`. Therefore, this `.replace` call does nothing, and the `<br />` would be still rendered as HTML anyway

Code after:
		returnItem.json.formQueryParameters = context.getRequestObject().query;
	}

	return returnItem;
}

export function renderForm({
	context,
	res,
	formTitle,
	formDescription,
	formFields,
	responseMode,
	mode,
	formSubmittedText,
	redirectUrl,
	appendAttribution,
	buttonLabel,
	customCss,
	authToken,
}: {
	context: IWebhookFunctions;
	res: Response;
	formTitle: string;
	formDescription: string;
	formFields: FormFieldsParameter;
	responseMode: string;
	mode: 'test' | 'production';
	formSubmittedText?: string;
	redirectUrl?: string;
	appendAttribution?: boolean;
	buttonLabel?: string;
	customCss?: string;
	authToken?: string;
}) {
	const instanceId = context.getInstanceId();

	const useResponseData = responseMode === 'responseNode';

	let query: IDataObject = {};

	if (context.getNode().type === FORM_TRIGGER_NODE_TYPE) {
		query = context.getRequestObject().query as IDataObject;
	} else if (context.getNode().type === FORM_NODE_TYPE) {
		const parentNodes = context.getParentNodes(context.getNode().name);
		const trigger = parentNodes.find(
			(node) => node.type === FORM_TRIGGER_NODE_TYPE,
		) as NodeTypeAndVersion;
		try {
			const triggerQueryParameters = context.evaluateExpression(
				`{{ $('${trigger?.name}').first().json.formQueryParameters }}`,
Repository: n8n-io/n8n
File: packages/@n8n/db/src/connection/db-connection-options.ts (TypeScript)

Code before:
		} = postgresConfig;

		let ssl: TlsOptions | boolean = postgresConfig.ssl.enabled;
		if (sslCa !== '' || sslCert !== '' || sslKey !== '' || !sslRejectUnauthorized) {
			ssl = {
				ca: sslCa || undefined,
				cert: sslCert || undefined,
				key: sslKey || undefined,
				rejectUnauthorized: sslRejectUnauthorized,
			};
		}

		return {
			type: 'postgres',
			...this.getCommonOptions(),
			...this.getOverrides('postgresdb'),
			schema: postgresConfig.schema,
			poolSize: postgresConfig.poolSize,
			migrations: postgresMigrations,
			connectTimeoutMS: postgresConfig.connectionTimeoutMs,
			ssl,
			extra: {
				idleTimeoutMillis: postgresConfig.idleTimeoutMs,
			},
		};
	}

	private getMysqlConnectionOptions(dbType: 'mariadb' | 'mysqldb'): MysqlConnectionOptions {
		const { mysqldb: mysqlConfig } = this.config;
		return {
			type: dbType === 'mysqldb' ? 'mysql' : 'mariadb',
			...this.getCommonOptions(),
			...this.getOverrides('mysqldb'),
			poolSize: mysqlConfig.poolSize,
			migrations: mysqlMigrations,
			timezone: 'Z', // set UTC as default
		};
	}
}


Review comment (suggestion):
`node-pg` client also has [`statement_timeout`](https://node-postgres.com/apis/client#new-client). I think that is the same thing? Pool [seems to pass](https://github.com/brianc/node-postgres/blob/master/packages/pg-pool/index.js#L233) the options directly to the client so this could


```suggestion
				...(statementTimeoutMs > 0 && { statement_timeout: statementTimeoutMs }),
```

if I'm not mistaken

Code after:
		} = postgresConfig;

		let ssl: TlsOptions | boolean = postgresConfig.ssl.enabled;
		if (sslCa !== '' || sslCert !== '' || sslKey !== '' || !sslRejectUnauthorized) {
			ssl = {
				ca: sslCa || undefined,
				cert: sslCert || undefined,
				key: sslKey || undefined,
				rejectUnauthorized: sslRejectUnauthorized,
			};
		}

		const { statementTimeoutMs } = postgresConfig;

		return {
			type: 'postgres',
			...this.getCommonOptions(),
			...this.getOverrides('postgresdb'),
			schema: postgresConfig.schema,
			poolSize: postgresConfig.poolSize,
			migrations: postgresMigrations,
			connectTimeoutMS: postgresConfig.connectionTimeoutMs,
			ssl,
			extra: {
				idleTimeoutMillis: postgresConfig.idleTimeoutMs,
				...(statementTimeoutMs > 0 && { options: `-c statement_timeout=${statementTimeoutMs}` }),
			},
		};
	}

	private getMysqlConnectionOptions(dbType: 'mariadb' | 'mysqldb'): MysqlConnectionOptions {
		const { mysqldb: mysqlConfig } = this.config;
		return {
			type: dbType === 'mysqldb' ? 'mysql' : 'mariadb',
			...this.getCommonOptions(),
			...this.getOverrides('mysqldb'),
			poolSize: mysqlConfig.poolSize,
			migrations: mysqlMigrations,
			timezone: 'Z', // set UTC as default
		};
	}
}
Repository: n8n-io/n8n
File: packages/cli/src/scaling/scaling.types.ts (TypeScript)

Code before:
	| JobFinishedMessage
	| JobFailedMessage
	| AbortJobMessage
	| SendChunkMessage;

/** Message sent by worker to main to respond to a webhook. */
export type RespondToWebhookMessage = {
	kind: 'respond-to-webhook';
	executionId: string;
	response: IExecuteResponsePromiseData;
	workerId: string;
};

export type JobFinishedProps = {
	success: boolean;
	error?: ExecutionError;
	status: ExecutionStatus;
	lastNodeExecuted?: string;
	usedDynamicCredentials?: boolean;
	metadata?: Record<string, string>;
	startedAt: Date;
	stoppedAt: Date;
};

/** Message sent by worker to main to report a job has finished. */
export type JobFinishedMessage = {
	kind: 'job-finished';
	executionId: string;
	workerId: string;
} & JobFinishedProps;

export type SendChunkMessage = {
	kind: 'send-chunk';
	executionId: string;
	chunkText: StructuredChunk;
	workerId: string;
};

/** Message sent by worker to main to report a job has failed. */
export type JobFailedMessage = {
	kind: 'job-failed';
	executionId: string;
	workerId: string;
	errorMsg: string;
	errorStack: string;
};

/** Message sent by main to worker to abort a job. */
export type AbortJobMessage = {
	kind: 'abort-job';
};

Review comment (suggestion):
I think we should also version these messages so the mains can handle either version coming in. That should make the transition easier, as workers might be running a different version than mains momentarily

Code after:
	| JobFinishedMessage
	| JobFailedMessage
	| AbortJobMessage
	| SendChunkMessage;

/** Message sent by worker to main to respond to a webhook. */
export type RespondToWebhookMessage = {
	kind: 'respond-to-webhook';
	executionId: string;
	response: IExecuteResponsePromiseData;
	workerId: string;
};

export type JobFinishedProps = {
	success: boolean;
	error?: ExecutionError;
	status: ExecutionStatus;
	lastNodeExecuted?: string;
	usedDynamicCredentials?: boolean;
	metadata?: Record<string, string>;
	startedAt: Date;
	stoppedAt: Date;
};

/** Message sent by worker to main to report a job has finished. */
export type JobFinishedMessage = JobFinishedMessageV1 | JobFinishedMessageV2;

/** @deprecated Old format without execution result details. */
type JobFinishedMessageV1 = {
	kind: 'job-finished';
	version?: undefined;
	executionId: string;
	workerId: string;
	success: boolean;
};

type JobFinishedMessageV2 = {
	kind: 'job-finished';
	version: 2;
	executionId: string;
	workerId: string;
} & JobFinishedProps;

export type SendChunkMessage = {
	kind: 'send-chunk';
	executionId: string;
	chunkText: StructuredChunk;
	workerId: string;
};

/** Message sent by worker to main to report a job has failed. */
Repository: n8n-io/n8n
File: packages/cli/src/workflows/workflow.service.ee.ts (TypeScript)

Code before:
						workflowId: workflow.id,
						projectId: destinationProjectId,
						role: 'workflow:owner',
					}),
				);
			}
		});
	}

	private async shareCredentialsWithProject(
		user: User,
		credentialIds: string[],
		projectId: string,
	) {
		await this.workflowRepository.manager.transaction(async (trx) => {
			let credentialIdsToShare: string[];

			if (hasGlobalScope(user, ['credential:share'], { mode: 'allOf' })) {
				credentialIdsToShare = credentialIds;
			} else {
				const accessibleIds = await this.credentialsFinderService.getCredentialIdsByUserAndRole(
					[user.id],
					{ scopes: ['credential:share'] },
					trx,
				);
				credentialIdsToShare = credentialIds.filter((id) => accessibleIds.includes(id));
			}

			for (const credentialId of credentialIdsToShare) {
				await this.enterpriseCredentialsService.shareWithProjects(
					user,
					credentialId,
					[projectId],
					trx,
				);
			}
		});
	}

	private async moveFoldersToDestination(
		sourceFolderId: string,
		childrenFolderIds: string[],
		destinationProjectId: string,
		destinationParentFolderId: string,
	) {
		await this.folderRepository.manager.transaction(async (trx) => {
			// Move all children folders to the destination project
			await trx.update(
				Folder,
				{ id: In(childrenFolderIds) },
				{ homeProject: { id: destinationProjectId } },

Review comment (security):
This is a linear search for every credential. Can add up if there are a lot of credentials. A `Set` would be more appropriate

Code after:
						workflowId: workflow.id,
						projectId: destinationProjectId,
						role: 'workflow:owner',
					}),
				);
			}
		});
	}

	private async shareCredentialsWithProject(
		user: User,
		credentialIds: string[],
		projectId: string,
	) {
		await this.workflowRepository.manager.transaction(async (trx) => {
			let credentialIdsToShare: string[];

			if (hasGlobalScope(user, ['credential:share'], { mode: 'allOf' })) {
				credentialIdsToShare = credentialIds;
			} else {
				const accessibleIds = new Set(
					await this.credentialsFinderService.getCredentialIdsByUserAndRole(
						[user.id],
						{ scopes: ['credential:share'] },
						trx,
					),
				);
				credentialIdsToShare = credentialIds.filter((id) => accessibleIds.has(id));
			}

			for (const credentialId of credentialIdsToShare) {
				await this.enterpriseCredentialsService.shareWithProjects(
					user,
					credentialId,
					[projectId],
					trx,
				);
			}
		});
	}

	private async moveFoldersToDestination(
		sourceFolderId: string,
		childrenFolderIds: string[],
		destinationProjectId: string,
		destinationParentFolderId: string,
	) {
		await this.folderRepository.manager.transaction(async (trx) => {
			// Move all children folders to the destination project
			await trx.update(
				Folder,