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

Code before:
};

export interface IntegrationResolvedRoute {
	/**
	 * The current **pattern** of the route. For example:
	 * - `src/pages/index.astro` has a pattern of `/`
	 * - `src/pages/blog/[...slug].astro` has a pattern of `/blog/[...slug]`
	 * - `src/pages/site/[blog]/[...slug].astro` has a pattern of `/site/[blog]/[...slug]`
	 */
	pattern: RouteData['route'];

	/**
	 *  Source component URL
	 */
	entrypoint: RouteData['component'];

	/**
	 * Whether the route is prerendered or not
	 */
	prerendered: RouteData['prerender'];

	/**
	 * Dynamic and spread route params
	 * ex. "/pages/[lang]/[...slug].astro" will output the params ['lang', '...slug']
	 */
	params: RouteData['params'];

	/**
	 * Whether the route comes from Astro core, an integration or the user's project
	 */
	origin: RouteData['origin'];
}


Review comment (suggestion):
I'll miss the `generate` function from `IntegrationRouteData` to generate URLs for a given route. Integrations will have to construct their own URL generator function based on the `pattern` and `params` which seems somewhat unnecessary since that function already exists in Astro and just isn't exposed.

It is not a blocker since it can be done in user-land

Code after:
	 */
	redirectRoute?: IntegrationRouteData;
};

export interface IntegrationResolvedRoute
	extends Pick<
		RouteData,
		'generate' | 'params' | 'pathname' | 'segments' | 'type' | 'redirect' | 'origin'
	> {
	/**
	 * {@link RouteData.route}
	 */
	pattern: RouteData['route'];

	/**
	 * {@link RouteData.pattern}
	 */
	patternRegex: RouteData['pattern'];

	/**
	 * {@link RouteData.component}
	 */
	entrypoint: RouteData['component'];

	/**
	 * {@link RouteData.prerender}
	 */
	isPrerendered: RouteData['prerender'];

	/**
	 * {@link RouteData.redirectRoute}
	 */
	redirectRoute?: IntegrationResolvedRoute;
}
Repository: withastro/astro
File: packages/db/src/core/cli/commands/execute/index.ts (TypeScript)

Code before:
import { existsSync } from 'node:fs';
import type { AstroConfig } from 'astro';
import { green } from 'kleur/colors';
import type { Arguments } from 'yargs-parser';
import {
	EXEC_DEFAULT_EXPORT_ERROR,
	EXEC_ERROR,
	FILE_NOT_FOUND_ERROR,
	MISSING_EXECUTE_PATH_ERROR,
} from '../../../errors.js';
import {
	getLocalVirtualModContents,
	getStudioVirtualModContents,
} from '../../../integration/vite-plugin-db.js';
import { bundleFile, importBundledFile } from '../../../load-file.js';
import type { DBConfig } from '../../../types.js';
import { getManagedRemoteToken } from '../../../utils.js';
import { isDbError } from '../../../../runtime/virtual.js';

export async function cmd({
	astroConfig,
	dbConfig,
	flags,
}: {
	astroConfig: AstroConfig;
	dbConfig: DBConfig;
	flags: Arguments;
}) {
	const filePath = flags._[4];
	if (typeof filePath !== 'string') {
		console.error(MISSING_EXECUTE_PATH_ERROR);
		process.exit(1);
	}

	const fileUrl = new URL(filePath, astroConfig.root);
	if (!existsSync(fileUrl)) {
		console.error(FILE_NOT_FOUND_ERROR(filePath));
		process.exit(1);
	}

	let virtualModContents: string;
	if (flags.remote) {
		const appToken = await getManagedRemoteToken(flags.token);

Review comment (bug):
I don't know if this causes the CI fail, but I _think_ we may need to copy `isDbError` into `../../../utils.js` so there's two implementations of it for now. The `virtual.js` is only for the `astro:db` virtual module and we don't want to mix it with the non-virtual code.

Code after:
import { existsSync } from 'node:fs';
import type { AstroConfig } from 'astro';
import { green } from 'kleur/colors';
import type { Arguments } from 'yargs-parser';
import {
	EXEC_DEFAULT_EXPORT_ERROR,
	EXEC_ERROR,
	FILE_NOT_FOUND_ERROR,
	MISSING_EXECUTE_PATH_ERROR,
} from '../../../errors.js';
import {
	getLocalVirtualModContents,
	getStudioVirtualModContents,
} from '../../../integration/vite-plugin-db.js';
import { bundleFile, importBundledFile } from '../../../load-file.js';
import type { DBConfig } from '../../../types.js';
import { getManagedRemoteToken } from '../../../utils.js';
import { isDbError } from '../../../../runtime/utils.js';

export async function cmd({
	astroConfig,
	dbConfig,
	flags,
}: {
	astroConfig: AstroConfig;
	dbConfig: DBConfig;
	flags: Arguments;
}) {
	const filePath = flags._[4];
	if (typeof filePath !== 'string') {
		console.error(MISSING_EXECUTE_PATH_ERROR);
		process.exit(1);
	}

	const fileUrl = new URL(filePath, astroConfig.root);
	if (!existsSync(fileUrl)) {
		console.error(FILE_NOT_FOUND_ERROR(filePath));
		process.exit(1);
	}

	let virtualModContents: string;
	if (flags.remote) {
		const appToken = await getManagedRemoteToken(flags.token);
Repository: withastro/astro
File: packages/astro/src/assets/build/generate.ts (TypeScript)

Code before:
type GenerationData = GenerationDataUncached | GenerationDataCached;

type AssetEnv = {
	logger: Logger;
	isSSR: boolean;
	count: { total: number; current: number };
	useCache: boolean;
	assetsCacheDir: URL;
	serverRoot: URL;
	clientRoot: URL;
	imageConfig: AstroConfig['image'];
	assetsFolder: AstroConfig['build']['assets'];
};

type ImageData = {
	data: Uint8Array;
	expires: number;
	etag?: string | null;
};

enum CacheStatus {
	Miss = 0,
	Revalidated,
	Hit,
}

export async function prepareAssetsGenerationEnv(
	pipeline: BuildPipeline,
	totalCount: number,
): Promise<AssetEnv> {
	const { config, logger } = pipeline;
	let useCache = true;
	const assetsCacheDir = new URL('assets/', config.cacheDir);
	const count = { total: totalCount, current: 1 };

	// Ensure that the cache directory exists
	try {
		await fs.promises.mkdir(assetsCacheDir, { recursive: true });
	} catch (err) {
		logger.warn(
			null,
			`An error was encountered while creating the cache directory. Proceeding without caching. Error: ${err}`,
		);
		useCache = false;
	}

	let serverRoot: URL, clientRoot: URL;
	if (isServerLikeOutput(config)) {
		serverRoot = config.build.server;
		clientRoot = config.build.client;
	} else {

Review comment (performance):
Internally, we try to avoid TS features that generate code. In this case, a variant is better:

```ts

const CACHE_STATUS = "miss" | "revalidated" | "hit"
```

Code after:

type AssetEnv = {
	logger: Logger;
	isSSR: boolean;
	count: { total: number; current: number };
	useCache: boolean;
	assetsCacheDir: URL;
	serverRoot: URL;
	clientRoot: URL;
	imageConfig: AstroConfig['image'];
	assetsFolder: AstroConfig['build']['assets'];
};

type ImageData = {
	data: Uint8Array;
	expires: number;
	etag?: string;
	lastModified?: string;
};

export async function prepareAssetsGenerationEnv(
	pipeline: BuildPipeline,
	totalCount: number,
): Promise<AssetEnv> {
	const { config, logger, settings } = pipeline;
	let useCache = true;
	const assetsCacheDir = new URL('assets/', config.cacheDir);
	const count = { total: totalCount, current: 1 };

	// Ensure that the cache directory exists
	try {
		await fs.promises.mkdir(assetsCacheDir, { recursive: true });
	} catch (err) {
		logger.warn(
			null,
			`An error was encountered while creating the cache directory. Proceeding without caching. Error: ${err}`,
		);
		useCache = false;
	}

	const isServerOutput = settings.buildOutput === 'server';
	let serverRoot: URL, clientRoot: URL;
	if (isServerOutput) {
		serverRoot = config.build.server;
		clientRoot = config.build.client;
	} else {
		serverRoot = getOutDirWithinCwd(config.outDir);
		clientRoot = config.outDir;
	}

	return {
Repository: withastro/astro
File: packages/astro/src/assets/build/generate.ts (TypeScript)

Code before:
				`An error was encountered while creating the cache directory. Proceeding without caching. Error: ${e}`,
			);
		} finally {
			// Write the final file
			await fs.promises.writeFile(finalFileURL, resultData.data);
		}

		return {
			cached: CacheStatus.Miss,
			weight: {
				// Divide by 1024 to get size in kilobytes
				before: Math.trunc(originalImage.data.byteLength / 1024),
				after: Math.trunc(Buffer.from(resultData.data).byteLength / 1024),
			},
		};
	}
}

async function writeRemoteCacheFile(cachedFileURL: URL, resultData: ImageData) {
	return await fs.promises.writeFile(
		cachedFileURL,
		JSON.stringify({
			data: Buffer.from(resultData.data).toString('base64'),
			expires: resultData.expires,
			etag: resultData.etag,
		}),
	);
}

export function getStaticImageList(): AssetsGlobalStaticImagesList {
	if (!globalThis?.astroAsset?.staticImages) {
		return new Map();
	}

	return globalThis.astroAsset.staticImages;
}

async function loadImage(path: string, env: AssetEnv): Promise<ImageData> {
	if (isRemotePath(path)) {
		return await loadRemoteImage(path);
	}

	return {
		data: await fs.promises.readFile(getFullImagePath(path, env)),
		expires: 0,
	};
}


Review comment (bug):
`JSON.stringify` and `Buffer.from` can be both fail at runtime, we should handle the errors somehow 

Code after:
			// Write the cache entry
			if (env.useCache) {
				if (isLocalImage) {
					await fs.promises.writeFile(cachedFileURL, resultData.data);
				} else {
					await writeRemoteCacheFile(cachedFileURL, resultData as ImageData, env);
				}
			}
		} catch (e) {
			env.logger.warn(
				null,
				`An error was encountered while creating the cache directory. Proceeding without caching. Error: ${e}`,
			);
		} finally {
			// Write the final file
			await fs.promises.writeFile(finalFileURL, resultData.data);
		}

		return {
			cached: 'miss',
			weight: {
				// Divide by 1024 to get size in kilobytes
				before: Math.trunc(originalImage.data.byteLength / 1024),
				after: Math.trunc(Buffer.from(resultData.data).byteLength / 1024),
			},
		};
	}
}

async function writeRemoteCacheFile(cachedFileURL: URL, resultData: ImageData, env: AssetEnv) {
	try {
		return await fs.promises.writeFile(
			cachedFileURL,
			JSON.stringify({
				data: Buffer.from(resultData.data).toString('base64'),
				expires: resultData.expires,
				etag: resultData.etag,
				lastModified: resultData.lastModified,
			}),
		);
	} catch (e) {
		env.logger.warn(
			null,
			`An error was encountered while writing the cache file for a remote asset. Proceeding without caching this asset. Error: ${e}`,
		);
	}
}

export function getStaticImageList(): AssetsGlobalStaticImagesList {
	if (!globalThis?.astroAsset?.staticImages) {
		return new Map();
Repository: withastro/astro
File: packages/astro/test/custom-renderer.test.js (JavaScript)

Code before:
			$ = cheerio.load(html);
			assert.equal($('h1').text(), 'Client Directives');
		});

		it('renders SSR custom renderer functional components as expected', async () => {
			const res = await fixture.fetch('/');
			assert.equal(res.status, 200);

			const html = await res.text();
			$ = cheerio.load(html);

			assert.equal($('p').length, 5);
		});

		it('does not log a warning message when using a custom renderer', async () => {
			const res = await fixture.fetch('/');
			assert.equal(res.status, 200);

			assert.equal(
				warnLogsWritten.indexOf(
					'If you intended to use a different renderer, please provide a valid client:only directive.',
				),
				-1,
			);
		});
	});
});


Review comment (suggestion):
I think I'd prefer removing this test since it's only guarding if we ever revert this PR, which if we do we probably have a good reason for it. That way we also avoiding patching `console.warn` for this test.

The other test cases are fine to keep though, we're lacking tests for custom renderers.

Code after:
import assert from 'node:assert/strict';
import { after, before, describe, it } from 'node:test';
import * as cheerio from 'cheerio';
import { loadFixture } from './test-utils.js';

describe('Custom Renderer - SSR', () => {
	let fixture;

	before(async () => {
		fixture = await loadFixture({
			root: './fixtures/custom-renderer/',
		});
	});

	describe('dev', () => {
		let devServer;

		before(async () => {
			devServer = await fixture.startDevServer();
		});

		after(async () => {
			await devServer.stop();
		});

		it('renders /', async () => {
			const html = await fixture.fetch('/').then((res) => res.text());
			const $ = cheerio.load(html);
			assert.equal($('h1').text(), 'Client Directives');
		});

		it('renders SSR custom renderer functional components as expected', async () => {
			const res = await fixture.fetch('/');
			assert.equal(res.status, 200);

			const html = await res.text();
			const $ = cheerio.load(html);

			assert.equal($('p').length, 5);
		});
	});
});
Repository: withastro/astro
File: packages/astro/templates/env.mjs (JavaScript)

Code before:
// @ts-check
import { schema } from 'virtual:astro:env/internal';
import {
	createInvalidVariablesError,
	getEnv as _getEnv,
	getEnvFieldType,
	setOnSetGetEnv,
	validateEnvVariable,
} from 'astro/env/runtime';

const getEnv = (key) => {
	// @@GET_ENV@@
	return _getEnv(key);
}

export const getSecret = (key) => {
	return getEnv(key)
};

const _internalGetSecret = (key) => {
	const rawVariable = getEnv(key);
	const variable = rawVariable === '' ? undefined : rawVariable;
	const options = schema[key];

	const result = validateEnvVariable(variable, options);
	if (result.ok) {
		return result.value;
	}
	const type = getEnvFieldType(options);
	throw createInvalidVariablesError(key, type, result);
};

setOnSetGetEnv(() => {
	// @@ON_SET_GET_ENV@@
});


Review comment (suggestion):
This code will end up with two `return` statements, which isn't the best. Maybe we can leave the placeholder and add an `else` clause to the `if` we have `vite-plugin-env.ts`, line 157, which will replace the placeholder with `return _getEnv(key)`

Code after:
// @ts-check
import { schema } from 'virtual:astro:env/internal';
import {
	createInvalidVariablesError,
	getEnv as _getEnv,
	getEnvFieldType,
	setOnSetGetEnv,
	validateEnvVariable,
} from 'astro/env/runtime';

// @ts-expect-error
/** @returns {string} */
// used while generating the virtual module
// biome-ignore lint/correctness/noUnusedFunctionParameters: `key` is used by the generated code
// biome-ignore lint/correctness/noUnusedVariables: `key` is used by the generated code
const getEnv = (key) => {
	// @@GET_ENV@@
};

export const getSecret = (key) => {
	return getEnv(key)
};

const _internalGetSecret = (key) => {
	const rawVariable = getEnv(key);
	const variable = rawVariable === '' ? undefined : rawVariable;
	const options = schema[key];

	const result = validateEnvVariable(variable, options);
	if (result.ok) {
		return result.value;
	}
	const type = getEnvFieldType(options);
	throw createInvalidVariablesError(key, type, result);
};

setOnSetGetEnv(() => {
	// @@ON_SET_GET_ENV@@
Repository: withastro/astro
File: packages/astro/src/core/render-context.ts (TypeScript)

Code before:
			request: this.request,
			response,
			site: pipeline.site,
			getActionResult: createGetActionResult(locals),
			get callAction() {
				return createCallAction(this);
			},
			url,
			get originPathname() {
				return getOriginPathname(renderContext.request);
			},
		};
	}

	getClientAddress() {
		const { pipeline, request, routeData, clientAddress } = this;

		if(routeData.prerender) {
			throw new AstroError(AstroErrorData.PrerenderClientAddressNotAvailable);
		}

		if(clientAddress) {
			return clientAddress;
		}

		// Legacy, should not need to get here.
		// Some adapters set this symbol so we can't remove support yet.
		// Adapters should be updated to provide it via RenderOptions instead.
		if (clientAddressSymbol in request) {
			return Reflect.get(request, clientAddressSymbol) as string;
		}

		if (pipeline.adapterName) {
			throw new AstroError({
				...AstroErrorData.ClientAddressNotAvailable,
				message: AstroErrorData.ClientAddressNotAvailable.message(pipeline.adapterName),
			});
		}

		throw new AstroError(AstroErrorData.StaticClientAddressNotAvailable);
	}

	/**
	 * API Context may be created multiple times per request, i18n data needs to be computed only once.
	 * So, it is computed and saved here on creation of the first APIContext and reused for later ones.
	 */
	#currentLocale: APIContext['currentLocale'];

	computeCurrentLocale() {
		const {
			url,

Review comment (suggestion):
```suggestion
		// TODO: Legacy, should not need to get here.
```

Code after:
			request: this.request,
			response,
			site: pipeline.site,
			getActionResult: createGetActionResult(locals),
			get callAction() {
				return createCallAction(this);
			},
			url,
			get originPathname() {
				return getOriginPathname(renderContext.request);
			},
		};
	}

	getClientAddress() {
		const { pipeline, request, routeData, clientAddress } = this;

		if(routeData.prerender) {
			throw new AstroError(AstroErrorData.PrerenderClientAddressNotAvailable);
		}

		if(clientAddress) {
			return clientAddress;
		}

		// TODO: Legacy, should not need to get here.
		// Some adapters set this symbol so we can't remove support yet.
		// Adapters should be updated to provide it via RenderOptions instead.
		if (clientAddressSymbol in request) {
			return Reflect.get(request, clientAddressSymbol) as string;
		}

		if (pipeline.adapterName) {
			throw new AstroError({
				...AstroErrorData.ClientAddressNotAvailable,
				message: AstroErrorData.ClientAddressNotAvailable.message(pipeline.adapterName),
			});
		}

		throw new AstroError(AstroErrorData.StaticClientAddressNotAvailable);
	}

	/**
	 * API Context may be created multiple times per request, i18n data needs to be computed only once.
	 * So, it is computed and saved here on creation of the first APIContext and reused for later ones.
	 */
	#currentLocale: APIContext['currentLocale'];

	computeCurrentLocale() {
		const {
			url,
Repository: withastro/astro
File: packages/astro/src/assets/utils/svg.ts (TypeScript)

Code before:
import { parse, renderSync } from 'ultrahtml';
import type { SvgComponentProps } from '../runtime.js';
import { dropAttributes } from '../runtime.js';
import type { ImageMetadata } from '../types.js';

function parseSvg(contents: string) {
	const root = parse(contents);
	const svgNode = root.children.find(
		({ name, type }: { name: string; type: number }) => type === 1 /* Element */ && name === 'svg',
	);
	if (!svgNode) {
		throw new Error('SVG file does not contain an <svg> element');
	}
	const { attributes, children } = svgNode;
	const body = renderSync({ ...root, children });

	return { attributes, body };
}

export type SvgRenderMode = 'inline' | 'sprite';

export function makeSvgComponent(
	meta: ImageMetadata,
	contents: Buffer | string,
	options?: { mode: SvgRenderMode },
) {
	const file = typeof contents === 'string' ? contents : contents.toString('utf-8');
	const { attributes, body: children } = parseSvg(file);
	const props: SvgComponentProps = {
		meta,
		attributes: dropAttributes({ mode: options?.mode, ...attributes }),
		children,
	};

	return `import { createSvgComponent } from 'astro/assets/runtime';
export default createSvgComponent(${JSON.stringify(props)})`;
}


Review comment (bug):
Having an optional `mode` should be a bug. Users should either provide a boolean, or provide an object with explicit `mode`

Code after:
import { parse, renderSync } from 'ultrahtml';
import type { SvgComponentProps } from '../runtime.js';
import { dropAttributes } from '../runtime.js';
import type { ImageMetadata } from '../types.js';

function parseSvg(contents: string) {
	const root = parse(contents);
	const svgNode = root.children.find(
		({ name, type }: { name: string; type: number }) => type === 1 /* Element */ && name === 'svg',
	);
	if (!svgNode) {
		throw new Error('SVG file does not contain an <svg> element');
	}
	const { attributes, children } = svgNode;
	const body = renderSync({ ...root, children });

	return { attributes, body };
}

export type SvgRenderMode = 'inline' | 'sprite';

export function makeSvgComponent(
	meta: ImageMetadata,
	contents: Buffer | string,
	options?: { mode?: SvgRenderMode },
) {
	const file = typeof contents === 'string' ? contents : contents.toString('utf-8');
	const { attributes, body: children } = parseSvg(file);
	const props: SvgComponentProps = {
		meta,
		attributes: dropAttributes({ mode: options?.mode, ...attributes }),
		children,
	};

	return `import { createSvgComponent } from 'astro/assets/runtime';
export default createSvgComponent(${JSON.stringify(props)})`;
}
Repository: withastro/astro
File: packages/astro/src/cli/info/index.ts (TypeScript)

Code before:
	} else if (system === 'win32') {
		command = 'clip';
	} else {
		// Unix: check if a supported command is installed

		const unixCommands: Array<[string, Array<string>]> = [
			['xclip', ['-sel', 'clipboard', '-l', '1']],
			['wl-copy', []],
		];
		for (const [unixCommand, unixArgs] of unixCommands) {
			try {
				const output = spawnSync('which', [unixCommand], { encoding: 'utf8' });
				if (output.stdout.trim()) {
					command = unixCommand;
					args = unixArgs;
					break;
				}
			} catch {
				continue;
			}
		}
	}

	if (!command) {
		console.error(colors.red('\nClipboard command not found!'));
		console.log('Please manually copy the text above.');
		return;
	}

	if (!force) {
		const { shouldCopy } = await prompts({
			type: 'confirm',
			name: 'shouldCopy',
			message: 'Copy to clipboard?',
			initial: true,
		});

		if (!shouldCopy) return;
	}

	try {
		const result = spawnSync(command, args, { input: text });
		if (result.error) {
			throw result.error;
		}
		console.log(colors.green('Copied to clipboard!'));
	} catch (err) {
		console.error(
			colors.red(`\nSorry, something went wrong!`) + ` Please copy the text above manually.`,
		);
	}

Review comment (bug):
Use `.info` instead. Usually `.log` is more for debugging 

Code after:
	} else if (system === 'win32') {
		command = 'clip';
	} else {
		// Unix: check if a supported command is installed

		const unixCommands: Array<[string, Array<string>]> = [
			['xclip', ['-sel', 'clipboard', '-l', '1']],
			['wl-copy', []],
		];
		for (const [unixCommand, unixArgs] of unixCommands) {
			try {
				const output = spawnSync('which', [unixCommand], { encoding: 'utf8' });
				if (output.stdout.trim()) {
					command = unixCommand;
					args = unixArgs;
					break;
				}
			} catch {
				continue;
			}
		}
	}

	if (!command) {
		console.error(colors.red('\nClipboard command not found!'));
		console.info('Please manually copy the text above.');
		return;
	}

	if (!force) {
		const { shouldCopy } = await prompts({
			type: 'confirm',
			name: 'shouldCopy',
			message: 'Copy to clipboard?',
			initial: true,
		});

		if (!shouldCopy) return;
	}

	try {
		const result = spawnSync(command, args, { input: text });
		if (result.error) {
			throw result.error;
		}
		console.info(colors.green('Copied to clipboard!'));
	} catch {
		console.error(
			colors.red(`\nSorry, something went wrong!`) + ` Please copy the text above manually.`,
		);
	}