ronantakizawa/codereview-bench huggingface dataset:
Repository: ragapp/ragapp
File: admin/sections/config/providers/shared.tsx (TypeScript)

Code before:
import {
  Select,
  SelectContent,
  SelectItem,
  SelectTrigger,
  SelectValue,
} from "@/components/ui/select";
import { UseFormReturn } from "react-hook-form";

const DEFAULT_TITLE = "Model";
const DEFAULT_DESCRIPTION =
  "Select a model to chat with. If you are not sure, leave it as is.";
const DEFAULT_MODEL_FIELD_NAME = "model";

export const ModelForm = ({
  form,
  title,
  description,
  model_field_name,
  defaultValues,
  supportedModels,
}: {
  form: UseFormReturn;
  title?: string;
  description?: string;
  model_field_name?: string;
  defaultValues: any;
  supportedModels: string[];
}) => {
  return (
    <FormField
      control={form.control}
      name={model_field_name ?? DEFAULT_MODEL_FIELD_NAME}
      render={({ field }) => (
        <FormItem>
          <FormLabel>{title ?? DEFAULT_TITLE}</FormLabel>
          <FormControl>
            <Select
              defaultValue={defaultValues.model ?? supportedModels[0]}
              onValueChange={field.onChange}
              {...field}
            >
              <SelectTrigger>
                <SelectValue placeholder={supportedModels[0]} />
              </SelectTrigger>
              <SelectContent>
                {supportedModels?.map((model: string) => (
                  <SelectItem key={model} value={model}>
                    {model}
                  </SelectItem>
                ))}

Review comment (suggestion):
FormField's call this just `name` - how about?
```suggestion
  name?: string;
```

Code after:
import {
  Select,
  SelectContent,
  SelectItem,
  SelectTrigger,
  SelectValue,
} from "@/components/ui/select";
import { UseFormReturn } from "react-hook-form";

const DEFAULT_TITLE = "Model";
const DEFAULT_DESCRIPTION =
  "Select a model to chat with. If you are not sure, leave it as is.";
const DEFAULT_NAME = "model";

export const ModelForm = ({
  form,
  title,
  description,
  name,
  defaultValue,
  supportedModels,
}: {
  form: UseFormReturn;
  title?: string;
  description?: string;
  name?: string;
  defaultValue: any;
  supportedModels: string[];
}) => {
  return (
    <FormField
      control={form.control}
      name={name ?? DEFAULT_NAME}
      render={({ field }) => (
        <FormItem>
          <FormLabel>{title ?? DEFAULT_TITLE}</FormLabel>
          <FormControl>
            <Select
              defaultValue={defaultValue ?? supportedModels[0]}
              onValueChange={field.onChange}
              {...field}
            >
              <SelectTrigger>
                <SelectValue placeholder={supportedModels[0]} />
              </SelectTrigger>
              <SelectContent>
                {supportedModels?.map((model: string) => (
                  <SelectItem key={model} value={model}>
                    {model}
                  </SelectItem>
                ))}
Repository: ragapp/ragapp
File: patch/frontend/app/components/ui/chat/hooks/use-config.ts (TypeScript)

Code before:
"use client";

import { useEffect, useMemo, useState } from "react";

export interface ChatConfig {
  backend?: string;
  starterQuestions?: string[];
}

export function useClientConfig(): ChatConfig {
  const chatAPI = process.env.NEXT_PUBLIC_CHAT_API;
  const [config, setConfig] = useState<ChatConfig>();

  const backendOrigin = useMemo(() => {
    if (chatAPI) {
      return chatAPI || "";
    } else {
      if (typeof window !== "undefined") {
        // Use BASE_URL from window.ENV
        return (window as any).ENV?.BASE_URL || "";
      }
      return "";
    }
  }, [chatAPI]);

  const configAPI = `${backendOrigin}/api/chat/config`;


Review comment (suggestion):
```suggestion
// Patching to use value from window.ENV.BASE_URL
"use client";
```

Code after:
// Patching to use value from window.ENV.BASE_URL  
"use client";

import { useEffect, useMemo, useState } from "react";

export interface ChatConfig {
  backend?: string;
  starterQuestions?: string[];
}

export function useClientConfig(): ChatConfig {
  const chatAPI = process.env.NEXT_PUBLIC_CHAT_API;
  const [config, setConfig] = useState<ChatConfig>();

  const backendOrigin = useMemo(() => {
    if (chatAPI) {
      return chatAPI || "";
    } else {
      if (typeof window !== "undefined") {
        // Use BASE_URL from window.ENV
        return (window as any).ENV?.BASE_URL || "";
      }
      return "";
    }
  }, [chatAPI]);

  const configAPI = `${backendOrigin}/api/chat/config`;
Repository: ragapp/ragapp
File: entrypoint.sh (Shell)

Code before:
#!/bin/bash

if [[ -n "$BASE_URL" ]]; then
    # Run sed command for all files in the static directory
    # Replace this: url(/_next with this: url($BASE_URL/_next (Mostly for link in CSS files)
    find static -type f -name "*.*" -exec sed -i 's|url(/_next|url('"$BASE_URL"'/_next|g' {} +
    # Replace this: "static/ with this: "$BASE_URL/static/
    # TODO: This is not working for some reason
    find static -type f -name "*.*" -exec sed -i 's|"static/|"'$BASE_URL'/static/|g' {} +

    # Logo and avatar
    find static -type f -name "*.*" -exec sed -i 's|"/logo.png|"'$BASE_URL'/logo.png|g' {} +
    find static -type f -name "*.*" -exec sed -i 's|"/llama.png|"'$BASE_URL'/llama.png|g' {} +

    find static -type f -name "*.*" -exec sed -i 's|"/_next/static|"'$BASE_URL'/_next/static|g' {} +
    find static/admin -type f -name "*.*" -exec sed -i 's|"/admin/_next/static|"'$BASE_URL'/admin/_next/static|g' {} +

    # Run sed command to generate window.ENV.API_URL to index.html
    # Add it at the start of the head tag
    sed -i '/<\/head>/i <script>window.ENV = {"BASE_URL":"'$BASE_URL'"};</script>' static/*.html static/**/*.html

    echo "Updated static files successfully!"
fi

echo "Running application..."

exec "$@"

Review comment (refactor):
can't we simplify this sed expression, if we use as asset dir in NextJs config a distinct prefix like: `https://static-assets.ragapp.org/` and replace using sed for this string?

Code after:
#!/bin/bash

if [[ -n "$BASE_URL" ]]; then
    # Rewrite asset prefix to BASE_URL
    find static -type f -name "*.*" -exec sed -i 's|https://static-assets.ragapp.org|'$BASE_URL'|g' {} +

    # Add BASE_URL to logo and avatar
    # TODO: find a way that is independent of assets used
    find static -type f -name "*.*" -exec sed -i 's|"/logo.png|"'$BASE_URL'/logo.png|g' {} +
    find static -type f -name "*.*" -exec sed -i 's|"/llama.png|"'$BASE_URL'/llama.png|g' {} +

    # Replace static files of admin app
    find static/admin -type f -name "*.*" -exec sed -i 's|"/admin/_next/static|"'$BASE_URL'/admin/_next/static|g' {} +

    # Run sed command to generate window.ENV.API_URL to index.html
    # Add it at the start of the head tag
    sed -i '/<\/head>/i <script>window.ENV = {"BASE_URL":"'$BASE_URL'"};</script>' static/*.html static/**/*.html

    echo "Updated static files successfully!"
else
    # Remove asset prefix
    find static -type f -name "*.*" -exec sed -i 's|https://static-assets.ragapp.org||g' {} +
    echo "Updated static files successfully!"
fi

echo "Running application..."

exec "$@"
Repository: ragapp/ragapp
File: entrypoint.sh (Shell)

Code before:
#!/bin/bash

if [[ -n "$BASE_URL" ]]; then
    # Rewrite asset prefix to BASE_URL
    find static -type f -name "*.*" -exec sed -i 's|https://static-assets.ragapp.org|'$BASE_URL'|g' {} +

    # Replacment for logo and avatar
    find static -type f -name "*.*" -exec sed -i 's|"/logo.png|"'$BASE_URL'/logo.png|g' {} +
    find static -type f -name "*.*" -exec sed -i 's|"/llama.png|"'$BASE_URL'/llama.png|g' {} +

    # Replacment for admin's static files
    find static/admin -type f -name "*.*" -exec sed -i 's|"/admin/_next/static|"'$BASE_URL'/admin/_next/static|g' {} +

    # Run sed command to generate window.ENV.API_URL to index.html
    # Add it at the start of the head tag
    sed -i '/<\/head>/i <script>window.ENV = {"BASE_URL":"'$BASE_URL'"};</script>' static/*.html static/**/*.html

    echo "Updated static files successfully!"
else
    # Remove asset prefix
    find static -type f -name "*.*" -exec sed -i 's|https://static-assets.ragapp.org||g' {} +
    echo "Updated static files successfully!"
fi

echo "Running application..."

exec "$@"

Review comment (suggestion):
```suggestion
    # Replace static files of admin app
```

Code after:
#!/bin/bash

if [[ -n "$BASE_URL" ]]; then
    # Rewrite asset prefix to BASE_URL
    find static -type f -name "*.*" -exec sed -i 's|https://static-assets.ragapp.org|'$BASE_URL'|g' {} +

    # Add BASE_URL to logo and avatar
    # TODO: find a way that is independent of assets used
    find static -type f -name "*.*" -exec sed -i 's|"/logo.png|"'$BASE_URL'/logo.png|g' {} +
    find static -type f -name "*.*" -exec sed -i 's|"/llama.png|"'$BASE_URL'/llama.png|g' {} +

    # Replace static files of admin app
    find static/admin -type f -name "*.*" -exec sed -i 's|"/admin/_next/static|"'$BASE_URL'/admin/_next/static|g' {} +

    # Run sed command to generate window.ENV.API_URL to index.html
    # Add it at the start of the head tag
    sed -i '/<\/head>/i <script>window.ENV = {"BASE_URL":"'$BASE_URL'"};</script>' static/*.html static/**/*.html

    echo "Updated static files successfully!"
else
    # Remove asset prefix
    find static -type f -name "*.*" -exec sed -i 's|https://static-assets.ragapp.org||g' {} +
    echo "Updated static files successfully!"
fi

echo "Running application..."

exec "$@"
Repository: ragapp/ragapp
File: entrypoint.sh (Shell)

Code before:
#!/bin/bash

if [[ -n "$BASE_URL" ]]; then
    # Rewrite asset prefix to BASE_URL
    find static -type f -name "*.*" -exec sed -i 's|https://static-assets.ragapp.org|'$BASE_URL'|g' {} +

    # Replacment for logo and avatar
    find static -type f -name "*.*" -exec sed -i 's|"/logo.png|"'$BASE_URL'/logo.png|g' {} +
    find static -type f -name "*.*" -exec sed -i 's|"/llama.png|"'$BASE_URL'/llama.png|g' {} +

    # Replace static files of admin app
    find static/admin -type f -name "*.*" -exec sed -i 's|"/admin/_next/static|"'$BASE_URL'/admin/_next/static|g' {} +

    # Run sed command to generate window.ENV.API_URL to index.html
    # Add it at the start of the head tag
    sed -i '/<\/head>/i <script>window.ENV = {"BASE_URL":"'$BASE_URL'"};</script>' static/*.html static/**/*.html

    echo "Updated static files successfully!"
else
    # Remove asset prefix
    find static -type f -name "*.*" -exec sed -i 's|https://static-assets.ragapp.org||g' {} +
    echo "Updated static files successfully!"
fi

echo "Running application..."

exec "$@"

Review comment (suggestion):
```suggestion
    # Add BASE_URL to logo and avatar
    # TODO: find a way that is independent of assets used
```

Code after:
#!/bin/bash

if [[ -n "$BASE_URL" ]]; then
    # Rewrite asset prefix to BASE_URL
    find static -type f -name "*.*" -exec sed -i 's|https://static-assets.ragapp.org|'$BASE_URL'|g' {} +

    # Add BASE_URL to logo and avatar
    # TODO: find a way that is independent of assets used
    find static -type f -name "*.*" -exec sed -i 's|"/logo.png|"'$BASE_URL'/logo.png|g' {} +
    find static -type f -name "*.*" -exec sed -i 's|"/llama.png|"'$BASE_URL'/llama.png|g' {} +

    # Replace static files of admin app
    find static/admin -type f -name "*.*" -exec sed -i 's|"/admin/_next/static|"'$BASE_URL'/admin/_next/static|g' {} +

    # Run sed command to generate window.ENV.API_URL to index.html
    # Add it at the start of the head tag
    sed -i '/<\/head>/i <script>window.ENV = {"BASE_URL":"'$BASE_URL'"};</script>' static/*.html static/**/*.html

    echo "Updated static files successfully!"
else
    # Remove asset prefix
    find static -type f -name "*.*" -exec sed -i 's|https://static-assets.ragapp.org||g' {} +
    echo "Updated static files successfully!"
fi

echo "Running application..."

exec "$@"
Repository: ragapp/ragapp
File: src/ragapp/admin-ui/sections/config/providers/ollama.tsx (TypeScript)

Code before:
        <>
          <ModelForm
            form={form}
            title="LLM Model"
            description="Select an LLM model to use for language generation."
            defaultValue={defaultValues.model}
            supportedModels={llmModels}
          />
          <FormMessage>
            One of the embedding models{" "}
            <i>{embeddingModels.join(", ")}</i> is required. Please pull an Ollama embedding model (e.g., <i>nomic-embed-text</i>) from &nbsp;
            <a
              href="https://ollama.com/library"
              target="_blank"
              rel="noreferrer"
            >
              https://ollama.com/library
            </a>
          </FormMessage>
        </>
      ) : (
        <>
          <ModelForm
            form={form}
            title="LLM Model"
            description="Select an LLM model to use for language generation."
            defaultValue={defaultValues.model}
            supportedModels={llmModels}
          />
          <ModelForm
            form={form}
            name="embedding_model"
            title="Embedding Model"
            description="Select an embedding model for text embeddings."
            defaultValue={defaultValues.embedding_model}
            supportedModels={embeddingModelsAvailable}
          />
        </>
      )}
    </>
  );
};


Review comment (suggestion):
ah i see, there are defaults, i think then better remove it here:
```suggestion
```

Code after:
            defaultValue={defaultValues.model}
            supportedModels={llmModels}
          />
          <FormMessage>
            One of the embedding models <i>{embeddingModels.join(", ")}</i> is
            required. Please pull an Ollama embedding model (e.g.,{" "}
            <i>nomic-embed-text</i>) from &nbsp;
            <a
              href="https://ollama.com/library"
              target="_blank"
              rel="noreferrer"
            >
              https://ollama.com/library
            </a>
          </FormMessage>
        </>
      ) : (
        <>
          <ModelForm
            form={form}
            defaultValue={defaultValues.model}
            supportedModels={llmModels}
          />
          <ModelForm
            form={form}
            name="embedding_model"
            title="Embedding Model"
            description="Select an embedding model for text embeddings."
            defaultValue={defaultValues.embedding_model}
            supportedModels={embeddingModelsAvailable}
          />
        </>
      )}
    </>
  );
};
Repository: ragapp/ragapp
File: src/ragapp/backend/middlewares/rate_limit.py (Python)

Code before:
import os
import time

from fastapi import HTTPException, Request, status
from fastapi.responses import Response

from backend.models.jwt import JWT
from backend.models.user_info import UserInfo
from backend.services.user_chat_service import UserChatService

CHAT_REQUEST_LIMIT_THRESHOLD = int(os.getenv("CHAT_REQUEST_LIMIT_THRESHOLD", 0))
CHAT_REQUEST_LIMIT_ENABLED = CHAT_REQUEST_LIMIT_THRESHOLD > 0


async def request_limit_middleware(request: Request) -> Response:
    if CHAT_REQUEST_LIMIT_ENABLED:
        jwt = JWT(request.cookies)
        user = UserInfo.from_jwt_data(jwt.data)
        time_frame = _get_time_frame()
        # Use user name as the key for rate limiting
        request_count = UserChatService.get_user_chat_request_count(
            user.username, time_frame
        )
        if not user.is_admin and request_count >= CHAT_REQUEST_LIMIT_THRESHOLD:
            raise HTTPException(
                status_code=status.HTTP_429_TOO_MANY_REQUESTS,
                detail=f"You have exceeded {CHAT_REQUEST_LIMIT_THRESHOLD} chat requests. Please try again later.",
            )

        UserChatService.update_user_chat_request_count(
            user.username, time_frame, request_count + 1
        )


def _get_time_frame():
    """
    Provide the time frame for the request count
    """
    # Use current date as the time frame
    return time.strftime("%Y-%m-%d")


Review comment (suggestion):
probably better to pass the user model now 
```suggestion
        request_count = UserChatService.get_user_chat_request_count(
            user, time_frame
```

Code after:
import os
import time

from fastapi import HTTPException, Request, status
from fastapi.responses import Response

from backend.models.user_info import UserInfo
from backend.services.user_chat_service import UserChatService

CHAT_REQUEST_LIMIT_THRESHOLD = int(os.getenv("CHAT_REQUEST_LIMIT_THRESHOLD", 0))
CHAT_REQUEST_LIMIT_ENABLED = CHAT_REQUEST_LIMIT_THRESHOLD > 0


async def request_limit_middleware(request: Request) -> Response:
    if CHAT_REQUEST_LIMIT_ENABLED:
        user = UserInfo.from_request(request)
        time_frame = _get_time_frame()
        request_count = UserChatService.get_user_chat_request_count(user, time_frame)
        if not user.is_admin and request_count >= CHAT_REQUEST_LIMIT_THRESHOLD:
            raise HTTPException(
                status_code=status.HTTP_429_TOO_MANY_REQUESTS,
                detail=f"You have exceeded {CHAT_REQUEST_LIMIT_THRESHOLD} chat requests. Please try again later.",
            )

        UserChatService.update_user_chat_request_count(
            user, time_frame, request_count + 1
        )


def _get_time_frame():
    """
    Provide the time frame for the request count
    """
    # Use current date as the time frame
    return time.strftime("%Y-%m-%d")
Repository: ragapp/ragapp
File: src/ragapp/backend/models/user_info.py (Python)

Code before:
from pydantic import BaseModel, computed_field

from .jwt import JWT

ADMIN_ROLE = "admin-manager"
JWT_USER_NAME_CLAIM = "preferred_username"  # The claim in the JWT token that stores the user ID or user name
JWT_USER_ROLES_CLAIM = (
    "X-Forwarded-Roles"  # The claim in the JWT token that stores the user roles
)


class UserInfo(BaseModel):
    user_name: str
    roles: list[str] = []

    @computed_field
    def is_admin(self) -> bool:
        return ADMIN_ROLE in self.roles

    @classmethod
    def from_jwt_data(cls, jwt_data: dict) -> "UserInfo":
        user_name = jwt_data.get(JWT_USER_NAME_CLAIM)
        roles = jwt_data.get(JWT_USER_ROLES_CLAIM, [])
        return cls(user_name=user_name, roles=roles)

    @classmethod
    def from_request(cls, request) -> "UserInfo":
        jwt = JWT(request.cookies)
        return cls.from_jwt_data(jwt.data)


Review comment (suggestion):
how about using `user_id` and using here
```suggestion
JWT_USER_ID_CLAIM = os.getenv("JWT_USER_ID_CLAIM", "preferred_username")  # The claim in the JWT token that stores the user ID (defaults to `preferred_username` from Keycloak)
```

Code after:
import os

from pydantic import BaseModel, computed_field

from .jwt import JWT

ADMIN_ROLE = "admin-manager"
JWT_USER_ID_CLAIM = os.getenv(
    "JWT_USER_ID_CLAIM", "preferred_username"
)  # The claim in the JWT token that stores the user ID (defaults to `preferred_username` from Keycloak)
JWT_USER_ROLES_CLAIM = (
    "X-Forwarded-Roles"  # The claim in the JWT token that stores the user roles
)


class UserInfo(BaseModel):
    user_id: str
    roles: list[str] = []

    @computed_field
    def is_admin(self) -> bool:
        return ADMIN_ROLE in self.roles

    @classmethod
    def from_jwt_data(cls, jwt_data: dict) -> "UserInfo":
        user_id = jwt_data.get(JWT_USER_ID_CLAIM)
        roles = jwt_data.get(JWT_USER_ROLES_CLAIM, [])
        return cls(user_id=user_id, roles=roles)

    @classmethod
    def from_request(cls, request) -> "UserInfo":
Repository: ragapp/ragapp
File: src/ragapp/patch/backend/app/engine/service.py (Python)

Code before:
import logging
import os
import time
import typing
from io import BytesIO
from typing import Any, Dict, List, Optional, Set, Tuple, Union

import requests
from app.api.routers.models import SourceNodes
from app.engine.index import get_client
from fastapi import BackgroundTasks
from llama_cloud import ManagedIngestionStatus, PipelineFileCreateCustomMetadataValue
from llama_index.core.schema import NodeWithScore
from pydantic import BaseModel

logger = logging.getLogger("uvicorn")


class LlamaCloudFile(BaseModel):
    file_name: str
    pipeline_id: str

    def __eq__(self, other):
        if not isinstance(other, LlamaCloudFile):
            return NotImplemented
        return (
            self.file_name == other.file_name and self.pipeline_id == other.pipeline_id

Review comment (refactor):
i see two options:

1. move this to ragapp/backend and reimport it here in the patch (then we would own the file service)
2. reference the exact commit id used from CL like:
```suggestion
# same as https://github.com/run-llama/create-llama/blob/{commit_id}/templates/components/vectordbs/python/llamacloud/service.py
import logging
```

Code after:
# same as https://github.com/run-llama/create-llama/blob/81ef7f0f9362f34339669bdc81f8ab885701f73b/templates/components/vectordbs/python/llamacloud/service.py
import logging
import os
import time
import typing
from io import BytesIO
from typing import Any, Dict, List, Optional, Set, Tuple, Union

import requests
from app.api.routers.models import SourceNodes
from app.engine.index import get_client
from fastapi import BackgroundTasks
from llama_cloud import ManagedIngestionStatus, PipelineFileCreateCustomMetadataValue
from llama_index.core.schema import NodeWithScore
from pydantic import BaseModel

logger = logging.getLogger("uvicorn")


class LlamaCloudFile(BaseModel):
    file_name: str
    pipeline_id: str

    def __eq__(self, other):
        if not isinstance(other, LlamaCloudFile):
            return NotImplemented
        return (
Repository: reduxjs/redux
File: test/helpers/reducers.js (JavaScript)

Code before:
import { ADD_TODO, BAD_TODO, BAD_TODO_ERROR } from './actionTypes';


function id(state = []) {
  return state.reduce((result, item) => (
    item.id > result ? item.id : result
  ), 0) + 1;
}

export function todos(state = [], action) {
  switch (action.type) {
  case ADD_TODO:
    return [...state, {
      id: id(state),
      text: action.text
    }];
  default:
    return state;
  }
}

export function todosReverse(state = [], action) {
  switch (action.type) {
  case ADD_TODO:
    return [{
      id: id(state),
      text: action.text
    }, ...state];
  default:
    return state;
  }
}

export function antipatternTodo(state = [], action) {
  switch (action.type) {
  case BAD_TODO:
    action.boundDispatchFn();
    return state;
  default:
    return state;
  }
}

export function antipatternTodoWithError(state = [], action) {
  switch (action.type) {
  case BAD_TODO_ERROR:
    throw new Error();
  default:
    return state;
  }
}


Review comment (suggestion):
I guess this one is just `throwingTodo`? Not really fair to call it `antipattern*` because it doesn't `dispatch` itself.


Code after:
import { ADD_TODO, DISPATCH_IN_MIDDLE, THROW_ERROR } from './actionTypes';


function id(state = []) {
  return state.reduce((result, item) => (
    item.id > result ? item.id : result
  ), 0) + 1;
}

export function todos(state = [], action) {
  switch (action.type) {
  case ADD_TODO:
    return [...state, {
      id: id(state),
      text: action.text
    }];
  default:
    return state;
  }
}

export function todosReverse(state = [], action) {
  switch (action.type) {
  case ADD_TODO:
    return [{
      id: id(state),
      text: action.text
    }, ...state];
  default:
    return state;
  }
}

export function dispatchInTheMiddleOfReducer(state = [], action) {
  switch (action.type) {
  case DISPATCH_IN_MIDDLE:
    action.boundDispatchFn();
    return state;
  default:
    return state;
  }
}

export function errorThrowingReducer(state = [], action) {
  switch (action.type) {
  case THROW_ERROR:
    throw new Error();
  default:
    return state;
  }
}