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

Code before:
					effect.fn = null;
				}
			}

			// If update_effect() has a flushSync() in it, we may have flushed another flush_queued_effects(),
			// which already handled this logic and did set eager_block_effects to null.
			if (eager_block_effects?.length > 0) {
				// TODO this feels incorrect! it gets the tests passing
				old_values.clear();

				for (const e of eager_block_effects) {
					update_effect(e);
				}

				eager_block_effects = [];
			}
		}
	}

	eager_block_effects = null;
}

/**
 * This is similar to `mark_reactions`, but it only marks async/block effects
 * depending on one of the sources, so that these effects can re-run after
 * another batch has been committed
 * @param {Value} value
 * @param {Source[]} sources
 */
function mark_effects(value, sources) {
	if (value.reactions !== null) {
		for (const reaction of value.reactions) {
			const flags = reaction.f;

			if ((flags & DERIVED) !== 0) {
				mark_effects(/** @type {Derived} */ (reaction), sources);
			} else if ((flags & (ASYNC | BLOCK_EFFECT)) !== 0 && depends_on(reaction, sources)) {
				set_signal_status(reaction, DIRTY);
				schedule_effect(/** @type {Effect} */ (reaction));
			}
		}
	}
}

/**
 * @param {Reaction} reaction
 * @param {Source[]} sources
 */
function depends_on(reaction, sources) {
	if (reaction.deps !== null) {
		for (const dep of reaction.deps) {

Review comment (performance):
```suggestion
 * depending on `value` and at least one of the other `sources`, so that
 * these effects can re-run after another batch has been committed
```

Code after:
		}
	}

	eager_block_effects = null;
}

/**
 * This is similar to `mark_reactions`, but it only marks async/block effects
 * depending on `value` and at least one of the other `sources`, so that
 * these effects can re-run after another batch has been committed
 * @param {Value} value
 * @param {Source[]} sources
 */
function mark_effects(value, sources) {
	if (value.reactions !== null) {
		for (const reaction of value.reactions) {
			const flags = reaction.f;

			if ((flags & DERIVED) !== 0) {
				mark_effects(/** @type {Derived} */ (reaction), sources);
			} else if ((flags & (ASYNC | BLOCK_EFFECT)) !== 0 && depends_on(reaction, sources)) {
				set_signal_status(reaction, DIRTY);
				schedule_effect(/** @type {Effect} */ (reaction));
			}
		}
	}
}

/**
 * @param {Reaction} reaction
 * @param {Source[]} sources
 */
function depends_on(reaction, sources) {
	if (reaction.deps !== null) {
		for (const dep of reaction.deps) {
			if (sources.includes(dep)) {
				return true;
			}

			if ((dep.f & DERIVED) !== 0 && depends_on(/** @type {Derived} */ (dep), sources)) {
				return true;
			}
		}
	}

	return false;
}

/**
 * @param {Effect} signal
 * @returns {void}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/server/visitors/CallExpression.js (JavaScript)

Code before:
	if (rune === '$state.eager') {
		return node.arguments[0];
	}

	if (rune === '$state.snapshot') {
		return b.call(
			'$.snapshot',
			/** @type {Expression} */ (context.visit(node.arguments[0])),
			is_ignored(node, 'state_snapshot_uncloneable') && b.true
		);
	}

	if (rune === '$inspect' || rune === '$inspect().with') {
		if (!dev) return b.empty;

		const call =
			rune === '$inspect'
				? node
				: /** @type {CallExpression} */ (/** @type {MemberExpression} */ (node.callee).object);

		const args = call.arguments.map((arg) => /** @type {Expression} */ (context.visit(arg)));

		const inspector =
			rune === '$inspect'
				? 'console.log'
				: /** @type {Expression} */ (context.visit(node.arguments[0]));

		// TODO is the `init` doing any work on the server? should it be `$inspect` instead?
		// should we include a stack trace?
		return b.call(inspector, b.literal('init'), ...args);
	}

	context.next();
}


Review comment (refactor):
It might be silly for `$$args` but this is a lot of specific and repeated code...can we have a little utility to do this work?

Code after:

	if (rune === '$state.eager') {
		return node.arguments[0];
	}

	if (rune === '$state.snapshot') {
		return b.call(
			'$.snapshot',
			/** @type {Expression} */ (context.visit(node.arguments[0])),
			is_ignored(node, 'state_snapshot_uncloneable') && b.true
		);
	}

	if (rune === '$inspect' || rune === '$inspect().with') {
		if (!dev) return b.empty;

		const { args, inspector } = get_inspect_args(rune, node, context.visit);

		return rune === '$inspect'
			? b.call(inspector, b.literal('$inspect('), ...args, b.literal(')'))
			: b.call(inspector, b.literal('init'), ...args);
	}

	context.next();
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/batch.js (JavaScript)

Code before:

	/**
	 * @param {Effect[]} effects
	 */
	#defer_effects(effects) {
		for (const e of effects) {
			const target = (e.f & DIRTY) !== 0 ? this.#dirty_effects : this.#maybe_dirty_effects;
			target.push(e);

			// mark as clean so they get scheduled if they depend on pending async state
			set_signal_status(e, CLEAN);
		}
	}

	/**
	 * Associate a change to a given source with the current
	 * batch, noting its previous and current values
	 * @param {Source} source
	 * @param {any} value
	 */
	capture(source, value) {
		if (!this.previous.has(source)) {
			this.previous.set(source, value);
		}

		// Don't save this as it would mean it's not thrown in the `runtime.js#get` function
		if ((source.f & ERROR_VALUE) === 0) {
			this.current.set(source, source.v);
			batch_values?.set(source, source.v);
		}
	}

	activate() {
		current_batch = this;
		this.apply();
	}

	deactivate() {
		current_batch = null;
		batch_values = null;
	}

	flush() {
		this.activate();

		if (queued_root_effects.length > 0) {
			flush_effects();

			if (current_batch !== null && current_batch !== this) {
				// this can happen if a new batch was created during `flush_effects()`
				return;

Review comment (performance):
```suggestion
		// Don't save errors in `batch_values`, or they won't be thrown in `runtime.js#get`
```

Code after:

	/**
	 * @param {Effect[]} effects
	 */
	#defer_effects(effects) {
		for (const e of effects) {
			const target = (e.f & DIRTY) !== 0 ? this.#dirty_effects : this.#maybe_dirty_effects;
			target.push(e);

			// mark as clean so they get scheduled if they depend on pending async state
			set_signal_status(e, CLEAN);
		}
	}

	/**
	 * Associate a change to a given source with the current
	 * batch, noting its previous and current values
	 * @param {Source} source
	 * @param {any} value
	 */
	capture(source, value) {
		if (!this.previous.has(source)) {
			this.previous.set(source, value);
		}

		// Don't save errors in `batch_values`, or they won't be thrown in `runtime.js#get`
		if ((source.f & ERROR_VALUE) === 0) {
			this.current.set(source, source.v);
			batch_values?.set(source, source.v);
		}
	}

	activate() {
		current_batch = this;
		this.apply();
	}

	deactivate() {
		current_batch = null;
		batch_values = null;
	}

	flush() {
		this.activate();

		if (queued_root_effects.length > 0) {
			flush_effects();

			if (current_batch !== null && current_batch !== this) {
				// this can happen if a new batch was created during `flush_effects()`
				return;
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/runtime.js (JavaScript)

Code before:
export let update_version = read_version;

/** @param {number} value */
export function set_update_version(value) {
	update_version = value;
}

export function increment_write_version() {
	return ++write_version;
}

/**
 * Determines whether a derived or effect is dirty.
 * If it is MAYBE_DIRTY, will set the status to CLEAN
 * @param {Reaction} reaction
 * @returns {boolean}
 */
export function is_dirty(reaction) {
	var flags = reaction.f;

	if ((flags & DIRTY) !== 0) {
		return true;
	}

	if (flags & DERIVED) {
		reaction.f &= ~WAS_MARKED;
	}

	if ((flags & MAYBE_DIRTY) !== 0) {
		var dependencies = reaction.deps;

		if (dependencies !== null) {
			var i;
			var dependency;
			var length = dependencies.length;

			for (i = 0; i < length; i++) {
				dependency = dependencies[i];

				if (is_dirty(/** @type {Derived} */ (dependency))) {
					update_derived(/** @type {Derived} */ (dependency));
				}

				if (dependency.wv > reaction.wv) {
					return true;
				}
			}
		}

		if (
			(flags & CONNECTED) !== 0 &&

Review comment (performance):
microscopically more efficient:

```suggestion
	if ((flags & WAS_MARKED) !== 0) {
		reaction.f ^= WAS_MARKED;
```

Code after:
export let update_version = read_version;

/** @param {number} value */
export function set_update_version(value) {
	update_version = value;
}

export function increment_write_version() {
	return ++write_version;
}

/**
 * Determines whether a derived or effect is dirty.
 * If it is MAYBE_DIRTY, will set the status to CLEAN
 * @param {Reaction} reaction
 * @returns {boolean}
 */
export function is_dirty(reaction) {
	var flags = reaction.f;

	if ((flags & DIRTY) !== 0) {
		return true;
	}

	if (flags & DERIVED) {
		reaction.f &= ~WAS_MARKED;
	}

	if ((flags & MAYBE_DIRTY) !== 0) {
		var dependencies = reaction.deps;

		if (dependencies !== null) {
			var length = dependencies.length;

			for (var i = 0; i < length; i++) {
				var dependency = dependencies[i];

				if (is_dirty(/** @type {Derived} */ (dependency))) {
					update_derived(/** @type {Derived} */ (dependency));
				}

				if (dependency.wv > reaction.wv) {
					return true;
				}
			}
		}

		if (
			(flags & CONNECTED) !== 0 &&
			// During time traveling we don't want to reset the status so that
			// traversal of the graph in the other batches still happens
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/deriveds.js (JavaScript)

Code before:
		// TODO can we avoid setting `derived.v` when `batch_values !== null`,
		// without causing the value to be stale later?
		derived.v = value;
		derived.wv = increment_write_version();
	}

	// don't mark derived clean if we're reading it inside a
	// cleanup function, or it will cache a stale value
	if (is_destroying_effect) {
		return;
	}

	// During time traveling we don't want to reset the status so that
	// traversal of the graph in the other batches still happens
	if (batch_values !== null) {
		// Delete the value as the current one is now the latest.
		// Deleting instead of updating handles the case where a derived
		// is subsequently indirectly updated in the same batch — without
		// deleting here we would incorrectly get the old value from `batch_values`
		// instead of recomputing it. The one drawback is that it's now a bit
		// more inefficient to get the value of that derived again in the same batch,
		// as it has to check is_dirty all the way up the graph all the time.
		// TODO if that turns out to be a performance problem, we could try
		// to save the current status of the derived in a map and restore it
		// before leaving the batch.
		batch_values.delete(derived);
	} else {
		var status = (derived.f & CONNECTED) === 0 ? MAYBE_DIRTY : CLEAN;
		set_signal_status(derived, status);
	}
}


Review comment (performance):
This feels redundant, since we never do `batch_values.set(derived, ...)` in the first place. If I delete this and the other `batch_values?.delete` occurrence in `batch.js`, all the tests still pass.

So basically what this PR is doing is removing caching for deriveds _altogether_ when time-travelling. It eliminates any problems around staleness, but it feels like quite a high cost

Code after:
		// TODO can we avoid setting `derived.v` when `batch_values !== null`,
		// without causing the value to be stale later?
		derived.v = value;
		derived.wv = increment_write_version();
	}

	// don't mark derived clean if we're reading it inside a
	// cleanup function, or it will cache a stale value
	if (is_destroying_effect) {
		return;
	}

	// During time traveling we don't want to reset the status so that
	// traversal of the graph in the other batches still happens
	if (batch_values !== null) {
		// only cache the value if we're in a tracking context, otherwise we won't
		// clear the cache in `mark_reactions` when dependencies are updated
		if (effect_tracking()) {
			batch_values.set(derived, derived.v);
		}
	} else {
		var status = (derived.f & CONNECTED) === 0 ? MAYBE_DIRTY : CLEAN;
		set_signal_status(derived, status);
	}
}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/1-parse/index.js (JavaScript)

Code before:
		while (this.index < this.template.length) {
			state = state(this) || fragment;
		}

		if (this.stack.length > 1) {
			const current = this.current();

			if (this.loose) {
				current.end = this.template.length;
			} else if (current.type === 'RegularElement') {
				current.end = current.start + 1;
				e.element_unclosed(current, current.name);
			} else {
				current.end = current.start + 1;
				e.block_unclosed(current);
			}
		}

		if (state !== fragment) {
			e.unexpected_eof(this.index);
		}

		const has_content = this.root.fragment.nodes.some(
			(node) => node.type !== 'Text' || node.data.trim().length > 0
		);
		if (has_content) {
			let start = /** @type {number} */ (this.root.fragment.nodes[0].start);
			while (regex_whitespace.test(template[start])) start += 1;

			let end = /** @type {number} */ (
				this.root.fragment.nodes[this.root.fragment.nodes.length - 1].end
			);
			while (regex_whitespace.test(template[end - 1])) end -= 1;

			this.root.start = start;
			this.root.end = end;
		} else {
			// @ts-ignore
			this.root.start = this.root.end = null;
		}

		const options_index = this.root.fragment.nodes.findIndex(
			/** @param {any} thing */
			(thing) => thing.type === 'SvelteOptions'
		);
		if (options_index !== -1) {
			const options = /** @type {AST.SvelteOptionsRaw} */ (this.root.fragment.nodes[options_index]);
			this.root.fragment.nodes.splice(options_index, 1);
			this.root.options = read_options(options);

			disallow_children(options);

Review comment (suggestion):
alternatively, less explicit/clear and more defensive:

```js
if (this.root.fragment.nodes.length) {
	// ...

	if (start <= end) {
		this.root.start = start;
		this.root.end = end;
	} else {
		// @ts-ignore
		this.root.start = this.root.end = null;
	}
} else {
	// @ts-ignore
	this.root.start = this.root.end = null;
}
```

Code after:
		while (this.index < this.template.length) {
			state = state(this) || fragment;
		}

		if (this.stack.length > 1) {
			const current = this.current();

			if (this.loose) {
				current.end = this.template.length;
			} else if (current.type === 'RegularElement') {
				current.end = current.start + 1;
				e.element_unclosed(current, current.name);
			} else {
				current.end = current.start + 1;
				e.block_unclosed(current);
			}
		}

		if (state !== fragment) {
			e.unexpected_eof(this.index);
		}

		this.root.start = 0;
		this.root.end = template.length;

		const options_index = this.root.fragment.nodes.findIndex(
			/** @param {any} thing */
			(thing) => thing.type === 'SvelteOptions'
		);
		if (options_index !== -1) {
			const options = /** @type {AST.SvelteOptionsRaw} */ (this.root.fragment.nodes[options_index]);
			this.root.fragment.nodes.splice(options_index, 1);
			this.root.options = read_options(options);

			disallow_children(options);

			// We need this for the old AST format
			Object.defineProperty(this.root.options, '__raw__', {
				value: options,
				enumerable: false
			});
		}
	}

	current() {
		return this.stack[this.stack.length - 1];
	}

	/**
	 * @param {any} err
	 * @returns {never}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/server/renderer.js (JavaScript)

Code before:
	 */
	async #hydratable_block(ctx) {
		if (ctx.lookup.size === 0) {
			return null;
		}

		let entries = [];
		let has_promises = false;

		for (const [k, v] of ctx.lookup) {
			if (v.promises) {
				has_promises = true;
				for (const p of v.promises) await p;
			}

			entries.push(`[${JSON.stringify(k)},${v.serialized}]`);
		}

		let prelude = `const h = (window.__svelte ??= {}).h ??= new Map();`;

		if (has_promises) {
			prelude = `const r = (v) => Promise.resolve(v);\n\t${prelude}`;
		}

		const body = `
{
	${prelude}

	for (const [k, v] of [
		${entries.join(',\n')}
	]) {
		h.set(k, v);
	}
}
`;

		let csp_attr = '';
		if (this.global.csp.nonce) {
			csp_attr = ` nonce="${this.global.csp.nonce}"`;
		} else if (this.global.csp.hash) {
			// note to future selves: this doesn't need to be optimized with a Map<body, hash>
			// because the it's impossible for identical data to occur multiple times in a single render
			// (this would require the same hydratable key:value pair to be serialized multiple times)
			const hash = await sha256(body);
			this.global.csp.script_hashes.push(`sha256-${hash}`);
		}

		return `<script${csp_attr}>${body}</script>`;
	}
}


Review comment (suggestion):
I dedented and changed the formatting of this stuff because it was confusing in the browser... I could be convinced to sacrifice browser readability for code readability but I don't think the readability here is that bad, and it also technically reduces the byte count

Code after:
	 */
	async #hydratable_block(ctx) {
		if (ctx.lookup.size === 0) {
			return null;
		}

		let entries = [];
		let has_promises = false;

		for (const [k, v] of ctx.lookup) {
			if (v.promises) {
				has_promises = true;
				for (const p of v.promises) await p;
			}

			entries.push(`[${JSON.stringify(k)},${v.serialized}]`);
		}

		let prelude = `const h = (window.__svelte ??= {}).h ??= new Map();`;

		if (has_promises) {
			prelude = `const r = (v) => Promise.resolve(v);
				${prelude}`;
		}

		const body = `
			{
				${prelude}

				for (const [k, v] of [
					${entries.join(',\n\t\t\t\t\t')}
				]) {
					h.set(k, v);
				}
			}
		`;

		let csp_attr = '';
		if (this.global.csp.nonce) {
			csp_attr = ` nonce="${this.global.csp.nonce}"`;
		} else if (this.global.csp.hash) {
			// note to future selves: this doesn't need to be optimized with a Map<body, hash>
			// because the it's impossible for identical data to occur multiple times in a single render
			// (this would require the same hydratable key:value pair to be serialized multiple times)
			const hash = await sha256(body);
			this.global.csp.script_hashes.push(`sha256-${hash}`);
		}

		return `\n\t\t<script${csp_attr}>${body}</script>`;
	}
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/server/crypto.js (JavaScript)

Code before:
let text_encoder;
// TODO - remove this and use global `crypto` when we drop Node 18
let crypto;

/** @param {string} data */
export async function sha256(data) {
	text_encoder ??= new TextEncoder();
	// @ts-ignore - we don't install node types in the prod build
	crypto ??= (await import('node:crypto')).webcrypto;
	const hash_buffer = await crypto.subtle.digest('SHA-256', text_encoder.encode(data));
	// @ts-ignore - we don't install node types in the prod build
	return Buffer.from(hash_buffer).toString('base64');
}


Review comment (suggestion):
in most cases we won't need to `import('node:crypto')`

```suggestion
let crypto = globalThis.crypto;
```

Code after:
import { BROWSER } from 'esm-env';

let text_encoder;
// TODO - remove this and use global `crypto` when we drop Node 18
let crypto;

/** @param {string} data */
export async function sha256(data) {
	text_encoder ??= new TextEncoder();

	// @ts-expect-error
	crypto ??= globalThis.crypto?.subtle?.digest
		? globalThis.crypto
		: // @ts-ignore - we don't install node types in the prod build
			(await import('node:crypto')).webcrypto;

	const hash_buffer = await crypto.subtle.digest('SHA-256', text_encoder.encode(data));

	return base64_encode(hash_buffer);
}

/**
 * @param {Uint8Array} bytes
 * @returns {string}
 */
export function base64_encode(bytes) {
	// Using `Buffer` is faster than iterating
	// @ts-ignore
	if (!BROWSER && globalThis.Buffer) {
		// @ts-ignore
Repository: sveltejs/svelte
File: packages/svelte/src/internal/server/crypto.js (JavaScript)

Code before:
let text_encoder;
// TODO - remove this and use global `crypto` when we drop Node 18
let crypto;

/** @param {string} data */
export async function sha256(data) {
	text_encoder ??= new TextEncoder();
	// @ts-ignore - we don't install node types in the prod build
	crypto ??= (await import('node:crypto')).webcrypto;
	const hash_buffer = await crypto.subtle.digest('SHA-256', text_encoder.encode(data));
	// @ts-ignore - we don't install node types in the prod build
	return Buffer.from(hash_buffer).toString('base64');
}


Review comment (suggestion):
come to think of it didn't we talk about skipping this and assuming that `crypto` is always available as a global, given that `hydratable` is behind the `experimental.async` flag? not worried about excluding people who want to use bleeding edge Svelte features but are still rocking Node 16 or whatever

Code after:
import { BROWSER } from 'esm-env';

let text_encoder;
// TODO - remove this and use global `crypto` when we drop Node 18
let crypto;

/** @param {string} data */
export async function sha256(data) {
	text_encoder ??= new TextEncoder();

	// @ts-expect-error
	crypto ??= globalThis.crypto?.subtle?.digest
		? globalThis.crypto
		: // @ts-ignore - we don't install node types in the prod build
			(await import('node:crypto')).webcrypto;

	const hash_buffer = await crypto.subtle.digest('SHA-256', text_encoder.encode(data));

	return base64_encode(hash_buffer);
}

/**
 * @param {Uint8Array} bytes
 * @returns {string}
 */
export function base64_encode(bytes) {
	// Using `Buffer` is faster than iterating
	// @ts-ignore
	if (!BROWSER && globalThis.Buffer) {
		// @ts-ignore
		return globalThis.Buffer.from(bytes).toString('base64');
	}

	let binary = '';