bito-code-review[bot] commented on code in PR #44044:
URL: https://github.com/apache/superset/pull/44044#discussion_r4079253325
##########
superset-frontend/packages/superset-ui-core/src/connection/callApi/parseResponse.ts:
##########
@@ -61,8 +61,13 @@ export default async function parseResponse<T extends
ParseMethod = 'json'>(
(value?.isGreaterThan?.(Number.MAX_SAFE_INTEGER) ||
value?.isLessThan?.(Number.MIN_SAFE_INTEGER))
) {
- // toFixed() avoids scientific notation, which BigInt() rejects.
- return BigInt(value.toFixed());
+ // Return as a decimal string to preserve full precision without
+ // producing a native bigint, which JSON.stringify cannot serialize
+ // (crashes ag-Grid, Redux DevTools, clipboard copy, and any other
+ // downstream consumer that calls JSON.stringify on result rows).
+ // bignumber.js .toFixed() always returns a non-scientific decimal
+ // string for integers, regardless of magnitude, so this is safe.
+ return value.toFixed();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Stale bigint handlers repo-wide</b></div>
<div id="fix">
Returning a string instead of `BigInt(value.toFixed())` removes native
`bigint` from `json-bigint` payloads, but sibling code still handles it:
`propertyComparator` in `Select/utils.tsx` (with a now-stale json-bigint
comment), plus `bigint` branches in `filters/utils.ts`,
`finestTemporalGrain.ts`, and `ResultSet/index.tsx`. Confirm these handlers are
now unreachable and update or remove them to avoid divergent payload-type
assumptions.
</div>
</div>
<small><i>Code Review Run #643ec2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-table/src/utils/formatValue.ts:
##########
@@ -51,12 +51,25 @@ function formatValue(
return [false, 'N/A'];
}
if (formatter) {
+ // Query results with integers beyond Number.MAX_SAFE_INTEGER are now
+ // parsed as decimal strings by parseResponse.ts (e.g.
"12345678901234567890").
+ // Accept both native bigint (legacy / direct callers) and decimal-integer
+ // strings. The /^-?\d+$/ guard is intentionally strict: floats, NaN,
+ // Infinity, scientific-notation strings, and pre-formatted values must
+ // NOT be coerced here — they flow through as-is so NumberFormatter can
+ // handle them with its own null/NaN/Infinity guards.
+ const numericValue: number =
+ typeof value === 'bigint'
+ ? Number(value)
+ : typeof value === 'string' && /^-?\d+$/.test(value)
+ ? Number(value)
+ : (value as number);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>duplicated integer guard</b></div>
<div id="fix">
The guard `(typeof value === 'number' || typeof value === 'bigint' ||
(typeof value === 'string' && /^-?\d+$/.test(value)))` and the regex are
re-typed in `formatValue.ts:64`, `getValueRange` (TableChart.tsx:506),
`cellBarStyles` (1210) and the cell-bar className (1338). If the accepted-value
set ever changes (e.g. floats), the four copies can diverge and break
range/cell-bar consistency. Centralize into one shared helper.
</div>
</div>
<small><i>Code Review Run #643ec2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -499,7 +499,17 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
function getValueRange(key: string, alignPositiveNegative: boolean) {
const nums = data
?.map(row => row?.[key])
- .filter(value => typeof value === 'number') as number[];
+ .filter(
+ value =>
+ typeof value === 'number' ||
+ typeof value === 'bigint' ||
+ (typeof value === 'string' && /^-?\d+$/.test(value)),
+ )
+ .map(value =>
+ typeof value === 'bigint' || typeof value === 'string'
+ ? Number(value)
+ : value,
+ ) as number[];
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>duplicated guard logic</b></div>
<div id="fix">
`getValueRange` re-implements the same bigint/integer-string guard as
`formatValue.ts:64` and the `cellBarStyles`/className conditions (1210, 1338).
Keeping the acceptance rule in one helper avoids silent divergence between the
range used for cell bars and the values the formatter coerces.
</div>
</div>
<small><i>Code Review Run #643ec2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
