ronantakizawa/codereview-bench huggingface dataset:
Repository: TabbyML/tabby
File: clients/tabby-agent/src/protocol.ts (TypeScript)

Code before:
   * The edits to apply.
   */
  edit: WorkspaceEdit;
  options?: {
    /**
     * Add undo stop before making the edits.
     */
    readonly undoStopBefore: boolean;
    /**
     * Add undo stop after making the edits.
     */
    readonly undoStopAfter: boolean;
  };
}

export type ChatEditResolveCommand = LspCommand & {
  title: string;
  tooltip?: string;
  command: "tabby/chat/edit/resolve";
  arguments: [ChatEditResolveParams];
};

/**
 * [Tabby] Opened File Tracking Request(➡️)
 *
 * This method is sent from the client to server with specific action to track opened file.
 *
 *
 * - method: `tabby/agent/openedFileOperation`
 * - params: {@link OpenedFileParams}
 * - result: void
 */
export namespace OpenedFileRequest {
  export const method = "tabby/agent/openedFileOperation";
  export const messageDirection = MessageDirection.clientToServer;
  export const type = new ProtocolNotificationType<OpenedFileParams, void>(method);
}
export interface EditorParams {
  uri: string;
  visibleRange: Range;
}
export type OpenedFileParams = {
  action: "change" | "test";
  activeEditor: EditorParams;
  visibleEditors?: EditorParams[];
};

/**
 * [Tabby] GenerateCommitMessage Request(↩️)
 *
 * This method is sent from the client to the server to generate a commit message for a git repository.

Review comment (suggestion):
```suggestion
 * [Tabby] Did Change Active Editor Notification(➡️)
 *
 * This method is sent from the client to server when the active editor changed.
```

Code after:
   * The edits to apply.
   */
  edit: WorkspaceEdit;
  options?: {
    /**
     * Add undo stop before making the edits.
     */
    readonly undoStopBefore: boolean;
    /**
     * Add undo stop after making the edits.
     */
    readonly undoStopAfter: boolean;
  };
}

export type ChatEditResolveCommand = LspCommand & {
  title: string;
  tooltip?: string;
  command: "tabby/chat/edit/resolve";
  arguments: [ChatEditResolveParams];
};

/**
 * [Tabby] Did Change Active Editor Notification(➡️)
 *
 * This method is sent from the client to server when the active editor changed.
 *
 *
 * - method: `tabby/editors/didChangeActiveEditor`
 * - params: {@link OpenedFileParams}
 * - result: void
 */
export namespace DidChangeActiveEditorNotification {
  export const method = "tabby/editors/didChangeActiveEditor";
  export const messageDirection = MessageDirection.clientToServer;
  export const type = new ProtocolNotificationType<DidChangeActiveEditorParams, void>(method);
}
export type DidChangeActiveEditorParams = {
  activeEditor: Location;
  visibleEditors: Location[] | undefined;
};

/**
 * [Tabby] GenerateCommitMessage Request(↩️)
 *
 * This method is sent from the client to the server to generate a commit message for a git repository.
 * - method: `tabby/chat/generateCommitMessage`
 * - params: {@link GenerateCommitMessageParams}
 * - result: {@link GenerateCommitMessageResult} | null
 * - error: {@link ChatFeatureNotAvailableError}
 */
Repository: TabbyML/tabby
File: clients/tabby-agent/src/codeCompletion/contexts.ts (TypeScript)

Code before:
            );
          }),
      )
      .map((snippet) => {
        let snippetFilepath = snippet.filepath;
        if (relativeFilepathRoot && snippetFilepath.startsWith(relativeFilepathRoot)) {
          snippetFilepath = path.relative(relativeFilepathRoot, snippetFilepath);
        }
        return {
          filepath: snippetFilepath,
          body: snippet.text,
          score: snippet.score,
        };
      })
      .sort((a, b) => b.score - a.score);

    const snippetsOpenedFiles = this.snippetsFromOpenedFiles
      ?.map((snippet) => {
        return {
          filepath: snippet.filepath,
          body: snippet.text,
          score: snippet.score,
        };
      })
      .sort((a, b) => b.score - a.score);

    // clipboard
    let clipboard = undefined;
    if (this.clipboard.length >= config.clipboard.minChars && this.clipboard.length <= config.clipboard.maxChars) {
      clipboard = this.clipboard;
    }
    return {
      prefix,
      suffix,
      filepath,
      git_url: gitUrl,
      declarations,
      relevant_snippets_from_changed_files: relevantSnippetsFromChangedFiles,
      relevant_snippets_from_recently_opened_files: snippetsOpenedFiles,
      clipboard,
    };
  }
}


Review comment (refactor):
The comments in https://github.com/TabbyML/tabby/blob/main/crates/tabby/src/services/completion.rs#L147 says that the snippets should have been deduplicated at client-side.  
It seems that this is not implemented. Please leave a FIXME if it is not planned in this PR.

Code after:
            );
          }),
      )
      .map((snippet) => {
        let snippetFilepath = snippet.filepath;
        if (relativeFilepathRoot && snippetFilepath.startsWith(relativeFilepathRoot)) {
          snippetFilepath = path.relative(relativeFilepathRoot, snippetFilepath);
        }
        return {
          filepath: snippetFilepath,
          body: snippet.text,
          score: snippet.score,
        };
      })
      .sort((a, b) => b.score - a.score);

    //FIXME(Sma1lboy): deduplicate in next few PR
    const snippetsOpenedFiles = this.snippetsFromOpenedFiles
      ?.map((snippet) => {
        return {
          filepath: snippet.filepath,
          body: snippet.text,
          score: snippet.score,
        };
      })
      .sort((a, b) => b.score - a.score);

    // clipboard
    let clipboard = undefined;
    if (this.clipboard.length >= config.clipboard.minChars && this.clipboard.length <= config.clipboard.maxChars) {
      clipboard = this.clipboard;
    }
    return {
      prefix,
      suffix,
      filepath,
      git_url: gitUrl,
      declarations,
      relevant_snippets_from_changed_files: relevantSnippetsFromChangedFiles,
      relevant_snippets_from_recently_opened_files: snippetsOpenedFiles,
      clipboard,
    };
  }
}
Repository: TabbyML/tabby
File: ee/tabby-webserver/src/service/answer.rs (Rust)

Code before:
                insert_hit.scores.bm25 /= len;
                insert_hit.scores.embedding /= len;
                insert_hit.scores.rrf /= len;
                insert_hit.doc.body = all_lines.join("\n");
                result.push(insert_hit);
            }
        } else {
            result.extend(file_hits);
        }
    }
    result
}

#[derive(Debug)]
// Error for when the file is too large
struct FileTooLargeError;

impl fmt::Display for FileTooLargeError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "File is too large (more than 200 lines)")
    }
}

impl Error for FileTooLargeError {}

//directly read the file content
pub fn read_file_content(path: &PathBuf) -> Result<Vec<String>, Box<dyn Error>> {
    let file = File::open(path).map_err(|e| {
        warn!("Error opening file {}: {}", path.display(), e);
        e
    })?;

    let reader = BufReader::new(file);
    let all_lines: Vec<String> = reader.lines().collect::<Result<_, _>>()?;

    if all_lines.len() > 200 {
        Err(Box::new(FileTooLargeError))
    } else {
        Ok(all_lines)
    }
}

#[cfg(test)]
pub mod testutils;

#[cfg(test)]
mod tests {

    use std::{path::PathBuf, sync::Arc};

    use juniper::ID;

Review comment (suggestion):
```suggestion
/// Read file content, ....
```

Document it will return nothing if line content > 200.

Code after:
                        });
                // average the scores
                let num_files = file_hits.len() as f32;
                insert_hit.scores.bm25 /= num_files;
                insert_hit.scores.embedding /= num_files;
                insert_hit.scores.rrf /= num_files;
                insert_hit.doc.body = file_content;
                result.push(insert_hit);
            }
        } else {
            result.extend(file_hits);
        }
    }
    result
}

/// Read file content and return raw file content string, it will return nothing if the file is over 200 lines
pub fn read_file_content(path: &Path) -> Option<String> {
    if count_lines(path).ok()? > 200 {
        return None;
    }

    let mut file = match File::open(path) {
        Ok(file) => file,
        Err(e) => {
            warn!("Error opening file {}: {}", path.display(), e);
            return None;
        }
    };
    let mut content = String::new();
    match file.read_to_string(&mut content) {
        Ok(_) => Some(content),
        Err(e) => {
            warn!("Error reading file {}: {}", path.display(), e);
            None
        }
    }
}

fn count_lines(path: &Path) -> std::io::Result<usize> {
    let mut count = 0;
    for line in BufReader::new(File::open(path)?).lines() {
        line?;
        count += 1;
    }
    Ok(count)
}

#[cfg(test)]
pub mod testutils;
Repository: TabbyML/tabby
File: ee/tabby-webserver/src/service/answer.rs (Rust)

Code before:
                insert_hit.scores.embedding /= len;
                insert_hit.scores.rrf /= len;
                insert_hit.doc.body = all_lines.join("\n");
                result.push(insert_hit);
            }
        } else {
            result.extend(file_hits);
        }
    }
    result
}

#[derive(Debug)]
// Error for when the file is too large
struct FileTooLargeError;

impl fmt::Display for FileTooLargeError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "File is too large (more than 200 lines)")
    }
}

impl Error for FileTooLargeError {}

//directly read the file content
pub fn read_file_content(path: &PathBuf) -> Result<Vec<String>, Box<dyn Error>> {
    let file = File::open(path).map_err(|e| {
        warn!("Error opening file {}: {}", path.display(), e);
        e
    })?;

    let reader = BufReader::new(file);
    let all_lines: Vec<String> = reader.lines().collect::<Result<_, _>>()?;

    if all_lines.len() > 200 {
        Err(Box::new(FileTooLargeError))
    } else {
        Ok(all_lines)
    }
}

#[cfg(test)]
pub mod testutils;

#[cfg(test)]
mod tests {

    use std::{path::PathBuf, sync::Arc};

    use juniper::ID;
    use tabby_common::{

Review comment (suggestion):
```suggestion
fn read_file_content(path: &Path) -> Option<String> {
```

Code after:
                // average the scores
                let num_files = file_hits.len() as f32;
                insert_hit.scores.bm25 /= num_files;
                insert_hit.scores.embedding /= num_files;
                insert_hit.scores.rrf /= num_files;
                insert_hit.doc.body = file_content;
                result.push(insert_hit);
            }
        } else {
            result.extend(file_hits);
        }
    }
    result
}

/// Read file content and return raw file content string, it will return nothing if the file is over 200 lines
pub fn read_file_content(path: &Path) -> Option<String> {
    if count_lines(path).ok()? > 200 {
        return None;
    }

    let mut file = match File::open(path) {
        Ok(file) => file,
        Err(e) => {
            warn!("Error opening file {}: {}", path.display(), e);
            return None;
        }
    };
    let mut content = String::new();
    match file.read_to_string(&mut content) {
        Ok(_) => Some(content),
        Err(e) => {
            warn!("Error reading file {}: {}", path.display(), e);
            None
        }
    }
}

fn count_lines(path: &Path) -> std::io::Result<usize> {
    let mut count = 0;
    for line in BufReader::new(File::open(path)?).lines() {
        line?;
        count += 1;
    }
    Ok(count)
}

#[cfg(test)]
pub mod testutils;

#[cfg(test)]
Repository: TabbyML/tabby
File: crates/tabby/src/routes/completions.rs (Rust)

Code before:
    Json(mut request): Json<CompletionRequest>,
) -> Result<Json<CompletionResponse>, StatusCode> {
    if let Some(user) = user {
        request.user.replace(user);
    }

    let user_agent = user_agent.map(|x| x.0.to_string());

    let mut use_crlf = false;
    if let Some(segments) = request.segments {
        let mut new_segments = segments.clone();
        if segments.prefix.contains("\r\n") {
            use_crlf = true;
            new_segments.prefix = segments.prefix.replace("\r\n", "\n");
        }
        if let Some(suffix) = segments.suffix {
            if suffix.contains("\r\n") {
                use_crlf = true;
                new_segments.suffix = Some(suffix.replace("\r\n", "\n"));
            }
        }
        request.segments = Some(new_segments);
    }

    match state
        .generate(&request, &allowed_code_repository, user_agent.as_deref())
        .await
    {
        Ok(resp) => {
            if use_crlf {
                let mut response_crlf = resp.clone();
                for (index, choice) in resp.choices.iter().enumerate() {
                    response_crlf.choices[index].text = choice.text.replace("\n", "\r\n");
                }

                return Ok(Json(response_crlf));
            }

            Ok(Json(resp))
        }
        Err(err) => {
            warn!("{}", err);
            Err(StatusCode::BAD_REQUEST)
        }
    }
}


Review comment (suggestion):
the crlf / lf logic shall go inside of `CompletionService.generate`, instead of being implemented in `routes/…`

Code after:
use std::sync::Arc;

use axum::{extract::State, Extension, Json};
use axum_extra::{headers, TypedHeader};
use hyper::StatusCode;
use tabby_common::axum::{AllowedCodeRepository, MaybeUser};
use tracing::{instrument, warn};

use crate::services::completion::{CompletionRequest, CompletionResponse, CompletionService};

#[utoipa::path(
    post,
    path = "/v1/completions",
    request_body = CompletionRequest,
    operation_id = "completion",
    tag = "v1",
    responses(
        (status = 200, description = "Success", body = CompletionResponse, content_type = "application/json"),
        (status = 400, description = "Bad Request")
    ),
    security(
        ("token" = [])
    )
)]
#[instrument(skip(state, request))]
pub async fn completions(
    State(state): State<Arc<CompletionService>>,
    Extension(allowed_code_repository): Extension<AllowedCodeRepository>,
    TypedHeader(MaybeUser(user)): TypedHeader<MaybeUser>,
    user_agent: Option<TypedHeader<headers::UserAgent>>,
    Json(mut request): Json<CompletionRequest>,
) -> Result<Json<CompletionResponse>, StatusCode> {
    if let Some(user) = user {
        request.user.replace(user);
    }

    let user_agent = user_agent.map(|x| x.0.to_string());

    match state
        .generate(&request, &allowed_code_repository, user_agent.as_deref())
        .await
    {
        Ok(resp) => Ok(Json(resp)),
        Err(err) => {
            warn!("{}", err);
            Err(StatusCode::BAD_REQUEST)
        }
    }
}
Repository: TabbyML/tabby
File: ee/tabby-schema/src/schema/mod.rs (Rust)

Code before:
            },
        )
        .await
    }

    /// List user groups.
    async fn user_groups(ctx: &Context) -> Result<Vec<UserGroup>> {
        check_user(ctx).await?;
        ctx.locator.user_group().list().await
    }

    async fn source_id_access_policies(
        ctx: &Context,
        source_id: String,
    ) -> Result<SourceIdAccessPolicy> {
        check_admin(ctx).await?;
        let read = ctx
            .locator
            .access_policy()
            .list_source_id_read_access(&source_id)
            .await?;

        Ok(SourceIdAccessPolicy { source_id, read })
    }

    async fn test_model_connection(ctx: &Context) -> Result<ModelHealth> {
        check_user_allow_auth_token(ctx).await?;

        let mut health = ModelHealth {
            completion: false,
            chat: false,
            embeddings: false,
        };

        if let Some(chat) = ctx.locator.chat() {
            let request = CreateChatCompletionRequest::default();
            if let Ok(_) = chat.chat(request).await {
                health.chat = true;
            }
        }

        Ok(health)
    }
}

#[derive(GraphQLObject)]
pub struct ServerInfo {
    is_admin_initialized: bool,
    is_chat_enabled: bool,
    is_email_configured: bool,
    allow_self_signup: bool,

Review comment (performance):
adding argument (can be an enum) of `chat`, `completion`, `embedding`, returns output of a struct contains:
1. whether the test is success.
2. latency

Code after:
        first: Option<i32>,
        last: Option<i32>,
    ) -> Result<Connection<CustomWebDocument>> {
        check_admin(ctx).await?;
        query_async(
            after,
            before,
            first,
            last,
            |after, before, first, last| async move {
                ctx.locator
                    .web_documents()
                    .list_custom_web_documents(ids, after, before, first, last)
                    .await
            },
        )
        .await
    }
    async fn preset_web_documents(
        ctx: &Context,
        ids: Option<Vec<ID>>,
        after: Option<String>,
        before: Option<String>,
        first: Option<i32>,
        last: Option<i32>,
        is_active: Option<bool>,
    ) -> Result<Connection<PresetWebDocument>> {
        check_admin(ctx).await?;
        query_async(
            after,
            before,
            first,
            last,
            |after, before, first, last| async move {
                ctx.locator
                    .web_documents()
                    .list_preset_web_documents(ids, after, before, first, last, is_active)
                    .await
            },
        )
        .await
    }

    /// List user groups.
    async fn user_groups(ctx: &Context) -> Result<Vec<UserGroup>> {
        check_user(ctx).await?;
        ctx.locator.user_group().list().await
    }

    async fn source_id_access_policies(
        ctx: &Context,
Repository: TabbyML/tabby
File: ee/tabby-webserver/src/service/mod.rs (Rust)

Code before:

    fn web_documents(&self) -> Arc<dyn WebDocumentService> {
        self.0.web_documents.clone()
    }

    fn thread(&self) -> Arc<dyn ThreadService> {
        self.0.thread.clone()
    }

    fn context(&self) -> Arc<dyn ContextService> {
        self.0.context.clone()
    }

    fn user_group(&self) -> Arc<dyn UserGroupService> {
        self.0.user_group.clone()
    }

    fn access_policy(&self) -> Arc<dyn AccessPolicyService> {
        self.0.access_policy.clone()
    }
}

pub async fn create_service_locator(
    logger: Arc<dyn EventLogger>,
    chat: Option<Arc<dyn ChatCompletionStream>>,
    completion: Option<Arc<CodeGeneration>>,
    code: Arc<dyn CodeSearch>,
    repository: Arc<dyn RepositoryService>,
    integration: Arc<dyn IntegrationService>,
    job: Arc<dyn JobService>,
    answer: Option<Arc<AnswerService>>,
    context: Arc<dyn ContextService>,
    web_documents: Arc<dyn WebDocumentService>,
    db: DbConn,
    embedding: Arc<dyn EmbeddingService>,
) -> Arc<dyn ServiceLocator> {
    Arc::new(ArcServerContext::new(
        ServerContext::new(
            logger,
            chat,
            completion,
            code,
            repository,
            integration,
            job,
            answer,
            context,
            web_documents,
            db,
            embedding,
        )

Review comment (suggestion):
```suggestion
    completion: Option<Arc<CompletionStream>>,
```

`CodeGeneration` is a wrapper interface - use CompletionStream directly.

Code after:

    fn web_documents(&self) -> Arc<dyn WebDocumentService> {
        self.0.web_documents.clone()
    }

    fn thread(&self) -> Arc<dyn ThreadService> {
        self.0.thread.clone()
    }

    fn context(&self) -> Arc<dyn ContextService> {
        self.0.context.clone()
    }

    fn user_group(&self) -> Arc<dyn UserGroupService> {
        self.0.user_group.clone()
    }

    fn access_policy(&self) -> Arc<dyn AccessPolicyService> {
        self.0.access_policy.clone()
    }
}

pub async fn create_service_locator(
    logger: Arc<dyn EventLogger>,
    chat: Option<Arc<dyn ChatCompletionStream>>,
    completion: Option<Arc<dyn CompletionStream>>,
    code: Arc<dyn CodeSearch>,
    repository: Arc<dyn RepositoryService>,
    integration: Arc<dyn IntegrationService>,
    job: Arc<dyn JobService>,
    answer: Option<Arc<AnswerService>>,
    context: Arc<dyn ContextService>,
    web_documents: Arc<dyn WebDocumentService>,
    db: DbConn,
    embedding: Arc<dyn EmbeddingService>,
) -> Arc<dyn ServiceLocator> {
    Arc::new(ArcServerContext::new(
        ServerContext::new(
            logger,
            chat,
            completion,
            code,
            repository,
            integration,
            job,
            answer,
            context,
            web_documents,
            db,
            embedding,
        )
Repository: TabbyML/tabby
File: ee/tabby-schema/src/schema/mod.rs (Rust)

Code before:
    }
    let user = ctx.locator.auth().get_user(&claims.sub).await?;
    Ok(user)
}

async fn check_license(ctx: &Context, license_type: &[LicenseType]) -> Result<(), CoreError> {
    let license = ctx.locator.license().read().await?;

    if !license_type.contains(&license.r#type) {
        return Err(CoreError::InvalidLicense(
            "Your plan doesn't include support for this feature.",
        ));
    }

    license.ensure_valid_license()
}

#[derive(GraphQLEnum)]
enum ModelHealthBackend {
    Chat,
    Completion,
    Embedding,
}

#[derive(GraphQLObject, Debug, Clone)]
struct ModelHealthResponse {
    latency: i32,
}

#[derive(Default)]
pub struct Query;

#[graphql_object(context = Context)]
impl Query {
    async fn registration_token(ctx: &Context) -> Result<String> {
        check_admin(ctx).await?;
        ctx.locator.worker().read_registration_token().await
    }

    async fn me(ctx: &Context) -> Result<UserSecured> {
        check_user_allow_auth_token(ctx).await
    }

    /// List users, accessible for all login users.
    async fn users(
        ctx: &Context,
        after: Option<String>,
        before: Option<String>,
        first: Option<i32>,
        last: Option<i32>,
    ) -> Result<Connection<UserValue>> {

Review comment (suggestion):
```suggestion
struct ModelBackendHealthInfo {
```

Code after:
        return Err(CoreError::Forbidden(
            "Invoking this API with an auth token is not allowed",
        ));
    }
    let user = ctx.locator.auth().get_user(&claims.sub).await?;
    Ok(user)
}

async fn check_license(ctx: &Context, license_type: &[LicenseType]) -> Result<(), CoreError> {
    let license = ctx.locator.license().read().await?;

    if !license_type.contains(&license.r#type) {
        return Err(CoreError::InvalidLicense(
            "Your plan doesn't include support for this feature.",
        ));
    }

    license.ensure_valid_license()
}

#[derive(GraphQLEnum)]
enum ModelHealthBackend {
    Chat,
    Completion,
    Embedding,
}

#[derive(GraphQLObject, Debug, Clone)]
struct ModelBackendHealthInfo {
    /// Latency in milliseconds.
    latency_ms: i32,
}

#[derive(Default)]
pub struct Query;

#[graphql_object(context = Context)]
impl Query {
    async fn registration_token(ctx: &Context) -> Result<String> {
        check_admin(ctx).await?;
        ctx.locator.worker().read_registration_token().await
    }

    async fn me(ctx: &Context) -> Result<UserSecured> {
        check_user_allow_auth_token(ctx).await
    }

    /// List users, accessible for all login users.
    async fn users(
        ctx: &Context,
        after: Option<String>,