ronantakizawa/codereview-bench huggingface dataset:
Repository: directus/directus
File: api/src/services/mail/index.ts (TypeScript)

Code before:
	mailer: Transporter;

	constructor(opts: AbstractServiceOptions) {
		this.schema = opts.schema;
		this.accountability = opts.accountability || null;
		this.knex = opts?.knex || getDatabase();
		this.mailer = getMailer();

		if (env['EMAIL_VERIFY_SETUP']) {
			this.mailer.verify((error) => {
				if (error) {
					logger.warn(`Email connection failed:`);
					logger.warn(error);
				}
			});
		}
	}

	async send<T>(options: EmailOptions): Promise<T> {
		const payload = await emitter.emitFilter(`email.send`, options, {
			database: getDatabase(),
			schema: null,
			accountability: null,
		});

		if (!payload) return null as T;

		const { template, ...emailOptions } = payload;

		let { html } = options;

		const defaultTemplateData = await this.getDefaultTemplateData();

		const from = `${defaultTemplateData.projectName} <${options.from || (env['EMAIL_FROM'] as string)}>`;

		if (template) {
			let templateData = template.data;

			templateData = {
				...defaultTemplateData,
				...templateData,
			};

			html = await this.renderTemplate(template.name, templateData);
		}

		if (typeof html === 'string') {
			// Some email clients start acting funky when line length exceeds 75 characters. See #6074
			html = html
				.split('\n')
				.map((line) => line.trim())

Review comment (suggestion):
We should probably adjust the method signature to return `Promise<T | null>`.

```suggestion
		if (!payload) return null;
```

Code after:
	mailer: Transporter;

	constructor(opts: AbstractServiceOptions) {
		this.schema = opts.schema;
		this.accountability = opts.accountability || null;
		this.knex = opts?.knex || getDatabase();
		this.mailer = getMailer();

		if (env['EMAIL_VERIFY_SETUP']) {
			this.mailer.verify((error) => {
				if (error) {
					logger.warn(`Email connection failed:`);
					logger.warn(error);
				}
			});
		}
	}

	async send<T>(options: EmailOptions): Promise<T | null> {
		const payload = await emitter.emitFilter(`email.send`, options, {
			database: getDatabase(),
			schema: null,
			accountability: null,
		});

		if (!payload) return null;

		const { template, ...emailOptions } = payload;

		let { html } = options;

		const defaultTemplateData = await this.getDefaultTemplateData();

		const from = `${defaultTemplateData.projectName} <${options.from || (env['EMAIL_FROM'] as string)}>`;

		if (template) {
			let templateData = template.data;

			templateData = {
				...defaultTemplateData,
				...templateData,
			};

			html = await this.renderTemplate(template.name, templateData);
		}

		if (typeof html === 'string') {
			// Some email clients start acting funky when line length exceeds 75 characters. See #6074
			html = html
				.split('\n')
				.map((line) => line.trim())
Repository: directus/directus
File: api/src/database/migrations/20240710A-permissions-policies.ts (TypeScript)

Code before:
		if (currentPerm.validation && Object.keys(currentPerm.validation)[0] === logicalKey) {
			validation = {
				[logicalKey]: [
					...(currentPerm.validation as LogicalFilterOR & LogicalFilterAND)[logicalKey],
					newPerm.validation,
				],
			} as LogicalFilterAND | LogicalFilterOR;
		} else if (currentPerm.validation) {
			// Empty {} supersedes other validations in _OR merge
			if (strategy === 'or' && (isEqual(currentPerm.validation, {}) || isEqual(newPerm.validation, {}))) {
				validation = {};
			} else {
				validation = {
					[logicalKey]: [currentPerm.validation, newPerm.validation],
				} as LogicalFilterAND | LogicalFilterOR;
			}
		} else {
			validation = {
				[logicalKey]: [newPerm.validation],
			} as LogicalFilterAND | LogicalFilterOR;
		}
	}

	if (newPerm.fields) {
		if (Array.isArray(currentPerm.fields) && strategy === 'or') {
			fields = [...new Set([...currentPerm.fields, ...newPerm.fields])];
		} else if (Array.isArray(currentPerm.fields) && strategy === 'and') {
			fields = intersection(currentPerm.fields, newPerm.fields);
		} else {
			fields = newPerm.fields;
		}

		if (fields.includes('*')) fields = ['*'];
	}

	if (newPerm.presets) {
		presets = merge({}, presets, newPerm.presets);
	}

	return omit(
		{
			...currentPerm,
			permissions,
			validation,
			fields,
			presets,
		},
		['id', 'system'],
	);
}


Review comment (suggestion):
AFAIK lodash `unique` is faster than creating a new Set in most cases (and personally I thin it feels semantically more correct)

```suggestion
			fields = unique([...currentPerm.fields, ...newPerm.fields]);
```

Code after:
					...(currentPerm.validation as LogicalFilterOR & LogicalFilterAND)[logicalKey],
					newPerm.validation,
				],
			} as LogicalFilterAND | LogicalFilterOR;
		} else if (currentPerm.validation) {
			// Empty {} supersedes other validations in _OR merge
			if (strategy === 'or' && (isEqual(currentPerm.validation, {}) || isEqual(newPerm.validation, {}))) {
				validation = {};
			} else {
				validation = {
					[logicalKey]: [currentPerm.validation, newPerm.validation],
				} as LogicalFilterAND | LogicalFilterOR;
			}
		} else {
			validation = {
				[logicalKey]: [newPerm.validation],
			} as LogicalFilterAND | LogicalFilterOR;
		}
	}

	if (newPerm.fields) {
		if (Array.isArray(currentPerm.fields) && strategy === 'or') {
			fields = uniq([...currentPerm.fields, ...newPerm.fields]);
		} else if (Array.isArray(currentPerm.fields) && strategy === 'and') {
			fields = intersection(currentPerm.fields, newPerm.fields);
		} else {
			fields = newPerm.fields;
		}

		if (fields.includes('*')) fields = ['*'];
	}

	if (newPerm.presets) {
		presets = merge({}, presets, newPerm.presets);
	}

	return omit(
		{
			...currentPerm,
			permissions,
			validation,
			fields,
			presets,
		},
		['id', 'system'],
	);
}

async function fetchRoleAccess(roles: string[], context: { knex: Knex }) {
	const roleAccess: RoleAccess = {
		admin_access: false,
Repository: directus/directus
File: api/src/server.ts (TypeScript)

Code before:

	async function onShutdown() {
		emitter.emitAction(
			'server.stop',
			{ server },
			{
				database: getDatabase(),
				schema: null,
				accountability: null,
			},
		);

		if (getNodeEnv() !== 'development') {
			logger.info('Directus shut down OK. Bye bye!');
		}
	}
}

export async function startServer(): Promise<void> {
	const server = await createServer();

	const host = env['HOST'] as string;
	const port = env['PORT'];

	server
		.listen(port, host, () => {
			logger.info(`Server started at http://${host}:${port}`);

			process.send?.('ready');

			emitter.emitAction(
				'server.start',
				{ server },
				{
					database: getDatabase(),
					schema: null,
					accountability: null,
				},
			);
		})
		.once('error', (err: any) => {
			if (err?.code === 'EADDRINUSE') {
				logger.error(`Port ${port} is already in use`);
				process.exit(1);
			} else {
				throw err;
			}
		});
}


Review comment (suggestion):
As far as I can tell, `express` doesn't accept a `host` parameter when a socket is used instead of a port. We'll have to update this to drop the host if a socket is passed instead of a port. 

Also, is it kosher to put a socket path inside the env var named `PORT`, or should we support a new `SOCKET`? I'm not familiar with the standards/best practices around this, but it feels odd to have a socket path given in a port 🤔 

Code after:
		logger.info('Database connections destroyed');
	}

	async function onShutdown() {
		emitter.emitAction(
			'server.stop',
			{ server },
			{
				database: getDatabase(),
				schema: null,
				accountability: null,
			},
		);

		if (getNodeEnv() !== 'development') {
			logger.info('Directus shut down OK. Bye bye!');
		}
	}
}

export async function startServer(): Promise<void> {
	const server = await createServer();

	const host = env['HOST'] as string;
	const path = env['UNIX_SOCKET_PATH'] as string | undefined;
	const port = env['PORT'] as string | undefined;

	let listenOptions: ListenOptions;

	if (path) {
		listenOptions = { path };
	} else {
		listenOptions = {
			host,
			port: parseInt(port || '8055'),
		};
	}

	server
		.listen(listenOptions, () => {
			const protocol = server instanceof https.Server ? 'https' : 'http';

			logger.info(
				`Server started at ${listenOptions.port ? `${protocol}://${getAddress(server)}` : getAddress(server)}`,
			);

			process.send?.('ready');

			emitter.emitAction(
				'server.start',
				{ server },
Repository: directus/directus
File: api/src/server.ts (TypeScript)

Code before:
		);

		if (getNodeEnv() !== 'development') {
			logger.info('Directus shut down OK. Bye bye!');
		}
	}
}

export async function startServer(): Promise<void> {
	const server = await createServer();

	const host = env['HOST'] as string;
	const path = env['UNIX_SOCKET_PATH'] as string | undefined;
	const port = env['PORT'] as string | undefined;

	let listenOptions: ListenOptions = {
		path,
	};

	if (port || !path) {
		// if port is present we ignore socket path
		listenOptions = {
			host,
			port: parseInt(port || '8055'),
		};
	}

	server
		.listen(listenOptions, () => {
			const protocol = server instanceof https.Server ? 'https' : 'http';

			logger.info(
				`Server started at ${listenOptions.port ? `${protocol}://${getAddress(server)}` : getAddress(server)}`,
			);

			process.send?.('ready');

			emitter.emitAction(
				'server.start',
				{ server },
				{
					database: getDatabase(),
					schema: null,
					accountability: null,
				},
			);
		})
		.once('error', (err: any) => {
			if (err?.code === 'EADDRINUSE') {
				logger.error(`${listenOptions.port ? `Port ${listenOptions.port}` : getAddress(server)} is already in use`);
				process.exit(1);

Review comment (suggestion):
Here be opinions!

I'd prefer it if we could do:

```
let listenOptions: ListenOptions;

if (path) {
  listenOptions = {...};
} else {
  listenOptions = {...};
}
```

Code after:
		);

		if (getNodeEnv() !== 'development') {
			logger.info('Directus shut down OK. Bye bye!');
		}
	}
}

export async function startServer(): Promise<void> {
	const server = await createServer();

	const host = env['HOST'] as string;
	const path = env['UNIX_SOCKET_PATH'] as string | undefined;
	const port = env['PORT'] as string | undefined;

	let listenOptions: ListenOptions;

	if (path) {
		listenOptions = { path };
	} else {
		listenOptions = {
			host,
			port: parseInt(port || '8055'),
		};
	}

	server
		.listen(listenOptions, () => {
			const protocol = server instanceof https.Server ? 'https' : 'http';

			logger.info(
				`Server started at ${listenOptions.port ? `${protocol}://${getAddress(server)}` : getAddress(server)}`,
			);

			process.send?.('ready');

			emitter.emitAction(
				'server.start',
				{ server },
				{
					database: getDatabase(),
					schema: null,
					accountability: null,
				},
			);
		})
		.once('error', (err: any) => {
			if (err?.code === 'EADDRINUSE') {
				logger.error(`${listenOptions.port ? `Port ${listenOptions.port}` : getAddress(server)} is already in use`);
				process.exit(1);
			} else {
Repository: directus/directus
File: api/src/utils/transaction.ts (TypeScript)

Code before:
			 * @link https://www.sqlite.org/rescode.html#busy
			 */
			const SQLITE_BUSY_ERROR_CODE = 'SQLITE_BUSY';

			if (
				(client === 'cockroachdb' && error?.code !== COCKROACH_RETRY_ERROR_CODE) ||
				(client === 'sqlite' && error?.code !== SQLITE_BUSY_ERROR_CODE)
			)
				throw error;

			const MAX_ATTEMPTS = 3;
			const BASE_DELAY = 100;

			const logger = useLogger();

			for (let attempt = 0; attempt < MAX_ATTEMPTS; ++attempt) {
				const delay = 2 ** attempt * BASE_DELAY;

				await new Promise((resolve) => setTimeout(resolve, delay));

				logger.trace(`Restarting failed transaction (attempt ${attempt + 1}/${MAX_ATTEMPTS})`);

				try {
					return await knex.transaction((trx) => handler(trx));
				} catch (error: any) {
					if (error?.code !== COCKROACH_RETRY_ERROR_CODE && error?.code !== SQLITE_BUSY_ERROR_CODE) throw error;
				}
			}

			/** Initial execution + additional attempts */
			const attempts = 1 + MAX_ATTEMPTS;
			throw new Error(`Transaction failed after ${attempts} attempts`, { cause: error });
		}
	}
};


Review comment (suggestion):
I think this should be an `||` as we need to throw in both the sqlite and cockroach case of the error. Additionally, since this can now be executed for both sqlite and cockroach we should probably add an explicit client check here as well, even though it is highly unlikely that the error codes overlap.
```suggestion
					if (
						(client === 'cockroachdb' && error?.code !== COCKROACH_RETRY_ERROR_CODE) ||
						(client === 'sqlite' && error?.code !== SQLITE_BUSY_ERROR_CODE)
					)
						throw error;
```

Code after:
					if (!shouldRetryTransaction(client, error)) throw error;
				}
			}

			/** Initial execution + additional attempts */
			const attempts = 1 + MAX_ATTEMPTS;
			throw new Error(`Transaction failed after ${attempts} attempts`, { cause: error });
		}
	}
};

function shouldRetryTransaction(client: DatabaseClient, error: unknown): boolean {
	/**
	 * This error code indicates that the transaction failed due to another
	 * concurrent or recent transaction attempting to write to the same data.
	 * This can usually be solved by restarting the transaction on client-side
	 * after a short delay, so that it is executed against the latest state.
	 *
	 * @link https://www.cockroachlabs.com/docs/stable/transaction-retry-error-reference
	 */
	const COCKROACH_RETRY_ERROR_CODE = '40001';

	/**
	 * SQLITE_BUSY is an error code returned by SQLite when an operation can't be
	 * performed due to a locked database file. This often arises due to multiple
	 * processes trying to simultaneously access the database, causing potential
	 * data inconsistencies. There are a few mechanisms to handle this case,
	 * one of which is to retry the complete transaction again
	 * on client-side after a short delay.
	 *
	 * @link https://www.sqlite.org/rescode.html#busy
	 */
	const SQLITE_BUSY_ERROR_CODE = 'SQLITE_BUSY';

	return (
		isObject(error) &&
		((client === 'cockroachdb' && error['code'] === COCKROACH_RETRY_ERROR_CODE) ||
			(client === 'sqlite' && error['code'] === SQLITE_BUSY_ERROR_CODE))
	);
}
Repository: directus/directus
File: api/src/database/index.ts (TypeScript)

Code before:

	database
		.on('query', ({ __knexUid }: QueryInfo) => {
			times.set(__knexUid, performance.now());
		})
		.on('query-response', (_response, queryInfo: QueryInfo) => {
			const time = times.get(queryInfo.__knexUid);
			let delta;

			if (time) {
				delta = performance.now() - time;
				times.delete(queryInfo.__knexUid);
			}

			logger.trace(`[${delta ? delta.toFixed(3) : '?'}ms] ${queryInfo.sql} [${(queryInfo.bindings ?? []).join(', ')}]`);
		});

	return database;
}

export function getSchemaInspector(database?: Knex): SchemaInspector {
	if (inspector) {
		return inspector;
	}

	database = database ?? getDatabase();

	inspector = createInspector(database);

	return inspector;
}

/**
 * Get database version. Value currently exists for MySQL only.
 *
 * @returns Cached database version
 */
export function getDatabaseVersion(): string | null {
	return databaseVersion;
}

export async function hasDatabaseConnection(database?: Knex): Promise<boolean> {
	database = database ?? getDatabase();

	try {
		if (getDatabaseClient(database) === 'oracle') {
			await database.raw('select 1 from DUAL');
		} else {
			await database.raw('SELECT 1');
		}


Review comment (suggestion):
Short and sweet

```suggestion
	database ??= getDatabase();
```

Code after:

	database
		.on('query', ({ __knexUid }: QueryInfo) => {
			times.set(__knexUid, performance.now());
		})
		.on('query-response', (_response, queryInfo: QueryInfo) => {
			const time = times.get(queryInfo.__knexUid);
			let delta;

			if (time) {
				delta = performance.now() - time;
				times.delete(queryInfo.__knexUid);
			}

			logger.trace(`[${delta ? delta.toFixed(3) : '?'}ms] ${queryInfo.sql} [${(queryInfo.bindings ?? []).join(', ')}]`);
		});

	return database;
}

export function getSchemaInspector(database?: Knex): SchemaInspector {
	if (inspector) {
		return inspector;
	}

	database ??= getDatabase();

	inspector = createInspector(database);

	return inspector;
}

/**
 * Get database version. Value currently exists for MySQL only.
 *
 * @returns Cached database version
 */
export function getDatabaseVersion(): string | null {
	return databaseVersion;
}

export async function hasDatabaseConnection(database?: Knex): Promise<boolean> {
	database = database ?? getDatabase();

	try {
		if (getDatabaseClient(database) === 'oracle') {
			await database.raw('select 1 from DUAL');
		} else {
			await database.raw('SELECT 1');
		}
Repository: directus/directus
File: api/src/permissions/utils/fetch-dynamic-variable-context.ts (TypeScript)

Code before:
			permissionContext,
			{ policies: options.policies },
			async (fields) => {
				const policiesService = new PoliciesService(context);

				return await policiesService.readMany(options.policies, {
					fields,
				});
			},
		);
	}

	return contextData;
}

async function fetchContextData(
	key: keyof RequiredPermissionContext,
	permissionContext: RequiredPermissionContext,
	cacheContext: Record<string, any>,
	fetch: (fields: string[]) => Promise<Record<string, any>>,
) {
	const { cache } = getCache();
	const env = useEnv();

	const fields = Array.from(permissionContext[key]!);
	const cacheKey = cache ? `filter-context-${key.slice(1)}-${getSimpleHash(JSON.stringify(cacheContext))}` : '';

	let data = undefined;

	if (cache) {
		data = await getCacheValue(cache, cacheKey);
	}

	if (!data) {
		data = await fetch(fields);

		if (cache && env['CACHE_ENABLED'] !== false) {
			await setCacheValue(cache, cacheKey, data);
		}
	}

	return data;
}


Review comment (suggestion):
Removing the `$` here, since I don't know if there might be any kv store where a `$` has a special meaning

Code after:
			permissionContext,
			{ policies: options.policies },
			async (fields) => {
				const policiesService = new PoliciesService(context);

				return await policiesService.readMany(options.policies, {
					fields,
				});
			},
		);
	}

	return contextData;
}

async function fetchContextData(
	key: keyof RequiredPermissionContext,
	permissionContext: RequiredPermissionContext,
	cacheContext: Record<string, any>,
	fetch: (fields: string[]) => Promise<Record<string, any>>,
) {
	const { cache } = getCache();
	const env = useEnv();

	const fields = Array.from(permissionContext[key]!);

	const cacheKey = cache
		? `filter-context-${key.slice(1)}-${getSimpleHash(JSON.stringify({ ...cacheContext, fields }))}`
		: '';

	let data = undefined;

	if (cache) {
		data = await getCacheValue(cache, cacheKey);
	}

	if (!data) {
		data = await fetch(fields);

		if (cache && env['CACHE_ENABLED'] !== false) {
			await setCacheValue(cache, cacheKey, data);
		}
	}

	return data;
}
Repository: directus/directus
File: api/src/permissions/utils/fetch-dynamic-variable-context.ts (TypeScript)

Code before:
			permissionContext,
			{ policies: options.policies },
			async (fields) => {
				const policiesService = new PoliciesService(context);

				return await policiesService.readMany(options.policies, {
					fields,
				});
			},
		);
	}

	return contextData;
}

async function fetchContextData(
	key: keyof RequiredPermissionContext,
	permissionContext: RequiredPermissionContext,
	cacheContext: Record<string, any>,
	fetch: (fields: string[]) => Promise<Record<string, any>>,
) {
	const { cache } = getCache();
	const env = useEnv();

	const fields = Array.from(permissionContext[key]!);
	const cacheKey = cache ? `filter-context-${key.slice(1)}-${getSimpleHash(JSON.stringify(cacheContext))}` : '';

	let data = undefined;

	if (cache) {
		data = await getCacheValue(cache, cacheKey);
	}

	if (!data) {
		data = await fetch(fields);

		if (cache && env['CACHE_ENABLED'] !== false) {
			await setCacheValue(cache, cacheKey, data);
		}
	}

	return data;
}


Review comment (performance):
This cache key is simple and has a high chance of collision. Including the `fields` in the cacheKey should be better but more testing is needed to confirm. 🤔 

On a side note, this `cache` would not be purged if `CACHE_AUTO_PURGE=false`.

Code after:
			permissionContext,
			{ policies: options.policies },
			async (fields) => {
				const policiesService = new PoliciesService(context);

				return await policiesService.readMany(options.policies, {
					fields,
				});
			},
		);
	}

	return contextData;
}

async function fetchContextData(
	key: keyof RequiredPermissionContext,
	permissionContext: RequiredPermissionContext,
	cacheContext: Record<string, any>,
	fetch: (fields: string[]) => Promise<Record<string, any>>,
) {
	const { cache } = getCache();
	const env = useEnv();

	const fields = Array.from(permissionContext[key]!);

	const cacheKey = cache
		? `filter-context-${key.slice(1)}-${getSimpleHash(JSON.stringify({ ...cacheContext, fields }))}`
		: '';

	let data = undefined;

	if (cache) {
		data = await getCacheValue(cache, cacheKey);
	}

	if (!data) {
		data = await fetch(fields);

		if (cache && env['CACHE_ENABLED'] !== false) {
			await setCacheValue(cache, cacheKey, data);
		}
	}

	return data;
}
Repository: directus/directus
File: app/src/routes/register/register-form.vue (Vue)

Code before:

function toggleHidePassword() {
	passwordHidden.value = !passwordHidden.value;
}
</script>

<template>
	<form novalidate @submit.prevent="onSubmit">
		<v-input
			v-model="email"
			autofocus
			autocomplete="username"
			type="email"
			:placeholder="t('email')"
			:disabled="isLoading"
		/>
		<v-input
			v-model="password"
			:type="passwordHidden ? 'password' : 'text'"
			autocomplete="new-password"
			:placeholder="t('password')"
			:disabled="isLoading"
		>
			<template #append>
				<v-icon
					v-tooltip="passwordHidden ? 'Show password' : 'Hide password'"
					:name="passwordHidden ? 'visibility' : 'visibility_off'"
					clickable
					@click="toggleHidePassword"
				/>
			</template>
		</v-input>

		<v-notice v-if="error" type="warning">
			{{ errorFormatted }}
		</v-notice>
		<div class="buttons">
			<v-button type="submit" :loading="isLoading" :disabled="isLoading" large>{{ t('register') }}</v-button>
		</div>
	</form>
</template>

<style lang="scss" scoped>
.v-input,
.v-notice {
	margin-bottom: 20px;
}

.buttons {
	display: flex;
	align-items: center;

Review comment (suggestion):
All text should use i18n placeholders and be added to the `en-US.yaml` translation file (only that one, since Crowding takes care of the rest), similar to `"t('confirm_password')"`

Code after:

<template>
	<form novalidate @submit.prevent="onSubmit">
		<v-input
			v-model="email"
			autofocus
			autocomplete="username"
			type="email"
			:placeholder="t('email')"
			:disabled="isLoading"
		/>
		<interface-system-input-password :value="password" :disabled="isLoading" @input="password = $event" />

		<v-notice v-if="error" type="warning">
			{{ errorFormatted }}
		</v-notice>
		<div class="buttons">
			<v-button type="submit" :loading="isLoading" :disabled="isLoading" large>{{ t('register') }}</v-button>
		</div>
	</form>
</template>

<style lang="scss" scoped>
.v-input,
.v-notice {
	margin-bottom: 20px;
}

.buttons {
	display: flex;
	align-items: center;
	justify-content: space-between;
}

.forgot-password {
	color: var(--theme--foreground-subdued);
	transition: color var(--fast) var(--transition);

	&:hover {
		color: var(--theme--foreground);
	}
}
</style>