ronantakizawa/codereview-bench huggingface dataset:
Repository: n8n-io/n8n
File: packages/core/src/observability/tracing/tracing.ts (TypeScript)

Code before:
export type StartSpanOpts = SentryStartSpanOptions;
export type Span = Sentry.Span;
export type SpanContextData = SentrySpanContextData;
export type SpanAttributes = SentrySpanAttributes;

/**
 * Interface for concrete tracing implementations
 */
export interface Tracer {
	startSpan<T>(options: StartSpanOpts, spanCb: (span: Span) => Promise<T>): Promise<T>;
}

const COMMON_TRACE_ATTRIBUTES = {
	workflow: {
		id: 'n8n.workflow.id',
		name: 'n8n.workflow.name',
	},
	node: {
		id: 'n8n.node.id',
		name: 'n8n.node.name',
		type: 'n8n.node.type',
		typeVersion: 'n8n.node.type_version',
	},
} as const;

export const enum SpanStatus {
	ok = SPAN_STATUS_OK,
	error = SPAN_STATUS_ERROR,
}

/**
 * Class to instrument the application with tracing. This class is a
 * singleton that delegates to the actual tracing implementation. The
 * tracing is active only if Sentry has been configured and tracing sampling
 * is enabled.
 *
 * @example
 * ```ts
 * Tracing.startSpan({
 * 	 name: "My Operation",
 *   attributes: {
 *     [Tracing.commonAttrs.workflow.id]: workflow.id,
 *   }
 * }, async (span) => {
 *   // Do the operation that is then traced
 * });
 * ```
 */
@Service()
export class Tracing {
	private tracer: Tracer = new NoopTracing();

Review comment (suggestion):
<img width="1432" height="882" alt="Capture 2026-02-03 at 10 39 31@2x" src="https://github.com/user-attachments/assets/6ba79ef4-ae0c-4359-85e8-67b4c3410979" />


Code after:
export type SpanContextData = SentrySpanContextData;
export type SpanAttributes = SentrySpanAttributes;

/**
 * Interface for concrete tracing implementations
 */
export interface Tracer {
	startSpan<T>(options: StartSpanOpts, spanCb: (span: Span) => Promise<T>): Promise<T>;
}

const COMMON_TRACE_ATTRIBUTES = {
	workflow: {
		id: 'n8n.workflow.id',
		name: 'n8n.workflow.name',
	},
	node: {
		id: 'n8n.node.id',
		name: 'n8n.node.name',
		type: 'n8n.node.type',
		typeVersion: 'n8n.node.type_version',
	},
} as const;

export const enum SpanStatus {
	ok = 1,
	error = 2,
}

/**
 * Class to instrument the application with tracing. This class is a
 * singleton that delegates to the actual tracing implementation. The
 * tracing is active only if Sentry has been configured and tracing sampling
 * is enabled.
 *
 * @example
 * ```ts
 * Tracing.startSpan({
 * 	 name: "My Operation",
 *   attributes: {
 *     [Tracing.commonAttrs.workflow.id]: workflow.id,
 *   }
 * }, async (span) => {
 *   // Do the operation that is then traced
 * });
 * ```
 */
@Service()
export class Tracing {
	private tracer: Tracer = new NoopTracing();

	/** Common n8n specific attribute names */
Repository: n8n-io/n8n
File: packages/@n8n/db/src/repositories/execution.repository.ts (TypeScript)

Code before:
		status: true,
		createdAt: true,
		startedAt: true,
		stoppedAt: true,
	};

	private annotationFields = {
		id: true,
		vote: true,
	};

	/**
	 * This function reduces duplicate rows in the raw result set of the query builder from *toQueryBuilderWithAnnotations*
	 * by merging the tags of the same execution annotation.
	 */
	private reduceExecutionsWithAnnotations(
		rawExecutionsWithTags: Array<
			ExecutionSummary & {
				annotation_id: number;
				annotation_vote: AnnotationVote;
				annotation_tags_id: string;
				annotation_tags_name: string;
			}
		>,
	) {
		const map = new Map<string, ExecutionSummary>();

		for (const {
			annotation_id: _,
			annotation_vote: vote,
			annotation_tags_id: tagId,
			annotation_tags_name: tagName,
			...row
		} of rawExecutionsWithTags) {
			let execution = map.get(row.id);
			if (!execution) {
				execution = {
					...row,
					annotation: { vote, tags: tagId ? [{ id: tagId, name: tagName }] : [] },
				};
				map.set(row.id, execution);
			} else if (tagId) {
				execution.annotation = execution.annotation ?? {
					vote,
					tags: [] as Array<{ id: string; name: string }>,
				};
				execution.annotation.tags.push({ id: tagId, name: tagName });
			}
		}

		return [...map.values()];

Review comment (suggestion):
nit: Would be more descriptive
```suggestion
		const summariesById = new Map<string, ExecutionSummary>();
```

Code after:
		status: true,
		createdAt: true,
		startedAt: true,
		stoppedAt: true,
	};

	private annotationFields = {
		id: true,
		vote: true,
	};

	/**
	 * This function reduces duplicate rows in the raw result set of the query builder from *toQueryBuilderWithAnnotations*
	 * by merging the tags of the same execution annotation.
	 */
	private reduceExecutionsWithAnnotations(
		rawExecutionsWithTags: Array<
			ExecutionSummary & {
				annotation_id: number;
				annotation_vote: AnnotationVote;
				annotation_tags_id: string;
				annotation_tags_name: string;
			}
		>,
	) {
		const summariesById = new Map<string, ExecutionSummary>();

		for (const {
			annotation_id: _,
			annotation_vote: vote,
			annotation_tags_id: tagId,
			annotation_tags_name: tagName,
			...row
		} of rawExecutionsWithTags) {
			let execution = summariesById.get(row.id);
			if (!execution) {
				execution = {
					...row,
					annotation: {
						vote,
						tags: tagId ? [{ id: tagId, name: tagName }] : [],
					},
				};
				summariesById.set(row.id, execution);
			} else if (tagId) {
				execution.annotation = execution.annotation ?? {
					vote,
					tags: [] as Array<{ id: string; name: string }>,
				};
				execution.annotation.tags.push({ id: tagId, name: tagName });
			}
Repository: n8n-io/n8n
File: packages/testing/playwright/tests/e2e/sentry/sentry-baseline.spec.ts (TypeScript)

Code before:
 */

test.use({ capability: 'kent' });

test.beforeEach(async ({ n8nContainer }) => {
	await n8nContainer.services.kent.clear();
});

test.describe('Sentry baseline', () => {
	test('frontend error is captured', async ({ n8n, n8nContainer }) => {
		const kent = n8nContainer.services.kent;
		await n8n.navigate.toHome();

		// Trigger unhandled error
		n8n.page.on('pageerror', () => {}); // Suppress Playwright error
		await n8n.page.evaluate(() => {
			setTimeout(() => {
				throw new Error('Test frontend error');
			}, 0);
		});

		// Poll until Kent captures the frontend error
		await expect
			.poll(
				async () => {
					const events = await kent.getEvents();
					return events.find(
						(e) =>
							kent.getSource(e) === 'frontend' &&
							kent.getType(e) === 'error' &&
							kent.getErrorMessage(e).includes('Test frontend error'),
					);
				},
				{ timeout: 10000 },
			)
			.toBeTruthy();
	});

	test('backend transaction is captured', async ({ n8n, n8nContainer }) => {
		const kent = n8nContainer.services.kent;
		await n8n.navigate.toHome();

		// Poll until Kent captures a backend transaction
		await expect
			.poll(
				async () => {
					const events = await kent.getEvents();
					return events.find(
						(e) => kent.getSource(e) === 'backend' && kent.getType(e) === 'transaction',
					);
				},

Review comment (suggestion):
Would be nice if the `getEvents` supported these filters directly, or have a separate method like `getEventsByFilter`

Code after:
	await n8nContainer.services.kent.clear();
});

test.describe('Sentry baseline', () => {
	test('frontend error is captured', async ({ n8n, n8nContainer }) => {
		const kent = n8nContainer.services.kent;
		await n8n.navigate.toHome();

		n8n.page.on('pageerror', () => {});
		await n8n.page.evaluate(() => {
			setTimeout(() => {
				throw new Error('Test frontend error');
			}, 0);
		});

		await expect
			.poll(
				async () =>
					await kent.getEvents({
						source: 'frontend',
						type: 'error',
						messageContains: 'Test frontend error',
					}),
				{ timeout: 10000 },
			)
			.toHaveLength(1);
	});

	test('backend transaction is captured', async ({ n8n, n8nContainer }) => {
		const kent = n8nContainer.services.kent;
		await n8n.navigate.toHome();

		await expect
			.poll(async () => await kent.getEvents({ source: 'backend', type: 'transaction' }), {
				timeout: 10000,
			})
			.not.toHaveLength(0);
	});

	test('events have deployment identification via server_name tag', async ({
		n8n,
		n8nContainer,
	}) => {
		const kent = n8nContainer.services.kent;
		await n8n.navigate.toHome();

		n8n.page.on('pageerror', () => {});
		await n8n.page.evaluate(() => {
			setTimeout(() => {
				throw new Error('Deployment test error');
			}, 0);
Repository: n8n-io/n8n
File: packages/cli/src/public-api/v1/handlers/executions/executions.handler.ts (TypeScript)

Code before:

			if (!execution) {
				return res.status(404).json({ message: 'Not Found' });
			}

			const annotation = await Container.get(ExecutionAnnotationRepository).findOne({
				where: { execution: { id } },
				relations: ['tags'],
			});

			const tags = (annotation?.tags ?? []).map(({ id: tagId, name, createdAt, updatedAt }) => ({
				id: tagId,
				name,
				createdAt,
				updatedAt,
			}));

			return res.json(tags);
		},
	],
	updateExecutionTags: [
		apiKeyHasScope('executionTags:update'),
		async (req: ExecutionRequest.UpdateTags, res: express.Response): Promise<express.Response> => {
			const { id } = req.params;
			const newTagIds = req.body.map((tag) => tag.id);
			const sharedWorkflowsIds = await getSharedWorkflowIds(req.user, ['workflow:read']);

			if (!sharedWorkflowsIds.length) {
				return res.status(404).json({ message: 'Not Found' });
			}

			const execution = await Container.get(
				ExecutionRepository,
			).getExecutionInWorkflowsForPublicApi(id, sharedWorkflowsIds, false);

			if (!execution) {
				return res.status(404).json({ message: 'Not Found' });
			}

			// Upsert annotation (create if it doesn't exist)
			await Container.get(ExecutionAnnotationRepository).upsert({ execution: { id } }, [
				'execution',
			]);

			const annotation = await Container.get(ExecutionAnnotationRepository).findOneOrFail({
				where: { execution: { id } },
			});

			try {
				await Container.get(AnnotationTagMappingRepository).overwriteTags(annotation.id, newTagIds);
			} catch (error) {

Review comment (suggestion):
It should be `workflow:update` than `workflow:read` here.

Code after:
			}

			const tags = await getExecutionTags(id);

			return res.json(tags);
		},
	],
	updateExecutionTags: [
		apiKeyHasScope('executionTags:update'),
		async (req: ExecutionRequest.UpdateTags, res: express.Response): Promise<express.Response> => {
			const { id } = req.params;
			const newTagIds = req.body.map((tag) => tag.id);
			const sharedWorkflowsIds = await getSharedWorkflowIds(req.user, ['workflow:update']);

			if (!sharedWorkflowsIds.length) {
				return res.status(404).json({ message: 'Not Found' });
			}

			const execution = await Container.get(
				ExecutionRepository,
			).getExecutionInWorkflowsForPublicApi(id, sharedWorkflowsIds, false);

			if (!execution) {
				return res.status(404).json({ message: 'Not Found' });
			}

			try {
				const updatedTags = await updateExecutionTags(id, newTagIds);
				const tags = mapAnnotationTags(updatedTags);
				return res.json(tags);
			} catch (error) {
				if (error instanceof QueryFailedError) {
					return res.status(404).json({ message: 'Some tags not found' });
				}
				throw error;
			}
		},
	],
};
Repository: n8n-io/n8n
File: packages/cli/src/public-api/types.ts (TypeScript)

Code before:
		offset?: number;
		lastId?: string;
	}
>;
export declare namespace ExecutionRequest {
	type GetAll = AuthenticatedRequest<
		{},
		{},
		{},
		{
			status?: ExecutionStatus;
			limit?: number;
			cursor?: string;
			offset?: number;
			includeData?: boolean;
			workflowId?: string;
			lastId?: string;
			projectId?: string;
		}
	>;

	type Get = AuthenticatedRequest<{ id: string }, {}, {}, { includeData?: boolean }>;
	type Delete = Get;
	type Retry = AuthenticatedRequest<{ id: string }, {}, { loadWorkflow?: boolean }, {}>;
	type GetTags = AuthenticatedRequest<{ id: string }>;
	type UpdateTags = AuthenticatedRequest<{ id: string }, {}, AnnotationTagEntity[]>;
}

export declare namespace TagRequest {
	type GetAll = AuthenticatedRequest<
		{},
		{},
		{},
		{
			limit?: number;
			cursor?: string;
			offset?: number;
		}
	>;

	type Create = AuthenticatedRequest<{}, {}, TagEntity>;
	type Get = AuthenticatedRequest<{ id: string }>;
	type Delete = Get;
	type Update = AuthenticatedRequest<{ id: string }, {}, TagEntity>;
}

export declare namespace CredentialTypeRequest {
	type Get = AuthenticatedRequest<{ credentialTypeName: string }, {}, {}, {}>;
}

export declare namespace WorkflowRequest {

Review comment (security):
Can we define the UpdateTags type as 
```
type UpdateTags = AuthenticatedRequest<{ id: string }, {}, Array<{ id: string }>>;
```

We only use the id property from AnnotationTagEntity. So, it's better if we use an Array of strings instead

Code after:
		offset?: number;
		lastId?: string;
	}
>;
export declare namespace ExecutionRequest {
	type GetAll = AuthenticatedRequest<
		{},
		{},
		{},
		{
			status?: ExecutionStatus;
			limit?: number;
			cursor?: string;
			offset?: number;
			includeData?: boolean;
			workflowId?: string;
			lastId?: string;
			projectId?: string;
		}
	>;

	type Get = AuthenticatedRequest<{ id: string }, {}, {}, { includeData?: boolean }>;
	type Delete = Get;
	type Retry = AuthenticatedRequest<{ id: string }, {}, { loadWorkflow?: boolean }, {}>;
	type GetTags = AuthenticatedRequest<{ id: string }>;
	type UpdateTags = AuthenticatedRequest<{ id: string }, {}, Array<{ id: string }>>;
}

export declare namespace TagRequest {
	type GetAll = AuthenticatedRequest<
		{},
		{},
		{},
		{
			limit?: number;
			cursor?: string;
			offset?: number;
		}
	>;

	type Create = AuthenticatedRequest<{}, {}, TagEntity>;
	type Get = AuthenticatedRequest<{ id: string }>;
	type Delete = Get;
	type Update = AuthenticatedRequest<{ id: string }, {}, TagEntity>;
}

export declare namespace CredentialTypeRequest {
	type Get = AuthenticatedRequest<{ credentialTypeName: string }, {}, {}, {}>;
}

export declare namespace WorkflowRequest {
Repository: n8n-io/n8n
File: packages/cli/src/modules/quick-connect/handlers/firecrawl.handler.ts (TypeScript)

Code before:

import type { QuickConnectOption } from '../quick-connect.config';
import type { IQuickConnectHandler } from './quick-connect.handler';

const FIRECRAWL_API_BASE_URL = 'https://api.firecrawl.dev';

interface FirecrawlCreateUserResponse {
	apiKey: string;
}

@Service()
export class FirecrawlHandler implements IQuickConnectHandler {
	readonly credentialType = 'firecrawlApi';

	async getCredentialData(
		config: QuickConnectOption,
		user: User,
	): Promise<ICredentialDataDecryptedObject> {
		const secret = config.backendFlowConfig!.secret;
		const response = await axios.post<FirecrawlCreateUserResponse>(
			`${FIRECRAWL_API_BASE_URL}/admin/integration/create-user`,
			{ email: user.email },
			{
				headers: {
					Authorization: `Bearer ${secret}`,
					'Content-Type': 'application/json',
				},
			},
		);

		return {
			apiKey: response.data.apiKey,
		};
	}
}


Review comment (security):
I was originally thinking this would be in the credential class for firecrawl, either as a function or a declarative version like we have for `test` or `authenticate`. I think this way is more than good, just wanted to know your opinion on it.

Code after:

import type { QuickConnectBackendOption } from '../quick-connect.config';
import type { IQuickConnectHandler } from './quick-connect.handler';

const FIRECRAWL_API_BASE_URL = 'https://api.firecrawl.dev';

interface FirecrawlCreateUserResponse {
	apiKey: string;
}

@Service()
export class FirecrawlHandler implements IQuickConnectHandler {
	readonly credentialType = 'firecrawlApi';

	async getCredentialData(
		config: QuickConnectBackendOption,
		user: User,
	): Promise<ICredentialDataDecryptedObject> {
		const secret = config.backendFlowConfig.secret;
		const response = await axios.post<FirecrawlCreateUserResponse>(
			`${FIRECRAWL_API_BASE_URL}/admin/integration/create-user`,
			{ email: user.email },
			{
				headers: {
					Authorization: `Bearer ${secret}`,
					'Content-Type': 'application/json',
				},
			},
		);

		return {
			apiKey: response.data.apiKey,
		};
	}
}
Repository: n8n-io/n8n
File: packages/frontend/editor-ui/src/features/settings/communityNodes/composables/useInstallNode.test.ts (TypeScript)

Code before:
			const { installNode } = useInstallNode();

			const result = await installNode({
				type: 'verified',
				packageName: 'test-package',
				nodeType: 'test-node',
			});

			expect(result.success).toBe(false);
			expect(result.error).toBe(error);
		});

		it('should not track telemetry events when telemetry is not provided', async () => {
			const { installNode } = useInstallNode();
			const { track } = useTelemetry();

			const result = await installNode({
				type: 'unverified',
				packageName: 'test-package',
			});

			expect(result.success).toBe(true);
			expect(track).not.toHaveBeenCalled();
		});

		it('should track telemetry events when telemetry is not provided', async () => {
			const { installNode } = useInstallNode();
			const { track } = useTelemetry();

			const result = await installNode({
				type: 'unverified',
				packageName: 'test-package',
				telemetry: {
					hasQuickConnect: true,
					source: 'node detail view',
				},
			});

			expect(result.success).toBe(true);
			expect(track).toHaveBeenCalledWith('user started cnr package install', {
				input_string: 'test-package',
				has_quick_connect: true,
				source: 'node detail view',
			});
		});
	});

	describe('loading state', () => {
		it('should set loading to true during installation', async () => {
			vi.mocked(communityNodesStore.installPackage).mockImplementation(async () => {
				await new Promise((resolve) => setTimeout(resolve, 10));

Review comment (suggestion):
Small nit:
```suggestion
		it('should track telemetry events when telemetry is provided', async () => {
```

Code after:
			const { installNode } = useInstallNode();

			const result = await installNode({
				type: 'verified',
				packageName: 'test-package',
				nodeType: 'test-node',
			});

			expect(result.success).toBe(false);
			expect(result.error).toBe(error);
		});

		it('should not track telemetry events when telemetry is not provided', async () => {
			const { installNode } = useInstallNode();
			const { track } = useTelemetry();

			const result = await installNode({
				type: 'unverified',
				packageName: 'test-package',
			});

			expect(result.success).toBe(true);
			expect(track).not.toHaveBeenCalled();
		});

		it('should track telemetry events when telemetry is provided', async () => {
			const { installNode } = useInstallNode();
			const { track } = useTelemetry();

			const result = await installNode({
				type: 'unverified',
				packageName: 'test-package',
				telemetry: {
					hasQuickConnect: true,
					source: 'node detail view',
				},
			});

			expect(result.success).toBe(true);
			expect(track).toHaveBeenCalledWith('user started cnr package install', {
				input_string: 'test-package',
				has_quick_connect: true,
				source: 'node detail view',
			});
		});
	});

	describe('loading state', () => {
		it('should set loading to true during installation', async () => {
			vi.mocked(communityNodesStore.installPackage).mockImplementation(async () => {
				await new Promise((resolve) => setTimeout(resolve, 10));
Repository: n8n-io/n8n
File: packages/cli/src/events/relays/telemetry.event-relay.ts (TypeScript)

Code before:
		errorMessage,
	}: RelayEventMap['external-secrets-provider-settings-saved']) {
		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,

Review comment (suggestion):
The `Telemetry` class used here automatically adds the `user_id` to each sent event. It doesn't have to be included here.

Code after:
		errorMessage,
	}: RelayEventMap['external-secrets-provider-settings-saved']) {
		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', {