codeant-ai-for-open-source[bot] commented on code in PR #38579:
URL: https://github.com/apache/superset/pull/38579#discussion_r2938812267
##########
superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.ts:
##########
@@ -42,23 +44,45 @@ function compareValues(
}
/**
- * Sorts table rows with mixed data types for react-table
- * @param rowA - First row to compare
- * @param rowB - Second row to compare
- * @param columnId - Column identifier for sorting
- * @param descending - Whether to sort in descending order
- * @returns Numeric comparison result for react-table
+ * Sorts table rows with mixed data types for react-table.
+ *
+ * react-table handles the asc/desc direction flip internally after calling
+ * this function, so we only return the raw comparison result.
*/
export function sortNumberWithMixedTypes(
rowA: any,
rowB: any,
columnId: string,
+ // eslint-disable-next-line @typescript-eslint/no-unused-vars
descending: boolean,
) {
- const valueA = rowA.values[columnId].props['data-value'];
- const valueB = rowB.values[columnId].props['data-value'];
+ const cellA = rowA.values?.[columnId];
+ const cellB = rowB.values?.[columnId];
+
+ // Value cells are React elements created in TimeTable with props:
+ // { valueField, column, reversedEntries }. We can recompute the numeric
+ // value using the same helper that the cell uses.
+ const propsA = cellA?.props as
+ | { valueField: string; column: ColumnConfig; reversedEntries: Entry[] }
+ | undefined;
+ const propsB = cellB?.props as
+ | { valueField: string; column: ColumnConfig; reversedEntries: Entry[] }
+ | undefined;
+
+ if (!propsA || !propsB) {
+ return 0;
+ }
- const comparison = compareValues(valueA, valueB, 'asSmallest');
+ const { value: valueA } = calculateCellValue(
+ propsA.valueField,
+ propsA.column,
+ propsA.reversedEntries,
+ );
+ const { value: valueB } = calculateCellValue(
+ propsB.valueField,
+ propsB.column,
+ propsB.reversedEntries,
+ );
Review Comment:
**Suggestion:** This comparator assumes every sortable cell has
`reversedEntries`, but sparkline columns pass `entries` instead. When a
sparkline column is sorted, `calculateCellValue` receives `undefined` and
crashes on `reversedEntries.length`. Normalize both cell shapes (ValueCell and
Sparkline) before calling `calculateCellValue`, and bail out when neither
provides usable entries. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Sparkline column sorting throws runtime TypeError.
- ❌ Default initial sort can crash spark-first tables.
- ⚠️ Sparkline row ordering becomes unusable.
```
</details>
```suggestion
const propsA = cellA?.props as
| {
valueField: string;
column: ColumnConfig;
reversedEntries?: Entry[];
entries?: Entry[];
}
| undefined;
const propsB = cellB?.props as
| {
valueField: string;
column: ColumnConfig;
reversedEntries?: Entry[];
entries?: Entry[];
}
| undefined;
const reversedEntriesA =
propsA?.reversedEntries ?? propsA?.entries?.slice().reverse();
const reversedEntriesB =
propsB?.reversedEntries ?? propsB?.entries?.slice().reverse();
if (!propsA || !propsB || !reversedEntriesA || !reversedEntriesB) {
return 0;
}
const { value: valueA } = calculateCellValue(
propsA.valueField,
propsA.column,
reversedEntriesA,
);
const { value: valueB } = calculateCellValue(
propsB.valueField,
propsB.column,
reversedEntriesB,
);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In Explore, add a Time-series table column with type `spark` (option
exists in
`superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/index.tsx:96-100`).
2. Chart props flow through `transformProps()` and pass `columnCollection` as
`columnConfigs`
(`superset-frontend/src/visualizations/TimeTable/config/transformProps/transformProps.ts:78-83`).
3. `TimeTable` applies `sortType: sortNumberWithMixedTypes` to every
configured column
(`superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:54-73`) and
renders spark
cells as `<Sparkline ... entries={entries} />` (`TimeTable.tsx:85-93`), so
spark cell
props contain `entries` (also defined in
`components/Sparkline/Sparkline.tsx:28-32`), not
`reversedEntries`.
4. When sorting runs (header click, or initial sort when `rowType ===
'column'` via
`TimeTable.tsx:119-127`), `sortNumberWithMixedTypes()` reads
`propsA.reversedEntries`/`propsB.reversedEntries` and passes `undefined` to
`calculateCellValue` (`sortUtils.ts:76-85`).
5. `calculateCellValue()` immediately dereferences `reversedEntries.length`
(`utils/valueCalculations/valueCalculations.ts:147`), causing runtime
`TypeError` for
sparkline 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/visualizations/TimeTable/utils/sortUtils/sortUtils.ts
**Line:** 65:85
**Comment:**
*Possible Bug: This comparator assumes every sortable cell has
`reversedEntries`, but sparkline columns pass `entries` instead. When a
sparkline column is sorted, `calculateCellValue` receives `undefined` and
crashes on `reversedEntries.length`. Normalize both cell shapes (ValueCell and
Sparkline) before calling `calculateCellValue`, and bail out when neither
provides usable entries.
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%2F38579&comment_hash=686a45aa486fcd8902fc7aa2363578589b12a15b1f2490818a210b4984ef034c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38579&comment_hash=686a45aa486fcd8902fc7aa2363578589b12a15b1f2490818a210b4984ef034c&reaction=dislike'>👎</a>
##########
superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.test.ts:
##########
@@ -86,7 +77,7 @@ describe('sortNumberWithMixedTypes', () => {
const result = sortNumberWithMixedTypes(rowA, rowB, 'testColumn', false);
- expect(result).toBeLessThan(0);
+ expect(typeof result).toBe('number');
Review Comment:
**Suggestion:** The string-number test currently only checks that the
comparator returns a number, which allows incorrect behavior (like returning
`0` for `'10'` vs `'20'`) to pass. This does not validate actual sorting and
can hide regressions in mixed-type numeric ordering. Assert the comparison
direction instead. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ CI may miss TimeTable string-numeric sort regressions.
- ⚠️ TimeTable header sorting can silently become incorrect.
```
</details>
```suggestion
expect(result).toBeLessThan(0);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.test.ts:74-80`;
the "should handle string numbers" test compares `'10'` and `'20'` but only
asserts
`typeof result === 'number'`.
2. Trace production usage:
`superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:72` wires
`sortNumberWithMixedTypes` into `TableView` `sortType`, so comparator
direction directly
controls TimeTable column sorting.
3. In
`superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.ts`
(comparator path at lines `33-43` and return at `87`), any future change
returning
`0`/`NaN` for string numerics would still satisfy `typeof result ===
'number'`.
4. Run the unit test suite for this file; the string-number test still
passes despite
wrong ordering semantics, so CI can miss a real regression affecting
header-click sorting
in TimeTable.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.test.ts
**Line:** 80:80
**Comment:**
*Logic Error: The string-number test currently only checks that the
comparator returns a number, which allows incorrect behavior (like returning
`0` for `'10'` vs `'20'`) to pass. This does not validate actual sorting and
can hide regressions in mixed-type numeric ordering. Assert the comparison
direction instead.
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%2F38579&comment_hash=d1a0877be6ca2ea6cc7802361babb4b3f00d0794bdbe95883e5bce50399d38ba&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38579&comment_hash=d1a0877be6ca2ea6cc7802361babb4b3f00d0794bdbe95883e5bce50399d38ba&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]
