ronantakizawa/codereview-bench huggingface dataset:
Repository: n8n-io/n8n
File: packages/cli/src/events/relays/telemetry.event-relay.ts (TypeScript)

Code before:
		this.telemetry.track('User updated external secrets settings', {
			user_id: userId,
			vault_type: vaultType,
			is_valid: isValid,
			is_new: isNew,
			error_message: errorMessage,
		});
	}

	private externalSecretsProviderReloaded({
		vaultType,
	}: RelayEventMap['external-secrets-provider-reloaded']) {
		this.telemetry.track('User reloaded external secrets', {
			vault_type: vaultType,
		});
	}

	private externalSecretsConnectionCreated({
		userId,
		providerKey,
		vaultType,
		projects,
	}: RelayEventMap['external-secrets-connection-created']) {
		this.telemetry.track('User created external secrets connection', {
			user_id: userId,
			provider_key: providerKey,
			vault_type: vaultType,
			scope: projects.length === 0 ? 'global' : 'project',
			project_ids: projects.map((project) => project.id),
		});
	}

	private externalSecretsConnectionUpdated({
		userId,
		providerKey,
		vaultType,
		projects,
	}: RelayEventMap['external-secrets-connection-updated']) {
		this.telemetry.track('User updated external secrets connection', {
			user_id: userId,
			provider_key: providerKey,
			vault_type: vaultType,
			scope: projects.length === 0 ? 'global' : 'project',
			project_ids: projects.map((project) => project.id),
		});
	}

	private externalSecretsConnectionDeleted({
		userId,
		providerKey,
		vaultType,

Review comment (suggestion):
I would be vary to send the providerKey to our telemetry data. I can't come up with a telemetry question that requires us to know this, and I think it's good to only include properties where we know we need them, e.g. `vault_type` and `scope`

Code after:
		this.telemetry.track('User updated external secrets settings', {
			user_id: userId,
			vault_type: vaultType,
			is_valid: isValid,
			is_new: isNew,
			error_message: errorMessage,
		});
	}

	private externalSecretsProviderReloaded({
		vaultType,
	}: RelayEventMap['external-secrets-provider-reloaded']) {
		this.telemetry.track('User reloaded external secrets', {
			vault_type: vaultType,
		});
	}

	private externalSecretsConnectionCreated({
		userId,
		vaultType,
		projects,
	}: RelayEventMap['external-secrets-connection-created']) {
		this.telemetry.track('User created external secrets connection', {
			user_id: userId,
			vault_type: vaultType,
			scope: projects.length === 0 ? 'global' : 'project',
			project_ids: projects.map((project) => project.id),
		});
	}

	private externalSecretsConnectionUpdated({
		userId,
		vaultType,
		projects,
	}: RelayEventMap['external-secrets-connection-updated']) {
		this.telemetry.track('User updated external secrets connection', {
			user_id: userId,
			vault_type: vaultType,
			scope: projects.length === 0 ? 'global' : 'project',
			project_ids: projects.map((project) => project.id),
		});
	}

	private externalSecretsConnectionDeleted({
		userId,
		vaultType,
		projects,
	}: RelayEventMap['external-secrets-connection-deleted']) {
		this.telemetry.track('User deleted external secrets connection', {
			user_id: userId,
			vault_type: vaultType,
Repository: n8n-io/n8n
File: packages/cli/src/modules/quick-connect/quick-connect.config.ts (TypeScript)

Code before:
import { Config, Env } from '@n8n/config';
import { z } from 'zod';

const backendFlowConfigSchema = z.object({
	secret: z.string(),
});

export type BackendFlowConfig = z.infer<typeof backendFlowConfigSchema>;

const quickConnectOptionSchema = z.object({
	packageName: z.string(),
	credentialType: z.string(),
	text: z.string(),
	quickConnectType: z.string(),
	serviceName: z.string(),
	consentText: z.string().optional(),
	config: z.record(z.string(), z.string()).optional(),
	backendFlowConfig: backendFlowConfigSchema.optional(),
});

export type QuickConnectOption = z.infer<typeof quickConnectOptionSchema>;

const quickConnectBackendOptionSchema = quickConnectOptionSchema.required({
	backendFlowConfig: true,
});

export type QuickConnectBackendOption = z.infer<typeof quickConnectBackendOptionSchema>;

const quickConnectOptionsSchema = z.string().pipe(
	z.preprocess((input: string) => {
		try {
			return JSON.parse(input);
		} catch {
			return [];
		}
	}, z.array(quickConnectOptionSchema)),
);

export type QuickConnectOptions = z.infer<typeof quickConnectOptionsSchema>;

@Config
export class QuickConnectConfig {

Review comment (suggestion):
Using an arbitrary `Record<string, string>` type here since we don't know what possible options we will need for other providers

Code after:
import { Config, Env } from '@n8n/config';
import { z } from 'zod';

const backendFlowConfigSchema = z.object({
	secret: z.string(),
});

export type BackendFlowConfig = z.infer<typeof backendFlowConfigSchema>;

const baseQuickConnectOptionSchema = z.object({
	packageName: z.string(),
	credentialType: z.string(),
	text: z.string(),
	quickConnectType: z.string(),
	serviceName: z.string(),
	consentText: z.string().optional(),
	config: z.never().optional(),
	backendFlowConfig: backendFlowConfigSchema.optional(),
});

const pineconeQuickConnectOptionSchema = baseQuickConnectOptionSchema.extend({
	quickConnectType: z.literal('pinecone'),
	config: z.object({
		integrationId: z.string(),
	}),
});

const quickConnectOptionSchema = z.union([
	pineconeQuickConnectOptionSchema,
	baseQuickConnectOptionSchema,
]);

export type QuickConnectOption = z.infer<typeof quickConnectOptionSchema>;

const quickConnectBackendOptionSchema = baseQuickConnectOptionSchema.required({
	backendFlowConfig: true,
});

export type QuickConnectBackendOption = z.infer<typeof quickConnectBackendOptionSchema>;

const quickConnectOptionsSchema = z.string().pipe(
	z.preprocess((input: string) => {
Repository: n8n-io/n8n
File: packages/@n8n/api-types/src/dto/variables/update-variable-request.dto.ts (TypeScript)

Code before:
import { z } from 'zod';

import { VARIABLE_KEY_MAX_LENGTH, variableTypeSchema, variableValueSchema } from './base.dto';
import { Z } from '../../zod-class';

/**
 * Please not that on the CreateVariableRequestDto we have made this regex
 * stricter by not allowing variable keys to start with a number, as that would break dot notation.
 * The Update DTO still supports it for backwards compatibility.
 */
const VARIABLE_KEY_REGEX = /^[A-Za-z0-9_]+$/;

const variableKeySchema = z
	.string()
	.min(1, 'key must be at least 1 character long')
	.max(VARIABLE_KEY_MAX_LENGTH, 'key cannot be longer than 50 characters')
	.regex(VARIABLE_KEY_REGEX, 'key can only contain characters A-Za-z0-9_');

export class UpdateVariableRequestDto extends Z.class({
	key: variableKeySchema.optional(),
	type: variableTypeSchema.optional(),
	value: variableValueSchema.optional(),
	projectId: z.string().max(36).optional().nullable(),
}) {}


Review comment (suggestion):
```suggestion
 * Please note that on the CreateVariableRequestDto we have made this regex
```

Code after:
import { z } from 'zod';

import { VARIABLE_KEY_MAX_LENGTH, variableTypeSchema, variableValueSchema } from './base.dto';
import { Z } from '../../zod-class';

/**
 * Please note that on the CreateVariableRequestDto we have made this regex
 * stricter by not allowing variable keys to start with a number, as that would break dot notation.
 * The Update DTO still supports it for backwards compatibility.
 */
const VARIABLE_KEY_REGEX = /^[A-Za-z0-9_]+$/;

const variableKeySchema = z
	.string()
	.min(1, 'key must be at least 1 character long')
	.max(VARIABLE_KEY_MAX_LENGTH, 'key cannot be longer than 50 characters')
	.regex(VARIABLE_KEY_REGEX, 'key can only contain characters A-Za-z0-9_');

export class UpdateVariableRequestDto extends Z.class({
	key: variableKeySchema.optional(),
	type: variableTypeSchema.optional(),
	value: variableValueSchema.optional(),
	projectId: z.string().max(36).optional().nullable(),
}) {}
Repository: n8n-io/n8n
File: packages/frontend/@n8n/design-system/src/components/N8nButton/Button.vue (Vue)

Code before:
	cursor: pointer;
	text-decoration: none;

	height: var(--button--height);
	padding: var(--button--padding);
	border-radius: var(--button--radius);
	font-size: var(--button--font-size);

	--button--color--background: transparent;
	--button--color--background-hover: transparent;
	--button--color--background-active: transparent;
	--button--color: light-dark(var(--color--neutral-900), var(--color--neutral-100));
	--button--shadow: none;
	--button--shadow--hover: none;
	--button--shadow--active: none;
	--button--border-color: transparent;
	--button--border-color--hover: transparent;
	--button--border-color--active: transparent;

	background-color: var(--button--color--background);
	color: var(--button--color);
	box-shadow: var(--button--shadow);
	border: 1px solid var(--button--border-color);

	> * {
		grid-area: 1 / 1;
	}

	&:hover {
		background-color: var(--button--color--background-hover);
		box-shadow: var(--button--shadow--hover);
		border-color: var(--button--border-color--hover);
	}

	&:active {
		background-color: var(--button--color--background-active);
		box-shadow: var(--button--shadow--active);
		border-color: var(--button--border-color--active);
	}

	&:focus {
		outline: none;
	}

	&:focus-visible {
		@include focus.focus-ring;
	}

	&.xsmall {
		--button--height: 1.5rem;
		--button--padding: 0 var(--spacing--2xs);

Review comment (suggestion):
FYI Removing border and putting it in `box-shadow` stops this weird big when using `grid` where the border pushes the children 1px out-of-line 🤷 

Code after:
	cursor: pointer;
	text-decoration: none;

	height: var(--button--height);
	padding: var(--button--padding);
	border-radius: var(--button--radius);
	font-size: var(--button--font-size);

	--button--color--background: transparent;
	--button--color--background-hover: transparent;
	--button--color--background-active: transparent;
	--button--color: light-dark(var(--color--neutral-900), var(--color--neutral-100));
	--button--shadow: 0 0 0 0 transparent;
	--button--shadow--hover: 0 0 0 0 transparent;
	--button--shadow--active: 0 0 0 0 transparent;
	--button--border--shadow: 0 0 0 0 transparent;
	--button--border--shadow--hover: 0 0 0 0 transparent;
	--button--border--shadow--active: 0 0 0 0 transparent;
	--button--border-color: transparent;
	--button--border-color--hover: transparent;
	--button--border-color--active: transparent;

	background-color: var(--button--color--background);
	color: var(--button--color);
	box-shadow: var(--button--shadow), var(--button--border--shadow);
	border: none;

	> * {
		grid-area: 1 / 1;
	}

	&:hover {
		background-color: var(--button--color--background-hover);
		box-shadow: var(--button--shadow--hover), var(--button--border--shadow--hover);
	}

	&:active {
		background-color: var(--button--color--background-active);
		box-shadow: var(--button--shadow--active), var(--button--border--shadow--active);
	}

	&:focus {
		outline: none;
	}

	&:focus-visible {
		@include focus.focus-ring;
	}

	&.xsmall {
		--button--height: 1.5rem;
Repository: n8n-io/n8n
File: packages/frontend/editor-ui/src/features/ai/chatHub/components/ToolsSelector.vue (Vue)

Code before:

const CREATE_NEW_TOOL_ID = 'action::create-new';

const menuItems = computed<ToolMenuItem[]>(() => {
	const query = searchQuery.value.toLowerCase();

	const toolItems: ToolMenuItem[] = chatStore.configuredTools
		.filter((tool) => {
			if (!query) return true;
			const def = tool.definition;
			const nodeType = nodeTypesStore.getNodeType(def.type, def.typeVersion);
			const nameMatch = def.name.toLowerCase().includes(query);
			const typeMatch = nodeType?.displayName.toLowerCase().includes(query);
			return nameMatch || typeMatch;
		})
		.map((tool) => ({
			id: `tool::${tool.definition.id}`,
			label: tool.definition.name,
			checked: checkedToolIdsSet.value.has(tool.definition.id),
			data: {
				nodeType: nodeTypesStore.getNodeType(tool.definition.type, tool.definition.typeVersion),
				tool: tool.definition,
			},
		}));

	toolItems.push({
		id: CREATE_NEW_TOOL_ID,
		label: i18n.baseText('chatHub.tools.selector.createNew'),
		icon: { type: 'icon', value: 'plus' },
	});

	return toolItems;
});

function handleSelect(id: string) {
	if (id === CREATE_NEW_TOOL_ID) {
		openToolsManager();
		return;
	}

	const [command, toolId] = id.split('::');

	if (command === 'tool') {
		emit('toggle', toolId);
	}
}

function handleSearch(query: string) {
	searchQuery.value = query;
}


Review comment (suggestion):
Model selector has the divider for menu items like this, maybe do the same here?

<img width="277" height="427" alt="Image" src="https://github.com/user-attachments/assets/8389979e-42cd-4ac2-841c-0a508e072fcf" />

Code after:

const CREATE_NEW_TOOL_ID = 'action::create-new';

const menuItems = computed<ToolMenuItem[]>(() => {
	const query = searchQuery.value.toLowerCase();

	const toolItems: ToolMenuItem[] = chatStore.configuredTools
		.filter((tool) => {
			if (!query) return true;
			const def = tool.definition;
			const nodeType = nodeTypesStore.getNodeType(def.type, def.typeVersion);
			const nameMatch = def.name.toLowerCase().includes(query);
			const typeMatch = nodeType?.displayName.toLowerCase().includes(query);
			return nameMatch || typeMatch;
		})
		.map((tool) => ({
			id: `tool::${tool.definition.id}`,
			label: tool.definition.name,
			checked: checkedToolIdsSet.value.has(tool.definition.id),
			data: {
				nodeType: nodeTypesStore.getNodeType(tool.definition.type, tool.definition.typeVersion),
				tool: tool.definition,
			},
		}));

	toolItems.push({
		id: CREATE_NEW_TOOL_ID,
		label: i18n.baseText('chatHub.tools.selector.createNew'),
		icon: { type: 'icon', value: 'plus' },
		divided: true,
	});

	return toolItems;
});

function handleSelect(id: string) {
	if (id === CREATE_NEW_TOOL_ID) {
		openToolsManager();
		return;
	}

	const [command, toolId] = id.split('::');

	if (command === 'tool') {
		emit('toggle', toolId);
	}
}

function handleSearch(query: string) {
	searchQuery.value = query;
}
Repository: n8n-io/n8n
File: packages/frontend/editor-ui/src/features/credentials/components/CredentialEdit/CredentialEdit.vue (Vue)

Code before:
			</div>
		</template>
	</Modal>
</template>

<style module lang="scss">
.credentialModal {
	--dialog--max-width: 1200px;
	--dialog--close--spacing--top: 36px;
	--dialog--max-height: 750px;

	:global(.el-dialog__header) {
		padding-bottom: 0;
		border-bottom: var(--border);
	}

	:global(.el-dialog__body) {
		padding-top: var(--spacing--lg);
		position: relative;
	}
}

.mainContent {
	flex: 1;
	overflow: auto;
	padding-left: var(--spacing--5xs);
	padding-bottom: 100px;
}

.credName {
	display: flex;
	width: 100%;
	flex-direction: column;
	gap: var(--spacing--4xs);
}

.credNameRow {
	display: flex;
	align-items: center;
	gap: var(--spacing--2xs);
	min-height: var(--spacing--md);
}

.dynamicTagContent {
	display: inline-flex;
	align-items: center;
	gap: var(--spacing--5xs);
}

.sidebar {
	max-width: 170px;

Review comment (bug):
Kind of a hacky solution, I tried a bunch of things but it's pretty mysterious.

It's definitely connected the the `overflow: auto` on the parent container.

Code after:
					<CredentialInfo :current-credential="currentCredential" />
				</div>
			</div>
		</template>
	</Modal>
</template>

<style module lang="scss">
.credentialModal {
	--dialog--max-width: 1200px;
	--dialog--close--spacing--top: 36px;
	--dialog--max-height: 750px;

	:global(.el-dialog__header) {
		padding-bottom: 0;
		border-bottom: var(--border);
	}

	:global(.el-dialog__body) {
		padding-top: var(--spacing--lg);
		position: relative;
	}
}

.mainContent {
	flex: 1;
	overflow: auto;
	padding-bottom: 100px;
}

.credName {
	display: flex;
	width: 100%;
	flex-direction: column;
	gap: var(--spacing--4xs);
}

.credNameRow {
	display: flex;
	align-items: center;
	gap: var(--spacing--2xs);
	min-height: var(--spacing--md);
}

.dynamicTagContent {
	display: inline-flex;
	align-items: center;
	gap: var(--spacing--5xs);
}

.sidebar {
Repository: n8n-io/n8n
File: packages/frontend/editor-ui/src/features/credentials/components/CredentialEdit/CredentialEdit.vue (Vue)

Code before:
			</div>
		</template>
	</Modal>
</template>

<style module lang="scss">
.credentialModal {
	--dialog--max-width: 1200px;
	--dialog--close--spacing--top: 36px;
	--dialog--max-height: 750px;

	:global(.el-dialog__header) {
		padding-bottom: 0;
		border-bottom: var(--border);
	}

	:global(.el-dialog__body) {
		padding-top: var(--spacing--lg);
		position: relative;
	}
}

.mainContent {
	flex: 1;
	overflow: auto;
	padding-left: 1px;
	padding-bottom: 100px;
}

.credName {
	display: flex;
	width: 100%;
	flex-direction: column;
	gap: var(--spacing--4xs);
}

.credNameRow {
	display: flex;
	align-items: center;
	gap: var(--spacing--2xs);
	min-height: var(--spacing--md);
}

.dynamicTagContent {
	display: inline-flex;
	align-items: center;
	gap: var(--spacing--5xs);
}

.sidebar {
	max-width: 170px;

Review comment (suggestion):
I've just realized that here the padding is added to `mainContent`, this will also offset all of the content like input fields and the input fields will be 1px to the right compared to the button. I also did not find where the problem comes from, but the padding should only be there for the button and not other elements

Code after:
					<CredentialInfo :current-credential="currentCredential" />
				</div>
			</div>
		</template>
	</Modal>
</template>

<style module lang="scss">
.credentialModal {
	--dialog--max-width: 1200px;
	--dialog--close--spacing--top: 36px;
	--dialog--max-height: 750px;

	:global(.el-dialog__header) {
		padding-bottom: 0;
		border-bottom: var(--border);
	}

	:global(.el-dialog__body) {
		padding-top: var(--spacing--lg);
		position: relative;
	}
}

.mainContent {
	flex: 1;
	overflow: auto;
	padding-bottom: 100px;
}

.credName {
	display: flex;
	width: 100%;
	flex-direction: column;
	gap: var(--spacing--4xs);
}

.credNameRow {
	display: flex;
	align-items: center;
	gap: var(--spacing--2xs);
	min-height: var(--spacing--md);
}

.dynamicTagContent {
	display: inline-flex;
	align-items: center;
	gap: var(--spacing--5xs);
}

.sidebar {
Repository: n8n-io/n8n
File: packages/frontend/@n8n/chat/src/components/MessageWithButtons.vue (Vue)

Code before:
<script setup lang="ts">
import { ref } from 'vue';

import Button from './Button.vue';
import MarkdownRenderer from './MarkdownRenderer.vue';

defineProps<{
	text: string;
	buttons: Array<{
		text: string;
		link: string;
		type: 'primary' | 'secondary';
	}>;
}>();

const clickedButtonIndex = ref<number | null>(null);

const isSameDomain = (link: string): boolean => {
	try {
		const url = new URL(link, window.location.href);
		return url.origin === window.location.origin;
	} catch {
		return false;
	}
};

const onClick = async (link: string, index: number) => {
	if (clickedButtonIndex.value !== null) {
		return;
	}

	const response = await fetch(link);
	if (response.ok) {
		clickedButtonIndex.value = index;
	}
};
</script>

<template>
	<div>
		<MarkdownRenderer :text="text" />
		<div :class="$style.buttons">
			<template v-for="(button, index) in buttons" :key="button.text">
				<Button
					v-if="
						isSameDomain(button.link) &&

Review comment (suggestion):
Can those urls include n8n webhook urls? Afaik buttons usually can lead to `form-waiting` or `form` endpoint.
Those endpoints can be modified by some env variable(don't remember the name) and it will be different from frontend url. Users may use a proxy for webhooks, which can lead to button not rendering even if it's technically their domain

Code after:
<script setup lang="ts">
import { ref } from 'vue';

import { useOptions } from '@n8n/chat/composables';

import Button from './Button.vue';
import MarkdownRenderer from './MarkdownRenderer.vue';

defineProps<{
	text: string;
	buttons: Array<{
		text: string;
		link: string;
		type: 'primary' | 'secondary';
	}>;
}>();

const chatOptions = useOptions();
const clickedButtonIndex = ref<number | null>(null);

const isButtonVisible = (link: string, index: number): boolean => {
	try {
		const validOrigin = new URL(chatOptions.options.webhookUrl).origin;
		const url = new URL(link, window.location.href);
		if (url.origin !== validOrigin) {
			return false;
		}
		return clickedButtonIndex.value === null || index === clickedButtonIndex.value;
	} catch {
		return false;
	}
};

const onClick = async (link: string, index: number) => {
	if (clickedButtonIndex.value !== null) {
		return;
	}

	const response = await fetch(link);
	if (response.ok) {
		clickedButtonIndex.value = index;
	}
};
</script>

<template>
Repository: n8n-io/n8n
File: packages/frontend/@n8n/chat/src/components/MessageWithButtons.vue (Vue)

Code before:
		const url = new URL(link, window.location.href);
		return validOrigins.includes(url.origin);
	} catch {
		return false;
	}
};

const onClick = async (link: string, index: number) => {
	if (clickedButtonIndex.value !== null) {
		return;
	}

	const response = await fetch(link);
	if (response.ok) {
		clickedButtonIndex.value = index;
	}
};
</script>

<template>
	<div>
		<MarkdownRenderer :text="text" />
		<div :class="$style.buttons">
			<template v-for="(button, index) in buttons" :key="button.text">
				<Button
					v-if="
						isValidOrigin(button.link) &&
						(clickedButtonIndex === null || index === clickedButtonIndex)
					"
					element="button"
					:type="button.type"
					:disabled="index === clickedButtonIndex"
					@click="onClick(button.link, index)"
					>{{ button.text }}</Button
				>
			</template>
		</div>
	</div>
</template>

<style lang="scss" module>
.buttons {
	display: flex;
	gap: var(--chat--spacing);
	margin-top: var(--chat--spacing);
}
</style>


Review comment (suggestion):
Would be nice to have `clickedButtonIndex === null || index === clickedButtonIndex` as a variable with a clear name, because it's not clear what is it for

Code after:
		const url = new URL(link, window.location.href);
		if (url.origin !== validOrigin) {
			return false;
		}
		return clickedButtonIndex.value === null || index === clickedButtonIndex.value;
	} catch {
		return false;
	}
};

const onClick = async (link: string, index: number) => {
	if (clickedButtonIndex.value !== null) {
		return;
	}

	const response = await fetch(link);
	if (response.ok) {
		clickedButtonIndex.value = index;
	}
};
</script>

<template>
	<div>
		<MarkdownRenderer :text="text" />
		<div :class="$style.buttons">
			<template v-for="(button, index) in buttons" :key="button.text">
				<Button
					v-if="isButtonVisible(button.link, index)"
					element="button"
					:type="button.type"
					:disabled="index === clickedButtonIndex"
					@click="onClick(button.link, index)"
					>{{ button.text }}</Button
				>
			</template>
		</div>
	</div>
</template>

<style lang="scss" module>
.buttons {
	display: flex;
	gap: var(--chat--spacing);
	margin-top: var(--chat--spacing);
}
</style>