ronantakizawa/codereview-bench huggingface dataset:
Repository: directus/directus
File: app/src/interfaces/tags/tags.vue (Vue)

Code before:
		removeTag(tag);
	} else {
		addTag(tag);
	}
}

function addTag(tag: string) {
	if (!tag || tag === '') return;
	// Remove any leading / trailing whitespace from the value
	tag = tag.trim();
	// Convert the tag to lowercase
	selectedValsLocal.value.push(tag);
	emitValue();
}

function removeTag(tag: string) {
	selectedValsLocal.value = selectedValsLocal.value.filter((savedTag) => savedTag !== tag);
	emitValue();
}

function emitValue() {
	emit('input', selectedVals.value);
}

function clearVariable() {
	isVariableMode.value = false;
	emit('input', null);
}
</script>

<template>
	<div class="interface-tags">
		<VInput v-if="isVariableMode" :model-value="value as string" readonly>
			<template #append>
				<span class="remove-variable">
					<VIcon v-tooltip="$t('interfaces.tags.remove_variable')" name="close" clickable @click.stop="clearVariable" />
				</span>
			</template>
		</VInput>
		<VInput
			v-else-if="allowCustom"
			:placeholder="placeholder || $t('interfaces.tags.add_tags')"
			:disabled
			:non-editable
			:dir="direction"
			@keydown="onInput"
			@blur="onInput"
		>
			<template v-if="iconLeft" #prepend><VIcon :name="iconLeft" /></template>
			<template #append><VIcon :name="iconRight" /></template>
		</VInput>

Review comment (suggestion):
If we emit `null` the `props.value` won’t be a string anymore and therefore `isVariableMode` will be false.

```suggestion
```

Code after:
	} else {
		addTag(tag);
	}
}

function addTag(tag: string) {
	if (!tag || tag === '') return;
	// Remove any leading / trailing whitespace from the value
	tag = tag.trim();
	// Convert the tag to lowercase
	selectedValsLocal.value.push(tag);
	emitValue();
}

function removeTag(tag: string) {
	selectedValsLocal.value = selectedValsLocal.value.filter((savedTag) => savedTag !== tag);
	emitValue();
}

function emitValue() {
	emit('input', selectedVals.value);
}

function clearVariable() {
	emit('input', null);
}
</script>

<template>
	<div class="interface-tags">
		<VInput
			v-if="isVariableMode || allowCustom"
			:model-value="isVariableMode ? (value as string) : undefined"
			:placeholder="placeholder || $t('interfaces.tags.add_tags')"
			:disabled="isVariableMode || disabled"
			:non-editable="isVariableMode || nonEditable"
			:dir="direction"
			@keydown="onInput"
			@blur="onInput"
		>
			<template v-if="iconLeft" #prepend><VIcon :name="iconLeft" /></template>
			<template #append>
				<VIcon
					v-if="isVariableMode"
					v-tooltip="$t('clear_value')"
					class="remove-variable"
					name="close"
					clickable
					:disabled
					@click.stop="clearVariable"
				/>
Repository: directus/directus
File: app/src/interfaces/tags/tags.vue (Vue)

Code before:
	// Convert the tag to lowercase
	selectedValsLocal.value.push(tag);
	emitValue();
}

function removeTag(tag: string) {
	selectedValsLocal.value = selectedValsLocal.value.filter((savedTag) => savedTag !== tag);
	emitValue();
}

function emitValue() {
	emit('input', selectedVals.value);
}

function clearVariable() {
	isVariableMode.value = false;
	emit('input', null);
}
</script>

<template>
	<div class="interface-tags">
		<VInput v-if="isVariableMode" :model-value="value as string" readonly>
			<template #append>
				<span class="remove-variable">
					<VIcon v-tooltip="$t('interfaces.tags.remove_variable')" name="close" clickable @click.stop="clearVariable" />
				</span>
			</template>
		</VInput>
		<VInput
			v-else-if="allowCustom"
			:placeholder="placeholder || $t('interfaces.tags.add_tags')"
			:disabled
			:non-editable
			:dir="direction"
			@keydown="onInput"
			@blur="onInput"
		>
			<template v-if="iconLeft" #prepend><VIcon :name="iconLeft" /></template>
			<template #append><VIcon :name="iconRight" /></template>
		</VInput>
		<div v-if="presetVals.length > 0 || customVals.length > 0" class="tags">
			<span v-if="presetVals.length > 0" class="presets tag-container">
				<VChip
					v-for="preset in presetVals"
					:key="preset"
					:class="['tag', { inactive: !selectedVals.includes(preset), 'non-editable': nonEditable }]"
					:disabled
					:dir="direction"
					small
					label

Review comment (suggestion):
I am not sure about “Remove Variable”. Let’s consider reusing the exisisting `clear_value` translation instead.

```suggestion
					<VIcon v-tooltip="$t('interfaces.tags.remove_variable')" name="close" clickable @click.stop="clearVariable" />
```

Code after:
	selectedValsLocal.value.push(tag);
	emitValue();
}

function removeTag(tag: string) {
	selectedValsLocal.value = selectedValsLocal.value.filter((savedTag) => savedTag !== tag);
	emitValue();
}

function emitValue() {
	emit('input', selectedVals.value);
}

function clearVariable() {
	emit('input', null);
}
</script>

<template>
	<div class="interface-tags">
		<VInput
			v-if="isVariableMode || allowCustom"
			:model-value="isVariableMode ? (value as string) : undefined"
			:placeholder="placeholder || $t('interfaces.tags.add_tags')"
			:disabled="isVariableMode || disabled"
			:non-editable="isVariableMode || nonEditable"
			:dir="direction"
			@keydown="onInput"
			@blur="onInput"
		>
			<template v-if="iconLeft" #prepend><VIcon :name="iconLeft" /></template>
			<template #append>
				<VIcon
					v-if="isVariableMode"
					v-tooltip="$t('clear_value')"
					class="remove-variable"
					name="close"
					clickable
					:disabled
					@click.stop="clearVariable"
				/>
				<VIcon v-else :name="iconRight" />
			</template>
		</VInput>
		<div v-if="presetVals.length > 0 || customVals.length > 0" class="tags">
			<span v-if="presetVals.length > 0" class="presets tag-container">
				<VChip
					v-for="preset in presetVals"
					:key="preset"
					:class="['tag', { inactive: !selectedVals.includes(preset), 'non-editable': nonEditable }]"
					:disabled
Repository: directus/directus
File: app/src/interfaces/tags/tags.vue (Vue)

Code before:
	tag = tag.trim();
	// Convert the tag to lowercase
	selectedValsLocal.value.push(tag);
	emitValue();
}

function removeTag(tag: string) {
	selectedValsLocal.value = selectedValsLocal.value.filter((savedTag) => savedTag !== tag);
	emitValue();
}

function emitValue() {
	emit('input', selectedVals.value);
}

function clearVariable() {
	emit('input', null);
}
</script>

<template>
	<div class="interface-tags">
		<VInput
			v-if="isVariableMode || allowCustom"
			:model-value="isVariableMode ? (value as string) : undefined"
			:placeholder="isVariableMode ? undefined : placeholder || $t('interfaces.tags.add_tags')"
			:disabled="isVariableMode || disabled"
			:non-editable="isVariableMode || nonEditable"
			:dir="direction"
			@keydown="onInput"
			@blur="onInput"
		>
			<template v-if="!isVariableMode && iconLeft" #prepend><VIcon :name="iconLeft" /></template>
			<template #append>
				<VIcon
					v-if="isVariableMode"
					v-tooltip="$t('clear_value')"
					class="remove-variable"
					name="close"
					clickable
					@click.stop="clearVariable"
				/>
				<VIcon v-else :name="iconRight" />
			</template>
		</VInput>
		<div v-if="presetVals.length > 0 || customVals.length > 0" class="tags">
			<span v-if="presetVals.length > 0" class="presets tag-container">
				<VChip
					v-for="preset in presetVals"
					:key="preset"
					:class="['tag', { inactive: !selectedVals.includes(preset), 'non-editable': nonEditable }]"

Review comment (suggestion):
Let’s revert this line as the placeholder won’t be displayed in variable mode anyway.

```suggestion
			:placeholder="placeholder || $t('interfaces.tags.add_tags')"
```

Code after:
function addTag(tag: string) {
	if (!tag || tag === '') return;
	// Remove any leading / trailing whitespace from the value
	tag = tag.trim();
	// Convert the tag to lowercase
	selectedValsLocal.value.push(tag);
	emitValue();
}

function removeTag(tag: string) {
	selectedValsLocal.value = selectedValsLocal.value.filter((savedTag) => savedTag !== tag);
	emitValue();
}

function emitValue() {
	emit('input', selectedVals.value);
}

function clearVariable() {
	emit('input', null);
}
</script>

<template>
	<div class="interface-tags">
		<VInput
			v-if="isVariableMode || allowCustom"
			:model-value="isVariableMode ? (value as string) : undefined"
			:placeholder="placeholder || $t('interfaces.tags.add_tags')"
			:disabled="isVariableMode || disabled"
			:non-editable="isVariableMode || nonEditable"
			:dir="direction"
			@keydown="onInput"
			@blur="onInput"
		>
			<template v-if="iconLeft" #prepend><VIcon :name="iconLeft" /></template>
			<template #append>
				<VIcon
					v-if="isVariableMode"
					v-tooltip="$t('clear_value')"
					class="remove-variable"
					name="close"
					clickable
					:disabled
					@click.stop="clearVariable"
				/>
				<VIcon v-else :name="iconRight" />
			</template>
		</VInput>
		<div v-if="presetVals.length > 0 || customVals.length > 0" class="tags">
			<span v-if="presetVals.length > 0" class="presets tag-container">
Repository: directus/directus
File: app/src/interfaces/tags/tags.vue (Vue)

Code before:
	selectedValsLocal.value = selectedValsLocal.value.filter((savedTag) => savedTag !== tag);
	emitValue();
}

function emitValue() {
	emit('input', selectedVals.value);
}

function clearVariable() {
	emit('input', null);
}
</script>

<template>
	<div class="interface-tags">
		<VInput
			v-if="isVariableMode || allowCustom"
			:model-value="isVariableMode ? (value as string) : undefined"
			:placeholder="isVariableMode ? undefined : placeholder || $t('interfaces.tags.add_tags')"
			:disabled="isVariableMode || disabled"
			:non-editable="isVariableMode || nonEditable"
			:dir="direction"
			@keydown="onInput"
			@blur="onInput"
		>
			<template v-if="!isVariableMode && iconLeft" #prepend><VIcon :name="iconLeft" /></template>
			<template #append>
				<VIcon
					v-if="isVariableMode"
					v-tooltip="$t('clear_value')"
					class="remove-variable"
					name="close"
					clickable
					@click.stop="clearVariable"
				/>
				<VIcon v-else :name="iconRight" />
			</template>
		</VInput>
		<div v-if="presetVals.length > 0 || customVals.length > 0" class="tags">
			<span v-if="presetVals.length > 0" class="presets tag-container">
				<VChip
					v-for="preset in presetVals"
					:key="preset"
					:class="['tag', { inactive: !selectedVals.includes(preset), 'non-editable': nonEditable }]"
					:disabled
					:dir="direction"
					small
					label
					clickable
					@click="toggleTag(preset)"
				>

Review comment (suggestion):
Let’s revert this line as it is not related to the variable mode.

```suggestion
			<template v-if="iconLeft" #prepend><VIcon :name="iconLeft" /></template>
```

Code after:
}

function removeTag(tag: string) {
	selectedValsLocal.value = selectedValsLocal.value.filter((savedTag) => savedTag !== tag);
	emitValue();
}

function emitValue() {
	emit('input', selectedVals.value);
}

function clearVariable() {
	emit('input', null);
}
</script>

<template>
	<div class="interface-tags">
		<VInput
			v-if="isVariableMode || allowCustom"
			:model-value="isVariableMode ? (value as string) : undefined"
			:placeholder="placeholder || $t('interfaces.tags.add_tags')"
			:disabled="isVariableMode || disabled"
			:non-editable="isVariableMode || nonEditable"
			:dir="direction"
			@keydown="onInput"
			@blur="onInput"
		>
			<template v-if="iconLeft" #prepend><VIcon :name="iconLeft" /></template>
			<template #append>
				<VIcon
					v-if="isVariableMode"
					v-tooltip="$t('clear_value')"
					class="remove-variable"
					name="close"
					clickable
					:disabled
					@click.stop="clearVariable"
				/>
				<VIcon v-else :name="iconRight" />
			</template>
		</VInput>
		<div v-if="presetVals.length > 0 || customVals.length > 0" class="tags">
			<span v-if="presetVals.length > 0" class="presets tag-container">
				<VChip
					v-for="preset in presetVals"
					:key="preset"
					:class="['tag', { inactive: !selectedVals.includes(preset), 'non-editable': nonEditable }]"
					:disabled
					:dir="direction"
					small
Repository: directus/directus
File: app/src/interfaces/tags/tags.vue (Vue)

Code before:
					class="custom-tags-delimiter"
					name="chevron_right"
				/>
				<VChip
					v-for="val in customVals"
					:key="val"
					:dir="direction"
					:disabled
					:class="{ 'non-editable': nonEditable }"
					class="tag"
					small
					label
					clickable
					@click="removeTag(val)"
				>
					{{ val }}
				</VChip>
			</span>
		</div>
	</div>
</template>

<style lang="scss" scoped>
.remove-variable {
	display: flex;

	--v-icon-color-hover: var(--v-remove-color, var(--theme--danger));
}

.tags {
	display: flex;
	flex-wrap: wrap;
	align-items: center;
	justify-content: flex-start;
	padding: 4px 0 0;

	span.tag-container {
		display: contents;
	}

	.custom-tags-delimiter,
	.tag {
		margin-block-start: 8px;
		margin-inline-end: 8px;
	}

	.custom-tags-delimiter.disabled {
		--v-icon-color: var(--theme--form--field--input--foreground-subdued);
	}

	.presets {

Review comment (suggestion):
Let’s remove the display definition as it is not used here.

```suggestion
```

Code after:
			<span v-if="customVals.length > 0 && allowCustom" class="custom tag-container">
				<VIcon
					v-if="presetVals.length > 0"
					:class="{ disabled: disabled && !nonEditable }"
					class="custom-tags-delimiter"
					name="chevron_right"
				/>
				<VChip
					v-for="val in customVals"
					:key="val"
					:dir="direction"
					:disabled
					:class="{ 'non-editable': nonEditable }"
					class="tag"
					small
					label
					clickable
					@click="removeTag(val)"
				>
					{{ val }}
				</VChip>
			</span>
		</div>
	</div>
</template>

<style lang="scss" scoped>
.remove-variable {
	--v-icon-color-hover: var(--v-remove-color, var(--theme--danger));
}

.tags {
	display: flex;
	flex-wrap: wrap;
	align-items: center;
	justify-content: flex-start;
	padding: 4px 0 0;

	span.tag-container {
		display: contents;
	}

	.custom-tags-delimiter,
	.tag {
		margin-block-start: 8px;
		margin-inline-end: 8px;
	}

	.custom-tags-delimiter.disabled {
		--v-icon-color: var(--theme--form--field--input--foreground-subdued);
	}
Repository: directus/directus
File: app/src/views/private/components/overlay-item-content.vue (Vue)

Code before:
	return {
		mainInitialValues,
		junctionInitialValues,
	};
}

function useFile() {
	const isDirectusFiles = computed(() => {
		return collection === 'directus_files' || relatedCollection === 'directus_files';
	});

	const file = computed(() => {
		if (isDirectusFiles.value === false) return null;

		const fileData = junctionField ? junctionInitialValues.value : mainInitialValues.value;

		return fileData || null;
	});

	return { file, isDirectusFiles };
}

function useValidationScrollToField() {
	const mainFormEl = useTemplateRef<any>('mainForm');
	const junctionFormEl = useTemplateRef<any>('junctionForm');
	const mainDrawerElement = inject<Ref<HTMLElement | null>>('main-element');

	watch(
		() => validationErrors,
		(newVal, oldVal) => {
			if (isEqual(newVal, oldVal)) return;
			if (newVal?.length > 0) mainDrawerElement?.value?.scrollTo({ top: 0, behavior: 'smooth' });
		},
	);

	return { scrollToField };

	function scrollToField(fieldKey: string) {
		const fieldOfMainForm = fields.find((field: Field) => field.field === fieldKey);

		if (fieldOfMainForm) {
			mainFormEl.value?.$refs?.el?.scrollIntoView({ behavior: 'smooth' });
			return;
		}

		junctionFormEl.value?.$refs?.el?.scrollIntoView({ behavior: 'smooth' });
	}
}
</script>

<template>

Review comment (suggestion):
Using the mainDrawerElement here assumes that overlay-item-content only renders inside the drawer, but it can also render inside a modal or popover. Instead let’s put a templateRef on the ValidationErrors element and use scrollIntoView() to scroll it into the view.

Code after:
		if (!initialValues) return null;
		if (!junctionField || !initialValues[junctionField] || !relatedPrimaryKeyField) return initialValues;

		const tempInitialValues = cloneDeep(initialValues);
		tempInitialValues[junctionField] = initialValues[junctionField][relatedPrimaryKeyField];

		return tempInitialValues;
	});

	const junctionInitialValues = computed(() => {
		if (!initialValues || !junctionField || !initialValues[junctionField]) return null;
		return initialValues[junctionField];
	});

	return {
		mainInitialValues,
		junctionInitialValues,
	};
}

function useFile() {
	const isDirectusFiles = computed(() => {
		return collection === 'directus_files' || relatedCollection === 'directus_files';
	});

	const file = computed(() => {
		if (isDirectusFiles.value === false) return null;

		const fileData = junctionField ? junctionInitialValues.value : mainInitialValues.value;

		return fileData || null;
	});

	return { file, isDirectusFiles };
}

function useValidationScrollToField() {
	const mainFormEl = useTemplateRef<any>('mainForm');
	const junctionFormEl = useTemplateRef<any>('junctionForm');

	return { scrollToField };

	function scrollToField(fieldKey: string) {
		const fieldOfMainForm = fields.find((field: Field) => field.field === fieldKey);

		if (fieldOfMainForm) {
			mainFormEl.value?.$refs?.el?.scrollIntoView({ behavior: 'smooth' });
			return;
		}

		junctionFormEl.value?.$refs?.el?.scrollIntoView({ behavior: 'smooth' });
Repository: directus/directus
File: api/src/services/files.test.ts (TypeScript)

Code before:
				expect(superUpdateOne).toHaveBeenCalledWith(
					sample.id,
					expect.objectContaining({
						...mockDataJPG,
						uploaded_on: mockDate.toISOString(),
						filename_disk: `${sample.id}.jpg`,
					}),
					{ emitEvents: false },
				);

				await service.uploadOne(new PassThrough(), mockDataPNG);

				expect(superUpdateOne).toHaveBeenCalledWith(
					sample.id,
					expect.objectContaining({
						...mockDataPNG,
						uploaded_on: mockDate.toISOString(),
						filename_disk: `${sample.id}.png`,
					}),
					{ emitEvents: false },
				);

				vi.useRealTimers();
			});

			it('returns 500 for permanent filesystem errors', async () => {
				const stream = Readable.from(Buffer.from('test content'));

				// Get real storage instance
				const storage = await getStorage();
				const disk = storage.location('local');

				// Spy on write to simulate EROFS
				vi.spyOn(disk, 'write').mockRejectedValue(Object.assign(new Error('read-only'), { code: 'EROFS' }));

				await expect(
					service.uploadOne(stream, {
						storage: 'local',
						filename_download: 'test.txt',
						type: 'text/plain',
					} as any),
				).rejects.toBeInstanceOf(InternalServerError);
			});
		});
	});
});


Review comment (suggestion):
The test only covers `EROFS`. mind adding `EACCES` and `EPERM` as well? A `test.each` would do the trick 🙏

Code after:

				expect(superUpdateOne).toHaveBeenCalledWith(
					sample.id,
					expect.objectContaining({
						...mockDataJPG,
						uploaded_on: mockDate.toISOString(),
						filename_disk: `${sample.id}.jpg`,
					}),
					{ emitEvents: false },
				);

				await service.uploadOne(new PassThrough(), mockDataPNG);

				expect(superUpdateOne).toHaveBeenCalledWith(
					sample.id,
					expect.objectContaining({
						...mockDataPNG,
						uploaded_on: mockDate.toISOString(),
						filename_disk: `${sample.id}.png`,
					}),
					{ emitEvents: false },
				);

				vi.useRealTimers();
			});

			describe('uploadOne - permanent filesystem errors', () => {
				const errorCodes = ['EROFS', 'EACCES', 'EPERM'] as const;

				test.each(errorCodes)('returns 500 for %s filesystem error', async (code: any) => {
					const stream = Readable.from(Buffer.from('test content'));

					const storage = await getStorage();
					const disk = storage.location('local');

					vi.spyOn(disk, 'write').mockRejectedValue(Object.assign(new Error('fs error'), { code }));

					await expect(
						service.uploadOne(stream, {
							storage: 'local',
							filename_download: 'test.txt',
							type: 'text/plain',
						} as any),
					).rejects.toBeInstanceOf(InternalServerError);
				});
			});
		});
	});
});
Repository: directus/directus
File: app/src/interfaces/list-o2m/list-o2m.vue (Vue)

Code before:
			.map((key) => {
				const field = fieldsStore.getField(relatedCollection, key);

				// when user has no permission to this field or junction collection
				if (!field) return null;

				const defaultWidth =
					contentWidth[key] !== undefined && contentWidth[key] < 10 ? contentWidth[key] * 16 + 10 : 160;

				return {
					text: field.name,
					value: key,
					width: columnWidths.value[key] ?? defaultWidth,
					sortable: !['json'].includes(field.type),
				};
			})
			.filter((h) => h !== null);
	},
	set: (val: Array<any>) => {
		const widths: Record<string, number> = {};

		val.forEach((h) => {
			widths[h.value] = h.width ?? 160;
		});

		columnWidths.value = { ...columnWidths.value, ...widths };
	},
});

const spacings = {
	compact: 32,
	cozy: 48,
	comfortable: 64,
};

const tableRowHeight = computed(() => spacings[props.tableSpacing] ?? spacings.cozy);

const allowDrag = computed(() => totalItemCount.value <= limit.value && relationInfo.value?.sortField !== undefined);

function sortItems(items: DisplayItem[]) {
	const info = relationInfo.value;
	const sortField = info?.sortField;
	if (!info || !sortField) return;

	const sortedItems = items.map((item, index) => {
		const relatedId = item?.[info.relatedPrimaryKeyField.field];

		const changes: Record<string, any> = {
			$index: item.$index,
			$type: item.$type,
			$edits: item.$edits,

Review comment (suggestion):
This is being triggered over and over while the mouse drags the column handle. Let's change this so that it either A, only writes to local storage when the user stops dragging, or B, debounce this to reduce the number of writes.

Code after:
				const field = fieldsStore.getField(relatedCollection, key);

				// when user has no permission to this field or junction collection
				if (!field) return null;

				const defaultWidth =
					contentWidth[key] !== undefined && contentWidth[key] < 10 ? contentWidth[key] * 16 + 10 : 160;

				return {
					text: field.name,
					value: key,
					width: getWidth(key, defaultWidth),
					sortable: !['json'].includes(field.type),
				};
			})
			.filter((h) => h !== null);
	},
	set: (val: Array<any>) => {
		const currentHeaders = headers.value;

		const changed = val.filter((h) => {
			const current = currentHeaders.find((ch: any) => ch.value === h.value);
			return current && current.width !== h.width;
		});

		if (changed.length > 0) updateWidths(changed);
	},
});

const spacings = {
	compact: 32,
	cozy: 48,
	comfortable: 64,
};

const tableRowHeight = computed(() => spacings[props.tableSpacing] ?? spacings.cozy);

const allowDrag = computed(() => totalItemCount.value <= limit.value && relationInfo.value?.sortField !== undefined);

function sortItems(items: DisplayItem[]) {
	const info = relationInfo.value;
	const sortField = info?.sortField;
	if (!info || !sortField) return;

	const sortedItems = items.map((item, index) => {
		const relatedId = item?.[info.relatedPrimaryKeyField.field];

		const changes: Record<string, any> = {
			$index: item.$index,
			$type: item.$type,
			$edits: item.$edits,