ronantakizawa/codereview-bench huggingface dataset:
Repository: apache/superset
File: superset-frontend/src/explore/components/PropertiesModal/index.tsx (TypeScript)

Code before:
      payload.tags = tags.map(tag => tag.id);
    }

    try {
      let res = await SupersetClient.put({
        endpoint: `/api/v1/chart/${slice.slice_id}`,
        headers: { 'Content-Type': 'application/json' },
        body: JSON.stringify(payload),
      });
      res = await SupersetClient.get({
        endpoint: `/api/v1/chart/${slice.slice_id}`,
      });
      onSave(res.json.result);
      addSuccessToast(t('Chart properties updated'));
      onHide();
    } catch (res) {
      const clientError = await getClientErrorObject(res);
      showError(clientError);
    }
    setSubmitting(false);
  };

  const ownersLabel = t('Owners');

  // get the owners of this slice
  useEffect(() => {
    fetchChartOwners();
  }, [fetchChartOwners]);

  // update name after it's changed in another modal
  useEffect(() => {
    setName(slice.slice_name || '');
  }, [slice.slice_name]);

  useEffect(() => {
    if (!isFeatureEnabled(FeatureFlag.TaggingSystem)) return;
    try {
      fetchTags(
        {
          objectType: OBJECT_TYPES.CHART,
          objectId: slice.slice_id,
          includeTypes: false,
        },
        (tags: TagType[]) => setTags(tags),
        error => {
          showError(error);
        },
      );
    } catch (error) {
      showError(error);
    }

Review comment (suggestion):
@rusackas so i was thinking about this again, and noticed that for datasets, there are actually multiple get requests. I was wondering if it was just better to return the data inside the put request as a response, or do it in a second get request.

Code after:
      certified_by: certifiedBy || null,
      certification_details:
        certifiedBy && certificationDetails ? certificationDetails : null,
    };
    if (selectedOwners) {
      payload.owners = (
        selectedOwners as {
          value: number;
          label: string;
        }[]
      ).map(o => o.value);
    }
    if (isFeatureEnabled(FeatureFlag.TaggingSystem)) {
      payload.tags = tags.map(tag => tag.id);
    }

    try {
      const chartEndpoint = `/api/v1/chart/${slice.slice_id}`;
      let res = await SupersetClient.put({
        endpoint: chartEndpoint,
        headers: { 'Content-Type': 'application/json' },
        body: JSON.stringify(payload),
      });
      res = await SupersetClient.get({
        endpoint: chartEndpoint,
      });
      onSave(res.json.result);
      addSuccessToast(t('Chart properties updated'));
      onHide();
    } catch (res) {
      const clientError = await getClientErrorObject(res);
      showError(clientError);
    }
    setSubmitting(false);
  };

  const ownersLabel = t('Owners');

  // get the owners of this slice
  useEffect(() => {
    fetchChartProperties();
  }, [slice.slice_id]);

  // update name after it's changed in another modal
  useEffect(() => {
    setName(slice.slice_name || '');
  }, [slice.slice_name]);

  const handleChangeTags = (tags: { label: string; value: number }[]) => {
    const parsedTags: TagType[] = ensureIsArray(tags).map(r => ({
      id: r.value,
Repository: apache/superset
File: superset-frontend/src/components/Datasource/DatasourceEditor.jsx (JavaScript)

Code before:
        key={4}
      >
        <CollectionTable
          tableColumns={['name', 'config']}
          onChange={this.onDatasourcePropChange.bind(this, 'spatials')}
          itemGenerator={() => ({
            name: t('<new spatial>'),
            type: t('<no type>'),
            config: null,
          })}
          collection={spatials}
          allowDeletes
          itemRenderers={{
            name: (d, onChange) => (
              <EditableTitle canEdit title={d} onSaveTitle={onChange} />
            ),
            config: (v, onChange) => (
              <SpatialControl value={v} onChange={onChange} choices={allCols} />
            ),
          }}
        />
      </Tabs.TabPane>
    );
  }

  renderSqlEditotOverlay = () => (
    <div
      css={theme => css`
        position: absolute;
        background: ${theme.colors.secondary.light5};
        align-items: center;
        display: flex;
        height: 100%;
        width: 100%;
        justify-content: center;
      `}
    >
      <div>
        <Loading position="inline-centered" />
        <span
          css={theme => css`
            display: block;
            margin: ${theme.gridUnit * 4}px auto;
            width: fit-content;
            color: ${theme.colors.grayscale.base};
          `}
        >
          {t('We are working on your query')}
        </span>
      </div>
    </div>

Review comment (suggestion):
small typo
```suggestion
  renderSqlEditorOverlay = () => (
```


Code after:
          itemGenerator={() => ({
            name: t('<new spatial>'),
            type: t('<no type>'),
            config: null,
          })}
          collection={spatials}
          allowDeletes
          itemRenderers={{
            name: (d, onChange) => (
              <EditableTitle canEdit title={d} onSaveTitle={onChange} />
            ),
            config: (v, onChange) => (
              <SpatialControl value={v} onChange={onChange} choices={allCols} />
            ),
          }}
        />
      </Tabs.TabPane>
    );
  }

  renderSqlEditorOverlay = () => (
    <div
      css={theme => css`
        position: absolute;
        background: ${theme.colors.secondary.light5};
        align-items: center;
        display: flex;
        height: 100%;
        width: 100%;
        justify-content: center;
      `}
    >
      <div>
        <Loading position="inline-centered" />
        <span
          css={theme => css`
            display: block;
            margin: ${theme.gridUnit * 4}px auto;
            width: fit-content;
            color: ${theme.colors.grayscale.base};
          `}
        >
          {t('We are working on your query')}
        </span>
      </div>
    </div>
  );

  renderOpenInSqlLabLink(isError = false) {
    return (
      <a
Repository: apache/superset
File: superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts (TypeScript)

Code before:
      if (Array.isArray(series.value)) {
        const seriesName = String(series?.name || '');
        denormalizedSeriesValues[seriesName] = {};

        return {
          ...series,
          value: normalizeArray(series.value as number[], 10, seriesName),
        };
      }
      return series;
    });
  }

  const indicator = metricLabels.map(metricLabel => {
    if (isNormalised) {
      return {
        name: metricLabel,
        max: 1,
        min: 0,
      };
    }
    const maxValueInControl = columnConfig?.[metricLabel]?.radarMetricMaxValue;
    const minValueInControl = columnConfig?.[metricLabel]?.radarMetricMinValue;

    // Ensure that 0 is at the center of the polar coordinates
    const metricValueAsMax =
      metricLabelAndMaxValueMap.get(metricLabel) === 0
        ? Number.MAX_SAFE_INTEGER
        : metricLabelAndMaxValueMap.get(metricLabel);
    const max =
      maxValueInControl === null ? metricValueAsMax : maxValueInControl;

    let min: number;
    // If the min value doesn't exist, set it to 0 (default),
    // if it is null, set it to the min value of the data,
    // otherwise, use the value from the control
    if (minValueInControl === undefined) {
      min = 0;
    } else if (minValueInControl === null) {
      min = metricLabelAndMinValueMap.get(metricLabel) || 0;
    } else {
      min = minValueInControl;
    }

    return {
      name: metricLabel,
      max,
      min,
    };
  });


Review comment (suggestion):
If the values are normalised we consider min & max as 0 & 1 respectively & hence we dont need to calculate the rest as they are only in case when isNormalised is not true & some column config is provided

Code after:
      return normalizedValue;
    });

  // Normalize the transformed data
  const normalizedTransformedData = transformedData.map(series => {
    if (Array.isArray(series.value)) {
      const seriesName = String(series?.name || '');
      denormalizedSeriesValues[seriesName] = {};

      return {
        ...series,
        value: normalizeArray(series.value as number[], 10, seriesName),
      };
    }
    return series;
  });

  const indicator = metricLabels.map(metricLabel => {
    const isMetricWithCustomBounds = metricsWithCustomBounds.has(metricLabel);
    if (!isMetricWithCustomBounds) {
      return {
        name: metricLabel,
        max: 1,
        min: 0,
      };
    }
    const maxValueInControl = columnConfig?.[metricLabel]?.radarMetricMaxValue;
    const minValueInControl = columnConfig?.[metricLabel]?.radarMetricMinValue;

    // Ensure that 0 is at the center of the polar coordinates
    const maxValue =
      metricLabelAndMaxValueMap.get(metricLabel) === 0
        ? Number.MAX_SAFE_INTEGER
        : globalMax;
    const max = isDefined(maxValueInControl) ? maxValueInControl : maxValue;

    let min: number;
    if (isDefined(minValueInControl)) {
      min = minValueInControl;
    } else {
      min = 0;
    }

    return {
      name: metricLabel,
      max,
      min,
    };
  });

  const series: RadarSeriesOption[] = [
Repository: apache/superset
File: superset-frontend/src/components/Chart/Chart.tsx (TypeScript)

Code before:

const MessageSpan = styled.span`
  display: block;
  text-align: center;
  margin: ${({ theme }) => theme.gridUnit * 4}px auto;
  width: fit-content;
  color: ${({ theme }) => theme.colors.grayscale.base};
`;

class Chart extends PureComponent<ChartProps, {}> {
  static defaultProps = defaultProps;

  renderStartTime: any;

  constructor(props: ChartProps) {
    if (props.datasource?.metrics) {
      // eslint-disable-next-line no-param-reassign
      props.datasource.currencyFormats = Object.fromEntries(
        props.datasource.metrics
          .filter(
            (metric): metric is Metric & { currency: Currency } =>
              metric.currency !== undefined,
          )
          .map(metric => [metric.metric_name, metric.currency]),
      ) as Record<string, Currency>;
    }
    super(props);
    this.handleRenderContainerFailure =
      this.handleRenderContainerFailure.bind(this);
  }

  componentDidMount() {
    if (this.props.triggerQuery) {
      this.runQuery();
    }
  }

  componentDidUpdate() {
    if (this.props.triggerQuery) {
      this.runQuery();
    }
  }

  runQuery() {
    // Create chart with POST request
    this.props.actions.postChartFormData(
      this.props.formData,
      Boolean(this.props.force || getUrlParam(URL_PARAMS.force)), // allow override via url params force=true
      this.props.timeout,
      this.props.chartId,
      this.props.dashboardId,

Review comment (refactor):
I know that modifying `chartProps` here at this stage is not the most elegant solution, but I think it's the DRYer way. If we don't do it here, then in every `transformProps` we would have to add similar logic (or call a util that does this).

Anyways, open to feedback! There could be another option I couldn't find.

Code after:
  margin: ${({ theme }) => theme.gridUnit * 4}px auto;
  width: fit-content;
  color: ${({ theme }) => theme.colors.grayscale.base};
`;

class Chart extends PureComponent<ChartProps, {}> {
  static defaultProps = defaultProps;

  renderStartTime: any;

  constructor(props: ChartProps) {
    super(props);
    this.handleRenderContainerFailure =
      this.handleRenderContainerFailure.bind(this);
  }

  componentDidMount() {
    if (this.props.triggerQuery) {
      this.runQuery();
    }
  }

  componentDidUpdate() {
    if (this.props.triggerQuery) {
      this.runQuery();
    }
  }

  runQuery() {
    // Create chart with POST request
    this.props.actions.postChartFormData(
      this.props.formData,
      Boolean(this.props.force || getUrlParam(URL_PARAMS.force)), // allow override via url params force=true
      this.props.timeout,
      this.props.chartId,
      this.props.dashboardId,
      this.props.ownState,
    );
  }

  handleRenderContainerFailure(
    error: Error,
    info: { componentStack: string } | null,
  ) {
    const { actions, chartId } = this.props;
    logging.warn(error);
    actions.chartRenderingFailed(
      error.toString(),
      chartId,
      info ? info.componentStack : null,
    );
Repository: apache/superset
File: superset-frontend/src/components/CachedLabel/index.tsx (TypeScript)

Code before:
import { Tooltip } from 'src/components/Tooltip';
import { TooltipContent } from './TooltipContent';
import { Icons } from '../Icons';

export interface CacheLabelProps {
  onClick?: MouseEventHandler<HTMLElement>;
  cachedTimestamp?: string;
  className?: string;
}

const CacheLabel: FC<CacheLabelProps> = ({
  className,
  onClick,
  cachedTimestamp,
}) => {
  const [hovered, setHovered] = useState(false);

  const labelType = hovered ? 'primary' : 'default';
  return (
    <Tooltip
      title={<TooltipContent cachedTimestamp={cachedTimestamp} />}
      id="cache-desc-tooltip"
    >
      <Label
        className={`${className}`}
        css={{ gap: '2px' }}
        type={labelType}
        onClick={onClick}
        onMouseOver={() => setHovered(true)}
        onMouseOut={() => setHovered(false)}
      >
        {t('Cached')}
        <Icons.SyncOutlined iconSize="m" />
      </Label>
    </Tooltip>
  );
};

export default CacheLabel;


Review comment (suggestion):
```suggestion
        css={theme => css`
          gap: ${theme.gridUnit * 0.5}px;
        `}
```

Code after:
import { Tooltip } from 'src/components/Tooltip';
import { TooltipContent } from './TooltipContent';
import { Icons } from '../Icons';

export interface CacheLabelProps {
  onClick?: MouseEventHandler<HTMLElement>;
  cachedTimestamp?: string;
  className?: string;
}

const CacheLabel: FC<CacheLabelProps> = ({
  className,
  onClick,
  cachedTimestamp,
}) => {
  const [hovered, setHovered] = useState(false);

  const labelType = hovered ? 'primary' : 'default';
  return (
    <Tooltip
      title={<TooltipContent cachedTimestamp={cachedTimestamp} />}
      id="cache-desc-tooltip"
    >
      <Label
        className={`${className}`}
        css={theme => css`
          gap: ${theme.gridUnit * 0.5}px;
        `}
        type={labelType}
        onClick={onClick}
        onMouseOver={() => setHovered(true)}
        onMouseOut={() => setHovered(false)}
      >
        {t('Cached')}
        <Icons.SyncOutlined iconSize="m" />
      </Label>
    </Tooltip>
  );
};

export default CacheLabel;
Repository: apache/superset
File: superset-frontend/src/dashboard/components/CssEditor/index.tsx (TypeScript)

Code before:
            value: row.template_name,
            css: row.css,
            label: row.template_name,
          }),
        );

        this.setState({ templates });
      })
      .catch(() => {
        this.props.addDangerToast(
          t('An error occurred while fetching available CSS templates'),
        );
      });
  }

  changeCss(css: string) {
    this.setState({ css }, () => {
      this.props.onChange(css);
    });
  }

  changeCssTemplate(info: { key: Key }) {
    const templateName = String(info.key);
    const selectedTemplate = this.state.templates?.find(
      template => template.label === templateName,
    );
    if (selectedTemplate) {
      this.changeCss(selectedTemplate.css);
    }
  }

  renderTemplateSelector() {
    if (this.state.templates) {
      const menu = (
        <Menu
          onClick={this.changeCssTemplate}
          items={this.state.templates.map(template => ({
            key: template.label,
            label: template.label,
          }))}
        />
      );
      return (
        <Dropdown dropdownRender={() => menu} placement="bottomRight">
          <Button>{t('Load a CSS template')}</Button>
        </Dropdown>
      );
    }
    return null;
  }


Review comment (suggestion):
```suggestion
    const selectedTemplate = this.state.templates?.find(
      template => template.label === info.key,
    );
```

Code after:
            value: row.template_name,
            css: row.css,
            label: row.template_name,
          }),
        );

        this.setState({ templates });
      })
      .catch(() => {
        this.props.addDangerToast(
          t('An error occurred while fetching available CSS templates'),
        );
      });
  }

  changeCss(css: string) {
    this.setState({ css }, () => {
      this.props.onChange(css);
    });
  }

  changeCssTemplate(info: { key: Key }) {
    const selectedTemplate = this.state.templates?.find(
      template => template.label === info.key,
    );
    if (selectedTemplate) {
      this.changeCss(selectedTemplate.css);
    }
  }

  renderTemplateSelector() {
    if (this.state.templates) {
      const menu = (
        <Menu
          onClick={this.changeCssTemplate}
          items={this.state.templates.map(template => ({
            key: template.label,
            label: template.label,
          }))}
        />
      );
      return (
        <Dropdown dropdownRender={() => menu} placement="bottomRight">
          <Button>{t('Load a CSS template')}</Button>
        </Dropdown>
      );
    }
    return null;
  }

  render() {
Repository: apache/superset
File: superset-frontend/packages/superset-ui-core/src/utils/html.tsx (TypeScript)

Code before:
import { FilterXSS, getDefaultWhiteList } from 'xss';

const xssFilter = new FilterXSS({
  whiteList: {
    ...getDefaultWhiteList(),
    span: ['style', 'class', 'title'],
    div: ['style', 'class'],
    a: ['style', 'class', 'href', 'title', 'target'],
    img: ['style', 'class', 'src', 'alt', 'title', 'width', 'height'],
    video: [
      'autoplay',
      'controls',
      'loop',
      'preload',
      'src',
      'height',
      'width',
      'muted',
    ],
  },
  stripIgnoreTag: true,
  css: false,
});

export function sanitizeHtml(htmlString: string) {
  return xssFilter.process(htmlString);
}

export function hasHtmlTagPattern(str: string): boolean {
  const htmlTagPattern =
    /<(html|head|body|div|span|a|p|h[1-6]|title|meta|link|script|style)/i;

  return htmlTagPattern.test(str);
}

export function isProbablyHTML(text: string) {
  const cleanedStr = text.trim().toLowerCase();

  if (
    cleanedStr.startsWith('<!doctype html>') &&
    hasHtmlTagPattern(cleanedStr)
  ) {
    return true;
  }

  const parser = new DOMParser();
  const doc = parser.parseFromString(cleanedStr, 'text/html');
  return Array.from(doc.body.childNodes).some(({ nodeType }) => nodeType === 1);
}

export function sanitizeHtmlIfNeeded(htmlString: string) {

Review comment (security):
Not sure about allowing style tags here, can't we change the styles directly?
https://cheatsheetseries.owasp.org/cheatsheets/XSS_Filter_Evasion_Cheat_Sheet.html#style-attribute-that-breaks-up-an-expression

Code after:
import { FilterXSS, getDefaultWhiteList } from 'xss';

const xssFilter = new FilterXSS({
  whiteList: {
    ...getDefaultWhiteList(),
    span: ['style', 'class', 'title'],
    div: ['style', 'class'],
    a: ['style', 'class', 'href', 'title', 'target'],
    img: ['style', 'class', 'src', 'alt', 'title', 'width', 'height'],
    video: [
      'autoplay',
      'controls',
      'loop',
      'preload',
      'src',
      'height',
      'width',
      'muted',
    ],
    table: ['width', 'border', 'align', 'valign', 'style'],
    tr: ['rowspan', 'align', 'valign', 'style'],
    td: ['width', 'rowspan', 'colspan', 'align', 'valign', 'style'],
    th: ['width', 'rowspan', 'colspan', 'align', 'valign', 'style'],
    tbody: ['align', 'valign', 'style'],
    thead: ['align', 'valign', 'style'],
    tfoot: ['align', 'valign', 'style'],
  },
  stripIgnoreTag: true,
  css: false,
});

export function sanitizeHtml(htmlString: string) {
  return xssFilter.process(htmlString);
}

export function hasHtmlTagPattern(str: string): boolean {
  const htmlTagPattern =
    /<(html|head|body|div|span|a|p|h[1-6]|title|meta|link|script|style)/i;

  return htmlTagPattern.test(str);
}

export function isProbablyHTML(text: string) {
  const cleanedStr = text.trim().toLowerCase();

  if (
    cleanedStr.startsWith('<!doctype html>') &&
    hasHtmlTagPattern(cleanedStr)
  ) {
    return true;
  }
Repository: apache/superset
File: tests/unit_tests/db_engine_specs/test_doris.py (Python)

Code before:
            {"param1": "some_value"},
        ),
    ],
)
def test_adjust_engine_params(
    sqlalchemy_uri: str,
    connect_args: dict[str, Any],
    return_schema: str,
    return_connect_args: dict[str, Any],
) -> None:
    from superset.db_engine_specs.doris import DorisEngineSpec

    url = make_url(sqlalchemy_uri)
    returned_url, returned_connect_args = DorisEngineSpec.adjust_engine_params(
        url, connect_args
    )

    assert returned_url.database == return_schema
    assert returned_connect_args == return_connect_args


@pytest.mark.parametrize(
    "url,expected_schema",
    [
        ("doris://localhost:9030/hive.test", "test"),
        ("doris://localhost:9030/hive", None),
    ],
)
def test_get_schema_from_engine_params(
    url: str, expected_schema: Optional[str]
) -> None:
    """
    Test the ``get_schema_from_engine_params`` method.
    """
    from superset.db_engine_specs.doris import DorisEngineSpec

    assert (
        DorisEngineSpec.get_schema_from_engine_params(
            make_url(url),
            {},
        )
        == expected_schema
    )


@pytest.mark.parametrize(
    "database_value,expected_catalog",
    [
        ("catalog1.schema1", "catalog1"),
        ("catalog1", "catalog1"),
        (None, None),

Review comment (bug):
This is another test that was wrong. In Doris `hive` in this test would correspond to the schema, not to the catalog — the catalog is optional, not the schema.

Code after:
            "catalog1.db1",
            {"param1": "some_value"},
        ),
        (
            "pydoris://user:password@host/catalog1.db1",
            {"param1": "some_value"},
            None,
            None,
            "catalog1.db1",
            {"param1": "some_value"},
        ),
        (
            "pydoris://user:password@host/catalog1.db1",
            {"param1": "some_value"},
            "catalog2",
            None,
            "catalog2.db1",
            {"param1": "some_value"},
        ),
        (
            "pydoris://user:password@host/catalog1.db1",
            {"param1": "some_value"},
            None,
            "db2",
            "catalog1.db2",
            {"param1": "some_value"},
        ),
        (
            "pydoris://user:password@host/catalog1.db1",
            {"param1": "some_value"},
            "catalog2",
            "db2",
            "catalog2.db2",
            {"param1": "some_value"},
        ),
    ],
)
def test_adjust_engine_params(
    sqlalchemy_uri: str,
    connect_args: dict[str, Any],
    catalog: str | None,
    schema: str | None,
    return_schema: str,
    return_connect_args: dict[str, Any],
) -> None:
    from superset.db_engine_specs.doris import DorisEngineSpec

    url = make_url(sqlalchemy_uri)
    returned_url, returned_connect_args = DorisEngineSpec.adjust_engine_params(
        url,
        connect_args,