codeant-ai-for-open-source[bot] commented on code in PR #37702:
URL: https://github.com/apache/superset/pull/37702#discussion_r2774140862
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx:
##########
@@ -720,7 +718,9 @@ const config: ControlPanelConfig = {
? colnames
.filter(
(colname: string, index: number) =>
- coltypes[index] === GenericDataType.Numeric,
+ coltypes[index] === GenericDataType.Numeric ||
+ coltypes[index] === GenericDataType.String ||
+ coltypes[index] === GenericDataType.Boolean,
Review Comment:
**Suggestion:** By expanding the filter to include string and boolean column
types, the subsequent time-comparison logic (`processComparisonColumns`) will
now generate "Main/#/△/%" pseudo-columns for non-metric fields that never exist
in the actual query result, so conditional formatting rules created for these
options will never match any data; restricting this list back to numeric types
keeps comparison options aligned with real metric columns. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Conditional formatting UI shows non-matching options.
- ⚠️ Users can create rules with no visual effect.
- ⚠️ AG Grid Table conditional formatting becomes misleading.
```
</details>
```suggestion
coltypes[index] === GenericDataType.Numeric,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the AG Grid Table chart editor so the control panel renders. The
conditional
formatting control's mapStateToProps executes in
superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx at
and around
lines 691-724 (function mapStateToProps(explore, _, chart) defined there).
2. Ensure the chart metadata (chart.queriesResponse[0]) passed into that
mapStateToProps
contains colnames and coltypes arrays where one column's coltypes[index]
equals
GenericDataType.String (i.e., the query result includes a string column).
The code reads
these arrays at lines ~713-716 and then evaluates hasTimeComparison at line
~698.
3. Enable Time Comparison (so explore?.controls?.time_compare?.value is
non-empty). With
time comparison enabled, the code path at lines 716-724 computes
numericColumns and uses
the .filter(...) predicate starting at line 719. Because the predicate
currently allows
GenericDataType.String and GenericDataType.Boolean (lines 721-723), the
string/boolean
column is included in numericColumns.
4. The code then calls processComparisonColumns(...) (later in the same
mapStateToProps
block) to generate pseudo-columns like "Main <col>" / "# <col>" / "△ <col>"
/ "% <col>"
and returns them as columnOptions for the conditional_formatting control. A
user selecting
one of these pseudo-columns in the Conditional Formatting UI will create a
rule for "Main
<string column>", but the actual query result columns
(chart.queriesResponse[0].colnames)
do not contain those pseudo-names, so the created rule never matches any
rendered table
column — conditional formatting has no effect. This reproduces the described
mismatch and
demonstrates the suggested change is meaningful.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx
**Line:** 721:723
**Comment:**
*Logic Error: By expanding the filter to include string and boolean
column types, the subsequent time-comparison logic (`processComparisonColumns`)
will now generate "Main/#/△/%" pseudo-columns for non-metric fields that never
exist in the actual query result, so conditional formatting rules created for
these options will never match any data; restricting this list back to numeric
types keeps comparison options aligned with real metric columns.
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>
--
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]
