ronantakizawa/codereview-bench huggingface dataset:
Repository: withastro/astro
File: packages/astro/src/types/public/config.ts (TypeScript)

Code before:
					 * ```astro
					 * ---
					 * import Logo from './path/to/svg/file.svg';
					 * ---
					 *
					 * <Logo size={24} mode="sprite" />
					 * ```
					 */
					mode: SvgRenderMode;
			  };

		/**
		 * @name experimental.serializeConfig
		 * @type {boolean}
		 * @default `false`
		 * @version 5.x
		 * @description
		 *
		 * Enables the use of the experimental virtual modules `astro:config/server` and `astro:config/client`.
		 *
		 * These two virtual modules contain a serializable subset of the Astro configuration.
		 */
		serializeConfig?: boolean;

		/**
		 * @name experimental.serializeConfig
		 * @type {boolean}
		 * @default `false`
		 * @version 5.x
		 * @description
		 * 
		 * When enabled, scripts and styles are rendered in the same order as they are defined.
		 */
		preserveScriptOrder: boolean;
	};
}

/**
 * Resolved Astro Config
 *
 * Config with user settings along with all defaults filled in.
 */
export interface AstroConfig extends AstroConfigType {
	// Public:
	// This is a more detailed type than zod validation gives us.
	// TypeScript still confirms zod validation matches this type.
	integrations: AstroIntegration[];
}
/**
 * An inline Astro config that takes highest priority when merging with the user config,
 * and includes inline-specific options to configure how Astro runs.

Review comment (suggestion):
```suggestion
		 * @name experimental.preserveScriptOrder
```

Code after:
		 * const cart = await Astro.session.get('cart');
		 * ---
		 *
		 * <a href="/checkout">🛒 {cart?.length ?? 0} items</a>
		 *
		 * ```
		 *
		 * For more details, see [the experimental session guide](https://docs.astro.build/en/reference/experimental-flags/sessions/).
		 *
		 */

		session?: boolean;
		/**
		 *
		 * @name experimental.svg
		 * @type {boolean|object}
		 * @default `undefined`
		 * @version 5.x
		 * @description
		 *
		 * This feature allows you to import SVG files directly into your Astro project. By default, Astro will inline the SVG content into your HTML output.
		 *
		 * To enable this feature, set `experimental.svg` to `true` in your Astro config:
		 *
		 * ```js
		 * {
		 *   experimental: {
		 * 	   svg: true,
		 * 	 },
		 * }
		 * ```
		 *
		 * To use this feature, import an SVG file in your Astro project, passing any common SVG attributes to the imported component.
		 * Astro also provides a `size` attribute to set equal `height` and `width` properties:
		 *
		 * ```astro
		 * ---
		 * import Logo from './path/to/svg/file.svg';
		 * ---
		 *
		 * <Logo size={24} />
		 * ```
		 *
		 * For a complete overview, and to give feedback on this experimental API,
		 * see the [Feature RFC](https://github.com/withastro/roadmap/pull/1035).
		 */
		svg?:
			| boolean
			| {
					/**
					 *
Repository: withastro/astro
File: packages/astro/src/core/config/schema.ts (TypeScript)

Code before:
								mode: 'inline' as SvgRenderMode,
							};
						}
					}
					return svgConfig;
				}),

			fonts: z
				.object({
					providers: z.array(fontProviderSchema).optional(),
					families: z.array(
						z.union([
							// Shorthand
							z
								.string()
								.transform(
									(name): FontFamilyAttributes => ({
										name,
										provider: GOOGLE_PROVIDER_NAME,
									}),
								),
							localFontFamilySchema,
							commonFontFamilySchema,
						]),
					),
				})
				.strict()
				.optional()
				.superRefine((fonts, ctx) => {
					if (!fonts) {
						return;
					}
					const providersNames = [
						...BUILTIN_PROVIDERS,
						...(fonts.providers ?? []).map((provider) => provider.name),
					];
					const visited = new Map<string, 'name' | 'as'>();

					for (let i = 0; i < fonts.families.length; i++) {
						const family = fonts.families[i];

						// Check for invalid providers
						if (!providersNames.includes(family.provider)) {
							ctx.addIssue({
								code: z.ZodIssueCode.custom,
								message: `Invalid provider "${family.provider}". Please use of the following: ${providersNames.map((name) => `"${name}"`).join(', ')}`,
								path: ['families', i, 'provider'],
							});
						}

						if (family.as) {

Review comment (bug):
I would avoid a `transform` inside for our configuration. I used `.transform` in the past, which is the side effect of passing the transformed configuration **to the integrations**. This means they will have incorrect types, and they might be able to delete our defaults.

I experienced this inconvenience when I implemented i18n. I suggest injecting these defaults after the integrations run their hooks.

Code after:
								mode: 'inline' as SvgRenderMode,
							};
						}
					}
					return svgConfig;
				}),

			fonts: z
				.object({
					providers: z.array(fontProviderSchema).optional(),
					families: z.array(
						z.union([
							// Shorthand
							z
								.string()
								.transform(
									(name): FontFamilyAttributes => ({
										name,
										provider: GOOGLE_PROVIDER_NAME,
									}),
								),
							localFontFamilySchema,
							commonFontFamilySchema.transform((family) => ({
								provider: GOOGLE_PROVIDER_NAME,
								...family,
							})),
						]),
					),
				})
				.strict()
				.optional()
				.superRefine((fonts, ctx) => {
					if (!fonts) {
						return;
					}
					const providersNames = [
						...BUILTIN_PROVIDERS,
						...(fonts.providers ?? []).map((provider) => provider.name),
					];
					const visited = new Map<string, 'name' | 'as'>();

					for (let i = 0; i < fonts.families.length; i++) {
						const family = fonts.families[i];

						// Check for invalid providers
						if (!providersNames.includes(family.provider)) {
							ctx.addIssue({
								code: z.ZodIssueCode.custom,
								message: `Invalid provider "${family.provider}". Please use of the following: ${providersNames.map((name) => `"${name}"`).join(', ')}`,
								path: ['families', i, 'provider'],
							});
Repository: withastro/astro
File: packages/astro/src/assets/vite-plugin-assets.ts (TypeScript)

Code before:

					const emitFile = shouldEmitFile ? this.emitFile.bind(this) : undefined;
					const imageMetadata = await emitESMImage(
						id,
						this.meta.watchMode,
						!!settings.config.experimental.svg,
						emitFile,
					);

					if (!imageMetadata) {
						throw new AstroError({
							...AstroErrorData.ImageNotFound,
							message: AstroErrorData.ImageNotFound.message(id),
						});
					}

					if (settings.config.experimental.svg && /\.svg$/.test(id)) {
						const { contents, ...metadata } = imageMetadata;
						// We know that the contents are present, as we only emit this property for SVG files
						return {
							code: makeSvgComponent(metadata, contents!, {
								mode: settings.config.experimental.svg.mode,
							}),
							moduleType: 'js',
						};
					}

					// We can only reliably determine if an image is used on the server, as we need to track its usage throughout the entire build.
					// Since you cannot use image optimization on the client anyway, it's safe to assume that if the user imported
					// an image on the client, it should be present in the final build.
					if (options?.ssr) {
						return {
							code: `export default ${getProxyCode(
								imageMetadata,
								settings.buildOutput === 'server',
							)}`,
							moduleType: 'js',
						};
					} else {
						globalThis.astroAsset.referencedImages.add(imageMetadata.fsPath);
						return {
							code: `export default ${JSON.stringify(imageMetadata)}`,
							moduleType: 'js',
						};
					}
				}
			},
		},
	];
}


Review comment (suggestion):
Makes it an object that returns `code` (rollup/rolldown compatible shape) and `moduleType` (only rolldown I think)

Code after:
					const emitFile = shouldEmitFile ? this.emitFile.bind(this) : undefined;
					const imageMetadata = await emitESMImage(
						id,
						this.meta.watchMode,
						!!settings.config.experimental.svg,
						emitFile,
					);

					if (!imageMetadata) {
						throw new AstroError({
							...AstroErrorData.ImageNotFound,
							message: AstroErrorData.ImageNotFound.message(id),
						});
					}

					if (settings.config.experimental.svg && /\.svg$/.test(id)) {
						const { contents, ...metadata } = imageMetadata;
						// We know that the contents are present, as we only emit this property for SVG files
						return { code: makeSvgComponent(metadata, contents!) };
					}

					// We can only reliably determine if an image is used on the server, as we need to track its usage throughout the entire build.
					// Since you cannot use image optimization on the client anyway, it's safe to assume that if the user imported
					// an image on the client, it should be present in the final build.
					if (options?.ssr) {
						return {
							code: `export default ${getProxyCode(
								imageMetadata,
								settings.buildOutput === 'server',
							)}`,
						};
					} else {
						globalThis.astroAsset.referencedImages.add(imageMetadata.fsPath);
						return {
							code: `export default ${JSON.stringify(imageMetadata)}`,
						};
					}
				}
			},
		},
	];
}
Repository: withastro/astro
File: packages/astro/src/assets/vite-plugin-assets.ts (TypeScript)

Code before:

					if (!imageMetadata) {
						throw new AstroError({
							...AstroErrorData.ImageNotFound,
							message: AstroErrorData.ImageNotFound.message(id),
						});
					}

					if (settings.config.experimental.svg && /\.svg$/.test(id)) {
						const { contents, ...metadata } = imageMetadata;
						// We know that the contents are present, as we only emit this property for SVG files
						return {
							code: makeSvgComponent(metadata, contents!, {
								mode: settings.config.experimental.svg.mode,
							}),
							moduleType: 'js',
						};
					}

					// We can only reliably determine if an image is used on the server, as we need to track its usage throughout the entire build.
					// Since you cannot use image optimization on the client anyway, it's safe to assume that if the user imported
					// an image on the client, it should be present in the final build.
					if (options?.ssr) {
						return {
							code: `export default ${getProxyCode(
								imageMetadata,
								settings.buildOutput === 'server',
							)}`,
							moduleType: 'js',
						};
					} else {
						globalThis.astroAsset.referencedImages.add(imageMetadata.fsPath);
						return {
							code: `export default ${JSON.stringify(imageMetadata)}`,
							moduleType: 'js',
						};
					}
				}
			},
		},
	];
}


Review comment (suggestion):
Makes it an object that returns `code` (rollup/rolldown compatible shape) and `moduleType` (only rolldown I think)

Code after:
					if (!imageMetadata) {
						throw new AstroError({
							...AstroErrorData.ImageNotFound,
							message: AstroErrorData.ImageNotFound.message(id),
						});
					}

					if (settings.config.experimental.svg && /\.svg$/.test(id)) {
						const { contents, ...metadata } = imageMetadata;
						// We know that the contents are present, as we only emit this property for SVG files
						return { code: makeSvgComponent(metadata, contents!) };
					}

					// We can only reliably determine if an image is used on the server, as we need to track its usage throughout the entire build.
					// Since you cannot use image optimization on the client anyway, it's safe to assume that if the user imported
					// an image on the client, it should be present in the final build.
					if (options?.ssr) {
						return {
							code: `export default ${getProxyCode(
								imageMetadata,
								settings.buildOutput === 'server',
							)}`,
						};
					} else {
						globalThis.astroAsset.referencedImages.add(imageMetadata.fsPath);
						return {
							code: `export default ${JSON.stringify(imageMetadata)}`,
						};
					}
				}
			},
		},
	];
}
Repository: withastro/astro
File: packages/astro/client.d.ts (TypeScript)

Code before:
	};

	type ImgAttributes = import('./dist/type-utils.js').WithRequired<
		Omit<import('./types').HTMLAttributes<'img'>, 'src' | 'width' | 'height'>,
		'alt'
	>;

	export type LocalImageProps = import('./dist/type-utils.js').Simplify<
		import('./dist/assets/types.js').LocalImageProps<ImgAttributes>
	>;
	export type RemoteImageProps = import('./dist/type-utils.js').Simplify<
		import('./dist/assets/types.js').RemoteImageProps<ImgAttributes>
	>;
	export const {
		getImage,
		getConfiguredImageService,
		imageConfig,
		Image,
		Picture,
		Font,
		inferRemoteSize,
	}: AstroAssets;
}

type ImageMetadata = import('./dist/assets/types.js').ImageMetadata;
type SvgComponent = import('./dist/assets/types.js').SvgComponent;

declare module '*.gif' {
	const metadata: ImageMetadata;
	export default metadata;
}
declare module '*.jpeg' {
	const metadata: ImageMetadata;
	export default metadata;
}
declare module '*.jpg' {
	const metadata: ImageMetadata;
	export default metadata;
}
declare module '*.png' {
	const metadata: ImageMetadata;
	export default metadata;
}
declare module '*.tiff' {
	const metadata: ImageMetadata;
	export default metadata;
}
declare module '*.webp' {
	const metadata: ImageMetadata;
	export default metadata;
}

Review comment (suggestion):
Otherwise the type would be global, which is confusing IMO. Check my next suggestion
```suggestion
```

Code after:
	type ImgAttributes = import('./dist/type-utils.js').WithRequired<
		Omit<import('./types').HTMLAttributes<'img'>, 'src' | 'width' | 'height'>,
		'alt'
	>;

	export type LocalImageProps = import('./dist/type-utils.js').Simplify<
		import('./dist/assets/types.js').LocalImageProps<ImgAttributes>
	>;
	export type RemoteImageProps = import('./dist/type-utils.js').Simplify<
		import('./dist/assets/types.js').RemoteImageProps<ImgAttributes>
	>;
	export const {
		getImage,
		getConfiguredImageService,
		imageConfig,
		Image,
		Picture,
		Font,
		inferRemoteSize,
	}: AstroAssets;
}

type ImageMetadata = import('./dist/assets/types.js').ImageMetadata;

declare module '*.gif' {
	const metadata: ImageMetadata;
	export default metadata;
}
declare module '*.jpeg' {
	const metadata: ImageMetadata;
	export default metadata;
}
declare module '*.jpg' {
	const metadata: ImageMetadata;
	export default metadata;
}
declare module '*.png' {
	const metadata: ImageMetadata;
	export default metadata;
}
declare module '*.tiff' {
	const metadata: ImageMetadata;
	export default metadata;
}
declare module '*.webp' {
	const metadata: ImageMetadata;
	export default metadata;
}
declare module '*.avif' {
	const metadata: ImageMetadata;
	export default metadata;
Repository: withastro/astro
File: packages/astro/src/assets/fonts/vite-plugin-fonts.ts (TypeScript)

Code before:

		const root = settings.config.root;
		const pathsToWarn = new Set<string>();

		for (const family of settings.config.experimental.fonts!) {
			families.push(
				await resolveFontFamily({
					family,
					root,
					resolveMod,
					generateNameWithHash: (_family) =>
						`${withoutQuotes(_family.name)}-${h64ToString(JSON.stringify(sortObjectByKey(_family)))}`,
					resolveLocalEntrypoint: (url) => {
						const resolvedPath = fileURLToPath(resolveEntrypoint(root, url));
						if (resolvedPath.startsWith(fileURLToPath(settings.config.publicDir))) {
							pathsToWarn.add(resolvedPath);
						}
						return resolvedPath;
					},
				}),
			);
		}

		for (const path of [...pathsToWarn]) {
			logger.warn(
				'assets',
				`Found a local font file ${JSON.stringify(path)} in the \`public/\` folder. To avoid duplicated files in the build output, move this file into \`src/\``,
			);
		}

		await loadFonts({
			base: baseUrl,
			families,
			storage,
			hashToUrlMap,
			resolvedMap,
			hashString: h64ToString,
			generateFallbackFontFace,
			getMetricsForFamily: async (name, font) => {
				return await readMetrics(name, await cache(storage!, font.hash, () => fetchFont(font.url)));
			},
			log: (message) => logger.info('assets', message),
		});
	}

	return {
		name: 'astro:fonts',
		config(_, { command }) {
			isBuild = command === 'build';
		},
		async buildStart() {

Review comment (suggestion):
```suggestion
			// TODO: remove when stabilizing
			logger.warn(
```

Code after:

		const root = settings.config.root;
		const pathsToWarn = new Set<string>();

		for (const family of settings.config.experimental.fonts!) {
			families.push(
				await resolveFontFamily({
					family,
					root,
					resolveMod,
					generateNameWithHash: (_family) =>
						`${withoutQuotes(_family.name)}-${h64ToString(JSON.stringify(sortObjectByKey(_family)))}`,
					resolveLocalEntrypoint: (url) => {
						const resolvedPath = fileURLToPath(resolveEntrypoint(root, url));
						if (resolvedPath.startsWith(fileURLToPath(settings.config.publicDir))) {
							pathsToWarn.add(resolvedPath);
						}
						return resolvedPath;
					},
				}),
			);
		}

		for (const path of [...pathsToWarn]) {
			// TODO: remove when stabilizing
			logger.warn(
				'assets',
				`Found a local font file ${JSON.stringify(path)} in the \`public/\` folder. To avoid duplicated files in the build output, move this file into \`src/\``,
			);
		}

		await loadFonts({
			base: baseUrl,
			families,
			storage,
			hashToUrlMap,
			resolvedMap,
			hashString: h64ToString,
			generateFallbackFontFace,
			getMetricsForFamily: async (name, font) => {
				return await readMetrics(name, await cache(storage!, font.hash, () => fetchFont(font.url)));
			},
			log: (message) => logger.info('assets', message),
		});
	}

	return {
		name: 'astro:fonts',
		config(_, { command }) {
			isBuild = command === 'build';
		},
Repository: withastro/astro
File: packages/astro/src/content/utils.ts (TypeScript)

Code before:
function searchConfig(
	fs: typeof fsMod,
	srcDir: URL,
	legacy?: boolean,
): { exists: boolean; url: URL } {
	const paths = [
		...(legacy
			? []
			: ['content.config.mjs', 'content.config.js', 'content.config.mts', 'content.config.ts']),
		'content/config.mjs',
		'content/config.js',
		'content/config.mts',
		'content/config.ts',
	];
	return search(fs, srcDir, paths);
}

function searchLiveConfig(fs: typeof fsMod, srcDir: URL): { exists: boolean; url: URL } {
	const paths = [
		'live-content.config.mjs',
		'live-content.config.js',
		'live-content.config.mts',
		'live-content.config.ts',
	];
	return search(fs, srcDir, paths);
}

function search(fs: typeof fsMod, srcDir: URL, paths: string[]): { exists: boolean; url: URL } {
	const urls = paths.map((p) => new URL(`./${p}`, srcDir));
	for (const file of urls) {
		if (fs.existsSync(file)) {
			return { exists: true, url: file };
		}
	}
	return { exists: false, url: urls[0] };
}

/**
 * Check for slug in content entry frontmatter and validate the type,
 * falling back to the `generatedSlug` if none is found.
 */
export async function getEntrySlug({
	id,
	collection,
	generatedSlug,
	contentEntryType,
	fileUrl,
	fs,
}: {
	fs: typeof fsMod;
	id: string;

Review comment (suggestion):
Some feedback:
- `content.config` disappears at the end of he build, but what about the `live.config` file? This isn't clear from the PR and the RFC. 
- nit: not very fond of the `live-content` name, because it could be `liveContent` too. Maybe `live.config.*` works better


Code after:
function searchConfig(
	fs: typeof fsMod,
	srcDir: URL,
	legacy?: boolean,
): { exists: boolean; url: URL } {
	const paths = [
		...(legacy
			? []
			: ['content.config.mjs', 'content.config.js', 'content.config.mts', 'content.config.ts']),
		'content/config.mjs',
		'content/config.js',
		'content/config.mts',
		'content/config.ts',
	];
	return search(fs, srcDir, paths);
}

function searchLiveConfig(fs: typeof fsMod, srcDir: URL): { exists: boolean; url: URL } {
	const paths = [
		'live.config.mjs',
		'live.config.js',
		'live.config.mts',
		'live.config.ts',
	];
	return search(fs, srcDir, paths);
}

function search(fs: typeof fsMod, srcDir: URL, paths: string[]): { exists: boolean; url: URL } {
	const urls = paths.map((p) => new URL(`./${p}`, srcDir));
	for (const file of urls) {
		if (fs.existsSync(file)) {
			return { exists: true, url: file };
		}
	}
	return { exists: false, url: urls[0] };
}

/**
 * Check for slug in content entry frontmatter and validate the type,
 * falling back to the `generatedSlug` if none is found.
 */
export async function getEntrySlug({
	id,
	collection,
	generatedSlug,
	contentEntryType,
	fileUrl,
	fs,
}: {
	fs: typeof fsMod;
	id: string;
Repository: withastro/astro
File: examples/ssr/src/api.ts (TypeScript)

Code before:
export interface Product {
	id: number;
	name: string;
	price: number;
	image: string;
}

interface User {
	id: number;
}

interface Cart {
	items: Array<{
		id: number;
		name: string;
		count: number;
	}>;
}

async function getJson<T>(
	incomingReq: Request,
	endpoint: string,
	timeout = 10_000
): Promise<T> {
	const origin = new URL(incomingReq.url).origin;
	const ctrl = new AbortController();
	const timer = setTimeout(() => ctrl.abort(), timeout);

	try {
		const response = await fetch(`${origin}${endpoint}`, {
			credentials: 'same-origin',
			headers: incomingReq.headers,
			signal: ctrl.signal,
		});
		if (!response.ok) {
			throw new Error(`GET ${endpoint} failed: ${response.statusText}`);
		}
		return response.json() as Promise<T>;
	} catch (error) {
		if (error instanceof DOMException || error instanceof TypeError) {
			throw new Error(`GET ${endpoint} failed: ${error.message}`);
		}
		throw error;
	} finally {
		clearTimeout(timer);
	}
}


Review comment (suggestion):
Since the timeout isn't used, i think it could be worth removing it altogether from the example. We try to keep examples minimal and as a starting point. So that probably means removing the abort controller too

Code after:
export interface Product {
	id: number;
	name: string;
	price: number;
	image: string;
}

interface User {
	id: number;
}

interface Cart {
	items: Array<{
		id: number;
		name: string;
		count: number;
	}>;
}

async function getJson<T>(
	incomingReq: Request,
	endpoint: string,
): Promise<T> {
	const origin = new URL(incomingReq.url).origin;
	try {
		const response = await fetch(`${origin}${endpoint}`, {
			credentials: 'same-origin',
			headers: incomingReq.headers,
		});
		if (!response.ok) {
			throw new Error(`GET ${endpoint} failed: ${response.statusText}`);
		}
		return response.json() as Promise<T>;
	} catch (error) {
		if (error instanceof DOMException || error instanceof TypeError) {
			throw new Error(`GET ${endpoint} failed: ${error.message}`);
		}
		throw error;
	}
}

export async function getProducts(incomingReq: Request): Promise<Product[]> {
	return getJson<Product[]>(incomingReq, '/api/products');
}

export async function getProduct(incomingReq: Request, id: number): Promise<Product> {
	return getJson<Product>(incomingReq, `/api/products/${id}`);
}