ronantakizawa/codereview-bench huggingface dataset:
Repository: directus/directus
File: api/src/database/helpers/schema/dialects/mssql.ts (TypeScript)

Code before:
		/*
		MSSQL requires all selected columns that are not aggregated over are to be present in the GROUP BY clause

		> When the select list has no aggregations, each column in the select list must be included in the GROUP BY list.

		https://learn.microsoft.com/en-us/sql/t-sql/queries/select-group-by-transact-sql?view=sql-server-ver16#g-syntax-variations-for-group-by

		MSSQL does not support aliases in the GROUP BY clause

		> The column expression cannot contain:
			A column alias that is defined in the SELECT list. It can use a column alias for a derived table that is defined
			in the FROM clause.

		https://learn.microsoft.com/en-us/sql/t-sql/queries/select-group-by-transact-sql?view=sql-server-ver16
		 */

		if (sortRecords.length > 0) {
			groupByFields.push(...sortRecords.map(({ column }) => column));
		}
	}

	override getColumnMaxLength(): number {
		return 128;
	}

	override getTableMaxLength(): number {
		return 128;
	}
}


Review comment (suggestion):
```suggestion
	override getTableNameMaxLength(): number {
```

Code after:
		/*
		MSSQL requires all selected columns that are not aggregated over are to be present in the GROUP BY clause

		> When the select list has no aggregations, each column in the select list must be included in the GROUP BY list.

		https://learn.microsoft.com/en-us/sql/t-sql/queries/select-group-by-transact-sql?view=sql-server-ver16#g-syntax-variations-for-group-by

		MSSQL does not support aliases in the GROUP BY clause

		> The column expression cannot contain:
			A column alias that is defined in the SELECT list. It can use a column alias for a derived table that is defined
			in the FROM clause.

		https://learn.microsoft.com/en-us/sql/t-sql/queries/select-group-by-transact-sql?view=sql-server-ver16
		 */

		if (sortRecords.length > 0) {
			groupByFields.push(...sortRecords.map(({ column }) => column));
		}
	}

	override getColumnNameMaxLength(): number {
		return 128;
	}

	override getTableNameMaxLength(): number {
		return 128;
	}
}
Repository: directus/directus
File: api/src/database/helpers/schema/types.ts (TypeScript)

Code before:
		table: string,
		primaryKey: string,
		orderByString: string,
		orderByFields: Knex.Raw[],
	): Knex.QueryBuilder {
		dbQuery.rowNumber(
			knex.ref('directus_row_number').toQuery(),
			knex.raw(`partition by ?? order by ${orderByString}`, [`${table}.${primaryKey}`, ...orderByFields]),
		);

		return dbQuery;
	}

	formatUUID(uuid: string): string {
		return uuid; // no-op by default
	}

	/**
	 * @returns Size of the database in bytes
	 */
	async getDatabaseSize(): Promise<number | null> {
		return null;
	}

	prepQueryParams(queryParams: Sql): Sql {
		return queryParams;
	}

	prepBindings(bindings: Knex.Value[]): any {
		return bindings;
	}

	addInnerSortFieldsToGroupBy(
		_groupByFields: (string | Knex.Raw)[],
		_sortRecords: SortRecord[],
		_hasRelationalSort: boolean,
	): void {
		// no-op by default
	}

	getColumnMaxLength() {
		return 64;
	}

	getTableMaxLength() {
		return 64;
	}
}


Review comment (suggestion):
Do all databases, except for `mysql` and `oracle`, have this limit? We should avoid unnecessarily restricting names unless the database has this restriction itself

Code after:

	/**
	 * @returns Size of the database in bytes
	 */
	async getDatabaseSize(): Promise<number | null> {
		return null;
	}

	prepQueryParams(queryParams: Sql): Sql {
		return queryParams;
	}

	prepBindings(bindings: Knex.Value[]): any {
		return bindings;
	}

	addInnerSortFieldsToGroupBy(
		_groupByFields: (string | Knex.Raw)[],
		_sortRecords: SortRecord[],
		_hasRelationalSort: boolean,
	): void {
		// no-op by default
	}

	getColumnNameMaxLength() {
		return 64;
	}

	getTableNameMaxLength() {
		return 64;
	}
}
Repository: directus/directus
File: api/src/database/migrations/20210510A-restructure-relations.ts (TypeScript)

Code before:
import type { Knex } from 'knex';
import { getHelpers } from '../helpers/index.js';

export async function up(knex: Knex): Promise<void> {
	const helper = getHelpers(knex).schema;

	await knex.schema.alterTable('directus_relations', (table) => {
		table.dropColumns('many_primary', 'one_primary');
		table.string('one_deselect_action').defaultTo('nullify');
	});

	await knex('directus_relations').update({ one_deselect_action: 'nullify' });

	await helper.changeToType('directus_relations', 'sort_field', 'string', {
		length: helper.getColumnNameMaxLength(),
	});

	await helper.changeToType('directus_relations', 'one_deselect_action', 'string', {
		nullable: false,
		default: 'nullify',
	});
}

export async function down(knex: Knex): Promise<void> {
	const helper = getHelpers(knex).schema;

	await helper.changeToType('directus_relations', 'sort_field', 'string', {
		length: 255,
	});

	await knex.schema.alterTable('directus_relations', (table) => {
		table.dropColumn('one_deselect_action');
		table.string('many_primary', helper.getColumnNameMaxLength());
		table.string('one_primary', helper.getColumnNameMaxLength());
	});
}


Review comment (refactor):
In the name of consistency should we use `helper.schema.getColumnNameMaxLength` and update usage accordingly?

Code after:
import type { Knex } from 'knex';
import { getHelpers } from '../helpers/index.js';

export async function up(knex: Knex): Promise<void> {
	const helper = getHelpers(knex);

	await knex.schema.alterTable('directus_relations', (table) => {
		table.dropColumns('many_primary', 'one_primary');
		table.string('one_deselect_action').defaultTo('nullify');
	});

	await knex('directus_relations').update({ one_deselect_action: 'nullify' });

	await helper.schema.changeToType('directus_relations', 'sort_field', 'string', {
		length: helper.schema.getColumnNameMaxLength(),
	});

	await helper.schema.changeToType('directus_relations', 'one_deselect_action', 'string', {
		nullable: false,
		default: 'nullify',
	});
}

export async function down(knex: Knex): Promise<void> {
	const helper = getHelpers(knex);

	await helper.schema.changeToType('directus_relations', 'sort_field', 'string', {
		length: 255,
	});

	await knex.schema.alterTable('directus_relations', (table) => {
		table.dropColumn('one_deselect_action');
		table.string('many_primary', helper.schema.getColumnNameMaxLength());
		table.string('one_primary', helper.schema.getColumnNameMaxLength());
	});
}
Repository: directus/directus
File: packages/utils/shared/generate-joi.ts (TypeScript)

Code before:
					const val = Number(value instanceof Date ? NaN : value);
					return !Number.isNaN(val) && Math.abs(val) <= Number.MAX_SAFE_INTEGER;
				})
			) {
				const values = compareValue as [number, number];
				schema[key] = getNumberSchema().less(Number(values[0])).greater(Number(values[1]));
			} else {
				const values = compareValue as [string, string];
				schema[key] = getDateSchema().less(values[0]).greater(values[1]);
			}
		}

		if (operator === '_submitted') {
			schema[key] = getAnySchema().required();
		}

		if (operator === '_regex') {
			if (compareValue === null || compareValue === undefined) {
				schema[key] = Joi.any().equal(true);
			} else {
				const wrapped =
					typeof compareValue === 'string' ? compareValue.startsWith('/') && compareValue.endsWith('/') : false;

				schema[key] = getStringSchema()
					.min(0)
					.regex(new RegExp(wrapped ? (compareValue as any).slice(1, -1) : compareValue), {});
			}
		}
	}

	schema[key] = schema[key] ?? Joi.any();

	if (options.requireAll) {
		schema[key] = schema[key]!.required();
	}

	return Joi.object(schema).unknown();
}


Review comment (suggestion):
```suggestion
					.regex(new RegExp(wrapped ? (compareValue as any).slice(1, -1) : compareValue));
```

Code after:
					const val = Number(value instanceof Date ? NaN : value);
					return !Number.isNaN(val) && Math.abs(val) <= Number.MAX_SAFE_INTEGER;
				})
			) {
				const values = compareValue as [number, number];
				schema[key] = getNumberSchema().less(Number(values[0])).greater(Number(values[1]));
			} else {
				const values = compareValue as [string, string];
				schema[key] = getDateSchema().less(values[0]).greater(values[1]);
			}
		}

		if (operator === '_submitted') {
			schema[key] = getAnySchema().required();
		}

		if (operator === '_regex') {
			if (compareValue === null || compareValue === undefined) {
				schema[key] = Joi.any().equal(true);
			} else {
				const wrapped =
					typeof compareValue === 'string' ? compareValue.startsWith('/') && compareValue.endsWith('/') : false;

				schema[key] = getStringSchema()
					.min(0)
					.regex(new RegExp(wrapped ? (compareValue as any).slice(1, -1) : compareValue));
			}
		}
	}

	schema[key] = schema[key] ?? Joi.any();

	if (options.requireAll) {
		schema[key] = schema[key]!.required();
	}

	return Joi.object(schema).unknown();
}
Repository: directus/directus
File: app/src/modules/settings/routes/flows/triggers.ts (TypeScript)

Code before:
				{
					label: t('url'),
					text: `${getPublicURL()}flows/trigger/${flow.id}`,
					copyable: true,
				},
			],
			options: ({ async, method }) => [
				{
					field: 'method',
					name: t('triggers.webhook.method'),
					type: 'string',
					meta: {
						width: 'half',
						interface: 'select-dropdown',
						options: {
							choices: [
								{ text: 'GET', value: 'GET' },
								{ text: 'POST', value: 'POST' },
							],
						},
					},
					schema: {
						default_value: 'GET',
					},
				},
				{
					field: 'error_on_reject',
					name: t('triggers.webhook.error_on_reject'),
					type: 'boolean',
					meta: {
						width: 'half' as Width,
						interface: 'toggle',
						conditions: [
							{
								rule: {
									async: {
										_eq: true,
									},
								},
								hidden: true,
							},
						],
					},
					schema: {
						default_value: false,
					},
				},
				{
					field: 'async',
					name: t('triggers.webhook.async'),
					type: 'boolean',

Review comment (suggestion):
We should move `error_on_reject` after the `async` option, currently to much content shift when async is selected

#### Before

https://github.com/user-attachments/assets/2e8bcbb3-576a-4eef-9fe6-c25fc25e6d27


#### After

https://github.com/user-attachments/assets/88872c23-7408-4f73-a07f-53ef20a0553e




Code after:
				{
					label: t('url'),
					text: `${getPublicURL()}flows/trigger/${flow.id}`,
					copyable: true,
				},
			],
			options: ({ async, method }) => [
				{
					field: 'method',
					name: t('triggers.webhook.method'),
					type: 'string',
					meta: {
						width: 'half',
						interface: 'select-dropdown',
						options: {
							choices: [
								{ text: 'GET', value: 'GET' },
								{ text: 'POST', value: 'POST' },
							],
						},
					},
					schema: {
						default_value: 'GET',
					},
				},
				{
					field: 'async',
					name: t('triggers.webhook.async'),
					type: 'boolean',
					meta: {
						width: 'half',
						interface: 'toggle',
						required: true,
					},
					schema: {
						default_value: false,
					},
				},
				{
					field: 'error_on_reject',
					name: t('triggers.webhook.error_on_reject'),
					type: 'boolean',
					meta: {
						width: 'half' as Width,
						interface: 'toggle',
						hidden: async,
					},
					schema: {
						default_value: false,
					},
				},
Repository: directus/directus
File: tests/blackbox/tests/db/routes/items/o2m.test.ts (TypeScript)

Code before:
				const queryLimit = Number(config.envs[vendor]['QUERY_LIMIT_DEFAULT']);

				const setupStates = Array.from({ length: queryLimit }, (_, i) => ({
					...createState(pkType),
					name: 'test_on_deselected_action_create_' + i,
				}));

				// Setup
				const createdItem = await CreateItem(vendor, {
					collection: localCollectionCountries,
					item: {
						...createCountry(pkType),
						name: 'test_on_deselected_action',
						states: setupStates,
					},
				});

				await UpdateItem(vendor, {
					id: createdItem.id,
					collection: localCollectionCountries,
					item: {
						states: {
							create: [
								{
									...createState(pkType),
									name: 'test_on_deselected_action_update_1',
								},
							],
							update: [],
							delete: [],
						},
					},
				});

				// Action
				await request(getUrl(vendor))
					.patch(`/items/${localCollectionCountries}/${createdItem.id}`)
					.send({
						states: [
							{
								...createState(pkType),
								name: 'test_on_deselected_action_patch',
							},
						],
					})
					.set('Authorization', `Bearer ${USER.ADMIN.TOKEN}`);

				const response = await request(getUrl(vendor))
					.get(`/items/${localCollectionStates}`)
					.query({
						filter: JSON.stringify({

Review comment (suggestion):
```suggestion
									name: 'test_on_deselected_action_update_over_limit',
```

Code after:
				const queryLimit = Number(config.envs[vendor]['QUERY_LIMIT_DEFAULT']);

				const setupStates = Array.from({ length: queryLimit }, (_, i) => ({
					...createState(pkType),
					name: 'test_on_deselected_action_create_' + i,
				}));

				// Setup
				const createdItem = await CreateItem(vendor, {
					collection: localCollectionCountries,
					item: {
						...createCountry(pkType),
						name: 'test_on_deselected_action',
						states: setupStates,
					},
				});

				await UpdateItem(vendor, {
					id: createdItem.id,
					collection: localCollectionCountries,
					item: {
						states: {
							create: [
								{
									...createState(pkType),
									name: 'test_on_deselected_action_update_over_limit',
								},
							],
							update: [],
							delete: [],
						},
					},
				});

				// Action
				await request(getUrl(vendor))
					.patch(`/items/${localCollectionCountries}/${createdItem.id}`)
					.send({
						states: [
							{
								...createState(pkType),
								name: 'test_on_deselected_action_patch',
							},
						],
					})
					.set('Authorization', `Bearer ${USER.ADMIN.TOKEN}`);

				const response = await request(getUrl(vendor))
					.get(`/items/${localCollectionStates}`)
					.query({
						filter: JSON.stringify({
Repository: directus/directus
File: packages/types/src/accountability.ts (TypeScript)

Code before:
export type ShareScope = {
	collection: string;
	item: string;
};

export type Accountability = {
	role: string | null;
	roles: string[];
	user: string | null;
	admin: boolean;
	app: boolean;
	share?: string | undefined;
	ip: string | null;
	userAgent?: string | undefined;
	origin?: string | undefined;
	session?: string | undefined;
};


Review comment (suggestion):
Not really sure about editing `Accountability` just for convenience, as it is used everywhere. And the expectation could have been in some block of logic, that if the key is set, that there also is a value present in accountability.

Code after:
export type ShareScope = {
	collection: string;
	item: string;
};

export type Accountability = {
	role: string | null;
	roles: string[];
	user: string | null;
	admin: boolean;
	app: boolean;
	share?: string;
	ip: string | null;
	userAgent?: string;
	origin?: string;
	session?: string;
};
Repository: directus/directus
File: app/src/modules/settings/routes/flows/components/logs-drawer.vue (Vue)

Code before:
<script setup lang="ts">
import { useExtensions } from '@/extensions';
import { computed, toRefs, unref } from 'vue';
import { useI18n } from 'vue-i18n';
import { getTriggers } from '../triggers';
import { FlowRaw } from '@directus/types';
import VDetail from '@/components/v-detail.vue';

const props = defineProps<{
	flow: FlowRaw;
	revision: any;
	closeDrawer: () => void;
}>();

const { flow, revision, closeDrawer } = toRefs(props);

const { t } = useI18n();

const { triggers } = getTriggers();
const { operations } = useExtensions();

const usedTrigger = computed(() => {
	return triggers.find((trigger) => trigger.id === unref(flow).trigger);
});

const triggerData = computed(() => {
	if (!unref(revision)?.data) return { trigger: null, accountability: null, options: null };

	const { data } = unref(revision)?.data as any;

	return {
		trigger: data.$trigger,
		accountability: data.$accountability,
		options: props.flow.options,
	};
});

const emit = defineEmits(['close']);

const steps = computed(() => {

Review comment (suggestion):
Let’s stick with the `emit` approach

```suggestion
const { flow, revision } = toRefs(props);
```

Code after:
<script setup lang="ts">
import { useExtensions } from '@/extensions';
import { computed, toRefs, unref } from 'vue';
import { useI18n } from 'vue-i18n';
import { getTriggers } from '../triggers';
import { FlowRaw } from '@directus/types';
import VDetail from '@/components/v-detail.vue';
import { Revision } from '@/types/revisions';

const props = defineProps<{
	flow: FlowRaw;
	revision: Revision;
}>();

const { flow, revision } = toRefs(props);

const { t } = useI18n();

const { triggers } = getTriggers();
const { operations } = useExtensions();

const usedTrigger = computed(() => {
	return triggers.find((trigger) => trigger.id === unref(flow).trigger);
});

const triggerData = computed(() => {
	if (!unref(revision)?.data) return { trigger: null, accountability: null, options: null };

	const { data } = unref(revision)?.data as any;

	return {
		trigger: data.$trigger,
		accountability: data.$accountability,
		options: flow.value.options,
	};
});

const emit = defineEmits(['close']);

const steps = computed(() => {
Repository: directus/directus
File: app/src/modules/settings/routes/flows/components/logs-drawer.vue (Vue)

Code before:
<script setup lang="ts">
import { useExtensions } from '@/extensions';
import { computed, toRefs, unref } from 'vue';
import { useI18n } from 'vue-i18n';
import { getTriggers } from '../triggers';
import { FlowRaw } from '@directus/types';
import VDetail from '@/components/v-detail.vue';

const props = defineProps<{
	flow: FlowRaw;
	revision: any;
	closeDrawer: () => void;
}>();

const { flow, revision, closeDrawer } = toRefs(props);

const { t } = useI18n();

const { triggers } = getTriggers();
const { operations } = useExtensions();

const usedTrigger = computed(() => {
	return triggers.find((trigger) => trigger.id === unref(flow).trigger);
});

const triggerData = computed(() => {
	if (!unref(revision)?.data) return { trigger: null, accountability: null, options: null };

	const { data } = unref(revision)?.data as any;

	return {
		trigger: data.$trigger,
		accountability: data.$accountability,
		options: props.flow.options,
	};
});

Review comment (suggestion):
Use the existing `Revision` type from app/src/types/revisions.ts

```suggestion
	revision: Revision;
```

Code after:
<script setup lang="ts">
import { useExtensions } from '@/extensions';
import { computed, toRefs, unref } from 'vue';
import { useI18n } from 'vue-i18n';
import { getTriggers } from '../triggers';
import { FlowRaw } from '@directus/types';
import VDetail from '@/components/v-detail.vue';
import { Revision } from '@/types/revisions';

const props = defineProps<{
	flow: FlowRaw;
	revision: Revision;
}>();

const { flow, revision } = toRefs(props);

const { t } = useI18n();

const { triggers } = getTriggers();
const { operations } = useExtensions();

const usedTrigger = computed(() => {
	return triggers.find((trigger) => trigger.id === unref(flow).trigger);
});

const triggerData = computed(() => {
	if (!unref(revision)?.data) return { trigger: null, accountability: null, options: null };

	const { data } = unref(revision)?.data as any;

	return {
		trigger: data.$trigger,
		accountability: data.$accountability,
		options: flow.value.options,
	};
});
Repository: directus/directus
File: app/src/modules/settings/routes/flows/components/logs-sidebar-detail.vue (Vue)

Code before:
			class="revisions-date-group"
			start-open
		>
			<div v-show="showFailedOnly && countFailedLogs(group) === 0" class="empty">
				{{ t('no_failed_logs') }}
			</div>
			<div class="scroll-container">
				<div v-for="revision in group.revisions" :key="revision.id" class="log">
					<button v-show="isLogVisible(revision)" @click="selectedRevision = revision">
						<v-icon
							name="fiber_manual_record"
							filled
							:color="revision.status === 'resolve' ? 'var(--theme--primary)' : 'var(--theme--secondary)'"
							small
						/>
						<v-icon name="play_arrow" color="var(--theme--primary)" small />
						{{ revision.timeRelative }}
					</button>
				</div>
			</div>
		</v-detail>

		<v-pagination v-if="pagesCount > 1" v-model="page" :length="pagesCount" :total-visible="3" />
	</sidebar-detail>

	<logs-drawer :close-drawer="() => (selectedRevision = null)" :flow="flow" :revision="selectedRevision"></logs-drawer>
</template>

<style lang="scss" scoped>
.v-progress-linear {
	margin: 24px 0;
}

.log {
	position: relative;
	display: block;

	button {
		position: relative;
		z-index: 2;
		display: block;
		width: 100%;
		text-align: left;
	}

	&::before {
		position: absolute;
		top: -4px;
		left: -4px;
		z-index: 1;
		width: calc(100% + 8px);

Review comment (suggestion):
Let’s keep the event approach for `@close`

```suggestion
	<logs-drawer :flow="flow" :revision="selectedRevision" @close="selectedRevision = null" />
```

Code after:
			</button>

			<div v-if="!revisionsByDate?.length" class="empty">{{ t('no_logs_on_page') }}</div>

			<v-detail
				v-for="group in revisionsByDate"
				:key="group.dateFormatted"
				:label="group.dateFormatted"
				class="revisions-date-group"
				start-open
			>
				<div class="scroll-container">
					<div v-for="revision in group.revisions" :key="revision.id" class="log">
						<button @click="selectedRevision = revision">
							<v-icon v-if="revision.status === 'resolve'" name="check_circle" color="var(--theme--primary)" small />
							<v-icon v-else name="cancel" color="var(--theme--secondary)" small />
							{{ revision.timeRelative }}
						</button>
					</div>
				</div>
			</v-detail>
		</template>

		<v-pagination v-if="pagesCount > 1" v-model="page" :length="pagesCount" :total-visible="3" />
	</sidebar-detail>

	<logs-drawer :flow="flow" :revision="selectedRevision" @close="selectedRevision = null"></logs-drawer>
</template>

<style lang="scss" scoped>
.v-progress-linear {
	margin: 24px 0;
}

.v-detail + .v-detail {
	margin-top: 12px;
}

.v-icon {
	vertical-align: text-top;
}

.toggle-failed {
	color: var(--theme--foreground-subdued);
	transition: color var(--fast) var(--transition);
	margin-bottom: 24px;

	&.active,
	&:hover {
		color: var(--theme--foreground);
	}