ronantakizawa/codereview-bench huggingface dataset:
Repository: google/iosched
File: mobile/src/main/java/com/google/samples/apps/iosched/ui/onboarding/WelcomePostConferenceFragment.kt (Kotlin)

Code before:

import android.os.Bundle
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import androidx.fragment.app.Fragment
import androidx.fragment.app.activityViewModels
import com.google.samples.apps.iosched.R

/**
 * First page of onboarding showing a welcome message post the conference.
 */
class WelcomePostConferenceFragment : Fragment() {

    private val viewModel: OnboardingViewModel by activityViewModels()

    override fun onCreateView(
        inflater: LayoutInflater,
        container: ViewGroup?,
        savedInstanceState: Bundle?
    ): View? {
        return inflater.inflate(R.layout.fragment_onboarding_welcome_post, container, false)
    }

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        viewModel.reportFullyDrawn()
    }
}


Review comment (suggestion):
OnboardingActivity doesn't gate `reportFullyDrawn()` with `doOnLayout` or similar, so I wonder if this will report too soon (the view is created, but is it actually shown yet?)

Code after:

import android.os.Bundle
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import androidx.core.view.doOnLayout
import androidx.fragment.app.Fragment
import com.google.samples.apps.iosched.R

/**
 * First page of onboarding showing a welcome message post the conference.
 */
class WelcomePostConferenceFragment : Fragment() {

    override fun onCreateView(
        inflater: LayoutInflater,
        container: ViewGroup?,
        savedInstanceState: Bundle?
    ): View? {
        return inflater.inflate(R.layout.fragment_onboarding_welcome_post, container, false)
    }

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        view.doOnLayout {
            activity?.reportFullyDrawn()
        }
    }
}
Repository: goplus/gop
File: cl/compile.go (Go)

Code before:
	return false
}

func (p *pkgCtx) handleRecover(e any, src ast.Node) {
	err := p.recoverErr(e, src)
	p.handleErr(err)
}

func (p *pkgCtx) recoverErr(e any, src ast.Node) error {
	err, ok := e.(error)
	if !ok {
		if src != nil {
			text := p.LoadExpr(src)
			err = p.newCodeErrorf(src.Pos(), "compile `%v`: %v", text, e)
		} else {
			err = fmt.Errorf("%v", e)
		}
	}
	return err
}

// lookupClassFile looks up the class file by name.
func (p *pkgCtx) lookupClassNode(name string) ast.Node {
	for f, cls := range p.classes {
		if name == cls.clsfile {
			return &EmptyNode{
				pos: f.Name.Pos(),
			}
		}
	}
	return nil
}

type EmptyNode struct {
	pos token.Pos
}

func (c *EmptyNode) Pos() token.Pos {
	return c.pos
}

func (c *EmptyNode) End() token.Pos {
	return c.pos
}

const (
	defaultGoFile  = ""
	skippingGoFile = "_skip"
	testingGoFile  = "_test"
)


Review comment (suggestion):
Now f.Name is a implicit package name and has correct `End()` position.
See https://github.com/goplus/gop/pull/2354


Code after:
	return false
}

func (p *pkgCtx) handleRecover(e any, src ast.Node) {
	err := p.recoverErr(e, src)
	p.handleErr(err)
}

func (p *pkgCtx) recoverErr(e any, src ast.Node) error {
	err, ok := e.(error)
	if !ok {
		if src != nil {
			text := p.LoadExpr(src)
			err = p.newCodeErrorf(src.Pos(), "compile `%v`: %v", text, e)
		} else {
			err = fmt.Errorf("%v", e)
		}
	}
	return err
}

// lookupClassFile looks up the class file by name.
func (p *pkgCtx) lookupClassNode(name string) ast.Node {
	for f, cls := range p.classes {
		if name == cls.clsfile {
			return f.Name
		}
	}
	return nil
}

const (
	defaultGoFile  = ""
	skippingGoFile = "_skip"
	testingGoFile  = "_test"
)

// NewPackage creates a Go+ package instance.
func NewPackage(pkgPath string, pkg *ast.Package, conf *Config) (p *gogen.Package, err error) {
	relBaseDir := conf.RelativeBase
	fset := conf.Fset
	files := pkg.Files
	interp := &nodeInterp{
		fset: fset, files: files, relBaseDir: relBaseDir,
	}
	ctx := &pkgCtx{
		fset:       fset,
		nodeInterp: interp,
		projs:      make(map[string]*gmxProject),
		classes:    make(map[*ast.File]*gmxClass),
		overpos:    make(map[string]token.Pos),
Repository: goplus/gop
File: x/typesutil/check.go (Go)

Code before:
						onErr(ce)
					}
				}
			} else if ce, ok := convErr(fset, err); ok {
				onErr(ce)
			} else {
				onErr(err)
			}
		}
		if debugPrintErr {
			log.Println("typesutil.Check err:", err)
			log.SingleStack()
		}
		// don't return even if err != nil
	}
	if len(files) > 0 {
		onErr := conf.Error
		if onErr != nil {
			conf.Error = func(err error) {
				if e, ok := convGoErr(err); ok {
					onErr(e)
				}
			}
		}
		scope := pkgTypes.Scope()
		objMap := DeleteObjects(scope, files)
		checker := types.NewChecker(conf, fset, pkgTypes, p.goInfo)
		err = checker.Files(files)
		// TODO(xsw): how to process error?
		CorrectTypesInfo(scope, objMap, p.xgoInfo.Uses)
		if opts.UpdateGoTypesOverload {
			gogen.InitThisGopPkg(pkgTypes)
		}
	}
	return
}

type astIdent interface {
	comparable
	ast.Node
}

type objMapT = map[types.Object]types.Object

// CorrectTypesInfo corrects types info to avoid there are two instances for the same Go object.
func CorrectTypesInfo[Ident astIdent](scope *types.Scope, objMap objMapT, uses map[Ident]types.Object) {
	for o := range objMap {
		objMap[o] = scope.Lookup(o.Name())
	}
	for id, old := range uses {
		if new := objMap[old]; new != nil {

Review comment (suggestion):
Field `conf.Error` should not be modified in `(*Checker).Files` function.

Code after:
		Outline:        opts.IgnoreFuncBodies,
	})
	if err != nil {
		if onErr := conf.Error; onErr != nil {
			if list, ok := err.(errors.List); ok {
				for _, e := range list {
					if ce, ok := convErr(fset, e); ok {
						onErr(ce)
					}
				}
			} else if ce, ok := convErr(fset, err); ok {
				onErr(ce)
			} else {
				onErr(err)
			}
		}
		if debugPrintErr {
			log.Println("typesutil.Check err:", err)
			log.SingleStack()
		}
		// don't return even if err != nil
	}
	if len(files) > 0 {
		onErr := conf.Error
		if onErr != nil {
			conf.Error = func(err error) {
				if e, ok := convGoErr(err); ok {
					onErr(e)
				}
			}
		}
		scope := pkgTypes.Scope()
		objMap := DeleteObjects(scope, files)
		checker := types.NewChecker(conf, fset, pkgTypes, p.goInfo)
		err = checker.Files(files)
		// TODO(xsw): how to process error?
		CorrectTypesInfo(scope, objMap, p.xgoInfo.Uses)
		if opts.UpdateGoTypesOverload {
			gogen.InitThisGopPkg(pkgTypes)
		}
	}
	return
}

type astIdent interface {
	comparable
	ast.Node
}

type objMapT = map[types.Object]types.Object
Repository: gradio-app/gradio
File: js/upload/src/Upload.svelte (Svelte)

Code before:
			if (
				file_extension && Array.isArray(filetype)
					? filetype.includes(file_extension)
					: file_extension === filetype
			) {
				return true;
			}
			dispatch("error", `Invalid file type only ${filetype} allowed.`);
			return false;
		});

		if (format != "blob") {
			await load_files(files_to_load);
		} else {
			if (file_count === "single") {
				dispatch("load", files_to_load[0]);
				return;
			}
			dispatch("load", files_to_load);
		}
	}

	export async function loadFilesFromDrop(e: DragEvent): Promise<void> {
		dragging = false;
		if (!e.dataTransfer?.files) return;
		const files_to_load = Array.from(e.dataTransfer.files).filter((file) => {
			const file_extension = "." + file.name.split(".").pop();
			if (
				file_extension &&
				is_valid_mimetype(accept_file_types, file_extension, file.type)
			) {
				return true;
			}
			if (
				file_extension && Array.isArray(filetype)
					? filetype.includes(file_extension)
					: file_extension === filetype
			) {
				return true;
			}
			dispatch("error", `Invalid file type only ${filetype} allowed.`);
			return false;
		});

		if (format != "blob") {
			await load_files(files_to_load);
		} else {
			if (file_count === "single") {
				dispatch("load", files_to_load[0]);
				return;
			}

Review comment (suggestion):
You can just use `is_valid_file` instead of implementing a new filter function

```js
		const files_to_load = Array.from(e.dataTransfer.files).filter(is_valid_file);
```

Code after:
			if (
				file_extension && Array.isArray(filetype)
					? filetype.includes(file_extension)
					: file_extension === filetype
			) {
				return true;
			}
			dispatch("error", `Invalid file type only ${filetype} allowed.`);
			return false;
		});

		if (format != "blob") {
			await load_files(files_to_load);
		} else {
			if (file_count === "single") {
				dispatch("load", files_to_load[0]);
				return;
			}
			dispatch("load", files_to_load);
		}
	}

	export async function load_files_from_drop(e: DragEvent): Promise<void> {
		dragging = false;
		if (!e.dataTransfer?.files) return;
		const files_to_load = Array.from(e.dataTransfer.files).filter(
			is_valid_file
		);

		if (format != "blob") {
			await load_files(files_to_load);
		} else {
			if (file_count === "single") {
				dispatch("load", files_to_load[0]);
				return;
			}
			dispatch("load", files_to_load);
		}
	}
</script>

{#if filetype === "clipboard"}
	<button
		class:hidden
		class:center
		class:boundedheight
		class:flex
		class:icon-mode={icon_upload}
		style:height={icon_upload
			? ""
			: height
Repository: gradio-app/gradio
File: js/json/shared/JSONNode.svelte (Svelte)

Code before:
<script lang="ts">
	import { onMount, createEventDispatcher, tick, afterUpdate } from "svelte";

	export let value: any;
	export let depth = 0;
	export let is_root = false;
	export let is_last_item = true;
	export let key: string | number | null = null;
	export let open = false;
	export let theme_mode: "system" | "light" | "dark" = "system";
	export let show_indices = false;

	const dispatch = createEventDispatcher();
	let root_element: HTMLElement;
	let collapsed = open ? depth >= 3 : true;
	let child_nodes: any[] = [];

	function is_collapsible(val: any): boolean {
		return val !== null && (typeof val === "object" || Array.isArray(val));
	}

	async function toggle_collapse(): Promise<void> {
		collapsed = !collapsed;
		await tick();
		dispatch("toggle", { collapsed, depth });
	}

	function get_collapsed_preview(val: any): string {
		if (Array.isArray(val)) return `Array(${val.length})`;
		if (typeof val === "object" && val !== null)
			return `Object(${Object.keys(val).length})`;
		return String(val);
	}

	$: if (is_collapsible(value)) {
		child_nodes = Object.entries(value);
	} else {
		child_nodes = [];
	}
	$: if (is_root && root_element) {

Review comment (suggestion):
I don't think this logic was correct before. If open was True, then it would be closed? It meant that the open parameter had no effect. Since the open parameter now works, I changed the default value in the python class to maintain the current behavior of having it open unless it was too deep.

Code after:
<script lang="ts">
	import { onMount, createEventDispatcher, tick, afterUpdate } from "svelte";

	export let value: any;
	export let depth = 0;
	export let is_root = false;
	export let is_last_item = true;
	export let key: string | number | null = null;
	export let open = false;
	export let theme_mode: "system" | "light" | "dark" = "system";
	export let show_indices = false;
	export let interactive = true;

	const dispatch = createEventDispatcher();
	let root_element: HTMLElement;
	let collapsed = open ? false : depth >= 3;
	let child_nodes: any[] = [];

	function is_collapsible(val: any): boolean {
		return val !== null && (typeof val === "object" || Array.isArray(val));
	}

	async function toggle_collapse(): Promise<void> {
		collapsed = !collapsed;
		await tick();
		dispatch("toggle", { collapsed, depth });
	}

	function get_collapsed_preview(val: any): string {
		if (Array.isArray(val)) return `Array(${val.length})`;
		if (typeof val === "object" && val !== null)
			return `Object(${Object.keys(val).length})`;
		return String(val);
	}

	$: if (is_collapsible(value)) {
		child_nodes = Object.entries(value);
	} else {
		child_nodes = [];
	}
Repository: gradio-app/gradio
File: js/dataframe/shared/EditableCell.svelte (Svelte)

Code before:
	}

	function handle_keydown(event: KeyboardEvent): void {
		if (event.key === "Enter") {
			if (!header) {
				is_expanded = !is_expanded;
			}
		}
		dispatch("keydown", event);
	}

	function handle_click(): void {
		if (!edit && !header) {
			is_expanded = !is_expanded;
		}
	}

	function handle_bool_change(new_value: boolean): void {
		value = new_value.toString();
		const event = new FocusEvent("blur");
		dispatch("blur", {
			blur_event: event,
			coords: coords
		});
	}
</script>

{#if edit && datatype !== "bool"}
	<input
		readonly={is_static}
		aria-readonly={is_static}
		role="textbox"
		aria-label={is_static ? "Cell is read-only" : "Edit cell"}
		bind:this={el}
		bind:value
		class:header
		tabindex="-1"
		on:blur={handle_blur}
		on:mousedown|stopPropagation
		on:mouseup|stopPropagation
		on:click|stopPropagation
		use:use_focus
		on:keydown={handle_keydown}
	/>
{/if}

{#if datatype === "bool"}
	<BooleanCell
		value={String(display_content)}
		{editable}
		{bool_input}

Review comment (suggestion):
This doesn't seem to work because in `handle_cell_blur` in `keyboard_utils.ts`, `input_el` is `null`...
So it doesn't proceed to `save_cell_value`

Code after:
		if (event.key === "Enter") {
			if (!header) {
				is_expanded = !is_expanded;
			}
		}
		dispatch("keydown", event);
	}

	function handle_click(): void {
		if (!edit && !header) {
			is_expanded = !is_expanded;
		}
	}

	function handle_bool_change(new_value: boolean): void {
		value = new_value.toString();
		dispatch("blur", {
			blur_event: {
				target: {
					type: "checkbox",
					checked: new_value,
					value: new_value.toString()
				}
			} as unknown as FocusEvent,
			coords: coords
		});
	}
</script>

{#if edit && datatype !== "bool"}
	<input
		readonly={is_static}
		aria-readonly={is_static}
		role="textbox"
		aria-label={is_static ? "Cell is read-only" : "Edit cell"}
		bind:this={el}
		bind:value
		class:header
		tabindex="-1"
		on:blur={handle_blur}
		on:mousedown|stopPropagation
		on:mouseup|stopPropagation
		on:click|stopPropagation
		use:use_focus
		on:keydown={handle_keydown}
	/>
{/if}

{#if datatype === "bool"}
	<BooleanCell
		value={String(display_content)}
Repository: gradio-app/gradio
File: js/dataframe/shared/BooleanCell.svelte (Svelte)

Code before:
<script lang="ts">
	import type { BoolInputType } from "./utils";
	import { BaseCheckbox } from "@gradio/checkbox";

	export let value: boolean | string = false;
	export let editable = true;
	export let bool_input: BoolInputType = "checkbox";
	export let on_change: (value: boolean) => void;

	$: bool_value =
		typeof value === "string" ? value.toLowerCase() === "true" : !!value;

	function handle_change(event: CustomEvent<boolean>): void {
		on_change(event.detail);
	}
</script>

{#if bool_input === "checkbox"}
	<div class="bool-cell checkbox">
		<BaseCheckbox
			bind:value={bool_value}
			label=""
			interactive={editable}
			on:change={handle_change}
		/>
	</div>
{:else}
	<div class="bool-cell text">

Review comment (suggestion):
Should add this prop to `BooleanCell` and `Checkbox` as well:
```ts
export let el: HTMLInputElement | null;
```

Code after:
<script lang="ts">
	import { BaseCheckbox } from "@gradio/checkbox";

	export let value: boolean | string = false;
	export let editable = true;
	export let on_change: (value: boolean) => void;

	$: bool_value =
		typeof value === "string" ? value.toLowerCase() === "true" : !!value;

	function handle_change(event: CustomEvent<boolean>): void {
		on_change(event.detail);
	}

	function handle_click(event: MouseEvent): void {
		event.stopPropagation();
	}

	function handle_keydown(event: KeyboardEvent): void {
		if (event.key === "Enter" || event.key === " ") {
			event.stopPropagation();
		}
	}
</script>

<div
	class="bool-cell checkbox"
	on:click={handle_click}
Repository: gradio-app/gradio
File: js/dataframe/shared/BooleanCell.svelte (Svelte)

Code before:
<script lang="ts">
	import type { BoolInputType } from "./utils";
	import { BaseCheckbox } from "@gradio/checkbox";

	export let value: boolean | string = false;
	export let editable = true;
	export let bool_input: BoolInputType = "checkbox";
	export let on_change: (value: boolean) => void;

	$: bool_value =
		typeof value === "string" ? value.toLowerCase() === "true" : !!value;

	function handle_change(event: CustomEvent<boolean>): void {
		on_change(event.detail);
	}
</script>

{#if bool_input === "checkbox"}
	<div class="bool-cell checkbox">
		<BaseCheckbox
			bind:value={bool_value}
			label=""
			interactive={editable}
			on:change={handle_change}
		/>
	</div>
{:else}
	<div class="bool-cell text">
		{bool_value ? "true" : "false"}
	</div>
{/if}

<style>
	.bool-cell {
		display: flex;
		align-items: center;
		justify-content: center;
		width: var(--size-full);
		height: var(--size-full);
	}

	.bool-cell.checkbox {
		justify-content: center;
	}


Review comment (suggestion):
should add
```ts
bind:el
```

and to `Checkbox`:
```ts
bind:this={el}
```

Code after:
<script lang="ts">
	import { BaseCheckbox } from "@gradio/checkbox";

	export let value: boolean | string = false;
	export let editable = true;
	export let on_change: (value: boolean) => void;

	$: bool_value =
		typeof value === "string" ? value.toLowerCase() === "true" : !!value;

	function handle_change(event: CustomEvent<boolean>): void {
		on_change(event.detail);
	}

	function handle_click(event: MouseEvent): void {
		event.stopPropagation();
	}

	function handle_keydown(event: KeyboardEvent): void {
		if (event.key === "Enter" || event.key === " ") {
			event.stopPropagation();
		}
	}
</script>

<div
	class="bool-cell checkbox"
	on:click={handle_click}
	on:keydown={handle_keydown}
	role="button"
	tabindex="-1"
>
	<BaseCheckbox
		bind:value={bool_value}
		label=""
		interactive={editable}
		on:change={handle_change}
	/>
</div>

<style>
	.bool-cell {
		display: flex;
		align-items: center;
		justify-content: center;
Repository: gradio-app/gradio
File: js/dataframe/shared/EditableCell.svelte (Svelte)

Code before:
		if (event.key === "Enter") {
			if (!header) {
				is_expanded = !is_expanded;
			}
		}
		dispatch("keydown", event);
	}

	function handle_click(): void {
		if (!edit && !header) {
			is_expanded = !is_expanded;
		}
	}

	function handle_bool_change(new_value: boolean): void {
		value = new_value.toString();
	}
</script>

{#if edit && datatype !== "bool"}
	<input
		readonly={is_static}
		aria-readonly={is_static}
		role="textbox"
		aria-label={is_static ? "Cell is read-only" : "Edit cell"}
		bind:this={el}
		bind:value
		class:header
		tabindex="-1"
		on:blur={handle_blur}
		on:mousedown|stopPropagation
		on:mouseup|stopPropagation
		on:click|stopPropagation
		use:use_focus
		on:keydown={handle_keydown}
	/>
{/if}

{#if datatype === "bool"}
	<BooleanCell
		bind:el
		value={String(display_content)}
		{editable}
		{bool_input}
		on_change={handle_bool_change}
	/>
{:else}
	<span
		class:dragging={is_dragging}
		on:click={handle_click}
		on:keydown={handle_keydown}

Review comment (suggestion):
This shouldn't be removed - the events `change` and `input` no longer emit

Code after:
			if (!header) {
				is_expanded = !is_expanded;
			}
		}
		dispatch("keydown", event);
	}

	function handle_click(): void {
		if (!edit && !header) {
			is_expanded = !is_expanded;
		}
	}

	function handle_bool_change(new_value: boolean): void {
		value = new_value.toString();
		dispatch("blur", {
			blur_event: {
				target: {
					type: "checkbox",
					checked: new_value,
					value: new_value.toString()
				}
			} as unknown as FocusEvent,
			coords: coords
		});
	}
</script>

{#if edit && datatype !== "bool"}
	<input
		readonly={is_static}
		aria-readonly={is_static}
		role="textbox"
		aria-label={is_static ? "Cell is read-only" : "Edit cell"}
		bind:this={el}
		bind:value
		class:header
		tabindex="-1"
		on:blur={handle_blur}
		on:mousedown|stopPropagation
		on:mouseup|stopPropagation
		on:click|stopPropagation
		use:use_focus
		on:keydown={handle_keydown}
	/>
{/if}

{#if datatype === "bool"}
	<BooleanCell
		value={String(display_content)}
		{editable}
Repository: gradio-app/gradio
File: js/spa/test/dataframe_colorful.spec.ts (TypeScript)

Code before:
import { test, expect } from "@self/tootils";

test("table rows are highlighted correctly", async ({ page }) => {
	const first_tr = await page.locator("tbody tr").first();
	await expect(first_tr).toHaveCSS("background-color", "rgb(17, 24, 39)");

	const second_tr = await page.locator("tbody tr").nth(1);
	await expect(second_tr).toHaveCSS("background-color", "rgb(11, 15, 25)");
});


Review comment (suggestion):
I think these might need to be switched to the light theme, but I can't get the tests to run locally. I think I might have messed up the PR bot by force pushing over the changelog file, because the tests didn't start.

Code after:
import { test, expect } from "@self/tootils";

test("table rows are highlighted correctly", async ({ page }) => {
	const first_tr = await page.locator("tbody tr").first();
	await expect(first_tr).toHaveCSS("background-color", "rgb(255, 255, 255)");

	const second_tr = await page.locator("tbody tr").nth(1);
	await expect(second_tr).toHaveCSS("background-color", "rgb(250, 250, 250)");
});
Repository: gradio-app/gradio
File: js/dataframe/shared/RowNumber.svelte (Svelte)

Code before:
	<td class="row-number" tabindex="-1" data-row={index} data-col="row-number">
		{index !== null ? index + 1 : ""}
	</td>
{/if}

<style>
	.row-number {
		text-align: center;
		padding: var(--size-1);
		min-width: var(--size-12);
		width: var(--size-12);
		overflow: hidden;
		text-overflow: ellipsis;
		white-space: nowrap;
		font-weight: var(--weight-semibold);
		background: var(--table-even-background-fill);
		border-right: 1px solid var(--border-color-primary);
	}

	:global(tr:nth-child(odd)) .row-number {
		background: var(--table-odd-background-fill);
	}

	:global(tr:nth-child(even)) .row-number {
		background: var(--table-even-background-fill);
	}
</style>


Review comment (bug):
Row number had the same bug, and didn't actually need the `td` to override the `tr` background.

Code after:
<script lang="ts">
	export let index: number | null = null;
	export let is_header = false;
</script>

{#if is_header}
	<th tabindex="-1" class="row-number">
		<div class="cell-wrap">
			<div class="header-content">
				<div class="header-text"></div>
			</div>
		</div>
	</th>
{:else}
	<td class="row-number" tabindex="-1" data-row={index} data-col="row-number">
		{index !== null ? index + 1 : ""}
	</td>
{/if}

<style>
	.row-number {
		text-align: center;
		padding: var(--size-1);
		min-width: var(--size-12);
		width: var(--size-12);
		overflow: hidden;
		text-overflow: ellipsis;
		white-space: nowrap;
		font-weight: var(--weight-semibold);
		border-right: 1px solid var(--border-color-primary);
	}
</style>