ronantakizawa/codereview-bench huggingface dataset:
Repository: withastro/astro
File: packages/markdown/remark/src/frontmatter.ts (TypeScript)

Code before:
import yaml from 'js-yaml';

export function isFrontmatterValid(frontmatter: Record<string, any>) {
	try {
		// ensure frontmatter is JSON-serializable
		JSON.stringify(frontmatter);
	} catch {
		return false;
	}
	return typeof frontmatter === 'object' && frontmatter !== null;
}

const frontmatterRE = /^\s*---([\s\S]*?\n)---/;
export function extractFrontmatter(code: string): string | undefined {
	return frontmatterRE.exec(code)?.[1];
}

export interface ParseFrontmatterOptions {
	/**
	 * How the frontmatter should be handled in the returned `content` string.
	 * - `preserve`: Keep the frontmatter.
	 * - `remove`: Remove the frontmatter.
	 * - `empty-with-spaces`: Replace the frontmatter with empty spaces. (preserves sourcemap line/col/offset)
	 * - `empty-with-lines`: Replace the frontmatter with empty line breaks. (preserves sourcemap line/col)
	 *
	 * @default 'remove'
	 */
	frontmatter: 'preserve' | 'remove' | 'empty-with-spaces' | 'empty-with-lines';
}

export interface ParseFrontmatterResult {
	frontmatter: Record<string, any>;
	rawFrontmatter: string;
	content: string;
}

export function parseFrontmatter(
	code: string,
	options?: ParseFrontmatterOptions,
): ParseFrontmatterResult {
	const rawFrontmatter = extractFrontmatter(code);

Review comment (suggestion):
```suggestion
// Capture frontmatter wrapped with `---`, including any characters and new lines within it.
// Only capture if it exists near the top of the file (whitespaces between the start of file and
// the start of `---` are allowed)
const frontmatterRE = /^\s*---([\s\S]*?\n)---/;
```

Code after:
import yaml from 'js-yaml';

export function isFrontmatterValid(frontmatter: Record<string, any>) {
	try {
		// ensure frontmatter is JSON-serializable
		JSON.stringify(frontmatter);
	} catch {
		return false;
	}
	return typeof frontmatter === 'object' && frontmatter !== null;
}

// Capture frontmatter wrapped with `---`, including any characters and new lines within it.
// Only capture if it exists near the top of the file (whitespaces between the start of file and
// the start of `---` are allowed)
const frontmatterRE = /^\s*---([\s\S]*?\n)---/;
export function extractFrontmatter(code: string): string | undefined {
	return frontmatterRE.exec(code)?.[1];
}

export interface ParseFrontmatterOptions {
	/**
	 * How the frontmatter should be handled in the returned `content` string.
	 * - `preserve`: Keep the frontmatter.
	 * - `remove`: Remove the frontmatter.
	 * - `empty-with-spaces`: Replace the frontmatter with empty spaces. (preserves sourcemap line/col/offset)
	 * - `empty-with-lines`: Replace the frontmatter with empty line breaks. (preserves sourcemap line/col)
	 *
	 * @default 'remove'
	 */
	frontmatter: 'preserve' | 'remove' | 'empty-with-spaces' | 'empty-with-lines';
}

export interface ParseFrontmatterResult {
	frontmatter: Record<string, any>;
	rawFrontmatter: string;
	content: string;
}

export function parseFrontmatter(
	code: string,
Repository: withastro/astro
File: packages/astro/test/units/vite-plugin-astro-server/response.test.js (JavaScript)

Code before:
			'access-control-allow-origin': '*',
			'x-single': 'single',
			'x-triple': 'one, two, three',
			'set-cookie': ['hello', 'world'],
		});
	});

	it('Can bail on streaming', async () => {
		const { req, res, done } = createRequestAndResponse({
			method: 'GET',
			url: '/streaming',
		});

		container.handle(req, res);

		await new Promise((resolve) => setTimeout(resolve, 500));
		res.emit('close');

		try {
			await done;

			assert.ok(true);
			
		} catch {
			assert.fail()
		}
	});
});


Review comment (bug):
Wouldn't the assert.fail hide the errors from the catch if it happened to help debug this in the future?

Code after:
			'access-control-allow-origin': '*',
			'x-single': 'single',
			'x-triple': 'one, two, three',
			'set-cookie': ['hello', 'world'],
		});
	});

	it('Can bail on streaming', async () => {
		const { req, res, done } = createRequestAndResponse({
			method: 'GET',
			url: '/streaming',
		});

		container.handle(req, res);

		await new Promise((resolve) => setTimeout(resolve, 500));
		res.emit('close');

		try {
			await done;

			assert.ok(true);
			
		} catch (err) {
			assert.fail(err)
		}
	});
});
Repository: withastro/astro
File: packages/astro/src/assets/utils/node/emitAsset.ts (TypeScript)

Code before:
	}

	const url = pathToFileURL(id);
	let fileData: Buffer;
	try {
		fileData = await fs.readFile(url);
	} catch {
		return undefined;
	}

	const fileMetadata = await imageMetadata(fileData, id);

	const emittedImage: Omit<ImageMetadataWithContents, 'fsPath'> = {
		src: '',
		...fileMetadata,
	};

	// Private for now, we generally don't want users to rely on filesystem paths, but we need it so that we can maybe remove the original asset from the build if it's unused.
	Object.defineProperty(emittedImage, 'fsPath', {
		enumerable: false,
		writable: false,
		value: id,
	});

	// Attach file data for SVGs
	// TODO: this is a workaround to prevent the a memory leak, and it must be fixed before we remove the experimental flag
	if (fileMetadata.format === 'svg' && experimentalSvgEnabled === true) {
		emittedImage.contents = fileData;
	}

	// Build
	let isBuild = typeof fileEmitter === 'function';
	if (isBuild) {
		const pathname = decodeURI(url.pathname);
		const filename = path.basename(pathname, path.extname(pathname) + `.${fileMetadata.format}`);

		try {
			// fileEmitter throws in dev
			const handle = fileEmitter!({
				name: filename,
				source: await fs.readFile(url),
				type: 'asset',
			});

			emittedImage.src = `__ASTRO_ASSET_IMAGE__${handle}__`;
		} catch {
			isBuild = false;
		}
	}

	if (!isBuild) {

Review comment (performance):
```suggestion
	// TODO: this is a workaround to prevent a memory leak, and it must be fixed before we remove the experimental flag, see
```

Code after:
	}

	const url = pathToFileURL(id);
	let fileData: Buffer;
	try {
		fileData = await fs.readFile(url);
	} catch {
		return undefined;
	}

	const fileMetadata = await imageMetadata(fileData, id);

	const emittedImage: Omit<ImageMetadataWithContents, 'fsPath'> = {
		src: '',
		...fileMetadata,
	};

	// Private for now, we generally don't want users to rely on filesystem paths, but we need it so that we can maybe remove the original asset from the build if it's unused.
	Object.defineProperty(emittedImage, 'fsPath', {
		enumerable: false,
		writable: false,
		value: id,
	});

	// Attach file data for SVGs
	// TODO: this is a workaround to prevent a memory leak, and it must be fixed before we remove the experimental flag, see
	if (fileMetadata.format === 'svg' && experimentalSvgEnabled === true) {
		emittedImage.contents = fileData;
	}

	// Build
	let isBuild = typeof fileEmitter === 'function';
	if (isBuild) {
		const pathname = decodeURI(url.pathname);
		const filename = path.basename(pathname, path.extname(pathname) + `.${fileMetadata.format}`);

		try {
			// fileEmitter throws in dev
			const handle = fileEmitter!({
				name: filename,
				source: await fs.readFile(url),
				type: 'asset',
			});

			emittedImage.src = `__ASTRO_ASSET_IMAGE__${handle}__`;
		} catch {
			isBuild = false;
		}
	}

	if (!isBuild) {
Repository: withastro/astro
File: packages/astro/test/i18n-routing.test.js (JavaScript)

Code before:
					prefixDefaultLocale: false,
					fallbackType: 'rewrite',
				},
				fallback: {
					fr: 'en',
				},
			},
		});
		await fixture.build();
		app = await fixture.loadTestAdapterApp();
	});

	it('should correctly rewrite to en', async () => {
		const request = new Request('http://example.com/fr');
		const response = await app.render(request);
		assert.equal(response.status, 200);
		const html = await response.text();
		assert.match(html, /Hello/);
	});

	it('should render paths with path parameters correctly', async () => {
		let request = new Request('http://example.com/new-site/fr/blog/1');
		let response = await app.render(request);
		assert.equal(response.status, 200);
		const text = await response.text();
		assert.equal(text.includes('Hello world'), true);
	});
});


Review comment (suggestion):
Use `assert.match`

```suggestion
		assert.match(text, /Hello world/);
```

Code after:
		// app = await fixture.loadTestAdapterApp();
	});

	it('should correctly rewrite to en', async () => {
		const html = await fixture.readFile('/fr/index.html');
		assert.match(html, /Hello/);
		// assert.fail()
	});

	it('should render fallback locale paths with path parameters correctly (fr)', async () => {
		const html = await fixture.readFile('/fr/blog/1/index.html');
		assert.match(html, /Hello world/);
	});

	it('should render fallback locale paths with path parameters correctly (es)', async () => {
		const html = await fixture.readFile('/es/blog/1/index.html');
		assert.match(html, /Hola mundo/);
	});

	it('should render fallback locale paths with query parameters correctly (it)', async () => {
		const html = await fixture.readFile('/it/blog/1/index.html');
		assert.match(html, /Hello world/);
	});
});

describe('Fallback rewrite SSR', () => {
	/** @type {import('./test-utils').Fixture} */
	let fixture;
	let app;

	before(async () => {
		fixture = await loadFixture({
			root: './fixtures/i18n-routing-fallback/',
			output: 'server',
			outDir: './dist/i18n-routing-fallback',
			adapter: testAdapter(),
			i18n: {
				defaultLocale: 'en',
				locales: ['en', 'fr', 'es', 'it', 'pt'],
				routing: {
					prefixDefaultLocale: false,
					fallbackType: 'rewrite',
				},
				fallback: {
					fr: 'en',
					it: 'en',
					es: 'pt',
				},
			},
		});
		await fixture.build();
Repository: withastro/astro
File: packages/astro/src/actions/runtime/virtual/shared.ts (TypeScript)

Code before:
	handler: () => MaybePromise<TOutput>,
): Promise<SafeResult<z.ZodType, TOutput>> {
	try {
		const data = await handler();
		return { data, error: undefined };
	} catch (e) {
		if (e instanceof ActionError) {
			return { data: undefined, error: e };
		}
		return {
			data: undefined,
			error: new ActionError({
				message: e instanceof Error ? e.message : 'Unknown error',
				code: 'INTERNAL_SERVER_ERROR',
			}),
		};
	}
}

export function getActionQueryString(name: string) {
	const searchParams = new URLSearchParams({ [_ACTION_QUERY_PARAMS.actionName]: name });
	return `?${searchParams.toString()}`;
}

export function getActionPath(action: ActionClient<any, any, any>) {
	return `${import.meta.env.BASE_URL.replace(/\/$/, '')}/_actions/${new URLSearchParams(action.toString()).get(ACTION_QUERY_PARAMS.actionName)}`;
}

export type SerializedActionResult =
	| {
			type: 'data';
			contentType: 'application/json+devalue';
			status: 200;
			body: string;
	  }
	| {
			type: 'error';
			contentType: 'application/json';
			status: number;
			body: string;
	  }
	| {
			type: 'empty';
			status: 204;
	  };

export function serializeActionResult(res: SafeResult<any, any>): SerializedActionResult {
	if (res.error) {
		if (import.meta.env?.DEV) {
			actionResultErrorStack.set(res.error.stack);
		}

Review comment (bug):
We need to handle `trailingSlash` too. I just [merged](https://github.com/withastro/astro/pull/12657) a bug where now action URLs have one. 

Code after:

export async function callSafely<TOutput>(
	handler: () => MaybePromise<TOutput>,
): Promise<SafeResult<z.ZodType, TOutput>> {
	try {
		const data = await handler();
		return { data, error: undefined };
	} catch (e) {
		if (e instanceof ActionError) {
			return { data: undefined, error: e };
		}
		return {
			data: undefined,
			error: new ActionError({
				message: e instanceof Error ? e.message : 'Unknown error',
				code: 'INTERNAL_SERVER_ERROR',
			}),
		};
	}
}

export function getActionQueryString(name: string) {
	const searchParams = new URLSearchParams({ [_ACTION_QUERY_PARAMS.actionName]: name });
	return `?${searchParams.toString()}`;
}

export type SerializedActionResult =
	| {
			type: 'data';
			contentType: 'application/json+devalue';
			status: 200;
			body: string;
	  }
	| {
			type: 'error';
			contentType: 'application/json';
			status: number;
			body: string;
	  }
	| {
			type: 'empty';
			status: 204;
	  };

export function serializeActionResult(res: SafeResult<any, any>): SerializedActionResult {
	if (res.error) {
		if (import.meta.env?.DEV) {
			actionResultErrorStack.set(res.error.stack);
		}

		let body: Record<string, any>;
Repository: withastro/astro
File: packages/astro/src/content/content-layer.ts (TypeScript)

Code before:
import { promises as fs, existsSync } from 'node:fs';
import { deterministicString } from 'deterministic-object-hash';
import PQueue from 'p-queue';
import type { FSWatcher } from 'vite';
import xxhash from 'xxhash-wasm';
import { AstroError, AstroErrorData } from '../core/errors/index.js';
import type { Logger } from '../core/logger/core.js';
import type { AstroSettings } from '../types/astro.js';

import type { ContentEntryType, RefreshContentOptions } from '../types/public/content.js';
import {
	ASSET_IMPORTS_FILE,
	COLLECTIONS_MANIFEST_FILE,
	CONTENT_LAYER_TYPE,
	DATA_STORE_FILE,
	MODULES_IMPORTS_FILE,
} from './consts.js';
import type { LoaderContext } from './loaders/types.js';
import type { MutableDataStore } from './mutable-data-store.js';
import {
	getEntryConfigByExtMap,
	getEntryDataAndImages,
	globalContentConfigObserver,
} from './utils.js';

export interface ContentLayerOptions {
	store: MutableDataStore;
	settings: AstroSettings;
	logger: Logger;
	watcher?: FSWatcher;
}

export class ContentLayer {
	#logger: Logger;

Review comment (suggestion):
Micro-nit, but I’m guessing this was introduced accidentally?
```suggestion
```

Code after:
import { promises as fs, existsSync } from 'node:fs';
import PQueue from 'p-queue';
import type { FSWatcher } from 'vite';
import xxhash from 'xxhash-wasm';
import { AstroError, AstroErrorData } from '../core/errors/index.js';
import type { Logger } from '../core/logger/core.js';
import type { AstroSettings } from '../types/astro.js';
import type { ContentEntryType, RefreshContentOptions } from '../types/public/content.js';
import {
	ASSET_IMPORTS_FILE,
	COLLECTIONS_MANIFEST_FILE,
	CONTENT_LAYER_TYPE,
	DATA_STORE_FILE,
	MODULES_IMPORTS_FILE,
} from './consts.js';
import type { LoaderContext } from './loaders/types.js';
import type { MutableDataStore } from './mutable-data-store.js';
import {
	getEntryConfigByExtMap,
	getEntryDataAndImages,
	globalContentConfigObserver,
	safeStringify,
} from './utils.js';

export interface ContentLayerOptions {
	store: MutableDataStore;
	settings: AstroSettings;
	logger: Logger;
	watcher?: FSWatcher;
}

export class ContentLayer {
	#logger: Logger;
	#store: MutableDataStore;
Repository: withastro/astro
File: packages/astro/src/core/session.ts (TypeScript)

Code before:
	// preserving in-memory changes and deletions.
	#partial = true;

	constructor(
		cookies: AstroCookies,
		{
			cookie: cookieConfig = DEFAULT_COOKIE_NAME,
			...config
		}: Exclude<ResolvedSessionConfig<TDriver>, undefined>,
	) {
		this.#cookies = cookies;
		if (typeof cookieConfig === 'object') {
			this.#cookieConfig = cookieConfig;
			this.#cookieName = cookieConfig.name || DEFAULT_COOKIE_NAME;
		} else {
			this.#cookieName = cookieConfig || DEFAULT_COOKIE_NAME;
		}
		this.#config = config;
	}

	/**
	 * Gets a session value. Returns `undefined` if the session or value does not exist.
	 */
	async get<T = any>(key: string): Promise<T | undefined> {
		return (await this.#ensureData()).get(key)?.data;
	}

	/**
	 * Checks if a session value exists.
	 */
	async has(key: string): Promise<boolean> {
		return (await this.#ensureData()).has(key);
	}

	/**
	 * Gets all session values.
	 */
	async keys() {
		return (await this.#ensureData()).keys();
	}

	/**
	 * Gets all session values.
	 */
	async values() {
		return [...(await this.#ensureData()).values()].map((entry) => entry.data);
	}

	/**
	 * Gets all session entries.
	 */

Review comment (suggestion):
Can't we move these lines inside the `if` block at line 70? It seems we don't need a temporary `cookieConfigObject` object.

Code after:
	// preserving in-memory changes and deletions.
	#partial = true;

	constructor(
		cookies: AstroCookies,
		{
			cookie: cookieConfig = DEFAULT_COOKIE_NAME,
			...config
		}: Exclude<ResolvedSessionConfig<TDriver>, undefined>,
	) {
		this.#cookies = cookies;
		let cookieConfigObject: AstroCookieSetOptions | undefined;
		if (typeof cookieConfig === 'object') {
			const { name = DEFAULT_COOKIE_NAME, ...rest } = cookieConfig;
			this.#cookieName = name;
			cookieConfigObject = rest;
		} else {
			this.#cookieName = cookieConfig || DEFAULT_COOKIE_NAME;
		}
		this.#cookieConfig = {
			sameSite: 'lax',
			secure: true,
			path: '/',
			...cookieConfigObject,
			httpOnly: true,
		};
		this.#config = config;
	}

	/**
	 * Gets a session value. Returns `undefined` if the session or value does not exist.
	 */
	async get<T = any>(key: string): Promise<T | undefined> {
		return (await this.#ensureData()).get(key)?.data;
	}

	/**
	 * Checks if a session value exists.
	 */
	async has(key: string): Promise<boolean> {
		return (await this.#ensureData()).has(key);
	}

	/**
	 * Gets all session values.
	 */
	async keys() {
		return (await this.#ensureData()).keys();
	}

	/**
Repository: withastro/astro
File: packages/astro/test/sessions.test.js (JavaScript)

Code before:
import assert from 'node:assert/strict';
import { before, describe, it } from 'node:test';
import testAdapter from './test-adapter.js';
import { loadFixture } from './test-utils.js';

describe('Astro.cookies', () => {
	/** @type {import('./test-utils').Fixture} */
	let fixture;

	before(async () => {
		fixture = await loadFixture({
			root: './fixtures/sessions/',
			output: 'server',
			adapter: testAdapter(),
		});
	});

	describe('Production', () => {
		let app;
		before(async () => {
			await fixture.build();
			app = await fixture.loadTestAdapterApp();
		});

		async function fetchResponse(path, requestInit) {
			const request = new Request('http://example.com' + path, requestInit);
			const response = await app.render(request);
			return response;
		}

		it('can regenerate session cookies upon request', async () => {

Review comment (suggestion):
```suggestion
describe('Astro.session', () => {
```

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

describe('Astro.session', () => {
	/** @type {import('./test-utils').Fixture} */
	let fixture;

	before(async () => {
		fixture = await loadFixture({
			root: './fixtures/sessions/',
			output: 'server',
			adapter: testAdapter(),
		});
	});

	describe('Production', () => {
		let app;
		before(async () => {
			await fixture.build();
			app = await fixture.loadTestAdapterApp();
		});

		async function fetchResponse(path, requestInit) {
			const request = new Request('http://example.com' + path, requestInit);
			const response = await app.render(request);
			return response;
		}

		it('can regenerate session cookies upon request', async () => {
Repository: withastro/astro
File: packages/astro/test/sessions.test.js (JavaScript)

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

describe('Astro.session', () => {
	/** @type {import('./test-utils').Fixture} */
	let fixture;

	before(async () => {
		// seems to be a race condition in unstorage when the session directory doesn't exist
		await fs.promises.mkdir('.astro/session', { recursive: true });
		fixture = await loadFixture({
			root: './fixtures/sessions/',
			output: 'server',
			adapter: testAdapter(),
		});
	});

	describe('Production', () => {
		let app;
		before(async () => {
			await fixture.build();
			app = await fixture.loadTestAdapterApp();
		});

		async function fetchResponse(path, requestInit) {
			const request = new Request('http://example.com' + path, requestInit);
			const response = await app.render(request);
			return response;
		}

		it('can regenerate session cookies upon request', async () => {
			const firstResponse = await fetchResponse('/regenerate', { method: 'GET' });
			const firstHeaders = Array.from(app.setCookieHeaders(firstResponse));
			const firstSessionId = firstHeaders[0].split(';')[0].split('=')[1];


Review comment (suggestion):
We should probably be creating this in production too in that case. It could go here: 

https://github.com/kaytwo/astro/blob/6e7afe876fa8ee07457dd60e145de70aa3f9f5c3/packages/astro/src/core/session.ts#L396

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

describe('Astro.session', () => {
	/** @type {import('./test-utils').Fixture} */
	let fixture;

	before(async () => {
		fixture = await loadFixture({
			root: './fixtures/sessions/',
			output: 'server',
			adapter: testAdapter(),
		});
	});

	describe('Production', () => {
		let app;
		before(async () => {
			await fixture.build();
			app = await fixture.loadTestAdapterApp();
		});

		async function fetchResponse(path, requestInit) {
			const request = new Request('http://example.com' + path, requestInit);
			const response = await app.render(request);
			return response;
		}

		it('can regenerate session cookies upon request', async () => {
			const firstResponse = await fetchResponse('/regenerate', { method: 'GET' });
			const firstHeaders = Array.from(app.setCookieHeaders(firstResponse));
			const firstSessionId = firstHeaders[0].split(';')[0].split('=')[1];

			const secondResponse = await fetchResponse('/regenerate', {
				method: 'GET',
				headers: {