ronantakizawa/codereview-bench huggingface dataset:
Repository: withastro/astro
File: packages/astro/src/core/session.ts (TypeScript)

Code before:
				);
			}
			throw err;
		}

		if (!driver) {
			throw new AstroError({
				...SessionStorageInitError,
				message: SessionStorageInitError.message(
					'The module did not export a driver.',
					this.#config.driver,
				),
			});
		}

		try {
			this.#storage = createStorage({
				driver: driver(this.#config.options),
			});
			if (this.#config.driver === 'fs-lite') {
				// seems to be a race condition in unstorage when the session directory doesn't exist
				// this ensures that the directory is created before we try to use it for real sessions
				const dummyId = crypto.randomUUID();
				await this.#storage.setItem(dummyId, 'init');
				await this.#storage.removeItem(dummyId);
			}
			return this.#storage;
		} catch (err) {
			throw new AstroError(
				{
					...SessionStorageInitError,
					message: SessionStorageInitError.message('Unknown error', this.#config.driver),
				},
				{ cause: err },
			);
		}
	}
}
// TODO: make this sync when we drop support for Node < 18.19.0
export function resolveSessionDriver(driver: string | undefined): Promise<string> | string | null {
	if (!driver) {
		return null;
	}
	if (driver === 'fs') {
		return import.meta.resolve(builtinDrivers.fsLite);
	}
	if (driver in builtinDrivers) {
		return import.meta.resolve(builtinDrivers[driver as keyof typeof builtinDrivers]);
	}
	return driver;
}

Review comment (suggestion):
OK, I think I understand this now. So is the problem that if you try to read a session when the directory hasn't been created then it has an error? I'm not happy with this workaround because we're otherwise not using the storage unless the user actually reads or writes a session. This is soemthign that should be fixed upstream: unstorage should just return undefined for gets if the directory doesn't exist, the same as if the file doesn't exist. 

Have you encountered this issue outside of tests? If not, I think the best bet is to add a workaround inside tests, and report the issue on the unstorage repo.

Code after:
				);
			}
			throw err;
		}

		if (!driver) {
			throw new AstroError({
				...SessionStorageInitError,
				message: SessionStorageInitError.message(
					'The module did not export a driver.',
					this.#config.driver,
				),
			});
		}

		try {
			this.#storage = createStorage({
				driver: driver(this.#config.options),
			});
			return this.#storage;
		} catch (err) {
			throw new AstroError(
				{
					...SessionStorageInitError,
					message: SessionStorageInitError.message('Unknown error', this.#config.driver),
				},
				{ cause: err },
			);
		}
	}
}
// TODO: make this sync when we drop support for Node < 18.19.0
export function resolveSessionDriver(driver: string | undefined): Promise<string> | string | null {
	if (!driver) {
		return null;
	}
	if (driver === 'fs') {
		return import.meta.resolve(builtinDrivers.fsLite);
	}
	if (driver in builtinDrivers) {
		return import.meta.resolve(builtinDrivers[driver as keyof typeof builtinDrivers]);
	}
	return driver;
}
Repository: withastro/astro
File: packages/astro/src/core/request.ts (TypeScript)

Code before:
import type { IncomingHttpHeaders } from 'node:http';
import { AstroError, AstroErrorData } from './errors/index.js';
import type { Logger } from './logger/core.js';

type HeaderType = Headers | Record<string, any> | IncomingHttpHeaders;
type RequestBody =
	| ArrayBuffer
	| Blob
	| ReadableStream
	| URLSearchParams
	| FormData
	| ReadableStream<Uint8Array>;

export interface CreateRequestOptions {
	url: URL | string;
	clientAddress?: string | undefined;
	headers: HeaderType;
	method?: string;
	body?: RequestBody | undefined | null;
	logger: Logger;
	locals?: object | undefined;
	/**
	 * Whether the request is being created for a static build or for a prerendered page within a hybrid/SSR build, or for emulating one of those in dev mode.
	 *
	 * When `true`, the request will not include search parameters or body, and warn when headers are accessed.
	 *
	 * @default false
	 */
	isPrerendered?: boolean;

	routePattern: string;

	init?: RequestInit;
}

/**
 * Used by astro internals to create a web standard request object.
 *
 * The user of this function may provide the data in a runtime-agnostic way.
 *
 * This is used by the static build to create fake requests for prerendering, and by the dev server to convert node requests into the standard request object.
 */
export function createRequest({

Review comment (suggestion):
I added `null` because `Request.body` is `ReadableStream<Uint8Array> | null`

Code after:
import type { IncomingHttpHeaders } from 'node:http';
import type { Logger } from './logger/core.js';

type HeaderType = Headers | Record<string, any> | IncomingHttpHeaders;
type RequestBody =
	| ArrayBuffer
	| Blob
	| ReadableStream
	| URLSearchParams
	| FormData
	| ReadableStream<Uint8Array>;

export interface CreateRequestOptions {
	url: URL | string;
	clientAddress?: string | undefined;
	headers: HeaderType;
	method?: string;
	body?: RequestBody | undefined | null;
	logger: Logger;
	locals?: object | undefined;
	/**
	 * Whether the request is being created for a static build or for a prerendered page within a hybrid/SSR build, or for emulating one of those in dev mode.
	 *
	 * When `true`, the request will not include search parameters or body, and warn when headers are accessed.
	 *
	 * @default false
	 */
	isPrerendered?: boolean;

	routePattern: string;

	init?: RequestInit;
}

/**
 * Used by astro internals to create a web standard request object.
 *
 * The user of this function may provide the data in a runtime-agnostic way.
 *
 * This is used by the static build to create fake requests for prerendering, and by the dev server to convert node requests into the standard request object.
 */
export function createRequest({
	url,
Repository: withastro/astro
File: packages/internal-helpers/src/path.ts (TypeScript)

Code before:
}

export function isRemotePath(src: string) {
	return /^(?:http|ftp|https|ws):?\/\//.test(src) || src.startsWith('data:');
}

export function slash(path: string) {
	return path.replace(/\\/g, '/');
}

export function fileExtension(path: string) {
	const ext = path.split('.').pop();
	return ext !== path ? `.${ext}` : '';
}

export function removeBase(path: string, base: string) {
	if (path.startsWith(base)) {
		return path.slice(removeTrailingForwardSlash(base).length);
	}
	return path;
}

const withFileExt = /\/[^/]+\.\w+$/;

export function hasFileExtension(path: string) {
	return withFileExt.test(path);
}


Review comment (suggestion):
```suggestion
const WITH_FILE_EXT = /\/[^/]+\.\w+$/;

export function hasFileExtension(path: string) {
	return WITH_FILE_EXT.test(path);
```

Code after:

export function removeFileExtension(path: string) {
	let idx = path.lastIndexOf('.');
	return idx === -1 ? path : path.slice(0, idx);
}

export function removeQueryString(path: string) {
	const index = path.lastIndexOf('?');
	return index > 0 ? path.substring(0, index) : path;
}

export function isRemotePath(src: string) {
	return /^(?:http|ftp|https|ws):?\/\//.test(src) || src.startsWith('data:');
}

export function slash(path: string) {
	return path.replace(/\\/g, '/');
}

export function fileExtension(path: string) {
	const ext = path.split('.').pop();
	return ext !== path ? `.${ext}` : '';
}

export function removeBase(path: string, base: string) {
	if (path.startsWith(base)) {
		return path.slice(removeTrailingForwardSlash(base).length);
	}
	return path;
}

const WITH_FILE_EXT = /\/[^/]+\.\w+$/;

export function hasFileExtension(path: string) {
	return WITH_FILE_EXT.test(path);
}
Repository: withastro/astro
File: packages/integrations/react/src/index.ts (TypeScript)

Code before:
import react, { type Options as ViteReactPluginOptions } from '@vitejs/plugin-react';
import type { AstroIntegration, ContainerRenderer } from 'astro';
import type * as vite from 'vite';
import {
	type ReactVersionConfig,
	type SupportedReactVersion,
	getReactMajorVersion,
	isUnsupportedVersion,
	versionsConfig,
} from './version.js';

export type ReactIntegrationOptions = Pick<
	ViteReactPluginOptions,
	'include' | 'exclude' | 'babel'
> & {
	experimentalReactChildren?: boolean;
	/**
	 * Disable streaming support for React components in order to support libraries that are not compatible with streaming like Stitches.js
	 */
	experimentalDisableStreaming?: boolean;
};

const FAST_REFRESH_PREAMBLE = react.preambleCode;

function getRenderer(reactConfig: ReactVersionConfig) {
	return {
		name: '@astrojs/react',
		clientEntrypoint: reactConfig.client,
		serverEntrypoint: reactConfig.server,
	};
}

function optionsPlugin(
	experimentalReactChildren: boolean,
	experimentalDisableStreaming: boolean
): vite.Plugin {
	const virtualModule = 'astro:react:opts';
	const virtualModuleId = '\0' + virtualModule;
	return {
		name: '@astrojs/react:opts',
		resolveId(id) {
			if (id === virtualModule) {
				return virtualModuleId;

Review comment (suggestion):
```suggestion
	 * Disable streaming in React components
```

Code after:
import react, { type Options as ViteReactPluginOptions } from '@vitejs/plugin-react';
import type { AstroIntegration, ContainerRenderer } from 'astro';
import type * as vite from 'vite';
import {
	type ReactVersionConfig,
	type SupportedReactVersion,
	getReactMajorVersion,
	isUnsupportedVersion,
	versionsConfig,
} from './version.js';

export type ReactIntegrationOptions = Pick<
	ViteReactPluginOptions,
	'include' | 'exclude' | 'babel'
> & {
	experimentalReactChildren?: boolean;
	/**
	 * Disable streaming in React components
	 */
	experimentalDisableStreaming?: boolean;
};

const FAST_REFRESH_PREAMBLE = react.preambleCode;

function getRenderer(reactConfig: ReactVersionConfig) {
	return {
		name: '@astrojs/react',
		clientEntrypoint: reactConfig.client,
		serverEntrypoint: reactConfig.server,
	};
}

function optionsPlugin({
	experimentalReactChildren = false,
	experimentalDisableStreaming = false
}: {
	experimentalReactChildren: boolean;
	experimentalDisableStreaming: boolean;
}): vite.Plugin {
	const virtualModule = 'astro:react:opts';
	const virtualModuleId = '\0' + virtualModule;
	return {
		name: '@astrojs/react:opts',
Repository: withastro/astro
File: packages/astro/test/serializeManifest.test.js (JavaScript)

Code before:
import { loadFixture } from './test-utils.js';

describe('astro:config/client', () => {
	/** @type {import('./test-utils').Fixture} */
	let fixture;
	let devServer;

	describe('when the experimental flag is not enabled', async () => {
		before(async () => {
			fixture = await loadFixture({
				root: './fixtures/astro-manifest/',
				experimental: {
					serializeConfig: false,
				},
			});
			devServer = await fixture.startDevServer();
		});

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

		it('should throw an error when importing the module', async () => {
			const response = await fixture.fetch('/');
			const html = await response.text();
			assert.match(html, /CantUseAstroConfigModule/);
		});
	});

	describe('when the experimental flag is enabled in dev', async () => {
		before(async () => {
			fixture = await loadFixture({
				root: './fixtures/astro-manifest/',
			});
			devServer = await fixture.startDevServer();
		});

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

		it('should return the expected properties', async () => {
			const response = await fixture.fetch('/');
			const html = await response.text();

			const $ = cheerio.load(html);

			assert.deepEqual(
				$('#config').text(),
				JSON.stringify({
					base: '/',

Review comment (suggestion):
```suggestion
			assert.match(html, /CantUseAstroConfigModuleError/);
```

Code after:
import { loadFixture } from './test-utils.js';

describe('astro:config/client', () => {
	/** @type {import('./test-utils').Fixture} */
	let fixture;
	let devServer;

	describe('when the experimental flag is not enabled', async () => {
		before(async () => {
			fixture = await loadFixture({
				root: './fixtures/astro-manifest/',
				experimental: {
					serializeConfig: false,
				},
			});
			devServer = await fixture.startDevServer();
		});

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

		it('should throw an error when importing the module', async () => {
			const response = await fixture.fetch('/');
			const html = await response.text();
			assert.match(html, /CantUseAstroConfigModuleError/);
		});
	});

	describe('when the experimental flag is enabled in dev', async () => {
		before(async () => {
			fixture = await loadFixture({
				root: './fixtures/astro-manifest/',
			});
			devServer = await fixture.startDevServer();
		});

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

		it('should return the expected properties', async () => {
			const response = await fixture.fetch('/');
			const html = await response.text();

			const $ = cheerio.load(html);

			assert.deepEqual(
				$('#config').text(),
				JSON.stringify({
					base: '/',
Repository: withastro/astro
File: packages/astro/src/core/app/middlewares.ts (TypeScript)

Code before:
 */
const FORM_CONTENT_TYPES = [
	'application/x-www-form-urlencoded',
	'multipart/form-data',
	'text/plain',
];

const SAFE_METHODS = ['GET', 'HEAD', 'OPTIONS', 'TRACE'];

/**
 * Returns a middleware function in charge to check the `origin` header.
 *
 * @private
 */
export function createOriginCheckMiddleware(): MiddlewareHandler {
	return defineMiddleware((context, next) => {
		const { request, url, isPrerendered } = context;
		// Prerendered pages should be excluded
		if (isPrerendered) {
			return next();
		}
		// Safe methods don't require origin check
		if (SAFE_METHODS.includes(request.method)) {
			return next();
		}
		const sameOrigin = request.headers.get('origin') === url.origin;

		const hasContentType = request.headers.has('content-type');
		if (hasContentType) {
			const formLikeHeader = hasFormLikeHeader(request.headers.get('content-type'));
			if (formLikeHeader && !sameOrigin) {
				return new Response(`Cross-site ${request.method} form submissions are forbidden`, {
					status: 403,
				});
			}
		} else {
			if (!sameOrigin) {
				return new Response(`Cross-site ${request.method} form submissions are forbidden`, {
					status: 403,
				});
			}
		}

		return next();
	});
}

function hasFormLikeHeader(contentType: string | null): boolean {
	if (contentType) {
		for (const FORM_CONTENT_TYPE of FORM_CONTENT_TYPES) {
			if (contentType.toLowerCase().includes(FORM_CONTENT_TYPE)) {

Review comment (suggestion):
a nuance, imho `isSameOrigin` would fit better, as seen also in `isPrerendered`, `hasContentType` etc.

Code after:
 */
const FORM_CONTENT_TYPES = [
	'application/x-www-form-urlencoded',
	'multipart/form-data',
	'text/plain',
];

// Note: TRACE is unsupported by undici/Node.js
const SAFE_METHODS = ['GET', 'HEAD', 'OPTIONS'];

/**
 * Returns a middleware function in charge to check the `origin` header.
 *
 * @private
 */
export function createOriginCheckMiddleware(): MiddlewareHandler {
	return defineMiddleware((context, next) => {
		const { request, url, isPrerendered } = context;
		// Prerendered pages should be excluded
		if (isPrerendered) {
			return next();
		}
		// Safe methods don't require origin check
		if (SAFE_METHODS.includes(request.method)) {
			return next();
		}
		const isSameOrigin = request.headers.get('origin') === url.origin;

		const hasContentType = request.headers.has('content-type');
		if (hasContentType) {
			const formLikeHeader = hasFormLikeHeader(request.headers.get('content-type'));
			if (formLikeHeader && !isSameOrigin) {
				return new Response(`Cross-site ${request.method} form submissions are forbidden`, {
					status: 403,
				});
			}
		} else {
			if (!isSameOrigin) {
				return new Response(`Cross-site ${request.method} form submissions are forbidden`, {
					status: 403,
				});
			}
		}

		return next();
	});
}

function hasFormLikeHeader(contentType: string | null): boolean {
	if (contentType) {
		for (const FORM_CONTENT_TYPE of FORM_CONTENT_TYPES) {
Repository: withastro/astro
File: packages/integrations/partytown/src/index.ts (TypeScript)

Code before:
			'astro:config:setup': ({ config: _config, command, injectScript }) => {
				const lib = `${appendForwardSlash(_config.base)}~partytown/`;
				const recreateIFrameScript = `;(e=>{e.addEventListener("astro:before-swap",e=>{let r=document.body.querySelector("iframe[src*='${lib}']");if(r)e.newDocument.body.append(r)})})(document);`;
				const partytownConfig = {
					lib,
					...options?.config,
					debug: options?.config?.debug ?? command === 'dev',
				};
				partytownSnippetHtml = partytownSnippet(partytownConfig);
				partytownSnippetHtml += recreateIFrameScript;
				injectScript('head-inline', partytownSnippetHtml);
			},
			'astro:server:setup': ({ server }) => {
				const lib = `/~partytown/`;
				server.middlewares.use(
					sirv(partytownLibDirectory, {
						mount: lib,
						dev: true,
						etag: true,
						extensions: [],
					}),
				);
			},
			'astro:build:done': async ({ dir }) => {
				let libValue = options?.config?.lib?.replace(/^\/?/, '');
				await copyLibFiles(fileURLToPath(new URL(libValue || '~partytown', dir)), {
					debugDir: options?.config?.debug ?? false,
				});
			},
			'astro:build:ssr': async ({ manifest }) => {
				const dirpath = libDirPath({ debugDir: false });
				const files = await fs.promises.readdir(dirpath);
				for (const file of files) {
					if (file === 'debug') continue;
					manifest.assets.push(`/~partytown/${file}`);
				}
			},
		},
	};
}


Review comment (suggestion):
```suggestion
				await copyLibFiles(fileURLToPath(new URL(options?.config?.lib?.replace(/^\/?/, '') ?? '~partytown', dir)), {
```

Code after:
			'astro:config:setup': ({ config: _config, command, injectScript }) => {
				const lib = `${appendForwardSlash(_config.base)}~partytown/`;
				const recreateIFrameScript = `;(e=>{e.addEventListener("astro:before-swap",e=>{let r=document.body.querySelector("iframe[src*='${lib}']");if(r)e.newDocument.body.append(r)})})(document);`;
				const partytownConfig = {
					lib,
					...options?.config,
					debug: options?.config?.debug ?? command === 'dev',
				};
				partytownSnippetHtml = partytownSnippet(partytownConfig);
				partytownSnippetHtml += recreateIFrameScript;
				injectScript('head-inline', partytownSnippetHtml);
			},
			'astro:server:setup': ({ server }) => {
				const lib = options?.config?.lib ?? `/~partytown/`;
				server.middlewares.use(
					sirv(partytownLibDirectory, {
						mount: lib,
						dev: true,
						etag: true,
						extensions: [],
					}),
				);
			},
			'astro:build:done': async ({ dir }) => {
				await copyLibFiles(
					fileURLToPath(new URL(options?.config?.lib?.replace(/^\/?/, '') ?? '~partytown', dir)),
					{
						debugDir: options?.config?.debug ?? false,
					},
				);
			},
			'astro:build:ssr': async ({ manifest }) => {
				const dirpath = libDirPath({ debugDir: false });
				const files = await fs.promises.readdir(dirpath);
				for (const file of files) {
					if (file === 'debug') continue;
					manifest.assets.push(`/~partytown/${file}`);
				}
			},
		},
	};
}
Repository: withastro/astro
File: packages/astro/src/core/routing/rewrite.ts (TypeScript)

Code before:
	} else if (payload instanceof Request) {
		newUrl = new URL(payload.url);
	} else {
		newUrl = new URL(payload, new URL(request.url).origin);
	}
	let pathname = newUrl.pathname;
	if (base !== '/' && newUrl.pathname.startsWith(base)) {
		pathname = shouldAppendForwardSlash(trailingSlash, buildFormat)
			? appendForwardSlash(newUrl.pathname)
			: removeTrailingForwardSlash(newUrl.pathname);
		pathname = pathname.slice(base.length);
	}

	let foundRoute;
	for (const route of routes) {
		if (route.pattern.test(decodeURI(pathname))) {
			foundRoute = route;
			break;
		}
	}

	if (foundRoute) {
		return {
			routeData: foundRoute,
			newUrl,
			pathname: decodeURI(pathname),
		};
	} else {
		const custom404 = routes.find((route) => route.route === '/404');
		if (custom404) {
			return { routeData: custom404, newUrl, pathname };
		} else {
			return { routeData: DEFAULT_404_ROUTE, newUrl, pathname };
		}
	}
}

/**
 * Utility function that creates a new `Request` with a new URL from an old `Request`.
 *
 * @param newUrl The new `URL`
 * @param oldRequest The old `Request`
 * @param isPrerendered It needs to be the flag of the previous routeData, before the rewrite
 * @param logger
 * @param routePattern
 */
export function copyRequest(
	newUrl: URL,
	oldRequest: Request,
	isPrerendered: boolean,
	logger: Logger,

Review comment (suggestion):
Nit: since we already do the decoding at line 58, let's do it once: 

```js
let decodedPathname = decodeURI(pathname)
```

And let's use this variable at line 58 and line 68

Code after:
	} else if (payload instanceof Request) {
		newUrl = new URL(payload.url);
	} else {
		newUrl = new URL(payload, new URL(request.url).origin);
	}
	let pathname = newUrl.pathname;
	if (base !== '/' && newUrl.pathname.startsWith(base)) {
		pathname = shouldAppendForwardSlash(trailingSlash, buildFormat)
			? appendForwardSlash(newUrl.pathname)
			: removeTrailingForwardSlash(newUrl.pathname);
		pathname = pathname.slice(base.length);
	}

	const decodedPathname = decodeURI(pathname);
	let foundRoute;
	for (const route of routes) {
		if (route.pattern.test(decodedPathname)) {
			foundRoute = route;
			break;
		}
	}

	if (foundRoute) {
		return {
			routeData: foundRoute,
			newUrl,
			pathname: decodedPathname,
		};
	} else {
		const custom404 = routes.find((route) => route.route === '/404');
		if (custom404) {
			return { routeData: custom404, newUrl, pathname };
		} else {
			return { routeData: DEFAULT_404_ROUTE, newUrl, pathname };
		}
	}
}

/**
 * Utility function that creates a new `Request` with a new URL from an old `Request`.
 *
 * @param newUrl The new `URL`
 * @param oldRequest The old `Request`
 * @param isPrerendered It needs to be the flag of the previous routeData, before the rewrite
 * @param logger
 * @param routePattern
 */
export function copyRequest(
	newUrl: URL,
	oldRequest: Request,
	isPrerendered: boolean,