codeant-ai-for-open-source[bot] commented on code in PR #40995:
URL: https://github.com/apache/superset/pull/40995#discussion_r3402239087
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -595,15 +609,15 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
drillBy: cellPoint.isMetric
? undefined
: {
- filters: [
- {
- col: cellPoint.key,
- op: '==',
- val: extractTextFromHTML(cellPoint.value),
- },
- ],
- groupbyFieldName: 'groupby',
- },
+ filters: [
+ {
+ col: cellPoint.key,
+ op: '==',
+ val: extractTextFromHTML(cellPoint.value),
+ },
+ ],
+ groupbyFieldName: 'groupby',
Review Comment:
**Suggestion:** `drillBy` is using the rendered table key directly, but
adhoc/custom-labeled columns need to be resolved back to the source column name
(as done for cross-filter). Without that mapping, drill-by requests can target
non-existent dataset columns and fail. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Drill-by from table fails on adhoc-labeled columns.
- ⚠️ Drill behavior inconsistent with echarts drill-by implementation.
- ⚠️ Users lose drill navigation for renamed dimensions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a Table chart using an adhoc dimension with a custom label (e.g.
sqlExpression
`state`, label `State_Renamed`) so the query result column is the label
(transformProps
builds `columnLabelToNameMap` at
`superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:21-29`
and passes it
to `TableChart` at `:293-295`).
2. Render this chart on a dashboard with context menu enabled; `TableChart`
receives
`columnLabelToNameMap` and builds context-menu handling at
`superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:578-630`,
where
`DataTable` passes `cellPoint.key` derived from `DataColumnMeta.key` for the
clicked
column.
3. Right-click on a cell in the adhoc column; `handleContextMenu` constructs
`drillBy`
filters at `TableChart.tsx:609-619` using `{ col: cellPoint.key, op: '==',
val:
extractTextFromHTML(cellPoint.value) }`, so `col` is the rendered column key
(`'State_Renamed'`), not the original dataset/sql expression (`'state'`).
4. The drill-by machinery (core drill-by handlers expect dataset/groupby
field names like
`'state'`, consistent with other charts such as `EchartsMixedTimeseries` at
`plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:39-45`)
receives a `col` that does not exist in the dataset, causing the generated
drill-by
Explore request to reference a non-existent column and return errors or
empty results for
that drill interaction.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2129b26bf4934790b755452c34ff7d6a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2129b26bf4934790b755452c34ff7d6a&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/plugin-chart-table/src/TableChart.tsx
**Line:** 609:619
**Comment:**
*Api Mismatch: `drillBy` is using the rendered table key directly, but
adhoc/custom-labeled columns need to be resolved back to the source column name
(as done for cross-filter). Without that mapping, drill-by requests can target
non-existent dataset columns and fail.
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%2F40995&comment_hash=c233d2743aec425c7962652f7f3f9a6888dda4f63e2afb0f3d3aed8ba097b73a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40995&comment_hash=c233d2743aec425c7962652f7f3f9a6888dda4f63e2afb0f3d3aed8ba097b73a&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1098,11 +1400,11 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
onClick:
emitCrossFilters && !valueRange && !isMetric
? () => {
- // allow selecting text in a cell
- if (!getSelectedText()) {
- toggleFilter(key, value);
- }
+ // allow selecting text in a cell
+ if (!getSelectedText()) {
+ toggleFilter(key, value);
Review Comment:
**Suggestion:** `getSelectedText()` reads global browser selection, so
selecting text anywhere on the page can block table cell click filtering. This
introduces inconsistent behavior where clicks do nothing despite targeting a
filterable cell. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Cell click filters silently ignored when page text selected.
- ⚠️ Users experience inconsistent table cross-filter behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a Table chart with cross-filtering enabled so `emitCrossFilters`
is true and
`toggleFilter` wiring in `getColumnConfigs` is active
(`TableChart.tsx:1216-1219`).
2. On the dashboard page, select any text anywhere in the document (for
example in a
filter label or another control), which populates `window.getSelection()`;
`getSelectedText()` from `@superset-ui/core` returns this global selection at
`packages/superset-ui-core/src/utils/getSelectedText.ts:19`.
3. Click on a non-metric, filterable cell in the table; the cell's `onClick`
handler at
`TableChart.tsx:1400-1407` executes and enters the branch `if
(!getSelectedText()) {
toggleFilter(key, value); }`, but since `getSelectedText()` returns a
non-empty string the
condition is false and `toggleFilter` is never called.
4. As a result, the cell click does not add/remove the filter even though
the cell is
marked filterable (`className` includes `dt-is-filter` at
`TableChart.tsx:1216-1219`),
causing confusing behavior where table cells sometimes appear
non-interactive solely
because text is selected elsewhere on the page (mirroring the same
global-selection
pattern seen in `PivotTableChart` at
`plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:11-14`).
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ea6ea69325ee4a8a94e0e539069494ff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ea6ea69325ee4a8a94e0e539069494ff&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/plugin-chart-table/src/TableChart.tsx
**Line:** 1403:1405
**Comment:**
*Logic Error: `getSelectedText()` reads global browser selection, so
selecting text anywhere on the page can block table cell click filtering. This
introduces inconsistent behavior where clicks do nothing despite targeting a
filterable cell.
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%2F40995&comment_hash=1197cb533e43f428107e93f0819a0ac8cd546a90f5ec5135eff33bb2d35efa4e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40995&comment_hash=1197cb533e43f428107e93f0819a0ac8cd546a90f5ec5135eff33bb2d35efa4e&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:
##########
@@ -228,6 +248,48 @@ const buildQuery: BuildQuery<TableChartFormData> = (
moreProps.row_offset = 0;
}
+ const visibleColumnKeys = Array.isArray(ownState?.visibleColumns)
+ ? ownState.visibleColumns.map(String)
+ : [];
+ const visibleColumnSet = new Set(visibleColumnKeys);
+
+ let underlyingColumnSet = visibleColumnSet;
+
+ if (isDownloadQuery && visibleColumnSet.size > 0) {
+ if (isTimeComparison(formData, baseQueryObject)) {
+ underlyingColumnSet = new Set(
+ Array.from(visibleColumnSet).map(key => {
+ if (key.startsWith('Main ')) return key.substring(5);
+ if (key.startsWith('# ')) return key.substring(2);
+ if (key.startsWith('△ ')) return key.substring(2);
+ if (key.startsWith('% ')) return key.substring(2);
+ return key;
+ }),
+ );
+ }
+
+ columns = columns.filter(column =>
+ underlyingColumnSet.has(
+ getColumnSelectionKey(column as string | AdhocColumn),
+ ),
+ );
+
+ if (Array.isArray(metrics) && metrics.length > 0) {
+ metrics = metrics.filter(metric =>
+ underlyingColumnSet.has(getMetricLabel(metric)),
+ );
Review Comment:
**Suggestion:** Filtering `metrics` by `getMetricLabel(metric)` breaks
percent-metric exports: visible percent columns are keyed like `%metric`, but
this check looks for `metric` without `%`, so required base metrics get
removed. That can leave contribution post-processing referencing missing
columns and produce empty/incorrect download results. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Downloads with percent metrics can omit required base metrics.
- ⚠️ Exported CSV/XLSX shows incorrect contribution values.
- ⚠️ Behavior diverges between on-screen table and downloads.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Table chart with aggregate query mode and at least one
percent metric
(`percent_metrics` in form data), so `buildQuery` computes `percentMetrics`
and
contribution post-processing (see `buildQuery.ts:166-201` where
`percentMetricsLabelsWithTimeComparison` and `rename_columns:
percentMetricLabels.map(m =>
`%${m}`)` are set).
2. On the dashboard, use the new column visibility UI in `TableChart`
(`renderColumnSelectDropdown` at `TableChart.tsx:918-1037`) to hide the base
metric column
(e.g. `metric1`) while keeping only the contribution column visible (whose
`DataColumnMeta.key` will be in the form `%metric1`).
3. Trigger a download (CSV/XLSX/JSON results) for this chart; `buildQuery`
is called with
`isDownloadQuery` true (`buildQuery.ts:241-247`), `visibleColumnKeys` is
populated from
`ownState.visibleColumns` as the visible table column keys (e.g.
`['%metric1']`) at
`buildQuery.ts:251-255`, and `underlyingColumnSet` is derived from these
keys at
`buildQuery.ts:256-268`.
4. In the download-specific filtering block, `metrics` are filtered at
`buildQuery.ts:277-280` using
`underlyingColumnSet.has(getMetricLabel(metric))`; for the
base metric `metric1`, `getMetricLabel(metric)` returns `'metric1'`, which
is not in
`underlyingColumnSet` (only `%metric1` is present), so the base metric is
removed while
contribution post-processing still expects it, leading to incorrect or empty
percent
values in the downloaded data compared to the on-screen table.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7295913958f24f848787bb6f1c90fdd5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7295913958f24f848787bb6f1c90fdd5&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/plugin-chart-table/src/buildQuery.ts
**Line:** 278:280
**Comment:**
*Logic Error: Filtering `metrics` by `getMetricLabel(metric)` breaks
percent-metric exports: visible percent columns are keyed like `%metric`, but
this check looks for `metric` without `%`, so required base metrics get
removed. That can leave contribution post-processing referencing missing
columns and produce empty/incorrect download results.
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%2F40995&comment_hash=50aa962eeaf1a6553579975da6958a5a83071380ab09b0351764a0de0e4ae0e9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40995&comment_hash=50aa962eeaf1a6553579975da6958a5a83071380ab09b0351764a0de0e4ae0e9&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:
##########
@@ -84,6 +103,7 @@ const buildQuery: BuildQuery<TableChartFormData> = (
return buildQueryContext(formDataCopy, baseQueryObject => {
let { metrics, orderby = [], columns = [] } = baseQueryObject;
+ columns = columns.filter(column => !isSyntheticPercentColumn(column));
Review Comment:
**Suggestion:** This removes any string column starting with `%`, which is
broader than synthetic percent columns and will also strip legitimate
user-defined columns/aliases that begin with `%`. That causes missing columns
in the generated query and incorrect result sets. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Table queries drop real columns starting with percent.
- ⚠️ Interactive groupby cannot include such percent-prefixed columns.
- ⚠️ Users see missing dimensions in table results.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use a dataset that defines a physical column or alias whose name begins
with `%` (e.g.
`%discount_flag`) and build a Table chart that selects this column so it
appears in the
base query object's `columns` array (as strings) before post-processing in
`buildQuery.ts:104-107`.
2. When `buildQuery` runs, it defines `isSyntheticPercentColumn` at
`buildQuery.ts:73-75`
as any string where `column.trim().startsWith('%')`, then immediately
filters `columns`
with `columns = columns.filter(column =>
!isSyntheticPercentColumn(column));` at
`buildQuery.ts:106`.
3. Because `%discount_flag` starts with `%`,
`isSyntheticPercentColumn('%discount_flag')`
returns true even though this column is not a synthetic contribution metric,
so it is
removed from `columns` and never reaches the query object
(`queryObject.columns` is built
from the filtered `columns` at `buildQuery.ts:308-319`).
4. The resulting query and chart data omit the legitimate `%discount_flag`
column, and the
same broad prefix filter is applied again when augmenting with
`interactive_groupby` at
`buildQuery.ts:396-401`, meaning any user-defined column starting with `%`
cannot be used
as a grouping dimension in this visualization.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f6e4c8006c884e489f42fa69a630ef47&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f6e4c8006c884e489f42fa69a630ef47&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/plugin-chart-table/src/buildQuery.ts
**Line:** 106:106
**Comment:**
*Incorrect Condition Logic: This removes any string column starting
with `%`, which is broader than synthetic percent columns and will also strip
legitimate user-defined columns/aliases that begin with `%`. That causes
missing columns in the generated query and incorrect result sets.
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%2F40995&comment_hash=e75550d72da75edd51ee6520301d6942aba4892ceb604897f081f66d50d5b21a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40995&comment_hash=e75550d72da75edd51ee6520301d6942aba4892ceb604897f081f66d50d5b21a&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]
