ronantakizawa/codereview-bench huggingface dataset:
Repository: sst/opencode
File: packages/app/src/components/prompt-input/attachments.ts (TypeScript)

Code before:
  }

  const handleGlobalDragLeave = (event: DragEvent) => {
    if (input.isDialogActive()) return
    if (!event.relatedTarget) {
      input.setDragging(false)
    }
  }

  const handleGlobalDrop = async (event: DragEvent) => {
    if (input.isDialogActive()) return

    event.preventDefault()
    input.setDragging(false)

    const dropped = event.dataTransfer?.files
    if (!dropped) return

    for (const file of Array.from(dropped)) {
      if (ACCEPTED_FILE_TYPES.includes(file.type)) {
        await addImageAttachment(file)
      }
    }
  }

  onMount(() => {
    document.addEventListener("dragover", handleGlobalDragOver)
    document.addEventListener("dragleave", handleGlobalDragLeave)
    document.addEventListener("drop", handleGlobalDrop)
  })

  onCleanup(() => {
    document.removeEventListener("dragover", handleGlobalDragOver)
    document.removeEventListener("dragleave", handleGlobalDragLeave)
    document.removeEventListener("drop", handleGlobalDrop)
  })

  return {
    addImageAttachment,
    removeImageAttachment,
    handlePaste,
  }
}


Review comment (suggestion):
when we receive a @mention drop, we leverage the existing code path to handle it, injecting the `@<filepath>` directly into prompt 

Code after:
      input.setDraggingType("image")
    } else if (hasText) {
      input.setDraggingType("@mention")
    }
  }

  const handleGlobalDragLeave = (event: DragEvent) => {
    if (input.isDialogActive()) return
    if (!event.relatedTarget) {
      input.setDraggingType(null)
    }
  }

  const handleGlobalDrop = async (event: DragEvent) => {
    if (input.isDialogActive()) return

    event.preventDefault()
    input.setDraggingType(null)

    const plainText = event.dataTransfer?.getData("text/plain")
    const filePrefix = "file:"
    if (plainText?.startsWith(filePrefix)) {
      const filePath = plainText.slice(filePrefix.length)
      input.focusEditor()
      input.addPart({ type: "file", path: filePath, content: "@" + filePath, start: 0, end: 0 })
      return
    }

    const dropped = event.dataTransfer?.files
    if (!dropped) return

    for (const file of Array.from(dropped)) {
      if (ACCEPTED_FILE_TYPES.includes(file.type)) {
        await addImageAttachment(file)
      }
    }
  }

  onMount(() => {
    document.addEventListener("dragover", handleGlobalDragOver)
    document.addEventListener("dragleave", handleGlobalDragLeave)
    document.addEventListener("drop", handleGlobalDrop)
  })

  onCleanup(() => {
    document.removeEventListener("dragover", handleGlobalDragOver)
    document.removeEventListener("dragleave", handleGlobalDragLeave)
    document.removeEventListener("drop", handleGlobalDrop)
  })

  return {
Repository: sst/opencode
File: packages/desktop/src-tauri/src/cli.rs (Rust)

Code before:
                    let line = String::from_utf8_lossy(&line_bytes);
                    tracing::info!(target: "sidecar", "{line}");
                }
                CommandEvent::Stderr(line_bytes) => {
                    let line = String::from_utf8_lossy(&line_bytes);
                    tracing::info!(target: "sidecar", "{line}");
                }
                CommandEvent::Error(err) => {
                    tracing::error!(target: "sidecar", "{err}");
                }
                CommandEvent::Terminated(payload) => {
                    tracing::info!(
                        target: "sidecar",
                        code = ?payload.code,
                        signal = ?payload.signal,
                        "Sidecar terminated"
                    );

                    if let Some(tx) = exit_tx.take() {
                        let _ = tx.send(payload);
                    }
                }
                _ => {}
            }
        }
    });

    (child, exit_rx)
}


Review comment (suggestion):
Should stderr lines be logged as warnings?
```suggestion
                        tracing::warn!("{line}");
```

Code after:
    tracing::info!(port, "Spawning sidecar");

    let envs = [
        ("OPENCODE_SERVER_USERNAME", "opencode".to_string()),
        ("OPENCODE_SERVER_PASSWORD", password.to_string()),
    ];

    let (events, child) = spawn_command(
        app,
        format!("--print-logs --log-level WARN serve --hostname {hostname} --port {port}").as_str(),
        &envs,
    )
    .expect("Failed to spawn opencode");

    let mut exit_tx = Some(exit_tx);
    tokio::spawn(
        events
            .for_each(move |event| {
                match event {
                    CommandEvent::Stdout(line_bytes) => {
                        let line = String::from_utf8_lossy(&line_bytes);
                        tracing::info!("{line}");
                    }
                    CommandEvent::Stderr(line_bytes) => {
                        let line = String::from_utf8_lossy(&line_bytes);
                        tracing::info!("{line}");
                    }
                    CommandEvent::Error(err) => {
                        tracing::error!("{err}");
                    }
                    CommandEvent::Terminated(payload) => {
                        tracing::info!(
                            code = ?payload.code,
                            signal = ?payload.signal,
                            "Sidecar terminated"
                        );

                        if let Some(tx) = exit_tx.take() {
                            let _ = tx.send(payload);
                        }
                    }
                    _ => {}
                }

                future::ready(())
            })
            .instrument(tracing::info_span!("sidecar")),
    );

    (child, exit_rx)
}
Repository: stanford-oval/storm
File: knowledge_storm/rm.py (Python)

Code before:
import logging
import os
from typing import Callable, Union, List, Dict
from typing_extensions import Dict

import dspy
import pandas as pd
import requests
from langchain_core.documents import Document
from langchain_huggingface import HuggingFaceEmbeddings
from langchain_qdrant import Qdrant
from qdrant_client import QdrantClient, models
from tqdm import tqdm
import requests
import json
from utils import WebPageHelper


class YouRM(dspy.Retrieve):
    def __init__(self, ydc_api_key=None, k=3, is_valid_source: Callable = None):
        super().__init__(k=k)
        if not ydc_api_key and not os.environ.get("YDC_API_KEY"):
            raise RuntimeError(
                "You must supply ydc_api_key or set environment variable YDC_API_KEY"
            )
        elif ydc_api_key:
            self.ydc_api_key = ydc_api_key
        else:
            self.ydc_api_key = os.environ["YDC_API_KEY"]
        self.usage = 0

        # If not None, is_valid_source shall be a function that takes a URL and returns a boolean.
        if is_valid_source:
            self.is_valid_source = is_valid_source
        else:
            self.is_valid_source = lambda x: True

    def get_usage_and_reset(self):
        usage = self.usage
        self.usage = 0


Review comment (refactor):
I think it would better to be `from .utils import WebPageHelper` since we would later wrap the source code into a python package.

Code after:
import logging
import os
from typing import Callable, Union, List

import dspy
import pandas as pd
import requests
from langchain_core.documents import Document
from langchain_huggingface import HuggingFaceEmbeddings
from langchain_qdrant import Qdrant
from qdrant_client import QdrantClient, models
from tqdm import tqdm

from .utils import WebPageHelper


class YouRM(dspy.Retrieve):
    def __init__(self, ydc_api_key=None, k=3, is_valid_source: Callable = None):
        super().__init__(k=k)
        if not ydc_api_key and not os.environ.get("YDC_API_KEY"):
            raise RuntimeError("You must supply ydc_api_key or set environment variable YDC_API_KEY")
        elif ydc_api_key:
            self.ydc_api_key = ydc_api_key
        else:
            self.ydc_api_key = os.environ["YDC_API_KEY"]
        self.usage = 0

        # If not None, is_valid_source shall be a function that takes a URL and returns a boolean.
        if is_valid_source:
            self.is_valid_source = is_valid_source
        else:
            self.is_valid_source = lambda x: True

    def get_usage_and_reset(self):
        usage = self.usage
        self.usage = 0

        return {'YouRM': usage}

    def forward(self, query_or_queries: Union[str, List[str]], exclude_urls: List[str] = []):
        """Search with You.com for self.k top passages for query or queries
Repository: stanford-oval/storm
File: knowledge_storm/rm.py (Python)

Code before:
import logging
import os
from typing import Callable, Union, List
from duckduckgo_search import DDGS
import dspy
import requests

from langchain_huggingface import HuggingFaceEmbeddings
from langchain_qdrant import Qdrant
from qdrant_client import QdrantClient

from .utils import WebPageHelper


class YouRM(dspy.Retrieve):
    def __init__(self, ydc_api_key=None, k=3, is_valid_source: Callable = None):
        super().__init__(k=k)
        if not ydc_api_key and not os.environ.get("YDC_API_KEY"):
            raise RuntimeError("You must supply ydc_api_key or set environment variable YDC_API_KEY")
        elif ydc_api_key:
            self.ydc_api_key = ydc_api_key
        else:
            self.ydc_api_key = os.environ["YDC_API_KEY"]
        self.usage = 0

        # If not None, is_valid_source shall be a function that takes a URL and returns a boolean.
        if is_valid_source:
            self.is_valid_source = is_valid_source
        else:

Review comment (refactor):
Could we move this to `DuckDuckGoSearchRM.__init__()`? In this way, people don't necessarily need to install this package and we don't need to change `requirements.txt`.  You can do something similar to [here](https://github.com/stanford-oval/storm/blob/53b7f1ece52c1315694bb16e689e69a654332768/knowledge_storm/lm.py#L247)

Code after:
import logging
import os
from typing import Callable, Union, List
import dspy
import requests

from langchain_huggingface import HuggingFaceEmbeddings
from langchain_qdrant import Qdrant
from qdrant_client import QdrantClient

from .utils import WebPageHelper


class YouRM(dspy.Retrieve):
    def __init__(self, ydc_api_key=None, k=3, is_valid_source: Callable = None):
        super().__init__(k=k)
        if not ydc_api_key and not os.environ.get("YDC_API_KEY"):
            raise RuntimeError("You must supply ydc_api_key or set environment variable YDC_API_KEY")
        elif ydc_api_key:
            self.ydc_api_key = ydc_api_key
        else:
            self.ydc_api_key = os.environ["YDC_API_KEY"]
        self.usage = 0

        # If not None, is_valid_source shall be a function that takes a URL and returns a boolean.
        if is_valid_source:
            self.is_valid_source = is_valid_source
        else:
            self.is_valid_source = lambda x: True
Repository: stanford-oval/storm
File: knowledge_storm/rm.py (Python)

Code before:
import logging
import os
from typing import Callable, Union, List

import backoff
import dspy
import requests
from dsp import backoff_hdlr, giveup_hdlr

from langchain_huggingface import HuggingFaceEmbeddings
from langchain_qdrant import Qdrant
from qdrant_client import QdrantClient

from .utils import WebPageHelper

from azure.core.credentials import AzureKeyCredential
from azure.search.documents import SearchClient

class YouRM(dspy.Retrieve):
    def __init__(self, ydc_api_key=None, k=3, is_valid_source: Callable = None):
        super().__init__(k=k)
        if not ydc_api_key and not os.environ.get("YDC_API_KEY"):
            raise RuntimeError(
                "You must supply ydc_api_key or set environment variable YDC_API_KEY"
            )
        elif ydc_api_key:
            self.ydc_api_key = ydc_api_key
        else:
            self.ydc_api_key = os.environ["YDC_API_KEY"]
        self.usage = 0

        # If not None, is_valid_source shall be a function that takes a URL and returns a boolean.
        if is_valid_source:
            self.is_valid_source = is_valid_source
        else:
            self.is_valid_source = lambda x: True

    def get_usage_and_reset(self):
        usage = self.usage
        self.usage = 0


Review comment (security):
Could you move this inside `AzureAISearch`? You can add the following code (check out `GoogleSearch` for example)
```python
try:
    from azure.core.credentials import AzureKeyCredential
    from azure.search.documents import SearchClient
except ImportError as err:
    raise ImportError("AzureAISearch requires `pip install azure-search-documents`.") from err
```

Code after:
import logging
import os
from typing import Callable, Union, List

import backoff
import dspy
import requests
from dsp import backoff_hdlr, giveup_hdlr

from langchain_huggingface import HuggingFaceEmbeddings
from langchain_qdrant import Qdrant
from qdrant_client import QdrantClient

from .utils import WebPageHelper


class YouRM(dspy.Retrieve):
    def __init__(self, ydc_api_key=None, k=3, is_valid_source: Callable = None):
        super().__init__(k=k)
        if not ydc_api_key and not os.environ.get("YDC_API_KEY"):
            raise RuntimeError(
                "You must supply ydc_api_key or set environment variable YDC_API_KEY"
            )
        elif ydc_api_key:
            self.ydc_api_key = ydc_api_key
        else:
            self.ydc_api_key = os.environ["YDC_API_KEY"]
        self.usage = 0

        # If not None, is_valid_source shall be a function that takes a URL and returns a boolean.
        if is_valid_source:
            self.is_valid_source = is_valid_source
        else:
            self.is_valid_source = lambda x: True

    def get_usage_and_reset(self):
        usage = self.usage
        self.usage = 0

        return {"YouRM": usage}
Repository: sz3/libcimbar
File: src/lib/cimb_translator/Common.cpp (C++)

Code before:
#include <string>
#include "bitmaps.h"

using std::array;
using std::string;
using std::vector;

namespace cimbar {

cv::Mat load_img(string path, const string& image_dir)
{
	if (image_dir != "")
		return cv::imread(image_dir + "/" + path);

	auto it = cimbar::bitmaps.find(path);
	if (it == cimbar::bitmaps.end())
		return cv::imread(path);

	string bytes = base91::decode(it->second);
	vector<char> data(bytes.data(), bytes.data() + bytes.size());
	return cv::imdecode(data, cv::IMREAD_COLOR);
}

RGB getColor(unsigned index)
{
	// opencv uses BGR, but we don't have to conform to this tyranny
	static constexpr array<RGB, 8> colors = {
	    RGB(0, 0xFF, 0xFF),
	    RGB(0xFF, 0xFF, 0),
	    RGB(0xFF, 0, 0xFF),
	    RGB(0, 0xFF, 0),
	    RGB(0, 0x7F, 0xFF),  // mid-blue
	    RGB(0xFF, 0xFF, 0xFF),
	    RGB(0xFF, 0, 0),
	    RGB(0xFF, 0x7F, 0),  // orange
	};
	return colors[index];
}

cv::Mat getTile(unsigned symbol_bits, unsigned symbol, bool dark, unsigned color, const string& image_dir)
{
	static cv::Vec3b background({0xFF, 0xFF, 0xFF});

	string imgPath = fmt::format("bitmap/{}/{:02x}.png", symbol_bits, symbol);
	cv::Mat tile = load_img(imgPath, image_dir);

	uchar r, g, b;
	std::tie(r, g, b) = getColor(color);
	cv::MatIterator_<cv::Vec3b> end = tile.end<cv::Vec3b>();
	for (cv::MatIterator_<cv::Vec3b> it = tile.begin<cv::Vec3b>(); it != end; ++it)
	{

Review comment (suggestion):
This particular color is up for debate. If the Red value is too high (and `7F` may be too high), it's close to magenta. If it's too low, the symbol is too dark overall, and we tend to get more symbol read errors. If the Green value is too high, it's too close to cyan.

(incidentally, the reason the colors are ordered the way they are is to make sure color errors are single-bit errors)

Code after:
#include <string>
#include "bitmaps.h"

using cimbar::RGB;
using std::array;
using std::string;
using std::vector;

namespace {
	RGB getColor4(unsigned index)
	{
		// opencv uses BGR, but we don't have to conform to this tyranny
		static constexpr array<RGB, 4> colors = {
		    RGB(0, 0xFF, 0xFF),
		    RGB(0xFF, 0xFF, 0),
		    RGB(0xFF, 0, 0xFF),
		    RGB(0, 0xFF, 0)
		};
		return colors[index];
	}

	RGB getColor8(unsigned index)
	{
		static constexpr array<RGB, 8> colors = {
		    RGB(0, 0xFF, 0xFF), // cyan
		    RGB(0x7F, 0x7F, 0xFF),  // mid-blue
		    RGB(0xFF, 0, 0xFF), // magenta
		    RGB(0xFF, 65, 65), // red
		    RGB(0xFF, 0x9F, 0),  // orange
		    RGB(0xFF, 0xFF, 0), // yellow
		    RGB(0xFF, 0xFF, 0xFF),
		    RGB(0, 0xFF, 0),
		};
		return colors[index];
	}
}

namespace cimbar {

cv::Mat load_img(string path, const string& image_dir)
{
	if (image_dir != "")
		return cv::imread(image_dir + "/" + path);

	auto it = cimbar::bitmaps.find(path);
	if (it == cimbar::bitmaps.end())
		return cv::imread(path);

	string bytes = base91::decode(it->second);
	vector<char> data(bytes.data(), bytes.data() + bytes.size());
	return cv::imdecode(data, cv::IMREAD_COLOR);
Repository: sz3/libcimbar
File: src/lib/cimb_translator/CimbDecoder.cpp (C++)

Code before:
#include "image_hash/hamming_distance.h"
#include "serialize/format.h"

#include <algorithm>
#include <iostream>
#include <tuple>
using std::get;
using std::string;

namespace {
	unsigned squared_difference(uchar a, uchar b)
	{
		return std::pow(a - b, 2);
	}

	uchar fix_single_color(uchar c, float adjustUp, uchar down)
	{
		c -= down;
		return (uchar)(c * adjustUp);
	}
}

CimbDecoder::CimbDecoder(unsigned symbol_bits, unsigned color_bits, bool dark, uchar ahashThreshold)
    : _symbolBits(symbol_bits)
    , _numSymbols(1 << symbol_bits)
    , _numColors(1 << color_bits)
    , _dark(dark)
    , _ahashThreshold(ahashThreshold)
{
	load_tiles();
}

uint64_t CimbDecoder::get_tile_hash(unsigned symbol) const
{
	cv::Mat tile = cimbar::getTile(_symbolBits, symbol, _dark);
	return image_hash::average_hash(tile);
}

bool CimbDecoder::load_tiles()
{
	unsigned numTiles = _numSymbols;
	for (unsigned i = 0; i < numTiles; ++i)
		_tileHashes.push_back(get_tile_hash(i));
	return true;
}

unsigned CimbDecoder::get_best_symbol(image_hash::ahash_result& results, unsigned& drift_offset, unsigned& best_distance) const
{
	drift_offset = 0;
	unsigned best_fit = 0;
	best_distance = 1000;

Review comment (suggestion):
Another magic number, probably overfit but also probably close -- the general idea here is to threshold the max value to 255, since if it's this high, it *should* be 255. Where to set the threshold is another question -- this feels a bit hack-y, because `down` isn't actually what we scale down by (perhaps it should be `down*adjustUp`).

I'm not particularly thrilled with the decode color logic at the moment. But, it's a start.

Code after:
#include "image_hash/hamming_distance.h"
#include "serialize/format.h"

#include <algorithm>
#include <iostream>
#include <tuple>
using std::get;
using std::string;

namespace {
	inline constexpr unsigned char operator"" _uchar(unsigned long long arg) noexcept
	{
		return static_cast<unsigned char>(arg);
	}

	template <typename T>
	unsigned squared_difference(T a, T b)
	{
		return std::pow(a - b, 2);
	}

	uchar fix_single_color(uchar c, float adjustUp, uchar down)
	{
		c -= down;
		c = (uchar)(c * adjustUp);
		if (c > (245 - down))
			c = 255;
		return c;
	}

	std::tuple<int,int,int> relative_color(std::tuple<uchar,uchar,uchar> c)
	{
		int r = std::get<0>(c);
		int g = std::get<1>(c);
		int b = std::get<2>(c);
		return {r - g, g - b, b - r};
	}

	unsigned color_diff(std::tuple<uchar,uchar,uchar> a, std::tuple<uchar,uchar,uchar> b)
	{
		std::tuple<int,int,int> rel1 = relative_color(a);
		std::tuple<int,int,int> rel2 = relative_color(b);
		return (
		    squared_difference(std::get<0>(rel1), std::get<0>(rel2)) +
		    squared_difference(std::get<1>(rel1), std::get<1>(rel2)) +
		    squared_difference(std::get<2>(rel1), std::get<2>(rel2))
		);
	}
}

CimbDecoder::CimbDecoder(unsigned symbol_bits, unsigned color_bits, bool dark, uchar ahashThreshold)
Repository: sz3/libcimbar
File: src/exe/cimbar/cimbar.cpp (C++)

Code before:
#include "fountain/fountain_decoder_sink.h"
#include "serialize/str.h"

#include "cxxopts/cxxopts.hpp"

#include <functional>
#include <iostream>
#include <string>
#include <vector>
using std::string;
using std::vector;

int decode(const vector<string>& infiles, std::function<int(cv::UMat, bool)>& decode, bool no_deskew, bool undistort, int preprocess)
{
	int err = 0;
	Undistort<SimpleCameraCalibration> und;
	for (const string& inf : infiles)
	{
		bool shouldPreprocess = (preprocess == 1);
		cv::UMat img = cv::imread(inf).getUMat(cv::ACCESS_RW);
		if (!no_deskew)
		{
			// attempt undistort
			// we don't fail outright, but we'll probably fail the decode :(
			if (undistort)
			{
				if (!und.undistort(img, img))
					err |= 1;
			}

			Extractor ext;
			int res = ext.extract(img, img);
			if (!res)
			{
				err |= 2;
				continue;
			}
			else if (preprocess != 0 and res == Extractor::NEEDS_SHARPEN)
				shouldPreprocess = true;
		}

		int bytes = decode(img, shouldPreprocess);
		if (!bytes)
			err |= 4;
	}
	return err;
}

int main(int argc, char** argv)
{
	cxxopts::Options options("cimbar encoder/decoder", "Demonstration program for cimbar codes");

Review comment (suggestion):
SimpleCameraCalibration is a hack, and will only work for a specific image -- so the `Undistort` caching it doesn't actually help.

(the initial idea was for the Undistort to also be able to load pre-computed camera parameters -- *that* should still work... but isn't implemented right now)

Code after:
#include "fountain/fountain_decoder_sink.h"
#include "serialize/str.h"

#include "cxxopts/cxxopts.hpp"

#include <functional>
#include <iostream>
#include <string>
#include <vector>
using std::string;
using std::vector;

int decode(const vector<string>& infiles, std::function<int(cv::UMat, bool)>& decode, bool no_deskew, bool undistort, int preprocess)
{
	int err = 0;
	for (const string& inf : infiles)
	{
		bool shouldPreprocess = (preprocess == 1);
		cv::UMat img = cv::imread(inf).getUMat(cv::ACCESS_RW);
		if (!no_deskew)
		{
			// attempt undistort
			// we don't fail outright, but we'll probably fail the decode :(
			if (undistort)
			{
				Undistort<SimpleCameraCalibration> und;
				if (!und.undistort(img, img))
					err |= 1;
			}

			Extractor ext;
			int res = ext.extract(img, img);
			if (!res)
			{
				err |= 2;
				continue;
			}
			else if (preprocess != 0 and res == Extractor::NEEDS_SHARPEN)
				shouldPreprocess = true;
		}

		int bytes = decode(img, shouldPreprocess);
		if (!bytes)
			err |= 4;
	}
	return err;
}

int main(int argc, char** argv)
{
	cxxopts::Options options("cimbar encoder/decoder", "Demonstration program for cimbar codes");