korbit-ai[bot] commented on code in PR #34276:
URL: https://github.com/apache/superset/pull/34276#discussion_r2231527898
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx:
##########
@@ -400,3 +400,83 @@ export const geojsonColumn = {
}),
},
};
+
+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)];
Review Comment:
### Missing Metric Value Validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The extractMetricsFromFormData function doesn't validate if metrics array
contains null or undefined values before returning.
###### Why this matters
If any metric value is null or undefined, it could cause issues when using
these metrics in the tooltip template rendering.
###### Suggested change ∙ *Feature Preview*
Add validation to filter out null or undefined metrics:
```javascript
return [...new Set(metrics)].filter(metric => metric != null);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cb54e1f1-0db9-4dec-a08c-056b0efb5ae3/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cb54e1f1-0db9-4dec-a08c-056b0efb5ae3?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cb54e1f1-0db9-4dec-a08c-056b0efb5ae3?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cb54e1f1-0db9-4dec-a08c-056b0efb5ae3?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cb54e1f1-0db9-4dec-a08c-056b0efb5ae3)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5f734e71-6568-463a-b973-97a20cdca8d5 -->
[](5f734e71-6568-463a-b973-97a20cdca8d5)
##########
superset-frontend/src/explore/components/controls/TextAreaControl.jsx:
##########
@@ -73,16 +79,28 @@ const defaultProps = {
textAreaStyles: {},
tooltipOptions: {},
hotkeys: [],
+ debounceDelay: null,
};
class TextAreaControl extends Component {
+ constructor(props) {
+ super(props);
+ if (props.debounceDelay) {
+ this.debouncedOnChange = debounce(props.onChange, props.debounceDelay);
+ }
+ }
+
onControlChange(event) {
const { value } = event.target;
this.props.onChange(value);
}
onAreaEditorChange(value) {
- this.props.onChange(value);
+ if (this.debouncedOnChange) {
+ this.debouncedOnChange(value);
+ } else {
+ this.props.onChange(value);
+ }
Review Comment:
### Missing Debounce Cleanup <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The debounced function is created in the constructor but never cleaned up,
which could lead to memory leaks and stale callbacks being executed after
component unmounting.
###### Why this matters
Without proper cleanup, the debounced function might continue to exist in
memory and execute even after the component is unmounted, potentially causing
memory leaks or calling callbacks on unmounted components.
###### Suggested change ∙ *Feature Preview*
Add componentWillUnmount lifecycle method to cancel debounced callbacks:
```jsx
componentWillUnmount() {
if (this.debouncedOnChange) {
this.debouncedOnChange.cancel();
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d77cbb95-6099-40a2-adbd-f26562f9696f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d77cbb95-6099-40a2-adbd-f26562f9696f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d77cbb95-6099-40a2-adbd-f26562f9696f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d77cbb95-6099-40a2-adbd-f26562f9696f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d77cbb95-6099-40a2-adbd-f26562f9696f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2e7bbfca-0910-47b5-8649-427bd23a8c0d -->
[](2e7bbfca-0910-47b5-8649-427bd23a8c0d)
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/index.ts:
##########
@@ -16,7 +16,16 @@
* specific language governing permissions and limitations
* under the License.
*/
+/**
+ * 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
+ */
Review Comment:
### Missing Purpose in Component Documentation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The documentation only lists what each component is, without explaining the
purpose or use cases of these DnD controls in the context of the application.
###### Why this matters
Without understanding the purpose of these controls, developers may misuse
them or have difficulty choosing the appropriate component for their use case.
###### Suggested change ∙ *Feature Preview*
/**
* Export DnD (Drag and Drop) control components for building interactive
chart controls.
* These components enable users to configure visualizations by dragging and
dropping
* available columns, metrics, and filters.
*
* - DndSelectLabel: Base label component for DnD controls
* - DndColumnSelect: Column selection control for choosing dataset columns
* - DndFilterSelect: Filter selection control for data filtering
* - DndMetricSelect: Metric selection control for aggregations
* - DndColumnMetricSelect: Combined column and metric selection control
*/
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/196e42e7-1490-4e71-b854-96dc0e8bf1e2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/196e42e7-1490-4e71-b854-96dc0e8bf1e2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/196e42e7-1490-4e71-b854-96dc0e8bf1e2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/196e42e7-1490-4e71-b854-96dc0e8bf1e2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/196e42e7-1490-4e71-b854-96dc0e8bf1e2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ce246938-a6ae-4ec0-bb2b-af271f31c1dc -->
[](ce246938-a6ae-4ec0-bb2b-af271f31c1dc)
##########
superset-frontend/src/explore/components/controls/TextAreaControl.jsx:
##########
@@ -27,6 +28,9 @@ import {
} from '@superset-ui/core/components';
import { t, withTheme } from '@superset-ui/core';
+// Import Ace Editor modes
+import 'ace-builds/src-min-noconflict/mode-handlebars';
Review Comment:
### Redundant Import Comment <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The comment 'Import Ace Editor modes' is redundant as it simply states what
the code does without providing any valuable context.
###### Why this matters
Self-explanatory comments that just restate the code reduce signal-to-noise
ratio and make the codebase harder to maintain.
###### Suggested change ∙ *Feature Preview*
Remove the comment line `// Import Ace Editor modes` as the import statement
is self-documenting.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a078e9c6-f8e3-4cfc-acdb-4eb894f96d2d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a078e9c6-f8e3-4cfc-acdb-4eb894f96d2d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a078e9c6-f8e3-4cfc-acdb-4eb894f96d2d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a078e9c6-f8e3-4cfc-acdb-4eb894f96d2d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a078e9c6-f8e3-4cfc-acdb-4eb894f96d2d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:1b54b1b1-8864-4567-9931-24b5bcae2c74 -->
[](1b54b1b1-8864-4567-9931-24b5bcae2c74)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/multiValueUtils.ts:
##########
@@ -0,0 +1,150 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { QueryFormData } from '@superset-ui/core';
+
+/**
+ * Deck.gl chart types that perform aggregation and can have multiple values
in tooltips
+ */
+export const AGGREGATED_DECK_GL_CHART_TYPES = [
+ 'deck_screengrid',
+ 'deck_heatmap',
+ 'deck_contour',
+ 'deck_hex',
+ 'deck_grid',
+];
+
+/**
+ * Chart types that do NOT aggregate data (each tooltip shows single data
point)
+ */
+export const NON_AGGREGATED_DECK_GL_CHART_TYPES = [
+ 'deck_scatter',
+ 'deck_arc',
+ 'deck_path',
+ 'deck_polygon',
+ 'deck_geojson',
+];
+
+/**
+ * Determines if a deck.gl chart type aggregates data points
+ */
+export function isAggregatedDeckGLChart(vizType: string): boolean {
+ return AGGREGATED_DECK_GL_CHART_TYPES.includes(vizType);
+}
+
+/**
+ * Determines if a tooltip content field might contain multiple values
+ * for the given chart configuration
+ */
+export function fieldHasMultipleValues(
+ item: any,
+ formData: QueryFormData,
+): boolean {
Review Comment:
### Untyped parameter reduces type safety <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function accepts an untyped 'any' parameter which violates type safety
and makes the code harder to maintain.
###### Why this matters
Using 'any' bypasses TypeScript's type checking system, making the code more
prone to runtime errors and harder to refactor.
###### Suggested change ∙ *Feature Preview*
```typescript
interface TooltipItem {
item_type?: 'metric' | 'column';
column_name?: string;
metric_name?: string;
label?: string;
}
export function fieldHasMultipleValues(
item: TooltipItem | string,
formData: QueryFormData,
): boolean {
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdc777bf-8030-4379-838d-0123a93f85d8/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdc777bf-8030-4379-838d-0123a93f85d8?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdc777bf-8030-4379-838d-0123a93f85d8?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdc777bf-8030-4379-838d-0123a93f85d8?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdc777bf-8030-4379-838d-0123a93f85d8)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:985df841-6cf5-4971-ac13-cd0c7dfb671a -->
[](985df841-6cf5-4971-ac13-cd0c7dfb671a)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/multiValueUtils.ts:
##########
@@ -0,0 +1,150 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { QueryFormData } from '@superset-ui/core';
+
+/**
+ * Deck.gl chart types that perform aggregation and can have multiple values
in tooltips
+ */
+export const AGGREGATED_DECK_GL_CHART_TYPES = [
+ 'deck_screengrid',
+ 'deck_heatmap',
+ 'deck_contour',
+ 'deck_hex',
+ 'deck_grid',
+];
+
+/**
+ * Chart types that do NOT aggregate data (each tooltip shows single data
point)
+ */
+export const NON_AGGREGATED_DECK_GL_CHART_TYPES = [
+ 'deck_scatter',
+ 'deck_arc',
+ 'deck_path',
+ 'deck_polygon',
+ 'deck_geojson',
+];
+
+/**
+ * Determines if a deck.gl chart type aggregates data points
+ */
+export function isAggregatedDeckGLChart(vizType: string): boolean {
+ return AGGREGATED_DECK_GL_CHART_TYPES.includes(vizType);
+}
+
+/**
+ * Determines if a tooltip content field might contain multiple values
+ * for the given chart configuration
+ */
+export function fieldHasMultipleValues(
+ item: any,
+ formData: QueryFormData,
+): boolean {
+ // Only aggregated deck.gl charts can have multiple values
+ if (!isAggregatedDeckGLChart(formData.viz_type)) {
+ return false;
+ }
+
+ // Skip metrics for now - they are typically aggregated already
+ if (item?.item_type === 'metric') {
+ return false;
+ }
+
+ // Only screengrid reliably supports multi-value fields with individual
point access
+ // Other aggregated charts (hex, grid, heatmap, contour) use different
aggregation methods
+ const supportsMultiValue = ['deck_screengrid'].includes(formData.viz_type);
+
+ if (!supportsMultiValue) {
+ return false;
+ }
+
+ // Columns in aggregated charts can have multiple values
+ if (item?.item_type === 'column') {
+ return true;
+ }
+
+ // String columns can have multiple values
+ if (typeof item === 'string') {
+ return true;
+ }
+
+ return false;
+}
+
+/**
+ * Creates a default template that limits multi-value fields
+ */
+export function createDefaultTemplateWithLimits(
+ tooltipContents: any[],
+ formData: QueryFormData,
+): string {
Review Comment:
### Function violates SRP with embedded helpers <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Function accepts an array of 'any' type and contains helper functions that
could be extracted to improve maintainability.
###### Why this matters
The function violates the Single Responsibility Principle by containing
helper functions and handling multiple concerns within a single function scope.
###### Suggested change ∙ *Feature Preview*
```typescript
// Extract helper functions to module level
const getFieldName = (item: TooltipItem | string): string | null => {
// ... implementation
};
const getFieldLabel = (item: TooltipItem | string): string => {
// ... implementation
};
export function createDefaultTemplateWithLimits(
tooltipContents: TooltipItem[],
formData: QueryFormData,
): string {
// Main function logic without embedded helpers
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fd1679d2-73c7-4037-91b5-e350141087cd/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fd1679d2-73c7-4037-91b5-e350141087cd?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fd1679d2-73c7-4037-91b5-e350141087cd?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fd1679d2-73c7-4037-91b5-e350141087cd?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fd1679d2-73c7-4037-91b5-e350141087cd)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:fb3f4113-ff33-4e1d-9641-a6f4da0b41d4 -->
[](fb3f4113-ff33-4e1d-9641-a6f4da0b41d4)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx:
##########
@@ -400,3 +400,83 @@ export const geojsonColumn = {
}),
},
};
+
+const extractMetricsFromFormData = formData => {
+ const metrics = [];
+ // Extract from controls
+ Object.values(formData).forEach(value => {
+ if (value?.type === 'metric' && value?.value) {
+ metrics.push(value.value);
+ }
+ });
Review Comment:
### Inefficient Form Data Iteration <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using Object.values() followed by forEach to iterate through all form data
properties is inefficient, especially since we're only interested in specific
properties.
###### Why this matters
This approach unnecessarily iterates through all form data properties,
including those that cannot possibly be metrics, leading to wasted CPU cycles
on large form datasets.
###### Suggested change ∙ *Feature Preview*
Replace with direct property access for known metric locations or use
Object.entries() with early continue statements:
```javascript
const extractMetricsFromFormData = formData => {
const metrics = new Set();
// Direct access to known metric locations
if (formData.metrics) {
(Array.isArray(formData.metrics) ? formData.metrics : [formData.metrics])
.forEach(metric => metrics.add(metric));
}
if (formData.point_radius_fixed?.value) {
metrics.add(formData.point_radius_fixed.value);
}
// For dynamic controls, only check relevant properties
for (const [key, value] of Object.entries(formData)) {
if (!value || typeof value !== 'object') continue;
if (value.type === 'metric' && value.value) {
metrics.add(value.value);
}
}
return Array.from(metrics);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/24fc8506-e62b-46fc-98a2-28c78d9dcc83/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/24fc8506-e62b-46fc-98a2-28c78d9dcc83?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/24fc8506-e62b-46fc-98a2-28c78d9dcc83?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/24fc8506-e62b-46fc-98a2-28c78d9dcc83?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/24fc8506-e62b-46fc-98a2-28c78d9dcc83)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9e81170e-9944-431a-9f41-57e1a6637d44 -->
[](9e81170e-9944-431a-9f41-57e1a6637d44)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/multiValueUtils.ts:
##########
@@ -0,0 +1,150 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { QueryFormData } from '@superset-ui/core';
+
+/**
+ * Deck.gl chart types that perform aggregation and can have multiple values
in tooltips
+ */
+export const AGGREGATED_DECK_GL_CHART_TYPES = [
+ 'deck_screengrid',
+ 'deck_heatmap',
+ 'deck_contour',
+ 'deck_hex',
+ 'deck_grid',
+];
+
+/**
+ * Chart types that do NOT aggregate data (each tooltip shows single data
point)
+ */
+export const NON_AGGREGATED_DECK_GL_CHART_TYPES = [
+ 'deck_scatter',
+ 'deck_arc',
+ 'deck_path',
+ 'deck_polygon',
+ 'deck_geojson',
+];
+
+/**
+ * Determines if a deck.gl chart type aggregates data points
+ */
+export function isAggregatedDeckGLChart(vizType: string): boolean {
+ return AGGREGATED_DECK_GL_CHART_TYPES.includes(vizType);
+}
+
+/**
+ * Determines if a tooltip content field might contain multiple values
+ * for the given chart configuration
+ */
+export function fieldHasMultipleValues(
+ item: any,
+ formData: QueryFormData,
+): boolean {
+ // Only aggregated deck.gl charts can have multiple values
+ if (!isAggregatedDeckGLChart(formData.viz_type)) {
+ return false;
+ }
+
+ // Skip metrics for now - they are typically aggregated already
+ if (item?.item_type === 'metric') {
+ return false;
+ }
+
+ // Only screengrid reliably supports multi-value fields with individual
point access
+ // Other aggregated charts (hex, grid, heatmap, contour) use different
aggregation methods
+ const supportsMultiValue = ['deck_screengrid'].includes(formData.viz_type);
Review Comment:
### Inconsistent Multi-Value Support <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code claims support for multi-value fields only for 'deck_screengrid'
while defining five aggregated chart types, creating an inconsistency between
the defined capability and actual implementation.
###### Why this matters
Users will expect multi-value support for all aggregated chart types but
will find it only works for screengrid, leading to confusion and potential data
visualization issues.
###### Suggested change ∙ *Feature Preview*
Either update the AGGREGATED_DECK_GL_CHART_TYPES to only include
'deck_screengrid' or implement multi-value support for all aggregated chart
types. If temporary, add a TODO comment explaining the limitation:
```typescript
// TODO: Currently only screengrid supports multi-value fields. Support for
other aggregated charts will be added in future releases
const supportsMultiValue = ['deck_screengrid'].includes(formData.viz_type);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1ba9dfa-e12c-4523-9a1e-84be29af95ea/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1ba9dfa-e12c-4523-9a1e-84be29af95ea?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1ba9dfa-e12c-4523-9a1e-84be29af95ea?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1ba9dfa-e12c-4523-9a1e-84be29af95ea?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1ba9dfa-e12c-4523-9a1e-84be29af95ea)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:eaf79b11-d45e-44c3-a94e-ebde82182b0d -->
[](eaf79b11-d45e-44c3-a94e-ebde82182b0d)
##########
superset-frontend/src/explore/components/controls/TextAreaControl.jsx:
##########
@@ -73,16 +79,28 @@ const defaultProps = {
textAreaStyles: {},
tooltipOptions: {},
hotkeys: [],
+ debounceDelay: null,
};
class TextAreaControl extends Component {
+ constructor(props) {
+ super(props);
+ if (props.debounceDelay) {
+ this.debouncedOnChange = debounce(props.onChange, props.debounceDelay);
+ }
+ }
+
onControlChange(event) {
const { value } = event.target;
this.props.onChange(value);
}
onAreaEditorChange(value) {
- this.props.onChange(value);
+ if (this.debouncedOnChange) {
+ this.debouncedOnChange(value);
+ } else {
+ this.props.onChange(value);
+ }
}
Review Comment:
### Duplicate Change Handler Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The component has two similar methods handling changes (onControlChange and
onAreaEditorChange) with different debouncing logic, violating the DRY
principle.
###### Why this matters
Having separate change handlers increases maintenance burden and creates
inconsistent behavior between the text area and editor modes.
###### Suggested change ∙ *Feature Preview*
Unify the change handling logic into a single method:
```javascript
handleChange(value) {
const finalValue = typeof value === 'object' ? value.target.value :
value;
if (this.debouncedOnChange) {
this.debouncedOnChange(finalValue);
} else {
this.props.onChange(finalValue);
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0fd7edfe-d9d1-4c4e-ab66-eaec236fdaa5/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0fd7edfe-d9d1-4c4e-ab66-eaec236fdaa5?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0fd7edfe-d9d1-4c4e-ab66-eaec236fdaa5?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0fd7edfe-d9d1-4c4e-ab66-eaec236fdaa5?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0fd7edfe-d9d1-4c4e-ab66-eaec236fdaa5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f4a2379e-3038-4f7a-b727-d9f88a1df050 -->
[](f4a2379e-3038-4f7a-b727-d9f88a1df050)
##########
superset-frontend/src/explore/components/controls/TextAreaControl.jsx:
##########
@@ -73,16 +79,28 @@ const defaultProps = {
textAreaStyles: {},
tooltipOptions: {},
hotkeys: [],
+ debounceDelay: null,
};
class TextAreaControl extends Component {
+ constructor(props) {
+ super(props);
+ if (props.debounceDelay) {
+ this.debouncedOnChange = debounce(props.onChange, props.debounceDelay);
Review Comment:
### Stale Callback in Debounced Function <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The debounced function is recreated only once in the constructor, which
means it won't update if the onChange prop changes during the component
lifecycle.
###### Why this matters
If the parent component provides a new onChange callback, the debounced
function will continue using the old callback, leading to incorrect behavior.
###### Suggested change ∙ *Feature Preview*
Move the debounce creation to componentDidUpdate and update when props
change:
```jsx
componentDidUpdate(prevProps) {
if (this.props.onChange !== prevProps.onChange &&
this.props.debounceDelay) {
if (this.debouncedOnChange) {
this.debouncedOnChange.cancel();
}
this.debouncedOnChange = debounce(this.props.onChange,
this.props.debounceDelay);
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ead54939-0358-4f3f-9a81-8a84a602920d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ead54939-0358-4f3f-9a81-8a84a602920d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ead54939-0358-4f3f-9a81-8a84a602920d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ead54939-0358-4f3f-9a81-8a84a602920d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ead54939-0358-4f3f-9a81-8a84a602920d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e113b19d-e9f2-45f0-8b94-0912080c2392 -->
[](e113b19d-e9f2-45f0-8b94-0912080c2392)
--
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]
