codeant-ai-for-open-source[bot] commented on code in PR #37561:
URL: https://github.com/apache/superset/pull/37561#discussion_r2744855485
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -353,8 +353,14 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
);
const timestampFormatter = useCallback(
- value => getTimeFormatterForGranularity(timeGrain)(value),
- [timeGrain],
+ value => {
+ // In Raw Records mode, don't apply time grain-based formatting
+ if (isRawRecords || !timeGrain) {
+ return String(value);
+ }
+ return getTimeFormatterForGranularity(timeGrain)(value);
Review Comment:
**Suggestion:** Null/undefined values are converted to the strings "null" or
"undefined" by `String(value)`, which will display those literal words in the
UI; explicitly return an empty string for null/undefined so empty cells remain
empty and avoid misleading text. [null pointer]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Cross-filter labels display literal "null"/"undefined".
- ⚠️ Context-menu filter labels show misleading text.
```
</details>
```suggestion
// Return empty string for null/undefined values
if (value == null) {
return '';
}
// In Raw Records mode, don't apply time grain-based formatting
if (isRawRecords || !timeGrain) {
return String(value);
}
const formatter = getTimeFormatterForGranularity(timeGrain);
return typeof formatter === 'function' ? formatter(value) :
String(value);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a Superset dashboard containing a Raw Data - Interactive Table chart
using this
plugin.
2. Ensure the chart has a temporal column and cross-filtering enabled
(emitCrossFilters
true). In the code, clicking a cell triggers toggleFilter which calls
getCrossFilterDataMask (TableChart.tsx), which builds labelElements using
timestampFormatter (TableChart.tsx: lines 355-364).
3. Click a table cell for a temporal column whose value is null/undefined.
getCrossFilterDataMask maps values with: const valueLabels =
filterValues.map(value =>
isTimestamp ? timestampFormatter(value) : value); this invokes
timestampFormatter at
TableChart.tsx:355-364.
4. Observe the UI shows the literal text "null" or "undefined" in the
cross-filter label /
filter chip instead of an empty label, because String(null) === "null". The
improved code
returns '' for null/undefined avoiding misleading text.
```
</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-table/src/TableChart.tsx
**Line:** 357:361
**Comment:**
*Null Pointer: Null/undefined values are converted to the strings
"null" or "undefined" by `String(value)`, which will display those literal
words in the UI; explicitly return an empty string for null/undefined so empty
cells remain empty and avoid misleading text.
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>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -251,8 +251,14 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
);
const timestampFormatter = useCallback(
- value => getTimeFormatterForGranularity(timeGrain)(value),
- [timeGrain],
+ value => {
+ // In Raw Records mode, don't apply time grain-based formatting
+ if (isRawRecords || !timeGrain) {
+ return String(value);
Review Comment:
**Suggestion:** Null/undefined values are coerced to the strings
"null"/"undefined" via `String(value)` when in Raw Records mode, which can
break downstream equality checks and cross-filtering; return null/undefined
as-is (or an appropriate sentinel) instead of converting them to strings. [null
pointer]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Cross-filter clicks produce incorrect filter values.
- ⚠️ getCrossFilterDataMask comparisons mismatch nulls.
- ⚠️ Interactive Table raw-record filtering affected.
```
</details>
```suggestion
(value: DataRecordValue) => {
// Preserve null/undefined in Raw Records mode so filters compare the
actual value
if (value == null) {
return value;
}
// In Raw Records mode, don't apply time grain-based formatting
if (isRawRecords || !timeGrain) {
return value;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a Raw Data - Interactive Table chart (per PR testing instructions)
that has a
temporal column. The chart component is at
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx.
2. Ensure the chart is rendered with the prop isRawRecords === true. The
timestamp
formatter is defined at AgGridTableChart.tsx:253-262 as `timestampFormatter`.
3. Click a table cell for that temporal column that contains a
null/undefined timestamp
value. The AG Grid click handler sends a CellClickedEvent to toggleFilter
defined at
AgGridTableChart.tsx:264-286. The click invokes toggleFilter and includes
event.value (the
raw cell value) and timestampFormatter in crossFilterProps at
AgGridTableChart.tsx:274-281.
4. getCrossFilterDataMask (imported from ./utils/getCrossFilterDataMask and
invoked at
AgGridTableChart.tsx:282) receives timestampFormatter and will use it to
format/compare
values when creating the cross-filter dataMask. Because timestampFormatter
currently does
String(value) for raw mode (AgGridTableChart.tsx:256-258), null/undefined
becomes the
string "null"/"undefined" and will not equal the actual null/undefined
values stored in
dataset rows, causing cross-filter comparisons to fail or create filters
with incorrect
values.
```
</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/AgGridTableChart.tsx
**Line:** 254:257
**Comment:**
*Null Pointer: Null/undefined values are coerced to the strings
"null"/"undefined" via `String(value)` when in Raw Records mode, which can
break downstream equality checks and cross-filtering; return null/undefined
as-is (or an appropriate sentinel) instead of converting them to strings.
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]
