ronantakizawa/codereview-bench huggingface dataset:
Repository: apache/superset
File: superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx (TypeScript)

Code before:
  value: number;
  valueRange: ValueRange;
  alignPositiveNegative: boolean;
}) {
  if (alignPositiveNegative) {
    return 0;
  }
  const [minValue, maxValue] = valueRange;
  const posExtent = Math.abs(Math.max(maxValue, 0));
  const negExtent = Math.abs(Math.min(minValue, 0));
  const tot = posExtent + negExtent;
  return Math.round((Math.min(negExtent + value, negExtent) / tot) * 100);
}

/**
 * Cell background color calculation for horizontal bar chart
 */
function cellBackground({
  value,
  colorPositiveNegative = false,
  theme,
}: {
  value: number;
  colorPositiveNegative: boolean;
  theme: SupersetTheme;
}) {
  if (!colorPositiveNegative) {
    const color = isThemeDark(theme)
      ? 'rgba(255,255,255,0.3)'
      : 'rgba(0,0,0,0.2)';
    return color;
  }

  const color = isThemeDark(theme)
    ? value < 0
      ? `rgba(255,100,100,0.4)`
      : `rgba(255,255,255,0.3)`
    : `rgba(${value < 0 ? 150 : 0},0,0,0.2)`;
  return color;
}

function SortIcon<D extends object>({ column }: { column: ColumnInstance<D> }) {
  const { isSorted, isSortedDesc } = column;
  let sortIcon = <FaSort />;
  if (isSorted) {
    sortIcon = isSortedDesc ? <FaSortDesc /> : <FaSortAsc />;
  }
  return sortIcon;
}

function SearchInput({

Review comment (refactor):
We're missing out on writing a nifty utility function here, maybe like `themeAdaptiveAlpha(0.3)` or similar... Not sure what the ideal name for it is but seems it'd be useful/helpful.

Either that or reuse some of these?
<img width="1235" height="327" alt="Image" src="https://github.com/user-attachments/assets/37323bdf-cc29-4c8a-94e3-9179026f6e7b" />

Code after:
  valueRange: ValueRange;
  alignPositiveNegative: boolean;
}) {
  if (alignPositiveNegative) {
    return 0;
  }
  const [minValue, maxValue] = valueRange;
  const posExtent = Math.abs(Math.max(maxValue, 0));
  const negExtent = Math.abs(Math.min(minValue, 0));
  const tot = posExtent + negExtent;
  return Math.round((Math.min(negExtent + value, negExtent) / tot) * 100);
}

/**
 * Cell background color calculation for horizontal bar chart
 */
function cellBackground({
  value,
  colorPositiveNegative = false,
  theme,
}: {
  value: number;
  colorPositiveNegative: boolean;
  theme: SupersetTheme;
}) {
  if (!colorPositiveNegative) {
    return `${theme.colorFillSecondary}50`;
  }

  if (value < 0) {
    return `${theme.colorError}50`;
  }

  return `${theme.colorSuccess}50`;
}

function SortIcon<D extends object>({ column }: { column: ColumnInstance<D> }) {
  const { isSorted, isSortedDesc } = column;
  let sortIcon = <FaSort />;
  if (isSorted) {
    sortIcon = isSortedDesc ? <FaSortDesc /> : <FaSortAsc />;
  }
  return sortIcon;
}

function SearchInput({
  count,
  value,
  onChange,
  onBlur,
  inputRef,
Repository: apache/superset
File: superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx (TypeScript)

Code before:
  const tot = posExtent + negExtent;
  return Math.round((Math.min(negExtent + value, negExtent) / tot) * 100);
}

/**
 * Cell background color calculation for horizontal bar chart
 */
function cellBackground({
  value,
  colorPositiveNegative = false,
  theme,
}: {
  value: number;
  colorPositiveNegative: boolean;
  theme: SupersetTheme;
}) {
  if (!colorPositiveNegative) {
    const color = isThemeDark(theme)
      ? 'rgba(255,255,255,0.3)'
      : 'rgba(0,0,0,0.2)';
    return color;
  }

  const color = isThemeDark(theme)
    ? value < 0
      ? `rgba(255,100,100,0.4)`
      : `rgba(255,255,255,0.3)`
    : `rgba(${value < 0 ? 150 : 0},0,0,0.2)`;
  return color;
}

function SortIcon<D extends object>({ column }: { column: ColumnInstance<D> }) {
  const { isSorted, isSortedDesc } = column;
  let sortIcon = <FaSort />;
  if (isSorted) {
    sortIcon = isSortedDesc ? <FaSortDesc /> : <FaSortAsc />;
  }
  return sortIcon;
}

function SearchInput({
  count,
  value,
  onChange,
  onBlur,
  inputRef,
}: SearchInputProps) {
  return (
    <Space direction="horizontal" size={4} className="dt-global-filter">
      {t('Search')}
      <Input

Review comment (suggestion):
no likey the hard-coded red here, would it make sense to use one of those? `colorErrorBg*` is alpha-transparency required here or not

Code after:
  return Math.round((Math.min(negExtent + value, negExtent) / tot) * 100);
}

/**
 * Cell background color calculation for horizontal bar chart
 */
function cellBackground({
  value,
  colorPositiveNegative = false,
  theme,
}: {
  value: number;
  colorPositiveNegative: boolean;
  theme: SupersetTheme;
}) {
  if (!colorPositiveNegative) {
    return `${theme.colorFillSecondary}50`;
  }

  if (value < 0) {
    return `${theme.colorError}50`;
  }

  return `${theme.colorSuccess}50`;
}

function SortIcon<D extends object>({ column }: { column: ColumnInstance<D> }) {
  const { isSorted, isSortedDesc } = column;
  let sortIcon = <FaSort />;
  if (isSorted) {
    sortIcon = isSortedDesc ? <FaSortDesc /> : <FaSortAsc />;
  }
  return sortIcon;
}

function SearchInput({
  count,
  value,
  onChange,
  onBlur,
  inputRef,
}: SearchInputProps) {
  return (
    <Space direction="horizontal" size={4} className="dt-global-filter">
      {t('Search')}
      <Input
        aria-label={t('Search %s records', count)}
        placeholder={tn('%s record', '%s records...', count, count)}
        value={value}
        onChange={onChange}
        onBlur={onBlur}
Repository: apache/superset
File: superset-frontend/src/dashboard/components/gridComponents/Tab/Tab.jsx (JavaScript)

Code before:
          [component.id]: {
            ...component,
            meta: {
              ...component.meta,
              text: nextTabText,
            },
          },
        });
      }
    },
    [props.updateComponents, props.component],
  );

  const handleDrop = useCallback(
    dropResult => {
      props.handleComponentDrop(dropResult);
      props.onDropOnTab(dropResult);
    },
    [props.handleComponentDrop, props.onDropOnTab],
  );

  const handleHoverTab = useCallback(() => {
    props.onHoverTab?.();
  }, [props.onHoverTab]);

  const handleTopDropTargetDrop = useCallback(
    dropResult => {
      if (dropResult) {
        props.handleComponentDrop({
          ...dropResult,
          destination: {
            ...dropResult.destination,
            // force appending as the first child if top drop target
            index: 0,
          },
        });
      }
    },
    [props.handleComponentDrop],
  );

  const shouldDropToChild = useCallback(item => item.type !== TAB_TYPE, []);

  const renderTabContent = useCallback(() => {
    const {
      component: tabComponent,
      depth,
      availableColumnCount,
      columnWidth,
      onResizeStart,
      onResize,

Review comment (suggestion):
Could we include only the tabChartIds instead of the entire dashboardLayout object?

const tabChartIds = useMemo(
  () => getChartIdsFromComponent(props.id, dashboardLayout),
  [props.id, dashboardLayout]
);

Code after:
  const dashboardInfo = useSelector(state => state.dashboardInfo);

  useEffect(() => {
    if (props.renderType === RENDER_TAB_CONTENT && props.isComponentVisible) {
      if (
        lastRefreshTime &&
        tabActivationTime &&
        lastRefreshTime > tabActivationTime
      ) {
        const chartIds = getChartIdsFromComponent(props.id, dashboardLayout);
        if (chartIds.length > 0) {
          requestAnimationFrame(() => {
            setTimeout(() => {
              dispatch(onRefresh(chartIds, true, 0, dashboardInfo.id));
            }, CHART_MOUNT_DELAY);
          });
        }
      }
    }
  }, [
    props.isComponentVisible,
    props.renderType,
    props.id,
    lastRefreshTime,
    tabActivationTime,
    dashboardLayout,
    dashboardInfo.id,
    dispatch,
  ]);

  const handleChangeTab = useCallback(
    ({ pathToTabIndex }) => {
      props.setDirectPathToChild(pathToTabIndex);
    },
    [props.setDirectPathToChild],
  );

  const handleChangeText = useCallback(
    nextTabText => {
      const { updateComponents, component } = props;
      if (nextTabText && nextTabText !== component.meta.text) {
        updateComponents({
          [component.id]: {
            ...component,
            meta: {
              ...component.meta,
              text: nextTabText,
            },
          },
        });
      }
Repository: apache/superset
File: superset-frontend/packages/superset-ui-core/src/components/TableCollection/TableCollection.test.tsx (TypeScript)

Code before:
  const checkboxes = screen.getAllByRole('checkbox');
  expect(checkboxes.length).toBeGreaterThan(0);
});

test('handleTableChange should convert array field to dot notation for nested fields', () => {
  const setSortBy = jest.fn();
  const sortingProps = {
    ...defaultProps,
    setSortBy,
  };

  const component = render(<TableCollection {...sortingProps} />);

  // Get the Table component instance
  const table = component.container.querySelector('.ant-table');
  expect(table).toBeInTheDocument();

  // Simulate the handleTableChange function behavior directly
  // This tests the logic without requiring actual table interaction
  const mockSorter = {
    field: ['database', 'database_name'], // Array format from AntD
    order: 'descend',
  };

  // Get the component instance to test the callback logic
  const handleTableChange = (_pagination: any, _filters: any, sorter: any) => {
    if (sorter && sorter.field) {
      // This is the logic we implemented in the fix
      const fieldId = Array.isArray(sorter.field)
        ? sorter.field.join('.')
        : sorter.field;

      setSortBy([
        {
          id: fieldId,
          desc: sorter.order === 'descend',
        },
      ]);
    }
  };

  // Test the callback logic with array field
  handleTableChange(null, null, mockSorter);

  expect(setSortBy).toHaveBeenCalledWith([
    {
      id: 'database.database_name', // Should be converted to dot notation
      desc: true,
    },
  ]);


Review comment (suggestion):
```suggestion
  const handleTableChange = (_pagination: any, _filters: any, sorter: SorterResult) => {
```

Code after:
    toggleAllRowsSelected,
    pageSize: 2,
    totalCount: 3,
    pageIndex: 0,
    onPageChange: jest.fn(),
  };
  render(<TableCollection {...selectionProps} />);

  // Check that selection checkboxes are rendered
  const checkboxes = screen.getAllByRole('checkbox');
  expect(checkboxes.length).toBeGreaterThan(0);
});

test('should call setSortBy when clicking sortable column header', () => {
  const setSortBy = jest.fn();
  const sortingProps = {
    ...defaultProps,
    setSortBy,
  };

  render(<TableCollection {...sortingProps} />);

  // Target the nested field column (the column that needs the array-to-dot conversion)
  const nestedFieldHeader = screen.getByText('Nested Field');
  expect(nestedFieldHeader).toBeInTheDocument();

  // Click on the nested field column header to trigger sorting
  fireEvent.click(nestedFieldHeader);

  // Verify setSortBy was called with the correct arguments and dot notation conversion
  expect(setSortBy).toHaveBeenCalledWith([
    {
      id: 'parent.child',
      desc: expect.any(Boolean),
    },
  ]);
});
Repository: apache/superset
File: superset-frontend/packages/superset-ui-core/src/components/TableCollection/TableCollection.test.tsx (TypeScript)

Code before:

  expect(screen.getByRole('table')).toBeInTheDocument();

  const mockSorter = {
    field: ['database', 'database_name'], // Array format from AntD
    order: 'descend' as const,
  };

  const handleTableChange = (
    _pagination: any,
    _filters: any,
    sorter: { field: string | string[]; order: 'ascend' | 'descend' },
  ) => {
    if (sorter && sorter.field) {
      const fieldId = Array.isArray(sorter.field)
        ? sorter.field.join('.')
        : sorter.field;

      setSortBy([
        {
          id: fieldId,
          desc: sorter.order === 'descend',
        },
      ]);
    }
  };

  // Test the callback logic with array field
  handleTableChange(null, null, mockSorter);

  expect(setSortBy).toHaveBeenCalledWith([
    {
      id: 'database.database_name', // Should be converted to dot notation
      desc: true,
    },
  ]);

  // Test with string field (should pass through unchanged)
  setSortBy.mockClear();
  const mockStringSorter = {
    field: 'table_name',
    order: 'ascend' as const,
  };

  handleTableChange(null, null, mockStringSorter);

  expect(setSortBy).toHaveBeenCalledWith([
    {
      id: 'table_name',
      desc: false,
    },

Review comment (suggestion):
This does the `implement same thing in test and test that` again (just one level above) and still doesn't interact with the component

Code after:

test('should call setSortBy when clicking sortable column header', () => {
  const setSortBy = jest.fn();
  const sortingProps = {
    ...defaultProps,
    setSortBy,
  };

  render(<TableCollection {...sortingProps} />);

  // Target the nested field column (the column that needs the array-to-dot conversion)
  const nestedFieldHeader = screen.getByText('Nested Field');
  expect(nestedFieldHeader).toBeInTheDocument();

  // Click on the nested field column header to trigger sorting
  fireEvent.click(nestedFieldHeader);

  // Verify setSortBy was called with the correct arguments and dot notation conversion
  expect(setSortBy).toHaveBeenCalledWith([
    {
      id: 'parent.child',
      desc: expect.any(Boolean),
    },
  ]);
});
Repository: apache/superset
File: superset/db_engine_specs/doris.py (Python)

Code before:

    @property
    def python_type(self) -> Optional[type[dict[Any, Any]]]:
        return dict


class STRUCT(TypeEngine):
    __visit_name__ = "STRUCT"

    @property
    def python_type(self) -> Optional[type[Any]]:
        return None


class DorisEngineSpec(MySQLEngineSpec):
    engine = "pydoris"
    engine_aliases = {"doris"}
    engine_name = "Apache Doris"
    max_column_name_length = 64
    default_driver = "pydoris"
    sqlalchemy_uri_placeholder = (
        "doris://user:password@host:port/catalog.db[?key=value&key=value...]"
    )
    encryption_parameters = {"ssl": "0"}
    supports_dynamic_schema = True
    supports_catalog = supports_dynamic_catalog = True

    column_type_mappings = (  # type: ignore
        (
            re.compile(r"^tinyint", re.IGNORECASE),
            TINYINT(),
            GenericDataType.NUMERIC,
        ),
        (
            re.compile(r"^largeint", re.IGNORECASE),
            LARGEINT(),
            GenericDataType.NUMERIC,
        ),
        (
            re.compile(r"^decimal.*", re.IGNORECASE),
            types.DECIMAL(),
            GenericDataType.NUMERIC,
        ),
        (
            re.compile(r"^double", re.IGNORECASE),
            DOUBLE(),
            GenericDataType.NUMERIC,
        ),
        (
            re.compile(r"^varchar(\((\d+)\))*$", re.IGNORECASE),
            types.VARCHAR(),

Review comment (suggestion):
Let's leave a comment here so we have more context in the future:
```suggestion
    supports_catalog = supports_dynamic_catalog = True
    # while technically supported by Doris, this generates invalid table identifiers
    supports_cross_catalog_queries = False
```

Code after:

    @property
    def python_type(self) -> Optional[type[dict[Any, Any]]]:
        return dict


class STRUCT(TypeEngine):
    __visit_name__ = "STRUCT"

    @property
    def python_type(self) -> Optional[type[Any]]:
        return None


class DorisEngineSpec(MySQLEngineSpec):
    engine = "pydoris"
    engine_aliases = {"doris"}
    engine_name = "Apache Doris"
    max_column_name_length = 64
    default_driver = "pydoris"
    sqlalchemy_uri_placeholder = (
        "doris://user:password@host:port/catalog.db[?key=value&key=value...]"
    )
    encryption_parameters = {"ssl": "0"}
    supports_dynamic_schema = True
    supports_catalog = supports_dynamic_catalog = True
    # while technically supported by Doris, this generates invalid table identifiers
    supports_cross_catalog_queries = False

    column_type_mappings = (  # type: ignore
        (
            re.compile(r"^tinyint", re.IGNORECASE),
            TINYINT(),
            GenericDataType.NUMERIC,
        ),
        (
            re.compile(r"^largeint", re.IGNORECASE),
            LARGEINT(),
            GenericDataType.NUMERIC,
        ),
        (
            re.compile(r"^decimal.*", re.IGNORECASE),
            types.DECIMAL(),
            GenericDataType.NUMERIC,
        ),
        (
            re.compile(r"^double", re.IGNORECASE),
            DOUBLE(),
            GenericDataType.NUMERIC,
        ),
        (
Repository: apache/superset
File: superset/sql/dialects/pinot.py (Python)

Code before:
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements.  See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership.  The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License.  You may obtain a copy of the License at
#
#   http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied.  See the License for the
# specific language governing permissions and limitations
# under the License.

"""
MySQL ANSI dialect for Apache Pinot.

This dialect is based on MySQL but follows ANSI SQL quoting conventions where
double quotes are used for identifiers instead of string literals.
"""

from __future__ import annotations

from sqlglot.dialects.mysql import MySQL


class Pinot(MySQL):
    """
    MySQL ANSI dialect used by Apache Pinot.

    The main difference from standard MySQL is that double quotes (") are used for
    identifiers instead of string literals, following ANSI SQL conventions.
    """

    class Tokenizer(MySQL.Tokenizer):
        QUOTES = ["'"]  # Only single quotes for strings
        IDENTIFIERS = ['"', "`"]  # Backticks and double quotes for identifiers
        STRING_ESCAPES = ["'", "\\"]  # Remove double quote from string escapes


Review comment (security):
nit: should these have type hints? 
```
class Tokenizer(MySQL.Tokenizer):
      QUOTES: list[str] = ["'"]
      IDENTIFIERS: list[str] = ['"', "`"]
      STRING_ESCAPES: list[str] = ["'", "\\"]
```

Code after:
"""
MySQL ANSI dialect for Apache Pinot.

This dialect is based on MySQL but follows ANSI SQL quoting conventions where
double quotes are used for identifiers instead of string literals.
"""

from __future__ import annotations

from sqlglot.dialects.mysql import MySQL


class Pinot(MySQL):
    """
    MySQL ANSI dialect used by Apache Pinot.

    The main difference from standard MySQL is that double quotes (") are used for
    identifiers instead of string literals, following ANSI SQL conventions.

    See: https://calcite.apache.org/javadocAggregate/org/apache/calcite/config/Lex.html#MYSQL_ANSI
    """

    class Tokenizer(MySQL.Tokenizer):
        QUOTES = ["'"]  # Only single quotes for strings
        IDENTIFIERS = ['"', "`"]  # Backticks and double quotes for identifiers
        STRING_ESCAPES = ["'", "\\"]  # Remove double quote from string escapes
Repository: apache/superset
File: superset-frontend/src/features/home/RightMenu.tsx (TypeScript)

Code before:
    // because the rest don't need this information. Not using
    // "Data" directly since we might change the label later on?
    if (
      openKeys.length > 1 &&
      !isEmpty(
        openKeys?.filter((key: string) =>
          key.includes(`sub2_${dropdownItems?.[0]?.label}`),
        ),
      )
    ) {
      if (canUploadData) checkAllowUploads();
      if (canDatabase || canDataset) existsNonExamplesDatabases();
    }
    return null;
  };
  const RightMenuExtension = extensionsRegistry.get('navbar.right');
  const RightMenuItemIconExtension = extensionsRegistry.get(
    'navbar.right-menu.item.icon',
  );

  const handleDatabaseAdd = () => setQuery({ databaseAdded: true });

  const handleLogout = () => {
    try {
      if (typeof window !== 'undefined') {
        if (window.localStorage?.removeItem) {
          window.localStorage.removeItem('redux');
        }
        if (window.sessionStorage?.removeItem) {
          window.sessionStorage.removeItem('login_attempted');
        }
      }
    } catch {
      // Swallow storage errors to avoid crashing when storage is unavailable
    }
  };

  // Use the theme menu hook
  const themeMenuItem = useThemeMenuItems({
    setThemeMode,
    themeMode,
    hasLocalOverride: hasDevOverride(),
    onClearLocalSettings: clearLocalOverrides,
    allowOSPreference: canDetectOSPreference(),
  });

  const languageMenuItem = useLanguageMenuItems({
    locale: navbarRight.locale || 'en',
    languages: navbarRight.languages || {},
  });


Review comment (suggestion):
```suggestion
        if (window.localStorage.removeItem) {
```

Code after:
        ),
      )
    ) {
      if (canUploadData) checkAllowUploads();
      if (canDatabase || canDataset) existsNonExamplesDatabases();
    }
    return null;
  };
  const RightMenuExtension = extensionsRegistry.get('navbar.right');
  const RightMenuItemIconExtension = extensionsRegistry.get(
    'navbar.right-menu.item.icon',
  );

  const handleDatabaseAdd = () => setQuery({ databaseAdded: true });

  const handleLogout = () => {
    try {
      window.localStorage.removeItem('redux');
      window.sessionStorage.removeItem('login_attempted');
    } catch (error) {
      console.warn('Failed to clear storage on logout:', error);
    }
  };

  // Use the theme menu hook
  const themeMenuItem = useThemeMenuItems({
    setThemeMode,
    themeMode,
    hasLocalOverride: hasDevOverride(),
    onClearLocalSettings: clearLocalOverrides,
    allowOSPreference: canDetectOSPreference(),
  });

  const languageMenuItem = useLanguageMenuItems({
    locale: navbarRight.locale || 'en',
    languages: navbarRight.languages || {},
  });

  // Build main menu items
  const menuItems = useMemo(() => {
    // Build menu items for the new dropdown
    const buildNewDropdownItems = (): MenuItem[] => {
      const items: MenuItem[] = [];

      dropdownItems?.forEach(menu => {
        const canShowChild = menu.childs?.some(
          item => typeof item === 'object' && !!item.perm,
        );

        if (menu.childs) {
          if (canShowChild) {