ronantakizawa/codereview-bench huggingface dataset:
Repository: denisidoro/navi
File: shell/navi.plugin.ps1 (PowerShell)

Code before:


$null = New-Module {
    ### Initial code from @lurebat (https://github.com/lurebat/)
    ### See #570 (https://github.com/denisidoro/navi/issues/570) for its original contribution
    function Invoke-NaviWidget {
        $ast = $tokens = $errors = $cursor = $null
        [Microsoft.PowerShell.PSConsoleReadLine]::GetBufferState([ref] $ast, [ref] $tokens, [ref] $errors, [ref] $cursor)

        $line = $ast.ToString().Trim()
        $output = $null

        if ([String]::IsNullOrEmpty($line)) {
            $output = navi --print
        }
        else {
            $best_match = (navi --print --best-match --query $line | Out-String).Trim()
            if ([String]::IsNullOrEmpty($best_match)) {
                $output = (navi --print --query "$line" | Out-String).Trim()
            }
            else {
                $output = $best_match
            }
        }

        [Microsoft.PowerShell.PSConsoleReadLine]::RevertLine()

        ### Handling the case when the user escapes without selecting any entry
        if (-Not([String]::IsNullOrEmpty($output))) {
            [Microsoft.PowerShell.PSConsoleReadLine]::Insert([String]$output)
        }
    }

    Set-PSReadlineKeyHandler -BriefDescription "A keybinding to open Navi Widget" -Chord Ctrl+g -ScriptBlock { Invoke-NaviWidget }
}


Review comment (suggestion):
By default, functions in a module are exported.
It is better to prevent functions from being exported.

```suggestion

    Export-ModuleMember -Function @()
}
```

Code after:
        $output = $null

        if ([String]::IsNullOrEmpty($line)) {
            $output = (Invoke-Navi "--print" | Out-String).Trim()
        }
        else {
            $best_match = (Invoke-Navi "--print --best-match --query `"$line`"" | Out-String).Trim()
            if ([String]::IsNullOrEmpty($best_match)) {
                $output = (Invoke-Navi "--print --query `"$line`"" | Out-String).Trim()
            }
            else {
                $output = $best_match
            }
        }

        [Microsoft.PowerShell.PSConsoleReadLine]::InvokePrompt()

        ### Handling the case when the user escapes without selecting any entry
        if (-Not([String]::IsNullOrEmpty($output))) {
            [Microsoft.PowerShell.PSConsoleReadLine]::Insert([String]$output)
        }
    }

    Set-PSReadlineKeyHandler -BriefDescription "A keybinding to open Navi Widget" -Chord Ctrl+g -ScriptBlock { Invoke-NaviWidget }
    Export-ModuleMember -Function @()
}
Repository: denisidoro/navi
File: src/common/clipboard.rs (Rust)

Code before:
      let mut cmd = std::process::Command::new("cmd.exe");
      cmd.arg("/C").arg("clip.exe");
      let mut child = cmd.stdin(std::process::Stdio::piped()).spawn()
         .map_err(|e| ShellSpawnError::new("clip.exe", e))?;
      if let Some(stdin) = child.stdin.as_mut() {
         use std::io::Write;
         stdin.write_all(text.as_bytes())?;
      }
      child.wait()?;
      Ok(())
   } else {
      // Use bash/zsh/fish/etc logic
      let mut script = String::new();
      script.push_str("exst() { type \"$1\" &>/dev/null; }\n");
      script.push_str("_copy() {\n");
      script.push_str("  if exst pbcopy; then pbcopy\n");
      script.push_str("  elif exst xclip; then xclip -selection clipboard\n");
      script.push_str("  elif exst clip.exe; then clip.exe\n");  // # Could be used on WSL or GitBash
      script.push_str("  else exit 55; fi\n");
      script.push_str("}\n");
      script.push_str(&format!(
         "read -r -d '' x <<\"{EOF}\"\n{text}\n{EOF}\n",
         EOF = EOF,
         text = text
      ));
      script.push_str("echo -n \"$x\" | _copy\n");
      shell::out()
         .arg(script.as_str())
         .spawn()
         .map_err(|e| ShellSpawnError::new(script.as_str(), e))?
         .wait()?;
      Ok(())
   }
}


Review comment (suggestion):
Can't we keep using a raw string? Having multiple lines of `script.push_str()` seems awfully unreadable at first sight.

Code after:
      let mut cmd = std::process::Command::new("cmd.exe");
      cmd.arg("/C").arg("clip.exe");
      let mut child = cmd.stdin(std::process::Stdio::piped()).spawn()
         .map_err(|e| ShellSpawnError::new("clip.exe", e))?;
      if let Some(stdin) = child.stdin.as_mut() {
         use std::io::Write;
         stdin.write_all(text.as_bytes())?;
      }
      child.wait()?;
      Ok(())
   } else {
      // Use bash/zsh/fish/etc logic
      let script = format!(
         r#"exst() {{ type "$1" &>/dev/null; }}
_copy() {{
  if exst pbcopy; then pbcopy
  elif exst xclip; then xclip -selection clipboard
  elif exst clip.exe; then clip.exe
  else exit 55; fi
}}
read -r -d '' x <<"{EOF}"
{text}
{EOF}
echo -n "$x" | _copy
"#,
         EOF = EOF,
         text = text
      );
      shell::out()
         .arg(script.as_str())
         .spawn()
         .map_err(|e| ShellSpawnError::new(script.as_str(), e))?
         .wait()?;
      Ok(())
   }
}
Repository: denisidoro/navi
File: src/config/yaml.rs (Rust)

Code before:
{
    let s: String = Deserialize::deserialize(deserializer)?;
    FinderChoice::from_str(s.to_lowercase().as_str())
        .map_err(|_| de::Error::custom(format!("Failed to deserialize finder: {s}")))
}

#[derive(Deserialize, Default, Debug)]
#[serde(default)]
pub struct Cheats {
    pub path: Option<String>,
    pub paths: Vec<String>,
}

#[derive(Deserialize, Default, Debug)]
#[serde(default)]
pub struct Search {
    pub tags: Option<String>,
}

#[derive(Deserialize, Debug)]
#[serde(default)]
pub struct Shell {
    pub command: String,
    pub finder_command: Option<String>,
    pub show_command: Option<bool>,
    pub command_print_color: Option<Color>,
}

#[derive(Deserialize, Debug)]
#[serde(default)]
#[derive(Default)]
pub struct Client {
    pub tealdeer: bool,
}

#[derive(Deserialize, Debug)]
#[serde(default)]
pub struct YamlConfig {
    pub style: Style,
    pub finder: Finder,
    pub cheats: Cheats,
    pub search: Search,
    pub shell: Shell,
    pub client: Client,
    pub source: String, // <= The source of the current configuration
}

impl YamlConfig {
    fn from_str(text: &str) -> Result<Self> {
        serde_yaml::from_str(text).map_err(|e| e.into())
    }

Review comment (suggestion):
I don't think this has its place in this PR. It's not related to windows support, is it?

It's a visual enhancement as I understand it, I have yet to test the code on a dedicated workspace but it would be better to separate both FR/PR as they can be two entire discussions as we are quite conscientious of the changes in the configuration format and file.

Code after:
{
    let s: String = Deserialize::deserialize(deserializer)?;
    FinderChoice::from_str(s.to_lowercase().as_str())
        .map_err(|_| de::Error::custom(format!("Failed to deserialize finder: {s}")))
}

#[derive(Deserialize, Default, Debug)]
#[serde(default)]
pub struct Cheats {
    pub path: Option<String>,
    pub paths: Vec<String>,
}

#[derive(Deserialize, Default, Debug)]
#[serde(default)]
pub struct Search {
    pub tags: Option<String>,
}

#[derive(Deserialize, Debug)]
#[serde(default)]
pub struct Shell {
    pub command: String,
    pub finder_command: Option<String>
}

#[derive(Deserialize, Debug)]
#[serde(default)]
#[derive(Default)]
pub struct Client {
    pub tealdeer: bool,
}

#[derive(Deserialize, Debug)]
#[serde(default)]
pub struct YamlConfig {
    pub style: Style,
    pub finder: Finder,
    pub cheats: Cheats,
    pub search: Search,
    pub shell: Shell,
    pub client: Client,
    pub source: String, // <= The source of the current configuration
}

impl YamlConfig {
    fn from_str(text: &str) -> Result<Self> {
        serde_yaml::from_str(text).map_err(|e| e.into())
    }

    fn from_path(path: &Path) -> Result<Self> {
Repository: denoland/fresh
File: www/utils/screenshot.ts (TypeScript)

Code before:
  throw new Error("Usage: screenshot <url> <id>");
}

const [url, id] = Deno.args;
const parsedUrl = new URL(url);
if (parsedUrl.protocol !== "http:" && parsedUrl.protocol !== "https:") {
  throw new Error("Invalid URL");
}

const browser = await puppeteer.launch({
  defaultViewport: { width: 1200, height: 675 },
  headless: true,
});
const page = await browser.newPage();
await page.goto(url, { waitUntil: "networkidle2" });
const raw = await page.screenshot();

await browser.close();

// convert to jpeg
const image2x = await Image.decode(raw);

const outputDir = new URL("../static/showcase/", import.meta.url);

const jpeg2x = new URL(`${id}2x.jpg`, outputDir).pathname;
await Deno.writeFile(jpeg2x, await image2x.encodeJPEG(80));

const jpeg1x = new URL(`${id}1x.jpg`, outputDir).pathname;
const image1x = image2x.resize(image2x.width / 2, Image.RESIZE_AUTO);
await Deno.writeFile(jpeg1x, await image1x.encodeJPEG(80));


Review comment (suggestion):
```suggestion

await Deno.writeFile(`./www/static/showcase/${id}2x.jpg`, await image2x.encodeJPEG(80));
```

Code after:
  throw new Error("Usage: screenshot <url> <id>");
}

const [url, id] = Deno.args;
const parsedUrl = new URL(url);
if (parsedUrl.protocol !== "http:" && parsedUrl.protocol !== "https:") {
  throw new Error("Invalid URL");
}

const browser = await launch();
const page = await browser.newPage(url);
await page.waitForNetworkIdle();
const raw = await page.screenshot();

await browser.close();

// convert to jpeg
const image2x = await Image.decode(raw);
await Deno.writeFile(
  `./www/static/showcase/${id}2x.jpg`,
  await image2x.encodeJPEG(80),
);

const image1x = image2x.resize(image2x.width / 2, Image.RESIZE_AUTO);
await Deno.writeFile(
  `./www/static/showcase/${id}1x.jpg`,
  await image1x.encodeJPEG(80),
);
Repository: denoland/fresh
File: www/utils/screenshot.ts (TypeScript)

Code before:
const parsedUrl = new URL(url);
if (parsedUrl.protocol !== "http:" && parsedUrl.protocol !== "https:") {
  throw new Error("Invalid URL");
}

const browser = await puppeteer.launch({
  defaultViewport: { width: 1200, height: 675 },
  headless: true,
});
const page = await browser.newPage();
await page.goto(url, { waitUntil: "networkidle2" });
const raw = await page.screenshot();

await browser.close();

// convert to jpeg
const image2x = await Image.decode(raw);

const outputDir = new URL("../static/showcase/", import.meta.url);

const jpeg2x = new URL(`${id}2x.jpg`, outputDir).pathname;
await Deno.writeFile(jpeg2x, await image2x.encodeJPEG(80));

const jpeg1x = new URL(`${id}1x.jpg`, outputDir).pathname;
const image1x = image2x.resize(image2x.width / 2, Image.RESIZE_AUTO);
await Deno.writeFile(jpeg1x, await image1x.encodeJPEG(80));


Review comment (suggestion):
```suggestion
const image1x = image2x.resize(image2x.width / 2, Image.RESIZE_AUTO);
await Deno.writeFile(`./www/static/showcase/${id}1x.jpg`, await image1x.encodeJPEG(80));
```

Code after:
import { launch } from "astral";
import { Image } from "imagescript";

if (Deno.args.length !== 2) {
  throw new Error("Usage: screenshot <url> <id>");
}

const [url, id] = Deno.args;
const parsedUrl = new URL(url);
if (parsedUrl.protocol !== "http:" && parsedUrl.protocol !== "https:") {
  throw new Error("Invalid URL");
}

const browser = await launch();
const page = await browser.newPage(url);
await page.waitForNetworkIdle();
const raw = await page.screenshot();

await browser.close();

// convert to jpeg
const image2x = await Image.decode(raw);
await Deno.writeFile(
  `./www/static/showcase/${id}2x.jpg`,
  await image2x.encodeJPEG(80),
);

const image1x = image2x.resize(image2x.width / 2, Image.RESIZE_AUTO);
await Deno.writeFile(
  `./www/static/showcase/${id}1x.jpg`,
  await image1x.encodeJPEG(80),
);
Repository: denoland/fresh
File: src/build_cache.ts (TypeScript)

Code before:
  #config: ResolvedFreshConfig;

  constructor(
    config: ResolvedFreshConfig,
    islands: Map<string, string>,
    files: Map<string, FileSnapshot>,
    public hasSnapshot: boolean,
  ) {
    this.#islands = islands;
    this.#fileInfo = files;
    this.#config = config;
  }

  async readFile(pathname: string): Promise<StaticFile | null> {
    const info = this.#fileInfo.get(pathname);
    if (info === undefined) return null;

    const base = info.generated
      ? this.#config.build.outDir
      : this.#config.staticDir;
    const filePath = info.generated
      ? path.join(base, "static", pathname)
      : path.join(base, pathname);

    // Check if path resolves outside of intended directory.
    if (path.relative(base, filePath).startsWith(".")) {
      return null;
    }

    const [stat, file] = await Promise.all([
      Deno.stat(filePath),
      Deno.open(filePath),
    ]);

    return {
      hash: info.hash,
      size: stat.size,
      readable: file.readable,
      close: () => file.close(),
    };
  }

  getIslandChunkName(islandName: string): string | null {
    return this.#islands.get(islandName) ?? null;
  }
}


Review comment (security):
I can't come up with a scenario where just `.` and not `..` can escape to a parent directory except for e.g. `./../`. But if this is deemed "risky", we could perhaps change it to:

```ts
    const rel = path.relative(base, filePath);
    if (rel.startsWith(".") && !rel.startsWith(".well-known/") {
```

The `.well-known` folder is the only example I can come up with currently that is required to start with `.`, though there are likely cases somewhere 😄 

Code after:
  #config: ResolvedFreshConfig;

  constructor(
    config: ResolvedFreshConfig,
    islands: Map<string, string>,
    files: Map<string, FileSnapshot>,
    public hasSnapshot: boolean,
  ) {
    this.#islands = islands;
    this.#fileInfo = files;
    this.#config = config;
  }

  async readFile(pathname: string): Promise<StaticFile | null> {
    const info = this.#fileInfo.get(pathname);
    if (info === undefined) return null;

    const base = info.generated
      ? this.#config.build.outDir
      : this.#config.staticDir;
    const filePath = info.generated
      ? path.join(base, "static", pathname)
      : path.join(base, pathname);

    // Check if path resolves outside of intended directory.
    if (path.relative(base, filePath).startsWith("..")) {
      return null;
    }

    const [stat, file] = await Promise.all([
      Deno.stat(filePath),
      Deno.open(filePath),
    ]);

    return {
      hash: info.hash,
      size: stat.size,
      readable: file.readable,
      close: () => file.close(),
    };
  }

  getIslandChunkName(islandName: string): string | null {
    return this.#islands.get(islandName) ?? null;
  }
}
Repository: denoland/fresh
File: src/utils.ts (TypeScript)

Code before:
export function mergePaths(a: string, b: string) {
  if (a === "" || a === "/" || a === "/*") return b;
  if (b === "/") return a;
  if (a.endsWith("/")) return a.slice(0, -1) + b;
  if (!b.startsWith("/")) return a + "/" + b;
  return a + b;
}

/**
 * Converts a file path to a valid JS export name.
 *
 * @example
 * ```ts
 * pathToExportName("/islands/foo.tsx");     // "foo"
 * pathToExportName("/islands/foo.v2.tsx");  // "foo_v2"
 * pathToExportName("/islands/nav-bar.tsx"); // "nav_bar"
 * pathToExportName("/islands/_.$bar.tsx");  // "_$bar"
 * pathToExportName("/islands/1.hello.tsx"); // "_hello"
 * ```
 */
export function pathToExportName(filePath: string): string {
  const name = path.basename(filePath, path.extname(filePath));
  // Regex for valid JS identifier characters
  const regex = /^[^a-z_$]|[^a-z0-9_$]/gi;
  return name.replaceAll(regex, "_").replaceAll(/_{2,}/g, "_");
}


Review comment (suggestion):
```suggestion

const INVALID_JS_IDENTIFIER = /^[^a-z_$]|[^a-z0-9_$]/gi;
const DOUBLE_UNDERSCORE = /_{2,}/g

/**
 * Converts a file path to a valid JS export name.
 *
 * @example
 * ```ts
 * pathToExportName("/islands/foo.tsx");     // "foo"
 * pathToExportName("/islands/foo.v2.tsx");  // "foo_v2"
 * pathToExportName("/islands/nav-bar.tsx"); // "nav_bar"
 * pathToExportName("/islands/_.$bar.tsx");  // "_$bar"
 * pathToExportName("/islands/1.hello.tsx"); // "_hello"
 * ```
 */
export function pathToExportName(filePath: string): string {
  return path.basename(filePath, path.extname(filePath))
    .replaceAll(INVALID_JS_IDENTIFIER, "_")
    .replaceAll(DOUBLE_UNDERSCORE, "_");
}
```

Code after:
export function mergePaths(a: string, b: string) {
  if (a === "" || a === "/" || a === "/*") return b;
  if (b === "/") return a;
  if (a.endsWith("/")) return a.slice(0, -1) + b;
  if (!b.startsWith("/")) return a + "/" + b;
  return a + b;
}

/**
 * Converts a file path to a valid JS export name.
 *
 * @example
 * ```ts
 * pathToExportName("/islands/foo.tsx");     // "foo"
 * pathToExportName("/islands/foo.v2.tsx");  // "foo_v2"
 * pathToExportName("/islands/nav-bar.tsx"); // "nav_bar"
 * pathToExportName("/islands/_.$bar.tsx");  // "_$bar"
 * pathToExportName("/islands/1.hello.tsx"); // "_hello"
 * pathToExportName("/islands/collapse...repeat_-dash.tsx");
 * // "collapse_repeat_dash"
 * ```
 */
export function pathToExportName(filePath: string): string {
  const name = path.basename(filePath, path.extname(filePath));
  // Regex for valid JS identifier characters
  const regex = /^[^a-z_$]|[^a-z0-9_$]/gi;
  return name.replaceAll(regex, "_").replaceAll(/_{2,}/g, "_");
}
Repository: denoland/fresh
File: src/context.ts (TypeScript)

Code before:
   */
  next(): Promise<Response>;
  render(vnode: VNode, init?: ResponseInit): Response | Promise<Response>;
}

export let getBuildCache: (ctx: FreshContext<unknown>) => BuildCache;

export class FreshReqContext<State>
  implements FreshContext<State>, PageProps<unknown, State> {
  config: ResolvedFreshConfig;
  url: URL;
  req: Request;
  params: Record<string, string>;
  state: State = {} as State;
  data: unknown = undefined;
  error: unknown | null = null;
  info: Deno.ServeHandlerInfo | Deno.ServeHandlerInfo;

  next: FreshContext<State>["next"];

  #islandRegistry: ServerIslandRegistry;
  #buildCache: BuildCache;

  // FIXME: remove after switching to <Slot />
  Component!: FunctionComponent;

  static {
    getBuildCache = (ctx) => (ctx as FreshReqContext<unknown>).#buildCache;
  }

  constructor(
    req: Request,
    url: URL,
    info: Deno.ServeHandlerInfo,
    params: Record<string, string>,
    config: ResolvedFreshConfig,
    next: FreshContext<State>["next"],
    islandRegistry: ServerIslandRegistry,
    buildCache: BuildCache,
  ) {
    this.url = url;
    this.req = req;
    this.info = info;
    this.params = params;
    this.config = config;
    this.next = next;
    this.#islandRegistry = islandRegistry;
    this.#buildCache = buildCache;
  }

  redirect(pathOrUrl: string, status = 302): Response {

Review comment (suggestion):
Most of the properties could safely add `readonly` here to match the interface without TS errors. But I can revert this if preferred, but I think it can help minimize mistakes.

Code after:
   *   const res = await ctx.next();
   *
   *   // do something after the middleware
   *   doSomethingAfter()
   *
   *   // Return the `Response`
   *   return res
   * }
   */
  next(): Promise<Response>;
  render(vnode: VNode, init?: ResponseInit): Response | Promise<Response>;
}

export let getBuildCache: (ctx: FreshContext<unknown>) => BuildCache;

export class FreshReqContext<State>
  implements FreshContext<State>, PageProps<unknown, State> {
  readonly config: ResolvedFreshConfig;
  readonly url: URL;
  readonly req: Request;
  readonly params: Record<string, string>;
  readonly state: State = {} as State;
  data: unknown = undefined;
  error: unknown | null = null;
  readonly info: Deno.ServeHandlerInfo | Deno.ServeHandlerInfo;
  readonly isPartial: boolean;

  next: FreshContext<State>["next"];

  #islandRegistry: ServerIslandRegistry;
  #buildCache: BuildCache;

  // FIXME: remove after switching to <Slot />
  Component!: FunctionComponent;

  static {
    getBuildCache = (ctx) => (ctx as FreshReqContext<unknown>).#buildCache;
  }

  constructor(
    req: Request,
    url: URL,
    info: Deno.ServeHandlerInfo,
    params: Record<string, string>,
    config: ResolvedFreshConfig,
    next: FreshContext<State>["next"],
    islandRegistry: ServerIslandRegistry,
    buildCache: BuildCache,
  ) {
    this.url = url;
    this.req = req;
Repository: denoland/fresh
File: src/middlewares/cors.ts (TypeScript)

Code before:
      if (varyValues.size > 0) {
        setResponseHeader("Vary", Array.from(varyValues).join(", "));
      } else {
        responseHeaders.delete("Vary"); // Ensure Vary is not set if no conditions met
      }

      responseHeaders.delete("Content-Length");
      responseHeaders.delete("Content-Type");

      return new Response(null, {
        status: 204,
        statusText: "No Content",
        headers: responseHeaders,
      });
    }

    // For non-OPTIONS requests
    const actualResponse = await ctx.next();

    // Apply common CORS headers (ACAO, Credentials, ExposeHeaders)
    responseHeaders.forEach((value, key) => {
      // Do not overwrite Vary if it was set by downstream, merge instead
      if (key.toLowerCase() !== "vary") {
        actualResponse.headers.set(key, value);
      }
    });

    // Merge our calculated varyValues with any existing Vary from downstream response
    const existingVary = actualResponse.headers.get("Vary");
    if (existingVary) {
      existingVary.split(/\s*,\s*/).forEach((v) => varyValues.add(v));
    }

    if (varyValues.size > 0) {
      actualResponse.headers.set("Vary", Array.from(varyValues).join(", "));
    }
    // If varyValues is empty and no existingVary, Vary header remains absent or as set by downstream.

    return actualResponse;
  };
}


Review comment (performance):
Doing a loop here is more expensive than a map lookup. The `Headers` class already takes care of casing:

```sh
> let a = new Headers()
undefined
> a.set("Vary", "foo")
undefined
> a.get("vary")
"foo"
> a.get("VARY")
"foo"
>
```

Code after:
      headers.set("Vary", Array.from(varyValues).join(", "));
    } else {
      headers.delete("Vary"); // Ensure Vary is not set if no conditions met
    }

    headers.delete("Content-Length");
    headers.delete("Content-Type");

    return new Response(null, {
      status: 204,
      statusText: "No Content",
      headers,
    });
  };

  return async (ctx: FreshContext): Promise<Response> => {
    const requestOrigin = ctx.req.headers.get("origin") || "";
    const allowOrigin = findAllowOrigin(requestOrigin, ctx);

    const varyValues = new Set<string>();
    // Add 'Origin' to Vary if a specific origin is allowed, not '*'
    if (opts.origin !== "*" && allowOrigin && allowOrigin !== "*") {
      varyValues.add("Origin");
    }

    if (ctx.req.method === "OPTIONS") {
      return OptionsResponse(ctx, allowOrigin, opts, varyValues);
    }

    // For non-OPTIONS requests
    const res = await ctx.next();

    addHeaderProperties(
      res.headers,
      allowOrigin,
      opts,
    );

    // Merge our calculated varyValues with any existing Vary from downstream response
    if (varyValues.size > 0) {
      const existingVary = res.headers.get("Vary");
      if (existingVary) {
        existingVary.split(/\s*,\s*/).forEach((v) => varyValues.add(v));
      }

      res.headers.set("Vary", Array.from(varyValues).join(", "));
    }

    return res;
  };
}