Copilot commented on code in PR #35863:
URL: https://github.com/apache/superset/pull/35863#discussion_r2470443152
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -403,10 +403,21 @@ const config: ControlPanelConfig = {
renderTrigger: true,
label: t('Conditional formatting'),
description: t('Apply conditional color formatting to metrics'),
+ shouldMapStateToProps() {
+ return true;
+ },
mapStateToProps(explore, _, chart) {
- const values =
- (explore?.controls?.metrics?.value as QueryFormMetric[]) ??
+ const metrics =
+ (explore?.controls?.metrics?.value as QueryFormMetric[]) ||
[];
+ const columns =
+ (explore?.controls?.groupbyColumns
+ ?.value as QueryFormMetric[]) || [];
+ const rows =
+ (explore?.controls?.groupbyRows
+ ?.value as QueryFormMetric[]) || [];
+ const values = [...new Set([...metrics, ...columns, ...rows])];
Review Comment:
The `Set` deduplication may not work as expected for `QueryFormColumn` and
`QueryFormMetric` types. When these are objects (e.g., `AdhocColumn` or
`AdhocMetric`), the `Set` will compare by reference, not by value, meaning
duplicate objects with the same properties will not be deduplicated.
For string values (physical columns/metrics), this works fine, but for adhoc
columns/metrics (which are objects), you may end up with duplicates. Consider
implementing a proper deduplication strategy based on a unique identifier
(e.g., label or sqlExpression for adhoc types).
```suggestion
// Deduplicate by value: string for physical, label for adhoc
const allItems = [...metrics, ...columns, ...rows];
const uniqueMap = new Map<string, QueryFormMetric>();
allItems.forEach(item => {
if (typeof item === 'string') {
uniqueMap.set(item, item);
} else if (item && typeof item === 'object' && 'label' in
item) {
uniqueMap.set(item.label, item);
}
});
const values = Array.from(uniqueMap.values());
```
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -403,10 +403,21 @@ const config: ControlPanelConfig = {
renderTrigger: true,
label: t('Conditional formatting'),
description: t('Apply conditional color formatting to metrics'),
+ shouldMapStateToProps() {
+ return true;
+ },
mapStateToProps(explore, _, chart) {
- const values =
- (explore?.controls?.metrics?.value as QueryFormMetric[]) ??
+ const metrics =
+ (explore?.controls?.metrics?.value as QueryFormMetric[]) ||
[];
+ const columns =
+ (explore?.controls?.groupbyColumns
+ ?.value as QueryFormMetric[]) || [];
+ const rows =
+ (explore?.controls?.groupbyRows
+ ?.value as QueryFormMetric[]) || [];
Review Comment:
The type assertion `as QueryFormMetric[]` is incorrect for `groupbyColumns`
and `groupbyRows`. According to the type definitions, these fields have type
`QueryFormColumn[]` (which can be either `PhysicalColumn` (string) or
`AdhocColumn`), not `QueryFormMetric[]`. This type mismatch could lead to type
safety issues and incorrect behavior downstream.
Consider using the correct type:
```typescript
const columns =
(explore?.controls?.groupbyColumns?.value as QueryFormColumn[]) || [];
const rows =
(explore?.controls?.groupbyRows?.value as QueryFormColumn[]) || [];
```
```suggestion
?.value as QueryFormColumn[]) || [];
const rows =
(explore?.controls?.groupbyRows
?.value as QueryFormColumn[]) || [];
```
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -403,10 +403,21 @@ const config: ControlPanelConfig = {
renderTrigger: true,
label: t('Conditional formatting'),
description: t('Apply conditional color formatting to metrics'),
+ shouldMapStateToProps() {
+ return true;
+ },
mapStateToProps(explore, _, chart) {
- const values =
- (explore?.controls?.metrics?.value as QueryFormMetric[]) ??
+ const metrics =
+ (explore?.controls?.metrics?.value as QueryFormMetric[]) ||
[];
+ const columns =
+ (explore?.controls?.groupbyColumns
+ ?.value as QueryFormMetric[]) || [];
+ const rows =
+ (explore?.controls?.groupbyRows
+ ?.value as QueryFormMetric[]) || [];
Review Comment:
The change from nullish coalescing operator (`??`) to logical OR operator
(`||`) introduces a behavioral difference. The `||` operator will treat empty
arrays as falsy and replace them with `[]`, whereas `??` would only replace
`null` or `undefined`. This could mask issues where an empty array is a valid
state that should be preserved.
Consider keeping the original `??` operator to maintain the existing
behavior, or ensure this change is intentional and won't affect cases where
`metrics` is an empty array.
```suggestion
(explore?.controls?.metrics?.value as QueryFormMetric[]) ??
[];
const columns =
(explore?.controls?.groupbyColumns
?.value as QueryFormMetric[]) ?? [];
const rows =
(explore?.controls?.groupbyRows
?.value as QueryFormMetric[]) ?? [];
```
--
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]
