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

Code before:

	if (debug) {
		var previous_captured_signals = captured_signals;
		var captured = new Set();
		set_captured_signals(captured);
		try {
			untrack(() => {
				debug();
			});
		} finally {
			set_captured_signals(previous_captured_signals);
		}
		if (captured.size > 0) {
			for (const dep of captured) {
				log_entry(dep);
			}
			return;
		}
	}

	const type = (signal.f & DERIVED) !== 0 ? '$derived' : '$state';
	const current_reaction = /** @type {Reaction} */ (active_reaction);
	const status =
		signal.w_version > current_reaction.w_version || current_reaction.w_version === 0
			? 'dirty'
			: 'clean';

	// eslint-disable-next-line no-console
	console.groupCollapsed(
		`%c${type}`,
		status !== 'clean'
			? 'color: CornflowerBlue; font-weight: bold'
			: 'color: grey; font-weight: bold',
		typeof value === 'object' && value !== null && STATE_SYMBOL in value
			? snapshot(value, true)
			: value
	);

	if (type === '$derived') {
		const deps = new Set(/** @type {Derived} */ (signal).deps);
		for (const dep of deps) {
			log_entry(dep);
		}
	}

	if (signal.created) {
		// eslint-disable-next-line no-console
		console.log(signal.created);
	}

	if (signal.updated) {

Review comment (suggestion):
unrelated to the PR but I hadn't noticed this code before — any reason we're using an enum here rather than a boolean? Feels like it could be this instead, with a corresponding change on line 53:

```js
const dirty = signal.w_version > current_reaction.w_version || current_reaction.w_version === 0;
```

Code after:

	if (debug) {
		var previous_captured_signals = captured_signals;
		var captured = new Set();
		set_captured_signals(captured);
		try {
			untrack(() => {
				debug();
			});
		} finally {
			set_captured_signals(previous_captured_signals);
		}
		if (captured.size > 0) {
			for (const dep of captured) {
				log_entry(dep);
			}
			return;
		}
	}

	const type = (signal.f & DERIVED) !== 0 ? '$derived' : '$state';
	const current_reaction = /** @type {Reaction} */ (active_reaction);
	const dirty = signal.wv > current_reaction.wv || current_reaction.wv === 0;

	// eslint-disable-next-line no-console
	console.groupCollapsed(
		`%c${type}`,
		dirty ? 'color: CornflowerBlue; font-weight: bold' : 'color: grey; font-weight: bold',
		typeof value === 'object' && value !== null && STATE_SYMBOL in value
			? snapshot(value, true)
			: value
	);

	if (type === '$derived') {
		const deps = new Set(/** @type {Derived} */ (signal).deps);
		for (const dep of deps) {
			log_entry(dep);
		}
	}

	if (signal.created) {
		// eslint-disable-next-line no-console
		console.log(signal.created);
	}

	if (signal.updated) {
		// eslint-disable-next-line no-console
		console.log(signal.updated);
	}

	const read = entry?.read;
Repository: sveltejs/svelte
File: packages/svelte/elements.d.ts (TypeScript)

Code before:
								{
									attribute?: string;
									reflect?: boolean;
									type?: 'String' | 'Boolean' | 'Number' | 'Array' | 'Object';
								}
						  >
						| undefined;
					extend?: (
						svelteCustomElementClass: new () => HTMLElement
					) => new () => HTMLElement | undefined;
			  };
		immutable?: boolean | undefined;
		accessors?: boolean | undefined;
		namespace?: string | undefined;
		[name: string]: any;
	};
	'svelte:head': { [name: string]: any };
	'svelte:boundary': {
		onerror?: (error: unknown, reset: () => void) => void;
		failed?: import('svelte').Snippet<[error: unknown, reset: () => void]>;
	};

	[name: string]: { [name: string]: any };
}

export type ClassValue = import('clsx').ClassArray | import('clsx').ClassDictionary;


Review comment (suggestion):
Originally I wanted to export `import('clsx').ClassValue` itself but considering this file only uses the two `ClassDictionary` and `ClassArray` types, I kept those intact

Code after:
								{
									attribute?: string;
									reflect?: boolean;
									type?: 'String' | 'Boolean' | 'Number' | 'Array' | 'Object';
								}
						  >
						| undefined;
					extend?: (
						svelteCustomElementClass: new () => HTMLElement
					) => new () => HTMLElement | undefined;
			  };
		immutable?: boolean | undefined;
		accessors?: boolean | undefined;
		namespace?: string | undefined;
		[name: string]: any;
	};
	'svelte:head': { [name: string]: any };
	'svelte:boundary': {
		onerror?: (error: unknown, reset: () => void) => void;
		failed?: import('svelte').Snippet<[error: unknown, reset: () => void]>;
	};

	[name: string]: { [name: string]: any };
}

export type ClassValue = string | import('clsx').ClassArray | import('clsx').ClassDictionary;
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/2-analyze/css/css-prune.js (JavaScript)

Code before:
			nodes = node.fragment.nodes;
			break;
		}

		if (node.type !== 'Fragment') {
			start = node;
		}
	}

	/** @type {Array<Compiler.AST.RegularElement | Compiler.AST.SvelteElement>} */
	const siblings = [];

	// ...then walk them, starting from the node containing the element in question
	// skipping nodes that appears before the element

	const seen = new Set();
	let skip = true;

	/** @param {Compiler.AST.SvelteNode} node */
	function get_siblings(node) {
		walk(node, null, {
			RegularElement(node) {
				if (node === element) {
					skip = false;
					if (include_self) siblings.push(node);
				} else if (!skip) siblings.push(node);
			},
			SvelteElement(node) {
				if (node === element) {
					skip = false;
					if (include_self) siblings.push(node);
				} else if (!skip) siblings.push(node);
			},
			RenderTag(node) {
				for (const snippet of node.metadata.snippets) {
					if (seen.has(snippet)) continue;

					seen.add(snippet);
					get_siblings(snippet.body);
				}
			}
		});
	}

	for (const node of nodes.slice(nodes.indexOf(start))) {
		get_siblings(node);
	}

	return siblings;
}


Review comment (suggestion):
```suggestion
				} else if (!skip) {
					siblings.push(node);
				}
```

Code after:
			nodes = node.fragment.nodes;
			break;
		}

		if (node.type !== 'Fragment') {
			start = node;
		}
	}

	/** @type {Array<Compiler.AST.RegularElement | Compiler.AST.SvelteElement>} */
	const siblings = [];

	// ...then walk them, starting from the node containing the element in question
	// skipping nodes that appears before the element

	const seen = new Set();
	let skip = true;

	/** @param {Compiler.AST.SvelteNode} node */
	function get_siblings(node) {
		walk(node, null, {
			RegularElement(node) {
				if (node === element) {
					skip = false;
					if (include_self) siblings.push(node);
				} else if (!skip) {
					siblings.push(node);
				}
			},
			SvelteElement(node) {
				if (node === element) {
					skip = false;
					if (include_self) siblings.push(node);
				} else if (!skip) {
					siblings.push(node);
				}
			},
			RenderTag(node) {
				for (const snippet of node.metadata.snippets) {
					if (seen.has(snippet)) continue;

					seen.add(snippet);
					get_siblings(snippet.body);
				}
			}
		});
	}

	for (const node of nodes.slice(nodes.indexOf(start))) {
		get_siblings(node);
	}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/2-analyze/css/css-prune.js (JavaScript)

Code before:
		}
	}

	/** @type {Array<Compiler.AST.RegularElement | Compiler.AST.SvelteElement>} */
	const siblings = [];

	// ...then walk them, starting from the node containing the element in question
	// skipping nodes that appears before the element

	const seen = new Set();
	let skip = true;

	/** @param {Compiler.AST.SvelteNode} node */
	function get_siblings(node) {
		walk(node, null, {
			RegularElement(node) {
				if (node === element) {
					skip = false;
					if (include_self) siblings.push(node);
				} else if (!skip) siblings.push(node);
			},
			SvelteElement(node) {
				if (node === element) {
					skip = false;
					if (include_self) siblings.push(node);
				} else if (!skip) siblings.push(node);
			},
			RenderTag(node) {
				for (const snippet of node.metadata.snippets) {
					if (seen.has(snippet)) continue;

					seen.add(snippet);
					get_siblings(snippet.body);
				}
			}
		});
	}

	for (const node of nodes.slice(nodes.indexOf(start))) {
		get_siblings(node);
	}

	return siblings;
}

/**
 * @param {any} operator
 * @param {any} expected_value
 * @param {any} case_insensitive
 * @param {any} value
 */

Review comment (suggestion):
```suggestion
				} else if (!skip) {
					siblings.push(node);
				}
```

Code after:
		}
	}

	/** @type {Array<Compiler.AST.RegularElement | Compiler.AST.SvelteElement>} */
	const siblings = [];

	// ...then walk them, starting from the node containing the element in question
	// skipping nodes that appears before the element

	const seen = new Set();
	let skip = true;

	/** @param {Compiler.AST.SvelteNode} node */
	function get_siblings(node) {
		walk(node, null, {
			RegularElement(node) {
				if (node === element) {
					skip = false;
					if (include_self) siblings.push(node);
				} else if (!skip) {
					siblings.push(node);
				}
			},
			SvelteElement(node) {
				if (node === element) {
					skip = false;
					if (include_self) siblings.push(node);
				} else if (!skip) {
					siblings.push(node);
				}
			},
			RenderTag(node) {
				for (const snippet of node.metadata.snippets) {
					if (seen.has(snippet)) continue;

					seen.add(snippet);
					get_siblings(snippet.body);
				}
			}
		});
	}

	for (const node of nodes.slice(nodes.indexOf(start))) {
		get_siblings(node);
	}

	return siblings;
}

/**
 * @param {any} operator
Repository: sveltejs/svelte
File: packages/svelte/tests/runtime-runes/samples/effect-tracking-unowned/main.svelte (Svelte)

Code before:
<script>
	import { writable, fromStore, toStore } from "svelte/store";

	const store = writable("previous");
	let text = $derived(fromStore(store).current + " message");

	const textStore = toStore(() => text);
</script>

<o>Store: {$store}</o>
<p>Text: {text}</p>
<button onclick={() => { store.set("new"); }}>Change Store</button>


Review comment (refactor):
We can simplify this further

```suggestion
	text; // read derived in a non-tracking context
```

Code after:
<script>
	import { writable, fromStore, toStore } from "svelte/store";

	const store = writable("previous");
	let text = $derived(fromStore(store).current + " message");

	text; // read derived in a non-tracking context
</script>

<o>Store: {$store}</o>
<p>Text: {text}</p>
<button onclick={() => { store.set("new"); }}>Change Store</button>
Repository: sveltejs/svelte
File: packages/svelte/tests/runtime-runes/samples/derived-unowned-12/main.svelte (Svelte)

Code before:
<script>
	import { untrack } from 'svelte';

	let count = $state(0);
	let state = $state({current: count});

	let linked = $derived.by(() => {
		count;

		untrack(() => state.current = count);
	  return untrack(() => state);
	});

	linked.current++;
</script>

<button onclick={() => linked.current++}>linked.current</button> {linked.current}
<button onclick={() => count++}>count</button> {count}


Review comment (suggestion):
```suggestion
		untrack(() => state.current = count);
		return untrack(() => state);
```

Code after:
<script>
	import { untrack } from 'svelte';

	let count = $state(0);
	let state = $state({current: count});

	let linked = $derived.by(() => {
		count;

		untrack(() => state.current = count);
		return untrack(() => state);
	});

	linked.current++;
</script>

<button onclick={() => linked.current++}>linked.current</button> {linked.current}
<button onclick={() => count++}>count</button> {count}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/elements/attributes.js (JavaScript)

Code before:
				}
			} else if (
				is_default ||
				(setters.includes(name) && (is_custom_element || typeof value !== 'string'))
			) {
				// @ts-ignore
				element[name] = value;
			} else if (typeof value !== 'function') {
				set_attribute(element, name, value);
			}
		}
		if (key === 'style' && '__styles' in element) {
			// reset styles to force style: directive to update
			element.__styles = {};
		}
	}

	if (is_hydrating_custom_element) {
		set_hydrating(true);
	}

	return current;
}

/** @type {Map<string, string[]>} */
var setters_cache = new Map();

/** @param {Element} element */
function get_setters(element) {
	var setters = setters_cache.get(element.nodeName);
	if (setters) return setters;
	setters_cache.set(element.nodeName, (setters = []));

	var descriptors;
	var proto = element; // In the case of custom elements there might be setters on the instance
	var element_proto = Element.prototype;

	// Stop at Element, from there on there's only unnecessary setters we're not interested in
	// Do not use contructor.name here as that's unreliable in some browser environments
	while (element_proto !== proto) {
		descriptors = get_descriptors(proto);

		for (var key in descriptors) {
			if (descriptors[key].set) {
				setters.push(key);
			}
		}

		proto = get_prototype_of(proto);
	}


Review comment (suggestion):
It would be good to get an idea from our website or something how many times this is normally called. If it's a lot then we probably want to avoid it for the simple element case – which is the vast majority of elements.

Code after:
			} else if (typeof value !== 'function') {
				set_attribute(element, name, value, skip_warning);
			}
		}
		if (key === 'style' && '__styles' in element) {
			// reset styles to force style: directive to update
			element.__styles = {};
		}
	}

	if (is_hydrating_custom_element) {
		set_hydrating(true);
	}

	return current;
}

/**
 *
 * @param {Element} element
 */
function get_attributes(element) {
	return /** @type {Record<string | symbol, unknown>} **/ (
		// @ts-expect-error
		element.__attributes ??= {
			[IS_CUSTOM_ELEMENT]: element.nodeName.includes('-'),
			[IS_HTML]: element.namespaceURI === NAMESPACE_HTML
		}
	);
}

/** @type {Map<string, string[]>} */
var setters_cache = new Map();

/** @param {Element} element */
function get_setters(element) {
	var setters = setters_cache.get(element.nodeName);
	if (setters) return setters;
	setters_cache.set(element.nodeName, (setters = []));

	var descriptors;
	var proto = element; // In the case of custom elements there might be setters on the instance
	var element_proto = Element.prototype;

	// Stop at Element, from there on there's only unnecessary setters we're not interested in
	// Do not use contructor.name here as that's unreliable in some browser environments
	while (element_proto !== proto) {
		descriptors = get_descriptors(proto);

		for (var key in descriptors) {
			if (descriptors[key].set) {
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/props.js (JavaScript)

Code before:
	return function (/** @type {any} */ value, /** @type {boolean} */ mutation) {
		// legacy nonsense — need to ensure the source is invalidated when necessary
		// also needed for when handling inspect logic so we can inspect the correct source signal
		if (captured_signals !== null) {
			// set this so that we don't reset to the parent value if `d`
			// is invalidated because of `invalidate_inner_signals` (rather
			// than because the parent or child value changed)
			from_child = was_from_child;
			// invoke getters so that signals are picked up by `invalidate_inner_signals`
			getter();
			get(inner_current_value);
		}

		if (arguments.length > 0) {
			const new_value = mutation ? get(current_value) : runes && bindable ? proxy(value) : value;

			if (!current_value.equals(new_value)) {
				from_child = true;
				set(inner_current_value, new_value);
				// To ensure the fallback value is consistent when used with proxies, we
				// update the local fallback_value, but only if the fallback is actively used
				if (fallback_used && fallback_value !== undefined) {
					fallback_value = new_value;
				}

				if (has_destoyed_component_ctx(current_value)) {
					return value;
				}

				untrack(() => get(current_value)); // force a synchronisation immediately
			}

			return value;
		}

		if (has_destoyed_component_ctx(current_value)) {
			return current_value.v;
		}

		return get(current_value);
	};
}


Review comment (suggestion):
```suggestion
				if (has_destroyed_component_ctx(current_value)) {
```

Code after:
			// than because the parent or child value changed)
			from_child = was_from_child;
			// invoke getters so that signals are picked up by `invalidate_inner_signals`
			getter();
			get(inner_current_value);
		}

		if (arguments.length > 0) {
			const new_value = mutation ? get(current_value) : runes && bindable ? proxy(value) : value;

			if (!current_value.equals(new_value)) {
				from_child = true;
				set(inner_current_value, new_value);
				// To ensure the fallback value is consistent when used with proxies, we
				// update the local fallback_value, but only if the fallback is actively used
				if (fallback_used && fallback_value !== undefined) {
					fallback_value = new_value;
				}

				if (has_destroyed_component_ctx(current_value)) {
					return value;
				}

				untrack(() => get(current_value)); // force a synchronisation immediately
			}

			return value;
		}

		if (has_destroyed_component_ctx(current_value)) {
			return current_value.v;
		}

		return get(current_value);
	};
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/props.js (JavaScript)

Code before:
			get(inner_current_value);
		}

		if (arguments.length > 0) {
			const new_value = mutation ? get(current_value) : runes && bindable ? proxy(value) : value;

			if (!current_value.equals(new_value)) {
				from_child = true;
				set(inner_current_value, new_value);
				// To ensure the fallback value is consistent when used with proxies, we
				// update the local fallback_value, but only if the fallback is actively used
				if (fallback_used && fallback_value !== undefined) {
					fallback_value = new_value;
				}

				if (has_destoyed_component_ctx(current_value)) {
					return value;
				}

				untrack(() => get(current_value)); // force a synchronisation immediately
			}

			return value;
		}

		if (has_destoyed_component_ctx(current_value)) {
			return current_value.v;
		}

		return get(current_value);
	};
}


Review comment (suggestion):
```suggestion
		if (has_destroyed_component_ctx(current_value)) {
```

Code after:
			if (!current_value.equals(new_value)) {
				from_child = true;
				set(inner_current_value, new_value);
				// To ensure the fallback value is consistent when used with proxies, we
				// update the local fallback_value, but only if the fallback is actively used
				if (fallback_used && fallback_value !== undefined) {
					fallback_value = new_value;
				}

				if (has_destroyed_component_ctx(current_value)) {
					return value;
				}

				untrack(() => get(current_value)); // force a synchronisation immediately
			}

			return value;
		}

		if (has_destroyed_component_ctx(current_value)) {
			return current_value.v;
		}

		return get(current_value);
	};
}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/scope.js (JavaScript)

Code before:
					case '**':
					case '-':
					case '/':
					case '<<':
					case '>>':
					case '>>>':
					case '^':
					case '|':
						this.values.add(NUMBER);
						break;

					case '+':
						if (a.is_string || b.is_string) {
							this.values.add(STRING);
						} else if (a.is_number && b.is_number) {
							this.values.add(NUMBER);
						} else {
							this.values.add(STRING);
							this.values.add(NUMBER);
						}
						break;

					default:
						// @ts-expect-error we can't guard against future operators without
						// TypeScript getting confused
						throw new Error(`Unknown operator ${expression.operator}`);
				}
				break;

			case 'ConditionalExpression':
				var test = scope.evaluate(expression.test);
				var consequent = scope.evaluate(expression.consequent);
				var alternate = scope.evaluate(expression.alternate);

				if (test.is_known) {
					for (const value of (test.value ? consequent : alternate).values) {
						this.values.add(value);
					}
				} else {
					for (const value of consequent.values) {
						this.values.add(value);
					}

					for (const value of alternate.values) {
						this.values.add(value);
					}
				}
				break;

			case 'LogicalExpression':
				a = scope.evaluate(expression.left);

Review comment (suggestion):
I'd rather have us warn here and add `UNKNOWN`, else people might be stuck with not using certain operators once they land if they can't upgrade to a version that supports it (people could be on v5 and we only add the operator in v6)

Code after:
					case '%':
					case '&':
					case '*':
					case '**':
					case '-':
					case '/':
					case '<<':
					case '>>':
					case '>>>':
					case '^':
					case '|':
						this.values.add(NUMBER);
						break;

					case '+':
						if (a.is_string || b.is_string) {
							this.values.add(STRING);
						} else if (a.is_number && b.is_number) {
							this.values.add(NUMBER);
						} else {
							this.values.add(STRING);
							this.values.add(NUMBER);
						}
						break;

					default:
						this.values.add(UNKNOWN);
				}
				break;
			}

			case 'ConditionalExpression': {
				const test = scope.evaluate(expression.test);
				const consequent = scope.evaluate(expression.consequent);
				const alternate = scope.evaluate(expression.alternate);

				if (test.is_known) {
					for (const value of (test.value ? consequent : alternate).values) {
						this.values.add(value);
					}
				} else {
					for (const value of consequent.values) {
						this.values.add(value);
					}

					for (const value of alternate.values) {
						this.values.add(value);
					}
				}
				break;
			}