codeant-ai-for-open-source[bot] commented on code in PR #38035: URL: https://github.com/apache/superset/pull/38035#discussion_r2867691364
########## superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx: ########## @@ -0,0 +1,607 @@ +/** + * 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 { render, screen, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { supersetTheme, ThemeProvider } from '@apache-superset/core/ui'; +import { Provider } from 'react-redux'; +import { configureStore } from '@reduxjs/toolkit'; +import { DatasourceType, SupersetClient } from '@superset-ui/core'; +import DeckMulti from './Multi'; +import * as fitViewportModule from '../utils/fitViewport'; + +// Mock DeckGLContainer +jest.mock('../DeckGLContainer', () => ({ + DeckGLContainerStyledWrapper: ({ viewport, layers }: any) => ( + <div + data-test="deckgl-container" Review Comment: **Suggestion:** The mocked DeckGL container uses a `data-test` attribute while the tests query by `data-testid`, so `getByTestId('deckgl-container')` will never find the element and the tests will consistently fail. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ DeckMulti test suite fails due to missing test id. - ❌ Frontend CI for deck.gl preset remains failing. - ⚠️ Obscures real regressions behind broken tests. ``` </details> ```suggestion data-testid="deckgl-container" ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the Jest test file `superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx` (e.g., `npm test -- Multi.test.tsx`). 2. Jest initializes the mock for `../DeckGLContainer` at lines 29–39, rendering a `<div>` with `data-test="deckgl-container"` instead of `data-testid`. 3. The test `should render DeckGLContainer` at lines 464–469 calls `screen.getByTestId('deckgl-container')`, which searches for an element with `data-testid="deckgl-container"`. 4. Because the rendered mock element only has `data-test` and not `data-testid`, `getByTestId('deckgl-container')` throws, causing this test and other tests using `screen.getByTestId('deckgl-container')` (e.g., lines 213–218, 244–252, 362–373, 575–605) to fail on every run. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx **Line:** 32:32 **Comment:** *Possible Bug: The mocked DeckGL container uses a `data-test` attribute while the tests query by `data-testid`, so `getByTestId('deckgl-container')` will never find the element and the tests will consistently fail. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=7338d038a3129bac75bfcf0709599221f60e6d7aa1518d262dc9e24c435af62e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=7338d038a3129bac75bfcf0709599221f60e6d7aa1518d262dc9e24c435af62e&reaction=dislike'>👎</a> ########## superset-frontend/plugins/preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx: ########## @@ -0,0 +1,343 @@ +/* eslint-disable react/sort-prop-types */ +/* eslint-disable react/require-default-props */ +/* eslint-disable react/no-unused-prop-types */ +/* eslint-disable react/no-access-state-in-setstate */ +/* eslint-disable camelcase */ +/* eslint-disable no-prototype-builtins */ +/** + * 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. + */ +/* eslint no-underscore-dangle: ["error", { "allow": ["", "__timestamp"] }] */ + +import { memo, useCallback, useEffect, useRef, useState } from 'react'; +import { + CategoricalColorNamespace, + Datasource, + FilterState, + HandlerFunction, + JsonObject, + JsonValue, + QueryFormData, + SetDataMaskHook, +} from '@superset-ui/core'; +import type { Layer } from '@deck.gl/core'; +import Legend from './components/Legend'; +import { hexToRGB } from './utils/colors'; +import sandboxedEval from './utils/sandbox'; +import fitViewport, { Viewport } from './utils/fitViewport'; +import { + DeckGLContainerHandle, + DeckGLContainerStyledWrapper, +} from './DeckGLContainer'; +import { GetLayerType } from './factory'; +import { ColorBreakpointType, ColorType, Point } from './types'; +import { TooltipProps } from './components/Tooltip'; +import { COLOR_SCHEME_TYPES, ColorSchemeType } from './utilities/utils'; +import { getColorBreakpointsBuckets } from './utils'; +import { DEFAULT_DECKGL_COLOR } from './utilities/Shared_DeckGL'; + +const { getScale } = CategoricalColorNamespace; + +function getCategories(fd: QueryFormData, data: JsonObject[]) { + const c = fd.color_picker || { r: 0, g: 0, b: 0, a: 1 }; + const fixedColor = [c.r, c.g, c.b, 255 * c.a]; + const appliedScheme = fd.color_scheme; + const colorFn = getScale(appliedScheme); + let categories: Record<any, { color: any; enabled: boolean }> = {}; + + const colorSchemeType = fd.color_scheme_type; + if (colorSchemeType === COLOR_SCHEME_TYPES.color_breakpoints) { + categories = getColorBreakpointsBuckets(fd.color_breakpoints); + } else { + data.forEach(d => { + if (d.cat_color != null && !categories.hasOwnProperty(d.cat_color)) { + let color; + if (fd.dimension) { + color = hexToRGB(colorFn(d.cat_color, fd.sliceId), c.a * 255); + } else { + color = fixedColor; + } + categories[d.cat_color] = { color, enabled: true }; + } + }); + } + + return categories; +} + +export type CategoricalDeckGLContainerProps = { + datasource: Datasource; + formData: QueryFormData; + getPoints: (data: JsonObject[]) => Point[]; + height: number; + width: number; + viewport: Viewport; + getLayer: GetLayerType<unknown>; + getHighlightLayer?: GetLayerType<unknown>; + payload: JsonObject; + onAddFilter?: HandlerFunction; + setControlValue: (control: string, value: JsonValue) => void; + filterState: FilterState; + setDataMask: SetDataMaskHook; + onContextMenu: HandlerFunction; + emitCrossFilters: boolean; +}; + +const CategoricalDeckGLContainer = (props: CategoricalDeckGLContainerProps) => { + const containerRef = useRef<DeckGLContainerHandle>(null); + + const getAdjustedViewport = useCallback(() => { + let viewport = { ...props.viewport }; + if (props.formData.autozoom) { + viewport = fitViewport(viewport, { + width: props.width, + height: props.height, + points: props.getPoints(props.payload.data.features || []), + }); + } + if (viewport.zoom < 0) { + viewport.zoom = 0; + } + return viewport; + }, [props]); + + const [categories, setCategories] = useState<JsonObject>( + getCategories(props.formData, props.payload.data.features || []), + ); + const [stateFormData, setStateFormData] = useState<JsonObject>( + props.payload.form_data, + ); + const [viewport, setViewport] = useState(getAdjustedViewport()); + + useEffect(() => { + if (props.payload.form_data !== stateFormData) { + const features = props.payload.data.features || []; + const categories = getCategories(props.formData, features); + + setViewport(getAdjustedViewport()); + setStateFormData(props.payload.form_data); + setCategories(categories); + } + }, [getAdjustedViewport, props, stateFormData]); + + const setTooltip = useCallback((tooltip: TooltipProps['tooltip']) => { + const { current } = containerRef; + if (current) { + current.setTooltip(tooltip); + } + }, []); + + const addColor = useCallback( + ( + data: JsonObject[], + fd: QueryFormData, + selectedColorScheme: ColorSchemeType, + ) => { + const appliedScheme = fd.color_scheme; + const colorFn = getScale(appliedScheme); + let color: ColorType; + + switch (selectedColorScheme) { + case COLOR_SCHEME_TYPES.fixed_color: { + color = fd.color_picker || { r: 0, g: 0, b: 0, a: 100 }; + const colorArray = [color.r, color.g, color.b, color.a * 255]; + + return data.map(d => ({ ...d, color: colorArray })); + } + case COLOR_SCHEME_TYPES.categorical_palette: { + if (!fd.dimension) { + const fallbackColor = fd.color_picker || { + r: 0, + g: 0, + b: 0, + a: 100, + }; + const colorArray = [ + fallbackColor.r, + fallbackColor.g, + fallbackColor.b, + fallbackColor.a * 255, + ]; + return data.map(d => ({ ...d, color: colorArray })); + } + + return data.map(d => ({ + ...d, + color: hexToRGB(colorFn(d.cat_color, fd.slice_id)), + })); Review Comment: **Suggestion:** The default alpha channel for fixed and fallback colors is set to 100 instead of 1, which after multiplying by 255 produces alpha values far outside the expected 0–255 range and can cause incorrect or undefined rendering of colors in deck.gl layers. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ MapLibre deck.gl charts ignore intended default color transparency. - ⚠️ Fallback categorical palette colors always render fully opaque. - ⚠️ Inconsistent alpha handling versus `getCategories` default (a: 1). ``` </details> ```suggestion case COLOR_SCHEME_TYPES.fixed_color: { color = fd.color_picker || { r: 0, g: 0, b: 0, a: 1 }; const colorArray = [color.r, color.g, color.b, color.a * 255]; return data.map(d => ({ ...d, color: colorArray })); } case COLOR_SCHEME_TYPES.categorical_palette: { if (!fd.dimension) { const fallbackColor = fd.color_picker || { r: 0, g: 0, b: 0, a: 1, }; const colorArray = [ fallbackColor.r, fallbackColor.g, fallbackColor.b, fallbackColor.a * 255, ]; return data.map(d => ({ ...d, color: colorArray })); } return data.map(d => ({ ...d, color: hexToRGB(colorFn(d.cat_color, fd.slice_id)), })); } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the frontend with this PR and create any MapLibre deck.gl chart that uses `CategoricalDeckGLContainer` (e.g. new `preset-chart-deckgl` scatter variant) so that `superset-frontend/plugins/preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx` is used. 2. Configure the chart with a `color_scheme_type` of `fixed_color` in the control panel, and leave the "Fixed color" control unset so that `formData.color_picker` is `undefined` in the client `QueryFormData`. 3. When the chart renders, `getLayers()` in `CategoricalDeckGLContainer.tsx` (around lines 227–283) calls `addColor()` with the current `formData` and data, which executes the `COLOR_SCHEME_TYPES.fixed_color` case starting at line 156. 4. Because `fd.color_picker` is falsy, the default `{ r: 0, g: 0, b: 0, a: 100 }` is used and converted to `[0, 0, 0, 100 * 255]` → `[0, 0, 0, 25500]`; this out-of-range alpha is passed into the deck.gl layer props via `DeckGLContainerStyledWrapper`, causing the alpha to be effectively clamped to full opacity and making it impossible to get the expected semi-transparent default behavior. The same issue occurs for the fallback path in the `COLOR_SCHEME_TYPES.categorical_palette` case (lines 162–176). ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx **Line:** 156:182 **Comment:** *Logic Error: The default alpha channel for fixed and fallback colors is set to 100 instead of 1, which after multiplying by 255 produces alpha values far outside the expected 0–255 range and can cause incorrect or undefined rendering of colors in deck.gl layers. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=2244e11ad5e340aa5abd0126b7076e3a81f291c9be85834c8d6dbedd8be379da&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=2244e11ad5e340aa5abd0126b7076e3a81f291c9be85834c8d6dbedd8be379da&reaction=dislike'>👎</a> ########## superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/Geojson.tsx: ########## @@ -0,0 +1,423 @@ +/** + * 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 { memo, useCallback, useMemo, useRef } from 'react'; +import { GeoJsonLayer, GeoJsonLayerProps } from '@deck.gl/layers'; +// ignoring the eslint error below since typescript prefers 'geojson' to '@types/geojson' +// eslint-disable-next-line import/no-unresolved +import { Feature, Geometry, GeoJsonProperties } from 'geojson'; +import geojsonExtent from '@mapbox/geojson-extent'; +import { + FilterState, + HandlerFunction, + JsonObject, + JsonValue, + QueryFormData, + SetDataMaskHook, + SqlaFormData, +} from '@superset-ui/core'; + +import { + DeckGLContainerHandle, + DeckGLContainerStyledWrapper, +} from '../../DeckGLContainer'; +import { hexToRGB } from '../../utils/colors'; +import sandboxedEval from '../../utils/sandbox'; +import { commonLayerProps } from '../common'; +import TooltipRow from '../../TooltipRow'; +import fitViewport, { Viewport } from '../../utils/fitViewport'; +import { TooltipProps } from '../../components/Tooltip'; +import { Point } from '../../types'; +import { GetLayerType } from '../../factory'; +import { HIGHLIGHT_COLOR_ARRAY } from '../../utils'; +import { BLACK_COLOR, PRIMARY_COLOR } from '../../utilities/controls'; + +type ProcessedFeature = Feature<Geometry, GeoJsonProperties> & { + properties: JsonObject; + extraProps?: JsonObject; +}; + +const propertyMap = { + fillColor: 'fillColor', + color: 'fillColor', + fill: 'fillColor', + 'fill-color': 'fillColor', + strokeColor: 'strokeColor', + 'stroke-color': 'strokeColor', + 'stroke-width': 'strokeWidth', +}; + +const alterProps = (props: JsonObject, propOverrides: JsonObject) => { + const newProps: JsonObject = {}; + Object.keys(props).forEach(k => { + if (k in propertyMap) { + newProps[propertyMap[k as keyof typeof propertyMap]] = props[k]; + } else { + newProps[k] = props[k]; + } + }); + if (typeof props.fillColor === 'string') { + newProps.fillColor = hexToRGB(props.fillColor); + } + if (typeof props.strokeColor === 'string') { + newProps.strokeColor = hexToRGB(props.strokeColor); + } + + return { + ...newProps, + ...propOverrides, + }; +}; +let features: ProcessedFeature[] = []; +const recurseGeoJson = ( + node: JsonObject, + propOverrides: JsonObject, + extraProps?: JsonObject, +) => { + if (node?.features) { + node.features.forEach((obj: JsonObject) => { + recurseGeoJson(obj, propOverrides, node.extraProps || extraProps); + }); + } + if (node?.geometry) { + const newNode = { + ...node, + properties: alterProps(node.properties, propOverrides), + } as ProcessedFeature; + if (!newNode.extraProps) { + newNode.extraProps = extraProps; + } + features.push(newNode); + } +}; + +function setTooltipContent(o: JsonObject) { + return ( + o.object?.extraProps && ( + <div className="deckgl-tooltip"> + {Object.keys(o.object.extraProps).map((prop, index) => ( + <TooltipRow + key={`prop-${index}`} + label={`${prop}: `} + value={`${o.object.extraProps?.[prop]}`} + /> + ))} + </div> + ) + ); +} + +const getFillColor = (feature: JsonObject, filterStateValue: unknown[]) => { + if (filterStateValue) { + if ( + JSON.stringify(feature.geometry.coordinates) === + JSON.stringify(filterStateValue?.[0]) + ) { + return HIGHLIGHT_COLOR_ARRAY; + } + + const fillColor = feature?.properties?.fillColor; + fillColor[3] = 125; + return fillColor; + } + return feature?.properties?.fillColor; Review Comment: **Suggestion:** The `getFillColor` helper assumes `feature.properties.fillColor` is always a defined array and mutates it in place, which can throw at runtime when it is undefined and also permanently changes the alpha channel so non-selected features stay semi-transparent even after clearing the filter. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ GeoJSON map crashes when fill color missing and filtered. - ⚠️ Non-selected features stay dim after clearing cross filters. ``` </details> ```suggestion const baseColor = feature?.properties?.fillColor; if (filterStateValue) { if ( JSON.stringify(feature.geometry.coordinates) === JSON.stringify(filterStateValue?.[0]) ) { return HIGHLIGHT_COLOR_ARRAY; } if (Array.isArray(baseColor) && baseColor.length >= 4) { return [baseColor[0], baseColor[1], baseColor[2], 125]; } return baseColor ?? HIGHLIGHT_COLOR_ARRAY; } return baseColor; ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render `DeckGLGeoJson` (superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/Geojson.tsx:366) as part of a deck.gl GeoJSON maplibre chart so that `getLayer` (lines 250–338) is called. 2. Configure the chart so that at least one feature in `payload.data` has no effective `fillColor`: either the GeoJSON `properties` omit `fillColor` and `formData.fill_color_picker` has alpha=0, causing `propOverrides.fillColor` not to be set in `getLayer` (around line 262). 3. Apply a native filter or cross-filter that sets `filterState.value` to a non-empty array for this chart; `getLayer` passes `filterState.value` into `getFillColor` via `getFillColor: (feature: JsonObject) => getFillColor(feature, filterState?.value)` (around lines 314–321). 4. When `getFillColor` executes (lines 125–139), `feature?.properties?.fillColor` is `undefined` for the affected feature; `const fillColor = feature?.properties?.fillColor; fillColor[3] = 125;` attempts to write index `3` on `undefined`, throwing a runtime TypeError and breaking the GeoJSON layer rendering. 5. In a separate but related scenario, render the same chart with a normal, opaque base fill color (so `feature.properties.fillColor` is a valid `[r,g,b,255]` array produced by `alterProps` at lines 65–85). 6. Apply a cross-filter so `filterState.value` becomes non-empty; during this render, for every non-selected feature, `getFillColor` mutates `feature.properties.fillColor[3] = 125` (lines 134–135), permanently changing the stored alpha on the `ProcessedFeature` objects accumulated by `recurseGeoJson` (lines 86–107). 7. Clear the filter so `filterState.value` becomes falsy and the component re-renders; `getFillColor` now returns `feature?.properties?.fillColor` directly (line 138), but this value already has alpha=125 from the prior mutation, so previously filtered features remain semi-transparent instead of returning to full opacity. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/Geojson.tsx **Line:** 126:138 **Comment:** *Logic Error: The `getFillColor` helper assumes `feature.properties.fillColor` is always a defined array and mutates it in place, which can throw at runtime when it is undefined and also permanently changes the alpha channel so non-selected features stay semi-transparent even after clearing the filter. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=3583aea89c87d95c0ae3185b0a54b1fb5fba87f7656ea795b052039da3022912&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=3583aea89c87d95c0ae3185b0a54b1fb5fba87f7656ea795b052039da3022912&reaction=dislike'>👎</a> ########## .github/workflows/bashlib.sh: ########## @@ -45,6 +45,7 @@ npm-install() { say "::group::Install npm packages" echo "npm: $(npm --version)" echo "node: $(node --version)" + export CYPRESS_INSTALL_BINARY=0 npm ci Review Comment: **Suggestion:** Exporting the Cypress environment variable globally causes it to persist for the entire workflow, so later `npm ci` invocations (e.g. in the Cypress install step) will also skip downloading the Cypress binary, which can break Cypress-based tests; the variable should instead be scoped only to this single `npm ci` call. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Cypress cache install step skips binary unexpectedly. - ❌ E2E runs may lack preinstalled Cypress binary. - ⚠️ CI caching for Cypress becomes unreliable and misleading. - ⚠️ Local dev workflows using both functions behave inconsistently. ``` </details> ```suggestion CYPRESS_INSTALL_BINARY=0 npm ci ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. From the repository root, start an interactive shell and source the script to load its functions: `. .github/workflows/bashlib.sh` (file `.github/workflows/bashlib.sh`). 2. In the same shell, run `npm-install` (function defined around lines 40–53 in `.github/workflows/bashlib.sh`), which executes: `export CYPRESS_INSTALL_BINARY=0` and then `npm ci` at lines 48–49, leaving `CYPRESS_INSTALL_BINARY=0` exported in the shell environment (`echo "$CYPRESS_INSTALL_BINARY"` prints `0`). 3. Still in the same shell process, run `cypress-install` (function defined later in the same file, around lines 110–123), which changes directory to `superset-frontend/cypress-base` and runs `npm ci` without overriding `CYPRESS_INSTALL_BINARY`; this `npm ci` therefore also sees `CYPRESS_INSTALL_BINARY=0` and skips downloading the Cypress binary during install. 4. Finally, in the same environment, execute `cypress-run-all` (function in `.github/workflows/bashlib.sh` around lines 125–165) or otherwise invoke Cypress from `superset-frontend/cypress-base`; Cypress either fails because the expected binary is missing or must re-download it at runtime, contradicting the purpose of the dedicated `cypress-install` + cache-restore/cache-save flow. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** .github/workflows/bashlib.sh **Line:** 48:49 **Comment:** *Logic Error: Exporting the Cypress environment variable globally causes it to persist for the entire workflow, so later `npm ci` invocations (e.g. in the Cypress install step) will also skip downloading the Cypress binary, which can break Cypress-based tests; the variable should instead be scoped only to this single `npm ci` call. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=fcf9c0ebc9766769989b3bee4ca0262e0a52b656b27209adf36b619874c8328e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=fcf9c0ebc9766769989b3bee4ca0262e0a52b656b27209adf36b619874c8328e&reaction=dislike'>👎</a> ########## superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx: ########## @@ -0,0 +1,414 @@ +/* eslint-disable react/jsx-handler-names */ +/* eslint-disable react/no-access-state-in-setstate */ +/* eslint-disable camelcase */ +/** + * 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 { memo, useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { useSelector } from 'react-redux'; +import { isEqual } from 'lodash'; +import { createSelector } from '@reduxjs/toolkit'; +import { + AdhocFilter, + ContextMenuFilters, + DataMask, + Datasource, + ensureIsArray, + ExtraFormData, + FilterState, + HandlerFunction, + isDefined, + JsonObject, + JsonValue, + QueryFormData, + QueryObjectFilterClause, + SupersetClient, + usePrevious, +} from '@superset-ui/core'; +import { styled } from '@apache-superset/core/ui'; +import { Layer } from '@deck.gl/core'; + +import { + DeckGLContainerHandle, + DeckGLContainerStyledWrapper, +} from '../DeckGLContainer'; +import { getExploreLongUrl } from '../utils/explore'; +import layerGenerators from '../layers'; +import fitViewport, { Viewport } from '../utils/fitViewport'; +import { TooltipProps } from '../components/Tooltip'; + +import { getPoints as getPointsArc } from '../layers/Arc/Arc'; +import { getPoints as getPointsPath } from '../layers/Path/Path'; +import { getPoints as getPointsPolygon } from '../layers/Polygon/Polygon'; +import { getPoints as getPointsGrid } from '../layers/Grid/Grid'; +import { getPoints as getPointsScatter } from '../layers/Scatter/Scatter'; +import { getPoints as getPointsContour } from '../layers/Contour/Contour'; +import { getPoints as getPointsHeatmap } from '../layers/Heatmap/Heatmap'; +import { getPoints as getPointsHex } from '../layers/Hex/Hex'; +import { getPoints as getPointsGeojson } from '../layers/Geojson/Geojson'; +import { getPoints as getPointsScreengrid } from '../layers/Screengrid/Screengrid'; + +type DataMaskState = Record< + string, + DataMask & { + extraFormData?: ExtraFormData & { visible_deckgl_layers?: number[] }; + } +>; + +export type DeckMultiProps = { + formData: QueryFormData; + payload: JsonObject; + setControlValue: (control: string, value: JsonValue) => void; + viewport: Viewport; + onAddFilter: HandlerFunction; + height: number; + width: number; + datasource: Datasource; + setDataMask?: (dataMask: DataMask) => void; + onContextMenu?: ( + clientX: number, + clientY: number, + filters?: ContextMenuFilters, + ) => void; + onSelect: () => void; + filterState?: FilterState; + emitCrossFilters?: boolean; +}; + +const MultiWrapper = styled.div<{ height: number; width: number }>` + position: relative; + height: ${({ height }) => height}px; + width: ${({ width }) => width}px; +`; + +const selectDataMask = createSelector( + (state: { dataMask?: DataMaskState }) => state.dataMask, + dataMask => dataMask || {}, +); + +const DeckMulti = (props: DeckMultiProps) => { + const containerRef = useRef<DeckGLContainerHandle>(); + + const dataMask = useSelector(selectDataMask); + + const layerVisibilityFilter = Object.values(dataMask).find( + mask => mask?.extraFormData?.visible_deckgl_layers !== undefined, + ); + + const visibleDeckLayersFromRedux = + layerVisibilityFilter?.extraFormData?.visible_deckgl_layers; + + const getAdjustedViewport = useCallback(() => { + let viewport = { ...props.viewport }; + + // Default to autozoom enabled for backward compatibility (undefined treated as true) + if (props.formData.autozoom !== false) { + const points = [ + ...getPointsPolygon(props.payload.data.features.deck_polygon || []), + ...getPointsPath(props.payload.data.features.deck_path || []), + ...getPointsGrid(props.payload.data.features.deck_grid || []), + ...getPointsScatter(props.payload.data.features.deck_scatter || []), + ...getPointsContour(props.payload.data.features.deck_contour || []), + ...getPointsHeatmap(props.payload.data.features.deck_heatmap || []), + ...getPointsHex(props.payload.data.features.deck_hex || []), + ...getPointsArc(props.payload.data.features.deck_arc || []), + ...getPointsGeojson(props.payload.data.features.deck_geojson || []), + ...getPointsScreengrid( + props.payload.data.features.deck_screengrid || [], + ), + ]; + + if (props.formData && points.length > 0) { + viewport = fitViewport(viewport, { + width: props.width, + height: props.height, + points, + }); + } + } + + if (viewport.zoom < 0) { + viewport.zoom = 0; + } + return viewport; + }, [props]); + + const [viewport, setViewport] = useState<Viewport>(getAdjustedViewport()); + const [subSlicesLayers, setSubSlicesLayers] = useState<Record<number, Layer>>( + {}, + ); + const [layerOrder, setLayerOrder] = useState<number[]>([]); + + const setTooltip = useCallback((tooltip: TooltipProps['tooltip']) => { + const { current } = containerRef; + if (current) { + current.setTooltip(tooltip); + } + }, []); + + const getLayerIndex = useCallback( + (sliceId: number, payloadIndex: number, deckSlices?: number[]): number => + deckSlices ? deckSlices.indexOf(sliceId) : payloadIndex, + [], + ); + + const processLayerFilters = useCallback( + ( + subslice: JsonObject, + formData: QueryFormData, + layerIndex: number, + ): { + extraFilters: (AdhocFilter | QueryObjectFilterClause)[]; + adhocFilters: AdhocFilter[]; + } => { + const layerFilterScope = formData.layer_filter_scope; + + const extraFilters: (AdhocFilter | QueryObjectFilterClause)[] = [ + ...(subslice.form_data.extra_filters || []), + ...(formData.extra_filters || []), + ]; + + const adhocFilters: AdhocFilter[] = [ + ...(subslice.form_data?.adhoc_filters || []), + ]; + + if (layerFilterScope) { + const filterDataMapping = formData.filter_data_mapping || {}; + let shouldAddDashboardAdhocFilters = false; + + Object.entries(layerFilterScope).forEach( + ([filterId, filterScope]: [string, number[]]) => { + const shouldApplyFilter = + ensureIsArray(filterScope).includes(layerIndex); + + if (shouldApplyFilter) { + shouldAddDashboardAdhocFilters = true; + const filtersFromThisFilter = filterDataMapping[filterId] || []; + extraFilters.push(...filtersFromThisFilter); + } + }, + ); + + if (shouldAddDashboardAdhocFilters) { + const dashboardAdhocFilters = formData.adhoc_filters || []; + adhocFilters.push(...dashboardAdhocFilters); + } + } else { + const originalExtraFormDataFilters = + formData.extra_form_data?.filters || []; + extraFilters.push(...originalExtraFormDataFilters); + + const dashboardAdhocFilters = formData.adhoc_filters || []; + adhocFilters.push(...dashboardAdhocFilters); + } + + return { extraFilters, adhocFilters }; + }, + [], + ); + + const createLayerFromData = useCallback( + (subslice: JsonObject, json: JsonObject): Layer => + // @ts-expect-error TODO(hainenber): define proper type for `form_data.viz_type` and call signature for functions in layerGenerators. + layerGenerators[subslice.form_data.viz_type]({ + formData: subslice.form_data, + payload: json, + setTooltip, + datasource: props.datasource, + onSelect: props.onSelect, + }), + [props.onSelect, props.datasource, setTooltip], + ); + + const loadSingleLayer = useCallback( + ( + subslice: JsonObject, + formData: QueryFormData, + payloadIndex: number, + ): void => { + const layerIndex = getLayerIndex( + subslice.slice_id, + payloadIndex, + formData.deck_slices, + ); + let extraFilters: (AdhocFilter | QueryObjectFilterClause)[] = []; + let adhocFilters: AdhocFilter[] = []; + const isExplore = (window.location.href || '').includes('explore'); + if (isExplore) { + // in explore all the filters are in the adhoc_filters + const adhocFiltersFromFormData = formData.adhoc_filters || []; + const finalAdhocFilters = adhocFiltersFromFormData + .map((filter: AdhocFilter & { layerFilterScope?: number[] }) => { + if (!isDefined(filter?.layerFilterScope)) { + return filter; + } + if ( + Array.isArray(filter.layerFilterScope) && + filter.layerFilterScope.length > 0 + ) { + if (filter.layerFilterScope.includes(-1)) { + return filter; + } + if (filter.layerFilterScope.includes(layerIndex)) { + return filter; + } + } + return undefined; + }) + .filter(filter => isDefined(filter)); + adhocFilters = finalAdhocFilters as AdhocFilter[]; + } else { + const { + extraFilters: processLayerFiltersResultExtraFilters, + adhocFilters: processLayerFiltersResultAdhocFilters, + } = processLayerFilters(subslice, formData, layerIndex); + extraFilters = processLayerFiltersResultExtraFilters; + adhocFilters = processLayerFiltersResultAdhocFilters; + } + + const subsliceCopy = { + ...subslice, + form_data: { + ...subslice.form_data, + extra_filters: extraFilters, + adhoc_filters: adhocFilters, + // Preserve dashboard context for embedded mode permissions + ...(formData.dashboardId && { dashboardId: formData.dashboardId }), + }, + } as any as JsonObject & { slice_id: number }; + + const url = getExploreLongUrl(subsliceCopy.form_data, 'json'); + + if (url) { + SupersetClient.get({ endpoint: url }) + .then(({ json }) => { + const layer = createLayerFromData(subsliceCopy, json); + setSubSlicesLayers(subSlicesLayers => ({ + ...subSlicesLayers, + [subsliceCopy.slice_id]: layer, + })); + }) + .catch(error => { + throw new Error( + `Error loading layer for slice ${subsliceCopy.slice_id}: ${error}`, + ); + }); + } + }, + [getLayerIndex, processLayerFilters, createLayerFromData], + ); + + const loadLayers = useCallback( + ( + formData: QueryFormData, + payload: JsonObject, + visibleLayers?: number[], + ): void => { + setViewport(getAdjustedViewport()); + setSubSlicesLayers({}); + + let visibleDeckLayers = visibleLayers; + + if (!visibleDeckLayers) { + visibleDeckLayers = ( + formData.extra_form_data as ExtraFormData & { + visible_deckgl_layers?: number[]; + } + )?.visible_deckgl_layers; + } + + const deckSlicesOrder = formData.deck_slices || []; + + payload.data.slices.forEach( + (subslice: { slice_id: number } & JsonObject, payloadIndex: number) => { + if (visibleDeckLayers && Array.isArray(visibleDeckLayers)) { + if (!visibleDeckLayers.includes(subslice.slice_id)) { + return; + } + } + + loadSingleLayer(subslice, formData, payloadIndex); + }, + ); + + const orderedSliceIds = deckSlicesOrder.filter((sliceId: number) => { + const subslice = payload.data.slices.find( + (s: { slice_id: number }) => s.slice_id === sliceId, + ); + if (!subslice) return false; + if (visibleDeckLayers && Array.isArray(visibleDeckLayers)) { + return visibleDeckLayers.includes(sliceId); + } + return true; + }); + + setLayerOrder(orderedSliceIds); + }, + [getAdjustedViewport, loadSingleLayer], + ); + + const prevDeckSlices = usePrevious(props.formData.deck_slices); + const prevVisibleLayersRedux = usePrevious(visibleDeckLayersFromRedux); + + useEffect(() => { + const { formData, payload } = props; + + const deckSlicesChanged = !isEqual(prevDeckSlices, formData.deck_slices); + const visibilityFilterChanged = !isEqual( + prevVisibleLayersRedux, + visibleDeckLayersFromRedux, + ); + + if (deckSlicesChanged || visibilityFilterChanged) { + loadLayers(formData, payload, undefined); + } + }, [ + loadLayers, + prevDeckSlices, + prevVisibleLayersRedux, + visibleDeckLayersFromRedux, + props, + ]); Review Comment: **Suggestion:** When the visible deck.gl layer set in Redux (`visible_deckgl_layers` inside `dataMask`) changes, `loadLayers` is called but the new visibility list is not passed into it, so the component continues to use the static visibility from `formData.extra_form_data` and ignores runtime visibility changes. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Redux-driven layer visibility changes not reflected in chart. - ⚠️ Multi-layer MapLibre deck.gl dashboards show stale layers. ``` </details> ```suggestion useEffect(() => { const { formData, payload } = props; const deckSlicesChanged = !isEqual(prevDeckSlices, formData.deck_slices); const visibilityFilterChanged = !isEqual( prevVisibleLayersRedux, visibleDeckLayersFromRedux, ); if (deckSlicesChanged || visibilityFilterChanged) { loadLayers(formData, payload, visibleDeckLayersFromRedux); } }, [ loadLayers, prevDeckSlices, prevVisibleLayersRedux, visibleDeckLayersFromRedux, props, ]); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create or open a chart that uses the `DeckMulti` component at `superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx:104` (the new MapLibre multi-layer deck.gl viz type). 2. Configure the chart with multiple deck.gl subslices so that `formData.deck_slices` and `payload.data.slices` are populated (loaded via the standard `ChartProps` pipeline in `superset-frontend/packages/superset-ui-core/src/chart/models/ChartProps.ts:117-294`). 3. Use a control or native filter that updates layer visibility via Redux `dataMask.extraFormData.visible_deckgl_layers`, which `DeckMulti` reads into `visibleDeckLayersFromRedux` at `Multi.tsx:96-112` through `selectDataMask`. 4. When `visibleDeckLayersFromRedux` changes, the `useEffect` at `Multi.tsx:368-386` detects the change (`visibilityFilterChanged` becomes true) and calls `loadLayers(formData, payload, undefined)` instead of passing `visibleDeckLayersFromRedux`. 5. Inside `loadLayers` at `Multi.tsx:320-360`, `visibleLayers` is `undefined`, so it falls back to `formData.extra_form_data.visible_deckgl_layers` and ignores the updated Redux visibility from `dataMask`, resulting in the same set of layers being loaded despite the visibility change. 6. In the UI, toggling layer visibility via the Redux-driven mechanism has no effect on which subslices are rendered, confirming that runtime visibility changes are being ignored. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx **Line:** 368:386 **Comment:** *Logic Error: When the visible deck.gl layer set in Redux (`visible_deckgl_layers` inside `dataMask`) changes, `loadLayers` is called but the new visibility list is not passed into it, so the component continues to use the static visibility from `formData.extra_form_data` and ignores runtime visibility changes. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=fedcc84347549d8a2df8e508e05434b3ac06d2c312ee11d657e7015e2ad08f78&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=fedcc84347549d8a2df8e508e05434b3ac06d2c312ee11d657e7015e2ad08f78&reaction=dislike'>👎</a> -- 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]
