ronantakizawa/codereview-bench huggingface dataset:
Repository: apache/superset
File: superset-frontend/packages/superset-ui-core/src/components/Select/types.ts (TypeScript)

Code before:
   */
  sortComparator?: (
    a: AntdLabeledValue,
    b: AntdLabeledValue,
    search?: string,
  ) => number;
  /**
   * Sets maxTagCount to 1. The overflow tag is always displayed in
   * the same line, line wrapping is disabled.
   * When the dropdown is open, sets maxTagCount to 0,
   * displays only the overflow tag.
   */
  oneLine?: boolean;

  suffixIcon?: ReactNode;

  value?: SelectValue | null;

  ref: Ref<RefSelectProps>;

  dropdownStyle?: React.CSSProperties;

  /**
   * It defines the alignment of the dropdown menu.
   */
  dropdownAlign?: AlignType;
}

export interface SelectProps extends BaseSelectProps {
  css?: Interpolation<Theme>;
  /**
   * It enables the user to select all options.
   * True by default.
   * */
  allowSelectAll?: boolean;
  /**
   * It defines the options of the Select.
   * The options can be static, an array of options.
   */
  options: SelectOptionsType;
}

export type AsyncSelectRef = RefSelectProps & {
  clearCache: () => void;
};

export type SelectOptionsTypePage = {
  data: SelectOptionsType;
  totalCount: number;
};


Review comment (suggestion):
Hey, i meant modifying `type AntdExposedProps` and add this there since it already exists we just don't expose it for now. No need to define a seperate type

Code after:
   */
  sortComparator?: (
    a: AntdLabeledValue,
    b: AntdLabeledValue,
    search?: string,
  ) => number;
  /**
   * Sets maxTagCount to 1. The overflow tag is always displayed in
   * the same line, line wrapping is disabled.
   * When the dropdown is open, sets maxTagCount to 0,
   * displays only the overflow tag.
   */
  oneLine?: boolean;

  suffixIcon?: ReactNode;

  value?: SelectValue | null;

  ref: Ref<RefSelectProps>;

  dropdownStyle?: React.CSSProperties;
}

export interface SelectProps extends BaseSelectProps {
  css?: Interpolation<Theme>;
  /**
   * It enables the user to select all options.
   * True by default.
   * */
  allowSelectAll?: boolean;
  /**
   * It defines the options of the Select.
   * The options can be static, an array of options.
   */
  options: SelectOptionsType;
}

export type AsyncSelectRef = RefSelectProps & {
  clearCache: () => void;
};

export type SelectOptionsTypePage = {
  data: SelectOptionsType;
  totalCount: number;
};

export type SelectOptionsPagePromise = (
  search: string,
  page: number,
  pageSize: number,
) => Promise<SelectOptionsTypePage>;
Repository: apache/superset
File: superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx (TypeScript)

Code before:
              'test-filter': {
                name: 'Test Filter',
              },
            },
          },
          dataMask: {
            'test-filter': {
              extraFormData: {},
              filterState: {
                value: undefined,
              },
            },
          },
        },
      },
    );

    const filterSelect = screen.getByRole('combobox');
    await userEvent.click(filterSelect);

    const falseOption = await screen.findByRole('option', { name: /false/i });
    expect(falseOption).toBeInTheDocument();
    await userEvent.click(falseOption);

    const selectedElements = await screen.findAllByTitle('FALSE');
    expect(selectedElements.length).toBeGreaterThan(0);

    expect(setDataMaskMock).toHaveBeenCalledWith(
      expect.objectContaining({
        extraFormData: {
          filters: [
            {
              col: 'is_active',
              op: 'IN',
              val: [false],
            },
          ],
        },
        filterState: expect.objectContaining({
          value: [false],
        }),
      }),
    );
  });

  test('preserves backend order even when sortAscending is false and sortMetric is specified', () => {
    const testData = [
      { gender: 'zebra' },
      { gender: 'alpha' },
      { gender: 'beta' },
    ];

Review comment (suggestion):
```suggestion
expect(await screen.findByTitle('FALSE')).toBeInTheDocument();
```

Code after:
      },
    };

    render(
      // @ts-ignore
      <SelectFilterPlugin
        // @ts-ignore
        {...transformProps(testProps)}
        setDataMask={jest.fn()}
        showOverflow={false}
      />,
      {
        useRedux: true,
        initialState: {
          nativeFilters: {
            filters: {
              'test-filter': {
                name: 'Test Filter',
              },
            },
          },
          dataMask: {
            'test-filter': {
              extraFormData: {},
              filterState: {
                value: [],
                label: '',
                excludeFilterValues: true,
              },
            },
          },
        },
      },
    );

    const filterSelect = screen.getAllByRole('combobox')[0];
    userEvent.click(filterSelect);

    // When sortMetric is specified, original order should be preserved regardless
    // of sortAscending value (zebra, alpha, beta)
    const options = screen.getAllByRole('option');
    expect(options[0]).toHaveTextContent('zebra');
    expect(options[1]).toHaveTextContent('alpha');
    expect(options[2]).toHaveTextContent('beta');
  });
});

test('Select boolean FALSE value in single-select mode', async () => {
  jest.useRealTimers();
  const setDataMaskMock = jest.fn();
  const testProps = {
Repository: apache/superset
File: superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx (TypeScript)

Code before:
              },
            },
          },
        },
      },
    );

    const filterSelect = screen.getAllByRole('combobox')[0];
    userEvent.click(filterSelect);

    // When sortMetric is specified, original order should be preserved regardless
    // of sortAscending value (zebra, alpha, beta)
    const options = screen.getAllByRole('option');
    expect(options[0]).toHaveTextContent('zebra');
    expect(options[1]).toHaveTextContent('alpha');
    expect(options[2]).toHaveTextContent('beta');
  });
});

test('Select boolean FALSE value in single-select mode', async () => {
  jest.useRealTimers();
  const setDataMaskMock = jest.fn();
  const testProps = {
    ...selectMultipleProps,
    formData: {
      ...selectMultipleProps.formData,
      multiSelect: false,
      groupby: ['is_active'],
    },
    queriesData: [
      {
        ...selectMultipleProps.queriesData[0],
        colnames: ['is_active'],
        data: [{ is_active: true }, { is_active: false }],
        applied_filters: [{ column: 'is_active' }],
      },
    ],
    filterState: { value: undefined },
  };

  render(
    // @ts-ignore
    <SelectFilterPlugin
      // @ts-ignore
      {...transformProps(testProps)}
      setDataMask={setDataMaskMock}
      showOverflow={false}
    />,
    {
      useRedux: true,
      initialState: {

Review comment (refactor):
I have used `jest.useRealTimers()` to avoid having to refactor the whole test file, but I'm open to changing that if there's a better solution available 

Code after:
              extraFormData: {},
              filterState: {
                value: [],
                label: '',
                excludeFilterValues: true,
              },
            },
          },
        },
      },
    );

    const filterSelect = screen.getAllByRole('combobox')[0];
    userEvent.click(filterSelect);

    // When sortMetric is specified, original order should be preserved regardless
    // of sortAscending value (zebra, alpha, beta)
    const options = screen.getAllByRole('option');
    expect(options[0]).toHaveTextContent('zebra');
    expect(options[1]).toHaveTextContent('alpha');
    expect(options[2]).toHaveTextContent('beta');
  });
});

test('Select boolean FALSE value in single-select mode', async () => {
  jest.useRealTimers();
  const setDataMaskMock = jest.fn();
  const testProps = {
    ...selectMultipleProps,
    formData: {
      ...selectMultipleProps.formData,
      multiSelect: false,
      groupby: ['is_active'],
    },
    queriesData: [
      {
        ...selectMultipleProps.queriesData[0],
        colnames: ['is_active'],
        data: [{ is_active: true }, { is_active: false }],
        applied_filters: [{ column: 'is_active' }],
      },
    ],
    filterState: { value: undefined },
  };

  render(
    // @ts-ignore
    <SelectFilterPlugin
      // @ts-ignore
      {...transformProps(testProps)}
      setDataMask={setDataMaskMock}
Repository: apache/superset
File: superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx (TypeScript)

Code before:

export const xAxisForceCategoricalControl = {
  name: 'xAxisForceCategorical',
  config: {
    type: 'CheckboxControl',
    label: () => t('Force categorical'),
    default: false,
    description: t('Treat values as categorical.'),
    initialValue: (control: ControlState, state: ControlPanelState | null) => {
      const isNumericXAxis = checkColumnType(
        getColumnLabel(state?.controls?.x_axis?.value as QueryFormColumn),
        state?.controls?.datasource?.datasource,
        [GenericDataType.Numeric],
      );
      if (!isNumericXAxis) {
        return control.value;
      }
      return state?.form_data?.x_axis_sort !== undefined || control.value;
    },
    renderTrigger: true,
    visibility: ({ controls }: { controls: ControlStateMapping }) =>
      checkColumnType(
        getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
        controls?.datasource?.datasource,
        [GenericDataType.Numeric],
      ),
    shouldMapStateToProps: () => true,
  },
};


Review comment (suggestion):
```suggestion
      return state?.form_data?.x_axis_sort ?? control.value;
```

Code after:

export const xAxisForceCategoricalControl = {
  name: 'xAxisForceCategorical',
  config: {
    type: 'CheckboxControl',
    label: () => t('Force categorical'),
    default: false,
    description: t('Treat values as categorical.'),
    initialValue: (control: ControlState, state: ControlPanelState | null) => {
      // Check if x-axis is numeric - only numeric columns should have
      // their categorical behavior influenced by x_axis_sort setting
      const isNumericXAxis = checkColumnType(
        getColumnLabel(state?.controls?.x_axis?.value as QueryFormColumn),
        state?.controls?.datasource?.datasource,
        [GenericDataType.Numeric],
      );

      // Non-numeric columns (temporal, text) should not be forced categorical
      // based on x_axis_sort - just use the control's existing value
      if (!isNumericXAxis) {
        return control.value;
      }

      // For numeric columns, force categorical if x_axis_sort is defined
      // (user wants to sort) or use the control's existing value
      return state?.form_data?.x_axis_sort !== undefined || control.value;
    },
    renderTrigger: true,
    visibility: ({ controls }: { controls: ControlStateMapping }) =>
      checkColumnType(
        getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
        controls?.datasource?.datasource,
        [GenericDataType.Numeric],
      ),
    shouldMapStateToProps: () => true,
  },
};
Repository: apache/superset
File: superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts (TypeScript)

Code before:
        markerEnabled,
        markerSize,
        areaOpacity: opacity,
        seriesType,
        legendState,
        stack,
        formatter: forcePercentFormatter
          ? percentFormatter
          : (getCustomFormatter(
              customFormatters,
              metrics,
              labelMap?.[seriesName]?.[0],
            ) ?? defaultFormatter),
        showValue,
        onlyTotal,
        totalStackedValues: sortedTotalValues,
        showValueIndexes,
        thresholdValues,
        richTooltip,
        sliceId,
        isHorizontal,
        lineStyle,
        timeCompare: array,
        timeShiftColor,
        theme,
        hasDimensions: groupBy.length > 0,
      },
    );
    if (transformedSeries) {
      if (stack === StackControlsValue.Stream) {
        // bug in Echarts - `stackStrategy: 'all'` doesn't work with nulls, so we cast them to 0
        series.push({
          ...transformedSeries,
          data: (transformedSeries.data as any).map(
            (row: [string | number, number]) => [row[0], row[1] ?? 0],
          ),
        });
      } else {
        series.push(transformedSeries);
      }
    }
  });

  if (stack === StackControlsValue.Stream) {
    const baselineSeries = getBaselineSeriesForStream(
      series.map(entry => entry.data) as [string | number, number][][],
      seriesType,
    );

    series.unshift(baselineSeries);
  }

Review comment (suggestion):
```suggestion
        hasDimensions: (groupby?.length ?? 0) > 0,
```

Code after:
        markerEnabled,
        markerSize,
        areaOpacity: opacity,
        seriesType,
        legendState,
        stack,
        formatter: forcePercentFormatter
          ? percentFormatter
          : (getCustomFormatter(
              customFormatters,
              metrics,
              labelMap?.[seriesName]?.[0],
            ) ?? defaultFormatter),
        showValue,
        onlyTotal,
        totalStackedValues: sortedTotalValues,
        showValueIndexes,
        thresholdValues,
        richTooltip,
        sliceId,
        isHorizontal,
        lineStyle,
        timeCompare: array,
        timeShiftColor,
        theme,
        hasDimensions: (groupBy?.length ?? 0) > 0,
      },
    );
    if (transformedSeries) {
      if (stack === StackControlsValue.Stream) {
        // bug in Echarts - `stackStrategy: 'all'` doesn't work with nulls, so we cast them to 0
        series.push({
          ...transformedSeries,
          data: (transformedSeries.data as any).map(
            (row: [string | number, number]) => [row[0], row[1] ?? 0],
          ),
        });
      } else {
        series.push(transformedSeries);
      }
    }
  });

  if (stack === StackControlsValue.Stream) {
    const baselineSeries = getBaselineSeriesForStream(
      series.map(entry => entry.data) as [string | number, number][][],
      seriesType,
    );

    series.unshift(baselineSeries);
  }
Repository: apache/superset
File: tests/integration_tests/utils/encrypt_tests.py (Python)

Code before:
        # Step 3: Rotate key to KEY_B
        self.app.config["SECRET_KEY"] = key_b

        # Step 4: Re-encrypt with KEY_B (simulating SecretsMigrator logic)
        # Decrypt using previous key
        previous_field = EncryptedType(type_in=field.underlying_type, key=key_a)
        decrypted_with_prev = previous_field.process_result_value(encrypted_a, dialect)
        assert decrypted_with_prev == test_value

        # Re-encrypt using current key (KEY_B, resolved via lambda)
        encrypted_b = field.process_bind_param(decrypted_with_prev, dialect)
        assert encrypted_b is not None
        assert encrypted_b != encrypted_a  # Different ciphertext

        # Step 5: Verify decryption with KEY_B works
        decrypted_b = field.process_result_value(encrypted_b, dialect)
        assert decrypted_b == test_value

        # Step 6: Verify KEY_A can no longer decrypt the new ciphertext
        self.app.config["SECRET_KEY"] = key_a
        try:
            bad_decrypt = field.process_result_value(encrypted_b, dialect)
            # If decryption doesn't raise, value should be garbage
            assert bad_decrypt != test_value
        except (ValueError, Exception):
            pass  # Expected: old key cannot decrypt new ciphertext

        # Restore original key
        self.app.config["SECRET_KEY"] = key_a


Review comment (suggestion):
there should be an assertion here for the thrown error to ensure that indeed it's the decryption issue

Code after:

        # Step 3: Rotate key to KEY_B
        self.app.config["SECRET_KEY"] = key_b

        # Step 4: Re-encrypt with KEY_B (simulating SecretsMigrator logic)
        # Decrypt using previous key
        previous_field = EncryptedType(type_in=field.underlying_type, key=key_a)
        decrypted_with_prev = previous_field.process_result_value(encrypted_a, dialect)
        assert decrypted_with_prev == test_value

        # Re-encrypt using current key (KEY_B, resolved via lambda)
        encrypted_b = field.process_bind_param(decrypted_with_prev, dialect)
        assert encrypted_b is not None
        assert encrypted_b != encrypted_a  # Different ciphertext

        # Step 5: Verify decryption with KEY_B works
        decrypted_b = field.process_result_value(encrypted_b, dialect)
        assert decrypted_b == test_value

        # Step 6: Verify KEY_A can no longer decrypt the new ciphertext
        self.app.config["SECRET_KEY"] = key_a
        with pytest.raises(ValueError, match="Invalid decryption key"):
            field.process_result_value(encrypted_b, dialect)

        # Restore original key
        self.app.config["SECRET_KEY"] = key_a
Repository: apify/crawlee-python
File: src/crawlee/http_crawler/_http_crawler.py (Python)

Code before:
            push_data=context.push_data,
            get_key_value_store=context.get_key_value_store,
            log=context.log,
            http_response=result.http_response,
        )

    async def _handle_blocked_request(self, context: HttpCrawlingContext) -> AsyncGenerator[HttpCrawlingContext, None]:
        """Try to detect if the request is blocked based on the HTTP status code.

        Args:
            context: The current crawling context.

        Raises:
            SessionError: If the request is considered blocked.

        Yields:
            The original crawling context if no errors are detected.
        """
        if self._retry_on_blocked:
            status_code = context.http_response.status_code

            if context.session and context.session.is_blocked_status_code(status_code=status_code):
                raise SessionError(f'Assuming the session is blocked based on HTTP status code {status_code}')

        yield context


Review comment (suggestion):
Wondering whether this part should not look more like this:

```python
if context.session and context.session.is_blocked_status_code(
    status_code=status_code,
    additional_blocked_status_codes=self._http_client.additional_blocked_status_codes,
    ignore_http_error_status_codes=self._http_client.ignore_http_error_status_codes
):
```

which would require further changes as well, but since you're touching it here, I'm mentioning it ... Maybe do it in other PR. If I am not overlooking anything.

Code after:
            push_data=context.push_data,
            get_key_value_store=context.get_key_value_store,
            log=context.log,
            http_response=result.http_response,
        )

    async def _handle_blocked_request(self, context: HttpCrawlingContext) -> AsyncGenerator[HttpCrawlingContext, None]:
        """Try to detect if the request is blocked based on the HTTP status code.

        Args:
            context: The current crawling context.

        Raises:
            SessionError: If the request is considered blocked.

        Yields:
            The original crawling context if no errors are detected.
        """
        if self._retry_on_blocked:
            status_code = context.http_response.status_code

            if (
                context.session
                and status_code not in self._http_client._ignore_http_error_status_codes  # noqa: SLF001
                and context.session.is_blocked_status_code(status_code=status_code)
            ):
                raise SessionError(f'Assuming the session is blocked based on HTTP status code {status_code}')

        yield context
Repository: apify/crawlee-python
File: docs/guides/proxy_management_examples/tiers_beautifulsoup.py (Python)

Code before:
import asyncio

from crawlee.beautifulsoup_crawler import BeautifulSoupCrawler, BeautifulSoupCrawlingContext
from crawlee.proxy_configuration import ProxyConfiguration


async def main() -> None:
    # Create a ProxyConfiguration object and pass it to the crawler.
    proxy_configuration = ProxyConfiguration(
        tiered_proxy_urls=[
            ['http://cheap-datacenter-proxy-1.com/', 'http://cheap-datacenter-proxy-2.com/'],
            ['http://expensive-residential-proxy-1.com/', 'http://expensive-residential-proxy-2.com/'],
        ]
    )
    crawler = BeautifulSoupCrawler(proxy_configuration=proxy_configuration)

    # Define the default request handler, which will be called for every request.
    @crawler.router.default_handler
    async def default_handler(context: BeautifulSoupCrawlingContext) -> None:
        # Log the proxy used for the current request.
        context.log.info(f'Proxy for the current request: {context.proxy_info}')

    # Run the crawler with the initial list of requests.
    await crawler.run(['https://crawlee.dev/'])


if __name__ == '__main__':
    asyncio.run(main())


Review comment (performance):
lets add comments to each line, something like this

```suggestion
            # lower tier, cheaper, preferred as long as they work
            ['http://cheap-datacenter-proxy-1.com/', 'http://cheap-datacenter-proxy-2.com/'],
            # higher tier, more expensive, used as a fallback
            ['http://expensive-residential-proxy-1.com/', 'http://expensive-residential-proxy-2.com/'],
```

Code after:
import asyncio

from crawlee.beautifulsoup_crawler import BeautifulSoupCrawler, BeautifulSoupCrawlingContext
from crawlee.proxy_configuration import ProxyConfiguration


async def main() -> None:
    # Create a ProxyConfiguration object and pass it to the crawler.
    proxy_configuration = ProxyConfiguration(
        tiered_proxy_urls=[
            # lower tier, cheaper, preferred as long as they work
            ['http://cheap-datacenter-proxy-1.com/', 'http://cheap-datacenter-proxy-2.com/'],
            # higher tier, more expensive, used as a fallback
            ['http://expensive-residential-proxy-1.com/', 'http://expensive-residential-proxy-2.com/'],
        ]
    )
    crawler = BeautifulSoupCrawler(proxy_configuration=proxy_configuration)

    # Define the default request handler, which will be called for every request.
    @crawler.router.default_handler
    async def default_handler(context: BeautifulSoupCrawlingContext) -> None:
        # Log the proxy used for the current request.
        context.log.info(f'Proxy for the current request: {context.proxy_info}')

    # Run the crawler with the initial list of requests.
    await crawler.run(['https://crawlee.dev/'])


if __name__ == '__main__':
    asyncio.run(main())
Repository: apify/crawlee-python
File: src/crawlee/beautifulsoup_crawler/__init__.py (Python)

Code before:
try:
    from .beautifulsoup_crawler import BeautifulSoupCrawler
    from .types import BeautifulSoupCrawlingContext
except ImportError as exc:
    raise ImportError(
        "To import anything from this subpacakge, you need to install the 'beautifulsoup' extra."
        "For example, if you use pip, run `pip install 'crawlee[beautifulsoup]'`.",
    ) from exc

__all__ = ['BeautifulSoupCrawler', 'BeautifulSoupCrawlingContext']


Review comment (suggestion):
```suggestion
        "To import anything from this subpackage, you need to install the 'beautifulsoup' extra."
```

Code after:
try:
    from .beautifulsoup_crawler import BeautifulSoupCrawler
    from .types import BeautifulSoupCrawlingContext
except ImportError as exc:
    raise ImportError(
        "To import anything from this subpackage, you need to install the 'beautifulsoup' extra."
        "For example, if you use pip, run `pip install 'crawlee[beautifulsoup]'`.",
    ) from exc

__all__ = ['BeautifulSoupCrawler', 'BeautifulSoupCrawlingContext']