ronantakizawa/codereview-bench huggingface dataset:
Repository: hydralauncher/hydra
File: src/renderer/src/pages/catalogue/game-item.tsx (TypeScript)

Code before:
      <div className="game-item__cover-placeholder">
        <QuestionIcon size={28} />
      </div>
    );
  }, [game.libraryImageUrl, game.title]);

  return (
    <button
      type="button"
      className="game-item"
      onClick={() => navigate(buildGameDetailsPath(game))}
    >
      {libraryImage}

      <div className="game-item__details">
        <span>{game.title}</span>
        <span className="game-item__genres">{genres.join(", ")}</span>

        <div className="game-item__repackers">
          {uniqueRepackers.map((repacker) => (
            <Badge key={repacker}>{repacker}</Badge>
          ))}
        </div>
      </div>
      <div
        className={"game-item__plus-wrapper" + (added ? " added" : "")}
        role="button"
        tabIndex={0}
        onClick={addGameToLibrary}
        onKeyDown={(e) => {
          if (e.key === "Enter" || e.key === " ") {
            e.preventDefault();
            addGameToLibrary(e);
          }
        }}
        title={added ? t("already_in_library") : t("add_to_library")}
      >
        {added ? <CheckIcon size={16} /> : <PlusIcon size={16} />}
      </div>
    </button>
  );
}


Review comment (suggestion):
you can use classnames for condicional classes, for example:

![image](https://github.com/user-attachments/assets/282b6162-4d89-43ef-80fd-3bea466a0f6c)


![image](https://github.com/user-attachments/assets/dd9a3c99-d596-4f35-ae16-5b02e044da55)


Code after:
        />
      );
    }

    return (
      <div className="game-item__cover-placeholder">
        <QuestionIcon size={28} />
      </div>
    );
  }, [game.libraryImageUrl, game.title]);

  return (
    <button
      type="button"
      className="game-item"
      onClick={() => navigate(buildGameDetailsPath(game))}
    >
      {libraryImage}

      <div className="game-item__details">
        <span>{game.title}</span>
        <span className="game-item__genres">{genres.join(", ")}</span>

        <div className="game-item__repackers">
          {uniqueRepackers.map((repacker) => (
            <Badge key={repacker}>{repacker}</Badge>
          ))}
        </div>
      </div>
      <div
        className={cn("game-item__plus-wrapper", {
          "game-item__plus-wrapper--added": added,
        })}
        role="button"
        tabIndex={0}
        onClick={addGameToLibrary}
        onKeyDown={(e) => {
          if (e.key === "Enter" || e.key === " ") {
            e.preventDefault();
            addGameToLibrary(e);
          }
        }}
        title={added ? t("already_in_library") : t("add_to_library")}
      >
        {added ? <CheckIcon size={16} /> : <PlusIcon size={16} />}
      </div>
    </button>
  );
}
Repository: hydralauncher/hydra
File: src/renderer/src/pages/catalogue/game-item.tsx (TypeScript)

Code before:
      <div className="game-item__cover-placeholder">
        <QuestionIcon size={28} />
      </div>
    );
  }, [game.libraryImageUrl, game.title]);

  return (
    <button
      type="button"
      className="game-item"
      onClick={() => navigate(buildGameDetailsPath(game))}
    >
      {libraryImage}

      <div className="game-item__details">
        <span>{game.title}</span>
        <span className="game-item__genres">{genres.join(", ")}</span>

        <div className="game-item__repackers">
          {uniqueRepackers.map((repacker) => (
            <Badge key={repacker}>{repacker}</Badge>
          ))}
        </div>
      </div>
      <div
        className={"game-item__plus-wrapper" + (added ? " added" : "")}
        role="button"
        tabIndex={0}
        onClick={addGameToLibrary}
        onKeyDown={(e) => {
          if (e.key === "Enter" || e.key === " ") {
            e.preventDefault();
            addGameToLibrary(e);
          }
        }}
        title={added ? t("already_in_library") : t("add_to_library")}
      >
        {added ? <CheckIcon size={16} /> : <PlusIcon size={16} />}
      </div>
    </button>
  );
}


Review comment (suggestion):
to follow [BEM](https://getbem.com/introduction/) patterns, the `added` class should be called `game-item__plus-wrapper--added`

Code after:
        />
      );
    }

    return (
      <div className="game-item__cover-placeholder">
        <QuestionIcon size={28} />
      </div>
    );
  }, [game.libraryImageUrl, game.title]);

  return (
    <button
      type="button"
      className="game-item"
      onClick={() => navigate(buildGameDetailsPath(game))}
    >
      {libraryImage}

      <div className="game-item__details">
        <span>{game.title}</span>
        <span className="game-item__genres">{genres.join(", ")}</span>

        <div className="game-item__repackers">
          {uniqueRepackers.map((repacker) => (
            <Badge key={repacker}>{repacker}</Badge>
          ))}
        </div>
      </div>
      <div
        className={cn("game-item__plus-wrapper", {
          "game-item__plus-wrapper--added": added,
        })}
        role="button"
        tabIndex={0}
        onClick={addGameToLibrary}
        onKeyDown={(e) => {
          if (e.key === "Enter" || e.key === " ") {
            e.preventDefault();
            addGameToLibrary(e);
          }
        }}
        title={added ? t("already_in_library") : t("add_to_library")}
      >
        {added ? <CheckIcon size={16} /> : <PlusIcon size={16} />}
      </div>
    </button>
  );
}
Repository: hydralauncher/hydra
File: src/main/services/download/js-http-downloader.ts (TypeScript)

Code before:
    }
    return undefined;
  }

  private cleanup(): void {
    if (this.writeStream) {
      this.writeStream.close();
      this.writeStream = null;
    }
    this.abortController = null;
  }

  private reset(): void {
    this.currentOptions = null;
    this.bytesDownloaded = 0;
    this.fileSize = 0;
    this.downloadSpeed = 0;
    this.status = "paused";
    this.folderName = "";
    this.isDownloading = false;
    this.retryCount = 0;
    this.isPaused = false;
    this.isStallRetry = false;
  }
}


Review comment (suggestion):
`cleanup()` sets `status = "paused"` and also resets `isPaused = false`. That will clobber both a successful download (`status` was set to `complete`) and a user-initiated pause (you lose the paused flag). Suggest: don’t force `status`/`isPaused` in cleanup; keep those transitions in explicit control-flow paths.

Code after:
        return decodeURIComponent(filename);
      }
    } catch {
      // Invalid URL
    }
    return undefined;
  }

  private cleanupResources(): void {
    if (this.writeStream) {
      this.writeStream.close();
      this.writeStream = null;
    }
    this.abortController = null;
  }

  private reset(): void {
    this.currentOptions = null;
    this.bytesDownloaded = 0;
    this.fileSize = 0;
    this.downloadSpeed = 0;
    this.status = "paused";
    this.folderName = "";
    this.isDownloading = false;
    this.retryCount = 0;
    this.isPaused = false;
    this.isStallRetry = false;
  }
}
Repository: hydralauncher/hydra
File: src/main/services/download/js-http-downloader.ts (TypeScript)

Code before:
import { Readable } from "node:stream";
import { pipeline } from "node:stream/promises";
import { logger } from "../logger";

export interface JsHttpDownloaderStatus {
  folderName: string;
  fileSize: number;
  progress: number;
  downloadSpeed: number;
  numPeers: number;
  numSeeds: number;
  status: "active" | "paused" | "complete" | "error";
  bytesDownloaded: number;
}

export interface JsHttpDownloaderOptions {
  url: string;
  savePath: string;
  filename?: string;
  headers?: Record<string, string>;
}

const MAX_RETRY_ATTEMPTS = 10;
const INITIAL_RETRY_DELAY_MS = 1000;
const MAX_RETRY_DELAY_MS = 15000;
const STALL_TIMEOUT_MS = 8000;

const RETRYABLE_ERROR_CODES = new Set([
  "ECONNRESET",
  "ETIMEDOUT",
  "ECONNREFUSED",
  "ENOTFOUND",
  "ENETUNREACH",
  "EHOSTUNREACH",
  "EPIPE",
  "EAI_AGAIN",
  "ECONNABORTED",
  "ESOCKETTIMEDOUT",
  "ERR_STREAM_PREMATURE_CLOSE",
]);

export class JsHttpDownloader {
  private abortController: AbortController | null = null;
  private writeStream: fs.WriteStream | null = null;
  private currentOptions: JsHttpDownloaderOptions | null = null;

  private bytesDownloaded = 0;
  private fileSize = 0;
  private downloadSpeed = 0;
  private status: "active" | "paused" | "complete" | "error" = "paused";
  private folderName = "";

Review comment (bug):
stall detection: interval is 5s and timeout is 8s, so worst-case detection can be ~13s (depending on alignment). not necessarily wrong, but if you want “~8s stall”, consider checking more frequently (e.g., 1s/2s) or bump the timeout to match the sampling interval.

Code after:
import { Readable } from "node:stream";
import { pipeline } from "node:stream/promises";
import { logger } from "../logger";

export interface JsHttpDownloaderStatus {
  folderName: string;
  fileSize: number;
  progress: number;
  downloadSpeed: number;
  numPeers: number;
  numSeeds: number;
  status: "active" | "paused" | "complete" | "error";
  bytesDownloaded: number;
}

export interface JsHttpDownloaderOptions {
  url: string;
  savePath: string;
  filename?: string;
  headers?: Record<string, string>;
}

const MAX_RETRY_ATTEMPTS = 10;
const INITIAL_RETRY_DELAY_MS = 1000;
const MAX_RETRY_DELAY_MS = 15000;
const STALL_TIMEOUT_MS = 8000;
const STALL_CHECK_INTERVAL_MS = 2000;

const RETRYABLE_ERROR_CODES = new Set([
  "ECONNRESET",
  "ETIMEDOUT",
  "ECONNREFUSED",
  "ENOTFOUND",
  "ENETUNREACH",
  "EHOSTUNREACH",
  "EPIPE",
  "EAI_AGAIN",
  "ECONNABORTED",
  "ESOCKETTIMEDOUT",
  "ERR_STREAM_PREMATURE_CLOSE",
]);

export class JsHttpDownloader {
  private abortController: AbortController | null = null;
  private writeStream: fs.WriteStream | null = null;
  private currentOptions: JsHttpDownloaderOptions | null = null;

  private bytesDownloaded = 0;
  private fileSize = 0;
  private downloadSpeed = 0;
  private status: "active" | "paused" | "complete" | "error" = "paused";
Repository: hydralauncher/hydra
File: src/main/services/download/js-http-downloader.ts (TypeScript)

Code before:
    if (elapsed >= 1) {
      const bytesDelta = this.bytesDownloaded - this.bytesAtLastSpeedUpdate;
      this.downloadSpeed = bytesDelta / elapsed;
      this.lastSpeedUpdate = now;
      this.bytesAtLastSpeedUpdate = this.bytesDownloaded;
    }
  }

  private extractFilename(url: string): string | undefined {
    try {
      const urlObj = new URL(url);
      const pathname = urlObj.pathname;
      const pathParts = pathname.split("/");
      const filename = pathParts.at(-1);

      if (filename?.includes(".") && filename.length > 0) {
        return decodeURIComponent(filename);
      }
    } catch {
      // Invalid URL
    }
    return undefined;
  }

  private cleanupResources(): void {
    if (this.writeStream) {
      this.writeStream.close();
      this.writeStream = null;
    }
    this.abortController = null;
  }

  private reset(): void {
    this.currentOptions = null;
    this.bytesDownloaded = 0;
    this.fileSize = 0;
    this.downloadSpeed = 0;
    this.status = "paused";
    this.folderName = "";
    this.isDownloading = false;
    this.retryCount = 0;
    this.isPaused = false;
    this.isStallRetry = false;
  }
}


Review comment (suggestion):
still a concern: `cleanupResources()` is now called in the `finally` of every attempt and it sets `status = "paused"`. That will overwrite `status = "complete"` set on success (and any other final status). Suggest keeping `cleanupResources()` purely about closing streams/abort controller/intervals and not mutating high-level state like `status`.

Code after:
      const pathParts = pathname.split("/");
      const filename = pathParts.at(-1);

      if (filename?.includes(".") && filename.length > 0) {
        return decodeURIComponent(filename);
      }
    } catch {
      // Invalid URL
    }
    return undefined;
  }

  private cleanupResources(): void {
    if (this.writeStream) {
      this.writeStream.close();
      this.writeStream = null;
    }
    this.abortController = null;
  }

  private reset(): void {
    this.currentOptions = null;
    this.bytesDownloaded = 0;
    this.fileSize = 0;
    this.downloadSpeed = 0;
    this.status = "paused";
    this.folderName = "";
    this.isDownloading = false;
    this.retryCount = 0;
    this.isPaused = false;
    this.isStallRetry = false;
  }
}
Repository: hydralauncher/hydra
File: src/main/services/download/js-http-downloader.ts (TypeScript)

Code before:
      this.bytesAtLastSpeedUpdate = this.bytesDownloaded;
    }
  }

  private extractFilename(url: string): string | undefined {
    try {
      const urlObj = new URL(url);
      const pathname = urlObj.pathname;
      const pathParts = pathname.split("/");
      const filename = pathParts.at(-1);

      if (filename?.includes(".") && filename.length > 0) {
        return decodeURIComponent(filename);
      }
    } catch {
      // Invalid URL
    }
    return undefined;
  }

  private cleanupResources(): void {
    if (this.writeStream) {
      this.writeStream.close();
      this.writeStream = null;
    }
    this.abortController = null;
  }

  private reset(): void {
    this.currentOptions = null;
    this.bytesDownloaded = 0;
    this.fileSize = 0;
    this.downloadSpeed = 0;
    this.status = "paused";
    this.folderName = "";
    this.isDownloading = false;
    this.retryCount = 0;
    this.isPaused = false;
    this.isStallRetry = false;
  }
}


Review comment (suggestion):
related: `cleanupResources()` resets `isPaused = false` (and `isStallRetry = false`). if the user paused/cancelled, this can lose the paused state immediately after abort. i’d avoid resetting these flags in cleanup; do it explicitly on `startDownload`/`resumeDownload` paths instead.

Code after:
        return decodeURIComponent(filename);
      }
    } catch {
      // Invalid URL
    }
    return undefined;
  }

  private cleanupResources(): void {
    if (this.writeStream) {
      this.writeStream.close();
      this.writeStream = null;
    }
    this.abortController = null;
  }

  private reset(): void {
    this.currentOptions = null;
    this.bytesDownloaded = 0;
    this.fileSize = 0;
    this.downloadSpeed = 0;
    this.status = "paused";
    this.folderName = "";
    this.isDownloading = false;
    this.retryCount = 0;
    this.isPaused = false;
    this.isStallRetry = false;
  }
}
Repository: jerryjliu/gpt_index
File: llama-index-core/tests/embeddings/test_utils.py (Python)

Code before:
import pytest
import os
import tempfile
from llama_index.core.embeddings.utils import load_embedding

def test_load_embedding_empty_file():
    """Test load_embedding with an empty file to cover the newly added error check."""
    with tempfile.NamedTemporaryFile(delete=False) as tmp_file:
        tmp_path = tmp_file.name
    
    try:
        with pytest.raises(ValueError, match="is empty"):
            load_embedding(tmp_path)
    finally:
        if os.path.exists(tmp_path):
            os.remove(tmp_path)

Review comment (suggestion):
This should probably be using the built-in `tmp_path` fixture provided by pytest, so that you don't have to worry about removing the file since pytest takes care of it

Code after:
from llama_index.core.embeddings.mock_embed_model import MockEmbedding
from llama_index.core.embeddings.utils import load_embedding, resolve_embed_model
from pytest import MonkeyPatch
import pytest
from pathlib import Path


def test_resolve_embed_model(monkeypatch: MonkeyPatch) -> None:
    # Test None
    embed_model = resolve_embed_model(None)
    assert isinstance(embed_model, MockEmbedding)


def test_load_embedding_empty_file(tmp_path: Path) -> None:
    """Test load_embedding with an empty file to cover the newly added error check."""
    empty_file = tmp_path / "empty.txt"
    empty_file.touch()

    with pytest.raises(ValueError, match="is empty"):
        load_embedding(str(empty_file))
Repository: jiahaog/nativefier
File: src/options/optionsMain.ts (TypeScript)

Code before:
import axios from 'axios';

import * as fs from 'fs';

import * as log from 'loglevel';

// package.json is `require`d to let tsc strip the `src` folder by determining
// baseUrl=src. A static import would prevent that and cause an ugly extra `src` folder in `lib`
const packageJson = require('../../package.json'); // eslint-disable-line @typescript-eslint/no-var-requires
import {
  DEFAULT_ELECTRON_VERSION,
  PLACEHOLDER_APP_DIR,
  ELECTRON_MAJOR_VERSION,
} from '../constants';
import { inferPlatform, inferArch } from '../infer/inferOs';
import { asyncConfig } from './asyncConfig';
import { AppOptions } from './model';
import { normalizeUrl } from './normalizeUrl';

const SEMVER_VERSION_NUMBER_REGEX = /\d+\.\d+\.\d+[-_\w\d.]*/;

/**
 * Process and validate raw user arguments
 */
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
export async function getOptions(rawOptions: any): Promise<AppOptions> {
  const options: AppOptions = {
    packager: {
      appCopyright: rawOptions.appCopyright,
      appVersion: rawOptions.appVersion,

Review comment (suggestion):
Nit: in many languages, it's conventionally common to order imports in the following order:

1. Standard library imports (here, `fs`)
2. Third-party library imports (here, `axios` and `loglevel`)
3. Internal imports

, each block ordered alphabetically and each block separated by a whitespace. So,

```suggestion
import * as fs from 'fs';

import axios from 'axios';
import * as log from 'loglevel';
```

Code after:
import * as fs from 'fs';

import axios from 'axios';
import * as log from 'loglevel';

// package.json is `require`d to let tsc strip the `src` folder by determining
// baseUrl=src. A static import would prevent that and cause an ugly extra `src` folder in `lib`
const packageJson = require('../../package.json'); // eslint-disable-line @typescript-eslint/no-var-requires
import {
  DEFAULT_ELECTRON_VERSION,
  PLACEHOLDER_APP_DIR,
  ELECTRON_MAJOR_VERSION,
} from '../constants';
import { inferPlatform, inferArch } from '../infer/inferOs';
import { asyncConfig } from './asyncConfig';
import { AppOptions } from './model';
import { normalizeUrl } from './normalizeUrl';

const SEMVER_VERSION_NUMBER_REGEX = /\d+\.\d+\.\d+[-_\w\d.]*/;

/**
 * Process and validate raw user arguments
 */
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
export async function getOptions(rawOptions: any): Promise<AppOptions> {
  const options: AppOptions = {
    packager: {
      appCopyright: rawOptions.appCopyright,
      appVersion: rawOptions.appVersion,
      arch: rawOptions.arch || inferArch(),
Repository: jiahaog/nativefier
File: src/cli.ts (TypeScript)

Code before:
#!/usr/bin/env node
import 'source-map-support/register';

import * as commander from 'commander';
import * as dns from 'dns';
import * as log from 'loglevel';

import { buildNativefierApp } from './main';
import { isArgFormatValid } from '../app/src/helpers/helpers';
import { isWindows } from './helpers/helpers';

// package.json is `require`d to let tsc strip the `src` folder by determining
// baseUrl=src. A static import would prevent that and cause an ugly extra "src" folder
const packageJson = require('../package.json'); // eslint-disable-line @typescript-eslint/no-var-requires

function collect(val: any, memo: any[]): any[] {
  memo.push(val);
  return memo;
}

function parseBooleanOrString(val: string): boolean | string {
  switch (val) {
    case 'true':
      return true;
    case 'false':
      return false;
    default:
      return val;
  }
}

function parseJson(val: string): any {
  if (!val) return {};
  try {

Review comment (refactor):
Careful with imports automatically-inserted by your editor, they sometimes do weird stuff.

```suggestion
import { isArgFormatValid } from './helpers/helpers';
```

Code after:
#!/usr/bin/env node
import 'source-map-support/register';

import * as commander from 'commander';
import * as dns from 'dns';
import * as log from 'loglevel';

import { buildNativefierApp } from './main';
import { isArgFormatValid } from './helpers/helpers';
import { isWindows } from './helpers/helpers';

// package.json is `require`d to let tsc strip the `src` folder by determining
// baseUrl=src. A static import would prevent that and cause an ugly extra "src" folder
const packageJson = require('../package.json'); // eslint-disable-line @typescript-eslint/no-var-requires

function collect(val: any, memo: any[]): any[] {
  memo.push(val);
  return memo;
}

function parseBooleanOrString(val: string): boolean | string {
  switch (val) {
    case 'true':
      return true;
    case 'false':
      return false;
    default:
      return val;
  }
}

function parseJson(val: string): any {
  if (!val) return {};
  try {
Repository: jiahaog/nativefier
File: src/helpers/helpers.ts (TypeScript)

Code before:
  const icoToIcns = pngToIcns && hasIdentify;
  const icoToPng = hasConvert;

  // Unsupported
  const icnsToPng = false;
  const icnsToIco = false;

  const formats = [];

  // Shell scripting is not supported on windows, temporary override
  if (isWindows()) {
    switch (platform) {
      case 'darwin':
        formats.push('.icns');
        break;
      case 'linux':
        formats.push('.png');
        break;
      case 'win32':
        formats.push('.ico');
        break;
      case 'all':
        formats.push('.icns');
        formats.push('.png');
        formats.push('.ico');
        break;
      default:
        throw new Error(`Unknown platform ${platform}`);
    }
    log.debug(
      `Allowed icon formats when building for ${platform} (limited on Windows):`,
      formats,
    );
    return formats;
  }

  switch (platform) {
    case 'darwin':
      formats.push('.icns');
      if (pngToIcns) {
        formats.push('.png');
      }
      if (icoToIcns) {
        formats.push('.ico');
      }
      break;
    case 'linux':
      formats.push('.png');
      if (icoToPng) {
        formats.push('.ico');
      }

Review comment (suggestion):
Hmmmm, but won't that end up break later? A Linux build won't be happy to be fed the `.ico` that you feed it if it found a `.ico` but because of this code decided not to convert it.

Code after:
        ext: path.extname(fileUrl),
      };
    });
}

export function getAllowedIconFormats(platform: string): string[] {
  const hasIdentify = hasbin.sync('identify') || hasbin.sync('gm');
  const hasConvert = hasbin.sync('convert') || hasbin.sync('gm');
  const hasIconUtil = hasbin.sync('iconutil');

  const pngToIcns = hasConvert && hasIconUtil;
  const pngToIco = hasConvert;
  const icoToIcns = pngToIcns && hasIdentify;
  const icoToPng = hasConvert;

  // Unsupported
  const icnsToPng = false;
  const icnsToIco = false;

  const formats = [];

  // Shell scripting is not supported on windows, temporary override
  if (isWindows()) {
    switch (platform) {
      case 'darwin':
        formats.push('.icns');
        break;
      case 'linux':
        formats.push('.png');
        break;
      case 'win32':
        formats.push('.ico');
        break;
      default:
        throw new Error(`Unknown platform ${platform}`);
    }
    log.debug(
      `Allowed icon formats when building for ${platform} (limited on Windows):`,
      formats,
    );
    return formats;
  }

  switch (platform) {
    case 'darwin':
      formats.push('.icns');
      if (pngToIcns) {
        formats.push('.png');
      }
      if (icoToIcns) {
        formats.push('.ico');