codeant-ai-for-open-source[bot] commented on code in PR #41957:
URL: https://github.com/apache/superset/pull/41957#discussion_r3565962531
##########
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:
##########
@@ -81,6 +85,12 @@ const TimeTable = ({
const valueField = row.label || row.metric_name || '';
const cellValues = columnConfigs.reduce<Record<string, ReactNode>>(
(acc, columnConfig) => {
+ const { value, errorMsg } = calculateCellValue(
+ valueField,
+ columnConfig,
+ reversedEntries,
+ );
+
if (columnConfig.colType === 'spark') {
Review Comment:
**Suggestion:** `calculateCellValue` is executed for every column before
checking `colType`, so sparkline columns do unnecessary value computation that
is immediately discarded; this adds avoidable per-row/per-column work in a hot
render path. Move the calculation into the non-spark branch to avoid wasted
processing. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ TimeTable sparkline columns perform redundant value calculations per
render.
- ⚠️ Large tables with many spark columns render more slowly.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the TimeTable component, inspect the memoizedRows computation at
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:81-126. For
each row,
columnConfigs.reduce is used to build cellValues; within the reducer,
calculateCellValue
is called unconditionally at lines 88-92 to derive { value, errorMsg } for
the current
column.
2. Immediately after that call, the code branches on columnConfig.colType at
lines 94-105.
If colType is 'spark', the reducer returns a Sparkline component: <Sparkline
valueField={valueField} column={columnConfig} entries={entries} /> (lines
98-102), and the
previously computed value and errorMsg are not referenced anywhere in the
spark branch.
3. Review calculateCellValue in
superset-frontend/src/visualizations/TimeTable/utils/valueCalculations/valueCalculations.ts:142-160.
This helper performs non-trivial work per cell: it reads reversedEntries,
derives the
“recent” value, and may call calculateTimeValue, calculateContribution, or
calculateAverage (lines 151-158), which themselves iterate over
reversedEntries and
perform numeric computations.
4. Because calculateCellValue is invoked for every column, including spark
columns whose
rendered cells never use its result, every TimeTable render with spark
columns performs
unnecessary per-row/per-column value computation that is immediately
discarded. This
wasted work sits on the core render path for TimeTable, so any dataset with
many rows and
spark columns will pay extra CPU cost without functional benefit.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5a6b8aaed3d747fc87387d6ed78aeddf&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=5a6b8aaed3d747fc87387d6ed78aeddf&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/src/visualizations/TimeTable/TimeTable.tsx
**Line:** 88:94
**Comment:**
*Performance: `calculateCellValue` is executed for every column before
checking `colType`, so sparkline columns do unnecessary value computation that
is immediately discarded; this adds avoidable per-row/per-column work in a hot
render path. Move the calculation into the non-spark branch to avoid wasted
processing.
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%2F41957&comment_hash=36dba5fe307a7f35acb4c8c80ffd5a09d24cbb715fa9c02b85d4dff5c5832905&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41957&comment_hash=36dba5fe307a7f35acb4c8c80ffd5a09d24cbb715fa9c02b85d4dff5c5832905&reaction=dislike'>👎</a>
##########
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:
##########
@@ -98,9 +108,9 @@ const TimeTable = ({
...acc,
[columnConfig.key]: (
<ValueCell
- valueField={valueField}
+ value={value}
+ errorMsg={errorMsg}
column={columnConfig}
- reversedEntries={reversedEntries}
/>
Review Comment:
**Suggestion:** The new `ValueCell` props no longer include the metadata
that the existing `sortNumberWithMixedTypes` comparator depends on
(`valueField`/`reversedEntries`), so non-spark columns now sort as equal and
user sorting/default sort behavior silently breaks. Preserve the previous sort
contract (pass required props) or update the sorter to read the precomputed
`value` prop. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ TimeTable visualization columns cannot be sorted by value.
- ❌ Default descending sort by latest metric no longer applies.
- ⚠️ Users see inconsistent ordering across reloads and aggregations.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Explore page and select the TimeTable visualization type, which
is wired
through VizType.TimeTable (see
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:13-20
where TimeTable
is listed in MATRIXIFY_INCOMPATIBLE_CHARTS, and
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeControl.test.tsx:72
where TimeTableChartPlugin is configured with key VizType.TimeTable).
2. Run a query that produces multiple rows and at least one non-spark metric
column, so
TimeTable renders; the TimeTable component builds columns with sortType set
to
sortNumberWithMixedTypes at
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:50-76, and
memoizedRows at
lines 81-126, where each non-spark column cell is rendered as <ValueCell
value={value}
errorMsg={errorMsg} column={columnConfig} /> (lines 107-115).
3. Note that ValueCell’s props, defined in
superset-frontend/src/visualizations/TimeTable/components/ValueCell/ValueCell.tsx:24-28,
only include value, errorMsg, and column; they do not include valueField or
reversedEntries. However, the shared comparator sortNumberWithMixedTypes in
superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.ts:57-106
directly inspects cellA.props and cellB.props and expects props with shape {
valueField,
column, reversedEntries?, entries? } (see the comments at lines 65-68 and
the typed casts
at lines 69-84).
4. When the user clicks a non-spark column header to sort, or when the
initialSortBy
default sort is applied (TimeTable.tsx:128-135), react-table calls
sortNumberWithMixedTypes. Because the ValueCell elements in
row.values[columnId] lack
valueField and reversedEntries, reversedEntriesA and reversedEntriesB
resolve to undefined
(lines 86-89 in sortUtils.ts), triggering the guard at lines 91-93 that
returns 0 for any
row pair, so all comparisons are “equal” and sorting for non-spark columns
silently fails
(no change in order despite sort gestures or default sort configuration).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=90bcd6340fa24be4a48014aad2c024ef&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=90bcd6340fa24be4a48014aad2c024ef&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/src/visualizations/TimeTable/TimeTable.tsx
**Line:** 110:114
**Comment:**
*Api Mismatch: The new `ValueCell` props no longer include the metadata
that the existing `sortNumberWithMixedTypes` comparator depends on
(`valueField`/`reversedEntries`), so non-spark columns now sort as equal and
user sorting/default sort behavior silently breaks. Preserve the previous sort
contract (pass required props) or update the sorter to read the precomputed
`value` prop.
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%2F41957&comment_hash=f43c00ef253e18a31732411d9bd06704b3c7b5292afa3f91b7f44adc90085732&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41957&comment_hash=f43c00ef253e18a31732411d9bd06704b3c7b5292afa3f91b7f44adc90085732&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]
