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

Code before:

	const message_descriptor = get_descriptor(error, 'message');

	// if the message was already changed and it's not configurable we can't change it
	// or it will throw a different error swallowing the original one

	var indent = is_firefox ? '  ' : '\t';
	var component_stack = `\n${indent}in ${effect.fn?.name || '<unknown>'}`;
	var context = effect.ctx;

	while (context !== null) {
		component_stack += `\n${indent}in ${context.function?.[FILENAME].split('/').pop()}`;
		context = context.p;
	}

	if (!message_descriptor || message_descriptor.configurable) {
		define_property(error, 'message', {
			value: error.message + `\n${component_stack}\n`
		});
	} else {
		// eslint-disable-next-line no-console
		console.error(
			"The following it's not a separate error: we usually modify the error message to show you this information but `message` was non configurable so we print them in a separate log.\n" +
				error.message +
				`\n${component_stack}\n`
		);
	}

	if (error.stack) {
		// Filter out internal modules
		define_property(error, 'stack', {
			value: error.stack
				.split('\n')
				.filter((line) => !line.includes('svelte/src/internal'))
				.join('\n')
		});
	}
}


Review comment (suggestion):
This is ... a lot 😄 I would trim this down to just this, or just not log anything at all. I don't know how many people will have worse stacks as a result, but I doubt it's many
```suggestion
		console.error(
			"[adjusted stack] " +
				error.message +
				`\n${component_stack}\n`
		);
```

Code after:

	const message_descriptor = get_descriptor(error, 'message');

	// if the message was already changed and it's not configurable we can't change it
	// or it will throw a different error swallowing the original error
	if (message_descriptor && !message_descriptor.configurable) return;

	var indent = is_firefox ? '  ' : '\t';
	var component_stack = `\n${indent}in ${effect.fn?.name || '<unknown>'}`;
	var context = effect.ctx;

	while (context !== null) {
		component_stack += `\n${indent}in ${context.function?.[FILENAME].split('/').pop()}`;
		context = context.p;
	}

	define_property(error, 'message', {
		value: error.message + `\n${component_stack}\n`
	});

	if (error.stack) {
		// Filter out internal modules
		define_property(error, 'stack', {
			value: error.stack
				.split('\n')
				.filter((line) => !line.includes('svelte/src/internal'))
				.join('\n')
		});
	}
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/blocks/boundary.js (JavaScript)

Code before:
				if (did_reset) {
					w.svelte_boundary_reset_noop();
					return;
				}

				did_reset = true;

				if (calling_on_error) {
					w.reset_misuse();
					throw error;
				}

				pause_effect(boundary_effect);

				with_boundary(boundary, () => {
					is_creating_fallback = false;
					boundary_effect = branch(() => boundary_fn(anchor));
				});
			};

			var previous_reaction = active_reaction;

			try {
				set_active_reaction(null);
				calling_on_error = true;
				onerror?.(error, reset);
				calling_on_error = false;
			} finally {
				set_active_reaction(previous_reaction);
			}

			if (failed) {
				// Render the `failed` snippet in a microtask
				queue_micro_task(() => {
					with_boundary(boundary, () => {
						is_creating_fallback = true;

						try {
							boundary_effect = branch(() => {
								failed(
									anchor,
									() => error,
									() => reset
								);
							});
						} catch (error) {
							invoke_error_boundary(error, /** @type {Effect} */ (boundary.parent));
						}

						is_creating_fallback = false;
					});

Review comment (suggestion):
@Rich-Harris People might want to auto-fix the state of the application inside the onerror for instance they might decide to show an error snackbar and reset a form without any confirmation from the user

Code after:

	/**
	 * A source containing the number of pending async deriveds/expressions.
	 * Only created if `$effect.pending()` is used inside the boundary,
	 * otherwise updating the source results in needless `Batch.ensure()`
	 * calls followed by no-op flushes
	 * @type {Source<number> | null}
	 */
	#effect_pending = null;

	#effect_pending_update = () => {
		if (this.#effect_pending) {
			internal_set(this.#effect_pending, this.#pending_count);
		}
	};

	#effect_pending_subscriber = createSubscriber(() => {
		this.#effect_pending = source(this.#pending_count);

		if (DEV) {
			tag(this.#effect_pending, '$effect.pending()');
		}

		return () => {
			this.#effect_pending = null;
		};
	});

	/**
	 * @param {TemplateNode} node
	 * @param {BoundaryProps} props
	 * @param {((anchor: Node) => void)} children
	 */
	constructor(node, props, children) {
		this.#anchor = node;
		this.#props = props;
		this.#children = children;

		this.#hydrate_open = hydrate_node;

		this.parent = /** @type {Effect} */ (active_effect).b;

		this.pending = !!this.#props.pending;

		this.#effect = block(() => {
			/** @type {Effect} */ (active_effect).b = this;

			if (hydrating) {
				hydrate_next();
			}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/Component.js (JavaScript)

Code before:
/** @import { AST } from '#compiler' */
/** @import { ComponentContext } from '../types' */
import { regex_is_valid_identifier } from '../../../patterns.js';
import { build_component } from './shared/component.js';

/**
 * @param {AST.Component} node
 * @param {ComponentContext} context
 */
export function Component(node, context) {
	const component = build_component(
		node,
		// avoid shadowing the component variable by a variable used in $.component
		node.metadata.dynamic
			? '$$component_' + node.name.replaceAll(/[^a-zA-Z_$0-9]/g, '_')
			: node.name,
		context
	);
	context.state.init.push(component);
}


Review comment (suggestion):
Don't we need to keep the `regex_is_valid_identifier` test here for the `node.name` branch?

Also IMO we should cut this out and move it into its own variable -- it's getting hard to read

Code after:
/** @import { AST } from '#compiler' */
/** @import { ComponentContext } from '../types' */
import { regex_is_valid_identifier } from '../../../patterns.js';
import { build_component } from './shared/component.js';

/**
 * @param {AST.Component} node
 * @param {ComponentContext} context
 */
export function Component(node, context) {
	const component = build_component(node, node.name, context);
	context.state.init.push(component);
}
Repository: sveltejs/svelte
File: packages/svelte/src/reactivity/map.js (JavaScript)

Code before:
 *
 * @template K
 * @template V
 * @extends {Map<K, V>}
 */
export class SvelteMap extends Map {
	/** @type {Map<K, Source<number>>} */
	#sources = new Map();
	#version = state(0);
	#size = state(0);
	/**@type {Reaction | null} */
	#initial_reaction = null;

	/**
	 * @param {Iterable<readonly [K, V]> | null | undefined} [value]
	 */
	constructor(value) {
		super();

		if (active_reaction !== null) {
			this.#initial_reaction = active_reaction;
			// since we only need `initial_reaction` as long as we are in a derived/effect we can
			// safely create a teardown function that will reset it to null and allow for GC
			teardown(() => {
				this.#initial_reaction = null;
			});
		}

		if (DEV) {
			// If the value is invalid then the native exception will fire here
			value = new Map(value);

			tag(this.#version, 'SvelteMap version');
			tag(this.#size, 'SvelteMap.size');
		}

		if (value) {
			for (var [key, v] of value) {
				super.set(key, v);
			}
			this.#size.v = super.size;
		}
	}

	/**
	 * If the active_reaction is the same as the reaction that created this SvelteMap,
	 * we use state so that it will not be a dependency of the reaction. Otherwise we
	 * use source so it will be.
	 *
	 * @template T
	 * @param {T} value

Review comment (performance):
presumably if there's a situation that would create a memory leak with this (which I guess means smuggling a reference to the map to something outside the current reaction?), `teardown` only mitigates it, since the reaction in question might never _be_ torn down, if it's an unowned derived.

do we therefore need a way to run code as soon as the current reaction has finished updating? #16316

Code after:
 * @template K
 * @template V
 * @extends {Map<K, V>}
 */
export class SvelteMap extends Map {
	/** @type {Map<K, Source<number>>} */
	#sources = new Map();
	#version = state(0);
	#size = state(0);
	#update_version = update_version || -1;

	/**
	 * @param {Iterable<readonly [K, V]> | null | undefined} [value]
	 */
	constructor(value) {
		super();

		if (DEV) {
			// If the value is invalid then the native exception will fire here
			value = new Map(value);

			tag(this.#version, 'SvelteMap version');
			tag(this.#size, 'SvelteMap.size');
		}

		if (value) {
			for (var [key, v] of value) {
				super.set(key, v);
			}
			this.#size.v = super.size;
		}
	}

	/**
	 * If the source is being created inside the same reaction as the SvelteMap instance,
	 * we use `state` so that it will not be a dependency of the reaction. Otherwise we
	 * use `source` so it will be.
	 *
	 * @template T
	 * @param {T} value
	 * @returns {Source<T>}
	 */
	#source(value) {
		return update_version === this.#update_version ? state(value) : source(value);
	}

	/** @param {K} key */
	has(key) {
		var sources = this.#sources;
		var s = sources.get(key);
Repository: sveltejs/svelte
File: packages/svelte/tests/signals/test.ts (TypeScript)

Code before:
			assert.deepEqual(log, ['A:0:0', 'B:0', 'A:1:2', 'B:2', 'A:2:4', 'B:4']);
		};
	});

	test('multiple effects with state and derived in it #2', () => {
		const log: string[] = [];

		let count = state(0);
		let double = derived(() => $.get(count) * 2);

		effect(() => {
			log.push(`A:${$.get(double)}`);
		});
		effect(() => {
			log.push(`B:${$.get(count)}:${$.get(double)}`);
		});

		return () => {
			flushSync(() => set(count, 1));
			flushSync(() => set(count, 2));

			assert.deepEqual(log, ['A:0', 'B:0:0', 'A:2', 'B:1:2', 'A:4', 'B:2:4']);
		};
	});

	test('derived from state', () => {
		const log: number[] = [];

		let count = state(0);
		let double = derived(() => $.get(count) * 2);

		effect(() => {
			log.push($.get(double));
		});

		return () => {
			flushSync(() => set(count, 1));
			flushSync(() => set(count, 2));

			assert.deepEqual(log, [0, 2, 4]);
		};
	});

	test('derived from derived', () => {
		const log: number[] = [];

		let count = state(0);
		let double = derived(() => $.get(count) * 2);
		let quadruple = derived(() => $.get(double) * 2);

		effect(() => {

Review comment (suggestion):
Is the name of the test correct? Isn't the change that we actually add the derived as a reaction? I think it makes sense: the problem is probably that we check if the derived is `UNOWNED` before executing it so if the execution changes that (connecting it) when we reach the point of the code you changes `skip_reaction` should be false (because now the derived it's not unowned anymore) but it's still true and you can check that by looking if it has reactions (or i suppose we could also check if `(reaction.f & UNOWNED) !== 0` in line?)

Code after:
			assert.deepEqual(log, ['A:0:0', 'B:0', 'A:1:2', 'B:2', 'A:2:4', 'B:4']);
		};
	});

	test('multiple effects with state and derived in it #2', () => {
		const log: string[] = [];

		let count = state(0);
		let double = derived(() => $.get(count) * 2);

		effect(() => {
			log.push(`A:${$.get(double)}`);
		});
		effect(() => {
			log.push(`B:${$.get(count)}:${$.get(double)}`);
		});

		return () => {
			flushSync(() => set(count, 1));
			flushSync(() => set(count, 2));

			assert.deepEqual(log, ['A:0', 'B:0:0', 'A:2', 'B:1:2', 'A:4', 'B:2:4']);
		};
	});

	test('unowned deriveds are not added as reactions but trigger effects', () => {
		var obj = state<any>(undefined);

		class C1 {
			#v = state(0);
			get v() {
				return $.get(this.#v);
			}
			set v(v: number) {
				set(this.#v, v);
			}
		}

		return () => {
			let d = derived(() => $.get(obj)?.v || '-');

			const log: number[] = [];
			assert.equal($.get(d), '-');

			let destroy = effect_root(() => {
				render_effect(() => {
					log.push($.get(d));
				});
			});

			set(obj, new C1());
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/context.js (JavaScript)

Code before:
/** @param {ComponentContext | null} context */
export function set_component_context(context) {
	component_context = context;
}

/** @type {DevStackEntry | null} */
export let dev_stack = null;

/** @param {DevStackEntry | null} stack */
export function set_dev_stack(stack) {
	dev_stack = stack;
}

/**
 * Execute a callback with a new dev stack entry
 * @param {() => any} callback - Function to execute
 * @param {DevStackEntry['type']} type - Type of block/component
 * @param {string} file - Source file
 * @param {number} line - Line number
 * @param {number} column - Column number
 * @param {Record<string, any>} [additional] - Any additional properties to add to the dev stack entry
 * @returns {any}
 */
export function with_dev_stack(callback, type, file, line, column, additional) {
	/** @type {DevStackEntry} */
	const new_entry = {
		type,
		file,
		line,
		column,
		parent: dev_stack,
		...additional
	};
	const previous_stack = dev_stack;
	dev_stack = new_entry;

	try {
		return callback();
	} finally {
		dev_stack = previous_stack;
	}
}

/**
 * The current component function. Different from current component context:
 * ```html
 * <!-- App.svelte -->
 * <Foo>
 *   <Bar /> <!-- context == Foo.svelte, function == App.svelte -->
 * </Foo>
 * ```

Review comment (suggestion):
```suggestion
	const parent = dev_stack;

	dev_stack = {
```

Code after:

/** @param {ComponentContext | null} context */
export function set_component_context(context) {
	component_context = context;
}

/** @type {DevStackEntry | null} */
export let dev_stack = null;

/** @param {DevStackEntry | null} stack */
export function set_dev_stack(stack) {
	dev_stack = stack;
}

/**
 * Execute a callback with a new dev stack entry
 * @param {() => any} callback - Function to execute
 * @param {DevStackEntry['type']} type - Type of block/component
 * @param {any} component - Component function
 * @param {number} line - Line number
 * @param {number} column - Column number
 * @param {Record<string, any>} [additional] - Any additional properties to add to the dev stack entry
 * @returns {any}
 */
export function add_svelte_meta(callback, type, component, line, column, additional) {
	const parent = dev_stack;

	dev_stack = {
		type,
		file: component[FILENAME],
		line,
		column,
		parent,
		...additional
	};

	try {
		return callback();
	} finally {
		dev_stack = parent;
	}
}

/**
 * The current component function. Different from current component context:
 * ```html
 * <!-- App.svelte -->
 * <Foo>
 *   <Bar /> <!-- context == Foo.svelte, function == App.svelte -->
 * </Foo>
 * ```
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js (JavaScript)

Code before:
 * @param {{ start?: number }} node - AST node for location info
 * @param {'component' | 'if' | 'each' | 'await' | 'key'} type - Type of block/component
 * @param {Record<string, number | string>} [additional] - Any additional properties to add to the dev stack entry
 * @returns {Statement} - Statement with or without dev stack wrapping
 */
export function with_dev_stack(call_expression, node, type, additional) {
	if (!dev) {
		return b.stmt(call_expression);
	}

	const location = node.start && locator(node.start);
	if (!location) {
		return b.stmt(call_expression);
	}

	return b.stmt(
		b.call(
			'$.with_dev_stack',
			b.arrow([], b.block([b.stmt(call_expression)])),
			b.literal(type),
			b.literal(filename),
			b.literal(location.line),
			b.literal(location.column),
			additional
				? b.object(Object.entries(additional).map(([key, value]) => b.init(key, b.literal(value))))
				: b.null
		)
	);
}


Review comment (suggestion):
we can use `&&` here because trailing undefined arguments to `b.call` are ignored

```suggestion
			additional && b.object(Object.entries(additional).map(([k, v]) => b.init(k, b.literal(v))))
```

Code after:
 * @param {{ start?: number }} node - AST node for location info
 * @param {'component' | 'if' | 'each' | 'await' | 'key' | 'render'} type - Type of block/component
 * @param {Record<string, number | string>} [additional] - Any additional properties to add to the dev stack entry
 * @returns {ExpressionStatement} - Statement with or without dev stack wrapping
 */
export function add_svelte_meta(expression, node, type, additional) {
	if (!dev) {
		return b.stmt(expression);
	}

	const location = node.start !== undefined && locator(node.start);
	if (!location) {
		return b.stmt(expression);
	}

	return b.stmt(
		b.call(
			'$.add_svelte_meta',
			b.arrow([], expression),
			b.literal(type),
			b.id(component_name),
			b.literal(location.line),
			b.literal(location.column),
			additional && b.object(Object.entries(additional).map(([k, v]) => b.init(k, b.literal(v))))
		)
	);
}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js (JavaScript)

Code before:

	return sequence;
}

/**
 * Wraps a statement/expression with dev stack tracking in dev mode
 * @param {CallExpression} call_expression - The function call to wrap (e.g., $.if, $.each, etc.)
 * @param {{ start?: number }} node - AST node for location info
 * @param {'component' | 'if' | 'each' | 'await' | 'key'} type - Type of block/component
 * @param {Record<string, number | string>} [additional] - Any additional properties to add to the dev stack entry
 * @returns {Statement} - Statement with or without dev stack wrapping
 */
export function with_dev_stack(call_expression, node, type, additional) {
	if (!dev) {
		return b.stmt(call_expression);
	}

	const location = node.start && locator(node.start);
	if (!location) {
		return b.stmt(call_expression);
	}

	return b.stmt(
		b.call(
			'$.with_dev_stack',
			b.arrow([], b.block([b.stmt(call_expression)])),
			b.literal(type),
			b.literal(filename),
			b.literal(location.line),
			b.literal(location.column),
			additional
				? b.object(Object.entries(additional).map(([key, value]) => b.init(key, b.literal(value))))
				: b.null
		)
	);
}


Review comment (suggestion):
using an implicit return will keep the statement on a single line in many cases 

```suggestion
			b.arrow([], call_expression),
```

Code after:

	return sequence;
}

/**
 * Wraps a statement/expression with dev stack tracking in dev mode
 * @param {Expression} expression - The function call to wrap (e.g., $.if, $.each, etc.)
 * @param {{ start?: number }} node - AST node for location info
 * @param {'component' | 'if' | 'each' | 'await' | 'key' | 'render'} type - Type of block/component
 * @param {Record<string, number | string>} [additional] - Any additional properties to add to the dev stack entry
 * @returns {ExpressionStatement} - Statement with or without dev stack wrapping
 */
export function add_svelte_meta(expression, node, type, additional) {
	if (!dev) {
		return b.stmt(expression);
	}

	const location = node.start !== undefined && locator(node.start);
	if (!location) {
		return b.stmt(expression);
	}

	return b.stmt(
		b.call(
			'$.add_svelte_meta',
			b.arrow([], expression),
			b.literal(type),
			b.id(component_name),
			b.literal(location.line),
			b.literal(location.column),
			additional && b.object(Object.entries(additional).map(([k, v]) => b.init(k, b.literal(v))))
		)
	);
}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js (JavaScript)

Code before:
}

/**
 * Wraps a statement/expression with dev stack tracking in dev mode
 * @param {CallExpression} call_expression - The function call to wrap (e.g., $.if, $.each, etc.)
 * @param {{ start?: number }} node - AST node for location info
 * @param {'component' | 'if' | 'each' | 'await' | 'key'} type - Type of block/component
 * @param {Record<string, number | string>} [additional] - Any additional properties to add to the dev stack entry
 * @returns {Statement} - Statement with or without dev stack wrapping
 */
export function with_dev_stack(call_expression, node, type, additional) {
	if (!dev) {
		return b.stmt(call_expression);
	}

	const location = node.start && locator(node.start);
	if (!location) {
		return b.stmt(call_expression);
	}

	return b.stmt(
		b.call(
			'$.with_dev_stack',
			b.arrow([], b.block([b.stmt(call_expression)])),
			b.literal(type),
			b.literal(filename),
			b.literal(location.line),
			b.literal(location.column),
			additional
				? b.object(Object.entries(additional).map(([key, value]) => b.init(key, b.literal(value))))
				: b.null
		)
	);
}


Review comment (suggestion):
it'd be nice to use `ComponentName` here rather than printing the literal filename, though as things stand that would involve passing the `analysis` object into this function. I think it would be good if `analysis` was globally available in the same way that things like `dev` and `locator` are, though that would mean shuffling some stuff around... might see how straightforward that is in a separate PR

Code after:
}

/**
 * Wraps a statement/expression with dev stack tracking in dev mode
 * @param {Expression} expression - The function call to wrap (e.g., $.if, $.each, etc.)
 * @param {{ start?: number }} node - AST node for location info
 * @param {'component' | 'if' | 'each' | 'await' | 'key' | 'render'} type - Type of block/component
 * @param {Record<string, number | string>} [additional] - Any additional properties to add to the dev stack entry
 * @returns {ExpressionStatement} - Statement with or without dev stack wrapping
 */
export function add_svelte_meta(expression, node, type, additional) {
	if (!dev) {
		return b.stmt(expression);
	}

	const location = node.start !== undefined && locator(node.start);
	if (!location) {
		return b.stmt(expression);
	}

	return b.stmt(
		b.call(
			'$.add_svelte_meta',
			b.arrow([], expression),
			b.literal(type),
			b.id(component_name),
			b.literal(location.line),
			b.literal(location.column),
			additional && b.object(Object.entries(additional).map(([k, v]) => b.init(k, b.literal(v))))
		)
	);
}