ronantakizawa/codereview-bench huggingface dataset:
Repository: All-Hands-AI/OpenHands
File: frontend/vitest.setup.ts (TypeScript)

Code before:
import { afterAll, afterEach, beforeAll, vi } from "vitest";
import { cleanup } from "@testing-library/react";
import { server } from "#/mocks/node";
import "@testing-library/jest-dom/vitest";

HTMLCanvasElement.prototype.getContext = vi.fn();
HTMLElement.prototype.scrollTo = vi.fn();

// Mock the i18n provider
vi.mock("react-i18next", async (importOriginal) => ({
  ...(await importOriginal<typeof import("react-i18next")>()),
  useTranslation: () => ({
    t: (key: string) => key,
    i18n: {
      language: "en",
      exists: () => false,
    },
  }),
}));

vi.mock("#/hooks/use-is-on-tos-page", () => ({
  useIsOnTosPage: () => false,
}));

// Mock requests during tests
beforeAll(() => server.listen({ onUnhandledRequest: "bypass" }));
afterEach(() => {
  server.resetHandlers();
  // Cleanup the document body after each test
  cleanup();
});
afterAll(() => server.close());


Review comment (suggestion):
I think it would be better to:

```ts
beforeAll(() => {
  vi.stubGlobal(“ResizeObserver”, MockResizeObserver);
});

afterAll(() => {
  vi.unstubAllGlobals();
});
```

Source: https://vitest.dev/api/vi.html#vi-stubglobal

Code after:
import { afterAll, afterEach, beforeAll, vi } from "vitest";
import { cleanup } from "@testing-library/react";
import { server } from "#/mocks/node";
import "@testing-library/jest-dom/vitest";

HTMLCanvasElement.prototype.getContext = vi.fn();
HTMLElement.prototype.scrollTo = vi.fn();

// Mock ResizeObserver for test environment
class MockResizeObserver {
  observe = vi.fn();
  unobserve = vi.fn();
  disconnect = vi.fn();
}

global.ResizeObserver = MockResizeObserver;

// Mock the i18n provider
vi.mock("react-i18next", async (importOriginal) => ({
  ...(await importOriginal<typeof import("react-i18next")>()),
  useTranslation: () => ({
    t: (key: string) => key,
    i18n: {
      language: "en",
      exists: () => false,
    },
  }),
}));

vi.mock("#/hooks/use-is-on-tos-page", () => ({
  useIsOnTosPage: () => false,
}));

// Mock requests during tests
beforeAll(() => server.listen({ onUnhandledRequest: "bypass" }));
afterEach(() => {
  server.resetHandlers();
  // Cleanup the document body after each test
  cleanup();
});
afterAll(() => server.close());
Repository: All-Hands-AI/OpenHands
File: frontend/src/components/features/maintenance/maintenance-banner.tsx (TypeScript)

Code before:
import { useTranslation } from "react-i18next";
import { useMemo, useState } from "react";
import { FaTriangleExclamation } from "react-icons/fa6";
import CloseIcon from "#/icons/close.svg?react";
import { cn } from "#/utils/utils";

interface MaintenanceBannerProps {
  startTime: string;
}

export function MaintenanceBanner({ startTime }: MaintenanceBannerProps) {
  const [isBannerShown, setIsBannerShown] = useState<boolean>(true);

  const { t } = useTranslation();

  // Convert EST timestamp to user's local timezone
  const formatMaintenanceTime = (estTimeString: string): string => {
    try {
      // Parse the EST timestamp
      // If the string doesn't include timezone info, assume it's EST
      let dateToFormat: Date;

      if (
        estTimeString.includes("T") &&
        (estTimeString.includes("-05:00") ||
          estTimeString.includes("-04:00") ||
          estTimeString.includes("EST") ||
          estTimeString.includes("EDT"))
      ) {
        // Already has timezone info
        dateToFormat = new Date(estTimeString);
      } else {
        // Assume EST and convert to UTC for proper parsing
        // EST is UTC-5, EDT is UTC-4, but we'll assume EST for simplicity
        const estDate = new Date(estTimeString);
        if (Number.isNaN(estDate.getTime())) {
          throw new Error("Invalid date");

Review comment (suggestion):
Doesn’t this imply that we will show the banner every time we reload the page? We should persist the user’s decision here for now. In the future, I want to consider storing the user’s preference in a userPref cookie or something

Code after:
import { useTranslation } from "react-i18next";
import { useMemo } from "react";
import { useLocalStorage } from "@uidotdev/usehooks";
import { FaTriangleExclamation } from "react-icons/fa6";
import CloseIcon from "#/icons/close.svg?react";
import { cn } from "#/utils/utils";

interface MaintenanceBannerProps {
  startTime: string;
}

export function MaintenanceBanner({ startTime }: MaintenanceBannerProps) {
  const { t } = useTranslation();
  const [dismissedAt, setDismissedAt] = useLocalStorage<string | null>(
    "maintenance_banner_dismissed_at",
    null,
  );

  // Convert EST timestamp to user's local timezone
  const formatMaintenanceTime = (estTimeString: string): string => {
    try {
      // Parse the EST timestamp
      // If the string doesn't include timezone info, assume it's EST
      let dateToFormat: Date;

      if (
        estTimeString.includes("T") &&
        (estTimeString.includes("-05:00") ||
          estTimeString.includes("-04:00") ||
          estTimeString.includes("EST") ||
          estTimeString.includes("EDT"))
      ) {
        // Already has timezone info
        dateToFormat = new Date(estTimeString);
      } else {
        // Assume EST and convert to UTC for proper parsing
        // EST is UTC-5, EDT is UTC-4, but we'll assume EST for simplicity
Repository: All-Hands-AI/OpenHands
File: tests/unit/integrations/protocols/test_http_client.py (Python)

Code before:
"""Unit tests for HTTPClient protocol."""

from unittest.mock import AsyncMock, Mock

import httpx
import pytest
from pydantic import SecretStr

from openhands.integrations.protocols.http_client import HTTPClient
from openhands.integrations.service_types import (
    AuthenticationError,
    RateLimitError,
    RequestMethod,
    ResourceNotFoundError,
    UnknownException,
    User,
)


class TestableHTTPClient:
    """Testable implementation of HTTPClient protocol for unit testing."""

    def __init__(self, provider_name: str = 'test-provider'):
        self.token = SecretStr('test-token')
        self.refresh = False
        self.external_auth_id = None
        self.external_auth_token = None
        self.external_token_manager = False
        self.base_domain = None
        self._provider_name = provider_name

    @property
    def provider(self) -> str:
        return self._provider_name

    async def get_latest_token(self) -> SecretStr | None:
        return self.token

    async def _get_headers(self) -> dict[str, str]:
        return {'Authorization': f'Bearer {self.token.get_secret_value()}'}

    async def _make_request(
        self,
        url: str,
        params: dict | None = None,

Review comment (refactor):
@openhands please fix the mocking for this; make sure `TestableHTTPClient` is implementing the `HTTPClient` protocol so that we aren't duplicating so much code

also remove the abstract method require `get_user` from the `HTTPClient` protocol; the base mixins should remain unchanged though they should still implement `get_user`

Code after:
"""Unit tests for HTTPClient abstract base class (ABC)."""

from typing import Any
from unittest.mock import AsyncMock, Mock

import httpx
import pytest
from pydantic import SecretStr

from openhands.integrations.protocols.http_client import HTTPClient
from openhands.integrations.service_types import (
    AuthenticationError,
    RateLimitError,
    RequestMethod,
    ResourceNotFoundError,
    UnknownException,
)


class TestableHTTPClient(HTTPClient):
    """Testable concrete implementation of HTTPClient for unit testing."""

    def __init__(self, provider_name: str = 'test-provider'):
        self.token = SecretStr('test-token')
        self.refresh = False
        self.external_auth_id = None
        self.external_auth_token = None
        self.external_token_manager = False
        self.base_domain = None
        self._provider_name = provider_name

    @property
    def provider(self) -> str:
        return self._provider_name

    @provider.setter
    def provider(self, value: str) -> None:
        self._provider_name = value

    async def get_latest_token(self) -> SecretStr | None:
        return self.token

    async def _get_headers(self) -> dict[str, Any]:
        return {'Authorization': f'Bearer {self.token.get_secret_value()}'}
Repository: All-Hands-AI/OpenHands
File: tests/unit/integrations/bitbucket/test_bitbucket_repos.py (Python)

Code before:
"""Tests for Bitbucket repository service URL parsing."""

from unittest.mock import patch

import pytest
from pydantic import SecretStr

from openhands.integrations.bitbucket.bitbucket_service import BitBucketService
from openhands.integrations.service_types import OwnerType, Repository
from openhands.integrations.service_types import ProviderType as ServiceProviderType


@pytest.fixture
def bitbucket_service():
    """Create a BitBucketService instance for testing."""
    return BitBucketService(token=SecretStr('test-token'))


@pytest.mark.asyncio
async def test_search_repositories_public_url_parsing_standard_url(bitbucket_service):
    """Test URL parsing with standard Bitbucket URL."""
    # Mock the get_repository_details_from_repo_name method
    mock_repo = Repository(
        id='1',
        full_name='workspace/repo',
        name='repo',

Review comment (refactor):
@openhands lets simplify this test file

1. let's only add tests for `search_repository` for now where we're correctly parsing the URL
2. remove all the other tests from here
3. check the parameters during the API call and verify that the right workspace repo combination was included in the call

Code after:
"""Tests for Bitbucket repository service URL parsing."""

from unittest.mock import patch

import pytest
from pydantic import SecretStr

from openhands.integrations.bitbucket.bitbucket_service import BitBucketService
from openhands.integrations.service_types import OwnerType, Repository
from openhands.integrations.service_types import ProviderType as ServiceProviderType


@pytest.fixture
def bitbucket_service():
    """Create a BitBucketService instance for testing."""
    return BitBucketService(token=SecretStr('test-token'))


@pytest.mark.asyncio
async def test_search_repositories_url_parsing_standard_url(bitbucket_service):
    """Test URL parsing with standard Bitbucket URL and verify correct workspace/repo extraction."""
    mock_repo = Repository(
        id='1',
        full_name='workspace/repo',
        name='repo',
        owner=OwnerType.USER,
Repository: All-Hands-AI/OpenHands
File: frontend/src/ui/card.tsx (TypeScript)

Code before:
      gap: "default",
      minHeight: "default",
    },
  },
);

interface CardProps extends VariantProps<typeof cardVariants> {
  children: ReactNode;
  className?: string;
  testId?: string;
}

export function Card({
  children,
  className = "",
  testId,
  gap,
  minHeight,
}: CardProps) {
  return (
    <section
      data-testid={testId}
      className={cn(cardVariants({ gap, minHeight }), className)}
    >
      {children}
    </section>
  );
}


Review comment (suggestion):
```suggestion
    <div
      data-testid={testId}
      className={cn(cardVariants({ gap, minHeight }), className)}
    >
      {children}
    </div>
```

Prefer `<section>` only when the card represents a document section with its own heading and role in page structure

Code after:
      gap: "default",
      minHeight: "default",
    },
  },
);

interface CardProps extends VariantProps<typeof cardVariants> {
  children: ReactNode;
  className?: string;
  testId?: string;
}

export function Card({
  children,
  className = "",
  testId,
  gap,
  minHeight,
}: CardProps) {
  return (
    <div
      data-testid={testId}
      className={cn(cardVariants({ gap, minHeight }), className)}
    >
      {children}
    </div>
  );
}
Repository: All-Hands-AI/OpenHands
File: frontend/src/hooks/use-auto-resize.ts (TypeScript)

Code before:
import { useCallback, useEffect, RefObject, useRef } from "react";
import { IMessageToSend } from "#/state/conversation-store";
import { useDragResize } from "./use-drag-resize";

// Constants
const DEFAULT_MIN_HEIGHT = 20;
const DEFAULT_MAX_HEIGHT = 120;
const HEIGHT_INCREMENT = 20;
const MANUAL_OVERSIZE_THRESHOLD = 50;

const EPS = 1.5; // px tolerance for "near min"

const getStyleHeightPx = (el: HTMLElement, fallback: number) => {
  const h = parseFloat(el.style.height || "");
  return Number.isFinite(h) ? h : fallback;
};

const setStyleHeightPx = (el: HTMLElement, h: number) => {
  // eslint-disable-next-line no-param-reassign
  el.style.height = `${h}px`;
};

// Manual height tracking utilities
const useManualHeight = () => {
  const hasUserResizedRef = useRef(false);
  const manualHeightRef = useRef<number | null>(null);
  return { hasUserResizedRef, manualHeightRef };
};

// -------------------------------

interface UseAutoResizeOptions {
  minHeight?: number;
  maxHeight?: number;
  enableManualResize?: boolean;
  onGripDragStart?: () => void;
  onGripDragEnd?: () => void;
  onHeightChange?: (height: number) => void; // New callback for height changes
  value?: IMessageToSend;
}

interface UseAutoResizeReturn {
  smartResize: () => void;
  handleGripMouseDown: (e: React.MouseEvent) => void;
  handleGripTouchStart: (e: React.TouchEvent) => void;

Review comment (suggestion):
**suggestion:** It appears that this ESLint rule can be resolved through proper code adjustments rather than being disabled.

```ts
el.style.setProperty('height', `${elementHeight}px`);
```

Code after:
import { useCallback, useEffect, RefObject, useRef } from "react";
import { IMessageToSend } from "#/state/conversation-store";
import { EPS } from "#/utils/constants";
import { getStyleHeightPx, setStyleHeightPx } from "#/utils/utils";
import { useDragResize } from "./use-drag-resize";

// Constants
const DEFAULT_MIN_HEIGHT = 20;
const DEFAULT_MAX_HEIGHT = 120;
const HEIGHT_INCREMENT = 20;
const MANUAL_OVERSIZE_THRESHOLD = 50;

// Manual height tracking utilities
const useManualHeight = () => {
  const hasUserResizedRef = useRef(false);
  const manualHeightRef = useRef<number | null>(null);
  return { hasUserResizedRef, manualHeightRef };
};

interface UseAutoResizeOptions {
  minHeight?: number;
  maxHeight?: number;
  enableManualResize?: boolean;
  onGripDragStart?: () => void;
  onGripDragEnd?: () => void;
  onHeightChange?: (height: number) => void; // New callback for height changes
  value?: IMessageToSend;
}

interface UseAutoResizeReturn {
  smartResize: () => void;
  handleGripMouseDown: (e: React.MouseEvent) => void;
  handleGripTouchStart: (e: React.TouchEvent) => void;
  increaseHeightForEmptyContent: () => void;
  resetManualResize: () => void;
}

// Height management utilities
interface HeightConstraints {
  minHeight: number;
  maxHeight: number;
}

interface HeightMeasurements {
  currentHeight: number;
Repository: All-Hands-AI/OpenHands
File: openhands-cli/openhands_cli/agent_chat.py (Python)

Code before:
                display_welcome(session_id)
                continue
            elif command == "/resume":
                if not (
                    conversation.state.agent_status == AgentExecutionStatus.PAUSED
                    or conversation.state.agent_status
                    == AgentExecutionStatus.WAITING_FOR_CONFIRMATION
                ):
                    print_formatted_text(
                        HTML("<red>No paused conversation to resume...</red>")
                    )

                    continue

                # Resume without new message
                message = None

            runner.process_message(message)

            print()  # Add spacing

        except KeyboardInterrupt:
            exit_confirmation = exit_session_confirmation()
            if exit_confirmation == UserConfirmation.ACCEPT:
                print_formatted_text(HTML("\n<yellow>Goodbye! 👋</yellow>"))
            break


    # Clean up terminal state
    _restore_tty()



Review comment (suggestion):
I think we should do this?
```suggestion
                break
```

Code after:
                display_welcome(session_id)
                continue
            elif command == "/resume":
                if not (
                    conversation.state.agent_status == AgentExecutionStatus.PAUSED
                    or conversation.state.agent_status
                    == AgentExecutionStatus.WAITING_FOR_CONFIRMATION
                ):
                    print_formatted_text(
                        HTML("<red>No paused conversation to resume...</red>")
                    )

                    continue

                # Resume without new message
                message = None

            runner.process_message(message)

            print()  # Add spacing

        except KeyboardInterrupt:
            exit_confirmation = exit_session_confirmation()
            if exit_confirmation == UserConfirmation.ACCEPT:
                print_formatted_text(HTML("\n<yellow>Goodbye! 👋</yellow>"))
                break


    # Clean up terminal state
    _restore_tty()
Repository: All-Hands-AI/OpenHands
File: openhands-cli/openhands_cli/simple_main.py (Python)

Code before:
#!/usr/bin/env python3
"""
Simple main entry point for OpenHands CLI.
This is a simplified version that demonstrates the TUI functionality.
"""

import logging
import os

debug_env = os.getenv('DEBUG', 'false').lower()
if debug_env != '1' and debug_env != 'true':
    logging.disable(logging.CRITICAL)

from prompt_toolkit import print_formatted_text
from prompt_toolkit.formatted_text import HTML
from openhands_cli.agent_chat import run_cli_entry



def main() -> None:
    """Main entry point for the OpenHands CLI.

    Raises:
        ImportError: If agent chat dependencies are missing
        Exception: On other error conditions
    """

    try:
        # Start agent chat directly by default
        run_cli_entry()

    except ImportError as e:
        print_formatted_text(
            HTML(f"<red>Error: Agent chat requires additional dependencies: {e}</red>")
        )
        print_formatted_text(
            HTML("<yellow>Please ensure the agent SDK is properly installed.</yellow>")
        )
        raise

Review comment (suggestion):
Shall we still show warnings and error? We should have VERY small set of them in agent SDK. These could be useful to warn user about the configuration error

Code after:
#!/usr/bin/env python3
"""
Simple main entry point for OpenHands CLI.
This is a simplified version that demonstrates the TUI functionality.
"""

import logging
import os

debug_env = os.getenv('DEBUG', 'false').lower()
if debug_env != '1' and debug_env != 'true':
    logging.disable(logging.WARNING)

from prompt_toolkit import print_formatted_text
from prompt_toolkit.formatted_text import HTML
from openhands_cli.agent_chat import run_cli_entry



def main() -> None:
    """Main entry point for the OpenHands CLI.

    Raises:
        ImportError: If agent chat dependencies are missing
        Exception: On other error conditions
    """

    try:
        # Start agent chat directly by default
        run_cli_entry()

    except ImportError as e:
        print_formatted_text(
            HTML(f"<red>Error: Agent chat requires additional dependencies: {e}</red>")
        )
        print_formatted_text(
            HTML("<yellow>Please ensure the agent SDK is properly installed.</yellow>")
        )
        raise
Repository: All-Hands-AI/OpenHands
File: frontend/src/stores/error-message-store.ts (TypeScript)

Code before:

interface ErrorMessageState {
  errorMessage: string | null;
}

interface ErrorMessageActions {
  setErrorMessage: (message: string) => void;
  getErrorMessage: () => string | null;
  removeErrorMessage: () => void;
}

type ErrorMessageStore = ErrorMessageState & ErrorMessageActions;

const initialState: ErrorMessageState = {
  errorMessage: null,
};

export const useErrorMessageStore = create<ErrorMessageStore>((set, get) => ({
  ...initialState,

  setErrorMessage: (message: string) =>
    set(() => ({
      errorMessage: message,
    })),

  getErrorMessage: () => get().errorMessage,

  removeErrorMessage: () =>
    set(() => ({
      errorMessage: null,
    })),
}));


Review comment (suggestion):
Same as the other PR, maybe we can get rid of this and retrieve the latest state through `const { errorMessage } = useErrorMessageStore()` directly

Code after:

interface ErrorMessageState {
  errorMessage: string | null;
}

interface ErrorMessageActions {
  setErrorMessage: (message: string) => void;
  removeErrorMessage: () => void;
}

type ErrorMessageStore = ErrorMessageState & ErrorMessageActions;

const initialState: ErrorMessageState = {
  errorMessage: null,
};

export const useErrorMessageStore = create<ErrorMessageStore>((set) => ({
  ...initialState,

  setErrorMessage: (message: string) =>
    set(() => ({
      errorMessage: message,
    })),

  removeErrorMessage: () =>
    set(() => ({
      errorMessage: null,
    })),
}));
Repository: All-Hands-AI/OpenHands
File: openhands-cli/openhands_cli/tui/settings/store.py (Python)

Code before:

            condenser_updates = {}
            if agent.condenser and isinstance(agent.condenser, LLMSummarizingCondenser):
                condenser_updates["llm"] = agent.condenser.llm.model_copy(update={"metadata": get_llm_metadata(
                    model_name=agent.condenser.llm.model,
                    llm_type="condenser",
                    session_id=session_id
                )})

            agent = agent.model_copy(update={
                "llm": updated_llm,
                "tools": updated_tools,
                "mcp_config": {'mcpServers': mcp_config} if mcp_config else {},
                "agent_context": agent_context,
                "condenser": agent.condenser.model_copy(
                    update=condenser_updates
                ) if agent.condenser else None
            })

            return agent
        except FileNotFoundError:
            return None
        except Exception as e:
            print_formatted_text(HTML("\n<red>Agent configuration file is corrupted!</red>"))
            raise e
            return None

    def save(self, agent: Agent) -> None:
        serialized_spec = agent.model_dump_json(context={"expose_secrets": True})
        self.file_store.write(AGENT_SETTINGS_PATH, serialized_spec)



Review comment (bug):
We didn't raise and error here so the user could be re-directed into the settings setup flow. The current logic will require the user to manually delete their settings file to use the CLI again

Code after:

            condenser_updates = {}
            if agent.condenser and isinstance(agent.condenser, LLMSummarizingCondenser):
                condenser_updates["llm"] = agent.condenser.llm.model_copy(update={"metadata": get_llm_metadata(
                    model_name=agent.condenser.llm.model,
                    llm_type="condenser",
                    session_id=session_id
                )})

            agent = agent.model_copy(update={
                "llm": updated_llm,
                "tools": updated_tools,
                "mcp_config": {'mcpServers': mcp_config} if mcp_config else {},
                "agent_context": agent_context,
                "condenser": agent.condenser.model_copy(
                    update=condenser_updates
                ) if agent.condenser else None
            })

            return agent
        except FileNotFoundError:
            return None
        except Exception:
            print_formatted_text(HTML("\n<red>Agent configuration file is corrupted!</red>"))
            return None

    def save(self, agent: Agent) -> None:
        serialized_spec = agent.model_dump_json(context={"expose_secrets": True})
        self.file_store.write(AGENT_SETTINGS_PATH, serialized_spec)