codeant-ai-for-open-source[bot] commented on code in PR #40457:
URL: https://github.com/apache/superset/pull/40457#discussion_r3308810240
##########
superset-frontend/plugins/plugin-chart-table/src/utils/formatValue.ts:
##########
@@ -70,7 +70,7 @@ export function formatColumnValue(
const { dataType, formatter, config = {}, currencyCodeColumn } = column;
const isNumber = dataType === GenericDataType.Numeric;
const smallNumberFormatter =
- config.d3SmallNumberFormat === undefined
+ !config.d3SmallNumberFormat || config.d3SmallNumberFormat.trim() === ''
Review Comment:
**Suggestion:** `config.d3SmallNumberFormat.trim()` assumes the runtime
value is always a string, but this config is loaded from serialized form data
and can be malformed; a non-string value will throw at render time and break
table formatting. Add a `typeof` check (or safe normalization) before invoking
`trim`. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Table chart cell rendering can throw TypeError on load.
- ❌ Drill-to-detail context menu formatting fails for affected columns.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Observe that table chart form data includes `column_config?:
Record<string,
TableColumnConfig>` at
`superset-frontend/plugins/plugin-chart-table/src/types.ts:55-72`
and that `TableColumnConfig.d3SmallNumberFormat?: string` is defined in
`superset-frontend/packages/superset-ui-chart-controls/src/types.ts:13-23`;
this config is
loaded from serialized chart form data without runtime validation.
2. In
`superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:21-37`,
`processColumns` reads `rawFormData: { column_config: columnConfig = {} }`
and for each
column does `const config = columnConfig[key] || {};` (line 45), passing
that `config`
into the `DataColumnMeta` objects consumed by the React `TableChart`
component.
3. In
`superset-frontend/plugins/plugin-chart-table/src/utils/formatValue.ts:65-88`,
`formatColumnValue` destructures `config = {}` from the `DataColumnMeta` and
computes
`smallNumberFormatter` using the condition `!config.d3SmallNumberFormat ||
config.d3SmallNumberFormat.trim() === ''` (line 73). If a chart's persisted
`column_config` has a non-string `d3SmallNumberFormat` (e.g. due to manual
REST API update
or corrupted JSON, such as `d3SmallNumberFormat: 123`), then
`!config.d3SmallNumberFormat`
is `false` and the code evaluates `config.d3SmallNumberFormat.trim()` on a
non-string.
4. When a Table chart is rendered, `TableChart` uses `formatColumnValue`
both for
drill-to-detail formatting at
`superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:677` and
`686` and for
cell rendering at `TableChart.tsx:1045` (`const [isHtml, text] =
formatColumnValue(column,
value, row.original);`). With the malformed `column_config`, rendering a
numeric cell with
`Math.abs(value) < 1` causes `formatColumnValue` to hit the small-number
branch and throw
`TypeError: config.d3SmallNumberFormat.trim is not a function`, breaking the
table chart
render and any drill-to-detail menu relying on `formattedVal`.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=59c4c8f15f934c5c9f126646804c9f74&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=59c4c8f15f934c5c9f126646804c9f74&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/utils/formatValue.ts
**Line:** 73:73
**Comment:**
*Type Error: `config.d3SmallNumberFormat.trim()` assumes the runtime
value is always a string, but this config is loaded from serialized form data
and can be malformed; a non-string value will throw at render time and break
table formatting. Add a `typeof` check (or safe normalization) before invoking
`trim`.
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%2F40457&comment_hash=7f61c22b610c7f11956bb6f573c1630f928b691120b35d3d9524c2643a228cb2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40457&comment_hash=7f61c22b610c7f11956bb6f573c1630f928b691120b35d3d9524c2643a228cb2&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/formatValue.ts:
##########
@@ -68,7 +68,7 @@ export function formatColumnValue(
const { dataType, formatter, config = {} } = column;
const isNumber = dataType === GenericDataType.Numeric;
const smallNumberFormatter =
- config.d3SmallNumberFormat === undefined
+ !config.d3SmallNumberFormat || config.d3SmallNumberFormat.trim() === ''
Review Comment:
**Suggestion:** `config.d3SmallNumberFormat.trim()` is called without a
runtime type guard, so malformed persisted chart config (for example a
non-string value coming from JSON/API payloads) will throw `TypeError: ...trim
is not a function` during cell rendering. Guard with `typeof ... === 'string'`
(or normalize to string safely) before calling `trim`. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ AG Grid table helper throws TypeError on malformed d3SmallNumberFormat.
- ⚠️ Future AG Grid small-number formatting may crash entire column.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Locate the AG Grid table formatting helper `formatColumnValue` defined at
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/formatValue.ts:64-84`,
which destructures `config = {}` from the `DataColumnMeta` and computes
`smallNumberFormatter` using `!config.d3SmallNumberFormat ||
config.d3SmallNumberFormat.trim() === ''` (line 71).
2. Note that `DataColumnMeta.config` is typed as `TableColumnConfig`
(imported from
`@superset-ui/chart-controls`) where `d3SmallNumberFormat?: string` (see
`superset-frontend/packages/superset-ui-chart-controls/src/types.ts:13-23`),
but this is a
TypeScript type only; there is no runtime validation when `config` is read
from form data
or other JSON.
3. In a unit test or any consumer code, construct a `DataColumnMeta` (or
`TableDataColumnMeta`) object with a malformed `config`, for example:
- file:
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/formatValue.ts`
- object: `{ key: 'num', label: 'num', dataType: GenericDataType.Numeric,
formatter:
undefined, config: { d3SmallNumberFormat: 123 as any } }`.
Then call `formatColumnValue(column, 0.5)` using this object.
4. When `formatColumnValue` executes, `config.d3SmallNumberFormat` is the
number `123`, so
`!config.d3SmallNumberFormat` is `false` and the code evaluates
`config.d3SmallNumberFormat.trim() === ''` at line 71. Since numbers do not
implement
`trim`, this throws `TypeError: config.d3SmallNumberFormat.trim is not a
function`, and
any AG Grid table path that reuses this helper for small-number formatting
would crash
similarly on malformed `column_config`.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=05ca4de178ec4fbab8fce9c0f5386f3d&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=05ca4de178ec4fbab8fce9c0f5386f3d&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/utils/formatValue.ts
**Line:** 71:71
**Comment:**
*Type Error: `config.d3SmallNumberFormat.trim()` is called without a
runtime type guard, so malformed persisted chart config (for example a
non-string value coming from JSON/API payloads) will throw `TypeError: ...trim
is not a function` during cell rendering. Guard with `typeof ... === 'string'`
(or normalize to string safely) before calling `trim`.
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%2F40457&comment_hash=1652fa4f889dd9165b0a93daf899288c8a156da4e0ba0275aa98291451765e53&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40457&comment_hash=1652fa4f889dd9165b0a93daf899288c8a156da4e0ba0275aa98291451765e53&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]
