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

Code before:
			renderer.push(`</${tag}>`);
		}
	}

	renderer.push('<!---->');
}

/**
 * Only available on the server and when compiling with the `server` option.
 * Takes a component and returns an object with `body` and `head` properties on it, which you can use to populate the HTML when server-rendering your app.
 * @template {Record<string, any>} Props
 * @param {Component<Props> | ComponentType<SvelteComponent<Props>>} component
 * @param {{ props?: Omit<Props, '$$slots' | '$$events'>; context?: Map<any, any>; idPrefix?: string; csp?: Csp }} [options]
 * @returns {RenderOutput}
 */
export function render(component, options = {}) {
	let csp;
	if (options.csp) {
		csp =
			'nonce' in options.csp
				? { nonce: options.csp.nonce, hash: false }
				: { hash: options.csp.hash, nonce: undefined };

		// @ts-expect-error
		if (options.csp.hash && options.csp.nonce) {
			w.invalid_csp();
		}
	}
	return Renderer.render(/** @type {Component<Props>} */ (component), {
		...options,
		csp
	});
}

/**
 * @param {string} hash
 * @param {Renderer} renderer
 * @param {(renderer: Renderer) => Promise<void> | void} fn
 * @returns {void}
 */
export function head(hash, renderer, fn) {
	renderer.head((renderer) => {
		renderer.push(`<!--${hash}-->`);
		renderer.child(fn);
		renderer.push(EMPTY_COMMENT);
	});
}

/**
 * @param {Renderer} renderer
 * @param {boolean} is_html

Review comment (suggestion):
see https://github.com/sveltejs/svelte/pull/17338/files#r2614826726 — I think this should be an error

Code after:
			renderer.push(`</${tag}>`);
		}
	}

	renderer.push('<!---->');
}

/**
 * Only available on the server and when compiling with the `server` option.
 * Takes a component and returns an object with `body` and `head` properties on it, which you can use to populate the HTML when server-rendering your app.
 * @template {Record<string, any>} Props
 * @param {Component<Props> | ComponentType<SvelteComponent<Props>>} component
 * @param {{ props?: Omit<Props, '$$slots' | '$$events'>; context?: Map<any, any>; idPrefix?: string; csp?: Csp }} [options]
 * @returns {RenderOutput}
 */
export function render(component, options = {}) {
	if (options.csp?.hash && options.csp.nonce) {
		e.invalid_csp();
	}
	return Renderer.render(/** @type {Component<Props>} */ (component), options);
}

/**
 * @param {string} hash
 * @param {Renderer} renderer
 * @param {(renderer: Renderer) => Promise<void> | void} fn
 * @returns {void}
 */
export function head(hash, renderer, fn) {
	renderer.head((renderer) => {
		renderer.push(`<!--${hash}-->`);
		renderer.child(fn);
		renderer.push(EMPTY_COMMENT);
	});
}

/**
 * @param {Renderer} renderer
 * @param {boolean} is_html
 * @param {Record<string, string>} props
 * @param {() => void} component
 * @param {boolean} dynamic
 * @returns {void}
 */
export function css_props(renderer, is_html, props, component, dynamic = false) {
	const styles = style_object_to_string(props);

	if (is_html) {
		renderer.push(`<svelte-css-wrapper style="display: contents; ${styles}">`);
	} else {
		renderer.push(`<g style="${styles}">`);
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/runtime.js (JavaScript)

Code before:

			// if the derived is dirty and has reactions, or depends on the values that just changed, re-execute
			// (a derived can be maybe_dirty due to the effect destroy removing its last reaction)
			if (
				((derived.f & CLEAN) === 0 && derived.reactions !== null) ||
				depends_on_old_values(derived)
			) {
				value = execute_derived(derived);
			}

			old_values.set(derived, value);

			return value;
		}
	} else if (
		is_derived &&
		(!batch_values?.has(signal) || (current_batch?.is_fork && !effect_tracking()))
	) {
		derived = /** @type {Derived} */ (signal);

		var needs_connection =
			(derived.f & CONNECTED) === 0 &&
			!untracking &&
			((is_updating_effect &&
				(effect_tracking() ||
					(active_effect !== null && (active_effect.f & BRANCH_EFFECT) !== 0))) ||
				// evaluating connected parent derived, so reconnect child deriveds too
				(active_reaction !== null && (active_reaction.f & CONNECTED) !== 0));

		if (is_dirty(derived)) {
			var is_new = derived.v === UNINITIALIZED;

			if (needs_connection) {
				derived.f |= CONNECTED;
			}

			update_derived(derived);

			if (needs_connection && !is_new && derived.deps !== null) {
				for (const dep of derived.deps) {
					(dep.reactions ??= []).push(derived);
				}
			}
		} else if (needs_connection) {
			reconnect(derived);
		}
	}

	if (batch_values?.has(signal)) {
		return batch_values.get(signal);
	}

Review comment (bug):
This part feels wrong - a branch effect is not a reactive context. E.g. if I have derived A which reads derived B which is disconnected and read A only at the top level of a script tag I think connecting it would lead to memory leaks because we can never clean up the reaction B.

Code after:

			// if the derived is dirty and has reactions, or depends on the values that just changed, re-execute
			// (a derived can be maybe_dirty due to the effect destroy removing its last reaction)
			if (
				((derived.f & CLEAN) === 0 && derived.reactions !== null) ||
				depends_on_old_values(derived)
			) {
				value = execute_derived(derived);
			}

			old_values.set(derived, value);

			return value;
		}

		// connect disconnected deriveds if we are reading them inside an effect,
		// or inside another derived that is already connected
		var should_connect =
			(derived.f & CONNECTED) === 0 &&
			!untracking &&
			active_reaction !== null &&
			(is_updating_effect || (active_reaction.f & CONNECTED) !== 0);

		var is_new = derived.deps === null;

		if (is_dirty(derived)) {
			if (should_connect) {
				// set the flag before `update_derived`, so that the derived
				// is added as a reaction to its dependencies
				derived.f |= CONNECTED;
			}

			update_derived(derived);
		}

		if (should_connect && !is_new) {
			reconnect(derived);
		}
	}

	if (batch_values?.has(signal)) {
		return batch_values.get(signal);
	}

	if ((signal.f & ERROR_VALUE) !== 0) {
		throw signal.v;
	}

	return signal.v;
}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/2-analyze/visitors/shared/a11y/index.js (JavaScript)

Code before:
	return value;
}

/**
 * @param {AST.Attribute | undefined} attribute
 */
function get_static_value(attribute) {
	if (!attribute) return null;
	if (attribute.value === true) return true;
	if (is_text_attribute(attribute)) return attribute.value[0].data;
	return null;
}

/**
 * @param {AST.RegularElement | AST.SvelteElement} element
 */
function has_content(element) {
	for (const node of element.fragment.nodes) {
		if (node.type === 'Text') {
			if (node.data.trim() === '') {
				continue;
			}
		}

		if (node.type === 'RegularElement' || node.type === 'SvelteElement') {
			// FIX START: Ignore elements with the popover attribute
			if (node.attributes.some((a) => a.type === 'Attribute' && a.name === 'popover')) {
				continue;
			}
			// FIX END

			if (
				node.name === 'img' &&
				node.attributes.some((node) => node.type === 'Attribute' && node.name === 'alt')
			) {
				return true;
			}

			if (!has_content(node)) {
				continue;
			}
		}

		// assume everything else has content — this will result in false positives
		// (e.g. an empty `{#if ...}{/if}`) but that's probably fine
		return true;
	}
}

/**
 * @param {ARIARoleRelationConcept} schema

Review comment (suggestion):
you don't need to include things like this in PRs! it's obvious from the diff where it starts while you're reviewing, and once it's merged it's just a weird floating comment with no context


```suggestion
```

Code after:
	return value;
}

/**
 * @param {AST.Attribute | undefined} attribute
 */
function get_static_value(attribute) {
	if (!attribute) return null;
	if (attribute.value === true) return true;
	if (is_text_attribute(attribute)) return attribute.value[0].data;
	return null;
}

/**
 * @param {AST.RegularElement | AST.SvelteElement} element
 */
function has_content(element) {
	for (const node of element.fragment.nodes) {
		if (node.type === 'Text') {
			if (node.data.trim() === '') {
				continue;
			}
		}

		if (node.type === 'RegularElement' || node.type === 'SvelteElement') {
			if (node.attributes.some((a) => a.type === 'Attribute' && a.name === 'popover')) {
				continue;
			}

			if (
				node.name === 'img' &&
				node.attributes.some((node) => node.type === 'Attribute' && node.name === 'alt')
			) {
				return true;
			}

			if (!has_content(node)) {
				continue;
			}
		}

		// assume everything else has content — this will result in false positives
		// (e.g. an empty `{#if ...}{/if}`) but that's probably fine
		return true;
	}
}

/**
 * @param {ARIARoleRelationConcept} schema
 * @param {string} tag_name
 * @param {Map<string, AST.Attribute>} attribute_map
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/elements/customizable-select.js (JavaScript)

Code before:
 */
function is_supported() {
	if (supported === null) {
		var select = document.createElement('select');
		select.innerHTML = '<option><span>t</span></option>';
		supported = /** @type {Element} */ (select.firstChild)?.firstChild?.nodeType === 1;
	}

	return supported;
}

/**
 * @param {Node} node
 * @param {string} tag_name
 */
function find_in_path(node, tag_name) {
	/**
	 * @type {HTMLElement | undefined | null}
	 */
	let parent = /** @type {HTMLElement} */ (node);
	while (parent && parent.tagName !== tag_name) {
		parent = parent.parentElement;
	}
	return parent;
}

/**
 *
 * @param {HTMLElement} element
 * @param {(new_element: HTMLElement) => void} update_element
 */
export function selectedcontent(element, update_element) {
	// if it's not supported no need for special logic
	if (!is_supported()) return;

	// we use the attach function directly just to make sure is executed when is mounted to the dom
	attach(element, () => () => {
		const select = /** @type {HTMLSelectElement | null} */ (find_in_path(element, 'SELECT'));
		if (select) {
			const observer = new MutationObserver((entries) => {
				// if the changes include SELECTEDCONTENT we don't need to do anything as it already changed
				if (
					entries.some(
						(el) => el.target instanceof HTMLElement && el.target.tagName === 'SELECTEDCONTENT'
					)
				) {
					return;
				}
				// if the changes doesn't include the selected options we don't need to do anything
				if (
					!entries.find((e) => {

Review comment (suggestion):
we can use `element.closest()`, no?

```suggestion
```

Code after:
	if (supported === null) {
		var select = document.createElement('select');
		select.innerHTML = '<option><span>t</span></option>';
		supported = /** @type {Element} */ (select.firstChild)?.firstChild?.nodeType === 1;
	}

	return supported;
}

/**
 *
 * @param {HTMLElement} element
 * @param {(new_element: HTMLElement) => void} update_element
 */
export function selectedcontent(element, update_element) {
	// if it's not supported no need for special logic
	if (!is_supported()) return;

	// we use the attach function directly just to make sure is executed when is mounted to the dom
	attach(element, () => () => {
		const select = element.closest('select');
		if (!select) return;

		const observer = new MutationObserver((entries) => {
			var selected = false;

			for (const entry of entries) {
				if (entry.target === element) {
					// the `<selectedcontent>` already changed, no need to replace it
					return;
				}

				// if the changes doesn't include the selected `<option>` we don't need to do anything
				selected ||= !!entry.target.parentElement?.closest('option')?.selected;
			}

			if (selected) {
				// replace the `<selectedcontent>` with a clone
				element.replaceWith((element = /** @type {HTMLElement} */ (element.cloneNode(true))));
				update_element(element);
			}
		});

		observer.observe(select, {
			childList: true,
			characterData: true,
			subtree: true
		});

		return () => {
			observer.disconnect();
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/elements/customizable-select.js (JavaScript)

Code before:
 * @param {HTMLElement} element
 * @param {(new_element: HTMLElement) => void} update_element
 */
export function selectedcontent(element, update_element) {
	// if it's not supported no need for special logic
	if (!is_supported()) return;

	// we use the attach function directly just to make sure is executed when is mounted to the dom
	attach(element, () => () => {
		const select = /** @type {HTMLSelectElement | null} */ (find_in_path(element, 'SELECT'));
		if (select) {
			const observer = new MutationObserver((entries) => {
				// if the changes include SELECTEDCONTENT we don't need to do anything as it already changed
				if (
					entries.some(
						(el) => el.target instanceof HTMLElement && el.target.tagName === 'SELECTEDCONTENT'
					)
				) {
					return;
				}
				// if the changes doesn't include the selected options we don't need to do anything
				if (
					!entries.find((e) => {
						const option = /** @type {HTMLOptionElement | null} */ (
							find_in_path(e.target, 'OPTION')
						);
						return option && [...select.selectedOptions].includes(option);
					})
				) {
					return;
				}
				// otherwise we replace selectedcontent with a new element to trigger the browser
				// reclone of the selected option
				element.replaceWith((element = /** @type {HTMLElement} */ (element.cloneNode(true))));
				update_element(element);
			});

			observer.observe(select, {
				childList: true,
				characterData: true,
				subtree: true
			});

			teardown(() => {
				observer.disconnect();
			});
		}
	});
}

/**

Review comment (suggestion):
```suggestion
						const option = e.target.parentElement?.closest('option');
```

Code after:
					// the `<selectedcontent>` already changed, no need to replace it
					return;
				}

				// if the changes doesn't include the selected `<option>` we don't need to do anything
				selected ||= !!entry.target.parentElement?.closest('option')?.selected;
			}

			if (selected) {
				// replace the `<selectedcontent>` with a clone
				element.replaceWith((element = /** @type {HTMLElement} */ (element.cloneNode(true))));
				update_element(element);
			}
		});

		observer.observe(select, {
			childList: true,
			characterData: true,
			subtree: true
		});

		return () => {
			observer.disconnect();
		};
	});
}

/**
 * Handles rich HTML content inside `<option>`, `<optgroup>`, or `<select>` elements with browser-specific branching.
 * Modern browsers preserve HTML inside options, while older browsers strip it to text only.
 *
 * @param {HTMLOptionElement | HTMLOptGroupElement | HTMLSelectElement} element The element to process
 * @param {() => void} rich_fn Function to process rich HTML content (modern browsers)
 */
export function customizable_select(element, rich_fn) {
	var was_hydrating = hydrating;

	if (!is_supported()) {
		set_hydrating(false);
		element.textContent = '';
		element.append(create_comment(''));
	}

	try {
		rich_fn();
	} finally {
		if (was_hydrating) {
			if (hydrating) {
				reset(element);
			} else {
				set_hydrating(true);
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/elements/customizable-select.js (JavaScript)

Code before:
				// if the changes doesn't include the selected options we don't need to do anything
				if (
					!entries.find((e) => {
						const option = /** @type {HTMLOptionElement | null} */ (
							find_in_path(e.target, 'OPTION')
						);
						return option && [...select.selectedOptions].includes(option);
					})
				) {
					return;
				}
				// otherwise we replace selectedcontent with a new element to trigger the browser
				// reclone of the selected option
				element.replaceWith((element = /** @type {HTMLElement} */ (element.cloneNode(true))));
				update_element(element);
			});

			observer.observe(select, {
				childList: true,
				characterData: true,
				subtree: true
			});

			teardown(() => {
				observer.disconnect();
			});
		}
	});
}

/**
 * Handles rich HTML content inside `<option>`, `<optgroup>`, or `<select>` elements with browser-specific branching.
 * Modern browsers preserve HTML inside options, while older browsers strip it to text only.
 *
 * @param {HTMLOptionElement | HTMLOptGroupElement | HTMLSelectElement} element The element to process
 * @param {() => void} rich_fn Function to process rich HTML content (modern browsers)
 */
export function customizable_select(element, rich_fn) {
	var was_hydrating = hydrating;

	if (!is_supported()) {
		set_hydrating(false);
		element.textContent = '';
		element.append(create_comment(''));
	}

	try {
		rich_fn();
	} finally {
		if (was_hydrating) {
			if (hydrating) {

Review comment (suggestion):
```suggestion
			return () => {
				observer.disconnect();
			};
```

Code after:

		return () => {
			observer.disconnect();
		};
	});
}

/**
 * Handles rich HTML content inside `<option>`, `<optgroup>`, or `<select>` elements with browser-specific branching.
 * Modern browsers preserve HTML inside options, while older browsers strip it to text only.
 *
 * @param {HTMLOptionElement | HTMLOptGroupElement | HTMLSelectElement} element The element to process
 * @param {() => void} rich_fn Function to process rich HTML content (modern browsers)
 */
export function customizable_select(element, rich_fn) {
	var was_hydrating = hydrating;

	if (!is_supported()) {
		set_hydrating(false);
		element.textContent = '';
		element.append(create_comment(''));
	}

	try {
		rich_fn();
	} finally {
		if (was_hydrating) {
			if (hydrating) {
				reset(element);
			} else {
				set_hydrating(true);
				set_hydrate_node(element);
			}
		}
	}
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/shared/validate.js (JavaScript)

Code before:
import { is_void } from '../../utils.js';
import * as w from './warnings.js';
import * as e from './errors.js';

export { invalid_default_snippet } from './errors.js';

/**
 * @param {() => string} tag_fn
 * @returns {void}
 */
export function validate_void_dynamic_element(tag_fn) {
	const tag = tag_fn();
	if (tag && typeof tag === 'string' && is_void(tag)) {
		w.dynamic_void_element_content(tag);
	}
}

/** @param {() => unknown} tag_fn */
export function validate_dynamic_element_tag(tag_fn) {
	const tag = tag_fn();
	const is_string = typeof tag === 'string';
	if (tag && !is_string) {
		e.svelte_element_invalid_this_value();
	}
}

/**
 * @param {any} store
 * @param {string} name
 */
export function validate_store(store, name) {
	if (store != null && typeof store.subscribe !== 'function') {
		e.store_invalid_shape(name);
	}
}

/**
 * @template {(...args: any[]) => unknown} T
 * @param {T} fn
 */

Review comment (suggestion):
```suggestion
	if (tag) {
		if (typeof tag !== 'string') {
			e.svelte_element_invalid_this_value();
		} else if (is_void(tag)) {
			w.dynamic_void_element_content(tag);
		}
	}
```

Code after:
import { is_void } from '../../utils.js';
import * as w from './warnings.js';
import * as e from './errors.js';

export { invalid_default_snippet } from './errors.js';

/**
 * @param {() => string} tag_fn
 * @returns {void}
 */
export function validate_void_dynamic_element(tag_fn) {
	const tag = tag_fn();
	if (tag && is_void(tag)) {
		w.dynamic_void_element_content(tag);
	}
}

/** @param {() => unknown} tag_fn */
export function validate_dynamic_element_tag(tag_fn) {
	const tag = tag_fn();
	const is_string = typeof tag === 'string';
	if (tag && !is_string) {
		e.svelte_element_invalid_this_value();
	}
}

/**
 * @param {any} store
 * @param {string} name
 */
export function validate_store(store, name) {
	if (store != null && typeof store.subscribe !== 'function') {
		e.store_invalid_shape(name);
	}
}

/**
 * @template {(...args: any[]) => unknown} T
 * @param {T} fn
 */
Repository: sveltejs/svelte
File: packages/svelte/src/internal/shared/validate.js (JavaScript)

Code before:
import { is_void } from '../../utils.js';
import * as w from './warnings.js';
import * as e from './errors.js';

export { invalid_default_snippet } from './errors.js';

/**
 * @param {() => string} tag_fn
 * @returns {void}
 */
export function validate_void_dynamic_element(tag_fn) {
	const tag = tag_fn();
	if (tag) {
		if (typeof tag !== 'string') {
			e.svelte_element_invalid_this_value();
		} else if (is_void(tag)) {
			w.dynamic_void_element_content(tag);
		}
	}
}

/** @param {() => unknown} tag_fn */
export function validate_dynamic_element_tag(tag_fn) {
	const tag = tag_fn();
	const is_string = typeof tag === 'string';
	if (tag && !is_string) {
		e.svelte_element_invalid_this_value();
	}
}

/**
 * @param {any} store
 * @param {string} name
 */
export function validate_store(store, name) {
	if (store != null && typeof store.subscribe !== 'function') {
		e.store_invalid_shape(name);
	}
}

/**
 * @template {(...args: any[]) => unknown} T
 * @param {T} fn

Review comment (suggestion):
Instead of this change, we should just swap the order of inserting these statements:
https://github.com/sveltejs/svelte/blob/c9ebd6a88553a3041d68c6063ec095fdd601f4b7/packages/svelte/src/compiler/phases/3-transform/server/visitors/SvelteElement.js#L32-L35
and
https://github.com/sveltejs/svelte/blob/c9ebd6a88553a3041d68c6063ec095fdd601f4b7/packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteElement.js#L120-L123
So we firstly check whether the tag is valid and then optionally whether it is a void tag.

Code after:
import { is_void } from '../../utils.js';
import * as w from './warnings.js';
import * as e from './errors.js';

export { invalid_default_snippet } from './errors.js';

/**
 * @param {() => string} tag_fn
 * @returns {void}
 */
export function validate_void_dynamic_element(tag_fn) {
	const tag = tag_fn();
	if (tag && is_void(tag)) {
		w.dynamic_void_element_content(tag);
	}
}

/** @param {() => unknown} tag_fn */
export function validate_dynamic_element_tag(tag_fn) {
	const tag = tag_fn();
	const is_string = typeof tag === 'string';
	if (tag && !is_string) {
		e.svelte_element_invalid_this_value();
	}
}

/**
 * @param {any} store
 * @param {string} name
 */
export function validate_store(store, name) {
	if (store != null && typeof store.subscribe !== 'function') {
		e.store_invalid_shape(name);
	}
}

/**
 * @template {(...args: any[]) => unknown} T
 * @param {T} fn
 */
export function prevent_snippet_stringification(fn) {
	fn.toString = () => {
		e.snippet_without_render_tag();