diffray-bot commented on code in PR #36890:
URL: https://github.com/apache/superset/pull/36890#discussion_r2658251016
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.tsx:
##########
@@ -44,9 +44,29 @@ function setTooltipContent(
verboseMap?: Record<string, string>,
) {
const defaultTooltipGenerator = (o: JsonObject) => {
- const label =
- verboseMap?.[formData.point_radius_fixed.value] ||
- getMetricLabel(formData.point_radius_fixed?.value);
+ // Only show metric info if point_radius_fixed is metric-based
+ let metricKey = null;
+ if (
+ formData.point_radius_fixed &&
+ typeof formData.point_radius_fixed === 'object' &&
+ formData.point_radius_fixed.type === 'metric'
+ ) {
+ const metricValue = formData.point_radius_fixed.value;
+ if (typeof metricValue === 'string') {
Review Comment:
🟠 **HIGH** - Unsafe 'any' type for metric parameter
**Agent:** typescript
**Category:** quality
**Description:**
The getMetricLabel function uses 'any' type for the metric parameter, which
bypasses TypeScript's compile-time type checking. This allows invalid data to
be passed without detection.
**Suggestion:**
Define a proper union type: type MetricInput = string | { label?: string;
verbose_name?: string; value?: string } | undefined
**Why this matters:** Weak typing defeats TypeScript's safety guarantees.
**Confidence:** 85%
**Rule:** `ts_any_type_usage`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.tsx:
##########
@@ -44,9 +44,29 @@ function setTooltipContent(
verboseMap?: Record<string, string>,
) {
const defaultTooltipGenerator = (o: JsonObject) => {
- const label =
- verboseMap?.[formData.point_radius_fixed.value] ||
- getMetricLabel(formData.point_radius_fixed?.value);
+ // Only show metric info if point_radius_fixed is metric-based
+ let metricKey = null;
+ if (
+ formData.point_radius_fixed &&
+ typeof formData.point_radius_fixed === 'object' &&
+ formData.point_radius_fixed.type === 'metric'
+ ) {
+ const metricValue = formData.point_radius_fixed.value;
+ if (typeof metricValue === 'string') {
Review Comment:
🟠 **HIGH** - Reimplemented metric label extraction duplicates
@superset-ui/core
**Agent:** [consistency](https://diffray.ai/agents/consistency)
**Category:** quality
**Description:**
Local getMetricLabel function duplicates functionality from
@superset-ui/core.getMetricLabel which is imported in transformUtils.ts. The
custom implementation is less robust than the core library's function.
**Suggestion:**
Import and use getMetricLabelFromFormData from transformUtils.ts or directly
use getMetricLabel from @superset-ui/core for consistent behavior.
**Why this matters:** Improves code quality and reliability.
**Confidence:** 80%
**Rule:** `quality_reinventing_wheel`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/transformUtils.ts:
##########
@@ -134,9 +134,27 @@ export function addPropertiesToFeature<T extends
Record<string, unknown>>(
}
export function getMetricLabelFromFormData(
- metric: string | { value?: string } | undefined,
+ metric:
+ | string
+ | { value?: string | number | object; type?: string }
+ | undefined,
): string | undefined {
if (!metric) return undefined;
if (typeof metric === 'string') return getMetricLabel(metric);
- return metric.value ? getMetricLabel(metric.value) : undefined;
+ // Only return metric label if it's a metric type, not a fixed value
+ if (typeof metric === 'object' && metric.type === 'metric' && metric.value) {
+ if (typeof metric.value === 'string') {
+ return getMetricLabel(metric.value);
+ }
+ if (typeof metric.value === 'object' && metric.value) {
+ // Handle object metrics (adhoc or saved metrics)
+ const metricObj = metric.value as any;
Review Comment:
🟠 **HIGH** - Unsafe 'as any' type assertion
**Agent:** react
**Category:** quality
**Description:**
Line 151 uses an unsafe type assertion 'as any' to convert metric.value to
an unspecified type. This bypasses TypeScript's type checking and defeats the
purpose of type safety.
**Suggestion:**
Replace 'const metricObj = metric.value as any;' with proper type checking.
Define an interface for metric objects and validate the structure before
accessing properties.
**Why this matters:** Weak typing defeats TypeScript's safety guarantees.
**Confidence:** 85%
**Rule:** `ts_avoid_unsafe_type_assertions`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.tsx:
##########
@@ -44,9 +44,29 @@ function setTooltipContent(
verboseMap?: Record<string, string>,
) {
const defaultTooltipGenerator = (o: JsonObject) => {
- const label =
- verboseMap?.[formData.point_radius_fixed.value] ||
- getMetricLabel(formData.point_radius_fixed?.value);
+ // Only show metric info if point_radius_fixed is metric-based
+ let metricKey = null;
+ if (
+ formData.point_radius_fixed &&
+ typeof formData.point_radius_fixed === 'object' &&
+ formData.point_radius_fixed.type === 'metric'
+ ) {
+ const metricValue = formData.point_radius_fixed.value;
+ if (typeof metricValue === 'string') {
Review Comment:
🟡 **MEDIUM** - getMetricLabel uses 'any' type and lacks JSDoc
**Agent:** [quality](https://diffray.ai/agents/quality)
**Category:** quality
**Description:**
The helper function uses 'any' type for metric parameter without
documentation explaining expected shapes.
**Suggestion:**
Define proper type for metric parameter (string | { label?: string;
verbose_name?: string; value?: string }) and add JSDoc.
**Confidence:** 75%
**Rule:** `jsdoc_incomplete_api_documentation`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/transformProps.test.ts:
##########
@@ -0,0 +1,303 @@
+/**
+ * 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 { ChartProps, DatasourceType } from '@superset-ui/core';
+import transformProps from './transformProps';
+
+interface ScatterFeature {
+ position: [number, number];
+ radius?: number;
+ metric?: number;
+ cat_color?: string;
+ extraProps?: Record<string, unknown>;
+}
+
+jest.mock('../spatialUtils', () => ({
+ ...jest.requireActual('../spatialUtils'),
+ getMapboxApiKey: jest.fn(() => 'mock-mapbox-key'),
+}));
+
+const mockChartProps: Partial<ChartProps> = {
+ rawFormData: {
+ spatial: {
+ type: 'latlong',
+ latCol: 'LATITUDE',
+ lonCol: 'LONGITUDE',
+ },
+ viewport: {},
+ },
+ queriesData: [
+ {
+ data: [
+ {
+ LATITUDE: 37.8,
+ LONGITUDE: -122.4,
+ population: 50000,
+ 'AVG(radius_value)': 100,
+ },
+ {
+ LATITUDE: 37.9,
+ LONGITUDE: -122.3,
+ population: 75000,
+ 'AVG(radius_value)': 200,
+ },
+ ],
+ },
+ ],
+ datasource: {
+ type: DatasourceType.Table,
+ id: 1,
+ name: 'test_datasource',
+ columns: [],
+ metrics: [],
+ },
+ height: 400,
+ width: 600,
+ hooks: {},
+ filterState: {},
+ emitCrossFilters: false,
+};
+
+test('Scatter transformProps should use fixed radius value when
point_radius_fixed type is "fix"', () => {
+ const fixedProps = {
+ ...mockChartProps,
+ rawFormData: {
+ ...mockChartProps.rawFormData,
+ point_radius_fixed: {
+ type: 'fix',
+ value: '1000',
+ },
+ },
+ };
+
+ const result = transformProps(fixedProps as ChartProps);
+ const features = result.payload.data.features as ScatterFeature[];
+
+ expect(features).toHaveLength(2);
+ expect(features[0]?.radius).toBe(1000);
+ expect(features[1]?.radius).toBe(1000);
+ // metric should not be set for fixed radius
+ expect(features[0]?.metric).toBeUndefined();
+ expect(features[1]?.metric).toBeUndefined();
+});
Review Comment:
🟡 **MEDIUM** - Missing position field verification in test
**Agent:** [testing](https://diffray.ai/agents/testing)
**Category:** quality
**Description:**
Test verifies radius and metric properties but doesn't verify that position
field is correctly populated from input coordinates.
**Suggestion:**
Add assertion to verify position matches input coordinates:
expect(features[0]?.position).toEqual([expect.any(Number), expect.any(Number)])
**Confidence:** 65%
**Rule:** `test_comprehensive_assertions`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/transformProps.ts:
##########
@@ -72,7 +73,15 @@ function processScatterData(
extraProps: feature.extraProps || {},
Review Comment:
🟡 **MEDIUM** - Unnecessary conditional check after type narrowing
**Agent:** refactoring
**Category:** quality
**Description:**
The ternary at line 57 checking if 'spatial' exists is redundant after the
early return at line 48.
**Suggestion:**
Remove the ternary and always spread the spatial columns since spatial is
guaranteed to exist at that point: `[spatial.lonCol, spatial.latCol,
spatial.lonlatCol, spatial.geohashCol].filter(Boolean)`
**Confidence:** 85%
**Rule:** `quality_redundant_condition`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/transformUtils.ts:
##########
@@ -134,9 +134,27 @@ export function addPropertiesToFeature<T extends
Record<string, unknown>>(
}
export function getMetricLabelFromFormData(
- metric: string | { value?: string } | undefined,
+ metric:
+ | string
+ | { value?: string | number | object; type?: string }
+ | undefined,
): string | undefined {
if (!metric) return undefined;
if (typeof metric === 'string') return getMetricLabel(metric);
- return metric.value ? getMetricLabel(metric.value) : undefined;
+ // Only return metric label if it's a metric type, not a fixed value
+ if (typeof metric === 'object' && metric.type === 'metric' && metric.value) {
+ if (typeof metric.value === 'string') {
+ return getMetricLabel(metric.value);
+ }
+ if (typeof metric.value === 'object' && metric.value) {
+ // Handle object metrics (adhoc or saved metrics)
+ const metricObj = metric.value as any;
+ const metricKey =
+ metricObj.label || metricObj.sqlExpression || metricObj.value;
+ if (typeof metricKey === 'string') {
+ return getMetricLabel(metricKey);
+ }
+ }
Review Comment:
🟡 **MEDIUM** - Redundant null/object type check after earlier logic
**Agent:** refactoring
**Category:** quality
**Description:**
Line 149 checks 'typeof metric.value === 'object' && metric.value' but
metric.value was already verified truthy at line 145.
**Suggestion:**
Remove the second 'metric.value' check: change line 149 to just 'if (typeof
metric.value === 'object')'
**Confidence:** 80%
**Rule:** `quality_redundant_condition`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/transformUtils.ts:
##########
@@ -134,9 +134,27 @@ export function addPropertiesToFeature<T extends
Record<string, unknown>>(
}
export function getMetricLabelFromFormData(
- metric: string | { value?: string } | undefined,
+ metric:
+ | string
Review Comment:
🔵 **LOW** - Use Object.entries() instead of Object.keys()
**Agent:** react
**Category:** quality
**Description:**
The addPropertiesToFeature function iterates over keys and then accesses
record[key], causing redundant lookups. Object.entries() provides both in one
iteration.
**Suggestion:**
Replace Object.keys().forEach() with Object.entries().forEach():
Object.entries(record).forEach(([key, value]) => { if (!excludeKeys.has(key)) {
result[key] = value; } });
**Confidence:** 65%
**Rule:** `ts_optimize_chained_array_operations`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.tsx:
##########
@@ -44,9 +44,29 @@ function setTooltipContent(
verboseMap?: Record<string, string>,
) {
const defaultTooltipGenerator = (o: JsonObject) => {
- const label =
- verboseMap?.[formData.point_radius_fixed.value] ||
- getMetricLabel(formData.point_radius_fixed?.value);
+ // Only show metric info if point_radius_fixed is metric-based
+ let metricKey = null;
+ if (
+ formData.point_radius_fixed &&
+ typeof formData.point_radius_fixed === 'object' &&
+ formData.point_radius_fixed.type === 'metric'
+ ) {
+ const metricValue = formData.point_radius_fixed.value;
+ if (typeof metricValue === 'string') {
+ metricKey = metricValue;
+ } else if (typeof metricValue === 'object' && metricValue) {
+ // Handle object metrics (adhoc or saved metrics)
+ metricKey =
+ metricValue.label || metricValue.sqlExpression || metricValue.value;
+ }
+ }
Review Comment:
🟡 **MEDIUM** - Metric key extraction logic duplicated from transformUtils
**Agent:** [consistency](https://diffray.ai/agents/consistency)
**Category:** quality
**Description:**
The metric extraction logic in setTooltipContent (lines 49-62) is similar to
getMetricLabelFromFormData in transformUtils.ts (lines 145-157). Both handle
the same pattern of checking type === 'metric' and extracting from
label/sqlExpression/value.
**Suggestion:**
Import getMetricLabelFromFormData from transformUtils.ts and use it to
extract the metric key, eliminating duplicate logic.
**Why this matters:** Improves code quality and reliability.
**Confidence:** 75%
**Rule:** `quality_reinventing_wheel`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts:
##########
@@ -78,15 +79,28 @@ export default function buildQuery(formData:
DeckScatterFormData) {
columns = withJsColumns as QueryFormColumn[];
Review Comment:
🟡 **MEDIUM** - Type assertion without validation
**Agent:** typescript
**Category:** quality
**Description:**
withJsColumns is cast as QueryFormColumn[] without runtime validation.
addJsColumnsToColumns returns string[], which is being cast to
QueryFormColumn[] (which can be string | object).
**Suggestion:**
Ensure addJsColumnsToColumns returns proper QueryFormColumn[] type, or
validate before casting. The function maps strings, so result should already be
string[] which is compatible.
**Confidence:** 70%
**Rule:** `typescript_avoid_type_assertions`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/transformProps.ts:
##########
@@ -98,14 +107,20 @@ export default function transformProps(chartProps:
ChartProps) {
const { spatial, point_radius_fixed, dimension, js_columns } =
Review Comment:
🟡 **MEDIUM** - Unsafe type assertion on feature object
**Agent:** typescript
**Category:** bug
**Description:**
Feature from processSpatialData is cast as DataRecord. processSpatialData
returns SpatialPoint[] (with position: [number, number], weight: number, and
[key: string]: unknown). DataRecord expects [key: string]: string | number |
null | undefined.
**Suggestion:**
Use a type guard or create a union type that properly represents the spatial
feature with position array. The position property ([number, number]) doesn't
match DataRecord's value type.
**Confidence:** 75%
**Rule:** `typescript_avoid_type_assertions`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/transformUtils.test.ts:
##########
@@ -0,0 +1,184 @@
+/**
+ * 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 { getMetricLabel } from '@superset-ui/core';
+import { getMetricLabelFromFormData, parseMetricValue } from
'./transformUtils';
+
+jest.mock('@superset-ui/core', () => ({
+ ...jest.requireActual('@superset-ui/core'),
+ getMetricLabel: jest.fn((metric: string) => metric),
+}));
+
+beforeEach(() => {
+ jest.clearAllMocks();
+});
+
+test('getMetricLabelFromFormData should return undefined for undefined input',
() => {
+ const result = getMetricLabelFromFormData(undefined);
+ expect(result).toBeUndefined();
+});
+
+test('getMetricLabelFromFormData should return undefined for null input', ()
=> {
+ const result = getMetricLabelFromFormData(null as any);
+ expect(result).toBeUndefined();
+});
+
+test('getMetricLabelFromFormData should handle string metric directly', () => {
+ const result = getMetricLabelFromFormData('AVG(value)');
+ expect(result).toBe('AVG(value)');
+ expect(getMetricLabel).toHaveBeenCalledWith('AVG(value)');
+});
+
+test('getMetricLabelFromFormData should return undefined for fixed type', ()
=> {
+ const result = getMetricLabelFromFormData({
+ type: 'fix',
+ value: '1000',
+ });
+ expect(result).toBeUndefined();
+ expect(getMetricLabel).not.toHaveBeenCalled();
+});
+
+test('getMetricLabelFromFormData should return undefined for fixed type with
numeric value', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'fix',
+ value: 1000,
+ });
+ expect(result).toBeUndefined();
+ expect(getMetricLabel).not.toHaveBeenCalled();
+});
+
+test('getMetricLabelFromFormData should return metric label for metric type
with string value', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'metric',
+ value: 'SUM(amount)',
+ });
+ expect(result).toBe('SUM(amount)');
+ expect(getMetricLabel).toHaveBeenCalledWith('SUM(amount)');
+});
+
+test('getMetricLabelFromFormData should handle object metric values', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'metric',
+ value: {
+ label: 'Total Sales',
+ sqlExpression: 'SUM(sales)',
+ },
+ });
+ expect(result).toBe('Total Sales');
+ expect(getMetricLabel).toHaveBeenCalledWith('Total Sales');
+});
+
+test('getMetricLabelFromFormData should use sqlExpression if label is
missing', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'metric',
+ value: {
+ sqlExpression: 'COUNT(*)',
+ },
+ });
+ expect(result).toBe('COUNT(*)');
+ expect(getMetricLabel).toHaveBeenCalledWith('COUNT(*)');
+});
+
+test('getMetricLabelFromFormData should use value field as fallback', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'metric',
+ value: {
+ value: 'AVG(price)',
+ },
+ });
+ expect(result).toBe('AVG(price)');
+ expect(getMetricLabel).toHaveBeenCalledWith('AVG(price)');
+});
+
+test('getMetricLabelFromFormData should return undefined for metric type with
numeric value', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'metric',
+ value: 123,
+ });
+ expect(result).toBeUndefined();
+ expect(getMetricLabel).not.toHaveBeenCalled();
+});
+
+test('getMetricLabelFromFormData should return undefined for object without
type', () => {
+ const result = getMetricLabelFromFormData({
+ value: 'AVG(value)',
+ });
+ expect(result).toBeUndefined();
+});
+
+test('getMetricLabelFromFormData should return undefined for empty object', ()
=> {
+ const result = getMetricLabelFromFormData({});
+ expect(result).toBeUndefined();
+});
+
+test('getMetricLabelFromFormData should return undefined for metric type
without value', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'metric',
+ });
+ expect(result).toBeUndefined();
+});
+
+test('parseMetricValue should parse numeric strings', () => {
+ expect(parseMetricValue('123')).toBe(123);
+ expect(parseMetricValue('123.45')).toBe(123.45);
+ expect(parseMetricValue('0')).toBe(0);
+ expect(parseMetricValue('-123')).toBe(-123);
+});
+
+test('parseMetricValue should handle numbers directly', () => {
+ expect(parseMetricValue(123)).toBe(123);
+ expect(parseMetricValue(123.45)).toBe(123.45);
+ expect(parseMetricValue(0)).toBe(0);
+ expect(parseMetricValue(-123)).toBe(-123);
+});
+
+test('parseMetricValue should return undefined for null', () => {
+ expect(parseMetricValue(null)).toBeUndefined();
+});
+
+test('parseMetricValue should return undefined for undefined', () => {
+ expect(parseMetricValue(undefined)).toBeUndefined();
+});
+
+test('parseMetricValue should return undefined for non-numeric strings', () =>
{
+ expect(parseMetricValue('abc')).toBeUndefined();
+ expect(parseMetricValue('12a34')).toBe(12); // parseFloat returns 12
+ expect(parseMetricValue('')).toBeUndefined();
+});
+
+test('parseMetricValue should handle edge cases', () => {
+ expect(parseMetricValue('Infinity')).toBe(Infinity);
+ expect(parseMetricValue('-Infinity')).toBe(-Infinity);
+ expect(parseMetricValue('NaN')).toBeUndefined();
+});
+
+test('parseMetricValue should handle boolean values', () => {
+ expect(parseMetricValue(true as any)).toBeUndefined();
+ expect(parseMetricValue(false as any)).toBeUndefined();
+});
+
+test('parseMetricValue should handle objects', () => {
+ expect(parseMetricValue({} as any)).toBeUndefined();
+ expect(parseMetricValue({ value: 123 } as any)).toBeUndefined();
+});
+
+test('parseMetricValue should handle arrays', () => {
+ expect(parseMetricValue([] as any)).toBeUndefined();
+ expect(parseMetricValue([123] as any)).toBe(123); // String([123]) = '123'
+});
Review Comment:
🔵 **LOW** - Test documents unexpected behavior with inline comment
**Agent:** [testing](https://diffray.ai/agents/testing)
**Category:** quality
**Description:**
Test 'parseMetricValue should handle arrays' expects [123] to return 123 via
String([123]) conversion. This relies on JavaScript's implicit array-to-string
coercion.
**Suggestion:**
Either document this as intentional behavior for backwards compatibility, or
consider if arrays should return undefined. The comment explains behavior but
not intent.
**Confidence:** 62%
**Rule:** `test_single_purpose_focus`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/transformUtils.test.ts:
##########
@@ -0,0 +1,184 @@
+/**
+ * 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 { getMetricLabel } from '@superset-ui/core';
+import { getMetricLabelFromFormData, parseMetricValue } from
'./transformUtils';
+
+jest.mock('@superset-ui/core', () => ({
+ ...jest.requireActual('@superset-ui/core'),
+ getMetricLabel: jest.fn((metric: string) => metric),
+}));
+
+beforeEach(() => {
+ jest.clearAllMocks();
+});
+
+test('getMetricLabelFromFormData should return undefined for undefined input',
() => {
+ const result = getMetricLabelFromFormData(undefined);
+ expect(result).toBeUndefined();
+});
+
+test('getMetricLabelFromFormData should return undefined for null input', ()
=> {
+ const result = getMetricLabelFromFormData(null as any);
+ expect(result).toBeUndefined();
Review Comment:
🟡 **MEDIUM** - Unnecessary 'as any' type assertion in test
**Agent:** typescript
**Category:** quality
**Description:**
Test uses 'null as any' to bypass type checking at line 38. The function
getMetricLabelFromFormData accepts undefined but not null per its type
signature.
**Suggestion:**
The function signature allows undefined but not null. Either update the
function signature to accept null explicitly, or test with undefined instead of
forcing null via 'as any'
**Confidence:** 72%
**Rule:** `ts_any_type_usage`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.test.ts:
##########
@@ -0,0 +1,294 @@
+/**
+ * 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 buildQuery, { DeckScatterFormData } from './buildQuery';
+
+const baseFormData: DeckScatterFormData = {
+ datasource: '1__table',
+ viz_type: 'deck_scatter',
+ spatial: {
+ type: 'latlong',
+ latCol: 'LATITUDE',
+ lonCol: 'LONGITUDE',
+ },
+ row_limit: 100,
+};
+
+test('Scatter buildQuery should not include metric when point_radius_fixed is
fixed type', () => {
+ const formData: DeckScatterFormData = {
+ ...baseFormData,
+ point_radius_fixed: {
+ type: 'fix',
+ value: '1000',
+ },
+ };
+
+ const queryContext = buildQuery(formData);
+ const [query] = queryContext.queries;
+
+ expect(query.metrics).toEqual([]);
+ expect(query.orderby).toEqual([]);
+});
+
+test('Scatter buildQuery should include metric when point_radius_fixed is
metric type', () => {
+ const formData: DeckScatterFormData = {
+ ...baseFormData,
+ point_radius_fixed: {
+ type: 'metric',
+ value: 'AVG(radius_value)',
+ },
+ };
+
+ const queryContext = buildQuery(formData);
+ const [query] = queryContext.queries;
+
+ expect(query.metrics).toContain('AVG(radius_value)');
+ expect(query.orderby).toEqual([['AVG(radius_value)', false]]);
+});
+
+test('Scatter buildQuery should handle numeric value in fixed type', () => {
+ const formData: DeckScatterFormData = {
+ ...baseFormData,
+ point_radius_fixed: {
+ type: 'fix',
+ value: 500,
+ },
+ };
+
+ const queryContext = buildQuery(formData);
+ const [query] = queryContext.queries;
+
+ // Fixed numeric value should not be included as a metric
+ expect(query.metrics).toEqual([]);
+ expect(query.orderby).toEqual([]);
+});
+
+test('Scatter buildQuery should handle missing point_radius_fixed', () => {
+ const formData: DeckScatterFormData = {
+ ...baseFormData,
+ // no point_radius_fixed
+ };
+
+ const queryContext = buildQuery(formData);
+ const [query] = queryContext.queries;
+
+ expect(query.metrics).toEqual([]);
+ expect(query.orderby).toEqual([]);
+});
+
+test('Scatter buildQuery should include spatial columns in query', () => {
+ const formData: DeckScatterFormData = {
+ ...baseFormData,
+ point_radius_fixed: {
+ type: 'fix',
+ value: '1000',
+ },
+ };
+
+ const queryContext = buildQuery(formData);
+ const [query] = queryContext.queries;
+
+ expect(query.columns).toContain('LATITUDE');
+ expect(query.columns).toContain('LONGITUDE');
+});
+
+test('Scatter buildQuery should include dimension column when specified', ()
=> {
+ const formData: DeckScatterFormData = {
+ ...baseFormData,
+ dimension: 'category',
+ point_radius_fixed: {
+ type: 'fix',
+ value: '1000',
+ },
+ };
+
+ const queryContext = buildQuery(formData);
+ const [query] = queryContext.queries;
+
+ expect(query.columns).toContain('category');
+});
+
+test('Scatter buildQuery should add spatial null filters', () => {
+ const formData: DeckScatterFormData = {
+ ...baseFormData,
+ point_radius_fixed: {
+ type: 'fix',
+ value: '1000',
+ },
+ };
+
+ const queryContext = buildQuery(formData);
+ const [query] = queryContext.queries;
+
+ const latFilter = query.filters?.find(
+ f => f.col === 'LATITUDE' && f.op === 'IS NOT NULL',
+ );
+ const lonFilter = query.filters?.find(
+ f => f.col === 'LONGITUDE' && f.op === 'IS NOT NULL',
+ );
+
+ expect(latFilter).toBeDefined();
+ expect(lonFilter).toBeDefined();
+});
Review Comment:
🟡 **MEDIUM** - Incomplete filter assertion verification
**Agent:** [testing](https://diffray.ai/agents/testing)
**Category:** quality
**Description:**
Test verifies spatial null filters exist but only checks they're defined,
not their complete structure. The filter should have op: 'IS NOT NULL' and col
properties verified.
**Suggestion:**
Add assertions for filter.op === 'IS NOT NULL' explicitly and verify total
filter count to catch regressions that might add/remove filters
**Confidence:** 60%
**Rule:** `test_incomplete_assertions`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/transformUtils.test.ts:
##########
@@ -0,0 +1,184 @@
+/**
+ * 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 { getMetricLabel } from '@superset-ui/core';
+import { getMetricLabelFromFormData, parseMetricValue } from
'./transformUtils';
+
+jest.mock('@superset-ui/core', () => ({
+ ...jest.requireActual('@superset-ui/core'),
+ getMetricLabel: jest.fn((metric: string) => metric),
+}));
+
+beforeEach(() => {
+ jest.clearAllMocks();
+});
+
+test('getMetricLabelFromFormData should return undefined for undefined input',
() => {
+ const result = getMetricLabelFromFormData(undefined);
+ expect(result).toBeUndefined();
+});
+
+test('getMetricLabelFromFormData should return undefined for null input', ()
=> {
+ const result = getMetricLabelFromFormData(null as any);
+ expect(result).toBeUndefined();
+});
+
+test('getMetricLabelFromFormData should handle string metric directly', () => {
+ const result = getMetricLabelFromFormData('AVG(value)');
+ expect(result).toBe('AVG(value)');
+ expect(getMetricLabel).toHaveBeenCalledWith('AVG(value)');
+});
+
+test('getMetricLabelFromFormData should return undefined for fixed type', ()
=> {
+ const result = getMetricLabelFromFormData({
+ type: 'fix',
+ value: '1000',
+ });
+ expect(result).toBeUndefined();
+ expect(getMetricLabel).not.toHaveBeenCalled();
+});
+
+test('getMetricLabelFromFormData should return undefined for fixed type with
numeric value', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'fix',
+ value: 1000,
+ });
+ expect(result).toBeUndefined();
+ expect(getMetricLabel).not.toHaveBeenCalled();
+});
+
+test('getMetricLabelFromFormData should return metric label for metric type
with string value', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'metric',
+ value: 'SUM(amount)',
+ });
+ expect(result).toBe('SUM(amount)');
+ expect(getMetricLabel).toHaveBeenCalledWith('SUM(amount)');
+});
+
+test('getMetricLabelFromFormData should handle object metric values', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'metric',
+ value: {
+ label: 'Total Sales',
+ sqlExpression: 'SUM(sales)',
+ },
+ });
+ expect(result).toBe('Total Sales');
+ expect(getMetricLabel).toHaveBeenCalledWith('Total Sales');
+});
+
+test('getMetricLabelFromFormData should use sqlExpression if label is
missing', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'metric',
+ value: {
+ sqlExpression: 'COUNT(*)',
+ },
+ });
+ expect(result).toBe('COUNT(*)');
+ expect(getMetricLabel).toHaveBeenCalledWith('COUNT(*)');
+});
+
+test('getMetricLabelFromFormData should use value field as fallback', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'metric',
+ value: {
+ value: 'AVG(price)',
+ },
+ });
+ expect(result).toBe('AVG(price)');
+ expect(getMetricLabel).toHaveBeenCalledWith('AVG(price)');
+});
+
+test('getMetricLabelFromFormData should return undefined for metric type with
numeric value', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'metric',
+ value: 123,
+ });
+ expect(result).toBeUndefined();
+ expect(getMetricLabel).not.toHaveBeenCalled();
+});
+
+test('getMetricLabelFromFormData should return undefined for object without
type', () => {
+ const result = getMetricLabelFromFormData({
+ value: 'AVG(value)',
+ });
+ expect(result).toBeUndefined();
+});
+
+test('getMetricLabelFromFormData should return undefined for empty object', ()
=> {
+ const result = getMetricLabelFromFormData({});
+ expect(result).toBeUndefined();
+});
+
+test('getMetricLabelFromFormData should return undefined for metric type
without value', () => {
+ const result = getMetricLabelFromFormData({
+ type: 'metric',
+ });
+ expect(result).toBeUndefined();
+});
+
+test('parseMetricValue should parse numeric strings', () => {
+ expect(parseMetricValue('123')).toBe(123);
+ expect(parseMetricValue('123.45')).toBe(123.45);
+ expect(parseMetricValue('0')).toBe(0);
+ expect(parseMetricValue('-123')).toBe(-123);
+});
+
+test('parseMetricValue should handle numbers directly', () => {
+ expect(parseMetricValue(123)).toBe(123);
+ expect(parseMetricValue(123.45)).toBe(123.45);
+ expect(parseMetricValue(0)).toBe(0);
+ expect(parseMetricValue(-123)).toBe(-123);
+});
+
+test('parseMetricValue should return undefined for null', () => {
+ expect(parseMetricValue(null)).toBeUndefined();
+});
+
+test('parseMetricValue should return undefined for undefined', () => {
+ expect(parseMetricValue(undefined)).toBeUndefined();
+});
+
+test('parseMetricValue should return undefined for non-numeric strings', () =>
{
+ expect(parseMetricValue('abc')).toBeUndefined();
+ expect(parseMetricValue('12a34')).toBe(12); // parseFloat returns 12
+ expect(parseMetricValue('')).toBeUndefined();
+});
Review Comment:
🟡 **MEDIUM** - Test documents parseFloat truncation quirk
**Agent:** [testing](https://diffray.ai/agents/testing)
**Category:** quality
**Description:**
Test shows parseMetricValue('12a34') returns 12 due to parseFloat behavior.
This edge case is documented with a comment but could be a separate explicit
test.
**Suggestion:**
Create a dedicated test for the parseFloat truncation behavior with
documentation explaining why this is acceptable behavior.
**Confidence:** 70%
**Rule:** `test_single_purpose_focus`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/transformProps.test.ts:
##########
@@ -0,0 +1,303 @@
+/**
+ * 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 { ChartProps, DatasourceType } from '@superset-ui/core';
+import transformProps from './transformProps';
+
+interface ScatterFeature {
+ position: [number, number];
+ radius?: number;
+ metric?: number;
+ cat_color?: string;
+ extraProps?: Record<string, unknown>;
+}
+
+jest.mock('../spatialUtils', () => ({
+ ...jest.requireActual('../spatialUtils'),
+ getMapboxApiKey: jest.fn(() => 'mock-mapbox-key'),
+}));
+
+const mockChartProps: Partial<ChartProps> = {
+ rawFormData: {
+ spatial: {
+ type: 'latlong',
+ latCol: 'LATITUDE',
+ lonCol: 'LONGITUDE',
+ },
+ viewport: {},
+ },
+ queriesData: [
+ {
+ data: [
+ {
+ LATITUDE: 37.8,
+ LONGITUDE: -122.4,
+ population: 50000,
+ 'AVG(radius_value)': 100,
+ },
+ {
+ LATITUDE: 37.9,
+ LONGITUDE: -122.3,
+ population: 75000,
+ 'AVG(radius_value)': 200,
+ },
+ ],
+ },
+ ],
+ datasource: {
+ type: DatasourceType.Table,
+ id: 1,
+ name: 'test_datasource',
+ columns: [],
+ metrics: [],
+ },
+ height: 400,
+ width: 600,
+ hooks: {},
+ filterState: {},
+ emitCrossFilters: false,
+};
+
+test('Scatter transformProps should use fixed radius value when
point_radius_fixed type is "fix"', () => {
+ const fixedProps = {
+ ...mockChartProps,
+ rawFormData: {
+ ...mockChartProps.rawFormData,
+ point_radius_fixed: {
+ type: 'fix',
+ value: '1000',
+ },
+ },
+ };
+
+ const result = transformProps(fixedProps as ChartProps);
+ const features = result.payload.data.features as ScatterFeature[];
+
+ expect(features).toHaveLength(2);
+ expect(features[0]?.radius).toBe(1000);
+ expect(features[1]?.radius).toBe(1000);
+ // metric should not be set for fixed radius
+ expect(features[0]?.metric).toBeUndefined();
+ expect(features[1]?.metric).toBeUndefined();
+});
+
+test('Scatter transformProps should use metric value for radius when
point_radius_fixed type is "metric"', () => {
+ const metricProps = {
+ ...mockChartProps,
+ rawFormData: {
+ ...mockChartProps.rawFormData,
+ point_radius_fixed: {
+ type: 'metric',
+ value: 'AVG(radius_value)',
+ },
+ },
+ };
+
+ const result = transformProps(metricProps as ChartProps);
+ const features = result.payload.data.features as ScatterFeature[];
+
+ expect(features).toHaveLength(2);
+ expect(features[0]?.radius).toBe(100);
+ expect(features[0]?.metric).toBe(100);
+ expect(features[1]?.radius).toBe(200);
+ expect(features[1]?.metric).toBe(200);
+});
+
+test('Scatter transformProps should handle numeric fixed radius value', () => {
+ const fixedProps = {
+ ...mockChartProps,
+ rawFormData: {
+ ...mockChartProps.rawFormData,
+ point_radius_fixed: {
+ type: 'fix',
+ value: 500, // numeric value
+ },
+ },
+ };
+
+ const result = transformProps(fixedProps as ChartProps);
+ const features = result.payload.data.features as ScatterFeature[];
+
+ expect(features).toHaveLength(2);
+ expect(features[0]?.radius).toBe(500);
+ expect(features[1]?.radius).toBe(500);
+});
+
+test('Scatter transformProps should handle missing point_radius_fixed', () => {
+ const propsWithoutRadius = {
+ ...mockChartProps,
+ rawFormData: {
+ ...mockChartProps.rawFormData,
+ // no point_radius_fixed
+ },
+ };
+
+ const result = transformProps(propsWithoutRadius as ChartProps);
+ const features = result.payload.data.features as ScatterFeature[];
+
+ expect(features).toHaveLength(2);
+ // radius should not be set
+ expect(features[0]?.radius).toBeUndefined();
+ expect(features[1]?.radius).toBeUndefined();
+});
+
+test('Scatter transformProps should handle dimension for category colors', ()
=> {
+ const propsWithDimension = {
+ ...mockChartProps,
Review Comment:
🟡 **MEDIUM** - Test behavior contradicts test description
**Agent:** [testing](https://diffray.ai/agents/testing)
**Category:** quality
**Description:**
Test 'parseMetricValue should return undefined for non-numeric strings' at
line 161 of transformUtils.test.ts actually asserts parseMetricValue('12a34')
returns 12, documenting parseFloat behavior but with misleading test name.
**Suggestion:**
Split into two tests: one for pure non-numeric strings ('abc', '') that
return undefined, and another documenting parseFloat's prefix-parsing behavior
for '12a34'
**Confidence:** 85%
**Rule:** `test_behavior_based_naming`
<sub>Review ID: `b316da0d-db4c-4c2e-a34a-c8a46a35f436`</sub>
<sub>Rate it 👍 or 👎 to improve future reviews | Powered by <a
href="https://diffray.ai?utm_source=github-private-issue";>diffray</a></sub>
--
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]
