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

Code before:
				item.a?.unfix();
				(to_animate ??= new Set()).delete(item);
			}
		}

		if (item !== current) {
			if (seen !== undefined && seen.has(item)) {
				if (matched.length < stashed.length) {
					// more efficient to move later items to the front
					var start = stashed[0];
					var j;

					// full key uniqueness check is dev-only,
					// key duplicates cause crushing only due to `matched` being empty
					if (matched.length === 0) {
						var map = new Map();
						for (j = 0; j < length; j += 1) {
							var k = get_key(array[j], j);
							if (map.has(k)) {
								/** @type {string|null} */
								var key_name = String(k);
								if (key_name.startsWith('[object ')) key_name = null;
								each_key_duplicate(String(j), String(map.get(k)), key_name);
							}
							map.set(k, j);
						}
					}

					prev = start.prev;

					var a = matched[0];
					var b = matched[matched.length - 1];

					for (j = 0; j < matched.length; j += 1) {
						move(matched[j], start, anchor);
					}

					for (j = 0; j < stashed.length; j += 1) {
						seen.delete(stashed[j]);
					}

					link(state, a.prev, b.next);
					link(state, prev, a);
					link(state, b, start);

					current = start;
					prev = b;
					i -= 1;

					matched = [];
					stashed = [];

Review comment (refactor):
The arguments aren't used to construct the message in prod, so we can actually omit all this code

```suggestion
						each_key_duplicate('', '', '');
```

Code after:
				(to_animate ??= new Set()).delete(effect);
			}
		}

		if (effect !== current) {
			if (seen !== undefined && seen.has(effect)) {
				if (matched.length < stashed.length) {
					// more efficient to move later items to the front
					var start = stashed[0];
					var j;

					prev = start.prev;

					var a = matched[0];
					var b = matched[matched.length - 1];

					for (j = 0; j < matched.length; j += 1) {
						move(matched[j], start, anchor);
					}

					for (j = 0; j < stashed.length; j += 1) {
						seen.delete(stashed[j]);
					}

					link(state, a.prev, b.next);
					link(state, prev, a);
					link(state, b, start);

					current = start;
					prev = b;
					i -= 1;

					matched = [];
					stashed = [];
				} else {
					// more efficient to move earlier items to the back
					seen.delete(effect);
					move(effect, current, anchor);

					link(state, effect.prev, effect.next);
					link(state, effect, prev === null ? state.effect.first : prev.next);
					link(state, prev, effect);

					prev = effect;
				}

				continue;
			}

			matched = [];
			stashed = [];
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/blocks/each.js (JavaScript)

Code before:

			current = prev.next;
			continue;
		}

		if (should_update) {
			update_item(item, value, i, flags);
		}

		if ((item.e.f & INERT) !== 0) {
			resume_effect(item.e);
			if (is_animated) {
				item.a?.unfix();
				(to_animate ??= new Set()).delete(item);
			}
		}

		if (item !== current) {
			if (seen !== undefined && seen.has(item)) {
				if (matched.length < stashed.length) {
					// more efficient to move later items to the front
					var start = stashed[0];
					var j;

					// full key uniqueness check is dev-only,
					// key duplicates cause crushing only due to `matched` being empty
					if (matched.length === 0) {
						var map = new Map();
						for (j = 0; j < length; j += 1) {
							var k = get_key(array[j], j);
							if (map.has(k)) {
								/** @type {string|null} */
								var key_name = String(k);
								if (key_name.startsWith('[object ')) key_name = null;
								each_key_duplicate(String(j), String(map.get(k)), key_name);
							}
							map.set(k, j);
						}
					}

					prev = start.prev;

					var a = matched[0];
					var b = matched[matched.length - 1];

					for (j = 0; j < matched.length; j += 1) {
						move(matched[j], start, anchor);
					}

					for (j = 0; j < stashed.length; j += 1) {
						seen.delete(stashed[j]);

Review comment (bug):
```suggestion
					// key duplicates cause crash only due to `matched` being empty
```

Code after:
				matched = [];
				stashed = [];

				current = skip_to_branch(prev.next);
				continue;
			}
		}

		if ((effect.f & INERT) !== 0) {
			resume_effect(effect);
			if (is_animated) {
				effect.nodes?.a?.unfix();
				(to_animate ??= new Set()).delete(effect);
			}
		}

		if (effect !== current) {
			if (seen !== undefined && seen.has(effect)) {
				if (matched.length < stashed.length) {
					// more efficient to move later items to the front
					var start = stashed[0];
					var j;

					prev = start.prev;

					var a = matched[0];
					var b = matched[matched.length - 1];

					for (j = 0; j < matched.length; j += 1) {
						move(matched[j], start, anchor);
					}

					for (j = 0; j < stashed.length; j += 1) {
						seen.delete(stashed[j]);
					}

					link(state, a.prev, b.next);
					link(state, prev, a);
					link(state, b, start);

					current = start;
					prev = b;
					i -= 1;

					matched = [];
					stashed = [];
				} else {
					// more efficient to move earlier items to the back
					seen.delete(effect);
					move(effect, current, anchor);
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/server/visitors/RegularElement.js (JavaScript)

Code before:
				b.init(
					directive.name,
					directive.value === true
						? b.id(directive.name)
						: build_attribute_value(directive.value, context, optimiser.transform, true)
				)
			);

			styles = b.object(properties);
		}

		let flags = 0;

		if (node.metadata.svg || node.metadata.mathml) {
			flags |= ELEMENT_IS_NAMESPACED | ELEMENT_PRESERVE_ATTRIBUTE_CASE;
		} else if (is_custom_element_node(node)) {
			flags |= ELEMENT_PRESERVE_ATTRIBUTE_CASE;
		}

		const css_hash =
			node.metadata.scoped && context.state.analysis.css.hash
				? b.literal(context.state.analysis.css.hash)
				: undefined;

		const flag_literal = flags ? b.literal(flags) : undefined;

		const inner_state = { ...state, template: [], init: [] };
		process_children(trimmed, { ...context, state: inner_state });

		const fn = b.arrow(
			[b.id('$$renderer')],
			b.block([...state.init, ...build_template(inner_state.template)])
		);

		const statement = b.stmt(
			b.call(
				'$$renderer.select',
				attributes_expression,
				css_hash,
				classes,
				styles,
				flag_literal,
				fn
			)
		);

		if (optimiser.expressions.length > 0) {
			context.state.template.push(
				create_child_block(b.block([optimiser.apply(), ...state.init, statement]), true)
			);
		} else {

Review comment (refactor):
It's anti-DRY. We can modify and use here `build_element_spread_attributes` to return `args` and the last lines move to `build_element_attributes`. Though then it will be rather `prepare_` than `build_`.

Code after:
		const statement = b.stmt(b.call('$$renderer.option', attributes, body, ...rest));

		if (optimiser.expressions.length > 0) {
			context.state.template.push(
				create_child_block(b.block([optimiser.apply(), ...state.init, statement]), true)
			);
		} else {
			context.state.template.push(...state.init, statement);
		}

		return;
	}

	if (body !== null) {
		// if this is a `<textarea>` value or a contenteditable binding, we only add
		// the body if the attribute/binding is falsy
		const inner_state = { ...state, template: [], init: [] };
		process_children(trimmed, { ...context, state: inner_state });

		let id = /** @type {Expression} */ (body);

		if (body.type !== 'Identifier') {
			id = b.id(state.scope.generate('$$body'));
			state.template.push(b.const(id, body));
		}

		// Use the body expression as the body if it's truthy, otherwise use the inner template
		state.template.push(
			b.if(
				id,
				b.block(build_template([id])),
				b.block([...inner_state.init, ...build_template(inner_state.template)])
			)
		);
	} else {
		process_children(trimmed, { ...context, state });
	}

	if (!node_is_void) {
		state.template.push(b.literal(`</${node.name}>`));
	}

	if (dev) {
		state.template.push(b.stmt(b.call('$.pop_element')));
	}

	if (optimiser.expressions.length > 0) {
		context.state.template.push(
			create_child_block(
				b.block([optimiser.apply(), ...state.init, ...build_template(state.template)]),
				true
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/batch.js (JavaScript)

Code before:

		this.deactivate();
	}

	/**
	 * Append and remove branches to/from the DOM
	 */
	#commit() {
		for (const fn of this.#callbacks) {
			fn();
		}

		this.#callbacks.clear();

		// If there are other pending batches, they now need to be 'rebased' —
		// in other words, we re-run block/async effects with the newly
		// committed state, unless the batch in question has a more
		// recent value for a given source
		if (batches.size > 1) {
			this.#previous.clear();

			let is_earlier = true;

			for (const batch of batches) {
				if (batch === this) {
					is_earlier = true;
					continue;
				}

				for (const source of this.current.keys()) {
					if (is_earlier && batch.current.has(source)) continue;
					mark_effects(source);
				}

				if (queued_root_effects.length > 0) {
					current_batch = batch;
					const revert = Batch.apply(batch);

					for (const root of queued_root_effects) {
						batch.#traverse_effect_tree(root);
					}

					queued_root_effects = [];
					revert();
				}
			}

			current_batch = null;
		}

		batches.delete(this);

Review comment (bug):
this concerns me - what was this logic supposed to do? Right now it's doing nothing because it's true before and after, and when I change this to `is_earlier = false` tests start failing

Code after:

		this.deactivate();
	}

	/**
	 * Append and remove branches to/from the DOM
	 */
	#commit() {
		for (const fn of this.#callbacks) {
			fn();
		}

		this.#callbacks.clear();

		// If there are other pending batches, they now need to be 'rebased' —
		// in other words, we re-run block/async effects with the newly
		// committed state, unless the batch in question has a more
		// recent value for a given source
		if (batches.size > 1) {
			this.#previous.clear();

			let is_earlier = true;

			for (const batch of batches) {
				if (batch === this) {
					is_earlier = false;
					continue;
				}

				for (const [source, value] of this.current) {
					if (batch.current.has(source)) {
						if (is_earlier) {
							// bring the value up to date
							batch.current.set(source, value);
						} else {
							// later batch has more recent value,
							// no need to re-run these effects
							continue;
						}
					}

					mark_effects(source);
				}

				if (queued_root_effects.length > 0) {
					current_batch = batch;
					const revert = Batch.apply(batch);

					for (const root of queued_root_effects) {
						batch.#traverse_effect_tree(root);
					}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/server/visitors/IfBlock.js (JavaScript)

Code before:
/** @import { AST } from '#compiler' */
/** @import { ComponentContext } from '../types.js' */
import * as b from '#compiler/builders';
import { block_close, block_open, block_open_else, create_async_block } from './shared/utils.js';

/**
 * @param {AST.IfBlock} node
 * @param {ComponentContext} context
 */
export function IfBlock(node, context) {
	const test = /** @type {Expression} */ (context.visit(node.test));
	const consequent = /** @type {BlockStatement} */ (context.visit(node.consequent));

	const alternate = node.alternate
		? /** @type {BlockStatement} */ (context.visit(node.alternate))
		: b.block([]);

	consequent.body.unshift(b.stmt(b.call(b.id('$$renderer.push'), block_open)));

	alternate.body.unshift(b.stmt(b.call(b.id('$$renderer.push'), block_open_else)));

	/** @type {Statement} */
	let statement = b.if(test, consequent, alternate);

	if (node.metadata.expression.has_await) {
		statement = create_async_block(b.block([statement]));
	}

	context.state.template.push(statement, block_close);
}


Review comment (suggestion):
From my understanding, `node.metadata.expression.has_await` checks `{#if await Promise.resolve(true)}`.

So it doesn't return true when only `consequent` or `alternate` have `await` operator.

Code after:
/** @import { AST } from '#compiler' */
/** @import { ComponentContext } from '../types.js' */
import * as b from '#compiler/builders';
import { block_close, block_open, block_open_else, create_async_block } from './shared/utils.js';

/**
 * @param {AST.IfBlock} node
 * @param {ComponentContext} context
 */
export function IfBlock(node, context) {
	const test = /** @type {Expression} */ (context.visit(node.test));
	const consequent = /** @type {BlockStatement} */ (context.visit(node.consequent));

	const alternate = node.alternate
		? /** @type {BlockStatement} */ (context.visit(node.alternate))
		: b.block([]);

	consequent.body.unshift(b.stmt(b.call(b.id('$$renderer.push'), block_open)));

	alternate.body.unshift(b.stmt(b.call(b.id('$$renderer.push'), block_open_else)));

	/** @type {Statement} */
	let statement = b.if(test, consequent, alternate);

	if (
		node.metadata.expression.has_await ||
		node.consequent.metadata.has_await ||
		node.alternate?.metadata.has_await
	) {
		statement = create_async_block(b.block([statement]));
	}

	context.state.template.push(statement, block_close);
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/deriveds.js (JavaScript)

Code before:
		e.async_derived_orphan();
	}

	var boundary = /** @type {Boundary} */ (parent.b);

	var promise = /** @type {Promise<V>} */ (/** @type {unknown} */ (undefined));
	var signal = source(/** @type {V} */ (UNINITIALIZED));

	// only suspend in async deriveds created on initialisation
	var should_suspend = !active_reaction;

	/** @type {Map<Batch, ReturnType<typeof deferred<V>>>} */
	var deferreds = new Map();

	async_effect(() => {
		if (DEV) current_async_effect = active_effect;

		/** @type {ReturnType<typeof deferred<V>>} */
		var d = deferred();
		promise = d.promise;

		try {
			// If this code is changed at some point, make sure to still access the then property
			// of fn() to read any signals it might access, so that we track them as dependencies.
			Promise.resolve(fn()).then(d.resolve, d.reject).then(unset_context);
		} catch (error) {
			d.reject(error);
		}

		if (DEV) current_async_effect = null;

		var batch = /** @type {Batch} */ (current_batch);
		var pending = boundary.is_pending();

		if (should_suspend) {
			boundary.update_pending_count(1);
			if (!pending) {
				batch.increment();

				deferreds.get(batch)?.reject(STALE_REACTION);
				deferreds.set(batch, d);
			}
		}

		/**
		 * @param {any} value
		 * @param {unknown} error
		 */
		const handler = (value, error = undefined) => {
			current_async_effect = null;


Review comment (suggestion):
It would be good to have a comment explaining why we want to have it up here in addition to the handler below, but otherwise lgtm 👍 

Code after:
		e.async_derived_orphan();
	}

	var boundary = /** @type {Boundary} */ (parent.b);

	var promise = /** @type {Promise<V>} */ (/** @type {unknown} */ (undefined));
	var signal = source(/** @type {V} */ (UNINITIALIZED));

	// only suspend in async deriveds created on initialisation
	var should_suspend = !active_reaction;

	/** @type {Map<Batch, ReturnType<typeof deferred<V>>>} */
	var deferreds = new Map();

	async_effect(() => {
		if (DEV) current_async_effect = active_effect;

		/** @type {ReturnType<typeof deferred<V>>} */
		var d = deferred();
		promise = d.promise;

		try {
			// If this code is changed at some point, make sure to still access the then property
			// of fn() to read any signals it might access, so that we track them as dependencies.
			// We call `unset_context` to undo any `save` calls that happen inside `fn()`
			Promise.resolve(fn()).then(d.resolve, d.reject).then(unset_context);
		} catch (error) {
			d.reject(error);
			unset_context();
		}

		if (DEV) current_async_effect = null;

		var batch = /** @type {Batch} */ (current_batch);
		var pending = boundary.is_pending();

		if (should_suspend) {
			boundary.update_pending_count(1);
			if (!pending) {
				batch.increment();

				deferreds.get(batch)?.reject(STALE_REACTION);
				deferreds.set(batch, d);
			}
		}

		/**
		 * @param {any} value
		 * @param {unknown} error
		 */
		const handler = (value, error = undefined) => {
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/batch.js (JavaScript)

Code before:
				}

				/** @type {Source[]} */
				const sources = [];

				for (const [source, value] of this.current) {
					if (batch.current.has(source)) {
						if (is_earlier && value !== batch.current.get(source)) {
							// bring the value up to date
							batch.current.set(source, value);
						} else {
							// same value or later batch has more recent value,
							// no need to re-run these effects
							continue;
						}
					}

					sources.push(source);
				}

				// Only reschedule an async effect if it depends on something else than
				// the sources that were updated in this batch and that something else changed.
				const others = [...batch.current.keys()].filter((s) => !this.current.has(s));
				if (others.length > 0) {
					for (const source of sources) {
						mark_effects(source, others);
					}

					if (queued_root_effects.length > 0) {
						current_batch = batch;
						const revert = Batch.apply(batch);

						for (const root of queued_root_effects) {
							batch.#traverse_effect_tree(root);
						}

						queued_root_effects = [];
						revert();
					}
				}
			}

			current_batch = null;
		}

		batches.delete(this);
	}

	increment() {
		this.#pending += 1;
	}

Review comment (suggestion):
may as well bail here

```suggestion
				}

				if (sources.length === 0) {
					continue;
				}
```

Code after:

			let is_earlier = true;

			for (const batch of batches) {
				if (batch === this) {
					is_earlier = false;
					continue;
				}

				/** @type {Source[]} */
				const sources = [];

				for (const [source, value] of this.current) {
					if (batch.current.has(source)) {
						if (is_earlier && value !== batch.current.get(source)) {
							// bring the value up to date
							batch.current.set(source, value);
						} else {
							// same value or later batch has more recent value,
							// no need to re-run these effects
							continue;
						}
					}

					sources.push(source);
				}

				if (sources.length === 0) {
					continue;
				}

				// Re-run async/block effects that depend on distinct values changed in both batches
				const others = [...batch.current.keys()].filter((s) => !this.current.has(s));
				if (others.length > 0) {
					for (const source of sources) {
						mark_effects(source, others);
					}

					if (queued_root_effects.length > 0) {
						current_batch = batch;
						batch.apply();

						for (const root of queued_root_effects) {
							batch.#traverse_effect_tree(root);
						}

						queued_root_effects = [];
						batch.deactivate();
					}
				}
			}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/batch.js (JavaScript)

Code before:
			for (const batch of batches) {
				if (batch === this) {
					is_earlier = false;
					continue;
				}

				/** @type {Source[]} */
				const sources = [];

				for (const [source, value] of this.current) {
					if (batch.current.has(source)) {
						if (is_earlier && value !== batch.current.get(source)) {
							// bring the value up to date
							batch.current.set(source, value);
						} else {
							// same value or later batch has more recent value,
							// no need to re-run these effects
							continue;
						}
					}

					sources.push(source);
				}

				// Only reschedule an async effect if it depends on something else than
				// the sources that were updated in this batch and that something else changed.
				const others = [...batch.current.keys()].filter((s) => !this.current.has(s));
				if (others.length > 0) {
					for (const source of sources) {
						mark_effects(source, others);
					}

					if (queued_root_effects.length > 0) {
						current_batch = batch;
						const revert = Batch.apply(batch);

						for (const root of queued_root_effects) {
							batch.#traverse_effect_tree(root);
						}

						queued_root_effects = [];
						revert();
					}
				}
			}

			current_batch = null;
		}

		batches.delete(this);
	}

Review comment (performance):
Rather than saying what we're _not_ doing, I think that outside the context of this PR it's clearer to say what we _are_ doing

```suggestion
				// Re-run async/block effects that depend on distinct values changed in both batches
```

Code after:
		// committed state, unless the batch in question has a more
		// recent value for a given source
		if (batches.size > 1) {
			this.#previous.clear();

			let is_earlier = true;

			for (const batch of batches) {
				if (batch === this) {
					is_earlier = false;
					continue;
				}

				/** @type {Source[]} */
				const sources = [];

				for (const [source, value] of this.current) {
					if (batch.current.has(source)) {
						if (is_earlier && value !== batch.current.get(source)) {
							// bring the value up to date
							batch.current.set(source, value);
						} else {
							// same value or later batch has more recent value,
							// no need to re-run these effects
							continue;
						}
					}

					sources.push(source);
				}

				if (sources.length === 0) {
					continue;
				}

				// Re-run async/block effects that depend on distinct values changed in both batches
				const others = [...batch.current.keys()].filter((s) => !this.current.has(s));
				if (others.length > 0) {
					for (const source of sources) {
						mark_effects(source, others);
					}

					if (queued_root_effects.length > 0) {
						current_batch = batch;
						batch.apply();

						for (const root of queued_root_effects) {
							batch.#traverse_effect_tree(root);
						}

						queued_root_effects = [];