codeant-ai-for-open-source[bot] commented on code in PR #36201:
URL: https://github.com/apache/superset/pull/36201#discussion_r2600469028
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/controlPanel.ts:
##########
@@ -36,8 +41,27 @@ import {
lineWidth,
tooltipContents,
tooltipTemplate,
+ jsFunctionControl,
} from '../../utilities/Shared_DeckGL';
import { dndGeojsonColumn } from '../../utilities/sharedDndControls';
+import { BLACK_COLOR } from '../../utilities/controls';
+
+const defaultLabelConfigGenerator = `() => ({
+ // Check the documentation at:
+ //
https://deck.gl/docs/api-reference/layers/geojson-layer#pointtype-options-2
+ getText: f => f.properties.name,
Review Comment:
**Suggestion:** The default label JS generator string uses `getText: f =>
f.properties.name` which will throw if `f` or `f.properties` is undefined when
the generated function is executed; guard access with optional chaining or a
fallback to avoid runtime TypeError when features lack a `properties` object.
[null pointer]
**Severity Level:** Minor ⚠️
```suggestion
getText: f => (f && f.properties ? f.properties.name : ''),
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggested change fixes a real runtime hazard: the default JS snippet
will be executed in user code (deck.gl callbacks) and can throw if f or
f.properties is undefined. Making the getter resilient (optional chaining or a
fallback) prevents TypeErrors when features lack properties. The improved code
addresses the issue directly.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/controlPanel.ts
**Line:** 52:52
**Comment:**
*Null Pointer: The default label JS generator string uses `getText: f
=> f.properties.name` which will throw if `f` or `f.properties` is undefined
when the generated function is executed; guard access with optional chaining or
a fallback to avoid runtime TypeError when features lack a `properties` object.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.test.ts:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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 { SqlaFormData } from '@superset-ui/core';
+import {
+ computeGeoJsonTextOptionsFromJsOutput,
+ computeGeoJsonTextOptionsFromFormData,
+ computeGeoJsonIconOptionsFromJsOutput,
+ computeGeoJsonIconOptionsFromFormData,
+} from './Geojson';
+
+jest.mock('@deck.gl/react', () => ({
+ __esModule: true,
+ default: () => null,
+}));
+
+test('computeGeoJsonTextOptionsFromJsOutput returns an empty object for
non-object input', () => {
+ expect(computeGeoJsonTextOptionsFromJsOutput(null)).toEqual({});
+ expect(computeGeoJsonTextOptionsFromJsOutput(42)).toEqual({});
+ expect(computeGeoJsonTextOptionsFromJsOutput([1, 2, 3])).toEqual({});
+ expect(computeGeoJsonTextOptionsFromJsOutput('string')).toEqual({});
+});
+
+test('computeGeoJsonTextOptionsFromJsOutput extracts valid text options from
the input object', () => {
+ const input = {
+ getText: 'name',
+ getTextColor: [1, 2, 3, 255],
+ invalidOption: true,
+ };
+ const expectedOutput = {
+ getText: 'name',
+ getTextColor: [1, 2, 3, 255],
+ };
+ expect(computeGeoJsonTextOptionsFromJsOutput(input)).toEqual(expectedOutput);
+});
+
+test('computeGeoJsonTextOptionsFromFormData computes text options based on
form data', () => {
+ const formData: SqlaFormData = {
+ label_property_name: 'name',
+ label_color: { r: 1, g: 2, b: 3, a: 1 },
+ label_size: 123,
+ label_size_unit: 'pixels',
+ datasource: 'test_datasource',
+ viz_type: 'deck_geojson',
+ };
+
+ const expectedOutput = {
+ getText: expect.any(Function),
+ getTextColor: [1, 2, 3, 255],
+ getTextSize: 123,
+ textSizeUnits: 'pixels',
+ };
+
+ const actualOutput = computeGeoJsonTextOptionsFromFormData(formData);
+ expect(actualOutput).toEqual(expectedOutput);
+
+ const sampleFeature = { properties: { name: 'Test' } };
+ expect(actualOutput.getText(sampleFeature)).toBe('Test');
+});
+
+test('computeGeoJsonIconOptionsFromJsOutput returns an empty object for
non-object input', () => {
+ expect(computeGeoJsonIconOptionsFromJsOutput(null)).toEqual({});
+ expect(computeGeoJsonIconOptionsFromJsOutput(42)).toEqual({});
+ expect(computeGeoJsonIconOptionsFromJsOutput([1, 2, 3])).toEqual({});
+ expect(computeGeoJsonIconOptionsFromJsOutput('string')).toEqual({});
+});
+
+test('computeGeoJsonIconOptionsFromJsOutput extracts valid icon options from
the input object', () => {
+ const input = {
+ getIcon: 'icon_name',
+ getIconColor: [1, 2, 3, 255],
+ invalidOption: false,
+ };
+
+ const expectedOutput = {
+ getIcon: 'icon_name',
+ getIconColor: [1, 2, 3, 255],
+ };
+
+ expect(computeGeoJsonIconOptionsFromJsOutput(input)).toEqual(expectedOutput);
+});
+
+test('computeGeoJsonIconOptionsFromFormData computes icon options based on
form data', () => {
+ const formData: SqlaFormData = {
+ icon_url: 'https://example.com/icon.png',
+ icon_size: 123,
+ icon_size_unit: 'pixels',
+ datasource: 'test_datasource',
+ viz_type: 'deck_geojson',
+ };
+
+ const expectedOutput = {
+ getIcon: expect.any(Function),
+ getIconSize: 123,
+ iconSizeUnits: 'pixels',
+ };
+
+ const actualOutput = computeGeoJsonIconOptionsFromFormData(formData);
+ expect(actualOutput).toEqual(expectedOutput);
Review Comment:
**Suggestion:** Fragile equality assertion for icon options: asserting exact
equality on the whole returned object will fail if the implementation returns
additional properties (or different numeric sizing behavior); assert the actual
object contains the expected subset instead of strict equality. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
expect(actualOutput).toEqual(expect.objectContaining(expectedOutput));
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Similar to the text options test, using
expect.objectContaining(expectedOutput) reduces brittleness if the
implementation returns extra harmless properties.
This is a reasonable improvement for test maintainability and doesn't weaken
checks for the keys we care about.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.test.ts
**Line:** 114:114
**Comment:**
*Possible Bug: Fragile equality assertion for icon options: asserting
exact equality on the whole returned object will fail if the implementation
returns additional properties (or different numeric sizing behavior); assert
the actual object contains the expected subset instead of strict equality.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/controls.ts:
##########
@@ -39,6 +39,7 @@ export function columnChoices(datasource: Dataset |
QueryResponse | null) {
}
export const PRIMARY_COLOR = { r: 0, g: 122, b: 135, a: 1 };
+export const BLACK_COLOR = { r: 0, g: 0, b: 0, a: 1 };
Review Comment:
**Suggestion:** Exported color object is mutable; because `BLACK_COLOR` is a
shared object, other modules can accidentally modify its properties at runtime
leading to subtle state/visualization bugs—freeze the object to make it
immutable. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
export const BLACK_COLOR = Object.freeze({ r: 0, g: 0, b: 0, a: 1 }) as
const;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Freezing the object prevents accidental runtime mutation of a shared
constant which can cause subtle UI/state bugs. The proposed Object.freeze(... )
as const is syntactically correct and reasonable here. Note that the change
tightens the exported type to readonly, which may surface compile errors if any
consumer mutates the object — that's a real safety win, not a functional bugfix
but a protective change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/controls.ts
**Line:** 42:42
**Comment:**
*Logic Error: Exported color object is mutable; because `BLACK_COLOR`
is a shared object, other modules can accidentally modify its properties at
runtime leading to subtle state/visualization bugs—freeze the object to make it
immutable.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.tsx:
##########
@@ -169,6 +279,38 @@ export const getLayer: GetLayerType<GeoJsonLayer> =
function ({
processedFeatures = jsFnMutator(features) as ProcessedFeature[];
Review Comment:
**Suggestion:** The code passes the module-level `features` array directly
into a user-provided mutator (`jsFnMutator(features)`), allowing the user
function to mutate shared state and causing race conditions if multiple layers
run concurrently; pass a defensive copy (shallow or deep depending on
expectations) to avoid accidental mutation of module-level state. [race
condition]
**Severity Level:** Minor ⚠️
```suggestion
// pass a copy to avoid mutation of module-level `features` and reduce
race conditions
const featuresCopy = features.slice();
processedFeatures = jsFnMutator(featuresCopy) as ProcessedFeature[];
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Correct — handing the module-level features array directly to arbitrary user
code lets that code mutate shared state (features) and can create surprising
bugs or race conditions if multiple calls interleave. Passing a copy (at least
a shallow copy) is a cheap and sensible defense that prevents accidental
mutation of the module-level array.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.tsx
**Line:** 279:279
**Comment:**
*Race Condition: The code passes the module-level `features` array
directly into a user-provided mutator (`jsFnMutator(features)`), allowing the
user function to mutate shared state and causing race conditions if multiple
layers run concurrently; pass a defensive copy (shallow or deep depending on
expectations) to avoid accidental mutation of module-level state.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.test.ts:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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 { SqlaFormData } from '@superset-ui/core';
+import {
+ computeGeoJsonTextOptionsFromJsOutput,
+ computeGeoJsonTextOptionsFromFormData,
+ computeGeoJsonIconOptionsFromJsOutput,
+ computeGeoJsonIconOptionsFromFormData,
+} from './Geojson';
+
+jest.mock('@deck.gl/react', () => ({
+ __esModule: true,
+ default: () => null,
+}));
+
+test('computeGeoJsonTextOptionsFromJsOutput returns an empty object for
non-object input', () => {
+ expect(computeGeoJsonTextOptionsFromJsOutput(null)).toEqual({});
+ expect(computeGeoJsonTextOptionsFromJsOutput(42)).toEqual({});
+ expect(computeGeoJsonTextOptionsFromJsOutput([1, 2, 3])).toEqual({});
+ expect(computeGeoJsonTextOptionsFromJsOutput('string')).toEqual({});
+});
+
+test('computeGeoJsonTextOptionsFromJsOutput extracts valid text options from
the input object', () => {
+ const input = {
+ getText: 'name',
+ getTextColor: [1, 2, 3, 255],
+ invalidOption: true,
+ };
+ const expectedOutput = {
+ getText: 'name',
+ getTextColor: [1, 2, 3, 255],
+ };
+ expect(computeGeoJsonTextOptionsFromJsOutput(input)).toEqual(expectedOutput);
+});
+
+test('computeGeoJsonTextOptionsFromFormData computes text options based on
form data', () => {
+ const formData: SqlaFormData = {
+ label_property_name: 'name',
+ label_color: { r: 1, g: 2, b: 3, a: 1 },
+ label_size: 123,
+ label_size_unit: 'pixels',
+ datasource: 'test_datasource',
+ viz_type: 'deck_geojson',
+ };
+
+ const expectedOutput = {
+ getText: expect.any(Function),
+ getTextColor: [1, 2, 3, 255],
+ getTextSize: 123,
+ textSizeUnits: 'pixels',
+ };
+
+ const actualOutput = computeGeoJsonTextOptionsFromFormData(formData);
+ expect(actualOutput).toEqual(expectedOutput);
Review Comment:
**Suggestion:** Fragile equality assertion: using a strict object equality
with an expected object that contains an asymmetric matcher
(`expect.any(Function)`) will fail if the actual output contains additional
valid properties; assert that the actual object contains the expected keys
instead of requiring exact equality. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
expect(actualOutput).toEqual(expect.objectContaining(expectedOutput));
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The recommendation to use expect.objectContaining makes the test more
resilient to harmless additions to the returned object.
It's a legitimate improvement to reduce brittle tests and won't hide an
actual regression in the asserted keys. The improved code directly replaces the
two-line assertion and is syntactically correct.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.test.ts
**Line:** 70:70
**Comment:**
*Possible Bug: Fragile equality assertion: using a strict object
equality with an expected object that contains an asymmetric matcher
(`expect.any(Function)`) will fail if the actual output contains additional
valid properties; assert that the actual object contains the expected keys
instead of requiring exact equality.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.tsx:
##########
@@ -169,6 +279,38 @@ export const getLayer: GetLayerType<GeoJsonLayer> =
function ({
processedFeatures = jsFnMutator(features) as ProcessedFeature[];
}
+ let pointType = 'circle';
+ if (fd.enable_labels) {
+ pointType = `${pointType}+text`;
+ }
+ if (fd.enable_icons) {
+ pointType = `${pointType}+icon`;
+ }
+
+ let labelOpts: Partial<GeoJsonLayerProps> = {};
+ if (fd.enable_labels) {
+ if (fd.enable_label_javascript_mode) {
+ const generator = sandboxedEval(fd.label_javascript_config_generator);
+ if (typeof generator === 'function') {
Review Comment:
**Suggestion:** Evaluating user-provided JavaScript with `sandboxedEval` and
immediately calling `generator()` is executed without error handling; if the
sandboxed evaluation or the user generator throws, the whole layer construction
will throw and break rendering. Wrap `sandboxedEval` and `generator()` calls in
a try/catch and fallback to a safe default (e.g. empty options) to prevent
unhandled exceptions from user code. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
try {
const generator =
sandboxedEval(fd.label_javascript_config_generator);
if (typeof generator === 'function') {
const out = generator();
labelOpts = computeGeoJsonTextOptionsFromJsOutput(out);
}
} catch (e) {
// eslint-disable-next-line no-console
console.error('Failed to evaluate label javascript generator:', e);
labelOpts = {};
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid and practical improvement — sandboxedEval or the generator()
invocation can throw and that would bubble up and break layer construction /
rendering. Wrapping evaluation and invocation in try/catch and falling back to
an empty options object avoids user code from taking down the layer. The
suggested patch is straightforward and addresses a real runtime risk introduced
by executing user-supplied code.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.tsx
**Line:** 293:294
**Comment:**
*Possible Bug: Evaluating user-provided JavaScript with `sandboxedEval`
and immediately calling `generator()` is executed without error handling; if
the sandboxed evaluation or the user generator throws, the whole layer
construction will throw and break rendering. Wrap `sandboxedEval` and
`generator()` calls in a try/catch and fallback to a safe default (e.g. empty
options) to prevent unhandled exceptions from user code.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
