korbit-ai[bot] commented on code in PR #34276:
URL: https://github.com/apache/superset/pull/34276#discussion_r2225539825
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/DeckGLContainer.tsx:
##########
@@ -102,6 +104,19 @@ export const DeckGLContainer = memo(
return props.layers as Layer[];
}, [props.layers]);
+ const isCustomTooltip = (content: ReactNode): boolean =>
+ isValidElement(content) && content.type === CustomTooltipWrapper;
Review Comment:
### Fragile Custom Tooltip Type Check <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The type checking of custom tooltips relies solely on comparing the
component type reference, which can break if the CustomTooltipWrapper is
imported from different module instances.
###### Why this matters
In certain bundling scenarios or when using dynamic imports, the same
component might have different references, causing the type check to fail and
custom tooltips not being rendered properly.
###### Suggested change ∙ *Feature Preview*
Use a more robust type checking mechanism by adding a custom identifier
property to the component:
```typescript
// In CustomTooltipWrapper.tsx
const CustomTooltipWrapper = (props) => {
static readonly componentType = 'CustomTooltipWrapper';
// ... rest of the component
};
// In DeckGLContainer.tsx
const isCustomTooltip = (content: ReactNode): boolean =>
isValidElement(content) &&
(content.type as any)?.componentType === 'CustomTooltipWrapper';
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e0d30f2-c977-45e1-be9d-9e636a11d5eb/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e0d30f2-c977-45e1-be9d-9e636a11d5eb?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e0d30f2-c977-45e1-be9d-9e636a11d5eb?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e0d30f2-c977-45e1-be9d-9e636a11d5eb?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e0d30f2-c977-45e1-be9d-9e636a11d5eb)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5728d1d0-1b97-4725-96ea-c46bb630f291 -->
[](5728d1d0-1b97-4725-96ea-c46bb630f291)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx:
##########
@@ -400,3 +400,73 @@ export const geojsonColumn = {
}),
},
};
+
+export const tooltipContents = {
+ name: 'tooltip_contents',
+ config: {
+ type: 'DndColumnMetricSelect',
+ label: t('Tooltip contents'),
+ multi: true,
+ freeForm: true,
+ clearable: true,
+ default: [],
+ description: t(
+ 'Drag columns and metrics here to customize tooltip content. Order
matters - items will appear in the same order in tooltips. Click the button to
manually select columns and metrics.',
+ ),
+ ghostButtonText: t('Drop columns/metrics here or click'),
+ disabledTabs: new Set(['saved', 'sqlExpression']),
+ mapStateToProps: state => {
+ const { datasource, form_data: formData } = state;
+
+ const selectedMetrics = [];
+
+ if (formData) {
+ // Scan form_data for FixedOrMetricControl patterns
+ Object.values(formData).forEach(value => {
+ if (value?.type === 'metric' && value?.value) {
+ selectedMetrics.push(value.value);
+ }
+ });
+
+ // Also check standard metrics field
+ if (formData.metrics) {
+ selectedMetrics.push(
+ ...(Array.isArray(formData.metrics)
+ ? formData.metrics
+ : [formData.metrics]),
+ );
+ }
+
+ // Add point_radius_fixed metric if it exists
+ if (formData.point_radius_fixed?.value) {
+ selectedMetrics.push(formData.point_radius_fixed.value);
+ }
Review Comment:
### Missing Type Check for point_radius_fixed Metric <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes point_radius_fixed.value is a metric without verifying its
type, which could lead to invalid metrics being included.
###### Why this matters
Without type checking, non-metric values could be incorrectly added to the
selectedMetrics array, potentially causing issues in tooltip rendering or
metric calculations.
###### Suggested change ∙ *Feature Preview*
Add type checking before adding the point_radius_fixed value:
```javascript
if (formData.point_radius_fixed?.type === 'metric' &&
formData.point_radius_fixed?.value) {
selectedMetrics.push(formData.point_radius_fixed.value);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/873ffe33-ecfb-4617-9ba1-431830c576ec/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/873ffe33-ecfb-4617-9ba1-431830c576ec?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/873ffe33-ecfb-4617-9ba1-431830c576ec?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/873ffe33-ecfb-4617-9ba1-431830c576ec?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/873ffe33-ecfb-4617-9ba1-431830c576ec)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f6ba07fd-73e5-4c24-86f7-85c6cd894123 -->
[](f6ba07fd-73e5-4c24-86f7-85c6cd894123)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx:
##########
@@ -400,3 +400,73 @@
}),
},
};
+
+export const tooltipContents = {
+ name: 'tooltip_contents',
+ config: {
+ type: 'DndColumnMetricSelect',
+ label: t('Tooltip contents'),
+ multi: true,
+ freeForm: true,
+ clearable: true,
+ default: [],
+ description: t(
+ 'Drag columns and metrics here to customize tooltip content. Order
matters - items will appear in the same order in tooltips. Click the button to
manually select columns and metrics.',
+ ),
+ ghostButtonText: t('Drop columns/metrics here or click'),
+ disabledTabs: new Set(['saved', 'sqlExpression']),
+ mapStateToProps: state => {
+ const { datasource, form_data: formData } = state;
+
+ const selectedMetrics = [];
+
+ if (formData) {
+ // Scan form_data for FixedOrMetricControl patterns
+ Object.values(formData).forEach(value => {
+ if (value?.type === 'metric' && value?.value) {
+ selectedMetrics.push(value.value);
+ }
+ });
+
+ // Also check standard metrics field
+ if (formData.metrics) {
+ selectedMetrics.push(
+ ...(Array.isArray(formData.metrics)
+ ? formData.metrics
+ : [formData.metrics]),
+ );
+ }
+
+ // Add point_radius_fixed metric if it exists
+ if (formData.point_radius_fixed?.value) {
+ selectedMetrics.push(formData.point_radius_fixed.value);
+ }
+ }
+ return {
+ columns: datasource?.columns || [],
+ savedMetrics: datasource?.metrics || [],
+ datasource,
+ selectedMetrics: [...new Set(selectedMetrics)], // Remove duplicates
+ disabledTabs: new Set(['saved', 'sqlExpression']),
+ };
+ },
+ },
+};
+
+export const tooltipTemplate = {
+ name: 'tooltip_template',
+ config: {
+ type: 'TextAreaControl',
+ label: t('Customize tooltips template'),
+ language: 'html',
Review Comment:
### Ambiguous Template Language Support <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The description mentions Handlebars templates but doesn't clarify if HTML is
the only supported template language or if other template languages are
supported.
###### Why this matters
Developers may be unsure about what template syntax they can use,
potentially leading to implementation errors.
###### Suggested change ∙ *Feature Preview*
language: 'html', // Only HTML with Handlebars syntax is supported
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f9a78c8-dec8-4806-99a8-07065f8708f4/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f9a78c8-dec8-4806-99a8-07065f8708f4?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f9a78c8-dec8-4806-99a8-07065f8708f4?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f9a78c8-dec8-4806-99a8-07065f8708f4?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f9a78c8-dec8-4806-99a8-07065f8708f4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4507d792-8c0a-462b-8834-f564b9a33bd3 -->
[](4507d792-8c0a-462b-8834-f564b9a33bd3)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Grid/Grid.tsx:
##########
@@ -31,19 +30,20 @@
import { createDeckGLComponent } from '../../factory';
import TooltipRow from '../../TooltipRow';
import { TooltipProps } from '../../components/Tooltip';
+import {
+ createTooltipContent,
+ CommonTooltipRows,
+} from '../../utilities/tooltipUtils';
+
+function defaultTooltipGenerator(o: JsonObject, formData: QueryFormData) {
Review Comment:
### Non-descriptive Parameter Name <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Parameter 'o' uses a single-letter name that doesn't convey its purpose or
content.
###### Why this matters
Single-letter variable names make code harder to understand as they don't
provide context about their purpose.
###### Suggested change ∙ *Feature Preview*
Rename the parameter to something more descriptive:
```typescript
function defaultTooltipGenerator(tooltipData: JsonObject, formData:
QueryFormData)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2cf65f57-d9cc-491b-ab13-652bf8110e34/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2cf65f57-d9cc-491b-ab13-652bf8110e34?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2cf65f57-d9cc-491b-ab13-652bf8110e34?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2cf65f57-d9cc-491b-ab13-652bf8110e34?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2cf65f57-d9cc-491b-ab13-652bf8110e34)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d3e913b9-2508-470b-b69e-95a05daeba81 -->
[](d3e913b9-2508-470b-b69e-95a05daeba81)
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/index.ts:
##########
@@ -20,3 +20,4 @@ export { default } from './DndSelectLabel';
export * from './DndColumnSelect';
export * from './DndFilterSelect';
export * from './DndMetricSelect';
+export * from './DndColumnMetricSelect';
Review Comment:
### Missing module differentiation documentation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
New exported module lacks documentation to explain its purpose and
difference from DndColumnSelect and DndMetricSelect.
###### Why this matters
Developers will have difficulty understanding when to use
DndColumnMetricSelect versus DndColumnSelect or DndMetricSelect without clear
differentiation.
###### Suggested change ∙ *Feature Preview*
Add a comment above the exports section:
```typescript
/**
* Export DnD (Drag and Drop) control components:
* - DndSelectLabel: Base label component for DnD controls
* - DndColumnSelect: Column selection control
* - DndFilterSelect: Filter selection control
* - DndMetricSelect: Metric selection control
* - DndColumnMetricSelect: Combined column and metric selection control
*/
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9b632b6-192f-45ee-a8f5-adbaad086010/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9b632b6-192f-45ee-a8f5-adbaad086010?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9b632b6-192f-45ee-a8f5-adbaad086010?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9b632b6-192f-45ee-a8f5-adbaad086010?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9b632b6-192f-45ee-a8f5-adbaad086010)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:fb00836c-aa2a-4552-a39a-c3121d59fce1 -->
[](fb00836c-aa2a-4552-a39a-c3121d59fce1)
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx:
##########
@@ -196,6 +232,47 @@
[setLabel, simpleColumns],
);
+ const onSimpleMetricChange = useCallback(
+ selectedMetricName => {
+ const selectedMetric = availableMetrics.find(
+ metric => metric.metric_name === selectedMetricName,
+ );
+ setSelectedCalculatedColumn(undefined);
+ setSelectedSimpleColumn(undefined);
+ setSelectedMetric(selectedMetric);
+ setAdhocColumn(undefined);
+ setLabel(
+ selectedMetric?.verbose_name || selectedMetric?.metric_name || '',
+ );
+ },
+ [setLabel, availableMetrics],
+ );
+
+ const onSimpleItemChange = useCallback(
+ selectedValue => {
+ const selectedColumn = simpleColumns.find(
+ col => col.column_name === selectedValue,
+ );
+ if (selectedColumn) {
+ onSimpleColumnChange(selectedValue);
+ return;
+ }
+
+ const selectedMetric = availableMetrics.find(
+ metric => metric.metric_name === selectedValue,
+ );
Review Comment:
### Inefficient Item Lookup Pattern <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple sequential linear searches (find operations) are performed on
arrays when looking up items by value.
###### Why this matters
Linear searches through arrays on every selection change can impact
performance, especially with larger datasets, as each find operation is O(n).
###### Suggested change ∙ *Feature Preview*
Create lookup maps for faster access:
```typescript
const columnMap = useMemo(() =>
Object.fromEntries(simpleColumns.map(col => [col.column_name, col])),
[simpleColumns]
);
const metricMap = useMemo(() =>
Object.fromEntries(availableMetrics.map(metric => [metric.metric_name,
metric])),
[availableMetrics]
);
const selectedColumn = columnMap[selectedValue];
const selectedMetric = metricMap[selectedValue];
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/739f6e04-94c4-472d-994c-656f94723497/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/739f6e04-94c4-472d-994c-656f94723497?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/739f6e04-94c4-472d-994c-656f94723497?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/739f6e04-94c4-472d-994c-656f94723497?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/739f6e04-94c4-472d-994c-656f94723497)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:60ec1665-2547-4f0e-ac6f-9c580bfd8638 -->
[](60ec1665-2547-4f0e-ac6f-9c580bfd8638)
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx:
##########
@@ -157,11 +182,20 @@
[columns],
);
+ // Filter metrics that are already selected in the chart
+ const availableMetrics = useMemo(() => {
+ if (!metrics?.length) return [];
+ return metrics.filter(metric =>
+ selectedMetrics.includes(metric.metric_name),
+ );
+ }, [metrics, selectedMetrics]);
Review Comment:
### Inefficient Metric Filtering Algorithm <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The .includes() method inside filter has O(n) complexity, making the overall
filtering operation O(n^2) when dealing with large arrays.
###### Why this matters
When dealing with large datasets, this nested loop operation could cause
significant performance degradation during metric filtering.
###### Suggested change ∙ *Feature Preview*
Convert selectedMetrics to a Set for O(1) lookup time:
```typescript
const availableMetrics = useMemo(() => {
if (!metrics?.length) return [];
const selectedMetricsSet = new Set(selectedMetrics);
return metrics.filter(metric =>
selectedMetricsSet.has(metric.metric_name));
}, [metrics, selectedMetrics]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c03b53e-de6c-43f0-82e7-d5bf9f761e7f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c03b53e-de6c-43f0-82e7-d5bf9f761e7f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c03b53e-de6c-43f0-82e7-d5bf9f761e7f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c03b53e-de6c-43f0-82e7-d5bf9f761e7f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c03b53e-de6c-43f0-82e7-d5bf9f761e7f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a2b5e697-88d4-45b4-8436-77b49f861341 -->
[](a2b5e697-88d4-45b4-8436-77b49f861341)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Grid/Grid.tsx:
##########
@@ -31,19 +30,20 @@ import { hexToRGB } from '../../utils/colors';
import { createDeckGLComponent } from '../../factory';
import TooltipRow from '../../TooltipRow';
import { TooltipProps } from '../../components/Tooltip';
+import {
+ createTooltipContent,
+ CommonTooltipRows,
+} from '../../utilities/tooltipUtils';
+
+function defaultTooltipGenerator(o: JsonObject, formData: QueryFormData) {
+ const metricLabel = formData.size?.label || formData.size?.value || 'Height';
-function setTooltipContent(o: JsonObject) {
return (
<div className="deckgl-tooltip">
+ {CommonTooltipRows.centroid(o)}
<TooltipRow
- // eslint-disable-next-line prefer-template
- label={t('Longitude and Latitude') + ': '}
- value={`${o.coordinate[0]}, ${o.coordinate[1]}`}
- />
- <TooltipRow
- // eslint-disable-next-line prefer-template
- label={t('Height') + ': '}
- value={`${o.object.elevationValue}`}
+ label={`${metricLabel}: `}
+ value={`${o.object?.elevationValue || o.object?.value || 'N/A'}`}
Review Comment:
### Incorrect Tooltip Value Fallback <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The tooltip value fallback logic might display incorrect data. While
elevationValue is the primary field for grid layers, falling back to a generic
'value' field could show unrelated data.
###### Why this matters
Grid layers specifically use elevationValue for their metrics. Falling back
to a generic 'value' property could display misleading information to users
when elevationValue is undefined.
###### Suggested change ∙ *Feature Preview*
Remove the fallback to generic 'value' field and only use the grid-specific
field:
```typescript
value={`${o.object?.elevationValue ?? 'N/A'}`}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a8434a59-c627-4575-8729-d1c6d8d197fa/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a8434a59-c627-4575-8729-d1c6d8d197fa?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a8434a59-c627-4575-8729-d1c6d8d197fa?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a8434a59-c627-4575-8729-d1c6d8d197fa?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a8434a59-c627-4575-8729-d1c6d8d197fa)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:91ad29c0-4e5f-4354-9d98-906b08d47b3e -->
[](91ad29c0-4e5f-4354-9d98-906b08d47b3e)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx:
##########
@@ -400,3 +400,73 @@
}),
},
};
+
+export const tooltipContents = {
+ name: 'tooltip_contents',
+ config: {
+ type: 'DndColumnMetricSelect',
+ label: t('Tooltip contents'),
+ multi: true,
+ freeForm: true,
+ clearable: true,
+ default: [],
+ description: t(
+ 'Drag columns and metrics here to customize tooltip content. Order
matters - items will appear in the same order in tooltips. Click the button to
manually select columns and metrics.',
+ ),
+ ghostButtonText: t('Drop columns/metrics here or click'),
+ disabledTabs: new Set(['saved', 'sqlExpression']),
+ mapStateToProps: state => {
+ const { datasource, form_data: formData } = state;
+
+ const selectedMetrics = [];
+
+ if (formData) {
+ // Scan form_data for FixedOrMetricControl patterns
+ Object.values(formData).forEach(value => {
+ if (value?.type === 'metric' && value?.value) {
+ selectedMetrics.push(value.value);
+ }
+ });
Review Comment:
### Extract Metric Collection Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The metrics extraction logic in tooltipContents.mapStateToProps is mixed
with data transformation, making it hard to maintain and test.
###### Why this matters
This tightly coupled implementation makes it difficult to modify the metric
extraction logic independently and complicates unit testing of the individual
transformation steps.
###### Suggested change ∙ *Feature Preview*
Extract the metric collection logic into a separate pure function:
```javascript
const extractMetricsFromFormData = (formData) => {
const metrics = [];
// Extract from controls
Object.values(formData).forEach(value => {
if (value?.type === 'metric' && value?.value) {
metrics.push(value.value);
}
});
// Extract from metrics field
if (formData.metrics) {
metrics.push(...(Array.isArray(formData.metrics) ? formData.metrics :
[formData.metrics]));
}
// Extract from point radius
if (formData.point_radius_fixed?.value) {
metrics.push(formData.point_radius_fixed.value);
}
return [...new Set(metrics)];
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e26b721-b286-4c71-aa4f-dffc39d0f301/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e26b721-b286-4c71-aa4f-dffc39d0f301?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e26b721-b286-4c71-aa4f-dffc39d0f301?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e26b721-b286-4c71-aa4f-dffc39d0f301?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e26b721-b286-4c71-aa4f-dffc39d0f301)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:07e01f43-3f8e-404b-a0f0-7a85358e2b00 -->
[](07e01f43-3f8e-404b-a0f0-7a85358e2b00)
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx:
##########
@@ -157,11 +182,20 @@ const ColumnSelectPopover = ({
[columns],
);
+ // Filter metrics that are already selected in the chart
+ const availableMetrics = useMemo(() => {
+ if (!metrics?.length) return [];
+ return metrics.filter(metric =>
+ selectedMetrics.includes(metric.metric_name),
+ );
Review Comment:
### Incorrect Metric Filtering Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The metrics filtering logic is inversed, showing only metrics that are
already selected instead of available ones to select.
###### Why this matters
Users will not be able to select new metrics as the dropdown only shows
metrics that are already selected in the chart, contrary to the intended
functionality of selecting additional metrics.
###### Suggested change ∙ *Feature Preview*
Change the filtering condition to show unselected metrics:
```typescript
return metrics.filter(metric =>
!selectedMetrics.includes(metric.metric_name),
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5fb9517-cc3f-48cc-a7a1-bc6aa3076071/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5fb9517-cc3f-48cc-a7a1-bc6aa3076071?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5fb9517-cc3f-48cc-a7a1-bc6aa3076071?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5fb9517-cc3f-48cc-a7a1-bc6aa3076071?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5fb9517-cc3f-48cc-a7a1-bc6aa3076071)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:263c2d11-b75d-484d-8f42-80e15e32c805 -->
[](263c2d11-b75d-484d-8f42-80e15e32c805)
--
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]
