ronantakizawa/codereview-bench huggingface dataset:
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/batch.js (JavaScript)

Code before:
	 * Deferred effects that are MAYBE_DIRTY
	 * @type {Set<Effect>}
	 */
	#maybe_dirty_effects = new Set();

	/**
	 * A set of branches that still exist, but will be destroyed when this batch
	 * is committed — we skip over these during `process`
	 * @type {Set<Effect>}
	 */
	skipped_effects = new Set();

	is_fork = false;

	is_deferred() {
		return this.is_fork || this.#blocking_pending > 0;
	}

	/**
	 *
	 * @param {Effect[]} root_effects
	 */
	process(root_effects) {
		queued_root_effects = [];

		previous_batch = null;

		this.apply();

		/** @type {Effect[]} */
		var effects = [];

		/** @type {Effect[]} */
		var render_effects = [];

		for (const root of root_effects) {
			this.#traverse_effect_tree(root, effects, render_effects);
			// Note: #traverse_effect_tree runs block effects eagerly, which can schedule effects,
			// which means queued_root_effects now may be filled again.

			// Helpful for debugging reactivity loss that has to do with branches being skipped:
			// log_inconsistent_branches(root);
		}

		if (!this.is_fork) {
			this.#resolve();
		}

		if (this.is_deferred()) {
			this.#defer_effects(render_effects);
			this.#defer_effects(effects);

Review comment (performance):
`previous_batch` is only read in a `render_effect` (for input bindings) and an `effect` (for select bindings), and as such is only relevant when effects are being flushed. setting it here has no effect

Code after:
	 * Deferred effects that are MAYBE_DIRTY
	 * @type {Set<Effect>}
	 */
	#maybe_dirty_effects = new Set();

	/**
	 * A set of branches that still exist, but will be destroyed when this batch
	 * is committed — we skip over these during `process`
	 * @type {Set<Effect>}
	 */
	skipped_effects = new Set();

	is_fork = false;

	is_deferred() {
		return this.is_fork || this.#blocking_pending > 0;
	}

	/**
	 *
	 * @param {Effect[]} root_effects
	 */
	process(root_effects) {
		queued_root_effects = [];

		this.apply();

		/** @type {Effect[]} */
		var effects = [];

		/** @type {Effect[]} */
		var render_effects = [];

		for (const root of root_effects) {
			this.#traverse_effect_tree(root, effects, render_effects);
			// Note: #traverse_effect_tree runs block effects eagerly, which can schedule effects,
			// which means queued_root_effects now may be filled again.

			// Helpful for debugging reactivity loss that has to do with branches being skipped:
			// log_inconsistent_branches(root);
		}

		if (this.is_deferred()) {
			this.#defer_effects(render_effects);
			this.#defer_effects(effects);
		} else {
			// append/remove branches
			for (const fn of this.#commit_callbacks) fn();
			this.#commit_callbacks.clear();

			if (this.#pending === 0) {
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/batch.js (JavaScript)

Code before:
	 */
	process(root_effects) {
		queued_root_effects = [];

		previous_batch = null;

		this.apply();

		/** @type {Effect[]} */
		var effects = [];

		/** @type {Effect[]} */
		var render_effects = [];

		for (const root of root_effects) {
			this.#traverse_effect_tree(root, effects, render_effects);
			// Note: #traverse_effect_tree runs block effects eagerly, which can schedule effects,
			// which means queued_root_effects now may be filled again.

			// Helpful for debugging reactivity loss that has to do with branches being skipped:
			// log_inconsistent_branches(root);
		}

		if (!this.is_fork) {
			this.#resolve();
		}

		if (this.is_deferred()) {
			this.#defer_effects(render_effects);
			this.#defer_effects(effects);
		} else {
			// If sources are written to, then work needs to happen in a separate batch, else prior sources would be mixed with
			// newly updated sources, which could lead to infinite loops when effects run over and over again.
			previous_batch = this;
			current_batch = null;

			flush_queued_effects(render_effects);
			flush_queued_effects(effects);

			previous_batch = null;

			this.#deferred?.resolve();
		}

		batch_values = null;
	}

	/**
	 * Traverse the effect tree, executing effects or stashing
	 * them for later execution as appropriate
	 * @param {Effect} root

Review comment (suggestion):
if `this.is_fork` is true, `this.is_deferred()` is false. So the call to `this.#resolve()` can safely be moved to the `else` clause below. From there, we can inline the body of `this.#resolve()`

Code after:
	 */
	process(root_effects) {
		queued_root_effects = [];

		this.apply();

		/** @type {Effect[]} */
		var effects = [];

		/** @type {Effect[]} */
		var render_effects = [];

		for (const root of root_effects) {
			this.#traverse_effect_tree(root, effects, render_effects);
			// Note: #traverse_effect_tree runs block effects eagerly, which can schedule effects,
			// which means queued_root_effects now may be filled again.

			// Helpful for debugging reactivity loss that has to do with branches being skipped:
			// log_inconsistent_branches(root);
		}

		if (this.is_deferred()) {
			this.#defer_effects(render_effects);
			this.#defer_effects(effects);
		} else {
			// append/remove branches
			for (const fn of this.#commit_callbacks) fn();
			this.#commit_callbacks.clear();

			if (this.#pending === 0) {
				this.#commit();
			}

			// If sources are written to, then work needs to happen in a separate batch, else prior sources would be mixed with
			// newly updated sources, which could lead to infinite loops when effects run over and over again.
			previous_batch = this;
			current_batch = null;

			flush_queued_effects(render_effects);
			flush_queued_effects(effects);

			previous_batch = null;

			this.#deferred?.resolve();
		}

		batch_values = null;
	}

	/**
	 * Traverse the effect tree, executing effects or stashing
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/batch.js (JavaScript)

Code before:
		queued_root_effects = [];

		previous_batch = null;

		this.apply();

		/** @type {Effect[]} */
		var effects = [];

		/** @type {Effect[]} */
		var render_effects = [];

		for (const root of root_effects) {
			this.#traverse_effect_tree(root, effects, render_effects);
			// Note: #traverse_effect_tree runs block effects eagerly, which can schedule effects,
			// which means queued_root_effects now may be filled again.

			// Helpful for debugging reactivity loss that has to do with branches being skipped:
			// log_inconsistent_branches(root);
		}

		if (!this.is_fork) {
			this.#resolve();
		}

		if (this.is_deferred()) {
			this.#defer_effects(render_effects);
			this.#defer_effects(effects);
		} else {
			// If sources are written to, then work needs to happen in a separate batch, else prior sources would be mixed with
			// newly updated sources, which could lead to infinite loops when effects run over and over again.
			previous_batch = this;
			current_batch = null;

			flush_queued_effects(render_effects);
			flush_queued_effects(effects);

			previous_batch = null;

			this.#deferred?.resolve();
		}

		batch_values = null;
	}

	/**
	 * Traverse the effect tree, executing effects or stashing
	 * them for later execution as appropriate
	 * @param {Effect} root
	 * @param {Effect[]} effects
	 * @param {Effect[]} render_effects

Review comment (suggestion):
If `this.is_deferred()` is false, it means that `#blocking_pending` is `0`, so we can remove the `if` guard around the `#commit_callbacks` stuff

Code after:
		queued_root_effects = [];

		this.apply();

		/** @type {Effect[]} */
		var effects = [];

		/** @type {Effect[]} */
		var render_effects = [];

		for (const root of root_effects) {
			this.#traverse_effect_tree(root, effects, render_effects);
			// Note: #traverse_effect_tree runs block effects eagerly, which can schedule effects,
			// which means queued_root_effects now may be filled again.

			// Helpful for debugging reactivity loss that has to do with branches being skipped:
			// log_inconsistent_branches(root);
		}

		if (this.is_deferred()) {
			this.#defer_effects(render_effects);
			this.#defer_effects(effects);
		} else {
			// append/remove branches
			for (const fn of this.#commit_callbacks) fn();
			this.#commit_callbacks.clear();

			if (this.#pending === 0) {
				this.#commit();
			}

			// If sources are written to, then work needs to happen in a separate batch, else prior sources would be mixed with
			// newly updated sources, which could lead to infinite loops when effects run over and over again.
			previous_batch = this;
			current_batch = null;

			flush_queued_effects(render_effects);
			flush_queued_effects(effects);

			previous_batch = null;

			this.#deferred?.resolve();
		}

		batch_values = null;
	}

	/**
	 * Traverse the effect tree, executing effects or stashing
	 * them for later execution as appropriate
	 * @param {Effect} root
Repository: sveltejs/svelte
File: packages/svelte/src/internal/server/renderer.js (JavaScript)

Code before:
	run(thunks) {
		const context = ssr_context;

		let promise = Promise.resolve(thunks[0]());
		const promises = [promise];

		for (const fn of thunks.slice(1)) {
			promise = promise.then(() => {
				const previous_context = ssr_context;
				set_ssr_context(context);

				try {
					return fn();
				} finally {
					set_ssr_context(previous_context);
				}
			});

			promises.push(promise);
		}

		const run_promise = Promise.all(promises);
		// avoid unhandled promise rejections — we'll end up throwing in collect_async if something fails
		run_promise.catch(() => {});
		this.promise = this.promise ? Promise.all([this.promise, run_promise]) : run_promise;
		this.promise.catch(() => {});

		return promises;
	}

	/**
	 * Create a child renderer. The child renderer inherits the state from the parent,
	 * but has its own content.
	 * @param {(renderer: Renderer) => MaybePromise<void>} fn
	 */
	child(fn) {
		const child = new Renderer(this.global, this);
		this.#out.push(child);

		const parent = ssr_context;

		set_ssr_context({
			...ssr_context,
			p: parent,
			c: null,
			r: child
		});

		const result = fn(child);

		set_ssr_context(parent);

Review comment (suggestion):
It's simpler than this — we only need to `.catch` the last promise in the chain, and since `$$renderer.run` is never called directly inside a `$$renderer.child` block, we can just assign it to `this.promise`

```suggestion
		promise.catch(noop);
		this.promise = promise;
```

Code after:
	 */
	run(thunks) {
		const context = ssr_context;

		let promise = Promise.resolve(thunks[0]());
		const promises = [promise];

		for (const fn of thunks.slice(1)) {
			promise = promise.then(() => {
				const previous_context = ssr_context;
				set_ssr_context(context);

				try {
					return fn();
				} finally {
					set_ssr_context(previous_context);
				}
			});

			promises.push(promise);
		}

		// prevent unhandled rejections, and attach the promise to the renderer instance
		// so that rejections correctly cause rendering to fail
		promise.catch(noop);
		this.promise = promise;

		return promises;
	}

	/**
	 * Create a child renderer. The child renderer inherits the state from the parent,
	 * but has its own content.
	 * @param {(renderer: Renderer) => MaybePromise<void>} fn
	 */
	child(fn) {
		const child = new Renderer(this.global, this);
		this.#out.push(child);

		const parent = ssr_context;

		set_ssr_context({
			...ssr_context,
			p: parent,
			c: null,
			r: child
		});

		const result = fn(child);

		set_ssr_context(parent);
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/blocks/key.js (JavaScript)

Code before:
import { block } from '../../reactivity/effects.js';
import { hydrate_next, hydrating } from '../hydration.js';
import { BranchManager } from './branches.js';

const NAN = Symbol('NaN');

/**
 * @template V
 * @param {TemplateNode} node
 * @param {() => V} get_key
 * @param {(anchor: Node) => TemplateNode | void} render_fn
 * @returns {void}
 */
export function key(node, get_key, render_fn) {
	if (hydrating) {
		hydrate_next();
	}

	var branches = new BranchManager(node);

	var legacy = !is_runes();

	block(() => {
		var key = get_key();

		if (isNaN(/** @type {any} */ (key))) {
			key = /** @type {any} */ (NAN);
		}

		// key blocks in Svelte <5 had stupid semantics
		if (legacy && key !== null && typeof key === 'object') {
			key = /** @type {V} */ ({});
		}

		branches.ensure(key, render_fn);
	});
}


Review comment (suggestion):
```suggestion
		// NaN !== NaN, hence we do this workaround to not trigger remounts unnecessarily
		if (key !== key) {
```

Code after:
import { block } from '../../reactivity/effects.js';
import { hydrate_next, hydrating } from '../hydration.js';
import { BranchManager } from './branches.js';

const NAN = Symbol('NaN');

/**
 * @template V
 * @param {TemplateNode} node
 * @param {() => V} get_key
 * @param {(anchor: Node) => TemplateNode | void} render_fn
 * @returns {void}
 */
export function key(node, get_key, render_fn) {
	if (hydrating) {
		hydrate_next();
	}

	var branches = new BranchManager(node);

	var legacy = !is_runes();

	block(() => {
		var key = get_key();

		// NaN !== NaN, hence we do this workaround to not trigger remounts unnecessarily
		if (key !== key) {
			key = /** @type {any} */ (NAN);
		}

		// key blocks in Svelte <5 had stupid semantics
		if (legacy && key !== null && typeof key === 'object') {
			key = /** @type {V} */ ({});
		}

		branches.ensure(key, render_fn);
	});
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/blocks/key.js (JavaScript)

Code before:
import { block } from '../../reactivity/effects.js';
import { hydrate_next, hydrating } from '../hydration.js';
import { BranchManager } from './branches.js';

const NAN = Symbol('NaN');

/**
 * @template V
 * @param {TemplateNode} node
 * @param {() => V} get_key
 * @param {(anchor: Node) => TemplateNode | void} render_fn
 * @returns {void}
 */
export function key(node, get_key, render_fn) {
	if (hydrating) {
		hydrate_next();
	}

	var branches = new BranchManager(node);

	var legacy = !is_runes();

	block(() => {
		var key = get_key();

		if (isNaN(/** @type {any} */ (key))) {
			key = /** @type {any} */ (NAN);
		}

		// key blocks in Svelte <5 had stupid semantics
		if (legacy && key !== null && typeof key === 'object') {
			key = /** @type {V} */ ({});
		}

		branches.ensure(key, render_fn);
	});
}


Review comment (suggestion):
I suspect you want to use `Number.isNaN` here instead. `isNaN` will return true for everything that can't be coerced into a number.

Code after:
import { block } from '../../reactivity/effects.js';
import { hydrate_next, hydrating } from '../hydration.js';
import { BranchManager } from './branches.js';

const NAN = Symbol('NaN');

/**
 * @template V
 * @param {TemplateNode} node
 * @param {() => V} get_key
 * @param {(anchor: Node) => TemplateNode | void} render_fn
 * @returns {void}
 */
export function key(node, get_key, render_fn) {
	if (hydrating) {
		hydrate_next();
	}

	var branches = new BranchManager(node);

	var legacy = !is_runes();

	block(() => {
		var key = get_key();

		// NaN !== NaN, hence we do this workaround to not trigger remounts unnecessarily
		if (key !== key) {
			key = /** @type {any} */ (NAN);
		}

		// key blocks in Svelte <5 had stupid semantics
		if (legacy && key !== null && typeof key === 'object') {
			key = /** @type {V} */ ({});
		}

		branches.ensure(key, render_fn);
	});
}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/2-analyze/css/css-prune.js (JavaScript)

Code before:
import {
	get_parent_rules,
	get_possible_values,
	is_outer_global,
	is_unscoped_pseudo_class
} from './utils.js';
import { regex_ends_with_whitespace, regex_starts_with_whitespace } from '../../patterns.js';
import { get_attribute_chunks, is_text_attribute } from '../../../utils/ast.js';

/** @typedef {typeof NODE_PROBABLY_EXISTS | typeof NODE_DEFINITELY_EXISTS} NodeExistsValue */
/** @typedef {typeof FORWARD | typeof BACKWARD} Direction */

const NODE_PROBABLY_EXISTS = 0;
const NODE_DEFINITELY_EXISTS = 1;
const FORWARD = 0;
const BACKWARD = 1;

const whitelist_attribute_selector = new Map([
	['details', ['open']],
	['dialog', ['open']]
]);

/**
 * HTML attributes whose enumerated values are case-insensitive per the HTML spec.
 * CSS attribute selectors match these values case-insensitively in HTML documents.
 * @see https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/Attribute_selectors#description
 */
const case_insensitive_attributes = new Set([
	'accept-charset',
	'autocapitalize',
	'autocomplete',
	'behavior',
	'charset',
	'crossorigin',
	'decoding',
	'dir',
	'direction',
	'draggable',
	'enctype',
	'enterkeyhint',
	'fetchpriority',
	'formenctype',
	'formmethod',
	'formtarget',
	'hidden',
	'http-equiv',
	'inputmode',
	'kind',
	'loading',
	'method',
	'preload',

Review comment (suggestion):
```suggestion
 * @see {@link https://html.spec.whatwg.org/multipage/semantics-other.html#case-sensitivity-of-selectors HTML spec}
```

Code after:
import {
	get_parent_rules,
	get_possible_values,
	is_outer_global,
	is_unscoped_pseudo_class
} from './utils.js';
import { regex_ends_with_whitespace, regex_starts_with_whitespace } from '../../patterns.js';
import { get_attribute_chunks, is_text_attribute } from '../../../utils/ast.js';

/** @typedef {typeof NODE_PROBABLY_EXISTS | typeof NODE_DEFINITELY_EXISTS} NodeExistsValue */
/** @typedef {typeof FORWARD | typeof BACKWARD} Direction */

const NODE_PROBABLY_EXISTS = 0;
const NODE_DEFINITELY_EXISTS = 1;
const FORWARD = 0;
const BACKWARD = 1;

const whitelist_attribute_selector = new Map([
	['details', ['open']],
	['dialog', ['open']]
]);

/**
 * HTML attributes whose enumerated values are case-insensitive per the HTML spec.
 * CSS attribute selectors match these values case-insensitively in HTML documents.
 * @see {@link https://html.spec.whatwg.org/multipage/semantics-other.html#case-sensitivity-of-selectors HTML spec}
 */
const case_insensitive_attributes = new Set([
	'accept-charset',
	'autocapitalize',
	'autocomplete',
	'behavior',
	'charset',
	'crossorigin',
	'decoding',
	'dir',
	'direction',
	'draggable',
	'enctype',
	'enterkeyhint',
	'fetchpriority',
	'formenctype',
	'formmethod',
	'formtarget',
	'hidden',
	'http-equiv',
	'inputmode',
	'kind',
	'loading',
	'method',
	'preload',
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/blocks/boundary.js (JavaScript)

Code before:
			}

			this.#run(() => {
				// If the failure happened while flushing effects, current_batch can be null
				Batch.ensure();

				this.#render();
			});
		};

		queue_micro_task(() => {
			try {
				calling_on_error = true;
				onerror?.(error, reset);
				calling_on_error = false;
			} catch (error) {
				invoke_error_boundary(error, this.#effect && this.#effect.parent);
			}

			if (failed) {
				this.#failed_effect = this.#run(() => {
					Batch.ensure();

					try {
						return branch(() => {
							// errors in `failed` snippets cause the boundary to error again
							var effect = /** @type {Effect} */ (active_effect);

							effect.b = this;
							effect.f |= BOUNDARY_EFFECT;

							failed(
								this.#anchor,
								() => error,
								() => reset
							);
						});
					} catch (error) {
						invoke_error_boundary(error, /** @type {Effect} */ (this.#effect.parent));
						return null;
					}
				});
			}
		});
	}
}

export function get_boundary() {
	return /** @type {Boundary} */ (/** @type {Effect} */ (active_effect).b);
}


Review comment (bug):

```suggestion
							// errors in `failed` snippets cause the boundary to error again
							// TODO Svelte 6: revisit this decision, most likely better to go to parent boundary instead
```

Code after:
			}

			this.#run(() => {
				// If the failure happened while flushing effects, current_batch can be null
				Batch.ensure();

				this.#render();
			});
		};

		queue_micro_task(() => {
			try {
				calling_on_error = true;
				onerror?.(error, reset);
				calling_on_error = false;
			} catch (error) {
				invoke_error_boundary(error, this.#effect && this.#effect.parent);
			}

			if (failed) {
				this.#failed_effect = this.#run(() => {
					Batch.ensure();

					try {
						return branch(() => {
							// errors in `failed` snippets cause the boundary to error again
							// TODO Svelte 6: revisit this decision, most likely better to go to parent boundary instead
							var effect = /** @type {Effect} */ (active_effect);

							effect.b = this;
							effect.f |= BOUNDARY_EFFECT;

							failed(
								this.#anchor,
								() => error,
								() => reset
							);
						});
					} catch (error) {
						invoke_error_boundary(error, /** @type {Effect} */ (this.#effect.parent));
						return null;
					}
				});
			}
		});
	}
}

export function get_boundary() {
	return /** @type {Boundary} */ (/** @type {Effect} */ (active_effect).b);
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/reconciler.js (JavaScript)

Code before:
/** @import {} from 'trusted-types' */

import { create_element } from './operations.js';

const policy =
	globalThis?.window?.trustedTypes &&
	/* @__PURE__ */ globalThis.window.trustedTypes.createPolicy('svelte-trusted-html', {
		/** @param {string} html */
		createHTML: (html) => {
			return html;
		}
	});

/** @param {string} html */
function create_trusted_html(html) {
	return /** @type {string} */ (policy?.createHTML(html) ?? html);
}

/**
 * @param {string} html
 * @param {boolean} trusted
 */
export function create_fragment_from_html(html, trusted = false) {
	var elem = create_element('template');
	html = html.replaceAll('<!>', '<!---->'); // XHTML compliance
	elem.innerHTML = trusted ? create_trusted_html(html) : html;
	return elem.content;
}


Review comment (bug):
```suggestion
	// We gotta write it like this because after downleveling the pure comment may end up in the wrong location
	globalThis?.window?.trustedTypes &&
```

Code after:
/** @import {} from 'trusted-types' */

import { create_element } from './operations.js';

const policy =
	// We gotta write it like this because after downleveling the pure comment may end up in the wrong location
	globalThis?.window?.trustedTypes &&
	/* @__PURE__ */ globalThis.window.trustedTypes.createPolicy('svelte-trusted-html', {
		/** @param {string} html */
		createHTML: (html) => {
			return html;
		}
	});

/** @param {string} html */
function create_trusted_html(html) {
	return /** @type {string} */ (policy?.createHTML(html) ?? html);
}

/**
 * @param {string} html
 * @param {boolean} trusted
 */
export function create_fragment_from_html(html, trusted = false) {
	var elem = create_element('template');
	html = html.replaceAll('<!>', '<!---->'); // XHTML compliance
	elem.innerHTML = trusted ? create_trusted_html(html) : html;
	return elem.content;
}