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

Code before:
		regexStr += '(:[0-9]+)?';
	}

	if (pathname) {
		if (pathname.endsWith('/**')) {
			// Match any path.
			regexStr += `(\\${pathname.replace('/**', '')}.*)`;
		}
		if (pathname.endsWith('/*')) {
			// Match one level of path
			regexStr += `(\\${pathname.replace('/*', '')}\/[^/?#]+)\/?`;
		} else {
			// Exact match
			regexStr += `(\\${pathname})`;
		}
	} else {
		// Default to matching any path
		regexStr += '(\\/[^?#]*)?';
	}
	if (!regexStr.endsWith('.*)')) {
		// Match query, but only if it's not already matched by the pathname
		regexStr += '([?][^#]*)?';
	}
	try {
		new RegExp(regexStr);
	} catch (_e) {
		logger.warn(
			`Could not generate a valid regex from the remotePattern "${JSON.stringify(
				pattern,
			)}". Please check the syntax.`,
		);
		return undefined;
	}
	return regexStr;
}

async function writeNetlifyFrameworkConfig(config: AstroConfig, logger: AstroIntegrationLogger) {
	const remoteImages: Array<string> = [];
	// Domains get a simple regex match
	remoteImages.push(
		...config.image.domains.map((domain) => `https?:\/\/${domain.replaceAll('.', '\\.')}\/.*`),
	);
	// Remote patterns need to be converted to regexes
	remoteImages.push(
		...config.image.remotePatterns
			.map((pattern) => remotePatternToRegex(pattern, logger))
			.filter(Boolean as unknown as (pattern?: string) => pattern is string),
	);

	const headers = config.build.assetsPrefix
		? undefined

Review comment (suggestion):
```suggestion
	} catch {
```
I think that works

Code after:
		regexStr += '(:[0-9]+)?';
	}

	if (pathname) {
		if (pathname.endsWith('/**')) {
			// Match any path.
			regexStr += `(\\${pathname.replace('/**', '')}.*)`;
		}
		if (pathname.endsWith('/*')) {
			// Match one level of path
			regexStr += `(\\${pathname.replace('/*', '')}\/[^/?#]+)\/?`;
		} else {
			// Exact match
			regexStr += `(\\${pathname})`;
		}
	} else {
		// Default to matching any path
		regexStr += '(\\/[^?#]*)?';
	}
	if (!regexStr.endsWith('.*)')) {
		// Match query, but only if it's not already matched by the pathname
		regexStr += '([?][^#]*)?';
	}
	try {
		new RegExp(regexStr);
	} catch {
		logger.warn(
			`Could not generate a valid regex from the remotePattern "${JSON.stringify(
				pattern,
			)}". Please check the syntax.`,
		);
		return undefined;
	}
	return regexStr;
}

async function writeNetlifyFrameworkConfig(config: AstroConfig, logger: AstroIntegrationLogger) {
	const remoteImages: Array<string> = [];
	// Domains get a simple regex match
	remoteImages.push(
		...config.image.domains.map((domain) => `https?:\/\/${domain.replaceAll('.', '\\.')}\/.*`),
	);
	// Remote patterns need to be converted to regexes
	remoteImages.push(
		...config.image.remotePatterns
			.map((pattern) => remotePatternToRegex(pattern, logger))
			.filter(Boolean as unknown as (pattern?: string) => pattern is string),
	);

	const headers = config.build.assetsPrefix
		? undefined
Repository: withastro/astro
File: packages/astro/src/assets/fonts/providers/local.ts (TypeScript)

Code before:

	for (const src of family.src) {
		for (const weight of src.weights ?? DEFAULTS.weights) {
			for (const style of src.styles ?? DEFAULTS.styles) {
				// TODO: handle fallbacks?
				// TODO: handle subset
				fonts.push({
					weight,
					style,
					src: src.paths.map((path) => ({
						url: proxyURL(fileURLToPath(new URL(path, root))),
						format: extractFontType(path),
					})),
				});
			}
		}
	}

	return {
		fonts,
	};
}

export class LocalFontsWatcher {
	getPaths: (() => Array<string>) | null = null;
	update: (() => void) | null = null;

	#matches(path: string): boolean {
		return this.getPaths?.().includes(path) ?? false;
	}

	onUpdate(path: string) {
		if (!this.#matches(path)) {
			return;
		}
		this.update?.();
	}
	onUnlink(path: string) {
		if (!this.#matches(path)) {
			return;
		}
		// TODO: improve
		throw new Error('File used for font deleted. Restore it or update your config');
	}
}


Review comment (suggestion):
If this function can be null, then have the `constructor` accept the `update` callback. Doing `const a = new AClass(); a.update() = something` isn't a good pattern, because it relies on the caller to **mutate** internal functions of the class. 

Same for `getPaths`. 

Code after:

	for (const src of family.src) {
		for (const weight of src.weights ?? DEFAULTS.weights) {
			for (const style of src.styles ?? DEFAULTS.styles) {
				// TODO: handle fallbacks?
				// TODO: handle subset
				fonts.push({
					weight,
					style,
					src: src.paths.map((path) => ({
						url: proxyURL(fileURLToPath(new URL(path, root))),
						format: extractFontType(path),
					})),
				});
			}
		}
	}

	return {
		fonts,
	};
}

/**
 * Orchestrates local font updates and deletions during development
 */
export class LocalFontsWatcher {
	/**
	 * Watched fonts files
	 */
	#paths: Array<string>;
	/**
	 * Action performed when a font file is updated
	 */
	#update: () => void;

	constructor({ paths, update }: { paths: Array<string>; update: () => void }) {
		this.#paths = paths;
		this.#update = update;
	}

	#matches(path: string): boolean {
		return this.#paths.includes(path);
	}

	/**
	 * Callback to call whenever a file is updated
	 */
	onUpdate(path: string): void {
		if (!this.#matches(path)) {
			return;
Repository: withastro/astro
File: packages/astro/src/assets/fonts/utils.ts (TypeScript)

Code before:
		await storage.setItemRaw(key, data);
		return { cached: false, data };
	};
}

export type CacheHandler = ReturnType<typeof createCache>;

export interface ProxyURLOptions {
	hashString: (value: string) => string;
	collect: (data: {
		hash: string;
		type: FontType;
		value: string;
	}) => string;
}

/**
 * The fonts data we receive contains urls or file paths we do no control.
 * However, we will emit font files ourselves so we store the original value
 * and replace it with a url we control. For example with the value "https://foo.bar/file.woff2":
 * - font type is woff2
 * - hash will be "<hash>.woff2"
 * - `collect` will save the association of the original url and the new hash for later use
 * - the returned url will be `/_astro/fonts/<hash>.woff2`
 */
export function proxyURL(value: string, { hashString, collect }: ProxyURLOptions): string {
	const type = extractFontType(value);
	const hash = `${hashString(value)}.${type}`;
	const url = collect({ hash, type, value });
	// Now that we collected the original url, we return our proxy so the consumer can override it
	return url;
}


Review comment (suggestion):
nit: Options are usually optional, which isn't the case. Also, please start documenting stuff because it's getting very difficult to understand the flow of the code.

For example:
- the documentation of this method should be updated because `hashString` and `collect` aren't passed directly anymore, but they are part of the options


You might want to use `JSDoc` to keep track of the variables. 

Code after:
		await storage.setItemRaw(key, data);
		return { cached: false, data };
	};
}

export type CacheHandler = ReturnType<typeof createCache>;

export interface ProxyURLOptions {
	/**
	 * The original URL
	 */
	value: string;
	/**
	 * Specifies how the hash is computed. Can be based on the value,
	 * a specific string for testing etc 
	 */
	hashString: (value: string) => string;
	/**
	 * Use the hook to save the associated value and hash, and possibly
	 * transform it (eg. apply a base)
	 */
	collect: (data: {
		hash: string;
		type: FontType;
		value: string;
	}) => string;
}

/**
 * The fonts data we receive contains urls or file paths we do no control.
 * However, we will emit font files ourselves so we store the original value
 * and replace it with a url we control. For example with the value "https://foo.bar/file.woff2":
 * - font type is woff2
 * - hash will be "<hash>.woff2"
 * - `collect` will save the association of the original url and the new hash for later use
 * - the returned url will be `/_astro/fonts/<hash>.woff2`
 */
export function proxyURL({ value, hashString, collect }: ProxyURLOptions): string {
	const type = extractFontType(value);
	const hash = `${hashString(value)}.${type}`;
	const url = collect({ hash, type, value });
	// Now that we collected the original url, we return our proxy so the consumer can override it
	return url;
}
Repository: withastro/astro
File: packages/astro/src/types/public/config.ts (TypeScript)

Code before:
	 * Set custom HTTP response headers to be sent in `astro dev` and `astro preview`.
	 */
	headers?: OutgoingHttpHeaders;

	/**
	 * @name server.open
	 * @type {string | boolean}
	 * @default `false`
	 * @version 4.1.0
	 * @description
	 * Controls whether the dev server should open in your browser window on startup.
	 *
	 * Pass a full URL string (e.g. "http://example.com") or a pathname (e.g. "/about") to specify the URL to open.
	 *
	 * ```js
	 * {
	 *   server: { open: "/about" }
	 * }
	 * ```
	 */
	open?: string | boolean;

	/**
	 * @name server.allowedHosts
	 * @type {string[] | true}
	 * @default `false`
	 * @version 5.4.0
	 * @description
	 * 
	 * A list of hostnames that Astro is allowed to respond to. When the value is set to `true`, any 
	 * hostname is allowed. 
	 *
	 * ```js
	 * {
	 *   server: { 
	 *   	allowedHosts: ['staging.example.com', 'qa.example.com']
	 *   }
	 * }
	 * ```
	 */
	allowedHosts?: string[] | boolean;
};

export type SessionDriverName = BuiltinDriverName | 'custom' | 'test';

interface CommonSessionConfig {
	/**
	 * Configures the session cookie. If set to a string, it will be used as the cookie name.
	 * Alternatively, you can pass an object with additional options.
	 */
	cookie?:

Review comment (suggestion):
Wouldn't the default be [an empty array](https://github.com/withastro/astro/blob/84d8fcf5df348bf988fdc6da09de141f88568534/packages/astro/src/core/config/schema.ts#L80) instead?
```suggestion
	 * @default `[]`
```

Code after:
	 * @description
	 *
	 * A list of hostnames that Astro is allowed to respond to. When the value is set to `true`, any
	 * hostname is allowed.
	 *
	 * ```js
	 * {
	 *   server: { 
	 *   	allowedHosts: ['staging.example.com', 'qa.example.com']
	 *   }
	 * }
	 * ```
	 */
	allowedHosts?: string[] | true;

	/**
	 * @name server.headers
	 * @typeraw {OutgoingHttpHeaders}
	 * @default `{}`
	 * @version 1.7.0
	 * @description
	 * Set custom HTTP response headers to be sent in `astro dev` and `astro preview`.
	 */
	headers?: OutgoingHttpHeaders;

	/**
	 * @name server.open
	 * @type {string | boolean}
	 * @default `false`
	 * @version 4.1.0
	 * @description
	 * Controls whether the dev server should open in your browser window on startup.
	 *
	 * Pass a full URL string (e.g. "http://example.com") or a pathname (e.g. "/about") to specify the URL to open.
	 *
	 * ```js
	 * {
	 *   server: { open: "/about" }
	 * }
	 * ```
	 */
	open?: string | boolean;
};

export type SessionDriverName = BuiltinDriverName | 'custom' | 'test';

interface CommonSessionConfig {
	/**
	 * Configures the session cookie. If set to a string, it will be used as the cookie name.
	 * Alternatively, you can pass an object with additional options.
	 */
Repository: withastro/astro
File: packages/astro/src/types/public/config.ts (TypeScript)

Code before:
	 * {
	 *   server: { open: "/about" }
	 * }
	 * ```
	 */
	open?: string | boolean;

	/**
	 * @name server.allowedHosts
	 * @type {string[] | true}
	 * @default `false`
	 * @version 5.4.0
	 * @description
	 * 
	 * A list of hostnames that Astro is allowed to respond to. When the value is set to `true`, any 
	 * hostname is allowed. 
	 *
	 * ```js
	 * {
	 *   server: { 
	 *   	allowedHosts: ['staging.example.com', 'qa.example.com']
	 *   }
	 * }
	 * ```
	 */
	allowedHosts?: string[] | boolean;
};

export type SessionDriverName = BuiltinDriverName | 'custom' | 'test';

interface CommonSessionConfig {
	/**
	 * Configures the session cookie. If set to a string, it will be used as the cookie name.
	 * Alternatively, you can pass an object with additional options.
	 */
	cookie?:
		| string
		| (Omit<AstroCookieSetOptions, 'httpOnly' | 'expires' | 'encode'> & { name?: string });

	/**
	 * Default session duration in seconds. If not set, the session will be stored until deleted, or until the cookie expires.
	 */
	ttl?: number;
}

interface BuiltinSessionConfig<TDriver extends keyof BuiltinDriverOptions>
	extends CommonSessionConfig {
	driver: TDriver;
	options?: BuiltinDriverOptions[TDriver];
}


Review comment (suggestion):
Shouldn't we use `true` here instead of `boolean`? Since the type is `z.literal(true)`, when using `false`, Typescript will throw an error, right?
```suggestion
	allowedHosts?: string[] | true;
```
Also, it seems we use `false` instead of `boolean` for `markdown.syntaxHighlight` which uses `z.literal(false)`. So it would be consistent.

Code after:
	/**
	 * @name server.headers
	 * @typeraw {OutgoingHttpHeaders}
	 * @default `{}`
	 * @version 1.7.0
	 * @description
	 * Set custom HTTP response headers to be sent in `astro dev` and `astro preview`.
	 */
	headers?: OutgoingHttpHeaders;

	/**
	 * @name server.open
	 * @type {string | boolean}
	 * @default `false`
	 * @version 4.1.0
	 * @description
	 * Controls whether the dev server should open in your browser window on startup.
	 *
	 * Pass a full URL string (e.g. "http://example.com") or a pathname (e.g. "/about") to specify the URL to open.
	 *
	 * ```js
	 * {
	 *   server: { open: "/about" }
	 * }
	 * ```
	 */
	open?: string | boolean;
};

export type SessionDriverName = BuiltinDriverName | 'custom' | 'test';

interface CommonSessionConfig {
	/**
	 * Configures the session cookie. If set to a string, it will be used as the cookie name.
	 * Alternatively, you can pass an object with additional options.
	 */
	cookie?:
		| string
		| (Omit<AstroCookieSetOptions, 'httpOnly' | 'expires' | 'encode'> & { name?: string });

	/**
	 * Default session duration in seconds. If not set, the session will be stored until deleted, or until the cookie expires.
	 */
	ttl?: number;
}

interface BuiltinSessionConfig<TDriver extends keyof BuiltinDriverOptions>
	extends CommonSessionConfig {
	driver: TDriver;
	options?: BuiltinDriverOptions[TDriver];
}
Repository: withastro/astro
File: packages/astro/src/cli/preview/index.ts (TypeScript)

Code before:
import { cyan } from 'kleur/colors';
import { printHelp } from '../../core/messages.js';
import previewServer from '../../core/preview/index.js';
import { type Flags, flagsToAstroInlineConfig } from '../flags.js';

interface PreviewOptions {
	flags: Flags;
}

export async function preview({ flags }: PreviewOptions) {
	if (flags?.help || flags?.h) {
		printHelp({
			commandName: 'astro preview',
			usage: '[...flags]',
			tables: {
				Flags: [
					['--port', `Specify which port to run on. Defaults to 4321.`],
					['--host', `Listen on all addresses, including LAN and public addresses.`],
					['--host <custom-address>', `Expose on a network IP address at <custom-address>`],
					['--open', 'Automatically open the app in the browser on server start'],
					['--allowed-hosts', 'Specify a comma-separated list of allowed hosts.'],
					['--help (-h)', 'See all available flags.'],
				],
			},
			description: `Starts a local server to serve your static dist/ directory. Check ${cyan(
				'https://docs.astro.build/en/reference/cli-reference/#astro-preview',
			)} for more information.`,
		});
		return;
	}

	const inlineConfig = flagsToAstroInlineConfig(flags);

	return await previewServer(inlineConfig);
}


Review comment (suggestion):
Same as above for the `dev` command:
```suggestion
					['--allowed-hosts', 'Specify a comma-separated list of allowed hosts or allow any hostname.'],
```

Code after:
import { cyan } from 'kleur/colors';
import { printHelp } from '../../core/messages.js';
import previewServer from '../../core/preview/index.js';
import { type Flags, flagsToAstroInlineConfig } from '../flags.js';

interface PreviewOptions {
	flags: Flags;
}

export async function preview({ flags }: PreviewOptions) {
	if (flags?.help || flags?.h) {
		printHelp({
			commandName: 'astro preview',
			usage: '[...flags]',
			tables: {
				Flags: [
					['--port', `Specify which port to run on. Defaults to 4321.`],
					['--host', `Listen on all addresses, including LAN and public addresses.`],
					['--host <custom-address>', `Expose on a network IP address at <custom-address>`],
					['--open', 'Automatically open the app in the browser on server start'],
					['--allowed-hosts', 'Specify a comma-separated list of allowed hosts or allow any hostname.'],
					['--help (-h)', 'See all available flags.'],
				],
			},
			description: `Starts a local server to serve your static dist/ directory. Check ${cyan(
				'https://docs.astro.build/en/reference/cli-reference/#astro-preview',
			)} for more information.`,
		});
		return;
	}

	const inlineConfig = flagsToAstroInlineConfig(flags);

	return await previewServer(inlineConfig);
}
Repository: withastro/astro
File: packages/astro/src/types/public/config.ts (TypeScript)

Code before:
	/**
	 * @name server.open
	 * @type {string | boolean}
	 * @default `false`
	 * @version 4.1.0
	 * @description
	 * Controls whether the dev server should open in your browser window on startup.
	 *
	 * Pass a full URL string (e.g. "http://example.com") or a pathname (e.g. "/about") to specify the URL to open.
	 *
	 * ```js
	 * {
	 *   server: { open: "/about" }
	 * }
	 * ```
	 */
	open?: string | boolean;

	/**
	 * @name server.allowedHosts
	 * @type {string[] | true}
	 * @default `[]`
	 * @version 5.4.0
	 * @description
	 * 
	 * A list of hostnames that Astro is allowed to respond to. When the value is set to `true`, any 
	 * hostname is allowed. 
	 *
	 * ```js
	 * {
	 *   server: { 
	 *   	allowedHosts: ['staging.example.com', 'qa.example.com']
	 *   }
	 * }
	 * ```
	 */
	allowedHosts?: string[] | true;
};

export type SessionDriverName = BuiltinDriverName | 'custom' | 'test';

interface CommonSessionConfig {
	/**
	 * Configures the session cookie. If set to a string, it will be used as the cookie name.
	 * Alternatively, you can pass an object with additional options.
	 */
	cookie?:
		| string
		| (Omit<AstroCookieSetOptions, 'httpOnly' | 'expires' | 'encode'> & { name?: string });

	/**

Review comment (suggestion):
Description looks great!

I noticed that Vite's documentation includes multiple warnings (which hosts can I trust? using `true` is dangerous and we suggest you don't do it...) Do we feel the need for any of that here?

Otherwise assuming it renders fine in preview, looks good to me!

Code after:
	 *
	 * ```js
	 * {
	 *   server: { 
	 *   	allowedHosts: ['staging.example.com', 'qa.example.com']
	 *   }
	 * }
	 * ```
	 */
	allowedHosts?: string[] | true;

	/**
	 * @name server.headers
	 * @typeraw {OutgoingHttpHeaders}
	 * @default `{}`
	 * @version 1.7.0
	 * @description
	 * Set custom HTTP response headers to be sent in `astro dev` and `astro preview`.
	 */
	headers?: OutgoingHttpHeaders;

	/**
	 * @name server.open
	 * @type {string | boolean}
	 * @default `false`
	 * @version 4.1.0
	 * @description
	 * Controls whether the dev server should open in your browser window on startup.
	 *
	 * Pass a full URL string (e.g. "http://example.com") or a pathname (e.g. "/about") to specify the URL to open.
	 *
	 * ```js
	 * {
	 *   server: { open: "/about" }
	 * }
	 * ```
	 */
	open?: string | boolean;
};

export type SessionDriverName = BuiltinDriverName | 'custom' | 'test';

interface CommonSessionConfig {
	/**
	 * Configures the session cookie. If set to a string, it will be used as the cookie name.
	 * Alternatively, you can pass an object with additional options.
	 */
	cookie?:
		| string
		| (Omit<AstroCookieSetOptions, 'httpOnly' | 'expires' | 'encode'> & { name?: string });
Repository: withastro/astro
File: packages/astro/src/types/public/config.ts (TypeScript)

Code before:
	 * @default `{}`
	 * @version 1.7.0
	 * @description
	 * Set custom HTTP response headers to be sent in `astro dev` and `astro preview`.
	 */
	headers?: OutgoingHttpHeaders;

	/**
	 * @name server.open
	 * @type {string | boolean}
	 * @default `false`
	 * @version 4.1.0
	 * @description
	 * Controls whether the dev server should open in your browser window on startup.
	 *
	 * Pass a full URL string (e.g. "http://example.com") or a pathname (e.g. "/about") to specify the URL to open.
	 *
	 * ```js
	 * {
	 *   server: { open: "/about" }
	 * }
	 * ```
	 */
	open?: string | boolean;

	/**
	 * @name server.allowedHosts
	 * @type {string[] | true}
	 * @default `[]`
	 * @version 5.4.0
	 * @description
	 * 
	 * A list of hostnames that Astro is allowed to respond to. When the value is set to `true`, any 
	 * hostname is allowed. 
	 *
	 * ```js
	 * {
	 *   server: { 
	 *   	allowedHosts: ['staging.example.com', 'qa.example.com']
	 *   }
	 * }
	 * ```
	 */
	allowedHosts?: string[] | true;
};

export type SessionDriverName = BuiltinDriverName | 'custom' | 'test';

interface CommonSessionConfig {
	/**
	 * Configures the session cookie. If set to a string, it will be used as the cookie name.

Review comment (suggestion):
The entry itself looks good, but we don't *have* to just add it at the end of existing ones.

Since there are already entries for (in this order):

- `server.host`
- `server.port`
- `server.open`

Where does this make the most *sense* for placement? Should it maybe come after `server.host`? (It definitely feels more related to `host` and even `port` than to `open`.)

Code after:
	 * @type {string[] | true}
	 * @default `[]`
	 * @version 5.4.0
	 * @description
	 *
	 * A list of hostnames that Astro is allowed to respond to. When the value is set to `true`, any
	 * hostname is allowed.
	 *
	 * ```js
	 * {
	 *   server: { 
	 *   	allowedHosts: ['staging.example.com', 'qa.example.com']
	 *   }
	 * }
	 * ```
	 */
	allowedHosts?: string[] | true;

	/**
	 * @name server.headers
	 * @typeraw {OutgoingHttpHeaders}
	 * @default `{}`
	 * @version 1.7.0
	 * @description
	 * Set custom HTTP response headers to be sent in `astro dev` and `astro preview`.
	 */
	headers?: OutgoingHttpHeaders;

	/**
	 * @name server.open
	 * @type {string | boolean}
	 * @default `false`
	 * @version 4.1.0
	 * @description
	 * Controls whether the dev server should open in your browser window on startup.
	 *
	 * Pass a full URL string (e.g. "http://example.com") or a pathname (e.g. "/about") to specify the URL to open.
	 *
	 * ```js
	 * {
	 *   server: { open: "/about" }
	 * }
	 * ```
	 */
	open?: string | boolean;
};

export type SessionDriverName = BuiltinDriverName | 'custom' | 'test';

interface CommonSessionConfig {
	/**