ronantakizawa/codereview-bench huggingface dataset:
Repository: pmndrs/zustand
File: tests/devtools.test.tsx (TypeScript)

Code before:
  it("sends { type: setStateName || 'setCount`, ...rest } as the action with current state", async () => {
    const options = {
      name: 'testOptionsName',
      enabled: true,
      inferActionName: true,
    }

    const api = createStore(
      devtools(
        (set) => ({
          count: 0,
          setCount: (newCount: number) => {
            set({ count: newCount })
          },
        }),
        options,
      ),
    )
    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
    // @ts-ignore
    api.getState().setCount(10)
    const [connection] = getNamedConnectionApis(options.name)
    expect(connection.send).toHaveBeenLastCalledWith(
      { type: 'Object.setCount' },
      { count: 10, setCount: expect.any(Function) },
    )
  })
})

describe('when it receives a message of type...', () => {
  describe('ACTION...', () => {
    it('does nothing', async () => {
      const initialState = { count: 0 }
      const api = createStore(devtools(() => initialState, { enabled: true }))
      const setState = vi.spyOn(api, 'setState')

      const [subscriber] = getNamedConnectionSubscribers(undefined)
      subscriber({
        type: 'ACTION',
        payload: '{ "type": "INCREMENT" }',
      })

      expect(api.getState()).toBe(initialState)
      expect(setState).not.toBeCalled()
    })

    it('unless action type is __setState', async () => {
      const initialState = { count: 0 }
      const api = createStore(devtools(() => initialState, { enabled: true }))

      const [connectionSubscriber] = getNamedConnectionSubscribers(undefined)

Review comment (suggestion):
I don't know how to type this, I couldn't find a similar usage in this test

Code after:
  })
})

describe('When state changes with automatic setter inferring...', () => {
  it("sends { type: setStateName || 'setCount`, ...rest } as the action with current state", async () => {
    const options = {
      name: 'testOptionsName',
      enabled: true,
    }

    const api = createStore<{
      count: number
      setCount: (count: number) => void
    }>()(
      devtools(
        (set) => ({
          count: 0,
          setCount: (newCount: number) => {
            set({ count: newCount })
          },
        }),
        options,
      ),
    )

    api.getState().setCount(10)
    const [connection] = getNamedConnectionApis(options.name)
    expect(connection.send).toHaveBeenLastCalledWith(
      { type: 'Object.setCount' },
      { count: 10, setCount: expect.any(Function) },
    )
  })
})

describe('when it receives a message of type...', () => {
  describe('ACTION...', () => {
    it('does nothing', async () => {
      const initialState = { count: 0 }
      const api = createStore(devtools(() => initialState, { enabled: true }))
      const setState = vi.spyOn(api, 'setState')

      const [subscriber] = getNamedConnectionSubscribers(undefined)
      subscriber({
        type: 'ACTION',
        payload: '{ "type": "INCREMENT" }',
      })

      expect(api.getState()).toBe(initialState)
      expect(setState).not.toBeCalled()
    })
Repository: pmndrs/zustand
File: src/vanilla/shallow.ts (TypeScript)

Code before:
// Ordered iterables
const compareIterables = (
  valueA: Iterable<unknown>,
  valueB: Iterable<unknown>,
) => {
  const iteratorA = valueA[Symbol.iterator]()
  const iteratorB = valueB[Symbol.iterator]()
  let nextA = iteratorA.next()
  let nextB = iteratorB.next()
  while (!nextA.done && !nextB.done) {
    if (!Object.is(nextA.value, nextB.value)) {
      return false
    }
    nextA = iteratorA.next()
    nextB = iteratorB.next()
  }
  return !!nextA.done && !!nextB.done
}

export function shallow<T>(valueA: T, valueB: T): boolean {
  if (Object.is(valueA, valueB)) {
    return true
  }
  if (type(valueA) !== type(valueB)) {
    return false
  }
  if (
    typeof valueA !== 'object' ||
    valueA === null ||
    typeof valueB !== 'object' ||
    valueB === null
  ) {
    return false
  }
  if (!isIterable(valueA) || !isIterable(valueB)) {
    return compareEntries(
      { entries: () => Object.entries(valueA) },
      { entries: () => Object.entries(valueB) },
    )
  }
  if (hasIterableEntries(valueA) && hasIterableEntries(valueB)) {
    return compareEntries(valueA, valueB)
  }
  return compareIterables(valueA, valueB)
}


Review comment (suggestion):
Clever! I didn't know about the `Object.prototype.toString` trick. I think the block below be removed

```
if (
    typeof valueA !== 'object' ||
    valueA === null ||
    typeof valueB !== 'object' ||
    valueB === null
  ) {
    return false
  }
  ```

Code after:
  valueB: Iterable<unknown>,
) => {
  const iteratorA = valueA[Symbol.iterator]()
  const iteratorB = valueB[Symbol.iterator]()
  let nextA = iteratorA.next()
  let nextB = iteratorB.next()
  while (!nextA.done && !nextB.done) {
    if (!Object.is(nextA.value, nextB.value)) {
      return false
    }
    nextA = iteratorA.next()
    nextB = iteratorB.next()
  }
  return !!nextA.done && !!nextB.done
}

export function shallow<T>(valueA: T, valueB: T): boolean {
  if (Object.is(valueA, valueB)) {
    return true
  }
  if (
    typeof valueA !== 'object' ||
    valueA === null ||
    typeof valueB !== 'object' ||
    valueB === null
  ) {
    return false
  }
  if (Object.getPrototypeOf(valueA) !== Object.getPrototypeOf(valueB)) {
    return false
  }
  if (isIterable(valueA) && isIterable(valueB)) {
    if (hasIterableEntries(valueA) && hasIterableEntries(valueB)) {
      return compareEntries(valueA, valueB)
    }
    return compareIterables(valueA, valueB)
  }
  // assume plain objects
  return compareEntries(
    { entries: () => Object.entries(valueA) },
    { entries: () => Object.entries(valueB) },
  )
}
Repository: pmndrs/zustand
File: src/vanilla/shallow.ts (TypeScript)

Code before:
const isIterable = (obj: object): obj is Iterable<unknown> =>
  Symbol.iterator in obj

const type = <T>(obj: T): string =>
  Object.prototype.toString.apply(obj).slice(8, -1).toLowerCase()

const hasIterableEntries = (
  value: Iterable<unknown>,
): value is Iterable<unknown> & {
  entries(): Iterable<[unknown, unknown]>
} =>
  // HACK: avoid checking entries type
  'entries' in value

const compareEntries = (
  valueA: { entries(): Iterable<[unknown, unknown]> },
  valueB: { entries(): Iterable<[unknown, unknown]> },
) => {
  const mapA = valueA instanceof Map ? valueA : new Map(valueA.entries())
  const mapB = valueB instanceof Map ? valueB : new Map(valueB.entries())
  if (mapA.size !== mapB.size) {
    return false
  }
  for (const [key, value] of mapA) {
    if (!Object.is(value, mapB.get(key))) {
      return false
    }
  }
  return true
}


Review comment (suggestion):
I don't think this is very nice and acceptable. If we want to distinguish an array, use `Array.isArray()`.

Code after:
const isIterable = (obj: object): obj is Iterable<unknown> =>
  Symbol.iterator in obj

const hasIterableEntries = (
  value: Iterable<unknown>,
): value is Iterable<unknown> & {
  entries(): Iterable<[unknown, unknown]>
} =>
  // HACK: avoid checking entries type
  'entries' in value

const compareEntries = (
  valueA: { entries(): Iterable<[unknown, unknown]> },
  valueB: { entries(): Iterable<[unknown, unknown]> },
) => {
  const mapA = valueA instanceof Map ? valueA : new Map(valueA.entries())
  const mapB = valueB instanceof Map ? valueB : new Map(valueB.entries())
  if (mapA.size !== mapB.size) {
    return false
  }
  for (const [key, value] of mapA) {
    if (!Object.is(value, mapB.get(key))) {
      return false
    }
  }
  return true
}

// Ordered iterables
const compareIterables = (
  valueA: Iterable<unknown>,
Repository: pocketbase/pocketbase
File: apis/health.go (Go)

Code before:
package apis

import (
	"github.com/labstack/echo/v5"
	"github.com/pocketbase/pocketbase/core"
	"net/http"
)

// bindHealthApi registers the health api endpoint.
func bindHealthApi(app core.App, rg *echo.Group) {
	api := healthApi{app: app}

	subGroup := rg.Group("/health")
	subGroup.GET("", api.healthCheck)
}

type healthApi struct {
	app core.App
}

// healthCheck returns a 200 OK response if the server is healthy.
func (api *healthApi) healthCheck(c echo.Context) error {
	payload := map[string]any{
		"code":    http.StatusOK,
		"status":  "ok",
		"message": "API is healthy.",
	}
	return c.JSON(http.StatusOK, payload)
}


Review comment (suggestion):
If `status` is not required by some 3rd party tool, let's minimize the returned fields and remove it from the response since it could change in the future (the `code` and 200 response should be enough indication for healthy state).

Code after:
package apis

import (
	"github.com/labstack/echo/v5"
	"github.com/pocketbase/pocketbase/core"
	"net/http"
)

// bindHealthApi registers the health api endpoint.
func bindHealthApi(app core.App, rg *echo.Group) {
	api := healthApi{app: app}

	subGroup := rg.Group("/health")
	subGroup.GET("", api.healthCheck)
}

type healthApi struct {
	app core.App
}

// healthCheck returns a 200 OK response if the server is healthy.
func (api *healthApi) healthCheck(c echo.Context) error {
	payload := map[string]any{
		"code":    http.StatusOK,
		"message": "API is healthy.",
	}
	return c.JSON(http.StatusOK, payload)
}
Repository: pocketbase/pocketbase
File: tools/filesystem/file.go (Go)

Code before:
	Path string
}

// Open implements the [filesystem.FileReader] interface.
func (r *PathReader) Open() (io.ReadSeekCloser, error) {
	return os.Open(r.Path)
}

// -------------------------------------------------------------------

var _ FileReader = (*BufReader)(nil)

type BufReader struct {
	Bytes []byte
}

// Open implements the [filesystem.FileReader] interface.
func (r *BufReader) Open() (io.ReadSeekCloser, error) {
	return NewBytesReadSeekCloser(r.Bytes), nil
}

type BytesReadSeekCloser struct {
	*bytes.Reader
}

func NewBytesReadSeekCloser(b []byte) *BytesReadSeekCloser {
	return &BytesReadSeekCloser{bytes.NewReader(b)}
}

func (r *BytesReadSeekCloser) Close() error {
	return nil
}

// -------------------------------------------------------------------

var extInvalidCharsRegex = regexp.MustCompile(`[^\w\.\*\-\+\=\#]+`)

func normalizeName(fr FileReader, name string) string {
	// extension
	// ---
	originalExt := filepath.Ext(name)
	cleanExt := extInvalidCharsRegex.ReplaceAllString(originalExt, "")
	if cleanExt == "" {
		// try to detect the extension from the file content
		cleanExt, _ = detectExtension(fr)
	}

	// name
	// ---
	cleanName := inflector.Snakecase(strings.TrimSuffix(name, originalExt))
	if length := len(cleanName); length < 3 {

Review comment (suggestion):
Since this is currently used only inside the `Open()`, let's change it to `newBytesReadSeekCloser()` to limit the surface of the exported package members (or remove it and directly call the returned struct literal in its place).

Code after:

// -------------------------------------------------------------------

var _ FileReader = (*PathReader)(nil)

type PathReader struct {
	Path string
}

// Open implements the [filesystem.FileReader] interface.
func (r *PathReader) Open() (io.ReadSeekCloser, error) {
	return os.Open(r.Path)
}

// -------------------------------------------------------------------

var _ FileReader = (*BytesReader)(nil)

type BytesReader struct {
	Bytes []byte
}

// Open implements the [filesystem.FileReader] interface.
func (r *BytesReader) Open() (io.ReadSeekCloser, error) {
	return &bytesReadSeekCloser{bytes.NewReader(r.Bytes)}, nil
}

// bytesReadSeekCloser implements io.ReadSeekCloser
type bytesReadSeekCloser struct {
	*bytes.Reader
}

func (r *bytesReadSeekCloser) Close() error {
	return nil
}

// -------------------------------------------------------------------

var extInvalidCharsRegex = regexp.MustCompile(`[^\w\.\*\-\+\=\#]+`)

func normalizeName(fr FileReader, name string) string {
	// extension
	// ---
	originalExt := filepath.Ext(name)
	cleanExt := extInvalidCharsRegex.ReplaceAllString(originalExt, "")
	if cleanExt == "" {
		// try to detect the extension from the file content
		cleanExt, _ = detectExtension(fr)
	}

	// name
Repository: pocketbase/pocketbase
File: tools/filesystem/file.go (Go)

Code before:
func (r *PathReader) Open() (io.ReadSeekCloser, error) {
	return os.Open(r.Path)
}

// -------------------------------------------------------------------

var _ FileReader = (*BufReader)(nil)

type BufReader struct {
	Bytes []byte
}

// Open implements the [filesystem.FileReader] interface.
func (r *BufReader) Open() (io.ReadSeekCloser, error) {
	return NewBytesReadSeekCloser(r.Bytes), nil
}

type BytesReadSeekCloser struct {
	*bytes.Reader
}

func NewBytesReadSeekCloser(b []byte) *BytesReadSeekCloser {
	return &BytesReadSeekCloser{bytes.NewReader(b)}
}

func (r *BytesReadSeekCloser) Close() error {
	return nil
}

// -------------------------------------------------------------------

var extInvalidCharsRegex = regexp.MustCompile(`[^\w\.\*\-\+\=\#]+`)

func normalizeName(fr FileReader, name string) string {
	// extension
	// ---
	originalExt := filepath.Ext(name)
	cleanExt := extInvalidCharsRegex.ReplaceAllString(originalExt, "")
	if cleanExt == "" {
		// try to detect the extension from the file content
		cleanExt, _ = detectExtension(fr)
	}

	// name
	// ---
	cleanName := inflector.Snakecase(strings.TrimSuffix(name, originalExt))
	if length := len(cleanName); length < 3 {
		// the name is too short so we concatenate an additional random part
		cleanName += security.RandomString(10)
	} else if length > 100 {
		// keep only the first 100 characters (it is multibyte safe after Snakecase)

Review comment (suggestion):
Add a comment that this "implements" the `io.ReadSeekCloser` interface to be a little more clear why this method is here, eg:
```
Close implements the [io.ReadSeekCloser] interface.
```

_I'm still not sure whether it would be better to reset/rewind the seeker, but let's keep it with `return nil` for now._

Code after:

type PathReader struct {
	Path string
}

// Open implements the [filesystem.FileReader] interface.
func (r *PathReader) Open() (io.ReadSeekCloser, error) {
	return os.Open(r.Path)
}

// -------------------------------------------------------------------

var _ FileReader = (*BytesReader)(nil)

type BytesReader struct {
	Bytes []byte
}

// Open implements the [filesystem.FileReader] interface.
func (r *BytesReader) Open() (io.ReadSeekCloser, error) {
	return &bytesReadSeekCloser{bytes.NewReader(r.Bytes)}, nil
}

// bytesReadSeekCloser implements io.ReadSeekCloser
type bytesReadSeekCloser struct {
	*bytes.Reader
}

func (r *bytesReadSeekCloser) Close() error {
	return nil
}

// -------------------------------------------------------------------

var extInvalidCharsRegex = regexp.MustCompile(`[^\w\.\*\-\+\=\#]+`)

func normalizeName(fr FileReader, name string) string {
	// extension
	// ---
	originalExt := filepath.Ext(name)
	cleanExt := extInvalidCharsRegex.ReplaceAllString(originalExt, "")
	if cleanExt == "" {
		// try to detect the extension from the file content
		cleanExt, _ = detectExtension(fr)
	}

	// name
	// ---
	cleanName := inflector.Snakecase(strings.TrimSuffix(name, originalExt))
	if length := len(cleanName); length < 3 {
		// the name is too short so we concatenate an additional random part
Repository: pocketbase/pocketbase
File: tools/filesystem/file.go (Go)

Code before:
	Header *multipart.FileHeader
}

// Open implements the [filesystem.FileReader] interface.
func (r *MultipartReader) Open() (io.ReadSeekCloser, error) {
	return r.Header.Open()
}

// -------------------------------------------------------------------

var _ FileReader = (*PathReader)(nil)

type PathReader struct {
	Path string
}

// Open implements the [filesystem.FileReader] interface.
func (r *PathReader) Open() (io.ReadSeekCloser, error) {
	return os.Open(r.Path)
}

// -------------------------------------------------------------------

var _ FileReader = (*bytesReader)(nil)

type bytesReader struct {
	Bytes []byte
}

// Open implements the [filesystem.FileReader] interface.
func (r *bytesReader) Open() (io.ReadSeekCloser, error) {
	return &bytesReadSeekCloser{bytes.NewReader(r.Bytes)}, nil
}

type bytesReadSeekCloser struct {
	*bytes.Reader
}

func (r *bytesReadSeekCloser) Close() error {
	return nil
}

// -------------------------------------------------------------------

var extInvalidCharsRegex = regexp.MustCompile(`[^\w\.\*\-\+\=\#]+`)

func normalizeName(fr FileReader, name string) string {
	// extension
	// ---
	originalExt := filepath.Ext(name)
	cleanExt := extInvalidCharsRegex.ReplaceAllString(originalExt, "")

Review comment (suggestion):
Similar to `PathReader` and `MultipartReader` reader, let's export this because we may use it as embedded struct or as a return type in some other code.

The `bytesReadSeekCloser` can remain unexported.

Code after:
	Header *multipart.FileHeader
}

// Open implements the [filesystem.FileReader] interface.
func (r *MultipartReader) Open() (io.ReadSeekCloser, error) {
	return r.Header.Open()
}

// -------------------------------------------------------------------

var _ FileReader = (*PathReader)(nil)

type PathReader struct {
	Path string
}

// Open implements the [filesystem.FileReader] interface.
func (r *PathReader) Open() (io.ReadSeekCloser, error) {
	return os.Open(r.Path)
}

// -------------------------------------------------------------------

var _ FileReader = (*BytesReader)(nil)

type BytesReader struct {
	Bytes []byte
}

// Open implements the [filesystem.FileReader] interface.
func (r *BytesReader) Open() (io.ReadSeekCloser, error) {
	return &bytesReadSeekCloser{bytes.NewReader(r.Bytes)}, nil
}

// bytesReadSeekCloser implements io.ReadSeekCloser
type bytesReadSeekCloser struct {
	*bytes.Reader
}

func (r *bytesReadSeekCloser) Close() error {
	return nil
}

// -------------------------------------------------------------------

var extInvalidCharsRegex = regexp.MustCompile(`[^\w\.\*\-\+\=\#]+`)

func normalizeName(fr FileReader, name string) string {
	// extension
	// ---
	originalExt := filepath.Ext(name)
Repository: pocketbase/pocketbase
File: ui/src/components/base/PreviewPopup.svelte (Svelte)

Code before:
    export function show(newUrl) {
        if (newUrl === "") {
            return;
        }

        url = newUrl;

        panel?.show();
    }

    export function hide() {
        return panel?.hide();
    }
</script>

<OverlayPanel bind:this={panel} class="preview {type}-preview" btnClose={false} popup on:show on:hide>
    <svelte:fragment slot="header">
        <button type="button" class="overlay-close" on:click|preventDefault={hide}>
            <i class="ri-close-line" />
        </button>
    </svelte:fragment>

    {#if type === "image"}
        <img src={url} alt="Preview {url}" />
    {:else if type === "pdf"}
        <object title={url.substring(url.lastIndexOf("/") + 1)} data={url} type="application/pdf">
            PDF embed not loaded.
        </object>
    {/if}

    <svelte:fragment slot="footer">
        <a
            href={url}
            title="Download"
            target="_blank"
            rel="noreferrer noopener"
            class="link-hint txt-ellipsis"
        >
            <i class="ri-file-download-line" />
            {url.substring(url.lastIndexOf("/") + 1)}
        </a>
        <div class="flex-fill" />
        <button type="button" class="btn btn-secondary" on:click={hide}>Close</button>
    </svelte:fragment>
</OverlayPanel>


Review comment (suggestion):
Since `url.substring(url.lastIndexOf("/") + 1)` is used below too, I think we can move it as computed/reactive declaration, something like:

```js
$: filename = url ? url.substring(url.lastIndexOf("/") + 1) : "";
```

Code after:
    export function show(newUrl) {
        if (newUrl === "") {
            return;
        }

        url = newUrl;

        panel?.show();
    }

    export function hide() {
        return panel?.hide();
    }

    $: filename = url.substring(url.lastIndexOf("/") + 1);
</script>

<OverlayPanel bind:this={panel} class="preview {type}-preview" btnClose={false} popup on:show on:hide>
    <svelte:fragment slot="header">
        <button type="button" class="overlay-close" on:click|preventDefault={hide}>
            <i class="ri-close-line" />
        </button>
    </svelte:fragment>

    {#if type === "image"}
        <img src={url} alt="Preview {url}" />
    {:else if type === "pdf"}
        <object title={filename} data={url} type="application/pdf"> PDF embed not loaded. </object>
    {/if}

    <svelte:fragment slot="footer">
        <a
            href={url}
            title="Download"
            target="_blank"
            rel="noreferrer noopener"
            class="link-hint txt-ellipsis"
        >
            <i class="ri-file-download-line" />
            {filename}
        </a>
        <div class="flex-fill" />
        <button type="button" class="btn btn-secondary" on:click={hide}>Close</button>
    </svelte:fragment>
</OverlayPanel>
Repository: pocketbase/pocketbase
File: ui/src/components/records/RecordFieldCell.svelte (Svelte)

Code before:
        </a>
    {:else if field.type === "date"}
        <FormattedDate date={record[field.name]} />
    {:else if field.type === "json"}
        <span class="txt txt-ellipsis">
            {cutText(JSON.stringify(record[field.name]))}
        </span>
    {:else if field.type === "select"}
        <div class="inline-flex">
            {#each CommonHelper.toArray(record[field.name]) as item, i (i + item)}
                <span class="label">{item}</span>
            {/each}
        </div>
    {:else if field.type === "relation" || field.type === "user"}
        <div class="inline-flex">
            {#each CommonHelper.toArray(record[field.name]).slice(0, 20) as item, i (i + item)}
                <IdLabel id={item} />
            {/each}
            {#if CommonHelper.toArray(record[field.name]).length > 20}
                ...
            {/if}
        </div>
    {:else if field.type === "file"}
        <div class="inline-flex">
            {#each CommonHelper.toArray(record[field.name]) as filename, i (i + filename)}
                <RecordFilePreview {record} {filename} />
            {/each}
        </div>
    {:else}
        <span class="txt txt-ellipsis" title={cutText(record[field.name])}>
            {cutText(record[field.name])}
        </span>
    {/if}
</td>


Review comment (suggestion):
Since now this will be always rendering a thumb, I think we can rename it to `RecordFileThumb` to be more clear. We also will need to remove the `figure` wrapping inside the RecordUpsert panel > FileField (_you can skip the `class:fade` check or keep it and wrap it in another div_).

---

_On second thought, it may be better to keep for now the component generic and maybe leave the figure tag and the thumb classes as they were?_

Code after:
        </a>
    {:else if field.type === "date"}
        <FormattedDate date={record[field.name]} />
    {:else if field.type === "json"}
        <span class="txt txt-ellipsis">
            {cutText(JSON.stringify(record[field.name]))}
        </span>
    {:else if field.type === "select"}
        <div class="inline-flex">
            {#each CommonHelper.toArray(record[field.name]) as item, i (i + item)}
                <span class="label">{item}</span>
            {/each}
        </div>
    {:else if field.type === "relation" || field.type === "user"}
        <div class="inline-flex">
            {#each CommonHelper.toArray(record[field.name]).slice(0, 20) as item, i (i + item)}
                <IdLabel id={item} />
            {/each}
            {#if CommonHelper.toArray(record[field.name]).length > 20}
                ...
            {/if}
        </div>
    {:else if field.type === "file"}
        <div class="inline-flex">
            {#each CommonHelper.toArray(record[field.name]) as filename, i (i + filename)}
                <RecordFileThumb {record} {filename} size="sm" />
            {/each}
        </div>
    {:else}
        <span class="txt txt-ellipsis" title={cutText(record[field.name])}>
            {cutText(record[field.name])}
        </span>
    {/if}
</td>