codeant-ai-for-open-source[bot] commented on code in PR #40180:
URL: https://github.com/apache/superset/pull/40180#discussion_r3252916101
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:
##########
@@ -218,6 +220,36 @@ const aggregatorsFactory = (formatter: NumberFormatter) =>
({
),
});
+const getDrillFilterValue = (
+ value: string,
+ formatter: DateFormatter | undefined,
+): string | number => {
+ if (formatter && value.trim() !== '' && Number.isFinite(Number(value))) {
+ return Number(value);
+ }
+ return value;
+};
+
+const getCrossFilterValue = (
+ value: DataRecordValue,
+ formatter: DateFormatter | undefined,
+): DataRecordValue => {
+ if (
+ formatter &&
+ typeof value === 'string' &&
+ value.trim() !== '' &&
+ Number.isFinite(Number(value))
+ ) {
+ return Number(value);
+ }
+ return value;
+};
+
+const getDrillFilterFormattedValue = (
+ value: string,
+ formatter: DateFormatter | undefined,
+) => formatter?.(Number(value)) || String(value);
Review Comment:
**Suggestion:** `formattedVal` is now always computed with `Number(value)`
whenever a date formatter exists, which breaks non-epoch temporal strings (for
example ISO timestamps) by passing `NaN` into the formatter. This can produce
incorrect labels or even throw at runtime in formatter implementations that
call `toISOString()` on an invalid date. Only coerce to number when the input
is actually a finite numeric string; otherwise pass the original value to the
formatter. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Drill-to-detail labels show "NaN" for some dates.
- ❌ TimeFormatter-based dateFormatters may throw on NaN input.
- ⚠️ Drill-to-detail context menus become unreliable for temporal columns.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Pivot Table chart using a dataset with a temporal column that
returns
non-numeric strings (for example, an ISO date like "2024-01-01") and no
explicit time
granularity, so the column type is `GenericDataType.Temporal` but
`isNumeric` returns
false for that column in
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:35-42`.
2. When the chart renders, `transformProps` builds `dateFormatters` at
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:124-147`.
For a temporal column that is not purely numeric and uses `SMART_DATE_ID`
without a
granularity, it sets `formatter = String` and stores it in
`dateFormatters[temporalColname]` (lines 135-144, 148-150).
3. In the React view, `PivotTableChart` receives this `dateFormatters` map
via props and
passes it into the pivot table. When a user right-clicks a row header for
that temporal
column, `handleContextMenu` in
`superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:277-343`
is
invoked. For each row key, it looks up the formatter (`const formatter =
dateFormatters[col];`) and calls `const formattedVal =
getDrillFilterFormattedValue(val,
formatter);` at lines 305-308.
4. `getDrillFilterFormattedValue` is defined at
`superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:248-251`
as
`formatter?.(Number(value)) || String(value);`. For a non-numeric header
string like
"2024-01-01" and `formatter = String`, this computes `Number("2024-01-01")`
(which is
`NaN`), then calls `String(NaN)`, so `formattedVal` becomes "NaN" instead of
the original
date. If the formatter is a `TimeFormatter` from `@superset-ui/core` (e.g.,
created by
`getTimeFormatter` or `getTimeFormatterForGranularity` in
`packages/superset-ui-core/src/time-format/TimeFormatterRegistrySingleton.ts:49-43`),
it
will receive `NaN` instead of a `Date`/number, which can lead to invalid
dates or runtime
errors in formatters that call `new Date(value)` or `toISOString()`
internally. The
resulting `drillToDetail` filters passed via `onContextMenu` have incorrect
or crashing
`formattedVal`, breaking drill-to-detail UX for those temporal columns.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fplugins%2Fplugin-chart-pivot-table%2Fsrc%2FPivotTableChart.tsx%0A%2A%2ALine%3A%2A%2A%20248%3A251%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20%60formattedVal%60%20is%20now%20always%20computed%20with%20%60Number%28value%29%60%20whenever%20a%20date%20formatter%20exists%2C%20which%20breaks%20non-epoch%20temporal%20strings%20%28for%20example%20ISO%20timestamps%29%20by%20passing%20%60NaN%60%20into%20the%20formatter.%20This%20can%20produce%20incorrect%20labels%20or%20even%20throw%20at%20runtime%20in%20formatter%20implementations%20that%20call%20%60toISOString%28%29%60%20on%20an%20invalid%20date.%20Only%20coerce%20to%20number%20when%20the%20input%20is%20actually%20a%20finite%20numeric%20string%3B%20otherwise%20pass%20the%20original%20value%20to%20the%20formatter.%0A%0AValidate%20the%20correctness%20of%20the
%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fplugins%2Fplugin-chart-pivot-table%2Fsrc%2FPivotTableChart.tsx%0A%2A%2ALine%3A%2A%2A%20248%3A251%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20%60formattedVal%60%20is%20now%20always%20computed%20with%20%60Number%28value%29%60%20whenever%20a%20date%20formatter%20exists%2C%20which%20breaks%20non-epoch%20t
emporal%20strings%20%28for%20example%20ISO%20timestamps%29%20by%20passing%20%60NaN%60%20into%20the%20formatter.%20This%20can%20produce%20incorrect%20labels%20or%20even%20throw%20at%20runtime%20in%20formatter%20implementations%20that%20call%20%60toISOString%28%29%60%20on%20an%20invalid%20date.%20Only%20coerce%20to%20number%20when%20the%20input%20is%20actually%20a%20finite%20numeric%20string%3B%20otherwise%20pass%20the%20original%20value%20to%20the%20formatter.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix
%0A)
*(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-pivot-table/src/PivotTableChart.tsx
**Line:** 248:251
**Comment:**
*Logic Error: `formattedVal` is now always computed with
`Number(value)` whenever a date formatter exists, which breaks non-epoch
temporal strings (for example ISO timestamps) by passing `NaN` into the
formatter. This can produce incorrect labels or even throw at runtime in
formatter implementations that call `toISOString()` on an invalid date. Only
coerce to number when the input is actually a finite numeric string; otherwise
pass the original value to the formatter.
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%2F40180&comment_hash=80baeef35ffb92da73ecf91a40d4894d4af8ee2e86215904f4ecff55476e81d8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40180&comment_hash=80baeef35ffb92da73ecf91a40d4894d4af8ee2e86215904f4ecff55476e81d8&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]
