codeant-ai-for-open-source[bot] commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3632076176
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -368,75 +385,202 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
[emitCrossFilters, setDataMask, timeGrain, timestampFormatter],
);
+ const drillColumns = isUsingTimeComparison
+ ? (filteredColumns as InputColumn[])
+ : (columns as InputColumn[]);
+
+ const handleContextMenu = useCallback(
+ (event: CellContextMenuEvent) => {
+ if (!onContextMenu || isRawRecords || !event.column || !event.data) {
+ return;
+ }
+ const nativeEvent = event.event as MouseEvent | null | undefined;
+ if (!nativeEvent) return;
+ nativeEvent.preventDefault();
+ nativeEvent.stopPropagation();
+
+ const rowData = event.data as Record<string, DataRecordValue>;
+ const key = event.column.getColId();
+ const cellValue = event.value as DataRecordValue;
+ const colDef = event.column.getColDef();
+ const isMetric = Boolean(
+ colDef.context?.isMetric || colDef.context?.isPercentMetric,
+ );
+
+ const drillToDetailFilters: BinaryQueryObjectFilterClause[] = [];
+ drillColumns.forEach(col => {
+ if (col.isMetric || col.isPercentMetric) return;
+ const dataRecordValue = rowData[col.key];
+
+ if (
+ dataRecordValue == null ||
+ (dataRecordValue instanceof DateWithFormatter &&
+ dataRecordValue.input == null)
+ ) {
+ drillToDetailFilters.push({
+ col: col.key,
+ op: 'IS NULL' as any,
+ val: null,
+ });
+ } else if (col.dataType === GenericDataType.Temporal && timeGrain) {
+ const startTime =
+ dataRecordValue instanceof Date
+ ? dataRecordValue
+ : new Date(dataRecordValue as string | number);
+
+ if (Number.isNaN(startTime.getTime())) {
+ // Malformed temporal value: fall back to an equality filter
+ // instead of building a TEMPORAL_RANGE, since toISOString()
+ // throws on an Invalid Date and would crash the context menu.
+ const sanitizedValue = extractTextFromHTML(dataRecordValue);
+ drillToDetailFilters.push({
+ col: col.key,
+ op: '==',
+ val: sanitizedValue as string | number | boolean,
+ formattedVal: formatColumnValue(col, sanitizedValue)[1],
+ });
+ } else {
+ const [rangeStartTime, rangeEndTime] = getTimeRangeFromGranularity(
+ startTime,
+ timeGrain,
+ );
+ const timeRangeValue = `${rangeStartTime.toISOString()} :
${rangeEndTime.toISOString()}`;
+
+ drillToDetailFilters.push({
+ col: col.key,
+ op: 'TEMPORAL_RANGE',
+ val: timeRangeValue,
+ grain: timeGrain,
+ formattedVal: formatColumnValue(col, dataRecordValue)[1],
+ });
+ }
+ } else {
+ const sanitizedValue = extractTextFromHTML(dataRecordValue);
+ drillToDetailFilters.push({
+ col: col.key,
+ op: '==',
+ val: sanitizedValue as string | number | boolean,
+ formattedVal: formatColumnValue(col, sanitizedValue)[1],
+ });
+ }
+ });
+
+ const isCellValueNull =
+ cellValue == null ||
+ (cellValue instanceof DateWithFormatter && cellValue.input == null);
+
+ onContextMenu(nativeEvent.clientX, nativeEvent.clientY, {
+ drillToDetail: drillToDetailFilters,
+ crossFilter: isMetric
+ ? undefined
+ : getCrossFilterDataMask({
+ key,
+ value: cellValue,
+ filters,
+ timeGrain,
+ isActiveFilterValue,
+ timestampFormatter,
+ }),
+ drillBy: isMetric
+ ? undefined
+ : {
+ filters: [
+ isCellValueNull
+ ? { col: key, op: 'IS NULL' as any, val: null }
+ : {
+ col: key,
+ op: '==' as any,
+ val: extractTextFromHTML(cellValue),
+ },
+ ],
+ groupbyFieldName: 'groupby',
+ },
+ });
Review Comment:
**Suggestion:** Drill-by value construction sanitizes the raw cell value as
HTML without handling non-string objects, so wrapped temporal values can be
serialized to an unusable string (for example `"[object Object]"`). This
produces incorrect drill-by filters and wrong query results; use the underlying
primitive value for temporal/object cell types before building the filter
payload. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Drill-by on wrapped temporal cells yields wrong filters.
- ⚠️ Users see empty or mismatched drill-by query results.
- ⚠️ Table V2 drill UX inconsistent with expected behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render `TableChart` from
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx`
with
`onContextMenu` provided (so drill actions are enabled), a non-metric
temporal dimension
column, and configuration that allows that column’s cells to be represented
as
`DateWithFormatter` instances (via the import at line 52).
2. Populate the `data` prop so a temporal cell value is a non-null
`DateWithFormatter`
instance; when the user right-clicks that cell, ThemedAgGridReact emits a
`CellContextMenuEvent` and `handleContextMenu` (around lines 392-509) is
called with
`event.value` equal to that `DateWithFormatter`.
3. In `handleContextMenu`, `isCellValueNull` is computed at lines 468-472
and is false for
this non-null wrapped value, so the drill-by branch at lines 484-497 is
taken. The filter
object uses `val: extractTextFromHTML(cellValue)` where `cellValue` is the
`DateWithFormatter` instance, causing `extractTextFromHTML` (from
`@superset-ui/core`,
imported at line 24) to serialize the object to a generic string such as
`"[object
Object]"` rather than the underlying temporal primitive.
4. When `onContextMenu` is invoked at lines 472-498, the
`drillBy.filters[0].val` field
contains this unusable serialized string. Downstream drill-by queries that
rely on this
filter value to constrain results by the selected temporal dimension will
fail to match
the original rows, leading to empty or incorrect drill-by results for
wrapped temporal
cells.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f4ec715d05314721bba2deab3d55872d&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=f4ec715d05314721bba2deab3d55872d&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-ag-grid-table/src/AgGridTableChart.tsx
**Line:** 484:498
**Comment:**
*Type Error: Drill-by value construction sanitizes the raw cell value
as HTML without handling non-string objects, so wrapped temporal values can be
serialized to an unusable string (for example `"[object Object]"`). This
produces incorrect drill-by filters and wrong query results; use the underlying
primitive value for temporal/object cell types before building the filter
payload.
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%2F42088&comment_hash=fbbae5fa40fb33e6217a2a5d351bf49f88fb1d8abc4c1d4045c2891781dce22b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=fbbae5fa40fb33e6217a2a5d351bf49f88fb1d8abc4c1d4045c2891781dce22b&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -368,75 +385,202 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
[emitCrossFilters, setDataMask, timeGrain, timestampFormatter],
);
+ const drillColumns = isUsingTimeComparison
+ ? (filteredColumns as InputColumn[])
+ : (columns as InputColumn[]);
+
+ const handleContextMenu = useCallback(
+ (event: CellContextMenuEvent) => {
+ if (!onContextMenu || isRawRecords || !event.column || !event.data) {
+ return;
+ }
+ const nativeEvent = event.event as MouseEvent | null | undefined;
+ if (!nativeEvent) return;
+ nativeEvent.preventDefault();
+ nativeEvent.stopPropagation();
+
+ const rowData = event.data as Record<string, DataRecordValue>;
+ const key = event.column.getColId();
+ const cellValue = event.value as DataRecordValue;
+ const colDef = event.column.getColDef();
+ const isMetric = Boolean(
+ colDef.context?.isMetric || colDef.context?.isPercentMetric,
+ );
+
+ const drillToDetailFilters: BinaryQueryObjectFilterClause[] = [];
+ drillColumns.forEach(col => {
+ if (col.isMetric || col.isPercentMetric) return;
+ const dataRecordValue = rowData[col.key];
+
+ if (
+ dataRecordValue == null ||
+ (dataRecordValue instanceof DateWithFormatter &&
+ dataRecordValue.input == null)
+ ) {
+ drillToDetailFilters.push({
+ col: col.key,
+ op: 'IS NULL' as any,
+ val: null,
+ });
+ } else if (col.dataType === GenericDataType.Temporal && timeGrain) {
+ const startTime =
+ dataRecordValue instanceof Date
+ ? dataRecordValue
+ : new Date(dataRecordValue as string | number);
+
+ if (Number.isNaN(startTime.getTime())) {
+ // Malformed temporal value: fall back to an equality filter
+ // instead of building a TEMPORAL_RANGE, since toISOString()
+ // throws on an Invalid Date and would crash the context menu.
+ const sanitizedValue = extractTextFromHTML(dataRecordValue);
+ drillToDetailFilters.push({
+ col: col.key,
+ op: '==',
+ val: sanitizedValue as string | number | boolean,
+ formattedVal: formatColumnValue(col, sanitizedValue)[1],
+ });
+ } else {
Review Comment:
**Suggestion:** Temporal drill filters incorrectly treat non-null wrapped
date values as invalid because only native `Date` is parsed. When a temporal
cell value is a `DateWithFormatter`, this falls into the invalid-date branch
and emits an equality filter instead of a `TEMPORAL_RANGE`, which breaks
drill-to-detail behavior for time-grain charts. Parse the wrapped temporal
input value before constructing the range. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Drill-to-detail on temporal columns misuses equality filters.
- ⚠️ Time-grain drill queries return incomplete or incorrect rows.
- ⚠️ V2 table drill behavior diverges from Table V1.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render the default export `TableChart` from
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:56-657`
with a temporal column (where `col.dataType === GenericDataType.Temporal`)
and a non-null
`timeGrain`, and with `isUsingTimeComparison` enabled so temporal cells can
be wrapped as
`DateWithFormatter` (import at line 52).
2. Ensure the grid data passed via the `data` prop includes a row where the
temporal
column value is a `DateWithFormatter` instance whose `input` property is
non-null; this
value is read in `handleContextMenu` as `rowData[col.key]` at approximately
lines 411-413.
3. Right-click that temporal cell in the rendered AG Grid table so
ThemedAgGridReact emits
a `CellContextMenuEvent` and `handleContextMenu` (defined around lines
392-509 in
`AgGridTableChart.tsx`) is invoked with `event.column` and `event.data`
populated.
4. Inside `handleContextMenu`, when the branch at line 424 (`else if
(col.dataType ===
GenericDataType.Temporal && timeGrain)`) executes, `dataRecordValue` is the
`DateWithFormatter` instance, `startTime` is computed at lines 425-428 using
`new
Date(dataRecordValue as string | number)`, which produces an invalid `Date`.
The
`Number.isNaN(startTime.getTime())` check at line 431 then passes, causing
the
equality-filter fallback branch (lines 432-441) to push an `op: '=='` filter
instead of
the intended `TEMPORAL_RANGE` filter. As a result, the `drillToDetail`
payload passed to
`onContextMenu` at lines 472-478 does not respect the time grain and
drill-to-detail
queries are executed with incorrect filters.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=867c281db6824bcf9ad4a2bd336654f3&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=867c281db6824bcf9ad4a2bd336654f3&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-ag-grid-table/src/AgGridTableChart.tsx
**Line:** 425:442
**Comment:**
*Logic Error: Temporal drill filters incorrectly treat non-null wrapped
date values as invalid because only native `Date` is parsed. When a temporal
cell value is a `DateWithFormatter`, this falls into the invalid-date branch
and emits an equality filter instead of a `TEMPORAL_RANGE`, which breaks
drill-to-detail behavior for time-grain charts. Parse the wrapped temporal
input value before constructing the range.
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%2F42088&comment_hash=9527526a7d768ab24b200685b69927d635cabf96458ce6e465086f624ad30c35&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=9527526a7d768ab24b200685b69927d635cabf96458ce6e465086f624ad30c35&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]
