codeant-ai-for-open-source[bot] commented on code in PR #39452:
URL: https://github.com/apache/superset/pull/39452#discussion_r3532995719
##########
superset-frontend/plugins/legacy-plugin-chart-horizon/src/HorizonRow.tsx:
##########
@@ -52,162 +50,140 @@ interface HorizonRowProps {
yDomain?: [number, number];
}
-const defaultProps: Partial<HorizonRowProps> = {
- className: '',
- width: 800,
- height: 20,
- bands: DEFAULT_COLORS.length >> 1,
- colors: DEFAULT_COLORS,
- colorScale: 'series',
- mode: 'offset',
- offsetX: 0,
- title: '',
- yDomain: undefined,
-};
-
-class HorizonRow extends PureComponent<HorizonRowProps> {
- static defaultProps = defaultProps;
-
- private canvas: HTMLCanvasElement | null = null;
-
- componentDidMount() {
- this.drawChart();
- }
-
- componentDidUpdate() {
- this.drawChart();
- }
-
- componentWillUnmount() {
- this.canvas = null;
- }
-
- drawChart() {
- if (this.canvas) {
- const {
- data: rawData,
- yDomain,
- width = 800,
- height = 20,
- bands = DEFAULT_COLORS.length >> 1,
- colors = DEFAULT_COLORS,
- colorScale,
- offsetX = 0,
- mode,
- } = this.props;
-
- const data =
- colorScale === 'change'
- ? rawData.map(d => ({ ...d, y: d.y - rawData[0].y }))
- : rawData;
-
- const context = this.canvas.getContext('2d');
- if (!context) return;
- context.imageSmoothingEnabled = false;
- context.clearRect(0, 0, width, height);
- // Reset transform
- context.setTransform(1, 0, 0, 1, 0, 0);
- context.translate(0.5, 0.5);
-
- const step = width / data.length;
- // the data frame currently being shown:
- const startIndex = Math.floor(Math.max(0, -(offsetX / step)));
- const endIndex = Math.floor(
- Math.min(data.length, startIndex + width / step),
- );
-
- // skip drawing if there's no data to be drawn
- if (startIndex > data.length) {
- return;
+function HorizonRow({
+ className = '',
+ width = 800,
+ height = 20,
+ data: rawData,
+ bands = DEFAULT_COLORS.length >> 1,
+ colors = DEFAULT_COLORS,
+ colorScale = 'series',
+ mode = 'offset',
+ offsetX = 0,
+ title = '',
+ yDomain,
+}: HorizonRowProps) {
+ const canvasRef = useRef<HTMLCanvasElement | null>(null);
+
+ const drawChart = useCallback(() => {
+ const canvas = canvasRef.current;
+ if (!canvas) return;
+
+ const data =
+ colorScale === 'change' && rawData.length > 0
+ ? rawData.map(d => ({ ...d, y: d.y - rawData[0].y }))
+ : rawData;
+
+ const context = canvas.getContext('2d');
+ if (!context) return;
+ context.imageSmoothingEnabled = false;
+ context.clearRect(0, 0, width, height);
+ // Reset transform
+ context.setTransform(1, 0, 0, 1, 0, 0);
+ context.translate(0.5, 0.5);
+
+ const step = width / data.length;
+ // the data frame currently being shown:
+ const startIndex = Math.floor(Math.max(0, -(offsetX / step)));
+ const endIndex = Math.floor(
+ Math.min(data.length, startIndex + width / step),
+ );
+
+ // skip drawing if there's no data to be drawn
+ if (startIndex > data.length) {
+ return;
+ }
+
+ // Create y-scale
+ const [min, max] =
+ yDomain || (d3Extent(data, d => d.y) as [number, number]);
+ const y = scaleLinear()
+ .domain([0, Math.max(-min, max)])
+ .range([0, height]);
Review Comment:
**Suggestion:** The draw path does not short-circuit when the dataset is
empty, so it computes `width / data.length` and then builds a y-domain from an
empty extent. That produces invalid numeric values (`Infinity`/`NaN`) and runs
the renderer with a broken scale. Add an early return when `data.length === 0`
before computing `step` and the extent. [logic error]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Horizon charts with empty series compute NaN y-scale domains.
- ⚠️ Internal Infinity/NaN state risks future changes causing errors.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/plugins/legacy-plugin-chart-horizon/src/HorizonChart.tsx:70-81`,
render `HorizonChart` with a `data` array where one `DataSeries` has
`values: []` (an
empty time series), e.g. `[{ key: ['series A'], values: [] }]`.
2. `HorizonChart` maps each series to a `HorizonRow` at
`HorizonChart.tsx:103-116`,
passing the empty `values` array as the `data` prop to `HorizonRow`
(`data={row.values}`).
3. Inside `HorizonRow` at `HorizonRow.tsx:53-76`, the `useEffect` hook
(`HorizonRow.tsx:177-179`) invokes `drawChart`, where `data` is the empty
array and `const
step = width / data.length;` at `HorizonRow.tsx:85` evaluates to `Infinity`.
4. Still in `drawChart`, `d3Extent(data, d => d.y)` over the empty `data`
array at
`HorizonRow.tsx:98-99` returns `[undefined, undefined]`, making
`Math.max(-min, max)` at
`HorizonRow.tsx:101` evaluate to `NaN` and producing a y-scale with domain
`[0, NaN]` at
`HorizonRow.tsx:100-102`, confirming that empty datasets put the renderer
into an invalid
numeric state even though no bands are ultimately drawn.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e99d285102094a2889cf559404d00028&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e99d285102094a2889cf559404d00028&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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-horizon/src/HorizonRow.tsx
**Line:** 85:102
**Comment:**
*Logic Error: The draw path does not short-circuit when the dataset is
empty, so it computes `width / data.length` and then builds a y-domain from an
empty extent. That produces invalid numeric values (`Infinity`/`NaN`) and runs
the renderer with a broken scale. Add an early return when `data.length === 0`
before computing `step` and the extent.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=92c5d564130394b05dc17240e73573fe96d20940022e821fb9617e722076d7f7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=92c5d564130394b05dc17240e73573fe96d20940022e821fb9617e722076d7f7&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/legacy-plugin-chart-horizon/test/HorizonRow.test.tsx:
##########
@@ -0,0 +1,102 @@
+/**
+ * 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.
+ */
+import { render, screen } from '@testing-library/react';
+import '@testing-library/jest-dom';
+import HorizonRow from '../src/HorizonRow';
+
+const mockContext = {
+ imageSmoothingEnabled: false,
+ fillStyle: '',
+ clearRect: jest.fn(),
+ setTransform: jest.fn(),
+ translate: jest.fn(),
+ scale: jest.fn(),
+ fillRect: jest.fn(),
+};
+
+beforeAll(() => {
+ jest
+ .spyOn(HTMLCanvasElement.prototype, 'getContext')
+ .mockReturnValue(mockContext as unknown as CanvasRenderingContext2D);
+});
Review Comment:
**Suggestion:** The suite replaces `HTMLCanvasElement.prototype.getContext`
in `beforeAll` but never restores it, so this mock leaks into other test files
and can hide real canvas behavior or break unrelated tests. Restore the spy in
an `afterAll` hook. [missing cleanup]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Canvas mocking in HorizonRow tests breaks EditableTitle rendering tests.
- ⚠️ Global prototype spy causes flaky cross-suite Jest behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/plugins/legacy-plugin-chart-horizon/test/HorizonRow.test.tsx:33-37`,
the `beforeAll` hook calls `jest.spyOn(HTMLCanvasElement.prototype,
'getContext')` and
`.mockReturnValue(mockContext as unknown as CanvasRenderingContext2D)`,
permanently
replacing the global `getContext` implementation for all canvases in the
Jest worker
process.
2. This suite only calls `jest.clearAllMocks()` in `beforeEach`
(`HorizonRow.test.tsx:39-41`), which resets mock call history but does not
restore
`HTMLCanvasElement.prototype.getContext` to its original implementation, and
there is no
`afterAll`/global `jest.restoreAllMocks()` configuration (verified by
searching Jest
config files for `restoreMocks: true` and `resetMocks: true` with no
matches).
3. In another test file, such as
`superset-frontend/packages/superset-ui-core/src/components/EditableTitle/index.tsx:19-25`,
the `EditableTitle` component’s `measureTextWidth` helper creates a canvas
and calls
`canvas.getContext('2d')`, then uses `context.font` and
`context.measureText(text).width`
to size the input.
4. When `EditableTitle` is rendered in its tests (e.g.
`EditableTitle.test.tsx:33-39` and
`Header.test.tsx:109-114`), `canvas.getContext` now returns the
`mockContext` from
`HorizonRow.test.tsx` (which lacks `font` and `measureText`), so the call to
`context.measureText` throws a TypeError; restoring the spy in an `afterAll`
hook (or
using `jest.restoreAllMocks`) confines the mock to this suite and prevents
such cross-test
failures.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=06ecf7b1f4984fed903225c9c1fbd144&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=06ecf7b1f4984fed903225c9c1fbd144&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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-horizon/test/HorizonRow.test.tsx
**Line:** 33:37
**Comment:**
*Missing Cleanup: The suite replaces
`HTMLCanvasElement.prototype.getContext` in `beforeAll` but never restores it,
so this mock leaks into other test files and can hide real canvas behavior or
break unrelated tests. Restore the spy in an `afterAll` hook.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=7ba19a30fc2d0898fdedd6ece1a4fa8e672111c201ed4e889966e9ed5f125122&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=7ba19a30fc2d0898fdedd6ece1a4fa8e672111c201ed4e889966e9ed5f125122&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:
##########
@@ -32,279 +32,324 @@ 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)[];
-}
-
-const defaultProps = {
- alpha: 0.05,
- liftValPrec: 4,
- pValPrec: 6,
-};
+function TTestTable({
+ alpha = 0.05,
+ data,
+ groups,
+ liftValPrec = 4,
+ metric,
+ pValPrec = 6,
+}: TTestTableProps) {
+ const [control, setControl] = useState(0);
+ // Mirrors `control` so the data-change effect can read the latest value
+ // without re-running (and recomputing) after every row click
+ const controlRef = useRef(0);
+ const [liftValues, setLiftValues] = useState<(string | number)[]>([]);
+ const [pValues, setPValues] = useState<(string | number)[]>([]);
-class TTestTable extends Component<TTestTableProps, TTestTableState> {
- static defaultProps = defaultProps;
-
- constructor(props: TTestTableProps) {
- super(props);
- this.state = {
- control: 0,
- liftValues: [],
- pValues: [],
- };
- }
+ 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;
+ });
- componentDidMount() {
- const { control } = this.state;
- this.computeTTest(control); // initially populate table
- }
-
- getLiftStatus(row: number): string {
- const { control, liftValues } = this.state;
- // Get a css class name for coloring
- if (row === control) {
- return 'control';
- }
- const liftVal = liftValues[row];
- if (Number.isNaN(liftVal) || !Number.isFinite(liftVal)) {
- return 'invalid'; // infinite or NaN values
- }
-
- return Number(liftVal) >= 0 ? 'true' : 'false'; // green on true, red on
false
- }
+ // A zero control sum yields "Infinity" (or "NaN" for 0/0) via toFixed,
+ // matching the original class component's output
+ return (((sumValues - sumControl) / sumControl) * 100).toFixed(
+ liftValPrec,
+ );
+ },
+ [liftValPrec],
+ );
- getPValueStatus(row: number): string {
- const { control, pValues } = this.state;
- if (row === control) {
- return 'control';
- }
- const pVal = pValues[row];
- if (Number.isNaN(pVal) || !Number.isFinite(pVal)) {
- return 'invalid';
- }
+ const computePValue = useCallback(
+ (
+ values: DataPointValue[],
+ controlValues: DataPointValue[],
+ ): string | number => {
+ // Compute the p-value from Student's t-test
+ // between two time series
+ let diffSum = 0;
+ let diffSqSum = 0;
+ let finiteCount = 0;
+ values.forEach((value, i) => {
+ const diff = controlValues[i].y - value.y;
+ /* eslint-disable-next-line */
+ if (isFinite(diff)) {
+ finiteCount += 1;
+ diffSum += diff;
+ diffSqSum += diff * diff;
+ }
+ });
+ const tvalue = -Math.abs(
+ diffSum *
+ Math.sqrt(
+ (finiteCount - 1) / (finiteCount * diffSqSum - diffSum * diffSum),
+ ),
+ );
+ try {
+ return (2 * new dist.Studentt(finiteCount - 1).cdf(tvalue)).toFixed(
+ pValPrec,
+ ); // two-sided test
+ } catch (error) {
+ return NaN;
+ }
+ },
+ [pValPrec],
+ );
- return ''; // p-values won't normally be colored
- }
+ const computeTTest = useCallback(
+ (controlIndex: number) => {
+ // Compute lift and p-values for each row
+ // against the selected control
+ const newPValues: (string | number)[] = [];
+ const newLiftValues: (string | number)[] = [];
+ if (!data) {
+ return;
+ }
+ for (let i = 0; i < data.length; i += 1) {
+ if (i === controlIndex) {
+ newPValues.push('control');
+ newLiftValues.push('control');
+ } else {
+ newPValues.push(
+ computePValue(data[i].values, data[controlIndex].values),
+ );
+ newLiftValues.push(
+ computeLift(data[i].values, data[controlIndex].values),
+ );
+ }
+ }
+ setControl(controlIndex);
+ controlRef.current = controlIndex;
+ setLiftValues(newLiftValues);
+ setPValues(newPValues);
+ },
+ [data, computeLift, computePValue],
+ );
- getSignificance(row: number): string | boolean {
- const { control, pValues } = this.state;
- const { alpha } = this.props;
- // Color significant as green, else red
- if (row === control) {
- return 'control';
+ // Recompute table when data changes, keeping control index in range.
+ // Row clicks call computeTTest directly, so `control` is read via a ref
+ // here to avoid a duplicate recompute after each click.
+ useEffect(() => {
+ if (!data || data.length === 0) {
+ setControl(0);
+ controlRef.current = 0;
+ setLiftValues([]);
+ setPValues([]);
+ return;
}
- // p-values significant below set threshold
- return Number(pValues[row]) <= alpha;
- }
+ computeTTest(Math.min(controlRef.current, data.length - 1));
+ }, [computeTTest, data]);
- computeLift(values: DataPointValue[], control: DataPointValue[]): string {
- const { liftValPrec } = this.props;
- // Compute the lift value between two time series
- let sumValues = 0;
- let sumControl = 0;
- values.forEach((value, i) => {
- sumValues += value.y;
- sumControl += control[i].y;
- });
+ const getLiftStatus = useCallback(
+ (row: number): string => {
+ // Get a css class name for coloring
+ if (row === control) {
+ return 'control';
+ }
+ const liftVal = liftValues[row];
+ const numericLiftVal = Number(liftVal);
+ if (Number.isNaN(numericLiftVal) || !Number.isFinite(numericLiftVal)) {
+ return 'invalid'; // infinite or NaN values
+ }
- return (((sumValues - sumControl) / sumControl) *
100).toFixed(liftValPrec);
- }
+ return numericLiftVal >= 0 ? 'true' : 'false'; // green on true, red on
false
+ },
+ [control, liftValues],
+ );
- computePValue(
- values: DataPointValue[],
- control: DataPointValue[],
- ): string | number {
- const { pValPrec } = this.props;
- // Compute the p-value from Student's t-test
- // between two time series
- let diffSum = 0;
- let diffSqSum = 0;
- let finiteCount = 0;
- values.forEach((value, i) => {
- const diff = control[i].y - value.y;
- /* eslint-disable-next-line */
- if (isFinite(diff)) {
- finiteCount += 1;
- diffSum += diff;
- diffSqSum += diff * diff;
+ const getPValueStatus = useCallback(
+ (row: number): string => {
+ if (row === control) {
+ return 'control';
+ }
+ const pVal = pValues[row];
+ const numericPVal = Number(pVal);
+ if (Number.isNaN(numericPVal) || !Number.isFinite(numericPVal)) {
+ return 'invalid';
}
- });
- const tvalue = -Math.abs(
- diffSum *
- Math.sqrt(
- (finiteCount - 1) / (finiteCount * diffSqSum - diffSum * diffSum),
- ),
- );
- try {
- return (2 * new dist.Studentt(finiteCount - 1).cdf(tvalue)).toFixed(
- pValPrec,
- ); // two-sided test
- } catch (error) {
- return NaN;
- }
- }
- computeTTest(control: number) {
- // Compute lift and p-values for each row
- // against the selected control
- const { data } = this.props;
- const pValues: (string | number)[] = [];
- const liftValues: (string | number)[] = [];
- if (!data) {
- return;
- }
- for (let i = 0; i < data.length; i += 1) {
- if (i === control) {
- pValues.push('control');
- liftValues.push('control');
- } else {
- pValues.push(this.computePValue(data[i].values, data[control].values));
- liftValues.push(this.computeLift(data[i].values,
data[control].values));
+ return ''; // p-values won't normally be colored
+ },
+ [control, pValues],
+ );
+
+ const getSignificance = useCallback(
+ (row: number): string | boolean => {
+ // Color significant as green, else red
+ if (row === control) {
+ return 'control';
}
- }
- this.setState({ control, liftValues, pValues });
- }
- render() {
- const { data, metric, groups } = this.props;
- const { control, liftValues, pValues } = this.state;
+ // p-values significant below set threshold
+ return Number(pValues[row]) <= alpha;
+ },
+ [control, pValues, alpha],
+ );
- if (!Array.isArray(groups) || groups.length === 0) {
- throw new Error('Group by param is required');
- }
+ const handleRowClick = useCallback(
+ (rowIndex: number) => {
+ computeTTest(rowIndex);
+ },
+ [computeTTest],
+ );
- // Render column header for each group
- const columns = groups.map((group, i) => (
- <Th key={i} column={group}>
- {group}
- </Th>
- ));
- const numGroups = groups.length;
- // Columns for p-value, lift-value, and significance (true/false)
- columns.push(
- <Th key={numGroups + 1} column="pValue">
- p-value
- </Th>,
- );
- columns.push(
- <Th key={numGroups + 2} column="liftValue">
- Lift %
- </Th>,
- );
- columns.push(
- <Th key={numGroups + 3} column="significant">
- Significant
- </Th>,
- );
- const rows = data.map((entry, i) => {
- const values = groups.map(
- (
- group,
- j, // group names
- ) => <Td key={j} column={group} data={entry.group[j]} />,
- );
- values.push(
- <Td
- key={numGroups + 1}
- className={this.getPValueStatus(i)}
- column="pValue"
- data={pValues[i]}
- />,
- );
- values.push(
- <Td
- key={numGroups + 2}
- className={this.getLiftStatus(i)}
- column="liftValue"
- data={liftValues[i]}
- />,
- );
- values.push(
- <Td
- key={numGroups + 3}
- className={this.getSignificance(i).toString()}
- column="significant"
- data={this.getSignificance(i)}
- />,
- );
+ // When sorted ascending, 'control' will always be at top
+ type SortConfigItem =
+ string | { column: string; sortFunction: (a: string, b: string) => number
};
- return (
- <Tr
- key={i}
- className={i === control ? 'control' : ''}
- onClick={this.computeTTest.bind(this, i)}
- >
- {values}
- </Tr>
- );
- });
- // When sorted ascending, 'control' will always be at top
- type SortConfigItem =
- | string
- | { column: string; sortFunction: (a: string, b: string) => number };
- const sortConfig: SortConfigItem[] = (groups as SortConfigItem[]).concat([
- {
- column: 'pValue',
- sortFunction: (a: string, b: string) => {
- if (a === 'control') {
- return -1;
- }
- if (b === 'control') {
- return 1;
- }
+ const sortConfig: SortConfigItem[] = useMemo(
+ () =>
+ (groups as SortConfigItem[]).concat([
+ {
+ column: 'pValue',
+ sortFunction: (a: string, b: string) => {
+ if (a === 'control') {
+ return -1;
+ }
+ if (b === 'control') {
+ return 1;
+ }
+ if (a === b) {
+ return 0;
+ }
- return a > b ? 1 : -1; // p-values ascending
+ return a > b ? 1 : -1; // p-values ascending
+ },
},
- },
- {
- column: 'liftValue',
- sortFunction: (a: string, b: string) => {
- if (a === 'control') {
- return -1;
- }
- if (b === 'control') {
- return 1;
- }
+ {
+ column: 'liftValue',
+ sortFunction: (a: string, b: string) => {
+ if (a === 'control') {
+ return -1;
+ }
+ if (b === 'control') {
+ return 1;
+ }
- return parseFloat(a) > parseFloat(b) ? -1 : 1; // lift values
descending
+ const liftA = parseFloat(a);
+ const liftB = parseFloat(b);
+ if (liftA === liftB) {
+ return 0;
+ }
+
+ return liftA > liftB ? -1 : 1; // lift values descending
Review Comment:
**Suggestion:** The lift comparator does not handle non-numeric values
(`NaN`, invalid strings) consistently: for mixed valid/invalid pairs it can
return `1` in both directions, which violates the comparator contract and
causes unstable sorting. Add explicit checks for non-finite parsed values and
return deterministic ordering (`0` when both invalid, one-sided ordering when
only one is invalid). [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Paired T-Test Lift % sorting misorders some rows.
- ⚠️ Users see inconsistent ordering when lift values are invalid.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/PairedTTest.tsx:124-133`,
render a Paired T-Test chart where for some metric `data[metric]` includes a
control row
and a variant row whose summed `y` values in `DataEntry.values` produce a
non-finite lift
(e.g., both sums are `0`, yielding `0/0` in `computeLift`).
2. `PairedTTest` passes this metric’s `DataEntry[]` into `TTestTable`
(`TTestTable.tsx:43-50`), where `computeTTest` at `TTestTable.tsx:113-139`
fills
`liftValues` with `'control'` for the control row and
`computeLift(...).toFixed(liftValPrec)` strings for other rows; for the
degenerate case,
`computeLift` at `TTestTable.tsx:58-73` returns `'NaN'` or another
non-numeric string.
3. When the user clicks the “Lift %” column header, Reactable sorts using
the custom
`sortFunction` for the `'liftValue'` column defined at
`TTestTable.tsx:235-251`, which
parses each cell with `parseFloat(a)` / `parseFloat(b)` and compares `liftA`
and `liftB`.
4. For a pair where one row has `liftValue='NaN'` and the other has a valid
numeric
string, `sortFunction(a, b)` returns `1` (because all comparisons with `NaN`
are false)
and `sortFunction(b, a)` also returns `1`, violating the comparator contract
and causing
unstable or incorrect ordering when the table is sorted by “Lift %” for such
datasets.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9187967e61934c37af6cc0e9f35c6b9a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9187967e61934c37af6cc0e9f35c6b9a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 236:250
**Comment:**
*Incorrect Condition Logic: The lift comparator does not handle
non-numeric values (`NaN`, invalid strings) consistently: for mixed
valid/invalid pairs it can return `1` in both directions, which violates the
comparator contract and causes unstable sorting. Add explicit checks for
non-finite parsed values and return deterministic ordering (`0` when both
invalid, one-sided ordering when only one is invalid).
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=64d02b588107368ef030e4ee1e795e64943a2b28a0f3336071065b3c2d4c1e89&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=64d02b588107368ef030e4ee1e795e64943a2b28a0f3336071065b3c2d4c1e89&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]
