Copilot commented on code in PR #44665:
URL: https://github.com/apache/superset/pull/44665#discussion_r4125784583
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -145,6 +145,22 @@ function getSortTypeByDataType(dataType: GenericDataType):
DefaultSortTypes {
return 'basic';
}
+// Parse a cell value into a number when it reads as one. Datasources can
+// deliver numeric columns as strings ("1.00"); bars are geometric and need
+// the numeric magnitude, the same way the XLSX export interprets them.
+function parseNumeric(value: unknown): number | undefined {
+ if (typeof value === 'number') {
+ return value;
+ }
+ if (typeof value === 'string' && value.trim() !== '') {
+ const parsed = Number(value);
+ if (!Number.isNaN(parsed)) {
+ return parsed;
+ }
+ }
+ return undefined;
+}
Review Comment:
`parseNumeric` currently accepts non-NaN results including
`Infinity`/`-Infinity`. Returning an infinite value can break downstream
range/width calculations (e.g., `cellWidth` divisions) or yield misleading
bars. Consider using `Number.isFinite(parsed)` (and similarly for `typeof value
=== 'number'`) so only finite numbers are treated as numeric.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1050,11 +1071,23 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
basicColorFormatters.length > 0;
const generalShowCellBars =
config.showCellBars === undefined ? showCellBars : config.showCellBars;
+ // A Cell bar conditional-formatting rule must keep working even when the
+ // generic "Show cell bars" toggle is off: the toggle controls the
default
+ // gradient, not whether an explicit formatter rule can draw its bar.
+ const hasCellBarFormatter =
+ hasColumnColorFormatters &&
+ columnColorFormatters.some(
+ formatter =>
+ formatter.objectFormatting === ObjectFormattingEnum.CELL_BAR &&
+ (formatter.columnFormatting
+ ? formatter.columnFormatting === key
+ : formatter.column === key),
+ );
const valueRange =
!hasBasicColorFormatters &&
- generalShowCellBars &&
+ (generalShowCellBars || hasCellBarFormatter) &&
(isMetric || isRawRecords || isPercentMetric) &&
- getValueRange(key, alignPositiveNegative);
+ getValueRange(key, alignPositiveNegative, hasCellBarFormatter);
Review Comment:
The PR description says “Numeric-as-string columns: no bar … → now parsed
for both bar geometry and comparator matching”, but `getValueRange(...,
coerceNumeric)` only enables coercion when `hasCellBarFormatter` is true. If a
user relies on the global “Show cell bars” gradient without any `CELL_BAR`
formatter, numeric strings still won’t produce a `valueRange` and bars still
won’t render. Either update the description to clarify the scope (cell-bar
rules only), or extend coercion to also apply when `generalShowCellBars` is
enabled for numeric columns (e.g., based on `dataType` / column metadata).
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1092,6 +1125,10 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
let valueRangeFlag = true;
let arrow = '';
const originKey = column.key.substring(column.label.length).trim();
+ // Cell bar geometry uses the numeric magnitude; string cells that
+ // read as numbers ("1.00") still draw a bar, matching how the rule
+ // engine treats them.
+ const numericValue = parseNumeric(value);
Review Comment:
`parseNumeric(value)` is now executed for every cell render, even when
neither global cell bars nor a matching cell-bar formatter will render a bar.
On large tables this adds avoidable per-cell work (especially `Number(...)` on
strings). Consider computing `numericValue` lazily only when needed (e.g., when
`valueRange` will be used and/or when a bar will actually be rendered).
--
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]
