ronantakizawa/codereview-bench huggingface dataset:
Repository: supabase/supabase
File: apps/studio/components/interfaces/Organization/Organization.utils.ts (TypeScript)

Code before:
/**
 * Builds an org-scoped URL for the wildcard org route (/org/_).
 *
 * When `slug` is undefined the destination is the org project list at
 * /org/<orgSlug>. When `slug` is an array the sub-path is preserved so the
 * user lands on the equivalent page for the chosen org.
 */
export function buildOrgUrl(
  slug: string | string[] | undefined,
  orgSlug: string,
  queryString: string
): string {
  const qs = queryString ? `?${queryString}` : ''
  if (!Array.isArray(slug)) {
    return `/org/${orgSlug}${qs}`
  }
  const slugPath = slug.reduce((a: string, b: string) => `${a}/${b}`, '').slice(1)
  return `/org/${orgSlug}/${slugPath}${qs}`
}

// Invite is expired if older than 24hrs
export function isInviteExpired(timestamp: string) {
  const inviteDate = new Date(timestamp)
  const now = new Date()
  const timeBetween = now.valueOf() - inviteDate.valueOf()
  if (timeBetween / 1000 / 60 / 60 < 24) {
    return false
  }
  return true
}


Review comment (suggestion):
I would expect the `orgSlug` to be a first param, then slug (maybe rename to `path` or `subPath`) and then params.

Code after:
/**
 * Builds an org-scoped URL for the wildcard org route (/org/_).
 *
 * When `slug` is undefined the destination is the org project list at
 * /org/<orgSlug>. When `slug` is an array the sub-path is preserved so the
 * user lands on the equivalent page for the chosen org.
 */
export function buildOrgUrl({
  slug,
  orgSlug,
  queryString,
}: {
  slug: string | string[] | undefined
  orgSlug: string
  queryString: string
}): string {
  const qs = queryString ? `?${queryString}` : ''
  if (!Array.isArray(slug)) {
    return `/org/${orgSlug}${qs}`
  }
  const slugPath = slug.reduce((a: string, b: string) => `${a}/${b}`, '').slice(1)
  return `/org/${orgSlug}/${slugPath}${qs}`
}

// Invite is expired if older than 24hrs
export function isInviteExpired(timestamp: string) {
  const inviteDate = new Date(timestamp)
  const now = new Date()
  const timeBetween = now.valueOf() - inviteDate.valueOf()
  if (timeBetween / 1000 / 60 / 60 < 24) {
    return false
  }
  return true
Repository: sveltejs/svelte
File: src/compiler/compile/css/Selector.ts (TypeScript)

Code before:

	validate(component: Component) {
		let start = 0;
		let end = this.blocks.length;

		for (; start < end; start += 1) {
			if (!this.blocks[start].global) break;
		}

		for (; end > start; end -= 1) {
			if (!this.blocks[end - 1].global) break;
		}

		for (let i = start; i < end; i += 1) {
			if (this.blocks[i].global) {
				component.error(this.blocks[i].selectors[0], {
					code: 'css-invalid-global',
					message: ':global(...) can be at the start or end of a selector sequence, but not in the middle'
				});
			}
		}

		for (const block of this.blocks) {
			for (const selector of block.selectors) {
				if (selector.type === 'PseudoClassSelector' && selector.name === 'global') {
					if (/[^\\],(?![^([]+(?:\)|\]))/.test(selector.children[0].value)) {
						component.error(selector, {
							code: 'css-invalid-global-selector',
							message: ':global(...) must contain a single selector'
						});
					}
				}
			}
		}
	}

	get_amount_class_specificity_increased() {
		let count = 0;
		for (const block of this.blocks) {
			if (block.should_encapsulate) {
				count ++;
			}
		}
		return count;
	}
}

function apply_selector(blocks: Block[], node: Element, to_encapsulate: Array<{ node: Element, block: Block }>): boolean {
	const block = blocks.pop();
	if (!block) return false;


Review comment (security):
it is possible to escape the `)` character in the selector:

```css
:global(.abc, .def\)) {
}
```

I propose update the regex to handle it:

```js
/[^\\],(?!([^([]+[^\\]|[^([\\])[)\]])/
```

Code after:

	validate(component: Component) {
		let start = 0;
		let end = this.blocks.length;

		for (; start < end; start += 1) {
			if (!this.blocks[start].global) break;
		}

		for (; end > start; end -= 1) {
			if (!this.blocks[end - 1].global) break;
		}

		for (let i = start; i < end; i += 1) {
			if (this.blocks[i].global) {
				component.error(this.blocks[i].selectors[0], {
					code: 'css-invalid-global',
					message: ':global(...) can be at the start or end of a selector sequence, but not in the middle'
				});
			}
		}

		for (const block of this.blocks) {
			for (const selector of block.selectors) {
				if (selector.type === 'PseudoClassSelector' && selector.name === 'global') {
					if (/[^\\],(?!([^([]+[^\\]|[^([\\])[)\]])/.test(selector.children[0].value)) {
						component.error(selector, {
							code: 'css-invalid-global-selector',
							message: ':global(...) must contain a single selector'
						});
					}
				}
			}
		}
	}

	get_amount_class_specificity_increased() {
		let count = 0;
		for (const block of this.blocks) {
			if (block.should_encapsulate) {
				count ++;
			}
		}
		return count;
	}
}

function apply_selector(blocks: Block[], node: Element, to_encapsulate: Array<{ node: Element, block: Block }>): boolean {
	const block = blocks.pop();
	if (!block) return false;
Repository: sveltejs/svelte
File: src/runtime/internal/ResizeObserverSingleton.ts (TypeScript)

Code before:
const MapImplementation = "WeakMap" in window ? WeakMap : Map;

/**
 * Resize observer singleton
 * https://groups.google.com/a/chromium.org/g/blink-dev/c/z6ienONUb5A/m/F5-VcUZtBAAJ
 */
export class ResizeObserverSingleton {
	constructor(readonly options?: ResizeObserverOptions) {}

	addListener(element: Element, callback: Callback) {
		if (!this._subscriptions.has(element)) {
			this._subscriptions.set(element, new Subscription(new Set([callback])));
			this._getObserver().observe(element, this.options);
		} else {
			const subscription = this._subscriptions.get(element)!;
			subscription.listeners.add(callback);
			callback.call(element, subscription.lastEntry!);
		}
	}

	removeListener(element: Element, callback: Callback) {
		const subscription = this._subscriptions.get(element);
		if (!subscription) return;
		if (subscription.listeners.size > 1) {
			subscription.listeners.delete(callback);
		} else {
			this._subscriptions.delete(element);
			this._getObserver().unobserve(element);
		}
	}


Review comment (suggestion):
It's a little unfortunate that this much code is needed to have a performant implementation of the resize observer. This adds quite a bit of code to the runtime (only if you use the feature of course), but I guess that's unavoidable..

Code after:
/**
 * Resize observer singleton.
 * One listener per element only!
 * https://groups.google.com/a/chromium.org/g/blink-dev/c/z6ienONUb5A/m/F5-VcUZtBAAJ
 */
export class ResizeObserverSingleton {
	constructor(readonly options?: ResizeObserverOptions) {}

	observe(element: Element, listener: Listener) {
		this._listeners.set(element, listener);
		this._getObserver().observe(element, this.options);
		return () => {
			this._listeners.delete(element);
			this._observer.unobserve(element); // this line can probably be removed
		};
	}

	static readonly entries: WeakMap<Element, ResizeObserverEntry> = 'WeakMap' in globalThis ? new WeakMap() : undefined;

	private readonly _listeners: WeakMap<Element, Listener> = 'WeakMap' in globalThis ? new WeakMap() : undefined;
	private _observer?: ResizeObserver;
	private _getObserver() {
		return this._observer ?? (this._observer = new ResizeObserver((entries) => {
			for (const entry of entries) {
				ResizeObserverSingleton.entries.set(entry.target, entry);
				this._listeners.get(entry.target)?.(entry);
			}
		}));
	}
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/runtime.js (JavaScript)

Code before:
import { DEV } from 'esm-env';
import {
	get_descriptor,
	get_descriptors,
	get_prototype_of,
	is_frozen,
	object_freeze
} from './utils.js';
import { snapshot } from './proxy.js';
import { destroy_effect, effect, execute_effect_teardown } from './reactivity/effects.js';
import {
	EFFECT,
	RENDER_EFFECT,
	DIRTY,
	MAYBE_DIRTY,
	CLEAN,
	DERIVED,
	UNOWNED,
	DESTROYED,
	INERT,
	BRANCH_EFFECT,
	STATE_SYMBOL,
	BLOCK_EFFECT,
	ROOT_EFFECT,
	LEGACY_DERIVED_PROP
} from './constants.js';
import { flush_tasks } from './dom/task.js';
import { add_owner } from './dev/ownership.js';

Review comment (suggestion):
```suggestion
	define_property,
	get_descriptor,
```

Code after:
import { DEV } from 'esm-env';
import {
	define_property,
	get_descriptors,
	get_prototype_of,
	is_frozen,
	object_freeze
} from './utils.js';
import { snapshot } from './proxy.js';
import { destroy_effect, effect, execute_effect_teardown } from './reactivity/effects.js';
import {
	EFFECT,
	RENDER_EFFECT,
	DIRTY,
	MAYBE_DIRTY,
	CLEAN,
	DERIVED,
	UNOWNED,
	DESTROYED,
	INERT,
	BRANCH_EFFECT,
	STATE_SYMBOL,
	BLOCK_EFFECT,
	ROOT_EFFECT,
	LEGACY_DERIVED_PROP,
	DISCONNECTED,
	STATE_FROZEN_SYMBOL
} from './constants.js';
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/runtime.js (JavaScript)

Code before:
	}

	const component_stack = [];

	const effect_name = effect.fn.name;

	if (effect_name) {
		component_stack.push(effect_name);
	}

	/** @type {import("#client").ComponentContext | null} */
	let current_context = component_context;

	while (current_context !== null) {
		var filename = current_context.function?.filename;

		if (filename) {
			const file = filename.split('/').at(-1);
			component_stack.push(file);
		}

		current_context = current_context.p;
	}

	const indent = /Firefox/.test(navigator.userAgent) ? '  ' : '\t';
	Object.defineProperty(error, 'message', {
		value: error.message + `\n${component_stack.map((name) => `\n${indent}in ${name}`).join('')}\n`
	});

	const stack = error.stack;

	// Filter out internal files from callstack
	if (stack) {
		const lines = stack.split('\n');
		const new_lines = [];
		for (let i = 0; i < lines.length; i++) {
			const line = lines[i];
			if (line.includes('svelte/src/internal')) {
				continue;
			}
			new_lines.push(line);
		}
		Object.defineProperty(error, 'stack', {
			value: error.stack + new_lines.join('\n')
		});
	}

	handled_errors.add(error);
	throw error;
}


Review comment (suggestion):
```suggestion
	define_property(error, 'message', {
```

Code after:
		}

		// Unowned signals are always maybe dirty, as we instead check their dependency versions.
		if (!is_unowned) {
			set_signal_status(reaction, CLEAN);
		}
		if (is_disconnected) {
			reaction.f ^= DISCONNECTED;
		}
	}

	return is_dirty;
}

/**
 * @param {Error} error
 * @param {import("#client").Effect} effect
 * @param {import("#client").ComponentContext | null} component_context
 */
function handle_error(error, effect, component_context) {
	// Given we don't yet have error boundaries, we will just always throw.
	if (!DEV || handled_errors.has(error) || component_context === null) {
		throw error;
	}

	const component_stack = [];

	const effect_name = effect.fn.name;

	if (effect_name) {
		component_stack.push(effect_name);
	}

	/** @type {import("#client").ComponentContext | null} */
	let current_context = component_context;

	while (current_context !== null) {
		var filename = current_context.function?.filename;

		if (filename) {
			const file = filename.split('/').at(-1);
			component_stack.push(file);
		}

		current_context = current_context.p;
	}

	const indent = /Firefox/.test(navigator.userAgent) ? '  ' : '\t';
	define_property(error, 'message', {
		value: error.message + `\n${component_stack.map((name) => `\n${indent}in ${name}`).join('')}\n`
	});
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/runtime.js (JavaScript)

Code before:
			const file = filename.split('/').at(-1);
			component_stack.push(file);
		}

		current_context = current_context.p;
	}

	const indent = /Firefox/.test(navigator.userAgent) ? '  ' : '\t';
	Object.defineProperty(error, 'message', {
		value: error.message + `\n${component_stack.map((name) => `\n${indent}in ${name}`).join('')}\n`
	});

	const stack = error.stack;

	// Filter out internal files from callstack
	if (stack) {
		const lines = stack.split('\n');
		const new_lines = [];
		for (let i = 0; i < lines.length; i++) {
			const line = lines[i];
			if (line.includes('svelte/src/internal')) {
				continue;
			}
			new_lines.push(line);
		}
		Object.defineProperty(error, 'stack', {
			value: error.stack + new_lines.join('\n')
		});
	}

	handled_errors.add(error);
	throw error;
}

/**
 * @template V
 * @param {import('#client').Reaction} signal
 * @returns {V}
 */
export function execute_reaction_fn(signal) {
	const previous_dependencies = current_dependencies;
	const previous_dependencies_index = current_dependencies_index;
	const previous_untracked_writes = current_untracked_writes;
	const previous_reaction = current_reaction;
	const previous_skip_reaction = current_skip_reaction;
	const previous_untracking = current_untracking;

	current_dependencies = /** @type {null | import('#client').Value[]} */ (null);
	current_dependencies_index = 0;
	current_untracked_writes = null;
	current_reaction = signal;

Review comment (suggestion):
```suggestion
		define_property(error, 'stack', {
```

Code after:
 * @param {import("#client").ComponentContext | null} component_context
 */
function handle_error(error, effect, component_context) {
	// Given we don't yet have error boundaries, we will just always throw.
	if (!DEV || handled_errors.has(error) || component_context === null) {
		throw error;
	}

	const component_stack = [];

	const effect_name = effect.fn.name;

	if (effect_name) {
		component_stack.push(effect_name);
	}

	/** @type {import("#client").ComponentContext | null} */
	let current_context = component_context;

	while (current_context !== null) {
		var filename = current_context.function?.filename;

		if (filename) {
			const file = filename.split('/').at(-1);
			component_stack.push(file);
		}

		current_context = current_context.p;
	}

	const indent = /Firefox/.test(navigator.userAgent) ? '  ' : '\t';
	define_property(error, 'message', {
		value: error.message + `\n${component_stack.map((name) => `\n${indent}in ${name}`).join('')}\n`
	});

	const stack = error.stack;

	// Filter out internal files from callstack
	if (stack) {
		const lines = stack.split('\n');
		const new_lines = [];
		for (let i = 0; i < lines.length; i++) {
			const line = lines[i];
			if (line.includes('svelte/src/internal')) {
				continue;
			}
			new_lines.push(line);
		}
		define_property(error, 'stack', {
			value: error.stack + new_lines.join('\n')
		});
Repository: sveltejs/svelte
File: packages/svelte/src/index.d.ts (TypeScript)

Code before:
// This should contain all the public interfaces (not all of them are actually importable, check current Svelte for which ones are).

import './ambient.js';

/**
 * @deprecated Svelte components were classes in Svelte 4. In Svelte 5, they are not anymore.
 * Use `mount` or `createRoot` instead to instantiate components.
 * See [breaking changes](https://svelte-5-preview.vercel.app/docs/breaking-changes#components-are-no-longer-classes)
 * for more info.
 */
export interface ComponentConstructorOptions<
	Props extends Record<string, any> = Record<string, any>
> {
	target: Element | Document | ShadowRoot;
	anchor?: Element;
	props?: Props;
	context?: Map<any, any>;
	hydrate?: boolean;
	intro?: boolean;
	$$inline?: boolean;
}

/**
 * Utility type for ensuring backwards compatibility on a type level that if there's a default slot, add 'children' to the props
 */
type Properties<Props, Slots> = Props &
	(Slots extends { default: any }
		? // This is unfortunate because it means "accepts no props" turns into "accepts any prop"
			// but the alternative is non-fixable type errors because of the way TypeScript index
			// signatures work (they will always take precedence and make an impossible-to-satisfy children type).
			Props extends Record<string, never>

Review comment (suggestion):
```suggestion
 * @deprecated In Svelte 4, components are classes. In Svelte 5, they are functions.
```

Code after:
// This should contain all the public interfaces (not all of them are actually importable, check current Svelte for which ones are).

import './ambient.js';

/**
 * @deprecated In Svelte 4, components are classes. In Svelte 5, they are functions.
 * Use `mount` or `createRoot` instead to instantiate components.
 * See [breaking changes](https://svelte-5-preview.vercel.app/docs/breaking-changes#components-are-no-longer-classes)
 * for more info.
 */
export interface ComponentConstructorOptions<
	Props extends Record<string, any> = Record<string, any>
> {
	target: Element | Document | ShadowRoot;
	anchor?: Element;
	props?: Props;
	context?: Map<any, any>;
	hydrate?: boolean;
	intro?: boolean;
	$$inline?: boolean;
}

/**
 * Utility type for ensuring backwards compatibility on a type level that if there's a default slot, add 'children' to the props
 */
type Properties<Props, Slots> = Props &
	(Slots extends { default: any }
		? // This is unfortunate because it means "accepts no props" turns into "accepts any prop"
			// but the alternative is non-fixable type errors because of the way TypeScript index
			// signatures work (they will always take precedence and make an impossible-to-satisfy children type).
			Props extends Record<string, never>
Repository: sveltejs/svelte
File: packages/svelte/types/index.d.ts (TypeScript)

Code before:
declare module 'svelte' {
	/**
	 * @deprecated Svelte components were classes in Svelte 4. In Svelte 5, they are not anymore.
	 * Use `mount` or `createRoot` instead to instantiate components.
	 * See [breaking changes](https://svelte-5-preview.vercel.app/docs/breaking-changes#components-are-no-longer-classes)
	 * for more info.
	 */
	export interface ComponentConstructorOptions<
		Props extends Record<string, any> = Record<string, any>
	> {
		target: Element | Document | ShadowRoot;
		anchor?: Element;
		props?: Props;
		context?: Map<any, any>;
		hydrate?: boolean;
		intro?: boolean;
		$$inline?: boolean;
	}

	/**
	 * Utility type for ensuring backwards compatibility on a type level that if there's a default slot, add 'children' to the props
	 */
	type Properties<Props, Slots> = Props &
		(Slots extends { default: any }
			? // This is unfortunate because it means "accepts no props" turns into "accepts any prop"
				// but the alternative is non-fixable type errors because of the way TypeScript index
				// signatures work (they will always take precedence and make an impossible-to-satisfy children type).
				Props extends Record<string, never>

Review comment (suggestion):
```suggestion
	 * @deprecated In Svelte 4, components are classes. In Svelte 5, they are functions.
```

Code after:
declare module 'svelte' {
	/**
	 * @deprecated In Svelte 4, components are classes. In Svelte 5, they are functions.
	 * Use `mount` or `createRoot` instead to instantiate components.
	 * See [breaking changes](https://svelte-5-preview.vercel.app/docs/breaking-changes#components-are-no-longer-classes)
	 * for more info.
	 */
	export interface ComponentConstructorOptions<
		Props extends Record<string, any> = Record<string, any>
	> {
		target: Element | Document | ShadowRoot;
		anchor?: Element;
		props?: Props;
		context?: Map<any, any>;
		hydrate?: boolean;
		intro?: boolean;
		$$inline?: boolean;
	}

	/**
	 * Utility type for ensuring backwards compatibility on a type level that if there's a default slot, add 'children' to the props
	 */
	type Properties<Props, Slots> = Props &
		(Slots extends { default: any }
			? // This is unfortunate because it means "accepts no props" turns into "accepts any prop"
				// but the alternative is non-fixable type errors because of the way TypeScript index
				// signatures work (they will always take precedence and make an impossible-to-satisfy children type).
				Props extends Record<string, never>
Repository: sveltejs/svelte
File: packages/svelte/tests/runtime-browser/samples/svelte-component-css-custom-properties/_config.js (JavaScript)

Code before:
		}

		function assert_slider_2() {
			const rail_color1 = target.querySelector('#component1 p');
			const track_color1 = target.querySelector('#component1 span');
			const rail_color2 = target.querySelector('#component2 p');
			const track_color2 = target.querySelector('#component2 span');

			assert_ok(rail_color1);
			assert_ok(track_color1);
			assert_ok(rail_color2);
			assert_ok(track_color2);

			assert.equal(window.getComputedStyle(rail_color1).color, 'rgb(0, 0, 0)');
			assert.equal(window.getComputedStyle(track_color1).color, 'rgb(255, 0, 0)');
			assert.equal(window.getComputedStyle(rail_color2).color, 'rgb(0, 255, 0)');
			assert.equal(window.getComputedStyle(track_color2).color, 'rgb(0, 0, 255)');
			assert.equal(rail_color1.textContent, 'Slider2');
			assert.equal(rail_color2.textContent, 'Slider2');
		}

		assert_slider_1();
		component.componentName = 'Slider2';
		assert_slider_2();
		component.componentName = 'Slider1';
		assert_slider_1();
	}
});


Review comment (bug):
Was confused by this deletion, and checked the current Svelte 4 REPL once more:
- when `this` is falsy initially, there's a `div`
- but if it's switching truhy from to falsy, then it will remove the wrapping `div`

--> feels like a bug in Svelte 4
--> what does this say about this change? Can we do this change after all, or not? I think having it falsy initially is very rare, so very few people would've run into this bug before. Always having the `div` might catch people's CSS selectors offguard. 

Code after:
		}

		function assert_slider_2() {
			const rail_color1 = target.querySelector('#component1 p');
			const track_color1 = target.querySelector('#component1 span');
			const rail_color2 = target.querySelector('#component2 p');
			const track_color2 = target.querySelector('#component2 span');

			assert_ok(rail_color1);
			assert_ok(track_color1);
			assert_ok(rail_color2);
			assert_ok(track_color2);

			assert.equal(window.getComputedStyle(rail_color1).color, 'rgb(0, 0, 0)');
			assert.equal(window.getComputedStyle(track_color1).color, 'rgb(255, 0, 0)');
			assert.equal(window.getComputedStyle(rail_color2).color, 'rgb(0, 255, 0)');
			assert.equal(window.getComputedStyle(track_color2).color, 'rgb(0, 0, 255)');
			assert.equal(rail_color1.textContent, 'Slider2');
			assert.equal(rail_color2.textContent, 'Slider2');
		}

		assert_slider_1();
		component.componentName = 'Slider2';
		assert_slider_2();
		component.componentName = undefined;
		assert.equal(window.document.querySelector('div')?.firstElementChild, null);
		component.componentName = 'Slider1';
		assert_slider_1();
	}
});