codeant-ai-for-open-source[bot] commented on code in PR #37902:
URL: https://github.com/apache/superset/pull/37902#discussion_r2844238188
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -2306,294 +2231,233 @@ class DatasourceEditor extends PureComponent<
/>
</div>
);
- }
+ }, [datasource, sortMetrics, onDatasourcePropChange, metricSearchTerm]);
- render() {
- const { datasource, activeTabKey } = this.state;
- const { metrics } = datasource;
- const sortedMetrics = metrics?.length ? this.sortMetrics(metrics) : [];
+ const sortedMetrics = useMemo(
+ () => (datasource.metrics?.length ? sortMetrics(datasource.metrics) : []),
+ [datasource.metrics, sortMetrics],
+ );
- return (
- <DatasourceContainer data-test="datasource-editor">
- {this.renderErrors()}
- <Alert
- css={theme => ({ marginBottom: theme.sizeUnit * 4 })}
- type="warning"
- message={
- <>
- {' '}
- <strong>{t('Be careful.')} </strong>
- {t(
- 'Changing these settings will affect all charts using this
dataset, including charts owned by other people.',
- )}
- </>
- }
- />
- <StyledTableTabs
- id="table-tabs"
- data-test="edit-dataset-tabs"
- onChange={this.handleTabSelect}
- defaultActiveKey={activeTabKey}
- items={[
- {
- key: TABS_KEYS.SOURCE,
- label: t('Source'),
- children: this.renderSourceFieldset(),
- },
- {
- key: TABS_KEYS.METRICS,
- label: (
- <CollectionTabTitle
- collection={sortedMetrics}
- title={t('Metrics')}
- />
- ),
- children: this.renderMetricCollection(),
- },
- {
- key: TABS_KEYS.COLUMNS,
- label: (
- <CollectionTabTitle
- collection={this.state.databaseColumns}
- title={t('Columns')}
- />
- ),
- children: (
- <StyledTableTabWrapper>
- {this.renderDefaultColumnSettings()}
- <ColumnButtonWrapper>
- <StyledButtonWrapper>
- <Button
- buttonSize="small"
- buttonStyle="tertiary"
- onClick={this.syncMetadata}
- className="sync-from-source"
- disabled={this.state.isEditMode}
- >
- <Icons.DatabaseOutlined iconSize="m" />
- {t('Sync columns from source')}
- </Button>
- </StyledButtonWrapper>
- </ColumnButtonWrapper>
- <Input.Search
- placeholder={t('Search columns by name')}
- value={this.state.columnSearchTerm}
- onChange={e =>
- this.setState({ columnSearchTerm: e.target.value })
- }
- style={{ marginBottom: 16, width: 300 }}
- allowClear
- />
- <ColumnCollectionTable
- className="columns-table"
- columns={this.state.databaseColumns}
- filterTerm={this.state.columnSearchTerm}
- filterFields={['column_name']}
- datasource={datasource}
- onColumnsChange={databaseColumns =>
- this.setColumns({ databaseColumns })
- }
- onDatasourceChange={this.onDatasourceChange}
- />
- {this.state.metadataLoading && <Loading />}
- </StyledTableTabWrapper>
- ),
- },
+ const tabItems = useMemo(
+ () => [
+ {
+ key: TABS_KEYS.SOURCE,
+ label: t('Source'),
+ children: renderSourceFieldset(),
+ },
+ {
+ key: TABS_KEYS.METRICS,
+ label: (
+ <CollectionTabTitle collection={sortedMetrics} title={t('Metrics')}
/>
+ ),
+ children: renderMetricCollection(),
+ },
+ {
+ key: TABS_KEYS.COLUMNS,
+ label: (
+ <CollectionTabTitle
+ collection={databaseColumns}
+ title={t('Columns')}
+ />
+ ),
+ children: (
+ <StyledTableTabWrapper>
+ {renderDefaultColumnSettings()}
+ <DefaultColumnSettingsTitle>
+ {t('Column Settings')}
+ </DefaultColumnSettingsTitle>
+ <ColumnButtonWrapper>
+ <StyledButtonWrapper>
+ <Button
+ buttonSize="small"
+ buttonStyle="tertiary"
+ onClick={syncMetadata}
+ className="sync-from-source"
+ disabled={isEditMode}
Review Comment:
**Suggestion:** The "Sync columns from source" button is disabled when edit
mode is enabled (`disabled={isEditMode}`), which inverts the intended behavior
and prevents users from syncing metadata at the only time they can actually
edit the dataset, effectively making the sync action unavailable during
editing. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Users cannot sync columns while dataset is in edit.
- ⚠️ Edit lock semantics inconsistent with other edit-only controls.
- ⚠️ Confusing UX around when metadata synchronization is allowed.
```
</details>
```suggestion
disabled={!isEditMode}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any dataset for editing via the dataset modal, which renders
`DatasourceEditor`
inside `DatasourceModal` at
`superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:33-40`
(verified
with Read).
2. In `DatasourceEditor`, note that edit locking is controlled by
`isEditMode` in
`renderSourceFieldset` at `DatasourceEditor.tsx:1719-1750`, where the lock
icon toggles
`isEditMode` via `onChangeEditMode`.
3. Switch to the "Columns" tab, whose content is defined in the `tabItems`
useMemo at
`DatasourceEditor.tsx:2241-2290`; observe the "Sync columns from source"
button rendered
at `DatasourceEditor.tsx:2271-2279` with `disabled={isEditMode}`.
4. Click the lock icon to enable edit mode (`isEditMode` becomes true,
enabling other
editing controls like the datasource type radios at
`DatasourceEditor.tsx:1719-1727`), and
observe that the "Sync columns from source" button is now disabled due to
`disabled={isEditMode}`, preventing metadata sync at precisely the time when
the user is
allowed to edit dataset columns.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
**Line:** 2276:2276
**Comment:**
*Logic Error: The "Sync columns from source" button is disabled when
edit mode is enabled (`disabled={isEditMode}`), which inverts the intended
behavior and prevents users from syncing metadata at the only time they can
actually edit the dataset, effectively making the sync action unavailable
during editing.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=9dded7dffd05467a6569b98d55c2d92b220e555c4f34d24f191a6e50f1c56eb6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=9dded7dffd05467a6569b98d55c2d92b220e555c4f34d24f191a6e50f1c56eb6&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/SelectControl.tsx:
##########
@@ -162,93 +149,120 @@ export const innerGetOptions = (props:
SelectControlProps): SelectOption[] => {
return { value: c as unknown as string | number, label: String(c) };
});
}
- return options;
+ return selectOptions;
};
-export default class SelectControl extends PureComponent<
- SelectControlProps,
- SelectControlState
-> {
- static defaultProps = defaultProps;
+function SelectControl({
+ ariaLabel,
+ autoFocus = false,
+ choices = [],
+ clearable = true,
+ description = null,
+ disabled = false,
+ freeForm = false,
+ isLoading = false,
+ mode,
+ multi = false,
+ isMulti,
+ name,
+ onChange = () => {},
+ onFocus = () => {},
+ onSelect,
+ onDeselect,
+ value,
+ default: defaultValue,
+ showHeader = true,
+ optionRenderer,
+ valueKey = 'value',
+ options: optionsProp,
+ placeholder,
+ filterOption,
+ tokenSeparators,
+ notFoundContent,
+ label = undefined,
+ renderTrigger,
+ validationErrors,
+ rightNode,
+ leftNode,
+ onClick,
+ hovered,
+ tooltipOnClick,
+ warning,
+ danger,
+ sortComparator,
+}: SelectControlProps) {
+ const [options, setOptions] = useState<SelectOption[]>(() =>
+ innerGetOptions({
+ choices,
+ optionRenderer,
+ valueKey,
+ options: optionsProp,
+ name,
+ }),
+ );
- constructor(props: SelectControlProps) {
- super(props);
- this.state = {
- options: this.getOptions(props),
- };
- this.onChange = this.onChange.bind(this);
- this.handleFilterOptions = this.handleFilterOptions.bind(this);
- }
+ // Track previous choices/options for comparison
+ const prevChoicesRef = useRef(choices);
+ const prevOptionsRef = useRef(optionsProp);
- componentDidUpdate(prevProps: SelectControlProps) {
+ useEffect(() => {
if (
- !isEqualArray(this.props.choices, prevProps.choices) ||
- !isEqualArray(this.props.options, prevProps.options)
+ !isEqualArray(choices, prevChoicesRef.current) ||
+ !isEqualArray(optionsProp, prevOptionsRef.current)
) {
- const options = this.getOptions(this.props);
- this.setState({ options });
+ const newOptions = innerGetOptions({
+ choices,
+ optionRenderer,
+ valueKey,
+ options: optionsProp,
+ name,
+ });
+ setOptions(newOptions);
+ prevChoicesRef.current = choices;
+ prevOptionsRef.current = optionsProp;
}
- }
+ }, [choices, optionsProp, optionRenderer, valueKey, name]);
// Beware: This is acting like an on-click instead of an on-change
// (firing every time user chooses vs firing only if a new option is chosen).
- onChange(val: SelectValue | SelectOption | SelectOption[]) {
- // will eventually call `exploreReducer`: SET_FIELD_VALUE
- const { valueKey = 'value' } = this.props;
- let onChangeVal: SelectValue = val as SelectValue;
+ const handleChange = useCallback(
+ (val: SelectValue | SelectOption | SelectOption[]) => {
+ // will eventually call `exploreReducer`: SET_FIELD_VALUE
+ let onChangeVal: SelectValue = val as SelectValue;
- if (Array.isArray(val)) {
- const values = val.map(v =>
- typeof v === 'object' &&
- v !== null &&
- (v as SelectOption)[valueKey] !== undefined
- ? (v as SelectOption)[valueKey]
- : v,
- );
- onChangeVal = values as (string | number)[];
- }
- if (
- typeof val === 'object' &&
- val !== null &&
- !Array.isArray(val) &&
- (val as SelectOption)[valueKey] !== undefined
- ) {
- onChangeVal = (val as SelectOption)[valueKey] as string | number;
- }
- this.props.onChange?.(onChangeVal, []);
- }
-
- getOptions(props: SelectControlProps) {
- return innerGetOptions(props);
- }
+ if (Array.isArray(val)) {
+ const values = val.map(v =>
+ typeof v === 'object' &&
+ v !== null &&
+ (v as SelectOption)[valueKey] !== undefined
+ ? (v as SelectOption)[valueKey]
+ : v,
+ );
+ onChangeVal = values as (string | number)[];
+ }
+ if (
+ typeof val === 'object' &&
+ val !== null &&
+ !Array.isArray(val) &&
+ (val as SelectOption)[valueKey] !== undefined
+ ) {
+ onChangeVal = (val as SelectOption)[valueKey] as string | number;
+ }
+ onChange?.(onChangeVal, []);
+ },
+ [onChange, valueKey],
+ );
- handleFilterOptions(text: string, option: SelectOption) {
- const { filterOption } = this.props;
- return filterOption?.({ data: option }, text) ?? true;
- }
+ const handleFilterOptions = useCallback(
+ (text: string, option: SelectOption) =>
+ filterOption?.({ data: option }, text) ?? true,
Review Comment:
**Suggestion:** The `handleFilterOptions` adapter passes arguments to the
user-provided `filterOption` callback in the wrong order, so callers that
expect `(input, option)` (where `input` is the search text and `option` is the
option object) will instead receive the option object as the first argument and
the search text as the second, which can break custom filters (e.g., calling
string methods on a non-string) and cause incorrect or failing option
filtering. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Custom-filtered Explore selects can throw runtime errors.
- ⚠️ Search behavior in affected controls becomes incorrect or inconsistent.
```
</details>
```suggestion
filterOption?.(text, { data: option }) ?? true,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In any test or storybook, render `SelectControl` from
`superset-frontend/src/explore/components/controls/SelectControl.tsx`
(function component
declared at lines 155–193 in that file) with a custom `filterOption` prop
implemented as
`(input: string, option: { data: any }) =>
input.toLowerCase().includes('foo')`.
2. Ensure `SelectControl` is given a non-empty `choices` or `options` prop
so that
`innerGetOptions` (lines 127–153) produces at least one `SelectOption`, and
the internal
`<Select>` from `@superset-ui/core/components` renders options with search
enabled
(default behavior in `Select.tsx` at lines 96–118 and 754–784).
3. Type any text into the select's search box in the rendered UI; the
underlying `Select`
component calls the `filterOption` function passed in `selectProps` (built in
`SelectControl` at lines 309–363) with arguments `(search: string, option:
SelectOption)`,
which are then forwarded to `handleFilterOptions` (lines 256–260).
4. Observe that `handleFilterOptions` currently calls the user-provided
`filterOption` as
`filterOption?.({ data: option }, text)`, so the first parameter is the
option object and
the second is the search string, contrary to the `SelectControlProps` type
`filterOption?:
(input: unknown, option: unknown) => boolean;` on line 65; this leads to
runtime errors
(e.g., attempting `input.toLowerCase()` on a non-string) or incorrect
filtering behavior
when custom `filterOption` logic assumes the documented `(input, option)`
order.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/SelectControl.tsx
**Line:** 258:258
**Comment:**
*Logic Error: The `handleFilterOptions` adapter passes arguments to the
user-provided `filterOption` callback in the wrong order, so callers that
expect `(input, option)` (where `input` is the search text and `option` is the
option object) will instead receive the option object as the first argument and
the search text as the second, which can break custom filters (e.g., calling
string methods on a non-string) and cause incorrect or failing option filtering.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=b7fcf0c1101f12199b1c5bbe71ba19ef6610b069cfe18b162c825ced942f65e6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=b7fcf0c1101f12199b1c5bbe71ba19ef6610b069cfe18b162c825ced942f65e6&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:
##########
@@ -32,279 +32,310 @@ export interface DataEntry {
}
interface TTestTableProps {
- alpha: number;
+ alpha?: number;
data: DataEntry[];
groups: string[];
- liftValPrec: number;
+ liftValPrec?: number;
metric: string;
- pValPrec: number;
+ pValPrec?: number;
}
-interface TTestTableState {
- control: number;
- liftValues: (string | number)[];
- pValues: (string | number)[];
-}
+function TTestTable({
+ alpha = 0.05,
+ data,
+ groups,
+ liftValPrec = 4,
+ metric,
+ pValPrec = 6,
+}: TTestTableProps) {
+ const [control, setControl] = useState(0);
+ const [liftValues, setLiftValues] = useState<(string | number)[]>([]);
+ const [pValues, setPValues] = useState<(string | number)[]>([]);
-const defaultProps = {
- alpha: 0.05,
- liftValPrec: 4,
- pValPrec: 6,
-};
+ const computeLift = useCallback(
+ (values: DataPointValue[], controlValues: DataPointValue[]): string => {
+ // Compute the lift value between two time series
+ let sumValues = 0;
+ let sumControl = 0;
+ values.forEach((value, i) => {
+ sumValues += value.y;
+ sumControl += controlValues[i].y;
+ });
-class TTestTable extends Component<TTestTableProps, TTestTableState> {
- static defaultProps = defaultProps;
+ return (((sumValues - sumControl) / sumControl) * 100).toFixed(
Review Comment:
**Suggestion:** The `computeLift` function divides by `sumControl` without
guarding for the case where the control sum is 0, which will produce `Infinity`
or `-Infinity` and then cause `toFixed` to throw a `RangeError` at runtime;
this will break rendering when the control group has a total of zero. Add a
zero-check to return a safe placeholder (e.g. `'NaN'`) when `sumControl` is 0
so the rest of the component can handle it as an invalid value. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Paired T-test legacy chart can crash on zero-control data.
- ❌ Dashboards using Paired T-test may fail to render.
- ⚠️ Explore view breaks when querying sparse control-group metrics.
- ⚠️ Reports/alerts using this viz may error on render.
```
</details>
```suggestion
// Avoid division by zero, which would produce Infinity and make
toFixed throw
if (sumControl === 0) {
return 'NaN';
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the Superset UI, create or open a chart using the "Paired TTest"
visualization type
(`VizType.PairedTTest = 'paired_ttest'` defined in
`superset-frontend/packages/superset-ui-core/src/chart/types/VizType.ts:45`
and registered
in `superset-frontend/src/visualizations/presets/MainPreset.ts:30,135` via
`PairedTTestChartPlugin`).
2. Configure the chart so that the query returns, for at least one metric, a
control group
whose time series has all `y` values equal to `0` (e.g., no events in the
control group),
producing `DataEntry[]` with `values` arrays where every
`controlValues[i].y` is `0`. This
data shape is consumed by `PairedTTest` in
`superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/PairedTTest.tsx:23-31,109-132`,
which passes `data[metric]` into `<TTestTable ... />`.
3. When the Paired T-test chart renders, `TTestTable` mounts
(`superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:43-50`),
triggering its `useEffect` at lines `137-153` which calls
`computeTTest(control)`
(`108-135`), and in turn calls `computeLift(data[i].values,
data[controlIndex].values)`
(`117-127` and `55-70`) for each row.
4. For the row where the selected control group's `values` all have `y ===
0`,
`computeLift` computes `sumControl = 0` and then executes `(((sumValues -
sumControl) /
sumControl) * 100).toFixed(liftValPrec)` (`65-67`), which attempts `toFixed`
on an
infinite/invalid number and throws a `RangeError`, causing the React
component for the
Paired T-test chart to error and fail to render in dashboards, Explore, or
reports when
that data condition occurs.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx
**Line:** 65:65
**Comment:**
*Logic Error: The `computeLift` function divides by `sumControl`
without guarding for the case where the control sum is 0, which will produce
`Infinity` or `-Infinity` and then cause `toFixed` to throw a `RangeError` at
runtime; this will break rendering when the control group has a total of zero.
Add a zero-check to return a safe placeholder (e.g. `'NaN'`) when `sumControl`
is 0 so the rest of the component can handle it as an invalid value.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=e3aeafa0bf8219dff264e86a352e59240057ab1eb1fd0ae84b7a37c0b6f7c4cd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=e3aeafa0bf8219dff264e86a352e59240057ab1eb1fd0ae84b7a37c0b6f7c4cd&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
